Replace Spectre.Console.Cli with AOT-trimmable alternative

[FrankRay78]

Summary

Replaces Spectre.Console.Cli with System.CommandLine to enable AOT compilation and trimming of the NetPace CLI.

Spectre.Console.Cli uses reflection-based command binding that isn't compatible with .NET's AOT/trim requirements. This PR swaps the CLI framework to System.CommandLine (which is AOT-friendly) while preserving Spectre.Console itself for rich terminal rendering (tables, markup, progress bars). Test infrastructure migrates from CommandAppTester to a new CommandLineTestHost that drives the real RootCommand through IServiceCollection, keeping tests close to production wiring.

The branch also re-integrates main's IClientInfoProvider / Hostname-IP feature (#167) into the new DI setup, and fills in two Error_Values regression tests for the Default and Minimal output formats that had orphan .verified.txt files on main but no matching test methods.

Spec

N/A — internal framework migration, no user-facing behavior change.

Changed Files

• CLI framework migration
  • src/NetPace.Console/Program.cs — rewritten to use System.CommandLine with Microsoft.Extensions.DependencyInjection; replaces TypeRegistrar/TypeResolver which are deleted
  • src/NetPace.Console/Commands/SpeedTestCommand.cs, ListServersCommand.cs, their Settings classes — refactored to no longer depend on Spectre.Console.Cli.AsyncCommand or attribute-based binding
  • src/NetPace.Console/CustomHelpProvider.cs — new help renderer, styling aligned with Spectre.Console
• Test harness migration
  • src/NetPace.Console.Tests/CommandLineTestHost.cs — new test host that runs real Program.RunAsync; registers ClientInfoProviderStub by default via TryAddSingleton
  • src/NetPace.Console.Tests/NetPaceConsoleTests.*.cs — migrated from TypeRegistrar/CommandAppTester to ServiceCollection/CommandLineTestHost
  • src/NetPace.Console.Tests/NetPaceConsoleTests.Default.cs, NetPaceConsoleTests.Minimal.cs — ported from main plus added the two missing Should_Not_Include_IPAddress_Or_Hostname_..._With_Stub_Providing_Error_Values tests that had orphan expectation files on main
• IClientInfoProvider integration — SpeedTestCommand constructor now takes IClientInfoProvider; Program.cs DI registers ClientInfoProvider (real) and ClientInfoProviderStub (test)

New Artifacts

None.

Test Plan

• dotnet build src/NetPace.sln — 0 warnings, 0 errors
• dotnet test src/NetPace.sln — 537 passed, 0 failed, 0 skipped (was 535 before this PR — added 2 new Error_Values tests)
• Zero disabled tests (no #if FALSE blocks remain)

[claude]

Code Review — PR #171: Replace Spectre.Console.Cli with System.CommandLine

Overall the migration is well-structured and the architectural decisions are sound. The plan to preserve Spectre.Console for rendering while swapping only the CLI parsing layer is the right call, and the new CommandLineTestHost keeps tests close to real production wiring. A few issues need addressing before this is merge-ready.

---

Bugs / Correctness Issues

[BLOCKER] ServiceProvider is never disposed (Program.cs:346)

var serviceProvider = services.BuildServiceProvider();
// ... no using/await using
return await RunAsync(serviceProvider, ...);

ServiceProvider implements both IDisposable and IAsyncDisposable. Any singleton that is itself IDisposable (e.g. HttpClient, database connections) will not be cleaned up. For a CLI process this usually goes unnoticed, but it suppresses IAsyncDisposable teardown of async singletons and is flagged by the DI analyzers. Fix:

await using var serviceProvider = services.BuildServiceProvider();
return await RunAsync(serviceProvider, ...);

The same leak exists in CommandLineTestHost.RunAsync (line 39).

---

[BLOCKER] CommandLineTestHost.RunAsync corrupts shared IServiceCollection on second call (CommandLineTestHost.cs:34)

serviceCollection.AddSingleton<IAnsiConsole>(testConsole);  // not TryAdd

IClientInfoProvider uses TryAddSingleton, which is idempotent, but IAnsiConsole uses AddSingleton. Calling RunAsync twice on the same CommandLineTestHost instance registers IAnsiConsole twice — the second registration silently wins. This is a latent bug: any test that calls RunAsync more than once on the same host will silently use the first run's console for DI resolution while capturing output to a new TestConsole. Change line 34 to:

serviceCollection.TryAddSingleton<IAnsiConsole>(testConsole);

Or better, build a fresh ServiceCollection scoped to each RunAsync call rather than mutating the shared one.

---

[HIGH] Help detection doesn't handle flags before --help (Program.cs:380–396)

if (args.Length > 0 && (args[0] is "-h" or "--help" or "-?"))   // root help
if (args.Length > 1 && (args[1] is "-h" or "--help" or "-?"))   // subcommand help

This only inspects args[0] and args[1]. As a result:

• netpace --csv --help — args[0] is --csv, args[1] is --help; the subcommand check fires, looks for a subcommand named --csv, finds nothing, falls through to parsing. --help is silently ignored.
• netpace servers --json --help — same failure; args[1] is --json, not --help.

Users expect --help to work anywhere in the argument list. A simple fix scans for the flag:

var hasHelp = args.Any(a => a is "-h" or "--help" or "-?");
var firstNonFlag = args.FirstOrDefault(a => !a.StartsWith('-'));
// then branch on subcommand vs root

---

[MEDIUM] CSVDelimiter is typed as string but documented as a single character (SpeedTestCommandSettings.cs:33)

/// Single character delimiter to use in CSV output.
public required string CSVDelimiter { get; init; }

--csv-delimiter abc is now accepted without error. The old char type enforced this constraint for free. The Validate extension method (or the Option<string> registration in Program.cs) should add a Length == 1 check, or the type should revert to char.

---

[LOW] Dead null-coalescing after ToArray() (Program.cs:359)

args!.Where(s => !s.Equals("--test"))!.ToArray() ?? Array.Empty<string>()

Enumerable.ToArray() never returns null; the ?? branch is dead code. The ! null-forgiving operators on args and the LINQ result also hint at fragile nullable handling at the entry point. This is a minor clarity issue but flags to the reader that someone was unsure whether args could be null here.

---

Security

Nothing concerning. No command injection risk, no hard-coded credentials, no insecure defaults introduced.

---

Convention Adherence

Missing XML docs on public ExecuteAsync overloads

ListServersCommand.ExecuteAsync and SpeedTestCommand.ExecuteAsync are public methods without <summary> docs. CLAUDE.md requires XML docs on all public APIs.

Bare catch block (CustomHelpProvider.cs:215)

catch
{
    return "";
}

A bare catch swallows OutOfMemoryException, StackOverflowException, and any future ThreadAbortException analogues. At minimum use catch (Exception). Since this is a formatting helper, a catch (InvalidOperationException) or catch (FormatException) would be more surgical and make failures visible during development.

Nullable bool? settings with non-nullable values (ListServersCommandSettings.cs)

ShowLatency and Fastest are bool? but are always set from Option<bool> which never produces null. This forces triple null-checks in ListServersCommand.cs. They should be bool.

---

Test Coverage

Coverage looks solid. All 83+ existing tests were migrated and two new Error_Values regression tests fill in the gaps from orphaned .verified.txt files. No new public behaviors appear untested.

One gap worth flagging: there is no test that verifies netpace --csv --help shows help (the broken case described above). Adding such a test would have caught the arg-scanning bug before merge.

---

Spec / Test-Plan Check

PR description states "0 warnings, 0 errors" and "537 passed, 0 failed". The two new tests (Should_Not_Include_IPAddress_Or_Hostname_..._With_Stub_Providing_Error_Values) are well-targeted and the .verified.txt files they exercise were already committed to main — this fills a real gap.

No user-facing behaviour change is claimed; that appears accurate from the diff. --help formatting has minor cosmetic differences (different wording in validation errors) which are reflected in updated snapshot files.

---

Summary

	Finding
Blocker	ServiceProvider not disposed in Main and CommandLineTestHost
Blocker	AddSingleton<IAnsiConsole> in test host mutates shared collection on re-use
High	--help anywhere except positions 0/1 is silently ignored
Medium	CSVDelimiter string type lacks length validation
Low	Dead ?? Array.Empty<string>() after ToArray()
Convention	Missing XML docs on ExecuteAsync methods; bare catch; unnecessary bool?
Coverage	No test for --flag --help invocation pattern

The migration strategy is sound and the test harness design is clean. Fix the two blockers and the help-scanning issue before merging — the rest can follow in a clean-up PR.

[FrankRay78]

@claude - Please see the following and action accordingly:

	Finding
Blocker	ServiceProvider not disposed in Main and CommandLineTestHost	Agree - please implement and push changes to this PR
Blocker	AddSingleton<IAnsiConsole> in test host mutates shared collection on re-use	Agree - please implement and push changes to this PR
High	--help anywhere except positions 0/1 is silently ignored	This is intended - we should note this in CLAUDE.md or the most appropriate convention
Medium	CSVDelimiter string type lacks length validation	Agree - please make the delimiter a native char type and push changes to this PR
Low	Dead ?? Array.Empty<string>() after ToArray()	Agree - please implement and push changes to this PR
Convention	Missing XML docs on ExecuteAsync methods; bare catch; unnecessary bool?	Agree - please implement and push changes to this PR
Coverage	No test for --flag --help invocation pattern	Not necessary - see comment above

[claude]

Claude finished @FrankRay78's task in 17m 43s —— View job

---

Implemented PR Review Fixes

All changes applied, built (0 warnings), and pushed. Here's what was done:

• ServiceProvider disposal — await using var serviceProvider in both Program.Main and CommandLineTestHost.RunAsync (upgrading the concurrent Copilot Autofix which used using; await using is correct since ServiceProvider implements IAsyncDisposable)
• TryAddSingleton<IAnsiConsole> — CommandLineTestHost.RunAsync now uses TryAddSingleton to avoid duplicate registrations on re-use
• --help position behaviour — documented in CLAUDE.md under a new CLI Help Behaviour section: --help is only recognised at position 0 (root help) or position 1 after a subcommand name; --flag --help is intentionally not supported
• CSVDelimiter as native char — SpeedTestCommandSettings.CSVDelimiter is now char; Program.cs keeps Option<string> for CLI parsing (System.CommandLine 2.0.1 has no built-in char parser) and adds a Validators.Add rule that rejects multi-character values; the string is converted to char[0] when building the settings
• Dead ?? Array.Empty<string>() — removed from Program.Main; ToArray() never returns null
• XML docs — <summary> added to SpeedTestCommand.ExecuteAsync and ListServersCommand.ExecuteAsync
• Bare catch blocks — replaced with catch (Exception) in CustomHelpProvider.GetDefaultValue and ListServersCommand.DisplayServersWithLatency
• bool? → bool — ListServersCommandSettings.ShowLatency and Fastest changed to bool; all null-checks removed from ListServersCommand.ExecuteAsync

Build: 0 errors, 0 warnings
Tests: 537 passed, 0 failed

Note on push: A Copilot Autofix commit (49c0ab5) was pushed to the branch concurrently (it added using var serviceProvider). Since git fetch/git pull weren't available in this runner context, I used the GitHub Git API to create a new commit (ea549451) directly on top of the Copilot commit, superseding its partial fix with await using and the full set of changes.
| Branch