ci: fail a PR whose merge silently deletes a test, unless it names it (#1976)

.github/workflows/ci.yml

@@ -156,6 +156,14 @@ jobs:
     steps:
       - id: checkout
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
+        with:
+          # Two, not one: on a `pull_request` event this checks out
+          # `refs/pull/N/merge`, and the deleted-test guard below reads that
+          # merge commit's first parent (the base branch tip) to ask what the
+          # merge dropped. It compares two TREES, not two histories, so one
+          # extra commit is the whole cost — a full-depth clone would buy
+          # nothing and slow every run.
+          fetch-depth: 2
 
       # Cheap and toolchain-free, so it runs before the Rust setup: a tracked
       # file that matches .gitignore means agent scratch reached the remote
@@ -284,6 +292,22 @@ jobs:
         if: ${{ !cancelled() && steps.checkout.outcome == 'success' }}
         run: ./scripts/check-left-behind.sh
 
+      # Also toolchain-free, and the only gate here that is a question about
+      # TWO trees rather than one — which is why it has no `make gate` step to
+      # match. Three times a PR has landed that silently deleted a test another
+      # PR added to the same file hours earlier: both sides green, the merge
+      # textually clean, and a witness quietly gone from the tree (#1976). It
+      # compares the base branch tip against the merge result, so it is quiet
+      # on a merely stale branch, and it asks for an acknowledgement rather
+      # than forbidding deletion — naming the test in this PR's description
+      # passes it. `pull_request` only: on a squash-merged push to `main` there
+      # is no merge to inspect and the report would arrive too late anyway.
+      - name: no test disappears in the merge unacknowledged
+        if: ${{ !cancelled() && steps.checkout.outcome == 'success' && github.event_name == 'pull_request' }}
+        env:
+          PR_BODY: ${{ github.event.pull_request.body }}
+        run: ./scripts/check-deleted-tests.sh
+
       # Also toolchain-free. AGENTS.md and CONTRIBUTING.md each restate what
       # `make gate` runs, and both had drifted twice — each time by omitting a
       # newly added guard, which is the direction that misleads a reader into

AGENTS.md

@@ -88,7 +88,8 @@ leaving `main` red for everyone (#1883).
 CI enforces the same steps split across three workflows:
 `/.github/workflows/ci.yml`'s required job runs everything except `invariants`
 and `doc-links`, and adds a `Cargo.lock` sync check, the prompt-cache golden
-fixtures, `stella context validate`, and a release smoke build (thin LTO);
+fixtures, `stella context validate`, a release smoke build (thin LTO), and the
+deleted-test guard (`scripts/check-deleted-tests.sh`);
 `docs-guards.yml` runs those two plus a second run of `command-docs`, because
 all three trigger on the `docs/**` and `*.md` paths `ci.yml` ignores; and
 `wire-schema.yml` runs `wire-schema` on `docs/wire/**` and the protocol crates,
@@ -316,6 +317,18 @@ project's test tree. `stella run --keep-witness` promotes it instead. See
 `website/content/docs/inference-pipeline.mdx` for the full stage flow, the distress-triggered guidance
 loop, and the `/pipeline` deck toggle.
 
+**A witness that no longer exists cannot fail.** Three times a PR has landed
+that silently deleted a test another PR added to the same file hours earlier —
+both branches green, the merge textually clean, git with no conflict to report
+because one side simply does not contain the other's lines (#1976, and the same
+shape in #1860). `check-deleted-tests` runs in CI on `pull_request` only,
+because it is the one question here about *two* trees: it compares the base
+branch tip against the merge result and names any `#[test]`/`#[tokio::test]`
+that did not survive. Deleting a test is not forbidden — renames and deliberate
+removals are ordinary — it just has to be **named in the PR description**, which
+turns an invisible deletion into a sentence a reviewer reads. It is deliberately
+not a `make gate` step: locally there is no second tree to compare.
+
 ---
 
 ## Nothing left behind — every finding becomes a fix or a GitHub issue

scripts/check-deleted-tests.sh

@@ -0,0 +1,233 @@
+#!/usr/bin/env bash
+#
+# Guard: a merge that deletes a test must say so out loud.
+# See #1976, and AGENTS.md § "The definition of done: witness tests".
+#
+# Three times now a PR has landed on `main` that silently deleted code another
+# PR added to the same file earlier the same day. The most recent (#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.
+#
+# 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
+# `main` went 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. The same shape is on record
+# for #1860 silently reverting #1836's forwarding in four crates.
+#
+# ── Why CI cannot already see this ───────────────────────────────────────────
+#
+# Both PRs are green against the `main` they branched from, and 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 at this repository's merge rate costs more than the defect.
+#
+# ── What it compares, and why that exact pair ────────────────────────────────
+#
+# The merge BASE against the merge RESULT — never the PR's branch point.
+#
+# That distinction is the whole guard. Comparing against the branch point would
+# miss precisely the defect this exists for: a test added to `main` *after* the
+# PR branched is absent from the branch point, so its disappearance from the
+# PR would look like nothing at all. Comparing `main`'s tip against the merged
+# tree asks the question that matters — "does everything main had survive this
+# merge?" — and is quiet on a stale branch, because git merges main's own
+# additions in unless the PR's side actively removed them.
+#
+# On a `pull_request` event the checkout is `refs/pull/N/merge`, so HEAD is the
+# merge commit, HEAD^1 is the base branch tip and HEAD^2 is the PR head. The
+# guard reads HEAD^1 vs HEAD and needs no PR metadata. It is a two-TREE
+# question, not a two-history one, so `fetch-depth: 2` is enough — there is no
+# need for a full clone.
+#
+# ── It asks for an acknowledgement; it does not forbid deletion ──────────────
+#
+# A removed test is not automatically wrong. Tests are legitimately renamed,
+# merged into a table-driven case, or deliberately dropped with the feature
+# they covered. So a removal fails the guard only while it is UNNAMED: writing
+# the test's name in the PR description (or in a commit message) passes it.
+#
+# That is the entire mechanism, and it 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.
+#
+# ── What it keys on, and the miss that follows ───────────────────────────────
+#
+# The bare function name of anything carrying `#[test]` or `#[tokio::test]`,
+# unqualified by file or module. Unqualified is a choice: it means a test MOVED
+# between files or modules is silently fine, which is the common, legitimate
+# case and would otherwise be constant noise.
+#
+# The cost is that a duplicated name masks a deletion — delete one `fn works()`
+# while another survives elsewhere and this guard sees the name still present.
+# Measured rather than assumed: at the time of writing the tree carries 6927
+# test attributes over 6867 distinct names, of which 51 (0.74%) are used more
+# than once. Nearly all of those are the per-adapter provider suites, where the
+# same contract is deliberately asserted against each vendor under one name
+# (`complete_maps_401_to_auth_error` and its siblings appear once per adapter).
+#
+# So the masked case is real but narrow, and it is narrow *because* of this
+# repository's house style of long sentence-shaped test names — it is not a
+# general-purpose assumption, and a tree full of `fn test_1` would need the
+# qualified key and the move-detection cost that comes with it.
+#
+# The second known miss is the mirror of the one documented in
+# scripts/check-left-behind.sh: a `#[test]` inside a STRING LITERAL is counted.
+# Two lines in the tree manage it (crates/stella-pipeline/src/witness/density.rs
+# and crates/stella-cli/src/candidate_ws/witness_tools.rs — both code that
+# analyses test code, whose fixtures put `#[test]` at the start of a Rust
+# line-continuation inside a literal). A shell script cannot reliably tell
+# whether a `#[test]` is inside a string, and this one does not try, for the
+# reason that script gives: a guard that guesses wrong cries wolf.
+#
+# It costs nothing here, because this is a DIFFERENCE detector and the noise is
+# symmetric — a fixture present in both trees cancels. It can only speak up if
+# such a fixture is edited, and that lands in the acknowledge path, which is the
+# benign direction.
+#
+# ── Deliberately NOT in `make gate` ──────────────────────────────────────────
+#
+# This is inherently a two-branch question, so there is nothing for it to
+# compare on a local `make gate` run — it lives in CI, on the merge result.
+# Running it by hand takes an explicit base:
+#
+#     ./scripts/check-deleted-tests.sh origin/main
+#
+# Uses portable POSIX tools so it runs on a bare CI runner (macOS ships bash
+# 3.2, so no associative arrays).
+set -euo pipefail
+
+repo_root="$(cd "$(dirname "$0")/.." && pwd)"
+cd "$repo_root"
+
+if ! git rev-parse --git-dir >/dev/null 2>&1; then
+  echo "check-deleted-tests: not a git repository; skipping."
+  exit 0
+fi
+
+head_ref="${2:-HEAD}"
+
+# Resolve the base. An explicit argument wins. Otherwise, if HEAD is a merge
+# commit we are on a `refs/pull/N/merge` checkout and its first parent is the
+# base branch tip — the pair this guard is designed around. A non-merge HEAD
+# has no merge to inspect, so there is nothing to say.
+if [ -n "${1:-}" ]; then
+  base_ref="$1"
+elif git rev-parse --verify --quiet "${head_ref}^2" >/dev/null; then
+  base_ref="${head_ref}^1"
+else
+  echo "check-deleted-tests: $head_ref is not a merge commit and no base ref was given; skipping."
+  echo "check-deleted-tests: pass a base explicitly to compare two trees, e.g. '$0 origin/main'."
+  exit 0
+fi
+
+if ! git rev-parse --verify --quiet "$base_ref" >/dev/null; then
+  echo "check-deleted-tests: base ref '$base_ref' is not present in this clone." >&2
+  echo "check-deleted-tests: in CI this means the checkout was too shallow — fetch-depth must be at least 2." >&2
+  exit 1
+fi
+
+# Every `#[test]` / `#[tokio::test]` function name in one tree, sorted and
+# deduplicated.
+#
+# One `git grep` over the whole tree at that revision, not one `git show` per
+# file: the per-file shape spawns a process for each of ~1100 source files,
+# which is far too slow for something on the PR path. `-A4` covers the
+# attributes that routinely sit between the test attribute and its `fn` line
+# (`#[should_panic]`, `#[ignore]`, `#[allow(...)]`, a doc comment); the awk
+# state machine gives up at the next test attribute rather than running on, so
+# a wider window costs nothing but a longer read.
+test_names_at() {
+  git grep -h -A4 -E '^[[:space:]]*#\[(tokio::)?test[](]' "$1" -- '*.rs' 2>/dev/null |
+    awk '
+      # A new test attribute always restarts the search, so an attribute block
+      # longer than the context window cannot bleed into the next test.
+      /^[[:space:]]*#\[(tokio::)?test[](]/ { looking = 1; next }
+      looking && match($0, /fn[[:space:]]+[A-Za-z0-9_]+/) {
+        name = substr($0, RSTART, RLENGTH)
+        sub(/^fn[[:space:]]+/, "", name)
+        print name
+        looking = 0
+      }
+    ' | LC_ALL=C sort -u
+}
+
+base_names="$(mktemp)"
+head_names="$(mktemp)"
+trap 'rm -f "$base_names" "$head_names"' EXIT
+
+test_names_at "$base_ref" >"$base_names"
+test_names_at "$head_ref" >"$head_names"
+
+# In the base tree and not in the merged tree.
+removed="$(LC_ALL=C comm -23 "$base_names" "$head_names")"
+
+if [ -z "$removed" ]; then
+  scanned=$(wc -l <"$base_names" | tr -d ' ')
+  # The verdict is already decided; the write is best-effort. SIGPIPE is
+  # ignored and the write's failure discarded, so a reader that closed the
+  # pipe (`| head -1`) cannot turn a green verdict into a failure (#1815).
+  trap '' PIPE
+  echo "check-deleted-tests: OK — $scanned test(s) in $base_ref, none lost by the merge." || true
+  exit 0
+fi
+
+# The acknowledgement text. `PR_BODY` is the reliable channel in CI — a
+# shallow checkout has no commit messages to read — and `git log` is a
+# best-effort addition for local runs and deeper clones, so a commit trailer
+# naming the test also passes.
+ack="${PR_BODY:-}"
+if commits="$(git log --format='%B' "$base_ref..$head_ref" 2>/dev/null)"; then
+  ack="$ack
+$commits"
+fi
+
+unacknowledged=""
+for name in $removed; do
+  case "$ack" in
+  *"$name"*) ;;
+  *) unacknowledged="$unacknowledged  $name
+" ;;
+  esac
+done
+
+if [ -z "$unacknowledged" ]; then
+  count=$(printf '%s\n' "$removed" | wc -l | tr -d ' ')
+  trap '' PIPE
+  echo "check-deleted-tests: OK — $count removed test(s), each named in the PR description or a commit." || true
+  exit 0
+fi
+
+{
+  echo "check-deleted-tests: FAILED"
+  echo ""
+  echo "These tests exist in $base_ref but not in the merged tree, and nothing"
+  echo "in the PR description or the branch's commit messages names them:"
+  echo ""
+  printf '%s' "$unacknowledged"
+  echo ""
+  echo "Two things this can mean, and they need opposite fixes:"
+  echo ""
+  echo "  1. YOU DID NOT MEAN TO DELETE THEM. This is the case the guard exists"
+  echo "     for (#1976): your branch rewrote a file from a base older than"
+  echo "     someone else's merge, so their test is simply absent from your"
+  echo "     side and git had no conflict to report. Rebase onto the base"
+  echo "     branch and restore the lines you did not intend to drop. A witness"
+  echo "     that no longer exists cannot fail, so nothing else will tell you."
+  echo ""
+  echo "  2. YOU DID. Renaming, folding into a table-driven case, or dropping a"
+  echo "     test with the feature it covered are all fine. Name each test above"
+  echo "     in the PR description (or a commit message) and this passes — the"
+  echo "     point is that a reviewer reads the sentence, not that the deletion"
+  echo "     is forbidden."
+  echo ""
+  echo "A moved or renamed test is reported here because this guard keys on the"
+  echo "bare function name; that is deliberate, and naming it in the PR is the"
+  echo "whole cost."
+} >&2
+
+exit 1