fix(cli): keep json-mode stdout pure when the daemon sends an unknown output type - #1895
fix(cli): keep json-mode stdout pure when the daemon sends an unknown output type#1895Aaronontheweb wants to merge 2 commits into
Conversation
The report named tool_activity as an unknown daemon output type. This branch defines no such type. No session output type maps to it. No code path sends it to a client. The daemon emitter does not exist here. The headless --json channel already sent ErrorOutput text to stderr. It did not send that text to stdout. Stdout held only the JSON envelope before this change. This change adds a direct safeguard. The mapper now marks an unknown output type as a protocol diagnostic. A protocol diagnostic is not a session error. HeadlessChannel sends a protocol diagnostic to stderr, the session log, and the logger. A real session error keeps its prior path. Stdout in --json mode never carries a diagnostic line. Tests: - DaemonClientMappingTests: an unknown output type sets IsProtocolDiagnostic to true. A real daemon error keeps the flag false. - HeadlessChannelJsonStdoutHygieneTests: an unknown output type flows through a full headless turn. Stdout holds one parseable JSON envelope. Stderr and the logger both record the diagnostic.
7d92b83 to
8ddc98e
Compare
Aaronontheweb
changed the title
Aaronontheweb
added
bug
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Adversarial review pass (standard Netclaw workflow). The core fix is sound: json-mode stdout purity is genuinely proven (JsonSerializer.Deserialize throws on trailing content, so the one-envelope assertion is real), no genuine daemon error can be mislabeled today, and the ErrorOutput record change is low-risk (nothing compares or serializes it directly). Findings inline — two majors, three minors. No blocking issues on the headless path itself.
| /// (for example, an output type the client build does not know). | ||
| /// A channel adapter must keep a protocol diagnostic out of any | ||
| /// machine-readable result envelope and must not report it as a turn | ||
| /// error. Defaults to false for every daemon-originated error. |
There was a problem hiding this comment.
Major — this contract is only honored by HeadlessChannel.
The doc promise ("must not report it as a turn error") is implemented by exactly one adapter. The interactive TUI ignores the flag: ChatViewModel treats any ErrorOutput as end-of-generation (clears pending interactions, IsGenerating = false — line 142-146) and ChatPage renders it as a red [error] (line 473-476). So a newer daemon streaming an unknown output type to an interactive client still surfaces exactly the false failure this PR is meant to eliminate.
Pre-existing behavior, not a regression — but this PR ships a new public API whose documented contract is false outside headless mode. Suggest honoring the flag in ChatViewModel/ChatPage (non-terminal, warning-level) or narrowing the doc comment to what is actually implemented.
| TimestampMs = dto.TimestampMs, | ||
| Message = $"Unknown output type from daemon: {dto.Type}" | ||
| Message = $"Unknown output type from daemon: {dto.Type}", | ||
| IsProtocolDiagnostic = true |
There was a problem hiding this comment.
Major (latent) — ToDto silently drops this flag.
The ToDto ErrorOutput branch (same file, lines 107-116) never serializes IsProtocolDiagnostic and SessionOutputDto has no field for it. Any future relay/forwarding/re-broadcast of a client-mapped diagnostic (or a round-trip test) re-labels it as Type="error" with the flag false — exactly the vice-versa mislabeling this flag exists to prevent. The daemon never sets the flag today, so no live path loses it, but this is the canonical conversion point in the exact file this PR edits. Suggest plumbing the flag through the DTO or documenting the loss explicitly. (Side note: the client default branch never sets CorrelationId, so a diagnostic carries a fresh random GUID — cosmetic, but evidence this path wasn't designed for the DTO round trip.)
| // It still must not go silent: stderr, the session log, and | ||
| // the structured logger all see it. | ||
| _logger.LogWarning("Daemon protocol diagnostic: {Message}", msg.Message); | ||
| Console.Error.WriteLine($"[diagnostic] {msg.Message}"); |
There was a problem hiding this comment.
Minor — unannounced stderr tag change in non-json mode, untested.
This branch is not gated on _jsonOutput, so non-json chat -p now prints [diagnostic] Unknown output type from daemon: X where it previously printed [error] .... Scripts and the eval harness that parse stderr for [error] silently stop seeing unknown types as failures. That is presumably the intent, but it is a behavior change to a stderr contract with no test and no migration note — and the non-json path of this new branch has zero coverage. Suggest gating on _jsonOutput, adding a non-json test, or at least calling out the tag change in the PR description.
| Assert.DoesNotContain("[error]", stdoutText, StringComparison.Ordinal); | ||
|
|
||
| // The diagnostic is not silently dropped — stderr and the logger both see it. | ||
| Assert.Contains("Unknown output type from daemon: tool_activity", stderrText, StringComparison.Ordinal); |
There was a problem hiding this comment.
Minor — the "session log sees it" claim is not tested.
PR body says the diagnostic reaches stderr + session log + structured logger, but this test only asserts stderr (line 113) and the RecordingLogger (lines 114-115). The Log(log, $"DIAGNOSTIC: ...") call in HeadlessChannel (line 280) — the session-log claim — is never verified; the test never reads the log file. Suggest asserting the log file content (the NetclawPaths dir is right there in the fixture).
| // The diagnostic is not silently dropped — stderr and the logger both see it. | ||
| Assert.Contains("Unknown output type from daemon: tool_activity", stderrText, StringComparison.Ordinal); | ||
| Assert.Contains(logger.Messages, m => m.Contains("tool_activity", StringComparison.Ordinal)); | ||
| Assert.Contains(logger.Levels, l => l == LogLevel.Warning); |
There was a problem hiding this comment.
Minor — no negative-path channel test.
The branch ordering (ErrorOutput { IsProtocolDiagnostic: true } before case ErrorOutput) is only exercised for the true path. There is no channel-level test proving a genuine daemon error in json mode still keeps stdout pure and prints [error] to stderr — the mapper tests cover the flag value, but the channel routing of real errors is asserted only by implication. Also untested: multiple diagnostics in one turn, and a diagnostic arriving after turn_completed. Suggest one negative-path test.
1 participant
Problem
The client mapper turns an unknown output type from the daemon into an
ErrorOutput. Before this change, the mapper did not mark this case asdifferent from a genuine session error. An eval investigation found a
diagnostic of this kind in captured output. The diagnostic caused a false
failure in the test harness.
What This PR Does
IsProtocolDiagnosticflag onErrorOutput. The mapper setsthe flag to
truewhen it maps an unknown output type.HeadlessChannelfor a protocol diagnostic. Thebranch sends the diagnostic to stderr, to the session log, and to the
structured logger.
still writes to stderr through the existing route.
diagnostic never reaches stdout.
This PR treats a protocol diagnostic as a distinct case from a session
error. A protocol diagnostic reports a client-side read failure. A session
error reports a real failure inside the user's session. Stdout must keep
the second case. Stdout must never carry the first case.
What This PR Does NOT Do
This PR does not change the eval harness that captures stderr output. A
separate PR, #1896, fixes the harness-side stderr capture.
Tests
DaemonClientMappingTests: one test proves an unknown output type setsIsProtocolDiagnostictotrue. A second test proves a genuine daemonerror keeps the flag
false.HeadlessChannelJsonStdoutHygieneTests: an end-to-end test sends anunknown output type through a full headless turn against a fake daemon
transport. The test proves stdout holds one parseable JSON envelope. The
test also proves stderr and the logger both record the diagnostic.
Validation
dotnet test src/Netclaw.Cli.Tests: 1355 tests passed.dotnet test src/Netclaw.Actors.Tests: 3149 tests passed, 1 testskipped (a Windows-only PowerShell test on a Linux runner).
dotnet slopwatch analyze: 0 issues found../scripts/Add-FileHeaders.ps1 -Verify: all files pass.upstream/devat commitb1832884with noconflicts.
Review Note
This PR followed the skunkworks review process. This PR received standard
verification only. This PR did not receive a dedicated adversarial review
pass. The change is small. Tests cover the new behavior directly.