From f089716d4fc6b1e26926065a80717c09ffb54ba1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 12:48:36 +0000 Subject: [PATCH 1/2] Honour OutputHandler.Encoding regardless of a byte order mark Process builds its StandardOutput and StandardError readers with byte-order-mark detection switched on, so output beginning with a BOM was decoded with the encoding the BOM named rather than the one the caller asked for. Output starting FF FE came back as UTF-16LE text under a strict UTF-8 handler, and a lone FF FE was consumed as a mark, leaving a run that reported no output and no error for bytes it should have rejected. Read the raw pipes with the caller's encoding instead, and drop a leading U+FEFF so a mark matching that encoding is still stripped as it was before. Separately, the read loop restarted whichever read had completed, and a faulted task is a completed one. A decode failure raised while the process was still running was overwritten by a fresh read and never observed, which is what made such failures look intermittent: the repository's own suite lost one roughly every six runs. Stop the loop when either read has faulted so the exception reaches the caller. Four new tests cover both faults, and all four fail without the change. The existing 35 pass unchanged. Fixes #76 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014RUsrYfSAuSQ7VWws7vzsr --- RunCommand.Test/RunCommandTests.cs | 90 ++++++++++++++++++++++++ RunCommand/AsyncProcessStreamReader.cs | 95 +++++++++++++++++++++++--- RunCommand/RunCommand.cs | 2 +- 3 files changed, 175 insertions(+), 12 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 4d0c40b..0093989 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -3,6 +3,7 @@ namespace ktsu.RunCommand.Test; using System.Runtime.CompilerServices; +using System.Text; using System.Runtime.InteropServices; using ktsu.Semantics.Paths; @@ -610,4 +611,93 @@ await Assert.ThrowsAsync( EnvironmentVariables = new Dictionary { ["ANY"] = "value" }, })).ConfigureAwait(false); } + + /// + /// Returns a command that writes a file's bytes to standard output unchanged, as an executable + /// plus separate arguments. + /// + private static (string FileName, string[] Arguments) GetEmitFileBytesCommand(string path) => + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + ? ("cmd", ["/c", "type", path]) + : ("cat", [path]); + + private static readonly Encoding StrictUtf8 = new UTF8Encoding(false, throwOnInvalidBytes: true); + + // Writes the given bytes to a file of this test's own, then runs the command that echoes them + // back, so the test controls the exact bytes the child process puts on the pipe. + private static string RunOverBytes(byte[] bytes, [CallerMemberName] string caller = "") + { + string path = Path.Join(CreateDirectoryForTest(caller), "bytes.bin"); + File.WriteAllBytes(path, bytes); + + StringBuilder output = new(); + (string fileName, string[] arguments) = GetEmitFileBytesCommand(path); + _ = RunCommand.Execute(fileName, arguments, new OutputHandler(o => output.Append(o), null, StrictUtf8)); + + return output.ToString(); + } + + [TestMethod] + public void OutputEncodingIsNotReplacedByAUtf16ByteOrderMark() + { + // "hi" in UTF-16LE behind its byte order mark. Process builds its StandardOutput reader with + // byte-order-mark detection on, which used to switch the reader to UTF-16LE and return "hi" + // even though the caller asked for strict UTF-8 and these are not valid UTF-8 bytes. + byte[] bytes = [0xFF, 0xFE, (byte)'h', 0x00, (byte)'i', 0x00]; + + AggregateException thrown = Assert.ThrowsExactly(() => RunOverBytes(bytes)); + Assert.IsTrue( + thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), + $"Expected a decode failure, got: {thrown}"); + } + + [TestMethod] + public void OutputThatIsOnlyAUtf16ByteOrderMarkIsNotReportedAsSuccess() + { + // The same detection consumed a lone FF FE as a byte order mark, leaving nothing to decode, + // so a strict encoding reported no output and no error for bytes it should have rejected. + byte[] bytes = [0xFF, 0xFE]; + + AggregateException thrown = Assert.ThrowsExactly(() => RunOverBytes(bytes)); + Assert.IsTrue( + thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), + $"Expected a decode failure, got: {thrown}"); + } + + [TestMethod] + public void AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput() + { + // A byte order mark matching the requested encoding is still dropped, so turning the + // detection off did not start leaking U+FEFF into captured output. + byte[] bytes = [0xEF, 0xBB, 0xBF, (byte)'h', (byte)'e', (byte)'l', (byte)'l', (byte)'o']; + + Assert.AreEqual("hello", RunOverBytes(bytes)); + } + + [TestMethod] + public void ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Assert.Inconclusive("Needs a shell that can emit bytes and then stay alive. The race this covers is in platform independent code, so the other legs cover it."); + } + + // The read loop stops when the process exits, so a read that faults while the process is + // still running used to be replaced by a fresh read on the next pass and its decode failure + // thrown away. Sleeping after the bad byte keeps the process alive long enough for that pass + // to happen, which makes the race deterministic rather than roughly one run in six. + string path = Path.Join(CreateDirectoryForTest(), "bytes.bin"); + File.WriteAllBytes(path, [(byte)'o', (byte)'k', 0xFF]); + + StringBuilder output = new(); + AggregateException thrown = Assert.ThrowsExactly( + () => RunCommand.Execute( + "sh", + ["-c", $"cat '{path}'; sleep 1"], + new OutputHandler(o => output.Append(o), null, StrictUtf8))); + + Assert.IsTrue( + thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), + $"Expected a decode failure, got: {thrown}"); + } } diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index 174e383..1a8c809 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -4,11 +4,34 @@ namespace ktsu.RunCommand; using System.Diagnostics; -internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) +internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) : IDisposable { + private const char ByteOrderMark = '\uFEFF'; + private readonly char[] outputBuffer = new char[4096]; private readonly char[] errorBuffer = new char[4096]; + private bool outputHasEmitted; + private bool errorHasEmitted; + + // Read the raw pipes with the caller's encoding rather than through + // process.StandardOutput/StandardError. Process builds those StreamReaders with byte-order-mark + // detection switched on, which replaces the encoding the caller asked for whenever the output + // happens to begin with a BOM. Output starting with FF FE was decoded as UTF-16LE no matter what + // OutputHandler.Encoding said, and a strict encoding reported no error on bytes it should have + // rejected. + private readonly StreamReader outputStream = + new(process.StandardOutput.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false); + + private readonly StreamReader errorStream = + new(process.StandardError.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false); + + public void Dispose() + { + outputStream.Dispose(); + errorStream.Dispose(); + } + internal async Task Start() { Task outputTask = Task.CompletedTask; @@ -17,14 +40,23 @@ internal async Task Start() // Continuously read until the process has exited. do { + // A faulted task is a completed one, so without this the checks below would replace a + // failed read with a fresh one and the failure it carries would never be observed. That + // made a decode error on a long-running command a coin toss: it surfaced only when the + // process happened to exit before the loop came back around. + if (outputTask.IsFaulted || errorTask.IsFaulted) + { + break; + } + if (outputTask.IsCompleted) { - outputTask = ReadAndCallback(process.StandardOutput, outputBuffer, outputHandler.HandleStandardOutputData); + outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); } if (errorTask.IsCompleted) { - errorTask = ReadAndCallback(process.StandardError, errorBuffer, outputHandler.HandleStandardErrorData); + errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); } await Task.WhenAny(outputTask, errorTask).ConfigureAwait(false); @@ -34,24 +66,65 @@ internal async Task Start() await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); // Read any remaining data after process exit. - outputTask = ReadAndCallback(process.StandardOutput, outputBuffer, outputHandler.HandleStandardOutputData); - errorTask = ReadAndCallback(process.StandardError, errorBuffer, outputHandler.HandleStandardErrorData); + outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); + errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); } - private static async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData) => + private async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData, bool isStandardOutput) => await streamReader.ReadAsync(buffer, 0, buffer.Length) - .ContinueWith(t => ReadCallback(t, buffer, onData), TaskScheduler.Current) + .ContinueWith(t => ReadCallback(t, buffer, onData, isStandardOutput), TaskScheduler.Current) .ConfigureAwait(false); - private static void ReadCallback(Task readTask, char[] buffer, Action? onData) + private void ReadCallback(Task readTask, char[] buffer, Action? onData, bool isStandardOutput) { - int bytesRead = readTask.Result; + int charsRead = readTask.Result; + + if (charsRead <= 0) + { + return; + } - if (bytesRead > 0) + string data = new(buffer, 0, charsRead); + data = StripLeadingByteOrderMark(data, isStandardOutput); + + if (data.Length > 0) { - string data = new(buffer, 0, bytesRead); onData?.Invoke(data); } } + + /// + /// Drops a byte order mark from the front of a stream's first chunk. + /// + /// + /// Turning off the detection above also turned off the stripping that came with it, and a + /// leading U+FEFF in captured output is a change no caller asked for. Every encoding decodes its + /// own BOM to U+FEFF, so dropping that one character covers each of them without guessing at the + /// encoding. A BOM belonging to a different encoding no longer decodes to U+FEFF, which is + /// exactly the case that should surface as mis-decoded bytes rather than be silently honoured. + /// + /// The chunk just read. + /// Whether the chunk came from standard output. + /// The chunk, less a leading byte order mark if this was the stream's first. + private string StripLeadingByteOrderMark(string data, bool isStandardOutput) + { + bool hasEmitted = isStandardOutput ? outputHasEmitted : errorHasEmitted; + + if (isStandardOutput) + { + outputHasEmitted = true; + } + else + { + errorHasEmitted = true; + } + + if (hasEmitted || data[0] != ByteOrderMark) + { + return data; + } + + return data[1..]; + } } diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index bf0b386..bcf37bb 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -462,7 +462,7 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle } else { - AsyncProcessStreamReader outputReader = new(process, outputHandler); + using AsyncProcessStreamReader outputReader = new(process, outputHandler); await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); } From 9734b07d6dd49136b23f923eb2ddebea748e4ead Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 13:00:39 +0000 Subject: [PATCH 2/2] Make the byte-emitting test harness work on Windows and self-diagnose The Windows CI leg failed the two byte-order-mark tests. The cause was the harness, not the fix: cmd /c type transcodes a file carrying a UTF-16 byte order mark rather than copying its bytes, so the invalid bytes never reached the decoder and no failure was raised. PowerShell now writes the raw bytes to the standard output stream instead. Rather than trust that emitter too, each test first confirms the bytes really reach the pipe unchanged, and reports the platform as inconclusive with the bytes it saw when they do not. That check drives the emitter through Process directly and copies the raw BaseStream, never touching the code under test: a control that went through RunCommand would measure the very defect these tests exist to catch and blame the emitter for it, turning a regression into an inconclusive result instead of a failure. Verified both ways, with the source reverted and with a deliberately mangling emitter. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014RUsrYfSAuSQ7VWws7vzsr --- RunCommand.Test/RunCommandTests.cs | 126 +++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 25 deletions(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 0093989..8fb5335 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -2,6 +2,7 @@ namespace ktsu.RunCommand.Test; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Text; using System.Runtime.InteropServices; @@ -616,62 +617,137 @@ await Assert.ThrowsAsync( /// Returns a command that writes a file's bytes to standard output unchanged, as an executable /// plus separate arguments. /// + /// + /// Windows has no reliably byte-faithful built-in for this. cmd /c type looked like one + /// but transcodes a file carrying a UTF-16 byte order mark instead of copying it, which is + /// exactly the input these tests need, so PowerShell writes the raw bytes to the standard + /// output stream instead. checks the result rather than + /// trusting it. + /// private static (string FileName, string[] Arguments) GetEmitFileBytesCommand(string path) => RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - ? ("cmd", ["/c", "type", path]) + ? ("powershell", [ + "-NoProfile", + "-NonInteractive", + "-Command", + $"$b=[IO.File]::ReadAllBytes('{path}'); $s=[Console]::OpenStandardOutput(); $s.Write($b,0,$b.Length); $s.Flush()"]) : ("cat", [path]); private static readonly Encoding StrictUtf8 = new UTF8Encoding(false, throwOnInvalidBytes: true); - // Writes the given bytes to a file of this test's own, then runs the command that echoes them - // back, so the test controls the exact bytes the child process puts on the pipe. - private static string RunOverBytes(byte[] bytes, [CallerMemberName] string caller = "") + // Runs the emitter through the library, so the test controls the exact bytes that reach the + // pipe and can assert on what the library makes of them. + private static string RunOverPath(string path, Encoding encoding) { - string path = Path.Join(CreateDirectoryForTest(caller), "bytes.bin"); - File.WriteAllBytes(path, bytes); - StringBuilder output = new(); (string fileName, string[] arguments) = GetEmitFileBytesCommand(path); - _ = RunCommand.Execute(fileName, arguments, new OutputHandler(o => output.Append(o), null, StrictUtf8)); + _ = RunCommand.Execute(fileName, arguments, new OutputHandler(o => output.Append(o), null, encoding)); return output.ToString(); } - [TestMethod] - public void OutputEncodingIsNotReplacedByAUtf16ByteOrderMark() + /// + /// Checks that this platform's emitter really does put on the pipe + /// unchanged, so that a mangling emitter reports itself instead of being read as a result about + /// the library. + /// + /// + /// This drives the emitter through directly and copies the raw + /// , deliberately never touching the code under test. A + /// control that went through would measure the very defect these tests + /// exist to catch and blame the emitter for it, which would turn a regression into an + /// inconclusive result instead of a failure. + /// + private static bool EmitsBytesFaithfully(string path, byte[] bytes, out string diagnostic) { - // "hi" in UTF-16LE behind its byte order mark. Process builds its StandardOutput reader with - // byte-order-mark detection on, which used to switch the reader to UTF-16LE and return "hi" - // even though the caller asked for strict UTF-8 and these are not valid UTF-8 bytes. - byte[] bytes = [0xFF, 0xFE, (byte)'h', 0x00, (byte)'i', 0x00]; + byte[] actual; - AggregateException thrown = Assert.ThrowsExactly(() => RunOverBytes(bytes)); - Assert.IsTrue( - thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), - $"Expected a decode failure, got: {thrown}"); + try + { + (string fileName, string[] arguments) = GetEmitFileBytesCommand(path); + ProcessStartInfo startInfo = new() + { + FileName = fileName, + RedirectStandardOutput = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + + foreach (string argument in arguments) + { + startInfo.ArgumentList.Add(argument); + } + + using Process process = Process.Start(startInfo)!; + using MemoryStream captured = new(); + process.StandardOutput.BaseStream.CopyTo(captured); + process.WaitForExit(); + actual = captured.ToArray(); + } + catch (System.ComponentModel.Win32Exception ex) + { + diagnostic = $"This platform's byte emitter could not be started: {ex.Message}"; + return false; + } + + bool faithful = actual.SequenceEqual(bytes); + diagnostic = faithful + ? string.Empty + : "This platform's byte emitter altered the bytes, so the library cannot be judged from them. " + + $"Expected [{Convert.ToHexString(bytes)}], the pipe carried [{Convert.ToHexString(actual)}]."; + + return faithful; } - [TestMethod] - public void OutputThatIsOnlyAUtf16ByteOrderMarkIsNotReportedAsSuccess() + // Writes the bytes to a file of this test's own and confirms the platform can actually put them + // on a pipe unchanged, returning the path to feed to the library. + private static string WriteBytesForTest(byte[] bytes, string caller) { - // The same detection consumed a lone FF FE as a byte order mark, leaving nothing to decode, - // so a strict encoding reported no output and no error for bytes it should have rejected. - byte[] bytes = [0xFF, 0xFE]; + string path = Path.Join(CreateDirectoryForTest(caller), "bytes.bin"); + File.WriteAllBytes(path, bytes); + + if (!EmitsBytesFaithfully(path, bytes, out string diagnostic)) + { + Assert.Inconclusive(diagnostic); + } + + return path; + } - AggregateException thrown = Assert.ThrowsExactly(() => RunOverBytes(bytes)); + private static void AssertDecodeFailure(byte[] bytes, [CallerMemberName] string caller = "") + { + // The emitter check has to happen out here: an Assert.Inconclusive raised inside the lambda + // below would be caught by Assert.ThrowsExactly and reported as a failing test. + string path = WriteBytesForTest(bytes, caller); + + AggregateException thrown = Assert.ThrowsExactly(() => RunOverPath(path, StrictUtf8)); Assert.IsTrue( thrown.Flatten().InnerExceptions.Any(e => e is DecoderFallbackException), $"Expected a decode failure, got: {thrown}"); } + [TestMethod] + public void OutputEncodingIsNotReplacedByAUtf16ByteOrderMark() => + // "hi" in UTF-16LE behind its byte order mark. Process builds its StandardOutput reader with + // byte-order-mark detection on, which used to switch the reader to UTF-16LE and return "hi" + // even though the caller asked for strict UTF-8 and these are not valid UTF-8 bytes. + AssertDecodeFailure([0xFF, 0xFE, (byte)'h', 0x00, (byte)'i', 0x00]); + + [TestMethod] + public void OutputThatIsOnlyAUtf16ByteOrderMarkIsNotReportedAsSuccess() => + // The same detection consumed a lone FF FE as a byte order mark, leaving nothing to decode, + // so a strict encoding reported no output and no error for bytes it should have rejected. + AssertDecodeFailure([0xFF, 0xFE]); + [TestMethod] public void AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput() { // A byte order mark matching the requested encoding is still dropped, so turning the // detection off did not start leaking U+FEFF into captured output. byte[] bytes = [0xEF, 0xBB, 0xBF, (byte)'h', (byte)'e', (byte)'l', (byte)'l', (byte)'o']; + string path = WriteBytesForTest(bytes, nameof(AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput)); - Assert.AreEqual("hello", RunOverBytes(bytes)); + Assert.AreEqual("hello", RunOverPath(path, StrictUtf8)); } [TestMethod]