-
Notifications
You must be signed in to change notification settings - Fork 28
fix(cli): keep json-mode stdout pure when the daemon sends an unknown output type #1895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,16 @@ public sealed record ErrorOutput : SessionOutput | |
| /// for diagnostic logging by subscribers and adapters. | ||
| /// </summary> | ||
| public Exception? Cause { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// True when this instance is not a real session error. The client | ||
| /// mapper sets it when it cannot read a wire message from the daemon | ||
| /// (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. | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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 |
||
| /// </summary> | ||
| public bool IsProtocolDiagnostic { get; init; } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| // ----------------------------------------------------------------------- | ||
| // <copyright file="HeadlessChannelJsonStdoutHygieneTests.cs" company="Petabridge, LLC"> | ||
| // Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com> | ||
| // </copyright> | ||
| // ----------------------------------------------------------------------- | ||
| using System.Text.Json; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Time.Testing; | ||
| using Netclaw.Actors.Protocol; | ||
| using Netclaw.Cli.Daemon; | ||
| using Netclaw.Configuration; | ||
| using Netclaw.Tests.Utilities; | ||
| using Xunit; | ||
|
|
||
| namespace Netclaw.Cli.Tests.Cli; | ||
|
|
||
| /// <summary> | ||
| /// Proves that <c>chat -p --json</c> keeps stdout pure JSON when the daemon | ||
| /// sends an output type this CLI build does not recognize (for example a | ||
| /// newer daemon streaming "tool_activity" to an older client). The client | ||
| /// mapper turns the unrecognized type into a diagnostic <c>ErrorOutput</c> | ||
| /// (<see cref="SessionOutputDtoMapper.FromDto"/>); this suite proves the | ||
| /// headless channel keeps that diagnostic off stdout, still surfaces it | ||
| /// (stderr + logger), and still emits a parseable JSON envelope. | ||
| /// </summary> | ||
| [Collection("Update verification")] | ||
| public sealed class HeadlessChannelJsonStdoutHygieneTests : IDisposable | ||
| { | ||
| private static readonly TimeSpan[] ImmediateDelays = [TimeSpan.Zero]; | ||
|
|
||
| private readonly DisposableTempDir _dir = new(); | ||
| private readonly TextWriter _originalOut = Console.Out; | ||
| private readonly TextWriter _originalError = Console.Error; | ||
|
|
||
| public void Dispose() | ||
| { | ||
| Console.SetOut(_originalOut); | ||
| Console.SetError(_originalError); | ||
| _dir.Dispose(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task Unrecognized_output_type_stays_off_stdout_and_envelope_stays_parseable() | ||
| { | ||
| var transport = new FakeDaemonHubTransport(); | ||
|
|
||
| // Model a daemon that streams an output type this CLI build does | ||
| // not know about (e.g. "tool_activity") ahead of the real reply. | ||
| transport.VoidInvokeHook = (method, args, _) => | ||
| { | ||
| if (method == "SendMessage") | ||
| { | ||
| var sessionId = (string)args[0]!; | ||
| transport.PushOutput(new SessionOutputDto | ||
| { | ||
| Type = "tool_activity", | ||
| SessionId = sessionId, | ||
| TimestampMs = 1 | ||
| }); | ||
| transport.PushOutput(new SessionOutputDto | ||
| { | ||
| Type = "text", | ||
| SessionId = sessionId, | ||
| TimestampMs = 2, | ||
| Text = "hello" | ||
| }); | ||
| transport.PushOutput(new SessionOutputDto | ||
| { | ||
| Type = "turn_completed", | ||
| SessionId = sessionId, | ||
| TimestampMs = 3, | ||
| TurnNumber = new TurnNumber(1) | ||
| }); | ||
| } | ||
|
|
||
| return Task.CompletedTask; | ||
| }; | ||
|
|
||
| await using var daemonClient = new DaemonClient( | ||
| "http://localhost", | ||
| transport, | ||
| reconnectDelays: ImmediateDelays, | ||
| rpcTimeout: TimeSpan.FromSeconds(5)); | ||
|
|
||
| var paths = new NetclawPaths(_dir.Path); | ||
| var lifetime = new RecordingHostLifetime(); | ||
| var logger = new RecordingLogger<HeadlessChannel>(); | ||
| var options = new HeadlessOptions("hi") { JsonOutput = true }; | ||
|
|
||
| var channel = new HeadlessChannel( | ||
| daemonClient, paths, lifetime, new FakeTimeProvider(), options, logger); | ||
|
|
||
| var stdout = new StringWriter(); | ||
| var stderr = new StringWriter(); | ||
| Console.SetOut(stdout); | ||
| Console.SetError(stderr); | ||
|
|
||
| await channel.StartAsync(TestContext.Current.CancellationToken); | ||
| await lifetime.StopRequested.Task.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); | ||
|
|
||
| var stdoutText = stdout.ToString(); | ||
| var stderrText = stderr.ToString(); | ||
|
|
||
| // Stdout carries exactly the JSON envelope — nothing else. | ||
| var envelope = JsonSerializer.Deserialize<JsonElement>(stdoutText.Trim()); | ||
| Assert.Equal("hello", envelope.GetProperty("response").GetString()); | ||
| Assert.DoesNotContain("Unknown output type", stdoutText, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("[diagnostic]", stdoutText, StringComparison.Ordinal); | ||
| 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); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| Assert.Contains(logger.Messages, m => m.Contains("tool_activity", StringComparison.Ordinal)); | ||
| Assert.Contains(logger.Levels, l => l == LogLevel.Warning); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor — no negative-path channel test. The branch ordering ( |
||
| } | ||
|
|
||
| private sealed class RecordingHostLifetime : IHostApplicationLifetime | ||
| { | ||
| public TaskCompletionSource StopRequested { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| public CancellationToken ApplicationStarted => CancellationToken.None; | ||
|
|
||
| public CancellationToken ApplicationStopping { get; } = CancellationToken.None; | ||
|
|
||
| public CancellationToken ApplicationStopped => CancellationToken.None; | ||
|
|
||
| public void StopApplication() => StopRequested.TrySetResult(); | ||
| } | ||
|
|
||
| private sealed class RecordingLogger<T> : ILogger<T> | ||
| { | ||
| public List<string> Messages { get; } = []; | ||
|
|
||
| public List<LogLevel> Levels { get; } = []; | ||
|
|
||
| public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null; | ||
|
|
||
| public bool IsEnabled(LogLevel logLevel) => true; | ||
|
|
||
| public void Log<TState>( | ||
| LogLevel logLevel, | ||
| EventId eventId, | ||
| TState state, | ||
| Exception? exception, | ||
| Func<TState, Exception?, string> formatter) | ||
| { | ||
| Levels.Add(logLevel); | ||
| Messages.Add(formatter(state, exception)); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -269,6 +269,17 @@ private void HandleOutput(SessionOutput output, StreamWriter? log) | |
| Log(log, $"USAGE: in={msg.InputTokens} out={msg.OutputTokens} total={msg.TotalTokens} cached={msg.CachedInputTokens} reasoning={msg.ReasoningTokens} context_window={msg.ContextWindowTokens} prompt_ms={msg.PromptMs} predicted_tok_s={msg.PredictedPerSecond}"); | ||
| break; | ||
|
|
||
| case ErrorOutput { IsProtocolDiagnostic: true } msg: | ||
| // The client could not read a daemon wire message (unknown | ||
| // output type). This is not a turn result, so it must never | ||
| // reach stdout — json mode stdout carries only the envelope. | ||
| // 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}"); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor — unannounced stderr tag change in non-json mode, untested. This branch is not gated on |
||
| Log(log, $"DIAGNOSTIC: {msg.Message}"); | ||
| break; | ||
|
|
||
| case ErrorOutput msg: | ||
| Console.Error.WriteLine($"[error] {msg.Message}"); | ||
| Log(log, $"ERROR: {msg.Message}"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major (latent) —
ToDtosilently drops this flag.The
ToDtoErrorOutputbranch (same file, lines 107-116) never serializesIsProtocolDiagnosticandSessionOutputDtohas no field for it. Any future relay/forwarding/re-broadcast of a client-mapped diagnostic (or a round-trip test) re-labels it asType="error"with the flagfalse— 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 setsCorrelationId, so a diagnostic carries a fresh random GUID — cosmetic, but evidence this path wasn't designed for the DTO round trip.)