docs(root): name the check that cannot tell the defect from the fix - #841
mobeenabdullah wants to merge 1 commit into
Conversation
Three instances turned up in one day, two of them in one subsystem, and the existing entry on a measurement standing in for a category does not cover them: there the measurement answered a narrower question than the claim, and widening it is the repair. These measurements are correctly scoped and correctly taken, and return the SAME value whether the code is broken or fixed, so no amount of care with the same observable helps. A drop-target ranking was asserted by ordering, which every candidate metric gets right, so it stayed green across two metrics whose margin width was wrong. An end-to-end probe reported an unbracketed edge to mean a one-directional resolver, which a correct bidirectional margin produces identically. An unresolved-thread count read as a clean review, which a finding written into a review summary leaves at zero. The tell is cheap and is stated as a question to run before trusting a green: what would this check report if the bug were present. The repair is to name a different property and then watch the new assertion reject the broken state, rather than assuming it would have.
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 19 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 908ad2bc77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Closing: the distinction this entry claimed does not holdBoth findings are correct, and together they remove the PR's reason to exist. The The other two examples belong to the existing entry. Ordering is narrower than margin width, not a different observable of the same scope, so "measure the wider thing" is the remedy already prescribed by That leaves no example supporting a separate failure mode. The section restated an existing rule with weaker examples, one of them false. What I am taking from itThe entry's own closing line is "make the failing case real before trusting the repair" — and I did not apply it to the entry. I generalised from three instances and never tested the generalisation against an instance I had not derived it from, which is a rule already in this repository and one I quoted to another lane today. Not refiling this. If the pattern is real it will produce an instance that the existing category genuinely cannot explain, and that instance is what should motivate the entry. Writing the rule first is what produced a false example. The underlying observations still hold and are recorded where they belong: the |
1 participant
Why a new entry rather than extending an existing one
derived-checks.mdalready has "A measurement standing in for a CATEGORY clears more than it checked". That one is about a measurement answering a narrower question than the claim built on it, and the repair is to measure the wider thing.These are different, and conflating them sends you to the wrong repair. The measurement is correctly scoped and correctly taken — and returns the same value whether the code is broken or fixed. Re-running it, widening it, or taking it more carefully cannot help, because the two states are not distinguishable in that observable at all. The repair is to name a different property.
The three instances
Two turned up in one subsystem on one day, which is what made it worth naming rather than filing as three war stories.
bracketed: falseThe first cost two successive wrong metrics on #829 — an ordering assertion stayed green while a 10px requirement became roughly 36px once the pointer sat 100px off a zone's horizontal centre. The second is a live guard in
e2e/tests/canvas/driver.tswhose own doc comment claims it separates those two cases. The third is an open finding against the merge gate.What the entry adds
A tell that is a question rather than a checklist item — "what would this check report if the bug were present?" — run against the broken state you are worried about, not against the code in front of you.
And a second step that the #829 round is the argument for: make the failing case real before trusting the repair. A width assertion added after a wrong metric shipped is only evidence once it has been seen to reject that metric. I confirmed the new one fails when the metric is reverted to the hypotenuse rather than assuming it would, which is the only reason I believe it.
Provenance
Suggested by the page-builder lane after the same shape appeared a third time; the instances come from two lanes and three separate PRs, so it is a repository pattern rather than one person's mistake.
Documentation only — no changeset, no code paths touched.