chore: address Copilot review round 8

- skill-eval: `only` no longer narrows `ours`. A negative case means "no skill
  of this repo fired", so filtering the repo's other model-invoked skills out of
  that set let `skills:eval -- testing` score a `local-dev` invocation as
  somebody else's skill and pass a negative case it should fail. Every
  model-invoked skill is now inspected; `only` applies when enqueueing.
  `collectCases` takes a skills directory so the focused-mode regression is
  actually testable — verified by mutation: restoring the early filter fails it.
- The workflow's `validate` step comment said two coverage guards; it runs four.

Signed-off-by: cliffhall <cliff@futurescale.com>

.github/workflows/main.yml

@@ -38,8 +38,9 @@ jobs:
         run: npm install
 
       - name: Validate (coverage guards, format, lint, typecheck, build, fast tests)
-        # Runs the two coverage guards first (verify:format-coverage +
-        # verify:typecheck-coverage), then validate:core, then each client's
+        # Runs the four durable guards first (verify:format-coverage,
+        # verify:skills, verify:typecheck-coverage, verify:dep-lockstep), then
+        # test:scripts, then validate:core, then each client's
         # self-validation: format:check + lint + typecheck + build + test (no
         # coverage instrumentation — fast). This also builds every client bundle
         # (web dist, cli/tui/launcher) that the smokes below need. The heavier

scripts/skill-eval.mjs

@@ -48,14 +48,27 @@ const RUNS = Number(process.env.RUNS ?? 3);
 const CONCURRENCY = Number(process.env.CONCURRENCY ?? 4);
 
 /** Collect the committed cases for every model-invoked skill (optionally one). */
-function collectCases(only) {
+/**
+ * Collect the committed cases, and the set of skill names that are OURS.
+ *
+ * `only` narrows which cases run, but it must NOT narrow `ours`: a negative
+ * case means "no skill of this repo fired", so excluding the repo's other
+ * model-invoked skills from that set would let `skills:eval -- testing` score a
+ * `local-dev` invocation as foreign and pass a negative case it should fail
+ * (Copilot). Every model-invoked skill is inspected; `only` is applied when
+ * enqueueing.
+ *
+ * @param {string | undefined} only
+ * @param {string} [skillsDir]
+ * @returns {{ cases: object[], ours: Set<string> }}
+ */
+export function collectCases(only, skillsDir = SKILLS_DIR) {
   const cases = [];
   const ours = new Set();
-  for (const dir of readdirSync(SKILLS_DIR).sort()) {
-    if (only && dir !== only) continue;
-    const skillFile = path.join(SKILLS_DIR, dir, "SKILL.md");
+  for (const dir of readdirSync(skillsDir).sort()) {
+    const skillFile = path.join(skillsDir, dir, "SKILL.md");
     if (
-      !statSync(path.join(SKILLS_DIR, dir)).isDirectory() ||
+      !statSync(path.join(skillsDir, dir)).isDirectory() ||
       !existsSync(skillFile)
     )
       continue;
@@ -71,7 +84,8 @@ function collectCases(only) {
     }
     if (!skill.modelInvoked) continue;
     ours.add(dir);
-    const evalsFile = path.join(SKILLS_DIR, dir, "evals", "evals.json");
+
+    const evalsFile = path.join(skillsDir, dir, "evals", "evals.json");
     if (!existsSync(evalsFile)) {
       throw new Error(
         `${dir} is model-invoked but has no evals/evals.json. Run \`npm run verify:skills\`.`,
@@ -89,6 +103,7 @@ function collectCases(only) {
     if (invalid.length > 0) {
       throw new Error(`${dir}/evals/evals.json: ${invalid.join("; ")}`);
     }
+    if (only && dir !== only) continue;
     for (const c of parsed) cases.push({ ...c, from: dir });
   }
   return { cases, ours };

scripts/skill-eval.test.mjs

@@ -11,7 +11,11 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 import { EventEmitter } from "node:events";
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import path from "node:path";
 import {
+  collectCases,
   collectSkillInvocations,
   runRejection,
   invokedSkillNames,
@@ -262,3 +266,78 @@ test("runPrompt asks for a shell on Windows", () => {
   }).catch(() => {});
   assert.equal(seen.shell, true);
 });
+
+/** A skills directory with two model-invoked skills and one name-only skill. */
+function skillsFixture() {
+  const root = mkdtempSync(path.join(tmpdir(), "skill-eval-"));
+  const write = (name, frontmatter, evals) => {
+    const dir = path.join(root, name);
+    mkdirSync(dir, { recursive: true });
+    writeFileSync(
+      path.join(dir, "SKILL.md"),
+      `---\nname: ${name}\ndescription: d\n${frontmatter}---\n\nBody\n`,
+    );
+    if (evals) {
+      mkdirSync(path.join(dir, "evals"), { recursive: true });
+      writeFileSync(
+        path.join(dir, "evals", "evals.json"),
+        JSON.stringify(evals),
+      );
+    }
+  };
+  write("alpha", "disable-model-invocation: false\n", [
+    { prompt: "a+", expect: "alpha" },
+    { prompt: "a-", expect: null },
+  ]);
+  write("beta", "disable-model-invocation: false\n", [
+    { prompt: "b+", expect: "beta" },
+    { prompt: "b-", expect: null },
+  ]);
+  write("gamma", "disable-model-invocation: true\n");
+  return root;
+}
+
+test("focused mode narrows the cases but not the repo's own skill set", () => {
+  // `ours` decides what a NEGATIVE case counts as a false trigger. Narrowing it
+  // with `only` would let `skills:eval -- alpha` score a `beta` invocation as
+  // somebody else's skill and pass a negative case it should fail.
+  const root = skillsFixture();
+
+  const all = collectCases(undefined, root);
+  assert.deepEqual([...all.ours].sort(), ["alpha", "beta"]);
+  assert.equal(all.cases.length, 4);
+
+  const focused = collectCases("alpha", root);
+  assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]);
+  assert.deepEqual(
+    focused.cases.map((c) => c.prompt),
+    ["a+", "a-"],
+  );
+  // The consequence, stated as the assertion that matters:
+  assert.equal(
+    sampleHit(null, new Set(['{"skill":"beta"}']), focused.ours),
+    false,
+  );
+
+  rmSync(root, { recursive: true, force: true });
+});
+
+test("a name-only skill is not part of the repo's model-invoked set", () => {
+  const root = skillsFixture();
+  const { ours } = collectCases(undefined, root);
+  assert.equal(ours.has("gamma"), false);
+  // So its firing does not fail a negative case — it cannot fire on its own.
+  assert.equal(sampleHit(null, new Set(['{"skill":"gamma"}']), ours), true);
+  rmSync(root, { recursive: true, force: true });
+});
+
+test("collection fails loudly rather than silently shrinking the set", () => {
+  const root = skillsFixture();
+  // A model-invoked skill whose evals vanished.
+  rmSync(path.join(root, "beta", "evals"), { recursive: true, force: true });
+  assert.throws(
+    () => collectCases(undefined, root),
+    /beta is model-invoked but has no/,
+  );
+  rmSync(root, { recursive: true, force: true });
+});