Skip to content

Commit 859a069

Browse files
committed
fix(reviewer-bot): honor approval suppression semantics
1 parent 2f1eba2 commit 859a069

5 files changed

Lines changed: 102 additions & 3 deletions

File tree

scripts/reviewer_bot_core/approval_policy.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,22 @@
2222
def compute_pr_approval_state_from_reviews(
2323
survivors: dict[str, dict],
2424
*,
25+
current_reviewer: str | None,
2526
current_head: str,
2627
permission_statuses: dict[str, str],
2728
) -> dict[str, object]:
2829
approvals = [review for review in survivors.values() if str(review.get("state", "")).upper() == "APPROVED"]
30+
current_reviewer_approvals = []
31+
current_reviewer_key = current_reviewer.lower() if isinstance(current_reviewer, str) and current_reviewer.strip() else None
32+
if current_reviewer_key is not None:
33+
for review in approvals:
34+
author = review.get("user", {}).get("login")
35+
if isinstance(author, str) and author.lower() == current_reviewer_key:
36+
current_reviewer_approvals.append(review)
2937
completion = {
30-
"completed": bool(approvals),
38+
"completed": bool(current_reviewer_approvals),
3139
"current_head_sha": current_head,
32-
"qualifying_review_ids": [review.get("id") for review in approvals],
40+
"qualifying_review_ids": [review.get("id") for review in current_reviewer_approvals],
3341
}
3442

3543
has_write_approval = False
@@ -100,6 +108,7 @@ def compute_pr_approval_state_result(
100108
)
101109
result = compute_pr_approval_state_from_reviews(
102110
survivors,
111+
current_reviewer=review_data.get("current_reviewer"),
103112
current_head=current_head,
104113
permission_statuses=permission_cache,
105114
)

scripts/reviewer_bot_core/reviewer_response_policy.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ def derive_reviewer_response_state(
201201
had_reviewer_review: bool = False,
202202
approval_result: dict[str, object] | None = None,
203203
current_head_approval_authors: tuple[str, ...] | None = None,
204+
stored_reviewer_review: dict | None | object = _UNSET,
204205
) -> dict[str, object]:
205206
current_reviewer = review_data.get("current_reviewer")
206207
if not isinstance(current_reviewer, str) or not current_reviewer.strip():
@@ -214,6 +215,8 @@ def derive_reviewer_response_state(
214215
reviewer_comment = review_data.get("reviewer_comment", {}).get("accepted")
215216
if reviewer_review is _UNSET:
216217
reviewer_review = review_data.get("reviewer_review", {}).get("accepted")
218+
if stored_reviewer_review is _UNSET:
219+
stored_reviewer_review = reviewer_review
217220
if contributor_comment is _UNSET:
218221
contributor_comment = review_data.get("contributor_comment", {}).get("accepted")
219222

@@ -353,7 +356,7 @@ def derive_reviewer_response_state(
353356
approval_authors = tuple(author for author in (current_head_approval_authors or ()) if isinstance(author, str) and author.strip())
354357
normalized_approval_authors = {author.lower() for author in approval_authors}
355358
if current_reviewer.lower() in normalized_approval_authors:
356-
latest_review_head = reviewer_review.get("reviewed_head_sha") if isinstance(reviewer_review, dict) else None
359+
latest_review_head = stored_reviewer_review.get("reviewed_head_sha") if isinstance(stored_reviewer_review, dict) else None
357360
if not isinstance(latest_review_head, str) or latest_review_head != current_head:
358361
return _decorate_response(
359362
state="projection_failed",
@@ -478,6 +481,7 @@ def compute_reviewer_response_state(
478481

479482
reviewer_comment = review_data.get("reviewer_comment", {}).get("accepted")
480483
reviewer_review = review_data.get("reviewer_review", {}).get("accepted")
484+
stored_reviewer_review = reviewer_review
481485
contributor_comment = review_data.get("contributor_comment", {}).get("accepted")
482486
had_reviewer_review = isinstance(reviewer_review, dict)
483487

@@ -580,4 +584,5 @@ def compute_reviewer_response_state(
580584
had_reviewer_review=had_reviewer_review,
581585
approval_result=approval_result,
582586
current_head_approval_authors=approval_authors,
587+
stored_reviewer_review=stored_reviewer_review,
583588
)

tests/unit/reviewer_bot/test_approval_policy_equivalence.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ def _legacy_compute_pr_approval_state_result(
128128
)
129129
result = compute_pr_approval_state_from_reviews(
130130
survivors,
131+
current_reviewer=review_data.get("current_reviewer"),
131132
current_head=current_head,
132133
permission_statuses=permission_cache,
133134
)

tests/unit/reviewer_bot/test_reviews_live_fetch.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,58 @@ def test_compute_reviewer_response_state_reports_awaiting_write_approval_after_c
241241
assert response_state["reason"] == "current_head_alternate_approval_present"
242242

243243

244+
def test_compute_reviewer_response_state_blocks_public_current_head_approval_contradiction_before_refresh(monkeypatch):
245+
state = make_state()
246+
review = make_tracked_review_state(state, 42, reviewer="alice", active_cycle_started_at="2026-03-17T09:00:00Z")
247+
accept_reviewer_review(
248+
review,
249+
semantic_key="pull_request_review:99",
250+
timestamp="2026-03-17T09:30:00Z",
251+
actor="alice",
252+
reviewed_head_sha="head-0",
253+
source_precedence=1,
254+
)
255+
routes = RouteGitHubApi().add_pull_request_snapshot(42, pull_request_payload(42, head_sha="head-1")).add_pull_request_reviews(
256+
42,
257+
[review_payload(10, state="APPROVED", submitted_at="2026-03-17T10:01:00Z", commit_id="head-1", author="alice")],
258+
)
259+
runtime = _runtime(monkeypatch, routes)
260+
runtime.github.get_user_permission_status = lambda username, required_permission="push": "granted"
261+
262+
response_state = reviews.compute_reviewer_response_state(runtime, 42, review)
263+
264+
assert response_state["state"] == "projection_failed"
265+
assert response_state["reason"] == "public_current_head_approval_contradiction"
266+
assert response_state["suppression_reason"] == "public_current_head_approval_contradiction"
267+
268+
269+
def test_rebuild_pr_approval_state_does_not_persist_completion_from_alternate_approval(monkeypatch):
270+
state = make_state()
271+
review = make_tracked_review_state(state, 42, reviewer="alice", active_cycle_started_at="2026-03-17T09:00:00Z")
272+
accept_reviewer_review(review, semantic_key="pull_request_review:10", timestamp="2026-03-17T10:01:00Z", actor="alice", reviewed_head_sha="head-1", source_precedence=1)
273+
routes = RouteGitHubApi().add_pull_request_snapshot(42, pull_request_payload(42, head_sha="head-1")).add_pull_request_reviews(
274+
42,
275+
[review_payload(11, state="APPROVED", submitted_at="2026-03-17T10:05:00Z", commit_id="head-1", author="bob")],
276+
)
277+
runtime = _runtime(monkeypatch, routes)
278+
runtime.github.get_user_permission_status = lambda username, required_permission="push": "granted"
279+
280+
completion, write_approval = reviews.rebuild_pr_approval_state(runtime, 42, review)
281+
282+
assert completion == {
283+
"completed": False,
284+
"current_head_sha": "head-1",
285+
"qualifying_review_ids": [],
286+
}
287+
assert write_approval == {
288+
"has_write_approval": True,
289+
"write_approvers": ["bob"],
290+
"current_head_sha": "head-1",
291+
}
292+
assert review["review_completed_at"] is None
293+
assert review["review_completion_source"] is None
294+
295+
244296
def test_compute_reviewer_response_state_keeps_contributor_handoff_when_stored_review_is_stale(monkeypatch):
245297
state = make_state()
246298
review = make_tracked_review_state(state, 42, reviewer="alice", active_cycle_started_at="2026-03-17T09:00:00Z")

tests/unit/reviewer_bot/test_reviews_projection.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ def test_compute_pr_approval_state_from_reviews_is_pure():
176176

177177
result = approval_policy.compute_pr_approval_state_from_reviews(
178178
survivors,
179+
current_reviewer="alice",
179180
current_head="head-1",
180181
permission_statuses={"alice": "granted"},
181182
)
@@ -185,6 +186,37 @@ def test_compute_pr_approval_state_from_reviews_is_pure():
185186
assert before["survivors"]["alice"]["id"] == 10
186187

187188

189+
def test_compute_pr_approval_state_from_reviews_does_not_mint_completion_from_alternate_approval():
190+
survivors = {
191+
"bob": {
192+
"id": 10,
193+
"state": "APPROVED",
194+
"submitted_at": reviews.parse_github_timestamp("2026-03-17T10:01:00Z"),
195+
"commit_id": "head-1",
196+
"user": {"login": "bob"},
197+
}
198+
}
199+
200+
result = approval_policy.compute_pr_approval_state_from_reviews(
201+
survivors,
202+
current_reviewer="alice",
203+
current_head="head-1",
204+
permission_statuses={"bob": "granted"},
205+
)
206+
207+
assert result["ok"] is True
208+
assert result["completion"] == {
209+
"completed": False,
210+
"current_head_sha": "head-1",
211+
"qualifying_review_ids": [],
212+
}
213+
assert result["write_approval"] == {
214+
"has_write_approval": True,
215+
"write_approvers": ["bob"],
216+
"current_head_sha": "head-1",
217+
}
218+
219+
188220
def test_normalize_reviews_with_parsed_timestamps_is_pure():
189221
review_items = [
190222
{

0 commit comments

Comments
 (0)