guardrails: guard test suites leak enclosing-repo state (sequencer exemption flips blocked fixtures)

[kyle-sexton]

Problem

block-noncanonical-commit.test.sh (and any guard suite whose hook consults ambient repo state) is sensitive to the state of the ENCLOSING repository: the hook's sequencer_in_progress exemption resolves git rev-parse --absolute-git-dir from the process cwd, so running the suite inside a worktree with an in-progress merge (MERGE_HEAD present) silently flips all 26 "blocked" fixtures to allowed — 37/63 instead of 63/63 — with no hint the environment caused it.

Repro

In a worktree of this repo: git merge <anything> to the conflicted state (do not commit), then run bash plugins/guardrails/hooks/block-noncanonical-commit.test.sh → 26 failures. Commit or abort the merge → 63/63. Observed live 2026-07-22 while restacking PR #903's branch; cost ~20 min of misdiagnosis toward the PR's own changes.

Fix direction

The suite already isolates git config (GIT_CONFIG_GLOBAL=/dev/null); extend the same doctrine to repo state — run fixtures from a mktemp -d cwd (or an initialized scratch repo when a fixture needs one) so the hook's repo-state probes never see the developer's checkout. Audit sibling guard suites for the same leak.

Classification

Test-infrastructure isolation gap; no guard behavior defect (the sequencer exemption behaved as designed). Fail-safe direction (spurious FAILURES, never spurious passes) — deferrable per the direction test.

[kyle-sexton]

This was generated by AI (triage-lane planning subagent).

Implementation plan — #942: guard test suites leak enclosing-repo state

Root cause (confirmed at origin/main)

plugins/guardrails/hooks/block-noncanonical-commit.sh resolves the repo whose sequencer state it probes from the process cwd fallback chain, not from anything the test pins:

• effective_dir() (block-noncanonical-commit.sh:137-138) computes base="${HOOK_CWD:-${CLAUDE_PROJECT_DIR:-.}}".
• sequencer_in_progress() (:160-178) runs git -C "$repo" rev-parse --absolute-git-dir and then checks for MERGE_HEAD CHERRY_PICK_HEAD REVERT_HEAD rebase-merge rebase-apply; the final gate calls it at :306.

In block-noncanonical-commit.test.sh, the run() helper (:24-31) builds its payload via command_json (guardrails-test-helpers.sh:58-60), which emits no cwd field. So HOOK_CWD is empty; with CLAUDE_PROJECT_DIR unset (the issue's manual-repro case) the chain lands on . — the process cwd. Run the suite from inside a worktree with MERGE_HEAD present and sequencer_in_progress returns true for every fixture, so each expected-2 (blocked) -m fixture exits 0 and its assert_exit fails. The two direct command_json message-assertion calls (:212-214) share the same exposure. The persisted-alias probe git -C "$(effective_dir ...)" config --get "alias.$sub" (:242) resolves against the same leaked dir (secondary vector; not what flips the counts).

Reproduced in an isolated copy of the hooks (never touching the checkout): clean non-repo cwd → full pass; cwd inside a scratch repo with MERGE_HEAD → a block of "blocked" fixtures flip to pass; cwd inside a freshly-initialized clean repo nested inside that mid-merge parent → full pass again. The nesting case proves git init on the scratch cwd is immune even when $TMPDIR itself sits inside a repo.

Affected-sites enumeration (the core deliverable)

Closed set of guard hooks that consult ambient git repo state, found via git grep -nE 'repo_root|rev-parse|is-inside|show-toplevel|check-ignore' over plugins/guardrails/hooks/*.sh (excluding *.test.sh) — this catches indirect probes through hook::repo_root (hook-utils.sh:219-222, git -C "$hint" rev-parse --show-toplevel) that a raw git -C/MERGE_HEAD token grep misses:

Hook	Ambient probe	Dir source	Verdict
block-noncanonical-commit.sh	rev-parse --absolute-git-dir (sequencer) + config --get alias.*	effective_dir → process-cwd fallback, test sends no cwd	AFFECTED — fix here
flag-commit-pr-skill-bypass.sh	hook::repo_root "${CLAUDE_PROJECT_DIR:-.}" (:132)	cwd-defaulting, but the test always pins CLAUDE_PROJECT_DIR to a hermetic mktemp dir (flag-commit-pr-skill-bypass.test.sh:77)	Not affected — already isolated
hardcoded-path-check.sh	check-ignore (:101), rev-parse --show-toplevel (:121)	file-anchored to payload file_path; test unset CLAUDE_PROJECT_DIR + per-case pins (hardcoded-path-check.test.sh:20)	Not affected
cli-flag-verify.sh	hook::repo_root "$(dirname "$FILE")" (:81)	file-anchored; result feeds telemetry only, not the verdict	Not affected

No-probe siblings (grep-clean; only ${CLAUDE_PROJECT_DIR:-} for telemetry emit or string-prefix scoping, no git subprocess against ambient state): block-dangerous-git.sh, block-hook-bypass.sh, block-no-verify.sh, secret-pattern-detection.sh, workflow-resilience-check.sh.

Empirical invariance confirms the classifications: running each non-affected suite from a clean cwd vs. a cwd inside a mid-merge repo (with CLAUDE_PROJECT_DIR also pointed at it) yields identical PASS/FAIL — flag-commit-pr-skill-bypass 25/0 both, cli-flag-verify 33/10 both, hardcoded-path-check 16/24 across clean/mid-merge/plain-repo (its non-16 count is an unrelated missing-lib/ dependency in the isolated harness, not ambient sensitivity — the point is the count does not move with repo state). Only block-noncanonical-commit moves.

Fix

Extend the suite's existing config-isolation doctrine (export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null, block-noncanonical-commit.test.sh:21) from git config to git state, once and process-wide, immediately after that line and before the first fixture:

# Extend that isolation from git CONFIG to git STATE. The hook resolves the repo
# whose in-progress-sequencer state it probes from the payload cwd, falling back
# to $CLAUDE_PROJECT_DIR then the process cwd (effective_dir in the hook). The
# fixtures below send no cwd, so an unpinned fallback lets the probe read the
# DEVELOPER'S checkout: run inside a worktree mid-merge (MERGE_HEAD present) and
# every "blocked" fixture silently flips to allowed. A freshly-initialized
# scratch repo as cwd, plus a neutralized CLAUDE_PROJECT_DIR, anchors the
# fallback on clean state we own; `git init` (not a bare dir) makes the git-dir
# resolution terminate here even if $TMPDIR itself nests inside a repo. Cleaned
# by the existing TEST_TMPDIR trap.
STATE_CWD="$TEST_TMPDIR/state-cwd"
mkdir -p "$STATE_CWD"
git init -q "$STATE_CWD"
cd "$STATE_CWD" || exit 1
unset CLAUDE_PROJECT_DIR

Notes:

• git init runs after the GIT_CONFIG_* exports, so no ambient identity/config leaks in; init needs no user identity.
• cd is safe: HOOK/HOOK_DIR are already absolute (:10-11), the helper is sourced by absolute path, and every fixture path (TEST_TMPDIR, SEQ, PCFG, GITDIR) is absolute.
• STATE_CWD lives under TEST_TMPDIR, so the existing trap 'rm -rf "$TEST_TMPDIR"' EXIT (:13) already reclaims it — no new trap.
• No per-fixture edits: the process-wide approach also covers the two direct command_json message-assertion calls at :212-214, which a per-run()-callsite cwd injection would miss.

Decision defaulted: per-suite inline fix (not a shared isolate_repo_state helper) — veto before merge. Rationale: the existing config isolation is itself an inline per-suite export (deliberately "visibly process-wide" per its own comment), exactly one suite is affected today, and a shared helper for a single caller is speculative generality. Keeping the state isolation adjacent to the config isolation keeps both concerns visible in one place. Promotion trigger to a shared guardrails-test-helpers.sh function: a second guard suite appears whose hook resolves its repo-state probe from the process-cwd fallback and whose test cannot pin it via CLAUDE_PROJECT_DIR or file-anchoring (the two idioms the other three probing suites already use).

Acceptance

Applied to block-noncanonical-commit.test.sh (the patch above is validated in principle — a clean scratch-repo cwd nested inside a mid-merge parent restores a full pass — but the exact patch must be applied and re-run):

1. Normal run (clean checkout): suite passes its full count.
2. Issue repro: in a worktree of this repo, drive it to an in-progress merge (git merge <x> to conflict, do not commit — or equivalently create MERGE_HEAD), then bash plugins/guardrails/hooks/block-noncanonical-commit.test.sh → same full count, no flips. Abort/commit the merge → still full count.

Full count = the suite's own assertion total: 48/48 on origin/main today (38 run() + 10 direct asserts). The issue's "63/63" was observed while restacking PR #903's branch, which adds fixtures not yet on main; state acceptance against the suite's own count, not a hard-coded 63.

Classification: test-infra, fail-safe (spurious FAILURES never spurious passes), low-risk, single-suite, single-file. Fix pattern is unambiguous with one maintainer-vetoable fork (inline vs shared helper). Confidence: high.

[kyle-sexton]

This was generated by AI (triage-decide lane — intake routing).

Routed status: ready per T2 (ready-with-noted-decision). The planning subagent's plan (above) is high-confidence with an empirically-verified sites enumeration — only block-noncanonical-commit.test.sh is state-sensitive; the sibling suites (flag-commit-pr-skill-bypass, cli-flag-verify, hardcoded-path-check) were confirmed invariant to enclosing-repo state. Single-suite, single-file, test-infra, fail-safe (spurious failures never spurious passes).

Decision defaulted: per-suite inline repo-state isolation (mirroring the existing inline GIT_CONFIG_GLOBAL=/dev/null config isolation), not a shared isolate_repo_state helper — veto before merge. Promotion trigger to a shared helper is stated in the plan: a second probing suite that can't pin via CLAUDE_PROJECT_DIR/file-anchoring.

No open prerequisite gates this.

[kyle-sexton]

Hit this again live on 2026-07-26 while driving PR #1085, independently of the 2026-07-22 sighting. Recording the recurrence, a partial mitigation that has now landed, and one correction to the classification above.

Recurrence

Mid-merge in a worktree (MERGE_HEAD present from git merge origin/main), block-noncanonical-commit.test.sh reported:

passed: 74   failed: 30

Same mechanism, same misdiagnosis cost — the 30 failures read as damage from the PR's own hook changes, and I spent a pass tracing them into the diff before spotting MERGE_HEAD. The blast radius grows with the suite: 63 cases when this was filed, 104 when I hit it, 117 now.

Partial mitigation already landed (PR #1085)

The traversal-bound and nested-repository cases added in #1085 pin cwd explicitly in the payload rather than inheriting the suite's ambient cwd:

TRAVERSAL_CWD="$TEST_TMPDIR/traversal"
mkdir -p "$TRAVERSAL_CWD"
MSYS_NO_PATHCONV=1 jq -n --arg c "$command" --arg d "$TRAVERSAL_CWD" \
  '{tool_name:"Bash",tool_input:{command:$c},cwd:$d}' | timeout "$secs" bash "$HOOK"

That is the doctrine this issue asks for, applied to the new cases only. The pre-existing run() helper is untouched and still inherits ambient cwd, so this issue stays open — retrofitting run() across the pre-existing cases was deliberately kept out of #1085 (already four review rounds deep, and changing what those assertions exercise is not a late-PR edit). The pinning pattern above is a working in-repo reference for whoever does the retrofit.

Correction: the direction is NOT uniformly fail-safe

The classification above says "Fail-safe direction (spurious FAILURES, never spurious passes) — deferrable per the direction test." That holds for blocked-expectation fixtures but not for allowed-expectation ones, and the suite has many:

run "git commit -F - (allowed)" "git commit -F - --cleanup=verbatim" 0

When the sequencer exemption short-circuits, the hook returns 0 for everything. A fixture whose expectation is 0 therefore passes without exercising the logic it names — a spurious pass, not a spurious failure. In the run above, 74 cases reported green while the guard was exempting every commit-path case; the green signal on those was vacuous.

Practical consequence: the suite cannot be trusted to catch a regression in the block path while a sequencer is live, because the loud half (blocked cases) is drowned in noise and the quiet half (allowed cases) is green for the wrong reason. That is a stronger argument for the retrofit than "spurious failures cost debugging time", and I would re-weigh the priority: medium / deferrable call in that light — flagging rather than relabelling, since priority is the triager's call.

Scope note

The audit this issue asks for ("Audit sibling guard suites for the same leak") is still outstanding. One data point from #1085: block-dangerous-git.test.sh is not exposed via this mechanism — it has no sequencer probe, no persisted-config lookup, and no HOOK_CWD read; its run_in helper already pins cwd to a fixture repo because lease-width resolution requires a known repository. So the leak tracks "hook consults ambient repo state", exactly as stated above, and the sibling audit should key on that property rather than sweeping every suite.

[kyle-sexton]

Second independent sighting of the spurious-pass mode, from a different sequencer state than the earlier one. Recording it because two reproducers from two different mechanisms is a much stronger case than one.

Trigger: REVERT_HEAD, from git revert --no-commit

The earlier sightings were MERGE_HEAD (mid-git merge). This one is REVERT_HEAD:

$ git revert --no-commit <sha>          # leaves REVERT_HEAD in place
$ bash plugins/guardrails/hooks/block-noncanonical-commit.test.sh
...
passed: 96   failed: 30

sequencer_in_progress checks MERGE_HEAD CHERRY_PICK_HEAD REVERT_HEAD rebase-merge rebase-apply, so it correctly saw a live sequencer and correctly exempted every commit. The guard did exactly the right thing; the suite reported nonsense.

Confirmed the cause directly rather than inferring it:

$ GD=$(git rev-parse --absolute-git-dir)
$ for f in MERGE_HEAD REVERT_HEAD CHERRY_PICK_HEAD; do [ -e "$GD/$f" ] && echo "PRESENT: $f"; done
PRESENT: REVERT_HEAD

So at least three of the five sequencer states sequencer_in_progress recognizes will do this — MERGE_HEAD and REVERT_HEAD are now observed, and CHERRY_PICK_HEAD / rebase-merge / rebase-apply have no reason to behave differently. This is not "a thing that happens if you run the suite mid-merge"; it is a property of running the suite during any sequencer operation, which is a normal thing to be doing while working on this very guard.

Why this matters more than the failure count

96 cases reported green while the guard was exempting every commit path. Those 96 are not passes — a fixture whose expectation is 0 passes whenever the hook returns 0, and during a sequencer the hook returns 0 for everything. They passed without exercising the logic they name.

That is the spurious-pass mode, which the issue body currently rules out:

Fail-safe direction (spurious FAILURES, never spurious passes) — deferrable per the direction test.

The 30 loud failures are the fail-safe half. The 96 quiet ones are not, and they are the half that matters for a security guard: while a sequencer is live, this suite cannot catch a regression in the block path, because the blocked-expectation cases are drowned in noise and the allowed-expectation cases are green for the wrong reason.

Practical consequence

Anyone debugging this guard mid-merge or mid-revert — i.e. anyone resolving conflicts on a PR that touches it — gets a suite that is simultaneously alarming and worthless. Both sightings cost real misdiagnosis time; in this one, the 30 failures initially read as damage from the branch's own hook changes.

Not asking for a relabel

Priority is the triager's call, so this is a flag rather than an edit. But the "deferrable per the direction test" reasoning rests on the never-spurious-passes claim, and that claim is now falsified by two independent reproducers, so the deferral argument is worth re-deriving rather than inherited.

The fix shape in the issue body is unchanged and still right: run fixtures from a mktemp -d cwd (or an initialized scratch repo where a fixture needs one) so the hook's repo-state probes never see the developer's checkout. There is a working in-repo reference for the pattern now — the traversal, nested-repository, and cd fixtures added on #1085 pin cwd explicitly in the payload:

TRAVERSAL_CWD="$TEST_TMPDIR/traversal"
mkdir -p "$TRAVERSAL_CWD"
MSYS_NO_PATHCONV=1 jq -n --arg c "$command" --arg d "$TRAVERSAL_CWD" \
  '{tool_name:"Bash",tool_input:{command:$c},cwd:$d}' | timeout "$secs" bash "$HOOK"

The pre-existing run() helper is still unpinned, which is why this issue stays open.