Skip to content

fix(claude): derive the post-edit child budget and keep a hand-raised timeout - #367

Open
L4XB wants to merge 1 commit into
trailhq:mainfrom
L4XB:fix/366-post-edit-budget-and-timeout-carry
Open

L4XB wants to merge 1 commit into
trailhq:mainfrom
L4XB:fix/366-post-edit-budget-and-timeout-carry

Conversation

@L4XB

@L4XB L4XB commented Sep 12, 2026

Copy link
Copy Markdown

Fixes #366. Both halves, since they are the same wiring and the workaround in the issue exists only because neither side alone was enough.

1. The post-edit child now tracks the installed budget

promptAskTimeout has derived its cap from the budget actually installed in the repo since the prompt hook hit exactly this; checkStaleCount had no equivalent and kept the flat CHILD_TIMEOUT_MS. postEditCheckTimeout is the same three lines against PostToolUse.

The sharp case is a repo wired before post-edit's budget was raised: PostToolUse is still 8000 there, the same number as the child, so the child could consume the entire hook budget and leave nothing for readWiring() + formatBlastRadius(). The SIGKILL takes emit() and patchStats() with it — no blast radius for the edit, no stats write, silently. That is the failure promptAskTimeout's own docstring describes, one hook over.

One thing the suggestion in the issue needed to work. hookTimeoutIn returned the first graft timeout it found in an event, not the smallest. For UserPromptSubmit that is the same thing (one entry). PostToolUse declares two — post-edit 10000 and tool-savings 8000 — so a straight installedHookTimeout(dir, 'PostToolUse') read back 10000, derived 8000, and the change would have been a no-op at the current template: still an 8s child inside a 10s budget, still ~2s for everything after it.

So hookTimeoutIn now takes the smallest within a file, which is what installedHookTimeout already does across files and for the reason it documents — nothing in the hook can tell which matcher launched it, guessing high gets the hook killed, guessing low only shortens one graft check. With that, the post-edit child is 6000 under the current template instead of 8000, which is the headroom the issue asks for. promptAskTimeout is unchanged unless a repo declares two UserPromptSubmit graft entries, where it also becomes conservative.

I did not touch HOOK_OVERHEAD_MS. Whether 2s is the right allowance after a graft check on a 568-card graph is a judgement your measurements support and mine do not — happy to follow up if you want a larger allowance for this hook specifically.

2. A hand-raised timeout survives a refresh

Both merge sites go through one mergeHookEntries, which keeps foreign entries untouched, replaces graft's from the template, and carries forward a timeout raised above it:

const previous = name === null ? undefined : raised.get(name);
return previous !== undefined && previous > (hook?.timeout ?? 0)
  ? { ...hook, timeout: previous }
  : hook;

Raise-only in both directions: a template can still lift the floor for every repo, and a deliberate raise is not pulled back down. Still exactly one graft entry per template block, so graft init converges rather than stacking — the property the // drop old Graft entries → idempotent comment was protecting, which turns out not to require discarding the value.

Matched by hook name rather than by the whole command, one step beyond the Math.max sketch in the issue. The command carries the helpers path, which differs between the repo-level and user-level installs, and settings-merge.ts:146 notes that the invocation form has changed before — so keying on the command would silently lose the raise on exactly the version bump that triggers the rewrite. Keying per hook also matters because PostToolUse has two entries and a raise belongs to the one it was made on.

Testing

test/claude-hooks.test.tspostEditCheckTimeout, in the idiom of the promptAskTimeout cases next to it: a repo wired at 80006000; the current template (10000 + 8000) → 6000; both raised to 2000018000; the MIN_CHILD_TIMEOUT_MS floor; unreadable/absent/non-numeric → 6000. Plus one asserting the two hooks read separate events, so neither can cap the other.

test/claude-settings-merge.test.ts — the carry-forward: a raise survives while its sibling entry keeps the template value; the template still raises a repo below it; a raise written by an older graft (different helpers path) is still recognised; '20000'/null/undefined/NaN/Infinity are ignored rather than carried; a foreign hook on the same event keeps its own timeout; merging twice is byte-identical.

node scripts/run-tests.mjs    1229 passed, 0 failed
tsc -p tsconfig.json --noEmit  clean

(LC_ALL=en_US.UTF-8 — on a German locale tool-savings counts a REAL savings line fails on a clean checkout too, because it matches ~100,000 against a toLocaleString that renders ~100.000. Unrelated to this branch; mentioning it in case it is worth pinning the locale in that assertion.)

Negative controls, each isolating one piece: reverting hookTimeoutIn to first-found reddens the derivation case and nothing else; removing the carry-forward reddens 3 of the merge cases.

To be straightforward about one gap: reverting checkStaleCount to the default cap reddens nothing. The derived number is unit-tested, but the wiring of it into that one call is only observable through a real timeout — a stub graft check sleeping several seconds — and I would rather not put a multi-second timing test into a 1229-test suite unless you want it. The change itself is the single argument on hooks.ts:176.

Two halves of the same hook wiring, from issue trailhq#366.

`checkStaleCount`'s `graft check` child kept the flat CHILD_TIMEOUT_MS
while `promptAskTimeout` has derived its cap from the installed budget
since the prompt hook hit the same problem. On a repo wired before
post-edit's budget was raised, PostToolUse is still 8000 - the same
number as the child - so the child could consume the whole hook and
leave nothing for readWiring() and formatBlastRadius(); the SIGKILL
takes emit() and patchStats() with it, so the edit gets no blast
radius and no stats write.

`hookTimeoutIn` returned the first graft timeout it found in an event
rather than the smallest. That made no difference for
UserPromptSubmit, which declares one entry, but PostToolUse declares
two (post-edit and tool-savings) and nothing in the hook can tell
which matcher launched it - the same reason installedHookTimeout
already takes the smallest across files. Taking the smallest within a
file too puts the post-edit child at 6000 under the current template
instead of 8000, which is the headroom the issue is about.

mergeGraftSettings dropped graft's own hook entries whole and rewrote
them from the template, so a timeout raised by hand in
.claude/settings.json could not survive - and reconcileWiring() fires
that rewrite on every version bump, so the raise reverted unattended.
Both merge sites now go through mergeHookEntries, which carries a
raised timeout forward per hook. Raise-only, so a template can still
lift the floor for every repo, and still one graft entry per template
block, so re-running converges. Matched by hook name rather than by
the whole command, since the helpers path differs between the
repo-level and user-level installs and has changed between versions.
@trailhq-graft

trailhq-graft Bot commented Sep 12, 2026

Copy link
Copy Markdown

🌱 graft blast radius

1 area changed → 3 areas can be affected. 5 dependent symbols, depth 2.
Tests: 1 area updated its tests.
Tag: @anirudhkumar-nanonets — 4 of 4 areas · @shhdwi — Hook Settings Merge, Wiring Configuration

flowchart TB
  A0(("Global Hook Installation<br/>2 symbols"))
  A1(("Wiring Configuration<br/>2 symbols"))
  A2(("Initialization<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Global Hook Installation 2 src/hosts/claude-global.ts:L68-L78 upsertGlobalHooks — calls, depth 1 Hook Settings Merge
Wiring Configuration 2 src/cli.ts:L1088-L1173 wireTarget — calls, depth 2 Hook Settings Merge
Initialization 1 src/claude/init.ts:L63-L104 runInit — calls, depth 1 Hook Settings Merge
Who knows this code — 2 people across 4 areas
Area Who knows it
Hook Settings Merge · changed @anirudhkumar-nanonets — 13 commits, last 10d ago · @shhdwi — 12 commits, last 1mo ago
Global Hook Installation · affected @anirudhkumar-nanonets — 1 commit, last 11d ago
Wiring Configuration · affected @anirudhkumar-nanonets — 39 commits, last 2d ago · @shhdwi — 23 commits, last 30d ago
Initialization · affected @anirudhkumar-nanonets — 8 commits, last 11d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 5 dependent symbols, grouped by area

Global Hook Installation — 2 symbols in 1 file

  • src/hosts/claude-global.ts:L68-L78 — upsertGlobalHooks (calls, depth 1)
    73: const { merged } = mergeGraftHooks(existing, helpers);
  • src/hosts/claude-global.ts:L93-L125 — installClaudeGlobal (calls, depth 2)
    109: // the template appends `/graft-hooks.cjs`, so the raw path produces a mixed

Wiring Configuration — 2 symbols in 2 files

  • src/cli.ts:L1088-L1173 — wireTarget (calls, depth 2)
  • src/upkeep-run.ts:L42-L51 — rewriteWiring (calls, depth 2)

Initialization — 1 symbol in 1 file

  • src/claude/init.ts:L63-L104 — runInit (calls, depth 1)
    75: const { merged, warnings } = mergeGraftSettings(existing, { statusline: opts.statusline });
Test signal per changed area — 1 ✓

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Hook Settings Merge — 2 of 8 reached · 2 test files changed here: test/claude-hooks.test.ts, test/claude-settings-merge.test.ts
    • not reached: hookTimeoutIn, checkStaleCount, graftHookName, mergeHookEntries, priorGraftTimeouts, mergeGraftHooks
3 test suites also reference this code

3 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/claude-init.test.ts
  • test/hosts-claude-global.test.ts
  • test/hosts-retract.test.ts

graft blast · origin/main...HEAD · depth 2 · 4 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

post-edit hook: a fixed 8s graft check child inside a 10s budget, and mergeGraftSettings drops a hand-raised timeout on every wiring refresh

1 participant