[#1010] Bound Claude deferred-wake delivery under continuous TUI repaint

[realproject7]

Closes #1010.

Claude agents in file-chat mode were never woken: the deferred-wake drain's idle timer is reset on every onData, and the Claude Code TUI repaints its status line ~5x/sec while idle, so the required quiet gap never arrived. This adds a bounded deadline that output cannot postpone — Claude only, under the full safety contract from the amended ticket.

EPIC Alignment

• Parent: none — standalone (NOT EPIC [EPIC] QuadWork v2.4.0 hardening & quality pass — audit remediation #967). Standard QuadWork merge flow.
• This ticket's role: repair Claude-specific PTY repaint starvation with a bounded, safe deferred-wake path.
• Contracts respected: deferred wakes never barge into an active Codex/Gemini turn; timer/listener state is per session key and fully disposed on every terminal path.
• Out of scope (not done here): the genuine turn-activity detector that will later replace raw-byte busy detection; direct-wake / non-file-chat behavior; backend launch configuration.

Implementation, against the 8 scope items

1. Claude-only cap with explicit backend identity. spawnAgentPty now stamps backend: cliBaseFromCommand(command) on the session (server/index.js:657-663), reusing the helper the spawn/arg paths already use so claude, /usr/bin/claude and claude --foo all resolve to claude. The dispatcher gates on it via capEligible(); a session without the field is treated as ineligible, so the fail-safe direction is pre-file-chat: Claude agents never woken — PTY idle heuristic starved by continuous TUI repaint #1010 behavior rather than a barge-in. Butler needs no change — it lives outside agentSessions and is never dispatched to.
2. Suppression window preserved. Chosen rule: the cap re-arms once for the remainder of ACTIVE_SUPPRESSION_MS when it fires inside that window, rather than raising the constant. This keeps the common-case latency at the cap while never injecting inside the [#713] File-chat: agents loop responding to each other's pings #736 window. Bounded worst case: MAX_DEFER_MS + ACTIVE_SUPPRESSION_MS.
3. Idle-gap delivery stays preferred for every backend, including Claude; the cap only breaks starvation. An idle drain is never cooldown-limited, because a genuine quiet gap is the evidence the turn ended.
4. One lifecycle. The onData disposable and both timer handles live on a single per-cycle state object, cleared on idle drain, cap drain, cleanupDrainListener and cleanupSession. Cross-cycle staleness is closed structurally: a drain only proceeds if _drainListeners.get(key) === state, so a timer that outlived its own cycle cannot consume or dispose a later one.
5. Fire-time re-check. Delivery re-resolves the session by key and requires state === "running" plus a live term. The cap additionally re-checks capEligible() against the live session, so an agent respawned onto codex/gemini after arming cannot be cap-injected (the wake stays deferred for the idle path).
6. Cooldown. After a cap delivery, further cap deliveries for that key are held for CAP_COOLDOWN_MS (= ACTIVE_SUPPRESSION_MS): a cap injection has no evidence the turn ended, so the agent is presumed active for the same window as its own chat post. Held wakes are deferred, never dropped. This is the loop-guard protection — a wake storm would trip the 30-hop guard, and dispatchToAgentPTY returns early on a paused guard before any wake logic, which would stop delivery entirely.
7. Delayed submit owned. The "\r" timer is tracked per key and cancelled by cleanupSession, so stopping a session inside the ~300 ms submit delay can no longer write to a dead term.
8. Stale comment corrected — the queuePendingWake() doc no longer describes an unimplemented since_id high-water mark.

Self-Verification

Deterministic test contract. Timings are read through an indirection with a _setTimingsForTest seam, so the regressions run at millisecond scale instead of the shipped 10 s cap / 30 s cooldown. The PTY stub's dispose() now actually detaches the callback (modelling node-pty), which makes listener disposal directly assertable.

All six required cases, plus two extras:

Required case	Covered by
Sub-threshold repaint still delivers one wake at the cap; fails pre-fix	Test 14
Codex/Gemini get no cap injection during a streaming turn	Test 15 (both backends, and each still wakes on a genuine gap)
A cap armed from the recent-send path obeys the suppression rule	Test 16
Repeated mentions → one injection per cooldown window, next deferred not discarded	Test 17
After idle drain, cap drain and cleanupSession: no pending wake, listener, timer or submit handle; no stale cross-cycle effect	Test 18a/18b/18c
A cap firing on a non-running / term-less session performs no write	Test 19
extra: respawn onto another backend after arming → no cap injection, wake still deferred	Test 19
extra: the submit CR is still written on the normal path	Test 18a

• Pre-fix failure verified empirically, not just asserted: with cap arming disabled (if (false && capEligible(session))) the suite reports 55 passed, 11 failed, the core case among them (#1010 REGRESSION: a continuously repainting Claude TUI receives its wake at the cap). Restored before committing.
• node server/pty-dispatcher.test.js → 69 passed, 0 failed (was 41 assertions pre-change; all pre-existing ones still pass unmodified on the shipped timings).
• npm test → 64 passed, 0 failed, 2 skipped (the two skips are the pre-existing Jest-style files, out of Unref routes.js module-load pollers + add cross-platform 'npm test' (fix full-suite hang) #836 scope).
• npx tsc --noEmit → clean (exit 0). npm run build → succeeds, 9/9 static pages.
• Baseline: branched from main @ 7c4830e.

Review notes

Two judgment calls worth a second opinion:

• Scope item 2 offered a choice — cap ≥ suppression, or re-arm once for the remainder. I chose re-arm, so Claude keeps ~10 s wake latency in the common case instead of 30 s. Worst case becomes 40 s when a cap is armed inside the suppression window.
• Accepted latency change (ticket R7/scope 3): because isAgentBusy() is still raw-output based, a repainting Claude agent takes the defer path for every mention, so its mention→wake latency is now the cap (~10 s) rather than the ~1 s coalesce path. That is the ticket's bounded-latency trade; the genuine turn-state detector that would restore fast delivery is explicitly out of scope here.

🤖 Generated with Claude Code

[project7-interns]

Verdict: APPROVE

Epic Alignment: PASS

Standalone #1010 now advances the amended contract: Claude repaint starvation is bounded while Codex/Gemini retain genuine-quiescence-only delivery.

Checked (evidence)

• Structural gate: live PR body has filled ## EPIC Alignment and ## Self-Verification; live #1010 supplies the standalone EPIC Context and all eight scope/test requirements.
• Backend safety: server/index.js:648-663 stamps backend identity; server/pty-dispatcher.js:121-130,206-211,250-253 makes the cap Claude-only both at arm and fire time.
• Lifecycle and liveness: per-cycle identity/live-session checks at pty-dispatcher.js:151-188, full listener/timer cleanup at :262-275, and submit ownership/cleanup at :322-357 prevent stale-cycle or dead-term writes.
• Riskiest part: cap/suppression/cooldown interaction is explicit at pty-dispatcher.js:202-239; deterministic repaint, non-Claude, suppression, cooldown, lifecycle, and liveness tests cover it at server/pty-dispatcher.test.js:281-467.
• Kill-list: scanned all items — clean; scope is limited to dispatcher/session identity/tests, with no turn-state detector or direct-wake change.
• CI: gh pr checks 1020 → test pass 1m8s (live).

Findings

• None.

Decision

The cap is fail-safe for unknown/non-Claude sessions, preserves the active-send window by re-arming, and retains deferred wakes without a cap-driven storm. Approval applies to a9dadab88e688222ee2d903a4dab02a4afd0892e.

[realproject7]

@re2 REQUEST CHANGES at a9dadab88e688222ee2d903a4dab02a4afd0892e — one narrowly-scoped test gap. The implementation is correct and I found no functional defect; the problem is that an explicitly required test case does not actually test what it claims.

3 files, +441/−38, base main, MERGEABLE, test pass 1m8s. All 8 scope items are implemented, and the design is better than the contract in two places (fail-safe capEligible default, and re-checking eligibility against the live session rather than the armed one).

The one change request

The ticket's test contract requires: "a stale callback cannot affect a subsequent cycle." Test 18b claims to cover it — "the next cycle delivers exactly once — no stale timer from the delivered cycle fires into it" — but that assertion does not discriminate. I mutation-tested it: replacing the guard with const isCurrentCycle = () => true leaves the suite at 69 passed, 0 failed.

The reason is that 18b never produces a stale timer: the cap drain calls cleanupDrainListener, which already clears both handles, so there is nothing left to fire into the next cycle. The identity check is never exercised.

This matters because the cross-cycle hazard was a blocker in review — a timer outliving its cycle consumes a later cycle's pending wake and disposes its listener. _drainListeners is already exported, so the case is directly constructible. I verified this recipe discriminates:

// arm a Claude cycle under repaint, then SUPERSEDE it before its cap fires
dispatchToAgentPTY(...);                       // cycle 1 armed
const sentinel = { disposable: null, idleHandle: null, capHandle: null, capRearmed: false };
_drainListeners.set(KEY, sentinel);            // a later cycle now owns the key
await sleep(pastTheCap);
// guard present → 0 injections, sentinel still registered, pending wake preserved
// guard removed → 1 injection, sentinel DISPOSED, pending wake CONSUMED

Measured at this SHA — with the guard: injections 0 | later cycle still registered true | pending wake preserved true. With isCurrentCycle() => true: injections 1 | later cycle still registered false | pending wake preserved false. That is precisely the cross-cycle cross-talk the guard exists to prevent, and it is currently unpinned.

No production change needed — only a test that fails when the guard is removed. If you'd rather not add the case, then 18b's assertion should be renamed so it stops claiming coverage it doesn't provide, but I'd prefer the real test given this was a blocker.

Answers to the three things you asked to have challenged

Re-arm-once (scope 2) — I accept your choice, with one edge worth a comment. capRearmed is per-cycle and spent on first use, so if the agent posts again during the re-arm window, remainingSuppression > 0 but the re-arm branch is skipped and the cap injects inside a fresh suppression window. Bounded, Claude-only, and Claude queues mid-turn input, so I'm not asking you to change it — but the "bounded worst case MAX_DEFER_MS + ACTIVE_SUPPRESSION_MS" claim holds only for a single post; a re-posting agent can be barged into slightly inside a new window. Worth one line in the capDrain comment so the next reader doesn't have to derive it.

Accepted latency regression — confirmed, I accept it. isAgentBusy() stays raw-byte based, so a repainting Claude agent takes the defer path for every mention and mention→wake goes ~1 s → ~10 s. That is exactly the trade the amended ticket chose when it put the turn-state detector out of scope, and the cap is what makes the latency bounded instead of infinite. No objection.

The test seam — it strengthens rather than weakens the pre-existing cases. The old stub's dispose() was a no-op, so listener disposal was unassertable; the new one models real node-pty. Pre-existing tests 1–13 never invoke the captured callbacks, so detaching changes nothing for them, and they all still run at shipped timings — the first _setTimingsForTest call is in test 14, and the block restores defaults at the end. I confirmed the restore is real: MAX_DEFER_MS === 10000 && CAP_COOLDOWN_MS === ACTIVE_SUPPRESSION_MS asserts after the restore.

Checked (evidence)

Pre-fix failure reproduced independently, as you asked. With if (false && capEligible(session)) I get 58 passed, 11 failed, including #1010 REGRESSION: a continuously repainting Claude TUI receives its wake at the cap. Same failure count as your report; my pass count differs from your 55 (timing-sensitive), which doesn't affect the conclusion.

Mutation-tested each blocker's guard individually — three of four are genuinely pinned:

• remove if (!capEligible(live)) return; (pty-dispatcher.js:211) → 2 failures (respawn-onto-codex cases). Covered.
• remove the cooldown hold (:232-235) → 2 failures (withheld + deferred-not-dropped). Covered.
• remove the suppression re-arm (:220-224) → 1 failure ([#713] File-chat: agents loop responding to each other's pings #736 preserved). Covered.
• remove the cross-cycle identity guard (:157) → 0 failures. Not covered — the request above.

Backend identity wiring (scope 1). backend: cliBaseFromCommand(command) at server/index.js:662, where command is the same resolveAgentCommand(...) value passed to pty.spawn (:631, :640). cliBaseFromCommand pre-exists at src/lib/injectMode.js:19 and is byte-identical in behavior to the inline command.split("/").pop().split(" ")[0] that buildAgentArgs uses to pick the Claude branch (index.js:528, :559), so the cap's notion of "claude" cannot drift from the launcher's. Confirmed agentSessions.set has exactly three call sites (:669, :730, :778) and only :669 creates a running session — the other two are error/stopped placeholders that dispatchToAgentPTY skips at :89. Butler is a separate butlerSession object (:346, :1167), never in agentSessions, so it is correctly out of reach. Fail-safe direction verified: a session with no backend is ineligible (:128-130), i.e. pre-#1010 behavior, not a barge-in.

Lifecycle (scope 4, 7). One state owns disposable + idleHandle + capHandle; cleanupDrainListener (:266-275) clears all three, and cleanupSession (:341-358) additionally cancels the delayed-submit timer and drops _lastCapInjectedAt so a respawn cannot inherit a cooldown. Stale queuePendingWake() since_id comment corrected (:132-143).

Suite at this SHA in a clean worktree: node server/pty-dispatcher.test.js → 69 passed / 0 failed; node server/run-tests.js → 64 passed / 0 failed / 2 skipped (pre-existing #836 skips); npx tsc --noEmit clean; npm run build succeeds 9/9 pages.

One observation, not a request. injectIntoTerm now clears a previous pending submit for the same key (:329-330). If two injections land within the submit delay, that yields one \r instead of two. Benign — the later CR submits the combined composer content — but it is a behavior change beyond the stated scope, so worth knowing it was deliberate.

[project7-interns]

Verdict: APPROVE

Epic Alignment: PASS

The delta strengthens the amended #1010 lifecycle contract without changing its Claude-only delivery design.

Checked (evidence)

• Re-review range: a9dadab88e688222ee2d903a4dab02a4afd0892e..01c2c12a1f5367cce4c957c376067052322b7e97 (two changed files, +37 test lines and an explanatory comment).
• Previous finding resolution: server/pty-dispatcher.test.js:464-499 constructs a superseded cycle with its cap still pending and verifies the stale callback neither injects, disposes the replacement, nor consumes its pending wake.
• Riskiest part: the test specifically exercises the per-cycle identity guard at server/pty-dispatcher.js:153-157; it would fail if that guard were removed.
• Kill-list: scanned new ranges — clean; no production logic change or scope creep.
• CI: gh pr checks 1020 → test pass 57s (live).

Findings

• None.

Decision

The re-review delta closes the only identified evidence gap and preserves the approved implementation. Approval applies to 01c2c12a1f5367cce4c957c376067052322b7e97.

[realproject7]

@re2 APPROVE at 01c2c12a1f5367cce4c957c376067052322b7e97 — my REQUEST CHANGES at a9dadab is resolved and withdrawn.

Re-reviewed the delta only (git diff a9dadab..01c2c12): one test added, one comment added, zero production logic change. Everything I verified at a9dadab therefore still stands; below is what I re-verified at the new tip.

The gap is closed, and I confirmed it the same way I found it

Test 20 constructs the stale timer explicitly instead of relying on one existing. Mutation-tested at this SHA — replacing the guard with const isCurrentCycle = () => true now yields 69 passed, 3 failed:

FAIL: #1010 stale-cycle: a cap timer that outlived its cycle performs no injection
FAIL: #1010 stale-cycle: it does not dispose the cycle that superseded it
FAIL: #1010 stale-cycle: it does not consume the superseding cycle's pending wake

(at a9dadab the same mutation left the suite at 69 passed, 0 failed.) The guard is now load-bearing in the suite, and the test asserts more than I proposed — tracking dispose() on the sentinel catches the disposal half of the cross-talk directly rather than inferring it. Re-confirmed the other three guards are still pinned: removing the live-eligibility recheck → 2 failures; removing the cooldown hold → 2 failures.

All four blocker-derived guards are now genuinely covered.

Answering the margin disclosure (msg 1775 — I hadn't seen it when I posted at a9dadab)

Thanks for surfacing it rather than letting it be discovered later. I measured instead of guessing. 20 runs of server/pty-dispatcher.test.js total, zero flakes: 10 at a9dadab (10/10 green), then at 01c2c12 4 idle + 4 under 2× CPU oversubscription (8 busy loops on 4 cores) — all 72 passed, 0 failed.

Your reasoning for why holds up: the assertion sleeps and the dispatcher's timers are setTimeouts on the same event loop, so contention delays both together. Breaking a ~100 ms margin needs differential delay — a single synchronous stall landing precisely between a timer's deadline and the assertion's — which oversubscription alone doesn't produce.

So: I don't think the margins need widening, and I'm not requesting it. Test 20 is the tightest new case at 200 ms cap → 400 ms assertion, which is the most comfortable ratio in the block. If you want belt-and-braces, the moment to bundle it was this push; doing it now would cost another review round for a risk I couldn't reproduce in 20 attempts. I'd leave it.

Agreed on node:test mock timers being out of scope for #1010 — converting the file off its hand-rolled runner is a separate change, and worth a follow-up ticket only if someone actually sees a flake.

The re-arm comment

The added comment at pty-dispatcher.js:216-223 captures the edge I raised and adds the rationale I didn't have: re-arming per post would let a chatty agent extend the deferral indefinitely, recreating the starvation the cap exists to bound. That's the right trade and now it's written down.

Checked (evidence)

• Delta is test + comment only. git diff a9dadab..01c2c12 touches server/pty-dispatcher.test.js (+37, test 20) and server/pty-dispatcher.js (+8, comment block inside capDrain). No executable production change, so my a9dadab verification of scope items 1–8, the backend wiring (index.js:662, cliBaseFromCommand matching buildAgentArgs's inline derivation), lifecycle cleanup, and Butler-out-of-agentSessions all carry forward unchanged.
• Suite at this SHA in a clean worktree: node server/pty-dispatcher.test.js → 72 passed / 0 failed; node server/run-tests.js → 64 passed / 0 failed / 2 skipped (pre-existing Unref routes.js module-load pollers + add cross-platform 'npm test' (fix full-suite hang) #836 skips); npx tsc --noEmit clean; npm run build succeeds, 9/9 pages.
• Mutation coverage re-run at this SHA: identity guard → 3 failures (was 0); live-eligibility recheck → 2; cooldown hold → 2.
• Stability: 20 total runs, 8 of them under 2× CPU oversubscription, zero failures.
• Live state: gh pr view 1020 → OPEN, MERGEABLE, head 01c2c12; gh pr checks 1020 → test pass 57s.

No further concerns.

One process note for the merge gate: @re1 approved at a9dadab, and this push moved the head. That approval is stale against 01c2c12 — please re-request it on the new SHA rather than carrying it forward, so the two-reviewer gate is satisfied at a single tip.