diff --git a/.github/reviewer-bot-tests/test_main.py b/.github/reviewer-bot-tests/test_main.py index 0bae04c0..2c8fa1b7 100644 --- 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 index 5b6526c2..481b59df 100644 --- 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 index 8578a578..8b8ba1d1 100644 --- 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 index 711306fd..4d0a7744 100644 --- 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 index 31705f12..0204f43e 100644 --- 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 index daf1d11e..d48f250c 100644 --- a/scripts/reviewer_bot_lib/sweeper.py +++ b/scripts/reviewer_bot_lib/sweeper.py @@ -519,15 +519,7 @@ def _repair_visible_review_gap(bot, review_data: dict, issue_number: int, source if repair is None: return False author, submitted_at, commit_id = repair - changed = bot.reviews_module.accept_channel_event( - review_data, - "reviewer_review", - semantic_key=source_event_key, - timestamp=submitted_at, - actor=author, - reviewed_head_sha=commit_id, - source_precedence=1, - ) + changed = bot.reviews_module.accept_reviewer_review_from_live_review(review_data, review, actor=author) bot.reviews_module.record_reviewer_activity(review_data, submitted_at) completion, _ = bot.reviews_module.rebuild_pr_approval_state(bot, issue_number, review_data) _mark_reconciled_source_event(review_data, source_event_key)