From e5daffa5de712aa815d0474ed42532540541270f Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Thu, 30 Apr 2026 14:39:16 +1000 Subject: [PATCH 1/6] Add JSON output for skill-validator check Fixes #600 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/skill-validator/src/Check/CheckCommand.cs | 682 +++++++++++++----- eng/skill-validator/src/Check/Models.cs | 136 +++- .../src/Check/PluginProfiler.cs | 12 +- eng/skill-validator/src/README.md | 6 +- .../src/SkillValidatorJsonContext.cs | 5 +- .../tests/Check/CheckCommandTests.cs | 144 ++++ 6 files changed, 803 insertions(+), 182 deletions(-) diff --git a/eng/skill-validator/src/Check/CheckCommand.cs b/eng/skill-validator/src/Check/CheckCommand.cs index cae5d5f547..d3266430ea 100644 --- a/eng/skill-validator/src/Check/CheckCommand.cs +++ b/eng/skill-validator/src/Check/CheckCommand.cs @@ -15,6 +15,7 @@ public static Command Create() var knownDomainsOpt = new Option("--known-domains") { Description = "Path to known-domains.txt for reference scanning" }; var verboseOpt = new Option("--verbose") { Description = "Show detailed output" }; var allowRepoTraversalOpt = new Option("--allow-repo-traversal") { Description = "Allow parent-directory traversals in file references" }; + var jsonOutputOpt = new Option("--json") { Description = "Write machine-readable JSON report to stdout", DefaultValueFactory = (_) => false }; var command = new Command("check", "Run static analysis checks on skills, plugins, and agents (no LLM required). Use --plugin to check an entire plugin directory (recommended).") { @@ -25,34 +26,16 @@ public static Command Create() knownDomainsOpt, verboseOpt, allowRepoTraversalOpt, + jsonOutputOpt, }; command.SetAction(async (parseResult, _) => { - var pluginPaths = parseResult.GetValue(pluginOpt) ?? []; - var skillPaths = parseResult.GetValue(skillsOpt) ?? []; - var agentPaths = parseResult.GetValue(agentsOpt) ?? []; - - bool hasPlugin = pluginPaths.Length > 0; - bool hasSkills = skillPaths.Length > 0; - bool hasAgents = agentPaths.Length > 0; - - if (!hasPlugin && !hasSkills && !hasAgents) - { - Console.Error.WriteLine("Specify one of --plugin, --skills, or --agents. Use --plugin to check an entire plugin directory."); - return 1; - } - if (hasPlugin && (hasSkills || hasAgents)) - { - Console.Error.WriteLine("--plugin cannot be combined with --skills or --agents. Use --plugin alone to check an entire plugin directory."); - return 1; - } - var config = new CheckConfig { - PluginPaths = pluginPaths, - SkillPaths = skillPaths, - AgentPaths = agentPaths, + PluginPaths = parseResult.GetValue(pluginOpt) ?? [], + SkillPaths = parseResult.GetValue(skillsOpt) ?? [], + AgentPaths = parseResult.GetValue(agentsOpt) ?? [], AllowedExternalDepsFile = parseResult.GetValue(allowedExternalDepsOpt), KnownDomainsFile = parseResult.GetValue(knownDomainsOpt), Verbose = parseResult.GetValue(verboseOpt), @@ -60,14 +43,9 @@ public static Command Create() { AllowRepoTraversal = parseResult.GetValue(allowRepoTraversalOpt), }, + OutputMode = parseResult.GetValue(jsonOutputOpt) ? CheckOutputMode.Json : CheckOutputMode.Console, }; - if (config.CheckOptions.AllowRepoTraversal && pluginPaths.Length > 0) - { - Console.Error.WriteLine("--allow-repo-traversal cannot be used with --plugin. Plugins must be portable — use --skills or --agents instead."); - return 1; - } - return await Run(config); }); @@ -76,26 +54,74 @@ public static Command Create() public static async Task Run(CheckConfig config) { - if (config.PluginPaths.Count > 0) - return await RunPluginCheck(config); + var report = await BuildReport(config); + RenderReport(report); + return report.ExitCode; + } - if (config.SkillPaths.Count > 0 && config.AgentPaths.Count > 0) - return await RunSkillsAndAgentsCheck(config); + private static async Task BuildReport(CheckConfig config) + { + var builder = new CheckReportBuilder(config, DetermineScope(config)); - if (config.SkillPaths.Count > 0) - return await RunSkillsCheck(config); + if (!ValidateConfig(config, builder)) + return builder.Build(1); + + int exitCode = config.PluginPaths.Count > 0 + ? await RunPluginCheck(config, builder) + : config.SkillPaths.Count > 0 && config.AgentPaths.Count > 0 + ? await RunSkillsAndAgentsCheck(config, builder) + : config.SkillPaths.Count > 0 + ? await RunSkillsCheck(config, builder) + : await RunAgentsCheck(config, builder); + + return builder.Build(exitCode); + } + + private static bool ValidateConfig(CheckConfig config, CheckReportBuilder builder) + { + bool hasPlugin = config.PluginPaths.Count > 0; + bool hasSkills = config.SkillPaths.Count > 0; + bool hasAgents = config.AgentPaths.Count > 0; + + if (!hasPlugin && !hasSkills && !hasAgents) + { + builder.AddPlainError("Specify one of --plugin, --skills, or --agents. Use --plugin to check an entire plugin directory."); + return false; + } + if (hasPlugin && (hasSkills || hasAgents)) + { + builder.AddPlainError("--plugin cannot be combined with --skills or --agents. Use --plugin alone to check an entire plugin directory."); + return false; + } + + if (config.CheckOptions.AllowRepoTraversal && hasPlugin) + { + builder.AddPlainError("--allow-repo-traversal cannot be used with --plugin. Plugins must be portable — use --skills or --agents instead."); + return false; + } + + return true; + } + + private static string DetermineScope(CheckConfig config) + { + if (config.PluginPaths.Count > 0) + return "plugin"; + if (config.SkillPaths.Count > 0 && config.AgentPaths.Count > 0) + return "skillsAndAgents"; + if (config.SkillPaths.Count > 0) + return "skills"; if (config.AgentPaths.Count > 0) - return await RunAgentsCheck(config); + return "agents"; - throw new ArgumentException("No paths specified to check."); + return "invalid"; } - private static async Task RunPluginCheck(CheckConfig config) + private static async Task RunPluginCheck(CheckConfig config, CheckReportBuilder builder) { var allPlugins = new List(); - // Track skills per plugin for aggregate checks - var pluginSkills = new Dictionary>(); + var pluginSkills = new Dictionary>(StringComparer.Ordinal); var allSkillsList = new List(); var agentDirs = new List(); @@ -103,12 +129,11 @@ private static async Task RunPluginCheck(CheckConfig config) { var fullPath = Path.GetFullPath(pluginDir); var pluginName = Path.GetFileName(fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); - - // Parse plugin.json var pluginJsonPath = Path.Combine(fullPath, "plugin.json"); + if (!File.Exists(pluginJsonPath)) { - Console.Error.WriteLine($"{Ansi.Red}❌ No plugin.json found in '{pluginDir}'{Ansi.Reset}"); + builder.AddGeneralError($"No plugin.json found in '{pluginDir}'"); return 1; } @@ -119,167 +144,143 @@ private static async Task RunPluginCheck(CheckConfig config) } catch (JsonException ex) { - Console.Error.WriteLine($"{Ansi.Red}❌ Malformed plugin.json in '{pluginDir}': {ex.Message}{Ansi.Reset}"); + builder.AddGeneralError($"Malformed plugin.json in '{pluginDir}': {ex.Message}"); return 1; } if (plugin is null) { - Console.Error.WriteLine($"{Ansi.Red}❌ Failed to parse plugin.json in '{pluginDir}'{Ansi.Reset}"); + builder.AddGeneralError($"Failed to parse plugin.json in '{pluginDir}'"); return 1; } allPlugins.Add(plugin); - // Resolve skills path from plugin.json and discover skills per plugin - var skillsDirs = new List(); - if (plugin is not null) + foreach (var skillPath in plugin.SkillPaths) { - foreach (var sp in plugin.SkillPaths) + if (!PluginDiscovery.TryGetSafeSubdirectory(fullPath, skillPath, out var dir, out _) || !Directory.Exists(dir)) + continue; + + var skills = await SkillDiscovery.DiscoverSkills(dir!); + if (!pluginSkills.TryGetValue(pluginName, out var pluginSkillList)) { - if (PluginDiscovery.TryGetSafeSubdirectory(fullPath, sp, out var dir, out _) - && Directory.Exists(dir)) - skillsDirs.Add(dir!); + pluginSkillList = []; + pluginSkills[pluginName] = pluginSkillList; } - } - foreach (var dir in skillsDirs) - { - var skills = await SkillDiscovery.DiscoverSkills(dir); - pluginSkills[pluginName] = new List(skills); + pluginSkillList.AddRange(skills); allSkillsList.AddRange(skills); } - // Collect agent directories var agents = await AgentDiscovery.DiscoverAgentsInPlugin(fullPath); foreach (var dir in agents.Select(a => Path.GetDirectoryName(a.Path)).Where(d => d is not null).Distinct()) agentDirs.Add(dir!); } - // Validate plugins first bool hasPluginErrors = false; foreach (var plugin in allPlugins) { var result = PluginProfiler.ValidatePlugin(plugin); - foreach (var warning in result.Warnings) - Console.WriteLine($"{Ansi.Yellow}⚠ [plugin:{result.Name}] {warning}{Ansi.Reset}"); + builder.Plugins.Add(result); + foreach (var error in result.Errors) - { - Console.Error.WriteLine($"{Ansi.Red}❌ [plugin:{result.Name}] {error}{Ansi.Reset}"); hasPluginErrors = true; - } } - Console.WriteLine($"Validated {allPlugins.Count} plugin(s)"); + if (hasPluginErrors) { - Console.Error.WriteLine("{Ansi.Red}Plugin spec conformance failures — fix the errors above.{Ansi.Reset}"); return 1; } - // Validate discovered skills (profiles, etc.) int skillResult = 0; if (allSkillsList.Count > 0) { - Console.WriteLine($"Found {allSkillsList.Count} skill(s)"); - if (ValidateSkillProfiles(allSkillsList, config.Verbose, config.CheckOptions)) + if (ValidateSkillProfiles(builder, allSkillsList, config.Verbose, config.CheckOptions)) skillResult = 1; - // Check for duplicate skill names across all skills - if (CheckDuplicateSkillNames(allSkillsList)) + if (CheckDuplicateSkillNames(builder, builder.Skills)) skillResult = 1; } - // Validate agents - var (allAgents, agentResult) = await RunAgentsCheckCore(agentDirs.Distinct().ToList()); + var (allAgents, discoveredAgents, agentResult) = await RunAgentsCheckCore(builder, agentDirs.Distinct().ToList()); if (allSkillsList.Count == 0 && allAgents.Count == 0) { - Console.Error.WriteLine("No skills or agents found in the specified plugin(s)."); + builder.AddPlainError("No skills or agents found in the specified plugin(s)."); return 1; } - // Aggregate description limits apply per plugin foreach (var (pluginName, skills) in pluginSkills) { int totalChars = skills.Sum(s => s.Description.Length); - if (totalChars > SkillProfiler.MaxAggregateDescriptionLength) - { - Console.Error.WriteLine( - $"{Ansi.Red}❌ Plugin '{pluginName}' aggregate description size is {totalChars:N0} characters — " + - $"maximum is {SkillProfiler.MaxAggregateDescriptionLength:N0}.{Ansi.Reset}"); - return 1; - } + if (totalChars <= SkillProfiler.MaxAggregateDescriptionLength) + continue; + + var message = $"Plugin '{pluginName}' aggregate description size is {totalChars:N0} characters — maximum is {SkillProfiler.MaxAggregateDescriptionLength:N0}."; + if (builder.Plugins.FirstOrDefault(p => string.Equals(p.Name, pluginName, StringComparison.Ordinal)) is { } pluginResult) + pluginResult.Errors.Add(message); + return 1; } - // Check for external dependencies (plugin-level check includes all three) - CheckExternalDeps(config.AllowedExternalDepsFile, allSkillsList, allAgents, allPlugins); + CheckExternalDeps(builder, config.AllowedExternalDepsFile, allSkillsList, discoveredAgents, allPlugins); - // Run reference scanner if known-domains file is provided - if (RunReferenceScanner(config.KnownDomainsFile, config.PluginPaths)) + if (RunReferenceScanner(builder, config.KnownDomainsFile, config.PluginPaths)) return 1; if (skillResult != 0 || agentResult != 0) return 1; - - Console.WriteLine($"{Ansi.Green}✅ All checks passed ({allSkillsList.Count} skill(s), {allAgents.Count} agent(s), {allPlugins.Count} plugin(s)){Ansi.Reset}"); return 0; } - private static async Task RunSkillsCheck(CheckConfig config) + private static async Task RunSkillsCheck(CheckConfig config, CheckReportBuilder builder) { - var (skills, result) = await RunSkillsCheckCore(config.SkillPaths, config.Verbose, config.CheckOptions); + var (skills, result) = await RunSkillsCheckCore(builder, config.SkillPaths, config.Verbose, config.CheckOptions); if (skills.Count == 0) - return 1; // error already printed + return 1; if (result != 0) return result; - // Run reference scanner on skill directories - if (RunReferenceScanner(config.KnownDomainsFile, config.SkillPaths)) + if (RunReferenceScanner(builder, config.KnownDomainsFile, config.SkillPaths)) return 1; - Console.WriteLine($"{Ansi.Green}✅ All checks passed ({skills.Count} skill(s)){Ansi.Reset}"); return 0; } - private static async Task RunAgentsCheck(CheckConfig config) + private static async Task RunAgentsCheck(CheckConfig config, CheckReportBuilder builder) { - var (agents, result) = await RunAgentsCheckCore(config.AgentPaths); + var (agents, _, result) = await RunAgentsCheckCore(builder, config.AgentPaths); if (agents.Count == 0) - return 1; // error already printed + return 1; if (result != 0) return result; - // Run reference scanner on agent directories - if (RunReferenceScanner(config.KnownDomainsFile, config.AgentPaths)) + if (RunReferenceScanner(builder, config.KnownDomainsFile, config.AgentPaths)) return 1; - Console.WriteLine($"{Ansi.Green}✅ All checks passed ({agents.Count} agent(s)){Ansi.Reset}"); return 0; } - private static async Task RunSkillsAndAgentsCheck(CheckConfig config) + private static async Task RunSkillsAndAgentsCheck(CheckConfig config, CheckReportBuilder builder) { - var (skills, skillResult) = await RunSkillsCheckCore(config.SkillPaths, config.Verbose, config.CheckOptions); - var (agents, agentResult) = await RunAgentsCheckCore(config.AgentPaths); + var (skills, skillResult) = await RunSkillsCheckCore(builder, config.SkillPaths, config.Verbose, config.CheckOptions); + var (agents, _, agentResult) = await RunAgentsCheckCore(builder, config.AgentPaths); if (skills.Count == 0 && agents.Count == 0) - return 1; // errors already printed + return 1; if (skillResult != 0 || agentResult != 0) return 1; - // Run reference scanner on all directories var allDirs = config.SkillPaths.Concat(config.AgentPaths).ToList(); - if (RunReferenceScanner(config.KnownDomainsFile, allDirs)) + if (RunReferenceScanner(builder, config.KnownDomainsFile, allDirs)) return 1; - Console.WriteLine($"{Ansi.Green}✅ All checks passed ({skills.Count} skill(s), {agents.Count} agent(s)){Ansi.Reset}"); return 0; } - private static async Task<(IReadOnlyList Skills, int Result)> RunSkillsCheckCore(IReadOnlyList skillPaths, bool verbose, CheckOptions? checkOptions = null) + private static async Task<(IReadOnlyList Skills, int Result)> RunSkillsCheckCore(CheckReportBuilder builder, IReadOnlyList skillPaths, bool verbose, CheckOptions? checkOptions = null) { var allSkills = new List(); foreach (var path in skillPaths) @@ -293,24 +294,21 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config) if (skillPaths.Count > 0) { var searched = string.Join(", ", skillPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); - Console.Error.WriteLine($"No skills found in the specified paths: {searched}"); + builder.AddPlainError($"No skills found in the specified paths: {searched}"); } - return (allSkills, 0); - } - Console.WriteLine($"Found {allSkills.Count} skill(s)"); + return ([], 0); + } - bool hasErrors = false; - if (ValidateSkillProfiles(allSkills, verbose, checkOptions)) - hasErrors = true; + bool hasErrors = ValidateSkillProfiles(builder, allSkills, verbose, checkOptions); - if (CheckDuplicateSkillNames(allSkills)) + if (CheckDuplicateSkillNames(builder, builder.Skills)) hasErrors = true; - return (allSkills, hasErrors ? 1 : 0); + return (builder.Skills, hasErrors ? 1 : 0); } - private static async Task<(IReadOnlyList Agents, int Result)> RunAgentsCheckCore(IReadOnlyList agentPaths) + private static async Task<(IReadOnlyList Agents, IReadOnlyList DiscoveredAgents, int Result)> RunAgentsCheckCore(CheckReportBuilder builder, IReadOnlyList agentPaths) { var allAgents = new List(); foreach (var path in agentPaths) @@ -324,46 +322,47 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config) if (agentPaths.Count > 0) { var searched = string.Join(", ", agentPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); - Console.Error.WriteLine($"No agents found in the specified paths: {searched}"); + builder.AddPlainError($"No agents found in the specified paths: {searched}"); } - return (allAgents, 0); - } - Console.WriteLine($"Found {allAgents.Count} agent(s)"); + return ([], [], 0); + } bool hasErrors = false; foreach (var agent in allAgents) { var profile = AgentProfiler.AnalyzeAgent(agent); - foreach (var warning in profile.Warnings) - Console.WriteLine($"{Ansi.Yellow}⚠ [agent:{profile.Name}] {warning}{Ansi.Reset}"); - foreach (var error in profile.Errors) + var result = new AgentCheckResult { - Console.Error.WriteLine($"{Ansi.Red}❌ [agent:{profile.Name}] {error}{Ansi.Reset}"); + Name = profile.Name, + FileName = profile.FileName, + Path = agent.Path, + }; + result.Warnings.AddRange(profile.Warnings); + result.Errors.AddRange(profile.Errors); + builder.Agents.Add(result); + + foreach (var error in profile.Errors) hasErrors = true; - } } - Console.WriteLine($"Validated {allAgents.Count} agent(s)\n"); if (hasErrors) - { - Console.Error.WriteLine("{Ansi.Red}Agent spec conformance failures — fix the errors above.{Ansi.Reset}"); - return (allAgents, 1); - } + return (builder.Agents, allAgents, 1); - return (allAgents, 0); + return (builder.Agents, allAgents, 0); } - private static bool CheckDuplicateSkillNames(IReadOnlyList skills) + private static bool CheckDuplicateSkillNames(CheckReportBuilder builder, IReadOnlyList skills) { - var seenNames = new Dictionary(StringComparer.Ordinal); // name -> first path + var seenNames = new Dictionary(StringComparer.Ordinal); bool hasDuplicates = false; foreach (var skill in skills) { if (seenNames.TryGetValue(skill.Name, out var firstPath)) { - Console.Error.WriteLine($"{Ansi.Red}❌ Duplicate skill name '{skill.Name}' found in '{skill.Path}' (first seen in '{firstPath}'){Ansi.Reset}"); + var message = $"Duplicate skill name '{skill.Name}' found in '{skill.Path}' (first seen in '{firstPath}')"; + skill.Errors.Add(message); hasDuplicates = true; } else @@ -375,96 +374,431 @@ private static bool CheckDuplicateSkillNames(IReadOnlyList skills) return hasDuplicates; } - private static bool ValidateSkillProfiles(IReadOnlyList skills, bool verbose, CheckOptions? checkOptions = null) + private static bool ValidateSkillProfiles(CheckReportBuilder builder, IReadOnlyList skills, bool verbose, CheckOptions? checkOptions = null) { bool hasErrors = false; foreach (var skill in skills) { var profile = SkillProfiler.AnalyzeSkill(skill, checkOptions); + var profileLine = SkillProfiler.FormatProfileLine(profile); + var result = new SkillCheckResult + { + Name = skill.Name, + Path = skill.Path, + SkillMdPath = skill.SkillMdPath, + Profile = profile, + ProfileLine = verbose ? profileLine : null, + }; - if (verbose) - Console.WriteLine($"[{skill.Name}] 📊 {SkillProfiler.FormatProfileLine(profile)}"); + result.Errors.AddRange(profile.Errors); + result.Warnings.AddRange(profile.Warnings); + builder.Skills.Add(result); foreach (var error in profile.Errors) - { - Console.Error.WriteLine($"{Ansi.Red}❌ [{skill.Name}] {error}{Ansi.Reset}"); hasErrors = true; - } - foreach (var warning in SkillProfiler.FormatProfileWarnings(profile)) - Console.WriteLine($"[{skill.Name}] {warning}"); } - if (hasErrors) - Console.Error.WriteLine("{Ansi.Red}Skill spec conformance failures — fix the errors above.{Ansi.Reset}"); - return hasErrors; } - private static void CheckExternalDeps(string? allowedExternalDepsFile, IReadOnlyList skills, IReadOnlyList agents, IReadOnlyList plugins) + private static void CheckExternalDeps(CheckReportBuilder builder, string? allowedExternalDepsFile, IReadOnlyList skills, IReadOnlyList agents, IReadOnlyList plugins) { if (allowedExternalDepsFile is null) return; - bool hasExternalDeps = false; var allowed = ExternalDependencyChecker.LoadAllowList(allowedExternalDepsFile); + foreach (var skill in skills) { foreach (var warning in ExternalDependencyChecker.CheckSkill(skill, allowed)) - { - Console.WriteLine($"{Ansi.Yellow}⚠ [skill:{skill.Name}] {warning}{Ansi.Reset}"); - hasExternalDeps = true; - } + builder.ExternalDependencies.Add(new ExternalDependencyResult("skill", skill.Name, warning)); } + foreach (var agent in agents) { foreach (var warning in ExternalDependencyChecker.CheckAgent(agent, allowed)) - { - Console.WriteLine($"{Ansi.Yellow}⚠ [agent:{agent.Name}] {warning}{Ansi.Reset}"); - hasExternalDeps = true; - } + builder.ExternalDependencies.Add(new ExternalDependencyResult("agent", agent.Name, warning)); } + foreach (var plugin in plugins) { foreach (var warning in ExternalDependencyChecker.CheckPlugin(plugin, allowed)) - { - Console.WriteLine($"{Ansi.Yellow}⚠ [plugin:{plugin.Name}] {warning}{Ansi.Reset}"); - hasExternalDeps = true; - } + builder.ExternalDependencies.Add(new ExternalDependencyResult("plugin", plugin.Name, warning)); } - if (hasExternalDeps) - Console.WriteLine(); } - /// - /// Run the reference scanner on discovered files. Returns true if errors were found. - /// - private static bool RunReferenceScanner(string? knownDomainsFile, IReadOnlyList directories) + private static bool RunReferenceScanner(CheckReportBuilder builder, string? knownDomainsFile, IReadOnlyList directories) { if (knownDomainsFile is null) + { + builder.ReferenceScan = new ReferenceScanReport(ReferenceScanStatus.Disabled, null, 0, []); return false; + } if (!File.Exists(knownDomainsFile)) { - Console.Error.WriteLine($"{Ansi.Red}❌ Known-domains file not found: '{knownDomainsFile}'{Ansi.Reset}"); + builder.ReferenceScan = new ReferenceScanReport(ReferenceScanStatus.MissingKnownDomainsFile, knownDomainsFile, 0, []); + builder.AddGeneralError($"Known-domains file not found: '{knownDomainsFile}'"); return true; } var knownDomains = ReferenceScanner.LoadKnownDomains(knownDomainsFile); var files = ReferenceScanner.DiscoverFiles(directories); var findings = ReferenceScanner.ScanFiles(files, knownDomains, knownDomainsFile); + builder.ReferenceScan = new ReferenceScanReport(ReferenceScanStatus.Completed, knownDomainsFile, files.Count, findings); if (findings.Count > 0) + return true; + + return false; + } + + private static void RenderReport(CheckReport report) + { + if (report.Invocation.OutputMode == CheckOutputMode.Json) + { + var jsonOutput = CreateJsonOutput(report); + Console.Out.WriteLine(JsonSerializer.Serialize(jsonOutput, SkillValidatorJsonContext.Default.CheckJsonOutput)); + return; + } + + RenderPlugins(report); + RenderSkills(report); + RenderAgents(report); + RenderExternalDependencies(report); + RenderReferenceScan(report); + RenderGeneralErrors(report); + + if (report.Succeeded) + Console.WriteLine($"{Ansi.Green}✅ {FormatSuccessSummary(report)}{Ansi.Reset}"); + } + + private static CheckJsonOutput CreateJsonOutput(CheckReport report) + { + var plugins = report.Plugins + .Select(source => new CheckJsonPlugin( + Name: source.Name, + DirectoryPath: source.DirectoryPath, + Errors: [.. source.Errors], + Warnings: source.Warnings + .Select(warning => new CheckJsonWarning("validation", warning)) + .ToList())) + .ToList(); + + var skills = report.Skills + .Select(source => new CheckJsonSkill( + Name: source.Name, + Path: source.Path, + SkillMdPath: source.SkillMdPath, + Errors: [.. source.Errors], + Warnings: source.Warnings + .Select(warning => new CheckJsonWarning("profile", warning)) + .ToList(), + Profile: source.Profile is null ? null : new CheckJsonSkillProfile( + Name: source.Profile.Name, + Chars4TokenCount: source.Profile.Chars4TokenCount, + BpeTokenCount: source.Profile.BpeTokenCount, + ComplexityTier: source.Profile.ComplexityTier, + SectionCount: source.Profile.SectionCount, + CodeBlockCount: source.Profile.CodeBlockCount, + NumberedStepCount: source.Profile.NumberedStepCount, + BulletCount: source.Profile.BulletCount, + HasFrontmatter: source.Profile.HasFrontmatter, + HasWhenToUse: source.Profile.HasWhenToUse, + HasWhenNotToUse: source.Profile.HasWhenNotToUse), + ProfileLine: source.ProfileLine)) + .ToList(); + + var agents = report.Agents + .Select(source => new CheckJsonAgent( + Name: source.Name, + FileName: source.FileName, + Path: source.Path, + Errors: [.. source.Errors], + Warnings: source.Warnings + .Select(warning => new CheckJsonWarning("validation", warning)) + .ToList())) + .ToList(); + + foreach (var dependency in report.ExternalDependencies) + { + switch (dependency.Kind) + { + case "plugin": + if (plugins.FirstOrDefault(plugin => string.Equals(plugin.Name, dependency.Name, StringComparison.Ordinal)) is { } plugin) + plugin.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + break; + case "skill": + if (skills.FirstOrDefault(skill => string.Equals(skill.Name, dependency.Name, StringComparison.Ordinal)) is { } skill) + skill.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + break; + case "agent": + if (agents.FirstOrDefault(agent => string.Equals(agent.Name, dependency.Name, StringComparison.Ordinal)) is { } agent) + agent.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + break; + } + } + + var topLevelErrors = report.GeneralErrors.ToList(); + + if (report.ReferenceScan is not null) + { + foreach (var finding in report.ReferenceScan.Findings) + { + var formattedFinding = $"{finding.Path}:{finding.LineNum} [{finding.Code}] {finding.Message}"; + if (TryAttachReferenceFinding(skills, agents, plugins, finding.Path, formattedFinding)) + continue; + + topLevelErrors.Add(formattedFinding); + } + } + + return new CheckJsonOutput( + Counts: new CheckJsonCounts( + PluginCount: plugins.Count, + SkillCount: skills.Count, + AgentCount: agents.Count), + Plugins: plugins, + Skills: skills, + Agents: agents, + Errors: topLevelErrors.Count > 0 ? topLevelErrors : null); + } + + private static void RenderPlugins(CheckReport report) + { + if (report.Plugins.Count == 0) + return; + + foreach (var plugin in report.Plugins) + { + foreach (var warning in plugin.Warnings) + Console.WriteLine($"{Ansi.Yellow}⚠ [plugin:{plugin.Name}] {warning}{Ansi.Reset}"); + foreach (var error in plugin.Errors) + Console.Error.WriteLine($"{Ansi.Red}❌ [plugin:{plugin.Name}] {error}{Ansi.Reset}"); + } + + Console.WriteLine($"Validated {report.Plugins.Count} plugin(s)"); + + if (report.Plugins.Any(plugin => plugin.Errors.Count > 0)) + Console.Error.WriteLine($"{Ansi.Red}Plugin spec conformance failures — fix the errors above.{Ansi.Reset}"); + } + + private static void RenderSkills(CheckReport report) + { + if (report.Skills.Count == 0) + return; + + Console.WriteLine($"Found {report.Skills.Count} skill(s)"); + + foreach (var skill in report.Skills) + { + if (skill.ProfileLine is not null) + Console.WriteLine($"[{skill.Name}] 📊 {skill.ProfileLine}"); + + foreach (var error in skill.Errors) + Console.Error.WriteLine($"{Ansi.Red}❌ [{skill.Name}] {error}{Ansi.Reset}"); + + foreach (var warning in skill.Warnings) + Console.WriteLine($"[{skill.Name}] ⚠ {warning}"); + } + + if (report.Skills.Any(skill => skill.Errors.Count > 0)) + Console.Error.WriteLine($"{Ansi.Red}Skill spec conformance failures — fix the errors above.{Ansi.Reset}"); + } + + private static void RenderAgents(CheckReport report) + { + if (report.Agents.Count == 0) + return; + + Console.WriteLine($"Found {report.Agents.Count} agent(s)"); + + foreach (var agent in report.Agents) + { + foreach (var warning in agent.Warnings) + Console.WriteLine($"{Ansi.Yellow}⚠ [agent:{agent.Name}] {warning}{Ansi.Reset}"); + foreach (var error in agent.Errors) + Console.Error.WriteLine($"{Ansi.Red}❌ [agent:{agent.Name}] {error}{Ansi.Reset}"); + } + + Console.WriteLine($"Validated {report.Agents.Count} agent(s)"); + + if (report.Agents.Any(agent => agent.Errors.Count > 0)) + Console.Error.WriteLine($"{Ansi.Red}Agent spec conformance failures — fix the errors above.{Ansi.Reset}"); + } + + private static void RenderExternalDependencies(CheckReport report) + { + if (report.ExternalDependencies.Count == 0) + return; + + foreach (var dependency in report.ExternalDependencies) + Console.WriteLine($"{Ansi.Yellow}⚠ [{dependency.Kind}:{dependency.Name}] {dependency.Message}{Ansi.Reset}"); + + Console.WriteLine(); + } + + private static void RenderReferenceScan(CheckReport report) + { + if (report.ReferenceScan is null) + return; + + if (report.ReferenceScan.Status == ReferenceScanStatus.Completed) { - Console.Error.WriteLine($"\n {findings.Count} reference error(s):\n"); - foreach (var f in findings) - Console.Error.WriteLine($" {Ansi.Red}❌ {f.Path}:{f.LineNum} [{f.Code}] {f.Message}{Ansi.Reset}"); + if (report.ReferenceScan.Findings.Count == 0) + { + Console.WriteLine($"--- Reference scan: {report.ReferenceScan.FilesScanned} file(s) scanned, 0 error(s) ---"); + return; + } + + Console.Error.WriteLine($"\n {report.ReferenceScan.Findings.Count} reference error(s):\n"); + foreach (var finding in report.ReferenceScan.Findings) + Console.Error.WriteLine($" {Ansi.Red}❌ {finding.Path}:{finding.LineNum} [{finding.Code}] {finding.Message}{Ansi.Reset}"); Console.Error.WriteLine(); - Console.Error.WriteLine($"{Ansi.Red}--- Reference scan: {files.Count} file(s) scanned, {findings.Count} error(s) ---{Ansi.Reset}"); - return true; + Console.Error.WriteLine($"{Ansi.Red}--- Reference scan: {report.ReferenceScan.FilesScanned} file(s) scanned, {report.ReferenceScan.Findings.Count} error(s) ---{Ansi.Reset}"); + } + } + + private static void RenderGeneralErrors(CheckReport report) + { + foreach (var error in report.GeneralErrors) + Console.Error.WriteLine($"{Ansi.Red}❌ {error}{Ansi.Reset}"); + } + + private static string FormatSuccessSummary(CheckReport report) => + report.Scope switch + { + "plugin" => $"All checks passed ({report.Counts.SkillCount} skill(s), {report.Counts.AgentCount} agent(s), {report.Counts.PluginCount} plugin(s))", + "skillsAndAgents" => $"All checks passed ({report.Counts.SkillCount} skill(s), {report.Counts.AgentCount} agent(s))", + "skills" => $"All checks passed ({report.Counts.SkillCount} skill(s))", + "agents" => $"All checks passed ({report.Counts.AgentCount} agent(s))", + _ => "All checks passed", + }; + + private static bool TryAttachReferenceFinding( + IReadOnlyList skills, + IReadOnlyList agents, + IReadOnlyList plugins, + string findingPath, + string formattedFinding) + { + foreach (var skill in skills) + { + if (IsPathWithin(findingPath, skill.Path) || IsPathWithin(findingPath, skill.SkillMdPath)) + { + skill.Errors.Add(formattedFinding); + return true; + } + } + + foreach (var agent in agents) + { + if (IsPathWithin(findingPath, agent.Path) || IsPathWithin(findingPath, Path.GetDirectoryName(agent.Path))) + { + agent.Errors.Add(formattedFinding); + return true; + } + } + + foreach (var plugin in plugins) + { + if (IsPathWithin(findingPath, plugin.DirectoryPath)) + { + plugin.Errors.Add(formattedFinding); + return true; + } } - Console.WriteLine($"--- Reference scan: {files.Count} file(s) scanned, 0 error(s) ---"); return false; } -} + private static bool IsPathWithin(string candidatePath, string? containerPath) + { + if (string.IsNullOrWhiteSpace(containerPath)) + return false; + + var fullCandidatePath = Path.GetFullPath(candidatePath); + var fullContainerPath = Path.GetFullPath(containerPath); + + if (string.Equals(fullCandidatePath, fullContainerPath, StringComparison.OrdinalIgnoreCase)) + return true; + + if (File.Exists(fullContainerPath)) + fullContainerPath = Path.GetDirectoryName(fullContainerPath)!; + + var normalizedContainer = fullContainerPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; + + return fullCandidatePath.StartsWith(normalizedContainer, StringComparison.OrdinalIgnoreCase); + } + + private sealed class CheckReportBuilder(CheckConfig config, string scope) + { + public List GeneralErrors { get; } = []; + + public List Plugins { get; } = []; + + public List Skills { get; } = []; + + public List Agents { get; } = []; + + public List ExternalDependencies { get; } = []; + + public ReferenceScanReport? ReferenceScan { get; set; } + + public void AddPlainError(string text) => GeneralErrors.Add(text); + + public void AddGeneralError(string text) => GeneralErrors.Add(text); + + public CheckReport Build(int exitCode) + { + var warningCount = Plugins.Sum(plugin => plugin.Warnings.Count) + + Skills.Sum(skill => skill.Warnings.Count) + + Agents.Sum(agent => agent.Warnings.Count) + + ExternalDependencies.Count; + + var errorCount = GeneralErrors.Count + + Plugins.Sum(plugin => plugin.Errors.Count) + + Skills.Sum(skill => skill.Errors.Count) + + Agents.Sum(agent => agent.Errors.Count) + + (ReferenceScan?.Findings.Count ?? 0); + + var infoCount = (Plugins.Count > 0 ? 1 : 0) + + (Skills.Count > 0 ? 1 : 0) + + Skills.Count(skill => skill.ProfileLine is not null) + + (Agents.Count > 0 ? 2 : 0) + + (ReferenceScan?.Status == ReferenceScanStatus.Completed && ReferenceScan.Findings.Count == 0 ? 1 : 0) + + (exitCode == 0 ? 1 : 0); + + var counts = new CheckCounts( + PluginCount: Plugins.Count, + SkillCount: Skills.Count, + AgentCount: Agents.Count, + ReferenceFileCount: ReferenceScan?.FilesScanned ?? 0, + InfoCount: infoCount, + WarningCount: warningCount, + ErrorCount: errorCount); + + return new CheckReport( + Scope: scope, + Invocation: new CheckInvocation( + PluginPaths: config.PluginPaths, + SkillPaths: config.SkillPaths, + AgentPaths: config.AgentPaths, + AllowedExternalDepsFile: config.AllowedExternalDepsFile, + KnownDomainsFile: config.KnownDomainsFile, + Verbose: config.Verbose, + AllowRepoTraversal: config.CheckOptions.AllowRepoTraversal, + OutputMode: config.OutputMode), + Succeeded: exitCode == 0, + ExitCode: exitCode, + Counts: counts, + GeneralErrors: GeneralErrors, + Plugins: Plugins, + Skills: Skills, + Agents: Agents, + ExternalDependencies: ExternalDependencies, + ReferenceScan: ReferenceScan); + } + } +} diff --git a/eng/skill-validator/src/Check/Models.cs b/eng/skill-validator/src/Check/Models.cs index b246368054..50d786ba58 100644 --- a/eng/skill-validator/src/Check/Models.cs +++ b/eng/skill-validator/src/Check/Models.cs @@ -6,11 +6,140 @@ public sealed record AgentProfile( IReadOnlyList Errors, IReadOnlyList Warnings); -public sealed record PluginValidationResult( +public sealed class PluginCheckResult +{ + public string Name { get; init; } = ""; + public string DirectoryPath { get; init; } = ""; + public List Errors { get; } = []; + public List Warnings { get; } = []; +} + +public sealed class SkillCheckResult +{ + public string Name { get; init; } = ""; + public string Path { get; init; } = ""; + public string SkillMdPath { get; init; } = ""; + public SkillProfile? Profile { get; init; } + public string? ProfileLine { get; init; } + public List Errors { get; } = []; + public List Warnings { get; } = []; +} + +public sealed class AgentCheckResult +{ + public string Name { get; init; } = ""; + public string FileName { get; init; } = ""; + public string Path { get; init; } = ""; + public List Errors { get; } = []; + public List Warnings { get; } = []; +} + +public sealed record ExternalDependencyResult( + string Kind, + string Name, + string Message); + +public sealed record CheckInvocation( + IReadOnlyList PluginPaths, + IReadOnlyList SkillPaths, + IReadOnlyList AgentPaths, + string? AllowedExternalDepsFile, + string? KnownDomainsFile, + bool Verbose, + bool AllowRepoTraversal, + CheckOutputMode OutputMode); + +public sealed record CheckCounts( + int PluginCount, + int SkillCount, + int AgentCount, + int ReferenceFileCount, + int InfoCount, + int WarningCount, + int ErrorCount); + +public sealed record CheckJsonCounts( + int PluginCount, + int SkillCount, + int AgentCount); + +public sealed record CheckJsonWarning( + string Kind, + string Message); + +public sealed record CheckJsonPlugin( string Name, string DirectoryPath, - IReadOnlyList Errors, - IReadOnlyList Warnings); + List Errors, + List Warnings); + +public sealed record CheckJsonSkillProfile( + string Name, + int Chars4TokenCount, + int BpeTokenCount, + string ComplexityTier, + int SectionCount, + int CodeBlockCount, + int NumberedStepCount, + int BulletCount, + bool HasFrontmatter, + bool HasWhenToUse, + bool HasWhenNotToUse); + +public sealed record CheckJsonSkill( + string Name, + string Path, + string SkillMdPath, + List Errors, + List Warnings, + CheckJsonSkillProfile? Profile = null, + string? ProfileLine = null); + +public sealed record CheckJsonAgent( + string Name, + string FileName, + string Path, + List Errors, + List Warnings); + +public enum ReferenceScanStatus +{ + Disabled, + MissingKnownDomainsFile, + Completed, +} + +public sealed record ReferenceScanReport( + ReferenceScanStatus Status, + string? KnownDomainsFile, + int FilesScanned, + IReadOnlyList Findings); + +public sealed record CheckReport( + string Scope, + CheckInvocation Invocation, + bool Succeeded, + int ExitCode, + CheckCounts Counts, + IReadOnlyList GeneralErrors, + IReadOnlyList Plugins, + IReadOnlyList Skills, + IReadOnlyList Agents, + IReadOnlyList ExternalDependencies, + ReferenceScanReport? ReferenceScan); + +public sealed record CheckJsonOutput( + CheckJsonCounts Counts, + IReadOnlyList Plugins, + IReadOnlyList Skills, + IReadOnlyList Agents, + IReadOnlyList? Errors = null); + +public enum CheckOutputMode +{ + Console, + Json, +} public sealed record CheckOptions { @@ -26,4 +155,5 @@ public sealed record CheckConfig public string? KnownDomainsFile { get; init; } public bool Verbose { get; init; } public CheckOptions CheckOptions { get; init; } = new(); + public CheckOutputMode OutputMode { get; init; } = CheckOutputMode.Console; } diff --git a/eng/skill-validator/src/Check/PluginProfiler.cs b/eng/skill-validator/src/Check/PluginProfiler.cs index 92536798d0..1b76d224f4 100644 --- a/eng/skill-validator/src/Check/PluginProfiler.cs +++ b/eng/skill-validator/src/Check/PluginProfiler.cs @@ -9,7 +9,7 @@ namespace SkillValidator.Check; /// public static class PluginProfiler { - public static PluginValidationResult ValidatePlugin(PluginInfo plugin) + public static PluginCheckResult ValidatePlugin(PluginInfo plugin) { var errors = new List(); var warnings = new List(); @@ -85,7 +85,13 @@ public static PluginValidationResult ValidatePlugin(PluginInfo plugin) ? plugin.Name : (!string.IsNullOrWhiteSpace(plugin.DirectoryName) ? plugin.DirectoryName : "(unknown)"); - return new PluginValidationResult(resultName, plugin.DirectoryPath, errors, warnings); + var result = new PluginCheckResult + { + Name = resultName, + DirectoryPath = plugin.DirectoryPath, + }; + result.Errors.AddRange(errors); + result.Warnings.AddRange(warnings); + return result; } } - diff --git a/eng/skill-validator/src/README.md b/eng/skill-validator/src/README.md index 354fed3a21..c3f346b446 100644 --- a/eng/skill-validator/src/README.md +++ b/eng/skill-validator/src/README.md @@ -106,6 +106,9 @@ skill-validator check --plugin ./plugins/my-plugin --allowed-external-deps ./eng # Verbose output skill-validator check --verbose --plugin ./plugins/my-plugin + +# Emit machine-readable JSON to stdout +skill-validator check --json --plugin ./plugins/my-plugin ``` ## `check` flags @@ -118,6 +121,7 @@ skill-validator check --verbose --plugin ./plugins/my-plugin | `--allowed-external-deps ` | *(none)* | Path to allowed-external-deps.txt; when omitted the external-deps check is skipped | | `--known-domains ` | *(none)* | Path to known-domains.txt for reference scanning; when omitted the reference scan is skipped | | `--verbose` | `false` | Show detailed output | +| `--json` | `false` | Write a machine-readable JSON report to stdout | > `--plugin` must be used alone. `--skills` and `--agents` can be combined. @@ -367,7 +371,7 @@ The complexity tier is derived from the BPE token count: | standard | 2,501 – 5,000 | Approaching diminishing returns | | comprehensive | > 5,000 | ✗ Performance degrades | -> **Note:** The `check` command outputs to the console only — it does not write result files. Warnings about skill size are always printed; the full profile line requires `--verbose`. +> **Note:** The `check` command writes to stdout. Use the default console output for human-readable summaries, or `--json` for a machine-readable payload you can pipe to a file or another tool. Warnings about skill size are always printed in console mode; the full profile line requires `--verbose`. ## Metrics & scoring diff --git a/eng/skill-validator/src/SkillValidatorJsonContext.cs b/eng/skill-validator/src/SkillValidatorJsonContext.cs index 2aaae673bf..2e4b9cd5e0 100644 --- a/eng/skill-validator/src/SkillValidatorJsonContext.cs +++ b/eng/skill-validator/src/SkillValidatorJsonContext.cs @@ -1,6 +1,7 @@ using System.Text.Json; using System.Text.Json.Nodes; using System.Text.Json.Serialization; +using SkillValidator.Check; using SkillValidator.Evaluate; using SkillValidator.Shared; @@ -10,7 +11,8 @@ namespace SkillValidator; WriteIndented = true, PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, PropertyNameCaseInsensitive = true, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)] + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + UseStringEnumConverter = true)] [JsonSerializable(typeof(ResultsOutput))] [JsonSerializable(typeof(ConsolidateData))] [JsonSerializable(typeof(SkillVerdict))] @@ -42,6 +44,7 @@ namespace SkillValidator; [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(string[]))] +[JsonSerializable(typeof(CheckJsonOutput))] [JsonSerializable(typeof(List))] [JsonSerializable(typeof(IReadOnlyList))] internal partial class SkillValidatorJsonContext : JsonSerializerContext; diff --git a/eng/skill-validator/tests/Check/CheckCommandTests.cs b/eng/skill-validator/tests/Check/CheckCommandTests.cs index 47310e1693..3efd3cea59 100644 --- a/eng/skill-validator/tests/Check/CheckCommandTests.cs +++ b/eng/skill-validator/tests/Check/CheckCommandTests.cs @@ -1,8 +1,14 @@ +using Xunit; +using System.Text.Json; using SkillValidator.Check; using SkillValidator.Shared; namespace SkillValidator.Tests; +[CollectionDefinition("CheckCommandConsole", DisableParallelization = true)] +public sealed class CheckCommandConsoleCollection; + +[Collection("CheckCommandConsole")] public class CheckCommandAggregateDescriptionTests { private static string CreatePluginFixture(string pluginName, params (string skillName, string description)[] skills) @@ -131,6 +137,7 @@ private static string CreatePluginInDir(string root, string pluginName, params ( } } +[Collection("CheckCommandConsole")] public class DuplicateSkillNameTests { private static string CreatePluginFixture(string pluginName, params (string skillName, string description)[] skills) @@ -204,6 +211,7 @@ public async Task DuplicateSkillNames_Fails() } } +[Collection("CheckCommandConsole")] public class CheckCommandFilePathTests { private static string CreateSkillFixture(string skillName, string description) @@ -322,3 +330,139 @@ public async Task CombinedSkillsAndAgents_WithFilePaths_Passes() } } } + +[Collection("CheckCommandConsole")] +public class CheckCommandJsonOutputTests +{ + private static string CreateSkillFixture(string skillName, string description) + { + var root = Path.Combine(Path.GetTempPath(), $"json-test-{Guid.NewGuid():N}"); + var skillDir = Path.Combine(root, skillName); + Directory.CreateDirectory(skillDir); + File.WriteAllText(Path.Combine(skillDir, "SKILL.md"), + $"---\nname: {skillName}\ndescription: {description}\n---\n# {skillName}\n\nContent.\n"); + return root; + } + + [Fact] + public async Task JsonOutput_WithSkills_WritesStructuredReportToStdout() + { + var root = CreateSkillFixture("json-skill", "A short description."); + try + { + var capture = await ConsoleCapture.RunAsync(() => CheckCommand.Run(new CheckConfig + { + SkillPaths = [Path.Combine(root, "json-skill")], + OutputMode = CheckOutputMode.Json, + })); + + Assert.Equal(0, capture.ExitCode); + Assert.Equal("", capture.StandardError); + + using var document = JsonDocument.Parse(capture.StandardOutput); + var report = document.RootElement; + var skill = report.GetProperty("skills")[0]; + var warning = skill.GetProperty("warnings")[0]; + + Assert.Equal(1, report.GetProperty("counts").GetProperty("skillCount").GetInt32()); + Assert.Equal(1, report.GetProperty("skills").GetArrayLength()); + Assert.False(report.TryGetProperty("messages", out _)); + Assert.False(report.TryGetProperty("invocation", out _)); + Assert.False(report.TryGetProperty("scope", out _)); + Assert.False(report.TryGetProperty("exitCode", out _)); + Assert.False(report.TryGetProperty("succeeded", out _)); + Assert.True(skill.GetProperty("warnings").GetArrayLength() > 0); + Assert.Equal("profile", warning.GetProperty("kind").GetString()); + Assert.False(string.IsNullOrWhiteSpace(warning.GetProperty("message").GetString())); + Assert.False(skill.GetProperty("profile").TryGetProperty("warnings", out _)); + } + finally + { + Directory.Delete(root, true); + } + } + + [Fact] + public async Task JsonFlag_WithMissingPaths_WritesStructuredFailureToStdout() + { + var command = CheckCommand.Create(); + var capture = await ConsoleCapture.RunAsync(() => command.Parse(["--json"]).InvokeAsync()); + + Assert.Equal(1, capture.ExitCode); + Assert.Equal("", capture.StandardError); + + using var document = JsonDocument.Parse(capture.StandardOutput); + var report = document.RootElement; + + Assert.Equal(0, report.GetProperty("counts").GetProperty("pluginCount").GetInt32()); + Assert.Equal(0, report.GetProperty("skills").GetArrayLength()); + Assert.Contains(report.GetProperty("errors").EnumerateArray(), + error => error.GetString()!.Contains("Specify one of --plugin, --skills, or --agents.", StringComparison.Ordinal)); + } + + [Fact] + public async Task JsonOutput_WithMissingKnownDomains_WritesReferenceFailureToStdout() + { + var root = CreateSkillFixture("json-skill", "A short description."); + try + { + var missingKnownDomains = Path.Combine(root, "known-domains.txt"); + var capture = await ConsoleCapture.RunAsync(() => CheckCommand.Run(new CheckConfig + { + SkillPaths = [Path.Combine(root, "json-skill")], + KnownDomainsFile = missingKnownDomains, + OutputMode = CheckOutputMode.Json, + })); + + Assert.Equal(1, capture.ExitCode); + Assert.Equal("", capture.StandardError); + + using var document = JsonDocument.Parse(capture.StandardOutput); + var report = document.RootElement; + + Assert.Equal(1, report.GetProperty("skills").GetArrayLength()); + Assert.Contains(report.GetProperty("errors").EnumerateArray(), + error => error.GetString() == $"Known-domains file not found: '{missingKnownDomains}'"); + Assert.False(report.TryGetProperty("referenceScan", out _)); + } + finally + { + Directory.Delete(root, true); + } + } +} + +public sealed record ConsoleCaptureResult( + int ExitCode, + string StandardOutput, + string StandardError); + +public static class ConsoleCapture +{ + private static readonly SemaphoreSlim s_lock = new(1, 1); + + public static async Task RunAsync(Func> action) + { + await s_lock.WaitAsync(); + + var originalOut = Console.Out; + var originalErr = Console.Error; + using var stdout = new StringWriter(); + using var stderr = new StringWriter(); + + Console.SetOut(stdout); + Console.SetError(stderr); + + try + { + var exitCode = await action(); + return new ConsoleCaptureResult(exitCode, stdout.ToString(), stderr.ToString()); + } + finally + { + Console.SetOut(originalOut); + Console.SetError(originalErr); + s_lock.Release(); + } + } +} From da42f1c95ea318149f577475b5a933c5fbda855d Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Fri, 1 May 2026 10:02:16 +1000 Subject: [PATCH 2/6] Fix empty check discovery results Address PR feedback by failing when explicit skill or agent paths discover nothing, including the combined check path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/skill-validator/src/Check/CheckCommand.cs | 2 + .../tests/Check/CheckCommandTests.cs | 37 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/eng/skill-validator/src/Check/CheckCommand.cs b/eng/skill-validator/src/Check/CheckCommand.cs index d3266430ea..455d6a3cc5 100644 --- a/eng/skill-validator/src/Check/CheckCommand.cs +++ b/eng/skill-validator/src/Check/CheckCommand.cs @@ -295,6 +295,7 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config, Check { var searched = string.Join(", ", skillPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); builder.AddPlainError($"No skills found in the specified paths: {searched}"); + return ([], 1); } return ([], 0); @@ -323,6 +324,7 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config, Check { var searched = string.Join(", ", agentPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); builder.AddPlainError($"No agents found in the specified paths: {searched}"); + return ([], [], 1); } return ([], [], 0); diff --git a/eng/skill-validator/tests/Check/CheckCommandTests.cs b/eng/skill-validator/tests/Check/CheckCommandTests.cs index 3efd3cea59..2c1bab3a9e 100644 --- a/eng/skill-validator/tests/Check/CheckCommandTests.cs +++ b/eng/skill-validator/tests/Check/CheckCommandTests.cs @@ -329,6 +329,43 @@ public async Task CombinedSkillsAndAgents_WithFilePaths_Passes() Directory.Delete(agentRoot, true); } } + + [Fact] + public async Task SkillsArg_WithNoDiscoveredSkills_Fails() + { + var root = Path.Combine(Path.GetTempPath(), $"file-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(root); + try + { + var config = new CheckConfig { SkillPaths = [root] }; + var result = await CheckCommand.Run(config); + Assert.Equal(1, result); + } + finally { Directory.Delete(root, true); } + } + + [Fact] + public async Task CombinedSkillsAndAgents_WithNoDiscoveredAgents_Fails() + { + var skillRoot = CreateSkillFixture("my-skill", "A short description."); + var emptyAgentRoot = Path.Combine(Path.GetTempPath(), $"file-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(emptyAgentRoot); + try + { + var config = new CheckConfig + { + SkillPaths = [Path.Combine(skillRoot, "my-skill")], + AgentPaths = [emptyAgentRoot], + }; + var result = await CheckCommand.Run(config); + Assert.Equal(1, result); + } + finally + { + Directory.Delete(skillRoot, true); + Directory.Delete(emptyAgentRoot, true); + } + } } [Collection("CheckCommandConsole")] From 57e4eac875fe5d98a7d66444848813a0eae9574f Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Wed, 10 Jun 2026 18:15:44 +1000 Subject: [PATCH 3/6] Address Copilot review comments - Replace AddPlainError with AddGeneralError (remove duplicate method) - Key pluginSkills by DirectoryPath instead of display name to avoid mismatch when plugin.Name differs from directory name - Use OS-aware path comparison in IsPathWithin (Ordinal on Unix, OrdinalIgnoreCase on Windows) - Pre-build name lookup dictionaries in CreateJsonOutput to eliminate O(n*m) FirstOrDefault scans for external dependency attachment - Extract CheckJsonSerializerContext scoped to check JSON output so UseStringEnumConverter does not affect unrelated JSON payloads Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/skill-validator/src/Check/CheckCommand.cs | 59 ++++++++++++------- .../src/Check/CheckJsonSerializerContext.cs | 12 ++++ .../src/SkillValidatorJsonContext.cs | 5 +- 3 files changed, 52 insertions(+), 24 deletions(-) create mode 100644 eng/skill-validator/src/Check/CheckJsonSerializerContext.cs diff --git a/eng/skill-validator/src/Check/CheckCommand.cs b/eng/skill-validator/src/Check/CheckCommand.cs index 446467fbdd..4cea185573 100644 --- a/eng/skill-validator/src/Check/CheckCommand.cs +++ b/eng/skill-validator/src/Check/CheckCommand.cs @@ -6,6 +6,10 @@ namespace SkillValidator.Check; public static class CheckCommand { + private static readonly StringComparison s_pathComparison = OperatingSystem.IsWindows() + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + public static Command Create() { var pluginOpt = new Option("--plugin") { Description = "Plugin directories to check (discovers skills, agents, plugin.json)", AllowMultipleArgumentsPerToken = true }; @@ -85,19 +89,19 @@ private static bool ValidateConfig(CheckConfig config, CheckReportBuilder builde if (!hasPlugin && !hasSkills && !hasAgents) { - builder.AddPlainError("Specify one of --plugin, --skills, or --agents. Use --plugin to check an entire plugin directory."); + builder.AddGeneralError("Specify one of --plugin, --skills, or --agents. Use --plugin to check an entire plugin directory."); return false; } if (hasPlugin && (hasSkills || hasAgents)) { - builder.AddPlainError("--plugin cannot be combined with --skills or --agents. Use --plugin alone to check an entire plugin directory."); + builder.AddGeneralError("--plugin cannot be combined with --skills or --agents. Use --plugin alone to check an entire plugin directory."); return false; } if (config.CheckOptions.AllowRepoTraversal && hasPlugin) { - builder.AddPlainError("--allow-repo-traversal cannot be used with --plugin. Plugins must be portable — use --skills or --agents instead."); + builder.AddGeneralError("--allow-repo-traversal cannot be used with --plugin. Plugins must be portable — use --skills or --agents instead."); return false; } @@ -128,7 +132,6 @@ private static async Task RunPluginCheck(CheckConfig config, CheckReportBui foreach (var pluginDir in config.PluginPaths) { var fullPath = Path.GetFullPath(pluginDir); - var pluginName = Path.GetFileName(fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); var pluginJsonPath = Path.Combine(fullPath, "plugin.json"); if (!File.Exists(pluginJsonPath)) @@ -162,10 +165,10 @@ private static async Task RunPluginCheck(CheckConfig config, CheckReportBui continue; var skills = await SkillDiscovery.DiscoverSkills(dir!); - if (!pluginSkills.TryGetValue(pluginName, out var pluginSkillList)) + if (!pluginSkills.TryGetValue(plugin.DirectoryPath, out var pluginSkillList)) { pluginSkillList = []; - pluginSkills[pluginName] = pluginSkillList; + pluginSkills[plugin.DirectoryPath] = pluginSkillList; } pluginSkillList.AddRange(skills); @@ -206,19 +209,25 @@ private static async Task RunPluginCheck(CheckConfig config, CheckReportBui if (allSkillsList.Count == 0 && allAgents.Count == 0) { - builder.AddPlainError("No skills or agents found in the specified plugin(s)."); + builder.AddGeneralError("No skills or agents found in the specified plugin(s)."); return 1; } - foreach (var (pluginName, skills) in pluginSkills) + foreach (var (pluginDirectoryPath, skills) in pluginSkills) { int totalChars = skills.Sum(s => s.Description.Length); if (totalChars <= SkillProfiler.MaxAggregateDescriptionLength) continue; - var message = $"Plugin '{pluginName}' aggregate description size is {totalChars:N0} characters — maximum is {SkillProfiler.MaxAggregateDescriptionLength:N0}."; - if (builder.Plugins.FirstOrDefault(p => string.Equals(p.Name, pluginName, StringComparison.Ordinal)) is { } pluginResult) + var pluginResult = builder.Plugins.FirstOrDefault(p => string.Equals(p.DirectoryPath, pluginDirectoryPath, StringComparison.Ordinal)); + var pluginLabel = pluginResult?.Name + ?? Path.GetFileName(pluginDirectoryPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); + var message = $"Plugin '{pluginLabel}' aggregate description size is {totalChars:N0} characters — maximum is {SkillProfiler.MaxAggregateDescriptionLength:N0}."; + if (pluginResult is not null) pluginResult.Errors.Add(message); + else + builder.AddGeneralError(message); + return 1; } @@ -294,7 +303,7 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config, Check if (skillPaths.Count > 0) { var searched = string.Join(", ", skillPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); - builder.AddPlainError($"No skills found in the specified paths: {searched}"); + builder.AddGeneralError($"No skills found in the specified paths: {searched}"); return ([], 1); } @@ -323,7 +332,7 @@ private static async Task RunSkillsAndAgentsCheck(CheckConfig config, Check if (agentPaths.Count > 0) { var searched = string.Join(", ", agentPaths.Select(p => $"\"{Path.GetFullPath(p)}\"")); - builder.AddPlainError($"No agents found in the specified paths: {searched}"); + builder.AddGeneralError($"No agents found in the specified paths: {searched}"); return ([], [], 1); } @@ -460,7 +469,7 @@ private static void RenderReport(CheckReport report) if (report.Invocation.OutputMode == CheckOutputMode.Json) { var jsonOutput = CreateJsonOutput(report); - Console.Out.WriteLine(JsonSerializer.Serialize(jsonOutput, SkillValidatorJsonContext.Default.CheckJsonOutput)); + Console.Out.WriteLine(JsonSerializer.Serialize(jsonOutput, CheckJsonSerializerContext.Default.CheckJsonOutput)); return; } @@ -522,20 +531,32 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) .ToList())) .ToList(); + var pluginsByName = new Dictionary(StringComparer.Ordinal); + foreach (var plugin in plugins) + pluginsByName.TryAdd(plugin.Name, plugin); + + var skillsByName = new Dictionary(StringComparer.Ordinal); + foreach (var skill in skills) + skillsByName.TryAdd(skill.Name, skill); + + var agentsByName = new Dictionary(StringComparer.Ordinal); + foreach (var agent in agents) + agentsByName.TryAdd(agent.Name, agent); + foreach (var dependency in report.ExternalDependencies) { switch (dependency.Kind) { case "plugin": - if (plugins.FirstOrDefault(plugin => string.Equals(plugin.Name, dependency.Name, StringComparison.Ordinal)) is { } plugin) + if (pluginsByName.TryGetValue(dependency.Name, out var plugin)) plugin.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; case "skill": - if (skills.FirstOrDefault(skill => string.Equals(skill.Name, dependency.Name, StringComparison.Ordinal)) is { } skill) + if (skillsByName.TryGetValue(dependency.Name, out var skill)) skill.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; case "agent": - if (agents.FirstOrDefault(agent => string.Equals(agent.Name, dependency.Name, StringComparison.Ordinal)) is { } agent) + if (agentsByName.TryGetValue(dependency.Name, out var agent)) agent.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; } @@ -722,7 +743,7 @@ private static bool IsPathWithin(string candidatePath, string? containerPath) var fullCandidatePath = Path.GetFullPath(candidatePath); var fullContainerPath = Path.GetFullPath(containerPath); - if (string.Equals(fullCandidatePath, fullContainerPath, StringComparison.OrdinalIgnoreCase)) + if (string.Equals(fullCandidatePath, fullContainerPath, s_pathComparison)) return true; if (File.Exists(fullContainerPath)) @@ -731,7 +752,7 @@ private static bool IsPathWithin(string candidatePath, string? containerPath) var normalizedContainer = fullContainerPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + Path.DirectorySeparatorChar; - return fullCandidatePath.StartsWith(normalizedContainer, StringComparison.OrdinalIgnoreCase); + return fullCandidatePath.StartsWith(normalizedContainer, s_pathComparison); } private sealed class CheckReportBuilder(CheckConfig config, string scope) @@ -748,8 +769,6 @@ private sealed class CheckReportBuilder(CheckConfig config, string scope) public ReferenceScanReport? ReferenceScan { get; set; } - public void AddPlainError(string text) => GeneralErrors.Add(text); - public void AddGeneralError(string text) => GeneralErrors.Add(text); public CheckReport Build(int exitCode) diff --git a/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs b/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs new file mode 100644 index 0000000000..1423c77a95 --- /dev/null +++ b/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs @@ -0,0 +1,12 @@ +using System.Text.Json.Serialization; + +namespace SkillValidator; + +[JsonSourceGenerationOptions( + WriteIndented = true, + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = true, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + UseStringEnumConverter = true)] +[JsonSerializable(typeof(Check.CheckJsonOutput))] +internal partial class CheckJsonSerializerContext : JsonSerializerContext; diff --git a/eng/skill-validator/src/SkillValidatorJsonContext.cs b/eng/skill-validator/src/SkillValidatorJsonContext.cs index 2e4b9cd5e0..2aaae673bf 100644 --- a/eng/skill-validator/src/SkillValidatorJsonContext.cs +++ b/eng/skill-validator/src/SkillValidatorJsonContext.cs @@ -1,7 +1,6 @@ using System.Text.Json; using System.Text.Json.Nodes; using System.Text.Json.Serialization; -using SkillValidator.Check; using SkillValidator.Evaluate; using SkillValidator.Shared; @@ -11,8 +10,7 @@ namespace SkillValidator; WriteIndented = true, PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, PropertyNameCaseInsensitive = true, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - UseStringEnumConverter = true)] + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)] [JsonSerializable(typeof(ResultsOutput))] [JsonSerializable(typeof(ConsolidateData))] [JsonSerializable(typeof(SkillVerdict))] @@ -44,7 +42,6 @@ namespace SkillValidator; [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(string[]))] -[JsonSerializable(typeof(CheckJsonOutput))] [JsonSerializable(typeof(List))] [JsonSerializable(typeof(IReadOnlyList))] internal partial class SkillValidatorJsonContext : JsonSerializerContext; From 71994b231153d8c272a711f5092d1fe0592c91a5 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Thu, 11 Jun 2026 14:31:04 +1000 Subject: [PATCH 4/6] Address latest Copilot PR feedback - Attach external dependency warnings by stable path identifiers (skill SKILL.md path, agent path, plugin directory path) - Use OS-aware comparer for plugin directory path matching - Restore console warning formatting via SkillProfiler.FormatProfileWarnings - Move CheckJsonSerializerContext into SkillValidator.Check namespace - Precompute reference-attachment targets to avoid repeated full-path normalization and container recalculation per finding - Add regression test covering duplicate skill names with external dependency warnings in JSON output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/skill-validator/src/Check/CheckCommand.cs | 122 +++++++++++------- .../src/Check/CheckJsonSerializerContext.cs | 4 +- eng/skill-validator/src/Check/Models.cs | 1 + .../tests/Check/CheckCommandTests.cs | 50 ++++++- 4 files changed, 123 insertions(+), 54 deletions(-) diff --git a/eng/skill-validator/src/Check/CheckCommand.cs b/eng/skill-validator/src/Check/CheckCommand.cs index 4cea185573..b8be9da6de 100644 --- a/eng/skill-validator/src/Check/CheckCommand.cs +++ b/eng/skill-validator/src/Check/CheckCommand.cs @@ -10,6 +10,10 @@ public static class CheckCommand ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + private static readonly StringComparer s_pathComparer = OperatingSystem.IsWindows() + ? StringComparer.OrdinalIgnoreCase + : StringComparer.Ordinal; + public static Command Create() { var pluginOpt = new Option("--plugin") { Description = "Plugin directories to check (discovers skills, agents, plugin.json)", AllowMultipleArgumentsPerToken = true }; @@ -125,7 +129,7 @@ private static string DetermineScope(CheckConfig config) private static async Task RunPluginCheck(CheckConfig config, CheckReportBuilder builder) { var allPlugins = new List(); - var pluginSkills = new Dictionary>(StringComparer.Ordinal); + var pluginSkills = new Dictionary>(s_pathComparer); var allSkillsList = new List(); var agentDirs = new List(); @@ -219,7 +223,7 @@ private static async Task RunPluginCheck(CheckConfig config, CheckReportBui if (totalChars <= SkillProfiler.MaxAggregateDescriptionLength) continue; - var pluginResult = builder.Plugins.FirstOrDefault(p => string.Equals(p.DirectoryPath, pluginDirectoryPath, StringComparison.Ordinal)); + var pluginResult = builder.Plugins.FirstOrDefault(p => string.Equals(p.DirectoryPath, pluginDirectoryPath, s_pathComparison)); var pluginLabel = pluginResult?.Name ?? Path.GetFileName(pluginDirectoryPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); var message = $"Plugin '{pluginLabel}' aggregate description size is {totalChars:N0} characters — maximum is {SkillProfiler.MaxAggregateDescriptionLength:N0}."; @@ -422,19 +426,19 @@ private static void CheckExternalDeps(CheckReportBuilder builder, string? allowe foreach (var skill in skills) { foreach (var warning in ExternalDependencyChecker.CheckSkill(skill, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("skill", skill.Name, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult("skill", skill.Name, skill.SkillMdPath, warning)); } foreach (var agent in agents) { foreach (var warning in ExternalDependencyChecker.CheckAgent(agent, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("agent", agent.Name, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult("agent", agent.Name, agent.Path, warning)); } foreach (var plugin in plugins) { foreach (var warning in ExternalDependencyChecker.CheckPlugin(plugin, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("plugin", plugin.Name, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult("plugin", plugin.Name, plugin.DirectoryPath, warning)); } } @@ -531,37 +535,41 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) .ToList())) .ToList(); - var pluginsByName = new Dictionary(StringComparer.Ordinal); + var pluginsByPath = new Dictionary(s_pathComparer); foreach (var plugin in plugins) - pluginsByName.TryAdd(plugin.Name, plugin); + pluginsByPath.TryAdd(NormalizePathKey(plugin.DirectoryPath), plugin); - var skillsByName = new Dictionary(StringComparer.Ordinal); + var skillsByPath = new Dictionary(s_pathComparer); foreach (var skill in skills) - skillsByName.TryAdd(skill.Name, skill); + skillsByPath.TryAdd(NormalizePathKey(skill.SkillMdPath), skill); - var agentsByName = new Dictionary(StringComparer.Ordinal); + var agentsByPath = new Dictionary(s_pathComparer); foreach (var agent in agents) - agentsByName.TryAdd(agent.Name, agent); + agentsByPath.TryAdd(NormalizePathKey(agent.Path), agent); foreach (var dependency in report.ExternalDependencies) { + var dependencyPath = NormalizePathKey(dependency.TargetPath); + switch (dependency.Kind) { case "plugin": - if (pluginsByName.TryGetValue(dependency.Name, out var plugin)) + if (pluginsByPath.TryGetValue(dependencyPath, out var plugin)) plugin.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; case "skill": - if (skillsByName.TryGetValue(dependency.Name, out var skill)) + if (skillsByPath.TryGetValue(dependencyPath, out var skill)) skill.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; case "agent": - if (agentsByName.TryGetValue(dependency.Name, out var agent)) + if (agentsByPath.TryGetValue(dependencyPath, out var agent)) agent.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); break; } } + var referenceTargets = CreateReferenceTargets(skills, agents, plugins); + var topLevelErrors = report.GeneralErrors.ToList(); if (report.ReferenceScan is not null) @@ -569,7 +577,7 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) foreach (var finding in report.ReferenceScan.Findings) { var formattedFinding = $"{finding.Path}:{finding.LineNum} [{finding.Code}] {finding.Message}"; - if (TryAttachReferenceFinding(skills, agents, plugins, finding.Path, formattedFinding)) + if (TryAttachReferenceFinding(referenceTargets, finding.Path, formattedFinding)) continue; topLevelErrors.Add(formattedFinding); @@ -621,8 +629,12 @@ private static void RenderSkills(CheckReport report) foreach (var error in skill.Errors) Console.Error.WriteLine($"{Ansi.Red}❌ [{skill.Name}] {error}{Ansi.Reset}"); - foreach (var warning in skill.Warnings) - Console.WriteLine($"[{skill.Name}] ⚠ {warning}"); + var formattedWarnings = skill.Profile is null + ? skill.Warnings + : SkillProfiler.FormatProfileWarnings(skill.Profile); + + foreach (var warning in formattedWarnings) + Console.WriteLine($"[{skill.Name}] {warning}"); } if (report.Skills.Any(skill => skill.Errors.Count > 0)) @@ -698,63 +710,73 @@ private static string FormatSuccessSummary(CheckReport report) => _ => "All checks passed", }; - private static bool TryAttachReferenceFinding( + private static IReadOnlyList CreateReferenceTargets( IReadOnlyList skills, IReadOnlyList agents, - IReadOnlyList plugins, - string findingPath, - string formattedFinding) + IReadOnlyList plugins) { + var targets = new List(skills.Count * 2 + agents.Count * 2 + plugins.Count); + foreach (var skill in skills) { - if (IsPathWithin(findingPath, skill.Path) || IsPathWithin(findingPath, skill.SkillMdPath)) - { - skill.Errors.Add(formattedFinding); - return true; - } + TryAddReferenceTarget(targets, skill.Path, error => skill.Errors.Add(error)); + TryAddReferenceTarget(targets, skill.SkillMdPath, error => skill.Errors.Add(error)); } foreach (var agent in agents) { - if (IsPathWithin(findingPath, agent.Path) || IsPathWithin(findingPath, Path.GetDirectoryName(agent.Path))) - { - agent.Errors.Add(formattedFinding); - return true; - } + TryAddReferenceTarget(targets, agent.Path, error => agent.Errors.Add(error)); + TryAddReferenceTarget(targets, Path.GetDirectoryName(agent.Path), error => agent.Errors.Add(error)); } foreach (var plugin in plugins) - { - if (IsPathWithin(findingPath, plugin.DirectoryPath)) - { - plugin.Errors.Add(formattedFinding); - return true; - } - } + TryAddReferenceTarget(targets, plugin.DirectoryPath, error => plugin.Errors.Add(error)); - return false; + return targets; } - private static bool IsPathWithin(string candidatePath, string? containerPath) + private static void TryAddReferenceTarget(List targets, string? containerPath, Action addError) { if (string.IsNullOrWhiteSpace(containerPath)) - return false; + return; - var fullCandidatePath = Path.GetFullPath(candidatePath); - var fullContainerPath = Path.GetFullPath(containerPath); + var normalizedPath = NormalizePathKey(containerPath); + var normalizedDirectoryPath = File.Exists(normalizedPath) + ? Path.GetDirectoryName(normalizedPath) ?? normalizedPath + : normalizedPath; + var normalizedDirectoryPrefix = normalizedDirectoryPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; - if (string.Equals(fullCandidatePath, fullContainerPath, s_pathComparison)) - return true; + targets.Add(new JsonReferenceTarget(normalizedPath, normalizedDirectoryPrefix, addError)); + } - if (File.Exists(fullContainerPath)) - fullContainerPath = Path.GetDirectoryName(fullContainerPath)!; + private static bool TryAttachReferenceFinding( + IReadOnlyList targets, + string findingPath, + string formattedFinding) + { + var normalizedFindingPath = NormalizePathKey(findingPath); - var normalizedContainer = fullContainerPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) - + Path.DirectorySeparatorChar; + foreach (var target in targets) + { + if (string.Equals(normalizedFindingPath, target.ExactPath, s_pathComparison) + || normalizedFindingPath.StartsWith(target.DirectoryPrefix, s_pathComparison)) + { + target.AddError(formattedFinding); + return true; + } + } - return fullCandidatePath.StartsWith(normalizedContainer, s_pathComparison); + return false; } + private static string NormalizePathKey(string path) => Path.GetFullPath(path); + + private sealed record JsonReferenceTarget( + string ExactPath, + string DirectoryPrefix, + Action AddError); + private sealed class CheckReportBuilder(CheckConfig config, string scope) { public List GeneralErrors { get; } = []; diff --git a/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs b/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs index 1423c77a95..d26f4ef487 100644 --- a/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs +++ b/eng/skill-validator/src/Check/CheckJsonSerializerContext.cs @@ -1,6 +1,6 @@ using System.Text.Json.Serialization; -namespace SkillValidator; +namespace SkillValidator.Check; [JsonSourceGenerationOptions( WriteIndented = true, @@ -8,5 +8,5 @@ namespace SkillValidator; PropertyNameCaseInsensitive = true, DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, UseStringEnumConverter = true)] -[JsonSerializable(typeof(Check.CheckJsonOutput))] +[JsonSerializable(typeof(CheckJsonOutput))] internal partial class CheckJsonSerializerContext : JsonSerializerContext; diff --git a/eng/skill-validator/src/Check/Models.cs b/eng/skill-validator/src/Check/Models.cs index 50d786ba58..8f75e491d3 100644 --- a/eng/skill-validator/src/Check/Models.cs +++ b/eng/skill-validator/src/Check/Models.cs @@ -37,6 +37,7 @@ public sealed class AgentCheckResult public sealed record ExternalDependencyResult( string Kind, string Name, + string TargetPath, string Message); public sealed record CheckInvocation( diff --git a/eng/skill-validator/tests/Check/CheckCommandTests.cs b/eng/skill-validator/tests/Check/CheckCommandTests.cs index 2c1bab3a9e..bf5cfaa407 100644 --- a/eng/skill-validator/tests/Check/CheckCommandTests.cs +++ b/eng/skill-validator/tests/Check/CheckCommandTests.cs @@ -371,13 +371,13 @@ public async Task CombinedSkillsAndAgents_WithNoDiscoveredAgents_Fails() [Collection("CheckCommandConsole")] public class CheckCommandJsonOutputTests { - private static string CreateSkillFixture(string skillName, string description) + private static string CreateSkillFixture(string skillName, string description, string body = "Content.") { var root = Path.Combine(Path.GetTempPath(), $"json-test-{Guid.NewGuid():N}"); var skillDir = Path.Combine(root, skillName); Directory.CreateDirectory(skillDir); File.WriteAllText(Path.Combine(skillDir, "SKILL.md"), - $"---\nname: {skillName}\ndescription: {description}\n---\n# {skillName}\n\nContent.\n"); + $"---\nname: {skillName}\ndescription: {description}\n---\n# {skillName}\n\n{body}\n"); return root; } @@ -467,6 +467,52 @@ public async Task JsonOutput_WithMissingKnownDomains_WritesReferenceFailureToStd Directory.Delete(root, true); } } + + [Fact] + public async Task JsonOutput_WithDuplicateSkillNames_AttachesExternalDependencyWarningsByPath() + { + var rootOne = CreateSkillFixture("shared-skill", "First description.", "#tool:custom/tool"); + var rootTwo = CreateSkillFixture("shared-skill", "Second description.", "#tool:custom/tool"); + + var allowListPath = Path.Combine(Path.GetTempPath(), $"allowlist-{Guid.NewGuid():N}.txt"); + + try + { + var capture = await ConsoleCapture.RunAsync(() => CheckCommand.Run(new CheckConfig + { + SkillPaths = [Path.Combine(rootOne, "shared-skill"), Path.Combine(rootTwo, "shared-skill")], + AllowedExternalDepsFile = allowListPath, + OutputMode = CheckOutputMode.Json, + })); + + Assert.Equal(1, capture.ExitCode); + Assert.Equal("", capture.StandardError); + + using var document = JsonDocument.Parse(capture.StandardOutput); + var report = document.RootElement; + var skills = report.GetProperty("skills").EnumerateArray().ToList(); + + Assert.Equal(2, skills.Count); + + foreach (var skill in skills) + { + var warningKinds = skill.GetProperty("warnings") + .EnumerateArray() + .Select(warning => warning.GetProperty("kind").GetString()) + .ToList(); + + Assert.Contains("externalDependency", warningKinds); + } + } + finally + { + if (File.Exists(allowListPath)) + File.Delete(allowListPath); + + Directory.Delete(rootOne, true); + Directory.Delete(rootTwo, true); + } + } } public sealed record ConsoleCaptureResult( From a9c40bb94b33bd78b88cdeed27afcd566ba6ba47 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Thu, 11 Jun 2026 14:44:02 +1000 Subject: [PATCH 5/6] Fix flaky duplicate-skill JSON test path - Run duplicate-name external dependency assertion through plugin mode, which is where external dependency checks are executed - Add plugin fixture helper for JSON output tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../tests/Check/CheckCommandTests.cs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/eng/skill-validator/tests/Check/CheckCommandTests.cs b/eng/skill-validator/tests/Check/CheckCommandTests.cs index bf5cfaa407..13f6b6dec2 100644 --- a/eng/skill-validator/tests/Check/CheckCommandTests.cs +++ b/eng/skill-validator/tests/Check/CheckCommandTests.cs @@ -381,6 +381,24 @@ private static string CreateSkillFixture(string skillName, string description, s return root; } + private static string CreatePluginFixture(string pluginName, string skillName, string description, string body = "Content.") + { + var root = Path.Combine(Path.GetTempPath(), $"json-plugin-test-{Guid.NewGuid():N}"); + var pluginDir = Path.Combine(root, pluginName); + var skillsDir = Path.Combine(pluginDir, "skills"); + var skillDir = Path.Combine(skillsDir, skillName); + + Directory.CreateDirectory(skillDir); + + File.WriteAllText(Path.Combine(pluginDir, "plugin.json"), + $$"""{"name":"{{pluginName}}","version":"1.0.0","description":"Test plugin.","skills":"./skills/"}"""); + + File.WriteAllText(Path.Combine(skillDir, "SKILL.md"), + $"---\nname: {skillName}\ndescription: {description}\n---\n# {skillName}\n\n{body}\n"); + + return root; + } + [Fact] public async Task JsonOutput_WithSkills_WritesStructuredReportToStdout() { @@ -471,8 +489,8 @@ public async Task JsonOutput_WithMissingKnownDomains_WritesReferenceFailureToStd [Fact] public async Task JsonOutput_WithDuplicateSkillNames_AttachesExternalDependencyWarningsByPath() { - var rootOne = CreateSkillFixture("shared-skill", "First description.", "#tool:custom/tool"); - var rootTwo = CreateSkillFixture("shared-skill", "Second description.", "#tool:custom/tool"); + var rootOne = CreatePluginFixture("plugin-one", "shared-skill", "First description.", "#tool:custom/tool"); + var rootTwo = CreatePluginFixture("plugin-two", "shared-skill", "Second description.", "#tool:custom/tool"); var allowListPath = Path.Combine(Path.GetTempPath(), $"allowlist-{Guid.NewGuid():N}.txt"); @@ -480,7 +498,7 @@ public async Task JsonOutput_WithDuplicateSkillNames_AttachesExternalDependencyW { var capture = await ConsoleCapture.RunAsync(() => CheckCommand.Run(new CheckConfig { - SkillPaths = [Path.Combine(rootOne, "shared-skill"), Path.Combine(rootTwo, "shared-skill")], + PluginPaths = [Path.Combine(rootOne, "plugin-one"), Path.Combine(rootTwo, "plugin-two")], AllowedExternalDepsFile = allowListPath, OutputMode = CheckOutputMode.Json, })); From 7196dfd2a27b126351666e606f5b21db79ab5cc4 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Thu, 11 Jun 2026 15:16:51 +1000 Subject: [PATCH 6/6] Address latest Copilot follow-up feedback - Reintroduce PluginValidationResult as obsolete compatibility type while keeping PluginCheckResult as the primary model - Avoid unnecessary profile-line formatting when verbose output is off - Replace string discriminators for external dependency kind with enum - Centralize JSON warning kind literals as constants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/skill-validator/src/Check/CheckCommand.cs | 27 +++++++++---------- eng/skill-validator/src/Check/Models.cs | 22 ++++++++++++++- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/eng/skill-validator/src/Check/CheckCommand.cs b/eng/skill-validator/src/Check/CheckCommand.cs index b8be9da6de..2dd32ac4e8 100644 --- a/eng/skill-validator/src/Check/CheckCommand.cs +++ b/eng/skill-validator/src/Check/CheckCommand.cs @@ -395,14 +395,13 @@ private static bool ValidateSkillProfiles(CheckReportBuilder builder, IReadOnlyL foreach (var skill in skills) { var profile = SkillProfiler.AnalyzeSkill(skill, checkOptions); - var profileLine = SkillProfiler.FormatProfileLine(profile); var result = new SkillCheckResult { Name = skill.Name, Path = skill.Path, SkillMdPath = skill.SkillMdPath, Profile = profile, - ProfileLine = verbose ? profileLine : null, + ProfileLine = verbose ? SkillProfiler.FormatProfileLine(profile) : null, }; result.Errors.AddRange(profile.Errors); @@ -426,19 +425,19 @@ private static void CheckExternalDeps(CheckReportBuilder builder, string? allowe foreach (var skill in skills) { foreach (var warning in ExternalDependencyChecker.CheckSkill(skill, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("skill", skill.Name, skill.SkillMdPath, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult(ExternalDependencyKind.Skill, skill.Name, skill.SkillMdPath, warning)); } foreach (var agent in agents) { foreach (var warning in ExternalDependencyChecker.CheckAgent(agent, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("agent", agent.Name, agent.Path, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult(ExternalDependencyKind.Agent, agent.Name, agent.Path, warning)); } foreach (var plugin in plugins) { foreach (var warning in ExternalDependencyChecker.CheckPlugin(plugin, allowed)) - builder.ExternalDependencies.Add(new ExternalDependencyResult("plugin", plugin.Name, plugin.DirectoryPath, warning)); + builder.ExternalDependencies.Add(new ExternalDependencyResult(ExternalDependencyKind.Plugin, plugin.Name, plugin.DirectoryPath, warning)); } } @@ -496,7 +495,7 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) DirectoryPath: source.DirectoryPath, Errors: [.. source.Errors], Warnings: source.Warnings - .Select(warning => new CheckJsonWarning("validation", warning)) + .Select(warning => new CheckJsonWarning(CheckJsonWarningKinds.Validation, warning)) .ToList())) .ToList(); @@ -507,7 +506,7 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) SkillMdPath: source.SkillMdPath, Errors: [.. source.Errors], Warnings: source.Warnings - .Select(warning => new CheckJsonWarning("profile", warning)) + .Select(warning => new CheckJsonWarning(CheckJsonWarningKinds.Profile, warning)) .ToList(), Profile: source.Profile is null ? null : new CheckJsonSkillProfile( Name: source.Profile.Name, @@ -531,7 +530,7 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) Path: source.Path, Errors: [.. source.Errors], Warnings: source.Warnings - .Select(warning => new CheckJsonWarning("validation", warning)) + .Select(warning => new CheckJsonWarning(CheckJsonWarningKinds.Validation, warning)) .ToList())) .ToList(); @@ -553,17 +552,17 @@ private static CheckJsonOutput CreateJsonOutput(CheckReport report) switch (dependency.Kind) { - case "plugin": + case ExternalDependencyKind.Plugin: if (pluginsByPath.TryGetValue(dependencyPath, out var plugin)) - plugin.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + plugin.Warnings.Add(new CheckJsonWarning(CheckJsonWarningKinds.ExternalDependency, dependency.Message)); break; - case "skill": + case ExternalDependencyKind.Skill: if (skillsByPath.TryGetValue(dependencyPath, out var skill)) - skill.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + skill.Warnings.Add(new CheckJsonWarning(CheckJsonWarningKinds.ExternalDependency, dependency.Message)); break; - case "agent": + case ExternalDependencyKind.Agent: if (agentsByPath.TryGetValue(dependencyPath, out var agent)) - agent.Warnings.Add(new CheckJsonWarning("externalDependency", dependency.Message)); + agent.Warnings.Add(new CheckJsonWarning(CheckJsonWarningKinds.ExternalDependency, dependency.Message)); break; } } diff --git a/eng/skill-validator/src/Check/Models.cs b/eng/skill-validator/src/Check/Models.cs index 8f75e491d3..3beb6db8c7 100644 --- a/eng/skill-validator/src/Check/Models.cs +++ b/eng/skill-validator/src/Check/Models.cs @@ -14,6 +14,12 @@ public sealed class PluginCheckResult public List Warnings { get; } = []; } +[Obsolete("Use PluginCheckResult.")] +public sealed record PluginValidationResult( + string Name, + IReadOnlyList Errors, + IReadOnlyList Warnings); + public sealed class SkillCheckResult { public string Name { get; init; } = ""; @@ -34,8 +40,15 @@ public sealed class AgentCheckResult public List Warnings { get; } = []; } +public enum ExternalDependencyKind +{ + Plugin, + Skill, + Agent, +} + public sealed record ExternalDependencyResult( - string Kind, + ExternalDependencyKind Kind, string Name, string TargetPath, string Message); @@ -64,6 +77,13 @@ public sealed record CheckJsonCounts( int SkillCount, int AgentCount); +public static class CheckJsonWarningKinds +{ + public const string Validation = "validation"; + public const string Profile = "profile"; + public const string ExternalDependency = "externalDependency"; +} + public sealed record CheckJsonWarning( string Kind, string Message);