fix(guardrails): block an exported MSYS path-conversion suppressor

[kyle-sexton]

Summary

D:\d was recreated for the third time on 2026-08-16 at 16:46:30 EDT, this time by an ordinary agent lane creating a worktree. #2841 shipped the convention, the helper and the detector for this class; none of them is a preventive control, and the detector is structurally incapable of firing before the damage.

The trigger turns out not to be the path form. The creating lane ran export MSYS_NO_PATHCONV=1 earlier in the same command string — to work around MSYS mangling an unrelated <rev>:<path> argument — and that export silently unconverted a git worktree add /d/worktrees/... seven segments later. The same lane had already run the identical path argument successfully. A guard keyed on the /[a-z]/ path shape has a false negative on the real incident, which is the same false-green shape this body of work exists to remove.

This PR adds a guard keyed on the environment instead, and fixes a shipped defect in the detector's remediation advice that the same incident exposed.

Fix

1. New PreToolUse guard: plugins/guardrails/hooks/block-exported-msys-pathconv.sh (Bash|PowerShell, Windows hosts only, default on, kill switch block_exported_msys_pathconv_enabled).

Blocks export MSYS_NO_PATHCONV / export MSYS2_ARG_CONV_EXCL and the declare -x / typeset -x spellings. Leaves the per-command prefix form and bare assignments alone. A cheap substring pre-filter runs before any parsing, so a command that never names either variable exits immediately.

2. scripts/check-drive-root-litter.sh remediation text. Its footer told the reader to "remove the phantom tree once you have confirmed it holds nothing else." The first real hit on this machine, D:\d\worktrees\ccp-measure2, is a live registered git worktree (.git → gitdir: D:/repos/.../.git/worktrees/ccp-measure2, present in git worktree list). Deleting it as advised strands the registry entry and leaves git worktree prune as the only cleanup — on a repository currently carrying 69 registered worktrees across live lanes. The text now sends the reader to git worktree remove --force first. git grep worktree over the script previously returned nothing.

3. docs/conventions/windows-path-emit/README.md gains rule 5 (never export a suppressor) and a section on the conditional colon-argument mangling that sends authors reaching for one.

Doc surfaces wired: guard table, kill-switch table, generated options table, docs/CATALOG.md, telemetry schema + example + producer registry, skills/setup/SKILL.md, count-bearing prose, CHANGELOG, version 0.28.30 → 0.29.0.

Why not extend block-windows-drive-tmp.sh

Reuse-or-replace applies, and the answer is an openly-scoped sibling rather than a silent second way. That guard is a path-shape and write-target matcher scoped to the literal component tmp, with tmp-specific allowances (/var/tmp, %TEMP%). This one is an environment-variable matcher with no path component at all. Neither would fire on the other's cases. Folding two disjoint matchers and two exclusion sets into one hook makes both harder to reason about and risks regressing a guard that works. Both headers now carry a reciprocal cross-reference naming the split.

Why not scripts/check-shell-portability.sh

An authoring-time lint class for this idiom is genuinely complementary and worth doing — but a parallel lane owns that dispatcher right now (#2840). Routed as a follow-up in #2870 rather than colliding.

Verification

Mechanism, by execution. No artifact was created at any real drive root; subst mapped throwaway virtual drives onto scratch subdirectories, all removed with removal verified by listing. D:\d was never modified, moved or removed, and git worktree prune was never run.

The exact incident command shape, PowerShell, throwaway repo on subst drive P::

git -C P:\repo worktree add --detach /d/worktrees/ccp-reproA

created and registered P:\d\worktrees\ccp-reproA. Drive-dependence, same argument from two drives:

Set-Location P:\ ; git init /d/worktrees/probe   ->  P:\d\worktrees\probe
Set-Location Q:\ ; git init /d/worktrees/probe   ->  Q:\d\worktrees\probe

An independent agent, given no sight of the above reasoning, re-derived the same result from scratch on X:/Y: and isolated the responsible layer — the result flips with argv conversion alone, so git.exe resolves /y/... against the current drive and Git Bash normally masks it.

The predicate, four ways in one measurement. git rev-parse --sq-quote prints exactly what git.exe received and creates nothing:

bash -c 'git rev-parse --sq-quote /d/probe'                      ->  'D:/probe'   converted
bash -c 'MSYS_NO_PATHCONV=1; git rev-parse --sq-quote /d/probe'  ->  'D:/probe'   bare assignment: no effect
bash -c 'export MSYS_NO_PATHCONV=1; git rev-parse ... /d/probe'  ->  '/d/probe'   THE DEFECT
bash -c 'MSYS_NO_PATHCONV=1 git ... /d/a; git ... /d/b'          ->  '/d/a' then 'D:/b'   prefix scopes it

Measured precision (ADR-0003). Corpus: 16,919 tool_input.command strings from 701 local transcript JSONL files (388 MB), split by tool, plus 710 lines of PowerShell console history.

matcher	corpus	n	fires	rate
path shape /[a-z]/ (rejected)	Bash tool	14,234	6,506	45.7 %
path shape, narrowed to git worktree add (rejected)	Bash tool	137	111	81 %
exported suppressor (shipped)	Bash tool	14,234	46	0.32 %
exported suppressor	PowerShell tool + console history	3,395	0	0 %
per-command prefix — the safe idiom, correctly not fired on	Bash tool	—	193	—

Numbers are not a regex approximation: every command in the corpus was replayed through the built hook binary, with OSTYPE=msys, as a real PreToolUse payload on stdin.

MSYS-mentioning Bash cmds     total=204   blocked=46   allowed=158
control sample (no mention)   total=400   blocked=0    allowed=400

The 46 span four distinct lanes (ccp-measure2, ccp-silent-revert-calib, ccp-silent-revert-fixture, ccp-verify-2843) and two repositories, including the lane family that produced this incident. They are unseeded — the reproductions run for this work used the safe prefix form and are among the 193 correctly left alone — so this is ADR-0003's measured-precision path, not its seeded-defect exemption.

Precision claim, stated exactly. All 46 are genuine instances of the anti-pattern: the exported form always has unbounded blast radius over the rest of the command string, and the remedy is one keystroke. What the measurement cannot claim is a per-fire count of "would have produced litter" — that depends on which later command consumed a path, which is not statically decidable. Two honesty notes: the corpus is one machine's, the deployment surface but not a fleet; and it contains this investigation, which is why no count keyed on a bare mention of the variables is used.

Declared coverage gaps, in the hook header rather than hidden: a suppressor exported by a script the command invokes; set -a plus a bare assignment; an expansion-built value; and any spawner outside the two tool surfaces (CI runners, subprocess), which no PreToolUse hook can see.

Test plan

New plugins/guardrails/hooks/block-exported-msys-pathconv.test.sh — PASS=50 FAIL=0. Auto-discovered by scripts/run-plugin-tests.sh; no ci.yml change, so check-lane-coverage.sh is unaffected. Covers: the POSIX host gate; the real #2870 incident shape; export, declare -x, typeset -x, chained-&&, both-variables and leading-unrelated-assignment forms; the safe per-command-prefix and bare-assignment forms; six mention-not-setting cases (grep, commit prose, gh issue create title, unset, a similarly-named variable, echo); an ordinary git worktree add with an MSYS path, asserting the design decision not to match a path shape; the PowerShell surface; fail-closed over-length and payload cases; and the kill switch.

Everything else run locally against the branch:

gate	result
scripts/check-drive-root-litter.test.sh	PASS 16/16 (2 new assertions on the remediation text)
plugins/guardrails/hooks/block-windows-drive-tmp.test.sh	PASS 93/93 (unchanged behavior after the header edit)
require-jq-posture.test.sh (auto-enrolls the new hook)	PASS 40/40
require-jq-notice-isolation.test.sh	PASS 2/2, 10 distinct keys
scripts/check-silent-skips.sh	clean
scripts/check-hook-exec-form.sh	clean
scripts/check-hook-userconfig-argv.sh	clean
shellcheck -x (repo .shellcheckrc) on all changed shell	rc=0
scripts/check-shell-portability.sh origin/main	clean, 8 files
check-changelog-parity.sh --check / --check-bump / --check-order	all pass
scripts/validate-plugins.sh	pass
sync-plugin-options-docs.py --check / generate-catalog.mjs --check / generate-cheatsheet.mjs --check	all in sync
markdownlint-cli2 on every changed markdown	0 errors

Both new files are mode 100755.

Related

• Closes guardrails: nothing prevents an exported MSYS_NO_PATHCONV from silently unconverting a later path argument — D:\d recreated a third time by an ordinary lane #2870
• docs(conventions): own the Git Bash to Windows path-emit rule and detect its residue #2841 — landed the convention, helper and detector this completes
• Windows verification harnesses: an MSYS /d/ path literal silently redirected two tzdata sabotage cases into a duplicate of case E #2834 (closed by docs(conventions): own the Git Bash to Windows path-emit rule and detect its residue #2841) — same family, harness-authored producer
• guardrails: nothing catches a POSIX /tmp path resolving to a Windows drive-root C:\tmp #2594 (closed) — produced block-windows-drive-tmp.sh, the sibling guard
• source-control: nothing prevents a raw git worktree add or EnterWorktree(name:) from landing inside a repository #2611 (closed by feat(source-control): prefer melodic.worktreeroot for worktree placement #2643) — produced worktree-add-containment-gate.sh, the second guard that did not fire
• docs/adr/0003-verification-guards-earn-default-on-by-measured-precision.md
• melodic-software/dotfiles#486 — machine-level rule for the same mechanism

Review-round matcher changes (added after the five review threads)

The three review-round commits (6f462b3b8, a8462fe82, acbb881f0) changed the matcher's firing envelope in both directions, and one widening is easy to miss from the commit messages alone:

• Narrowed (review thread on quoted prose): without a shell word in the command string, the export keyword must now sit at command position, so commit messages, echo arguments, and grep patterns quoting export MSYS_NO_PATHCONV=1 are allowed.
• Widened (verification finding): quote normalization now strips quotes from BOTH sides of every token, so a shell name at the very end of a quoted string flips the matcher into loose mode. Concretely, git commit -m "do not export MSYS_NO_PATHCONV=1 in bash" now blocks (the trailing bash" reads as the word bash), while the same sentence not ending in the shell name — "...in bash, do not export MSYS_NO_PATHCONV=1" — stays allowed. This is inside the header's declared residual false-positive class, errs fail-closed with an instructive message, and is the accepted cost of closing a true-leak false negative: a fully quoted shell word ('bash', or "C:\Program Files\Git\bin\bash.exe" behind PowerShell's &) previously evaded the shell-word check entirely.
• Widened (verification finding, pre-existing gap closed): the assignment side of the token walk gets the same quote normalization, so bash -c "MSYS_NO_PATHCONV=1 bash -c '...'" — where the suppressor prefix is the first word of a quoted child command string — now blocks.

All declared residuals are documented in the hook header and pinned by tests (127 assertions, up from 66 at review time).

[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 1m 19s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: all 15 changed files (git diff origin/main...HEAD). The only executable surface is the new PreToolUse guard plugins/guardrails/hooks/block-exported-msys-pathconv.sh (and its test file); everything else is documentation, a manifest/CHANGELOG bump, a hooks.json registration entry, and a remediation-text edit in scripts/check-drive-root-litter.sh. No .github/workflows/* files are touched, so no Actions trust-boundary or permissions surface is in scope.

Findings: none.

Notes from the review, for context (not findings):

• The new hook (block-exported-msys-pathconv.sh) only pattern-matches tool_input.command with [[ =~ ]] — it never evals or re-executes the string, so there's no command-injection path through the guard itself.
• Telemetry emission (L426-L434) builds JSON via jq -n --arg, which is injection-safe, and the schema explicitly forbids carrying the full command string (privacy-safe subject only).
• The regex's declared coverage gaps (script-exported suppressor, set -a + bare assignment, expansion-built values, non-tool spawners) are false-negative gaps in a defensive/preventive guard, not exploitable vulnerabilities — they're documented in the hook header and PR body rather than hidden, consistent with this plugin's existing disclosure pattern.
• plugin.json's new block_exported_msys_pathconv_enabled kill switch defaults to true (fail-safe-on), matching the sibling guard's posture — no permission-widening or default-off regression.

No security-relevant findings in this PR.

[github-actions]

Last security-reviewed head: b8573da3177975b1ca0633b4ec062125ff4709b4. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6e3faaaa2

ℹ️ 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 56s —— View job

---

Claude's review

• Gather context (PR diff, CLAUDE.md, changed files)
• Review new guard block-exported-msys-pathconv.sh for correctness
• Review test coverage
• Review doc/schema/catalog updates for consistency
• Post findings as inline comments
• Post summary review

Summary

Reviewed both commits (5b3bfc7, b6e3faa) against origin/main. This is a well-evidenced, carefully-scoped PR: the guard is keyed on the actual mechanism (an exported suppressor), not a path shape, with measured precision data backing the design choice, and every doc surface (catalog, kill-switch table, telemetry schema/example, changelog, version bump, sibling cross-reference) is wired consistently. check-drive-root-litter.sh's remediation fix (registered-worktree awareness before deletion) is correct and has matching new test assertions.

One correctness finding, posted inline on plugins/guardrails/hooks/block-exported-msys-pathconv.sh:161:

The is_exported_suppressor regex only matches the single-flag spelling declare -x / typeset -x. It does not match combined flags (declare -gx, declare -xg) or separately-given flags (declare -x -g, declare -g -x) — and declare -gx is the idiomatic way to export a variable as global from inside a shell function, not an exotic spelling. A command like declare -gx MSYS_NO_PATHCONV=1; git worktree add /d/worktrees/x passes the substring pre-filter but is not matched by the regex, so it sails through unblocked — reproducing the exact defect class this PR exists to close, just one flag-order away from what's tested. The test suite only covers the plain -x forms, so this gap isn't caught by CI.

I traced this by hand (the sandbox here blocks executing arbitrary bash scripts, only specific git read commands are permitted), but the ERE logic is unambiguous: the alternation declare[[:space:]]+-x requires the literal -x token to appear immediately after declare and whitespace, so any other flag between or combined with it breaks the match.

Everything else — the host gate, the fail-closed posture (NUL byte, oversized command, missing jq), the telemetry wiring, and the doc/count bookkeeping across 8+ surfaces — checked out.

[github-actions]

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

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head)
• Apply skip gate
• Post final results

Skip gate hit — no new security-relevant surface.

The last security review covered head b6e3faa (all 15 files, full diff) with no findings. The only change since then is 87289bf — "use portable placeholders in the new guard's examples" — and its diff is a pure cosmetic substitution across three files:

• docs/conventions/windows-path-emit/README.md — example path text
• plugins/guardrails/hooks/block-exported-msys-pathconv.sh — a remediation-message string literal (D:/repos/... → <repo-root>)
• plugins/guardrails/hooks/block-exported-msys-pathconv.test.sh — corresponding test-fixture strings

None of this touches the guard's matching logic (is_exported_suppressor, the host gate, the fail-closed paths, or telemetry emission), any GitHub Actions workflow, permissions, or dependency pins — it only swaps a literal example path for a generic placeholder in human-facing text. No logic, trust-boundary, or injection surface changed.

No security-relevant surface to review on this update — no findings.

[claude]

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

---

Code review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, changed files)
• Apply skip gate
• Review changed files for correctness/maintainability
• Post results

Skip gate applied — condition 4. This lane already posted a full code-review pass on this PR at commit b6e3faa (the "Claude's review" comment above, 2026-08-16T21:37:15Z), with one correctness finding inline on plugins/guardrails/hooks/block-exported-msys-pathconv.sh:161.

The only commit since then, 87289bf ("use portable placeholders in the new guard's examples"), is a pure text substitution — D:/repos/... → <repo-root> / <drive>:/<repo-root>/... — across the doc, the hook's block-message string, and three test fixture strings. I diffed it directly (git show 87289bf): it does not touch is_exported_suppressor or any other matching logic in block-exported-msys-pathconv.sh, and the test suite still asserts the same 50 cases.

That means the prior finding still applies unchanged: is_exported_suppressor matches only the single-flag spelling declare -x / typeset -x and misses combined/multi-flag forms (declare -gx, declare -xg, declare -x -g, typeset -gx) — declare -gx MSYS_NO_PATHCONV=1; git worktree add /d/worktrees/x still passes the guard unblocked. Codex's review on the same commit independently raised two related gaps on the same line (raw-substring matching over quoted/comment/heredoc text producing false positives, e.g. git commit -m "fix: block export MSYS_NO_PATHCONV=1"; and unhandled export --/declare -rx forms).

No new files, and no changes to reviewable logic, since that pass — re-reviewing and re-posting the identical finding would be noise. Nothing further to add from this lane at 87289bf.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Code review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, changed files)
• Apply skip gate — new logic since last review, does not apply
• Review new commit a7b8291 ("also block a suppressor prefixed onto a child shell")
• Check test coverage for the new behavior
• Post findings

Summary

The prior review rounds on this PR (through 87289bf) already covered everything up to and including the is_exported_suppressor matcher, and this lane's earlier pass on 87289bf correctly skip-gated (cosmetic-only diff). The newest commit, a7b8291, is not cosmetic — it adds a second matcher, leaks_into_child_shell, to close a real gap the author found via adversarial review: a per-command prefix (MSYS_NO_PATHCONV=1 bash -c '...') scopes to one process, and when that process is a shell, the suppressor leaks into every command inside it. That reasoning is sound and well-evidenced (measured zero new false positives on the existing 14,234-command corpus), and doc/schema/changelog/README/options-table wiring for the new "child-shell" telemetry form is consistent across all six touched surfaces. New tests cover bash -c, env ... bash -c, sh -c, and an absolute path directly to bash — all pass, and correctly leave the safe non-shell-prefix idiom (MSYS_NO_PATHCONV=1 git show ...) alone.

One correctness finding, posted inline on plugins/guardrails/hooks/block-exported-msys-pathconv.sh:184:

leaks_into_child_shell only recognizes a bare literal env token as an optional launcher between the suppressor assignment and the shell word. Traced by hand (this sandbox permits only specific git read commands, not arbitrary script execution): MSYS_NO_PATHCONV=1 /usr/bin/env bash -c '...' — a very ordinary, POSIX-habitual spelling — does not match, because the final [^[:space:]]*(bash|sh|dash|zsh|ksh) clause has to consume the token /usr/bin/env itself, which doesn't end in any listed shell suffix, and there's no other occurrence of MSYS_NO_PATHCONV= in the string to re-anchor the match from. command bash -c '...' has the same gap. This isn't in the header's declared-gaps list, and it's the identical failure shape as the declare -gx finding from the earlier review round on the export matcher: a plausible spelling one hop away from what's tested slips through a guard whose entire thesis is "false negatives are unacceptable here." Test coverage only exercises the bare-env-prefix and direct-absolute-path forms, not the path-qualified-env form, so CI doesn't catch it either.

Everything else in this commit — the corpus-measured false-positive cost, the doc/schema/changelog consistency, the non-leaking-idiom tests staying green — checked out.

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head b6e3faa, all 15 files reviewed with no findings). Two commits landed since: 87289bf (cosmetic placeholder text, previously confirmed no new surface) and the current head, a7b8291 — "also block a suppressor prefixed onto a child shell."

a7b8291 touches executable guard logic, so I reviewed it directly rather than invoking the skip gate on it:

• block-exported-msys-pathconv.sh — adds leaks_into_child_shell(), a second [[ =~ ]] matcher for MSYS_NO_PATHCONV=1 bash -c '...' / env MSYS_NO_PATHCONV=1 sh -c '...' forms. This tightens the guard (blocks more commands that were previously allowed); it doesn't loosen any check, doesn't eval/re-execute the matched string, and doesn't touch the telemetry payload construction (still jq -n --arg, still no full-command field — see schema). The new regex runs only after the existing MAX_COMMAND_LEN=16384 fail-closed cap, so it inherits the same bound on worst-case input size as the already-reviewed is_exported_suppressor matcher.
• block-windows-drive-tmp.sh — comment-only cross-reference addition, no logic change.
• Remaining changes in this commit are the test file, changelog, and doc prose reflecting the new matcher — no executable surface.

Findings: none. No injection path, no trust-boundary change, no permission-widening, no .github/workflows/* touched.

Note, not a security finding (out of scope for this lane — flagged already by the code-review lane and by Codex's review): is_exported_suppressor still only matches the single-flag spelling declare -x / typeset -x and misses combined forms like declare -gx. That's a completeness/correctness gap in a defensive guard against an agent's own accidental Windows path mishandling, not an exploitable vulnerability — no untrusted input or trust boundary is involved, consistent with why this lane didn't raise it originally.

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head a7b8291, reviewed with no findings). One commit has landed since: 8014042 — "replace the child-shell ERE with a linear token walk." It touches executable guard logic (not cosmetic), so I reviewed it directly rather than invoking the skip gate.

What changed: leaks_into_child_shell() in block-exported-msys-pathconv.sh was rewritten from a single ERE to a linear token walk, specifically to fix a catastrophic-backtracking hang the author found on a real corpus string — a legitimate defensive fix (an unbounded hook is worse than one with a gap, since PreToolUse has a timeout). Only this hook and its test file changed; no workflow, permissions, or telemetry-schema surface is touched.

Findings: none that meet this lane's scope (no untrusted-input/trust-boundary crossing, no injection path, no permission-widening — this hook still only pattern-matches tool_input.command, never evals it, and the telemetry payload construction is unchanged).

Note, not a security finding (out of scope for this lane, consistent with how the prior declare -gx gap was handled — matcher-completeness gaps in this defensive, self-mishap-guarding hook are the code-review lane's territory): tokens=($s) at line 194 is an unquoted array assignment, and set -f/set -o noglob is never enabled anywhere in the script (verified: only set -uo pipefail at the top). In bash, an unquoted arr=($var) performs pathname expansion in addition to word-splitting — the # shellcheck disable=SC2206 comment above it only addresses the word-splitting half of what that check warns about. Concretely, with the repo root as the hook's working directory (confirmed via ls -1: AGENTS.md CLAUDE.md LICENSE README.md REVIEW.md SECURITY.md _typos.toml docs lib lychee.toml package-lock.json package.json plugins prompts ruff.toml scripts), a command word landing in the "prefix" position that contains a glob metacharacter — e.g. MSYS_NO_PATHCONV=1 * -c 'git worktree add /d/worktrees/x' — has its * token silently expanded into that whole file listing before the classifier ever runs, so the loop checks AGENTS.md's basename instead of the real command word, seen resets, and the match is missed. The blast radius is narrow (only the token immediately after the suppressor/env/NAME=value chain is at risk, and typical command words like bash/git have no glob characters to trigger expansion at all), and the test suite exercises no glob-bearing case, so this wouldn't be caught by CI. read -ra tokens <<< "$s" would word-split without the pathname-expansion side effect. Flagging for the code-review lane / author's own tracking rather than as a security finding, since it's the same completeness-gap category as the declare -gx and env-launcher gaps already raised on this PR, not an attacker-controlled trust-boundary issue.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Code review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, changed files)
• Apply skip gate — new logic since last review, does not apply
• Review new commit 8014042 ("replace the child-shell ERE with a linear token walk")
• Check test coverage for the new implementation
• Post findings as inline comments
• Post summary review

Summary

Skip gate checked and does not apply: the code-review lane's last pass covered through a7b8291 (with a finding on that commit's leaks_into_child_shell regex), and 87289bf was already correctly skip-gated as cosmetic-only. The current head, 8014042, rewrites leaks_into_child_shell from a single ERE to a linear token walk — that's touched logic, not a pre-existing/untouched-line issue, so it's in scope for this round.

The commit's stated goal — fix a catastrophic-backtracking ERE that hung on a real corpus string (MSYS_NO_PATHCONV=1 git show 'origin/main:.claude/settings.json') and falsely matched the sh inside show — is achieved. The token walk is linear, and the two new regression pins (the corpus string, and a command whose name merely ends in sh) pass and correctly exercise that fix.

One correctness finding, posted inline on plugins/guardrails/hooks/block-exported-msys-pathconv.sh:199:

The rewritten launcher check only recognizes a bare literal env/env.exe token between the suppressor assignment and the shell word. Traced by hand: MSYS_NO_PATHCONV=1 /usr/bin/env bash -c 'git worktree add /d/worktrees/x' is not blocked — /usr/bin/env fails the literal env comparison, falls through to the "command word" branch, gets basenamed down to env (which isn't a recognized shell), and the walk concludes the prefix ended there and never re-examines the following bash token. command bash -c '...' has the same gap. This is the identical failure the code-review lane flagged on the prior regex implementation at a7b8291 (/usr/bin/env bash -c slipping through) — it survives the rewrite unchanged, because the basename normalization applied to the final command-word check (base="${tok##*/}", two lines below) is never applied to the env launcher check itself. It's not in the header's declared-coverage-gaps list, and the new tests this commit adds don't cover it (only bare-env-prefix and absolute-bash-path forms are exercised) — so CI doesn't catch it either. Suggested fix and test additions are in the inline comment.

Everything else in this commit — the token-walk correctness for the cases it does handle (bare env, NAME=value chains, quoted/backslash/.exe normalization on the final command word, the show/refresh non-shell regression pins) — checked out.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Code review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, changed files)
• Apply skip gate — condition 4 (already-reviewed head, no new review surface)
• Review new commit 976e3b0 ("add an explicit default case to the token-walk scanner")
• Post results

Skip gate applied. This lane's last full pass on this PR covered through 8014042 (2026-08-16T23:29:18Z), with one open finding on leaks_into_child_shell's launcher check. The only commit since then, 976e3b0, adds a single *) ;; default arm (plus a comment) to the case "$tok" inside leaks_into_child_shell, purely to satisfy ShellCheck's optional SC2249 check. I read the diff directly: it's a no-op branch on an already-exhaustive scan step — the loop already falls through for any token that isn't a MSYS_NO_PATHCONV=*/MSYS2_ARG_CONV_EXCL=* assignment, and an explicit empty *) arm changes nothing about which tokens set seen, how the walk advances, or the function's return value. No test assertions changed, and none needed to.

Nothing in this commit touches is_exported_suppressor or the launcher-recognition logic, so the two correctness findings already on record for this PR still stand unchanged and unaddressed as of this head:

• is_exported_suppressor still only matches the single-flag spelling declare -x/typeset -x, missing combined/multi-flag forms (declare -gx, declare -xg, declare -x -g) — flagged inline at block-exported-msys-pathconv.sh#L167 in the 2026-08-16T21:41:19Z round.
• leaks_into_child_shell's launcher check (now around L199-L204) still compares the raw token against literal env/env.exe rather than a basename-normalized form, so a path-qualified launcher (/usr/bin/env bash -c '...') or command bash -c '...' still slips through unblocked — flagged inline at the 2026-08-16T23:35:01Z round.

Since this commit is a pure lint/style no-op with no new logic to review, there's nothing new to anchor an inline comment to, and re-posting the two standing findings verbatim would be noise rather than signal — they remain open for the author's tracking as recorded above.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate
• Post final results

Skip gate hit — no new security-relevant surface.

The last full security pass on this PR covered head 8014042 (all executable-logic changes reviewed, no findings). The only commit since then is 976e3b0 — "add an explicit default case to the token-walk scanner" — and its entire diff is:

 plugins/guardrails/hooks/block-exported-msys-pathconv.sh | 4 ++++
 1 file changed, 4 insertions(+)

specifically a no-op *) ;; default branch added to the case statement inside leaks_into_child_shell() (block-exported-msys-pathconv.sh#L215-L218), added purely to satisfy ShellCheck's SC2249 (require-default-case) lint. It changes nothing about which tokens are matched, how the walk terminates, or the guard's allow/deny verdict — an unmatched token already fell through to the next loop iteration before this change; now that fall-through is explicit. No new branch, no new input handling, no change to the telemetry payload, and no .github/workflows/* file touched.

No security-relevant surface to review on this update — no findings.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — does not apply, new logic since last review
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head 976e3b0, reviewed with no findings — a pure lint no-op). One commit has landed since: 6f462b3 — "address all five review findings on the suppressor matcher." It rewrites core matching logic in block-exported-msys-pathconv.sh (not cosmetic), so I reviewed it directly rather than invoking the skip gate.

What changed: the commit resolves all five open review threads on this PR (the Codex P1 quoted-prose over-firing finding, the Codex P2 export --/declare -rx under-firing finding, this lane's own declare -gx finding, and the two /usr/bin/env bash -c launcher findings) by adding contains_shell_word(), rewriting is_exported_suppressor() to match flag clusters and command position, and extending the token-basename normalization in leaks_into_child_shell() to launchers beyond bare env.

Findings: none that meet this lane's scope. This is still a pure [[ =~ ]] pattern-matcher over tool_input.command — it never evals or re-executes the string, the telemetry payload construction is untouched (still jq -n --arg, no full-command field), and no workflow, permissions, or trust-boundary surface is touched. I checked the new nested-quantifier regexes in is_exported_suppressor (the (-[A-Za-z-]+[[:space:]]+)+ flag-cluster group followed by the assign group) for the same catastrophic-backtracking shape that forced the leaks_into_child_shell rewrite in an earlier commit: unlike that case, the flag-cluster and assignment groups are anchored on disjoint leading characters (- vs. [A-Za-z_]), so there's no combinatorial split ambiguity between them — this doesn't reproduce that bug.

Two notes, not findings (consistent with how this lane has scoped every prior round on this PR — completeness/false-negative gaps in this self-protective, non-privileged guard are the code-review lane's territory, not a trust-boundary or injection issue, since nothing here processes attacker-controlled input across a privilege boundary):

• The commit's own header now explicitly declares a new residual gap: an export guarded by a keyword rather than a separator (if true; then export MSYS_NO_PATHCONV=1; fi; git worktree add /d/worktrees/x) is not at a recognized "command position" and has no shell word in the outer command string, so it is not matched — even though then genuinely executes the export in the top-level shell. This is disclosed in the header rather than hidden, consistent with this plugin's existing disclosure pattern, so it doesn't change my assessment.
• contains_shell_word() (new) reuses the same unquoted tokens=($s) word-split pattern (# shellcheck disable=SC2206, no set -f) already noted as an out-of-scope completeness gap on leaks_into_child_shell in the 2026-08-16T23:29:11Z round — a glob-bearing token could still spuriously widen or narrow which mode is_exported_suppressor uses. Same category, same reasoning: narrow blast radius, no test coverage either way, flagged for the code-review lane rather than as a security finding.

No injection path, no permission-widening, no .github/workflows/* touched.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — does not apply, new logic since last review
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head 6f462b3, reviewed with no findings). One commit has landed since: a8462fe — "strip trailing quotes so a quoted shell word is recognized." It touches executable guard logic (not cosmetic), so I reviewed it directly rather than invoking the skip gate.

What changed: contains_shell_word() and the launcher-normalization step inside leaks_into_child_shell() previously stripped only a leading quote character from each token before comparing its basename to a known shell name. A fully quoted shell word ('bash', or the PowerShell call-operator form "C:\Program Files\Git\bin\bash.exe") therefore normalized to bash' / bash.exe", matched no known shell, and caused is_exported_suppressor to fall back to its stricter command-position-only matching mode — silently regressing a case the review-finding commit (6f462b3) itself was supposed to have fixed. This commit adds the symmetric trailing-quote strip (${tok%\'} / ${tok%\"}) at both sites.

Findings: none. This is a pure detection-tightening fix to a [[ =~ ]] / token-walk pattern-matcher — it doesn't eval or re-execute tool_input.command, doesn't touch telemetry payload construction, and doesn't touch any .github/workflows/* file. The direction of the bug it fixes (a quoted shell word silently falling through to a stricter matching mode, i.e. under-blocking) is consistent with this guard's existing fail-closed posture, and the fix moves strictly toward blocking more of the real attack surface, not less. I checked the quote-stripping order for a mismatched-quote edge case (e.g. a token like "bash'): the four strips run unconditionally regardless of which quote character is actually present, so mismatched pairs still normalize to the bare word — this can only widen matches (more blocked), never create a bypass.

Three new tests (quoted bare shell word, quoted absolute shell path, PowerShell call operator on a quoted bash.exe path) pin the previously-regressed cases as blocked; suite grows from 117 to 124 assertions.

No injection path, no permission-widening, no trust-boundary change.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — does not apply, new logic since last review
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head a8462fe, reviewed with no findings). One commit has landed since: acbb881 — "normalize quotes on the assignment side of the token walk." It touches executable guard logic (not cosmetic), so I reviewed it directly rather than invoking the skip gate.

What changed: in leaks_into_child_shell(), quote-stripping (${tok#\'}, ${tok#\"}, ${tok%\'}, ${tok%\"}) now runs once at the top of the token loop, for every token, before either the seen-prefix assignment check or the command-word/launcher/shell check. Previously it only ran inside the ((seen)) branch, after the NAME=value continuation check — so the entry-point check (case "$tok" in MSYS_NO_PATHCONV=* | ...) and the prefix-continuation check never saw a quote-stripped token. That meant the first word of a quoted child command string, e.g. bash -c "MSYS_NO_PATHCONV=1 bash -c '...'", word-split to "MSYS_NO_PATHCONV=1 and never matched MSYS_NO_PATHCONV=*, so the inner leak (a suppressor exported into a nested child shell) went unblocked.

Findings: none. This is a strict tightening of an already-reviewed [[ =~ ]]/token-walk pattern matcher — it moves an existing quote-stripping operation earlier in the same loop so it applies to more tokens, closing a false negative rather than opening a new code path. I traced the change for the inverse risk (a token previously correctly classified now being missed because stripping runs earlier) and found none: the stripped token is used identically at every downstream check point (assignment-glob match, NAME=value regex, basename derivation), so applying the same normalization earlier can only cause a token to match a pattern it didn't match before, never the reverse. The commit doesn't touch is_exported_suppressor, contains_shell_word's ordering, telemetry construction (still jq -n --arg, no full-command field), the host/kill-switch gates, or any .github/workflows/* file. This remains a pure pattern-matcher over tool_input.command — it never evals or re-executes the string.

One new test (prefix at the start of a quoted child command string (blocked)) pins the fixed case; suite grows from 124 to 127 assertions.

No injection path, no permission-widening, no trust-boundary change.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — condition 3, no new security-relevant surface
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Skip gate hit — no new security-relevant surface.

The last full security pass on this PR covered head acbb881 (all executable guard logic reviewed, no findings). Three commits have landed since:

• 465178c — an unrelated origin/main commit (feat(docs-hygiene): add audit-progressive-disclosure skill) pulled in by the merge below; it touches no file in this PR's change set.
• 5766357 — merge of origin/main into the branch (resolving a docs/CATALOG.md conflict from the stale-base-overlap gate); the merge itself introduces no diff of its own beyond what each side already contributed.
• de2e109 — "satisfy the comment-hygiene and portability gates," the only commit touching files in scope. I diffed it directly (git show de2e109): it rewords six code comments in block-exported-msys-pathconv.sh / .test.sh to drop PR-number references (required by the repo's comment-hygiene gate), and adds a portability-ok annotation comment above an existing PowerShell test fixture string to satisfy the shell-portability gate. Confirmed by inspection that no regex, token-walk logic, matcher behavior, telemetry payload construction, or test assertion values changed — only comment text and one added comment line.

No injection path, no permission-widening, no trust-boundary change, and no .github/workflows/* file touched.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — does not apply, new content since last review
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final results

Scope reviewed: the diff since the last full security pass (head de2e109, reviewed with no findings). The current head, 13c1c62, is a merge of origin/main (bringing in 0ef753f, PR #2840/#2876's fixture-git-isolation work) with a hand "re-resolution" of the guardrails version-bump conflict, per its own message.

The inherited main-side content (scripts/check-fixture-git-isolation.sh, the new fixture-git-isolation-gate CI job, etc.) already went through its own review on main and isn't this PR's authorship — the new fixture-git-isolation-gate job in .github/workflows/ci.yml uses a pinned actions/checkout SHA with persist-credentials: false and runs only repo-local test scripts — no new trust-boundary or permissions surface.

Findings: none that meet this lane's scope (no injection path, no eval/re-execution of untrusted input, no trust-boundary change, no permission-widening).

Note, not a security finding, but worth surfacing loudly — the manual conflict re-resolution in this merge commit dropped the new guard's manifest entry entirely. plugins/guardrails/.claude-plugin/plugin.json at this head:

• Has no block_exported_msys_pathconv_enabled key under config at all (compare the surviving sibling entries at L34-L57 — block_no_verify_enabled, block_dangerous_git_enabled, block_hook_bypass_enabled, block_windows_drive_tmp_enabled, block_noncanonical_commit_enabled, block_convention_gate_enabled are all present; the new guard's is not).
• The manifest description now reads "Thirteen safety guards" with the MSYS-suppressor clause removed, while hooks.json still registers the hook, and README.md, docs/CATALOG.md, and CHANGELOG.md all still say "Fourteen" and still document the block_exported_msys_pathconv_enabled kill switch (e.g. README.md#L16-L17, #L204, #L345).

Practically, this doesn't create an exploitable gap: hook::is_enabled (plugins/guardrails/hooks/hook-utils.sh:41) defaults an unmirrored option to "true", so the guard keeps blocking — the failure direction is "the advertised kill switch can no longer be turned off," not "the guard silently stops running." But it does mean README.md's generated options table and docs/CATALOG.md's description are now stale relative to plugin.json (both are machine-generated from plugin.json by scripts/sync-plugin-options-docs.py and scripts/generate-catalog.mjs respectively), which — unlike the matcher-completeness gaps raised in earlier rounds — is very likely to fail sync-plugin-options-docs.py --check and generate-catalog.mjs --check, both gates this PR's own test plan reports as passing (against a pre-merge state). Flagging for the author/code-review lane rather than as a security finding, since no untrusted input or trust boundary is involved and the guard itself stays fail-closed.

No injection path, no permission-widening, no new .github/workflows/* trust surface.
· branch fix/2870-block-exported-msys-pathconv

[claude]

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

---

Security review

• Gather context (PR diff since last reviewed head, changed files)
• Apply skip gate — condition 3, no new security-relevant surface
• Post final results

Skip gate hit — no new security-relevant surface.

The last full security pass on this PR covered head 13c1c62 (all executable guard logic reviewed, no findings; one non-security note flagged that the manual merge-conflict resolution had silently dropped the manifest's block_exported_msys_pathconv_enabled kill switch and "Fourteen"-guard description). The current head, b8573da, is a merge commit whose own message is "Merge origin/main, restore the manifest the first resolution clobbered."

Restricting to this PR's actual scope (git diff origin/main...HEAD, which still totals exactly the 15 files / 803 insertions / 10 deletions this PR's body describes — confirming the merge resolved cleanly and pulled in nothing extra), the diff between the last-reviewed head (13c1c62) and this head (b8573da) touches only three files, and all three are non-executable bookkeeping:

• plugins/guardrails/.claude-plugin/plugin.json — restores the dropped block_exported_msys_pathconv_enabled kill-switch entry and the "Fourteen safety guards" description, resolving exactly the discrepancy the prior review flagged.
• plugins/guardrails/CHANGELOG.md — re-inserts the 0.28.33/0.28.32 entries that main published in the interim, ahead of this PR's 0.29.0 entry, restoring correct changelog ordering.
• docs/CATALOG.md — picks up an unrelated one-line catalog entry (ai-slop plugin) and a code-tidying description update from main; not authored by this PR.

No guard logic, test file, telemetry schema, or hooks.json registration changed between these two heads (block-exported-msys-pathconv.sh, its test file, block-windows-drive-tmp.sh, hooks.json, check-drive-root-litter.sh/.test.sh, and the telemetry schema/example are all byte-identical to what was already reviewed at 13c1c62). No .github/workflows/* file is touched by this PR.

No security-relevant surface to review on this update — no findings.
· branch fix/2870-block-exported-msys-pathconv