fix(repo-hygiene): close the three HIGH clean-skill audit findings

[kyle-sexton]

Closes #3850

Summary

Four small, independently reversible fixes to repo-hygiene:clean, bundled because the operating-model contract on #3346 says PR 1 lands them together. Each was a case of the skill promising or implying a safety property the code did not deliver: a guard registered where it could never fire, a pull-request lookup that discarded evidence without saying so, a repository-root resolution that ended the run, and a frontmatter comment naming a gate that does not cover what it claimed.

A follow-up commit (400845c3) answers the fresh-context review: the guard now has an acknowledgement path on the PowerShell tool, section 4.3 consumes the stash audit's PR-map status line, and the shell-portability note below is corrected. A second follow-up (04bbe657) hardens the ack dispatch to default-deny and quotes the PowerShell stash selector, both described under 1c below.

Fix

1. The destructive guard never saw PowerShell. The frontmatter hook registered matcher: "Bash" while destructive-guard.sh has always carried a PowerShell spelling (Remove-Item[[:space:]].*-Recurse). A Bash-only matcher can never hand the guard a PowerShell tool call, so that pattern was unreachable and on a host whose primary shell is PowerShell the guard was absent, not merely partial. Now matcher: "Bash|PowerShell". The spelling was verified against the local marketplace rather than assumed: it is what plugins/guardrails/hooks/hooks.json, plugins/disk-hygiene/skills/clean/SKILL.md, scripts/check-hook-exec-form.test.sh and docs/conventions/hook-budget/README.md all use. A PowerShell tool call carries its command in the same .tool_input.command field Bash uses (plugins/guardrails/hooks/block-dangerous-git.sh reads exactly that).

1b. The guard accepts a PowerShell-valid acknowledgement (review follow-up). Registering the guard on the PowerShell tool exposed a gap: the only documented ack, the Bash prefix CLEAN_GUARD_ACK=1 <command>, is not something PowerShell can execute (pwsh -NoProfile -Command 'CLEAN_GUARD_ACK=1 Write-Host hi' exits 1, "term 'CLEAN_GUARD_ACK=1' is not recognized"), and the PowerShell-valid $env:CLEAN_GUARD_ACK=1; Remove-Item -Recurse -Force obj still exited 2 from the guard. So a confirmed Remove-Item -Recurse, git reset --hard, or git stash drop on the PowerShell tool had no ack path short of the kill switch, and the test case "ack prefix works on the PowerShell tool too" asserted a spelling PowerShell cannot run. The guard now reads tool_name and accepts, on the PowerShell tool only, $env:CLEAN_GUARD_ACK=1; <command>: optional leading whitespace, the $env:CLEAN_GUARD_ACK assignment with the literal value 1 (bare or quoted), then the ; that ends the statement, and nothing else before it. The Bash prefix stays Bash-only. Each spelling is a real assignment only in its own shell (Bash treats $env:CLEAN_GUARD_ACK=1; as a failed command lookup and runs the rest anyway), so the guard accepts it only there. The block reason names the spelling for the tool that was blocked. SKILL.md documents both spellings in the guard paragraph and in the stash-drop instruction.

1c. Tool gating hardened to default-deny, and the PowerShell stash selector quoted (second review follow-up, 04bbe657). Two bounded changes, neither of which closes a live bypass.

Tool gating. The ack dispatch was written as "PowerShell gets the $env: spelling, everything else gets the Bash prefix", so tool_name absent, "", null, 123, powershell, Powershell, "PowerShell " and Foo all took the Bash path and the Bash prefix counted as an ack there. Nothing was open: a bare destructive command exited 2 under every one of those, and PowerShell cannot execute the Bash prefix anyway. The problem was future-facing, a third shell tool added later would silently inherit an ack path nobody chose to give it. The dispatch is now default-deny on tool_name: the Bash prefix only when the tool is exactly Bash, the $env: spelling only when it is exactly PowerShell, and any other or missing tool_name gets no ack path at all. Blocking is byte-identical for every tool; the block reason for an ungated tool now says there is no acknowledgement path instead of naming a spelling that tool cannot honour. The test corpus gains 42 gating cases (Foo, powershell, Powershell, "PowerShell ", " Bash", bash, Bash2, string "123", string "null", empty string, JSON null, JSON 123, and an absent tool_name key), each asserting that neither ack spelling unblocks, that a bare destructive command still exits 2, and that a benign command still exits 0. Mutating the dispatch back to the old "not PowerShell" form kills 15 of the new assertions, and leaves the blocking assertions green, which is the correct signature for a grant removal rather than a bypass fix.

Stash selector quoting. The documented PowerShell drop and the matching test payload used a bare git stash drop stash@{0}. Under pwsh that is splatting syntax, git receives a mangled argument and reports unknown switch `e', and the stash count does not change, so the documented example did not do what it said. Verified live: unquoted exits 129 with two stashes still present, git stash drop 'stash@{0}' exits 0 and drops one. The ack itself always propagated correctly; only the selector needed quoting, and only on the PowerShell lane. Fixed in SKILL.md §4.3 and in the test corpus, with the live pwsh run now part of the suite (guarded by command -v pwsh, skipping cleanly without it). The Bash lane needs no quoting and is untouched. context/git-branch-cleanup.md carries no stash example, and a sweep for stash@ across the plugin found no other PowerShell-context occurrence.

Out of scope and unchanged, per the review: the ack regex itself (a 61-payload attack pass found no bypass) and the guard's destructive-command coverage (#3852, which also records the unmatched git stash drop;).

2. The pull-request map was silently truncated, at both sites. git-branch-audit.sh and git-stash-audit.sh each ran gh pr list --state all --limit 200 with errors suppressed on both the lookup and its parsing. That lookup is the documented mitigation for squash merges, which git branch --merged structurally cannot see, so a short or missing map did not soften a verdict, it inverted one: a branch whose work had landed lost the only evidence that it landed and reported as unmerged or needing review. Worse, an unauthenticated or missing gh produced an empty map indistinguishable from a repository with no pull requests.

Both sites now share one clean_pr_map helper in scripts/lib/clean-common.sh that:

• raises the cap to an explicit high --limit (per the parent's recorded correction, gh pr list has no unlimited sentinel and rejects --limit 0), overridable with CLEAN_PR_LIST_LIMIT;
• detects truncation by equality, returned count against requested limit, and emits PRDataTruncated:;
• emits PRCount: <n> for a usable map, so PRCount: 0 is a real answer;
• emits PRDataUnavailable: <why> when gh or jq is missing, the call fails, or the output is not a PR array, and emits no count in that case, so a failed lookup cannot be read as an empty repository.

Fixing only the branch-audit site (the one the audit reported) would have left half the bug, so the stash-audit site is fixed with it. Section 4.3 of SKILL.md now tells the reader to read the status line first and surface it in the confirmation gate, worded the way section 4.2 already did for the branch audit (review follow-up).

3. An unguarded repository-root resolution aborted the menu path. Step 0 ran a bare REPO_ROOT=$(git rev-parse --show-toplevel) on a path the skill documents as reachable from anywhere; outside a repository that exits 128 and takes the run down with it. Step 0 now resolves no root at all, and the tiers that need one (§4 git, §6 tree) resolve it themselves with 2>/dev/null and stop that tier on an empty result. The bundled scripts already self-resolve and print Error: not a git repository.

4. The frontmatter claimed guard coverage that does not exist. The allowed-tools comment said the six mutating scripts "stay behind the PreToolUse destructive guard and the permission flow". They do not: the guard matches destructive command shapes and matches none of those scripts, nor git branch -D, nor git push --delete. Withholding the grant, so the permission flow and the confirmation gate apply, is the actual mechanism. The claim is corrected, the guard is not extended. The guard's header honestly declares it a best-effort net rather than a security boundary with accepted gaps; that posture is preserved and pointed at #3852, which owns the threat-model decision. The same overstatement in allowed-tools-pairing.test.sh's header comment and in plugin.json's userConfig description is corrected too.

Manifest bumped to 0.10.34 with a matching CHANGELOG entry. Re-verified free at 04bbe657: origin/main is at bf24e0e5 carrying 0.10.33, and no other open pull-request head claims 0.10.34 (all 28 other open heads were checked; the highest is 0.10.33).

Verification

Every acceptance criterion is met. Stated per criterion, with the commands that show it:

Criterion	Status	Evidence
PowerShell recursive force-removal is inspected and blocked, shown by a test	met	destructive-guard.test.sh asserts both halves: the registered matcher parsed out of SKILL.md reaches PowerShell, and three Remove-Item -Recurse -Force spellings driven through a tool_name: "PowerShell" payload exit 2. Asserting only the verdict would have passed with the bug fully intact, since the guard script was never the broken part.
A confirmed destructive command on the PowerShell tool has an acknowledgement path	met	Five $env:CLEAN_GUARD_ACK=1; spellings (bare, "1", '1', spaced) exit 0 on the PowerShell tool. When pwsh is on PATH the test executes the accepted spelling for real and asserts the value reaches the following command (ack=1), and asserts the Bash prefix exits 1 under pwsh.
Each ack spelling is accepted only on its own tool; an unknown tool gets none	met	42 gating cases over Foo, powershell, Powershell, "PowerShell ", " Bash", bash, Bash2, "123", "null", the empty string, JSON null, JSON 123, and an absent key: neither ack spelling unblocks, a bare destructive command still exits 2, a benign one still exits 0, and the block reason for an ungated tool contains no ack spelling at all.
The ack widening is tight	met	A 16-payload bypass corpus on the PowerShell tool stays at exit 2: token in a trailing comment, in a leading comment, inside a double-quoted and a single-quoted string, after the destructive command, inside a later pipeline segment (ForEach-Object { ... }), as the second statement, value 0, '', $null, 10, no ; separator, `
The documented PowerShell stash drop actually drops	met	Live pwsh against a two-stash fixture, now in the suite: git stash drop stash@{0} exits 129 with unknown switch `e' and both stashes still present; git stash drop 'stash@{0}' exits 0 and one stash is gone.
Existing Bash coverage unchanged	met	Every pre-existing Bash case in destructive-guard.test.sh is untouched and still passes; 131/131 green.
Neither audit script caps at a plausibly exceeded value; truncation reported	met	Both call sites go through clean_pr_map. Tests in both audit suites set CLEAN_PR_LIST_LIMIT=1 to reach the boundary without a 100000-row fixture and assert PRDataTruncated:.
A failed lookup is distinguishable from a repo with genuinely no PRs	met	Both suites assert PRCount: 0 with no PRDataUnavailable: for an empty-array gh, and PRDataUnavailable: with no PRCount: for a gh that exits 1 and for one that exits 0 with non-array output.
The menu path outside a repository does not terminate with an error status	met	Confirmed empirically in an empty non-repo directory: git rev-parse --show-toplevel exits 128 there, while resolve-clean-action.sh prints Action: menu and exits 0. The aborting call is gone from that path.
Frontmatter no longer claims coverage the guard lacks; posture preserved	met	allowed-tools-pairing.test.sh gains two assertions: one bans the specific overstatement, one requires any mention of the guard in allowed-tools to state its real scope, so a reworded overstatement cannot slip past.
affected-tests.sh --run selects and passes the mapped suites	met, with one pre-existing unrelated failure noted below	See the commands below.

Revert-sensitivity checked, not assumed. Each new contract was proven to fail when its fix is undone:

• reverting matcher to "Bash" → FAIL: [23] hook registration reaches the PowerShell tool — expected PowerShell in: - matcher: "Bash";
• reverting SKILL.md to HEAD → all three new allowed-tools-pairing assertions fail, including unguarded '$(git rev-parse --show-toplevel)' ends the run outside a repo: skills/clean/SKILL.md:122;
• the PowerShell ack cases were run against four guard mutations on a scratch copy, each caught: the accepted value changed from 1 to 2 (5 failures, the five accepted spellings), the start anchor dropped from the PowerShell regex (5 failures, the comment, quoted-string, and second-statement bypasses go green), the tool check removed so the PowerShell spelling counts on every tool (4 failures), and the guard as committed at 7d382d31 before the first follow-up (7 failures, including the replaced case that used to assert the Bash prefix on PowerShell);
• mutating the 04bbe657 dispatch back to "PowerShell gets $env:, everything else gets the Bash prefix" → 15/131 checks failed, every one of them a new gating assertion (13 "Bash ack prefix grants nothing" cases plus the two block-reason assertions). The "bare destructive still blocked" and "benign still allowed" cases stay green under that mutation, which is the expected signature: the old dispatch handed out an ack nobody chose, it did not fail open.

Commands run, in the foreground, with actual results:

• bash scripts/affected-tests.sh --run --shard <i>/4 for legs 0 through 3, run one after another (the unsharded run exceeds the tool ceiling; the legs are the same selection partitioned four ways). Legs 0, 2 and 3 exit 3 (success, with 14 NOT RUN non-shell suites delegated to their own lanes). Leg 1 exits 1 on a single pre-existing failure in a file this PR does not touch: plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh, 2 of 24 process-budget cases, and the same 2 fail identically on a clean git archive origin/main export at bf24e0e5 (measured -1 — the pid-stamped PS4 did not reach the traced shell). destructive-guard.test.sh runs in leg 1 and passes. scripts/check-shell-portability.test.sh and plugins/desktop-notification/hooks/desktop-notification.test.sh, both intermittent pre-existing failures noted in earlier runs, passed in this one.
• bash plugins/repo-hygiene/skills/clean/scripts/destructive-guard.test.sh → All 131 checks passed. (was 76 before this follow-up), including the six live pwsh cases.
• bash plugins/repo-hygiene/skills/clean/scripts/git-branch-audit.test.sh → OK: git-branch-audit.sh tests passed (29 cases).
• bash plugins/repo-hygiene/skills/clean/scripts/git-stash-audit.test.sh → OK: git-stash-audit.sh tests passed (19 cases).
• bash plugins/repo-hygiene/scripts/allowed-tools-pairing.test.sh → All allowed-tools pairing checks passed.
• bash scripts/check-changelog-parity.sh --check, --check-order, --check-bump origin/main, --check-preserved origin/main → all four exit 0 at 04bbe657.
• bash scripts/check-purged-em-dashes.sh → no em dashes.
• shellcheck -x and shfmt -d over all changed shell files → clean, exit 0.

Related

• Refs repo-hygiene:clean 0.10.11 — audit: destructive guard gaps, PowerShell matcher bypass, silent 200-PR truncation #3346, the parent audit container. Its operating-model contract comment governs this PR's scope and delivery order, including the --limit 0 correction used above.
• Refs repo-hygiene:clean: decide whether the destructive guard should cover branch and remote-branch deletion #3852, the human-gated sibling that owns whether the guard's coverage should grow to git branch -D and git push --delete. Deliberately out of scope here: extending the guard would pre-empt that threat-model decision. (Noted while testing, for that issue: git stash drop; with the ; directly after drop is not matched by the current pattern, which requires whitespace or end of line there; pre-existing, untouched here.)
• Refs repo-hygiene:clean: emit each branch tip and require tip capture before any deletion batch #3853 and repo-hygiene:clean: add a lossy-but-deletable verdict surfaced as its own block before the deletion confirmation #3854, the two recovery children in the same group. They share this plugin's CHANGELOG.md and .claude-plugin/plugin.json; this PR touches only its own entry and bumps to 0.10.34.
• Pre-existing, unrelated to this branch: plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh fails on a clean origin/main export; plugins/desktop-notification/hooks/desktop-notification.test.sh and scripts/check-shell-portability.test.sh fail intermittently for causes not yet identified and should get their own issues.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

---

Generated by Claude Code

[claude]

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

---

Code review complete

• Gather context (PR body, changed files list, skill instructions)
• Run skip-gate checks (PR open, ready for review, not trivial — proceeded)
• Fetch full diff (gh pr diff 3887, origin/main unreachable for git diff in this sandbox — network fetch required approval, so gh pr diff was used instead)
• Review changed files for correctness/maintainability
• Post findings

Findings

No blocking correctness or maintainability issues found.

What I checked closely

• destructive-guard.sh tool dispatch (is_acknowledged, the case "$TOOL" blocks): confirmed the Bash/PowerShell exact-match default-deny dispatch is consistent between the ack check and the block-reason message, and traced the PowerShell ack regex (plugins/repo-hygiene/skills/clean/scripts/destructive-guard.sh#L102-L120) against the bypass payloads in the test file — none of the comment/quoted-string/second-statement/wrong-tool spellings match. The # shellcheck disable=SC2016 placed before the case "$TOOL" in block (not directly above the offending $env: line) is valid ShellCheck usage — a directive before a compound command suppresses the whole block, not just the next physical line — so that isn't a lint gap despite the visual distance.
• clean_pr_map in clean-common.sh (plugins/repo-hygiene/skills/clean/scripts/lib/clean-common.sh#L811-L847): verified the dynamic jq filter built from ${fields//,/,.} actually expands to a syntactically valid [.field1,.field2,...] construction for both 3- and 4-field callers, and that truncation/unavailable/count status lines are mutually exclusive as documented.
• Both audit scripts' new PR_MAP_FILE + trap ... EXIT: no pre-existing EXIT trap in either script that this would silently clobber.
• SKILL.md step-0 root-resolution removal: confirmed resolve-clean-action.sh never consumed the old REPO_ROOT line, so dropping it from step 0 is safe, and confirmed no other doc in the skill assumes step 0 set it.
• allowed-tools-pairing.test.sh's new frontmatter-overstatement checks: traced the awk/grep extraction against the actual SKILL.md frontmatter text to confirm the new assertions actually exercise the corrected comment (they do — it matches none of these six scripts satisfies the "states real scope" check, and the corrected text no longer contains the banned behind the ... destructive guard phrasing).
• Cross-referenced the tool_name: "PowerShell" spelling against other guardrails hooks in the marketplace (plugins/guardrails/hooks/block-dangerous-git.sh and others) — consistent.

One minor documentation wrinkle, not worth a blocking finding: SKILL.md §4's note "(§4 and §6 are the tiers that need one)" (permalink) is followed by a REPO_ROOT=$(...) snippet that only appears once, under §4 — §6 delegates to git-tree-reset.sh, which resolves its own root internally, so there's nothing to add there. Not a functional issue since the script self-resolves either way, just a slightly imprecise cross-reference.

The PR's own verification section (revert-sensitivity checks, mutation testing on the guard, 131/131 local test pass) is author-claimed and I did not independently re-run it — noting that per the review contract rather than restating it as confirmed.

[claude]

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

---

Security review complete

• Load /review:security-review skill instructions
• Run skip gate (open PR, security-relevant surface: destructive-command guard + PreToolUse hook matcher, no prior applicable review)
• Fetch PR diff (gh pr diff) and read changed files
• Hunt for logic / trust-boundary / injection / credential-handling issues in the diff
• Adversarially verify candidate findings
• Post findings or report clean

No security issues found.

This PR is net security-positive: it closes a guard-registration gap (the PreToolUse matcher was Bash-only while the guard's own patterns already covered PowerShell, so PowerShell hosts had no guard at all — SKILL.md#L176), tightens the acknowledgement dispatch in destructive-guard.sh to default-deny by tool_name rather than "anything that isn't PowerShell" (destructive-guard.sh#L102-L120), and replaces silent PR-lookup truncation/failure with explicit PRCount: / PRDataTruncated: / PRDataUnavailable: status lines so a failed gh pr list can no longer be misread as "no PRs, safe to delete" (clean-common.sh#L811-L847).

Specifically checked and found sound:

• Ack regex bypass surface (destructive-guard.sh's is_acknowledged): the PowerShell $env: regex is anchored at the start, requires the literal value 1/'1'/"1" immediately followed by ;, and the tool dispatch (Bash → prefix spelling, PowerShell → $env: spelling, anything else → no ack path) is a proper allowlist, not a denylist. The PR's own 16-payload bypass corpus (comment, quoted string, post-command, pipeline segment, falsy values, wrong tool) and 42-case tool-gating corpus exercise exactly the cases that would matter here; nothing in the diff contradicts that coverage.
• Injection surface in clean_pr_map (clean-common.sh): $fields is always a call-site literal ('headRefName,state,number,headRefOid' / 'headRefName,state,number'), never attacker- or repo-controlled, so the jq-filter string built from it (".[] | [.${fields//,/,.}] | @tsv") has no injection path. $limit (from CLEAN_PR_LIST_LIMIT) is passed quoted to gh pr list --limit, so no command injection; a non-numeric override just degrades to a harmless gh error caught by the existing failure path.
• Predictable temp-file fallback (PR_MAP_FILE="${TMPDIR:-/tmp}/clean-pr-map.$$" when mktemp fails): this is a pre-existing pattern already used elsewhere in this same plugin (clean-batch.sh, clean-common.sh's manifest path), not something this PR introduces — out of scope as a pre-existing issue per the review's high-signal bar.
• Shell-quoting changes (PowerShell stash selector, SKILL.md §4.3): correctness fix for git receiving a mangled argument, not a security boundary; no new injection surface.

No CRITICAL/IMPORTANT/SUGGESTION findings to report.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa1b6d8881

ℹ️ 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".

[github-actions]

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

[github-actions]

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