Fix migrate-static-to-wrapper skill activation on evals (#864)

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>

plugins/dotnet-test/skills/generate-testability-wrappers/SKILL.md

@@ -1,17 +1,18 @@
 ---
 name: generate-testability-wrappers
 description: >
-  Generate wrapper interfaces and DI registration for hard-to-test static dependencies in C#.
-  Produces IFileSystem, IEnvironmentProvider, IConsole, IProcessRunner wrappers, or guides adoption
-  of TimeProvider and IHttpClientFactory.
+  Generate wrapper interfaces and DI registration for hard-to-test static dependencies in C#,
+  when the abstraction does NOT exist yet. Produces IFileSystem, IEnvironmentProvider, IConsole,
+  IProcessRunner wrappers, or guides first-time adoption of TimeProvider and IHttpClientFactory
+  and registering them in DI.
   USE FOR: generate wrapper for static, create IFileSystem wrapper, wrap DateTime.Now,
   make static testable, make class testable, create abstraction for File.*, generate
-  DI registration, TimeProvider adoption, IHttpClientFactory setup, testability wrapper,
-  mock-friendly interface, mock time in tests, create the right abstraction to mock,
-  how to mock DateTime, test code using File.ReadAllText, what abstraction for Environment,
-  how to make statics injectable, adopt System.IO.Abstractions, make file calls testable.
-  DO NOT USE FOR: detecting statics (use detect-static-dependencies), migrating call
-  sites (use migrate-static-to-wrapper), general interface design not about testability.
+  DI registration, set up/adopt TimeProvider when it is not registered yet, IHttpClientFactory
+  setup, testability wrapper, create the right abstraction to mock, what abstraction for
+  Environment, how to make statics injectable, adopt System.IO.Abstractions.
+  DO NOT USE FOR: detecting statics (use detect-static-dependencies), migrating
+  call sites or replacing existing DateTime.*/File.* usages once the wrapper is created
+  or already registered in DI (use migrate-static-to-wrapper), general interface design.
 license: MIT
 ---
 

plugins/dotnet-test/skills/migrate-static-to-wrapper/SKILL.md

@@ -1,18 +1,19 @@
 ---
 name: migrate-static-to-wrapper
 description: >
-  Mechanically replace static dependency call sites with wrapper or built-in
-  abstraction calls across a bounded scope (file, project, or namespace).
-  Performs codemod-style bulk replacement of DateTime.UtcNow to TimeProvider.GetUtcNow(),
-  File.ReadAllText to IFileSystem, and similar transformations. Adds constructor
-  injection parameters and updates DI registration.
-  USE FOR: replace DateTime.Now/UtcNow with TimeProvider, migrate static calls
-  to wrapper, bulk replace File.* with IFileSystem, codemod static to
-  injectable, add constructor injection for a dependency, mechanical or scoped
-  migration of statics, convert static calls to use an abstraction, update call
-  sites.
-  DO NOT USE FOR: detecting statics (use detect-static-dependencies), generating
-  wrappers (use generate-testability-wrappers), migrating between test frameworks.
+  Replace existing static dependency call sites with wrapper or built-in
+  abstraction calls when the abstraction already exists or is already registered
+  in DI. Codemod-style bulk replacement of DateTime.Now/UtcNow to TimeProvider,
+  File.ReadAllText to IFileSystem, and similar, across a bounded scope (file,
+  project, or namespace). Adds the constructor injection parameter to affected classes.
+  USE FOR: replace all DateTime.UtcNow/DateTime.Now calls with TimeProvider and add
+  the constructor parameter, TimeProvider already registered in DI so migrate the call
+  sites, migrate static calls to wrapper, bulk replace File.* with IFileSystem, codemod
+  static to injectable, add constructor injection for an existing dependency, scoped
+  migration of statics, migrate statics in only certain scoped files.
+  DO NOT USE FOR: detecting statics (use detect-static-dependencies), creating the
+  wrapper or registering it when it does not exist yet (use
+  generate-testability-wrappers), migrating between test frameworks.
 license: MIT
 ---
 
@@ -89,6 +90,39 @@ Add the new dependency following the class's existing pattern:
 - **Primary constructor** (C# 12+): Add parameter to primary constructor: `public class OrderProcessor(ILogger<OrderProcessor> logger, TimeProvider timeProvider)`
 - **Traditional constructor**: Add `private readonly` field + constructor parameter, matching the existing field naming convention (`_camelCase` or `m_camelCase`)
 
+#### Static classes: use ambient context (no constructor injection)
+
+A `static` class with only static members **cannot** receive constructor injection — adding an instance constructor or instance field would break it. Do **not** convert it to a non-static class just to inject the dependency; that changes its design and every call site. Instead, apply the **ambient context** pattern: expose a static, settable seam that defaults to the real implementation and is overridden once at composition/test setup.
+
+```csharp
+public static class TimestampFormatter
+{
+    // Ambient seam — defaults to the real clock, swap in tests.
+    public static TimeProvider Clock { get; set; } = TimeProvider.System;
+
+    public static string Now() => Clock.GetUtcNow().ToString("O");
+}
+```
+
+- Production: leave `Clock` at its `TimeProvider.System` default, or assign the DI-resolved `TimeProvider` once at startup (`TimestampFormatter.Clock = app.Services.GetRequiredService<TimeProvider>();`).
+- Tests: override `Clock` with a `FakeTimeProvider` and **always restore it in a `finally`** so a failing assertion can't leak the fake into other tests:
+
+  ```csharp
+  var original = TimestampFormatter.Clock;
+  TimestampFormatter.Clock = new FakeTimeProvider(instant);
+  try
+  {
+      // exercise code under test
+  }
+  finally
+  {
+      TimestampFormatter.Clock = original;
+  }
+  ```
+
+- **Parallelism caveat**: a mutable static seam is process-global. Tests that mutate it must **not** run in parallel with each other (or with code that reads it) — put them in a non-parallel collection/class (e.g. xUnit `[Collection]` with parallelization disabled, or MSTest `[DoNotParallelize]`). If tests must run in parallel, prefer constructor injection (convert the caller) over an ambient static.
+- The same seam works for other statics (`IFileSystem`, custom wrappers): a `public static <Abstraction> X { get; set; }` defaulting to the real implementation, with the same restore-in-`finally` and non-parallel discipline.
+
 ### Step 4: Replace call sites
 
 Perform each replacement mechanically. For each call site:
@@ -171,7 +205,7 @@ Summarize what was done:
 | Pitfall | Solution |
 |---------|----------|
 | Replacing statics in test code | Only replace in production code; tests should use fakes/mocks |
-| Breaking static classes | Static classes can't have constructors — use ambient context for these |
+| Breaking static classes | Static classes can't have constructors — use the ambient context seam (Step 3) instead of converting them to non-static |
 | Missing `FakeTimeProvider` NuGet | Add `Microsoft.Extensions.TimeProvider.Testing` to test project |
 | Replacing in expression-bodied members without updating return type | `DateTime` → `DateTimeOffset` when using `TimeProvider.GetUtcNow()` — verify type compatibility |
 | Migrating too much at once | Stick to the defined scope — one project or namespace per run |

tests/dotnet-test/migrate-static-to-wrapper/eval.yaml

@@ -1,9 +1,10 @@
 scenarios:
   - name: "Migrate DateTime.UtcNow to TimeProvider in a service class"
     prompt: |
-      I have a CouponService that uses DateTime.UtcNow in several places. TimeProvider
-      is already registered in my DI container. Can you replace all the DateTime.UtcNow
-      calls with TimeProvider and add the constructor parameter?
+      I have a CouponService that uses DateTime.UtcNow and DateTime.Now in
+      several places. TimeProvider is already registered in my DI container.
+      Can you replace all the DateTime calls with TimeProvider and add the
+      constructor parameter?
     setup:
       copy_test_files: true
     assertions:
@@ -13,9 +14,13 @@ scenarios:
       - type: "file_not_contains"
         path: "**/CouponService.cs"
         value: "DateTime.UtcNow"
+      - type: "file_not_contains"
+        path: "**/CouponService.cs"
+        value: "DateTime.Now"
       - type: "exit_success"
     rubric:
-      - "Replaced all DateTime.UtcNow calls in CouponService.cs with TimeProvider calls"
+      - "Replaced all DateTime.Now and DateTime.UtcNow calls in CouponService.cs with TimeProvider calls"
+      - "Preserved local-vs-UTC time semantics — the local-time call (DateTime.Now) maps to a local TimeProvider call and the UTC calls map to a UTC call, rather than conflating them (which would silently change IsRedeemableToday's behavior across time zones)"
       - "Added TimeProvider as a constructor parameter to CouponService"
       - "Stored TimeProvider in a readonly field following the class's existing naming convention"
       - "Added any required `using` directives so the migrated file still compiles"
@@ -27,21 +32,28 @@ scenarios:
 
   - name: "Migrate only in scoped files, leaving others untouched"
     prompt: |
-      I want to migrate DateTime.UtcNow to TimeProvider, but only in CouponService.cs
-      for now. Don't touch AuditLogger.cs yet — I'll do that in a separate pass.
-      TimeProvider is already in my DI container.
+      I want to migrate DateTime.Now and DateTime.UtcNow to TimeProvider, but
+      only in CouponService.cs for now. Don't touch AuditLogger.cs yet — I'll do
+      that in a separate pass. TimeProvider is already in my DI container.
     setup:
       copy_test_files: true
     assertions:
       - type: "file_contains"
         path: "**/CouponService.cs"
         value: "TimeProvider"
+      - type: "file_not_contains"
+        path: "**/CouponService.cs"
+        value: "DateTime.UtcNow"
+      - type: "file_not_contains"
+        path: "**/CouponService.cs"
+        value: "DateTime.Now"
       - type: "file_contains"
         path: "**/AuditLogger.cs"
         value: "DateTime.UtcNow"
       - type: "exit_success"
     rubric:
-      - "Migrated DateTime.UtcNow to TimeProvider in CouponService.cs"
+      - "Migrated DateTime.Now and DateTime.UtcNow to TimeProvider in CouponService.cs"
+      - "Preserved local-vs-UTC time semantics — the local-time call (DateTime.Now) maps to a local TimeProvider call and the UTC calls map to a UTC call, rather than conflating them"
       - "Left AuditLogger.cs completely untouched with DateTime.UtcNow calls still present"
       - "Migrated incrementally rather than too much at once, keeping the change to the single file the user scoped"
       - "Respected the scope boundary the user specified"

tests/dotnet-test/migrate-static-to-wrapper/fixtures/ready-to-migrate/Services/CouponService.cs

@@ -29,6 +29,13 @@ public class CouponService
         return true;
     }
 
+    public bool IsRedeemableToday(Coupon coupon)
+    {
+        // Intentionally uses local-time semantics (DateTime.Now) — redemption is
+        // judged against the storefront's local calendar day, not UTC.
+        return coupon.ExpiresAt.Date >= DateTime.Now.Date;
+    }
+
     public TimeSpan TimeUntilExpiry(Coupon coupon)
     {
         var remaining = coupon.ExpiresAt - DateTime.UtcNow;