Address PR #362 review: validation, bounded telemetry, IDisposable, env-config, clearer messages, FAQ move

Resolves the remaining review comments (sellakumaran):

- [6] --output-dir: validate up front in RunAsync; exit 1 on empty/whitespace
  instead of a deep exception late in the run (test added).
- [3] telemetry: bound LogEvaluateUsageAsync with a 5s linked-CTS timeout so a
  slow/hung endpoint cannot stall discovery; reword the misleading "fire-and-forget".
- [5] auth token: warn when --auth-token is used (visible to ps / shell history)
  and fall back to A365_MCP_AUTH_TOKEN; document the env var in help.
- [8] trust boundary: state it in the evaluate command help (size cap deferred).
- [4] SchemaDiscoveryService: implement IDisposable to dispose the owned HttpClient.
- [9] model IDs: centralize in EvalModelConstants with A365_EVAL_COPILOT_MODEL /
  A365_EVAL_CLAUDE_MODEL overrides; drop the "update when newer" comment.
- [1b] no-agent message: name only the requested engine (or all, for auto), built
  from the launcher registry; [1c] log probe failure at Debug and mention -v.
- [11] move FAQ.md to docs/agent365-guided-setup/a365-evaluate-faq.md; remove a
  stale (broken) docs/rai link.

Verified by running the evaluate subcommand: empty --output-dir exits 1; a resume
run loads the checklist, analyzes, and writes the report (exit 0). Build clean,
281 tests green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: ashragrawal

…65.DevTools.Cli/Services/Evaluate/FAQ.md …ent365-guided-setup/a365-evaluate-faq.mdsrc/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/FAQ.md renamed to docs/agent365-guided-setup/a365-evaluate-faq.md src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/FAQ.md renamed to docs/agent365-guided-setup/a365-evaluate-faq.md

@@ -71,7 +71,3 @@ Three output files are written to the directory you specify with `--output-dir`:
 Temporary files created during the run (the prompt file passed to the coding agent CLI) are deleted automatically when the run finishes or is cancelled. The auth token supplied via `--auth-token` is never written to disk.
 
 If you want to remove all evaluation output, delete the three files above from your output directory. No other local state is created by this command.
-
----
-
-For the full technical record — threat model, STRIDE analysis, and XPIA mitigation details — see [`docs/rai/`](../../../../docs/rai/README.md).

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

@@ -47,7 +47,7 @@ public static Command CreateCommand(
 
         if (evaluationPipelineService is not null)
         {
-            developMcpCommand.AddCommand(CreateEvaluateSubcommand(evaluationPipelineService));
+            developMcpCommand.AddCommand(CreateEvaluateSubcommand(logger, evaluationPipelineService));
         }
 
         return developMcpCommand;
@@ -56,14 +56,15 @@ public static Command CreateCommand(
     /// <summary>
     /// Creates the evaluate subcommand for MCP server tool schema quality evaluation.
     /// </summary>
-    private static Command CreateEvaluateSubcommand(IEvaluationPipelineService pipelineService)
+    private static Command CreateEvaluateSubcommand(ILogger logger, IEvaluationPipelineService pipelineService)
     {
         var command = new Command(
             "evaluate",
             "Evaluate MCP server tool schema quality and generate an HTML report. " +
             "Uses a locally installed coding agent (GitHub Copilot or Claude Code) to score semantic checks. " +
             "If no agent is detected, the command stops after writing the checklist so you can score it manually with your own LLM, " +
-            "or pass --eval-engine none to skip agent probing entirely.");
+            "or pass --eval-engine none to skip agent probing entirely. " +
+            "Only run this against MCP servers you trust: the server's tool names and descriptions are sent to a locally running coding agent.");
 
         // Use a required option (not a positional argument) for consistency with other
         // develop-mcp subcommands and Azure CLI conventions.
@@ -89,7 +90,7 @@ private static Command CreateEvaluateSubcommand(IEvaluationPipelineService pipel
 
         var authTokenOption = new Option<string?>(
             "--auth-token",
-            "Bearer token for MCP server authentication");
+            "Bearer token for MCP server authentication. Prefer the A365_MCP_AUTH_TOKEN environment variable; a token passed on the command line is visible to process listings (ps / Task Manager) and shell history.");
 
         command.AddOption(serverUrlOption);
         command.AddOption(outputDirOption);
@@ -104,6 +105,19 @@ private static Command CreateEvaluateSubcommand(IEvaluationPipelineService pipel
             var authToken = context.ParseResult.GetValueForOption(authTokenOption);
             var ct = context.GetCancellationToken();
 
+            // Secret handling: a token on the command line is visible to other processes
+            // (ps / Task Manager) and lands in shell history. A non-empty value here can
+            // only have come from --auth-token, so warn and steer to the env var; when the
+            // flag is absent, fall back to A365_MCP_AUTH_TOKEN.
+            if (!string.IsNullOrWhiteSpace(authToken))
+            {
+                logger.LogWarning("Passing a token via --auth-token exposes it to process listings (ps / Task Manager) and shell history. Prefer the A365_MCP_AUTH_TOKEN environment variable.");
+            }
+            else
+            {
+                authToken = Environment.GetEnvironmentVariable("A365_MCP_AUTH_TOKEN");
+            }
+
             context.ExitCode = await pipelineService.RunAsync(serverUrl, outputDir, evalEngine, authToken, ct);
         });
 

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/ChecklistEvaluator.cs

@@ -81,7 +81,7 @@ public async Task<ChecklistEvaluationResult> EvaluateAsync(
         if (engine == EvalEngine.None)
         {
             await WriteChecklistAsync(checklist, checklistPath, cancellationToken);
-            LogManualEvaluationInstructions(checklistPath, totalUnevaluatedBefore, engineNotFound: false, agentAttempted: false);
+            LogManualEvaluationInstructions(checklistPath, totalUnevaluatedBefore, engineNotFound: false, agentAttempted: false, requested: engine);
             return new ChecklistEvaluationResult { Checklist = checklist, Outcome = EvaluationOutcome.OptedOut };
         }
 
@@ -93,7 +93,7 @@ public async Task<ChecklistEvaluationResult> EvaluateAsync(
 
         if (enginesToTry.Count == 0)
         {
-            LogManualEvaluationInstructions(checklistPath, totalUnevaluatedBefore, engineNotFound: true, agentAttempted: false);
+            LogManualEvaluationInstructions(checklistPath, totalUnevaluatedBefore, engineNotFound: true, agentAttempted: false, requested: engine);
             return new ChecklistEvaluationResult { Checklist = checklist, Outcome = EvaluationOutcome.CouldNotEvaluate };
         }
 
@@ -183,7 +183,7 @@ public async Task<ChecklistEvaluationResult> EvaluateAsync(
             // hit tool-permission limits, timed out, or returned without edits. Rather
             // than silently producing an inflated report, give the user the same BYOL
             // fallback they'd get if no agent was installed at all.
-            LogManualEvaluationInstructions(checklistPath, remainingUnevaluated, engineNotFound: false, agentAttempted: true);
+            LogManualEvaluationInstructions(checklistPath, remainingUnevaluated, engineNotFound: false, agentAttempted: true, requested: engine);
         }
 
         // Only treat evaluation as completed when nothing is left unscored.
@@ -617,7 +617,7 @@ private static int CountTotalSemanticChecks(EvaluationChecklist checklist)
         return count;
     }
 
-    private void LogManualEvaluationInstructions(string checklistPath, int unscoredCount, bool engineNotFound, bool agentAttempted)
+    private void LogManualEvaluationInstructions(string checklistPath, int unscoredCount, bool engineNotFound, bool agentAttempted, EvalEngine requested)
     {
         var fullPath = Path.GetFullPath(checklistPath);
         var promptPath = Path.Combine(Path.GetDirectoryName(fullPath) ?? ".", "semantic_eval_prompt.txt");
@@ -635,7 +635,20 @@ private void LogManualEvaluationInstructions(string checklistPath, int unscoredC
 
         if (engineNotFound)
         {
-            _logger.LogWarning("      No coding agent CLI detected (looked for `copilot` and `claude`)");
+            if (requested == EvalEngine.Auto)
+            {
+                // Built from the registry so a newly added engine appears here automatically.
+                var probed = string.Join(" and ", _launchers.Select(l => $"{l.DisplayName} (`{l.CliCommand}`)"));
+                _logger.LogWarning("      No coding agent CLI detected (looked for {Probed}). Run with -v to see why each probe failed.", probed);
+            }
+            else
+            {
+                // The user asked for one specific engine; name only that one, not both.
+                var launcher = LauncherFor(requested);
+                var name = launcher?.DisplayName ?? FormatEngineName(requested);
+                var binary = launcher?.CliCommand ?? requested.ToString();
+                _logger.LogWarning("      {Name} CLI not found on PATH (looked for `{Binary}`). Run with -v to see why the probe failed.", name, binary);
+            }
         }
         else if (agentAttempted)
         {

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/ClaudeCodeLauncher.cs

@@ -29,7 +29,7 @@ public ClaudeCodeLauncher(CommandExecutor executor, ILogger<ClaudeCodeLauncher>
 
     public override SemanticCheckPrompts.AgentToolset Toolset => new(ReadToolName: "Read", EditToolName: "Edit");
 
-    protected override string ProbeCommand => "claude";
+    public override string CliCommand => "claude";
 
     public override async Task<bool> LaunchAsync(
         string prompt,
@@ -63,7 +63,7 @@ private async Task<bool> LaunchViaFileAsync(
             await File.WriteAllTextAsync(promptFile, prompt, cancellationToken);
 
             var metaPrompt = $"Read and follow the instructions in the file at: {promptFile}";
-            var (fileName, fileArguments) = WrapForPlatform("claude", $"-p \"{metaPrompt}\" --model haiku --allowedTools Read,Edit");
+            var (fileName, fileArguments) = WrapForPlatform("claude", $"-p \"{metaPrompt}\" --model {EvalModelConstants.ClaudeModel} --allowedTools Read,Edit");
 
             var startInfo = new ProcessStartInfo
             {
@@ -98,7 +98,7 @@ private async Task<bool> LaunchViaStdinAsync(
         var startInfo = new ProcessStartInfo
         {
             FileName = "claude",
-            Arguments = "-p - --model haiku --allowedTools Read,Edit",
+            Arguments = $"-p - --model {EvalModelConstants.ClaudeModel} --allowedTools Read,Edit",
             WorkingDirectory = workingDirectory,
             RedirectStandardInput = true,
             RedirectStandardOutput = true,

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/CodingAgentLauncherBase.cs

@@ -63,12 +63,12 @@ protected CodingAgentLauncherBase(CommandExecutor executor, ILogger logger)
     /// <inheritdoc />
     public abstract SemanticCheckPrompts.AgentToolset Toolset { get; }
 
-    /// <summary>The CLI binary probed with <c>--version</c> to detect availability.</summary>
-    protected abstract string ProbeCommand { get; }
+    /// <inheritdoc />
+    public abstract string CliCommand { get; }
 
     /// <inheritdoc />
     public Task<bool> IsAvailableAsync(CancellationToken cancellationToken = default)
-        => ProbeCommandAsync(ProbeCommand, "--version", cancellationToken);
+        => ProbeCommandAsync(CliCommand, "--version", cancellationToken);
 
     /// <inheritdoc />
     public abstract Task<bool> LaunchAsync(string prompt, string workingDirectory, TimeSpan timeout, CancellationToken cancellationToken = default);
@@ -185,6 +185,15 @@ private async Task<bool> ProbeCommandAsync(string command, string arguments, Can
                 suppressErrorLogging: true,
                 cancellationToken: cancellationToken);
 
+            if (!result.Success)
+            {
+                // The probe ran but reported failure (non-zero exit) rather than being
+                // absent. Surface why at Debug so `-v` distinguishes "not installed" from
+                // "installed but broken" (auth failure, changed --version flag, etc.).
+                Logger.LogDebug("{Command} probe exited {ExitCode}; treating as unavailable. {StdErr}",
+                    command, result.ExitCode, result.StandardError?.Trim());
+            }
+
             return result.Success;
         }
         catch (Exception ex)

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/EvalModelConstants.cs

@@ -0,0 +1,36 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+namespace Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate;
+
+/// <summary>
+/// Single source of truth for the coding-agent model identifiers used during semantic
+/// evaluation. Each model has a per-engine environment-variable override so a user can
+/// move to a newer model without waiting for a CLI release.
+///
+/// GitHub Copilot requires an exact model ID (no aliases); Claude Code accepts an
+/// alias — so the two values are deliberately separate, not a single shared constant.
+/// </summary>
+internal static class EvalModelConstants
+{
+    /// <summary>Environment variable that overrides the GitHub Copilot model.</summary>
+    public const string CopilotModelEnvVar = "A365_EVAL_COPILOT_MODEL";
+
+    /// <summary>Environment variable that overrides the Claude Code model.</summary>
+    public const string ClaudeModelEnvVar = "A365_EVAL_CLAUDE_MODEL";
+
+    private const string DefaultCopilotModel = "claude-haiku-4.5";
+    private const string DefaultClaudeModel = "haiku";
+
+    /// <summary>Copilot model ID; overridable via <see cref="CopilotModelEnvVar"/>.</summary>
+    public static string CopilotModel => FromEnvOrDefault(CopilotModelEnvVar, DefaultCopilotModel);
+
+    /// <summary>Claude model ID/alias; overridable via <see cref="ClaudeModelEnvVar"/>.</summary>
+    public static string ClaudeModel => FromEnvOrDefault(ClaudeModelEnvVar, DefaultClaudeModel);
+
+    private static string FromEnvOrDefault(string envVar, string fallback)
+    {
+        var value = Environment.GetEnvironmentVariable(envVar);
+        return string.IsNullOrWhiteSpace(value) ? fallback : value;
+    }
+}

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/EvaluationPipelineService.cs

@@ -53,11 +53,31 @@ public async Task<int> RunAsync(string serverUrl, string outputDir, string evalE
     {
         try
         {
-            // Fire-and-forget telemetry marker so non-CLI surfaces that drive evaluations
-            // also get attributed. Identity is extracted server-side from the bearer token.
-            // CLI does not pass the evaluated server URL — that is customer content.
-            // Failures are swallowed inside the service.
-            await _toolingService.LogEvaluateUsageAsync(cancellationToken);
+            // Validate the output directory up front so an explicit empty value fails fast
+            // with a targeted error instead of a deep exception from Directory.CreateDirectory
+            // late in the run.
+            if (string.IsNullOrWhiteSpace(outputDir))
+            {
+                _logger.LogError("--output-dir cannot be empty or whitespace.");
+                return 1;
+            }
+
+            // Best-effort usage telemetry, time-bounded so a slow or hung endpoint cannot
+            // stall the user's run before discovery starts. Identity is extracted server-side
+            // from the bearer token; the CLI does not pass the evaluated server URL (customer
+            // content). Failures (swallowed inside the service) and the timeout are ignored.
+            using (var telemetryCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
+            {
+                telemetryCts.CancelAfter(TimeSpan.FromSeconds(5));
+                try
+                {
+                    await _toolingService.LogEvaluateUsageAsync(telemetryCts.Token);
+                }
+                catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)
+                {
+                    // Telemetry timed out — non-fatal; continue with the evaluation.
+                }
+            }
 
             var engine = ParseEvalEngine(evalEngine);
 

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/GitHubCopilotLauncher.cs

@@ -13,10 +13,6 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate;
 /// </summary>
 internal sealed class GitHubCopilotLauncher : CodingAgentLauncherBase
 {
-    // Copilot requires an exact model ID (no aliases like "haiku").
-    // Update this when a newer Haiku version becomes available.
-    private const string CopilotModel = "claude-haiku-4.5";
-
     public GitHubCopilotLauncher(CommandExecutor executor, ILogger<GitHubCopilotLauncher> logger)
         : base(executor, logger)
     {
@@ -28,7 +24,7 @@ public GitHubCopilotLauncher(CommandExecutor executor, ILogger<GitHubCopilotLaun
 
     public override SemanticCheckPrompts.AgentToolset Toolset => new(ReadToolName: "view", EditToolName: "edit");
 
-    protected override string ProbeCommand => "copilot";
+    public override string CliCommand => "copilot";
 
     public override async Task<bool> LaunchAsync(
         string prompt,
@@ -57,7 +53,7 @@ public override async Task<bool> LaunchAsync(
             // sandbox — so view/create/edit stay confined.
             var (fileName, fileArguments) = WrapForPlatform(
                 "copilot",
-                $"-p \"{metaPrompt}\" --model {CopilotModel} --allow-all-tools " +
+                $"-p \"{metaPrompt}\" --model {EvalModelConstants.CopilotModel} --allow-all-tools " +
                 // Restrict visible tools to just read + edit. `create` is specifically
                 // excluded because Copilot's create cannot overwrite existing files and
                 // exposing it leads the model down workaround loops (sibling files,

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/ICodingAgentLauncher.cs

@@ -29,6 +29,9 @@ internal interface ICodingAgentLauncher
     /// </summary>
     SemanticCheckPrompts.AgentToolset Toolset { get; }
 
+    /// <summary>The CLI binary name on PATH (e.g. "copilot", "claude").</summary>
+    string CliCommand { get; }
+
     /// <summary>
     /// Returns true when the agent's CLI is installed and responds on PATH.
     /// </summary>

src/Microsoft.Agents.A365.DevTools.Cli/Services/Evaluate/SchemaDiscoveryService.cs

@@ -16,7 +16,7 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate;
 /// Implements the MCP protocol handshake (initialize, notifications/initialized, tools/list)
 /// over JSON-RPC 2.0 POST requests.
 /// </summary>
-internal sealed class SchemaDiscoveryService : ISchemaDiscoveryService
+internal sealed class SchemaDiscoveryService : ISchemaDiscoveryService, IDisposable
 {
     private const string McpProtocolVersion = "2025-03-26";
     private const string ClientName = "a365-evaluate";
@@ -33,6 +33,14 @@ public SchemaDiscoveryService(ILogger<SchemaDiscoveryService> logger, HttpMessag
         _httpClient = handler != null ? new HttpClient(handler) : HttpClientFactory.CreateAuthenticatedClient();
     }
 
+    /// <summary>
+    /// Disposes the owned <see cref="HttpClient"/>. The service is registered as a
+    /// singleton (Program.cs), so in practice this runs at process shutdown;
+    /// implementing it keeps the IDisposable contract correct if the registration
+    /// lifetime ever changes to transient or scoped.
+    /// </summary>
+    public void Dispose() => _httpClient.Dispose();
+
     /// <inheritdoc />
     public async Task<List<ToolSchema>> DiscoverToolsAsync(string serverUrl, string? authToken = null, CancellationToken cancellationToken = default)
     {