Replace bv's logging with Buildvana.Core.ConsoleOutput

[rdeago]

Summary

• Adds Buildvana.Core.ConsoleOutput: a System.Console-backed IReporter (contracts in Buildvana.Core.Abstractions, BCL-only) that replaces bv's misuse of Microsoft.Extensions.Logging for console output.
• Messages now render as clean, color-coded lines with no class-name category; dotnet/MSBuild output streams through live (stdout→stdout, stderr→stderr) instead of being hidden unless the build fails. Work is grouped into depth-tagged activities ([1] Build … [1] Build: done (3.2s)).
• Removes the SpectreLogger bridge and Microsoft.Extensions.Logging(.Abstractions) from the tool; Buildvana.Sdk.Tasks keeps its MSBuild bridge with an explicit M.E.Logging.Abstractions reference.

Notable design choices (reviewer context)

• Palette: foreground-only, theme-adaptive — error: red, warning: yellow, info:/detail:/trace: uncolored; labels ragged (single space). NO_COLOR honored; --color overrides it.
• Activities: IActivityScope.Complete() gates the outcome line (no "done" on exception); both the starting and outcome line render at Info; an instance stack supplies the [depth] tag.
• GitService was not given an activity (its logging is all-Detail discovery + silent ops); a "Git" activity likely belongs at the callers later.
• Ctrl-C now reports via error: (always visible) rather than the old yellow line.

Additional changes (not in the issue)

• Improved overall handling of dotnet invocations in DotNetService.
• Removed dotnet parameters of non-general use (they will be added in this repo's buildvana.jsonc file when it is created).
• Added DI support for Lazy<T>: an injected Lazy<T> will resolve T from the service provider when evaluated. This lets us inject Lazy<NuGetPushConfiguration> into DotNetService instead of the service provider itself.

Test plan

• dotnet bv pack — 0 errors, 0 warnings, tests pass, artifacts produced
• ReSharper inspectcode --severity=WARNING — 0 results
• Ran the locally-built tool against a throwaway repo: verified activity headers + depth, live MSBuild streaming, and the error: path

Closes #286

🤖 Generated with Claude Code

[rdeago]

@claude please review this PR.

[claude]

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

---

PR Review

• Gather context from PR description and changed files list
• Read abstractions: IReporter, IActivityScope, MessageLevel, Verbosity, null stubs, extensions
• Read implementation: ConsoleReporter, ConsoleReporter.ActivityScope
• Read migration: Program.cs, BuildPipeline.cs, services
• Read supporting changes: IProcessRunner, ProcessRunner, project files
• Analyze for correctness, design, style, and C# guidelines compliance
• Post review findings

---

This is a well-crafted PR that cleanly separates console-output concerns from Microsoft.Extensions.Logging and builds a purpose-built reporter hierarchy. The design is sound, the thread-safety model is correct, and the migration is systematic. No correctness bugs found. My observations are below.

---

Design observations

Activity "done" at Detail vs "begin" at Info

At Normal verbosity, users see the activity begin marker ([1] Build) but not the completion marker ([1] Build: done (3.2s)). The end of a step is therefore visually unmarked at the most common verbosity level; users rely on silence (no errors) to infer completion. The PR description calls this deliberate ("BEGIN renders at Info, DONE at Detail"), so I'm not flagging it as wrong — just making sure the asymmetry is an explicitly accepted trade-off rather than an oversight. If this ever proves confusing in practice, moving the done line to Info (or making it conditional: Info when Elapsed > threshold) would be a minor change.

---

Style-guide issue

DotNetService.cs:256 — declaration exceeds 120 chars

// ~148 chars — should be wrapped
private Task<ProcessResult> RunDotNetAsync(IEnumerable<string> args, bool streamOutput = true, CancellationToken cancellationToken = default)

Per the project's style guide, parameter lists that push the line past 120 characters must be wrapped one-param-per-line, with the closing ) on the last parameter's line (SA1111):

private Task<ProcessResult> RunDotNetAsync(
    IEnumerable<string> args,
    bool streamOutput = true,
    CancellationToken cancellationToken = default)
    => _processRunner.RunAsync(
        DotNetMuxer,
        args,
        onStdout: streamOutput ? _reporter.ChildOutput : null,
        onStderr: streamOutput ? _reporter.ChildError : null,
        cancellationToken: cancellationToken);

Fix this →%60%20must%20stay%20on%20the%20last%20parameter%E2%80%99s%20line%20per%20SA1111.)

---

Nits / observations (no action required)

NO_COLOR spec compliance — The check is { Length: > 0 } silently ignores NO_COLOR= (set but empty). The NO_COLOR spec says to disable color "regardless of value". The practical impact is negligible (the --color/--no-color flags fully override it), but worth knowing the deviation exists.

NullReporter.Verbosity = Quiet — IsEnabled(Error) returns true for Quiet (0 ≤ 0), so error-level CompositeFormat messages on the null reporter still incur string formatting before being silently discarded. This diverges from NullLogger's IsEnabled = false (which skips formatting entirely). Not a concern in practice — tests don't hot-loop through a null reporter — just noting the pattern difference.

---

What looks good

• Thread safety: System.Threading.Lock + locking in every public write path (including ChildOutput/ChildError which arrive on background threads) is correct and idiomatic.
• ProcessRunner fan-out: stdout/stderr → capture + optional delegate is clean; the null-callback branch avoids creating a delegate when streaming isn't requested.
• Activity depth via stack: naturally handles nesting without an explicit counter; the ReferenceEquals guard in EndActivity is a sound defensive measure against misuse.
• Color detection priority: --color/--no-color flag → NO_COLOR env var → redirect detection is exactly the right precedence order.
• Null stubs (NullReporter, NullActivityScope): allocation-free, BCL-only, singleton — matches the architecture's stated convention (cf. NullLogger).
• File naming and partial class split (ConsoleReporter.cs + ConsoleReporter.ActivityScope.cs): follows the style guide's nested-type file naming rule.
• Extension block pragmas: match the project's mandated template verbatim (correct warning IDs and trailing links).
• _reporter.ChildOutput / _reporter.ChildError method-group delegates in DotNetService.RunDotNetAsync: clean, and the streamOutput flag keeping internal evaluations (dotnet msbuild -getProperty:...) silent is the right call.
• Removal of SpectreLogger/SpectreLoggerProvider: clean excision with no orphaned references.

---

One blocking issue (the long declaration in DotNetService.cs) and two non-blocking observations. Everything else LGTM.

[rdeago]

@claude thanks for the review!

Activity "done" at Detail vs "begin" at Info

They're both at Info now.

DotNetService.cs:256 — declaration exceeds 120 chars

Fixed.

NO_COLOR spec compliance — The check is { Length: > 0 } silently ignores NO_COLOR= (set but empty). The NO_COLOR spec says to disable color "regardless of value".

The spec says "when present and not an empty string (regardless of its value)" (emphasis mine). Our check is fully compliant.

NullReporter.Verbosity = Quiet — IsEnabled(Error) returns true for Quiet (0 ≤ 0), so error-level CompositeFormat messages on the null reporter still incur string formatting before being silently discarded.

There's no None verbosity by design choice; Quiet is the verbosity level that best approaches what NullReporter actually does. More than a handful error messages in the same run are an extreme improbability, so further optimization would be pointless.

I have also added other changes, not in the original scope of the PR. Please review again.

[claude]

Claude finished @rdeago's task in 5m 50s —— View job

---

PR Re-Review

• Gather context and read all changed files
• Confirm previous-review fixes
• Review new/changed code
• Post findings

---

Previous-review items — confirmed

• "done" at Info level: confirmed (EndActivity calls IsEnabled(MessageLevel.Info), same as BeginActivity). ✓
• DotNetService.cs:256 long declaration: confirmed wrapped. ✓
• NO_COLOR spec: you're right — the spec says "when present and not an empty string"; my reading was wrong. ✓
• NullReporter.Verbosity = Quiet: accepted; no further comment. ✓

---

New code — correctness issue

ChildOutput / ChildError verbosity filter is inverted — ConsoleReporter.cs lines 74–83 and 95–104

if (verbosity is not null && (int)verbosity <= (int)Verbosity)
{
    return;  // ← discards the line
}

This discards lines when verbosity ≤ currentVerbosity, i.e. when the reporter's verbosity is at or above the specified threshold — and shows them only when the threshold exceeds the current verbosity. That is the opposite of "show at this verbosity level and above."

The interface doc says "The verbosity level at which the line should be written"; the method comment says "if a verbosity is specified, respect it … useful for child processes whose verbosity we control." Both point to a minimum-threshold contract:

• Show when currentVerbosity >= threshold → (int)Verbosity >= (int)verbosity
• Discard when (int)verbosity > (int)Verbosity

The fix:

if (verbosity is not null && (int)verbosity > (int)Verbosity)
{
    return;
}

The same fix applies to ChildError. Since every current call site passes null this is latent, but the interface is brand-new and has no locked-in callers, so now is exactly the right time to fix it.

Fix this →

---

Question — TestSolutionAsync --output direction

DotNetService.cs (inside TestSolutionAsync):

"--output",
_reporter.Verbosity >= Verbosity.Detailed ? "Normal" : "Detailed",

Lower reporter verbosity → "Detailed" MTP output; higher reporter verbosity → "Normal" MTP output. This is counterintuitive on the surface. I can see an argument for it ("at higher verbosity the live dotnet stream already carries context; at lower verbosity the test runner should be more talkative"), but it's surprising enough that I want to confirm it's deliberate rather than a transposed ?:

---

New additions — what looks good

InvocationKind enum: clean three-way split (Normal / Informational / Internal) that makes dotnet verbosity and streaming intent explicit at each call site. The Internal suppression for the getProperty:IsTestingPlatformApplication probe is exactly right.

LazyResolver<T> / AddLazySupport(): minimal and correct. LazyThreadSafetyMode.ExecutionAndPublication is the right default for a DI-backed lazy.

Lazy<NuGetPushConfiguration> injection in DotNetService: removes the service-locator anti-pattern cleanly.

IActivityScope.Complete(string? outcomeMessage): the optional outcome message is a clean addition and the FormatActivityLine formatting (done (3.2s) - message) is readable.

": starting..." in begin message: a clear improvement over just the title; users can now distinguish "activity opened" from "activity closed".

--coverage / --force-english-output removal: PR description covers these; the intent to move project-specific flags to buildvana.jsonc is a valid strategy.

NuGetPushAllAsync: downgrading per-package lines to Detail and emitting a summary at Info is the right tradeoff for normal verbosity.

ContinuousIntegrationBuildArg rename (dotnetTest → asMSBuildPassthrough): semantics inverted in both the parameter name and the conditional, end result identical, new name much clearer.

ProcessRunner stderr fan-out: the onStderr is null short-circuit avoids the 3-target merge in the common case.

Remaining service migrations (GitService, SelfReferenceUpdater, ChangelogService, etc.): mechanical ILogger → IReporter swap, uniformly correct — LogInformation → Info, LogDebug → Detail, LogWarning → Warning.

---

One blocking correctness issue (inverted verbosity filter in ChildOutput/ChildError) and one question about the --output direction in TestSolutionAsync. Everything else LGTM.

[rdeago]

Thanks again @claude!

ChildOutput / ChildError verbosity filter is inverted — ConsoleReporter.cs lines 74–83 and 95–104

Fixed. Also added an extension method on IReporter to verify that its verbosity is at least as specified, so checks become cleaner and less error-prone.

Question — TestSolutionAsync --output direction

One more inverted check. Thanks for the catch.

Please review again.

[claude]

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

---

PR Re-Review (round 3)

• Gather context from trigger comment
• Confirm previous-review fixes (verbosity filter, --output direction)
• Review new IsVerbosityAtLeast extension method
• Review remaining new/changed code
• Post findings

---

Previous-review items — all confirmed

• Verbosity filter in ChildOutput/ChildError: now uses !this.IsVerbosityAtLeast(v) — correctly returns early only when current verbosity falls below the threshold. ✓
• --output direction in TestSolutionAsync: IsVerbosityAtLeast(Verbosity.Detailed) ? "Detailed" : "Normal" — higher verbosity gives more detail. ✓
• IsVerbosityAtLeast: (int)minimumVerbosity <= (int)@this.Verbosity is the right expression. Clean addition. ✓

---

One new item

InvocationKind.Informational is defined but never used — DotNetService.InvocationKind.cs

// ReSharper disable once UnusedMember.Local
Informational,

The suppression comment is the tell: the member exists but nothing calls RunDotNetAsync with InvocationKind.Informational. The style guide is clear — don't add abstractions beyond what the task requires. Remove this member until there is an actual call site for it; adding it back then is a one-liner.

Fix this →

---

What looks good

• IsVerbosityAtLeast placement: adding it to ReporterExtensions (the abstractions layer, BCL-only) rather than in the implementation is the right call.
• ChildOutput/ChildError pattern: minimumVerbosity is { } v && !this.IsVerbosityAtLeast(v) — idiomatic null check + guard.
• NuGetPushAllAsync failure path: ProcessRunner.RunAsync captures stderr and includes it in the BuildFailedException message when the exit code is non-zero, so push errors surface even without streaming. The downgrade of per-package lines to Detail + summary at Info is the right tradeoff.
• RunDotNetAsync verbosity forwarding: $"--verbosity={_reporter.Verbosity}" produces e.g. --verbosity=Normal; the .NET CLI accepts case-insensitive verbosity values.
• ContinuousIntegrationBuildArg rename (dotnetTest → asMSBuildPassthrough + inverted conditional): the new name and polarity are clearer.

---

One blocking item (Informational dead code). Everything else LGTM.

[rdeago]

InvocationKind.Informational is defined but never used — DotNetService.InvocationKind.cs

It's used now.