review-rigor: R1d — shared-state and sibling-fix interactions within one change - #278
Open
ClintonSarkar wants to merge 1 commit into
Open
Conversation
…hin one change R1b enumerates callers of a shared symbol across services; R1d is its intra-change twin. Two triggers: a fix that repurposes a shared field/flag read by more than one function (a field can carry two roles — a cache that is both a getter's published value and a diff's baseline — and serving one silently corrupts the other), and multiple fixes to one subsystem in a single change where each changes state a sibling depends on (verify the cross-product, not each fix in isolation — a new guard and a new skip can each be correct alone and combine into a stranded lane). Names the self-review trap: checking each fix against the blocker it closes passes them all while the interaction goes unexamined. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Adds R1d to the shared review-rigor R1 family (
docs/core/review-rigor.md): the intra-change twin of R1b. Where R1b enumerates callers of a shared symbol across services, R1d fires when (1) a fix repurposes a shared field/flag/cached value read by more than one function, or (2) several fixes land in one change and each changes state a sibling fix depends on. The check is to enumerate every reader of the changed state (and every sibling fix touching it) and verify the cross-product, not each fix against its own purpose in isolation.Why
A change carried several fixes to one subsystem, and over successive review rounds each fix seeded the next round's blocker — because the fixes interacted through shared state that per-fix self-review never examined. One fix's "skip the healthy lane" predicate and another fix's new guard on that lane's I/O were each correct alone but combined to strand the lane. A later fix stopped clearing a cached field so a diff would survive — and broke the other role of that same field (the value a getter published to a heartbeat), advertising stale data. Each fix passed review against the blocker it closed; the interaction between fixes went unchecked until a reviewer's tests targeted exactly those seams.
Generalized to the class: any change where one field/flag is read in more than one place and the diff changes its meaning or lifecycle, or where two-plus behavioral fixes touch the same subsystem.
📚 Learned by detritus — grow `godot-idx#184 review rounds 2-3`