fix(cli): stop the drive-attach input-stream flood; recover or exit non-zero - #1453
Conversation
…e error per keystroke A drive session whose PTY input stream died logged [drive] input stream send failed: PTY input stream is closed once per inbound stdin chunk, forever, and stayed alive while doing it. The session was unusable but looked idle, and exited 0 when the human finally pressed Ctrl+C. There was no retry loop. The repetition was 1:1 with stdin: the SDK's PtyInputStream latches `_closed` and rejects every later send with that exact string (transport.ts:194-203), and the CLI caught the rejection, logged, and returned without ever nulling the handle, reading the `closed` getter it already exposes, or ending the session. Because KeybindParser forwards every byte except Ctrl+C / Ctrl+], a source TUI with mouse tracking on turned pointer movement into a flood with no keystroke at all. The stream dies easily and asymmetrically: the broker pings the events WebSocket every 30s (listen_api.rs:2932/:2984) but never pings the input WebSocket, so an idle input socket is silent on the wire and any idle timeout reaps it alone — the screen keeps updating while input is dead. A broker-side write error (PTY worker restart) closes it the same way. The keepalive gap itself is filed separately as #1450. Both attach modes now share `attach-input-recovery.ts`: report the loss once, reopen with bounded exponential backoff, and exit non-zero with a readable message when that is exhausted. A reopen resolves by agent *name*, and a name is not an identity, so a successful reopen is not accepted until the worker process is verified unchanged. The broker exposes no per-instance token — only the harness pid on GET /api/spawned (worker.rs:242) — so the check is a heuristic that fails closed: identity missing at attach, unreadable after reconnect, or changed all refuse the replacement rather than route keystrokes into a PTY the human did not attach to. Partial fix for #1419 — acceptance criteria 2 and 3. The event-WS resume half (criteria 1, 4, 5: replay reconciliation, snapshot re-sync, queued message repair, connection rotation) is deliberately left open there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPTY input streams now recover after closure or send failure. Recovery uses bounded backoff, worker identity checks, predictive-echo rollback, outage input dropping, teardown cancellation, and non-zero exhaustion results. Tests cover recovery, validation, cancellation, and permanent closure. ChangesPTY input-stream recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AttachSession
participant InputStreamRecovery
participant PTYInputStream
participant BrokerConnection
AttachSession->>InputStreamRecovery: report input-stream failure
InputStreamRecovery->>PTYInputStream: open replacement stream
InputStreamRecovery->>BrokerConnection: fetch worker identity
BrokerConnection-->>InputStreamRecovery: return worker identity
InputStreamRecovery-->>AttachSession: install verified replacement
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/cli/src/cli/lib/attach-input-recovery.ts (2)
126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the stale function name in the comment.
The comment names
isDead(). The exported predicate isisUsable()(line 120). Callers short-circuit with!inputRecovery.isUsable(stream).📝 Proposed comment fix
- // Drop the dead handle first: `isDead()` then short-circuits every chunk - // that arrives mid-recovery, which is what actually silences the flood. + // Drop the dead handle first: `isUsable()` then short-circuits every chunk + // that arrives mid-recovery, which is what actually silences the flood.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-input-recovery.ts` around lines 126 - 127, Update the recovery comment near the dead-handle cleanup to reference the exported isUsable() predicate and its !inputRecovery.isUsable(stream) short-circuit instead of the stale isDead() name.
110-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake
cancel()release the pending backoff waiter.
cancel()clears the timer but never resolves the promise created at line 207. If a caller callscancel()whileisSettled()is still false, the recovery loop never resumes andinFlightstays non-null, sorecover()is blocked for the rest of the session. Both current call sites run insidefinish()aftersettled = true, so this is not reachable today. The public contract on line 87 ("Cancel a pending backoff timer") does not state that requirement.♻️ Proposed fix to make cancellation self-contained
let inFlight: Promise<void> | null = null; let timer: ReturnType<typeof setTimeout> | null = null; + let cancelled = false; + let releaseWait: (() => void) | null = null; const cancel = (): void => { + cancelled = true; if (timer) { clearTimeout(timer); timer = null; } + releaseWait?.(); + releaseWait = null; };Then check
cancellednext to eachisSettled()guard in the loop, and setreleaseWait = resolveinside the backoff promise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-input-recovery.ts` around lines 110 - 121, Update the recovery loop and backoff promise to make cancellation self-contained: assign the pending wait resolver to releaseWait, have cancel() invoke it while clearing the timer, and check cancelled alongside each isSettled() guard so a cancelled wait resumes and exits cleanly without leaving inFlight blocked.packages/cli/src/cli/lib/attach-passthrough.ts (2)
60-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the shared attach helpers out of
attach-drive.ts.
attach-passthrough.tsnow importsfetchWorkerIdentityandCliPtyInputStreamfromattach-drive.js. That makes passthrough depend on the drive module for shared contracts.attach-input-recovery.tsalready importsCliPtyInputStreamfrom the same place. A shared module (for exampleattach-input-recovery.tsor a newattach-shared.ts) would keep the dependency direction clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-passthrough.ts` around lines 60 - 67, Move the shared attach contracts fetchWorkerIdentity and CliPtyInputStream out of attach-drive.ts into a neutral shared module, then update attach-passthrough.ts and attach-input-recovery.ts to import them from that module. Keep attach-drive.ts using the shared definitions without making passthrough depend on the drive module.
580-595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated identity gate.
This
verifyIdentitybody is identical to the drive implementation inpackages/cli/src/cli/lib/attach-drive.tslines 988-1002, including the three reason strings. A divergence in one copy would produce inconsistent operator messages across the two attach modes. Move the gate into a helper exported fromattach-input-recovery.tsthat takes a baseline getter and agetWorkerIdentityfunction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-passthrough.ts` around lines 580 - 595, Extract the shared identity verification logic from verifyIdentity in the passthrough and drive attach flows into an exported helper in attach-input-recovery.ts. Have the helper accept a baseline-identity getter and getWorkerIdentity function, preserve the existing null checks, comparison, and reason strings, and update both implementations to delegate to it.packages/cli/src/cli/lib/attach-drive.ts (1)
1228-1236: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the added attach-time round trip is acceptable on the setup path.
deps.getWorkerIdentityruns inside thetryblock that reports "could not open PTY input stream".fetchWorkerIdentitycatches its own errors and returnsnull, so the default path is safe. An injected implementation that rejects would abort the attach with a misleading message. The call also adds oneGET /api/spawnedround trip before raw mode is set.Consider wrapping the call so a rejection degrades to
nullinstead of failing attach.♻️ Proposed hardening
- attachedWorkerIdentity = await deps.getWorkerIdentity(connection, name); + attachedWorkerIdentity = await deps + .getWorkerIdentity(connection, name) + .catch(() => null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-drive.ts` around lines 1228 - 1236, Harden the attach-time identity lookup around deps.getWorkerIdentity so any rejection is converted to null and does not abort the initial attach. Preserve the existing settled cleanup path and allow setup to continue without an identity baseline, while keeping the current PTY error handling for actual stream failures.packages/cli/src/cli/lib/attach-passthrough.test.ts (1)
1284-1287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the exhaustion message exists before matching its content.
findreturnsundefinedwhen no line matches.expect(undefined).toContain(...)then fails with a type error instead of naming the missing message. The drive suite guards withtoBeDefined()first (packages/cli/src/cli/lib/attach-drive.test.tsline 2236).💚 Proposed test fix
const exhausted = errors .map((args) => String(args[0])) .find((line) => line.includes('could not be reopened')); + expect(exhausted).toBeDefined(); expect(exhausted).toContain('Alice is still running');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-passthrough.test.ts` around lines 1284 - 1287, Update the exhaustion assertion in the attach-passthrough test to first verify that the value returned by errors.find for “could not be reopened” is defined, then assert it contains “Alice is still running,” matching the guarded assertion pattern used by the drive test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/cli/lib/attach-drive.ts`:
- Around line 1035-1041: Update the send rejection handlers in the attach-drive
flow around inputRecovery.recover and the corresponding attach-passthrough
handler to detect and ignore the input_backpressure error before recovery. Only
call inputRecovery.recover for closed or missing input-stream failures,
preserving the existing settled guard and error-description handling.
In `@packages/cli/src/cli/lib/attach-input-recovery.ts`:
- Around line 164-176: Ensure attemptReopen owns and closes replacement on every
exit path. In packages/cli/src/cli/lib/attach-input-recovery.ts#L164-L176, move
closeQuietly above attemptReopen, declare replacement as CliPtyInputStream |
null, and close it in the catch block before returning 'retry'. At
packages/cli/src/cli/lib/attach-input-recovery.ts#L184-L196, call
closeQuietly(replacement, `${label} client exiting`) before returning 'settled'.
---
Nitpick comments:
In `@packages/cli/src/cli/lib/attach-drive.ts`:
- Around line 1228-1236: Harden the attach-time identity lookup around
deps.getWorkerIdentity so any rejection is converted to null and does not abort
the initial attach. Preserve the existing settled cleanup path and allow setup
to continue without an identity baseline, while keeping the current PTY error
handling for actual stream failures.
In `@packages/cli/src/cli/lib/attach-input-recovery.ts`:
- Around line 126-127: Update the recovery comment near the dead-handle cleanup
to reference the exported isUsable() predicate and its
!inputRecovery.isUsable(stream) short-circuit instead of the stale isDead()
name.
- Around line 110-121: Update the recovery loop and backoff promise to make
cancellation self-contained: assign the pending wait resolver to releaseWait,
have cancel() invoke it while clearing the timer, and check cancelled alongside
each isSettled() guard so a cancelled wait resumes and exits cleanly without
leaving inFlight blocked.
In `@packages/cli/src/cli/lib/attach-passthrough.test.ts`:
- Around line 1284-1287: Update the exhaustion assertion in the
attach-passthrough test to first verify that the value returned by errors.find
for “could not be reopened” is defined, then assert it contains “Alice is still
running,” matching the guarded assertion pattern used by the drive test.
In `@packages/cli/src/cli/lib/attach-passthrough.ts`:
- Around line 60-67: Move the shared attach contracts fetchWorkerIdentity and
CliPtyInputStream out of attach-drive.ts into a neutral shared module, then
update attach-passthrough.ts and attach-input-recovery.ts to import them from
that module. Keep attach-drive.ts using the shared definitions without making
passthrough depend on the drive module.
- Around line 580-595: Extract the shared identity verification logic from
verifyIdentity in the passthrough and drive attach flows into an exported helper
in attach-input-recovery.ts. Have the helper accept a baseline-identity getter
and getWorkerIdentity function, preserve the existing null checks, comparison,
and reason strings, and update both implementations to delegate to it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ead678c7-97fe-4554-a97f-1f1e2da19cbe
📒 Files selected for processing (6)
packages/cli/src/cli/lib/attach-drive.test.tspackages/cli/src/cli/lib/attach-drive.tspackages/cli/src/cli/lib/attach-input-recovery.tspackages/cli/src/cli/lib/attach-passthrough.test.tspackages/cli/src/cli/lib/attach-passthrough.tspackages/harness-driver/src/pty-input-stream.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a9d485794
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const verdict = await verifyIdentity(); | ||
| if (isSettled()) return 'settled'; |
There was a problem hiding this comment.
If the user detaches or the events WebSocket closes while verifyIdentity() is pending, teardown sees inputStream as null and cannot close this local replacement; once verification resolves, this branch returns without calling closeQuietly. The successfully opened input WebSocket therefore remains live, and because a normal code-0 attach does not force process.exit, it can keep the CLI running after Ctrl+C. Close the replacement before returning from this settled path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — fixed in d2a2b91. Verified the mechanism you describe: recover() calls setStream(null) up front, so teardown genuinely cannot see this replacement, and the settled-after-verify branch returned without closing it.
attemptReopen now owns replacement on every exit path — the settled-during-verify return and the abandoned-attempt catch both closeQuietly before returning.
Assertion: "closes the replacement stream when the user detaches mid-verification", which parks the verifier on a gate, detaches, then asserts the socket is closed.
| * This turns that into one liveness event: report once, reopen with bounded | ||
| * exponential backoff, and on exhaustion hand control back to the caller so the | ||
| * session exits non-zero. A dead seat that looks alive is the defect; a | ||
| * readable exit a supervisor can act on is the contract (#1419). |
There was a problem hiding this comment.
This changes user-visible node agent attach --mode drive|passthrough behavior by adding recovery and non-zero failure exits, but the commit leaves the root [Unreleased] section empty. Add an impact-first Fixed entry and raise the heading to [Unreleased - Patch] as required for the first pending user-visible change.
AGENTS.md reference: AGENTS.md:L31-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — fixed in d2a2b91. Heading raised to [Unreleased - Patch] with two impact-first Fixed entries: the flood/recovery/non-zero-exit change, and the same-worker verification on reopen.
The first entry names the mouse-tracking amplifier explicitly, since a reader who tries to reproduce this by typing will find a quiet session and conclude it is fixed already.
| void stream.send(decoded).catch((err: unknown) => { | ||
| if (settled) return; | ||
| const message = describeError(err); | ||
| deps.log(`[drive] input stream send failed: ${message}`); | ||
| // The keystroke never reached the PTY — drop any optimistic echo | ||
| // for it so the screen doesn't show input the agent didn't get. | ||
| predictiveEcho?.rollback(); | ||
| // Only the first failure speaks; `recover` no-ops while a recovery | ||
| // is already in flight, and it does the echo rollback. | ||
| inputRecovery.recover(describeError(err)); |
There was a problem hiding this comment.
When the broker is slow enough that queued input exceeds PtyInputStream's 1 MiB high-water mark, send() rejects with input_backpressure while the stream remains open and usable. This unconditional catch treats that recoverable rejection as stream loss, closes the healthy socket, drops all outstanding input, and can detach non-zero if reopening or identity verification fails; passthrough has the same handler. Restrict recovery to errors that actually indicate a dead transport and retain the previous rollback/report behavior for backpressure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — fixed in d2a2b91. This is a regression this PR introduced, not a pre-existing issue, so thank you for catching it.
Confirmed against transport.ts:206-214: send() rejects input_backpressure with retryable: true before queuing, leaving the socket open and usable. The unconditional catch would have closed a healthy stream, dropped outstanding input, and re-run the identity gate — turning a briefly slow broker into a detach.
Send rejections now route through handleSendFailure, which classifies on the error code. Backpressure rolls back the optimistic echo (the keystroke genuinely did not land) and reports once per episode, reset on the next successful send — reporting per keystroke there would rebuild the exact flood this PR removes. Everything else is treated as loss.
Assertions: attach-drive.test.ts → "does NOT tear down a healthy stream when a send hits backpressure", plus three in attach-input-recovery.test.ts. Both red-checked.
Verified against a live broker: a PTY worker reports `pid: null` and `workerPid: 30209`. The harness `pid` (worker.rs:242 = `handle.harness_pid`) stays null until the worker completes the harness ready handshake (worker_events.rs:849), while `workerPid` (worker.rs:243 = `handle.child.id()`) is the PTY child itself and is populated as soon as the worker spawns. Keying the reopen identity gate on `pid` alone therefore made identity unverifiable — and so every reopen refused — for exactly the class of worker drive attaches to. Prefer `workerPid`, fold in the harness pid when the broker has both, and return null only when neither is present. `workerPid` is on the wire but absent from the `ListAgent` type, so it is read off the record defensively rather than widening the contract here. Found by the live-broker reproduction, not by the unit suite — the fakes had modelled `pid` as always present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Addresses the review findings on #1453. Two were defects this PR itself introduced. Identity gate is no longer optional (cubic P1). `verifyIdentity` was optional, so a caller that omitted it reconnected the session to whichever worker currently owned the agent name and forwarded the user's keystrokes there — the exact reattach-by-name hole the gate exists to close. It is now required by type and refused at runtime when absent. Backpressure no longer tears down a healthy stream (codex P2, coderabbit Major). `PtyInputStream.send()` rejects `input_backpressure` while the socket is open and usable (transport.ts:206-214, retryable: true). The unconditional catch treated that as transport loss, closing a healthy socket, dropping outstanding input, and potentially detaching non-zero because the broker was briefly slow. Send rejections now route through `handleSendFailure`, which rolls back the echo and reports once per episode for backpressure and only recovers on real loss. Also fixed: - A verifier that throws or stalls left the session with no input stream and no exhaustion path, so it hung instead of exiting non-zero (cubic P1). Both collapse into the existing `{ ok: false, reason }` refusal. - `attemptReopen` now owns the replacement stream on every exit path (codex P1, coderabbit). The abandoned-attempt and settled-during-verify paths returned without closing it, leaking a live socket with no owner that could keep the CLI alive past a clean detach. - `cancel()` resolves the pending backoff instead of only clearing the timer, so the loop unwinds and `inFlight` clears; it previously left the helper permanently `isRecovering()` and detach cleanup incomplete (cubic P2). - Open and identity waits are bounded by `attemptTimeoutMs`, so a stalled socket or hung broker call cannot park the session in recovery forever and make `maxAttempts` a fiction (cubic P2). - Ctrl+C sharing a stdin chunk with other bytes now still detaches during an outage in both modes; the dead-stream branch returned before the keybind actions ran, so "ab\x03" was swallowed and the human could not escape a broken session (cubic P2). - CHANGELOG `[Unreleased - Patch]` entry for the user-visible attach behaviour change (codex P1). - Detach-cancellation test now settles past the full 300ms backoff span rather than the first ~60ms, so a timer surviving teardown is actually caught (cubic P3). New assertions, each red-checked against the vulnerable shape: a reopen without a verifier is refused; backpressure does not tear down a healthy stream or report more than once per episode; a throwing verifier exits non-zero; a stalling verifier times out; abandoned and settled attempts close their sockets; cancel ends the loop; Ctrl+C detaches mid-outage. Live end-to-end re-verified after the change: real pty, real broker, worker killed underneath, 200 mouse reports and zero keystrokes — one line, exit 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/cli/lib/attach-input-recovery.test.ts (1)
131-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStop the recovery loop before the test ends.
This test starts recovery and asserts synchronously. It never awaits the loop, so the backoff timer and the reopen attempt keep running after the test returns. The harness callbacks only append to local arrays, so the risk is low. Cancelling keeps the suite free of background work that outlives its test.
♻️ Proposed cleanup
expect(h.recovery.isRecovering()).toBe(true); expect(h.logs.some((l) => l.includes('input stream lost'))).toBe(true); + h.settle(); + h.recovery.cancel(); });Change the test callback to
asyncfor the cleanup above if you prefer to awaitsettle()instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/lib/attach-input-recovery.test.ts` around lines 131 - 138, Update the test using harness() and handleSendFailure() to stop the recovery loop before it finishes, either by cancelling recovery explicitly or making the test callback async and awaiting the harness cleanup/settle method. Preserve the existing synchronous assertions for recovery state and the “input stream lost” log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the changelog entry for the attach command to qualify that
every byte except Ctrl+C/Ctrl+] is forwarded during normal operation, while
input may be dropped when inputRecovery.isUsable(stream) is false during
recovery.
- Line 8: Update the changelog’s top-level pending-release heading from
“Unreleased - Patch” to the standard “Unreleased” form, while preserving the
patch-level designation for the eventual SemVer release heading.
---
Nitpick comments:
In `@packages/cli/src/cli/lib/attach-input-recovery.test.ts`:
- Around line 131-138: Update the test using harness() and handleSendFailure()
to stop the recovery loop before it finishes, either by cancelling recovery
explicitly or making the test callback async and awaiting the harness
cleanup/settle method. Preserve the existing synchronous assertions for recovery
state and the “input stream lost” log.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b76123-fcbb-4ee1-bf2a-b09516e6b7cc
📒 Files selected for processing (7)
CHANGELOG.mdpackages/cli/src/cli/lib/attach-drive.test.tspackages/cli/src/cli/lib/attach-drive.tspackages/cli/src/cli/lib/attach-input-recovery.test.tspackages/cli/src/cli/lib/attach-input-recovery.tspackages/cli/src/cli/lib/attach-passthrough.test.tspackages/cli/src/cli/lib/attach-passthrough.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cli/src/cli/lib/attach-passthrough.test.ts
- packages/cli/src/cli/lib/attach-passthrough.ts
- packages/cli/src/cli/lib/attach-drive.ts
- packages/cli/src/cli/lib/attach-drive.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…e refusal path Second round of review findings on #1453. Three accepted, one rejected. A verifier that throws synchronously still stranded the session. The type permits a non-`async` function, and `verifyIdentity()` was evaluated in argument position, so the throw escaped before `.catch()` was attached — skipping the refusal, the non-zero exhaustion exit, and the close of the replacement stream. Same failure the async-throw fix closed, reachable by a different route. It is now invoked inside the promise chain. Also: - CHANGELOG: qualify the byte-forwarding claim. It described why the flood happened but read as a present-tense statement, which the recovery path now contradicts by dropping input while the stream is down. - Drop the dead `identityCalls` array from the drive test harness. The earlier fix replaced it as the index source with `identityCallCount`, leaving it written but never read. Rejected, with reasoning on the thread: CodeRabbit asked to lower the changelog heading from `[Unreleased - Patch]` to `[Unreleased]`. AGENTS.md requires the opposite — the first pending user-visible change must set a release level, and the level is monotonic and must never be lowered. That heading is there because an earlier codex P1 required it, citing the same section. New assertion, red-checked against argument-position invocation: "refuses when the verifier throws synchronously" asserts the exhaustion exit runs and the replacement socket is closed. Prettier run over the changed files this time, so CI's formatting check passes without a bot follow-up commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Partial fix for #1419. Reported directly by Khaliq, who was hitting it repeatedly.
The bug
Attaching in drive mode floods the terminal, on repeat, with:
Once it starts it never recovers. The session stays alive, looks idle, and exits 0 when the human finally presses Ctrl+C — so a supervisor sees a clean detach.
Root cause
Two independent halves.
Why the stream closes, and why only this one. A drive session holds two long-lived WebSockets and the broker treats them differently. The events/output WS is pinged every 30s (
crates/broker/src/listen_api.rs:2932, sent at:2984). The PTY input WS is pinged by nobody in either direction — the broker's handler (listen_api.rs:1783-1866) only ever answers a Ping (:2036), andPtyInputStream(packages/harness-driver/src/transport.ts) never sends one. An idle input socket is therefore completely silent on the wire while the output socket is actively kept warm, so any idle timeout between client and broker reaps the input socket alone. A broker-side write error — a PTY worker restart — closes it the same way (listen_api.rs:1837-1843). Either path latches_closedattransport.ts:143.That asymmetry is why this presents as a hang rather than a disconnect: the screen keeps updating while input is permanently dead. The keepalive gap is filed separately as #1450 — that is the actual prevention; everything in this PR is recovery.
Why it repeats forever — and it is not a retry loop. Nothing re-sends; there is no retry and no backoff anywhere on this path.
PtyInputStream.send()checks_closedand rejects immediately with that exact string (transport.ts:194-203— the only site producing this wording; the close path saysPTY input stream closed (1006)).attach-drive.tscaught the rejection, logged, rolled back predictive echo, and returned. It never nulled the handle, never read theclosedgettertransport.ts:167already exposes, and never calledfinish(). So the line repeats once per inbound stdin chunk, forever.Why it floods rather than trickles
KeybindParserforwards every byte except0x03and0x1d. If the agent's TUI has mouse tracking enabled — most do — every pointer movement over the terminal is an SGR mouse report on stdin, and each one produces a line. Khaliq never had to type. Anyone trying to reproduce this by typing slowly will wonder why their session is quiet; move the mouse instead. Focus in/out events do the same.Reproduction
Reproduced on demand at three levels, all using the real close path rather than a mocked failure:
PtyInputStream, close the socket server-side with 1006, then send: 200 sends → 200 identicalPTY input stream is closedrejections, zero reopen attempts, nothing back on the wire.runDriveSessionLoopwith a dead input stream and 200 synthetic mouse reports and zero keystrokes: 200 flood lines, one unique message, session still running.bashPTY worker on a real local broker (11.4.2), real input WebSocket, real close event:node agent attach <name> --mode drive, against a throwaway worker killed underneath a live session, driven by 200 SGR mouse reports and zero keystrokes:One line instead of two hundred, and a non-zero exit, with the human never touching the keyboard — the exact condition the original flood needed. Run on a genuine 80x24 pty (stdlib
pty.openpty(); macOSscript(1)cannot be used here, as it requires its own stdin to be a tty and exits 1 before starting the child, which is indistinguishable from the CLI's own failure exit).Worth noting across runs 3 and 4: the close reason differed (
agent_not_foundvswrite EPIPE) and both funnel into the same single liveness event, so the fix is not keyed to a particular error string.Two things the live run showed that the unit repros did not:
stream.closedwas stillfalseimmediately after the worker was released. The socket only fails on the next write, which errors (pty_input_error→agent_not_found) and closes the stream; every send after that hits the latched guard. So an idle drive session over a dead worker looks perfectly healthy until the human types — exactly the reported experience.agent_not_foundtriggers the single liveness event and the remaining 24 never reach the log.The fix
Both attach modes now share
packages/cli/src/cli/lib/attach-input-recovery.ts. A closed input stream is a session-liveness event, not a per-keystroke error:isUsable()short-circuits every chunk that arrives mid-outage — that is what actually silences the flood, not log dedup.attach-passthrough.tscarried the identical defect (:565) and is fixed by the same shared helper rather than a second copy.Reopen will not silently reattach to a different worker
A reopen resolves by agent name, and a name is not an identity. If the worker died and something else claimed the name, a socket that opens successfully would route the human's keystrokes into a different agent's PTY. A restart of the same agent is equally wrong for input safety.
The broker exposes no per-worker-instance token — no
instance_id,run_id,epoch, or absolute spawn timestamp reaches the wire. That gap is filed on its own as #1454, since it outlives this bug: anything that reconnects, adopts, or re-registers by name has the same blind spot.What is available is two pids, and the live broker showed they are not interchangeable:
workerPid(worker.rs:243=handle.child.id()) is the PTY child — the process whose terminal we are driving. Populated as soon as the worker spawns.pid(worker.rs:242=handle.harness_pid) is the harness wrapper, and stays null until the worker completes the ready handshake (worker_events.rs:849).A live
bashPTY worker reportedpid: null,workerPid: 30209. The first version of this gate keyed onpid, which would have made identity unverifiable — and so every reopen refused — for exactly the class of worker drive attaches to. It now prefersworkerPidand folds in the harness pid when both are present. The unit fakes had modelledpidas always present; only the live run caught this.The gate fails closed: the replacement is refused when identity was unavailable at attach, unreadable after reconnect, or changed.
nullis treated as "cannot verify", never as "verified", and a refusal is not retried — a replaced worker does not become the original one on a later attempt.This is honest about its limits: the OS can reuse a pid. The durable fix — tracked in #1454 — is for the broker to surface the per-spawn identity it already holds in memory:
WorkerHandle.spawned_at(worker.rs:134),PersistedAgent.started_at(broker.rs:41, absolute unix seconds, already written tostate.json), or theMetricsCollectorper-namespawns/restartscounters (metrics.rs:45-47, already a working generation number). Each is a one-line surfacing job; none is exposed by any route today.Tests — what each assertion catches
Red-checked: every assertion below was run against the pre-fix code and fails there, except where noted.
packages/harness-driver/src/pty-input-stream.test.tsPtyInputStreamever grows its own reconnect, this fails and the CLI recovery must be revisited rather than silently doubling up.attach-drive.test.ts—runDriveSession — lost PTY input streamattach-drive.test.ts—fetchWorkerIdentityattach-passthrough.test.ts— the same flood and exit-code assertions againstrunPassthroughSession.Full run:
packages/cli/src/cli/lib+packages/harness-driver/src→ 518 passed, 9 skipped. Repo-widevitest runhas 5 pre-existing failures in telemetry/cloud-auth/MCP-startup; verified identical on a cleanmainworktree, unrelated to this change.Scope
Deliberately the bounded subset of #1419 — its acceptance criteria 2 and 3. The event-WS resume half (criteria 1, 4, 5: reconnect on 1000/1005 without detach, replay reconciliation and snapshot re-sync, queued-message repair, connection rotation) is a materially larger lifecycle rewrite and is left open in #1419 — please do not read that issue as closed by this PR.
🤖 Generated with Claude Code