Capture the repo's hard-won skill-authoring lessons as guidance - #979
Conversation
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
There was a problem hiding this comment.
Pull request overview
This PR consolidates repository-specific “skill authoring” and evaluation hard-earned lessons into first-class guidance, updating contributor docs and the repo’s own authoring skills to reflect the current Vally eval schema and common failure modes.
Changes:
- Adds a new
.agents/skills/improve-skill-qualitytriage playbook plus reference docs for baseline-delta writing and eval failure diagnosis. - Updates
.agents/skills/create-skilland rewrites.agents/skills/create-skill-testto emphasize routing-by-description, Vallystimuli/graders/defaults, statistical power, and fixture hygiene. - Updates CONTRIBUTING/AGENTS and adjusts
.gitignoreso the repo’s own authoring skills aren’t accidentally ignored.
Show a summary per file
| File | Description |
|---|---|
CONTRIBUTING.md |
Refreshes eval authoring guidance (Vally schema, defaults vs config, power sizing, and experiment runs caveat). |
AGENTS.md |
Documents the repo’s intended “use the authoring skills + run eval-quality gate” workflow. |
.gitignore |
Stops ignoring the repo-owned .agents/skills/* directories while still ignoring installed third-party skills. |
.agents/skills/improve-skill-quality/SKILL.md |
New triage workflow for diagnosing regressions/no-verdict/no-activation/cost issues before rewriting content. |
.agents/skills/improve-skill-quality/references/writing-for-baseline-delta.md |
Reference patterns for writing skill content that beats baseline. |
.agents/skills/improve-skill-quality/references/eval-triage.md |
Symptom → cause → fix catalogue for harness/fixtures/power/design/activation issues. |
.agents/skills/create-skill/SKILL.md |
Extends skill scaffolding guidance (description-as-router, baseline-delta writing patterns, validation). |
.agents/skills/create-skill-test/SKILL.md |
Rewrites eval authoring guidance to the current Vally schema and codifies power/fixture/overfitting rules. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
.agents/skills/create-skill/SKILL.md:117
- Same path issue as the earlier directory snippet: this optional-layout example should point at
plugins/<plugin>/skills/<skill-name>/rather thanskills/<skill-name>/to match the repo layout.
skills/<skill-name>/
- Files reviewed: 7/8 changed files
- Comments generated: 2
|
👋 @Evangelink — this PR has 2 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
… 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/<plugin>/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
Review roundsRan three independent review passes over the diff, each pointed at the implementation rather than the prose: Copilot, Claude Opus 4.8, and GPT-5.6 (Sol + Terra). All findings verified against Accepted (10)The statistics were wrong, and both Copilot and Opus caught it independently. The docs conflated the gate's two independent bars.
Also corrected outside the original scope
Rejected (2)
Validation
|
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
.agents/skills/create-skill-test/SKILL.md:174
- The
run-commandrow listsstdout_matchesas an optional config key, but existing evals also usestdout_contains(e.g. tests/dotnet-test-migration/migrate-xunit-to-mstest/eval.yaml:51-56). Includingstdout_containshere would prevent readers from missing a commonly-used (and often more stable) assertion option for run-command graders.
| `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 |
eng/eval-quality/README.md:183
- This callout correctly explains that ties at 6–7 counted trials can still pass (because the sign test is over discordant trials), but the current
check_eval_quality.pywarning text for 5–7 trials still says “every trial a win with no ties… one tie makes them unwinnable” (eng/eval-quality/check_eval_quality.py:406-412). That mismatch will confuse contributors when the gate prints the warning. Consider updating the gate’s warning message to match the discordant-trials explanation here (or adjusting the wording here until the warning copy is updated).
> **Landing on 5 exactly is a trap, and the gate now warns about it.** The table
> 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
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
Review — PR #979Thanks for putting this together, Amaury — this is a thorough extraction of lessons that were previously scattered across PR bodies and a few people's heads. What I verified
Findings1. Agent eval path guidance gap (worth addressing) 2. Pre-existing terminology drift in 3. 4. SummaryThis is excellent work. The (Copilot, commenting on Abhitej's behalf.) |
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/<plugin>/agent.<name>/ 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
AbhitejJohn
left a comment
AbhitejJohn
left a comment
There was a problem hiding this comment.
Thanks for putting this together.
|
Thanks @AbhitejJohn — all four are addressed in c8b60e8. No follow-up tickets needed; each turned out to be a few lines in files this PR already touches. 1. Agent eval gap — fixed, and it was worse than a doc gapYou were right that this needed addressing. Confirmed the mechanism: # dotnet-skills.experiment.yaml
evals:
- tests/*/!(agent.*)/eval.yamland Three changes:
2.
|
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
.agents/skills/create-skill-test/SKILL.md:146
- In the environment skeleton, the comment on
environment.skillssays these are "extra skills loaded in the isolated arm only", but there’s nothing in this YAML shape that would scope them to only the skilled/isolated arm. In the canonical skill-vs-baseline experiment,environment.skillsis supplied by the experiment variants; in agent.* evals,environment.skillsis the set of skills the agent may invoke. This wording is likely to mislead authors into thinking they can safely add skills without contaminating the baseline arm.
skills:
- binlog-failure-analysis # extra skills loaded in the isolated arm only
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
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
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
.agents/skills/improve-skill-quality/SKILL.md:56
- The sentence says a fixture-related failure "also matches the two rows above it", but the Fixture row is the first row in the table. This reads as a direction error and makes the classification guidance confusing.
fixture belongs in the Fixture row even though it also matches the two rows above it.
.agents/skills/create-skill-test/SKILL.md:146
- In the Vally-based evaluation harness,
environment.skillsentries are treated as paths to skill directories (each must be a directory containing aSKILL.md, e.g.plugins/<plugin>/skills/<skill>). Using a bare skill name likebinlog-failure-analysisin this example is likely to produce a spec that doesn't load the dependency as intended.
skills:
- binlog-failure-analysis # extra skills loaded in the isolated arm only
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
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/<plugin>/skills/<skill>` 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
|
Acting on two suppressed findings from the Copilot review summaries — flagging here because one of them changed documented guidance after your approval, @AbhitejJohn.
# dotnet-skills.experiment.yaml
vary:
- /environment/skills
variants:
baseline: { environment: { skills: [] } }
skilled: { environment: { skills: [plugins/${eval.grandparent}/skills/${eval.parent}] } }The experiment supplies that key per arm, so anything 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. It is meaningful only in While checking, I noticed the three agent specs in this repo disagree on how those entries are spelled — Stale cross-reference: moving the fixture row to the top of the triage table last round left the sentence beneath it saying a fixture failure "also matches the two rows above it". Reworded. Both in 13e67d1. markdownlint and |
Why
Our skills score well, but the reasons live in PR bodies and in a handful of engineers' heads. This mines ~35 merged PRs (#830–#976), issue #899, and
eng/eval-quality/README.mdinto guidance contributors can actually follow, and fixes two latent traps found along the way.What
New:
.agents/skills/improve-skill-quality— a triage playbook for an eval that regressed, returned no verdict, or failed to activate. Its core claim is the one lesson the history repeats loudest: classify before you rewrite. 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 (writing-for-baseline-delta.md) and the symptom → cause → fix catalogue with PR citations (eval-triage.md).Rewritten:
.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 intests/usestimuli:/graders:/environment. Anyone following the skill today authored a broken eval. The rewrite also folds in the trial floor, the dormancy-guard rule, and the fixture rules (tracked by git, buildable, self-consistent, actually reproduces its bug).Extended:
.agents/skills/create-skill— description-as-router rules (trigger words, sibling partitioning, exclusion audit, menu budget) and a "write for delta over the baseline model" section.CONTRIBUTING.md— a "what consistently separates a passing skill from a failing one" playbook, eval sizing with the sign-test arithmetic, a corrected eval snippet, and a fix to the stale "raiserunsindotnet-skills.experiment.yaml" advice — an experiment-levelrunsoverwrites every eval's own value rather than defaulting it..gitignore—.agents/skills/was silently swallowing the repo's own authoring skills; the new one only landed because I checked. Replaced with.agents/skills/*plus negations for the five owned directories, so installed third-party skills stay ignored. (This is the same defect class as the.gitignore-swallowed coverage fixture in #945.)Validation
markdownlint-cli2— 0 errorspython eng/eval-quality/check_eval_quality.py— no errorsrunsadvice, an over-absolutedisable-model-invocationrule thatfilter-syntaxcontradicts, a wrong "8 trials tolerates a loss" row, andrun-command's required config) — all fixed before this was pushed.No files under
plugins/change, so no evaluation is triggered.