diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 8bf29e76..0bab8872 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -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. diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index a043f2ac..748cdaba 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -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} invoked + * @param {Iterable<{payload: string} | string>} invoked * @param {Set | 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} 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} 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); diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 491a7098..97d4b5b5 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -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, ); });