Adding validate cli command for local validation#456
Conversation
Co-authored-by: Copilot <copilot@github.com>
…ntCheck Co-authored-by: Copilot <copilot@github.com>
Dependency ReviewThe following issues were found:
License Issuessrc/Tests/Microsoft.Agents.A365.DevTools.Validation.Tests/Microsoft.Agents.A365.DevTools.Validation.Tests.csproj
OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Pull request overview
Adds a new a365 validate CLI command that performs local, end-to-end validation of an Agent 365 project and emits a structured JSON report (a365.validate.json). This introduces a new Microsoft.Agents.A365.DevTools.Validation subproject to host shared validation contracts/models and bot callback helpers, while the CLI command orchestrates tiered requirement checks and prints a console summary.
Changes:
- Introduce
a365 validatecommand, register it in the CLI, and persist a structured validation report. - Add new Validation subproject with report models, metadata types, validation contracts, and an
HttpListener-based bot callback receiver. - Add/expand requirement checks (manifest, build, boot, telemetry, blueprint registration) and comprehensive xUnit coverage for the new behavior.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the new a365 validate command and Validation subproject under [Unreleased]. |
| src/Microsoft.Agents.A365.DevTools.Cli.sln | Adds the new Validation and Validation.Tests projects to the solution. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/ValidateCommand.cs | Implements the validate command orchestration, console summary, and report writing. |
| src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs | Adds the validate command name constant. |
| src/Microsoft.Agents.A365.DevTools.Cli/Microsoft.Agents.A365.DevTools.Cli.csproj | References the new Validation project. |
| src/Microsoft.Agents.A365.DevTools.Cli/Program.cs | Registers the new validate command in the root command. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/CommandExecutor.cs | Changes missing-executable behavior to return a failed CommandResult instead of throwing. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementCheckResult.cs | Adds Metadata for validate-specific structured reporting. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/BlueprintRegistrationRequirementCheck.cs | Adds/updates blueprint registration validation, including metadata population and tooling-manifest-derived scopes. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/LocalRuntimeRequirementCheck.cs | Adds/updates local runtime boot probing and metadata/log capture. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ProjectBuildRequirementCheck.cs | Adds/updates build validation (dotnet/npm/python) and dependency install support (uv/pip/npm). |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/TelemetryRequirementCheck.cs | Adds telemetry log parsing to detect GenAI operation spans and parent-link gaps. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ToolingManifestRequirementCheck.cs | Adds tooling manifest JSON/schema validation (when present). |
| src/Microsoft.Agents.A365.DevTools.Validation/HttpListenerBotCallbackReceiver.cs | Implements local callback receiver for Bot Framework serviceUrl responses. |
| src/Microsoft.Agents.A365.DevTools.Validation/IBotCallbackReceiver.cs | Defines the callback receiver contract and BotCallbackResponse model. |
| src/Microsoft.Agents.A365.DevTools.Validation/Microsoft.Agents.A365.DevTools.Validation.csproj | Creates the new Validation project. |
| src/Microsoft.Agents.A365.DevTools.Validation/RequirementCheckMetadata.cs | Defines typed metadata attached to RequirementCheckResult.Metadata for report mapping. |
| src/Microsoft.Agents.A365.DevTools.Validation/ValidateReport.cs | Defines the structured a365.validate.json report schema (tiers + summary + agent info). |
| src/Microsoft.Agents.A365.DevTools.Validation/ValidationContracts.cs | Adds reusable validation contracts (ValidationOutcome, ValidationIssue, CliValidationCoordinator, etc.). |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/ValidateCommandTests.cs | Adds CLI invocation tests covering exit codes and report output. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Microsoft.Agents.A365.DevTools.Cli.Tests.csproj | Adds a reference to the Validation project for CLI tests. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/CommandExecutorTests.cs | Updates tests for new “missing command returns failed result” behavior. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/BlueprintRegistrationRequirementCheckTests.cs | Adds coverage for blueprint registration tier (existence, warnings, permissions metadata). |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/LocalRuntimeRequirementCheckTests.cs | Adds boot-tier tests (process start, health probe, port resolution, Python entrypoint parsing). |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ProjectBuildRequirementCheckTests.cs | Adds build-tier tests for dotnet/npm/python build and dependency detection (uv/pip). |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/TelemetryRequirementCheckTests.cs | Adds telemetry parsing tests for multiple console exporter formats and hierarchy checks. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Requirements/ToolingManifestRequirementCheckTests.cs | Adds manifest-schema validation tests (invalid JSON, duplicates, missing url, etc.). |
| src/Tests/Microsoft.Agents.A365.DevTools.Validation.Tests/Microsoft.Agents.A365.DevTools.Validation.Tests.csproj | Adds a new test project for the Validation subproject. |
| src/Tests/Microsoft.Agents.A365.DevTools.Validation.Tests/Validation/HttpListenerBotCallbackReceiverTests.cs | Adds tests for substantive-message classification logic. |
| src/Tests/Microsoft.Agents.A365.DevTools.Validation.Tests/Validation/ValidationContractsTests.cs | Adds tests for ValidationOutcome success/failure helper contracts. |
| // Status markers — use characters supported across Windows/macOS/Linux terminals | ||
| private const string PassMark = "\u221A"; // √ (square root, same as Windows renders for checkmark) | ||
| private const string FailMark = "X"; | ||
| private const string WarnMark = "!"; | ||
| private const string SkipMark = "-"; |
| /// <summary> | ||
| /// After core registration checks pass, verify inheritable permissions | ||
| /// by comparing the static baseline + tooling manifest scopes against what is actually in Entra. | ||
| /// Missing or mismatched permissions produce a warning (not a failure). | ||
| /// </summary> |
| var manifestPath = Path.Combine( | ||
| config.DeploymentProjectPath ?? Directory.GetCurrentDirectory(), | ||
| McpConstants.ToolingManifestFileName); | ||
|
|
| logger.LogDebug("Analyzing agent console log at {LogPath}", _agentConsoleLogPath); | ||
|
|
||
| string[] logLines; | ||
| try | ||
| { | ||
| logLines = File.ReadAllLines(_agentConsoleLogPath); | ||
| } | ||
| catch (Exception ex) | ||
| { |
| public static bool IsSubstantiveMessage(BotCallbackResponse response) | ||
| { | ||
| if (string.Equals(response.Type, "typing", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return response.Type == "message" && !string.IsNullOrWhiteSpace(response.Text); | ||
| } |
| var projectPath = ResolveProjectPath(config); | ||
| var manifestPath = Path.Combine(projectPath, McpConstants.ToolingManifestFileName); | ||
|
|
||
| if (!File.Exists(manifestPath)) | ||
| { | ||
| return RequirementCheckResult.Success("ToolingManifest.json not present, skipping"); | ||
| } |
| return platform switch | ||
| { | ||
| ProjectPlatform.DotNet => ("dotnet", | ||
| $"build --no-restore /p:TreatWarningsAsErrors=true -fl -flp:logfile={buildLogFile};verbosity=normal"), |
There was a problem hiding this comment.
the dotnet build arguments interpolate the MSBuild file-logger path unquoted:
build --no-restore /p:TreatWarningsAsErrors=true -fl -flp:logfile={buildLogFile};verbosity=normal.
CommandExecutor assigns this verbatim to a single ProcessStartInfo.Arguments string. buildLogFile
resolves under %LocalAppData%, which commonly contains a space (e.g. C:\Users\First Last\AppData\Local...).
The space splits the token, so dotnet build receives a stray argument (the path tail) and is likely to
fail (MSB1009 / "specify which project") - i.e. the BUILD CHECK can fail outright on any machine whose
profile path contains a space, not merely lose the diagnostic log.
Fix: quote the switch - -fl "-flp:logfile={buildLogFile};verbosity=normal"
| { | ||
| try | ||
| { | ||
| var scopesByAudience = ManifestHelper.GetScopesByAudienceAsync(manifestPath).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Sync-over-async: `BuildExpectedPermissions` (declared line 371, called from the async CheckImplementationAsync
at line 223) calls `ManifestHelper.GetScopesByAudienceAsync(manifestPath).GetAwaiter().GetResult()` at line 400
(confirmed: GetScopesByAudienceAsync is `public static async Task<...>`). The CLI runs on the thread pool with
no SynchronizationContext, so this does NOT deadlock - but it blocks a pool thread for the file I/O and is an
avoidable smell. The method is sync only because it lacks a token. Fix: make it
`BuildExpectedPermissionsAsync(..., CancellationToken)`, `await` the call, and await from the caller. Resolves
CR-10 at the same time.
| catch (Exception ex) | ||
| { | ||
| logger.LogDebug(ex, "Failed to read tooling manifest at {Path}, skipping manifest scopes", manifestPath); | ||
| } |
There was a problem hiding this comment.
The bare `catch (Exception ex)` at line 416 (logs "skipping manifest scopes" at 418) swallows
OperationCanceledException - Ctrl+C during the manifest read is silently treated as "manifest missing" and
validation continues. Notably this is the ONLY bare catch in the file: lines 93, 117, 146, 231, 255 all use
`catch (Exception ex) when (ex is not OperationCanceledException)`. So this is both a correctness issue and an
inconsistency with the file's own established pattern. Fix: add the same `when (ex is not
OperationCanceledException)` filter (or fold into the CR-01 async refactor).
| : response[..maxLength] + "..."; | ||
| } | ||
|
|
||
| private static bool IsErrorResponse(string? responseText) |
There was a problem hiding this comment.
`IsErrorResponse` (declared line 1043, used at line 726 to set turnOk=false in non-playground mode) fails a
conversation turn when the agent text contains any of "error/exception/failed/not found/unauthorized/forbidden/
timeout/...". This produces false failures on legitimate replies, e.g. "the file was not found in SharePoint"
or "let me retry the task that failed". Recommend dropping the keyword heuristic and gating only on
`agentResponded` (a real Bot Framework response), or narrowing to an unambiguous structural signal (a
stack-trace pattern, or an explicit error-type activity).
| { | ||
| _platformDetector = platformDetector ?? throw new ArgumentNullException(nameof(platformDetector)); | ||
| _processService = processService ?? throw new ArgumentNullException(nameof(processService)); | ||
| _httpClient = httpClient ?? new HttpClient(); |
There was a problem hiding this comment.
`_httpClient = httpClient ?? new HttpClient();` (LocalRuntimeRequirementCheck line 58; same pattern in
ConversationRequirementCheck line 197). Confirmed neither class implements IDisposable (declarations at
LocalRuntimeRequirementCheck:18 and ConversationRequirementCheck:21, both `: RequirementCheck` only). The
internally-created HttpClient is never disposed. Mitigating factor: `a365` is a one-shot CLI, so these are
reclaimed at process exit (no socket exhaustion in practice) - hence medium, not high. Still a CLAUDE.md
disposal-rule violation: track ownership (`_ownsHttpClient = httpClient is null`), implement IDisposable,
dispose only when owned, and dispose the checks in ValidateCommand after the run.
|
|
||
| if (warnings.Count > 0) | ||
| { | ||
| var result = RequirementCheckResult.Failure( |
There was a problem hiding this comment.
Doc/behavior mismatch: the XML doc at line 197 says "Missing or mismatched permissions produce a warning (not
a failure)", but at line 348 `if (warnings.Count > 0)` the code returns `RequirementCheckResult.Failure(
"Blueprint registered but permissions/consent gaps detected", ...)` (line 350). So a permission/consent gap
makes `a365 validate` exit non-zero, contradicting the doc. Align one to the other - use Warning, or correct
the doc to say "failure".
| @@ -0,0 +1,343 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
File organization. Verified type counts: ValidateReport.cs has 14 public types,
RequirementCheckMetadata.cs has 4, ValidationContracts.cs has 6 - all sitting flat in the project root. Two
issues: (a) the CLI project organizes data types under a Models/ folder, but this new subproject has none;
(b) the repo convention is one public type per file. The 24 types fall into clean buckets:
- Report DTOs (14, ValidateReport.cs): ValidateReport, AgentInfo, ValidationTiers, TierResult,
StructuralTierResult, StructuralCheck, BuildTierResult, BootTierResult, ConversationTierResult,
TelemetryTierResult, BlueprintTierResult, BlueprintResourceResult, ConversationTurnResult, SummaryResult.
- Check-metadata DTOs (4, RequirementCheckMetadata.cs): RequirementCheckMetadata,
MacMetricComparisonMetadata, ConversationTurnMetadata, BlueprintResourcePermission.
- Contracts (6, ValidationContracts.cs): ValidationSeverity, ValidationIssue, ValidationOutcome,
ValidationLoadResult<T>, CliValidationCoordinator<T>, IValidator<T>.
Recommendation: move the report + check-metadata DTOs (groups 1 and 2 - pure [JsonPropertyName]-decorated
data, no behavior) into a Models/ folder, one type per file, mirroring the CLI project. For group 3, resolve
per CR-14 first: if CliValidationCoordinator<T> and IValidator<T> are removed (recommended, they are unused),
the remaining ValidationSeverity/ValidationIssue/ValidationOutcome/ValidationLoadResult are also models and
can go under Models/ too; if kept, place them in a Contracts/ (or Abstractions/) folder. Optionally align the
namespace to ...DevTools.Validation.Models (larger mechanical change; C# does not require folder==namespace,
so this is a separate decision). Note: this mirrors the CLI project's convention rather than a formally
documented cross-subproject rule, so it is a recommendation, not a hard requirement.
| /// Keywords that identify a log line as telemetry-related (console exporter output). | ||
| /// A line must contain at least one of these to be considered relevant. | ||
| /// </summary> | ||
| internal static readonly string[] TelemetryContextKeywords = new[] |
There was a problem hiding this comment.
Dead constants (both verified to have no references beyond their declaration). `MaxTelemetryLines` (line 22)
is documented as an analysis cap but is never applied - the whole log is read via File.ReadAllLines.
`TelemetryContextKeywords` (line 28) is never referenced. Either apply MaxTelemetryLines (e.g.
`.TakeLast(MaxTelemetryLines)`) and wire the keyword pre-filter, or remove both.
sellakumaran
left a comment
There was a problem hiding this comment.
Please see my comments
Address review comment: the command summary used a Unicode square root character that may not render consistently across terminals and log files. Replaced with descriptive plain-text markers (PASS/FAIL/WARN/SKIP). Also added IDisposable tracking for requirement checks.
…tionRequirementCheck - Fix XML doc/behavior mismatch: updated comment to say 'failure' not 'warning' for missing/mismatched permissions (matches actual behavior at line 350) - Convert BuildExpectedPermissions to async (BuildExpectedPermissionsAsync) to eliminate sync-over-async .GetAwaiter().GetResult() call - Add OperationCanceledException filter to catch block for manifest read, matching the pattern used elsewhere in the file - Fix DeploymentProjectPath empty/whitespace handling: treat empty/whitespace as 'use CWD' instead of relying on null-coalescing alone
…rofile paths The -flp:logfile= argument was unquoted, causing dotnet build to fail with MSB1009 when the user's profile path contains spaces (e.g. 'First Last'). Wrapped the switch in quotes to handle this correctly.
…duce false positives Replaced broad keyword matching (error/exception/failed/not found/etc.) with structural signals: stack traces, exception type headers, HTTP 4xx/5xx codes, and 'internal server error'. This prevents false failures on legitimate agent replies like 'the file was not found in SharePoint'. Also implemented IDisposable pattern to properly dispose internally-created HttpClient instances. Updated test to use 'internal server error' text that matches the new structural detection.
…acking Added _ownsHttpClient flag and IDisposable implementation to properly dispose internally-created HttpClient instances, following CLAUDE.md disposal rules. When an HttpClient is injected from outside, it is not disposed.
…orce MaxTelemetryLines cap Removed unused TelemetryContextKeywords array (never referenced beyond declaration). Applied MaxTelemetryLines cap by analyzing only the last 200 lines of the agent console log, preventing unbounded memory usage for large log files.
…SubstantiveMessage The 'typing' check already used OrdinalIgnoreCase but the 'message' check used case-sensitive == comparison. This inconsistency could cause legitimate message activities to be ignored if the type casing differs.
…behavior Added comment explaining that returning Success for a missing ToolingManifest.json is intentional since agents that do not use MCP tool servers will not have one.
…ype per file Moved all DTO types from flat root files (ValidateReport.cs, RequirementCheckMetadata.cs, ValidationContracts.cs) into individual files under Models/, following the CLI project's convention of one public type per file. Removed unused CliValidationCoordinator<T> and IValidator<T> types. Namespace remains unchanged (no breaking change for consumers).
…ad of MessagingEndpoint The test was still using config.MessagingEndpoint which was replaced by file-based port resolution (launchSettings.json / .env). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… keys Add optional closing quote after key name to match formats like parent_id from Python console exporter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| private readonly IProcessService _processService; | ||
| private readonly string _tempDir; | ||
|
|
||
| public LocalRuntimeRequirementCheckTests() | ||
| { | ||
| _logger = Substitute.For<ILogger>(); | ||
| _platformDetector = new PlatformDetector(Substitute.For<ILogger<PlatformDetector>>()); | ||
| _processService = Substitute.For<IProcessService>(); | ||
| _tempDir = Path.Combine(Path.GetTempPath(), $"a365-runtime-test-{Guid.NewGuid():N}"); | ||
| Directory.CreateDirectory(_tempDir); | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| if (Directory.Exists(_tempDir)) | ||
| { | ||
| Directory.Delete(_tempDir, recursive: true); | ||
| } | ||
| } | ||
|
|
||
| private LocalRuntimeRequirementCheck CreateCheck(HttpMessageHandler? handler = null) | ||
| { | ||
| var httpClient = handler is not null ? new HttpClient(handler) : new HttpClient(); | ||
| return new LocalRuntimeRequirementCheck(_platformDetector, _processService, httpClient); | ||
| } |
| var allLines = File.ReadAllLines(_agentConsoleLogPath); | ||
| logLines = allLines.Length > MaxTelemetryLines | ||
| ? allLines[^MaxTelemetryLines..] | ||
| : allLines; |
| private async Task<(RequirementCheckResult? FailureResult, CommandResult? Output)> InstallDependenciesAsync( | ||
| ProjectPlatform platform, | ||
| string projectPath, | ||
| ILogger logger, | ||
| CancellationToken cancellationToken) |
| var installResult = await _commandExecutor.ExecuteAsync( | ||
| "pip", "install uv", | ||
| captureOutput: true, | ||
| suppressErrorLogging: true, | ||
| cancellationToken: cancellationToken); |
| if (!File.Exists(manifestPath)) | ||
| { | ||
| // ToolingManifest.json is optional — agents that do not use MCP tool servers | ||
| // will not have one. Returning Success here is intentional. | ||
| return RequirementCheckResult.Success("ToolingManifest.json not present, skipping"); | ||
| } |
- Dispose check instances in LocalRuntimeRequirementCheckTests via tracked _disposables list disposed in test class Dispose() - Stream telemetry log tail with File.ReadLines().TakeLast() instead of File.ReadAllLines() to avoid loading entire log into memory - Use python -m pip instead of bare pip for uv install to target the active Python interpreter - Update CHANGELOG to accurately describe validate command behavior (build step installs dependencies) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ProjectPlatform.DotNet => ("dotnet", | ||
| $"build --no-restore /p:TreatWarningsAsErrors=true -fl \"-flp:logfile={buildLogFile};verbosity=normal\""), | ||
| ProjectPlatform.NodeJs => ("npm", "run build"), |
| stopwatch.Stop(); | ||
| logger.LogDebug("Health endpoint returned {StatusCode}", (int)response.StatusCode); | ||
| return new RequirementCheckResult | ||
| { | ||
| Passed = true, | ||
| Details = $"{platform} app running on port {port}, health endpoint returned HTTP {(int)response.StatusCode}", | ||
| Metadata = new RequirementCheckMetadata | ||
| { | ||
| Port = port, | ||
| BootMs = stopwatch.ElapsedMilliseconds, | ||
| Platform = platform.ToString() | ||
| } | ||
| }; |
| // The lines before the first traceId won't be in any block | ||
| // instrumentationScope needs to be AFTER traceId or we need to handle this |
- Add dotnet restore step before --no-restore build so fresh clones don't fail with missing assets file - Await HandleRequestAsync in ListenLoopAsync instead of fire-and-forget for deterministic error handling - Write boot log and include BootLogFile in metadata on success path (not just failure) for diagnosing flaky startups - Fix misleading test comment about SplitIntoSpanBlocks behavior - Update ProjectBuildRequirementCheckTests to mock dotnet restore separately from build Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| logger.LogDebug("Installing Python dependencies: {Command} {Arguments}", command, arguments); | ||
|
|
||
| var result = await _commandExecutor.ExecuteAsync( | ||
| command, | ||
| arguments!, | ||
| workingDirectory: projectPath, | ||
| captureOutput: true, | ||
| suppressErrorLogging: true, | ||
| cancellationToken: cancellationToken); |
| ProjectPlatform.DotNet => ("dotnet", | ||
| $"build --no-restore /p:TreatWarningsAsErrors=true -fl \"-flp:logfile={buildLogFile};verbosity=normal\""), |
| public BearerTokenRequirementCheck(PlatformDetector platformDetector) | ||
| { | ||
| _platformDetector = platformDetector; | ||
| } |
…rocess Single quotes in ProcessStartInfo.Arguments are passed literally to /bin/sh on Linux, causing 'sleep 60' to be interpreted as a malformed command (exit code 2). Use escaped double quotes instead, which .NET passes correctly to the shell via execve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
f3a1491 to
21c41bb
Compare
Add a365 validate command
Summary
Adds a new a365 validate command that performs end-to-end validation of an Agent 365 project. The command runs a tiered set of checks and produces a structured JSON report (a365.validate.json).
Validation Tiers
Structural - ToolingManifestRequirementCheck - Validates ToolingManifest.json exists and is well-formed
Build - ProjectBuildRequirementCheck - Builds the project (.NET/Node/Python), captures exit code and logs
Boot - LocalRuntimeRequirementCheck - Starts the agent locally, health-probes /api/messages
Conversation - ConversationRequirementCheck - Sends multi-turn Bot Framework activities, verifies agent responses via serviceUrl callback
Telemetry - TelemetryRequirementCheck - Parses agent console logs for OTel span output
Blueprint - BlueprintRegistrationRequirementCheck - Validates Entra app, service principal, registration, and permission