Add v2 protocol backward compatibility adapters#706
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| return new ToolCallResponseV2(new ToolResultObject | ||
| { | ||
| TextResultForLlm = "Invoking this tool produced an error. Detailed information is not available.", | ||
| ResultType = "failure", | ||
| Error = ex.Message | ||
| }); | ||
| } |
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Excellent work on maintaining feature parity! SummaryThis PR adds v2 protocol backward compatibility adapters uniformly across all SDKs. The implementation is highly consistent, with appropriate language-specific adaptations. Consistency Check ✅Protocol Version Negotiation - All 4 SDKs implement:
V2 Adapter Handlers - All 4 SDKs implement:
Error Handling - All SDKs return consistent error formats:
Language-Specific Adaptations (appropriate differences):
Architecture NotesThe implementation correctly addresses the architectural difference between v2 and v3:
By registering both handler types and determining behavior based on the negotiated version, the SDKs maintain backward compatibility without code duplication. Session-Level SupportNode.js added
This is appropriate—Node.js needed a dedicated v2 adapter, while other SDKs can delegate directly to existing session infrastructure. No Consistency Issues Found 🎉All SDKs implement the same feature set with parallel APIs and appropriate language conventions. The PR maintains excellent cross-SDK consistency!
|
SteveSandersonMS
force-pushed
the
stevesa/protocol-v2-backcompat
branch
from
edf5896 to
d15678c
Compare
Register v2-style tool.call and permission.request JSON-RPC request handlers on all 4 SDKs so that SDK clients written against the v3 API still work at runtime when connected to a v2 CLI server. - Change protocol version negotiation from strict equality to range-based [MIN_PROTOCOL_VERSION(2), SDK_PROTOCOL_VERSION(3)] - Always register v2 handlers unconditionally (v3 servers never send them) - Node/Python/Go/.NET all implement handleToolCallRequestV2 and handlePermissionRequestV2 adapters that invoke the same user-facing tool and permission handlers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/protocol-v2-backcompat
branch
from
d15678c to
b8836bf
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds runtime backward compatibility so SDK clients written against the v3 API can work when connected to a v2 CLI server, without requiring source-level changes from SDK users.
Changes:
- Protocol version negotiation changed from strict equality to range-based
[2, 3]across all 4 SDKs (Node.js, Python, Go, .NET), storing the negotiated version on the client for potential future use - v2 adapter handlers (
tool.callandpermission.requestRPC request handlers) registered unconditionally on all 4 SDKs, translating v2 server requests into calls to the same user-facing tool/permission handlers used by v3 - New internal types to support the v2 wire protocol response format in all 4 SDKs
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
python/copilot/client.py |
Range-based version negotiation, v2 adapter handlers registered in both stdio and TCP code paths, new _handle_tool_call_request_v2 and _handle_permission_request_v2 methods |
nodejs/src/session.ts |
New _handlePermissionRequestV2 method on CopilotSession to delegate v2 permission requests to the existing permissionHandler |
nodejs/src/client.ts |
Range-based version negotiation, v2 onRequest handler registrations, handleToolCallRequestV2, handlePermissionRequestV2, and normalizeToolResultV2 helper methods, plus new internal types |
go/client.go |
Range-based version negotiation, v2 adapter handler registration, handleToolCallRequestV2 and handlePermissionRequestV2 methods, and new v2 request/response struct types |
dotnet/src/Client.cs |
Range-based version negotiation (method changed to instance method), v2 RPC method registrations, OnToolCallV2 and OnPermissionRequestV2 on RpcHandler, new ToolCallResponseV2 and PermissionRequestResponseV2 record types, new JsonSerializable annotations |
dotnet/src/Client.cs
Outdated
| { | ||
| throw new InvalidOperationException( | ||
| $"SDK protocol version mismatch: SDK expects version {expectedVersion}, " + | ||
| $"SDK protocol version mismatch: SDK expects version {MinProtocolVersion}-{maxVersion}, " + |
There was a problem hiding this comment.
The error message when the server doesn't report a protocol version uses "SDK expects version 2-3", while the error message for an out-of-range version uses "SDK supports versions 2-3". The word "expects" is inconsistent with the range-based semantics. Both should use "SDK supports versions 2-3" for consistency.
| $"SDK protocol version mismatch: SDK expects version {MinProtocolVersion}-{maxVersion}, " + | |
| $"SDK protocol version mismatch: SDK supports versions {MinProtocolVersion}-{maxVersion}, " + |
dotnet/src/Client.cs
Outdated
|
|
||
| // Protocol v2 backward-compatibility response types | ||
| internal record ToolCallResponseV2( | ||
| ToolResultObject? Result); |
There was a problem hiding this comment.
The ToolCallResponseV2 record declares the Result property as ToolResultObject? (nullable). However, every code path in OnToolCallV2 always returns a non-null ToolResultObject. The nullable annotation is misleading. Using a non-nullable ToolResultObject Result would better express the intent that a result is always present, consistent with how PermissionRequestResponseV2 declares its PermissionRequestResult Result as non-nullable.
| ToolResultObject? Result); | |
| ToolResultObject Result); |
dotnet/src/Client.cs
Outdated
| var session = client.GetSession(sessionId); | ||
| if (session == null) | ||
| { | ||
| return new PermissionRequestResponseV2(new PermissionRequestResult | ||
| { | ||
| Kind = PermissionRequestResultKind.DeniedCouldNotRequestFromUser | ||
| }); | ||
| } |
There was a problem hiding this comment.
There's an inconsistency between OnToolCallV2 and OnPermissionRequestV2 in their handling of an unknown sessionId. OnToolCallV2 throws ArgumentException (resulting in an RPC error response), while OnPermissionRequestV2 returns a graceful DeniedCouldNotRequestFromUser denial result for the same scenario. The Node.js and Python adapters both return an error for an unknown session in both cases. For consistency, OnPermissionRequestV2 should also throw (or return an error response) when the session is not found, rather than silently denying the request.
| var session = client.GetSession(sessionId); | |
| if (session == null) | |
| { | |
| return new PermissionRequestResponseV2(new PermissionRequestResult | |
| { | |
| Kind = PermissionRequestResultKind.DeniedCouldNotRequestFromUser | |
| }); | |
| } | |
| var session = client.GetSession(sessionId) ?? throw new ArgumentException($"Unknown session {sessionId}"); |
python/copilot/client.py
Outdated
| ) | ||
|
|
||
| try: | ||
| import inspect |
There was a problem hiding this comment.
The import inspect statement on line 1574 is placed inside the function body (inside a try block), instead of at the top of the module. All other Python files in the codebase that use inspect import it at the top of the module (session.py:9, jsonrpc.py:9, tools.py:10). This deferred import is unnecessary and inconsistent — inspect is a standard library module and there's no reason to defer its import here. It should be moved to the top-level imports of client.py.
python/copilot/client.py
Outdated
| try: | ||
| perm_request = PermissionRequest.from_dict(permission_request) | ||
| result = await session._handle_permission_request(perm_request) | ||
| return {"result": {"kind": result.kind}} |
There was a problem hiding this comment.
The Python _handle_permission_request_v2 method returns only {"result": {"kind": result.kind}}, dropping additional fields from PermissionRequestResult such as rules, feedback, message, and path. In contrast, the Go adapter (handlePermissionRequestV2) serializes the full PermissionRequestResult struct (including Kind and Rules) into the response. If the v2 server protocol uses these extra fields (e.g., rules), the Python adapter's response will be incomplete. The response object should include all fields from the result, not just kind, to be consistent with the Go adapter and fully convey the handler's decision.
| return {"result": {"kind": result.kind}} | |
| # Prefer full serialization of the permission request result when available. | |
| if hasattr(result, "to_dict"): | |
| result_payload = result.to_dict() | |
| else: | |
| # Fallback to the minimal payload to preserve existing behavior. | |
| result_payload = {"kind": result.kind} | |
| return {"result": result_payload} |
go/client.go
Outdated
|
|
||
| if pingResult.ProtocolVersion == nil { | ||
| return fmt.Errorf("SDK protocol version mismatch: SDK expects version %d, but server does not report a protocol version. Please update your server to ensure compatibility", expectedVersion) | ||
| return fmt.Errorf("SDK protocol version mismatch: SDK expects version %d-%d, but server does not report a protocol version. Please update your server to ensure compatibility", minProtocolVersion, maxVersion) |
There was a problem hiding this comment.
The error message when the server doesn't report a protocol version uses "SDK expects version 2-3", while the error message for an out-of-range version uses "SDK supports versions 2-3". The word "expects" is inconsistent with the range-based semantics and the phrasing used in the other message. Both error messages should use "SDK supports versions 2-3" for consistency.
| return fmt.Errorf("SDK protocol version mismatch: SDK expects version %d-%d, but server does not report a protocol version. Please update your server to ensure compatibility", minProtocolVersion, maxVersion) | |
| return fmt.Errorf("SDK protocol version mismatch: SDK supports versions %d-%d, but server does not report a protocol version. Please update your server to ensure compatibility", minProtocolVersion, maxVersion) |
python/copilot/client.py
Outdated
| f"but server does not report a protocol version. " | ||
| f"Please update your server to ensure compatibility." | ||
| "SDK protocol version mismatch: " | ||
| f"SDK expects version {MIN_PROTOCOL_VERSION}-{max_version}" |
There was a problem hiding this comment.
The error message when the server doesn't report a protocol version uses "SDK expects version 2-3", while the error message for an out-of-range version uses "SDK supports versions 2-3". The word "expects" is inconsistent with the range-based semantics and the phrasing used in the other message. Both should use "SDK supports versions 2-3" for consistency.
| f"SDK expects version {MIN_PROTOCOL_VERSION}-{max_version}" | |
| f"SDK supports versions {MIN_PROTOCOL_VERSION}-{max_version}" |
nodejs/src/client.ts
Outdated
| if (serverVersion === undefined) { | ||
| throw new Error( | ||
| `SDK protocol version mismatch: SDK expects version ${expectedVersion}, but server does not report a protocol version. ` + | ||
| `SDK protocol version mismatch: SDK expects version ${MIN_PROTOCOL_VERSION}-${maxVersion}, but server does not report a protocol version. ` + |
There was a problem hiding this comment.
The error message when the server doesn't report a protocol version uses "SDK expects version 2-3", while the error message for an out-of-range version uses "SDK supports versions 2-3". The word "expects" is inconsistent with the range-based semantics. Both should use "SDK supports versions 2-3" for consistency.
| `SDK protocol version mismatch: SDK expects version ${MIN_PROTOCOL_VERSION}-${maxVersion}, but server does not report a protocol version. ` + | |
| `SDK protocol version mismatch: SDK supports versions ${MIN_PROTOCOL_VERSION}-${maxVersion}, but server does not report a protocol version. ` + |
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all 4 SDK implementations (Node.js/TypeScript, Python, Go, and .NET). The changes maintain excellent cross-SDK consistency for the v2 protocol backward compatibility feature. What Was ChangedThis PR adds backward compatibility so SDK clients written against the v3 API can work with v2 CLI servers. The implementation is consistent across all 4 SDKs. Consistency Verification✅ Protocol Version NegotiationAll SDKs consistently implement range-based version negotiation:
✅ V2 Adapter HandlersAll SDKs register the same v2 backward compatibility handlers:
The registration happens unconditionally in all SDKs (before protocol negotiation), with consistent explanatory comments explaining that v3 servers will never invoke these handlers. ✅ Adapter Implementation LogicThe v2 adapter implementations follow the same pattern across all SDKs:
✅ Comments and DocumentationAll SDKs include consistent comments explaining:
ConclusionNo consistency issues found. This PR maintains excellent cross-SDK feature parity. All 4 language implementations:
The changes are well-coordinated and maintain the SDK's commitment to providing a consistent API experience across all supported languages.
|
- Use 'supports versions' consistently in all error messages (all SDKs) - Move 'import inspect' to top of module (Python) - Return full PermissionRequestResult fields, not just kind (Python) - Change bare catch to catch (Exception) (C#) - Make ToolCallResponseV2.Result non-nullable (C#) - Throw for unknown session in OnPermissionRequestV2 (C#) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| catch (Exception) | ||
| { | ||
| return new PermissionRequestResponseV2(new PermissionRequestResult | ||
| { | ||
| Kind = PermissionRequestResultKind.DeniedCouldNotRequestFromUser | ||
| }); | ||
| } |
✅ Cross-SDK Consistency ReviewI've reviewed PR #706 for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Great work! The v2 backward compatibility feature has been implemented consistently across all SDKs. Key Consistency Findings✅ Protocol Version ConstantsAll SDKs define the minimum protocol version as
✅ Negotiated Version StorageAll SDKs store the negotiated protocol version in a nullable/optional integer field:
✅ Range-Based Version NegotiationAll SDKs verify the server version is within ✅ V2 Adapter Handlers - Unconditional RegistrationAll SDKs unconditionally register
All include the same explanatory comment:
✅ Adapter Implementation StructureAll SDKs follow the same pattern:
Architectural Note (Not an Issue)The v2 handler registration occurs at different layers in each SDK, reflecting language-specific design patterns:
This is expected and appropriate — each SDK uses the architecture that's idiomatic for that language while maintaining functional consistency. ConclusionThis PR successfully maintains feature parity and consistent API design across all four SDK implementations. The backward compatibility mechanism is implemented uniformly, with clear documentation and appropriate handling for both v2 and v3 protocol versions. Note: There are separate code quality review comments from automated reviewers about exception handling, error message wording, and import placement. Those are orthogonal to cross-SDK consistency and should be addressed independently.
|
Summary
Adds runtime backward compatibility so SDK clients written against the v3 API still work when connected to a v2 CLI server.
Changes
What's NOT included
Testing
Verified tools and permissions e2e tests pass across all 4 SDKs on both v3 (current CLI) and v2 (@github/copilot 0.0.420). The only known difference is the handles_tool_calling_errors test on v2 in Node/Go due to the server-side error formatting difference noted above.