feat(guardrails): default-off the two behavioral injectors and narrow the -m gate to real newlines - #2058
Conversation
… the -m gate to real newlines Guardrails slice of #2021's remediation plan (proposed-work lines 1-2) plus the two cosmetic dangling items. Version 0.19.3 -> 0.20.0. - flag-commit-pr-skill-bypass and workflow-resilience-check default OFF: behavioral-class prose injectors, config-disabled per the instruction-economy evidence gate (scripts kept; opt back in via the existing userConfig switch). plugin.json defaults flip to false and each script uses an explicit ${VAR:-false} opt-in test, since hook::check_enabled's unset fallback is "true". - block-noncanonical-commit narrowed: only a -m/--message value that actually contains a newline blocks (the cross-shell mangling hazard). Single-line -m, bare git commit, and repeated single-line -m pass; exemptions and fail-closed refusals unchanged; PowerShell here-string -m still fails closed (uninspectable, multi-line by form). Test suite respelled in both directions. - hooks.json: merged the two identical Bash|PowerShell PreToolUse groups into one (behavior identical per the current hooks reference). - Fixed three stale "Triggered on Bash" headers (wired Bash|PowerShell). Tests: block-noncanonical-commit 181/0, block-hook-bypass 240/0, flag-commit-pr-skill-bypass 29/0, workflow-resilience-check 16/0; shellcheck clean; repo gates (silent-skips, changelog parity, shell portability, manifest keys, markdownlint) pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 finished @kyle-sexton's task in 3m 5s —— View job Security review complete
No security issues found. This PR touches only
No injection surfaces, credential/token handling, or authorization logic were touched by this diff. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 157ae63704
ℹ️ 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".
… events The synchronize event for 3996770 and the reopen event both fired only the pull_request_target workflows; ci/pr-title never started. No file changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brings in 0.19.4 (block-dangerous-git lease-message fix, #2047); the 0.20.0 entry now stacks above 0.19.4 and the version bump reads 0.19.4 -> 0.20.0. Content changes from this branch are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Security review complete
Reviewed only the files this PR touches ( One finding, already surfaced by Codex — independently confirmed, and it's the only thing worth flagging: [SUGGESTION, confidence: high] The message-detection arms only match the fully-spelled I'd keep this at SUGGESTION rather than higher severity: the script's own header is explicit that this is "a friction guard against the accidental anti-pattern, not a sandbox" — there's no trust boundary or attacker-controlled input here, just a self-inflicted shell-mangling foot-gun the guard is trying to nudge a legitimate caller away from. A caller who deliberately reaches for an abbreviated long option to route around a friction guard has bypassed the spirit of the check, not a security control. Worth closing the gap for completeness (and to keep the guard's own "no known bypass beyond the documented residual" posture accurate), but it doesn't need to block merge. Everything else reviewed clean:
|
…eading The merge resolution glued the 0.20.0 section's last bullet to the ## [0.19.4] heading (MD032/MD022). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 47s —— View job Security review complete
Reviewed the full diff between [SUGGESTION, confidence: high] The message-detection arms match only Kept at SUGGESTION, not higher: the header is explicit this is "a friction guard against the accidental anti-pattern, not a sandbox" (block-noncanonical-commit.sh#L72) — there's no trust boundary or attacker-controlled input, just a self-inflicted shell-mangling foot-gun. Worth closing before merge for completeness (and so the "documented residual" claim stays accurate), but not a blocker. Everything else reviewed clean:
|
|
Claude finished @kyle-sexton's task in 3m 24s —— View job Code review complete
Scoped to the 12 files this PR touches (per One finding posted inline (suggestion-level, non-blocking):
What I verified by reading the code (not re-stating the author's own test-run claims, which I did not independently execute — my Bash tool required interactive approval I don't have in this environment, so I could not run the test suites myself):
Nothing else stood out as a correctness, architecture, or maintainability concern in the reviewed diff. |
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…arated form Review follow-ups on the -m narrowing: - git's parse-options accepts any unique long-option prefix, and --message is git commit's only m-initial long option, so --m through --messag all parse as --message (verified on git 2.55: `git commit --dry-run --mess=x` and each shorter prefix parse; --mainline errors). The scan now recognizes every abbreviation in both the =-attached and the separated form, so `git commit --mess="multi<NL>line"` no longer bypasses the newline gate. - New test cases pin the separated `--message <msg>` form and the abbreviated attached/separated forms, multi-line (deny) and single-line (allow) each, plus the shortest `--m=` spelling. Suite: 188 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 50s —— View job Security review complete
No security issues found on the current HEAD (
No injection surfaces, credential/token handling, or authorization logic elsewhere in the diff. This PR contains no |
…ge patterns
The literal one-letter-short option prefix in the two case arms trips
the typos gate ("should be message"). The case patterns keep the
literal behind the config-blessed spellchecker:disable-line pragma; the
three prose mentions are reworded to describe the prefix range instead
of spelling it. No behavior change (spot-checked both directions;
typos --config _typos.toml now clean on the plugin).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 59s —— View job Security review complete
No security issues found on the current HEAD ( This sync added exactly one commit since the last reviewed HEAD ( I diffed
This confirms the fix for the previously-flagged Everything else reviewed and confirmed sound in prior passes on this PR remains unchanged at this HEAD:
No injection surfaces, credential/token handling, or authorization logic touched by this diff. No |
|
Claude finished @kyle-sexton's task in 4m 24s —— View job Code review complete
Reviewed the current HEAD ( One finding posted inline (test-coverage, non-blocking):
What I verified by reading the code (I could not execute the test suites or
Nothing else stood out as a correctness, architecture, or maintainability concern in the reviewed diff. |
…breviation --m was the only abbreviation covered in one direction only; the separated form now has multi-line (deny) and single-line (allow) cases like every other spelling. Suite: 190 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed — the separated |
|
Claude finished @kyle-sexton's task in 1m 3s —— View job Security review complete
No security issues found on the current HEAD ( Only one commit landed since the last reviewed HEAD ( I diffed I re-read the current state of Everything else reviewed and confirmed sound in prior passes on this PR remains unchanged at this HEAD:
No injection surfaces, credential/token handling, or authorization logic touched by this diff. No |
|
Claude finished @kyle-sexton's task in 2m 9s —— View job Code review complete
This sync added one commit since the last code-review pass on this PR ( One finding posted inline (test-coverage, non-blocking — a partial fix of my own prior finding on this PR):
What I verified by reading the code directly (this sandbox denies interactive Bash approval for anything beyond read-only
Nothing else stood out as a correctness, architecture, or maintainability concern. |
The attached shortest abbreviation was pinned only in the blocked direction; it now has the allow control like every sibling pair. Suite: 191 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 37s —— View job Security review complete
No security issues found on the current HEAD ( Only one commit landed since the last reviewed HEAD ( I diffed I re-read Everything else reviewed and confirmed sound in prior passes on this PR remains unchanged at this HEAD:
No injection surfaces, credential/token handling, or authorization logic touched by this diff. No |
|
Fixed in f34aff3 — the attached |
|
Claude finished @kyle-sexton's task in 51s —— View job Code review complete
This sync added exactly one commit since the last code-review pass on this PR ( +run "git commit --m=<shortest abbreviation> single-line (allowed)" \
+ "git commit --m='feat: x'" 0This closes the gap flagged in my prior review pass on this PR (block-noncanonical-commit.test.sh#L67 at I re-read the full abbreviation block at current HEAD (lines 53-73) to confirm the set is now symmetric and non-duplicated: No other file changed in this sync — No new findings. This test-only commit fully addresses the last open item from this lane's review history on this PR; I have no further inline comments to add. Note: I could not execute the test suite myself in this environment (interactive Bash approval unavailable beyond read-only |
|
Claude finished @kyle-sexton's task in 3m 55s —— View job Security review complete
This sync merged [IMPORTANT, confidence: high] The merge silently reverted this PR's own
Meanwhile the
Why this matters for a guardrails plugin specifically: the PR's own fresh-docs research (cited in the CHANGELOG) establishes that Everything else re-verified clean against the current
|
|
Claude finished @kyle-sexton's task in 4m 9s —— View job Code review complete
One finding posted inline, and it's significant: the
The hook scripts themselves are unaffected and correct — both Full detail and a fix link are in the inline comment on Everything else re-confirmed sound (unchanged from the prior code-review pass at
Note: I could not execute the test suites, Branch: |
The origin/main merge resolved plugins/guardrails/.claude-plugin/plugin.json by taking main's copy and re-applying only the version bump — silently reverting this PR's payload: the two injector defaults (back to true) and the three description updates. Restored from the pre-merge commit; caught by fresh-context verification of the PR tree against the changelog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 18s —— View job Security review complete
No security issues found on the current HEAD ( The one prior finding on this PR — the
Everything else re-verified at current HEAD and unaffected by the merge (diffed each hook file against the PR base directly, not assuming a no-op):
No injection surfaces, credential/token handling, or authorization logic touched by this diff. No |
|
Confirmed and fixed in c2176a8 — the merge resolution had taken main's plugin.json and re-applied only the version bump, reverting the two injector defaults and three description updates. plugin.json is restored from the pre-merge commit (both defaults false again; catalog regenerated, which the revert had also stalled). Independently caught by a fresh-context verification pass diffing the PR tree against its changelog claims before merge. |
One conflict, in the guardrails suite's 20-hop dual-spelling traversal case, where both sides edited the same call. They are orthogonal and both are kept: - main (#2058) narrowed the `-m` gate to real newlines, so the case's command had to become a genuinely multi-line `-m` to still expect exit 2. - this branch replaced the wall-clock ceiling with `run_guarded`, which asserts the exit code and keeps `timeout` only as a hang guard. The resolution takes main's command and label with this branch's mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…line number Merging main brought #2058's hook changes in ahead of the cited text, moving "The ceiling counts ANALYSES rather than seconds" from line 489 to 507 — so the citation that carries this file's whole boundedness argument pointed a reader at an unrelated MEMO paragraph instead. Re-pinning the number would re-arm the same trap on the next edit to the hook. The surrounding sentence already names HOOK_ALIAS_WORK_MAX, which is greppable and survives line shifts, so the citation now points at where the cap is set and the coordinate is dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No linked issue
Summary
Implements the guardrails slice of #2021's remediation plan (proposed-work lines 1 and 2), plus the
two cosmetic dangling items the same classification flagged: the two clean behavioral-class context
injectors are config-disabled (default off, scripts kept, opt back in via userConfig),
block-noncanonical-commitis narrowed to the actual-newline-mhazard, the two identicalBash|PowerShellPreToolUse matcher groups are merged into one, and three stale "Triggered on Bash"headers now say Bash and PowerShell. guardrails
0.19.3 → 0.20.0.Fix
1. Config-off the two behavioral injectors (
flag-commit-pr-skill-bypass,workflow-resilience-check). Perdocs/PLUGIN-PHILOSOPHY.md"Instruction economy", ablation isconfig-disable first where a kill switch exists — no deletion. Mechanism: the
plugin.jsonuserConfig defaults flip to
"default": false, and each script replaceshook::check_enabled(whose unset-env fallback is
true) with an explicit opt-in test(
[[ "${CLAUDE_PLUGIN_OPTION_<NAME>_ENABLED:-false}" == "true" ]] || exit 0) — the same shapesession-flow's default-off
observer-armuses — so the OFF default holds even where the harnessdoes not materialize userConfig defaults into the hook environment. Both hooks stay wired; both
test suites now export the switch ON for behavior cases and pin the unset-switch no-op.
2. Narrow
block-noncanonical-committo actual-newline-m. The guard denied everygit committhat was not the-F -stdin form, single-line-m "fix: typo"included. It nowblocks only when a
-m/--messagevalue really contains a newline (separated, attached,--message=, and separated-cluster-amspellings), which is the cross-shell mangling hazard;single-line
-m, baregit commit, and repeated single-line-mparagraphs pass. Exemptions andthe fail-closed structural refusals are unchanged. On PowerShell, a here-string
-mvalue stillblocks (blanked to a placeholder — uninspectable, multi-line by construction of the form) while a
single-line literal
-mpasses. The test suite is respelled in both directions: everyalias/wrapper/traversal fixture that asserted a block now carries a real-newline payload, and new
cases pin the allowed forms. Documented fail-open residual: a message attached to a short-option
cluster (
-am"multi<NL>line").3.
hooks.jsonwiring cleanup. The two structurally separate PreToolUse groups with thebyte-identical
Bash|PowerShellmatcher are merged into one six-hook group — behavior identicalper the current hooks reference (all matching hooks run in parallel; same-matcher groups fire
independently).
4. Stale headers.
block-hook-bypass.sh,block-noncanonical-commit.sh, andflag-commit-pr-skill-bypass.shsaid "Triggered on Bash tool calls" while wiredBash|PowerShell.Fresh-docs mandate (contract surfaces touched:
hooks/hooks.json,plugin.json): verified thissession against https://code.claude.com/docs/en/hooks (hook entry schema,
timeoutin seconds,statusMessage, parallel execution within a group, independent firing of same-matcher groups,PreToolUse exit-code semantics) and https://code.claude.com/docs/en/plugins-reference (userConfig
option fields —
type/title/description/default;defaultis the "value used when the userprovides nothing"; options are exported to hook processes as
CLAUDE_PLUGIN_OPTION_<KEY>), bothfetched 2026-08-08 via the
docs/OFFICIAL-DOCS.mdindex.Verification
plugins/guardrails/hooks/block-noncanonical-commit.test.sh— 181 passed, 0 failedplugins/guardrails/hooks/flag-commit-pr-skill-bypass.test.sh— 29 passed, 0 failedplugins/guardrails/hooks/workflow-resilience-check.test.sh— 16 passed, 0 failedplugins/guardrails/hooks/block-hook-bypass.test.sh(touched header only) — 240 passed, 0 failedshellcheck -xclean on all seven touched shell files;bash -ncleancheck-silent-skips(no silent prerequisite skips),check-hook-userconfig-argv,check-changelog-parity --check/--check-bump origin/main/--check-order,check-shell-portability --paths <touched files>,check-manifest-duplicate-keys,check-contract-clause-coverage— all pass;markdownlint-cli2clean on the three touchedmarkdown files
node scripts/generate-catalog.mjs(docs/CATALOG.md updated, committed) andnode scripts/generate-cheatsheet.mjs(no diff)Related
proposed-work lines; the issue stays open for the remaining lines and the classification-authority
decision)
🤖 Generated with Claude Code