feat(skills): measure a skill reached from another skill

[cliffhall]

Closes #2204

skills:eval ran every case with --max-turns 1, so it measured exactly one thing: whether a skill is the model's first tool call. That is the right measurement for a skill a user reaches directly, and it leaves one class entirely unmeasured — a skill reached from inside another skill. testing opens by telling the model that picking a fixture is /test-servers and that it has to load it; test-servers scores 5/5, and all five of its cases ask for it by name. A skill only ever reachable through that hand-off would score a clean 100% while the hand-off silently never fired.

What changed

An eval case is now one of two shapes, and exactly one:

{ "prompt": "…", "expect": "test-servers" }                 // first move, 1 turn
{ "prompt": "…", "chain": ["testing", "test-servers"] }     // hand-off, 14 turns

• A chain ends with the skill whose file it lives in. The case measures whether this skill is reachable, so the file that goes red is the one belonging to the skill that stopped being reached. Anchoring on the first link would file the testing → test-servers measurement under testing, where a test-servers description edit would never be seen.
• Links are validated against the repo's model-invoked set, so a chain through a skill the model cannot invoke is a hard error rather than a permanent 0% that reads as a description problem. That is why verify:skills and collectCases both validate in a second pass — test-servers sorts before testing, so a per-directory check would reject a live chain as unknown purely because of where the alphabet put it.
• Scoring is an ordered subsequence, not a prefix and not a contiguous run: the model may load something before the chain starts and something unrelated in between, and neither changes the claim that A led to B. collectSkillInvocations therefore returns an ordered array rather than a Set — a B, A, B run has to stay distinguishable from one that never reached B from A.
• A chained case counts toward neither floor (five positives, one negative). It measures a different thing, so letting it stand in would let a skill ship with no measurement of the way users actually reach it.
• The two rates are reported in separate columns and never summed, with CHAIN_MAX_TURNS (14) and CHAIN_THRESHOLD as their own knobs.

Read-only containment no longer leans on --max-turns 1, which was doing much of it by itself. The deny list gains the agentic and network tools — Task in particular, whose subagent the flag does not reach.

Measured

skills:eval -- test-servers:

First move (1 turn)
PASS 100%  test-servers               … × 5 positives, × 2 negatives

Hand-off (14 turns)
     67%  testing → test-servers      Write an integration test that exercises tool listing against a real server.
     33%  testing → test-servers      Add end-to-end coverage for tool-list pagination against a live server.

So the hand-off is real and unreliable — the fact nothing could observe before this change. All seven first-move cases held at 100%, so the new column displaced nothing.

Two calls that follow from the numbers, both written up in docs/skill-authoring.md:

• CHAIN_THRESHOLD defaults to 0.5, not 0.8. The weakest claim worth asserting is that the pointer is taken more often than not. At 0.8 both committed cases are red no matter how strongly the first skill points at the second, and the column stops carrying signal; at 0.5 the difference between 67% and 33% is the signal. skills:eval is not a gate, so a hand-off case below threshold is a signal to investigate the pointer's strength, not a build break.
• A ["pr-flow", "test-servers"] probe measured 0% and was dropped rather than committed. pr-flow says nothing about test fixtures, so the case had no lever short of broadening a description onto another skill's ground. The doc now says to confirm the first link's body actually points at the target before writing a chained case.

Acceptance

• The suite can express and score "loading A should lead to loading B", with committed cases for the testing → test-servers hand-off.
• The chained measurement is reported distinctly from the first-move rate.
• docs/skill-authoring.md gains a section on when to write a chained case instead of a direct one.

Testing

• npm run test:scripts — 512 pass (23 new cases across skill-eval.test.mjs, skill-manifest.test.mjs, verify-skills.main.test.mjs), covering the subsequence semantics, the ordered-array change, the chain validator's five rules, the wider turn budget, the deny list, and the two-pass ordering trap.
• npm run verify:skills — OK, listing unchanged at 3234/4000.
• npm run local:gate — exit 0: 326 + 27 + 27 + 1 test files green, coverage 98.64 / 98.96 / 96.32 / 100 across web, cli, tui, launcher, all smokes OK, Storybook 514 tests passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe

[Copilot]

🟡 Changes recommended

The committed cases can false-pass without a real hand-off, and multi-turn execution is not fully contained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds multi-turn skill hand-off measurement to the repository’s skill evaluation tooling.

Changes:

• Introduces validated chain cases with ordered-subsequence scoring.
• Separates first-move and hand-off reporting and thresholds.
• Adds tests, documentation, and initial testing → test-servers cases.

File summaries

File	Description
.claude/skills/test-servers/evals/evals.json	Adds hand-off evaluation cases.
AGENTS.md	Documents chain-case requirements.
docs/skill-authoring.md	Adds hand-off authoring guidance.
scripts/lib/skill-manifest.mjs	Validates chain case structure and links.
scripts/lib/skill-manifest.test.mjs	Tests chain validation.
scripts/skill-eval.mjs	Runs, scores, and reports multi-turn chains.
scripts/skill-eval.test.mjs	Tests invocation ordering and turn policies.
scripts/verify-skills.main.test.mjs	Tests repository-wide link validation.
scripts/verify-skills.mjs	Performs two-pass chain validation.

Review details

Suppressed comments (1)

.claude/skills/test-servers/evals/evals.json:30

• This second chain case also names the target's direct trigger (“live server”), so it measures two independently matching skills in order rather than whether testing caused the hand-off. Phrase it as a testing task without the server cue; the testing skill should be what introduces the need for test-servers.

    "prompt": "Add end-to-end coverage for tool-list pagination against a live server.",

• Files reviewed: 9/9 changed files
• Comments generated: 5
• Review effort level: Balanced

[cliffhall]

Round 1 — all five implemented, in f20bbac4

Every finding was real; the first one changed what this PR reports.

1. The chain prompts carried the target skill's own trigger (evals.json, both cases including the suppressed comment on line 30) — fixed, and it moved the numbers.

You were right that the ordered pair could pass without the hand-off: test-servers claims "a change needs a real server to exercise it", so a prompt saying "against a real/live server" matches it directly, and the model could load testing and then pick test-servers from the original prompt in that order. Both prompts now carry no server cue:

	before	after
Write an integration test that exercises tool listing …	"…against a real server" — 100%	"…end to end" — 33%
Add end-to-end coverage for tool-list pagination …	"…against a live server" — 67%	"…the tool-list pagination path" — 33%

That gap is the artefact, and 33% is what the pointer alone is worth. Both cases are now red against the 0.5 bar and stay that way — skills:eval is deliberately not a gate, the number is the finding, and lowering the bar to turn the column green would discard the only signal this feature adds. Filed as #2247 with the trap written down, so a fix can't re-measure the artefact. The doc gained a ⚠️ section on it, with the before/after rates as the evidence of how much it hides.

2. A deny list cannot bound a 14-turn run (skill-eval.mjs) — fixed. The .mcp.json observation was exact. Now:

--allowedTools Read,Glob,Grep,Skill    # what the harness actually needs
--disallowedTools <the 10>             # kept: a deny is unconditional, an allow list governs pre-approval
--strict-mcp-config                    # with no --mcp-config, so every configured server is dropped

One residual I've stated rather than papered over: a contributor's own plugin tools are outside all three mechanisms. --bare would remove them — and skills with them, which would measure nothing.

3. CHAIN_THRESHOLD compared inclusively — fixed. passesThreshold(rate, threshold, strict) is strict for chains (> 0.5, so 2/4 no longer passes) and a floor for first moves (4/5 still clears 0.8). A strict bound of 1.0 is unreachable, so it's now rejected up front rather than failing every case while looking like a trigger problem.

4. The probe command in the doc was stale — fixed. It now carries the same allow/deny/--strict-mcp-config policy, with a ⚠️ saying it must move with runPrompt in the same edit, plus a hand-off probe form at --max-turns 14.

5. The reporting had no coverage — fixed. formatReport is extracted and exported, and four new cases assert both headings, the per-group thresholds, the two separate summary lines, that no line merges them, the combined exit, and both single-kind selections.

npm run test:scripts is 517 → 521 passing; npm run local:gate re-run against the fixes.

[Copilot]

🟡 Changes recommended

Tool containment is not fully enforced, and malformed chain thresholds can produce misleading results.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Round 2 — both implemented, in 3ec12572

1. --allowedTools is not the availability filter — correct, and it means round 1's containment fix was only half a fix. Permission ≠ availability, so a tool a user's or a plugin's settings already permit stayed reachable for all 14 turns, and the bound still rested on the deny list being complete — the exact property round 1 set out to stop depending on. Now:

--tools Read,Glob,Grep,Skill          # the availability filter — this is the bound
--allowedTools Read,Glob,Grep,Skill   # so those four never need a prompt a
                                      # headless run cannot answer
--disallowedTools <the 10>
--strict-mcp-config                   # no --mcp-config, so every server is dropped

Verified against the real CLI, not just its help text: the full flag set runs a prompt to a clean end_turn. The test asserts both flags; both probe snippets in docs/skill-authoring.md carry --tools with a ⚠️ naming which one bounds.

2. The threshold guard let NaN and negatives through — also correct, and both directions were silent. CHAIN_THRESHOLD must now be in [0, 1) (half-open: the chain bar is strict, so 1 is unreachable) and THRESHOLD in [0, 1] (inclusive: a first-move floor of 5/5 is legitimate). The message names the offending value as typed, so abc reads back as abc rather than NaN. Tested by spawning the script per bad value, since the guards live in main.

npm run test:scripts 517 → 518. npm run local:gate re-running.

[cliffhall]

Gate status, stated plainly

npm run local:gate has now run three times on this branch:

run	commits	result
1	base change	exit 0, fully clean
2	+ round 1 fixes	exit 1 — 1 of 7408: ServerImportJsonModal › guards against a live edit before the debounce re-validates
3	+ round 2 fixes	exit 1 — 1 of 7408: ServerSettingsModal › maps the OAuth insufficient-scope policy into settings (SEP-2350)

Both failures are contention, not regression, and I checked rather than assumed:

• A different file each time, neither touched by this diff — which is source-only changes to scripts/, docs/, AGENTS.md and one evals file.
• Each passed in the same run's validate stage and failed only in coverage.
• Each passes in isolation: ServerImportJsonModal 16/16 in 11.3s (vs 24.9s in the loaded run), ServerSettingsModal 37/37 in 22.5s (vs 90.8s).
• Both are timing-sensitive (a debounce and a policy round-trip) and both runs were on a machine another process had at load 50–155.

Every other stage was green in all three runs: lint, format, typecheck, build, the four client suites (326 / 27 / 27 / 1 files), coverage thresholds, verify:build-gate, verify:bundle-externals, the CLI/TUI/web/Firefox smokes, and Storybook (514 tests).

Also merged v2/main into this branch to pick up #2246 — the sync that was pushed here — rather than force-pushing over it. npm run test:scripts is 518 passing on the merged tree and verify:skills is OK at 3234/4000.

[Copilot]

🟡 Changes recommended

Flattening calls across assistant messages can falsely report parallel skill invocations as a causal hand-off.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Correcting my earlier gate note

I attributed the run-2 and run-3 failures to machine contention. A fourth run shows that was only half right, so the earlier comment overstated the case.

run	stage	machine	result
1	—	quiet	clean, exit 0
2	coverage	load 50–126	ServerImportJsonModal › debounce guard, 24 945 ms
3	coverage	load ~150	ServerSettingsModal › SEP-2350, 90 787 ms
4	validate	load ~18–40	ServerImportJsonModal › debounce guard again, 11 275 ms / 1 567 ms

Run 4 failed on a quiet machine, in a different stage, with the test itself taking 1.6 s. Contention does not explain that. The honest read is a real timing race that parallelism makes more likely — not merely a slow box — and I have filed it as #2250 (v2 / bug / Todo / High) rather than papering over it.

What has not changed is that this PR cannot be the cause, and that is checkable rather than a judgement call:

$ git diff origin/v2/main...HEAD --name-only | grep -c "^clients/"
0

The diff touches .claude/skills/test-servers/evals/evals.json, AGENTS.md, docs/skill-authoring.md and five files under scripts/ — nothing the web vitest run loads. In isolation the test is stable at 6/6, ~11 s each.

Everything else was green across all four runs: lint, format, typecheck, build, the CLI/TUI/launcher suites, coverage thresholds, verify:build-gate, verify:bundle-externals, all four smokes, and Storybook.

[Copilot]

🟢 Approval recommended

The implementation satisfies #2204 with comprehensive validation, isolation, reporting, and regression coverage.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation satisfies the stated acceptance criteria with comprehensive validation, containment, and regression coverage.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review loop complete — two consecutive 🟢 approvals, zero comments

round	verdict	findings	addressed in
1	🟡 changes recommended	5	f20bbac4
2	🟡 changes recommended	2	3ec12572
3	🟡 changes recommended	1	313a5809
4	🟢 approval recommended	0	—
5	🟢 approval recommended	0	—

Round 5 was requested deliberately: one zero-comment round is not a stopping condition, since Copilot's inline comments can lag its review body. Two in a row is.

Every one of the eight findings was real, and three of them changed what this PR measures rather than just how it reads:

• The chain prompts carried test-servers' own trigger, so the ordered pair could score without the hand-off. Removing the cue dropped the rate from 100%/67% to 33%/33% — the size of the artefact.
• --allowedTools grants permission but does not restrict availability, so round 1's containment fix was only half a fix until --tools was added.
• Position in the stream is not causation: two Skill blocks in one assistant message are parallel guesses, and scoring them as a hand-off was a second route to the same false pass.

The feature therefore ships having already caught two ways a chained case can lie about itself, both written up in docs/skill-authoring.md so the next person writing one does not rediscover them.

Two issues filed from this work rather than folded in:

• The testing → test-servers hand-off fires only a third of the time #2247 — the testing → test-servers hand-off is weak (33–100% across RUNS=3 runs). The red column is the finding, not a defect in the harness.
• Flaky web test: ServerImportJsonModal debounce guard fails under a full parallel run #2250 — ServerImportJsonModal › debounce guard is a pre-existing flaky web test. This PR touches zero files under clients/ (git diff origin/v2/main...HEAD --name-only | grep -c "^clients/" → 0), and the test passes 6/6 in isolation.

[cliffhall]

CI failure is v2/main’s, not this PR’s — filed as #2252

The coverage job failed on:

FAIL clients/web/src/test/core/auth/revocation.test.ts
  > revokeStoredOAuthTokens (plan + execute)
  > shares one deadline across grants instead of one per grant
AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times

That test is not reachable from this PR, and the evidence is not a judgement call:

1. The diff touches nine files — .claude/, AGENTS.md, docs/, scripts/ — and none under clients/:
   git diff origin/v2/main...HEAD --name-only | grep -c "^clients/" → 0.
2. The test arrived on this branch through the v2/main sync (test(auth): make the revocation fixture a real form-urldecoder #2246, commit 40335235), which is in the merge base.
3. It failed on v2/main itself at da47506e, in both build and coverage — run 33930912178 — and 0373bf9e (chore(deps): refresh fast-uri, qs and browserslist to clear npm audit #2249) then went green with no change to that code.
4. Locally it passes 3/3.

The race

executeRevocationPlan shares one budget: remainingMs = deadlineAt - Date.now(); if (remainingMs <= 0) skip. The test hangs every fetch so grant a should burn all 30 ms and b/c should never be attempted. But grant a’s abort is a setTimeout, and a timer that fires a fraction early leaves remainingMs marginally positive — so grant b is issued with a near-zero budget and the count is 2. The assertion sits exactly on a boundary timer resolution can land either side of.

Filed as #2252 (v2 / bug / Todo / High) with both candidate fixes weighed — an epsilon or performance.now() in core/auth/revocation.ts being the better one. I have not fixed it here: it is v2/main’s bug, and this PR deliberately touches no client source.

I have re-run the failed jobs.