diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8eaf61799..a08cc3687 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,7 @@ jobs: mcp: ${{ steps.filter.outputs.mcp }} mcpCliHarness: ${{ steps.filter.outputs.mcpCliHarness }} engine: ${{ steps.filter.outputs.engine }} + contract: ${{ steps.filter.outputs.contract }} discoveryIndex: ${{ steps.filter.outputs.discoveryIndex }} miner: ${{ steps.filter.outputs.miner }} minerTestHarness: ${{ steps.filter.outputs.minerTestHarness }} @@ -188,6 +189,10 @@ jobs: engine: - 'packages/loopover-engine/**' - 'package-lock.json' + contract: + - 'packages/loopover-contract/**' + - 'scripts/gen-contract-api-schemas.ts' + - 'package-lock.json' miner: - 'packages/loopover-miner/**' - 'scripts/check-miner-package.ts' @@ -478,6 +483,56 @@ jobs: env: NODE_OPTIONS: "" run: node --experimental-strip-types scripts/validate-observability-configs.ts + # ── DRIFT CHECKS THAT CI PREVIOUSLY DID NOT RUN (#10269) ──────────────────────────────────────── + # `npm run test:ci` (the local gate) and this file are two independently hand-maintained lists, and + # they had drifted: 22 of the 42 checks reachable from `test:ci` ran in NO workflow at all. They were + # therefore enforced only when a human happened to run the full local aggregate -- which on a repo + # whose gate auto-merges on green CI makes them a convention, not a gate. #10237 is the same class + # having already fired twice on one artifact, found both times by a human and never by CI. + # + # Deliberately ONE ungated block rather than 19 individually-gated steps. Per-check path gating is + # where the fidelity bugs actually live -- #10268 had to add a whole `contract` filter because + # nothing here watched packages/loopover-contract/** -- and 19 hand-chosen gates would be exactly the + # hand-maintained list #9860 exists to reject. These are all sub-second file-reading scripts, so + # running them unconditionally costs far less than choosing 19 gates correctly. + # + # Ordering: this sits after "Build contract package" because the two control-plane generators import + # @loopover/contract/control-plane. Everything else here reads source, package.json or workflow files. + # + # NOT here, on purpose: mcp:tool-reference:check resolves @loopover/engine through node_modules to + # its dist/, so it cannot run before "Build engine package" -- it has its own step below, carrying + # that build's condition rather than a gate chosen from its own inputs. It is the ONLY check in this + # set with a build prerequisite; verified by deleting packages/loopover-engine/dist and re-running + # all of them, which is also how CI caught it here rather than anyone predicting it. + # + # NOT here either: db:migrations:immutable:check, release-commit-parsing:check and + # releasable-commit-types:check all need git history this job does not have (its checkout is shallow + # by design). They run in `drift-checks-history` below -- see that job for why putting them here + # would have been worse than leaving them unwired. + - name: Drift checks (unconditional) + run: | + set -euo pipefail + npm run turbo-inputs:check + npm run workspace-dep-ranges:check + npm run control-plane:contract:check + npm run control-plane:openapi:check + npm run coverage-boltons:check + npm run ui-derived-types:check + npm run server-manifest:check + npm run dead-exports:check + npm run publishable-deps:check + npm run fixture-clock-races:check + npm run typecheck-coverage:check + npm run test-wiring:check + npm run checkers-wired:check + npm run focus-manifest-fields:check + npm run maintainer-associations:check + npm run release-linked-versions:check + npm run mcp:client-config:check + npm run record-mapper-fields:check + npm run verdict-backoff-chokepoint:check + npm run dispatch-provenance:check + npm run ci-drift-checks-wired:check # Runs ahead of Typecheck AND "Test with coverage" (#ci-engine-build-order): src/mcp/find-opportunities.ts # (root backend, since #2281/#3985) imports packages/loopover-miner/lib/opportunity-fanout.js -- # a .js-suffixed specifier esbuild/Vite resolve straight to the real (gitignored, uncompiled) @@ -545,6 +600,16 @@ jobs: - name: Build engine package if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' || needs.changes.outputs.engine == 'true' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.miner == 'true' }} run: npx turbo run build --filter=@loopover/engine + # The one drift check with a build prerequisite (#10269): it resolves a bare `@loopover/engine` + # specifier through node_modules to dist/, so it must follow the build above and carries that build's + # condition VERBATIM. The gate is therefore derived ("run iff the thing I need was built"), not + # hand-picked from this check's own inputs -- which is the distinction that keeps it out of the + # hand-maintained-gate class the unconditional block above exists to avoid. If the two conditions ever + # diverge this check silently stops running, so they must move together, exactly as the Typecheck step + # and this build already must. + - name: MCP tool-reference drift check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' || needs.changes.outputs.engine == 'true' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.miner == 'true' }} + run: npm run mcp:tool-reference:check # Mirrors "MCP package check"/"Miner package check" below: the published npm tarball is a # different surface than the workspace build above (files field, forbidden paths/content, # stale README wording) and needs its own dry-run validation (#8591). @@ -822,6 +887,19 @@ jobs: - name: OpenAPI drift check if: ${{ github.event_name == 'push' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.uiContract == 'true' }} run: npm run ui:openapi:check + # Same generated-artifact-drift class as the OpenAPI check above, and it was enforced ONLY by the + # local `npm run test:ci` aggregate -- exactly the local-only gap called out on the selfhost/miner + # env-reference steps in validate-code, which each went stale with zero CI signal until someone + # happened to run the full local script. It has now bitten this file twice (#10237): once when the + # discovery-route schemas were added without teaching the generator, leaving `main` red for every PR + # that ran the local gate, and again when #10160 added `linkedIssueMaintainerExempt` without + # regenerating. Neither surfaced in CI. + # Its inputs are src/openapi/schemas.ts (`backend`), apps/loopover-ui/public/openapi.json (`ui`), and + # the contract package's own modules, which it scans to emit imports -- hence the third filter, added + # with this step because nothing here previously watched packages/loopover-contract/** at all. + - name: Contract api-schemas drift check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.contract == 'true' }} + run: npm run contract:api-schemas:check # Checks apps/loopover-ui/src' known-latest MCP version string against the published package, so # its dependency is `ui` (the file it scans) + `mcp` (the package it checks against) -- NOT the # OpenAPI contract, which this script never reads. @@ -956,6 +1034,58 @@ jobs: # never serializes with the fast drift/typecheck/build checks -- that split remains). It ran 2023-style # as a 3-shard matrix for wall-clock; unsharded again 2026-07-24 (see the job's own header comment) to # trade PR latency for 2 fewer runners per PR and the simpler, flake-free single-report pipeline. + # The three drift checks that need real git history (#10269). They are a SEPARATE job purely because + # validate-code's checkout is shallow on purpose ("this job no longer uploads to Codecov ... so it has no + # reason to fetch full history anymore"), and each of these three degrades differently and silently there: + # + # - db:migrations:immutable:check exits 1 with "no orb-v* tags visible", so it would fail EVERY PR. + # - release-commit-parsing:check returns [] and PASSES when it cannot resolve the base. + # - releasable-commit-types:check prints "cannot resolve ...; skipping" and exits 0. + # + # The last two are why this is a job and not a step in validate-code: wired there they would be + # permanently green while verifying nothing, which is the "guards nothing while looking like a guard" + # failure (#9860) that #10269 exists to fix. Reproducing that at the CI layer while claiming to fix it + # would be worse than leaving them unwired, because the green would then be read as coverage. + drift-checks-history: + name: drift-checks-history + needs: changes + if: ${{ github.event_name == 'push' || github.event.pull_request.draft != true }} + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + # fetch-depth: 0 + tags is the entire point of this job -- the immutability check compares migration + # blobs against every orb-v* tag, and both commit checks diff against origin/. + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} + fetch-depth: 0 + fetch-tags: true + - name: Setup workspace + uses: ./.github/actions/setup-workspace + # A PR checkout leaves no ref for the base branch, and both commit checks diff against + # `origin/`. Fetch it explicitly rather than assuming fetch-depth: 0 created the remote ref. + - name: Fetch base ref + if: ${{ github.event_name == 'pull_request' }} + run: git fetch --no-tags --quiet origin "+refs/heads/${{ github.base_ref }}:refs/remotes/origin/${{ github.base_ref }}" + # Fails loudly if the history this job exists to provide is somehow absent, rather than letting the + # two skip-on-error checks below report a green that means nothing. + - name: Assert history is present + run: | + set -euo pipefail + tags="$(git tag -l 'orb-v*' | wc -l | tr -d ' ')" + if [ "$tags" -eq 0 ]; then + echo "::error::No orb-v* tags after fetch-tags -- the history checks below would be vacuous." + exit 1 + fi + echo "orb-v* tags visible: $tags" + - name: Released-migration immutability check + run: npm run db:migrations:immutable:check + - name: Release commit-parsing check + run: npm run release-commit-parsing:check + - name: Releasable commit-types check + run: npm run releasable-commit-types:check + validate-tests: name: validate-tests needs: changes @@ -1283,7 +1413,7 @@ jobs: # Path-filtered jobs report "skipped", which is treated as success. validate: name: validate - needs: [changes, validate-code, validate-tests] + needs: [changes, validate-code, validate-tests, drift-checks-history] if: ${{ always() }} # Pure result-aggregation (reads needs.*.result, echoes pass/fail) -- no build/test work, so it never # needed the self-hosted pool's cached toolchain (#2507). diff --git a/package.json b/package.json index 7b7959a27..4d8456b44 100644 --- a/package.json +++ b/package.json @@ -97,6 +97,7 @@ "release-commit-parsing:check": "tsx scripts/check-release-commit-parsing.ts", "releasable-commit-types:check": "tsx scripts/check-releasable-commit-types.ts", "checkers-wired:check": "node --experimental-strip-types scripts/check-checkers-wired.ts", + "ci-drift-checks-wired:check": "node --experimental-strip-types scripts/check-ci-drift-checks-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", @@ -152,7 +153,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 fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types: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 verdict-backoff-chokepoint:check && npm run record-mapper-fields:check && npm run dispatch-provenance: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 fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run ci-drift-checks-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types: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 verdict-backoff-chokepoint:check && npm run record-mapper-fields:check && npm run dispatch-provenance: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-ci-drift-checks-wired.ts b/scripts/check-ci-drift-checks-wired.ts new file mode 100644 index 000000000..f9781413a --- /dev/null +++ b/scripts/check-ci-drift-checks-wired.ts @@ -0,0 +1,118 @@ +// A check in `test:ci` is not a gate unless a workflow runs it too (#10269). +// +// `npm run test:ci` is the documented local gate; `.github/workflows/**` is what actually blocks a merge. +// They are two independently hand-maintained lists, and they drifted: 22 of the 42 checks reachable from +// `test:ci` ran in NO workflow at all. On a repo whose gate auto-merges on green CI, such a check is a +// convention, not a gate -- and it reads as coverage, which is worse than not having it. +// +// This is check-checkers-wired.ts's sibling, on the other axis. That one asks "does this scripts/check-*.ts +// run ANYWHERE (test:ci included)?" and so is satisfied by the local gate alone. This one asks the question +// that matters for merge safety: "does CI actually run it?" +// +// It is deliberately COMPUTED, never a list of what is wired -- the same reasoning as #9860. A hand-kept +// roster of "checks CI runs" would drift from ci.yml exactly the way ci.yml drifted from test:ci. +// +// ALLOWED_LOCAL_ONLY is the escape hatch and each entry must carry a reason. Reaching for it should feel +// like a concession: "add it to the list too" is the fix this file exists to reject. + +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +const WORKFLOWS_DIR = ".github/workflows"; +const ROOT_SCRIPT = "test:ci"; + +/** `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; + +/** + * Which npm-script names count as a "check" for this rule. + * + * Naming-based on purpose: it is the convention this repo already follows for every drift/wiring guard, and + * it means a newly added check is covered the moment it is named like its siblings, with nothing to + * register. Build, test and coverage steps are out of scope -- CI runs those through its own jobs. + */ +export function isDriftCheckScript(name: string): boolean { + // `-audit` as well as `:audit`: this repo names both ways (`ui:version-audit`, + // `test:miner-deployment-docs-audit`), and a `:audit$`-only pattern silently matched NEITHER -- a + // scoping bug that would have quietly excluded them from the rule while looking like it covered them. + return /:check$|drift-check$|[:-]audit$/.test(name); +} + +/** Checks that legitimately run only in the local gate, with the reason. Deliberately empty. */ +const ALLOWED_LOCAL_ONLY: Record = {}; + +/** The npm scripts transitively reachable from `root`, following `npm run ` references. */ +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. + if (match[2]) continue; + if (!seen.has(match[1]!)) queue.push(match[1]!); + } + } + return seen; +} + +/** Every drift check the local gate runs. */ +export function driftChecksInLocalGate(scripts: Record, root = ROOT_SCRIPT): string[] { + return [...reachableNpmScripts(scripts, root)].filter((name) => name !== root && isDriftCheckScript(name)).sort(); +} + +/** + * PURE. Checks the local gate runs that no workflow does. + * + * Matched on `npm run ` rather than on the script file, because that is how a workflow invokes one and + * it is the same string the local gate uses -- so a rename breaks both sides together instead of silently + * satisfying this while running nothing. The word-boundary guard stops `foo:check` being considered wired by + * a workflow that only mentions `foo:check:extra`. + */ +export function checksMissingFromWorkflows( + checks: readonly string[], + workflowText: string, + allowed: Record = ALLOWED_LOCAL_ONLY, +): string[] { + return checks.filter((name) => { + if (name in allowed) return false; + return !new RegExp(`npm run ${name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}(?![\\w:.-])`).test(workflowText); + }); +} + +function main(): void { + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { scripts: Record }; + const workflowText = readdirSync(WORKFLOWS_DIR) + .filter((entry) => entry.endsWith(".yml") || entry.endsWith(".yaml")) + .map((entry) => readFileSync(join(WORKFLOWS_DIR, entry), "utf8")) + .join("\n"); + + const checks = driftChecksInLocalGate(pkg.scripts); + const missing = checksMissingFromWorkflows(checks, workflowText); + + if (missing.length > 0) { + console.error(`check-ci-drift-checks-wired: ${missing.length} of ${checks.length} check(s) run in the local gate but in NO workflow (#10269):\n`); + for (const name of missing) console.error(` npm run ${name}`); + console.error( + [ + "", + "A check CI never runs does not gate a merge -- it only looks like it does.", + "Fix by adding it to the appropriate place in .github/workflows/ci.yml:", + ' • most checks — the "Drift checks (unconditional)" block in validate-code', + " • needs git history (tags, or a diff against the base) — the drift-checks-history job,", + " since validate-code's checkout is shallow and several checks pass VACUOUSLY there", + "", + "Only as a last resort, add it to ALLOWED_LOCAL_ONLY in this file WITH a reason.", + ].join("\n"), + ); + process.exit(1); + } + + console.log(`check-ci-drift-checks-wired: all ${checks.length} local-gate drift check(s) also run in a workflow.`); +} + +if (process.argv[1]?.endsWith("check-ci-drift-checks-wired.ts")) main(); diff --git a/test/unit/check-ci-drift-checks-wired.test.ts b/test/unit/check-ci-drift-checks-wired.test.ts new file mode 100644 index 000000000..d82b7abb5 --- /dev/null +++ b/test/unit/check-ci-drift-checks-wired.test.ts @@ -0,0 +1,162 @@ +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { + checksMissingFromWorkflows, + driftChecksInLocalGate, + isDriftCheckScript, + reachableNpmScripts, +} from "../../scripts/check-ci-drift-checks-wired"; + +// #10269. `npm run test:ci` and .github/workflows/** are two independently hand-maintained lists, and they +// drifted: 22 of 42 local-gate checks ran in NO workflow, so they gated nothing on a repo whose gate +// auto-merges on green CI. The last block pins the real tree, which is the assertion that actually matters. + +describe("isDriftCheckScript", () => { + it("matches the three naming conventions this repo uses for guards", () => { + expect(isDriftCheckScript("dead-exports:check")).toBe(true); + expect(isDriftCheckScript("manifest:drift-check")).toBe(true); + expect(isDriftCheckScript("ui:version-audit")).toBe(true); + }); + + it("does not sweep in build, test or coverage steps", () => { + // CI runs those through its own jobs; treating them as drift checks would make this unsatisfiable. + for (const name of ["build:mcp", "test:coverage", "typecheck", "ui:build", "rees:install"]) { + expect(isDriftCheckScript(name)).toBe(false); + } + }); + + it("requires the suffix at the END, so a check-shaped prefix does not count", () => { + expect(isDriftCheckScript("foo:check:extra")).toBe(false); + }); +}); + +describe("reachableNpmScripts", () => { + const scripts = { + "test:ci": "npm run a:check && npm run aggregate", + aggregate: "npm run b:check", + "a:check": "node a.ts", + "b:check": "node b.ts", + "orphan:check": "node orphan.ts", + }; + + it("follows references transitively, since checks sit behind aggregate scripts", () => { + const reachable = reachableNpmScripts(scripts, "test:ci"); + expect(reachable.has("a:check")).toBe(true); + expect(reachable.has("b:check")).toBe(true); + expect(reachable.has("orphan:check")).toBe(false); + }); + + it("does not follow a --workspace call as a root script", () => { + // `npm run build --workspace X` targets the workspace's script, not a root one. + const reachable = reachableNpmScripts({ "test:ci": "npm run build --workspace @loopover/engine", build: "x" }, "test:ci"); + expect(reachable.has("build")).toBe(false); + }); + + it("terminates on a cycle rather than hanging", () => { + const reachable = reachableNpmScripts({ "test:ci": "npm run a", a: "npm run test:ci" }, "test:ci"); + expect([...reachable].sort()).toEqual(["a", "test:ci"]); + }); + + it("tolerates a reference to a script that does not exist", () => { + expect(reachableNpmScripts({ "test:ci": "npm run missing:check" }, "test:ci").has("missing:check")).toBe(true); + }); +}); + +describe("driftChecksInLocalGate", () => { + it("returns only the checks, sorted, and never the root itself", () => { + const scripts = { "test:ci": "npm run z:check && npm run a:check && npm run build:mcp", "a:check": "x", "z:check": "x", "build:mcp": "x" }; + expect(driftChecksInLocalGate(scripts)).toEqual(["a:check", "z:check"]); + }); +}); + +describe("checksMissingFromWorkflows", () => { + it("treats a check invoked by any workflow as wired", () => { + expect(checksMissingFromWorkflows(["a:check"], " - run: npm run a:check\n")).toEqual([]); + }); + + it("reports a check no workflow runs", () => { + expect(checksMissingFromWorkflows(["a:check"], " - run: npm run something-else\n")).toEqual(["a:check"]); + }); + + it("REGRESSION: a longer sibling name does not count as wiring the shorter one", () => { + // Without the word-boundary guard, `npm run foo:check:extra` in a workflow would satisfy `foo:check` + // by substring match -- silently marking an unwired check as wired, which is this file's whole failure + // mode rather than a cosmetic bug. + expect(checksMissingFromWorkflows(["foo:check"], "run: npm run foo:check:extra\n")).toEqual(["foo:check"]); + }); + + it("honours an allowlist entry", () => { + expect(checksMissingFromWorkflows(["a:check"], "", { "a:check": "reason" })).toEqual([]); + }); + + it("does not treat a bare mention of the name as an invocation", () => { + // A comment naming the script is not the workflow running it. + expect(checksMissingFromWorkflows(["a:check"], "# see a:check for details\n")).toEqual(["a:check"]); + }); +}); + +describe("the real tree satisfies the invariant (#10269)", () => { + const ciYaml = readFileSync(".github/workflows/ci.yml", "utf8"); + + /** One step's own text: from its `- name:` up to the next step at the same indent. */ + function stepText(name: string): string { + const start = ciYaml.indexOf(` - name: ${name}\n`); + expect(start, `step "${name}" not found in ci.yml`).toBeGreaterThan(-1); + const rest = ciYaml.slice(start + 1); + const next = rest.indexOf(" - name: "); + return next === -1 ? rest : rest.slice(0, next); + } + + /** A step's `if:` condition. */ + function conditionOf(name: string): string { + return /^ +if: (.+)$/m.exec(stepText(name))![1]!; + } + + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { scripts: Record }; + const workflowText = readdirSync(".github/workflows") + .filter((entry) => entry.endsWith(".yml") || entry.endsWith(".yaml")) + .map((entry) => readFileSync(join(".github/workflows", entry), "utf8")) + .join("\n"); + + it("every drift check the local gate runs is also run by a workflow", () => { + const missing = checksMissingFromWorkflows(driftChecksInLocalGate(pkg.scripts), workflowText); + expect(missing).toEqual([]); + }); + + it("the three history-dependent checks run in drift-checks-history, NOT in validate-code", () => { + // The load-bearing placement. In validate-code's shallow checkout db:migrations:immutable:check fails + // outright, while release-commit-parsing:check and releasable-commit-types:check both pass VACUOUSLY -- + // green while verifying nothing, which is the exact failure #10269 exists to remove. If someone moves + // them into the unconditional block, the checks go quietly decorative and this is what catches it. + const historyJob = ciYaml.slice(ciYaml.indexOf(" drift-checks-history:"), ciYaml.indexOf(" validate-tests:")); + const block = stepText("Drift checks (unconditional)"); + + for (const check of ["db:migrations:immutable:check", "release-commit-parsing:check", "releasable-commit-types:check"]) { + expect(historyJob).toContain(`npm run ${check}`); + expect(block).not.toContain(`npm run ${check}`); + } + expect(historyJob).toContain("fetch-depth: 0"); + expect(historyJob).toContain("fetch-tags: true"); + }); + + it("mcp:tool-reference:check runs AFTER the engine build, carrying that build's exact condition", () => { + // It resolves a bare `@loopover/engine` specifier through node_modules to dist/, so in the + // unconditional block -- which runs before "Build engine package" -- it dies with ERR_MODULE_NOT_FOUND. + // CI caught this; locally it passed only because the worktree happened to have a built engine. + // The condition is copied from the build VERBATIM: if the two ever diverge, this check silently stops + // running on the PRs that build the engine, which is the failure mode #10269 is about. + expect(stepText("Drift checks (unconditional)")).not.toContain("npm run mcp:tool-reference:check"); + expect(conditionOf("MCP tool-reference drift check")).toBe(conditionOf("Build engine package")); + expect(ciYaml.indexOf(" - name: MCP tool-reference drift check")).toBeGreaterThan(ciYaml.indexOf(" - name: Build engine package")); + }); + + it("the aggregator depends on drift-checks-history, so its failure actually blocks the PR", () => { + // A job nothing depends on cannot fail a merge -- it would be the same "looks like a gate" problem in a + // new place. + const validateJob = ciYaml.slice(ciYaml.indexOf(" validate:\n")); + expect(validateJob.slice(0, 300)).toContain("drift-checks-history"); + }); +}); diff --git a/test/unit/ci-skip-draft-prs.test.ts b/test/unit/ci-skip-draft-prs.test.ts index 7635ae29a..749cdac32 100644 --- a/test/unit/ci-skip-draft-prs.test.ts +++ b/test/unit/ci-skip-draft-prs.test.ts @@ -29,7 +29,7 @@ describe("ci.yml skips the heavy jobs for draft pull requests", () => { expect(pullRequest.types).toEqual(["opened", "synchronize", "reopened", "ready_for_review"]); }); - it.each(["validate-code", "validate-tests"])( + it.each(["validate-code", "validate-tests", "drift-checks-history"])( "%s's if-condition requires github.event.pull_request.draft != true alongside the existing push/path-filter checks", (jobName) => { const job = record(jobs[jobName], `jobs.${jobName}`); @@ -51,8 +51,12 @@ describe("ci.yml skips the heavy jobs for draft pull requests", () => { }); it("validate still aggregates the gated jobs and treats a skipped dependency as success", () => { + // Enumerated rather than "contains", so a NEW gated job cannot be added without either being + // aggregated here or consciously left out: a job nothing depends on cannot fail the PR, which would + // make it look like a gate while gating nothing. `drift-checks-history` (#10269) is in the list for + // exactly that reason. const job = record(jobs.validate, "jobs.validate"); - expect(job.needs).toEqual(["changes", "validate-code", "validate-tests"]); + expect(job.needs).toEqual(["changes", "validate-code", "validate-tests", "drift-checks-history"]); expect(String(job.if)).toBe("${{ always() }}"); }); });