Skip to content

A merge that deletes another PR's test is invisible to CI — add a deleted-test guard #1976

Description

@macanderson

Problem

Three separate times now, a PR has landed on main that silently deleted code
another PR added to the same file earlier the same day, and CI could not see
it. The most recent (fixed in #1975): #1951 rewrote
crates/stella-pipeline/src/pipeline/tests/verification_hardening.rs from a
pre-#1945 base, deleting the PassingShell double, shell_call_result, and
the witness a_revision_halts_at_the_step_where_the_tracked_test_flips that
#1945 had added hours earlier.

main went red. It was caught only because #1945 had also added a
mod flip_halt_arming; line that survived the rewrite and referenced two of the
deleted symbols. Had #1945 added only the test and the 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.

This is the same shape recorded for #1860 silently reverting #1836's forwarding
in four crates.

Why CI cannot see it

Both PRs are green against the main they branched from. The merge is textually
clean — git has no conflict to report, because one side simply does not contain
the other's lines. Branch protection's "require branches to be up to date"
would catch it, but it is off (and turning it on serializes every merge, which
is its own cost at this repo's merge rate).

Fix direction

The cheap, high-value version — a deleted-test guard, in the spirit of
check-left-behind.sh:

For a PR's merge base against main, enumerate #[test] / #[tokio::test]
function names present in the base and absent in the head. A non-empty set is
not automatically wrong — tests are legitimately renamed, moved between
modules, or deliberately removed — so the guard should fail asking for an
acknowledgement
rather than fail outright: the PR body (or a commit trailer)
must name each removed test. That converts an invisible deletion into a
sentence a reviewer reads.

Cheaper still, and worth doing regardless: this only needs to run in CI on the
merge result, not in make gate locally, since it is inherently a
two-branch question.

Deliberately NOT proposed: requiring up-to-date branches, or forbidding test
file rewrites. Both cost more than the defect.

Files

  • scripts/ — new guard, modelled on scripts/check-left-behind.sh (the
    closest existing shape: a mechanical check whose output is "name the thing or
    fix it")
  • MakefileGATE_STEPS, if it is to run locally too (note: adding a step
    there is five edits, per the gate-parity guard — Makefile, AGENTS.md,
    CONTRIBUTING.md, and both spelled-out lists)
  • .github/workflows/ci.yml — the merge-result job

Verify

Reconstruct the real case: branch A adds a test to
verification_hardening.rs; branch B rewrites the same file from before A;
merge both. The guard must name the deleted test. Confirm a genuine rename
(same body, new name) is also reported, and that acknowledging it in the PR
body passes.

Definition of done

  • A test deleted by a merge cannot reach main without a human having named it
    in the PR body.
  • The guard is quiet on ordinary PRs (no false positives on a branch that adds
    tests only).
  • Documented wherever the gate is described, if it joins GATE_STEPS.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:ciGitHub Actions, release pipeline, packagingtriageUntyped request — convert by adding bug / feature / epic

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions