Surface which commit each bot's last review actually saw (#27)#29
Conversation
The cheap half of #27. A receipt binds to the head and a push invalidates it. That answers "was this exact code reviewed" — but not "by whom, and how much of it did they see". A bot can review commit 1, go rate-limited through a material redesign, and the merge proceed on a fallback receipt taken at commit 5. #22 shipped a fail-open rework the primary reviewer never saw; #25 repeated it smaller, three commits behind. Both times the gap was real, both times it was only reconstructible by reading the PR thread afterwards, and both times someone had to notice it by hand. Now `pr_watch` reads it off the reviews it ALREADY fetches — no extra `gh` call: ⚠ review coverage: coderabbit's last review was of 954b93f, not the current head — a receipt taken now does not mean it saw this design Verified against the real #25, where it reproduces exactly the gap I had to work out manually before merging it an hour ago. **Reported, never gating.** The faithful fix — invalidate a receipt when the diff changes *shape*, not merely when the head moves — is the other half of #27 and stays open, because it risks becoming a wedge on a repo whose bot is permanently unavailable. Making the gap visible is the part that is unambiguously safe, and it is what would have changed the operator's judgment on both #22 and #25. Malformed input is dropped rather than reported as coverage of an unknown commit, and the function cannot raise — it runs on the ordinary poll path. Tests 286 → 289. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ 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 |
The adversarial review ran 12 mutants; 9 died, 2 were real gaps, 1 equivalent.
**`anchored=True` had no test.** Flipping it to `anchored=False` passed all 289
— and with that flip `xcoderabbit` reviewing the head reads as CodeRabbit having
covered it, suppressing the exact warning this feature exists to raise. It is
the one property here with an actual adversary (anyone may review on a public
repo), and the PR body asserted it while nothing checked it.
**Newest-per-bot was untested independent of array order.** Replacing the whole
comparison with `if True:` — last-in-array wins, timestamps ignored — also
passed. The only multi-review fixture listed its reviews ascending, so the two
rules were indistinguishable. Now pinned with a descending fixture and a
shuffled one. Also pinned the tie-break direction that matters: an undated
review must never displace a dated one, since an undated review at the head
would otherwise set `covers_head` and hide the gap.
**A non-string `oid` crashed `render` on the poll path.** `isinstance(commit,
dict)` validates the container, not the value, so `{"oid": 12345}` passed the
filter and then died on `sha[:7]` — in the function whose stated job is
tolerating malformed input, and in the test that claimed to cover it. Now
type-checked. Same for a non-string `submittedAt`, which raised TypeError
against a sibling review's string.
All four mutants now die.
Corrected three claims that overstated the code: the docstring said "one entry
per bot that has reviewed at all" (a bot whose reviews carry no usable SHA gets
no entry — under-reporting, deliberately, but not what it said); "the function
cannot raise" was absolute and wasn't; and the lexicographic timestamp compare's
dependence on gh's exact serialization was undocumented. The doc's "known gaps"
list now includes coverage's own blind spot — a bot that never reviewed produces
no entry and no warning.
Tests 289 → 293.
Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
…to speak The general-correctness lens, run because the doctrine requires both and the adversarial pass had noted it was only one. It found a real bug that pass missed — which is the disjointness claim holding up again. **The `str()` coercion I added last commit was actively wrong.** It stopped the TypeError and, in doing so, rendered garbage as a string sorting ABOVE every real timestamp (`"20260725" > "2026-07-25T…"`). So a malformed review at the head displaced the real dated one and set `covers_head: True` — suppressing the warning, in exactly the direction the docstring beside it calls "the direction that matters". Type-checking instead of coercing is equally crash-proof and sorts garbage to the bottom with the undated ones. The sharpest part: `test_a_non_string_sha_or_timestamp_cannot_break_the_poll` feeds precisely that input and asserts only that it does not raise. It walked over the hole. **`coverage` was a pass-through that arrived empty where it mattered most.** `record_review` calls `summarize_review_bots` without it, so `coverage` was `[]` on the receipt path — "no data" indistinguishable from "every bot is current", on the one path whose entire subject is what a receipt covers. Now computed inside the function like every other key, and recorded on the receipt as `bots_behind_head` next to `override` and `bot_signal`. Its own message says "a receipt taken now does not mean it saw this design"; it was printing everywhere except where a receipt is taken. **It now defers to a bot that is mid-review.** A bot reviewing a just-pushed head is behind it by construction, and the pending line already says a verdict is coming — so the warning was firing on every poll of the healthy window, training the reader to skim past the case it exists for. **Two mutants that survived the first pass now die.** Rendering the line unconditionally, and wiring the wrong head into the coverage call, both passed 292 tests: nothing drove `build_report` -> `render` with a bot AT the head and asserted silence. Selectivity is the whole value of the warning. Also: a comment describing a guard the previous commit deleted (the repo's named failure mode, again); `coverage` missing from both report-schema docstrings; the doc paragraph sitting under a sentence that said every signal feeds the merge gate, which this one does not. Tests 293 → 297. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Third verification pass on this branch, third round of real findings — and both HIGH ones were introduced by the previous commit. **`bots_behind_head` was never recorded under `--allow-pending-bot-review`.** `behind` was populated inside `if not allow_pending_bot`, so the override path fetched the reviews and discarded them. That override IS the #22/#25 scenario: a bot queued or rate-limited through a redesign, merged on a fallback receipt. Combined with the second bug below it was a total blind spot — neither the poll nor the receipt said the reviewer's last review was of an old commit. Coverage now comes off the `pr view` snapshot before the branch, independent of the check fetch. **The pending-deference swallowed the case it was written to protect.** It deferred to *any* pending entry, but a pending check that has aged past the grace window or been cancelled by an announced outage is the engine saying "this verdict is not coming" — the reviewer-went-away case. So after ~15 minutes of polling any stuck bot, and on the #22 two-check outage shape, the coverage warning went quiet precisely when it should speak. Only an actively *blocking* pending entry defers now. Both were invisible to the two lenses that reviewed the commit introducing them, and both are now pinned by mutation. Also: `bots=bots` threading was correct and pinned by nothing (dropping it passed all 297, since every other test uses the default list where scoped and unscoped are identical); a `.get` on a key the next lookup indexes directly; an over-long docstring line; and the doc claimed `--record-review` "records the same thing" as the poll, which is no longer true — the receipt deliberately does not defer to a pending bot. Tests 297 → 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
Fourth verification pass on this branch, and the first that could not break the
code — no HIGH or MEDIUM. Four LOW items:
- **Coverage was computed twice on the normal path**, and `reviews=`/`head=` on
`record_review`'s `summarize_review_bots` call had gone dead when the direct
call was hoisted above the override branch. Harmless today (pure, no I/O) but
a latent divergence trap: the two calls resolve their bot list differently, so
the receipt and the poll would silently disagree the moment this function
takes a `bots` argument — the exact class
`test_coverage_honours_a_non_default_review_bots_list` was just written to pin.
- **A comment quoted a message string that does not exist** in the tree. Same
stale-comment class this repo keeps finding, three lines below the block the
previous commit edited.
- `record_review`'s docstring enumerated two of the three keys the receipt can
carry; `bots_behind_head` was missing.
- Noted why `.get("blocking")` sits next to direct indexing: a pending entry
missing that key falls out of the deference set and the warning fires, which
is the safe direction.
Tests unchanged at 300.
Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
|
Merging. Receipt: Four review passes — adversarial, general-correctness (the doctrine requires both for this file), then two verification rounds. The first three each found real defects, and each of the last two found a defect introduced by the previous round's fix. The fourth was the first pass on this branch that could not break the code. The two worth naming, because both were mine and both were subtle:
Every finding is pinned by mutation rather than by a test that merely names the property. That was the specific weakness the earlier rounds kept exposing. On stopping here. I never got a clean pass on the sibling PR #25 and got one only on the fourth attempt here, so "the last round found nothing" is weak evidence by this branch's own record. What makes merging reasonable is the risk ceiling, not the round count: CodeRabbit has been rate-limited on every head of this PR; the whole review load was carried by the fallback panel, which is why #26 is the recommended next task. |
I had the wrap-up independently fact-checked against `git log` / `gh` rather than self-certifying it, because the handoff is read at the start of every future session. It came back with 13 issues, three HIGH. The one worth naming: **I reintroduced a number that a review round on #25 had explicitly corrected** — "four ways to corrupt an adopter's config" when `321907e` had already changed it to three, and `init.sh`'s own comment enumerates three. I did that while writing up the lesson that PR bodies keep drifting from their diffs. The friction entry recording that failure now records that it recurred inside the document describing it, and is filed at M not L. **The base rate was understated, in my own argument's favour.** I wrote "eleven rounds, ten with findings" and "#29 produced a clean pass on the fourth attempt". Neither is true: session-wide it is **13 rounds across three PRs, all 13 with findings**, and #29's fourth pass found no HIGH/MEDIUM but still found four LOW including a stale comment. Self-inflicted defects — introduced by the previous round's fix — were 7, not 5. So "re-review until a pass finds nothing new" never terminated, which sharpens rather than weakens the point: the rule needs a stopping criterion, and the one actually used was blast radius. **The proposed rule change was aimed at a rule that already exists.** `safety-critical-changes.md` rule 3 already mandates re-review after every fix round and already notes that fix rounds introduce regressions. The proposal now asks for what is genuinely missing — a termination criterion, and the blast-radius calibration that let #29 merge where #25's standard would not have. Also corrected: the mutation-proved tally (five properties session-wide, not two in one place and four in another); "the panel carried the entire review load" (CodeRabbit completed 3 reviews across 17 heads — most, not all, and one of its three caught a real bug); "five manual launches per round" (two lenses per round, ~10 rounds); a "126 report pairs" figure that exists only in a PR comment and is not reproducible from the repo, now pointing at the in-repo tests instead; a dangling "the two entries above this line" after those entries were deleted; and two inbox entries whose issues this session closed, now marked done. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
CodeRabbit, on the wrap-up. My correction commit fixed the handoff's tally to five session-wide and left the friction log saying two — so the two documents the next session reads disagreed with each other about the very number the correction was about. Both now say five, with the properties enumerated (three on #29, two on #25) so the figure is checkable rather than asserted. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
* Wrap-up: Phase 3b — the queued-vs-unavailable ambiguity, closed Three PRs merged, three issues closed, two filed. - **#25** (closes #19 + #23) — a configured bot resolves to *unavailable* (outage on a comment body OR a status-check description, the surface that was invisible) or *pending* (a verdict still coming). Nothing reaches `converged`, which is what let both be fixed at once without touching the anti-wedge property. - **#28** (closes #10) — the state_paths suite failed from inside a lane worktree because its fixture cleared every sandbox *env* signal and not the *cwd* one. - **#29** (half of #27) — `review_bots.coverage` reports which commit each bot's last review actually saw. - **#26 / #27 filed** from the friction-log inbox, both with concrete sketches. The handoff and friction log carry the detail. The two entries worth repeating: **Eleven review rounds, ten with findings — and five of those were defects introduced by the previous round's fix**, twice at HIGH. #25 never produced a clean pass. That is not a caution about fix rounds; it is the base rate, and the doctrine currently states it as advice. **Reading is not running.** The #19 guard was dead code for the exact bot it was written for, because CodeRabbit's pending check reports the zero timestamp — no amount of re-reading found that; polling the live PR did. Same for a change that looked plainly correct and aborted `init.sh` under `set -eu`. Also recorded: `pre-push` refused this very commit's first attempt, because the narrative files had been swept into a lane branch. The guard fired on its own author with a message that made the fix obvious — logged as the positive case this log rarely captures. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix 13 errors the fact-check found in this wrap-up I had the wrap-up independently fact-checked against `git log` / `gh` rather than self-certifying it, because the handoff is read at the start of every future session. It came back with 13 issues, three HIGH. The one worth naming: **I reintroduced a number that a review round on #25 had explicitly corrected** — "four ways to corrupt an adopter's config" when `321907e` had already changed it to three, and `init.sh`'s own comment enumerates three. I did that while writing up the lesson that PR bodies keep drifting from their diffs. The friction entry recording that failure now records that it recurred inside the document describing it, and is filed at M not L. **The base rate was understated, in my own argument's favour.** I wrote "eleven rounds, ten with findings" and "#29 produced a clean pass on the fourth attempt". Neither is true: session-wide it is **13 rounds across three PRs, all 13 with findings**, and #29's fourth pass found no HIGH/MEDIUM but still found four LOW including a stale comment. Self-inflicted defects — introduced by the previous round's fix — were 7, not 5. So "re-review until a pass finds nothing new" never terminated, which sharpens rather than weakens the point: the rule needs a stopping criterion, and the one actually used was blast radius. **The proposed rule change was aimed at a rule that already exists.** `safety-critical-changes.md` rule 3 already mandates re-review after every fix round and already notes that fix rounds introduce regressions. The proposal now asks for what is genuinely missing — a termination criterion, and the blast-radius calibration that let #29 merge where #25's standard would not have. Also corrected: the mutation-proved tally (five properties session-wide, not two in one place and four in another); "the panel carried the entire review load" (CodeRabbit completed 3 reviews across 17 heads — most, not all, and one of its three caught a real bug); "five manual launches per round" (two lenses per round, ~10 rounds); a "126 report pairs" figure that exists only in a PR comment and is not reproducible from the repo, now pointing at the in-repo tests instead; a dangling "the two entries above this line" after those entries were deleted; and two inbox entries whose issues this session closed, now marked done. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Align the mutation tally across both narrative files CodeRabbit, on the wrap-up. My correction commit fixed the handoff's tally to five session-wide and left the friction log saying two — so the two documents the next session reads disagreed with each other about the very number the correction was about. Both now say five, with the properties enumerated (three on #29, two on #25) so the figure is checkable rather than asserted. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
…eted (#34) * Wrap-up: the fallback review is a panel, and its verification was deleted #31 merged, closing #26. `review.fallback_panel` is the primary substitute when a review bot can't review — one isolated, fresh-context reviewer per lens — with `fallback_commands` kept as the explicitly degraded one-lens mode. The lens contract lives in the new kit-owned `docs/agentic-dev-kit/fallback-review-panel.md`. Two issues filed from the session: **#32** (verify lens coverage instead of self-reporting it) and **#33** (the drift self-check makes mutation testing report false kills). Measured, not asserted: suite 300 at `1ef5fd6` → 320 at `7485512`. **The session's result is a retraction.** Four successive mechanisms were built to make a receipt's coverage claim verifiable by the engine, and each was defeated by the next review round — the last by one decorated character in a field the caller writes themselves, after which the render affirmed the forgery. Rule 1 already says to treat a tightened matcher as a stopgap; four tightenings is well past taking that seriously. What ships records the claim and labels it a claim. Three findings in the friction log matter beyond this PR: - **#33 is retroactive.** Mutation evidence cited across #25, #28 and #29 may be worthless wherever the reviewer did not exclude the drift test — and "N mutants died" was used as a reason to stop reviewing. A false-negative testing tool is worse than none, because it justifies confidence. - **Concurrent review lenses in one working tree destroy each other's work.** Fixed as contract item 7, but recorded for the pattern: any doctrine that says "run N reviewers concurrently" owes them isolation, and mine did not until it bit. - **Deleting a check reintroduced the bug it was masking.** When removing a mechanism as unfit, enumerate what it was rejecting and confirm each case is still handled — the deletion commit did neither. Adopter upgrade instructions are written and now unblocked (they were held until #31 landed): `~/Documents/openkitchen-devkit-upgrade.md` and `~/Documents/cs-toolkit-devkit-adoption.md`. cs-toolkit turned out **not** to be an adopter at all — no config, no init.sh, its own divergent engines — so that one is an adoption plus a fixer change from `done` to `converged`, not an upgrade. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Correct ten errors the wrap-up fact-check found I had the wrap-up fact-checked against `git log` / `gh` / the code rather than self-certifying it. Ten problems, two HIGH. **The handoff never mentioned #33.** That file is what `/session-start` reads, so a next session would not have learned that an open, H-severity, self-described *retroactive* issue exists — while the friction log called it the session's most consequential finding. Now in both the session block and "Open, and owned by nothing yet". **"Verified directly" was attached to the wrong claim.** I verified the drift check's *mechanism* — trivially reproducible. The *figure* (17/17 reported, 7 survivors when excluded) is a reviewing lens's report; the 17 mutants are enumerated nowhere. Both files presented it as first-party measurement, and the friction log's "Measured on #31" was the stronger overclaim. Now labelled attested. Three MEDIUM omissions, each of which would have misled: - the 7 survivors were **closed inside #31** (7/7 caught once real coverage was added) — without that, a reader concludes 7 mutants are live on `main` - the comma-as-punctuation regression is described in past tense with no resolution; it was **fixed inside #31**, so a next session could re-fix something already shipped - the **autonomous self-merge path never displays review coverage** — `dev_session.sh merge` gates on `mergeable` alone. That is the caveat that qualifies the session's headline decision, and it appeared in neither file Also: "a third field" where the same sentence names three predecessors; the unverifiable-artifact conclusion attributed to the adversarial lens when the commit records it in the author's voice; the invalidation scope now naming #31 itself rather than a date; and the friction log flagged as at budget, so a triage sweep is required before the next entry. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
The cheap half of #27. The other half stays open — see below.
The gap
A receipt binds to the current
headRefOidand any push invalidates it. Thatanswers "was this exact code reviewed" — but not "by whom, and how much of it
did they see". A bot can review commit 1, go rate-limited through a material
redesign, and the merge still proceed on a fallback receipt taken at commit 5.
design then changed materially (a fail-open rework) and the rate limit never
lifted. Merged on a fallback receipt.
954b93f, then rate-limitedacross four re-requests while three further commits landed.
Both times the gap was real, both times it was only reconstructible by reading
the PR thread afterwards, and both times a human had to notice it.
What this adds
review_bots.coverage— the commit each configured bot's last reviewactually covered, read off the
reviewsarraypr_watchalready fetches. Noextra
ghcall. When a bot is behind the head, the render says so:It defers to a bot that is actively pending (mid-review of a just-pushed
head is behind it by construction), but not once that check ages past the
grace window or is cancelled by an announced outage — that is the engine saying
the verdict is not coming, which is the case this exists for.
--record-reviewrecords the same gap on the receipt asbots_behind_head,next to
overrideandbot_signal. It is recorded even under--allow-pending-bot-review, since that override is itself the #22/#25scenario.
Verified against the real #25, where it reproduces exactly the gap I had to
work out by hand before merging that PR an hour earlier.
Reported, never gating — and why that's the whole design
The faithful fix is to invalidate a receipt when the diff changes shape rather
than only when the head moves (e.g. require a fresh receipt when a later push
touches a file the recorded review never saw). That stays open on #27, because
it risks becoming a wedge on a repo whose bot is permanently unavailable — the
same tension that made #19 and #23 unfixable in isolation.
Making the gap visible is the part that is unambiguously safe, and it is the
part that would have changed the operator's judgment on both #22 and #25.
Consistent with the rest of this family:
signalrecords why a guard didn'trun,
bot_signalandoverriderecord what a receipt was taken over, andthis records how far behind the reviewer that did run actually is. None of
them block; all of them make an omission legible at merge time.
What review found
Four passes: adversarial, general-correctness (the doctrine requires both for
this file), then two verification rounds. Each of the first three found real
defects, and each of the last two found a defect introduced by the previous
round's fix — a
str()coercion that made garbage sort above real timestampsand suppressed the warning; then an override path that never recorded the gap it
exists to record, plus a pending-deference that swallowed the
reviewer-went-away case. The fourth pass was the first clean one.
Every finding is pinned by mutation, not merely by a test that names the
property — that was the specific weakness the earlier rounds kept exposing.
Details
commit.oidis dropped rather than reported as coverageof an unknown commit, and the function cannot raise — it runs on the ordinary
poll path.
cannot claim coverage.
Tests 286 → 300.
Part of #27 — leaving the issue open for the shape-change half.
https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp