Move eight P0/P1 skills to staging - #972
Conversation
Skill Coverage Report
|
AbhitejJohn
left a comment
There was a problem hiding this comment.
MMR verdict: 🟡 Request changes
The mechanical removal and config cleanup are correct and well-executed. The blocker is that 17 retained files still carry 31 references to the eight removed skills, including one dead markdown link and a retained agent eval that mounts a deleted skill. None of this is disclosed in the PR body, and the "Reference scan: 0 errors" line doesn't actually cover it.
What's proven correct ✅
- Config cleanup is complete for the 4 modified files — 0 leftover matches for any of the 8 skill names.
- All 8 skills retained on
origin/staging—git cat-file -e origin/staging:<path>succeeded for all 8SKILL.md. - No new eval-quality errors — clean
origin/main= 4 errors, PR HEAD = 3; the one that drops ismaui-collectionview(removed here). The remaining 3 (maui-dependency-injection,maui-shell-navigation,maui-theming) are pre-existing onmainand out of scope. This PR reduces the count.
Findings ⚠️
- Retained agent eval mounts a deleted skill (highest severity).
tests/dotnet-test/agent.testability-migration/eval.yaml:20and:50list../../plugins/dotnet-test/skills/migrate-static-to-wrapperunderenvironment.skills, but that directory is deleted. Probable/latent setup failure — if the harness skips missing mounts, the scenario is still semantically invalid since it no longer loads the skill it claims to test. (This file isn't in the diff, so it can't be inline-anchored.) - Dead markdown link in a shipped agent —
plugins/dotnet-msbuild/agents/msbuild-code-review.agent.md:53links to the deletedmsbuild-antipatterns/SKILL.md. - Retained agents advertise removed skills —
msbuild.agent.md/build-perf.agent.mdlisteval-performance/msbuild-antipatterns/check-bin-obj-clashas available; the review agent citesmsbuild-antipatternsAP-13/14/21 by name. - Prose hand-offs dead-end — e.g.
maui-data-binding/SKILL.md:38→ "use themaui-collectionviewskill";migrate-dotnet8-to-dotnet9/SKILL.md:30→ "use themigrate-dotnet9-to-dotnet10skill". ~28 such references across the 17 files. - Misleading validation line — "Reference scan: 0 errors" is literally true but
ReferenceScanner.csonly scans external URLs; it never validates cross-skill/relative links, so it gives no assurance about findings 1–4.
Not blockers
- The 3 remaining eval-quality errors are pre-existing maui exemptions — optional opportunistic cleanup.
- Scope is otherwise tight: only the 8 skills' trees + their exact config entries.
Verified empirically on a worktree at PR HEAD; cross-checked with a second model family. Inline notes below.
(Copilot, commenting on Abhitej's behalf.)
| "dotnet-diag" = @("analyzing-dotnet-performance", "dotnet-trace-collect", "dump-collect", "microbenchmarking") | ||
| "dotnet-maui" = @("dotnet-maui-doctor") | ||
| "dotnet-msbuild" = @("binlog-failure-analysis", "build-parallelism", "build-perf-diagnostics", "incremental-build", "msbuild-antipatterns", "msbuild-modernization") | ||
| "dotnet-msbuild" = @("binlog-failure-analysis", "build-parallelism", "build-perf-diagnostics", "incremental-build", "msbuild-modernization") |
There was a problem hiding this comment.
Thanks for pulling all of this together, folks — the config cleanup itself is clean (I checked all four modified files and there are no leftover entries for the eight skills).
One gap though: we drop msbuild-antipatterns (and eval-performance / check-bin-obj-clash) here, but several retained msbuild artifacts still point at them. A few concrete ones:
plugins/dotnet-msbuild/agents/msbuild-code-review.agent.md:53has a hard markdown link[msbuild-antipatterns skill](../skills/msbuild-antipatterns/SKILL.md)— that target is deleted, so it's now a dead link.msbuild.agent.mdandbuild-perf.agent.mdstill advertiseeval-performance/msbuild-antipatterns/check-bin-obj-clashin their "available skills" lists, and the review agent leans onmsbuild-antipatternsAP-13/AP-14/AP-21 by name.
Since these agents ship on main, they'll route the model to skills that no longer exist. The PR's "Reference scan: 0 errors" line only covers external URLs (that's all ReferenceScanner.cs checks), so it wouldn't catch any of this. Could we scrub or soften the inbound references in the same change? Happy to send a small pass against the branch if that helps.
(Copilot, commenting on Abhitej's behalf.)
| tests/dotnet-maui/maui-safe-area/eval.yaml | ||
| tests/dotnet-maui/maui-shell-navigation/eval.yaml | ||
| tests/dotnet-maui/maui-theming/eval.yaml | ||
| tests/dotnet-msbuild/msbuild-antipatterns/eval.yaml | ||
| tests/dotnet-template-engine/template-smart-defaults/eval.yaml | ||
| tests/dotnet-test/find-untested-sources/eval.yaml | ||
| tests/dotnet-test/generate-testability-wrappers/eval.yaml |
There was a problem hiding this comment.
Heads-up while we're in this file: check_eval_quality.py is already red on main because these three (maui-dependency-injection, maui-shell-navigation, maui-theming) are stale underpowered exemptions that now clear the 5-trial floor. I verified it — clean origin/main reports 4 errors, this branch reports 3, and the one that drops is maui-collectionview (which this PR removes). So this PR isn't the cause and actually improves the count.
Since we're editing the allowlist anyway, want to drop these three stale lines too and get the gate green in the same change? None of them are in the eight-skill scope, so it's purely opportunistic — fine to skip if you'd rather keep this PR tightly scoped.
(Copilot, commenting on Abhitej's behalf.)
| "dotnet-msbuild" = @("binlog-failure-analysis", "build-parallelism", "build-perf-diagnostics", "incremental-build", "msbuild-modernization") | ||
| "dotnet-test" = @("migrate-vstest-to-mtp", "run-tests", "writing-mstest-tests") | ||
| "dotnet-upgrade" = @("dotnet-aot-compat", "migrate-dotnet9-to-dotnet10", "migrate-nullable-references") | ||
| "dotnet-upgrade" = @("migrate-nullable-references") |
There was a problem hiding this comment.
Same inbound-reference nit on the upgrade side: plugins/dotnet-upgrade/skills/migrate-dotnet8-to-dotnet9/SKILL.md:30 still tells users to "use the migrate-dotnet9-to-dotnet10 skill as the next step", which we're removing here — so that hand-off now dead-ends on main. Curious whether we want to update it to point at migrate-dotnet10-to-dotnet11 or just drop the pointer while the skill is on staging.
(Copilot, commenting on Abhitej's behalf.)
|
👋 @y87feng — this PR has changes requested,3 unresolved review thread(s),merge conflict. When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
|
I'd like to push back on staging The skill works. Against Head-to-head on Opus 4.8:
A blind cross-family judge scored that as Eval defects:
The label is also inconsistent with the harness. From #893's own numbers (~33 trials, 13L, 2T) that's ~18 wins, and On usage: 30-day invocation counts are the wrong instrument for incident-driven crash triage. Its sibling Suggest dropping this skill from the PR and fixing the eval instead: real BuildIds in every fixture, |
|
Re: @steveisok's point that Run: https://github.com/dotnet/skills/actions/runs/30862999210
Takeaways:
Recommendation: drop I'm re-running the other skills being staged in this PR to check whether any of them fail for the same measurement reasons, and will follow up with those results. |
Cross-family re-validation of the staged skills (latest
|
| Skill | Original | Re-run verdict | Re-category |
|---|---|---|---|
| android-tombstone-symbolication | P0 FIX-REGRESSION | 4/5 positive, regressed=false |
eval-defect (biased + underpowered) — fix in flight |
| check-bin-obj-clash | P1 TRIM-COST | n=1 → can't pass by construction | eval-defect (underpowered) |
| eval-performance | strengthen | n=1 → can't pass by construction | eval-defect (underpowered) |
| msbuild-antipatterns | P0 | neutral; 3/4 stimuli off-target (F#) | eval-defect (construct-invalid) |
| migrate-static-to-wrapper | P0 FIX-REGRESSION | positive; sonnet46 credibly better, 0 regressions | keep — skill is helpful |
| migrate-dotnet9-to-dotnet10 | P0 FIX-REGRESSION | helps haiku (+0.66); gpt dip is polish-only (both correct) | real but overstated → P2 rubric fix |
| dotnet-aot-compat | P1 TRIM-COST | n=1 → can't pass by construction | eval-defect (underpowered) |
| maui-collectionview | P0 | fixed + revalidated in merged #947 | stale — drop |
Why the "fails" aren't regressions: three evals have a single stimulus (one trial → sign test p=0.5 always, can never pass); one amplifies marginal "both-correct" losses into a "regression" via direction-based sign test; one has fixture/grader/guardrail bias; one tests off-target scenarios. Details per plugin in #896 / #899 / #901 / #895.
Suggested changes to this PR
- Reframe from "shelve 8 P0/P1 skills" → "fix these evals and re-baseline." The defect is mostly the instrument.
- Drop:
maui-collectionview(stale) andmigrate-static-to-wrapper(skill is helpful). - Keep skill, fix eval (don't stage on pass-rate): android-tombstone (in flight), check-bin-obj-clash, eval-performance, msbuild-antipatterns, dotnet-aot-compat.
- Only genuine follow-up:
migrate-dotnet9-to-dotnet10— and that's a rubric-tightening ("both correct ⇒ tie"), not a P0 shelve. - If cost is the real driver for the TRIM-COST skills, state it explicitly and separately from the (invalid) pass-rate argument.
Net: of the 8, 0–1 warrant staging on the original premise; the rest are eval repairs tracked in #986.
Summary
Move eight cross-family P0/P1 skills out of
mainwhile retaining their complete pre-removal versions on the long-livedstagingbranch.This PR removes each selected skill together with its eval/fixture directory, then cleans up only the associated CODEOWNERS, external-dependency allowlist, eval-quality allowlist, and dashboard sample-data entries.
Why these skills
The selection combines the latest corrected cross-family classifications (#893, #895, #896, #899, #901, including the #909 judge-infrastructure correction) with independently verified 30-day Copilot CLI usage from Kusto.
Telemetry window: 2026-07-02 11:11:59 through 2026-07-30 18:39:32. Automation accounts are accounts invoking at least 20 of the 92 skills; globally they account for 87.5% of invocations.
android-tombstone-symbolicationdotnet-diagFIX-REGRESSIONmaui-collectionviewdotnet-mauiFIX-REGRESSIONmsbuild-antipatternsdotnet-msbuildFIX-DISCOVERYeval-performancedotnet-msbuildFIX-REGRESSIONcheck-bin-obj-clashdotnet-msbuildTRIM-COSTmigrate-static-to-wrapperdotnet-testFIX-REGRESSIONmigrate-dotnet9-to-dotnet10dotnet-upgradeFIX-REGRESSIONdotnet-aot-compatdotnet-upgradeTRIM-COSTstagingis a quarantine/repair branch, not a deletion of history. Skills with demonstrated demand can be fixed and revalidated there before being proposed back tomain.Explicitly retained on main
dotnet-blazorskills, includingplan-ui-changeandsupport-prerenderingmicrobenchmarking, based on strong organic usage (31 users, 127 invocations, 7 users with 5+ invocations, 10 multi-day users)code-testing-agent, based on its exceptionally broad organic usageValidation
skill-validator checkacross all plugins: passedgit diff --check: passedorigin/staging