From 1bda9ecb8ad0ac1c422673dcb3be07bf85a0c3de Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Mon, 13 Apr 2026 22:53:40 +0900 Subject: [PATCH 1/2] fix(reviewer-bot): close final follow-up trusted path gaps Keep trusted status-label and FLS guidance paths on their retained owners, preserve workflow-dispatch projection follow-up, and harden bootstrapped proof so these regressions are caught before merge. --- scripts/reviewer_bot_lib/commands.py | 3 +- scripts/reviewer_bot_lib/lifecycle.py | 5 +- scripts/reviewer_bot_lib/maintenance.py | 5 +- scripts/reviewer_bot_lib/reviews.py | 18 ++- .../test_fake_runtime_contract.py | 14 ++ .../test_reviewer_board_workflow_contracts.py | 12 ++ .../reviewer_bot/test_runtime_protocols.py | 9 ++ tests/fixtures/fake_runtime.py | 127 +++++++++++++++++- .../reviewer_bot/test_app_execution.py | 116 +++++++++++++++- tests/unit/reviewer_bot/test_commands.py | 25 ++++ tests/unit/reviewer_bot/test_lifecycle.py | 18 +++ tests/unit/reviewer_bot/test_maintenance.py | 15 +++ .../reviewer_bot/test_reviews_live_fetch.py | 18 ++- 13 files changed, 366 insertions(+), 19 deletions(-) diff --git a/scripts/reviewer_bot_lib/commands.py b/scripts/reviewer_bot_lib/commands.py index fff8be8eb..055c0f8c4 100644 --- a/scripts/reviewer_bot_lib/commands.py +++ b/scripts/reviewer_bot_lib/commands.py @@ -9,6 +9,7 @@ from .event_inputs import build_assignment_request as decode_assignment_request from .guidance import ( get_assignment_failure_comment, + get_fls_audit_guidance, get_issue_guidance, get_pr_guidance, ) @@ -139,7 +140,7 @@ def _apply_assignment_side_effects( else: labels = set(request.issue_labels) guidance = ( - bot.get_fls_audit_guidance(reviewer, request.issue_author) + get_fls_audit_guidance(reviewer, request.issue_author) if bot.FLS_AUDIT_LABEL in labels else get_issue_guidance(reviewer, request.issue_author) ) diff --git a/scripts/reviewer_bot_lib/lifecycle.py b/scripts/reviewer_bot_lib/lifecycle.py index 6b2f8b530..3fe5265fb 100644 --- a/scripts/reviewer_bot_lib/lifecycle.py +++ b/scripts/reviewer_bot_lib/lifecycle.py @@ -58,9 +58,10 @@ def _semantic_digest(value: str) -> str: def _write_transition_notice_marker_cutover(bot) -> None: - config_dir = Path(bot.get_config_value("OPENCODE_CONFIG_DIR") or "") - if not config_dir: + config_dir_value = bot.get_config_value("OPENCODE_CONFIG_DIR").strip() + if not config_dir_value: return + config_dir = Path(config_dir_value) artifact_path = config_dir / "reviewer-bot" / "maintainability-remediation" / "transition-notice-marker-cutover.json" artifact_path.parent.mkdir(parents=True, exist_ok=True) artifact_path.write_text( diff --git a/scripts/reviewer_bot_lib/maintenance.py b/scripts/reviewer_bot_lib/maintenance.py index 771d2afd1..c06a29810 100644 --- a/scripts/reviewer_bot_lib/maintenance.py +++ b/scripts/reviewer_bot_lib/maintenance.py @@ -87,7 +87,10 @@ def handle_manual_dispatch(bot, state: dict) -> bool: bot.collect_touched_item(issue_number) return False if action == "check-overdue": - return maintenance_schedule.handle_scheduled_check_result(bot, state).state_changed + result = maintenance_schedule.handle_scheduled_check_result(bot, state) + for issue_number in result.touched_items: + bot.collect_touched_item(issue_number) + return result.state_changed if action == "execute-pending-privileged-command": source_event_key = request.privileged_source_event_key if not source_event_key: diff --git a/scripts/reviewer_bot_lib/reviews.py b/scripts/reviewer_bot_lib/reviews.py index 89d883ba3..5e41deaf1 100644 --- a/scripts/reviewer_bot_lib/reviews.py +++ b/scripts/reviewer_bot_lib/reviews.py @@ -140,6 +140,8 @@ def is_triage_or_higher(bot, username: str) -> bool: def trigger_mandatory_approver_escalation(bot, state: dict, issue_number: int) -> bool: from scripts.reviewer_bot_core import mandatory_approver_policy + from . import github_api + review_data = review_state.ensure_review_entry(state, issue_number, create=True) if review_data is None: return False @@ -156,7 +158,7 @@ def trigger_mandatory_approver_escalation(bot, state: dict, issue_number: int) - state_changed = True if decision["attempt_label_apply"]: try: - if bot.add_label_with_status(issue_number, MANDATORY_TRIAGE_APPROVER_LABEL): + if github_api.add_label_with_status(bot, issue_number, MANDATORY_TRIAGE_APPROVER_LABEL): if decision["record_label_applied_at"]: review_data["mandatory_approver_label_applied_at"] = str(decision["now"]) state_changed = True @@ -172,6 +174,8 @@ def trigger_mandatory_approver_escalation(bot, state: dict, issue_number: int) - def satisfy_mandatory_approver_requirement(bot, state: dict, issue_number: int, approver: str) -> bool: from scripts.reviewer_bot_core import mandatory_approver_policy + from . import github_api + review_data = review_state.ensure_review_entry(state, issue_number, create=True) if review_data is None: return False @@ -186,7 +190,7 @@ def satisfy_mandatory_approver_requirement(bot, state: dict, issue_number: int, review_data["mandatory_approver_satisfied_by"] = str(decision["approver"]) review_data["mandatory_approver_satisfied_at"] = str(decision["now"]) try: - bot.remove_label_with_status(issue_number, MANDATORY_TRIAGE_APPROVER_LABEL) + github_api.remove_label_with_status(bot, issue_number, MANDATORY_TRIAGE_APPROVER_LABEL) except RuntimeError as exc: _log(bot, "warning", f"Unable to remove escalation label on #{issue_number}: {exc}", issue_number=issue_number, error=str(exc)) bot.github.post_comment(issue_number, MANDATORY_TRIAGE_SATISFIED_TEMPLATE.format(approver=approver)) @@ -332,6 +336,8 @@ def project_status_labels_for_item( def sync_status_labels(bot, issue_number: int, desired_labels: set[str], actual_labels: Iterable[str]) -> bool: + from . import github_api + actual_status_labels = {label for label in actual_labels if label in STATUS_LABELS} to_add = desired_labels - actual_status_labels to_remove = actual_status_labels - desired_labels @@ -342,11 +348,11 @@ def sync_status_labels(bot, issue_number: int, desired_labels: set[str], actual_ raise RuntimeError(f"Unable to ensure reviewer-bot status label exists: {label}") changed = False for label in sorted(to_remove): - if not bot.remove_label_with_status(issue_number, label): + if not github_api.remove_label_with_status(bot, issue_number, label): raise RuntimeError(f"Unable to remove reviewer-bot status label '{label}' from #{issue_number}") changed = True for label in sorted(to_add): - if not bot.add_label_with_status(issue_number, label): + if not github_api.add_label_with_status(bot, issue_number, label): raise RuntimeError(f"Unable to add reviewer-bot status label '{label}' to #{issue_number}") changed = True return changed @@ -356,7 +362,7 @@ def sync_status_labels_for_items(bot, state: dict, issue_numbers: Iterable[int]) changed = False for issue_number in sorted({n for n in issue_numbers if isinstance(n, int) and n > 0}): issue_snapshot = bot.github.get_issue_or_pr_snapshot(issue_number) - desired_labels, metadata = bot.project_status_labels_for_item(issue_number, state, issue_snapshot=issue_snapshot) + desired_labels, metadata = project_status_labels_for_item(bot, issue_number, state, issue_snapshot=issue_snapshot) if desired_labels is None: reason = metadata.get("reason") if isinstance(metadata, dict) else "unknown" raise RuntimeError(f"Failed to derive reviewer-bot status labels for #{issue_number}: {reason}") @@ -370,7 +376,7 @@ def sync_status_labels_for_items(bot, state: dict, issue_numbers: Iterable[int]) name = label.get("name") if isinstance(name, str): actual_labels.add(name) - if bot.sync_status_labels(issue_number, desired_labels, actual_labels): + if sync_status_labels(bot, issue_number, desired_labels, actual_labels): changed = True return changed diff --git a/tests/contract/reviewer_bot/test_fake_runtime_contract.py b/tests/contract/reviewer_bot/test_fake_runtime_contract.py index 4b9474d0f..4e7ee3951 100644 --- a/tests/contract/reviewer_bot/test_fake_runtime_contract.py +++ b/tests/contract/reviewer_bot/test_fake_runtime_contract.py @@ -194,6 +194,20 @@ def test_fake_runtime_review_state_compatibility_surface_is_limited(monkeypatch) assert hasattr(runtime, name) is False +def test_fake_runtime_adapter_views_do_not_alias_unrelated_retained_roles(monkeypatch): + runtime = FakeReviewerBotRuntime(monkeypatch) + + assert runtime.adapters.github is runtime.github + assert runtime.adapters.review_state is not runtime.adapters.commands + assert runtime.adapters.review_state is not runtime.adapters.queue + assert hasattr(runtime.adapters.review_state, "compute_reviewer_response_state") + assert hasattr(runtime.adapters.commands, "parse_command") + assert hasattr(runtime.adapters.queue, "get_next_reviewer") + assert hasattr(runtime.adapters.state_lock, "assert_lock_held") + assert hasattr(runtime.adapters.commands, "get_next_reviewer") is False + assert hasattr(runtime.adapters.queue, "parse_command") is False + + def test_fake_runtime_rejects_unknown_handler_names(monkeypatch): runtime = FakeReviewerBotRuntime(monkeypatch) diff --git a/tests/contract/reviewer_bot/test_reviewer_board_workflow_contracts.py b/tests/contract/reviewer_bot/test_reviewer_board_workflow_contracts.py index f06f795bf..04ae07673 100644 --- a/tests/contract/reviewer_bot/test_reviewer_board_workflow_contracts.py +++ b/tests/contract/reviewer_bot/test_reviewer_board_workflow_contracts.py @@ -28,3 +28,15 @@ def test_sweeper_repair_workflow_scopes_reviewer_board_env_to_preview_only(): "REVIEWER_BOARD_TOKEN: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.action == 'preview-reviewer-board' && secrets.REVIEWER_BOARD_TOKEN || '' }}" in workflow_text ) + + +def test_sweeper_repair_workflow_exports_retained_manual_dispatch_env_contract(): + data = yaml.safe_load(Path(".github/workflows/reviewer-bot-sweeper-repair.yml").read_text(encoding="utf-8")) + on_block = data.get("on", data.get(True)) + action_input = on_block["workflow_dispatch"]["inputs"]["action"] + workflow_text = Path(".github/workflows/reviewer-bot-sweeper-repair.yml").read_text(encoding="utf-8") + + assert "check-overdue" in action_input["options"] + assert "repair-review-status-labels" in action_input["options"] + assert "MANUAL_ACTION: ${{ github.event.inputs.action }}" in workflow_text + assert "ISSUE_NUMBER: ${{ github.event.inputs.issue_number }}" in workflow_text diff --git a/tests/contract/reviewer_bot/test_runtime_protocols.py b/tests/contract/reviewer_bot/test_runtime_protocols.py index 607820f04..57a46fe92 100644 --- a/tests/contract/reviewer_bot/test_runtime_protocols.py +++ b/tests/contract/reviewer_bot/test_runtime_protocols.py @@ -38,6 +38,15 @@ def _assert_core_runtime_surface(runtime) -> None: assert runtime.domain.locks is runtime.locks assert runtime.domain.handlers is runtime.handlers + for helper_name in ( + "project_status_labels_for_item", + "sync_status_labels", + "add_label_with_status", + "remove_label_with_status", + "get_fls_audit_guidance", + ): + assert hasattr(runtime, helper_name) is False + def test_fake_runtime_default_lock_state_matches_production_contract(monkeypatch): runtime = FakeReviewerBotRuntime(monkeypatch) diff --git a/tests/fixtures/fake_runtime.py b/tests/fixtures/fake_runtime.py index 48031f4ab..15ac79a52 100644 --- a/tests/fixtures/fake_runtime.py +++ b/tests/fixtures/fake_runtime.py @@ -107,15 +107,127 @@ def __init__(self, *, state_store, github, locks, handlers, workflow, adapters, self.compat = compat +class _AdapterView: + def __init__(self, target, allowed: set[str]): + self._target = target + self._allowed = set(allowed) + + def __getattr__(self, name: str): + if name not in self._allowed: + raise AttributeError(name) + return getattr(self._target, name) + + class FakeRuntimeAdapterServices: def __init__(self, runtime: "FakeReviewerBotRuntime"): self._runtime = runtime - self.workflow = runtime.workflow - self.review = runtime.compat.review - self.review_state = runtime.compat.review - self.commands = runtime.compat.review - self.queue = runtime.compat.review - self.automation = runtime.compat.automation + self.github = runtime.github + self.workflow = _AdapterView( + runtime.workflow, + { + "process_pass_until_expirations", + "sync_members_with_queue", + "sync_status_labels_for_items", + "fetch_members", + }, + ) + self.review_state = _AdapterView( + runtime.compat.review, + { + "maybe_record_head_observation_repair", + "handle_transition_notice", + "ensure_review_entry", + "set_current_reviewer", + "update_reviewer_activity", + "mark_review_complete", + "is_triage_or_higher", + "trigger_mandatory_approver_escalation", + "satisfy_mandatory_approver_requirement", + "compute_reviewer_response_state", + "rebuild_pr_approval_state", + }, + ) + self.commands = _AdapterView( + SimpleNamespace( + handle_pass_command=runtime.compat.review.handle_pass_command, + handle_pass_until_command=runtime.compat.review.handle_pass_until_command, + handle_label_command=runtime.compat.review.handle_label_command, + handle_sync_members_command=runtime.compat.review.handle_sync_members_command, + handle_queue_command=runtime.compat.review.handle_queue_command, + handle_commands_command=runtime.compat.review.handle_commands_command, + handle_claim_command=runtime.compat.review.handle_claim_command, + handle_release_command=runtime.compat.review.handle_release_command, + handle_rectify_command=runtime.compat.review.handle_rectify_command, + handle_assign_command=runtime.compat.review.handle_assign_command, + handle_assign_from_queue_command=runtime.compat.review.handle_assign_from_queue_command, + handle_accept_no_fls_changes_command=runtime.compat.automation.handle_accept_no_fls_changes_command, + get_commands_help=runtime.compat.review.get_commands_help, + strip_code_blocks=runtime.compat.review.strip_code_blocks, + parse_command=runtime.compat.review.parse_command, + ), + { + "handle_pass_command", + "handle_pass_until_command", + "handle_label_command", + "handle_sync_members_command", + "handle_queue_command", + "handle_commands_command", + "handle_claim_command", + "handle_release_command", + "handle_rectify_command", + "handle_assign_command", + "handle_assign_from_queue_command", + "handle_accept_no_fls_changes_command", + "get_commands_help", + "strip_code_blocks", + "parse_command", + }, + ) + self.queue = _AdapterView( + runtime.compat.review, + { + "get_next_reviewer", + "record_assignment", + "reposition_member_as_next", + }, + ) + self.automation = _AdapterView( + runtime.compat.automation, + { + "run_command", + "summarize_output", + "list_changed_files", + "get_default_branch", + "find_open_pr_for_branch_status", + "create_pull_request", + "fetch_members", + "handle_accept_no_fls_changes_command", + }, + ) + self.state_lock = _AdapterView( + runtime.compat.state_lock, + { + "assert_lock_held", + "parse_iso8601_timestamp", + "normalize_lock_metadata", + "get_state_issue", + "clear_lock_metadata", + "get_state_issue_snapshot", + "conditional_patch_state_issue", + "render_state_issue_body", + "get_state_issue_html_url", + "get_lock_ref_display", + "get_lock_ref_snapshot", + "build_lock_metadata", + "create_lock_commit", + "cas_update_lock_ref", + "lock_is_currently_valid", + "renew_state_issue_lease_lock", + "ensure_state_issue_lease_lock_fresh", + "acquire_state_issue_lease_lock", + "release_state_issue_lease_lock", + }, + ) def process_pass_until_expirations(self, state: dict): return self._runtime.workflow.process_pass_until_expirations(state) @@ -278,6 +390,9 @@ def parse_iso8601_timestamp(self, value: Any): def normalize_lock_metadata(self, lock_meta: dict | None): return state_store_module.normalize_lock_metadata(lock_meta) + def assert_lock_held(self, context: str) -> None: + return state_store_module.assert_lock_held(self._runtime, context) + def get_state_issue(self): return state_store_module.get_state_issue(self._runtime) diff --git a/tests/integration/reviewer_bot/test_app_execution.py b/tests/integration/reviewer_bot/test_app_execution.py index f8e944b31..a4d051985 100644 --- a/tests/integration/reviewer_bot/test_app_execution.py +++ b/tests/integration/reviewer_bot/test_app_execution.py @@ -1,16 +1,66 @@ import json from pathlib import Path +from urllib.parse import unquote import pytest from scripts import reviewer_bot -from scripts.reviewer_bot_lib import event_inputs, reconcile +from scripts.reviewer_bot_lib import ( + event_inputs, + maintenance, + maintenance_schedule, + reconcile, +) +from scripts.reviewer_bot_lib.config import STATUS_AWAITING_REVIEWER_RESPONSE_LABEL from tests.fixtures.app_harness import AppHarness from tests.fixtures.reviewer_bot import make_state, make_tracked_review_state pytestmark = pytest.mark.integration +def _configure_bootstrapped_runtime_with_real_status_projection(monkeypatch, state, *, issue_state: str): + runtime = reviewer_bot._runtime_bot() + label_ops = [] + + def acquire_lock(): + runtime.ACTIVE_LEASE_CONTEXT = object() + return runtime.ACTIVE_LEASE_CONTEXT + + def release_lock(): + runtime.ACTIVE_LEASE_CONTEXT = None + return True + + def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs): + if method == "POST" and endpoint == "labels": + return runtime.GitHubApiResult(201, {}, {}, "created", True, None, 0, None) + if method == "DELETE" and endpoint.startswith("issues/42/labels/"): + label_ops.append(("remove", unquote(endpoint.rsplit("/", 1)[-1]))) + return runtime.GitHubApiResult(204, None, {}, "", True, None, 0, None) + if method == "POST" and endpoint == "issues/42/labels": + label_ops.append(("add", data["labels"][0])) + return runtime.GitHubApiResult(200, {}, {}, "ok", True, None, 0, None) + raise AssertionError((method, endpoint, data)) + + monkeypatch.setattr(runtime.locks, "acquire", acquire_lock) + monkeypatch.setattr(runtime.locks, "release", release_lock) + monkeypatch.setattr(runtime.state_store, "load_state", lambda *, fail_on_unavailable=False: state) + monkeypatch.setattr(runtime.state_store, "save_state", lambda current_state: True) + 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( + runtime.github, + "get_issue_or_pr_snapshot", + lambda issue_number: { + "number": issue_number, + "state": issue_state, + "labels": [{"name": STATUS_AWAITING_REVIEWER_RESPONSE_LABEL}], + "pull_request": {}, + }, + ) + monkeypatch.setattr(runtime, "github_api_request", github_api_request) + return runtime, label_ops + + def test_app_harness_exposes_focused_runtime_services(monkeypatch): harness = AppHarness(monkeypatch) @@ -286,6 +336,70 @@ def handle_closed_event(current_state): assert runtime.ACTIVE_LEASE_CONTEXT is None +def test_bootstrapped_runtime_pr_metadata_closed_executes_real_status_label_projection_path(monkeypatch): + state = make_state() + review = make_tracked_review_state(state, 42, reviewer="alice") + assert review is not None + runtime, label_ops = _configure_bootstrapped_runtime_with_real_status_projection( + monkeypatch, state, issue_state="closed" + ) + monkeypatch.setenv("EVENT_NAME", "pull_request_target") + monkeypatch.setenv("EVENT_ACTION", "closed") + monkeypatch.setenv("ISSUE_NUMBER", "42") + monkeypatch.setenv("IS_PULL_REQUEST", "true") + monkeypatch.setenv("ISSUE_AUTHOR", "dana") + monkeypatch.setenv("ISSUE_LABELS", '["triage"]') + monkeypatch.setenv("PR_HEAD_SHA", "head-1") + monkeypatch.setenv("EVENT_CREATED_AT", "2026-04-13T04:31:00Z") + + result = reviewer_bot.execute_run(reviewer_bot.build_event_context(runtime), runtime) + + assert result.exit_code == 0 + assert result.state_changed is True + assert label_ops == [("remove", STATUS_AWAITING_REVIEWER_RESPONSE_LABEL)] + assert runtime.ACTIVE_LEASE_CONTEXT is None + + +def test_bootstrapped_runtime_workflow_dispatch_repair_status_labels_uses_real_projection_path(monkeypatch): + state = make_state() + runtime, label_ops = _configure_bootstrapped_runtime_with_real_status_projection( + monkeypatch, state, issue_state="open" + ) + monkeypatch.setattr(maintenance.reviews, "list_open_items_with_status_labels", lambda bot: [42]) + monkeypatch.setenv("EVENT_NAME", "workflow_dispatch") + monkeypatch.setenv("EVENT_ACTION", "") + monkeypatch.setenv("MANUAL_ACTION", "repair-review-status-labels") + + result = reviewer_bot.execute_run(reviewer_bot.build_event_context(runtime), runtime) + + assert result.exit_code == 0 + assert result.state_changed is True + assert label_ops == [("remove", STATUS_AWAITING_REVIEWER_RESPONSE_LABEL)] + assert runtime.ACTIVE_LEASE_CONTEXT is None + + +def test_bootstrapped_runtime_workflow_dispatch_check_overdue_preserves_touched_item_projection(monkeypatch): + state = make_state() + runtime, label_ops = _configure_bootstrapped_runtime_with_real_status_projection( + monkeypatch, state, issue_state="open" + ) + monkeypatch.setattr( + maintenance_schedule, + "handle_scheduled_check_result", + lambda bot, current: maintenance.ScheduleHandlerResult(False, [42]), + ) + monkeypatch.setenv("EVENT_NAME", "workflow_dispatch") + monkeypatch.setenv("EVENT_ACTION", "") + monkeypatch.setenv("MANUAL_ACTION", "check-overdue") + + result = reviewer_bot.execute_run(reviewer_bot.build_event_context(runtime), runtime) + + assert result.exit_code == 0 + assert result.state_changed is True + assert label_ops == [("remove", STATUS_AWAITING_REVIEWER_RESPONSE_LABEL)] + assert runtime.ACTIVE_LEASE_CONTEXT is None + + 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_commands.py b/tests/unit/reviewer_bot/test_commands.py index f715c9d8b..78977e2b5 100644 --- a/tests/unit/reviewer_bot/test_commands.py +++ b/tests/unit/reviewer_bot/test_commands.py @@ -144,6 +144,31 @@ def test_claim_command_posts_pr_guidance_on_success(monkeypatch): assert posted == [guidance.get_pr_guidance("felix91gr", "PLeVasseur")] +def test_assign_command_uses_guidance_owner_for_fls_issue_guidance(monkeypatch): + harness = CommandHarness(monkeypatch) + state = make_state() + state["queue"] = [{"github": "felix91gr", "name": "Félix Fischer"}] + request = harness.typed_assignment_request( + issue_number=42, + issue_author="PLeVasseur", + is_pull_request=False, + issue_labels=(FLS_AUDIT_LABEL,), + ) + harness.stub_assignees([]) + harness.stub_assignment() + harness.runtime.get_fls_audit_guidance = lambda *args, **kwargs: pytest.fail( + "commands.py should use guidance.get_fls_audit_guidance directly" + ) + posted = [] + harness.runtime.github.post_comment = lambda issue_number, body: posted.append(body) or True + + response, success = harness.handle_assign(state, 42, "@felix91gr", request=request) + + assert success is True + assert response == "✅ @felix91gr has been assigned as reviewer." + assert posted == [guidance.get_fls_audit_guidance("felix91gr", "PLeVasseur")] + + def test_pass_command_posts_pr_guidance_for_new_reviewer(monkeypatch): harness = CommandHarness(monkeypatch) state = make_state() diff --git a/tests/unit/reviewer_bot/test_lifecycle.py b/tests/unit/reviewer_bot/test_lifecycle.py index 47fae20c5..7ecf56bae 100644 --- a/tests/unit/reviewer_bot/test_lifecycle.py +++ b/tests/unit/reviewer_bot/test_lifecycle.py @@ -106,6 +106,24 @@ def test_handle_transition_notice_message_does_not_claim_reassignment(monkeypatc assert "/pass" in posted[0] +@pytest.mark.parametrize("config_value", [None, ""]) +def test_handle_transition_notice_skips_cutover_artifact_write_when_opencode_config_dir_is_blank_or_unset( + monkeypatch, tmp_path, config_value +): + monkeypatch.chdir(tmp_path) + runtime = FakeReviewerBotRuntime(monkeypatch) + state = make_state() + review_state.ensure_review_entry(state, 42, create=True) + runtime.github.post_comment = lambda issue_number, body: True + if config_value is not None: + runtime.set_config_value("OPENCODE_CONFIG_DIR", config_value) + + assert lifecycle.handle_transition_notice(runtime, state, 42, "alice") is True + assert not ( + tmp_path / "reviewer-bot" / "maintainability-remediation" / "transition-notice-marker-cutover.json" + ).exists() + + def test_l1_fake_runtime_and_bootstrap_keep_override_wiring_explicit_without_canonical_introspection(): fake_runtime_text = Path("tests/fixtures/fake_runtime.py").read_text(encoding="utf-8") bootstrap_text = Path("scripts/reviewer_bot_lib/bootstrap_runtime.py").read_text(encoding="utf-8") diff --git a/tests/unit/reviewer_bot/test_maintenance.py b/tests/unit/reviewer_bot/test_maintenance.py index f79449fb0..b0f188b55 100644 --- a/tests/unit/reviewer_bot/test_maintenance.py +++ b/tests/unit/reviewer_bot/test_maintenance.py @@ -155,6 +155,21 @@ def test_finalize_schedule_result_drains_touched_items_for_projection_followup(m assert bot.drain_touched_items() == [] +def test_manual_dispatch_check_overdue_preserves_touched_items_for_projection_followup(monkeypatch): + bot = FakeReviewerBotRuntime(monkeypatch) + bot.ACTIVE_LEASE_CONTEXT = object() + state = make_state() + bot.set_config_value("MANUAL_ACTION", "check-overdue") + monkeypatch.setattr( + maintenance_schedule, + "handle_scheduled_check_result", + lambda runtime, current: maintenance.ScheduleHandlerResult(False, [42, 99]), + ) + + assert maintenance.handle_manual_dispatch(bot, state) is False + assert bot.drain_touched_items() == [42, 99] + + def test_scheduled_check_clears_head_observation_repair_marker_after_success(monkeypatch): state = make_state() review = review_state.ensure_review_entry(state, 42, create=True) diff --git a/tests/unit/reviewer_bot/test_reviews_live_fetch.py b/tests/unit/reviewer_bot/test_reviews_live_fetch.py index 9203623a7..bd3ffb386 100644 --- a/tests/unit/reviewer_bot/test_reviews_live_fetch.py +++ b/tests/unit/reviewer_bot/test_reviews_live_fetch.py @@ -327,12 +327,19 @@ def test_trigger_mandatory_approver_escalation_sets_required_label_and_ping(monk review = make_tracked_review_state(state, 42, reviewer="alice") comments = [] labels = [] + + def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs): + assert method == "POST" + assert endpoint == "issues/42/labels" + labels.append((42, data["labels"][0])) + return SimpleNamespace(status_code=200, text="ok") + runtime = SimpleNamespace( github=SimpleNamespace( ensure_label_exists=lambda label: True, post_comment=lambda issue_number, body: comments.append((issue_number, body)) or True, ), - add_label_with_status=lambda issue_number, label: labels.append((issue_number, label)) or True, + github_api_request=github_api_request, logger=SimpleNamespace(event=lambda *args, **kwargs: None), ) monkeypatch.setattr(reviews, "_now_iso", lambda: "2026-03-21T10:00:00+00:00") @@ -375,8 +382,15 @@ def test_satisfy_mandatory_approver_requirement_clears_required_and_records_sati review["mandatory_approver_required"] = True removed = [] comments = [] + + def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs): + assert method == "DELETE" + assert endpoint.startswith("issues/42/labels/") + removed.append((42, reviews.MANDATORY_TRIAGE_APPROVER_LABEL)) + return SimpleNamespace(status_code=204, text="ok") + runtime = SimpleNamespace( - remove_label_with_status=lambda issue_number, label: removed.append((issue_number, label)) or True, + github_api_request=github_api_request, github=SimpleNamespace(post_comment=lambda issue_number, body: comments.append((issue_number, body)) or True), logger=SimpleNamespace(event=lambda *args, **kwargs: None), ) From ce6f5948d7e1a1b6395d77982e83651deae9ff9d Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Mon, 13 Apr 2026 22:57:30 +0900 Subject: [PATCH 2/2] fix(reviewer-bot): align mandatory approver proof harness Keep the mandatory approver equivalence fixtures exercising the retained transport-owned label path so branch CI matches the final follow-up owner model. --- .../test_mandatory_approver_policy_equivalence.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/reviewer_bot/test_mandatory_approver_policy_equivalence.py b/tests/unit/reviewer_bot/test_mandatory_approver_policy_equivalence.py index 026b04022..fc153524c 100644 --- a/tests/unit/reviewer_bot/test_mandatory_approver_policy_equivalence.py +++ b/tests/unit/reviewer_bot/test_mandatory_approver_policy_equivalence.py @@ -18,11 +18,22 @@ def _make_bot(*, ensure_label_exists=True, add_label_result=True, post_comment_r comments = [] labels_added = [] labels_removed = [] + + def github_api_request(method, endpoint, data=None, extra_headers=None, **kwargs): + if method == "POST" and endpoint == "issues/42/labels": + labels_added.append((42, data["labels"][0])) + return SimpleNamespace(status_code=200 if add_label_result else 500, text="ok") + if method == "DELETE" and endpoint.startswith("issues/42/labels/"): + labels_removed.append((42, reviews.MANDATORY_TRIAGE_APPROVER_LABEL)) + return SimpleNamespace(status_code=403 if remove_label_error else 204, text="ok") + raise AssertionError((method, endpoint, data)) + bot = SimpleNamespace( github=SimpleNamespace( ensure_label_exists=lambda label: ensure_label_exists, post_comment=lambda issue_number, body: comments.append((issue_number, body)) or post_comment_result, ), + github_api_request=github_api_request, add_label_with_status=lambda issue_number, label: labels_added.append((issue_number, label)) or add_label_result, remove_label_with_status=( (lambda issue_number, label: (_ for _ in ()).throw(RuntimeError("remove failed")))