refactor: address review feedback on the Azure CLI subscription lookup

src/Commands/CommandHelpers.cs

@@ -13,16 +13,23 @@ public static class CommandHelpers
     /// Returns a ValidationResult error if subscription is required but cannot be resolved.
     /// Sets the subscription on the settings if resolved from Azure CLI.
     /// </summary>
-    public static ValidationResult ValidateAndResolveSubscription(Guid? subscription, bool isSubscriptionBased, Action<Guid> setSubscription)
+    /// <param name="resolveSubscriptionId">
+    /// Supplies the default subscription ID. Defaults to querying the Azure CLI; overridden by
+    /// tests so both the success and failure paths can be exercised without an Azure CLI install.
+    /// </param>
+    public static ValidationResult ValidateAndResolveSubscription(Guid? subscription, bool isSubscriptionBased,
+        Action<Guid> setSubscription, Func<string>? resolveSubscriptionId = null)
     {
         if (!isSubscriptionBased || subscription.HasValue)
             return ValidationResult.Success();
 
+        resolveSubscriptionId ??= AzCommand.GetDefaultAzureSubscriptionId;
+
         string reason;
 
         try
         {
-            var subscriptionId = AzCommand.GetDefaultAzureSubscriptionId();
+            var subscriptionId = resolveSubscriptionId();
 
             if (Guid.TryParse(subscriptionId, out var resolved))
             {

src/Infrastructure/AzCommand.cs

@@ -6,13 +6,19 @@ namespace AzureCostCli.Infrastructure;
 
 public static class AzCommand
 {
+    private const string AzExecutable = "az";
     private const string AzArguments = "account show --output json";
 
     /// <summary>
     /// How long to wait for the Azure CLI to respond before giving up.
     /// </summary>
     private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(30);
 
+    /// <summary>
+    /// How long to wait for the Azure CLI to actually terminate after being killed.
+    /// </summary>
+    private static readonly TimeSpan KillTimeout = TimeSpan.FromSeconds(2);
+
     /// <summary>
     /// Builds the start info used to invoke the Azure CLI.
     /// </summary>
@@ -30,12 +36,12 @@ internal static ProcessStartInfo BuildStartInfo(bool isWindows)
         var startInfo = isWindows
             ? new ProcessStartInfo
             {
-                FileName = Environment.GetEnvironmentVariable("ComSpec") ?? "cmd.exe",
-                Arguments = $"/c az {AzArguments}"
+                FileName = ResolveCommandInterpreter(),
+                Arguments = $"/c {AzExecutable} {AzArguments}"
             }
             : new ProcessStartInfo
             {
-                FileName = "az",
+                FileName = AzExecutable,
                 Arguments = AzArguments
             };
 
@@ -47,6 +53,29 @@ internal static ProcessStartInfo BuildStartInfo(bool isWindows)
         return startInfo;
     }
 
+    /// <summary>
+    /// Resolves the Windows command interpreter, preferring an absolute path over a PATH lookup.
+    /// </summary>
+    internal static string ResolveCommandInterpreter()
+    {
+        // ComSpec is normally an unquoted absolute path, but tolerate a quoted value:
+        // ProcessStartInfo.FileName is not parsed as a command line, so quotes would be
+        // taken as part of the file name and the process would fail to start.
+        var comSpec = Environment.GetEnvironmentVariable("ComSpec")?.Trim().Trim('"');
+
+        if (!string.IsNullOrWhiteSpace(comSpec))
+            return comSpec;
+
+        // Fall back to the system directory rather than a bare "cmd.exe", so resolution does
+        // not depend on the PATH. GetFolderPath returns an empty string off Windows, in which
+        // case this degrades to the bare name.
+        var systemDirectory = Environment.GetFolderPath(Environment.SpecialFolder.System);
+
+        return string.IsNullOrEmpty(systemDirectory)
+            ? "cmd.exe"
+            : Path.Combine(systemDirectory, "cmd.exe");
+    }
+
     public static string GetDefaultAzureSubscriptionId()
     {
         using var process = new Process { StartInfo = BuildStartInfo() };
@@ -58,7 +87,7 @@ public static string GetDefaultAzureSubscriptionId()
         catch (Win32Exception ex)
         {
             throw new Exception(
-                $"Unable to start the Azure CLI ('{process.StartInfo.FileName}'). " +
+                $"Unable to start the Azure CLI ('{AzExecutable}'){DescribeLauncher(process.StartInfo.FileName)}. " +
                 "Make sure the Azure CLI is installed and available on the PATH. " +
                 $"({ex.Message})", ex);
         }
@@ -71,31 +100,34 @@ public static string GetDefaultAzureSubscriptionId()
 
         if (!process.WaitForExit((int)Timeout.TotalMilliseconds))
         {
-            try
-            {
-                process.Kill(entireProcessTree: true);
-            }
-            catch
-            {
-                // Nothing useful to do if the process cannot be killed.
-            }
+            KillAndObserve(process, outputTask, errorTask);
 
             throw new Exception(
-                $"Timed out after {Timeout.TotalSeconds:N0} seconds waiting for 'az {AzArguments}'.");
+                $"Timed out after {Timeout.TotalSeconds:N0} seconds waiting for " +
+                $"'{AzExecutable} {AzArguments}'.");
         }
 
-        var output = outputTask.GetAwaiter().GetResult();
-        var error = errorTask.GetAwaiter().GetResult();
+        return ParseSubscriptionId(
+            process.ExitCode,
+            outputTask.GetAwaiter().GetResult(),
+            errorTask.GetAwaiter().GetResult());
+    }
 
-        if (process.ExitCode != 0)
+    /// <summary>
+    /// Interprets the outcome of the Azure CLI invocation and extracts the subscription ID.
+    /// </summary>
+    internal static string ParseSubscriptionId(int exitCode, string output, string error)
+    {
+        if (exitCode != 0)
         {
             throw new Exception(
-                $"Error executing 'az {AzArguments}' (exit code {process.ExitCode}): {error.Trim()}");
+                $"Error executing '{AzExecutable} {AzArguments}' (exit code {exitCode}): {error.Trim()}");
         }
 
         using var jsonDocument = ParseJson(output);
 
-        if (jsonDocument.RootElement.TryGetProperty("id", out var idElement) &&
+        if (jsonDocument.RootElement.ValueKind == JsonValueKind.Object &&
+            jsonDocument.RootElement.TryGetProperty("id", out var idElement) &&
             idElement.GetString() is { Length: > 0 } subscriptionId)
         {
             return subscriptionId;
@@ -104,6 +136,44 @@ public static string GetDefaultAzureSubscriptionId()
         throw new Exception("Unable to find the 'id' property in the JSON output.");
     }
 
+    /// <summary>
+    /// Describes the launcher when the Azure CLI is not invoked directly, so that diagnostics
+    /// do not read as if the command interpreter itself were the Azure CLI.
+    /// </summary>
+    internal static string DescribeLauncher(string fileName) =>
+        string.Equals(fileName, AzExecutable, StringComparison.OrdinalIgnoreCase)
+            ? string.Empty
+            : $" via the command interpreter ('{fileName}')";
+
+    /// <summary>
+    /// Terminates a process that overran its timeout and observes the in-flight pipe reads,
+    /// so neither the process nor the read tasks outlive this call unnoticed.
+    /// </summary>
+    private static void KillAndObserve(Process process, params Task[] readTasks)
+    {
+        try
+        {
+            process.Kill(entireProcessTree: true);
+            process.WaitForExit((int)KillTimeout.TotalMilliseconds);
+        }
+        catch
+        {
+            // Nothing useful to do if the process cannot be killed; the original timeout
+            // is the error worth surfacing.
+        }
+
+        try
+        {
+            // Give the reads a moment to complete against the now-closed pipes and observe
+            // any faults, so they are not left as unobserved task exceptions.
+            Task.WaitAll(readTasks, KillTimeout);
+        }
+        catch
+        {
+            // Faulted reads are expected once the process has been killed.
+        }
+    }
+
     private static JsonDocument ParseJson(string output)
     {
         try
@@ -115,7 +185,7 @@ private static JsonDocument ParseJson(string output)
             // Most likely an 'output' default configured through 'az configure' or AZURE_CORE_OUTPUT,
             // which the explicit --output json should already override, but be explicit about it.
             throw new Exception(
-                $"The output of 'az {AzArguments}' is not valid JSON: {ex.Message}", ex);
+                $"The output of '{AzExecutable} {AzArguments}' is not valid JSON: {ex.Message}", ex);
         }
     }
 }

tests/AzureCostCli.Tests/Commands/CommandHelpersTests.cs

@@ -56,20 +56,56 @@ public void ValidateAndResolveSubscription_NoSubscriptionAndSubscriptionBased_At
     }
 
     [Fact]
-    public void ValidateAndResolveSubscription_WhenAzCliResolutionFails_ErrorExplainsWhy()
+    public void ValidateAndResolveSubscription_WhenResolverSucceeds_SetsResolvedSubscription()
     {
-        // Act - environment dependent: az CLI may or may not be present.
+        // Arrange
+        var expected = Guid.NewGuid();
+        Guid captured = Guid.Empty;
+
+        // Act
         var result = CommandHelpers.ValidateAndResolveSubscription(
-            subscription: null, isSubscriptionBased: true, _ => { });
+            subscription: null, isSubscriptionBased: true, id => captured = id,
+            resolveSubscriptionId: () => expected.ToString());
 
-        // Assert - a failure must say more than "unable to retrieve"; the underlying
-        // reason is appended in parentheses so the user can actually diagnose it.
-        if (!result.Successful)
-        {
-            result.Message.ShouldNotBeNull();
-            result.Message.ShouldContain("az login");
-            result.Message.ShouldMatch(@"\(.+\)");
-        }
+        // Assert
+        result.Successful.ShouldBeTrue();
+        captured.ShouldBe(expected);
+    }
+
+    [Fact]
+    public void ValidateAndResolveSubscription_WhenResolverThrows_ErrorExplainsWhy()
+    {
+        // Arrange
+        Guid captured = Guid.Empty;
+
+        // Act
+        var result = CommandHelpers.ValidateAndResolveSubscription(
+            subscription: null, isSubscriptionBased: true, id => captured = id,
+            resolveSubscriptionId: () => throw new Exception("az is sulking"));
+
+        // Assert - the underlying reason must survive, otherwise the failure is undiagnosable.
+        result.Successful.ShouldBeFalse();
+        result.Message.ShouldNotBeNull();
+        result.Message.ShouldContain("az login");
+        result.Message.ShouldContain("az is sulking");
+        captured.ShouldBe(Guid.Empty);
+    }
+
+    [Fact]
+    public void ValidateAndResolveSubscription_WhenResolverReturnsNonGuid_ErrorReportsTheValue()
+    {
+        // Arrange
+        Guid captured = Guid.Empty;
+
+        // Act - a non-GUID is a different failure from "az is unusable" and must say so.
+        var result = CommandHelpers.ValidateAndResolveSubscription(
+            subscription: null, isSubscriptionBased: true, id => captured = id,
+            resolveSubscriptionId: () => "not-a-guid");
+
+        // Assert
+        result.Successful.ShouldBeFalse();
+        result.Message.ShouldContain("not-a-guid");
+        captured.ShouldBe(Guid.Empty);
     }
 
     [Fact]

tests/AzureCostCli.Tests/Infrastructure/AzCommandTests.cs

@@ -56,4 +56,80 @@ public void BuildStartInfo_DefaultsToTheCurrentOperatingSystem()
         actual.FileName.ShouldBe(expected.FileName);
         actual.Arguments.ShouldBe(expected.Arguments);
     }
+
+    [Fact]
+    public void ResolveCommandInterpreter_NeverReturnsAQuotedPath()
+    {
+        // ProcessStartInfo.FileName is not parsed as a command line, so a quoted ComSpec
+        // would be taken as part of the file name and fail to start.
+        var interpreter = AzCommand.ResolveCommandInterpreter();
+
+        interpreter.ShouldNotBeNullOrWhiteSpace();
+        interpreter.ShouldNotStartWith("\"");
+        interpreter.ShouldNotEndWith("\"");
+        interpreter.ShouldContain("cmd", Case.Insensitive);
+    }
+
+    [Fact]
+    public void DescribeLauncher_WhenAzIsInvokedDirectly_AddsNothing()
+    {
+        AzCommand.DescribeLauncher("az").ShouldBeEmpty();
+    }
+
+    [Fact]
+    public void DescribeLauncher_WhenLaunchedViaInterpreter_NamesItAsTheLauncherNotTheCli()
+    {
+        // The startup failure message must not read as though cmd.exe were the Azure CLI.
+        var description = AzCommand.DescribeLauncher(@"C:\WINDOWS\system32\cmd.exe");
+
+        description.ShouldContain("command interpreter");
+        description.ShouldContain("cmd.exe");
+        description.ShouldNotContain("Azure CLI");
+    }
+
+    [Fact]
+    public void ParseSubscriptionId_WithValidJson_ReturnsId()
+    {
+        var id = AzCommand.ParseSubscriptionId(0, """{"id":"abc-123","name":"Sub"}""", "");
+
+        id.ShouldBe("abc-123");
+    }
+
+    [Fact]
+    public void ParseSubscriptionId_WithNonZeroExitCode_ReportsExitCodeAndStderr()
+    {
+        var ex = Should.Throw<Exception>(() =>
+            AzCommand.ParseSubscriptionId(9009, "", "'az' is not recognized"));
+
+        ex.Message.ShouldContain("9009");
+        ex.Message.ShouldContain("'az' is not recognized");
+    }
+
+    [Fact]
+    public void ParseSubscriptionId_WithNonJsonOutput_ExplainsTheOutputWasNotJson()
+    {
+        // What a user with 'az configure --defaults output=table' would have seen.
+        var ex = Should.Throw<Exception>(() =>
+            AzCommand.ParseSubscriptionId(0, "Name    CloudName    SubscriptionId", ""));
+
+        ex.Message.ShouldContain("not valid JSON");
+    }
+
+    [Fact]
+    public void ParseSubscriptionId_WithoutIdProperty_Throws()
+    {
+        var ex = Should.Throw<Exception>(() =>
+            AzCommand.ParseSubscriptionId(0, """{"name":"Sub"}""", ""));
+
+        ex.Message.ShouldContain("'id'");
+    }
+
+    [Theory]
+    [InlineData("""{"id":""}""")]
+    [InlineData("""{"id":null}""")]
+    [InlineData("[]")]
+    public void ParseSubscriptionId_WithUnusableId_Throws(string output)
+    {
+        Should.Throw<Exception>(() => AzCommand.ParseSubscriptionId(0, output, ""));
+    }
 }