Cross-SDK Consistency Review

Scope: PR #1752 — @github/copilot 1.0.64-3 dependency update + codegen regeneration + Node.js handwritten bug fixes.

✅ Generated files — consistent across all SDKs

The regenerated files (rpc.ts, rpc.py, zrpc.go, Rpc.cs, api_types.rs, Java generated types) all correctly reflect the new schema additions (new RPC wrappers for account login/logout, provider model config, etc.). No consistency gaps here.

✅ .NET logging refactors — idiomatic, no cross-SDK action needed

Client.cs and JsonRpc.cs migrate inline logger.LogWarning(...) calls to source-generated [LoggerMessage]-attributed partial methods. This is a .NET-specific best practice and does not need to be mirrored.

⚠️ Node.js handwritten bug fixes — cross-SDK gaps

The changes to nodejs/src/client.ts and nodejs/src/session.ts introduce a _markDisconnected() pattern with two improvements not yet present in the other SDKs:

1. forceStop() now marks in-flight sessions as disconnected

Node.js (new):

// client.ts
for (const session of this.sessions.values()) {
    session._markDisconnected();  // <— NEW: clears all handlers, sets flag
}
this.sessions.clear();

Before this fix, in-flight tool/permission/command handlers could complete after forceStop() and attempt to call rpc.tools.handlePendingToolCall(...) on a closed connection, resulting in error logs. The fix prevents the RPC callbacks from being made at all.

All four SDKs do catch the resulting errors (Python catches ProcessExitedError/OSError, .NET catches ObjectDisposedException/IOException, Go silently ignores goroutine errors, Java logs at WARNING), so this is not a crash — but it generates noisy log output during force-stop, which the Node.js fix eliminates.

2. Guards in broadcast event handler and execution callbacks

Node.js (new):

// session.ts — early return if already disconnected
private _handleBroadcastEvent(event: SessionEvent): void {
    if (this.disconnected) { return; }  // <— NEW
    ...
}

// After tool handler completes, before calling back to server:
if (this.disconnected) { return; }  // <— NEW
await this.rpc.tools.handlePendingToolCall({ requestId, result });

Of the other SDKs:

• Python _handle_broadcast_event does not check _destroyed before dispatching asyncio.ensure_future(self._execute_tool_and_respond(...)). After _destroyed = True is set, a race window exists.
• Go handleBroadcastEvent and executeToolAndRespond have no disconnection check before calling s.RPC.Tools.HandlePendingToolCall(...). Additionally, Disconnect() doesn't clear handlers if the session.destroy RPC call fails (no defer/finally equivalent around the clears).
• .NET _eventChannel.Writer.TryComplete() is called early in DisposeAsync(), which naturally stops new events from being enqueued — this is an effective mitigation for the broadcast path. However, ExecuteToolAndRespondAsync doesn't check _isDisposed before calling Rpc.Tools.HandlePendingToolCallAsync(...) on the success path.
• Java handleBroadcastEventAsync does not check isTerminated before dispatching CompletableFuture.runAsync(task, executor).

Suggested follow-up work

These are not blocking issues for this PR (the codegen update is the primary change), but to maintain parity with the Node.js fix:

1. Python — In force_stop(), set _destroyed = True on each session before clearing self._sessions. Also consider adding a _destroyed check at the top of _handle_broadcast_event.
2. Go — Wrap the handler-clearing block in Disconnect() in a defer so handlers are always cleared even when the RPC fails. Consider a disconnection check in executeToolAndRespond before the response RPC call.
3. Java — In forceStop(), set isTerminated = true on each session (atomically under synchronized) before calling sessions.clear().
4. .NET — The _eventChannel completion already mitigates the broadcast case. Consider adding an if (_isDisposed != 0) return; guard in ExecuteToolAndRespondAsync after the tool result is obtained, similar to the Node.js pattern.

Generated by SDK Consistency Review Agent for issue #1752 · sonnet46 5.5M · ◷