Repository navigation
fix: release-review fixes before 0.12 (remote diagnostics, live-analysis tokens, publish safety, migration notes) - #171
Conversation
.editorconfig sets charset = utf-8, and no .cs file carried a byte-order mark at v0.11.0. Thirty files picked one up during 0.12 work. Only the three leading bytes are removed; line endings are untouched.
A failed .nupkg push was only a warning, and the job failed only when every package failed. With the GitHub release already created, a green run could hide a version whose packages are partly missing from NuGet; 0.12 adds two new packages, which widens that window. The job now fails with an error naming the packages, and a re-run recovers through --skip-duplicate as docs/publishing.md describes. A symbol package that fails stays a warning, since it only degrades debugging.
A backend method failure reached the client as "The remote backend failed." and its detail went only to System.Diagnostics.Trace, which no default .NET logging provider reads. Terminal failures and a fault that stopped the listener accepting were just as invisible, so an operator in a container saw nothing while the client got a sanitized error. NamedPipeReplServer now hands each of these exceptions to the onConnectionError callback it already took, and RemoteReplServer.RunAsync gains overloads taking the same callback as onError for hosts that serve their own streams. The trace stays as the record when no callback is given. A callback that throws is contained: the client still receives its error and the connection keeps serving. The accept loop also reports its fault before faulting Completion, and sample 05 now watches Completion like sample 10, so the host stops instead of running on without its administration endpoint. docs/remote-cli.md documents the callback and updates its hosting example. TDD: the backend and terminal tests were red (callback never called, 20 s timeout) with a successful build before the fix.
Live analysis reported a missing or unconvertible value with the C#
parameter name ("Supply a value for 'retryCount'."), while the binder's
refusals for the same parameter name the token the user types
("Option '--retryCount' requires exactly one value."). Both messages now
use the option's display token, falling back to the parameter name for an
argument-only parameter or a route value, through a new
OptionSchema.ResolveDisplayName.
TDD: both rows of the new test were red with a successful build before
the fix, and red again with the Core change stashed.
WriteAsync's remark said a failed connection is reported as an IOException, but once the WebSocket is closing or closed (CloseSent, Closed, Aborted) a write completes without sending and without throwing. That has been the behaviour since 0.11, when the guard was an Open-only check, so changing it now would break shipped callers. The remark now says what happens: a failed send throws IOException, a write after the close is discarded, writes after only the peer's close are still sent, and a dead peer is detected by reading. A test pins the discard.
The release review asked whether the startup input set by a prepared run could leak into a run started from one of its handlers, since RunStartupOutcomeAsync restores _preparedStartup by hand but never _startupInput. It cannot: PendingInvocationScope clears the startup input for every new run and restores it on disposal. This test pins that. Removing the clear makes it fail with the outer input replayed (outer, outer, outer).
Sample 10's exit-code resolver wrote every failure's full exception to stderr. That is what a failed serve needs in a container log, but a client command against a server that is not running (health --ready, exec, console) printed the framework's "Could not connect to pipe ..." line and then a TimeoutException stack trace for the same condition. The resolver now writes the exception only for the serve route. TDD: the new client test was red, with the stack trace after the error line, before the change; the existing serve test still requires the full exception.
Run* rejects an undefined SessionScope, and a standalone run an undefined ProcessSignalHandling, with ArgumentOutOfRangeException, since numeric configuration or deserialization can produce such values. Neither the options nor the overloads said so. The two properties now state when the run fails, and each overload that takes ReplRunOptions documents the exception.
The release review found breaking or observable changes whose migration had no "Changed in 0.12" note where readers look for one: - best-practices: Scoped resolves per session and per MCP connection (#74) - configuration-reference and execution-pipeline: framework exit codes and stderr routing (#85); process-signal ownership, labelled (#80) - mcp-reference: the operator events and the 2008 client message (#106, #163); the SDK 2.x upgrade section, labelled (#71) - mcp-conformance: unknown resource and prompt error codes (#107) - testing-toolkit: result read-through on Results.Exit (#128)
…after an accept fault Review of the previous commit found three gaps: - NamedPipeReplServer.DisposeAsync rethrew a fault that had already stopped the endpoint accepting, because draining awaited Completion. A host reacting to that fault, as samples 05 and 10 do, then logged a critical shutdown failure and called Environment.Exit(1), skipping its own disposal. Disposal now waits for the accept loop without rethrowing its fault, which Completion and the callback already carry. - The listener's callback was named onConnectionError, although it now receives backend, terminal and endpoint failures, while RunAsync calls the same callback onError. Repl.Remote has not shipped, so the listener parameter is renamed onError, and sample 05 logs "Remote CLI failure". - A cleanup failure after DisposeAsync timed out still went only to the trace; it now reaches the callback too. The callback docs now say it may run for several connections at once. A new test covers a backend factory that throws: the callback receives the exception and the next connection is served. Removing the report makes it fail. The accept-fault path has no seam to inject a fault and remains untested.
GitHub ends a pwsh step with the last native command's exit code, so a .snupkg push that failed last made the step fail although the script treats symbol failures as warnings. The script now exits 0 after its own package check, and records package and symbol failures in separate lists instead of splitting one list by extension.
The ArgumentOutOfRangeException documentation said any undefined enum setting of options throws, but only SessionScope, and ProcessSignalHandling for a standalone run, are validated. It now names both. Two overloads without an options parameter inherited that exception: the WebSocket session's options-less RunAsync now inherits only the cancellation exception, and ReplApp.RunAsync(request, cancellationToken) has its own documentation.
- Live analysis: rows for a renamed option, expecting '--retries', and for an argument-only parameter, expecting its bare name, so neither "--" + parameter name nor a broken fallback passes. - WebSocketStream: the silent discard is pinned for CloseSent and Closed as well as Aborted; narrowing the guard to Aborted fails both. - Nested prepared run: the description no longer claims the test proves the outer input is restored, which the AsyncLocal guarantees anyway.
|
@autocarl please review |
Codex Review SummaryThis 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. |
Sample 10 opted out of Repl's signal handling with ProcessSignalHandling = None, then handled Ctrl+C itself through Console.CancelKeyPress and mapped the resulting cancellation to 130 by hand. That covered less than the CLI profile it already used: SIGTERM, which docker stop sends, and Ctrl+Break never cancelled a running client command. The launcher now runs with the profile's default, so Repl cancels the command cooperatively on any of them and an interrupted run exits 130 or 143. The console mode is unaffected, since it reads Ctrl+C as input and forwards it to the server. A new test sends a real SIGINT or SIGTERM to the launcher while exec waits on a server the test owns, and expects the remote command to be cancelled and the conventional exit code. Like the existing process signal tests it runs on Unix only, so it was not observed locally, where it is skipped.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Reviewed exact head 4afb343, including the new launcher signal commit. One confirmed P2 in publication failure accounting; see inline finding.
Completed validation: Release solution build 0 warnings / 0 errors; seven test suites 2,718 passed / 25 skipped / 0 failed, including real SIGINT/SIGTERM launcher tests. Additional accept-loop fault injection verifies the operator callback, faulted Completion, clean disposal and backend ownership, including a throwing callback. Baseline controls reproduce four option-token diagnostic failures and the stopped-server stack trace; argument-only controls remain green.
The PowerShell branch simulation passed, but its fake dotnet executable did not model automatic symbol upload. Independent review caught that coverage gap, and a real CLI/local HTTP reproduction confirms the inline issue. No external packages were published.
Independent static reviews covered the unchanged remote, input, docs and publishing changes at 3ddade7; parent inspected the new delta and reran the full build/suites at this head. Applicable CI is green. No further introduced P0-P2 confirmed. Requesting the small publishing correction before approval; no merge or product edits performed.
dotnet nuget push on a .nupkg also pushes the adjacent .snupkg unless --no-symbols is given. A failure of the symbol endpoint alone made that command exit non-zero after the package itself was published, so the package was counted as failed and the job failed, against the policy that a symbol failure is only a warning. The package loop now passes --no-symbols; the symbol loop pushes the .snupkg files on their own.
The cancellation table only said that stopping the server or container stops the host. It now names Ctrl+C, Ctrl+Break and SIGTERM, including docker stop, which Repl now owns in the launcher, and their exit codes.
|
@autocarl please review |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Targeted revalidation at 12fc35d: the previously requested publishing correction is verified fixed (see existing thread). No remaining technical blocker confirmed. Only workflow and architecture documentation changed since fully validated 4afb343; no C# rebuild or full-suite rerun was warranted. The new workflow was exercised with the real CLI/local HTTP endpoint for success, symbol-only failure and package failure; results are 0, 0 and 1 respectively. Previous C# validation was 2,718 passed / 25 skipped. Formal approval is held only for the current head CI jobs still in progress, not for another code correction. No merge or external package publication performed.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved at exact head 12fc35d. This supersedes my earlier REQUEST_CHANGES review: the automatic-symbol-upload P2 is fixed and verified, and the only remaining gate (current-head CI) is now successful, including Linux/Windows/macOS, documentation runtimes/container, Inspector, packaging and stress checks. No new code changes since the targeted revalidation, so no additional rebuild was performed. Prior unchanged-C# validation: Release solution build clean, 2,718 passed / 25 skipped. Updated publication script tested with the actual CLI against a local HTTP endpoint: full success exits 0, symbol-only failure exits 0, package failure exits 1. No remaining confirmed introduced P0-P2. No merge performed.
Follow-up to an ai-slop review of this branch: - The ArgumentOutOfRangeException sentence was pasted on 22 Run* overloads, and a reword already had to touch every copy. It now lives on ReplApp.RunAsync(string[], ReplRunOptions?, CancellationToken); the other overloads inherit that exception with the repository's existing inheritdoc path idiom. - The single-stream RemoteReplServer.RunAsync documents onError by pointing at the two-stream overload instead of repeating its text. - Sample 10's Program.cs comment and architecture overview point at the cancellation table, which alone states what each signal stops and the exit codes.
Fixes from the 0.12 release review panel that should land before
nbgv prepare-release. The items deferred to 0.13 are tracked in #166, #167, #168, #169 and #170.Changes
Repl.Remotefailures reach the operator. Backend method, terminal, connection and accept-loop failures were only written toSystem.Diagnostics.Trace, which no default .NET logging provider reads. They now go to the listener's error callback, andRemoteReplServer.RunAsyncgains overloads taking the same callback.onErroronNamedPipeReplServer, since it now receives more than connection failures.Repl.Remotehas not shipped.DisposeAsyncno longer rethrows a fault that already stopped the endpoint accepting. Before, a host reacting to that fault logged a critical shutdown failure and exited without disposing.Completion, as sample 10 does.docs/remote-cli.mdgains a "Failure reporting" section.servefails. A client command against a stopped server printed the one-line error, then aTimeoutExceptionstack trace.ProcessSignalHandling = Noneand handled only Ctrl+C throughConsole.CancelKeyPress, so SIGTERM fromdocker stopand Ctrl+Break never cancelled a running client command. It now uses the CLI profile's default: cooperative cancellation, exit130or143.WebSocketStream.WriteAsyncdiscards a write once the socket is closing or closed, as it has since 0.11; its remark claimed anIOException.Run*documents theArgumentOutOfRangeExceptionfor an undefinedSessionScopeorProcessSignalHandling..editorconfig; only those three bytes change.Not changed
WebSocketStream.WriteAsynckeeps discarding writes after close. Throwing would break callers shipped with 0.11, so the documentation was corrected instead.Test plan
WriteAsyncguard is narrowed toAborted, and when the nested-run clear is removed.