Fix dotnet-test findings from the refreshed cross-family eval (#899)

plugins/dotnet-test/skills/code-testing-agent/SKILL.md

@@ -93,7 +93,7 @@ artifacts described below, and apply the same completion contract.
 
 For multi-file requests:
 
-1. Turn every explicit user requirement into a checklist before implementation. Include requested layers, collaborators to mock, boundary cases, integrations, coverage thresholds, and report artifacts.
+1. Turn every explicit user requirement into a checklist before implementation. Include requested layers, collaborators to mock, boundary cases, integrations, coverage thresholds, and report artifacts. Copy multi-condition requirements verbatim — they must each map to one test that exercises the whole combination.
 2. Research only the requested module or project and write the checklist plus a compact target inventory to `.testagent/research.md`.
 3. Reuse manifests, symbol references, and deterministic pairing tools instead of reading every source and test file.
 4. For multi-file scopes in C#, Python, TypeScript/JavaScript, Go, Java, Rust, or Ruby, run `find-untested-sources` once and consume its pairing and suggested-path output; do not repeat that discovery manually.
@@ -129,6 +129,20 @@ Behavioral rows cite exact generated test names. Non-behavioral rows cite the
 relevant project file, validation command, or coverage report. A generic list
 of tested areas is not a substitute for requirement-by-requirement evidence.
 
+**Quote the user's requirement verbatim in each row.** When the request names a
+specific combination — "a case where a composite discount, regional tax, and
+weight-based shipping all apply", "the difference between summed and chained
+discounts", "constructor validation for every class" — the row must cite the one
+test that demonstrates exactly that. A test that merely exercises the same
+collaborators does not satisfy a requirement about their interaction, and
+per-class requirements need a citation per class.
+
+**Cite a clean run, not an attempt.** The commands behind the evidence table must
+have finished successfully: quote the final passing test summary and, when
+thresholds were requested, the per-module coverage table from a run that exited
+0. If the last coverage run exited non-zero, fix it and re-run before reporting;
+never infer threshold clearance from a failed or partial run.
+
 ## State Management
 
 All pipeline state is stored in `.testagent/` folder:

plugins/dotnet-test/skills/coverage-analysis/SKILL.md

@@ -443,11 +443,16 @@ As soon as Phase 3 completes, **your immediately next assistant response must co
 
 The response must include, at minimum:
 
+0. **A direct answer to the question that was actually asked, in the first 2–4 sentences.** For "why is my coverage stuck?" / "what's blocking me?", name the blocking members and the lines involved before any table. The standard sections below still follow.
 1. Overall line and branch coverage — read directly from the `OVERALL_LINE_COVERAGE:` / `OVERALL_BRANCH_COVERAGE:` lines emitted by `Compute-CrapScores.ps1` (no extra Cobertura parsing required)
 2. The Risk Hotspots table built from `Compute-CrapScores.ps1` `HOTSPOTS:` output (CRAP scores, complexity, coverage)
 3. Identification of the highest-risk method(s) and what is blocking coverage
 4. 1–3 prioritized, specific recommendations (which method to test, expected CRAP/coverage impact)
 
+**Every number must come from the script output, and the arithmetic must reconcile.** Uncovered lines attributed to individual members must not exceed the project's total uncovered lines, and the coverage you project after a recommendation must follow from those counts.
+
+**List every member below threshold, not just the worst one.** `Extract-MethodCoverage.ps1` returns the full below-threshold set: name the others even if briefly. Only say "the rest is fine / leave it alone" when that set is otherwise empty — claiming one method is the entire gap when the extractor found more is a factual error.
+
 Use `references/output-format.md` verbatim for fixed headings, table structures, symbols, and emoji. Use `references/guidelines.md` for prioritization rules and style.
 
 If Phase 5 has not yet run when you compose this summary, mark the `## 📁 Reports` section's HTML/Text/CSV/GitHub-markdown rows as `Not generated (optional — request HTML reports to enable)`. Only the `coverage-analysis.md` and raw Cobertura paths are guaranteed to exist.
@@ -531,3 +536,5 @@ After Phase 5 completes successfully, you may follow up with a short message poi
 - **ReportGenerator install failure** — if `dotnet tool install` fails (no internet) during Phase 5, leave the existing Phase 4 summary as the final output and note that HTML reports were skipped. Do not retry or block on the install.
 - **Method name mismatches in Cobertura** — async methods, lambdas, and local functions may have compiler-generated names. The scripts use the Cobertura method name/signature directly; verify against source if results look unexpected.
 - **Mixed coverage providers** — when a solution contains both Coverlet and Microsoft CodeCoverage projects, the skill runs per-project to avoid dual-provider conflicts. This is slower but correct.
+- **Numbers that don't reconcile** — per-member uncovered lines that exceed the project total, or a projected coverage figure that doesn't follow from the counts, make the whole analysis untrustworthy. Re-read the script output rather than estimating.
+- **Declaring one method "the entire gap"** — check the full below-threshold list from `Extract-MethodCoverage.ps1` first; naming a single blocker while other uncovered members exist misdirects the user's next test.

plugins/dotnet-test/skills/crap-score/SKILL.md

@@ -71,6 +71,21 @@ Check the test project's `.csproj` for the coverage package, then run the approp
 | `Microsoft.Testing.Extensions.CodeCoverage` (.NET 9) | `dotnet test -- --coverage --coverage-output-format cobertura --coverage-output ./TestResults` | `--coverage-output` path |
 | `Microsoft.Testing.Extensions.CodeCoverage` (.NET 10+) | `dotnet test --coverage --coverage-output-format cobertura --coverage-output ./TestResults` | `--coverage-output` path |
 
+#### Never estimate coverage
+
+**Guessed coverage produces wrong CRAP scores, which is worse than no answer.** If the first command yields no Cobertura XML, work down this list before giving up:
+
+1. Add a provider if none is referenced: `dotnet add <test.csproj> package coverlet.collector`, then re-run.
+2. Use the standalone collector, which works even when the test host or a shared assembly blocks the in-proc collector:
+   `dotnet tool install --global dotnet-coverage` then
+   `dotnet-coverage collect -f cobertura -o coverage.cobertura.xml "dotnet test <test.csproj>"`.
+3. Convert or summarize an existing report with ReportGenerator when only binary `.coverage` output exists:
+   `dotnet tool install --global dotnet-reportgenerator-globaltool` then
+   `reportgenerator -reports:<file> -targetdir:cov -reporttypes:Cobertura`.
+4. Tests fail but still run? Coverage is collected from the tests that executed — continue with that data and note the failures.
+
+If every path fails, **report that coverage could not be collected, show the commands you tried and their errors, and stop.** Report complexity on its own if useful, but never publish a CRAP number derived from an assumed coverage percentage.
+
 ### Step 2: Compute cyclomatic complexity
 
 Analyze the target source files to determine cyclomatic complexity per method. Count the following decision points (each adds 1 to the base complexity of 1):
@@ -146,12 +161,16 @@ Report this as: "To bring `ProcessOrder` (complexity 12) below CRAP 15, increase
 ## Validation
 
 - Verify that coverage data was collected successfully (Cobertura XML exists and contains data)
+- Confirm every coverage figure came from that XML — no estimated, assumed, or source-comment-derived values
 - Cross-check that method names in coverage data match the source code
 - Confirm CRAP scores by spot-checking the formula on one method manually
 - Ensure a 100%-covered method's CRAP equals its complexity exactly
 
 ## Common Pitfalls
 
+- **Estimating coverage when collection fails**: never do it — the resulting CRAP scores are wrong in the direction that matters. Work through the fallbacks in Step 1, then report the blocker instead.
+- **Trusting a stale complexity comment in the source**: compute cyclomatic complexity from the current code; a `// complexity: 7` comment left by a previous author is not evidence.
+- **Giving up on a shared-assembly or test-host collector error**: `dotnet-coverage collect` runs out of process and usually succeeds where the in-proc collector fails.
 - **Stale coverage data**: Always regenerate coverage before computing CRAP scores. Old coverage files will produce misleading results.
 - **Method name mismatches**: Cobertura XML may use mangled/compiler-generated names for async methods, lambdas, or local functions. Match by line ranges when names don't align.
 - **Generated code**: Exclude auto-generated files (e.g., `*.Designer.cs`, `*.g.cs`) from analysis unless explicitly requested.

plugins/dotnet-test/skills/detect-static-dependencies/SKILL.md

@@ -64,15 +64,28 @@ Scan each file for calls matching these categories:
 | Category | Patterns to search for | Recommended replacement |
 |----------|----------------------|------------------------|
 | **Time** | `DateTime.Now`, `DateTime.UtcNow`, `DateTime.Today`, `DateTimeOffset.Now`, `DateTimeOffset.UtcNow`, `Task.Delay(`, `new CancellationTokenSource(TimeSpan` | `TimeProvider` (.NET 8+) |
-| **File System** | `File.ReadAllText(`, `File.WriteAllText(`, `File.Exists(`, `File.Delete(`, `File.Copy(`, `File.Move(`, `Directory.Exists(`, `Directory.CreateDirectory(`, `Directory.GetFiles(`, `Directory.Delete(`, `Path.Combine(`, `Path.GetTempPath(` | `IFileSystem` (System.IO.Abstractions NuGet) |
+| **File System** | `File.ReadAllText(`, `File.WriteAllText(`, `File.Exists(`, `File.Delete(`, `File.Copy(`, `File.Move(`, `Directory.Exists(`, `Directory.CreateDirectory(`, `Directory.GetFiles(`, `Directory.Delete(`, `Path.GetTempPath(`, and instance members that hit the disk (`new FileInfo(...)`, `new DirectoryInfo(...)`, `.LastWriteTimeUtc`, `new StreamReader(path)`) | `IFileSystem` (System.IO.Abstractions NuGet) |
+| **Randomness / identity** | `new Random(`, `Random.Shared`, `Guid.NewGuid(` | `TimeProvider`-style seam: inject `Random` / an `IGuidProvider` |
+| **Culture / serialization** | `CultureInfo.CurrentCulture`, `CultureInfo.CurrentUICulture`, `JsonSerializer.Serialize(`, `JsonSerializer.Deserialize(` | Pass culture/options explicitly, or inject a serializer abstraction |
 | **Environment** | `Environment.GetEnvironmentVariable(`, `Environment.SetEnvironmentVariable(`, `Environment.MachineName`, `Environment.UserName`, `Environment.CurrentDirectory`, `Environment.Exit(` | Custom `IEnvironmentProvider` |
 | **Network** | `new HttpClient(`, `HttpClient.GetAsync(`, `HttpClient.PostAsync(`, `HttpClient.SendAsync(` | `IHttpClientFactory` (built-in) |
 | **Console** | `Console.WriteLine(`, `Console.ReadLine(`, `Console.Write(`, `Console.ReadKey(` | `IConsole` wrapper or `ILogger` |
 | **Process** | `Process.Start(`, `Process.GetCurrentProcess(`, `Process.GetProcessesByName(` | Custom `IProcessRunner` |
 
 ### Step 3: Aggregate and rank results
 
-Count each static call pattern across the entire scan scope. Produce a summary with:
+Count each static call pattern across the entire scan scope.
+
+**Counting rules — inaccurate totals are the main way this report loses to an ad-hoc scan:**
+
+- **One authoritative total.** Every call site you found belongs in the category summary and the grand total. Never park real findings in an "additional observations" section that the totals exclude.
+- **Classify by what the member touches, not by whether it is `static`.** Instance members that reach the same untestable resource still count and belong in the matching category (`new FileInfo(path).LastWriteTimeUtc` → File System; `httpClient.GetAsync(...)` → Network). Say "hidden dependency", not "static", when the member is an instance call.
+- **Exclude deterministic pure helpers from the "needs wrapping" total.** `Path.Combine`, `Path.GetExtension`, `Path.GetFileName`, and `Math.*`/`string.*` statics take no ambient input and are trivially testable. List them, if at all, in a separate "no action needed" note — never as testability blockers.
+- **Cover every category before reporting** — time, file system, environment, network, console, process, randomness (`new Random()`, `Guid.NewGuid()`), culture (`CultureInfo.CurrentCulture`), and serialization/statics such as `JsonSerializer`. Omitting a category that is present is an under-count.
+- **Give `file:line` for every occurrence** so the user can jump straight to it.
+- **Reconcile before publishing.** The category totals, the top-patterns table, and the per-file table must sum to the same grand total.
+
+Produce a summary with:
 
 1. **Category summary** — total call sites per category (time, filesystem, env, etc.)
 2. **Top patterns** — the 10 most frequent individual patterns ranked by count
@@ -125,15 +138,18 @@ Format the output as a structured report:
 
 ### Step 5: Suggest next steps
 
-Based on the report, recommend:
-- Which category to tackle first (fewest dependencies, best built-in support)
-- Whether to use `generate-testability-wrappers` for custom wrapper generation
-- Whether to use `migrate-static-to-wrapper` for mechanical bulk migration
+Based on the report, recommend which category to tackle first (highest count, best built-in support). Keep this to a few lines.
+
+Mention `generate-testability-wrappers` or `migrate-static-to-wrapper` only when the user's next action clearly needs them — a hand-off note, not a sales pitch. Never end an audit with promotional next-steps that dilute the findings.
 
 ## Validation
 
 - [ ] All `.cs` files in scope were scanned (check count)
 - [ ] Report includes category totals, top patterns, and affected files
+- [ ] Category totals, top patterns, and per-file counts reconcile to the same grand total
+- [ ] Every occurrence carries a `file:line` location
+- [ ] No findings are held outside the totals in an "additional" section
+- [ ] Deterministic pure helpers (`Path.Combine`, `Math.*`) are not counted as testability blockers
 - [ ] Each detected pattern has a recommended replacement listed
 - [ ] `obj/` and `bin/` directories were excluded
 - [ ] Migration priority is ordered by impact (count × ease of replacement)
@@ -147,3 +163,6 @@ Based on the report, recommend:
 | Missing statics inside lambdas/LINQ | Search covers all code within `.cs` files, including lambdas |
 | Recommending `TimeProvider` on < .NET 8 | Check `TargetFramework` in `.csproj` — if < net8.0, recommend `NodaTime.IClock` or custom `ISystemClock` |
 | Ignoring test projects | Only scan production code — exclude `*.Tests.csproj` projects from the scan |
+| Under-counting by relegating findings | Real call sites belong in the category totals, not in a trailing "also noticed" paragraph that the totals ignore |
+| Calling an instance member a static | `new FileInfo(p).LastWriteTimeUtc` is an instance call but still a hidden file-system dependency — count it under File System and describe it accurately |
+| Recommending a wrapper for `Path.Combine` | Pure, deterministic helpers need no seam; listing them as blockers makes the recommendations wrong |

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

@@ -61,6 +61,8 @@ Before modifying any code:
 
 ### Step 2: Plan the migration for each file
 
+**Migrate exactly what was asked — nothing adjacent.** If the user named a member (`DateTime.UtcNow`), migrate only that member and leave siblings such as `DateTime.Now` untouched. If the user named files, do not touch other files. Never migrate a call site whose comment or name marks it as deliberate (e.g. `// intentional local time`). List everything you deliberately left alone under "Remaining (out of scope)" so the user can ask for it in a follow-up; suggesting is fine, silently widening the scope is not.
+
 For each file containing the static pattern, determine:
 
 1. **Which class(es) contain the call sites** — identify the class declarations
@@ -171,6 +173,8 @@ After all changes in the current scope:
 dotnet build <project.csproj>
 ```
 
+**Report the build result you actually observed.** Only write "build succeeded" when the command exited 0; if it failed — including restore/NuGet failures such as "assets file not found" — say so, quote the error, and either fix it (`dotnet restore`, add the missing package) or hand the user a precise blocker. A false success claim is worse than an unfinished migration.
+
 If the build fails:
 - **Missing using**: Add the required `using` directive
 - **Missing NuGet package**: Run `dotnet add package <name>`
@@ -205,11 +209,13 @@ Summarize what was done:
 ## Validation
 
 - [ ] All call sites in scope were replaced (none missed)
+- [ ] No call site outside the requested member/file scope was modified
+- [ ] Call sites documented as intentional (e.g. local time) were left untouched and reported
 - [ ] Constructor injection added to all affected classes
 - [ ] Field naming follows existing class conventions
 - [ ] Required `using` directives added
 - [ ] Required NuGet packages referenced
-- [ ] Build succeeds after migration
+- [ ] Build succeeds after migration, and the reported result matches the actual command exit code
 - [ ] Test files updated with appropriate test doubles
 - [ ] No behavioral changes introduced (wrapper delegates directly to the static)
 - [ ] `DateTimeKind` preserved — former `DateTime.UtcNow` stays `Utc` (`.UtcDateTime`), former `DateTime.Now` stays `Local` (`.LocalDateTime`)
@@ -223,4 +229,6 @@ Summarize what was done:
 | Missing `FakeTimeProvider` NuGet | Add `Microsoft.Extensions.TimeProvider.Testing` to test project |
 | Replacing a `DateTime` value with `.DateTime` off a `DateTimeOffset` | `DateTimeOffset.DateTime` returns `Kind == Unspecified` — use `.UtcDateTime` (for former `DateTime.UtcNow`) or `.LocalDateTime` (for former `DateTime.Now`) to preserve the original `DateTimeKind`. Only change the field/return type to `DateTimeOffset` if the user asked for it. |
 | Migrating too much at once | Stick to the defined scope — one project or namespace per run |
+| Migrating `DateTime.Now` when only `UtcNow` was requested | Respect the literal request; list the other call sites as out-of-scope suggestions instead of rewriting them |
+| Claiming "Build succeeded" after a failed restore | Read the exit code and output; report the real failure and fix it or surface it as a blocker |
 | Forgetting DI registration | Always verify `Program.cs`/`Startup.cs` has the registration before replacing call sites |