fix(shell): bound pipe read to MaxOutputChars before truncating (#1293)

Directory.Packages.props

@@ -84,6 +84,10 @@
     <PackageVersion Include="Aspire.Hosting.Testing" Version="$(AspireHostingVersion)" />
     <PackageVersion Include="CommunityToolkit.Aspire.Hosting.Ollama" Version="$(CommunityToolkitAspireVersion)" />
   </ItemGroup>
+  <!-- Benchmarks (benchmarks/Netclaw.Benchmarks only) -->
+  <ItemGroup>
+    <PackageVersion Include="BenchmarkDotNet" Version="0.15.8" />
+  </ItemGroup>
   <!-- Source generators -->
   <ItemGroup>
     <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="5.3.0" />

Netclaw.slnx

@@ -40,4 +40,7 @@
   <Folder Name="/tests/">
     <Project Path="tests/Netclaw.SmokeMcpServer/Netclaw.SmokeMcpServer.csproj" />
   </Folder>
+  <Folder Name="/benchmarks/">
+    <Project Path="benchmarks/Netclaw.Benchmarks/Netclaw.Benchmarks.csproj" />
+  </Folder>
 </Solution>

benchmarks/Netclaw.Benchmarks/Netclaw.Benchmarks.csproj

@@ -0,0 +1,26 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>net10.0</TargetFramework>
+    <OutputType>Exe</OutputType>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+    <AssemblyName>Netclaw.Benchmarks</AssemblyName>
+    <!-- BenchmarkDotNet requires Release and an optimized, non-debuggable build to
+         produce trustworthy numbers; building it as part of the solution in Debug is
+         fine (it just won't be run that way). It is a manual console tool, not a
+         published package. It inherits TreatWarningsAsErrors=true from the repo root
+         like every other project. -->
+    <IsPackable>false</IsPackable>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="BenchmarkDotNet" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\Netclaw.Actors\Netclaw.Actors.csproj" />
+    <ProjectReference Include="..\..\src\Netclaw.Configuration\Netclaw.Configuration.csproj" />
+  </ItemGroup>
+
+</Project>

benchmarks/Netclaw.Benchmarks/Program.cs

@@ -0,0 +1,14 @@
+// -----------------------------------------------------------------------
+// <copyright file="Program.cs" company="Petabridge, LLC">
+//      Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com>
+// </copyright>
+// -----------------------------------------------------------------------
+
+using BenchmarkDotNet.Running;
+
+// Usage:
+//   dotnet run -c Release --project benchmarks/Netclaw.Benchmarks                # pick interactively
+//   dotnet run -c Release --project benchmarks/Netclaw.Benchmarks -- --filter '*'  # run everything
+BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
+
+internal sealed partial class Program;

benchmarks/Netclaw.Benchmarks/ShellDrainBenchmarks.cs

@@ -0,0 +1,59 @@
+// -----------------------------------------------------------------------
+// <copyright file="ShellDrainBenchmarks.cs" company="Petabridge, LLC">
+//      Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com>
+// </copyright>
+// -----------------------------------------------------------------------
+
+using BenchmarkDotNet.Attributes;
+using Netclaw.Actors.Tools;
+
+namespace Netclaw.Benchmarks;
+
+/// <summary>
+/// Isolates the shell-output drain algorithm from the rest of <c>ShellTool</c>
+/// (no process spawn, no real pipe I/O) so the numbers reflect only the cost of
+/// turning a child's stdout stream into a bounded, redaction-ready string.
+///
+/// The comparison is the regression fix for #1293:
+///   <see cref="ShellTool.BoundedDrainAsync"/> (head+tail ring buffer, capped at
+///   read time) versus the old path — <see cref="System.IO.TextReader.ReadToEndAsync()"/>
+///   followed by <see cref="ShellTool.TruncateOutput"/>, which materialised the
+///   entire output before applying the cap. The story we want the numbers to tell
+///   is allocation shape: the old path is O(total output) and lands on the LOH for
+///   anything large; the new path is O(cap) regardless of how chatty the child is.
+/// </summary>
+[MemoryDiagnoser]
+public class ShellDrainBenchmarks
+{
+    // Production default for ToolConfig.MaxOutputChars. Kept as a literal here so
+    // the benchmark exercises the real cap without taking a Configuration dependency
+    // at measurement time.
+    private const int Cap = 32_000;
+
+    /// <summary>
+    /// Total characters the synthetic child "writes". Chosen to straddle the cap:
+    /// below it (no truncation), exactly at it, modestly over (LOH territory), and
+    /// far over (the pathological autonomous-log-pull case that triggered #1293).
+    /// </summary>
+    [Params(1_000, 32_000, 1_000_000, 50_000_000)]
+    public int TotalChars;
+
+    [Benchmark(Baseline = true, Description = "ReadToEnd + TruncateOutput (pre-#1293)")]
+    public async Task<int> ReadToEnd_ThenTruncate()
+    {
+        // Reader is constructed per-invocation because it is stateful (consumed as
+        // it is read). Its own allocation is a few dozen bytes and identical across
+        // both benchmarks, so it does not distort the head-to-head allocation story.
+        var reader = new SyntheticCharReader(TotalChars);
+        var all = await reader.ReadToEndAsync();
+        return ShellTool.TruncateOutput(all, Cap).Length;
+    }
+
+    [Benchmark(Description = "BoundedDrainAsync (post-#1293)")]
+    public async Task<int> BoundedDrain()
+    {
+        var reader = new SyntheticCharReader(TotalChars);
+        var (text, _) = await ShellTool.BoundedDrainAsync(reader, Cap);
+        return text.Length;
+    }
+}

benchmarks/Netclaw.Benchmarks/SyntheticCharReader.cs

@@ -0,0 +1,56 @@
+// -----------------------------------------------------------------------
+// <copyright file="SyntheticCharReader.cs" company="Petabridge, LLC">
+//      Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com>
+// </copyright>
+// -----------------------------------------------------------------------
+
+namespace Netclaw.Benchmarks;
+
+/// <summary>
+/// A <see cref="TextReader"/> that synthesises a fixed number of characters
+/// without ever holding the full payload in memory. It stands in for a chatty
+/// child process's stdout so the drain benchmark measures the algorithm's
+/// allocations and nothing else — the input itself allocates O(1), so anything
+/// the <c>[MemoryDiagnoser]</c> reports belongs to the code under test.
+///
+/// All reads complete synchronously: the goal is to feed the drain loop as fast
+/// as possible, not to model pipe latency.
+/// </summary>
+internal sealed class SyntheticCharReader(long total) : TextReader
+{
+    private const char Fill = 'x';
+    private long _produced;
+
+    public override int Read(char[] buffer, int index, int count)
+    {
+        var remaining = total - _produced;
+        if (remaining <= 0)
+            return 0;
+
+        var n = (int)Math.Min(count, remaining);
+        Array.Fill(buffer, Fill, index, n);
+        _produced += n;
+        return n;
+    }
+
+    public override int Read(Span<char> buffer)
+    {
+        var remaining = total - _produced;
+        if (remaining <= 0)
+            return 0;
+
+        var n = (int)Math.Min(buffer.Length, remaining);
+        buffer[..n].Fill(Fill);
+        _produced += n;
+        return n;
+    }
+
+    // BoundedDrainAsync reads via the ValueTask Memory<char> overload (below).
+    // The char[] Task overload is kept too so any reader path resolves to the
+    // synchronous fast path above and the benchmark measures the algorithm only.
+    public override ValueTask<int> ReadAsync(Memory<char> buffer, CancellationToken cancellationToken = default)
+        => ValueTask.FromResult(Read(buffer.Span));
+
+    public override Task<int> ReadAsync(char[] buffer, int index, int count)
+        => Task.FromResult(Read(buffer, index, count));
+}

src/Netclaw.Actors.Tests/Tools/ShellToolTests.cs

@@ -103,15 +103,16 @@ public async Task Caller_cancellation_kills_child_process_tree_and_returns_grace
     public async Task Output_truncation_applies()
     {
         var tool = new ShellTool(new ToolConfig { MaxOutputChars = 50 });
-        // Generate output longer than 50 chars — use cross-platform command
-        var command = OperatingSystem.IsWindows()
-            ? "python -c \"print('x' * 200)\""
-            : "printf 'x%.0s' {1..200}";
-        var args = ToolInput.Create("Command", command);
+        // Write >50 chars to stdout with a command that needs no interpreter.
+        // `echo` is a builtin on both bash and cmd.exe; a long literal is
+        // deterministic. (python on the Windows runner resolves to the Store
+        // stub, which writes to stderr — so the truncation would land on stderr
+        // and the marker would read "[stderr truncated", not "[stdout truncated".)
+        var args = ToolInput.Create("Command", $"echo {new string('x', 200)}");
 
         var result = await tool.ExecuteAsync(args, CancellationToken.None);
 
-        Assert.Contains("[output truncated]", result);
+        Assert.Contains("[stdout truncated", result);
     }
 
     [Fact]
@@ -366,6 +367,111 @@ public void TruncateOutput_truncates_and_appends_indicator()
         Assert.EndsWith("[output truncated]", result);
     }
 
+    // ── BoundedDrainAsync ──
+
+    [Fact]
+    public async Task BoundedDrain_short_output_returned_verbatim()
+    {
+        var input = "hello world";
+        var reader = new StringReader(input);
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100);
+        Assert.Equal(input, text);
+        Assert.False(truncated);
+    }
+
+    [Fact]
+    public async Task BoundedDrain_empty_input_returns_empty()
+    {
+        var reader = new StringReader("");
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100);
+        Assert.Equal("", text);
+        Assert.False(truncated);
+    }
+
+    [Fact]
+    public async Task BoundedDrain_output_exactly_at_cap_not_truncated()
+    {
+        var input = new string('a', 100);
+        var reader = new StringReader(input);
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 100);
+        Assert.Equal(input, text);
+        Assert.False(truncated);
+    }
+
+    [Fact]
+    public async Task BoundedDrain_long_output_truncated_with_head_and_tail()
+    {
+        // 100-char head marker + separator + 100-char tail marker, with filler in the middle
+        var head = new string('H', 100);
+        var middle = new string('M', 5000);
+        var tail = new string('T', 100);
+        var input = head + middle + tail;
+
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 200);
+
+        Assert.True(truncated);
+        Assert.StartsWith(new string('H', 100), text);  // head preserved
+        Assert.EndsWith(new string('T', 100), text);    // tail preserved
+        Assert.Contains("...", text);                    // separator present
+        Assert.DoesNotContain("M", text);                // middle discarded
+    }
+
+    [Fact]
+    public async Task BoundedDrain_head_and_tail_split_evenly()
+    {
+        // maxChars=10 → headCap=5, tailCap=5
+        var input = "AAAAAXXXXXXBBBBB"; // 16 chars: 5 head, 6 overflow discard, 5 tail
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 10);
+
+        Assert.True(truncated);
+        Assert.StartsWith("AAAAA", text);
+        Assert.EndsWith("BBBBB", text);
+    }
+
+    [Fact]
+    public async Task BoundedDrain_disabled_cap_returns_full_output()
+    {
+        var input = new string('x', 10_000);
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(new StringReader(input), 0);
+        Assert.Equal(input, text);
+        Assert.False(truncated);
+    }
+
+    [Fact]
+    public async Task BoundedDrain_tail_ring_wraps_across_small_chunks()
+    {
+        // Drives the ring's wraparound + start-advance path that the StringReader
+        // tests skip: each read delivers a chunk smaller than tailCap, so the tail
+        // window is rebuilt incrementally and must wrap rather than reset wholesale.
+        // maxChars=10 → headCap=5 ("ABCDE"), tailCap=5; last 5 of "FGHIJKLMNO" = "KLMNO".
+        var reader = new ChunkedReader("ABCDEFGHIJKLMNO", chunkSize: 3);
+
+        var (text, truncated) = await ShellTool.BoundedDrainAsync(reader, 10);
+
+        Assert.True(truncated);
+        Assert.Equal("ABCDE\n...\nKLMNO", text);
+    }
+
+    // Hands out at most chunkSize chars per read so tests can exercise the tail
+    // ring's incremental wrap path — real pipe reads arrive in arbitrary slices,
+    // not the single 4KB gulp a StringReader gives.
+    private sealed class ChunkedReader(string data, int chunkSize) : TextReader
+    {
+        private int _pos;
+
+        public override ValueTask<int> ReadAsync(Memory<char> buffer, CancellationToken cancellationToken = default)
+        {
+            var remaining = data.Length - _pos;
+            if (remaining <= 0)
+                return ValueTask.FromResult(0);
+
+            var n = Math.Min(Math.Min(chunkSize, buffer.Length), remaining);
+            data.AsSpan(_pos, n).CopyTo(buffer.Span);
+            _pos += n;
+            return ValueTask.FromResult(n);
+        }
+    }
+
     [Fact]
     public async Task Command_referencing_denied_path_returns_access_denied()
     {

src/Netclaw.Actors/Netclaw.Actors.csproj

@@ -9,6 +9,7 @@
   <ItemGroup>
     <InternalsVisibleTo Include="Netclaw.Actors.Tests" />
     <InternalsVisibleTo Include="Netclaw.Daemon.Tests" />
+    <InternalsVisibleTo Include="Netclaw.Benchmarks" />
   </ItemGroup>
 
   <ItemGroup>