SDK: Align canvas with codegen pipeline, add e2e tests#1413
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1413 · ● 5.3M
There was a problem hiding this comment.
Pull request overview
This PR migrates canvas provider callbacks (canvas.* server→client requests) away from hand-wired dispatch and into the codegen “client session API handlers” pipeline, and adds new canvas E2E coverage across SDKs.
Changes:
- Replace manual canvas request wiring/dispatch with codegen-generated
CanvasHandler/register*ClientSessionApiHandlersplumbing (Node/Python/Go/.NET) and new Rustcanvas_dispatchmodule. - Replace hand-maintained canvas wire-contract types with codegen re-exports / aliases across SDKs.
- Add new canvas E2E tests + a new replay snapshot scenario.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/canvas/canvas_list_discovers_declared_canvases.yaml | Adds replay snapshot baseline for canvas scenario. |
| rust/tests/session_test.rs | Updates provider callback method name to canvas.invokeAction. |
| rust/tests/e2e/canvas.rs | Adds Rust canvas E2E test for listing declared canvases. |
| rust/tests/e2e.rs | Registers new Rust canvas E2E module. |
| rust/src/types.rs | Updates docs to reflect canvas.invokeAction naming. |
| rust/src/session.rs | Routes canvas.* requests to centralized dispatch module. |
| rust/src/lib.rs | Registers new internal canvas_dispatch module. |
| rust/src/generated/api_types.rs | Adds generated canvas rpc method constants and provider request/result types. |
| rust/src/canvas.rs | Replaces manual canvas types with generated aliases/re-exports. |
| rust/src/canvas_dispatch.rs | New internal dispatcher for provider-side canvas.* inbound requests. |
| python/test_canvas.py | Updates unit tests to validate codegen type aliases + adapter wiring. |
| python/e2e/test_canvas_e2e.py | Adds Python canvas E2E coverage (list/open/invoke/close). |
| python/copilot/session.py | Registers codegen canvas handler adapter on sessions. |
| python/copilot/client.py | Removes manual canvas.* request handlers in favor of codegen registration. |
| python/copilot/canvas.py | Switches public canvas context/response types to codegen aliases and trims manual helpers/types. |
| python/copilot/init.py | Updates public exports for new canvas type names/aliases. |
| nodejs/test/e2e/canvas.e2e.test.ts | Adds Node canvas E2E tests (list/open/invoke/close). |
| nodejs/test/client.test.ts | Updates unit tests to use clientSessionApis.canvas instead of direct dispatch helpers. |
| nodejs/src/session.ts | Wires clientSessionApis.canvas adapter from registered canvases. |
| nodejs/src/generated/rpc.ts | Adds codegen canvas provider request/result types + handler registrations. |
| nodejs/src/client.ts | Removes manual connection.onRequest(canvas.*) wiring and type guards; relies on codegen registration. |
| nodejs/src/canvas.ts | Re-exports codegen canvas wire-contract types; removes manual duplicates. |
| go/types.go | Updates canvas method naming; removes ExtensionInfo from session configs. |
| go/session.go | Adds Go canvas client-session adapter implementing generated rpc.CanvasHandler. |
| go/rpc/zrpc.go | Adds generated canvas provider request types + handler registration for canvas.*. |
| go/internal/e2e/canvas_e2e_test.go | Adds Go canvas E2E coverage in a single end-to-end test. |
| go/client.go | Removes manual canvas.* request handlers and stops sending extensionInfo in create/resume. |
| go/canvas.go | Replaces manual canvas wire-contract types with generated aliases; removes old helper structs. |
| go/canvas_test.go | Updates Go unit tests to validate new adapter-based dispatch and JSON round-trip. |
| dotnet/test/Unit/CanvasTests.cs | Updates unit tests to use generated canvas provider types. |
| dotnet/test/E2E/CanvasE2ETests.cs | Adds .NET canvas E2E tests for list/open/invoke/close. |
| dotnet/src/Types.cs | Updates docs and JSON source-gen registration for generated canvas types. |
| dotnet/src/Session.cs | Installs generated canvas handler adapter via ClientSessionApis.Canvas. |
| dotnet/src/Generated/Rpc.cs | Adds generated canvas provider request/result types + ClientSessionApiHandlers.Canvas. |
| dotnet/src/Client.cs | Removes manual canvas.* RPC methods; relies on codegen registration. |
| dotnet/src/Canvas.cs | Removes manual canvas context/response types; updates ICanvasHandler to use generated request/result types. |
Copilot's findings
Files not reviewed (1)
- go/rpc/zrpc.go: Language not supported
Comments suppressed due to low confidence (1)
python/copilot/canvas.py:43
CanvasHostCapabilitieswas removed from the public surface without a backwards-compatible alias. Other SDKs keep the old name as an alias to the generatedCanvasHostContextCapabilitiestype, so existing users importingCanvasHostCapabilitieswill now break. Consider addingCanvasHostCapabilities = CanvasHostContextCapabilitiesand re-exporting it in__all__(andcopilot/__init__.py) to preserve the public API while still using the codegen type.
from .generated.rpc import (
CanvasAction,
CanvasHostContext,
CanvasHostContextCapabilities,
CanvasJsonSchema,
CanvasProviderCloseRequest,
CanvasProviderInvokeActionRequest,
CanvasProviderOpenRequest,
CanvasProviderOpenResult,
OpenCanvasInstance,
)
__all__ = [
"CanvasAction",
"CanvasActionContext",
"CanvasDeclaration",
"CanvasError",
"CanvasHandler",
"CanvasHostContext",
"CanvasHostContextCapabilities",
"CanvasJsonSchema",
"CanvasLifecycleContext",
"CanvasOpenContext",
"CanvasOpenResponse",
"ExtensionInfo",
"OpenCanvasInstance",
]
- Files reviewed: 32/37 changed files
- Comments generated: 7
| async def open(self, params: CanvasProviderOpenRequest) -> CanvasProviderOpenResult: | ||
| try: | ||
| return await self._handler.on_open(params) | ||
| except CanvasError as err: | ||
| raise JsonRpcError(-32603, err.message, data=err.to_envelope()) from err | ||
|
|
||
| async def close(self, params: CanvasProviderCloseRequest) -> None: | ||
| try: | ||
| await self._handler.on_close(params) | ||
| except CanvasError as err: | ||
| raise JsonRpcError(-32603, err.message, data=err.to_envelope()) from err | ||
|
|
||
| async def invoke_action(self, params: CanvasProviderInvokeActionRequest) -> Any: | ||
| try: | ||
| return await self._handler.on_action(params) | ||
| except CanvasError as err: | ||
| raise JsonRpcError(-32603, err.message, data=err.to_envelope()) from err |
There was a problem hiding this comment.
Not addressing — this is a pre-existing behavior and the other SDKs don't wrap unexpected exceptions with a structured envelope either.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1413 · ● 6.5M
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Resolved — the PR now covers all 5 SDKs consistently. The initial commit was Node-only; subsequent commits added C#, Python, Go, and Rust. |
|
Thanks — follow-up commit drops all aliases across all languages and fixes opaque result codegen for C# and Go. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Replace hand-wired canvas provider request handlers with the codegen-generated CanvasHandler interface from the client session API pipeline. - Remove manual canvas.open/close/invokeAction onRequest handlers from client.ts - Remove dispatchCanvasProviderRequest and manual type guards from canvas.ts - Wire canvas dispatch through generated CanvasHandler in session.registerCanvases() - Replace manual CanvasOpenResponse/CanvasHostContext/CanvasOpenContext/ CanvasActionContext/CanvasLifecycleContext/CanvasJsonSchema with re-exports from codegen (single source of truth from runtime Zod schemas) - Add 4 canvas e2e tests: list, open, invokeAction, close lifecycle Co-authored-by: Copilot <[email protected]>
Replace hand-wired canvas RPC handlers with codegen-generated CanvasHandler across all four remaining SDK languages, matching the Node implementation: - Replace manual context/response types with codegen type aliases - Remove hand-wired canvas.open/close/action.invoke handlers - Wire canvas dispatch through ClientSessionApiHandlers - Add canvas e2e tests (list, open, invokeAction, close) Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…(Node) The TypeScript codegen was computing groupExperimental but not using it to add @experimental JSDoc to handler interfaces. C#, Python, and Go already did this correctly. Co-authored-by: Copilot <[email protected]>
Adds 3 new e2e tests matching the Node/C# canvas test coverage: - canvas_open_round_trip: verifies server->client open dispatch and response - canvas_invoke_action_round_trip: verifies action dispatch with input/result - canvas_close_round_trip: verifies close dispatch and list_open cleanup Co-authored-by: Copilot <[email protected]>
- Remove friendly type aliases (CanvasOpenContext, CanvasOpenResponse, etc.)
from all 5 languages; use codegen names (CanvasProviderOpenRequest,
CanvasProviderOpenResult, etc.) directly. Canvas is experimental and
days old, so no backward-compat concern.
- Fix C# codegen: opaque results (x-opaque-json) now emit 'object' instead
of a named wrapper type, preventing double-wrapping of invokeAction result.
- Fix Go codegen: opaque results now emit 'any' instead of a named struct
pointer, preventing the same double-wrapping issue.
- Restore accidentally removed ExtensionInfo from Go SessionConfig,
ResumeSessionConfig, and wire structs.
- Fix Go canvas.close adapter to return nil (null on wire) instead of
empty struct ({} on wire).
- Update all tests (unit + e2e) across all languages.
Co-authored-by: Copilot <[email protected]>
Node: catch CanvasError and generic exceptions in canvas adapter,
throw ResponseError with {code, message} data envelope.
Python: catch generic exceptions (not just CanvasError) in
_CanvasHandlerAdapter, wrap as canvas_handler_error envelope.
All 5 SDKs now consistently produce structured error envelopes
for canvas handler failures.
Co-authored-by: Copilot <[email protected]>
CanvasInvokeActionResult gets collapsed to Any by quicktype when all properties are x-opaque-json. The method emitter tried Any.from_dict() which fails because Any is a typing construct, not a generated class. Add isAnyType check so opaque/Any-typed results skip from_dict and return the raw value directly. Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
93a8fd6 to
ff3c68f
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <[email protected]>
Cross-SDK Consistency Review ✅This PR modifies all four canvas-supporting SDKs (Node.js, Python, Go, .NET) with consistent, parallel changes:
Handler signatures are equivalently updated across all languages, following each language's conventions (async/await in .NET/Python, interface in Go, function type in Node.js). Java has no canvas implementation and is not applicable. No cross-SDK consistency issues found.
|
- Fix Prettier formatting in Node.js SDK (client.ts and other files) - Fix Rust clippy needless_borrow warnings in canvas tests - Fix Rust clippy await_holding_lock warnings by scoping MutexGuards Co-authored-by: Copilot <[email protected]>
Resolved add/add conflicts in canvas E2E test files by keeping this PR's test suite (the 5 tests addressing stephentoub's review feedback: session disposal + nullable fix). PR #1413 (which landed on main) added a different set of canvas E2E tests focused on `session.canvas.list` API; those remain on main without duplication here. Co-authored-by: Copilot <[email protected]>
Main's #1413 aligned canvas with the codegen pipeline. Re-applied followups on top: - Re-applied Experimental markers to hand-written canvas surfaces (Rust doc-comments, Node @experimental JSDoc, Python module/class docstrings, Go per-symbol doc comments). - Re-applied Default derive on CanvasDeclaration in Rust. - Re-applied .NET CanvasError -> CanvasException rename across Canvas.cs, Session.cs, and CanvasTests.cs. - Re-applied CanvasErrorHelpers.Build simplification (JsonObject + TypesJsonContext.Default.JsonObject); removed CanvasErrorPayload and CanvasJsonContext partial. - Re-applied Session.cs NullJsonElement cache and simplified SerializeActionResult. - Updated index.ts re-export to CanvasHostContextCapabilities to match codegen rename. Co-authored-by: Copilot <[email protected]>
Summary
Replaces hand-wired canvas provider request handlers with the codegen-generated
CanvasHandlerinterface from the client session API pipeline. Canvas server→client calls now flow through the same codegen machinery as sessionFs, eliminating manual type definitions and dispatch logic.Ship together with the companion runtime PR that adds canvas to the codegen schema.
What changed
Removed manual dispatch (client.ts)
connection.onRequest(canvas.*)registrationshandleCanvasProviderRequestandhandleCanvasActionInvokeRequestmethodsisCanvasProviderRequestParamsandisCanvasActionInvokeParamstype guardsAll replaced by the generated
registerClientSessionApiHandlerswhich routes canvas calls automatically.Replaced manual types with codegen re-exports (canvas.ts)
6 manually-defined types totaling ~40 fields have been replaced with re-exports from codegen:
CanvasJsonSchemaCanvasHostContextCanvasOpenResponseCanvasProviderOpenResultCanvasOpenContextCanvasProviderOpenRequestCanvasActionContextCanvasProviderInvokeActionRequestCanvasLifecycleContextCanvasProviderCloseRequestZero manual wire-contract type definitions remain. The public export names are preserved as aliases so downstream consumers don't need to change imports.
Simplified handler dispatch (session.ts)
registerCanvases()now populatesclientSessionApis.canvaswith aCanvasHandlerimplementation. Since the context types ARE the codegen request types, handler dispatch passesparamsdirectly — no manual field-by-field destructuring.New e2e tests
4 canvas e2e tests covering the full server→client round-trip:
session.canvas.listBehavioral changes
None. The public API surface (
createCanvas,Canvas, all exported types) is preserved. Theinputfield type on context types changes fromunknownto{ [k: string]: unknown } | undefined(matching the codegen output), which is a compatible narrowing.