fix(guardrails): defer unparsable PowerShell commits to the guards that can block them (#1858)

[kyle-sexton]

Summary

Stage 2 of #1809's recorded ordering decision, filed as #1858.

block-noncanonical-commit blocked ps::classify_git_command rc 2 — "not faithfully tokenizable, and something git-shaped is in there" — with a message naming a canonical commit shape it never got to read, while block-dangerous-git blocked the same input with a message describing what was actually observed. The rc-2 arm now defers, taking the same exit 0 the sibling content gate block-convention-violation already takes; both nonzero arms collapse into one deferral.

The deferral emits a new telemetry form value, powershell-deferred, so it stays distinguishable from an evaluated allow — rc 1 previously exited with no telemetry record at all, which would have made the change invisible in a fleet.

Also corrects five telemetry schemas (block-noncanonical-commit, block-no-verify, block-dangerous-git, block-hook-bypass, flag-commit-pr-skill-bypass) that described tool as always "Bash" and subject as always the tokenized Bash:<first-token> form. hooks.json registers all five on Bash|PowerShell, each emits the payload's real tool_name, and hook::extract_bash_subject returns the bare tool name for a PowerShell call rather than tokenizing it. Pre-existing drift on the same telemetry surface this change touches; descriptions corrected, no payload change.

Premise challenge — recommend adjudicating before merge

This implements #1858 exactly as decided, but the decision's justification does not survive verification. Full write-up posted on the issue; summary here.

The cited precedent is category-mismatched. On origin/main, four guards call ps::classify_git_command with identical arguments:

Guard	ps rc=2	Class
block-dangerous-git.sh:1072	exit 2	blocking mechanic
block-no-verify.sh:231	exit 2	blocking mechanic
block-noncanonical-commit.sh:797	exit 2	blocking mechanic
block-convention-violation.sh:375	exit 0	content gate

Three blocking guards hold the fail-closed posture; the one that defers is the content gate. #1858 cites that content gate as precedent for changing a blocking guard.

"Coverage cost is provably zero" is false, not merely overclaimed. Each guard carries its own kill switch. block_dangerous_git_enabled=false + block_no_verify_enabled=false + block_noncanonical_commit_enabled=true now allows a git-shaped unparsable PowerShell commit through. Under a default install, and under any configuration retaining either sibling, coverage is unchanged — that narrower claim is what the CHANGELOG states, and the contract test pins the residual at exactly that width so it cannot silently widen.

Deferred alternative, named not built. If adjudication keeps fail-closed, the motivating complaint — a double block whose second message names a commit shape the guard never read — is fixable by message accuracy rather than deletion: give block-noncanonical-commit and block-no-verify an unparsable message describing what was observed, the way block-dangerous-git already does. Zero coverage change.

Test plan

• plugins/guardrails/hooks/block-noncanonical-commit.test.sh — 172 passed, 0 failed. New coverage:
  • the two rc-2 fixtures now assert exit 0 here, relabelled "deferred — classifier rc 2" (not "allowed" — the semantic is deferral);
  • each fixture asserted against both block-dangerous-git and block-no-verify: exit 2 and stderr matching the unparsable reason, so a sibling blocking these for an unrelated reason cannot keep the test green while the coupling breaks;
  • the residual pinned directly — with both sibling kill switches off, all three guards allow the rc-2 commit.
• Sibling suites, all green at HEAD: block-dangerous-git 319/0, block-no-verify 112/0, block-convention-violation 31/0, flag-commit-pr-skill-bypass 28/0. Captured before the schema-only follow-up round, and unaffected by it since no script changed: block-hook-bypass 211/0, secret-pattern-detection 42/0, hardcoded-path-check 72/0.
• Follow-up commit e35deb47 lands the tool description fix in the four sibling schemas: the first commit's search string spelled the JSON value's inner quotes unescaped, so it matched only block-noncanonical-commit while the quote-free subject correction landed in all five. Caught by Codex review on this PR; verified by re-parsing all five schemas.
• shellcheck -x clean on all three touched shell files.
• scripts/check-changelog-parity.sh --check, --check-bump origin/main, --check-order — all pass.
• scripts/check-changed-skills.sh origin/main — no skills touched.
• markdownlint-cli2 clean on the CHANGELOG; all 31 telemetry schemas re-parse as valid JSON.

Related

• Closes guardrails: block-noncanonical-commit exits 0 on classify rc=2 (stage 2 of #1809's ordering) #1858
• marketplace: two unconditional in-place rewriters race on every Write with no defined precedence, nobody sums the always-on per-turn cost, and thirteen CRITICALs need an ordering #1809 — the ordering decision; stage 1 is PR fix(typos-format,markdown-format): single-writer default ends the unconditional rewriter race #1859
• Constraint disposition: guardrails: block-noncanonical-commit exits 0 on classify rc=2 (stage 2 of #1809's ordering) #1858 restates marketplace: two unconditional in-place rewriters race on every Write with no defined precedence, nobody sums the always-on per-turn cost, and thirteen CRITICALs need an ordering #1809's hard constraint that the block-hook-bypass.sh write-bypass-leak closure must not merge before fix(typos-format,markdown-format): single-writer default ends the unconditional rewriter race #1859. That closure is a separate item, not touched here — this PR's whole scope is the rc-2 deferral, so the constraint does not gate it. fix(typos-format,markdown-format): single-writer default ends the unconditional rewriter race #1859 is still open; the write-leak work remains gated on it.
• Sibling-lane overlap: the in-flight wrapper-argv lane (guardrails: hook::git_resolve_index restarts outside env parsing after -S, so env -S '-i git commit -m x' bypasses every git guard (fail-open, shared with main) #1814/guardrails: hook::git_resolve_index does not peel sudo option clusters, so sudo -bD <dir> git … loses the wrapper chdir (fail-open, shared with main) #1811/guardrails: block-convention-violation passes its whole argv to effective_dir, so a wrapper's -C hides a persisted alias (fail-open, shared with main) #1810, branch fix/1814-wrapper-argv-resolver) also edits block-noncanonical-commit.sh and block-convention-violation.sh. Different code path (hook::git_resolve_index rc 2, not ps::classify_git_command rc 2) and non-overlapping hunks. Worth noting: that lane reaches the opposite conclusion on its neighbouring rc-2 surface, writing the doctrine explicitly into block-dangerous-git and block-no-verify — "Each blocking guard holds this posture itself (per-hook kill switches mean none may delegate it to a sibling)." Independent corroboration of the premise challenge above. Based on origin/main only.

[claude]

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

---

Review complete

• Gather context (diff, changed files, PR body/premise-challenge)
• Review block-noncanonical-commit.sh deferral logic
• Review block-convention-violation.sh change
• Review telemetry schema corrections
• Review test coverage (block-noncanonical-commit.test.sh)
• Review CHANGELOG entry
• Post final review

Summary

This is a well-scoped, well-tested change. I verified the mechanics against the actual code (not just the PR description):

• The rc-2 deferral in block-noncanonical-commit.sh correctly collapses both nonzero classifier arms into one exit 0 deferral, mirroring block-convention-violation's existing pattern.
• ps::print_unparsable_block_message isn't orphaned — it's still used by block-no-verify.sh:234.
• hooks.json already registers all five telemetry-touched guards on "Bash|PowerShell" (unchanged by this PR), and hook::extract_bash_subject in lib/hook-utils.sh already returns the bare tool name for non-Bash tools — confirming the schema fixes are pure doc corrections for pre-existing drift, not a behavior change.
• The new test's residual-pinning block correctly uses CLAUDE_PLUGIN_OPTION_BLOCK_DANGEROUS_GIT_ENABLED / ..._BLOCK_NO_VERIFY_ENABLED, which match hook::check_enabled's CLAUDE_PLUGIN_OPTION_<NAME>_ENABLED convention exactly.
• The sibling-block assertion's "cannot be parsed with confidence" substring is present in both ps::print_unparsable_block_message and ps::print_unparsable_git_block_message (the two different messages block-no-verify and block-dangerous-git respectively print), so run_sibling correctly validates both. This is a genuinely good regression guard — it fails if a sibling later stops handling this rc-2 case for the reason the deferral relies on, not just if the exit code changes.
• CHANGELOG entry and the 0.18.4 → 0.19.0 minor version bump both correctly reflect a behavior change (not a pure fix), and the residual exposure is stated plainly rather than glossed over.

No correctness, error-handling, or maintainability issues found in the diff itself.

On the premise challenge in the PR body: the author's own write-up is the substantive part of this PR, and it's compelling — precedent-by-category-mismatch (citing a content gate's deferral posture to justify changing a blocking guard) and the "coverage cost is zero" claim being narrower than stated (true only for default/partial-disable configs, not the "both siblings off" case the tests now pin) are both accurate as I read the code. That's an architectural/product decision (fail-closed-per-guard vs. deferred-to-sibling), not a code-review defect — I'd defer to human adjudication per #1809 rather than block on it here, since the code as written is internally consistent, correctly tested, and honestly documents its own residual. If adjudication favors the "message accuracy, not deletion" alternative the author sketched, that's a follow-up, not a flaw in this PR's execution.

Note: I couldn't run the test suite in this sandbox (bash invocations were blocked by the approval gate) — relying on static verification of the mechanics above plus the PR's stated 172/0 test run.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Read changed shell hooks and JSON schemas
• Analyze for logic flaws, injection surfaces, authz gaps, secrets handling
• Post findings

No security issues found. This PR changes a fail-closed→defer control-flow decision in one guard, plus five telemetry-schema description corrections. Neither introduces a vulnerability beyond what the PR already discloses.

Scope reviewed (diff against the PR's actual parent d5d02a2, since origin/main has since diverged with an unrelated merge and no longer shares a merge-base with this branch):

• block-noncanonical-commit.sh — the rc-2 arm of ps::classify_git_command now exit 0s instead of blocking.
• block-convention-violation.sh — comment-only reword, no logic change.
• 5 telemetry .schema.json files — description text only, no schema/validation change.
• block-noncanonical-commit.test.sh, CHANGELOG.md, plugin.json — tests/docs/version bump.

Notes from verification (not new findings — confirming the PR's own disclosure holds up):

• ps::classify_git_command (plugins/guardrails/lib/powershell/ps-command.sh:331-366, unmodified) only ever returns 0/1/2, so the new ((ps_rc == 0)) || { defer } is exhaustive against the old case 2|1|* — no rc value falls through unhandled or gets treated inconsistently.
• The coverage residual the PR calls out (a git-shaped unparsable PowerShell commit passes when both block-dangerous-git and block-no-verify are individually disabled via their own kill switches while block-noncanonical-commit stays enabled) is real, but it is: explicitly documented in-code, in the CHANGELOG, and in the PR body; pinned by a dedicated test asserting that exact residual width; and requires two independent config changes to reach (not the default posture). This is a disclosed, tested tradeoff — not a hidden vulnerability — and the PR itself flags it for adjudication rather than asserting it's cost-free.
• emit_tel's telemetry payload construction (block-noncanonical-commit.sh:105-112, unmodified by this PR) builds JSON via jq -n --arg, so attacker-influenced COMMAND/SUBJECT content can't inject into the telemetry JSON.
• hook::extract_bash_subject (hook-utils.sh:592-596, unmodified) already returns the bare tool name for non-Bash calls — confirms the corrected schema descriptions describe actual current behavior, not a claim ahead of the code.
• No GitHub Actions workflows, CI config, or permissions are touched by this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d23fcb567a

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

[claude]

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

---

Code review complete

• Gather context (diff, changed files, prior review comments)
• Verify the fix in e35deb4 (tool description correction landing in all five schemas)
• Re-review block-noncanonical-commit.sh deferral logic and test coverage
• Check CHANGELOG/version bump consistency
• Post final review

Summary

This synchronize re-review covers the one new commit since the prior pass: e35deb4, which fixes the gap Codex's review flagged.

Codex's finding is fully resolved. The prior commit's tool-description correction used a search string with unescaped inner quotes, so it matched only block-noncanonical-commit.schema.json's on-disk Always \"Bash\" form and silently skipped the other four schemas, while the adjacent quote-free subject correction landed everywhere. I diffed e35deb4 directly and confirmed all four sibling files now carry the same corrected wording as block-noncanonical-commit:

• block-dangerous-git.schema.json#L12
• block-hook-bypass.schema.json#L12
• block-no-verify.schema.json#L12
• flag-commit-pr-skill-bypass.schema.json#L12 (correctly kept as "advisory", not "guard" — matches this schema's existing terminology)

Each now reads The invoking tool — "Bash" or "PowerShell"; this guard/advisory matches both., consistent with the subject description beside it. No schema documents a Bash-only tool contract next to a PowerShell-aware subject contract anymore — the contradiction Codex identified is gone.

Verified independently, not just re-read the diff:

• All five .schema.json files parse as valid JSON (jq empty, all clean).
• plugin.json's version stayed at 0.19.0 — this is a within-release correction to the same commit's schema fix, not a new behavior change, so no additional version bump was warranted.
• The CHANGELOG's "Fixed" section already described the intended end state ("Five telemetry schemas no longer claim their guard is Bash-only... Descriptions corrected; no payload change") — that description is now actually true across all five files, so no changelog edit was needed for this follow-up.
• No test suite asserts on the schema description strings themselves (only scripts/sync-standards-contract.test.sh touches schema files, for a different concern), so this class of doc/description drift isn't regression-guarded — worth being aware of, but a plain-text description field in a docs artifact doesn't need one; not a blocking gap.

No correctness, error-handling, or maintainability issues found. This closes out the one open item from the previous review pass; nothing else changed in the diff.

Note: as in the previous pass, I couldn't execute bash test-runner invocations in this sandbox (blocked by the approval gate) — verification here is static (JSON parsing, diff inspection, cross-file comparison) rather than running block-noncanonical-commit.test.sh. The PR's stated 172/0 test run is unverified by me directly.

[claude]

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

---

Security review complete

• Gather context (git diff against the PR's parent commit d5d02a2, since origin/main still shares no merge-base with this branch)
• Review new commit e35deb4 (the four sibling schema tool-description fixes)
• Re-check full PR diff for security-relevant surfaces
• Post findings

No security issues found.

What changed since the last review on this PR: one new commit, e35deb4, landing exactly what Codex's review comment flagged: the prior commit's tool-description correction used a search string with unescaped inner quotes, so it matched only block-noncanonical-commit.schema.json and silently missed the other four sibling schemas (block-dangerous-git, block-hook-bypass, block-no-verify, flag-commit-pr-skill-bypass), leaving them documenting a PowerShell subject shape next to a tool contract still claiming Bash-only. e35deb4 is a 4-file, 4-line diff — each file's tool.description changes from Always "Bash" — this guard matches only Bash tool calls. to The invoking tool — "Bash" or "PowerShell"; this guard matches both. Pure JSON string-literal edits, no schema shape, hooks.json registration, or producer code touched.

Full-PR scope re-verified at HEAD (e35deb4), diffed against the PR's actual parent d5d02a2 (origin/main has no merge-base with this branch):

• block-noncanonical-commit.sh:792-813 — the rc-2 classifier arm now defers (exit 0) instead of blocking; unchanged from the prior review. emit_tel (line 105-112) still builds the telemetry payload via jq -n --arg, so attacker-influenced COMMAND/SUBJECT content can't inject into the JSON, and the new "powershell-deferred" form value passed at line 810 is a static string literal — not attacker-controlled.
• block-convention-violation.sh:371-379 — comment-only reword, no logic change.
• block-noncanonical-commit.test.sh — new fixtures and a run_sibling helper asserting the same rc-2 inputs are still blocked by block-dangerous-git/block-no-verify, plus a residual-pinning block using CLAUDE_PLUGIN_OPTION_BLOCK_DANGEROUS_GIT_ENABLED/..._BLOCK_NO_VERIFY_ENABLED. Test-only, no execution-path or trust-boundary change.
• CHANGELOG.md, plugin.json (0.18.4 → 0.19.0) — docs/version only.

No GitHub Actions workflows, CI config, secrets handling, or permissions are touched by this PR. The disclosed coverage residual (a git-shaped unparsable PowerShell commit passes when both block-dangerous-git and block-no-verify are individually disabled while block-noncanonical-commit stays enabled) is unchanged from the prior review: explicitly documented in-code, in the CHANGELOG, and in the PR body, requires two independent config changes to reach, and is now pinned by a dedicated test — a disclosed, tested tradeoff flagged for adjudication, not a hidden vulnerability.

[claude]

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

---

Security review complete

• Diff current HEAD against origin/main and against the previously-reviewed commit
• Confirm whether any PR-relevant content changed since the last security pass
• Post findings

No new content to review. This trigger fired on a Merge branch 'main' into fix/1858-ps-unparsable-commit-defer commit (d5ac4aa), not a new commit on this PR's own work. I confirmed this directly:

• git diff origin/main...HEAD --stat shows exactly the same 10 files, same +106/-28, as the PR's stated changed-files list — unchanged from what was already reviewed at e35deb4.
• git diff e35deb4..HEAD --stat shows 58 files touched, but every one of them belongs to unrelated plugins (claude-config, claude-memory, disk-hygiene, education, markdown-format, plugin-quality, source-control, typos-format) merged in from main — none is a file this PR touches. The guardrails/telemetry files this PR actually changes are byte-identical to the version already reviewed.

Conclusion: my prior security review from this PR (commit e35deb4, 12:15 UTC) still stands — no security issues found, and there is nothing new in this diff to re-examine. That review covered: the block-noncanonical-commit.sh rc-2 deferral (exit 0 on unparsable classifier input, static "powershell-deferred" telemetry form, jq -n --arg-built payload with no injection surface), the comment-only block-convention-violation.sh change, and the five telemetry schema description corrections (doc-only, no schema/validation change). No GitHub Actions workflows, CI config, permissions, or secrets handling are touched by this PR. The disclosed coverage residual (a git-shaped unparsable PowerShell commit passes only when both block-dangerous-git and block-no-verify are independently disabled) remains a documented, tested tradeoff flagged for adjudication — not a hidden vulnerability.

[claude]

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

---

Code review complete

• Gather context (diff, changed files, prior review history)
• Review block-noncanonical-commit.sh deferral logic
• Review block-convention-violation.sh change
• Review telemetry schema corrections (all 5 files)
• Review test coverage (block-noncanonical-commit.test.sh)
• Review CHANGELOG/version bump
• Post final review

Summary

This synchronize trigger fired off the merge commit d5ac4aa ("Merge branch 'main' into fix/1858-ps-unparsable-commit-defer"). I diffed origin/main...HEAD and confirmed the PR's actual content is unchanged from the last full review at e35deb4: the merge only pulled in unrelated upstream work (hardcoded-path-check fix, disk-hygiene/typos-format/markdown-format/plugin-quality/etc. changes on other plugins) — none of which touches this PR's ten changed files. git diff e35deb4..HEAD scoped to the PR's files shows only an unrelated [0.18.5] CHANGELOG section inserted below this PR's own [0.19.0] entry by the merge, with no functional change.

Confirming the mechanics still hold at current HEAD (re-verified directly, not from memory):

• block-noncanonical-commit.sh:807-813 — rc-2 arm still correctly collapses into one exit 0 deferral emitting powershell-deferred, mirroring block-convention-violation.sh:376-381's existing deferral pattern (comment-only reword there, no logic change).
• All five telemetry schemas' tool descriptions are consistent ("Bash" or "PowerShell"; this guard/advisory matches both), and block-noncanonical-commit.schema.json's form description documents the new powershell-deferred value. Codex's earlier finding (four sibling schemas missed by the first commit's unescaped-quote search string) is fully resolved as of e35deb4 — confirmed by direct diff.
• Test coverage in block-noncanonical-commit.test.sh still does the right thing: asserts the deferral (exit 0) and that both block-dangerous-git/block-no-verify still block the same two rc-2 fixtures with the unparsable-reason substring in stderr (run_sibling), and pins the exact residual width with both sibling kill switches disabled. This is a genuinely strong regression guard against the coupling silently breaking.
• CHANGELOG [0.19.0] entry and the 0.18.5 → 0.19.0 version bump are consistent with a behavior change, and the residual is stated plainly rather than glossed over.

No new correctness, error-handling, test-coverage, or maintainability issues found — this pass found nothing beyond what the prior two code-review passes already covered, since no PR-scoped file changed.

On the premise challenge: unchanged from the prior review — this is an architectural/product decision (fail-closed-per-guard vs. deferred-to-sibling) worth human adjudication per #1809, not a code-review defect. The code as written remains internally consistent, correctly tested, and honestly documents its own residual exposure.

Note: as in prior passes, I could not execute the test suite directly in this environment; verification here is via source/diff inspection, not by running block-noncanonical-commit.test.sh.