From 0d947ad56982c3114dc6bb1598c942c030554d38 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 18:03:37 -0400 Subject: [PATCH] Review round 8: using a fixture, not connecting, is the trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, all three accepted. 1. Connecting still is not the condition. 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` says so in its own header: "the SDK's own server cannot emit these shapes: they are exactly what its types forbid". Same for `listSalvage-era.test.ts`. Real transport, real client, zero `test-servers/` dependency. Round 4 already stated the right condition at the top — "does this test depend on a fixture from `test-servers/`" — and then three downstream sites went on saying "connects". Those are now scoped to fixture use, and the hand-rolled exception is stated where the trigger is defined rather than left to be discovered, with both files named. Connecting is kept as a hint, since it usually does imply a fixture, with the instruction to check whether one can express the case first. 2. "description-strength measurement" was the wrong surface, and this PR is what made it wrong: a chained prompt is deliberately written so the target's *description* cannot fire on it, so the number measures the pointer in the first skill's body and nothing else. Left as-is it would send the next person tuning a red hand-off at the description. 3. "an 0.8 bar" -> "a 0.8 bar". Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, RUNS=5. Staged by explicit path, not `git add -A` — see ec6895a5. 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 | 42 ++++++++++++++++++++------------- docs/skill-authoring.md | 7 ++++-- scripts/skill-eval.mjs | 2 +- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 5207db6e..840ee9ac 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -20,11 +20,17 @@ The condition is **"does this test depend on a fixture from `test-servers/`?"** — not which tier it lands in, and not which directory it lands in. There are two ways to depend on one, and they need different halves of that skill: -- **It connects.** 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. +- **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. + ⚠️ **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` + and `listSalvage-era.test.ts` need wire shapes the SDK's own server refuses to + emit. Real transport, real client, no `test-servers/` dependency. Check + whether a fixture can express the case before reaching for one. - **It names or runs the built fixture without connecting.** `smoke:tui` boots the TUI against a catalog whose stdio command *is* the built fixture, then asserts it survives. No transport is driven and no protocol era applies, but @@ -79,8 +85,10 @@ web-owned test living under `src/test/` instead is a bug. folder is picked up by the integration project (node env, 30s timeouts) via a 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. + alongside the connecting ones. If the test you are adding here **needs a + fixture from `test-servers/`, load that skill first**; the fixture is half of + that test. Connecting is a strong hint but not the rule — see the + hand-rolled-server exception above. 3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`. @@ -125,14 +133,15 @@ 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, and the boundary cuts -across the tiers rather than along them.** Needing `test-servers/`: the -**connecting** web integration tests, the out-of-process CLI tests, the smokes +across the tiers rather than along them.** Needing `test-servers/`: the web +integration tests **that drive one**, the out-of-process CLI tests, the smokes that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`), `pack:verify`, and **`smoke:tui`** — which never asserts a round trip but calls `ensureTestServers({ requires: ["stdio"] })` and hands the built fixture to the TUI as its catalog's stdio command. Not needing it: the -pure tests inside the same integration project, `smoke:launcher`, `smoke:web` -and `smoke:web:browser` (all three stop at boot without a fixture), and every +pure tests inside the same integration project, the connecting tests that +deliberately hand-roll a server, `smoke:launcher`, `smoke:web` and +`smoke:web:browser` (all three stop at boot without a fixture), and every Storybook play function (fixture props). **Load the `test-servers` skill as soon as a task puts you on the fixture side of that line** — whichever tier it sits in. @@ -214,12 +223,11 @@ shared helper that wraps one. ## Test servers, not mocks -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 and use all of it**: 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. +The tests that drive MCP behaviour over a transport use a real server rather +than a mock, and **for the ones that get that server from `test-servers/`, load +the skill and use all of it**: 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 test that only *names* the built fixture needs that skill too, for a narrower reason.** `smoke:tui` boots the TUI against a catalog whose stdio diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index d1a9070f..2f6f2851 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -292,8 +292,11 @@ inclusive `>=`), so at `RUNS=5` only a clean 5/5 would pass it — 4/5 would not *range* of pointer strengths a repo actually has: a hand-off is a noisier measurement than a first move, so a bar set where a strong pointer sits marks every merely-adequate one red and stops distinguishing them from a broken one. -Read a hand-off number as a description-strength measurement, not a verdict — -and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. +Read a hand-off number as a **pointer**-strength measurement — the strength of +what the *first* skill's body says about the second, not of either description, +since a well-written chained prompt is one the target's description cannot +trigger on its own. Not a verdict, either; 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 diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 2464f5fa..780c09bb 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -62,7 +62,7 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // `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 -// measured 100%/80%. That is NOT an example of clearing an 0.8 bar — this +// 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 // is a column that still separates an adequate pointer from a broken one — a