From 0798ecf5fa1d3e1989ca4d3a750bab9f600e2fd8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 04:11:25 -0400 Subject: [PATCH] chore: address Copilot review round 14 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The CI wiring vouch accepted any syntactically-executable `run:`, so a step moved into a release-only job or put behind `if: false` still reported as wired while PR CI validated nothing. It now requires an UNCONDITIONAL step: any `if:` on the job or the step disqualifies it, since this guard cannot evaluate an Actions expression and "runs sometimes" is not the property being asserted. Seven predicate cases, including the two conditional shapes and a multi-line `run:` that must still count. - The `local:gate` stage list now has one owner, `docs/quality-gate.md`. It had three copies, and they drifted apart across rounds 5, 6 and 8 of this review — which is the argument the PR itself is making, applied to itself. Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 8 ++- README.md | 2 +- scripts/verify-skills.main.test.mjs | 62 ++++++++++++++++++++++-- scripts/verify-skills.mjs | 70 +++++++++++++++++++++------ 4 files changed, 121 insertions(+), 21 deletions(-) diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index e24e7efe..f17d574a 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -23,8 +23,12 @@ npm run format # auto-fix: core/, scripts/, the shared surface, every clien npm run local:gate # several minutes ``` -`local:gate` is `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → -`verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. +The stages it runs, in order, are listed in +[`docs/quality-gate.md`](../../../docs/quality-gate.md) — deliberately in one +place only. That list drifted apart across three copies while this PR was in +review, which is the argument for not making a fourth. `npm run local:gate` +prints each stage as it starts, so the running command is the other reliable +answer. It is a **strict superset** of GitHub CI (which additionally runs `npm install`, and runs `coverage` as a parallel job). So the direction that matters holds: diff --git a/README.md b/README.md index b7671ed6..4bcb4c10 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ npm run coverage # the per-file ≥90% gate (lines/statements/functions/bran npm run local:gate # MANDATORY before pushing — a strict superset of GitHub CI ``` -`npm run local:gate` is `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. See [Testing and the quality gate](./docs/quality-gate.md) for what each stage covers and why two of them are local-only, and [`AGENTS.md`](./AGENTS.md) for the testing rules themselves. +`npm run local:gate` chains every check below, plus the smokes and the Storybook tests. [Testing and the quality gate](./docs/quality-gate.md) owns the stage list and says what each one covers and why two are local-only; [`AGENTS.md`](./AGENTS.md) holds the testing rules themselves. ## Contributing — `AGENTS.md`, `CLAUDE.md`, and the skills diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index cdd114ba..74b766a9 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -24,7 +24,7 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { checkWiring } from "./verify-skills.mjs"; +import { checkWiring, ciRunsUnconditionally } from "./verify-skills.mjs"; const SCRIPT = path.join( path.dirname(fileURLToPath(import.meta.url)), @@ -236,7 +236,7 @@ test("checkWiring catches the authoritative validator dropped from CI", () => { WIRED_SCRIPTS, "jobs:\n build:\n steps:\n - run: npm run validate\n", ).join(), - /has no step that runs/, + /has no unconditional step that runs/, ); }); @@ -275,7 +275,7 @@ test("checkWiring is not satisfied by a mention outside an executable step", () for (const workflow of mentions) { assert.match( checkWiring(WIRED_SCRIPTS, workflow).join(), - /has no step that runs/, + /has no unconditional step that runs/, `should not count: ${workflow.split("\n")[0]}`, ); } @@ -286,3 +286,59 @@ test("checkWiring accepts the command inside a multi-line run step", () => { "jobs:\n build:\n steps:\n - run: |\n npm run validate\n npm run verify:skills:cli\n"; assert.deepEqual(checkWiring(WIRED_SCRIPTS, workflow), []); }); + +test("ciRunsUnconditionally requires a step that runs on every push", () => { + const C = "npm run verify:skills:cli"; + const wired = + "jobs:\n build:\n steps:\n - run: npm run verify:skills:cli\n"; + assert.equal(ciRunsUnconditionally(wired, C), true); + + // A multi-line `run:` block is the obvious way to over-correct; it counts. + assert.equal( + ciRunsUnconditionally( + "jobs:\n build:\n steps:\n - run: |\n npm run validate\n npm run verify:skills:cli\n", + C, + ), + true, + ); +}); + +test("ciRunsUnconditionally rejects a step that only runs sometimes", () => { + const C = "npm run verify:skills:cli"; + // Both are syntactically `run:` steps, so an executable-position check alone + // reports them as wired while PR CI validates nothing. + const releaseOnly = + "jobs:\n publish:\n if: github.event_name == 'release'\n steps:\n - run: npm run verify:skills:cli\n"; + const stepGated = + "jobs:\n build:\n steps:\n - if: false\n run: npm run verify:skills:cli\n"; + assert.equal(ciRunsUnconditionally(releaseOnly, C), false); + assert.equal(ciRunsUnconditionally(stepGated, C), false); +}); + +test("ciRunsUnconditionally is not satisfied by an unrelated or absent command", () => { + const C = "npm run verify:skills:cli"; + // `verify:skills` must not satisfy a search for `verify:skills:cli`. + assert.equal( + ciRunsUnconditionally( + "jobs:\n build:\n steps:\n - run: npm run verify:skills\n", + C, + ), + false, + ); + assert.equal(ciRunsUnconditionally("not: yaml: at: all:", C), false); + assert.equal(ciRunsUnconditionally("on: push\n", C), false); +}); + +test("the repository's workflow runs the validator unconditionally", () => { + const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", + ); + assert.equal( + ciRunsUnconditionally( + readFileSync(path.join(repoRoot, ".github/workflows/main.yml"), "utf8"), + "npm run verify:skills:cli", + ), + true, + ); +}); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 126e3519..636f6c95 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -32,7 +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 { parse as parseYaml } from "yaml"; import { compareVersions, parseClaudeVersion, @@ -60,6 +60,46 @@ export function skillDirs( .sort(); } +/** + * Whether the workflow has a step that runs `command` on every push. + * + * Deliberately conservative: a job or step carrying **any** `if:` is skipped. + * This guard cannot evaluate an Actions expression, and the property being + * asserted is "CI validates the skills on a PR" — which a conditional step does + * not establish. Over-strictness costs a false failure that is obvious to fix; + * under-strictness costs a guard that reports green while nothing runs. + * + * @param {string} workflowText + * @param {string} command + * @returns {boolean} + */ +export function ciRunsUnconditionally(workflowText, command) { + let doc; + try { + doc = parseYaml(workflowText); + } catch { + return false; + } + const jobs = doc?.jobs; + if (jobs === null || typeof jobs !== "object") return false; + + for (const job of Object.values(jobs)) { + if (job === null || typeof job !== "object") continue; + if ("if" in job) 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; + // A plain substring is enough and cannot over-match: the command is a + // full `npm run