chore: address Copilot review round 17
- ciRunsUnconditionally never looked at the workflow-level `on:` trigger, so a
workflow switched to `on: release` still contained the step and reported as
wired while no PR ever ran the validator. It now requires `push` or
`pull_request`, handling all three shapes `on:` takes. The positive fixtures
had no trigger at all, so they could not assert the "on every push" half of
the contract; they carry one now, alongside a release-only negative. Verified
by mutation.
- Stale JSDoc from the round-16 version change: `probeClaudeVersion` is now
generic in its parser's result rather than hard-coding `number[]`,
`validatorCommand` documents `{ parts, prerelease }`, and `runValidator`
documents the `platform` option it already accepted. Annotations that lie make
valid call sites look type-invalid.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
c26995bf82
commit
1203246537
4 changed files with 82 additions and 22 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 = {}) {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue