fix: keep sweeper diagnostics nonmutating

Author: PLeVasseur

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_lib/deferred_gap_bookkeeping.py

@@ -71,32 +71,46 @@ def ensure_observer_discovery_watermark(review_data: dict, surface: str) -> dict
     return current
 
 
-def list_deferred_gap_keys(review_data: dict) -> list[str]:
-    return list(_deferred_gaps(review_data))
+def _observer_now_iso(bot) -> str:
+    return _now_iso(bot)
+
+
+def record_observer_watermark_event(bot, review_data: dict, surface: str, event_time: str, event_id: str) -> None:
+    current = ensure_observer_discovery_watermark(review_data, surface)
+    current.update(
+        {
+            "last_scan_started_at": current.get("last_scan_started_at") or _observer_now_iso(bot),
+            "last_scan_completed_at": _observer_now_iso(bot),
+            "last_safe_event_time": event_time,
+            "last_safe_event_id": event_id,
+            "lookback_seconds": bot.DEFERRED_DISCOVERY_OVERLAP_SECONDS if hasattr(bot, "DEFERRED_DISCOVERY_OVERLAP_SECONDS") else 3600,
+            "bootstrap_window_seconds": bot.DEFERRED_DISCOVERY_BOOTSTRAP_WINDOW_SECONDS if hasattr(bot, "DEFERRED_DISCOVERY_BOOTSTRAP_WINDOW_SECONDS") else 604800,
+            "bootstrap_completed_at": current.get("bootstrap_completed_at") or _observer_now_iso(bot),
+        }
+    )
 
 
-def _deferred_gap_keys(review_data: dict) -> list[str]:
-    return list_deferred_gap_keys(review_data)
+def record_observer_watermark_empty_scan(bot, review_data: dict, surface: str) -> None:
+    watermark = ensure_observer_discovery_watermark(review_data, surface)
+    watermark["last_scan_started_at"] = watermark.get("last_scan_started_at") or _observer_now_iso(bot)
+    watermark["last_scan_completed_at"] = _observer_now_iso(bot)
+    watermark["bootstrap_completed_at"] = watermark.get("bootstrap_completed_at") or _observer_now_iso(bot)
+
+
+def list_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 _get_deferred_gap(review_data: dict, source_event_key: str) -> dict:
-    return get_deferred_gap(review_data, source_event_key)
-
-
 def get_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 _deferred_gap_reason(review_data: dict, source_event_key: str) -> str | None:
-    return get_deferred_gap_reason(review_data, source_event_key)
-
-
 def _now_iso(bot) -> str:
     return bot.clock.now().isoformat()
 
@@ -130,7 +144,9 @@ def clear_deferred_gap(review_data: dict, source_event_key: str) -> bool:
     return False
 
 
-def _clear_source_event_key(review_data: dict, source_event_key: str) -> bool:
+def clear_automation_comment_gap(review_data: dict, source_event_key: str) -> bool:
+    if not source_event_key.startswith("issue_comment:"):
+        return False
     return clear_deferred_gap(review_data, source_event_key)
 
 
@@ -144,10 +160,6 @@ def update_deferred_gap_fields(review_data: dict, source_event_key: str, fields:
     return previous != existing
 
 
-def _update_deferred_gap_fields(review_data: dict, source_event_key: str, fields: dict) -> bool:
-    return update_deferred_gap_fields(review_data, source_event_key, fields)
-
-
 def mark_reconciled_source_event(
     review_data: dict,
     source_event_key: str,
@@ -170,35 +182,27 @@ 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),
         source_event_key,
     )
 
 
-def _was_reconciled_source_event(review_data: dict, source_event_key: str) -> bool:
-    return was_reconciled_source_event(review_data, source_event_key)
-
-
 def _payload_or_existing(payload: dict, existing: dict, key: str):
     value = payload.get(key)
     return existing.get(key) if value is None else value
 
 
+def _source_event_created_at(payload: dict, existing: dict):
+    return (
+        payload.get("source_created_at")
+        or payload.get("source_submitted_at")
+        or payload.get("source_dismissed_at")
+        or existing.get("source_event_created_at")
+    )
+
+
 def record_deferred_gap_diagnostic(
     bot,
     review_data: dict,
@@ -216,43 +220,26 @@ def record_deferred_gap_diagnostic(
     if not isinstance(existing, dict):
         existing = {}
     previous = deepcopy(existing)
-    existing.update(
-        {
-            "source_event_key": source_event_key,
-            "source_event_kind": f"{payload.get('source_event_name')}:{payload.get('source_event_action')}",
-            "pr_number": payload.get("pr_number"),
-            "reason": reason,
-            "source_event_created_at": payload.get("source_created_at") or payload.get("source_submitted_at"),
-            "source_run_id": _payload_or_existing(payload, existing, "source_run_id"),
-            "source_run_attempt": _payload_or_existing(payload, existing, "source_run_attempt"),
-            "source_workflow_file": _payload_or_existing(payload, existing, "source_workflow_file"),
-            "source_artifact_name": _payload_or_existing(payload, existing, "source_artifact_name"),
-            "first_noted_at": existing.get("first_noted_at") or _now_iso(bot),
-            "last_checked_at": _now_iso(bot),
-            "operator_action_required": True,
-            "diagnostic_summary": diagnostic_summary,
-            "failure_kind": failure_kind,
-        }
-    )
+    fields = {
+        "source_event_key": source_event_key,
+        "source_event_kind": f"{payload.get('source_event_name')}:{payload.get('source_event_action')}",
+        "pr_number": payload.get("pr_number"),
+        "reason": reason,
+        "source_event_created_at": _source_event_created_at(payload, existing),
+        "source_run_id": _payload_or_existing(payload, existing, "source_run_id"),
+        "source_run_attempt": _payload_or_existing(payload, existing, "source_run_attempt"),
+        "source_workflow_file": _payload_or_existing(payload, existing, "source_workflow_file"),
+        "source_artifact_name": _payload_or_existing(payload, existing, "source_artifact_name"),
+        "first_noted_at": existing.get("first_noted_at") or _now_iso(bot),
+        "last_checked_at": _now_iso(bot),
+        "operator_action_required": True,
+        "diagnostic_summary": diagnostic_summary,
+        "failure_kind": failure_kind,
+    }
+    source_dismissed_at = _payload_or_existing(payload, existing, "source_dismissed_at")
+    if source_dismissed_at is not None:
+        fields["source_dismissed_at"] = source_dismissed_at
+    existing.update(fields)
     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/maintenance.py

@@ -93,10 +93,7 @@ def handle_manual_dispatch(bot, state: dict) -> bool:
             bot.collect_touched_item(issue_number)
         return False
     if action == "check-overdue":
-        result = maintenance_schedule.handle_scheduled_check_result(bot, state)
-        for issue_number in result.touched_items:
-            bot.collect_touched_item(issue_number)
-        return result.state_changed
+        raise RuntimeError("manual check-overdue must be dispatched through handle_scheduled_check_result")
     if action == "execute-pending-privileged-command":
         source_event_key = request.privileged_source_event_key
         if not source_event_key:

scripts/reviewer_bot_lib/sweeper.py

@@ -14,13 +14,9 @@
 from . import sweeper_observer_correlation as observer_correlation
 from .config import REVIEW_FRESHNESS_RUNBOOK_PATH
 from .review_state import (
-    accept_reviewer_review_from_live_review,
     get_current_cycle_boundary,
-    record_reviewer_activity,
-    refresh_reviewer_review_from_live_preferred_review,
     semantic_key_seen,
 )
-from .reviews import rebuild_pr_approval_state
 from .runtime_protocols import SweeperContext
 
 
@@ -93,7 +89,6 @@ def _sleep(bot: SweeperContext, seconds: float) -> None:
 _fetch_workflow_runs_for_file = observer_correlation.fetch_workflow_runs_for_file
 _fetch_run_detail = observer_correlation.fetch_run_detail
 inspect_run_artifact_payloads = observer_correlation.inspect_run_artifact_payloads
-_update_observer_watermark = observer_correlation.update_observer_watermark
 
 
 def _complete_surface_scan(bot, review_data: dict, surface: str, discovered: list[dict]) -> None:
@@ -102,12 +97,9 @@ def _complete_surface_scan(bot, review_data: dict, surface: str, discovered: lis
         review_data,
         surface,
         discovered,
-        _load_surface_watermark,
     )
 
 
-
-
 def _diagnose_deferred_event(
     bot,
     review_data: dict,
@@ -263,7 +255,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_deferred_gap(review_data, source_event_key)
+    return gap_bookkeeping.clear_automation_comment_gap(review_data, source_event_key)
 
 
 def _maybe_fetch_single_candidate_run_detail(bot, run_correlation: dict, artifact_correlation: dict | None) -> dict | None:
@@ -284,33 +276,6 @@ def _maybe_fetch_single_candidate_run_detail(bot, run_correlation: dict, artifac
     return _fetch_run_detail(bot, run_id)
 
 
-def _repair_visible_review_gap(bot, review_data: dict, issue_number: int, source_event_key: str, review: dict) -> bool:
-    repair = deferred_gap_diagnosis.recommend_review_submission_gap_repair(
-        review_data,
-        review,
-        source_event_key,
-        artifact_status=None,
-        current_cycle_boundary=get_current_cycle_boundary(bot, review_data),
-    )
-    if repair is None:
-        return False
-    payload = repair["payload"]
-    author = str(payload["author"])
-    submitted_at = str(payload["submitted_at"])
-    changed = accept_reviewer_review_from_live_review(review_data, review, actor=author)
-    changed = refresh_reviewer_review_from_live_preferred_review(
-        bot,
-        issue_number,
-        review_data,
-        actor=author,
-    )[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_deferred_gap(review_data, source_event_key)
-    return changed or completion is not None or reconciled_changed or gap_cleared_changed
-
-
 def _discover_visible_comment_events(bot, issue_number: int, review_data: dict) -> tuple[list[dict] | None, bool]:
     watermark = _load_surface_watermark(review_data, "comments")
     watermark["last_scan_started_at"] = _now_iso()
@@ -585,22 +550,13 @@ def sweep_deferred_gaps(bot, state: dict) -> bool:
                     run_detail = _maybe_fetch_single_candidate_run_detail(bot, run_correlation, artifact_correlation)
                 review_payload = discovered.get("review") if isinstance(discovered.get("review"), dict) else None
                 artifact_status = artifact_correlation.get("status") if isinstance(artifact_correlation, dict) else None
-                repair_recommendation = deferred_gap_diagnosis.recommend_review_submission_gap_repair(
+                visible_review_diagnostic = deferred_gap_diagnosis.describe_review_submission_gap_diagnostic(
                     review_data,
                     review_payload,
                     source_event_key,
                     artifact_status=artifact_status,
                     current_cycle_boundary=get_current_cycle_boundary(bot, review_data),
                 )
-                if repair_recommendation is not None and _repair_visible_review_gap(
-                    bot,
-                    review_data,
-                    issue_number,
-                    source_event_key,
-                    review_payload,
-                ):
-                    changed = True
-                    continue
                 reason, diagnostic_reason = deferred_gap_diagnosis.evaluate_deferred_gap_state(
                     {
                         **existing_gap,
@@ -611,6 +567,8 @@ def sweep_deferred_gaps(bot, state: dict) -> bool:
                     artifact_correlation,
                     runbook_signature=_approval_pending_signature_from_runbook(),
                 )
+                if visible_review_diagnostic is not None:
+                    diagnostic_reason = f"{diagnostic_reason}; {visible_review_diagnostic['category']}"
                 _record_gap_diagnostics(
                     bot,
                     review_data,
@@ -626,15 +584,17 @@ def sweep_deferred_gaps(bot, state: dict) -> bool:
                     reason=reason,
                     diagnostic_reason=diagnostic_reason,
                 )
+                if visible_review_diagnostic is not None:
+                    gap_bookkeeping.update_deferred_gap_fields(
+                        review_data,
+                        source_event_key,
+                        {"visible_review_diagnostic": visible_review_diagnostic},
+                    )
                 changed = True
             if discovered_reviews:
-                last_review = discovered_reviews[-1]
-                _update_observer_watermark(bot, review_data, "reviews_submitted", last_review["source_created_at"], last_review["object_id"])
+                _complete_surface_scan(bot, review_data, "reviews_submitted", discovered_reviews)
             else:
-                watermark = _load_surface_watermark(review_data, "reviews_submitted")
-                watermark["last_scan_started_at"] = watermark.get("last_scan_started_at") or _now_iso()
-                watermark["last_scan_completed_at"] = _now_iso()
-                watermark["bootstrap_completed_at"] = watermark.get("bootstrap_completed_at") or _now_iso()
+                gap_bookkeeping.record_observer_watermark_empty_scan(bot, review_data, "reviews_submitted")
         discovered_review_comments, review_comments_complete = _discover_visible_review_comment_events(bot, issue_number, review_data)
         if review_comments_complete and isinstance(discovered_review_comments, list):
             for discovered in discovered_review_comments: