Skip to content

ci: require zero orphan tests without a baseline - #4271

Merged
miguel-heygen merged 2 commits into
mainfrom
hfoss39/orphans-zero
Sep 22, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
hfoss39/orphans-zero

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

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.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the reason.length > 10 assertion in that guard's own test.
  • The deletion is complete. ratchet, baselineIssues, previousBaseline and baseArgument have zero remaining references anywhere in the repo — the previousBaseline/ratchet hits in scripts/contrastRatchet.test.ts and under packages/studio belong to the unrelated contrast and hex ratchets. execFileSync correctly leaves the import list with no remaining use in the file, and --base survives only in check-no-main-deletions.mjs, check-pr-captures.mjs and the fallow step, none of which this touches.
  • The repo-wide zero survives the merge. main has moved to 867787b2f (#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 at check-test-reachability.mjs:416-419, before verdict() on line 420. --report applies no verdict at all.
  • "asserting a nonzero exit for each violation" — the witness asserts status === 0 at check-test-reachability.test.mjs:248, :252 and :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

@miguel-heygen
miguel-heygen merged commit c029bf4 into main Sep 22, 2026
53 checks passed
@miguel-heygen
miguel-heygen deleted the hfoss39/orphans-zero branch September 22, 2026 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants