Address review comments on PR #418

Grant flagged two real issues in the #412 follow-up. Failing tests
written first, then fixes; both tests now green.

- DevelopCommand: --dry-run short-circuited before the new
  --project-path typo-guard, so `--project-path /typo --dry-run`
  silently exited 0 — exactly the failure the guard was meant to
  prevent. Moved the path resolution and typo-guard above the
  dry-run block so dry-run validates --project-path the same way
  real mode does. Dry-run output now also includes the resolved
  manifest path in the "Would update" line.

- CleanupCommand.BuildBootstrapConfigForCleanupAsync (the fallback
  used when resolver == null) was a near-duplicate of the resolver
  copy and still carried the global-generated-config fallback that
  the original PR removed. Removed the global fallback in the
  duplicate too. Added a TODO to collapse the duplication in a
  follow-up.

New regression tests:

- AddMcpServers_DryRunWithBogusProjectPath_ReturnsExitCode1 —
  invocation test that fails today (exit 0) and passes after the
  fix (exit 1, no dir created).

- CleanupCommand_BuildBootstrapConfigForCleanupAsync_DoesNotInheritResourceIdsFromOutsideCwd —
  reflection-based unit test against the private static duplicate.
  Fails today (poisoned global resource IDs leak through), passes
  after the fix. If a future PR deletes the duplicate, the BindingFlags
  lookup will fail loudly and the test can be removed alongside.

Full suite: 1436 passed / 0 failed / 12 skipped.
Manual verify: typo'd --project-path with --dry-run now exits 1 with
a clear directory-does-not-exist error and creates nothing on disk.

Author: sellakumaran

src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs

@@ -1362,9 +1362,12 @@ private static void PrintOrphanSummary(
         // Step 4: Load generated config.
         // Only take agentRegistrationId from the file when the blueprint IDs match,
         // confirming the file belongs to this agent.
-        var localGeneratedPath = Path.Combine(Environment.CurrentDirectory, "a365.generated.config.json");
-        var globalGeneratedPath = Path.Combine(ConfigService.GetGlobalConfigDirectory(), "a365.generated.config.json");
-        var generatedConfigPath = File.Exists(localGeneratedPath) ? localGeneratedPath : globalGeneratedPath;
+        // Project-local only — the global-directory fallback was removed because it could
+        // pull resource IDs from a different project's leftover state. Mirrors the same
+        // policy applied to BootstrapConfigResolver.BuildBootstrapConfigForCleanupAsync.
+        // TODO: this method is a near-duplicate of the resolver copy; collapse into a single
+        // shared helper in a follow-up PR.
+        var generatedConfigPath = Path.Combine(Environment.CurrentDirectory, "a365.generated.config.json");
 
         string? agentRegistrationId = null;
         string? agenticAppId = null;

src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopCommand.cs

@@ -501,20 +501,11 @@ private static Command CreateAddMcpServersSubcommand(ILogger logger, IConfigServ
                 return;
             }
 
-            // Dry run mode
-            if (dryRun)
-            {
-                logger.LogInformation("[DRY RUN] Would add the following MCP servers to configuration:");
-                foreach (var serverName in servers)
-                {
-                    logger.LogInformation("[DRY RUN]   - {Server}", serverName);
-                }
-                logger.LogInformation("[DRY RUN] Would update {FileName}", McpConstants.ToolingManifestFileName);
-                await Task.CompletedTask;
-                return;
-            }
-
-            // Resolve manifest path without requiring a365.config.json
+            // Resolve manifest path without requiring a365.config.json.
+            // Resolution and the --project-path typo-guard MUST run before the --dry-run
+            // short-circuit below: dry-run is a preview of what the real command would do,
+            // so a typo'd --project-path must fail dry-run too — otherwise users get a
+            // misleading exit-0 preview and a confusing exit-1 when they run for real.
             var manifestPath = await ResolveToolingManifestPath(projectPath, configService, logger);
 
             if (!File.Exists(manifestPath))
@@ -533,6 +524,20 @@ private static Command CreateAddMcpServersSubcommand(ILogger logger, IConfigServ
                     McpConstants.ToolingManifestFileName, manifestPath);
             }
 
+            // Dry run mode
+            if (dryRun)
+            {
+                logger.LogInformation("[DRY RUN] Would add the following MCP servers to configuration:");
+                foreach (var serverName in servers)
+                {
+                    logger.LogInformation("[DRY RUN]   - {Server}", serverName);
+                }
+                logger.LogInformation("[DRY RUN] Would update {FileName} at {Path}",
+                    McpConstants.ToolingManifestFileName, manifestPath);
+                await Task.CompletedTask;
+                return;
+            }
+
             try
             {
                 // Read existing manifest if present

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AddMcpServersMissingManifestTests.cs

@@ -199,6 +199,28 @@ public async Task AddMcpServers_DryRunWithNoManifest_DoesNotCreateManifest()
             .Should().BeFalse(because: "--dry-run must never write to disk");
     }
 
+    [Fact]
+    public async Task AddMcpServers_DryRunWithBogusProjectPath_ReturnsExitCode1()
+    {
+        // DevelopCommand.cs:504-515 BEFORE the --project-path typo-guard at lines
+        // 520-530. A typo'd --project-path X --dry-run silently exits 0 — exactly
+        // the failure the guard is supposed to prevent.
+        var bogusDir = Path.Combine(Path.GetTempPath(),
+            "a365-issue412-doesnotexist-dryrun-" + Guid.NewGuid().ToString("N"));
+        Directory.Exists(bogusDir).Should().BeFalse(because: "the test pre-condition is a typo'd path");
+
+        var exitCode = await BuildRoot().InvokeAsync(new[]
+        {
+            "develop", "add-mcp-servers", "mcp_M365Copilot",
+            "--project-path", bogusDir, "--dry-run"
+        });
+
+        exitCode.Should().Be(1,
+            because: "--dry-run must validate --project-path the same way real-mode does; otherwise dry-run gives users false confidence that their command would succeed when run for real");
+        Directory.Exists(bogusDir).Should().BeFalse(
+            because: "--dry-run must never create directories, even by accident");
+    }
+
     [Fact]
     public async Task AddMcpServers_ExistingManifest_UpsertsServer()
     {

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GlobalConfigDirectoryCleanupTests.cs

@@ -1,7 +1,9 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System.Reflection;
 using FluentAssertions;
+using Microsoft.Agents.A365.DevTools.Cli.Commands;
 using Microsoft.Agents.A365.DevTools.Cli.Constants;
 using Microsoft.Agents.A365.DevTools.Cli.Models;
 using Microsoft.Agents.A365.DevTools.Cli.Services;
@@ -207,4 +209,90 @@ await File.WriteAllTextAsync(globalGeneratedPath, $$"""
         result.AgentBlueprintServicePrincipalObjectId.Should().BeNull(
             because: "AgentBlueprintServicePrincipalObjectId must not leak in from outside CWD");
     }
+
+    [Fact]
+    public async Task CleanupCommand_BuildBootstrapConfigForCleanupAsync_DoesNotInheritResourceIdsFromOutsideCwd()
+    {
+        // The global-fallback pattern the PR removed from
+        // BootstrapConfigResolver.BuildBootstrapConfigForCleanupAsync still exists as
+        // a near-identical duplicate in CleanupCommand.BuildBootstrapConfigForCleanupAsync
+        // (the fallback path used when resolver == null). The original PR fixed one of
+        // two copies; this test pins the same invariant on the duplicate so the fix
+        // can land symmetrically (or the duplicate can be deleted, in which case this
+        // test will fail with a clear "method not found" and can be removed too).
+        //
+        // Uses reflection because the method is `private static`. If the production code
+        // is refactored to delete or rename the method, the BindingFlags lookup will
+        // surface the change loudly rather than silently passing.
+        var poisonedBlueprintId = Guid.NewGuid().ToString();
+        const string poisonedRegistrationId = "REG_FROM_GLOBAL_SHOULD_NOT_LEAK_VIA_CLEANUP";
+        var globalGeneratedDir = ResolvedGlobalConfigDir();
+        Directory.CreateDirectory(globalGeneratedDir);
+        var globalGeneratedPath = Path.Combine(globalGeneratedDir, "a365.generated.config.json");
+        // Casing intentional: matches the duplicate reader's case-sensitive
+        // SetupHelpers.GetJsonString lookups. "AgenticAppId" is PascalCase by design
+        // — see the ResolveAsync_InCleanupMode test above for the full rationale.
+        await File.WriteAllTextAsync(globalGeneratedPath, $$"""
+        {
+          "agentBlueprintId": "{{poisonedBlueprintId}}",
+          "agentRegistrationId": "{{poisonedRegistrationId}}",
+          "AgenticAppId": "AGENTIC_FROM_GLOBAL_SHOULD_NOT_LEAK_VIA_CLEANUP",
+          "agentBlueprintServicePrincipalObjectId": "SP_FROM_GLOBAL_SHOULD_NOT_LEAK_VIA_CLEANUP"
+        }
+        """);
+        File.Exists(Path.Combine(_localCwd, "a365.generated.config.json")).Should().BeFalse(
+            because: "the CWD must be clean so the only available source of poison is the global file");
+
+        var executorLogger = Substitute.For<ILogger<CommandExecutor>>();
+        var executor = Substitute.For<CommandExecutor>(executorLogger);
+        executor.ExecuteAsync(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string?>(),
+                Arg.Any<bool>(), Arg.Any<bool>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult(new CommandResult
+            {
+                ExitCode = 0, StandardOutput = "explicit-tenant", StandardError = string.Empty
+            }));
+
+        var graphApiService = Substitute.ForPartsOf<GraphApiService>(
+            Substitute.For<ILogger<GraphApiService>>(),
+            executor,
+            (Func<Task<string?>>)(() => Task.FromResult<string?>(null)));
+        // The cleanup duplicate calls FindApplicationByDisplayNameAsync WITHOUT a
+        // CancellationToken argument (see CleanupCommand.cs ~line 1356-1357), so the
+        // 2-arg overload must be stubbed in addition to the 3-arg one for safety.
+        graphApiService.FindApplicationByDisplayNameAsync(
+                Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<string?>(poisonedBlueprintId));
+        graphApiService.FindApplicationByDisplayNameAsync(
+                Arg.Any<string>(), Arg.Any<string>())
+            .Returns(Task.FromResult<string?>(poisonedBlueprintId));
+
+        var cleanupLogger = Substitute.For<ILogger<CleanupCommand>>();
+
+        var method = typeof(CleanupCommand).GetMethod(
+            "BuildBootstrapConfigForCleanupAsync",
+            BindingFlags.NonPublic | BindingFlags.Static);
+        method.Should().NotBeNull(
+            because: "the private static fallback duplicate must exist for this regression test to verify; if a later PR collapses or deletes the duplicate, delete this test too");
+
+        var task = (Task<Agent365Config?>)method!.Invoke(null, new object?[]
+        {
+            "CleanupAgent",
+            "explicit-tenant",
+            executor,
+            graphApiService,
+            cleanupLogger
+        })!;
+        var result = await task;
+
+        result.Should().NotBeNull(
+            because: "the fallback must still build a valid config from Entra even when no local generated file exists");
+        result!.AgentBlueprintId.Should().Be(poisonedBlueprintId,
+            because: "the blueprint ID is the authoritative Entra value, independent of any on-disk file");
+        result.AgentRegistrationId.Should().BeNull(
+            because: "AgentRegistrationId must not leak in from outside CWD via the cleanup duplicate — only the project-local generated config may be consulted");
+        result.AgenticAppId.Should().BeNull(
+            because: "AgenticAppId must not leak in from outside CWD via the cleanup duplicate");
+        result.AgentBlueprintServicePrincipalObjectId.Should().BeNull(
+            because: "AgentBlueprintServicePrincipalObjectId must not leak in from outside CWD via the cleanup duplicate");
+    }
 }