hooks: content-aware transcript wait in enforce-insight-publish (race fix + test)#316
Conversation
The Stop hook races Claude Code's transcript writer: the turn's final `text` block (which carries the ★ Insight) is often flushed AFTER the hook fires. The old wait polled line count only, so a writer that hadn't started flushing yet read as "stable & empty" — the hook missed the insight and failed to block (confirmed 3/4 firings in the original incident). Make the wait content-aware: poll until the line count is stable AND the current turn contains an assistant `text` block, with a ~2s hard cap so a turn that legitimately ends without text (pure tool_use) isn't penalized. Adds a regression test that backgrounds the final text-block append ~300ms after the hook starts and asserts it still blocks — fails on the old line-count-only wait, passes now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Summary: Content-aware transcript wait is a well-targeted fix for a documented race, and the new regression test fails cleanly against the old line-count-only logic. The main gap is documentation drift: hooks/README.md still describes the old algorithm (the hooks own comment now points at a stale section).
Findings:
- [Important] home/.claude/hooks/README.md:282 — stale guidance.
- [Suggestion] home/.claude/hooks/enforce-insight-publish.sh:69 — hard cap doubled to ~2s; pure-tool_use turns always pay the full budget.
- [Suggestion] home/.claude/hooks/enforce-insight-publish.sh:79 — full transcript slurp per poll iteration once stable.
- [Suggestion] tests/test-hooks.sh:1431 — no test for tool_use-only turn that ends without a text block.
Verdict: REQUEST_CHANGES — README update needed; suggestions are optional.
Automated review by Claude Code
| # a not-yet-written text block read as "stable & empty" → missed insight. | ||
| # The content gate fixes that. Hard cap ~2s so a turn that legitimately ends | ||
| # without a text block (e.g. pure tool_use) isn't penalized — proceed and | ||
| # enforce on whatever is present. See hooks/README.md Gotchas. |
There was a problem hiding this comment.
[Important] Documentation drift. This comment says See hooks/README.md Gotchas but home/.claude/hooks/README.md:282 still describes the OLD algorithm verbatim: poll wc -l every 100ms, break when unchanged for two samples, hard-cap at 1s. With this PR that is no longer the pattern — it is now stable-for-2-samples PLUS a content gate, with a ~2s cap. Per the project CLAUDE.md (When adding or modifying hooks, update the README), please update the Gotcha entry so it matches the new behavior and rationale (content gate + writer-not-yet-started failure mode + the 2s cap reasoning).
| prev_lines=$(wc -l < "$TRANSCRIPT_PATH" 2>/dev/null || echo 0) | ||
| for _ in 1 2 3 4 5 6 7 8 9 10; do | ||
| stable=0 | ||
| for _ in $(seq 1 20); do |
There was a problem hiding this comment.
[Suggestion] Hard cap is effectively doubled (1s -> 2s) on every Stop firing. For turns that legitimately end with no text block (the pure tool_use case called out in the comment), the loop will ALWAYS run all 20 iterations because the content gate can never trip — adding a worst-case +1s of latency to those Stop hooks vs. the old design. If you wanted to recover that, you could break out once stable has been very high for several consecutive iterations (e.g. stable >= 5 => ~500ms idle) on the grounds that a turn that has been quiescent that long is not racing the writer anymore, regardless of text-block presence. Optional — happy to keep the simpler shape if the latency is acceptable.
| prev_lines=$cur_lines | ||
| fi | ||
| # Only run the (costlier) content check once the file looks settled. | ||
| if [[ "$stable" -ge 2 ]] && turn_has_assistant_text; then |
There was a problem hiding this comment.
[Suggestion] Once stable >= 2, turn_has_assistant_text runs on every subsequent iteration — and each call slurps the entire transcript via jq -s. In the no-text path, that is up to ~18 full-file slurps inside the 2s budget. On long-running sessions where the transcript is large, each slurp can be non-trivial (tens of ms+) and the loops own wall time can drift past the intended 2s cap. Two cheap ways to bound this: (a) gate the content check to run at most every Nth iteration, or (b) once the file looks stable, capture the line count and only re-run the content check when it changes. Not blocking — just flagging the cost shape.
| [[ $exit_code -eq 0 ]] && \ | ||
| [[ "$output" == *"\"decision\": \"block\""* ]] && \ | ||
| [[ "$output" == *"★ Insight"* ]] | ||
| } |
There was a problem hiding this comment.
[Suggestion] Nice regression test — it cleanly fails against the old line-count-only logic. The complementary case worth adding is the pure tool_use turn, no text block ever lands path that the new comment explicitly calls out as acceptable: assert that the hook exits within ~2s (hard cap) and emits NO block decision. That pins down the trade-off the PR is making and guards against a future refactor that accidentally turns the cap into a hang or a false block.
- [Important] hooks/README.md: the "Stop hooks race the transcript writer" Gotcha still described the old line-count-only algorithm. Updated to the content-gate behavior + the 500ms quiescence early-out + 2s cap rationale (CLAUDE.md requires README updates when hooks change). - [Suggestion] avoid the doubled worst-case latency: a textless (pure tool_use) turn can never trip the content gate, so break once the file has been quiescent ~500ms (5 stable samples). Ordered `stable -ge 5` first so it short-circuits the jq content check (which now runs only on stable samples 2–4, bounding the per-poll slurp cost). - [Suggestion] add a regression test for the pure tool_use turn: asserts no block decision and bounded exit (no hang / no false block). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Feedback AddressedImplemented
All 90 hook tests pass; shellcheck clean. |
|
Automated review by Claude Code. Summary: Content-aware transcript wait correctly fixes the writer race. All four items from the prior review round are addressed: README updated to match the new algorithm, pure-tool_use turns early-out at ~500ms (not the full 2s cap), jq runs at most three times (stable samples 2-4), and a new regression test pins the textless-turn contract. Previously Addressed (Filtered):
Findings: [Suggestion] home/.claude/hooks/enforce-insight-publish.sh — the is_real_user definition and $start computation (finding the index of the last real user message) are duplicated verbatim between turn_has_assistant_text (lines 37-49) and the main ANALYSIS block (lines 106-115). If the rule for what counts as a real user event ever changes (e.g., another non-user event type to filter), both copies must stay in lockstep or the wait gate and the enforcement check will drift apart. Not blocking — sharing jq snippets between a shell function and an inline filter is genuinely awkward — but worth a cross-reference comment so a future editor knows to update both. Verdict: APPROVE — code, docs, and tests are consistent; only the minor Suggestion above. (Note: inline comment posting via gh api was blocked by sandbox restrictions on file creation; finding posted as a PR comment instead, per the documented fallback path.) |
… and settings From a 37-agent adversarially-verified staleness audit (31 confirmed findings) plus the two suggestions deferred from #324: CI / review wiring: - claude-code-review.yml: --model opus → fable (most capable GA model; ~2x Opus pricing, bounded per-PR run; fall back to opus if Fable's cyber classifiers ever refuse a security-heavy diff); add concurrency group with cancel-in-progress so rapid pushes stop billing overlapping full-model reviews of stale commits - claude.yml: pin --model fable (was silently tracking the CC default, which changed to Sonnet 5 under our feet in v2.1.197) - claude-review.md prompt: the mandated submission flow (cat > file, sed -i, VAR=$(...)) prefix-matched none of the allowed tools, so every review had to improvise — rewritten as bare gh pr view + single gh api --input - heredoc with literal SHA; add behavioral-guard live-validation bullet (repo-agnostic) - dotfiles.yml: apt-get update before zsh install; drop redundant shellcheck/jq installs (preinstalled on ubuntu-latest) Models: - agents: audit-* (5), rfc-* (2), improve-workflow → model: fable; status-report stays haiku, summarize-work stays sonnet - settings.json: opus[1m] → fable (1M context is Fable's default; the [1m] suffix is auto-stripped). fastMode kept — inert on Fable, active again if the model is ever flipped back to opus - summarize skill: claude-sonnet-4-6 → claude-sonnet-5 (5 refs); mcp-builder eval script/docs: claude-3-x → claude-sonnet-5 Stale tool references: - work.md: mcp__github__get_issue → issue_read(method: "get") - pr-review.md: get_pull_request_comments/reviews → pull_request_read(method: ...); Task( → Agent( spawn examples - settings.json: drop code-simplifier plugin (redundant with built-in /simplify since v2.1.154) - bootstrap.sh: GitHub MCP via deprecated npx @modelcontextprotocol/server-github → official remote HTTP server with ${GITHUB_TOKEN} header expanded by CC at request time Docs/skills sync: - hook-authoring skill: trigger list, lifecycle, input-field table, and example refs now cover all 10 registered hooks (was 5), pointing at hooks/README.md as source of truth - hooks/README.md: enforce-insight step 3 matches the content-aware wait actually implemented (#316) - catppuccin skill: correct yazi (flavor package via ya pkg) and bat (vendor submodule) rows; rust-async skill: native async fn in traits (Rust 1.75+) as the default, async-trait for dyn/Send cases Deferred #324 suggestions: - notification.sh: permission padlock matches raw input, not the truncated display message (+ regression test) - test-hooks.sh: document the mock-jq byte-vs-codepoint truncation coverage boundary; fix stop_hook_active test running under mock jq (passed vacuously — now uses real jq like its siblings) Skipped pending live-payload verification on a real machine: statusline stdin repo/PR fields + used_percentage, SessionStart sessionTitle. Validated: make check green (101 hook + 35 bootstrap tests), new hook behavior re-exercised against real jq with a stubbed zellij. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrBLzQixo3zx8wSHVSGUXS
… and settings From a 37-agent adversarially-verified staleness audit (31 confirmed findings) plus the two suggestions deferred from #324: CI / review wiring: - claude-code-review.yml: --model opus → fable (most capable GA model; ~2x Opus pricing, bounded per-PR run; fall back to opus if Fable's cyber classifiers ever refuse a security-heavy diff); add concurrency group with cancel-in-progress so rapid pushes stop billing overlapping full-model reviews of stale commits - claude.yml: pin --model fable (was silently tracking the CC default, which changed to Sonnet 5 under our feet in v2.1.197) - claude-review.md prompt: the mandated submission flow (cat > file, sed -i, VAR=$(...)) prefix-matched none of the allowed tools, so every review had to improvise — rewritten as bare gh pr view + single gh api --input - heredoc with literal SHA; add behavioral-guard live-validation bullet (repo-agnostic) - dotfiles.yml: apt-get update before zsh install; drop redundant shellcheck/jq installs (preinstalled on ubuntu-latest) Models: - agents: audit-* (5), rfc-* (2), improve-workflow → model: fable; status-report stays haiku, summarize-work stays sonnet - settings.json: opus[1m] → fable (1M context is Fable's default; the [1m] suffix is auto-stripped). fastMode kept — inert on Fable, active again if the model is ever flipped back to opus - summarize skill: claude-sonnet-4-6 → claude-sonnet-5 (5 refs); mcp-builder eval script/docs: claude-3-x → claude-sonnet-5 Stale tool references: - work.md: mcp__github__get_issue → issue_read(method: "get") - pr-review.md: get_pull_request_comments/reviews → pull_request_read(method: ...); Task( → Agent( spawn examples - settings.json: drop code-simplifier plugin (redundant with built-in /simplify since v2.1.154) - bootstrap.sh: GitHub MCP via deprecated npx @modelcontextprotocol/server-github → official remote HTTP server with ${GITHUB_TOKEN} header expanded by CC at request time Docs/skills sync: - hook-authoring skill: trigger list, lifecycle, input-field table, and example refs now cover all 10 registered hooks (was 5), pointing at hooks/README.md as source of truth - hooks/README.md: enforce-insight step 3 matches the content-aware wait actually implemented (#316) - catppuccin skill: correct yazi (flavor package via ya pkg) and bat (vendor submodule) rows; rust-async skill: native async fn in traits (Rust 1.75+) as the default, async-trait for dyn/Send cases Deferred #324 suggestions: - notification.sh: permission padlock matches raw input, not the truncated display message (+ regression test) - test-hooks.sh: document the mock-jq byte-vs-codepoint truncation coverage boundary; fix stop_hook_active test running under mock jq (passed vacuously — now uses real jq like its siblings) Skipped pending live-payload verification on a real machine: statusline stdin repo/PR fields + used_percentage, SessionStart sessionTitle. Validated: make check green (101 hook + 35 bootstrap tests), new hook behavior re-exercised against real jq with a stubbed zellij. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrBLzQixo3zx8wSHVSGUXS
… and settings From a 37-agent adversarially-verified staleness audit (31 confirmed findings) plus the two suggestions deferred from #324: CI / review wiring: - claude-code-review.yml: --model opus → fable (most capable GA model; ~2x Opus pricing, bounded per-PR run; fall back to opus if Fable's cyber classifiers ever refuse a security-heavy diff); add concurrency group with cancel-in-progress so rapid pushes stop billing overlapping full-model reviews of stale commits - claude.yml: pin --model fable (was silently tracking the CC default, which changed to Sonnet 5 under our feet in v2.1.197) - claude-review.md prompt: the mandated submission flow (cat > file, sed -i, VAR=$(...)) prefix-matched none of the allowed tools, so every review had to improvise — rewritten as bare gh pr view + single gh api --input - heredoc with literal SHA; add behavioral-guard live-validation bullet (repo-agnostic) - dotfiles.yml: apt-get update before zsh install; drop redundant shellcheck/jq installs (preinstalled on ubuntu-latest) Models: - agents: audit-* (5), rfc-* (2), improve-workflow → model: fable; status-report stays haiku, summarize-work stays sonnet - settings.json: opus[1m] → fable (1M context is Fable's default; the [1m] suffix is auto-stripped). fastMode kept — inert on Fable, active again if the model is ever flipped back to opus - summarize skill: claude-sonnet-4-6 → claude-sonnet-5 (5 refs); mcp-builder eval script/docs: claude-3-x → claude-sonnet-5 Stale tool references: - work.md: mcp__github__get_issue → issue_read(method: "get") - pr-review.md: get_pull_request_comments/reviews → pull_request_read(method: ...); Task( → Agent( spawn examples - settings.json: drop code-simplifier plugin (redundant with built-in /simplify since v2.1.154) - bootstrap.sh: GitHub MCP via deprecated npx @modelcontextprotocol/server-github → official remote HTTP server with ${GITHUB_TOKEN} header expanded by CC at request time Docs/skills sync: - hook-authoring skill: trigger list, lifecycle, input-field table, and example refs now cover all 10 registered hooks (was 5), pointing at hooks/README.md as source of truth - hooks/README.md: enforce-insight step 3 matches the content-aware wait actually implemented (#316) - catppuccin skill: correct yazi (flavor package via ya pkg) and bat (vendor submodule) rows; rust-async skill: native async fn in traits (Rust 1.75+) as the default, async-trait for dyn/Send cases Deferred #324 suggestions: - notification.sh: permission padlock matches raw input, not the truncated display message (+ regression test) - test-hooks.sh: document the mock-jq byte-vs-codepoint truncation coverage boundary; fix stop_hook_active test running under mock jq (passed vacuously — now uses real jq like its siblings) Skipped pending live-payload verification on a real machine: statusline stdin repo/PR fields + used_percentage, SessionStart sessionTitle. Validated: make check green (101 hook + 35 bootstrap tests), new hook behavior re-exercised against real jq with a stubbed zellij. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrBLzQixo3zx8wSHVSGUXS
… and settings (#325) * Modernize CC setup for the Claude 5 era: Fable in CI reviews, agents, and settings From a 37-agent adversarially-verified staleness audit (31 confirmed findings) plus the two suggestions deferred from #324: CI / review wiring: - claude-code-review.yml: --model opus → fable (most capable GA model; ~2x Opus pricing, bounded per-PR run; fall back to opus if Fable's cyber classifiers ever refuse a security-heavy diff); add concurrency group with cancel-in-progress so rapid pushes stop billing overlapping full-model reviews of stale commits - claude.yml: pin --model fable (was silently tracking the CC default, which changed to Sonnet 5 under our feet in v2.1.197) - claude-review.md prompt: the mandated submission flow (cat > file, sed -i, VAR=$(...)) prefix-matched none of the allowed tools, so every review had to improvise — rewritten as bare gh pr view + single gh api --input - heredoc with literal SHA; add behavioral-guard live-validation bullet (repo-agnostic) - dotfiles.yml: apt-get update before zsh install; drop redundant shellcheck/jq installs (preinstalled on ubuntu-latest) Models: - agents: audit-* (5), rfc-* (2), improve-workflow → model: fable; status-report stays haiku, summarize-work stays sonnet - settings.json: opus[1m] → fable (1M context is Fable's default; the [1m] suffix is auto-stripped). fastMode kept — inert on Fable, active again if the model is ever flipped back to opus - summarize skill: claude-sonnet-4-6 → claude-sonnet-5 (5 refs); mcp-builder eval script/docs: claude-3-x → claude-sonnet-5 Stale tool references: - work.md: mcp__github__get_issue → issue_read(method: "get") - pr-review.md: get_pull_request_comments/reviews → pull_request_read(method: ...); Task( → Agent( spawn examples - settings.json: drop code-simplifier plugin (redundant with built-in /simplify since v2.1.154) - bootstrap.sh: GitHub MCP via deprecated npx @modelcontextprotocol/server-github → official remote HTTP server with ${GITHUB_TOKEN} header expanded by CC at request time Docs/skills sync: - hook-authoring skill: trigger list, lifecycle, input-field table, and example refs now cover all 10 registered hooks (was 5), pointing at hooks/README.md as source of truth - hooks/README.md: enforce-insight step 3 matches the content-aware wait actually implemented (#316) - catppuccin skill: correct yazi (flavor package via ya pkg) and bat (vendor submodule) rows; rust-async skill: native async fn in traits (Rust 1.75+) as the default, async-trait for dyn/Send cases Deferred #324 suggestions: - notification.sh: permission padlock matches raw input, not the truncated display message (+ regression test) - test-hooks.sh: document the mock-jq byte-vs-codepoint truncation coverage boundary; fix stop_hook_active test running under mock jq (passed vacuously — now uses real jq like its siblings) Skipped pending live-payload verification on a real machine: statusline stdin repo/PR fields + used_percentage, SessionStart sessionTitle. Validated: make check green (101 hook + 35 bootstrap tests), new hook behavior re-exercised against real jq with a stubbed zellij. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrBLzQixo3zx8wSHVSGUXS * hooks: address #317 follow-up suggestions on the drain's bounded consume - Pin the CLI assumption the bound relies on: --limit is applied after --exclude (PEEKED_COUNT is a post-exclusion count), consistently between peek and consume — documented at the consume site so a CLI flag-semantics change can't silently reopen the loss window - New multi-event test: one directed + one ambient peeked → consume --limit must equal 2, locking the bound to the actual peeked count rather than a coincidental 1 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrBLzQixo3zx8wSHVSGUXS --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Fix the transcript-stabilization race in
enforce-insight-publish.sh(the Stop hook that enforces ★ Insight → publish_event).Problem: the hook races Claude Code's transcript writer. The turn's final
textblock (carrying the insight) is often flushed after the hook fires. The old wait polled line count only — a writer that hadn't started flushing yet read as "stable & empty", so the hook missed the insight and failed to block (the original incident saw this in 3/4 firings).Fix: content-aware wait — poll until line count is stable and the current turn contains an assistant
textblock, with a ~2s hard cap so a turn that legitimately ends without text (puretool_use) isn't penalized.Test: new regression test backgrounds the final text-block append ~300ms after the hook starts and asserts it still blocks. Fails on the old line-count-only wait; passes now.
Separate PR (not bundled in the audit PR #314) because it's the enforcement hook and warranted its own focused change + test.
Test plan
make check— 89 hook tests pass (was 88 + new race test), shellcheck clean🤖 Generated with Claude Code