From 7f27e7fee02e0faeb6078adf8bc2e15cdbd15fb2 Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Tue, 20 Oct 2026 14:39:26 +0200 Subject: [PATCH] fix: stop blaming PATH when the Very Good CLI version cannot be read Closes #171 Two bugs made the SessionStart hook blame PATH for a Dart SDK upgrade. `very_good --version` was read with `head -1`. When the shim's snapshot is stale it falls back to `dart pub global run`, which prints its resolution chatter to stdout ahead of the version, so the first line held no version and a working CLI read as unverifiable. Match past the first line, anchored so a version-shaped fragment of a path in that chatter cannot win: a stub whose chatter names /tmp/vgv-1.2.3/pkg reads as `outdated:1.2.3` unanchored. `unverifiable` then covered both an absent dart and an unreadable version, and all three hooks reported it as a PATH problem. Split it on `command -v dart`: `dart_missing` keeps the PATH advice, no longer naming one shell's startup file, and `unverifiable` points at the rebuild and the `/mcp` reconnect a server started before the upgrade needs. The stubs printed "very_good 1.5.0"; the real CLI prints a bare "1.5.0", so they now print that. jq reaches the hooks through a symlinked directory, because its own directory holds dart on a Homebrew machine and the dart_missing cases then ran the wrong branch; each suite refuses to run if dart resolves on its base PATH. --- CLAUDE.md | 4 +- README.md | 4 +- evals/README.md | 11 ++-- hooks/scripts/block-cli-workarounds.sh | 9 +++- hooks/scripts/block-cli-workarounds_test.sh | 31 ++++++++++- hooks/scripts/check-vgv-cli.sh | 6 +-- hooks/scripts/check-vgv-cli_test.sh | 16 +++++- hooks/scripts/vgv-cli-common.sh | 22 +++++--- hooks/scripts/warn-missing-mcp.sh | 12 +++-- hooks/scripts/warn-missing-mcp_test.sh | 59 ++++++++++++++++++++- 10 files changed, 144 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b4ad15a..41a863b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,13 +10,13 @@ The `hooks/` directory contains SessionStart, PreToolUse, and PostToolUse hooks These run **when a session begins**: -- `warn-missing-mcp.sh` — checks if Very Good CLI is installed and >= 1.5.0; outputs a warning to Claude's context if missing, outdated, or present but unable to run because `dart` is not on the `PATH` hooks inherit (non-blocking) +- `warn-missing-mcp.sh` — checks if Very Good CLI is installed and >= 1.5.0; outputs a warning to Claude's context if missing, outdated, or silent about its version (non-blocking). It names which of the two causes it is: `dart` is not on the `PATH` hooks inherit, or a Dart SDK upgrade left a stale snapshot that `very_good --version` rebuilds ### PreToolUse Hooks These run **before** a tool call is executed: -- `mcp__.*very-good-cli__.*` matcher → `check-vgv-cli.sh` — reads `tool_name` from the payload and exits 0 unless the caller is a Very Good CLI tool, so a host that does not apply the matcher cannot have the decision land on an unrelated tool; for its own tools it auto-approves the call by returning a PreToolUse `allow` decision, so it is always permitted regardless of run mode (interactive, headless, or `skipAutoPermissionPrompt`) and never dead-ends when the tool isn't on `permissions.allow`; denies with an install/upgrade message if the CLI is missing or < 1.5.0, and stands aside when the CLI is present but its version cannot be read (`dart` missing from `PATH`), leaving normal permission handling to apply. The `.*` in the matcher covers both the bare `mcp__very-good-cli__*` server (repo-root `.mcp.json`) and the plugin-namespaced `mcp__plugin__very-good-cli__*` form used when installed from a marketplace +- `mcp__.*very-good-cli__.*` matcher → `check-vgv-cli.sh` — reads `tool_name` from the payload and exits 0 unless the caller is a Very Good CLI tool, so a host that does not apply the matcher cannot have the decision land on an unrelated tool; for its own tools it auto-approves the call by returning a PreToolUse `allow` decision, so it is always permitted regardless of run mode (interactive, headless, or `skipAutoPermissionPrompt`) and never dead-ends when the tool isn't on `permissions.allow`; denies with an install/upgrade message if the CLI is missing or < 1.5.0, and stands aside when the CLI is present but its version cannot be read, for either reason, leaving normal permission handling to apply. The `.*` in the matcher covers both the bare `mcp__very-good-cli__*` server (repo-root `.mcp.json`) and the plugin-namespaced `mcp__plugin__very-good-cli__*` form used when installed from a marketplace - `Bash` matcher → `block-cli-workarounds.sh` — denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` in the shell and points to the MCP tool. Quoted text is data unless `eval`, `sh -c` or `$( )` runs it. A heredoc body with a quoted delimiter (`<<'EOF'`) is data too, unless the opening line feeds it to a shell (`bash <<'EOF'`, `cat <<'EOF' | sh`), which runs it; an unquoted `<.md`. -The mocks are reachable only when `check_vgv_cli` returns `unverifiable`. In a run +The mocks are reachable only when `check_vgv_cli` comes back inconclusive, in either +state. In a run `very_good` resolves on PATH but `very_good --version` answers nothing, because it is a shim that execs `dart` under a throwaway `$HOME`. Read as `not_installed`, the PreToolUse hook denies the call and the model gets the hook's text as the tool result. diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 5b3d1fc..2a96c1b 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -55,10 +55,15 @@ deny_with_cli_check() { outdated:*) reason="Very Good CLI ${cli_status#outdated:} is too old (requires >= ${MIN_VERSION}). Update with: dart pub global activate very_good_cli." ;; - unverifiable) + dart_missing) # Redirecting to the MCP tool here would be a dead end: the server starts through # the same very_good shim, which cannot exec dart from this PATH either. - reason="Very Good CLI was found but could not run: dart is not on the PATH available to hooks, so the very_good_cli MCP server cannot start either. Add the Dart SDK bin directory to PATH for non-interactive shells (e.g. in ~/.zprofile) and start a new session." + reason="Very Good CLI was found but could not run: dart is not on the PATH available to hooks, so the very_good_cli MCP server cannot start either. Put the Dart SDK bin directory on the PATH your login shell always exports, or start Claude Code from a shell where dart already resolves, then begin a new session." + ;; + unverifiable) + # dart resolves, so the server is not a dead end. Redirect as usual, and say how + # to clear a stale snapshot if the tool does turn out to be the thing failing. + reason="$mcp_hint If that tool fails (for example with 'Invalid SDK hash'), the Dart SDK most likely changed under the installed Very Good CLI: run 'very_good --version' once in a terminal to rebuild it, then reconnect the server with /mcp." ;; *) reason="$mcp_hint" diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index 3c8ab2e..5ab2fad 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -21,7 +21,19 @@ FAILED=0 STUB_DIR="$(mktemp -d)" trap 'rm -rf "$STUB_DIR"' EXIT -BASE_PATH="$(dirname "$(command -v jq)"):/usr/bin:/bin:/usr/sbin:/sbin" +# jq reaches the hook through a directory holding nothing but jq. Putting jq's own +# directory on PATH drags its neighbours in with it, and on a Homebrew machine those +# include dart, which no_dart then cannot make disappear. +mkdir -p "$STUB_DIR/jq-bin" +ln -s "$(command -v jq)" "$STUB_DIR/jq-bin/jq" +BASE_PATH="$STUB_DIR/jq-bin:/usr/bin:/bin:/usr/sbin:/sbin" + +# The dart_missing cases need a PATH on which dart does not resolve. Say so outright +# rather than quietly exercise the other branch. +if PATH="$BASE_PATH" command -v dart >/dev/null 2>&1; then + echo "BASE_PATH resolves dart; the dart_missing cases cannot run here" >&2 + exit 1 +fi # Install a stubbed very_good. With a version argument the stub reports that version; # with no argument it fails the way the real shim does when `dart` is missing from PATH. @@ -32,13 +44,19 @@ stub_cli() { if [ -z "$version" ]; then printf '#!/bin/sh\necho "very_good: dart: command not found" >&2\nexit 127\n' > "$target" else - printf '#!/bin/sh\necho "very_good %s"\n' "$version" > "$target" + printf '#!/bin/sh\necho "%s"\n' "$version" > "$target" fi chmod +x "$target" } no_cli() { rm -f "$STUB_DIR/very_good"; } +# Install or remove a stubbed dart. The hooks only probe for its presence, so the stub +# needs no behavior: it is what separates "dart is off this PATH" from "dart runs and +# the version is still unreadable", which carry different diagnoses. +stub_dart() { printf '#!/bin/sh\nexit 0\n' > "$STUB_DIR/dart"; chmod +x "$STUB_DIR/dart"; } +no_dart() { rm -f "$STUB_DIR/dart"; } + # Run the hook on a payload. Sets LAST_RESULT to the permissionDecision ("deny"/"allow"), # "aside" when the hook exited 0 with no decision, or "exit:" on a non-zero exit. # The full stdout is left in LAST_OUTPUT for reason assertions. These are globals rather @@ -381,6 +399,15 @@ stub_cli assert_blocked "flutter test" assert_reason_contains "dart is not on the PATH" "CLI that cannot run points at PATH, not the MCP tool" +# With dart on PATH the shim runs, so the MCP server is not a dead end and the reason +# must still redirect to it rather than blame PATH. +stub_dart +stub_cli +assert_blocked "flutter test" +assert_reason_contains "MCP 'test' tool" "unreadable version with dart present still redirects" +assert_reason_contains "very_good --version" "unreadable version with dart present asks for a rebuild" +no_dart + echo "" echo "--- Deny reason says the whole call was refused ---" diff --git a/hooks/scripts/check-vgv-cli.sh b/hooks/scripts/check-vgv-cli.sh index 985c052..13ed1a3 100644 --- a/hooks/scripts/check-vgv-cli.sh +++ b/hooks/scripts/check-vgv-cli.sh @@ -32,9 +32,9 @@ case "$cli_status" in version="${cli_status#outdated:}" deny "Very Good CLI ${version} is too old. This tool requires Very Good CLI >= ${MIN_VERSION}. Update with: dart pub global activate very_good_cli" ;; - unverifiable) - # The CLI is present but its version could not be read. Stand aside rather than - # deny a genuine call on an inconclusive check; normal permission handling applies. + dart_missing | unverifiable) + # Inconclusive, for either reason. Stand aside rather than deny a genuine call on an + # inconclusive check; normal permission handling applies. exit 0 ;; esac diff --git a/hooks/scripts/check-vgv-cli_test.sh b/hooks/scripts/check-vgv-cli_test.sh index 2a049af..2caaf82 100644 --- a/hooks/scripts/check-vgv-cli_test.sh +++ b/hooks/scripts/check-vgv-cli_test.sh @@ -20,7 +20,19 @@ FAILED=0 STUB_DIR="$(mktemp -d)" trap 'rm -rf "$STUB_DIR"' EXIT -BASE_PATH="$(dirname "$(command -v jq)"):/usr/bin:/bin:/usr/sbin:/sbin" +# jq reaches the hook through a directory holding nothing but jq. Putting jq's own +# directory on PATH drags its neighbours in with it, and on a Homebrew machine those +# include dart, which no_dart then cannot make disappear. +mkdir -p "$STUB_DIR/jq-bin" +ln -s "$(command -v jq)" "$STUB_DIR/jq-bin/jq" +BASE_PATH="$STUB_DIR/jq-bin:/usr/bin:/bin:/usr/sbin:/sbin" + +# The dart_missing cases need a PATH on which dart does not resolve. Say so outright +# rather than quietly exercise the other branch. +if PATH="$BASE_PATH" command -v dart >/dev/null 2>&1; then + echo "BASE_PATH resolves dart; the dart_missing cases cannot run here" >&2 + exit 1 +fi # Install a stubbed very_good. With a version argument the stub reports that version; # with no argument it fails the way the real shim does when `dart` is missing from PATH. @@ -37,7 +49,7 @@ stub_cli() { if [ -z "$version" ]; then printf '#!/bin/sh\necho "very_good: dart: command not found" >&2\nexit 127\n' > "$target" else - printf '#!/bin/sh\necho "very_good %s"\n' "$version" > "$target" + printf '#!/bin/sh\necho "%s"\n' "$version" > "$target" fi chmod +x "$target" } diff --git a/hooks/scripts/vgv-cli-common.sh b/hooks/scripts/vgv-cli-common.sh index f56ec88..2fb2909 100755 --- a/hooks/scripts/vgv-cli-common.sh +++ b/hooks/scripts/vgv-cli-common.sh @@ -72,21 +72,29 @@ resolve_vgv_bin() { } # Check Very Good CLI availability and version. -# Returns: "ok", "not_installed", "outdated:", or "unverifiable". +# Returns: "ok", "not_installed", "outdated:", "dart_missing", or "unverifiable". # -# "unverifiable" means the binary was found but its version could not be read. The -# installed very_good is a shell shim that execs `dart`, so a PATH without dart on it -# makes the check inconclusive. Callers must not report that as "not installed". +# The last two both mean the version could not be read, so callers must not report +# either as "not installed". dart_missing is a PATH without dart, which the very_good +# shim needs to exec at all; unverifiable is dart present and the version still empty. check_vgv_cli() { local vgv_bin if ! vgv_bin=$(resolve_vgv_bin); then echo "not_installed" return fi - RAW=$("$vgv_bin" --version 2>/dev/null) - VERSION=$(echo "$RAW" | head -1 | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1) + # A stale snapshot sends the shim through `dart pub global run`, which prints pub's + # chatter to stdout ahead of the version, so the match must look past the first line. + # Anchoring keeps a version-shaped fragment of a path in that chatter from winning. + VERSION=$("$vgv_bin" --version 2>/dev/null | grep -m1 -oE '^[0-9]+\.[0-9]+\.[0-9]+$') if [ -z "$VERSION" ]; then - echo "unverifiable" + # Blaming PATH for every unreadable version sends users to edit profiles that were + # never at fault, so only say that when dart really is absent. + if command -v dart &>/dev/null; then + echo "unverifiable" + else + echo "dart_missing" + fi return fi IFS='.' read -r MAJOR MINOR PATCH <<< "$VERSION" diff --git a/hooks/scripts/warn-missing-mcp.sh b/hooks/scripts/warn-missing-mcp.sh index 928ab86..05135d5 100755 --- a/hooks/scripts/warn-missing-mcp.sh +++ b/hooks/scripts/warn-missing-mcp.sh @@ -6,6 +6,9 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "$SCRIPT_DIR/vgv-cli-common.sh" +# Neither unreadable-version notice may imply the server is down: hooks and the MCP client +# do not share a PATH, and under `claude plugin eval` the server is mocked and answers +# anyway. Say what is known and tell the session to carry on. See evals/README.md. cli_status=$(check_vgv_cli) case "$cli_status" in not_installed) @@ -15,12 +18,11 @@ case "$cli_status" in version="${cli_status#outdated:}" echo "⚠️ Very Good CLI ${version} is too old. The Very Good CLI MCP server requires >= ${MIN_VERSION}. Update with: dart pub global activate very_good_cli" ;; + dart_missing) + echo "ℹ️ Very Good CLI is installed but its version could not be verified here: dart is not on the PATH this hook inherits, so ${MIN_VERSION}+ could not be confirmed. If the Very Good CLI MCP tools answer, disregard this and carry on. If they do not, this is the likely cause. Put the Dart SDK bin directory on the PATH your login shell always exports, or start Claude Code from a shell where dart already resolves, then begin a new session." + ;; unverifiable) - # The very_good shim execs dart, so a PATH without dart hides the version from this - # hook. That does not prove the server is down: hooks and the MCP client do not share - # a PATH, and under `claude plugin eval` the server is mocked and always answers. Say - # what is known, not what it implies, or a session spends its answer on a false blocker. - echo "ℹ️ Very Good CLI is installed but its version could not be verified here: dart is not on the PATH this hook inherits, so ${MIN_VERSION}+ could not be confirmed. If the Very Good CLI MCP tools answer, disregard this and carry on. If they do not, this is the likely cause — add the Dart SDK bin directory to PATH for non-interactive shells (e.g. in ~/.zprofile) and start a new session." + echo "ℹ️ Very Good CLI is installed and dart is on the PATH, but the CLI did not report a version, so ${MIN_VERSION}+ could not be confirmed. If the Very Good CLI MCP tools answer, disregard this and carry on. If they fail (for example with 'Invalid SDK hash'), the Dart SDK most likely changed under the installed CLI: run 'very_good --version' once in a terminal to rebuild it, then reconnect the server with /mcp or start a new session." ;; esac diff --git a/hooks/scripts/warn-missing-mcp_test.sh b/hooks/scripts/warn-missing-mcp_test.sh index f7915c3..b2158e4 100755 --- a/hooks/scripts/warn-missing-mcp_test.sh +++ b/hooks/scripts/warn-missing-mcp_test.sh @@ -21,6 +21,13 @@ trap 'rm -rf "$STUB_DIR"' EXIT BASE_PATH="/usr/bin:/bin:/usr/sbin:/sbin" +# The dart_missing cases need a PATH on which dart does not resolve. Say so outright +# rather than quietly exercise the other branch. +if PATH="$BASE_PATH" command -v dart >/dev/null 2>&1; then + echo "BASE_PATH resolves dart; the dart_missing cases cannot run here" >&2 + exit 1 +fi + # Install a stubbed very_good. With a version argument the stub reports that version; # with no argument it fails the way the real shim does when `dart` is missing from PATH. # Usage: stub_cli [version] [--in-pub-cache] @@ -36,13 +43,30 @@ stub_cli() { if [ -z "$version" ]; then printf '#!/bin/sh\necho "very_good: dart: command not found" >&2\nexit 127\n' > "$target" else - printf '#!/bin/sh\necho "very_good %s"\n' "$version" > "$target" + printf '#!/bin/sh\necho "%s"\n' "$version" > "$target" fi chmod +x "$target" } no_cli() { rm -rf "$STUB_DIR/pub-cache" "$STUB_DIR/very_good"; } +# Install or remove a stubbed dart. The hooks only probe for its presence, so the stub +# needs no behavior: it is what separates "dart is off this PATH" from "dart runs and +# the version is still unreadable", which carry different diagnoses. +stub_dart() { printf '#!/bin/sh\nexit 0\n' > "$STUB_DIR/dart"; chmod +x "$STUB_DIR/dart"; } +no_dart() { rm -f "$STUB_DIR/dart"; } + +# Install a stubbed very_good that prints what the real shim prints once its snapshot is +# stale: the fallback `dart pub global run` sends pub's resolution chatter to stdout +# ahead of the version. The chatter carries a version-shaped path on purpose, so an +# unanchored match would read 1.2.3 as the CLI version. +stub_cli_rebuilding() { + rm -rf "$STUB_DIR/pub-cache" "$STUB_DIR/very_good" + printf '#!/bin/sh\necho "Resolving dependencies..."\necho "No dependencies would change in /tmp/vgv-1.2.3/pkg"\necho "%s"\n' "$1" \ + > "$STUB_DIR/very_good" + chmod +x "$STUB_DIR/very_good" +} + # Runs the hook. Leaves stdout in LAST_OUTPUT and the exit status in LAST_STATUS. LAST_OUTPUT="" LAST_STATUS=0 @@ -77,6 +101,17 @@ assert_warns() { fi } +assert_omits() { + local needle="$1" label="$2" + run_hook + assert_exit_zero "$label" + if [[ "$LAST_OUTPUT" != *"$needle"* ]]; then + pass "omits '$needle': $label" + else + fail "expected no mention of '$needle': $label" + fi +} + echo "=== warn-missing-mcp tests ===" echo "" echo "--- CLI present and current (no warning) ---" @@ -84,6 +119,16 @@ stub_cli 1.6.0 assert_silent "version above minimum" stub_cli 1.5.0 assert_silent "version exactly at minimum" + +echo "" +echo "--- Version behind pub's rebuild chatter (still current) ---" +# The reported bug: a stale snapshot sends the shim through `dart pub global run`, whose +# chatter lands on stdout before the version. Reading only the first line turned a +# working CLI into a warning about PATH. +stub_dart +stub_cli_rebuilding 1.5.0 +assert_silent "version printed after rebuild chatter" +no_dart stub_cli 1.6.0 --in-pub-cache assert_silent "resolved from PUB_CACHE, not PATH" @@ -100,6 +145,18 @@ echo "--- Version unreadable (shim present, dart missing) ---" stub_cli assert_warns "dart is not on the PATH" "CLI that cannot run points at PATH" +echo "" +echo "--- Version unreadable, dart present (not a PATH problem) ---" +# dart resolves, so PATH is not at fault. Blaming it here is the bug this split fixes: +# it sends users to edit shell profiles when the real cause is a snapshot built for an +# older SDK, cleared by running very_good once. +stub_dart +stub_cli +assert_warns "very_good --version" "unreadable version with dart present asks for a rebuild" +assert_warns "/mcp" "unreadable version with dart present says to reconnect the server" +assert_omits "dart is not on the PATH" "unreadable version with dart present never blames PATH" +no_dart + echo "" echo "=== Results: $PASSED passed, $FAILED failed ==="