From b14c1dcbaa16c2a935aa194858a1c58e219574cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 31 Jul 2026 19:36:43 +0200 Subject: [PATCH 1/5] Capture the repo's hard-won skill-authoring lessons as guidance Mines ~35 merged PRs (#830-#976), issue #899 and eng/eval-quality/README.md into reusable guidance so contributors do not have to rediscover it. - Add .agents/skills/improve-skill-quality: a triage playbook for evals that regress, return no verdict, or fail to activate. Classify before rewriting - broken fixtures, underpowered trial counts, forced tools, stale spec keys and harness errors have all masqueraded as skill regressions. Two reference docs carry the content patterns and the symptom/cause/fix catalogue with citations. - Rewrite .agents/skills/create-skill-test. It documented a pre-Vally schema (scenarios:/assertions:/setup.copy_test_files) that no longer loads: all 97 eval specs use stimuli:/graders:/environment. Anyone following it authored a broken eval. Also folds in the trial floor, dormancy-guard and fixture rules. - Extend .agents/skills/create-skill with the description-as-router rules and a "write for delta over the baseline model" section. - CONTRIBUTING.md: add the quality playbook, document eval sizing, and correct the eval snippet and the stale "raise runs in dotnet-skills.experiment.yaml" advice - an experiment-level runs overwrites every eval's own value. - .gitignore: .agents/skills/ was silently swallowing the repo's own authoring skills. Replaced with .agents/skills/* plus negations for the five owned directories, so installed third-party skills stay ignored. Validated: markdownlint clean, check_eval_quality.py clean, all relative links resolve, frontmatter parses, descriptions under 1024 chars, bodies under 500 lines. Claims fact-checked against the repo by a review pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --- .agents/skills/create-skill-test/SKILL.md | 554 +++++++----------- .agents/skills/create-skill/SKILL.md | 95 ++- .agents/skills/improve-skill-quality/SKILL.md | 187 ++++++ .../references/eval-triage.md | 91 +++ .../references/writing-for-baseline-delta.md | 151 +++++ .gitignore | 11 +- AGENTS.md | 18 + CONTRIBUTING.md | 97 ++- 8 files changed, 853 insertions(+), 351 deletions(-) create mode 100644 .agents/skills/improve-skill-quality/SKILL.md create mode 100644 .agents/skills/improve-skill-quality/references/eval-triage.md create mode 100644 .agents/skills/improve-skill-quality/references/writing-for-baseline-delta.md diff --git a/.agents/skills/create-skill-test/SKILL.md b/.agents/skills/create-skill-test/SKILL.md index b64215a280..8f40bf27a7 100644 --- a/.agents/skills/create-skill-test/SKILL.md +++ b/.agents/skills/create-skill-test/SKILL.md @@ -1,414 +1,312 @@ --- name: create-skill-test -description: Scaffolds eval.yaml test files for agent skills in the dotnet/skills repository. Use when creating skill tests, writing evaluation scenarios, defining assertions and rubrics, or setting up test fixture files. Handles eval.yaml generation, fixture organization, and overfitting avoidance. Do not use for running or debugging existing tests nor for skills authoring. +description: Scaffolds eval.yaml evaluation specs for agent skills in the dotnet/skills repository. Use when creating skill tests, writing evaluation stimuli, defining graders and rubrics, sizing an eval for statistical power, or setting up test fixture files. Handles the Vally eval.yaml schema, fixture organization, and overfitting avoidance. Do not use for running or debugging existing evals (use improve-skill-quality) nor for skills authoring (use create-skill). --- # Create Skill Test -This skill helps you scaffold evaluation tests (`eval.yaml`) for agent skills, ensuring they conform to the dotnet/skills repository conventions, pass the skill-validator checks, and avoid common overfitting pitfalls. +Scaffold an evaluation spec (`eval.yaml`) for a skill or agent so it conforms to the Vally schema, +passes `skill-validator check` and `check_eval_quality.py`, is powerful enough to return a verdict, +and does not overfit to the skill's own wording. ## When to Use -- Creating a new `eval.yaml` test file for a skill -- Adding scenarios to an existing eval file -- Setting up test fixture files alongside eval definitions -- Reviewing whether rubric items and assertions risk overfitting +- Creating a new `eval.yaml` for a skill or agent +- Adding stimuli to an existing eval +- Sizing an eval so the pass gate can actually be reached +- Setting up or repairing fixture files alongside an eval +- Reviewing whether rubric items and graders risk overfitting ## When Not to Use -- Running or debugging existing tests (use the skill-validator directly) -- Modifying the skill-validator tool itself -- Creating or editing SKILL.md files (use the `create-skill` skill) +- Diagnosing a failing or regressed eval — use `improve-skill-quality` +- Modifying the skill-validator or the evaluation workflows +- Creating or editing `SKILL.md` files — use `create-skill` ## Inputs | Input | Required | Description | |-------|----------|-------------| -| Skill name | Yes | The skill being tested (must match a skill under `plugins//skills/`) | -| Plugin name | Yes | The plugin the skill belongs to (e.g., `dotnet-msbuild`) | -| Skill content | Recommended | The SKILL.md content to understand what the skill teaches | -| Scenario descriptions | Recommended | What situations the agent should be tested on | +| Skill or agent name | Yes | Must exist under `plugins//skills/` or `plugins//agents/` | +| Plugin name | Yes | e.g. `dotnet-msbuild` | +| Skill content | Yes | Read it — you cannot write non-overfitted rubric items without it | +| Failure modes to discriminate | Recommended | Each becomes one stimulus | ## Workflow -### Step 1: Locate the target and determine the test directory +### Step 1: Locate the target and the test directory -Tests live at: - -``` -# For skills: -tests///eval.yaml - -# For agents (agent. prefix convention): -tests//agent./eval.yaml +```text +tests///eval.yaml # skills +tests//agent./eval.yaml # agents (the agent. prefix disambiguates) ``` -For skills, verify the skill exists at `plugins//skills//SKILL.md`. For agents, verify the agent exists at `plugins//agents/.agent.md`. Read the target content to understand what it does -- this is critical for writing non-overfitted rubric items. - -### Step 2: Create the test directory and eval.yaml - -Create the directory and file: - -``` -# For skills: -tests/// -+-- eval.yaml - -# For agents: -tests//agent./ -+-- eval.yaml -``` +Verify the target exists at `plugins//skills//SKILL.md` or +`plugins//agents/.agent.md`, and read it. -The `agent.` prefix disambiguates agent test directories from skill test directories that might share the same name. +**Be careful with a skill that sets `disable-model-invocation: true`.** The model cannot invoke it, +so any eval graded on the skill self-activating compares two identical arms and returns judge noise. +The honest coverage for such skills is dependency-level — through the evals of the skills that load +them, and through the plugin arm. `tests/dotnet-test/filter-syntax/eval.yaml` is the one exception +here: its stimuli are ordinary user requests graded on whether the *answer* carries correct syntax +rather than on activation. Whether that produces a measurable gap is still unconfirmed, so read its +first real verdict before copying the pattern. -### Step 3: Write scenarios +### Step 2: Write the spec skeleton -Each scenario needs a `name`, `prompt`, at least one `assertion`, and a `rubric`. Use this structure: +The spec is Vally format. Every eval in this repo uses `stimuli:` and `graders:`; `scenarios:` and +`assertions:` are a pre-Vally format that no longer loads. ```yaml -scenarios: - - name: "Descriptive scenario name" - prompt: "Natural language task description as a developer would phrase it" - setup: - copy_test_files: true # OR use inline files - assertions: - - type: "output_contains" - value: "expected text" +name: +description: Evaluates the / skill +type: capability +defaults: + timeout: 5m + runs: 1 +stimuli: + - name: + prompt: + environment: + files: + - src: fixtures//Project.csproj + dest: Project.csproj + graders: + - type: output-matches + config: + pattern: (root cause|underlying issue) + - type: exit-success + - type: prompt rubric: - - "The agent correctly identified the root cause" - - "The agent suggested a concrete, actionable fix" - timeout: 120 + - ``` -#### Scenario guidelines +> **`defaults:` replaces `config:` — it does not join it.** `config` is a deprecated alias for the +> same block and vally **throws** on a spec declaring both. Most existing evals here still open with +> `config:`; when you add `runs`, merge the two into one `defaults:` block carrying `timeout` and +> `runs`. The failure is invisible otherwise: the job exits 0 with no verdicts and the PR comment +> blames "transient infrastructure". -- **Name**: Describe *what* is being tested, not *how* (e.g., "Diagnose missing package reference" not "Test binlog replay and error extraction"). -- **Prompt**: Write as a natural developer request. Never mention the skill name or instruct the agent to "use a skill." Neutral prompts prevent prompt overfitting. -- **Timeout**: Default is 120 seconds. Use 300-600 for scenarios requiring builds, benchmarks, or multi-step operations. +### Step 3: Size the eval for power before writing content -### Step 4: Configure setup +`trials = stimuli × runs`, and the pass gate is an exact one-sided sign test over the **discordant** +(non-tie) trials. -Choose one of three setup strategies: +| trials | best possible record | p | meaning | +|---:|---|---:|---| +| 1–4 | clean sweep | ≥ 0.0625 | no record can pass | +| 5 | 5W/0T/0L | 0.031 | passes only on a clean sweep | +| 8 discordant | 7W/0T/1L | 0.035 | a single loss becomes survivable | -#### Option A: Copy test files (recommended for complex fixtures) +Ties do not count — the test conditions on the **discordant** (non-tie) trials, so 4W/3T/1L over +eight trials is five discordant trials and fails. Five is an **eligibility floor**, not adequate power — one tie at five trials makes a pass +arithmetically unreachable. A run measuring a 32% tie rate certified a genuinely-helping five-trial +eval roughly one time in ten; at fifteen trials, nine times in ten. -Place fixture files alongside `eval.yaml` and enable auto-copy: +Prefer **more stimuli** over more `runs`: repeats measure the same task. Raise `runs` only when a +stimulus is genuinely expensive to add (full build/test pipelines), and write the reasoning in a +comment above `defaults:`. -```yaml -setup: - copy_test_files: true -``` +Do not set `runs` in `dotnet-skills.experiment.yaml`; experiment overrides overwrite every eval's +own value rather than defaulting it. -All files in the directory (except `eval.yaml`) are copied into the agent's working directory. +### Step 4: Write stimuli -#### Option B: Inline files (good for small, self-contained scenarios) - -```yaml -setup: - files: - - path: "MyProject/MyProject.csproj" - content: | - - - net10.0 - - - - path: "MyProject/Program.cs" - content: | - Console.WriteLine("Hello"); -``` +- **Name** describes *what* is tested, not *how*. +- **Prompt** is a natural developer request. Never mention the skill, the agent, or its vocabulary — + cued prompts inflate the overfit score and bias the baseline. +- Each stimulus should discriminate a **different** property of the skill. Five stimuli covering one + property give arithmetic, not evidence. +- Include a boundary / no-op stimulus for any skill that migrates or rewrites code, proving it + leaves already-correct input alone. -#### Option C: Reference fixture files from a subdirectory +### Step 5: Configure the environment ```yaml -setup: +environment: files: - - path: "TestProject.csproj" - source: "fixtures/scenario-a/TestProject.csproj" -``` - -Use this when multiple scenarios share a `fixtures/` directory with separate subdirectories. - -#### Setup commands (optional) - -Run shell commands before the agent starts (e.g., to build a project and generate artifacts): - -```yaml -setup: - copy_test_files: true + - src: fixtures/broken-build/App.csproj # path relative to eval.yaml + dest: App.csproj # path in the agent's working directory + - src: fixtures/broken-build # a directory + dest: . commands: - - "dotnet build -bl:build.binlog" -``` - -#### Scenario dependencies (optional) - -Some agents route to specific skills, or some skills depend on sibling agents. In the **isolated** run, only the target is loaded — so the scenario must declare its dependencies using `additional_required_skills` and/or `additional_required_agents`: - -```yaml -setup: - copy_test_files: true - additional_required_skills: - - binlog-failure-analysis # loaded in isolated run alongside the target - additional_required_agents: - - build-perf # registered in isolated run alongside the target + - dotnet build -bl || exit 0 # guard intentional failures + skills: + - binlog-failure-analysis # extra skills loaded in the isolated arm only ``` -- Names are resolved from the same plugin's `skills/` or `agents/` directory. -- These only affect the **isolated** run. The **plugin** run already loads everything; the **baseline** loads nothing. -- Different scenarios of the same target can declare different dependencies (per-scenario granularity). -- If a declared name cannot be resolved, the validator fails with an error. - -### Step 5: Write assertions - -Assertions are hard pass/fail checks. Use them for objective, binary-verifiable criteria. - -| Type | Required fields | Description | -|------|----------------|-------------| -| `output_contains` | `value` | Agent output contains text (case-insensitive) | -| `output_not_contains` | `value` | Agent output must NOT contain text | -| `output_matches` | `pattern` | Agent output matches regex | -| `output_not_matches` | `pattern` | Agent output does NOT match regex | -| `file_exists` | `path` | File matching glob exists in work dir | -| `file_not_exists` | `path` | No file matching glob exists | -| `file_contains` | `path`, `value` | File at glob path contains text | -| `file_not_contains` | `path`, `value` | File at glob path does NOT contain text | -| `exit_success` | -- | Agent produced non-empty output | - -#### Assertion guidelines - -- Prefer **broad** assertions that multiple valid approaches would satisfy. -- Avoid **narrow** assertions that gate on a specific syntax or flag the LLM already knows. -- Use `output_matches` with regex alternation for flexible matching: `"(root cause|primary error|underlying issue)"`. -- Use `file_contains` / `file_not_contains` to verify the agent modified files correctly. -- Use `output_not_contains` and `file_not_exists` to verify the agent avoided incorrect actions. - -### Step 6: Write rubric items - -Rubric items are evaluated by an LLM judge using pairwise comparison (baseline vs. skill-enhanced). Quality metrics (rubric-based at 40% weight plus overall judgment at 30%) together dominate the composite improvement score. - -#### The three rubric classifications (and how to stay in "outcome") - -The overfitting judge classifies each rubric item: +Fixture rules — each one has already cost a real result: + +- **Every referenced fixture must be tracked by git.** `.gitignore` (e.g. `coverage*.xml`) has + silently swallowed a committed fixture: the eval passed locally and failed at setup in CI. Verify + with `git ls-files`, not by looking at the working tree. +- **Every buildable fixture must build.** Judges penalize agents for "pre-existing build issues" + that the fixture author introduced. +- **Every fixture must reproduce the bug its stimulus is named for.** If it does not, the baseline + scores well and the skill has nothing to add. +- **Coverage fixtures must be internally consistent.** A Cobertura report whose declared + `line-rate`, summary totals (`lines-covered`/`lines-valid`), and `` elements disagree lets + the two arms read different truths, and the loss is the fixture's fault. Update any rubric item or + prompt that quotes a figure in the same change. +- **Do not wire duplicate fixtures** to raise `n`; rename leftovers add trials without evidence. +- A setup command that is *expected* to fail while still producing its artifact must be guarded + (`|| exit 0`), or vally drops the trial. +- A cleanup command that strips sources must skip directories containing `SKILL.md` — the staged + skill lives there, and deleting it aborts only the skilled arm. + +### Step 6: Write graders + +Graders are hard pass/fail checks evaluated on every arm. + +| Type | Required config | Purpose | +|------|-----------------|---------| +| `output-matches` / `output-not-matches` | `pattern` | Regex over agent output | +| `output-contains` / `output-not-contains` | `substring` | Literal text in output | +| `file-exists` / `file-not-exists` | `path` | Glob against the work directory | +| `file-contains` / `file-not-contains` | `path`, `value` | Content of a produced file | +| `run-command` | `command` (plus optional `expected_exit_code`, `timeout`, `stdout_matches`) | Verify produced code actually builds/runs | +| `exit-success` | — | Agent produced non-empty output | +| `prompt` | — | Runs the LLM judge against the `rubric` | + +Rules: + +- A grader whose `config` is absent or missing its required key parses fine and **enforces nothing**. + The usual cause is an indentation slip during an edit; `check_eval_quality.py` blocks it. +- Prefer broad patterns that several valid approaches satisfy: + `(root cause|primary error|underlying issue)`. +- **If the skill mandates an output shape, assert on it.** A skill required to emit a decisive + `Recommendation:` line can silently stop doing so while the eval still passes. +- Use `file-not-contains` / `file-not-exists` to prove the agent avoided an incorrect action. + +### Step 7: Write rubric items + +Rubric items are judged pairwise (baseline vs. skilled). The overfitting judge classifies each item: | Classification | Description | Goal | |---------------|-------------|------| -| **outcome** | Tests whether the agent reached a correct result. Describes WHAT, not HOW. | Target this | -| **technique** | Tests whether the agent used a skill-specific procedure. | Minimize | -| **vocabulary** | Tests whether the agent used specific terminology from the skill. | Avoid | - -#### Rubric writing rules +| **outcome** | Whether the agent reached a correct result — WHAT, not HOW | Target this | +| **technique** | Whether the agent used a skill-specific procedure | Minimize | +| **vocabulary** | Whether the agent used the skill's terminology | Avoid | -1. **Test outcomes, not methods.** Write "Identified the root cause of the build failure" -- not "Replayed the binlog using `dotnet build /flp`." -2. **Allow alternative approaches.** If multiple valid solutions exist, the rubric item should accept any of them. -3. **Never reference the skill by name** or use phrasing copied directly from the SKILL.md. -4. **Don't test pre-existing LLM knowledge.** If the LLM already knows something (common APIs, standard syntax, basic escaping), testing for it adds no signal. -5. **Test findings, not diagnostic steps.** Write "Correctly determined that the root cause is a missing PackageReference" -- not "Used `dotnet restore` to check package resolution." -6. **Each item should be independently evaluable.** Avoid compound items that test multiple things. +1. Test outcomes, not methods: "Identified the root cause of the build failure", not "Replayed the + binlog using `dotnet build /flp`". +2. Accept any valid approach. +3. Never reference the skill by name, and never reuse `SKILL.md` phrasing. +4. Never reward using the skill — that measures activation, not user value. +5. Do not test knowledge the model already has; it adds no delta. +6. Keep each item independently evaluable. +7. Do not reward raw volume (test count, report length); judges will compare it when both arms act. -#### Examples +**Good:** -**Well-designed (outcome-focused):** ```yaml rubric: - - "Correctly identified the missing NuGet package as the root cause of the build failure" - - "Recognized that downstream project failures were cascading from the root cause, not independent errors" - - "Suggested a concrete fix that would resolve the root cause" + - Correctly identified the missing NuGet package as the root cause of the build failure + - Recognized that downstream failures cascaded from that root cause + - Suggested a concrete fix that resolves it ``` -**Overfitted (vocabulary/technique):** +**Overfitted:** + ```yaml rubric: - - "Replayed the binary log using 'dotnet build /flp:v=diag'" # technique: gates on specific command - - "Measured cold, warm, and no-op build scenarios" # vocabulary: uses skill's labels - - "Used the --clreventlevel flag with dotnet trace collect" # vocabulary: gates on specific flag + - Replayed the binary log using 'dotnet build /flp:v=diag' # technique + - Measured cold, warm, and no-op build scenarios # vocabulary + - Used the template-comparison skill # rewards activation ``` -### Step 7: Add optional constraints +### Step 8: Add constraints sparingly ```yaml -expect_tools: ["bash"] # Agent must use these tools -reject_tools: ["create_file"] # Agent must NOT use these tools -max_turns: 10 # Maximum agent iterations -max_tokens: 5000 # Maximum token budget +constraints: + expect_tools: [bash] + reject_tools: [edit, create] + reject_skills: [some-skill] ``` -Use constraints sparingly -- only when the scenario specifically requires or forbids certain agent behaviors. - -### Step 8: Add non-activation scenarios with `expect_activation: false` - -Many skills have clear boundaries -- situations where the skill should recognize it does not apply and decline gracefully. Test these boundaries using `expect_activation: false`. - -#### How `expect_activation: false` works - -When a scenario has `expect_activation: false`: - -1. **All three runs still execute** (baseline, skilled-isolated, skilled-plugin) and assertions are evaluated on each. The flag does not change which runs are performed. -2. **Activation verdict is inverted** -- if the skill is not activated for this prompt, the evaluator reports it as `[Info] not activated (expected)` instead of treating it as a failure. -3. **The scenario is excluded from the noise test** -- the multi-skill activation test only runs positive (`expect_activation: true`) scenarios. +- `expect_tools: [bash]` on an **advisory** question forces a restore or build and converts an + answer into a timeout with no quality benefit. Only require tools when the task genuinely needs + them. +- `reject_tools` is the right way to keep a read-only stimulus read-only. -#### When to use non-activation scenarios +### Step 9: Add dormancy guards -Add `expect_activation: false` scenarios when the skill has explicit "When Not to Use" boundaries. Common patterns: - -| Pattern | Example | -|---------|---------| -| **Wrong input format** | Skill handles Android tombstones; scenario provides an iOS crash log | -| **Out-of-scope request** | Skill collects dumps; scenario asks to *analyze* a dump | -| **Incompatible project type** | Skill converts PackageReference to CPM; scenario has packages.config | -| **Wrong framework version** | Skill migrates .NET 8 to 9; scenario provides a .NET 8 app and asks for .NET 10 migration | -| **Prerequisite not met** | Skill requires a specific file format that isn't present | - -#### Example: Wrong input format - -```yaml -- name: "Reject iOS crash log as wrong format" - prompt: "I have a crash log file at crashlog_ios.txt from a crashed app. Please symbolicate the .NET runtime frames." - expect_activation: false - setup: - copy_test_files: true - assertions: - - type: "output_matches" - pattern: "(iOS|Apple|not.*(Android|tombstone)|wrong.*(format|type))" - rubric: - - "Recognized that this is an iOS crash log, not an Android tombstone" - - "Did NOT attempt to apply the Android tombstone symbolication workflow" - - "Explained that iOS crash logs require a different symbolication process" -``` - -#### Example: Out-of-scope request +A dormancy guard proves the skill stays dormant on an off-target request that superficially matches +it. Add one per real "when not to use" boundary: wrong input format, out-of-scope request, +incompatible project type, wrong framework version, prerequisite absent. ```yaml -- name: "Decline dump analysis request" - prompt: | - I already have a .dmp crash dump file from my .NET app. Can you help - me analyze it to find the root cause of the crash? - expect_activation: false - assertions: - - type: "output_matches" - pattern: "(out of scope|not cover|does not|cannot|only.*collect)" - rubric: - - "Clearly states that dump analysis is out of scope for this skill" - - "Does not attempt to open or analyze the dump file" - - "Does not install analysis tools like dotnet-dump analyze, lldb, or windbg" - timeout: 30 -``` - -#### Example: Incompatible project type - -```yaml -- name: "Decline CPM conversion for packages.config project" - prompt: "Convert my simple-packages-config/LegacyApp project to Central Package Management." - expect_activation: false - setup: - copy_test_files: true - assertions: - - type: "output_contains" - value: "packages.config" - - type: "file_not_exists" - path: "simple-packages-config/Directory.Packages.props" - rubric: - - "Detected the project uses packages.config instead of PackageReference format" - - "Informed the user that CPM requires PackageReference and cannot be applied to packages.config projects" - - "Suggested migrating from packages.config to PackageReference first" - - "Did not attempt to create Directory.Packages.props or modify any project files" + - name: Decline dump analysis request + prompt: | + I already have a .dmp crash dump from my .NET app. Can you help me + analyze it to find the root cause of the crash? + expect_activation: false + graders: + - type: output-matches + config: + pattern: (out of scope|not cover|does not|cannot|only.*collect) + - type: prompt + rubric: + - Stated that dump analysis is out of scope + - Did not open or analyze the dump file + - Did not install analysis tools such as dotnet-dump analyze, lldb, or windbg + - Suggested the correct alternative ``` -#### Rubric guidelines for non-activation scenarios +> **Never combine `expect_activation: false` with `constraints.reject_skills`.** That forces the +> skilled arm to run skill-free, making it identical to the baseline; the score is then pure judge +> noise. Across four evals the same guard scored −0.4, +0.4, +0.4 and 0, and twice cost a skill its +> pass. `expect_activation: false` **alone** is the repo convention. -Non-activation rubric items typically verify three things: +Guard rubrics verify three things: **recognition** (why it does not apply), **restraint** (no +workflow, no file changes, no installs), **redirection** (the correct next step). -1. **Recognition** -- The agent identified *why* the skill doesn't apply. -2. **Restraint** -- The agent did NOT attempt the skill's workflow (no file modifications, no tool installs). -3. **Redirection** -- The agent suggested the correct alternative approach or next step. - -### Step 9: Validate the eval.yaml - -Run the static validator: +### Step 10: Validate ```bash dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- check --plugin ./plugins/ +python eng/eval-quality/check_eval_quality.py +./eng/run-skill-evals.sh ``` -Then run evaluation (at least 3 runs for reliable results): - -```bash -# For skills: -dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- evaluate \ - --runs 3 \ - --tests-dir tests/ \ - plugins//skills/ - -# For agents: -dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- evaluate \ - --runs 3 \ - --tests-dir tests/ \ - plugins//agents/.agent.md -``` - -## eval.yaml Template - -```yaml -scenarios: - - name: "" - prompt: "" - setup: - copy_test_files: true - assertions: - - type: "output_contains" - value: "" - - type: "exit_success" - rubric: - - "" - - "" - - "" - timeout: 120 +`check_eval_quality.py` blocks ten structural defect classes that each already cost a real result: +missing or untracked fixtures, self-contradicting coverage fixtures, empty grader configs, dormancy +guards with `reject_skills`, sub-floor trial counts, duplicate YAML keys, and `config:`/`defaults:` +collisions. Do not add a new eval to `eng/eval-quality/underpowered-allowlist.txt` — the gate rejects +allowlist entries that are new relative to the base branch. - - name: "" - prompt: "" - expect_activation: false - setup: - copy_test_files: true - assertions: - - type: "output_matches" - pattern: "" - - type: "file_not_exists" - path: "" - rubric: - - "" - - "" - - "" - timeout: 120 -``` +For the official run, submit a PR review containing `/evaluate` so it binds to the reviewed commit. ## Validation Checklist -After creating a test, verify: - -- [ ] Test directory matches `tests///` for skills or `tests//agent./` for agents -- [ ] Target exists at `plugins//skills//SKILL.md` (skill) or `plugins//agents/.agent.md` (agent) -- [ ] Every scenario has `name`, `prompt`, at least one assertion, and rubric items -- [ ] Prompts are written as natural developer requests (no skill/agent name references) -- [ ] Assertions are broad enough that multiple valid approaches pass -- [ ] Rubric items test outcomes, not specific techniques or vocabulary -- [ ] Fixture files are present when `copy_test_files: true` is used -- [ ] `source` paths in setup files point to existing fixture files -- [ ] `additional_required_skills`/`additional_required_agents` names exist in the same plugin -- [ ] Timeouts are reasonable for the scenario complexity -- [ ] Non-activation scenarios use `expect_activation: false` and verify recognition, restraint, and redirection -- [ ] `dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- check` passes +- [ ] Directory is `tests///` or `tests//agent./` +- [ ] Spec uses `stimuli:` / `graders:`, and exactly one of `defaults:` or `config:` +- [ ] `stimuli × runs` clears 5 with room for the expected tie rate +- [ ] Each stimulus discriminates a different property +- [ ] Prompts never name the skill, the agent, or its vocabulary +- [ ] Every referenced fixture exists, is tracked by `git ls-files`, and builds +- [ ] Every fixture reproduces the failure its stimulus is named for +- [ ] Every grader has its required `config` key +- [ ] Any output shape the skill mandates has a grader +- [ ] Rubric items are outcome-shaped and never reward using the skill +- [ ] Dormancy guards use `expect_activation: false` alone +- [ ] `skill-validator check` and `check_eval_quality.py` pass ## Common Pitfalls | Pitfall | Solution | |---------|----------| -| Prompt mentions the skill by name | Rewrite as a natural developer request describing the problem | -| Prompt mentions the agent by name | Same as above — agent name in prompts biases the baseline | -| Rubric tests a specific diagnostic command | Rewrite to test the finding or outcome that command produces | -| Assertion gates on syntax the LLM already knows | Use a broader pattern or test the result instead | -| All rubric items test the same aspect | Diversify: test identification, fix quality, and error avoidance | -| Missing fixture files for `copy_test_files` | Add the required project/source files alongside eval.yaml | -| Timeout too short for builds | Use 300-600s for scenarios that compile or run benchmarks | -| Single scenario covers the entire skill | Break into focused scenarios testing different aspects | -| Compound rubric items testing multiple things | Split into separate, independently-evaluable items | -| No non-activation scenarios for skill with clear boundaries | Add `expect_activation: false` scenarios for each "When Not to Use" case | -| Agent test missing `additional_required_skills` | If the agent routes to specific skills, declare them so the isolated run loads them | +| Writing `scenarios:` / `assertions:` | That format no longer loads; use `stimuli:` / `graders:` | +| Adding `defaults: runs:` beside an existing `config:` | Merge into one `defaults:` block | +| Landing an eval at exactly 5 trials | A single tie makes a pass unreachable; size for the tie rate | +| Raising `runs` instead of adding stimuli | Repeats measure one task and add no cross-task evidence | +| Prompt mentions the skill or agent by name | Rewrite as a natural developer request | +| Rubric rewards using the skill | Assert activation with tooling; rubric measures outcomes | +| Fixture present but ignored by git | Verify with `git ls-files`; CI setup will fail otherwise | +| Fixture that does not build or does not reproduce the bug | Fix the fixture before blaming the skill | +| Dormancy guard with `reject_skills` | Use `expect_activation: false` alone | +| `expect_tools: [bash]` on an advisory question | Drop it; it causes timeouts, not quality | +| Timeout too short for code generation | Use ~360s; empty output fails every grader | +| Duplicate YAML key left behind by an edit | It overwrites the next stimulus field by field — delete the stray block | +| Direct activation-graded eval for a `disable-model-invocation: true` skill | Cover it through a consumer skill, or grade the answer content as `filter-syntax` does | +| Agent eval missing `environment.skills` | Declare the skills the agent routes to, or the isolated arm cannot use them | diff --git a/.agents/skills/create-skill/SKILL.md b/.agents/skills/create-skill/SKILL.md index d47e509222..fae94e3eb0 100644 --- a/.agents/skills/create-skill/SKILL.md +++ b/.agents/skills/create-skill/SKILL.md @@ -1,6 +1,6 @@ --- name: create-skill -description: Scaffolds new agent skills for the dotnet/skills repository. Use when creating a new skill, generating SKILL.md files, or setting up skill directory structures. Handles frontmatter generation, section templates, and validation guidance. +description: Scaffolds new agent skills for the dotnet/skills repository. Use when creating a new skill, generating SKILL.md files, writing a skill description that the runtime will actually route to, or setting up skill directory structures. Handles frontmatter generation, section templates, and validation guidance. Do not use for fixing a skill that already fails its evaluation (use improve-skill-quality) or for writing eval.yaml (use create-skill-test). --- # Create Skill @@ -17,6 +17,8 @@ This skill helps you scaffold new agent skills that conform to the Agent Skills ## When Not to Use - Modifying existing skills (edit directly instead) +- Diagnosing or fixing a skill that fails its evaluation (use `improve-skill-quality`) +- Writing the skill's `eval.yaml` (use `create-skill-test`) - Creating custom agents (use the agents/ directory pattern) ## Inputs @@ -38,25 +40,66 @@ Ensure the name: - Does not contain consecutive hyphens - Is between 1-64 characters -### Step 2: Create the skill directory +### Step 2: Write the description — it is the router -``` -skills// -└── SKILL.md -``` - -### Step 3: Generate SKILL.md with frontmatter - -Create the file with required YAML frontmatter: +The `description` is the **only** text the runtime sees when deciding whether to load the skill. +A perfect body behind a weak description never runs. ```yaml --- name: -description: +description: . USE FOR: . DO NOT USE FOR: . --- ``` -### Step 4: Add body content sections +- Lead with an action verb and use the user's own words: symptoms, error codes (`CS1501`, + `MSTEST0014`), artifact names (`.testsettings`, `binlog`), and requests phrased as a developer + would type them. +- Partition against sibling skills on the **real discriminator**, not the topic. "Does the + abstraction already exist?" separates two skills; "testing" does not. Add the matching exclusion + to **both** siblings. +- Claim the ambiguous words that would otherwise route to a sibling. If prompts say "review my + tests" and a sibling owns "review", say so explicitly. +- Check every `DO NOT USE FOR` clause against the scenarios the skill exists to serve — an + exclusion like "already on v3" can lock out the post-upgrade fixes that are the skill's purpose. +- Budget: 1,024 characters per description, and the whole plugin's rendered skill menu is also + budgeted. A helper skill users should never invoke directly can set + `disable-model-invocation: true` to free menu space while staying invocable by name. + +### Step 3: Write for delta over the baseline model + +Every skill is scored head-to-head against the same model with **no skill loaded**. Content the +model already produces unaided is worth zero; content that makes it slower or more hedged is worth +less than zero. See +[improve-skill-quality/references/writing-for-baseline-delta.md](../improve-skill-quality/references/writing-for-baseline-delta.md) +for the full evidence. + +| Do | Instead of | +|----|------------| +| Encode the decision the model would otherwise get wrong | Restating API signatures it already reproduces | +| "When A, do B, never C, verify D" tables | Lists of plausible alternatives | +| A concrete output contract (exact command, verdict line, findings table) | "Consider…", "you may want to…" | +| Scale output structure to input size | A 12-section dashboard for an 8-test suite | +| Stop-conditions that prevent over-applying | Acting before measuring, rewriting working code | +| Instructing the agent to discover repo paths | Marking discoverable paths as required inputs | +| Reporting restore/build/test failures truthfully | Claiming success after a failed command | +| Verifying load-bearing API claims by compiling or probing | Trusting a source read | +| Gating rare or expensive paths behind `references/` | One large SKILL.md carrying every path | + +Do not over-correct: a skilled answer shorter and less actionable than the baseline's still loses. + +### Step 4: Create the skill directory + +``` +skills// +└── SKILL.md +``` + +### Step 5: Generate SKILL.md with frontmatter + +Create the file with the frontmatter drafted in Step 2. + +### Step 6: Add body content sections Include these recommended sections: @@ -68,7 +111,7 @@ Include these recommended sections: 6. **Validation**: How to confirm the skill worked correctly 7. **Common Pitfalls**: Known traps and how to avoid them -### Step 5: Add optional directories (if needed) +### Step 7: Add optional directories (if needed) ``` skills// @@ -78,7 +121,7 @@ skills// └── assets/ # Templates, images, data files ``` -### Step 6: Update CODEOWNERS +### Step 8: Update CODEOWNERS Add entries in `.github/CODEOWNERS` for the new skill and its test directory: @@ -89,12 +132,19 @@ Add entries in `.github/CODEOWNERS` for the new skill and its test directory: Match the owner pattern used by sibling skills in the same plugin. -### Step 7: Validate the skill +### Step 9: Validate the skill - Confirm frontmatter fields are valid - Ensure SKILL.md is under 500 lines - Check that file references use relative paths - Verify instructions are actionable and specific +- Run `dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- check --plugin ./plugins/` + +### Step 10: Add the eval + +A skill without an `eval.yaml` has no evidence that it improves on the baseline. Use +`create-skill-test` to add one in the same pull request, and size it for statistical power — an eval +below five trials can never return a passing verdict. ## SKILL.md Template @@ -161,6 +211,10 @@ After creating a skill, verify: - [ ] Validation section exists with observable success criteria - [ ] No secrets, tokens, or internal URLs included - [ ] `.github/CODEOWNERS` has entries for the new skill and its test directory +- [ ] The description names concrete triggers and excludes the nearest sibling skills +- [ ] Every section changes a decision the unskilled model would otherwise get wrong +- [ ] The skill states when **not** to act, and what a truthful failure report looks like +- [ ] An `eval.yaml` exists and clears the trial floor ## Common Pitfalls @@ -173,9 +227,16 @@ After creating a skill, verify: | SKILL.md too long | Move detailed content to `references/` files | | Hardcoded environment assumptions | Document requirements in `compatibility` field | | Missing CODEOWNERS entry | Add entries for both `/plugins//skills//` and `/tests///` matching sibling skills' owner pattern | +| Skill restates what the model already knows | Cut it; a skill is scored as a delta over the unskilled model | +| Discoverable paths listed as required inputs | Tell the agent to discover them, or it will stop and ask the user | +| Description partitioned by topic against a sibling | Partition on the real discriminator and exclude on both sides | +| Exclusion clause blocks the skill's own use cases | Re-read every "do not use for" clause against real workflow phases | +| Skill added without an eval | Add `eval.yaml` in the same PR; unevaluated skills carry no evidence | ## References - [Agent Skills Specification](https://agentskills.io/specification) -- [Repository README](../../README.md) -- [Contributing Guidelines](../../CONTRIBUTING.md) +- [Repository README](../../../README.md) +- [Contributing Guidelines](../../../CONTRIBUTING.md) +- [create-skill-test](../create-skill-test/SKILL.md) — authoring the skill's `eval.yaml` +- [improve-skill-quality](../improve-skill-quality/SKILL.md) — fixing a skill that loses to its baseline diff --git a/.agents/skills/improve-skill-quality/SKILL.md b/.agents/skills/improve-skill-quality/SKILL.md new file mode 100644 index 0000000000..f61fd268b2 --- /dev/null +++ b/.agents/skills/improve-skill-quality/SKILL.md @@ -0,0 +1,187 @@ +--- +name: improve-skill-quality +description: Diagnoses and fixes skills in the dotnet/skills repository that lose to their own baseline, fail to activate, time out, or return "no credible improvement". Use when an evaluation verdict is a regression or underpowered, when a skill regressed after a change, when /evaluate reports no results, or when deciding whether a weak skill should be strengthened or retired. Do not use for scaffolding a brand-new skill (use create-skill) or a brand-new eval (use create-skill-test). +--- + +# Improve Skill Quality + +Turn a failing or unconvincing evaluation into a targeted fix. The single most common mistake +in this repo is rewriting skill prose in response to a verdict whose real cause was the eval, +the fixtures, or the harness. Classify first, then fix. + +## When to Use + +- An evaluation verdict is a regression, underpowered, or "no credible improvement". +- A skill wins in the isolated arm but not in the plugin arm, or is reported "not activated". +- `/evaluate` reports "Evaluation ran but produced no results". +- A skill scores well but costs too much (tokens, turns, wall time, plugin menu budget). +- Deciding whether to strengthen or retire a persistently weak skill. + +## When Not to Use + +- Creating a new skill from scratch — use `create-skill`. +- Creating a new `eval.yaml` from scratch — use `create-skill-test`. +- Changing the harness itself (`eng/skill-validator`, `eng/vally-adapter`, `evaluation*.yml`). + +## Inputs + +| Input | Required | Description | +|-------|----------|-------------| +| Verdict evidence | Yes | The `/evaluate` PR comment, or `results.json` from the run artifacts | +| Losing trial transcripts | Yes for content fixes | Baseline vs. skilled output plus the judge's stated reason | +| W/T/L record and trial count | Yes | Distinguishes a real regression from an underpowered eval | +| Activation status per arm | Yes | Isolated and plugin activation are different failures | + +## Workflow + +### Step 1: Get the evidence before forming a hypothesis + +Read [InvestigatingResults.md](../../../eng/vally-adapter/InvestigatingResults.md) for how to +download artifacts and read `results.json`. Extract, per failing stimulus: + +- win / tie / loss record and total trials (`trials = stimuli × runs`) +- activation status in the **isolated** and **plugin** arms, separately +- the judge's verbatim reason on each losing trial +- whether any trial errored, timed out, or produced empty output + +Do not proceed until you can quote a losing trial. "Every change is driven by the judge evidence +from the losing trials, not by style preference" is the standard this repo holds itself to. + +### Step 2: Classify the failure + +Work down this table and stop at the first row that matches. Rows are ordered by how often the +symptom has been misdiagnosed as a skill-content problem. + +| Symptom | Real cause class | Go to | +|---------|------------------|-------| +| No `results.json`, "produced no results", or the spec never loaded | Harness / spec-load | Step 3 | +| Trials errored, timed out, or returned empty output | Reliability | Step 3 | +| A fixture does not build, is untracked by git, or contradicts itself | Fixture | Step 4 | +| Positive record (e.g. 16W/8T/1L) but the verdict is still not a pass | Statistical power | Step 5 | +| Skilled arm equals baseline arm by construction | Eval design | Step 6 | +| Activated and lost on quality, judge names a concrete defect | Skill content | Step 7 | +| Activated in isolation, not in plugin | Activation / routing | Step 8 | +| Not activated in either arm | Frontmatter description | Step 8 | +| Wins but costs far more than baseline | Scope and cost | Step 7 | + +### Step 3: Rule out harness and reliability causes + +See [references/eval-triage.md](references/eval-triage.md) for the full catalogue. The recurring ones: + +- A spec declaring both `config:` and `defaults:` is rejected by vally, the job still exits 0, and + the PR comment blames "transient infrastructure". Merge them into one `defaults:` block. +- An errored trial is not automatically a fixture problem — judge-side auth and `session.idle` + failures look identical from the verdict and need harness fixes, not SDK pins. +- `expect_tools: [bash]` on an advisory question forces a restore or build and turns an answer into + a timeout with no quality gain. +- Genuine code-generation stimuli need roughly 360s; a timeout yields empty output, which fails + every grader and hides the real quality signal. + +### Step 4: Verify the fixtures before touching the skill + +Run `python eng/eval-quality/check_eval_quality.py` — it blocks ten defect classes that each already +cost a real result here. Then confirm by hand: + +- every buildable fixture actually builds, and every fixture actually reproduces the bug its + stimulus is named for; +- every referenced fixture is in the git index (`git ls-files`), not merely on disk — `.gitignore` + has silently swallowed committed coverage fixtures; +- coverage fixtures are self-consistent: declared `line-rate`, summary totals, and the `` + elements must all report the same number, or the two arms legitimately read different truths. + +### Step 5: Check whether the eval could ever have passed + +The gate is an exact one-sided sign test over **discordant** (non-tie) trials. + +- Below 5 trials no record can pass, however good the skill. +- At 5–7 trials only a clean sweep passes; one tie makes a pass arithmetically unreachable. +- Tolerating a single loss needs 8 discordant trials. + +So a positive record with a failing verdict is a power problem, not a content problem. Fix it by +adding **discriminating stimuli** (cross-task evidence) rather than raising `runs` (repetition +only) — except where each stimulus drives an expensive pipeline. Record the reasoning in a comment +above `defaults:`, as `tests/dotnet-test/grade-tests/eval.yaml` does. + +### Step 6: Check whether the two arms differ at all + +An eval that compares the skill against itself measures judge noise: + +- A dormancy guard (`expect_activation: false`) must **not** also set `constraints.reject_skills`. + That makes the skilled arm skill-free, i.e. identical to baseline. Across four evals the same + guard scored −0.4, +0.4, +0.4 and 0, twice costing a skill its pass. +- A skill with `disable-model-invocation: true` cannot self-activate, so an eval graded on + activation compares two identical arms. Cover it through a consumer skill, or grade the answer + content instead (`tests/dotnet-test/filter-syntax/eval.yaml` is the one such eval here, and its + first real verdict is still outstanding). +- A grader whose `config` is missing its required key enforces nothing, so the stimulus has one + fewer assertion than it appears to. + +### Step 7: Fix skill content against the losing trial + +Only now change the skill. Apply the patterns in +[references/writing-for-baseline-delta.md](references/writing-for-baseline-delta.md); the ones that +most often flip a loss: + +- Replace reference prose the model already knows with decisions it would otherwise get wrong. +- Add stop-conditions so a strong skill does not over-apply — but do not over-correct into + answering more narrowly than the baseline did. +- Scale output structure to input size; a dashboard for an 8-test suite loses to a direct answer. +- Require truthful validation reporting; claiming "Build succeeded" after a failed restore is an + automatic loss. +- Verify load-bearing API claims by compiling or probing, not by reading source. +- For cost regressions, gate rare or expensive paths behind `references/` reads and size any + orchestration to the user's scope. + +### Step 8: Fix activation + +Activation failures are frontmatter and routing failures, not body failures. See +[references/eval-triage.md](references/eval-triage.md). Summary: + +| Failure | Fix | +|---------|-----| +| Not activated in any arm | Put the user's own words in `description`: symptoms, error codes, artifact names, quoted requests | +| A sibling skill wins the prompt | Claim the exact ambiguous words in `description`, and add matching exclusions on **both** siblings | +| Model answers with no skill at all | Raise the stakes in the description, de-crowd the plugin menu, verify with the plugin arm | +| Boundary excludes real scenarios | Re-read every "do not use for" clause against every eval prompt and real workflow phase | +| Description at the 1,024-char ceiling | Cut restated body content, not trigger phrases; check the plugin menu budget too | + +### Step 9: Re-validate + +```bash +dotnet run --project eng/skill-validator/src/SkillValidator.csproj -- check --plugin ./plugins/ +python eng/eval-quality/check_eval_quality.py +./eng/run-skill-evals.sh +``` + +Then request the official run by submitting a PR review containing `/evaluate` (Files changed → +Review changes), which binds the run to the reviewed commit. Before declaring a regression on the +result, confirm the skill payload actually changed — reruns on byte-identical content have shifted +7W/2T/2L to 4W/5T/2L. + +## Validation + +- [ ] A losing trial and the judge's stated reason are quoted in the PR description. +- [ ] The failure was classified before any content was edited. +- [ ] `check_eval_quality.py` and `skill-validator check` both pass. +- [ ] Trial count clears the power bar for the observed tie rate, not just the floor of 5. +- [ ] Isolated **and** plugin activation are both reported. +- [ ] The PR body records root cause, fix, and validation so the lesson is reusable. + +## Common Pitfalls + +| Pitfall | Solution | +|---------|----------| +| Rewriting skill prose in response to an underpowered verdict | Underpowered means too few discordant trials; add discriminating stimuli instead | +| Adding `defaults: runs:` to a spec that already has `config:` | Merge into a single `defaults:` block; vally rejects specs with both | +| Padding `runs` to clear the trial floor | Five repeats of one stimulus measure one task; add stimuli | +| Treating an errored trial as fixture nondeterminism | Read the stderr first; judge-side auth failures need harness fixes | +| Fixing a "wrong" answer that the fixture actually made wrong | Check fixture self-consistency before blaming the response | +| Strengthening a skill nobody uses and nothing passes | Weak eval signal plus thin telemetry is a valid retirement case | +| Landing a fix without re-running | Verify the invoked payload contains the fix; judge noise is real | + +## References + +- [references/writing-for-baseline-delta.md](references/writing-for-baseline-delta.md) — content patterns that beat the unskilled model +- [references/eval-triage.md](references/eval-triage.md) — symptom, cause and fix catalogue with PR citations +- [eng/eval-quality/README.md](../../../eng/eval-quality/README.md) — the ten structural gate checks and why each exists +- [eng/vally-adapter/InvestigatingResults.md](../../../eng/vally-adapter/InvestigatingResults.md) — downloading artifacts and reading `results.json` diff --git a/.agents/skills/improve-skill-quality/references/eval-triage.md b/.agents/skills/improve-skill-quality/references/eval-triage.md new file mode 100644 index 0000000000..86db3263f5 --- /dev/null +++ b/.agents/skills/improve-skill-quality/references/eval-triage.md @@ -0,0 +1,91 @@ +# Evaluation triage catalogue + +Symptom → cause → fix, with the PR where each was diagnosed. Use with +`SKILL.md` Step 2; this file is the detail behind each cause class. + +## Harness and reliability + +| Symptom | Cause | Fix | Evidence | +|---------|-------|-----|----------| +| "Evaluation ran but produced no results", advice says transient infrastructure | Spec declares both `config:` and `defaults:`; vally throws, the job still exits 0 | Merge into one `defaults:` block carrying `timeout` and `runs` | PR #971 | +| Same message, nothing in `plugins/` changed | Genuine LLM-session auth failure | Re-post `/evaluate`; inspect job logs before touching content | PR #932 | +| Trial errored, avgN unusually low | Judge-side CAPI / `session.idle` timeout, not fixture nondeterminism | Read the trial stderr first; fix the harness, do not pin an SDK | PR #907 | +| Timeout on an advisory question | `expect_tools: [bash]` forced a restore or build | Drop the tool requirement; the answer was always textual | PR #861 | +| Every grader fails and output is empty | Code-generation stimulus timed out | Raise to ~360s | PR #862, PR #863 | +| Only the skilled arm aborts with "contains no SKILL.md" | A setup cleanup command deleted the staged skill directory | Skip directories carrying `SKILL.md` when stripping sources | PR #878 | +| Trials silently dropped | Setup command exited non-zero although its artifact was produced | Guard intentional failures, e.g. `dotnet build -bl \|\| exit 0` | PR #878 | +| One arm has unmatched trajectories | Comparison judge error or arm timeout biases the remaining trials | Report inconclusive, do not read it as a regression | PR #887 | +| Fixture resolves locally, SDK-not-found in CI | Fixture pin without roll-forward | `global.json` with `rollForward: latestMajor` | PR #907 | + +## Fixtures + +| Symptom | Cause | Fix | Evidence | +|---------|-------|-----|----------| +| Judge penalizes the agent for "pre-existing build issues" | The fixture does not compile | Build every buildable fixture before shipping the eval | PR #949 | +| Scenario fails at setup in CI but passes locally | Fixture is on disk but not in the git index (`.gitignore` swallowed it) | Verify with `git ls-files`; the gate now blocks this | PR #945, PR #953 | +| Judge says the response "made a critical error" about a number | Cobertura fixture is split-brain: declared `line-rate` disagrees with its `` payload or summary totals | Make declared rate, summary totals and payload agree, then re-derive any rubric quoting a figure | PR #964, PR #945 | +| Baseline scores suspiciously well | The fixture never reproduces the bug the stimulus is named for | Rebuild the fixture until it produces the real error | PR #974 | +| `n` rose but power did not | Duplicate or rename-leftover fixtures wired in as new stimuli | Delete byte-equivalent leftovers; only wire fixtures exercising new behavior | PR #971, PR #945 | + +## Statistical power + +The pass gate is an exact one-sided sign test over discordant trials, and `trials = stimuli × runs`. + +| trials | best possible record | p | verdict | +|---:|---|---:|---| +| 1–4 | clean sweep | ≥ 0.0625 | can never pass | +| 5 | 5W/0T/0L | 0.031 | passes only on a sweep | +| 8 discordant | 7W/0T/1L | 0.035 | a single loss becomes survivable | + +Ties do not count — the test conditions on the discordant (non-tie) trials, so 4W/3T/1L over eight +trials is five discordant trials and fails. + +Consequences seen in real runs: + +- Five `dotnet-test` evals raised to exactly 5 trials returned 16W/8T/1L overall — every skill + winning, none regressing — and **all five failed**, four because ties made a pass unreachable + before the run started. (PR #971, `eng/eval-quality/README.md`) +- At the 32% tie rate measured there, a genuinely-helping skill parked at 5 trials is certified + about one run in ten; at 15 trials, about nine in ten. +- Adding stimuli is strictly better than raising `runs`: repeats measure one task. Use `runs` only + where a stimulus is genuinely expensive — `code-testing-agent` uses `defaults.runs: 2` because + each stimulus drives a full npm/pytest/dotnet pipeline inside a 60-minute budget. (PR #974) +- The verdict reads each trial's **winner**, never its magnitude: weighting a confidence interval by + "slightly better" vs "much better" made a stronger win look like variance and reversed verdicts on + identical records. (PR #965, PR #952) + +## Eval design + +| Symptom | Cause | Fix | Evidence | +|---------|-------|-----|----------| +| A dormancy guard scores randomly across runs | `expect_activation: false` combined with `constraints.reject_skills`, making the skilled arm skill-free and identical to baseline | Use `expect_activation: false` alone | PR #945, PR #953 | +| A reference skill shows no improvement | `disable-model-invocation: true` means the model cannot self-activate it, so an activation-graded eval compares identical arms | Cover it through a consumer skill, or grade answer content as `filter-syntax` does | PR #971, PR #976, issue #899 | +| An eval "passes" while the skill stopped emitting its signature output | No grader asserts the mandated shape | Add a grader for the exact contract (e.g. the `Recommendation:` line) | PR #904 | +| Overfit score high, user value unclear | Rubric items reward using the skill, or prompts echo skill vocabulary | Assert activation with `expect_tools`; keep rubric items outcome-shaped | PR #904 | +| Both arms write tests and the judge compares volume | Non-activation rubric rewards raw output | Add anti-hijack criteria: do not invoke the skill, do not reward test count | PR #945 | +| A grader appears to enforce something but does not | `config:` is missing its required key after an indentation slip | `check_eval_quality.py` blocks it; verify the key is present | `eng/eval-quality/README.md` | +| Two stimuli behave identically | Duplicate YAML key — a leftover `prompt:`/`graders:` block overwrites the following stimulus field by field | Delete the stray block after confirming it is not a distinct stimulus that lost its `- name:` | PR #971 | +| Eval measures path recall | The skill is a map to reference files | Do not create the eval; test the consumer's outcome instead | PR #974 | + +## Activation + +| Symptom | Cause | Fix | Evidence | +|---------|-------|-----|----------| +| Not activated in any arm | Triggering words absent from `description` | Add symptoms, error codes, artifact names in user language | PR #974 | +| Not activated for the scenarios the skill exists for | An over-broad exclusion clause | Re-read every "do not use for" clause against every eval prompt | PR #974 | +| Wrong sibling wins the prompt | Descriptions partitioned by topic instead of by discriminator | Partition on the real question, and add handoff exclusions on both sides | PR #864 | +| Sibling wins on one ambiguous word | The target skill never claims that word | Claim it explicitly — "review" had to be claimed by `writing-mstest-tests` | PR #863 | +| Isolated activation perfect, plugin arm fails | The model self-serves: reads the file and answers with no skill at all | Raise stakes in the description, de-crowd the menu, verify in the plugin arm | PR #850 | +| Menu pressure across a plugin | Helper/reference skills consuming budget | `disable-model-invocation: true` keeps them invocable by name only | PR #850 | + +## Process + +| Rule | Evidence | +|------|----------| +| Trigger `/evaluate` by submitting a PR review (Files changed → Review changes) so the run binds to the reviewed commit | PR #956, PR #949 | +| Before declaring a regression, confirm the invoked payload changed — reruns on byte-identical content moved 7W/2T/2L to 4W/5T/2L | PR #974 | +| Use cross-family evaluation for broad rewrites; single-family passes hide model-specific regressions | issue #899, PR #947 | +| Workflow changes cannot be validated by the PR's own evaluation (GitHub runs workflow definitions from `main`) — use manual dispatches | PR #872 | +| Prefer deterministic scripts over agentic workflows for deterministic policy | PR #928 | +| Agent `tools:` allowlists are host-specific and case-sensitive; an allowlist can grant zero tools | PR #856, PR #847 | +| Keep `InvestigatingResults.md` in sync whenever verdict fields, scoring, or PR-comment wording change | PR #965, PR #932 | diff --git a/.agents/skills/improve-skill-quality/references/writing-for-baseline-delta.md b/.agents/skills/improve-skill-quality/references/writing-for-baseline-delta.md new file mode 100644 index 0000000000..b722dce349 --- /dev/null +++ b/.agents/skills/improve-skill-quality/references/writing-for-baseline-delta.md @@ -0,0 +1,151 @@ +# Writing skill content that beats the baseline + +Every skill in this repo is scored head-to-head against the *same model with no skill loaded*. +The score is a **delta**, so content the model already produces unaided is worth zero — and +content that makes the model slower, longer, or more hedged is worth less than zero. + +Each rule below is traced to the PR where it was learned. + +## 1. Encode decisions, not knowledge + +**Rule:** Write what the model should *do* when it sees a symptom, not what an API *is*. + +`system-text-json-net11` scored 0% improvement because it "read as reference prose — API +signatures the model already reproduces". The rewrite into imperative decision guidance is what +moved it. (PR #926) + +Practical test: delete any sentence the unskilled model would have written anyway. If most of the +skill disappears, it is a reference doc, not a skill. + +## 2. Use "when A, do B, never C, verify D" tables + +**Rule:** Route with decision tables so the model picks one answer instead of listing plausible +alternatives. + +PR #926 added a table mapping PascalCase / typed-metadata / probing requests to exact APIs and +their anti-patterns. PR #947 added "Rules That Change the Answer" tables across the MAUI skills. +Tables also give graders something concrete to assert on. + +## 3. Demand a concrete final artifact + +**Rule:** Specify the exact shape of the answer — the command, the verdict line, the findings +table, the recommendation. + +Generic advice did not move the template-engine skills; output contracts did: "exact single +`dotnet new` command", "one-line verdict header", "single findings table", decisive +`Recommendation:` lines. (PR #904) + +If the skill mandates an output shape, the eval must assert on that shape, or the skill can +silently stop emitting it. + +## 4. Scale structure to input size + +**Rule:** Preserve the full report template for large inputs; answer directly for small ones. + +`assertion-quality` regressed on an 8-test fixture because the skill forced a "Summary Dashboard + +12 categories" onto a trivial input and lost to a concise baseline. The fix instructed the skill to +scale report depth to suite size and complexity. (PR #865) + +## 5. Add stop-conditions — then check you have not over-corrected + +**Rule:** Say when *not* to act; then verify the skilled answer is still at least as complete as +the baseline's. + +Strong skills regress by doing too much: rewriting working code, answering beyond the question, +prescribing a remedy before measuring. PR #910 added stop-conditions to `eval-performance` so it +would not act on compile-time slowness or unmeasured builds; PR #947 added scope control to +`maui-collectionview`. + +The opposite failure is just as real: in PR #947 the scope-control wording "over-corrected the +other skills into answering too narrowly", and failing scenarios had skilled answers *shorter* than +baseline. Length is not the goal, but omitting the implementation detail the baseline supplied is a +loss. + +## 6. Do not make discoverable inputs "required" + +**Rule:** Only mark an input required when a human genuinely must supply it. Otherwise instruct the +agent to discover it. + +A `Project or solution path | Yes` row in an Inputs table made the agent answer "I need to see your +project file" while `TestProject.csproj` sat in the working directory. (PR #974) + +## 7. Verify load-bearing claims empirically + +**Rule:** Compile or probe anything the skill asserts about API surface or runtime behavior. + +The most damaging skill defects found in review were factual: MAUI docs taught `ItemSizingStrategy` +on `LinearItemsLayout`, which does not compile (MAUIX2002), and a theming "fix" was reverted after a +runtime probe disproved the source-reading that motivated it. (PR #947) + +## 8. Preserve semantics in migration mappings + +**Rule:** Migration tables need semantic guardrails, not just API substitutions. + +A one-cell mapping steered frontier models into a behavior change: `TimeProvider` guidance using +`.DateTime` silently set `DateTimeKind.Unspecified`; the correct mapping is `.UtcDateTime` / +`.LocalDateTime`. (PR #906) + +## 9. Require truthful validation reporting + +**Rule:** Tell the agent to distinguish restore, build and test failures, and to cite a clean run +before claiming success. + +`migrate-static-to-wrapper` lost trials for claiming "Build succeeded" after a restore failure; +`code-testing-agent` had to be told to cite a clean run. (PR #945) + +## 10. Prove already-correct inputs are left alone + +**Rule:** Any skill that migrates or rewrites code must state the no-op condition, and the eval must +test it. + +PR #929 added an "already on v3" boundary fixture verifying no changes were made; the reviewer +called it "the single best addition" in the PR. + +## 11. Keep the common path in `SKILL.md`, gate the rest + +**Rule:** Rare, expensive or platform-specific paths belong behind a `references/` read. + +`coverage-analysis` went from 30 KB to 15 KB by moving PowerShell and report-generation paths into +references read only when needed — cost down, contract unchanged. (PR #971) + +## 12. Size orchestration to the request + +**Rule:** Do not run a full research → plan → implement pipeline for one function. + +`code-testing-agent` was split into focused and broad paths so a single-function request skips +`.testagent/` artifacts and extra passes. (PR #971) + +## 13. Structure beats verbosity + +**Rule:** When a scenario ties despite a longer skilled answer, the missing differentiator is a +recommended shape, not more words. + +Both arms hardcoded colors in the MAUI theming scenario. The winning change was the rule "define +the palette once — don't scatter literals", not a longer explanation. (PR #947) + +## 14. Retirement is a legitimate outcome + +**Rule:** A skill that is weak across model families, thinly used, and costing menu budget should be +cut, not polished indefinitely. + +`mcp-csharp-debug` was cut after strengthening 0 of 5 families, with owner confirmation; the change +removed the skill, its eval, its CODEOWNERS entry and cross-skill references. (PR #938) +`dotnet-test-frameworks` was removed as a duplicate subset that "nothing actually loaded". (PR #851) + +## Frontmatter: the description is the router + +The `description` is the only text the runtime sees when deciding whether to load the skill. (PR #974) + +| Rule | Evidence | +|------|----------| +| Include symptoms, error codes and artifact names in the user's words (`CS1501`, `.testsettings`, `MSTEST0014`) | PR #974 | +| Lead with an action verb and quote natural requests ("what's wrong with my build file?") | PR #910 | +| Partition siblings by the real discriminator ("abstraction already exists" vs "create wrapper first"), with exclusions on both sides | PR #864 | +| Claim the ambiguous words that route to the wrong sibling — `writing-mstest-tests` had to claim "review" | PR #863 | +| Never exclude a phase the skill exists to serve; "already on MSTest v3+" blocked its own post-bump fixtures | PR #974 | +| Watch both limits: 1,024 characters per description **and** the plugin menu budget | PR #974, PR #910 | +| A helper skill that users should not invoke can set `disable-model-invocation: true` to free menu budget | PR #850 | + +Menu pressure is measurable: disabling model invocation for one helper skill dropped the +`dotnet-test` menu from 14,981 to 14,261 characters (PR #850), and PR #910 tracked the msbuild menu +budget explicitly as part of a description rewrite. diff --git a/.gitignore b/.gitignore index 9481074e8e..ca1b89e782 100644 --- a/.gitignore +++ b/.gitignore @@ -3,8 +3,15 @@ ## ## Get latest from https://github.com/github/gitignore/blob/main/VisualStudio.gitignore -# Installed skills (other authors' content) -.agents/skills/ +# Installed skills (other authors' content). The repo's own authoring skills are +# re-included below; a directory pattern would make that impossible, so this +# ignores the *entries* under .agents/skills/ rather than the directory itself. +.agents/skills/* +!.agents/skills/authoring-github-workflows/ +!.agents/skills/create-custom-agent/ +!.agents/skills/create-skill/ +!.agents/skills/create-skill-test/ +!.agents/skills/improve-skill-quality/ # User-specific files *.rsuser diff --git a/AGENTS.md b/AGENTS.md index d472039e21..7bc1805649 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,6 +2,24 @@ This repository contains skill plugins under `plugins/`. Each subdirectory in `plugins/` is an independent plugin (e.g., `plugins/dotnet-msbuild`, `plugins/dotnet`). +## Working on skills and evals + +Use the repository's own authoring skills under `.agents/skills/` instead of improvising: + +- `create-skill` — scaffolding a new skill, and writing a `description` the runtime will route to. +- `create-skill-test` — writing or resizing an `eval.yaml`. Evals use the Vally schema + (`stimuli:` / `graders:` / `defaults:`); `scenarios:` / `assertions:` no longer load. +- `improve-skill-quality` — an eval regressed, produced no verdict, or the skill did not activate. + Classify the failure before editing skill content; broken fixtures, underpowered trial counts and + harness errors routinely masquerade as skill regressions. + +Before pushing eval changes, run `python eng/eval-quality/check_eval_quality.py`. It blocks ten +structural defect classes documented in `eng/eval-quality/README.md`, each of which has already cost +a real evaluation result here. + +The distilled quality rules — what makes a skill beat its own baseline — live in the "Quality bar" +section of `CONTRIBUTING.md`. + ## Skill-Validator The skill-validator is a shipping tool — its NuGet package and `.tar.gz` archives are built from `eng/skill-validator/src/`. Content referenced at runtime or bundled with the tool (docs, README, etc.) must live under `src/` so it is included in the published output. Do not add references from `src/` to files outside of it, except for explicitly linked packaging assets (such as the repo-root `LICENSE` file) referenced by the project file. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ae6608b8c5..aca4c4b73b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -250,8 +250,9 @@ A minimal eval file: name: my-skill description: Evaluates the / skill type: capability -config: +defaults: timeout: 3m + runs: 1 stimuli: - name: "Describe what the agent should do" prompt: | @@ -269,8 +270,39 @@ stimuli: - The agent suggested a concrete fix ``` +> [!IMPORTANT] +> `defaults:` and `config:` are the same block — `config` is a deprecated alias — and vally +> **rejects** a spec declaring both. Many existing evals still open with `config:`; when you add +> `runs`, merge the two into a single `defaults:` block. The failure is silent: the job exits 0 with +> no verdicts and the PR comment blames "transient infrastructure". + Each skill is evaluated in up to three variants — **baseline** (no skills), **skilled** (only the skill under test), and **plugin** (the whole plugin loaded) — and a skill "passes" only when the skilled run is a *credible* improvement over baseline. To assert that a skill should stay dormant for an out-of-scope task, add `expect_activation: false` to that stimulus. See any existing `tests/*/*/eval.yaml` for a fuller example of the grader and stimulus format. +#### Size the eval so it can return a verdict + +The pass gate is an exact one-sided sign test over the **discordant** (non-tie) trials, and +`trials = stimuli × runs`. + +| trials | best possible record | meaning | +| ---: | --- | --- | +| 1–4 | a clean sweep | no record can pass | +| 5 | 5W/0T/0L | passes only on a clean sweep — one tie makes a pass unreachable | +| 8 discordant | 7W/0T/1L | the point at which a single loss becomes survivable | + +Ties do not count: the test conditions on the **discordant** (non-tie) trials, so 4W/3T/1L over +eight trials is five discordant trials and fails. Five is an *eligibility floor*, not adequate +power. A run that measured a 32% tie rate certified a +genuinely-helping five-trial eval about one time in ten; at fifteen trials, about nine times in ten. +Prefer adding **discriminating stimuli** over raising `runs` — repeats measure the same task. See +[`eng/eval-quality/README.md`](eng/eval-quality/README.md) for the full derivation and for the ten +structural defects the CI quality gate blocks. + +Run the gate locally before pushing: + +```bash +python eng/eval-quality/check_eval_quality.py +``` + @@ -290,10 +322,13 @@ Prerequisites: Node.js 20+ and the [GitHub CLI](https://cli.github.com) signed i ./eng/run-skill-evals.sh ``` -Per-skill verdicts are written to `./eval-results///results.json`, and the raw experiment output goes to `./eval-results/_experiment/`. Model, judge model, and runs-per-stimulus come from the `overrides:` block in `dotnet-skills.experiment.yaml`. +Per-skill verdicts are written to `./eval-results///results.json`, and the raw experiment output goes to `./eval-results/_experiment/`. Model and judge model come from the `overrides:` block in `dotnet-skills.experiment.yaml`. -> [!WARNING] -> LLM evaluations are noisy. For results you intend to share in a Pull Request, raise `runs` in `dotnet-skills.experiment.yaml` to at least 3 (5 is better) for reliable signal. +> [!WARNING] +> LLM evaluations are noisy. Runs-per-stimulus is deliberately **not** set in +> `dotnet-skills.experiment.yaml`: an experiment-level `runs` overwrites every eval's own value +> instead of defaulting it, making per-eval trial counts impossible to express. Raise the eval's own +> `defaults.runs` instead — or, better, add discriminating stimuli. ### CI evaluation @@ -374,6 +409,60 @@ Skills and agents in this repo should be: - **Verifiable**: always include a way to validate success. - **Tool-conscious**: don't assume capabilities that might not exist in every runtime. +### What consistently separates a passing skill from a failing one + +Every skill is scored head-to-head against the *same model with no skill loaded*, so the score is a +**delta**. The rules below are the ones this repo has learned the hard way, each from a merged fix. + +**Content** + +- Encode the decisions the model gets wrong; delete anything it already produces unaided. A skill + that reads as reference prose ties its own baseline. +- Prefer "when A, do B, never C, verify D" tables over lists of plausible alternatives, and end with + a concrete output contract (the exact command, the verdict line, the findings table). +- Scale output structure to input size. A twelve-section report for an eight-test suite loses to a + concise direct answer. +- Add stop-conditions so a strong skill doesn't over-apply — then check you haven't over-corrected + into answering more narrowly than the baseline did. +- Tell the agent to discover repo paths rather than listing them as required inputs; a "required" + project path makes the agent ask the user for a file that is already in the working directory. +- Require truthful validation reporting. Claiming "Build succeeded" after a failed restore is an + automatic loss. +- Verify load-bearing API claims by compiling or probing, not by reading source. +- Keep the common path in `SKILL.md` and gate rare or expensive paths behind `references/` reads. + +**Activation** + +- The `description` is the only text the runtime sees when choosing a skill. Put the user's own + words in it: symptoms, error codes, artifact names, quoted requests. +- Partition against sibling skills on the real discriminator, not the shared topic, and add the + matching exclusion to both siblings. +- Re-read every "do not use for" clause against the scenarios the skill exists to serve — an + exclusion can lock out the skill's own purpose. +- Watch both budgets: 1,024 characters per description, and the plugin's rendered skill menu. + +**When something fails** + +- Classify before you rewrite. Broken fixtures, underpowered trial counts, forced tools, stale spec + keys and harness errors have all masqueraded as skill regressions. +- Read the losing trial and the judge's stated reason, and drive the fix from that evidence rather + than from style preference. +- A positive win/tie/loss record with a failing verdict is a power problem, not a content problem. +- A skill that is weak across model families, thinly used, and costing menu budget is a candidate + for retirement, not indefinite polishing. + +### Authoring skills for this repository + +The repository ships agent skills for working on itself, under `.agents/skills/`: + +| Skill | Use it when | +|-------|-------------| +| `create-skill` | Scaffolding a new skill and writing a description the runtime will route to | +| `create-skill-test` | Writing or resizing an `eval.yaml` | +| `improve-skill-quality` | An evaluation regressed, returned no verdict, or the skill didn't activate | +| `create-custom-agent` | Adding an agent definition | +| `authoring-github-workflows` | Editing anything under `.github/workflows/` | + ## Skill-Validator & Evaluation workflow Changes to `eng/skill-validator` or the `.github/workflows/evaluation*.yml` workflows must be made from a branch in the `dotnet/skills` repository (i.e., not from a fork). This is a security measure. From cdba3aadb78a0d3da39ade38ae1a6c8369b30e1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 3 Aug 2026 00:40:14 +0200 Subject: [PATCH 2/5] Address multi-model review: correct the sign-test arithmetic and eval exceptions Three review rounds (Copilot, Claude Opus 4.8, GPT-5.6 Sol/Terra) against the implementation in eng/vally-adapter/adapt.mjs. Statistics. The docs conflated the gate's two independent bars. adapt.mjs gates on (1) counted trials >= 5, else `underpowered`, and (2) p <= 0.05 on a sign test over the *discordant* (non-tie) trials. So "at 5-7 trials one tie makes a pass unreachable" was wrong: at 6 trials 5W/1T/0L is 5 discordant and passes at p = 0.031, and at 7 trials two ties are survivable. Every trial table is now keyed on discordant trials. eng/eval-quality/README.md carried the same over-generalisation and is corrected too, since these docs restate it. Eval coverage. "Every skill needs an eval" and "no direct eval for a disable-model-invocation skill" contradicted each other. Reference skills are now explicitly exempt and covered through their consumers, and filter-syntax is no longer described as the sole answer-graded exception - platform-detection uses the same pattern. Fixtures. "Every buildable fixture must build" forbade the intentionally broken fixtures the same document demonstrates. Now: healthy fixtures build, deliberately broken ones fail only for the reason their stimulus is about. Triage routing. The first-match classifier sent fixture failures to the harness row before the fixture row could match, and treated any positive-but-failing record as a power problem even though adapt.mjs requires `conclusive` first. Added an inconclusive row, moved the fixture row up, and scoped "quote a losing trial" to content fixes so it no longer blocks the no-results path. Also: expect_tools does not assert activation (the harness reports it separately); CONTRIBUTING claimed evaluations run automatically on plugin changes when a maintainer must trigger /evaluate against a bound commit; the minimal eval example is labelled as a sub-floor skeleton; create-skill scaffolds into plugins//skills/ rather than a non-existent top-level skills/. Validated: markdownlint clean, check_eval_quality.py clean, adapt.test.mjs 21 pass, links and anchors resolve. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --- .agents/skills/create-skill-test/SKILL.md | 47 ++++++++++-------- .agents/skills/create-skill/SKILL.md | 12 +++-- .agents/skills/improve-skill-quality/SKILL.md | 49 +++++++++++++------ .../references/eval-triage.md | 22 +++++---- CONTRIBUTING.md | 32 +++++++----- eng/eval-quality/README.md | 16 +++--- 6 files changed, 112 insertions(+), 66 deletions(-) diff --git a/.agents/skills/create-skill-test/SKILL.md b/.agents/skills/create-skill-test/SKILL.md index 8f40bf27a7..bfbc4c248d 100644 --- a/.agents/skills/create-skill-test/SKILL.md +++ b/.agents/skills/create-skill-test/SKILL.md @@ -47,10 +47,10 @@ Verify the target exists at `plugins//skills//SKILL.md` or **Be careful with a skill that sets `disable-model-invocation: true`.** The model cannot invoke it, so any eval graded on the skill self-activating compares two identical arms and returns judge noise. The honest coverage for such skills is dependency-level — through the evals of the skills that load -them, and through the plugin arm. `tests/dotnet-test/filter-syntax/eval.yaml` is the one exception -here: its stimuli are ordinary user requests graded on whether the *answer* carries correct syntax -rather than on activation. Whether that produces a measurable gap is still unconfirmed, so read its -first real verdict before copying the pattern. +them, and through the plugin arm. Two here take the other route and grade the *answer* rather than +activation: `tests/dotnet-test/filter-syntax/eval.yaml` and +`tests/dotnet-test/platform-detection/eval.yaml`. Whether that produces a measurable gap for a skill +the model cannot invoke is still unconfirmed, so read a real verdict before copying the pattern. ### Step 2: Write the spec skeleton @@ -89,17 +89,21 @@ stimuli: ### Step 3: Size the eval for power before writing content -`trials = stimuli × runs`, and the pass gate is an exact one-sided sign test over the **discordant** -(non-tie) trials. +`trials = stimuli × runs`, and the gate has two independent bars: -| trials | best possible record | p | meaning | -|---:|---|---:|---| -| 1–4 | clean sweep | ≥ 0.0625 | no record can pass | -| 5 | 5W/0T/0L | 0.031 | passes only on a clean sweep | -| 8 discordant | 7W/0T/1L | 0.035 | a single loss becomes survivable | +1. **Counted trials ≥ 5**, else the verdict is `underpowered` — never a pass, never a regression. +2. **p ≤ 0.05 on an exact one-sided sign test over the *discordant* (non-tie) trials.** Ties are not + discarded; they hold the discordant count down. -Ties do not count — the test conditions on the **discordant** (non-tie) trials, so 4W/3T/1L over -eight trials is five discordant trials and fails. Five is an **eligibility floor**, not adequate power — one tie at five trials makes a pass +| discordant trials | records that pass | p | +|---:|---|---:| +| ≤ 4 | none | ≥ 0.0625 | +| 5–7 | zero losses only (5W/0L) | 0.031 | +| 8 | one loss survivable (7W/1L) | 0.035 | + +At exactly 5 counted trials a single tie is fatal — it leaves 4 discordant. At 6 counted trials one +tie is survivable (5W/1T/0L); at 7, up to two are (5W/2T/0L). A loss is not. Five is an +**eligibility floor**, not adequate power — one tie at five trials makes a pass arithmetically unreachable. A run measuring a 32% tie rate certified a genuinely-helping five-trial eval roughly one time in ten; at fifteen trials, nine times in ten. @@ -140,8 +144,10 @@ Fixture rules — each one has already cost a real result: - **Every referenced fixture must be tracked by git.** `.gitignore` (e.g. `coverage*.xml`) has silently swallowed a committed fixture: the eval passed locally and failed at setup in CI. Verify with `git ls-files`, not by looking at the working tree. -- **Every buildable fixture must build.** Judges penalize agents for "pre-existing build issues" - that the fixture author introduced. +- **Every fixture must behave as its stimulus assumes.** A fixture meant to be healthy must build; a + fixture meant to be broken must fail for the exact reason the stimulus is about, and no other. + Judges penalize agents for unrelated "pre-existing build issues" that the fixture author + introduced. - **Every fixture must reproduce the bug its stimulus is named for.** If it does not, the baseline scores well and the skill has nothing to add. - **Coverage fixtures must be internally consistent.** A Cobertura report whose declared @@ -192,7 +198,8 @@ Rubric items are judged pairwise (baseline vs. skilled). The overfitting judge c binlog using `dotnet build /flp`". 2. Accept any valid approach. 3. Never reference the skill by name, and never reuse `SKILL.md` phrasing. -4. Never reward using the skill — that measures activation, not user value. +4. Never reward using the skill — the harness reports activation separately, so a rubric item that + does this measures nothing and inflates the overfit score. 5. Do not test knowledge the model already has; it adds no delta. 6. Keep each item independently evaluable. 7. Do not reward raw volume (test count, report length); judges will compare it when both arms act. @@ -284,8 +291,8 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t - [ ] `stimuli × runs` clears 5 with room for the expected tie rate - [ ] Each stimulus discriminates a different property - [ ] Prompts never name the skill, the agent, or its vocabulary -- [ ] Every referenced fixture exists, is tracked by `git ls-files`, and builds -- [ ] Every fixture reproduces the failure its stimulus is named for +- [ ] Every referenced fixture exists and is tracked by `git ls-files` +- [ ] Every fixture behaves as its stimulus assumes — healthy ones build, deliberately broken ones fail only for the stated reason - [ ] Every grader has its required `config` key - [ ] Any output shape the skill mandates has a grader - [ ] Rubric items are outcome-shaped and never reward using the skill @@ -301,9 +308,9 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t | Landing an eval at exactly 5 trials | A single tie makes a pass unreachable; size for the tie rate | | Raising `runs` instead of adding stimuli | Repeats measure one task and add no cross-task evidence | | Prompt mentions the skill or agent by name | Rewrite as a natural developer request | -| Rubric rewards using the skill | Assert activation with tooling; rubric measures outcomes | +| Rubric rewards using the skill | Drop the item — the harness reports activation separately; rubrics measure outcomes | | Fixture present but ignored by git | Verify with `git ls-files`; CI setup will fail otherwise | -| Fixture that does not build or does not reproduce the bug | Fix the fixture before blaming the skill | +| Fixture that does not build, or breaks for the wrong reason | Fix the fixture before blaming the skill | | Dormancy guard with `reject_skills` | Use `expect_activation: false` alone | | `expect_tools: [bash]` on an advisory question | Drop it; it causes timeouts, not quality | | Timeout too short for code generation | Use ~360s; empty output fails every grader | diff --git a/.agents/skills/create-skill/SKILL.md b/.agents/skills/create-skill/SKILL.md index fae94e3eb0..093c99b1ac 100644 --- a/.agents/skills/create-skill/SKILL.md +++ b/.agents/skills/create-skill/SKILL.md @@ -91,7 +91,7 @@ Do not over-correct: a skilled answer shorter and less actionable than the basel ### Step 4: Create the skill directory ``` -skills// +plugins//skills// └── SKILL.md ``` @@ -114,7 +114,7 @@ Include these recommended sections: ### Step 7: Add optional directories (if needed) ``` -skills// +plugins//skills// ├── SKILL.md ├── scripts/ # Executable code agents can run ├── references/ # Additional documentation loaded on demand @@ -144,7 +144,11 @@ Match the owner pattern used by sibling skills in the same plugin. A skill without an `eval.yaml` has no evidence that it improves on the baseline. Use `create-skill-test` to add one in the same pull request, and size it for statistical power — an eval -below five trials can never return a passing verdict. +below five counted trials can never return a passing verdict. + +The exception is a helper skill with `disable-model-invocation: true`: the model cannot +self-activate it, so an activation-graded eval compares two identical arms. Cover it through the +evals of the skills that load it instead. ## SKILL.md Template @@ -214,7 +218,7 @@ After creating a skill, verify: - [ ] The description names concrete triggers and excludes the nearest sibling skills - [ ] Every section changes a decision the unskilled model would otherwise get wrong - [ ] The skill states when **not** to act, and what a truthful failure report looks like -- [ ] An `eval.yaml` exists and clears the trial floor +- [ ] An `eval.yaml` exists and clears the trial floor (or the skill is `disable-model-invocation: true` and covered through its consumers) ## Common Pitfalls diff --git a/.agents/skills/improve-skill-quality/SKILL.md b/.agents/skills/improve-skill-quality/SKILL.md index f61fd268b2..ae574de44f 100644 --- a/.agents/skills/improve-skill-quality/SKILL.md +++ b/.agents/skills/improve-skill-quality/SKILL.md @@ -44,26 +44,34 @@ download artifacts and read `results.json`. Extract, per failing stimulus: - the judge's verbatim reason on each losing trial - whether any trial errored, timed out, or produced empty output -Do not proceed until you can quote a losing trial. "Every change is driven by the judge evidence -from the losing trials, not by style preference" is the standard this repo holds itself to. +Do not change skill content until you can quote a losing trial and the judge's reason for it. For the +other cause classes the evidence is different: harness failures are diagnosed from the job log and +the spec, and power problems from the trial record — neither has a losing trial to quote, and +demanding one is what sends people rewriting prose instead. ### Step 2: Classify the failure Work down this table and stop at the first row that matches. Rows are ordered by how often the -symptom has been misdiagnosed as a skill-content problem. +symptom has been misdiagnosed as a skill-content problem. A setup or trial failure that traces to a +fixture belongs in the Fixture row even though it also matches the two rows above it. | Symptom | Real cause class | Go to | |---------|------------------|-------| +| A fixture does not build, is untracked by git, breaks for the wrong reason, or contradicts itself | Fixture | Step 4 | | No `results.json`, "produced no results", or the spec never loaded | Harness / spec-load | Step 3 | | Trials errored, timed out, or returned empty output | Reliability | Step 3 | -| A fixture does not build, is untracked by git, or contradicts itself | Fixture | Step 4 | -| Positive record (e.g. 16W/8T/1L) but the verdict is still not a pass | Statistical power | Step 5 | +| Trajectories unmatched, a trial errored, or the summary disagrees — verdict reported inconclusive | Reliability (not power) | Step 3 | +| Positive record (e.g. 16W/8T/1L), comparison conclusive, verdict still not a pass | Statistical power | Step 5 | | Skilled arm equals baseline arm by construction | Eval design | Step 6 | | Activated and lost on quality, judge names a concrete defect | Skill content | Step 7 | | Activated in isolation, not in plugin | Activation / routing | Step 8 | | Not activated in either arm | Frontmatter description | Step 8 | | Wins but costs far more than baseline | Scope and cost | Step 7 | +A verdict is only a *measured* result when the comparison was conclusive: `adapt.mjs` requires zero +errored trials, zero unmatched trajectories, and an agreeing summary before it will report a pass or +a regression. Confirm that before reading a record as a power problem. + ### Step 3: Rule out harness and reliability causes See [references/eval-triage.md](references/eval-triage.md) for the full catalogue. The recurring ones: @@ -76,14 +84,17 @@ See [references/eval-triage.md](references/eval-triage.md) for the full catalogu a timeout with no quality gain. - Genuine code-generation stimuli need roughly 360s; a timeout yields empty output, which fails every grader and hides the real quality signal. +- Unmatched trajectories, an errored trial, or a summary that disagrees make the comparison + **inconclusive**: the remaining matched trials are biased, so the record is not a measured null + and must not be read as a power or content problem. ### Step 4: Verify the fixtures before touching the skill Run `python eng/eval-quality/check_eval_quality.py` — it blocks ten defect classes that each already cost a real result here. Then confirm by hand: -- every buildable fixture actually builds, and every fixture actually reproduces the bug its - stimulus is named for; +- every fixture behaves as its stimulus assumes — a fixture meant to be healthy builds, and one + meant to be broken fails for the exact reason the stimulus is about and no other; - every referenced fixture is in the git index (`git ls-files`), not merely on disk — `.gitignore` has silently swallowed committed coverage fixtures; - coverage fixtures are self-consistent: declared `line-rate`, summary totals, and the `` @@ -91,11 +102,21 @@ cost a real result here. Then confirm by hand: ### Step 5: Check whether the eval could ever have passed -The gate is an exact one-sided sign test over **discordant** (non-tie) trials. +The gate has two independent bars, and confusing them is the usual misdiagnosis: + +1. **Counted trials ≥ 5** (`trials = stimuli × runs`). Below that the verdict is reported + `underpowered` — never a pass, never a regression. +2. **The sign test must reach p ≤ 0.05 over the *discordant* (non-tie) trials.** Ties are not + discarded silently; they hold the discordant count down. + +| discordant trials | records that pass | p | +|---:|---|---:| +| ≤ 4 | none, however good the skill | ≥ 0.0625 | +| 5–7 | zero losses only (5W/0L) | 0.031 | +| 8 | one loss survivable (7W/1L) | 0.035 | -- Below 5 trials no record can pass, however good the skill. -- At 5–7 trials only a clean sweep passes; one tie makes a pass arithmetically unreachable. -- Tolerating a single loss needs 8 discordant trials. +So at exactly 5 counted trials a single tie is fatal — it leaves 4 discordant. At 6 counted trials +one tie is survivable (5W/1T/0L); at 7, up to two are (5W/2T/0L). A loss is not. So a positive record with a failing verdict is a power problem, not a content problem. Fix it by adding **discriminating stimuli** (cross-task evidence) rather than raising `runs` (repetition @@ -111,8 +132,8 @@ An eval that compares the skill against itself measures judge noise: guard scored −0.4, +0.4, +0.4 and 0, twice costing a skill its pass. - A skill with `disable-model-invocation: true` cannot self-activate, so an eval graded on activation compares two identical arms. Cover it through a consumer skill, or grade the answer - content instead (`tests/dotnet-test/filter-syntax/eval.yaml` is the one such eval here, and its - first real verdict is still outstanding). + content instead, as `tests/dotnet-test/filter-syntax/eval.yaml` and + `tests/dotnet-test/platform-detection/eval.yaml` do. - A grader whose `config` is missing its required key enforces nothing, so the stimulus has one fewer assertion than it appears to. @@ -160,7 +181,7 @@ result, confirm the skill payload actually changed — reruns on byte-identical ## Validation -- [ ] A losing trial and the judge's stated reason are quoted in the PR description. +- [ ] For a content fix, a losing trial and the judge's stated reason are quoted in the PR description. - [ ] The failure was classified before any content was edited. - [ ] `check_eval_quality.py` and `skill-validator check` both pass. - [ ] Trial count clears the power bar for the observed tie rate, not just the floor of 5. diff --git a/.agents/skills/improve-skill-quality/references/eval-triage.md b/.agents/skills/improve-skill-quality/references/eval-triage.md index 86db3263f5..c635894ffb 100644 --- a/.agents/skills/improve-skill-quality/references/eval-triage.md +++ b/.agents/skills/improve-skill-quality/references/eval-triage.md @@ -21,7 +21,7 @@ Symptom → cause → fix, with the PR where each was diagnosed. Use with | Symptom | Cause | Fix | Evidence | |---------|-------|-----|----------| -| Judge penalizes the agent for "pre-existing build issues" | The fixture does not compile | Build every buildable fixture before shipping the eval | PR #949 | +| Judge penalizes the agent for "pre-existing build issues" | A fixture meant to be healthy does not compile | Build every healthy fixture before shipping the eval; a deliberately broken one must fail only for the reason its stimulus is about | PR #949 | | Scenario fails at setup in CI but passes locally | Fixture is on disk but not in the git index (`.gitignore` swallowed it) | Verify with `git ls-files`; the gate now blocks this | PR #945, PR #953 | | Judge says the response "made a critical error" about a number | Cobertura fixture is split-brain: declared `line-rate` disagrees with its `` payload or summary totals | Make declared rate, summary totals and payload agree, then re-derive any rubric quoting a figure | PR #964, PR #945 | | Baseline scores suspiciously well | The fixture never reproduces the bug the stimulus is named for | Rebuild the fixture until it produces the real error | PR #974 | @@ -29,16 +29,18 @@ Symptom → cause → fix, with the PR where each was diagnosed. Use with ## Statistical power -The pass gate is an exact one-sided sign test over discordant trials, and `trials = stimuli × runs`. +The gate has two independent bars: **counted trials ≥ 5** (else `underpowered`), and **p ≤ 0.05 on +an exact one-sided sign test over the discordant (non-tie) trials**. `trials = stimuli × runs`. -| trials | best possible record | p | verdict | -|---:|---|---:|---| -| 1–4 | clean sweep | ≥ 0.0625 | can never pass | -| 5 | 5W/0T/0L | 0.031 | passes only on a sweep | -| 8 discordant | 7W/0T/1L | 0.035 | a single loss becomes survivable | +| discordant trials | records that pass | p | +|---:|---|---:| +| ≤ 4 | none | ≥ 0.0625 | +| 5–7 | zero losses only (5W/0L) | 0.031 | +| 8 | one loss survivable (7W/1L) | 0.035 | -Ties do not count — the test conditions on the discordant (non-tie) trials, so 4W/3T/1L over eight -trials is five discordant trials and fails. +At exactly 5 counted trials one tie is fatal — it leaves 4 discordant. At 6 counted trials one tie is +survivable (5W/1T/0L); at 7, up to two are (5W/2T/0L). A loss is not: 4W/3T/1L over eight trials is +five discordant and fails. Consequences seen in real runs: @@ -61,7 +63,7 @@ Consequences seen in real runs: | A dormancy guard scores randomly across runs | `expect_activation: false` combined with `constraints.reject_skills`, making the skilled arm skill-free and identical to baseline | Use `expect_activation: false` alone | PR #945, PR #953 | | A reference skill shows no improvement | `disable-model-invocation: true` means the model cannot self-activate it, so an activation-graded eval compares identical arms | Cover it through a consumer skill, or grade answer content as `filter-syntax` does | PR #971, PR #976, issue #899 | | An eval "passes" while the skill stopped emitting its signature output | No grader asserts the mandated shape | Add a grader for the exact contract (e.g. the `Recommendation:` line) | PR #904 | -| Overfit score high, user value unclear | Rubric items reward using the skill, or prompts echo skill vocabulary | Assert activation with `expect_tools`; keep rubric items outcome-shaped | PR #904 | +| Overfit score high, user value unclear | Rubric items reward using the skill, or prompts echo skill vocabulary | Drop them: the harness already reports activation separately, so a rubric never needs to. Keep rubric items outcome-shaped and de-cue the prompt | PR #904 | | Both arms write tests and the judge compares volume | Non-activation rubric rewards raw output | Add anti-hijack criteria: do not invoke the skill, do not reward test count | PR #945 | | A grader appears to enforce something but does not | `config:` is missing its required key after an indentation slip | `check_eval_quality.py` blocks it; verify the key is present | `eng/eval-quality/README.md` | | Two stimuli behave identically | Duplicate YAML key — a leftover `prompt:`/`graders:` block overwrites the following stimulus field by field | Delete the stray block after confirming it is not a distinct stimulus that lost its `- name:` | PR #971 | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aca4c4b73b..7495681057 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -244,7 +244,12 @@ Each skill should have an `eval.yaml` file that defines test scenarios. Tests li tests///eval.yaml ``` -A minimal eval file: +The exception is a helper or reference skill that sets `disable-model-invocation: true`. The model +cannot self-activate it, so an activation-graded eval would compare two identical arms. Cover those +through the evals of the skills that load them and through the plugin arm instead. + +The skeleton below shows the shape only — it declares a single trial and would therefore be rejected +by the quality gate. See [Size the eval so it can return a verdict](#size-the-eval-so-it-can-return-a-verdict) for the real bar. ```yaml name: my-skill @@ -280,17 +285,22 @@ Each skill is evaluated in up to three variants — **baseline** (no skills), ** #### Size the eval so it can return a verdict -The pass gate is an exact one-sided sign test over the **discordant** (non-tie) trials, and -`trials = stimuli × runs`. +The pass gate has two independent bars. `trials = stimuli × runs`. + +1. **Counted trials ≥ 5**, else the verdict is reported `underpowered` — never a pass, never a + regression. +2. **p ≤ 0.05 on an exact one-sided sign test over the *discordant* (non-tie) trials.** Ties are not + discarded; they hold the discordant count down. -| trials | best possible record | meaning | -| ---: | --- | --- | -| 1–4 | a clean sweep | no record can pass | -| 5 | 5W/0T/0L | passes only on a clean sweep — one tie makes a pass unreachable | -| 8 discordant | 7W/0T/1L | the point at which a single loss becomes survivable | +| discordant trials | records that pass | p | +| ---: | --- | ---: | +| ≤ 4 | none, however good the skill | ≥ 0.0625 | +| 5–7 | zero losses only (5W/0L) | 0.031 | +| 8 | one loss survivable (7W/1L) | 0.035 | -Ties do not count: the test conditions on the **discordant** (non-tie) trials, so 4W/3T/1L over -eight trials is five discordant trials and fails. Five is an *eligibility floor*, not adequate +At exactly 5 counted trials a single tie is fatal — it leaves 4 discordant. At 6 counted trials one +tie is survivable (5W/1T/0L); at 7, up to two are (5W/2T/0L). A loss is not. Five is an *eligibility +floor*, not adequate power. A run that measured a 32% tie rate certified a genuinely-helping five-trial eval about one time in ten; at fifteen trials, about nine times in ten. Prefer adding **discriminating stimuli** over raising `runs` — repeats measure the same task. See @@ -332,7 +342,7 @@ Per-skill verdicts are written to `./eval-results///results.json` ### CI evaluation -Tests run automatically on pull requests that modify files under `plugins/`. The evaluation workflow discovers changed plugins and evaluates each one. Results are posted as a PR comment and uploaded as build artifacts. +Tests do **not** run automatically on pull requests. When a PR changes skills, the `pr-status` job posts a pending commit status and a maintainer must trigger the evaluation, binding it to a specific reviewed commit — either by submitting a PR review ("Files changed" → "Review changes") whose body contains `/evaluate` (recommended, no SHA to copy), or by commenting `/evaluate `. A bare `/evaluate` comment only posts guidance. Results are posted as a PR comment and uploaded as build artifacts. If a scenario fails or regresses, see [Investigating Results](eng/vally-adapter/InvestigatingResults.md) for how to download artifacts, interpret `results.json`, and diagnose common failure patterns. diff --git a/eng/eval-quality/README.md b/eng/eval-quality/README.md index c58b9b5114..01646ace24 100644 --- a/eng/eval-quality/README.md +++ b/eng/eval-quality/README.md @@ -177,10 +177,11 @@ considerably more. Below it, `eng/vally-adapter/adapt.mjs` marks the verdict This check makes that state un-shippable for *new* evals. > **Landing on 5 exactly is a trap, and the gate now warns about it.** The table -> above is the *best possible* record. At 5, 6 or 7 trials the only record that -> passes is every trial a win — no ties, no losses — because the sign test -> conditions on the discordant trials and one tie at n=5 leaves 4, back below the -> floor. Tolerating a single loss needs 8 discordant trials. +> above is the *best possible* record. A pass needs **five discordant (non-tie) +> trials with no losses**, so at exactly 5 trials a single tie is fatal — it +> leaves 4 discordant, back below the floor. At 6 trials one tie is survivable +> (5W/1T/0L is 5 discordant and passes at p = 0.031) and at 7 trials up to two +> are, but a loss still is not: tolerating one needs 8 discordant trials. > > Run `30611635547` is the worked example. Five `dotnet-test` evals had just been > raised to exactly 5 trials. They returned **16W / 8T / 1L** overall — every @@ -351,9 +352,10 @@ available. See check 8 for how, and for why the floor sits at five. ### Evals parked at the floor -Evals at 5–7 trials, where the only passing record is a flawless sweep. These -*are* eligible for a verdict, so they are not underpowered — they are simply -one tie away from being unable to produce one. See the callout under check 8 for +Evals at 5–7 trials, where a pass still requires a loss-free record and enough +non-tie trials to clear the floor. These *are* eligible for a verdict, so they +are not underpowered — but at 5 trials a single tie removes the possibility of +one, and at 6–7 it takes only one or two more. See the callout under check 8 for the run that made this concrete. Raise them unless their scenarios are near-certain discriminators. From c8b60e8151e02b5c522a76cfa0bb338f98de90ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 3 Aug 2026 09:55:27 +0200 Subject: [PATCH 3/5] Address review: document that agent evals sit outside the verdict flow Abhitej's review found one actionable gap plus three nits; all four are small enough to land here rather than as follow-ups. Agent evals. create-skill-test told authors to write tests//agent./ eval.yaml, size it for the trial floor, and validate with run-skill-evals.sh -- none of which applies. The canonical experiment declares `evals: tests/*/!(agent.*)/eval.yaml`, so agent specs are excluded: no verdict is computed, the floor does not apply, and the runner drops them even when named explicitly because --eval-filter is intersected with that glob. Step 1 now says so, Step 10 shows the EXPERIMENT_FILE route for actually exercising one, and the checklist and pitfalls are scoped accordingly. Nits: aligned eng/eval-quality/README.md check 8 on "stimuli" (it still said "scenarios" while every authoring doc says stimuli); added a .gitignore comment so the next repo-owned authoring skill gets a negation instead of being silently untracked; and noted in improve-skill-quality that eng/skill-validator/src/docs/InvestigatingResults.md documents the retired skill-validator evaluate schema, so the vally-adapter one is the guide to read. Validated: markdownlint clean, check_eval_quality.py clean, 22/22 eval-quality self-tests, gitignore negations still exact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --- .agents/skills/create-skill-test/SKILL.md | 21 ++++++++++++++++++- .agents/skills/improve-skill-quality/SKILL.md | 2 +- .gitignore | 2 ++ eng/eval-quality/README.md | 4 ++-- 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/.agents/skills/create-skill-test/SKILL.md b/.agents/skills/create-skill-test/SKILL.md index bfbc4c248d..a82e3846ed 100644 --- a/.agents/skills/create-skill-test/SKILL.md +++ b/.agents/skills/create-skill-test/SKILL.md @@ -44,6 +44,13 @@ tests//agent./eval.yaml # agents (the agent. prefix disam Verify the target exists at `plugins//skills//SKILL.md` or `plugins//agents/.agent.md`, and read it. +**Agent evals sit outside the verdict flow.** The canonical experiment declares +`evals: tests/*/!(agent.*)/eval.yaml`, so `agent.*` specs are excluded: no verdict is ever computed +for them, the trial floor does not apply, and `./eng/run-skill-evals.sh` drops them even when you +name one explicitly (its `--eval-filter` is intersected with that glob). Everything below about +sizing for statistical power therefore applies to **skill** evals. Author agent evals for the +scenario coverage and the deterministic graders, and run them as described in Step 10. + **Be careful with a skill that sets `disable-model-invocation: true`.** The model cannot invoke it, so any eval graded on the skill self-activating compares two identical arms and returns judge noise. The honest coverage for such skills is dependency-level — through the evals of the skills that load @@ -276,6 +283,16 @@ python eng/eval-quality/check_eval_quality.py ./eng/run-skill-evals.sh ``` +For an **agent** eval, the third command is a no-op: `agent.*` is outside the experiment's `evals:` +glob. Exercise one by pointing the runner at an experiment file whose glob includes it: + +```bash +# copy dotnet-skills.experiment.yaml, widen its evals: glob to tests/*/agent.*/eval.yaml +EXPERIMENT_FILE=my-agent.experiment.yaml ./eng/run-skill-evals.sh +``` + +Read the trajectories rather than the verdict — there is no sign-test result for an agent eval. + `check_eval_quality.py` blocks ten structural defect classes that each already cost a real result: missing or untracked fixtures, self-contradicting coverage fixtures, empty grader configs, dormancy guards with `reject_skills`, sub-floor trial counts, duplicate YAML keys, and `config:`/`defaults:` @@ -288,7 +305,7 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t - [ ] Directory is `tests///` or `tests//agent./` - [ ] Spec uses `stimuli:` / `graders:`, and exactly one of `defaults:` or `config:` -- [ ] `stimuli × runs` clears 5 with room for the expected tie rate +- [ ] For a skill eval, `stimuli × runs` clears 5 with room for the expected tie rate (agent evals are exempt — they get no verdict) - [ ] Each stimulus discriminates a different property - [ ] Prompts never name the skill, the agent, or its vocabulary - [ ] Every referenced fixture exists and is tracked by `git ls-files` @@ -316,4 +333,6 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t | Timeout too short for code generation | Use ~360s; empty output fails every grader | | Duplicate YAML key left behind by an edit | It overwrites the next stimulus field by field — delete the stray block | | Direct activation-graded eval for a `disable-model-invocation: true` skill | Cover it through a consumer skill, or grade the answer content as `filter-syntax` does | +| Agent eval sized for the trial floor | `agent.*` evals get no verdict; size them for scenario coverage instead | +| Agent eval "run" with `./eng/run-skill-evals.sh` | The glob drops it — use a widened `EXPERIMENT_FILE` | | Agent eval missing `environment.skills` | Declare the skills the agent routes to, or the isolated arm cannot use them | diff --git a/.agents/skills/improve-skill-quality/SKILL.md b/.agents/skills/improve-skill-quality/SKILL.md index ae574de44f..bc234761b5 100644 --- a/.agents/skills/improve-skill-quality/SKILL.md +++ b/.agents/skills/improve-skill-quality/SKILL.md @@ -205,4 +205,4 @@ result, confirm the skill payload actually changed — reruns on byte-identical - [references/writing-for-baseline-delta.md](references/writing-for-baseline-delta.md) — content patterns that beat the unskilled model - [references/eval-triage.md](references/eval-triage.md) — symptom, cause and fix catalogue with PR citations - [eng/eval-quality/README.md](../../../eng/eval-quality/README.md) — the ten structural gate checks and why each exists -- [eng/vally-adapter/InvestigatingResults.md](../../../eng/vally-adapter/InvestigatingResults.md) — downloading artifacts and reading `results.json` +- [eng/vally-adapter/InvestigatingResults.md](../../../eng/vally-adapter/InvestigatingResults.md) — downloading artifacts and reading `results.json`. This is the current guide; the similarly-named `eng/skill-validator/src/docs/InvestigatingResults.md` documents the retired `skill-validator evaluate` schema and does not describe today's results. diff --git a/.gitignore b/.gitignore index ca1b89e782..dcd7add7d1 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,8 @@ # Installed skills (other authors' content). The repo's own authoring skills are # re-included below; a directory pattern would make that impossible, so this # ignores the *entries* under .agents/skills/ rather than the directory itself. +# Adding a new repo-owned authoring skill? Add a negation for it here too, or it +# will be silently untracked. .agents/skills/* !.agents/skills/authoring-github-workflows/ !.agents/skills/create-custom-agent/ diff --git a/eng/eval-quality/README.md b/eng/eval-quality/README.md index 01646ace24..0ff70f1447 100644 --- a/eng/eval-quality/README.md +++ b/eng/eval-quality/README.md @@ -147,11 +147,11 @@ measures the real property. ### 8. Fewer than 5 trials behind a verdict -Trials, not scenarios, are what the pass gate is computed over. `vally compare` +Trials, not stimuli, are what the pass gate is computed over. `vally compare` produces one head-to-head trial per stimulus per run, so ``` -trials = scenarios × defaults.runs +trials = stimuli × defaults.runs ``` and the gate is an exact one-sided **sign test**: more wins than losses, at From 85a8c844f67cf729b45f2587442f841b2bd38a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 3 Aug 2026 10:03:46 +0200 Subject: [PATCH 4/5] Generalize two triage rows that were written in test-skill vocabulary The triage catalogue is meant to apply to every plugin, but two rows stated their rule in dotnet-test terms, so an author on another plugin would read past them: - the split-brain fixture row was framed entirely around Cobertura `line-rate`; the rule is that a fixture must never state the same fact twice in two places that disagree, whatever the format. Cobertura is now the named example. - the volume-comparison row said "both arms write tests" / "do not reward test count"; the rule is that when both arms produce the same kind of artifact the judge falls back on comparing quantity. Also generalized the one-line summary of the first rule in SKILL.md Step 4 so the detail and the summary do not disagree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --- .agents/skills/improve-skill-quality/SKILL.md | 5 +++-- .../skills/improve-skill-quality/references/eval-triage.md | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.agents/skills/improve-skill-quality/SKILL.md b/.agents/skills/improve-skill-quality/SKILL.md index bc234761b5..73bf301e9b 100644 --- a/.agents/skills/improve-skill-quality/SKILL.md +++ b/.agents/skills/improve-skill-quality/SKILL.md @@ -97,8 +97,9 @@ cost a real result here. Then confirm by hand: meant to be broken fails for the exact reason the stimulus is about and no other; - every referenced fixture is in the git index (`git ls-files`), not merely on disk — `.gitignore` has silently swallowed committed coverage fixtures; -- coverage fixtures are self-consistent: declared `line-rate`, summary totals, and the `` - elements must all report the same number, or the two arms legitimately read different truths. +- a fixture never states the same fact in two places that disagree — a Cobertura report whose + declared `line-rate`, summary totals and `` elements differ is the canonical case — or the + two arms legitimately read different truths. ### Step 5: Check whether the eval could ever have passed diff --git a/.agents/skills/improve-skill-quality/references/eval-triage.md b/.agents/skills/improve-skill-quality/references/eval-triage.md index c635894ffb..87b5ee2139 100644 --- a/.agents/skills/improve-skill-quality/references/eval-triage.md +++ b/.agents/skills/improve-skill-quality/references/eval-triage.md @@ -23,7 +23,7 @@ Symptom → cause → fix, with the PR where each was diagnosed. Use with |---------|-------|-----|----------| | Judge penalizes the agent for "pre-existing build issues" | A fixture meant to be healthy does not compile | Build every healthy fixture before shipping the eval; a deliberately broken one must fail only for the reason its stimulus is about | PR #949 | | Scenario fails at setup in CI but passes locally | Fixture is on disk but not in the git index (`.gitignore` swallowed it) | Verify with `git ls-files`; the gate now blocks this | PR #945, PR #953 | -| Judge says the response "made a critical error" about a number | Cobertura fixture is split-brain: declared `line-rate` disagrees with its `` payload or summary totals | Make declared rate, summary totals and payload agree, then re-derive any rubric quoting a figure | PR #964, PR #945 | +| Judge says the response "made a critical error" about a value the fixture supplies | The fixture states the same fact in two places and they disagree, so each arm can legitimately read a different truth — e.g. a Cobertura report whose declared `line-rate` contradicts its `` payload or summary totals | Make every representation of the value agree, then re-derive any rubric or prompt that quotes it | PR #964, PR #945 | | Baseline scores suspiciously well | The fixture never reproduces the bug the stimulus is named for | Rebuild the fixture until it produces the real error | PR #974 | | `n` rose but power did not | Duplicate or rename-leftover fixtures wired in as new stimuli | Delete byte-equivalent leftovers; only wire fixtures exercising new behavior | PR #971, PR #945 | @@ -64,7 +64,7 @@ Consequences seen in real runs: | A reference skill shows no improvement | `disable-model-invocation: true` means the model cannot self-activate it, so an activation-graded eval compares identical arms | Cover it through a consumer skill, or grade answer content as `filter-syntax` does | PR #971, PR #976, issue #899 | | An eval "passes" while the skill stopped emitting its signature output | No grader asserts the mandated shape | Add a grader for the exact contract (e.g. the `Recommendation:` line) | PR #904 | | Overfit score high, user value unclear | Rubric items reward using the skill, or prompts echo skill vocabulary | Drop them: the harness already reports activation separately, so a rubric never needs to. Keep rubric items outcome-shaped and de-cue the prompt | PR #904 | -| Both arms write tests and the judge compares volume | Non-activation rubric rewards raw output | Add anti-hijack criteria: do not invoke the skill, do not reward test count | PR #945 | +| Both arms produce the same kind of artifact and the judge falls back on comparing volume | The rubric rewards raw output instead of the property under test | Add anti-hijack criteria: do not invoke the skill, and do not reward quantity (number of tests, findings, or lines produced) | PR #945 | | A grader appears to enforce something but does not | `config:` is missing its required key after an indentation slip | `check_eval_quality.py` blocks it; verify the key is present | `eng/eval-quality/README.md` | | Two stimuli behave identically | Duplicate YAML key — a leftover `prompt:`/`graders:` block overwrites the following stimulus field by field | Delete the stray block after confirming it is not a distinct stimulus that lost its `- name:` | PR #971 | | Eval measures path recall | The skill is a map to reference files | Do not create the eval; test the consumer's outcome instead | PR #974 | From 13e67d16e1ebd3e095a0865453919ed0878689ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Mon, 3 Aug 2026 10:12:41 +0200 Subject: [PATCH 5/5] Correct the environment.skills guidance and a stale cross-reference Two suppressed findings from the Copilot review summaries, both real. environment.skills. The Step 5 skeleton documented it as "extra skills loaded in the isolated arm only", which is wrong in both halves. The experiment declares `vary: /environment/skills` and supplies the value for each arm itself (`[]` for baseline, `plugins//skills/` for skilled), so whatever a skill eval declares is replaced in every arm -- it cannot add a skill to one arm, and in a skill eval it does nothing at all. The key is meaningful only in `agent.*` evals, which the experiment does not vary. Removed it from the generic skeleton, documented where it does apply, and pointed at an existing agent eval to copy rather than reproducing a spelling: the three agent specs in this repo disagree about whether entries are bare names or relative paths, so the doc should not assert one. Stale cross-reference. Moving the fixture row to the top of the triage table last round left the sentence below it still saying a fixture failure "also matches the two rows above it", when it is now the first row. Reworded to say why it leads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --- .agents/skills/create-skill-test/SKILL.md | 14 +++++++++++--- .agents/skills/improve-skill-quality/SKILL.md | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.agents/skills/create-skill-test/SKILL.md b/.agents/skills/create-skill-test/SKILL.md index a82e3846ed..20e27150b6 100644 --- a/.agents/skills/create-skill-test/SKILL.md +++ b/.agents/skills/create-skill-test/SKILL.md @@ -142,10 +142,17 @@ environment: dest: . commands: - dotnet build -bl || exit 0 # guard intentional failures - skills: - - binlog-failure-analysis # extra skills loaded in the isolated arm only ``` +**Do not set `environment.skills` in a skill eval.** The experiment declares +`vary: /environment/skills` and supplies the value itself — `[]` for the baseline arm and +`plugins//skills/` for the skilled arm — so anything the eval declares is replaced, +in every arm. It cannot add a skill to one arm only. `environment.skills` is meaningful only in an +`agent.*` eval, which the experiment does not vary; there it is the set of skills the agent may +invoke. Copy the shape from an existing agent eval such as +`tests/dotnet-test/agent.test-quality-auditor/eval.yaml` rather than reproducing a remembered form — +the specs in this repo are not consistent about how they spell those entries. + Fixture rules — each one has already cost a real result: - **Every referenced fixture must be tracked by git.** `.gitignore` (e.g. `coverage*.xml`) has @@ -335,4 +342,5 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t | Direct activation-graded eval for a `disable-model-invocation: true` skill | Cover it through a consumer skill, or grade the answer content as `filter-syntax` does | | Agent eval sized for the trial floor | `agent.*` evals get no verdict; size them for scenario coverage instead | | Agent eval "run" with `./eng/run-skill-evals.sh` | The glob drops it — use a widened `EXPERIMENT_FILE` | -| Agent eval missing `environment.skills` | Declare the skills the agent routes to, or the isolated arm cannot use them | +| Agent eval missing `environment.skills` | Declare the skills the agent routes to, or it cannot invoke them | +| `environment.skills` set in a **skill** eval | The experiment varies that key and replaces it in every arm; the declaration does nothing | diff --git a/.agents/skills/improve-skill-quality/SKILL.md b/.agents/skills/improve-skill-quality/SKILL.md index 73bf301e9b..401e90d42d 100644 --- a/.agents/skills/improve-skill-quality/SKILL.md +++ b/.agents/skills/improve-skill-quality/SKILL.md @@ -52,8 +52,8 @@ demanding one is what sends people rewriting prose instead. ### Step 2: Classify the failure Work down this table and stop at the first row that matches. Rows are ordered by how often the -symptom has been misdiagnosed as a skill-content problem. A setup or trial failure that traces to a -fixture belongs in the Fixture row even though it also matches the two rows above it. +symptom has been misdiagnosed as a skill-content problem — the fixture row is first because a +fixture failure also presents as a setup or reliability failure and gets misfiled as one. | Symptom | Real cause class | Go to | |---------|------------------|-------|