Repository navigation
refactor(guardrails): read hook commands from shfmt's parse tree, remove the hand-written segmenter - #12896
Merged
Conversation
HOOK_BASH_READER=shfmt selects a reader on shfmt's parse tree, with the default reader unchanged. The old segmenter stays until the owner decides how a refusing reader meets the oracle table: see issue 12471. Refs: #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VQb9Q2e8NLMQ1mwcztzP39
…utable Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VQb9Q2e8NLMQ1mwcztzP39
…utable Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VQb9Q2e8NLMQ1mwcztzP39
…ll shfmt in cloud sessions The shfmt command reader (still opt-in through HOOK_BASH_READER=shfmt) now says why it refused: shfmt or jq missing, the 5 s bound, shfmt's own syntax error line, or the reason a cloud session's install failed. It uses a shfmt in ~/.local/bin when PATH has none. - guardrails SessionStart step (cloud sessions only) installs the pinned, hash-checked shfmt v3.14.1 into ~/.local/bin and never blocks the session. - The PowerShell reduction drops a statement-opening `&` call operator or empty `;`, which bash's grammar cannot read; block-unresolved-pr-merge parses PowerShell text with its backtick escapes dropped. - The bash oracle counts a row the reader refuses (exit 2 with its BLOCKED text) as not hidden; any other non-zero exit fails the row. - Each kind of parser call site has a fail-closed test under the shfmt reader, the run-guards forked chain included. - CI installs the pinned shfmt on the Windows jobs that run the guard suites, and in the plugin eval job when it targets guardrails. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]>
… suites red) Save point for issue 12471 step 2: hook::bash_parse_segments reads every command through shfmt, and the hand-written segmenter is removed. Here-doc body commands come after the top-level ones; destructive-guard reads substitution here-doc bodies from the whole command's parse. The suites whose expectations the owner ruled on are updated in the next commits. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…s count The parse timer no longer starts a sleep watcher that the parent killed at a race-dependent point. One subshell starts shfmt, prints its pid, waits and prints its status; the next reads the status with the read -t builtin and kills shfmt only when none comes in time. A parse that answers in time now forks the same 7 processes and execs shfmt and jq on every run, and needs no sleep or timeout on PATH. - Process census in the reader-path suites counts processes, not the Go runtime's threads (CLONE_THREAD clones), and pins exact counts: one parse is 7 creations and 2 execs. - Owner rulings on issue 12471 applied: unclosed here-doc rows and the --config-env substitution rows now expect deny; the fence-echo and escaped-quote-delimiter rows assert the reader's refusal text. - pr-body-linkage-gate's header states that a command the reader cannot parse is denied. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]>
The shape rows in lib/hook-utils.test.sh that asserted the removed segmenter's split now assert what bash runs: each turns its `git reset --hard` into `echo HIT >&2` and checks that the reader hands that command to the callback exactly when bash (extglob off or on) runs it. Rows shfmt cannot parse assert the refusal and its reason, in a child process. Rows whose split shfmt reproduces are kept. Two reader gaps the rows exposed are closed: - A whole-command second reading (extglob off, or the array `#` one) that does not parse now refuses instead of being dropped: bash runs every line before its syntax error, here-doc bodies included. Before, `!(x)#'` followed by `cat <<B` with a `$(git reset --hard)` body was allowed. - The extglob-off and array rewrites apply their edits in offset order, so a nested `!(x)#` no longer garbles the second reading. - A here-doc body the reader would place inside backticks (an empty body after an opener whose line ends inside a backtick) is refused. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]>
kyle-sexton
marked this pull request as ready for review
November 3, 2026 18:01
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aad571a38f
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
claude-security-review needs no new review of Chunk 1 of 2 (37 files)claude-security-review: full review of chunk 1 of 2 (37 files) at critical-findings: 0 No security issues found in this chunk. The command reader (
Guards
New SessionStart row (
Workflows (
Trust controls. No trusted-actor list, token scope, kill switch or merge hold changes in this chunk. Instruction-surface lens. No standing instruction is deleted or loosened. The README and reference text add requirements and document refusals. Test rows only flip from allow to deny or refused. Prior findings. All ten earlier inline findings are marked
None is raised again. Round cap applied (past the cap: yes). Only CRITICAL and IMPORTANT findings would be posted, and this chunk has none. Pre-existing, not counted. For PowerShell, Spend, not a hold. The CI jobs and cloud sessions add a small shfmt download, and each hook call costs one shfmt+jq parse. That uses existing Actions minutes and session time, with no billing change. Chunk 2 of 2 (10 files)claude-security-review: chunk 2 of 2 at critical-findings: 0 I found no security issues in this chunk. What I checked:
Prior findings. Every prior finding on this PR is on Spend. Nothing in this chunk spends credit or bills. The higher fork-budget pins only record what the shfmt parse already costs on each hook call. |
Contributor
|
claude-review needs no new review of Chunk 1 of 2 (37 files)claude-review, chunk 1 of 2 at Round cap applied. What I read. I read every hunk of the 37 files in chunk 1:
What I checked. I read the code only. I had no shell and ran nothing, so the suite counts and timings in the PR body are the author's claims and I did not confirm them.
Closing issue #12471.
Dropped under the cap (advisory, not counted). lib/hook-utils-shfmt.test.sh L96-98 still says a Settled threads. I re-raised none. I found no embedded instructions in the PR context, settled-threads or prior-findings files. Manifests. No Spend (not a finding). The new steps download shfmt in the existing CI jobs and in guardrails eval runs, which adds a few seconds of runner time on runners already in use. Each hook call adds one shfmt and jq parse. Chunk 2 of 2 (10 files)claude-review, chunk 2 of 2: no findings. Round cap applied. What I read. I read every hunk of all 10 files in
None of these files are copies listed in What I checked. I only read code. I had no shell, so I ran nothing. The suite counts and exact fork pins in the PR body and the owner's replies are the author's claims, and I did not confirm them.
Dropped under the cap (advisory, not counted).
Settled threads. I raised none of them again. The claim-gate fail-open thread and the jq-floor thread are both marked Spend (not a finding). On the PR's own measurement, each parsed hook call costs about 4 ms more at p50 and 26 ms at p95. This uses only local compute and creates no charge. |
…ings
- block-root-delete-target reads a word cut where a substitution starts
(`eval $(…)` as `eval $`, `~/$(whoami)` as `~/$`, no word for `<(…)`),
the shape its decoder-sink, eval and root rules were written for, and a
`{ …; }` group makes the pipe feed open. Restores the decoder-sink,
engine-parity-52 and group-pipe denials origin/main gave.
- The joined reading of an unquoted here-doc body with a backslash-newline
is parsed strictly: bash runs that body up to its first error, so a
joined body that does not parse now refuses instead of being dropped.
- An empty here-doc body starts on the line after its own opener (or after
the body before it), not the start of the command or the previous body.
- block-unresolved-pr-merge drops PowerShell's `&` call operator and empty
`;` statements before the bash parse, as the git guards' reduction does.
- cloud-shfmt.sh installs the abort boundary like every registered hook;
the exec-bash pin allows exactly that one SessionStart row.
- shfmt is a required prerequisite with a v3.14.1 floor in guardrails,
source-control and repo-hygiene; curl is declared for the cloud install;
those three fragments are minor bumps.
- The check-plugins job installs the pinned shfmt before the performance
counters, and guardrails-commit-guard-spawns counts the parse's one jq.
- hook-utils-shfmt.test.sh clears GIT_DIR and friends; engine parity 127
and 128 expect the reader's refusal of an unclosed quote.
Refs #12471
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
Author
Same push also fixes the CI failures: the delete-target split suites (decoder sinks, |
…itution - An extglob pattern's text is reparsed for the commands its substitutions run; a pattern holding `$(`, a backtick, `<(` or `>(` now refuses when it does not parse. `echo @($(cmd)|)` and `echo @(;$(cmd))` hid `cmd` from every guard with extglob on. A pattern with no substitution stays soft. - HOOK_BPS_JQ spells the splice quotes as ', so jq really turns a word's LF or CR into an ANSI-C `$'\n'` and its output carries none raw (a jq that writes CRLF would otherwise put a CR into a word). - The comments over the reparse loops say which list is soft and why. - cloud-shfmt.sh leaves before any library loads outside a cloud session (check-killswitch-hoist); the abort-boundary forced-abort run passes CLAUDE_CODE_REMOTE=true so it still reaches the boundary. - The guardrails, source-control and repo-hygiene READMEs name shfmt v3.14.1+ under Requirements. - The ~/.local/bin shfmt row runs a wrapper that keeps the real HOME, so a version manager's shim still resolves. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
Member
Author
|
…ight - An extglob pattern holding `$(`, a backtick, `<(` or `>(` is refused with its own reason, with no reparse: a `#`, blank-then-`#` or newline in the pattern is literal to bash but a comment or separator to a command parse, so `echo @(#$(cmd))` and `echo @(x #$(cmd))` hid `cmd`. A pattern without a substitution keeps the soft parse. - The extglob-off reading is queued only for a `!(` or `f@()` that opens a command. As an argument or a `[[ ]]` pattern (`ls !(*.md)`, `rm -- !(keep.txt)`, `[[ $x == !(y) ]]`) bash with extglob off rejects the line and runs nothing, and these are allowed again. - The guardrails README says an older shfmt is used as found; the repo-hygiene shfmt entry drops the `apt` hint, which installs a build below its own 3.14.1 floor. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
Member
Author
|
kyle-sexton
enabled auto-merge
November 3, 2026 21:15
Nested extglob patterns each take a parse, and an extglob-off reading parses the pattern again, so `echo @(!(x); @(!(x); @(… x …)))` doubled the parse count per level: sixteen levels (153 bytes) ran past 60 s, the Bash row's hook timeout. A budget shared down the nested calls and reset at a top-level call (HOOK_BPS_MAX_PARSES, 64) now refuses past the cap with its own reason, and identical pattern texts are parsed once per call. The sixteen-deep nest and a hundred flat patterns now refuse within a second. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
The reader's jq program calls utf8bytelength, first shipped in jq 1.6; jq 1.5 cannot compile it, so every Bash and PowerShell call would be refused while the prerequisites probe reported jq as fine. The jq entry in guardrails, source-control and repo-hygiene now carries a version check with min 1.6. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…e failure worktree-add-claim-gate.sh runs after the command (PostToolUse) and is documented as fail-open, but a reader refusal (no shfmt or jq, the 5 s bound, a syntax error, the parse budget) printed BLOCKED and exited 2 after the add had run. The library gains HOOK_BPS_FAIL_OPEN, assigned 0 when it loads so the environment cannot set it; the claim gate sets it around its parse, and a refusal then exits 0 quietly. Every PreToolUse caller still refuses. The claim gate is the only reader caller off PreToolUse. source-control's shfmt degrade text now says which gates deny and that the claim gate leaves the tree unclaimed. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…t an associative array On bash 3.2 `local -A` fails, so the pattern dedupe used an indexed array and evaluated each extglob pattern's text as an arithmetic subscript, running a `$(...)` inside it in the hook before the substitution refusal. The refusal now runs first, and the dedupe keeps a newline-delimited list of %q-quoted texts. Checked under bash 3.2.57: the old form runs the substitution, the new one keeps each distinct pattern once and runs nothing. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…their openers
HOOK_HEREDOC_BODIES was sorted by opener offset while segments reach the
callback by statement, so a here-doc opened in a substitution of a statement
that starts earlier, or in a here-doc body (whose statements come after the
top-level ones), took the index a later-called opener's body should have.
block-root-delete-target and destructive-guard number bodies by counting
openers in callback order, so `X=$(true <<A … A) bash <<A` judged only the
inner body and let the shell's script run unchecked. Each opener is now
ranked by its holding statement as the segments are sorted, then by place,
and the list follows that rank. HOOK_HEREDOC_BODIES_SRC keeps the source
order for the scans that walk the command from its start
(block-unresolved-pr-merge's body strip, destructive-guard's substitution
scan), and run-guards replays both.
An extglob pattern holding bash 5.3's `${ cmd; }` or `${| cmd; }` (a blank,
tab, newline or `|` after `${`) is refused like the other substitutions.
Refs #12471
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
block-root-delete-target and repo-hygiene's destructive-guard matched the here-doc a shell reads to a body by counting `<<` openers in callback order. That count has now failed three ways: a here-doc opened in an earlier substitution or in a body, a redirect-only statement (a body no callback counts), and a joined body's or extglob pattern's second reading (an opener with no body in the list). With the counts equal again, `<<A z A` then `bash <<A rm -rf / A` then a joined `cat <<A` body judged only `z`. Whenever any command reads a here-doc as its script, both guards now judge every here-doc body in the command as that script, and the per-index match is gone. No consumer needs callback order any longer, so HOOK_HEREDOC_BODIES goes back to one source-order list (the callback ranking, HOOK_HEREDOC_BODIES_SRC and its run-guards replay are removed), and the contract says a consumer reads the bodies as a set. Refs #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…nd sees
shfmt keeps a `!(…)` at a command's start as one extglob literal, so a
here-doc inside it has no body in the first tree. The extglob-off reading
and the pattern parse saw `bash <<A`, but HOOK_HEREDOC_BODIES is the first
tree's, so the guards got a shell reading a here-doc with no body and
`!(bash <<A` / `rm -rf /` / `A` / `)` ran unchecked.
After each second reading, every body it lists must be one the first tree
holds, or the reader refuses ("a here-doc only a second reading of the
command sees"), or a soft caller returns 1:
- an extglob pattern or a backtick's text: the first tree holds none of its
bodies, so any body refuses;
- a joined here-doc body, read as `: <<D` then the joined text: its first
body is the joined form of a first-tree body; any other refuses;
- the extglob-off and array `#` readings, which insert one character per
rewrite: each body's start and end, moved back past those insertions,
must be a first-tree body's.
Ordinary `!(x)` and `@(a|b)` commands, nested `!(x)#` here-docs and the
harmless here-doc pair are unchanged.
Refs #12471
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
kyle-sexton
disabled auto-merge
November 4, 2026 00:11
…slash shfmt ends an ExtGlob pattern at a `)` whatever its quoting, while bash skips a quoted one, so `echo @(x')'$(cmd)) #'` hid a substitution bash runs: shfmt reported the pattern as `x'` and the rest as a quoted literal. A pattern holding a quote or backslash now refuses. New form rows for the $(...) and backtick forms failed before the change. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
kyle-sexton
enabled auto-merge
November 4, 2026 00:20
kyle-sexton
disabled auto-merge
November 4, 2026 00:22
Contributor
|
The pr-verify verifier failed head What it wrote (165 lines)This pull request leaves the merge queue, and ci-status holds it out while it carries |
github-actions
Bot
added
the
pr-verify-failed
shfmt keeps an extglob pattern as one literal and ends it at a `)` bash
reads inside a quote, `${…}` or `$[…]`, and bash reads the same text as
commands with extglob off. Re-reading patterns and the extglob-off text let
a command through seven ways, the last being `( echo @(${x:-) #$(cmd)})`
with a closing `)` line. The reader no longer reads them: a command whose
tree holds any ExtGlob node is refused ("an extglob pattern, which shfmt
and bash do not read the same way"), soft calls included.
Removed: the extglob pattern reparse, its dedupe, the substitution, quote
and backslash refusal arms, the extglob-off rewrite and its offset mapping.
The array `#` reading, the backtick and joined here-doc readings and the
parse budget stay. Plain `ls !(x)`, `@(a|b)` and `[[ $x == !(foo) ]]` now
refuse, which is accepted fail-closed; the parse-differences page and the
guardrails README say so.
Refs #12471
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…-skip Main now requires every SessionStart row that needs node to run through the node-or-skip stub pair (scripts/node-notice-rows.test.sh, #12993), so a session with no node skips it silently. The cloud-shfmt row launched node in exec form; it now uses the canonical polyglot, and guardrails carries the two stubs as registered shared copies. The exec-bash test pins the new row by its command. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
…nd in the PR body Refs: #12471 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015te8CrNfM6v83JDbLRFDA9
github-actions
Bot
removed
the
pr-verify-failed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #12471
Summary
The guardrails command reader now gets each Bash command's structure from shfmt's parse tree (
shfmt -ln bash --to-json) instead of the hand-written segmenter, which is removed. A command shfmt cannot parse is refused (exit 2, fail closed). Thehook::bash_parse_segmentscallback contract every guard uses is unchanged.Fix
lib/hook-utils.sh(and its synced copies): the reader walks shfmt's JSON for segments, redirects and here-doc bodies, and adds an extglob-off second reading and an array#second reading where bash's own reading depends on them. A second reading that does not parse is refused. shfmt runs under a 5 s bound built onread -t; a timeout, a syntax error, invalid UTF-8 or a missing shfmt each refuse with their own reason.git --config-env=alias.<n>=$(...) <n>(block-dangerous-git,block-noncanonical-commit): main allows it, because the old reader split the word at$(and never saw the alias as the subcommand. In a freshgit initrepository with git 2.53.0,NAME='!touch pwned' bash -c 'git --config-env=alias.rh=$(printf NAME) rh'exits 0 and createspwned, so the alias's shell command runs. The guard now denies when a substitution supplies the alias name.plugins/guardrails/hooks/cloud-shfmt.sh(SessionStart): installs the pinned, hash-checked shfmt in cloud sessions and records why an install failed; the refusal message quotes that reason.windows_amd64digest), and the eval job installs it when the target is guardrails.pr-body-linkage-gate.sh: its header now says a parser refusal also denies.git reset --hardreaches the callback if and only if bash runs it (extglob off and on).--config-envalias and two message-only refusals expect deny.bps_oracle_rowcounts a refused row as not hidden, since it runs nothing.Verification
Oracle: 2895 rows where bash runs the HIT word, 1710 refused, none hidden.
lib/hook-utils.test.sh720/0 andhook-utils-shfmt.test.sh139/0. Every guard suite on the reader's path passes, among themblock-dangerous-git1020/0,run-guards730/0,block-root-delete-target1478/0,block-cross-domain2779/0 andpr-body-linkage-gate183/0.PowerShell differential against origin/main: 0 lost and 0 gained over 851 commands and 11036 token cells. shellcheck: 38 files, 0 findings. Shared-copy sync
--check/--check-bumpandvalidate-plugins.shpass. The timer passes on bash 3.2.57.Latency per hook call: one full
run-guards.shdispatch with the Bash guard list fromhooks.json, interleaved per command, origin/main'splugins/guardrails(fromgit archive) against this branch, over the 1025 of 1139 corpus commands both arms allow. The corpus is the distinct commands harvested from the guardrails suites, NUL-separated. Measured 2026-11-03 on a quiet WSL2 host (32 CPUs, load 2.4 to 1.7), one run. "added" is the p50 and p95 of each command's own difference (this branch minus origin/main, same command), so it is not the difference of the two rows above it.The commands that measured it, run from the worktree root (
timing.shandsummary.pybelow):timing.sh
summary.py
melo-desk-001 Windows Token census: 300 Git Bash spawns changed Token objects by +11 (an equal idle period, -9), so the spawns leak no Token per spawn.
Left out:
read -ton a pipe under Windows Git Bash runs only in this PR's Windows jobs. The timing harness is one-off tooling, so it is quoted above rather than committed.Related
!(x)#; cmd \<newline>...that bash accepts with extglob off #12895: the reader over-refuses!(x)#; cmd \<newline>..., which bash accepts with extglob off. It fails closed and is left to that issue.🤖 Generated with Claude Code