Repository navigation
core: process the active case of a C# 15 union command result - #199
carldebilly wants to merge 2 commits into
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.
| public static async ValueTask<object?> InvokeAsync( | ||
| Delegate handler, | ||
| object?[] arguments) => | ||
| UnwrapUnion(await InvokeHandlerAsync(handler, arguments).ConfigureAwait(false)); |
There was a problem hiding this comment.
[P2] Await active awaitable cases before classifying the union result
InvokeHandlerAsync awaits only the original delegate result; UnwrapUnion then exposes a Task/ValueTask active case after that awaiting step, so it reaches rendering as the task wrapper rather than its awaited value. This violates the documented active-case-as-direct-return contract and can silently change an explicit failure/exit verdict into success.
Reproduced through the public ReplApp API on net11.0 at this exact head: define public record ExitPayload(int ExitCode, object? Payload) : IExitResult; and public union ExitCase(Task, ExitPayload);. A handler returning Task.FromResult(new ExitPayload(7, "done")) directly prints "done" and exits 7. Returning (ExitCase)Task.FromResult(new ExitPayload(7, "done")) instead prints the Task properties (result, status, id, etc.) and exits 0. Plain Task and ValueTask union cases likewise render wrappers, while the direct-return controls and a non-awaitable union case render the bare payload. A faulted Task case also changes the classified error compared with the direct Task.
Normalize the active case through awaitable handling before downstream classification/rendering (with the intended cycle/depth bound), and cover both Task and ValueTask active cases, including exit-result and faulted cases.
There was a problem hiding this comment.
Fixed in f273279. InvokeAsync now awaits a case that is a Task<T>/ValueTask<T> inside the bounded unwrap (UnwrapUnionAsync, sharing AwaitResultAsync with the handler's own result), so the case behaves as the same task returned directly. Only values that come out of a union are awaited; a handler's own Task<Task<T>> keeps its single await.
Your reproduction is now a test: (UnionPending)Task.FromResult(new UnionExit(7, "done")) exits 7 and prints done. Also covered, all red before the fix: Task and ValueTask cases, a faulted Task case whose exit code and output equal those of the same faulted task returned directly, and a tuple element whose union case is a task. The same commit processes a union a middleware sets as ReplExecutionContext.Result.
The branch is rebased on main (now including #198); full suite on net10.0 and net11.0: 5594 total, 0 failed, 74 skipped.
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
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.