PR #473 moved the failure investigation prompt out of the PR comment and
only into the workflow run summary. This made it harder for users to
discover — they had to click through to the run details to find it.
Move it back to the PR comment so the copy-paste prompt is visible
inline on the PR, while keeping it in the workflow summary as well.
During multi-model skill-validator evals, judge calls intermittently
fail with:
`Failed to persist session events: Error: Failed to append to JSONL`
`file session-state\events.jsonl: Error: timeout while waiting for`
`mutex to become available`
The error wording (the word `mutex`) does not originate in our C# code
and is reported by the GitHub Copilot SDK over RPC, so the underlying
contention lives inside the SDK's session event-persistence layer.
AgentRunner already gives each session its own `configDir` and
LlmSession (judge) gives each call a unique temp root, so paths cannot
collide ACROSS sessions — this points to intra-session contention on a
single events.jsonl path.
Mitigation: serialize concurrent writes per resolved path inside our
handler with a `ConcurrentDictionary<string, SemaphoreSlim>` keyed by
the absolute path. Both `WriteFileAsync` and `AppendFileAsync` route
through a per-path lock helper. Reads remain unlocked.
Even though the conflict surfaces inside the SDK, removing one variable
(our handler issuing concurrent writes to the same file) is good defense
and makes the `ISessionFsHandler` thread-safe in its own right. The
dictionary is bounded by the number of unique files written within one
session (typically <10), so leak risk is negligible for the per-session
object lifetime.
Validation: build passes; full xUnit suite 545/545 tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* skill-validator: bump per-plugin aggregate description cap from 15K to 20K
The dotnet-test plugin (22 skills) is now at ~14K aggregate description
characters and bumping into the 15,000 cap on every meaningful description
change. This blocks legitimate plugin growth on what is, on inspection, an
informal repo guardrail rather than a documented Copilot constraint.
Origin of the 15K number
Traced via 'git log -S MaxAggregateDescriptionLength' to PR #238,
which references issue #222. The issue verbatim says:
`users may apply a limit to the total size of 'description' across`
`all skills in a plugin (...). 15K characters was mentioned, we`
`could choose smaller.`
So the value is informal — picked because it `was mentioned` and
explicitly leaving room to `choose smaller`.
Spec status
The agentskills.io specification (https://agentskills.io/specification)
documents per-skill description (1024), compatibility (500), and name
(64) limits. It does NOT define any aggregate per-plugin cap. The
`MaxDescriptionLength = 1024` constant in this same file already cites
the spec; the aggregate constant carried no citation comment.
What this PR does
- Bumps `MaxAggregateDescriptionLength` from 15,000 to 20,000. Picks a
round value that gives ~30% headroom over the largest current plugin
(dotnet-test ~14K) without making the cap meaningless.
- Adds a comment documenting the origin (informal, issue #222),
the absence of a spec source, and a TODO to validate this guardrail
against literature on skill-routing degradation and to run experiments
(skill-validator already has the infrastructure to A/B-measure
description noise vs activation accuracy).
What this PR does NOT do
- It does not remove the cap. The cost concern in #222 is real (every
description is loaded at agent startup for routing); we just don't yet
have evidence for the right ceiling.
- It does not change per-skill limits.
Validation
All 545 tests pass; the existing `CheckCommandAggregateDescriptionTests`
reference the constant symbolically and continue to pass with the new
value.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Pin Nerdbank.MessagePack to 1.1.62 and add spec URL to cap comment
- Transitively pin Nerdbank.MessagePack from 1.0.2 (vulnerable via StreamJsonRpc 2.24.84 -> GitHub.Copilot.SDK) to 1.1.62, which contains the fix for the stackalloc DateTime vulnerability (GHSA-2cwq-pwfr-wcw3). Without this pin, restore fails with NU1903 since TreatWarningsAsErrors is true.
- Add the agentskills.io specification URL to the MaxAggregateDescriptionLength comment so future readers can verify the claim that the per-plugin aggregate cap is a local repo policy rather than a spec-mandated limit (addresses PR #655 review feedback).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
/-prefix paths bypassed the flag and hit the depth check unconditionally. Now treated symmetrically with .. traversal: allowed only with the flag, otherwise produce a clear absolute (repo-rooted) path error. Also classify // as a protocol-relative URL.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add validator options
* Address PR review: validate options and add tests
- Reject --allow-repo-traversal with --plugin mode (plugins must be portable)
- Validate --max-declaration-lines and --max-agent-lines are positive integers
- Add unit tests for MaxDeclarationLines override in SkillProfiler
- Add unit tests for AllowRepoTraversal suppression and depth check preservation
- Add unit tests for MaxAgentLines override and fallback to MaxDeclarationLines
* Remove body line count checks and --max-declaration-lines/--max-agent-lines options
Line count checks are redundant with BPE token-based complexity classification,
which already provides better guidance. Keep --allow-repo-traversal only.
* Address review: remove unused CheckOptions from AgentProfiler, fix traversal test
- Remove unused CheckOptions parameter from AgentProfiler.AnalyzeAgent and
RunAgentsCheckCore since line checks were removed
- Fix AllowRepoTraversalSuppressesParentTraversalError test to use a non-deep
path (../SKILL.md) and assert no file-ref errors at all
* docs: document token count reporting in skill-validator
Add a 'Getting the token count for a skill' section to the README
explaining how to use 'check --verbose' to see BPE token counts and
complexity tiers.
Expand the Run metrics table in InvestigatingResults.md with the
inputTokens, outputTokens, cache, and judge token fields available
in results.json. Update the example analysis script to print
input/output tokens.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: address review feedback on token count documentation
- Fix tier table: replace 'Optimal sweet spot' with 'Recommended tier (sweet spot: 800–2,500)' to align with earlier statement
- Fix tokenEstimate description to accurately reflect implementation behavior (cache fallback when inputTokens is zero)
Agent-Logs-Url: https://github.com/dotnet/skills/sessions/dd5ded07-a968-4574-bfe9-1602b4434f07
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* Skill-validator: perf, cancellation, and reliability improvements
Performance:
- Cache skill-validator-dist.tar.gz in CI (evaluation.yml and skill-check.yml)
to skip .NET SDK setup, build, and tar on cache hit. Evaluate and comment-on-pr
jobs restore from cache with artifact download fallback.
- Replace SemaphoreSlim with Lock in SessionDatabase for lighter-weight
synchronous locking.
Cancellation:
- Thread CancellationToken from System.CommandLine entry point through the
entire evaluate call chain: Run -> EvaluateTarget -> EvaluateSkill/EvaluateAgent
-> ExecuteScenario/ExecuteAgentScenario -> ExecuteRun/ExecuteAgentRun, plus
EvaluateSkillNoise -> ExecuteNoiseTest.
- Add CancellationToken parameter to AgentRunner.RunAgent, Judge.JudgeRun,
PairwiseJudge.Judge, and OverfittingJudge.Analyze, forwarding to
RetryHelper and LlmSession.SendAsync respectively.
- Pass CancellationToken to all ConcurrencyLimiter.RunAsync call sites so
semaphore waits are cancellable.
- Re-throw OperationCanceledException at the top-level skill evaluation loop
so Ctrl+C / CI cancellation propagates instead of being swallowed as a
skill failure.
Reliability:
- Smart timeline truncation in Judge and PairwiseJudge: error events
(session.error, runner.error) are always preserved during truncation
regardless of position, with a summary marker showing event type breakdown.
- Add jitter (±25%) to RetryHelper exponential backoff to prevent thundering
herd on rate-limit recovery.
- Add CancellationToken parameter to ConcurrencyLimiter.RunAsync<T>.
- Add ConcurrencyLimiter tests (6 tests) and RetryHelper jitter tests (2 tests).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR review feedback
- RetryHelper: re-clamp delay after jitter so it never exceeds the
remaining budget or MaxSingleDelayMs.
- Judge/PairwiseJudge: handle edge case where error count alone exceeds
MaxTimelineEvents by trimming errors with a summary instead of
unconditionally appending all of them.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add high-variance detection and refactor FailureKind to enum
- Add Statistics.CoefficientOfVariation() for measuring score consistency
- Flag scenarios with CV > 0.5 as high-variance in ScenarioComparison
- Show console warning and markdown footnote for high-variance scenarios
- Include high-variance scenario names in Comparator verdict reason
- Refactor FailureKind from magic strings to strongly-typed enum with
JsonStringEnumMemberName attributes for backwards-compatible JSON output
- Add 6 new CV unit tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Move skill-validator docs under src/ and include in packages
Move README.md to src/ and InvestigatingResults.md + OverfittingDetection.md
to src/docs/ so they ship in the NuGet package and .tar.gz archives.
- Include docs/*.md in NuGet pack and publish output via CopyToPublishDirectory
- Add LICENSE to publish output (tar.gz only, not NuGet — uses PackageLicenseExpression)
- Replace enumerated docOnlyFiles lists with a regex pattern in CI workflows
- Deduplicate eval.yaml docs between CONTRIBUTING.md and the shipping README
- Add test fixture files walkthrough to README (moved from CONTRIBUTING.md)
- Add AGENTS.md guidance: src/ is the packaging boundary
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Update link format in failure diagnosis message
* Update AGENTS.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Add evaluation troubleshooting guide and link from PR comments
Add InvestigatingResults.md with:
- How to find and download evaluation artifacts from workflow runs
- Full results.json schema documentation
- Common failure patterns (timeouts, empty output, variance, rubric
issues, skill not activated) with diagnosis steps and fixes
- Example analysis script for AI agents to parse results
Link the doc from:
- Reporter.cs: show a link in the PR comment when any skill fails
- skill-validator README.md: Output section
- CONTRIBUTING.md: CI evaluation section
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Update InvestigatingResults.md
* Address review feedback: fix hard-coded URLs and optional plugin run docs
- Reporter.cs: Use repo-relative link instead of hard-coded GitHub URL
- evaluation.yml: Use github.sha instead of hard-coded 'main' branch
- InvestigatingResults.md: Clarify skilledPlugin run is optional
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* THROWAWAY: add trailing newline to trigger eval workflow
This change is intentionally trivial and will be reverted before merge.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR #441 review feedback (round 2)
- Reporter.cs: Remove leading / from relative link
- evaluation.yml: Use jq on results.json instead of grepping for emoji
- evaluation.yml: Use needs.gate.outputs.head_sha for revision-pinned URL
- InvestigatingResults.md: Mark pluginImprovementScore/pluginBreakdown as optional
- InvestigatingResults.md: Fix skill-not-activated symptoms to use
skillActivation fields instead of turnCount
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Use specific skillActivation field names and fix timestamp description
- Use skillActivationIsolated/skillActivationPlugin instead of generic skillActivation
- Clarify timestamp is when results were written, not run start
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix timestamp field description (was lost in prior commit)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Revert "THROWAWAY: add trailing newline to trigger eval workflow"
This reverts commit 83bd7f7601.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Show subagents invocations
* Address PR feedback: fix XML doc and sanitize agent names in markdown
- Update ExtractSubagentActivation XML doc to accurately describe that
all subagent event types are counted (not just Started/Completed)
- Add markdown table cell sanitization for agent names in FormatSubagentCell
to prevent injection via pipe chars or newlines in plugin-controlled content
- Extract SanitizeMarkdownTableCell helper for reuse
Reorganize the skill-validator project from layer-based grouping
(Commands/, Services/, Models/, Utilities/) to feature-based slices
(Check/, Evaluate/, Consolidate/, Shared/).
Key changes:
- Check/: CheckCommand, SkillProfiler, AgentProfiler, PluginValidator,
ExternalDependencyChecker, ReferenceScanner
- Evaluate/: EvaluateCommand, RejudgeCommand (now a subcommand of
evaluate), AgentRunner, Judge, PairwiseJudge, and other eval services
- Consolidate/: ConsolidateCommand
- Shared/: Models, SkillDiscovery, PluginParser (extracted from
PluginValidator), Reporter, and utilities
Split PluginValidator: parsing helpers (ParsePluginJson,
TryGetSafeSubdirectory) moved to Shared/PluginParser.cs; validation
logic stays in Check/PluginValidator.cs.
Move RawFrontmatter and RawAgentFrontmatter types from EvalSchema into
SkillDiscovery since they are only consumed by discovery.
Wire RejudgeCommand as subcommand: skill-validator evaluate rejudge.
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Integrate reference scanner into skill-validator CheckCommand
- Port reference scanner logic from scan.ps1 to C# ReferenceScanner service
- Add --known-domains option to CheckCommand for reference scanning
- Move known-domains.txt and allowed-external-deps.txt to eng/
- Remove standalone scan.ps1 and reference-validation.yml workflow
- Reference scanning now runs as part of skill-check.yml via skill-validator
- Add 34 unit tests for ReferenceScanner (all 463 tests pass)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR feedback: repoRoot derivation, early validation, .agents scanning, SRI policy
- Derive repoRoot from --known-domains file path instead of cwd
- Fail early with clear error when known-domains file is missing
- Add .agents/**/*.md scanning to DiscoverFiles (regression fix)
- Fix SRI policy loophole: SRI suppresses SCRIPT-NO-SRI only, domain
and HTTPS checks still apply
- Use actual known-domains file path in error messages instead of
hardcoded eng/known-domains.txt
- Update doc comments to reflect full set of scanned paths
- Add tests for .agents discovery and SRI domain enforcement
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Remove repo-specific paths from ReferenceScanner
DiscoverFiles now only scans directories it is given -- no hardcoded
knowledge of .agents, agentic-workflows, eng, or README.md. The tool
is repo-agnostic; callers control what gets scanned via --plugin dirs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Drop repoRoot from ReferenceScanner API
ScanFile/ScanFiles no longer take a repoRoot parameter. Finding paths
use the file path as provided by the caller, keeping the scanner
completely decoupled from any repo layout assumptions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Prune stale domains and add json.schemastore.org to known-domains.txt
Removed 10 entries no longer referenced by any plugin content:
agentskills.io, visualstudio.com, mcr.microsoft.com, dotnet.github.io,
github.com/dotnet/docs, github.com/dotnet/dotnet-api-docs,
github.com/dotnet/skills, github.com/github/gh-aw,
github.com/modelcontextprotocol/csharp-sdk, developers.openai.com.
Added json.schemastore.org (referenced by dotnet-template-engine).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix case-insensitive regex, update docs, add tests
- Make UrlRegex and PipeToShellRegex case-insensitive to match the
original PowerShell behavior (HTTP://, CURL, Wget all now detected)
- Update DiscoverFiles doc comment to remove stale 'individual files' claim
- Narrow CONTRIBUTING.md scope description to match actual plugin-only scanning
- Add tests for mixed/upper-case URL and pipe-to-shell detection
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Remove IPv6 loopback support, fix pipe-to-shell allowlist case sensitivity
- Remove Ipv6LoopbackRegex and IPv6 test data (not needed)
- Add case-insensitive + culture-invariant matching for pipe-to-shell allowlist
- Add mixed-case allowlist test
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix agents field in plugin.json to use explicit file paths
Claude Code's manifest validator rejects directory paths in the agents
field. Replace directory references with explicit .agent.md file paths
in all affected plugins (dotnet-diag, dotnet-msbuild, dotnet-template-engine).
Fixes https://github.com/dotnet/skills/issues/416
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Validate that agents paths are explicit files, not directories
Claude Code's manifest validator rejects directory paths in the agents
field. Update PluginValidator to error when an agent path resolves to a
directory, guiding authors to use explicit .agent.md file paths instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add 'run_command_and_assert' assertion
* Rename for clarity
* Add tests
* Fix race condition in command output handling
* Fix async output race condition in RunCommandAndAssert
WaitForExit(TimeSpan) does not guarantee that async output event
handlers have completed. Add parameterless WaitForExit() call after
the timed wait to ensure stdout/stderr are fully captured before
checking their contents. Also handle the timeout case explicitly
by killing the process.
* Address PR review: extract CommandAssertionArgs, add regex matching, configurable timeout
- Extract command-specific fields from Assertion into a dedicated
CommandAssertionArgs record type for cleaner separation of concerns
- Add ExpectedStdOutMatches/ExpectedStdErrorMatches for regex-based
stdout/stderr assertions (matches type asserts)
- Make command timeout configurable via assertion-level 'command_timeout'
field, falling back to the scenario timeout when not specified
- Show actual timeout value in timeout error messages
- Update EvalSchema parsing, EvaluateCommand call sites, and all tests
- Add new tests for regex matching and timeout configuration
---------
Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com>
* Initial plan
* Remove agents string field from plugin.json and handle array format in parser
Claude Code CLI validates `agents` as array-of-strings per its plugin manifest
schema. Our plugin.json files had `"agents": "./agents/"` (a string), which
caused "agents: Invalid input" validation errors on install.
Remove the field from the 3 affected plugin.json files — agents are still
discovered by convention from the `agents/` directory. Also make ParsePluginJson
resilient to both string and array formats for forward-compatibility.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Remove `agents` string field from plugin.json for Claude Code CLI compatibility
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Change agents field to array format in plugin.json, update validator to read array-first with string fallback
Plugin.json files now declare agents as an array of file paths:
"agents": ["./agents/foo.agent.md", "./agents/bar.agent.md"]
ParsePluginJson reads the array first (preferred, Claude Code schema),
falls back to a string path (legacy). PluginInfo gains an AgentPaths
field. Validator and discovery use the array when present, else fall
back to directory-based convention.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Simplify agents to directory-path array form ["./agents/"], handle directories in array entries
Per Copilot CLI spec, agents field values are "Path(s) to agent directories"
— no need to list individual files. Use ["./agents/"] to mirror how
skills uses "./skills/". Updated validator and discovery to handle both
directory and file entries in the array.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Delete .nuget/nuget.exe
* Apply suggestions from code review
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Convert skills field to array format in plugin.json, update validator to handle both forms
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Align AgentPaths null checks to use { Count: > 0 } pattern for consistency
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Change `agents` and `skills` fields in plugin.json to array format for Claude Code CLI compatibility
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Delete .nuget/nuget.exe
* Consolidate SkillsPath/SkillPaths and AgentsPath/AgentPaths into single array fields
Remove dual string/array fields from PluginInfo. The parser now
normalizes legacy string values into single-element arrays, so all
downstream code uses only SkillPaths and AgentPaths (IReadOnlyList<string>).
This eliminates all array-vs-string fallback branching.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Add proper skills isolation (copying and allowlisting)
* Address PR review: harden isolation, skip symlinks, precompute allowlist
- Pass null skillPath in isolated mode so the original skill tree
(including siblings) is not accessible; only staged copies are allowed.
- Harden CopyDirectory to skip symlinks/reparse points and verify that
recursed paths stay within the source root.
- Precompute additionalAllowedDirs once at session creation instead of
allocating on every permission request.
- Add IsolatedStagingDoesNotExposeOriginalOrSiblingSkills test that
verifies both directory-level and permission-level isolation.
* Fix unit test
* Make evaluate a subcommand with tests-dir as required argument
Change EvaluateCommand from RootCommand to a regular Command('evaluate')
so it is a subcommand like check, consolidate, and rejudge. Convert
--tests-dir from an optional Option to a required positional Argument
placed after the skill paths.
Update .csproj RunArguments, evaluation-run workflow, README, and
CONTRIBUTING.md to reflect the new CLI syntax.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Default --parallel-skills/scenarios/runs to 3
Update the default from 1 to 3 for all three parallelism options and
remove the now-redundant overrides from RunArguments in the .csproj.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Replace RunArguments with launchSettings.json, publish in skill-check
Add Properties/launchSettings.json with default evaluate args for local
development. Remove RunArguments from .csproj. Update skill-check.yml to
publish the validator and invoke the binary directly instead of using
dotnet run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Keep --tests-dir as a required option, not positional
A variadic paths argument greedily consumes all positional values,
making a trailing positional tests-dir impossible to parse. Keep
--tests-dir as a named required option which matches the previous UX.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Move testsDirOpt after resultsDirOpt, drop evaluate from CONTRIBUTING examples
Reorder testsDirOpt declaration and registration to follow resultsDirOpt.
Remove the evaluate subcommand from CONTRIBUTING.md dotnet run examples
since the launch profile already provides it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Refactor skill-validator: add check command, rename validate to evaluate
- Add new 'check' command for CI skill validation (frontmatter, structure)
- Rename ValidateCommand to EvaluateCommand to clarify its purpose
- Add skill-check.yml GitHub Actions workflow
- Refactor SkillDiscovery for improved skill detection
- Simplify OverfittingJudge and SkillProfiler
- Update tests to match refactored APIs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix check subcommand: use EvaluateCommand as root command
System.CommandLine 2.0.4 requires the primary command to be the
RootCommand. Restores the upstream pattern where the evaluate/validate
command is the root and check/consolidate/rejudge are subcommands.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Pass all plugins in single check invocation
Build plugin args dynamically and pass them all at once instead of
looping per plugin.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR review comments
- Remove 'evaluate' subcommand token from README, evaluation-run.yml,
and RunArguments in csproj (evaluate is now the root command)
- Wrap ParsePluginJson in try/catch for JsonException with clean error
- Add CheckCommandTests with aggregate description limit coverage
(under/at/over limit and multi-plugin cases)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add --keep-sessions flag and rejudge command
- Add --keep-sessions flag to preserve agent session data in SQLite DB
- Create SessionDatabase service with WAL mode and thread-safe writes
- Update AgentRunner to separate config/work dir lifecycle
- Track session metadata, RunMetrics, judge and pairwise results
- Add rejudge command to re-run judges on saved sessions
- Add 8 tests for SessionDatabase (CRUD, concurrency, isolation)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add prompt and skill_content columns to sessions.db
Store the eval scenario prompt and SKILL.md content in the sessions
table so they are available for browsing and replay without needing
the original skill directory.
Both columns are optional (nullable) for backward compatibility.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Replace skill_content with skill_sha for change detection
Compute a SHA-256 hash (truncated to 12 hex chars) over all files in
the skill directory, sorted by relative path for determinism. This
gives lightweight change detection without storing potentially large
skill content in the database.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR feedback: relative config_dir, schema_info table
- Use session ID as config dir folder name (sessions/<id>) so the
DB record links directly to its files on disk
- Store config_dir as relative path for portable results directories
- Add schema_info table with type='skill-validator' and version='1'
for easy DB detection by external tools like dotnet-replay
- Add GetSchemaInfo() method for consumers
- Extract GetSessions(whereClause) for reuse
- Add tests for schema_info and relative config_dir
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix: share timestamped results dir between sessions and reporters
When --keep-sessions is active, pass the already-created timestamped
directory to Reporter.ReportResults so sessions.db, session files, and
report outputs (results.json, summary.md) all live in the same folder
instead of creating two separate timestamped directories.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Adapt rejudge for current validator model
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR #149 review feedback
Persist the judge model for rejudge, use fresh judge workdirs, tighten session DB hashing/foreign key behavior, and cache skill SHA calculation during validation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix rejudge rubric persistence
Persist scenario rubrics in sessions.db so rejudge can reconstruct the original EvalScenario rubric, self-migrate older databases to the new schema, and validate --judge-mode inputs explicitly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Guard SessionDatabase reads with semaphore
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix validator session cleanup and reporting
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix eval source file resolution to use eval directory
The SetupWorkDir method resolved setup file 'source:' paths relative to the
skill directory (plugins/.../skills/<name>/), but eval.yaml files live under
tests/. The ../../../../ relative paths resolved correctly but were rejected
by the path traversal security check because the resolved path escaped the
skill directory boundary.
Fix by preferring the eval directory as the base for source resolution when
evalPath is available, falling back to skillPath for backward compatibility
with scenarios embedded in SKILL.md files.
Also update run-tests eval.yaml source paths from
'../../../../tests/dotnet-test/run-tests/fixtures/...' to 'fixtures/...'
(relative to the eval directory where the fixtures actually live).
This was causing all 11 source-based run-tests scenarios to get zero fixture
files, making the agent find an empty workspace and fail to evaluate.
* Address review: extract ResolveSourcePath helper, add warning, fix remaining eval.yaml paths
- Extract source-resolution and path-traversal check into internal
ResolveSourcePath helper for testability
- Emit a clear warning when file.Source is set but no base directory
is available (instead of silently continuing)
- Add unit tests covering: evalPath-based resolution, skillPath fallback,
null paths warning, traversal rejection, and evalPath preference
- Fix path traversal patterns in analyzing-dotnet-performance and
clr-activation-debugging eval.yaml files (same issue as run-tests)
* Add structural external dependency checker for skills, agents, and plugins
Add ExternalDependencyChecker to the skill-validator that produces advisory
warnings for structural dependencies that need human review:
- Script files in skill scripts/ directories (.ps1, .sh, .py, .bat, .cmd, .bash)
- INVOKES patterns referencing external scripts in skill descriptions
- #tool: references to non-built-in tools in skill/agent content
- Non-built-in tools declared in agent frontmatter tools array
- MCP server declarations in plugin.json files
URL scanning is intentionally excluded — it is handled by the reference
scanner (eng/reference-scanner/scan.ps1) with its known-domains.txt allowlist.
Also adds Tools property to AgentInfo record and RawAgentFrontmatter to
support parsing the tools array from agent YAML frontmatter.
Based on dotnet/skills#337 by ManishJayaswal, trimmed to structural checks
only (URL scanning superseded by #336).
Co-authored-by: ManishJayaswal <ManishJayaswal@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add allowlist support for external dependency checker
Introduce an allowlist file (eng/skill-validator/allowed-external-deps.txt)
so known external dependencies can be pre-approved. Each finding generates
a deterministic key (type:name:detail) that is checked against the list.
Allowed items are silently suppressed; unrecognized entries produce warnings.
The allowlist is pre-seeded with the 5 findings currently on main so the
checker runs clean out of the box. Over time, entries can be removed as
issues are resolved, and checks can be promoted to hard errors.
Adds 7 tests covering allowlist loading and filtering.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review feedback: naming, dedup, test robustness
- Rename 'errors' variables to 'findings' in all Check* methods to
reflect advisory (warning) semantics, not hard failures.
- Rename test methods from FlagsError to FlagsWarning, NoError to
NoWarning for consistency.
- De-duplicate #tool: reference warnings so repeated mentions of the
same tool in a skill/agent produce only one finding.
- Use GUID-based temp path instead of hardcoded '/nonexistent/path.txt'
in LoadAllowList_MissingFile test.
- Add 2 dedup tests (skill and agent) bringing total to 379.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix comments: tool refs scan full content, not just body
Update comments in CheckSkill and CheckAgent to accurately reflect
that #tool: references are scanned in the full markdown content
(including frontmatter), not just the body.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: ManishJayaswal <ManishJayaswal@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add plugin-aware skill validation with activation detection
* Address Copilot review comments (round 4)
- DESIGN doc: mark Targeted Skill Activation TODO as implemented
- ValidateCommand: fix misleading --plugin-dir comment
- Comparator: set PluginScore to null when no plugin runs exist
- ComparatorTests: copy missing fields in rehydrated ScenarioComparison
- AgentRunner: fix GetPluginClient docstring, route RunAgentCore through it
- AgentRunner: stage isolated skill to temp dir for true single-skill isolation
- dashboard.js: fix triple chart comment to match actual series names
- RunnerTests: update assertions for new isolated skill staging behavior
* Remove implementation doc
* Security Update
Identified and remediated 15 vulnerabilities across the skill-validator
runtime, dashboard, and CI/CD pipelines.
Changes:
- Runtime (AgentRunner.cs): path traversal prevention, deny-by-default
permissions, env scrubbing, MCP server hardening
- Regex (AssertionEvaluator.cs): ReDoS timeout
- Logging (SkillDiscovery.cs): exception logging
- Dashboard (dashboard.html, dashboard.js): SRI hash, XSS escaping
- CI/CD (5 workflow files): SHA pinning, persist-credentials: false
- Tests (RunnerTests.cs): 45+ new security test cases
MCP Server Defense-in-Depth (5 layers):
1. Command allowlist: only dotnet, node, npx, python, python3, uvx
2. Path rejection: no /, \, or .. in command names
3. Args validation: per-runtime dangerous flag blocklist
4. Env sanitization: strips PATH, LD_PRELOAD, NODE_OPTIONS,
DOTNET_STARTUP_HOOKS, etc.
5. Cwd dropped: custom working directories not passed to SDK
Subprocess Isolation:
- Sensitive env vars + prefix patterns scrubbed from child processes
- 120-second timeout with Kill(entireProcessTree: true)
- Deny-by-default permission model
Supply Chain:
- All GitHub Action references pinned to full commit SHAs
- All checkout steps set persist-credentials: false
- Chart.js CDN pinned to v4.4.7 with SRI integrity hash
* Update .github/workflows/skill-validator.yml
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update eng/skill-validator/src/Services/AgentRunner.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update eng/skill-validator/src/Services/AgentRunner.cs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Update tests to use fullCommandText field name
Update ExtractsCommandProperty and PrefersPathOverCommand tests to use
the fullCommandText property instead of command, matching the updated
permission request schema in AgentRunner.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add 'dnx' to allowed MCP commands
* Address PR review feedback: security hardening fixes
- CheckPermission: normalize paths with TrimEndingDirectorySeparator to
prevent double-separator false denials when dirs have trailing slash
- SanitizeMcpArgs: reject prefix forms (-econsole.log) and --flag=value
variants that bypass exact-match blocklist checks
- Setup file path traversal: normalize canonicalWorkDir and
canonicalSkillPath to avoid double-separator in StartsWith checks
- IsAllowedMcpCommand: use GetFileName instead of
GetFileNameWithoutExtension to block dotnet.bat/node.cmd; only strip
.exe on Windows
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* prune
* fix
* Fix concurrency setting
* Add binlog generation and upload to skill-validator workflow
Produce binary logs for build, test, and pack steps using /bl: flag.
Upload binlogs as artifacts (with if: always()) for build diagnostics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Improve CheckPermission diagnostics and fix AOT build error
Log allowed directories alongside denied paths to help diagnose
permission mismatches. Replace JsonSerializer.Serialize with manual
string building to fix IL3050 AOT compilation error.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Extract fileName from permission requests
Some permission requests use 'fileName' instead of 'path'. Handle
this property to avoid false denials for file-level operations.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* nownar nu1903 temporarily
* Log config dir in verbose mode alongside work dir
Include skilled/baseline label for consistency with the work dir log.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add skilled/baseline label to permission denial logs
Helps identify which run type triggered the denial when debugging.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix empty runLabel brackets and add labels to judge permission checks
Conditionally format the label suffix to avoid empty () when no
runLabel is supplied. Pass 'judge' and 'pairwise-judge' labels from
the respective judge call sites.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Allow pairwise judge to read from skilled work directory
The pairwise judge compares baseline and skilled runs in one session,
but previously only allowed reads from the baseline work directory.
Add additionalAllowedDirs parameter to CheckPermission and pass the
skilled work directory through PairwiseJudgeOptions.SkilledWorkDir.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix tests to match allow-by-default for unrecognized permission requests
CheckPermission was changed to allow requests with no extractable
path/command (allow-by-default), but four tests still expected the
old deny-by-default behavior.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Remove NoWarn
* Pin discover job checkout action to full commit SHA
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Dan Moseley <danmose@microsoft.com>
dependabot[bot]