* 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>
* 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>
The build/check pipeline was failing repo-wide with NU1903 (warnings as
errors): Microsoft.Data.Sqlite 10.0.7 transitively pulls
SQLitePCLRaw.lib.e_sqlite3 2.1.11, whose bundled SQLite is older than
3.50.2 and carries a known high-severity vulnerability
(GHSA-2m69-gcr7-jv3q / CVE-2025-6965). A NuGet advisory update started
flagging it, breaking every build.
Pin the native lib to 3.50.3 (patched SQLite), following the existing
transitive-pin pattern in this project. The 3.50.x native package is
ABI-compatible with the 2.1.x managed provider; verified: restore is
clean (no NU1903/NU1605), build succeeds with 0 warnings, and all 586
tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* 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>
The dotnet-test plugin (largest and most active) reached ~20,397 aggregate
description characters after the find-untested-sources-polyglot skill merged
(#735), exceeding the 20,000 cap and failing the skill-validator 'check' job
on every PR that merges with main (e.g. #767, #768) as well as main itself.
This is legitimate plugin growth — a useful cross-language skill — not
metadata bloat. The cap's own comment notes it is an unvalidated local
heuristic meant to 'leave enough headroom for reasonable plugin growth', with
precedent for raising it as plugins grow (15,000 -> 20,000). Bump to 22,000
to restore ~1.6k headroom rather than degrade the routing keyword lists of
existing dotnet-test skills (owned by @dotnet/dotnet-testing) to squeeze back
under the old limit.
Tests reference SkillProfiler.MaxAggregateDescriptionLength dynamically, so no
test changes are required.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* 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>
* 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>
* Fix dotnet-msbuild Codex plugin install by externalizing mcpServers
Move the inline mcpServers configuration to a separate .mcp.json file
inside .codex-plugin/, and update .codex-plugin/plugin.json to reference
it via a relative path string. This matches the Codex plugin spec which
requires mcpServers to be a path reference to a .mcp.json file rather
than an embedded object.
The root plugin.json retains the inline mcpServers format for the
skill-validator. Both validator methods (FindPluginMcpServers and
ExternalDependencyChecker.CheckPlugin) are updated to also handle the
Codex string path format for forward compatibility.
Fixes#738
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review: validate mcpServers path and log parse errors
- Reject rooted paths and paths with '..' segments to prevent directory
traversal when resolving mcpServers string references.
- Log parse errors in ResolveMcpFile to stderr (consistent with
plugin.json parse error handling) instead of silently swallowing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR #473 moved the failure investigation prompt out of the PR comment and
only into the workflow run summary. This made it harder for users to
discover — they had to click through to the run details to find it.
Move it back to the PR comment so the copy-paste prompt is visible
inline on the PR, while keeping it in the workflow summary as well.
During multi-model skill-validator evals, judge calls intermittently
fail with:
`Failed to persist session events: Error: Failed to append to JSONL`
`file session-state\events.jsonl: Error: timeout while waiting for`
`mutex to become available`
The error wording (the word `mutex`) does not originate in our C# code
and is reported by the GitHub Copilot SDK over RPC, so the underlying
contention lives inside the SDK's session event-persistence layer.
AgentRunner already gives each session its own `configDir` and
LlmSession (judge) gives each call a unique temp root, so paths cannot
collide ACROSS sessions — this points to intra-session contention on a
single events.jsonl path.
Mitigation: serialize concurrent writes per resolved path inside our
handler with a `ConcurrentDictionary<string, SemaphoreSlim>` keyed by
the absolute path. Both `WriteFileAsync` and `AppendFileAsync` route
through a per-path lock helper. Reads remain unlocked.
Even though the conflict surfaces inside the SDK, removing one variable
(our handler issuing concurrent writes to the same file) is good defense
and makes the `ISessionFsHandler` thread-safe in its own right. The
dictionary is bounded by the number of unique files written within one
session (typically <10), so leak risk is negligible for the per-session
object lifetime.
Validation: build passes; full xUnit suite 545/545 tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* skill-validator: bump per-plugin aggregate description cap from 15K to 20K
The dotnet-test plugin (22 skills) is now at ~14K aggregate description
characters and bumping into the 15,000 cap on every meaningful description
change. This blocks legitimate plugin growth on what is, on inspection, an
informal repo guardrail rather than a documented Copilot constraint.
Origin of the 15K number
Traced via 'git log -S MaxAggregateDescriptionLength' to PR #238,
which references issue #222. The issue verbatim says:
`users may apply a limit to the total size of 'description' across`
`all skills in a plugin (...). 15K characters was mentioned, we`
`could choose smaller.`
So the value is informal — picked because it `was mentioned` and
explicitly leaving room to `choose smaller`.
Spec status
The agentskills.io specification (https://agentskills.io/specification)
documents per-skill description (1024), compatibility (500), and name
(64) limits. It does NOT define any aggregate per-plugin cap. The
`MaxDescriptionLength = 1024` constant in this same file already cites
the spec; the aggregate constant carried no citation comment.
What this PR does
- Bumps `MaxAggregateDescriptionLength` from 15,000 to 20,000. Picks a
round value that gives ~30% headroom over the largest current plugin
(dotnet-test ~14K) without making the cap meaningless.
- Adds a comment documenting the origin (informal, issue #222),
the absence of a spec source, and a TODO to validate this guardrail
against literature on skill-routing degradation and to run experiments
(skill-validator already has the infrastructure to A/B-measure
description noise vs activation accuracy).
What this PR does NOT do
- It does not remove the cap. The cost concern in #222 is real (every
description is loaded at agent startup for routing); we just don't yet
have evidence for the right ceiling.
- It does not change per-skill limits.
Validation
All 545 tests pass; the existing `CheckCommandAggregateDescriptionTests`
reference the constant symbolically and continue to pass with the new
value.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Pin Nerdbank.MessagePack to 1.1.62 and add spec URL to cap comment
- Transitively pin Nerdbank.MessagePack from 1.0.2 (vulnerable via StreamJsonRpc 2.24.84 -> GitHub.Copilot.SDK) to 1.1.62, which contains the fix for the stackalloc DateTime vulnerability (GHSA-2cwq-pwfr-wcw3). Without this pin, restore fails with NU1903 since TreatWarningsAsErrors is true.
- Add the agentskills.io specification URL to the MaxAggregateDescriptionLength comment so future readers can verify the claim that the per-plugin aggregate cap is a local repo policy rather than a spec-mandated limit (addresses PR #655 review feedback).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
/-prefix paths bypassed the flag and hit the depth check unconditionally. Now treated symmetrically with .. traversal: allowed only with the flag, otherwise produce a clear absolute (repo-rooted) path error. Also classify // as a protocol-relative URL.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add validator options
* Address PR review: validate options and add tests
- Reject --allow-repo-traversal with --plugin mode (plugins must be portable)
- Validate --max-declaration-lines and --max-agent-lines are positive integers
- Add unit tests for MaxDeclarationLines override in SkillProfiler
- Add unit tests for AllowRepoTraversal suppression and depth check preservation
- Add unit tests for MaxAgentLines override and fallback to MaxDeclarationLines
* Remove body line count checks and --max-declaration-lines/--max-agent-lines options
Line count checks are redundant with BPE token-based complexity classification,
which already provides better guidance. Keep --allow-repo-traversal only.
* Address review: remove unused CheckOptions from AgentProfiler, fix traversal test
- Remove unused CheckOptions parameter from AgentProfiler.AnalyzeAgent and
RunAgentsCheckCore since line checks were removed
- Fix AllowRepoTraversalSuppressesParentTraversalError test to use a non-deep
path (../SKILL.md) and assert no file-ref errors at all
* docs: document token count reporting in skill-validator
Add a 'Getting the token count for a skill' section to the README
explaining how to use 'check --verbose' to see BPE token counts and
complexity tiers.
Expand the Run metrics table in InvestigatingResults.md with the
inputTokens, outputTokens, cache, and judge token fields available
in results.json. Update the example analysis script to print
input/output tokens.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: address review feedback on token count documentation
- Fix tier table: replace 'Optimal sweet spot' with 'Recommended tier (sweet spot: 800–2,500)' to align with earlier statement
- Fix tokenEstimate description to accurately reflect implementation behavior (cache fallback when inputTokens is zero)
Agent-Logs-Url: https://github.com/dotnet/skills/sessions/dd5ded07-a968-4574-bfe9-1602b4434f07
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* Skill-validator: perf, cancellation, and reliability improvements
Performance:
- Cache skill-validator-dist.tar.gz in CI (evaluation.yml and skill-check.yml)
to skip .NET SDK setup, build, and tar on cache hit. Evaluate and comment-on-pr
jobs restore from cache with artifact download fallback.
- Replace SemaphoreSlim with Lock in SessionDatabase for lighter-weight
synchronous locking.
Cancellation:
- Thread CancellationToken from System.CommandLine entry point through the
entire evaluate call chain: Run -> EvaluateTarget -> EvaluateSkill/EvaluateAgent
-> ExecuteScenario/ExecuteAgentScenario -> ExecuteRun/ExecuteAgentRun, plus
EvaluateSkillNoise -> ExecuteNoiseTest.
- Add CancellationToken parameter to AgentRunner.RunAgent, Judge.JudgeRun,
PairwiseJudge.Judge, and OverfittingJudge.Analyze, forwarding to
RetryHelper and LlmSession.SendAsync respectively.
- Pass CancellationToken to all ConcurrencyLimiter.RunAsync call sites so
semaphore waits are cancellable.
- Re-throw OperationCanceledException at the top-level skill evaluation loop
so Ctrl+C / CI cancellation propagates instead of being swallowed as a
skill failure.
Reliability:
- Smart timeline truncation in Judge and PairwiseJudge: error events
(session.error, runner.error) are always preserved during truncation
regardless of position, with a summary marker showing event type breakdown.
- Add jitter (±25%) to RetryHelper exponential backoff to prevent thundering
herd on rate-limit recovery.
- Add CancellationToken parameter to ConcurrencyLimiter.RunAsync<T>.
- Add ConcurrencyLimiter tests (6 tests) and RetryHelper jitter tests (2 tests).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR review feedback
- RetryHelper: re-clamp delay after jitter so it never exceeds the
remaining budget or MaxSingleDelayMs.
- Judge/PairwiseJudge: handle edge case where error count alone exceeds
MaxTimelineEvents by trimming errors with a summary instead of
unconditionally appending all of them.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add high-variance detection and refactor FailureKind to enum
- Add Statistics.CoefficientOfVariation() for measuring score consistency
- Flag scenarios with CV > 0.5 as high-variance in ScenarioComparison
- Show console warning and markdown footnote for high-variance scenarios
- Include high-variance scenario names in Comparator verdict reason
- Refactor FailureKind from magic strings to strongly-typed enum with
JsonStringEnumMemberName attributes for backwards-compatible JSON output
- Add 6 new CV unit tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Move skill-validator docs under src/ and include in packages
Move README.md to src/ and InvestigatingResults.md + OverfittingDetection.md
to src/docs/ so they ship in the NuGet package and .tar.gz archives.
- Include docs/*.md in NuGet pack and publish output via CopyToPublishDirectory
- Add LICENSE to publish output (tar.gz only, not NuGet — uses PackageLicenseExpression)
- Replace enumerated docOnlyFiles lists with a regex pattern in CI workflows
- Deduplicate eval.yaml docs between CONTRIBUTING.md and the shipping README
- Add test fixture files walkthrough to README (moved from CONTRIBUTING.md)
- Add AGENTS.md guidance: src/ is the packaging boundary
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Update link format in failure diagnosis message
* Update AGENTS.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Add evaluation troubleshooting guide and link from PR comments
Add InvestigatingResults.md with:
- How to find and download evaluation artifacts from workflow runs
- Full results.json schema documentation
- Common failure patterns (timeouts, empty output, variance, rubric
issues, skill not activated) with diagnosis steps and fixes
- Example analysis script for AI agents to parse results
Link the doc from:
- Reporter.cs: show a link in the PR comment when any skill fails
- skill-validator README.md: Output section
- CONTRIBUTING.md: CI evaluation section
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Update InvestigatingResults.md
* Address review feedback: fix hard-coded URLs and optional plugin run docs
- Reporter.cs: Use repo-relative link instead of hard-coded GitHub URL
- evaluation.yml: Use github.sha instead of hard-coded 'main' branch
- InvestigatingResults.md: Clarify skilledPlugin run is optional
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* THROWAWAY: add trailing newline to trigger eval workflow
This change is intentionally trivial and will be reverted before merge.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR #441 review feedback (round 2)
- Reporter.cs: Remove leading / from relative link
- evaluation.yml: Use jq on results.json instead of grepping for emoji
- evaluation.yml: Use needs.gate.outputs.head_sha for revision-pinned URL
- InvestigatingResults.md: Mark pluginImprovementScore/pluginBreakdown as optional
- InvestigatingResults.md: Fix skill-not-activated symptoms to use
skillActivation fields instead of turnCount
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Use specific skillActivation field names and fix timestamp description
- Use skillActivationIsolated/skillActivationPlugin instead of generic skillActivation
- Clarify timestamp is when results were written, not run start
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix timestamp field description (was lost in prior commit)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Revert "THROWAWAY: add trailing newline to trigger eval workflow"
This reverts commit 83bd7f7601.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Show subagents invocations
* Address PR feedback: fix XML doc and sanitize agent names in markdown
- Update ExtractSubagentActivation XML doc to accurately describe that
all subagent event types are counted (not just Started/Completed)
- Add markdown table cell sanitization for agent names in FormatSubagentCell
to prevent injection via pipe chars or newlines in plugin-controlled content
- Extract SanitizeMarkdownTableCell helper for reuse
Reorganize the skill-validator project from layer-based grouping
(Commands/, Services/, Models/, Utilities/) to feature-based slices
(Check/, Evaluate/, Consolidate/, Shared/).
Key changes:
- Check/: CheckCommand, SkillProfiler, AgentProfiler, PluginValidator,
ExternalDependencyChecker, ReferenceScanner
- Evaluate/: EvaluateCommand, RejudgeCommand (now a subcommand of
evaluate), AgentRunner, Judge, PairwiseJudge, and other eval services
- Consolidate/: ConsolidateCommand
- Shared/: Models, SkillDiscovery, PluginParser (extracted from
PluginValidator), Reporter, and utilities
Split PluginValidator: parsing helpers (ParsePluginJson,
TryGetSafeSubdirectory) moved to Shared/PluginParser.cs; validation
logic stays in Check/PluginValidator.cs.
Move RawFrontmatter and RawAgentFrontmatter types from EvalSchema into
SkillDiscovery since they are only consumed by discovery.
Wire RejudgeCommand as subcommand: skill-validator evaluate rejudge.
Co-authored-by: Viktor Hofer <vihofer@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Integrate reference scanner into skill-validator CheckCommand
- Port reference scanner logic from scan.ps1 to C# ReferenceScanner service
- Add --known-domains option to CheckCommand for reference scanning
- Move known-domains.txt and allowed-external-deps.txt to eng/
- Remove standalone scan.ps1 and reference-validation.yml workflow
- Reference scanning now runs as part of skill-check.yml via skill-validator
- Add 34 unit tests for ReferenceScanner (all 463 tests pass)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR feedback: repoRoot derivation, early validation, .agents scanning, SRI policy
- Derive repoRoot from --known-domains file path instead of cwd
- Fail early with clear error when known-domains file is missing
- Add .agents/**/*.md scanning to DiscoverFiles (regression fix)
- Fix SRI policy loophole: SRI suppresses SCRIPT-NO-SRI only, domain
and HTTPS checks still apply
- Use actual known-domains file path in error messages instead of
hardcoded eng/known-domains.txt
- Update doc comments to reflect full set of scanned paths
- Add tests for .agents discovery and SRI domain enforcement
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Remove repo-specific paths from ReferenceScanner
DiscoverFiles now only scans directories it is given -- no hardcoded
knowledge of .agents, agentic-workflows, eng, or README.md. The tool
is repo-agnostic; callers control what gets scanned via --plugin dirs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Drop repoRoot from ReferenceScanner API
ScanFile/ScanFiles no longer take a repoRoot parameter. Finding paths
use the file path as provided by the caller, keeping the scanner
completely decoupled from any repo layout assumptions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Prune stale domains and add json.schemastore.org to known-domains.txt
Removed 10 entries no longer referenced by any plugin content:
agentskills.io, visualstudio.com, mcr.microsoft.com, dotnet.github.io,
github.com/dotnet/docs, github.com/dotnet/dotnet-api-docs,
github.com/dotnet/skills, github.com/github/gh-aw,
github.com/modelcontextprotocol/csharp-sdk, developers.openai.com.
Added json.schemastore.org (referenced by dotnet-template-engine).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix case-insensitive regex, update docs, add tests
- Make UrlRegex and PipeToShellRegex case-insensitive to match the
original PowerShell behavior (HTTP://, CURL, Wget all now detected)
- Update DiscoverFiles doc comment to remove stale 'individual files' claim
- Narrow CONTRIBUTING.md scope description to match actual plugin-only scanning
- Add tests for mixed/upper-case URL and pipe-to-shell detection
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Remove IPv6 loopback support, fix pipe-to-shell allowlist case sensitivity
- Remove Ipv6LoopbackRegex and IPv6 test data (not needed)
- Add case-insensitive + culture-invariant matching for pipe-to-shell allowlist
- Add mixed-case allowlist test
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix agents field in plugin.json to use explicit file paths
Claude Code's manifest validator rejects directory paths in the agents
field. Replace directory references with explicit .agent.md file paths
in all affected plugins (dotnet-diag, dotnet-msbuild, dotnet-template-engine).
Fixes https://github.com/dotnet/skills/issues/416
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Validate that agents paths are explicit files, not directories
Claude Code's manifest validator rejects directory paths in the agents
field. Update PluginValidator to error when an agent path resolves to a
directory, guiding authors to use explicit .agent.md file paths instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add 'run_command_and_assert' assertion
* Rename for clarity
* Add tests
* Fix race condition in command output handling
* Fix async output race condition in RunCommandAndAssert
WaitForExit(TimeSpan) does not guarantee that async output event
handlers have completed. Add parameterless WaitForExit() call after
the timed wait to ensure stdout/stderr are fully captured before
checking their contents. Also handle the timeout case explicitly
by killing the process.
* Address PR review: extract CommandAssertionArgs, add regex matching, configurable timeout
- Extract command-specific fields from Assertion into a dedicated
CommandAssertionArgs record type for cleaner separation of concerns
- Add ExpectedStdOutMatches/ExpectedStdErrorMatches for regex-based
stdout/stderr assertions (matches type asserts)
- Make command timeout configurable via assertion-level 'command_timeout'
field, falling back to the scenario timeout when not specified
- Show actual timeout value in timeout error messages
- Update EvalSchema parsing, EvaluateCommand call sites, and all tests
- Add new tests for regex matching and timeout configuration
---------
Co-authored-by: Jan Krivanek <jankrivanek@microsoft.com>
* Initial plan
* Remove agents string field from plugin.json and handle array format in parser
Claude Code CLI validates `agents` as array-of-strings per its plugin manifest
schema. Our plugin.json files had `"agents": "./agents/"` (a string), which
caused "agents: Invalid input" validation errors on install.
Remove the field from the 3 affected plugin.json files — agents are still
discovered by convention from the `agents/` directory. Also make ParsePluginJson
resilient to both string and array formats for forward-compatibility.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Remove `agents` string field from plugin.json for Claude Code CLI compatibility
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Change agents field to array format in plugin.json, update validator to read array-first with string fallback
Plugin.json files now declare agents as an array of file paths:
"agents": ["./agents/foo.agent.md", "./agents/bar.agent.md"]
ParsePluginJson reads the array first (preferred, Claude Code schema),
falls back to a string path (legacy). PluginInfo gains an AgentPaths
field. Validator and discovery use the array when present, else fall
back to directory-based convention.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Simplify agents to directory-path array form ["./agents/"], handle directories in array entries
Per Copilot CLI spec, agents field values are "Path(s) to agent directories"
— no need to list individual files. Use ["./agents/"] to mirror how
skills uses "./skills/". Updated validator and discovery to handle both
directory and file entries in the array.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Delete .nuget/nuget.exe
* Apply suggestions from code review
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Convert skills field to array format in plugin.json, update validator to handle both forms
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Align AgentPaths null checks to use { Count: > 0 } pattern for consistency
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Change `agents` and `skills` fields in plugin.json to array format for Claude Code CLI compatibility
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
* Delete .nuget/nuget.exe
* Consolidate SkillsPath/SkillPaths and AgentsPath/AgentPaths into single array fields
Remove dual string/array fields from PluginInfo. The parser now
normalizes legacy string values into single-element arrays, so all
downstream code uses only SkillPaths and AgentPaths (IReadOnlyList<string>).
This eliminates all array-vs-string fallback branching.
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ViktorHofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Add proper skills isolation (copying and allowlisting)
* Address PR review: harden isolation, skip symlinks, precompute allowlist
- Pass null skillPath in isolated mode so the original skill tree
(including siblings) is not accessible; only staged copies are allowed.
- Harden CopyDirectory to skip symlinks/reparse points and verify that
recursed paths stay within the source root.
- Precompute additionalAllowedDirs once at session creation instead of
allocating on every permission request.
- Add IsolatedStagingDoesNotExposeOriginalOrSiblingSkills test that
verifies both directory-level and permission-level isolation.
* Fix unit test
Amaury Levé