Skip to content

C# SDK: re-land x-opaque-json → JsonElement mapping with object boundary at RPC params#1359

Draft
SteveSandersonMS wants to merge 18 commits into
mainfrom
stevesandersonms/stevesa-fallback-on-jsonelement
Draft

C# SDK: re-land x-opaque-json → JsonElement mapping with object boundary at RPC params#1359
SteveSandersonMS wants to merge 18 commits into
mainfrom
stevesandersonms/stevesa-fallback-on-jsonelement

Conversation

@SteveSandersonMS
Copy link
Copy Markdown
Contributor

@SteveSandersonMS SteveSandersonMS commented May 21, 2026

What this PR does

1. Codegen: x-opaque-jsonJsonElement (scripts/codegen/csharp.ts)

The runtime schema generator now stamps x-opaque-json: true on every field/type that legitimately holds free-form JSON (tool args, hook payloads, telemetry, form schemas, etc.) and rejects everything else at build time. The codegen consumes that marker:

  • DTO fields with x-opaque-json: trueJsonElement / JsonElement? (was object?).
  • All bare-object fallbacks removed from the codegen; reaching an unmappable shape is now a hard error.
  • Hand-written types that round-trip opaque JSON (ToolInvocation.Arguments, hook in/out, related serialization plumbing in Session.cs, SessionFsProvider.cs, Client.cs) switched to JsonElement on the wire. New TestJsonContext for tests.

2. RPC-boundary special-case (the bit that made #1343 painful)

The reverted PR forced consumers to pass JsonElement for outbound RPC method parameters (e.g. Rpc.Tools.HandlePendingToolCallAsync(... JsonElement result, ...)). That's worse UX than the old object? surface: callers naturally hold a typed CLR value (string, dictionary, generated DTO) and don't want to serialize before each call.

Fixed by introducing an asymmetric boundary:

  • Inbound opaque-JSON (events, hook inputs, tool args, generated event DTOs, DTO fields) stays JsonElement(?) — consumers can introspect the wire payload directly.

  • Outbound opaque-JSON at top-level RPC method parameters now accepts object/object?. The codegen generates a call to a new helper:

    public static JsonElement? ToJsonElementForWire(object? value)

    which short-circuits if value is already a JsonElement, otherwise serializes the runtime type using SerializerOptionsForMessageFormatter (the chained source-gen JsonSerializerContexts the SDK uses for all JSON-RPC traffic). This matches pre-revert serialization exactly.

    DTO field types stay JsonElement(?) — the object only appears at the public RPC surface. A generated call site looks like:

    public async Task<HandlePendingToolCallResult> HandlePendingToolCallAsync(
        string requestId, object? result = null, string? error = null, ...)
    {
        var request = new HandlePendingToolCallRequest {
            ..., Result = CopilotClient.ToJsonElementForWire(result), ...
        };
        ...
    }
  • ToolResultObject.ToolTelemetry reverted to IDictionary<string, object>? for the same reason (it's a consumer-constructed outbound type). Tests updated.

Validation

  • dotnet build clean across net472 / net8.0 / net10.0.
  • 98 (net8.0) + 91 (net472) unit tests pass; failures match baseline (4 ConnectionTokenTests rely on the test-harness proxy subprocess and aren't relevant to this change).
  • TS / Python / Go / Rust codegens regenerate cleanly against the updated runtime schema.

Dependencies

This PR requires runtime changes to be merged first (or for the runtime's regenerated generated/*.schema.json to ship a copy). The branch already contains regenerated artifacts from the runtime PR's schema; once the runtime merges and re-emits, no codegen changes will be needed here.

Comment thread scripts/codegen/csharp.ts Fixed
Comment thread scripts/codegen/csharp.ts Fixed
Comment thread scripts/codegen/csharp.ts Fixed
Comment thread scripts/codegen/csharp.ts Fixed
Comment thread dotnet/src/Client.cs Outdated
{
null => null,
JsonElement je => je,
_ => JsonSerializer.SerializeToElement(value, value.GetType(), SerializerOptionsForMessageFormatter)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid the need for the suppress messages by passing in a JsonTypeInfo instead of Type, e.g. value.GetType() => SerializerOptionsForMessageFormatter.GetTypeInfo(value.GetType())

SteveSandersonMS and others added 12 commits May 21, 2026 15:35
C# codegen now hard-errors instead of falling back to `object` for
unmappable schema nodes. Schemas tagged with `x-opaque-json: true` map
to `JsonElement`/`JsonElement?`. Hand-written types that round-trip
arbitrary JSON (ToolInvocation.Arguments, ToolResultObject.ToolTelemetry,
hook *Input* ToolArgs/ToolResult) are retyped to JsonElement to match
the generated wire types. Boundary code in Session/Client/SessionFsProvider
converts between the hand-written user-facing object?-typed dictionaries
(ElicitationSchema.Properties, ElicitationResult.Content) and the wire
JsonElement form.

This is the SDK side of github/copilot-agent-runtime#8375 — replays the
non-Generated/* changes from the reverted PR #1343.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous change forced consumers to pass JsonElement for RPC method
parameters whose schema is opaque-JSON (e.g. Rpc.Tools.HandlePendingToolCallAsync
result, McpConfigAddRequest config). This was a UX regression vs. accepting
'object?' as before, since most callers naturally have a typed CLR value
(string, dictionary, generated DTO) and would otherwise need to manually
SerializeToElement before each call.

Boundary special-case in csharp.ts: when the natural C# type for a top-level
RPC method parameter is JsonElement/JsonElement?, emit 'object'/'object?' at
the public surface and convert in the generated body via a new helper
CopilotClient.ToJsonElementForWire(object?). The helper short-circuits when
the value is already a JsonElement and otherwise serializes the runtime type
using the chained source-generated JsonSerializerContexts that the SDK
configures for JSON-RPC message formatting (matching pre-revert behaviour).

DTO field types are unaffected — opaque-JSON properties on any generated
class continue to be JsonElement(?). The asymmetry is intentional: inbound
opaque-JSON (events, hook inputs, tool args) stays as JsonElement so consumers
can introspect directly; outbound opaque-JSON (consumer-constructed) accepts
'object' for ergonomics.

ToolResultObject.ToolTelemetry reverted to IDictionary<string, object>? for
the same reason — it is a consumer-constructed outbound type. Tests
constructing ToolResultObject updated accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Properties annotated with .asInternal() in the runtime Zod schema now
emit as `internal` with [JsonInclude] (required for STJ source-gen to
(de)serialize non-public members).

Drop the `required` modifier when a property is internal: CS9032 forbids
`internal required` members in a public class. The runtime keeps the
field's semantic intent (must be present on the wire) but the SDK side
no longer requires external callers to initialize a member they cannot
see.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror the property-level internal handling for experimental: emit
[Experimental(Diagnostics.Experimental)] on properties whose runtime
Zod schema is marked .asExperimental(), and drop the `required`
modifier in that case to avoid CS9042 (required + obsolete-family
attribute on a non-experimental containing class).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runtime schema generator now rejects required properties marked internal/experimental whose containing type isn't, so the C# codegen no longer needs to silently drop \
equired\ to avoid CS9032/CS9042. The property-level marker support stays - it's still useful for optional properties.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 3 callsites converting Dictionary<string, object> to Dictionary<string, JsonElement> (SessionFsProvider rows, MCP elicitation response Content, elicitation schema Properties) were using TypesJsonContext.Default.Object, which restricts type resolution to the Types source-gen context. This is too narrow — at this boundary the runtime type can be any primitive, dictionary, or generated DTO from any of the chained contexts (Client, Types, Session, SessionEvents, Rpc).

Route all 3 through CopilotClient.ToJsonElementForWire, which uses the same chained serializer options that the JSON-RPC message formatter uses, matching pre-opaque-JSON-PR serialization behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…experimental

Mirrors the existing C# codegen support for property-level markers:
- isSchemaInternal -> emit #[doc(hidden)] above the field
- isSchemaExperimental -> emit the same /// **Experimental.** ... doc
  block used at the type level, indented to the field

Imports isSchemaInternal from ./utils.js (previously only the type-level
isSchemaExperimental was used). Regenerated rust/src/generated/* against
the current runtime schemas.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bility=experimental

Adds a shared schema walker in utils.ts that appends @internal /
@experimental tags to the description of any property carrying
visibility:internal or stability:experimental inline. The typescript
codegen calls this on each schemaForCompile before json-schema-to-typescript
compiles it, so the tags appear naturally inside the generated JSDoc
block for that property (no regex post-processing required).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y=experimental

Mirrors the existing type-level support: where a property's inline schema carries
visibility:internal or stability:experimental, the dataclass field gets the
corresponding `# Internal:` / `# Experimental:` comment line. Adds a shared
pushPyFieldMarkers helper so the comment block isn't duplicated between the
ordinary dataclass emitter and the flat discriminated-union emitter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors the existing C#/Rust/TypeScript/Python prop-marker treatment: when
a field carries x-internal or x-experimental in the JSON Schema, emit a
`// Internal: ...` / `// Experimental: ...` doc comment above the Go
struct field.

Factor the four near-identical field-emission sites in go.ts through a new
`pushGoFieldMarkers` helper that consolidates Deprecated/Experimental/
Internal `pushGo*` calls in one place.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commit b484198 introduced 590 UTF-8 double-encoded byte sequences in scripts/codegen/csharp.ts (box-drawing characters in section banners and em-dashes in comments). This commit restores the file to a clean UTF-8 encoding and reapplies the intended logic change (removing the unnecessary !propInternal && !propExperimental guards from the reqMod expressions in the four sites that had it).

While here, factor the duplicated propInternal + [JsonInclude] + visibility pattern into a small pushCSharpInternalAttribute helper used by all six property-emission sites in this file, and delete the dead propExperimental declarations that were left over from the original revert hack.

Generated output (SessionEvents.cs, Rpc.cs) is byte-identical to before this commit; the change is purely TypeScript-side cleanup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refreshes generated output (C# Rpc.cs, TS rpc.ts, Python rpc.py, Go zrpc.go/zsession_events.go, Rust *.rs) to pick up two changes that landed on main while the branch was open:
- namespace renamed from GitHub.Copilot.SDK.* to GitHub.Copilot.*
- improved description wording for AccountQuotaSnapshot.overage fields

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS SteveSandersonMS force-pushed the stevesandersonms/stevesa-fallback-on-jsonelement branch from 78d5ff2 to 12d3837 Compare May 21, 2026 14:45
@github-actions

This comment has been minimized.

Per @stephentoub: passing JsonTypeInfo (looked up via the configured resolver chain) instead of Type avoids the need for IL2026/IL3050 suppressions because the typed overload is statically analyzable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

- Add shared isOpaqueJson() in scripts/codegen/utils.ts; csharp.ts imports
  it instead of defining its own copy.
- typescript.ts: map {x-opaque-json: true} schemas to `unknown` instead
  of letting json-schema-to-typescript fabricate a {[k:string]:unknown}
  object shape from the missing type keyword.
- Narrow SDK surface casts at boundaries where the wire is opaque JSON
  but the SDK contract requires a tighter shape (session.ts elicitation,
  sessionFsProvider sqlite bindParams).
- Regenerated all language outputs against the latest runtime schema.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot mentioned this pull request May 21, 2026
SteveSandersonMS and others added 2 commits May 21, 2026 18:04
The previous commit mapped x-opaque-json schemas to `unknown` in the TS
codegen, which broke backcompat: fields that used to be `Record<string,
unknown>` (compiled from empty schemas by json-schema-to-typescript)
became bare `unknown`, forcing SDK consumers and tests to add casts.

Instead, strip the x-opaque-json marker during the TS normalize step.
j-s-to-ts then sees the same typeless schema it saw on main and emits
the same `{[k: string]: unknown | undefined}` shape. C# codegen still
reads the marker (from the un-normalized schema) and emits JsonElement.

Reverts the unnecessary narrowing casts in session.ts/sessionFsProvider.ts
and the test cast in session-event-types.test.ts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the inline `delete rewritten["x-opaque-json"]` out of the TS
codegen's rewrite loop and into a named helper next to isOpaqueJson, so
the paired `read the marker` / `ignore the marker` operations live
together and each language codegen calls the one it needs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

SteveSandersonMS and others added 2 commits May 21, 2026 18:59
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR maintains good cross-SDK consistency. Here's a summary:

Changes propagated to all SDKs

The schema-level changes (regenerated from the same source) are applied consistently across Node.js, Python, Go, Rust, and .NET:

  • compact method now accepts optional HistoryCompactRequest params (customInstructions) — updated in all SDKs
  • SessionCompactionCompleteData gains customInstructions field — updated in all SDKs
  • HistoryCompactRequest type added to all SDK generated types
  • Comment/doc updates (overage → "additional usage", experimental markers) — consistent across SDKs

Language idiom differences (expected, not issues)

  • Rust exposes the new parameter as a separate compact_with_params(params) method rather than making the existing compact() parameter optional — this is idiomatic Rust and intentional.

Primary .NET-specific change

The JsonElementobject boundary refactoring is .NET-specific and doesn't have a cross-SDK equivalent (other SDKs use native JSON types like any, dict, serde_json::Value, map[string]any which already serve the same purpose without needing this distinction).

No consistency gaps identified.

Generated by SDK Consistency Review Agent for issue #1359 · ● 4.3M ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants