chore: address Copilot review round 19

The `local:gate` half of the wiring vouch used `reachableScripts`, which
extracts any `npm run …` substring — so `echo npm run verify:skills:cli && npm
run coverage` satisfied it while the validator never ran. Same over-match the CI
half had in round 15, from the other side.

`scriptChainRuns` walks the script graph instead, splitting each body on the
separators that begin a new command and requiring an exact invocation of the
target. A segment naming some other script is followed with or without trailing
flags, since `npm run coverage --silent` still runs it, and `seen` makes a
cyclic graph terminate. Regression cases for the mention, a longer script name,
and the cycle; verified by mutation.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-29 05:18:18 -04:00
commit b91b746867
2 changed files with 115 additions and 2 deletions

View file

@ -28,6 +28,7 @@ import {
checkWiring,
ciRunsUnconditionally,
runsCommand,
scriptChainRuns,
} from "./verify-skills.mjs";
const SCRIPT = path.join(
@ -408,3 +409,77 @@ test("ciRunsUnconditionally requires the workflow to fire on ordinary changes",
);
assert.equal(ciRunsUnconditionally(step, C), false);
});
test("scriptChainRuns follows the chain to a real invocation", () => {
const T = "verify:skills:cli";
assert.equal(
scriptChainRuns(
{ "local:gate": `npm run validate && npm run ${T}` },
"local:gate",
T,
),
true,
);
// Through an intermediate script, with or without flags on the way.
assert.equal(
scriptChainRuns(
{ "local:gate": "npm run validate", validate: `npm run ${T}` },
"local:gate",
T,
),
true,
);
assert.equal(
scriptChainRuns(
{ "local:gate": "npm run validate --silent", validate: `npm run ${T}` },
"local:gate",
T,
),
true,
);
});
test("scriptChainRuns is not satisfied by a mention or a longer name", () => {
// `reachableScripts` extracts any `npm run …` substring, so both of these
// passed the old check while the validator never ran.
const T = "verify:skills:cli";
assert.equal(
scriptChainRuns(
{ "local:gate": `echo npm run ${T} && npm run coverage` },
"local:gate",
T,
),
false,
);
assert.equal(
scriptChainRuns({ "local:gate": `npm run ${T}:disabled` }, "local:gate", T),
false,
);
assert.equal(
scriptChainRuns({ "local:gate": "npm run coverage" }, "local:gate", T),
false,
);
assert.equal(scriptChainRuns({}, "local:gate", T), false);
});
test("scriptChainRuns terminates on a cyclic script graph", () => {
assert.equal(
scriptChainRuns(
{ "local:gate": "npm run a", a: "npm run b", b: "npm run a" },
"local:gate",
"verify:skills:cli",
),
false,
);
});
test("checkWiring rejects a local:gate that only mentions the validator", () => {
const scripts = {
...WIRED_SCRIPTS,
"local:gate": "echo npm run verify:skills:cli && npm run coverage",
};
assert.match(
checkWiring(scripts, WIRED_WORKFLOW).join(),
/local:gate` no longer runs `verify:skills:cli`/,
);
});

View file

@ -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 { reachableScripts, rootReachesScript } from "./lib/npm-scripts.mjs";
import { rootReachesScript } from "./lib/npm-scripts.mjs";
import { parse as parseYaml } from "yaml";
import {
formatClaudeVersion,
@ -82,6 +82,44 @@ export function runsCommand(script, command) {
.some((segment) => segment === command);
}
/**
* Whether `npm run <entry>` reaches an invocation of `npm run <target>`.
*
* `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.
*
* 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<string, string>} scripts The root manifest's `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);
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;
}
/**
* Whether a workflow's `on:` includes an event that fires for ordinary changes.
*
@ -182,7 +220,7 @@ export function checkWiring(rootScripts, workflowText) {
"the root `validate` no longer runs `verify:format-coverage` (a sibling guard). Restore it.",
);
}
if (!reachableScripts(rootScripts, "local:gate").has("verify:skills:cli")) {
if (!scriptChainRuns(rootScripts, "local:gate", "verify:skills:cli")) {
problems.push(
"`npm run local:gate` no longer runs `verify:skills:cli`, so the authoritative validator never runs locally. Restore it.",
);