fix(guardrails): read the payload cwd, and stop env -S hiding commands from every git guard

[kyle-sexton]

What

Two live holes on origin/main. One is specific to block-dangerous-git's lease-width probe; the
other is in the shared argv resolver and reached every guard in every plugin.

hook-utils.sh exists in 17 places — lib/hook-utils.sh plus a synced copy in each of 16
plugins — and all 17 were stale. An independent adversary confirmed the resolver hole is not
lease-specific: behind env -S, block-no-verify allowed git commit --no-verify and
block-dangerous-git allowed git reset --hard. All 17 copies are patched here.

It also proved the lease hole live rather than theoretical: in a SHA-256 repository carrying a ref
literally named 0123456789abcdef0123456789abcdef01234567, the cleared force push clobbered the
remote branch with unrelated orphan history, rc=0, with rev-parse captured before and after.

The guard allows --force-with-lease=<ref>:<expect> only when <expect> is a full-width object
id for that repository's hash format, because git cannot resolve one to something newer at push
time. Hex of the other width is an ordinary, movable ref name there — a 40-hex lease in a SHA-256
repository is exactly the hole --force-with-lease exists to close.

Route 1 — the payload's cwd was never read. The probe ran git rev-parse --show-object-format from the hook process's directory. Claude Code launches hooks from the
session root and runs the Bash tool wherever the session stands, so the two differ routinely. No
wrapper and no cd were required: a plain git push was enough.

Route 2 — env -S / --split-string spliced options past the parser. -S exists so a shebang
line can pass OPTIONS to env (#!/usr/bin/env -S -i prog), so its split words are env's own
arguments. hook::git_resolve_index spliced them back into its scan but resumed at the command
dispatcher, which read a leading option in the split string as the command NAME and abandoned the
segment. env -S '-C <dir> git push --force' resolved to no git at all — so this was not only a
lease-width hole; a bare env -S '-v git push --force' also went unexamined.

The fix

• The payload's .cwd is read and replayed as a leading -C, ahead of
  HOOK_GIT_RESOLVED_WRAPPER_DIRS, which already precede git's own options. That reproduces
  execution order end to end and composes under git's own rules — a later -C composes onto an
  earlier one, an absolute one wins — so it is the same mechanism the wrapper replay already ships,
  with a first term added. Not a cd: a cd would move the hook process and leak across the
  recursive alias walk.
• The base chain is HOOK_EFFECTIVE_BASE → HOOK_CWD → CLAUDE_PROJECT_DIR → ., adopted
  verbatim from block-noncanonical-commit rather than invented a second time.
  HOOK_EFFECTIVE_BASE is not decoration: a ! shell alias runs its body as a fresh command in the
  relocated repository, so the base is relocated for that reparse and save/restored around it. This
  guard recurses through ! aliases the same way the sibling does.
• hook::git_resolve_index resumes inside env's own option loop after an -S splice. That also
  keeps env's single chdir slot last-wins across the splice (env -C a -S '-C b git …' lands in
  b), matching GNU env.
• The repo_oid_width known-gap docblock is restated at its real width (see below).

Behaviour change, stated so it is not read as a regression

A RELATIVE -C / --git-dir / --work-tree / --namespace now rebases onto the payload cwd
instead of the hook process's directory. That is the correct resolution — a relative path written in
a tool call means relative to where that call runs — and it is a change only in the sense that the
previous answer was measured from the wrong origin. An absolute one is unaffected. Cases 4b/4c
below pin it, and there is a test for the absolute form staying put.

One further consequence of adopting the sibling's chain: with no .cwd in the payload at all,
CLAUDE_PROJECT_DIR is preferred over the hook process's directory. A real PreToolUse payload
always carries cwd, and this matches block-noncanonical-commit; case 5b pins it either way.

Verification

Every row was run against both trees from one script — PRE is origin/main extracted verbatim,
POST is this branch — over real SHA-1 and SHA-256 fixture repositories. Exit 2 = BLOCKED, 0 =
ALLOWED. Two independent liveness columns, because a table can be inert in two different ways:

• pPOST — the width the hook's own probe resolved, scraped from bash -x (_repo_oid_width=NN).
  The guard fails closed on width 0, so a BLOCK from 0 is fail-closed noise, not the fix working.
  Every POST=BLOCKED row below resolved a real width.
• EXEC — what the command's git actually does: the push replaced by rev-parse --show-object-format, the exact wrapper form run for real from the payload cwd. A form that never
  reaches git is not a bypass.

case	PRE	POST	pPRE	pPOST	EXEC	what it pins
1a	0	2	40	64	sha256	payload cwd = SHA-256 repo, hook process in SHA-1 one, 40-hex lease — the bypass
1b	2	2	64	64	sha256	control: both directories agree; fixture discriminates
1c	2	0	64	40	sha1	opposite direction — payload cwd = SHA-1 repo, 40-hex is a genuine object id where it runs
2a	0	2	–	64	sha256	env -S '-C <sha256> git …'
2b	0	2	–	64	sha256	env --split-string='-C <sha256> git …'
2c	0	2	–	–	sha1	env -S '-v git push --force' — a plain force push hidden behind a leading option
2d	2	2	–	–	sha1	no-regression: env -S 'git push --force' (no leading option) was and stays blocked
2e	0	2	–	64	sha256	env -C <sha1> -S '-C <sha256> …' — one slot, last wins
2f	0	0	–	40	sha1	env -C <sha256> -S '-C <sha1> …' — last wins the other way (semantics pin, paired with 2e)
3a	0	2	40	64	sha256	git -C <sha256> -c alias.y='!git <lease>' y — the ! body runs in the relocated repo
3b	2	0	64	40	sha1	opposite direction through the same ! path
4a	2	2	64	64	sha256	relative git -C with both directories agreeing — unchanged
4b	2	0	0	40	sha1	relative git -C resolves against the payload cwd (PRE probed width 0 — it was resolving nothing)
4c	2	0	0	40	sha1	relative --git-dir rebases the same way — the disclosed change
5a	2	2	64	64	sha256	no .cwd, no CLAUDE_PROJECT_DIR → . (pre-fix behaviour preserved)
5b	2	0	64	40	sha256	no .cwd → CLAUDE_PROJECT_DIR (chain rung 2; EXEC differs because the divergence is synthetic)
6a	0	0	–	–	(none)	inert-form control: env FOO=1 -C <dir> git … — coreutils stops at NAME=VALUE, rc 127, git never runs, so there is nothing to block

– in a probe column means no probe ran (no lease expectation on that row, or no git resolved).

Every case that claims a fix carries a control that FAILS against origin/main: 1a, 2a, 2b, 2c,
2e, 3a (PRE allowed, POST blocked) and 1c, 3b, 4b, 4c, 5b (PRE blocked, POST allowed). 1b, 2d, 4a,
5a and 6a answer the same on both trees by design and are labelled as controls, not as evidence.

Regression coverage added

• plugins/guardrails/hooks/block-dangerous-git.test.sh — 341 → 363 pass / 0 fail. run_in now
  states the payload cwd alongside the process directory (without it the suite silently measures
  CLAUDE_PROJECT_DIR, i.e. the host repository, in any session that exports it); run_split and
  run_nocwd cover the divergent and degraded payload shapes.
• lib/hook-utils.test.sh — 164 pass / 0 fail, with resolver-level env -S cases including the
  attached-operand spelling, the last-wins slot across a splice, and a self-referential
  env -S '-S -S' termination check.

Not in scope, deliberately

• A shell cd relocation (cd X && git push …, (cd X && …), sh -c 'cd X && …'). Resolving
  it means evaluating arbitrary shell word expansion, which this guard deliberately does not do. It
  remains a documented gap — and the docblock describing it is corrected in this PR, because it
  listed a "compound cd" as one of three required conjuncts when at the time none of them were
  required. A documented gap that reads narrower than it is, is how this one survived review.
• A persisted (config-file) alias carrying the lease (git config alias.yolo 'push --force-with-lease=…' then env -C <dir> git yolo). This guard resolves inline -c aliases only;
  persisted-alias resolution is a separate capability block-noncanonical-commit has and this one
  does not. Flagged in fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124 for triage, not asserted there as a bypass.
• An explicit --git-dir / --work-tree inherited by a ! shell-alias body. git EXPORTS them
  into the body's environment (verified on git 2.54.0 — the body prints sha256 from a SHA-1
  directory and sees GIT_DIR set), so the body works in a repository the composed directory does
  not name. effective_dir composes -C only, so the lease is judged against the base.
  Reproduced against BOTH origin/main and this branch (PRE=0, POST=0, EXEC=sha256) — it is
  pre-existing and of the same family, not introduced here, and closing it means replaying the
  inherited globals rather than a directory: a larger mechanism than the base chain fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124's design
  section scopes this change to. Now documented in the effective_dir docblock and the CHANGELOG
  rather than left implicit, on the same principle that motivated the docblock correction above.
• The claimed relative-git -C misprobe that does not reproduce. fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124 records it as tested
  against origin/main and not reproducing — the relative form resolves against the hook process's
  cwd and the command's cwd, which are the same directory in that scenario. It is subsumed by
  route 1, not separate, and no separate change was made for it.

Two findings from adversarial review, folded in

• A false git semantic in the diff's own prose. It said a ! shell-alias body "starts in THIS
  segment's relocated directory". Measured: a ! body runs from the repository top level, not
  the caller's directory (alias.wd='!pwd' from <repo>/sub prints <repo>). The conclusion is
  unchanged — an object format is a property of the repository, and the composed directory and its
  top level are the same repository — but the claim is corrected rather than left load-bearing on a
  wrong premise.
• An unexplained asymmetry that turned out to be correct. effective_dir composes only -C
  while collect_git_locating_opts also replays --git-dir/--work-tree/--namespace. The
  reviewer expected a bug and found it right: only -C relocates a ! body (git -C <other> -c alias.wd='!pwd' wd moves, git --git-dir=<other> … does not). A comment now says why, so the
  next reader does not file it as the bug this one nearly did.

The known gap's primary symptom is a FALSE BLOCK, not a bypass

Worth stating plainly because reviewers reasonably read "known gap" as "hole": with a shell cd,
the probe measures a base that is frequently not a repository at all, answers width 0, and fails
closed. So

cd <repo> && git push --force-with-lease=main:<literal full-width sha> origin main   -> BLOCKED

— the exact form the guard's own block message prescribes — is denied from a session root that is
not itself a repository. Fail-closed is the right default for an unresolvable base, and this is not
a regression (it behaves the same on origin/main), but the docblock now records the false block as
the symptom to measure, because a guard that refuses correct usage it just recommended teaches
people to route around it.

Conversely, the fix removes a false block as well as a bypass: the inverse-skew row (hook
process in SHA-256, payload cwd in SHA-1, 40-hex lease) goes DENY → ALLOW, which is correct because
that word is a genuine object id where the command runs.

What was NOT tested — carried forward rather than buried

• No PowerShell payloads were used by the adversarial pass at all. The guard matches
  Bash|PowerShell, so the entire lease-width and env -S surface is unverified on that arm by the
  adversary. This branch adds PowerShell cases of its own (payload-cwd pinning plus a missing-cwd
  tool-name case) but they do not cover the env -S surface.
• hook::require_jq was not read, and this guard now requests three payload fields instead of
  two. The behaviour when jq is absent — the guard skipping entirely — is a separate, already-filed
  concern, not something this branch changes.
• The abbreviated-hex rows (7 and 12 hex) were examined and deliberately not "fixed": ambiguity
  with a short ref name is real, and blocking them is correct.
• +refspec force detection held on every form tried; -S termination held across six degenerate
  operands under a 25 s timeout.
• The 13/0 PRE-vs-POST discrimination split reproduced twice, but the final uncontended full pass
  was still running when the adversary reported.

Blast radius

lib/hook-utils.sh is a synced library: scripts/sync-hook-utils.sh distributes it to every plugin
carrying hooks/hook-utils.sh — 16 plugin copies plus the lib/ source, 17 files, all stale on
origin/main — and each plugin must bump so consumers receive the change. All 16 carrying plugins
are bumped with a CHANGELOG entry; guardrails takes a minor bump (0.23.1 → 0.24.0) for the
behaviour change above, the other 15 take a patch. scripts/sync-hook-utils.sh --check-bump origin/main and scripts/check-changelog-parity.sh --check-bump origin/main both pass, as do
--check-order, check-silent-skips.sh and check-cross-plugin-source-drift.sh --check.

Closes #2124

Related

• fix(guardrails): block --force-with-lease forms that state no expected value #1275 — where PRRT_kwDOTCGFQM6TzGBZ was filed
• fix(guardrails): replay a wrapper's chdir in the lease hash probe and rescope skill-reference-verify reconstruction #2100 — the partial fix this completes, and the round-one verification that wrongly closed the thread
• Triage the remaining stranded P2 review findings from the post-merge sweep #1938 — the stranded post-merge review-findings sweep
• perf(guardrails): parse each hook payload in one jq process, not two or three #2120 — the previous lib/hook-utils.sh change, whose 15-plugin fan-out this one mirrors

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 3m 57s —— View job

---

Claude finished the review

• Gather context (PR description, changed files list)
• Review lib/hook-utils.sh (synced to 16 plugin copies) — env -S resolver fix
• Review plugins/guardrails/hooks/block-dangerous-git.sh — payload cwd fix
• Review test coverage (lib/hook-utils.test.sh, block-dangerous-git.test.sh)
• Spot-check CHANGELOG/version bump parity across plugins
• Post findings as inline comments + summary

Summary

This is a careful, well-scoped fix. I traced both changes against the surrounding code rather than taking the PR body's claims at face value:

hook::git_resolve_index (lib/hook-utils.sh, all 17 synced copies identical by blob SHA 805f68da...) — changing continue 2 to continue after an -S/--split-string splice is correct: it resumes inside env's own option-parsing while loop rather than jumping to the outer command-dispatch loop, which is what let a leading option in the split string (e.g. -C, -v) be misread as the command name and abandon the whole segment. I traced env -S '-S -S' by hand through hook::env_s_split and confirmed it terminates in 3 splices (argv shrinks to empty), matching the new regression test at lib/hook-utils.test.sh#L2065-2069.

block-dangerous-git.sh payload-cwd fix — I traced the full chain: HOOK_EFFECTIVE_BASE is initialized once at top-level (before hook::bash_parse_segments runs) from HOOK_CWD → CLAUDE_PROJECT_DIR → .; collect_git_locating_opts prepends it as a leading -C; effective_dir (new) recomputes the composed directory for !-alias reparses using git_locating_opts[@]:2 (correctly skipping the base pair it already defaults to, avoiding double-application); and the reparse save/restores HOOK_EFFECTIVE_BASE around the recursive hook::bash_parse_segments call so the relocation doesn't leak to sibling segments. This all matches block-noncanonical-commit.sh's existing effective_dir/base-chain pattern byte-for-byte (verified by diffing the two), so it isn't a new mechanism — it's adopting a working one.

Test coverage is substantial and targets the actual mechanism, not just outcomes — run_split pins the payload-cwd-vs-hook-process divergence in both directions, !-alias relocation and its non-leak into a following segment, the relative-vs-absolute -C/--git-dir distinction, and the full base-chain fallback (run_nocwd). The env -S cases cover the attached-operand form, last-wins across a splice in both directions, and the "inert form" control (env FOO=1 -C ..., which correctly never reaches git). I did not execute the test suites myself — this review sandbox's bash <script> execution requires interactive approval that wasn't available here — so the "363 pass / 0 fail" and "164 pass / 0 fail" counts in the PR body are author-claimed and unverified by me; I only confirmed the test assertions are well-targeted by reading them.

CHANGELOG/version bumps: spot-checked guardrails (0.23.1→0.24.0, minor — correct, this has a documented behavior change) and actionlint (0.8.1→0.8.2, patch) plus several others; all 16 non-guardrails plugins take a patch bump and their CHANGELOG entries accurately summarize the shared hook-utils.sh fix. No unrelated files are touched — the 52 changed files are exactly lib/hook-utils.sh + 16 synced copies + their CHANGELOGs/plugin.json bumps + the guardrails-specific guard/test/CHANGELOG changes.

One low-severity, pre-existing edge case I looked at but am not filing as a new finding: effective_dir's textual -C composition would misparse a directory whose two-word --git-dir value is literally the string -C (it would misread that value as a fresh -C flag on the next loop iteration). I confirmed this is byte-identical to block-noncanonical-commit.sh's existing effective_dir (both at block-noncanonical-commit.sh#L417-L432) — it's inherited verbatim from the sibling guard exactly as the PR states ("adopted verbatim from block-noncanonical-commit"), not introduced by this change, and requires a directory literally named -C, so I don't think it's worth a finding here.

No inline findings — I didn't find a correctness bug, error-handling gap, or test-coverage hole in the changed logic itself.

[claude]

Claude finished @kyle-sexton's task in 3m 13s —— View job

---

Security review complete

Scope: lib/hook-utils.sh (shared resolver, synced verbatim to 16 plugin copies), plugins/guardrails/hooks/block-dangerous-git.sh, and the two associated test files. Version/CHANGELOG-only files across the other 15 plugins were spot-checked and contain no functional changes (confirmed all 16 hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh via cmp).

• Gather context (PR diff, changed files)
• Read lib/hook-utils.sh diff (shared resolver, synced to 16 plugins)
• Read plugins/guardrails/hooks/block-dangerous-git.sh diff (lease-width probe)
• Read test files for behavior confirmation
• Check plugin.json/CHANGELOG diffs for anomalies
• Compile findings and post review

Findings

No security issues found. This PR is itself a security-hardening fix, and I could not identify a new vulnerability it introduces.

Notes from the review (not findings, just what I checked and ruled out):

• env -S resume fix (lib/hook-utils.sh#L1172-L1189): changing continue 2 → continue correctly resumes inside env's own option-parsing loop (the while ((i < n)) at line 1131) rather than the outer command-dispatch loop. Traced the control flow by hand against the described env -S '-C <dir> git push --force' bypass — the fix closes it as claimed, and the shared array/index vars (i, n, w) are function-scoped so the splice-and-resume is safe across the nested loop boundary.
• Payload .cwd as probe base (block-dangerous-git.sh#L337-L344, #L403-L426): the value is passed to git as a discrete -C argv element (never through eval/sh -c/string interpolation into a shell command), so there's no injection surface from an attacker-shaped path string. It reflects the real directory Claude Code's harness reports the tool call runs in, so using it as the probe's origin makes the probe more accurate, not less trustworthy.
• HOOK_EFFECTIVE_BASE save/restore across the ! shell-alias recursion (#L643-L695): checked for a state-leak path where a relocated base could persist into a sibling segment (which would under-probe a later command). Every entry into the relocated state is paired with an unconditional restore on the same code path (including when alias_reexpand_admit short-circuits), and local scoping gives each recursive check_segment call its own saved_base. The test suite includes a case for exactly this leak ("a segment after a relocating ! alias is measured from the payload cwd again").
• effective_dir() (#L409-L426) does pure textual path composition (no realpath/subprocess), consistent with the PR's stated rationale for avoiding symlink-resolution bypass in either direction.
• No eval, sh -c, or string-built commands were introduced; all git invocations use array expansion (git "$@" / "${git_locating_opts[@]}").
• No workflow files, permissions blocks, or dependency pins changed in this diff — the fan-out to 15 other plugins is version/CHANGELOG-only (verified via diff for a representative sample, including claude-ops's larger diff, which is a — → literal em-dash normalization plus the version bump, nothing functional).
• The PR's own "Not in scope, deliberately" section (shell cd relocation, persisted-alias lease, !-alias GIT_DIR inheritance) matches what I independently traced through the code — these are honestly disclosed pre-existing/residual gaps, not issues newly introduced or newly hidden by this change.

I was unable to execute the test suites in this sandboxed environment (script execution requires interactive approval not available here), so the 363/164 pass counts in the PR description are unverified by me, but the code-level reasoning for the fix checks out against manual trace-through and the diff's own regression tests.

[github-actions]

Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d39f49ebca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 4m 34s —— View job

---

Code review complete

• Gather context (PR description, diff, changed files)
• Review lib/hook-utils.sh resolver fix (correctness, maintainability)
• Review plugins/guardrails/hooks/block-dangerous-git.sh payload-cwd fix
• Review test coverage (lib/hook-utils.test.sh, block-dangerous-git.test.sh)
• Check CHANGELOG/version bump parity across the 16 synced plugins
• Note existing Codex finding (alias memo key) for context
• Post findings as inline comments + summary

Scope note

This is the correctness/architecture/maintainability lane; security aspects are left to the dedicated security-review lane (already run on this PR). I did not re-run the test suites myself — no shell-execution tool is available to this lane — so the "363/164 pass" counts in the PR body are author-claimed and unverified by me. All statements below are from reading the diff and tracing the control flow by hand.

Findings

One inline comment posted:

• block-dangerous-git.sh:693 — the ${git_locating_opts[@]:2} slice depends on collect_git_locating_opts always prepending exactly the base's -C <dir> pair before anything else. That's true today and I confirmed it's correct (avoids double-applying the base, since effective_dir defaults to the same HOOK_EFFECTIVE_BASE), but the magic offset carries no comment at the point of use, unlike the rest of this PR's unusually well-documented style. block-noncanonical-commit.sh's equivalent call site avoids the coupling entirely (its callers pass wrapper_cd + git's own global slice directly, never prepending the base into the sliced array). Low severity — not a functional bug, just a latent trap for a future edit to collect_git_locating_opts.

What I verified independently

• hook::git_resolve_index's continue vs continue 2 fix (lib/hook-utils.sh:1172-1189): traced the nesting by hand — the -S/--split-string case sits inside env's own while ((i < n)) option loop (line 1131), itself nested inside the outer segment-scan while (line 1110). continue correctly resumes the inner loop; the old continue 2 jumped to the outer loop's command dispatcher, which is exactly the bug described. Hand-traced env -S '-S -S' through the splice logic and confirmed it terminates in 3 iterations (matches the new regression test at lib/hook-utils.test.sh:2065-2069), and confirmed env -C a -S '-C b …' is last-wins because env_ci stays in scope across the continue.
• The HOOK_EFFECTIVE_BASE chain and !-alias relocation in block-dangerous-git.sh: collect_git_locating_opts (line 342) leads with -C "${HOOK_EFFECTIVE_BASE:-${HOOK_CWD:-${CLAUDE_PROJECT_DIR:-.}}}"; effective_dir (line 408) composes the same chain plus every -C seen so far; the reparse save/restore around the !-alias recursion (lines 663, 693, 696, 710-711) is symmetric on every path I could find — I did not find a return/exit between the save and restore that would leak a relocated base into a sibling segment.
• The alias-memo Codex finding is already fixed in this branch. The Codex review comment (posted against commit d39f49e) flagged that HOOK_ALIAS_MEMO's key didn't include the effective base, so a memoized verdict from one repository could wrongly be reused in another. The latest commit, bc3cf8e ("fix(guardrails): key the alias re-expansion memo on the effective base"), addresses this directly: alias_reexpand_admit (line 547) now folds HOOK_EFFECTIVE_BASE into the memo key via printf -v q '%q' before ${#HOOK_ALIAS_SEEN[@]}. The new test at block-dangerous-git.test.sh (memo_alias cases, "the same alias text under a SHA-1 then a SHA-256 -C: the memo must not reuse the first base's verdict") pins exactly this scenario. I consider that finding resolved as of HEAD.
• repo_oid_width's cache (line 276-286): its key is "$*" over the full git_locating_opts array, which now leads with the base — so it's base-keyed by construction, consistent with the CHANGELOG's claim that this cache needed no separate fix.
• Test coverage in both test files targets the actual mechanism (payload-cwd-vs-process divergence in both directions, !-alias relocation and non-leak into a following segment, relative-vs-absolute -C/--git-dir, the full base-chain fallback, env -S last-wins/termination/inert-form cases) rather than just outcomes. I didn't find a gap in what's being pinned.
• CHANGELOG/version-bump parity: spot-checked guardrails (0.23.1→0.24.0, minor bump — correct given the documented relative--C behavior change), plus actionlint and typos-format (both 0.x.1→0.x.2 patch bumps) — all three CHANGELOG entries accurately describe the shared hook-utils.sh fix or the guardrails-specific fix, and I didn't spot any unrelated file changes riding along.

No correctness bugs, error-handling gaps, or test-coverage holes found beyond the one low-severity note above.

[claude]

Claude finished @kyle-sexton's task in 2m 36s —— View job

---

Security review complete

Scope: files changed in this PR (gh-diff against origin/main), with focus on the newest commit bc3cf8e pushed since the two prior automated reviews on this PR — it lands the fix for the P1 finding Codex flagged on HOOK_ALIAS_MEMO's key composition.

• Gather context (git log origin/main..HEAD, diff of the new commit)
• Review lib/hook-utils.sh diff (shared resolver, synced to 16 plugin copies) — env -S resolver fix
• Review plugins/guardrails/hooks/block-dangerous-git.sh diff (payload cwd / lease-width probe)
• Verify the Codex P1 finding on HOOK_ALIAS_MEMO key composition
• Spot-check the 15 version/CHANGELOG-only plugin diffs for anomalies

Findings

No security issues found. The reported P1 has been fixed, and I found nothing new.

Codex's HOOK_ALIAS_MEMO finding — verified fixed in commit bc3cf8e (block-dangerous-git.sh#L547-L566). The exact bypass Codex described — same ! alias text re-expanded once under a SHA-1 -C (where a 40-hex lease is a genuine object id, correctly allowed and memoized) and again under a SHA-256 -C (where the identical text should be blocked) — is closed by folding HOOK_EFFECTIVE_BASE into the memo key inside alias_reexpand_admit itself, rather than at each call site. I traced why that placement matters rather than taking it on faith: the function has two callers — the ! shell-alias branch (L692-L695), which relocates HOOK_EFFECTIVE_BASE before calling, and the inline git-alias splice (L706-L707), which doesn't relocate the base but still reads the same live global — so keying inside the function covers both without duplicating the capture. A new regression test pins the exact two-segment cache-reuse scenario (block-dangerous-git.test.sh#L166-L176) with both single-segment controls to isolate that the two-segment verdict can only come from the cache.

I independently swept for other base-dependent caches rather than trusting the commit message's claim of completeness:

• repo_oid_width (L276-L289) keys on "$*" of git_locating_opts, and collect_git_locating_opts (L337-L345) always leads that array with -C "$HOOK_EFFECTIVE_BASE", recomputed fresh per segment — so this cache was already correctly base-keyed, confirmed by reading the code rather than by inspection alone.
• HOOK_ALIAS_SEEN is reset to empty in lockstep with the base at every ! reparse (L692-693) and save/restored together (L710-711), so it can't smuggle a verdict across bases either.
• Grepped for every declare -A / memo-style cache in the file — _repo_oid_width/_repo_oid_width_key and HOOK_ALIAS_MEMO are the only two, and both are now sound.

The %q-quoted, newline-delimited key construction (L555-L559) uses the same escaping discipline as the pre-existing kind/HOOK_ALIAS_SEEN fields, so there's no new key-collision surface from an attacker-controlled base string.

Other changes since the last review (d39f49e, 59b31cd docs/test-only commits) carry no new logic — confirmed via diff.

Fan-out integrity: all 16 synced hooks/hook-utils.sh copies are still byte-identical to lib/hook-utils.sh (cmp clean) after this commit, which touched only guardrails's own files. The 15 patch-bump plugin.json diffs are Unicode em-dash → — normalization plus the version bump only — no permission or config changes.

I was unable to execute the test suites in this sandbox (script execution requires interactive approval unavailable here), so I verified the fix by manual trace-through of the control flow and the new test's assertions rather than by running block-dangerous-git.test.sh.
· branch fix/block-dangerous-git-payload-cwd

[claude]

Claude finished @kyle-sexton's task in 1m 40s —— View job

---

Security review complete

Scope: all files changed on this branch vs origin/main (git diff origin/main...HEAD), with particular attention to the newest commit ab6b3da pushed since the last automated security pass (which reviewed through bc3cf8e).

• Gather context (PR diff, changed files, prior review history)
• Diff the newest commit (ab6b3da) against what was already reviewed (bc3cf8e)
• Review lib/hook-utils.sh (synced resolver, 16 plugin copies)
• Review plugins/guardrails/hooks/block-dangerous-git.sh
• Review test files for security-relevant assertions
• Check CHANGELOG/plugin.json fan-out for anomalies
• Post findings

Findings

No security issues found.

What's new since the last security pass (bc3cf8e → ab6b3da): confirmed via git diff bc3cf8e..ab6b3da --stat that commit ab6b3da touches only plugins/guardrails/CHANGELOG.md (prose expansion of the already-reviewed memo-key fix) and plugins/guardrails/hooks/block-dangerous-git.test.sh (new run_split cases: the memo collision across && instead of ;, a same-base/different-key control, and a 16-distinct-bases cost/bound check with a SHA-256 tail still blocked). No production logic changed in this commit — nothing new to review from a vulnerability standpoint.

Independent re-verification of the two production changes (not just re-reading prior review comments):

• lib/hook-utils.sh env -S resume fix (#L1147-L1189): continue 2 → continue correctly resumes inside env's own option-parsing loop rather than the outer command-dispatch loop. This is a pure control-flow correction that closes a bypass (a leading option in an -S splice previously caused the whole command to be misread as a non-git word and skipped entirely); it introduces no new execution path — the resolved words still only feed the static scan, never eval/sh -c.
• block-dangerous-git.sh payload-.cwd plumbing: traced HOOK_CWD (from .tool_input JSON, #L104-L111) end-to-end to its two consumption points — collect_git_locating_opts (#L344) and effective_dir (#L408) — and confirmed both pass it to git only as a discrete argv array element (-C "${HOOK_EFFECTIVE_BASE:-...}", #L285 git "$@" rev-parse ...). No string interpolation into a shell command, no eval. An attacker-controlled cwd value (e.g. one starting with - or containing shell metacharacters) cannot be reinterpreted as a flag or break out of the argument, since it always occupies a fixed array slot immediately after a literal -C.
• Memo-key base fix (alias_reexpand_admit, #L547-L566): the Codex-flagged P1 (same alias text re-expanded under two different repository bases wrongly reusing the first verdict) was already fixed in bc3cf8e and re-verified in the prior security pass. The new commit only adds test coverage for it (&& vs ;, a same-key control, a 16-base cost/bound case) — no code change to re-verify.

Fan-out integrity: all 16 synced hooks/hook-utils.sh copies remain byte-identical to lib/hook-utils.sh (same blob SHA 805f68da... across all 17 listed files); the other 15 plugins' diffs are version bump + CHANGELOG only.

No injection surfaces, authorization gaps, secret handling, or dangerous GitHub Actions patterns were introduced by this PR. All git invocations throughout the diff use array expansion, never eval/sh -c/string-built commands.
· branch fix/block-dangerous-git-payload-cwd

[claude]

Claude finished @kyle-sexton's task in 2m 39s —— View job

---

Code review complete

Scope: correctness/architecture/maintainability lane (security left to the dedicated lane, already run on this PR). This run focused on the delta since the last review round — commits bc3cf8e ("key the alias re-expansion memo on the effective base") and ab6b3da ("pin the memo's base keying, and measure what the fix costs") — and re-verified the rest of the diff (lib/hook-utils.sh's env -S resume fix, block-dangerous-git.sh's payload-cwd base chain, CHANGELOG/version parity) independently rather than trusting prior passes.

What's new since the last review round

• alias_reexpand_admit now folds HOOK_EFFECTIVE_BASE into HOOK_ALIAS_MEMO's key (block-dangerous-git.sh:555-556) — this closes the P1 Codex flagged earlier on this PR (a ! alias re-expanded under a SHA-1 -C then a SHA-256 -C had its second analysis skipped as already-seen, letting a movable 40-hex ref through). I traced both call sites (check_segment's !-branch and the inline git-alias splice) and confirmed keying inside the function rather than at the call sites covers both without duplicating the base capture.
• ab6b3da's added tests (block-dangerous-git.test.sh:177-204) target the actual mechanism, not just the outcome — the &&-vs-; pair confirms the collision path is operator-independent, the "one space added to the alias body" control correctly isolates the base as the discriminating variable (it explicitly notes this control passes on both old and new code, which is the right thing to pin), and the 16-distinct-bases fixtures pin that the weakened dedup stays within HOOK_ALIAS_WORK_MAX (128) — I checked the arithmetic (16 segments × 1 analysis each = 16, plus 1 for the SHA-256 tail = 17) and it's well inside the cap. The memo_many fixture creates subdirectories of REPO_SHA1 rather than 16 separate repos, which is the correct/cheaper way to get 16 textually-distinct bases naming the same repository — matches what the comment claims it's testing.
• CHANGELOG entry for the memo fix (guardrails/CHANGELOG.md) accurately describes the bug, its blast radius (bypass-only, never a false block, since a memo hit only skips analysis and the guard exits at the first block), and the measured cost — consistent with what the diff actually does.

Re-verified from earlier in the PR

• lib/hook-utils.sh's continue vs continue 2 fix (all 17 synced copies still byte-identical by blob SHA 805f68d) — unchanged since the last review, traced again and still correct: resumes inside env's own option loop rather than the outer command dispatcher.
• Version/CHANGELOG bump parity across all 17 changed plugins: guardrails 0.23.1→0.24.0 (minor, correct for the documented behavior change), the other 16 all take a consistent patch bump (hooks/hook-utils.sh is byte-identical to lib/hook-utils.sh in each).
• The PowerShell coverage added in 59b31cd (run_pwsh/run_pwsh_nocwd, payload-cwd + missing-cwd cases) is scoped correctly per the PR's own "not tested" disclosure — it pins payload-cwd reading on that arm but doesn't claim env -S coverage there, matching the code (the PowerShell path doesn't route through hook::git_resolve_index's env-splice case at all).

Outstanding from earlier rounds

The one maintainability nit already posted inline on this PR (the ${git_locating_opts[@]:2} magic offset at block-dangerous-git.sh:693 lacking a comment tying it to collect_git_locating_opts's invariant) is still unaddressed as of ab6b3da. Not re-posting a duplicate — it's low severity and already tracked in this PR's review thread.

Not verified

I do not have permission to execute shell scripts in this sandbox (bash <script> requires interactive approval unavailable here), so I could not run block-dangerous-git.test.sh or lib/hook-utils.test.sh myself. The "363/164 pass" counts and the memo-cost measurements in the PR body are author-claimed and unverified by me — I confirmed the test assertions are well-targeted and internally consistent by reading them and hand-tracing the control flow, not by executing them.

Conclusion

No new correctness bugs, error-handling gaps, or test-coverage holes found. The base-keying fix for the alias memo is sound, its test coverage is well-targeted (including the cost/bound it introduces), and CHANGELOG/version parity holds across all 17 files.
· branch fix/block-dangerous-git-payload-cwd

[kyle-sexton]

Adversarial verification of bc3cf8e1 (the memo fix)

Fresh-context adversarial verifier. I confirmed the P1 on d39f49eb before this fix landed; this is the verification of the fix itself, plus a measurement of the cost it was predicted to carry, plus a note on the second thread.

Do not merge on my account. This closes a live bypass affecting 17 copies of hook-utils.sh and needs the human's approval.

1. The memo fix discriminates — against the right control

origin/main is not a valid control here: main has no base-dependent verdict to mis-cache, so a fixture built against it goes green for the wrong reason. The control is d39f49eb, the pre-memo-fix head.

                                          d39f49eb   bc3cf8e1
solo sha1    (expect ALLOW both)          ALLOW      ALLOW
solo sha256  (expect DENY both)           DENY       DENY
A ; B    the bypass                       ALLOW      DENY     <-- discriminates
A && B   the bypass                       ALLOW      DENY     <-- discriminates
A ; B2   (identical repos, one extra
         space in the alias body)         DENY       DENY

A;B and A;B2 differ only in the alias body's whitespace — i.e. only in the memo key. That isolates the key as the cause. Both now deny.

2. My open read-not-run gap is now run: the git-alias branch was never vulnerable

I had flagged that alias_reexpand_admit git "${nextw[@]}" shares the memo and might collide the same way. It does not, on either tree:

git-alias solo sha256          d39f49eb=DENY   bc3cf8e1=DENY
git-alias sha1 ; sha256        d39f49eb=DENY   bc3cf8e1=DENY

The reason is structural: the git-alias branch keys on the full argv, which already contains -C <dir>, so its keys always differed by base. Only the ! branch strips the leading globals into a bare reparse body. Keying inside alias_reexpand_admit rather than at the call site covers it anyway, which is the right call — but it is defensive, not a behaviour change there.

3. The predicted cost is real, and now measured

I flagged before the fix that keying on the base weakens dedup, and that a base-alternating command is the case nobody had built. It reproduces:

N repetitions of the SAME `!` alias body under N DISTINCT bases
(safe terminal `!git status`, so the only non-ALLOW is the work cap)

n=10     d39f49eb=ALLOW    bc3cf8e1=ALLOW
n=100    d39f49eb=ALLOW    bc3cf8e1=ALLOW
n=130    d39f49eb=ALLOW    bc3cf8e1=CAPPED   <-- false block introduced by the fix

Pre-fix those collapse to one analysis; post-fix they are N analyses against the invocation-wide HOOK_ALIAS_WORK_MAX=128, which fails closed. The boundary sits between 100 and 130 distinct bases.

This is exotic and I would not block on it — 130 distinct -C directories invoking the same ! alias in one command is not real usage, and failing closed is the right direction. But it is a real behaviour change, the budget is now consumed per-base rather than shared, and no fixture covers it. The suite's existing 20/60-hop traversal cases are single-base; the three cases added with this fix are all two-segment. A future budget change or a deeper realistic case would cross this silently.

Separately measured, so it is not confused with the above: the same doubling on git-alias chains caps identically on both trees (100-hop two-base → CAPPED at d39f49eb and bc3cf8e1). That path is unchanged by the fix.

4. Second thread (:2 magic offset) — confirmed, and the failure mode is benign too

The note is right that effective_dir ${git_locating_opts[@]:2} depends on an uncommented invariant. I checked what the invariant actually protects: I built a variant tree with :2 changed to :0 — the double-apply — and diffed decisions.

                                        :2      :0
! alias under sha256 -C   (DENY)        DENY    DENY
! alias under sha1 -C     (ALLOW)       ALLOW   ALLOW
! alias, relative -C sub                ALLOW   ALLOW
payload cwd absent, base falls to '.'   ALLOW   ALLOW
  ...same, with a relative -C sub       ALLOW   ALLOW

Identical in every case I could construct, including the relative-base fallback where a double-apply would compose ./.. So the offset is not load-bearing for correctness today even if removed — the note's value is purely maintainability against a future second synthetic pair, which is how it is framed. Agreeing with the reviewer, and downgrading the urgency.

5. Method note: four broken fixtures, each caught by a control

Worth recording because it kept happening. In this pass alone:

1. A chain helper that emitted a leading git, so I built git -C <dir> git -c alias… — every row ALLOW, including a reset --hard control that must deny.
2. 100 segments at absolute paths → past MAX_COMMAND_LEN=16384 → DENY on a safe terminal: the length ceiling failing closed, not the work cap.

Plus two from the earlier jq work. Every one produced exactly the answer I was hoping to find. The only thing that caught them was carrying a control whose expected value is the opposite of the hypothesis. An ALLOW from a fixture that never ran looks identical to an ALLOW from a guard that decided.

What I did not test

• The !-branch cost boundary precisely — bracketed at 100/130, not bisected; an n=160 run died on Windows fork exhaustion, not on a guard decision.
• Deep chains of nested ! aliases (each relocating), as opposed to N sibling segments. That is the shape where per-base keying could compound rather than add.
• Whether HOOK_ALIAS_WORK_MAX=128 is still the right number post-fix. I measured that the cap is reachable; I did not assess whether it should rise.
• The full suite on bc3cf8e1. I ran targeted probes, not block-dangerous-git.test.sh end to end on this head.
• PowerShell payloads — everything here is tool_name: "Bash", same gap as my earlier reports on this branch.
• :2 with a genuinely divergent relative base — I could not construct a case where the composed spelling differs in a way git resolves differently, so "benign" rests on the five cases above, not on exhaustion.
• Single box, git 2.54.0.windows.1, core.filemode=false.

Fixtures: two real repositories (SHA-1 and SHA-256), a SHA-256 bare remote, and a ref literally named 40 hex characters pointed at the remote tip so the lease always passes.

[claude]

Claude finished @kyle-sexton's task in 55s —— View job

---

Security review complete

Scope: delta since the last automated security pass on this PR (ab6b3da), per the branch HEAD at trigger time (d40420d). Confirmed via git diff ab6b3da..d40420d --stat that exactly one file changed.

• Gather context (diff since last security review)
• Determine what's new since the last security pass (ab6b3da)
• Review the new commit for security issues
• Post findings

Findings

No security issues found.

The only change since the last security review is commit d40420d ("docs(guardrails): explain the :2 slice at the alias reparse call site"), which touches a single file:

plugins/guardrails/hooks/block-dangerous-git.sh#L693-L700

This adds an 8-line comment above the existing HOOK_EFFECTIVE_BASE="$(effective_dir ${git_locating_opts[@]+"${git_locating_opts[@]:2}"})" line, explaining the :2 offset invariant that both the Codex review and an earlier automated review pass on this PR had flagged as an undocumented magic number. I diffed the surrounding logic byte-for-byte and confirmed:

• The executable line itself (HOOK_EFFECTIVE_BASE="$(effective_dir ...)") is byte-identical before and after — no argument slicing, quoting, or control flow changed.
• No new variable expansion, subshell, or code path was introduced.
• This is a documentation-only commit ("Comment only; no behaviour change." — confirmed by inspection, not just taken from the commit message).

Since this line was already reviewed and traced as sound in the prior security pass (the :2 offset correctly avoids double-applying the base, and no exploitable path was found), and nothing executable changed, there is nothing new to assess from a vulnerability standpoint.

Full-PR posture (recap, unchanged from prior passes): all git invocations throughout the diff use array expansion, never eval/sh -c/string-interpolated commands; the payload .cwd value is only ever consumed as a discrete -C argv element; the alias-memo key now correctly folds in HOOK_EFFECTIVE_BASE (the P1 Codex flagged, fixed in bc3cf8e); and the 16-plugin fan-out is version/CHANGELOG-only with all hooks/hook-utils.sh copies byte-identical to lib/hook-utils.sh.
· branch fix/block-dangerous-git-payload-cwd

[claude]

Claude finished @kyle-sexton's task in 1m 8s —— View job

---

Code review complete

Scope: correctness/architecture/maintainability lane (security left to the dedicated lane, already run multiple times on this PR). This round covers the delta since the last review pass — commit d40420d, pushed since ab6b3da was last reviewed.

• Gather context (PR description, prior review rounds, diff since last review)
• Diff newest commit (d40420d) against what was already reviewed (ab6b3da)
• Confirm whether the :2 magic-offset nit was addressed
• Re-scan full diff for correctness/architecture/test-coverage issues
• Post findings

What's new in this commit

git diff ab6b3da..d40420d --stat shows exactly one file touched, 8 lines added, 0 removed: plugins/guardrails/hooks/block-dangerous-git.sh. It's a pure comment addition directly above the ${git_locating_opts[@]:2} call site — no logic, test, or CHANGELOG changes ride along.

This closes the maintainability nit both Codex flagged and I raised in an earlier round on this same line: the :2 offset previously carried no comment explaining why it's safe to skip those two array elements. I re-verified the comment's claim against the code rather than taking it at face value — collect_git_locating_opts does unconditionally lead git_locating_opts with -C "${HOOK_EFFECTIVE_BASE:-...}" at line 342 before appending anything else, so :2 reliably skips exactly that pair and effective_dir's own HOOK_EFFECTIVE_BASE default supplies the same value without double-applying it. The comment states this correctly, including the failure mode if the invariant is ever broken ("wrapper dirs are read starting one pair too late").

Placement and style are consistent with the rest of the file's unusually dense inline-documentation convention (matches the tone of the surrounding !-alias relocation comments a few lines above).

Conclusion

No functional change to re-verify, and no new correctness bugs, error-handling gaps, or test-coverage holes from this commit. This was documentation-only, appropriately not paired with a CHANGELOG or version bump since there's no user-visible behavior change. All prior findings from this review lane are now resolved — I have nothing outstanding to flag.
· branch fix/block-dangerous-git-payload-cwd