fix(guardrails): scope block-convention-violation's effective_dir to git's own globals

[kyle-sexton]

What changed

block-convention-violation.sh's effective_dir scanned every word of the command for -C —
no [git, subcommand) slice, no wrapper replay. That is the pre-#1785 shape, and this hook was the
last un-migrated caller after #2100 finished the sibling in block-dangerous-git.sh.

It failed in the opposite direction from that sibling. #2100's hole was blindness to a chdir
that really happened. This one is indiscriminate: it invents chdirs that are not there. In
env -u -C git <alias> … GNU env's -u NAME consumes -C as the variable to unset, so git never
moves — yet the every-word scan composed <cwd>/git and read that directory's aliases.

The reachable consumer is the gitconfig alias lookup (:310), which has neither a stdin-form
gate nor an exemption gate, and which fails open: reading the wrong repository's config silently
misses the expansion, so the guard never learns the real subcommand is commit and the team
convention goes unenforced. The sequencer_in_progress probe at the same call site is corrected
with it.

effective_dir now receives git's own globals only — the slice from the resolved git token (gi)
to the subcommand — preceded by any genuine wrapper chdir replayed from
HOOK_GIT_RESOLVED_WRAPPER_DIRS. That resolver is the one parser able to tell a real env -C <dir>
from the -C in env -u -C git. This is exactly the shape block-noncanonical-commit.sh and
block-dangerous-git.sh already use; the docblock points at the sibling's rationale rather than
restating it.

Verification

Every case was written as a standalone fixture and run against a pristine origin/main worktree
before the fix existed, so both columns below are measured, not reasoned about. The controls flip
in opposite directions, which a single-direction fixture cannot fake.

case	pre	post	expected	discriminates?
env -u -C git qc, alias in the true repo	0	2	2	yes — fails pre-fix
env -u -C git qc, alias only in a decoy <cwd>/git	2	0	0	yes — fails pre-fix, opposite direction
git qs -C dec (post-subcommand -C)	2	0	0	yes — fails pre-fix
env -C inner git qc (genuine wrapper chdir)	2	2	2	no — must not regress
git qc (no wrapper; machinery liveness)	2	2	2	no — proves the fixture is real

Liveness. The git qc baseline row is the proof the alias machinery is actually wired and the
fixture repositories are real git repos with a real .claude/source-control.md — without it, a
uniformly silent hook would read as "all controls pass". The decoy/true pair is the second liveness
proof: the hook speaks when the alias sits in the composed directory and goes quiet when it does
not, which can only happen if it genuinely read a repository rather than echoing the payload.

git commit -C HEAD is deliberately NOT the control, per the issue's reachability section.
-C sets exempt=1 at :343-345 and :350-351 returns before effective_dir is ever called, so
that invocation answers "allowed" on both trees and reads as already fixed. The positional case runs
through the alias lookup instead, with an alias ending in -- so the -C dec git appends to the
expansion cannot re-trigger the reuse-message exemption in the recursed frame. Without that --,
the case answered 0 on both trees for a reason unrelated to effective_dir — that first draft was
caught and discarded.

HOOK_GIT_RESOLVED_WRAPPER_DIRS was printed and confirmed empty for env -u -C git, so the
lead control passes because the slice is right, not because the resolver invented a compensating
wrapper directory.

Suite: 37 pass, 0 fail; all 3 affected suites green (scripts/affected-tests.sh --run).

Scope

A relative -C inside a !-shell-alias body still composes from the payload cwd here, because this
hook has no HOOK_EFFECTIVE_BASE tracking the way block-noncanonical-commit.sh does. That is a
distinct pre-existing gap, not this defect, and is deliberately left alone.

Adversarial verification status

A fresh-context adversarial verifier was spawned for this PR and did not return a verdict before
the authoring session ended — the machine was saturated by concurrent agents and every spawned
verifier stalled inside a long test sweep. Treat this PR as carrying the author's own evidence only.

What partially substitutes for it, and why it is not nothing: the PRE column in the table above was
produced by running the shipped test file against the unmodified origin/main hook in a
pristine worktree, which is precisely the headline check such a verifier performs. What is still
unverified by a second party is the "can you break it" attack surface and the payload-supply
question called out below.

Closes #2113

Related

• fix(guardrails): replay a wrapper's chdir in the lease hash probe and rescope skill-reference-verify reconstruction #2100 — the sibling fix in block-dangerous-git.sh, and where this was found
• fix(guardrails): stop parsing a wrapper's options as git's globals #1785 — built the shared parser in hooks/hook-utils.sh and migrated the first caller
• fix(guardrails): skill-reference-verify reports an untouched reference under replace_all: true #2129 — the other guardrails defect from this sweep, shipped separately
• Another agent is concurrently bumping guardrails for fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124; this branch bumps 0.23.1 → 0.24.0
  and the second of the two to merge will need to re-bump.

[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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fab759ed38

ℹ️ 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 3m 2s —— View job

---

Security Review — PR #2152

• Gather context (diff, changed files)
• Review block-convention-violation.sh changes for security issues
• Review block-convention-violation.test.sh changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

Scope: the 4 files changed in this PR (plugins/guardrails/hooks/block-convention-violation.sh, its test file, plugin.json, CHANGELOG.md), diffed against origin/main.

Summary

No new vulnerabilities. This PR tightens an existing guardrail — it closes a fail-open hole where effective_dir's old every-word -C scan could compose the wrong directory (e.g. <cwd>/git for env -u -C git qc …, since GNU env -u consumes -C as the flag it unsets) and silently read the wrong repo's alias.<sub> config, letting a convention-violating commit subject through unblocked. The new code scopes the scan to ${w[@]:gi:sub_idx-gi} (git's own globals, from the resolved git token to the subcommand) plus any genuine wrapper chdir replayed from HOOK_GIT_RESOLVED_WRAPPER_DIRS — the same contract block-dangerous-git.sh/block-noncanonical-commit.sh already use. All values that reach git -C "<path>" config --get "alias.$sub" are passed as discrete argv elements (never eval'd/string-interpolated into a shell command), so there's no command-injection surface introduced by this change, and the ${w[@]:gi:sub_idx-gi} slice is safe under set -u since hook::git_resolve_subcommand guarantees sub_idx > gi whenever it returns 0.

Note on the Codex review comment (not a security finding)

Codex's inline comment on line 349 (wrapper cwd not preserved through env -S alias re-expansion) looks correct on inspection: when hook::git_resolve_index splices an -S-split command, the env -C <dir> prefix is consumed into HOOK_GIT_RESOLVED_WRAPPER_DIRS but dropped from the word array itself; the recursive check_segment "${w[@]:0:gi+1}" … call for a persisted alias then re-invokes hook::git_resolve_index, which resets that global to empty before the recursed frame can read it. The practical effect, though, is the safe direction for a security guard: sequencer_in_progress in the recursed frame probes the wrong (unrelocated) directory, so a real in-progress sequencer state can be missed and a prepared commit message gets blocked instead of exempted — over-enforcement, not a bypass. It doesn't weaken this guard's security posture (no scenario found where it causes a violating commit to be let through), so I'm not raising it as a security finding here; it's a correctness/usability bug worth fixing separately, and Codex has already flagged it precisely.

Verdict: No security vulnerabilities identified in the changed files.

[claude]

Claude finished @kyle-sexton's task in 6m 30s —— View job

---

Review complete

Todo list

• Gather context (read CLAUDE.md, AGENTS.md, PR diff)
• Review block-convention-violation.sh changes for correctness
• Review block-convention-violation.test.sh for coverage
• Review CHANGELOG.md / plugin.json version bump
• Check Codex's inline finding about -S/wrapper-cwd preservation
• Post inline findings
• Post final summary

Summary

The core fix is sound: effective_dir now receives a bounded [gi, sub_idx) slice of git's own globals plus any wrapper -C replayed from HOOK_GIT_RESOLVED_WRAPPER_DIRS, matching the contract block-noncanonical-commit.sh and block-dangerous-git.sh already use. I traced the five verification-table scenarios against the code and they check out: the env -u -C git decoy/true-repo pair, the post-subcommand -C case (blocked from mattering by the loop's -- break), and the genuine env -C <dir> wrapper-replay case all resolve the way the PR claims for the code paths exercised.

One confirmed gap (posted inline, and it independently corroborates Codex's finding on line 349): the two !-shell-alias reparse branches (lines 333-336 and 351-355) call hook::bash_parse_segments on just the alias body text, without carrying forward the wrapper's env -C <dir> prefix the way the plain-alias recursion does. In the recursive frame, wrapper_cd comes back empty, so the sequencer_in_progress check at line 391-392 probes the payload cwd instead of the wrapper's real target directory. Concretely, env -C inner git qc ... where inner's persisted alias is a !git commit ... shell alias and inner has a merge in progress would have its sequencer exemption evaluated against the wrong repo, and a violating prepared merge-commit subject would get content-gated instead of exempted. The sibling block-noncanonical-commit.sh already solves exactly this via HOOK_EFFECTIVE_BASE threading (its lines 668-676, 733-741) that this file's effective_dir has no equivalent for. I also flagged the matching test coverage gap — no case here exercises a !-shell alias at all.

This is distinct from the gap the PR's own "Scope" section already discloses (a relative -C inside an alias body); this one is about the wrapper's chdir before git being dropped during shell-alias recursion, and it's reachable through the exact consumer (alias lookup) this PR is fixing.

Other notes:

• CHANGELOG.md and the plugin.json minor version bump (0.24.0 → 0.25.0) are consistent with the described behavior change, and the changelog's "Acceptance behavior changes" callout is accurate against the diff.
• The docblock added at block-convention-violation.sh:194-210 and :300-305 is accurate for the paths it describes, but its claim of parity with block-dangerous-git.sh's boundary doesn't extend to the shell-alias recursion case above — worth tightening once that gap is addressed, so a future reader doesn't assume full parity exists.
• I was not able to run scripts/affected-tests.sh or the test file directly in this sandbox (script execution requires approval that isn't available in this review lane), so the PR's "37 pass, 0 fail" and the PRE/POST table are author-claimed and not independently verified by me. Everything above is from reading the diff and tracing the relevant code paths by hand, including the sibling files' HOOK_EFFECTIVE_BASE mechanism.
• No security-relevant findings noted; per this repo's review-lane split, security review is left to the dedicated security lane.

Branch: fix/block-convention-violation-effective-dir ·

[github-actions]

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

[kyle-sexton]

Adversarial verification: PARTIALLY UPHELD

Second, independent pass. Brief was to refute, not confirm. I could not break the fix — but the ticket's stated acceptance is not literally met, and I am recording that rather than rounding up.

Trees pinned: gr-2113 @ fab759ed (re-checked unchanged at the end), gr-base @ fd075c27 (= origin/main).

Hook pristineness, by hash rather than by diff — gr-base's test file was later modified by another agent, so git status there is dirty and a --stat check alone would look suspicious:

block-convention-violation.sh  index=f6a21e580bd223de  worktree=f6a21e580bd223de
hook-utils.sh                  index=bac0f0dbb0dc0c5f  worktree=bac0f0dbb0dc0c5f

Only *.test.sh differs. I drove gr-base's hook via a copy of this PR's suite, then deleted the copy.

---

1. Are the shipped controls dead in some third way?

This is the third attempt at fixtures on this file, so I treated every expect 0 as guilty. An exit 0 cannot distinguish "read the true repo, correctly silent" from "never reached line 348." I built positive twins — same fixture shape, alias staged in the true repo instead of the decoy:

Twin	gr-base	gr-2113
C′ decoy <cwd>/git exists, alias in TRUE repo	0	2
D′ decoy <cwd>/dec exists, alias in TRUE repo	0	2

Both flip to blocked. The alias lookup genuinely reaches :348 and genuinely reads the true repository in exactly those fixtures — so the -- terminator on alias.qs does its job and the recursed frame is not exempt-short-circuited. The shipped controls are alive. Fixture reality asserted, not inferred: decoy <cwd>/git alias.qc = [commit], true repo alias.qc = [].

The dead-control shape you flagged reproduces exactly as documented: git commit -C HEAD -F - → 0 on both trees. Correctly excluded.

2. The lead control's mechanism, re-derived independently

I sourced hook-utils.sh directly and called hook::git_resolve_index / hook::git_resolve_subcommand outside the hook, printing the resolver's own state:

env -u -C git qc         rc=0 gi=3 sub=qc  sub_idx=4 WRAPPER_DIRS[n=0]=()      slice=[git]
env -C inner git qc      rc=0 gi=3 sub=qc  sub_idx=4 WRAPPER_DIRS[n=1]=(inner) slice=[git]
git qs -C dec            rc=0 gi=0 sub=qs  sub_idx=1 WRAPPER_DIRS[n=0]=()      slice=[git]
git -C inner qc          rc=0 gi=0 sub=qc  sub_idx=3 WRAPPER_DIRS[n=0]=()      slice=[git -C inner]
sudo -C 3 git qc         rc=0 gi=3 sub=qc  sub_idx=4 WRAPPER_DIRS[n=0]=()      slice=[git]
sudo -D inner git qc     rc=0 gi=3 sub=qc  sub_idx=4 WRAPPER_DIRS[n=1]=(inner) slice=[git]
env -C a -C inner git qc rc=0 gi=5 sub=qc  sub_idx=6 WRAPPER_DIRS[n=1]=(inner) slice=[git]

WRAPPER_DIRS is empty for env -u -C git and the slice is bare [git], so effective_dir falls through to payload cwd. The case passes because the slice is right, not because the resolver invented a compensating directory. Confirmed independently. Also visible here: sudo -C 3 correctly records no chdir (close-from, not --chdir), and env -C a -C inner is last-wins at n=1, matching GNU env.

3. Does the 2/2 row discriminate against a double-apply?

Yes — and I proved it rather than argued it. Mutant M3 duplicates the replay (wrapper_cd+=(-C "$wdir") twice):

case	gr-base	gr-2113	M3 double-apply
E env -C inner git qc	2	2	0
W1 env -C 'my dir' git qc	2	2	0
W6 env -C <ABS> git qc	2	2	2 ← blind

A relative wrapper dir composes <cwd>/inner/inner under double-apply, finds nothing, and the row drops to 0. So E guards both directions.

One caveat worth recording: a double-apply is invisible when the wrapper dir is absolute — <abs> then <abs> again is idempotent under effective_dir's absolute-path branch at :218. W6 stays 2 under M3. If a future test replaces E's relative inner with an absolute path, the double-apply guard silently evaporates. Keep E relative.

---

Per-case discrimination table (the 6 new run invocations)

gr-base: PASS=34 FAIL=3. gr-2113: PASS=37 FAIL=0.

#	New case	gr-base	gr-2113	Discriminates?
1	env -u -C git <alias>: alias resolves in the TRUE repo (blocked)	0	2	YES
2	env -u -C git <alias>: conforming subject still allowed	0	0	no
3	env -u -C git <alias>: decoy repo at <cwd>/git is not read	2	0	YES
4	post-subcommand -C is not a chdir (decoy repo not read)	2	0	YES
5	env -C <dir> git <alias>: wrapper chdir is replayed (blocked)	2	2	no
6	env -C <dir> git <alias>: conforming subject still allowed	0	0	no

This is the whole of the PARTIALLY. Case 5 does not fail against the current implementation — base's every-word scan hits the same -C inner by accident — so the ticket's "Every test carries a control that fails against the current implementation" is not literally met for the wrapper-replay claim. It is a wording/acceptance gap, not a test-quality gap: M1 and M3 show case 5 is load-bearing in both directions.

Cheapest close: add env --chdir=inner git <alias> as case 5's control. Same replay path, and it does discriminate (base 0 → fixed 2), because base's scan does not recognize the = spelling.

Correctness attack — 21 probe cases x 4 trees

M1 = replay stripped. M2 = both call sites reverted to effective_dir "${w[@]}".

base	fix	M1	M2	case
0	2	2	0	A env -u -C git <alias> (alias in TRUE repo)
2	0	0	2	C alias only in decoy <cwd>/git
2	0	0	2	D git qs -C dec, alias only in decoy
2	2	0	2	E env -C inner git <alias>
0	2	2	0	C′ / D′ twins (alias in TRUE repo)
2	2	0	2	W1 env -C 'my dir' — space in directory
0	2	0	0	W2 env --chdir=inner
0	2	0	0	W3 env -Cinner (attached)
0	2	0	0	W4 env -vC inner (clustered peel)
0	2	0	0	W5 env -C nope -C inner (last-wins)
2	2	0	2	W6 env -C <ABS>
0	2	0	0	W7 env -C inner -S 'git qc -F -…'
2	2	2	2	W8 git -C inner <alias>
2	0	0	2	S1 sudo -C 3 git <alias> (close-from, not chdir)
0	2	2	0	S1′ twin: sudo -C 3, alias in TRUE repo
0	2	0	0	S2 sudo -D inner git <alias>
2	0	0	2	Q1 env -u -C git commit -F - mid-merge (sequencer, :392)
0	0	0	0	Z1 git commit -C HEAD -F - (exempt before effective_dir)

The fixed column matches the wanted answer on every row. Three results carry the weight:

• M2 reproduces gr-base on all 21 rows. The only behavioral delta in this diff is effective_dir's argument list — nothing incidental (docblock, the wrapper_cd loop's mere existence, the CHANGELOG) is doing work.
• W1 = 2 on the fixed tree proves ${wrapper_cd[@]+"${wrapper_cd[@]}"} does not word-split. A split would compose <cwd>/my, find no alias, return 0. On Windows this was the likeliest way the fix could be quietly wrong; it isn't.
• Q1 shows the second consumer (sequencer_in_progress, :392) is fixed too: base 2 → fixed 0. No committed case covers that consumer.

Windows path forms, since fixtures elsewhere in this repo hit 8.3: effective_dir never compares path forms, it only composes, so the shortname risk here is a decoy — but I tested it. With cwd delivered as /tmp/…, as C:/Users/KYLESE~1/…, and as C:/…/TMP~1.MHG/REPO~1.J5F, the lead case reads 0/0 on base and 2/2 on fixed. The ^[A-Za-z]:[\/] branch works on real Windows operands.

git -Cinner (attached git -C) is not a gap — git rejects it: unknown option: -Csub, exit 129, git 2.54.0.windows.1.

One pre-existing defect found — not this PR's, do not block on it

Identical on both trees:

base	fixed	case
0	0	git -C inner commit -F -, MERGE_HEAD in inner → ALLOW (correct)
2	2	git -C inner qc -F - (alias), MERGE_HEAD in inner → should ALLOW
0	0	git qc -F - (alias), MERGE_HEAD in true repo → ALLOW (correct)

The alias-recursion argv rebuild at :341/:361 — check_segment "${w[@]:0:gi+1}" <expansion> "${w[@]:sub_idx+1}" — drops git's own globals, so -C inner is lost in the recursed frame and the sequencer is probed in the wrong repo. Wrapper words survive (they live below gi); git's own -C does not. Fails closed (over-blocks a legitimate mid-merge alias commit). Present on origin/main, untouched here. Worth its own ticket — it is the same positional-context family as this fix, one frame deeper.

Regressions

bash scripts/affected-tests.sh --run on gr-2113: exit 0, 3 selected suites — resolve-convention-pattern all PASS, block-convention-violation PASS=37 FAIL=0, commit-msg-convention PASS=15 FAIL=0.

---

Read-vs-run inventory

Ran (real exit codes, both trees unless noted): the full committed suite on gr-base and gr-2113; a 21-case adversarial probe on base + fixed + three mutants; a 3-case pre-existing-gap probe on both; a 4-case Windows-path-form probe on both; a direct resolver derivation sourcing hook-utils.sh; git -Csub acceptance against real git; affected-tests.sh --run; index-vs-worktree hashes on both hook files.

Read only, did not run — hook::git_resolve_index's nice/nohup/timeout/eval/command -v wrapper branches; hook::git_alias_expansion's rc-2 --config-env refusal; first_herestring_subject.

What I did NOT test

• The PowerShell surface. No case drives ps::classify_git_command with a wrapper chdir. Everything above is tool_name: "Bash".
• !-shell aliases (:333-337 inline, :351-355 gitconfig). Both re-parse via hook::bash_parse_segments rather than calling effective_dir, so I judged them out of blast radius — but I ran none. This is adjacent to the known-and-deliberately-unfixed relative--C-inside-a-shell-alias-body item, which I am not reporting as new.
• --git-dir= / --work-tree= as alternative repo relocations. effective_dir does not model them at all, on either tree. Untested, unfixed, pre-existing.
• sudo's declared clustered-flag gap (sudo -bD dir git …), documented fail-open at hook-utils.sh:1223-1225. I tested only unclustered -D/-C.
• The sequencer consumer at :392 beyond my single Q1 shape.
• M3's tail. The double-apply probe covered rows through W9 and then hit a fork failure from box saturation (dofork: child -1 … exit code 0xC0000142) — S1/S1′/S2/U1/Q1/Z1 were not measured under M3. Every row the double-apply question actually turns on (E, W1, W6) completed.
• The full repository suite — only affected-tests.sh's 3 selected suites.
• gh pr create --title and the kill switch beyond the suite's existing coverage.

Verdict

PARTIALLY UPHELD. The fix is correct — 28 constructed cases, zero counterexamples, and the two mechanisms you asked me to doubt both re-derive independently. Ship it. The only outstanding item is the case-5 acceptance wording: either add the env --chdir=inner control (one line, genuinely base-failing) or amend the ticket to say the wrapper-replay claim is mutation-proven rather than base-failing. Do not silently round that to a clean discharge — that is the exact shape this file has already produced twice.

Verified by an adversarial reviewer instructed to refute. Probe scripts and mutants retained locally under the session scratchpad.

[claude]

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

---

Security Review — PR #2152

• Gather context (diff, changed files)
• Review block-convention-violation.sh changes for security issues
• Review block-convention-violation.test.sh changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

Scope: the 4 files changed in this PR, diffed origin/main...HEAD at 7d9e2e4
(plugins/guardrails/hooks/block-convention-violation.sh, its test file, plugin.json,
CHANGELOG.md).

Summary

No security vulnerabilities identified. This PR closes an existing fail-open guardrail hole:
effective_dir's old every-word -C scan could compose the wrong directory (e.g. <cwd>/git for
env -u -C git qc …, since GNU env -u consumes -C as the flag it unsets) and silently read the
wrong repo's alias.<sub> config, letting a convention-violating commit subject through unblocked.
The fix scopes the scan to git's own globals — the resolved-git-token→subcommand slice
(${w[@]:gi:sub_idx-gi} at
block-convention-violation.sh#L337)
— preceded by any genuine wrapper chdir replayed from HOOK_GIT_RESOLVED_WRAPPER_DIRS, matching the
contract block-dangerous-git.sh/block-noncanonical-commit.sh already use.

Two findings from earlier automated reviews on this PR (Codex's inline comment and the prior Claude
pass, both about the wrapper's env -C <dir> chdir being dropped when a !-shell alias body
re-parses as a fresh top-level command) have already been fixed in a follow-up commit
(47ebd37), which threads HOOK_EFFECTIVE_BASE through the reparse
(L362-L365
and
L384-L387)
and adds matching negative/positive test pairs. That is a correctness fix, not itself a new attack
surface, so I'm noting it rather than re-flagging it.

Points checked, no issue found

• No injection surface. Every value that reaches git -C "$seg_dir" config --get "alias.$sub"
  (L378)
  and sequencer_in_progress "$seg_dir"
  (L424)
  is passed as a discrete argv element to git, never eval'd or string-interpolated into a shell
  command, so arbitrary payload content (including shell metacharacters in a -C argument) can't
  escape into command execution — same as pre-existing behavior, unchanged by this diff.
• set -u safety. ${w[@]:gi:sub_idx-gi} is safe because hook::git_resolve_subcommand
  guarantees sub_idx > gi whenever it returns 0 (checked in hook-utils.sh); the wrapper_cd
  array is built with the ${arr[@]+"${arr[@]}"} idiom, which is correct under set -u for a
  possibly-empty/unset array.
• HOOK_EFFECTIVE_BASE global-state scoping. The save/set/restore around each !-alias reparse
  (saved_base=...; HOOK_EFFECTIVE_BASE=...; hook::bash_parse_segments ...; HOOK_EFFECTIVE_BASE=$saved_base)
  nests correctly because hook::bash_parse_segments invokes its callback directly
  ("$cb" "${seg[@]}" in hook-utils.sh, not through a subshell/command-substitution), so recursive
  alias expansions unwind the global in the same process without cross-contaminating sibling
  segments or leaking across top-level hook invocations (each hook invocation is a fresh process).
• Directory composition (effective_dir, L229-244) is read-only downstream. The composed path is
  only ever used for git config --get and git rev-parse --absolute-git-dir — both read-only — so
  even a maximally adversarial payload directs the hook to read config from an arbitrary
  already-locally-accessible path, not to write or execute anything; this mirrors what the real git
  invocation would itself resolve to, which is the guard's whole purpose.
• Version bump / CHANGELOG (0.24.0 → 0.25.0) are metadata/doc changes only, consistent with
  the described behavior change, no config-surface or permission changes.
• Test file adds only local git-repo fixtures and hook invocations under the test harness; no new
  eval-of-untrusted-input or credential handling.

Verdict: No security vulnerabilities identified in the changed files. This is a guardrail
strengthening (closes a fail-open path), not a regression.

Branch: fix/block-convention-violation-effective-dir

[claude]

Claude finished @kyle-sexton's task in 5m 16s —— View job

---

Review complete

Todo list

• Gather context (diff vs origin/main, CLAUDE.md/AGENTS.md, prior review threads)
• Review block-convention-violation.sh changes for correctness
• Review block-convention-violation.test.sh for coverage
• Review plugin.json / CHANGELOG.md
• Cross-check prior findings (Codex + earlier Claude passes + author's adversarial verification) against current HEAD
• Post inline findings
• Post final summary

What's new since the last review pass

Commit 47ebd37 was pushed since the earlier reviews on this PR. It's a direct fix for the exact gap Codex and an earlier Claude pass both flagged: the !-shell-alias reparse (hook::bash_parse_segments "$reparse"/"$preparse" check_segment at what are now lines 364/386) parses the alias body as a brand-new top-level command, so it previously carried neither the wrapper's -C nor git's own globals into the recursed frame — dropping the wrapper's resolved directory before the sequencer_in_progress probe.

I traced the fix by hand (I could not execute bash/shellcheck in this review lane — script execution requires approval unavailable here, same limitation the prior Claude pass hit — so the "37/40 pass" and PRE/POST tables throughout this PR remain author-claimed, not independently run by me):

• effective_dir's base now falls back to HOOK_EFFECTIVE_BASE (block-convention-violation.sh:230), and both !-reparse call sites now save/set/restore it around the recursive hook::bash_parse_segments call (lines 362-365, 384-387), mirroring block-noncanonical-commit.sh's existing mechanism.
• The value carried is seg_dir, computed once per check_segment frame from wrapper_cd + git's own [gi, sub_idx) slice — the same input already used for the alias-config lookup and the sequencer probe, so this is correct regardless of whether the wrapper reached git via a literal -C or via -S-splitting (Codex's variant).
• I confirmed the non-! alias recursion (plain alias.x = commit, lines 366-372/388-395) does not need this carry: it re-passes "${w[@]:0:gi+1}", which — per hook::git_resolve_index (HOOK_GIT_RESOLVED_WORDS=("$@") in hook-utils.sh:1103) — includes the wrapper's literal prefix words, so the recursive hook::git_resolve_index call re-derives the same wrapper directory independently. The bug was specific to the !-body branch, which discards everything but the alias text.
• The new docblock's claim that this guard's two consumers (config --get, rev-parse --absolute-git-dir) agree on any directory inside one repository, diverging only when a separate repo is nested below the composed path — is accurate and matches why the sibling's more expensive alias_launch_dir (--show-toplevel/--show-prefix) isn't needed here; the caveat is honestly disclosed rather than silently assumed.

No new correctness issues found in this commit. Two test-coverage gaps remain, posted inline:

• block-convention-violation.test.sh:182-183 — the wrapper-chdir-replayed control doesn't actually discriminate against origin/main (both answer 2), a gap the author's own adversarial-verification comment already identified and proposed a fix for (env --chdir=inner), but that control wasn't added in this push.
• block-convention-violation.sh:423 — the sequencer_in_progress consumer has no test reaching it through a wrapper chdir on a plain (non-alias) git commit; also self-flagged by the author as "Q1 ... no committed case covers that consumer."

Neither blocks merge — both are pre-identified, honestly disclosed gaps in an already very thoroughly adversarially-tested PR, not newly discovered defects.

CHANGELOG.md/plugin.json (0.24.0 → 0.25.0) are consistent with the diff and correctly fold both fixes into one unreleased version.
· Branch: fix/block-convention-violation-effective-dir

[claude]

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

---

Security Review — PR #2152

• Gather context (diff, changed files)
• Review block-convention-violation.sh changes for security issues
• Review block-convention-violation.test.sh changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

Scope: the 4 files changed in this PR, diffed origin/main...HEAD at 1975864 (plugins/guardrails/hooks/block-convention-violation.sh, its test file, plugin.json, CHANGELOG.md). No workflow files are touched by this PR, so the GitHub Actions trust-boundary checks in this lane's brief (pull_request_target, permission widening, unpinned actions) don't apply here.

Verdict: no security vulnerabilities identified

This PR tightens an existing fail-open guardrail: effective_dir's prior every-word -C scan could compose the wrong directory (e.g. <cwd>/git for env -u -C git qc …, since GNU env -u NAME consumes -C as the variable being unset) and silently read the wrong repository's alias.<sub> config, letting a convention-violating commit subject through unblocked. I traced the new data flow end to end looking specifically for injection, trust-boundary, and logic-bypass issues (this guard's "security" property is: can it be made to silently not enforce, i.e. fail open):

• No injection surface. Every attacker-influenceable value that reaches a git invocation is passed as a discrete argv element, never eval'd or string-interpolated into a shell command:
  • git -C "$seg_dir" config --get "alias.$sub"
  • sequencer_in_progress "$seg_dir"
  • seg_dir itself (L337) is built from array elements (wrapper_cd, "${w[@]:gi:sub_idx-gi}"), not a joined/re-parsed string, so there's no word-splitting or re-tokenization step where a malicious directory name containing shell metacharacters could escape into command execution.
  • The !-alias reparse path (pre-existing, untouched by this diff) already quotes trailing args with printf '%q' before handing the string to the hook's own tokenizer (hook::bash_parse_segments), not a real shell — no new exposure there.
• Both consumers of the new seg_dir are read-only (config --get, rev-parse --absolute-git-dir inside sequencer_in_progress). Even a maximally adversarial wrapper/alias chain can only steer the hook to read config/ref-state from a path git itself would resolve to — it can't be leveraged to write, execute, or exfiltrate anything beyond what the guard already had access to.
• HOOK_EFFECTIVE_BASE global-state handling is safe. The new save/set/restore around each !-alias reparse (L363-L365, L385-L387) executes sequentially (no subshell), and without set -e a nonzero return from the recursive hook::bash_parse_segments call doesn't skip the restore line, so nested/failed reparses can't leak a stale base into a sibling segment. Each hook invocation is also a fresh process (confirmed against the test harness: run() in the test file invokes bash "$HOOK" as a subprocess per case), so there's no cross-invocation contamination either.
• set -u safety unaffected. ${w[@]:gi:sub_idx-gi} and the ${arr[@]+"${arr[@]}"} idiom used for wrapper_cd/HOOK_GIT_RESOLVED_WRAPPER_DIRS are unchanged patterns already proven safe elsewhere in this file.
• The net effect of the fix is a strict narrowing of the guard's blind spot (fewer decoy-directory false negatives), not a widening of any acceptance/allow path — the "Acceptance behavior changes" callout in the CHANGELOG accurately describes the only two behavior deltas, and both make the gate more restrictive, not less.
• plugin.json / CHANGELOG.md — version bump (0.24.0 → 0.25.0) and doc-only changes, no config-surface or permission changes, nothing security-relevant.
• Test file — adds only local, ephemeral git-repo fixtures under mktemp -d and hook invocations through the existing test harness; no new eval-of-untrusted-input, credential handling, or network access.

This is a guardrail-strengthening change (closes a fail-open path), and I found nothing that introduces a new vulnerability or weakens an existing security boundary.

Branch: fix/block-convention-violation-effective-dir

[claude]

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

---

Code Review — PR #2152

• Gather context (diff vs origin/main, CLAUDE.md/AGENTS.md, prior review threads)
• Review block-convention-violation.sh changes for correctness/architecture
• Review block-convention-violation.test.sh for coverage
• Review plugin.json / CHANGELOG.md
• Post inline findings (none — see below)
• Post final summary

Summary

I did an independent trace of the diff at 1975864 against origin/main, focused on correctness/architecture/test-coverage/maintainability (security is the dedicated lane's scope per REVIEW.md). No new findings to raise — both prior gaps this PR itself surfaced (Codex's and an earlier Claude pass's !-alias wrapper-cwd drop, and the author's own adversarial-verification "case 5 doesn't discriminate" finding) are now fixed and covered, and I could not find a third.

What I traced and confirms as correct:

• effective_dir (block-convention-violation.sh:229-243) now bases off HOOK_EFFECTIVE_BASE before falling back to HOOK_CWD/CLAUDE_PROJECT_DIR, and every reference to it is guarded with ${VAR:-...} under set -u — no unbound-variable risk.
• wrapper_cd (:323-327) is correctly local per check_segment frame, so recursive calls can't leak stale wrapper state across frames.
• The two !-shell-alias reparse sites (:362-365, :384-387) now save/set/restore HOOK_EFFECTIVE_BASE around hook::bash_parse_segments ... check_segment, matching block-noncanonical-commit.sh's existing mechanism. I confirmed the non-! alias recursion (:370, :393) correctly does not need this: it re-passes "${w[@]:0:gi+1}" verbatim, and hook::git_resolve_index (hook-utils.sh:1102-1104) re-derives the same wrapper dirs from that slice independently.
• seg_dir is computed once per frame (:336-337) and reused for both consumers (alias lookup :377, sequencer_in_progress :423) — matches the stated intent of avoiding a second subshell.
• The PowerShell path (:440-455) reduces to the same Bash-tokenizer-faithful form and runs through the identical check_segment, so it inherits the fix uniformly rather than needing a parallel patch.
• New test cases (block-convention-violation.test.sh:127-244) are structured as discriminating pairs (block/allow, decoy/true-repo, sequencer/no-sequencer) rather than lone assertions — consistent with the pattern the author's own adversarial-verification comment argued for, and each new case I checked against hook-utils.sh's resolver logic (e.g. --chdir=, env -u -C) is a valid exercise of that logic, not a tautology.
• CHANGELOG.md/plugin.json (0.24.0 → 0.25.0): the entry accurately describes both fixed defects, correctly notes the second one is pre-existing rather than introduced by the scoping change, and the minor bump is justified by the "Acceptance behavior changes" callout.

One pre-existing, out-of-scope observation (not a finding on this PR — the code path is untouched by the diff, so I'm not filing it as a blocking comment): the !-alias reparse branches (:364, :386) don't set HOOK_NO_ALIAS before recursing, unlike the plain-alias branches (:369/:371, :392/:394) which do. A self-referential persisted shell alias (e.g. alias.qc = '!git qc') would recurse through check_segment → hook::git_alias_expansion → reparse indefinitely, since nothing bounds that specific recursion to one level. This shape exists identically on origin/main today (origin/main block-convention-violation.sh#L320-L332) — this PR only threads HOOK_EFFECTIVE_BASE through the existing calls, it doesn't add or remove recursion bounding. Worth its own ticket if it's judged worth guarding (a user would have to deliberately author a self-referential alias to trigger it), analogous to the other pre-existing gap the author already disclosed and declined to fix in this PR.

I was not able to execute bash/the test suite/shellcheck in this review lane (script execution requires approval unavailable here), so I did not independently run the "37/40 pass" claim or the PRE/POST table — that remains author-claimed, verified only by the two prior review rounds and the author's own adversarial pass on this PR, not by me. Everything above is from reading the diff and hand-tracing it against hook-utils.sh's resolver contract.

Verdict: No correctness, architecture, or test-coverage issues found in this push. Recommend merge; the one observation above is a separate, pre-existing, non-blocking item.
· Branch: fix/block-convention-violation-effective-dir

[claude]

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

---

Security Review — PR #2152

• Gather context (diff, changed files)
• Review block-convention-violation.sh changes for security issues
• Review block-convention-violation.test.sh changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

Scope: the 4 files changed in this PR, diffed origin/main...HEAD at 8877836 (plugins/guardrails/hooks/block-convention-violation.sh, its test file, plugin.json, CHANGELOG.md). The merge commit at HEAD pulls in unrelated origin/main content (an NUL-byte fix in block-dangerous-git.sh/block-no-verify.sh) but touches none of this PR's own files beyond folding its own CHANGELOG section above the newly-merged one — verified by diffing this PR's 4 files against the prior reviewed commit (1975864): no content delta.

Verdict: no security vulnerabilities identified

This PR tightens an existing fail-open guardrail. effective_dir's prior every-word -C scan could compose the wrong directory (e.g. <cwd>/git for env -u -C git qc …, since GNU env -u NAME consumes -C as the variable being unset) and silently read the wrong repository's alias.<sub> config, letting a convention-violating commit subject through unblocked. I traced the full data flow for injection, trust-boundary, and fail-open/fail-closed logic issues:

• No injection surface. Every attacker-influenceable value that reaches a git invocation is passed as a discrete argv element, never eval'd or string-interpolated into a shell command:
  • seg_dir="$(effective_dir ${wrapper_cd[@]+"${wrapper_cd[@]}"} "${w[@]:gi:sub_idx-gi}")" builds seg_dir from array elements, not a joined/re-parsed string.
  • git -C "$seg_dir" config --get "alias.$sub" and sequencer_in_progress "$seg_dir" — both read-only git operations (config --get, rev-parse --absolute-git-dir internally), so even an adversarial wrapper/alias chain can only steer the hook to read config/ref-state from a path git itself would resolve to, not write or execute.
  • The !-alias reparse path (pre-existing, untouched here) quotes each trailing arg with printf '%q' (L358, L382) before handing the string to the hook's own tokenizer (hook::bash_parse_segments), not a real shell — no new exposure from the HOOK_EFFECTIVE_BASE threading added around those calls.
• HOOK_EFFECTIVE_BASE global-state handling is safe. The new save/set/restore around each !-alias reparse (L363-L365, L385-L387) runs sequentially with no set -e in effect around it, so a nonzero return from the recursive hook::bash_parse_segments call doesn't skip the restore — no stale-base leak across sibling segments. Each hook invocation is also a fresh process, so there's no cross-invocation contamination.
• set -u safety unaffected. ${w[@]:gi:sub_idx-gi} is safe because hook::git_resolve_subcommand guarantees sub_idx > gi on success (pre-existing contract, unchanged); ${wrapper_cd[@]+"${wrapper_cd[@]}"} at L326 and its consumer at L337 use the correct possibly-unset idiom.
• Net effect of the fix is a strict narrowing of the guard's blind spot (fewer decoy-directory false negatives). The two behavior deltas the CHANGELOG documents both make the gate more restrictive, not less — no new allow-path was introduced.
• plugin.json/CHANGELOG.md — version bump (0.24.1 → 0.25.0, correctly based off the post-merge predecessor) and doc-only changes, no config-surface or permission changes.
• Test file — adds only local, ephemeral git-repo fixtures under mktemp-backed temp dirs and hook invocations through the existing harness; no eval-of-untrusted-input, credential handling, or network access.

This is a guardrail-strengthening change (closes a fail-open path in a commit-message convention check), not a regression. No CI/workflow files are touched by this PR, so the Actions trust-boundary checks in this lane's brief don't apply here.
· Branch: fix/block-convention-violation-effective-dir

[claude]

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

---

Code Review — PR #2152 (sync at 8877836)

• Gather context (diff vs origin/main, current HEAD state, prior review threads)
• Review block-convention-violation.sh changes for correctness/architecture
• Review block-convention-violation.test.sh for coverage
• Review plugin.json / CHANGELOG.md
• Post inline findings (none — see below)
• Post final summary

Summary

This sync merges origin/main into the branch (adding the mutation-testing plugin commit and a few other unrelated merges) — no new commits touch the four files in scope beyond 30e68ce, which was already reviewed. I independently re-traced the full diff at 8877836 against origin/main rather than relying on the prior passes' conclusions, focused on correctness/architecture/test-coverage/maintainability (security is the dedicated lane's scope per this repo's review split).

No new findings. The two gaps raised in earlier review rounds on this PR (the !-shell-alias wrapper-cwd drop, and the case-5/sequencer test controls that didn't discriminate against origin/main) are both present and closed in the current diff:

• effective_dir (block-convention-violation.sh:229-230) falls back through HOOK_EFFECTIVE_BASE before HOOK_CWD/CLAUDE_PROJECT_DIR. seg_dir is computed once per check_segment frame (:337) and reused for both the alias-config lookup and sequencer_in_progress (:423), replacing two separate effective_dir "${w[@]}" calls with the correct bounded slice.
• I hand-traced the HOOK_EFFECTIVE_BASE save/set/restore around both !-alias reparse sites (:362-365, :384-387) against hook::bash_parse_segments's implementation in hook-utils.sh: the callback is invoked directly ("$cb" "${seg[@]}", not through a subshell or command substitution), so the global assignment is visible to arbitrarily deep nested recursion and correctly unwound on return, including the case where a !-alias reparse internally recurses into a plain alias (check_segment "${w[@]:0:gi+1}" ... at :370/:393) — that inner call happens inside the dynamic extent where HOOK_EFFECTIVE_BASE is still set to the outer seg_dir, so it composes correctly without needing its own carry.
• The plain (non-!) alias recursion deliberately doesn't need this fallback: it re-passes "${w[@]:0:gi+1}", and hook::git_resolve_index re-derives the same wrapper dirs from that slice independently.
• Test coverage (block-convention-violation.test.sh:127-247) now includes discriminating pairs for: the env -u -C git decoy/true-repo split, the post-subcommand -C non-chdir case, the env --chdir=inner attached long-form (added specifically because the -C spelling doesn't discriminate against origin/main, per the author's own adversarial-verification finding), a wrapper-chdir-through-sequencer pair, and three !-alias-through-wrapper-chdir cases with a no-MERGE_HEAD control. Each "now exempt/allowed" assertion is paired with a "still gated" twin, so a case can't pass merely because a code path became unreachable — consistent with the failure mode this file has already produced twice per the PR's own history.
• CHANGELOG.md/plugin.json (0.24.1 → 0.25.0): the entry accurately separates the two fixed defects, correctly labels the second as pre-existing rather than introduced by the scoping change, and the version base (0.24.1) matches origin/main's current guardrails version — no bump conflict with the concurrently-mentioned fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124 work.

Verification note: I attempted to run block-convention-violation.test.sh directly in this review lane and could not — script execution requires approval unavailable here, the same limitation the prior review passes on this PR hit. Everything above is from reading the diff and hand-tracing execution against hook-utils.sh's actual implementation, not from executing the suite. The "37/40/43 pass" figures and PRE/POST tables throughout this PR's history remain author-claimed and are not independently confirmed by me.

Verdict: No correctness, architecture, or test-coverage issues found in the current head. Recommend merge.
· Branch: fix/block-convention-violation-effective-dir

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31547843480
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31547843446
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).