Skip to content

fix: scope PreToolUse hooks to their own tools - #155

Merged
ryzizub merged 2 commits into
VeryGoodOpenSource:mainfrom
HadiHassan22:fix/scope-hooks-to-their-own-tools
Sep 22, 2026
Merged

ryzizub merged 2 commits into
VeryGoodOpenSource:mainfrom
HadiHassan22:fix/scope-hooks-to-their-own-tools

Conversation

@HadiHassan22

@HadiHassan22 HadiHassan22 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #154.

check-vgv-cli.sh never read its stdin payload. It ran check_vgv_cli and emitted a
deny/allow decision for whatever tool invoked it, trusting the hooks.json matcher to
scope 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, silently
auto-approving unrelated MCP calls and bypassing the permission prompt.

What changed

Both PreToolUse hooks now confirm from the payload that the call is their business.
A matcher is a filter, not a guarantee, so each hook checks tool_name itself and exits 0 when
the tool is not its own.

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 was inspected and could be denied:

# before
$ echo '{"tool_name":"MCP:run_terminal_cmd","tool_input":{"command":"flutter test"}}' | bash hooks/scripts/block-cli-workarounds.sh
{"hookSpecificOutput":{"permissionDecision":"deny", ...}}

# after — stands aside

It now scopes on tool_name instead, accepting the shell tool under either name hosts give it.
When a host sends no tool_name at all it falls through to the command check, so enforcement is
never silently dropped.

The CLI lookup is fixed too, and this half is host-independent. very_good installs to
~/.pub-cache/bin, which an interactive shell adds to PATH but a hook subprocess does not
necessarily inherit — it is not in /etc/paths. A present CLI was reported missing, producing
both 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_good is a sh shim that execs
dart, so a PATH without dart makes the check inconclusive rather than negative.

$ env -i HOME=$HOME PATH=/usr/bin:/bin:/usr/sbin:/sbin ~/.pub-cache/bin/very_good --version
~/.pub-cache/bin/very_good: line 8: dart: command not found   (exit 127)

check_vgv_cli now returns unverifiable for that case, and every caller handles it:

  • check-vgv-cli.sh stands aside instead of denying a genuine Very Good CLI call on an
    inconclusive check, so normal permission handling applies.
  • warn-missing-mcp.sh warns that the CLI was found but cannot run because dart is not on
    the PATH hooks inherit. It no longer claims the CLI is not installed, and no longer goes
    silent either.
  • block-cli-workarounds.sh denies with the same PATH message instead of redirecting to the
    MCP tool. The MCP server starts through the same very_good shim, so that redirect would
    have 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) and hooks/scripts/warn-missing-mcp_test.sh
(12 cases), plus tool-scoping and deny-reason cases in block-cli-workarounds_test.sh. Every case
runs against a stubbed very_good on a PATH containing nothing else, so results do not depend
on 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.sh helpers used to grep for the permissionDecision key rather
than 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 jq and treat a non-zero exit as its
own outcome; flipping deny to allow in vgv-cli-common.sh now fails 18 of 40 cases, and a
hook 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 is
that the scripts no longer depend on the matcher being correct, plus a PATH fix that affects
Claude Code identically. #126 workstream 4 factors these hooks for multi-host adaptation; scoping
on tool_name is a prerequisite for it.

One cspell word (Anson) was added: it is pre-existing in ci.yaml and this PR touches that
file, which brings it into the spell check's changed-file set.

Known limitation, not addressed here

Cursor's preToolUse payload carries no server segment — a Very Good CLI test call arrives as
MCP:test, indistinguishable from any other server's test. The guard therefore stands aside for
genuine 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, or
Cursor's beforeMCPExecution event. Called out in #154 as a follow-up rather than widened here.

Type of Change

  • New feature (feat)
  • Bug fix (fix)
  • Code refactor (refactor)
  • Documentation (docs)
  • CI change (ci)
  • Chore (chore)

🤖 Generated with Claude Code

@HadiHassan22
HadiHassan22 requested a review from a team as a code owner September 13, 2026 17:08
@ryzizub ryzizub self-assigned this Sep 14, 2026
Comment thread hooks/scripts/vgv-cli-common.sh
Comment thread hooks/scripts/block-cli-workarounds_test.sh Outdated
@ryzizub

ryzizub commented Sep 21, 2026

Copy link
Copy Markdown
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>
@HadiHassan22

Copy link
Copy Markdown
Contributor Author

@HadiHassan22 do you have bandwith to continue this fix?

@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>
HadiHassan22 and others added 2 commits September 21, 2026 16:40
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
HadiHassan22 force-pushed the fix/scope-hooks-to-their-own-tools branch from cec0b29 to 73ea40f Compare September 21, 2026 13:44
@ryzizub
ryzizub merged commit 614fba2 into VeryGoodOpenSource:main Sep 22, 2026
5 checks passed
@vgvbot vgvbot mentioned this pull request Sep 22, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: check-vgv-cli decides for every tool, not just Very Good CLI ones

2 participants