skills/dotnet__skills
1
0
Fork 0
mirror of https://github.com/dotnet/skills.git synced 2026-09-20 09:49:54 +08:00
Code Issues Packages Projects Releases Wiki Activity
589 Commits 131 Branches 2 Tags
eval-exec-opus48
Commit Graph

107 Commits

Author SHA1 Message Date
Viktor Hofer c5435dd3da Make evaluate a subcommand with tests-dir as required argument (#398) 
* 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>
 2026-03-18 08:59:03 +00:00
Viktor Hofer 1a20e39a8d Refactor skill-validator: add check command, rename validate to evaluate (#388) 
* 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>
 2026-03-17 17:00:47 +01:00
Larry Ewing 1f127d4239 Add --keep-sessions flag and rejudge command to skill-validator (#149) 
* 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>
 2026-03-16 18:22:47 -05:00
Amaury Levé ab201ee691 Fix eval source file resolution to use eval directory (#374) 
* 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)
 2026-03-16 16:24:45 +00:00
Dan Moseley b966ac4dec Add structural external dependency checker (#355) 
* 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>
 2026-03-13 10:25:13 -06:00
Jan Krivanek 6666c9e5e8 [Infra] Add validation with whole plugin loaded (#316) 
* 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
 2026-03-12 22:44:36 +00:00
Abhitej John bb974f7360 Security Update (#331) 
* 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>
 2026-03-12 18:00:55 +00:00
Jan Krivanek 676523ca42 Bump dependency (#345) 2026-03-12 16:28:28 +00:00
Korolev Dmitry a2be8cce61 feat: noise-test evaluation option (#310) 
* use BPE tokenizer

* noise-test impl

* address metrics calculation

* address copilot comments

* more separation & refactor

* cleanup safely

* nit
 2026-03-11 20:39:15 +01:00
dependabot[bot] f8d6e3170f Bump the microsoft-and-system group with 2 updates (#324) 
Bumps Microsoft.Extensions.FileSystemGlobbing from 10.0.3 to 10.0.4
Bumps System.CommandLine from 2.0.3 to 2.0.4

---
updated-dependencies:
- dependency-name: Microsoft.Extensions.FileSystemGlobbing
  dependency-version: 10.0.4
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
- dependency-name: System.CommandLine
  dependency-version: 2.0.4
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
- dependency-name: Microsoft.Extensions.FileSystemGlobbing
  dependency-version: 10.0.4
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
- dependency-name: System.CommandLine
  dependency-version: 2.0.4
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
 2026-03-11 09:43:38 +01:00
dependabot[bot] b482146c0a Bump the all-other-nuget group with 1 update (#325) 
* Bump the all-other-nuget group with 1 update

Bumps GitHub.Copilot.SDK from 0.1.30 to 0.1.32

---
updated-dependencies:
- dependency-name: GitHub.Copilot.SDK
  dependency-version: 0.1.32
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: all-other-nuget
- dependency-name: GitHub.Copilot.SDK
  dependency-version: 0.1.32
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: all-other-nuget
...

Signed-off-by: dependabot[bot] <support@github.com>

* React to PermissionRequestResultKind breaking change in GitHub.Copilot.SDK 0.1.32 (#326)

* Initial plan

* Fix PermissionRequestResultKind breaking change in SDK 0.1.32

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>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
 2026-03-11 09:41:39 +01:00
Korolev Dmitry 484d5d2c1f tokenization: use BPE instead of chars\4 approximation (#309) 
* use BPE tokenizer

* address PR comments 1

* elaborate on model

* nit
 2026-03-10 20:16:57 +01:00
Dan Moseley 70ea8cf225 Include error details in evaluation failure PR comments (#297) 
* Include error details in evaluation failure PR comments

When evaluation fails due to validation errors (e.g., description too long),
the PR comment previously only said 'Evaluation failed. View workflow run',
requiring a click-through to discover the actual error.

Two fixes:
- Reporter.cs: GenerateMarkdownSummary() now renders an Errors section for
  verdicts with FailureKind set, so the marker comment shows what went wrong.
- evaluation-fork-pr.yml: The report-status job now downloads result artifacts
  and extracts error reasons from results.json, including them directly in the
  failure comment posted to the PR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* TEST: inflate build-perf-diagnostics description to trigger validation error

Temporarily makes the description 1221 chars (limit is 1024) to verify that
the new error reporting shows the validation failure in the PR comment.

This commit should be reverted after verifying the error display.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Tighten error filter to pre-eval failures and add tests

Address review feedback:
- Restrict the Errors section in markdown summary to verdicts with no
  scenarios (pre-eval failures like spec_conformance_failure), avoiding
  duplication with threshold/regression failures already shown in the table.
- Add three tests: errors section appears for pre-eval failures, is omitted
  when all verdicts pass, and is omitted for failures that have scenarios.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix error details in evaluation-run.yml post-completion-comment

The failure comment was posted by evaluation-run.yml's post-completion-comment
job (shared by both same-repo and fork PR workflows), not just by the fork PR
workflow. Apply the same artifact download + error extraction to this job so
error details appear for same-repo PRs too.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Promote eval prompt skill-name mention from warning to error

When an eval scenario prompt explicitly references the skill name,
it biases baseline runs and forces activation instead of testing
organic discovery. Move the check from warnings to errors so it
blocks evaluation via the existing profile.Errors fail-fast path.
No existing evals on main have this pattern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Sanitize @ in error output to prevent mention injection

Neutralize @ characters with a zero-width space in all three locations
where error text from PR-controlled content is rendered into PR comments:
Reporter.cs, evaluation-run.yml, and evaluation-fork-pr.yml.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Distinguish errors vs warnings in evaluation failure comments

Pre-eval structural errors (spec conformance, missing eval) get a
red X icon, while eval verdict failures (below threshold, completion
regression) get a warning icon. This makes it clear which failures
are blocking issues vs expected score variance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Only show pre-eval errors in failure comment, link to results table

The failure comment no longer repeats verdict warnings (below
threshold, completion regression) that are already visible in the
results table. It only shows pre-eval structural errors (spec
conformance failures) and links to the results table for full details.

Three cases:
- Pre-eval errors: show them + link to results table
- No errors but failed (verdict): just link to results table
- No results table: fallback with workflow run link

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove duplicate failure comment from fork workflow

The reusable evaluation-run.yml already posts failure comments when
pr-number is provided. The fork workflow was duplicating this by
posting its own failure comment, resulting in two comments per failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Revert "TEST: inflate build-perf-diagnostics description to trigger validation error"

This reverts commit 5f9056695a.

* Address review feedback: boundary-aware match, markdown escaping, safe find loop

- SkillProfiler: use boundary-aware regex for skill-name-in-prompt check
  to avoid false positives on short skill names (e.g. 'go')
- Reporter: wrap skill name in inline code and escape backticks/newlines
  to prevent markdown injection from PR-controlled content
- evaluation-run.yml: use find -print0 with while read loop to handle
  paths with spaces; match Reporter inline-code format in jq output

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Clean up comments in evaluation-fork-pr.yml

Removed redundant comments regarding failure comments for fork PRs.

* Refine comments on eval prompts and skill discovery

Clarify comment regarding eval prompts and skill discovery errors.

* Update eng/skill-validator/src/Services/Reporter.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update .github/workflows/evaluation-run.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Reapply "TEST: inflate build-perf-diagnostics description to trigger validation error"

This reverts commit 331ed870c5.

* Fix test assertion to match updated markdown error format

The Reporter.GenerateMarkdownSummary method was updated in 95ede45 to wrap
both skill name and reason in a single backtick pair for better markdown
injection prevention, but the test assertion was not updated to match.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update eng/skill-validator/src/Services/Reporter.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update .github/workflows/evaluation-run.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Link to newest results-table comment instead of oldest

The GitHub comments API returns oldest-first, so head -1 was linking
the failure comment to the oldest results-table marker comment rather
than the most recent run. Use tail -1 to select the newest match.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Consolidate PR comment jobs into a single comment-on-pr job

Merge comment-on-pr and post-completion-comment into one job that always
posts a new comment with the full results summary. When no result
artifacts exist (evaluate skipped or failed entirely), a short failure
message is posted instead.

This removes the find-and-update-existing-comment pattern, the separate
error extraction via jq, the PR_COMMENT_MARKER, and the comment_action
workflow output.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Guard consolidation step against missing validator binary

When result artifacts download succeeds but the validator binary
download/extract fails, the consolidate command would fail silently
(step is continue-on-error) and no PR comment would be posted. Check
that the validator executable exists before attempting consolidation,
falling through to the generic failure message otherwise.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Skip results table header when there are no scenario rows

Only emit the markdown table when at least one scenario row exists,
so error-only summaries show just the heading and errors section
without an empty table.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Display errors as bullet points in markdown summary

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Show validation errors with status emoji in heading

Use '### ❌ Skill validation errors:' heading for the errors section
to make the failure status immediately visible.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Revert "Reapply "TEST: inflate build-perf-diagnostics description to trigger validation error""

This reverts commit 59d5d8dfd4.

* Update eng/skill-validator/tests/OverfittingJudgeTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update .github/workflows/evaluation-run.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

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>
 2026-03-10 12:51:12 +01:00
Dan Moseley abdd4d03d4 Improve evaluation results table readability and add verdict explanations (#299) 
* Add verdict footnotes to evaluation results table

The results table showed a Verdict column (pass/fail) based on a weighted
composite score, but only displayed the raw quality delta. When the
composite disagreed with quality (e.g., quality improved but token/time
overhead made the composite negative), the verdict was unexplained.

Add a Notes column with footnote references [1], [2] etc. that link to
explanations below the table. Footnotes are only added when the verdict
disagrees with the quality delta direction, keeping the table clean for
straightforward cases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* TEST: inflate csharp-scripts description to exercise verdict footnotes

Expands the skill description to trigger token overhead in evaluation,
which should produce a scenario where the verdict footnote feature
activates. **Revert before merge.**

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address review feedback on BuildVerdictFootnote

- Return null for neutral verdicts (ImprovementScore == 0) and unknown
  quality delta (null), avoiding misleading footnotes for the yellow
  neutral icon or when the table shows delta as dash.
- Align quality direction threshold with the F1 rounding used in the
  delta column so footnotes only fire when the displayed value disagrees
  with the verdict.
- Use DefaultWeights.Values instead of hard-coded weight literals to
  avoid drift if weights change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix roundedDelta==0 footnote bug and improve test coverage

- Return null from BuildVerdictFootnote when rounded quality delta is zero,
  preventing misleading 'offset quality gain' text when the table shows 0.0
- Make test ImprovementScore values consistent with breakdown * DefaultWeights
- Add test for 'quality dropped but composite positive' branch
- Add test for roundedDelta==0 edge case

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Improve results table: collapse quality columns and clarify footnotes

- Merge Baseline, With Skill, and delta columns into a single Quality
  column showing 'base -> skill' with the better score bolded and an
  arrow indicator, reducing table width for narrow viewports
- Reword footnotes from cryptic 'composite -X% -- factors offset
  quality gain' to readable 'Quality improved but weighted score is
  -X% due to: factors'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Show raw metric values in verdict footnotes

Footnotes now include the actual baseline and skill values for each
contributing metric, e.g.:
  tokens (1000 -> 8000), tool calls (5 -> 15), time (2.1s -> 8.5s)

This lets readers immediately see the magnitude of each factor without
digging into workflow logs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Use emoji arrows, align footnote gating with Quality column

- Replace tiny unicode arrows with emoji (📈/📉) in Quality column
- Use raw quality delta for footnote direction instead of rounded delta,
  matching FormatQualityCell which compares unrounded scores
- Rename test to MarkdownTable_NoFootnoteWhenQualityUnchanged with
  truly equal scores instead of near-zero delta

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Make footnote references bidirectional hyperlinks

Table [1] links jump to the footnote explanation at the bottom,
and footnote [1] links jump back to the table row.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Restore dash assertion in ShowsDashWhenNoOverfitting test

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix flaky ReturnsEmptyForNonSkillDirectory test

Use a fresh empty temp directory instead of Path.GetTempPath() which
can contain stray SKILL.md files left by other tests or processes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Use <strong> instead of ** for bold inside HTML anchor tags

GitHub markdown does not render ** inside raw HTML elements.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Merge Notes into Verdict column and add footnote for quality-unchanged cases

- Remove Notes column (7 -> 6 columns) to reduce table width; footnote
  reference now appears next to the verdict icon
- Add footnote when quality is unchanged but verdict is negative, with
  text: 'Quality unchanged but weighted score is -X% due to: ...'
- Update tests accordingly

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Revert "TEST: inflate csharp-scripts description to exercise verdict footnotes"

This reverts commit d86dee0eee.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
 2026-03-10 09:27:13 +01:00
Jan Krivanek dcdad225a9 Fix 429 rate limiting in evaluation pipeline (#284) 
* Fix 429 rate limiting in evaluation pipeline

- P0: Add retry with exponential backoff to ListModelsAsync() in ValidateCommand
  using the existing RetryHelper (3 retries, 2s base delay, 60s budget)
- P1: Reduce scheduled-run parallelism to match infra-change settings (2/3/3)
  to avoid overloading the API when all 5 plugins run in parallel
- P2: Assign Copilot tokens deterministically by matrix job index instead of
  randomly, eliminating ~96% token collision probability
- P3: Add retry with backoff to AgentRunner.RunAgent() for mid-eval 429s
  (2 retries, 5s base delay, scenario timeout + 60s budget)

Fixes dotnet/skills#168

* Delete docs/fix-429-rate-limiting.md
 2026-03-08 21:11:02 +01:00
Dan Moseley 8e50fc8558 Add agent and plugin spec conformance validation (#279) 
* Add agent and plugin spec conformance validation

Extend the validation pipeline to check .agent.md files and plugin.json
files alongside skills. Previously only SKILL.md files were validated.

Agent validation (AgentProfiler):
- Frontmatter required (error)
- Name and description required (error)
- Name format: same rules as skills (lowercase alphanum + hyphens, 64 max)
- Name must match filename minus .agent.md suffix
- Description max 1024 chars
- Body max 500 lines

Plugin validation (PluginValidator):
- Name required, must match directory name
- Version and description required
- Description max 1024 chars
- Skills path required and must exist
- Agents path warned if specified but missing

Discovery (SkillDiscovery):
- DiscoverAgents() walks up from skill paths to plugin roots,
  scans agents/ directories for .agent.md files
- DiscoverPlugins() finds plugin.json from skill paths

Integration (ValidateCommand):
- Runs after skill discovery, before evaluation
- Errors block with exit code 1 (same as spec_conformance_failure)

All 4 existing agents and 2 plugins pass cleanly.
25 new tests (13 agent, 12 plugin).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Code review fixes: spec comments, ValidateNameFormat, kind-specific messages

- Refactor ValidateName into ValidateNameFormat (format-only) + ValidateName
  (format + directory match) so agent/plugin validation doesn't abuse the
  skill directory-match check by passing name twice.
- Warning messages now say 'Agent name' or 'Plugin name' instead of
  'Skill name' when called from AgentProfiler/PluginValidator.
- Add spec URL comments to AgentProfiler and PluginValidator referencing
  agentskills.io/specification, VS Code agent-plugins docs, and Claude Code
  plugins-reference.
- Add missing agent name edge-case tests: leading/trailing hyphen,
  consecutive hyphens, and message-prefix assertions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove unused agents path from plugins that have no agents

The dotnet, dotnet-data, and dotnet-upgrade plugins declared an agents
path but had no agents/ directory, producing validation warnings. Remove
the field; it can be added back when agents are actually created.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Promote agent/plugin name validation from warnings to errors

The agentskills.io spec uses 'Must' for all name constraints (length,
character set, no leading/trailing hyphens, no consecutive hyphens).
For a publishing validator these should block, not just warn. Agent
filename mismatch is also promoted since the IDE won't discover the
agent if the name doesn't match.

Skill name validation stays as warnings (lenient loading, not part
of this PR).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address PR review feedback

- Fix FrontmatterRegex: remove RegexOptions.Multiline so a horizontal
  rule (---) in the body is not mistaken for frontmatter. Detection and
  stripping now both anchor to start of string.
- Use filename as fallback identifier in AgentProfile when agent.Name
  is empty, so error output shows [agent:foo.agent.md] not [agent:].
- Add TryGetSafeSubdirectory to PluginValidator: rejects absolute paths
  and parent-directory traversal in skills/agents path fields. Used by
  both PluginValidator and SkillDiscovery.DiscoverAgents.
- Use DirectoryName as fallback for plugin result name when Name is
  empty, so error output shows [plugin:my-dir] not [plugin:].
- Add tests for path traversal rejection and name fallback behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Detect orphaned test directories with no matching plugin/skill

Add FindOrphanedTestDirectories to SkillDiscovery: checks that every
directory under tests/{plugin}/{skill}/ has a corresponding directory
under plugins/{plugin}/skills/{skill}/. Plugin-level orphans (tests/
directory with no matching plugins/ directory) are also flagged.

Integrated into ValidateCommand after plugin/agent validation so it
runs on every validation pass. Errors are blocking (exit 1).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address PR #279 review feedback

- Use case-sensitive (Ordinal) comparison for agent filename matching
  since agent names are required to be lowercase by spec.
- Replace OrdinalIgnoreCase path-under-root check in TryGetSafeSubdirectory
  with Path.GetRelativePath + '..' prefix check for correct behavior on
  case-sensitive filesystems.
- Let ParsePluginJson throw JsonException on malformed JSON instead of
  silently returning null. ValidateCommand catches it as a blocking error.
- Add test for malformed plugin.json.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Increase eval timeouts for dump-collect and dotnet-trace-collect to 120s

The with-skill runs for these skills were hitting the 60s scenario timeout
in CI, causing the agent to be killed before producing output. Other skills
in the same plugin already use 120s. The 30s decline/negative-test scenario
is left unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix fixture paths in clr-activation-debugging and analyzing-dotnet-performance evals

The source paths referenced tests/dotnet/ but the fixtures live under
tests/dotnet-diag/. This caused file-not-found exceptions at runtime,
which set hasRejections and caused unconditional exit 1 (bypassing
--verdict-warn-only).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Surface execution errors in validation summary

Previously, skills that threw exceptions (e.g. missing fixture files)
were silently excluded from the verdict count. The summary showed
'2/4 skills passed' with no indication that 2 other skills crashed
entirely. Now the summary shows '2/6 skills passed (2 rejected due
to execution errors)' and lists the specific error messages near
the summary rather than buried in the middle of the log.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Reduce parallelism for infra-change evaluation runs

When infrastructure changes trigger evaluation of all plugins
simultaneously, the high default parallelism (5x5x5) causes API
rate limits (429s) and scenario timeouts from contention. Reduce
to 2x3x3 for infra-change runs only; regular single-plugin PR
evaluations keep the default 5x5x5.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-08 15:40:06 +01:00
Dan Moseley 4cc0e46dc1 Promote skill spec conformance checks from warnings to errors (#278) 
* Promote skill spec conformance checks from warnings to errors

The agentskills.io spec (https://agentskills.io/specification) uses
'Must' for all of these constraints:

- name: 'Must be 1-64 characters'
- name: 'May only contain lowercase alphanumeric and hyphens'
- name: 'Must not start or end with -'
- name: 'Must not contain consecutive hyphens'
- name: 'Must match the parent directory name'
- description: 'Must be 1-1024 characters' (non-empty)
- compatibility: 'Must be 1-500 characters if provided'

These were previously warnings following the lenient-loading guidance
from the client implementation guide, but that guidance is for client
loaders, not publishing validators. For a CI tool that gates what gets
published, spec violations should be blocking errors.

All 25 existing skills already comply — no changes needed to skills.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address PR review feedback: empty compatibility, empty name, test

- Error when compatibility field is provided but empty/whitespace,
  matching spec requirement of 1-500 characters when provided.
- Add explicit check for empty/whitespace name with targeted error
  message instead of falling through to regex mismatch.
- Add test for empty compatibility string rejection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-07 09:36:44 +01:00
Dan Moseley 698b335ab2 Enforce description size limits and add spec conformance checks (#238) 
* Enforce description size limits (per-skill 1024, per-plugin 15K)

Add infrastructure to reject skills with oversized descriptions, per issue #222:

- SkillProfiler: check description length, flag DescriptionTooLong
- ValidateCommand: block oversized descriptions (survives --verdict-warn-only)
- New eng/validate-descriptions.ps1: validates per-skill and per-plugin aggregate limits
- New CI workflow: runs validation on PRs touching plugins/
- build.ps1: adds size checks to local build validation
- Unit tests for all new SkillProfiler behavior

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update --verdict-warn-only help text to mention description_too_long

Address review feedback: the option description now lists description_too_long
alongside execution errors and --require-evals as conditions that remain
fatal even when --verdict-warn-only is enabled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add agentskills.io spec conformance checks to SkillProfiler

Validate name rules (max 64 chars, lowercase+hyphens, no edge/consecutive
hyphens, must match directory), compatibility field max 500 chars, body
max 500 lines, and file reference depth.

Scan of existing skills found 2 violations:
- dotnet-msbuild/msbuild-antipatterns: 582 lines (max 500)
- dotnet-msbuild/directory-build-organization: cross-skill ref 3 levels deep

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address review feedback on spec conformance checks

- Fix MakeSkill SkillMdPath to derive from resolved path (SkillProfileTests.cs)
- Fix RepoRoot in build.ps1 to be actual repo root (was agentic-workflows/)
- Fix body line count off-by-one: trim trailing newlines before splitting
- Fix file ref depth: normalize paths, handle .. traversals, strip #anchor,
  compute depth from directory segments only
- Add tests for parent traversal, anchor fragments, ./ normalization,
  and trailing newline body count

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Consolidate description validation into skill-validator C#

Move the per-plugin aggregate description limit (15K chars) from the
standalone PowerShell script into ValidateCommand, where it runs after
skill discovery. This eliminates the separate validate-descriptions.ps1,
its CI workflow, its PowerShell tests, and the delegation from build.ps1.

Also rename FailureKind from "description_too_long" to the generic
"spec_conformance_failure" so future spec checks reuse the same kind.

Deleted:
  - eng/validate-descriptions.ps1
  - eng/tests/validate-descriptions.tests.ps1
  - .github/workflows/validate-descriptions.yml

Added:
  - ValidateCommand.CheckAggregateDescriptionLimits()
  - ValidateCommand.DerivePluginName()
  - 6 C# tests for aggregate limit + DerivePluginName

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Upgrade body-lines and file-ref-depth from warnings to blocking errors

Now that #247 has merged and fixed the two existing violations
(msbuild-antipatterns 582 lines, directory-build-organization cross-ref),
these spec conformance checks can be blocking.

Also replaces the DescriptionTooLong bool with a general Errors list on
SkillProfile, so all spec conformance violations (description length,
body lines, file ref depth) flow through the same mechanism.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Revert unrelated lock file changes

The lock file diffs were side effects of running build.ps1 to verify
the validation changes — they picked up upstream content changes
unrelated to this PR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-07 00:16:33 -07:00
Jan Krivanek e392d3cfde Fix flaky test (#258) 
* Fix flaky test

* Update eng/skill-validator/tests/RetryHelperTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
 2026-03-06 12:32:53 +01:00
Viktor Hofer 75263eeef4 Use non-zipped artifacts to preserve file permissions (#257) 
- Upload skill-validator-dist as tar.gz with archive: false to preserve
  execute permissions through artifact round-trip
- Remove chmod +x workarounds in evaluate and comment-on-pr jobs
- Include README.md in publish output via CopyToPublishDirectory

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-06 10:10:04 +01:00
Viktor Hofer 78f1ef2d53 skill-validator: improve UX for published tool (#223) 
* skill-validator: improve UX for published tool

- Make paths argument required (OneOrMore arity) so invoking without
  args shows full help text instead of a cryptic error
- Add Installation section to README covering dotnet tool install from
  nightly release, platform-specific AOT packages, and pipeline artifacts
- Update all usage examples to use the skillvalidator command
- Update CI integration example to install from nightly release
- Include README.md as NuGet package readme

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Address PR feedback: clarify release wording and chmod instructions

- Replace ambiguous 'GitHub pre-release artifacts' with a direct link
  to the skill-validator-nightly GitHub Release
- Use 'chmod +x' instead of 'give +x permissions'
- Capitalize NuGet consistently

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Align ToolCommandName with AssemblyName (skill-validator)

Fixes naming mismatch where dotnet tool install produced 'skillvalidator'
but the AOT binary was 'skill-validator'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update README.md

* Publish tar.gz archives in skill-validator workflow

- Add tar.gz creation from dotnet pack publish output for each RID
- Upgrade upload-artifact to v7 with archive: false (no double-zip)
- Upgrade download-artifact to v8 for non-zipped artifact support
- Include tar.gz files in nightly GitHub release

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update README

* Update README

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
 2026-03-06 09:50:40 +01:00
Cameron Aavik a837a9a9e7 Reduce bias introduced by path names during evaluation (#253) 2026-03-06 07:41:07 +01:00
Viktor Hofer 54e6fe8485 Enable NativeAOT publishing for SkillValidator (#221) 
* Enable NativeAOT publishing for SkillValidator

Migrate all serialization to source-generated, AOT-compatible patterns:

- Add PublishAot to csproj and Vecc.YamlDotNet.Analyzers.StaticGenerator package
- Create SkillValidatorJsonContext with source-generated serializers for all types
- Create SkillValidatorYamlContext with static YAML deserialization for eval config
  and frontmatter parsing
- Change AgentEvent.Data from Dictionary<string, object?> to
  Dictionary<string, JsonNode?> for AOT-safe serialization
- Replace anonymous type in Reporter.cs with named ResultsOutput record
- Fix JsonStringEnumConverter to generic AOT-safe JsonStringEnumConverter<T>
- Move ConsolidateData to Models.cs for JSON context accessibility
- Switch EvalSchema and SkillDiscovery to StaticDeserializerBuilder
- Add RawFrontmatter model for strongly-typed YAML frontmatter parsing
- Update all JsonSerializer call sites to use context-based overloads

The published binary is a single 14.6 MB native executable.
All 198 existing tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add NativeAOT publish matrix and daily scheduled builds to CI

- Single build job with matrix for linux-x64, linux-arm64, win-x64, win-arm64, osx-arm64
- Each matrix entry: build, test, NativeAOT publish, pack NuGet, upload tarball
- collect-packages job: builds RID-agnostic package, merges with per-RID packages
  into a single skill-validator-nupkgs artifact
- Add daily schedule trigger (08:00 UTC) with 90-day retention
- Keep existing path-based triggers for push/PR

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add NativeAOT publish matrix and daily scheduled builds to CI

- Single build job with matrix for linux-x64, linux-arm64, win-x64, win-arm64, osx-arm64
- Each matrix entry: build, test, NativeAOT publish, pack NuGet, upload binary
- Upload publish directory directly (Actions wraps in zip automatically)
- collect-packages job: builds RID-agnostic package, merges with per-RID packages
  into a single skill-validator-nupkgs artifact
- Add daily schedule trigger (08:00 UTC) with 90-day retention
- Keep existing path-based triggers for push/PR

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix flaky DelayClampedToRemainingBudget test

Increase totalTimeoutMs from 200ms to 2000ms. The tight budget was
causing the CTS to fire before the retry attempt on slower CI runners
(macOS arm64). The test still validates that the 60s base delay gets
clamped — the 5s wall-time assertion is the real check.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Cancel in-progress workflow runs on new push

Add concurrency group so that pushing a new commit to a PR
automatically cancels any still-running workflow for that ref.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix per-RID NuGet pack with --use-current-runtime

The pack step was producing the same agnostic package for every RID.
Adding --use-current-runtime ensures each matrix entry produces a
RID-specific nupkg that gets collected into the combined artifact.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove redundant publish step; pack already produces native binary

dotnet pack --use-current-runtime already runs publish internally,
producing the native AOT binary in artifacts/publish/. The separate
dotnet publish step was redundant.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Rename nupkgs artifact to nuget-bundled

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove redundant PublishTrimmed; implied by PublishAot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix flaky ExponentialBackoff_DelaysIncrease test

Increase baseDelayMs from 50ms to 200ms so that OS scheduling jitter
on slow CI runners (macOS arm64) doesn't dominate the measured gaps.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove per-RID publish directory artifacts; nupkgs suffice

NuGet packages are zip files containing the native binaries, so the
separate skill-validator-<rid> publish folder uploads are redundant.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove collect-packages job; produce agnostic nupkg from linux-x64

Pack the RID-agnostic package from the linux-x64 matrix entry instead
of a separate downstream job, eliminating the bundle step entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Rename artifact names from nuget-* to skill-validator-*

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Rename agnostic artifact to skill-validator

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Simplify NuGet artifact uploads; remove rm -rf and collect-packages

Use precise glob patterns with the stable package name to distinguish
RID-specific from agnostic nupkgs. The agnostic upload uses a negation
pattern to exclude the RID-specific package on linux-x64.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Extract retention-days and agnostic RID into workflow env vars

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add nightly release job using gh release

On scheduled runs, downloads all nupkg artifacts and publishes them
as a rolling 'skill-validator-nightly' pre-release on the repo.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Restore PropertyNameCaseInsensitive for JSON deserialization

The original MCPServerDef deserialization used PropertyNameCaseInsensitive
which was dropped when switching to source-generated context. Add it back
to the JsonSourceGenerationOptions to preserve flexible property casing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-05 21:07:17 +01:00
Jan Krivanek 62d65dcc6e [Infra] Check for overfit in the scenario prompt as well (#218) 
* Check for overfit in the scenario prompt as well

* Update eng/skill-validator/src/Services/OverfittingJudge.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix tests

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
 2026-03-05 20:17:14 +01:00
Viktor Hofer 982a0bb726 Disallow eval.yml without scenarios (#216) 
Reject eval configs with an empty scenarios list at parse time,
rather than silently accepting them. This ensures authors don't
accidentally ship eval files that test nothing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-05 16:56:04 +01:00
Dan Moseley 98d9f28578 Warn when eval prompt explicitly references skill name (#212) 
* Warn when eval prompt explicitly references skill name

Add a profile warning when an eval scenario prompt contains the skill
name (e.g. 'Use the migrate-dotnet10-to-dotnet11 skill'). This pattern
biases baseline runs (agent wastes time searching for a non-existent
skill file) and forces activation rather than testing organic discovery.

The warning flows through the existing ProfileWarnings pipeline so it
surfaces in console output, JSON results, and the 'Possible causes'
section when a verdict fails.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address review: guard empty skill name, add unit tests

Guard against empty/whitespace skill names that would false-positive on
string.Contains. Add three unit tests covering: prompt mentions skill
name (warns), prompt does not mention skill name (no warn), and empty
skill name (no warn).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-05 07:11:15 +01:00
Jan Krivanek e1f513f486 [Infra] Improve judges retry and resiliency (#188) 
* Add better retry for judge

* Extract funtionality and guard against single scenario failing whole eval run

* Reflect PR comments

* Reflect PR comments

* Apply timeout and parallelism defaults to evaluation-run.yml

Upstream refactored the evaluation workflow to use a reusable workflow
(evaluation-run.yml). Move our timeout (60->90min) and parallelism
(3->5) changes to that file.
 2026-03-04 19:35:32 +01:00
Viktor Hofer 7dd45ea1a8 Enable evaluation for fork PRs via reusable workflow (#198) 
* Enable evaluation for fork PRs via reusable workflow

Refactor the evaluation workflow into three files:

- evaluation-run.yml: reusable workflow with build-validator, evaluate,
  and comment-on-pr jobs (shared by both callers)
- evaluation.yml: thin caller for pull_request and schedule triggers;
  skips fork PRs which are handled by the new fork workflow
- evaluation-fork-pr.yml: thin caller using pull_request_target with
  environment-gated secrets for fork PRs

Also clear GITHUB_TOKEN from the process environment after the
CopilotClient reads it, so eval scenarios and setup commands cannot
access the credential.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address PR review: harden permissions and cleanup

- Add persist-credentials: false to untrusted content checkout
- Add explicit permissions (contents: read) to build-validator and
  evaluate jobs to limit blast radius
- Guard comment-on-pr against skipped evaluate result
- Remove unused is_infra input from reusable workflow

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Restrict discover job permissions and document workflow-level scope

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-03-04 18:18:21 +01:00
dependabot[bot] 491c9c89b6 Bump the all-other-nuget group with 1 update (#197) 
Bumps GitHub.Copilot.SDK from 0.1.26 to 0.1.30

---
updated-dependencies:
- dependency-name: GitHub.Copilot.SDK
  dependency-version: 0.1.30
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: all-other-nuget
- dependency-name: GitHub.Copilot.SDK
  dependency-version: 0.1.30
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: all-other-nuget
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
 2026-03-04 14:58:30 +01:00
dependabot[bot] e143b04999 Bump the microsoft-and-system group with 1 update (#196) 
Bumps Microsoft.Extensions.FileSystemGlobbing from 10.0.0 to 10.0.3

---
updated-dependencies:
- dependency-name: Microsoft.Extensions.FileSystemGlobbing
  dependency-version: 10.0.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
- dependency-name: Microsoft.Extensions.FileSystemGlobbing
  dependency-version: 10.0.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: microsoft-and-system
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
 2026-03-04 14:11:16 +01:00
Jared Parsons 4793aa27e9 Replace binlog mcp with replay to textlog (#162) 
Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com>
 2026-03-02 21:32:23 +01:00
Dan Moseley 63c6ee1eec Nullable reference migration skill (#39) 
* Add nullable-reference-migration skill

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance: annotate based on design intent, not warnings

Annotations should reflect the intended nullability contract of each
member. Deciding intent first and then fixing warnings avoids the
common mistake of over-annotating with ? or scattering ! to silence
the compiler.

Learned from dotnet/runtime nullable annotation guidelines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add pitfall: ?. (null-conditional) changes behavior unlike ? and !

During nullable migration it is tempting to change obj.Method() to
obj?.Method() to silence a warning. Unlike ? on a type or ! on an
expression, ?. changes the generated IL and silently skips the call
when the value is null, which can introduce subtle regressions.

Learned from dotnet/runtime PR #57418, #65262, #67198 review comments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add pitfall: adding ? to value types changes the actual type

For reference types, ? is a metadata-only annotation. For value types
(enums, structs, int, etc.), ? wraps the type in Nullable<T>, which
changes the method signature and binary layout. This was caught as a
breaking change in dotnet/runtime PR #65262 when LogLevel was
accidentally made nullable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance: Debug.Assert as compiler hint and reducing ! usage

Debug.Assert(x != null) informs the nullable flow analyzer just like
an if-check, eliminating the need for ! operators in the subsequent
scope. Also adds guidance that if every call site of an internal
method needs !, the parameter should probably be nullable instead.

Learned from dotnet/runtime PR #67198 and PR #67198 review feedback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Enhance CS8618 guidance with [MemberNotNull] for helper methods

When a helper method called from the constructor initializes fields,
[MemberNotNull(nameof(field))] tells the compiler those fields are
guaranteed non-null after the call, eliminating CS8618 warnings
without resorting to ! or making fields nullable.

Learned from dotnet/runtime PR #67198 where this pattern eliminated
many suppression operators.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance for struct default fields and post-Dispose state

Struct reference-type fields are null in default(T). Annotate based
on whether default is valid usage: nullable if yes, non-nullable if
not. Also adds guidance that post-Dispose nullability should not
weaken annotations, since post-Dispose use is a contract violation.

Learned from dotnet/runtime PR #528 (struct fields), nullability.md
(InterfaceMapping vs CancellationToken), and PR #65262 (Dispose).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance: prioritize public API annotations over internal code

Public and protected APIs define the contract consumers depend on
and should be annotated first and most carefully. Internal code can
tolerate ! more liberally since it does not affect callers.

Learned from dotnet/runtime PR #67198 review feedback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add pitfall: do not remove ArgumentNullException checks

Non-nullable parameters still need runtime null validation because
callers may not have NRT enabled (other languages, older C#,
suppressed warnings). Removing existing checks during migration
can introduce regressions for those callers.

Learned from dotnet/runtime nullability.md guidelines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Enhance Try method annotation patterns with specific recipes

Clarifies the distinction between generic and non-generic Try method
out parameters: use [NotNullWhen(true)] out T? for non-generic, and
[MaybeNullWhen(false)] out T for generic. Also notes that
Equals(object?) overrides should use [NotNullWhen(true)].

Learned from dotnet/runtime nullability.md DO guidelines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance for override covariance and IEquatable<T?>

Override return types can be stricter (non-nullable) than the base
even if the base returns T?. Also adds that reference types should
implement IEquatable<T?> and IComparable<T?> since callers commonly
pass null to Equals.

Learned from dotnet/runtime PR #67198 and nullability.md guidelines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add three-pass code review checklist for nullable changes

Nullable PRs need broader review than a typical diff: (1) verify no
behavior/IL changes, (2) review explicit annotation changes for
correct contracts, (3) scan unchanged APIs whose implicit nullability
changed when the nullable context was enabled.

Learned from dotnet/runtime nullability.md Code Review Guidance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add link to dotnet/runtime nullable annotation guidelines

This is the authoritative internal document used when annotating the
.NET libraries and contains detailed guidance on gray areas, virtual
methods, struct defaults, and attribute usage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Strengthen null validation pitfall: NRTs are compile-time only

Nullable annotations do not prevent null at runtime. Callers using
older C#, other .NET languages, reflection, dynamic, or the !
operator can still pass null. Emphasizes that runtime validation on
public APIs remains essential for correctness and security.

Learned from community blog posts (Maarten Balliauw, JetBrains)
and dotnet/runtime nullability.md.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Define NRT abbreviation at first use in body text

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Note that [MaybeNull] should be rare — prefer T? when possible

The .NET runtime codebase has only ~7 uses of [MaybeNull]. In most cases
T? is a better choice. Reserve [MaybeNull] for edge cases like
AsyncLocal<T>.Value where T? would incorrectly allow setting to null
when T is non-nullable.

Source: dotnet/runtime nullability guidelines

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance to use [NotNullWhen(true)] on Equals overrides

When Equals(object? obj) returns true, the argument is guaranteed
non-null. Annotating with [NotNullWhen(true)] lets callers skip
redundant null checks after equality tests.

Source: dotnet/runtime nullability guidelines

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance for widely-overridden virtual return types

For virtual/abstract methods with many overrides, check whether
existing overrides return null in practice. The .NET runtime annotated
Object.ToString() as string? (overrides commonly return null) but
Exception.Message as string (null overrides vanishingly rare).
When in doubt, prefer T?.

Source: dotnet/runtime nullability guidelines (ToString case study)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Prefer multiple [MemberNotNull] attributes over params overload

The params string[] overload of MemberNotNull is not CLS-compliant
and may require #pragma warning disable. Use separate attributes
for each field instead.

Source: dotnet/runtime nullability guidelines

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Warn that compiler does not flag misapplied nullable attributes

Attributes like [DisallowNull] on a non-nullable parameter or
[MaybeNull] on a by-value input parameter are silently ignored.
Even Roslyn team members get tripped up by this.

Source: dotnet/roslyn#36073 (jcouv, RikkiGibson)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Warn about false safety from nullable-oblivious libraries

Unannotated dependencies produce no nullable warnings at all,
giving a false sense of safety. Return values from oblivious APIs
should be treated as potentially null.

Source: dotnet/roslyn#44063 (jcouv), dotnet/roslyn#38941

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Clarify Try method annotation: generic vs non-generic out params

Non-generic out params: declare nullable with [NotNullWhen(true)].
Generic out params: keep non-nullable with [MaybeNullWhen(false)].
Using T? on generic params would change value-type signatures to
Nullable<T>.

Source: dotnet/runtime nullability guidelines

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance for unconstrained generics and LINQ null filtering

For generic methods returning default on unconstrained T, use
[return: MaybeNull] T instead of T? to avoid changing value-type
signatures to Nullable<T>.

LINQ Where(x => x != null) does not narrow T? to T. Use a
WhereNotNull() helper (as Roslyn itself does) or OfType<T>().

Source: dotnet/roslyn#30953 (jcouv, roji), dotnet/roslyn#39586 (sharwell)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add pitfall: var is always considered nullable by the compiler

The compiler treats var as nullable regardless of the assigned
expression — flow analysis determines the actual null-state. This
can surprise developers who expect var to inherit non-nullability
from the right-hand side.

Learned from Maarten Balliauw's nullable migration blog series.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add strategy guidance: start at the center, work outward

Annotating core domain models, DTOs, and shared utilities first
eliminates cascading warnings across the codebase and gives the
biggest return on effort. Applies to all three rollout strategies.

Learned from Maarten Balliauw's JetBrains 2025 talk and blog series.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add dedicated EF Core section with schema migration warning

EF Core infers column nullability from NRT annotations. Enabling
NRTs can silently generate migrations that make previously nullable
columns required, risking data loss. Consolidates all EF Core
guidance (navigation properties, schema impact, migration review)
into a dedicated section and removes the scattered EF references.

Learned from Maarten Balliauw's JetBrains 2025 talk, Damir Arh's
blog, and EF Core documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* EF Core: warn that #nullable disable warnings is insufficient for entity files

#nullable disable warnings only suppresses compiler diagnostics but leaves
the annotation context active. EF Core reads annotations via reflection,
so properties without ? are still treated as required, potentially altering
database schema. Use #nullable disable to fully opt entity files out.

Source: dotnet/efcore#33245 (confirmed by EF team member roji).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* EF Core: add DbSet<T> initialization guidance

DbSet properties should remain non-nullable since EF Core always
initializes them. EF Core 7.0+ suppresses CS8618 automatically;
older versions can use = null! or expression body => Set<T>().

Source: official EF Core NRT docs page.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* EF Core: expand navigation property guidance with three approaches

Replace single-sentence guidance with the three patterns from the
official EF Core NRT docs: (a) non-nullable with null!, (b) nullable
when code checks load state, (c) non-nullable wrapping nullable
backing field that throws. Also note collection navigations must
always be non-nullable (empty, never null).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* EF Core: add LINQ query guidance for optional navigations

EF Core translates LINQ to SQL so null navigations don't throw at
runtime, but the compiler still warns. Use ! in Where/Include
expressions to suppress false positives. From official EF Core docs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* ASP.NET Core: MVC treats non-nullable properties as [Required]

When NRTs are enabled, ASP.NET Core MVC/Web API implicitly adds
[Required(AllowEmptyStrings = true)] to every non-nullable reference
type property in DTOs and view models. This can cause previously
accepted requests to return 400 Bad Request. Controllable via
SuppressImplicitRequiredAttributeForNonNullableReferenceTypes.

Source: official ASP.NET Core model validation docs, aspnetcore#22656.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* ASP.NET Core: System.Text.Json RespectNullableAnnotations and limits

STJ (.NET 9+) can enforce NRT annotations at serialization time via
RespectNullableAnnotations, but cannot enforce collection element
nullability (List<string> vs List<string?> are indistinguishable in
IL), top-level types, or generic properties. Document the opt-in
and its limitations.

Source: official STJ nullable annotations docs, aspnetcore#57234
(confirmed by eiriktsarpalis and terrajobst).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* ASP.NET Core: warn about #nullable disable warnings on model files

Same pitfall as EF Core: #nullable disable warnings only suppresses
compiler diagnostics but leaves annotations active. MVC reads them
via reflection to infer [Required], so model files not yet migrated
must use #nullable disable to fully opt out.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Expand rollout strategy guidance and add suppression analysis use case

Add 'analyzing existing suppressions' to When to Use section.
Expand the start-at-center strategy with guidance to work outward
through layers and keep PRs scoped per project or layer.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add helper extension methods section with WhereNotNull

Add a Helper Extension Methods section with WhereNotNull for both
reference and value types. This is the pattern used by the Roslyn
compiler codebase to filter nulls with correct type narrowing, which
.Where(x => x != null) cannot provide. Update the inline reference
in Step 3 to link to the new section.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add incremental build checkpoints to NRT migration workflow

Add explicit build checkpoint instructions after Steps 2-6 so the
AI builds and checks warning counts after each phase rather than
attempting all fixes blind.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add commit strategy guidance to NRT migration workflow

Tell the AI when to commit during the migration: at each logical
boundary (enable, dereference fixes, annotations, attributes,
cleanup) to keep diffs reviewable and prevent lost work.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add 'when to ask the user' guidance for annotation decisions

Tell the AI to ask the user before changing public API contracts,
deciding nullable vs required for ambiguous properties, or choosing
between null checks and suppressions when design intent is unclear.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add inline anti-pattern callouts with warning markers

Add prominent ❌ and ⚠️ callouts inline in Steps 3 and 4 where
dangerous mistakes are most likely: using ?. to silence warnings,
sprinkling !, adding ? to value types, letting warnings drive
annotations, and removing existing null argument validation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add decision flowchart for resolving nullable warnings

Add a structured decision tree in Step 3 that guides the AI through
each warning: is null valid by design? Can you prove non-null? This
prevents defaulting to ! or over-annotating with ?.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add scope-based prioritization guidance to Step 3

Tell the AI to work in dependency order (core models first, then
consumers) and prioritize public/protected members within each file
to minimize cascading warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add verify-before-claiming guidance to validation step

Tell the AI to verify annotations match design intent, check for
accidental ?. operators, confirm no removed null checks, and audit
! usage before reporting the migration as complete.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add conditional behavior guidance based on project type

Tell the AI to tailor its approach based on whether the project is
a library (public API contracts first), application (boundaries
first), or test project (more liberal use of !).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Flag public API methods missing runtime null validation

Tell the AI to check for missing ArgumentNullException guards on
non-nullable public/protected parameters during annotation. Without
runtime checks, null passed at runtime causes NRE instead of ANE.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add Purpose paragraph to match recommended SKILL.md structure

Add a one-paragraph Purpose description under the top-level heading,
describing the outcome of using this skill.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Couple notes

* Add missing reference links to More Info section

Add links to Nullable reference types overview, C# language reference
page, and null-forgiving operator reference. These are foundational
docs referenced throughout the skill.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add breaking changes section for library NRT annotations

Warn that incorrect NRT annotations are source-breaking changes for
library consumers. Covers implicit non-nullability, parameters that
should be nullable, return types, and versioning guidance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add guidance: methods tolerating null should accept nullable params

If a method handles null gracefully (returns null, returns default,
returns a failure result) rather than throwing, the parameter should
be T?, not T. Based on BCL conventions and NodaTime design discussions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Make build/test references build-system agnostic

Not all repos use dotnet build/test directly — some use build scripts,
msbuild, cake, nuke, etc. Add build/test command as optional inputs,
update Step 1 to detect the build system, and replace hardcoded
'dotnet build'/'dotnet test' with generic 'build'/'test' in
checkpoints and validation checklist.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove cake/nuke from build script examples

These are uncommon enough to not warrant explicit mention. Keep the
list focused on what an agent would actually encounter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Recommend WarningsAsErrors in project file instead of CLI flag

<WarningsAsErrors>nullable</WarningsAsErrors> in the .csproj or
Directory.Build.props permanently prevents nullable regressions,
which is better than relying on a CLI flag that CI might miss.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Track and document breaking changes for public libraries

When annotating a public library, the AI should create a
nullable-breaking-changes.md listing all public API changes that
could affect consumers (return types narrowed/widened, parameters
changed, value type nullability, new guards). This file serves as
a review artifact and basis for release notes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add trigger phrases to skill description for discovery

Add CS8602, CS8618, CS86xx, #nullable enable, annotating APIs, and
null-forgiving operator keywords so agents discover this skill for
a wider range of nullable-related user requests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Move framework-specific sections to references/ for progressive disclosure

Move EF Core, ASP.NET Core, breaking changes, and helper extension
method content into references/ files. SKILL.md retains one-line
summaries with links so the agent loads detailed content only when
relevant. Reduces SKILL.md from 218 to 194 lines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add NRT readiness scanner script

Add scripts/Scan-NullableReadiness.ps1, a PowerShell Core script that
scans a project or solution and reports NRT migration status: project
settings (Nullable, LangVersion, TargetFramework, WarningsAsErrors)
and source-level counts (#nullable directives, ! operators, #pragma
CS86xx suppressions). Supports -Path, -Json, and -Recurse parameters.

Update SKILL.md Steps 1 and 6 to reference the script as an optional
automation step.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix strict mode .Count errors in Scan-NullableReadiness.ps1

Wrap pipeline results in @() before accessing .Count to avoid
'property Count cannot be found' errors under Set-StrictMode -Version Latest
when a pipeline returns a single object instead of an array.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add automated tests for Scan-NullableReadiness.ps1

Test script runs the scanner against three fixture projects:
- nrt-disabled: no <Nullable>, has ! operators and #pragma suppressions
- nrt-enabled: <Nullable>enable, clean code
- nrt-partial: <Nullable>enable but files with #nullable disable and !

Validates JSON output matches expected NRT readiness state (19 assertions).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Recommend clean builds for validation checkpoints

Incremental builds only recompile changed files, hiding nullable
warnings in untouched files. All build checkpoints should use
clean builds (e.g., --no-incremental) to surface all warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Move nullable-reference-migration to src/dotnet/ layout

Align with upstream repo restructuring that moved skills and tests
under src/dotnet/.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Improve ! operator counting by stripping comments and strings

The scanner now removes block comments, line comments, verbatim strings,
interpolated strings, and regular strings before counting null-forgiving
operators. This eliminates false positives from ! in prose like
'Important!' or 'Value required!'.

Tests updated with exact assertions and fixtures enriched with !
in comments, XML docs, and string literals to verify filtering.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix misleading 'var is always nullable' wording

var does not inherently introduce nullability — the inferred type
carries nullability from the right-hand side expression. Updated
the pitfall description to reflect this accurately.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Make STJ RespectNullableAnnotations limitations more explicit

Clarify that the feature is opt-in, affects both serialization and
deserialization, and explicitly list what it does not cover:
collection elements, dictionary values, top-level types, and
generic type parameters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add caveat that Debug.Assert is stripped from Release builds

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Clarify that required does not enforce during deserialization

The required keyword is a compile-time and object-initializer
constraint. It does not replace runtime validation unless the
serializer explicitly supports it (e.g., STJ with [JsonRequired]).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add explicit version callouts for required, DbSet, JsonRequired

Consistently tag version requirements so the skill adapts across
repos targeting different .NET versions:
- required (C# 11+) on all mentions
- EF Core 7.0+ (.NET 7) for DbSet CS8618 suppression
- [JsonRequired] (.NET 7+)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add DTO vs domain model nullability guidance

DTOs should be nullable by default (they cross trust boundaries),
domain models should prefer non-nullable invariants. This distinction
is where migrations most often go wrong.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add gray area heuristics for parameter nullability decisions

From dotnet/runtime nullability guidelines: when a parameter is
neither validated nor documented for null, check usage patterns
in the codebase, whether null acts as a default/nop, and whether
similar methods accept null. When in doubt, prefer nullable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Never infer nullability intent from usage frequency or naming

Explicit guard against AI hallucination: if intent is not clear
from code or documentation, ask the user rather than guessing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Strengthen serialization bypass warning in pitfalls table

Reworded to anchor the rule: serializers bypass constructors,
non-nullable properties on DTOs do not imply runtime safety.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Stop and ask user if lang version or TFM is insufficient for NRTs

If the project targets C# 7.x or a framework that defaults below
C# 8.0, explicitly inform the user what needs to change and ask
whether to update or abort. Do not silently proceed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add = null! guidance for late-initialized fields in NRT skill

Add recommended technique for CS8618 warnings on fields set after
construction: use = null! on the field declaration instead of adding !
at every use site. Document the pattern in the CS8618 table row, the
annotation decision flowchart, and the Step 3 guidance section.

Motivated by exercising the skill on System.Text.RegularExpressions,
where = null! reduced ! operator count from 423 to 178 compared to
the original 2019 NRT annotation PR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Categorize ! operators as null!/default! vs assertions in scanner

Break down the ! operator count into two categories:
- null!/default! initializers (safe, expected for late-initialized fields)
- assertions (value! patterns that warrant scrutiny)

This gives a more nuanced readiness assessment — a project with
30 null! and 5 assertions is in better shape than one with 35
assertions, even though the total is similar.

Update fixtures with null!/default! patterns and add 6 new test
assertions (25 total, up from 19).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add pre-migration effort estimate for CS8618 warnings

Count uninitialised reference-type fields and auto-properties as a
rough predictor of CS8618 warnings before NRT is enabled. Excludes
value types, events, const, and 'required' properties.

Shown only when NRT is not yet enabled, labeled as approximate.
In the corefx regex migration, CS8618 was 47% of all warnings (236
of 502), making this the single most useful pre-migration signal.

Adds 3 test assertions (28 total).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add per-file ! breakdown in scanner output

Show null!/default! vs assertion counts for each flagged file in
both human-readable and JSON output. This helps reviewers quickly
see which files have mostly safe initializers vs genuine assertions
that warrant scrutiny.

Example: 'RegexCompiler.cs: 31 ! (24 null!/default!, 7 assertions)'
tells the reviewer only 7 uses need attention, not 31.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Show per-file uninit fields pre-migration, suppress post-migration

Flag files with >5 uninitialised reference-type fields in 'Files
needing attention' so the pre-migration report shows where the most
CS8618 work will be. Suppress this metric when NRT is already
enabled since it's no longer actionable. Also skip empty lines in
per-file output when a file only triggered the uninit threshold but
NRT is enabled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Context-aware section headings in scanner output

Replace generic 'Files needing attention' with context-dependent labels:
- Pre-migration (NRT not enabled): 'Migration work needed'
- Post-migration with remaining directives: 'Remaining cleanup'
- Post-migration clean: 'Suppression audit (Step 6)' — frames !
  operators as review candidates, not problems

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove Step 6 reference from scanner output to avoid sync issues

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add before/after example output to scanner script header

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Advocate RespectNullableAnnotations as best practice for .NET 9+

Reframe guidance from neutral opt-in to prescriptive best practice,
add motivation for why it matters, and recommend companion setting
RespectRequiredConstructorParameters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add zero-IL-change principle as hard rule for NRT migration

Add prominent top-level principle: NRT migration must never change
runtime behavior or generated IL. Missing null guards should be
flagged as TODO comments (with user consent), not added inline.
Update the existing null validation guidance to align.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add TFM caveat to Debug.Assert null-state guidance

Debug.Assert acts as a nullable flow-analysis hint only on
.NET Core 3.0+ where it has [DoesNotReturnIf(false)]. On
.NET Framework/netstandard2.0 it lacks this annotation and
does not affect null-state analysis.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Revert Debug.Assert TFM caveat

The skill targets .NET Core 3.0+ where Debug.Assert has
[DoesNotReturnIf(false)]. No need to document .NET Framework
behavior since we don't encourage NRTs on older TFMs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add return-type semantic nullability guidance to NRT skill

Round-trip experiment on Dapper (51 files, 984 warnings) revealed the agent
systematically removed ? from 40 public return types (OrDefault, ExecuteScalar)
to satisfy the compiler, but those methods can return null by design. Added
explicit guidance that return types must reflect semantic nullability, not just
compiler satisfaction, with specific patterns to watch for.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Strip string literals before comments in scanner

Reorder the regex passes so strings are removed before comments.
A string like "http://..." contains // that was previously
mis-parsed as a line comment, corrupting downstream text.

Add a URL-in-string test case to the nrt-enabled fixture.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add NRT skill guidance from real-world evaluation

Tested the skill against NimblePros/eShopOnWeb (ASP.NET Core + EF Core).
Stripped NRT annotations from 44 files, re-annotated using the skill,
and compared against the originals. Found 3 areas for improvement:

ef-core.md:
- Add guidance to always pair #pragma warning disable CS8618 with
  #pragma warning restore CS8618 to prevent suppression leakage

aspnet-core.md:
- Add Razor Pages [BindProperty] initialization guidance
- Add collection property default initialization guidance
- Call out the ?.+! anti-pattern (contradictory null handling)

SKILL.md:
- Add guard clause library awareness (Ardalis.GuardClauses, Dawn.Guard
  use [NotNull] attributes that narrow null state — avoid redundant !)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Rename skill to migrate-nullable-references, wire scanner tests into eval.yaml, merge main

- Rename nullable-reference-migration -> migrate-nullable-references per CONTRIBUTING.md naming convention (lead with action verb)
- Update YAML front-matter name field
- Update scanner script and test script references to new directory name
- Add scanner script eval scenario to eval.yaml using test fixtures
- Merge latest main for CI/eval infrastructure updates

Fixes #38

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove en-us locale from learn.microsoft.com URLs in NRT skill

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove scanner eval scenario (script tests stay in Test-ScanNullableReadiness.ps1)

The scanner script scenario doesn't work as an eval scenario because the baseline
run has no access to skill resources (source: files aren't resolved when skill=null).
Scanner tests are validated separately via Test-ScanNullableReadiness.ps1.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Improve eval rubric for correctness and fix code formatting

Replace generic throughput rubric with 5 correctness-focused criteria:
- Value-type avoidance (int, bool, DateTime)
- Nullable return types for FirstOrDefault methods
- Late-init field handling (_header in CsvExporter)
- Interface/implementation consistency
- Zero-warning build

Also fix C# code formatting in setup files (namespace/class
declarations were collapsed onto single lines).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Improve migrate-nullable-references skill routing and deduplication

- Add structured USE FOR / DO NOT USE FOR / INVOKES to description
- Add multi-project solution migration order note in Step 2
- Deduplicate serialization/DTO pitfall (cross-ref Step 4)
- Shorten redundant ArgumentNullException pitfall row

Multi-model validation: Sonnet 4.8/5, GPT-5.1 4.6/5, Haiku 4.8/5 (avg 4.73)
No regressions detected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add minimal API parameter optionality guidance per halter73 review

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix contradiction in CS86xx fix guidance per eerhardt review

The warning fix table recommended adding null checks/guards, contradicting
the zero-runtime-behavior-change rule in line 35. Reworded to prioritize
annotation-only fixes (? and !) and note that null checks change behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Skill: prefer [NotNullWhen(true)] over ! at call sites

When a boolean-returning helper guarantees a nullable parameter is non-null,
annotating the parameter with [NotNullWhen(true)] is better than sprinkling
! at every call site. This is metadata-only (no behavior change) and gives
the compiler real flow information.

Based on feedback from reviewing skill-guided NRT migration output on
dotnet/roslyn#58107 — the agent used ! without comment instead of annotating
the helper method.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove WhereNotNull helper extensions; refine skill scope

- Delete helper-extensions.md (WhereNotNull was rejected as a BCL API and
  is only used internally in Roslyn)
- Update LINQ guidance to recommend OfType<T>() instead
- Add 'fixing a handful of warnings' to DO NOT USE FOR in description

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Rename Scan-NullableReadiness to Get-NullableReadiness per convention

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add ASP.NET Core + EF Core eval scenario for NRT migration

Exercises EF Core navigation property nullability, DbSet handling,
and ASP.NET model validation behavior changes when enabling NRTs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add file_not_contains assertion and file-by-file eval scenario

Add file_not_contains assertion type to skill-validator, mirroring
file_contains but asserting the value is absent. Includes 3 tests.

Add eval scenario testing file-by-file NRT migration: agent must
modify only the targeted file while leaving others unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix YAML frontmatter: use block scalar for description with colons

The colons in USE FOR:/DO NOT USE FOR:/INVOKES: caused YAML parse
failures in skill-validator. Use > block scalar syntax per lewing review.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Strengthen CS8603 return-type guidance and add return null! anti-pattern

Clarify that non-nullable return type is a promise to callers.
Add explicit warning against return null!/default! pattern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Extract nullable attributes table to optional reference

Move the System.Diagnostics.CodeAnalysis attributes table from Step 5
inline content to references/nullable-attributes.md, reducing the core
skill by ~500 tokens. The inline step retains a summary listing the
key attribute names so the agent knows what's available and when to
load the reference.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Tweak ASP.NET/EF eval scenario prompt wording

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Move skill to plugins/tests layout, add CODEOWNERS for migrate-nullable-references

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address review: remove spoon-fed build commands, refocus rubric on skill value

- Remove explicit 'Build with:' lines from eval prompts so the skill's
  Step 1 (discover build command) is actually exercised
- Reword file-by-file rubric to test the skill's migration strategy
  rather than general instruction compliance

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Larry Ewing <lewing@microsoft.com>
 2026-03-02 10:12:21 -08:00
Viktor Hofer 0b249be6b6 Rename from components to plugins 2026-02-27 20:19:46 +01:00
Jan Krivanek 00b9fb17ef [Infra] Add PoC of overfitting judging (#140) 
* Add overfitting judging

* Add the design doc

* Add async I/O

* Add overfit detection description
 2026-02-27 18:13:25 +01:00
Viktor Hofer dcafe88a5b Fix running SkillValidator on Windows locally (#138) 
The NormalizeDirectory function + the quote resulted in the quote getting escaped: `\"`. Use NormalizePath instead as we don't need the trailing slash.
 2026-02-27 14:47:43 +01:00
Jan Krivanek c595feb95d Support skill negative tests (#139) 2026-02-27 14:45:06 +01:00
Elinor Fung 3e4ba910f0 Fix quotes in RunArguments for SkillValidator (#134) 2026-02-26 11:32:40 -08:00
Jan Krivanek e381a6afd2 Fix MCP discovery (#120) 2026-02-25 14:21:31 +01:00
Jan Krivanek e2da8038d4 Add timeout info to dashboard (#119) 2026-02-25 13:45:00 +01:00
Jan Krivanek a9cf756c68 [Infra] Increase + Capture timeouts (C#) (#117) 
* Increase and capture timeouts

* Fix timeout checking

* Fix gitignore

* Improve the timeout display info

* Fix timeout indicator position in markdown table

Move the  timeout indicator after the '/5' denominator so scores
render as '1.0/5  timeout' instead of '1.0  timeout/5'.
 2026-02-25 12:04:12 +01:00
Viktor Hofer cfc22971e1 Port skill-validator from TypeScript to .NET C# (#106) 
* Port skill-validator from TypeScript to .NET C#

Replace the TypeScript skill-validator with a .NET 10 C# implementation.
Uses System.CommandLine for CLI parsing, GitHub.Copilot.SDK for Copilot
communication, YamlDotNet for YAML, and xUnit v3 with MTP v2 for testing.

All 135 unit tests ported and passing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update CI workflows for .NET skill-validator

Replace Node.js setup and npm commands with dotnet build/test/publish.
Use global.json for SDK version instead of hardcoded dotnet-version.
Publish self-contained binary for evaluate runners.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix permission denied for Copilot SDK native binary in CI

Restore execute permissions on the native copilot binary after
artifact download, which strips file permissions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* nit updates

* Apply upstream changes: --verdict-warn-only, safeDirName, fix benches

Port three origin/main commits to the C# skill-validator and workflow:

- Add --verdict-warn-only flag replacing --strict; verdict failures
  (threshold, completion, activation) become warnings while execution
  errors and --require-evals missing_eval failures remain fatal.
- Add FailureKind tracking to SkillVerdict and Comparator.
- Add SafeDirName sanitizer for skill directory names in Reporter.
- Add per-skill verdict.json generation in JSON reporter.
- Update evaluation.yml: remove continue-on-error, use --verdict-warn-only,
  change default infra runs to 2, skip comment-on-pr on cancellation,
  use ResultsFile for benchmark data generation.
- Update generate-benchmark-data.ps1 to read verdicts from results.json.
- Update README with new CLI flags and CI example.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update CONTRIBUTING.md for .NET skill-validator

Replace Node.js/npm prerequisites and commands with .NET 10 SDK
equivalents (dotnet run).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update test commands in CONTRIBUTING.md to use .csproj file

* Add RunArguments for default results-dir and verbose logging

- Set RunArguments in SkillValidator.csproj using MSBuild NormalizeDirectory
  to pass --results-dir pointing to artifacts/TestResults/skill-validator/
- Log configured results dir at startup when --verbose is enabled

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix path

* Fix AgentRunner: capture tool completion events and implement permission checking

- Add ToolExecutionCompleteEvent handler to capture Success/Result data
  from tool completions, fixing timeline showing all tools as FAIL
- Add ToolExecutionStartEvent arguments capture for timeline detail
- Implement CheckPermission with path extraction from PermissionRequest
  ExtensionData, replacing the stub that always returned true
- Update Judge and PairwiseJudge to pass PermissionRequest directly

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix CheckPermission tests for cross-platform CI (Linux)

Use Path.GetTempPath() and Path.Combine() instead of hardcoded Windows
paths so tests pass on both Windows and Linux CI runners.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Capture skills & tokens usages

* Fix thread safety, resource leaks, and null safety in skill-validator

- Use ConcurrentBag and SemaphoreSlim with double-check locking for
  _workDirs and _sharedClient in AgentRunner (race conditions under
  parallel runs/scenarios)
- Make Spinner IDisposable; protect _frame/_message/_active with lock
  and Interlocked; use 'using var' at call site
- Use 'using var' for timeout Timers in Judge and PairwiseJudge to
  prevent leaks on exception paths
- Replace perRunPairwise[0] fallback with FirstOrDefault() to avoid
  IndexOutOfRangeException when list is empty

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update eng/skill-validator/README.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Address PR review feedback

- Add per-type required field validation in ParseAssertion (e.g.
  file_contains requires both path and value, output_matches requires
  pattern)
- Fix JsonDocument leaks in LlmJson: use 'using' and Clone() on
  RootElement in both ParseLlmJson and TryParseJson
- Pin Microsoft.Extensions.FileSystemGlobbing to 10.0.0 instead of
  floating 10.* for reproducible builds
- Fix README Development section to reference SkillValidator.slnx
  instead of .sln

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
 2026-02-25 09:06:26 +01:00
Jan Krivanek 1e2df36e2d Add test for binlog creation skill (#109) 
* Add test for binlog creation skill

* Fix regex pattern for binlog generation assertions

* Reflect PR comments

* Improve test cases
 2026-02-24 17:13:28 +01:00
Jan Krivanek 3e07557cac Add tests for binlog-failure-analysis skill (#107) 
* Add tests for binlog-failure-analysis skill

* Add support for using MCP within eval tests

* Update the skill and testcase - to better target the usage

* Change maxLevels default value from 4 to 2
 2026-02-24 17:12:54 +01:00
Viktor Hofer bfcbe4d05b Add --verdict-warn-only option to skill-validator for non-fatal skill failures (#111) 
* Add --warn-only option to skill-validator for non-fatal skill failures

This avoids the continue-on-error setting the yml which sometimes hides real issues.

* Change default runs from 1 to 2 in evaluation.yml

Updated the number of runs to 2 for better testing balance.

* Rename --warn-only to --threshold-warn-only and fail on execution errors

- Rename --warn-only to --threshold-warn-only to clarify it only applies
  to threshold-based verdict failures, not execution errors.
- Track rejected evaluation promises and return exit code 1 for execution
  errors even in --threshold-warn-only mode.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove redundant --strict option, clarify --threshold-warn-only

--strict only implied --require-evals, which already exists as its own
flag. Remove --strict to avoid confusion about its interaction with
--threshold-warn-only. Update --threshold-warn-only help text to clarify
it only suppresses threshold failures, not runtime/config errors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Only post PR comment when evaluate matrix succeeds

When a new push triggers cancel-in-progress, the evaluate matrix jobs
get cancelled but comment-on-pr still ran due to always(). This caused
partial/stale results to be posted. Require evaluate to succeed before
posting the PR comment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add --verdict-warn-only flag with failureKind tracking

--verdict-warn-only treats verdict failures (threshold, completion
regression, skill not activated) as warnings (exit 0), while execution
errors and --require-evals missing_eval failures remain fatal.

Add failureKind field to SkillVerdict to categorize failure reasons.
Remove --strict (redundant with --require-evals).
Update README and evaluation.yml with documentation.
Allow comment-on-pr to run on evaluate failure (skip only on cancel).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-02-24 14:08:31 +01:00
Viktor Hofer e4ea5b6225 Refine parameter description in generate-benchmark-data.ps1 and add s… (#110) 
* Refine parameter description in generate-benchmark-data.ps1 and add safeDirName function to sanitize skill names in reporter.ts

* refactor: make safeDirName a true sanitizer instead of a throwing validator

- Slugify skill names for directory use instead of hard-failing on
  unexpected characters from SKILL.md frontmatter display names.
- Use path.basename() for robust single-segment check instead of
  string.includes('..') which false-positives on names like 'foo..bar'.
- Throw on genuinely invalid names (empty, '.', '..', or all-special-chars).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-02-24 12:57:52 +01:00
Viktor Hofer 3c8499de55 Fix empty benches arrays in benchmark data generation (#108) 
* Fix empty benches arrays in benchmark data generation

The json reporter lost per-skill verdict.json generation during a
refactor (b461016), causing generate-benchmark-data.ps1 to find no
verdict files and produce empty benches arrays.

Fix by:
- Restoring verdict.json generation in the json reporter
- Updating generate-benchmark-data.ps1 to read verdicts directly from
  results.json instead of scanning for per-skill verdict.json files

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Simplify generate-benchmark-data to accept ResultsFile path directly

Replace ResultsDir parameter with ResultsFile since the script only
needs the results.json path. Update evaluation.yml caller accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update eng/dashboard/generate-benchmark-data.ps1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
 2026-02-24 12:38:21 +01:00
Viktor Hofer f9cf1d6d12 Fix extractJson picking up non-JSON brace groups (e.g., C# code) (#100) 
* Fix extractJson picking up non-JSON brace groups (e.g., C# code)

extractJson now validates each brace-matched candidate with JSON.parse
before returning it. If the candidate isn't valid JSON (even after
escape sanitization), it skips to the next brace group. This prevents
C# code snippets or other non-JSON content from being mistakenly
extracted as the judge's JSON response.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add test for skipping brace groups with invalid escapes

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-02-24 09:22:55 +01:00
Jan Krivanek 2e3619585b [Infra] Add activated skills info (#98) 
* Set config dir to work dir for skills isolation

* Add skills activation info

* Reflected PR comments

* Fix after merge
 2026-02-24 08:47:35 +01:00
Viktor Hofer ac6209e666 [Infra] Fix JSON parse error from invalid LLM escape sequences in pairwise judge (#99) 
* Fix JSON parse error from invalid LLM escape sequences in pairwise judge

Wrap JSON.parse in try/catch and retry after stripping invalid escape
sequences (e.g., \' or \a) that LLMs sometimes produce in reasoning text.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Address PR feedback: improve error context and add parser tests

- Preserve original error and direction in all failure paths
- Only attempt escape sanitization when invalid escapes are detected
- Include JSON snippet in error messages for debugging
- Export parsePairwiseResponse for direct testing
- Add 7 tests covering valid JSON, invalid escapes, malformed JSON,
  missing JSON, and direction reversal

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Extract shared LLM JSON parsing into llm-json.ts

- Create llm-json.ts with extractJson() and parseLlmJson() helpers
- Refactor judge.ts and pairwise-judge.ts to use shared helpers
- Remove duplicated extractOutermostJson and escape-sanitization logic
- Add 11 tests for the shared module (extractJson + parseLlmJson)
- Add 8 tests for judge.ts parseJudgeResponse

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-02-23 23:18:14 +01:00
Viktor Hofer ae222c23a4 [Infra] Improve evaluation workflow (#97) 
* feat: update build-validator job and README for Node.js version requirement

- Update an existing comment if it exists with the total summary
- Build the skill-validator just once and upload
- Clean-up when passing CLI args to skill-validator

* Update .github/workflows/evaluation.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Prune dev dependencies before uploading validator artifact

Reduces artifact size by removing devDependencies (typescript, vitest, etc.)
from node_modules/ since the evaluate job only needs the built dist/.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Updates

* Consolidate step summary into comment-on-pr job

Move GITHUB_STEP_SUMMARY writing from per-matrix Generate Summary step
into the existing Consolidate summaries step in comment-on-pr job.
This produces a single unified summary instead of fragmented per-component
summaries. The job now runs for all event types, with the PR comment
step scoped to pull_request events only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Refactor reporters to use --results-dir for file output

- Remove --no-save-results option and saveRunResults function
- Reporters no longer accept inline paths (e.g., json:path)
- Add --results-dir option: when specified, file reporters (json, junit,
  markdown) write to that directory; when omitted, they write to console
- Enhance markdown reporter with per-scenario judge reports including
  rubric scores, reasoning, and agent output
- Update workflow, README, and CONTRIBUTING docs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Default --results-dir to .skill-validator-results and enable json/markdown reporters by default

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix unresolved matrix expression in skipped evaluate job name

Replace if: has_entries with if: !cancelled() && !failure() so the job
proceeds with an empty matrix, creating zero instances instead of a
skipped job with unresolved expressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update required nodejs version in CONTRIBUTING doc

* Address PR feedback: fix timeout message, improve reporter output handling

- Use effective timeout value in error message to avoid 'undefineds'
- Only create resultsDir when file-based reporters are selected
- Write results into timestamped subdirectory (yyyyMMdd-HHmmss) to avoid stale output
- Use scenario-level improvementScore for markdown verdict icons
- Update workflow to find summary.md in timestamped subdirs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update bin-obj clash skill

* Fix PR comment step: use gh api instead of gh pr comment

gh pr comment requires a git repo checkout, but comment-on-pr job
only downloads artifacts. Switch to gh api for creating comments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Touch one more skill

* Address PR feedback: remove dead code, fix formatting, improve workflow

- Remove dead validation check in cli.ts (resultsDir always has default)
- Fix missing spaces in variable declarations in reporter.ts
- Quote command substitution in consolidation script
- Use explicit needs.build-validator.result == 'success' condition
- Strip duplicate headers when consolidating multi-component summaries

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Replace markdown concatenation with consolidate subcommand

Instead of fragile bash string manipulation to combine per-component
summary.md files (which caused duplicate headers, empty tables, and
repeated model lines), add a 'consolidate' subcommand that reads
results.json files and generates a single clean markdown summary.

The subcommand takes explicit file paths as arguments — the caller
controls exactly which files to merge. Runner-specific concerns
(comment markers, run URLs) stay in the workflow script.

- Extract generateMarkdownSummary() from reportMarkdown() in reporter.ts
- Add 'skill-validator consolidate' subcommand in cli.ts
- Update workflow to use the new subcommand
- Document consolidate in README.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-02-23 21:07:41 +01:00