Repository navigation
core: process the active case of a C# 15 union command result - #199
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Changes requested at exact head 9e80985, against merge-base 00b35aa. One introduced P2 confirmed by public-API execution: awaitable union cases bypass awaiting, leak task wrappers into output and can lose an explicit exit code. See inline reproduction and controls. Independent read-only review covered all five changed files; parent inspected the delta/callers and executed the candidate. No other concrete introduced P0-P2 established. Documented deferred MCP schemas/UI resources, paging discovery and net10 polyfills are not reported as defects.
Validation: exact-head Release unit-test project build with warnings as errors passed; both unit-test targets passed, 1348 net10.0 plus 1360 net11.0 (2708 passed, 0 failed/skipped). These existing tests do not cover the reproduced awaitable-case failure. External SDK11 probe built and executed against this checkout; no product source edits or package publication.
Separate unresolved CI gate: Build, Test, Pack failed in Repl.RemoteTests/net11.0, When_TheHostStops_Then_CommandCleanupCannotDisableItsDeadline(blocking-callback,1): exit 0 instead of expected 1. This is not attributed to the union delta without introduction evidence; it remains unvalidated rather than being dismissed as flaky. Local unit-test success is not a claim of green full CI.
46ed50a to
806bc8c
Compare
On net11.0 a handler may return a union (sync, Task<T> or ValueTask<T>). The command invoker replaces it with the case it holds, after the await, so exit codes, error classification, rendering and middleware see the value the handler meant to return. A union nested in a union, or placed as an element of a returned tuple, is unwrapped too; a default union behaves like null. The unwrapping compiles for net11.0 only: native unions need the .NET 11 runtime, and net10.0 behaviour is unchanged. A union is not a bindable input and is refused as a usage error, which a test now pins.
A union whose active case was a Task<T> or ValueTask<T> reached rendering as the task: an IExitResult behind it was lost (exit 0) and a faulted task passed as success. The case is now awaited as the same task returned directly would be, inside the bounded unwrap; only values that come out of a union are awaited, so a handler's own Task<Task<T>> keeps its single await. A union a middleware sets as ReplExecutionContext.Result is unwrapped before rendering, and a union element of a returned tuple gets the same await. A union handler parameter keeps the generic behaviour of any type Repl cannot convert from command input (a usage error), and can still come from the services; the union-specific message is gone, and #202 tracks a Map-time check for every unconvertible input type. The docs list the union semantics and limits.
806bc8c to
f273279
Compare
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved at exact head f273279, against unchanged base cc5a043. This supersedes my earlier request-changes review: the awaitable union-case blocker is fixed and no other concrete introduced P0-P2 was found in the four-file delta, including independent read-only revalidation.
Previously completed exact-head validation: warning-as-error Release unit-test project/dependency build; 1373 net10.0 and 1396 net11.0 unit tests passed (2769, no failures/skips); original nine-case external public-API reproduction rebuilt against this checkout. Task/ValueTask union cases match direct-return controls, faulted cases preserve the error/exit behavior, and the original union Task now exits 7 instead of 0. No product source edits or full-solution local rebuild.
Refreshed live head/base, review threads and CI before approval. The macOS rerun now passes, including its Test step, closing the previously missing validation gate. Linux/Windows builds/tests, Build Test Pack, container/shell/MCP smoke, signal stress and documentation/security checks are successful; GitHub Release is skipped. The prior macOS Actions environment-file failure remains historical, not attributed to the union change. No new local rebuild was needed for this CI-only transition. No merge performed.
Closes #177.
What changed
CommandInvoker.InvokeAsyncreplaces a union result by its active case (System.Runtime.CompilerServices.IUnion.Value) after the await, for sync,Task<T>andValueTask<T>handlers. Everything downstream (middleware'sReplExecutionContext.Result, classification, exit codes, every output format,Repl.Testing) sees the case.Task<T>/ValueTask<T>is awaited, as the same task returned directly would be; a faulted task fails the run the same way. Only values that come out of a union are awaited, so a handler's ownTask<Task<T>>keeps its single await.IUnionthat leads back to itself); adefaultunion or a case holdingnullbehaves likenull.ReplExecutionContext.Resultis processed once the pipeline completes.net11.0only (#if NET11_0_OR_GREATER): native unions need the .NET 11 runtime.net10.0is unchanged.docs/commands.md: a "Union return values (.NET 11)" section with a working example, the semantics and the limits, plus a row in the return-pattern table.Verified on SDK 11.0.100
IUnionis in corelib withobject? Value; value-type cases come out boxed;default(union).Valueisnull; a union holding a union yields anIUnion.Tests
Given_UnionResults(net11.0only), written first. The awaitable-case fix added 9 more tests that failed before it (Task and ValueTask cases, anIExitResultbehind a task, a faulted task compared with the task returned directly, a middleware setting a union, a tuple element whose case is a task). Also covered: sync/Task/ValueTask handlers, value-type, default and null cases, nested unions,IExitResultandIReplResultcases compared with the same result returned directly, JSON and human output equal to the bare payload, the depth bound, a middleware observing the active case, and a union parameter (usage error from command input, injected from the services).Full suite on net10.0 and net11.0: 5594 total, 0 failed, 74 skipped (Windows);
-warnaserrorand markdownlint clean.Limits, documented
Resultshelper (Results.Exit(3, lookup)) is rendered as an ordinary object.IUnion(an assembly compiled fornet10.0) gets no unwrapping, even on the .NET 11 runtime.McpAppResourceInvokercalls its handlers without going throughCommandInvoker, so a UI resource handler returning a union is not unwrapped; those handlers return strings.anyOfoutput schema is mcp: describe union command results with anyOf in the output schema #185.