From df7adb79e2c91524b62ca7ad1430b876642b8f70 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 17:42:31 +0300 Subject: [PATCH 1/5] Surface which commit each bot's last review actually saw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/agentic-dev-kit/workflows/pr-watch.md | 17 +++++ kit-manifest.json | 4 +- scripts/pr_watch.py | 59 +++++++++++++++ scripts/tests/test_pr_watch.py | 83 ++++++++++++++++++++++ 4 files changed, 161 insertions(+), 2 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 44a5e63..2b27578 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -174,6 +174,23 @@ Self-pace on a bounded cadence — don't busy-wait: loop must be able to finish while a bot that never reports sits pending forever. Every signal here feeds the merge gate only. + It also reports **`review_bots.coverage`** — the commit each bot's *last* + review actually saw. A receipt binds to the head and a push invalidates it, + which 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. When a bot's last review is behind the head the render says so: + + ``` + ⚠ review coverage: coderabbit's last review was of 954b93f, not the current + head — a receipt taken now does not mean it saw this design + ``` + + Reported, never gating — deliberately the cheap half of the problem, because + invalidating a receipt on a shape change risks wedging a repo whose bot is + permanently unavailable. Treat it as the prompt to re-request a review, or to + say plainly what the receipt does and does not cover. + **Known gaps, so you don't mistake them for coverage:** - The pending block only exists once the bot has *registered* a check. In the window between `gh pr ready` and the bot creating its check row, "the bot diff --git a/kit-manifest.json b/kit-manifest.json index ff0facc..8a5617a 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "02ec1f95ef6bb0228ec7d5923823fcba5d82a6ec6a355072f39ebf6cfa3b0556" + "sha256": "0398f907d1e6521d77d8961ad1f3884e76bc534805241baac205f419ae6543a2" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "d354acdfb0374013cbb64aa5fe7d8961b3f72356a31055c92c5edfa40d9ce3be" + "sha256": "4b69541a5f35c88597afb400a1056620fc3a17a44c667c59e73daa84ffeb3280" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 888edd1..a309e2f 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -870,6 +870,51 @@ def _age_minutes(timestamp: str | None, now: datetime) -> float | None: return max(0.0, age) +def bot_review_coverage( + reviews: list[dict], + head: str | None, + *, + bots: tuple[str, ...] | None = None, +) -> list[dict]: + """Which commit each configured bot's LAST review actually covered. + + A receipt binds to the current head and a push invalidates it, which 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 still proceed on a fallback receipt taken at commit + 5. That happened on #22 (a fail-open rework the primary reviewer never saw) + and again, smaller, on #25. + + This does not gate anything. It makes the gap *visible at merge time* + instead of reconstructible only by reading the PR thread afterwards — + deliberately the cheap half of issue #27, because the expensive half + (invalidating a receipt when the diff changes shape) risks becoming a wedge + on a repo whose bot is permanently unavailable. + + Returns one entry per bot that has reviewed at all, newest first: + ``{bot, sha, submitted_at, covers_head}``. + """ + if bots is None: + bots = _REVIEW_BOTS + latest: dict[str, dict] = {} + for raw in reviews or []: + bot = _match_bot(_author(raw), bots, anchored=True) + if not bot: + continue + sha = (raw.get("commit") or {}).get("oid") if isinstance(raw.get("commit"), dict) else None + if not sha: + continue + submitted = raw.get("submittedAt") or "" + if bot not in latest or submitted >= latest[bot]["submitted_at"]: + latest[bot] = { + "bot": bot, + "sha": sha, + "submitted_at": submitted, + "covers_head": bool(head) and sha == head, + } + return sorted(latest.values(), key=lambda e: e["submitted_at"], reverse=True) + + def summarize_review_bots( check_details: list[dict], comments: list[dict], @@ -879,6 +924,7 @@ def summarize_review_bots( grace_minutes: float | None = None, pending_since: dict | None = None, signal: str = "ok", + coverage: list[dict] | None = None, ) -> dict: """Resolve each configured review bot to *unavailable*, *pending*, or neither. @@ -1077,6 +1123,9 @@ def summarize_review_bots( return { "grace_minutes": grace_minutes, "signal": "skipped" if not bots else signal, + # Per-bot last-reviewed SHA — see :func:`bot_review_coverage`. Reported, + # never gating. + "coverage": coverage or [], "unavailable": unavailable, "pending": pending, "blockers": blockers, @@ -1470,6 +1519,9 @@ def build_report( now=now or datetime.now(timezone.utc), pending_since=prior_pending_since or {}, signal=details.signal, + coverage=bot_review_coverage( + view.get("reviews") or [], view.get("headRefOid") + ), ) # False-settle guard: right after a push, `gh` can still report the OLD @@ -1609,6 +1661,13 @@ def render(report: dict) -> str: " ⚠ review-bot state could not be read — a queued or rate-limited " "reviewer will NOT be detected on this poll (see stderr)" ) + for entry in bots.get("coverage") or []: + if not entry["covers_head"]: + lines.append( + f" ⚠ review coverage: {entry['bot']}'s last review was of " + f"{entry['sha'][:7]}, not the current head — a receipt taken now " + "does not mean it saw this design" + ) for entry in bots.get("unavailable") or []: lines.append( f" ⚠ review unavailable [{entry['surface']}] {entry['where']}: " diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index a79394e..700c3d4 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1697,3 +1697,86 @@ def test_normal_coderabbit_walkthrough_remains_noise() -> None: assert report["new_comments"] == [] assert report["done"] is True + + +# --------------------------------------------------------------------------- # +# review coverage: which commit the bot's last review actually saw (issue #27) +# --------------------------------------------------------------------------- # + + +def _review(login: str, sha: str, at: str) -> dict: + return {"author": {"login": login}, "commit": {"oid": sha}, "submittedAt": at} + + +def test_a_bot_whose_last_review_predates_the_head_is_surfaced() -> None: + """The #22 shape, and #25's smaller repeat. + + A receipt binds to the head and a push invalidates it — which answers "was + this exact code reviewed", 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 proceeds on a fallback receipt taken at commit 5. Nothing said so. + """ + pr_watch = _load_pr_watch() + reviews = [ + _review("coderabbitai", "aaaaaaa1", "2026-07-25T12:00:00Z"), + _review("coderabbitai", "bbbbbbb2", "2026-07-25T13:00:00Z"), # newest + _review("topij", "ccccccc3", "2026-07-25T14:00:00Z"), # not a bot + ] + + behind = pr_watch.bot_review_coverage(reviews, "zzzzzzz9") + current = pr_watch.bot_review_coverage(reviews, "bbbbbbb2") + + # Newest review per bot wins, and a human's review is not a bot's coverage. + assert [e["bot"] for e in behind] == ["coderabbit"] + assert behind[0]["sha"] == "bbbbbbb2" + assert behind[0]["covers_head"] is False + assert current[0]["covers_head"] is True + + +def test_review_coverage_is_reported_and_never_gates() -> None: + """Deliberately the cheap half of #27. Invalidating a receipt when the diff + changes *shape* is the faithful fix, but it risks becoming a wedge on a repo + whose bot is permanently unavailable — so this only makes the gap visible at + merge time instead of reconstructible from the PR thread afterwards. + """ + pr_watch = _load_pr_watch() + view = _green_view( + reviews=[_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")] + ) + + report = pr_watch.build_report( + view, [], set(), review_receipt={"head": "abc123", "source": "fallback:panel"} + ) + + assert report["review_bots"]["coverage"][0]["covers_head"] is False + assert "review coverage" in pr_watch.render(report) + assert "0ldc0de" in pr_watch.render(report) + # …and the merge gate is untouched by it. + assert report["mergeable"] is True + assert report["merge_blockers"] == [] + + +def test_coverage_tolerates_a_missing_or_malformed_commit_field() -> None: + """`gh` shapes drift and a review can predate the field. Anything unusable + is dropped rather than reported as coverage of an unknown commit — and it + must never raise, since this feeds the ordinary poll path.""" + pr_watch = _load_pr_watch() + + assert pr_watch.bot_review_coverage([], "abc") == [] + assert pr_watch.bot_review_coverage(None, "abc") == [] + assert ( + pr_watch.bot_review_coverage( + [ + {"author": {"login": "coderabbitai"}, "submittedAt": "x"}, # no commit + {"author": {"login": "coderabbitai"}, "commit": "oops"}, # not a dict + {"author": {"login": "coderabbitai"}, "commit": {}}, # no oid + ], + "abc", + ) + == [] + ) + # No head to compare against: reported, but never claimed to cover it. + orphan = pr_watch.bot_review_coverage( + [_review("coderabbitai", "aaa", "2026-07-25T12:00:00Z")], None + ) + assert orphan[0]["covers_head"] is False From 107d70505f3c0b46f101a34c1ea1d7181998ea18 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 17:52:36 +0300 Subject: [PATCH 2/5] Pin the two properties a mutation pass proved were unpinned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/agentic-dev-kit/workflows/pr-watch.md | 3 + kit-manifest.json | 4 +- scripts/pr_watch.py | 36 +++++-- scripts/tests/test_pr_watch.py | 103 +++++++++++++++++++++ 4 files changed, 138 insertions(+), 8 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 2b27578..80184ab 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -192,6 +192,9 @@ Self-pace on a bounded cadence — don't busy-wait: say plainly what the receipt does and does not cover. **Known gaps, so you don't mistake them for coverage:** + - `coverage` reports only bots that have reviewed *and* whose review carried a + commit SHA. A bot that has never reviewed at all produces no entry and no + warning — that case is the pending/unavailable machinery's, not this one's. - The pending block only exists once the bot has *registered* a check. In the window between `gh pr ready` and the bot creating its check row, "the bot hasn't started yet" is indistinguishable from "this repo has no bot", so a diff --git a/kit-manifest.json b/kit-manifest.json index 8a5617a..81e86c6 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "0398f907d1e6521d77d8961ad1f3884e76bc534805241baac205f419ae6543a2" + "sha256": "6d97055a3091a18c786254ba7e570335fe460315d51306db59d104aecc37dea0" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "4b69541a5f35c88597afb400a1056620fc3a17a44c667c59e73daa84ffeb3280" + "sha256": "f7f88dd918ed3258aa7492df7761c45f24c9557cf2dba5079d692081c0684a97" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index a309e2f..7069b9d 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -891,26 +891,50 @@ def bot_review_coverage( (invalidating a receipt when the diff changes shape) risks becoming a wedge on a repo whose bot is permanently unavailable. - Returns one entry per bot that has reviewed at all, newest first: - ``{bot, sha, submitted_at, covers_head}``. + Returns one entry per bot with at least one review carrying a usable commit + SHA, newest first: ``{bot, sha, submitted_at, covers_head}``. A bot whose + reviews all lack one is indistinguishable here from a bot that never + reviewed — the under-reporting direction, chosen because claiming coverage + of an unknown commit would be worse than claiming none. + + Recency is a lexicographic compare on GitHub's ISO timestamps, which is + correct only because ``gh`` emits fixed-width ``Z``-suffixed UTC with no + fractional seconds (``2026-07-25T12:29:16Z``). An offset form or a + fractional second would sort wrong; neither is reachable from ``gh``. Ties + resolve to the later array element, matching gh's ascending submission + order — and an undated review can never displace a dated one, which is the + direction that matters: an undated review sitting at the head would + otherwise set ``covers_head`` and suppress the warning. """ if bots is None: bots = _REVIEW_BOTS latest: dict[str, dict] = {} for raw in reviews or []: + # Anchored: a comment author is not the repo's to control, so a + # lookalike login (`xcoderabbit`) must not be able to claim that the + # reviewer covered this head. bot = _match_bot(_author(raw), bots, anchored=True) if not bot: continue - sha = (raw.get("commit") or {}).get("oid") if isinstance(raw.get("commit"), dict) else None - if not sha: + commit = raw.get("commit") + sha = commit.get("oid") if isinstance(commit, dict) else None + # Type-checked, not just truthy: a non-string `oid` survives an + # `isinstance(commit, dict)` guard and then kills `render` on + # `sha[:7]` — on the ordinary poll path, for a field this is supposed + # to be tolerant of. + if not isinstance(sha, str) or not sha: continue - submitted = raw.get("submittedAt") or "" + # `str()`: a non-string timestamp reaching the `>=` below raises + # TypeError against a sibling review's string. + submitted = str(raw.get("submittedAt") or "") if bot not in latest or submitted >= latest[bot]["submitted_at"]: latest[bot] = { "bot": bot, "sha": sha, "submitted_at": submitted, - "covers_head": bool(head) and sha == head, + # `sha` is a non-empty str by here, so a falsy `head` already + # compares False — the guard is intent, not arithmetic. + "covers_head": sha == head, } return sorted(latest.values(), key=lambda e: e["submitted_at"], reverse=True) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 700c3d4..01585dc 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1780,3 +1780,106 @@ def test_coverage_tolerates_a_missing_or_malformed_commit_field() -> None: [_review("coderabbitai", "aaa", "2026-07-25T12:00:00Z")], None ) assert orphan[0]["covers_head"] is False + + +def test_a_lookalike_login_cannot_claim_the_bot_reviewed_this_head() -> None: + """The one property here with an actual adversary. + + On a public repo any account can open a review, so `xcoderabbit` reviewing + the current head must not read as CodeRabbit having covered it — that would + suppress the very warning this feature exists to raise. Anchored matching + gives that; without a test, flipping `anchored=True` to `False` passes the + whole suite (verified by mutation). + """ + pr_watch = _load_pr_watch() + + for impostor in ("xcoderabbit", "my-coderabbit-fan", "notcoderabbit"): + assert ( + pr_watch.bot_review_coverage( + [_review(impostor, "abc123", "2026-07-25T12:00:00Z")], "abc123" + ) + == [] + ), impostor + + # The real logins still count, in both spellings GitHub uses. + for real in ("coderabbitai", "coderabbitai[bot]"): + covered = pr_watch.bot_review_coverage( + [_review(real, "abc123", "2026-07-25T12:00:00Z")], "abc123" + ) + assert covered and covered[0]["covers_head"] is True, real + + +def test_the_newest_review_wins_regardless_of_array_order() -> None: + """Pinned independently of array order. + + The other fixture happens to list its reviews ascending, so "newest by + timestamp" and "last in the array" are indistinguishable there — replacing + the whole comparison with `if True:` passes the suite (verified by + mutation). This one lists them descending, so only the timestamp can be + right. + """ + pr_watch = _load_pr_watch() + descending = [ + _review("coderabbitai", "newest0", "2026-07-25T18:00:00Z"), + _review("coderabbitai", "middle0", "2026-07-25T15:00:00Z"), + _review("coderabbitai", "oldest0", "2026-07-25T12:00:00Z"), + ] + + assert pr_watch.bot_review_coverage(descending, "zzz")[0]["sha"] == "newest0" + # …and the same set shuffled resolves identically. + shuffled = [descending[1], descending[2], descending[0]] + assert pr_watch.bot_review_coverage(shuffled, "zzz")[0]["sha"] == "newest0" + + +def test_an_undated_review_never_displaces_a_dated_one() -> None: + """The safety-relevant direction of the tie-break. + + An undated review that happened to sit at the head would otherwise set + `covers_head` and suppress the warning — reporting coverage the reviewer + never gave. Asserted in both array orders, since the comparison is + order-sensitive by construction. + """ + pr_watch = _load_pr_watch() + dated = _review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z") + undated = {"author": {"login": "coderabbitai"}, "commit": {"oid": "current"}} + + for order in ([dated, undated], [undated, dated]): + covered = pr_watch.bot_review_coverage(order, "current") + assert covered[0]["sha"] == "0ldc0de", order + assert covered[0]["covers_head"] is False, order + + +def test_a_non_string_sha_or_timestamp_cannot_break_the_poll() -> None: + """`isinstance(commit, dict)` validates the container, not the value. + + A non-string `oid` passes that guard and then kills `render` on `sha[:7]` — + on the ordinary poll path, in the function whose stated job is tolerating + malformed input. A non-string timestamp raises TypeError against a sibling + review's string. + """ + pr_watch = _load_pr_watch() + + assert ( + pr_watch.bot_review_coverage( + [{"author": {"login": "coderabbitai"}, "commit": {"oid": 12345}}], "abc" + ) + == [] + ) + mixed = [ + _review("coderabbitai", "aaaaaaa", "2026-07-25T12:00:00Z"), + { + "author": {"login": "coderabbitai"}, + "commit": {"oid": "bbbbbbb"}, + "submittedAt": 20260725, # not a string + }, + ] + assert pr_watch.bot_review_coverage(mixed, "zzz") # does not raise + + # …and the render survives whatever survives the filter. + report = pr_watch.build_report( + _green_view(reviews=[{"author": {"login": "coderabbitai"}, "commit": {"oid": 1}}]), + [], + set(), + ) + assert report["review_bots"]["coverage"] == [] + pr_watch.render(report) # would raise on `sha[:7]` without the type check From 4ae8d8f5017fcad743ebde03aa9c9b7c51598d10 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 18:05:15 +0300 Subject: [PATCH 3/5] Fix a bug the first lens missed, and put the warning where it claims to speak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/agentic-dev-kit/workflows/pr-watch.md | 17 +++- kit-manifest.json | 4 +- scripts/pr_watch.py | 70 ++++++++++---- scripts/tests/test_pr_watch.py | 106 +++++++++++++++++++++ 4 files changed, 173 insertions(+), 24 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 80184ab..1a9b365 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -172,7 +172,7 @@ Self-pace on a bounded cadence — don't busy-wait: None of this reaches `converged`. That is deliberate and load-bearing: the watch loop must be able to finish while a bot that never reports sits pending forever. - Every signal here feeds the merge gate only. + Every signal *above* feeds the merge gate only. It also reports **`review_bots.coverage`** — the commit each bot's *last* review actually saw. A receipt binds to the head and a push invalidates it, @@ -183,13 +183,22 @@ Self-pace on a bounded cadence — don't busy-wait: ``` ⚠ review coverage: coderabbit's last review was of 954b93f, not the current - head — a receipt taken now does not mean it saw this design + head — a receipt taken now would not stand for its review of this design; + re-request it, or say so explicitly ``` + It defers to a bot that is currently *pending*: one mid-review of a just-pushed + head is behind it by construction, and the pending line already says a verdict + is coming. Warning in that window too would fire on every poll of the healthy + case and train you to skim past the one this exists for. + + `--record-review` records the same thing on the receipt as `bots_behind_head`, + alongside `override` and `bot_signal` — all three say what the receipt does + *not* stand for. + Reported, never gating — deliberately the cheap half of the problem, because invalidating a receipt on a shape change risks wedging a repo whose bot is - permanently unavailable. Treat it as the prompt to re-request a review, or to - say plainly what the receipt does and does not cover. + permanently unavailable. **Known gaps, so you don't mistake them for coverage:** - `coverage` reports only bots that have reviewed *and* whose review carried a diff --git a/kit-manifest.json b/kit-manifest.json index 81e86c6..d2d11fb 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "6d97055a3091a18c786254ba7e570335fe460315d51306db59d104aecc37dea0" + "sha256": "6f4c67f5248b2223392173c95fc9abcbcc2e0c28d27ebd884c920ea444c02154" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "f7f88dd918ed3258aa7492df7761c45f24c9557cf2dba5079d692081c0684a97" + "sha256": "6ae72a7fd7aab988659f78bf19ec5fd7b928d349d8f357e2a25db661d7f3eb75" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 7069b9d..cf59844 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -924,16 +924,20 @@ def bot_review_coverage( # to be tolerant of. if not isinstance(sha, str) or not sha: continue - # `str()`: a non-string timestamp reaching the `>=` below raises - # TypeError against a sibling review's string. - submitted = str(raw.get("submittedAt") or "") + # Type-checked, NOT coerced. `str(...)` is equally crash-proof and + # actively wrong: it renders garbage as a string that sorts ABOVE every + # real timestamp (`"20260725" > "2026-07-25T…"`, `"{'x': 1}" >` + # anything), so a malformed review at the head displaces the real dated + # one and sets `covers_head` — suppressing the very warning this exists + # to raise. Unusable timestamps must sort to the BOTTOM, exactly like + # the missing ones below. + submitted = raw.get("submittedAt") + submitted = submitted if isinstance(submitted, str) else "" if bot not in latest or submitted >= latest[bot]["submitted_at"]: latest[bot] = { "bot": bot, "sha": sha, "submitted_at": submitted, - # `sha` is a non-empty str by here, so a falsy `head` already - # compares False — the guard is intent, not arithmetic. "covers_head": sha == head, } return sorted(latest.values(), key=lambda e: e["submitted_at"], reverse=True) @@ -948,7 +952,8 @@ def summarize_review_bots( grace_minutes: float | None = None, pending_since: dict | None = None, signal: str = "ok", - coverage: list[dict] | None = None, + reviews: list[dict] | None = None, + head: str | None = None, ) -> dict: """Resolve each configured review bot to *unavailable*, *pending*, or neither. @@ -1004,8 +1009,9 @@ def summarize_review_bots( The map is scoped to a head: a push means a fresh review, so the caller resets it. - Returns ``{grace_minutes, signal, unavailable, pending, blockers, - pending_since}``. ``blockers`` are ready-made ``merge_blockers`` strings; + Returns ``{grace_minutes, signal, coverage, unavailable, pending, blockers, + pending_since}``. ``coverage`` is :func:`bot_review_coverage` over + ``reviews``/``head`` — which commit each bot's last review actually saw. ``blockers`` are ready-made ``merge_blockers`` strings; ``pending_since`` is the updated map for the caller to persist. ``unavailable[].bot`` is ``None`` when a comment matched a marker but its @@ -1147,9 +1153,12 @@ def summarize_review_bots( return { "grace_minutes": grace_minutes, "signal": "skipped" if not bots else signal, - # Per-bot last-reviewed SHA — see :func:`bot_review_coverage`. Reported, - # never gating. - "coverage": coverage or [], + # Per-bot last-reviewed SHA — see :func:`bot_review_coverage`. Computed + # here rather than passed in, so every caller gets it: as a parameter it + # arrived EMPTY on the `record_review` path, where "no data" and "every + # bot is current" were indistinguishable — on the one path whose whole + # subject is what a receipt covers. Reported, never gating. + "coverage": bot_review_coverage(reviews or [], head, bots=bots), "unavailable": unavailable, "pending": pending, "blockers": blockers, @@ -1395,7 +1404,9 @@ def record_review( expected_head = expected_head.strip() if not expected_head: raise ValueError("expected reviewed head must not be empty") - snapshot = _gh_json(["pr", "view", str(pr), "--json", "number,headRefOid"]) + snapshot = _gh_json( + ["pr", "view", str(pr), "--json", "number,headRefOid,reviews"] + ) current_head = snapshot.get("headRefOid") if not current_head: raise ValueError("PR has no headRefOid; cannot bind review evidence") @@ -1409,6 +1420,7 @@ def record_review( # Stays "ok" under an explicit override: `override` already records that the # bot state was deliberately not consulted, so a second key would be noise. bot_signal = "ok" + behind: list[dict] = [] if not allow_pending_bot: # Checks only. Comments cannot cancel a pending block (see # :func:`summarize_review_bots`), so fetching them here would cost a @@ -1421,8 +1433,11 @@ def record_review( now=now, pending_since=read_pending_since(state, current_head), signal=details.signal, + reviews=snapshot.get("reviews") or [], + head=current_head, ) bot_signal = bot_status["signal"] + behind = [e for e in bot_status["coverage"] if not e["covers_head"]] if bot_status["blockers"]: # Persist the first sighting BEFORE refusing. Without this, a cold # `--record-review` (no poll loop running) restarts the grace clock @@ -1459,6 +1474,12 @@ def record_review( # was unreadable and there was no guard to run, so flagging it would put # a permanent false warning on every receipt a bot-less adopter takes. receipt["bot_signal"] = bot_signal + if behind: + # The sibling of `override` and `bot_signal`: all three record what this + # receipt does NOT stand for. Its own message says "a receipt taken now + # does not mean it saw this design" — which was printing everywhere + # except where a receipt is taken. + receipt["bots_behind_head"] = {e["bot"]: e["sha"] for e in behind} state["review_receipt"] = receipt save_state(pr, state) return {"pr": pr, "recorded_review": True, "review_receipt": receipt} @@ -1512,7 +1533,10 @@ def build_report( bot resolved to *unavailable* (an outage announced on either the comment or the check-description surface — an action signal, never a blocker) or *pending* (a verdict still coming, which blocks the merge gate until it - ages past the grace window). Advisory to ``converged`` by construction. + ages past the grace window). Also carries ``coverage`` (which commit each + bot's last review saw) and ``signal`` (whether that state could be read at + all) — both reported, neither gating. Advisory to ``converged`` by + construction. - ``merge_blockers`` — deterministic reasons the PR is not currently safe to merge (draft, blocked/unknown merge state, requested changes, non-open PR, missing current-head review evidence, or a configured review bot whose own @@ -1543,9 +1567,8 @@ def build_report( now=now or datetime.now(timezone.utc), pending_since=prior_pending_since or {}, signal=details.signal, - coverage=bot_review_coverage( - view.get("reviews") or [], view.get("headRefOid") - ), + reviews=view.get("reviews") or [], + head=view.get("headRefOid"), ) # False-settle guard: right after a push, `gh` can still report the OLD @@ -1685,12 +1708,18 @@ def render(report: dict) -> str: " ⚠ review-bot state could not be read — a queued or rate-limited " "reviewer will NOT be detected on this poll (see stderr)" ) + # A bot mid-review of a just-pushed head is behind it BY CONSTRUCTION, and + # the pending line above already says a verdict is coming. Warning there too + # would fire on every poll of the healthy window and train the operator to + # skim past the case this exists for: a reviewer that went away commits ago. + reviewing = {e["bot"] for e in bots.get("pending") or []} for entry in bots.get("coverage") or []: - if not entry["covers_head"]: + if not entry.get("covers_head") and entry["bot"] not in reviewing: lines.append( f" ⚠ review coverage: {entry['bot']}'s last review was of " f"{entry['sha'][:7]}, not the current head — a receipt taken now " - "does not mean it saw this design" + "would not stand for its review of this design; re-request it, or " + "say so explicitly" ) for entry in bots.get("unavailable") or []: lines.append( @@ -1741,6 +1770,11 @@ def render_record_review(report: dict) -> str: f" ⚠ review-bot state was unreadable ({receipt['bot_signal']}) when this " "receipt was taken — the queued-reviewer guard did not run" ) + for bot, sha in (receipt.get("bots_behind_head") or {}).items(): + lines.append( + f" ⚠ {bot}'s last review was of {sha[:7]}, not this head — this receipt " + "does not stand for its review of this design" + ) return "\n".join(lines) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 01585dc..86dc419 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1782,6 +1782,112 @@ def test_coverage_tolerates_a_missing_or_malformed_commit_field() -> None: assert orphan[0]["covers_head"] is False +def test_the_coverage_warning_is_silent_when_the_bot_is_current() -> None: + """Selectivity is the entire value of this warning, and it was the one thing + left unpinned after a dedicated mutation pass. + + Two mutants survived on the same hole: rendering the line unconditionally, + and wiring the WRONG head into the coverage call. Both are invisible unless + a test drives `build_report` -> `render` with a bot whose last review IS at + the head and asserts silence — unit-level `covers_head is True` does not. + """ + pr_watch = _load_pr_watch() + at_head = _green_view( + reviews=[_review("coderabbitai", "abc123", "2026-07-25T12:00:00Z")] + ) + behind = _green_view( + reviews=[_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")] + ) + + assert "review coverage" not in pr_watch.render( + pr_watch.build_report(at_head, [], set()) + ) + assert "review coverage" in pr_watch.render( + pr_watch.build_report(behind, [], set()) + ) + + +def test_the_coverage_warning_defers_to_a_bot_that_is_mid_review() -> None: + """A bot reviewing a just-pushed head is behind it by construction. + + The pending line already says a verdict is coming, so warning as well would + fire on every poll of the healthy window — training the operator to skim + past the case this exists for, a reviewer that went away commits ago. + """ + pr_watch = _load_pr_watch() + view = _green_view( + reviews=[_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")] + ) + mid_review = [ + _bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1)) + ] + + quiet = pr_watch.render( + pr_watch.build_report(view, [], set(), check_details=mid_review, now=NOW) + ) + loud = pr_watch.render(pr_watch.build_report(view, [], set(), now=NOW)) + + assert "review coverage" not in quiet + assert "has not reported yet" in quiet # the pending line still says it + assert "review coverage" in loud + + +def test_a_receipt_records_which_bots_were_behind_the_head( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The message says "a receipt taken now does not mean it saw this design" — + and it was printing everywhere except where a receipt is taken. + + `override` and `bot_signal` both record what a receipt does NOT stand for. + This is their sibling, and it was absent from the one path whose entire + subject is what the receipt covers. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda args: { + "number": 9, + "headRefOid": "abc123", + "reviews": [_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")], + }, + ) + monkeypatch.setattr( + pr_watch, "fetch_check_details", lambda pr, **kw: pr_watch.CheckDetails([], "ok") + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + report = pr_watch.record_review(9, "fallback:panel", "abc123", now=NOW) + + assert recorded[0]["review_receipt"]["bots_behind_head"] == {"coderabbit": "0ldc0de"} + assert "does not stand for its review" in pr_watch.render_record_review(report) + + +def test_an_unusable_timestamp_sorts_to_the_bottom_not_the_top() -> None: + """`str()` coercion is crash-proof and actively wrong. + + It renders garbage as a string sorting ABOVE every real timestamp + (`"20260725" > "2026-07-25T…"`), so a malformed review at the head displaces + the real dated one and sets `covers_head` — suppressing the warning. The + neighbouring crash test fed exactly this input and asserted only that it did + not raise, walking straight over the hole. + """ + pr_watch = _load_pr_watch() + dated = _review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z") + + for junk in (20260725, {"x": 1}, ["a"], 3.5): + at_head = { + "author": {"login": "coderabbitai"}, + "commit": {"oid": "current"}, + "submittedAt": junk, + } + covered = pr_watch.bot_review_coverage([dated, at_head], "current") + assert covered[0]["sha"] == "0ldc0de", junk + assert covered[0]["covers_head"] is False, junk + + def test_a_lookalike_login_cannot_claim_the_bot_reviewed_this_head() -> None: """The one property here with an actual adversary. From 57ebb3b2d464751eb698e472a21ff0b2cbd6b050 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 18:19:07 +0300 Subject: [PATCH 4/5] The override path was silent about exactly what it exists to record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/agentic-dev-kit/workflows/pr-watch.md | 18 ++-- kit-manifest.json | 4 +- scripts/pr_watch.py | 32 +++++-- scripts/tests/test_pr_watch.py | 103 +++++++++++++++++++++ 4 files changed, 141 insertions(+), 16 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 1a9b365..d3f488b 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -187,14 +187,18 @@ Self-pace on a bounded cadence — don't busy-wait: re-request it, or say so explicitly ``` - It defers to a bot that is currently *pending*: one mid-review of a just-pushed + It defers to a bot that is *actively* pending — one mid-review of a just-pushed head is behind it by construction, and the pending line already says a verdict - is coming. Warning in that window too would fire on every poll of the healthy - case and train you to skim past the one this exists for. - - `--record-review` records the same thing on the receipt as `bots_behind_head`, - alongside `override` and `bot_signal` — all three say what the receipt does - *not* stand for. + is coming. It does **not** defer 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 reviewer-went-away case this exists for. + + `--record-review` records the same gap on the receipt as `bots_behind_head`, + next to `override` and `bot_signal` — all three say what the receipt does *not* + stand for. It is recorded even under `--allow-pending-bot-review`, since that + override is itself the #22/#25 scenario. (Unlike the poll render, the receipt + does not defer to a pending bot: recording a receipt is the moment the gap + matters most.) Reported, never gating — deliberately the cheap half of the problem, because invalidating a receipt on a shape change risks wedging a repo whose bot is diff --git a/kit-manifest.json b/kit-manifest.json index d2d11fb..dfeb298 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "6f4c67f5248b2223392173c95fc9abcbcc2e0c28d27ebd884c920ea444c02154" + "sha256": "7f116a78706776feb68e9065f434eedd63a1eec420f3b8314899b8e4e58dfbfe" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "6ae72a7fd7aab988659f78bf19ec5fd7b928d349d8f357e2a25db661d7f3eb75" + "sha256": "88455847b83226714f230b1d66f9e5976e738d4709bdc2c20544c38a05e20dc9" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index cf59844..5b03af9 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1011,8 +1011,9 @@ def summarize_review_bots( Returns ``{grace_minutes, signal, coverage, unavailable, pending, blockers, pending_since}``. ``coverage`` is :func:`bot_review_coverage` over - ``reviews``/``head`` — which commit each bot's last review actually saw. ``blockers`` are ready-made ``merge_blockers`` strings; - ``pending_since`` is the updated map for the caller to persist. + ``reviews``/``head`` — which commit each bot's last review actually saw. + ``blockers`` are ready-made ``merge_blockers`` strings; ``pending_since`` is + the updated map for the caller to persist. ``unavailable[].bot`` is ``None`` when a comment matched a marker but its author matches no configured bot — a human writing "review skipped" in a PR @@ -1420,7 +1421,16 @@ def record_review( # Stays "ok" under an explicit override: `override` already records that the # bot state was deliberately not consulted, so a second key would be noise. bot_signal = "ok" - behind: list[dict] = [] + # Computed BEFORE the override branch, and independently of the check fetch: + # it comes from the `pr view` snapshot. Populating it inside + # `if not allow_pending_bot` made the receipt silent on exactly the path this + # exists for — the override IS the #22/#25 scenario, a bot queued or + # rate-limited through a redesign and merged on a fallback receipt. + behind = [ + e + for e in bot_review_coverage(snapshot.get("reviews") or [], current_head) + if not e["covers_head"] + ] if not allow_pending_bot: # Checks only. Comments cannot cancel a pending block (see # :func:`summarize_review_bots`), so fetching them here would cost a @@ -1437,7 +1447,6 @@ def record_review( head=current_head, ) bot_signal = bot_status["signal"] - behind = [e for e in bot_status["coverage"] if not e["covers_head"]] if bot_status["blockers"]: # Persist the first sighting BEFORE refusing. Without this, a cold # `--record-review` (no poll loop running) restarts the grace clock @@ -1711,10 +1720,19 @@ def render(report: dict) -> str: # A bot mid-review of a just-pushed head is behind it BY CONSTRUCTION, and # the pending line above already says a verdict is coming. Warning there too # would fire on every poll of the healthy window and train the operator to - # skim past the case this exists for: a reviewer that went away commits ago. - reviewing = {e["bot"] for e in bots.get("pending") or []} + # skim past the case this exists for. + # + # Only a BLOCKING pending entry defers, though. 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" — which is precisely the + # reviewer-went-away case, so suppressing coverage there silenced the warning + # in the one situation it was written for. + reviewing = {e["bot"] for e in bots.get("pending") or [] if e.get("blocking")} for entry in bots.get("coverage") or []: - if not entry.get("covers_head") and entry["bot"] not in reviewing: + if not entry["covers_head"] and entry["bot"] not in reviewing: + # Direct indexing, not `.get`: bot_review_coverage always emits all + # four keys, so a `.get` here would only imply a partial entry is + # expected while the very next lookup would KeyError on one. lines.append( f" ⚠ review coverage: {entry['bot']}'s last review was of " f"{entry['sha'][:7]}, not the current head — a receipt taken now " diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 86dc419..8b584bd 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1865,6 +1865,109 @@ def test_a_receipt_records_which_bots_were_behind_the_head( assert "does not stand for its review" in pr_watch.render_record_review(report) +def test_the_override_path_still_records_which_bots_were_behind( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`--allow-pending-bot-review` IS the #22/#25 scenario. + + A bot queued or rate-limited through a redesign, merged on a fallback + receipt — the exact case this feature was written for. Computing `behind` + inside `if not allow_pending_bot` made the receipt silent precisely there, + and combined with the pending-deference below it was a total blind spot: + neither the poll nor the receipt said the reviewer's last review was old. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda args: { + "number": 9, + "headRefOid": "abc123", + "reviews": [_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")], + }, + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + report = pr_watch.record_review( + 9, "fallback:panel", "abc123", allow_pending_bot=True, now=NOW + ) + receipt = recorded[0]["review_receipt"] + + assert receipt["override"] == "pending-bot" + assert receipt["bots_behind_head"] == {"coderabbit": "0ldc0de"} + rendered = pr_watch.render_record_review(report) + assert "recorded over an active override" in rendered + assert "does not stand for its review" in rendered + + +def test_only_a_blocking_pending_bot_silences_the_coverage_warning() -> None: + """The deference must not swallow the case it claims to protect. + + 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. Deferring to *any* pending entry suppressed the + coverage warning in exactly that situation, reachable after ~15 minutes of + polling any stuck bot, and on the #22 two-check outage shape. + """ + pr_watch = _load_pr_watch() + view = _green_view( + reviews=[_review("coderabbitai", "0ldc0de", "2026-07-25T12:00:00Z")] + ) + + def _render(details): + return pr_watch.render( + pr_watch.build_report(view, [], set(), check_details=details, now=NOW) + ) + + # Actively blocking (mid-review) — deferred, as designed. + assert "review coverage" not in _render( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))] + ) + # Aged past grace — "treated as not coming", so coverage MUST speak up. + assert "review coverage" in _render( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(600))] + ) + # Cancelled by an announced outage — the #22 shape. Same. + assert "review coverage" in _render( + [ + _bot_check(description="Review rate limited"), + _bot_check( + name="CodeRabbit / incremental", + state="PENDING", + bucket="pending", + startedAt=_minutes_ago(1), + ), + ] + ) + + +def test_coverage_honours_a_non_default_review_bots_list() -> None: + """`bots=bots` threading was correct and pinned by nothing — dropping it + passed the whole suite, because every other test uses the default list where + scoped and unscoped are identical.""" + pr_watch = _load_pr_watch() + reviews = [ + _review("otherbot", "0ldc0de", "2026-07-25T12:00:00Z"), + _review("coderabbitai", "abc123", "2026-07-25T13:00:00Z"), + ] + + scoped = pr_watch.summarize_review_bots( + [], [], now=NOW, bots=("otherbot",), reviews=reviews, head="abc123" + ) + + assert [e["bot"] for e in scoped["coverage"]] == ["otherbot"] + assert scoped["coverage"][0]["covers_head"] is False + # …and a repo with no bots configured gets no coverage at all. + assert ( + pr_watch.summarize_review_bots( + [], [], now=NOW, bots=(), reviews=reviews, head="abc123" + )["coverage"] + == [] + ) + + def test_an_unusable_timestamp_sorts_to_the_bottom_not_the_top() -> None: """`str()` coercion is crash-proof and actively wrong. From 478a6332a765c1cf98c5166aed30b46acf350343 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 18:28:38 +0300 Subject: [PATCH 5/5] Tidy what the first clean pass found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- kit-manifest.json | 2 +- scripts/pr_watch.py | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index dfeb298..fea5b7f 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "88455847b83226714f230b1d66f9e5976e738d4709bdc2c20544c38a05e20dc9" + "sha256": "b04cc9a678a28c9d292b8f4d98062fd24f82e1a5bbdc5ac380dbca7e9394639b" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 5b03af9..d9d7aec 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1396,8 +1396,10 @@ def record_review( :func:`summarize_review_bots`) — but the grace window bounds that wait to minutes. ``allow_pending_bot`` is the operator's documented override for the remaining case: evidence the queued review will never arrive that pr_watch - cannot see. It is recorded on the receipt, as is a failed check read (but - not "no bots configured" — nothing was unreadable in that case). + cannot see. It is recorded on the receipt as ``override``, as is a failed + check read (``bot_signal``; but not "no bots configured" — nothing was + unreadable in that case) and any bot whose last review predates this head + (``bots_behind_head``). All three say what the receipt does NOT stand for. """ source = source.strip() if not source: @@ -1443,8 +1445,10 @@ def record_review( now=now, pending_since=read_pending_since(state, current_head), signal=details.signal, - reviews=snapshot.get("reviews") or [], - head=current_head, + # No `reviews=`/`head=`: coverage is computed directly above, for + # every path including the override. Passing them here too would + # compute it twice from two different bot lists — identical today, + # silently divergent the moment this function takes a `bots` arg. ) bot_signal = bot_status["signal"] if bot_status["blockers"]: @@ -1485,9 +1489,9 @@ def record_review( receipt["bot_signal"] = bot_signal if behind: # The sibling of `override` and `bot_signal`: all three record what this - # receipt does NOT stand for. Its own message says "a receipt taken now - # does not mean it saw this design" — which was printing everywhere - # except where a receipt is taken. + # receipt does NOT stand for. The poll render warns that a receipt taken + # now would not stand for the bot's review of this design — and that was + # printing everywhere except where a receipt is actually taken. receipt["bots_behind_head"] = {e["bot"]: e["sha"] for e in behind} state["review_receipt"] = receipt save_state(pr, state) @@ -1727,6 +1731,8 @@ def render(report: dict) -> str: # engine saying "this verdict is not coming" — which is precisely the # reviewer-went-away case, so suppressing coverage there silenced the warning # in the one situation it was written for. + # `.get` here, direct indexing below: an entry missing `blocking` falls out + # of `reviewing` and the warning FIRES, which is the safe direction. reviewing = {e["bot"] for e in bots.get("pending") or [] if e.get("blocking")} for entry in bots.get("coverage") or []: if not entry["covers_head"] and entry["bot"] not in reviewing: