diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 44a5e63..d3f488b 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -172,9 +172,42 @@ 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, + 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 would not stand for its review of this design; + re-request it, or say so explicitly + ``` + + 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. 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 + permanently unavailable. **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 ff0facc..fea5b7f 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": "7f116a78706776feb68e9065f434eedd63a1eec420f3b8314899b8e4e58dfbfe" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "d354acdfb0374013cbb64aa5fe7d8961b3f72356a31055c92c5edfa40d9ce3be" + "sha256": "b04cc9a678a28c9d292b8f4d98062fd24f82e1a5bbdc5ac380dbca7e9394639b" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 888edd1..d9d7aec 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -870,6 +870,79 @@ 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 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 + 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 + # 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, + "covers_head": 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 +952,8 @@ def summarize_review_bots( grace_minutes: float | None = None, pending_since: dict | None = None, signal: str = "ok", + reviews: list[dict] | None = None, + head: str | None = None, ) -> dict: """Resolve each configured review bot to *unavailable*, *pending*, or neither. @@ -934,9 +1009,11 @@ 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; - ``pending_since`` is the updated map for the caller to persist. + 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 author matches no configured bot — a human writing "review skipped" in a PR @@ -1077,6 +1154,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`. 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, @@ -1313,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: @@ -1322,7 +1407,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") @@ -1336,6 +1423,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" + # 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 @@ -1348,6 +1445,10 @@ def record_review( now=now, pending_since=read_pending_since(state, current_head), signal=details.signal, + # 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"]: @@ -1386,6 +1487,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. 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) return {"pr": pr, "recorded_review": True, "review_receipt": receipt} @@ -1439,7 +1546,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 @@ -1470,6 +1580,8 @@ def build_report( now=now or datetime.now(timezone.utc), pending_since=prior_pending_since or {}, signal=details.signal, + reviews=view.get("reviews") or [], + head=view.get("headRefOid"), ) # False-settle guard: right after a push, `gh` can still report the OLD @@ -1609,6 +1721,30 @@ 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. + # + # 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. + # `.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: + # 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 " + "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( f" ⚠ review unavailable [{entry['surface']}] {entry['where']}: " @@ -1658,6 +1794,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 a79394e..8b584bd 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1697,3 +1697,398 @@ 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 + + +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_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. + + 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. + + 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