diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d86cce57..7515d6e6 100644 --- a/.github/workflows/main.yml +++ b/.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 diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index ad0b75c6..2c8689a9 100755 --- a/scripts/skill-eval.mjs +++ b/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 }} + */ +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 }; diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 5acb710b..7c8536f8 100644 --- a/scripts/skill-eval.test.mjs +++ b/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 }); +});