refactor: extract pure resolver methods for subscription resolution by jongio · Pull Request #2395 · microsoft/mcp

jongio · 2026-04-13T22:10:18Z

Summary

Alternative approach to #2391 for fixing subscription tests when Azure CLI is logged in. Instead of changing SetAzureSubscriptionId to conditionally return CLI profile (which breaks the Set/Get contract), this extracts the resolution logic into pure internal methods with explicit inputs.

Changes

Production code

CommandHelper.cs: Added ResolveDefaultSubscription(profileSubscription, envSubscription) and ResolveSubscription(optionValue, defaultSubscription) as pure internal methods. Refactored GetDefaultSubscription/GetSubscription to delegate to them.
EnvironmentVariableHelpers.cs: Kept ClearAzureSubscriptionId() (useful addition from Fix subscription ID tests when logged in with Azure CLI #2391 that is not in main yet).

Test code

CommandHelperResolverTests.cs (NEW): 13 deterministic tests covering the priority chain with explicit inputs - no env/CLI dependency.
CommandHelperTests.cs: Integration tests use GetDefaultSubscription() for expected values so they pass regardless of CLI login state. Added IDisposable for cleanup.
SubscriptionCommandTests.cs: Same pattern - uses GetDefaultSubscription() for expected values. Added IDisposable.
RegistryListCommandTests.cs: Robust validation test that accounts for CLI profile existence. Uses ClearAzureSubscriptionId() and try/finally for cleanup.
ContainerAppListCommandTests.cs: Same pattern as Registry - uses ClearAzureSubscriptionId() and try/finally for cleanup.

Why this approach

Pure resolvers - ResolveDefaultSubscription and ResolveSubscription take explicit inputs, have no global state, and are fully deterministic in tests
No broken contracts - SetAzureSubscriptionId remains a pure setter
No tautological assertions - Pure resolver tests assert against known inputs, not the same code path being tested
No layering inversion - EnvironmentHelpers doesn't depend on CommandHelper

Supersedes #2391

Test results

20 CommandHelper tests pass (7 integration + 13 pure resolver)
3 SubscriptionCommand tests pass
8 ACR Registry tests pass
Pre-existing SQL test failures are unrelated (confirmed on base branch)

Invoking Livetests

Copilot submitted PRs are not trustworthy by default. Users with write access to the repo need to validate the contents of this PR before leaving a comment with the text /azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.

Instead of changing SetAzureSubscriptionId to conditionally return CLI profile (which breaks the Set/Get contract and creates tautological assertions), this extracts the resolution logic into pure internal methods with explicit inputs: - ResolveDefaultSubscription(profileSubscription, envSubscription) - ResolveSubscription(optionValue, defaultSubscription) These are fully deterministic and testable without any global state or environment dependencies. The existing GetDefaultSubscription/GetSubscription methods are refactored to delegate to these resolvers. EnvironmentVariableHelpers.SetAzureSubscriptionId is reverted to a pure setter (void, string?) to restore the clean contract. ClearAzureSubscriptionId is kept as a useful addition. Integration tests now use GetDefaultSubscription() for expected values so they pass regardless of whether Azure CLI is logged in. Pure resolver tests cover the priority chain deterministically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Refactors subscription resolution in CommandHelper by extracting deterministic “pure” resolver methods and updates unit tests to be resilient to Azure CLI login state, reducing environment-dependent test failures.

Changes:

Added internal pure resolver methods for default-subscription and option-vs-default subscription resolution, and refactored existing APIs to delegate to them.
Introduced dedicated resolver unit tests and adjusted existing tests to use resolved defaults (CLI profile > env var) and clean up AZURE_SUBSCRIPTION_ID.
Added EnvironmentHelpers.ClearAzureSubscriptionId() and updated tests to use it.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tools/Azure.Mcp.Tools.ContainerApps/tests/Azure.Mcp.Tools.ContainerApps.UnitTests/ContainerApp/ContainerAppListCommandTests.cs	Updates env var handling for validation tests (currently still CLI-state sensitive for empty-args case).
tools/Azure.Mcp.Tools.Acr/tests/Azure.Mcp.Tools.Acr.UnitTests/Registry/RegistryListCommandTests.cs	Makes validation test robust to CLI default-subscription presence; restores env var after test.
core/Microsoft.Mcp.Core/src/Helpers/EnvironmentVariableHelpers.cs	Adds `ClearAzureSubscriptionId()` and simplifies helpers with expression-bodied members.
core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs	Extracts pure resolver methods and refactors `GetDefaultSubscription`/`GetSubscription` to use them.
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperTests.cs	Adjusts expectations to use resolved default subscription and adds cleanup via `IDisposable`.
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperResolverTests.cs	New deterministic unit tests covering resolver priority/placeholder behavior.
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Subscription/SubscriptionCommandTests.cs	Updates expected subscription to resolved default and adds env cleanup via `IDisposable`.

Comments suppressed due to low confidence (1)

tools/Azure.Mcp.Tools.ContainerApps/tests/Azure.Mcp.Tools.ContainerApps.UnitTests/ContainerApp/ContainerAppListCommandTests.cs:86

This validation test still assumes the empty-args case fails. However, SubscriptionCommand validation succeeds whenever CommandHelper.HasSubscriptionAvailable() is true, which includes having an Azure CLI default subscription (even with AZURE_SUBSCRIPTION_ID cleared). Mirror the RegistryListCommandTests behavior by detecting a non-empty CommandHelper.GetDefaultSubscription() and flipping the expected outcome (or split the test into two cases).

    [Theory]
    [InlineData("--subscription sub", true)]
    [InlineData("--subscription sub --resource-group rg", true)]
    [InlineData("", false)]
    public async Task ExecuteAsync_ValidatesInputCorrectly(string args, bool shouldSucceed)
    {
        var originalSubscriptionId = Environment.GetEnvironmentVariable("AZURE_SUBSCRIPTION_ID");
        try
        {
            // Ensure environment variable fallback does not interfere with validation tests
            EnvironmentHelpers.ClearAzureSubscriptionId();
            // Arrange
            if (shouldSucceed)
            {
                _service.ListContainerApps(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<RetryPolicyOptions>(), Arg.Any<CancellationToken>())
                    .Returns(new ResourceQueryResults<ContainerAppInfo>(
                    [
                        new("app1", "eastus", "rg1", "/subscriptions/sub/resourceGroups/rg1/providers/Microsoft.App/managedEnvironments/env1", "Succeeded"),
                        new("app2", "eastus2", "rg2", "/subscriptions/sub/resourceGroups/rg2/providers/Microsoft.App/managedEnvironments/env2", "Succeeded")
                    ], false));
            }

            var parseResult = _commandDefinition.Parse(args);

            // Act
            var response = await _command.ExecuteAsync(_context, parseResult, TestContext.Current.CancellationToken);

            // Assert
            Assert.Equal(shouldSucceed ? HttpStatusCode.OK : HttpStatusCode.BadRequest, response.Status);
            if (shouldSucceed)
            {
                Assert.NotNull(response.Results);
            }
            else
            {
                Assert.Contains("required", response.Message.ToLower());
            }

Copilot · 2026-04-13T22:17:57Z

...Azure.Mcp.Tools.Acr/tests/Azure.Mcp.Tools.Acr.UnitTests/Registry/RegistryListCommandTests.cs

+
+            // When Azure CLI is logged in, the empty-args case will resolve a subscription
+            // from the CLI profile and succeed rather than fail validation.
+            if (!shouldSucceed && CommandHelper.GetDefaultSubscription() != null)


The adjustment for Azure CLI login uses CommandHelper.GetDefaultSubscription() != null, but validation uses !string.IsNullOrEmpty(GetDefaultSubscription()). If the CLI/profile resolution ever yields an empty string, this test would incorrectly flip shouldSucceed to true while validation still fails. Use the same non-empty check as CommandHelper.HasSubscriptionAvailable().

Suggested change

if (!shouldSucceed && CommandHelper.GetDefaultSubscription() != null)

if (!shouldSucceed && CommandHelper.HasSubscriptionAvailable())

Copilot · 2026-04-13T22:17:57Z

core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperResolverTests.cs

+    public void ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty()
+    {
+        var result = CommandHelper.ResolveDefaultSubscription("", "");
+        Assert.Equal("", result);


The test ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty asserts an empty string result when both inputs are empty. That contradicts the production method’s doc (“first non-empty subscription source”) and makes it easier for empty env vars to be treated as a “resolved” value. Consider updating the resolver to treat empty as absent and changing this test to expect null.

Suggested change

public void ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty()

{

var result = CommandHelper.ResolveDefaultSubscription("", "");

Assert.Equal("", result);

public void ResolveDefaultSubscription_ReturnsNull_WhenBothEmpty()

{

var result = CommandHelper.ResolveDefaultSubscription("", "");

Assert.Null(result);

Copilot · 2026-04-13T22:17:58Z

core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperTests.cs

    [Fact]
    public void GetSubscription_EmptySubscriptionParameter_ReturnsEnvironmentValue()
    {
        // Arrange
        EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
+        var expected = CommandHelper.GetDefaultSubscription();
        var parseResult = GetParseResult(["--subscription", ""]);

        // Act
        var actual = CommandHelper.GetSubscription(parseResult);

        // Assert
-        Assert.Equal("env-subs", actual);
+        Assert.Equal(expected, actual);
    }

    [Fact]
    public void GetSubscription_MissingSubscriptionParameter_ReturnsEnvironmentValue()
    {
        // Arrange
        EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
+        var expected = CommandHelper.GetDefaultSubscription();
        var parseResult = GetParseResult([]);

        // Act
        var actual = CommandHelper.GetSubscription(parseResult);

        // Assert
-        Assert.Equal("env-subs", actual);
+        Assert.Equal(expected, actual);


Several test names still claim they validate “environment value” behavior, but the assertions now intentionally accept Azure CLI profile precedence by comparing to GetDefaultSubscription(). Consider renaming these tests (or adding clarifying comments) so the name matches what’s actually being asserted.

Copilot · 2026-04-13T22:17:58Z

...Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Subscription/SubscriptionCommandTests.cs

@@ -56,6 +61,7 @@ public async Task ExecuteAsync_WithEnvironmentVariableOnly_CallsServiceWithCorre
    {
        // Arrange
        EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
+        var expectedSubscription = CommandHelper.GetDefaultSubscription();



These test names still refer to “environment variable only”, but the expected/verified subscription is now the resolved default (Azure CLI profile can override the env var). Rename the tests to reflect that they validate default-subscription resolution rather than specifically env-var behavior.

Copilot · 2026-04-13T22:17:58Z

core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs

+    /// Priority: Azure CLI profile > AZURE_SUBSCRIPTION_ID environment variable.
+    /// </summary>
+    internal static string? ResolveDefaultSubscription(string? profileSubscription, string? envSubscription)
+        => !string.IsNullOrEmpty(profileSubscription) ? profileSubscription : envSubscription;


ResolveDefaultSubscription’s XML doc says it returns the first non-empty source, but the implementation returns envSubscription even when it’s an empty string (e.g., env var set to ""). Consider treating envSubscription as absent when null/empty (and returning null when both sources are null/empty), or adjust the doc/tests to match the intended behavior.

Suggested change

=> !string.IsNullOrEmpty(profileSubscription) ? profileSubscription : envSubscription;

=> !string.IsNullOrEmpty(profileSubscription)

? profileSubscription

: !string.IsNullOrEmpty(envSubscription)

? envSubscription

: null;

Copilot · 2026-04-13T22:17:59Z

core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs

+    /// <summary>
+    /// Pure resolution logic: returns the explicit option value if valid, otherwise the default.
+    /// Placeholder values (containing "subscription" or "default") are treated as absent.
+    /// </summary>
+    internal static string? ResolveSubscription(string? optionValue, string? defaultSubscription)
    {
-        // Primary: Azure CLI profile (set via 'az account set') - cached to avoid repeated file I/O
-        var profileDefault = s_profileDefault.Value;
-        if (!string.IsNullOrEmpty(profileDefault))
+        if (!string.IsNullOrEmpty(optionValue) && !IsPlaceholder(optionValue))
        {
-            return profileDefault;
+            return optionValue;
        }

-        // Fallback: AZURE_SUBSCRIPTION_ID environment variable (cheap, not cached)
-        return EnvironmentHelpers.GetAzureSubscriptionId();
+        return !string.IsNullOrEmpty(defaultSubscription)
+            ? defaultSubscription
+            : optionValue;


ResolveSubscription’s XML doc says placeholder values are treated as absent, but the method currently returns the placeholder when defaultSubscription is null/empty (to preserve the old behavior). Either update the documentation to reflect this fallback, or change the behavior so placeholders are never returned.

Copilot · 2026-04-13T22:17:59Z

core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs

+            : optionValue;
    }

    private static bool IsPlaceholder(string value) => value.Contains("subscription") || value.Contains("default");


IsPlaceholder uses case-sensitive substring matching. This will miss placeholder text like "Azure Subscription 1" or "Default" depending on casing, which can cause unintended validation behavior. Use a StringComparison (e.g., OrdinalIgnoreCase) when checking for placeholder tokens.

Suggested change

private static bool IsPlaceholder(string value) => value.Contains("subscription") || value.Contains("default");

private static bool IsPlaceholder(string value) =>

value.Contains("subscription", StringComparison.OrdinalIgnoreCase) ||

value.Contains("default", StringComparison.OrdinalIgnoreCase);

jongio · 2026-04-13T22:20:40Z

Recreating to target the correct base branch (FixLoggedInTests from PR #2391)

alzimmermsft and others added 2 commits April 13, 2026 10:52

Fix subscription ID tests when logged in with Azure CLI

1b0fc5f

jongio requested review from a team as code owners April 13, 2026 22:10

jongio requested review from JasonYeMSFT, JonathanCrd, KarishmaGhiya, alzimmermsft, Copilot, hallipr, joshfree and tmeschter April 13, 2026 22:10

github-project-automation bot added this to Azure MCP Server Apr 13, 2026

github-project-automation bot moved this to Untriaged in Azure MCP Server Apr 13, 2026

github-actions bot added the tools-ACR label Apr 13, 2026

Copilot started reviewing on behalf of jongio April 13, 2026 22:14 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

jongio closed this Apr 13, 2026

github-project-automation bot moved this from Untriaged to Done in Azure MCP Server Apr 13, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: extract pure resolver methods for subscription resolution#2395

refactor: extract pure resolver methods for subscription resolution#2395
jongio wants to merge 2 commits intomicrosoft:mainfrom
jongio:fix/subscription-test-pure-resolvers

jongio commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

jongio commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (!shouldSucceed && CommandHelper.GetDefaultSubscription() != null)
	if (!shouldSucceed && CommandHelper.HasSubscriptionAvailable())

Conversation

jongio commented Apr 13, 2026

Summary

Changes

Production code

Test code

Why this approach

Test results

Invoking Livetests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

jongio commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants