Corrective Stage 1 State-Save Follow-Up: align state-issue writes

scripts/reviewer_bot_lib/bootstrap_runtime.py

@@ -337,8 +337,8 @@ def clear_lock_metadata(self):
     def get_state_issue_snapshot(self):
         return state_store.get_state_issue_snapshot(self._runtime())
 
-    def conditional_patch_state_issue(self, body, etag=None):
-        return state_store.conditional_patch_state_issue(self._runtime(), body, etag)
+    def patch_state_issue(self, body):
+        return state_store.patch_state_issue(self._runtime(), body)
 
     def render_state_issue_body(self, current_state, base_body=None, *, preserve_state_block=False):
         return state_store.render_state_issue_body(current_state, base_body, preserve_state_block=preserve_state_block)

scripts/reviewer_bot_lib/runtime.py

@@ -427,8 +427,8 @@ def clear_lock_metadata(self):
     def get_state_issue_snapshot(self):
         return self.adapters.state_lock.get_state_issue_snapshot()
 
-    def conditional_patch_state_issue(self, body: str, etag: str | None = None):
-        return self.adapters.state_lock.conditional_patch_state_issue(body, etag)
+    def patch_state_issue(self, body: str):
+        return self.adapters.state_lock.patch_state_issue(body)
 
     def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
         return self.adapters.state_lock.render_state_issue_body(

scripts/reviewer_bot_lib/runtime_protocols.py

@@ -261,7 +261,7 @@ def get_state_issue(self) -> dict | None: ...
 
     def get_state_issue_snapshot(self) -> StateIssueSnapshot | None: ...
 
-    def conditional_patch_state_issue(self, body: str, etag: str | None = None) -> GitHubApiResult: ...
+    def patch_state_issue(self, body: str) -> GitHubApiResult: ...
 
 
 @runtime_checkable

scripts/reviewer_bot_lib/state_store.py

@@ -293,14 +293,12 @@ def get_state_issue_snapshot(bot: StateStoreContext) -> StateIssueSnapshot | Non
     return StateIssueSnapshot(body=body, etag=response.headers.get("etag"), html_url=html_url)
 
 
-def conditional_patch_state_issue(bot: StateStoreContext, body: str, etag: str | None = None):
+def patch_state_issue(bot: StateStoreContext, body: str):
     state_issue_number = _state_issue_number(bot)
-    extra_headers = {"If-Match": etag} if isinstance(etag, str) and etag else None
     return bot.github_api_request(
         "PATCH",
         f"issues/{state_issue_number}",
         {"body": body},
-        extra_headers=extra_headers,
         suppress_error_log=True,
     )
 
@@ -379,24 +377,11 @@ def save_state(bot: StateStoreContext, state: dict) -> bool:
 
         body = bot.render_state_issue_body(state, snapshot.body)
 
-        response = bot.conditional_patch_state_issue(body, snapshot.etag)
+        response = bot.patch_state_issue(body)
         if response.status_code == 200:
             _log(bot, "info", f"State saved to issue #{state_issue_number}", state_issue_number=state_issue_number)
             return True
 
-        if response.status_code in {409, 412}:
-            _log(
-                bot,
-                "warning",
-                f"State save hit conflict (status {response.status_code}); retrying ({attempt}/{lock_api_retry_limit})",
-                state_issue_number=state_issue_number,
-                status_code=response.status_code,
-                retry_attempt=attempt,
-            )
-            delay = _retry_delay(bot, lock_retry_base_seconds, attempt)
-            _sleep(bot, delay)
-            continue
-
         if response.status_code == 404:
             _log(bot, "error", f"State issue #{state_issue_number} not found during save_state", state_issue_number=state_issue_number)
             return False

tests/contract/reviewer_bot/test_adapter_contract.py

@@ -544,6 +544,8 @@ def test_bootstrap_runtime_wires_explicit_adapter_services():
     assert hasattr(runtime.adapters.commands, "handle_pass_command")
     assert hasattr(runtime.adapters.queue, "get_next_reviewer")
     assert hasattr(runtime.adapters.state_lock, "assert_lock_held")
+    assert hasattr(runtime.adapters.state_lock, "patch_state_issue")
+    assert hasattr(runtime.adapters.state_lock, "conditional_patch_state_issue") is False
     assert hasattr(runtime.adapters.state_lock, "render_state_issue_body")
 
 

tests/contract/reviewer_bot/test_fake_runtime_contract.py

@@ -164,7 +164,8 @@ def test_fake_runtime_exposes_retained_runtime_labels_and_lock_helpers(monkeypat
     assert hasattr(runtime, "normalize_lock_metadata")
     assert hasattr(runtime, "get_state_issue")
     assert hasattr(runtime, "get_state_issue_snapshot")
-    assert hasattr(runtime, "conditional_patch_state_issue")
+    assert hasattr(runtime, "patch_state_issue")
+    assert hasattr(runtime, "conditional_patch_state_issue") is False
     assert hasattr(runtime, "render_state_issue_body")
     assert hasattr(runtime, "get_lock_ref_snapshot")
     assert hasattr(runtime, "renew_state_issue_lease_lock")

tests/contract/reviewer_bot/test_runtime_protocols.py

@@ -39,6 +39,7 @@ def _assert_core_runtime_surface(runtime) -> None:
     assert runtime.domain.handlers is runtime.handlers
 
     for helper_name in (
+        "conditional_patch_state_issue",
         "project_status_labels_for_item",
         "sync_status_labels",
         "add_label_with_status",

tests/fixtures/fake_runtime.py

@@ -213,7 +213,7 @@ def __init__(self, runtime: "FakeReviewerBotRuntime"):
                 "get_state_issue",
                 "clear_lock_metadata",
                 "get_state_issue_snapshot",
-                "conditional_patch_state_issue",
+                "patch_state_issue",
                 "render_state_issue_body",
                 "get_state_issue_html_url",
                 "get_lock_ref_display",
@@ -402,8 +402,8 @@ def clear_lock_metadata(self):
     def get_state_issue_snapshot(self):
         return state_store_module.get_state_issue_snapshot(self._runtime)
 
-    def conditional_patch_state_issue(self, body: str, etag: str | None = None):
-        return state_store_module.conditional_patch_state_issue(self._runtime, body, etag)
+    def patch_state_issue(self, body: str):
+        return state_store_module.patch_state_issue(self._runtime, body)
 
     def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
         return state_store_module.render_state_issue_body(state, base_body, preserve_state_block=preserve_state_block)
@@ -620,8 +620,8 @@ def clear_lock_metadata(self):
     def get_state_issue_snapshot(self):
         return self.compat.state_lock.get_state_issue_snapshot()
 
-    def conditional_patch_state_issue(self, body: str, etag: str | None = None):
-        return self.compat.state_lock.conditional_patch_state_issue(body, etag)
+    def patch_state_issue(self, body: str):
+        return self.compat.state_lock.patch_state_issue(body)
 
     def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
         return self.compat.state_lock.render_state_issue_body(

tests/integration/reviewer_bot/test_app_execution.py

@@ -1,6 +1,6 @@
 import json
 from pathlib import Path
-from urllib.parse import unquote
+from urllib.parse import unquote, urlparse
 
 import pytest
 
@@ -10,10 +10,13 @@
     maintenance,
     maintenance_schedule,
     reconcile,
+    state_store,
 )
 from scripts.reviewer_bot_lib.config import STATUS_AWAITING_REVIEWER_RESPONSE_LABEL
 from tests.fixtures.app_harness import AppHarness
+from tests.fixtures.http_responses import FakeGitHubResponse
 from tests.fixtures.reviewer_bot import make_state, make_tracked_review_state
+from tests.fixtures.reviewer_bot_fakes import RouteGitHubApi, github_result
 
 pytestmark = pytest.mark.integration
 
@@ -61,6 +64,27 @@ def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs
     return runtime, label_ops
 
 
+def _route_bootstrapped_rest_request(routes: RouteGitHubApi):
+    def request(method, url, *, headers=None, json_data=None, timeout_seconds=None):
+        parts = urlparse(url).path.strip("/").split("/")
+        endpoint = "/".join(parts[3:])
+        result = routes.github_api_request(
+            method,
+            endpoint,
+            data=json_data,
+            extra_headers=headers,
+            timeout_seconds=timeout_seconds,
+        )
+        return FakeGitHubResponse(
+            result.status_code or 0,
+            payload=result.payload,
+            text=result.text,
+            headers=result.headers,
+        )
+
+    return request
+
+
 def test_app_harness_exposes_focused_runtime_services(monkeypatch):
     harness = AppHarness(monkeypatch)
 
@@ -400,6 +424,83 @@ def test_bootstrapped_runtime_workflow_dispatch_check_overdue_preserves_touched_
     assert runtime.ACTIVE_LEASE_CONTEXT is None
 
 
+def test_bootstrapped_runtime_workflow_dispatch_check_overdue_uses_real_save_state_without_conditional_issue_headers(
+    monkeypatch,
+):
+    runtime = reviewer_bot._runtime_bot()
+    state = make_state()
+    state["status_projection_epoch"] = runtime.STATUS_PROJECTION_EPOCH
+    issue_number = "314"
+    issue_body = state_store.render_state_issue_body(state)
+    routes = RouteGitHubApi().add_request_sequence(
+        "GET",
+        f"issues/{issue_number}",
+        [
+            github_result(
+                200,
+                {"body": issue_body, "html_url": f"https://example.com/issues/{issue_number}"},
+                headers={"ETag": '"etag-1"'},
+            ),
+            github_result(
+                200,
+                {"body": issue_body, "html_url": f"https://example.com/issues/{issue_number}"},
+                headers={"ETag": '"etag-2"'},
+            ),
+        ],
+    ).add_request(
+        "PATCH",
+        f"issues/{issue_number}",
+        status_code=200,
+        payload={"body": "updated"},
+    )
+
+    def acquire_lock():
+        runtime.ACTIVE_LEASE_CONTEXT = object()
+        return runtime.ACTIVE_LEASE_CONTEXT
+
+    def release_lock():
+        runtime.ACTIVE_LEASE_CONTEXT = None
+        return True
+
+    def handle_scheduled_check_result(bot, current_state):
+        del bot
+        current_state["current_index"] = 1
+        return maintenance.ScheduleHandlerResult(True, [])
+
+    monkeypatch.setattr(runtime.locks, "acquire", acquire_lock)
+    monkeypatch.setattr(runtime.locks, "release", release_lock)
+    monkeypatch.setattr(runtime.locks, "refresh", lambda: True)
+    monkeypatch.setattr(runtime.rest_transport, "request", _route_bootstrapped_rest_request(routes))
+    monkeypatch.setattr(runtime.adapters.workflow, "process_pass_until_expirations", lambda current_state: (current_state, []))
+    monkeypatch.setattr(runtime.adapters.workflow, "sync_members_with_queue", lambda current_state: (current_state, []))
+    monkeypatch.setattr(maintenance_schedule, "handle_scheduled_check_result", handle_scheduled_check_result)
+    monkeypatch.setenv("EVENT_NAME", "workflow_dispatch")
+    monkeypatch.setenv("EVENT_ACTION", "")
+    monkeypatch.setenv("MANUAL_ACTION", "check-overdue")
+    monkeypatch.setenv("GITHUB_TOKEN", "token")
+    monkeypatch.setenv("REPO_OWNER", "rustfoundation")
+    monkeypatch.setenv("REPO_NAME", "safety-critical-rust-coding-guidelines")
+    monkeypatch.setenv("STATE_ISSUE_NUMBER", issue_number)
+
+    result = reviewer_bot.execute_run(reviewer_bot.build_event_context(runtime), runtime)
+
+    assert result.exit_code == 0
+    assert result.state_changed is True
+    assert runtime.ACTIVE_LEASE_CONTEXT is None
+    assert sum(
+        1
+        for call in routes.request_calls
+        if call.method == "GET" and call.endpoint == f"issues/{issue_number}"
+    ) >= 2
+    patch_call = routes.request_calls[-1]
+    assert patch_call.method == "PATCH"
+    assert patch_call.endpoint == f"issues/{issue_number}"
+    assert patch_call.data is not None
+    assert "current_index: 1" in patch_call.data["body"]
+    assert patch_call.extra_headers is not None
+    assert all(header_name.lower() != "if-match" for header_name in patch_call.extra_headers)
+
+
 def test_d4a_app_branch_to_phase_map_is_frozen_pre_edit():
     app_text = Path("scripts/reviewer_bot_lib/app.py").read_text(encoding="utf-8")
 

tests/unit/reviewer_bot/test_state_store.py

@@ -80,7 +80,7 @@ def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
     assert observed["retry_policy"] == "idempotent_read"
 
 
-def test_conditional_patch_state_issue_sends_if_match_header(monkeypatch):
+def test_patch_state_issue_uses_plain_issue_write_request(monkeypatch):
     observed = {}
 
     def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
@@ -98,26 +98,12 @@ def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
 
     bot = _bot(monkeypatch, STATE_ISSUE_NUMBER=1, github_api_request=fake_request)
 
-    state_store.conditional_patch_state_issue(bot, "updated", '"etag-1"')
-
-    assert observed["extra_headers"] == {"If-Match": '"etag-1"'}
-
-
-def test_conditional_patch_state_issue_omits_if_match_when_etag_missing(monkeypatch):
-    observed = {}
-
-    def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
-        observed["extra_headers"] = extra_headers
-        return GitHubApiResult(200, {"body": data["body"]}, {}, "ok", True, None, 0, None)
-
-    bot = _bot(monkeypatch, STATE_ISSUE_NUMBER=1, github_api_request=fake_request)
-
-    state_store.conditional_patch_state_issue(bot, "updated", None)
+    state_store.patch_state_issue(bot, "updated")
 
     assert observed["extra_headers"] is None
 
 
-def test_save_state_retries_precondition_failed_conflict_uses_injected_time_services(monkeypatch):
+def test_save_state_retries_retryable_write_failure_uses_injected_time_services(monkeypatch):
     state = make_state()
     snapshot = StateIssueSnapshot(
         body="body",
@@ -126,7 +112,7 @@ def test_save_state_retries_precondition_failed_conflict_uses_injected_time_serv
     )
     responses = iter(
         [
-            GitHubApiResult(412, {"message": "precondition failed"}, {}, "precondition failed", False, None, 0, None),
+            GitHubApiResult(502, {"message": "bad gateway"}, {}, "bad gateway", False, None, 0, None),
             GitHubApiResult(200, {"body": "updated"}, {}, "ok", True, None, 0, None),
         ]
     )
@@ -141,9 +127,8 @@ def test_save_state_retries_precondition_failed_conflict_uses_injected_time_serv
     bot.ACTIVE_LEASE_CONTEXT = object()
     bot.locks.stub(refresh=lambda: True)
     bot.get_state_issue_snapshot = lambda: snapshot
-    bot.parse_lock_metadata_from_issue_body = lambda body: {}
     bot.render_state_issue_body = lambda state_obj, base_body: "updated"
-    bot.conditional_patch_state_issue = lambda body, etag=None: next(responses)
+    bot.patch_state_issue = lambda body: next(responses)
 
     assert state_store.save_state(bot, state) is True
     assert state["last_updated"] == clock.now().isoformat()