diff --git a/scripts/reviewer_bot_lib/bootstrap_runtime.py b/scripts/reviewer_bot_lib/bootstrap_runtime.py index 8d1865f2a..ff379d86c 100644 --- a/scripts/reviewer_bot_lib/bootstrap_runtime.py +++ b/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) diff --git a/scripts/reviewer_bot_lib/runtime.py b/scripts/reviewer_bot_lib/runtime.py index 0fc57ae5d..6748dbf8a 100644 --- a/scripts/reviewer_bot_lib/runtime.py +++ b/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( diff --git a/scripts/reviewer_bot_lib/runtime_protocols.py b/scripts/reviewer_bot_lib/runtime_protocols.py index c3a10ca1a..59bb44c26 100644 --- a/scripts/reviewer_bot_lib/runtime_protocols.py +++ b/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 diff --git a/scripts/reviewer_bot_lib/state_store.py b/scripts/reviewer_bot_lib/state_store.py index d18f6747c..13d951bd0 100644 --- a/scripts/reviewer_bot_lib/state_store.py +++ b/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 diff --git a/tests/contract/reviewer_bot/test_adapter_contract.py b/tests/contract/reviewer_bot/test_adapter_contract.py index 88a61f5fd..69ad26bfa 100644 --- a/tests/contract/reviewer_bot/test_adapter_contract.py +++ b/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") diff --git a/tests/contract/reviewer_bot/test_fake_runtime_contract.py b/tests/contract/reviewer_bot/test_fake_runtime_contract.py index 4e7ee3951..fcadb3b40 100644 --- a/tests/contract/reviewer_bot/test_fake_runtime_contract.py +++ b/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") diff --git a/tests/contract/reviewer_bot/test_runtime_protocols.py b/tests/contract/reviewer_bot/test_runtime_protocols.py index 57a46fe92..2f398e089 100644 --- a/tests/contract/reviewer_bot/test_runtime_protocols.py +++ b/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", diff --git a/tests/fixtures/fake_runtime.py b/tests/fixtures/fake_runtime.py index 15ac79a52..1dbd24937 100644 --- a/tests/fixtures/fake_runtime.py +++ b/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( diff --git a/tests/integration/reviewer_bot/test_app_execution.py b/tests/integration/reviewer_bot/test_app_execution.py index a4d051985..018736f81 100644 --- a/tests/integration/reviewer_bot/test_app_execution.py +++ b/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") diff --git a/tests/unit/reviewer_bot/test_state_store.py b/tests/unit/reviewer_bot/test_state_store.py index 66a40ef88..cc91c3c7c 100644 --- a/tests/unit/reviewer_bot/test_state_store.py +++ b/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()