diff --git a/scripts/lib/npm-scripts.mjs b/scripts/lib/npm-scripts.mjs index fbb616de..0427c45d 100644 --- a/scripts/lib/npm-scripts.mjs +++ b/scripts/lib/npm-scripts.mjs @@ -85,5 +85,49 @@ export function rootRunsClientValidate(rootScripts, clientDir) { * `validate` is caught by its sibling. Only deleting both slips through. */ export function rootReachesScript(rootScripts, scriptName) { - return reachableScripts(rootScripts, "validate").has(scriptName); + return scriptChainRuns(rootScripts, "validate", scriptName); +} + +/** + * Whether `npm run ` reaches a real invocation of `npm run `. + * + * `reachableScripts` matches every `npm run …` **substring**, which is right for + * harvesting (over-inclusion is safe there) and wrong for a vouch: `echo npm run + * verify:skills` would satisfy it while the guard never executes (Copilot). + * + * Splitting is on `\n`, `;` and `&&` only — deliberately NOT on `|` or a bare + * `&`, because `||` is the failure-masking form and both of its shapes must be + * rejected: `npm run X || true` swallows the guard's own failure, and `true || + * npm run X` never runs it. Leaving them inside a segment means the segment does + * not equal the invocation, so both are refused without special-casing either. + * + * A segment naming some OTHER script is followed, with or without trailing + * flags, as are npm's implicit `pre`/`post` hooks. Only the target is matched + * exactly, which suits the argument-less scripts a vouch asks about. + * + * @param {Record} scripts + * @param {string} entry + * @param {string} target + */ +export function scriptChainRuns(scripts, entry, target) { + const seen = new Set(); + const queue = [entry]; + while (queue.length > 0) { + const name = queue.shift(); + if (seen.has(name)) continue; + seen.add(name); + // npm runs pre/post implicitly around . + for (const hook of [`pre${name}`, `post${name}`]) + if (typeof scripts?.[hook] === "string") queue.push(hook); + const body = scripts?.[name]; + if (typeof body !== "string") continue; + for (const segment of body.split(/\n|;|&&/).map((part) => part.trim())) { + if (segment === `npm run ${target}`) return true; + const tokens = segment.split(/\s+/); + if (tokens[0] === "npm" && tokens[1] === "run" && tokens[2]) { + queue.push(tokens[2]); + } + } + } + return false; } diff --git a/scripts/verify-format-coverage.main.test.mjs b/scripts/verify-format-coverage.main.test.mjs index fd0d6f63..c2480f73 100644 --- a/scripts/verify-format-coverage.main.test.mjs +++ b/scripts/verify-format-coverage.main.test.mjs @@ -44,15 +44,16 @@ function scriptsRunning(guards) { * symlink — see the note in `verify-dep-lockstep.main.test.mjs`. */ function runWithGuards(guards) { + return runWithScripts(scriptsRunning(guards)); +} + +/** Same, for a fixture whose `scripts` are built by hand. */ +function runWithScripts(scripts) { const dir = realpathSync(mkdtempSync(path.join(tmpdir(), "format-cov-"))); try { writeFileSync( path.join(dir, "package.json"), - JSON.stringify( - { name: "fixture", scripts: scriptsRunning(guards) }, - null, - 2, - ), + JSON.stringify({ name: "fixture", scripts }, null, 2), ); mkdirSync(path.join(dir, "scripts", "lib"), { recursive: true }); for (const rel of [ @@ -90,6 +91,20 @@ for (const dropped of ALL_SIBLINGS) { }); } +test("vouch: a mention is not an invocation", () => { + // `rootReachesScript` used to match any `npm run …` substring, so a `validate` + // reading `echo npm run verify:skills` satisfied the vouch while the guard + // never executed — the cycle protecting nothing (Copilot). + const scripts = scriptsRunning(ALL_SIBLINGS); + scripts.validate = scripts.validate.replace( + "npm run verify:skills", + "echo npm run verify:skills", + ); + const { status, out } = runWithScripts(scripts); + assert.equal(status, 1, out); + assert.match(out, /no longer runs `verify:skills`/); +}); + test("vouch: passes when every sibling is still wired", () => { // The run still fails afterwards — the fixture has no client manifests to // harvest globs from — so assert on the *reason*, not the exit status: no diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index 6ec67382..a4d268ca 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -28,8 +28,8 @@ import { checkWiring, ciRunsUnconditionally, runsCommand, - scriptChainRuns, } from "./verify-skills.mjs"; +import { scriptChainRuns } from "./lib/npm-scripts.mjs"; const SCRIPT = path.join( path.dirname(fileURLToPath(import.meta.url)), @@ -483,3 +483,73 @@ test("checkWiring rejects a local:gate that only mentions the validator", () => /local:gate` no longer runs `verify:skills:cli`/, ); }); + +test("runsCommand rejects failure-masking and conditional shell forms", () => { + // `npm run X || true` swallows a rejection so CI stays green; `true || npm + // run X` never runs the validator. Both are `run:` steps containing the + // command, so a separator-agnostic split counted them (Copilot). + const C = "npm run verify:skills:cli"; + for (const script of [`${C} || true`, `true || ${C}`, `${C} | tee log`]) { + assert.equal(runsCommand(script, C), false, script); + } +}); + +test("ciRunsUnconditionally rejects a step whose failure cannot fail the job", () => { + const C = "npm run verify:skills:cli"; + const head = "on:\n push:\njobs:\n build:\n"; + // `continue-on-error` means a rejected skill leaves the workflow green, so + // the step asserts nothing — on the step or on the whole job. + assert.equal( + ciRunsUnconditionally( + `${head} steps:\n - continue-on-error: true\n run: ${C}\n`, + C, + ), + false, + ); + assert.equal( + ciRunsUnconditionally( + `${head} steps:\n - continue-on-error: "true"\n run: ${C}\n`, + C, + ), + false, + ); + assert.equal( + ciRunsUnconditionally( + `on:\n push:\njobs:\n build:\n continue-on-error: true\n steps:\n - run: ${C}\n`, + C, + ), + false, + ); + // An explicit `false` is the normal case and must still count. + assert.equal( + ciRunsUnconditionally( + `${head} steps:\n - continue-on-error: false\n run: ${C}\n`, + C, + ), + true, + ); +}); + +test("scriptChainRuns rejects failure-masking chains", () => { + const T = "verify:skills:cli"; + assert.equal( + scriptChainRuns({ "local:gate": `npm run ${T} || true` }, "local:gate", T), + false, + ); + assert.equal( + scriptChainRuns({ "local:gate": `true || npm run ${T}` }, "local:gate", T), + false, + ); +}); + +test("scriptChainRuns follows npm's implicit pre/post hooks", () => { + const T = "verify:skills:cli"; + assert.equal( + scriptChainRuns( + { "local:gate": "npm run coverage", precoverage: `npm run ${T}` }, + "local:gate", + T, + ), + true, + ); +}); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 64da9515..4124302a 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -31,7 +31,7 @@ import { spawnSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { claudeSpawnArgs, probeClaudeVersion } from "./lib/claude-cli.mjs"; -import { rootReachesScript } from "./lib/npm-scripts.mjs"; +import { rootReachesScript, scriptChainRuns } from "./lib/npm-scripts.mjs"; import { parse as parseYaml } from "yaml"; import { formatClaudeVersion, @@ -72,52 +72,35 @@ export function skillDirs( * this guard checks, which take no arguments; a command that did would need * prefix matching with an argument boundary instead. * + * Splitting is on `\n`, `;` and `&&` only — deliberately NOT on `|` or a bare + * `&`. `||` is the failure-masking form, and both of its shapes must be + * rejected: `npm run X || true` swallows a rejection so CI stays green, and + * `true || npm run X` never runs the validator at all (Copilot). Leaving `||` + * inside the segment means the segment does not equal the invocation, so both + * are refused without special-casing either. + * * @param {string} script A step's `run:` block. * @param {string} command e.g. `npm run verify:skills:cli`. */ export function runsCommand(script, command) { return script - .split(/[\n;|&]+/) + .split(/\n|;|&&/) .map((segment) => segment.trim()) .some((segment) => segment === command); } /** - * Whether `npm run ` reaches an invocation of `npm run `. + * Whether an Actions boolean-ish value means "yes". * - * `reachableScripts` extracts every `npm run …` **substring**, so a gate such as - * `echo npm run verify:skills:cli && npm run coverage` satisfies it while the - * validator never runs (Copilot) — the same over-match the CI half had, from the - * other side. This walks the chain instead, splitting each script on the - * separators that begin a new command and requiring an exact invocation, the - * way `runsCommand` does. + * YAML gives `true`, but a workflow may equally write `"true"` — and an + * expression (`${{ … }}`) is unevaluable here, so it is treated as masking on + * the same conservative principle the `if:` checks use. * - * A segment naming some OTHER script is followed, with or without trailing - * flags: `npm run coverage --silent` still runs `coverage`. Only the target - * itself is matched exactly, since these gate scripts take no arguments. - * - * @param {Record} scripts The root manifest's `scripts`. - * @param {string} entry - * @param {string} target + * @param {unknown} value */ -export function scriptChainRuns(scripts, entry, target) { - const seen = new Set(); - const queue = [entry]; - while (queue.length > 0) { - const name = queue.shift(); - if (seen.has(name)) continue; - seen.add(name); - const body = scripts?.[name]; - if (typeof body !== "string") continue; - for (const segment of body.split(/[\n;|&]+/).map((s) => s.trim())) { - if (segment === `npm run ${target}`) return true; - const tokens = segment.split(/\s+/); - if (tokens[0] === "npm" && tokens[1] === "run" && tokens[2]) { - queue.push(tokens[2]); - } - } - } - return false; +function isTruthy(value) { + if (value === undefined || value === false) return false; + return value !== "false"; } /** @@ -144,10 +127,12 @@ function triggersOnPush(on) { /** * Whether the workflow has a step that runs `command` on every push. * - * Three things must hold, and the guard has needed each one added in turn: + * Four things must hold, and the guard has needed each one added in turn: * the WORKFLOW must fire on a push or pull_request (one switched to `on: - * release` still contains the step but never runs it on a PR — Copilot), the - * JOB must be unconditional, and so must the STEP. + * release` still contains the step but never runs it on a PR), the JOB must be + * unconditional, so must the STEP, and neither may carry `continue-on-error` — + * a step whose failure cannot fail the job asserts nothing, so a rejected skill + * would leave the workflow green (Copilot). * * Deliberately conservative: a job or step carrying **any** `if:` is skipped. * This guard cannot evaluate an Actions expression, and the property being @@ -173,11 +158,11 @@ export function ciRunsUnconditionally(workflowText, command) { for (const job of Object.values(jobs)) { if (job === null || typeof job !== "object") continue; - if ("if" in job) continue; + if ("if" in job || isTruthy(job["continue-on-error"])) continue; const steps = Array.isArray(job.steps) ? job.steps : []; for (const step of steps) { if (step === null || typeof step !== "object") continue; - if ("if" in step) continue; + if ("if" in step || isTruthy(step["continue-on-error"])) continue; if (typeof step.run === "string" && runsCommand(step.run, command)) return true; }