Fix migrate-static-to-wrapper skill activation on evals#864
Conversation
Sharpen the description boundary between migrate-static-to-wrapper and generate-testability-wrappers so prompts that replace call sites when the abstraction is already registered in DI activate the migration skill. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Updates skill descriptions in the dotnet-test plugin to reduce routing overlap and ensure the migrate-static-to-wrapper skill activates reliably in eval scenarios where the abstraction (e.g., TimeProvider) is already available via DI.
Changes:
- Expanded
migrate-static-to-wrapper“USE FOR” triggers to include “replace call sites / add constructor parameter when already registered in DI” and “scoped files only” phrasing. - Tightened
generate-testability-wrappers“DO NOT USE” guidance to explicitly hand off tomigrate-static-to-wrapperwhen the abstraction already exists or is already registered in DI.
Show a summary per file
| File | Description |
|---|---|
| plugins/dotnet-test/skills/migrate-static-to-wrapper/SKILL.md | Adds more specific activation phrases to better target call-site migration when DI registration already exists and for scoped-file-only migrations. |
| plugins/dotnet-test/skills/generate-testability-wrappers/SKILL.md | Narrows the “DO NOT USE” guidance to reduce overlap and route existing-abstraction call-site migrations to the correct skill. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
Skill Validation Results
[1] Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
- Teach the ambient-context seam for genuinely static classes in the skill, so the static-class scenario (previously 2/5) can reach the rubric's expected answer instead of recommending a non-static rewrite. - Give the two named migrate scenarios real headroom by introducing a realistic local-vs-UTC pitfall (DateTime.Now alongside DateTime.UtcNow) in the CouponService fixture, so a naive baseline that conflates local and UTC time loses quality while the skill's mapping guidance adds measurable value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
Skill Validation Results
[1] (Plugin) Quality unchanged but weighted score is -10.0% due to: tokens (12829 → 65641), tool calls (0 → 4), time (12.3s → 29.7s) Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
|
✅ Evaluation passed for |
Eval artifacts showed generate-testability-wrappers reliably out-routing migrate-static-to-wrapper in plugin mode (and even stealing the static-class scenario). Rewrite both descriptions around the real discriminator: whether the abstraction already exists / is registered in DI. migrate now leads with the concrete 'already registered -> replace call sites' phrasing; generate is scoped to first-time wrapper creation/registration. Both stay under the 1024-char limit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
Skill Validation Results
[1] Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
- Scoped-files scenario: assert DateTime.Now is also removed from CouponService and align rubric wording with the both-symbols prompt. - Scenario 1 rubric: mention DateTime.Now alongside DateTime.UtcNow. - Fixture comment: stop overstating a UTC-offset guarantee the code does not provide; just note the intentional local-time semantics. - Ambient-context guidance: recommend restore-in-finally and call out the parallel-test implications of a mutable static seam. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Skill Coverage Report
|
|
/evaluate |
Skill Validation Results
[1] Model: claude-opus-4.6 | Judge: claude-opus-4.6 🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Problem
migrate-static-to-wrapperwas failing skill activation on the eval, and its routing overlapped with the sibling skillgenerate-testability-wrappers.Changes
descriptionboundary so migration prompts (where the abstraction is already registered in DI) reliably activatemigrate-static-to-wrapperinstead ofgenerate-testability-wrappers. Isolated activation is now ✅ across all scenarios.generate-testability-wrappersout-routing (and even stealing) migration scenarios in plugin mode. Rewrote both descriptions around the real discriminator — does the abstraction already exist / is it registered? (migrate= replace existing call sites;generate= first-time wrapper creation/registration). Plugin-arm activation for the migration scenarios is now materially improved andgenerateno longer steals them.TimeProviderseam defaulting toTimeProvider.System, overridden in tests) so the genuinely-hard static-class scenario reaches the rubric's expected answer instead of recommending a non-static rewrite (2.0/5 → 2.7/5).DateTime.NowalongsideDateTime.UtcNow) to theCouponServicefixture plus a matching rubric item, so migrations that conflate local and UTC time are penalized.About the remaining ❌ scenario cells
The evaluation gate passes (
evaluation-status: pass). The residual red cells are not activation bugs — analysis of theresults.jsonartifacts shows they are the skill-validator's documented Pattern #8 (baseline already good, no headroom) plus Pattern #3 (high run-to-run variance, CV up to ~480%):migrate-static-to-wrappersits ~9,200 chars into the 15,000-char alphabetical skill-menu budget, so it is not truncated; plugin-arm flips are single-run routing variance, which the footnotes themselves suggest addressing with--runs 5.Per the guide's "improving the skill vs. gaming the eval" guidance, no further description edits are warranted for these saturated/noisy cases.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com