chore: address Copilot review round 10
The CI half of the wiring vouch matched the workflow's raw text, so a YAML comment, a step `name:`, or an unused anchor mentioning `npm run verify:skills:cli` satisfied it — including this guard's own docblock wording. Deleting the real step would then have left `verify:skills` green, which is the vacuous-guard failure the vouch exists to prevent. It now reads the workflow's executable regions via `extractExecutableRegions`, the same helper the #2146 workflow gate is built on, and matches only a `run:`. Covered by three should-not-count cases (comment, step name, workflow input) plus a multi-line `run:` that must count; verified by mutation — restoring the substring check fails them. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
17068fc89e
commit
23c59d7a17
2 changed files with 45 additions and 4 deletions
|
|
@ -236,12 +236,15 @@ test("checkWiring catches the authoritative validator dropped from CI", () => {
|
|||
WIRED_SCRIPTS,
|
||||
"jobs:\n build:\n steps:\n - run: npm run validate\n",
|
||||
).join(),
|
||||
/workflows\/main\.yml` no longer runs/,
|
||||
/has no step that runs/,
|
||||
);
|
||||
});
|
||||
|
||||
test("checkWiring reports every broken link at once", () => {
|
||||
assert.equal(checkWiring({ validate: "", "local:gate": "" }, "").length, 3);
|
||||
assert.equal(
|
||||
checkWiring({ validate: "", "local:gate": "" }, "jobs: {}\n").length,
|
||||
3,
|
||||
);
|
||||
});
|
||||
|
||||
test("the repository as it stands is wired", () => {
|
||||
|
|
@ -259,3 +262,27 @@ test("the repository as it stands is wired", () => {
|
|||
);
|
||||
assert.deepEqual(checkWiring(scripts, workflow), []);
|
||||
});
|
||||
|
||||
test("checkWiring is not satisfied by a mention outside an executable step", () => {
|
||||
// A raw substring match would pass on any of these while CI ran nothing —
|
||||
// including on a step whose NAME describes the check it no longer performs,
|
||||
// which is exactly how a wiring guard goes quietly vacuous.
|
||||
const mentions = [
|
||||
"# npm run verify:skills:cli\njobs:\n build:\n steps:\n - run: npm run validate\n",
|
||||
"jobs:\n build:\n steps:\n - name: npm run verify:skills:cli\n run: npm run validate\n",
|
||||
"on:\n workflow_dispatch:\n inputs:\n cmd:\n default: npm run verify:skills:cli\njobs:\n build:\n steps:\n - run: npm run validate\n",
|
||||
];
|
||||
for (const workflow of mentions) {
|
||||
assert.match(
|
||||
checkWiring(WIRED_SCRIPTS, workflow).join(),
|
||||
/has no step that runs/,
|
||||
`should not count: ${workflow.split("\n")[0]}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test("checkWiring accepts the command inside a multi-line run step", () => {
|
||||
const workflow =
|
||||
"jobs:\n build:\n steps:\n - run: |\n npm run validate\n npm run verify:skills:cli\n";
|
||||
assert.deepEqual(checkWiring(WIRED_SCRIPTS, workflow), []);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ 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 { extractExecutableRegions } from "./lib/workflow-gate.mjs";
|
||||
import {
|
||||
compareVersions,
|
||||
parseClaudeVersion,
|
||||
|
|
@ -73,6 +74,14 @@ export function skillDirs(
|
|||
* the step that actually runs the authoritative validator, and it is
|
||||
* reachable from neither `validate` nor any test, so nothing else would
|
||||
* notice it going missing.
|
||||
*
|
||||
* The workflow half reads the **executable** regions rather than the file's
|
||||
* text. A raw substring match is satisfied by a comment, a step `name:`, or an
|
||||
* unused anchor that merely mentions the command — so deleting the real step
|
||||
* while leaving this very docblock's wording nearby would keep the guard green
|
||||
* (Copilot). `extractExecutableRegions` already descends only the schema paths
|
||||
* Actions actually runs, with aliases resolved, and it is the same helper the
|
||||
* #2146 workflow gate is built on.
|
||||
*/
|
||||
export function checkWiring(rootScripts, workflowText) {
|
||||
const problems = [];
|
||||
|
|
@ -86,9 +95,14 @@ export function checkWiring(rootScripts, workflowText) {
|
|||
"`npm run local:gate` no longer runs `verify:skills:cli`, so the authoritative validator never runs locally. Restore it.",
|
||||
);
|
||||
}
|
||||
if (!/npm run verify:skills:cli/.test(workflowText)) {
|
||||
const runsInCi = extractExecutableRegions(workflowText, "main.yml").some(
|
||||
(region) =>
|
||||
region.kind === "run" &&
|
||||
/\bnpm run verify:skills:cli\b/.test(region.text),
|
||||
);
|
||||
if (!runsInCi) {
|
||||
problems.push(
|
||||
"`.github/workflows/main.yml` no longer runs `npm run verify:skills:cli`, so the authoritative validator never runs in CI. Restore it.",
|
||||
"`.github/workflows/main.yml` has no step that runs `npm run verify:skills:cli`, so the authoritative validator never runs in CI. Restore it.",
|
||||
);
|
||||
}
|
||||
return problems;
|
||||
|
|
|
|||
Loading…
Reference in a new issue