From cd7fe9a036e5b2ec82a70950f3e161023a1a483e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 16:46:48 -0400 Subject: [PATCH] Review round 2: "an integration test" is not the trigger either; connecting is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 61 +++++++++++++++++++-------------- docs/skill-authoring.md | 33 +++++++++++------- 2 files changed, 56 insertions(+), 38 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index c1683d8c..c4bb87f6 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -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). diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 8acc4e88..15fb448c 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -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