fix(dotnet): preserve unknown values of open protocol enums - #462
Merged
Connor Peet (connor4312) merged 9 commits intoOct 1, 2026
Merged
Connor Peet (connor4312) merged 9 commits into
Connor Peet (connor4312) merged 9 commits into
Conversation
`types/` requires every protocol enum to declare `@exhaustive` or `@nonexhaustive`, and `versioning.md` guarantees that a value added to an open enum is a PATCH-compatible, additive change that an older peer must relay intact. The Rust, Swift and Kotlin generators honor that contract, and the Go and TypeScript mirrors are tolerant by construction — but `generate-csharp.ts` never consulted it. Every string enum became a closed C# `enum` behind `WireEnumConverter<T>`, which throws on an unrecognized wire value, so a host adding (for example) a `ToolCallStatus` value in a PATCH release caused a whole-message decode failure in the .NET client. Open enums are now generated as a `readonly struct` wrapping the raw wire string, with the known values as static members and a generated per-type converter — mirroring Kotlin's `value class` and Rust's `Unknown(String)`. Closed enums keep the existing `enum` + `WireEnumConverter<T>` mapping, which correctly rejects a value the contract says is invalid. Because the generated converters replace the reflective one on this path, the open-enum surface is also trimming- and AOT-friendly. `GeneratedActionMetadata.GetWireName` collapses to `ActionType.Value`, so it stays correct for an action type this build does not know. Also adds `ConfigPropertySchema.minItems` / `maxItems` so an array-typed config property can express its cardinality, matching the JSON Schema fields the type already carries (`items`, `required`, `additionalProperties`). Round-trip fixture 045 locks the behavior: `MessageOrigin.kind` is an open enum on a directly-typed field — the one case neither the unknown-union fixture (003) nor the open-string-field fixture (024) covered, and the case a closed language enum cannot represent. Verified to fail before this change and pass after. Closes microsoft#366 Closes microsoft#432 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sukanth Gunda (sukanth)
requested review from
Connor Peet (connor4312) and
roblourens
as code owners
September 23, 2026 05:09
Copilot started reviewing on behalf of
Connor Peet (connor4312)
September 23, 2026 14:39
View session
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Open-enum unions still reject unknown discriminants, and cardinality fields lack required schema constraints.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds .NET forward compatibility for @nonexhaustive enums and configuration array cardinality metadata.
Changes:
- Generates raw-string C# structs and converters for open enums.
- Adds
minItemsandmaxItemsacross schemas and clients. - Adds unknown-enum conformance coverage and documentation.
| File | Review |
|---|---|
types/test-cases/round-trips/KNOWN-FIDELITY-GAPS.md |
Documents enum fidelity coverage. |
types/test-cases/round-trips/045-message-origin-unknown-kind-preserved.json |
Tests unknown enum preservation. |
types/common/state.ts |
Adds cardinality fields. Moderate: constrain both to non-negative integers. Nit: add shared round-trip coverage. |
scripts/generate-csharp.ts |
Generates open enums. Moderate: unknown union discriminants remain unsupported for SessionOriginKind, CustomizationEnablementKind, ChangesetOperationTargetKind, and ChatSourceKind. |
schema/state.schema.json |
Adds cardinality schema fields. |
schema/notifications.schema.json |
Regenerates shared schema definitions. |
schema/errors.schema.json |
Regenerates shared schema definitions. |
schema/commands.schema.json |
Regenerates shared schema definitions. |
schema/actions.schema.json |
Regenerates shared schema definitions. |
docs/guide/state-model.md |
Documents cardinality fields. |
docs/.changes/20260923-dotnet-open-enum-forward-compat.json |
Records the .NET fix. Nit: align its compatibility claim with versioning.md. |
docs/.changes/20260923-config-array-cardinality.json |
Records the cardinality addition. |
clients/swift/AgentHostProtocol/Sources/AgentHostProtocol/Generated/State.generated.swift |
Adds Swift cardinality fields. |
clients/swift/AgentHostProtocol/Sources/AgentHostProtocol/Generated/Commands.generated.swift |
Updates generated Swift models. |
clients/rust/crates/ahp-types/src/state.rs |
Adds Rust cardinality fields. |
clients/kotlin/src/main/kotlin/com/microsoft/agenthostprotocol/generated/State.generated.kt |
Adds Kotlin cardinality fields. |
clients/kotlin/src/main/kotlin/com/microsoft/agenthostprotocol/generated/Commands.generated.kt |
Updates generated Kotlin models. |
clients/go/ahptypes/state.generated.go |
Adds Go cardinality fields. |
clients/dotnet/src/AgentHostProtocol/Generated/ActionMetadata.generated.cs |
Uses raw action wire values. |
clients/dotnet/src/AgentHostProtocol.Abstractions/Generated/State.generated.cs |
Adds open enums and cardinality fields. |
clients/dotnet/src/AgentHostProtocol.Abstractions/Generated/Notifications.generated.cs |
Generates an open notification enum. |
clients/dotnet/src/AgentHostProtocol.Abstractions/Generated/Commands.generated.cs |
Generates open command enums. |
clients/dotnet/src/AgentHostProtocol.Abstractions/Generated/Actions.generated.cs |
Generates open action types. |
clients/dotnet/AGENTS.md |
Documents .NET enum generation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses review feedback on microsoft#462. The open-enum fix covered enums used as plain field types, but two open enums are also union discriminators: `SessionOriginKind` and `CustomizationEnablementKind`. Their generated `UnionConverter`s hardcoded `allowUnknown: false`, so an added wire value was rejected by the union decoder before the new struct converters ever ran — the whole-message decode failure the original fix set out to remove. The other five generators already derive this from the discriminator enum's `@exhaustive` / `@nonexhaustive` annotation via the shared `discriminatedUnionAllowsUnknown` helper; only C# maintained the flag by hand. The C# generator now calls the same helper, so union tolerance stays in lockstep with the protocol declaration instead of drifting. This also corrects the inverse drift the hand-maintained flags had accumulated: `TerminalClaim`, `ChatInputAnswer`, and `CustomizationLoadState` were wrongly permissive, accepting discriminators their `@exhaustive` contracts say are invalid. .NET now matches Rust exactly on every union. `ChangesetOperationTarget` (also `@nonexhaustive`) is derived now too, and the `StateAction` and `CustomizationEnablement` unions name their discriminator enum explicitly because their variants are synthesized or inline and carry no resolvable discriminator property. Marks `minItems` / `maxItems` as `@integer` with `@minimum 0`, so the generated schemas constrain them the way every client already models them. Fixtures: 046 covers the new config cardinality fields end to end, and 047 covers an unknown discriminator on an open union — verified to fail before this change and pass after. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Connor Peet (connor4312)
September 23, 2026 17:11
View session
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical zero-value regression and moderate union-generation drift must be resolved before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
Addresses review feedback on microsoft#462. Generated records left their literal discriminator without an initializer and relied on the property's zero value. That was already wrong before this PR: a closed C# enum's zero value is its *first* member, so `new SessionReadyAction()` silently serialized `"root/agentsChanged"` — the correct discriminator on exactly one of the 117 action records and wrong on the other 116. Moving `ActionType` to an open-enum struct changed the symptom to an empty wire string but not the underlying defect. Rather than restore the previous zero-value semantics, this pins each record's discriminator to its own literal, which is what the property's type already says it must be. `new SessionReadyAction()` now serializes `"session/ready"`. The generator derives the initializer from the discriminant it already resolves, and the hand-written session-action records — whose wire types are aliases with no `ActionType` member — pin theirs through the open enum's raw constructor. `default(ActionType)` on its own still yields an empty value. That is deliberate: it is an explicit programmer error rather than a decoded payload, and an obviously-invalid discriminator is safer than one that is silently valid but wrong. Also derives the hand-written `ChatOrigin` union's `allowUnknown` from `ChatOriginKind` instead of hardcoding it, so no union in the generator maintains that flag by hand any more. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Connor Peet (connor4312)
September 23, 2026 21:00
View session
Upstream landed microsoft#450 (typed file-edit models), which added round-trip fixtures 045-049 — colliding with the 045/046/047 numbers this branch had already taken. Git merges both sets without a textual conflict, so the collision surfaces as duplicate numeric prefixes rather than a failure: every fixture still runs and passes, but two different files share each of three numbers, and the corpus docs referencing "fixture 045" become ambiguous. Renumbers this branch's three fixtures to 050-052, above upstream's highest, and updates the two places that referenced them by number (KNOWN-FIDELITY-GAPS.md and fixture 052's own cross-reference). microsoft#450 also edited `scripts/generate-csharp.ts`, the same file this branch changes. The merge is clean and `npm run generate` produces no drift on the merged tree, so the two sets of generator changes compose. Verified on the merged tree: root suite 451 pass, .NET 0 failed, plus Rust, Go, TypeScript, Kotlin, and Swift green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Upstream landed microsoft#471 (blocking MCP server startup), which adds a new `ActionType` member and touches `scripts/generate-csharp.ts` — the same generator this branch rewrites. The canonical sources (`types/`, `scripts/`) merged cleanly; the only conflicts were in three generated .NET mirrors: Actions.generated.cs, State.generated.cs, ActionMetadata.generated.cs Generated files are never hand-edited, so the resolution is to regenerate them from the merged canonical sources rather than reconcile the markers by hand. `npm run generate` produces output containing both sides: upstream's new `session/mcpServerBackgroundRequested` action and this branch's open-enum representation. Worth noting the two changes compose correctly — the new action record picks up this branch's pinned discriminator automatically (`= ActionType.SessionMcpServerBackgroundRequested`), so it does not reintroduce the zero-value defect on a record added after that fix. Verified on the merged tree: root suite 454 pass (up from 451 with upstream's three new reducer fixtures), .NET 0 failed, plus Rust, Go, TypeScript, Kotlin, and Swift green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Upstream landed microsoft#461 (scheduled automation run limits), which again touches `scripts/generate-csharp.ts` and adds round-trip fixtures numbered 045-050. The merge itself is clean, and regeneration shows the two generator changes still compose: upstream's new `AutomationAfterRunsCondition` / `AutomationAfterDateCondition` records pick up this branch's pinned discriminator automatically, so types added after that fix do not reintroduce the zero-value defect. Last tick this branch renumbered its fixtures to dodge a collision with microsoft#450. That turns out to be a treadmill worth stepping off: `upstream/main` already carries nine duplicate numeric prefixes (019, 030, 031, 041, and 045-049), several created by microsoft#461 and microsoft#450 colliding with each other. Prefixes are assigned per-branch, so any parallel PR can take a number, and chasing that costs a rename plus a full CI run every time. Rather than renumber again, this makes the references stable: the corpus doc and the fixture cross-references now name fixtures by filename instead of "fixture NNN", with a note recording why. The fixture files themselves are left alone, matching how the repository already tolerates shared prefixes. Verified on the merged tree: root suite 471 pass, .NET 0 failed, plus Rust, Go, TypeScript, Kotlin, and Swift green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Upstream landed microsoft#483 (per-chat change summaries), which touches `types/channels-chat/state.ts`, `types/channels-session/state.ts`, the schemas, and every client's generated mirror — including the two .NET mirrors this branch also modifies. The merge is clean and regeneration produces no drift, so the new chat-summary fields and this branch's open-enum work compose without interference. Kept rather than deferred: the overlap is in `Actions.generated.cs` / `State.generated.cs`, so leaving it unmerged would have surfaced as a regenerate-to-resolve conflict on a later tick. Also picks up microsoft#477, a Dependabot lockfile bump under `plugins/`, which is outside the protocol surface. Verified on the merged tree: root suite 471 pass with the generated-output freshness check green, .NET 0 failed, plus Rust, Go, TypeScript, Kotlin, and Swift. Every action record still pins its own discriminator (0 unpinned). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Upstream landed microsoft#484 (durable chat move and reorder contracts), which adds a `ChatMoveDestination` union and edits `scripts/generate-csharp.ts` — the same generator this branch rewrites. Unlike the previous merges, this one conflicted in hand-written source rather than only in generated output. The conflict was a clean semantic overlap on adjacent lines: this branch threads `project` into `generateDiscriminatedUnion` so union tolerance is derived from the discriminator annotation, while upstream appends a third union to the same call block. Taking either side alone would have dropped the other, so the resolution keeps both — all three unions now generate through the project-aware signature. The two conflicted .NET mirrors are regenerated from the merged sources rather than hand-resolved. The result confirms the changes compose: `ChatMoveDestinationConverter` derives `allowUnknown: true` from `ChatMoveDestinationKind`'s `@nonexhaustive` annotation via the shared helper, instead of the hardcoded flag it would have carried. Verified on the merged tree: root suite 499 pass with generated-output freshness green, .NET 0 failed (including upstream's new ChatMoveDestinationTests), plus Rust, Go, TypeScript, Kotlin, and Swift. Every action record still pins its own discriminator (0 unpinned). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Connor Peet (connor4312)
previously approved these changes
Oct 1, 2026
Connor Peet (connor4312)
enabled auto-merge (squash)
October 1, 2026 16:56
Connor Peet (connor4312)
approved these changes
Oct 1, 2026
Logan Ramos (lramos15)
approved these changes
Oct 1, 2026
Sukanth Gunda (sukanth)
deleted the
fix-dotnet-open-enum-forward-compat
branch
October 1, 2026 19:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Teaches the C# generator the
@exhaustive/@nonexhaustiveenum contract that already exists intypes/, so the .NET client stops failing to decode enum values added by newer peers. Also addsConfigPropertySchema.minItems/maxItems.Closes #366
Closes #432
Why
types/requires every protocol enum to declare@exhaustiveor@nonexhaustive(scripts/enum-compatibility.tsthrows otherwise), andversioning.mdguarantees that adding a value to an open enum is an additive, PATCH-compatible change that an older peer must relay intact.Support for that contract turned out to be uneven:
@nonexhaustive?Unknown(String)variantcase unknown(String)value class … (val rawValue: String)type X string+ constsgenerate-csharp.tsnever consulted itEvery C# string enum was emitted as a closed
enumbehindWireEnumConverter<T>, whoseReadthrowsJsonExceptionon an unrecognized value.ToolCallStatusandCustomizationTypeare@nonexhaustiveintypes/, so a host adding a value to either in a PATCH release causes a whole-message decode failure in the .NET client — exactly the 1.0.0 blocker #366 describes, still live for this one client.It went unnoticed because the corpus never covered this case. Fixture 003 covers an unknown union variant (
allowUnknown→ rawJsonElement) and fixture 024 covers an unknown value of an open string field — both mechanisms .NET already handled. Nothing covered an unknown value of an open enum on a directly-typed field, which is the case a closed language enum cannot represent.How
readonly structwrapping the raw wire string, with the known values as static members and a generated per-type converter. This mirrors Kotlin'svalue classand Rust'sUnknown(String). Because the generated converters replace the reflectiveWireEnumConverter<T>on this path, the open-enum surface is also trimming/AOT-friendly (a step toward .NET client: support trimming and Native AOT #410).enum+[WireValue]+WireEnumConverter<T>mapping, which correctly rejects a value the contract says is invalid.allowUnknownfrom the discriminator enum's annotation via the shareddiscriminatedUnionAllowsUnknownhelper, instead of a hand-maintained per-union flag. The other five generators already did this; C# was the only one keeping the flag by hand, and it had drifted in both directions (see below).GeneratedActionMetadata.GetWireNamecollapses toActionType.Value, so it stays correct for an action type this build doesn't know (it previously threwArgumentOutOfRangeException).ConfigPropertySchema.minItems/maxItems— purely additive, matching the JSON Schema fields the type already carries (items,required,additionalProperties,readOnly).No hand-written .NET source needed to change. The only constant-pattern usages of an affected enum were 98 lines in one generated file; the one protocol enum used in a hand-written
switch(PendingMessageKind) is@exhaustiveand is untouched.Review follow-up (f133ae1)
Copilot's reviewer caught a real gap:
SessionOriginKindandCustomizationEnablementKindare@nonexhaustiveand union discriminators, so theirUnionConverters rejected an added wire value before the new struct converters ever ran. DerivingallowUnknownfrom the annotation fixes that, and also corrects the inverse drift the hand-maintained flags had accumulated:TerminalClaim,ChatInputAnswer, andCustomizationLoadStatewere wrongly permissive, accepting discriminators their@exhaustivecontracts call invalid. .NET now matches Rust exactly on every union.ChangesetOperationTargetis derived now too.Also from review:
minItems/maxItemsare tagged@integer+@minimum 0, so the schemas constrain them the way every client already models them.Tests / conformance
Three new round-trip fixtures, each verified as a real regression guard by reverting the relevant generator change and confirming it fails:
045-message-origin-unknown-kind-preserved.json— an open enum on a plain (non-discriminant) field.046-config-schema-array-cardinality.json— the newminItems/maxItemsthroughSessionModelInfo.configSchema. UsesminItems: 0, the value most likely to be silently dropped as falsy.047-session-origin-unknown-kind-preserved.json— an unknown discriminator on an open union, which fixture 045 does not reach.npm testBUILD SUCCESSFUL, incl. fixture 045swift buildclean — see caveatCaveat: Swift's XCTest suite could not run in my environment (XCTest ships with full Xcode; only CommandLineTools was usable). The Swift library compiles cleanly, and its only change is the two additive generated fields that Go/Kotlin/TypeScript verified green — worth a CI confirmation.
Compatibility
No wire-format change — the JSON is identical in both directions. This is a source-breaking change for .NET consumers who
switchover an affected enum with constant patterns, which is precisely why it belongs before the 1.0 guarantee takes effect (the .NET client is on0.9.0).PROTOCOL_VERSIONis unchanged.Happy to split the
minItems/maxItemsaddition into its own PR if you'd prefer to review the two independently.Out of scope
otlp/export*to clients (OTLP channel:otlp/export*server→client notifications aren't surfaced by the clients #342).@exhaustive; this change honors the existing annotations rather than revising them.