fix(guard): quote-aware detection + path scoping — close 4 live false positives - #292
Merged
Conversation
branch-guard.sh's Bash write-through detection (Patterns 1-4) coarse-scanned
the raw command string for shell metacharacters with plain grep, which has
no notion of quoting. Three false positives fired live in one session,
including during an audit of this exact bug class:
awk 'NR>=203 && ...' -> '>' inside a single-quoted
awk program read as a redirect
grep -E '>[^=]' ... -> same, inside a grep pattern
grep -n '...\"cp \|...' ... -> literal "cp " substring inside
a quoted search pattern read
as a cp invocation
Same failure class Group 14c (2026-07-10) already fixed for heredoc bodies
via HAS_HEREDOC; this generalizes it to any quoted span, not just heredoc
bodies. COMMAND_SCAN strips quoted-span CONTENTS before the coarse `grep -q`
presence checks; extraction (`grep -oE`) still runs against the ORIGINAL
$COMMAND, so a real target quoted for spaces is unaffected.
A fourth, unrelated bug fired alongside it: `cp <file> /tmp/...` was flagged
as "creates a new code file on dev" even though /tmp is nowhere near the
repo — only /dev/* was excluded. Added a PROJECT_ROOT prefix check on the
resolved write target. PROJECT_ROOT is a realpath (`git rev-parse
--show-toplevel`); $CWD often isn't (macOS's /tmp -> /private/tmp, and
`mktemp -d` returns the /tmp form) — a naive literal-prefix compare broke
6 pre-existing in-repo-write tests during development. Fixed by
canonicalizing the target's directory via `cd && pwd -P` (POSIX, avoids
realpath/readlink -f, which aren't universally available) before comparing.
Verified by planted defect AND by replaying the actual live false positives
verbatim against the fixed hook (all resolve to exit=0), plus an isolated
confirmation that a genuine redirect on a fresh dev-branch repo still
returns exit=2 (blocked). New Group 22 (9 tests): 5 catch the false-positive
class (including the 2 real historical commands), 4 are regression guards
proving the fix doesn't weaken real detection (quoted target with spaces,
cp inside the repo, a real touch alongside an unrelated quoted decoy).
Full suite: 133/133 (was 124/133 mid-development, before the pwd -P fix).
E2E: 30/30 (1 skipped, unrelated). Full pytest: 2601 passed / 1 failed (the
1 is the expected repo-vs-installed-hook divergence this branch itself
causes, cleared by install-guards.sh — same pattern as PRs #290/#291).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Discovered while running this skill to audit the false positives fixed in 7b51969. Step 5's documented config schema (nested "branches" object, "version": 2, allowed_extensions, pr_body_scan, force_push_allow, dev_allowed_extensions) is read ZERO times by scripts/branch-guard.sh — grepped for every one of those keys, all 0 matches. The real schema is a flat branch-name -> protection-level map, read via a single lookup, _json_get ".\"${BRANCH}\"" (branch-guard.sh:234). Following the skill's own instructions would have produced a config file the guard silently ignores. Also removed a fabricated "destructive git in PR body" rule (grepped the script for pr_body/PR body/--body: 0 matches — no such scanning exists) and replaced Step 4's fictional recommendations with the two real friction classes this session actually found: quoted-span misdetection and missing path scoping, neither fixable via any config key. Added a classification step (Step 4) splitting false positives into branch-policy (config-fixable) vs. detection-logic (requires a code PR to branch-guard.sh, not a config change) — the skill's "never modify branch-guard.sh" constraint was previously silent on what to do when the bug IS in branch-guard.sh; now it says so explicitly rather than implying a config workaround exists for everything. Verified: skill_standards_audit.py 100/100, markdownlint clean, description 400 chars (well under the 1536 cap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7b51969's quote-stripping added printf | sed | sed (3 forks) to every Bash tool call on dev/main. dev's own dogfood perf budget (test_branch_guard_under_200ms) went from 207ms (pre-existing, already marginal on this machine — same class as the test_performance_cached_run xfail note re: unreliable wall-clock timing on shared runners) to 236ms. Collapsed to a single sed invocation (two -e expressions, here-string instead of printf|pipe) -> 3 forks to 1. Re-measured 0.80-0.90s per pytest invocation across 3 runs, consistently under budget. Full suite (133/133), E2E (30/30), and full pytest (2602 passed / 0 failed) all still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…D_SCAN Adversarial review of PR #292's own Group 22 tests surfaced a real false negative the quote-stripping fix introduced: two independent sed passes (single-quote pass, then double-quote pass) let a single-quote-pair span across two unrelated double-quoted strings that each contain one apostrophe (e.g. "it's" ... "don't"), erasing a real redirect sitting between them from COMMAND_SCAN. Confirmed live: a genuine `> new_file.py` write-through on dev went completely undetected. Fix: combine both quote types into one alternation pattern (`'...'|"..."`) so quote-type resolves at the first quote character encountered and can never pair across a boundary of the other type. Verified via planted-defect replay: the new regression test fails against the reverted two-pass sed and passes against this fix; full suite 134/134.
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.
The bug — reproduced 4 times live, once mid-investigation of the bug itself
branch-guard.sh's Bash write-through detection (Patterns 1-4) coarse-scans the raw commandstring for shell metacharacters with plain
grep— no notion of quoting. Text inside asingle- or double-quoted argument was scanned identically to real shell syntax:
The third one fired while auditing this exact bug class — my own
grepcommand, written toinvestigate the false positives, tripped the same bug it was investigating.
A fourth, unrelated bug fired alongside it:
cp <installed-hook> /tmp/...bakwas flagged as"creates a new code file on dev" even though
/tmpis nowhere near the repo — only/dev/*wasexcluded from path scoping, not general out-of-repo targets.
Fix
Quote stripping (
COMMAND_SCAN): strips quoted-span contents before the coarsegrep -qpresence checks in Patterns 1-4. Extraction (
grep -oE) still runs against the original$COMMAND, so a real target quoted for spaces is unaffected. Same failure class Group 14c(2026-07-10) already fixed for heredoc bodies via
HAS_HEREDOC— this generalizes it to anyquoted span, not just heredoc bodies.
Path scoping: a write target is now checked against a
PROJECT_ROOTprefix before beingflagged. Caught a real portability trap during development:
PROJECT_ROOTis a realpath (git rev-parse --show-toplevel), but$CWDoften isn't (macOS's/tmp→/private/tmp;mktemp -dreturns the
/tmpform) — a naive literal-prefix compare broke 6 pre-existing in-repo-writetests. Fixed by canonicalizing the target's directory via
cd && pwd -P(POSIX, avoidsrealpath/readlink -f, which aren't universally available — a documented gotcha in this repo).Perf: the quote-stripping added 3 process forks (
printf | sed | sed) to every Bash toolcall, which pushed the dogfood perf budget (
test_branch_guard_under_200ms) from analready-marginal 207ms baseline to 236ms. Collapsed to one
sedinvocation (two-eexpressions, here-string instead of
printf+pipe) — 3 forks to 1, consistently back underbudget (0.80-0.90s per pytest invocation across 3 runs).
Also: the guard-audit skill's config schema was fictional
Running
/craft:guard-auditto triage these false positives surfaced that its documented Step 5config schema (nested
"branches"object,"version": 2,allowed_extensions,pr_body_scan,force_push_allow,dev_allowed_extensions) is read zero times bybranch-guard.sh—grepped every key, 0 matches each. The real schema is a flat
branch-name → protection-levelmap read via one lookup,
_json_get ".\"${BRANCH}\"". Following the skill's own Step 5 wouldhave produced a config file the guard silently ignores.
Corrected the schema, removed a fabricated "destructive git in PR body" rule (0 matches for any
PR-body scanning in the script), and added a classification step: branch-policy false
positives are config-fixable; detection-logic bugs (like this PR's) are not — they need a
code PR, and the skill now says so instead of implying a config workaround exists for everything.
Test evidence
Planted defects + the real historical repro, not just a green run. All 4 live false
positives, replayed verbatim against the fixed hook:
Isolated confirmation a genuine redirect on a fresh dev-branch repo still blocks:
New Group 22 in
tests/test_branch_guard.sh(9 tests): 5 catch the false-positive class(including the 2 real historical commands verbatim), 4 are regression guards proving the fix
doesn't weaken real detection — a quoted target with spaces,
cpgenuinely inside the repo, anda real
touchbypass-marker creation alongside an unrelated quoted decoy.Update: adversarial review of this PR's own tests found a second real bug
Requested review of the new Group 22 tests (not the covered code) — treat each test as
potentially unable to fail, and check whether the fix it backs is actually correct. Reading
COMMAND_SCAN's quote-stripping line (sed -E -e "s/'[^']*'/'Q'/g" -e 's/"[^"]*"/"Q"/g') closelysurfaced a real false negative, more serious than any of the false positives this PR fixes:
Two independent
sedpasses — one for single quotes, one for double quotes — let asingle-quote-pair span across two unrelated double-quoted strings. A command containing two
ordinary contractions straddling a real redirect erases that redirect from the scan entirely:
Confirmed live against the actual hook on a fresh
dev-branch repo:exit=0— a genuine new.pyfile created via redirect ondevwent completely undetected. Root cause:'[^']*'doesn'tknow it's inside someone else's
"..."— with one apostrophe init'sand one indon't, thetwo independent passes pair those two unrelated apostrophes and treat everything between them
(including the real
> new_file.py && echo) as literal quoted text to strip.Fix: combine both quote types into a single alternation pattern (
'[^']*'|"[^"]*") instead oftwo independent passes, so quote-type is resolved at the first quote character encountered and can
never pair across a boundary of the other type. Verified same-cost (still one
sedprocess) anddoesn't regress any of the 9 original Group 22 cases.
Verified via planted-defect replay, per this session's standing discipline — a test is only
proven if it can be shown to fail on the bug it claims to catch:
Full suite re-verified after the fix:
tests/test_branch_guard.sh134/134 (was 133 + 1 new test).e2e_perf_50_invocationsfails locally on this machine (~10s vs 5s budget) but is confirmedpre-existing on
dev's own baseline (~9.8s, unrelated to this PR) and passes in CI's own log(
Passed: 30, Failed: 0) — a local-runner timing artifact, not a regression.Notes
~/.claude/hooks/branch-guard.shre-synced viainstall-guards.shduring development —same pattern as v4: folio split (Phases 3 + 3.5 + 3.6) — craft 94→47 commands #290/feat(check): add ci-bash-suites validator — close the gate-divergence gap #291 (this branch modifies the hook text, so repo/installed diverge until
synced).
git stash popmistake briefly polluted the working tree withan old, unrelated stash from a prior session (46 conflicted files, none touched by this PR).
Recovered cleanly: commits were never at risk (
git stashnever touches commit history), thepre-existing stash entries were verified intact throughout, and the working tree was restored
file-by-file against
HEAD. No commit in this PR was affected.🤖 Generated with Claude Code