mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
eval-exec-opus48
56 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Amaury Levé
|
77154137e8 |
dotnet-test: make code-testing agent tools Claude Code-compatible + add cross-host portability check (#856)
* dotnet-test: make code-testing agent tools declarations Claude Code-compatible PR #847 added `tools: ["agent", "skill", "read", "search", "edit", "execute"]` to the code-testing-* agents to enable VS Code / Copilot CLI subagent fan-out. Those lowercase aliases map to real tools in VS Code and the Copilot CLI, but Claude Code matches `tools:` against its own vocabulary (Task, Skill, Read, Glob, Grep, Edit, Write, Bash). None of the aliases matched, so when these agents are loaded into Claude Code via --plugin-dir and selected with `claude --agent`, the agent was granted ZERO tools. A tool-less model asked to generate tests emits a textual <tool_call> block and exits after one turn, producing no file changes. Append the Claude Code tool names to each agent's `tools:` list so the same declaration works across all three runtimes (each honors the names it knows and ignores the foreign ones): - Orchestrators (generator, implementer): add Task, Skill, Read, Glob, Grep, Edit, Write, Bash (Task is the Claude Code equivalent of the `agent` fan-out tool). - Workers (researcher, planner, builder, tester, fixer, linter): add Skill, Read, Glob, Grep, Edit, Write, Bash. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * skill-validator: complete built-in tools + add cross-host tool portability check Two related follow-ups to the agent tools fix: 1. Address the skill-check review feedback. The validator's BuiltInTools set was missing three legitimate host tool spellings that are not case-insensitive matches of existing entries, so they were flagged as non-built-in: - "write" — Claude Code file-creation tool (Copilot CLI / VS Code: "create") - "agent" — Copilot CLI / VS Code subagent fan-out tool (Claude Code: "task") - "execute" — Copilot CLI / VS Code run-command tool (Claude Code: "bash") "agent" and "execute" were already flagged before this branch (introduced by the fan-out PR); adding them to BuiltInTools clears the pre-existing warnings. 2. Add a cross-host tool portability check (CheckAgentToolPortability) so an agent that declares a capability for only one host is flagged. Tool names are matched case-sensitively (hosts resolve tools by exact spelling), so an agent that lists e.g. only "edit" (Copilot / VS Code) without "Edit"/"Write" (Claude Code) is reported as working on one host and silently tool-less on the other. Findings are advisory (do not fail CI) and allowlistable via "agent-tool-portability:AGENT:capability". Wired into the agents loop in CheckCommand and covered by unit tests. Also make the one existing single-host agent (optimizing-dotnet-performance) portable by adding its Claude Code tool spellings, so the new check reports a clean tree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Amaury Levé
|
592a53008e |
skill-validator: restore 15K aggregate cap as the real Copilot CLI skill-menu budget (#803)
* skill-validator: restore 15K aggregate cap, document it as the real Copilot CLI skill-menu budget The per-plugin aggregate description cap had been raised 15,000 -> 20,000 -> 22,000 under the belief that 15K was 'a local repo policy, NOT a documented Copilot constraint'. That belief was wrong: the GitHub Copilot CLI renders the model-facing <available_skills> menu under a hard 15,000- char budget (the agent SDK's SKILL_CHAR_BUDGET, default 15e3, confirmed in CLI 1.0.36 and 1.0.61). Skills are listed alphabetically and emitted with their full <description> only until the budget is exhausted; every skill past the cut-off collapses to a bare name with no description and can no longer be reliably model-activated. Raising the validator cap merely masked this silent menu truncation — e.g. dotnet-test's run-tests and test-* skills stopped activating in plugin eval runs because they fell into the name-only overflow. Changes: - SkillProfiler.MaxAggregateDescriptionLength: 22,000 -> 15,000, with the comment rewritten to document the real Copilot CLI budget (and correct the prior 'not a documented constraint' claim). - CheckCommand aggregate now excludes skills marked 'disable-model-invocation: true' — the CLI drops those from the menu, so they do not consume the budget. This makes the cap satisfiable by hiding reference / agent-orchestrated primitives rather than only by trimming. - InvestigatingResults.md: document plugin-arm-only non-activation caused by skill-menu budget overflow, and how to fix it. Note: dotnet-test currently exceeds 15K and must be slimmed below it (via disable-model-invocation on reference/primitive skills plus description trims) before this cap can go green repo-wide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * skill-validator: use source-generated regex for disable-model-invocation check Address review: replace Regex.IsMatch(pattern-string) with a [GeneratedRegex] partial method (AOT-friendly, no per-call cache lookup), matching FrontmatterParser's style. Runs once per skill during checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * skill-validator: parse disable-model-invocation via YAML to avoid block-scalar false positives The regex-based check matched any line in the frontmatter, so a block-scalar description that merely mentioned 'disable-model-invocation: true' on its own line was wrongly treated as disabling model invocation. Parse the frontmatter with the existing YAML deserializer (which correctly handles block scalars) by adding a DisableModelInvocation field to SkillFrontmatter, and drop the regex entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
YuliiaKovalova
|
76a5ec8cb3 |
Decouple agent runs from judging in skill-validator (--no-judge + cross-dir rejudge) (#769)
* Decouple agent runs from judging in skill-validator Allow baseline and treatment agent runs to execute in one parallel pool, with judging deferred to a final step, removing the serial precompute-baseline -> evaluate barrier. evaluate --no-judge: runs the requested agent arms and persists sessions.db + metrics, but skips all judging and requires no baseline file (exits 0 on successful runs). Forces session persistence; mutually exclusive with --baseline-out/--baseline-from; requires --results-dir. Only the agent model is validated (judge model recorded as metadata); warns when zero runs persist. rejudge --baseline-dir: cross-directory judging that pairs each treatment run with its baseline run by the BaselineStore key (prompt SHA + target SHA, persisted in a new sessions.db baseline_key column), runs the same pairwise/independent judges and pass/fail gates evaluate uses, and writes verdicts/reports. Baseline and treatment must share --model (rejected if either side mixes models); effective judge model resolves to --judge-model, else the treatment DB's, else the baseline DB's, rejecting a persisted mismatch. The inline per-run judging is extracted into a shared JudgeRunGroup helper reused by both the same-dir and cross-dir paths (no duplicated judging logic). Adds tests for baseline_key round-trip + legacy migration, ComputeScenarioKey, cross-dir pairing, and model/judge-model compatibility. Updates README and InvestigatingResults docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Hoist ComputeScenarioKey to scenario level to avoid per-run fixture hashing Compute the baseline key once per scenario instead of once per run, so multi-run evaluations (--runs N) don't re-hash fixture files N times. The key is threaded into ExecuteRun/ExecuteAgentRun as a parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Memoize scenario-key fixture hashing for the duration of an evaluation Route the persisted baseline_key through an evaluation-scoped BaselineStore cache (ComputeScenarioKeyCached) instead of the static ComputeScenarioKey, so a scenario's setup fixtures are hashed at most once across every run, arm, and scenario of an evaluation. When a baseline store already exists (--baseline-from /--baseline-out) its warm input-hash cache is reused; otherwise a dedicated, side-effect-free cache (BaselineStore.ForKeyCache) is threaded through. Addresses the PR review comment about ComputeScenarioKey re-hashing large fixture directories. Adds a test asserting the cached variant equals the canonical static computation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump session schema version to 3 for the baseline_key column The baseline_key column was added without bumping schema_info.version, so external tools keying off the version couldn't detect the new column. Introduce a single SchemaVersion constant (= 3), stamp it after migrations run, and drop the drift-prone version literal from the raw CREATE SQL so SetSchemaInfo is the sole source of truth. Update the schema-info and legacy-upgrade tests to expect version 3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fail fast on judging-dependent options under --no-judge; fix README prefix Under --no-judge the noise test and overfitting fix cannot run (they require judging), so reject --noise-skills-dir and --overfitting-fix up front instead of silently ignoring them, mirroring the existing --baseline-out/--baseline-from guard. Add EvaluateCommandTests covering these early rejections (they short- circuit before any model/network call). Also prefix the decoupled-runs README example with 'skill-validator' for consistency with the other CLI invocations in the doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
YuliiaKovalova
|
bcaa91882c |
Add reusable shared baseline to skill-validator evaluate (--baseline-out / --baseline-from) (#754)
* Add shared/precomputed baseline reuse to skill-validator evaluate (#751) Add --baseline-out and --baseline-from options to the evaluate command so the no-skill/no-agent baseline arm can be computed once and reused as a shared control group across multiple skill/agent evaluations. This eliminates redundant baseline runs and removes baseline run-to-run variance from cross-config comparisons. - New BaselineStore + BaselineFile/BaselineScenarioEntry models, keyed per scenario on SHA-256(prompt) with a header recording version, model, validator version and runs. Load validates version + model and fails fast on mismatch or missing scenarios. - Register the new serializable types in SkillValidatorJsonContext (AOT source-gen). - Wire two mutually-exclusive CLI options into ValidatorConfig; thread an optional BaselineStore through both execution paths. - On reuse, skip the baseline agent run, its assertions/constraints/ task-completion/judging, and attribute no extra pairwise tokens to the baseline; report the scenario with the baseline-reused session phase and a reused status. In write mode, record each scenario's averaged baseline and persist it after the run. - Add unit tests for BaselineStore and document the feature in README and InvestigatingResults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bind reused baseline to fixture identity (targetSha), not just prompt Align baseline reuse with the (prompt, model, targetSha) identity contract from the upstream eval-harness design: previously the per-scenario reuse key was the prompt SHA + model only, so two scenarios that share an identical prompt but feed the agent different input artifacts (e.g. a different build.binlog) would collide and silently reuse the wrong baseline. - Add BaselineScenarioEntry.TargetSha: a SHA-256 over the scenario's materialized inputs — files auto-copied via copy_test_files, explicit setup files (inline content or copied sources), and the setup command recipe. The reuse key is now (promptSha, targetSha); both must match. Bump the on-disk schema to version 2. - Memoize target hashing per process via a cheap, file-I/O-free setup signature to avoid re-hashing large fixtures across the N runs. - Thread the originating eval.yaml path into Record/TryGetBaseline/ FindMissingScenarios so inputs can be fingerprinted. - Tests: target SHA is stable and content-sensitive; same-prompt/different-fixture scenarios do not reuse each other's baseline and are surfaced by FindMissingScenarios. Update README and InvestigatingResults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden baseline reuse identity after review Address rubber-duck review findings on the shared-baseline feature: - Fix Sha256Hex 32-byte bug: a 32-byte input was treated as an already-computed digest and not hashed. Split into Sha256Hex (always hash) + HexDigest (encode existing digest). - Broaden reuse identity: the cached baseline RunResult depends on the judge model and on per-scenario evaluation criteria (rubric, assertions, expect/reject tools, turn/token/timeout limits). Add JudgeModel to the baseline header (validated on load) and fold the criteria into the per-scenario targetSha so changing them invalidates reuse instead of silently serving a stale result. - Mirror AgentRunner.SetupWorkDir exactly when hashing copied fixtures: exclude only the top-level eval.yaml (nested eval.yaml files are copied, so they must be hashed). - Make the target-SHA cache instance-scoped (memoizing only the expensive fixture-input hashing) so it can't serve stale hashes or leak across evaluations/tests; hash inline file Content in the cache key instead of embedding it. - Deterministic Save ordering (Name, PromptSha, TargetSha); guard Load against null Scenarios; enrich FindMissingScenarios output with the eval path. - Document that setup commands are fingerprinted by recipe, so reuse assumes they are deterministic/hermetic. - Tests + docs updated; add judge-model-mismatch and criteria-identity tests (562 pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review comments on baseline reuse - Mirror AgentRunner.SetupWorkDir exactly when hashing copied fixtures: enumerate only the files actually copied (top-level siblings except eval.yaml, recursing into directories) and skip reparse points and out-of-root junctions, instead of blindly hashing every file under the eval directory. This keeps the fixture identity restricted to the intentionally-copied set so stray output/log files can't poison reuse. - Stream baseline JSON to/from disk (File.OpenRead/File.Create with JsonSerializer) so large baselines never materialize as one giant in-memory string. - Enrich the fail-fast 'missing scenario' output with the eval path and short prompt/target SHA prefixes so it is actionable when scenario names collide across eval files. - Add a test locking in recursive (nested-directory) fixture hashing. 563 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address second round of PR review comments - Skip top-level reparse points in EnumerateCopiedFixtures (not just nested ones) so a top-level symlink/junction can't cause hashing of data outside the eval directory; code now matches the docstring. - Record uses first-writer-wins (TryAdd) instead of overwriting, so a scenario identity recorded by multiple parallel targets yields a deterministic --baseline-out regardless of completion order. - Persist the baseline judge result to the session DB even when the baseline is reused, so the registered 'baseline-reused' session record is complete for downstream investigation tooling (pairwise was already saved; the judge result was incorrectly gated on a fresh run). - Add first-writer-wins test (564 pass). Note: BaselineStore stays internal — the test project already has InternalsVisibleTo, so it compiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Pin MessagePack to 2.5.301 to fix NU1903 vulnerability The transitive MessagePack 2.5.198 (via GitHub.Copilot.SDK -> StreamJsonRpc) has a high-severity vulnerability (GHSA-hv8m-jj95-wg3x) that fails the build under TreatWarningsAsErrors. Pin a direct reference to the patched 2.5.301. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: bare-filename fixture hashing, clone reused baseline, consistent token attribution - ComputeInputsSha: normalize evalPath via Path.GetFullPath so a bare filename still hashes sibling fixtures (avoids TargetSha collisions / unsafe reuse). - RunMetrics.Clone(): per-run copy with fresh collections; reuse paths now clone the cached baseline so concurrent evaluations never share a mutable instance. - Pairwise judge tokens attributed to both compared runs in every mode (the baseline clone makes this safe), keeping token deltas comparable across --baseline-from modes. - Reword Record first-writer-wins doc to describe the within-run stabilization guarantee rather than order-independence. - Add tests for bare-filename fixture hashing and clone isolation (566 pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Aaron Powell
|
3134b816de |
Add JSON output for skill-validator check (#601)
* Add JSON output for skill-validator check Fixes #600 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix empty check discovery results Address PR feedback by failing when explicit skill or agent paths discover nothing, including the combined check path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot review comments - Replace AddPlainError with AddGeneralError (remove duplicate method) - Key pluginSkills by DirectoryPath instead of display name to avoid mismatch when plugin.Name differs from directory name - Use OS-aware path comparison in IsPathWithin (Ordinal on Unix, OrdinalIgnoreCase on Windows) - Pre-build name lookup dictionaries in CreateJsonOutput to eliminate O(n*m) FirstOrDefault scans for external dependency attachment - Extract CheckJsonSerializerContext scoped to check JSON output so UseStringEnumConverter does not affect unrelated JSON payloads Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address latest Copilot PR feedback - Attach external dependency warnings by stable path identifiers (skill SKILL.md path, agent path, plugin directory path) - Use OS-aware comparer for plugin directory path matching - Restore console warning formatting via SkillProfiler.FormatProfileWarnings - Move CheckJsonSerializerContext into SkillValidator.Check namespace - Precompute reference-attachment targets to avoid repeated full-path normalization and container recalculation per finding - Add regression test covering duplicate skill names with external dependency warnings in JSON output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix flaky duplicate-skill JSON test path - Run duplicate-name external dependency assertion through plugin mode, which is where external dependency checks are executed - Add plugin fixture helper for JSON output tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address latest Copilot follow-up feedback - Reintroduce PluginValidationResult as obsolete compatibility type while keeping PluginCheckResult as the primary model - Avoid unnecessary profile-line formatting when verbose output is off - Replace string discriminators for external dependency kind with enum - Centralize JSON warning kind literals as constants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jan Krivanek
|
92954153dd |
Bump GitHub.Copilot.SDK to 0.3.0 and address breaking changes (#659)
* Bump GitHub.Copilot.SDK to 0.3.0 and address breaking changes - GitHub.Copilot.SDK 0.2.2 → 0.3.0 - YamlDotNet 16.3.0 → 17.1.0 - Vecc.YamlDotNet.Analyzers.StaticGenerator 16.3.0 → 17.0.0 - Nerdbank.MessagePack pinned to 1.1.62 (GHSA-2cwq-pwfr-wcw3) Breaking changes addressed: - LocalSessionFsHandler now extends abstract SessionFsProvider (simplified method signatures with raw parameters) - McpServers config uses McpStdioServerConfig instead of Dictionary<string,object> - SessionFsSetProviderRequestConventions → SessionFsSetProviderConventions - PermissionRequestResultKind.DeniedByRules → .UserNotAvailable (obsolete) - Mtime/Birthtime now DateTimeOffset instead of string - Entry/EntryType → SessionFsReaddirWithTypesEntry/EntryType * Address review: validate MCP server type, suppress YamlDotNet trim warning - Reject non-stdio MCP server types with a clear error message instead of silently treating them as stdio. - Suppress IL2104 (YamlDotNet 17.x not fully trim-compatible) so that PublishAot native builds succeed. |
||
|
Jan Krivanek
|
b9d343f3b8 | Isolate failures during eval and judging (#611) | ||
|
Tomas Grosup
|
1613c98fe5 |
Honor --allow-repo-traversal for /-prefixed file references (#604)
/-prefix paths bypassed the flag and hit the depth check unconditionally. Now treated symmetrically with .. traversal: allowed only with the flag, otherwise produce a clear absolute (repo-rooted) path error. Also classify // as a protocol-relative URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jan Krivanek
|
6ebb5616b2 | Support checking skills and agents at single invocation (#533) | ||
|
Jan Krivanek
|
e6f416848c | Fix the repo path traversal check (#525) | ||
|
Jan Krivanek
|
292a4bcfa3 |
Add --allow-repo-traversal and remove body line count checks (#524)
* Add validator options * Address PR review: validate options and add tests - Reject --allow-repo-traversal with --plugin mode (plugins must be portable) - Validate --max-declaration-lines and --max-agent-lines are positive integers - Add unit tests for MaxDeclarationLines override in SkillProfiler - Add unit tests for AllowRepoTraversal suppression and depth check preservation - Add unit tests for MaxAgentLines override and fallback to MaxDeclarationLines * Remove body line count checks and --max-declaration-lines/--max-agent-lines options Line count checks are redundant with BPE token-based complexity classification, which already provides better guidance. Keep --allow-repo-traversal only. * Address review: remove unused CheckOptions from AgentProfiler, fix traversal test - Remove unused CheckOptions parameter from AgentProfiler.AnalyzeAgent and RunAgentsCheckCore since line checks were removed - Fix AllowRepoTraversalSuppressesParentTraversalError test to use a non-deep path (../SKILL.md) and assert no file-ref errors at all |
||
|
Viktor Hofer
|
3fff1f2573 |
Skill-validator: perf, cancellation, and reliability improvements (#482)
* Skill-validator: perf, cancellation, and reliability improvements Performance: - Cache skill-validator-dist.tar.gz in CI (evaluation.yml and skill-check.yml) to skip .NET SDK setup, build, and tar on cache hit. Evaluate and comment-on-pr jobs restore from cache with artifact download fallback. - Replace SemaphoreSlim with Lock in SessionDatabase for lighter-weight synchronous locking. Cancellation: - Thread CancellationToken from System.CommandLine entry point through the entire evaluate call chain: Run -> EvaluateTarget -> EvaluateSkill/EvaluateAgent -> ExecuteScenario/ExecuteAgentScenario -> ExecuteRun/ExecuteAgentRun, plus EvaluateSkillNoise -> ExecuteNoiseTest. - Add CancellationToken parameter to AgentRunner.RunAgent, Judge.JudgeRun, PairwiseJudge.Judge, and OverfittingJudge.Analyze, forwarding to RetryHelper and LlmSession.SendAsync respectively. - Pass CancellationToken to all ConcurrencyLimiter.RunAsync call sites so semaphore waits are cancellable. - Re-throw OperationCanceledException at the top-level skill evaluation loop so Ctrl+C / CI cancellation propagates instead of being swallowed as a skill failure. Reliability: - Smart timeline truncation in Judge and PairwiseJudge: error events (session.error, runner.error) are always preserved during truncation regardless of position, with a summary marker showing event type breakdown. - Add jitter (±25%) to RetryHelper exponential backoff to prevent thundering herd on rate-limit recovery. - Add CancellationToken parameter to ConcurrencyLimiter.RunAsync<T>. - Add ConcurrencyLimiter tests (6 tests) and RetryHelper jitter tests (2 tests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review feedback - RetryHelper: re-clamp delay after jitter so it never exceeds the remaining budget or MaxSingleDelayMs. - Judge/PairwiseJudge: handle edge case where error count alone exceeds MaxTimelineEvents by trimming errors with a summary instead of unconditionally appending all of them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add high-variance detection and refactor FailureKind to enum - Add Statistics.CoefficientOfVariation() for measuring score consistency - Flag scenarios with CV > 0.5 as high-variance in ScenarioComparison - Show console warning and markdown footnote for high-variance scenarios - Include high-variance scenario names in Comparator verdict reason - Refactor FailureKind from magic strings to strongly-typed enum with JsonStringEnumMemberName attributes for backwards-compatible JSON output - Add 6 new CV unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Copilot
|
d8d3f3d369 |
Fix duplicate entries in Skills Loaded (and Agents Invoked) columns of evaluation table (#462)
* Initial plan * Fix duplicate entries in Skills Loaded column of evaluation table Agent-Logs-Url: https://github.com/dotnet/skills/sessions/2430176d-bf62-461d-bb79-781e34d7d084 Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com> * Remove accidentally committed .nuget binary Agent-Logs-Url: https://github.com/dotnet/skills/sessions/2430176d-bf62-461d-bb79-781e34d7d084 Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com> |
||
|
Jan Krivanek
|
0e47ef6545 | Add more static checks (#453) | ||
|
Jan Krivanek
|
c94bc5cdc3 |
Add Ability to author agent evals (#436)
* Add ability to author agent evals * Adjust |
||
|
Jan Krivanek
|
22542a5d75 |
Track and show subagents invocations (#427)
* Show subagents invocations * Address PR feedback: fix XML doc and sanitize agent names in markdown - Update ExtractSubagentActivation XML doc to accurately describe that all subagent event types are counted (not just Started/Completed) - Add markdown table cell sanitization for agent names in FormatSubagentCell to prevent injection via pipe chars or newlines in plugin-controlled content - Extract SanitizeMarkdownTableCell helper for reuse |
||
|
Viktor Hofer
|
b92dde1854 |
Refactor skill-validator to vertical slice architecture (#429)
Reorganize the skill-validator project from layer-based grouping (Commands/, Services/, Models/, Utilities/) to feature-based slices (Check/, Evaluate/, Consolidate/, Shared/). Key changes: - Check/: CheckCommand, SkillProfiler, AgentProfiler, PluginValidator, ExternalDependencyChecker, ReferenceScanner - Evaluate/: EvaluateCommand, RejudgeCommand (now a subcommand of evaluate), AgentRunner, Judge, PairwiseJudge, and other eval services - Consolidate/: ConsolidateCommand - Shared/: Models, SkillDiscovery, PluginParser (extracted from PluginValidator), Reporter, and utilities Split PluginValidator: parsing helpers (ParsePluginJson, TryGetSafeSubdirectory) moved to Shared/PluginParser.cs; validation logic stays in Check/PluginValidator.cs. Move RawFrontmatter and RawAgentFrontmatter types from EvalSchema into SkillDiscovery since they are only consumed by discovery. Wire RejudgeCommand as subcommand: skill-validator evaluate rejudge. Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Viktor Hofer
|
d97128e5d9 |
Move static analysis checks from eval to check command (#425)
* Move static analysis checks from eval to check command Remove SkillProfiler.AnalyzeSkill from the evaluate command's per-skill flow. All spec conformance checks (name, description, compatibility, body line count, file reference depth, token size warnings, etc.) are already enforced by the check command — running them again in eval was redundant. The eval-specific 'skill name in eval prompt' check is extracted into EvaluateCommand.ValidateEvalPrompts and kept in eval since it requires eval config context. The evalConfig parameter is removed from SkillProfiler.AnalyzeSkill along with the dead ResourceFileCount field. Tests updated to exercise ValidateEvalPrompts directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [WIP] [WIP] Address feedback: Remove ProfileWarnings from vedict model (#426) * Initial plan * Remove ProfileWarnings from verdict model and reporter output Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/skills/sessions/9b7c3d0a-1693-4238-a397-2200f83634b5 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> --------- Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> |
||
|
Viktor Hofer
|
c4c26175e2 |
Integrate reference scanner into skill-validator CheckCommand (#422)
* Integrate reference scanner into skill-validator CheckCommand - Port reference scanner logic from scan.ps1 to C# ReferenceScanner service - Add --known-domains option to CheckCommand for reference scanning - Move known-domains.txt and allowed-external-deps.txt to eng/ - Remove standalone scan.ps1 and reference-validation.yml workflow - Reference scanning now runs as part of skill-check.yml via skill-validator - Add 34 unit tests for ReferenceScanner (all 463 tests pass) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR feedback: repoRoot derivation, early validation, .agents scanning, SRI policy - Derive repoRoot from --known-domains file path instead of cwd - Fail early with clear error when known-domains file is missing - Add .agents/**/*.md scanning to DiscoverFiles (regression fix) - Fix SRI policy loophole: SRI suppresses SCRIPT-NO-SRI only, domain and HTTPS checks still apply - Use actual known-domains file path in error messages instead of hardcoded eng/known-domains.txt - Update doc comments to reflect full set of scanned paths - Add tests for .agents discovery and SRI domain enforcement Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove repo-specific paths from ReferenceScanner DiscoverFiles now only scans directories it is given -- no hardcoded knowledge of .agents, agentic-workflows, eng, or README.md. The tool is repo-agnostic; callers control what gets scanned via --plugin dirs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Drop repoRoot from ReferenceScanner API ScanFile/ScanFiles no longer take a repoRoot parameter. Finding paths use the file path as provided by the caller, keeping the scanner completely decoupled from any repo layout assumptions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Prune stale domains and add json.schemastore.org to known-domains.txt Removed 10 entries no longer referenced by any plugin content: agentskills.io, visualstudio.com, mcr.microsoft.com, dotnet.github.io, github.com/dotnet/docs, github.com/dotnet/dotnet-api-docs, github.com/dotnet/skills, github.com/github/gh-aw, github.com/modelcontextprotocol/csharp-sdk, developers.openai.com. Added json.schemastore.org (referenced by dotnet-template-engine). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix case-insensitive regex, update docs, add tests - Make UrlRegex and PipeToShellRegex case-insensitive to match the original PowerShell behavior (HTTP://, CURL, Wget all now detected) - Update DiscoverFiles doc comment to remove stale 'individual files' claim - Narrow CONTRIBUTING.md scope description to match actual plugin-only scanning - Add tests for mixed/upper-case URL and pipe-to-shell detection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove IPv6 loopback support, fix pipe-to-shell allowlist case sensitivity - Remove Ipv6LoopbackRegex and IPv6 test data (not needed) - Add case-insensitive + culture-invariant matching for pipe-to-shell allowlist - Add mixed-case allowlist test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jan Krivanek
|
fcfa85a221 | Mock out exponential backoff unit test (#421) | ||
|
Viktor Hofer
|
a895c226b3 |
Fix agents field in plugin.json to use explicit file paths (#419)
* Fix agents field in plugin.json to use explicit file paths Claude Code's manifest validator rejects directory paths in the agents field. Replace directory references with explicit .agent.md file paths in all affected plugins (dotnet-diag, dotnet-msbuild, dotnet-template-engine). Fixes https://github.com/dotnet/skills/issues/416 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Validate that agents paths are explicit files, not directories Claude Code's manifest validator rejects directory paths in the agents field. Update PluginValidator to error when an agent path resolves to a directory, guiding authors to use explicit .agent.md file paths instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Youssef Victor
|
c4937f8f68 |
Add 'run_command_and_assert' assertion (#401)
* Add 'run_command_and_assert' assertion * Rename for clarity * Add tests * Fix race condition in command output handling * Fix async output race condition in RunCommandAndAssert WaitForExit(TimeSpan) does not guarantee that async output event handlers have completed. Add parameterless WaitForExit() call after the timed wait to ensure stdout/stderr are fully captured before checking their contents. Also handle the timeout case explicitly by killing the process. * Address PR review: extract CommandAssertionArgs, add regex matching, configurable timeout - Extract command-specific fields from Assertion into a dedicated CommandAssertionArgs record type for cleaner separation of concerns - Add ExpectedStdOutMatches/ExpectedStdErrorMatches for regex-based stdout/stderr assertions (matches type asserts) - Make command timeout configurable via assertion-level 'command_timeout' field, falling back to the scenario timeout when not specified - Show actual timeout value in timeout error messages - Update EvalSchema parsing, EvaluateCommand call sites, and all tests - Add new tests for regex matching and timeout configuration --------- Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com> |
||
|
Copilot
|
6face98ff6 |
Change agents and skills fields in plugin.json to array format for Claude Code CLI compatibility (#407)
* Initial plan * Remove agents string field from plugin.json and handle array format in parser Claude Code CLI validates `agents` as array-of-strings per its plugin manifest schema. Our plugin.json files had `"agents": "./agents/"` (a string), which caused "agents: Invalid input" validation errors on install. Remove the field from the 3 affected plugin.json files — agents are still discovered by convention from the `agents/` directory. Also make ParsePluginJson resilient to both string and array formats for forward-compatibility. Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Remove `agents` string field from plugin.json for Claude Code CLI compatibility Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Change agents field to array format in plugin.json, update validator to read array-first with string fallback Plugin.json files now declare agents as an array of file paths: "agents": ["./agents/foo.agent.md", "./agents/bar.agent.md"] ParsePluginJson reads the array first (preferred, Claude Code schema), falls back to a string path (legacy). PluginInfo gains an AgentPaths field. Validator and discovery use the array when present, else fall back to directory-based convention. Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Simplify agents to directory-path array form ["./agents/"], handle directories in array entries Per Copilot CLI spec, agents field values are "Path(s) to agent directories" — no need to list individual files. Use ["./agents/"] to mirror how skills uses "./skills/". Updated validator and discovery to handle both directory and file entries in the array. Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Delete .nuget/nuget.exe * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Convert skills field to array format in plugin.json, update validator to handle both forms Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Align AgentPaths null checks to use { Count: > 0 } pattern for consistency Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Change `agents` and `skills` fields in plugin.json to array format for Claude Code CLI compatibility Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> * Delete .nuget/nuget.exe * Consolidate SkillsPath/SkillPaths and AgentsPath/AgentPaths into single array fields Remove dual string/array fields from PluginInfo. The parser now normalizes legacy string values into single-element arrays, so all downstream code uses only SkillPaths and AgentPaths (IReadOnlyList<string>). This eliminates all array-vs-string fallback branching. Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com> Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Jan Krivanek
|
68d83b6ea8 | Add parallelism configuration options to eval.yaml (#410) | ||
|
Jan Krivanek
|
defd58af6a |
Add proper skills isolation (copying and allowlisting) (#402)
* Add proper skills isolation (copying and allowlisting) * Address PR review: harden isolation, skip symlinks, precompute allowlist - Pass null skillPath in isolated mode so the original skill tree (including siblings) is not accessible; only staged copies are allowed. - Harden CopyDirectory to skip symlinks/reparse points and verify that recursed paths stay within the source root. - Precompute additionalAllowedDirs once at session creation instead of allocating on every permission request. - Add IsolatedStagingDoesNotExposeOriginalOrSiblingSkills test that verifies both directory-level and permission-level isolation. * Fix unit test |
||
|
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> |
||
|
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> |
||
|
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) |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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 |
||
|
Korolev Dmitry
|
484d5d2c1f |
tokenization: use BPE instead of chars\4 approximation (#309)
* use BPE tokenizer * address PR comments 1 * elaborate on model * nit |
||
|
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 |
||
|
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
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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. |
||
|
Jared Parsons
|
4793aa27e9 |
Replace binlog mcp with replay to textlog (#162)
Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com> |
||
|
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> |
||
|
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 |
||
|
Jan Krivanek
|
e381a6afd2 | Fix MCP discovery (#120) | ||
|
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'. |
||
|
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> |