Fix dotnet-test findings from the refreshed cross-family eval (#899)#945
Open
Evangelink wants to merge 8 commits into
Open
Fix dotnet-test findings from the refreshed cross-family eval (#899)#945Evangelink wants to merge 8 commits into
Evangelink wants to merge 8 commits into
Conversation
Every change below is driven by judge evidence from the losing trials of the refreshed 5-family dotnet-test matrix (runs 30108473397 + recovery runs), not by style preference. Eval measurement fix — the "discovery" P2s were an artifact: - assertion-quality, test-gap-analysis, test-smell-detection, and test-tagging each have a decline stimulus with `constraints.reject_skills: ["*"]`, so the skill cannot activate there by construction. Without `expect_activation: false` the adapter counted those dormant runs as missed activations, which is exactly the 75-88% invocation rates reported in the scorecard. Annotating them (the convention already used by agent.test-quality-auditor) removes the false signal; the non-activations were the only ones observed for these skills. Skill fixes: - test-gap-analysis: baselines won by actually running the suite while the skill reasoned statically and reported survivors that the tests in fact kill. Added Step 4b: confirm every reported survivor by applying it, re-running the covering tests, and reverting; fall back to reasoning only when the suite cannot run, labelled unverified. Calibrated severity down for strong suites. - test-anti-patterns: baselines won on depth, not polish. Added a depth bar — account for every test in scope, give exact expected values in fixes, name the adjacent error-path/boundary gaps, and keep counts consistent. Trimmed three pitfall rows that duplicated the calibration step so the skill stays under the profiler's "comprehensive" threshold. - detect-static-dependencies: losses were all counting accuracy. One authoritative total (no findings parked outside it), classify by the resource touched rather than by the `static` keyword, exclude pure helpers such as Path.Combine from the needs-wrapping total, require file:line, and add the missing randomness/culture/serialization categories. - test-smell-detection: the calibration rule told models to downgrade Sleepy Test for integration tests, which is what lost both losing scenarios. Fixed sleeps now stay High in any category; Mystery Guest and Eager Test still downgrade. - crap-score: losses came from estimating coverage after collection failed. Added the dotnet-coverage/ReportGenerator recovery path and a hard rule never to publish a CRAP score built on assumed coverage. - coverage-analysis: answer the asked question first, reconcile every number against the script output, and list every below-threshold member instead of declaring one method the entire gap. - migrate-static-to-wrapper: migrate exactly what was requested (no adjacent DateTime.Now rewrites, respect intentional-use comments) and never report "build succeeded" when the build or restore failed. - code-testing-agent: quote each requirement verbatim in the evidence table so multi-condition requirements map to a test that covers the whole combination, and cite a clean run rather than a coverage attempt that exited non-zero. Validation: skill-validator check passes (20 skills, 10 agents); markdownlint clean; eval specs parse and the adapter now reports all four decline stimuli as expect-dormant. Refs #899 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Contributor
Skill Coverage Report
Uncovered:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes dotnet-test cross-family eval findings by (1) correcting evaluation metadata so “decline / reject-skills” stimuli are treated as expected non-activations, and (2) tightening several skill instructions to match judge evidence (accuracy, calibration, and reporting discipline).
Changes:
- Added
expect_activation: falseto decline stimuli that explicitly reject skills, preventing dormant runs from being counted as missed activations. - Updated multiple dotnet-test skill guides to address observed failure modes (e.g., empirically verifying pseudo-mutation “survivors”, not downgrading fixed sleeps in integration tests, stricter scope/build-result reporting).
- Strengthened reporting/validation checklists in several skills to reduce inconsistencies and under-specified evidence.
Show a summary per file
| File | Description |
|---|---|
| tests/dotnet-test/test-tagging/eval.yaml | Marks reject-skills decline stimulus as expected-dormant via expect_activation: false. |
| tests/dotnet-test/test-smell-detection/eval.yaml | Marks reject-skills decline stimulus as expected-dormant via expect_activation: false. |
| tests/dotnet-test/test-gap-analysis/eval.yaml | Marks reject-skills decline stimulus as expected-dormant via expect_activation: false. |
| tests/dotnet-test/assertion-quality/eval.yaml | Marks reject-skills decline stimulus as expected-dormant via expect_activation: false. |
| plugins/dotnet-test/skills/test-smell-detection/SKILL.md | Keeps Sleepy Test severity high even for integration tests; updates calibration/validation/pitfalls accordingly. |
| plugins/dotnet-test/skills/test-gap-analysis/SKILL.md | Adds Step 4b to verify reported survivors by actually applying mutations and re-running tests; calibrates messaging for strong suites. |
| plugins/dotnet-test/skills/test-anti-patterns/SKILL.md | Adds explicit “depth bar” and validation checks to avoid shallow/inconsistent reports. |
| plugins/dotnet-test/skills/migrate-static-to-wrapper/SKILL.md | Enforces strict migration scope and truthful build-result reporting; expands validation/pitfalls. |
| plugins/dotnet-test/skills/detect-static-dependencies/SKILL.md | Improves counting rules and categorization (hidden deps, reconcile totals, exclude pure helpers), adds missing categories, trims promotional next steps. |
| plugins/dotnet-test/skills/crap-score/SKILL.md | Adds a “never estimate coverage” rule with fallback collection paths before computing CRAP. |
| plugins/dotnet-test/skills/coverage-analysis/SKILL.md | Requires answering the user’s question early and reconciling all numbers; ensures all below-threshold members are listed. |
| plugins/dotnet-test/skills/code-testing-agent/SKILL.md | Requires verbatim requirement quoting per evidence row and citing only clean (exit 0) runs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 0
github-actions
Bot
added
the
pr-state/ready-for-eval
Contributor
📊 Skill Evaluation Results10 skill(s) evaluated — 5 improved, 5 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
✅ assertion-quality — detailsReason: Mean preference +32.0% [95% CI 9.8%, 54.2%], win rate 80.0% (4W/1T/0L over 5 trial(s)) — credibly better
❌ code-testing-agent — detailsReason: Mean preference +20.0% [95% CI -234.1%, 274.1%], win rate 50.0% (1W/1T/0L over 2 trial(s)) — not credible (95% CI includes 0)
✅ coverage-analysis — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — credibly better
❌ crap-score — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
✅ detect-static-dependencies — detailsReason: Mean preference +50.0% [95% CI 24.3%, 75.7%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
❌ migrate-static-to-wrapper — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
❌ test-anti-patterns — detailsReason: Mean preference +20.0% [95% CI -3.0%, 43.0%], win rate 50.0% (3W/3T/0L over 6 trial(s)) — not credible (95% CI includes 0)
❌ test-gap-analysis — detailsReason: Mean preference +30.0% [95% CI -1.8%, 61.8%], win rate 75.0% (3W/1T/0L over 4 trial(s)) — not credible (95% CI includes 0)
✅ test-smell-detection — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (4W/0T/0L over 4 trial(s)) — credibly better
✅ test-tagging — detailsReason: Mean preference +45.0% [95% CI 8.4%, 81.6%], win rate 87.5% (7W/0T/1L over 8 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
github-actions
Bot
added
waiting-on-review
Contributor
|
✅ Evaluation passed for |
The PR eval reported 5 of 10 skills as "no credible improvement". Reproducing the gate arithmetic from the artifacts shows the dominant cause is statistical power, not skill quality. The gate is `mean > 0 AND ci_low > 0` with a t-based CI over per-trial scores, which reduces to `sqrt(n) * (mean/sd) > t(n-1)`. The required mean/sd ratio is brutal at small n: n=2 -> 8.98 n=3 -> 2.48 n=4 -> 1.59 n=5 -> 1.24 n=6 -> 1.05 n=8 -> 0.84 Recomputing each reported CI from the per-trial scores reproduces the published numbers exactly, which confirms the mechanism: crap-score [0.4,1.0,0.4] n=3 CI [-0.261, 1.461] migrate-static-to-wrapper [1.0,0.4,0.4] n=3 CI [-0.261, 1.461] test-gap-analysis [0.4,0,0.4,0.4] n=4 CI [-0.018, 0.618] test-anti-patterns [0.4,0,0.4,0,0,0.4] n=6 CI [-0.030, 0.430] code-testing-agent [0,0.4] n=2 CI [-2.341, 2.741] crap-score and migrate-static-to-wrapper won 100% of their trials (3W/0T/0L) and still failed: at n=3 nothing short of three identically-sized wins can clear the gate. That is a property of a thin eval, not of the skill. Scenario counts are raised with discriminating cases, four of them by wiring up fixtures that were already committed but had no stimulus referencing them: - test-gap-analysis 4 -> 6, using the orphaned `report-quality` fixture (trivial auto-properties and an auto-generated .g.cs to skip, private helpers reachable only through the public API, and a deliberately weak Assert.IsTrue that cannot kill arithmetic mutations) and the orphaned `rust-error-propagation` fixture (an untested `?` propagation path and an untested `<=` boundary). - test-anti-patterns 6 -> 8, using the orphaned `pytest-mixed` fixture (which also checks the calibration rule that pytest's bare `assert` must not be flagged) and the orphaned `assertion-problems` fixture (which separates Critical false-confidence assertions from a Low-severity message nit). - crap-score 3 -> 6, with a new `refactor-required` fixture whose numbers are self-consistent: ApplySurcharges has complexity 13 behind a stale "Complexity: 4" comment (CRAP 28.4, needs 77.2% coverage), ClassifyAccount has complexity 17 so coverage alone can never reach CRAP < 15, and RoundToCurrency is 100% covered so its CRAP equals its complexity exactly. - migrate-static-to-wrapper 3 -> 5, adding a DateTimeKind-preservation scenario over the existing fixture and a new `static-helper` fixture where a static class must gain an ambient TimeProvider seam without breaking its callers. - code-testing-agent 2 -> 3, with a compact C# fixture that must extend an existing suite to the untested method only. This eval stays the weakest: each scenario is expensive, so raising `runs` is a better lever than adding more heavyweight scenarios. Verification: - every eval spec parses and all 254 fixture references resolve - the three new fixtures compile; the shipping-quotes fixture restores, builds and its three seed tests pass under `dotnet test` in a clean workspace - skill-validator check passes (20 skills, 10 agents) - markdownlint clean Refs #899 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
Diagnosing the 5 "no credible improvement" resultsI pulled the artifacts from run 30150316653 and recomputed the gate arithmetic from the per-trial scores. The dominant cause is statistical power, not skill quality. The gate is The required
Recomputing each published CI from its per-trial scores reproduces the reported numbers exactly, which confirms the mechanism:
The headline consequence: Secondary cause: two structural sources of zeros
What this commit changesScenario counts are raised with discriminating cases. Four of the new scenarios wire up fixtures that were already committed to the repo but had no stimulus referencing them — purpose-built discriminators that were simply never connected:
The new
Projected effect, assuming observed per-scenario behaviour repeats and the new scenarios win at the modal +0.4:
Known residual:
|
github-actions
Bot
added
waiting-on-author
Contributor
|
👋 @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 |
- BillableWeightTests: the ZeroOrNegative test only asserted the zero case, so its name overstated what it covered. Made it data-driven over 0 and -1 so the name matches the assertions. This matters more than usual here: the file is the seed suite for a test-quality eval, and a misleading test name is exactly what these skills are supposed to flag. - detect-static-dependencies: the Step 3 lead-in said to count each "static call pattern", which contradicted the rule immediately below it that instance members reaching the same untestable resource must also be counted. Reworded to "call site" and made the instance-member inclusion explicit. Verified: the fixture restores, builds and now passes 4 tests (was 3); skill-validator check passes; markdownlint clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate |
Evangelink
enabled auto-merge (squash)
July 26, 2026 19:40
Contributor
📊 Skill Evaluation Results9 skill(s) evaluated — 6 improved, 3 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
❌ assertion-quality — detailsReason: Mean preference +16.0% [95% CI -28.4%, 60.4%], win rate 60.0% (3W/1T/1L over 5 trial(s)) — not credible (95% CI includes 0)
❌ code-testing-agent — detailsReason: Mean preference +0.0% [95% CI -99.4%, 99.4%], win rate 33.3% (1W/1T/1L over 3 trial(s)) — no improvement
✅ coverage-analysis — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — credibly better
✅ detect-static-dependencies — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
❌ migrate-static-to-wrapper — detailsReason: Mean preference +20.0% [95% CI -54.5%, 94.5%], win rate 60.0% (3W/0T/2L over 5 trial(s)) — not credible (95% CI includes 0)
✅ test-anti-patterns — detailsReason: Mean preference +30.0% [95% CI 14.5%, 45.5%], win rate 75.0% (6W/2T/0L over 8 trial(s)) — credibly better
✅ test-gap-analysis — detailsReason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
✅ test-smell-detection — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (4W/0T/0L over 4 trial(s)) — credibly better
✅ test-tagging — detailsReason: Mean preference +42.5% [95% CI 1.1%, 83.9%], win rate 50.0% (4W/4T/0L over 8 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Diagnosed from run 30217274869 artifacts (per-trial evidence plus the raw
_experiment trajectories), not from the summary table.
1. Untracked Cobertura fixture (review finding, real bug)
`.gitignore` line 168 (`coverage*.xml`) silently swallowed
tests/dotnet-test/crap-score/fixtures/refactor-required/coverage.cobertura.xml,
so `git add -A` never staged it. Three crap-score scenarios reference that file
and would have failed at setup in CI. My previous verification only checked the
working tree, which is why it passed locally.
Added a negation for `tests/**/fixtures/**/coverage*.{xml,json,info}` so eval
fixtures are never silently dropped again — the three pre-existing coverage
fixtures in the repo had to be force-added for the same reason. The verification
script now asserts every referenced fixture is *tracked by git*, not merely
present on disk.
2. migrate-static-to-wrapper — missed activation, self-inflicted
Both losing scenarios recorded skillActivationCount = 0: the skill never loaded,
so the "skilled" arm was effectively unassisted and lost on its own merits. The
skill content was right; discovery was broken.
Root cause is an internal contradiction. Step 3 documents the ambient-seam
pattern for static classes and states the seam "is the answer", yet
`When Not to Use` said "the code does not use dependency injection and the user
hasn't chosen ambient context" — which describes the static-helper scenario
exactly, so the model correctly followed the skill's own guidance and declined.
`DO NOT USE FOR` also excluded "creating or registering the wrapper when it does
not exist yet", which reads as excluding built-ins like TimeProvider.
Fixed the contradiction and added the missing triggers (static/utility class
needing an ambient seam; behavior-preserving refactors that must keep
DateTimeKind). Descriptions were then trimmed to stay inside the 1,024-char
per-skill and 15,000-char plugin menu budgets.
Worth noting the unassisted arm produced `GetLocalNow().DateTime` — the exact
DateTimeKind regression #906 fixed — and migrated out of scope. The scenario is
doing its job; it just never had the skill loaded.
3. code-testing-agent — missed activation + format-shaped rubrics
The workspace-integrity loss also had skillActivationCount = 0, so no completion
contract was produced and the judge scored it marginally below baseline. Made
the trigger explicit for "standard test-generation workflow" phrasing, sparse
workspaces, and extending an existing suite.
Overfitting was the highest in the plugin (0.35), driven by three rubric items
scored vocabulary/0.85 that demanded the literal `Requirement | Evidence` table:
"a developer who writes perfect tests but doesn't produce this exact table
format would fail". Reshaped all three to the underlying outcome — every
requirement individually traceable to a named test. The graders still enforce
the skill's stated contract, so the requirement is not weakened.
4. assertion-quality — dormancy guard measured nothing
The -40% loss was on a stimulus carrying `reject_skills: ["*"]`, which forces
the skilled arm skill-free. Treatment then equals control by construction, so
the head-to-head score is pure judge noise: across the four evals using this
pattern it landed on -0.4, +0.4, +0.4 and 0. Here it cost the pass.
The repo's own convention for dormancy guards is `expect_activation: false`
with no `reject_skills` (agent.test-quality-auditor, agent.test-migration,
system-text-json-net11). Aligned this guard with it, so the skill is loaded and
the property actually under test is that it stays dormant on an off-target
request. Left the three passing evals' guards alone to avoid churning results
this change does not need to touch.
Also wired up the orphaned `jest-shallow` fixture — shallow toBeDefined /
toBeTruthy assertions, an always-true, a self-comparison, an un-awaited async
assertion that silently passes, plus two legitimately good assertions for
calibration — which raises n from 5 to 6 and adds polyglot coverage.
Reshaped the technique-shaped rubric items flagged in assertion-quality and
migrate-static-to-wrapper so they score the result rather than whether the
answer used the skill's taxonomy words.
Verification: skill-validator check passes (20 skills, 10 agents, plugin menu
15,000-char budget respected); all 257 fixture references parse, exist and are
git-tracked; markdownlint clean.
Refs #899
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate |
github-actions
Bot
added
pr-state/ready-for-eval
Contributor
📊 Skill Evaluation Results10 skill(s) evaluated — 6 improved, 4 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
✅ assertion-quality — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
❌ code-testing-agent — detailsReason: Mean preference +13.3% [95% CI -101.4%, 128.1%], win rate 66.7% (2W/0T/1L over 3 trial(s)) — not credible (95% CI includes 0)
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
✅ crap-score — detailsReason: Mean preference +50.0% [95% CI 24.3%, 75.7%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
✅ detect-static-dependencies — detailsReason: Mean preference +50.0% [95% CI 24.3%, 75.7%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
❌ migrate-static-to-wrapper — detailsReason: Mean preference +36.0% [95% CI -14.9%, 86.9%], win rate 60.0% (3W/2T/0L over 5 trial(s)) — not credible (95% CI includes 0)
✅ test-anti-patterns — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (8W/0T/0L over 8 trial(s)) — credibly better
❌ test-gap-analysis — detailsReason: Mean preference +6.7% [95% CI -24.9%, 38.3%], win rate 33.3% (2W/3T/1L over 6 trial(s)) — not credible (95% CI includes 0)
✅ test-smell-detection — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (4W/0T/0L over 4 trial(s)) — credibly better
✅ test-tagging — detailsReason: Mean preference +45.0% [95% CI 13.1%, 76.9%], win rate 75.0% (6W/2T/0L over 8 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps
|
1. test-gap-analysis — regression this PR introduced Previous run: PASS, +33.3% [16.2, 50.5], 5W/1T/0L. Latest run: FAIL, +6.7%, 2W/3T/1L, with -40% on the "Decline request to write new tests from scratch" guard. Removing `reject_skills: ["*"]` from that guard (c09d9c6) was correct — it stopped comparing two identical skill-free arms — but it was only half the change. Once the skill is actually loaded, the judge scores the stimulus against its rubric, and this rubric said only: - Wrote test methods for the ShoppingCart class - Covered the AddItem and GetTotal methods Neither item mentions the property the guard exists to measure, so the judge fell back to comparing volume: "Response A produced 12 tests ... Response B produced 10 active tests ... A edges ahead with more comprehensive test coverage." Raw test count decides an off-target-request guard. assertion-quality's equivalent guard got the matching rubric item in the same commit ("Did not derail into an assertion-quality audit of code the user never asked about") and scored +40%, 6W/0T/0L. The rubric half was simply not applied to the other three evals. Added the anti-hijack criterion and an explicit instruction not to reward raw test count to the test-gap-analysis, test-smell-detection and test-tagging guards, so all four now measure the same property. (The three win->tie collapses in the same skill are all logged "Position-swap inconsistent" — judge variance at one trial per scenario, not a content change.) 2. code-testing-agent — "Extend an existing suite to the untested method only" Lost -40% on: "Both responses successfully completed the task with passing tests covering all required areas. Response A achieved slightly more coverage (12 vs 11 new tests)." Both satisfied every requirement; the judge broke the tie on one extra test. This eval already guards against exactly that, on the other two scenarios: Vitest: "Avoided redundant tests ... a smaller focused suite is not penalized merely for having fewer tests than another passing suite" workspace: "evaluates suite quality by meaningful, nonredundant cases rather than rewarding a higher raw test count" The scenario I added in 39b406a did not inherit that convention. Added it. 3. code-testing-agent — plugin-arm activation was 2/3 The missing activation is this same scenario (isolated True, plugin False), and it is correct routing rather than a bug: the fixture was MSTest, and this skill's own description says "DO NOT USE FOR: MSTest-specific test authoring or modernization (use writing-mstest-tests)". In the plugin arm the sibling legitimately won the request. The scenario is about extending coverage to an untested method with mocked seams, not about MSTest authoring, so the fixture is the thing that was wrong. Converted it to xUnit v3 (with TestingPlatformDotnetTestSupport, matching the repo's Microsoft.Testing.Platform requirement), which removes the overlap and adds framework diversity — the three scenarios are now xUnit, pytest and Vitest. Graders are unchanged: they key on the csproj path and the retained test-method name, both of which survive the conversion. Verification: the converted fixture restores, builds and passes its 4 seed tests under `dotnet test` in a clean workspace outside the repo; skill-validator check passes (20 skills, 10 agents); 257/257 fixture references parse, exist and are git-tracked; markdownlint clean. The verification script now also asserts that every `expect_activation: false` guard carries no `reject_skills` and does have an anti-hijack rubric item. Refs #899 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate |
Comment on lines
+168
to
+186
| environment: | ||
| files: | ||
| - src: fixtures/csharp-shipping-quotes | ||
| dest: fixtures/csharp-shipping-quotes | ||
| graders: | ||
| - type: run-command | ||
| config: | ||
| command: sh -c "dotnet test fixtures/csharp-shipping-quotes/tests/ShippingQuotes.Tests.csproj" | ||
| expected_exit_code: 0 | ||
| timeout: 10m | ||
| - type: run-command | ||
| config: | ||
| command: sh -c "grep -q 'BillableWeight_UnderOneKilo_BillsTheOneKiloMinimum' fixtures/csharp-shipping-quotes/tests/BillableWeightTests.cs" | ||
| expected_exit_code: 0 | ||
| timeout: 1m | ||
| - type: output-matches | ||
| config: | ||
| pattern: \|\s*Requirement\s*\|\s*Evidence\s*\| | ||
| - type: prompt |
Member
Author
There was a problem hiding this comment.
Good catch — that guard was too weak for a scenario whose whole purpose is scope containment. Fixed in 5ad3743.
The single grep is replaced by a byte-for-byte assertion:
- setup snapshots the suite to
.eval-baseline/before the run - a grader
diff -us the two, so any edit fails - a second grader requires some other
.csfile undertests/to referenceQuoteAsync, so the new coverage has to land in a new file — and has to exist
I replayed the graders over the fixture with each cheat applied rather than assuming the discrimination:
| case | new graders | old grep |
|---|---|---|
| correct: new file, baseline untouched | PASS | PASS |
| cheat: appended tests into baseline file | FAIL | PASS |
| cheat: rewrote baseline, kept grepped symbol | FAIL | PASS |
cheat: no QuoteAsync tests added at all |
FAIL | PASS |
Two robustness details came out of writing it:
- The snapshot lives in the workspace, not
/tmp. No existing eval relies on/tmpsurviving fromenvironment.commandsinto graders, and if those ran in different containers the diff would fail for every run and silently break the scenario..eval-baseline/sits outside the test project so it is never compiled. --include='*.cs'is quoted so the shell can't expand the glob beforegrepsees it.
The prompt now states the requirement explicitly ("put them in a new test file and leave BillableWeightTests.cs exactly as it is"), so the tightened grader checks a stated requirement rather than acting as a trap.
Member
Author
|
/evaluate |
github-actions
Bot
added
waiting-on-author
Contributor
📊 Skill Evaluation Results10 skill(s) evaluated — 6 improved, 4 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
❌ assertion-quality — detailsReason: Mean preference +20.0% [95% CI -15.1%, 55.1%], win rate 66.7% (4W/1T/1L over 6 trial(s)) — not credible (95% CI includes 0)
❌ code-testing-agent — detailsReason: Mean preference -13.3% [95% CI -128.1%, 101.4%], win rate 33.3% (1W/0T/2L over 3 trial(s)) — no improvement
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
✅ crap-score — detailsReason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
✅ detect-static-dependencies — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
✅ migrate-static-to-wrapper — detailsReason: Mean preference +52.0% [95% CI 18.7%, 85.3%], win rate 100.0% (5W/0T/0L over 5 trial(s)) — credibly better
✅ test-anti-patterns — detailsReason: Mean preference +25.0% [95% CI 7.7%, 42.3%], win rate 62.5% (5W/3T/0L over 8 trial(s)) — credibly better
✅ test-gap-analysis — detailsReason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
❌ test-smell-detection — detailsReason: Mean preference +30.0% [95% CI -1.8%, 61.8%], win rate 75.0% (3W/1T/0L over 4 trial(s)) — not credible (95% CI includes 0)
✅ test-tagging — detailsReason: Mean preference +60.0% [95% CI 22.1%, 97.9%], win rate 75.0% (6W/2T/0L over 8 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps
|
Contributor
📊 Skill Evaluation Results10 skill(s) evaluated — 7 improved, 3 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
✅ assertion-quality — detailsReason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
❌ code-testing-agent — detailsReason: Mean preference +13.3% [95% CI -44.0%, 70.7%], win rate 33.3% (1W/2T/0L over 3 trial(s)) — not credible (95% CI includes 0)
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
✅ crap-score — detailsReason: Mean preference +43.3% [95% CI 9.7%, 77.0%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
✅ detect-static-dependencies — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
✅ migrate-static-to-wrapper — detailsReason: Mean preference +52.0% [95% CI 18.7%, 85.3%], win rate 100.0% (5W/0T/0L over 5 trial(s)) — credibly better
✅ test-anti-patterns — detailsReason: Mean preference +20.0% [95% CI 2.1%, 37.9%], win rate 50.0% (4W/4T/0L over 8 trial(s)) — credibly better
✅ test-gap-analysis — detailsReason: Mean preference +26.7% [95% CI 5.0%, 48.3%], win rate 66.7% (4W/2T/0L over 6 trial(s)) — credibly better
✅ test-smell-detection — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (4W/0T/0L over 4 trial(s)) — credibly better
❌ test-tagging — detailsReason: Mean preference +35.0% [95% CI -25.5%, 95.5%], win rate 75.0% (6W/0T/2L over 8 trial(s)) — not credible (95% CI includes 0)
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Review finding on the "Extend an existing suite to the untested method only"
scenario: the guard meant to enforce "leave BillableWeightTests.cs untouched"
only grepped for a single method name, so a run could rewrite the file, or
duplicate the BillableWeight coverage into it, and still pass — in a scenario
whose entire purpose is scope containment.
Replaced it with a byte-for-byte assertion:
- setup copies the suite to `.eval-baseline/` before the run
- a grader diffs the two, so any edit at all fails
- a second grader requires some *other* .cs file under tests/ to reference
QuoteAsync, so the new coverage has to land in a new file and has to exist
Verified the discrimination rather than assuming it, by replaying the graders
over the fixture with each cheat applied:
correct: new file, baseline untouched new=PASS old=PASS
cheat: appended tests into baseline file new=FAIL old=PASS
cheat: rewrote baseline, kept grepped symbol new=FAIL old=PASS
cheat: no QuoteAsync tests added at all new=FAIL old=PASS
Two robustness details while writing it:
- The snapshot lives in the workspace, not /tmp. No existing eval relies on
/tmp surviving from `environment.commands` into graders, and if the two ran
in different containers the diff would fail for every run and silently break
the scenario. `.eval-baseline/` sits outside the test project, so it is never
compiled.
- `--include='*.cs'` is quoted so the shell cannot expand the glob before grep
sees it.
The prompt now states the requirement explicitly ("put them in a new test file
and leave BillableWeightTests.cs exactly as it is"), so the tightened grader
checks a stated requirement rather than acting as a trap.
Refs #899
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate |
Contributor
📊 Skill Evaluation Results10 skill(s) evaluated — 6 improved, 4 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0);
ℹ️ Column legend
✅ assertion-quality — detailsReason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
❌ code-testing-agent — detailsReason: Mean preference -13.3% [95% CI -128.1%, 101.4%], win rate 33.3% (1W/0T/2L over 3 trial(s)) — no improvement
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
✅ crap-score — detailsReason: Mean preference +43.3% [95% CI 9.7%, 77.0%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
✅ detect-static-dependencies — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (6W/0T/0L over 6 trial(s)) — credibly better
❌ migrate-static-to-wrapper — detailsReason: Mean preference +44.0% [95% CI -0.4%, 88.4%], win rate 80.0% (4W/1T/0L over 5 trial(s)) — not credible (95% CI includes 0)
✅ test-anti-patterns — detailsReason: Mean preference +30.0% [95% CI 14.5%, 45.5%], win rate 75.0% (6W/2T/0L over 8 trial(s)) — credibly better
✅ test-gap-analysis — detailsReason: Mean preference +26.7% [95% CI 5.0%, 48.3%], win rate 66.7% (4W/2T/0L over 6 trial(s)) — credibly better
❌ test-smell-detection — detailsReason: Mean preference +20.0% [95% CI -16.7%, 56.7%], win rate 50.0% (2W/2T/0L over 4 trial(s)) — not credible (95% CI includes 0)
✅ test-tagging — detailsReason: Mean preference +47.5% [95% CI 8.6%, 86.4%], win rate 62.5% (5W/3T/0L over 8 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
A repo-wide audit of all 97 eval specs, rather than another round of scenario-by-scenario patching. ## Audit findings (measured) | Finding | Scale | | --- | --- | | Skills with no eval at all | 5 | | Evals at n=1 (one judge call decides pass/fail) | 18 | | Evals at n=2 | 10 | | Evals at n=3 | 21 | | **Underpowered (n<=3) of 93 skill evals** | **49 (53%)** | | Orphaned fixtures (committed, never referenced) | 12 | | Cobertura methods whose line-rate contradicts own <lines> | 6 | `dotnet-skills.experiment.yaml` sets `runs: 1`, so n is the scenario count and a single judge call decides each scenario. That is the root cause behind results like `coverage-analysis` winning 100% of its trials in four consecutive runs and failing all four. ## 1. eng/eval-quality — a CI gate for defect classes that cost real results Every failing check corresponds to a bug that has already cost an evaluation on this repo, and every one was invisible to the existing checks: the specs parsed, skill-validator passed, and the damage surfaced only as a skill losing to its own baseline. Fails on (all structural — file existence, git state, YAML keys, so they cannot fire spuriously on prose): - a referenced fixture missing on disk - a referenced fixture not tracked by git (`.gitignore` silently swallowed a Cobertura fixture; it passed locally and would have failed at setup in CI) - a Cobertura fixture whose declared line-rate contradicts its own <lines> - a dormancy guard that also sets `reject_skills` (makes the skilled arm identical to the baseline arm, so the score is judge noise) Reports without failing: statistical power, orphaned fixtures, uncovered skills, and dormancy guards that appear to lack an anti-hijack rubric item. That last one needs phrase matching over free text and will always have false positives — it flagged a well-formed guard in `system-text-json-net11` whose rubric says "Does NOT load or reference the skill" — so it warns rather than blocks. A gate that fails spuriously is a gate the team switches off. Failing on power would also break 28 existing evals across 8 plugins, which is a maintainer decision. `selftest_eval_quality.py` injects each defect into a scratch tree and asserts the gate rejects it, then asserts a clean tree passes. CI runs the self-test before the gate, so the gate cannot silently stop working. ## 2. Fixed #950 — 6 inconsistent Cobertura methods `crap-score/partial-coverage` (ProcessOrder, GetOrderStatus, CancelOrder) and `coverage-analysis/partial-coverage` (Enroll, CalculateGpa, Search). The declared rate is the intent — the rubrics are written against it — so the <lines> data was corrected to match, not the reverse, and class/package totals were rebuilt. Verified that the risk ordering each rubric asserts still holds: ProcessOrder remains the top hotspot at CRAP 26.6, CalculateGpa at 52.2. ## 3. Wired up four orphaned fixtures (12 -> 8) Purpose-built discriminators that were committed but never referenced: - `test-smell-detection` n=4 -> 6: `junit-smells` (all 10 catalogued smells in JUnit, plus a well-written test for contrast) and `skip-and-magic` (a documented skip vs a bare one, and contextually obvious counts that must NOT be flagged as magic numbers). This skill last failed at 2W/2T/0L — no losses, purely n. - `test-tagging` n=8 -> 9: `go-report-only`. Go has no attribute-based trait mechanism, so the scenario checks the skill classifies and reports instead of inventing a [TestCategory] equivalent that does not compile. - `find-untested-sources` n=3 -> 4: `pairing-repo`, a src/ + tests/ split where OrderProcessor is unpaired and CustomerService is already covered. ## Verification - eval quality gate: 0 errors across 97 specs; self-test 6/6 - `actionlint` clean on the new workflow - skill-validator: 20 skills, 10 agents - Cobertura fixtures internally consistent and rubric intent preserved Refs #899. Closes #950. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate |
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
eng/eval-quality/check_eval_quality.py:164
open(...).read()should use a context manager (with open(...) as f:) to ensure the handle is closed promptly, especially when scanning many eval specs.
fx = os.path.join(os.path.dirname(spec), "fixtures")
if not os.path.isdir(fx):
continue
raw = open(spec, encoding="utf-8").read()
found += [f"{spec}: fixture '{n}' is committed but no stimulus references it"
eng/eval-quality/check_eval_quality.py:201
- Like the earlier helper, this YAML load uses
open(...)without a context manager. Usingwith open(...)avoids leaving file descriptors open while iterating all eval specs.
try:
doc = yaml.safe_load(open(spec, encoding="utf-8")) or {}
except yaml.YAMLError as exc:
- Files reviewed: 32/33 changed files
- Comments generated: 3
Comment on lines
+295
to
+311
| environment: | ||
| files: | ||
| - src: fixtures/go-report-only/calculator/go.mod | ||
| dest: calculator/go.mod | ||
| - src: fixtures/go-report-only/calculator/calculator.go | ||
| dest: calculator/calculator.go | ||
| - src: fixtures/go-report-only/calculator/calculator_test.go | ||
| dest: calculator/calculator_test.go | ||
| graders: | ||
| - type: output-matches | ||
| config: | ||
| pattern: (TestDivide_ByZero_ReturnsError|by.?zero|error path|negative) | ||
| - type: run-command | ||
| config: | ||
| command: sh -c "git diff --quiet -- calculator/calculator_test.go || test ! -d .git" | ||
| expected_exit_code: 0 | ||
| timeout: 1m |
Comment on lines
+21
to
+23
| paths: | ||
| - "tests/**" | ||
| - "eng/eval-quality/**" |
Comment on lines
+141
to
+143
| for spec in specs: | ||
| doc = yaml.safe_load(open(spec, encoding="utf-8")) or {} | ||
| n = len(doc.get("stimuli") or []) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Fixes the findings from the refreshed
dotnet-testcross-family evaluation posted to #899 (run 30108473397 plus the three recovery runs). Every change is driven by the judge evidence from the losing trials, not by style preference — I pulled the per-trial evidence out of the artifacts and fixed the specific behaviour the judges penalised.1. Dormancy guards were measuring nothing
assertion-quality,test-gap-analysis,test-smell-detectionandtest-taggingeach have a decline stimulus carryingconstraints.reject_skills: ["*"], which forces the skilled arm skill-free. Treatment then equals control by construction, so the head-to-head score is pure judge noise — across the four evals it landed on −0.4, +0.4, +0.4 and 0, and twice cost a skill its pass.Fixed in two halves:
reject_skills, matching the repo's own convention (agent.test-quality-auditor,agent.test-migration,system-text-json-net11all useexpect_activation: falsealone). The skill is now actually loaded, so the guard measures the real property.Applying only the first half is what briefly regressed
test-gap-analysis; with both halves it is green again.2. Skill fixes, one per failure mode
test-gap-analysistest-anti-patternsdetect-static-dependenciesfile:linetest-smell-detectionThread.Sleep(3000)away as an "integration style issue"crap-scorecoverage-analysismigrate-static-to-wrappercode-testing-agent3. Two activation bugs, found via
skillActivationCount = 0migrate-static-to-wrappercontradicted itself. Step 3 documents the ambient-seam pattern for static classes and says the seam "is the answer", but When Not to Use said "the code does not use dependency injection and the user hasn't chosen ambient context" — describing the failing scenario exactly. Both losing scenarios therefore ran unassisted, and one reproduced the veryGetLocalNow().DateTimeDateTimeKindregression Fix migrate-static-to-wrapper DateTimeKind regression (#899) #906 fixed.code-testing-agentdid not fire on "standard test-generation workflow" phrasing for a sparse workspace. Its extend-suite fixture was also MSTest, so in the plugin arm the siblingwriting-mstest-testscorrectly won the routing (this skill's own description disclaims MSTest authoring). Converted that fixture to xUnit v3, which removed the overlap and gives the three scenarios framework diversity (xUnit / pytest / Vitest).4. A fixture that was never committed, and one that lied
.gitignore(coverage*.xml) silently swallowedcrap-score/fixtures/refactor-required/coverage.cobertura.xml, so three scenarios would have failed at setup in CI. Added a negation fortests/**/fixtures/**/coverage*and hardened verification to assert fixtures are git-tracked, not merely present on disk.line-rate="0.55"while its<lines>implied 0.80. The skill documents both parse paths, so the arms could read different inputs. Rebuilt it consistent. The new invariant check found 6 further inconsistent methods in two pre-existing fixtures — filed as Two Cobertura eval fixtures declare a line-rate their own line data contradicts #950 rather than bundled here, since their rubrics quote the declared figures.Checks
skill-validator check --plugin ./plugins/dotnet-test— ✅ 20 skills, 10 agents; plugin skill-menu inside the 15,000-char budgetmarkdownlint-cli2— 0 errorsreject_skillsand carrying an anti-hijack rubric itemEvaluation (run 30268608409) — 6 improved / 4 not credible
Starting point for this PR was 5 improved / 5 (comment).
test-taggingcrap-scoredetect-static-dependenciesassertion-qualitytest-anti-patternstest-gap-analysiscoverage-analysismigrate-static-to-wrappertest-smell-detectioncode-testing-agentKnown limitation: the gate is unstable at one trial per scenario
Three of the four remaining ❌ have zero losses. The gate is
mean > 0 ∧ ci_low > 0, i.e.sqrt(n) × (mean/sd) > t(n-1), which demands a mean/sd ratio of 2.48 at n=3:migrate-static-to-wrapperis 4W/1T/0L and missed by 0.4 of a percentage point (ci_low = −0.4%). It passed at +52.0% the previous run with the same eval.coverage-analysishas now won 100% of its trials in four consecutive runs (+40%, +60%, +60%, +60%) and failed all four.test-smell-detectionis 2W/2T/0L — no losses.None of these is a content regression and none has been chased by weakening or deleting scenarios. The durable fix is
runs=3, which turns 3 scenarios into 9 trials and drops the required ratio to 0.77. Run-to-run churn among untouched evals is large:test-taggingwent 6W/0T/2L → 5W/3T/0L andtest-smell-detection4W/0T/0L → 2W/2T/0L with no eval change between runs.Open:
code-testing-agentjudges on test volumeThis is the one remaining case with real losses, and it is a consistent, diagnosable pattern rather than variance. Across three runs the judge broke near-ties on raw test count:
Both affected scenarios already carry an explicit rubric instruction not to reward a higher raw test count, and the skilled arm scores 4.3–4.9/5 quality against a 3.8–4.0 baseline — it produces focused, correct suites that the judge then ranks below larger ones. This is a rubric-weighting problem rather than a skill defect, and it needs its own change (the scenarios are expensive, so
runs=3would also help here).Refs #899. Follow-up filed as #950.