perf(hooks): reject irrelevant tool calls with builtins before any spawn - #2246
Conversation
Every Bash/PowerShell tool call fires three hook invocations, and all three launched a string of helper processes purely to conclude they had nothing to do. Measured on Windows that is ~5-6s of dead wait per command; measured here it is 22 process spawns (8 + 4 + 10) against a ~2.3ms bare-bash floor. Both scripts already exited early for irrelevant commands. The cost was the preamble before those exits — four jq runs, a grep and a `git rev-parse` in pr-handoff-stop.sh, two jq runs and a grep in push-format-guard.sh — so this moves the cheapest possible discriminator above them, using shell builtins only: - `$(cat)` -> the `read` builtin (one fewer fork+exec; benchmarked equal or faster for payloads from 1 KB to 2 MB). - push-format-guard.sh: a `*git*push*` glob on the raw payload. Deliberately a superset of the `git[[:space:]]+push` regex it stands in for, so it can only let more through, never less. - pr-handoff-stop.sh post: a `*pull*` glob, superset of the case-sensitive `github.com/…/pull/<n>` URL gate that is the only thing post mode acts on. - pr-handoff-stop.sh pre: the marker-existence gate, moved above the parse, with a builtin git-dir resolver (handles linked worktrees' `.git` pointer files). It yields to `git rev-parse` whenever its answer is not certain — a GIT_* override, an unresolvable pointer, a bare repo, or no repository above cwd. Both `\u` arms keep the superset argument airtight against an encoder that unicode-escapes ASCII, at the cost of the slow path for those rare payloads. Result: 22 spawns -> 0 for an ordinary command; all three invocations now sit at the bare-bash startup floor (29.67/15.97/33.94ms -> 3.29/2.73/2.75ms locally). Neither guard is weakened. Verified by mutation-testing the exact case each one exists to catch: an unformatted `git push` from a checkout with no wired .githooks/pre-push is still denied (plain, compound-quoted, and with jq removed), while the same push on a formatted tree is still allowed; a post-PR CronCreate is still denied inside and past the budget, as is CI polling past it, across a main checkout, a linked worktree and a jq-less PATH (45/45). A 3,402-pair differential of old vs new produced byte-identical stdout and exit codes in all three marker states. Gates: tests/pr-handoff-stop.test.ts + session-start-hook + claude-code-settings 145/145; full offline suite 7705 passed with one pre-existing container failure (tests/claude-cloud-profile.test.ts, reproduced on a clean tree); format:check clean. LF endings and 100755 index modes preserved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017eKsTaFcpHvRhSmeH4JraY
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:31 minutes Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8d5e30a696
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Addresses the Codex P2 review finding on PR #2246. Verified before fixing, by differential reproduction rather than taking the report at face value: from a subdirectory with GIT_CEILING_DIRECTORIES excluding the enclosing checkout, `git rev-parse --absolute-git-dir` reports NO repository, so the parent hook put the PR marker in TMPDIR — while the new builtin walk ascended into the excluded checkout and wrote it to that repo's .git instead. OLD marker in TMPDIR: YES marker in excluded repo .git: no NEW marker in TMPDIR: no marker in excluded repo .git: YES That is the "guard silently stops firing" class this change was supposed to be immune to, not a cosmetic path difference: a post/pre pair that straddles the disagreement writes the marker in one place and reads it from another, so the babysit budget is never enforced. The previous commit's claim that the resolver yields on "a GIT_* override" was therefore wrong — it honoured GIT_DIR, GIT_COMMON_DIR and GIT_WORK_TREE but walked straight past discovery controls. Fix: treat every git discovery control as uncertain and fall through to the authoritative `git rev-parse`, adding GIT_CEILING_DIRECTORIES and GIT_DISCOVERY_ACROSS_FILESYSTEM to that bail-out. Old and new now agree: OLD TMPDIR: YES excluded repo .git: no NEW TMPDIR: YES excluded repo .git: no Adds the focused ceiling-excluded-subdirectory test the review asked for. It asserts both halves — post writes to TMPDIR and not into the excluded checkout, and pre reads the same location so CronCreate is still denied. Mutation-checked rather than assumed green: with the two discovery controls removed again it fails on the exact assertion (`expected false, received true`), and passes with them restored. Not claimed as fixed: a walk can still cross a filesystem boundary that git's default discovery would not, because that has no environment signal to read and no builtin way to detect a mount. It needs cwd outside any repository with a foreign repository above it across a mount — unreachable in this repo's runtime, and both modes stay internally consistent there, so the guard still fires. Gates: pr-handoff-stop + session-start-hook + claude-code-settings + guard-push 211/211. Mutation suites re-run unchanged — push-format-guard 6/6, babysit-budget 45/45 across a main checkout, a linked worktree and a jq-less PATH. Differential of pre-change vs post-change script over 3402 invocation pairs in all three marker states: 0 differences. Fast path intact — still 0 spawns for an ordinary command on all three hook invocations. format:changed clean; LF and 100755 kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017eKsTaFcpHvRhSmeH4JraY
…claude/zen-goodall-qenxit
Summary
.claude/hooks/push-format-guard.shand.claude/hooks/pr-handoff-stop.sh, above everyjq,grepandgitinvocation, so an irrelevant tool call costs one bash startup instead of a dozen process spawns.pr-handoff-stop.sh pre,push-format-guard.sh,pr-handoff-stop.sh post). Measured on the Windows workstation on 2026-08-22 that is ~5-6s of dead wait per command; measured here it is 22 process spawns (8 + 4 + 10) against a ~2.3ms bare-bash floor. Both scripts already exited early for irrelevant commands — the cost was the preamble that ran before those exits, not the logic after them.payload="$(cat)"with thereadbuiltin in both scripts. That removes one fork+exec per invocation and is not slower: benchmarked against payloads from 1 KB to 2 MB it wins below 256 KB and draws above.push-format-guard.sh: add a*git*push*glob on the raw payload. It is deliberately a superset of thegit[[:space:]]+pushregex it stands in for — that regex cannot match unless the bytesgitappear before the bytespush, whether the command reaches it jq-decoded, grep-extracted, or as the raw payload — so it can only ever let more through, never less.pr-handoff-stop.shpost mode: add a*pull*glob. Post mode acts only whentool_responsecarries agithub.com/…/pull/<n>URL, and that gate is case-sensitive, so the lowercase bytespullmust be present; JSON escaping/as\/cannot hide them.pr-handoff-stop.shpre mode: move the existing marker-existence gate above the payload parse, and resolve the git directory with a builtin walk that handles a linked worktree's.gitpointer file. The resolver yields togit rev-parse --absolute-git-dirwhenever its answer is not certain — aGIT_*override, an unresolvable pointer, a cwd that is itself a git dir, or no repository above cwd — sogit rev-parseremains the authority everywhere the fast path declines to answer.\uglob arms keep the superset argument airtight against an encoder that unicode-escapes ASCII, at the cost of taking the slow path for those rare payloads.Result for an ordinary command: 22 spawns → 0, and all three invocations now sit at the bare-bash startup floor.
PreToolUse pr-handoff-stop.sh prePreToolUse push-format-guard.shPostToolUse pr-handoff-stop.sh postBare
bash -c 'exit 0'on the same machine is 2.31ms, so the remaining cost is bash startup and nothing else. Spawn count is the portable proxy for the Windows problem: a spawn costs ~1.5ms here and ~150-400ms on Git Bash.Verification
npm run verify:pr-localThe single
testfailure is pre-existing and unrelated to this diff —tests/claude-cloud-profile.test.ts > reports pending rather than false completion when another run already holds a tier's lock, an artefact of this container's own SessionStart provisioner holding a tier lock. Reproduced on a clean tree with these two files stashed:With the diff applied the rest of the suite is green:
Tests 1 failed | 7705 passed | 1 skipped (7707).The three checks
verify:pr-localdid not reach were run directly and pass:Hook contract tests (
tests/pr-handoff-stop.test.ts,tests/session-start-hook.test.ts,tests/claude-code-settings.test.ts), the smallest gate that covers this change:Identical to the pre-change baseline (145/145).
npm run verify:releasenot run: not a release or handoff-confidence claim, and it is provider-backed.npm run eval:retrieval:quality,npm run eval:rag,npm run eval:qualitynot run: no retrieval, ranking, selection, chunking, scoring, or answer-generation surface is touched.classifyPullRequestFilesreturnsragRanking: falsefor both changed paths.npm run check:production-readinessnot run: no clinical workflow, privacy, environment, Supabase, source-governance, or deployment behaviour changed.classifyPullRequestFilesreturnsclinicalRisk: false, operationalRisk: false.npm run check:deployment-readinessnot run: no deployment startup, hosting, or rollout behaviour changed.Mutation testing — proving neither guard was weakened
A speed-up that turns a guard into a check that cannot fail is a regression, not an optimisation, so each guard was tested against the exact case it exists to catch rather than only against the existing suite.
push-format-guard.sh— agit pushfrom a checkout with no wired.githooks/pre-pushand an unformatted tree. All six cases behave identically before and after the change:Case D matters as much as case A: it shows the guard still discriminates rather than having become a blanket allow or a blanket deny.
pr-handoff-stop.sh— a post-PRCronCreate, and CI polling past the babysit budget. 45 assertions across three environments, including a linked worktree where.gitis a file (the riskiest path for the new builtin resolver) and a PATH withjqremoved:Each environment covers: no marker before a PR exists → allow;
postwrites the marker at exactly the git dirprereads;gh pr checksinside the budget → allow;CronCreateinside the budget → deny; past the budgetCronCreate,gh pr checks,gh run watch, a quoted compound hidinggh pr checks,ScheduleWakeupandmcp__github__get_pull_request→ deny;git push,gh pr merge, theCLAUDE_ALLOW_PR_FOLLOW=1prefix and a different session id → allow. The worktree lines above confirm the builtin resolver produced byte-identical output togit rev-parse --absolute-git-dir. The same 45 assertions were run against the pre-change script and also returned45 passed, 0 failed.Differential fuzz. Old script vs new, requiring byte-identical stdout and exit code, over a corpus of commands, tool names, tool responses and malformed payloads, in all three marker states (none / inside budget / past budget):
Hook file contract
tests/session-start-hook.test.tsasserts every hook is100755in the index with no CR bytes. Both preserved — the scripts were rewritten by file replacement, which drops the executable bit on disk, so it was restored and pinned in the index:git diff --summaryis empty (no mode change), andCR=0for all five hook blobs.Risk and rollout
git rev-parse— it declines and falls through whenever its answer is uncertain (anyGIT_DIR/GIT_COMMON_DIR/GIT_WORK_TREEoverride, an unresolvable.gitpointer, a bare repo, or no repository above cwd). The one genuinely new code path is that resolver, and it is exercised directly by the linked-worktree scenario above.git revertthe single commit. The hooks are self-contained scripts with no state, no migration and no consumers; reverting restores the previous behaviour immediately at the next tool call. Both guards fail open by contract, so even a malformed script leaves tool calls exactly as they were rather than blocking work.Notes
\uglob arms look redundant and are deliberate.JSON.stringifynever escapes ASCII letters, so in practice they never fire; they exist so the "strict superset" claim is provable rather than probabilistic. Their only cost is that a payload carrying a unicode escape takes the old slow path.pr-handoff-stop.shpost mode still runs on every Bash/PowerShell call by design (the settings matcher is broad so it can catchgh pr create). The*pull*glob is what makes that cheap; the marker-writing path itself is untouched.premode falls through to the full parse for the rest of that session. That is the intended minority case — the budget is 30 minutes and the enforcement logic is unchanged.Generated by Claude Code