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

chore: land the #2247 round-9 review findings on v2/main (#2322)

Browse source 
* chore: land the #2247 round-9 review findings on v2/main

Copilot filed a ninth review round on #2247 after it had merged, so none of
it reached v2/main. All five findings are addressed here.

1. The testing -> test-servers hand-off delivered the wrong half of the
   procedure. test-servers documented only the two-process manual path
   (server-composable.js --config <name>.json plus a second Inspector
   process), but an integration test builds the server in-process from the
   @modelcontextprotocol/inspector-test-server API and owns its lifecycle.
   Adds a "Two ways to use a fixture — pick the right one first" section
   with the in-process recipe, the four mechanics that bite (one barrel,
   start/stop in afterEach, read started.url, era is a constructor option),
   and a pointer to the reference test; renames "Run one" to "Run one by
   hand (two processes)" so the manual path names itself. testing now says
   in the connecting bullet that an automated test does NOT spawn.

2. testing's heading asked "does the test need a real server?", which its
   own hand-rolled-server exception contradicts. It now asks whether the
   test uses a test-servers/ fixture.

3. "imports" was too broad a dependency test — test-server-scope.test.ts
   imports the package to unit-test scope middleware as a pure function and
   never starts a server. The condition is now "starts, spawns, or
   configures", with that test named as the counterexample.

4. docs/skill-authoring.md attributed the 33% -> 100% rise to imperative
   wording and placement independently. The #2247 edit changed both at
   once, so only the combination is established; the split into two levers
   is now labelled the plausible mechanism, not a measured result.

5. The CHAIN_THRESHOLD comment generalised "a well-shaped pointer clears
   0.8" from one two-case run of that same multi-variable edit. It now
   reads as an existence proof about the ceiling, not a property of
   pointers in general.

Also records the headline lesson where it can be found again: a green
hand-off number proves skill B loaded, never that B answered the question —
in AGENTS.md's chain-case rules and in the skill-authoring checklist.

Docs, skill bodies and one comment only; no runtime code changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix: address Copilot review round 1 on #2264

The new section claimed two shapes and there are three — the automated
stdio path was missing, and it is the repo's established pattern
(getTestMcpServerCommand() into a stdio transport, used by five web
integration files and the whole CLI suite). Telling callers an automated
test "does not spawn" told them not to use it.

- test-servers: "Two ways" -> "Three ways", with a table and a subsection
  for the spawned-stdio shape — the transport owns the child (disconnect,
  not stop()), no config can be selected because that entry point runs the
  stdio default, and it is still the build. Names a reference test for it.
- test-servers: the build is no longer scoped to the manual path. All three
  shapes resolve test-servers/build/ — two through the barrel alias, one by
  running the emitted .js — so the staleness hazard is called out as
  applying to every shape, and `Build first` no longer opens by claiming
  the only reason is subprocess spawning.
- testing: the "does not spawn" instruction is replaced by the two
  automated shapes and when to pick each. What they share, and what the
  hand-off actually needed to say, is that neither picks a showcase config.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix: address Copilot review round 2 on #2264

Round 1 fixed the missing stdio shape but kept the wrong axis. Cutting
the taxonomy by "automated vs by hand" excluded the config-driven smokes
— smoke-web-elicitation.mjs spawns server-composable.js --config
directly, and lib/mcp-app-flow.mjs does it for smoke:web:app,
smoke:web:tabs and pack:verify. Those are automated AND need every bit
of the config and protocol-era guidance a person at two terminals does,
so the skill was routing their authors away from it.

- test-servers: the cut is now how the SERVER IS STOOD UP, stated as
  such. Third shape is "spawned composable HTTP", whose consumers are
  the smokes, pack:verify, and a person by hand — the config/era
  guidance belongs to the shape, whoever starts it. Table gains a
  Consumers column and loses the Who column that caused this.
- test-servers: the stale "an automated test builds the server
  in-process instead" line under `Run one by hand` is gone (suppressed
  comment); that heading is now the same shape driven by a person.
- testing: routes by what is driving — integration/CLI test to
  in-process HTTP, stdio-is-the-point to spawned stdio, and a smoke to
  the composable-config path with the era guidance it actually needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>

* fix: address Copilot review round 3 on #2264

Round 2 replaced one wrong axis with a narrower one: it routed "a smoke"
wholesale to the config-driven composable path, and smoke:cli does not
use it. smoke-cli.mjs starts an in-process createTestServerHttp for the
header round-trip (so it can read back what the CLI sent) and writes the
built stdio entry into a --catalog for the connect checks; smoke:tui uses
only the latter. Neither picks a config.

- test-servers: consumers filled in per row — in-process HTTP gains CLI
  tests and smoke:cli, spawned stdio gains smoke:cli and smoke:tui, and
  the composable row is narrowed to the WEB smokes. A warning states the
  point directly: "a smoke" is not a shape, pick by the row.
- test-servers: the stdio shape now covers naming the built entry in a
  catalog without connecting, which is what those two smokes do.
- testing: same narrowing, plus the reason spelled out so the word
  "smoke" stops being a routing key.
- testing: the dependency criterion said "starts, spawns, or configures",
  which excluded the build-only smoke:tui case documented directly above
  it — the skill both required and rejected the hand-off for it
  (suppressed comment). It now reads "starts, spawns, configures, or
  hands a built entry to the subject under test".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>

---------

Signed-off-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall 2026-09-11 08:22:16 -04:00 • committed by GitHub
commit ab80221ce6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 250 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -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 <name>.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