Skip to content

Canvas SDK: post-merge review followups (PR #1401) #1420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jmoseley wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Canvas SDK: post-merge review followups (PR #1401) #1420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 32 additions & 44 deletions dotnet/src/Canvas.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
* Copyright (c) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------------------------------------------*/

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -95,68 +97,68 @@ public sealed class CanvasHostCapabilities
public sealed class CanvasOpenContext
{
/// <summary>Session that requested the canvas.</summary>
public string SessionId { get; init; } = string.Empty;
public string SessionId { get; set; } = string.Empty;

/// <summary>Owning provider identifier.</summary>
public string ExtensionId { get; init; } = string.Empty;
public string ExtensionId { get; set; } = string.Empty;

/// <summary>Canvas id from the declaring <see cref="CanvasDeclaration"/>.</summary>
public string CanvasId { get; init; } = string.Empty;
public string CanvasId { get; set; } = string.Empty;

/// <summary>Stable instance id supplied by the runtime.</summary>
public string InstanceId { get; init; } = string.Empty;
public string InstanceId { get; set; } = string.Empty;

/// <summary>Validated input payload.</summary>
public JsonElement Input { get; init; }
public JsonElement Input { get; set; }

/// <summary>Host capabilities supplied by the runtime.</summary>
public CanvasHostContext? Host { get; init; }
public CanvasHostContext? Host { get; set; }
}

/// <summary>Context handed to <see cref="ICanvasHandler.OnActionAsync"/>.</summary>
[Experimental(Diagnostics.Experimental)]
public sealed class CanvasActionContext
{
/// <summary>Session that invoked the action.</summary>
public string SessionId { get; init; } = string.Empty;
public string SessionId { get; set; } = string.Empty;

/// <summary>Owning provider identifier.</summary>
public string ExtensionId { get; init; } = string.Empty;
public string ExtensionId { get; set; } = string.Empty;

/// <summary>Canvas id targeted by the action.</summary>
public string CanvasId { get; init; } = string.Empty;
public string CanvasId { get; set; } = string.Empty;

/// <summary>Instance id targeted by the action.</summary>
public string InstanceId { get; init; } = string.Empty;
public string InstanceId { get; set; } = string.Empty;

/// <summary>Action name from <see cref="CanvasAction.Name"/>.</summary>
public string ActionName { get; init; } = string.Empty;
public string ActionName { get; set; } = string.Empty;

/// <summary>Validated input payload.</summary>
public JsonElement Input { get; init; }
public JsonElement Input { get; set; }

/// <summary>Host capabilities supplied by the runtime.</summary>
public CanvasHostContext? Host { get; init; }
public CanvasHostContext? Host { get; set; }
}

/// <summary>Context handed to a canvas's close lifecycle hook.</summary>
[Experimental(Diagnostics.Experimental)]
public sealed class CanvasLifecycleContext
{
/// <summary>Session owning the canvas instance.</summary>
public string SessionId { get; init; } = string.Empty;
public string SessionId { get; set; } = string.Empty;

/// <summary>Owning provider identifier.</summary>
public string ExtensionId { get; init; } = string.Empty;
public string ExtensionId { get; set; } = string.Empty;

/// <summary>Canvas id from the declaring <see cref="CanvasDeclaration"/>.</summary>
public string CanvasId { get; init; } = string.Empty;
public string CanvasId { get; set; } = string.Empty;

/// <summary>Instance id this lifecycle event applies to.</summary>
public string InstanceId { get; init; } = string.Empty;
public string InstanceId { get; set; } = string.Empty;

/// <summary>Host capabilities supplied by the runtime.</summary>
public CanvasHostContext? Host { get; init; }
public CanvasHostContext? Host { get; set; }
}

/// <summary>Structured error returned from canvas handlers.</summary>
Expand All @@ -166,12 +168,12 @@ public sealed class CanvasLifecycleContext
/// in a generic <c>canvas_handler_error</c> envelope.
/// </remarks>
[Experimental(Diagnostics.Experimental)]
public sealed class CanvasError : Exception
public sealed class CanvasException : Exception
{
/// <summary>Initializes a new <see cref="CanvasError"/>.</summary>
/// <summary>Initializes a new <see cref="CanvasException"/>.</summary>
/// <param name="code">Machine-readable error code.</param>
/// <param name="message">Human-readable message.</param>
public CanvasError(string code, string message) : base(message)
public CanvasException(string code, string message) : base(message)
{
Code = code;
}
Expand All @@ -182,13 +184,13 @@ public CanvasError(string code, string message) : base(message)
/// <summary>
/// Default error returned when a custom action has no handler.
/// </summary>
public static CanvasError NoHandler() => new(
public static CanvasException NoHandler() => new(
"canvas_action_no_handler",
"No handler implemented for this canvas action");
}

/// <summary>
/// Internal helpers used by the session runtime to translate <see cref="CanvasError"/>
/// Internal helpers used by the session runtime to translate <see cref="CanvasException"/>
/// (and other handler-thrown exceptions) into structured JSON-RPC error responses.
/// </summary>
internal static class CanvasErrorHelpers
Expand All @@ -203,31 +205,17 @@ public static LocalRpcInvocationException HandlerError(string message) => Build(
"canvas_handler_error",
message);

public static LocalRpcInvocationException ToRpcException(CanvasError error) => Build(error.Code, error.Message);
public static LocalRpcInvocationException ToRpcException(CanvasException error) => Build(error.Code, error.Message);

private static LocalRpcInvocationException Build(string code, string message)
{
var json = JsonSerializer.Serialize(
new CanvasErrorPayload { Code = code, Message = message },
CanvasJsonContext.Default.CanvasErrorPayload);
using var doc = JsonDocument.Parse(json);
return new LocalRpcInvocationException(InternalError, message, doc.RootElement.Clone());
}

internal sealed class CanvasErrorPayload
{
[JsonPropertyName("code")]
public string Code { get; set; } = string.Empty;

[JsonPropertyName("message")]
public string Message { get; set; } = string.Empty;
JsonElement payload = JsonSerializer.SerializeToElement(
new JsonObject { ["code"] = code, ["message"] = message },
TypesJsonContext.Default.JsonObject);
return new LocalRpcInvocationException(InternalError, message, payload);
}
}

[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
[JsonSerializable(typeof(CanvasErrorHelpers.CanvasErrorPayload))]
internal partial class CanvasJsonContext : JsonSerializerContext;

/// <summary>
/// Provider-side canvas lifecycle handler.
/// </summary>
Expand Down Expand Up @@ -259,7 +247,7 @@ public interface ICanvasHandler

/// <summary>
/// Handle a non-lifecycle action declared by the canvas.
/// Default: throws <see cref="CanvasError.NoHandler"/>.
/// Default: throws <see cref="CanvasException.NoHandler"/>.
/// </summary>
Task<object?> OnActionAsync(CanvasActionContext context, CancellationToken cancellationToken);
}
Expand All @@ -284,5 +272,5 @@ public virtual Task OnCloseAsync(CanvasLifecycleContext context, CancellationTok

/// <inheritdoc />
public virtual Task<object?> OnActionAsync(CanvasActionContext context, CancellationToken cancellationToken)
=> Task.FromException<object?>(CanvasError.NoHandler());
=> Task.FromException<object?>(CanvasException.NoHandler());
}
7 changes: 7 additions & 0 deletions dotnet/src/JsonRpc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,13 @@ private async Task HandleIncomingMethodAsync(string methodName, JsonElement mess
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
// `InvokeHandlerAsync` dispatches handlers via reflection
// (`Delegate.DynamicInvoke` / `MethodInfo.Invoke`), which wraps
// any exception thrown inside the user-supplied handler in a
// `TargetInvocationException`. Unwrap so we surface the original
// failure (e.g. `LocalRpcInvocationException`, `CanvasException`)
// to the JSON-RPC error response instead of the reflection
// wrapper.
var actual = ex is TargetInvocationException tie && tie.InnerException != null ? tie.InnerException : ex;
if (_logger.IsEnabled(LogLevel.Debug))
{
Expand Down
18 changes: 9 additions & 9 deletions dotnet/src/Session.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@ internal async ValueTask<CanvasOpenResponse> HandleCanvasOpenAsync(
{
return await handler.OnOpenAsync(ctx, CancellationToken.None).ConfigureAwait(false);
}
catch (CanvasError ce)
catch (CanvasException ce)
{
throw CanvasErrorHelpers.ToRpcException(ce);
}
Expand Down Expand Up @@ -943,7 +943,7 @@ internal async ValueTask HandleCanvasCloseAsync(
{
await handler.OnCloseAsync(ctx, CancellationToken.None).ConfigureAwait(false);
}
catch (CanvasError ce)
catch (CanvasException ce)
{
throw CanvasErrorHelpers.ToRpcException(ce);
}
Expand Down Expand Up @@ -977,7 +977,7 @@ internal async ValueTask<JsonElement> HandleCanvasActionAsync(
var result = await handler.OnActionAsync(ctx, CancellationToken.None).ConfigureAwait(false);
return SerializeActionResult(result);
}
catch (CanvasError ce)
catch (CanvasException ce)
{
throw CanvasErrorHelpers.ToRpcException(ce);
}
Expand All @@ -987,15 +987,15 @@ internal async ValueTask<JsonElement> HandleCanvasActionAsync(
}
}

// Cached JsonElement representing the JSON literal `null`. Hoisted because
// canvas action handlers frequently return null/void, and parsing "null"
// on every call would allocate a fresh JsonDocument.
private static readonly JsonElement NullJsonElement = JsonSerializer.SerializeToElement<object?>(null, TypesJsonContext.Default.Object);

private static JsonElement SerializeActionResult(object? value)
{
var element = CopilotClient.ToJsonElementForWire(value);
if (element.HasValue)
{
return element.Value;
}
using var doc = JsonDocument.Parse("null");
return doc.RootElement.Clone();
return element ?? NullJsonElement;
}
#pragma warning restore GHCP001

Expand Down
2 changes: 2 additions & 0 deletions dotnet/src/Types.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;

namespace GitHub.Copilot;
Expand Down Expand Up @@ -3085,6 +3086,7 @@ public sealed class SystemMessageTransformRpcResponse
[JsonSerializable(typeof(ToolResultObject))]
[JsonSerializable(typeof(JsonElement))]
[JsonSerializable(typeof(JsonElement?))]
[JsonSerializable(typeof(JsonObject))]
[JsonSerializable(typeof(object))]
[JsonSerializable(typeof(Dictionary<string, object>))]
[JsonSerializable(typeof(string[]))]
Expand Down
8 changes: 4 additions & 4 deletions dotnet/test/Unit/CanvasTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,18 @@ public async Task CanvasHandlerBase_DefaultOnClose_Completes()
}

[Fact]
public async Task CanvasHandlerBase_DefaultOnAction_ThrowsNoHandlerCanvasError()
public async Task CanvasHandlerBase_DefaultOnAction_ThrowsNoHandlerCanvasException()
{
var handler = new TestHandler();
var ex = await Assert.ThrowsAsync<CanvasError>(
var ex = await Assert.ThrowsAsync<CanvasException>(
() => handler.OnActionAsync(new CanvasActionContext(), CancellationToken.None));
Assert.Equal("canvas_action_no_handler", ex.Code);
}

[Fact]
public void CanvasError_NoHandler_HasExpectedCode()
public void CanvasException_NoHandler_HasExpectedCode()
{
var err = CanvasError.NoHandler();
var err = CanvasException.NoHandler();
Assert.Equal("canvas_action_no_handler", err.Code);
Assert.False(string.IsNullOrEmpty(err.Message));
}
Expand Down
26 changes: 26 additions & 0 deletions go/canvas.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (

// CanvasDeclaration is the declarative metadata for a single canvas, sent over
// the wire on `session.create` / `session.resume`.
//
// Experimental: CanvasDeclaration is part of an experimental API and may change or be removed.
type CanvasDeclaration struct {
// ID is the canvas identifier, unique within the declaring connection.
ID string `json:"id"`
Expand All @@ -28,6 +30,8 @@ type CanvasDeclaration struct {
}

// CanvasOpenResponse is the response returned from CanvasHandler.OnOpen.
//
// Experimental: CanvasOpenResponse is part of an experimental API and may change or be removed.
type CanvasOpenResponse struct {
// URL the host should render. Optional for canvases with no visual surface.
URL *string `json:"url,omitempty"`
Expand All @@ -38,18 +42,24 @@ type CanvasOpenResponse struct {
}

// CanvasHostContext carries host capability hints passed to canvas provider callbacks.
//
// Experimental: CanvasHostContext is part of an experimental API and may change or be removed.
type CanvasHostContext struct {
// Capabilities describes host feature support relevant to canvases.
Capabilities CanvasHostCapabilities `json:"capabilities"`
}

// CanvasHostCapabilities describes host capability details passed to canvas provider callbacks.
//
// Experimental: CanvasHostCapabilities is part of an experimental API and may change or be removed.
type CanvasHostCapabilities struct {
// Canvases indicates whether the host supports canvas rendering.
Canvases bool `json:"canvases"`
}

// CanvasOpenContext is the context handed to CanvasHandler.OnOpen.
//
// Experimental: CanvasOpenContext is part of an experimental API and may change or be removed.
type CanvasOpenContext struct {
// SessionID is the session that requested the canvas.
SessionID string
Expand All @@ -66,6 +76,8 @@ type CanvasOpenContext struct {
}

// CanvasActionContext is the context handed to CanvasHandler.OnAction.
//
// Experimental: CanvasActionContext is part of an experimental API and may change or be removed.
type CanvasActionContext struct {
// SessionID is the session that invoked the action.
SessionID string
Expand All @@ -84,6 +96,8 @@ type CanvasActionContext struct {
}

// CanvasLifecycleContext is the context handed to a canvas's close lifecycle hook.
//
// Experimental: CanvasLifecycleContext is part of an experimental API and may change or be removed.
type CanvasLifecycleContext struct {
// SessionID is the session owning the canvas instance.
SessionID string
Expand All @@ -102,6 +116,8 @@ type CanvasLifecycleContext struct {
// Wire envelope:
//
// { "code": "<code>", "message": "<message>" }
//
// Experimental: CanvasError is part of an experimental API and may change or be removed.
type CanvasError struct {
// Code is the machine-readable error code.
Code string `json:"code"`
Expand All @@ -115,11 +131,15 @@ func (e *CanvasError) Error() string {
}

// NewCanvasError constructs a new error envelope with the given code and message.
//
// Experimental: NewCanvasError is part of an experimental API and may change or be removed.
func NewCanvasError(code, message string) *CanvasError {
return &CanvasError{Code: code, Message: message}
}

// CanvasErrorNoHandler is the default error returned when a custom action has no handler.
//
// Experimental: CanvasErrorNoHandler is part of an experimental API and may change or be removed.
func CanvasErrorNoHandler() *CanvasError {
return NewCanvasError(
"canvas_action_no_handler",
Expand All @@ -140,6 +160,8 @@ func CanvasErrorNoHandler() *CanvasError {
//
// Embed CanvasHandlerDefaults to inherit no-op defaults for OnClose and a
// "no handler" error for OnAction.
//
// Experimental: CanvasHandler is part of an experimental API and may change or be removed.
type CanvasHandler interface {
OnOpen(ctx context.Context, c CanvasOpenContext) (CanvasOpenResponse, error)
OnClose(ctx context.Context, c CanvasLifecycleContext) error
Expand All @@ -155,6 +177,8 @@ type CanvasHandler interface {
// copilot.CanvasHandlerDefaults
// }
// func (h *myHandler) OnOpen(ctx context.Context, c copilot.CanvasOpenContext) (copilot.CanvasOpenResponse, error) { ... }
//
// Experimental: CanvasHandlerDefaults is part of an experimental API and may change or be removed.
type CanvasHandlerDefaults struct{}

// OnClose returns nil by default.
Expand Down Expand Up @@ -224,6 +248,8 @@ func (p *canvasInvokeParams) toActionContext() CanvasActionContext {

// ExtensionInfo carries stable extension identity for session participants
// that provide canvases.
//
// Experimental: ExtensionInfo is part of an experimental API and may change or be removed.
type ExtensionInfo struct {
// Source is the extension namespace/source, e.g. "github-app".
Source string `json:"source"`
Expand Down
Loading
Loading