From a40b216bcd10f2e67428598551696a8056de4d3f Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 04:33:07 -0300 Subject: [PATCH 1/3] Guard against silently unrun CLI test files (Fixes #2923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Vitest baseExclude contract this issue was filed against is gone: PR #3056 replaced it with structural discovery in packages/cli/run-bun-tests.ts, and all 670 tracked CLI test files are now discovered and run. What was missing is the third resolution the issue asks for — something that makes a future silent exclusion fail loudly. run-bun-tests.ts walks a hardcoded TEST_ROOTS list, so a tracked test file added anywhere else under packages/cli would never run while every existing test still passed. The new guard compares the git-tracked test set against the runner's own discoverTestFiles() and fails, naming the file, when the two disagree or when a path is discovered more than once. --- .github/workflows/ci.yml | 4 + package.json | 1 + project-plans/issue2923/plan.md | 134 +++++ scripts/check-cli-test-discovery.ts | 292 +++++++++++ .../check-cli-test-discovery.bun.test.ts | 468 ++++++++++++++++++ 5 files changed, 899 insertions(+) create mode 100644 project-plans/issue2923/plan.md create mode 100644 scripts/check-cli-test-discovery.ts create mode 100644 scripts/tests/check-cli-test-discovery.bun.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ca1939e8c9..daa172426e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -379,6 +379,10 @@ jobs: run: |- npm run lint:no-new-js + - name: 'Run CLI test-discovery guard (#2923)' + run: |- + npm run lint:cli-test-discovery + - name: 'Run copyright-year guard (#2820)' env: COPYRIGHT_GUARD_BASE: >- diff --git a/package.json b/package.json index 13e4f9e502..5a69a86ebc 100644 --- a/package.json +++ b/package.json @@ -99,6 +99,7 @@ "lint:genai-enclave": "bun scripts/check-genai-enclave.ts", "lint:legacy-paths": "bun scripts/check-legacy-paths.ts", "lint:no-new-js": "bun scripts/check-no-new-js-files.ts", + "lint:cli-test-discovery": "bun scripts/check-cli-test-discovery.ts", "lint:copyright-year": "bun scripts/check-copyright-year.ts", "lint:doc-links": "bun scripts/check-doc-links.ts", "lint:doc-placement": "bun scripts/check-doc-placement.ts", diff --git a/project-plans/issue2923/plan.md b/project-plans/issue2923/plan.md new file mode 100644 index 0000000000..61343b8992 --- /dev/null +++ b/project-plans/issue2923/plan.md @@ -0,0 +1,134 @@ +# Issue #2923 — CLI test files must never be silently excluded + +## 1. Ground truth on `main` (measured, not assumed) + +The issue was filed against a Vitest setup that no longer exists. PR #3056 +("Migrate the CLI workspace to Bun-native test execution", Fixes #2843) deleted +`packages/cli/vitest.config.ts`, `packages/cli/vitest.test-groups.ts`, +`baseExclude` and `SELECTED_FILE_COUNT`, and replaced them with +`packages/cli/run-bun-tests.ts`, which discovers test files structurally with — +in its own words — "no manifest, allow-list or exclude list". + +Measurements taken on `main` at 42ca2a989: + +| Check | Result | +| -------------------------------------------------------------------- | ------------ | +| `git ls-files packages/cli` matching `.(test\|spec\|bun).(ts\|tsx)` | 670 | +| `discoverTestFiles()` from `run-bun-tests.ts` | 670 | +| Structurally excluded files | **0** | +| Issue repro: `bun test ./src/ui/components/ModelConfigDialog.test.tsx` | **21 pass** | +| `src/ui/components/*.test.tsx`, one process per file | **39/39 pass** | +| Files containing unconditional `describe.skip`/`it.skip`/`test.skip` | **0** | +| Tracked test files inside skipped dirs (`dist`, `coverage`, dot-dirs…) | **0** | +| Tracked test files outside `TEST_ROOTS` | **0** | + +CI path confirmed: `npm run test:ci --workspaces` → `packages/cli` → +`bun run-bun-tests.ts`. The runner used in CI is the runner measured above. + +### Conditional-skip audit (a qualification, not a structural exclusion) + +"Nothing is excluded" is a claim about **discovery**: no tracked test file is +structurally unreachable. It is not a claim that every discovered file executes +assertions in every environment. A separate audit of `skipIf` found: + +- Most uses are platform gates (`process.platform === 'win32'`, clipboard + availability, unreadable-path support). These are legitimate: the case cannot + run on that OS, and it does run on the others. +- One file, `packages/cli/test/ui/commands/authCommand-logout.test.ts`, gates + all four of its suites on `process.env.CI === 'true'`. Measured: under + `CI=true` it reports **0 pass / 21 skip**; under `CI=false`, **21 pass**. + +That file is discovered and invoked — the runner and this guard both do their +job — but it asserts nothing on CI. It is an explicit, greppable, deliberate +skip rather than the silent structural exclusion #2923 is about, and unpicking +an OAuth logout suite is a different subsystem. It is therefore **out of scope +here and reported separately** rather than fixed in this change. + +### Consequence for the issue's three proposed resolutions + +1. **"Fix the harness so Ink component tests can render, delete the exclude + patterns, raise `SELECTED_FILE_COUNT`."** — Already satisfied. The exclude + patterns and the count oracle are gone; the Ink component tests render and + pass. Nothing to do. +2. **"Delete any genuinely superseded test file rather than leaving it + excluded."** — Vacuous. Nothing is excluded, so nothing is left excluded. + No test file is deleted by this change. +3. **"Add a guard so a test file cannot be added without landing in exactly one + routing group, making a future silent exclusion fail loudly."** — + **Not satisfied. This is the work.** + +## 2. The gap that remains + +`run-bun-tests.ts` walks a hardcoded root list: + + const TEST_ROOTS = ['src', 'test', 'test-bun', 'test-utils']; + +A tracked test file added anywhere else under `packages/cli` — `scripts/`, +`bin/`, the workspace root, or any new directory — is silently never run, and +every existing test still passes. `packages/cli/scripts/` and +`packages/cli/bin/` already exist, so this is reachable, not hypothetical. + +The existing tests in `packages/cli/test/run-bun-tests.test.ts` pin discovery +behaviour against synthetic temp directories only. Nothing asserts that the +**real** workspace is fully covered, so the exact regression #2923 describes — +a test file that exists but never runs — would reproduce today without any +signal. + +## 3. Accepted behaviour + +A repo guard, modelled on the established `scripts/check-*.ts` pattern +(`check-test-shards.ts`, `check-no-new-js-files.ts`), that fails loudly when a +CLI test file is not run. + +| ID | Behaviour | +| --- | --------- | +| AC1 | When a tracked test file under `packages/cli` is not returned by the runner's `discoverTestFiles()`, the guard exits non-zero and names the offending file(s) with an actionable fix message. | +| AC2 | Against the real repo as it stands, the guard exits 0 and reports the covered count. | +| AC3 | When `discoverTestFiles()` returns the same file more than once, the guard exits non-zero — the "exactly one" half of the contract. | +| AC4 | The guard classifies repo files using its **own** pattern constant, independent of the runner's `TEST_FILE_PATTERN`. Narrowing the runner's pattern (e.g. dropping `.bun`) must fail the guard rather than silently shrink both sides of the comparison. | +| AC5 | The guard runs on every PR: wired into `package.json` as a `lint:*` script and invoked from the CI lint job next to the sibling guards. | +| AC6 | Behavioural tests prove AC1–AC4 — pure comparison helpers directly, the real repo end-to-end, and a temp git repo where a rogue test file outside the roots produces the exact failure message. | + +### Inputs and boundary cases + +- **Oracle is `git ls-files`.** Untracked local scratch files must NOT fail the + guard; they are not part of the repo and CI never sees them. A new test file + is caught when it is committed, which is the gate that matters. +- `node_modules`, `dist`, `coverage` are gitignored, therefore never tracked, + therefore never candidates. +- A tracked test file inside a directory the runner skips (`__snapshots__`, a + dot-directory) is a real silent exclusion and must fail. None exist today. +- Paths are normalised to POSIX so the guard behaves identically on Windows. +- `git` missing or the directory not being a repo fails closed with a + diagnosable message, never a raw stack trace. + +### Explicitly out of scope + +- Changing `TEST_ROOTS` or any runner discovery behaviour. The guard makes the + gap loud; widening discovery is a separate decision. +- Any other workspace's test configuration. +- Deleting or rewriting any test file. +- Re-introducing a file-count oracle. Set equality against `git ls-files` is + strictly stronger than the `SELECTED_FILE_COUNT` integer the issue names, and + does not generate churn on every added test. + +## 4. Deliverables + +- `scripts/check-cli-test-discovery.ts` — the guard. `evaluateDiscovery()` holds + the whole decision and returns a verdict plus the exact text to print; + `main()` is a thin shell over it (gather inputs, print, exit) so both halves + of the contract are covered by tests of real decision-making rather than of + helpers the program might not consult. `findUndiscoveredTestFiles()` and + `findDuplicateDiscoveries()` are exported for direct unit coverage. +- `scripts/tests/check-cli-test-discovery.bun.test.ts` — behavioural tests + (`bun:test`). +- `package.json` — `lint:cli-test-discovery` script. +- `.github/workflows/ci.yml` — guard step in the lint job. + +## 5. Evidence required to close + +- Guard passes on the real repo (AC2), and fails with the named file in a temp + repo (AC1) and on a duplicate (AC3) and on a narrowed runner pattern (AC4). +- `npm run test`, `lint`, `typecheck`, `format`, `build`, and the CLI smoke all + pass locally. +- CI green on the PR. diff --git a/scripts/check-cli-test-discovery.ts b/scripts/check-cli-test-discovery.ts new file mode 100644 index 0000000000..55b8a71893 --- /dev/null +++ b/scripts/check-cli-test-discovery.ts @@ -0,0 +1,292 @@ +#!/usr/bin/env bun +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * check-cli-test-discovery.ts + * + * Issue #2923 — guard that every tracked CLI test file is actually discovered + * by the structural test runner. + * + * Background: the CLI workspace used to run under Vitest with a large + * `baseExclude` glob list plus a separate integration-only command + * (`SELECTED_FILE_COUNT`). Files matching either were silently never run, and + * they drifted out of sync with the product with no signal. PR #3056 replaced + * that setup with `packages/cli/run-bun-tests.ts`, which discovers test files + * purely structurally — no manifest, no allow-list, no exclude-list. + * + * The remaining failure mode is NOT a list: it is that the runner walks a + * hardcoded `TEST_ROOTS` list (`['src', 'test', 'test-bun', 'test-utils']`). + * A tracked test file added anywhere else under `packages/cli` (`scripts/`, + * `bin/`, the workspace root, or a brand-new directory) is silently never run, + * and every existing test still passes. This guard compares the git-tracked + * test files against the runner's `discoverTestFiles()` and fails loudly when + * the two sets disagree — keeping structural discovery honest. + * + * Design (mirrors scripts/check-no-new-js-files.ts): + * - **Tracked files only.** `git ls-files` is the source of truth for what is + * committed; untracked/generated files never produce a false positive. + * - **Own pattern.** Candidates are classified by this guard's OWN + * `CLI_TEST_FILE_PATTERN`, deliberately NOT imported from the runner. + * Importing it would let both sides of the comparison shrink together + * (e.g. dropping `.bun` from the runner) and defeat the guard. + * - **Real runner.** The discovered set comes from the REAL + * `discoverTestFiles` in `packages/cli/run-bun-tests.ts`, not a copy. + * + * Usage: + * bun scripts/check-cli-test-discovery.ts # enforce (CI) + * bun scripts/check-cli-test-discovery.ts --root # alternate CLI workspace (tests) + */ + +import { execFileSync } from 'node:child_process'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { discoverTestFiles } from '../packages/cli/run-bun-tests.ts'; + +const EXIT_PASS = 0; +const EXIT_FAIL = 1; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const DEFAULT_CLI_ROOT = resolve(__dirname, '..', 'packages', 'cli'); + +/** + * The test-file naming convention this guard looks for. This is a DELIBERATE + * duplicate of `TEST_FILE_PATTERN` in `packages/cli/run-bun-tests.ts`. It is + * NOT imported from there on purpose: if the runner's pattern were narrowed + * (e.g. `.bun` dropped), importing it would make both the "candidate" side and + * the "discovered" side shrink together, and a dropped file would silently + * pass. Duplicating keeps the two sides independent so a narrowing fails here. + * + * Exported so the test suite can assert the pattern classifies candidates + * without deferring to the runner (AC4). + */ +export const CLI_TEST_FILE_PATTERN = /\.(test|spec|bun)\.(ts|tsx)$/; + +interface ParsedArgs { + readonly root: string; +} + +function parseArgs(argv: readonly string[]): ParsedArgs { + let root = DEFAULT_CLI_ROOT; + for (let i = 0; i < argv.length; i++) { + const a = argv[i]; + if (a === '--root') { + const value = argv[++i]; + if (value === undefined) { + failWith('--root requires a directory argument.'); + } + root = resolve(value); + } else { + // Fail fast: silently ignoring an unknown flag would let a typo such as + // `--rot /tmp/fixture` run against the default repo root and report a + // PASS for a workspace the caller never meant to check. + failWith(`unknown argument "${a}". Supported: --root .`); + } + } + return { root }; +} + +/** Print an error and exit(1). Kept tiny so arg-validation stays readable. */ +function failWith(message: string): never { + console.error(`cli-test-discovery guard: ${message}`); + process.exit(EXIT_FAIL); +} + +// ─── Git interaction ──────────────────────────────────────────────────────── + +/** + * List tracked test files (by the guard's own pattern) under `cliRoot`. + * Returns POSIX paths relative to `cliRoot`, sorted. + * + * `git ls-files` is the source of truth for what is committed: untracked and + * generated files are never reported. Paths are returned exactly as Git + * reports them (POSIX, no surrounding whitespace). Git stores paths with + * forward slashes even on Windows and the NUL-delimited `-z` output has no + * trailing whitespace, so neither `trim()` nor backslash normalization is + * applied — both could mask distinct filenames and let a missing file slip by. + */ +function listTrackedTestFiles(cliRoot: string): string[] { + let out: string; + try { + out = execFileSync('git', ['ls-files', '-z'], { + cwd: cliRoot, + encoding: 'utf8', + timeout: 60_000, + }); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + throw new Error( + `cli-test-discovery guard: \`git ls-files\` failed in ${cliRoot} ` + + `(is this a git repository and is git on PATH?): ${msg}`, + ); + } + const files = out + .split('\0') + .filter((f) => f.length > 0 && CLI_TEST_FILE_PATTERN.test(f)); + return sortPosix(files); +} + +// ─── Pure comparison logic (exported for unit tests) ──────────────────────── + +/** + * Returns the tracked test files that are NOT returned by the runner's + * `discoverTestFiles()`, sorted POSIX. This is the single "every tracked test + * is discovered" decision (the AC1 half of the contract); pure so it can be + * unit-tested without git or the filesystem. + */ +export function findUndiscoveredTestFiles( + trackedTestFiles: readonly string[], + discoveredFiles: readonly string[], +): string[] { + const discovered = new Set(discoveredFiles.map(toPosix)); + const missing: string[] = []; + for (const f of trackedTestFiles) { + const normalized = toPosix(f); + if (!discovered.has(normalized)) { + missing.push(normalized); + } + } + return sortPosix(missing); +} + +/** + * Returns paths the runner returned more than once, sorted POSIX. This is the + * "exactly one" half of the contract (AC3): a duplicate discovery would either + * double a file's run or, after a dedupe, silently hide that two roots overlap. + */ +export function findDuplicateDiscoveries( + discoveredFiles: readonly string[], +): string[] { + const counts = new Map(); + for (const raw of discoveredFiles) { + const normalized = toPosix(raw); + counts.set(normalized, (counts.get(normalized) ?? 0) + 1); + } + const dups: string[] = []; + for (const [file, count] of counts) { + if (count > 1) { + dups.push(file); + } + } + return sortPosix(dups); +} + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +function toPosix(p: string): string { + return p.replace(/\\/g, '/'); +} + +function sortPosix(files: readonly string[]): string[] { + // Fixed 'en' locale for deterministic ordering across environments + // (developer machines vs CI with different LANG/LC_ALL settings). + return [...files].sort((a, b) => toPosix(a).localeCompare(toPosix(b), 'en')); +} + +const FIX_HINT = + 'Fix: move the file under an existing entry of `TEST_ROOTS` in ' + + 'packages/cli/run-bun-tests.ts (src, test, test-bun, test-utils), or add its ' + + 'parent directory to `TEST_ROOTS`. Never add it to an exclude list — the ' + + 'whole point of structural discovery is that nothing is excluded.'; + +// ─── Decision (exported for behavioural tests) ────────────────────────────── + +export interface DiscoveryVerdict { + /** True when every tracked test file is discovered exactly once. */ + readonly ok: boolean; + /** The exact text the guard prints — to stdout on pass, stderr on fail. */ + readonly report: string; +} + +/** + * The guard's whole decision: given the tracked set and the runner's discovered + * set, is the discovery contract upheld, and what should be printed? + * + * `main()` is deliberately a thin shell over this function (gather inputs, + * print `report`, exit on `ok`) so that both halves of the contract — "every + * tracked test is discovered" and "each is discovered exactly once" — are + * covered by tests of real decision-making rather than of helpers that the + * program might not actually consult. + */ +export function evaluateDiscovery( + trackedTestFiles: readonly string[], + discoveredFiles: readonly string[], +): DiscoveryVerdict { + const duplicates = findDuplicateDiscoveries(discoveredFiles); + if (duplicates.length > 0) { + const plural = duplicates.length === 1 ? '' : 's'; + return { + ok: false, + report: [ + `\ncli-test-discovery guard FAILED: discoverTestFiles() returned ` + + `${duplicates.length} path${plural} more than once:\n`, + ...duplicates.map((d) => ` ${d}`), + '\nEach file must be discovered exactly once. Check for overlapping ' + + '`TEST_ROOTS` or nested roots in packages/cli/run-bun-tests.ts.', + ].join('\n'), + }; + } + + const missing = findUndiscoveredTestFiles(trackedTestFiles, discoveredFiles); + if (missing.length > 0) { + const plural = missing.length === 1 ? '' : 's'; + return { + ok: false, + report: [ + `\ncli-test-discovery guard FAILED: ${missing.length} tracked CLI ` + + `test file${plural} not discovered by run-bun-tests.ts:\n`, + ...missing.map((m) => ` ${m}`), + `\n${FIX_HINT}`, + ].join('\n'), + }; + } + + // Both counts are reported because they answer different questions: + // `tracked` is what the repo contains, `discovered` is what the runner will + // execute. Discovered may legitimately exceed tracked when a developer has + // an uncommitted local test file, which is not a failure. + return { + ok: true, + report: + `cli-test-discovery guard PASSED: all ${trackedTestFiles.length} tracked ` + + `CLI test files are discovered by run-bun-tests.ts ` + + `(discoverTestFiles returned ${discoveredFiles.length}).`, + }; +} + +// ─── Main ─────────────────────────────────────────────────────────────────── + +function main(): void { + const args = parseArgs(process.argv.slice(2)); + + const verdict = evaluateDiscovery( + listTrackedTestFiles(args.root), + discoverTestFiles(args.root), + ); + + if (verdict.ok) { + console.log(verdict.report); + process.exit(EXIT_PASS); + } + console.error(verdict.report); + process.exit(EXIT_FAIL); +} + +if ( + process.argv[1] !== undefined && + resolve(process.argv[1]) === fileURLToPath(import.meta.url) +) { + try { + main(); + } catch (e) { + // Fail-closed: any operational error (git unavailable, not a repo) exits 1 + // with a clear message instead of a raw stack trace, so CI misconfiguration + // is immediately diagnosable. + const msg = e instanceof Error ? e.message : String(e); + console.error(`cli-test-discovery guard FAILED: ${msg}`); + process.exit(EXIT_FAIL); + } +} diff --git a/scripts/tests/check-cli-test-discovery.bun.test.ts b/scripts/tests/check-cli-test-discovery.bun.test.ts new file mode 100644 index 0000000000..eb1a0d9915 --- /dev/null +++ b/scripts/tests/check-cli-test-discovery.bun.test.ts @@ -0,0 +1,468 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Behavioral tests for scripts/check-cli-test-discovery.ts (issue #2923). + * + * Coverage: + * 1. Pure comparison logic (AC1/AC3/AC4) — findUndiscoveredTestFiles and + * findDuplicateDiscoveries classify membership and duplication without git + * or the filesystem. + * 2. End-to-end against the REAL repo (AC2) — the guard must pass today, and + * the direct findUndiscoveredTestFiles(, ) + * assertion is the one that would have caught the original #2923 + * regression. + * 3. AC1 negative, temp git repo — a tracked test file OUTSIDE the runner's + * `TEST_ROOTS` fails the guard and is named in stderr. + * 4. AC3 — duplicate detection via the pure helper. + * 5. AC4 — the guard's pattern is genuinely independent of the runner (it + * classifies `.bun.ts` on its own). + * + * No mock theater: tests invoke the real guard script (pure helpers are + * imported directly) and the real git via a temp repo, per dev-docs/RULES.md. + */ + +import { execFile, execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; + +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { promisify } from 'node:util'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'bun:test'; + +import { + CLI_TEST_FILE_PATTERN, + evaluateDiscovery, + findDuplicateDiscoveries, + findUndiscoveredTestFiles, +} from '../check-cli-test-discovery.ts'; +import { discoverTestFiles } from '../../packages/cli/run-bun-tests.ts'; + +const execFileAsync = promisify(execFile); + +const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = resolve(SCRIPT_DIR, '..', '..'); +const SCRIPT = join(REPO_ROOT, 'scripts', 'check-cli-test-discovery.ts'); +const CLI_ROOT = join(REPO_ROOT, 'packages', 'cli'); +const RUNTIME = process.env.BUN_EXECUTABLE || 'bun'; + +// Paths used by the temp-git-repo negative tests; the expected guard message +// is derived from them so the assertion stays in sync if a path changes. +const ROGUE_TEST_PATH = 'scripts/rogue.test.ts'; +const ROGUE_BUN_PATH = 'scripts/rogue.bun.ts'; + +/** Minimal valid Bun test source used to populate temp-repo fixtures. */ +const STUB_TEST_SOURCE = `import { it } from 'bun:test'; +`; + +/** git argv prefix that sets identity inline, so a clean CI machine can commit. */ +const GIT_IDENTITY = [ + '-c', + 'user.email=test@example.com', + '-c', + 'user.name=Test', +]; + +// Fail fast with a descriptive error if bun is missing, rather than a cryptic +// ENOENT deep in a child-process spawn later in the test run. +try { + execFileSync(RUNTIME, ['--version'], { encoding: 'utf8', stdio: 'pipe' }); +} catch { + throw new Error( + `[cli-test-discovery] Runtime "${RUNTIME}" not found. Set BUN_EXECUTABLE or install bun.`, + ); +} + +interface ScriptResult { + readonly code: number; + readonly stdout: string; + readonly stderr: string; +} + +/** + * Run the guard against an arbitrary root via the real script. Mirrors the + * reference guard's async-run helper (no mock theater). + */ +async function runGuard( + args: readonly string[], + options: { timeout?: number } = {}, +): Promise { + const timeout = options.timeout ?? 30_000; + let stdout = ''; + let stderr = ''; + let exitCode = 0; + try { + const result = await execFileAsync(RUNTIME, [SCRIPT, ...args], { + cwd: REPO_ROOT, + encoding: 'utf8', + timeout, + maxBuffer: 10 * 1024 * 1024, + }); + stdout = result.stdout; + stderr = result.stderr; + } catch (error) { + const err = error as { + code?: number; + signal?: string; + message: string; + stdout?: string; + stderr?: string; + }; + if (err.signal === 'SIGTERM') { + throw new Error( + `Guard timed out after ${timeout / 1000}s: ${err.message}`, + ); + } + stdout = err.stdout ?? ''; + stderr = err.stderr ?? ''; + exitCode = typeof err.code === 'number' ? err.code : 1; + } + return { code: exitCode, stdout, stderr }; +} + +interface TempRepo { + readonly root: string; + /** Write a file under the temp repo, creating parent dirs as needed. */ + write(relPath: string, content: string): void; +} + +/** + * Build a temp git repo with the minimum CLI-workspace shape (a `src/` test + * root and git identity configured for a clean CI machine), then hand it to a + * test. Always removes the temp dir in `finally`. + */ +async function withTempGitRepo( + fn: (repo: TempRepo) => Promise, +): Promise { + const root = mkdtempSync(join(tmpdir(), 'cli-test-discovery-')); + let fnError: unknown; + let result: ScriptResult | undefined; + let cleanupError: unknown; + try { + execFileSync('git', ['init', '-q'], { cwd: root, encoding: 'utf8' }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { + cwd: root, + encoding: 'utf8', + }); + execFileSync('git', ['config', 'user.name', 'Test'], { + cwd: root, + encoding: 'utf8', + }); + + const write = (relPath: string, content: string): void => { + const full = join(root, relPath); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, content); + }; + + // Seed one legitimate test file under a real test root so discovery is + // non-empty and the rogue file is the sole cause of failure. + write('src/legit.test.ts', "import { it } from 'bun:test';\n"); + + result = await fn({ root, write }); + } catch (error) { + fnError = error; + } + try { + rmSync(root, { recursive: true, force: true }); + } catch (error) { + cleanupError = error; + } + if (fnError !== undefined && cleanupError !== undefined) { + const fnMsg = fnError instanceof Error ? fnError.message : String(fnError); + const cleanupMsg = + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError); + throw new AggregateError( + [fnError, cleanupError], + `[cli-test-discovery] fn failed (${fnMsg}) AND temp cleanup failed for ${root}: ${cleanupMsg}`, + ); + } + if (cleanupError !== undefined) { + throw cleanupError; + } + if (fnError !== undefined) { + throw fnError; + } + return result!; +} + +/** Stage and commit all files in a temp repo using inline git identity. */ +function commitAll(root: string, message: string): void { + execFileSync('git', ['add', '.'], { cwd: root, encoding: 'utf8' }); + execFileSync('git', [...GIT_IDENTITY, 'commit', '-q', '-m', message], { + cwd: root, + encoding: 'utf8', + }); +} + +// --------------------------------------------------------------------------- +// Pure comparison logic (AC1 / AC3 / AC4) +// --------------------------------------------------------------------------- + +describe('findUndiscoveredTestFiles', () => { + it('returns tracked files missing from the discovered set, sorted', () => { + const tracked = ['src/a.test.ts', 'test/b.spec.tsx', 'scripts/c.test.ts']; + const discovered = ['src/a.test.ts']; + expect(findUndiscoveredTestFiles(tracked, discovered)).toEqual([ + 'scripts/c.test.ts', + 'test/b.spec.tsx', + ]); + }); + + it('returns an empty array when every tracked file is discovered', () => { + const tracked = ['src/a.test.ts', 'test/b.spec.tsx']; + const discovered = ['test/b.spec.tsx', 'src/a.test.ts']; + expect(findUndiscoveredTestFiles(tracked, discovered)).toEqual([]); + }); + + it('returns an empty array when nothing is tracked', () => { + expect(findUndiscoveredTestFiles([], ['src/a.test.ts'])).toEqual([]); + }); + + it('normalizes backslash paths to POSIX before comparison', () => { + const tracked = ['src\\sub\\a.test.ts']; + const discovered = ['src/sub/a.test.ts']; + expect(findUndiscoveredTestFiles(tracked, discovered)).toEqual([]); + }); + + it('returns results sorted by POSIX path', () => { + const tracked = [ + 'scripts/zzz.test.ts', + 'src/aaa.test.ts', + 'test/mmm.spec.ts', + ]; + expect(findUndiscoveredTestFiles(tracked, [])).toEqual([ + 'scripts/zzz.test.ts', + 'src/aaa.test.ts', + 'test/mmm.spec.ts', + ]); + }); +}); + +describe('findDuplicateDiscoveries', () => { + it('returns paths appearing more than once', () => { + const discovered = ['src/a.test.ts', 'src/a.test.ts', 'test/b.spec.ts']; + expect(findDuplicateDiscoveries(discovered)).toEqual(['src/a.test.ts']); + }); + + it('returns an empty array for a clean, unique set', () => { + const discovered = ['src/a.test.ts', 'test/b.spec.ts']; + expect(findDuplicateDiscoveries(discovered)).toEqual([]); + }); + + it('reports every duplicate, sorted', () => { + const discovered = [ + 'test/b.spec.ts', + 'src/a.test.ts', + 'test/b.spec.ts', + 'src/a.test.ts', + ]; + expect(findDuplicateDiscoveries(discovered)).toEqual([ + 'src/a.test.ts', + 'test/b.spec.ts', + ]); + }); + + it('returns an empty array for an empty input', () => { + expect(findDuplicateDiscoveries([])).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// Real repository (AC2): current state must be fully covered +// --------------------------------------------------------------------------- + +/** Lists tracked CLI test files via the same git oracle the guard uses. */ +function listTrackedCliTests(root: string): string[] { + const out = execFileSync('git', ['ls-files', '-z'], { + cwd: root, + encoding: 'utf8', + }); + return out + .split('\0') + .filter((f) => f.length > 0 && CLI_TEST_FILE_PATTERN.test(f)) + .sort((a, b) => a.localeCompare(b, 'en')); +} + +describe('real repository state (AC2)', () => { + it('the guard PASSES against the real repo', async () => { + const { code, stdout } = await runGuard([]); + expect(code).toBe(0); + expect(stdout).toContain('cli-test-discovery guard PASSED'); + }, 30_000); + + it('every tracked CLI test file is discovered — the #2923 regression guard', () => { + // This is the assertion that would have caught the original #2923 + // regression: the git-tracked set and the runner's discovered set agree. + const tracked = listTrackedCliTests(CLI_ROOT); + const discovered = discoverTestFiles(CLI_ROOT); + expect(findUndiscoveredTestFiles(tracked, discovered)).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// AC1: a tracked test file OUTSIDE the runner's TEST_ROOTS fails the guard +// --------------------------------------------------------------------------- + +describe('rogue test file outside TEST_ROOTS fails (AC1)', () => { + it('FAILS and names the rogue file in stderr', async () => { + const { code, stderr } = await withTempGitRepo(async ({ root, write }) => { + write(ROGUE_TEST_PATH, "import { it } from 'bun:test';\n"); + commitAll(root, 'add rogue'); + return runGuard(['--root', root]); + }); + expect(code).toBe(1); + expect(stderr).toContain('cli-test-discovery guard FAILED'); + expect(stderr).toContain(ROGUE_TEST_PATH); + // The fix message must point at TEST_ROOTS, never an exclude list. + expect(stderr).toContain('TEST_ROOTS'); + }, 30_000); +}); + +// --------------------------------------------------------------------------- +// AC3: duplicate discovery fails the guard +// --------------------------------------------------------------------------- + +describe('duplicate discovery fails (AC3)', () => { + it('a tracked file discovered twice is reported by the pure helper', () => { + const tracked = ['src/a.test.ts']; + const discovered = ['src/a.test.ts', 'src/a.test.ts']; + expect(findDuplicateDiscoveries(discovered)).toEqual(['src/a.test.ts']); + // And the tracked file is still "discovered" (not in the missing set)... + expect(findUndiscoveredTestFiles(tracked, discovered)).toEqual([]); + }); + + it('a clean discovery has no duplicates', () => { + const discovered = ['src/a.test.ts', 'test/b.spec.ts']; + expect(findDuplicateDiscoveries(discovered)).toEqual([]); + }); + + // The guard's decision — not just the helper. `main()` is a thin shell over + // evaluateDiscovery, so these assertions fail if the duplicate branch is ever + // disconnected from the program. A duplicate cannot be produced through the + // real runner (each TEST_ROOTS entry is walked once and directories are + // de-duplicated by real path), so the decision function is the deepest level + // at which this half of the contract can be exercised. + it('the guard verdict REJECTS a duplicated discovery and names the path', () => { + const verdict = evaluateDiscovery( + ['src/a.test.ts'], + ['src/a.test.ts', 'src/a.test.ts'], + ); + expect(verdict.ok).toBe(false); + expect(verdict.report).toContain('cli-test-discovery guard FAILED'); + expect(verdict.report).toContain('more than once'); + expect(verdict.report).toContain('src/a.test.ts'); + }); + + it('the guard verdict REJECTS an undiscovered tracked file with the fix hint', () => { + const verdict = evaluateDiscovery(['scripts/rogue.test.ts'], []); + expect(verdict.ok).toBe(false); + expect(verdict.report).toContain('cli-test-discovery guard FAILED'); + expect(verdict.report).toContain('scripts/rogue.test.ts'); + expect(verdict.report).toContain('TEST_ROOTS'); + }); + + it('the guard verdict ACCEPTS a fully covered, duplicate-free set', () => { + const verdict = evaluateDiscovery( + ['src/a.test.ts'], + ['src/a.test.ts', 'src/local-scratch.test.ts'], + ); + expect(verdict.ok).toBe(true); + expect(verdict.report).toContain('cli-test-discovery guard PASSED'); + // Reports both counts: 1 tracked, 2 discovered. + expect(verdict.report).toContain('all 1 tracked'); + expect(verdict.report).toContain('returned 2'); + }); +}); + +// --------------------------------------------------------------------------- +// Git-oracle boundaries documented in the plan +// --------------------------------------------------------------------------- + +describe('git oracle boundaries', () => { + it('an UNTRACKED test file outside TEST_ROOTS does not fail the guard', async () => { + // A developer's uncommitted scratch file is not part of the repo and CI + // never sees it, so it must not break their local run. + const { code, stdout } = await withTempGitRepo(async ({ root, write }) => { + commitAll(root, 'baseline'); + write(ROGUE_TEST_PATH, STUB_TEST_SOURCE); + return runGuard(['--root', root]); + }); + expect(code).toBe(0); + expect(stdout).toContain('cli-test-discovery guard PASSED'); + }, 30_000); + + it('a TRACKED test file deleted from disk fails closed', async () => { + // Inside a real test root, so being outside TEST_ROOTS cannot be the cause: + // the index still claims the file exists but the runner cannot find it. + // Failing here is correct — the repo and the run disagree. + const deletedPath = 'src/vanished.test.ts'; + const { code, stderr } = await withTempGitRepo(async ({ root, write }) => { + write(deletedPath, STUB_TEST_SOURCE); + commitAll(root, 'add then delete'); + rmSync(join(root, deletedPath)); + return runGuard(['--root', root]); + }); + expect(code).toBe(1); + expect(stderr).toContain(deletedPath); + }, 30_000); + + it('rejects an unknown argument instead of silently checking the real repo', async () => { + const { code, stderr } = await runGuard(['--rot', '/tmp/nowhere']); + expect(code).toBe(1); + expect(stderr).toContain('unknown argument'); + }, 30_000); +}); + +// --------------------------------------------------------------------------- +// AC4: the guard's pattern is genuinely independent of the runner +// --------------------------------------------------------------------------- + +describe('pattern independence (AC4)', () => { + it('the guard classifies a .bun.ts file without deferring to the runner', () => { + // The guard's OWN pattern must recognise every convention the runner does. + expect(CLI_TEST_FILE_PATTERN.test('foo.bun.ts')).toBe(true); + expect(CLI_TEST_FILE_PATTERN.test('foo.bun.tsx')).toBe(true); + expect(CLI_TEST_FILE_PATTERN.test('foo.test.ts')).toBe(true); + expect(CLI_TEST_FILE_PATTERN.test('foo.spec.tsx')).toBe(true); + }); + + it('rejects non-test files', () => { + expect(CLI_TEST_FILE_PATTERN.test('config.ts')).toBe(false); + expect(CLI_TEST_FILE_PATTERN.test('README.md')).toBe(false); + }); + + it('a tracked .bun.ts file outside TEST_ROOTS is reported (no runner help)', async () => { + // The guard nominates this candidate from its own pattern; the runner never + // reaches it because it is outside the roots. So the guard fails. + const { code, stderr } = await withTempGitRepo(async ({ root, write }) => { + write(ROGUE_BUN_PATH, "import { it } from 'bun:test';\n"); + commitAll(root, 'add rogue bun'); + return runGuard(['--root', root]); + }); + expect(code).toBe(1); + expect(stderr).toContain(ROGUE_BUN_PATH); + }, 30_000); + + it('detects drift if the runner stops recognising a convention the guard knows', async () => { + // A `.bun.ts` file INSIDE a real test root must pass today, because the + // runner's TEST_FILE_PATTERN still matches `.bun`. This is the drift + // detector for AC4: if someone narrows the runner's pattern, the guard + // still nominates this file as a candidate (its own pattern is unchanged) + // but the runner no longer discovers it, so this test flips to failing + // instead of both sides silently shrinking together. + const { code, stdout } = await withTempGitRepo(async ({ root, write }) => { + write('src/convention.bun.ts', "import { it } from 'bun:test';\n"); + commitAll(root, 'add bun-suffixed suite inside a test root'); + return runGuard(['--root', root]); + }); + expect(code).toBe(0); + expect(stdout).toContain('cli-test-discovery guard PASSED'); + }, 30_000); +}); From 79d7c0ba6c5162e8f5ca3c725e7a97b5cda2f803 Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 04:39:25 -0300 Subject: [PATCH 2/3] Address review: prove sorting, distinguish maxBuffer overflow from timeout The sorted-output test passed pre-sorted input, so it would still pass if sorting were dropped. It now supplies unsorted input. Node kills a child with SIGTERM for both a timeout and a maxBuffer overflow, so the helper reported runaway output as a timeout and hid the real cause. The overflow is now identified by its error code first, matching the handling in scripts/tests/cli-import-boundary.test.ts. --- .../check-cli-test-discovery.bun.test.ts | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/scripts/tests/check-cli-test-discovery.bun.test.ts b/scripts/tests/check-cli-test-discovery.bun.test.ts index eb1a0d9915..c72a33748d 100644 --- a/scripts/tests/check-cli-test-discovery.bun.test.ts +++ b/scripts/tests/check-cli-test-discovery.bun.test.ts @@ -55,6 +55,12 @@ const RUNTIME = process.env.BUN_EXECUTABLE || 'bun'; const ROGUE_TEST_PATH = 'scripts/rogue.test.ts'; const ROGUE_BUN_PATH = 'scripts/rogue.bun.ts'; +/** + * Output cap for a guard subprocess. Named so the overflow diagnostic and the + * limit it reports cannot drift apart. + */ +const MAX_OUTPUT_BYTES = 10 * 1024 * 1024; + /** Minimal valid Bun test source used to populate temp-repo fixtures. */ const STUB_TEST_SOURCE = `import { it } from 'bun:test'; `; @@ -100,18 +106,28 @@ async function runGuard( cwd: REPO_ROOT, encoding: 'utf8', timeout, - maxBuffer: 10 * 1024 * 1024, + maxBuffer: MAX_OUTPUT_BYTES, }); stdout = result.stdout; stderr = result.stderr; } catch (error) { const err = error as { - code?: number; + code?: number | string; signal?: string; message: string; stdout?: string; stderr?: string; }; + // Node kills the child with SIGTERM for BOTH a timeout and a maxBuffer + // overflow, so the buffer case must be identified by its error code first. + // Otherwise runaway output is misreported as a timeout and the real cause + // is hidden. + if (err.code === 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER') { + throw new Error( + `Guard exceeded the ${MAX_OUTPUT_BYTES / (1024 * 1024)} MB output ` + + `buffer — likely runaway output: ${err.message}`, + ); + } if (err.signal === 'SIGTERM') { throw new Error( `Guard timed out after ${timeout / 1000}s: ${err.message}`, @@ -232,10 +248,12 @@ describe('findUndiscoveredTestFiles', () => { }); it('returns results sorted by POSIX path', () => { + // Input is deliberately NOT in sorted order, so the assertion fails if + // sorting is ever dropped. Passing pre-sorted input would prove nothing. const tracked = [ + 'test/mmm.spec.ts', 'scripts/zzz.test.ts', 'src/aaa.test.ts', - 'test/mmm.spec.ts', ]; expect(findUndiscoveredTestFiles(tracked, [])).toEqual([ 'scripts/zzz.test.ts', From 38321fd8cc8ab06f540753d31b0ece289a52231e Mon Sep 17 00:00:00 2001 From: acoliver Date: Thu, 6 Aug 2026 05:20:31 -0300 Subject: [PATCH 3/3] Report both discovery violations in a single run evaluateDiscovery returned as soon as it found duplicates, so a run that had both duplicates and undiscovered files only reported the duplicates. The reader had to fix one, re-run, and only then learn about the other. Both are now computed up front and every violation present is reported together. --- scripts/check-cli-test-discovery.ts | 57 +++++++++++-------- .../check-cli-test-discovery.bun.test.ts | 14 +++++ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/scripts/check-cli-test-discovery.ts b/scripts/check-cli-test-discovery.ts index 55b8a71893..002621ab34 100644 --- a/scripts/check-cli-test-discovery.ts +++ b/scripts/check-cli-test-discovery.ts @@ -192,6 +192,29 @@ const FIX_HINT = 'parent directory to `TEST_ROOTS`. Never add it to an exclude list — the ' + 'whole point of structural discovery is that nothing is excluded.'; +/** Renders the "discovered more than once" half of a failure report. */ +function describeDuplicates(duplicates: readonly string[]): string { + const plural = duplicates.length === 1 ? '' : 's'; + return [ + `\ncli-test-discovery guard FAILED: discoverTestFiles() returned ` + + `${duplicates.length} path${plural} more than once:\n`, + ...duplicates.map((d) => ` ${d}`), + '\nEach file must be discovered exactly once. Check for overlapping ' + + '`TEST_ROOTS` or nested roots in packages/cli/run-bun-tests.ts.', + ].join('\n'); +} + +/** Renders the "tracked but never discovered" half of a failure report. */ +function describeMissing(missing: readonly string[]): string { + const plural = missing.length === 1 ? '' : 's'; + return [ + `\ncli-test-discovery guard FAILED: ${missing.length} tracked CLI ` + + `test file${plural} not discovered by run-bun-tests.ts:\n`, + ...missing.map((m) => ` ${m}`), + `\n${FIX_HINT}`, + ].join('\n'); +} + // ─── Decision (exported for behavioural tests) ────────────────────────────── export interface DiscoveryVerdict { @@ -215,33 +238,21 @@ export function evaluateDiscovery( trackedTestFiles: readonly string[], discoveredFiles: readonly string[], ): DiscoveryVerdict { + // Both violations are computed before reporting, so a run that has duplicates + // AND missing files shows the whole contract breach at once instead of making + // the reader fix one, re-run, and only then learn about the other. const duplicates = findDuplicateDiscoveries(discoveredFiles); + const missing = findUndiscoveredTestFiles(trackedTestFiles, discoveredFiles); + + const sections: string[] = []; if (duplicates.length > 0) { - const plural = duplicates.length === 1 ? '' : 's'; - return { - ok: false, - report: [ - `\ncli-test-discovery guard FAILED: discoverTestFiles() returned ` + - `${duplicates.length} path${plural} more than once:\n`, - ...duplicates.map((d) => ` ${d}`), - '\nEach file must be discovered exactly once. Check for overlapping ' + - '`TEST_ROOTS` or nested roots in packages/cli/run-bun-tests.ts.', - ].join('\n'), - }; + sections.push(describeDuplicates(duplicates)); } - - const missing = findUndiscoveredTestFiles(trackedTestFiles, discoveredFiles); if (missing.length > 0) { - const plural = missing.length === 1 ? '' : 's'; - return { - ok: false, - report: [ - `\ncli-test-discovery guard FAILED: ${missing.length} tracked CLI ` + - `test file${plural} not discovered by run-bun-tests.ts:\n`, - ...missing.map((m) => ` ${m}`), - `\n${FIX_HINT}`, - ].join('\n'), - }; + sections.push(describeMissing(missing)); + } + if (sections.length > 0) { + return { ok: false, report: sections.join('\n') }; } // Both counts are reported because they answer different questions: diff --git a/scripts/tests/check-cli-test-discovery.bun.test.ts b/scripts/tests/check-cli-test-discovery.bun.test.ts index c72a33748d..8d59b51519 100644 --- a/scripts/tests/check-cli-test-discovery.bun.test.ts +++ b/scripts/tests/check-cli-test-discovery.bun.test.ts @@ -386,6 +386,20 @@ describe('duplicate discovery fails (AC3)', () => { expect(verdict.report).toContain('TEST_ROOTS'); }); + it('reports duplicates AND missing files together in one run', () => { + // Both halves of the contract can be broken at once; the reader should not + // have to fix one, re-run, and only then be told about the other. + const verdict = evaluateDiscovery( + ['src/a.test.ts', 'scripts/rogue.test.ts'], + ['src/a.test.ts', 'src/a.test.ts'], + ); + expect(verdict.ok).toBe(false); + expect(verdict.report).toContain('more than once'); + expect(verdict.report).toContain('src/a.test.ts'); + expect(verdict.report).toContain('not discovered by run-bun-tests.ts'); + expect(verdict.report).toContain('scripts/rogue.test.ts'); + }); + it('the guard verdict ACCEPTS a fully covered, duplicate-free set', () => { const verdict = evaluateDiscovery( ['src/a.test.ts'],