fix(reviewer-bot): correct reviewer response labels (#541)

PLeVasseur · web-flow · commit 2bd49bd64bc0 · 2026-03-26T11:23:32.000-04:00
diff --git a/.github/reviewer-bot-tests/test_main.py b/.github/reviewer-bot-tests/test_main.py
@@ -292,6 +292,7 @@ def test_main_schedule_backfills_existing_transition_notice_without_duplicate_co
     monkeypatch.setattr(reviewer_bot, "sync_members_with_queue", lambda current: (current, []))
     monkeypatch.setattr(reviewer_bot.maintenance_module, "sweep_deferred_gaps", lambda bot, current: False)
     monkeypatch.setattr(reviewer_bot.maintenance_module, "maybe_record_head_observation_repair", lambda bot, issue_number, review_data: False)
+    monkeypatch.setattr(reviewer_bot, "get_pull_request_reviews", lambda issue_number: [])
     monkeypatch.setattr(reviewer_bot, "get_issue_or_pr_snapshot", lambda issue_number: {"number": issue_number, "state": "open", "pull_request": {}, "labels": []})
     monkeypatch.setattr(reviewer_bot, "save_state", lambda current: True)
     monkeypatch.setattr(reviewer_bot, "sync_status_labels_for_items", lambda current, issue_numbers: True)
diff --git a/.github/reviewer-bot-tests/test_reviewer_bot.py b/.github/reviewer-bot-tests/test_reviewer_bot.py
@@ -396,6 +396,7 @@ def test_scheduled_check_backfills_transition_notice_without_reposting(monkeypat
     review["transition_warning_sent"] = "2026-03-10T00:00:00Z"
     monkeypatch.setattr(reviewer_bot.maintenance_module, "sweep_deferred_gaps", lambda bot, state: False)
     monkeypatch.setattr(reviewer_bot.maintenance_module, "maybe_record_head_observation_repair", lambda bot, issue_number, review_data: False)
+    monkeypatch.setattr(reviewer_bot, "get_pull_request_reviews", lambda issue_number: [])
     monkeypatch.setattr(reviewer_bot, "get_issue_or_pr_snapshot", lambda issue_number: {"pull_request": {}})
     posted = []
     monkeypatch.setattr(reviewer_bot, "post_comment", lambda issue_number, body: posted.append(body) or True)
@@ -418,6 +419,36 @@ def fake_api(method, endpoint, data=None):
     assert posted == []
 
 
+def test_scheduled_check_repairs_missing_reviewer_review_state(monkeypatch):
+    state = make_state()
+    review = reviewer_bot.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["active_cycle_started_at"] = "2026-03-17T09:00:00Z"
+    monkeypatch.setattr(reviewer_bot.maintenance_module, "sweep_deferred_gaps", lambda bot, state: False)
+    monkeypatch.setattr(reviewer_bot.maintenance_module, "maybe_record_head_observation_repair", lambda bot, issue_number, review_data: False)
+    monkeypatch.setattr(reviewer_bot.maintenance_module, "check_overdue_reviews", lambda bot, state: [])
+    monkeypatch.setattr(reviewer_bot, "get_issue_or_pr_snapshot", lambda issue_number: {"pull_request": {}})
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_pull_request_reviews",
+        lambda issue_number: [
+            {
+                "id": 10,
+                "state": "COMMENTED",
+                "submitted_at": "2026-03-17T10:01:00Z",
+                "commit_id": "head-1",
+                "user": {"login": "alice"},
+            }
+        ],
+    )
+    assert reviewer_bot.handle_scheduled_check(state) is True
+    accepted = review["reviewer_review"]["accepted"]
+    assert accepted is not None
+    assert accepted["semantic_key"] == "pull_request_review:10"
+    assert review["last_reviewer_activity"] == "2026-03-17T10:01:00Z"
+
+
 def test_issue_edit_by_author_records_contributor_freshness(monkeypatch):
     state = make_state()
     review = reviewer_bot.ensure_review_entry(state, 42, create=True)
@@ -474,6 +505,127 @@ def test_project_status_labels_uses_commit_id_and_comment_freshness(monkeypatch)
     assert metadata["reason"] == "review_head_stale"
 
 
+def test_project_status_labels_uses_live_current_reviewer_review_when_channel_state_missing(monkeypatch):
+    state = make_state()
+    review = reviewer_bot.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["active_cycle_started_at"] = "2026-03-17T09:00:00Z"
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_issue_or_pr_snapshot",
+        lambda issue_number: {"number": issue_number, "state": "open", "pull_request": {}, "labels": []},
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "github_api",
+        lambda method, endpoint, data=None: {"head": {"sha": "head-1"}} if endpoint == "pulls/42" else None,
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_pull_request_reviews",
+        lambda issue_number: [
+            {
+                "id": 10,
+                "state": "COMMENTED",
+                "submitted_at": "2026-03-17T10:01:00Z",
+                "commit_id": "head-1",
+                "user": {"login": "alice"},
+            }
+        ],
+    )
+    desired_labels, metadata = reviewer_bot.project_status_labels_for_item(42, state)
+    assert desired_labels == {reviewer_bot.STATUS_AWAITING_CONTRIBUTOR_RESPONSE_LABEL}
+    assert metadata["reason"] == "completion_missing"
+
+
+def test_project_status_labels_uses_live_review_fallback_for_stale_head(monkeypatch):
+    state = make_state()
+    review = reviewer_bot.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["active_cycle_started_at"] = "2026-03-17T09:00:00Z"
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_issue_or_pr_snapshot",
+        lambda issue_number: {"number": issue_number, "state": "open", "pull_request": {}, "labels": []},
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "github_api",
+        lambda method, endpoint, data=None: {"head": {"sha": "head-2"}} if endpoint == "pulls/42" else None,
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_pull_request_reviews",
+        lambda issue_number: [
+            {
+                "id": 10,
+                "state": "COMMENTED",
+                "submitted_at": "2026-03-17T10:01:00Z",
+                "commit_id": "head-1",
+                "user": {"login": "alice"},
+            }
+        ],
+    )
+    desired_labels, metadata = reviewer_bot.project_status_labels_for_item(42, state)
+    assert desired_labels == {reviewer_bot.STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}
+    assert metadata["reason"] == "review_head_stale"
+
+
+def test_project_status_labels_prefers_newer_contributor_comment_over_live_review_fallback(monkeypatch):
+    state = make_state()
+    review = reviewer_bot.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["active_cycle_started_at"] = "2026-03-17T09:00:00Z"
+    reviewer_bot.reviews_module.accept_channel_event(
+        review,
+        "contributor_comment",
+        semantic_key="issue_comment:20",
+        timestamp="2026-03-17T10:05:00Z",
+        actor="bob",
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_issue_or_pr_snapshot",
+        lambda issue_number: {"number": issue_number, "state": "open", "pull_request": {}, "labels": []},
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "github_api",
+        lambda method, endpoint, data=None: {"head": {"sha": "head-1"}} if endpoint == "pulls/42" else None,
+    )
+    monkeypatch.setattr(
+        reviewer_bot,
+        "get_pull_request_reviews",
+        lambda issue_number: [
+            {
+                "id": 10,
+                "state": "COMMENTED",
+                "submitted_at": "2026-03-17T10:01:00Z",
+                "commit_id": "head-1",
+                "user": {"login": "alice"},
+            }
+        ],
+    )
+    desired_labels, metadata = reviewer_bot.project_status_labels_for_item(42, state)
+    assert desired_labels == {reviewer_bot.STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}
+    assert metadata["reason"] == "contributor_comment_newer"
+
+
+def test_record_reviewer_activity_does_not_regress_timestamp_on_legacy_backfill():
+    review = reviewer_bot.ensure_review_entry(make_state(), 42, create=True)
+    assert review is not None
+    review["last_reviewer_activity"] = "2026-03-20T10:00:00Z"
+    review["transition_warning_sent"] = "2026-03-21T10:00:00Z"
+    review["transition_notice_sent_at"] = "2026-03-22T10:00:00Z"
+    reviewer_bot.reviews_module.record_reviewer_activity(review, "2026-03-18T10:00:00Z")
+    assert review["last_reviewer_activity"] == "2026-03-20T10:00:00Z"
+    assert review["transition_warning_sent"] is None
+    assert review["transition_notice_sent_at"] is None
+
+
 def test_project_status_labels_emits_awaiting_write_approval_only_after_completion(monkeypatch):
     state = make_state()
     review = reviewer_bot.ensure_review_entry(state, 42, create=True)
diff --git a/scripts/reviewer_bot_lib/maintenance.py b/scripts/reviewer_bot_lib/maintenance.py
@@ -98,6 +98,8 @@ def handle_scheduled_check(bot, state: dict) -> bool:
             issue_snapshot = bot.get_issue_or_pr_snapshot(issue_number)
             if not isinstance(issue_snapshot, dict) or not isinstance(issue_snapshot.get("pull_request"), dict):
                 continue
+            if bot.reviews_module.repair_missing_reviewer_review_state(bot, issue_number, review_data):
+                changed = True
             if maybe_record_head_observation_repair(bot, issue_number, review_data):
                 changed = True
     overdue_reviews = check_overdue_reviews(bot, state)
diff --git a/scripts/reviewer_bot_lib/reconcile.py b/scripts/reviewer_bot_lib/reconcile.py
@@ -11,7 +11,11 @@
     _record_conversation_freshness,
     classify_comment_payload,
 )
-from .reviews import find_triage_approval_after, get_latest_review_by_reviewer
+from .reviews import (
+    accept_reviewer_review_from_live_review,
+    find_triage_approval_after,
+    get_latest_valid_current_reviewer_review_for_cycle,
+)
 
 
 def _now_iso(bot) -> str:
@@ -53,20 +57,10 @@ def _record_review_rebuild(bot, state: dict, issue_number: int, review_data: dic
     completion, _ = bot.reviews_module.rebuild_pr_approval_state(bot, issue_number, review_data, pull_request=pull_request, reviews=reviews)
     if completion is None:
         raise RuntimeError(f"Unable to rebuild approval state for PR #{issue_number}")
-    latest = get_latest_review_by_reviewer(bot, reviews, str(review_data.get("current_reviewer", "")))
+    latest = get_latest_valid_current_reviewer_review_for_cycle(bot, issue_number, review_data, reviews=reviews)
     if latest is not None:
-        commit_id = latest.get("commit_id")
         submitted_at = latest.get("submitted_at")
-        if isinstance(commit_id, str) and isinstance(submitted_at, str):
-            bot.reviews_module.accept_channel_event(
-                review_data,
-                "reviewer_review",
-                semantic_key=f"pull_request_review:{latest.get('id')}",
-                timestamp=submitted_at,
-                actor=review_data.get("current_reviewer"),
-                reviewed_head_sha=commit_id,
-                source_precedence=1,
-            )
+        if accept_reviewer_review_from_live_review(review_data, latest, actor=review_data.get("current_reviewer")) and isinstance(submitted_at, str):
             bot.reviews_module.record_reviewer_activity(review_data, submitted_at)
     return bool(completion.get("completed"))
 
@@ -93,24 +87,14 @@ def reconcile_active_review_entry(
     reviews = bot.get_pull_request_reviews(issue_number)
     if reviews is None:
         return f"❌ Failed to fetch reviews for PR #{issue_number}; cannot run `/rectify`.", False, False
-    latest_review = get_latest_review_by_reviewer(bot, reviews, assigned_reviewer)
+    latest_review = get_latest_valid_current_reviewer_review_for_cycle(bot, issue_number, review_data, reviews=reviews)
     messages: list[str] = []
     if latest_review is not None:
         latest_state = str(latest_review.get("state", "")).upper()
-        commit_id = latest_review.get("commit_id")
         submitted_at = latest_review.get("submitted_at")
-        if latest_state in {"APPROVED", "COMMENTED", "CHANGES_REQUESTED"} and isinstance(commit_id, str) and isinstance(submitted_at, str):
-            state_changed = bot.reviews_module.accept_channel_event(
-                review_data,
-                "reviewer_review",
-                semantic_key=f"pull_request_review:{latest_review.get('id')}",
-                timestamp=submitted_at,
-                actor=assigned_reviewer,
-                reviewed_head_sha=commit_id,
-                source_precedence=1,
-            ) or state_changed
-            bot.reviews_module.record_reviewer_activity(review_data, submitted_at)
+        if accept_reviewer_review_from_live_review(review_data, latest_review, actor=assigned_reviewer) and isinstance(submitted_at, str):
             state_changed = True
+            bot.reviews_module.record_reviewer_activity(review_data, submitted_at)
             messages.append(f"latest review by @{assigned_reviewer} is `{latest_state}`")
     if _record_review_rebuild(bot, state, issue_number, review_data):
         state_changed = True
diff --git a/scripts/reviewer_bot_lib/reviews.py b/scripts/reviewer_bot_lib/reviews.py
@@ -48,6 +48,99 @@ def get_latest_review_by_reviewer(bot, reviews: list[dict], reviewer: str) -> di
     return latest_review
 
 
+def get_latest_valid_current_reviewer_review_for_cycle(
+    bot,
+    issue_number: int,
+    review_data: dict,
+    *,
+    reviews: list[dict] | None = None,
+) -> dict | None:
+    current_reviewer = review_data.get("current_reviewer")
+    if not isinstance(current_reviewer, str) or not current_reviewer.strip():
+        return None
+    boundary = get_current_cycle_boundary(bot, review_data)
+    if boundary is None:
+        return None
+    if reviews is None:
+        reviews = bot.get_pull_request_reviews(issue_number)
+    if reviews is None:
+        return None
+    latest_review = None
+    latest_key = (datetime.min.replace(tzinfo=timezone.utc), "")
+    for review in reviews:
+        if not isinstance(review, dict):
+            continue
+        author = review.get("user", {}).get("login")
+        if not isinstance(author, str) or author.lower() != current_reviewer.lower():
+            continue
+        state = str(review.get("state", "")).upper()
+        if state not in {"APPROVED", "COMMENTED", "CHANGES_REQUESTED"}:
+            continue
+        submitted_at = parse_github_timestamp(review.get("submitted_at"))
+        if submitted_at is None or submitted_at < boundary:
+            continue
+        commit_id = review.get("commit_id")
+        if not isinstance(commit_id, str) or not commit_id.strip():
+            continue
+        review_id = str(review.get("id", ""))
+        review_key = (submitted_at, review_id)
+        if review_key >= latest_key:
+            latest_key = review_key
+            latest_review = review
+    return latest_review
+
+
+def build_reviewer_review_record_from_live_review(review: dict, *, actor: str | None = None) -> dict | None:
+    if not isinstance(review, dict):
+        return None
+    review_id = review.get("id")
+    submitted_at = review.get("submitted_at")
+    commit_id = review.get("commit_id")
+    author = actor if isinstance(actor, str) and actor.strip() else review.get("user", {}).get("login")
+    if not isinstance(review_id, int) or not isinstance(submitted_at, str) or not isinstance(commit_id, str):
+        return None
+    if not isinstance(author, str) or not author.strip():
+        return None
+    return {
+        "semantic_key": f"pull_request_review:{review_id}",
+        "timestamp": submitted_at,
+        "actor": author,
+        "reviewed_head_sha": commit_id,
+        "source_precedence": 1,
+        "payload": {},
+    }
+
+
+def accept_reviewer_review_from_live_review(review_data: dict, review: dict, *, actor: str | None = None) -> bool:
+    record = build_reviewer_review_record_from_live_review(review, actor=actor)
+    if record is None:
+        return False
+    return accept_channel_event(
+        review_data,
+        "reviewer_review",
+        semantic_key=record["semantic_key"],
+        timestamp=record["timestamp"],
+        actor=record["actor"],
+        reviewed_head_sha=record["reviewed_head_sha"],
+        source_precedence=record["source_precedence"],
+        payload=record["payload"],
+    )
+
+
+def repair_missing_reviewer_review_state(bot, issue_number: int, review_data: dict, *, reviews: list[dict] | None = None) -> bool:
+    reviewer_review = review_data.get("reviewer_review", {}).get("accepted")
+    if isinstance(reviewer_review, dict):
+        return False
+    live_review = get_latest_valid_current_reviewer_review_for_cycle(bot, issue_number, review_data, reviews=reviews)
+    if live_review is None:
+        return False
+    submitted_at = live_review.get("submitted_at")
+    changed = accept_reviewer_review_from_live_review(review_data, live_review, actor=review_data.get("current_reviewer"))
+    if isinstance(submitted_at, str):
+        record_reviewer_activity(review_data, submitted_at)
+    return changed
+
+
 def find_triage_approval_after(bot, reviews: list[dict], since: datetime | None) -> tuple[str, datetime] | None:
     permission_cache: dict[str, bool] = {}
     approvals: list[tuple[datetime, str, str]] = []
@@ -180,7 +273,10 @@ def clear_transition_timers(review_data: dict) -> None:
 
 
 def record_reviewer_activity(review_data: dict, timestamp: str) -> None:
-    review_data["last_reviewer_activity"] = timestamp
+    current = parse_github_timestamp(review_data.get("last_reviewer_activity"))
+    candidate = parse_github_timestamp(timestamp)
+    if current is None or candidate is None or candidate >= current:
+        review_data["last_reviewer_activity"] = timestamp
     clear_transition_timers(review_data)
 
 
@@ -554,6 +650,11 @@ def project_status_labels_for_item(
     reviewer_review = review_data.get("reviewer_review", {}).get("accepted")
     contributor_comment = review_data.get("contributor_comment", {}).get("accepted")
 
+    if not reviewer_comment and not reviewer_review and is_pr:
+        live_review = get_latest_valid_current_reviewer_review_for_cycle(bot, issue_number, review_data)
+        if live_review is not None:
+            reviewer_review = build_reviewer_review_record_from_live_review(live_review, actor=current_reviewer)
+
     if not reviewer_comment and not reviewer_review:
         return ({STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}, {"state": "awaiting_reviewer_response", "reason": "no_reviewer_activity"})
 
@@ -572,7 +673,10 @@ def project_status_labels_for_item(
         if not isinstance(latest_review_head, str) or latest_review_head != current_head:
             return ({STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}, {"state": "awaiting_reviewer_response", "reason": "review_head_stale"})
 
-    if _compare_cross_channel_conversation(contributor_comment, reviewer_comment) > 0:
+    latest_reviewer_response = reviewer_comment
+    if _compare_records(reviewer_review, latest_reviewer_response) > 0:
+        latest_reviewer_response = reviewer_review
+    if _compare_cross_channel_conversation(contributor_comment, latest_reviewer_response) > 0:
         return ({STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}, {"state": "awaiting_reviewer_response", "reason": "contributor_comment_newer"})
 
     if is_pr:
diff --git a/scripts/reviewer_bot_lib/sweeper.py b/scripts/reviewer_bot_lib/sweeper.py
