Skip to content

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

Closed
tomdps wants to merge 1 commit into
devfrom
zeroshot/crimson-keep-65
Closed

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

Conversation

@tomdps

@tomdps tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Closes #748

@tomdps
tomdps enabled auto-merge July 25, 2026 04:58
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a publishable TypeScript v1 Cluster Protocol client.

  • Generates and packages the protocol wire types for CommonJS and ESM consumers.
  • Implements unary JSON-RPC calls, multiplexed WebSocket transport, and watch/logs/agent-attach subscriptions.
  • Adds reconnect, cancellation, overflow handling, package smoke tests, and CI checks.

Confidence Score: 3/5

The PR should not merge until watch reconnection uses the fresh connection and request IDs cannot collide between clients sharing a transport.

A real WebSocket reconnect sends the replacement watch over the disconnected transport, while independently constructed clients can also reject valid concurrent calls because their per-client counters produce duplicate IDs in one transport-wide pending map.

Files Needing Attention: src/cluster/watch-subscription.ts, src/cluster/cluster-client.ts, src/cluster/multiplex.ts

Important Files Changed

Filename Overview
src/cluster/watch-subscription.ts Implements deduplicated watch streams, but reconnect mixes the supplied fresh client with the original closed subscription transport.
src/cluster/cluster-client.ts Implements typed unary calls and cancellation, but allocates IDs per client despite transports being shareable.
src/cluster/multiplex.ts Adds request/subscription demultiplexing and bounded queues; its transport-wide pending map exposes per-client request-ID collisions.
src/cluster/websocket-transport.ts Adds the Node/browser WebSocket binding and connects it to the multiplexed transport.
package.json Publishes the cluster subpath and adds generation, build, test, and lifecycle scripts.
scripts/generate-cluster-protocol-ts.js Generates TypeScript protocol projections and supports deterministic drift checking.

Sequence Diagram

sequenceDiagram
    participant App
    participant OldWatch as Original watch transport
    participant FreshClient as Supplied ClusterClient
    participant FreshSocket as Fresh transport
    App->>OldWatch: stream.reconnect(FreshClient)
    OldWatch->>FreshClient: get(atCursor)
    FreshClient->>FreshSocket: JSON-RPC get
    FreshSocket-->>FreshClient: coherent snapshot
    FreshClient-->>OldWatch: snapshot
    OldWatch->>OldWatch: establishWatch(this.transport)
    OldWatch--xOldWatch: send through closed original socket
    OldWatch-->>App: reconnect rejects
Loading

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

runId: this.runId,
fromCursor: snapshot.atCursor ?? this.lastDelivered,
};
return establishWatch(this.transport, params, this.seen);

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 Reconnect reuses closed transport

When the original WebSocket disconnects and the caller supplies a client using a freshly dialed connection, get() uses the fresh transport but establishWatch(this.transport, ...) sends the replacement watch through the original closed socket, causing reconnect() to reject instead of restoring the stream.

Context Used: AGENTS.md (source)


export class ClusterClient {
private readonly transport: JsonRpcTransport;
private nextId = 1;

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 Request IDs collide across clients

When two ClusterClient instances share one transport and have overlapping requests at the same sequence number, both allocate the same ID from their per-client counters. The transport-wide pending map rejects the second valid request as already pending instead of sending it.

@tomdps
tomdps disabled auto-merge July 25, 2026 05:06
@tomdps

tomdps commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

CI and external review found three release-blocking defects: the package architecture test fails because the cluster guard comment names the provider-helper source; reconnect opens the replacement watch on the closed original transport; and per-client request counters collide when clients share one multiplexed transport. Closing this attempt so a fresh Zeroshot run can repair the contract from current dev.

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