Skip to content

Commit a4fb859

Browse files
committed
fix(reviewer-bot): close follow-up bootstrap and workflow gaps
1 parent 95fbf53 commit a4fb859

8 files changed

Lines changed: 151 additions & 1 deletion

File tree

.github/workflows/reviewer-bot-issue-comment-direct.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,15 @@ jobs:
5151
EVENT_NAME: issue_comment
5252
EVENT_ACTION: created
5353
ISSUE_NUMBER: ${{ github.event.issue.number }}
54+
IS_PULL_REQUEST: 'false'
5455
ISSUE_STATE: ${{ github.event.issue.state }}
5556
ISSUE_TITLE: ${{ github.event.issue.title }}
5657
ISSUE_AUTHOR: ${{ github.event.issue.user.login }}
5758
ISSUE_HTML_URL: ${{ github.event.issue.html_url }}
5859
ISSUE_LABELS: ${{ toJson(github.event.issue.labels.*.name) }}
5960
COMMENT_BODY: ${{ github.event.comment.body }}
6061
COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
62+
COMMENT_AUTHOR_ID: ${{ github.event.comment.user.id }}
6163
COMMENT_ID: ${{ github.event.comment.id }}
6264
COMMENT_CREATED_AT: ${{ github.event.comment.created_at }}
6365
COMMENT_USER_TYPE: ${{ github.event.comment.user.type }}

.github/workflows/reviewer-bot-pr-metadata.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ jobs:
4848
EVENT_NAME: pull_request_target
4949
EVENT_ACTION: ${{ github.event.action }}
5050
ISSUE_NUMBER: ${{ github.event.pull_request.number }}
51+
LABEL_NAME: ${{ github.event.label.name }}
5152
ISSUE_AUTHOR: ${{ github.event.pull_request.user.login }}
5253
ISSUE_LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }}
5354
IS_PULL_REQUEST: 'true'

scripts/reviewer_bot_lib/bootstrap_runtime.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,9 @@ def _runtime(self):
322322
def normalize_lock_metadata(self, lock_meta):
323323
return state_store.normalize_lock_metadata(lock_meta)
324324

325+
def assert_lock_held(self, operation):
326+
return state_store.assert_lock_held(self._runtime(), operation)
327+
325328
def parse_iso8601_timestamp(self, value):
326329
return state_store.parse_iso8601_timestamp(value)
327330

tests/contract/reviewer_bot/test_adapter_contract.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,9 +543,20 @@ def test_bootstrap_runtime_wires_explicit_adapter_services():
543543
assert hasattr(runtime.adapters.review_state, "ensure_review_entry")
544544
assert hasattr(runtime.adapters.commands, "handle_pass_command")
545545
assert hasattr(runtime.adapters.queue, "get_next_reviewer")
546+
assert hasattr(runtime.adapters.state_lock, "assert_lock_held")
546547
assert hasattr(runtime.adapters.state_lock, "render_state_issue_body")
547548

548549

550+
def test_bootstrapped_runtime_state_lock_assertion_delegates_through_adapter_surface():
551+
runtime = reviewer_bot._runtime_bot()
552+
553+
with pytest.raises(RuntimeError, match="Mutating path reached without lease lock: contract-check"):
554+
runtime.assert_lock_held("contract-check")
555+
556+
runtime.ACTIVE_LEASE_CONTEXT = object()
557+
runtime.assert_lock_held("contract-check")
558+
559+
549560
def test_status_label_sync_contract_stays_on_workflow_adapter_surface():
550561
runtime = reviewer_bot._runtime_bot()
551562

@@ -586,6 +597,12 @@ def test_f2a_runtime_surface_inventory_matches_bootstrap_adapter_examples():
586597
capabilities = {entry["capability"]: entry for entry in inventory["capability_triples"]}
587598

588599
assert capabilities["comment-event dispatch"]["bootstrap_adapter"].endswith("handle_comment_event")
600+
assert capabilities["state-lock assertion delegation"]["runtime_forwarder"].endswith(
601+
"assert_lock_held"
602+
)
603+
assert capabilities["state-lock assertion delegation"]["bootstrap_adapter"].endswith(
604+
"assert_lock_held"
605+
)
589606
assert "workflow-run dispatch" not in capabilities
590607
assert capabilities["sync status labels"]["bootstrap_adapter"].endswith(
591608
"sync_status_labels_for_items"

tests/contract/reviewer_bot/test_fake_runtime_contract.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,9 @@ def test_f2a_runtime_surface_inventory_matches_fake_runtime_branch_examples():
334334
capabilities = {entry["capability"]: entry for entry in inventory["capability_triples"]}
335335

336336
assert capabilities["comment-event dispatch"]["fake_runtime_branch"].endswith("handle_comment_event")
337+
assert capabilities["state-lock assertion delegation"]["fake_runtime_branch"].endswith(
338+
"assert_lock_held"
339+
)
337340
assert capabilities["privileged accept-no-fls-changes execution"]["fake_runtime_branch"].endswith(
338341
"handle_accept_no_fls_changes_command"
339342
)

tests/contract/reviewer_bot/test_workflow_files.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,21 @@ def test_issue_comment_direct_workflow_exports_issue_state():
2727
)
2828
assert "ISSUE_STATE: ${{ github.event.issue.state }}" in workflow_text
2929

30+
31+
def test_issue_comment_direct_workflow_exports_retained_request_inputs():
32+
workflow_text = Path(".github/workflows/reviewer-bot-issue-comment-direct.yml").read_text(
33+
encoding="utf-8"
34+
)
35+
assert "IS_PULL_REQUEST: 'false'" in workflow_text
36+
assert "COMMENT_AUTHOR_ID: ${{ github.event.comment.user.id }}" in workflow_text
37+
38+
39+
def test_pr_metadata_workflow_exports_label_name_for_labeled_path():
40+
workflow_text = Path(".github/workflows/reviewer-bot-pr-metadata.yml").read_text(
41+
encoding="utf-8"
42+
)
43+
assert "LABEL_NAME: ${{ github.event.label.name }}" in workflow_text
44+
3045
def test_pr_comment_router_workflow_builds_payload_inline_without_bot_src_root():
3146
workflow = Path(".github/workflows/reviewer-bot-pr-comment-router.yml").read_text(encoding="utf-8")
3247
assert "build_pr_comment_observer_payload" not in workflow

tests/fixtures/equivalence/runtime_surface/triple_inventory.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,13 @@
6565
"fake_runtime_branch": "tests/fixtures/fake_runtime.py:sync_status_labels_for_items",
6666
"classification": "retained final surface"
6767
},
68+
{
69+
"capability": "state-lock assertion delegation",
70+
"runtime_forwarder": "scripts/reviewer_bot_lib/runtime.py:assert_lock_held",
71+
"bootstrap_adapter": "scripts/reviewer_bot_lib/bootstrap_runtime.py:_BootstrapStateLockAdapterServices.assert_lock_held",
72+
"fake_runtime_branch": "tests/fixtures/fake_runtime.py:assert_lock_held",
73+
"classification": "retained final surface"
74+
},
6875
{
6976
"capability": "privileged branch lookup",
7077
"runtime_forwarder": null,

tests/integration/reviewer_bot/test_app_execution.py

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33

44
import pytest
55

6-
from scripts.reviewer_bot_lib import reconcile
6+
from scripts import reviewer_bot
7+
from scripts.reviewer_bot_lib import event_inputs, reconcile
78
from tests.fixtures.app_harness import AppHarness
89
from tests.fixtures.reviewer_bot import make_state, make_tracked_review_state
910

@@ -184,6 +185,107 @@ def test_execute_run_returns_failure_for_invalid_workflow_run_context(monkeypatc
184185
assert result.state_changed is False
185186

186187

188+
def test_bootstrapped_runtime_executes_direct_issue_comment_path_with_strict_request_inputs(monkeypatch):
189+
runtime = reviewer_bot._runtime_bot()
190+
state = make_state()
191+
seen = {}
192+
193+
def acquire_lock():
194+
runtime.ACTIVE_LEASE_CONTEXT = object()
195+
return runtime.ACTIVE_LEASE_CONTEXT
196+
197+
def release_lock():
198+
runtime.ACTIVE_LEASE_CONTEXT = None
199+
return True
200+
201+
def handle_comment_event(current_state):
202+
assert current_state is state
203+
seen["request"] = event_inputs.build_comment_event_request(runtime)
204+
runtime.collect_touched_item(42)
205+
return True
206+
207+
monkeypatch.setattr(runtime.locks, "acquire", acquire_lock)
208+
monkeypatch.setattr(runtime.locks, "release", release_lock)
209+
monkeypatch.setattr(runtime.state_store, "load_state", lambda *, fail_on_unavailable=False: state)
210+
monkeypatch.setattr(runtime.state_store, "save_state", lambda current_state: True)
211+
monkeypatch.setattr(runtime.adapters.workflow, "process_pass_until_expirations", lambda current_state: (current_state, []))
212+
monkeypatch.setattr(runtime.adapters.workflow, "sync_members_with_queue", lambda current_state: (current_state, []))
213+
monkeypatch.setattr(runtime.adapters.workflow, "sync_status_labels_for_items", lambda current_state, issue_numbers: False)
214+
monkeypatch.setattr(runtime.handlers, "handle_comment_event", handle_comment_event)
215+
monkeypatch.setenv("EVENT_NAME", "issue_comment")
216+
monkeypatch.setenv("EVENT_ACTION", "created")
217+
monkeypatch.setenv("ISSUE_NUMBER", "42")
218+
monkeypatch.setenv("IS_PULL_REQUEST", "false")
219+
monkeypatch.setenv("ISSUE_STATE", "open")
220+
monkeypatch.setenv("ISSUE_AUTHOR", "dana")
221+
monkeypatch.setenv("ISSUE_LABELS", '["triage"]')
222+
monkeypatch.setenv("COMMENT_ID", "100")
223+
monkeypatch.setenv("COMMENT_AUTHOR", "alice")
224+
monkeypatch.setenv("COMMENT_AUTHOR_ID", "200")
225+
monkeypatch.setenv("COMMENT_BODY", "hello")
226+
monkeypatch.setenv("COMMENT_CREATED_AT", "2026-04-13T04:30:00Z")
227+
monkeypatch.delenv("COMMENT_SOURCE_EVENT_KEY", raising=False)
228+
monkeypatch.setenv("COMMENT_USER_TYPE", "User")
229+
monkeypatch.setenv("COMMENT_SENDER_TYPE", "User")
230+
monkeypatch.delenv("COMMENT_INSTALLATION_ID", raising=False)
231+
monkeypatch.setenv("COMMENT_PERFORMED_VIA_GITHUB_APP", "false")
232+
233+
context = reviewer_bot.build_event_context(runtime)
234+
result = reviewer_bot.execute_run(context, runtime)
235+
236+
assert result.exit_code == 0
237+
assert result.state_changed is True
238+
assert seen["request"].is_pull_request is False
239+
assert seen["request"].comment_author_id == 200
240+
assert seen["request"].comment_source_event_key == "issue_comment:100"
241+
assert runtime.ACTIVE_LEASE_CONTEXT is None
242+
243+
244+
def test_bootstrapped_runtime_executes_pr_metadata_closed_dispatch_path(monkeypatch):
245+
runtime = reviewer_bot._runtime_bot()
246+
state = make_state()
247+
calls = []
248+
249+
def acquire_lock():
250+
runtime.ACTIVE_LEASE_CONTEXT = object()
251+
return runtime.ACTIVE_LEASE_CONTEXT
252+
253+
def release_lock():
254+
runtime.ACTIVE_LEASE_CONTEXT = None
255+
return True
256+
257+
def handle_closed_event(current_state):
258+
calls.append(current_state)
259+
return True
260+
261+
monkeypatch.setattr(runtime.locks, "acquire", acquire_lock)
262+
monkeypatch.setattr(runtime.locks, "release", release_lock)
263+
monkeypatch.setattr(runtime.state_store, "load_state", lambda *, fail_on_unavailable=False: state)
264+
monkeypatch.setattr(runtime.state_store, "save_state", lambda current_state: True)
265+
monkeypatch.setattr(runtime.adapters.workflow, "process_pass_until_expirations", lambda current_state: (current_state, []))
266+
monkeypatch.setattr(runtime.adapters.workflow, "sync_members_with_queue", lambda current_state: (current_state, []))
267+
monkeypatch.setattr(runtime.adapters.workflow, "sync_status_labels_for_items", lambda current_state, issue_numbers: False)
268+
monkeypatch.setattr(runtime.handlers, "handle_closed_event", handle_closed_event)
269+
monkeypatch.setenv("EVENT_NAME", "pull_request_target")
270+
monkeypatch.setenv("EVENT_ACTION", "closed")
271+
monkeypatch.setenv("ISSUE_NUMBER", "42")
272+
monkeypatch.setenv("IS_PULL_REQUEST", "true")
273+
monkeypatch.setenv("ISSUE_AUTHOR", "dana")
274+
monkeypatch.setenv("ISSUE_LABELS", '["triage"]')
275+
monkeypatch.setenv("PR_HEAD_SHA", "head-1")
276+
monkeypatch.setenv("EVENT_CREATED_AT", "2026-04-13T04:31:00Z")
277+
278+
context = reviewer_bot.build_event_context(runtime)
279+
result = reviewer_bot.execute_run(context, runtime)
280+
281+
assert context.event_name == "pull_request_target"
282+
assert context.event_action == "closed"
283+
assert calls == [state]
284+
assert result.exit_code == 0
285+
assert result.state_changed is True
286+
assert runtime.ACTIVE_LEASE_CONTEXT is None
287+
288+
187289
def test_d4a_app_branch_to_phase_map_is_frozen_pre_edit():
188290
app_text = Path("scripts/reviewer_bot_lib/app.py").read_text(encoding="utf-8")
189291

0 commit comments

Comments
 (0)