[OrcaWin]

	Files	Added	Deleted	Net
Test	2	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​121	$\color{#cf222e}{\Huge{\mathbf{−}}}$​3	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​118
Prod	6	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​218	$\color{#cf222e}{\Huge{\mathbf{−}}}$​143	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​75

ELI5

This splits the runtime's WebSocket server into small, testable pieces taken from #16741: the per-connection lifecycle, and the connection and message limits. It also adds an option to require an exact port, which the headless server needs for --port. All Bun WebSocket code from #16741 is dropped.

What Changed

• node-websocket-lifecycle.ts: handles each connection, covering the pre-auth timer, heartbeat, finalize on close, over-capacity rejection (1013) and shutdown.
• websocket-transport-limits.ts: holds the message, connection and TCP caps.
• ws-transport.ts: now a thin class, down from 355 to 260 lines. It adds an opt-in strictPort.
• requirePinnedWsPort (runtime RPC state and lifecycle): binds only the configured port. On a bind failure it stops the Unix socket transport and rethrows.
• Adapted:
  • Handlers are read through getters, so a handler registered after start still reaches existing sockets.
  • Binary frames are converted with a checked branch.
  • The rationale comments are kept.
• Wire: no changes.

Why

Design D9, T1 and D9.4: keep the Node WebSocket path and drop Bun's. The headless server (T6) will set requirePinnedWsPort for --port.

Linked Issue

Part of the Node runtime plan (Phase 3). Stacked on #24181.

Visual Proof

N/A. There's no behavior change; the new option is opt-in and unused so far.

Testing

• Ported tests pass.

• Added in review: a test that a failed required-port start releases the Unix socket.

• Not yet ported: transportGeneration plumbing (read only by T3 and T7) and the remaining T1 renderer and test-server pieces.

• I manually tested these changes locally

• Automated tests added/updated, or explained why not below

Agent skill upstream boundary

• Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

[greptile-apps]

Confidence Score: 4/5

[High risk] Refactors WebSocket lifecycle and adds port-binding constraints.

The PR does not appear safe to merge while a failed required-port start can retain stale mobile socket wiring for a retry.

Findings

1.  Failed start retains old identity ▶
2.  Shutdown cuts off asset responses ▶

Summary

The PR extracts the Node WebSocket connection lifecycle and transport limits, adds an opt-in required-port bind, and introduces relay work-drain and producer-publication primitives.

• Required-port bind failure now stops the Unix socket transport.
• Relay request contexts gain transport generations, and producer notifications gain write-settlement callbacks.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Runtime[Runtime RPC startup] --> Unix[Unix socket]
  Runtime --> WS[Node WebSocket transport]
  WS --> Lifecycle[Connection lifecycle]
  WS --> Limits[Transport limits]
  Runtime -->|required port bind fails| Cleanup[Stop Unix socket and fail startup]
  Relay[Relay dispatcher] --> Admission[Work admission and drain]
  Relay --> Publication[Producer publication]
  Publication --> Writer[Writer settlement]

Loading

_{Reviews (3) · Last reviewed commit: "test(runtime-rpc): require a failed pinn..."}

[greptile-apps]

Failed start retains old identity

If a caller retries start() on the same server after a required-port bind failure, this branch has left mobileSocketWiring in place. The retry creates a fresh pairing identity but reuses wiring that holds the previous registry and keypair. If device or key state changed between attempts, the new listener can authenticate against stale identity state. Clear the wiring when this start fails.

Knowledge Base Used: Remote runtime transport

[greptile-apps]

Shutdown cuts off asset responses

closeAllConnections() closes active HTTP connections, not just idle keep-alive connections. This server also streams web-client assets, so stopping or rebinding it during an asset response now cuts off that download and forces the browser to retry. Consider letting active responses finish within a bounded drain while closing idle connections promptly.

Knowledge Base Used: Remote runtime transport

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[pullfrog]

ℹ️ No critical issues — the extraction is faithful. One behavior note inline and a small coverage gap below.

Reviewed changes

• Extract node-websocket-lifecycle.ts: per-connection lifecycle (attachNodeWebSocketLifecycle), shutdown (stopNodeWebSocketTransport), pre-auth timer clearing, and over-capacity rejection move here. Handlers are read through lazy getters, so a handler registered after start still reaches earlier sockets. The logic is equivalent to the removed handleConnection/stop bodies.
• Extract websocket-transport-limits.ts: message/connection/TCP caps become named exports (1 MiB, 128, 256); values unchanged.
• Opt-in strictPort on WebSocketTransport: binds only the configured port and rethrows on failure instead of falling through to the persisted fallback or port 0.
• requirePinnedWsPort server option: threads strictPort, skips the fallback port, and stops the already-started Unix socket transport before rethrowing so a failed start leaves no orphaned endpoint. Unused in production so far; orcad/serve are the intended consumers.
• Tests: a failed required-port start releases its Unix socket; stop() while an HTTP request is unanswered; strictPort fails closed.

ℹ️ Nitpicks

• The new strictPort/requirePinnedWsPort success path has no positive test — only the failure case and the non-required fallback case are covered. A test that starts a transport on a free configured port and asserts resolvedPort === port would pin the main behavior of the new option.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

stop() now hard-closes in-flight HTTP connections via closeAllConnections(); the implementation this was extracted from only called close() and waited. That is a real behavior change (a useful one — it removes a way for a keep-alive static-web request to wedge shutdown, or the widen rebind that calls current.stop()), but the PR description says there is no behavior change. Worth calling out in the description since it is bundled into the extraction.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: c5f4a2fa-773d-4255-a379-47ae63371160

📥 Commits

Reviewing files that changed from the base of the PR and between 0b812bd and d18d20d.

📒 Files selected for processing (8)

• src/main/runtime/rpc/node-websocket-lifecycle.ts
• src/main/runtime/rpc/websocket-transport-limits.ts
• src/main/runtime/rpc/ws-transport.test.ts
• src/main/runtime/rpc/ws-transport.ts
• src/main/runtime/runtime-rpc-required-ws-port.test.ts
• src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
• src/main/runtime/runtime-rpc/runtime-rpc-pairing-types.ts
• src/main/runtime/runtime-rpc/runtime-rpc-state.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The change extracts Node WebSocket lifecycle handling into shared helpers and defines shared message, WebSocket connection, and TCP connection limits. It adds strict WebSocket port binding and a runtime RPC option that requires the configured port. When required-port startup fails, the runtime stops the Unix socket transport and propagates the error. Tests cover transport shutdown, strict binding, and fallback behavior when a pinned port is optional.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d18d2

The pinned-port option is opt-in, while existing fallback behavior remains available. Required-port failures clean up the Unix transport and preserve the bind error. The change is mergeable subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d18d2

The opt-in setting prevents fallback to an unexpected port, but startup can still succeed without the required listener when identity setup fails. The inspected changes preserve authentication controls; deployment and recovery coverage remain incomplete.

Retained concerns

• Low · reliability · inferred: Required-port mode can report successful startup with only a Unix listener when pairing identity initialization fails. The new bind-failure cleanup branch does not cover this earlier failure, weakening the intended all-or-nothing readiness contract for managed endpoints. Authentication remains unavailable rather than bypassed; this is a failure-containment concern, not a verified security vulnerability.

Security review details

Security Blast Radius

• inferred — The demonstrated attackable surface is the runtime listener and its connection resources on the selected interfaces. Unauthenticated clients can submit frames to the E2EE parser and consume bounded connection capacity. Broader tenant, datastore or environment authority is not established by the supplied evidence.

Security Findings and Attack Paths

• observed — The inspected frame path still enters E2EEChannel before application dispatch. Text and binary application callbacks require an authenticated socket entry, and identity assignment clears the pre-auth timer. The extraction does not bypass these inspected controls.

Trust Boundaries and Controls

• observed — Device identity remains resolved through registry token validation. For relay connections, the resolved device must also match the relay device identity; transport attachment does not itself grant authenticated application access.

Resilience and Maintainability Implications

• inferred — An existing start/stop race can let shutdown snapshot empty transport state while startup awaits binding, then let startup publish listeners afterward. Base/head comparison shows the relevant ownership sequencing predates this PR; changed exposure was not established. Same-instance restart semantics remain unresolved.

Hardening Proposals

• proposed — Define required-listener readiness across identity initialization and binding failures, and serialize startup with shutdown so every pending listener remains covered by its cleanup owner. Explicitly define whether a stopped server instance may restart.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the two main changes: extracting the Node WebSocket lifecycle and adding opt-in pinned-port behavior.
Description check	✅ Passed	The description covers the main template sections, including the plain-language summary, changes, rationale, issue context, visual proof, and testing. It also explains deferred work and includes the a…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}