fix: close reviewer cleanup gaps

Author: PLeVasseur

scripts/reviewer_bot_lib/app.py

@@ -150,6 +150,7 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
     sync_changes: list[str] = []
     restored: list[str] = []
     loaded_active_reviews_count = 0
+    loaded_active_review_numbers: set[int] = set()
     touched_items: list[int] = []
     projection_failure: RuntimeError | None = None
     loaded_epoch: str | None = None
@@ -186,6 +187,11 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
         active_reviews = state.get("active_reviews")
         if isinstance(active_reviews, dict):
             loaded_active_reviews_count = len(active_reviews)
+            loaded_active_review_numbers = {
+                int(issue_key)
+                for issue_key in active_reviews
+                if isinstance(issue_key, str) and issue_key.isdigit()
+            }
         loaded_epoch = state.get("freshness_runtime_epoch") if isinstance(state.get("freshness_runtime_epoch"), str) else None
 
         if lock_required:
@@ -297,10 +303,16 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                 allow_empty_override = (
                     bot.get_config_value("ALLOW_EMPTY_ACTIVE_REVIEWS_WRITE").strip().lower() == "true"
                 )
+                allow_closed_cleanup_empty = (
+                    loaded_active_reviews_count == len(loaded_active_review_numbers)
+                    and bool(loaded_active_review_numbers)
+                    and set(touched_items) == loaded_active_review_numbers
+                )
                 if (
                     loaded_active_reviews_count > 0
                     and current_active_reviews_count == 0
                     and not allow_empty_override
+                    and not allow_closed_cleanup_empty
                 ):
                     raise RuntimeError(
                         "STATE_GUARD_BLOCKED_EMPTY_ACTIVE_REVIEWS: refusing to persist schedule "

scripts/reviewer_bot_lib/comment_routing.py

@@ -182,7 +182,6 @@ def handle_comment_event(
     issue_number = comment_request.issue_number
     if not issue_number:
         return False
-    bot.collect_touched_item(issue_number)
     route = _route_issue_comment_trust(
         bot,
         comment_request,
@@ -200,11 +199,13 @@ def handle_comment_event(
                 issue_state=comment_request.issue_state,
             )
             return False
+        bot.collect_touched_item(issue_number)
         return _process_comment_event(bot, state, comment_request)
     if route == PrCommentRouterOutcome.TRUSTED_DIRECT:
         if comment_request.issue_state != "open":
             return False
         if not _require_v18_for_pr(bot, state, comment_request, "pr_trusted_direct_comment"):
             return False
+        bot.collect_touched_item(issue_number)
         return _process_comment_event(bot, state, comment_request)
     raise RuntimeError("Deferred PR comment events must not mutate directly in trusted workflows")

scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py

@@ -53,6 +53,20 @@ def get_observer_discovery_watermarks(review_data: dict) -> dict:
     return _observer_discovery_watermarks(review_data)
 
 
+def _deferred_gap_keys(review_data: dict) -> list[str]:
+    return list(_deferred_gaps(review_data))
+
+
+def _get_deferred_gap(review_data: dict, source_event_key: str) -> dict:
+    gap = _deferred_gaps(review_data).get(source_event_key)
+    return gap if isinstance(gap, dict) else {}
+
+
+def _deferred_gap_reason(review_data: dict, source_event_key: str) -> str | None:
+    reason = _get_deferred_gap(review_data, source_event_key).get("reason")
+    return reason if isinstance(reason, str) else None
+
+
 def _now_iso(bot) -> str:
     return bot.clock.now().isoformat()
 
@@ -77,6 +91,16 @@ def _clear_source_event_key(review_data: dict, source_event_key: str) -> bool:
     return False
 
 
+def _update_deferred_gap_fields(review_data: dict, source_event_key: str, fields: dict) -> bool:
+    deferred_gaps = _deferred_gaps(review_data)
+    existing = deferred_gaps.get(source_event_key)
+    if not isinstance(existing, dict):
+        return False
+    previous = deepcopy(existing)
+    existing.update(fields)
+    return previous != existing
+
+
 def _mark_reconciled_source_event(
     review_data: dict,
     source_event_key: str,
@@ -100,7 +124,13 @@ def _mark_reconciled_source_event(
 
 
 def _was_reconciled_source_event(review_data: dict, source_event_key: str) -> bool:
-    return source_event_key in _reconciled_source_events(review_data)
+    existing = _reconciled_source_events(review_data).get(source_event_key)
+    if not isinstance(existing, dict):
+        return False
+    if existing.get("source_event_key") != source_event_key:
+        return False
+    reconciled_at = existing.get("reconciled_at")
+    return isinstance(reconciled_at, str) and bool(reconciled_at.strip())
 
 
 def _payload_or_existing(payload: dict, existing: dict, key: str):

scripts/reviewer_bot_lib/lifecycle.py

@@ -558,8 +558,20 @@ def handle_closed_event(bot, state: dict) -> bool:
     if not issue_number:
         return False
     bot.collect_touched_item(issue_number)
+    return remove_closed_review_entry(bot, state, issue_number, reason="closed_event")
+
+
+def remove_closed_review_entry(bot, state: dict, issue_number: int, *, reason: str) -> bool:
     issue_key = str(issue_number)
     if isinstance(state.get("active_reviews"), dict) and issue_key in state["active_reviews"]:
         del state["active_reviews"][issue_key]
+        _log(
+            bot,
+            "info",
+            f"Removed active review row for closed item #{issue_number}",
+            issue_number=issue_number,
+            reason=reason,
+        )
+        bot.collect_touched_item(issue_number)
         return True
     return False

scripts/reviewer_bot_lib/maintenance_schedule.py

@@ -4,7 +4,11 @@
 
 from dataclasses import dataclass
 
-from .lifecycle import handle_transition_notice, maybe_record_head_observation_repair
+from .lifecycle import (
+    handle_transition_notice,
+    maybe_record_head_observation_repair,
+    remove_closed_review_entry,
+)
 from .overdue import (
     backfill_transition_notice_if_present,
     check_overdue_reviews,
@@ -108,13 +112,16 @@ def _run_tracked_pr_repairs(bot, state: dict) -> bool:
     active_reviews = state.get("active_reviews")
     if not isinstance(active_reviews, dict):
         return False
-    for issue_key, review_data in active_reviews.items():
+    for issue_key, review_data in list(active_reviews.items()):
         if not isinstance(review_data, dict) or not review_data.get("current_reviewer"):
             continue
         issue_number = int(issue_key)
         issue_snapshot = bot.github.get_issue_or_pr_snapshot(issue_number)
         if not isinstance(issue_snapshot, dict) or not isinstance(issue_snapshot.get("pull_request"), dict):
             continue
+        if str(issue_snapshot.get("state", "")).lower() == "closed":
+            changed = remove_closed_review_entry(bot, state, issue_number, reason="scheduled_closed_snapshot") or changed
+            continue
         changed = _run_tracked_pr_repair(bot, issue_number, review_data) or changed
     return changed
 

scripts/reviewer_bot_lib/sweeper.py

@@ -121,7 +121,7 @@ def _diagnose_deferred_event(
     source_event_kind: str,
     workflow_runs: list[dict] | None,
 ) -> None:
-    existing_gap = gap_bookkeeping._deferred_gaps(review_data).get(source_event_key, {})
+    existing_gap = gap_bookkeeping._get_deferred_gap(review_data, source_event_key)
     run_correlation = deferred_gap_diagnosis.correlate_candidate_observer_runs(
         source_event_key,
         source_event_kind=source_event_kind,
@@ -275,11 +275,9 @@ def _purge_bot_authored_comment_gap(bot, review_data: dict, source_event_key: st
     live_comment = _fetch_live_issue_comment(bot, comment_id)
     if not isinstance(live_comment, dict) or not _is_automation_comment(live_comment):
         return False
-    deferred_gaps = gap_bookkeeping._deferred_gaps(review_data)
-    if source_event_key not in deferred_gaps:
+    if source_event_key not in gap_bookkeeping._deferred_gap_keys(review_data):
         return False
-    deferred_gaps.pop(source_event_key, None)
-    return True
+    return gap_bookkeeping._clear_source_event_key(review_data, source_event_key)
 
 
 def _maybe_fetch_single_candidate_run_detail(bot, run_correlation: dict, artifact_correlation: dict | None) -> dict | None:
@@ -488,28 +486,28 @@ def _record_gap_diagnostics(
         reason,
         f"Trusted sweeper diagnostics for {source_event_key}: {diagnostic_reason}. See {bot.REVIEW_FRESHNESS_RUNBOOK_PATH}.",
     )
-    gap = gap_bookkeeping._deferred_gaps(review_data)[source_event_key]
-    gap["full_scan_complete"] = bool(run_correlation.get("full_scan_complete"))
-    gap["later_recheck_complete"] = bool(run_correlation.get("later_recheck_complete"))
-    gap["correlated_run_found"] = bool(run_correlation.get("correlated_run"))
+    gap_fields = {
+        "full_scan_complete": bool(run_correlation.get("full_scan_complete")),
+        "later_recheck_complete": bool(run_correlation.get("later_recheck_complete")),
+        "correlated_run_found": bool(run_correlation.get("correlated_run")),
+    }
     raw_candidate_run_ids = run_correlation.get("candidate_run_ids")
     if isinstance(raw_candidate_run_ids, list):
-        gap["candidate_run_ids"] = raw_candidate_run_ids
+        gap_fields["candidate_run_ids"] = raw_candidate_run_ids
     if isinstance(run_detail, dict):
-        gap["run_created_at"] = run_detail.get("created_at")
+        gap_fields["run_created_at"] = run_detail.get("created_at")
     if isinstance(artifact_correlation, dict):
         prior_visibility = artifact_correlation.get("prior_visibility", {}).get(run_correlation.get("correlated_run"), {})
         if isinstance(prior_visibility, dict):
-            gap.update(prior_visibility)
+            gap_fields.update(prior_visibility)
+    gap_bookkeeping._update_deferred_gap_fields(review_data, source_event_key, gap_fields)
 
 
 def _should_skip_discovered_key(bot, review_data: dict, source_event_key: str, channels: tuple[str, ...]) -> bool:
     if gap_bookkeeping._was_reconciled_source_event(review_data, source_event_key):
         return True
-    deferred_gaps = gap_bookkeeping._deferred_gaps(review_data)
-    if source_event_key in deferred_gaps:
-        existing_gap = deferred_gaps.get(source_event_key)
-        if isinstance(existing_gap, dict) and existing_gap.get("reason") in {
+    if source_event_key in gap_bookkeeping._deferred_gap_keys(review_data):
+        if gap_bookkeeping._deferred_gap_reason(review_data, source_event_key) in {
             "awaiting_observer_run",
             "awaiting_observer_approval",
             "observer_in_progress",
@@ -538,8 +536,7 @@ def sweep_deferred_gaps(bot, state: dict) -> bool:
         pull_request, _ = _read_api_payload(bot, f"pulls/{issue_number}")
         if not isinstance(pull_request, dict) or str(pull_request.get("state", "")).lower() != "open":
             continue
-        deferred_gaps = gap_bookkeeping._deferred_gaps(review_data)
-        for source_event_key in list(deferred_gaps):
+        for source_event_key in gap_bookkeeping._deferred_gap_keys(review_data):
             if _purge_bot_authored_comment_gap(bot, review_data, source_event_key):
                 changed = True
         discovered_comments, comments_complete = _discover_visible_comment_events(bot, issue_number, review_data)
@@ -572,7 +569,7 @@ def sweep_deferred_gaps(bot, state: dict) -> bool:
                 submitted_at = discovered["source_created_at"]
                 if _should_skip_discovered_key(bot, review_data, source_event_key, ("reviewer_review",)):
                     continue
-                existing_gap = gap_bookkeeping._deferred_gaps(review_data).get(source_event_key, {})
+                existing_gap = gap_bookkeeping._get_deferred_gap(review_data, source_event_key)
                 workflow_file = ".github/workflows/reviewer-bot-pr-review-submitted-observer.yml"
                 workflow_runs = _fetch_workflow_runs_for_file(bot, workflow_file, "pull_request_review")
                 run_correlation = deferred_gap_diagnosis.correlate_candidate_observer_runs(

tests/contract/reviewer_bot/test_workflow_files.py

@@ -99,6 +99,15 @@ def test_pr_metadata_workflow_exports_raw_timestamp_boundary_fields():
     assert "EVENT_CREATED_AT: ${{ github.event.pull_request.updated_at }}" not in workflow_text
 
 
+def test_reconcile_workflow_permissions_cover_live_replay_reads():
+    workflow = yaml.safe_load(Path(".github/workflows/reviewer-bot-reconcile.yml").read_text(encoding="utf-8"))
+    permissions = workflow["jobs"]["reconcile"]["permissions"]
+
+    assert permissions["actions"] == "read"
+    assert permissions["issues"] in {"read", "write"}
+    assert permissions["pull-requests"] in {"read", "write"}
+
+
 @pytest.mark.parametrize(
     "workflow_path",
     [

tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py

@@ -53,9 +53,8 @@ def fake_load_state(*, fail_on_unavailable=False):
 
     assert result.exit_code == 0
     assert save_calls == []
-    assert len(sync_calls) == 1
-    assert sync_calls[0][0] is initial_state
-    assert sync_calls[0][1] == [42]
+    assert sync_calls == []
+
 
 def test_execute_run_closed_issue_comment_without_entry_skips_save(monkeypatch):
     harness = AppHarness(monkeypatch)
@@ -91,7 +90,56 @@ def test_execute_run_closed_issue_comment_without_entry_skips_save(monkeypatch):
 
     assert result.exit_code == 0
     assert save_called["value"] is False
-    assert sync_calls == [[42]]
+    assert sync_calls == []
+
+
+def test_execute_run_closed_pr_comment_safe_noop_does_not_save_or_project(monkeypatch):
+    harness = AppHarness(monkeypatch)
+    harness.set_event(
+        EVENT_NAME="issue_comment",
+        EVENT_ACTION="created",
+        ISSUE_NUMBER=42,
+        IS_PULL_REQUEST="true",
+        ISSUE_STATE="closed",
+        ISSUE_AUTHOR="dana",
+        COMMENT_USER_TYPE="User",
+        COMMENT_SENDER_TYPE="User",
+        COMMENT_AUTHOR="alice",
+        COMMENT_AUTHOR_ID=101,
+        COMMENT_AUTHOR_ASSOCIATION="MEMBER",
+        COMMENT_ID=100,
+        COMMENT_CREATED_AT="2026-03-17T10:00:00Z",
+        COMMENT_BODY="@guidelines-bot /queue",
+        COMMENT_PERFORMED_VIA_GITHUB_APP="false",
+        REVIEWER_BOT_ROUTE_OUTCOME="trusted_direct",
+        REVIEWER_BOT_TRUST_CLASS="pr_trusted_direct",
+        GITHUB_REPOSITORY="rustfoundation/safety-critical-rust-coding-guidelines",
+        PR_HEAD_FULL_NAME="rustfoundation/safety-critical-rust-coding-guidelines",
+        PR_AUTHOR="dana",
+        GITHUB_RUN_ID="123",
+        GITHUB_RUN_ATTEMPT="1",
+    )
+
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    save_called = {"value": False}
+    sync_calls = []
+
+    harness.stub_lock(acquire=lambda: None, release=lambda: True)
+    harness.stub_load_state(lambda *, fail_on_unavailable=False: state)
+    harness.stub_pass_until(lambda current: (current, []))
+    harness.stub_sync_members(lambda current: (current, []))
+    harness.stub_save_state(lambda current: save_called.__setitem__("value", True) or True)
+    harness.stub_sync_status_labels(lambda current, issue_numbers: sync_calls.append(list(issue_numbers)) or False)
+
+    result = harness.run_execute()
+
+    assert result.exit_code == 0
+    assert state["active_reviews"]["42"] is review
+    assert save_called["value"] is False
+    assert sync_calls == []
 
 
 def test_execute_run_late_workflow_run_reconcile_missing_row_safe_noop(monkeypatch):

tests/integration/reviewer_bot/test_app_schedule_bookkeeping.py

@@ -219,3 +219,35 @@ def test_execute_run_schedule_warning_diagnostic_mutation_projects_touched_item(
     assert result.state_changed is True
     assert saved_states
     assert synced == [[42]]
+
+
+def test_execute_run_schedule_removes_closed_pr_rows_through_lifecycle_owner(monkeypatch):
+    harness = AppHarness(monkeypatch)
+    harness.set_event(EVENT_NAME="schedule", EVENT_ACTION="")
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    saved_active_reviews = []
+    synced = []
+
+    harness.stub_lock(acquire=lambda: None, release=lambda: True)
+    harness.stub_load_state(lambda *, fail_on_unavailable=False: state)
+    harness.stub_pass_until(lambda current: (current, []))
+    harness.stub_sync_members(lambda current: (current, []))
+    harness.runtime.github.get_issue_or_pr_snapshot = lambda issue_number: {
+        "number": issue_number,
+        "state": "closed",
+        "pull_request": {},
+        "labels": [],
+    }
+    monkeypatch.setattr(maintenance_schedule, "sweep_deferred_gaps", lambda bot, current: False)
+    monkeypatch.setattr(maintenance_schedule, "check_overdue_reviews", lambda bot, current: [])
+    harness.stub_save_state(lambda current: saved_active_reviews.append(dict(current["active_reviews"])) or True)
+    harness.stub_sync_status_labels(lambda current, issue_numbers: synced.append(list(issue_numbers)) or True)
+
+    result = harness.run_execute()
+
+    assert result.exit_code == 0
+    assert saved_active_reviews == [{}]
+    assert synced == [[42]]