ci: fail a PR whose merge silently deletes a test, unless it names it (#1976) - #2036
Merged
Conversation
Three times a PR has landed on `main` that deleted code another PR added to the same file hours earlier, invisibly. The most recent: #1951 rewrote crates/stella-pipeline/src/pipeline/tests/verification_hardening.rs from a pre-#1945 base, dropping the witness `a_revision_halts_at_the_step_where_the_tracked_test_flips` that #1945 had added the same day. Both PRs were green against the `main` they branched from, and the merge was textually clean — git had no conflict to report, because one side simply did not contain the other's lines. It was caught only by luck: #1945 had also added a `mod flip_halt_arming;` line that survived the rewrite and referenced two deleted symbols, so `main` went red. Had #1945 added only the test and its doubles, the deletion would have compiled clean. A witness that no longer exists cannot fail, so nothing downstream would ever have reported it. `scripts/check-deleted-tests.sh` compares the base branch tip against the merge result and names any `#[test]`/`#[tokio::test]` that did not survive. That pair is the design: comparing against the PR's branch point would miss exactly this defect, since a test added to `main` after the branch point is absent there too. Comparing against the merged tree asks "did everything main had survive?", and is quiet on a merely stale branch because git merges main's own additions in unless the PR's side removed them. It asks for an acknowledgement rather than forbidding deletion: renames and deliberate removals are ordinary, so a removed test passes once the PR description names it. The goal is not to adjudicate whether a deletion was correct — a script cannot — but to turn an invisible deletion into a sentence a reviewer reads. Deliberately NOT a GATE_STEPS entry: it is the one question here about two trees, and a local `make gate` has no second tree to compare. It runs on `pull_request` only, needs `fetch-depth: 2` (two trees, not two histories), and keys on the bare test name so a test moved between modules stays quiet. The cost of the unqualified key is measured in the script header rather than assumed: 51 of 6867 distinct test names (0.74%) are duplicated today, nearly all per-adapter provider suites. Witness, reproduced from the real history rather than a fixture: $ ./scripts/check-deleted-tests.sh eddf970 2a142b2 # 1945 -> 1951 check-deleted-tests: FAILED a_revision_halts_at_the_step_where_the_tracked_test_flips and with the name in PR_BODY it passes, as does a range that only adds tests. Closes #1976
A `#[test]` at the start of a Rust line-continuation inside a string literal is counted as a test. Two lines in the tree do it — witness/density.rs and candidate_ws/witness_tools.rs, both code that analyses test code. The guard is a difference detector, so the noise is symmetric and cancels; it can only speak up if such a fixture is edited, which lands in the acknowledge path. Documented rather than parsed around, for the reason check-left-behind.sh gives about the same shape: a shell script cannot reliably tell whether a token is inside a string, and one that guesses wrong cries wolf.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
Reviewer's GuideAdds a CI guard that compares the base branch tip to the PR merge result to detect tests deleted by the merge unless explicitly acknowledged, wires it into the GitHub Actions workflow, documents its behavior and limitations in AGENTS.md, and implements the guard as a shell script that scans Rust test functions between two git trees. Sequence diagram for the new deleted-test CI guard on pull_requestsequenceDiagram
participant github_actions
participant checkout
participant check_deleted_tests_sh
github_actions->>checkout: actions/checkout
note right of checkout: fetch-depth: 2
github_actions->>check_deleted_tests_sh: run ./scripts/check-deleted-tests.sh
activate check_deleted_tests_sh
check_deleted_tests_sh-->>check_deleted_tests_sh: read PR_BODY env
check_deleted_tests_sh-->>check_deleted_tests_sh: compare base_branch_tip tree vs merge_result tree
alt [no tests removed]
check_deleted_tests_sh-->>github_actions: exit 0 (OK — none lost by the merge)
else [tests removed and all names mentioned in PR_BODY]
check_deleted_tests_sh-->>github_actions: exit 0 (OK — removed test(s) acknowledged)
else [tests removed and at least one name not mentioned]
check_deleted_tests_sh-->>github_actions: exit 1 (FAILED — unnamed removed test(s))
end
deactivate check_deleted_tests_sh
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Three times a PR has landed on
mainthat silently deleted code another PRadded to the same file hours earlier, and CI could not see it.
The most recent (fixed in #1975): #1951 rewrote
crates/stella-pipeline/src/pipeline/tests/verification_hardening.rsfrom apre-#1945 base, deleting the
PassingShelldouble,shell_call_result, and thewitness
a_revision_halts_at_the_step_where_the_tracked_test_flipsthat #1945had added hours earlier.
It was caught only by luck: #1945 had also added a
mod flip_halt_arming;line that survived the rewrite and referenced two of the deleted symbols, so
mainwent red. Had #1945 added only the test and its doubles — no new module —the deletion would have compiled clean and silently removed a witness from the
tree. A witness that no longer exists cannot fail, so nothing downstream would
ever have reported it. Same shape on record for #1860 reverting #1836's
forwarding in four crates.
Both PRs are green against the
mainthey branched from, and the merge istextually clean: git has no conflict to report, because one side simply does not
contain the other's lines.
The design decision that matters
scripts/check-deleted-tests.shcompares the base branch tip against themerge result — never the PR's branch point.
That pair is the whole guard. Comparing against the branch point would miss
exactly this defect: a test added to
mainafter the PR branched is absentfrom the branch point too, so its disappearance would look like nothing at all.
Comparing main's tip against the merged tree asks the question that matters —
"did everything main had survive this merge?" — and is quiet on a merely stale
branch, because git merges main's own additions in unless the PR's side actively
removed them.
On a
pull_requestevent the checkout isrefs/pull/N/merge, soHEAD^1isthe base branch tip. The guard needs no PR metadata, and because it compares two
trees rather than two histories,
fetch-depth: 2is sufficient — nofull-depth clone.
It asks for an acknowledgement, it does not forbid deletion
A removed test is not automatically wrong — renames, folding into a table-driven
case, and deliberate removal with the feature covered are all ordinary. So a
removal fails only while unnamed: writing the test's name in the PR
description (or a commit message) passes it.
That mechanism is deliberately weak. The goal is not to adjudicate whether a
deletion was correct — a script cannot — but to convert an invisible deletion
into a sentence a reviewer reads.
Deliberately NOT a
GATE_STEPSentryIt is the one question here about two trees, and a local
make gatehas nosecond tree to compare, so there is nothing for it to do there. This also means
the five-edit gate-parity dance does not apply;
check-gate-paritystill reports25 steps, unchanged.
Closes #1976
The witness
main, passes here), orThe witness is reproduced from real history, not a synthetic fixture —
eddf9700is #1945 (added the witness),2a142b26is #1951 (deleted it):It names exactly the test that was really lost. The other three required
behaviours, all verified:
A genuine rename is reported too (the old name is gone), which is intended —
naming it in the PR is the whole cost.
The gate
shellcheck— cleanmake guards-fast— all green, includingcheck-gate-parity(25 steps,unchanged) and
check-action-pinsdescription now lists the guard among what
ci.ymladds beyond the gateCloses #1976appears both here and as a commit trailerNo Rust changed, so fmt/clippy/test are untouched by this PR.
Nothing left behind
Two limitations are measured and documented in the script header rather than
left implicit:
a test moved between modules silently fine (the common legitimate case),
at the cost that a duplicated name masks a deletion. Measured: 51 of 6867
distinct test names (0.74%) are duplicated today, nearly all the per-adapter
provider suites where one contract is asserted against each vendor under one
name. The house style of long sentence-shaped test names is what makes the
unqualified key work — it is not a general assumption.
#[test]inside a multi-line string fixture is counted. Two lines in thetree (
witness/density.rs,candidate_ws/witness_tools.rs— code thatanalyses test code) put
#[test]at the start of a continuation line insidea string literal. It is symmetric noise, so a difference detector cancels it;
it could only false-positive if such a fixture were edited, which lands in
the acknowledge path by design.
Ground-rule check
stella-core; no new depsAnything reviewers should know?
fetch-depth: 2on thecheckjob's checkout is the one change that touchesevery CI run. It is one extra commit, not a full clone — deliberately the
smallest thing that makes the merge commit's first parent readable.
The guard runs on
pull_requestonly: on a squash-merged push tomainthere isno merge commit to inspect, and the report would arrive too late to act on
anyway. The script self-skips on a non-merge
HEADwith no explicit base, sorunning it by hand needs a base ref:
./scripts/check-deleted-tests.sh origin/main.Summary by Sourcery
Add a CI guard that detects tests removed by a PR’s merge unless their deletion is explicitly acknowledged, and document this behaviour in agent guidance.
CI:
Documentation:
Chores: