[go] Use types to encode and decode jsonrpc queries#372
[go] Use types to encode and decode jsonrpc queries#372
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Go SDK's JSON-RPC 2.0 implementation to use strongly-typed request and response structs instead of generic map[string]any types. The refactoring introduces generic helper functions for type-safe marshaling/unmarshaling, removes manual parsing logic, and consolidates notification and request handling.
Changes:
- Introduced typed request/response structs for all JSON-RPC methods (session.create, session.send, etc.)
- Added generic helper functions
NotificationHandlerForandRequestHandlerForin the jsonrpc2 package - Removed manual map-based parameter construction and parsing throughout client.go and session.go
- Simplified response handling by directly unmarshaling into typed structs
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| go/types.go | Added internal request/response types; removed JSON tags from public UserInputRequest/Response; changed public response types to unexported; added JSON tags to InfiniteSessionConfig and Tool |
| go/session.go | Refactored Send, GetMessages, Destroy, Abort to use typed structs; replaced manual hook input parsing with direct unmarshaling |
| go/internal/jsonrpc2/jsonrpc2.go | Added generic helpers for type-safe handlers; unified request/notification handling; removed separate Notification type |
| go/client_test.go | Updated test to use new typed toolCallRequest/Response structs |
| go/client.go | Refactored all API methods to use typed requests; fixed copy-paste bug in ResumeSessionWithOptions; renamed dispatchLifecycleEvent to handleLifecycleEvent |
Comments suppressed due to low confidence (2)
go/internal/jsonrpc2/jsonrpc2.go:125
- The reflection logic to detect pointer types has the same issue as in
NotificationHandlerFor:reflect.TypeOf(in)operates on a zero value which could be problematic for interface types. Consider usingreflect.TypeFor[In]()(Go 1.22+) to get the type parameter's type directly, which would be more robust.
func RequestHandlerFor[In, Out any](handler func(params In) (Out, *Error)) RequestHandler {
return func(params json.RawMessage) (json.RawMessage, *Error) {
var in In
// If In is a pointer type, allocate the underlying value and unmarshal into it directly
var target any = &in
if t := reflect.TypeOf(in); t != nil && t.Kind() == reflect.Pointer {
in = reflect.New(t.Elem()).Interface().(In)
target = in
}
go/client.go:875
- The method documentation (line 865) references the old exported type name
PingResponse, but the actual return type has been changed to the unexported*pingResponse. This documentation should be updated to either describe the response without referencing a specific type name (e.g., "Returns a response containing the message and server timestamp") or the type should remain exported to match the documentation and maintain API compatibility.
func (c *Client) Ping(ctx context.Context, message string) (*pingResponse, error) {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all SDK implementations. Good news: This refactoring improves cross-SDK consistency rather than introducing inconsistencies. Summary of ChangesThis PR refactors the Go SDK's JSON-RPC handling from generic Consistency AnalysisCurrent state across SDKs:
Verdict: ✅ No Action NeededThis PR aligns the Go SDK with the type-safe patterns already established in Node.js and .NET. Python's dictionary-based approach is the outlier, but that's intentional—it's idiomatic for Python and still provides type safety through TypedDict annotations. Key benefits of this change:
No follow-up work needed in other SDKs. The change is internal to Go and brings it into alignment with existing patterns.
|
SDK Consistency ReviewI've reviewed PR #372 for cross-SDK consistency. The refactoring to use strongly-typed request/response structs is excellent and aligns with how Node.js and .NET already handle JSON-RPC. However, I found one consistency issue regarding public API design:
|
| SDK | Ping() return type |
Is type exported? |
|---|---|---|
| Python | PingResponse |
✅ Yes (public dataclass) |
| .NET | PingResponse |
✅ Yes (public class) |
| Node.js | Inline type | N/A (inline: { message: string; timestamp: number; ... }) |
| Go (this PR) | pingResponse |
❌ No (changed from public to private) |
Issue: The Go SDK previously exported PingResponse (matching Python/.NET), but this PR makes it private (pingResponse). This creates an inconsistency where Python and .NET users can reference PingResponse in their code (e.g., for type annotations or variable declarations), but Go users cannot.
Example use case: A user might want to store or pass around the ping response:
// Before this PR (works):
var response *copilot.PingResponse
response, err := client.Ping(ctx, "hello")
// After this PR (breaks):
var response *copilot.pingResponse // ❌ Cannot reference unexported typeRecommendation: Consider keeping PingResponse as a public type alias or re-exporting it:
// Option 1: Type alias
type PingResponse = pingResponse
// Option 2: Re-export with documentation
// PingResponse is the response from a ping request
type PingResponse struct {
Message string `json:"message"`
Timestamp int64 `json:"timestamp"`
ProtocolVersion *int `json:"protocolVersion,omitempty"`
}This would maintain backward compatibility and consistency with Python/.NET while keeping the internal types private.
Note: The same concern applies to any other public methods that previously returned exported types (if applicable). I also noticed the documentation comment still references "Returns a PingResponse" which should be updated to match the actual return type.
Overall assessment: The internal refactoring is well-done and brings Go closer to Node.js/.NET's type-safe approach. The visibility change is the only consistency concern I found.
AI generated by SDK Consistency Review Agent
✅ Cross-SDK Consistency ReviewI've reviewed PR #372 for consistency across the Node.js, Python, Go, and .NET SDK implementations. SummaryThis PR maintains cross-SDK consistency. The refactoring to use strongly-typed JSON-RPC structs in Go is a language-appropriate optimization that aligns with idiomatic Go patterns and doesn't create feature parity issues. AnalysisWhat changed:
Cross-language comparison: Each SDK appropriately uses typing patterns idiomatic to its language:
Key points:
RecommendationNo action needed. This refactoring improves Go SDK code quality without affecting cross-SDK consistency. The change makes the Go implementation more aligned with statically-typed best practices, similar to how .NET already operates, while appropriately leaving Python's dynamic approach unchanged.
|
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across the four SDK implementations. No concerns found - this change actually improves cross-SDK consistency. SummaryThis PR refactors the Go SDK's JSON-RPC handling from generic Current State:
Why This Improves ConsistencyThe refactoring eliminates ~468 lines of manual map-building code in favor of direct struct marshaling, which:
Note on PythonPython's use of generic dicts at the JSON-RPC transport layer (in Recommendation: ✅ Approve - this PR brings Go closer to the established patterns in Node.js and .NET.
|
✅ Cross-SDK Consistency ReviewThis PR maintains cross-SDK consistency. Here's my analysis: SummaryThis PR refactors the Go SDK's internal JSON-RPC handling to use strongly-typed structs instead of Key Findings1. No Public API Changes
2. Language-Specific OptimizationThis refactoring doesn't need to be ported to other SDKs because:
3. Test Helper Alignment ✅The PR removes the Go-specific
All SDKs already provide this public method, so removing the redundant test helper actually improves consistency by using the public API that users would use. ConclusionNo consistency issues found. This is a backward-compatible internal improvement that brings the Go SDK's implementation quality in line with other language implementations without affecting the public API surface.
|
Cross-SDK Consistency Review ✅I've reviewed this PR for cross-language SDK consistency. This change improves consistency across the SDK implementations. SummaryThis PR refactors the Go SDK's JSON-RPC handling from using
No Consistency Issues Found
This refactoring is language-appropriate (Go's static typing naturally benefits from explicit structs) and makes the Go SDK's internals more maintainable while preserving feature parity.
|
|
Sorry for the noise. This is ready for review. |
|
Sorry about this, you have a merge conflict! |
…/qmuntal/sdkpriv
Fixed 👍 |
SDK Consistency ReviewI've reviewed this PR for cross-SDK consistency. The changes are focused on the Go SDK and include two main aspects: 1. ✅ Strongly-typed JSON-RPC structs (Good for consistency)The PR refactors Go SDK internals to use strongly-typed structs (e.g., Cross-SDK status:
Recommendation: This change improves Go's type safety and brings it closer to .NET's approach. Consider documenting this as a best practice, though it's less critical for dynamically-typed Python and acceptable for TypeScript which already has structural typing. 2.
|
Cross-SDK Consistency ReviewThank you for improving the Go SDK's type safety and test reliability! I've reviewed this PR for cross-SDK consistency. ✅ Internal Implementation Changes (Go-specific)The JSON-RPC 2.0 refactoring to use strongly-typed structs instead of
|
qmuntal
force-pushed
the
dev/qmuntal/sdkpriv
branch
from
42ad3b2 to
31f5f81
Compare
✅ Cross-SDK Consistency Review: No Issues FoundI've reviewed PR #372 for consistency across the four SDK implementations (Node.js/TypeScript, Python, Go, .NET), and I'm happy to report that this change maintains and actually improves cross-SDK consistency. SummaryThis PR refactors the Go SDK's JSON-RPC client to use strongly-typed request/response structs instead of Cross-SDK ComparisonJSON-RPC Request/Response Type Handling
Consistency Finding: ✅ All four SDKs now treat session JSON-RPC request/response types as internal implementation details, not public API surface.
Public API ConsistencyThe following types remain public in Go (as they should, since they're returned by public methods): ✅ These are also public in all other SDKs:
Benefits of This Change
ConclusionThis PR improves internal code quality without breaking the public API and aligns Go's JSON-RPC handling with .NET's best practices. The public API surface remains consistent across all four SDKs. No action needed from other SDK maintainers. ✨
|
2 participants
This pull request refactors the JSON-RPC 2.0 client and session code to use strongly-typed request and response structs instead of generic
map[string]anytypes. It introduces generic helpers for request and notification handlers, improves type safety, and simplifies marshaling and unmarshaling of parameters and results. The changes also remove manual parsing logic in favor of direct JSON unmarshaling into typed structs.