feat(ci): add forgotten-sibling-tests quality gate (#9530)

New CI policy gate that detects when a PR changes source file Y but the test siblings of Y's consumers are not in the same diff. Prevents the 'forgotten sibling test' pattern (7 documented occurrences in PR #9529).

- Extracts shared import resolution library (resolveImport, sourceDepsOf) from build-test-impact-map.mjs
- Creates check-forgotten-sibling-tests.mjs gate with consumer-walk logic
- Adds forgotten-sibling-allowlist.json for false positive mitigation
- Wires into pr-test-policy CI job and adds npm script
- Documents in QUALITY_GATES.md
- 17 TDD unit tests for isSourceFile, resolveBase, testSiblingOf, isAllowlisted, findConsumers
This commit is contained in:
diegosouzapw
2026-08-06 21:53:00 -03:00
parent 5f471181fa
commit 6c5329bcfe
9 changed files with 471 additions and 57 deletions

View File

@@ -587,6 +587,9 @@ jobs:
# Anti test-masking: flag net assert removal / new assert.ok(true) in changed tests.
- name: Detect test-masking (weakened assertions)
run: npm run check:test-masking
# Detect forgotten sibling tests (consumers whose test sibling is not in the diff).
- name: Detect forgotten sibling tests
run: npm run check:forgotten-sibling-tests
# Evidence-in-PR-body (Hard Rule #18 mechanized): claims of "tests pass" must carry output.
- name: Require evidence in PR body
run: npm run check:pr-evidence

View File

@@ -0,0 +1 @@
- **feat(ci):** new quality gate `check-forgotten-sibling-tests` detects when a source symbol changes but consumer tests are not in the same diff — preventing the 7 documented "forgotten sibling test" occurrences from PR #9529 ([#9530](https://github.com/diegosouzapw/OmniRoute/issues/9530))

View File

@@ -0,0 +1,10 @@
{
"_comment": "Forgotten-sibling-tests allowlist (check-forgotten-sibling-tests.mjs). Each entry exempts a (sourcePath, forgottenSibling) pair. Use when a consumer test legitimately does not need changes (compatible refactor, same interface). Every entry needs a reason with the tracking issue or PR ref. Wildcard sourcePath=\"*\" exempts the sibling regardless of which source triggered it.",
"_schema": [
{
"sourcePath": "src/lib/example.ts",
"forgottenSibling": "tests/unit/lib/example-consumer.test.ts",
"reason": "Issue #1234: compatible interface change only — consumer tests verify contract, not implementation"
}
]
}

View File

@@ -176,11 +176,12 @@ Full i18n validation matrix (one job per locale). Entire job is advisory.
Runs on pull requests only.
| Script | Validates | Blocking |
| ---------------------- | -------------------------------------------------------------------------------------------------------------------------- | -------- |
| `check:pr-test-policy` | PRs that change production code in `src/`, `open-sse/`, `electron/`, or `bin/` must include or update tests (Hard Rule #8) | Yes |
| `check:test-masking` | Changed test files do not reduce net assert count or add `assert.ok(true)` tautologies | Yes |
| `check:pr-evidence` | PR body cites test/VPS evidence for the change (mechanizes Hard Rule #18 by grepping PR prose — fragile, see Backlog) | Yes |
| Script | Validates | Blocking |
| ------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- |
| `check:pr-test-policy` | PRs that change production code in `src/`, `open-sse/`, `electron/`, or `bin/` must include or update tests (Hard Rule #8) | Yes |
| `check:test-masking` | Changed test files do not reduce net assert count or add `assert.ok(true)` tautologies | Yes |
| `check:pr-evidence` | PR body cites test/VPS evidence for the change (mechanizes Hard Rule #18 by grepping PR prose — fragile, see Backlog) | Yes |
| `check:forgotten-sibling-tests` | When a PR changes source file Y, detects if any production consumer Z of Y has a test sibling that is NOT included in the same diff — preventing the forgotten-sibling-test pattern | Yes |
### Job: `test-vitest`

View File

@@ -229,6 +229,7 @@
"coverage:report": "cross-env NODE_OPTIONS=--max-old-space-size=8192 c8 report --merge-async --output-dir coverage --exclude=tests/** --exclude=**/*.test.* --reporter=text --reporter=text-summary --reporter=html --reporter=json-summary --reporter=lcov",
"coverage:summary": "node scripts/check/test-report-summary.mjs --input coverage/coverage-summary.json --output coverage/coverage-report.md",
"check:pr-test-policy": "node scripts/check/check-pr-test-policy.mjs",
"check:forgotten-sibling-tests": "node scripts/check/check-forgotten-sibling-tests.mjs",
"coverage:report:legacy": "c8 report --output-dir coverage --exclude=open-sse --reporter=text --reporter=text-summary",
"test:all": "npm run test:unit && npm run test:vitest && npm run test:vitest:ui && npm run test:ecosystem && npm run test:e2e",
"check": "npm run lint && npm run test",

View File

@@ -0,0 +1,241 @@
#!/usr/bin/env node
// scripts/check/check-forgotten-sibling-tests.mjs
// Gate: when a PR changes source file Y, detects if any production consumer Z of Y
// has a test sibling (Z.test.ts or Z/index.ts → Z/test.ts) that is NOT included in
// the same PR diff. Prevents the "forgotten sibling test" pattern (7 occurrences
// fixed in PR #9529).
//
// Usage:
// node scripts/check/check-forgotten-sibling-tests.mjs
//
// Environment (PR context):
// GITHUB_BASE_SHA or GITHUB_BASE_REF — base of the PR diff
// FORGOTTEN_SIBLING_MAX_CHANGED — threshold for release-PR skip (default 300)
//
// No PR context → no-ops (exit 0, no output).
import fs from "node:fs";
import path from "node:path";
import { execFileSync } from "node:child_process";
import { pathToFileURL } from "node:url";
import { globSync } from "tinyglobby";
import { ROOT, IMPORT_RE, EXTS, SRC_ROOTS, resolveImport } from "./lib/importResolution.mjs";
// ─── Constants ────────────────────────────────────────────────────────────────
const SOURCE_ROOTS = ["src/", "open-sse/", "bin/"];
const EXCLUDED_PATTERNS = [
/\/tests\//,
/\/migrations\//,
/\/__tests__\//,
/\.test\./,
/\.spec\./,
/\/node_modules\//,
/\/config\/(?:quality|eslint|tsconfig)/,
];
const DEFAULT_MAX_CHANGED = 300;
// ─── Helpers (exported for testing) ───────────────────────────────────────────
export function runGit(args) {
return execFileSync("git", args, { encoding: "utf8" }).trim();
}
export function isSourceFile(filePath) {
if (EXCLUDED_PATTERNS.some((p) => p.test(filePath))) return false;
return (
SOURCE_ROOTS.some((root) => filePath.startsWith(root)) && EXTS.some((e) => filePath.endsWith(e))
);
}
export function resolveBase() {
if (process.env.GITHUB_BASE_SHA) return process.env.GITHUB_BASE_SHA;
if (process.env.GITHUB_BASE_REF) return `origin/${process.env.GITHUB_BASE_REF}`;
return null;
}
/**
* Test sibling of a production file Z.
* Convention: Z.ts → Z.test.ts, or Z/index.ts → Z/test.ts.
* Returns the repo-relative path of the test sibling, or null if none.
*/
export function testSiblingOf(relPath) {
const abs = path.join(ROOT, relPath);
const dir = path.dirname(abs);
const base = path.basename(abs).replace(/\.(ts|tsx|mts|js|mjs)$/, "");
const candidates = [
path.join(dir, `${base}.test.ts`),
path.join(dir, `${base}.test.tsx`),
path.join(dir, `${base}.test.mjs`),
];
// Also try Z/test/ subdirectory
const testDir = path.join(dir.replace(/\/?$/, ""), "test");
candidates.push(
path.join(testDir, `${base}.test.ts`),
path.join(testDir, `${base}.test.tsx`),
path.join(testDir, `${base}.test.mjs`)
);
// For Z/index.ts or Z/route.ts, also try Z/test.ts
if (base === "index" || base === "route") {
candidates.push(
path.join(dir, `${base}.test.ts`),
path.join(dir, `${base}.test.tsx`),
path.join(dir, `${base}.test.mjs`),
path.join(dir, "test.ts"),
path.join(dir, "test.tsx"),
path.join(dir, "test.mjs")
);
}
for (const c of candidates) {
if (fs.existsSync(c)) return path.relative(ROOT, c);
}
return null;
}
/**
* For a given changed source file (repo-relative path), find all production
* files under SOURCE_ROOTS that directly import from it. Uses the pre-built
* reverse dependency map.
*/
export function findConsumers(changedRelPath, prodFileMap) {
const abs = path.join(ROOT, changedRelPath);
const consumers = [];
for (const [consumerRel, deps] of Object.entries(prodFileMap)) {
if (deps.has(abs)) consumers.push(consumerRel);
}
return consumers.sort();
}
/**
* Build a map of all production files → their resolved direct import deps (Set of absolute paths).
*/
export function buildProdFileMap() {
const map = {};
const prodFiles = globSync(
SRC_ROOTS.map((r) => `${r}/**/*.{ts,tsx,mts,js,mjs}`),
{ cwd: ROOT, ignore: ["**/node_modules/**", "**/tests/**", "**/__tests__/**"] }
);
for (const f of prodFiles) {
if (!isSourceFile(f)) continue;
const fullPath = path.join(ROOT, f);
let code;
try {
code = fs.readFileSync(fullPath, "utf8");
} catch {
continue;
}
const deps = new Set();
for (const m of code.matchAll(IMPORT_RE)) {
const spec = m[1] || m[2] || m[3];
if (!spec) continue;
const r = resolveImport(spec, fullPath);
if (!r) continue;
deps.add(r);
}
map[f] = deps;
}
return map;
}
/**
* Check if an allowlist entry exempts a (changedFile, consumerWithMissingTest) pair.
*/
export function isAllowlisted(changedFile, missingTest, allowlist) {
for (const entry of allowlist) {
if (entry.sourcePath === changedFile && entry.forgottenSibling === missingTest) return true;
if (entry.sourcePath === "*" && entry.forgottenSibling === missingTest) return true;
}
return false;
}
// ─── Main ─────────────────────────────────────────────────────────────────────
function main() {
const base = resolveBase();
if (!base) {
console.log("[forgotten-sibling] no base ref (not a PR context) — skipping check.");
return;
}
// Read allowlist
let allowlist = [];
try {
const raw = JSON.parse(
fs.readFileSync(path.join(ROOT, "config/quality/forgotten-sibling-allowlist.json"), "utf8")
);
allowlist = Array.isArray(raw) ? raw : [];
} catch {
// No allowlist file or parse error — treat as empty
}
// Get changed files
const changedFiles = runGit(["diff", "--name-only", "--diff-filter=ACM", `${base}...HEAD`])
.split(/\r?\n/)
.map((s) => s.trim())
.filter(Boolean);
const changedSources = changedFiles.filter(isSourceFile);
const changedTestFiles = new Set(
changedFiles.filter((f) => /\.(?:test|spec)\.(?:ts|tsx|mjs)$/.test(f))
);
// Release PR skip: if too many changed files, skip the detailed consumer walk.
const maxChanged = Number(process.env.FORGOTTEN_SIBLING_MAX_CHANGED) || DEFAULT_MAX_CHANGED;
if (maxChanged > 0 && changedSources.length > maxChanged) {
console.log(
`[forgotten-sibling] ${changedSources.length} source file(s) changed exceeds ` +
`threshold (${maxChanged}) — skipping consumer scan (release PR).\n` +
` A diff this large is a release PR or mass refactor; each file already ` +
`passed this gate on its own PR during the cycle.`
);
return;
}
if (changedSources.length === 0) {
console.log("[forgotten-sibling] no changed source files — OK.");
return;
}
// Build the production dependency map (source → consumers)
const prodFileMap = buildProdFileMap();
const flags = [];
for (const changedSource of changedSources) {
const consumers = findConsumers(changedSource, prodFileMap);
if (consumers.length === 0) continue;
for (const consumer of consumers) {
const testSibling = testSiblingOf(consumer);
if (!testSibling) continue;
if (changedTestFiles.has(testSibling)) continue;
if (isAllowlisted(changedSource, testSibling, allowlist)) continue;
flags.push(
`${changedSource}: \`${consumer}\` imports this file and has a test sibling ` +
`(\`${testSibling}\`) that is NOT in the current diff. ` +
"When the public API of the changed source changes, consumer tests " +
"may need updating too."
);
}
}
if (flags.length) {
console.error(
`[forgotten-sibling] ${flags.length} forgotten sibling test(s) detected:\n` +
flags.map((f) => `${f}`).join("\n") +
`\n → Add the missing test files to this PR or add an allowlist entry ` +
`(config/quality/forgotten-sibling-allowlist.json) with a justification.`
);
process.exit(1);
}
console.log(
`[forgotten-sibling] OK — ${changedSources.length} source file(s) changed, ` +
`no forgotten sibling tests.`
);
}
if (import.meta.url === pathToFileURL(process.argv[1]).href) main();

View File

@@ -0,0 +1,67 @@
#!/usr/bin/env node
// scripts/check/lib/importResolution.mjs
// Shared import resolution logic extracted from build-test-impact-map.mjs.
// Provides resolveImport(), sourceDepsOf(), IMPORT_RE, EXTS, SRC_ROOTS, ROOT.
import fs from "node:fs";
import path from "node:path";
import { fileURLToPath } from "node:url";
export const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../../..");
export const SRC_ROOTS = ["src", "open-sse"];
export const IMPORT_RE =
/(?:import|export)[^'"]*from\s*['"]([^'"]+)['"]|require\(\s*['"]([^'"]+)['"]\s*\)|import\(\s*['"]([^'"]+)['"]\s*\)/g;
export const EXTS = [".ts", ".tsx", ".mts", ".js", ".mjs"];
/**
* Resolve an import specifier to an absolute file path.
* Handles `@/` aliases, `@omniroute/open-sse` aliases, and relative paths.
* Returns null for external/npm imports or unresolvable specs.
*/
export function resolveImport(spec, fromFile) {
let base;
if (spec.startsWith("@/")) base = path.join(ROOT, "src", spec.slice(2));
else if (spec.startsWith("@omniroute/open-sse"))
base = path.join(ROOT, "open-sse", spec.replace(/^@omniroute\/open-sse\/?/, ""));
else if (spec.startsWith(".")) base = path.resolve(path.dirname(fromFile), spec);
else return null;
for (const e of EXTS) {
if (fs.existsSync(base + e)) return base + e;
}
for (const e of EXTS) {
const idx = path.join(base, "index" + e);
if (fs.existsSync(idx)) return idx;
}
return fs.existsSync(base) && fs.statSync(base).isFile() ? base : null;
}
/**
* Walk the transitive import graph of a file and return all source-relative
* paths it depends on (files under src/ or open-sse/).
*/
export function sourceDepsOf(entry) {
const seen = new Set();
const stack = [entry];
const sources = new Set();
while (stack.length) {
const f = stack.pop();
if (seen.has(f)) continue;
seen.add(f);
let code;
try {
code = fs.readFileSync(f, "utf8");
} catch {
continue;
}
for (const m of code.matchAll(IMPORT_RE)) {
const spec = m[1] || m[2] || m[3];
if (!spec) continue;
const r = resolveImport(spec, f);
if (!r) continue;
const rel = path.relative(ROOT, r);
if (SRC_ROOTS.some((s) => rel.startsWith(s + path.sep))) sources.add(rel);
stack.push(r);
}
}
return sources;
}

View File

@@ -1,57 +1,14 @@
import fs from "node:fs";
import path from "node:path";
import { fileURLToPath } from "node:url";
import { globSync } from "tinyglobby";
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../..");
const SRC_ROOTS = ["src", "open-sse"];
const IMPORT_RE =
/(?:import|export)[^'"]*from\s*['"]([^'"]+)['"]|require\(\s*['"]([^'"]+)['"]\s*\)|import\(\s*['"]([^'"]+)['"]\s*\)/g;
const EXTS = [".ts", ".tsx", ".mts", ".js", ".mjs"];
function resolveImport(spec, fromFile) {
let base;
if (spec.startsWith("@/")) base = path.join(ROOT, "src", spec.slice(2));
else if (spec.startsWith("@omniroute/open-sse"))
base = path.join(ROOT, "open-sse", spec.replace(/^@omniroute\/open-sse\/?/, ""));
else if (spec.startsWith(".")) base = path.resolve(path.dirname(fromFile), spec);
else return null;
for (const e of EXTS) {
if (fs.existsSync(base + e)) return base + e;
}
for (const e of EXTS) {
const idx = path.join(base, "index" + e);
if (fs.existsSync(idx)) return idx;
}
return fs.existsSync(base) && fs.statSync(base).isFile() ? base : null;
}
function sourceDepsOf(entry) {
const seen = new Set();
const stack = [entry];
const sources = new Set();
while (stack.length) {
const f = stack.pop();
if (seen.has(f)) continue;
seen.add(f);
let code;
try {
code = fs.readFileSync(f, "utf8");
} catch {
continue;
}
for (const m of code.matchAll(IMPORT_RE)) {
const spec = m[1] || m[2] || m[3];
if (!spec) continue;
const r = resolveImport(spec, f);
if (!r) continue;
const rel = path.relative(ROOT, r);
if (SRC_ROOTS.some((s) => rel.startsWith(s + path.sep))) sources.add(rel);
stack.push(r);
}
}
return sources;
}
import {
ROOT,
IMPORT_RE,
EXTS,
SRC_ROOTS,
resolveImport,
sourceDepsOf,
} from "../check/lib/importResolution.mjs";
// Mirror EXACTLY the `npm run test:unit` glob — the curated set of node:test files.
// The TIA step runs the selected subset via `node --test`, so it must NOT include
@@ -79,7 +36,10 @@ for (const tf of testFiles) {
}
for (const k of Object.keys(map)) map[k].sort();
const out = path.join(ROOT, "config/quality/test-impact-map.json");
fs.writeFileSync(out, JSON.stringify({ generatedFrom: "import-graph", sources: map }, null, 2) + "\n");
fs.writeFileSync(
out,
JSON.stringify({ generatedFrom: "import-graph", sources: map }, null, 2) + "\n"
);
console.log(
`test-impact-map: ${Object.keys(map).length} source files mapped from ${testFiles.length} test files`
);

View File

@@ -0,0 +1,130 @@
// tests/unit/build/check-forgotten-sibling-tests.test.mjs
// TDD tests for the forgotten-sibling-tests gate (check-forgotten-sibling-tests.mjs).
import assert from "node:assert";
import { describe, it } from "node:test";
import {
testSiblingOf,
isAllowlisted,
isSourceFile,
resolveBase,
findConsumers,
} from "../../../scripts/check/check-forgotten-sibling-tests.mjs";
import { ROOT } from "../../../scripts/check/lib/importResolution.mjs";
describe("check-forgotten-sibling-tests", () => {
describe("isSourceFile", () => {
it("returns true for src/ .ts files", () => {
assert.strictEqual(isSourceFile("src/lib/foo.ts"), true);
});
it("returns true for open-sse/ files", () => {
assert.strictEqual(isSourceFile("open-sse/services/foo.ts"), true);
});
it("returns true for bin/ files", () => {
assert.strictEqual(isSourceFile("bin/cli.ts"), true);
});
it("returns false for test files under tests/", () => {
assert.strictEqual(isSourceFile("tests/unit/foo.test.ts"), false);
});
it("returns false for migration files", () => {
assert.strictEqual(isSourceFile("src/lib/db/migrations/001.sql"), false);
});
it("returns false for node_modules", () => {
assert.strictEqual(isSourceFile("node_modules/foo/index.ts"), false);
});
it("returns false for markdown files", () => {
assert.strictEqual(isSourceFile("docs/readme.md"), false);
});
});
describe("resolveBase", () => {
it("returns GITHUB_BASE_SHA when set", () => {
const prev = process.env.GITHUB_BASE_SHA;
process.env.GITHUB_BASE_SHA = "abc123";
assert.strictEqual(resolveBase(), "abc123");
process.env.GITHUB_BASE_SHA = prev;
});
it("returns origin/REF when only GITHUB_BASE_REF is set", () => {
const prev = process.env.GITHUB_BASE_REF;
delete process.env.GITHUB_BASE_SHA;
process.env.GITHUB_BASE_REF = "release/v3.8.50";
assert.strictEqual(resolveBase(), "origin/release/v3.8.50");
process.env.GITHUB_BASE_REF = prev;
});
it("returns null when neither env var is set", () => {
const prevSha = process.env.GITHUB_BASE_SHA;
const prevRef = process.env.GITHUB_BASE_REF;
delete process.env.GITHUB_BASE_SHA;
delete process.env.GITHUB_BASE_REF;
assert.strictEqual(resolveBase(), null);
process.env.GITHUB_BASE_SHA = prevSha;
process.env.GITHUB_BASE_REF = prevRef;
});
});
describe("testSiblingOf", () => {
it("returns null for a file that has no test sibling", () => {
const result = testSiblingOf("src/lib/db/core.ts");
assert.strictEqual(result, null);
});
});
describe("isAllowlisted", () => {
const allowlist = [
{
sourcePath: "src/lib/example.ts",
forgottenSibling: "tests/unit/lib/consumer.test.ts",
reason: "test",
},
{
sourcePath: "*",
forgottenSibling: "tests/unit/lib/wildcard.test.ts",
reason: "wildcard test",
},
];
it("returns true for exact match", () => {
assert.strictEqual(
isAllowlisted("src/lib/example.ts", "tests/unit/lib/consumer.test.ts", allowlist),
true
);
});
it("returns true for wildcard sourcePath", () => {
assert.strictEqual(
isAllowlisted("src/lib/other.ts", "tests/unit/lib/wildcard.test.ts", allowlist),
true
);
});
it("returns false for non-allowlisted pair", () => {
assert.strictEqual(
isAllowlisted("src/lib/example.ts", "tests/unit/lib/other.test.ts", allowlist),
false
);
});
it("returns false for empty allowlist", () => {
assert.strictEqual(
isAllowlisted("src/lib/example.ts", "tests/unit/lib/consumer.test.ts", []),
false
);
});
});
describe("findConsumers", () => {
it("returns consumers that import the given file", () => {
const absPath = ROOT + "/src/lib/target.ts";
const prodFileMap = {
"src/lib/consumer.ts": new Set([absPath]),
"src/lib/unrelated.ts": new Set(["/other/path.ts"]),
};
const result = findConsumers("src/lib/target.ts", prodFileMap);
assert.deepStrictEqual(result, ["src/lib/consumer.ts"]);
});
it("returns empty array when no consumers import the file", () => {
const prodFileMap = {
"src/lib/consumer.ts": new Set([ROOT + "/src/lib/other.ts"]),
};
const result = findConsumers("src/lib/target.ts", prodFileMap);
assert.deepStrictEqual(result, []);
});
});
});