Skip to content

guardrails: recover the PreToolUse spawn-reduction work from the closed #1385 #1403

Description

@kyle-sexton

Why this exists

PR #1385 tried to fix the PreToolUse fail-open in two halves: raise the timeouts, and make the guards
cheap enough to finish inside one. The timeout half landed separately as #1379. The performance
half did not land, and it is still worth recovering
— closed with #1385 rather than merged, because
a fresh-context verification found the change unsafe in its then-current state.

Raising a timeout narrows the fail-open window; it does not remove it. If the residual load amplifier
is spawn-proportional — the natural reading, given a census of 65 process spawns per Bash tool call
through MSYS fork emulation under Defender — then the 60 s #1379 bought is consumed 2–3× faster than
it would be with the spawn reduction in place, because each guard stays 2–3× more expensive (7 spawns
versus 3 for the git guards, 14 versus 7 for hardcoded-path-check). Same cliff, proportionally less
margin.

What #1385 contained that is worth keeping

  • A hook::jq_fields batched field extraction in lib/hook-utils.sh, replacing per-field jq
    invocations across eight guards, delivered correctly through scripts/sync-hook-utils.sh with all
    13 plugin copies verified identical.
  • A strip_quoted_spans shell-builtin replacement for a printf | sed | sed pipeline in
    flag-commit-pr-skill-bypass.sh.
  • A deferred git rev-parse --is-inside-work-tree probe in hardcoded-path-check.sh.
  • A derivation for the 60 s budget from a measured 437 ms per-spawn unit price, which fix(guardrails): raise PreToolUse blocking-guard timeouts from 10/15s to 60s #1379's number
    does not carry.

Preconditions any revival must meet

These are the verifier's five, restated as acceptance criteria rather than as narrative:

  1. Fix the four contract-test regressions, starting with the two that fail open. Measured on
    Windows against the post-change tree: block-no-verify 111/112 (git commit --no-<backslash newline>verify continuation — expected exit 2, got 0), block-convention-violation 30/31
    (violating PowerShell here-string subject — expected exit 2, got 0), block-hook-bypass 201/203,
    flag-commit-pr-skill-bypass 27/28. All four involve a multi-line command value; every guard whose
    payloads are single-line passed with the same extraction edit.
  2. Attribute those failures before acting on them. The post-change numbers are first-hand; the
    pre-change baseline was fix(guardrails): raise PreToolUse blocking-guard timeouts from 10/15s to 60s #1379's reported figures, not an own measurement. The control arm is a
    three-way differential: A = live worktree at HEAD, B = HEAD through the extraction procedure,
    C = HEAD~1 through the same procedure. B is the control — only if B reproduces A is the
    extraction faithful and C comparable. Cheaper fallback if B diverges: invoke the single failing
    payload directly against both versions of the one guard.
  3. Add regression tests for multi-line payloads — backslash-newline continuations, PowerShell
    here-strings, embedded and escaped quotes. fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #1385 modified eight guards and added zero tests; the
    existing suite caught these four only by luck of prior coverage.
  4. Commit or reproduce the claimed equivalence harness. fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #1385's commit message claimed
    strip_literals was byte-identical on 20,041 inputs including 20,000 fuzzed; no harness, corpus,
    or output was committed anywhere in the diff, and a here-string test in that same guard fails.
  5. Do not re-raise the advisory hooks' timeouts without a justification fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #1385 did not carry.
    flag-commit-pr-skill-bypass and workflow-resilience-check never block, so raising them from
    10 s to 60 s buys up to 50 s of additional worst-case wait for zero enforcement value. fix(guardrails): raise PreToolUse blocking-guard timeouts from 10/15s to 60s #1379
    deliberately declined this.

The gate-design finding underneath all of it

CI has no Windows runner. Every lane in .github/workflows/ is runs-on: ubuntu-24.04, while the
defect being fixed is MSYS fork emulation under Defender — Windows-only — and the guards ship to
Windows users. A green plugin-gate is therefore uninformative about a change to this code on the
platform that produced the defect. This is a finding against the repository's gate design as much as
against any one PR, and it is why "CI is green" could not substitute for behavior-preservation
evidence here. It deserves its own ticket if one does not already exist.

Also unresolved from that PR

plugins/guardrails/scripts/hook_latency_report.py was added by #1385 and never reviewed — 120 lines,
no test, no CI wiring, and two open review findings on it (fan-out counts not filtered by the selected
hook event; percentile indices not computed by nearest rank). If the performance work is revived,
either bring that script with it under review or leave it out.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-humanHuman-in-the-loop required; autonomous sessions must not resolve items carrying this.priority: highSignificant impact, or blocks an imminent release; staff this cycle.work-class: scopedA briefed fix or small feature; blast radius bounded by the brief, tests exist.

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions