|
| 1 | +# Deletion Review |
| 2 | + |
| 3 | +Method for reviewing a diff that **deletes** externally observable names or state |
| 4 | +writes. Pairs with the `deletion-reviewer` agent (`.claude/agents/deletion-reviewer.md`); |
| 5 | +under a harness without that agent, work through this guide directly. |
| 6 | + |
| 7 | +## When this review applies |
| 8 | + |
| 9 | +The diff removes any of: |
| 10 | + |
| 11 | +- a public symbol: type, function, event, enum variant, interface method, parameter |
| 12 | +- a wire name: RPC method, JSON response field, WS subscription |
| 13 | +- a metric name or a metric **label value** |
| 14 | +- a config key, CLI flag, or env var |
| 15 | +- a test or subtest name |
| 16 | + |
| 17 | +Deletions fail differently from additions: the compiler proves that no *code* still |
| 18 | +needs the removed thing, and generic code review then stops there. The two checks below |
| 19 | +cover what neither proves. |
| 20 | + |
| 21 | +## Check 1: the reference sweep goes beyond code |
| 22 | + |
| 23 | +Build a deletion inventory in both code form and **string form** (JSON keys, metric |
| 24 | +label values, method names, subtest names), then sweep the whole tree. Reference sites |
| 25 | +that no compiler sees, in rough order of how often they are missed: |
| 26 | + |
| 27 | +1. **Public docs** (`docs/public-docs/`): field lists *and* example payloads — a JSON |
| 28 | + example embeds a wire field a second time, inside a code fence. |
| 29 | +2. **Grafana dashboards** and monitoring config (`**/grafana/**/*.json`): a removed |
| 30 | + metric or label value leaves a panel charting a permanently empty series. When |
| 31 | + dashboards exist in duplicated variants, keep them byte-identical. |
| 32 | +3. **CI config, justfiles, workflows**: test names, binary names, package lists — |
| 33 | + these are enumerated strings that silently skip or break when a name changes. |
| 34 | +4. READMEs, compose files, scripts. |
| 35 | + |
| 36 | +Classify every hit into exactly one of: |
| 37 | + |
| 38 | +- **must-update** — fix in the same PR; |
| 39 | +- **deliberate survivor** — e.g. a shared Go type another service still populates, or |
| 40 | + an enum value kept for wire compatibility; state the reason in the PR; |
| 41 | +- **same-name, different concept** — verify the boundary and leave it alone. Names are |
| 42 | + overloaded: the same word can label a chain head in one subsystem and a per-message |
| 43 | + validation threshold in another. The sweep must not overreach into live functionality |
| 44 | + that shares the name; when the boundary is subtle, record it in the PR description. |
| 45 | + |
| 46 | +## Check 2: deleted writes — prove *when* the survivors fire, not *whether* they exist |
| 47 | + |
| 48 | +The subtlest deletion bug: removed code was one of several writers to state that |
| 49 | +survives (a status field, tracker, metric, head label), and review "confirms" safety by |
| 50 | +observing that other writers exist. Existence is not coverage. |
| 51 | + |
| 52 | +For each deleted write: |
| 53 | + |
| 54 | +1. Enumerate the surviving writers of the same state. |
| 55 | +2. For each, establish the exact firing conditions — triggering event, guards, mode. |
| 56 | +3. Prove the union covers every window the deleted writer covered. The windows that get |
| 57 | + missed: **startup/initialization**, **sync modes** (EL/snap sync, before derivation |
| 58 | + runs and forkchoice updates are gated), **resets**, **reorgs** (writers that must |
| 59 | + move a value *backward*), and **error/halt paths**. |
| 60 | + |
| 61 | +An uncovered window means the value goes silently stale exactly when operators or |
| 62 | +downstream services (health monitors, dashboards) are watching it. If the old coupling |
| 63 | +was incidental, make the new coupling explicit rather than restoring the deleted path. |
| 64 | + |
| 65 | +Tests for the fix must pin the semantics, not just the happy case: if the write must |
| 66 | +also move a value backward, assert that, or a later "hardening" to advances-only |
| 67 | +reintroduces the staleness. |
| 68 | + |
| 69 | +## Consequential cleanups |
| 70 | + |
| 71 | +- **Now-unproducible code**: error variants or branches whose only producer was |
| 72 | + deleted. Delete them with the producer. |
| 73 | +- **Dead parameters**: values threaded through interfaces that nothing reads after the |
| 74 | + removal — shrink the signatures in the same PR. |
| 75 | +- **Orphaned duplicates**: the deleted code may have been the only reason a *local |
| 76 | + duplicate* of an upstream type or helper existed (a re-declared error type, a copied |
| 77 | + parser). Symbol greps for the deleted names miss these; ask what the deleted code |
| 78 | + justified, not just what it referenced. |
| 79 | +- **Vacuous tests**: an assertion on a removed field can start comparing zero-to-zero |
| 80 | + and pass forever. Prefer making the removed concept an explicit error over returning |
| 81 | + a zero value. When a removed assertion is *replaced*, prove the replacement can fail: |
| 82 | + temporarily invert the property it guards (e.g. add `deny_unknown_fields` to test a |
| 83 | + lenient-parse contract) and watch it go red — a test that cannot fail by construction |
| 84 | + (asserting what the type system already guarantees) protects nothing. |
| 85 | +- **Wire compatibility**: removed RPC/JSON fields parse as zero values in lenient |
| 86 | + clients — trace what each in-repo consumer does with that zero, and disclose the |
| 87 | + removal (breaking-change marker + migration note) for out-of-repo readers. |
| 88 | + |
| 89 | +## Dependency and workspace fallout |
| 90 | + |
| 91 | +Deletions reach build metadata in ways no compiler or clippy run flags — every green |
| 92 | +check stays silent on all of the following: |
| 93 | + |
| 94 | +- **Orphaned dependencies**: deleting a file or module can strip a crate's last use of |
| 95 | + a dependency. Grep for every symbol the deleted file imported — all of them, no |
| 96 | + "certainly still used" shortcuts — and run the unused-dependency gate |
| 97 | + (`cargo +nightly udeps --release --workspace --all-features --all-targets`, the CI |
| 98 | + command) locally; it is the only check that catches this class. |
| 99 | +- **Feature-forwarding removal**: when the removed dependency carried entries in the |
| 100 | + crate's `[features]` lists (`"dep/feature"` forwards), verify each downstream |
| 101 | + consumer enables the forwarded features itself — the one that silently relied on the |
| 102 | + transitive enable is the finding. |
| 103 | +- **Stale feature-list strings**: `"dep/feature"` entries whose enabling code was |
| 104 | + deleted survive symbol greps (they name the dependency, not the symbol) and no |
| 105 | + tooling flags them. Sweep `[features]` sections for the deleted crate and feature |
| 106 | + names explicitly. |
| 107 | +- **Separate-workspace lockfiles**: workspaces that path-depend on a changed crate |
| 108 | + (e.g. the SP1 guest programs workspace) resolve independently. Regenerate their |
| 109 | + lockfiles (CI gates on freshness — `just lock-sp1-guest` / |
| 110 | + `just check-sp1-guest-lock`) and compile the affected consumers **under that |
| 111 | + workspace's own resolution** (`--manifest-path`), not the root workspace's; a crate |
| 112 | + that relied on a now-gone transitive feature enable only fails there. |
| 113 | + |
| 114 | +## False-positive traps |
| 115 | + |
| 116 | +- **Shared types**: a field removed from one implementation's output may legitimately |
| 117 | + remain in a shared struct that another service still populates. Removing it there is |
| 118 | + a separate, wider change — do not flag the survivor as a leftover. |
| 119 | +- **Overloaded names**: see "same-name, different concept" above; a grep hit is a |
| 120 | + question, not a finding. |
| 121 | +- **Duplicated artifacts**: paired dashboards or mirrored configs must be updated |
| 122 | + together; flagging only one file is an incomplete finding. |
0 commit comments