refactor: close retained reviewer residue (#565)

* refactor: close retained reviewer residue

* fix: require exact review dismissal time

* test: align closed arrival expectations

* fix: suppress closed review replays

* fix: close reviewer cleanup gaps

* fix: tighten reviewer cleanup ownership

* fix: harden reviewer cleanup metadata

* fix: guard manual reviewer cleanup

* fix: keep sweeper diagnostics nonmutating

* refactor: tighten deferred gap ownership

* refactor: simplify deferred maintenance boundaries

Author: PLeVasseur

docs/reviewer-bot-review-freshness-operator-runbook.md

@@ -108,4 +108,5 @@ Do not emit `artifact_expired` unless prior visibility or documented retention p
 - Missing runs are not proof that nothing happened.
 - Operator-visible workflow summaries should point to this file path: `docs/reviewer-bot-review-freshness-operator-runbook.md`.
 - Same-repo trusted-direct PR comment handling is limited to repo-associated `User` principals with `OWNER`, `MEMBER`, or `COLLABORATOR` association in the dedicated trusted workflow.
-- Non-human automation PR comments, reviewer-bot self-comments, cross-repo PR comments, Dependabot PR comments, and all PR review freshness events stay deferred or ignored in this rollout.
+- Non-human automation PR comments, reviewer-bot self-comments, cross-repo PR comments, Dependabot PR comments, and PR review freshness events are handled only by trusted observer and reconcile paths in this rollout.
+- Deferred review dismissal replay requires exact source time from the observer payload or an exact `review_dismissed` timeline event matched by review id; otherwise it remains diagnostic-only and must not be replayed manually.

scripts/reviewer_bot_core/deferred_gap_diagnosis.py

@@ -259,7 +259,7 @@ def evaluate_deferred_gap_state(
     return "artifact_invalid", str(artifact_correlation.get("reason") or "artifact_invalid")
 
 
-def recommend_visible_review_repair(
+def describe_visible_review_submission(
     review_data: dict,
     review: dict,
     source_event_key: str,
@@ -284,10 +284,14 @@ def recommend_visible_review_repair(
         return None
     if source_event_key != f"pull_request_review:{review_id}":
         return None
-    return author, submitted_at, commit_id
+    return {
+        "author": author,
+        "submitted_at": submitted_at,
+        "commit_id": commit_id,
+    }
 
 
-def recommend_review_submission_gap_repair(
+def describe_review_submission_gap_diagnostic(
     review_data: dict,
     review: dict | None,
     source_event_key: str,
@@ -297,20 +301,15 @@ def recommend_review_submission_gap_repair(
 ) -> dict[str, object] | None:
     if review is None or artifact_status == "exact_artifact_match":
         return None
-    repair = recommend_visible_review_repair(
+    visible_review = describe_visible_review_submission(
         review_data,
         review,
         source_event_key,
         current_cycle_boundary=current_cycle_boundary,
     )
-    if repair is None:
+    if visible_review is None:
         return None
-    author, submitted_at, commit_id = repair
     return {
-        "category": "review_submission_repair",
-        "payload": {
-            "author": author,
-            "submitted_at": submitted_at,
-            "commit_id": commit_id,
-        },
+        "category": "visible_review_without_replay_artifact",
+        "payload": visible_review,
     }

scripts/reviewer_bot_core/state_adapters.py

@@ -103,6 +103,8 @@ def ensure_sidecar_subtree(review_entry: dict[str, Any], *, state_last_updated:
         sidecars = {}
         review_entry["sidecars"] = sidecars
 
+    # Codec boundary exception: runtime sidecar mutation belongs to
+    # deferred_gap_bookkeeping, but persisted legacy shapes are normalized here.
     sidecars["pending_privileged_commands"] = (
         deepcopy(sidecars.get("pending_privileged_commands"))
         if isinstance(sidecars.get("pending_privileged_commands"), dict)

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:
@@ -241,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 maintenance.is_schedule_like_manual_action(context.manual_action):
+                schedule_result = bot.handlers.handle_manual_dispatch_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)
@@ -289,21 +299,28 @@ 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
                 )
                 allow_empty_override = (
                     bot.get_config_value("ALLOW_EMPTY_ACTIVE_REVIEWS_WRITE").strip().lower() == "true"
                 )
+                allow_closed_cleanup_empty = (
+                    schedule_result is not None
+                    and loaded_active_reviews_count == len(loaded_active_review_numbers)
+                    and bool(loaded_active_review_numbers)
+                    and set(schedule_result.closed_cleanup_removed_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 "
+                        "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/bootstrap_runtime.py

@@ -187,6 +187,9 @@ def handle_comment_event(self, current_state):
     def handle_manual_dispatch(self, current_state):
         return maintenance.handle_manual_dispatch(self._runtime_getter(), current_state)
 
+    def handle_manual_dispatch_result(self, current_state):
+        return maintenance.handle_manual_dispatch_result(self._runtime_getter(), current_state)
+
     def handle_scheduled_check_result(self, current_state):
         return maintenance.handle_scheduled_check_result(self._runtime_getter(), current_state)
 

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,
@@ -192,20 +191,21 @@ def handle_comment_event(
         return False
     if route == comment_routing_policy.ProcessingTarget.ISSUE_DIRECT:
         if comment_request.issue_state == "closed":
-            removed = state.get("active_reviews", {}).pop(str(issue_number), None)
             _log(
                 bot,
                 "info",
                 f"Ignoring direct comment on closed issue #{issue_number}",
                 issue_number=issue_number,
                 issue_state=comment_request.issue_state,
             )
-            return removed is not None
+            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")