perf(guardrails): run the guard chain in-process and answer payload fields without jq - #4185
kyle-sexton wants to merge 7 commits into
Conversation
…ields without jq run-guards.sh sources its guards into its own shell instead of one command-substitution subshell per guard: `exit` is a dispatcher function that records the guard's status and runs the next guard from inside the call, stdout documents are collected through hook::emit_document, and a guard that dies of a hard error hands its status to the abort boundary's new chain slot (_GAB_CONTINUE), which runs the guards still owed in one subshell. Per-invocation analysis state (the alias memo) is reset before each guard, so a later guard's alias walk is not answered by an earlier guard's memo. lib/hook-utils.sh answers a well-formed payload's plain-string fields with its builtin JSON parser (hook::_fast_fields) and runs jq only for a shape it cannot prove; hook::jq_fields_uncached names the same body for the dispatcher's cache, hook::emit_document is the one stdout path, and hook::extract_bash_subject_to is the in-shell telemetry subject. The 17 carrying plugins take the synced copy with a patch bump. Measured on Windows 11 + Git Bash with an exact job-object census of the harness's own invocation: the Bash-lane chain went from 23 process creations to 3 and 880 ms to 285 ms isolated p50; the PowerShell lane from 100 to 80 creations and 3.3 s to 2.7 s. Decisions are byte-identical (rc, stdout, stderr) against 0.33.11 over the perf baseline's 17-command corpus in both tool modes, over 653 commands harvested from the guard suites on the Bash lane and 200 on the PowerShell lane, and over the Write, Edit and drive-root-tmp lanes. Handoff item: 20260913-034031 (program 20260915-153000). Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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. |
|
Claude finished @kyle-sexton's task in 7m 12s —— View job Code review complete
This is a large diff (65 files) but almost all of it is mechanical: Traced the trickiest parts by hand — the Two findings posted inline:
Both are linked with a "Fix this →" Claude Code URL in the inline comments. |
|
Claude finished @kyle-sexton's task in 5m 42s —— View job Security review (
|
|
Last security-reviewed head: |
![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: 952befe110
ℹ️ 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".
| local -A __hu_idx=() | ||
| local -A __hu_want=() |
There was a problem hiding this comment.
On stock macOS Bash 3.2, which remains explicitly supported by plugins such as typos-format (plugins/typos-format/README.md:88-90), local -A is unsupported. _fast_fields is now invoked by hook::jq_fields before its jq fallback, and hooks running with set -u subsequently abort when these undeclared arrays are subscripted; for example, typos-format.sh:485-487 reaches this path during normal classification, so typo formatting no longer completes on that supported runtime. Gate the fast path on Bash 4+ or replace these maps with a Bash-3.2-compatible representation.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # it, or by falling off its end, never reaches this trap: the dispatcher applies | ||
| # guard::_abort_settle to the status itself and merges the notice document with | ||
| # the other guards' output. The trap fires only for a guard that dies of a hard | ||
| # error, when the shell is exiting, and then hands the status to the chain slot |
There was a problem hiding this comment.
Stale/contradictory chain-slot contract. The guidance just above (lines 56–62, unchanged by this PR) says a function plugged into the chain slot must work "under the same handler discipline: builtins only, never exits, never touches the trap." But the chain slot this PR actually wires in, _GAB_CONTINUE=run_guards::guard_died (run-guards.sh:504), does none of that: it calls into run_guards::guard_done → run_guards::record (which itself calls guard::_abort_settle) → and eventually run_guards::run_rest_in_subshell (forks a subshell, sources further guard scripts) → run_guards::finish (spawns jq for the merge, calls builtin exit). It never returns to guard::_abort_on_exit, so the settle/exit lines below the if [[ -n "$_GAB_CONTINUE" ]] block are dead code whenever a dispatcher is chained in.
Functionally this seems fine (traced through it and didn't find a double-settle or a lost trap), but the comment block a future contributor reads to add a second chain-slot consumer states an invariant this PR's own consumer violates by design, with no note reconciling the two. Worth either loosening lines 60–61 to describe run-guards.sh as the documented exception, or explicitly calling out here that the in-process dispatcher case is exempt from the builtins-only/never-exits rule.
| __hu_n=${#_HOOK_JSON_PARTS[@]} | ||
| for ((__hu_i = 1; __hu_i < __hu_n; __hu_i += 2)); do | ||
| __hu_part=${_HOOK_JSON_PARTS[__hu_i]} | ||
| ((${#__hu_part} <= 60)) || continue |
There was a problem hiding this comment.
hook::_fast_fields can silently prove a wrong "absent" instead of falling back to jq for a key name longer than 60 chars. This loop is what builds __hu_idx, the map from a decoded string body to the part index that spells a wanted key/subkey name — but it skips any string body over 60 chars (((${#__hu_part} <= 60)) || continue) before that comparison happens. The filter regex a few lines up (__hu_re="^\\.($__hu_ident)(\\.($__hu_ident))?\$") places no length cap on the identifier it captures into __hu_k1/__hu_k2.
So if a caller ever asks for e.g. .tool_input.a_field_name_that_happens_to_be_longer_than_sixty_characters_total, and the payload genuinely has that key present with a value, its key-string body (>60 chars) never gets indexed, __hu_idx[<name>] stays at the unset sentinel, and the per-filter loop treats it as -2 ("no string in the payload spells the key: absent") — emitting "" and returning 0 (proven) rather than 2 (fall back to jq). That breaks the function's own documented contract ("Proven: HOOK_JQ_FIELDS holds ... exactly what jq prints ... Not proven: run jq") — a present value is reported as absent instead of triggering the jq fallback.
No current caller trips this (PRIME_FILTERS in run-guards.sh and every guard's own filters are short, well under 60 chars), so it's latent rather than live today. But this is a public function synced verbatim into 17 plugins and used directly for hook block/allow decisions, so a future caller passing a longer field name would get a silently wrong answer rather than a safe fallback. Worth either documenting the 60-char ceiling as a hard precondition on filter key length (with a guard that returns 2 when a requested key name exceeds it), or removing the asymmetry so a long-but-present key still falls through to jq.
| # it, or by falling off its end, never reaches this trap: the dispatcher applies | ||
| # guard::_abort_settle to the status itself and merges the notice document with | ||
| # the other guards' output. The trap fires only for a guard that dies of a hard | ||
| # error, when the shell is exiting, and then hands the status to the chain slot |
There was a problem hiding this comment.
Stale/contradictory chain-slot contract. The guidance just above (lines 56–62, unchanged by this PR) says a function plugged into the chain slot must work "under the same handler discipline: builtins only, never exits, never touches the trap." But the chain slot this PR actually wires in, _GAB_CONTINUE=run_guards::guard_died (run-guards.sh:504), does none of that: it calls into run_guards::guard_done → run_guards::record (which itself calls guard::_abort_settle, jq-free but non-builtin control flow) → and eventually run_guards::run_rest_in_subshell (forks a subshell, sources further guard scripts) → run_guards::finish (spawns jq for the merge, calls builtin exit). It never returns to guard::_abort_on_exit, so the settle/exit lines below the if [[ -n "$_GAB_CONTINUE" ]] block are dead code whenever a dispatcher is chained in.
Functionally this seems fine (I traced through it and didn't find a double-settle or a lost trap), but the comment block a future contributor reads to add a second chain-slot consumer states an invariant this PR's own consumer violates by design, with no note reconciling the two. Worth either loosening lines 60–61 to describe run-guards.sh as the documented exception, or explicitly calling out here that the "in-process dispatcher" case is exempt from the builtins-only/never-exits rule.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
hook::extract_bash_subject_to assigns SUBJECT through a nameref, which shellcheck 0.11 cannot follow; CI's lint lane failed SC2154 on the two guards that read it in emit_tel. Declaring the variable empty first is the idiom block-hook-bypass already uses. block-no-verify 256/0 and flag-commit-pr-skill-bypass 35/0 unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CI's machine-specific-paths hygiene check refuses a Windows user path in a fixture. The value is opaque to the parser under test; hook-utils 504/0 unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ty lint The shell-portability lint reads the backslash-w in the previous spelling as a GNU-only regex class. The value is opaque to the parser under test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ts key bound hook::_fast_fields indexes the payload's key strings with an associative array, which Bash added in 4.0, and it ran on every hook::jq_fields call. On the 3.2 shell macOS ships, and which these hooks document support for, `local -A` fails per call. hook::_fast_fields_supported is the predicate, split out the way hook::read_supports_nchars is so a test can force the below-floor branch on a modern host, and hook::jq_fields_uncached asks it before entering the fast path. Below the floor jq answers, unchanged. The index loop also skipped any string body longer than a fixed 60 bytes before decoding it, while nothing capped the key names a caller may ask for. Two wrong answers came out of that: a requested key longer than 60 characters was proven ABSENT while present, and a key of 11 or more characters spelled with \u escapes (hook_event_name is 15, 90 escaped) was missed the same way. The bound is now six times the longest requested key name, the width of `\uXXXX` per identifier character, which is the bound the header comment always described. The suite gains four cases: the below-floor branch forced with the fast path replaced by a tripwire, a present 70-character key, an absent one, and hook_event_name spelled entirely in \u escapes. Each compares the fast path against jq rather than against an expectation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The chain-slot paragraph asks a function plugged into _GAB_CONTINUE to be builtins only, never exit, and never touch the trap, and the slot's own comment says it must not return. run-guards.sh's consumer does none of that: run_guards::guard_died forks a subshell for the guards still owed, spawns jq to merge their documents, and ends at `builtin exit`. Say so. run-guards.sh is the one documented exception, and it is one because it is the dispatcher finishing the run the process owes rather than a hook doing exit-time work. The discipline is unchanged for everyone else, and the slot comment now matches it: a chained function that returns hands control back and the handler settles the status. Comment only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lliding plugins Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
No related issue: handoff-inbox item 20260913-034031 under program item 20260915-153000 (spawn budget per tool call); no GitHub issue was filed for either.
Summary
Every Bash and PowerShell tool call waited on the guardrails
PreToolUsechain, and on Windows Git Bash that chain cost 23 process creations and about 880 ms before the tool's own command started. Almost all of it was the dispatcher's own shape: one command-substitution subshell per guard, a$(declare -f), a< <(printf | jq)process substitution, and a fork per telemetry subject. This change runs the chain inside one shell and answers payload fields without spawning jq, without changing any guard's decision.Fix
plugins/guardrails/hooks/run-guards.shsources each guard into its own shell. A guard'sexitis a dispatcher function that records the status and runs the next guard from inside the call; stdout documents go throughhook::emit_document; a guard that dies of a hard error hands its status to the abort boundary's new chain slot (_GAB_CONTINUEinabort-boundary.sh), which settles that guard's posture and runs the guards still owed in one subshell. Per-invocation analysis state (the alias memo) is reset before each guard.lib/hook-utils.shgainshook::_fast_fields, a builtin JSON field parser that answers a well-formed payload's plain-string fields and falls back to jq only for a shape it cannot prove (NUL escape, duplicate key, non-string value);hook::jq_fields_uncached,hook::emit_document,hook::extract_bash_subject_to,hook::reset_analysis_state. Synced to the 17 carrying plugins'hooks/hook-utils.shwith a patch bump each; guardrails is 0.34.0.block-*.shandflag-commit-pr-skill-bypass.shresolve their telemetry subject in-shell via the_tohelpers;block-windows-drive-tmp.shmasks quoted redirects in-shell (mask_quoted_redirect_ops_to).Design notes: bash 5.3 does not propagate
breakacross function calls, so exit-chaining replaced that; a second fatal error inside an EXIT trap ends bash with no result, so the still-owed guards run in one subshell rather than in the trap. Stdout collection is by contract, not fd capture: a guard that printfs a document directly bypasses the merge (documented in the run-guards.sh header).Verification
Measured on Windows 11 + Git Bash 5.3 with an exact job-object census (
TotalProcesses) of the harness's ownbash -c "<hooks.json command>"invocation, n=5 isolated p50:true)exit 0)The 3 remaining Bash-lane creations are the harness's
bash -c, theenvof the shebang, and bash itself; the chain spawns nothing. The 80 PowerShell-lane creations are all insidelib/powershell/ps-command.shand are the next item (20260914-183000).Decisions byte-identical (rc, stdout, stderr) against 0.33.11 via a two-root differential:
guard_path_differential.json17-command corpus x {Bash, PowerShell}: 34 cases, 0 mismatchesSuites:
lib/hook-utils.test.sh504 pass (tests 22 to 24 added: fast fields vs jq, emit_document, subject_to);plugins/guardrails/hooks/run-guards.test.shadds in-process chain, hard-error, alias-memo regression and no-fork xtrace cases; its 11 failures on this host (stale-path lane and if-row cases) are identical on main and pre-exist this change.scripts/sync-hook-utils.sh --check-bump origin/mainandcheck-changelog-parity.sh --check-bump origin/mainpass. A grep of the chained guards for process-global state escapes (builtin exit,exec,trap,shopt,set -e) finds only each guard'sset -uo pipefail, which the dispatcher already runs under.The wide affected-suite set from
scripts/affected-tests.sh --base origin/main(every plugin carrying hook-utils.sh) is left to CI.Security review (fresh-context reviewer over the diff, both trees runnable): no confirmed bypass. Three residuals it could not close were closed afterwards:
_HOOK_UTR_TARGET_PHYSICALis set and reset around eachunder_temp_rootcall (hook-utils.sh 1393 to 1400), so it never leaks between guards; the jq merge inrun_guards::finishruns under the sameopen 0 2boundary the pre-change dispatcher armed at its line 85; and a 33-payload fuzz ofhook::_fast_fieldsagainst the library's jq arm (escapes,\uand surrogate forms, NUL, duplicate keys at both depths, nested same-name keys, key-in-value, non-string values, CRLF, a 200 KB value, invalid escapes, trailing junk) shows no divergence under eitherLC_ALL=Coren_US.UTF-8; every shape the parser cannot prove falls back to jq.Residual:
hooks.jsonkeeps the shebang exec form.bash "<script>"would drop theenvhop (3 to 2 creations), but the docs say shell-form commands run undersh -con macOS/Linux, where/bin/shis bash 3.2, so the form change is cross-platform unsafe and not applied.Related
🤖 Generated with Claude Code