Route bv narration to standard error, keeping standard output for deliverables

[rdeago]

Closes #315.

What changed

bv now follows the prevailing CLI convention (git, npm, cargo, kubectl, …): results go to standard output, narration goes to standard error, so results stay pipeable at any verbosity.

• ConsoleReporter writes all leveled diagnostics (error:/warning:/info:/detail:/trace:) and activity header/outcome lines to standard error. ChildOutput (child dotnet standard output — the payload of build commands) stays on standard output; ChildError was already on standard error.
• The startup logo also moves to standard error: technically a separate concern, but piped output isn't clean until it moves too. bv --version output and help remain standard-output deliverables.
• Color auto-detection now probes Console.IsErrorRedirected (plus NO_COLOR and, on Windows, whether virtual-terminal processing can be enabled on the standard error handle). --color/--no-color still override everything.

Design notes

• Labels are colored with raw ANSI escape sequences (new AnsiEscapes class) instead of Console.ForegroundColor. The BCL implements ForegroundColor against standard output — on Unix it emits the escape sequences to standard output — so with diagnostics on standard error it would corrupt a redirected deliverable stream (e.g. bv build > log). AnsiEscapes.Foreground(ConsoleColor) maps all 16 ConsoleColors to the SGR codes the BCL uses on Unix, so error:/warning: render exactly as before.
• New VirtualTerminal.TryEnableOnStandardError() enables ENABLE_VIRTUAL_TERMINAL_PROCESSING on the Windows standard error handle (ConPTY hosts like Windows Terminal already interpret VT; legacy conhost needs the explicit opt-in). On failure, auto-detected color turns off. Returns true outright on non-Windows.
• ConsoleReporter finally has unit tests (new Buildvana.Core.ConsoleOutput.Tests project, 43 tests): per-level stream routing, verbosity gating, label-only coloring, activity lines, child passthrough.

Breaking change

Scripts and CI steps that captured diagnostics from bv's standard output must now capture standard error (e.g. via 2>&1). Changelog entry added under Changes to existing features.

Coverage follow-up

Codecov flagged the patch coverage of the first push. Rather than just chase the number, this PR also establishes the project's coverage policy, now codified in .claude/rules/testing.md: testable code gets tested; code whose behavior is owned by the environment is excluded in source via [ExcludeFromCodeCoverage(Justification = ...)], never through Codecov configuration.

Applied here:

• AnsiEscapes.Foreground now computes the SGR code from the ConsoleColor bit layout (the same red/blue bit swap the BCL uses on Unix) instead of a 16-arm switch; new AnsiEscapesTests hold the literal 16-color table as an independent oracle, plus boundary out-of-range throws.
• --verbosity parsing moved from Program to a new VerbosityParser in the CommandLine layer, with tests pinning the accepted aliases and the error message.
• New ConsoleReporter tests cover the unknown-MessageLevel throw and the double-dispose idempotence of activity scopes.
• Excluded with justification: VirtualTerminal (P/Invoke over console state), Program (composition root), ConsoleReporter.DetectColor/IsNoColorSet (process-global console/environment probes).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.85%. Comparing base (ea43e46) to head (6770b62).

Files with missing lines	Patch %	Lines
...rc/Buildvana.Core.ConsoleOutput/ConsoleReporter.cs	88.88%	0 Missing and 1 partial ⚠️
src/Buildvana.Tool/CommandLine/VerbosityParser.cs	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
+ Coverage   47.27%   49.85%   +2.57%     
==========================================
  Files         126      130       +4     
  Lines        4044     4016      -28     
  Branches      714      714              
==========================================
+ Hits         1912     2002      +90     
+ Misses       2045     1925     -120     
- Partials       87       89       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 6m 17s —— View job

---

Review — routing bv narration to standard error

• Read the diff against origin/main
• Review AnsiEscapes / VirtualTerminal / ConsoleReporter
• Review VerbosityParser and Program
• Review tests, changelog, and coverage policy
• Post review

The core call is right, and the reasoning behind it is the interesting part of the PR: Console.ForegroundColor is implemented against standard output (on Unix it writes the escape sequence to stdout regardless of where the text goes), so moving diagnostics to stderr without moving off ForegroundColor would have quietly corrupted bv build > log. AnsiEscapes is the correct answer to that, and the bit-swap matches the BCL's ConsoleColor→ANSI index table exactly (Red 12 → 91, Yellow 14 → 93 — verified against the runtime's s_consoleColorToAnsiCode). Routing is consistent end to end: ChildOutput on stdout, everything else on stderr, deliverables (--version, help, bv version show) still on stdout via Spectre. CI is green (3/3), and I checked that nothing in this repo's workflows or docs parses bv's stdout, and that no server adapter emits ::-style workflow commands that would care about the stream.

One correctness issue, plus some smaller things.

---

Correctness

1. --color never enables virtual-terminal processing on Windows — src/Buildvana.Core.ConsoleOutput/ConsoleReporter.cs:48

_useColor = colorOverride ?? DetectColor();

VirtualTerminal.TryEnableOnStandardError() is reachable only from DetectColor(), i.e. only when colorOverride is null. Pass --color explicitly on legacy conhost (cmd.exe/PowerShell in the classic console host, where ConPTY isn't in play) and the reporter writes raw sequences into a buffer that doesn't interpret them: ←[91merror:←[0m something happened. Before this PR that path worked, because Console.ForegroundColor went through SetConsoleTextAttribute.

It may be masked in practice — Program.cs:43 touches AnsiConsole.Console, Spectre's capability detection sets ENABLE_VIRTUAL_TERMINAL_PROCESSING on the stdout handle, and that mode belongs to the console screen buffer, which stdout and stderr normally share — but the reporter shouldn't depend on a side effect of an unrelated library's detection pass. Attempting VT whenever color will be used is a one-liner; whether an explicit --color should still win when the enable fails is your call (I'd honor it — the user asked).

Fix this →

---

Robustness

2. Non-Windows assumes any terminal understands VT — src/Buildvana.Core.ConsoleOutput/VirtualTerminal.cs:34-37

TryEnableOnStandardError() returns true unconditionally off Windows, so auto-detected color emits escapes even under TERM=dumb or an unset TERM. Console.ForegroundColor used to consult terminfo and emit nothing when the terminal had no setaf. Narrow in practice (such environments usually redirect stderr, which DetectColor already checks), but a TERM check restores parity — and it belongs in the non-Windows branch of VirtualTerminal, since TERM is normally unset on Windows and a check in DetectColor would misfire there. Fix this →

3. A leveled line is up to five auto-flushed writes — ConsoleReporter.cs:154-172

Console.Error auto-flushes, so each diagnostic costs up to five write syscalls and the line is not atomic at the OS level. _writeLock serializes the reporter's own writes, but not the logo (Program.cs:70) or Spectre's stdout writes. Composing the line into one string and issuing a single WriteLine is cheaper and atomic; the existing tests assert on concatenated output, so they pass unchanged. Fix this →

---

Tests and coverage

The policy in .claude/rules/testing.md is sound, and the AnsiEscapes split — literal table in the test as oracle, computation in the implementation — is a genuinely good pattern: the test can't degenerate into a restatement of the code. The trade-off is that the bit arithmetic is less immediately readable than the switch it replaced, but the comment carries it.

Both partials Codecov still flags look closable with honest behavior tests rather than exclusions:

• EndActivity (ConsoleReporter.cs:178) — the Count > 0 && ReferenceEquals(...) false branch is reachable by disposing nested scopes out of order (outer.Dispose() before inner.Dispose()). That's real documented behavior worth pinning: the wrong scope must not be popped.
• ChildOutput/ChildError — no test covers "minimum verbosity specified and satisfied"; only the null and below-minimum cases exist. One test each closes the branch.
• VerbosityParser.Parse — Guard.IsNotNull(raw) has no test; Parse(null!) throwing ArgumentNullException is part of the contract.

Also missing for symmetry: ChildError_NoMinimumVerbosity_IgnoresVerbosity (the ChildOutput twin exists).

Optional, not for this PR: the tests have to swap Console.Out/Console.Error process-wide, hence [NotInParallel] plus the TUnit0055 suppression. An internal constructor taking two TextWriters (defaulting to the console) would be exactly the "small, honest design change" your own rule prefers, and would let these tests run in parallel. DetectColor's exclusion would stand either way.

---

Nits

• VerbosityParser.cs:22 — Guard.IsNotNull(raw) can throw ArgumentNullException, but only BuildFailedException is documented; add the second <exception>.
• AnsiEscapes.cs remarks — "looks the same as one rendered by setting Console.ForegroundColor" is true in effect, but on Unix the BCL emits terminfo-derived sequences (which for some terminals are not literally \e[9xm). "The same color indices" would be exact.
• VirtualTerminal.cs — SetLastError = true on all three imports, but no Marshal.GetLastWin32Error() call anywhere; harmless, just noise on GetStdHandle.
• Enabling VT mutates the shell's screen-buffer mode and never restores it. Standard practice for CLIs, so I'd leave it — worth a sentence in the remarks, since the current text says "per-screen-buffer" without noting the process outlives the change.

---

Also verified

• AnsiEscapes.Foreground matches the BCL table for all 16 colors, including the Gray→37 / DarkGray→90 boundary; out-of-range guard covers both ends.
• The MessageLevel(-1) test is valid: IsEnabled is (int)level <= (int)Verbosity, so -1 passes the gate and reaches StyleFor's throw.
• No query command emits its result through IReporter (only VersionShowCommand, via Spectre on stdout), so nothing that scripts consume moved to stderr by accident.
• CHANGELOG entry is accurate, correctly marked **BREAKING CHANGE**, and covers the color-detection consequence.

Nothing here blocks except item 1, which is a small fix.
· branch fix/315-reporter-stderr

[rdeago]

All findings addressed in five commits, one nit declined with rationale, and the optional refactor now has a home. Item by item:

1. --color never enables VT on Windows — fixed in 27af538. The constructor now attempts VirtualTerminal.TryEnableOnStandardError() whenever the override forces color on; auto-detection keeps folding the attempt into DetectColor. Judgment call taken as you leaned: the explicit override stays authoritative even when enabling fails — the user asked.

2. Non-Windows assumes any terminal understands VT — fixed in 230aaf3. The non-Windows branch now requires a usable TERM (set, non-empty, not dumb) as a cheap proxy for the terminfo lookup Console.ForegroundColor used to do. It lives in VirtualTerminal as you suggested, since a TERM check in DetectColor would misfire on Windows.

3. A leveled line is up to five auto-flushed writes — fixed in 4eec85b. WriteLeveledLine composes the whole line and issues a single WriteLine; output is byte-identical and the existing tests pass unchanged.

Tests and coverage — closed in 077b9ec:

• Out-of-order disposal is pinned through observable behavior: after disposing Outer while Inner is still open, a next sibling activity still nests at depth 2 ([2] Next: starting...), which blind popping would break; Outer's outcome line is asserted too.
• ChildOutput and ChildError each gained the satisfied-minimum case, and ChildError_NoMinimumVerbosity_IgnoresVerbosity completes the symmetry with its existing ChildOutput twin.
• VerbosityParser.Parse(null) throwing ArgumentNullException is pinned as part of the contract.

Nits — 8ae30c0:

• ArgumentNullException is now documented on Parse.
• The AnsiEscapes remarks claim color-index parity with Console.ForegroundColor rather than visual identity, noting the Unix BCL may emit terminfo-derived sequences instead of these literal codes.
• The VirtualTerminal remarks state that the mode change outlives the process and is deliberately not restored, per common CLI practice.
• Declined: dropping SetLastError = true. It stays on purpose — it documents the intended semantics of the imports and keeps error codes available should console misbehavior ever need diagnosing in the field. A comment above the imports now carries that rationale, so it no longer reads as leftover noise.

TextWriter injection — agreed on both counts: it is the honest design per our own testing rule, and it is not for this PR. Opened #328 so the follow-up actually exists instead of being politely mentioned by everyone and done by no one.

State after the five commits: 530 tests passing, zero warnings, ReSharper solution-wide inspection clean.

@claude please take another look.

[claude]

Claude finished @rdeago's task in 2m 55s —— View job

---

Re-review — follow-up commits

• Read the five follow-up commits against bd3ef3f
• Verify fix 1 (VT enabling under forced color)
• Verify fix 2 (TERM check off Windows)
• Verify fix 3 (single composed write)
• Verify new tests actually pin the claimed behavior
• Check docs/nit commit and CI state
• Post verdict

All three findings are genuinely fixed, and the tests added are behavior tests rather than coverage padding. CI is green on 8ae30c0 (3/3). Verification, item by item:

1 — forced color now enables VT (ConsoleReporter.cs:55-58). colorOverride == true attempts the enable and discards the result, so --color on legacy conhost gets a screen buffer that interprets the sequences, and still wins if the enable fails. The colorOverride == false case correctly skips the attempt (nothing to enable), and DetectColor still short-circuits before it when NO_COLOR is set or stderr is redirected. The XML doc on the parameter now states both halves of the rule. Correct.

2 — TERM gate (VirtualTerminal.cs:36-42). Unset, empty, or dumb → false, which flows into DetectColor only; an explicit --color still overrides, which is the right split. Placement in VirtualTerminal keeps the check from misfiring on Windows.

3 — single composed write (ConsoleReporter.cs:167-174). Byte-identical to the old sequence on both paths ({color}{word}:{reset} {message} vs. {word}: {message}, then one newline).

Tests. The out-of-order test is the real thing, not a coverage stub: the [2] Next: starting... assertion is what discriminates: had EndActivity popped blindly, outer.Dispose() would have popped Inner and inner.Dispose() would have popped Outer, leaving Next at depth 1. (The [1] Outer: done assertion alone wouldn't have caught it, since Depth is captured at construction.) ChildOutput/ChildError satisfied-minimum and the ChildError no-minimum twin close the gates on both sides, and Parse(null!) pins the guard.

---

One follow-on from fix 2

VirtualTerminal's class-level [ExcludeFromCodeCoverage] justification — "Thin wrapper over Win32 console-mode APIs … under a test runner standard error is redirected, so only the failure path would ever run" — was accurate when the non-Windows branch was return true;. It now describes a branch that no longer exists: on Linux CI, where these tests actually run, the only code that executes is the TERM check, which is neither a Win32 wrapper nor redirection-dependent. Your own rule says the justification must be phrased as why a test cannot honestly exercise this code, and this one no longer covers what it excludes.

Two honest options, and the rule prefers the second: extend the justification to name the environment read, or extract the decision as a pure helper (internal static bool IsTerminalCapable(string? term)) and test it, leaving the Environment.GetEnvironmentVariable call and the P/Invoke path excluded. The latter is three lines and covers the one bit of new logic that isn't environment-owned. Either is fine; the current state is the one that isn't. Fix this →

Two notes, no action needed

• WriteLeveledLine's comment claims the composed line "keeps the line atomic at the OS level". One WriteLine is one write only while the line fits Console's writer buffer (256 chars, AutoFlush on); a longer diagnostic — an MSBuild error easily is — still flushes in chunks. The change is still right (one syscall for the common case, fewer interleaving windows); the sentence just promises slightly more than it delivers. "usually a single write, and never five" would be exact.
• If Codecov still flags a partial at EndActivity, it's the _activityStack.Count > 0 half, and it's unreachable: Dispose is idempotent and a scope is popped only by its own EndActivity, so the stack always holds at least that scope when the method runs. It's a guard against a state the type can't reach — keep it, don't chase the number.

On the declined nit

SetLastError = true staying is fine, and the comment settles it. My point was only that it reads as leftover; documented intent is a better answer than removal.

Everything I raised last round is resolved. The VirtualTerminal justification is the only thing I'd still touch, and it's a comment or a three-line extraction — your call whether it rides along here or with #328.
· branch fix/315-reporter-stderr

[rdeago]

Follow-on from fix 2 — agreed on both the diagnosis and which option the coverage rule prefers: the justification described a branch that no longer existed, and the TERM check is exactly the "pure logic extracted out of plumbing" case the policy asks for.

6770b62 extracts the decision as VirtualTerminal.IsNonWindowsTerminalCapable(string?) (internal, exposed to the test project via InternalsVisibleTo) and tests it directly, including pinning the Ordinal comparison as intentional ("Dumb" counts as capable). The class-level exclusion is gone; what remains excluded is scoped to what is genuinely environment-owned:

• TryEnableOnStandardError — reads TERM off Windows, manipulates the attached console on Windows;
• the three P/Invoke imports — their attributes merge into the generator-emitted implementations, so the marshalling stubs (never executed off Windows) stay out of the coverage denominator rather than surfacing as uncovered lines once the class-level attribute is removed.

State: 535 tests passing, zero warnings, ReSharper inspection clean.

@claude please take another look.

[claude]

Claude finished @rdeago's task in 3m 4s —— View job

---

Re-review — 6770b62 (TERM capability extraction)

• Read 6770b62 against 8ae30c0
• Verify IsNonWindowsTerminalCapable extraction and call site
• Verify the new tests pin the claimed behavior
• Check the scoping of the remaining exclusions
• Post verdict

This closes it. The extraction is the option the rule asks for, it's behavior-preserving, and the exclusions now describe what they actually exclude.

The refactor carries no behavior. IsNonWindowsTerminalCapable is character-for-character the expression that was inlined in TryEnableOnStandardError (VirtualTerminal.cs:71), and the call site passes exactly what the old code read. Nothing got tightened or loosened under cover of a coverage fix — which is the failure mode worth checking for in a commit like this one.

The exclusion scoping is honest now. Off Windows TryEnableOnStandardError is a single delegation over one Environment.GetEnvironmentVariable; on Windows it's three console-mode calls. Both halves are environment-owned, and the justification names both. The P/Invoke exclusions are correctly placed on the defining partial declarations: attributes on the defining and implementing declarations are combined onto the emitted method, so the generated marshalling stubs — compiled into the assembly on every platform, executed on none but Windows — leave the denominator with them.

Where you stopped is the part I'd call out. DetectColor was not also split, and shouldn't be: !IsNoColorSet() && !Console.IsErrorRedirected && VirtualTerminal.TryEnableOnStandardError() is a composition of three environment probes, and extracting it as ShouldUseColor(bool, bool, bool) would produce a test that asserts a && b && c — precisely the coverage theater .claude/rules/testing.md names. The distinction that makes one extraction right and the other wrong is "does the pure part contain a decision that isn't visible from the signature", and the TERM interpretation does (dumb is a magic value; case-sensitivity is a choice) while boolean conjunction doesn't. The split lands on the right side of that line.

The test earns its place. All four value classes plus the one arm that isn't inferable from the signature: "Dumb" → capable. Ordinal is the correct call — terminfo entries are looked up as case-sensitive filenames, so a TERM that isn't literally dumb isn't the dumb terminal — and the comment says so rather than leaving a reader to assume OrdinalIgnoreCase was an oversight.

Mechanics check out. InternalsVisibleTo matches the convention already used by four projects here, and nothing in the repo sets SignAssembly, so no public-key dance. The test file carries the BOM and follows the no-namespace internal sealed shape of its siblings. CI's build/test/pack run on the latest push is green (CodeQL still in progress at the time of writing).

---

One note, no action

The attribute-merge reasoning is right for the stubs themselves. What I can't verify from here is whether the collector also drops the __PInvoke local function the LibraryImport generator nests inside each stub — attribution of compiler-generated members to an excluded parent is tool-specific. If a stray uncovered method ever surfaces under VirtualTerminal despite all this, that's what it is; not worth chasing, and it doesn't affect the correctness of what you wrote.

---

Nothing outstanding from any of the three rounds. Items 1–3 from the first review are fixed and verified, the coverage follow-on is resolved, and the TextWriter injection has a home in #328 rather than a mention in a comment thread.
· branch fix/315-reporter-stderr