chore: address Copilot review round 9
- Enrol both skill gates in the repo's gate-integrity vouch cycle. A gate that stops being invoked fails silently with every fixture test green, which is the failure the existing cycle exists to prevent. `verify:format-coverage` now vouches for `verify:skills`, `verify:skills` vouches back, and it additionally asserts the two links nothing else could see: `verify:skills:cli` present in `local:gate` and in the workflow — neither is reachable from `validate` or from any test. `checkWiring` is pure and unit-tested, with a live case against the real package.json and workflow so the fixtures cannot drift. All four links verified by mutation. - The existing sibling-vouch fixture is now a table over every sibling, so adding a fourth guard later does not need a new test. - release skill: stop hard-coding `2.3.0`, which has already shipped — the tag command aborted on an existing tag and the branch template named the wrong release. Both now read the version from `package.json` (the tag from `origin/main`'s), with a warning never to copy a version out of the file. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
5078a5bc50
commit
17068fc89e
5 changed files with 177 additions and 26 deletions
|
|
@ -33,11 +33,17 @@ The bump is part of the milestone's work, so it belongs on the develop branch
|
|||
and flows into `main` with everything else.
|
||||
|
||||
```sh
|
||||
git checkout -b v2/chore/<ISSUE>-bump-2-3-0 v2/main
|
||||
# The branch name carries the version you are bumping TO, so read it first.
|
||||
node -p "require('./package.json').version" # what is on v2/main now
|
||||
git checkout -b v2/chore/<ISSUE>-bump-<X-Y-Z> v2/main
|
||||
npm version minor --no-git-tag-version # or major / patch; bump only, no tag
|
||||
# PR → v2/main
|
||||
node -p "require('./package.json').version" # confirm, then PR → v2/main
|
||||
```
|
||||
|
||||
⚠️ **Never copy a version out of this file.** It would be a version that has
|
||||
already shipped by the time you read it, and following it would cut a release
|
||||
branch named for the wrong release (Copilot).
|
||||
|
||||
⚠️ **`--no-git-tag-version` is load-bearing.** A bare `npm version` also tags,
|
||||
and the tag would land on a `v2/main` commit — but the release must be cut from
|
||||
`main`, so the tag has to point at the merge commit there (step 3). Tagging here
|
||||
|
|
@ -57,9 +63,14 @@ it behind, something went wrong.
|
|||
|
||||
## 3. Tag `origin/main` and draft the Release
|
||||
|
||||
Derive the tag from the version that just landed, rather than typing one — a
|
||||
hard-coded tag is either already taken (so `git tag` aborts) or, worse, wrong:
|
||||
|
||||
```sh
|
||||
git fetch origin main
|
||||
git tag 2.3.0 origin/main && git push origin 2.3.0
|
||||
VERSION=$(git show origin/main:package.json | node -p "JSON.parse(require('fs').readFileSync(0)).version")
|
||||
echo "$VERSION" # sanity-check before tagging
|
||||
git tag "$VERSION" origin/main && git push origin "$VERSION"
|
||||
# then draft & publish a GitHub Release for that tag → triggers `publish`
|
||||
```
|
||||
|
||||
|
|
@ -69,8 +80,8 @@ divergent local `main` can quietly produce or replay local commits. Tagging
|
|||
`HEAD` there tags a commit that is not on `origin/main`, and `git push origin
|
||||
<tag>` pushes only the tag — leaving a release whose commit was never published.
|
||||
|
||||
⚠️ **No `v` prefix.** This repo's release tags are bare `x.y.z` — `2.2.0`,
|
||||
`2.1.0`, `2.0.0`. npm's own `tag-version-prefix` defaults to `v` and the repo
|
||||
⚠️ **No `v` prefix.** This repo's release tags are bare `x.y.z` — which is why
|
||||
the command above tags `$VERSION` and not `v$VERSION`. npm's own `tag-version-prefix` defaults to `v` and the repo
|
||||
sets no `.npmrc`, so a bare `npm version` would have produced a mismatched tag;
|
||||
tagging by hand is what keeps it right. (The workflow's assert step strips a
|
||||
leading `v` before comparing, so a `v`-prefixed tag would still publish — it
|
||||
|
|
|
|||
|
|
@ -74,24 +74,26 @@ function runWithGuards(guards) {
|
|||
}
|
||||
}
|
||||
|
||||
const BOTH_SIBLINGS = ["verify:typecheck-coverage", "verify:dep-lockstep"];
|
||||
const ALL_SIBLINGS = [
|
||||
"verify:typecheck-coverage",
|
||||
"verify:dep-lockstep",
|
||||
"verify:skills",
|
||||
];
|
||||
|
||||
test("vouch: fails when `verify:dep-lockstep` is dropped from validate", () => {
|
||||
const { status, out } = runWithGuards(["verify:typecheck-coverage"]);
|
||||
assert.equal(status, 1, out);
|
||||
assert.match(out, /no longer runs `verify:dep-lockstep`/);
|
||||
});
|
||||
for (const dropped of ALL_SIBLINGS) {
|
||||
test(`vouch: fails when \`${dropped}\` is dropped from validate`, () => {
|
||||
const { status, out } = runWithGuards(
|
||||
ALL_SIBLINGS.filter((g) => g !== dropped),
|
||||
);
|
||||
assert.equal(status, 1, out);
|
||||
assert.match(out, new RegExp("no longer runs `" + dropped + "`"));
|
||||
});
|
||||
}
|
||||
|
||||
test("vouch: fails when `verify:typecheck-coverage` is dropped from validate", () => {
|
||||
const { status, out } = runWithGuards(["verify:dep-lockstep"]);
|
||||
assert.equal(status, 1, out);
|
||||
assert.match(out, /no longer runs `verify:typecheck-coverage`/);
|
||||
});
|
||||
|
||||
test("vouch: passes when both siblings are still wired", () => {
|
||||
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
|
||||
// sibling may be reported missing.
|
||||
const { out } = runWithGuards(BOTH_SIBLINGS);
|
||||
const { out } = runWithGuards(ALL_SIBLINGS);
|
||||
assert.doesNotMatch(out, /no longer runs/);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -166,15 +166,19 @@ function trackedSourceFiles() {
|
|||
return out.split("\n").filter(Boolean);
|
||||
}
|
||||
|
||||
// Vouch for the sibling guards: a guard can't detect being unrun itself, so the
|
||||
// three form a cycle instead. This one checks BOTH others; each of them checks
|
||||
// only this one. So dropping `verify:typecheck-coverage` or
|
||||
// `verify:dep-lockstep` is caught here, and dropping *this* guard is caught by
|
||||
// either of them. Only removing all three at once slips through.
|
||||
// Vouch for the sibling guards: a guard can't detect being unrun itself, so they
|
||||
// form a cycle instead. This one checks the others; each of them checks only
|
||||
// this one. So dropping `verify:typecheck-coverage`, `verify:dep-lockstep` or
|
||||
// `verify:skills` is caught here, and dropping *this* guard is caught by any of
|
||||
// them. Only removing all of them at once slips through.
|
||||
const rootScripts = JSON.parse(
|
||||
readFileSync(path.join(repoRoot, "package.json"), "utf8"),
|
||||
).scripts;
|
||||
for (const sibling of ["verify:typecheck-coverage", "verify:dep-lockstep"]) {
|
||||
for (const sibling of [
|
||||
"verify:typecheck-coverage",
|
||||
"verify:dep-lockstep",
|
||||
"verify:skills",
|
||||
]) {
|
||||
if (rootReachesScript(rootScripts, sibling)) continue;
|
||||
console.error(
|
||||
`verify:format-coverage — the root \`validate\` no longer runs \`${sibling}\` (its sibling guard). Restore it.`,
|
||||
|
|
|
|||
|
|
@ -12,11 +12,20 @@
|
|||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||
import {
|
||||
cpSync,
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
readFileSync,
|
||||
rmSync,
|
||||
writeFileSync,
|
||||
} from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import { checkWiring } from "./verify-skills.mjs";
|
||||
|
||||
const SCRIPT = path.join(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"verify-skills.mjs",
|
||||
|
|
@ -175,3 +184,78 @@ test("reports every offender in one pass rather than dying on the first", () =>
|
|||
assert.match(out, /3 problem\(s\)/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// --- wiring vouch -----------------------------------------------------------
|
||||
//
|
||||
// The gates cannot detect being unrun, so they vouch for each other — the same
|
||||
// cycle the sibling guards use. `checkWiring` is the pure half of that, driven
|
||||
// here with fixture inputs; the other half (`verify:format-coverage` noticing
|
||||
// `verify:skills` gone from `validate`) lives in that guard.
|
||||
//
|
||||
// Three distinct links, each of which would otherwise disappear in silence:
|
||||
// `verify:skills` reachable from `validate`, `verify:skills:cli` in
|
||||
// `local:gate`, and `verify:skills:cli` in the workflow.
|
||||
|
||||
const WIRED_SCRIPTS = {
|
||||
validate: "npm run verify:format-coverage && npm run verify:skills",
|
||||
"verify:format-coverage": "node scripts/verify-format-coverage.mjs",
|
||||
"verify:skills": "node scripts/verify-skills.mjs",
|
||||
"local:gate":
|
||||
"npm run validate && npm run verify:skills:cli && npm run coverage",
|
||||
"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";
|
||||
|
||||
test("checkWiring is silent when both gates are wired", () => {
|
||||
assert.deepEqual(checkWiring(WIRED_SCRIPTS, WIRED_WORKFLOW), []);
|
||||
});
|
||||
|
||||
test("checkWiring catches a sibling guard dropped from validate", () => {
|
||||
const scripts = { ...WIRED_SCRIPTS, validate: "npm run verify:skills" };
|
||||
assert.match(
|
||||
checkWiring(scripts, WIRED_WORKFLOW).join(),
|
||||
/no longer runs `verify:format-coverage`/,
|
||||
);
|
||||
});
|
||||
|
||||
test("checkWiring catches the authoritative validator dropped from local:gate", () => {
|
||||
const scripts = {
|
||||
...WIRED_SCRIPTS,
|
||||
"local:gate": "npm run validate && npm run coverage",
|
||||
};
|
||||
assert.match(
|
||||
checkWiring(scripts, WIRED_WORKFLOW).join(),
|
||||
/local:gate` no longer runs `verify:skills:cli`/,
|
||||
);
|
||||
});
|
||||
|
||||
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",
|
||||
).join(),
|
||||
/workflows\/main\.yml` no longer runs/,
|
||||
);
|
||||
});
|
||||
|
||||
test("checkWiring reports every broken link at once", () => {
|
||||
assert.equal(checkWiring({ validate: "", "local:gate": "" }, "").length, 3);
|
||||
});
|
||||
|
||||
test("the repository as it stands is wired", () => {
|
||||
// The live assertion: fixtures can drift from what the repo actually does.
|
||||
const repoRoot = path.resolve(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"..",
|
||||
);
|
||||
const scripts = JSON.parse(
|
||||
readFileSync(path.join(repoRoot, "package.json"), "utf8"),
|
||||
).scripts;
|
||||
const workflow = readFileSync(
|
||||
path.join(repoRoot, ".github/workflows/main.yml"),
|
||||
"utf8",
|
||||
);
|
||||
assert.deepEqual(checkWiring(scripts, workflow), []);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -31,6 +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 { reachableScripts, rootReachesScript } from "./lib/npm-scripts.mjs";
|
||||
import {
|
||||
compareVersions,
|
||||
parseClaudeVersion,
|
||||
|
|
@ -58,6 +59,41 @@ export function skillDirs(
|
|||
.sort();
|
||||
}
|
||||
|
||||
/**
|
||||
* Assert the two skill gates are still WIRED, not merely present.
|
||||
*
|
||||
* A gate that stops being invoked fails silently and every fixture test stays
|
||||
* green — which is the same failure the sibling guards' vouch cycle exists to
|
||||
* prevent, so this joins it (Copilot). Two distinct links:
|
||||
*
|
||||
* - `verify:skills` (this file) must be reachable from the root `validate`.
|
||||
* It cannot detect its own absence, so `verify:format-coverage` vouches for
|
||||
* it and this vouches back — the established cycle.
|
||||
* - `verify:skills:cli` must be in `local:gate` AND in the workflow. That is
|
||||
* 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.
|
||||
*/
|
||||
export function checkWiring(rootScripts, workflowText) {
|
||||
const problems = [];
|
||||
if (!rootReachesScript(rootScripts, "verify:format-coverage")) {
|
||||
problems.push(
|
||||
"the root `validate` no longer runs `verify:format-coverage` (a sibling guard). Restore it.",
|
||||
);
|
||||
}
|
||||
if (!reachableScripts(rootScripts, "local:gate").has("verify:skills:cli")) {
|
||||
problems.push(
|
||||
"`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)) {
|
||||
problems.push(
|
||||
"`.github/workflows/main.yml` no longer runs `npm run verify:skills:cli`, so the authoritative validator never runs in CI. Restore it.",
|
||||
);
|
||||
}
|
||||
return problems;
|
||||
}
|
||||
|
||||
function main(argv = process.argv.slice(2)) {
|
||||
// An explicit directory is the seam the fixture tests drive; without it this
|
||||
// guard could only ever be exercised against the repo's own (green) skills,
|
||||
|
|
@ -66,6 +102,20 @@ function main(argv = process.argv.slice(2)) {
|
|||
const SKILLS_DIR = override ? path.resolve(override) : DEFAULT_SKILLS_DIR;
|
||||
const failures = [];
|
||||
|
||||
// Only meaningful against the real repo; a fixture run has no wiring.
|
||||
if (!override) {
|
||||
const rootScripts = JSON.parse(
|
||||
readFileSync(path.join(ROOT, "package.json"), "utf8"),
|
||||
).scripts;
|
||||
const workflow = readFileSync(
|
||||
path.join(ROOT, ".github", "workflows", "main.yml"),
|
||||
"utf8",
|
||||
);
|
||||
for (const problem of checkWiring(rootScripts, workflow)) {
|
||||
failures.push(problem);
|
||||
}
|
||||
}
|
||||
|
||||
if (!existsSync(SKILLS_DIR)) {
|
||||
console.error(
|
||||
`verify:skills — ${path.relative(ROOT, SKILLS_DIR)} does not exist.`,
|
||||
|
|
|
|||
Loading…
Reference in a new issue