diff --git a/scripts/lib/claude-cli.mjs b/scripts/lib/claude-cli.mjs index eda39761..d7cbdb62 100644 --- a/scripts/lib/claude-cli.mjs +++ b/scripts/lib/claude-cli.mjs @@ -43,9 +43,15 @@ export function claudeSpawnArgs( * `--version` with something unrecognizable cannot be trusted to be the pinned * one either, and the callers all treat null as "use the pinned package". * - * @param {(text: string) => number[] | null} parseVersion + * Generic in the parser's result: this helper only distinguishes "parsed" from + * "did not", so the shape is whatever the caller's parser returns — today + * `{ parts, prerelease }` from `parseClaudeVersion`. Hard-coding `number[]` + * here made valid callers look type-invalid (Copilot). + * + * @template T + * @param {(text: string) => T | null} parseVersion * @param {{ spawn?: typeof spawnSync, platform?: string }} [io] - * @returns {number[] | null} + * @returns {T | null} */ export function probeClaudeVersion( parseVersion, diff --git a/scripts/verify-skills-cli.mjs b/scripts/verify-skills-cli.mjs index ee01b247..13c91b4c 100644 --- a/scripts/verify-skills-cli.mjs +++ b/scripts/verify-skills-cli.mjs @@ -37,13 +37,14 @@ const SKILLS_DIR = ".claude/skills"; /** * The argv that runs the pinned validator. * - * A local CLI is used only when it matches `PINNED_CLI_VERSION` **exactly**. - * Accepting anything at or above the floor would defeat the pin: a maintainer on - * a newer CLI would validate against a different schema than CI's, so the same - * `local:gate` could disagree across machines — which is the failure a pin - * exists to prevent (Copilot). Everyone else runs the pinned package. + * A local CLI is used only when it matches `PINNED_CLI_VERSION` **exactly** — + * same release triple and no prerelease. Accepting anything at or above a floor + * would defeat the pin: a maintainer on a different CLI would validate against a + * different schema than CI's, so the same `local:gate` could disagree across + * machines, which is the failure a pin exists to prevent (Copilot). Everyone + * else runs the pinned package. * - * @param {{ version: number[] | null }} local + * @param {{ version: { parts: number[], prerelease: string | null } | null }} local * @returns {{ command: string, args: string[], via: string }} */ export function validatorCommand({ version }) { @@ -80,9 +81,10 @@ export function validatorCommand({ version }) { * propagation of a rejected validator would leave `test:scripts` green while * this gate quietly stopped gating (Copilot). * - * @param {{ probe?: () => number[] | null, + * @param {{ probe?: () => { parts: number[], prerelease: string | null } | null, * spawn?: (cmd: string, args: string[], opts: object) => {error?: Error, status?: number|null}, - * log?: (msg: string) => void, error?: (msg: string) => void }} [io] + * log?: (msg: string) => void, error?: (msg: string) => void, + * platform?: string }} [io] * @returns {number} Process exit code. */ export function runValidator(io = {}) { diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index 4d5e0e50..333b8571 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -209,7 +209,7 @@ const WIRED_SCRIPTS = { "verify:skills:cli": "node scripts/verify-skills-cli.mjs", }; const WIRED_WORKFLOW = - "jobs:\n build:\n steps:\n - run: npm run verify:skills:cli\n"; + "on:\n push:\njobs:\n build:\n steps:\n - run: npm run verify:skills:cli\n"; test("checkWiring is silent when both gates are wired", () => { assert.deepEqual(checkWiring(WIRED_SCRIPTS, WIRED_WORKFLOW), []); @@ -238,7 +238,7 @@ test("checkWiring catches the authoritative validator dropped from CI", () => { assert.match( checkWiring( WIRED_SCRIPTS, - "jobs:\n build:\n steps:\n - run: npm run validate\n", + "on:\n push:\njobs:\n build:\n steps:\n - run: npm run validate\n", ).join(), /has no unconditional step that runs/, ); @@ -272,9 +272,9 @@ test("checkWiring is not satisfied by a mention outside an executable step", () // 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", + "# npm run verify:skills:cli\non:\n push:\njobs:\n build:\n steps:\n - run: npm run validate\n", + "on:\n push:\njobs:\n build:\n steps:\n - name: npm run verify:skills:cli\n run: npm run validate\n", + "on:\n push:\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( @@ -287,20 +287,20 @@ test("checkWiring is not satisfied by a mention outside an executable step", () 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"; + "on:\n push:\njobs:\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"; + "on:\n push:\njobs:\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", + "on:\n push:\njobs:\n build:\n steps:\n - run: |\n npm run validate\n npm run verify:skills:cli\n", C, ), true, @@ -312,9 +312,9 @@ test("ciRunsUnconditionally rejects a step that only runs sometimes", () => { // 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"; + "on:\n push:\njobs:\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"; + "on:\n push:\njobs:\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); }); @@ -324,7 +324,7 @@ test("ciRunsUnconditionally is not satisfied by an unrelated or absent command", // `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", + "on:\n push:\njobs:\n build:\n steps:\n - run: npm run verify:skills\n", C, ), false, @@ -378,9 +378,33 @@ test("ciRunsUnconditionally inherits the exact-invocation rule", () => { const C = "npm run verify:skills:cli"; assert.equal( ciRunsUnconditionally( - "jobs:\n build:\n steps:\n - run: npm run verify:skills:cli:disabled\n", + "on:\n push:\njobs:\n build:\n steps:\n - run: npm run verify:skills:cli:disabled\n", C, ), false, ); }); + +test("ciRunsUnconditionally requires the workflow to fire on ordinary changes", () => { + const C = "npm run verify:skills:cli"; + const step = + "jobs:\n build:\n steps:\n - run: npm run verify:skills:cli\n"; + + // `on:` takes three shapes, and any of them may name the event. + assert.equal(ciRunsUnconditionally(`on:\n push:\n${step}`, C), true); + assert.equal(ciRunsUnconditionally(`on: push\n${step}`, C), true); + assert.equal(ciRunsUnconditionally(`on: [push, release]\n${step}`, C), true); + assert.equal(ciRunsUnconditionally(`on:\n pull_request:\n${step}`, C), true); + + // A workflow switched to release-only still CONTAINS the step, and an + // unconditional-step check alone reports it as wired — while no PR ever runs + // the validator (Copilot). + assert.equal( + ciRunsUnconditionally( + `on:\n release:\n types: [published]\n${step}`, + C, + ), + false, + ); + assert.equal(ciRunsUnconditionally(step, C), false); +}); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index b3ffee50..d9fd693b 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -82,9 +82,35 @@ export function runsCommand(script, command) { .some((segment) => segment === command); } +/** + * Whether a workflow's `on:` includes an event that fires for ordinary changes. + * + * `on:` takes three shapes — a bare string, a list, or a map of event names. + * Note the YAML 1.1 `on` → `true` pitfall does not apply here: the `yaml` + * package parses to the 1.2 core schema, where the key stays the string `on`. + * + * @param {unknown} on + */ +function triggersOnPush(on) { + const events = + typeof on === "string" + ? [on] + : Array.isArray(on) + ? on + : on !== null && typeof on === "object" + ? Object.keys(on) + : []; + return events.some((e) => e === "push" || e === "pull_request"); +} + /** * 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: + * 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. + * * 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 @@ -102,6 +128,8 @@ export function ciRunsUnconditionally(workflowText, command) { } catch { return false; } + if (!triggersOnPush(doc?.on)) return false; + const jobs = doc?.jobs; if (jobs === null || typeof jobs !== "object") return false;