From a42580452eea068de9ef8ab670c5a301a8d78711 Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Wed, 11 Feb 2026 07:07:03 +0900 Subject: [PATCH 1/3] fix(reviewer-bot): enforce lease-safe state updates and triage escalation --- .../reviewer-bot-tests/test_reviewer_bot.py | 439 ++++- .github/workflows/reviewer-bot-reconcile.yml | 3 + .github/workflows/reviewer-bot.yml | 7 +- scripts/reviewer_bot.py | 1562 ++++++++++++++--- 4 files changed, 1780 insertions(+), 231 deletions(-) diff --git a/.github/reviewer-bot-tests/test_reviewer_bot.py b/.github/reviewer-bot-tests/test_reviewer_bot.py index ee50d2483..68758bc69 100644 --- a/.github/reviewer-bot-tests/test_reviewer_bot.py +++ b/.github/reviewer-bot-tests/test_reviewer_bot.py @@ -52,25 +52,58 @@ def clear_env(): "WORKFLOW_RUN_HEAD_SHA", "WORKFLOW_RUN_RECONCILE_PR_NUMBER", "WORKFLOW_RUN_RECONCILE_HEAD_SHA", + "WORKFLOW_RUN_ID", + "WORKFLOW_NAME", + "WORKFLOW_JOB_NAME", } with pytest.MonkeyPatch().context() as monkeypatch: for name in env_vars: monkeypatch.delenv(name, raising=False) + monkeypatch.setattr( + reviewer_bot, + "ACTIVE_LEASE_CONTEXT", + reviewer_bot.LeaseContext( + lock_token="test-lock-token", + lock_owner_run_id="test-run", + lock_owner_workflow="test-workflow", + lock_owner_job="test-job", + state_issue_url="https://example.com/state", + ), + ) yield @pytest.fixture def stub_api(monkeypatch): monkeypatch.setattr(reviewer_bot, "github_api", lambda *args, **kwargs: {}) + monkeypatch.setattr( + reviewer_bot, + "github_api_request", + lambda *args, **kwargs: reviewer_bot.GitHubApiResult( + status_code=200, + payload={}, + headers={}, + text="", + ok=True, + ), + ) monkeypatch.setattr(reviewer_bot, "add_reaction", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "post_comment", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "assign_reviewer", lambda *args, **kwargs: True) + monkeypatch.setattr( + reviewer_bot, + "request_reviewer_assignment", + lambda *args, **kwargs: reviewer_bot.AssignmentAttempt(success=True, status_code=201), + ) monkeypatch.setattr(reviewer_bot, "unassign_reviewer", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "remove_pr_reviewer", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "remove_assignee", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "get_repo_labels", lambda *args, **kwargs: {"a", "b"}) monkeypatch.setattr(reviewer_bot, "add_label", lambda *args, **kwargs: True) + monkeypatch.setattr(reviewer_bot, "add_label_with_status", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "remove_label", lambda *args, **kwargs: True) + monkeypatch.setattr(reviewer_bot, "remove_label_with_status", lambda *args, **kwargs: True) + monkeypatch.setattr(reviewer_bot, "ensure_label_exists", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "fetch_members", lambda *args, **kwargs: []) @@ -122,20 +155,103 @@ def test_load_state_applies_defaults(monkeypatch): def test_save_state_formats_issue_body(monkeypatch): payload = {} + monkeypatch.setattr(reviewer_bot, "STATE_ISSUE_NUMBER", 314) + + initial_body = reviewer_bot.render_state_issue_body(make_state(), reviewer_bot.clear_lock_metadata()) + + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=initial_body, + etag='"abc123"', + html_url="https://example.com/issues/314", + ), + ) + + def capture_patch(body, etag): + payload["body"] = body + payload["etag"] = etag + return reviewer_bot.GitHubApiResult( + status_code=200, + payload={"ok": True}, + headers={}, + text="", + ok=True, + ) + + monkeypatch.setattr(reviewer_bot, "conditional_patch_state_issue", capture_patch) + state = make_state() + assert reviewer_bot.save_state(state) is True + assert payload["etag"] == '"abc123"' + assert reviewer_bot.STATE_BLOCK_START_MARKER in payload["body"] + assert reviewer_bot.LOCK_BLOCK_START_MARKER in payload["body"] + assert "```yaml" in payload["body"] + + +def test_parse_lock_metadata_from_issue_body_uses_markers(): + lock_meta = reviewer_bot.normalize_lock_metadata( + { + "schema_version": 1, + "lock_owner_run_id": "123", + "lock_owner_workflow": "Reviewer Bot", + "lock_owner_job": "reviewer-bot", + "lock_token": "abcdef", + "lock_acquired_at": "2026-02-11T00:00:00+00:00", + "lock_expires_at": "2026-02-11T00:05:00+00:00", + } + ) + body = reviewer_bot.render_state_issue_body(make_state(), lock_meta) + + parsed = reviewer_bot.parse_lock_metadata_from_issue_body(body) - def capture_api(method, endpoint, data=None): - payload["method"] = method - payload["endpoint"] = endpoint - payload["data"] = data - return {"ok": True} + assert parsed == lock_meta - monkeypatch.setattr(reviewer_bot, "github_api", capture_api) + +def test_save_state_preserves_lock_metadata_across_save(monkeypatch): + payload = {} monkeypatch.setattr(reviewer_bot, "STATE_ISSUE_NUMBER", 314) + expected_lock_meta = reviewer_bot.normalize_lock_metadata( + { + "schema_version": 1, + "lock_owner_run_id": "run-42", + "lock_owner_workflow": "Reviewer Bot", + "lock_owner_job": "reviewer-bot", + "lock_token": "lock-token-42", + "lock_acquired_at": "2026-02-11T10:00:00+00:00", + "lock_expires_at": "2026-02-11T10:05:00+00:00", + } + ) + initial_body = reviewer_bot.render_state_issue_body(make_state(), expected_lock_meta) + + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=initial_body, + etag='"etag-state"', + html_url="https://example.com/issues/314", + ), + ) + + def capture_patch(body, etag): + payload["body"] = body + payload["etag"] = etag + return reviewer_bot.GitHubApiResult( + status_code=200, + payload={"ok": True}, + headers={}, + text="", + ok=True, + ) + + monkeypatch.setattr(reviewer_bot, "conditional_patch_state_issue", capture_patch) + state = make_state() assert reviewer_bot.save_state(state) is True - assert payload["method"] == "PATCH" - assert payload["endpoint"] == "issues/314" - assert "```yaml" in payload["data"]["body"] + assert payload["etag"] == '"etag-state"' + parsed_lock = reviewer_bot.parse_lock_metadata_from_issue_body(payload["body"]) + assert parsed_lock == expected_lock_meta def test_strip_code_blocks_removes_fenced_indented_inline(): @@ -197,6 +313,7 @@ def __init__(self, status_code, content): self.status_code = status_code self.content = content self.text = "error" + self.headers = {} def json(self): return {"ok": True} @@ -212,6 +329,159 @@ def fake_request(*args, **kwargs): assert reviewer_bot.github_api("GET", "issues/1") is None +def test_acquire_state_issue_lease_lock_success(monkeypatch): + monkeypatch.setattr(reviewer_bot, "ACTIVE_LEASE_CONTEXT", None) + monkeypatch.setenv("WORKFLOW_RUN_ID", "9001") + monkeypatch.setenv("WORKFLOW_NAME", "Reviewer Bot") + monkeypatch.setenv("WORKFLOW_JOB_NAME", "reviewer-bot") + monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) + monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) + + body = reviewer_bot.render_state_issue_body(make_state(), reviewer_bot.clear_lock_metadata()) + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=body, + etag='"etag"', + html_url="https://example.com/issues/314", + ), + ) + + monkeypatch.setattr( + reviewer_bot, + "conditional_patch_state_issue", + lambda body, etag: reviewer_bot.GitHubApiResult( + status_code=200, + payload={"ok": True}, + headers={}, + text="", + ok=True, + ), + ) + + ctx = reviewer_bot.acquire_state_issue_lease_lock() + + assert ctx.lock_owner_run_id == "9001" + assert ctx.lock_owner_workflow == "Reviewer Bot" + assert ctx.lock_owner_job == "reviewer-bot" + assert reviewer_bot.ACTIVE_LEASE_CONTEXT is not None + + +def test_acquire_state_issue_lease_lock_retries_on_conflict(monkeypatch): + monkeypatch.setattr(reviewer_bot, "ACTIVE_LEASE_CONTEXT", None) + monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) + monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) + + body = reviewer_bot.render_state_issue_body(make_state(), reviewer_bot.clear_lock_metadata()) + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=body, + etag='"etag"', + html_url="https://example.com/issues/314", + ), + ) + + statuses = iter([412, 200]) + + monkeypatch.setattr( + reviewer_bot, + "conditional_patch_state_issue", + lambda body, etag: reviewer_bot.GitHubApiResult( + status_code=next(statuses), + payload={"ok": True}, + headers={}, + text="", + ok=True, + ), + ) + + ctx = reviewer_bot.acquire_state_issue_lease_lock() + assert ctx.lock_token + + +def test_acquire_state_issue_lease_lock_takes_over_expired_lock(monkeypatch): + monkeypatch.setattr(reviewer_bot, "ACTIVE_LEASE_CONTEXT", None) + monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) + monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) + + expired_lock = reviewer_bot.normalize_lock_metadata( + { + "schema_version": 1, + "lock_owner_run_id": "123", + "lock_owner_workflow": "Reviewer Bot", + "lock_owner_job": "reviewer-bot", + "lock_token": "stale-lock", + "lock_acquired_at": "2020-01-01T00:00:00+00:00", + "lock_expires_at": "2020-01-01T00:01:00+00:00", + } + ) + body = reviewer_bot.render_state_issue_body(make_state(), expired_lock) + + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=body, + etag='"etag"', + html_url="https://example.com/issues/314", + ), + ) + + monkeypatch.setattr( + reviewer_bot, + "conditional_patch_state_issue", + lambda body, etag: reviewer_bot.GitHubApiResult( + status_code=200, + payload={"ok": True}, + headers={}, + text="", + ok=True, + ), + ) + + ctx = reviewer_bot.acquire_state_issue_lease_lock() + assert ctx.lock_token != "stale-lock" + + +def test_acquire_state_issue_lease_lock_times_out(monkeypatch): + monkeypatch.setattr(reviewer_bot, "ACTIVE_LEASE_CONTEXT", None) + monkeypatch.setattr(reviewer_bot, "LOCK_MAX_WAIT_SECONDS", 1) + monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) + monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) + + valid_lock = reviewer_bot.normalize_lock_metadata( + { + "schema_version": 1, + "lock_owner_run_id": "other-run", + "lock_owner_workflow": "Reviewer Bot", + "lock_owner_job": "reviewer-bot", + "lock_token": "active-lock", + "lock_acquired_at": "2999-01-01T00:00:00+00:00", + "lock_expires_at": "2999-01-01T00:10:00+00:00", + } + ) + body = reviewer_bot.render_state_issue_body(make_state(), valid_lock) + + monkeypatch.setattr( + reviewer_bot, + "get_state_issue_snapshot", + lambda: reviewer_bot.StateIssueSnapshot( + body=body, + etag='"etag"', + html_url="https://example.com/issues/314", + ), + ) + + monotonic_values = iter([0.0, 0.0, 2.0, 2.0]) + monkeypatch.setattr(reviewer_bot.time, "monotonic", lambda: next(monotonic_values)) + + with pytest.raises(RuntimeError, match="Timed out waiting for reviewer-bot lease lock"): + reviewer_bot.acquire_state_issue_lease_lock() + + def test_handle_pass_command_requires_current_reviewer(stub_api, monkeypatch): state = make_state() issue_number = 123 @@ -523,6 +793,53 @@ def test_handle_comment_event_assign_from_queue(stub_api, captured_comments, mon assert "assigned as reviewer" in captured_comments[1]["body"] +def test_handle_assign_from_queue_truthful_when_pr_request_returns_422( + stub_api, captured_comments, monkeypatch +): + state = make_state() + os.environ["IS_PULL_REQUEST"] = "true" + os.environ["ISSUE_AUTHOR"] = "dana" + + monkeypatch.setattr(reviewer_bot, "get_issue_assignees", lambda *args, **kwargs: []) + monkeypatch.setattr(reviewer_bot, "get_next_reviewer", lambda *args, **kwargs: "alice") + monkeypatch.setattr( + reviewer_bot, + "request_reviewer_assignment", + lambda *args, **kwargs: reviewer_bot.AssignmentAttempt(success=False, status_code=422), + ) + + response, success = reviewer_bot.handle_assign_from_queue_command(state, 42) + + assert success is True + assert "has been assigned as reviewer" not in response + assert "remains designated as reviewer" in response + assert len(captured_comments) == 1 + assert captured_comments[0]["body"] == reviewer_bot.REVIEWER_REQUEST_422_TEMPLATE.format(reviewer="alice") + + +def test_handle_issue_or_pr_opened_pr_request_422_posts_truthful_message( + stub_api, captured_comments, monkeypatch +): + state = make_state() + os.environ["ISSUE_NUMBER"] = "42" + os.environ["ISSUE_AUTHOR"] = "dana" + os.environ["ISSUE_LABELS"] = '["coding guideline"]' + os.environ["IS_PULL_REQUEST"] = "true" + monkeypatch.setattr(reviewer_bot, "get_issue_assignees", lambda *args, **kwargs: []) + monkeypatch.setattr(reviewer_bot, "get_next_reviewer", lambda *args, **kwargs: "alice") + monkeypatch.setattr( + reviewer_bot, + "request_reviewer_assignment", + lambda *args, **kwargs: reviewer_bot.AssignmentAttempt(success=False, status_code=422), + ) + + handled = reviewer_bot.handle_issue_or_pr_opened(state) + + assert handled is True + assert len(captured_comments) == 1 + assert captured_comments[0]["body"] == reviewer_bot.REVIEWER_REQUEST_422_TEMPLATE.format(reviewer="alice") + + def test_handle_issue_or_pr_opened_assigns_reviewer(stub_api, captured_comments, monkeypatch): state = make_state() os.environ["ISSUE_NUMBER"] = "42" @@ -733,12 +1050,13 @@ def test_reconcile_active_review_entry_marks_complete_for_approved(monkeypatch): }, ], ) + monkeypatch.setattr(reviewer_bot, "check_user_permission", lambda *args, **kwargs: True) message, success, state_changed = reviewer_bot.reconcile_active_review_entry(state, 42) assert success is True assert state_changed is True - assert "marked review complete" in message + assert "applied approval transitions" in message review_data = state["active_reviews"]["42"] assert review_data["review_completed_at"] is not None assert review_data["review_completed_by"] == "alice" @@ -943,6 +1261,7 @@ def test_handle_workflow_run_event_reconciles_approval(monkeypatch): } ], ) + monkeypatch.setattr(reviewer_bot, "check_user_permission", lambda *args, **kwargs: True) posted_comments = [] @@ -1043,6 +1362,7 @@ def test_handle_workflow_run_event_comment_failure_is_non_fatal(monkeypatch, cap } ], ) + monkeypatch.setattr(reviewer_bot, "check_user_permission", lambda *args, **kwargs: True) monkeypatch.setattr(reviewer_bot, "post_comment", lambda issue_number, body: False) handled = reviewer_bot.handle_workflow_run_event(state) @@ -1100,6 +1420,101 @@ def test_handle_pull_request_review_event_approval_marks_complete(stub_api): assert review_data["last_reviewer_activity"] != "2000-01-01T00:00:00+00:00" +def test_read_level_designated_approval_triggers_mandatory_escalation( + stub_api, captured_comments, monkeypatch +): + state = make_state() + state["active_reviews"]["42"] = { + "current_reviewer": "alice", + "assigned_at": "2000-01-01T00:00:00+00:00", + "last_reviewer_activity": "2000-01-01T00:00:00+00:00", + } + os.environ["ISSUE_NUMBER"] = "42" + os.environ["REVIEW_STATE"] = "approved" + os.environ["REVIEW_AUTHOR"] = "alice" + monkeypatch.setattr(reviewer_bot, "check_user_permission", lambda *args, **kwargs: False) + + handled = reviewer_bot.handle_pull_request_review_event(state) + + assert handled is True + review_data = state["active_reviews"]["42"] + assert review_data["review_completed_at"] is not None + assert review_data["mandatory_approver_required"] is True + assert review_data["mandatory_approver_pinged_at"] is not None + assert any( + c["body"] == reviewer_bot.MANDATORY_TRIAGE_ESCALATION_TEMPLATE + for c in captured_comments + ) + + +def test_read_level_escalation_comment_posts_once_per_review_cycle( + stub_api, captured_comments, monkeypatch +): + state = make_state() + state["active_reviews"]["42"] = { + "current_reviewer": "alice", + "assigned_at": "2000-01-01T00:00:00+00:00", + "last_reviewer_activity": "2000-01-01T00:00:00+00:00", + } + os.environ["ISSUE_NUMBER"] = "42" + os.environ["REVIEW_STATE"] = "approved" + os.environ["REVIEW_AUTHOR"] = "alice" + monkeypatch.setattr(reviewer_bot, "check_user_permission", lambda *args, **kwargs: False) + + assert reviewer_bot.handle_pull_request_review_event(state) is True + assert reviewer_bot.handle_pull_request_review_event(state) is False + + escalation_comments = [ + c for c in captured_comments if c["body"] == reviewer_bot.MANDATORY_TRIAGE_ESCALATION_TEMPLATE + ] + assert len(escalation_comments) == 1 + + +def test_triage_approval_clears_mandatory_escalation( + stub_api, captured_comments, monkeypatch +): + state = make_state() + state["active_reviews"]["42"] = { + "current_reviewer": "alice", + "assigned_at": "2000-01-01T00:00:00+00:00", + "last_reviewer_activity": "2000-01-01T00:00:00+00:00", + "mandatory_approver_required": True, + "mandatory_approver_pinged_at": "2026-02-11T10:00:00+00:00", + "mandatory_approver_label_applied_at": "2026-02-11T10:00:00+00:00", + } + os.environ["ISSUE_NUMBER"] = "42" + os.environ["REVIEW_STATE"] = "approved" + os.environ["REVIEW_AUTHOR"] = "bob" + + removed = {"called": False} + + def fake_remove_label(issue_number, label): + removed["called"] = True + assert issue_number == 42 + assert label == reviewer_bot.MANDATORY_TRIAGE_APPROVER_LABEL + return True + + monkeypatch.setattr( + reviewer_bot, + "check_user_permission", + lambda username, required_permission="triage": username.lower() == "bob", + ) + monkeypatch.setattr(reviewer_bot, "remove_label_with_status", fake_remove_label) + + handled = reviewer_bot.handle_pull_request_review_event(state) + + assert handled is True + review_data = state["active_reviews"]["42"] + assert review_data["mandatory_approver_required"] is False + assert review_data["mandatory_approver_satisfied_by"] == "bob" + assert review_data["mandatory_approver_satisfied_at"] is not None + assert removed["called"] is True + assert any( + c["body"] == reviewer_bot.MANDATORY_TRIAGE_SATISFIED_TEMPLATE.format(approver="bob") + for c in captured_comments + ) + + @pytest.mark.parametrize("review_state", ["commented", "changes_requested"]) def test_handle_pull_request_review_event_updates_activity(stub_api, review_state): state = make_state() @@ -1271,6 +1686,8 @@ def test_handle_comment_event_unknown_command(stub_api, captured_comments): def test_main_fails_when_save_state_fails(monkeypatch): monkeypatch.setenv("EVENT_NAME", "issue_comment") monkeypatch.setenv("EVENT_ACTION", "created") + monkeypatch.setattr(reviewer_bot, "acquire_state_issue_lease_lock", lambda: None) + monkeypatch.setattr(reviewer_bot, "release_state_issue_lease_lock", lambda: True) monkeypatch.setattr(reviewer_bot, "load_state", lambda: make_state()) monkeypatch.setattr(reviewer_bot, "process_pass_until_expirations", lambda state: (state, [])) monkeypatch.setattr(reviewer_bot, "sync_members_with_queue", lambda state: (state, [])) @@ -1287,6 +1704,8 @@ def test_main_workflow_run_fails_closed_on_invalid_context(monkeypatch): monkeypatch.setenv("EVENT_NAME", "workflow_run") monkeypatch.setenv("EVENT_ACTION", "completed") monkeypatch.setenv("WORKFLOW_RUN_EVENT", "pull_request_review") + monkeypatch.setattr(reviewer_bot, "acquire_state_issue_lease_lock", lambda: None) + monkeypatch.setattr(reviewer_bot, "release_state_issue_lease_lock", lambda: True) monkeypatch.setattr(reviewer_bot, "load_state", lambda: make_state()) monkeypatch.setattr(reviewer_bot, "process_pass_until_expirations", lambda state: (state, [])) monkeypatch.setattr(reviewer_bot, "sync_members_with_queue", lambda state: (state, [])) diff --git a/.github/workflows/reviewer-bot-reconcile.yml b/.github/workflows/reviewer-bot-reconcile.yml index 66784ffde..325e505e7 100644 --- a/.github/workflows/reviewer-bot-reconcile.yml +++ b/.github/workflows/reviewer-bot-reconcile.yml @@ -140,5 +140,8 @@ jobs: WORKFLOW_RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} REPO_OWNER: ${{ github.repository_owner }} REPO_NAME: ${{ github.event.repository.name }} + WORKFLOW_RUN_ID: ${{ github.run_id }} + WORKFLOW_NAME: ${{ github.workflow }} + WORKFLOW_JOB_NAME: ${{ github.job }} run: | uv run python scripts/reviewer_bot.py diff --git a/.github/workflows/reviewer-bot.yml b/.github/workflows/reviewer-bot.yml index d9a9f7fd5..923a0613e 100644 --- a/.github/workflows/reviewer-bot.yml +++ b/.github/workflows/reviewer-bot.yml @@ -35,8 +35,8 @@ on: - show-state - check-overdue -# Ensure only one instance runs at a time per issue/PR -# This prevents race conditions when both 'opened' and 'labeled' fire together +# Per-issue/PR concurrency only reduces duplicate same-item runs. +# Global state serialization is enforced by reviewer_bot.py lease locking on issue #314. concurrency: group: reviewer-bot-${{ github.event.issue.number || github.event.pull_request.number || 'manual' }} cancel-in-progress: false @@ -101,6 +101,9 @@ jobs: # Repository info REPO_OWNER: ${{ github.repository_owner }} REPO_NAME: ${{ github.event.repository.name }} + WORKFLOW_RUN_ID: ${{ github.run_id }} + WORKFLOW_NAME: ${{ github.workflow }} + WORKFLOW_JOB_NAME: ${{ github.job }} # Labels on the issue/PR (as JSON) ISSUE_LABELS: ${{ toJson(github.event.issue.labels.*.name || github.event.pull_request.labels.*.name) }} run: | diff --git a/scripts/reviewer_bot.py b/scripts/reviewer_bot.py index b1c6b2e2a..eab96c77e 100644 --- a/scripts/reviewer_bot.py +++ b/scripts/reviewer_bot.py @@ -56,21 +56,20 @@ import json import os +import random import re import subprocess import sys +import time +import uuid +from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path - -import yaml +from typing import Any # GitHub API interaction -try: - import requests -except ImportError: - # requests is available via uv - pass - +import requests +import yaml # ============================================================================== # Configuration @@ -87,6 +86,50 @@ MEMBERS_URL = "https://raw.githubusercontent.com/rustfoundation/safety-critical-rust-consortium/main/subcommittee/coding-guidelines/members.md" MAX_RECENT_ASSIGNMENTS = 20 +STATE_BLOCK_START_MARKER = "" +STATE_BLOCK_END_MARKER = "" +LOCK_BLOCK_START_MARKER = "" +LOCK_BLOCK_END_MARKER = "" + +LOCK_SCHEMA_VERSION = 1 +LOCK_LEASE_TTL_SECONDS = int(os.environ.get("REVIEWER_BOT_LOCK_TTL_SECONDS", "300")) +LOCK_RETRY_BASE_SECONDS = float(os.environ.get("REVIEWER_BOT_LOCK_RETRY_SECONDS", "2")) +LOCK_MAX_WAIT_SECONDS = int(os.environ.get("REVIEWER_BOT_LOCK_MAX_WAIT_SECONDS", "1200")) +LOCK_API_RETRY_LIMIT = int(os.environ.get("REVIEWER_BOT_LOCK_API_RETRY_LIMIT", "5")) + +MANDATORY_TRIAGE_APPROVER_LABEL = "triage approver required" +MANDATORY_TRIAGE_PING_TARGETS = [ + "@PLeVasseur", + "@felix91gr", + "@rcseacord", + "@plaindocs", + "@AlexCeleste", + "@sei-dsvoboda", +] + +REVIEWER_REQUEST_422_TEMPLATE = ( + "@{reviewer} is designated as reviewer by queue rotation, but GitHub could not add them to PR " + "Reviewers automatically (API 422). A triage+ approver may still be required before merge queue." +) +MANDATORY_TRIAGE_ESCALATION_TEMPLATE = ( + "Mandatory triage approval required before merge queue. Pinging " + f"{' '.join(MANDATORY_TRIAGE_PING_TARGETS)}. " + "Label applied: `triage approver required`." +) +MANDATORY_TRIAGE_SATISFIED_TEMPLATE = ( + "Mandatory triage approval satisfied by @{approver}; removed `triage approver required`." +) + +LOCK_METADATA_KEYS = [ + "schema_version", + "lock_owner_run_id", + "lock_owner_workflow", + "lock_owner_job", + "lock_token", + "lock_acquired_at", + "lock_expires_at", +] + # Review deadline configuration REVIEW_DEADLINE_DAYS = 14 # Days before first warning TRANSITION_PERIOD_DAYS = 14 # Days after warning before transition to Observer @@ -121,6 +164,52 @@ def get_commands_help() -> str: # ============================================================================== +@dataclass +class GitHubApiResult: + status_code: int + payload: Any + headers: dict[str, str] + text: str + ok: bool + + +@dataclass +class AssignmentAttempt: + success: bool + status_code: int | None + exhausted_retryable_failure: bool = False + + +@dataclass +class StateIssueSnapshot: + body: str + etag: str | None + html_url: str + + +@dataclass +class StateIssueBodyParts: + prefix: str + state_block_inner: str | None + between_state_and_lock: str + lock_block_inner: str | None + suffix: str + has_state_markers: bool + has_lock_markers: bool + + +@dataclass +class LeaseContext: + lock_token: str + lock_owner_run_id: str + lock_owner_workflow: str + lock_owner_job: str + state_issue_url: str + + +ACTIVE_LEASE_CONTEXT: LeaseContext | None = None + + def get_github_token() -> str: """Get the GitHub token from environment.""" token = os.environ.get("GITHUB_TOKEN") @@ -130,8 +219,15 @@ def get_github_token() -> str: return token -def github_api(method: str, endpoint: str, data: dict | None = None) -> dict | None: - """Make a GitHub API request.""" +def github_api_request( + method: str, + endpoint: str, + data: dict | None = None, + extra_headers: dict[str, str] | None = None, + *, + suppress_error_log: bool = False, +) -> GitHubApiResult: + """Make a GitHub API request and return status, payload, and headers.""" token = get_github_token() repo = f"{os.environ['REPO_OWNER']}/{os.environ['REPO_NAME']}" url = f"https://api.github.com/repos/{repo}/{endpoint}" @@ -141,16 +237,43 @@ def github_api(method: str, endpoint: str, data: dict | None = None) -> dict | N "Accept": "application/vnd.github.v3+json", "X-GitHub-Api-Version": "2022-11-28", } + if extra_headers: + headers.update(extra_headers) response = requests.request(method, url, headers=headers, json=data) - if response.status_code >= 400: - print(f"GitHub API error: {response.status_code} - {response.text}", file=sys.stderr) - return None - + payload: Any = None if response.content: - return response.json() - return {} + try: + payload = response.json() + except ValueError: + payload = None + + ok = response.status_code < 400 + if not ok and not suppress_error_log: + print( + f"GitHub API error: {response.status_code} - {response.text}", + file=sys.stderr, + ) + + normalized_headers = {key.lower(): value for key, value in response.headers.items()} + return GitHubApiResult( + status_code=response.status_code, + payload=payload, + headers=normalized_headers, + text=response.text, + ok=ok, + ) + + +def github_api(method: str, endpoint: str, data: dict | None = None) -> Any | None: + """Backward-compatible wrapper around github_api_request.""" + response = github_api_request(method, endpoint, data) + if not response.ok: + return None + if response.payload is None: + return {} + return response.payload def post_comment(issue_number: int, body: str) -> bool: @@ -180,20 +303,155 @@ def remove_label(issue_number: int, label: str) -> bool: return True -def assign_reviewer(issue_number: int, username: str) -> bool: - """Assign a user as a reviewer (via assignees for issues, reviewers for PRs).""" +def add_label_with_status(issue_number: int, label: str) -> bool: + """Add a label with explicit HTTP status handling.""" + response = github_api_request( + "POST", + f"issues/{issue_number}/labels", + {"labels": [label]}, + suppress_error_log=True, + ) + if response.status_code in {200, 201}: + return True + if response.status_code in {401, 403}: + raise RuntimeError( + f"Permission denied adding label '{label}' to #{issue_number}: {response.text}" + ) + print( + f"WARNING: Failed to add label '{label}' to #{issue_number} " + f"(status {response.status_code}): {response.text}", + file=sys.stderr, + ) + return False + + +def remove_label_with_status(issue_number: int, label: str) -> bool: + """Remove a label with explicit HTTP status handling.""" + response = github_api_request( + "DELETE", + f"issues/{issue_number}/labels/{label}", + suppress_error_log=True, + ) + if response.status_code in {200, 204, 404}: + return True + if response.status_code in {401, 403}: + raise RuntimeError( + f"Permission denied removing label '{label}' from #{issue_number}: {response.text}" + ) + print( + f"WARNING: Failed to remove label '{label}' from #{issue_number} " + f"(status {response.status_code}): {response.text}", + file=sys.stderr, + ) + return False + + +def ensure_label_exists(label: str) -> bool: + """Create label if missing; treat 422 as already exists.""" + response = github_api_request( + "POST", + "labels", + { + "name": label, + "color": "d73a4a", + "description": "Indicates triage+ approval is required before merge queue", + }, + suppress_error_log=True, + ) + + if response.status_code == 201: + return True + if response.status_code == 422: + return True + + print( + f"WARNING: Failed to ensure label '{label}' exists (status {response.status_code}): " + f"{response.text}", + file=sys.stderr, + ) + return False + + +def request_reviewer_assignment(issue_number: int, username: str) -> AssignmentAttempt: + """Request reviewer/assignee with status-aware handling and retries.""" is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" if is_pr: - # For PRs, request review - result = github_api("POST", f"pulls/{issue_number}/requested_reviewers", - {"reviewers": [username]}) + endpoint = f"pulls/{issue_number}/requested_reviewers" + payload = {"reviewers": [username]} + assignment_target = "PR reviewer" else: - # For issues, use assignees - result = github_api("POST", f"issues/{issue_number}/assignees", - {"assignees": [username]}) + endpoint = f"issues/{issue_number}/assignees" + payload = {"assignees": [username]} + assignment_target = "issue assignee" - return result is not None + for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): + response = github_api_request("POST", endpoint, payload, suppress_error_log=True) + + if response.status_code in {200, 201}: + return AssignmentAttempt(success=True, status_code=response.status_code) + + if response.status_code == 422: + # Queue policy remains permissive: reviewer is still designated in bot state. + return AssignmentAttempt(success=False, status_code=422) + + if response.status_code in {401, 403}: + raise RuntimeError( + f"Permission denied requesting {assignment_target} @{username} on " + f"#{issue_number} (status {response.status_code}): {response.text}" + ) + + if response.status_code == 429 or response.status_code >= 500: + if attempt < LOCK_API_RETRY_LIMIT: + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + print( + f"Retryable {assignment_target} API failure for @{username} on #{issue_number} " + f"(status {response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})" + ) + time.sleep(delay) + continue + return AssignmentAttempt( + success=False, + status_code=response.status_code, + exhausted_retryable_failure=True, + ) + + print( + f"WARNING: Unexpected {assignment_target} API status {response.status_code} " + f"for @{username} on #{issue_number}: {response.text}", + file=sys.stderr, + ) + return AssignmentAttempt(success=False, status_code=response.status_code) + + return AssignmentAttempt(success=False, status_code=None, exhausted_retryable_failure=True) + + +def assign_reviewer(issue_number: int, username: str) -> bool: + """Backward-compatible reviewer assignment boolean wrapper.""" + attempt = request_reviewer_assignment(issue_number, username) + return attempt.success + + +def get_assignment_failure_comment(reviewer: str, attempt: AssignmentAttempt) -> str | None: + """Return truthful assignment warning comment text when GitHub assignment fails.""" + is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" + + if attempt.status_code == 422: + if is_pr: + return REVIEWER_REQUEST_422_TEMPLATE.format(reviewer=reviewer) + return ( + f"@{reviewer} is designated as reviewer by queue rotation, but GitHub could not " + "add them as an assignee automatically (API 422)." + ) + + if attempt.exhausted_retryable_failure: + return ( + f"@{reviewer} is designated as reviewer by queue rotation, but GitHub could not " + f"add them to PR Reviewers automatically after retries (status {attempt.status_code}). " + "A triage+ approver may still be required before merge queue." + ) + + return None def get_issue_assignees(issue_number: int) -> list[str]: @@ -353,31 +611,264 @@ def get_state_issue() -> dict | None: if not STATE_ISSUE_NUMBER: print("ERROR: STATE_ISSUE_NUMBER not set", file=sys.stderr) return None - + return github_api("GET", f"issues/{STATE_ISSUE_NUMBER}") -def parse_state_from_issue(issue: dict) -> dict: - """Parse YAML state from issue body.""" - body = issue.get("body", "") or "" - - # Extract YAML from code block if present - yaml_match = re.search(r"```ya?ml\n(.*?)\n```", body, re.DOTALL) - if yaml_match: - yaml_content = yaml_match.group(1) - else: - # Try to parse the whole body as YAML - yaml_content = body - +def default_state_issue_prefix() -> str: + """Return canonical prefix text for state issue layout.""" + return ( + "## Reviewer Bot State\n\n" + "> WARNING: DO NOT EDIT MANUALLY - This issue is automatically maintained by the reviewer bot.\n" + "> Use bot commands instead (see " + "[CONTRIBUTING.md](https://github.com/rustfoundation/safety-critical-rust-coding-guidelines/blob/main/CONTRIBUTING.md) " + "for details).\n\n" + "This issue tracks the round-robin assignment of reviewers for coding guidelines.\n\n" + "### Current State\n\n" + ) + + +def split_state_issue_body(body: str) -> StateIssueBodyParts: + """Split state issue body into prefix, state block, lock block, and suffix.""" + if not body: + return StateIssueBodyParts( + prefix=default_state_issue_prefix(), + state_block_inner=None, + between_state_and_lock="\n\n", + lock_block_inner=None, + suffix="\n", + has_state_markers=False, + has_lock_markers=False, + ) + + state_start = body.find(STATE_BLOCK_START_MARKER) + state_end = body.find(STATE_BLOCK_END_MARKER) + lock_start = body.find(LOCK_BLOCK_START_MARKER) + lock_end = body.find(LOCK_BLOCK_END_MARKER) + + has_state_markers = state_start >= 0 and state_end > state_start + has_lock_markers = lock_start >= 0 and lock_end > lock_start + + if has_state_markers and has_lock_markers and state_end < lock_start: + return StateIssueBodyParts( + prefix=body[:state_start], + state_block_inner=body[state_start + len(STATE_BLOCK_START_MARKER):state_end], + between_state_and_lock=body[state_end + len(STATE_BLOCK_END_MARKER):lock_start], + lock_block_inner=body[lock_start + len(LOCK_BLOCK_START_MARKER):lock_end], + suffix=body[lock_end + len(LOCK_BLOCK_END_MARKER):], + has_state_markers=True, + has_lock_markers=True, + ) + + # Partial/legacy format fallback: migrate to canonical marker layout. + return StateIssueBodyParts( + prefix=default_state_issue_prefix(), + state_block_inner=None, + between_state_and_lock="\n\n", + lock_block_inner=None, + suffix="\n", + has_state_markers=False, + has_lock_markers=False, + ) + + +def extract_fenced_block(inner_block: str, language_pattern: str) -> str | None: + """Extract fenced block content from marker inner block.""" + if not inner_block: + return None + + match = re.search( + rf"```(?:{language_pattern})\n(.*?)\n```", + inner_block, + re.DOTALL, + ) + if match: + return match.group(1) + return None + + +def normalize_lock_metadata(lock_meta: dict | None) -> dict: + """Normalize lock metadata to schema v1 keys.""" + normalized: dict[str, Any] = dict.fromkeys(LOCK_METADATA_KEYS) + normalized["schema_version"] = LOCK_SCHEMA_VERSION + + if not isinstance(lock_meta, dict): + return normalized + + for key in LOCK_METADATA_KEYS: + if key == "schema_version": + schema_value = lock_meta.get("schema_version") + if isinstance(schema_value, int): + normalized["schema_version"] = schema_value + continue + if key in lock_meta: + normalized[key] = lock_meta.get(key) + + return normalized + + +def parse_state_yaml_from_issue_body(body: str) -> dict: + """Parse YAML state from issue body markers or legacy YAML block.""" + parts = split_state_issue_body(body) + + yaml_content = None + if parts.has_state_markers and parts.state_block_inner is not None: + yaml_content = extract_fenced_block(parts.state_block_inner, "ya?ml") + + if yaml_content is None: + yaml_match = re.search(r"```ya?ml\n(.*?)\n```", body, re.DOTALL) + if yaml_match: + yaml_content = yaml_match.group(1) + else: + yaml_content = body + try: state = yaml.safe_load(yaml_content) or {} - except yaml.YAMLError as e: - print(f"WARNING: Failed to parse state YAML: {e}", file=sys.stderr) + except yaml.YAMLError as exc: + print(f"WARNING: Failed to parse state YAML: {exc}", file=sys.stderr) state = {} - + + if not isinstance(state, dict): + return {} return state +def parse_lock_metadata_from_issue_body(body: str) -> dict: + """Parse lock metadata JSON block from issue body markers.""" + parts = split_state_issue_body(body) + if not parts.has_lock_markers or parts.lock_block_inner is None: + return normalize_lock_metadata(None) + + lock_json = extract_fenced_block(parts.lock_block_inner, "json") + if lock_json is None: + return normalize_lock_metadata(None) + + try: + parsed = json.loads(lock_json) + except json.JSONDecodeError as exc: + print(f"WARNING: Failed to parse lock metadata JSON: {exc}", file=sys.stderr) + return normalize_lock_metadata(None) + + if not isinstance(parsed, dict): + return normalize_lock_metadata(None) + + return normalize_lock_metadata(parsed) + + +def render_marked_fenced_block( + start_marker: str, + end_marker: str, + language: str, + content: str, +) -> str: + """Render a marker-delimited fenced block.""" + normalized = content.rstrip("\n") + return f"{start_marker}\n```{language}\n{normalized}\n```\n{end_marker}" + + +def render_state_issue_body( + state: dict, + lock_meta: dict, + base_body: str | None = None, + *, + preserve_state_block: bool = False, +) -> str: + """Render full issue body preserving markers and surrounding text.""" + parts = split_state_issue_body(base_body or "") + + if preserve_state_block and parts.has_state_markers and parts.state_block_inner is not None: + state_section = f"{STATE_BLOCK_START_MARKER}{parts.state_block_inner}{STATE_BLOCK_END_MARKER}" + else: + yaml_content = yaml.dump( + state, + default_flow_style=False, + sort_keys=False, + allow_unicode=True, + ) + state_section = render_marked_fenced_block( + STATE_BLOCK_START_MARKER, + STATE_BLOCK_END_MARKER, + "yaml", + yaml_content, + ) + + lock_json = json.dumps(normalize_lock_metadata(lock_meta), indent=2, sort_keys=False) + lock_section = render_marked_fenced_block( + LOCK_BLOCK_START_MARKER, + LOCK_BLOCK_END_MARKER, + "json", + lock_json, + ) + + prefix = parts.prefix or default_state_issue_prefix() + between = parts.between_state_and_lock if parts.has_state_markers and parts.has_lock_markers else "\n\n" + suffix = parts.suffix if parts.has_lock_markers else "\n" + + return f"{prefix}{state_section}{between}{lock_section}{suffix}" + + +def parse_state_from_issue(issue: dict) -> dict: + """Parse YAML state from issue payload.""" + body = issue.get("body", "") or "" + return parse_state_yaml_from_issue_body(body) + + +def get_state_issue_snapshot() -> StateIssueSnapshot | None: + """Fetch state issue body plus ETag for conditional writes.""" + if not STATE_ISSUE_NUMBER: + print("ERROR: STATE_ISSUE_NUMBER not set", file=sys.stderr) + return None + + response = github_api_request( + "GET", + f"issues/{STATE_ISSUE_NUMBER}", + suppress_error_log=True, + ) + if response.status_code != 200: + print( + "ERROR: Failed to fetch state issue " + f"#{STATE_ISSUE_NUMBER} (status {response.status_code}): {response.text}", + file=sys.stderr, + ) + return None + + if not isinstance(response.payload, dict): + print("ERROR: State issue response payload was not an object", file=sys.stderr) + return None + + body = response.payload.get("body") + if not isinstance(body, str): + body = "" + + html_url = response.payload.get("html_url") + if not isinstance(html_url, str) or not html_url: + repo = f"{os.environ.get('REPO_OWNER', '')}/{os.environ.get('REPO_NAME', '')}".strip("/") + html_url = f"https://github.com/{repo}/issues/{STATE_ISSUE_NUMBER}" if repo else "" + + return StateIssueSnapshot( + body=body, + etag=response.headers.get("etag"), + html_url=html_url, + ) + + +def conditional_patch_state_issue(body: str, etag: str) -> GitHubApiResult: + """Patch state issue body with optimistic concurrency via If-Match.""" + return github_api_request( + "PATCH", + f"issues/{STATE_ISSUE_NUMBER}", + {"body": body}, + extra_headers={"If-Match": etag}, + suppress_error_log=True, + ) + + +def assert_lock_held(operation: str) -> None: + """Fail fast when mutating state outside the lease lock boundary.""" + if ACTIVE_LEASE_CONTEXT is None: + raise RuntimeError(f"Mutating path reached without lease lock: {operation}") + + def load_state() -> dict: """Load the current state from the state issue.""" default_state = { @@ -416,44 +907,350 @@ def load_state() -> dict: def save_state(state: dict) -> bool: """Save the state to the state issue. Returns True on success.""" + assert_lock_held("save_state") + if not STATE_ISSUE_NUMBER: print("ERROR: STATE_ISSUE_NUMBER not set", file=sys.stderr) return False - + state["last_updated"] = datetime.now(timezone.utc).isoformat() - # Format the issue body with YAML in a code block - yaml_content = yaml.dump(state, default_flow_style=False, sort_keys=False, - allow_unicode=True) - - body = f"""## 📊 Reviewer Bot State + for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): + snapshot = get_state_issue_snapshot() + if snapshot is None: + return False + + if not snapshot.etag: + print("ERROR: Missing ETag on state issue GET; cannot perform conditional PATCH", file=sys.stderr) + return False -> ⚠ī¸ **DO NOT EDIT MANUALLY** - This issue is automatically maintained by the reviewer bot. -> Use bot commands instead (see [CONTRIBUTING.md](https://github.com/rustfoundation/safety-critical-rust-coding-guidelines/blob/main/CONTRIBUTING.md) for details). + lock_meta = parse_lock_metadata_from_issue_body(snapshot.body) + body = render_state_issue_body(state, lock_meta, snapshot.body) -This issue tracks the round-robin assignment of reviewers for coding guidelines. + response = conditional_patch_state_issue(body, snapshot.etag) + if response.status_code == 200: + print(f"State saved to issue #{STATE_ISSUE_NUMBER} with If-Match") + return True -### Current State + if response.status_code in {409, 412}: + print( + "WARNING: State save hit optimistic concurrency conflict " + f"(status {response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + time.sleep(delay) + continue -```yaml -{yaml_content}``` + if response.status_code == 404: + print( + f"ERROR: State issue #{STATE_ISSUE_NUMBER} not found during save_state", + file=sys.stderr, + ) + return False -### What This Tracks + if response.status_code in {401, 403}: + print( + "ERROR: Permission failure while saving state issue " + f"#{STATE_ISSUE_NUMBER} (status {response.status_code}): {response.text}", + file=sys.stderr, + ) + return False -- **queue**: Active reviewers in rotation order -- **current_index**: Position in queue (who's next) -- **pass_until**: Reviewers temporarily away with return dates -- **recent_assignments**: Last {MAX_RECENT_ASSIGNMENTS} assignments for visibility -- **active_reviews**: Per-issue/PR tracking of who passed and the current designated reviewer -""" + if response.status_code == 429 or response.status_code >= 500: + if attempt < LOCK_API_RETRY_LIMIT: + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + print( + "WARNING: Retryable state issue write failure " + f"(status {response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + time.sleep(delay) + continue + print( + "ERROR: Exhausted retries while saving state issue " + f"#{STATE_ISSUE_NUMBER}; last status {response.status_code}: {response.text}", + file=sys.stderr, + ) + return False + + print( + f"ERROR: Unexpected status {response.status_code} while saving state issue: {response.text}", + file=sys.stderr, + ) + return False + + print( + f"ERROR: Failed to save state to issue #{STATE_ISSUE_NUMBER} after retries", + file=sys.stderr, + ) + return False + + +def parse_iso8601_timestamp(value: Any) -> datetime | None: + """Parse an ISO8601 timestamp and normalize to UTC timezone.""" + if not isinstance(value, str) or not value: + return None + + try: + parsed = datetime.fromisoformat(value.replace("Z", "+00:00")) + except ValueError: + return None + + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +def lock_is_currently_valid(lock_meta: dict, now: datetime | None = None) -> bool: + """Return True when lock metadata contains a non-expired lease token.""" + if not isinstance(lock_meta, dict): + return False - result = github_api("PATCH", f"issues/{STATE_ISSUE_NUMBER}", {"body": body}) - if result: - print(f"State saved to issue #{STATE_ISSUE_NUMBER}") + lock_token = lock_meta.get("lock_token") + if not isinstance(lock_token, str) or not lock_token: + return False + + expires_at = parse_iso8601_timestamp(lock_meta.get("lock_expires_at")) + if expires_at is None: + return False + + now = now or datetime.now(timezone.utc) + return expires_at > now + + +def get_lock_owner_context() -> tuple[str, str, str]: + """Get lock owner identity from workflow context.""" + run_id = ( + os.environ.get("WORKFLOW_RUN_ID", "").strip() + or os.environ.get("GITHUB_RUN_ID", "").strip() + or "local-run" + ) + workflow = ( + os.environ.get("WORKFLOW_NAME", "").strip() + or os.environ.get("GITHUB_WORKFLOW", "").strip() + or "reviewer-bot" + ) + job = ( + os.environ.get("WORKFLOW_JOB_NAME", "").strip() + or os.environ.get("GITHUB_JOB", "").strip() + or "reviewer-bot" + ) + return run_id, workflow, job + + +def build_lock_metadata( + lock_token: str, + lock_owner_run_id: str, + lock_owner_workflow: str, + lock_owner_job: str, +) -> dict: + """Build normalized lock metadata document.""" + acquired_at = datetime.now(timezone.utc) + expires_at = acquired_at.timestamp() + LOCK_LEASE_TTL_SECONDS + return normalize_lock_metadata( + { + "schema_version": LOCK_SCHEMA_VERSION, + "lock_owner_run_id": lock_owner_run_id, + "lock_owner_workflow": lock_owner_workflow, + "lock_owner_job": lock_owner_job, + "lock_token": lock_token, + "lock_acquired_at": acquired_at.isoformat(), + "lock_expires_at": datetime.fromtimestamp(expires_at, tz=timezone.utc).isoformat(), + } + ) + + +def clear_lock_metadata() -> dict: + """Return normalized empty lock metadata.""" + return normalize_lock_metadata(None) + + +def acquire_state_issue_lease_lock() -> LeaseContext: + """Acquire durable lease lock from in-repo state issue using If-Match.""" + global ACTIVE_LEASE_CONTEXT + + if ACTIVE_LEASE_CONTEXT is not None: + return ACTIVE_LEASE_CONTEXT + + lock_token = uuid.uuid4().hex + lock_owner_run_id, lock_owner_workflow, lock_owner_job = get_lock_owner_context() + wait_started_at = time.monotonic() + attempt = 0 + + while True: + attempt += 1 + elapsed = time.monotonic() - wait_started_at + if elapsed > LOCK_MAX_WAIT_SECONDS: + raise RuntimeError( + "Timed out waiting for reviewer-bot lease lock " + f"after {int(elapsed)}s (run_id={lock_owner_run_id}, token_prefix={lock_token[:8]}, " + f"state_issue={STATE_ISSUE_NUMBER})" + ) + + snapshot = get_state_issue_snapshot() + if snapshot is None: + raise RuntimeError("Failed to read state issue while acquiring lease lock") + + if not snapshot.etag: + raise RuntimeError("State issue GET response did not include ETag header") + + current_lock = parse_lock_metadata_from_issue_body(snapshot.body) + now = datetime.now(timezone.utc) + lock_valid = lock_is_currently_valid(current_lock, now) + + if not lock_valid: + desired_lock = build_lock_metadata( + lock_token, + lock_owner_run_id, + lock_owner_workflow, + lock_owner_job, + ) + state = parse_state_yaml_from_issue_body(snapshot.body) + updated_body = render_state_issue_body( + state, + desired_lock, + snapshot.body, + preserve_state_block=True, + ) + + patch_response = conditional_patch_state_issue(updated_body, snapshot.etag) + if patch_response.status_code == 200: + ACTIVE_LEASE_CONTEXT = LeaseContext( + lock_token=lock_token, + lock_owner_run_id=lock_owner_run_id, + lock_owner_workflow=lock_owner_workflow, + lock_owner_job=lock_owner_job, + state_issue_url=snapshot.html_url, + ) + print( + "Acquired reviewer-bot lease lock " + f"(run_id={lock_owner_run_id}, token_prefix={lock_token[:8]}, " + f"issue={snapshot.html_url or STATE_ISSUE_NUMBER})" + ) + return ACTIVE_LEASE_CONTEXT + + if patch_response.status_code in {409, 412}: + print( + "Lease lock acquire conflict " + f"(status {patch_response.status_code}); retrying (attempt {attempt})" + ) + elif patch_response.status_code == 404: + raise RuntimeError( + f"State issue #{STATE_ISSUE_NUMBER} not found while acquiring lease lock" + ) + elif patch_response.status_code in {401, 403}: + raise RuntimeError( + "Insufficient permission to acquire reviewer-bot lease lock " + f"(status {patch_response.status_code}): {patch_response.text}" + ) + elif patch_response.status_code == 429 or patch_response.status_code >= 500: + print( + "Retryable lease lock acquire failure " + f"(status {patch_response.status_code}); retrying (attempt {attempt})" + ) + else: + raise RuntimeError( + "Unexpected status while acquiring reviewer-bot lease lock " + f"(status {patch_response.status_code}): {patch_response.text}" + ) + else: + lock_owner = current_lock.get("lock_owner_run_id") or "unknown" + lock_expires_at = current_lock.get("lock_expires_at") or "unknown" + print( + "Reviewer-bot lease lock currently held by " + f"run_id={lock_owner} until {lock_expires_at}; waiting" + ) + + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + time.sleep(delay) + + +def release_state_issue_lease_lock() -> bool: + """Release lease lock if currently owned by this run context.""" + global ACTIVE_LEASE_CONTEXT + + if ACTIVE_LEASE_CONTEXT is None: return True - else: - print(f"ERROR: Failed to save state to issue #{STATE_ISSUE_NUMBER}", file=sys.stderr) + + context = ACTIVE_LEASE_CONTEXT + released = False + + try: + for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): + snapshot = get_state_issue_snapshot() + if snapshot is None: + break + + if not snapshot.etag: + print( + "ERROR: Missing ETag while releasing reviewer-bot lease lock", + file=sys.stderr, + ) + break + + current_lock = parse_lock_metadata_from_issue_body(snapshot.body) + current_token = current_lock.get("lock_token") + if current_token and current_token != context.lock_token: + print( + "WARNING: Lease lock token mismatch during release; " + f"expected prefix={context.lock_token[:8]}, got prefix={str(current_token)[:8]}", + file=sys.stderr, + ) + return False + + state = parse_state_yaml_from_issue_body(snapshot.body) + updated_body = render_state_issue_body( + state, + clear_lock_metadata(), + snapshot.body, + preserve_state_block=True, + ) + patch_response = conditional_patch_state_issue(updated_body, snapshot.etag) + + if patch_response.status_code == 200: + released = True + print( + "Released reviewer-bot lease lock " + f"(run_id={context.lock_owner_run_id}, token_prefix={context.lock_token[:8]})" + ) + return True + + if patch_response.status_code in {409, 412, 429} or patch_response.status_code >= 500: + print( + "Retryable lease lock release failure " + f"(status {patch_response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + time.sleep(delay) + continue + + if patch_response.status_code in {401, 403, 404}: + print( + "ERROR: Hard failure releasing reviewer-bot lease lock " + f"(status {patch_response.status_code}): {patch_response.text}", + file=sys.stderr, + ) + break + + print( + "ERROR: Unexpected status while releasing reviewer-bot lease lock " + f"(status {patch_response.status_code}): {patch_response.text}", + file=sys.stderr, + ) + break + return False + finally: + if not released: + print( + "ERROR: Lease lock release failed " + f"(run_id={context.lock_owner_run_id}, token_prefix={context.lock_token[:8]}, " + f"state_issue_url={context.state_issue_url})", + file=sys.stderr, + ) + ACTIVE_LEASE_CONTEXT = None def sync_members_with_queue(state: dict) -> tuple[dict, list[str]]: @@ -942,9 +1739,9 @@ def handle_pass_command(state: dict, issue_number: int, comment_author: str, # Unassign the passed reviewer first (best effort - may fail if no permissions) unassign_reviewer(issue_number, passed_reviewer) - # Assign the substitute to this issue (best effort) + # Assign the substitute to this issue. is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, next_reviewer) # Don't fail if this doesn't work + assignment_attempt = request_reviewer_assignment(issue_number, next_reviewer) # Track the new reviewer in our state (this is the source of truth) set_current_reviewer(state, issue_number, next_reviewer) @@ -952,16 +1749,28 @@ def handle_pass_command(state: dict, issue_number: int, comment_author: str, # Record the assignment record_assignment(state, next_reviewer, issue_number, "pr" if is_pr else "issue") + assignment_line = f"@{next_reviewer} is now assigned as the reviewer." + if not assignment_attempt.success: + failure_comment = get_assignment_failure_comment(next_reviewer, assignment_attempt) + if failure_comment: + assignment_line = failure_comment + else: + status_text = assignment_attempt.status_code or "unknown" + assignment_line = ( + f"@{next_reviewer} is designated as reviewer in bot state, but GitHub " + f"assignment could not be confirmed (status {status_text})." + ) + reason_text = f" Reason: {reason}" if reason else "" if is_first_pass: return (f"✅ @{passed_reviewer} has passed this review.{reason_text}\n\n" - f"@{next_reviewer} is now assigned as the reviewer.\n\n" + f"{assignment_line}\n\n" f"_@{passed_reviewer} is next in queue for future issues._"), True else: # Get the original passer (first in the skip list) original_passer = issue_data["skipped"][0] return (f"✅ @{passed_reviewer} has passed this review.{reason_text}\n\n" - f"@{next_reviewer} is now assigned as the reviewer.\n\n" + f"{assignment_line}\n\n" f"_@{original_passer} remains next in queue for future issues._"), True @@ -1055,11 +1864,23 @@ def handle_pass_until_command(state: dict, issue_number: int, comment_author: st if next_reviewer: is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, next_reviewer) + assignment_attempt = request_reviewer_assignment(issue_number, next_reviewer) set_current_reviewer(state, issue_number, next_reviewer) record_assignment(state, next_reviewer, issue_number, "pr" if is_pr else "issue") - reassigned_msg = f"\n\n@{next_reviewer} has been assigned as the new reviewer for this issue." + if assignment_attempt.success: + reassigned_msg = f"\n\n@{next_reviewer} has been assigned as the new reviewer for this issue." + else: + failure_comment = get_assignment_failure_comment(next_reviewer, assignment_attempt) + if failure_comment: + reassigned_msg = f"\n\n{failure_comment}" + else: + status_text = assignment_attempt.status_code or "unknown" + reassigned_msg = ( + "\n\n" + f"@{next_reviewer} is designated as the new reviewer in bot state, but " + f"GitHub assignment is not confirmed (status {status_text})." + ) else: # Clear the current reviewer if "active_reviews" in state and issue_key in state["active_reviews"]: @@ -1481,9 +2302,9 @@ def handle_claim_command(state: dict, issue_number: int, for assignee in current_assignees: unassign_reviewer(issue_number, assignee) - # Assign the claimer (best effort) + # Assign the claimer. is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, comment_author) + assignment_attempt = request_reviewer_assignment(issue_number, comment_author) # Track the reviewer in our state set_current_reviewer(state, issue_number, comment_author, assignment_method="claim") @@ -1496,7 +2317,13 @@ def handle_claim_command(state: dict, issue_number: int, else: prev_text = "" - return f"✅ @{comment_author} has claimed this review{prev_text}.", True + response = f"✅ @{comment_author} has claimed this review{prev_text}." + if not assignment_attempt.success: + failure_comment = get_assignment_failure_comment(comment_author, assignment_attempt) + if failure_comment: + response = f"{response}\n\n{failure_comment}" + + return response, True def handle_release_command(state: dict, issue_number: int, @@ -1647,9 +2474,9 @@ def handle_assign_command(state: dict, issue_number: int, for assignee in current_assignees: unassign_reviewer(issue_number, assignee) - # Assign the specified user (best effort) + # Assign the specified user. is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, username) + assignment_attempt = request_reviewer_assignment(issue_number, username) # Track the reviewer in our state set_current_reviewer(state, issue_number, username, assignment_method="manual") @@ -1662,7 +2489,18 @@ def handle_assign_command(state: dict, issue_number: int, else: prev_text = "" - return f"✅ @{username} has been assigned as reviewer{prev_text}.", True + if assignment_attempt.success: + return f"✅ @{username} has been assigned as reviewer{prev_text}.", True + + response = ( + f"✅ @{username} remains designated as reviewer in bot state{prev_text}. " + "GitHub reviewer assignment could not be completed." + ) + failure_comment = get_assignment_failure_comment(username, assignment_attempt) + if failure_comment: + response = f"{response}\n\n{failure_comment}" + + return response, True def handle_assign_from_queue_command(state: dict, issue_number: int) -> tuple[str, bool]: @@ -1689,9 +2527,9 @@ def handle_assign_from_queue_command(state: dict, issue_number: int) -> tuple[st return ("❌ No reviewers available in the queue. " f"Please use `{BOT_MENTION} /sync-members` to update the queue."), False - # Assign the reviewer (best effort - may fail if no permissions) + # Assign the reviewer. is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, next_reviewer) + assignment_attempt = request_reviewer_assignment(issue_number, next_reviewer) # Track the reviewer in our state (source of truth for pass command) set_current_reviewer(state, issue_number, next_reviewer) @@ -1704,15 +2542,26 @@ def handle_assign_from_queue_command(state: dict, issue_number: int) -> tuple[st else: prev_text = "" + failure_comment = get_assignment_failure_comment(next_reviewer, assignment_attempt) + if failure_comment: + post_comment(issue_number, failure_comment) + # Post the appropriate guidance if is_pr: - guidance = get_pr_guidance(next_reviewer, issue_author) + if assignment_attempt.success: + guidance = get_pr_guidance(next_reviewer, issue_author) + post_comment(issue_number, guidance) else: guidance = get_issue_guidance(next_reviewer, issue_author) + post_comment(issue_number, guidance) - post_comment(issue_number, guidance) + if assignment_attempt.success: + return f"✅ @{next_reviewer} (next in queue) has been assigned as reviewer{prev_text}.", True - return f"✅ @{next_reviewer} (next in queue) has been assigned as reviewer{prev_text}.", True + return ( + f"✅ @{next_reviewer} remains designated as reviewer in bot state{prev_text}." + " GitHub reviewer assignment could not be completed." + ), True # ============================================================================== @@ -1741,6 +2590,11 @@ def ensure_review_entry(state: dict, issue_number: int, create: bool = False) -> "review_completed_at": None, "review_completed_by": None, "review_completion_source": None, + "mandatory_approver_required": False, + "mandatory_approver_label_applied_at": None, + "mandatory_approver_pinged_at": None, + "mandatory_approver_satisfied_by": None, + "mandatory_approver_satisfied_at": None, } state["active_reviews"][issue_key] = review_entry elif isinstance(review_entry, list): @@ -1754,6 +2608,11 @@ def ensure_review_entry(state: dict, issue_number: int, create: bool = False) -> "review_completed_at": None, "review_completed_by": None, "review_completion_source": None, + "mandatory_approver_required": False, + "mandatory_approver_label_applied_at": None, + "mandatory_approver_pinged_at": None, + "mandatory_approver_satisfied_by": None, + "mandatory_approver_satisfied_at": None, } state["active_reviews"][issue_key] = review_entry @@ -1772,6 +2631,11 @@ def ensure_review_entry(state: dict, issue_number: int, create: bool = False) -> "review_completed_at": None, "review_completed_by": None, "review_completion_source": None, + "mandatory_approver_required": False, + "mandatory_approver_label_applied_at": None, + "mandatory_approver_pinged_at": None, + "mandatory_approver_satisfied_by": None, + "mandatory_approver_satisfied_at": None, } for field, default in required_fields.items(): @@ -1806,6 +2670,11 @@ def set_current_reviewer(state: dict, issue_number: int, reviewer: str, review_data["review_completed_at"] = None review_data["review_completed_by"] = None review_data["review_completion_source"] = None + review_data["mandatory_approver_required"] = False + review_data["mandatory_approver_label_applied_at"] = None + review_data["mandatory_approver_pinged_at"] = None + review_data["mandatory_approver_satisfied_by"] = None + review_data["mandatory_approver_satisfied_at"] = None def update_reviewer_activity(state: dict, issue_number: int, reviewer: str) -> bool: @@ -1860,6 +2729,131 @@ def mark_review_complete( return True +def is_triage_or_higher(username: str) -> bool: + """Return True when user has triage+ permissions.""" + return check_user_permission(username, "triage") + + +def trigger_mandatory_approver_escalation(state: dict, issue_number: int) -> bool: + """Open mandatory triage-approval escalation for a PR review cycle.""" + review_data = ensure_review_entry(state, issue_number, create=True) + if review_data is None: + return False + + now = datetime.now(timezone.utc).isoformat() + state_changed = False + + if not review_data.get("mandatory_approver_required"): + review_data["mandatory_approver_required"] = True + review_data["mandatory_approver_satisfied_by"] = None + review_data["mandatory_approver_satisfied_at"] = None + state_changed = True + + label_ensure_ok = ensure_label_exists(MANDATORY_TRIAGE_APPROVER_LABEL) + if label_ensure_ok: + try: + if add_label_with_status(issue_number, MANDATORY_TRIAGE_APPROVER_LABEL): + if review_data.get("mandatory_approver_label_applied_at") is None: + review_data["mandatory_approver_label_applied_at"] = now + state_changed = True + except RuntimeError as exc: + print( + f"WARNING: Unable to apply escalation label on #{issue_number}: {exc}", + file=sys.stderr, + ) + else: + print( + "WARNING: Escalation label ensure/create failed; proceeding with comment-only escalation", + file=sys.stderr, + ) + + if review_data.get("mandatory_approver_pinged_at") is None: + if post_comment(issue_number, MANDATORY_TRIAGE_ESCALATION_TEMPLATE): + review_data["mandatory_approver_pinged_at"] = now + state_changed = True + + return state_changed + + +def satisfy_mandatory_approver_requirement( + state: dict, + issue_number: int, + approver: str, +) -> bool: + """Close mandatory triage escalation after first triage+ approval.""" + review_data = ensure_review_entry(state, issue_number, create=True) + if review_data is None: + return False + + if not review_data.get("mandatory_approver_required"): + return False + + if review_data.get("mandatory_approver_satisfied_at"): + return False + + now = datetime.now(timezone.utc).isoformat() + review_data["mandatory_approver_required"] = False + review_data["mandatory_approver_satisfied_by"] = approver + review_data["mandatory_approver_satisfied_at"] = now + + try: + remove_label_with_status(issue_number, MANDATORY_TRIAGE_APPROVER_LABEL) + except RuntimeError as exc: + print( + f"WARNING: Unable to remove escalation label on #{issue_number}: {exc}", + file=sys.stderr, + ) + + post_comment(issue_number, MANDATORY_TRIAGE_SATISFIED_TEMPLATE.format(approver=approver)) + return True + + +def handle_pr_approved_review( + state: dict, + issue_number: int, + review_author: str, + completion_source: str, +) -> bool: + """Apply approval transitions for designated and mandatory triage flows.""" + review_data = ensure_review_entry(state, issue_number) + if review_data is None: + print(f"No active review entry for #{issue_number}") + return False + + current_reviewer = review_data.get("current_reviewer") + author_is_designated = ( + isinstance(current_reviewer, str) + and current_reviewer.lower() == review_author.lower() + ) + + author_is_triage = is_triage_or_higher(review_author) + state_changed = False + + if author_is_designated: + if mark_review_complete(state, issue_number, review_author, completion_source): + state_changed = True + + if author_is_triage: + if satisfy_mandatory_approver_requirement(state, issue_number, review_author): + state_changed = True + return state_changed + + if trigger_mandatory_approver_escalation(state, issue_number): + state_changed = True + return state_changed + + if review_data.get("mandatory_approver_required") and author_is_triage: + if satisfy_mandatory_approver_requirement(state, issue_number, review_author): + state_changed = True + return state_changed + + print( + f"Ignoring approved review from @{review_author} on #{issue_number}; " + f"designated reviewer is @{current_reviewer}" + ) + return state_changed + + def parse_github_timestamp(value: str | None) -> datetime | None: """Parse a GitHub timestamp string into a datetime.""" if not isinstance(value, str) or not value: @@ -1903,6 +2897,44 @@ def get_latest_review_by_reviewer(reviews: list[dict], reviewer: str) -> dict | return latest_review +def find_triage_approval_after( + reviews: list[dict], + since: datetime | None, +) -> tuple[str, datetime] | None: + """Find the first triage+ approval submitted after `since`.""" + permission_cache: dict[str, bool] = {} + approvals: list[tuple[datetime, int, str]] = [] + + for index, review in enumerate(reviews): + state = str(review.get("state", "")).upper() + if state != "APPROVED": + continue + + author = review.get("user", {}).get("login") + if not isinstance(author, str) or not author: + continue + + submitted_at = parse_github_timestamp(review.get("submitted_at")) + if submitted_at is None: + continue + + if since is not None and submitted_at <= since: + continue + + approvals.append((submitted_at, index, author)) + + approvals.sort(key=lambda item: (item[0], item[1])) + + for submitted_at, _, author in approvals: + cache_key = author.lower() + if cache_key not in permission_cache: + permission_cache[cache_key] = is_triage_or_higher(author) + if permission_cache[cache_key]: + return author, submitted_at + + return None + + def reconcile_active_review_entry( state: dict, issue_number: int, @@ -1926,7 +2958,7 @@ def reconcile_active_review_entry( False, ) - if review_data.get("review_completed_at"): + if review_data.get("review_completed_at") and not review_data.get("mandatory_approver_required"): return f"ℹī¸ Review for #{issue_number} is already marked complete; no changes made.", True, False if require_pull_request_context: @@ -1943,55 +2975,68 @@ def reconcile_active_review_entry( if reviews is None: return f"❌ Failed to fetch reviews for PR #{issue_number}; cannot run `/rectify`.", False, False + state_changed = False + messages: list[str] = [] + latest_review = get_latest_review_by_reviewer(reviews, assigned_reviewer) if latest_review is None: - return ( - f"ℹī¸ No review by assigned reviewer @{assigned_reviewer} was found on PR #{issue_number}; " - "nothing to rectify.", - True, - False, - ) - - latest_state = str(latest_review.get("state", "")).upper() - if latest_state == "APPROVED": - changed = mark_review_complete( - state, - issue_number, - assigned_reviewer, - completion_source, + messages.append( + f"No review by assigned reviewer @{assigned_reviewer} was found on PR #{issue_number}." ) - if changed: - return ( - f"✅ Rectified PR #{issue_number}: latest review by @{assigned_reviewer} is " - "`APPROVED`; marked review complete.", - True, - True, + else: + latest_state = str(latest_review.get("state", "")).upper() + if latest_state == "APPROVED": + changed = handle_pr_approved_review( + state, + issue_number, + assigned_reviewer, + completion_source, ) - return f"ℹī¸ Review for #{issue_number} is already complete; no changes made.", True, False - - if latest_state in {"COMMENTED", "CHANGES_REQUESTED"}: - changed = update_reviewer_activity(state, issue_number, assigned_reviewer) - if changed: - return ( - f"✅ Rectified PR #{issue_number}: latest review by @{assigned_reviewer} is " - f"`{latest_state}`; refreshed reviewer activity.", - True, - True, + if changed: + state_changed = True + messages.append( + f"latest review by @{assigned_reviewer} is `APPROVED`; applied approval transitions" + ) + else: + messages.append( + f"latest review by @{assigned_reviewer} is `APPROVED`, but state already reflected it" + ) + elif latest_state in {"COMMENTED", "CHANGES_REQUESTED"}: + changed = update_reviewer_activity(state, issue_number, assigned_reviewer) + if changed: + state_changed = True + messages.append( + f"latest review by @{assigned_reviewer} is `{latest_state}`; refreshed reviewer activity" + ) + else: + messages.append( + f"latest assigned-reviewer state is `{latest_state}` and no update was needed" + ) + else: + state_name = latest_state or "UNKNOWN" + messages.append( + f"latest review by @{assigned_reviewer} is `{state_name}` and no reconciliation transition applies" ) - return ( - f"ℹī¸ Latest assigned-reviewer state is `{latest_state}` for #{issue_number}, but " - "no state update was needed.", - True, - False, + + review_data = ensure_review_entry(state, issue_number, create=True) + if review_data and review_data.get("mandatory_approver_required"): + escalation_opened_at = ( + parse_iso8601_timestamp(review_data.get("mandatory_approver_pinged_at")) + or parse_iso8601_timestamp(review_data.get("mandatory_approver_label_applied_at")) ) + triage_approval = find_triage_approval_after(reviews, escalation_opened_at) + if triage_approval is not None: + approver, _ = triage_approval + if satisfy_mandatory_approver_requirement(state, issue_number, approver): + state_changed = True + messages.append(f"mandatory triage approval satisfied by @{approver}") - state_name = latest_state or "UNKNOWN" - return ( - f"ℹī¸ Latest review by @{assigned_reviewer} on PR #{issue_number} is `{state_name}`; " - "no reconciliation transition applies.", - True, - False, - ) + if state_changed: + detail = "; ".join(messages) if messages else "applied state reconciliation transitions" + return f"✅ Rectified PR #{issue_number}: {detail}.", True, True + + detail = "; ".join(messages) if messages else "no reconciliation transitions applied" + return f"ℹī¸ Rectify checked PR #{issue_number}: {detail}.", True, False def handle_rectify_command(state: dict, issue_number: int, comment_author: str) -> tuple[str, bool, bool]: @@ -2188,6 +3233,8 @@ def handle_issue_or_pr_opened(state: dict) -> bool: Returns True if we took action, False otherwise. """ + assert_lock_held("handle_issue_or_pr_opened") + issue_number = int(os.environ.get("ISSUE_NUMBER", 0)) if not issue_number: print("No issue number found") @@ -2241,9 +3288,9 @@ def handle_issue_or_pr_opened(state: dict) -> bool: f"Please use `{BOT_MENTION} /sync-members` to update the queue.") return False - # Assign the reviewer (best effort - may fail if no permissions) + # Assign the reviewer. is_pr = os.environ.get("IS_PULL_REQUEST", "false").lower() == "true" - assign_reviewer(issue_number, reviewer) + assignment_attempt = request_reviewer_assignment(issue_number, reviewer) # Track the reviewer in our state (source of truth for pass command) set_current_reviewer(state, issue_number, reviewer) @@ -2251,15 +3298,21 @@ def handle_issue_or_pr_opened(state: dict) -> bool: # Record the assignment record_assignment(state, reviewer, issue_number, "pr" if is_pr else "issue") + failure_comment = get_assignment_failure_comment(reviewer, assignment_attempt) + if failure_comment: + post_comment(issue_number, failure_comment) + # Post guidance comment if is_pr: - guidance = get_pr_guidance(reviewer, issue_author) - elif FLS_AUDIT_LABEL in labels: - guidance = get_fls_audit_guidance(reviewer, issue_author) + if assignment_attempt.success: + guidance = get_pr_guidance(reviewer, issue_author) + post_comment(issue_number, guidance) else: - guidance = get_issue_guidance(reviewer, issue_author) - - post_comment(issue_number, guidance) + if FLS_AUDIT_LABEL in labels: + guidance = get_fls_audit_guidance(reviewer, issue_author) + else: + guidance = get_issue_guidance(reviewer, issue_author) + post_comment(issue_number, guidance) return True @@ -2271,6 +3324,8 @@ def handle_labeled_event(state: dict) -> bool: We already know from LABEL_NAME that the correct label was added, so we skip the label check that handle_issue_or_pr_opened does. """ + assert_lock_held("handle_labeled_event") + issue_number = int(os.environ.get("ISSUE_NUMBER", 0)) if not issue_number: print("No issue number found") @@ -2331,8 +3386,8 @@ def handle_labeled_event(state: dict) -> bool: f"Please use `{BOT_MENTION} /sync-members` to update the queue.") return False - # Assign the reviewer (best effort - may fail if no permissions) - assign_reviewer(issue_number, reviewer) + # Assign the reviewer. + assignment_attempt = request_reviewer_assignment(issue_number, reviewer) # Track the reviewer in our state set_current_reviewer(state, issue_number, reviewer) @@ -2340,21 +3395,29 @@ def handle_labeled_event(state: dict) -> bool: # Record the assignment record_assignment(state, reviewer, issue_number, "pr" if is_pr else "issue") + failure_comment = get_assignment_failure_comment(reviewer, assignment_attempt) + if failure_comment: + post_comment(issue_number, failure_comment) + # Post guidance comment if is_pr: - guidance = get_pr_guidance(reviewer, issue_author) - elif label_name == FLS_AUDIT_LABEL: - guidance = get_fls_audit_guidance(reviewer, issue_author) + if assignment_attempt.success: + guidance = get_pr_guidance(reviewer, issue_author) + post_comment(issue_number, guidance) else: - guidance = get_issue_guidance(reviewer, issue_author) - - post_comment(issue_number, guidance) + if label_name == FLS_AUDIT_LABEL: + guidance = get_fls_audit_guidance(reviewer, issue_author) + else: + guidance = get_issue_guidance(reviewer, issue_author) + post_comment(issue_number, guidance) return True def handle_pull_request_review_event(state: dict) -> bool: """Handle submitted PR reviews for activity and completion tracking.""" + assert_lock_held("handle_pull_request_review_event") + issue_number = int(os.environ.get("ISSUE_NUMBER", 0)) if not issue_number: print("No issue number found") @@ -2382,15 +3445,8 @@ def handle_pull_request_review_event(state: dict) -> bool: return False current_reviewer = review_data.get("current_reviewer") - if not current_reviewer or current_reviewer.lower() != review_author.lower(): - print( - f"Ignoring review from @{review_author} on #{issue_number}; " - f"current reviewer is @{current_reviewer}" - ) - return False - if review_state == "APPROVED": - return mark_review_complete( + return handle_pr_approved_review( state, issue_number, review_author, @@ -2398,6 +3454,12 @@ def handle_pull_request_review_event(state: dict) -> bool: ) if review_state in {"COMMENTED", "CHANGES_REQUESTED"}: + if not current_reviewer or current_reviewer.lower() != review_author.lower(): + print( + f"Ignoring review from @{review_author} on #{issue_number}; " + f"current reviewer is @{current_reviewer}" + ) + return False return update_reviewer_activity(state, issue_number, review_author) print(f"Ignoring review state '{review_state}' for #{issue_number}") @@ -2406,6 +3468,8 @@ def handle_pull_request_review_event(state: dict) -> bool: def handle_workflow_run_event(state: dict) -> bool: """Handle trusted second-hop workflow_run reconciliation.""" + assert_lock_held("handle_workflow_run_event") + workflow_run_event = os.environ.get("WORKFLOW_RUN_EVENT", "").strip() if workflow_run_event != "pull_request_review": observed = workflow_run_event or "" @@ -2445,9 +3509,11 @@ def handle_closed_event(state: dict) -> bool: Handle when an issue or PR is closed. Cleans up the active_reviews entry to prevent state from growing indefinitely. - + Returns True if we modified state, False otherwise. """ + assert_lock_held("handle_closed_event") + issue_number = int(os.environ.get("ISSUE_NUMBER", 0)) if not issue_number: print("No issue number found for closed event") @@ -2470,6 +3536,8 @@ def handle_comment_event(state: dict) -> bool: Returns True if we took action, False otherwise. """ + assert_lock_held("handle_comment_event") + comment_body = os.environ.get("COMMENT_BODY", "") comment_author = os.environ.get("COMMENT_AUTHOR", "") comment_id = os.environ.get("COMMENT_ID", "") @@ -2611,6 +3679,8 @@ def handle_comment_event(state: dict) -> bool: def handle_manual_dispatch(state: dict) -> bool: """Handle manual workflow dispatch.""" + assert_lock_held("handle_manual_dispatch") + action = os.environ.get("MANUAL_ACTION", "") if action == "sync-members": @@ -2638,9 +3708,11 @@ def handle_scheduled_check(state: dict) -> bool: 1. Checks all active reviews for overdue ones 2. Posts warnings for reviews that are 14+ days overdue 3. Posts transition notices and reassigns for 28+ days overdue - + Returns True if any action was taken, False otherwise. """ + assert_lock_held("handle_scheduled_check") + print("Running scheduled check for overdue reviews...") overdue_reviews = check_overdue_reviews(state) @@ -2688,7 +3760,7 @@ def handle_scheduled_check(state: dict) -> bool: unassign_reviewer(issue_number, reviewer) # Assign new reviewer - assign_reviewer(issue_number, next_reviewer) + assignment_attempt = request_reviewer_assignment(issue_number, next_reviewer) set_current_reviewer(state, issue_number, next_reviewer) # Track the skip @@ -2697,6 +3769,10 @@ def handle_scheduled_check(state: dict) -> bool: state["active_reviews"][issue_key]["skipped"].append(reviewer) # Post assignment comment (assume issue since we don't track type here) + failure_comment = get_assignment_failure_comment(next_reviewer, assignment_attempt) + if failure_comment: + post_comment(issue_number, failure_comment) + guidance = get_issue_guidance(next_reviewer, "the contributor") post_comment(issue_number, guidance) @@ -2717,6 +3793,21 @@ def handle_scheduled_check(state: dict) -> bool: # ============================================================================== +def event_requires_lease_lock(event_name: str, event_action: str) -> bool: + """Return True for event paths that may mutate reviewer-bot state.""" + if event_name in {"issues", "pull_request_target"}: + return event_action in {"opened", "labeled", "closed"} + if event_name == "issue_comment": + return event_action == "created" + if event_name == "pull_request_review": + return event_action == "submitted" + if event_name == "workflow_run": + return event_action == "completed" + if event_name in {"workflow_dispatch", "schedule"}: + return True + return False + + def main(): """Main entry point for the reviewer bot.""" event_name = os.environ.get("EVENT_NAME", "") @@ -2724,78 +3815,111 @@ def main(): print(f"Event: {event_name}, Action: {event_action}") - # Load current state - state = load_state() - - # Process any expired pass-until entries - state, restored = process_pass_until_expirations(state) - if restored: - print(f"Restored from pass-until: {restored}") - - # Always sync members on any event - state, sync_changes = sync_members_with_queue(state) - if sync_changes: - print(f"Members sync changes: {sync_changes}") + lock_required = event_requires_lease_lock(event_name, event_action) + lock_acquired = False + release_failed = False + exit_code = 0 - # Handle the event state_changed = False + sync_changes: list[str] = [] + restored: list[str] = [] - if event_name == "issues": - if event_action == "opened": - state_changed = handle_issue_or_pr_opened(state) - elif event_action == "labeled": - state_changed = handle_labeled_event(state) - elif event_action == "closed": - state_changed = handle_closed_event(state) - - elif event_name == "pull_request_target": - if event_action == "opened": - state_changed = handle_issue_or_pr_opened(state) - elif event_action == "labeled": - state_changed = handle_labeled_event(state) - elif event_action == "closed": - state_changed = handle_closed_event(state) - - elif event_name == "pull_request_review": - if event_action == "submitted": - state_changed = handle_pull_request_review_event(state) - - elif event_name == "issue_comment": - if event_action == "created": - state_changed = handle_comment_event(state) - - elif event_name == "workflow_dispatch": - state_changed = handle_manual_dispatch(state) - - elif event_name == "schedule": - # Nightly check for overdue reviews - state_changed = handle_scheduled_check(state) - - elif event_name == "workflow_run": - if event_action == "completed": - try: + try: + if lock_required: + acquire_state_issue_lease_lock() + lock_acquired = True + + # Load current state + state = load_state() + + if lock_required: + # Process any expired pass-until entries + state, restored = process_pass_until_expirations(state) + if restored: + print(f"Restored from pass-until: {restored}") + + # Always sync members on mutating paths + state, sync_changes = sync_members_with_queue(state) + if sync_changes: + print(f"Members sync changes: {sync_changes}") + + # Handle the event + if event_name == "issues": + if event_action == "opened": + state_changed = handle_issue_or_pr_opened(state) + elif event_action == "labeled": + state_changed = handle_labeled_event(state) + elif event_action == "closed": + state_changed = handle_closed_event(state) + + elif event_name == "pull_request_target": + if event_action == "opened": + state_changed = handle_issue_or_pr_opened(state) + elif event_action == "labeled": + state_changed = handle_labeled_event(state) + elif event_action == "closed": + state_changed = handle_closed_event(state) + + elif event_name == "pull_request_review": + if event_action == "submitted": + state_changed = handle_pull_request_review_event(state) + + elif event_name == "issue_comment": + if event_action == "created": + state_changed = handle_comment_event(state) + + elif event_name == "workflow_dispatch": + state_changed = handle_manual_dispatch(state) + + elif event_name == "schedule": + # Nightly check for overdue reviews + state_changed = handle_scheduled_check(state) + + elif event_name == "workflow_run": + if event_action == "completed": state_changed = handle_workflow_run_event(state) - except RuntimeError as exc: - print(f"ERROR: {exc}", file=sys.stderr) - sys.exit(1) - - # Save state if changed (or if we synced members/pass-until) - if state_changed or sync_changes or restored: - print("State updates detected; attempting to persist reviewer-bot state.") - if not save_state(state): - print( - "ERROR: State updates were computed but could not be persisted. " - "Failing this run to avoid silent success.", - file=sys.stderr, - ) - sys.exit(1) - # Set output for the workflow - with open(os.environ.get("GITHUB_OUTPUT", "/dev/null"), "a") as f: - f.write("state_changed=true\n") - else: - with open(os.environ.get("GITHUB_OUTPUT", "/dev/null"), "a") as f: - f.write("state_changed=false\n") + # Save state if changed (or if we synced members/pass-until) + if state_changed or sync_changes or restored: + if not lock_acquired: + raise RuntimeError( + "State mutation reached save path without lease lock. " + "Acquire lock before mutating state." + ) + + print("State updates detected; attempting to persist reviewer-bot state.") + if not save_state(state): + raise RuntimeError( + "State updates were computed but could not be persisted. " + "Failing this run to avoid silent success." + ) + + with open(os.environ.get("GITHUB_OUTPUT", "/dev/null"), "a") as f: + f.write("state_changed=true\n") + else: + with open(os.environ.get("GITHUB_OUTPUT", "/dev/null"), "a") as f: + f.write("state_changed=false\n") + + except RuntimeError as exc: + print(f"ERROR: {exc}", file=sys.stderr) + exit_code = 1 + except Exception as exc: # pragma: no cover - defensive hard-fail path + print(f"ERROR: Unexpected reviewer-bot failure: {exc}", file=sys.stderr) + exit_code = 1 + finally: + if lock_acquired: + if not release_state_issue_lease_lock(): + release_failed = True + + if release_failed: + print( + "ERROR: Failed to release reviewer-bot lease lock after processing event.", + file=sys.stderr, + ) + exit_code = 1 + + if exit_code: + sys.exit(exit_code) if __name__ == "__main__": From 32275810de6b59879882cb40a623f103093dfcc0 Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Thu, 19 Feb 2026 00:23:31 +0900 Subject: [PATCH 2/3] fix(reviewer-bot): serialize mutating events with git-ref CAS lock --- .github/workflows/reviewer-bot-reconcile.yml | 2 +- .github/workflows/reviewer-bot.yml | 6 - scripts/reviewer_bot.py | 592 ++++++++++++++++--- 3 files changed, 501 insertions(+), 99 deletions(-) diff --git a/.github/workflows/reviewer-bot-reconcile.yml b/.github/workflows/reviewer-bot-reconcile.yml index 325e505e7..c954dc8b9 100644 --- a/.github/workflows/reviewer-bot-reconcile.yml +++ b/.github/workflows/reviewer-bot-reconcile.yml @@ -8,7 +8,7 @@ on: permissions: issues: write pull-requests: write - contents: read + contents: write actions: read env: diff --git a/.github/workflows/reviewer-bot.yml b/.github/workflows/reviewer-bot.yml index 923a0613e..49554b727 100644 --- a/.github/workflows/reviewer-bot.yml +++ b/.github/workflows/reviewer-bot.yml @@ -35,12 +35,6 @@ on: - show-state - check-overdue -# Per-issue/PR concurrency only reduces duplicate same-item runs. -# Global state serialization is enforced by reviewer_bot.py lease locking on issue #314. -concurrency: - group: reviewer-bot-${{ github.event.issue.number || github.event.pull_request.number || 'manual' }} - cancel-in-progress: false - permissions: issues: write pull-requests: write diff --git a/scripts/reviewer_bot.py b/scripts/reviewer_bot.py index eab96c77e..fe70ff05a 100644 --- a/scripts/reviewer_bot.py +++ b/scripts/reviewer_bot.py @@ -96,6 +96,16 @@ LOCK_RETRY_BASE_SECONDS = float(os.environ.get("REVIEWER_BOT_LOCK_RETRY_SECONDS", "2")) LOCK_MAX_WAIT_SECONDS = int(os.environ.get("REVIEWER_BOT_LOCK_MAX_WAIT_SECONDS", "1200")) LOCK_API_RETRY_LIMIT = int(os.environ.get("REVIEWER_BOT_LOCK_API_RETRY_LIMIT", "5")) +LOCK_RENEWAL_WINDOW_SECONDS = int( + os.environ.get("REVIEWER_BOT_LOCK_RENEWAL_WINDOW_SECONDS", "60") +) +LOCK_REF_NAME = os.environ.get("REVIEWER_BOT_LOCK_REF_NAME", "heads/reviewer-bot-state-lock") +LOCK_REF_BOOTSTRAP_BRANCH = os.environ.get("REVIEWER_BOT_LOCK_BOOTSTRAP_BRANCH", "main") +LOCK_COMMIT_MARKER = "reviewer-bot-lock-v1" + +EVENT_INTENT_MUTATING = "mutating" +EVENT_INTENT_NON_MUTATING_DEFER = "non_mutating_defer" +EVENT_INTENT_NON_MUTATING_READONLY = "non_mutating_readonly" MANDATORY_TRIAGE_APPROVER_LABEL = "triage approver required" MANDATORY_TRIAGE_PING_TARGETS = [ @@ -122,6 +132,7 @@ LOCK_METADATA_KEYS = [ "schema_version", + "lock_state", "lock_owner_run_id", "lock_owner_workflow", "lock_owner_job", @@ -205,6 +216,8 @@ class LeaseContext: lock_owner_workflow: str lock_owner_job: str state_issue_url: str + lock_ref: str = "" + lock_expires_at: str | None = None ACTIVE_LEASE_CONTEXT: LeaseContext | None = None @@ -814,7 +827,7 @@ def parse_state_from_issue(issue: dict) -> dict: def get_state_issue_snapshot() -> StateIssueSnapshot | None: - """Fetch state issue body plus ETag for conditional writes.""" + """Fetch state issue body plus metadata for state writes.""" if not STATE_ISSUE_NUMBER: print("ERROR: STATE_ISSUE_NUMBER not set", file=sys.stderr) return None @@ -852,19 +865,22 @@ def get_state_issue_snapshot() -> StateIssueSnapshot | None: ) -def conditional_patch_state_issue(body: str, etag: str) -> GitHubApiResult: - """Patch state issue body with optimistic concurrency via If-Match.""" +def conditional_patch_state_issue(body: str, etag: str | None = None) -> GitHubApiResult: + """Patch state issue body. + + The `etag` argument is retained for compatibility with tests and call sites, + but state serialization is handled by the dedicated lock backend. + """ return github_api_request( "PATCH", f"issues/{STATE_ISSUE_NUMBER}", {"body": body}, - extra_headers={"If-Match": etag}, suppress_error_log=True, ) def assert_lock_held(operation: str) -> None: - """Fail fast when mutating state outside the lease lock boundary.""" + """Fail fast when mutating state outside the lock boundary.""" if ACTIVE_LEASE_CONTEXT is None: raise RuntimeError(f"Mutating path reached without lease lock: {operation}") @@ -916,12 +932,12 @@ def save_state(state: dict) -> bool: state["last_updated"] = datetime.now(timezone.utc).isoformat() for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): - snapshot = get_state_issue_snapshot() - if snapshot is None: + if not ensure_state_issue_lease_lock_fresh(): + print("ERROR: Failed to refresh reviewer-bot lease lock before save", file=sys.stderr) return False - if not snapshot.etag: - print("ERROR: Missing ETag on state issue GET; cannot perform conditional PATCH", file=sys.stderr) + snapshot = get_state_issue_snapshot() + if snapshot is None: return False lock_meta = parse_lock_metadata_from_issue_body(snapshot.body) @@ -929,12 +945,12 @@ def save_state(state: dict) -> bool: response = conditional_patch_state_issue(body, snapshot.etag) if response.status_code == 200: - print(f"State saved to issue #{STATE_ISSUE_NUMBER} with If-Match") + print(f"State saved to issue #{STATE_ISSUE_NUMBER}") return True if response.status_code in {409, 412}: print( - "WARNING: State save hit optimistic concurrency conflict " + "WARNING: State save hit conflict " f"(status {response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", file=sys.stderr, ) @@ -1007,6 +1023,9 @@ def lock_is_currently_valid(lock_meta: dict, now: datetime | None = None) -> boo if not isinstance(lock_meta, dict): return False + if lock_meta.get("lock_state") != "locked": + return False + lock_token = lock_meta.get("lock_token") if not isinstance(lock_token, str) or not lock_token: return False @@ -1051,6 +1070,7 @@ def build_lock_metadata( return normalize_lock_metadata( { "schema_version": LOCK_SCHEMA_VERSION, + "lock_state": "locked", "lock_owner_run_id": lock_owner_run_id, "lock_owner_workflow": lock_owner_workflow, "lock_owner_job": lock_owner_job, @@ -1063,11 +1083,335 @@ def build_lock_metadata( def clear_lock_metadata() -> dict: """Return normalized empty lock metadata.""" - return normalize_lock_metadata(None) + return normalize_lock_metadata( + { + "schema_version": LOCK_SCHEMA_VERSION, + "lock_state": "unlocked", + } + ) + + +def normalize_lock_ref_name(ref_name: str) -> str: + """Normalize lock ref name to REST API path form (without refs/ prefix).""" + normalized = ref_name.strip() + if normalized.startswith("refs/"): + normalized = normalized[len("refs/") :] + if not normalized: + normalized = "heads/reviewer-bot-state-lock" + return normalized + + +def get_lock_ref_name() -> str: + """Return normalized lock ref name.""" + return normalize_lock_ref_name(LOCK_REF_NAME) + + +def get_lock_ref_display() -> str: + """Return full lock ref for logs.""" + return f"refs/{get_lock_ref_name()}" + + +def get_state_issue_html_url() -> str: + """Build canonical state issue URL for diagnostics.""" + repo = f"{os.environ.get('REPO_OWNER', '')}/{os.environ.get('REPO_NAME', '')}".strip("/") + if not repo or not STATE_ISSUE_NUMBER: + return "" + return f"https://github.com/{repo}/issues/{STATE_ISSUE_NUMBER}" + + +def extract_ref_sha(payload: Any) -> str | None: + """Extract git ref target SHA from API payload.""" + if not isinstance(payload, dict): + return None + obj = payload.get("object") + if isinstance(obj, dict): + sha = obj.get("sha") + if isinstance(sha, str) and sha: + return sha + sha = payload.get("sha") + if isinstance(sha, str) and sha: + return sha + return None + + +def extract_commit_tree_sha(payload: Any) -> str | None: + """Extract commit tree SHA from API payload.""" + if not isinstance(payload, dict): + return None + tree = payload.get("tree") + if not isinstance(tree, dict): + return None + tree_sha = tree.get("sha") + if isinstance(tree_sha, str) and tree_sha: + return tree_sha + return None + + +def extract_commit_sha(payload: Any) -> str | None: + """Extract commit SHA from API payload.""" + if not isinstance(payload, dict): + return None + sha = payload.get("sha") + if isinstance(sha, str) and sha: + return sha + return None + + +def render_lock_commit_message(lock_meta: dict) -> str: + """Render lock metadata into a deterministic commit message payload.""" + normalized = normalize_lock_metadata(lock_meta) + payload = json.dumps(normalized, sort_keys=True) + return f"{LOCK_COMMIT_MARKER}\n{payload}\n" + + +def parse_lock_metadata_from_lock_commit_message(message: str) -> dict: + """Parse lock metadata from lock commit message.""" + if not isinstance(message, str): + return clear_lock_metadata() + + lines = message.splitlines() + if not lines: + return clear_lock_metadata() + + if lines[0].strip() != LOCK_COMMIT_MARKER: + return clear_lock_metadata() + + payload = "\n".join(lines[1:]).strip() + if not payload: + return clear_lock_metadata() + + try: + parsed = json.loads(payload) + except json.JSONDecodeError: + return clear_lock_metadata() + + if not isinstance(parsed, dict): + return clear_lock_metadata() + return normalize_lock_metadata(parsed) + + +def ensure_lock_ref_exists() -> str: + """Ensure lock ref exists and return current head SHA.""" + lock_ref = get_lock_ref_name() + get_response = github_api_request("GET", f"git/ref/{lock_ref}", suppress_error_log=True) + if get_response.status_code == 200: + ref_sha = extract_ref_sha(get_response.payload) + if ref_sha: + return ref_sha + raise RuntimeError("Lock ref GET returned success but no SHA") + + if get_response.status_code not in {404}: + raise RuntimeError( + "Failed to read reviewer-bot lock ref " + f"{get_lock_ref_display()} (status {get_response.status_code}): {get_response.text}" + ) + + base_branch = LOCK_REF_BOOTSTRAP_BRANCH.strip() or "main" + base_response = github_api_request( + "GET", + f"git/ref/heads/{base_branch}", + suppress_error_log=True, + ) + if base_response.status_code != 200: + raise RuntimeError( + "Failed to bootstrap reviewer-bot lock ref from " + f"heads/{base_branch} (status {base_response.status_code}): {base_response.text}" + ) + + base_sha = extract_ref_sha(base_response.payload) + if not base_sha: + raise RuntimeError("Bootstrap branch ref response missing SHA") + + create_response = github_api_request( + "POST", + "git/refs", + { + "ref": f"refs/{lock_ref}", + "sha": base_sha, + }, + suppress_error_log=True, + ) + if create_response.status_code not in {201, 422}: + raise RuntimeError( + "Failed to create reviewer-bot lock ref " + f"{get_lock_ref_display()} (status {create_response.status_code}): {create_response.text}" + ) + + refresh_response = github_api_request("GET", f"git/ref/{lock_ref}", suppress_error_log=True) + if refresh_response.status_code != 200: + raise RuntimeError( + "Unable to read reviewer-bot lock ref after create " + f"(status {refresh_response.status_code}): {refresh_response.text}" + ) + + ref_sha = extract_ref_sha(refresh_response.payload) + if not ref_sha: + raise RuntimeError("Reviewer-bot lock ref exists but SHA was missing") + return ref_sha + + +def get_lock_ref_snapshot() -> tuple[str, str, dict]: + """Return lock ref head SHA, tree SHA, and parsed lock metadata.""" + ref_sha = ensure_lock_ref_exists() + commit_response = github_api_request( + "GET", + f"git/commits/{ref_sha}", + suppress_error_log=True, + ) + if commit_response.status_code != 200: + raise RuntimeError( + "Failed to read lock commit " + f"{ref_sha} (status {commit_response.status_code}): {commit_response.text}" + ) + + if not isinstance(commit_response.payload, dict): + raise RuntimeError("Lock commit response payload was not a JSON object") + + tree_sha = extract_commit_tree_sha(commit_response.payload) + if not tree_sha: + raise RuntimeError("Lock commit payload missing tree SHA") + + message = commit_response.payload.get("message") + if not isinstance(message, str): + message = "" + + lock_meta = parse_lock_metadata_from_lock_commit_message(message) + return ref_sha, tree_sha, lock_meta + + +def create_lock_commit(parent_sha: str, tree_sha: str, lock_meta: dict) -> GitHubApiResult: + """Create lock state commit that references existing tree.""" + return github_api_request( + "POST", + "git/commits", + { + "message": render_lock_commit_message(lock_meta), + "tree": tree_sha, + "parents": [parent_sha], + }, + suppress_error_log=True, + ) + + +def cas_update_lock_ref(new_sha: str) -> GitHubApiResult: + """Update lock ref with fast-forward (CAS-like) semantics.""" + return github_api_request( + "PATCH", + f"git/refs/{get_lock_ref_name()}", + { + "sha": new_sha, + "force": False, + }, + suppress_error_log=True, + ) + + +def ensure_state_issue_lease_lock_fresh() -> bool: + """Renew lock if expiration is near while this run still owns it.""" + context = ACTIVE_LEASE_CONTEXT + if context is None: + return False + + if not context.lock_expires_at: + return True + + expires_at = parse_iso8601_timestamp(context.lock_expires_at) + if expires_at is None: + return renew_state_issue_lease_lock(context) + + remaining_seconds = (expires_at - datetime.now(timezone.utc)).total_seconds() + if remaining_seconds > LOCK_RENEWAL_WINDOW_SECONDS: + return True + + print( + "Reviewer-bot lease lock nearing expiry; attempting renewal " + f"(remaining={int(remaining_seconds)}s, token_prefix={context.lock_token[:8]})" + ) + return renew_state_issue_lease_lock(context) + + +def renew_state_issue_lease_lock(context: LeaseContext) -> bool: + """Renew currently held lock lease by appending refreshed lock commit.""" + for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): + try: + ref_head_sha, tree_sha, current_lock = get_lock_ref_snapshot() + except RuntimeError as exc: + print(f"ERROR: Failed to read lock snapshot during renewal: {exc}", file=sys.stderr) + return False + + current_token = current_lock.get("lock_token") + if current_token != context.lock_token: + print( + "ERROR: Cannot renew reviewer-bot lock due to token mismatch " + f"(expected prefix={context.lock_token[:8]}, got prefix={str(current_token)[:8]})", + file=sys.stderr, + ) + return False + + desired_lock = build_lock_metadata( + context.lock_token, + context.lock_owner_run_id, + context.lock_owner_workflow, + context.lock_owner_job, + ) + + create_response = create_lock_commit(ref_head_sha, tree_sha, desired_lock) + if create_response.status_code != 201: + if create_response.status_code == 429 or create_response.status_code >= 500: + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + print( + "Retryable lease lock renewal commit failure " + f"(status {create_response.status_code}); retrying " + f"({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + time.sleep(delay) + continue + print( + "ERROR: Failed to create lock renewal commit " + f"(status {create_response.status_code}): {create_response.text}", + file=sys.stderr, + ) + return False + + new_commit_sha = extract_commit_sha(create_response.payload) + if not new_commit_sha: + print("ERROR: Lock renewal commit response missing SHA", file=sys.stderr) + return False + + update_response = cas_update_lock_ref(new_commit_sha) + if update_response.status_code == 200: + context.lock_expires_at = desired_lock.get("lock_expires_at") + print( + "Renewed reviewer-bot lease lock " + f"(run_id={context.lock_owner_run_id}, token_prefix={context.lock_token[:8]})" + ) + return True + + if update_response.status_code in {409, 422, 429} or update_response.status_code >= 500: + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + print( + "Retryable lease lock renewal ref update failure " + f"(status {update_response.status_code}); retrying " + f"({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + time.sleep(delay) + continue + + print( + "ERROR: Failed to update lock ref during renewal " + f"(status {update_response.status_code}): {update_response.text}", + file=sys.stderr, + ) + return False + + print("ERROR: Exhausted retries while renewing reviewer-bot lease lock", file=sys.stderr) + return False def acquire_state_issue_lease_lock() -> LeaseContext: - """Acquire durable lease lock from in-repo state issue using If-Match.""" + """Acquire durable lease lock using git-ref CAS updates.""" global ACTIVE_LEASE_CONTEXT if ACTIVE_LEASE_CONTEXT is not None: @@ -1085,17 +1429,10 @@ def acquire_state_issue_lease_lock() -> LeaseContext: raise RuntimeError( "Timed out waiting for reviewer-bot lease lock " f"after {int(elapsed)}s (run_id={lock_owner_run_id}, token_prefix={lock_token[:8]}, " - f"state_issue={STATE_ISSUE_NUMBER})" + f"lock_ref={get_lock_ref_display()})" ) - snapshot = get_state_issue_snapshot() - if snapshot is None: - raise RuntimeError("Failed to read state issue while acquiring lease lock") - - if not snapshot.etag: - raise RuntimeError("State issue GET response did not include ETag header") - - current_lock = parse_lock_metadata_from_issue_body(snapshot.body) + ref_head_sha, tree_sha, current_lock = get_lock_ref_snapshot() now = datetime.now(timezone.utc) lock_valid = lock_is_currently_valid(current_lock, now) @@ -1106,60 +1443,79 @@ def acquire_state_issue_lease_lock() -> LeaseContext: lock_owner_workflow, lock_owner_job, ) - state = parse_state_yaml_from_issue_body(snapshot.body) - updated_body = render_state_issue_body( - state, - desired_lock, - snapshot.body, - preserve_state_block=True, - ) + create_response = create_lock_commit(ref_head_sha, tree_sha, desired_lock) + if create_response.status_code != 201: + if create_response.status_code == 429 or create_response.status_code >= 500: + print( + "Retryable lease lock acquire commit failure " + f"(status {create_response.status_code}); retrying (attempt {attempt})" + ) + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + time.sleep(delay) + continue + if create_response.status_code in {401, 403}: + raise RuntimeError( + "Insufficient permission to create reviewer-bot lock commit " + f"(status {create_response.status_code}): {create_response.text}" + ) + raise RuntimeError( + "Unexpected status while creating reviewer-bot lock commit " + f"(status {create_response.status_code}): {create_response.text}" + ) - patch_response = conditional_patch_state_issue(updated_body, snapshot.etag) - if patch_response.status_code == 200: + new_commit_sha = extract_commit_sha(create_response.payload) + if not new_commit_sha: + raise RuntimeError("Lock acquire commit response did not include commit SHA") + + update_response = cas_update_lock_ref(new_commit_sha) + if update_response.status_code == 200: ACTIVE_LEASE_CONTEXT = LeaseContext( lock_token=lock_token, lock_owner_run_id=lock_owner_run_id, lock_owner_workflow=lock_owner_workflow, lock_owner_job=lock_owner_job, - state_issue_url=snapshot.html_url, + state_issue_url=get_state_issue_html_url(), + lock_ref=get_lock_ref_display(), + lock_expires_at=desired_lock.get("lock_expires_at"), ) print( "Acquired reviewer-bot lease lock " f"(run_id={lock_owner_run_id}, token_prefix={lock_token[:8]}, " - f"issue={snapshot.html_url or STATE_ISSUE_NUMBER})" + f"lock_ref={get_lock_ref_display()})" ) return ACTIVE_LEASE_CONTEXT - if patch_response.status_code in {409, 412}: + if update_response.status_code in {409, 422}: print( "Lease lock acquire conflict " - f"(status {patch_response.status_code}); retrying (attempt {attempt})" + f"(status {update_response.status_code}); retrying (attempt {attempt})" ) - elif patch_response.status_code == 404: + elif update_response.status_code == 404: raise RuntimeError( - f"State issue #{STATE_ISSUE_NUMBER} not found while acquiring lease lock" + f"Lock ref {get_lock_ref_display()} not found while acquiring lease lock" ) - elif patch_response.status_code in {401, 403}: + elif update_response.status_code in {401, 403}: raise RuntimeError( "Insufficient permission to acquire reviewer-bot lease lock " - f"(status {patch_response.status_code}): {patch_response.text}" + f"(status {update_response.status_code}): {update_response.text}" ) - elif patch_response.status_code == 429 or patch_response.status_code >= 500: + elif update_response.status_code == 429 or update_response.status_code >= 500: print( "Retryable lease lock acquire failure " - f"(status {patch_response.status_code}); retrying (attempt {attempt})" + f"(status {update_response.status_code}); retrying (attempt {attempt})" ) else: raise RuntimeError( "Unexpected status while acquiring reviewer-bot lease lock " - f"(status {patch_response.status_code}): {patch_response.text}" + f"(status {update_response.status_code}): {update_response.text}" ) else: lock_owner = current_lock.get("lock_owner_run_id") or "unknown" lock_expires_at = current_lock.get("lock_expires_at") or "unknown" print( "Reviewer-bot lease lock currently held by " - f"run_id={lock_owner} until {lock_expires_at}; waiting" + f"run_id={lock_owner} until {lock_expires_at}; waiting " + f"(lock_ref={get_lock_ref_display()})" ) delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) @@ -1178,20 +1534,14 @@ def release_state_issue_lease_lock() -> bool: try: for attempt in range(1, LOCK_API_RETRY_LIMIT + 1): - snapshot = get_state_issue_snapshot() - if snapshot is None: - break - - if not snapshot.etag: - print( - "ERROR: Missing ETag while releasing reviewer-bot lease lock", - file=sys.stderr, - ) + try: + ref_head_sha, tree_sha, current_lock = get_lock_ref_snapshot() + except RuntimeError as exc: + print(f"ERROR: Failed to read lock snapshot while releasing lock: {exc}", file=sys.stderr) break - current_lock = parse_lock_metadata_from_issue_body(snapshot.body) current_token = current_lock.get("lock_token") - if current_token and current_token != context.lock_token: + if current_token != context.lock_token: print( "WARNING: Lease lock token mismatch during release; " f"expected prefix={context.lock_token[:8]}, got prefix={str(current_token)[:8]}", @@ -1199,44 +1549,62 @@ def release_state_issue_lease_lock() -> bool: ) return False - state = parse_state_yaml_from_issue_body(snapshot.body) - updated_body = render_state_issue_body( - state, - clear_lock_metadata(), - snapshot.body, - preserve_state_block=True, - ) - patch_response = conditional_patch_state_issue(updated_body, snapshot.etag) + create_response = create_lock_commit(ref_head_sha, tree_sha, clear_lock_metadata()) + if create_response.status_code != 201: + if create_response.status_code in {429} or create_response.status_code >= 500: + print( + "Retryable lease lock release commit failure " + f"(status {create_response.status_code}); retrying " + f"({attempt}/{LOCK_API_RETRY_LIMIT})", + file=sys.stderr, + ) + delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) + time.sleep(delay) + continue + print( + "ERROR: Failed to create lock release commit " + f"(status {create_response.status_code}): {create_response.text}", + file=sys.stderr, + ) + break - if patch_response.status_code == 200: + new_commit_sha = extract_commit_sha(create_response.payload) + if not new_commit_sha: + print("ERROR: Lock release commit response missing SHA", file=sys.stderr) + break + + update_response = cas_update_lock_ref(new_commit_sha) + + if update_response.status_code == 200: released = True print( "Released reviewer-bot lease lock " - f"(run_id={context.lock_owner_run_id}, token_prefix={context.lock_token[:8]})" + f"(run_id={context.lock_owner_run_id}, token_prefix={context.lock_token[:8]}, " + f"lock_ref={get_lock_ref_display()})" ) return True - if patch_response.status_code in {409, 412, 429} or patch_response.status_code >= 500: + if update_response.status_code in {409, 422, 429} or update_response.status_code >= 500: print( "Retryable lease lock release failure " - f"(status {patch_response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", + f"(status {update_response.status_code}); retrying ({attempt}/{LOCK_API_RETRY_LIMIT})", file=sys.stderr, ) delay = LOCK_RETRY_BASE_SECONDS + random.uniform(0, LOCK_RETRY_BASE_SECONDS) time.sleep(delay) continue - if patch_response.status_code in {401, 403, 404}: + if update_response.status_code in {401, 403, 404}: print( "ERROR: Hard failure releasing reviewer-bot lease lock " - f"(status {patch_response.status_code}): {patch_response.text}", + f"(status {update_response.status_code}): {update_response.text}", file=sys.stderr, ) break print( "ERROR: Unexpected status while releasing reviewer-bot lease lock " - f"(status {patch_response.status_code}): {patch_response.text}", + f"(status {update_response.status_code}): {update_response.text}", file=sys.stderr, ) break @@ -3416,8 +3784,6 @@ def handle_labeled_event(state: dict) -> bool: def handle_pull_request_review_event(state: dict) -> bool: """Handle submitted PR reviews for activity and completion tracking.""" - assert_lock_held("handle_pull_request_review_event") - issue_number = int(os.environ.get("ISSUE_NUMBER", 0)) if not issue_number: print("No issue number found") @@ -3433,12 +3799,14 @@ def handle_pull_request_review_event(state: dict) -> bool: if is_cross_repo: print( "Deferring cross-repo pull_request_review reconciliation for " - f"#{issue_number}: this event may have read-only permissions and cannot " - "persist reviewer-bot state. Use `@guidelines-bot /rectify` on the PR to " - "reconcile and persist state." + f"#{issue_number}: this event may have read-only permissions. " + "A trusted workflow_run reconcile will persist state after this run succeeds. " + "If needed, use `@guidelines-bot /rectify` as manual fallback." ) return False + assert_lock_held("handle_pull_request_review_event") + review_data = ensure_review_entry(state, issue_number) if review_data is None: print(f"No active review entry for #{issue_number}") @@ -3679,20 +4047,20 @@ def handle_comment_event(state: dict) -> bool: def handle_manual_dispatch(state: dict) -> bool: """Handle manual workflow dispatch.""" - assert_lock_held("handle_manual_dispatch") - action = os.environ.get("MANUAL_ACTION", "") + if action == "show-state": + print(f"Current state:\n{yaml.dump(state, default_flow_style=False)}") + return False + + assert_lock_held("handle_manual_dispatch") + if action == "sync-members": state, changes = sync_members_with_queue(state) if changes: print(f"Sync changes: {changes}") return True - elif action == "show-state": - print(f"Current state:\n{yaml.dump(state, default_flow_style=False)}") - return False - elif action == "check-overdue": # Manually trigger the overdue review check return handle_scheduled_check(state) @@ -3793,19 +4161,49 @@ def handle_scheduled_check(state: dict) -> bool: # ============================================================================== -def event_requires_lease_lock(event_name: str, event_action: str) -> bool: - """Return True for event paths that may mutate reviewer-bot state.""" +def classify_event_intent(event_name: str, event_action: str) -> str: + """Classify whether a run can mutate reviewer-bot state.""" if event_name in {"issues", "pull_request_target"}: - return event_action in {"opened", "labeled", "closed"} + if event_action in {"opened", "labeled", "closed"}: + return EVENT_INTENT_MUTATING + return EVENT_INTENT_NON_MUTATING_READONLY + if event_name == "issue_comment": - return event_action == "created" + if event_action == "created": + return EVENT_INTENT_MUTATING + return EVENT_INTENT_NON_MUTATING_READONLY + if event_name == "pull_request_review": - return event_action == "submitted" + if event_action != "submitted": + return EVENT_INTENT_NON_MUTATING_READONLY + is_cross_repo = os.environ.get("PR_IS_CROSS_REPOSITORY", "false").lower() == "true" + if is_cross_repo: + return EVENT_INTENT_NON_MUTATING_DEFER + return EVENT_INTENT_MUTATING + if event_name == "workflow_run": - return event_action == "completed" - if event_name in {"workflow_dispatch", "schedule"}: - return True - return False + if event_action != "completed": + return EVENT_INTENT_NON_MUTATING_READONLY + workflow_run_event = os.environ.get("WORKFLOW_RUN_EVENT", "").strip() + if workflow_run_event == "pull_request_review": + return EVENT_INTENT_MUTATING + return EVENT_INTENT_NON_MUTATING_READONLY + + if event_name == "workflow_dispatch": + action = os.environ.get("MANUAL_ACTION", "").strip() + if action == "show-state": + return EVENT_INTENT_NON_MUTATING_READONLY + return EVENT_INTENT_MUTATING + + if event_name == "schedule": + return EVENT_INTENT_MUTATING + + return EVENT_INTENT_NON_MUTATING_READONLY + + +def event_requires_lease_lock(event_name: str, event_action: str) -> bool: + """Backwards-compatible helper for tests and call sites.""" + return classify_event_intent(event_name, event_action) == EVENT_INTENT_MUTATING def main(): @@ -3813,9 +4211,13 @@ def main(): event_name = os.environ.get("EVENT_NAME", "") event_action = os.environ.get("EVENT_ACTION", "") - print(f"Event: {event_name}, Action: {event_action}") + event_intent = classify_event_intent(event_name, event_action) + lock_required = event_intent == EVENT_INTENT_MUTATING + print( + f"Event: {event_name}, Action: {event_action}, Intent: {event_intent}, " + f"Lock Required: {lock_required}" + ) - lock_required = event_requires_lease_lock(event_name, event_action) lock_acquired = False release_failed = False exit_code = 0 @@ -3877,7 +4279,13 @@ def main(): elif event_name == "workflow_run": if event_action == "completed": - state_changed = handle_workflow_run_event(state) + if os.environ.get("WORKFLOW_RUN_EVENT", "").strip() == "pull_request_review": + state_changed = handle_workflow_run_event(state) + else: + print( + "Ignoring workflow_run event with unsupported source event: " + f"{os.environ.get('WORKFLOW_RUN_EVENT', '').strip() or ''}" + ) # Save state if changed (or if we synced members/pass-until) if state_changed or sync_changes or restored: From 71d3cc604ed64d4e5970fcec6da33029b08682b2 Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Thu, 19 Feb 2026 00:23:36 +0900 Subject: [PATCH 3/3] test(reviewer-bot): cover intent routing and git-ref lock behavior --- .../reviewer-bot-tests/test_reviewer_bot.py | 152 +++++++++++++----- 1 file changed, 113 insertions(+), 39 deletions(-) diff --git a/.github/reviewer-bot-tests/test_reviewer_bot.py b/.github/reviewer-bot-tests/test_reviewer_bot.py index 68758bc69..4d52ff5e2 100644 --- a/.github/reviewer-bot-tests/test_reviewer_bot.py +++ b/.github/reviewer-bot-tests/test_reviewer_bot.py @@ -336,22 +336,26 @@ def test_acquire_state_issue_lease_lock_success(monkeypatch): monkeypatch.setenv("WORKFLOW_JOB_NAME", "reviewer-bot") monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) - - body = reviewer_bot.render_state_issue_body(make_state(), reviewer_bot.clear_lock_metadata()) monkeypatch.setattr( reviewer_bot, - "get_state_issue_snapshot", - lambda: reviewer_bot.StateIssueSnapshot( - body=body, - etag='"etag"', - html_url="https://example.com/issues/314", + "get_lock_ref_snapshot", + lambda: ("parent-sha", "tree-sha", reviewer_bot.clear_lock_metadata()), + ) + monkeypatch.setattr( + reviewer_bot, + "create_lock_commit", + lambda parent_sha, tree_sha, lock_meta: reviewer_bot.GitHubApiResult( + status_code=201, + payload={"sha": "new-lock-commit-sha"}, + headers={}, + text="", + ok=True, ), ) - monkeypatch.setattr( reviewer_bot, - "conditional_patch_state_issue", - lambda body, etag: reviewer_bot.GitHubApiResult( + "cas_update_lock_ref", + lambda new_sha: reviewer_bot.GitHubApiResult( status_code=200, payload={"ok": True}, headers={}, @@ -366,6 +370,8 @@ def test_acquire_state_issue_lease_lock_success(monkeypatch): assert ctx.lock_owner_workflow == "Reviewer Bot" assert ctx.lock_owner_job == "reviewer-bot" assert reviewer_bot.ACTIVE_LEASE_CONTEXT is not None + assert ctx.lock_ref == "refs/heads/reviewer-bot-state-lock" + assert isinstance(ctx.lock_expires_at, str) def test_acquire_state_issue_lease_lock_retries_on_conflict(monkeypatch): @@ -373,23 +379,27 @@ def test_acquire_state_issue_lease_lock_retries_on_conflict(monkeypatch): monkeypatch.setattr(reviewer_bot.random, "uniform", lambda a, b: 0.0) monkeypatch.setattr(reviewer_bot.time, "sleep", lambda _: None) - body = reviewer_bot.render_state_issue_body(make_state(), reviewer_bot.clear_lock_metadata()) monkeypatch.setattr( reviewer_bot, - "get_state_issue_snapshot", - lambda: reviewer_bot.StateIssueSnapshot( - body=body, - etag='"etag"', - html_url="https://example.com/issues/314", + "get_lock_ref_snapshot", + lambda: ("parent-sha", "tree-sha", reviewer_bot.clear_lock_metadata()), + ) + monkeypatch.setattr( + reviewer_bot, + "create_lock_commit", + lambda parent_sha, tree_sha, lock_meta: reviewer_bot.GitHubApiResult( + status_code=201, + payload={"sha": "new-lock-commit-sha"}, + headers={}, + text="", + ok=True, ), ) - - statuses = iter([412, 200]) - + statuses = iter([409, 200]) monkeypatch.setattr( reviewer_bot, - "conditional_patch_state_issue", - lambda body, etag: reviewer_bot.GitHubApiResult( + "cas_update_lock_ref", + lambda new_sha: reviewer_bot.GitHubApiResult( status_code=next(statuses), payload={"ok": True}, headers={}, @@ -413,27 +423,33 @@ def test_acquire_state_issue_lease_lock_takes_over_expired_lock(monkeypatch): "lock_owner_run_id": "123", "lock_owner_workflow": "Reviewer Bot", "lock_owner_job": "reviewer-bot", + "lock_state": "locked", "lock_token": "stale-lock", "lock_acquired_at": "2020-01-01T00:00:00+00:00", "lock_expires_at": "2020-01-01T00:01:00+00:00", } ) - body = reviewer_bot.render_state_issue_body(make_state(), expired_lock) + monkeypatch.setattr( + reviewer_bot, + "get_lock_ref_snapshot", + lambda: ("parent-sha", "tree-sha", expired_lock), + ) monkeypatch.setattr( reviewer_bot, - "get_state_issue_snapshot", - lambda: reviewer_bot.StateIssueSnapshot( - body=body, - etag='"etag"', - html_url="https://example.com/issues/314", + "create_lock_commit", + lambda parent_sha, tree_sha, lock_meta: reviewer_bot.GitHubApiResult( + status_code=201, + payload={"sha": "new-lock-commit-sha"}, + headers={}, + text="", + ok=True, ), ) - monkeypatch.setattr( reviewer_bot, - "conditional_patch_state_issue", - lambda body, etag: reviewer_bot.GitHubApiResult( + "cas_update_lock_ref", + lambda new_sha: reviewer_bot.GitHubApiResult( status_code=200, payload={"ok": True}, headers={}, @@ -458,24 +474,19 @@ def test_acquire_state_issue_lease_lock_times_out(monkeypatch): "lock_owner_run_id": "other-run", "lock_owner_workflow": "Reviewer Bot", "lock_owner_job": "reviewer-bot", + "lock_state": "locked", "lock_token": "active-lock", "lock_acquired_at": "2999-01-01T00:00:00+00:00", "lock_expires_at": "2999-01-01T00:10:00+00:00", } ) - body = reviewer_bot.render_state_issue_body(make_state(), valid_lock) - monkeypatch.setattr( reviewer_bot, - "get_state_issue_snapshot", - lambda: reviewer_bot.StateIssueSnapshot( - body=body, - etag='"etag"', - html_url="https://example.com/issues/314", - ), + "get_lock_ref_snapshot", + lambda: ("parent-sha", "tree-sha", valid_lock), ) - monotonic_values = iter([0.0, 0.0, 2.0, 2.0]) + monotonic_values = iter([0.0, 0.0, 2.0]) monkeypatch.setattr(reviewer_bot.time, "monotonic", lambda: next(monotonic_values)) with pytest.raises(RuntimeError, match="Timed out waiting for reviewer-bot lease lock"): @@ -1683,6 +1694,69 @@ def test_handle_comment_event_unknown_command(stub_api, captured_comments): assert "Unknown command" in captured_comments[0]["body"] +def test_classify_event_intent_cross_repo_review_is_non_mutating_defer(monkeypatch): + monkeypatch.setenv("PR_IS_CROSS_REPOSITORY", "true") + intent = reviewer_bot.classify_event_intent("pull_request_review", "submitted") + assert intent == reviewer_bot.EVENT_INTENT_NON_MUTATING_DEFER + + +def test_classify_event_intent_same_repo_review_is_mutating(monkeypatch): + monkeypatch.setenv("PR_IS_CROSS_REPOSITORY", "false") + intent = reviewer_bot.classify_event_intent("pull_request_review", "submitted") + assert intent == reviewer_bot.EVENT_INTENT_MUTATING + + +def test_main_cross_repo_review_does_not_acquire_lock(monkeypatch): + monkeypatch.setenv("EVENT_NAME", "pull_request_review") + monkeypatch.setenv("EVENT_ACTION", "submitted") + monkeypatch.setenv("PR_IS_CROSS_REPOSITORY", "true") + + acquire_called = {"value": False} + + def fail_if_called(): + acquire_called["value"] = True + raise AssertionError("acquire_state_issue_lease_lock should not be called") + + monkeypatch.setattr(reviewer_bot, "acquire_state_issue_lease_lock", fail_if_called) + monkeypatch.setattr(reviewer_bot, "load_state", lambda: make_state()) + monkeypatch.setattr(reviewer_bot, "handle_pull_request_review_event", lambda state: False) + + reviewer_bot.main() + + assert acquire_called["value"] is False + + +def test_main_same_repo_review_acquires_lock(monkeypatch): + monkeypatch.setenv("EVENT_NAME", "pull_request_review") + monkeypatch.setenv("EVENT_ACTION", "submitted") + monkeypatch.setenv("PR_IS_CROSS_REPOSITORY", "false") + + acquire_called = {"value": False} + + def fake_acquire(): + acquire_called["value"] = True + return reviewer_bot.LeaseContext( + lock_token="token", + lock_owner_run_id="run", + lock_owner_workflow="workflow", + lock_owner_job="job", + state_issue_url="https://example.com/issues/314", + lock_ref="refs/heads/reviewer-bot-state-lock", + lock_expires_at="2999-01-01T00:00:00+00:00", + ) + + monkeypatch.setattr(reviewer_bot, "acquire_state_issue_lease_lock", fake_acquire) + monkeypatch.setattr(reviewer_bot, "release_state_issue_lease_lock", lambda: True) + monkeypatch.setattr(reviewer_bot, "load_state", lambda: make_state()) + monkeypatch.setattr(reviewer_bot, "process_pass_until_expirations", lambda state: (state, [])) + monkeypatch.setattr(reviewer_bot, "sync_members_with_queue", lambda state: (state, [])) + monkeypatch.setattr(reviewer_bot, "handle_pull_request_review_event", lambda state: False) + + reviewer_bot.main() + + assert acquire_called["value"] is True + + def test_main_fails_when_save_state_fails(monkeypatch): monkeypatch.setenv("EVENT_NAME", "issue_comment") monkeypatch.setenv("EVENT_ACTION", "created")