Close the remaining dotnet-test eval follow-ups (#899)

eng/eval-quality/README.md

@@ -16,8 +16,8 @@ python eng/eval-quality/selftest_eval_quality.py       # prove the gate still fi
 
 ## Failing checks
 
-All eight are **structural** — they inspect file existence, git state, declared
-numbers, or YAML keys. None of them interprets prose, so they cannot fire
+All nine are **structural** — they inspect file existence, git state, declared
+numbers, or YAML shape/keys. None of them interprets prose, so they cannot fire
 spuriously on a well-written eval.
 
 ### 1. Referenced fixture missing on disk
@@ -203,6 +203,42 @@ grandfathered eval is not treated as growth. `agent.*` evals are exempt
 outright: the experiment's `evals:` glob excludes them, so no verdict is ever
 computed and the floor has nothing to protect.
 
+### 9. Duplicate key in a mapping
+
+`yaml.safe_load` accepts duplicate keys silently and keeps the **last** one. So
+a stray second `prompt:` / `environment:` / `graders:` / `rubric:` block — the
+tail an edit left behind when it moved a scenario — lands inside whichever
+stimulus follows it and overwrites *that stimulus's own values*, field by field.
+
+The result is the worst shape a defect can take here: the spec parses, the
+scenario count is exactly what the author intended, and one scenario is a
+byte-identical rerun of another. It runs the wrong prompt against the wrong
+fixture, and the discriminator it was added for does not exist.
+
+Observed live in #971. `grade-tests` was raised from 4 to 5 scenarios to clear
+the trial floor, and the new "production code available" scenario shipped as a
+silent clone of the "production code unavailable" one:
+
+```yaml
+  - name: Grade C# tests with the production code available
+    prompt: |            # <- overwritten
+      ...
+    constraints:
+      reject_tools: [edit, create]
+    prompt: |            # <- leftover tail; this is the one that survives
+      ...Payments.Tests/PaymentGatewayTests.cs...
+```
+
+`yaml.safe_load(...)` returned 5 stimuli with the 5 expected `name:` values, and
+`dotnet-production-available/` — a fixture built for the scenario — was never
+loaded. Validating a spec by parsing it and counting scenarios, which is what
+the PR had done, cannot see this. Only the parser can, so the gate uses a loader
+that refuses duplicate keys and reports both line numbers.
+
+Fix it by deleting the stray block. Check it really is stray first: compare it
+against the scenario it duplicates before removing it, so a genuinely distinct
+scenario that merely lost its `- name:` line is restored rather than dropped.
+
 ## Why the gate scores direction, not magnitude
 
 Worth recording, because the check above is only half of what went wrong.
@@ -268,6 +304,24 @@ fixtures beside it.
 A skill that ships with `SKILL.md` but has no `tests/<plugin>/<skill>/eval.yaml`
 carries zero evidence of impact.
 
+**Reference skills are reported separately.** A skill whose frontmatter sets
+`disable-model-invocation: true` is dropped from the Copilot CLI's
+`<available_skills>` menu, so the model cannot reach it from a user prompt — a
+consumer skill or agent loads it by name. The experiment's `skilled` variant
+loads exactly one skill (`plugins/${eval.grandparent}/skills/${eval.parent}`),
+so a direct-activation eval for one of these would run an arm the model can
+never invoke: treatment equals control by construction and the head-to-head
+score is judge noise. That is the same defect failing check 7 exists to prevent,
+and adding such an eval would make the number worse, not better.
+
+The honest coverage for these is **dependency-level**: they are exercised
+through the evals of the skills that load them (for example `run-tests` and
+`mtp-hot-reload` for `filter-syntax` and `platform-detection`, the polyglot
+analysis skills for `test-analysis-extensions`, and `code-testing-agent` for
+`code-testing-extensions`), and in the plugin arm, where the whole plugin is
+loaded. Closing this properly needs harness support for declaring a dependency
+in the skilled variant, not a per-skill eval file.
+
 ### Dormancy guard without an anti-hijack rubric item
 
 Once `reject_skills` is removed the skill loads, so the judge scores the guard

eng/eval-quality/check_eval_quality.py

@@ -34,6 +34,10 @@
      exceed counted trials, so below five no possible record produces a pass —
      the eval cannot answer the question it exists to answer. Existing evals are
      grandfathered through a shrink-only allowlist.
+  9. Duplicate key in a mapping. YAML keeps the last one, so a stray second
+     `prompt:`/`environment:`/`graders:` block silently overwrites the scenario
+     it lands in, turning it into a clone of another. Scenario counts still look
+     right, which is why only the parser can catch it.
 
 Every failing check above is structural — it inspects file existence, git
 state, declared numbers, or YAML shape/keys — so it cannot fire spuriously on
@@ -100,6 +104,49 @@
 warnings: list[str] = []
 
 
+class NoDuplicateKeys(yaml.SafeLoader):
+    """SafeLoader that refuses duplicate keys in a mapping.
+
+    `yaml.safe_load` accepts them silently and keeps the **last** one, so a
+    stimulus that accidentally carries a second `prompt:`/`environment:`/
+    `graders:`/`rubric:` block parses cleanly while every one of its own values
+    is overwritten by the stray copy. The spec then still reports the right
+    number of scenarios, but one of them is a clone of another: it runs the
+    wrong prompt against the wrong fixture, and the discriminator it was added
+    for does not exist.
+
+    Observed live on this repo: an edit to `grade-tests` left the tail of the
+    scenario it had moved sitting after the next `constraints:` block. The spec
+    parsed, `len(doc["stimuli"])` was the expected 5, and the new
+    "production code available" scenario was silently a byte-identical rerun of
+    the "production code unavailable" one — the fixture it was built around was
+    never loaded. Counting scenarios cannot see this; only the parser can.
+    """
+
+
+def _mapping_without_duplicates(loader, node, deep=False):
+    loader.flatten_mapping(node)
+    seen: dict[object, int] = {}
+    mapping = {}
+    for key_node, value_node in node.value:
+        key = loader.construct_object(key_node, deep=deep)
+        if key in seen:
+            raise yaml.constructor.ConstructorError(
+                None, None,
+                f"duplicate key {key!r} (first at line {seen[key]}, again at line "
+                f"{key_node.start_mark.line + 1}). YAML keeps the last one, so the "
+                f"earlier value is silently discarded — usually a leftover block "
+                f"from an edit that makes one scenario a clone of another",
+                node.start_mark)
+        seen[key] = key_node.start_mark.line + 1
+        mapping[key] = loader.construct_object(value_node, deep=deep)
+    return mapping
+
+
+NoDuplicateKeys.add_constructor(
+    yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, _mapping_without_duplicates)
+
+
 def git_tracked_files() -> set[str]:
     # `git ls-files` reports the index, which already includes newly staged
     # additions. Unioning in `git diff --cached --name-only` as well looked
@@ -440,19 +487,53 @@ def report_orphans(specs: list[str]) -> None:
         warnings.extend(f"    {f}" for f in found)
 
 
+def _is_reference_skill(skill_dir: str) -> bool:
+    """True when a skill is deliberately hidden from the model-facing menu.
+
+    `disable-model-invocation: true` drops the skill from the Copilot CLI's
+    `<available_skills>` menu, so the model cannot invoke it from a user prompt
+    — it is loaded by name from a consumer skill or agent instead. The
+    experiment's `skilled` variant loads exactly one skill, so a
+    direct-activation eval for such a skill would run an arm the model can
+    never reach: treatment equals control by construction and the head-to-head
+    score is judge noise, the same defect failing check 7 exists to prevent.
+    They are exercised through the evals of the skills that load them.
+    """
+    path = os.path.join(skill_dir, "SKILL.md")
+    try:
+        with open(path, encoding="utf-8") as fh:
+            head = fh.read(4000)
+    except OSError:
+        return False
+    front = head.split("\n---", 1)[0] if head.startswith("---") else ""
+    return re.search(r"^disable-model-invocation:\s*true\s*$", front, re.M) is not None
+
+
 def report_uncovered() -> None:
     missing = []
+    reference = []
     for plugin_dir in sorted(glob.glob("plugins/*")):
         plugin = os.path.basename(plugin_dir)
         evals = {os.path.basename(os.path.dirname(f))
                  for f in glob.glob(f"tests/{plugin}/*/eval.yaml")}
         for skill_dir in sorted(glob.glob(f"{plugin_dir}/skills/*")):
             skill = os.path.basename(skill_dir)
-            if os.path.isdir(skill_dir) and skill not in evals:
+            if not os.path.isdir(skill_dir) or skill in evals:
+                continue
+            if _is_reference_skill(skill_dir):
+                reference.append(f"    {plugin}/{skill}")
+            else:
                 missing.append(f"    {plugin}/{skill}")
     if missing:
         warnings.append(f"{len(missing)} skill(s) have no eval at all:")
         warnings.extend(missing)
+    if reference:
+        warnings.append(
+            f"{len(reference)} reference skill(s) have no eval — they set "
+            f"`disable-model-invocation: true`, so a direct-activation eval would "
+            f"compare two identical arms. Cover them through the consumers that "
+            f"load them:")
+        warnings.extend(reference)
 
 
 def check_floor_agreement() -> None:
@@ -506,7 +587,7 @@ def main() -> int:
     for spec in specs:
         try:
             with open(spec, encoding="utf-8") as fh:
-                doc = yaml.safe_load(fh) or {}
+                doc = yaml.load(fh, NoDuplicateKeys) or {}
         except yaml.YAMLError as exc:
             errors.append(f"{spec}: YAML parse error: {exc}")
             continue

eng/eval-quality/selftest_eval_quality.py

@@ -185,6 +185,22 @@ def empty_grader_config(d):
         )
 
 
+def duplicate_stimulus_keys(d):
+    # A leftover block from an edit lands inside the stimulus that follows it,
+    # duplicating `prompt:` and `rubric:` at the same mapping level. YAML keeps
+    # the LAST value, so the scenario silently runs someone else's prompt while
+    # `len(doc["stimuli"])` is unchanged — counting scenarios cannot see this,
+    # which is why the gate has to reject it at parse time. Cost a real scenario
+    # in #971: `grade-tests` shipped a "production code available" case that was
+    # a byte-identical rerun of the "production code unavailable" one.
+    with open(EV(d), "a") as f:
+        f.write(
+            "    prompt: a stray prompt from an earlier scenario\n"
+            "    rubric:\n"
+            "      - A stray rubric item\n"
+        )
+
+
 def grandfathered_reports_its_arithmetic(d):
     # The gate's job for a grandfathered eval is to tell the contributor what to
     # change, so the reported figure must be the trial arithmetic and not just
@@ -333,6 +349,7 @@ def unresolvable_base_ref(d):
     case("Cobertura file totals contradict file line-rate", inconsistent_file_totals, expect_fail=True),
     case("Cobertura aggregate rate contradicts its payload", aggregate_contradicts_payload, expect_fail=True),
     case("grader with an empty config enforces nothing", empty_grader_config, expect_fail=True),
+    case("duplicate key silently overwrites a scenario", duplicate_stimulus_keys, expect_fail=True),
     case("dormancy guard also sets reject_skills", guard_with_reject_skills, expect_fail=True),
     case("well-formed dormancy guard", guard_ok, expect_fail=False),
     case("eval below the trial floor", underpowered, expect_fail=True),

eng/eval-quality/underpowered-allowlist.txt

@@ -63,22 +63,13 @@ tests/dotnet-msbuild/item-management/eval.yaml
 tests/dotnet-msbuild/property-patterns/eval.yaml
 tests/dotnet-msbuild/target-authoring/eval.yaml
 tests/dotnet-template-engine/template-comparison/eval.yaml
-tests/dotnet-test/code-testing-agent/eval.yaml
-tests/dotnet-test/coverage-analysis/eval.yaml
 tests/dotnet-upgrade/migrate-nullable-references/eval.yaml
 tests/dotnet/setup-local-sdk/eval.yaml
 tests/dotnet11/system-text-json-net11/eval.yaml
 tests/dotnet-blazor/use-js-interop/eval.yaml
 tests/dotnet-experimental/exp-test-maintainability/eval.yaml
 tests/dotnet-maui/maui-app-lifecycle/eval.yaml
-tests/dotnet-maui/maui-collectionview/eval.yaml
 tests/dotnet-maui/maui-data-binding/eval.yaml
-tests/dotnet-maui/maui-dependency-injection/eval.yaml
 tests/dotnet-maui/maui-safe-area/eval.yaml
-tests/dotnet-maui/maui-shell-navigation/eval.yaml
-tests/dotnet-maui/maui-theming/eval.yaml
 tests/dotnet-msbuild/msbuild-antipatterns/eval.yaml
 tests/dotnet-template-engine/template-smart-defaults/eval.yaml
-tests/dotnet-test/find-untested-sources/eval.yaml
-tests/dotnet-test/generate-testability-wrappers/eval.yaml
-tests/dotnet-test/grade-tests/eval.yaml

plugins/dotnet-test/README.md

@@ -72,6 +72,16 @@ For non-.NET languages, use the native coverage tool: `coverage.py`/`pytest-cov`
 | **platform-detection** *(.NET)* | Detect VSTest vs MTP and identify the test framework from project files |
 | **filter-syntax** *(.NET)* | Test filter syntax reference for VSTest and MTP across all frameworks |
 
+These four set `disable-model-invocation: true`, so the CLI keeps them out of the
+model-facing skill menu and a consumer loads them by name. They deliberately have
+no `tests/dotnet-test/<skill>/eval.yaml`: the experiment's skilled arm loads a
+single skill, which the model could never invoke here, so such an eval would
+compare two identical arms and score judge noise. They are measured through the
+evals of the skills that load them — `run-tests` and `mtp-hot-reload` for
+`platform-detection` and `filter-syntax`, the polyglot analysis skills and
+`grade-tests` for `test-analysis-extensions`, and `code-testing-agent` for
+`code-testing-extensions`. See `eng/eval-quality/README.md`.
+
 ## Agents
 
 ### User-facing agents

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

@@ -77,7 +77,21 @@ This skill coordinates multiple specialized agents in a **Research → Plan →
 Make sure you understand what user is asking and for what scope.
 When the user does not express strong requirements for test style, coverage goals, or conventions, source the guidelines from [unit-test-generation.prompt.md](unit-test-generation.prompt.md). This prompt provides best practices for discovering conventions, parameterization strategies, coverage goals (aim for 80%), and language-specific patterns.
 
-### Step 2: Invoke the Test Generator
+### Step 2: Size the request before invoking anything
+
+Match the machinery to the scope. Running the full pipeline on a one-file
+request costs turns and tool calls without improving the tests.
+
+| Scope | What it looks like | How to run it |
+| --- | --- | --- |
+| **Focused** | One function, class, or file; "tests for X only"; extending an existing suite with the missing cases | Skip the `.testagent/` artifacts and the sub-agent fan-out. Keep the requirement checklist in your head (or in the final table), read only the target and one neighbouring test for conventions, write the tests, run the narrowest test command, review your own assertions inline. |
+| **Broad** | A project, package, or module set; "comprehensive suite"; a coverage threshold to clear across several files | Run the full Research → Plan → Implement pipeline in Step 3, with the `.testagent/` artifacts and the completion contract below. |
+
+When in doubt, start focused and escalate only if the request turns out to span
+several files. Escalating costs one extra pass; running the broad pipeline on a
+focused request costs several.
+
+### Step 3: Invoke the Test Generator (broad scope)
 
 Start by calling the `code-testing-generator` agent with your test generation request:
 
@@ -91,7 +105,7 @@ If `code-testing-generator` is unavailable, do not skip the workflow. Execute th
 same Research → Plan → Implement sequence inline, create the `.testagent/`
 artifacts described below, and apply the same completion contract.
 
-### Step 3: Execute with bounded context
+### Step 4: Execute with bounded context
 
 For multi-file requests:
 
@@ -106,12 +120,16 @@ For multi-file requests:
 
 ### Completion contract
 
+Every scope must satisfy points 3–5 below. Points 1 and 2 are the **broad-scope**
+artifacts: on a focused request the same reasoning happens inline and no
+`.testagent/` files are written.
+
 Do not report completion until all of these are true:
 
-1. `.testagent/research.md` records the bounded target inventory, existing test
-   conventions, and the acceptance checklist.
-2. `.testagent/plan.md` maps each checklist item to a planned test or an explicit
-   blocker.
+1. *(broad scope)* `.testagent/research.md` records the bounded target
+   inventory, existing test conventions, and the acceptance checklist.
+2. *(broad scope)* `.testagent/plan.md` maps each checklist item to a planned
+   test or an explicit blocker.
 3. Generated tests compile and pass with the narrowest relevant test command.
 4. Every explicit user requirement is backed by a concrete test and assertion.
    Fix missing mock seams, boundary cases, state transitions, and property
@@ -121,10 +139,11 @@ Do not report completion until all of these are true:
    blocked. For non-behavioral requirements such as scaffolding, scope limits,
    commands, or coverage artifacts, cite the relevant file, command, or report
    instead of forcing a test-name mapping.
-5. Review the generated tests for behavior gaps and weak assertions. Invoke
-   `test-gap-analysis` and `assertion-quality` when available; otherwise perform
-   the equivalent review inline and record the findings and fixes in
-   `.testagent/status.md`.
+5. Review the generated tests for behavior gaps and weak assertions. On a broad
+   scope, invoke `test-gap-analysis` and `assertion-quality` when available and
+   record the findings and fixes in `.testagent/status.md`. On a focused scope,
+   do the equivalent review inline — re-read each generated assertion against
+   the source — without spawning extra passes.
 
 The final response MUST include a compact `Requirement | Evidence` table.
 Behavioral rows cite exact generated test names. Non-behavioral rows cite the
@@ -147,7 +166,8 @@ never infer threshold clearance from a failed or partial run.
 
 ## State Management
 
-All pipeline state is stored in `.testagent/` folder:
+Broad-scope runs store pipeline state in the `.testagent/` folder. A focused
+request does not create these files:
 
 | File                     | Purpose                      |
 | ------------------------ | ---------------------------- |