fix: wait for transcript to stabilize in Stop hook (race with writer) - #308
Conversation
The Stop hook races Claude Code's transcript writer: when the hook reads transcript_path, the current turn's final `text` block is often not yet flushed — only the preceding `thinking` block is visible — so the hook misses the insight and fails to block. Empirically on this machine: 3 of 4 deliberate-violation test firings lost the race. Hook duration is not the determining factor (a 334ms firing still lost); what matters is wall-clock timing of the writer. Fix: poll `wc -l` every 100ms, break when unchanged across two samples, hard-cap at 1s. Typical added latency 100-200ms, worst case 1s. Cheaper than a fixed `sleep 0.3` most of the time and more resilient to slow machines. README "Gotchas" section gets a note about this pattern since any content-inspecting Stop hook needs the same treatment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Summary: Well-diagnosed race fix. The quiescence-polling approach is reasonable, the hard cap protects against pathological cases, the short-circuits (no jq / no transcript / stop_hook_active) remain ahead of the wait loop so graceful-degradation and infinite-loop-prevention paths do not incur any latency, and the README/docstring updates are thorough. All findings below are suggestions.
Verdict: APPROVE - No Critical or Important issues. All findings are suggestions; they are posted in this body (inline posting via gh api failed repeatedly due to sandbox constraints in this review environment).
Suggestion 1 - home/.claude/hooks/enforce-insight-publish.sh:41
The prev_lines=-1 sentinel forces at least one sleep 0.1 even when the transcript is already stable (common case in tests and non-racing turns). You could initialize prev_lines from an initial wc -l read before the loop - you still need two equal samples to declare stability, but the first observation is free. Minor; 100ms is well within any Stop budget.
Suggestion 2 - home/.claude/hooks/enforce-insight-publish.sh:47
Quiescence detection can be fooled by a writer that pauses >=100ms between consecutive flushes (two identical samples declare stability even though more lines are coming). Given the PR empirical data (race window under 100ms on the fast machine tested), this is an acceptable trade-off, but worth calling out as a known limitation in the comment or the hooks README gotcha - otherwise a future reader debugging a slow-writer regression will not have the context to understand why the detector gave up too early.
Suggestion 3 - home/.claude/hooks/enforce-insight-publish.sh (tests)
No automated test covers the new quiescence loop itself - existing fixtures in tests/test-hooks.sh are all static files, so they exercise the stable-on-first-comparison path but not the grew-then-stabilized path that this fix exists for. A background-writer test (append a line to the transcript after ~50ms, then invoke the hook) would verify the loop actually catches late writes and does not race-and-return early. Live validation via deliberate violation is still the gold standard, but a regression test would catch a future refactor that inadvertently removes the wait.
Suggestion 4 - home/.claude/hooks/enforce-insight-publish.sh:40 (comment text)
See PR discussion for empirical data will rot once the PR is forgotten. Consider inlining the key number (e.g., 3/4 firings lost this race on a fast machine; race window observed under 100ms) or pointing at the gotcha paragraph in hooks/README.md, which is in-repo and easier to rediscover than a PR URL.
Automated review by Claude Code
- Initialize prev_lines from an initial wc -l read instead of -1 sentinel. Cuts typical-case latency in half (~100ms, down from ~200ms, because already-stable turns exit on the first comparison). - Comment on the known limitation: a writer pausing >=100ms between flushes can fool the detector. Acceptable given observed sub-100ms race window; widen interval if a slow-writer regression hits. - Replace "See PR discussion" with a pointer to the Gotchas entry in hooks/README.md, which is in-repo and more durable. Per claude-review suggestions on PR #308. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Feedback AddressedImplemented
Skipped
|
There was a problem hiding this comment.
Code Review — Summary: Targeted race fix for the Stop hook. Quiescence polling (wc -l every 100ms, break on two equal samples, 1s hard cap) is a pragmatic workaround for a Claude-Code-platform gap. The hook short-circuits (no jq, no transcript, stop_hook_active) remain ahead of the wait loop so the graceful-degradation and infinite-loop-prevention paths still cost nothing. README docstring (step 3) and the new Gotchas paragraph both accurately describe the fix, and the inline comment cross-references the Gotchas entry rather than a PR URL that would rot. Previously Addressed (Filtered): All three items implemented in the prior review round are in the code: (1) prev_lines initialized from pre-loop wc -l at line 46 - already-stable turns exit on the first comparison (~100ms); (2) Known-limitation comment above the loop at lines 42-45 re: writer pauses >=100ms between flushes; (4) Inline comment now points at the Gotchas entry in hooks/README.md at lines 38-40 instead of the ephemeral PR discussion. Item 3 (background-writer test) was skipped with sound rationale - bash-level tests that simulate races to verify race handling are inherently flaky. Live deliberate-violation remains the gold standard. I agree. Verdict: APPROVE - No Critical or Important issues. Fix is correct, scoped, and well-documented. --- Automated review by Claude Code
…ism in hook-authoring (#309) * Add live-validate gate to /pr-create + fixture-realism nudge Two harness-level recall fixes for lessons memory alone failed to fire: 1. /pr-create: new step 4 gates on `git diff --name-only` matching home/.claude/{hooks,plugins,settings.json}. Surfaces a live-validation prompt before PR creation — for behavioral guards (hook/lint/policy that BLOCKS something), CI fixtures can't prove the guard is wired in and reachable. References the new memory. 2. hook-authoring SKILL.md: "Fixture realism" nudge in the Testing section. Sample real transcript content before writing regex; don't hand-write fixtures from a mental model. Both surfaced by improve-workflow reflection on the three-PR Stop-hook arc (#303 → #307 → #308) where each round shipped with insufficient empirical validation. Skill prose stays tight (~3-5 lines each) to respect the cost-per-invocation rule from PR #306. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address reviewer feedback on PR #309 - pr-create.md: replace prose trigger with deterministic `grep -qE` one-liner, and drop the dead `home/.claude/plugins/` arm (plugins aren't vendored in this repo; they're referenced via enabledPlugins in settings.json). - hook-authoring SKILL.md: fix dangling reference. The backtick case study lives inline in enforce-insight-publish.sh, not in hooks/README.md Gotchas (which covers jq multiline, PATH-stubbing, and the transcript race — not backticks). Point at the script with its inline comment. Verified the new regex via dry-run: matches `home/.claude/hooks/foo.sh` and `home/.claude/settings.json`, correctly does NOT match `home/.claude/skills/` or `home/.claude/commands/`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix backtick imbalance on fixture-realism line PR #309 round-2 review caught that my dangling-reference fix introduced an odd backtick count (9) on hook-authoring/SKILL.md:209 — the inlined regex contained a literal backtick that closed an inline code span prematurely. Drop the inlined regex from the prose entirely; the file pointer itself is the durable reference, and the script's inline comment has the regex + narrative anyway. Backtick count is now even (6, three pairs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
The Stop hook shipped in PR #303 / fixed in #307 still doesn't reliably fire in production because it races Claude Code's transcript writer. At hook-run time, the current turn's final
textblock is often not yet flushed totranscript_path— only the precedingthinkingblock is visible — so the regex matches nothing and the hook declines to block.Empirical evidence
Over 4 deliberate-violation tests this session:
Hook duration is not the determining factor — test 4's 334ms hook still lost. What matters is wall-clock timing of the transcript write relative to hook start, which varies between turns independent of hook speed.
Full diagnostic broadcast to event bus:
gotcha_discoveredevent #4028.Fix
Poll
wc -lon the transcript every 100ms at the start of the analysis phase; break when the line count is unchanged across two samples. Hard-cap at 1s to prevent hangs.sleep 0.3: adaptive; pays latency only when actually racing; resilient on slow machines where the race may be longer.README updates
Follow-up worth considering
This is a platform gap: any Stop hook that inspects the current turn's content hits this race. A cleaner fix would live in Anthropic's Claude Code — pass the current turn's message content in the Stop hook's stdin JSON alongside
transcript_path. Worth reporting upstream.Test plan
make checkpasses (29/29 bootstrap + 88/88 hooks)Live validation of this fix will happen the same way that caught the original bug: restart CC, emit an insight without publishing, observe the hook blocks reliably.
🤖 Generated with Claude Code