Skip to content

Fix codegen for discriminated unions nested within other types#736

Merged
SteveSandersonMS merged 2 commits intomainfrom
stevesa/codegen-discriminated-unions
Mar 9, 2026
Merged

Fix codegen for discriminated unions nested within other types#736
SteveSandersonMS merged 2 commits intomainfrom
stevesa/codegen-discriminated-unions

Conversation

@SteveSandersonMS
Copy link
Contributor

@SteveSandersonMS SteveSandersonMS commented Mar 9, 2026

Previously we only handled discriminated unions at the top level for C#. This resulted in a weak and loosely typed representation of some of the newer APIs. This PR turns this into strongly typed APIs while retaining protocol v2 backcompat.

Note that other languages aren't changed:

  • TypeScript already used nested types to represent discriminated unions
  • Go and Python use quicktype's built-in discriminated union representation, which is a "kind" discriminator and then an object that has the union of all properties across all subtypes with all being optional (which is not as nice as TypeScript or C# but is idiomatic for those languages)

…sult is strongly-typed APIs for permission requests
Copilot AI review requested due to automatic review settings March 9, 2026 12:16
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner March 9, 2026 12:16
…ants

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

github-actions bot commented Mar 9, 2026

Cross-SDK Consistency Review

This PR improves C# codegen to generate strongly-typed discriminated unions for PermissionRequest and similar types, replacing the previous weak object type. This is excellent progress! 🎉

However, there's a cross-SDK consistency gap:

Current State After This PR

SDK PermissionRequest Type Discriminated Union Support
TypeScript ✅ Strongly-typed union Native TypeScript union types with kind discriminators
C# ✅ Strongly-typed classes (after this PR) [JsonPolymorphic] with derived types
Python ❌ Weak single class All variant fields as optional attributes on one PermissionRequest class
Go ❌ Weak single struct All variant fields as pointers on one PermissionRequest struct

Impact

The Python and Go SDKs currently have the same issue this PR fixes for C#:

  • No compile-time type safety when working with different permission request kinds
  • All fields are optional/nullable even though specific variants have required fields
  • No IntelliSense/autocomplete guidance for which fields apply to which kind

Example

TypeScript (already strong):

if (request.kind === "shell") {
  console.log(request.fullCommandText); // ✅ TypeScript knows this exists
}

C# (strong after this PR):

if (request is PermissionRequestShell shell) {
  Console.WriteLine(shell.FullCommandText); // ✅ C# knows this exists
}

Python (currently weak):

if request.kind == PermissionRequestKind.SHELL:
    print(request.full_command_text)  # ⚠️ Not type-safe, could be None

Go (currently weak):

if request.Kind == KindShell {
    fmt.Println(*request.FullCommandText) // ⚠️ Not type-safe, pointer could be nil
}

Root Cause

  • TypeScript & C# use hand-written codegen (scripts/codegen/typescript.ts, scripts/codegen/csharp.ts) with discriminated union support
  • Python & Go use quicktype library (scripts/codegen/python.ts:114, scripts/codegen/go.ts:84) which flattens discriminated unions into a single type

Recommendation

To maintain feature parity across all SDKs, consider either:

  1. Update Python & Go codegen to use custom logic similar to the C# implementation (may be significant work)
  2. File follow-up issues to track this as tech debt for Python and Go
  3. Document the limitation in Python/Go documentation so users understand the current type safety differences

This PR is good to merge as-is since it improves C# without breaking anything, but the consistency gap is worth addressing in the roadmap.


Note: This is an automated consistency review. I'm not blocking the PR, just surfacing the cross-language gap for the team's awareness.

Generated by SDK Consistency Review Agent for issue #736 ·

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the C#/.NET codegen pipeline so discriminated unions nested inside other types are generated as strongly-typed polymorphic classes (instead of weakly-typed object), improving the .NET SDK’s type safety while keeping protocol v2 behavior working.

Changes:

  • Add C# codegen support for nested discriminated unions (anyOf with a shared const discriminator), plus a rename override to keep the public .NET type name as PermissionRequest.
  • Switch .NET permission-request handling to deserialize via the generated SessionEventsJsonContext and pass a strongly typed PermissionRequest through the permission handler flow.
  • Update .NET tests to assert against specific derived permission request types (e.g., PermissionRequestCustomTool, PermissionRequestShell).

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
scripts/codegen/csharp.ts Adds nested-discriminated-union generation and type rename overrides for cleaner public C# type names.
dotnet/src/Generated/SessionEvents.cs Generated output now models permissionRequest as a polymorphic PermissionRequest union with derived variants.
dotnet/src/Session.cs Deserializes permission requests using SessionEventsJsonContext and threads typed requests through broadcast handling.
dotnet/src/Types.cs Removes the prior hand-written PermissionRequest type and its serializer registration.
dotnet/test/ToolsTests.cs Updates assertions to use PermissionRequestCustomTool.ToolName instead of extension data.
dotnet/test/PermissionTests.cs Updates toolCallId assertion to pattern-match the appropriate derived permission request type.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Cross-SDK Consistency Review ✅

I've reviewed this PR for consistency across all four SDK implementations. This change maintains appropriate cross-SDK consistency.

Summary of Change

This PR improves the C# code generation to handle discriminated unions nested within other types, specifically for PermissionRequest. Previously, the .NET SDK typed PermissionRequestedData.PermissionRequest as object, requiring manual deserialization and extension data access. Now it generates a strongly-typed polymorphic hierarchy.

Cross-Language Analysis

✅ TypeScript - Already uses inline discriminated unions (lines 2178-2374 in session-events.ts). The permissionRequest property is a union type with variants like { kind: "shell"; ... } | { kind: "write"; ... } etc. This is the most natural representation for TypeScript.

✅ .NET (this PR) - Now generates:

  • Base class PermissionRequest with [JsonPolymorphic] attributes
  • Derived classes PermissionRequestShell, PermissionRequestWrite, etc.
  • Uses C#'s native polymorphic serialization for strongly-typed access

✅ Python - Uses quicktype's standard representation (as noted in PR description):

  • Single PermissionRequest class with kind: PermissionRequestKind discriminator
  • All properties are optional fields (union of all subtypes)
  • This is idiomatic for Python's duck-typing

✅ Go - Uses quicktype's standard representation:

  • Single PermissionRequest struct with Kind PermissionRequestKind
  • All properties are optional pointers
  • This is idiomatic for Go's structural typing

Consistency Assessment

Each language now has a semantically equivalent but idiomatically appropriate representation:

  • TypeScript: Inline union types (strongest compile-time checking)
  • .NET: Polymorphic class hierarchy with base/derived types
  • Python/Go: Unified type with kind discriminator (follows quicktype conventions)

The PR description explicitly acknowledges this:

"Note that other languages aren't changed: TypeScript already used nested types to represent discriminated unions; Go and Python use quicktype's built-in discriminated union representation, which is a "kind" discriminator and then an object that has the union of all properties across all subtypes with all being optional (which is not as nice as TypeScript or C# but is idiomatic for those languages)"

Recommendation

Approved - This is an appropriate language-specific improvement that brings .NET in line with TypeScript's type safety while respecting the idiomatic patterns of each language. The codegen changes are scoped to C# only, and the generated types maintain protocol compatibility across all SDKs.

Generated by SDK Consistency Review Agent for issue #736 ·

@SteveSandersonMS SteveSandersonMS merged commit ad51d74 into main Mar 9, 2026
38 of 39 checks passed
@SteveSandersonMS SteveSandersonMS deleted the stevesa/codegen-discriminated-unions branch March 9, 2026 12:38
@phil65
Copy link

phil65 commented Mar 9, 2026

Are there plans to properly discriminate the event models for python, too? Or would PRs for this be welcome? currently it is horrible to work with the given models, using datamodel-codegen for example would improve the models significantly.

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.

3 participants