fix: guard manual reviewer cleanup

Author: PLeVasseur

scripts/reviewer_bot_lib/app.py

@@ -247,7 +247,11 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                     state_changed = bot.handlers.handle_comment_event(state)
 
         elif event_name == "workflow_dispatch":
-            state_changed = bot.handlers.handle_manual_dispatch(state)
+            if context.manual_action == "check-overdue":
+                schedule_result = bot.handlers.handle_scheduled_check_result(state)
+                state_changed = schedule_result.state_changed
+            else:
+                state_changed = bot.handlers.handle_manual_dispatch(state)
 
         elif event_name == "schedule":
             schedule_result = bot.handlers.handle_scheduled_check_result(state)
@@ -295,7 +299,7 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                     "Acquire lock before mutating state."
                 )
 
-            if event_name == "schedule":
+            if schedule_result is not None:
                 current_active_reviews = state.get("active_reviews")
                 current_active_reviews_count = (
                     len(current_active_reviews) if isinstance(current_active_reviews, dict) else 0
@@ -316,7 +320,7 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                     and not allow_closed_cleanup_empty
                 ):
                     raise RuntimeError(
-                        "STATE_GUARD_BLOCKED_EMPTY_ACTIVE_REVIEWS: refusing to persist schedule "
+                        "STATE_GUARD_BLOCKED_EMPTY_ACTIVE_REVIEWS: refusing to persist maintenance "
                         f"state update that drops active_reviews from {loaded_active_reviews_count} "
                         "to 0. Set ALLOW_EMPTY_ACTIVE_REVIEWS_WRITE=true to override."
                     )

scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py

@@ -122,14 +122,18 @@ def _ensure_source_event_key(review_data: dict, source_event_key: str, payload:
     deferred_gaps[source_event_key] = payload
 
 
-def _clear_source_event_key(review_data: dict, source_event_key: str) -> bool:
+def clear_deferred_gap(review_data: dict, source_event_key: str) -> bool:
     deferred_gaps = _deferred_gaps(review_data)
     if source_event_key in deferred_gaps:
         deferred_gaps.pop(source_event_key, None)
         return True
     return False
 
 
+def _clear_source_event_key(review_data: dict, source_event_key: str) -> bool:
+    return clear_deferred_gap(review_data, source_event_key)
+
+
 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)
@@ -144,7 +148,7 @@ def _update_deferred_gap_fields(review_data: dict, source_event_key: str, fields
     return update_deferred_gap_fields(review_data, source_event_key, fields)
 
 
-def _mark_reconciled_source_event(
+def mark_reconciled_source_event(
     review_data: dict,
     source_event_key: str,
     *,
@@ -166,6 +170,19 @@ def _mark_reconciled_source_event(
     return True
 
 
+def _mark_reconciled_source_event(
+    review_data: dict,
+    source_event_key: str,
+    *,
+    reconciled_at: str | None = None,
+) -> bool:
+    return mark_reconciled_source_event(
+        review_data,
+        source_event_key,
+        reconciled_at=reconciled_at,
+    )
+
+
 def was_reconciled_source_event(review_data: dict, source_event_key: str) -> bool:
     return _is_valid_reconciled_source_event(
         _reconciled_source_events(review_data).get(source_event_key),
@@ -182,7 +199,7 @@ def _payload_or_existing(payload: dict, existing: dict, key: str):
     return existing.get(key) if value is None else value
 
 
-def _update_deferred_gap(
+def record_deferred_gap_diagnostic(
     bot,
     review_data: dict,
     payload: dict,
@@ -220,3 +237,22 @@ def _update_deferred_gap(
     changed = previous != existing
     deferred_gaps[source_event_key] = existing
     return changed
+
+
+def _update_deferred_gap(
+    bot,
+    review_data: dict,
+    payload: dict,
+    reason: str,
+    diagnostic_summary: str,
+    *,
+    failure_kind: str | None = None,
+) -> bool:
+    return record_deferred_gap_diagnostic(
+        bot,
+        review_data,
+        payload,
+        reason,
+        diagnostic_summary,
+        failure_kind=failure_kind,
+    )

scripts/reviewer_bot_lib/reconcile.py

@@ -306,7 +306,7 @@ def replay_request(comment_context: LiveCommentReplayContext | None = None, *, c
         )
 
     def record_artifact_invalid(problem: InvalidEventInput) -> bool:
-        return gap_bookkeeping._update_deferred_gap(
+        return gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             payload,
@@ -334,7 +334,7 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool:
                 changed = record_conversation_freshness(bot, state, replay_request())
             except InvalidEventInput as exc:
                 return record_artifact_invalid(exc)
-        gap_changed = gap_bookkeeping._update_deferred_gap(
+        gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             payload,
@@ -370,7 +370,7 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool:
                 changed = record_conversation_freshness(bot, state, replay_request(comment_context, comment_body=live_body))
             except InvalidEventInput as exc:
                 return record_artifact_invalid(exc)
-        gap_changed = gap_bookkeeping._update_deferred_gap(
+        gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             payload,
@@ -396,7 +396,7 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool:
         except InvalidEventInput as exc:
             return record_artifact_invalid(exc)
     if decision.failed_closed_reason is not None:
-        gap_changed = gap_bookkeeping._update_deferred_gap(
+        gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             payload,
@@ -418,14 +418,14 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool:
             return record_artifact_invalid(exc)
     reconciled_changed = False
     if decision.mark_reconciled:
-        reconciled_changed = gap_bookkeeping._mark_reconciled_source_event(
+        reconciled_changed = gap_bookkeeping.mark_reconciled_source_event(
             review_data,
             str(payload.get("source_event_key", "")),
             reconciled_at=_now_iso(bot),
         )
         gap_cleared_changed = False
         if decision.clear_gap:
-            gap_cleared_changed = gap_bookkeeping._clear_source_event_key(review_data, str(payload.get("source_event_key", "")))
+            gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, str(payload.get("source_event_key", "")))
         return changed or reconciled_changed or gap_cleared_changed
 
 
@@ -507,12 +507,12 @@ def _handle_review_submitted_workflow_run(
         state_changed = True
     if _record_review_rebuild(bot, state, pr_number, review_data):
         state_changed = True
-    reconciled_changed = gap_bookkeeping._mark_reconciled_source_event(
+    reconciled_changed = gap_bookkeeping.mark_reconciled_source_event(
         review_data,
         source_event_key,
         reconciled_at=_now_iso(bot),
     )
-    gap_cleared_changed = gap_bookkeeping._clear_source_event_key(review_data, source_event_key)
+    gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key)
     return state_changed or reconciled_changed or gap_cleared_changed
 
 
@@ -530,7 +530,7 @@ def _handle_review_dismissed_workflow_run(
     source_event_key = context.source_event_key
     dismissal_time = _resolve_review_dismissal_time(bot, context, parsed_payload.raw_payload)
     if not dismissal_time.exact:
-        return gap_bookkeeping._update_deferred_gap(
+        return gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             parsed_payload.raw_payload,
@@ -559,14 +559,14 @@ def _handle_review_dismissed_workflow_run(
     state_changed = _record_review_rebuild(bot, state, context.pr_number, review_data) or state_changed
     reconciled_changed = False
     if decision.mark_reconciled:
-        reconciled_changed = gap_bookkeeping._mark_reconciled_source_event(
+        reconciled_changed = gap_bookkeeping.mark_reconciled_source_event(
             review_data,
             source_event_key,
             reconciled_at=_now_iso(bot),
         )
     gap_cleared_changed = False
     if decision.clear_gap:
-        gap_cleared_changed = gap_bookkeeping._clear_source_event_key(review_data, source_event_key)
+        gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key)
     return state_changed or reconciled_changed or gap_cleared_changed
 
 
@@ -750,7 +750,7 @@ def _build_result(state_changed: bool, pr_number: int) -> WorkflowRunHandlerResu
     except RuntimeError as exc:
         bot.collect_touched_item(pr_number)
         failure_kind = exc.failure_kind if isinstance(exc, ReconcileReadError) else None
-        gap_changed = gap_bookkeeping._update_deferred_gap(
+        gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic(
             bot,
             review_data,
             payload,

scripts/reviewer_bot_lib/sweeper.py

@@ -263,7 +263,7 @@ def _purge_bot_authored_comment_gap(bot, review_data: dict, source_event_key: st
         return False
     if source_event_key not in gap_bookkeeping.list_deferred_gap_keys(review_data):
         return False
-    return gap_bookkeeping._clear_source_event_key(review_data, source_event_key)
+    return gap_bookkeeping.clear_deferred_gap(review_data, source_event_key)
 
 
 def _maybe_fetch_single_candidate_run_detail(bot, run_correlation: dict, artifact_correlation: dict | None) -> dict | None:
@@ -306,8 +306,8 @@ def _repair_visible_review_gap(bot, review_data: dict, issue_number: int, source
     )[0] or changed
     record_reviewer_activity(review_data, submitted_at)
     completion, _ = rebuild_pr_approval_state(bot, issue_number, review_data)
-    reconciled_changed = gap_bookkeeping._mark_reconciled_source_event(review_data, source_event_key)
-    gap_cleared_changed = gap_bookkeeping._clear_source_event_key(review_data, source_event_key)
+    reconciled_changed = gap_bookkeeping.mark_reconciled_source_event(review_data, source_event_key)
+    gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key)
     return changed or completion is not None or reconciled_changed or gap_cleared_changed
 
 
@@ -456,7 +456,7 @@ def _record_gap_diagnostics(
     reason: str,
     diagnostic_reason: str,
 ) -> None:
-    gap_bookkeeping._update_deferred_gap(
+    gap_bookkeeping.record_deferred_gap_diagnostic(
         bot,
         review_data,
         {

tests/contract/reviewer_bot/test_support_layer_ownership.py

@@ -113,10 +113,10 @@ def test_f1a_support_layer_inventory_fixture_records_candidate_classifications_a
     assert symbols["scripts.reviewer_bot_core.live_review_support.normalize_reviews_with_parsed_timestamps"]["classification"] == "retained support owner"
     assert symbols["scripts.reviewer_bot_core.live_review_support.filter_current_head_reviews_for_cycle"]["classification"] == "retained support owner"
     assert symbols["scripts.reviewer_bot_core.live_review_support.collect_permission_statuses"]["classification"] == "retained support owner"
-    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping._clear_source_event_key"]["classification"] == "retained support owner"
-    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping._mark_reconciled_source_event"]["classification"] == "retained support owner"
-    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping._update_deferred_gap"]["classification"] == "retained support owner"
-    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping._was_reconciled_source_event"]["classification"] == "retained support owner"
+    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping.clear_deferred_gap"]["classification"] == "retained support owner"
+    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping.mark_reconciled_source_event"]["classification"] == "retained support owner"
+    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping.record_deferred_gap_diagnostic"]["classification"] == "retained support owner"
+    assert symbols["scripts.reviewer_bot_lib.deferred_gap_bookkeeping.was_reconciled_source_event"]["classification"] == "retained support owner"
     assert symbols["scripts.reviewer_bot_lib.reviews.list_open_items_with_status_labels"]["classification"] == "retained support behavior"
     assert symbols["scripts.reviewer_bot_lib.reviews.rebuild_pr_approval_state"]["classification"] == "retained final surface"
     assert symbols["scripts.reviewer_bot_lib.sweeper.sweep_deferred_gaps"]["classification"] == "retained final surface"
@@ -135,10 +135,10 @@ def test_f1a_support_layer_inventory_records_transitional_importer_examples_with
         "scripts.reviewer_bot_core.live_review_support.normalize_reviews_with_parsed_timestamps",
         "scripts.reviewer_bot_core.live_review_support.filter_current_head_reviews_for_cycle",
         "scripts.reviewer_bot_core.live_review_support.collect_permission_statuses",
-        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._clear_source_event_key",
-        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._mark_reconciled_source_event",
-        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._update_deferred_gap",
-        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._was_reconciled_source_event",
+        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.clear_deferred_gap",
+        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.mark_reconciled_source_event",
+        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.record_deferred_gap_diagnostic",
+        "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.was_reconciled_source_event",
         "scripts.reviewer_bot_lib.reviews.list_open_items_with_status_labels",
         "scripts.reviewer_bot_lib.reviews.rebuild_pr_approval_state",
         "scripts.reviewer_bot_lib.reviews.rebuild_pr_approval_state_result",

tests/fixtures/equivalence/support_layer/symbol_inventory.json

@@ -155,7 +155,7 @@
       ]
     },
     {
-      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._clear_source_event_key",
+      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.clear_deferred_gap",
       "classification": "retained support owner",
       "production_importers": [
         "scripts/reviewer_bot_lib/reconcile.py",
@@ -166,7 +166,7 @@
       ]
     },
     {
-      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._mark_reconciled_source_event",
+      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.mark_reconciled_source_event",
       "classification": "retained support owner",
       "production_importers": [
         "scripts/reviewer_bot_lib/reconcile.py",
@@ -177,7 +177,7 @@
       ]
     },
     {
-      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._update_deferred_gap",
+      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.record_deferred_gap_diagnostic",
       "classification": "retained support owner",
       "production_importers": [
         "scripts/reviewer_bot_lib/reconcile.py",
@@ -188,7 +188,7 @@
       ]
     },
     {
-      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping._was_reconciled_source_event",
+      "symbol": "scripts.reviewer_bot_lib.deferred_gap_bookkeeping.was_reconciled_source_event",
       "classification": "retained support owner",
       "production_importers": [
         "scripts/reviewer_bot_lib/sweeper.py"

tests/integration/reviewer_bot/test_app_schedule_bookkeeping.py

@@ -25,7 +25,7 @@ def test_execute_run_schedule_sweeper_bookkeeping_only_mutation_still_saves_stat
     save_calls = []
 
     def fake_sweep(bot, current):
-        deferred_gap_bookkeeping._mark_reconciled_source_event(
+        deferred_gap_bookkeeping.mark_reconciled_source_event(
             current["active_reviews"]["42"],
             "pull_request_review:500",
             reconciled_at="2026-03-18T00:00:00+00:00",
@@ -381,3 +381,65 @@ def fake_schedule_result(bot, current):
 
     assert result.exit_code == 1
     assert save_called["value"] is False
+
+
+def test_execute_run_manual_check_overdue_uses_closed_cleanup_rows_for_empty_guard(monkeypatch):
+    harness = AppHarness(monkeypatch)
+    harness.set_event(EVENT_NAME="workflow_dispatch", EVENT_ACTION="", MANUAL_ACTION="check-overdue")
+    state = make_state()
+    state["status_projection_epoch"] = None
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    saved_active_reviews = []
+    synced = []
+
+    def fake_schedule_result(current):
+        current["active_reviews"].clear()
+        return maintenance.ScheduleHandlerResult(
+            True,
+            [42],
+            closed_cleanup_removed_items=(42,),
+        )
+
+    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_handler("handle_scheduled_check_result", fake_schedule_result)
+    monkeypatch.setattr(app, "collect_status_projection_repair_items", lambda bot, current: [99])
+    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, 99]]
+
+
+def test_execute_run_manual_check_overdue_empty_guard_blocks_unowned_full_drop(monkeypatch):
+    harness = AppHarness(monkeypatch)
+    harness.set_event(EVENT_NAME="workflow_dispatch", EVENT_ACTION="", MANUAL_ACTION="check-overdue")
+    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}
+
+    def fake_schedule_result(current):
+        current["active_reviews"].clear()
+        return maintenance.ScheduleHandlerResult(True, [42])
+
+    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_handler("handle_scheduled_check_result", fake_schedule_result)
+    harness.stub_save_state(lambda current: save_called.__setitem__("value", True) or True)
+    harness.stub_sync_status_labels(lambda current, issue_numbers: True)
+
+    result = harness.run_execute()
+
+    assert result.exit_code == 1
+    assert save_called["value"] is False