From 200b3ee2a4a2023ab7f8f64d9bf61d0f0a0b604d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:51:12 -0700 Subject: [PATCH] feat(ci): guard against new *-coverage.test.ts bolt-on files (#8580) Adds scripts/check-coverage-bolt-on-filenames.ts, a drift-style check (alongside docs:drift-check/manifest:drift-check/engine-parity:drift-check) that fails when a test file matches *-coverage.test.ts or *-branch-coverage.test.ts, with an allowlist for genuine exceptions (rees-coverage-script.test.ts tests scripts/rees-coverage.js, real coverage tooling -- the name is a coincidence). Wired into test:ci right after docs:drift-check. This closes out epic #8574: the whole existing bolt-on family (predicted-gate-engine-coverage.test.ts, reward-risk-engine-branch- coverage.test.ts, focus-manifest-engine-branch-coverage.test.ts, signals-coverage.test.ts) has now merged, so the check lands green. Also updates the contributing-to-loopover skill (SKILL.md Phase 3 + reference.md's CI check table) so future contributors are told to extend a module's existing suite instead of bolting on a new coverage-named file. --- .../skills/contributing-to-loopover/SKILL.md | 6 ++ .../contributing-to-loopover/reference.md | 1 + package.json | 3 +- scripts/check-coverage-bolt-on-filenames.ts | 57 +++++++++++++++++ ...-coverage-bolt-on-filenames-script.test.ts | 62 +++++++++++++++++++ 5 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 scripts/check-coverage-bolt-on-filenames.ts create mode 100644 test/unit/check-coverage-bolt-on-filenames-script.test.ts diff --git a/.claude/skills/contributing-to-loopover/SKILL.md b/.claude/skills/contributing-to-loopover/SKILL.md index d05ca48883..a67656b589 100644 --- a/.claude/skills/contributing-to-loopover/SKILL.md +++ b/.claude/skills/contributing-to-loopover/SKILL.md @@ -164,6 +164,12 @@ This is where most PRs fail Codecov. The bar is **every changed line AND every c `createTestEnv()`), `test/workers/` (Cloudflare pool, separate config), plus `test/contract/`. Use the `createTestEnv()` / `TestD1Database` helpers (in-memory SQLite, applies all migrations) and `vi.stubGlobal("fetch", …)` for GitHub calls. Patterns + snippets in `reference.md`. +- **Never close a coverage gap with a new `*-coverage.test.ts` / `*-branch-coverage.test.ts` bolt-on + file.** Add the missing branch case(s) to the module's own existing test file instead. A CI check + (`coverage-boltons:check`, part of `test:ci`) blocks this filename pattern — a whole family of these + (`predicted-gate-engine-coverage.test.ts`, `reward-risk-engine-branch-coverage.test.ts`, + `focus-manifest-engine-branch-coverage.test.ts`, `signals-coverage.test.ts`) accumulated in the repo + and each needed its own dedicated consolidation PR to undo (epic #8574). - **Branch coverage is the trap.** Every `if/else`, ternary `? :`, `&&`/`||`, and especially every nullish fallback `?? 0` / `?? []` is **two branches** — you must exercise *both* sides. The classic miss: a `SUM(...)` over an empty set returns **NULL**, so `count ?? 0` needs a test where the value diff --git a/.claude/skills/contributing-to-loopover/reference.md b/.claude/skills/contributing-to-loopover/reference.md index ba30822c89..007188cd76 100644 --- a/.claude/skills/contributing-to-loopover/reference.md +++ b/.claude/skills/contributing-to-loopover/reference.md @@ -54,6 +54,7 @@ validate the real publishable artifact still compiles and packs cleanly. | docs → drift | doc/code claim checker | `npm run docs:drift-check` | a doc makes a claim the mechanical lint can verify is now false | | docs → command-reference | generated CLI reference drift | `npm run command-reference:check` | committed command-reference doc is stale (run `npm run command-reference`) | | manifest drift | `.loopover.yml` vs bundled fallback YAML | `npm run manifest:drift-check` | `src/config/loopover-repo-focus-manifest.ts`'s bundled YAML diverges from the real root `.loopover.yml` | +| coverage bolt-on filenames | blocks new `*-coverage.test.ts` / `*-branch-coverage.test.ts` files | `npm run coverage-boltons:check` | a new test file matches the bolt-on pattern instead of extending its module's existing suite (see Phase 3) | | engine-parity drift | `src/{review,settings,signals}` vs `loopover-engine` twins | `npm run engine-parity:drift-check` | a hand-duplicated twin file pair diverges, or the installed `@loopover/engine` semver skews from the monorepo package | | branding drift | "gittensory" leaking into runtime source | `npm run branding-drift:check` | a file's gittensory-string hit count rises above the recorded baseline (`scripts/branding-drift-baseline.json`) | | engines/.nvmrc sync | every `engines.node` (root + each workspace that declares one) vs `.nvmrc`'s pinned major | `npm run engines-nvmrc:check` | any declared `engines.node` still allows the major above `.nvmrc`'s pin (paired with the root `.npmrc`'s `engine-strict=true`, which turns that mismatch into a hard local `npm install`/`npm run` error instead of a silently-ignored warning) | diff --git a/package.json b/package.json index 34ffc553dc..945386fc24 100644 --- a/package.json +++ b/package.json @@ -71,6 +71,7 @@ "ui:version-audit": "node --experimental-strip-types scripts/check-ui-mcp-version-copy.ts", "ui:version-audit:sync": "node --experimental-strip-types scripts/check-ui-mcp-version-copy.ts --write", "docs:drift-check": "node --experimental-strip-types scripts/check-docs-drift.ts", + "coverage-boltons:check": "node --experimental-strip-types scripts/check-coverage-bolt-on-filenames.ts", "roadmap:drift-check": "node --experimental-strip-types scripts/check-roadmap-issue-drift.ts", "branding-drift:check": "node --experimental-strip-types scripts/check-branding-drift.ts", "branding-drift:update": "node --experimental-strip-types scripts/check-branding-drift.ts --update", @@ -111,7 +112,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: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 typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-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 command-reference: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: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 typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons: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 command-reference: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-coverage-bolt-on-filenames.ts b/scripts/check-coverage-bolt-on-filenames.ts new file mode 100644 index 0000000000..90c84d8849 --- /dev/null +++ b/scripts/check-coverage-bolt-on-filenames.ts @@ -0,0 +1,57 @@ +#!/usr/bin/env node +// Blocks the "-coverage.test.ts" / "-branch-coverage.test.ts" bolt-on pattern from recurring (#8580, +// epic #8574). A contributor's cheapest path to the 99% Codecov patch-coverage bar is a new file of +// generic "exercises X branches" tests instead of extending the module's existing suite -- that's how +// predicted-gate-engine-coverage.test.ts, reward-risk-engine-branch-coverage.test.ts, +// focus-manifest-engine-branch-coverage.test.ts, and signals-coverage.test.ts all came to exist, each +// requiring a dedicated consolidation PR to undo (#8575/#8577/#8578/#8576). Extend the module's own +// test file instead; if a name below is a genuine exception, add it to ALLOWED_FILENAMES with a reason. +import { readdirSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +const TEST_ROOT = "test"; +const BOLT_ON_PATTERN = /-(branch-)?coverage\.test\.ts$/; + +/** rees-coverage-script.test.ts tests scripts/rees-coverage.js (real coverage-report tooling) -- the + * name is a coincidence, not the bolt-on pattern; the "coverage" in its subject IS the point of the file. */ +const ALLOWED_FILENAMES = new Set(["rees-coverage-script.test.ts"]); + +function defaultListTestFiles(root: string): string[] { + return readdirSync(root, { recursive: true }) + .map(String) + .filter((entry) => entry.endsWith(".test.ts")); +} + +/** Pure: given the repo's test filenames, returns the bolt-on-named ones that aren't allowlisted. + * `listTestFiles` is injectable so tests can simulate a fresh offender without touching the real tree. */ +export function findCoverageBoltOnFilenames(options: { + root?: string; + listTestFiles?: (root: string) => string[]; +} = {}): string[] { + const { root = TEST_ROOT, listTestFiles = defaultListTestFiles } = options; + return listTestFiles(root) + .filter((entry) => BOLT_ON_PATTERN.test(entry.split("/").pop() ?? entry)) + .filter((entry) => !ALLOWED_FILENAMES.has(entry.split("/").pop() ?? entry)) + .sort(); +} + +function main() { + const offenders = findCoverageBoltOnFilenames(); + + if (offenders.length > 0) { + console.error(`Found ${offenders.length} "-coverage.test.ts" / "-branch-coverage.test.ts" bolt-on file(s):`); + for (const offender of offenders) console.error(` test/${offender}`); + console.error( + "\nExtend the module's existing test file instead of adding a new coverage-named bolt-on file " + + "(see .claude/skills/contributing-to-loopover/reference.md's test-patterns section). If this is a " + + "genuine exception, add its filename to ALLOWED_FILENAMES in scripts/check-coverage-bolt-on-filenames.ts " + + "with a one-line reason.", + ); + process.exit(1); + } + + console.log('Coverage bolt-on filename check ok: no new "-coverage.test.ts" / "-branch-coverage.test.ts" files.'); +} + +// Guard so importing this module for its pure export (tests) never triggers the exit side effect. +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/test/unit/check-coverage-bolt-on-filenames-script.test.ts b/test/unit/check-coverage-bolt-on-filenames-script.test.ts new file mode 100644 index 0000000000..4eeb5fac33 --- /dev/null +++ b/test/unit/check-coverage-bolt-on-filenames-script.test.ts @@ -0,0 +1,62 @@ +import { execFileSync } from "node:child_process"; +import { describe, expect, it } from "vitest"; +import { findCoverageBoltOnFilenames } from "../../scripts/check-coverage-bolt-on-filenames.js"; + +describe("check-coverage-bolt-on-filenames script", () => { + it("flags a new *-coverage.test.ts file", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["unit/foo.test.ts", "unit/foo-coverage.test.ts"], + }); + expect(offenders).toEqual(["unit/foo-coverage.test.ts"]); + }); + + it("flags a new *-branch-coverage.test.ts file", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["unit/bar-branch-coverage.test.ts"], + }); + expect(offenders).toEqual(["unit/bar-branch-coverage.test.ts"]); + }); + + it("does not flag a file that merely contains 'coverage' mid-name, not as a bolt-on suffix", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["unit/coverage-report-parser.test.ts", "unit/rees-coverage-script.test.ts"], + }); + expect(offenders).toEqual([]); + }); + + it("allowlists rees-coverage-script.test.ts (tests scripts/rees-coverage.js, real coverage tooling)", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["unit/rees-coverage-script.test.ts"], + }); + expect(offenders).toEqual([]); + }); + + it("sorts the offenders and matches regardless of subdirectory nesting", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["integration/z-coverage.test.ts", "unit/a-branch-coverage.test.ts"], + }); + expect(offenders).toEqual(["integration/z-coverage.test.ts", "unit/a-branch-coverage.test.ts"]); + }); + + it("passes cleanly when nothing matches the bolt-on pattern", () => { + const offenders = findCoverageBoltOnFilenames({ + listTestFiles: () => ["unit/foo.test.ts", "unit/bar.test.ts"], + }); + expect(offenders).toEqual([]); + }); + + // Most important regression test in this file: the real repo, post-consolidation (#8574), must have + // zero bolt-on-named test files. If this fails, a new one has landed -- extend its module's existing + // suite instead of weakening this check. + it("the real repo has zero coverage bolt-on filenames (regression guard)", () => { + const offenders = findCoverageBoltOnFilenames(); + expect(offenders).toEqual([]); + }); + + it("prints a clean summary and exits 0 for the real repo state when run as a subprocess", () => { + const output = execFileSync("node", ["--experimental-strip-types", "scripts/check-coverage-bolt-on-filenames.ts"], { + encoding: "utf8", + }); + expect(output).toMatch(/Coverage bolt-on filename check ok/); + }); +});