refactor: address review feedback on the Azure CLI subscription lookup

[mivano]

Follow-up to #349, addressing all four review comments. No behavioural change on the happy path — verified the subscription still auto-resolves and returns real cost data.

1. Injectable resolver — the substantive one

ValidateAndResolveSubscription now takes an optional Func<string> for the subscription lookup, defaulting to AzCommand.GetDefaultAzureSubscriptionId. All 11 call sites are unchanged.

The reviewer was right that the old test was vacuous on any machine with a logged-in Azure CLI — including mine, so CI green never actually proved the reason-appending worked; only a manual repro did. They were also right that forcing failure by mutating PATH/ComSpec in-process would be flaky, since AssemblyInfo.cs:3 sets DisableTestParallelization = false. That correctly rules out the cheap fix, so the seam is the answer.

Three deterministic tests replace it: resolver succeeds, resolver throws (reason must survive into the message), resolver returns a non-GUID (must be reported as a distinct failure).

2. Startup failure message named the wrong binary

The message reported process.StartInfo.FileName, which on Windows is the command interpreter — so it read "Unable to start the Azure CLI ('cmd.exe')" and pointed a maintainer at entirely the wrong executable. It now names az and describes the interpreter separately:

Unable to start the Azure CLI ('az') via the command interpreter ('C:\WINDOWS\system32\cmd.exe'). ...

Non-Windows is unchanged (no launcher suffix, since az is invoked directly). DescribeLauncher is internal so this is directly tested — otherwise the message would only be reachable when cmd.exe itself fails to start, i.e. never in practice.

3. Command interpreter resolution

ComSpec is trimmed of surrounding quotes: ProcessStartInfo.FileName is not parsed as a command line, so a quoted value would be taken as part of the file name and fail to start. The fallback now resolves cmd.exe from the system directory rather than relying on a PATH lookup.

Worth noting this is hardening rather than a fix for an observed failure — COMSPEC is system-set to an unquoted absolute path, so a quoted value means something already went out of its way to break it. It's two lines and strictly more predictable, so it's not worth arguing about.

4. Timeout path

After Kill, the process now gets a 2-second grace period to actually terminate, and the two in-flight pipe reads are observed rather than left dangling.

Half of this comment was overstated — unobserved Task exceptions have not torn down the process since .NET 4.5, so that part was benign. The legitimate half is throwing while az may still be shutting down, which is now handled. This path is also the least likely to ever execute.

Extra: testable outcome parsing

ParseSubscriptionId(exitCode, stdout, stderr) is extracted so the exit-code, non-JSON and missing-id branches are unit-testable without spawning a process — these previously had no coverage at all. This also covers the --output json regression directly: a table-formatted response now has an explicit test asserting the "not valid JSON" diagnostic.

Testing

304 tests pass (292 → 304: one environment-dependent test removed, 13 deterministic ones added). Manually re-verified on macOS that the happy path and the az-absent path both still behave correctly.

As with #349, the Windows runtime path itself remains unexecuted — no Windows machine, CI is ubuntu-only. Everything about it is unit-tested at the argument/message level, but confirmation from a Windows user on #347 is still the thing that would actually close the loop.

🤖 Generated with Claude Code

[github-actions]

File	Coverage	Lines	Branches		Missing
All files	30%	29%	31%	❌

Minimum allowed coverage is 75%

Generated by 🐒 cobertura-action against ad80d6d

[Copilot]

Pull request overview

Refactors Azure CLI default subscription lookup to be more testable and diagnosable, while preserving the existing “happy path” behavior for subscription auto-resolution across commands.

Changes:

• Adds an injectable subscription-ID resolver seam to CommandHelpers.ValidateAndResolveSubscription to enable deterministic success/failure tests without relying on a local Azure CLI install.
• Improves Windows process-launch robustness and diagnostics in AzCommand (command interpreter resolution, clearer “could not start” messaging, timeout cleanup, and extracted outcome parsing).
• Expands unit test coverage for interpreter resolution, launcher description, and subscription-id parsing edge cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
tests/AzureCostCli.Tests/Infrastructure/AzCommandTests.cs	Adds focused unit tests for interpreter resolution, launcher description, and subscription-id parsing branches.
tests/AzureCostCli.Tests/Commands/CommandHelpersTests.cs	Replaces the previously environment-dependent failure-path check with deterministic resolver seam tests.
src/Infrastructure/AzCommand.cs	Refactors Azure CLI invocation for clearer diagnostics, safer Windows launcher resolution, timeout cleanup, and testable parsing.
src/Commands/CommandHelpers.cs	Introduces optional resolveSubscriptionId parameter to make subscription resolution deterministic in tests without changing call sites.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mivano]

Good catch — fixed in ad80d6d.

Task.WaitAll(tasks, timeout) does return false on timeout rather than throwing, so a read that faulted after the grace period elapsed was left unobserved. That's precisely what the method claimed to prevent, so the code and its own comment disagreed. Faulted continuations are now attached up front, before the kill and the wait, so observation no longer depends on the wait completing:

internal static void ObserveFaults(params Task[] tasks)
{
    foreach (var task in tasks)
    {
        _ = task.ContinueWith(
            static faulted => _ = faulted.Exception,
            CancellationToken.None,
            TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
            TaskScheduler.Default);
    }
}

Extracted as internal so it's covered by tests: one asserts a task faulting after the call returns is still observed, another that already-completed, faulted and cancelled tasks are handled safely.

One scoping note for the record: the practical impact here is low, since unobserved Task exceptions have not torn down the process since .NET 4.5 (ThrowUnobservedTaskExceptions defaults to false). The reason to fix it is correctness of intent rather than crash risk — the method is named KillAndObserve and documented as observing faults, so it should actually do that on every path.

I have not asserted the GC-finalization property itself (i.e. that TaskScheduler.UnobservedTaskException never fires). Doing so needs a global event handler plus a forced GC.Collect/WaitForPendingFinalizers, which would be flaky with test parallelization enabled — the same constraint that ruled out mutating PATH/ComSpec in the earlier round. The continuation is attached unconditionally, which is the property that matters.

306 tests pass.