[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.
|
1 participant
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.