Skip to content

Commit c158011

Browse files
authored
fix(reviewer-bot): align state issue saves with supported writes (#553)
1 parent 74c8f6d commit c158011

10 files changed

Lines changed: 124 additions & 49 deletions

File tree

scripts/reviewer_bot_lib/bootstrap_runtime.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,8 @@ def clear_lock_metadata(self):
337337
def get_state_issue_snapshot(self):
338338
return state_store.get_state_issue_snapshot(self._runtime())
339339

340-
def conditional_patch_state_issue(self, body, etag=None):
341-
return state_store.conditional_patch_state_issue(self._runtime(), body, etag)
340+
def patch_state_issue(self, body):
341+
return state_store.patch_state_issue(self._runtime(), body)
342342

343343
def render_state_issue_body(self, current_state, base_body=None, *, preserve_state_block=False):
344344
return state_store.render_state_issue_body(current_state, base_body, preserve_state_block=preserve_state_block)

scripts/reviewer_bot_lib/runtime.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -427,8 +427,8 @@ def clear_lock_metadata(self):
427427
def get_state_issue_snapshot(self):
428428
return self.adapters.state_lock.get_state_issue_snapshot()
429429

430-
def conditional_patch_state_issue(self, body: str, etag: str | None = None):
431-
return self.adapters.state_lock.conditional_patch_state_issue(body, etag)
430+
def patch_state_issue(self, body: str):
431+
return self.adapters.state_lock.patch_state_issue(body)
432432

433433
def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
434434
return self.adapters.state_lock.render_state_issue_body(

scripts/reviewer_bot_lib/runtime_protocols.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ def get_state_issue(self) -> dict | None: ...
261261

262262
def get_state_issue_snapshot(self) -> StateIssueSnapshot | None: ...
263263

264-
def conditional_patch_state_issue(self, body: str, etag: str | None = None) -> GitHubApiResult: ...
264+
def patch_state_issue(self, body: str) -> GitHubApiResult: ...
265265

266266

267267
@runtime_checkable

scripts/reviewer_bot_lib/state_store.py

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -293,14 +293,12 @@ def get_state_issue_snapshot(bot: StateStoreContext) -> StateIssueSnapshot | Non
293293
return StateIssueSnapshot(body=body, etag=response.headers.get("etag"), html_url=html_url)
294294

295295

296-
def conditional_patch_state_issue(bot: StateStoreContext, body: str, etag: str | None = None):
296+
def patch_state_issue(bot: StateStoreContext, body: str):
297297
state_issue_number = _state_issue_number(bot)
298-
extra_headers = {"If-Match": etag} if isinstance(etag, str) and etag else None
299298
return bot.github_api_request(
300299
"PATCH",
301300
f"issues/{state_issue_number}",
302301
{"body": body},
303-
extra_headers=extra_headers,
304302
suppress_error_log=True,
305303
)
306304

@@ -379,24 +377,11 @@ def save_state(bot: StateStoreContext, state: dict) -> bool:
379377

380378
body = bot.render_state_issue_body(state, snapshot.body)
381379

382-
response = bot.conditional_patch_state_issue(body, snapshot.etag)
380+
response = bot.patch_state_issue(body)
383381
if response.status_code == 200:
384382
_log(bot, "info", f"State saved to issue #{state_issue_number}", state_issue_number=state_issue_number)
385383
return True
386384

387-
if response.status_code in {409, 412}:
388-
_log(
389-
bot,
390-
"warning",
391-
f"State save hit conflict (status {response.status_code}); retrying ({attempt}/{lock_api_retry_limit})",
392-
state_issue_number=state_issue_number,
393-
status_code=response.status_code,
394-
retry_attempt=attempt,
395-
)
396-
delay = _retry_delay(bot, lock_retry_base_seconds, attempt)
397-
_sleep(bot, delay)
398-
continue
399-
400385
if response.status_code == 404:
401386
_log(bot, "error", f"State issue #{state_issue_number} not found during save_state", state_issue_number=state_issue_number)
402387
return False

tests/contract/reviewer_bot/test_adapter_contract.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,8 @@ def test_bootstrap_runtime_wires_explicit_adapter_services():
544544
assert hasattr(runtime.adapters.commands, "handle_pass_command")
545545
assert hasattr(runtime.adapters.queue, "get_next_reviewer")
546546
assert hasattr(runtime.adapters.state_lock, "assert_lock_held")
547+
assert hasattr(runtime.adapters.state_lock, "patch_state_issue")
548+
assert hasattr(runtime.adapters.state_lock, "conditional_patch_state_issue") is False
547549
assert hasattr(runtime.adapters.state_lock, "render_state_issue_body")
548550

549551

tests/contract/reviewer_bot/test_fake_runtime_contract.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ def test_fake_runtime_exposes_retained_runtime_labels_and_lock_helpers(monkeypat
164164
assert hasattr(runtime, "normalize_lock_metadata")
165165
assert hasattr(runtime, "get_state_issue")
166166
assert hasattr(runtime, "get_state_issue_snapshot")
167-
assert hasattr(runtime, "conditional_patch_state_issue")
167+
assert hasattr(runtime, "patch_state_issue")
168+
assert hasattr(runtime, "conditional_patch_state_issue") is False
168169
assert hasattr(runtime, "render_state_issue_body")
169170
assert hasattr(runtime, "get_lock_ref_snapshot")
170171
assert hasattr(runtime, "renew_state_issue_lease_lock")

tests/contract/reviewer_bot/test_runtime_protocols.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ def _assert_core_runtime_surface(runtime) -> None:
3939
assert runtime.domain.handlers is runtime.handlers
4040

4141
for helper_name in (
42+
"conditional_patch_state_issue",
4243
"project_status_labels_for_item",
4344
"sync_status_labels",
4445
"add_label_with_status",

tests/fixtures/fake_runtime.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ def __init__(self, runtime: "FakeReviewerBotRuntime"):
213213
"get_state_issue",
214214
"clear_lock_metadata",
215215
"get_state_issue_snapshot",
216-
"conditional_patch_state_issue",
216+
"patch_state_issue",
217217
"render_state_issue_body",
218218
"get_state_issue_html_url",
219219
"get_lock_ref_display",
@@ -402,8 +402,8 @@ def clear_lock_metadata(self):
402402
def get_state_issue_snapshot(self):
403403
return state_store_module.get_state_issue_snapshot(self._runtime)
404404

405-
def conditional_patch_state_issue(self, body: str, etag: str | None = None):
406-
return state_store_module.conditional_patch_state_issue(self._runtime, body, etag)
405+
def patch_state_issue(self, body: str):
406+
return state_store_module.patch_state_issue(self._runtime, body)
407407

408408
def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
409409
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):
620620
def get_state_issue_snapshot(self):
621621
return self.compat.state_lock.get_state_issue_snapshot()
622622

623-
def conditional_patch_state_issue(self, body: str, etag: str | None = None):
624-
return self.compat.state_lock.conditional_patch_state_issue(body, etag)
623+
def patch_state_issue(self, body: str):
624+
return self.compat.state_lock.patch_state_issue(body)
625625

626626
def render_state_issue_body(self, state: dict, base_body: str | None = None, *, preserve_state_block: bool = False):
627627
return self.compat.state_lock.render_state_issue_body(

tests/integration/reviewer_bot/test_app_execution.py

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import json
22
from pathlib import Path
3-
from urllib.parse import unquote
3+
from urllib.parse import unquote, urlparse
44

55
import pytest
66

@@ -10,10 +10,13 @@
1010
maintenance,
1111
maintenance_schedule,
1212
reconcile,
13+
state_store,
1314
)
1415
from scripts.reviewer_bot_lib.config import STATUS_AWAITING_REVIEWER_RESPONSE_LABEL
1516
from tests.fixtures.app_harness import AppHarness
17+
from tests.fixtures.http_responses import FakeGitHubResponse
1618
from tests.fixtures.reviewer_bot import make_state, make_tracked_review_state
19+
from tests.fixtures.reviewer_bot_fakes import RouteGitHubApi, github_result
1720

1821
pytestmark = pytest.mark.integration
1922

@@ -61,6 +64,27 @@ def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs
6164
return runtime, label_ops
6265

6366

67+
def _route_bootstrapped_rest_request(routes: RouteGitHubApi):
68+
def request(method, url, *, headers=None, json_data=None, timeout_seconds=None):
69+
parts = urlparse(url).path.strip("/").split("/")
70+
endpoint = "/".join(parts[3:])
71+
result = routes.github_api_request(
72+
method,
73+
endpoint,
74+
data=json_data,
75+
extra_headers=headers,
76+
timeout_seconds=timeout_seconds,
77+
)
78+
return FakeGitHubResponse(
79+
result.status_code or 0,
80+
payload=result.payload,
81+
text=result.text,
82+
headers=result.headers,
83+
)
84+
85+
return request
86+
87+
6488
def test_app_harness_exposes_focused_runtime_services(monkeypatch):
6589
harness = AppHarness(monkeypatch)
6690

@@ -400,6 +424,83 @@ def test_bootstrapped_runtime_workflow_dispatch_check_overdue_preserves_touched_
400424
assert runtime.ACTIVE_LEASE_CONTEXT is None
401425

402426

427+
def test_bootstrapped_runtime_workflow_dispatch_check_overdue_uses_real_save_state_without_conditional_issue_headers(
428+
monkeypatch,
429+
):
430+
runtime = reviewer_bot._runtime_bot()
431+
state = make_state()
432+
state["status_projection_epoch"] = runtime.STATUS_PROJECTION_EPOCH
433+
issue_number = "314"
434+
issue_body = state_store.render_state_issue_body(state)
435+
routes = RouteGitHubApi().add_request_sequence(
436+
"GET",
437+
f"issues/{issue_number}",
438+
[
439+
github_result(
440+
200,
441+
{"body": issue_body, "html_url": f"https://example.com/issues/{issue_number}"},
442+
headers={"ETag": '"etag-1"'},
443+
),
444+
github_result(
445+
200,
446+
{"body": issue_body, "html_url": f"https://example.com/issues/{issue_number}"},
447+
headers={"ETag": '"etag-2"'},
448+
),
449+
],
450+
).add_request(
451+
"PATCH",
452+
f"issues/{issue_number}",
453+
status_code=200,
454+
payload={"body": "updated"},
455+
)
456+
457+
def acquire_lock():
458+
runtime.ACTIVE_LEASE_CONTEXT = object()
459+
return runtime.ACTIVE_LEASE_CONTEXT
460+
461+
def release_lock():
462+
runtime.ACTIVE_LEASE_CONTEXT = None
463+
return True
464+
465+
def handle_scheduled_check_result(bot, current_state):
466+
del bot
467+
current_state["current_index"] = 1
468+
return maintenance.ScheduleHandlerResult(True, [])
469+
470+
monkeypatch.setattr(runtime.locks, "acquire", acquire_lock)
471+
monkeypatch.setattr(runtime.locks, "release", release_lock)
472+
monkeypatch.setattr(runtime.locks, "refresh", lambda: True)
473+
monkeypatch.setattr(runtime.rest_transport, "request", _route_bootstrapped_rest_request(routes))
474+
monkeypatch.setattr(runtime.adapters.workflow, "process_pass_until_expirations", lambda current_state: (current_state, []))
475+
monkeypatch.setattr(runtime.adapters.workflow, "sync_members_with_queue", lambda current_state: (current_state, []))
476+
monkeypatch.setattr(maintenance_schedule, "handle_scheduled_check_result", handle_scheduled_check_result)
477+
monkeypatch.setenv("EVENT_NAME", "workflow_dispatch")
478+
monkeypatch.setenv("EVENT_ACTION", "")
479+
monkeypatch.setenv("MANUAL_ACTION", "check-overdue")
480+
monkeypatch.setenv("GITHUB_TOKEN", "token")
481+
monkeypatch.setenv("REPO_OWNER", "rustfoundation")
482+
monkeypatch.setenv("REPO_NAME", "safety-critical-rust-coding-guidelines")
483+
monkeypatch.setenv("STATE_ISSUE_NUMBER", issue_number)
484+
485+
result = reviewer_bot.execute_run(reviewer_bot.build_event_context(runtime), runtime)
486+
487+
assert result.exit_code == 0
488+
assert result.state_changed is True
489+
assert runtime.ACTIVE_LEASE_CONTEXT is None
490+
assert sum(
491+
1
492+
for call in routes.request_calls
493+
if call.method == "GET" and call.endpoint == f"issues/{issue_number}"
494+
) >= 2
495+
patch_call = routes.request_calls[-1]
496+
assert patch_call.method == "PATCH"
497+
assert patch_call.endpoint == f"issues/{issue_number}"
498+
assert patch_call.data is not None
499+
assert "current_index: 1" in patch_call.data["body"]
500+
assert patch_call.extra_headers is not None
501+
assert all(header_name.lower() != "if-match" for header_name in patch_call.extra_headers)
502+
503+
403504
def test_d4a_app_branch_to_phase_map_is_frozen_pre_edit():
404505
app_text = Path("scripts/reviewer_bot_lib/app.py").read_text(encoding="utf-8")
405506

tests/unit/reviewer_bot/test_state_store.py

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
8080
assert observed["retry_policy"] == "idempotent_read"
8181

8282

83-
def test_conditional_patch_state_issue_sends_if_match_header(monkeypatch):
83+
def test_patch_state_issue_uses_plain_issue_write_request(monkeypatch):
8484
observed = {}
8585

8686
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):
9898

9999
bot = _bot(monkeypatch, STATE_ISSUE_NUMBER=1, github_api_request=fake_request)
100100

101-
state_store.conditional_patch_state_issue(bot, "updated", '"etag-1"')
102-
103-
assert observed["extra_headers"] == {"If-Match": '"etag-1"'}
104-
105-
106-
def test_conditional_patch_state_issue_omits_if_match_when_etag_missing(monkeypatch):
107-
observed = {}
108-
109-
def fake_request(method, endpoint, data=None, extra_headers=None, **kwargs):
110-
observed["extra_headers"] = extra_headers
111-
return GitHubApiResult(200, {"body": data["body"]}, {}, "ok", True, None, 0, None)
112-
113-
bot = _bot(monkeypatch, STATE_ISSUE_NUMBER=1, github_api_request=fake_request)
114-
115-
state_store.conditional_patch_state_issue(bot, "updated", None)
101+
state_store.patch_state_issue(bot, "updated")
116102

117103
assert observed["extra_headers"] is None
118104

119105

120-
def test_save_state_retries_precondition_failed_conflict_uses_injected_time_services(monkeypatch):
106+
def test_save_state_retries_retryable_write_failure_uses_injected_time_services(monkeypatch):
121107
state = make_state()
122108
snapshot = StateIssueSnapshot(
123109
body="body",
@@ -126,7 +112,7 @@ def test_save_state_retries_precondition_failed_conflict_uses_injected_time_serv
126112
)
127113
responses = iter(
128114
[
129-
GitHubApiResult(412, {"message": "precondition failed"}, {}, "precondition failed", False, None, 0, None),
115+
GitHubApiResult(502, {"message": "bad gateway"}, {}, "bad gateway", False, None, 0, None),
130116
GitHubApiResult(200, {"body": "updated"}, {}, "ok", True, None, 0, None),
131117
]
132118
)
@@ -141,9 +127,8 @@ def test_save_state_retries_precondition_failed_conflict_uses_injected_time_serv
141127
bot.ACTIVE_LEASE_CONTEXT = object()
142128
bot.locks.stub(refresh=lambda: True)
143129
bot.get_state_issue_snapshot = lambda: snapshot
144-
bot.parse_lock_metadata_from_issue_body = lambda body: {}
145130
bot.render_state_issue_body = lambda state_obj, base_body: "updated"
146-
bot.conditional_patch_state_issue = lambda body, etag=None: next(responses)
131+
bot.patch_state_issue = lambda body: next(responses)
147132

148133
assert state_store.save_state(bot, state) is True
149134
assert state["last_updated"] == clock.now().isoformat()

0 commit comments

Comments
 (0)