Skip to content

feat: feat(cluster-protocol): publish the TypeScript cluster client with full v1 parity#800

Closed
tomdps wants to merge 1 commit into
devfrom
zeroshot/crimson-raven-82
Closed

feat: feat(cluster-protocol): publish the TypeScript cluster client with full v1 parity#800
tomdps wants to merge 1 commit into
devfrom
zeroshot/crimson-raven-82

Conversation

@tomdps

@tomdps tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Closes #748

@tomdps
tomdps enabled auto-merge July 25, 2026 06:19
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown

Greptile Summary

Introduces a published TypeScript client for cluster protocol v1.

  • Adds shared WebSocket request multiplexing, unary methods, cancellable subscriptions, and reconnectable watch streams.
  • Generates protocol types and publishes CommonJS, ESM, and TypeScript declarations through the cluster package subpath.
  • Adds protocol parity, cancellation, reconnect, shared-ID, and packed-package tests.

Confidence Score: 3/5

The PR should not merge until synchronous request-send failures clean up pending state; the unbounded subscription queue should also receive overflow protection.

Calls made on a closed transport register deferred requests before the frame write throws and never remove those entries, while inbound subscription events can accumulate without a client-side capacity bound.

Files Needing Attention: src/cluster/transport/multiplexer.ts and src/cluster/transport/async-queue.ts

Security Review

The new subscription transport accepts an unbounded number of queued notifications from the connected peer, allowing a slow consumer and sustained inbound event stream to exhaust client memory.

Important Files Changed

Filename Overview
src/cluster/transport/multiplexer.ts Implements shared request routing and subscription dispatch, but synchronous send failures retain pending request entries.
src/cluster/transport/async-queue.ts Decouples WebSocket handling from subscription consumers using an unbounded queue that lacks required overflow handling.
src/cluster/subscriptions/watch.ts Implements cursor-based replay and cross-reconnect de-duplication consistently with the documented at-least-once protocol.
src/cluster/connect.ts Creates a shared multiplexer and exposes unary and subscription APIs over one connected WebSocket.
src/cluster/transport/websocket-factory.ts Supports injected WebSocket implementations and a lazy ws fallback on Node versions without a global WebSocket.
package.json Adds the cluster export, build pipeline, runtime WebSocket dependency, and generated package artifacts.

Sequence Diagram

sequenceDiagram
  participant App
  participant Client as ClusterClient
  participant Mux as ConnectionMultiplexer
  participant WS as WebSocket
  App->>Client: unary call / subscribe
  Client->>Mux: allocate shared request ID
  Mux->>WS: JSON-RPC request
  WS-->>Mux: response / notification
  Mux-->>Client: resolve pending call / enqueue event
  Client-->>App: result / async delivery
  App->>Client: cancel or reconnect
  Client->>Mux: cancellation notification or fresh subscription
Loading

Reviews (1): Last reviewed commit: "feat: implement #748 - feat(cluster-prot..." | Re-trigger Greptile

Comment on lines +105 to +108
this.pending.set(id, {kind: 'unary', resolve: deferred.resolve, reject: deferred.reject});
const unregisterAbort = this.registerAbort(id, deferred.reject, signal);

this.sendRequest(id, method, params);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Pending requests survive send failures

When a unary call or subscription is started after the WebSocket leaves the OPEN state, the deferred is added to pending before writeFrame() throws. The failed entry is never removed, so repeated calls on the closed connection retain additional unreachable deferred state for the lifetime of the connection.

Comment on lines +11 to +18
public push(item: T): void {
if (this.closed) return;
const waiter = this.waiting.shift();
if (waiter) {
waiter({value: item, done: false});
return;
}
this.buffered.push(item);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Subscription queue has no bound

Inbound watch, logs, and attach notifications are appended without a capacity or overflow path when the consumer falls behind. Sustained delivery therefore grows the queue with every unread event, increasing memory use and retaining buffered deliveries even after close() while the stream remains referenced.

How this was verified: The notification router forwards peer-supplied events directly to push(), whose fallback unconditionally appends to an uncapped array.

Context Used: AGENTS.md (source)

@tomdps
tomdps added this pull request to the merge queue Jul 25, 2026
@tomdps

tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

External review found another release-blocking P1 plus a security-relevant P2: frame-send failure leaves an unreachable deferred in the transport-wide pending map, and the subscription async queue is unbounded for a slow consumer. The campaign acceptance bar prohibits unresolved P1/P0 findings, and the protocol client must preserve the Rust queue bound/overflow behavior. Closing this attempt for a fresh Zeroshot repair.

@tomdps tomdps closed this Jul 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a manual request Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant