chore: address Copilot review round 20
All three findings are the same shape: a check that accepts a command which
exists but cannot gate.
- Splitting on `|` and `&` conflated `||`, so both failure-masking forms counted:
`npm run X || true` swallows a rejection and `true || npm run X` never runs
the validator. Both `runsCommand` and the chain walk now split on `\n`, `;`
and `&&` only, which refuses each without special-casing either.
- A CI step (or job) carrying `continue-on-error` asserts nothing, since a
rejected skill leaves the workflow green. Both are now skipped, treating a
string `"true"` and an unevaluable `${{ }}` expression as masking on the same
conservative principle as the `if:` checks; an explicit `false` still counts.
- `rootReachesScript` was substring-based, so a `validate` reading `echo npm run
verify:skills` satisfied the sibling vouch while the guard never executed —
the cycle protecting nothing. It now uses the exact chain walk, which is
therefore shared in `lib/npm-scripts.mjs` and fixes the same hole in all four
vouches, not just this PR's. `reachableScripts` keeps its substring behavior
for the harvesting callers, where over-inclusion is safe.
The chain walk also follows npm's implicit pre/post hooks, with a test — a guard
invoked from a `prevalidate` is genuinely run, and reporting it missing would be
a false failure introduced by the fix.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
b91b746867
commit
a2ddece7a5
4 changed files with 160 additions and 46 deletions
|
|
@ -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 <entry>` reaches a real invocation of `npm run <target>`.
|
||||
*
|
||||
* `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<string, string>} 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<name>/post<name> implicitly around <name>.
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 <entry>` reaches an invocation of `npm run <target>`.
|
||||
* 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<string, string>} 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;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue