Guard against silently unrun CLI test files (Fixes #2923)

[acoliver]

TLDR

Issue #2923 was filed against a Vitest exclude contract that no longer exists. PR #3056 (Fixes #2843) deleted packages/cli/vitest.config.ts, vitest.test-groups.ts, baseExclude and SELECTED_FILE_COUNT, replacing them with structural discovery in packages/cli/run-bun-tests.ts. Measured on main: all 670 tracked CLI test files are discovered and run — zero excluded, and the issue's own repro file passes.

So resolutions 1 and 2 of the issue are already satisfied. What was still missing is resolution 3: a guard that makes a future silent exclusion fail loudly. That is what this PR adds.

The hole being closed: run-bun-tests.ts walks a hardcoded TEST_ROOTS = ['src', 'test', 'test-bun', 'test-utils']. A tracked test file added anywhere else under packages/cli — scripts/, bin/, the workspace root, a brand-new directory — would never run, and every existing test would still pass. packages/cli/scripts/ and packages/cli/bin/ already exist, so this is reachable, not hypothetical.

Reviewers should look closely at: the deliberate duplication of the test-file regex (justified below), and evaluateDiscovery(), which holds the guard's entire decision so both halves of the contract are covered by tests of real decision-making.

Dive Deeper

Measured ground truth on main (42ca2a9)

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 with unconditional describe.skip/it.skip/test.skip	0
Tracked test files in runner-skipped 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 measured is the runner CI uses.

What the guard does

scripts/check-cli-test-discovery.ts compares two independently-derived sets and fails when they disagree:

• Candidates come from git ls-files in packages/cli, classified by the guard's own pattern constant.
• Discovered comes from the real discoverTestFiles() imported from run-bun-tests.ts — not a copy.

It fails on either half of the contract: a tracked test file that is not discovered, or a path discovered more than once.

Why the regex is deliberately duplicated

Importing the runner's TEST_FILE_PATTERN would let both sides of the comparison shrink together. If someone narrowed the runner to drop .bun, the candidate set and the discovered set would both lose those files and the guard would pass while eleven suites stopped running. Duplicating keeps the sides independent. The DRY cost is one small regex; the test at pattern independence (AC4) > detects drift... is the lock that flips to failing if the runner narrows.

Design note

evaluateDiscovery() returns a verdict plus the exact text to print, and main() is a thin shell over it (gather inputs, print, exit). This exists so the "exactly once" half is covered by a test of the program's real decision rather than of a helper the program might not consult. 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 that half can be exercised.

Deliberately out of scope

• No change to TEST_ROOTS or any discovery behaviour. The guard makes the gap loud; widening discovery is a separate decision.
• No test file deleted. Nothing is excluded, so issue resolution 2 is vacuous.
• No file-count oracle reintroduced. 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.
• One deferred finding, reported not fixed: packages/cli/test/ui/commands/authCommand-logout.test.ts gates all four suites on process.env.CI === 'true'. Measured: 0 pass / 21 skip under CI=true, 21 pass under CI=false. It is discovered and invoked — the runner and this guard both do their job — but it asserts nothing on CI. That is an explicit, greppable skip rather than the silent structural exclusion this issue is about, and unpicking an OAuth logout suite is a different subsystem. Documented in the plan; worth its own issue.

Review findings triage

• Fixed (blocker): the duplicate half was originally asserted only against a helper, so disconnecting it from the program would have left every test green. Hence evaluateDiscovery().
• Fixed: unknown CLI arguments were silently ignored, so a typo like --rot /tmp/x would run against the real repo and report a misleading PASS. Now fails fast.
• Fixed: the sorted-output test passed pre-sorted input and would still have passed if sorting were dropped. Now uses unsorted input.
• Fixed: Node kills a child with SIGTERM for both a timeout and a maxBuffer overflow, so runaway output was misreported as a timeout. The overflow is now identified by its error code first, matching scripts/tests/cli-import-boundary.test.ts.
• Fixed: the plan claimed zero skips; corrected to zero unconditional skips, with the conditional-skip audit documented.
• Rejected: a suggestion to switch the main-module check to import.meta.main. Verified against the codebase: 8 sibling scripts/check-*.ts guards use the process.argv[1] comparison and zero use import.meta.main. Adopting it would make this guard the odd one out.

Reviewer Test Plan

Confirm the guard passes today:

npm run lint:cli-test-discovery
# cli-test-discovery guard PASSED: all 670 tracked CLI test files are discovered ...

Confirm it actually catches a silently-unrun file — plant one where the runner cannot reach it:

mkdir -p packages/cli/scripts
printf "import { it, expect } from 'bun:test';\nit('would never run', () => { expect(1).toBe(1); });\n" > packages/cli/scripts/rogue.test.ts
git add packages/cli/scripts/rogue.test.ts
npm run lint:cli-test-discovery ; echo "exit=$?"

Expected: exit 1, naming scripts/rogue.test.ts and pointing at TEST_ROOTS — never at an exclude list. Then clean up:

git rm --cached -q packages/cli/scripts/rogue.test.ts
rm -rf packages/cli/scripts

Run the guard's own suite:

bun test --preload ./test-setup/augment-bun-vi.ts scripts/tests/check-cli-test-discovery.bun.test.ts

Verify the issue's original claim is resolved:

cd packages/cli && bun test ./src/ui/components/ModelConfigDialog.test.tsx

Testing Matrix

	🍏	🪟	🐧
npm run	✅	❓	❓
npx	❓	❓	❓
Docker	❓	❓	❓
Podman	❓	-	-
Seatbelt	❓	-	-

Verified locally on macOS: npm run lint (exit 0), npm run typecheck (0 errors), npm run build (exit 0), npm run test:scripts (PASSED), npm run lint:eslint-guard (passed), prettier clean, and the CLI smoke test via bun scripts/start.ts --profile-load stepfun-37. The guard's own suite: 24 pass / 0 fail. Paths are normalised to POSIX so behaviour is identical on Windows; the CI lint job itself runs on Ubuntu.

Linked issues / bugs

Fixes #2923

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 60d81ad5-16e9-470b-b0a8-aac934b1ecdb

📥 Commits

Reviewing files that changed from the base of the PR and between 42ca2a9 and 38321fd.

⛔ Files ignored due to path filters (1)

• project-plans/issue2923/plan.md is excluded by !project-plans/**

📒 Files selected for processing (4)

• .github/workflows/ci.yml
• package.json
• scripts/check-cli-test-discovery.ts
• scripts/tests/check-cli-test-discovery.bun.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Walkthrough

This PR changes 5 file(s).

• project-plans/issue2923/plan.md: (per-file summary unavailable)
• package.json: (per-file summary unavailable)
• scripts/tests/check-cli-test-discovery.bun.test.ts: (per-file summary unavailable)
• scripts/check-cli-test-discovery.ts: (per-file summary unavailable)
• .github/workflows/ci.yml: (per-file summary unavailable)

Changes

Layer	File(s)	Summary
project-plans/issue2923	project-plans/issue2923/plan.md	Changes in project-plans/issue2923
.	package.json	Changes in .
scripts/tests	scripts/tests/check-cli-test-discovery.bun.test.ts	Changes in scripts/tests
scripts	scripts/check-cli-test-discovery.ts	Changes in scripts
.github/workflows	.github/workflows/ci.yml	Changes in .github/workflows

Magnitude

🎯 2 (M)
942 additions, 0 deletions, 5 changed files across 0 packages, 8 acceptance criteria

Related

No related items found.

---

Walkthrough generated by LLxprt PR Review. Planner issue: #2256

[github-actions]

OpenCodeReview — PR #3099

• Reviewed head SHA: 38321fd8cc8ab06f540753d31b0ece289a52231e
• Merge base: 42ca2a9898600c0cc558c4c932d3c0cd1aa96379
• Range: incremental from 79d7c0ba6c5162e8f5ca3c725e7a97b5cda2f803
• Range fallback: none
• Scope: selected 2 file(s), +48/-23; cumulative 5 file(s), +942/-0
• Tokens: 52178 total (41111 input, 11067 output, 22656 cache)
• OCR version: open-code-review v1.8.4 (e78474478) linux/amd64 built at: 2026-08-01T03:27:37Z https://github.com/alibaba/open-code-review
• Phase: review
• Exit code: 0
• Run: https://github.com/vybestack/llxprt-code/actions/runs/31084508111
• No findings.
• Artifacts: ocr-review-output contains raw JSON, stdout, stderr, preview, phase, and exit-code diagnostics.
• WARNING: Changed-file coverage 0/2 preview files covered is below the 90% threshold.

[github-actions]

Code Coverage Summary

Package	Lines	Statements	Functions	Branches
CLI	N/A%	N/A%	N/A%	N/A%
Core	N/A%	N/A%	N/A%	N/A%

CLI Package - Full Text Report

CLI full-text-summary.txt not found at: coverage_cli/packages/cli/coverage/full-text-summary.txt

Core Package - Full Text Report

Core full-text-summary.txt not found at: coverage_core/packages/core/coverage/full-text-summary.txt

For detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run.