Skip to content

Honour OutputHandler.Encoding regardless of a byte order mark - #77

Merged
matt-edmondson merged 2 commits into
mainfrom
claude/issue-76-output-encoding
Sep 24, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
claude/issue-76-output-encoding

Conversation

@matt-edmondson

@matt-edmondson matt-edmondson commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #76.

Two independent defects made OutputHandler.Encoding unreliable. Both are measured at main
(afa8bc1), .NET SDK 10.0.401, Linux.

1. A byte order mark replaced the requested encoding

AsyncProcessStreamReader read process.StandardOutput, and Process builds that StreamReader
with detectEncodingFromByteOrderMarks: true. Output beginning with a BOM was decoded with the
encoding the BOM named, not the one the caller asked for.

bytes on stdout, requested strict UTF-8 before after
FF FE 68 00 69 00 (UTF-16LE BOM + hi) hi, decoded as UTF-16LE, no error DecoderFallbackException
FE FF 00 68 00 69 (UTF-16BE BOM + hi) hi, decoded as UTF-16BE, no error DecoderFallbackException
FF FE alone exit 0, empty output, no error DecoderFallbackException
EF BB BF hello (UTF-8 BOM) hello hello — unchanged
FF alone DecoderFallbackException unchanged

The third row is what ktsu-dev/GitBranchStateCache#27 reported as a decode failure that fires
"only sometimes". It was not intermittent — those two bytes were eaten as a mark, leaving nothing
to decode.

The fix reads BaseStream with the caller's encoding and detectEncodingFromByteOrderMarks: false.
Turning detection off also turns off the BOM stripping that came with it, so
StripLeadingByteOrderMark drops a single leading U+FEFF from each stream's first chunk. Every
encoding decodes its own BOM to U+FEFF, so that covers all of them without guessing — and a BOM
belonging to a different encoding no longer decodes to U+FEFF, which is precisely the case that
should now surface as bad bytes. Row four shows the benign case is byte-for-byte unchanged.

2. A faulted read was discarded while the process was still running

The loop restarted whichever read had completed — and a faulted task is a completed one:

if (outputTask.IsCompleted)
{
    outputTask = ReadAndCallback(process.StandardOutput, ...);   // overwrites the failure
}

A decode failure raised while the process was still alive was overwritten by a fresh read, which
then returned 0 at EOF, and the call reported success. Whether the exception surfaced depended
purely on whether the process exited before the next pass.

This is not theoretical — it flaked this PR's own tests at about one run in six before I found
it. The loop now stops when either read has faulted, so Task.WhenAll rethrows. This applies to
any exception from a read or an OutputHandler callback, not only decoding.

AsyncProcessStreamReader now owns its two StreamReaders, so it implements IDisposable and
RunAsync holds it in a using. Task.WhenAll completes both reads before rethrowing, so neither
reader is disposed with a read in flight.

The test harness, and why it is shaped this way

The first push failed the Windows leg, and the cause was the harness rather than the fix:
cmd /c type transcodes a file carrying a UTF-16 byte order mark instead of 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 either, each test first confirms the bytes really reach the pipe
unchanged and reports the platform as inconclusive — quoting the bytes it actually saw — when they
do not. That control drives the emitter through Process directly and copies the raw BaseStream,
deliberately never touching the code under test: an earlier version of it went through
RunCommand, which meant that on unfixed source it measured the very defect these tests exist to
catch and blamed the emitter for it, turning a regression into an inconclusive result instead of a
failure. That was caught and fixed before this push.

Verification

  • Mutation-checked, each fix independently. Reverting both: 3 new tests fail. Reverting only
    the faulted-task guard: 2 fail. Both restored: all pass. So neither fix is carrying the other.
  • The control was mutation-checked too. With a deliberately mangling emitter it reports 0
    failures and 2 skips naming the exact bytes (Expected [FFFE], the pipe carried [FE]); with the
    source reverted it reports genuine failures and no emitter excuses. It cannot produce a false
    green or a false red.
  • The flake is gone. 20+ consecutive full-suite runs, 0 failures. Before the guard, 1 failure
    in 6; ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning makes that race deterministic and
    fails every time without the fix.
  • CI is green on all three platforms. Windows runs 39 tests, 0 failed, 36 succeeded, 3 skipped
    — the two pre-existing UAC-elevation tests plus the race test. The three byte-order-mark tests
    run and pass on Windows
    , so the PowerShell emitter is byte-faithful there and that coverage is
    real, not skipped.
  • Build: clean in Debug and Release across all target frameworks, 0 warnings.

Caveats

  • The race test is skipped on Windows via Assert.Inconclusive, because it needs a shell that can
    emit bytes and then stay alive. The code it covers is platform independent, so the Linux and
    macOS legs cover it.
  • SonarCloud's quality gate passes with 100% coverage on new code. It reports three INFO
    maintainability smells in the new tests, none posted as review comments: MSTEST0037 twice
    (Assert.Contains over Assert.IsTrue) and MSTEST0061 ([OSCondition] over an
    Assert.Inconclusive platform guard). Left alone rather than spending a CI cycle on them; happy
    to fold them in if this PR is touched again.

Not in this PR

Decode failures still reach the caller wrapped in AggregateException (from ReadCallback's
readTask.Result), so catch (DecoderFallbackException) does not catch one unchanged —
ktsu-dev/GitBranchStateCache#27 notes this. Unwrapping is a visible change to the exception
callers see, so it is left for its own decision. CommandOptions also still has no way to redirect
or close the child's stdin, which is the other half of what that issue needs.

🤖 Generated with Claude Code

https://claude.ai/code/session_014RUsrYfSAuSQ7VWws7vzsr

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RUsrYfSAuSQ7VWws7vzsr
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RUsrYfSAuSQ7VWws7vzsr
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OutputHandler.Encoding is silently overridden by a byte order mark, and decode failures are sometimes discarded

2 participants