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

feat(skills): address Copilot review round 3 on #2204

Browse source 
One finding, and a sharp one: position in the stream is not causation.

`collectSkillInvocations` flattened every `tool_use` block across every
assistant event into one ordered array, and `chainHit` read position
alone. But the model can emit several `Skill` blocks in a SINGLE assistant
message, and it has not seen the first skill's body when it does — those
are parallel guesses, not a hand-off. A run that emitted `testing` and
`test-servers` together in one message scored as `A` led to `B`, and would
have gone on scoring after the pointer in `testing`'s body was deleted:
exactly the false pass this feature exists to rule out, arriving by a
second route (Copilot).

Each recorded invocation now carries the assistant event it came from —
`{payload, turn}` — and `chainHit` requires every link after the first to
land in a STRICTLY later turn. The turn count is threaded through
`collectSkillInvocations`'s `turnOffset`/`nextTurn` so a stream read in
chunks stays monotonic rather than restarting per read; without that, a
chunk boundary would reset the numbering and reintroduce the same bug on
long runs.

The scan stays greedy, which is still correct under the constraint: taking
the earliest occurrence of a link can only leave more room for the rest,
so no later starting point succeeds where the greedy one fails.

`sampleHit` is deliberately unaffected — a first-move case asks only
whether a skill fired at all, so it reads names and ignores turns.

Four new assertions in one case: a same-turn pair scores false; the same
two skills a turn apart score true; a same-turn pair does not poison a
later genuine hand-off; and only the chain's first link is unconstrained.
Plus a `collectSkillInvocations` case pinning the turn tagging and the
cross-chunk offset.

`docs/skill-authoring.md` now lists this as the second way a chained case
can false-pass, alongside a prompt carrying the target's own trigger.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-04 20:28:37 -04:00
commit 313a580982
3 changed files with 168 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

10
docs/skill-authoring.md
View file

@ -216,7 +216,7 @@ for everything else — a prompt someone would actually type to reach this skill
directly is a first-move case even when a hand-off could also get there, and it
is the cheaper measurement by an order of magnitude.
Five rules the shape enforces, each for a reason worth knowing:
Six rules the shape enforces, each for a reason worth knowing:
- **The chain ends with the skill whose file it lives in.** The case exists to
measure whether _this_ skill is reachable, so the file that must go red when
@ -232,6 +232,14 @@ Five rules the shape enforces, each for a reason worth knowing:
run.** The model may load something before the chain starts and something
unrelated in between; neither changes the claim that A led to B. What does not
score is the reverse order.
- **Every link after the first must land in a later assistant turn.** Position
in the stream is not causation: the model can emit several `tool_use` blocks
in one message, and it has not seen the first skill's body when it does — so
two `Skill` calls in the same turn are parallel guesses, not a hand-off, and
a flat index would score them as one (Copilot). This is the difference
between "B was loaded after A" and "A led to B", and it is the second way a
chained case can false-pass — the first being a prompt that carries the
target's own trigger, below. Only the chain's *first* link is unconstrained.
- **Repeats and unknown links are rejected.** A repeated link cannot be
observed, and a link naming a skill the model cannot invoke can never fire —
it would score a permanent 0% that reads as a description problem.

78
scripts/skill-eval.mjs
View file

@ -187,15 +187,27 @@ export function collectCases(only, skillsDir = SKILLS_DIR) {
* run that loaded B, then A, then B again indistinguishable from one that never
* reached B from A (#2204).
*
* Each entry also carries the **assistant event** it came from, and that
* boundary is what makes the order mean something. The model may emit several
* `tool_use` blocks in one message, and it cannot see the first skill's body
* until the message after — so two `Skill` calls in the SAME event are
* concurrent guesses, not a hand-off, however they happen to be ordered inside
* the array. Flattening the stream and reading position alone would score that
* as `A` leading to `B` (Copilot); `chainHit` requires a later turn instead.
*
* @param {string} text One or more newline-delimited JSON events. A trailing
* partial line is ignored, so this can be fed incrementally.
* @returns {{ invoked: string[], rest: string, result: string | null }}
* @param {number} [turnOffset] Assistant events already seen, so a stream fed
* in chunks keeps one monotonic turn count rather than restarting per chunk.
* @returns {{ invoked: {payload: string, turn: number}[], rest: string,
* result: string | null, nextTurn: number }}
*/
export function collectSkillInvocations(text) {
export function collectSkillInvocations(text, turnOffset = 0) {
const lines = text.split("\n");
const rest = lines.pop() ?? "";
const invoked = [];
let result = null;
let turn = turnOffset;
for (const line of lines) {
if (!line.trim()) continue;
let evt;
@ -207,13 +219,26 @@ export function collectSkillInvocations(text) {
}
if (evt?.type === "result") result = evt.subtype ?? null;
if (evt?.type !== "assistant") continue;
// One assistant event is one turn: everything inside it was decided at
// once, before any of its results came back.
turn++;
for (const block of evt.message?.content ?? []) {
if (block?.type !== "tool_use" || block.name !== "Skill") continue;
// Don't assume the input field's name — match on the whole payload.
invoked.push(JSON.stringify(block.input ?? {}));
invoked.push({ payload: JSON.stringify(block.input ?? {}), turn });
}
}
return { invoked, rest, result };
return { invoked, rest, result, nextTurn: turn };
}
/**
* The skill names one recorded invocation asked for.
*
* @param {{payload: string} | string} entry
* @returns {string[]}
*/
function entryNames(entry) {
return invokedSkillNames(typeof entry === "string" ? entry : entry.payload);
}
/**
@ -281,12 +306,15 @@ export function invokedSkillNames(payload) {
* false failure about someone else's environment rather than about these
* skills (Copilot).
*
* Turn boundaries are irrelevant here — a first-move case asks only whether a
* skill fired at all — so this reads the names and ignores the rest.
*
* @param {string | null} expect Skill name, or null for a negative case.
* @param {Iterable<string>} invoked
* @param {Iterable<{payload: string} | string>} invoked
* @param {Set<string> | null} [ours] Repo skill names. Null counts any skill.
*/
export function sampleHit(expect, invoked, ours = null) {
const names = [...invoked].flatMap(invokedSkillNames);
const names = [...invoked].flatMap(entryNames);
if (expect === null) {
return ours === null ? names.length === 0 : !names.some((n) => ours.has(n));
}
@ -302,19 +330,35 @@ export function sampleHit(expect, invoked, ours = null) {
* and it may well load something before the chain's first link — neither
* changes the fact that A led to B, which is the only claim the case makes.
*
* Every link after the first must land in a **strictly later assistant turn**
* than the one before. Position in the stream is not causation: the model can
* emit several `tool_use` blocks in one message, and it has not seen the first
* skill's body when it does, so two `Skill` calls in the same turn are parallel
* guesses that a flat index would happily score as a hand-off (Copilot). This
* is the whole difference between "B was loaded after A" and "A led to B".
*
* The scan stays greedy, which is still correct under that constraint: taking
* the EARLIEST occurrence of a link can only leave more room for the rest, so
* no later starting point could succeed where the greedy one fails.
*
* Nothing is asserted about foreign skills here, unlike a negative case. A
* hand-off case names exactly what it wants and a contributor's own
* `~/.claude/skills` entry firing alongside it says nothing either way.
*
* @param {string[]} chain Ordered skill names, ending with the owning skill.
* @param {Iterable<string>} invoked
* @param {Iterable<{payload: string, turn: number}>} invoked
* @returns {boolean}
*/
export function chainHit(chain, invoked) {
const names = [...invoked].flatMap(invokedSkillNames);
let want = 0;
for (const name of names) {
if (name === chain[want]) want++;
let prevTurn = -Infinity;
for (const entry of invoked) {
if (!entryNames(entry).includes(chain[want])) continue;
// A link in the same turn as the previous one cannot have been caused by
// it — the model had not seen that skill's body yet.
if (want > 0 && !(entry.turn > prevTurn)) continue;
prevTurn = entry.turn;
want++;
if (want === chain.length) return true;
}
return false;
@ -402,7 +446,8 @@ const DISALLOWED_TOOLS = [
*
* @param {string} prompt
* @param {{ spawnFn?: typeof spawn, cwd?: string, maxTurns?: number }} [opts]
* @returns {Promise<string[]>} Skill payloads, in the order they fired.
* @returns {Promise<{payload: string, turn: number}[]>} Skill invocations, in
* the order they fired, each tagged with the assistant turn it came from.
*/
export function runPrompt(
prompt,
@ -447,10 +492,17 @@ export function runPrompt(
let buf = "";
const invoked = [];
let result = null;
// Carried across chunks so the turn count is monotonic over the whole
// stream rather than restarting at each read.
let turnOffset = 0;
p.stdout.on("data", (chunk) => {
const parsed = collectSkillInvocations(buf + chunk.toString());
const parsed = collectSkillInvocations(
buf + chunk.toString(),
turnOffset,
);
buf = parsed.rest;
for (const payload of parsed.invoked) invoked.push(payload);
turnOffset = parsed.nextTurn;
for (const entry of parsed.invoked) invoked.push(entry);
if (parsed.result !== null) result = parsed.result;
});
p.on("error", reject);

100
scripts/skill-eval.test.mjs
View file

@ -30,8 +30,16 @@ import {
} from "./skill-eval.mjs";
import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs";
/** The payloads a run records, in the order the skills fired. */
const fired = (...names) => names.map((n) => JSON.stringify({ skill: n }));
/**
* What a run records, one skill per assistant turn — the shape a genuine
* hand-off has.
*/
const fired = (...names) =>
names.map((n, i) => ({ payload: JSON.stringify({ skill: n }), turn: i + 1 }));
/** Several skills emitted together in ONE assistant turn. */
const firedTogether = (turn, ...names) =>
names.map((n) => ({ payload: JSON.stringify({ skill: n }), turn }));
/** The eval script itself, for the cases that must exercise `main`'s guards. */
const SCRIPT_PATH = fileURLToPath(new URL("./skill-eval.mjs", import.meta.url));
@ -49,7 +57,8 @@ test("collectSkillInvocations finds Skill tool_use payloads", () => {
assistant(skillUse("testing")) + "\n",
);
assert.equal(invoked.length, 1);
assert.ok(invoked[0].includes("testing"));
assert.ok(invoked[0].payload.includes("testing"));
assert.equal(invoked[0].turn, 1);
});
test("collectSkillInvocations ignores other tools and other event types", () => {
@ -79,7 +88,9 @@ test("collectSkillInvocations holds back a trailing partial line", () => {
test("collectSkillInvocations tolerates a tool_use with no input", () => {
const text = assistant({ type: "tool_use", name: "Skill" }) + "\n";
assert.deepEqual([...collectSkillInvocations(text).invoked], ["{}"]);
assert.deepEqual(collectSkillInvocations(text).invoked, [
{ payload: "{}", turn: 1 },
]);
});
test("sampleHit scores positive and negative cases", () => {
@ -104,8 +115,85 @@ test("collectSkillInvocations preserves order and repeats", () => {
].join("\n") + "\n";
const { invoked } = collectSkillInvocations(text);
assert.deepEqual(
invoked.map((p) => JSON.parse(p).skill),
["test-servers", "testing", "test-servers"],
invoked.map((e) => [JSON.parse(e.payload).skill, e.turn]),
[
["test-servers", 1],
["testing", 2],
["test-servers", 3],
],
);
});
test("collectSkillInvocations tags each invocation with its assistant turn", () => {
// Two `Skill` blocks in ONE message share a turn: the model chose both before
// seeing either result, so nothing in that message can have caused anything
// else in it. The turn is what lets `chainHit` tell that apart from a
// hand-off; a flat index cannot.
const text =
[
assistant(skillUse("testing"), skillUse("test-servers")),
assistant(skillUse("board-ops")),
].join("\n") + "\n";
const { invoked, nextTurn } = collectSkillInvocations(text);
assert.deepEqual(
invoked.map((e) => [JSON.parse(e.payload).skill, e.turn]),
[
["testing", 1],
["test-servers", 1],
["board-ops", 2],
],
);
// The count carries across chunks, so a stream read in pieces stays monotonic.
assert.equal(nextTurn, 2);
const more = collectSkillInvocations(
assistant(skillUse("local-dev")) + "\n",
nextTurn,
);
assert.equal(more.invoked[0].turn, 3);
});
test("chainHit refuses two skills loaded in the same assistant turn", () => {
// The finding this pins: the model can emit several `tool_use` blocks in one
// message, and it has NOT seen the first skill's body when it does. So an
// [A, B] pair from one message is two parallel guesses, and scoring it as a
// hand-off would report a causal link that cannot exist — a case that would
// keep passing after the pointer in `testing` was deleted.
assert.equal(
chainHit(
["testing", "test-servers"],
firedTogether(1, "testing", "test-servers"),
),
false,
);
// The same two skills one turn apart is the real thing.
assert.equal(
chainHit(
["testing", "test-servers"],
[...firedTogether(1, "testing"), ...firedTogether(2, "test-servers")],
),
true,
);
// A same-turn pair does not poison a later genuine hand-off.
assert.equal(
chainHit(
["testing", "test-servers"],
[
...firedTogether(1, "testing", "test-servers"),
...firedTogether(2, "test-servers"),
],
),
true,
);
// Only the FIRST link is unconstrained; every later one needs a new turn.
assert.equal(
chainHit(
["local-dev", "testing", "test-servers"],
[
...firedTogether(1, "local-dev"),
...firedTogether(2, "testing", "test-servers"),
],
),
false,
);
});