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

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>

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.

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<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) {
+  let prevTurn = -Infinity;
-    if (name === chain[want]) want++;
+  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);

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),
+    invoked.map((e) => [JSON.parse(e.payload).skill, e.turn]),
-    ["test-servers", "testing", "test-servers"],
+    [
+      ["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,
   );
 });