Repository navigation
refactor(guardrails): read hook commands from shfmt's parse tree, remove the hand-written segmenter - #12896
Open
kyle-sexton wants to merge 19 commits into
Open
refactor(guardrails): read hook commands from shfmt's parse tree, remove the hand-written segmenter#12896kyle-sexton wants to merge 19 commits into
kyle-sexton wants to merge 19 commits into
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 partial review of Chunk 1 of 2 (38 files)claude-security-review, chunk 1 of 2 (38 files at I found one new IMPORTANT finding, posted inline on What else I checked (no findings):
Bookkeeping:
critical-findings: 0 Chunk 2 of 2 (6 files)claude-security-review, chunk 2 of 2 (6 files under critical-findings: 0 What I checked
Other notes
|
Contributor
|
claude-review reviewed claude-review: incremental review of Round cap applied. Scope. I read every hunk of the 27 files in the incremental list:
What I checked. I read the code only; I had no shell, so I ran nothing.
Settled threads. None re-raised. The here-doc ordering thread ( Spend (not a finding). None in this push. |
…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
This branch has not been deployed
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 1025 commands both allow. The commands are harvested from the guardrails suites. Measured 2026-11-03 on a quiet WSL2 host.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 and is not 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