Merge pull request #2261 from modelcontextprotocol/v2/fix/2247-testing-handoff

Reshape the testing → test-servers pointer into an imperative step

.claude/skills/testing/SKILL.md

@@ -11,11 +11,56 @@ statements, functions, and branches.** That rule and the React/Mantine
 conventions live in [`AGENTS.md`](../../../AGENTS.md); this skill is where a
 test goes, how to run it, and how to clear the gate.
 
-⚠️ **Anything that needs a real server to run against — an integration test, a
-smoke, reproducing a bug by hand — is `/test-servers`, and you have to load it.**
-Integration and smoke tests here drive a real server over a real transport
-rather than a mock, so picking, building and connecting to a fixture is a
-procedure of its own that this skill does not carry.
+## Before you write it: does the test need a real server?
+
+**If it does, load the `test-servers` skill now — that is step one, before
+choosing a location or writing a line.**
+
+The condition is **"does this test depend on a fixture from `test-servers/`?"**
+— not which tier it lands in, and not which directory it lands in. There are two
+ways to depend on one, and they need different halves of that skill:
+
+- **It connects to a fixture.** An integration test that connects; an
+  end-to-end test that connects; a smoke that drives a connected flow; a
+  coverage gap only reachable over a real connection; reproducing a reported bug
+  against a server. These need the whole procedure — which showcase config,
+  which protocol era, and the staleness hazard.
+  ⚠️ **Connecting is a strong hint, not the rule.** A few integration tests
+  deliberately hand-roll a JSON-RPC server because the composable fixture
+  *cannot* produce what they assert on — `inspectorClient-malformed-list.test.ts`
+  and `listSalvage-era.test.ts` need wire shapes the SDK's own server refuses to
+  emit. Real transport, real client, no `test-servers/` dependency. Check
+  whether a fixture can express the case before reaching for one.
+- **It names or runs the built fixture without connecting.** `smoke:tui` boots
+  the TUI against a catalog whose stdio command *is* the built fixture, then
+  asserts it survives. No transport is driven and no protocol era applies, but
+  the **build and staleness** half lands on it in full.
+
+⚠️ **"A build ran" is not the dependency — using the artefact is.**
+`clients/web`'s `pretest` runs `test-servers:build` before *every* unit run, so
+the fixture is on disk for tests that never reference it. What counts is whether
+the test imports, spawns, or points a config at it.
+
+So the condition does **not** hold when the test renders a component from
+fixture props, exercises a pure function or a parser, or is a smoke that touches
+no fixture — `smoke:launcher` checks `--help`, and `smoke:web` /
+`smoke:web:browser` only assert the SPA is served and paints.
+
+⚠️ **Neither the tier nor the folder decides this.** `src/test/integration/`
+holds `storage/store-id.test.ts`, which validates a string, and `mcp/import/*`,
+which parses config files, right beside the tests that drive a live connection.
+They sit there for the node env and the 30s timeout, not because they connect —
+placement is the project manifest, so it cannot also be the fixture trigger.
+Ask what the test *does*, not where it lives.
+
+**In the connecting case**, the test drives a **real server over a real
+transport, never a mock**, and picking the fixture, building it, and connecting
+with the right protocol era is a procedure this skill does not carry. Writing
+one without `test-servers` means hand-rolling a fixture that already exists, or
+mocking the thing the tier exists to avoid mocking. **In the build-only case**,
+none of the transport or protocol-era guidance applies — what you need from
+`test-servers` is how to build the fixture and why a stale build keeps serving
+old code.
 
 ## Where the test file goes
 
@@ -38,7 +83,12 @@ web-owned test living under `src/test/` instead is a bug.
    `core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`,
    `auth/node/`, `storage/`). **Placement is the manifest** — any file under that
    folder is picked up by the integration project (node env, 30s timeouts) via a
-   folder glob; there is no enumeration to keep in sync.
+   folder glob; there is no enumeration to keep in sync. ⚠️ Placement is *not*
+   the fixture trigger, though — this folder holds pure parser and storage tests
+   alongside the connecting ones. If the test you are adding here **needs a
+   fixture from `test-servers/`, load that skill first**; the fixture is half of
+   that test. Connecting is a strong hint but not the rule — see the
+   hand-rolled-server exception above.
 3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`,
    `fixtures/`, `scrollAreaStoryAssertions.ts`.
 
@@ -82,6 +132,20 @@ spawns the built binary) → smokes through the built launcher (`npm run smoke`)
 Storybook play functions (`test:storybook`) → the published-tarball check
 (`npm run pack:verify`, local/release only — needs network).
 
+⚠️ **Depth in that list is not the fixture boundary, and the boundary cuts
+across the tiers rather than along them.** Needing `test-servers/`: the web
+integration tests **that drive one**, the out-of-process CLI tests, the smokes
+that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`,
+`smoke:web:tabs`), `pack:verify`, and **`smoke:tui`** — which never asserts a
+round trip but calls `ensureTestServers({ requires: ["stdio"] })` and hands the
+built fixture to the TUI as its catalog's stdio command. Not needing it: the
+pure tests inside the same integration project, the connecting tests that
+deliberately hand-roll a server, `smoke:launcher`, `smoke:web` and
+`smoke:web:browser` (all three stop at boot without a fixture), and every
+Storybook play function (fixture props). **Load the `test-servers` skill as soon
+as a task puts you on the fixture side of that line** — whichever tier it sits
+in.
+
 `validate` runs the per-client `test` scripts — so web **unit** plus cli's
 out-of-process `e2e.test.ts`, but **not** web's integration project, which runs
 inside the `coverage` gate. CI therefore has no separate `test:integration` step.
@@ -159,5 +223,19 @@ shared helper that wraps one.
 
 ## Test servers, not mocks
 
-Integration and smoke tests drive a real server over a real transport. See
-`/test-servers` for picking and building one.
+The tests that drive MCP behaviour over a transport use a real server rather
+than a mock, and **for the ones that get that server from `test-servers/`, load
+the skill and use all of it**: which showcase config covers the feature, which
+protocol era to connect with, how to add a combination that does not exist yet,
+and why a fixture can keep serving stale code after an edit.
+
+**A test that only *names* the built fixture needs that skill too, for a
+narrower reason.** `smoke:tui` boots the TUI against a catalog whose stdio
+command is the build output and asserts it survives — it opens no transport, so
+config choice and protocol era do not apply to it, but **building the fixture
+and the staleness hazard do.** Load the skill and take that half.
+
+A pure test that happens to live in the integration project, and a smoke that
+references no fixture, need neither (see the tier list above) — and note that
+`clients/web`'s `pretest` builds `test-servers/` before every unit run, so its
+presence on disk says nothing about whether your test depends on it.

docs/skill-authoring.md

@@ -284,22 +284,51 @@ pass 2/4 whenever `RUNS` is even, reporting a result the criterion does not
 license (Copilot). A strict bound of `1.0` is therefore unreachable and the
 harness rejects it up front rather than failing every case.
 
-At 0.8 a hand-off case would be red no matter how strongly the first skill
-pointed at the second, and the column would stop carrying signal. Read a
-hand-off number as a description-strength measurement, not a verdict — and read
-it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points.
+0.5 is a floor on **useful reliability for a second-hop load**, not a claim that
+0.8 is out of reach — a well-shaped pointer clears it outright, at 100% in the
+worked example below. Note that a *chain* bar of 0.8 would be a **strict** one
+(`> 0.8`, the comparison this threshold uses; the first-move 0.8 is the
+inclusive `>=`), so at `RUNS=5` only a clean 5/5 would pass it — 4/5 would not. What 0.5 buys is that the column keeps carrying signal across the
+*range* of pointer strengths a repo actually has: a hand-off is a noisier
+measurement than a first move, so a bar set where a strong pointer sits marks
+every merely-adequate one red and stops distinguishing them from a broken one.
+Read a hand-off number as a **pointer**-strength measurement — the strength of
+what the *first* skill's body says about the second, not of either description,
+since a well-written chained prompt is one the target's description cannot
+trigger on its own. Not a verdict, either; and read it at `RUNS=5`, since at
+`RUNS=3` one sample is worth 33 points.
 
-**The committed cases have measured 33% / 33% on one `RUNS=3` run and 100% /
-33% on another, and at least one of them being red is the intended state rather
-than an oversight.** `skills:eval` is not a gate (see below), and the number is
-the finding: `testing` points at `test-servers` in its first paragraph and the
-model follows that pointer *sometimes*. Strengthening it is its own change
-against its own issue (#2247); lowering the bar to turn the column green would
-throw away the only signal this feature adds.
+**A red hand-off case is a finding about the pointer, not a build break — and
+the fix is to reshape the pointer, never to lower the bar.** The committed
+`testing` -> `test-servers` cases are the worked example.
+
+Against the **weak** pointer — a one-sentence ⚠️ near the top of `testing`
+*classifying* which work belongs to `test-servers` — they measured **33% / 33%**
+on one `RUNS=3` run and **100% / 33%** on another. Those two runs are the
+unchanged-pointer pair the warning below is about: same prompts, same body, 67
+points apart on the first case.
+
+Rewriting that classification into an imperative first step ("load the
+`test-servers` skill now — that is step one"), and repeating it at the two later
+points where the model actually decides it is writing a fixture-backed test, took
+them to **100% / 100% at `RUNS=5`** with the prompts unchanged (#2247). Nothing
+else moved: the two descriptions were not touched, and the same suite scored
+**63/63** first-move cases at 100%. (An intermediate build of that change
+measured 100% / 80% on the full suite and 100% / 100% on a focused
+`-- test-servers` run — a reminder that even `RUNS=5` still carries 20 points of
+noise, well short of the 67 above.)
+
+The transferable part is that **a pointer is followed when it reads as an action
+with a trigger, and skimmed when it reads as a fact.** #2202 found the same
+lever on a *description*'s shape; this is it applied to a body. The corollary is
+where to put one: the top of a body is read before the model knows it needs the
+second skill, so a pointer that lives only there is a pointer it has already
+scrolled past by the time it matters.
 
 ⚠️ **Do not read a rise between two `RUNS=3` runs as an improvement.** One
-sample is 33 points there, and the two runs above straddle a 67-point swing on
-the same prompt with no change to the pointer. Note in particular that the
+sample is 33 points there, and the two weak-pointer runs above (33% / 33% and
+100% / 33%) straddle a 67-point swing on the same prompt with no change to the
+pointer. Note in particular that the
 turn-boundary rule added later can only ever *lower* a chained score — it
 rejects matches a flatter reading accepted — so a higher number after it is
 noise by construction, not an effect. `RUNS=5` is the smallest honest setting
@@ -372,7 +401,7 @@ The summary is two lines, never one:
 
 ```
 7/7 first-move cases at or above 80%.
-1/2 hand-off cases above 50%.
+2/2 hand-off cases above 50%.
 ```
 
 Narrowing the run never narrows what a **negative** case is scored against — a

scripts/skill-eval.mjs

@@ -56,11 +56,20 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8);
 // A hand-off is a harder thing to hit than a first move, and what counts as
 // acceptable is a separate judgement rather than one inherited from a number
 // tuned for the other measurement. 0.5 is the weakest claim worth asserting —
-// the pointer is taken more often than not. It is deliberately not 0.8: the
-// committed `testing` -> `test-servers` cases measure 33% (RUNS=3) against a
-// pointer that is live and stated in the first paragraph of `testing`'s body,
-// so an 0.8 bar would mark every hand-off red regardless of how strongly the
-// first skill points at the second, and the column would stop carrying signal.
+// the pointer is taken more often than not — and it is a floor on useful
+// reliability for a SECOND-HOP load, not a claim that 0.8 is unreachable. A
+// well-shaped pointer clears 0.8 outright: the committed `testing` ->
+// `test-servers` cases measured 33% (RUNS=3) when `testing` merely classified
+// which work belonged to `test-servers`, and 100%/100% (RUNS=5) once #2247
+// reshaped that into an imperative step. (An intermediate build of that change
+// measured 100%/80%. That is NOT an example of clearing a 0.8 bar — this
+// threshold is compared strictly, so 80% would fail one — but it is worth
+// knowing as the residual noise still present at RUNS=5.) What 0.5 buys
+// is a column that still separates an adequate pointer from a broken one — a
+// hand-off is a noisier measurement than a first move, so a bar set where a
+// STRONG pointer sits would mark both red.
+// A reshaped pointer therefore raises the ceiling those cases reach, not the
+// floor a *new* hand-off case should be judged against.
 //
 // It is compared STRICTLY, unlike the first-move threshold. "More often than
 // not" is `> 0.5`, and an inclusive compare passes exactly half the samples