Refactor csharp-refactoring skill documentation and evaluation tests for clarity and consistency

plugins/dotnet/skills/csharp-refactoring/SKILL.md

@@ -16,16 +16,22 @@ The most valuable thing this skill does is *not* restructure code you were told
 catching a request that is **not** behavior-preserving before you run it through a refactor's contract.
 When the request changes results, decline the refactor framing and handle it honestly:
 
-- **Framework / NuGet version bump** → not a refactor; redirect to the `dotnet-upgrade` skills.
+- **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) → a feature, not a refactor. If asked,
-  build it as a feature *with its own tests*; don't claim behavior is preserved.
+  build it as a feature *with its own tests* and state that the new behavior is intentional; don't claim
+  the feature itself is behavior-preserving. You can still perform a separable structural cleanup around
+  it, but distinguish the refactor from the feature in both the implementation and the final report.
 - **Bug fix or "simplification" that changes output** (e.g. always charge shipping, bump a discount) →
   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."
+  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 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.
+  → 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.
 
@@ -42,6 +48,13 @@ 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.
+
 ## Verify proportionally
 
 Confirm behavior is preserved after the edit — scaled to blast radius, not a fixed ceremony:

tests/dotnet/csharp-refactoring/eval.yaml

@@ -4,8 +4,8 @@ type: capability
 defaults:
   timeout: 15m
 stimuli:
-  - name: Rename a method across its declaration and every caller
-    prompt: "In the Billing class library, the method `OrderProcessor.DoStuff` is badly named for what it does — it computes an invoice. Rename it to `ComputeInvoice` everywhere it is declared and called, without changing any behavior. The solution is at Fixture.sln."
+  - name: Rename a shipped public method without breaking existing callers
+    prompt: "In the Billing class library, rename the shipped public method `AppSettingsHelper.ParseIntSetting` to `ParseIntegerSetting` and migrate this solution's callers, while preserving compatibility for existing callers of the old public API. `PublicAPI.Shipped.txt` records the shipped surface. The solution is at Fixture.sln."
     environment: &fixture-environment
       files:
         - src: Fixture.sln
@@ -33,82 +33,109 @@ stimuli:
     graders:
       - type: file-contains
         config:
-          path: src/Billing/OrderProcessor.cs
-          value: ComputeInvoice
-      # The rename must propagate to every caller. BillingTests.cs calls the method
-      # directly, so a correct rename lands the new name here too (and the old call
-      # would otherwise fail to compile).
+          path: src/Billing/AppSettingsHelper.cs
+          value: ParseIntegerSetting
       - type: file-contains
         config:
           path: tests/Billing.Tests/BillingTests.cs
-          value: ComputeInvoice
-      # Behavior-preservation + full-propagation gate: if any declaration or caller
-      # was missed, the test project fails to compile and this grader fails.
+          value: ParseIntegerSetting
+      - type: file-contains
+        config:
+          path: src/Billing/AppSettingsHelper.cs
+          value: Obsolete
+      - type: file-contains
+        config:
+          path: src/Billing/AppSettingsHelper.cs
+          value: ParseIntSetting
       - type: run-command
         config:
           command: dotnet test Fixture.sln --verbosity normal
           expected_exit_code: 0
           stdout_contains: "Total tests: 10"
           timeout: 5m
+      - 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:
-      - Renames by tracking true binding references rather than a blind text find-and-replace that would also hit comments, strings, or unrelated test method names
+      - Migrates in-repository callers to ParseIntegerSetting while preserving the shipped ParseIntSetting entry point as an obsolete forwarding compatibility shim
+      - Treats PublicAPI.Shipped.txt as evidence that deleting the old method would break a public contract, rather than blindly removing every old-name occurrence
       - Rebuilds and re-runs the existing tests after the rename to confirm behavior is preserved
-      - Keeps this a single, focused operation without bundling unrelated edits
 
-  - name: Extract a repeated calculation into a private helper
-    prompt: "`OrderProcessor.DoStuff` is too long. Extract the discount-then-tax calculation into a private helper method and call it, without changing what the code computes. The solution is at Fixture.sln."
+  - name: Rename a member declared by a generated partial source
+    prompt: "Rename the private `Coupons.RateFor` helper to `DiscountRateFor` everywhere it is declared and called, preserving behavior. The solution is at Fixture.sln."
     environment: *fixture-environment
     graders:
-      # Behavior-preservation gate: the extracted helper must compute identical
-      # results, so the existing tests must still pass.
+      - type: file-contains
+        config:
+          path: src/Billing/Coupons.g.cs.template
+          value: DiscountRateFor
+      - type: file-contains
+        config:
+          path: src/Billing/Coupons.cs
+          value: DiscountRateFor
+      - type: file-not-contains
+        config:
+          path: src/Billing/Coupons.cs
+          value: amount * RateFor(code)
       - type: run-command
         config:
           command: dotnet test Fixture.sln --verbosity normal
           expected_exit_code: 0
           stdout_contains: "Total tests: 10"
           timeout: 5m
-      # A private helper method now exists (OrderProcessor had none before).
-      - type: file-contains
-        config:
-          path: src/Billing/OrderProcessor.cs
-          value: private decimal
       - type: output-matches
         config:
-          pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green))
+          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:
-      - Extracts the block into a new method preserving the exact same computation and results
-      - Verifies behavior is preserved by building and running the existing tests after extracting
-      - Does not slip a bug fix or behavior change into the extraction
+      - Finds the generated declaration and the hand-authored partial caller as binding-related parts of one rename
+      - Edits Coupons.g.cs.template, the build's source of truth, rather than a generated file under obj
+      - Rebuilds and re-runs the tests so generation occurs again and proves the renamed declaration and caller agree
 
-  - name: Consolidate a duplicated block into a single shared helper
-    prompt: "Inside `OrderProcessor.DoStuff` the discount-plus-tax calculation is duplicated (it appears twice, once for the order total and once for the quote). Consolidate the duplicated logic into a single shared helper used by both, without changing behavior. The solution is at Fixture.sln."
+  - name: Consolidate duplicate public helpers without breaking shipped callers
+    prompt: "`ConfigReader` duplicates the parsing implementation in `AppSettingsHelper`. Consolidate the implementation so there is one source of truth, but preserve the shipped `ConfigReader.ReadInt` and `ReadBool` APIs for existing callers. Migrate this solution's callers where appropriate and verify Fixture.sln."
     environment: *fixture-environment
     graders:
-      # Behavior-preservation gate: the single shared helper must be equivalent to
-      # both original copies, so the existing tests must still pass.
+      - type: file-contains
+        config:
+          path: src/Billing/AppSettingsHelper.cs
+          value: AppSettingsHelper.ParseIntSetting
+      - type: file-contains
+        config:
+          path: src/Billing/AppSettingsHelper.cs
+          value: AppSettingsHelper.ParseBoolSetting
+      - type: file-contains
+        config:
+          path: src/Billing/AppSettingsHelper.cs
+          value: class ConfigReader
+      - type: file-contains
+        config:
+          path: src/Billing/PublicAPI.Shipped.txt
+          value: Billing.ConfigReader
       - type: run-command
         config:
           command: dotnet test Fixture.sln --verbosity normal
           expected_exit_code: 0
           stdout_contains: "Total tests: 10"
           timeout: 5m
-      # A shared private helper now exists (OrderProcessor had none before).
-      - type: file-contains
+      - type: output-matches
         config:
-          path: src/Billing/OrderProcessor.cs
-          value: private decimal
+          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:
-      - Factors the duplicated block into one shared helper that both call sites use
-      - Confirms the consolidated helper is semantically equivalent to each original copy
-      - Re-runs the build and tests after consolidating to prove behavior is unchanged
+      - Removes the duplicate parsing implementation by making ConfigReader delegate to AppSettingsHelper
+      - Preserves the shipped ConfigReader type and methods rather than deleting a public contract during de-duplication
+      - Re-runs the build and tests after consolidating to prove behavior and existing callers remain intact
 
   - name: Inline a pass-through wrapper and update callers
     prompt: "`LegacyTax.ApplyTaxWrapper` does nothing but forward to `TaxRules.Apply`. Inline it: update every caller to call `TaxRules.Apply` directly and remove the wrapper, without changing behavior. The solution is at Fixture.sln."
@@ -159,10 +186,6 @@ stimuli:
         config:
           path: src/Billing/Pricing.cs
           value: class SilverPricing
-      - type: file-contains
-        config:
-          path: src/Billing/Pricing.cs
-          value: class Pricing
       - type: output-matches
         config:
           pattern: (dotnet (build|test)|rebuild|re-?run.{0,20}tests|tests? .{0,20}(pass|green))
@@ -172,7 +195,8 @@ stimuli:
       - Updates all usages and re-runs the build and tests to prove behavior is unchanged
 
   - name: Decline a framework and package upgrade dressed up as a refactor
-    prompt: Upgrade a Billing project to a newer target framework and bump all its NuGet packages to their latest major versions, treating this as a behavior-preserving refactor.
+    prompt: Upgrade the Billing project in Fixture.sln to a newer target framework and bump all its NuGet packages to their latest major versions, treating this as a behavior-preserving refactor.
+    environment: *fixture-environment
     graders:
       - type: output-matches
         config:
@@ -184,7 +208,8 @@ stimuli:
       - Does not silently perform a behavior-changing version bump under the guise of refactoring
 
   - name: Decline a new-feature request dressed up as a refactor
-    prompt: 'Add a new "platinum" loyalty tier at 15% off and wire it into the pricing tiers so orders can use it, treating this as a behavior-preserving refactor.'
+    prompt: 'In Fixture.sln, add a new "platinum" loyalty tier at 15% off and wire it into the pricing tiers so orders can use it, treating this as a behavior-preserving refactor.'
+    environment: *fixture-environment
     graders:
       - type: output-matches
         config:
@@ -196,13 +221,35 @@ stimuli:
       - If it proceeds, does so as an explicit feature addition with its own new tests, not under the guise of a no-behavior-change refactor
 
   - name: Keep behavior-changing bug fixes out of a behavior-preserving refactor
-    prompt: Fix the known bugs first and keep any refactors in a separate follow-up PR. Do not mix behavior-changing bug fixes with refactoring under a behavior-preserving contract.
+    prompt: "In Fixture.sln, rename `OrderProcessor.DoStuff` to `ComputeInvoice` and, while you are there, change the free-shipping threshold from $100 to $50. Treat the whole change as one behavior-preserving refactor."
+    environment: *fixture-environment
     graders:
+      - type: file-contains
+        config:
+          path: src/Billing/OrderProcessor.cs
+          value: ComputeInvoice
+      - type: file-contains
+        config:
+          path: tests/Billing.Tests/BillingTests.cs
+          value: ComputeInvoice
+      - type: file-contains
+        config:
+          path: src/Billing/OrderProcessor.cs
+          value: subtotal >= 100m
       - type: output-matches
         config:
-          pattern: (separate|follow[- ]?up|bug fix|behavior[- ]?(preserving|chang)|not (a )?refactor)
+          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_contains: "Total tests: 10"
+          timeout: 5m
       - type: prompt
     rubric:
-      - Recognizes that a behavior-changing bug fix is distinct from a behavior-preserving refactor
-      - Keeps the bug fix separate from any refactoring rather than bundling them under one behavior-preserving contract
-      - Does not claim behavior is preserved for a change that deliberately alters behavior
+      - Completes the requested rename as a focused behavior-preserving refactor and migrates its callers
+      - 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