fix(hooks): reawaken jobs without replacing user hooks - #176
Conversation
Ark0N
left a comment
There was a problem hiding this comment.
Verdict: Approve. This PR depends entirely on Claude Code capabilities not documented in this repo, so this review verified every one of them against the installed CLI binaries (2.1.218/219/220), the published npm platform packages, and real session transcripts. All claims check out, including the one version-compatibility risk, which turned out to be a non-issue (details below).
What it does (5 files, +467/-56)
- New
PostToolUse(Bash)rewake hook (hooks-config.ts): an inline Node helper (embedded vianode -ewith exec-formargs) reads the hook's stdin JSON, extracts the background task id from the Bash result, tails the session transcript for the matchingqueue-operationcompletion entry, and exits 2 with a message on stderr.asyncRewake: truemakes Claude Code run it in the background and wake an idle model on exit 2. Crucially, it never injects terminal input, so it cannot submit a user's half-typed prompt. - Per-handler ownership merge (
mergeCodemanHooks): the actual bug fix. The old self-heal replaced the wholehooksobject whenever it contained a Codeman curl, destroying user hooks in mixed configurations. Now only handlers identified as Codeman's (by/api/hook-eventor theCODEMAN_BACKGROUND_REWAKE_V1marker) are replaced; user events, matchers, and sibling handlers in the same matcher group survive.writeHooksConfig(case create) now merges too instead of clobbering. refreshStaleHookSecretrenamed torefreshStaleCodemanHooks: the launch-time self-heal now also upgrades old Codeman blocks that lack the rewake hook; still strictly no-op for user-owned or absent hooks (the "never impose" policy is preserved).- Hooks reference doc refresh, and 62 tests including ones that actually spawn the helper.
Capability claims: all verified real
asyncRewakeexists, with the CLI's own schema description: "If true, hook runs in background and wakes the model on exit code 2 (blocking error). Implies async." Exactly what the PR's doc claims.- Exec-form
argsexists (landed upstream in 2.1.207 as a shell-injection fix): the command is spawned directly with no shell, so the inline script needs no quoting and no shell parsing ever sees it. timeoutis in seconds (the CLI multiplies by 1000), so the 6-hour21600is correct.- Transcript format matches byte-for-byte: real transcripts contain
"type":"queue-operation"entries with<task-id>,<status>completed</status>,<output-file>exactly as the helper parses, and real background-Bash results carrybackgroundTaskId, the helper's primary extraction key.
Version-skew risk: investigated and resolved
The theoretical bad case was a CLI that honors args but strips asyncRewake, which would make the helper run synchronously and block the turn while it polls. Bisecting the published @anthropic-ai/claude-code-linux-x64 binaries shows 2.1.207, the first version with exec-form args, already ships asyncRewake, so that combination does not exist in any release. CLIs older than 2.1.207 strip both fields, degrading the hook to a ~50ms no-op (node reads the hook JSON as a script, exits 1, non-blocking). Safe across the board.
Test verification
- Author's claim reproduces at the PR head: 62/62, plus
tsc --noEmitand prettier clean. - Branch is behind master but cleanly mergeable with zero drift under its hunks. The full CI-equivalent sweep on a local merge with current master: 3744 tests passed. (One unrelated suite,
test/webview-proxy.test.ts, failed only in the review worktree environment: a minimal probe with zero PR code fails identically there, and master CI passed that exact suite on a fresh install. Not attributable to this PR.) - Live
tmux -L codemansocket snapshot before/after the sweep: unchanged.
Non-blocking follow-ups
- The helper never self-terminates. It relies on Claude Code to enforce the timeout and reap children; if orphaned (session dies), a 1s stat-poll loop persists indefinitely. Cheap hardening: record start time and
process.exit(0)after the 6h budget, and/or exit whenprocess.ppid === 1. - Rewake vs respawn interaction: a rewake arriving after Codeman's respawn cycle has
/cleared the session lands its reminder in a fresh conversation. Harmless, but worth knowing for respawn-heavy sessions. - Pre-existing units bug made more glaring:
HOOK_TIMEOUT_MS = 10000flows into the same seconds-typed field, so the existing curl hooks have had ~2.8-hour effective timeouts all along (10s intended). Not this PR's regression (it correctly uses seconds); worth a follow-up fix. - Minor: the status alternation
(completed|failed|killed|error)matches the CLI's notification template (completed|failed|killed);stopped/cancelledwill not rewake, which is arguably correct for user-initiated stops. AndisCodemanHookHandlerclaims any handler mentioning/api/hook-event(pre-existing semantics).
Security
Positive. The inline script is static (no runtime data is interpolated into code), task ids are charset-validated before use, exec-form means no shell ever parses the script, and the stderr-to-system-reminder path can only be fed by harness-written queue-operation lines (tool output cannot forge one, since transcript JSON-escaping prevents raw newline injection). Settings writes stay under the per-path lock, and ownership detection errs conservative.
…abilize CI teardown Follow-ups from the PR #175/#176 reviews: - Rewake helper self-terminates on its own 6h deadline and when orphaned, instead of relying on Claude Code to reap the poller - Rewake marker versioned (V2) with a version-agnostic ownership prefix, so future script updates replace older handlers instead of duplicating them; regression test covers the V1 to V2 swap - HOOK_TIMEOUT_MS renamed to HOOK_TIMEOUT_SECONDS = 10: the hook timeout field is seconds (the CLI multiplies by 1000), so the curl hooks have effectively had a ~2.8h timeout since COD-54 - Test echo PTY switches to raw mode: each input byte echoes exactly once (tty line discipline doubled every line and buffered until Enter) - test/setup.ts: drain in-flight console-log rpc forwards before environment teardown (fixes the EnvironmentTeardownError that failed CI twice on the merge commit with all 3820 tests passing), clean the temp home on process exit (fully-skipped files leaked it), fix the Windows Playwright cache fallback path - test/webview-proxy.test.ts: stop naming the vitest environment directive in prose; vitest matches it inside comments and silently ran the whole file under the jsdom environment while the comment claimed node - CLAUDE.md: document the temp-HOME and echo-PTY test isolation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
PostToolUsehook for background Bash commands.Root causes
Background commands can complete while the agent is idle, but completion alone did not reliably wake the agent. Injecting terminal input is unsafe because it can submit a user's draft, so the hook observes the transcript instead.
The initial refresh implementation also treated the entire
hooksobject as Codeman-owned when any handler referenced/api/hook-event. Mixed configurations therefore lost unrelated user hooks during self-healing. Ownership is now evaluated per handler.Scope
This is the hook lifecycle slice extracted from #173. It includes hook configuration, its reference documentation, production call-site renames, and focused tests. It does not change terminal transport or mobile UI behavior.
Validation
npx vitest run --config config/vitest.config.ts test/hooks-config.test.ts test/hook-secret-selfheal.test.ts: 62 passednpm run build: passedgit diff --check: passed