ci: require zero orphan tests without a baseline - #4271
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Approved at 317425394842aee31c4f8b2b1b9fcca628d18888.
A gate that reports "no violations" looks identical whether the rule landed or the tool stopped looking, so I injected both violations rather than reading the predicate. Every exit code below comes from a direct run with output redirected to a file — never read through a pipe, where $? is the last stage's status.
The gate is earned, not decorative
| injected into the head tree | exit | stderr |
|---|---|---|
| nothing (clean head) | 0 | — (Test reachability verified: zero orphan tests.) |
scripts/test-reachability-baseline.json restored, untracked |
1 | Test reachability baseline is forbidden; every test must be reachable. |
same, git added |
1 | same |
rames-probe/orphan.test.mjs planted + tracked |
1 | rames-probe/orphan.test.mjs: no CI runner selects this test |
| both at once | 1 | both lines |
| everything reverted | 0 | back to the clean line |
scripts/check-test-reachability.test.mjs is 27/27 green. Run from a subdirectory the script dies loudly on the unreadable manifest rather than passing vacuously, so a wrong cwd cannot produce a silent all-clear.
Body claims I checked against source
- "Stacked on #4260" — true, and literally: this branch's direct parent is
cff467a35, #4260's squash merge. - "The deletion is named in the existing deletion guard" — true,
scripts/check-no-main-deletions.mjs:43-46; the 66-character reason clears thereason.length > 10assertion in that guard's own test. - The deletion is complete.
ratchet,baselineIssues,previousBaselineandbaseArgumenthave zero remaining references anywhere in the repo — thepreviousBaseline/ratchethits inscripts/contrastRatchet.test.tsand underpackages/studiobelong to the unrelated contrast and hex ratchets.execFileSynccorrectly leaves the import list with no remaining use in the file, and--basesurvives only incheck-no-main-deletions.mjs,check-pr-captures.mjsand thefallowstep, none of which this touches. - The repo-wide zero survives the merge.
mainhas moved to867787b2f(#4268) since the branch point and added no new test files, only edits to existing ones — so nothing has landed that this gate would reject on the merge commit.
1. The body describes the first commit, not the head
317425394 ("fix: keep reachability reports diagnostic-only") reversed the body's central claim, and the body was not updated:
- "The report command now shares the same failure decision as the required check" — at head
main()returns atcheck-test-reachability.mjs:416-419, beforeverdict()on line 420.--reportapplies no verdict at all. - "asserting a nonzero exit for each violation" — the witness asserts
status === 0atcheck-test-reachability.test.mjs:248,:252and:258, and the test is named "report CLI prints diagnostics without applying the gate verdict".
The head behaviour is the better of the two: the required check is the third CI step, and a --report that failed would abort the job at the diagnostic step before the test step ever ran. So this is the description to fix, not the code. Same paragraph says "before this draft is ready" — this isn't a draft.
2. The exit code this PR exists to produce has no witness in the suite
verdict() is unit-tested for the strings it returns (:123-128), and the only CLI witness runs --report, which by design cannot fail. Nothing in the suite ever spawns the script without that flag. So the single contract that ships here — node scripts/check-test-reachability.mjs exiting non-zero — is the one thing the tests do not defend: move the --report early return below the verdict, or drop process.exitCode = 1, and all 27 tests still pass while CI goes quiet.
Not a defect at this head — I measured it firing above. But the harness for closing it is already sitting in that test, about four lines:
const gate = () => spawnSync(process.execPath, [script], { cwd, encoding: "utf8" });
assert.equal(gate().status, 0); // clean fixture
writeFileSync(baseline, '{"total":0,"files":{}}');
assert.equal(gate().status, 1);
assert.match(gate().stderr, /baseline is forbidden/);Neither finding is a correctness problem, so this is an approval. Note require_last_push_approval is on for main, so a further push voids this review; strict_required_status_checks_policy is off, so the branch being behind main does not force an update push.
— Rames
Every test must be reachable through CI for the files it protects. This removes the empty reachability baseline and rejects its presence, so a new orphan cannot be accepted as debt.
The report command now shares the same failure decision as the required check. A CLI witness starts from a reachable fixture, restores an empty baseline and then plants an orphan, asserting a nonzero exit for each violation.
Stacked on #4260. The predecessor wires the remaining tests; this change makes zero permanent. The deletion is named in the existing deletion guard.
Validation: source review complete; local execution is unavailable. CI must supply the before/after report and focused test output before this draft is ready. Expected report: zero findings before and after, with baseline presence forbidden afterward. No measured CI result is claimed yet.
No visible change
This CI-only checker and test update changes no rendered product behavior.
This PR supersedes closed #4261 after its stacked base branch merged.