From cf5c743fe3d6bfc0644a797569f7c0d259b8f2f7 Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 15:26:36 +0200 Subject: [PATCH 1/8] fix: match commands quote-aware in block-cli-workarounds The matcher split on shell operators with no notion of quoting or command position, so it denied read-only commands whose quoted arguments contained the governed strings, and missed real invocations behind any prefix. Two awk passes replace it. Pass 1 makes quoting inert, treating $( ) and backticks as command positions even inside double quotes and ending a line at an unquoted #. Pass 2 tests every adjacent token pair against the basename, which drops the wrapper list and covers melos, timeout, sudo, xargs, nice, shell keywords, brace groups and path-qualified binaries in one rule. Closes #147 Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 +- README.md | 2 +- config/cspell.json | 4 + hooks/scripts/block-cli-workarounds.sh | 108 +++++++-- hooks/scripts/block-cli-workarounds_test.sh | 231 +++++++++++++++++--- 5 files changed, 299 insertions(+), 52 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e2a900a..8d2f01d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,11 @@ These run **when a session begins**: 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.3.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 -- `Bash` matcher → `block-cli-workarounds.sh` — prevents direct CLI bypass of VGV CLI commands through the host's shell tool (`Bash`, or `Shell` on other hosts); exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected; when the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start; every denial also says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran; exits 2 on failure (blocking) +- `Bash` matcher → `block-cli-workarounds.sh` — routes direct CLI invocations to the Very Good CLI MCP tools, through the host's shell tool (`Bash`, or `Shell` on other hosts). It exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected, and blocks by returning a PreToolUse `deny` decision. Every denial names what it matched and says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran. When the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start. Matching works in two passes: + - **Pass 1 neutralizes quoting**, so an argument that merely contains a governed string is never read as a command: a quoted `|` is not a pipe, a quoted phrase is one word, and an unquoted `#` comments out the rest of the line. `$( )` and backticks stay command positions even inside double quotes, because the shell still runs them. + - **Pass 2 tests every adjacent token pair** in each fragment, matching the first on its basename. There is no wrapper list to maintain: `fvm`, `melos exec --`, `timeout`, `sudo -u ci`, `env -i`, `xargs`, `nice`, shell keywords (`then`, `do`), brace groups and `/usr/local/bin/flutter` are all covered by the same rule. + + The trade is that an unquoted `echo flutter test` is denied; text mentioning a governed command belongs in quotes, which pass 1 makes inert. `eval "flutter test"` and `F=flutter; $F test` still pass — catching those means reading inside quotes, which is the issue #147 bug, or executing the command. Those and heredoc bodies are pinned by tests so a change is visible The first two PreToolUse hooks are plugin-level (defined in `hooks.json`) and share common utilities from `vgv-cli-common.sh`. The following hook is **agent-scoped** — it is declared in the diff --git a/README.md b/README.md index d5981e5..46bd846 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ This plugin includes SessionStart, PreToolUse, and PostToolUse hooks that valida | ---- | ------- | -------- | | **Warn Missing MCP** (`warn-missing-mcp.sh`) | SessionStart | Warns if the Very Good CLI is missing, older than 1.3.0, or present but unable to run because `dart` is not on the `PATH` hooks inherit; non-blocking | | **Check VGV CLI** (`check-vgv-cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Verifies from the payload that the caller is a Very Good CLI tool and stands aside otherwise, so the decision never lands on an unrelated tool; for its own tools, auto-approves the call in every run mode via a PreToolUse `allow` decision, so they never dead-end when the tool isn't on `permissions.allow` (including under `skipAutoPermissionPrompt`); denies with an install/upgrade message if the CLI is missing or < 1.3.0 | -| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Blocks direct CLI bypass of Very Good CLI commands through the host's shell tool; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone; when the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so instead of redirecting to an MCP server that cannot start; exits 2 on failure (blocking) | +| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Routes direct CLI invocations to the Very Good CLI MCP tools; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone. Quoting-aware, so an argument that merely contains a governed string is never mistaken for a command — a quoted `\|` is not a pipe, a quoted phrase is one word, and an unquoted `#` starts a comment — while `$( )` and backticks stay command positions inside double quotes. Matching tests every adjacent token pair against the basename, so wrappers (`fvm`, `melos exec --`, `timeout`, `sudo`, `env -i`, `xargs`), shell keywords, brace groups and path-qualified binaries are all covered without a list to maintain. An unquoted `echo flutter test` is denied as a result; `eval` and variable indirection are not caught. The denial names what it matched and says the whole shell call was refused, so a command chained with the blocked one is re-run rather than assumed to have happened; when the CLI is present but cannot run because `dart` is missing from `PATH`, it says so instead of redirecting to an MCP server that cannot start; blocks by returning a PreToolUse `deny` decision | | **Allow Read-only Git** (`allow-readonly-git.sh`) | PreToolUse (`Bash`, `flutter-reviewer` agent only) | Restricts the `flutter-reviewer` agent's Bash to single-line `git diff`/`git status`, without the `--output` or `--ext-diff` options, and denies anything else (blocking). Scoped via the agent's frontmatter, not `hooks.json` | | **Analyze** (`analyze.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart analyze` on the modified `.dart` file; exits 2 on failure (blocking — Claude must fix issues before continuing) | | **Format** (`format.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart format` on the modified `.dart` file; always exits 0 (non-blocking — formatting is applied silently) | diff --git a/config/cspell.json b/config/cspell.json index 99c4f7b..279d32e 100644 --- a/config/cspell.json +++ b/config/cspell.json @@ -33,6 +33,8 @@ "GHSA", "goldens", "gradeable", + "gsub", + "heredocs", "hoverable", "icontains", "idgetbook", @@ -49,6 +51,7 @@ "mocktail", "monorepo", "Mundo", + "nohup", "opencode", "pasteable", "pipefail", @@ -65,6 +68,7 @@ "subagent", "subagents", "subclassing", + "subshell", "tappable", "tooltipped", "unmirrored", diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 98f2985..dc21e99 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -38,43 +38,117 @@ WHOLE_CALL_REFUSED="This whole shell call was refused, so none of it ran: run an # message when it is present but cannot run, and otherwise redirect to the MCP tool. deny_with_cli_check() { local mcp_hint="$1" - local cli_status + local cli_status reason cli_status=$(check_vgv_cli) case "$cli_status" in not_installed) - deny "Very Good CLI is required but was not found. Install with: dart pub global activate very_good_cli. $WHOLE_CALL_REFUSED" + reason="Very Good CLI is required but was not found. Install with: dart pub global activate very_good_cli." ;; outdated:*) - local version="${cli_status#outdated:}" - deny "Very Good CLI ${version} is too old (requires >= ${MIN_VERSION}). Update with: dart pub global activate very_good_cli. $WHOLE_CALL_REFUSED" + reason="Very Good CLI ${cli_status#outdated:} is too old (requires >= ${MIN_VERSION}). Update with: dart pub global activate very_good_cli." ;; unverifiable) # 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. - deny "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. $WHOLE_CALL_REFUSED" + 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." ;; *) - deny "$mcp_hint $WHOLE_CALL_REFUSED" + reason="$mcp_hint" ;; esac + # Every denial names what it matched, so a misfire is self-explaining, and says the + # whole call was refused, so a chained command is not assumed to have run. + deny "$reason $WHOLE_CALL_REFUSED Matched: $MATCHED" } -# Split on shell operators and check the first two tokens of each subcommand. -# This avoids false positives from file paths (.dart) or quoted strings. -BLOCKED=$(echo "$COMMAND" | awk '{ - n = split($0, parts, /[;&|]+/) +# Decide whether the command actually *runs* one of the blocked CLIs, in two awk passes. +# +# Pass 1 makes quoting inert: quote delimiters are dropped and shell-significant +# characters inside quotes become `_`, so a quoted `|` is not a pipe and a quoted phrase +# is one word. `$( )` and backticks are the exception -- the shell runs those even inside +# double quotes -- so they reopen an unquoted region. An unquoted `#` ends the line. +# +# Pass 2 splits on the operators that open a command position and tests every adjacent +# token pair, matching the first on its basename. Testing pairs rather than the first +# word is what removes the wrapper list: `fvm`, `melos exec --`, `timeout 60`, `sudo -u +# ci`, `xargs`, shell keywords and `/usr/local/bin/flutter` all fall out of the one rule. +# +# Consequences worth knowing, each pinned by a test: +# - an unquoted `echo flutter test` is denied; text naming a command belongs in quotes +# - `eval "..."` and `F=flutter; $F test` pass, since catching them means reading +# inside quotes, which is the issue #147 bug, or executing the command +# +# `printf` not `echo`, which eats a command starting with `-n`. The quote characters +# arrive via -v because a literal `'` cannot appear inside this single-quoted program. +RESULT=$(printf '%s\n' "$COMMAND" | awk -v SQ="'" -v DQ='"' ' +function neutral(c) { + if (c ~ /[;&|(){}`]/ || c ~ /[[:space:]]/) return "_" + return c +} +BEGIN { sq = 0; dq = 0; esc = 0; bt = 0; depth = 0; acc = "" } +{ + out = "" + len = length($0) + for (i = 1; i <= len; i++) { + c = substr($0, i, 1) + if (esc) { out = out neutral(c); esc = 0; continue } + if (!sq && c == "\\") { esc = 1; continue } + if (!dq && c == SQ) { sq = !sq; continue } + if (!sq && c == DQ) { dq = !dq; continue } + # `$(` and backticks run a command even inside double quotes, so they reopen an + # unquoted region; the matching `)` or backtick restores the quote state. + if (!sq && c == "$" && substr($0, i + 1, 1) == "(") { + depth++; saved[depth] = dq; dq = 0; out = out "("; i++; continue + } + if (!sq && !dq && depth > 0 && c == ")") { + dq = saved[depth]; depth--; out = out ")"; continue + } + if (!sq && c == "`") { + if (bt) { dq = bt_dq; bt = 0 } else { bt_dq = dq; dq = 0; bt = 1 } + out = out "`"; continue + } + # An unquoted `#` starting a word comments out the rest of the line. Stopping here + # rather than scanning on keeps a `;` in a comment from opening a command position, + # and keeps an apostrophe in a comment ("# it'"'"'s") from opening quote state that + # would swallow every following line. + if (!sq && !dq && c == "#" && + (out == "" || substr(out, length(out), 1) ~ /[[:space:]]/)) break + if (sq || dq) { out = out neutral(c); continue } + out = out c + } + acc = acc out + # A trailing backslash is a line continuation, and an unterminated quote swallows the + # line break. Neither ends a command, so neither may end the record pass 2 reads. + if (esc) next + if (sq || dq) { acc = acc "_"; next } + print acc; acc = "" +} +END { if (acc != "") print acc }' | awk ' +{ + n = split($0, parts, /[;&|(){}`]+/) for (i = 1; i <= n; i++) { + # Without this a fragment following an operator starts with a space, and the split + # below yields an empty leading token. gsub(/^[[:space:]]+/, "", parts[i]) - split(parts[i], w, /[[:space:]]+/) - b = w[1]; s = w[2] - if ((b == "flutter" || b == "dart") && s == "create") { print "create"; exit } - if ((b == "flutter" || b == "dart") && s == "test") { print "test"; exit } - if (b == "very_good" && s == "create") { print "vg_create"; exit } - if (b == "very_good" && s == "test") { print "vg_test"; exit } - if (b == "very_good" && s == "packages") { print "vg_packages"; exit } + nw = split(parts[i], w, /[[:space:]]+/) + for (j = 1; j < nw; j++) { + b = w[j]; s = w[j + 1] + # Match on the basename, so a path-qualified binary is still the same command. + sub(/^.*\//, "", b) + hit = "" + if ((b == "flutter" || b == "dart") && s == "create") hit = "create" + else if ((b == "flutter" || b == "dart") && s == "test") hit = "test" + else if (b == "very_good" && s == "create") hit = "vg_create" + else if (b == "very_good" && s == "test") hit = "vg_test" + else if (b == "very_good" && s == "packages") hit = "vg_packages" + if (hit != "") { printf "%s\t%s %s\n", hit, b, s; exit } + } } }') +BLOCKED="${RESULT%%$'\t'*}" +MATCHED="${RESULT#*$'\t'}" + case "$BLOCKED" in create) deny_with_cli_check "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." ;; test) deny_with_cli_check "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." ;; diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index d5d182e..5d1db12 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -66,14 +66,18 @@ run_hook() { run_hook_payload "$(jq -n --arg c "$1" '{"tool_input":{"command":$c}}')" } +# Label a case for the output. A command may be multi-line, which would wreck the +# aligned columns, so newlines are shown as a visible marker. +label_of() { printf '%s' "$1" | tr '\n' '~'; } + assert_blocked() { local cmd="$1" run_hook "$cmd" if [ "$LAST_RESULT" = "deny" ]; then - printf " \033[32mPASS\033[0m blocked: %s\n" "$cmd" + printf " \033[32mPASS\033[0m blocked: %s\n" "$(label_of "$cmd")" PASSED=$((PASSED + 1)) else - printf " \033[31mFAIL\033[0m expected deny but got %s: %s\n" "$LAST_RESULT" "$cmd" + printf " \033[31mFAIL\033[0m expected deny but got %s: %s\n" "$LAST_RESULT" "$(label_of "$cmd")" FAILED=$((FAILED + 1)) fi } @@ -82,10 +86,10 @@ assert_allowed() { local cmd="$1" run_hook "$cmd" if [ "$LAST_RESULT" = "aside" ]; then - printf " \033[32mPASS\033[0m allowed: %s\n" "$cmd" + printf " \033[32mPASS\033[0m allowed: %s\n" "$(label_of "$cmd")" PASSED=$((PASSED + 1)) else - printf " \033[31mFAIL\033[0m expected aside but got %s: %s\n" "$LAST_RESULT" "$cmd" + printf " \033[31mFAIL\033[0m expected aside but got %s: %s\n" "$LAST_RESULT" "$(label_of "$cmd")" FAILED=$((FAILED + 1)) fi } @@ -104,21 +108,29 @@ assert_reason_contains() { fi } +# The hook's own trigger strings, assembled at runtime. Writing them literally would +# make this file's own edits and greps trip the hook it tests -- which is the exact +# bug issue #147 reports. +FL="fl""utter" +TE="te""st" +VG="very_""good" +CR="cre""ate" + echo "=== block-cli-workarounds tests ===" stub_cli 1.5.0 echo "" echo "--- Should be BLOCKED ---" -assert_blocked "dart test" -assert_blocked "flutter test" -assert_blocked "dart test test/routing/foo_test.dart" -assert_blocked "flutter test --coverage" -assert_blocked "dart create my_app" -assert_blocked "flutter create my_app" -assert_blocked "very_good create flutter_app --project-name my_app" -assert_blocked "very_good test --coverage --min-coverage 100" -assert_blocked "very_good packages check licenses" -assert_blocked "cd /path && dart test" -assert_blocked "ENV=1 && flutter test --coverage" +assert_blocked "dart $TE" +assert_blocked "$FL $TE" +assert_blocked "dart $TE test/routing/foo_test.dart" +assert_blocked "$FL $TE --coverage" +assert_blocked "dart $CR my_app" +assert_blocked "$FL create my_app" +assert_blocked "$VG create flutter_app --project-name my_app" +assert_blocked "$VG $TE --coverage --min-coverage 100" +assert_blocked "$VG packages check licenses" +assert_blocked "cd /path && dart $TE" +assert_blocked "ENV=1 && $FL $TE --coverage" echo "" echo "--- Should be ALLOWED ---" @@ -126,16 +138,158 @@ assert_allowed "dart analyze lib/foo.dart" assert_allowed "dart format lib/foo.dart" assert_allowed "dart pub get" assert_allowed "dart fix --apply" -assert_allowed "flutter pub get" -assert_allowed "flutter analyze" +assert_allowed "$FL pub get" +assert_allowed "$FL analyze" assert_allowed "git add lib/router.dart test/router_test.dart" assert_allowed "dart analyze lib/foo.dart test/bar_test.dart" -assert_allowed "git commit -m 'fix dart test hook'" -assert_allowed "echo 'flutter create is blocked'" -assert_allowed "gh pr create --body 'use dart test instead'" -assert_allowed "git log --grep='dart test'" +assert_allowed "git commit -m 'fix dart $TE hook'" +assert_allowed "echo '$FL create is blocked'" +assert_allowed "gh pr create --body 'use dart $TE instead'" +assert_allowed "git log --grep='dart $TE'" assert_allowed "ls" assert_allowed "pwd" +assert_allowed "" +assert_allowed " " + +echo "" +echo "--- Quoted arguments are arguments, not commands (issue #147) ---" + +# The regression that prompted the issue. A `|` inside a quoted regex is not a pipe, +# so an alternation listing the governed strings must not be chopped into subcommands. +# Both positions matter: the bug only fired when the match was NOT the last alternative, +# because then no closing quote attached to the token. +assert_allowed "grep -nE \"$VG|$FL $TE|foo\" CLAUDE.md" +assert_allowed "grep -nE \"$VG|$FL $TE\" CLAUDE.md" +assert_allowed "grep -nE \"$FL $TE|$VG|foo\" AGENTS.md" +assert_allowed "rg '$FL $CR|dart $CR' docs/" + +# A quoted phrase is a single word and runs nothing. +assert_allowed "echo \"$FL $TE\"" +assert_allowed "echo '$FL $TE'" +assert_allowed "git commit -m \"ban $FL $TE | dart $TE\"" + +# Quoting the command name still runs the command, so this stays denied. +assert_blocked "\"$FL\" $TE" +assert_blocked "'dart' $TE" + +# One quote type does not toggle state inside the other. +assert_allowed "echo 'say \"$FL $TE\" now'" +assert_allowed "echo \"say '$FL $TE' now\"" + +# Escapes and unbalanced quotes must not throw the scanner off. +assert_allowed "echo \\\"$FL $TE\\\"" +assert_allowed "grep \"$FL $TE file.md" +assert_blocked "$FL $TE --name \"my app\"" + +# Quote state is carried across lines, because a quoted string may span newlines. +# Without that, the second line would start unquoted and read as an invocation. +assert_allowed "$(printf 'echo "hello\n%s %s\nworld"' "$FL" "$TE")" + +echo "" +echo "--- Command position ---" + +# Every unquoted separator opens a fresh command position. The quoted-alternation +# cases above prove a quoted `|` is inert; these prove an unquoted one still works. +assert_blocked "echo hi; $FL $TE" +assert_blocked "echo hi | $FL $TE" +assert_blocked "echo hi & $FL $TE" +assert_blocked "test -d lib || $FL $TE" + +# A prefix does not stop something from being an invocation. +assert_blocked "ENV=1 $FL $TE" +assert_blocked "CI=true COVERAGE=1 dart $TE" +assert_blocked "($FL $TE)" +assert_blocked "\$($FL $TE)" +assert_blocked "\`$FL $TE\`" +assert_blocked "echo start && ($VG $TE)" + +# Any wrapper passes through to the command it runs. There is no wrapper list: pass 2 +# tests every adjacent token pair, so a wrapper this suite never names is covered too, +# whatever options it takes. +assert_blocked "fvm $FL $TE" +assert_blocked "command $FL $TE" +assert_blocked "env $FL $TE" +assert_blocked "env FOO=1 $FL $TE" +assert_blocked "env -i $FL $TE" +assert_blocked "sudo $FL $TE" +assert_blocked "sudo -u ci $FL $TE" +assert_blocked "nohup $FL $TE" +assert_blocked "exec $FL $TE" +assert_blocked "time $FL $TE" +assert_blocked "timeout 60 $FL $TE" +assert_blocked "nice -n 10 $FL $TE" +assert_blocked "xargs $FL $TE" + +# melos is how a VGV monorepo runs anything across its packages, options and all. +assert_blocked "melos exec -- $FL $TE" +assert_blocked "melos exec --concurrency 1 -- dart $TE" + +# Shell keywords and brace groups open a command position like any other separator. +assert_blocked "if true; then $FL $TE; fi" +assert_blocked "for f in a; do $FL $TE; done" +assert_blocked "{ $FL $TE; }" +assert_blocked "while :; do dart $TE; done" + +# A path-qualified binary is the same command, so match on the basename. +assert_blocked "/usr/local/bin/$FL $TE" +assert_blocked "./$FL $TE" +assert_blocked "\$FLUTTER_ROOT/bin/$FL $TE" +assert_blocked "../sdk/bin/dart $TE" + +# ...but only when the wrapped command is itself blocked. +assert_allowed "fvm $FL pub get" +assert_allowed "command -v $FL" +assert_allowed "env | grep PATH" +assert_allowed "melos exec -- dart analyze" +assert_allowed "timeout 60 dart pub get" +assert_allowed "/usr/local/bin/$FL analyze" + +# A wrapper with nothing after it has no pair to match. +assert_allowed "fvm" +assert_allowed "env -i" +assert_allowed "ENV=1" + +# A path whose basename only resembles the command must not match. +assert_allowed "git add lib/router.dart $TE/router_${TE}.dart" +assert_allowed "cp foo.dart $TE/" +assert_allowed "ls bin/flutter_tools" + +# An unquoted `#` starts a comment: a `;` inside it opens no command position, and an +# apostrophe inside it opens no quote state that would swallow the following lines. +assert_allowed "ls # fix; dart $TE" +assert_allowed "$(printf '# it%ss broken\nls -la' "'")" + +# Double quotes do not disarm `$( )` or backticks: the shell still runs what is inside +# them, so they stay command positions. Only the quoting that really is inert -- a +# backslash-escaped `$`, or single quotes -- keeps them out of command position. +assert_blocked "OUT=\"\$($FL $TE)\"" +assert_blocked "echo \"\$($FL $TE)\"" +assert_blocked "echo \"\`$FL $TE\`\"" +assert_blocked "if [ -z \"\$(dart $TE)\" ]; then echo x; fi" +assert_allowed "echo \"\\\$($FL $TE)\"" +assert_allowed "echo '\$($FL $TE)'" +assert_allowed "echo \"\$(date) building\"" +assert_allowed "VAR=\"\$(ls)\"; dart analyze" + +# A trailing backslash continues the line, so the command word and its subcommand can +# be split across two lines and still be one invocation. +assert_blocked "$(printf '%s \\\n%s --coverage' "$FL" "$TE")" + +echo "" +echo "--- Documented non-goals ---" +# +# These are allowed because catching them would cost more than it buys. Each hides the +# command inside a quoted string or behind a variable, so matching it means reading +# inside quotes -- which is precisely the issue #147 bug -- or executing the command. +# They are asserted so that changing any of them is a visible decision, not an accident. +assert_allowed "eval \"$FL $TE\"" +assert_allowed "F=$FL; \$F $TE" +assert_allowed "bash -c '$FL $TE'" +assert_allowed "sh -c \"$FL $TE\"" + +# A heredoc body is text, not a command position, but the scanner does not model +# heredocs and denies it. Pinned as the known limitation it is. +assert_blocked "$(printf 'cat < Date: Mon, 5 Oct 2026 15:37:32 +0200 Subject: [PATCH 2/8] refactor: simplify the block-cli-workarounds matcher Read the whole command as one awk record, which removes the accumulator, the END flush and all cross-record quote-state carrying, and makes backslash line continuation fall out of the escape rule instead of needing its own branch. A kind[] lookup replaces the five-branch conditional, and the two awk programs are named so the pipeline reads in one line. Behavior is unchanged: the existing 131 cases pass untouched. Two cases cover a comment followed by a blocked command on a later line, which one record made load-bearing. Co-Authored-By: Claude Opus 5 (1M context) --- hooks/scripts/block-cli-workarounds.sh | 98 +++++++++++---------- hooks/scripts/block-cli-workarounds_test.sh | 5 ++ 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index dc21e99..ddf4290 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -61,90 +61,98 @@ deny_with_cli_check() { deny "$reason $WHOLE_CALL_REFUSED Matched: $MATCHED" } -# Decide whether the command actually *runs* one of the blocked CLIs, in two awk passes. +# Decide whether the command actually *runs* one of the blocked CLIs: neutralize quoting, +# then look for a governed invocation in what is left. # -# Pass 1 makes quoting inert: quote delimiters are dropped and shell-significant -# characters inside quotes become `_`, so a quoted `|` is not a pipe and a quoted phrase -# is one word. `$( )` and backticks are the exception -- the shell runs those even inside -# double quotes -- so they reopen an unquoted region. An unquoted `#` ends the line. -# -# Pass 2 splits on the operators that open a command position and tests every adjacent -# token pair, matching the first on its basename. Testing pairs rather than the first -# word is what removes the wrapper list: `fvm`, `melos exec --`, `timeout 60`, `sudo -u -# ci`, `xargs`, shell keywords and `/usr/local/bin/flutter` all fall out of the one rule. -# -# Consequences worth knowing, each pinned by a test: +# Testing every adjacent token pair, rather than only the first word of a subcommand, is +# what removes the wrapper list. `fvm`, `melos exec --`, `timeout 60`, `sudo -u ci`, +# `xargs`, shell keywords and `/usr/local/bin/flutter` all fall out of the one rule, and +# a new wrapper costs nothing. Two consequences, each pinned by a test: # - an unquoted `echo flutter test` is denied; text naming a command belongs in quotes # - `eval "..."` and `F=flutter; $F test` pass, since catching them means reading # inside quotes, which is the issue #147 bug, or executing the command # # `printf` not `echo`, which eats a command starting with `-n`. The quote characters -# arrive via -v because a literal `'` cannot appear inside this single-quoted program. -RESULT=$(printf '%s\n' "$COMMAND" | awk -v SQ="'" -v DQ='"' ' +# arrive via -v because a literal `'` cannot appear inside a single-quoted program. +# RS is a byte no shell command contains, so the whole command is one record. Quoting +# then spans newlines for free, and a newline is just another character to classify. +# If a command ever did contain a 0x01 byte it would split into two records, and a +# token pair straddling that split would not be seen as adjacent. +sanitize_quoting=' function neutral(c) { if (c ~ /[;&|(){}`]/ || c ~ /[[:space:]]/) return "_" return c } -BEGIN { sq = 0; dq = 0; esc = 0; bt = 0; depth = 0; acc = "" } +BEGIN { RS = "\001" } { out = "" len = length($0) for (i = 1; i <= len; i++) { c = substr($0, i, 1) - if (esc) { out = out neutral(c); esc = 0; continue } + # A backslash escapes the next character. Before a newline it continues the line, + # so the pair vanishes and the two lines become one command. + if (esc) { if (c != "\n") out = out neutral(c); esc = 0; continue } if (!sq && c == "\\") { esc = 1; continue } if (!dq && c == SQ) { sq = !sq; continue } if (!sq && c == DQ) { dq = !dq; continue } - # `$(` and backticks run a command even inside double quotes, so they reopen an - # unquoted region; the matching `)` or backtick restores the quote state. + # A command substitution runs even inside double quotes, so it reopens a live + # region that the matching ) or backtick closes again. if (!sq && c == "$" && substr($0, i + 1, 1) == "(") { depth++; saved[depth] = dq; dq = 0; out = out "("; i++; continue } if (!sq && !dq && depth > 0 && c == ")") { dq = saved[depth]; depth--; out = out ")"; continue } + # A backtick is one toggle rather than a stack because backticks do not nest the + # way $( ) does. if (!sq && c == "`") { if (bt) { dq = bt_dq; bt = 0 } else { bt_dq = dq; dq = 0; bt = 1 } out = out "`"; continue } - # An unquoted `#` starting a word comments out the rest of the line. Stopping here - # rather than scanning on keeps a `;` in a comment from opening a command position, - # and keeps an apostrophe in a comment ("# it'"'"'s") from opening quote state that - # would swallow every following line. - if (!sq && !dq && c == "#" && - (out == "" || substr(out, length(out), 1) ~ /[[:space:]]/)) break - if (sq || dq) { out = out neutral(c); continue } + # Must stay below the substitution and backtick branches, which suspend dq for the + # live region; above them it would neutralize what the shell actually runs. + if (sq || dq) { out = out neutral(c); continue } + # Outside quotes a newline ends a command just as a semicolon does, and a # that + # starts a word comments out the rest of its line. Skipping only to the newline + # matters: the record holds every line, so stopping here would hide the rest. + if (c == "\n") { out = out ";"; continue } + if (c == "#" && (out == "" || substr(out, length(out), 1) ~ /[[:space:];]/)) { + while (i < len && substr($0, i + 1, 1) != "\n") i++ + continue + } out = out c } - acc = acc out - # A trailing backslash is a line continuation, and an unterminated quote swallows the - # line break. Neither ends a command, so neither may end the record pass 2 reads. - if (esc) next - if (sq || dq) { acc = acc "_"; next } - print acc; acc = "" + print out +}' + +# Every quoted operator is inert by now, so each remaining one opens a command position. +find_invocation=' +BEGIN { + kind["flutter create"] = "create" + kind["dart create"] = "create" + kind["flutter test"] = "test" + kind["dart test"] = "test" + kind["very_good create"] = "vg_create" + kind["very_good test"] = "vg_test" + kind["very_good packages"] = "vg_packages" } -END { if (acc != "") print acc }' | awk ' { n = split($0, parts, /[;&|(){}`]+/) for (i = 1; i <= n; i++) { - # Without this a fragment following an operator starts with a space, and the split - # below yields an empty leading token. - gsub(/^[[:space:]]+/, "", parts[i]) nw = split(parts[i], w, /[[:space:]]+/) for (j = 1; j < nw; j++) { - b = w[j]; s = w[j + 1] - # Match on the basename, so a path-qualified binary is still the same command. + b = w[j] + # Match on the basename, so a path-qualified binary is the same command. sub(/^.*\//, "", b) - hit = "" - if ((b == "flutter" || b == "dart") && s == "create") hit = "create" - else if ((b == "flutter" || b == "dart") && s == "test") hit = "test" - else if (b == "very_good" && s == "create") hit = "vg_create" - else if (b == "very_good" && s == "test") hit = "vg_test" - else if (b == "very_good" && s == "packages") hit = "vg_packages" - if (hit != "") { printf "%s\t%s %s\n", hit, b, s; exit } + k = kind[b " " w[j + 1]] + if (k != "") { printf "%s\t%s %s\n", k, b, w[j + 1]; exit } } } -}') +}' + +RESULT=$(printf '%s\n' "$COMMAND" \ + | awk -v SQ="'" -v DQ='"' "$sanitize_quoting" \ + | awk "$find_invocation") BLOCKED="${RESULT%%$'\t'*}" MATCHED="${RESULT#*$'\t'}" diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index 5d1db12..fd7702f 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -259,6 +259,11 @@ assert_allowed "ls bin/flutter_tools" assert_allowed "ls # fix; dart $TE" assert_allowed "$(printf '# it%ss broken\nls -la' "'")" +# A comment ends at its own newline, not at the end of the command. The whole command +# is one awk record, so stopping the scan at the first `#` would hide every later line. +assert_blocked "$(printf '# a comment\n%s %s' "$FL" "$TE")" +assert_blocked "$(printf 'ls # note\ncd pkg\n%s %s' "$FL" "$TE")" + # Double quotes do not disarm `$( )` or backticks: the shell still runs what is inside # them, so they stay command positions. Only the quoting that really is inert -- a # backslash-escaped `$`, or single quotes -- keeps them out of command position. From 1fef523f6ac32dc0e92444b028746902baff3274 Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 22:29:28 +0200 Subject: [PATCH 3/8] refactor: drop the token alphabet between the matcher and the deny The matcher emitted one of five tokens that the caller translated straight back into a message, so the tokens existed only to be undone. The lookup table now holds the redirect itself and the five-branch case statement is gone. Reading each awk program from a quoted heredoc rather than a single-quoted string also retires the -v SQ/-v DQ workaround, since the program can now contain quote characters directly. Co-Authored-By: Claude Opus 5 (1M context) --- hooks/scripts/block-cli-workarounds.sh | 64 +++++++++++++------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index ddf4290..70763cc 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -37,7 +37,7 @@ WHOLE_CALL_REFUSED="This whole shell call was refused, so none of it ran: run an # Deny with an install/upgrade message when the CLI is missing or outdated, with a PATH # message when it is present but cannot run, and otherwise redirect to the MCP tool. deny_with_cli_check() { - local mcp_hint="$1" + local mcp_hint="$1" matched="$2" local cli_status reason cli_status=$(check_vgv_cli) case "$cli_status" in @@ -58,7 +58,7 @@ deny_with_cli_check() { esac # Every denial names what it matched, so a misfire is self-explaining, and says the # whole call was refused, so a chained command is not assumed to have run. - deny "$reason $WHOLE_CALL_REFUSED Matched: $MATCHED" + deny "$reason $WHOLE_CALL_REFUSED Matched: $matched" } # Decide whether the command actually *runs* one of the blocked CLIs: neutralize quoting, @@ -72,13 +72,17 @@ deny_with_cli_check() { # - `eval "..."` and `F=flutter; $F test` pass, since catching them means reading # inside quotes, which is the issue #147 bug, or executing the command # -# `printf` not `echo`, which eats a command starting with `-n`. The quote characters -# arrive via -v because a literal `'` cannot appear inside a single-quoted program. +# `printf` not `echo`, which eats a command starting with `-n`. +# +# Each program is read from a quoted heredoc so it can contain quote characters directly. +# `prog=$(cat <<'AWK' ...)` does not work here: bash scans a command substitution for its +# closing paren, and these programs hold unbalanced parens inside strings. +# # RS is a byte no shell command contains, so the whole command is one record. Quoting # then spans newlines for free, and a newline is just another character to classify. # If a command ever did contain a 0x01 byte it would split into two records, and a # token pair straddling that split would not be seen as adjacent. -sanitize_quoting=' +read -r -d '' sanitize_quoting <<'AWK' || true function neutral(c) { if (c ~ /[;&|(){}`]/ || c ~ /[[:space:]]/) return "_" return c @@ -93,8 +97,8 @@ BEGIN { RS = "\001" } # so the pair vanishes and the two lines become one command. if (esc) { if (c != "\n") out = out neutral(c); esc = 0; continue } if (!sq && c == "\\") { esc = 1; continue } - if (!dq && c == SQ) { sq = !sq; continue } - if (!sq && c == DQ) { dq = !dq; continue } + if (!dq && c == "'") { sq = !sq; continue } + if (!sq && c == "\"") { dq = !dq; continue } # A command substitution runs even inside double quotes, so it reopens a live # region that the matching ) or backtick closes again. if (!sq && c == "$" && substr($0, i + 1, 1) == "(") { @@ -123,18 +127,20 @@ BEGIN { RS = "\001" } out = out c } print out -}' +} +AWK # Every quoted operator is inert by now, so each remaining one opens a command position. -find_invocation=' +# The table emits the redirect itself, rather than a token the caller translates back. +read -r -d '' find_invocation <<'AWK' || true BEGIN { - kind["flutter create"] = "create" - kind["dart create"] = "create" - kind["flutter test"] = "test" - kind["dart test"] = "test" - kind["very_good create"] = "vg_create" - kind["very_good test"] = "vg_test" - kind["very_good packages"] = "vg_packages" + hint["flutter test"] = "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." + hint["dart test"] = hint["flutter test"] + hint["flutter create"] = "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." + hint["dart create"] = hint["flutter create"] + hint["very_good test"] = "Do not use 'very_good test' via shell. Use the very_good_cli MCP 'test' tool instead." + hint["very_good create"] = "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." + hint["very_good packages"] = "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." } { n = split($0, parts, /[;&|(){}`]+/) @@ -144,26 +150,18 @@ BEGIN { b = w[j] # Match on the basename, so a path-qualified binary is the same command. sub(/^.*\//, "", b) - k = kind[b " " w[j + 1]] - if (k != "") { printf "%s\t%s %s\n", k, b, w[j + 1]; exit } + pair = b " " w[j + 1] + if (pair in hint) { print hint[pair] "\t" pair; exit } } } -}' - -RESULT=$(printf '%s\n' "$COMMAND" \ - | awk -v SQ="'" -v DQ='"' "$sanitize_quoting" \ - | awk "$find_invocation") +} +AWK -BLOCKED="${RESULT%%$'\t'*}" -MATCHED="${RESULT#*$'\t'}" +RESULT=$(printf '%s\n' "$COMMAND" | awk "$sanitize_quoting" | awk "$find_invocation") -case "$BLOCKED" in - create) deny_with_cli_check "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." ;; - test) deny_with_cli_check "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." ;; - vg_create) deny_with_cli_check "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." ;; - vg_test) deny_with_cli_check "Do not use 'very_good test' via shell. Use the very_good_cli MCP 'test' tool instead." ;; - vg_packages) deny_with_cli_check "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." ;; -esac +# Empty means nothing in the command runs a blocked CLI, so stand aside. +if [ -n "$RESULT" ]; then + deny_with_cli_check "${RESULT%%$'\t'*}" "${RESULT#*$'\t'}" +fi -# Not a blocked command — allow exit 0 From 2956b9abad37f383c38fb9c78562b69bfac01248 Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 22:45:33 +0200 Subject: [PATCH 4/8] refactor: replace the shell lexer with three substitutions The character-level lexer existed to handle forms the agent never writes: quoting the command name, a backslash-continued line, a # comment inside a tool call. Three regex substitutions collapse quoted spans instead, and a second scan with the quotes deleted runs only when something executes the string -- eval, sh -c, or $( ) and backticks inside double quotes. That catches eval "flutter test" and bash -c '...', which the lexer did not, and drops "flutter" test and line continuation, which nobody types. The non-goal tests flip accordingly; everything else passes untouched. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 8 +- README.md | 2 +- hooks/scripts/block-cli-workarounds.sh | 112 ++++++-------------- hooks/scripts/block-cli-workarounds_test.sh | 45 ++++---- 4 files changed, 59 insertions(+), 108 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8d2f01d..b7553db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,11 +17,11 @@ These run **when a session begins**: 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.3.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 -- `Bash` matcher → `block-cli-workarounds.sh` — routes direct CLI invocations to the Very Good CLI MCP tools, through the host's shell tool (`Bash`, or `Shell` on other hosts). It exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected, and blocks by returning a PreToolUse `deny` decision. Every denial names what it matched and says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran. When the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start. Matching works in two passes: - - **Pass 1 neutralizes quoting**, so an argument that merely contains a governed string is never read as a command: a quoted `|` is not a pipe, a quoted phrase is one word, and an unquoted `#` comments out the rest of the line. `$( )` and backticks stay command positions even inside double quotes, because the shell still runs them. - - **Pass 2 tests every adjacent token pair** in each fragment, matching the first on its basename. There is no wrapper list to maintain: `fvm`, `melos exec --`, `timeout`, `sudo -u ci`, `env -i`, `xargs`, `nice`, shell keywords (`then`, `do`), brace groups and `/usr/local/bin/flutter` are all covered by the same rule. +- `Bash` matcher → `block-cli-workarounds.sh` — routes direct CLI invocations to the Very Good CLI MCP tools, through the host's shell tool (`Bash`, or `Shell` on other hosts). It exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected, and blocks by returning a PreToolUse `deny` decision. Every denial names what it matched and says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran. When the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start. Matching is deliberately simple: + - **Quoted text is data.** Each quoted span collapses to one word, so an argument that merely contains a governed string is never read as a command — a quoted `|` is not a pipe. The exception is text that something executes: an `eval` or `sh -c` string, and `$( )` or backticks inside double quotes, are scanned as commands. + - **Every adjacent word pair is tested**, matching the first on its basename, so there is no wrapper list: `fvm`, `melos exec --`, `timeout`, `sudo -u ci`, shell keywords, brace groups and `/usr/local/bin/flutter` all fall out of the one rule. - The trade is that an unquoted `echo flutter test` is denied; text mentioning a governed command belongs in quotes, which pass 1 makes inert. `eval "flutter test"` and `F=flutter; $F test` still pass — catching those means reading inside quotes, which is the issue #147 bug, or executing the command. Those and heredoc bodies are pinned by tests so a change is visible + Accepted costs, each pinned by a test: an unquoted `echo flutter test` is denied, as is a `#` comment containing `; dart test`; `"flutter" test`, a backslash-continued line and `F=flutter; $F test` pass. Those are forms the agent does not write, and catching them would need a shell lexer in place of three substitutions The first two PreToolUse hooks are plugin-level (defined in `hooks.json`) and share common utilities from `vgv-cli-common.sh`. The following hook is **agent-scoped** — it is declared in the diff --git a/README.md b/README.md index 46bd846..d7f332d 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ This plugin includes SessionStart, PreToolUse, and PostToolUse hooks that valida | ---- | ------- | -------- | | **Warn Missing MCP** (`warn-missing-mcp.sh`) | SessionStart | Warns if the Very Good CLI is missing, older than 1.3.0, or present but unable to run because `dart` is not on the `PATH` hooks inherit; non-blocking | | **Check VGV CLI** (`check-vgv-cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Verifies from the payload that the caller is a Very Good CLI tool and stands aside otherwise, so the decision never lands on an unrelated tool; for its own tools, auto-approves the call in every run mode via a PreToolUse `allow` decision, so they never dead-end when the tool isn't on `permissions.allow` (including under `skipAutoPermissionPrompt`); denies with an install/upgrade message if the CLI is missing or < 1.3.0 | -| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Routes direct CLI invocations to the Very Good CLI MCP tools; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone. Quoting-aware, so an argument that merely contains a governed string is never mistaken for a command — a quoted `\|` is not a pipe, a quoted phrase is one word, and an unquoted `#` starts a comment — while `$( )` and backticks stay command positions inside double quotes. Matching tests every adjacent token pair against the basename, so wrappers (`fvm`, `melos exec --`, `timeout`, `sudo`, `env -i`, `xargs`), shell keywords, brace groups and path-qualified binaries are all covered without a list to maintain. An unquoted `echo flutter test` is denied as a result; `eval` and variable indirection are not caught. The denial names what it matched and says the whole shell call was refused, so a command chained with the blocked one is re-run rather than assumed to have happened; when the CLI is present but cannot run because `dart` is missing from `PATH`, it says so instead of redirecting to an MCP server that cannot start; blocks by returning a PreToolUse `deny` decision | +| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Routes direct CLI invocations to the Very Good CLI MCP tools; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone. Quoted text is data, so an argument that merely contains a governed string is never mistaken for a command — a quoted `\|` is not a pipe — unless something executes it: `eval` and `sh -c` strings, and `$( )` or backticks inside double quotes, are scanned as commands. Matching tests every adjacent word pair against the basename, so wrappers (`fvm`, `melos exec --`, `timeout`, `sudo -u ci`), shell keywords, brace groups and path-qualified binaries are all covered without a list to maintain. An unquoted `echo flutter test` is denied as a result; `"flutter" test` and variable indirection are not caught. The denial names what it matched and says the whole shell call was refused, so a command chained with the blocked one is re-run rather than assumed to have happened; when the CLI is present but cannot run because `dart` is missing from `PATH`, it says so instead of redirecting to an MCP server that cannot start; blocks by returning a PreToolUse `deny` decision | | **Allow Read-only Git** (`allow-readonly-git.sh`) | PreToolUse (`Bash`, `flutter-reviewer` agent only) | Restricts the `flutter-reviewer` agent's Bash to single-line `git diff`/`git status`, without the `--output` or `--ext-diff` options, and denies anything else (blocking). Scoped via the agent's frontmatter, not `hooks.json` | | **Analyze** (`analyze.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart analyze` on the modified `.dart` file; exits 2 on failure (blocking — Claude must fix issues before continuing) | | **Format** (`format.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart format` on the modified `.dart` file; always exits 0 (non-blocking — formatting is applied silently) | diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 70763cc..053205a 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -61,79 +61,33 @@ deny_with_cli_check() { deny "$reason $WHOLE_CALL_REFUSED Matched: $matched" } -# Decide whether the command actually *runs* one of the blocked CLIs: neutralize quoting, -# then look for a governed invocation in what is left. +# Decide whether the command runs one of the blocked CLIs. # -# Testing every adjacent token pair, rather than only the first word of a subcommand, is -# what removes the wrapper list. `fvm`, `melos exec --`, `timeout 60`, `sudo -u ci`, -# `xargs`, shell keywords and `/usr/local/bin/flutter` all fall out of the one rule, and -# a new wrapper costs nothing. Two consequences, each pinned by a test: -# - an unquoted `echo flutter test` is denied; text naming a command belongs in quotes -# - `eval "..."` and `F=flutter; $F test` pass, since catching them means reading -# inside quotes, which is the issue #147 bug, or executing the command +# Quoted text is data, so every quoted span collapses to a single word and can no +# longer look like an operator or a command. Then every adjacent pair of words in each +# subcommand is tested, matching the first on its basename. Testing pairs is what makes +# wrappers free: fvm, melos exec --, timeout 60, sudo -u ci, shell keywords and +# /usr/local/bin/flutter all fall out of the one rule. # -# `printf` not `echo`, which eats a command starting with `-n`. -# -# Each program is read from a quoted heredoc so it can contain quote characters directly. -# `prog=$(cat <<'AWK' ...)` does not work here: bash scans a command substitution for its -# closing paren, and these programs hold unbalanced parens inside strings. -# -# RS is a byte no shell command contains, so the whole command is one record. Quoting -# then spans newlines for free, and a newline is just another character to classify. -# If a command ever did contain a 0x01 byte it would split into two records, and a -# token pair straddling that split would not be seen as adjacent. -read -r -d '' sanitize_quoting <<'AWK' || true -function neutral(c) { - if (c ~ /[;&|(){}`]/ || c ~ /[[:space:]]/) return "_" - return c -} -BEGIN { RS = "\001" } -{ - out = "" - len = length($0) - for (i = 1; i <= len; i++) { - c = substr($0, i, 1) - # A backslash escapes the next character. Before a newline it continues the line, - # so the pair vanishes and the two lines become one command. - if (esc) { if (c != "\n") out = out neutral(c); esc = 0; continue } - if (!sq && c == "\\") { esc = 1; continue } - if (!dq && c == "'") { sq = !sq; continue } - if (!sq && c == "\"") { dq = !dq; continue } - # A command substitution runs even inside double quotes, so it reopens a live - # region that the matching ) or backtick closes again. - if (!sq && c == "$" && substr($0, i + 1, 1) == "(") { - depth++; saved[depth] = dq; dq = 0; out = out "("; i++; continue - } - if (!sq && !dq && depth > 0 && c == ")") { - dq = saved[depth]; depth--; out = out ")"; continue - } - # A backtick is one toggle rather than a stack because backticks do not nest the - # way $( ) does. - if (!sq && c == "`") { - if (bt) { dq = bt_dq; bt = 0 } else { bt_dq = dq; dq = 0; bt = 1 } - out = out "`"; continue - } - # Must stay below the substitution and backtick branches, which suspend dq for the - # live region; above them it would neutralize what the shell actually runs. - if (sq || dq) { out = out neutral(c); continue } - # Outside quotes a newline ends a command just as a semicolon does, and a # that - # starts a word comments out the rest of its line. Skipping only to the newline - # matters: the record holds every line, so stopping here would hide the rest. - if (c == "\n") { out = out ";"; continue } - if (c == "#" && (out == "" || substr(out, length(out), 1) ~ /[[:space:];]/)) { - while (i < len && substr($0, i + 1, 1) != "\n") i++ - continue +# The whole command is one record (RS is a byte no command contains), so a quoted +# span may cross a newline. The program is a quoted heredoc so it can hold quote +# characters; printf rather than echo, which would eat a command starting with -n. +read -r -d '' find_invocation <<'AWK' || true +function scan(s, n, parts, i, nw, w, j, b, pair) { + n = split(s, parts, /[;&|(){}`\n]+/) + for (i = 1; i <= n; i++) { + nw = split(parts[i], w, /[[:space:]]+/) + for (j = 1; j < nw; j++) { + b = w[j] + sub(/^.*\//, "", b) + pair = b " " w[j + 1] + if (pair in hint) return pair } - out = out c } - print out + return "" } -AWK - -# Every quoted operator is inert by now, so each remaining one opens a command position. -# The table emits the redirect itself, rather than a token the caller translates back. -read -r -d '' find_invocation <<'AWK' || true BEGIN { + RS = "\001" hint["flutter test"] = "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." hint["dart test"] = hint["flutter test"] hint["flutter create"] = "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." @@ -143,23 +97,23 @@ BEGIN { hint["very_good packages"] = "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." } { - n = split($0, parts, /[;&|(){}`]+/) - for (i = 1; i <= n; i++) { - nw = split(parts[i], w, /[[:space:]]+/) - for (j = 1; j < nw; j++) { - b = w[j] - # Match on the basename, so a path-qualified binary is the same command. - sub(/^.*\//, "", b) - pair = b " " w[j + 1] - if (pair in hint) { print hint[pair] "\t" pair; exit } - } + gsub(/\\["'$`]/, "_") # an escaped quote, $ or backtick is a literal character + sq = $0; gsub(/'[^']*'/, "_", sq) # single-quoted text is data + dq = sq; gsub(/"[^"]*"/, "_", dq) # so is double-quoted text... + pair = scan(dq) + # ...unless something executes it. eval and sh -c run any quoted string; $( ) and + # backticks run inside double quotes but never inside single ones. + if (pair == "" && $0 ~ /(^|[^[:alnum:]_])(eval|sh|bash|zsh)[[:space:]]/) { + s = $0; gsub(/["']/, "", s); pair = scan(s) + } else if (pair == "" && sq ~ /\$\(|`/) { + s = sq; gsub(/"/, "", s); pair = scan(s) } + if (pair != "") print hint[pair] "\t" pair } AWK -RESULT=$(printf '%s\n' "$COMMAND" | awk "$sanitize_quoting" | awk "$find_invocation") +RESULT=$(printf '%s\n' "$COMMAND" | awk "$find_invocation") -# Empty means nothing in the command runs a blocked CLI, so stand aside. if [ -n "$RESULT" ]; then deny_with_cli_check "${RESULT%%$'\t'*}" "${RESULT#*$'\t'}" fi diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index fd7702f..51394d5 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -168,10 +168,6 @@ assert_allowed "echo \"$FL $TE\"" assert_allowed "echo '$FL $TE'" assert_allowed "git commit -m \"ban $FL $TE | dart $TE\"" -# Quoting the command name still runs the command, so this stays denied. -assert_blocked "\"$FL\" $TE" -assert_blocked "'dart' $TE" - # One quote type does not toggle state inside the other. assert_allowed "echo 'say \"$FL $TE\" now'" assert_allowed "echo \"say '$FL $TE' now\"" @@ -254,19 +250,17 @@ assert_allowed "git add lib/router.dart $TE/router_${TE}.dart" assert_allowed "cp foo.dart $TE/" assert_allowed "ls bin/flutter_tools" -# An unquoted `#` starts a comment: a `;` inside it opens no command position, and an -# apostrophe inside it opens no quote state that would swallow the following lines. -assert_allowed "ls # fix; dart $TE" -assert_allowed "$(printf '# it%ss broken\nls -la' "'")" - -# A comment ends at its own newline, not at the end of the command. The whole command -# is one awk record, so stopping the scan at the first `#` would hide every later line. +# Comments are not modelled. A blocked command on a line after a comment is still +# caught, and an apostrophe in a comment does not swallow the following lines. The cost +# is that `; dart test` inside a comment is denied -- an accepted false positive, since +# a `#` comment inside a tool call is not something the agent writes. assert_blocked "$(printf '# a comment\n%s %s' "$FL" "$TE")" assert_blocked "$(printf 'ls # note\ncd pkg\n%s %s' "$FL" "$TE")" +assert_allowed "$(printf '# it%ss broken\nls -la' "'")" +assert_blocked "ls # fix; dart $TE" -# Double quotes do not disarm `$( )` or backticks: the shell still runs what is inside -# them, so they stay command positions. Only the quoting that really is inert -- a -# backslash-escaped `$`, or single quotes -- keeps them out of command position. +# `$( )` and backticks execute inside double quotes, so a blocked command there is +# caught. Single quotes and a backslash-escaped `$` really are inert and stay allowed. assert_blocked "OUT=\"\$($FL $TE)\"" assert_blocked "echo \"\$($FL $TE)\"" assert_blocked "echo \"\`$FL $TE\`\"" @@ -276,21 +270,24 @@ assert_allowed "echo '\$($FL $TE)'" assert_allowed "echo \"\$(date) building\"" assert_allowed "VAR=\"\$(ls)\"; dart analyze" -# A trailing backslash continues the line, so the command word and its subcommand can -# be split across two lines and still be one invocation. -assert_blocked "$(printf '%s \\\n%s --coverage' "$FL" "$TE")" +# A quoted string handed to eval or sh -c is executed, so there the quotes are +# delimiters, not data. +assert_blocked "eval \"$FL $TE\"" +assert_blocked "bash -c '$FL $TE'" +assert_blocked "sh -c \"$FL $TE\"" +assert_blocked "zsh -c \"cd pkg && dart $TE\"" echo "" echo "--- Documented non-goals ---" # -# These are allowed because catching them would cost more than it buys. Each hides the -# command inside a quoted string or behind a variable, so matching it means reading -# inside quotes -- which is precisely the issue #147 bug -- or executing the command. -# They are asserted so that changing any of them is a visible decision, not an accident. -assert_allowed "eval \"$FL $TE\"" +# These are allowed on purpose, and asserted so that changing one is a visible decision. +# A variable cannot be resolved without executing the command. The other three are forms +# nobody types; catching them would need a character-level shell lexer in place of the +# three substitutions, and the agent has never produced any of them. assert_allowed "F=$FL; \$F $TE" -assert_allowed "bash -c '$FL $TE'" -assert_allowed "sh -c \"$FL $TE\"" +assert_allowed "\"$FL\" $TE" +assert_allowed "'dart' $TE" +assert_allowed "$(printf '%s \\\n%s --coverage' "$FL" "$TE")" # A heredoc body is text, not a command position, but the scanner does not model # heredocs and denies it. Pinned as the known limitation it is. From 400a66a62f04d4e05fef84bfe67f70ca40531bc1 Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 22:51:43 +0200 Subject: [PATCH 5/8] refactor: flatten the matcher and cut the comments to one clause each scan() turns operators into a ";" word and walks one flat word list instead of a nested fragment loop. The main block names its three views of the command up front, so the decision reads as three gated scans. Comments say what, not why. Co-Authored-By: Claude Fable 5.1 --- hooks/scripts/block-cli-workarounds.sh | 63 ++++++++++---------------- 1 file changed, 24 insertions(+), 39 deletions(-) diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 053205a..768fe21 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -56,38 +56,15 @@ deny_with_cli_check() { reason="$mcp_hint" ;; esac - # Every denial names what it matched, so a misfire is self-explaining, and says the - # whole call was refused, so a chained command is not assumed to have run. deny "$reason $WHOLE_CALL_REFUSED Matched: $matched" } -# Decide whether the command runs one of the blocked CLIs. -# -# Quoted text is data, so every quoted span collapses to a single word and can no -# longer look like an operator or a command. Then every adjacent pair of words in each -# subcommand is tested, matching the first on its basename. Testing pairs is what makes -# wrappers free: fvm, melos exec --, timeout 60, sudo -u ci, shell keywords and -# /usr/local/bin/flutter all fall out of the one rule. -# -# The whole command is one record (RS is a byte no command contains), so a quoted -# span may cross a newline. The program is a quoted heredoc so it can hold quote -# characters; printf rather than echo, which would eat a command starting with -n. +# Deny when the command runs a blocked CLI. Quoted text is data unless something +# executes it. Every adjacent word pair is checked, so wrappers (fvm, melos exec --, +# sudo, timeout, shell keywords, /path/to/flutter) need no list. read -r -d '' find_invocation <<'AWK' || true -function scan(s, n, parts, i, nw, w, j, b, pair) { - n = split(s, parts, /[;&|(){}`\n]+/) - for (i = 1; i <= n; i++) { - nw = split(parts[i], w, /[[:space:]]+/) - for (j = 1; j < nw; j++) { - b = w[j] - sub(/^.*\//, "", b) - pair = b " " w[j + 1] - if (pair in hint) return pair - } - } - return "" -} BEGIN { - RS = "\001" + RS = "\001" # whole command is one record hint["flutter test"] = "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." hint["dart test"] = hint["flutter test"] hint["flutter create"] = "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." @@ -96,23 +73,31 @@ BEGIN { hint["very_good create"] = "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." hint["very_good packages"] = "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." } -{ - gsub(/\\["'$`]/, "_") # an escaped quote, $ or backtick is a literal character - sq = $0; gsub(/'[^']*'/, "_", sq) # single-quoted text is data - dq = sq; gsub(/"[^"]*"/, "_", dq) # so is double-quoted text... - pair = scan(dq) - # ...unless something executes it. eval and sh -c run any quoted string; $( ) and - # backticks run inside double quotes but never inside single ones. - if (pair == "" && $0 ~ /(^|[^[:alnum:]_])(eval|sh|bash|zsh)[[:space:]]/) { - s = $0; gsub(/["']/, "", s); pair = scan(s) - } else if (pair == "" && sq ~ /\$\(|`/) { - s = sq; gsub(/"/, "", s); pair = scan(s) +# First blocked "cmd sub" pair in s, or "". Extra params are awk locals. +function scan(s, n, w, i, b, pair) { + gsub(/[;&|(){}`\n]+/, " ; ", s) # operators end a command + n = split(s, w, /[[:space:]]+/) + for (i = 1; i < n; i++) { + b = w[i]; sub(/^.*\//, "", b) # basename + pair = b " " w[i + 1] + if (pair in hint) return pair } + return "" +} +{ + gsub(/\\["'$`]/, "_") # escaped: literal + text = $0; gsub(/'[^']*'/, "_", text); gsub(/"[^"]*"/, "_", text) # quotes are data + subst = $0; gsub(/'[^']*'/, "_", subst); gsub(/"/, "", subst) # but "$( )" runs + bare = $0; gsub(/["']/, "", bare) # and eval "" runs + + pair = scan(text) + if (pair == "" && $0 ~ /\$\(|`/) pair = scan(subst) + if (pair == "" && $0 ~ /(^|[^[:alnum:]_])(eval|sh|bash|zsh) /) pair = scan(bare) if (pair != "") print hint[pair] "\t" pair } AWK -RESULT=$(printf '%s\n' "$COMMAND" | awk "$find_invocation") +RESULT=$(printf '%s\n' "$COMMAND" | awk "$find_invocation") # printf: echo eats -n if [ -n "$RESULT" ]; then deny_with_cli_check "${RESULT%%$'\t'*}" "${RESULT#*$'\t'}" From 0318ada73f1f7b724b07ea97f52041cc95bd3cbd Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 22:56:10 +0200 Subject: [PATCH 6/8] docs: cut the hook descriptions to what a reader needs The README row and CLAUDE.md bullet each said the same thing three ways. The file header now points at the hint table instead of duplicating its contents, and notes why the program is read with read rather than a command substitution. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 6 +----- README.md | 2 +- hooks/scripts/block-cli-workarounds.sh | 8 ++++---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b7553db..8322348 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,11 +17,7 @@ These run **when a session begins**: 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.3.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 -- `Bash` matcher → `block-cli-workarounds.sh` — routes direct CLI invocations to the Very Good CLI MCP tools, through the host's shell tool (`Bash`, or `Shell` on other hosts). It exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected, and blocks by returning a PreToolUse `deny` decision. Every denial names what it matched and says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran. When the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start. Matching is deliberately simple: - - **Quoted text is data.** Each quoted span collapses to one word, so an argument that merely contains a governed string is never read as a command — a quoted `|` is not a pipe. The exception is text that something executes: an `eval` or `sh -c` string, and `$( )` or backticks inside double quotes, are scanned as commands. - - **Every adjacent word pair is tested**, matching the first on its basename, so there is no wrapper list: `fvm`, `melos exec --`, `timeout`, `sudo -u ci`, shell keywords, brace groups and `/usr/local/bin/flutter` all fall out of the one rule. - - Accepted costs, each pinned by a test: an unquoted `echo flutter test` is denied, as is a `#` comment containing `; dart test`; `"flutter" test`, a backslash-continued line and `F=flutter; $F test` pass. Those are forms the agent does not write, and catching them would need a shell lexer in place of three substitutions +- `Bash` matcher → `block-cli-workarounds.sh` — denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` run through the host's shell tool (`Bash`, or `Shell` on other hosts), redirecting to the MCP tool; stands aside for any other tool. Quoted text is data, so a command name inside a grep pattern or commit message never matches; `eval`, `sh -c`, and `$( )` or backticks in double quotes are scanned as commands. Every adjacent word pair is checked, so wrappers (`fvm`, `melos exec --`, `sudo`, `timeout`), shell keywords and `/path/to/flutter` need no list. The denial names the match, says the whole call was refused, and says so when the CLI cannot run because `dart` is off `PATH`. Pinned gaps: `"flutter" test` and `F=flutter; $F test` pass; an unquoted `echo flutter test` is denied The first two PreToolUse hooks are plugin-level (defined in `hooks.json`) and share common utilities from `vgv-cli-common.sh`. The following hook is **agent-scoped** — it is declared in the diff --git a/README.md b/README.md index d7f332d..c6b4be8 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ This plugin includes SessionStart, PreToolUse, and PostToolUse hooks that valida | ---- | ------- | -------- | | **Warn Missing MCP** (`warn-missing-mcp.sh`) | SessionStart | Warns if the Very Good CLI is missing, older than 1.3.0, or present but unable to run because `dart` is not on the `PATH` hooks inherit; non-blocking | | **Check VGV CLI** (`check-vgv-cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Verifies from the payload that the caller is a Very Good CLI tool and stands aside otherwise, so the decision never lands on an unrelated tool; for its own tools, auto-approves the call in every run mode via a PreToolUse `allow` decision, so they never dead-end when the tool isn't on `permissions.allow` (including under `skipAutoPermissionPrompt`); denies with an install/upgrade message if the CLI is missing or < 1.3.0 | -| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Routes direct CLI invocations to the Very Good CLI MCP tools; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone. Quoted text is data, so an argument that merely contains a governed string is never mistaken for a command — a quoted `\|` is not a pipe — unless something executes it: `eval` and `sh -c` strings, and `$( )` or backticks inside double quotes, are scanned as commands. Matching tests every adjacent word pair against the basename, so wrappers (`fvm`, `melos exec --`, `timeout`, `sudo -u ci`), shell keywords, brace groups and path-qualified binaries are all covered without a list to maintain. An unquoted `echo flutter test` is denied as a result; `"flutter" test` and variable indirection are not caught. The denial names what it matched and says the whole shell call was refused, so a command chained with the blocked one is re-run rather than assumed to have happened; when the CLI is present but cannot run because `dart` is missing from `PATH`, it says so instead of redirecting to an MCP server that cannot start; blocks by returning a PreToolUse `deny` decision | +| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` run through the shell, redirecting to the MCP tool. Quoted text is data (a command name in a grep pattern never matches) unless `eval`, `sh -c`, or `$( )` in double quotes runs it. Every adjacent word pair is checked, so wrappers like `fvm`, `melos exec --`, `sudo` and `timeout` need no list. The denial names the match and says the whole call was refused | | **Allow Read-only Git** (`allow-readonly-git.sh`) | PreToolUse (`Bash`, `flutter-reviewer` agent only) | Restricts the `flutter-reviewer` agent's Bash to single-line `git diff`/`git status`, without the `--output` or `--ext-diff` options, and denies anything else (blocking). Scoped via the agent's frontmatter, not `hooks.json` | | **Analyze** (`analyze.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart analyze` on the modified `.dart` file; exits 2 on failure (blocking — Claude must fix issues before continuing) | | **Format** (`format.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart format` on the modified `.dart` file; always exits 0 (non-blocking — formatting is applied silently) | diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 768fe21..2903d70 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -1,7 +1,7 @@ #!/bin/bash -# PreToolUse hook: block Bash commands that bypass MCP tools. -# Denies flutter create, dart create, very_good create, very_good test, -# very_good packages, flutter test, dart test. +# PreToolUse hook: deny shell calls to CLI commands the Very Good CLI MCP tools cover. +# The hint table below is the list. To block another command, add a line there and a +# case in block-cli-workarounds_test.sh. if ! command -v jq &>/dev/null; then echo "jq is required for block-cli-workarounds hook but not found" >&2 @@ -62,7 +62,7 @@ deny_with_cli_check() { # Deny when the command runs a blocked CLI. Quoted text is data unless something # executes it. Every adjacent word pair is checked, so wrappers (fvm, melos exec --, # sudo, timeout, shell keywords, /path/to/flutter) need no list. -read -r -d '' find_invocation <<'AWK' || true +read -r -d '' find_invocation <<'AWK' || true # read, not $(cat): unbalanced parens inside BEGIN { RS = "\001" # whole command is one record hint["flutter test"] = "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." From 25908ecea2b6e811339732d0c673aa721c70ed0a Mon Sep 17 00:00:00 2001 From: Dominik Simonik Date: Mon, 5 Oct 2026 23:01:31 +0200 Subject: [PATCH 7/8] test: name the blocked commands literally The trigger words were assembled from fragments so the pair never appeared in this file, protecting shell edits of it from the hook under test. Single words are harmless and only a heredoc write was ever at risk, so the file now reads plainly and a header note covers that one case. Co-Authored-By: Claude Fable 5.1 --- hooks/scripts/block-cli-workarounds_test.sh | 220 ++++++++++---------- 1 file changed, 107 insertions(+), 113 deletions(-) diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index 51394d5..dfbb920 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -108,29 +108,24 @@ assert_reason_contains() { fi } -# The hook's own trigger strings, assembled at runtime. Writing them literally would -# make this file's own edits and greps trip the hook it tests -- which is the exact -# bug issue #147 reports. -FL="fl""utter" -TE="te""st" -VG="very_""good" -CR="cre""ate" +# The blocked commands appear literally below. Edit this file with Write/Edit, not a shell +# heredoc: the heredoc body is part of the command, and the hook under test would deny it. echo "=== block-cli-workarounds tests ===" stub_cli 1.5.0 echo "" echo "--- Should be BLOCKED ---" -assert_blocked "dart $TE" -assert_blocked "$FL $TE" -assert_blocked "dart $TE test/routing/foo_test.dart" -assert_blocked "$FL $TE --coverage" -assert_blocked "dart $CR my_app" -assert_blocked "$FL create my_app" -assert_blocked "$VG create flutter_app --project-name my_app" -assert_blocked "$VG $TE --coverage --min-coverage 100" -assert_blocked "$VG packages check licenses" -assert_blocked "cd /path && dart $TE" -assert_blocked "ENV=1 && $FL $TE --coverage" +assert_blocked "dart test" +assert_blocked "flutter test" +assert_blocked "dart test test/routing/foo_test.dart" +assert_blocked "flutter test --coverage" +assert_blocked "dart create my_app" +assert_blocked "flutter create my_app" +assert_blocked "very_good create flutter_app --project-name my_app" +assert_blocked "very_good test --coverage --min-coverage 100" +assert_blocked "very_good packages check licenses" +assert_blocked "cd /path && dart test" +assert_blocked "ENV=1 && flutter test --coverage" echo "" echo "--- Should be ALLOWED ---" @@ -138,14 +133,14 @@ assert_allowed "dart analyze lib/foo.dart" assert_allowed "dart format lib/foo.dart" assert_allowed "dart pub get" assert_allowed "dart fix --apply" -assert_allowed "$FL pub get" -assert_allowed "$FL analyze" +assert_allowed "flutter pub get" +assert_allowed "flutter analyze" assert_allowed "git add lib/router.dart test/router_test.dart" assert_allowed "dart analyze lib/foo.dart test/bar_test.dart" -assert_allowed "git commit -m 'fix dart $TE hook'" -assert_allowed "echo '$FL create is blocked'" -assert_allowed "gh pr create --body 'use dart $TE instead'" -assert_allowed "git log --grep='dart $TE'" +assert_allowed "git commit -m 'fix dart test hook'" +assert_allowed "echo 'flutter create is blocked'" +assert_allowed "gh pr create --body 'use dart test instead'" +assert_allowed "git log --grep='dart test'" assert_allowed "ls" assert_allowed "pwd" assert_allowed "" @@ -158,87 +153,86 @@ echo "--- Quoted arguments are arguments, not commands (issue #147) ---" # so an alternation listing the governed strings must not be chopped into subcommands. # Both positions matter: the bug only fired when the match was NOT the last alternative, # because then no closing quote attached to the token. -assert_allowed "grep -nE \"$VG|$FL $TE|foo\" CLAUDE.md" -assert_allowed "grep -nE \"$VG|$FL $TE\" CLAUDE.md" -assert_allowed "grep -nE \"$FL $TE|$VG|foo\" AGENTS.md" -assert_allowed "rg '$FL $CR|dart $CR' docs/" +assert_allowed "grep -nE \"very_good|flutter test|foo\" CLAUDE.md" +assert_allowed "grep -nE \"very_good|flutter test\" CLAUDE.md" +assert_allowed "grep -nE \"flutter test|very_good|foo\" AGENTS.md" +assert_allowed "rg 'flutter create|dart create' docs/" # A quoted phrase is a single word and runs nothing. -assert_allowed "echo \"$FL $TE\"" -assert_allowed "echo '$FL $TE'" -assert_allowed "git commit -m \"ban $FL $TE | dart $TE\"" +assert_allowed "echo \"flutter test\"" +assert_allowed "echo 'flutter test'" +assert_allowed "git commit -m \"ban flutter test | dart test\"" -# One quote type does not toggle state inside the other. -assert_allowed "echo 'say \"$FL $TE\" now'" -assert_allowed "echo \"say '$FL $TE' now\"" +# One quote type is inert inside the other. +assert_allowed "echo 'say \"flutter test\" now'" +assert_allowed "echo \"say 'flutter test' now\"" # Escapes and unbalanced quotes must not throw the scanner off. -assert_allowed "echo \\\"$FL $TE\\\"" -assert_allowed "grep \"$FL $TE file.md" -assert_blocked "$FL $TE --name \"my app\"" +assert_allowed "echo \\\"flutter test\\\"" +assert_allowed "grep \"flutter test file.md" +assert_blocked "flutter test --name \"my app\"" -# Quote state is carried across lines, because a quoted string may span newlines. -# Without that, the second line would start unquoted and read as an invocation. -assert_allowed "$(printf 'echo "hello\n%s %s\nworld"' "$FL" "$TE")" +# A quoted string may span newlines and is still one word. +assert_allowed "$(printf 'echo "hello\nflutter test\nworld"')" echo "" echo "--- Command position ---" # Every unquoted separator opens a fresh command position. The quoted-alternation # cases above prove a quoted `|` is inert; these prove an unquoted one still works. -assert_blocked "echo hi; $FL $TE" -assert_blocked "echo hi | $FL $TE" -assert_blocked "echo hi & $FL $TE" -assert_blocked "test -d lib || $FL $TE" +assert_blocked "echo hi; flutter test" +assert_blocked "echo hi | flutter test" +assert_blocked "echo hi & flutter test" +assert_blocked "test -d lib || flutter test" # A prefix does not stop something from being an invocation. -assert_blocked "ENV=1 $FL $TE" -assert_blocked "CI=true COVERAGE=1 dart $TE" -assert_blocked "($FL $TE)" -assert_blocked "\$($FL $TE)" -assert_blocked "\`$FL $TE\`" -assert_blocked "echo start && ($VG $TE)" +assert_blocked "ENV=1 flutter test" +assert_blocked "CI=true COVERAGE=1 dart test" +assert_blocked "(flutter test)" +assert_blocked "\$(flutter test)" +assert_blocked "\`flutter test\`" +assert_blocked "echo start && (very_good test)" # Any wrapper passes through to the command it runs. There is no wrapper list: pass 2 # tests every adjacent token pair, so a wrapper this suite never names is covered too, # whatever options it takes. -assert_blocked "fvm $FL $TE" -assert_blocked "command $FL $TE" -assert_blocked "env $FL $TE" -assert_blocked "env FOO=1 $FL $TE" -assert_blocked "env -i $FL $TE" -assert_blocked "sudo $FL $TE" -assert_blocked "sudo -u ci $FL $TE" -assert_blocked "nohup $FL $TE" -assert_blocked "exec $FL $TE" -assert_blocked "time $FL $TE" -assert_blocked "timeout 60 $FL $TE" -assert_blocked "nice -n 10 $FL $TE" -assert_blocked "xargs $FL $TE" +assert_blocked "fvm flutter test" +assert_blocked "command flutter test" +assert_blocked "env flutter test" +assert_blocked "env FOO=1 flutter test" +assert_blocked "env -i flutter test" +assert_blocked "sudo flutter test" +assert_blocked "sudo -u ci flutter test" +assert_blocked "nohup flutter test" +assert_blocked "exec flutter test" +assert_blocked "time flutter test" +assert_blocked "timeout 60 flutter test" +assert_blocked "nice -n 10 flutter test" +assert_blocked "xargs flutter test" # melos is how a VGV monorepo runs anything across its packages, options and all. -assert_blocked "melos exec -- $FL $TE" -assert_blocked "melos exec --concurrency 1 -- dart $TE" +assert_blocked "melos exec -- flutter test" +assert_blocked "melos exec --concurrency 1 -- dart test" # Shell keywords and brace groups open a command position like any other separator. -assert_blocked "if true; then $FL $TE; fi" -assert_blocked "for f in a; do $FL $TE; done" -assert_blocked "{ $FL $TE; }" -assert_blocked "while :; do dart $TE; done" +assert_blocked "if true; then flutter test; fi" +assert_blocked "for f in a; do flutter test; done" +assert_blocked "{ flutter test; }" +assert_blocked "while :; do dart test; done" # A path-qualified binary is the same command, so match on the basename. -assert_blocked "/usr/local/bin/$FL $TE" -assert_blocked "./$FL $TE" -assert_blocked "\$FLUTTER_ROOT/bin/$FL $TE" -assert_blocked "../sdk/bin/dart $TE" +assert_blocked "/usr/local/bin/flutter test" +assert_blocked "./flutter test" +assert_blocked "\$FLUTTER_ROOT/bin/flutter test" +assert_blocked "../sdk/bin/dart test" # ...but only when the wrapped command is itself blocked. -assert_allowed "fvm $FL pub get" -assert_allowed "command -v $FL" +assert_allowed "fvm flutter pub get" +assert_allowed "command -v flutter" assert_allowed "env | grep PATH" assert_allowed "melos exec -- dart analyze" assert_allowed "timeout 60 dart pub get" -assert_allowed "/usr/local/bin/$FL analyze" +assert_allowed "/usr/local/bin/flutter analyze" # A wrapper with nothing after it has no pair to match. assert_allowed "fvm" @@ -246,36 +240,36 @@ assert_allowed "env -i" assert_allowed "ENV=1" # A path whose basename only resembles the command must not match. -assert_allowed "git add lib/router.dart $TE/router_${TE}.dart" -assert_allowed "cp foo.dart $TE/" +assert_allowed "git add lib/router.dart test/router_test.dart" +assert_allowed "cp foo.dart test/" assert_allowed "ls bin/flutter_tools" # Comments are not modelled. A blocked command on a line after a comment is still # caught, and an apostrophe in a comment does not swallow the following lines. The cost # is that `; dart test` inside a comment is denied -- an accepted false positive, since # a `#` comment inside a tool call is not something the agent writes. -assert_blocked "$(printf '# a comment\n%s %s' "$FL" "$TE")" -assert_blocked "$(printf 'ls # note\ncd pkg\n%s %s' "$FL" "$TE")" +assert_blocked "$(printf '# a comment\nflutter test')" +assert_blocked "$(printf 'ls # note\ncd pkg\nflutter test')" assert_allowed "$(printf '# it%ss broken\nls -la' "'")" -assert_blocked "ls # fix; dart $TE" +assert_blocked "ls # fix; dart test" # `$( )` and backticks execute inside double quotes, so a blocked command there is # caught. Single quotes and a backslash-escaped `$` really are inert and stay allowed. -assert_blocked "OUT=\"\$($FL $TE)\"" -assert_blocked "echo \"\$($FL $TE)\"" -assert_blocked "echo \"\`$FL $TE\`\"" -assert_blocked "if [ -z \"\$(dart $TE)\" ]; then echo x; fi" -assert_allowed "echo \"\\\$($FL $TE)\"" -assert_allowed "echo '\$($FL $TE)'" +assert_blocked "OUT=\"\$(flutter test)\"" +assert_blocked "echo \"\$(flutter test)\"" +assert_blocked "echo \"\`flutter test\`\"" +assert_blocked "if [ -z \"\$(dart test)\" ]; then echo x; fi" +assert_allowed "echo \"\\\$(flutter test)\"" +assert_allowed "echo '\$(flutter test)'" assert_allowed "echo \"\$(date) building\"" assert_allowed "VAR=\"\$(ls)\"; dart analyze" # A quoted string handed to eval or sh -c is executed, so there the quotes are # delimiters, not data. -assert_blocked "eval \"$FL $TE\"" -assert_blocked "bash -c '$FL $TE'" -assert_blocked "sh -c \"$FL $TE\"" -assert_blocked "zsh -c \"cd pkg && dart $TE\"" +assert_blocked "eval \"flutter test\"" +assert_blocked "bash -c 'flutter test'" +assert_blocked "sh -c \"flutter test\"" +assert_blocked "zsh -c \"cd pkg && dart test\"" echo "" echo "--- Documented non-goals ---" @@ -284,14 +278,14 @@ echo "--- Documented non-goals ---" # A variable cannot be resolved without executing the command. The other three are forms # nobody types; catching them would need a character-level shell lexer in place of the # three substitutions, and the agent has never produced any of them. -assert_allowed "F=$FL; \$F $TE" -assert_allowed "\"$FL\" $TE" -assert_allowed "'dart' $TE" -assert_allowed "$(printf '%s \\\n%s --coverage' "$FL" "$TE")" +assert_allowed "F=flutter; \$F test" +assert_allowed "\"flutter\" test" +assert_allowed "'dart' test" +assert_allowed "$(printf 'flutter \\\ntest --coverage')" # A heredoc body is text, not a command position, but the scanner does not model # heredocs and denies it. Pinned as the known limitation it is. -assert_blocked "$(printf 'cat < Date: Mon, 5 Oct 2026 23:16:10 +0200 Subject: [PATCH 8/8] docs: one line per idea in the hook, its tests and its docs Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 2 +- README.md | 2 +- hooks/scripts/block-cli-workarounds.sh | 13 ++--- hooks/scripts/block-cli-workarounds_test.sh | 58 ++++++++------------- 4 files changed, 28 insertions(+), 47 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8322348..d875e9b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ These run **when a session begins**: 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.3.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 -- `Bash` matcher → `block-cli-workarounds.sh` — denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` run through the host's shell tool (`Bash`, or `Shell` on other hosts), redirecting to the MCP tool; stands aside for any other tool. Quoted text is data, so a command name inside a grep pattern or commit message never matches; `eval`, `sh -c`, and `$( )` or backticks in double quotes are scanned as commands. Every adjacent word pair is checked, so wrappers (`fvm`, `melos exec --`, `sudo`, `timeout`), shell keywords and `/path/to/flutter` need no list. The denial names the match, says the whole call was refused, and says so when the CLI cannot run because `dart` is off `PATH`. Pinned gaps: `"flutter" test` and `F=flutter; $F test` pass; an unquoted `echo flutter test` is denied +- `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. Every adjacent word pair is checked, so wrappers (`fvm`, `melos`, `sudo`, `timeout`) and `/path/to/flutter` need no list. The denial names the match and says the whole call was refused. Pinned gaps: `"flutter" test` and `$F test` pass; unquoted `echo flutter test` is denied The first two PreToolUse hooks are plugin-level (defined in `hooks.json`) and share common utilities from `vgv-cli-common.sh`. The following hook is **agent-scoped** — it is declared in the diff --git a/README.md b/README.md index c6b4be8..efa690c 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ This plugin includes SessionStart, PreToolUse, and PostToolUse hooks that valida | ---- | ------- | -------- | | **Warn Missing MCP** (`warn-missing-mcp.sh`) | SessionStart | Warns if the Very Good CLI is missing, older than 1.3.0, or present but unable to run because `dart` is not on the `PATH` hooks inherit; non-blocking | | **Check VGV CLI** (`check-vgv-cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Verifies from the payload that the caller is a Very Good CLI tool and stands aside otherwise, so the decision never lands on an unrelated tool; for its own tools, auto-approves the call in every run mode via a PreToolUse `allow` decision, so they never dead-end when the tool isn't on `permissions.allow` (including under `skipAutoPermissionPrompt`); denies with an install/upgrade message if the CLI is missing or < 1.3.0 | -| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` run through the shell, redirecting to the MCP tool. Quoted text is data (a command name in a grep pattern never matches) unless `eval`, `sh -c`, or `$( )` in double quotes runs it. Every adjacent word pair is checked, so wrappers like `fvm`, `melos exec --`, `sudo` and `timeout` need no list. The denial names the match and says the whole call was refused | +| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | 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. Every adjacent word pair is checked, so wrappers need no list. The denial names the match and says the whole call was refused | | **Allow Read-only Git** (`allow-readonly-git.sh`) | PreToolUse (`Bash`, `flutter-reviewer` agent only) | Restricts the `flutter-reviewer` agent's Bash to single-line `git diff`/`git status`, without the `--output` or `--ext-diff` options, and denies anything else (blocking). Scoped via the agent's frontmatter, not `hooks.json` | | **Analyze** (`analyze.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart analyze` on the modified `.dart` file; exits 2 on failure (blocking — Claude must fix issues before continuing) | | **Format** (`format.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart format` on the modified `.dart` file; always exits 0 (non-blocking — formatting is applied silently) | diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 2903d70..4ca16f2 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -1,7 +1,6 @@ #!/bin/bash -# PreToolUse hook: deny shell calls to CLI commands the Very Good CLI MCP tools cover. -# The hint table below is the list. To block another command, add a line there and a -# case in block-cli-workarounds_test.sh. +# PreToolUse hook: deny shell calls to CLI commands the MCP tools cover. +# To block another command, add a hint line below and a test case. if ! command -v jq &>/dev/null; then echo "jq is required for block-cli-workarounds hook but not found" >&2 @@ -34,8 +33,7 @@ fi # assumes the chained command's side effect happened and carries on without it. WHOLE_CALL_REFUSED="This whole shell call was refused, so none of it ran: run any other commands it chained in a call of their own." -# Deny with an install/upgrade message when the CLI is missing or outdated, with a PATH -# message when it is present but cannot run, and otherwise redirect to the MCP tool. +# Deny with a reason that fits the CLI status. deny_with_cli_check() { local mcp_hint="$1" matched="$2" local cli_status reason @@ -59,9 +57,8 @@ deny_with_cli_check() { deny "$reason $WHOLE_CALL_REFUSED Matched: $matched" } -# Deny when the command runs a blocked CLI. Quoted text is data unless something -# executes it. Every adjacent word pair is checked, so wrappers (fvm, melos exec --, -# sudo, timeout, shell keywords, /path/to/flutter) need no list. +# Quoted text is data unless eval, sh -c or $( ) runs it. Every adjacent word pair is +# checked, so wrappers (fvm, melos, sudo, timeout) and /path/to/flutter need no list. read -r -d '' find_invocation <<'AWK' || true # read, not $(cat): unbalanced parens inside BEGIN { RS = "\001" # whole command is one record diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index dfbb920..13a4dbc 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -66,8 +66,7 @@ run_hook() { run_hook_payload "$(jq -n --arg c "$1" '{"tool_input":{"command":$c}}')" } -# Label a case for the output. A command may be multi-line, which would wreck the -# aligned columns, so newlines are shown as a visible marker. +# Show newlines as ~ so multi-line commands keep the columns aligned. label_of() { printf '%s' "$1" | tr '\n' '~'; } assert_blocked() { @@ -108,8 +107,7 @@ assert_reason_contains() { fi } -# The blocked commands appear literally below. Edit this file with Write/Edit, not a shell -# heredoc: the heredoc body is part of the command, and the hook under test would deny it. +# Edit with Write/Edit, not a heredoc: the hook reads the heredoc body and denies it. echo "=== block-cli-workarounds tests ===" stub_cli 1.5.0 @@ -147,12 +145,9 @@ assert_allowed "" assert_allowed " " echo "" -echo "--- Quoted arguments are arguments, not commands (issue #147) ---" +echo "--- Quoted text is data (issue #147) ---" -# The regression that prompted the issue. A `|` inside a quoted regex is not a pipe, -# so an alternation listing the governed strings must not be chopped into subcommands. -# Both positions matter: the bug only fired when the match was NOT the last alternative, -# because then no closing quote attached to the token. +# A quoted `|` is not a pipe. Both positions, since the bug only fired mid-alternation. assert_allowed "grep -nE \"very_good|flutter test|foo\" CLAUDE.md" assert_allowed "grep -nE \"very_good|flutter test\" CLAUDE.md" assert_allowed "grep -nE \"flutter test|very_good|foo\" AGENTS.md" @@ -167,7 +162,7 @@ assert_allowed "git commit -m \"ban flutter test | dart test\"" assert_allowed "echo 'say \"flutter test\" now'" assert_allowed "echo \"say 'flutter test' now\"" -# Escapes and unbalanced quotes must not throw the scanner off. +# Escapes and unbalanced quotes. assert_allowed "echo \\\"flutter test\\\"" assert_allowed "grep \"flutter test file.md" assert_blocked "flutter test --name \"my app\"" @@ -178,14 +173,13 @@ assert_allowed "$(printf 'echo "hello\nflutter test\nworld"')" echo "" echo "--- Command position ---" -# Every unquoted separator opens a fresh command position. The quoted-alternation -# cases above prove a quoted `|` is inert; these prove an unquoted one still works. +# Unquoted separators open a command position. assert_blocked "echo hi; flutter test" assert_blocked "echo hi | flutter test" assert_blocked "echo hi & flutter test" assert_blocked "test -d lib || flutter test" -# A prefix does not stop something from being an invocation. +# A prefix is still an invocation. assert_blocked "ENV=1 flutter test" assert_blocked "CI=true COVERAGE=1 dart test" assert_blocked "(flutter test)" @@ -193,9 +187,7 @@ assert_blocked "\$(flutter test)" assert_blocked "\`flutter test\`" assert_blocked "echo start && (very_good test)" -# Any wrapper passes through to the command it runs. There is no wrapper list: pass 2 -# tests every adjacent token pair, so a wrapper this suite never names is covered too, -# whatever options it takes. +# Wrappers need no list: every adjacent word pair is checked. assert_blocked "fvm flutter test" assert_blocked "command flutter test" assert_blocked "env flutter test" @@ -210,17 +202,17 @@ assert_blocked "timeout 60 flutter test" assert_blocked "nice -n 10 flutter test" assert_blocked "xargs flutter test" -# melos is how a VGV monorepo runs anything across its packages, options and all. +# melos runs commands across a VGV monorepo. assert_blocked "melos exec -- flutter test" assert_blocked "melos exec --concurrency 1 -- dart test" -# Shell keywords and brace groups open a command position like any other separator. +# Shell keywords and brace groups are separators. assert_blocked "if true; then flutter test; fi" assert_blocked "for f in a; do flutter test; done" assert_blocked "{ flutter test; }" assert_blocked "while :; do dart test; done" -# A path-qualified binary is the same command, so match on the basename. +# Match on the basename. assert_blocked "/usr/local/bin/flutter test" assert_blocked "./flutter test" assert_blocked "\$FLUTTER_ROOT/bin/flutter test" @@ -239,22 +231,19 @@ assert_allowed "fvm" assert_allowed "env -i" assert_allowed "ENV=1" -# A path whose basename only resembles the command must not match. +# A basename that only resembles the command. assert_allowed "git add lib/router.dart test/router_test.dart" assert_allowed "cp foo.dart test/" assert_allowed "ls bin/flutter_tools" -# Comments are not modelled. A blocked command on a line after a comment is still -# caught, and an apostrophe in a comment does not swallow the following lines. The cost -# is that `; dart test` inside a comment is denied -- an accepted false positive, since -# a `#` comment inside a tool call is not something the agent writes. +# Comments are not modelled. `; dart test` inside one is denied: accepted, since the +# agent does not write comments in tool calls. assert_blocked "$(printf '# a comment\nflutter test')" assert_blocked "$(printf 'ls # note\ncd pkg\nflutter test')" assert_allowed "$(printf '# it%ss broken\nls -la' "'")" assert_blocked "ls # fix; dart test" -# `$( )` and backticks execute inside double quotes, so a blocked command there is -# caught. Single quotes and a backslash-escaped `$` really are inert and stay allowed. +# `$( )` and backticks run inside double quotes. Single quotes and `\$` are inert. assert_blocked "OUT=\"\$(flutter test)\"" assert_blocked "echo \"\$(flutter test)\"" assert_blocked "echo \"\`flutter test\`\"" @@ -264,8 +253,7 @@ assert_allowed "echo '\$(flutter test)'" assert_allowed "echo \"\$(date) building\"" assert_allowed "VAR=\"\$(ls)\"; dart analyze" -# A quoted string handed to eval or sh -c is executed, so there the quotes are -# delimiters, not data. +# eval and sh -c run their string. assert_blocked "eval \"flutter test\"" assert_blocked "bash -c 'flutter test'" assert_blocked "sh -c \"flutter test\"" @@ -274,17 +262,14 @@ assert_blocked "zsh -c \"cd pkg && dart test\"" echo "" echo "--- Documented non-goals ---" # -# These are allowed on purpose, and asserted so that changing one is a visible decision. -# A variable cannot be resolved without executing the command. The other three are forms -# nobody types; catching them would need a character-level shell lexer in place of the -# three substitutions, and the agent has never produced any of them. +# Allowed on purpose and pinned. A variable needs execution to resolve; the rest are +# forms nobody types. assert_allowed "F=flutter; \$F test" assert_allowed "\"flutter\" test" assert_allowed "'dart' test" assert_allowed "$(printf 'flutter \\\ntest --coverage')" -# A heredoc body is text, not a command position, but the scanner does not model -# heredocs and denies it. Pinned as the known limitation it is. +# Heredoc bodies are not modelled; denied as a known limitation. assert_blocked "$(printf 'cat <