Repository navigation
ts sdk: typed protocol + transports for building cmux-tui frontends - #7886
Conversation
…ends Typed request/response/event definitions for all 50 implemented commands (discriminated unions on cmd/event), Transport abstraction with the existing unix JSON-lines client (wire-identical) and a WebSocketTransport working in browser and node (injectable WebSocket ctor, no ws dependency), typed attachSurface streaming with base64->Uint8Array decode (Buffer-free shared paths). Dual entries: cmux (browser condition safe), cmux/node, cmux/browser. README frontend example. 8 tests incl. compile-checked unions + fake-WS transport contract.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe TypeScript binding is refactored around typed protocol contracts and a shared transport interface. It adds browser WebSocket and Node Unix-socket transports, routed requests and async streams, conditional package exports, runtime-safe Base64 helpers, documentation, and tests. ChangesTypeScript client library
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CmuxClient
participant MessageRouter
participant Transport
CmuxClient->>MessageRouter: Send typed request
MessageRouter->>Transport: Send JSON payload
Transport-->>MessageRouter: Return response or event
MessageRouter-->>CmuxClient: Resolve request or yield stream event
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 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 |
Greptile SummaryThis PR turns the TypeScript binding into a typed SDK for cmux frontends. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "ts sdk: require params in the convenienc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "resized": { | ||
| if (typeof event.replay !== "string") throw new Error("resized replay is not base64 text"); | ||
| return { ...event, replay: decodeBase64(event.replay) } as DecodedAttachEvent; | ||
| } |
There was a problem hiding this comment.
The server sends resized attach payloads in data, but this branch reads replay. A normal surface resize therefore throws resized replay is not base64 text, fails the attach stream, and drops later terminal output.
| case "resized": { | |
| if (typeof event.replay !== "string") throw new Error("resized replay is not base64 text"); | |
| return { ...event, replay: decodeBase64(event.replay) } as DecodedAttachEvent; | |
| } | |
| case "resized": { | |
| if (typeof event.data !== "string") throw new Error("resized data is not base64 text"); | |
| return { ...event, data: decodeBase64(event.data) } as DecodedAttachEvent; | |
| } |
| export interface WaitForRequest extends CmuxRequestBase { | ||
| cmd: "wait-for"; | ||
| surface: IdRef; | ||
| pattern: string; | ||
| /** `0` performs one immediate check. */ | ||
| timeout_ms: number; |
There was a problem hiding this comment.
This type accepts a string IdRef, but the current server deserializes wait-for.surface as a numeric SurfaceId. Calls such as waitFor("surface:1", ...) compile and then fail as bad requests; the same unsupported string shape is exposed for run.pane, notify.surface, list-agents.surface, and report-agent.surface.
| return; | ||
| } | ||
|
|
||
| const key = object.id === undefined ? this.pending.keys().next().value : this.idKey(object.id as Json); |
There was a problem hiding this comment.
When two requests are pending and a response omits id, this fallback assigns it to the oldest map entry rather than its originating command. An out-of-order response can give one caller the wrong result while the intended caller times out; idless responses should only be accepted when exactly one request is pending.
| async next(timeoutMs = this.timeoutMs): Promise<T> { | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
| if (this.buffered.length > 0) return this.buffered.shift()!; | ||
| for (;;) { | ||
| const value = await this.conn.recv(timeoutMs); | ||
| if (typeof value.event !== "string") continue; | ||
| const event = value as T; | ||
| if (event.event === "detached") this.close(); | ||
| if (this.buffered.length > 0) { | ||
| const event = this.buffered.shift()!; | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; | ||
| } | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
|
|
||
| const waiter: StreamWaiter<T> = { |
There was a problem hiding this comment.
If a terminal event is delivered directly to an existing waiter, endsAfterDrain becomes true with an empty buffer, but the stream is not finished. The next next() call registers another waiter and eventually reports a timeout even though the server already ended the stream.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8e4135a. Configure here.
| this.waiters.push(waiter); | ||
| }); | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; |
There was a problem hiding this comment.
Events after detached attach
Medium Severity
attachSurface marks detached as terminal but only closes the stream after the internal buffer drains. If another attach event is pushed in the same transport read (or was already queued behind detached), next() can still yield it after detached, unlike the prior client which stopped the attach connection on detached.
Reviewed by Cursor Bugbot for commit 8e4135a. Configure here.
…ommand has required params (judge note)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cmux-tui/bindings/typescript/src/client.ts`:
- Around line 192-222: Decouple async iteration from the request timeout: update
next() so an omitted timeout skips timer creation rather than defaulting to
this.timeoutMs, while preserving bounded waits when next(ms) is called
explicitly. Update [Symbol.asyncIterator] to call next() without a timeout so
idle subscribe() and attachSurface() streams remain open indefinitely, and
ensure waiter resolution/rejection still clears any timer when one exists.
- Around line 181-266: Replace the O(n) buffered-array dequeue in
CmuxStream.next with a head-index-based queue or small ring buffer, eliminating
buffered.shift() while preserving FIFO ordering and existing drain/finish
behavior. Update push and any empty-queue checks to remain correct as events are
consumed and buffered.
- Around line 286-288: Update the client close lifecycle to track and clean up
streams using dedicated transports. Add an open-stream collection, register each
stream created by openStream(), and remove it from that collection inside the
stream cleanup callback; have close() clean up all tracked streams before
closing the main transport.
- Around line 66-73: The SendOptions declaration manually duplicates protocol
fields; replace it with a type derived from CmuxRequestParams for the "send"
command, matching the existing NewTabOptions and SplitOptions aliases and
preserving the send protocol’s current field behavior.
- Around line 138-151: Update the pending-response handling in the client
message-processing method so messages without an id use the implicit
pending-request path only when exactly one request is in flight. If zero or
multiple entries exist, fail closed by rejecting the message as a protocol error
instead of selecting the first pending key; keep explicit-id responses
unchanged.
In `@cmux-tui/bindings/typescript/src/node-transport.ts`:
- Around line 66-68: Update close() to invoke finish() synchronously before or
as part of destroying the socket, ensuring this.closed is set immediately and
pending sends are handled consistently. Preserve the existing guard against
repeated closes and socket cleanup behavior.
In `@cmux-tui/bindings/typescript/src/websocket-transport.ts`:
- Around line 105-108: Update the private flush() method to handle exceptions
from socket.send() by catching the failure and routing it through fail(),
ensuring errorHandlers are notified. Avoid removing a queued message until its
send succeeds, so messages remain available if sending fails.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 100f525a-b78d-4c4e-9393-7a543733fae0
📒 Files selected for processing (23)
cmux-tui/bindings/typescript/README.mdcmux-tui/bindings/typescript/package.jsoncmux-tui/bindings/typescript/src/base64.tscmux-tui/bindings/typescript/src/browser.tscmux-tui/bindings/typescript/src/client.tscmux-tui/bindings/typescript/src/index.tscmux-tui/bindings/typescript/src/node-client.tscmux-tui/bindings/typescript/src/node-transport.tscmux-tui/bindings/typescript/src/protocol/commands.tscmux-tui/bindings/typescript/src/protocol/common.tscmux-tui/bindings/typescript/src/protocol/events.tscmux-tui/bindings/typescript/src/protocol/index.tscmux-tui/bindings/typescript/src/protocol/tree.tscmux-tui/bindings/typescript/src/transport.tscmux-tui/bindings/typescript/src/types.tscmux-tui/bindings/typescript/src/websocket-transport.tscmux-tui/bindings/typescript/test/base64.test.tscmux-tui/bindings/typescript/test/client.test.tscmux-tui/bindings/typescript/test/protocol-types.tscmux-tui/bindings/typescript/test/unix-transport.test.tscmux-tui/bindings/typescript/test/websocket-transport.test.tscmux-tui/bindings/typescript/tsconfig.jsoncmux-tui/spec/bindings.md
| export type NewTabOptions = CmuxRequestParams<"new-tab">; | ||
| export type NewBrowserTabOptions = Omit<CmuxRequestParams<"new-browser-tab">, "url">; | ||
| export type NewWorkspaceOptions = CmuxRequestParams<"new-workspace">; | ||
| export type NewScreenOptions = CmuxRequestParams<"new-screen">; | ||
| export type SplitOptions = Omit<CmuxRequestParams<"split">, "pane" | "dir">; | ||
| export type SelectOptions = CmuxRequestParams<"select-screen">; | ||
| export type SelectTabOptions = CmuxRequestParams<"select-tab">; | ||
| export interface SendOptions { text?: string | null; bytes?: string | Uint8Array | null } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SendOptions hand-duplicates fields instead of deriving from the protocol type.
Every sibling options alias (NewTabOptions, SplitOptions, etc.) derives from CmuxRequestParams<C>, keeping a single source of truth with the generated protocol. SendOptions is hand-written and can silently drift if the send command's params change.
♻️ Proposed fix
-export interface SendOptions { text?: string | null; bytes?: string | Uint8Array | null }
+export type SendOptions = Omit<CmuxRequestParams<"send">, "surface" | "bytes"> & {
+ bytes?: string | Uint8Array | null;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type NewTabOptions = CmuxRequestParams<"new-tab">; | |
| export type NewBrowserTabOptions = Omit<CmuxRequestParams<"new-browser-tab">, "url">; | |
| export type NewWorkspaceOptions = CmuxRequestParams<"new-workspace">; | |
| export type NewScreenOptions = CmuxRequestParams<"new-screen">; | |
| export type SplitOptions = Omit<CmuxRequestParams<"split">, "pane" | "dir">; | |
| export type SelectOptions = CmuxRequestParams<"select-screen">; | |
| export type SelectTabOptions = CmuxRequestParams<"select-tab">; | |
| export interface SendOptions { text?: string | null; bytes?: string | Uint8Array | null } | |
| export type NewTabOptions = CmuxRequestParams<"new-tab">; | |
| export type NewBrowserTabOptions = Omit<CmuxRequestParams<"new-browser-tab">, "url">; | |
| export type NewWorkspaceOptions = CmuxRequestParams<"new-workspace">; | |
| export type NewScreenOptions = CmuxRequestParams<"new-screen">; | |
| export type SplitOptions = Omit<CmuxRequestParams<"split">, "pane" | "dir">; | |
| export type SelectOptions = CmuxRequestParams<"select-screen">; | |
| export type SelectTabOptions = CmuxRequestParams<"select-tab">; | |
| export type SendOptions = Omit<CmuxRequestParams<"send">, "surface" | "bytes"> & { | |
| bytes?: string | Uint8Array | 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 `@cmux-tui/bindings/typescript/src/client.ts` around lines 66 - 73, The
SendOptions declaration manually duplicates protocol fields; replace it with a
type derived from CmuxRequestParams for the "send" command, matching the
existing NewTabOptions and SplitOptions aliases and preserving the send
protocol’s current field behavior.
| const object = value as Record<string, unknown>; | ||
| if (typeof object.event === "string") { | ||
| for (const handler of this.eventHandlers) handler(object as UnknownEvent); | ||
| return; | ||
| } | ||
|
|
||
| const key = object.id === undefined ? this.pending.keys().next().value : this.idKey(object.id as Json); | ||
| if (key === undefined) return; | ||
| const pending = this.pending.get(key); | ||
| if (!pending) return; | ||
| clearTimeout(pending.timer); | ||
| this.pending.delete(key); | ||
| pending.resolve(object as unknown as CmuxResponse<unknown>); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd bindings.md cmux-tui/spec
rg -n -i -A3 -B3 '\bid\b' cmux-tui/spec/bindings.md | head -100Repository: manaflow-ai/cmux
Length of output: 3741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== client.ts around receive =="
sed -n '120,165p' cmux-tui/bindings/typescript/src/client.ts
echo
echo "== protocol spec around commands/events =="
sed -n '1,220p' cmux-tui/spec/bindings.md
echo
echo "== searches for id-less / optional id / response shape =="
rg -n -i 'optional command id|command id|response|event discriminator|id\\s*:' cmux-tui/spec/bindings.md cmux-tui/bindings/typescript/src/client.tsRepository: manaflow-ai/cmux
Length of output: 15791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== spec tail around binding suite requirements =="
sed -n '260,300p' cmux-tui/spec/bindings.md
echo
echo "== response and command id shapes in TypeScript bindings =="
rg -n -A2 -B2 '"id"|id\?:|id:' cmux-tui/bindings/typescript/src cmux-tui/spec -g '!**/node_modules/**'Repository: manaflow-ai/cmux
Length of output: 27296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate TypeScript protocol types =="
fd -a 'protocol*.ts' cmux-tui/bindings/typescript/src
echo
echo "== search for optional response id fields =="
rg -n -A3 -B3 'interface .*Response|type .*Response|id\?:|id:' cmux-tui/bindings/typescript/srcRepository: manaflow-ai/cmux
Length of output: 10174
Fail closed on messages without id.
cmux-tui/bindings/typescript/src/client.ts:138-151 routes them to this.pending.keys().next().value, which can resolve the wrong in-flight request when more than one call is pending. Only accept this path when there is exactly one pending request; otherwise reject it as a protocol error.
🤖 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 `@cmux-tui/bindings/typescript/src/client.ts` around lines 138 - 151, Update
the pending-response handling in the client message-processing method so
messages without an id use the implicit pending-request path only when exactly
one request is in flight. If zero or multiple entries exist, fail closed by
rejecting the message as a protocol error instead of selecting the first pending
key; keep explicit-id responses unchanged.
| export class CmuxStream<T extends { event: string }> implements AsyncIterable<T> { | ||
| private readonly buffered: T[] = []; | ||
| private readonly waiters: StreamWaiter<T>[] = []; | ||
| private closed = false; | ||
| private endsAfterDrain = false; | ||
|
|
||
| private constructor( | ||
| private readonly conn: JsonLineConnection, | ||
| constructor( | ||
| private readonly timeoutMs: number, | ||
| buffered: T[], | ||
| ) { | ||
| this.buffered = buffered; | ||
| } | ||
|
|
||
| static async open<T extends EventObject>( | ||
| socketPath: string, | ||
| timeoutMs: number, | ||
| request: JsonObject, | ||
| ): Promise<CmuxStream<T>> { | ||
| const conn = await JsonLineConnection.connect(socketPath); | ||
| await conn.send(request); | ||
| const requestId = request.id; | ||
| const buffered: T[] = []; | ||
| for (;;) { | ||
| const value = await conn.recv(timeoutMs); | ||
| if (typeof value.event === "string") { | ||
| buffered.push(value as T); | ||
| continue; | ||
| } | ||
| if (value.id !== requestId) continue; | ||
| const response = value as ResponseEnvelope; | ||
| if (response.ok === true) return new CmuxStream(conn, timeoutMs, buffered); | ||
| throw new CmuxCommandError(response.error || "unknown error", response.id, response); | ||
| } | ||
| } | ||
| private readonly cleanup: () => void, | ||
| ) {} | ||
|
|
||
| async next(timeoutMs = this.timeoutMs): Promise<T> { | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
| if (this.buffered.length > 0) return this.buffered.shift()!; | ||
| for (;;) { | ||
| const value = await this.conn.recv(timeoutMs); | ||
| if (typeof value.event !== "string") continue; | ||
| const event = value as T; | ||
| if (event.event === "detached") this.close(); | ||
| if (this.buffered.length > 0) { | ||
| const event = this.buffered.shift()!; | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; | ||
| } | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
|
|
||
| const waiter: StreamWaiter<T> = { | ||
| active: true, | ||
| resolve: () => undefined, | ||
| reject: () => undefined, | ||
| }; | ||
| const event = await new Promise<T>((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| waiter.active = false; | ||
| reject(new CmuxTimeoutError("stream did not produce an event")); | ||
| }, timeoutMs); | ||
| waiter.resolve = (value) => { | ||
| clearTimeout(timer); | ||
| resolve(value); | ||
| }; | ||
| waiter.reject = (error) => { | ||
| clearTimeout(timer); | ||
| reject(error); | ||
| }; | ||
| this.waiters.push(waiter); | ||
| }); | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; | ||
| } | ||
|
|
||
| close(): void { | ||
| if (!this.closed) { | ||
| this.closed = true; | ||
| this.conn.close(); | ||
| if (this.closed) return; | ||
| this.finish(); | ||
| this.rejectWaiters(new CmuxConnectionError("stream is closed")); | ||
| } | ||
|
|
||
| push(event: T, terminal = false): void { | ||
| if (this.closed) return; | ||
| let delivered = false; | ||
| while (this.waiters.length > 0) { | ||
| const waiter = this.waiters.shift()!; | ||
| if (!waiter.active) continue; | ||
| waiter.resolve(event); | ||
| delivered = true; | ||
| break; | ||
| } | ||
| if (!delivered) this.buffered.push(event); | ||
| if (terminal) this.endsAfterDrain = true; | ||
| } | ||
|
|
||
| fail(error: Error): void { | ||
| if (this.closed) return; | ||
| this.finish(); | ||
| this.rejectWaiters(error); | ||
| } | ||
|
|
||
| async *[Symbol.asyncIterator](): AsyncIterator<T> { | ||
| while (!this.closed) { | ||
| yield await this.next(); | ||
| while (!this.closed) yield await this.next(); | ||
| } | ||
|
|
||
| private finish(): void { | ||
| if (this.closed) return; | ||
| this.closed = true; | ||
| this.cleanup(); | ||
| } | ||
|
|
||
| private rejectWaiters(error: Error): void { | ||
| while (this.waiters.length > 0) { | ||
| const waiter = this.waiters.shift()!; | ||
| if (waiter.active) waiter.reject(error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Array-based FIFO queue in CmuxStream uses O(n) .shift() per dequeue.
buffered is a plain array drained with .shift(). For a high-throughput attachSurface() stream (rapid output events) where the consumer lags the producer, this becomes an O(n²)-style drain cost as the guideline on avoiding unbenchmarked slower algorithms in hot paths warns against.
Consider a head-index-based dequeue (or a small ring buffer) instead of Array.shift() for buffered.
As per path instructions, "flag nested full-collection scans, per-target rescans for batch actions, repeated sort/filter/map work in hot paths... and unbenchmarked algorithm choices for paths expected to handle roughly 1000 workspaces or similar records."
🤖 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 `@cmux-tui/bindings/typescript/src/client.ts` around lines 181 - 266, Replace
the O(n) buffered-array dequeue in CmuxStream.next with a head-index-based queue
or small ring buffer, eliminating buffered.shift() while preserving FIFO
ordering and existing drain/finish behavior. Update push and any empty-queue
checks to remain correct as events are consumed and buffered.
Source: Path instructions
| async next(timeoutMs = this.timeoutMs): Promise<T> { | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
| if (this.buffered.length > 0) return this.buffered.shift()!; | ||
| for (;;) { | ||
| const value = await this.conn.recv(timeoutMs); | ||
| if (typeof value.event !== "string") continue; | ||
| const event = value as T; | ||
| if (event.event === "detached") this.close(); | ||
| if (this.buffered.length > 0) { | ||
| const event = this.buffered.shift()!; | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; | ||
| } | ||
| if (this.closed) throw new CmuxConnectionError("stream is closed"); | ||
|
|
||
| const waiter: StreamWaiter<T> = { | ||
| active: true, | ||
| resolve: () => undefined, | ||
| reject: () => undefined, | ||
| }; | ||
| const event = await new Promise<T>((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| waiter.active = false; | ||
| reject(new CmuxTimeoutError("stream did not produce an event")); | ||
| }, timeoutMs); | ||
| waiter.resolve = (value) => { | ||
| clearTimeout(timer); | ||
| resolve(value); | ||
| }; | ||
| waiter.reject = (error) => { | ||
| clearTimeout(timer); | ||
| reject(error); | ||
| }; | ||
| this.waiters.push(waiter); | ||
| }); | ||
| if (this.endsAfterDrain && this.buffered.length === 0) this.finish(); | ||
| return event; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
for await on subscribe()/attachSurface() throws after any idle gap.
[Symbol.asyncIterator] calls this.next() with no argument, so it always uses the request timeoutMs (default 10s) as the per-read timeout. Any legitimate idle period (sparse subscribe events, quiet terminal) longer than that throws CmuxTimeoutError and ends the iteration — this breaks the primary intended usage of the new typed streaming API. Note a naive fix of passing Infinity/Number.POSITIVE_INFINITY into next() would backfire: Node's setTimeout clamps out-of-range/non-finite delays to ~1ms, so the timer would fire almost immediately instead of never.
Recommend decoupling the iterator's idle-read timeout from the request timeoutMs (e.g. skip creating the timer entirely in next() when no explicit timeout is requested, and have [Symbol.asyncIterator] opt into "no timeout" by default), leaving next(ms) available for callers who want a bounded wait.
Also applies to: 250-252
🤖 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 `@cmux-tui/bindings/typescript/src/client.ts` around lines 192 - 222, Decouple
async iteration from the request timeout: update next() so an omitted timeout
skips timer creation rather than defaulting to this.timeoutMs, while preserving
bounded waits when next(ms) is called explicitly. Update [Symbol.asyncIterator]
to call next() without a timeout so idle subscribe() and attachSurface() streams
remain open indefinitely, and ensure waiter resolution/rejection still clears
any timer when one exists.
| async close(): Promise<void> { | ||
| const conn = await this.connPromise; | ||
| conn.close(); | ||
| this.transport.close(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
close() doesn't close dedicated stream transports, leaking open sockets.
streamTransportFactory-created transports (used by subscribe()/attachSurface() when configured) are only closed via each CmuxStream's own cleanup() — triggered by the stream's own close()/fail()/drain. If a caller calls client.close() while a stream created through a dedicated transport is still open, that transport is never closed.
🛠️ Proposed fix: track open streams for cleanup on client close
private readonly streamTransportFactory?: () => Transport;
+ private readonly openStreams = new Set<{ close(): void }>();
private nextRequestId = 1;
...
async close(): Promise<void> {
+ for (const stream of this.openStreams) stream.close();
this.transport.close();
}And in openStream(), add/remove stream from this.openStreams on creation and inside the cleanup callback.
🤖 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 `@cmux-tui/bindings/typescript/src/client.ts` around lines 286 - 288, Update
the client close lifecycle to track and clean up streams using dedicated
transports. Add an open-stream collection, register each stream created by
openStream(), and remove it from that collection inside the stream cleanup
callback; have close() clean up all tracked streams before closing the main
transport.
| close(): void { | ||
| if (!this.closed) this.socket.destroy(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
close() doesn't set closed synchronously, allowing send() to succeed after close.
close() calls socket.destroy() but relies on the socket's asynchronous close event to set this.closed = true via finish(). Between close() and that event, send() checks this.closed (still false) and either writes to a destroyed socket or silently queues to pending — which finish() later clears without error. Calling this.finish() from close() sets closed synchronously so send() throws immediately.
🔧 Proposed fix
close(): void {
- if (!this.closed) this.socket.destroy();
+ if (this.closed) return;
+ this.socket.destroy();
+ this.finish();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| close(): void { | |
| if (!this.closed) this.socket.destroy(); | |
| } | |
| close(): void { | |
| if (this.closed) return; | |
| this.socket.destroy(); | |
| this.finish(); | |
| } |
🤖 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 `@cmux-tui/bindings/typescript/src/node-transport.ts` around lines 66 - 68,
Update close() to invoke finish() synchronously before or as part of destroying
the socket, ensuring this.closed is set immediately and pending sends are
handled consistently. Preserve the existing guard against repeated closes and
socket cleanup behavior.
| private flush(): void { | ||
| if (this.closed) return; | ||
| while (this.pending.length > 0) this.socket.send(this.pending.shift()!); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled exception in flush() can silently lose queued messages.
flush() is called from the "open" event handler (line 51). If this.socket.send() throws — e.g., the socket closes between the open event and the send call — the exception propagates unhandled through the event listener, errorHandlers are never notified, and messages already shifted from this.pending are permanently lost.
🔒 Proposed fix: wrap flush in try-catch and route to fail()
private flush(): void {
if (this.closed) return;
- while (this.pending.length > 0) this.socket.send(this.pending.shift()!);
+ while (this.pending.length > 0) {
+ try {
+ this.socket.send(this.pending.shift()!);
+ } catch (error) {
+ this.fail(error instanceof Error ? error : new Error(String(error)));
+ return;
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private flush(): void { | |
| if (this.closed) return; | |
| while (this.pending.length > 0) this.socket.send(this.pending.shift()!); | |
| } | |
| private flush(): void { | |
| if (this.closed) return; | |
| while (this.pending.length > 0) { | |
| try { | |
| this.socket.send(this.pending.shift()!); | |
| } catch (error) { | |
| this.fail(error instanceof Error ? error : new Error(String(error))); | |
| return; | |
| } | |
| } | |
| } |
🤖 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 `@cmux-tui/bindings/typescript/src/websocket-transport.ts` around lines 105 -
108, Update the private flush() method to handle exceptions from socket.send()
by catching the failure and routing it through fail(), ensuring errorHandlers
are notified. Avoid removing a queued message until its send succeeds, so
messages remain available if sending fails.
This branch was successfully deployed
1 participant
Turns the TypeScript binding into the client library for third-party cmux-tui frontends (the xterm.js React reference app and eventually the Swift app's web layer consume this): complete typed protocol (all 50 commands + events, discriminated unions mirroring spec/), a Transport abstraction with the wire-identical unix client and a browser+node WebSocketTransport (injected ctor, no hard ws dep), typed attach streaming with Uint8Array VT payloads, browser-safe package exports (cmux / cmux/node / cmux/browser). Conformance TS e2e unchanged and must stay green in CI (local server build blocked by the known host zig issue). 8/8 tests, npm pack clean.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Large client refactor and expanded protocol surface; behavior changes mainly around streaming/multiplexing and attach decoding, though Node zero-arg usage is preserved via a thin wrapper.
Overview
Refactors the cmux TypeScript binding from a Node-only Unix-socket client into a frontend SDK: injectable
Transport, a sharedMessageRouter, andCmuxStreamfor subscribe/attach instead of socket-specific JSON-line I/O.Adds a
protocol/module with discriminatedCmuxRequest/ event unions (full v6 command set), a genericrequest()that infers response types, and many new typed command helpers (layout, agents,run,copy, etc.).attachSurface()now decodesvt-state,output, andresizedpayloads toUint8Arrayvia portable base64 helpers (noBufferin shared code).Ships
UnixSocketTransportandWebSocketTransport(browser global or injected ctor), conditional package exports (cmux,cmux/browser,cmux/node), ESMNodeNextbuild,npm test, and updatedbindings.mdTypeScript contract. Nodenew CmuxClient()/ env socket defaults remain on thecmux/node(and root Node) entry vianode-client.Reviewed by Cursor Bugbot for commit 45c299f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Turns the TypeScript bindings into a typed frontend SDK for
cmuxwith a full protocol, pluggable transports, and browser-safe exports. Keeps the Node API compatible while adding typed streaming attach and a generic, type‑inferred request method that enforces required params.New Features
TransportwithUnixSocketTransport(JSON-lines) andWebSocketTransport(browser + Node via injectedWebSocket).CmuxClient.request(cmd, params)enforces required params;request({ cmd, ... })infers result types. Existing command methods remain.attachSurface()yields a stream;vt-state,output, andresizedpayloads decode toUint8Array.cmux(browser condition),cmux/browser, andcmux/node; no runtime deps.Migration
import { CmuxClient } from "cmux"still works.cmux/nodeis available explicitly. Requires Node 20+.cmuxorcmux/browserwithWebSocketTransport.ws) intoWebSocketTransport.streamTransportFactory(default for Unix sockets).Written for commit 45c299f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests