Skip to content

The testing → test-servers hand-off lands on the wrong half of the procedure #2264

Description

@cliffhall

Follow-up to #2247 / #2261. Copilot filed a ninth review round after that PR merged, so none of it is on v2/main. Five findings, three of them substantive.

The headline one: the hand-off succeeds by the metric and still delivers the wrong instructions

#2247 got testing → test-servers to 100%, but test-servers's body documents one way to use a fixture: run server-composable.js --config <name>.json as a second process and connect the Inspector to it. That is the by-hand path.

An integration test does not work that way. It builds the server in-process and owns its lifecycle:

// clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts
import { createTestServerHttp, createTestServerInfo, createEchoTool }
  from "@modelcontextprotocol/inspector-test-server";

const started = createTestServerHttp({ serverInfo: , tools: [createEchoTool()] });
await started.start();
// … afterEach → started.stop()

No process is spawned, no JSON config is read, no showcase file is picked. So the model loads the skill — the thing #2247 spent eight review rounds securing — and gets the API/lifecycle guidance it needs nowhere, plus a nudge toward an external config and process it should not use.

This is worth stating plainly: a green hand-off number is not evidence the hand-off is useful. The eval can only observe that skill B was loaded. Whether B then answers the question is outside what any chain case measures, and #2247 did not notice because 100% looked like success.

The rest

# Finding File
2 Heading still asks "does the test need a real server?", which the body's own hand-rolled-server exception contradicts. Should ask about a test-servers/ fixture. .claude/skills/testing/SKILL.md
3 "imports" is too broad a dependency test — src/test/core/mcp/test-server-scope.test.ts imports @modelcontextprotocol/inspector-test-server to unit-test its scope middleware as a pure function and never starts a server. The condition is starts, spawns, or configures a fixture. same
4 The worked example attributes the 33% → 100% rise to imperative wording and placement independently; the edit changed both at once, so only the combination is established. docs/skill-authoring.md
5 CHAIN_THRESHOLD comment generalises "a well-shaped pointer clears 0.8" from one two-case run of a multi-variable edit. scripts/skill-eval.mjs

State

All five are already written, as an uncommitted patch against 0d947ad5 — 4 files, +81/-20, including a new "Two ways to use a fixture — pick the right one first" section in test-servers with the in-process recipe and a pointer to the reference test. It is saved locally and needs re-applying on a fresh branch off v2/main.

⚠️ It is not verified. The one skills:eval run against that patch reported 31/63 first-move and 0/2 hand-off — a collapse across skills the patch does not touch, which is the signature of a rate limit rather than a regression (docs/skill-authoring.md warns the eval "goes red on a rate limit"). That has to be re-run before anything is concluded; if it reproduces, the patch is at fault and finding 1's new section is the first suspect, since it is the only large body addition.

Acceptance

  • The five findings fixed on v2/main.
  • RUNS=5 npm run skills:eval re-run clean: hand-off stays above 50%, no first-move regression. The 0/2 reading explained — rate limit or real.
  • A reader who lands in test-servers from an integration-test prompt finds the in-process API, not only the two-process one.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    choreMaintenance: deps, build tooling, CI, cleanup — no user-facing behavior changev2Issues and PRs for v2

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions