Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR rewrites the SDK code generation system to generate both session-event types and typed RPC method wrappers across Node.js, Python, Go, and .NET from JSON schema inputs (session-events.schema.json and the new api.schema.json).
Changes:
- Adds a new
scripts/codegenpackage (TS/ESM) with shared schema/file utilities and per-language generators. - Generates new typed RPC wrapper APIs for each language (and wires them into the SDK client/session surfaces as
rpc/Rpcaccessors). - Removes the legacy Node-based session-event generator scripts and updates workflows to verify generated outputs are up to date.
Reviewed changes
Copilot reviewed 21 out of 28 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/utils.ts | Shared schema discovery, post-processing, and file output helpers for codegen. |
| scripts/codegen/typescript.ts | Generates Node session-event TS types and Node typed RPC wrapper factory functions. |
| scripts/codegen/python.ts | Generates Python session-event types and Python typed RPC wrapper classes/types. |
| scripts/codegen/go.ts | Generates Go session-event types and Go typed RPC wrapper types/methods. |
| scripts/codegen/csharp.ts | Generates .NET session-event types and .NET typed RPC wrapper types/methods. |
| scripts/codegen/package.json | New codegen package scripts/deps to run all generators via tsx. |
| scripts/codegen/package-lock.json | Locks codegen package dependencies. |
| scripts/codegen/.gitignore | Ignores codegen-local node_modules/. |
| .github/workflows/codegen-check.yml | Adds CI workflow to run codegen and fail if generated files differ. |
| nodejs/package.json | Replaces generate:session-types with a unified generate that runs the new codegen package. |
| nodejs/src/generated/session-events.ts | Regenerated session-event types with updated header/schema source. |
| nodejs/src/generated/rpc.ts | New generated typed server/session RPC wrapper factories for Node. |
| nodejs/src/client.ts | Adds client.rpc accessor exposing typed server-scoped RPC wrappers. |
| nodejs/src/session.ts | Adds session.rpc accessor exposing typed session-scoped RPC wrappers. |
| nodejs/scripts/generate-session-types.ts | Removes legacy session-event generator script. |
| nodejs/scripts/generate-csharp-session-types.ts | Removes legacy C# session-event generator helper script. |
| python/copilot/generated/session_events.py | Regenerated session-event types with updated header/schema source. |
| python/copilot/generated/rpc.py | New generated typed server/session RPC wrapper classes and dataclasses. |
| python/copilot/client.py | Adds client.rpc accessor and initializes typed server RPC on connect. |
| python/copilot/session.py | Adds session.rpc accessor that lazily constructs typed session RPC. |
| go/generated_session_events.go | Regenerated session-event types with updated header/schema source. |
| go/rpc/generated_rpc.go | New generated typed server/session RPC wrapper types and methods for Go. |
| go/client.go | Adds exported typed server RPC field on Client and initializes on connect. |
| go/session.go | Adds exported typed session RPC field on Session and initializes on construction. |
| dotnet/src/Generated/SessionEvents.cs | Regenerated session-event types with updated header/schema source. |
| dotnet/src/Generated/Rpc.cs | New generated typed server/session RPC wrapper types and methods for .NET. |
| dotnet/src/Client.cs | Adds client.Rpc accessor and initializes typed server RPC on connect. |
| dotnet/src/Session.cs | Adds session.Rpc accessor for typed session-scoped RPC wrappers. |
Files not reviewed (1)
- scripts/codegen/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
python/copilot/client.py:27
SessionRpcis imported but never used in this module. Ruff enables pyflakes (F401) for non-generated code, so this will fail linting; remove the unused import or use it for typing where needed.
from .generated.rpc import ServerRpc
from .generated.session_events import session_event_from_dict
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
SDK Consistency Review SummaryI've reviewed this PR for cross-SDK consistency across the TypeScript, Python, Go, and .NET implementations. This is a substantial codegen rewrite that adds RPC method wrappers for all four SDKs. ✅ What's ConsistentThe PR does an excellent job maintaining consistency in most areas:
|
|
All review feedback addressed - see commit details |
Cross-SDK Consistency ReviewI've reviewed PR #464 for consistency across all four SDK implementations (TypeScript, Python, Go, and .NET). This PR adds RPC code generation and exposes typed RPC methods through ✅ Consistent ImplementationThe PR does an excellent job maintaining consistency across SDKs:
|
Cross-SDK Consistency Review ✅I've reviewed PR #464 for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Excellent work on maintaining consistency! 🎉 SummaryThis PR adds RPC code generation infrastructure to all four SDKs, generating typed wrappers for server and session-scoped RPC methods from a shared ✅ Consistency Verified1. API Surface
2. RPC Methods Server-scoped:
Session-scoped:
3. Naming Conventions
4. Type Definitions // Node.js
interface PingResult { message: string; timestamp: number; protocolVersion: number; }
# Python
`@dataclass`
class PingResult:
message: str
protocol_version: float # snake_case
timestamp: float
// Go
type PingResult struct {
Message string `json:"message"`
ProtocolVersion float64 `json:"protocolVersion"`
Timestamp float64 `json:"timestamp"`
}
// C#
public class PingResult {
[JsonPropertyName("message")] public string Message { get; set; }
[JsonPropertyName("timestamp")] public double Timestamp { get; set; }
[JsonPropertyName("protocolVersion")] public double ProtocolVersion { get; set; }
}5. Async Patterns
6. Error Handling
7. Documentation
📋 Optional EnhancementOne minor observation (not blocking): The Node.js SDK doesn't export RPC types from Users who want TypeScript type checking for RPC parameters/results would need to import directly from the generated file: import type { PingResult, ModelsListResult } from '`@github/copilot-sdk`/generated/rpc';This might be intentional to keep the public API surface minimal, but if you want users to easily access these types, consider adding to export type {
PingResult,
PingParams,
ModelsListResult,
ToolsListResult,
ToolsListParams,
AccountGetQuotaResult,
SessionModelGetCurrentResult,
SessionModelSwitchToResult,
SessionModelSwitchToParams,
} from "./generated/rpc.js";Python, Go, and .NET don't have this concern since:
However, this is a minor style preference and doesn't impact the core functionality or cross-SDK consistency. 🎯 ConclusionThis PR demonstrates exemplary cross-language consistency. All four SDKs:
No blocking consistency issues found. The RPC codegen infrastructure is well-architected and maintains excellent feature parity across all SDK implementations. 🚀
|
✅ SDK Consistency Review: No Issues FoundI've completed a thorough cross-SDK consistency review of this PR. The RPC codegen implementation demonstrates excellent consistency across all four SDKs (Node.js/TypeScript, Python, Go, and .NET). Key Findings:✅ Feature Parity Confirmed:
✅ Appropriate Language Conventions:
✅ Comprehensive Testing:
✅ Previous Feedback Addressed: No action required — this PR maintains strong cross-SDK consistency and is ready from an SDK consistency perspective.
|
Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations. Overall, the RPC codegen work is very well done and maintains excellent feature parity! 🎉 ✅ What's Consistent
|
| } | ||
|
|
||
| /// <summary>Calls "tools.list".</summary> | ||
| public async Task<ToolsListResult> ListAsync(string? model = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK Consistency Note: This method signature differs from Node.js/Python/Go, which require a ToolsListParams object:
// Node.js
client.rpc.tools.list({ model: "claude-sonnet-4.5" })
# Python
client.rpc.tools.list(ToolsListParams(model="claude-sonnet-4.5"))The .NET approach of unpacking to individual parameters is more idiomatic for C#, but creates a difference in developer experience across languages. This pattern also appears in PingAsync(), SwitchToAsync(), etc.
Consider documenting this design choice if it's intentional, especially since future parameter additions might require different API evolution strategies (overloads vs. params objects) across SDKs.
AI generated by SDK Consistency Review Agent for #464
| } | ||
|
|
||
| /// <summary>Calls "ping".</summary> | ||
| public async Task<PingResult> PingAsync(string? message = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cancellation Support: .NET includes CancellationToken cancellationToken = default on all async RPC methods, and Go requires context.Context as the first parameter.
However, Node.js and Python don't have explicit cancellation parameters in the generated RPC methods:
- Node.js methods don't accept
AbortSignal - Python methods don't have cancellation parameters
This means:
- .NET/Go: Users can cancel RPC calls via
CancellationToken/Context - Node.js/Python: No built-in way to cancel RPC calls from the generated API
Consider adding cancellation support to Node.js and Python for consistency, or document this difference if cancellation should be handled at a different layer.
AI generated by SDK Consistency Review Agent for #464
Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). Overall, the implementation maintains excellent consistency with appropriate language-specific conventions. ✅ Consistent ImplementationThe PR successfully adds typed RPC functionality to all four SDKs with:
|
1 participant
This is almost a full rewrite of the per-language codegen. It no longer only emits session event types - it now also emits RPC method wrappers for everything defined by the new
api.schema.jsonthat will soon be added in the@github/copilotpackage.