Repository navigation
fix: make block-cli-workarounds quote-aware and wrapper-proof - #172
Merged
Merged
Conversation
ryzizub
marked this pull request as draft
October 5, 2026 13:22
ryzizub
force-pushed
the
session/wary-finch-555u
branch
2 times, most recently
from
October 5, 2026 14:14
01bbcad to
89e7e17
Compare
ryzizub
marked this pull request as ready for review
October 5, 2026 21:22
Contributor
|
Heads up — I'm auto-updating this PR by merging the latest |
erickzanardo
approved these changes
Oct 6, 2026
Contributor
|
Heads up — I'm auto-updating this PR by merging the latest |
Contributor
|
I tried to auto-update this PR with |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ryzizub
force-pushed
the
session/wary-finch-555u
branch
from
October 13, 2026 12:27
400db6f to
a078868
Compare
Two bugs found in review. An apostrophe inside "..." opened a single-quote span that swallowed the rest of the line, because the two quote passes ran one after the other rather than left to right. `echo "don't" && flutter test` was allowed. One pass now consumes each span in order, so a quote inside another quote cannot open its own. The $( ) pass stripped every double quote, which made all double-quoted text live as soon as `$(` appeared anywhere. That denied the ordinary `git commit -m "$(cat <<'EOF' ... EOF)"` whenever the message mentioned a governed command. A heredoc body with a quoted delimiter expands nothing, so it is dropped before scanning; an unquoted <<EOF still expands and is still scanned. Both forms are pinned by tests.
erickzanardo
approved these changes
Oct 19, 2026
The matcher was an awk program held in a bash string, read through `read -r -d ''` because a heredoc substitution would have choked on its parentheses. Two grammars in one file, with no highlighting and nothing able to lint either of them. It now lives in find-blocked-invocation.awk and runs via `awk -f`. The script checks the file is there and fails loudly if it is not, since a missing matcher would otherwise stand aside and drop enforcement silently. Readability changes inside the matcher: - the "data"/"subst"/"bare" mode string becomes two flags behind three named readings: as_written, as_substitution and as_eval_target - the three readings are computed where they are used, not all up front - the operator substitution that finds command starts is now split_on_operators, and scan is blocked_pair - single-letter locals get names No behaviour change: the same 140 tests pass.
Contributor
|
Heads up — I'm auto-updating this PR by merging the latest |
The hook is a bash script, and so is the hook beside it
(allow-readonly-git.sh, which does its matching with case globs and
read -ra). Keeping the matcher in awk meant a second grammar whose
behaviour rested on constructs proven on only two implementations, and an
awk that behaved differently would not error -- it would print nothing,
the hook would stand aside, and enforcement would disappear silently.
Walking quotes in bash also states the rule the earlier bug broke. Taking
the first quote and then its partner is what the shell itself does, so an
apostrophe inside "..." cannot open a span of its own. No regex engine is
involved and there is no leftmost-longest behaviour to rely on.
Two things measurement forced:
- blocked_pair splits on operators with tr, not ${s//[;&|...]/ }. A
bracket-class // over a long string runs in quadratic time in bash 3.2:
27s on an 8 KB command, against 35ms for the same work in tr.
- the hook now refuses to start without tr, the way it already refuses
without jq, so a missing tool cannot turn into a silent enforcement gap.
Verified against the awk version on 35 inputs covering quoting, heredocs,
substitution, eval, wrappers and unbalanced quotes: no decision differs.
The same 140 tests pass.
Cost: ~60ms on a command under 1 KB against awk's 35ms, rising to ~400ms
at 8 KB, where awk stayed flat. collapse_quotes is O(spans x length) and
that is the floor for this approach.
Checking every adjacent word pair is what lets the hook catch fvm, melos, sudo and a path-qualified binary with no list to maintain. The cost is that a command taking flutter or dart as an argument is refused too, and nothing recorded that. - the denial now says the match is on an adjacent pair anywhere in the call, so a surprising refusal reads as a guard misfiring rather than as a broken harness, which is what #60 and #147 both reported. It stops short of "quote it and it will pass": `"flutter" test` still runs, so that remedy would ship a bypass with every denial - `echo`, `man`, `which` and `very_good dart test --coverage` are pinned as denied. The last of those is listed as allowed in #147; the deviation is deliberate and now visible in a test rather than waiting to be rediscovered - all three repro commands from #60 are pinned as allowed; the third was not covered - the three readings get their names back as as_written, as_substitution and as_eval_target, instead of 0 0 / 0 1 / 1 1 at the call sites Tests 140 -> 146.
`very_good dart test` was refused because of the `dart test` inside it, so the denial told the operator not to use `dart test` -- a command they had not run. `very_good dart` is now a blocked pair of its own, and since the scan runs left to right it is reached first and the denial names the command that was actually invoked. `very_good dart` has exactly one subcommand, test, so matching the pair cannot over-reach. Also drops a claim I had added in the previous commit. It said refusing `very_good dart test --coverage` was a deliberate deviation from the table in #147. It is not: `very_good --help` documents the subcommand and the `very_good test` entry points at it, so the command does run the Dart test suite and refusing it is the point. That row of the table is wrong, and the old script had a real bypass there. The case moves to the blocked group with that recorded; the argument false positives (`echo`, `man`, `which`) stay where they were, since those are the genuine cost. Tests 146 -> 148.
ryzizub
changed the title
marcossevilla
approved these changes
Oct 20, 2026
erickzanardo
approved these changes
Oct 20, 2026
erickzanardo
left a comment
erickzanardo
left a comment
Member
There was a problem hiding this comment.
Found one additional suggestion, but approving already!
A quoted delimiter stops expansion, not execution. `bash <<'EOF'` and `cat <<'EOF' | sh` hand the body to a shell, which runs it line by line, but the body was dropped before anything looked at it. Caught in review. The body now stays in place when the line opening the heredoc invokes a shell, and is scanned like any other command. `git push <<'EOF'` is not a shell, and the usual `git commit -m "$(cat <<'EOF' ... )"` still has its body dropped as data. The two regex tests had to be ordered rather than combined: BASH_REMATCH holds the last match, so the delimiter has to be captured by the test that runs last. Combining them silently emptied the delimiter and left every heredoc body in place. Tests 148 -> 154. The five new denials fail against the previous script.
The row had grown to 756 characters, twice the next longest in the table, and most of it was matcher internals: basename matching, wrappers, heredoc delimiters, the pinned gaps. A reader of the README wants to know what is blocked, where it sends them, and why their call was refused. The internals stay in CLAUDE.md, which is where someone changing the hook looks.
erickzanardo
approved these changes
Oct 20, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #147.
The hook split on
[;&|]+with no quote awareness, sogrep "a|flutter test|b"was denied whilefvm flutter testran.What changed
Quoted spans are collapsed before anything is split, consuming the call left to right, so a quote inside another quote cannot open a span of its own. Quoted text is data unless
eval,sh -cor$( )runs it; a heredoc body with a quoted delimiter is data too. Matching is then on every adjacent word pair, compared on its basename — no wrapper list to maintain.The matcher is bash, not awk. The hook is a bash script and the hook beside it matches in bash; an awk that behaved differently would print nothing, stand aside, and drop enforcement without ever erroring.
Before / after
grep -nE "very_good|flutter test|foo" CLAUDE.mdfvm flutter test,melos exec -- flutter test,sudo -u ci flutter test/usr/local/bin/flutter test,if true; then flutter test; fieval "flutter test",bash -c 'flutter test',OUT="$(flutter test)"very_good dart test --coverageDeliberate costs, pinned by tests
echo flutter test,man flutter testandwhich flutter testare denied too. Narrowing to a command position would need exactly the wrapper list the pair rule exists to avoid."flutter" testandF=flutter; $F testpass. Catching those means reading inside quotes — the fix: block-cli-workarounds denies commands with a quoted regex alternation #147 bug — or running the command.For the reviewer
eval, wrappers and unbalanced quotes: no decision differs.very_good dartis a real subcommand, so allowing it was a bypass. Commented on the issue.tr, not${s//[…]/}: a bracket-class global substitution is quadratic in bash 3.2, 27s on an 8 KB command. The hook now refuses to start withoutjqortrrather than standing aside.🤖 Generated with Claude Code