hooks: make the pre-push gate refuse inside its own budget instead of being cancelled open - #61
Merged
Merged
Conversation
The emitted pre-push gate carried `"timeout": 300` and nothing else. When a project's checks outran it, Claude Code recorded `hook_cancelled` and ran the push anyway: measured on one project, seven pushes completed after their gate was cancelled without ever reaching a verdict, and a warm check there already costs ~179s of the 300s. So the floor was present when the repo was fast and absent when it was slow, which is the wrong way round. The gate now watches its own clock. GATE_BUDGET (280s) is the deadline it enforces on itself, GATE_HOOK_TIMEOUT (300s) is what settings.json allows it, and the gap is what turns a slow check into a named refusal instead of a silent pass. Both numbers are emitted from one place so they cannot drift apart, and the tests assert the ordering. Raising the ceiling to 900 was the alternative and is rejected: it moves the number without removing the fail-open, and it is wrong again the next time the repo grows. The watchdog is bash rather than timeout(1). A missing timeout binary exits 127, and only exit 2 blocks a tool call, so the dependency would itself be a silent fail-open. It signals the check's whole process group: killing `npm run test` alone leaves the real runner holding the hook's stdout, which the new test caught by taking 60s to run. The price is real and stated rather than hidden - a push against a cold cache is now refused instead of allowed, and the remedy is the workflow crews already follow: run the check once by hand, which warms the cache, then push. Nothing is weakened on the way. A check that fails still blocks by name, a check that passes inside the budget still lets the push through, and the uncommitted-tree refusal is untouched.
Review found the first cut narrowed the fail-open instead of closing it. The watchdog only sent TERM and then waited, so the hook returned when the check chose to die rather than when the budget said. Measured against the shipped code with the budget at 3s: a check that traps TERM and cleans up for 8s ran 11s, and a check that ignores TERM never returned at all. In production the slack is 280 to 300, so either shape carries the hook past the harness timeout, which cancels it and lets the push through - the exact failure this is for. The watchdog now escalates to KILL after GATE_GRACE, which also ends a check that is stopped rather than killed. The test that proves it is the one shape none of the others covered: a check that ignores TERM. Without the escalation that test hangs; with it the gate refuses in four seconds. Two smaller things from the same review. The watchdog is now started under set -m and signalled by group, so it stops leaking an orphan sleep per check per push. And the refusal wording no longer claims a check "did not reach a verdict" when the budget was spent before it ever started, or when it landed exactly on the boundary. The invariant the tests assert is now budget + grace < hook timeout, since the kill is what bounds the hook rather than the deadline alone.
The emitted header said an overrun is "read as consent". That is an interpretation, not a measurement: what was measured on one project is hook_cancelled with timedOut true, and seven pushes completing afterwards. The harness does not treat the cancellation as approval - it simply never hears a verdict - and the push proceeds ungated either way. This file is firstmate's starter bundle, so the sentence ships to every project it touches; two documents in one fleet should not describe the same mechanism two ways. Adopt the measured phrasing here and in the test matrix's rationale. Comments only; no behavior change.
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
1 participant
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.
The starter bundle's pre-push gate fails open on timeout.
bin/fm-hooks-install.shemits the hook with"timeout": 300; when the project's checks outrun that, Claude Code cancels the hook, and a cancelled hook does not block - the tool call carries on through the normal permission flow. The harness does not read the cancellation as approval, it simply never hears a verdict, but the push proceeds ungated either way.That is measured, not inferred. On one project seven pushes completed after their gate was cancelled without concluding (
hook_cancelled,timedOut: true,durationMs: 300481). A warm check there is ~179s and a cold one exceeds 300s, so the gate was absent exactly when the repo was slow.This is worse than an absent gate. Every project's
AGENTS.mdtells its crew the quality floor is enforced on push whether or not an agent cooperates, and a cancelled hook produces neither a block nor a warning.The fix: refuse inside the budget
The gate now watches its own clock rather than waiting to be cancelled.
GATE_BUDGET=280,GATE_GRACE=5,GATE_HOOK_TIMEOUT=300, emitted from one place in the installer.budget + grace < timeoutis the no-fail-open invariant, and a test asserts it against the emitted artifacts so the three numbers cannot drift apart.gate_rungives all checks one shared deadline (SECONDSis the whole hook's age, not each check's), and a check that overruns exits 2 with a named reason.timeout(1). Where that binary is absent the hook would exit 127, and only exit 2 blocks a tool call - so the dependency would itself be a silent fail-open.set -mand are signalled by process group. Signalling the check alone is not enough:npm run testis a shell that spawns the real runner, and a surviving grandchild keeps holding the hook's stdout after the hook is done.Raising
timeoutto 900 was considered and rejected: it moves the ceiling without removing it, and keeps fail-open semantics.The price, stated rather than hidden
A push against a cold cache is now refused instead of allowed. The remedy is the workflow crews already follow - run the check once by hand, which warms the cache, then push. That price is named in the header, in the installer's summary output, and in the refusal message the agent actually sees.
How this sits with the
core.hooksPathworkThese are complementary, not competing. This PR fixes the starter bundle that every project inherits.
thecompanyis separately moving its own gate into git viacore.hooksPath(PR kunchenguid#181). A project that has not made that move still gets a gate that refuses rather than one that quietly disappears.Whether the bundle itself should eventually emit the
core.hooksPathshape is an open question, and it belongs tohook-matcher-skips-floor, which is also where the matcher half of the original report lands. Neither is touched here.Tests
tests/fm-hooks-install.test.shgrows from (a)-(f) to (a)-(j):(h) through (j) drive the real emitted hook end to end against a committed fixture project with an
npmshim, with the deadline shrunk to 3s so an overrun is measured in seconds. (i) hangs without the KILL escalation, verified by stripping it from a scratch copy.bin/fm-lint.shclean (ShellCheck 0.11.0). Full suite: 80 passed, exit 0.The behaviour was also exercised by hand against a real emitted hook: budget overrun, TERM-ignoring check, ordinary failure, typecheck failing before tests run, budget exhausted before the last check, both passing, non-push command ignored, no controlling terminal, grandchild survival, and an orphan sweep afterwards.
🤖 Generated with Claude Code