Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 29 additions & 26 deletions .agents/skills/create-skill-test/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ Verify the target exists at `plugins/<plugin>/skills/<skill-name>/SKILL.md` or

**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
for them, the stimulus 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). The distinct-stimulus
floor therefore applies to **skill** evals only. 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,
Expand Down Expand Up @@ -89,34 +89,33 @@ stimuli:
```

> **`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
> same block and vally **throws** on a spec declaring both. Some existing evals still open with
> `config:`; when you change settings, replace it with one `defaults:` block. The failure is
> invisible otherwise: the job exits 0 with no verdicts and the PR comment
> blames "transient infrastructure".

### Step 3: Size the eval for power before writing content

`trials = stimuli × runs`, and the gate has two independent bars:
The gate gives each distinct stimulus one vote. Repeated runs for one stimulus collapse to one
majority-direction vote and remain available as reliability evidence.

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
1. **Distinct stimuli ≥ 5**, else the verdict is `underpowered` — never a pass, never a regression.
2. **p ≤ 0.05 on an exact one-sided sign test over *discordant* (non-tie) stimulus votes.** Ties are not
discarded; they hold the discordant count down.

| discordant trials | records that pass | p |
| discordant stimulus votes | 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.
At exactly 5 stimuli, one tie is fatal because it leaves 4 discordant votes. At 6 stimuli one tie
is survivable; at 7, up to two are. A loss is not. Five is an **eligibility floor**, not adequate
power. For example, 80% power needs 8 discordant votes only for a true 90% conditional win rate;
it needs 18 at 80%, 37 at 70%, and 158 at 60%. Size for the effect and tie rate you need to detect.

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:`.
Use `runs` for reliability, not task breadth. Vally recommends 3 runs in CI and 5–10 nightly for
pass rate, pass@k, pass^k, and flakiness. Extra runs never clear the five-stimulus floor.

Do not set `runs` in `dotnet-skills.experiment.yaml`; experiment overrides overwrite every eval's
own value rather than defaulting it.
Expand All @@ -128,6 +127,8 @@ own value rather than defaulting it.
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.
- Give every stimulus a stable, unique `name`. Vally pairs comparison trajectories by
`(stimulus name, trial index)`; duplicate names make slot identity ambiguous.
- Include a boundary / no-op stimulus for any skill that migrates or rewrites code, proving it
leaves already-correct input alone.

Expand Down Expand Up @@ -300,10 +301,11 @@ EXPERIMENT_FILE=my-agent.experiment.yaml ./eng/run-skill-evals.sh <plugin>

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:
`check_eval_quality.py` blocks eleven structural defect classes that can corrupt a 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
guards with `reject_skills`, sub-floor stimulus counts, duplicate YAML keys or stimulus names, 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.

For the official run, submit a PR review containing `/evaluate` so it binds to the reviewed commit.
Expand All @@ -312,8 +314,8 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t

- [ ] Directory is `tests/<plugin>/<skill-name>/` or `tests/<plugin>/agent.<agent-name>/`
- [ ] Spec uses `stimuli:` / `graders:`, and exactly one of `defaults:` or `config:`
- [ ] 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
- [ ] For a skill eval, at least 5 distinct stimuli exist, with more for the effect and tie rate that must be detected (agent evals are exempt)
- [ ] Each stimulus discriminates a different property and has a stable, unique name
- [ ] Prompts never name the skill, the agent, or its vocabulary
- [ ] 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
Expand All @@ -329,8 +331,8 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t
|---------|----------|
| 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 |
| Landing an eval at exactly 5 stimuli | A single tie makes a pass unreachable; size for the effect and tie rate |
| Raising `runs` to clear the floor | Repeats measure reliability for one task; add stimuli |
| Prompt mentions the skill or agent by name | Rewrite as a natural developer request |
| 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 |
Expand All @@ -339,8 +341,9 @@ For the official run, submit a PR review containing `/evaluate` so it binds to t
| `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 |
| Duplicate stimulus names | Vally uses names as comparison identity — give every stimulus a stable, unique name |
| 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 sized for the stimulus 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 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 |
4 changes: 2 additions & 2 deletions .agents/skills/create-skill/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ 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 counted trials can never return a passing verdict.
below five distinct stimuli 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
Expand Down Expand Up @@ -218,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 (or the skill is `disable-model-invocation: true` and covered through its consumers)
- [ ] An `eval.yaml` exists and clears the distinct-stimulus floor (or the skill is `disable-model-invocation: true` and covered through its consumers)

## Common Pitfalls

Expand Down
27 changes: 13 additions & 14 deletions .agents/skills/improve-skill-quality/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ the fixtures, or the harness. Classify first, then fix.
|-------|----------|-------------|
| 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 |
| Stimulus-vote W/T/L and repeated-run W/T/L | Yes | Separates cross-task evidence from reliability |
| Activation status per arm | Yes | Isolated and plugin activation are different failures |

## Workflow
Expand All @@ -39,7 +39,7 @@ the fixtures, or the harness. Classify first, then fix.
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`)
- authoritative stimulus-vote W/T/L and separate repeated-run W/T/L
- 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
Expand Down Expand Up @@ -90,7 +90,7 @@ See [references/eval-triage.md](references/eval-triage.md) for the full catalogu

### 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
Run `python eng/eval-quality/check_eval_quality.py` — it blocks eleven defect classes that can
cost a real result here. Then confirm by hand:

- every fixture behaves as its stimulus assumes — a fixture meant to be healthy builds, and one
Expand All @@ -105,24 +105,23 @@ cost a real result here. Then confirm by hand:

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
1. **Distinct stimuli ≥ 5.** 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
2. **The sign test must reach p ≤ 0.05 over the *discordant* (non-tie) stimulus votes.** Ties are not
discarded silently; they hold the discordant count down.

| discordant trials | records that pass | p |
| discordant stimulus votes | 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 |

So at exactly 5 counted trials a single tie is fatal — it leaves 4 discordant. At 6 counted trials
So at exactly 5 stimuli a single tie is fatal — it leaves 4 discordant. At 6 stimuli
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
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.
adding **discriminating stimuli**. Raising `runs` measures reliability for the same task and cannot
clear the floor.

### Step 6: Check whether the two arms differ at all

Expand Down Expand Up @@ -185,17 +184,17 @@ result, confirm the skill payload actually changed — reruns on byte-identical
- [ ] 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.
- [ ] Distinct-stimulus count clears the power bar for the target effect and observed tie rate.
- [ ] 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 |
| Rewriting skill prose in response to an underpowered verdict | Underpowered means too few distinct stimuli; 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 |
| Padding `runs` to clear the stimulus floor | Repeats measure reliability for 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 |
Expand All @@ -205,5 +204,5 @@ 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/eval-quality/README.md](../../../eng/eval-quality/README.md) — the eleven structural gate checks and why each exists
- [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.
25 changes: 12 additions & 13 deletions .agents/skills/improve-skill-quality/references/eval-triage.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,29 @@ Symptom → cause → fix, with the PR where each was diagnosed. Use with

## Statistical power

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`.
The gate has two independent bars: **distinct stimuli ≥ 5** (else `underpowered`), and **p ≤ 0.05
on an exact one-sided sign test over discordant (non-tie) stimulus votes**. Repeated runs collapse
to one vote per stimulus and remain reliability evidence.

| discordant trials | records that pass | p |
| discordant stimulus votes | 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 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.
At exactly 5 stimuli one tie is fatal — it leaves 4 discordant votes. At 6 stimuli one tie is
survivable; at 7, up to two are. A loss is not: 4W/3T/1L over eight stimulus votes fails.

Consequences seen in real runs:

- Five `dotnet-test` evals raised to exactly 5 trials returned 16W/8T/1L overall — every skill
- Five `dotnet-test` evals raised to exactly 5 distinct stimuli 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
- At the 32% tie rate measured there, a genuinely-helping skill parked at 5 stimulus votes is
certified about one run in ten; at 15 stimulus votes, about nine in ten.
- Adding stimuli increases task breadth. Raising `runs` measures reliability for the same tasks and
cannot clear the stimulus floor.
- The verdict reads each repeated run'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)

Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/eval-quality.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ on:
- "tests/**"
- "plugins/**"
- "eng/eval-quality/**"
# check_floor_agreement() reads MIN_CREDIBLE_TRIALS out of the adapter, so
# check_floor_agreement() reads MIN_CREDIBLE_STIMULI out of the adapter, so
# the one diff that can break the floor's two-language agreement is a diff
# to the adapter. Without this the guard would never run on it.
- "eng/vally-adapter/**"
Expand All @@ -25,7 +25,7 @@ on:
branches: [main]
# Kept in sync with the pull_request paths above. The gate reads plugins/*
# (skills with no eval), .gitignore (fixtures excluded from the index), and
# eng/vally-adapter (the trial floor it must agree with), so omitting any of
# eng/vally-adapter (the stimulus floor it must agree with), so omitting any of
# them here would let a direct push or a squash-merge that touches only
# those land on main without the gate ever running.
paths:
Expand Down
Loading