Add .NET refactoring and breaking-change skills#873
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Skill Coverage Report
Uncovered:
|
|
/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 |
There was a problem hiding this comment.
Pull request overview
This PR adds two new .NET-focused skills to the plugins/dotnet plugin—one for behavior-preserving C# refactoring workflows and one for .NET-specific breaking-change/compatibility guardrails—along with eval coverage and fixtures that exercise the key hazards (public API baselines, multi-targeting #if, generated/partial code, and friend assemblies).
Changes:
- Added
csharp-refactoringskill guidance plus an operation catalog reference. - Added
dotnet-breaking-changesskill guidance plus focused reference docs for the four “hidden surfaces”. - Added eval suites + identical lightweight Billing fixture solutions under
tests/dotnet/for both skills; updated CODEOWNERS and dotnet plugin README.
Show a summary per file
| File | Description |
|---|---|
| tests/dotnet/dotnet-breaking-changes/tests/Billing.Tests/BillingTests.cs | Adds fixture tests used by the breaking-change eval scenarios. |
| tests/dotnet/dotnet-breaking-changes/tests/Billing.Tests/Billing.Tests.csproj | Adds an xUnit test project targeting net10.0 for the breaking-change fixture solution. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/PublicAPI.Shipped.txt | Adds a public API baseline file used by eval prompts/assertions. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/Pricing.cs | Adds pricing/tax types used for refactor/breaking-change exercises. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/PlatformInfo.cs | Adds multi-targeting #if example surface for breaking-change checks. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/OrderProcessor.cs | Adds intentionally-refactorable implementation used by scenarios. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/Coupons.g.cs.template | Adds generator-input template to simulate generated/partial hazards. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/Coupons.cs | Adds hand-authored partial type paired with the generated template. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/Billing.csproj | Adds multi-targeted library project and build-time generation hook. |
| tests/dotnet/dotnet-breaking-changes/src/Billing/AppSettingsHelper.cs | Adds duplicated parsing helpers for public-API consolidation scenarios. |
| tests/dotnet/dotnet-breaking-changes/Fixture.sln | Adds the breaking-change fixture solution container. |
| tests/dotnet/dotnet-breaking-changes/eval.yaml | Adds breaking-change eval scenarios and build/test gates. |
| tests/dotnet/csharp-refactoring/tests/Billing.Tests/BillingTests.cs | Adds fixture tests used by the refactoring eval scenarios. |
| tests/dotnet/csharp-refactoring/tests/Billing.Tests/Billing.Tests.csproj | Adds an xUnit test project targeting net10.0 for the refactoring fixture solution. |
| tests/dotnet/csharp-refactoring/src/Billing/PublicAPI.Shipped.txt | Adds a public API baseline file used by refactoring prompts/assertions. |
| tests/dotnet/csharp-refactoring/src/Billing/Pricing.cs | Adds pricing/tax types used for refactoring exercises. |
| tests/dotnet/csharp-refactoring/src/Billing/PlatformInfo.cs | Adds multi-targeting #if example surface for refactoring checks. |
| tests/dotnet/csharp-refactoring/src/Billing/OrderProcessor.cs | Adds intentionally-refactorable implementation used by scenarios. |
| tests/dotnet/csharp-refactoring/src/Billing/Coupons.g.cs.template | Adds generator-input template to simulate generated/partial hazards. |
| tests/dotnet/csharp-refactoring/src/Billing/Coupons.cs | Adds hand-authored partial type paired with the generated template. |
| tests/dotnet/csharp-refactoring/src/Billing/Billing.csproj | Adds multi-targeted library project and build-time generation hook. |
| tests/dotnet/csharp-refactoring/src/Billing/AppSettingsHelper.cs | Adds duplicated parsing helpers used in refactoring scenarios. |
| tests/dotnet/csharp-refactoring/Fixture.sln | Adds the refactoring fixture solution container. |
| tests/dotnet/csharp-refactoring/eval.yaml | Adds refactoring eval scenarios and build/test gates. |
| plugins/dotnet/skills/dotnet-breaking-changes/SKILL.md | Introduces the breaking-change skill and when/when-not-to-use guidance. |
| plugins/dotnet/skills/dotnet-breaking-changes/references/source-generation.md | Adds detailed guidance for generated/partial code hazards. |
| plugins/dotnet/skills/dotnet-breaking-changes/references/public-api.md | Adds detailed guidance for public API gating and compatibility decisions. |
| plugins/dotnet/skills/dotnet-breaking-changes/references/multi-targeting.md | Adds detailed guidance for multi-targeting and #if branch correctness. |
| plugins/dotnet/skills/dotnet-breaking-changes/references/internals-visible-to.md | Adds detailed guidance for friend-assembly/internal-surface hazards. |
| plugins/dotnet/skills/csharp-refactoring/SKILL.md | Introduces the refactoring skill and a stepwise safety contract. |
| plugins/dotnet/skills/csharp-refactoring/references/operation-catalog.md | Adds a more detailed refactoring operation taxonomy reference. |
| plugins/dotnet/README.md | Updates the dotnet plugin skill list to include the new skills. |
| eng/known-domains.txt | Adds github.com/dotnet/skills to known domains. |
| .github/CODEOWNERS | Adds ownership entries for the new skills and their tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 34/34 changed files
- Comments generated: 0
|
/evaluate |
| contract unchanged. On a multi-targeted project, build/test **each** TFM — a green default build can | ||
| still be broken on another target. | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
I suggest removing lines 147-152. Models know how to build and test and should favor the repos instructions for doing it over this generic option.
There was a problem hiding this comment.
Thanks Wendy — addressed in the v3 trim (df61fce). The section now leads with: "Use the repo's own build/test workflow when it documents one (\README/\CONTRIBUTING, \�uild., \�ng/, \global.json, .github/workflows); its instructions win over any generic command."* The generic \dotnet build/\dotnet test\ is kept only as an explicit \Otherwise:\ fallback for repos with no documented workflow, and trimmed to a few lines. Happy to drop the fallback entirely if you'd prefer zero generic commands.
|
👋 @AbhitejJohn — this PR has 1 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 |
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 |
JanKrivanek
left a comment
JanKrivanek
left a comment
There was a problem hiding this comment.
Since the skills have nontrivial size - the main concern now is overlap between the two skills + cross-loading cost. csharp-refactoring instructs the agent to also load dotnet-breaking-changes, and the two share material ("stop and escalate," public API, forwarders, partial/generated). The split is defensible (breaking-changes is meant to apply to features/fixes too, not just refactors), but loading two long skills for one refactor is a real context cost that the evals don't yet justify.
Trim study for PR #873: csharp-refactoring-trim is a ~30% shorter rewrite of csharp-refactoring (drops When-to-use/Inputs/Outputs boilerplate + verbose LSP launch prose, compresses the dbc-overlapping hazards section) while preserving every rubric-rewarded behavior. Adds a 'trimmed' experiment arm and bumps runs to 3 so CI generates baseline/skilled/trimmed trajectories for local cross-family re-judging. Eval-only scratch branch; not part of the PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
👋 @AbhitejJohn — this PR has 1 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 |
Robustness probe for the csharp-refactoring trim: all prior CI trajectories were executed by claude-opus-4.6. Flip executor to gpt-5.5 (GPT family) and CI judge to claude-opus-4.8 so the (executor, judge) pair stays cross-family. Tests whether trimmed-vs-current non-inferiority holds under a different model family. Eval-branch only; not on PR #873. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
Closes the 'refactoring-only endpoint too narrow' risk flagged by the
cross-family rubber-duck. Adds two boundary stimuli never used in trim tuning:
9. Separate a smuggled behavior change from a requested rename (rename +
smuggled 10%->12% discount change) -> should separate/flag, not bundle.
10. Recognize a behavior-changing simplification is not a refactor (remove
free-shipping-over-\ rule under a 'cleanup' framing) -> should not
silently change observable behavior.
Executor reverted to claude-opus-4.6 / judge gpt-5.5 (re-judge) to match the
stringent Confirm A family where boundary behavior looked weakest.
Eval-branch only; not on PR #873.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
Replaces the shipped skill body with the validated v3 variant: judgment-first, rigor proportional to blast radius, redundant catalog/list scaffolding removed. ~53% smaller (12,811->6,039 chars) with equal or better cross-family eval quality and no measured regression. Description (929 chars) and all safety rules retained. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
👋 @AbhitejJohn — this PR has 1 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 |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
@JanKrivanek — fair concern, thanks. I took the size + cross-loading cost seriously and did two things: trimmed both skills and ran a breadth eval to confirm the trims don't cost quality. Size (SKILL.md bodies; the
Combined the two bodies drop ~40% (20.8k→12.4k chars), which directly cuts the "load two long skills for one refactor" cost you flagged. I removed duplicated prose — the Eval — does the trim hurt? 5 executor families (opus-4.8, gpt-5.5, sonnet-4.6, haiku-4.5, mai-flash), a 2-family judge ensemble (gpt-5.5 + claude-opus-4.8), blinded arm order, n=5, with a negative-control scenario per skill:
Honest caveats: single self-authored fixture suite, small scenario counts (6 for dbc), and CIs are bootstrapped over trials — so treat the sub-significant deltas as suggestive rather than proof of broad generalization. I'm not claiming formal non-inferiority, only that the smaller versions hold up at least as well as the larger ones across every family I could measure. On the split: I kept it deliberately (breaking-changes is meant to apply to features/fixes, not only refactors), but the shared material is now much thinner, so cross-loading is meaningfully cheaper. Re-triggering |
|
/evaluate |
|
❌ Evaluation failed. View workflow run |
Rely on the SDK-default language version per pinned TFMs (net8.0;net10.0) so fixture compile behavior does not drift with the installed SDK. Both fixtures build clean (0 warnings/errors) without it. Addresses reviewer feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
/evaluate |
|
|
||
| ## Partial types span files | ||
|
|
||
| A `partial class`/`record`/`struct` (and, since C# 13, `partial` properties/methods) is one type split |
| # Multi-target gate: the new method must compile on BOTH net8.0 and net10.0. | ||
| - type: run_command_and_assert | ||
| command_to_run: "dotnet" | ||
| command_arguments: "build Fixture.sln -v:q" | ||
| expected_exit_code: 0 | ||
| command_timeout: 300 | ||
| # Existing behavior stays green after the additive change. | ||
| - type: run_command_and_assert | ||
| command_to_run: "dotnet" | ||
| command_arguments: "test Fixture.sln -v:q" | ||
| expected_exit_code: 0 |
|
❌ Evaluation failed. View workflow run |
Picks up the Vally-harness eval migration (#877) and related infra so the /evaluate workflow can discover and run this PR's evals. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
…ally harness schema Adapts both eval.yaml files from the legacy scenarios/assertions format to the stimuli/graders/config schema introduced by #877 (Migrate LLM evals to the Vally harness). Prompts and rubrics are preserved verbatim; assertions map 1:1 to graders (file_contains->file-contains, run_command_and_assert-> run-command, output_matches->output-matches) and copy_test_files-> environment.files. Validated with `vally experiment run --dry-run`: both evals now resolve and match the experiment filter (previously matched none). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
/evaluate |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (8)
tests/dotnet/csharp-refactoring/eval.yaml:62
file-containsgraders usevalue:(notsubstring:). Usingsubstring:here may cause the assertion to be ignored.
- type: file-contains
config:
path: src/Billing/OrderProcessor.cs
substring: private
tests/dotnet/csharp-refactoring/eval.yaml:92
file-containsgraders usevalue:(notsubstring:). Usingsubstring:here may cause the assertion to be ignored.
- type: file-contains
config:
path: src/Billing/OrderProcessor.cs
substring: private
tests/dotnet/csharp-refactoring/eval.yaml:157
file-not-containsgraders usevalue:(notsubstring:). Usingsubstring:here may cause the assertion to be ignored.
- type: file-not-contains
config:
path: src/Billing/Pricing.cs
substring: class GoldPricing
- type: file-not-contains
config:
path: src/Billing/Pricing.cs
substring: class SilverPricing
tests/dotnet/dotnet-breaking-changes/eval.yaml:50
file-containsgraders usevalue:(notsubstring:). Withsubstring:the grader may ignore the match and the scenario can pass/fail incorrectly.
- type: file-contains
config:
path: src/Billing/PublicAPI.Shipped.txt
substring: ParseIntSetting
tests/dotnet/dotnet-breaking-changes/eval.yaml:76
file-containsgraders usevalue:(notsubstring:). Withsubstring:the grader may ignore the match and the scenario can pass/fail incorrectly.
- type: file-contains
config:
path: src/Billing/PlatformInfo.cs
substring: FormatLabel
tests/dotnet/dotnet-breaking-changes/eval.yaml:118
file-containsgraders usevalue:(notsubstring:). Withsubstring:the grader may ignore the match and the scenario can pass/fail incorrectly.
- type: file-contains
config:
path: src/Billing/Coupons.cs
substring: Apply
# Positive check that the fix went to the GENERATOR INPUT (the template),
# which is where the generated part's reference to the renamed member lives.
# Hand-editing the obj/ output would be overwritten on the next build.
- type: file-contains
config:
path: src/Billing/Coupons.g.cs.template
substring: Apply
tests/dotnet/dotnet-breaking-changes/eval.yaml:147
file-containsgraders usevalue:(notsubstring:). Withsubstring:the grader may ignore the match and the scenario can pass/fail incorrectly.
- type: file-contains
config:
path: src/Billing/PlatformInfo.cs
substring: Tag
tests/dotnet/dotnet-breaking-changes/eval.yaml:181
file-containsgraders usevalue:(notsubstring:). Withsubstring:the grader may ignore the match and the scenario can pass/fail incorrectly.
- type: file-contains
config:
path: src/Billing/OrderProcessor.cs
substring: runningTotal
- Files reviewed: 34/34 changed files
- Comments generated: 5
| The #1 way a "rename" silently corrupts code is editing textual matches (comments, strings, unrelated | ||
| overloads) instead of real **bindings**. Find every binding reference first, then edit semantically. Use | ||
| the strongest tool available: an IDE/Roslyn workspace refactoring, then the C# LSP the | ||
| [`dotnet` plugin declares](https://github.com/dotnet/skills/blob/main/plugins/dotnet/lsp.json) |
| name: dotnet-breaking-changes | ||
| description: Evaluates the dotnet/dotnet-breaking-changes skill | ||
| type: capability | ||
| config: | ||
| timeout: 15m | ||
| stimuli: | ||
| - name: Answer a public-API question before consolidating duplicated parsing | ||
| prompt: "Is `AppSettingsHelper` part of the public API anywhere? Its int/bool setting parsing looks duplicated with `ConfigReader`. If it's safe to do, consolidate the setting parsing into one place and update the callers, without changing behavior. The solution is at Fixture.sln." |
|
❌ Evaluation failed. View workflow run |
…ot 'substring' The Vally file-contains and file-not-contains graders require the config key 'value' (per the schema and all main eval.yaml files); 'substring' is rejected with [invalid-grader-config]. Verified with `vally lint --eval-spec`: both evals now lint with 0 errors, matching main's passing evals (only the benign scoring-defaults + config-deprecation warnings that main evals also emit). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
|
/evaluate |
📊 Skill Evaluation Results2 skill(s) evaluated — 0 improved, 2 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
|
👋 @AbhitejJohn — this PR has 5 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 |
Summary
Refactoring is a common operation for .NET developers, and a dedicated skill can help agents make these changes in the way .NET teams expect: behavior-preserving, incremental, and validated with build/test gates.
This PR adds two related skills:
csharp-refactoring: guides safe C#/.NET refactoring work such as rename, move, extract, inline, split, consolidate/de-duplicate, and modernization while preserving behavior.dotnet-breaking-changes: provides the compatibility guardrails needed when a refactor touches public API, multi-targeting, source-generated/partial code, orInternalsVisibleTosurfaces.Why these skills
csharp-refactoringfocuses on the refactoring workflow itself: establish a green baseline, choose one named operation, find true references, prefer semantics-aware edits, and re-gate after each step. Its reference file expands the operation catalog and maps common refactoring operations to the kinds of changes .NET teams regularly make.dotnet-breaking-changescovers the .NET-specific surfaces where a change can compile and pass tests while still breaking downstream consumers. Its reference files explain how to inspect and handle:PublicAPI.*, ApiCompat, package validation)#ifbranchesInternalsVisibleToTogether, the skills let an agent keep refactoring focused on behavior preservation while still checking the .NET compatibility surfaces that matter in real repos.
Validation