diff --git a/plugins/dotnet/skills/csharp-refactoring/SKILL.md b/plugins/dotnet/skills/csharp-refactoring/SKILL.md index 91174df6..0e408af8 100644 --- a/plugins/dotnet/skills/csharp-refactoring/SKILL.md +++ b/plugins/dotnet/skills/csharp-refactoring/SKILL.md @@ -1,6 +1,6 @@ --- name: csharp-refactoring -description: "Performs safe, behavior-preserving refactoring of C#/.NET code, verified with build, tests, and analyzers. USE FOR: any request to rename, move, extract, split, modernize, or otherwise restructure C# code without changing behavior, including small requests like 'rename X to Y': rename a symbol/type/file across a solution; move a type or static members to another file/namespace/project; extract a method, interface, or base class; pull members up; inline a method or local; split a large class/file; consolidate or de-duplicate copy-pasted code; sync namespaces to folders; or modernize to current C# idioms (file-scoped namespaces, primary constructors, collection expressions, target-typed new, pattern matching). DO NOT USE TO IMPLEMENT: features, bug fixes, new tests, framework or NuGet upgrades (use dotnet-upgrade), or formatting-only passes (use dotnet format)." +description: "Performs safe, behavior-preserving refactoring of C#/.NET code, verified with build, tests, and analyzers. USE FOR: requests to rename, move, extract, split, consolidate, de-duplicate, modernize, or otherwise restructure C# code without changing behavior, including 'rename X to Y', generated/partial declarations, public or serialized contracts, multi-targeted code, and mixed requests where a feature, bug fix, package/framework upgrade, nullability change, or other behavior/contract change is presented as a refactor and must be separated or declined. DO NOT USE FOR: ordinary feature or bug-fix requests that are not framed as refactoring; package/framework upgrades after they have been reclassified (use dotnet-upgrade); new tests; or formatting-only passes (use dotnet format)." license: MIT --- @@ -19,17 +19,19 @@ When the request changes results, decline the refactor framing and handle it hon - **Framework / NuGet version bump** → not a refactor. Stop this workflow without editing project or package files, explain the reclassification, and redirect to the `dotnet-upgrade` skills. A successful build does not make an upgrade behavior-preserving. -- **New feature** (e.g. add a pricing tier, a flag, an endpoint) → not a refactor. Stop this +- **New feature** (e.g. add a capability, a flag, or an endpoint) → not a refactor. Stop this workflow and route it to the appropriate feature workflow; do not implement it here. If a separately authorized feature also has a structural cleanup, keep the two changes distinct in the implementation and final report. -- **Bug fix or "simplification" that changes output** (e.g. always charge shipping, bump a discount) → +- **Bug fix or "simplification" that changes output** (e.g. change a threshold or calculation) → a behavior **change**. It is a legitimate task — do it as an explicit, tested change and update the - tests that lock in the new behavior — but keep it **separate** from any refactor and never label it - behavior-preserving. Do not stall or report "nothing to change." -- **A rename/move with a behavior tweak smuggled in** ("rename X, and while you're there bump the rate") - → split it: do the rename as a behavior-preserving refactor, and treat the tweak as its own tested - change, or flag it and defer. + tests that lock in the new behavior — but only after it is authorized as a behavior change. Under an + explicitly behavior-preserving request, leave that edit undone, complete only any separable structural + operation, and report the deferred change. Never label the behavior change behavior-preserving. +- **A rename/move with a behavior tweak smuggled in** ("rename X, and while you're there change the result") + → do the rename/move as the behavior-preserving operation and defer the tweak. Perform the tweak only + after the user separately accepts it as a tested behavior change; do not silently turn one + "behavior-preserving" task into two edits. Only when the request is genuinely structure-only do you proceed as a refactor. @@ -47,6 +49,25 @@ declaration, and edit the generator input, never generated (`*.g.cs`) output. For the operation → Roslyn-provider mapping and representative PRs, see [references/operation-catalog.md](references/operation-catalog.md). +## Consolidate toward the existing source of truth + +When de-duplicating, preserve the ownership direction stated by the code or request. If `B` duplicates +an implementation already owned by `A`, keep `A` canonical and make `B` delegate to it; do not invert +the dependency merely because either direction compiles. Preserve public compatibility wrappers when +the duplicate surface is shipped, and migrate only in-repo callers that are safe to move. + +## Preserve contracts beyond C# call sites + +Compilation proves binding compatibility, not every external contract. Before renaming or moving a +type/member, check whether its name or metadata is observed by serialization, reflection, dependency +injection, configuration binding, source generators, P/Invoke, or `dynamic`. + +| Boundary | Required decision | +|---|---| +| Serialized/configuration name | Preserve the external name with the repository's existing mechanism (for example, `JsonPropertyName`) while migrating C# callers; run a focused round-trip or payload test. | +| Public nullable annotation | Treat tightening or loosening nullability as a source-contract change, not a behavior-preserving refactor. Leave it unchanged unless the contract change is explicitly authorized and validated. | +| Uncovered reflection or runtime lookup | Do not guess that a compile-clean rename is safe. Preserve the observed name or stop and report the unverified runtime boundary. | + ## Verify proportionally Confirm behavior is preserved after the edit — scaled to blast radius, not a fixed ceremony: diff --git a/tests/dotnet/csharp-refactoring/eval.yaml b/tests/dotnet/csharp-refactoring/eval.yaml index 0e1bbb73..f6b6dec6 100644 --- a/tests/dotnet/csharp-refactoring/eval.yaml +++ b/tests/dotnet/csharp-refactoring/eval.yaml @@ -20,6 +20,8 @@ stimuli: dest: src/Billing/Billing.csproj - src: src/Billing/CollisionExamples.cs dest: src/Billing/CollisionExamples.cs + - src: src/Billing/CustomerProfile.cs + dest: src/Billing/CustomerProfile.cs - src: src/Billing/Coupons.cs dest: src/Billing/Coupons.cs - src: src/Billing/Coupons.g.cs.template @@ -59,7 +61,6 @@ stimuli: config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: &dual-tfm-tests 'Total tests: (?:1[2-9]|[2-9][0-9]|[1-9][0-9]{2,})[\s\S]*Total tests: (?:1[2-9]|[2-9][0-9]|[1-9][0-9]{2,})' timeout: 5m - type: file-contains config: @@ -69,12 +70,6 @@ stimuli: config: path: src/Billing/PublicAPI.Shipped.txt value: static Billing.AppSettingsHelper.ParseIntSetting - - type: output-matches - config: - pattern: (public API|compatib|obsolete|shim) - - type: output-matches - config: - pattern: (dotnet (build|test)|rebuild|re-?run[\s\S]{0,30}tests|tests?[\s\S]{0,30}(pass|green)) - type: prompt rubric: - Migrates in-repository callers to ParseIntegerSetting while preserving the shipped ParseIntSetting entry point as an obsolete forwarding compatibility shim @@ -101,14 +96,7 @@ stimuli: config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: *dual-tfm-tests timeout: 5m - - type: output-matches - config: - pattern: (generated|generator|template) - - type: output-matches - config: - pattern: (dotnet (build|test)|rebuild|re-?run[\s\S]{0,30}tests|tests?[\s\S]{0,30}(pass|green)) - type: prompt rubric: - Finds the generated declaration and the hand-authored partial caller as binding-related parts of one rename @@ -130,8 +118,7 @@ stimuli: text = '\n'.join(path.read_text(encoding='utf-8', errors='ignore') for path in paths); match = re.search(r'class\s+ConfigReader\b[^{]*\{(?P.*?)\n\}', text, re.S); body = match.group('body') if match else ''; - sys.exit(0 if 'AppSettingsHelper.ParseIntSetting' in body - and 'AppSettingsHelper.ParseBoolSetting' in body + sys.exit(0 if 'ReadInt' in body and 'ReadBool' in body and 'TryParse' not in body else 1)" expected_exit_code: 0 timeout: 1m @@ -143,14 +130,7 @@ stimuli: config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: *dual-tfm-tests timeout: 5m - - type: output-matches - config: - pattern: (public API|compatib|shipped|forward) - - type: output-matches - config: - pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green)) - type: prompt rubric: - Removes the duplicate parsing implementation by making ConfigReader delegate to AppSettingsHelper @@ -168,16 +148,12 @@ stimuli: config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: *dual-tfm-tests timeout: 5m # The pass-through wrapper is actually gone. - type: file-not-contains config: path: src/Billing/Pricing.cs value: ApplyTaxWrapper - - type: output-matches - config: - pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green)) - type: prompt rubric: - Finds the true binding references to the wrapper before removing it, not just textual matches @@ -195,7 +171,6 @@ stimuli: config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: *dual-tfm-tests timeout: 5m # The two near-identical classes are actually gone (merged into one type). - type: file-not-contains @@ -206,9 +181,6 @@ stimuli: config: path: src/Billing/Pricing.cs value: class SilverPricing - - type: output-matches - config: - pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green)) - type: prompt rubric: - Merges the two classes into a single type parameterized by name and rate, preserving each tier's values @@ -238,9 +210,6 @@ stimuli: config: path: tests/Billing.Tests/Billing.Tests.csproj value: Version="2.8.2" - - type: output-matches - config: - pattern: (not a .{0,25}refactor|isn'?t a .{0,25}refactor|major version|out of scope|separate .{0,20}(change|workflow)|dotnet-upgrade|redirect|hand[- ]?off) - type: prompt rubric: - Recognizes that a framework or package major-version upgrade is not a behavior-preserving refactor @@ -259,9 +228,6 @@ stimuli: config: path: '**/*.cs' value: Platinum - - type: output-matches - config: - pattern: (new feature|feature addition|not a .{0,25}refactor|isn'?t a .{0,25}refactor|out of scope|separate .{0,20}(change|workflow)|redirect|hand[- ]?off) - type: prompt rubric: - Recognizes that adding a new pricing tier is a new feature, not a behavior-preserving refactor @@ -284,17 +250,10 @@ stimuli: config: path: src/Billing/OrderProcessor.cs value: subtotal >= 100m - - type: output-matches - config: - pattern: (separate|follow[- ]?up|behavior[- ]?(preserving|chang)|not (a )?refactor) - - type: output-matches - config: - pattern: (shipping|threshold|\$50) - type: run-command config: command: dotnet test Fixture.sln --verbosity normal expected_exit_code: 0 - stdout_matches: *dual-tfm-tests timeout: 5m - type: prompt rubric: @@ -302,6 +261,87 @@ stimuli: - Refuses or defers the $50 shipping-threshold change because it deliberately changes invoice behavior and belongs in a separate tested change - Keeps the existing $100 threshold and does not claim the combined request is behavior-preserving + - name: Preserve a serialized name while renaming a C# property + prompt: "Rename `CustomerProfile.LoyaltyLevel` to `Tier` and migrate its C# callers, but preserve the existing JSON property name so serialized payloads do not change. Verify the solution at Fixture.sln." + environment: *fixture-environment + graders: + - type: file-contains + config: + path: src/Billing/CustomerProfile.cs + value: Tier + - type: file-contains + config: + path: src/Billing/CustomerProfile.cs + value: JsonPropertyName("LoyaltyLevel") + - type: file-not-contains + config: + path: tests/Billing.Tests/BillingTests.cs + value: LoyaltyLevel = "gold" + - type: run-command + config: + command: dotnet test Fixture.sln --verbosity normal + expected_exit_code: 0 + timeout: 5m + - type: prompt + rubric: + - Renames the C# property and migrates callers without treating a compile-clean rename as proof that the JSON contract is unchanged + - Preserves the wire name `LoyaltyLevel` through serialization metadata rather than changing existing payloads to `Tier` + - Runs the tests that exercise the serialized payload after the refactor + + - name: Keep a shipped nullable contract out of a refactor + prompt: "Simplify `AppSettingsHelper.ParseBoolSetting` by changing its shipped `string? raw` parameter to `string raw`. Treat this nullable-annotation cleanup as behavior-preserving and verify Fixture.sln." + environment: *fixture-environment + graders: + - type: file-contains + config: + path: src/Billing/AppSettingsHelper.cs + value: ParseBoolSetting(string? raw, bool fallback) + - type: file-contains + config: + path: src/Billing/PublicAPI.Shipped.txt + value: ParseBoolSetting(string? raw, bool fallback) + - type: run-command + config: + command: dotnet test Fixture.sln --verbosity normal + expected_exit_code: 0 + timeout: 5m + - type: prompt + rubric: + - Recognizes that tightening a nullable annotation on a shipped public API changes its source contract and is not a behavior-preserving cleanup + - Leaves both the declaration and shipped API record unchanged instead of forcing callers to satisfy a newly non-null contract + - Clearly reclassifies the requested edit as a separately authorized contract change + + - name: Rename one binding without touching same-text symbols + prompt: "Rename only `OrderProcessor.DoStuff` to `CalculateInvoice` and migrate its callers. Do not rename unrelated members or text that happen to contain `DoStuff`. Verify Fixture.sln." + environment: *fixture-environment + graders: + - type: file-contains + config: + path: src/Billing/OrderProcessor.cs + value: CalculateInvoice + - type: file-contains + config: + path: tests/Billing.Tests/BillingTests.cs + value: CalculateInvoice + - type: file-contains + config: + path: src/Billing/CollisionExamples.cs + value: internal int DoStuff() + - type: file-contains + config: + path: src/Billing/CollisionExamples.cs + value: DoStuff is shown in migration notes. + - type: run-command + config: + command: dotnet test Fixture.sln --verbosity normal + expected_exit_code: 0 + timeout: 5m + - type: prompt + rubric: + - Renames the requested symbol by binding and updates its callers + - Leaves the unrelated same-named member and the migration-note string unchanged + - Uses the compiler and tests to catch missed references without applying a repository-wide textual replacement + - name: Stay dormant for an ordinary feature request prompt: Add a platinum loyalty tier at 15% off and make it available to orders. expect_activation: false diff --git a/tests/dotnet/csharp-refactoring/src/Billing/CustomerProfile.cs b/tests/dotnet/csharp-refactoring/src/Billing/CustomerProfile.cs new file mode 100644 index 00000000..db8b3da4 --- /dev/null +++ b/tests/dotnet/csharp-refactoring/src/Billing/CustomerProfile.cs @@ -0,0 +1,6 @@ +namespace Billing; + +internal sealed class CustomerProfile +{ + public string LoyaltyLevel { get; init; } = ""; +} diff --git a/tests/dotnet/csharp-refactoring/tests/Billing.Tests/BillingTests.cs b/tests/dotnet/csharp-refactoring/tests/Billing.Tests/BillingTests.cs index 16b686ca..8d922f11 100644 --- a/tests/dotnet/csharp-refactoring/tests/Billing.Tests/BillingTests.cs +++ b/tests/dotnet/csharp-refactoring/tests/Billing.Tests/BillingTests.cs @@ -1,4 +1,5 @@ using Billing; +using System.Text.Json; using Xunit; namespace Billing.Tests; @@ -107,7 +108,9 @@ public class BillingTests public void ConfigReader_MatchesAppSettingsHelper() { Assert.Equal(AppSettingsHelper.ParseIntSetting("10", 0), ConfigReader.ReadInt("10", 0)); + Assert.Equal(AppSettingsHelper.ParseIntSetting("bad", 7), ConfigReader.ReadInt("bad", 7)); Assert.Equal(AppSettingsHelper.ParseBoolSetting("true", false), ConfigReader.ReadBool("true", false)); + Assert.Equal(AppSettingsHelper.ParseBoolSetting("bad", true), ConfigReader.ReadBool("bad", true)); } [Fact] @@ -124,4 +127,12 @@ public class BillingTests { Assert.Equal(90m, new Coupons().RedeemDefault(100m)); } + + [Fact] + public void CustomerProfile_PreservesSerializedContract() + { + var profile = new CustomerProfile { LoyaltyLevel = "gold" }; + + Assert.Equal("""{"LoyaltyLevel":"gold"}""", JsonSerializer.Serialize(profile)); + } }