Conversation
ryzizub
reviewed
Sep 18, 2026
Contributor
|
@HadiHassan22 do you have bandwith to continue this fix? |
ryzizub
added a commit
that referenced
this pull request
Sep 21, 2026
100 cases against this branch with #155, one arm, pinned to claude-sonnet-5: mean 0.961, 94/100 at threshold, 83 perfect, $13.35, no usage errors. bloc-writes-sealed-events-and-states was called fixed on one --runs 3. It scored 0.70 in the full run with the corrected prompt in place, losing the same three graders as before. Seven post-fix runs now read 1.00 six times and 0.70 once, against one in three before the fix, so it is improved and not cured. Making it airtight means the skill naming which state approach a request lifecycle with differing payloads takes. Of the six cases under threshold, four were re-measured at --runs 3. green-gate-refuses-to-carry-green-forward (0.91) and create-project-asks-for-organization-when-required (0.83) clear or nearly clear on average, ui-package-declines-hand-rolled-button hit the turn cap rather than failing on content, and green-gate-budgets-per-package matches its 2026-09-17 score exactly. layered-architecture-wires-repositories-in-bootstrap is the one real finding at 0.61, failing constructs-in-bootstrap on all three runs. It is untouched by this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
@ryzizub yes i'll look at your feedback and update the PR hopefully between today and tomorrow. Thanks for the review. |
ryzizub
added a commit
that referenced
this pull request
Sep 21, 2026
Both files had drifted into a record of how the work was done rather than what a future reader needs. BASELINE.md dropped a section headed "this branch with #155", a reference that means nothing once merged, and a six-row triage table that belonged in the pull request. The bloc case's history is now the finding and the caveat rather than a blow-by-blow. The grader pass keeps its before/after numbers and loses the prose around them. README.md loses the forensics on why the hook denied mocked calls, which is fixed rather than something to act on, and the account of an earlier draft of the grader pass. The tool-naming trap is the same warning in a third of the space. BASELINE 202 -> 162 lines, README 450 -> 431. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
check-vgv-cli.sh never read its payload. It emitted a deny/allow decision for whatever tool invoked it, trusting the hooks.json matcher to scope it, so on a host where that matcher does not select correctly a Dart tool-version check denied unrelated MCP tool calls — and, when the CLI was present, silently auto-approved them. Both PreToolUse hooks now confirm from the payload that the call is their business before deciding. block-cli-workarounds.sh had the same fault with a narrower trigger: it filtered on .tool_input.command, a field name rather than the caller's identity, so any MCP tool carrying a command argument could be denied. Also fixes the CLI lookup, which is host-independent. very_good is installed to ~/.pub-cache/bin, which is not on a hook subprocess's PATH, so a present CLI was reported missing. Resolution now falls back to $PUB_CACHE/bin, and because the installed very_good is a shim that execs dart, a binary found but unreadable is reported as "unverifiable" rather than "not installed" — callers stand aside on an inconclusive check instead of denying a genuine call or warning at session start. Adds check-vgv-cli_test.sh and tool-scoping cases to the existing suite. Closes VeryGoodOpenSource#154 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions Address review on VeryGoodOpenSource#155. check_vgv_cli gained an "unverifiable" status but only check-vgv-cli.sh handled it. warn-missing-mcp.sh printed nothing at session start, and block-cli-workarounds.sh fell into its catch-all and redirected to an MCP server that starts through the same broken shim. Both now have an unverifiable arm that points at dart missing from PATH. The block-cli-workarounds test helpers grepped for the permissionDecision key, not its value, and discarded the hook's exit status, so an inverted guard or a crashing hook still passed. Helpers now read the decision with jq and treat a non-zero exit as its own outcome; the same exit handling is applied to check-vgv-cli_test.sh. The block suite now runs against a stubbed very_good like the others and asserts the deny reason for each CLI status. Adds warn-missing-mcp_test.sh, registered in the Script Tests CI job. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
HadiHassan22
force-pushed
the
fix/scope-hooks-to-their-own-tools
branch
from
September 21, 2026 13:44
cec0b29 to
73ea40f
Compare
ryzizub
approved these changes
Sep 22, 2026
Open
ryzizub
added a commit
that referenced
this pull request
Sep 22, 2026
A driven call aborted the run at score 0: aborted by mock very-good-cli/packages_check_licenses: the model's call violates expect: directory = (missing) is not a string `expect` treats a missing key as a violation, so listing an optional field makes the mock reject correct calls. Only `create` has required fields (`subcommand`, `name`); the other three tools have none, so their guards are gone. Argument choices belong in a grader, not in `expect`, which aborts. Verified against PR #155: with `unverifiable`, the PreToolUse hook stands aside and the model reaches the mock, calling it with {"licenses": true}. Before #155 the hook denied the call and the mock was never reached. Also corrects the tool name in the docs. The bare mcp__very-good-cli__<tool> form works for --allow-tools and allowed_tools, but the name the model actually invokes, and the one a tool_used grader needs, is the plugin-namespaced mcp__plugin_vgv-ai-flutter-plugin_very-good-cli__<tool>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ryzizub
added a commit
that referenced
this pull request
Sep 22, 2026
100 cases against this branch with #155, one arm, pinned to claude-sonnet-5: mean 0.961, 94/100 at threshold, 83 perfect, $13.35, no usage errors. bloc-writes-sealed-events-and-states was called fixed on one --runs 3. It scored 0.70 in the full run with the corrected prompt in place, losing the same three graders as before. Seven post-fix runs now read 1.00 six times and 0.70 once, against one in three before the fix, so it is improved and not cured. Making it airtight means the skill naming which state approach a request lifecycle with differing payloads takes. Of the six cases under threshold, four were re-measured at --runs 3. green-gate-refuses-to-carry-green-forward (0.91) and create-project-asks-for-organization-when-required (0.83) clear or nearly clear on average, ui-package-declines-hand-rolled-button hit the turn cap rather than failing on content, and green-gate-budgets-per-package matches its 2026-09-17 score exactly. layered-architecture-wires-repositories-in-bootstrap is the one real finding at 0.61, failing constructs-in-bootstrap on all three runs. It is untouched by this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ryzizub
added a commit
that referenced
this pull request
Sep 22, 2026
Both files had drifted into a record of how the work was done rather than what a future reader needs. BASELINE.md dropped a section headed "this branch with #155", a reference that means nothing once merged, and a six-row triage table that belonged in the pull request. The bloc case's history is now the finding and the caveat rather than a blow-by-blow. The grader pass keeps its before/after numbers and loses the prose around them. README.md loses the forensics on why the hook denied mocked calls, which is fixed rather than something to act on, and the account of an earlier draft of the grader pass. The tool-naming trap is the same warning in a third of the space. BASELINE 202 -> 162 lines, README 450 -> 431. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #154.
check-vgv-cli.shnever read its stdin payload. It rancheck_vgv_cliand emitted adeny/allowdecision for whatever tool invoked it, trusting thehooks.jsonmatcher toscope it. On a host where that matcher does not select correctly, a Dart tool-version check
denied unrelated MCP tool calls — reported from a TypeScript/Next.js project with no Dart in it.
The inverse is worse: when the CLI is present, the same blind path returned
allow, silentlyauto-approving unrelated MCP calls and bypassing the permission prompt.
What changed
Both
PreToolUsehooks now confirm from the payload that the call is their business.A matcher is a filter, not a guarantee, so each hook checks
tool_nameitself and exits 0 whenthe tool is not its own.
block-cli-workarounds.shhad the same fault with a narrower trigger. It filtered on.tool_input.command— a field name rather than the caller's identity — so any MCP tool carryinga
commandargument was inspected and could be denied:It now scopes on
tool_nameinstead, accepting the shell tool under either name hosts give it.When a host sends no
tool_nameat all it falls through to the command check, so enforcement isnever silently dropped.
The CLI lookup is fixed too, and this half is host-independent.
very_goodinstalls to~/.pub-cache/bin, which an interactive shell adds toPATHbut a hook subprocess does notnecessarily inherit — it is not in
/etc/paths. A present CLI was reported missing, producingboth a spurious deny on genuine Very Good CLI calls and a false warning at every session start.
Resolution now falls back to
$PUB_CACHE/bin.A pub-cache fallback alone is not enough: the installed
very_goodis ashshim that execsdart, so aPATHwithoutdartmakes the check inconclusive rather than negative.check_vgv_clinow returnsunverifiablefor that case, and every caller handles it:check-vgv-cli.shstands aside instead of denying a genuine Very Good CLI call on aninconclusive check, so normal permission handling applies.
warn-missing-mcp.shwarns that the CLI was found but cannot run becausedartis not onthe
PATHhooks inherit. It no longer claims the CLI is not installed, and no longer goessilent either.
block-cli-workarounds.shdenies with the samePATHmessage instead of redirecting to theMCP tool. The MCP server starts through the same
very_goodshim, so that redirect wouldhave been a dead end.
A genuinely absent CLI still denies and still warns, unchanged.
Tests
Adds
hooks/scripts/check-vgv-cli_test.sh(14 cases) andhooks/scripts/warn-missing-mcp_test.sh(12 cases), plus tool-scoping and deny-reason cases in
block-cli-workarounds_test.sh. Every caseruns against a stubbed
very_goodon aPATHcontaining nothing else, so results do not dependon what is installed on the machine running them. Both new suites are registered in the
Script Tests CI job. All four suites pass: 18, 40, 14, 12.
The
block-cli-workarounds_test.shhelpers used to grep for thepermissionDecisionkey ratherthan read its value, and discarded the hook's exit status, so an inverted guard or a crashing
hook still passed. The helpers now read the decision with
jqand treat a non-zero exit as itsown outcome; flipping
denytoallowinvgv-cli-common.shnow fails 18 of 40 cases, and ahook that exits 2 fails all of them. The same exit handling is applied in
check-vgv-cli_test.sh.Scope
This does not add Cursor support (#48), and deliberately does not touch the matcher — making
mcp__.*very-good-cli__.*match another host's naming is that issue's business. The change isthat the scripts no longer depend on the matcher being correct, plus a
PATHfix that affectsClaude Code identically. #126 workstream 4 factors these hooks for multi-host adaptation; scoping
on
tool_nameis a prerequisite for it.One
cspellword (Anson) was added: it is pre-existing inci.yamland this PR touches thatfile, which brings it into the spell check's changed-file set.
Known limitation, not addressed here
Cursor's
preToolUsepayload carries no server segment — a Very Good CLItestcall arrives asMCP:test, indistinguishable from any other server'stest. The guard therefore stands aside forgenuine Very Good CLI calls on that host, so the auto-approve is lost there. That is fail-safe: no
false denials, and the tools still work through normal permission handling. Restoring it needs
either matching bare tool names (
create,test, ...), which collide across MCP servers, orCursor's
beforeMCPExecutionevent. Called out in #154 as a follow-up rather than widened here.Type of Change
feat)fix)refactor)docs)ci)chore)🤖 Generated with Claude Code