fix(guardrails): classify destructive git subcommands as mutating

[kyle-sexton]

Problem

ps::git_command_is_readonly decides whether a PowerShell command may skip the fail-closed sink
under the readonly-ok scope. Its subcommand alternation omitted destructive forms, so commands
that irrecoverably destroy uncommitted work classified as READ-ONLY.

This violates the library's own stated invariant, from the header comment of
plugins/guardrails/lib/powershell/ps-command.sh:

OVER-BLOCK, NEVER UNDER-BLOCK is the invariant.

On main the alternation is a single line:

[[ "$lc" =~ (^|[^[:alnum:]_.-])(commit|push|reset|rebase|checkout|merge|cherry-pick|revert|stash|am|tag|notes|worktree)([^[:alnum:]_.-]|$) ]] && return 1

clean and restore are absent — and so are a dozen more.

Measured, not asserted

A harness sourcing the library directly and calling ps::git_command_is_readonly on 33 commands,
run against both trees on the same machine:

tree	pass	fail
origin/main	19	14
this branch	33	0

The 14 forms that classify as read-only on main today:

git clean -fdx · git restore . · git restore --staged --worktree . · git switch main ·
git rm -r src · git mv a b · git add . · git pull · git branch -D feature ·
git update-ref refs/heads/main HEAD · git sparse-checkout set src ·
git submodule update --init · git gc --prune=now · git filter-branch --force

All 14 are fixed. All 12 genuinely read-only forms in the harness — status, log, fetch,
rev-list, merge-base, diff, show, describe, ls-files, cat-file, blame, rev-parse
— still classify read-only, so this does not trade a fail-open for a fail-closed regression.

Approach

The set is derived from a stated predicate rather than accumulated by example: a subcommand is
not read-only when it can create, modify, delete or overwrite working-tree or index content;
create, delete, move or rewrite local refs or history; alter the stash, configuration or repository
administrative state; or publish to a remote.

Deliberate omissions (interrogators, artifact producers, create-only plumbing, repo-creating forms)
are justified in the function's own comment rather than left implicit.

Two properties are load-bearing and documented in place:

• Each alternation stays a literal in pattern position. A variable spliced into a [[ =~ ]]
  pattern is the one bash construct whose quote-removal is genuinely version-sensitive, and a
  predicate that silently stops matching here fails OPEN.
• The --excluding token boundary is intentional. --prune, ls-remote, merge-base,
  --no-merges and --tags must keep classifying read-only; the boundary is what prevents a flag
  from matching a subcommand name.

The fetch allowance from #1415 is preserved, with its own residual recorded. The
negative-shape-match residual — a sink-routing construct can still hide a subcommand, e.g.
git ('cle'+'an') — is documented as deferred to an allowlist inversion rather than silently
carried.

Exposure and ordering

No live exposure today. block-no-verify is the only caller passing readonly-ok, and its
own path does not reach these forms in practice.

That is precisely why this lands now: it is the ordering prerequisite for any change that
widens readonly-ok usage. Widening that scope before this fix would open all 14 forms at once.

Related

• guardrails: PowerShell commands with no git commit are blocked fail-closed on {} or () grouping #1415 — introduced the readonly-ok scope this function gates.
• fix(guardrails): PS fail-closed messages, lazy classifier load, sink-shape allow #2667 — its 0.28.25 CHANGELOG entry sits directly below this one; preserved intact by the
  rebase.
• guardrails: block-dangerous-git.sh blocks PowerShell commands that invoke no git command #2592 — the command-position git probe on the first line of this function, unchanged here.
• fix(disk-hygiene): stop flagging PowerShell $null discards as file redirection #2671 — a sibling false-positive fix in disk-hygiene's belt, merged separately.

No linked issue. The defect was found by direct source audit rather than an operator report,
and no open issue covers it; filing one to immediately close it would add tracker noise without
adding information. The full measured evidence is in this PR body.

[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 4m 23s —— View job

---

Code review complete

• Load /review:code-review skill instructions
• Gather context (diff, CLAUDE.md, REVIEW.md, security-review lane check)
• Review ps-command.sh changes for correctness
• Review test coverage in block-no-verify.test.sh
• Review CHANGELOG / plugin.json bump
• Post findings

This lane covers correctness/maintainability; security findings are scoped to /review:security-review since this repo carries a claude-security-review.yml lane — but I'm noting one finding below anyway because it's specifically about whether the new classifier itself is complete, which is this PR's whole premise.

Posted 2 inline comments:

1. git credential-cache / git credential-store aren't caught by the new blocklist (ps-command.sh:411). These are real, directly-dispatched git subcommands (git credential-store store, git credential-cache exit) that write credentials to disk / manage a caching daemon — mutating under this function's own predicate. The --excluding token boundary this PR documents as load-bearing means the listed credential entry does not match either hyphenated form (the character right after credential is -, which fails the trailing boundary). That's exactly the "hyphenated sibling needs its own entry" class the PR already handles for commit-graph/merge-index/merge-one-file/update-server-info — credential-cache/credential-store look like an oversight in an otherwise very deliberate list, not an intentional omission (they're absent from the exclusion rationale in the header comment). I verified this by manually tracing the regex/boundary logic (I could not execute the test suite in this sandbox — see note below), so treat it as a reasoned deduction, not an executed repro.

2. Comment nit (ps-command.sh:406): "split across five tests" — there are six alternation lines (410–415), not five.

Everything else checked out:

• All 60 alternation entries across the six regex lines are in strict alphabetical order as claimed, and I didn't find other missing mutating subcommands beyond the one above (traced through the full standard + plumbing git subcommand surface against the PR's own stated predicate).
• Token-boundary reasoning (--exclusion keeping --prune, ls-remote, merge-base, --tags, --staged read-only) checks out against the regex as written.
• New test cases in block-no-verify.test.sh line up one-for-one with the new blocklist entries (the finding above is exactly the kind of gap the "one case per subcommand" test design is meant to catch, but no test exists for credential-cache/credential-store since they're not in the list yet).
• CHANGELOG and plugin.json version bump are consistent with the diff and with each other.

Not independently verified: I could not execute plugins/guardrails/hooks/block-no-verify.test.sh in this sandboxed environment (bash execution beyond git/gh read commands required approval that wasn't available), so the PR's "33/33 pass" claim is author-reported and unverified by me, not independently confirmed.

[claude]

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

---

Security review complete

Scope: /review:security-review — security-relevant findings only, scoped to files changed in this PR (gh pr diff against origin/main).

• Gather context (PR diff, changed files)
• Read plugins/guardrails/lib/powershell/ps-command.sh diff
• Read plugins/guardrails/hooks/block-no-verify.test.sh diff
• Check skip gate / security-relevant surface — in scope (this PR is a fail-closed security classifier)
• Hunt for vulnerabilities (logic flaws, regex bypass, injection)
• Adversarial validation of candidate finding (independent verifier agent — CONFIRMED)
• Post final findings

This PR's own stated goal is to close every hyphen-boundary gap where a mutating git subcommand slips through ps::git_command_is_readonly because the token-boundary class [^[:alnum:]_.-] deliberately excludes - (so credential won't spuriously match inside credential-store unless credential-store is also listed — the same reasoning the PR applies to checkout/checkout-index, merge/merge-file, update-ref/update-server-info, etc.). One pair in that exact bug class was missed.

IMPORTANT — git credential-store / git credential-cache still classify as read-only

ps-command.sh#L410-L415

The new alternation lists credential (line 411) — which covers git credential reject/approve/fill — but git-credential-store(1) and git-credential-cache(1) are separate git subcommands, not modes of git credential. git credential-store store writes a plaintext username/password to disk (default ~/.git-credentials); git credential-cache spawns a background daemon caching credentials over a Unix socket. Neither credential-store, credential-cache, store, nor cache appears in any of the six alternation lines.

Because the boundary class excludes -, the listed token credential does not match inside credential-store (the character right after credential is -, which fails the trailing-boundary class) — the identical hyphen-sibling gap this PR fixes for a dozen other pairs. ps::git_command_is_readonly therefore returns 0 (read-only) for:

• git credential-store --file ~/.git-credentials store
• git credential-cache exit

The new test coverage doesn't catch this either: block-no-verify.test.sh#L339 is labeled "credential-store write" but actually exercises git credential reject — the plumbing dispatcher, not the real credential-store/credential-cache subcommands.

Exposure today: same caveat the PR itself gives for the whole change — block-no-verify (the only caller of readonly-ok) only inspects commit/push forms, so this doesn't enable a bypass through today's single caller. But it undercuts the PR's own completeness claim (derived from a stated predicate, meant to close this exact bug class) and is squarely the "ordering prerequisite" risk the PR calls out: any future guard that widens readonly-ok usage would inherit this specific gap.

Fix this →

No other security issues found. The rest of the diff (the version bump, CHANGELOG entry, and the large new test block) is consistent, and the regex construction elsewhere (literal-only alternations, boundary-class reasoning, alphabetical grouping) held up under a full alternation-by-alternation trace against git's actual porcelain/plumbing subcommand set.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c632081ae

ℹ️ 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]

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

[github-actions]

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

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files, HEAD SHA eb64a18)
• Load /review:security-review skill instructions
• Follow skill instructions exactly
• Post findings

Skip gate hit — no new review performed. Per the skill's skip gate, this head already has a successful security review that still applies: the only change since the last-reviewed SHA (9c63208, reviewed above) is a one-word comment reword in plugins/guardrails/hooks/block-no-verify.test.sh ("mis-classified" → "wrongly-classified"), with zero functional or security-relevant diff:

- # a mis-classified subcommand is observable. Every case below carries a SINK
+ # a wrongly-classified subcommand is observable. Every case below carries a SINK

No new security-relevant surface was introduced, so this lane has nothing further to add against eb64a18.

For reference, the prior security review (against 9c63208) is still open and unaddressed: git credential-store / git credential-cache still classify as read-only under ps::git_command_is_readonly — see the earlier finding on ps-command.sh#L410-L415. That finding still applies verbatim at the current HEAD since the relevant code is unchanged by this push.

[claude]

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

---

Code review complete

• Load /review:code-review skill instructions
• Gather context (diff since last reviewed head 9c63208 → current head eb64a18)
• Review changes introduced since last review
• Post findings

Skip gate hit — no new review surface.

The only change since this lane's last review (9c63208 → eb64a18) is a one-line comment reword in the test file, to fix a spell-check gate:

- # a mis-classified subcommand is observable. Every case below carries a SINK
+ # a wrongly-classified subcommand is observable. Every case below carries a SINK

block-no-verify.test.sh#L255 @ eb64a18

No functional or logic change — ps-command.sh is untouched by this commit. This is exactly the "trivial/automated, no meaningful review surface" case, so per this lane's skip gate I'm not re-running a full review.

For context, this lane's prior review (on 9c6320 -> 9c63208) already flagged one open, unresolved finding that still applies: git credential-cache / git credential-store aren't caught by the new blocklist in ps::git_command_is_readonly (ps-command.sh:411) — same hyphen-boundary gap class the PR fixes elsewhere (commit-graph, merge-index, update-server-info, etc.), but credential-cache/credential-store weren't added. That finding wasn't addressed by this commit and remains open.

[claude]

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

---

Security review complete

Scope: /review:security-review — security-relevant findings only, for the commits since this lane's last review (eb64a18 → 8f87861499774de911e29d44b453e95cd99b44ec).

• Load /review:security-review skill instructions
• Gather context (diff since last reviewed head eb64a18)
• Determine skip-gate eligibility — new security-relevant surface, so full review performed
• Verify the new commit against the two previously open findings
• Post findings

Not a skip. Commit 8f87861 ("classify credential-cache, credential-store, citool and gui as mutating") changes the mutating-subcommand alternation in ps::git_command_is_readonly itself — that's exactly the security-relevant surface this lane exists to review, so I traced it rather than invoking the skip gate.

Both previously open findings are resolved:

1. git credential-store / git credential-cache (ps-command.sh#L418) — both are now listed as explicit literal alternatives alongside credential. I traced the boundary/alternation logic: because POSIX ERE [[ =~ ]] resolves alternation with leftmost-longest matching, git credential-store store now matches the credential-store alternative (boundary satisfied on both sides), not a truncated credential match that would fail the --excluding boundary. Correct.
2. git citool / git gui (flagged by the Codex review) (ps-command.sh#L417-L418) — citool and gui are now both listed as their own tokens, correctly blocking the GUI commit frontends that inherit the same disabled-hook-manager environment as git commit.

Alphabetical ordering and boundary correctness hold for both new entries in their respective alternation lines, and matching test cases were added in block-no-verify.test.sh#L320-L326 for credential-store/credential-cache, and further down for citool/gui, each asserting exit code 2 (blocked).

Not independently executed: as with the prior review round, I could not run plugins/guardrails/hooks/block-no-verify.test.sh in this sandbox (bash execution beyond simple read-only commands requires approval that isn't available here), so this is a traced/reasoned verification of the regex and test additions, not an executed repro.

No new security issues found in this commit. Both findings from the prior review rounds on this PR are now addressed.

[claude]

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

---

Code review complete

• Gather context (PR diff, prior comments/reviews already posted)
• Load /review:code-review skill instructions
• Review changes introduced since last review (eb64a18 → 8f87861)
• Post findings

Reviewed the delta since this lane's last review (eb64a18 → 8f87861). Unlike the prior two pushes (comment-only rewords that hit the skip gate), this commit is substantive — it's the fix for the two open findings from earlier rounds — so I did a full re-review rather than skipping.

Both previously-flagged gaps are now closed correctly:

1. credential-cache / credential-store — added as their own literal tokens to the second alternation line (ps-command.sh#L418), matching test cases added at block-no-verify.test.sh#L320-323.
2. citool / gui (Codex finding, P1) — added to the first and second alternation lines respectively (ps-command.sh#L417-418), with matching test cases at block-no-verify.test.sh#L325-329.
3. The "five tests" comment nit is also fixed — now correctly reads "six tests" (ps-command.sh#L406).

Verified by trace:

• Alphabetical order holds across both edited alternation lines (citool between cherry-pick/clean; credential-cache/credential-store between credential/cvsexportcommit; gui between gc/http-push).
• New entries are literals in pattern position (no variable splicing), consistent with the load-bearing constraint this PR documents.
• New test commands (git credential-store store, git credential-cache exit, git citool, git gui) exercise the actual subcommands being fixed, not a plumbing stand-in.

No new findings. I did not spot any other gaps in this delta.

Not independently verified: I could not execute block-no-verify.test.sh in this sandbox (bash execution beyond git/gh requires approval that wasn't available), so I did not independently confirm the 4 new test cases pass — this is author-claimed, not verified by me.

One pre-existing, unchanged nit for awareness (not a new finding, no action needed): the test label at block-no-verify.test.sh#L350 still reads "credential-store write" while exercising git credential reject — now that a real credential-store test exists at line 320, that label reads a bit more confusingly than before. Untouched by this commit, so out of scope here, but worth a rename if this PR touches that line again.

[kyle-sexton]

All three review threads addressed in 8f878614.

credential-cache / credential-store — confirmed and fixed. Reproduced against the branch before the change:

READONLY  git credential-cache exit
READONLY  git credential-store store

Exactly the hyphenated-sibling class the comment already called out for commit-graph / merge-index / update-server-info, and I missed adding the two credential relatives to the list I had just written. Both now have dedicated entries and dedicated test cases, and the comment's hyphenated-sibling note names them.

citool / gui — confirmed and fixed. Same reproduction:

READONLY  git citool
READONLY  git gui

git help -a describes citool as the "Graphical alternative to git-commit", so a commit made through either frontend inherits the session environment — including a disabled hook manager — and must not ride the readonly-ok path. Both added with test cases noting that rationale.

"five tests" count — corrected to six.

Post-change verification, all four now mutating with no read-only regression:

mutating  git credential-cache exit
mutating  git credential-store store
mutating  git citool
mutating  git gui
READONLY  git status --short
READONLY  git log --oneline
READONLY  git fetch origin
READONLY  git merge-base main HEAD
READONLY  git rev-list --count HEAD
READONLY  git ls-remote origin
READONLY  git diff --no-merges

The boundary-sensitive cases (ls-remote, merge-base, --no-merges) are in that list deliberately — they are what the --excluding boundary exists to protect, and adding two hyphenated entries could plausibly have disturbed them.