diff --git a/.claude/skills/test-servers/SKILL.md b/.claude/skills/test-servers/SKILL.md index 58da2bd5..5c252e1a 100644 --- a/.claude/skills/test-servers/SKILL.md +++ b/.claude/skills/test-servers/SKILL.md @@ -17,9 +17,171 @@ and what the broken build did — is [`docs/test-servers.md`](../../../docs/test-servers.md). This skill is how to run one. +## Three ways to use a fixture — pick the right one first + +A fixture is stood up in **one of three shapes**, and most of what follows is +about the third. Establish which one you are in before reading further, because +the showcase **config file and the protocol-era table** belong to that one alone. + +⚠️ **The cut is how the server is stood up, not who is driving.** Automated and +by-hand is the wrong axis: the composable-config shape has *both* kinds of +consumer, and a smoke that spawns it needs every bit of the config and era +guidance a person at two terminals does. + +| Shape | Server runs | Config file | Consumers | +| --- | --- | --- | --- | +| **In-process HTTP** | inside the test process, built from the API | none — options are constructor args | integration tests, CLI tests, **`smoke:cli`** | +| **Spawned stdio** | a child process the transport (or the binary under test) starts | none — the stdio fixture runs its default config | integration tests, the CLI suites, **`smoke:cli`**, **`smoke:tui`** | +| **Spawned composable HTTP** | a child process started with `--config` | **yes** — a showcase `--config .json` | the **web** smokes, `pack:verify`, **and** a person by hand | + +⚠️ **"A smoke" is not a shape** — `smoke:cli` uses the first two and `smoke:tui` +the second, while only the config-driven web smokes use the third. Pick by the +row, never by the caller's category. + +- **In-process HTTP — `createTestServerHttp`.** The caller *constructs* the + server and owns its lifecycle. No subprocess, no JSON config, no showcase + config to pick. This is the shape for anything needing HTTP/SSE, a specific + tool set, or the modern handler — and it is not test-only: `scripts/smoke-cli.mjs` + starts one in the smoke process so it can read back the headers the CLI sent. +- **Spawned stdio — `getTestMcpServerCommand()`.** The test hands the built + fixture's `{ command, args }` to a stdio transport (or to the built CLI), and + the transport spawns it. A subprocess *is* started, but **still no config + file**: that entry point runs the stdio server's default config, so there is + nothing to pick. Reach for it when stdio is the point (`InspectorClient` over + stdio, the CLI's out-of-process E2E suite) and the default tool set is enough. + A caller may also just *name* the built entry rather than connect to it — + `smoke:cli` and `smoke:tui` write it into a `--catalog` as + `{ type: "stdio", command: node, args: [] }` — which is still + this shape, and still the build. +- **Spawned composable HTTP — `server-composable.js --config .json`.** + Picking the showcase config and the protocol era applies **to this shape**, + whoever starts it. Two consumers, and they differ only in who runs the second + process: + - **A script.** `scripts/smoke-web-elicitation.mjs` spawns it directly; + `scripts/lib/mcp-app-flow.mjs` (`startMcpAppServer`) does it for + `smoke:web:app`, `smoke:web:tabs` and `pack:verify`. These are automated and + config-driven, and the whole of this skill applies to them. ⚠️ **Not every + smoke is here** — `smoke:cli` and `smoke:tui` use the two shapes above and + pick no config at all. + - **You, in a terminal**, with the Inspector in another — `Run one by hand` + below. + +⚠️ **The build applies to all three.** Every shape resolves +`test-servers/build/` — the two API-driven ones through the +`@modelcontextprotocol/inspector-test-server` alias, the composable one by +running the emitted `.js` directly — so `Build first` and its stale-build hazard +are **not** guidance for one path. Read that section whichever shape you are in. + +### Automated, in-process HTTP: build the server from the API + +```ts +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + createEchoTool, +} from "@modelcontextprotocol/inspector-test-server"; + +let server: TestServerHttp | null = null; + +afterEach(async () => { + // Stop it even when the assertion threw, or the port leaks into the next test. + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } +}); + +it("…", async () => { + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("excluded-tools-test", "1.0.0"), + tools: [createEchoTool()], + // `modern: {}` opts the fixture into the 2026-07-28 handler; omit it for legacy. + }); + await started.start(); + server = started; + + // `started.url` is the bound URL — read it, never reconstruct it from a port. + // …connect an InspectorClient to it and assert. +}); +``` + +The reference test is +[`clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts`](../../../clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts) +— read it before writing a new one; it is the shape fixture-backed integration +tests follow when the server is built in-process. (Stdio-backed ones follow the +next subsection instead.) + +Four mechanics of this path: + +- **The factories come from one barrel.** `createTestServerHttp` / + `createTestServerStdio` build the server; the `create*Tool`, + `create*Resource` and `create*Prompt` fixtures in + `test-servers/src/test-server-fixtures.ts` populate it; + `createTestServerInfo` fills in `serverInfo`. Prefer an existing fixture + factory to hand-writing a `ToolDefinition` — that is what makes the fixture a + shared one. +- **`start()` then `stop()`, and `stop()` in an `afterEach`.** The server binds a + real port, so a test that throws before stopping leaks it into the rest of the + file. +- **Read `started.url`.** `createTestServerHttp` resolves through + `findAvailablePort()`, which walks upward when the port is taken, so an + assumed port is the same bug the two-process path has. +- **Era is a constructor option, not a config file.** `modern: {}` on the config + object selects the modern handler; the client side picks its own negotiation + (`eraToVersionNegotiation`). The showcase-config era table below does not + apply. + +⚠️ **The barrel is an alias to the BUILD, not to the source** — +`vitest.shared.mts` maps `@modelcontextprotocol/inspector-test-server` to +`test-servers/build/index.js`. So `Build first` applies to this path in full, +stale-build hazard included: an edit to `test-servers/src` that is not rebuilt +is invisible to an in-process test exactly as it is to a spawned one. + +### Automated, spawned stdio: hand over the command + +```ts +import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; + +const { command, args } = getTestMcpServerCommand(); +const client = new InspectorClient( + { type: "stdio", command, args }, + { environment: { transport: createTransportNode } }, +); +await client.connect(); +// … afterEach → client.disconnect(), which is what stops the child. +``` + +`getTestMcpServerCommand()` returns `node `. +Three consequences: + +- **You do not own the process, the transport does.** There is no `start()` / + `stop()` pair — disconnecting the client is what reaps the child, so the + `afterEach` that matters is `client.disconnect()`. +- **No config is selected and none can be.** That entry point starts the stdio + server on its **default** config, so the showcase-config table and the + protocol-era guidance below do not apply. If the case needs a specific tool + set or the modern handler, it is an in-process HTTP test, not this. +- **It is still the build.** The path comes from the module's own resolved + location under the alias, so it is `test-servers/build/`, with the same + staleness hazard. + +The same command feeds the CLI's out-of-process E2E suite +(`clients/cli/__tests__/e2e.test.ts`), which spawns the built CLI *and* lets it +spawn the fixture. Reference tests for this shape: +`clients/web/src/test/integration/mcp/inspectorClient-response-rejected.test.ts` +and `clients/cli/__tests__/methods.test.ts`. + ## Build first -The servers are spawned as real subprocesses, so the build output must exist: +Every shape above resolves generated output — the in-process one imports the +barrel, which is **aliased to `test-servers/build/index.js`**, and the stdio and +composable-config ones run emitted `.js` as real subprocesses. So the build must +exist whichever one you are in: ```sh cd clients/web && npm run test-servers:build # tsc -p test-servers → test-servers/build/ @@ -41,9 +203,11 @@ rm -rf test-servers/build The `.tsbuildinfo` is pinned inside `build/` so that clean actually invalidates the cache. -## Run one +## Run one by hand (two processes) -Two processes: the test server, then the Inspector. +This is the **spawned composable HTTP** shape from the section above, driven by +you rather than by a smoke script — the config and era guidance is the same +either way. Two processes: the test server, then the Inspector. ```sh # 1. The server, from the repo root, with the config you picked: diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 85c8caa5..62a52e5c 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -11,7 +11,7 @@ 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. -## Before you write it: does the test need a real server? +## Before you write it: does the test use a `test-servers/` fixture? **If it does, load the `test-servers` skill now — that is step one, before choosing a location or writing a line.** @@ -23,8 +23,35 @@ 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. + against a server. These need the whole procedure — the staleness hazard, and + then whichever half matches how the server is stood up. + ⚠️ **Which shape you need depends on what is driving, and there are three.** + All three are the **"Three ways to use a fixture"** section of `test-servers`, + which names the entry point and a reference for each — read the right third: + - **An integration or CLI test → in-process HTTP.** + `createTestServerHttp(...)` / `.start()` / `.stop()`, with the test owning + the lifecycle. Use it when the case needs HTTP or SSE, a specific tool set, + or the modern handler (`modern: {}` is a constructor option). **No showcase + config and no era table apply.** + - **An integration or CLI test where stdio is the point → spawned stdio.** + `getTestMcpServerCommand()` handed to a stdio transport or to the built CLI, + which spawns it. A subprocess *is* started, but it runs the stdio fixture's + **default** config, so there is still nothing to pick — and nothing to + override, so if the case needs a specific tool set it is an in-process HTTP + test instead. + - **A config-driven web smoke, or `pack:verify` → spawned composable HTTP.** + `smoke:web:elicitation`, `smoke:web:app`, `smoke:web:tabs` and `pack:verify` + all spawn `server-composable.js --config .json`. **The showcase-config + and protocol-era guidance applies to you in full** — being automated does + not exempt a smoke from it. + + ⚠️ **"A smoke" is not a shape, so do not route by that word.** `smoke:cli` + uses the first two — an in-process `createTestServerHttp` for the header + round-trip, and the built stdio entry in a `--catalog` for the connect checks + — and `smoke:tui` uses the stdio entry alone. Only the web smokes above are + config-driven. Pick by what the caller actually stands up. + + What applies to all three is that section's build warning. ⚠️ **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` @@ -39,7 +66,19 @@ ways to depend on one, and they need different halves of that skill: ⚠️ **"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. +the test **starts, spawns, configures, or hands a built entry to the subject +under test**. That last clause is what covers `smoke:tui`, which drives no +transport at all and still depends on the fixture — see the build-only bullet +above. + +⚠️ **And *importing* the package is not the dependency either.** The barrel +exports plain functions as well as server factories, so a test can import from +it and never stand a server up — `src/test/core/mcp/test-server-scope.test.ts` +imports `createScopeCheckMiddleware` and friends to unit-test the scope +middleware as a pure function, with no `start()` anywhere in the file. None of +the procedure applies to it — no config, no era, no lifecycle — it is an +ordinary unit test that happens to import its subject from that package. Ask +whether a *server* runs, not whether the import line is present. 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 diff --git a/AGENTS.md b/AGENTS.md index 4f09d1c1..e80ed76f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -318,6 +318,12 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 negative requirement, and it is only worth writing where the first link's body actually points at the target — a chain through a skill that says nothing about it is a permanent 0% with no lever. + ⚠️ **A green chain case proves the second skill LOADED, never that it + answered.** `testing` -> `test-servers` held 100% while `test-servers` + documented only the two-process manual path, so a model arriving from an + integration-test prompt was handed the wrong half of the procedure (#2264). + When a chain goes green, read the target's body as the caller who arrives + through the pointer; no number measures that. ⚠️ **The gate cannot catch a description that never matches.** `verify:skills` checks that a skill is well-formed and that its cases exist; only `skills:eval` observes whether it actually fires, and that cannot be gated — diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 2f6f2851..8582c732 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -201,6 +201,17 @@ ever taken.** `test-servers` scores 5/5 on its own cases and every one of them asks for it by name; a skill only ever reached _through_ another would score a clean 100% while the hand-off silently never fired (#2204). +⚠️ **A green hand-off number is not evidence the hand-off is USEFUL.** The case +can observe one thing — that skill B was loaded. Whether B then answers the +question the prompt actually asked is outside what any `chain` case measures, in +either direction: B can be loaded and say nothing relevant, or say something +actively wrong for the caller that arrived through A. `testing` -> +`test-servers` scored 100% while `test-servers` documented only the two-process +manual path, so every model that followed the pointer from an *integration-test* +prompt was handed the wrong half of the procedure (#2264). So when a chain case +goes green, **read B's body as the caller who arrives through A** and check that +what it says fits that caller. The number cannot do that for you. + A chained case names the ordered skills one run should load: ```json @@ -285,8 +296,8 @@ license (Copilot). A strict bound of `1.0` is therefore unreachable and the harness rejects it up front rather than failing every case. 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 is out of reach — the one pointer measured after reshaping cleared 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 @@ -319,11 +330,18 @@ measured 100% / 80% on the full suite and 100% / 100% on a focused 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. +with a trigger, placed where the decision is actually made.** #2202 found the +same lever on a *description*'s shape; this is it applied to a body. + +⚠️ **Read that as ONE lever, not two.** The #2247 edit changed the wording *and* +the placement in the same revision, and only the combination was measured, so +the run establishes the pair — it does not license attributing the rise to +imperative phrasing alone, nor to repetition alone. The split into two levers is +the plausible reading of the mechanism, not a measured result: the top of a body +is read before the model knows it needs the second skill, so a pointer that +lives only there has plausibly been scrolled past by the time it matters. Taking +them apart would need two more runs — reword without moving, and move without +rewording — and nobody has paid for those. Until someone does, ship both. ⚠️ **Do not read a rise between two `RUNS=3` runs as an improvement.** One sample is 33 points there, and the two weak-pointer runs above (33% / 33% and @@ -452,7 +470,9 @@ break. hand-off has a `chain` case — a pointer between skills is otherwise measured by nothing at all, and a skill reached only that way scores a clean 100% on direct cases while the hand-off never fires. It does not count toward the - floor in 4. + floor in 4. **Then read the target's body as the caller arriving through that + pointer** — the case proves the load, never that what loads is the half that + caller needs (#2264). 6. `npm run verify:skills` passes and the listing is under budget. 7. `RUNS=5 npm run skills:eval` — the **whole** suite — is ≥80% on every first-move case, including the skills you did not touch, and the hand-off diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 780c09bb..ed7fc245 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -57,11 +57,14 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // 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 — 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` -> +// reliability for a SECOND-HOP load, not a claim that 0.8 is unreachable. One +// well-shaped pointer has cleared 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 +// reshaped that into an imperative step. That is ONE pointer over ONE edit that +// moved wording and placement together, so read it as an existence proof that +// the ceiling is above 0.8 — not as a general property of "well-shaped" +// pointers, and not as grounds for raising this floor (Copilot, #2264). (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