Tool refactoring Phase 2: replace CakeHost with Spectre.Console.Cli

[rdeago]

Closes #236.

Summary

Phase 2 of the Tool refactoring: swap the Frosting host for a Spectre.Console.Cli.CommandApp over a plain ServiceProvider. After this lands, no production code under src/Buildvana.Tool/**/*.cs references a Cake.* type — only the Cake.Frosting package reference in csproj
remains, and Phase 3 will remove it.

Three substantive commits:

1. 39dd2b3 — Add MSBuildProperties singleton plumbed through DotNetService.StandardMSBuildArgs().
   No population path yet; this commit only widens the seam so the Spectre.Console.Cli cutover can push parsed /p:Key=Value pairs into it.
2. 402ec90 — The host swap itself.
   • New Program.Main builds a ServiceCollection (IAnsiConsole, SpectreLoggerProvider, MSBuildProperties, all Phase-1 services), wraps it in a TypeRegistrar / TypeResolver adapter pair, and dispatches via CommandApp. BuildFailedException is caught at the top and its ExitCode surfaced to the process.
   • Each FrostingTask becomes an AsyncCommand. The pre-pipeline (Clean → Restore → Build → Test → …) is expressed as explicit awaits in the command body; the step bodies live in a static BuildSteps class so they can be invoked individually (e.g., for a future bv just <step>).
   • BaseSettings / BuildSettings / ReleaseSettings carry global, build, and release options; SettingsApplier maps them to OptionsService / SpectreLoggerProvider.MinLevel / IAnsiConsole.Profile.Capabilities.Ansi. Both camelCase and kebab-case aliases are accepted.
   • /p:Key=Value (and -p:Key=Value) are pre-extracted from args in Program.Main and pushed into the MSBuildProperties singleton.
   • ICommandOptions / EmptyCommandOptions are removed; OptionsService now reads SetOption-pushed values and env-vars only.
   • Frosting host scaffolding is deleted: BuildContext, BuildStartup, BuildLifetime, CommandLineParser, CakeLogLoggerProvider*, EnvVar, Tasks/, Options/.
3. 5cf9bcf — Inspectcode now reports zero results at WARNING severity for the whole solution. Pre-existing findings cleared as drive-by:
   • Delete GitLabServerRelease.cs (unreachable — adapter throws before construction).
   • Delete GitProtocol enum and drop the unused GitUrlInfo.Protocol property; URL-form parsing is preserved.
   • Drop GitService.Origin (set in ctor, never read).
   • Tag ProcessResult and GitService with [PublicAPI].
   • Tag GetWinePath / GetWinePaths with [UsedImplicitly(WithMembers)] so MSBuild [Output] properties aren't flagged.
   • Drop redundant using Buildvana.Core from the two HomeDirectory providers.
   • Suppress two PossibleMultipleEnumeration false positives in ProcessRunner.RunAsync with // ReSharper disable once comments. Root cause: CommunityToolkit.Diagnostics.Guard.IsNotNull is not annotated [NoEnumeration]. An external-annotations XML file would be the right fix; inspectcode didn't pick it up from the documented locations during this branch.

Acceptance criteria — all met

• dotnet bv clean, restore, build, test, pack, release all dispatch through Spectre.Console.Cli.
• dotnet bv --help and per-command --help render Spectre's help with the same information content as before.
• dotnet bv build /p:ContinuousIntegrationBuild=true forwards the MSBuild property.
• --no-color emits plain text; default in a TTY emits color.
• BuildFailedException from a service surfaces as a non-zero process exit code.
• grep -r 'Cake\.' src/Buildvana.Tool/**/*.cs returns no production matches (only the Cake.Frosting package reference in csproj remains; Phase 3).
• dotnet bv build self-hosts.

Test plan

• dotnet bv pack — 0 errors, 0 warnings.
• inspectcode --swea --severity=WARNING — 0 results.
• Both packages produced (Buildvana.Sdk, bv).
• Self-hosting smoke check after merge & publish (next build cycle).

[rdeago]

@claude please review.

[claude]

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

---

PR Review: Tool refactoring Phase 2 — replace CakeHost with Spectre.Console.Cli

Tasks:

• Read CLAUDE.md and project rules
• Examine changed files
• Review core infrastructure changes
• Review new CLI commands and settings
• Review BuildSteps and command pipeline
• Review service changes (MSBuildProperties, DotNetService, etc.)
• Check for correctness issues, style violations, and potential bugs
• Post final review

---

Overall this is a clean, well-structured refactor. The commit breakdown is logical, the code is idiomatic, and the acceptance criteria are largely met. I have two medium-priority findings and three lower-priority observations.

---

Medium priority

1. Stale pragma comment in SpectreLoggerProvider.cs

SpectreLoggerProvider.cs:21–23:

#pragma warning disable CA1812 // Avoid uninstantiated internal classes - Will be instantiated via DI in a soon-to-come commit.
internal sealed class SpectreLoggerProvider : ILoggerProvider
#pragma warning restore CA1812

The comment says "Will be instantiated via DI in a soon-to-come commit" — but this PR is that commit. The pragma is also redundant since CA1812 is already suppressed project-wide in the .csproj (with a correct justification for Spectre TypeRegistrar/TypeResolver classes). The whole pragma block can be removed. Fix this →

2. Acceptance criterion mentions dotnet bv prepare, but no such command exists

The PR body lists dotnet bv prepare as an acceptance-checked item. However, Program.cs:57–66 registers only clean / restore / build / test / pack / release. There is no PrepareCommand anywhere in the source tree — nor was there a corresponding PrepareTask in the old Frosting code. The CLAUDE.md docs also don't mention a prepare command.

This looks like a copy-paste error in the PR description (perhaps prepare was an early internal name for pack). Worth correcting in the PR body to avoid confusion, but no code change needed if there was never a real prepare command.

---

Lower priority / observations

3. MSBuildProperties key comparison is case-sensitive; MSBuild properties are not

MSBuildProperties.cs:16:

private readonly Dictionary<string, string> _properties = new(StringComparer.Ordinal);

MSBuild treats property names as case-insensitive. With Ordinal comparison, /p:Foo=1 /p:FOO=2 are stored under two separate keys and both forwarded to MSBuild (which deduplicates them itself, using the last value). No breakage, but it diverges from MSBuild semantics and can produce redundant args. StringComparer.OrdinalIgnoreCase would mirror MSBuild more faithfully and deduplicate on our side. Fix this →

4. User-specified /p: properties are silently not forwarded to dotnet test

DotNetService.cs:151:

List<string> args = ["test", solution.SolutionPath, "-c", Configuration, ContinuousIntegrationBuildArg()];

TestSolutionAsync deliberately omits StandardMSBuildArgs() (which includes _msbuildProperties.EnumerateAsArgs()) because MTP test runners fail on unrecognized MSBuild args. The comment explains this, but it means that user-supplied /p:Key=Value arguments are silently swallowed for the test invocation. The PR description says dotnet bv build /p:ContinuousIntegrationBuild=true forwards the property — which is true for build/restore/pack, but not for test. Low risk in practice, but the behavior is surprising and worth a note in the comment.

5. Command descriptions don't surface the "includes previous steps" convention

BuildCommand.cs:13: [Description("Build all projects.")] — but bv build actually runs clean → restore → build. Similarly, bv test runs clean → restore → build → test.

The CLAUDE.md documents this convention, but --help output only shows the [Description] string, so CLI users won't see it. Consider expanding the descriptions (e.g., "Clean, restore, and build all projects.") for better discoverability. Minor UX point, not a bug.

---

Positive notes

• The SettingsApplier.ApplyColor cancellation logic (color == noColor → no-op) correctly handles both "neither flag" and "both flags" cases without extra branching.
• ExtractMSBuildProperties correctly handles the eq == 0 case (property name is empty, e.g. /p:=value): it falls through to cleanArgs, letting Spectre report the error.
• DotNetService singleton ordering is safe: SettingsApplier.Apply is called before any BuildSteps.* in every command body, so Configuration is set before DotNetService is first resolved.
• The TypeRegistrar/TypeResolver adapter pair is a textbook correct Spectre DI integration.
• Clearing the ReSharper findings in the drive-by commit is appreciated — zero-result inspectcode output at merge is a good policy.

[rdeago]

1. Stale pragma comment in SpectreLoggerProvider.cs

Removed.

2. Acceptance criterion mentions dotnet bv prepare, but no such command exists

Text corrected.

3. MSBuildProperties key comparison is case-sensitive; MSBuild properties are not

Fixed.

4. User-specified /p: properties are silently not forwarded to dotnet test

Fixed.

5. Command descriptions don't surface the "includes previous steps" convention

Fixed.