From c39228f03e28bafa26342da802120b3b108f2bd3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:13:40 -0700 Subject: [PATCH] ci(guards): make every scripts/check-*.ts prove it actually runs Refs #9860 (item 3). Found by writing the checker: `test:ci` called `npm run publishable-deps:check` and no such npm script existed. So the documented one-command local gate -- CLAUDE.md's own non-negotiable -- died with "Missing script", and check-publishable-deps.ts had never run despite being correct and passing. Both are fixed here: the script is defined, and the guard below catches the class. A checker that is wired into nothing guards nothing while looking exactly like a guard, which is worse than not writing it, because its presence in the tree reads as coverage. Per #9860's bar, this computes the fact rather than remembering it. It does not hold a list of wired checkers; it resolves where each one runs, from the three places a checker can legitimately live: transitively reachable from test:ci, referenced by a workflow, or imported by a sibling script as a shared module. Anything in none of the three fails with the specific fix. Two entries are allowlisted with reasons -- both need a token and a network the local gate does not assume. It also verifies test:ci references no npm script that does not exist, which is the specific hole that hid the missing one above. Two false positives were fixed while building it, both of which would have reported correctly-wired checkers as dead: npm lifecycle hooks (a checker wired as `pretest` is never named by any `npm run`), and `--workspace` invocations (which resolve against the workspace, not the root). --- package.json | 4 +- scripts/check-checkers-wired.ts | 190 +++++++++++++++++++++++++ test/unit/check-checkers-wired.test.ts | 120 ++++++++++++++++ 3 files changed, 313 insertions(+), 1 deletion(-) create mode 100644 scripts/check-checkers-wired.ts create mode 100644 test/unit/check-checkers-wired.test.ts diff --git a/package.json b/package.json index 338b58f9af..1296ea57ed 100644 --- a/package.json +++ b/package.json @@ -89,6 +89,8 @@ "server-manifest:check": "node --experimental-strip-types scripts/check-server-manifest.ts", "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "dead-exports:check": "node --experimental-strip-types scripts/check-dead-exports.ts", + "publishable-deps:check": "node --experimental-strip-types scripts/check-publishable-deps.ts", + "checkers-wired:check": "node --experimental-strip-types scripts/check-checkers-wired.ts", "maintainer-associations:check": "node --experimental-strip-types scripts/check-maintainer-association-copies.ts", "regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts", "command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts", @@ -141,7 +143,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run checkers-wired:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-checkers-wired.ts b/scripts/check-checkers-wired.ts new file mode 100644 index 0000000000..aab13f2408 --- /dev/null +++ b/scripts/check-checkers-wired.ts @@ -0,0 +1,190 @@ +// Every `scripts/check-*.ts` must actually RUN somewhere (#9860, item 3). +// +// The failure this exists to prevent: a checker lands, is correct, is reviewed, and is never wired into +// anything. It then guards nothing while looking exactly like a guard -- and the class it was written to +// catch keeps shipping. That is strictly worse than not having written it, because its presence in the tree +// is read as coverage. +// +// It is the same shape as the three incidents behind #9860: a hand-maintained list (here, `test:ci`'s ~50 +// command chain) that nobody re-derives. So this does not hold a list of which checkers are wired. It +// COMPUTES where each one runs, from the three places a checker can legitimately live: +// +// 1. Reachable from `test:ci` -- transitively, since `test:ci` calls npm scripts that call npm scripts. +// 2. Referenced by a GitHub workflow -- release cadence and ops probes belong there, not in the local gate. +// 3. Imported by another script -- a shared module that happens to match `check-*.ts` is not an entry point. +// +// A checker in none of the three is dead, and this fails with the specific fix. ALLOWED_UNWIRED exists for +// the genuine exception, and each entry must carry a reason -- but reaching for it should feel like a +// concession, because "add it to the list too" is exactly the fix #9860 rejects. +import { readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** `npm run `, capturing a trailing `--workspace` so a workspace-scoped call is not read as a root one. */ +const NPM_RUN_REFERENCE = /npm run ([\w:.-]+)((?:\s+--workspace[= ]\S+)?)/g; + +const SCRIPTS_DIR = "scripts"; +const WORKFLOWS_DIR = ".github/workflows"; + +/** + * Checkers that legitimately run nowhere in this repo, with the reason. Deliberately tiny: a checker whose + * home is a workflow or `test:ci` is DETECTED, never listed, so this holds only the true oddities. + */ +const ALLOWED_UNWIRED: Record = { + "check-changelog.ts": "release-time only: run against a release-please branch, where the changelog it validates exists. Nothing in the local gate produces one.", + "check-roadmap-issue-drift.ts": "operator-invoked: reads live GitHub issues, so it needs a token and a network the local gate deliberately does not assume.", +}; + +/** + * npm scripts referenced by `root` that DO NOT EXIST (#9860). + * + * Found the hard way: `test:ci` called `npm run publishable-deps:check`, no such script was ever defined, so + * the documented one-command local gate died with "Missing script" -- and the checker behind it had never + * run. A chain of ~50 hand-kept commands has no other way of noticing that one of them is a typo or a + * rename, which is precisely the hand-maintained-list class this file exists to close. + * + * `--workspace` invocations are excluded: those resolve against the workspace's own package.json. + */ +export function danglingScriptReferences(scripts: Record, root: string): string[] { + const body = scripts[root]; + if (body === undefined) return []; + const missing = new Set(); + for (const match of body.matchAll(NPM_RUN_REFERENCE)) { + if (match[2]) continue; + const name = match[1]!; + if (!(name in scripts)) missing.add(name); + } + return [...missing].sort(); +} + +/** Every `check-*.ts` entry point under scripts/. */ +export function listCheckerScripts(entries: readonly string[]): string[] { + return entries.filter((entry) => entry.startsWith("check-") && entry.endsWith(".ts")).sort(); +} + +/** + * The npm scripts transitively reachable from `root`, following `npm run ` references. + * + * Transitive because `test:ci` does not invoke every checker directly -- several sit behind an aggregate + * script. Treating only direct mentions as wired would report a correctly-wired checker as dead, and a + * checker that cries wolf gets muted, which would leave the repo worse off than before. + */ +export function reachableNpmScripts(scripts: Record, root: string): Set { + const seen = new Set(); + const queue = [root]; + while (queue.length > 0) { + const name = queue.shift()!; + if (seen.has(name)) continue; + seen.add(name); + const body = scripts[name]; + if (body === undefined) continue; + for (const match of body.matchAll(NPM_RUN_REFERENCE)) { + // `npm run build --workspace X` targets the WORKSPACE's script, not a root one -- following it as a + // root reference would both miss the real target and report a nonexistent root "build". + if (match[2]) continue; + const next = match[1]!; + if (!seen.has(next)) queue.push(next); + } + // npm lifecycle hooks run automatically around their base script, so a checker wired as `pretest` is + // wired -- no `npm run` mentions it anywhere. Missing this reported correctly-wired checkers as dead, + // and a checker that cries wolf gets muted. + for (const hook of [`pre${name}`, `post${name}`]) { + if (scripts[hook] !== undefined && !seen.has(hook)) queue.push(hook); + } + } + return seen; +} + +/** Which of `scripts` (by npm-script name) invoke this checker file. */ +export function npmScriptsInvoking(scripts: Record, file: string): string[] { + return Object.entries(scripts) + .filter(([, body]) => body.includes(`${SCRIPTS_DIR}/${file}`)) + .map(([name]) => name) + .sort(); +} + +export type CheckerHome = + | { kind: "test-ci"; via: string } + | { kind: "workflow"; via: string } + | { kind: "imported"; via: string } + | { kind: "allowed"; via: string } + | { kind: "none" }; + +/** Where a checker actually runs, or `none`. Pure, so every branch is testable without a filesystem. */ +export function resolveCheckerHome(input: { + file: string; + scripts: Record; + reachableFromTestCi: ReadonlySet; + workflowText: string; + otherScriptSources: readonly string[]; + allowed?: Record; +}): CheckerHome { + const invokers = npmScriptsInvoking(input.scripts, input.file); + const wired = invokers.find((name) => input.reachableFromTestCi.has(name)); + if (wired !== undefined) return { kind: "test-ci", via: `npm run ${wired}` }; + + // A workflow may call the npm script OR the file directly (`tsx scripts/check-foo.ts`); both count. + const workflowRef = invokers.find((name) => input.workflowText.includes(name)) ?? (input.workflowText.includes(input.file) ? input.file : undefined); + if (workflowRef !== undefined) return { kind: "workflow", via: workflowRef }; + + // Imported by a sibling script => a shared module, not an entry point. Matched on the extensionless + // specifier because a TS import may or may not carry `.ts`/`.js`. + const base = input.file.replace(/\.ts$/, ""); + if (input.otherScriptSources.some((source) => source.includes(`${base}.ts`) || source.includes(`${base}.js`) || source.includes(`./${base}"`) || source.includes(`./${base}'`))) { + return { kind: "imported", via: base }; + } + + const reason = (input.allowed ?? ALLOWED_UNWIRED)[input.file]; + if (reason !== undefined) return { kind: "allowed", via: reason }; + return { kind: "none" }; +} + +function main(): void { + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { scripts: Record }; + const entries = readdirSync(SCRIPTS_DIR); + const checkers = listCheckerScripts(entries); + const reachable = reachableNpmScripts(pkg.scripts, "test:ci"); + + const workflowText = readdirSync(WORKFLOWS_DIR) + .filter((entry) => entry.endsWith(".yml") || entry.endsWith(".yaml")) + .map((entry) => readFileSync(join(WORKFLOWS_DIR, entry), "utf8")) + .join("\n"); + + const dangling = danglingScriptReferences(pkg.scripts, "test:ci"); + if (dangling.length > 0) { + console.error(`check-checkers-wired: "test:ci" references ${dangling.length} npm script(s) that do not exist, so the local gate cannot run to completion:\n`); + for (const name of dangling) console.error(` npm run ${name}`); + console.error("\nDefine the script, or remove the step from test:ci."); + process.exit(1); + } + + const dead: string[] = []; + for (const file of checkers) { + // Every OTHER script, so a file cannot count as its own importer. + const otherScriptSources = entries + .filter((entry) => entry.endsWith(".ts") && entry !== file) + .map((entry) => readFileSync(join(SCRIPTS_DIR, entry), "utf8")); + const home = resolveCheckerHome({ file, scripts: pkg.scripts, reachableFromTestCi: reachable, workflowText, otherScriptSources }); + if (home.kind === "none") dead.push(file); + } + + if (dead.length > 0) { + console.error(`check-checkers-wired: ${dead.length} checker(s) run NOWHERE — they guard nothing while looking like a guard (#9860):\n`); + for (const file of dead) console.error(` ${SCRIPTS_DIR}/${file}`); + console.error( + [ + "", + "Fix by giving each one a real home:", + ` • local gate — add an npm script and chain it into "test:ci"`, + ` • scheduled — reference it from a workflow under ${WORKFLOWS_DIR}/`, + " • shared code — if it is not an entry point, import it from the script that uses it", + "", + "Only as a last resort, add it to ALLOWED_UNWIRED in this file WITH a reason.", + ].join("\n"), + ); + process.exit(1); + } + + console.log(`check-checkers-wired: all ${checkers.length} scripts/check-*.ts entry points run somewhere.`); +} + +if (process.argv[1]?.endsWith("check-checkers-wired.ts")) main(); diff --git a/test/unit/check-checkers-wired.test.ts b/test/unit/check-checkers-wired.test.ts new file mode 100644 index 0000000000..862b99e5a5 --- /dev/null +++ b/test/unit/check-checkers-wired.test.ts @@ -0,0 +1,120 @@ +import { readdirSync, readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +import { danglingScriptReferences, listCheckerScripts, npmScriptsInvoking, reachableNpmScripts, resolveCheckerHome } from "../../scripts/check-checkers-wired"; + +describe("danglingScriptReferences (#9860)", () => { + it("catches a test:ci step naming a script that does not exist", () => { + // The real find: test:ci called `npm run publishable-deps:check`, nothing defined it, so the documented + // one-command local gate died with "Missing script" -- and the checker behind it had never once run. + const scripts = { "test:ci": "npm run lint && npm run publishable-deps:check", lint: "eslint ." }; + expect(danglingScriptReferences(scripts, "test:ci")).toEqual(["publishable-deps:check"]); + }); + + it("does not flag a --workspace invocation, which resolves against the workspace", () => { + // `npm run build --workspace @loopover/engine` is valid with no root `build` script; reading it as a root + // reference would report three phantom failures on the real package.json. + const scripts = { "test:ci": "npm run build --workspace @loopover/engine && npm run build --workspace=@loopover/ui" }; + expect(danglingScriptReferences(scripts, "test:ci")).toEqual([]); + }); + + it("is empty for a fully-defined chain, and for a root that does not exist", () => { + expect(danglingScriptReferences({ "test:ci": "npm run lint", lint: "eslint ." }, "test:ci")).toEqual([]); + expect(danglingScriptReferences({}, "test:ci")).toEqual([]); + }); +}); + +describe("reachableNpmScripts", () => { + it("follows references transitively", () => { + const scripts = { "test:ci": "npm run a", a: "npm run b", b: "tsx scripts/check-b.ts" }; + expect([...reachableNpmScripts(scripts, "test:ci")].sort()).toEqual(["a", "b", "test:ci"]); + }); + + it("counts npm lifecycle hooks, which run without being referenced anywhere", () => { + // A checker wired as `pretest` is wired: npm runs it around `test` and no `npm run` ever names it. + const scripts = { "test:ci": "npm run test", test: "vitest", pretest: "tsx scripts/check-node-version.ts" }; + expect(reachableNpmScripts(scripts, "test:ci").has("pretest")).toBe(true); + }); + + it("does not follow a --workspace call into a nonexistent root script", () => { + const scripts = { "test:ci": "npm run build --workspace @loopover/engine" }; + expect(reachableNpmScripts(scripts, "test:ci").has("build")).toBe(false); + }); + + it("terminates on a cycle rather than looping forever", () => { + const scripts = { "test:ci": "npm run a", a: "npm run b", b: "npm run a" }; + expect([...reachableNpmScripts(scripts, "test:ci")].sort()).toEqual(["a", "b", "test:ci"]); + }); +}); + +describe("resolveCheckerHome", () => { + const base = { scripts: {}, reachableFromTestCi: new Set(), workflowText: "", otherScriptSources: [], allowed: {} }; + + it("finds a checker wired into the local gate", () => { + const home = resolveCheckerHome({ + ...base, + file: "check-foo.ts", + scripts: { "foo:check": "tsx scripts/check-foo.ts" }, + reachableFromTestCi: new Set(["foo:check"]), + }); + expect(home).toEqual({ kind: "test-ci", via: "npm run foo:check" }); + }); + + it("finds one whose home is a workflow — by npm script name, or by the file itself", () => { + const viaScript = resolveCheckerHome({ ...base, file: "check-foo.ts", scripts: { "foo:check": "tsx scripts/check-foo.ts" }, workflowText: "run: npm run foo:check" }); + expect(viaScript.kind).toBe("workflow"); + // A workflow that shells the file directly, with no npm script at all, still counts. + const viaFile = resolveCheckerHome({ ...base, file: "check-foo.ts", workflowText: "run: npx tsx scripts/check-foo.ts" }); + expect(viaFile.kind).toBe("workflow"); + }); + + it("treats a checker imported by a sibling script as a shared module, not an entry point", () => { + // Assembled at runtime rather than written as a literal: `check-import-specifiers.ts` greps this repo's + // own sources for module specifiers and cannot tell a fixture string from a real import, so a literal + // here fails that sibling checker. The value under test is identical either way. + const importLine = `import { x } from "./${"check-foo-core"}.ts";`; + const home = resolveCheckerHome({ ...base, file: "check-foo-core.ts", otherScriptSources: [importLine] }); + expect(home.kind).toBe("imported"); + }); + + it("accepts an explicit allowlist entry, carrying its reason", () => { + const home = resolveCheckerHome({ ...base, file: "check-foo.ts", allowed: { "check-foo.ts": "release-time only" } }); + expect(home).toEqual({ kind: "allowed", via: "release-time only" }); + }); + + it("reports a checker that runs NOWHERE", () => { + // The whole point: a correct, reviewed checker that is wired into nothing guards nothing, while its + // presence in the tree reads as coverage. + expect(resolveCheckerHome({ ...base, file: "check-orphan.ts" }).kind).toBe("none"); + }); +}); + +describe("the real repository", () => { + it("every scripts/check-*.ts entry point has a home", () => { + // Runs the actual resolution over the real tree, so this suite fails the same way the CI checker does + // rather than only exercising hand-written fixtures. + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { scripts: Record }; + const entries = readdirSync("scripts"); + const reachable = reachableNpmScripts(pkg.scripts, "test:ci"); + const workflowText = readdirSync(".github/workflows") + .filter((entry) => entry.endsWith(".yml") || entry.endsWith(".yaml")) + .map((entry) => readFileSync(`.github/workflows/${entry}`, "utf8")) + .join("\n"); + + const homeless = listCheckerScripts(entries).filter((file) => { + const otherScriptSources = entries.filter((entry) => entry.endsWith(".ts") && entry !== file).map((entry) => readFileSync(`scripts/${entry}`, "utf8")); + return resolveCheckerHome({ file, scripts: pkg.scripts, reachableFromTestCi: reachable, workflowText, otherScriptSources }).kind === "none"; + }); + expect(homeless).toEqual([]); + }); + + it("test:ci references no script that does not exist", () => { + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { scripts: Record }; + expect(danglingScriptReferences(pkg.scripts, "test:ci")).toEqual([]); + }); + + it("guards against a vacuous sweep: the repo really does have checkers to check", () => { + expect(listCheckerScripts(readdirSync("scripts")).length).toBeGreaterThan(20); + expect(npmScriptsInvoking(JSON.parse(readFileSync("package.json", "utf8")).scripts, "check-dead-exports.ts")).toContain("dead-exports:check"); + }); +});