feat(terminal): add terminal.set_delivery_mode WS frame for --node drive attach - #1495
Merged
Conversation
…ive attach The `--node` drive attach was failing with "broker remained in manual_flush mode" because attach-fleet-node.ts's delivery-mode proxy handler was a static stub that always returned `manual_flush` for drive mode, never forwarding PUT requests to the remote broker. Root cause: the loopback proxy had no channel to the remote broker's HTTP API — only a Relaycast WebSocket terminal session. Fix adds a proper WS-level protocol round-trip: - broker/terminal_control.rs: add TerminalFromCloud::SetDeliveryMode and TerminalToCloud::DeliveryMode frame types (InboundDeliveryMode already has snake_case serde serialization; expected_revision is a decimal string to match the existing HTTP wire format) - runtime/fleet.rs: handle SetDeliveryMode by calling handle_api_request with a oneshot channel, reusing the existing SetInboundDeliveryMode code path (CAS guards, interactive-hold frame, queue flush, sdk event emission); view sessions are rejected; reply forwarded as DeliveryMode or Error - attach-fleet-node.ts: replace stub with real WS forwarding; track loopbackDeliveryMode locally (updated on each broker reply); pending delivery-mode promise serialises in-flight PUTs; terminal.error while a delivery-mode PUT is in flight is routed to the pending request rather than tearing down the session; close() cancels any pending request cleanly Cloud-side DO (node.ts in relaycast) still needs the new frame types added to its passthrough allowlist — tracked separately (cloud git state is broken, needs re-clone). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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 skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change forwards delivery-mode updates from the CLI proxy through the terminal WebSocket to the broker. It adds guarded wire messages, broker session handling, response tracking, timeout and shutdown handling, tests, and changelog coverage. ChangesDelivery-mode forwarding
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR adds the WebSocket delivery-mode path and associated handling without any identified merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLIClient
participant attach_fleet_node
participant terminal_control
participant fleet_runtime
participant ListenAPI
CLIClient->>attach_fleet_node: Request delivery-mode change
attach_fleet_node->>terminal_control: Send terminal.set_delivery_mode
terminal_control->>fleet_runtime: Forward delivery-mode request
fleet_runtime->>ListenAPI: Apply guarded mode change
ListenAPI-->>fleet_runtime: Return mode, flushed, matched, revision
fleet_runtime-->>terminal_control: Send terminal.delivery_mode
terminal_control-->>attach_fleet_node: Resolve pending request
attach_fleet_node-->>CLIClient: Return broker result
Possibly related PRs
Suggested reviewers: 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `agent-relay drive --node` to state
only the user-visible fix: it no longer fails with “broker remained in
manual_flush mode.” Remove references to the Relaycast terminal WebSocket,
remote broker forwarding, and the previous static stub response.
In `@crates/broker/src/runtime/fleet.rs`:
- Around line 447-457: Update the expected_revision parsing in the
SetInboundDeliveryMode request flow to reject any provided value that fails u64
parsing instead of converting it to None. Before calling handle_api_request,
return terminal.error with the invalid_revision error; preserve None only when
expected_revision is absent.
In `@crates/broker/src/terminal_control.rs`:
- Around line 61-69: Correlate delivery-mode requests and replies with a unique
request identifier: in crates/broker/src/terminal_control.rs lines 61-69 add
request_id to SetDeliveryMode, and in lines 115-121 echo it in DeliveryMode and
operation-scoped errors; in crates/broker/src/runtime/fleet.rs lines 425-490
preserve the identifier in success and error replies; in
packages/cli/src/cli/lib/attach-fleet-node.ts lines 302-320 generate and store
an identifier per PUT, and in lines 584-607 settle the pending request only when
the reply identifier matches. Ensure unrelated terminal.error messages cannot
settle the active delivery-mode request.
In `@packages/cli/src/cli/lib/attach-fleet-node.ts`:
- Around line 660-665: Move the pendingDeliveryMode cleanup from the explicit
shutdown path into endTerminal so terminal.closed immediately clears its timer
and rejects the request with the terminal-close error. Update close() to reuse
endTerminal’s cleanup instead of duplicating the logic, preserving the existing
pending request state transitions.
🪄 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: 5f972e6a-8564-4099-b56b-f33ffcb72bbf
📒 Files selected for processing (4)
CHANGELOG.mdcrates/broker/src/runtime/fleet.rscrates/broker/src/terminal_control.rspackages/cli/src/cli/lib/attach-fleet-node.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:12">
P3: The new changelog bullet includes implementation backstory ("instead of returning a static stub response") and supervisor mechanism detail, which contradicts the CHANGELOG style rules to drop internal review notes and backstory and keep entries concise and impact-first. The user-visible effect is only that `agent-relay drive --node` no longer fails with the manual_flush error; trim the entry to that effect.</violation>
</file>
<file name="crates/broker/src/runtime/fleet.rs">
<violation number="1" location="crates/broker/src/runtime/fleet.rs:447">
P2: When a terminal frame contains a malformed or overflowing `expected_revision`, this code silently drops the CAS guard and applies the delivery-mode change unconditionally. Reject invalid revisions with a `terminal.error` using `invalid_revision`, matching the HTTP path.</violation>
</file>
<file name="crates/broker/src/terminal_control.rs">
<violation number="1" location="crates/broker/src/terminal_control.rs:65">
P2: When a caller sends an invalid `expected_mode`, serde rejects the entire frame before the runtime can respond, so the request times out instead of returning an invalid-mode error. Validate these fields while preserving a `terminal.error` response for malformed requests.</violation>
</file>
<file name="packages/cli/src/cli/lib/attach-fleet-node.ts">
<violation number="1" location="packages/cli/src/cli/lib/attach-fleet-node.ts:214">
P2: The first GET reports an inferred mode rather than the worker's current mode. When the worker started in the opposite mode, `switchInboundDeliveryModeOrAbort` saves this guess and detach restores the wrong delivery mode; obtain the initial mode through the session protocol or avoid restoring an unverified value.</violation>
<violation number="2" location="packages/cli/src/cli/lib/attach-fleet-node.ts:291">
P2: When the broker reports `agent_not_found` for this PUT, return HTTP 404 instead of always returning 503. The attach preflight relies on 404 to produce the correct no-agent or cross-node placement error.</violation>
<violation number="3" location="packages/cli/src/cli/lib/attach-fleet-node.ts:296">
P2: A delayed broker reply can be lost after the timeout or satisfy a later PUT because delivery-mode frames have no request correlation. Preserve the unknown operation state until the reply is drained, or add request correlation so late replies cannot be applied to another request.</violation>
<violation number="4" location="packages/cli/src/cli/lib/attach-fleet-node.ts:296">
P2: This pendingDeliveryMode cleanup only runs inside close(), but terminal.closed frames call endTerminal, not close(). When the session closes via a terminal.closed frame while a delivery-mode PUT is pending, the request isn't rejected here and instead waits out the full 10s DELIVERY_MODE_TIMEOUT_MS, returning delivery_mode_timeout instead of the actual close error. Move this cleanup into endTerminal and call it from close() as well.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Addresses all 7 issues from CodeRabbit and Cubic reviews on the terminal.set_delivery_mode WebSocket frame implementation: 1. invalid_revision: reject unparseable expected_revision with terminal.error(invalid_revision) instead of silently dropping the CAS guard (which would allow an unconditional delivery-mode change) 2. expected_mode: validate the raw string in fleet.rs and return terminal.error(invalid_mode) for unknown values, rather than having serde silently drop the whole frame — expected_mode changed to Option<String> in the protocol struct 3. request_id correlation: add request_id to SetDeliveryMode, DeliveryMode, and Error so delayed broker replies cannot resolve or reject a later in-flight PUT; CLI generates a per-PUT ID and validates it on terminal.delivery_mode / terminal.error responses 4. endTerminal cleanup: pendingDeliveryMode is now rejected in endTerminal so a terminal.closed frame during an in-flight PUT rejects it immediately rather than waiting for the 10 s timeout 5. HTTP 404 for agent_not_found: delivery-mode PUT returns 404 instead of 503 when the broker reports agent_not_found, matching the attach preflight's expected status code for placement errors 6. Initial delivery mode: terminal.ready now includes the worker's current delivery_mode from the broker, seeding loopbackDeliveryMode with the authoritative value rather than an inferred guess 7. CHANGELOG: trim entry to impact-only per style rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
Four remaining defects across the delivery-mode WS path, verified live against the current head before fixing: - crates/broker/src/runtime/fleet.rs: the WS terminal.set_delivery_mode handler matched expected_mode against literal "auto_inject"/"manual_flush" strings, rejecting a syntactically-valid value with different casing or padding (e.g. "MANUAL_FLUSH", " manual_flush ") as invalid_mode, while the HTTP delivery-mode route accepted it via InboundDeliveryMode::parse. Extract parse_expected_mode() and route both through InboundDeliveryMode::parse so WS and HTTP semantics match exactly. - crates/broker/src/runtime/worker_events.rs: a fresh PTY with no prior entry in delivery_states caused the terminal.ready frame to omit the worker's delivery mode instead of falling back to the broker's logical default (auto_inject), leaving the attach client's own possibly-wrong inferred default (manual_flush for --node drive) in place. Extract resolve_ready_delivery_mode() and fall back to AutoInject when no state entry exists. - packages/cli/src/cli/lib/attach-fleet-node.ts: extract a shared rejectPendingDeliveryMode() helper used by endTerminal, close(), and now also the bounded-reconnect close handler. Previously a transport disconnect that took the reconnecting path (as opposed to permanent teardown) left an in-flight delivery-mode PUT pending — Relaycast drops the old lane's terminal session state on disconnect, so the frame already sent is lost and a reconnect can never resurrect a reply, guaranteeing a 10s delivery_mode_timeout on any transient reconnect during a mode switch. Reject the pending PUT immediately with a retryable delivery_mode_disconnected error instead. Tests: added packages/cli/src/cli/lib/attach-fleet-node.test.ts (new file — none existed for this loopback adapter) covering the happy path plus both prompt-rejection regressions against a real local WebSocketServer standing in for Relaycast; added Rust unit tests for parse_expected_mode and resolve_ready_delivery_mode. Confirmed the reconnect-path test times out at 10s without the fix and passes in <1s with it. cargo fmt --all -- --check, cargo clippy -- -D warnings, cargo test -p agent-relay-broker (923 passed), npm run lint, npm run typecheck, and vitest run packages/cli/src (1039 passed) are all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/cli/lib/attach-fleet-node.test.ts">
<violation number="1" location="packages/cli/src/cli/lib/attach-fleet-node.test.ts:101">
P3: The PR description lists "close() cancels any pending request cleanly" as delivered behavior (see rejectPendingDeliveryMode in close()), but none of the three tests cover it. The suite covers a successful reply, a terminal.closed rejection, and a transport-disconnect rejection, leaving the close()-cancellation path and its `closed` error code unverified. Add a test that puts a delivery-mode request in flight, calls proxy.close(), and asserts the PUT resolves promptly with 503 / error.code `closed`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… PUTs Cubic flagged (P3, confidence 8) that the delivery-mode PUT test suite added in 6dc9e61 covered a successful reply, a terminal.closed rejection, and a transport-disconnect rejection, but not the close() path itself — the PR description calls out "close() cancels any pending request cleanly" (rejectPendingDeliveryMode in close()) as delivered behavior, and that specific path had no direct test. Add a test that puts a delivery-mode PUT in flight (the remote receives the frame but deliberately never replies), calls proxy.close(), and asserts the PUT resolves promptly with 503 / error.code "closed" rather than hanging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agent-relay drive --node <agent>was failing with "Error: could not switch '' to auto_inject mode: broker remained in manual_flush mode while draining queued messages" every timepackages/cli/src/cli/lib/attach-fleet-node.tshad a static stub that always returnedmanual_flushfor drive sessions, ignoring the PUT body — the real broker was never contacted--ssh-hostworked because it tunnels directly to the broker's HTTP API;--nodegoes through a Relaycast WebSocket terminal session where no delivery-mode path existedChanges
crates/broker/src/terminal_control.rsTerminalFromCloud::SetDeliveryMode(client→node) withmode, optionalexpected_mode, andexpected_revision(decimal string, matches HTTP wire format)TerminalToCloud::DeliveryMode(node→client) with full CAS reply payloadcrates/broker/src/runtime/fleet.rsSetDeliveryMode: validates session exists and is not view-only, callsself.handle_api_request(ListenApiRequest::SetInboundDeliveryMode { ... })via oneshot channel — reuses the exact HTTP code path (CAS guards, queue flush onmanual_flush → auto_inject, interactive-hold frame, sdk event emission)DeliveryModeon success orErroron failure (agent_not_found, etc.)packages/cli/src/cli/lib/attach-fleet-node.tsloopbackDeliveryMode(updated on each broker reply)terminal.set_delivery_modeframe, awaitsterminal.delivery_modereply (10s timeout)terminal.errorwhile a delivery-mode PUT is in flight routes to the pending request without tearing down the terminal sessionclose()cancels any pending request cleanlyCompanion PR: AgentWorkforce/cloud#? (adds
terminal.set_delivery_mode/terminal.delivery_modeto the NodeDO passthrough allowlists)Test plan
cargo test --package agent-relay-broker --lib terminal_control— both tests passnpx tsc --noEmitin packages/cli)agent-relay drive --node <agent-name>on a fleet worker — should attach without the manual_flush error🤖 Generated with Claude Code