serhiy-bzhezytskyy Red-team your own diff
Purpose
A green suite says the change works. It says nothing about whether the change will survive the people who have to read it, and those people are not generic: each has rules they have enforced in writing, on other contributors' PRs, repeatedly. Attacking your own diff against those rules finds a different class of defect from testing — the guard that hides a failure someone wanted exposed, the title that stopped describing the change, the word the project does not use, the helper twice as long as the invariant requires.
Doing it before pushing also converts what would have been review round-trips into a diff that answers them in advance. In a house where an outside PR waits a month for attention, a round-trip saved is worth more than a defect found.
When to use
- A revision answering a review is written and about to be pushed.
- The change grew beyond what the reviewer asked for — the widened scope is what breaks titles, invalidates earlier measurements, and creates unrelated-change objections.
- You added a guard, a helper, or a default inside something with many callers.
- Before replying to a review at all: the reply's claims are part of the diff's surface.
When NOT to use
- A one-line change nobody has reviewed yet, in a house with no recorded reviewer rules. Push and learn what they say.
- You want the automated reviewer's rulebook rather than a named human's — that is run-the-bots-review-before-it-does; its findings are public on the repo's other PRs and knowable in advance.
- The doubt is about inputs you did not imagine rather than rules you did not follow — then the cheaper adversary is fuzz-before-you-claim-done.
- The reviewer has already approved and asked for nothing. Re-litigating spends the attention this is meant to protect.
- You have no profile material: gather the quotes first (see
../../PRINCIPLES.mdon receipts) — attacking from generic best practice produces generic objections and misses the house's own.
The practice (checklist)
Attack the code.
- ⛔ Does the change hide a failure someone wants exposed? If you added a guard inside a shared helper, enumerate every caller and check none of them expects the guarded condition to fail. Sampling is not enough — check all of them, then say how many you checked.
- ⛔ Re-measure any inertness claim after widening. "This branch never executes in normal runs" is scoped to the code it was measured on. Moving the guard into a helper with N callers voids it. Re-probe and count the calls and the fires.
- Diff what you deleted, not only what you added —
git diff | grep -E '^-.*(assert|wait|&&)'. A rewrite quietly relaxing a two-sided check into a one-sided one is invisible in the added lines. - Read the source behind your assumptions. Any belief about a reply's shape, a coalescing rule, or an ordering guarantee is checkable. One such read here halved a helper: the loop was walking every element because I had not verified that the source already merges them.
Attack the packaging — this is where the real defect usually is.
- ⭐⭐ Does the title still describe the change? After a scope increase, almost never. And if the house squash-merges, the PR title becomes the permanent commit subject.
- ⭐ Vocabulary: grep the files you touched for every noun in your title and body. Words with zero occurrences in the project are imported and a reviewer will notice.
- ⭐ Comment symmetry: count comment lines against call sites. One site carrying four lines of explanation while five carry none is an inconsistency; and an explanation that narrates the bug you found is development process, not delivered state. The why belongs once, in the helper.
- Style parity, measured not assumed: timeout constants, multi-line message shapes, naming convention — count what the file already does and match the majority.
- Commit split: split along what the reviewer asked for versus what you added on your own,
and only if the house's own revisions do that. Measure it —
gh pr view N --json commits --jq '[.commits[].messageHeadline]'on merged PRs that had review rounds. ⛔ And never--amend/force-push once a reviewer is on the thread.
Then resolve, not argue. Each objection ends in one of three states, and say which: refuted by a measurement; a live style objection you are choosing to keep, with the reason; or a real defect you fixed. ⚠️ Never "considered and dismissed".
Rationalizations
| Shortcut | Why it fails |
|---|---|
| "The tests pass, so it's ready." | The tests cover behaviour. They say nothing about a title that lags the diff, a word the project never uses, or a guard that suppresses a failure a test exists to catch. |
| "A reviewer will tell me if something's off." | That costs a round-trip. In a repo where 591 open PRs are older than thirty days, the round-trip is the expensive part, not the fix. |
| "I already reviewed it carefully." | Careful reading found none of the three defects here. Enumerating a named person's recorded rules found them, because the rules point at places care does not look. |
| "It's the same change, just applied more widely." | Wider is exactly what invalidates prior measurements and titles. The inertness claim and the PR title both broke on widening, and nothing else changed. |
| "The helper is fine, it works." | It worked at twice the length and O(range) instead of O(entries), because I had not read the invariant that made the short form correct. Working is not the bar for code a reviewer must hold in their head. |
| "Splitting commits is cleaner." | Measure the house. Ours squash-merges, and its own revision commits are often one, with subjects as short as comment. My instinct said two; the measurement said one. |
RECEIPT
redis/redis#15636, 2026-08-13. A core reviewer asked for a helper; the revision that answered him
also extended the fix to eight more tests, so it touched a proc with 33 call sites. Before
pushing, it was attacked against three reviewers' recorded rules — harvested from ~1,600 of their
inline comments — producing eleven vectors:
Nine refuted by measurement, not by argument:
- "the guard masks a failure a caller wants" (his own rule, stated on another PR: "it feels like we are making the test pass without actually testing what we want") → all 9 call sites with error-expecting context assert after a successful setup; none expects the setup to fail.
- "the guard is not inert in normal runs" → probed: 47 calls, 0 fires, assertions unchanged. ⚠️ This claim had been measured on the one-call-site version and carried over illegitimately; the attack is what forced the re-measure.
- "it relaxes a two-sided wait to one-sided" → the file uses both forms, 3 sites each, and a probe showed no task or trim in flight after the helper returns.
- timeout parity (
1000 10is the file's idiom, 36 occurrences vs 7), reply-shape assumption (verified in the C source, not remembered), hard-coded range (all 5 sites migrate exactly that range), performance (349 s vs a ~350 s baseline), stale-view risk, and single-caller predicate.
One kept as a style objection, with the reason stated rather than silently resolved.
One real defect, and it was in the packaging: ⭐⭐ the title still said "…before ASM error-handling tests import them" while the change now touched a shared helper and ten tests. The house squash-merges, so that title would have become the permanent commit subject. Its top reviewer asks for exactly this in 28 separate comments — "pls update the PR title and top comment".
Three changes the attack produced before anything was pushed:
- the title, rewritten to cover the real scope, using the house's own
Tests:prefix; - a four-line comment cut — six call sites, only one carried an explanation, and that explanation retold the bug rather than describing the delivered state;
- the ownership check reduced from 17 lines to 9 and from O(slots) to O(entries), after reading
the C that guarantees contiguous entries are already merged. Total diff
+47/-22→+34/-21.
⇒ And the attack's discipline caught one more thing on its way out: a leftover sentence reading "the 9th test fails with a different error I haven't looked into". Looking took ten minutes and showed it was the same defect with the polarity reversed — the test's own comment described a precondition it never created. Nine of nine, not eight of nine. Final sweep: 0 failures of 89, down from 9; ordered run unchanged at 104 assertions.
Lifecycle
- Run it on the revision, not the original. The original was reviewed; the revision is what has outgrown its title and its earlier measurements.
- Keep the vector list per house, with each objection's verdict and the command that settled it. It becomes the pre-push gate for the next change and it takes minutes the second time.
- ⚠️ A vector that ends in "probably fine" has not been resolved. Push it to a measurement or record it as an accepted style objection with the reason, so the next person is not re-deciding it.
- ⭐ Report the attack's yield honestly in your own notes — nine refuted, one kept, one real. A red-team that finds nothing was aimed at the wrong rules.