Write down the invariants this codebase has broken more than once - #941
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes add ChangesInvariant guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/INVARIANTS.md`:
- Around line 65-68: Update the “A search that finds things cannot prove
absence” section to distinguish grep -c output from its exit status: state that
printing 0 means no matching lines, while exit status 1 indicates no match and
exit status 0 indicates a match.
- Around line 90-99: Update the “Identify a target by what an operation changed”
guidance to avoid requiring exactly one changed entry. Keep snapshot-and-diff as
a way to find candidate entries, then validate the allowed semantic changes,
including related value, metadata, version, or companion-entry updates; require
a single change only when it is part of the contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61e5db5c-8986-4002-b130-4e62fe6cdaf3
📒 Files selected for processing (3)
CONTRIBUTING.mddocs/INVARIANTS.mddocs/SUMMARY.md
|
Both corrections taken, and the second is the more interesting one. On the search rule, the wording conflated two senses of "returns". The printed count is zero on no match while the exit status is one, so a script branching on the status and a person reading the number are looking at opposite values. In a document arguing for precision about what a signal means, that was the wrong sentence to be loose in. Both senses are now stated. On identifying a target by what changed, the review is right that requiring exactly one changed entry is too strong, and right about why: a valid write can also update a version, an index or a companion value. What makes it worth more than a wording fix is that it contradicted the rule directly above it. Asserting the count pins an artifact of today's implementation rather than the property being tested, which is the exact mistake the previous section describes. It now says to diff to find candidates and then assert what you expect that write to have changed, with a note that the count form is the earlier mistake wearing different clothes. Worth recording that the count form was correct in the case it came from, where the write genuinely added one entry, which is presumably why it read as a general rule when I wrote it down. That is the failure mode of writing rules from a single incident. |
1 participant
Summary
Nine rules, each violated in code that had already been reviewed, compiled and passed CI, and each caught by an adversarial reader or a failing test rather than by anyone re-reading the change. That is the argument for writing them down: they are not obvious at the point of writing, and inspection does not catch them.
Ordered by consequence. The first is the one that loses funds: never destroy a source until the copy reads back, where reading back means retrieving the value, not observing that a write returned success. The share migration erased an original after a commit that reported success having stored nothing, because a failed read looks like an absent value and writing an absent value removes the destination key. The first attempt at fixing it repeated the error one level up by guarding on whether the destination contained the key, which resolves a name and never decrypts.
The rest cover the same ground at lower cost: absence and failure need to be distinguishable wherever they cross a boundary, bounding a cost is a different change from relaxing a guard, a search that finds things cannot prove absence, and a test should assert the property that must hold rather than the artifact that happens to be present.
Two are about the tooling rather than the code. A test that has never failed proves nothing, with the two failure modes named: a test can be vacuous because its setup never reaches the code, and it can be masked later, as happened when a bound added elsewhere began rejecting a staged input before the behaviour under test ran. And green CI is not correctness, with this repository's specific blind spots listed: the platform and reproducibility jobs do not run on pull requests, draft pull requests skip instrumented tests entirely, and keep-android cannot be built locally without the full cross-compile.
Every example is a real one from this codebase, with the mechanism rather than a summary, because the mechanism is what makes the rule usable on a case that does not look the same.
Linked from the contributing guide, gated on the changes where it matters, and added to the docs index so it is reachable rather than orphaned.
Test plan
Documentation only. Verified the linked paths resolve and that the doc is listed in the index alongside the existing pages.
Summary by CodeRabbit