modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

Review round 2: "an integration test" is not the trigger either; connecting is

Browse source 
Copilot, both findings accepted — and the first is the same defect one
level down from round 1.

1. Round 1 replaced "a smoke" with a transport condition but left "an
   integration test" unqualified, and left the tier note treating web
   integration as wholly fixture-dependent. It is not: **29 of the 74
   files in `src/test/integration/` reference a server at all.**
   `storage/store-id.test.ts` validates a string; `mcp/import/*` parses
   config files. They sit there for the node env and the 30s timeout —
   placement is the project manifest, which is exactly why it cannot
   also be the fixture trigger.

   All four sites now say "an integration test **that connects**" and
   name the counter-examples, including the folder-is-not-the-trigger
   point at the `integration` bullet itself, and the tier ⚠️ now states
   that the boundary cuts *across* the tiers rather than along them.

2. Replacing the worked example orphaned the warning after it: "the two
   runs above straddle a 67-point swing … with no change to the pointer"
   had nothing left to refer to, since the 33%/33% and 100%/33% pair it
   names was gone and the runs on show differed by 20 points across a
   pointer rewrite. The weak-pointer pair is restored as its own
   paragraph, the warning names both figures explicitly, and the 20-point
   intermediate is now framed as RUNS=5 residual noise — contrasted with
   the 67, rather than standing in for it.

Re-measured, since "that connects" narrows the phrasing the two chain
prompts have to match: 100% / 100% hand-off, 63/63 first-move at 100%,
full suite at RUNS=5. Unchanged from round 1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-05 16:46:48 -04:00
commit cd7fe9a036
2 changed files with 56 additions and 38 deletions
Download patch file Download diff file Expand all files Collapse all files

61
.claude/skills/testing/SKILL.md
View file

@ -16,14 +16,21 @@ test goes, how to run it, and how to clear the gate.
**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 exercise MCP behaviour over a transport?"**, not
which tier the test lands in. It does whenever the task is: an integration test;
an end-to-end test that connects; a smoke that drives a **connected** flow; a
coverage gap only reachable over a real connection; or reproducing a reported
bug against a server. It does **not** for a test that renders a component from
fixture props, or for a boot-only smoke that never connects — `smoke:launcher`
checks `--help` and `smoke:web` / `smoke:web:browser` only assert the SPA is
served and paints.
The condition is **"does this test exercise MCP behaviour over a transport?"** —
not which tier it lands in, and not which directory it lands in. It does
whenever the task is: 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; or reproducing a reported bug against a
server.
It does **not** when the test renders a component from fixture props, exercises
a pure function or a parser, or is a boot-only smoke that never connects.
⚠️ **Neither the tier nor the folder decides this.** Only 29 of the 74 files in
`src/test/integration/` touch a server at all — `storage/store-id.test.ts`
validates a string, and `mcp/import/*` parses config files; they sit there for
the node env and the 30s timeout, not because they connect. Likewise
`smoke:launcher` checks `--help`, and `smoke:web` / `smoke:web:browser` only
assert the SPA is served and paints.
When it does apply, the test drives a **real server over a real transport, never
a mock**, and picking the fixture, building it, and connecting with the right
@ -52,9 +59,10 @@ 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. ⚠️ These run against a
real server, so **load the `test-servers` skill before writing one** — the
fixture is half the test.
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 **connects**,
**load the `test-servers` skill first**; the fixture is half of that test.
3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`,
`fixtures/`, `scrollAreaStoryAssertions.ts`.
@ -98,14 +106,15 @@ 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 — connecting is.** Web
integration, the out-of-process CLI tests, the smokes that actually connect
(`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and
`pack:verify` all need a fixture from `test-servers/`; `smoke:launcher`,
`smoke:web`, `smoke:web:browser` and every Storybook play function do not — the
first three stop at boot, and play functions render from fixture props. **Load
the `test-servers` skill as soon as a task puts you on the connecting side of
that line.**
⚠️ **Depth in that list is not the fixture boundary — connecting is, and it
cuts across the tiers rather than along them.** The **connecting** web
integration tests, the out-of-process CLI tests, the smokes that actually
connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and
`pack:verify` need a fixture from `test-servers/`. The rest do not: the pure
tests inside the same integration project, `smoke:launcher`, `smoke:web`,
`smoke:web:browser` (all three stop at boot) and every Storybook play function
(fixture props). **Load the `test-servers` skill as soon as a task puts you on
the connecting 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
@ -184,9 +193,11 @@ shared helper that wraps one.
## Test servers, not mocks
Integration tests, and the smokes that drive a connected flow, use a real server
over a real transport rather than a mock. **For those, load the `test-servers`
skill to pick, build and run the fixture** — 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
boot-only smoke needs none of it (see the tier list above).
The tests that drive MCP behaviour over a transport — the connecting integration
tests and the connected smokes — use a real server rather than a mock. **For
those, load the `test-servers` skill to pick, build and run the fixture** —
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 pure test that happens to live in the
integration project, and a boot-only smoke, need none of it (see the tier list
above).

33
docs/skill-authoring.md
View file

@ -295,17 +295,23 @@ and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points.
**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. They measured
33% / 33% at `RUNS=3` when `testing` opened with a one-sentence ⚠️ *classifying*
which work belongs to `test-servers`; rewriting that 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 an
integration 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 measured 100% / 80% on the full suite and 100% / 100% on a focused
`-- test-servers` run — worth knowing as the size of the run-to-run noise still
present at `RUNS=5`.)
`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 connecting 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
@ -315,8 +321,9 @@ 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