Handle tool and permission broadcasts via event model (protocol v3)#686
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Cross-SDK Consistency ReviewThis PR introduces a breaking architectural change to the Node.js SDK by migrating from an RPC callback model to a broadcast event model for handling tool calls and permission requests. This change bumps the protocol version from 2 to 3. 🚨 Critical Inconsistency: Protocol Version MismatchNode.js SDK (this PR):
Python, Go, and .NET SDKs (current state):
ImpactThis creates a breaking inconsistency across the SDK implementations:
RecommendationTo maintain cross-SDK consistency, all four SDKs should be updated together to implement the broadcast event model and bump to protocol version 3: Required changes for Python SDK:
Required changes for Go SDK:
Required changes for .NET SDK:
Alternative ApproachIf updating all SDKs simultaneously is not feasible, consider:
NoteThe PR description mentions: "
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Node SDK to handle external tool calls and permission requests via broadcast session events (instead of per-connection RPC callbacks), aligns the Node client with a breaking runtime protocol change, and adds multi-client E2E coverage for the new behavior.
Changes:
- Switch tool/permission handling to the broadcast event model by intercepting
external_tool.requestedandpermission.requestedinCopilotSessionand responding via new session RPC methods. - Bump Node
SDK_PROTOCOL_VERSIONto 3 and regenerate protocol typings (rpc.ts,session-events.ts) to match updated runtime schemas. - Update/add E2E tests and snapshots, including new multi-client broadcast scenarios.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/client.ts | Removes legacy tool.call / permission.request request handlers and relies on session event dispatch for tool/permission handling. |
| nodejs/src/session.ts | Adds internal handling for broadcast request events and responds via new session RPC methods. |
| nodejs/src/sdkProtocolVersion.ts | Bumps Node SDK protocol version from 2 → 3. |
| nodejs/src/generated/rpc.ts | Adds new session RPC methods for responding to pending tool calls and permission requests; updates some schema fields. |
| nodejs/src/generated/session-events.ts | Regenerates/expands session event types, including new external tool events and updated permission completion payload. |
| nodejs/test/e2e/harness/sdkTestContext.ts | Adds useStdio option to allow TCP-mode tests needed for multi-client scenarios. |
| nodejs/test/e2e/multi-client.test.ts | Adds new multi-client E2E tests validating broadcast semantics for tools and permissions. |
| test/snapshots/tools/*.yaml | Updates tool-related snapshots to reflect new runtime/SDK behavior and outputs. |
| test/snapshots/multi_client/*.yaml | Adds new snapshots for multi-client broadcast scenarios. |
Comments suppressed due to low confidence (1)
nodejs/src/client.ts:1281
tool.callrequest handling (and the privatehandleToolCallRequesthelper) was removed here, but there is still a unit test referencinghandleToolCallRequest(seenodejs/test/client.test.ts). This will fail TypeScript compilation/tests until that test is updated or removed to reflect the new broadcast-event tool handling model.
this.connection.onNotification("session.event", (notification: unknown) => {
this.handleSessionEventNotification(notification);
});
this.connection.onNotification("session.lifecycle", (notification: unknown) => {
this.handleSessionLifecycleNotification(notification);
});
// External tool calls and permission requests are now handled via broadcast events:
// the server sends external_tool.requested / permission.requested as session event
// notifications, and CopilotSession._dispatchEvent handles them internally by
// executing the handler and responding via session.tools.respond / session.permissions.respond RPC.
|
| SDK | Protocol Version | Tool/Permission Pattern | Multi-Client Support |
|---|---|---|---|
| Node.js | 3 ✅ | Broadcast events ✅ | Yes ✅ |
| Python | 2 ❌ | RPC callbacks ❌ | No ❌ |
| Go | 2 ❌ | RPC callbacks ❌ | No ❌ |
| .NET | 2 ❌ | RPC callbacks ❌ | No ❌ |
Key Inconsistencies
1. Protocol Version Mismatch
The Node SDK bumps to protocol version 3, while Python, Go, and .NET remain on version 2. This means:
- Node SDK clients will only work with runtime version 3+
- Other SDKs will only work with runtime version 2
- Users cannot mix SDK languages in multi-client scenarios
Files affected:
- ✅
nodejs/src/sdkProtocolVersion.ts- bumped to 3 - ❌
python/copilot/sdk_protocol_version.py- still 2 - ❌
go/sdk_protocol_version.go- still 2 - ❌
dotnet/src/SdkProtocolVersion.cs- still 2
2. Tool & Permission Handling Pattern
Node SDK now uses broadcast events (external_tool.requested, permission.requested) and responds via new RPC methods, while other SDKs still use the old callback pattern:
Node.js (new pattern):
- Listen for
external_tool.requested/permission.requestedsession events - Respond via
session.tools.handlePendingToolCall/session.permissions.handlePendingPermissionRequestRPC
Python/Go/.NET (old pattern):
- Still register
tool.call/permission.requestRPC request handlers - Respond directly to the RPC request
Files that need updates:
- ❌
python/copilot/client.py:1351-1352- still setstool.callandpermission.requesthandlers - ❌
go/client.go:1289-1290- still setstool.callandpermission.requesthandlers - ❌
dotnet/src/Client.cs:1125-1126- still setstool.callandpermission.requesthandlers
3. Multi-Client Support
The Node SDK now supports multiple clients connecting to the same session (all receive broadcast events). This capability doesn't exist in other SDKs and would require:
- Event handling for broadcast request events
- Coordination of responses from multiple clients
- E2E tests for multi-client scenarios
Files that demonstrate this:
- ✅
nodejs/test/e2e/multi-client.test.ts- 5 new multi-client test cases - ❌ Python, Go, .NET - no multi-client tests or support
4. New Extension Export
Node SDK adds extension.ts export for child process scenarios - unclear if this is needed in other SDKs.
Recommendations
To maintain cross-SDK consistency, the following options should be considered:
Option A: Update all SDKs in parallel
- Port the broadcast event model to Python, Go, and .NET
- Bump all SDKs to protocol version 3 simultaneously
- Add multi-client support and tests across all languages
- Ensures feature parity but requires significant coordination
Option B: Staged rollout with version compatibility
- Document that Node SDK v3 requires runtime v3+
- Plan follow-up PRs to port changes to other SDKs
- Track the migration in a GitHub issue
- Temporary inconsistency is acceptable if tracked and planned
Option C: Keep protocol version 2 support in Node SDK
- Make the Node SDK backward compatible with both v2 and v3 protocols
- Allow gradual migration of other SDKs
- More complex but maintains broader compatibility
Questions for Maintainers
- Is there a timeline for porting these changes to Python, Go, and .NET?
- Should protocol version 3 support be tracked as a feature gap in other SDKs?
- Are there any runtime compatibility considerations that affect the rollout strategy?
- Should this PR be labeled as breaking change / requires coordination across SDKs?
Note: This is a technical consistency review, not a judgment on code quality. The Node SDK implementation looks well-designed - the concern is ensuring the other SDKs evolve in parallel to maintain API parity across languages.
Generated by SDK Consistency Review Agent for issue #686 · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
Cross-SDK Consistency: This protocol version bump to 3 is not reflected in the other SDKs:
- Python:
python/copilot/sdk_protocol_version.py- still at version 2 - Go:
go/sdk_protocol_version.go- still at version 2 - .NET:
dotnet/src/SdkProtocolVersion.cs- still at version 2
This creates a compatibility issue where Node SDK clients will only work with runtime v3+, while other SDK clients remain on v2. Consider coordinating this version bump across all SDKs or documenting the migration plan.
| @@ -0,0 +1,289 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Cross-SDK Consistency: These multi-client E2E tests demonstrate new functionality that doesn't exist in the other SDKs. For feature parity, similar tests should be added to:
python/e2e/- no multi-client tests currentlygo/- no multi-client tests currentlydotnet/test/- no multi-client tests currently
The multi-client capability requires the broadcast event model, so these tests can't be ported until the other SDKs implement the same pattern.
| * rejections, consistent with standard EventEmitter / event handler semantics. | ||
| * @internal | ||
| */ | ||
| private _handleBroadcastEvent(event: SessionEvent): void { |
There was a problem hiding this comment.
Cross-SDK Consistency: This new broadcast event handling pattern for external_tool.requested and permission.requested is Node-only. The other SDKs still use the old RPC callback pattern:
- Python:
client.py:1351-1352- still registerstool.callandpermission.requestRPC handlers - Go:
client.go:1289-1290- same old pattern - .NET:
Client.cs:1125-1126- same old pattern
For consistency, these SDKs would need similar _handleBroadcastEvent implementations that:
- Listen for broadcast session events instead of RPC requests
- Execute handlers locally
- Respond via the new RPC methods (
session.tools.handlePendingToolCall,session.permissions.handlePendingPermissionRequest)
|
|
||
| import { CopilotClient } from "./client.js"; | ||
|
|
||
| export const extension = new CopilotClient({ isChildProcess: true }); |
There was a problem hiding this comment.
Cross-SDK Consistency: This new extension export for child process scenarios is Node-only.
Questions:
- Is this functionality needed in other SDKs?
- Should Python, Go, and .NET provide equivalent child process integration patterns?
- Is this a Node-specific use case or a general SDK feature?
If this is generally useful, consider adding equivalent exports in the other SDKs.
Cross-SDK Consistency ReviewThis PR introduces a significant architectural change to the Node.js SDK, replacing the RPC callback pattern for tool calls and permission requests with a broadcast event model. This is a breaking change (protocol version bumped from 2 → 3) that enables multi-client scenarios where multiple SDK clients can connect to the same CLI session. Summary of Changes (Node.js SDK)Before (Protocol v2): The runtime invoked After (Protocol v3): The runtime broadcasts
This enables:
|
Cross-SDK Consistency ReviewThis PR introduces a significant architectural change that currently only affects the Node.js SDK. The other three SDK implementations (Python, Go, and .NET) still use the old RPC callback pattern and will need similar updates to maintain feature parity. Summary of ChangesArchitecture Change:
Protocol Version: Bumped from 2 → 3 (breaking change) Cross-SDK Impact✅ Node.js/TypeScript (this PR)
❌ Python (
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
Cross-SDK consistency note: The protocol version files in other SDKs still show version 2:
python/copilot/sdk_protocol_version.py:9-SDK_PROTOCOL_VERSION = 2go/sdk_protocol_version.go:7-const SdkProtocolVersion = 2dotnet/src/SdkProtocolVersion.cs:14-private const int Version = 2
These will need to be updated when the broadcast event model is implemented in those SDKs. (Note: These files are auto-generated by update-protocol-version.ts, so the generator script may need updates too.)
| * rejections, consistent with standard EventEmitter / event handler semantics. | ||
| * @internal | ||
| */ | ||
| private _handleBroadcastEvent(event: SessionEvent): void { |
There was a problem hiding this comment.
Cross-SDK consistency note: This broadcast event handling pattern (external_tool.requested and permission.requested) needs to be implemented in the other SDKs:
Python equivalent location: python/copilot/session.py - Add similar event interception in the event dispatch logic
Go equivalent location: go/session.go - Add broadcast event handling
.NET equivalent location: dotnet/src/Session.cs - Add broadcast event handling
Currently, Python uses client.py:1531-1580 for _handle_tool_call_request (RPC callback pattern), Go uses client.go:1309 for handleToolCallRequest, and .NET has similar RPC handler patterns that need to be replaced with this event-based approach.
| } else if (typeof rawResult === "string") { | ||
| result = rawResult; | ||
| } else { | ||
| result = JSON.stringify(rawResult); |
There was a problem hiding this comment.
Cross-SDK consistency note: The new RPC methods session.tools.handlePendingToolCall will need to be added to the generated RPC clients in other SDKs:
- Python: Generated RPC types (likely in a similar location to
nodejs/src/generated/rpc.ts) - Go: Generated RPC client
- .NET: Generated RPC client
This appears to be part of the codegen from runtime schemas, so it may be automatically generated once the other SDKs regenerate their types from the updated schemas.
| } | ||
|
|
||
| if (options.isChildProcess && (options.cliUrl || options.useStdio === false)) { | ||
| throw new Error( |
There was a problem hiding this comment.
Cross-SDK consistency note: These RPC request handler removals from client.ts have equivalents in other SDKs that will need similar cleanup:
-
Python
client.py:1351-1352:self._client.set_request_handler("tool.call", self._handle_tool_call_request) self._client.set_request_handler("permission.request", self._handle_permission_request)
-
Go
client.go:1289-1290:c.client.SetRequestHandler("tool.call", jsonrpc2.RequestHandlerFor(c.handleToolCallRequest)) c.client.SetRequestHandler("permission.request", jsonrpc2.RequestHandlerFor(c.handlePermissionRequest))
-
.NET: Similar handler registration that needs to be removed
These handlers should be removed and replaced with the broadcast event pattern.
| @@ -0,0 +1,289 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Cross-SDK consistency note: Excellent multi-client test coverage! When implementing the broadcast event model in Python, Go, and .NET, similar E2E tests should be added:
- Both clients seeing tool request/completion events
- Permission approval/rejection scenarios with multiple clients
- Tool union semantics (multiple clients with different tools)
- Tool persistence when clients disconnect
These test scenarios will help validate that the broadcast model works correctly across all SDK implementations.
Change verify_protocol_version() from strict equality to >= check. The Copilot CLI v0.0.423 reports protocol version 3, but the v2 RPC callback pattern (tool.call, permission.request) is still supported. Protocol v3 adds broadcast events for multi-client scenarios (github/copilot-sdk#686) which we don't need yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency ReviewThis PR implements a significant architectural change in the Node.js SDK only — replacing the RPC callback pattern for tool calls and permission requests with a broadcast event model. This creates a major inconsistency across the SDK implementations. What Changed in Node.jsThe Node.js SDK now:
Current State in Other SDKsPython, Go, and .NET still use the old RPC callback pattern:
Protocol Version Mismatch
Generated Code is Ready✅ All SDKs already have the generated types for:
RecommendationsTo maintain cross-SDK consistency, the Python, Go, and .NET SDKs need equivalent updates:
This is a breaking protocol change that should ideally be coordinated across all language implementations to maintain feature parity and prevent version fragmentation.
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| ToolResult, | ||
| ToolResultObject, | ||
| TypedSessionLifecycleHandler, | ||
| } from "./types.js"; |
There was a problem hiding this comment.
Good: Removing the old RPC callback types. However, Python, Go, and .NET still reference and use these patterns in their client implementations:
- Python
client.py: Uses_handle_tool_call_requestand_handle_permission_request(lines 1351-1352, 1432-1433) - Go
client.go: UseshandleToolCallRequestandhandlePermissionRequest(lines 1289-1290) - .NET
Client.cs: UsesOnToolCallandOnPermissionRequest(lines 1125-1126)
These will need to be removed in coordination with implementing the broadcast event model.
| * rejections, consistent with standard EventEmitter / event handler semantics. | ||
| * @internal | ||
| */ | ||
| private _handleBroadcastEvent(event: SessionEvent): void { |
There was a problem hiding this comment.
Missing in other SDKs: This new _handleBroadcastEvent implementation is Node.js-specific.
Python, Go, and .NET need equivalent implementations to:
- Listen for
external_tool.requestedandpermission.requestedevents in their_dispatch_event/dispatchEvent/DispatchEventmethods - Execute local tool/permission handlers
- Respond via
handle_pending_tool_call/HandlePendingToolCallandhandle_pending_permission_request/HandlePendingPermissionRequestRPC methods
The generated types are already present in all SDKs, so the implementation should be straightforward.
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
Protocol version inconsistency: This bumps the Node.js SDK to version 3, but Python, Go, and .NET SDKs remain at version 2. This will cause protocol version mismatch errors when different SDK clients try to connect to the same runtime.
Consider coordinating this version bump across all SDKs, or use the protocol version range negotiation to allow v2/v3 coexistence during a transition period.
Cross-SDK Consistency Review: Protocol Version & Event HandlingThis PR implements a significant architectural change to tool and permission handling, but only for the Node.js SDK. This creates a protocol version mismatch and feature gap across the SDK implementations. 📋 Summary of Changes (Node.js SDK)Protocol Evolution:
Old pattern (v2): // Runtime calls SDK directly via RPC
RPC: tool.call → SDK handler → Response
RPC: permission.request → SDK handler → ResponseNew pattern (v3): // Runtime broadcasts events, SDK responds via RPC
Event: external_tool.requested → SDK handler → RPC: session.tools.handlePendingToolCall
Event: permission.requested → SDK handler → RPC: session.permissions.handlePendingPermissionRequest🚨 Consistency Issues Identified1. Protocol Version Mismatch
2. Missing Broadcast Event Handling ImplementationThe new Node.js (this PR): // nodejs/src/session.ts
private _handleBroadcastEvent(event: SessionEvent): void {
if (event.type === "external_tool.requested") {
// Execute handler and respond via RPC
await this.rpc.tools.handlePendingToolCall({...})
}
// ...
}Python (still v2 pattern): # python/copilot/client.py
self._client.set_request_handler("tool.call", self._handle_tool_call_request)
# No broadcast event handling ❌Go (still v2 pattern): // go/client.go
c.client.SetRequestHandler("tool.call", jsonrpc2.RequestHandlerFor(c.handleToolCallRequest))
// No broadcast event handling ❌.NET (still v2 pattern): // dotnet/src/Client.cs
rpc.AddLocalRpcMethod("tool.call", handler.OnToolCall);
// No broadcast event handling ❌📦 What's Updated vs. What's Missing✅ Updated (all SDKs):
❌ Missing (Python, Go, .NET):
💡 RecommendationThe new multi-client E2E tests in this PR demonstrate scenarios where multiple SDK clients interact with the same session. However, those tests will only work correctly if all SDK clients use the same protocol version and event handling model. Options:
📝 Additional Notes
Question for the PR author: Is the plan to update Python, Go, and .NET SDKs in follow-up PRs, or is this intended to be a Node.js-specific change? If the former, it would be helpful to have tracking issues linked in the PR description.
|
…callbacks Update the Node SDK to listen for broadcast events (external_tool.requested, permission.requested) and respond via sharedApi RPC methods (session.tools.handlePendingToolCall, session.permissions.handlePendingPermissionRequest) instead of the old tool.call/permission.request RPC handlers. - Regenerate rpc.ts and session-events.ts from runtime schemas - Add _handleBroadcastEvent to session.ts for event interception - Remove old RPC handlers from client.ts - Add useStdio option to test harness for TCP mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verify that when two SDK clients are connected to the same CLI process, both see external_tool.requested/completed and permission.requested/completed events. The permission test has client 1 manually approve while client 2 observes, confirming the broadcast model works across connections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ent 1 approves Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rtions Add E2E test verifying one client can reject a permission and both clients see the denial in the permission.completed event. Regenerate SDK types from updated runtime schemas so permission.completed includes result.kind enum. Use type-narrowing filters instead of casts in test assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…leanup - two clients register different tools and agent uses both: client1 has city_lookup, client2 has currency_lookup, verifies union semantics - disconnecting client removes its tools: client2's ephemeral_tool removed after disconnect, client1's stable_tool persists - Update existing test: client2 resumes with NO tools (doesn't overwrite) - Regenerated snapshots for existing tests (minor wording changes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Bump SDK_PROTOCOL_VERSION from 2 to 3 to match runtime breaking change - Wrap fallback RPC calls in _executeToolAndRespond and _executePermissionAndRespond with try/catch to prevent unhandled rejections if the connection is disposed mid-flight Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
These are no different from any other event handler — let rejections propagate naturally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove unused session2 variable in multi-client test - Document fire-and-forget semantics in _handleBroadcastEvent - Fix null/undefined handling in tool result serialization - Fix stale RPC method names in comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/handle-broadcasts-for-permissions-and-tools
branch
from
34a8357 to
965b84b
Compare
Cross-SDK Consistency ReviewThis PR introduces a breaking change that migrates from RPC callbacks to a broadcast event model for tool calls and permission requests. However, the implementation is incomplete across SDKs, creating a significant feature parity gap. ✅ Fully Implemented (Protocol v3)
❌ Missing Implementation (Still on Protocol v2)
Required Changes for Python & .NETTo bring Python and .NET into feature parity, they need:
ImpactUntil Python and .NET are updated:
RecommendationEither:
The current state leaves Python and .NET users with a broken SDK when used with the new runtime.
|
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| @@ -0,0 +1,294 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Cross-SDK consistency: This new multi-client E2E test file validates the broadcast event model with multiple connected clients (5 test scenarios). Similar tests exist in Go (go/internal/e2e/multi_client_test.go).
Missing in Python and .NET: No equivalent multi-client tests exist to validate the broadcast model. Consider adding these tests when migrating Python/.NET to protocol v3.
| * rejections, consistent with standard EventEmitter / event handler semantics. | ||
| * @internal | ||
| */ | ||
| private _handleBroadcastEvent(event: SessionEvent): void { |
There was a problem hiding this comment.
Cross-SDK consistency: This new _handleBroadcastEvent method implements the protocol v3 broadcast model for Node.js (similar implementation exists in Go's handleBroadcastEvent).
Missing in Python and .NET: Neither SDK has implemented equivalent broadcast event handling. They still use the old RPC callback pattern (tool.call / permission.request handlers in the client).
To maintain feature parity, Python and .NET need:
- A similar method to intercept and handle
external_tool.requested/permission.requestedevents - Logic to execute local handlers and respond via
session.tools.handlePendingToolCall/session.permissions.handlePendingPermissionRequestRPCs (the generated RPC methods already exist but aren't being used)
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
Cross-SDK consistency: Protocol version bumped to 3 in Node.js and Go, but Python and .NET are still on version 2.
- Python:
python/copilot/sdk_protocol_version.py:9→SDK_PROTOCOL_VERSION = 2 - .NET:
dotnet/src/SdkProtocolVersion.cs:14→Version = 2
These should be updated in sync to avoid protocol mismatch issues when different language SDKs interact with the same runtime.
Cross-SDK Consistency ReviewThis PR implements a major architectural change (protocol v2 → v3) that replaces the RPC callback pattern with a broadcast event model for tool calls and permission requests. However, the changes are only implemented in Node.js and Go, creating a breaking inconsistency with Python and .NET. 🔴 Critical Issues1. Protocol Version MismatchThe protocol version has been bumped to 3 in only 2 out of 4 SDKs:
Files:
2. Broadcast Event Handling Not ImplementedNode.js and Go have added broadcast event handling, but Python and .NET still use the old RPC callback pattern: Node.js & Go additions (✅):
Python & .NET still using old pattern (❌):
3. Missing E2E TestsMulti-client E2E tests were added for Node.js and Go to verify the new broadcast behavior, but not for Python and .NET:
📋 Required Changes for ConsistencyTo bring Python and .NET into parity with Node.js and Go, the following changes are needed: Python (
|
Cross-SDK Consistency ReviewThis PR implements a significant architectural change to handle tool calls and permission requests via a broadcast event model (protocol v3) instead of the RPC callback pattern (protocol v2). However, this feature has only been implemented in Node.js, Go, and .NET, while Python is missing this implementation. What ChangedThe PR replaces the old RPC callback pattern with a broadcast event model where:
Implementation Status by SDK✅ Node.js (
✅ Go (
✅ .NET (
❌ Python (
ImpactThis creates a breaking inconsistency across SDKs:
RecommendationThe Python SDK needs the same changes:
Would you like me to create a follow-up issue to track the Python implementation, or should this PR be updated to include Python changes before merging?
|
| if self._client2: | ||
| try: | ||
| await self._client2.stop() | ||
| except Exception: |
| if self._client1: | ||
| try: | ||
| await self._client1.stop() | ||
| except Exception: |
Cross-SDK Consistency Review ✅ (with one issue)I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Overall, the implementation is highly consistent across all languages! 🎉 What's Consistent ✅The PR successfully implements the protocol v3 broadcast model across all four SDKs:
Critical Issue Found
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #686
| * This must match the version expected by the copilot-agent-runtime server. | ||
| */ | ||
| export const SDK_PROTOCOL_VERSION = 2; | ||
| export const SDK_PROTOCOL_VERSION = 3; |
There was a problem hiding this comment.
sdk-protocol-version.json file at the repository root is still set to version 2, but all SDK implementations are now at version 3.
According to the comment in this file (// Code generated by update-protocol-version.ts. DO NOT EDIT.), these files should be generated from sdk-protocol-version.json. However, it appears the SDK files were manually updated instead.
Recommendation: Update sdk-protocol-version.json to version 3 to match the SDK implementations:
{
"version": 3
}This ensures the source of truth matches the generated files and maintains consistency with the documented code generation process.
| using var sub2 = session2.On(evt => client2Events.Add(evt)); | ||
|
|
||
| // Write a file so the agent has something to edit | ||
| await File.WriteAllTextAsync(Path.Combine(Ctx.WorkDir, "protected.txt"), "protected content"); |
| }); | ||
|
|
||
| // Verify the file was NOT modified | ||
| var content = await File.ReadAllTextAsync(Path.Combine(Ctx.WorkDir, "protected.txt")); |
SteveSandersonMS
changed the title
| catch (Exception ex) | ||
| { | ||
| try | ||
| { | ||
| await Rpc.Tools.HandlePendingToolCallAsync(requestId, result: null, error: ex.Message); | ||
| } | ||
| catch (IOException) | ||
| { | ||
| // Connection lost or RPC error — nothing we can do | ||
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Connection already disposed — nothing we can do | ||
| } | ||
| } |
| catch (Exception) | ||
| { | ||
| try | ||
| { | ||
| await Rpc.Permissions.HandlePendingPermissionRequestAsync(requestId, new PermissionRequestResult | ||
| { | ||
| Kind = PermissionRequestResultKind.DeniedCouldNotRequestFromUser | ||
| }); | ||
| } | ||
| catch (IOException) | ||
| { | ||
| // Connection lost or RPC error — nothing we can do | ||
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Connection already disposed — nothing we can do | ||
| } | ||
| } |
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). This is an excellent example of maintaining feature parity across a multi-language SDK repository. SummaryThis PR implements protocol v3's broadcast event model for tool calls and permission requests, replacing the previous RPC callback pattern. All four SDKs have been updated consistently. Consistency Analysis✅ Core ImplementationAll four SDKs implement the same pattern:
✅ Test CoverageAll four SDKs implement the same 5 multi-client test scenarios:
Test file locations:
✅ Generated CodeAll SDKs regenerated their RPC and session event types from updated schemas:
ConclusionThis PR maintains excellent cross-SDK consistency. The implementation follows language-specific conventions (naming, async patterns, error handling) while keeping the API surface and behavior equivalent across all four SDKs. No inconsistencies or missing features detected. Great work! 🎉
|
…t to Python/Go/.NET (protocol v3)
SteveSandersonMS
force-pushed
the
stevesa/handle-broadcasts-for-permissions-and-tools
branch
from
51ae8be to
31d4acf
Compare
✅ Cross-SDK Consistency ReviewI've analyzed PR #686 for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Here are my findings: SummaryExcellent work! This PR implements the protocol v3 broadcast model consistently across all SDKs with proper feature parity. Key Changes Verified ✓1. Protocol Version BumpAll four SDKs correctly bumped from version 2 to 3:
2. Broadcast Event HandlingAll SDKs implement equivalent broadcast event handlers following their respective naming conventions:
Each handles the same events:
3. New RPC Response MethodsAll SDKs implement the new response methods with semantically equivalent parameters: Tool handling:
Permission handling:
4. Breaking Changes Applied UniformlyAll SDKs removed the old RPC callback handlers:
5. Test Coverage ParityAll SDKs implement the same 5 core multi-client test scenarios:
Language-Specific Conventions ✓The implementations properly respect language idioms:
Recommendation✅ No consistency issues found. This PR is an excellent example of maintaining feature parity across a multi-language SDK. All four implementations follow the same architectural pattern and provide equivalent functionality. The changes maintain cross-SDK consistency while respecting each language's conventions. Automated review by SDK Consistency Agent
|
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Here's my analysis: SummaryThis PR maintains excellent cross-SDK consistency. All four SDKs have been updated to implement the protocol v3 broadcast model for tool calls and permission requests. Key Changes Verified Across All SDKs1. Protocol Version Bump ✅All SDKs consistently bumped to protocol version 3:
2. Broadcast Event Handling ✅All SDKs implement the new broadcast event model consistently:
3. RPC Method Names ✅All SDKs use the correct RPC methods (respecting language conventions):
4. Old RPC Handler Removal ✅
5. Error Handling ✅All SDKs use consistent error fallback values:
6. E2E Test Coverage ✅All SDKs include multi-client E2E tests:
ConclusionThis PR demonstrates exemplary cross-language consistency. The API surface, behavior, and test coverage are parallel across all four SDK implementations, with differences limited to idiomatic language conventions (camelCase vs snake_case vs PascalCase). No consistency issues found. ✅
|
| else: | ||
| self._process.kill() | ||
| self._process = None | ||
| except Exception: |
SteveSandersonMS
force-pushed
the
stevesa/handle-broadcasts-for-permissions-and-tools
branch
from
ce65917 to
8d55ec4
Compare
SteveSandersonMS
deleted the
stevesa/handle-broadcasts-for-permissions-and-tools
branch
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). SummaryThis PR implements protocol v3 with the broadcast event model for tool calls and permission requests. The implementation is highly consistent across all SDKs with only minor differences that are appropriate for each language's idioms. ✅ What's Consistent1. Protocol Version UpdateAll four SDKs bump from v2 → v3:
2. RPC Handler RemovalAll SDKs consistently removed the old RPC request handlers:
3. Broadcast Event HandlingAll SDKs implement broadcast event handling in their Session classes:
All handle the same two event types:
4. RPC Response MethodsAll SDKs use identical RPC method names to respond:
5. Test CoverageAll four SDKs include comprehensive multi-client E2E tests (5 tests each):
All tests share the same snapshot files under 🔍 Minor Differences (Expected & Appropriate)These differences reflect language idioms and best practices:
📊 Implementation Quality
🎯 VerdictNo consistency issues found. This PR maintains excellent cross-SDK consistency while respecting language-specific idioms. The broadcast event model is implemented equivalently across all four languages, and the test coverage ensures behavioral parity. Great work maintaining consistency across this complex multi-SDK change! 🚀
|
Overview
Replace the RPC callback pattern for external tool calls and permission requests with a broadcast event model. The runtime now broadcasts
external_tool.requestedandpermission.requestedas session events to all connected clients, and clients respond viasession.tools.handlePendingToolCallandsession.permissions.handlePendingPermissionRequestRPC methods.Changes
Event-based handling (Node SDK):
tool.callandpermission.requestRPC request handlers fromclient.ts_handleBroadcastEventinsession.tsthat intercepts broadcast request events, executes local handlers, and responds via the new RPC methodsnormalizeToolResult,buildUnsupportedToolResult, and related helper methods that are no longer neededCodegen updates:
rpc.tsandsession-events.tsfrom updated runtime schemaspermission.completedevent now includesresult.kindenum for observing outcomesProtocol version:
SDK_PROTOCOL_VERSIONfrom 2 to 3 to match the runtime breaking changeE2E tests (5 new multi-client tests):
The runtime must include the broadcast model changes and per-connection tool tracking for these tests to pass. Until then, E2E tests that depend on the new protocol (multi-client tests, and any tests using protocol version 3) will fail.