Fix dotnet-test findings from the refreshed cross-family eval (#899) - #945
Merged
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
Evangelink
added a commit
that referenced
this pull request
Jul 28, 2026
Split out of #945, where every defect class below was first found the hard way: as a skill mysteriously losing to its own baseline, with the eval specs parsing cleanly and skill-validator passing throughout. ## Why An audit of all 97 eval specs found: | 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 | None of this was visible to any existing check. ## What fails the build All four are structural — they inspect file existence, git state, or YAML keys, never prose — so they cannot fire spuriously on a well-written eval: 1. A referenced fixture missing on disk. 2. A referenced fixture not tracked by git. `.gitignore` carries `coverage*.xml` (correct for Coverlet output) and silently swallowed a committed Cobertura *fixture*: `git add -A` reported success, the eval passed locally, and three scenarios would have failed at setup in CI. Only the git index catches this. 3. A Cobertura fixture whose declared `line-rate` contradicts its own `<lines>`. The crap-score skill documents both parse paths, so the two arms of a comparison can read different coverage inputs and the eval measures the disagreement instead of the skill. Observed live: a scenario lost -40% with the judge writing "Response B made a critical error by manually counting line hits (12/15 = 80%) instead of using the XML's recorded line-rate of 0.55" — the fixture was wrong, not the response. 4. A dormancy guard (`expect_activation: false`) that also sets `reject_skills`. That forces the skilled arm skill-free, making it identical to the baseline arm, so the score is judge noise. The same guard scored -0.4, +0.4, +0.4 and 0 across four evals and twice cost a skill its pass. ## What only warns Statistical power, orphaned fixtures, uncovered skills, and dormancy guards that appear to lack an anti-hijack rubric item. Two deliberate calls here: - The anti-hijack check began as an error and immediately false-positived on a well-formed guard in `system-text-json-net11` whose rubric reads "Does NOT load or reference the system-text-json-net11 skill". Detecting that property needs phrase matching over free text and will always have false positives, so it warns. A gate that blocks a PR spuriously is a gate the team switches off. - Failing on power would break 28 existing evals across 8 plugins, and the fix (raising `runs`) is a CI cost decision. See #952. ## Self-test `selftest_eval_quality.py` injects each defect into a scratch tree, asserts the gate rejects it, and asserts a clean tree passes. CI runs the self-test *before* the gate, so the gate cannot silently stop working. Refs #899, #952. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Evangelink
added a commit
that referenced
this pull request
Jul 28, 2026
A grader whose config is absent, null, or missing its required key parses as valid YAML and enforces nothing, so the scenario appears to carry one more assertion than it really does. The usual cause is an indentation slip during an edit, which leaves `- type: output-matches` / `config:` with the pattern attached to the next list item. This is not hypothetical: it shipped in 705b876 on #945 and was caught only by review. It survived YAML validation (the document parses) and a bespoke regex validator, which did `(g.get("config") or {}).get( "pattern")` and silently skipped the broken entry - so the pattern count was 262 both before and after the fix. Verified end to end: restoring the broken eval.yaml from 705b876 into the working tree makes the gate exit 1 with 'Detect time-related statics and recommend TimeProvider' grader[3] (output-matches) has no config; it silently enforces nothing. Check the indentation of the 'pattern:' line. and the fixed tree is clean. The self-test gains a matching case (now 8/8) that injects the same indentation slip. Swept the repo while adding this: 1,906 graders across 97 specs, no other instance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Member
Author
|
/evaluate 9b2b3bb |
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
tests/dotnet-test/migrate-static-to-wrapper/eval.yaml:131
- The new "Migrate a static helper class" stimulus graders only assert that RetentionPolicy.cs contains the words "TimeProvider" and "static". That can still pass even if the migration leaves
DateTime.UtcNowin place, doesn’t default the seam toTimeProvider.System, or changes method signatures (all of which the rubric says must not happen). Strengthen the graders to enforce the key invariants directly from the file content.
graders:
- type: file-contains
config:
path: "**/RetentionPolicy.cs"
value: TimeProvider
tests/dotnet-test/migrate-static-to-wrapper/fixtures/static-helper/Utilities/RetentionPolicy.cs:6
- This fixture’s XML doc comment claims there are "~40 call sites" in the codebase, but this fixture directory currently contains only RetentionPolicy.cs and its .csproj (no callers). That makes the comment misleading for anyone reading the fixture in isolation; consider describing the fixture’s purpose/invariants instead of referencing non-existent files.
/// <summary>
/// Called from ~40 call sites across the codebase as <c>RetentionPolicy.IsExpired(...)</c>.
/// Callers must keep working unchanged.
/// </summary>
- Files reviewed: 29/30 changed files
- Comments generated: 0 new
github-actions
Bot
added
pr-state/ready-for-eval
Member
Author
|
/evaluate 9b2b3bb |
Member
Author
|
/evaluate 9b2b3bb |
Evangelink
had a problem deploying
to
copilot-pat-pool
July 28, 2026 16:10 — with 
GitHub Actions
Error
GitHub Actions
Error
* Add a CI quality gate for evaluation specs and fixtures Split out of #945, where every defect class below was first found the hard way: as a skill mysteriously losing to its own baseline, with the eval specs parsing cleanly and skill-validator passing throughout. ## Why An audit of all 97 eval specs found: | 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 | None of this was visible to any existing check. ## What fails the build All four are structural — they inspect file existence, git state, or YAML keys, never prose — so they cannot fire spuriously on a well-written eval: 1. A referenced fixture missing on disk. 2. A referenced fixture not tracked by git. `.gitignore` carries `coverage*.xml` (correct for Coverlet output) and silently swallowed a committed Cobertura *fixture*: `git add -A` reported success, the eval passed locally, and three scenarios would have failed at setup in CI. Only the git index catches this. 3. A Cobertura fixture whose declared `line-rate` contradicts its own `<lines>`. The crap-score skill documents both parse paths, so the two arms of a comparison can read different coverage inputs and the eval measures the disagreement instead of the skill. Observed live: a scenario lost -40% with the judge writing "Response B made a critical error by manually counting line hits (12/15 = 80%) instead of using the XML's recorded line-rate of 0.55" — the fixture was wrong, not the response. 4. A dormancy guard (`expect_activation: false`) that also sets `reject_skills`. That forces the skilled arm skill-free, making it identical to the baseline arm, so the score is judge noise. The same guard scored -0.4, +0.4, +0.4 and 0 across four evals and twice cost a skill its pass. ## What only warns Statistical power, orphaned fixtures, uncovered skills, and dormancy guards that appear to lack an anti-hijack rubric item. Two deliberate calls here: - The anti-hijack check began as an error and immediately false-positived on a well-formed guard in `system-text-json-net11` whose rubric reads "Does NOT load or reference the system-text-json-net11 skill". Detecting that property needs phrase matching over free text and will always have false positives, so it warns. A gate that blocks a PR spuriously is a gate the team switches off. - Failing on power would break 28 existing evals across 8 plugins, and the fix (raising `runs`) is a CI cost decision. See #952. ## Self-test `selftest_eval_quality.py` injects each defect into a scratch tree, asserts the gate rejects it, and asserts a clean tree passes. CI runs the self-test *before* the gate, so the gate cannot silently stop working. Refs #899, #952. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa * Address review: index-only tracking check, file-level Cobertura totals git_tracked_files() unioned `git ls-files` with `git diff --cached --name-only`. The second arm is redundant for additions (ls-files already reports the index) and wrong for deletions: a fixture staged for removal but left on disk appears in diff --cached and was counted back as "tracked", a false negative for the exact untracked-fixture class the check exists to catch. It was invisible because scratch() ran `git init; git add -A` with no commit, so there was no HEAD, diff --cached errored, and the arm was inert. scratch() now commits. Verified: with the commit in place and the two-arm implementation restored, the self-test fails on "fixture present but NOT tracked by git" (expected=FAIL got=PASS), so the regression is genuinely covered rather than accidentally passing. Added a fifth failing check for whole-file Cobertura totals. Review found coverage-analysis/partial-coverage declaring line-rate="0.47" alongside lines-covered="35" lines-valid="60" (58.3%); every method was internally consistent, so the method-level check could not see it. Compares two declared values, so it cannot fire on well-formed input. Also reports, as a warning, file/package/class line-rate disagreeing with the <lines> beneath it. Warning rather than error because the only live instance (coverage-analysis/fixtures/plateau, declaring 75% against a 47% payload) cannot be recomputed: its prompt quotes 75% and its rubric requires CalculateGpa to remain the 0% blocker, which caps the achievable rate at 48.9%. Failing on it would compel rewriting a passing scenario. Extended the push trigger to match the pull_request paths. The gate reads plugins/* and .gitignore, so a merge touching only those could previously land on main without it running. Replaced the three bare open() calls with context managers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa * Add a check for graders that silently enforce nothing A grader whose config is absent, null, or missing its required key parses as valid YAML and enforces nothing, so the scenario appears to carry one more assertion than it really does. The usual cause is an indentation slip during an edit, which leaves `- type: output-matches` / `config:` with the pattern attached to the next list item. This is not hypothetical: it shipped in 705b876 on #945 and was caught only by review. It survived YAML validation (the document parses) and a bespoke regex validator, which did `(g.get("config") or {}).get( "pattern")` and silently skipped the broken entry - so the pattern count was 262 both before and after the fix. Verified end to end: restoring the broken eval.yaml from 705b876 into the working tree makes the gate exit 1 with 'Detect time-related statics and recommend TimeProvider' grader[3] (output-matches) has no config; it silently enforces nothing. Check the indentation of the 'pattern:' line. and the fixed tree is clean. The self-test gains a matching case (now 8/8) that injects the same indentation slip. Swept the repo while adding this: 1,906 graders across 97 specs, no other instance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
eng/eval-quality/selftest_eval_quality.py:90
- This
git rm --cachedcall ignores the return code. If it fails (path typo, git not installed, etc.), the “untracked fixture” case may no longer be exercising the intended bug class and could yield a false sense of coverage. The self-test should enforce success explicitly.
def untracked_fixture(d):
# Present on disk but excluded from git — the .gitignore class of bug.
with open(os.path.join(d, ".gitignore"), "w") as f:
f.write("Thing.cs\n")
subprocess.run(["git", "rm", "--cached", "-q",
"tests/demo/widget/fixtures/sample/Thing.cs"], cwd=d, capture_output=True)
eng/eval-quality/selftest_eval_quality.py:59
- In the self-test harness,
git add -Ais run withoutcheck=True, so a failing git command (e.g., bad PATH, permissions, or an unexpected repo state) can be silently ignored and the test may report misleading PASS/FAIL results. Since this script’s purpose is to prove the gate fires reliably, it should fail fast if staging fails.
This issue also appears on line 84 of the same file.
mutate(d)
subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True)
code, out = run_gate(d)
- Files reviewed: 33/34 changed files
- Comments generated: 0 new
Evangelink
temporarily deployed
to
copilot-pat-pool
July 29, 2026 08:06 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
📊 Skill Evaluation Results11 skill(s) evaluated — 5 improved, 5 no credible improvement, 1 inconclusive. 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 +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
✅ 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
❌ find-untested-sources — 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)
❌ 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
|
| Scenario | Mean preference | Trials (W/T/L) |
|---|---|---|
| = Acknowledge well-tested code with few surviving mutations | +0.0% | 0/0/0 |
| ▼ Analyse error propagation gaps in a Rust crate | -40.0% | 0/0/1 |
| = Decline request to write new tests from scratch | +0.0% | 0/1/0 |
| = Find boundary mutation gaps in tiered discount and shipping logic | +0.0% | 0/1/0 |
| ▲ Find logic and null-check mutation gaps in access control code | +40.0% | 1/0/0 |
| ▲ Skip trivial and generated code while tracing private call chains | +40.0% | 1/0/0 |
✅ test-smell-detection — details
Reason: Mean preference +33.3% [95% CI 16.2%, 50.5%], win rate 83.3% (5W/1T/0L over 6 trial(s)) — credibly better
| Scenario | Mean preference | Trials (W/T/L) |
|---|---|---|
| ▲ Audit a JUnit suite using Java-specific smell markers | +40.0% | 1/0/0 |
| = Calibrate skips and contextually obvious numbers | +0.0% | 0/1/0 |
| ▲ Decline request to write new tests from scratch | +40.0% | 1/0/0 |
| ▲ Detect multiple test smells in order processing test suite | +40.0% | 1/0/0 |
| ▲ Recognize integration tests and avoid false positives for external resources | +40.0% | 1/0/0 |
| ▲ Recognize well-written tests with no significant smells | +40.0% | 1/0/0 |
❌ test-tagging — details
Reason: Mean preference +26.7% [95% CI -13.3%, 66.6%], win rate 55.6% (5W/2T/2L over 9 trial(s)) — not credible (95% CI includes 0)
| Scenario | Mean preference | Trials (W/T/L) |
|---|---|---|
| ▲ Accurately classify NUnit tests with misleading method names | +40.0% | 1/0/0 |
| ▼ Audit test distribution without modifying files | -40.0% | 0/0/1 |
| = Classify Go tests where no trait attribute mechanism exists | +0.0% | 0/1/0 |
| ▼ Decline request to write new tests | -40.0% | 0/0/1 |
| ▲ Tag MSTest tests and verify the project still builds | +100.0% | 1/0/0 |
| = Tag a partially-tagged MSTest suite without duplicating existing traits | +0.0% | 0/1/0 |
| ▲ Tag an untagged MSTest test suite | +100.0% | 1/0/0 |
| ▲ Tag an untagged NUnit test suite | +40.0% | 1/0/0 |
| ▲ Tag an untagged xUnit test suite | +40.0% | 1/0/0 |
🔍 Full Results - additional metrics and failure investigation steps
To investigate failures, paste this to your AI coding agent:
For PR 945 in dotnet/skills, download eval artifacts with
gh run download 30434166431 --repo dotnet/skills --pattern "vally-results-*" --dir ./eval-results, then fetch https://raw.githubusercontent.com/dotnet/skills/6ef2aca556932a918e2ffeff92b60d9f5a3abbd9/eng/vally-adapter/InvestigatingResults.md and follow it to analyze the results.json files. Diagnose each failure, suggest fixes to the eval.yaml and skill content, and tell me what to fix first.
▶ Sessions Visualisation -- interactive replay of all evaluation sessions
📊 Session Analytics (preview) -- aggregated metrics across evaluation sessions
github-actions
Bot
added
waiting-on-review
Contributor
|
✅ Evaluation passed for |
YuliiaKovalova
approved these changes
Jul 29, 2026
This was referenced Jul 30, 2026
Evangelink
added a commit
that referenced
this pull request
Jul 31, 2026
…aint Run 30611635547 reported 0 of 5 skills improved. The skills were not the problem: across 25 trials they scored 16W/8T/1L, every skill winning and none regressing. Four of the five could not have passed at any record. The sign test conditions on discordant (non-tie) trials, so at exactly 5 trials the only passing record is a clean 5W/0T/0L sweep. One tie drops the discordant count to 4, which is back below the floor. My previous commit raised all five evals to exactly 5 - the eligibility floor, which the README I wrote says is 'not a guarantee of adequate power'. At the 32% tie rate this run measured, a genuinely-helping skill parked there is certified about one run in ten; at 15 trials it is about nine in ten. - runs: 3 on the four evals whose scenarios discriminate (5 -> 15 trials). - generate-testability-wrappers had a real self-contradiction: When Not to Use disclaimed the no-DI case while Step 5 IS the answer for it, so the ambient-seam scenario never activated and lost unassisted. Same defect #945 fixed in migrate-static-to-wrapper; fixed the same way. - The already-abstracted scenario is a dormancy guard and now says so. - code-testing-agent is left at 5 trials on purpose. The only grader its baseline ever fails is the Requirement|Evidence table regex - every functional grader passes identically in both arms - so it is inert on outcomes and more runs would only re-measure a tie at the highest cost in the plugin. Removed the skill trigger phrase the overfitting judge flagged from the one scenario this PR added. - adapt.mjs said 'not credible (p=0.500)' for a 1W/4T/0L record, which describes a measured null rather than an unwinnable test. It now names the tie count and says no record could have passed. - The gate warns on evals sitting at 5-7 trials; 21 across the repo do. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 024b3241-d7af-418a-b9cd-3bb9ee9bf0ee
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, one that lied, and six more like it
.gitignorecarriescoverage*.xml(correct for Coverlet output) and silently swallowedcrap-score/fixtures/refactor-required/coverage.cobertura.xml.git add -Areported success, the eval passedlocally, and three scenarios would have failed at setup in CI. Added a negation for
tests/**/fixtures/**/coverage*.line-rate="0.55"while its<lines>implied 0.80. Thecrap-scoreskill documentsboth parse paths, so the two arms of a comparison can read different coverage inputs and the eval measures the
disagreement instead of the skill — a scenario lost −40% to exactly this. Rebuilt it consistent.
crap-score/partial-coverageandcoverage-analysis/partial-coverage(Two Cobertura eval fixtures declare a line-rate their own line data contradicts #950). Fixed here too. The declared rate is the intent — the rubrics arewritten against it — so the
<lines>data was corrected to match, not the reverse, and class/package totals wererebuilt. Verified the risk ordering each rubric asserts still holds:
ProcessOrderremains the top hotspot atCRAP 26.6,
CalculateGpaat 52.2.5. Wired up four orphaned fixtures
Purpose-built discriminators that were committed but that no stimulus referenced — the cheapest way to raise
nwithout new CI cost:
test-smell-detectionjunit-smells(all 10 catalogued smells in JUnit, plus a well-written test for contrast) andskip-and-magic(a documented skip vs a bare one, and self-documenting counts that must not be flagged as magic numbers)test-tagginggo-report-only— Go has no attribute-based trait mechanism, so the skill must classify and report rather than invent a[TestCategory]equivalent that does not compilefind-untested-sourcespairing-repo— asrc/+tests/split whereOrderProcessoris unpaired andCustomerServiceis already coveredChecks
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 30292432365) — 6 of 11
crap-scorecoverage-analysisfind-untested-sourcesdetect-static-dependenciestest-anti-patternstest-smell-detectionmigrate-static-to-wrappertest-taggingassertion-qualitytest-gap-analysiscode-testing-agentcoverage-analysispassed for the first time in six runs, andfind-untested-sourcespassed after its orphaned fixture raised n from 3 to 4. Two of the five remaining
failures have zero losses.
The gate, not the skills, is the binding constraint — now measured directly
Splitting the CI gate out of this PR produced an accidental A/A test: the only
difference between the two evaluated commits is four infrastructure files the harness
never reads.
Byte-identical inputs, and 3 of 11 verdicts flipped:
coverage-analysisfind-untested-sourcestest-taggingcoverage-analysisis the clearest case: it had failed five consecutive runs whilewinning 100% of its trials, then passed with the same 3W/0T/0L record. The judge
scored one trial +0.4 instead of +0.6,
sdcollapsed to 0, and the CI becamedegenerate. At n=3 the verdict turns on the magnitude the judge assigns, not on
whether the skill won.
Consequence for review: verdict changes across runs should not be attributed to
code changes without checking the diff first. Full analysis and the proposal
(per-eval
runsoverrides rather than a blanketruns: 3, which would push the~75-minute job past the 360-minute cap) is in #952.
Nothing here has been chased by weakening or deleting scenarios.
The repo-wide eval quality gate that came out of this work was split into #953 at
review request: it gates every plugin and warns about fixtures and skills in plugins
this PR does not own, so it carries a different blast radius and reviewer set.
Refs #899. Closes #950. Power analysis: #952. Quality gate: #953.