Addressing additional copilot comments

Author: deepaligargms

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

@@ -311,6 +311,11 @@ private static Command CreatePublishSubcommand(
             description: "Publisher name written into the MOS package manifest. Required for custom (user-created) MCP servers; ignored for 1p Microsoft-owned servers (e.g. msdyn_DataverseMCPServer) which always publish as 'Microsoft'.");
         command.AddOption(publisherNameOption);
 
+        var yesOption = new Option<bool>(
+            ["--yes", "-y"],
+            description: "Skip the interactive 'Proceed with publish? (y/N)' confirmation. Useful for non-interactive contexts (CI/CD pipelines, scripts). Matches az CLI convention.");
+        command.AddOption(yesOption);
+
         var dryRunOption = new Option<bool>("--dry-run", "Show what would be done without executing");
         command.AddOption(dryRunOption);
 
@@ -325,6 +330,7 @@ private static Command CreatePublishSubcommand(
                 Alias: context.ParseResult.GetValueForOption(aliasOption),
                 DisplayName: context.ParseResult.GetValueForOption(displayNameOption),
                 PublisherName: context.ParseResult.GetValueForOption(publisherNameOption),
+                Yes: context.ParseResult.GetValueForOption(yesOption),
                 DryRun: context.ParseResult.GetValueForOption(dryRunOption));
 
             var executor = new PublishCommandExecutor(logger, toolingService, graphApiService);

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

@@ -18,6 +18,7 @@ internal record RawPublishArgs(
     string? Alias,
     string? DisplayName,
     string? PublisherName,
+    bool Yes,
     bool DryRun);
 
 /// <summary>
@@ -32,6 +33,9 @@ internal record RawPublishArgs(
 /// </summary>
 internal class PublishCommandExecutor
 {
+    // protected (instead of private) on the seam methods + non-sealed class so tests can stub
+    // out the parts that hit external systems (Azure CLI for tenant detection). The class is
+    // still internal — overrides only happen in the test assembly via InternalsVisibleTo.
     private readonly ILogger _logger;
     private readonly IAgent365ToolingService _toolingService;
     private readonly GraphApiService? _graphApiService;
@@ -62,6 +66,10 @@ private sealed record ResolvedInput
         // classify ahead of time without knowing the server's mapping, so it leaves the value
         // null when unspecified and lets the platform decide.
         public string? PublisherName { get; init; }
+
+        // When true, skip the interactive "Proceed with publish? (y/N)" confirmation. Set via
+        // --yes / -y. Required for non-interactive contexts (CI scripts, automation).
+        public required bool Yes { get; init; }
     }
 
     internal sealed record EntraAppSet(
@@ -94,14 +102,21 @@ internal async Task<bool> ExecuteAsync(RawPublishArgs args, CancellationToken ct
             return true;
         }
 
-        Console.Write("Proceed with publish? (y/N): ");
-        var confirmation = Console.ReadLine()?.Trim().ToLowerInvariant();
-        if (confirmation != "y" && confirmation != "yes")
+        if (!input.Yes)
         {
-            Console.WriteLine("Publish cancelled.");
-            // User cancellation is not a failure — exit 0. Matches the register command's same
-            // prompt-cancel path.
-            return true;
+            Console.Write("Proceed with publish? (y/N): ");
+            var confirmation = Console.ReadLine()?.Trim().ToLowerInvariant();
+            if (confirmation != "y" && confirmation != "yes")
+            {
+                Console.WriteLine("Publish cancelled.");
+                // User cancellation is not a failure — exit 0. Matches the register command's same
+                // prompt-cancel path.
+                return true;
+            }
+        }
+        else
+        {
+            _logger.LogDebug("Skipping interactive confirmation (--yes was supplied).");
         }
 
         Console.WriteLine();
@@ -276,6 +291,7 @@ internal async Task<bool> ExecuteAsync(RawPublishArgs args, CancellationToken ct
                 Alias = alias,
                 DisplayName = displayName,
                 PublisherName = string.IsNullOrWhiteSpace(publisherName) ? null : publisherName,
+                Yes = args.Yes,
                 DryRun = args.DryRun,
             };
         }
@@ -302,7 +318,7 @@ private void DisplayPublishSummary(ResolvedInput input)
         Console.WriteLine();
     }
 
-    private async Task<string?> DetectTenantIdAsync()
+    protected virtual async Task<string?> DetectTenantIdAsync()
     {
         var tenantId = await TenantDetectionHelper.DetectTenantIdAsync(null, _logger);
 

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

@@ -95,13 +95,13 @@ public async Task AzureCliStyleParameters_AreAcceptedCorrectly(string command, p
     }
 
     [Fact]
-    public async Task ServiceIntegration_PublishCommand_AcceptsAllNamedParameters()
+    public async Task PublishCommand_AcceptsAllNamedParameters_InDryRun()
     {
-        // Verifies the publish CLI parses every documented flag without error. The publish flow now
-        // orchestrates Entra app creation + redirect-URI back-fill via GraphApiService (mirroring
-        // register-external-mcp-server), so end-to-end "params flow to PublishServerAsync" can't be
-        // exercised here without mocking Graph too — that path is covered by the
-        // <see cref="DryRunMode_NeverCallsActualServices"/> regression test and by manual E2E testing.
+        // Verifies the publish CLI parses every documented flag without error. Dry-run
+        // short-circuits before any platform call, so this is a pure CLI parsing test —
+        // it does NOT verify that the parsed values flow into PublishServerAsync.
+        // That contract is covered by PublishCommand_ForwardsParsedParametersToToolingService
+        // (which mocks Graph + tenant detection so the non-dry-run path can be exercised).
 
         // Arrange
         var testEnvId = "test-environment-123";
@@ -121,8 +121,140 @@ public async Task ServiceIntegration_PublishCommand_AcceptsAllNamedParameters()
         });
 
         // Assert — successful parse + dispatch, no service calls.
-        result.Should().Be(0);
-        await _mockToolingService.DidNotReceive().PublishServerAsync(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<PublishMcpServerRequest>());
+        result.Should().Be(
+            0,
+            because: "dry-run should never trigger a non-zero exit code when all flags parse cleanly.");
+        await _mockToolingService.DidNotReceive().PublishServerAsync(
+            Arg.Any<string>(), Arg.Any<string>(), Arg.Any<PublishMcpServerRequest>());
+    }
+
+    /// <summary>
+    /// Strengthened contract test: verifies that every parsed publish flag flows into the actual
+    /// <see cref="IAgent365ToolingService.PublishServerAsync"/> call. Exercises the non-dry-run
+    /// path by (a) using <c>--yes</c> to bypass the interactive confirmation prompt, (b) mocking
+    /// <see cref="GraphApiService"/> so Entra app creation succeeds without a real tenant, and
+    /// (c) subclassing <see cref="PublishCommandExecutor"/> to stub out tenant auto-detection so
+    /// the executor doesn't shell out to Azure CLI in CI. Catches breakage of the CLI-to-service
+    /// contract (alias/display-name/publisher-name mapping, request shaping, and selecting the
+    /// correct service method) that the dry-run-only test above can't catch.
+    /// </summary>
+    [Fact]
+    public async Task PublishCommand_ForwardsParsedParametersToToolingService()
+    {
+        // Arrange
+        const string TestTenantId = "test-tenant-99999";
+        const string TestEnvironmentId = "test-env-forward";
+        const string TestServerName = "msdyn_TestServer";
+        const string TestAlias = "test-alias-forward";
+        const string TestDisplayName = "Test Display Forward";
+        const string TestPublisherName = "Contoso Forward";
+        const string TestPublicClientsObjectId = "public-clients-object-id";
+        const string TestPublicClientsClientId = "public-clients-client-id";
+
+        var logger = Substitute.For<ILogger>();
+        var toolingService = Substitute.For<IAgent365ToolingService>();
+        var graphApiService = Substitute.For<GraphApiService>();
+
+        // Mock Graph so CreateEntraAppsAsync → factory.CreatePublicClientsAppAsync succeeds.
+        graphApiService.CreateEntraAppAsync(
+                TestTenantId, Arg.Any<string>(), serviceTreeId: Arg.Any<string?>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<(string ObjectId, string ClientId)?>(
+                (TestPublicClientsObjectId, TestPublicClientsClientId)));
+        graphApiService.UpdateAppPublicClientRedirectUrisAsync(
+                TestTenantId, TestPublicClientsObjectId, Arg.Any<string[]>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult(true));
+
+        // Mock Graph for ConfigureEntraAppsAsync → required-resource-access grant on Public Clients.
+        graphApiService.GetOAuth2PermissionScopeIdAsync(
+                TestTenantId, Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<Guid?>(Guid.NewGuid()));
+        graphApiService.AddRequiredResourceAccessAsync(
+                TestTenantId, Arg.Any<string>(), Arg.Any<string>(), Arg.Any<Guid>(), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult(true));
+
+        // Capture what gets forwarded to PublishServerAsync.
+        string? capturedEnvId = null;
+        string? capturedServerName = null;
+        PublishMcpServerRequest? capturedRequest = null;
+        toolingService.PublishServerAsync(
+                Arg.Do<string>(e => capturedEnvId = e),
+                Arg.Do<string>(s => capturedServerName = s),
+                Arg.Do<PublishMcpServerRequest>(r => capturedRequest = r),
+                Arg.Any<CancellationToken>())
+            .Returns(new PublishMcpServerResponse
+            {
+                Status = "Success",
+                McpServerAppId = Guid.NewGuid().ToString(),
+                McpServerScope = "Tools.ListInvoke.All",
+            });
+
+        var executor = new TestablePublishCommandExecutor(
+            logger, toolingService, graphApiService, TestTenantId);
+
+        var args = new RawPublishArgs(
+            EnvironmentId: TestEnvironmentId,
+            ServerName: TestServerName,
+            Alias: TestAlias,
+            DisplayName: TestDisplayName,
+            PublisherName: TestPublisherName,
+            Yes: true,
+            DryRun: false);
+
+        // Act
+        var result = await executor.ExecuteAsync(args);
+
+        // Assert
+        result.Should().BeTrue(
+            because: "the happy path with all dependencies mocked must return true so the publish " +
+                     "handler exits 0.");
+        capturedRequest.Should().NotBeNull(
+            because: "PublishServerAsync must be invoked once the prompt is skipped, the tenant " +
+                     "is detected, and the Public Clients app is created.");
+        capturedEnvId.Should().Be(
+            TestEnvironmentId,
+            because: "--environment-id must flow unchanged into PublishServerAsync's first positional arg.");
+        capturedServerName.Should().Be(
+            TestServerName,
+            because: "--server-name must flow unchanged into PublishServerAsync's second positional arg.");
+        capturedRequest!.Alias.Should().Be(
+            TestAlias,
+            because: "--alias must populate request.Alias; this is the platform's `name` for the published row.");
+        capturedRequest.DisplayName.Should().Be(
+            TestDisplayName,
+            because: "--display-name must populate request.DisplayName; the platform's v2 validator " +
+                     "requires it and surfaces it in MOS.");
+        capturedRequest.PublisherName.Should().Be(
+            TestPublisherName,
+            because: "--publisher-name must populate request.PublisherName so the MOS manifest's " +
+                     "developer field is set for custom servers (the platform rejects empty values " +
+                     "for non-1p servers).");
+        capturedRequest.PublicClientsAppId.Should().Be(
+            TestPublicClientsClientId,
+            because: "the just-created Public Clients Entra app's clientId must be carried to the " +
+                     "platform so it can be echoed back and the CLI can grant the PPMI scope on it " +
+                     "post-publish.");
+    }
+
+    /// <summary>
+    /// Test-only subclass of <see cref="PublishCommandExecutor"/> that stubs out
+    /// <see cref="PublishCommandExecutor.DetectTenantIdAsync"/> with a known value, so the
+    /// strengthened contract test doesn't need to shell out to <c>az account show</c> in CI.
+    /// </summary>
+    private sealed class TestablePublishCommandExecutor : PublishCommandExecutor
+    {
+        private readonly string? _tenantId;
+
+        public TestablePublishCommandExecutor(
+            ILogger logger,
+            IAgent365ToolingService toolingService,
+            GraphApiService? graphApiService,
+            string? tenantId)
+            : base(logger, toolingService, graphApiService)
+        {
+            _tenantId = tenantId;
+        }
+
+        protected override Task<string?> DetectTenantIdAsync() => Task.FromResult(_tenantId);
     }
 
     [Fact]

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

@@ -37,6 +37,7 @@ public async Task ExecuteAsync_DryRun_LogsEntraAppNamesDerivedFromServerName()
             Alias: alias,
             DisplayName: "Test Display",
             PublisherName: null,
+            Yes: false,
             DryRun: true);
 
         var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null);
@@ -89,6 +90,7 @@ public async Task ExecuteAsync_DryRun_OmitsA365ProxyApp_WhileCustomConnectorFlow
             Alias: alias,
             DisplayName: "Test Display",
             PublisherName: null,
+            Yes: false,
             DryRun: true);
 
         var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null);
@@ -139,6 +141,7 @@ public async Task ExecuteAsync_DryRun_AcceptsPublisherName_WithoutCallingPlatfor
             Alias: "myAlias",
             DisplayName: "Test Display",
             PublisherName: "Contoso",
+            Yes: false,
             DryRun: true);
 
         var executor = new PublishCommandExecutor(logger, toolingService, graphApiService: null);