fix: harden reviewer-bot reconcile freshness

Author: PLeVasseur

.github/workflows/reviewer-bot-reconcile.yml

@@ -17,7 +17,6 @@ env:
 
 jobs:
   reconcile:
-    if: ${{ github.event.workflow_run.conclusion == 'success' }}
     runs-on: ubuntu-latest
     permissions:
       # Temporary lock debt: contents:write is allowed only for the existing lock-ref API operations.

scripts/reviewer_bot_lib/app.py

@@ -117,6 +117,16 @@ def record_state_issue_write_receipt(
     return receipt
 
 
+def _store_state_issue_write_receipt(state: dict, receipt: StateIssueWriteReceipt) -> bool:
+    receipts = state.setdefault("state_issue_write_receipts", {})
+    if not isinstance(receipts, dict):
+        receipts = {}
+        state["state_issue_write_receipts"] = receipts
+    key = f"{receipt.mutation_kind}:{receipt.issue_number or 'global'}:{receipt.recorded_at or 'pending'}"
+    receipts[key] = receipt.to_output()
+    return True
+
+
 def _stable_state_hash(state: dict | None) -> str | None:
     if not isinstance(state, dict):
         return None
@@ -485,6 +495,17 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                 workflow_run_result=workflow_run_result,
             )
             state_hash_after = _stable_state_hash(state)
+            success_receipt = build_state_issue_write_receipt(
+                issue_number=_primary_issue_number(context, touched_items),
+                mutation_kind=f"{event_name}:{event_action or context.manual_action or 'none'}",
+                issue_body_hash_before=loaded_state_hash,
+                issue_body_hash_after=state_hash_after,
+                external_side_effects_recorded=save_side_effects,
+                state_save_attempted=True,
+                state_save_succeeded=True,
+                recorded_at=bot.datetime.now(bot.timezone.utc).isoformat(),
+            )
+            _store_state_issue_write_receipt(state, success_receipt)
             if not bot.state_store.save_state(state):
                 record_state_issue_write_receipt(
                     bot,
@@ -501,16 +522,8 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes
                     "State updates were computed but could not be persisted. "
                     "Failing this run to avoid silent success."
                 )
-            record_state_issue_write_receipt(
-                bot,
-                issue_number=_primary_issue_number(context, touched_items),
-                mutation_kind=f"{event_name}:{event_action or context.manual_action or 'none'}",
-                issue_body_hash_before=loaded_state_hash,
-                issue_body_hash_after=state_hash_after,
-                external_side_effects_recorded=save_side_effects,
-                state_save_attempted=True,
-                state_save_succeeded=True,
-            )
+            if hasattr(bot, "logger"):
+                bot.logger.event("info", "state issue write receipt", **success_receipt.to_output())
 
             if touched_items:
                 state = bot.state_store.load_state(fail_on_unavailable=True)

scripts/reviewer_bot_lib/overdue.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 from dataclasses import dataclass
+from datetime import datetime, timezone
 
 from . import assignment_flow
 from .config import TRANSITION_NOTICE_MARKER_PREFIX, TRANSITION_WARNING_MARKER_PREFIX
@@ -253,6 +254,18 @@ def build_reminder_delivery_persistence_result(
     )
 
 
+def _parse_reminder_timestamp(value: object) -> datetime | None:
+    if isinstance(value, datetime):
+        return value if value.tzinfo is not None else value.replace(tzinfo=timezone.utc)
+    if not isinstance(value, str) or not value.strip():
+        return None
+    try:
+        timestamp = datetime.fromisoformat(value.replace("Z", "+00:00"))
+    except ValueError:
+        return None
+    return timestamp if timestamp.tzinfo is not None else timestamp.replace(tzinfo=timezone.utc)
+
+
 def derive_reminder_cadence_decision(
     response,
     *,
@@ -262,23 +275,38 @@ def derive_reminder_cadence_decision(
     review_deadline_days: int,
     transition_period_days: int,
 ) -> ReminderCadenceDecision:
-    del now, review_deadline_days, transition_period_days
     issue_number = int(getattr(response, "scope", None).issue_number or 0) if getattr(response, "scope", None) else 0
     reviewer = getattr(getattr(response, "scope", None), "reviewer", None)
     effective_receipt = receipt if receipt is not None and receipt.receipt_kind != "none" else None
     legacy_duplicate_count = reminder_scan.baseline_count if reminder_scan is not None else 0
     exhausted = (effective_receipt is not None and effective_receipt.receipt_kind in {"transition", "legacy_transition"}) or legacy_duplicate_count >= 2
+    now_dt = _parse_reminder_timestamp(now)
+    anchor_dt = _parse_reminder_timestamp(getattr(response, "anchor_timestamp", None))
+    receipt_dt = _parse_reminder_timestamp(effective_receipt.created_at if effective_receipt else None)
+    cadence_state = "exhausted" if exhausted else "not_started"
+    may_post_warning = False
+    may_post_transition = False
+    if not exhausted and getattr(response, "response_state", None) == "awaiting_reviewer_response":
+        if effective_receipt is not None and effective_receipt.receipt_kind in {"warning", "legacy_warning_or_reminder"}:
+            if now_dt is not None and receipt_dt is not None and (now_dt - receipt_dt).days >= transition_period_days:
+                cadence_state = "transition_due"
+                may_post_transition = True
+        elif effective_receipt is None and now_dt is not None and anchor_dt is not None and (now_dt - anchor_dt).days >= review_deadline_days:
+            cadence_state = "warning_due"
+            may_post_warning = True
+        elif now_dt is None or (effective_receipt is None and anchor_dt is None):
+            cadence_state = "blocked"
     return ReminderCadenceDecision(
         issue_number=issue_number,
         reviewer=reviewer,
         scope=getattr(response, "scope", None),
-        cadence_state="exhausted" if exhausted else "not_started",
+        cadence_state=cadence_state,
         exhaustion_reason="legacy_duplicate_reminders_exhausted" if legacy_duplicate_count >= 2 else "transition_notice_sent" if exhausted else "not_exhausted",
         warning_receipt=effective_receipt if effective_receipt and effective_receipt.receipt_kind in {"warning", "legacy_warning_or_reminder"} else None,
         transition_receipt=effective_receipt if effective_receipt and effective_receipt.receipt_kind in {"transition", "legacy_transition"} else None,
         legacy_duplicate_count=legacy_duplicate_count,
-        may_post_warning=not exhausted and effective_receipt is None,
-        may_post_transition=not exhausted and effective_receipt is not None and effective_receipt.receipt_kind == "warning",
+        may_post_warning=may_post_warning,
+        may_post_transition=may_post_transition,
         must_project_reassignment_needed=exhausted,
     )
 
@@ -295,10 +323,10 @@ def decide_overdue_reminder(
     if getattr(response, "response_state", None) != "awaiting_reviewer_response" or cadence.must_project_reassignment_needed:
         action = "none"
         reason = cadence.exhaustion_reason or "not_awaiting_reviewer_response"
-    elif cadence.may_post_transition:
+    elif cadence.cadence_state == "transition_due" and cadence.may_post_transition:
         action = "transition"
         reason = "transition_due"
-    elif cadence.may_post_warning:
+    elif cadence.cadence_state == "warning_due" and cadence.may_post_warning:
         action = "warning"
         reason = "warning_due"
     else:
@@ -469,16 +497,16 @@ def _effective_response_with_cadence(bot, issue_number: int, review_data: dict,
     response_payload.setdefault("issue_number", issue_number)
     response_payload.setdefault("current_reviewer", review_data.get("current_reviewer"))
     response = reviewer_response_policy.to_reviewer_response_decision(response_payload)
+    reminder_scan = _scan_live_reminder_comments(bot, issue_number)
     receipt = derive_reminder_scope_receipt(
         issue_number=issue_number,
         reviewer=getattr(response.scope, "reviewer", None) if response.scope else review_data.get("current_reviewer"),
         head_sha=getattr(response.scope, "head_sha", None) if response.scope else None,
         cycle_key=getattr(response.scope, "cycle_key", None) if response.scope else None,
         scope_key=getattr(response.scope, "scope_key", None) if response.scope else None,
         persisted_state=review_data,
-        scanned_comments=(),
+        scanned_comments=reminder_scan.records if reminder_scan is not None else (),
     )
-    reminder_scan = _scan_live_reminder_comments(bot, issue_number)
     cadence = derive_reminder_cadence_decision(
         response,
         receipt=receipt,
@@ -598,46 +626,15 @@ def evaluate_overdue_review_preview(bot, state: dict, issue_number: int) -> dict
     }
     if authority["authority_status"] != "tracked_reviewer_confirmed":
         return preview
-    if cadence.must_project_reassignment_needed:
-        return preview
-    if response_name != "awaiting_reviewer_response":
-        return preview
-    current_reviewer = review_data.get("current_reviewer")
-    anchor_timestamp = effective_response.anchor_timestamp
-    if not isinstance(current_reviewer, str) or not current_reviewer.strip() or not isinstance(anchor_timestamp, str) or not anchor_timestamp:
-        return preview
-    try:
-        now = bot.datetime.now(bot.timezone.utc)
-        anchor_dt = bot.datetime.fromisoformat(anchor_timestamp.replace("Z", "+00:00"))
-    except (ValueError, AttributeError):
-        return preview
-    transition_warning_sent = review_data.get("transition_warning_sent")
-    if isinstance(transition_warning_sent, str) and transition_warning_sent:
-        try:
-            warning_dt = bot.datetime.fromisoformat(transition_warning_sent.replace("Z", "+00:00"))
-        except (ValueError, AttributeError):
-            return preview
-        if (now - warning_dt).days < bot.TRANSITION_PERIOD_DAYS:
-            return preview
-        existing_notice = find_existing_transition_notice_result(
-            bot,
-            issue_number,
-            transition_warning_sent,
-            current_reviewer,
-        )
-        preview["would_post_transition"] = existing_notice.get("status") == "missing"
-        return preview
-    if (now - anchor_dt).days < bot.REVIEW_DEADLINE_DAYS:
-        return preview
-    existing_warning = _find_existing_warning_comment(
-        bot,
-        issue_number,
-        current_reviewer,
-        anchor_timestamp,
-        current_scope_key=current_scope_key,
-        current_scope_basis=current_scope_basis,
+    reminder_decision = decide_overdue_reminder(
+        effective_response,
+        cadence=cadence,
+        now=bot.datetime.now(bot.timezone.utc),
+        review_deadline_days=bot.REVIEW_DEADLINE_DAYS,
+        transition_period_days=bot.TRANSITION_PERIOD_DAYS,
     )
-    preview["would_post_warning"] = existing_warning.get("status") == "missing"
+    preview["would_post_warning"] = reminder_decision.action == "warning"
+    preview["would_post_transition"] = reminder_decision.action == "transition"
     return preview
 
 
@@ -661,9 +658,6 @@ def check_overdue_reviews(bot, state: dict) -> list[dict]:
         if review_data.get("review_completed_at"):
             continue
 
-        if review_data.get("transition_notice_sent_at"):
-            continue
-
         current_reviewer = review_data.get("current_reviewer")
         if not current_reviewer:
             continue
@@ -756,30 +750,27 @@ def check_overdue_reviews(bot, state: dict) -> list[dict]:
         if days_since_activity < bot.REVIEW_DEADLINE_DAYS:
             continue
 
-        transition_warning_sent = review_data.get("transition_warning_sent")
-        if transition_warning_sent:
-            try:
-                warning_dt = bot.datetime.fromisoformat(transition_warning_sent.replace("Z", "+00:00"))
-                days_since_warning = (now - warning_dt).days
-
-                if days_since_warning >= bot.TRANSITION_PERIOD_DAYS:
-                    overdue.append(
-                        {
-                            "issue_number": issue_number,
-                            "reviewer": current_reviewer,
-                            "days_overdue": days_since_activity,
-                            "days_since_warning": days_since_warning,
-                            "needs_warning": False,
-                            "needs_transition": True,
-                            "anchor_reason": anchor_reason,
-                            "anchor_timestamp": last_activity,
-                            "current_scope_key": current_scope_key,
-                            "current_scope_basis": current_scope_basis,
-                        }
-                    )
-            except (ValueError, AttributeError):
-                pass
-        else:
+        if reminder_decision.action == "transition":
+            warning_dt = _parse_reminder_timestamp(
+                reminder_decision.receipt.created_at if reminder_decision.receipt is not None else None
+            )
+            if warning_dt is None:
+                continue
+            overdue.append(
+                {
+                    "issue_number": issue_number,
+                    "reviewer": current_reviewer,
+                    "days_overdue": days_since_activity,
+                    "days_since_warning": (now - warning_dt).days,
+                    "needs_warning": False,
+                    "needs_transition": True,
+                    "anchor_reason": anchor_reason,
+                    "anchor_timestamp": last_activity,
+                    "current_scope_key": current_scope_key,
+                    "current_scope_basis": current_scope_basis,
+                }
+            )
+        elif reminder_decision.action == "warning":
             overdue.append(
                 {
                     "issue_number": issue_number,

scripts/reviewer_bot_lib/reconcile.py

@@ -790,7 +790,7 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool:
     if (
         decision.mark_reconciled
         and admission.mark_reconciled_allowed
-        and command_receipt.result in {"pass_replayed_and_persisted", "pass_diagnostic_only"}
+        and command_receipt.result == "pass_replayed_and_persisted"
     ):
         reconciled_changed = gap_bookkeeping.mark_reconciled_source_event(
             review_data,

tests/contract/reviewer_bot/test_opencode_isolation.py

@@ -0,0 +1,25 @@
+from pathlib import Path
+
+import pytest
+
+pytestmark = pytest.mark.contract
+
+
+def _shipped_behavior_files() -> list[Path]:
+    roots = [Path("scripts/reviewer_bot_core"), Path("scripts/reviewer_bot_lib"), Path(".github/workflows")]
+    files: list[Path] = []
+    for root in roots:
+        files.extend(path for path in root.rglob("*") if path.is_file() and path.suffix in {".py", ".yml", ".yaml"})
+    files.extend(path for path in Path("docs").glob("reviewer-bot-*.md") if path.is_file())
+    return sorted(files)
+
+
+def test_shipped_reviewer_bot_behavior_does_not_depend_on_opencode_tooling():
+    forbidden = ("OPENCODE_", "OPENCODE_CONFIG_DIR", "opencode-project-agents")
+    offenders: list[str] = []
+    for path in _shipped_behavior_files():
+        text = path.read_text(encoding="utf-8")
+        if any(token in text for token in forbidden):
+            offenders.append(str(path))
+
+    assert offenders == []

tests/contract/reviewer_bot/test_workflow_files.py

@@ -108,6 +108,16 @@ def test_reconcile_workflow_permissions_cover_live_replay_reads():
     assert permissions["pull-requests"] in {"read", "write"}
 
 
+def test_reconcile_workflow_does_not_gate_completed_observers_by_success():
+    workflow_text = Path(".github/workflows/reviewer-bot-reconcile.yml").read_text(encoding="utf-8")
+    workflow = yaml.safe_load(workflow_text)
+    reconcile_job = workflow["jobs"]["reconcile"]
+
+    assert "if" not in reconcile_job
+    assert "github.event.workflow_run.conclusion == 'success'" not in workflow_text
+    assert "WORKFLOW_RUN_TRIGGERING_CONCLUSION: ${{ github.event.workflow_run.conclusion }}" in workflow_text
+
+
 @pytest.mark.parametrize(
     "workflow_path",
     [
@@ -402,6 +412,17 @@ def test_workflow_policy_split_and_lock_only_boundaries():
                     else:
                         assert "@" in value and len(value.split("@", 1)[1]) == 40
 
+
+def test_reviewer_bot_workflows_use_shared_source_action_without_raw_extraction():
+    workflows_dir = Path(".github/workflows")
+    for path in sorted(workflows_dir.glob("reviewer-bot-*.yml")):
+        text = path.read_text(encoding="utf-8")
+        assert "archive.extractall(" not in text
+        assert "tar.extractall(" not in text
+        if 'uv run --project "$BOT_SRC_ROOT"' in text or 'python "$BOT_SRC_ROOT/scripts/reviewer_bot.py"' in text:
+            assert "uses: ./.github/actions/reviewer-bot-source" in text
+            assert "BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }}" in text
+
 def test_workflow_summaries_and_runbook_references_exist():
     runbook = Path("docs/reviewer-bot-review-freshness-operator-runbook.md")
     assert runbook.exists()

tests/integration/reviewer_bot/test_app_execution.py

@@ -343,6 +343,9 @@ def test_execute_run_opened_issue_adopts_existing_single_live_assignee(monkeypat
     assert result.exit_code == 0
     assert result.state_changed is True
     assert saved[-1]["active_reviews"]["42"]["current_reviewer"] == "alice"
+    receipt = next(iter(saved[-1]["state_issue_write_receipts"].values()))
+    assert receipt["write_status"] == "persisted"
+    assert receipt["state_save_succeeded"] is True
 
 def test_execute_run_returns_failure_when_save_state_fails(monkeypatch):
     harness = AppHarness(monkeypatch)
@@ -358,6 +361,9 @@ def test_execute_run_returns_failure_when_save_state_fails(monkeypatch):
 
     assert result.exit_code == 1
     assert result.state_changed is True
+    receipt_logs = [record for record in harness.runtime.logger.records if record["message"] == "state issue write receipt"]
+    assert receipt_logs[-1]["fields"]["write_status"] == "failed_after_external_side_effect"
+    assert receipt_logs[-1]["fields"]["next_recovery_action"] == "recover_from_live_github_receipt"
 
 
 def test_execute_run_releases_lock_after_save_failure(monkeypatch):

tests/integration/reviewer_bot/test_pr264_incident_replay_card.py

@@ -14,7 +14,7 @@
 pytestmark = pytest.mark.integration
 
 
-def test_pr264_canonical_replay_card_suppresses_same_scope_reviewer_activity_after_reconcile(monkeypatch):
+def test_pr264_canonical_replay_card_keeps_plain_lgtm_diagnostic_only(monkeypatch):
     state = make_state()
     review = make_tracked_review_state(
         state,
@@ -47,9 +47,10 @@ def test_pr264_canonical_replay_card_suppresses_same_scope_reviewer_activity_aft
         author_association="CONTRIBUTOR",
     )
 
-    assert harness.run(state) is True
+    assert harness.run(state) is False
     assert review["reviewer_comment"].get("accepted") is None
-    assert review["sidecars"]["reconciled_source_events"]["issue_comment:210"]["source_event_key"] == "issue_comment:210"
+    assert "issue_comment:210" not in review["sidecars"]["reconciled_source_events"]
+    assert "issue_comment:210" not in review["sidecars"]["deferred_gaps"]
 
     routes = RouteGitHubApi().add_request(
         "GET",