From 8b55f10aabc8c31197010a6dbe728b82202cf48f Mon Sep 17 00:00:00 2001 From: Pete LeVasseur Date: Wed, 6 May 2026 04:55:33 +0900 Subject: [PATCH 1/8] fix: harden reviewer-bot reconcile freshness --- .../actions/reviewer-bot-source/action.yml | 18 + .../reviewer-bot-issue-comment-direct.yml | 30 +- .github/workflows/reviewer-bot-issues.yml | 30 +- .../reviewer-bot-pr-comment-router.yml | 57 +-- .../workflows/reviewer-bot-pr-metadata.yml | 28 +- .github/workflows/reviewer-bot-preview.yml | 28 +- .../reviewer-bot-privileged-commands.yml | 26 +- .github/workflows/reviewer-bot-reconcile.yml | 28 +- .../workflows/reviewer-bot-sweeper-repair.yml | 30 +- .github/workflows/reviewer-bot-tests.yml | 3 + scripts/reviewer_bot_core/approval_policy.py | 361 +++++++++++++++- .../comment_command_policy.py | 65 +++ .../comment_freshness_policy.py | 126 +++++- .../deferred_gap_diagnosis.py | 6 + .../reviewer_bot_core/live_review_support.py | 195 ++++++++- .../reconcile_replay_policy.py | 61 +++ .../reviewer_response_policy.py | 213 ++++++++- .../reviewer_review_helpers.py | 62 ++- scripts/reviewer_bot_lib/app.py | 107 ++++- scripts/reviewer_bot_lib/assignment_flow.py | 409 ++++++++++++++++-- .../reviewer_bot_lib/comment_application.py | 78 +++- scripts/reviewer_bot_lib/context.py | 2 + .../deferred_gap_bookkeeping.py | 152 +++++++ scripts/reviewer_bot_lib/event_inputs.py | 83 +++- scripts/reviewer_bot_lib/lifecycle.py | 112 ++++- scripts/reviewer_bot_lib/overdue.py | 307 +++++++++++++ scripts/reviewer_bot_lib/reconcile.py | 376 ++++++++++++++-- .../reviewer_bot_lib/reconcile_payloads.py | 302 ++++++++++++- scripts/reviewer_bot_lib/reminder_comments.py | 145 +++++++ scripts/reviewer_bot_lib/repair_records.py | 93 ++++ scripts/reviewer_bot_lib/sweeper.py | 22 +- .../test_reviewer_bot_test_surface.py | 21 + .../reviewer_bot/test_workflow_files.py | 29 +- .../reviewer_bot/test_pr264_lgtm_replay.py | 83 ++++ .../test_reconcile_workflow_run.py | 14 +- .../unit/reviewer_bot/test_approval_policy.py | 55 +++ .../test_comment_freshness_policy.py | 49 +++ .../test_comment_routing_policy.py | 33 ++ tests/unit/reviewer_bot/test_event_inputs.py | 28 ++ tests/unit/reviewer_bot/test_lifecycle.py | 8 +- .../reviewer_bot/test_live_review_support.py | 48 ++ tests/unit/reviewer_bot/test_overdue.py | 5 +- .../reviewer_bot/test_overdue_decisions.py | 38 ++ .../test_overdue_reminder_scope.py | 48 ++ .../reviewer_bot/test_reconcile_admission.py | 53 +++ .../reviewer_bot/test_reconcile_payloads.py | 53 +++ .../unit/reviewer_bot/test_reconcile_unit.py | 11 +- .../reviewer_bot/test_reminder_comments.py | 40 ++ .../test_reviewer_response_policy.py | 47 ++ .../test_reviewer_review_helpers.py | 34 ++ .../reviewer_bot/test_reviews_live_fetch.py | 44 +- 51 files changed, 3859 insertions(+), 437 deletions(-) create mode 100644 .github/actions/reviewer-bot-source/action.yml create mode 100644 scripts/reviewer_bot_lib/reminder_comments.py create mode 100644 tests/contract/reviewer_bot/test_reviewer_bot_test_surface.py create mode 100644 tests/integration/reviewer_bot/test_pr264_lgtm_replay.py create mode 100644 tests/unit/reviewer_bot/test_approval_policy.py create mode 100644 tests/unit/reviewer_bot/test_comment_freshness_policy.py create mode 100644 tests/unit/reviewer_bot/test_comment_routing_policy.py create mode 100644 tests/unit/reviewer_bot/test_event_inputs.py create mode 100644 tests/unit/reviewer_bot/test_live_review_support.py create mode 100644 tests/unit/reviewer_bot/test_overdue_decisions.py create mode 100644 tests/unit/reviewer_bot/test_overdue_reminder_scope.py create mode 100644 tests/unit/reviewer_bot/test_reconcile_admission.py create mode 100644 tests/unit/reviewer_bot/test_reconcile_payloads.py create mode 100644 tests/unit/reviewer_bot/test_reminder_comments.py create mode 100644 tests/unit/reviewer_bot/test_reviewer_response_policy.py create mode 100644 tests/unit/reviewer_bot/test_reviewer_review_helpers.py diff --git a/.github/actions/reviewer-bot-source/action.yml b/.github/actions/reviewer-bot-source/action.yml new file mode 100644 index 000000000..638568987 --- /dev/null +++ b/.github/actions/reviewer-bot-source/action.yml @@ -0,0 +1,18 @@ +name: Reviewer Bot Source +description: Select the trusted checked-out reviewer-bot source root. +outputs: + bot-src-root: + description: Absolute path to the trusted reviewer-bot source checkout. + value: ${{ steps.source-root.outputs.bot-src-root }} +runs: + using: composite + steps: + - id: source-root + shell: bash + run: | + if [ ! -f "$GITHUB_WORKSPACE/scripts/reviewer_bot.py" ]; then + echo "reviewer-bot source checkout is missing scripts/reviewer_bot.py" >&2 + exit 1 + fi + printf 'bot-src-root=%s\n' "$GITHUB_WORKSPACE" >> "$GITHUB_OUTPUT" + printf 'BOT_SRC_ROOT=%s\n' "$GITHUB_WORKSPACE" >> "$GITHUB_ENV" diff --git a/.github/workflows/reviewer-bot-issue-comment-direct.yml b/.github/workflows/reviewer-bot-issue-comment-direct.yml index 58f071063..eab053fee 100644 --- a/.github/workflows/reviewer-bot-issue-comment-direct.yml +++ b/.github/workflows/reviewer-bot-issue-comment-direct.yml @@ -23,30 +23,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - repo = os.environ['GITHUB_REPOSITORY'] - ref = os.environ['GITHUB_SHA'] - req = urllib.request.Request( - f'https://api.github.com/repos/{repo}/tarball/{ref}', - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: issue_comment EVENT_ACTION: created diff --git a/.github/workflows/reviewer-bot-issues.yml b/.github/workflows/reviewer-bot-issues.yml index 265a02dea..e9eb8a57a 100644 --- a/.github/workflows/reviewer-bot-issues.yml +++ b/.github/workflows/reviewer-bot-issues.yml @@ -22,30 +22,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - repo = os.environ['GITHUB_REPOSITORY'] - ref = os.environ['GITHUB_SHA'] - req = urllib.request.Request( - f'https://api.github.com/repos/{repo}/tarball/{ref}', - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: issues EVENT_ACTION: ${{ github.event.action }} diff --git a/.github/workflows/reviewer-bot-pr-comment-router.yml b/.github/workflows/reviewer-bot-pr-comment-router.yml index c2bd0668e..887a0cc85 100644 --- a/.github/workflows/reviewer-bot-pr-comment-router.yml +++ b/.github/workflows/reviewer-bot-pr-comment-router.yml @@ -28,29 +28,17 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Route PR comment id: route env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} PAYLOAD_PATH: ${{ runner.temp }}/deferred-comment.json run: | @@ -197,6 +185,7 @@ jobs: needs: [route-pr-comment] runs-on: ubuntu-latest permissions: + # Temporary lock debt: contents:write is allowed only for the existing lock-ref API operations. contents: write issues: write pull-requests: write @@ -204,28 +193,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: issue_comment EVENT_ACTION: created diff --git a/.github/workflows/reviewer-bot-pr-metadata.yml b/.github/workflows/reviewer-bot-pr-metadata.yml index fb108a31d..4b2e57bd2 100644 --- a/.github/workflows/reviewer-bot-pr-metadata.yml +++ b/.github/workflows/reviewer-bot-pr-metadata.yml @@ -22,28 +22,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: pull_request_target EVENT_ACTION: ${{ github.event.action }} diff --git a/.github/workflows/reviewer-bot-preview.yml b/.github/workflows/reviewer-bot-preview.yml index b4b429716..ef212ba17 100644 --- a/.github/workflows/reviewer-bot-preview.yml +++ b/.github/workflows/reviewer-bot-preview.yml @@ -36,28 +36,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot preview env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: workflow_dispatch EVENT_ACTION: '' diff --git a/.github/workflows/reviewer-bot-privileged-commands.yml b/.github/workflows/reviewer-bot-privileged-commands.yml index 61485a29f..6680d21c2 100644 --- a/.github/workflows/reviewer-bot-privileged-commands.yml +++ b/.github/workflows/reviewer-bot-privileged-commands.yml @@ -18,6 +18,7 @@ jobs: privileged-command-executor: runs-on: ubuntu-latest permissions: + # Temporary lock debt: contents:write is allowed only for the existing lock-ref API operations. contents: write issues: write pull-requests: write @@ -27,27 +28,9 @@ jobs: uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Document isolated privileged execution boundary run: | { @@ -57,6 +40,7 @@ jobs: } >> "$GITHUB_STEP_SUMMARY" - name: Execute persisted privileged command env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: workflow_dispatch EVENT_ACTION: execute-pending-privileged-command diff --git a/.github/workflows/reviewer-bot-reconcile.yml b/.github/workflows/reviewer-bot-reconcile.yml index 74a2176e6..809fcd77e 100644 --- a/.github/workflows/reviewer-bot-reconcile.yml +++ b/.github/workflows/reviewer-bot-reconcile.yml @@ -36,26 +36,13 @@ jobs: run-id: ${{ github.event.workflow_run.id }} repository: ${{ github.repository }} github-token: ${{ secrets.GITHUB_TOKEN }} - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - req = urllib.request.Request( - f"https://api.github.com/repos/{os.environ['GITHUB_REPOSITORY']}/tarball/{os.environ['GITHUB_SHA']}", - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Select deferred payload run: | python - <<'PY' @@ -69,6 +56,7 @@ jobs: PY - name: Run trusted reconcile env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: workflow_run EVENT_ACTION: completed diff --git a/.github/workflows/reviewer-bot-sweeper-repair.yml b/.github/workflows/reviewer-bot-sweeper-repair.yml index 8e9bb1eb8..d0f27dab8 100644 --- a/.github/workflows/reviewer-bot-sweeper-repair.yml +++ b/.github/workflows/reviewer-bot-sweeper-repair.yml @@ -34,30 +34,16 @@ jobs: steps: - name: Install uv run: python -m pip install uv - - name: Fetch trusted bot source tarball - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - python - <<'PY' - import io, os, tarfile, urllib.request - from pathlib import Path - repo = os.environ['GITHUB_REPOSITORY'] - ref = os.environ['GITHUB_SHA'] - req = urllib.request.Request( - f'https://api.github.com/repos/{repo}/tarball/{ref}', - headers={'Authorization': f"Bearer {os.environ['GITHUB_TOKEN']}", 'Accept': 'application/vnd.github+json'}, - ) - target = Path(os.environ['RUNNER_TEMP']) / 'reviewer-bot-src' - target.mkdir(parents=True, exist_ok=True) - with urllib.request.urlopen(req) as response: - data = response.read() - with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive: - archive.extractall(target) - roots = list(target.iterdir()) - print(f'BOT_SRC_ROOT={roots[0]}', file=open(os.environ['GITHUB_ENV'], 'a', encoding='utf-8')) - PY + - name: Checkout trusted bot source + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: ${{ github.sha }} + - name: Select trusted bot source + id: bot-source + uses: ./.github/actions/reviewer-bot-source - name: Run reviewer bot env: + BOT_SRC_ROOT: ${{ steps.bot-source.outputs.bot-src-root }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} EVENT_NAME: ${{ github.event_name }} EVENT_ACTION: ${{ github.event.action }} diff --git a/.github/workflows/reviewer-bot-tests.yml b/.github/workflows/reviewer-bot-tests.yml index fc00adb04..45ff9546d 100644 --- a/.github/workflows/reviewer-bot-tests.yml +++ b/.github/workflows/reviewer-bot-tests.yml @@ -4,6 +4,7 @@ on: push: paths: - 'scripts/reviewer_bot.py' + - 'scripts/reviewer_bot_core/**' - 'scripts/reviewer_bot_lib/**' - 'scripts/*.py' - 'tests/**' @@ -12,6 +13,7 @@ on: pull_request: paths: - 'scripts/reviewer_bot.py' + - 'scripts/reviewer_bot_core/**' - 'scripts/reviewer_bot_lib/**' - 'scripts/*.py' - 'tests/**' @@ -83,6 +85,7 @@ jobs: COVERAGE_FILE=artifacts/coverage/.coverage uv run pytest tests/unit/reviewer_bot tests/integration/reviewer_bot --cov=scripts.reviewer_bot + --cov=scripts.reviewer_bot_core --cov=scripts.reviewer_bot_lib --cov-branch --cov-report=term-missing diff --git a/scripts/reviewer_bot_core/approval_policy.py b/scripts/reviewer_bot_core/approval_policy.py index 0e53c0131..e205d8a7d 100644 --- a/scripts/reviewer_bot_core/approval_policy.py +++ b/scripts/reviewer_bot_core/approval_policy.py @@ -16,9 +16,267 @@ from __future__ import annotations +from dataclasses import dataclass + from . import live_review_support +@dataclass(frozen=True) +class CompletionAuthorityDecision: + issue_number: int | None + tracked_reviewer: str | None + head_sha: str | None + completion_state: str + completion_timestamp: str | None + timestamp_source: str + source_review_id: int | str | None + source_review_state: str | None + non_assigned_review_diagnostic: bool + can_set_review_completed_at: bool + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "tracked_reviewer": self.tracked_reviewer, + "head_sha": self.head_sha, + "completion_state": self.completion_state, + "completion_timestamp": self.completion_timestamp, + "timestamp_source": self.timestamp_source, + "source_review_id": self.source_review_id, + "source_review_state": self.source_review_state, + "non_assigned_review_diagnostic": self.non_assigned_review_diagnostic, + "can_set_review_completed_at": self.can_set_review_completed_at, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class WriteApprovalAuthorityDecision: + issue_number: int | None + head_sha: str | None + assigned_reviewer: str | None + assigned_review_id: int | str | None + assigned_review_state: str | None + assigned_round_complete: bool + write_approval_state: str + write_approval_source: str + approving_reviewer: str | None + approving_review_id: int | str | None + permission_source: str | None + dismissal_supersession_status: str + response_state: str + diagnostic_reason: str | None + can_project_final_state: bool + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "head_sha": self.head_sha, + "assigned_reviewer": self.assigned_reviewer, + "assigned_review_id": self.assigned_review_id, + "assigned_review_state": self.assigned_review_state, + "assigned_round_complete": self.assigned_round_complete, + "write_approval_state": self.write_approval_state, + "write_approval_source": self.write_approval_source, + "approving_reviewer": self.approving_reviewer, + "approving_review_id": self.approving_review_id, + "permission_source": self.permission_source, + "dismissal_supersession_status": self.dismissal_supersession_status, + "response_state": self.response_state, + "diagnostic_reason": self.diagnostic_reason, + "can_project_final_state": self.can_project_final_state, + } + + +def derive_completion_authority_decision( + *, + issue_number: int | None, + tracked_reviewer: str | None, + head_sha: str | None, + review_classification: live_review_support.ReviewFreshnessClassification | None, + non_assigned_review_diagnostic: bool, +) -> CompletionAuthorityDecision: + if review_classification is None: + return CompletionAuthorityDecision( + issue_number=issue_number, + tracked_reviewer=tracked_reviewer, + head_sha=head_sha, + completion_state="not_completed", + completion_timestamp=None, + timestamp_source="none", + source_review_id=None, + source_review_state=None, + non_assigned_review_diagnostic=non_assigned_review_diagnostic, + can_set_review_completed_at=False, + diagnostic_reason="no_review_classification", + ) + if review_classification.classified_scope != "current_head_assigned_reviewer": + return CompletionAuthorityDecision( + issue_number=issue_number, + tracked_reviewer=tracked_reviewer, + head_sha=head_sha, + completion_state="not_completed", + completion_timestamp=None, + timestamp_source="non_assigned_review_diagnostic" if non_assigned_review_diagnostic else "none", + source_review_id=review_classification.review_id, + source_review_state=review_classification.state, + non_assigned_review_diagnostic=non_assigned_review_diagnostic, + can_set_review_completed_at=False, + diagnostic_reason=review_classification.diagnostic_reason, + ) + if not review_classification.submitted_at: + return CompletionAuthorityDecision( + issue_number=issue_number, + tracked_reviewer=tracked_reviewer, + head_sha=head_sha, + completion_state="blocked", + completion_timestamp=None, + timestamp_source="blocked_missing_timestamp", + source_review_id=review_classification.review_id, + source_review_state=review_classification.state, + non_assigned_review_diagnostic=False, + can_set_review_completed_at=False, + diagnostic_reason="missing_review_submitted_at", + ) + return CompletionAuthorityDecision( + issue_number=issue_number, + tracked_reviewer=tracked_reviewer, + head_sha=head_sha, + completion_state="completed_by_tracked_reviewer", + completion_timestamp=review_classification.submitted_at, + timestamp_source="current_head_tracked_reviewer_review", + source_review_id=review_classification.review_id, + source_review_state=review_classification.state, + non_assigned_review_diagnostic=False, + can_set_review_completed_at=True, + diagnostic_reason=None, + ) + + +def derive_write_approval_authority_decision( + *, + issue_number: int | None, + head_sha: str | None, + assigned_reviewer: str | None, + assigned_review_classification: live_review_support.ReviewFreshnessClassification | None, + visible_review_classifications: tuple[live_review_support.ReviewFreshnessClassification, ...], + permission_evidence: object | None, + dismissal_evidence: object | None, +) -> WriteApprovalAuthorityDecision: + del dismissal_evidence + assigned_complete = assigned_review_classification is not None and assigned_review_classification.classified_scope == "current_head_assigned_reviewer" + assigned_state = assigned_review_classification.state if assigned_review_classification is not None else None + if not assigned_complete: + return WriteApprovalAuthorityDecision( + issue_number=issue_number, + head_sha=head_sha, + assigned_reviewer=assigned_reviewer, + assigned_review_id=assigned_review_classification.review_id if assigned_review_classification else None, + assigned_review_state=assigned_state, + assigned_round_complete=False, + write_approval_state="blocked_unavailable_authority", + write_approval_source="blocked_untrusted_review_reads", + approving_reviewer=None, + approving_review_id=None, + permission_source=None, + dismissal_supersession_status="blocked_untrusted", + response_state="projection_failed", + diagnostic_reason="assigned_round_not_complete", + can_project_final_state=False, + ) + if assigned_state != "APPROVED": + return WriteApprovalAuthorityDecision( + issue_number=issue_number, + head_sha=head_sha, + assigned_reviewer=assigned_reviewer, + assigned_review_id=assigned_review_classification.review_id, + assigned_review_state=assigned_state, + assigned_round_complete=True, + write_approval_state="not_required_for_non_approval_round_completion", + write_approval_source="assigned_reviewer_current_head_non_approval_review", + approving_reviewer=assigned_review_classification.author, + approving_review_id=assigned_review_classification.review_id, + permission_source="not_required_for_non_approval_round_completion", + dismissal_supersession_status="pass_not_dismissed_or_superseded", + response_state="awaiting_contributor_response", + diagnostic_reason=None, + can_project_final_state=True, + ) + permission_map = ( + {str(login).lower(): status for login, status in permission_evidence.items() if isinstance(login, str)} + if isinstance(permission_evidence, dict) + else {} + ) + for classification in visible_review_classifications: + if classification.state != "APPROVED" or classification.classified_scope not in { + "current_head_assigned_reviewer", + "current_head_alternate_reviewer", + }: + continue + permission = permission_map.get((classification.author or "").lower(), "unavailable") + if permission == "unavailable" and permission_map: + return WriteApprovalAuthorityDecision( + issue_number=issue_number, + head_sha=head_sha, + assigned_reviewer=assigned_reviewer, + assigned_review_id=assigned_review_classification.review_id, + assigned_review_state=assigned_state, + assigned_round_complete=True, + write_approval_state="blocked_unavailable_authority", + write_approval_source="github_permission_read_unavailable", + approving_reviewer=classification.author, + approving_review_id=classification.review_id, + permission_source="github_permission_read", + dismissal_supersession_status="blocked_untrusted", + response_state="projection_failed", + diagnostic_reason="permission_unavailable", + can_project_final_state=False, + ) + assigned_reviewer_approval_without_permission_read = ( + classification.author == assigned_review_classification.author and not permission_map + ) + if permission == "granted" or assigned_reviewer_approval_without_permission_read: + return WriteApprovalAuthorityDecision( + issue_number=issue_number, + head_sha=head_sha, + assigned_reviewer=assigned_reviewer, + assigned_review_id=assigned_review_classification.review_id, + assigned_review_state=assigned_state, + assigned_round_complete=True, + write_approval_state="visible_write_approval", + write_approval_source=( + "assigned_reviewer_current_head_approval" + if classification.author == assigned_review_classification.author + else "non_assigned_current_head_write_approval_after_assigned_round" + ), + approving_reviewer=classification.author, + approving_review_id=classification.review_id, + permission_source="github_permission_read" if permission_map else None, + dismissal_supersession_status="pass_not_dismissed_or_superseded", + response_state="done", + diagnostic_reason=None, + can_project_final_state=True, + ) + return WriteApprovalAuthorityDecision( + issue_number=issue_number, + head_sha=head_sha, + assigned_reviewer=assigned_reviewer, + assigned_review_id=assigned_review_classification.review_id, + assigned_review_state=assigned_state, + assigned_round_complete=True, + write_approval_state="visibly_missing_write_approval", + write_approval_source="none_visible_after_trusted_reads", + approving_reviewer=None, + approving_review_id=None, + permission_source="github_permission_read" if permission_map else None, + dismissal_supersession_status="pass_not_dismissed_or_superseded", + response_state="awaiting_write_approval", + diagnostic_reason=None, + can_project_final_state=True, + ) + + def compute_pr_approval_state_from_reviews( survivors: dict[str, dict], *, @@ -92,29 +350,94 @@ def compute_pr_approval_state_result( if not reviews_result.get("ok"): return reviews_result reviews = reviews_result["reviews"] - - normalized_reviews = live_review_support.normalize_reviews_with_parsed_timestamps( - reviews, - parse_timestamp=live_review_support.parse_github_timestamp, + context = live_review_support.build_current_review_context( + bot, + issue_number, + review_data, + pull_request=pull_request, + reviews=reviews, + ) + if not context.live_reviews_available: + return live_review_support.projection_failure_result(str(context.live_reviews_failure_kind or "reviews_unavailable")) + current_reviewer = review_data.get("current_reviewer") if isinstance(review_data.get("current_reviewer"), str) else None + current_head_classifications = tuple( + classification + for classification in context.classifications + if classification.classified_scope in {"current_head_assigned_reviewer", "current_head_alternate_reviewer"} ) - survivors = live_review_support.filter_current_head_reviews_for_cycle( - normalized_reviews, - boundary=boundary, - current_head=current_head, + assigned_classifications = tuple( + classification + for classification in current_head_classifications + if classification.classified_scope == "current_head_assigned_reviewer" ) - permission_cache = live_review_support.collect_permission_statuses( - survivors, - permission_status=lambda author: live_review_support.permission_status(bot, author, "push"), + assigned_classification = max( + assigned_classifications, + key=lambda item: (item.submitted_at or "", str(item.review_id)), + default=None, ) - result = compute_pr_approval_state_from_reviews( - survivors, - current_reviewer=review_data.get("current_reviewer"), - current_head=current_head, - permission_statuses=permission_cache, + completion_decision = derive_completion_authority_decision( + issue_number=issue_number, + tracked_reviewer=current_reviewer, + head_sha=current_head, + review_classification=assigned_classification, + non_assigned_review_diagnostic=any( + item.classified_scope == "current_head_alternate_reviewer" for item in current_head_classifications + ), + ) + if not completion_decision.can_set_review_completed_at: + return { + "ok": True, + "completion": { + "completed": False, + "current_head_sha": current_head, + "qualifying_review_ids": [], + "authority_decision": completion_decision.to_output(), + }, + "write_approval": { + "has_write_approval": False, + "write_approvers": [], + "current_head_sha": current_head, + "response_state": "awaiting_reviewer_response", + }, + "current_head_sha": current_head, + } + permission_evidence = {} + for classification in current_head_classifications: + if classification.state != "APPROVED" or not classification.author: + continue + permission_evidence[classification.author.lower()] = live_review_support.permission_status(bot, classification.author, "push") + write_decision = derive_write_approval_authority_decision( + issue_number=issue_number, + head_sha=current_head, + assigned_reviewer=current_reviewer, + assigned_review_classification=assigned_classification, + visible_review_classifications=current_head_classifications, + permission_evidence=permission_evidence, + dismissal_evidence=None, ) - if not result.get("ok"): - return live_review_support.projection_failure_result(str(result.get("reason"))) - return result + if write_decision.response_state == "projection_failed": + return live_review_support.projection_failure_result(write_decision.diagnostic_reason or write_decision.write_approval_state) + completion = { + "completed": completion_decision.can_set_review_completed_at, + "current_head_sha": current_head, + "qualifying_review_ids": ( + [completion_decision.source_review_id] if completion_decision.can_set_review_completed_at else [] + ), + "authority_decision": completion_decision.to_output(), + } + write_approval = { + "has_write_approval": write_decision.response_state == "done", + "write_approvers": [write_decision.approving_reviewer] if write_decision.response_state == "done" and write_decision.approving_reviewer else [], + "current_head_sha": current_head, + "response_state": write_decision.response_state, + "authority_decision": write_decision.to_output(), + } + return { + "ok": True, + "completion": completion, + "write_approval": write_approval, + "current_head_sha": current_head, + } def find_triage_approval_after(bot, reviews: list[dict], since) -> tuple[str, object] | None: diff --git a/scripts/reviewer_bot_core/comment_command_policy.py b/scripts/reviewer_bot_core/comment_command_policy.py index 079a840d9..8264f735f 100644 --- a/scripts/reviewer_bot_core/comment_command_policy.py +++ b/scripts/reviewer_bot_core/comment_command_policy.py @@ -50,6 +50,32 @@ class ExecuteOrdinaryCommandDecision: needs_assignment_request: bool +@dataclass(frozen=True) +class AssignmentCommandAuthorization: + command_name: str + actor: str + issue_number: int + target: str | None + current_reviewer: str | None + is_assigned: bool + actor_permission: str + authorized: bool + reason: str + + def to_output(self) -> dict[str, object]: + return { + "command_name": self.command_name, + "actor": self.actor, + "issue_number": self.issue_number, + "target": self.target, + "current_reviewer": self.current_reviewer, + "is_assigned": self.is_assigned, + "actor_permission": self.actor_permission, + "authorized": self.authorized, + "reason": self.reason, + } + + CommentCommandDecision = ( IgnoreDecision | InlineResponseDecision @@ -58,6 +84,45 @@ class ExecuteOrdinaryCommandDecision: ) +def authorize_assignment_command( + command_name: str, + *, + actor: str, + issue_number: int, + target: str | None, + current_reviewer: str | None, + actor_permission: str, +) -> AssignmentCommandAuthorization: + permission = actor_permission.strip().lower() if isinstance(actor_permission, str) else "" + is_assigned = ( + isinstance(current_reviewer, str) + and isinstance(actor, str) + and current_reviewer.strip() + and actor.lower() == current_reviewer.lower() + ) + triage_or_better = permission in {"admin", "maintain", "write", "triage", "granted"} + if command_name in {OrdinaryCommandId.PASS.value, OrdinaryCommandId.RELEASE.value, OrdinaryCommandId.CLAIM.value}: + authorized = is_assigned or triage_or_better or command_name == OrdinaryCommandId.CLAIM.value + reason = "assigned_reviewer" if is_assigned else "triage_override" if triage_or_better else "claim_allowed" if authorized else "actor_not_authorized" + elif command_name in {OrdinaryCommandId.ASSIGN_SPECIFIC.value, OrdinaryCommandId.ASSIGN_FROM_QUEUE.value, "r?"}: + authorized = triage_or_better or is_assigned + reason = "triage_override" if triage_or_better else "assigned_reviewer" if is_assigned else "actor_not_authorized" + else: + authorized = False + reason = "not_assignment_command" + return AssignmentCommandAuthorization( + command_name=command_name, + actor=actor, + issue_number=issue_number, + target=target, + current_reviewer=current_reviewer, + is_assigned=bool(is_assigned), + actor_permission=permission or "unknown", + authorized=authorized, + reason=reason, + ) + + def decide_comment_command(bot, request, classified, *, actor_class: str, commands_help: str) -> CommentCommandDecision: if not isinstance(classified, dict): return IgnoreDecision() diff --git a/scripts/reviewer_bot_core/comment_freshness_policy.py b/scripts/reviewer_bot_core/comment_freshness_policy.py index 5f64bea73..522e2b91d 100644 --- a/scripts/reviewer_bot_core/comment_freshness_policy.py +++ b/scripts/reviewer_bot_core/comment_freshness_policy.py @@ -9,6 +9,36 @@ from dataclasses import dataclass +@dataclass(frozen=True) +class CommentFreshnessEvent: + issue_number: int + is_pull_request: bool + source_event_key: str + comment_id: int + actor: str + created_at: str + channel_kind: str + source_kind: str + reviewed_head_sha: str | None + issue_author: str + current_reviewer: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "is_pull_request": self.is_pull_request, + "source_event_key": self.source_event_key, + "comment_id": self.comment_id, + "actor": self.actor, + "created_at": self.created_at, + "channel_kind": self.channel_kind, + "source_kind": self.source_kind, + "reviewed_head_sha": self.reviewed_head_sha, + "issue_author": self.issue_author, + "current_reviewer": self.current_reviewer, + } + + @dataclass(frozen=True) class CommentFreshnessDecision: kind: str @@ -17,29 +47,99 @@ class CommentFreshnessDecision: timestamp: str | None = None actor: str | None = None update_reviewer_activity: bool = False + diagnostic_reason: str | None = None + def to_output(self) -> dict[str, object]: + return { + "kind": self.kind, + "channel_name": self.channel_name, + "semantic_key": self.semantic_key, + "timestamp": self.timestamp, + "actor": self.actor, + "update_reviewer_activity": self.update_reviewer_activity, + "diagnostic_reason": self.diagnostic_reason, + } -def decide_comment_freshness(review_data: dict, request) -> CommentFreshnessDecision: - comment_author = request.comment_author - created_at = request.comment_created_at - semantic_key = request.comment_source_event_key or f"issue_comment:{request.comment_id}" - if request.issue_author and request.issue_author.lower() == comment_author.lower(): + +def build_comment_freshness_event(review_data: dict, request) -> CommentFreshnessEvent: + source_kind = getattr(request, "comment_source_kind", "issue_comment") or "issue_comment" + channel_kind = "review_comment" if source_kind == "pull_request_review_comment" else "issue_thread" + return CommentFreshnessEvent( + issue_number=int(request.issue_number), + is_pull_request=bool(request.is_pull_request), + source_event_key=request.comment_source_event_key or f"{source_kind}:{request.comment_id}", + comment_id=int(request.comment_id), + actor=str(request.comment_author), + created_at=str(request.comment_created_at), + channel_kind=channel_kind, + source_kind=str(source_kind), + reviewed_head_sha=getattr(request, "reviewed_head_sha", None), + issue_author=str(request.issue_author), + current_reviewer=review_data.get("current_reviewer") if isinstance(review_data.get("current_reviewer"), str) else None, + ) + + +def _same_login(left: str | None, right: str | None) -> bool: + return isinstance(left, str) and isinstance(right, str) and bool(left.strip()) and left.lower() == right.lower() + + +def decide_comment_freshness_event( + event: CommentFreshnessEvent, + *, + current_head_sha: str | None, +) -> CommentFreshnessDecision: + if event.source_kind not in {"issue_comment", "pull_request_review_comment"}: + return CommentFreshnessDecision(kind="blocked", diagnostic_reason="unsupported_comment_source_kind") + if event.channel_kind not in {"issue_thread", "review_comment"}: + return CommentFreshnessDecision(kind="blocked", diagnostic_reason="unsupported_comment_channel_kind") + + if event.source_kind == "pull_request_review_comment": + if not event.reviewed_head_sha: + return CommentFreshnessDecision(kind="diagnostic_only", diagnostic_reason="missing_reviewed_head_sha") + if current_head_sha and event.reviewed_head_sha != current_head_sha: + return CommentFreshnessDecision(kind="diagnostic_only", diagnostic_reason="stale_reviewed_head_sha") + if _same_login(event.issue_author, event.actor): + return CommentFreshnessDecision( + kind="accept_channel_event", + channel_name="contributor_comment", + semantic_key=event.source_event_key, + timestamp=event.created_at, + actor=event.actor, + update_reviewer_activity=False, + ) + return CommentFreshnessDecision(kind="diagnostic_only", diagnostic_reason="review_comments_do_not_suppress_reminders") + + if _same_login(event.issue_author, event.actor): return CommentFreshnessDecision( kind="accept_channel_event", channel_name="contributor_comment", - semantic_key=semantic_key, - timestamp=created_at, - actor=comment_author, + semantic_key=event.source_event_key, + timestamp=event.created_at, + actor=event.actor, update_reviewer_activity=False, ) - current_reviewer = review_data.get("current_reviewer") - if isinstance(current_reviewer, str) and current_reviewer.lower() == comment_author.lower(): + if _same_login(event.current_reviewer, event.actor): + if event.is_pull_request: + return CommentFreshnessDecision( + kind="diagnostic_only", + semantic_key=event.source_event_key, + timestamp=event.created_at, + actor=event.actor, + update_reviewer_activity=False, + diagnostic_reason="plain_pr_reviewer_comment_is_diagnostic_only", + ) return CommentFreshnessDecision( kind="accept_channel_event", channel_name="reviewer_comment", - semantic_key=semantic_key, - timestamp=created_at, - actor=comment_author, + semantic_key=event.source_event_key, + timestamp=event.created_at, + actor=event.actor, update_reviewer_activity=True, ) return CommentFreshnessDecision(kind="noop") + + +def decide_comment_freshness(review_data: dict, request) -> CommentFreshnessDecision: + event = build_comment_freshness_event(review_data, request) + current_head_sha = review_data.get("active_head_sha") if isinstance(review_data.get("active_head_sha"), str) else None + return decide_comment_freshness_event(event, current_head_sha=current_head_sha) diff --git a/scripts/reviewer_bot_core/deferred_gap_diagnosis.py b/scripts/reviewer_bot_core/deferred_gap_diagnosis.py index 785f8b55d..0edc46036 100644 --- a/scripts/reviewer_bot_core/deferred_gap_diagnosis.py +++ b/scripts/reviewer_bot_core/deferred_gap_diagnosis.py @@ -9,6 +9,12 @@ from datetime import datetime, timedelta, timezone from typing import Any +APPROVAL_PENDING_SIGNATURE = { + "status": "waiting", + "conclusion": None, + "name": "approval_pending", +} + def _now() -> datetime: return datetime.now(timezone.utc) diff --git a/scripts/reviewer_bot_core/live_review_support.py b/scripts/reviewer_bot_core/live_review_support.py index 7897c977f..65b186a54 100644 --- a/scripts/reviewer_bot_core/live_review_support.py +++ b/scripts/reviewer_bot_core/live_review_support.py @@ -2,10 +2,67 @@ from __future__ import annotations +from dataclasses import dataclass from datetime import datetime, timezone from typing import Any +@dataclass(frozen=True) +class ReviewFreshnessClassification: + review_id: int | str + author: str | None + state: str | None + submitted_at: str | None + commit_id: str | None + classified_scope: str + is_assigned_reviewer: bool + is_current_head: bool + is_after_cycle_boundary: bool + is_dismissed_or_superseded: bool + diagnostic_reason: str | None + payload: dict[str, object] + + def to_output(self) -> dict[str, object]: + return { + "review_id": self.review_id, + "author": self.author, + "state": self.state, + "submitted_at": self.submitted_at, + "commit_id": self.commit_id, + "classified_scope": self.classified_scope, + "is_assigned_reviewer": self.is_assigned_reviewer, + "is_current_head": self.is_current_head, + "is_after_cycle_boundary": self.is_after_cycle_boundary, + "is_dismissed_or_superseded": self.is_dismissed_or_superseded, + "diagnostic_reason": self.diagnostic_reason, + "payload": dict(self.payload), + } + + +@dataclass(frozen=True) +class CurrentReviewContext: + issue_number: int + current_head_sha: str | None + cycle_boundary: str | None + live_reviews_available: bool + live_reviews_failure_kind: str | None + classifications: tuple[ReviewFreshnessClassification, ...] + + def to_output(self) -> dict[str, object]: + classifications = sorted( + self.classifications, + key=lambda item: (item.submitted_at or "", str(item.review_id), item.author or ""), + ) + return { + "issue_number": self.issue_number, + "current_head_sha": self.current_head_sha, + "cycle_boundary": self.cycle_boundary, + "live_reviews_available": self.live_reviews_available, + "live_reviews_failure_kind": self.live_reviews_failure_kind, + "classifications": [item.to_output() for item in classifications], + } + + def get_current_cycle_boundary(review_data: dict, *, parse_timestamp) -> datetime | None: for field in ("active_cycle_started_at", "cycle_started_at", "assigned_at"): boundary = parse_timestamp(review_data.get(field)) @@ -115,6 +172,71 @@ def normalize_reviews_with_parsed_timestamps(reviews: list[dict], *, parse_times return normalized_reviews +def _timestamp_value(value: object) -> datetime | None: + if isinstance(value, datetime): + return value + return parse_github_timestamp(value) + + +def classify_review_freshness( + review, + *, + current_head_sha: str | None, + cycle_boundary: object | None, + assigned_reviewer: str | None, +) -> ReviewFreshnessClassification: + review_id = getattr(review, "review_id", None) + author = getattr(review, "author", None) + state = getattr(review, "state", None) + submitted_at = getattr(review, "submitted_at", None) + commit_id = getattr(review, "commit_id", None) + payload = getattr(review, "payload", {}) + if not isinstance(payload, dict): + payload = {} + malformed = not review_id or not isinstance(state, str) or not state.strip() + is_dismissed = isinstance(state, str) and state.upper() == "DISMISSED" + submitted_dt = _timestamp_value(submitted_at) + boundary_dt = _timestamp_value(cycle_boundary) + is_after_boundary = boundary_dt is None or (submitted_dt is not None and submitted_dt >= boundary_dt) + is_current_head = isinstance(commit_id, str) and isinstance(current_head_sha, str) and commit_id.strip() == current_head_sha.strip() + is_assigned = isinstance(author, str) and isinstance(assigned_reviewer, str) and author.lower() == assigned_reviewer.lower() + if malformed: + scope = "malformed" + reason = "malformed_review_snapshot" + elif is_dismissed: + scope = "dismissed_or_superseded" + reason = "review_dismissed_or_superseded" + elif not is_after_boundary: + scope = "before_cycle" + reason = "review_before_cycle_boundary" + elif not is_current_head: + scope = "stale_head" + reason = "review_not_on_current_head" + elif is_assigned: + scope = "current_head_assigned_reviewer" + reason = None + elif isinstance(author, str) and author.strip(): + scope = "current_head_alternate_reviewer" + reason = "alternate_reviewer_diagnostic_only" + else: + scope = "unknown" + reason = "unknown_review_author" + return ReviewFreshnessClassification( + review_id=review_id if isinstance(review_id, (int, str)) else "", + author=author if isinstance(author, str) and author.strip() else None, + state=state.upper() if isinstance(state, str) and state.strip() else None, + submitted_at=submitted_at if isinstance(submitted_at, str) and submitted_at.strip() else None, + commit_id=commit_id if isinstance(commit_id, str) and commit_id.strip() else None, + classified_scope=scope, + is_assigned_reviewer=is_assigned, + is_current_head=is_current_head, + is_after_cycle_boundary=is_after_boundary, + is_dismissed_or_superseded=is_dismissed, + diagnostic_reason=reason, + payload=payload, + ) + + def filter_current_head_reviews_for_cycle( reviews: list[dict], *, @@ -122,17 +244,27 @@ def filter_current_head_reviews_for_cycle( current_head: str, ) -> dict[str, dict]: survivors: dict[str, dict] = {} + from . import reviewer_review_helpers + for review in reviews: if not isinstance(review, dict): continue - state = str(review.get("state", "")).upper() - if state == "DISMISSED": + snapshot = reviewer_review_helpers.build_review_snapshot_record(review) + if snapshot is None: continue - submitted_at = review.get("submitted_at") - if not isinstance(submitted_at, datetime) or submitted_at < boundary: + classification = classify_review_freshness( + snapshot, + current_head_sha=current_head, + cycle_boundary=boundary, + assigned_reviewer=None, + ) + if classification.classified_scope not in { + "current_head_assigned_reviewer", + "current_head_alternate_reviewer", + }: continue - commit_id = review.get("commit_id") - if not isinstance(commit_id, str) or not commit_id.strip() or commit_id.strip() != current_head: + submitted_at = review.get("submitted_at") + if not isinstance(submitted_at, datetime): continue author = review.get("user", {}).get("login") if not isinstance(author, str) or not author.strip(): @@ -153,6 +285,57 @@ def filter_current_head_reviews_for_cycle( return survivors +def build_current_review_context( + bot, + issue_number: int, + review_data: dict, + *, + pull_request: dict | None = None, + reviews: list[dict] | None = None, +) -> CurrentReviewContext: + from . import reviewer_review_helpers + + pull_request_result = read_pull_request_result(bot, issue_number, pull_request) + current_head_sha = None + if pull_request_result.get("ok"): + head = pull_request_result.get("pull_request", {}).get("head") + current_head_sha = head.get("sha") if isinstance(head, dict) else None + boundary = get_current_cycle_boundary(review_data, parse_timestamp=parse_github_timestamp) + cycle_boundary = boundary.isoformat() if isinstance(boundary, datetime) else None + reviews_result = read_pull_request_reviews_result(bot, issue_number, reviews) + if not reviews_result.get("ok"): + return CurrentReviewContext( + issue_number=issue_number, + current_head_sha=current_head_sha, + cycle_boundary=cycle_boundary, + live_reviews_available=False, + live_reviews_failure_kind=str(reviews_result.get("failure_kind") or reviews_result.get("reason") or "unavailable"), + classifications=(), + ) + assigned_reviewer = review_data.get("current_reviewer") if isinstance(review_data.get("current_reviewer"), str) else None + classifications = [] + for review in reviews_result.get("reviews", []): + snapshot = reviewer_review_helpers.build_review_snapshot_record(review) + if snapshot is None: + continue + classifications.append( + classify_review_freshness( + snapshot, + current_head_sha=current_head_sha, + cycle_boundary=cycle_boundary, + assigned_reviewer=assigned_reviewer, + ) + ) + return CurrentReviewContext( + issue_number=issue_number, + current_head_sha=current_head_sha if isinstance(current_head_sha, str) and current_head_sha.strip() else None, + cycle_boundary=cycle_boundary, + live_reviews_available=True, + live_reviews_failure_kind=None, + classifications=tuple(classifications), + ) + + def collect_permission_statuses(survivors: dict[str, dict], *, permission_status) -> dict[str, str]: statuses: dict[str, str] = {} for review in survivors.values(): diff --git a/scripts/reviewer_bot_core/reconcile_replay_policy.py b/scripts/reviewer_bot_core/reconcile_replay_policy.py index 40bc4f6a0..e6108ceb9 100644 --- a/scripts/reviewer_bot_core/reconcile_replay_policy.py +++ b/scripts/reviewer_bot_core/reconcile_replay_policy.py @@ -32,6 +32,28 @@ class ReviewReplayDecision: clear_gap: bool +@dataclass(frozen=True) +class DismissedReviewReplayPlan: + record_channel_event: bool + rebuild_live_approval: bool + mark_reconciled: bool + clear_gap: bool + replay_timestamp: str | None + diagnostic_reason: str | None + failure_kind: str | None + + def to_output(self) -> dict[str, object]: + return { + "record_channel_event": self.record_channel_event, + "rebuild_live_approval": self.rebuild_live_approval, + "mark_reconciled": self.mark_reconciled, + "clear_gap": self.clear_gap, + "replay_timestamp": self.replay_timestamp, + "diagnostic_reason": self.diagnostic_reason, + "failure_kind": self.failure_kind, + } + + def decide_comment_replay( *, comment_id: int, @@ -155,3 +177,42 @@ def decide_review_dismissed_replay(*, source_event_key: str, timestamp: str) -> mark_reconciled=True, clear_gap=True, ) + + +def decide_review_dismissed_replay_plan( + *, + source_event_key: str, + dismissal_timestamp: str | None, + dismissal_exact: bool, + live_pr_readable: bool, +) -> DismissedReviewReplayPlan: + del source_event_key + if not live_pr_readable: + return DismissedReviewReplayPlan( + record_channel_event=False, + rebuild_live_approval=False, + mark_reconciled=False, + clear_gap=False, + replay_timestamp=None, + diagnostic_reason="live_pr_unreadable", + failure_kind="live_pr_unreadable", + ) + if not dismissal_exact or not isinstance(dismissal_timestamp, str) or not dismissal_timestamp.strip(): + return DismissedReviewReplayPlan( + record_channel_event=False, + rebuild_live_approval=True, + mark_reconciled=False, + clear_gap=False, + replay_timestamp=None, + diagnostic_reason="dismissal_time_not_exact", + failure_kind="dismissal_time_unavailable", + ) + return DismissedReviewReplayPlan( + record_channel_event=True, + rebuild_live_approval=True, + mark_reconciled=True, + clear_gap=True, + replay_timestamp=dismissal_timestamp, + diagnostic_reason=None, + failure_kind=None, + ) diff --git a/scripts/reviewer_bot_core/reviewer_response_policy.py b/scripts/reviewer_bot_core/reviewer_response_policy.py index 54d4ed581..80da95130 100644 --- a/scripts/reviewer_bot_core/reviewer_response_policy.py +++ b/scripts/reviewer_bot_core/reviewer_response_policy.py @@ -14,6 +14,7 @@ from __future__ import annotations +from dataclasses import dataclass from datetime import datetime, timezone from . import live_review_support, reviewer_review_helpers @@ -41,6 +42,163 @@ ) +@dataclass(frozen=True) +class ReviewCycleScope: + issue_number: int | None + reviewer: str | None + head_sha: str | None + cycle_key: str | None + scope_key: str | None + scope_basis: str | None + anchor_timestamp: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "head_sha": self.head_sha, + "cycle_key": self.cycle_key, + "scope_key": self.scope_key, + "scope_basis": self.scope_basis, + "anchor_timestamp": self.anchor_timestamp, + } + + +@dataclass(frozen=True) +class ReviewerResponseDecision: + response_state: str + reason: str | None + suppression_reason: str | None + scope: ReviewCycleScope | None + current_head_sha: str | None + anchor_timestamp: str | None + reviewer_authority_outcome: str | None + latest_reviewer_activity_kind: str | None + latest_reviewer_activity_timestamp: str | None + latest_contributor_handoff_timestamp: str | None + suppresses_overdue_reminder: bool + suppresses_reassignment_followup: bool + completion_state: str | None + write_approval_authority: dict[str, object] | None + + def to_output(self) -> dict[str, object]: + return { + "response_state": self.response_state, + "reason": self.reason, + "suppression_reason": self.suppression_reason, + "scope": self.scope.to_output() if self.scope is not None else None, + "current_head_sha": self.current_head_sha, + "anchor_timestamp": self.anchor_timestamp, + "reviewer_authority_outcome": self.reviewer_authority_outcome, + "latest_reviewer_activity_kind": self.latest_reviewer_activity_kind, + "latest_reviewer_activity_timestamp": self.latest_reviewer_activity_timestamp, + "latest_contributor_handoff_timestamp": self.latest_contributor_handoff_timestamp, + "suppresses_overdue_reminder": self.suppresses_overdue_reminder, + "suppresses_reassignment_followup": self.suppresses_reassignment_followup, + "completion_state": self.completion_state, + "write_approval_authority": self.write_approval_authority, + } + + +@dataclass(frozen=True) +class ReviewerActivityRecord: + kind: str + actor: str | None + timestamp: str | None + source_event_key: str | None + reviewed_head_sha: str | None + activity_scope: str + payload: dict[str, object] + + def to_output(self) -> dict[str, object]: + return { + "kind": self.kind, + "actor": self.actor, + "timestamp": self.timestamp, + "source_event_key": self.source_event_key, + "reviewed_head_sha": self.reviewed_head_sha, + "activity_scope": self.activity_scope, + "payload": dict(self.payload), + } + + +def derive_review_cycle_scope( + payload: dict[str, object], + *, + issue_number: int | None = None, + reviewer: str | None = None, +) -> ReviewCycleScope | None: + scope_key = payload.get("current_scope_key") or payload.get("scope_key") + if not isinstance(scope_key, str) or not scope_key.strip(): + return None + return ReviewCycleScope( + issue_number=issue_number, + reviewer=reviewer, + head_sha=payload.get("current_head_sha") if isinstance(payload.get("current_head_sha"), str) else None, + cycle_key=payload.get("cycle_key") if isinstance(payload.get("cycle_key"), str) else None, + scope_key=scope_key, + scope_basis=payload.get("current_scope_basis") if isinstance(payload.get("current_scope_basis"), str) else None, + anchor_timestamp=payload.get("anchor_timestamp") if isinstance(payload.get("anchor_timestamp"), str) else None, + ) + + +def to_reviewer_response_decision(payload: dict[str, object]) -> ReviewerResponseDecision: + response_state = str(payload.get("response_state") or payload.get("state") or "projection_failed") + scope = derive_review_cycle_scope( + payload, + issue_number=payload.get("issue_number") if isinstance(payload.get("issue_number"), int) else None, + reviewer=payload.get("current_reviewer") if isinstance(payload.get("current_reviewer"), str) else None, + ) + return ReviewerResponseDecision( + response_state=response_state, + reason=payload.get("reason") if isinstance(payload.get("reason"), str) else None, + suppression_reason=payload.get("suppression_reason") if isinstance(payload.get("suppression_reason"), str) else None, + scope=scope, + current_head_sha=payload.get("current_head_sha") if isinstance(payload.get("current_head_sha"), str) else None, + anchor_timestamp=payload.get("anchor_timestamp") if isinstance(payload.get("anchor_timestamp"), str) else None, + reviewer_authority_outcome=payload.get("reviewer_authority_outcome") if isinstance(payload.get("reviewer_authority_outcome"), str) else None, + latest_reviewer_activity_kind=payload.get("latest_reviewer_activity_kind") if isinstance(payload.get("latest_reviewer_activity_kind"), str) else None, + latest_reviewer_activity_timestamp=payload.get("latest_reviewer_activity_timestamp") if isinstance(payload.get("latest_reviewer_activity_timestamp"), str) else None, + latest_contributor_handoff_timestamp=payload.get("latest_contributor_handoff_timestamp") if isinstance(payload.get("latest_contributor_handoff_timestamp"), str) else None, + suppresses_overdue_reminder=bool(payload.get("suppresses_overdue_reminder", response_state != "awaiting_reviewer_response")), + suppresses_reassignment_followup=bool(payload.get("suppresses_reassignment_followup", response_state in {"done", "closed", "untracked"})), + completion_state=payload.get("completion_state") if isinstance(payload.get("completion_state"), str) else None, + write_approval_authority=payload.get("write_approval_authority") if isinstance(payload.get("write_approval_authority"), dict) else None, + ) + + +def apply_reminder_cadence_overlay(response: ReviewerResponseDecision, cadence) -> ReviewerResponseDecision: + if cadence is None or not getattr(cadence, "must_project_reassignment_needed", False): + return response + reason = getattr(cadence, "exhaustion_reason", None) or "legacy_duplicate_reminders_exhausted" + return ReviewerResponseDecision( + response_state="reviewer_reassignment_needed", + reason=reason, + suppression_reason=reason, + scope=response.scope, + current_head_sha=response.current_head_sha, + anchor_timestamp=response.anchor_timestamp, + reviewer_authority_outcome=response.reviewer_authority_outcome, + latest_reviewer_activity_kind=response.latest_reviewer_activity_kind, + latest_reviewer_activity_timestamp=response.latest_reviewer_activity_timestamp, + latest_contributor_handoff_timestamp=response.latest_contributor_handoff_timestamp, + suppresses_overdue_reminder=True, + suppresses_reassignment_followup=True, + completion_state=response.completion_state, + write_approval_authority=response.write_approval_authority, + ) + + +def classify_reviewer_activity_scope(activity: ReviewerActivityRecord, *, current_head_sha: str | None) -> str: + if activity.activity_scope in {"current_head", "stale_head", "issue_thread", "diagnostic_only", "unknown"}: + return activity.activity_scope + if activity.reviewed_head_sha is None: + return "issue_thread" if activity.kind == "reviewer_comment" else "diagnostic_only" + if isinstance(current_head_sha, str) and activity.reviewed_head_sha == current_head_sha: + return "current_head" + return "stale_head" + + def _record_timestamp(record: dict | None, *, parse_timestamp) -> datetime | None: if not isinstance(record, dict): return None @@ -289,7 +447,7 @@ def _decorate_response( scope_fields: dict[str, object], **payload, ) -> dict[str, object]: - return { + legacy = { "state": state, "response_state": state, "reason": reason, @@ -297,6 +455,14 @@ def _decorate_response( **scope_fields, **payload, } + decision = to_reviewer_response_decision(legacy) + return { + **legacy, + **decision.to_output(), + "state": decision.response_state, + "current_scope_key": scope_fields.get("current_scope_key"), + "current_scope_basis": scope_fields.get("current_scope_basis"), + } def _record_for_current_reviewer(record: dict | None | object, current_reviewer: str) -> dict | None: @@ -669,26 +835,7 @@ def derive_reviewer_response_state( contributor_comment=contributor_comment, contributor_handoff=contributor_handoff, ) - if any(author.lower() != current_reviewer.lower() for author in approval_authors): - return _decorate_response( - state="awaiting_contributor_response", - reason="current_head_alternate_approval_present", - scope_fields=_current_scope_fields( - review_data, - current_reviewer, - current_head, - contributor_handoff, - alternate_current_head_approval=True, - alternate_current_head_cycle_boundary=alternate_current_head_cycle_boundary, - ), - anchor_timestamp=latest_reviewer_response.get("timestamp") if isinstance(latest_reviewer_response, dict) else None, - current_head_sha=current_head, - reviewer_comment=reviewer_comment, - reviewer_review=reviewer_review, - current_cycle_reviewer_handoff=reviewer_handoff, - contributor_comment=contributor_comment, - contributor_handoff=contributor_handoff, - ) + del alternate_current_head_cycle_boundary latest_review_head = reviewer_review.get("reviewed_head_sha") if isinstance(reviewer_review, dict) else None if not isinstance(latest_review_head, str) or latest_review_head != current_head: @@ -741,6 +888,30 @@ def derive_reviewer_response_state( contributor_comment=contributor_comment, contributor_handoff=contributor_handoff, ) + authority_response_state = write_approval.get("response_state") + if authority_response_state == "awaiting_contributor_response": + return _decorate_response( + state="awaiting_contributor_response", + reason="assigned_reviewer_review_submitted", + scope_fields=_current_scope_fields(review_data, current_reviewer, current_head, contributor_handoff), + anchor_timestamp=latest_reviewer_response.get("timestamp") if isinstance(latest_reviewer_response, dict) else None, + current_head_sha=current_head, + reviewer_comment=reviewer_comment, + reviewer_review=reviewer_review, + current_cycle_reviewer_handoff=reviewer_handoff, + contributor_comment=contributor_comment, + contributor_handoff=contributor_handoff, + ) + if authority_response_state == "projection_failed": + authority = write_approval.get("authority_decision") + reason = "write_approval_authority_unavailable" + if isinstance(authority, dict) and isinstance(authority.get("diagnostic_reason"), str): + reason = str(authority["diagnostic_reason"]) + return _decorate_response( + state="projection_failed", + reason=reason, + scope_fields=_current_scope_fields(review_data, current_reviewer, current_head, contributor_handoff), + ) if not write_approval.get("has_write_approval"): return _decorate_response( state="awaiting_write_approval", diff --git a/scripts/reviewer_bot_core/reviewer_review_helpers.py b/scripts/reviewer_bot_core/reviewer_review_helpers.py index ea5604794..f5dcc79cf 100644 --- a/scripts/reviewer_bot_core/reviewer_review_helpers.py +++ b/scripts/reviewer_bot_core/reviewer_review_helpers.py @@ -1,10 +1,33 @@ from __future__ import annotations +from dataclasses import dataclass from datetime import datetime, timezone from . import live_review_support +@dataclass(frozen=True) +class ReviewSnapshotRecord: + review_id: int | str + state: str + author: str | None + submitted_at: str | None + commit_id: str | None + source_precedence: int + payload: dict[str, object] + + def to_output(self) -> dict[str, object]: + return { + "review_id": self.review_id, + "state": self.state, + "author": self.author, + "submitted_at": self.submitted_at, + "commit_id": self.commit_id, + "source_precedence": self.source_precedence, + "payload": dict(self.payload), + } + + def compare_records( left: dict | None, right: dict | None, @@ -111,21 +134,40 @@ def get_preferred_current_reviewer_review_for_cycle( def build_reviewer_review_record_from_live_review(review: dict, *, actor: str | None = None) -> dict | None: + snapshot = build_review_snapshot_record(review, actor=actor) + if snapshot is None: + return None + if snapshot.submitted_at is None or snapshot.commit_id is None or snapshot.author is None: + return None + return { + "semantic_key": f"pull_request_review:{snapshot.review_id}", + "timestamp": snapshot.submitted_at, + "actor": snapshot.author, + "reviewed_head_sha": snapshot.commit_id, + "source_precedence": 1, + "payload": snapshot.to_output(), + } + + +def build_review_snapshot_record(review: dict, *, actor: str | None = None) -> ReviewSnapshotRecord | None: if not isinstance(review, dict): return None review_id = review.get("id") + state = review.get("state") submitted_at = review.get("submitted_at") commit_id = review.get("commit_id") author = actor if isinstance(actor, str) and actor.strip() else review.get("user", {}).get("login") - if not isinstance(review_id, int) or not isinstance(submitted_at, str) or not isinstance(commit_id, str): + if not isinstance(review_id, (int, str)) or not str(review_id).strip(): return None - if not isinstance(author, str) or not author.strip(): + if not isinstance(state, str) or not state.strip(): return None - return { - "semantic_key": f"pull_request_review:{review_id}", - "timestamp": submitted_at, - "actor": author, - "reviewed_head_sha": commit_id, - "source_precedence": 1, - "payload": {}, - } + payload = {key: value for key, value in review.items() if key not in {"body"}} + return ReviewSnapshotRecord( + review_id=review_id, + state=state.upper(), + author=author if isinstance(author, str) and author.strip() else None, + submitted_at=submitted_at if isinstance(submitted_at, str) and submitted_at.strip() else None, + commit_id=commit_id if isinstance(commit_id, str) and commit_id.strip() else None, + source_precedence=1, + payload=payload, + ) diff --git a/scripts/reviewer_bot_lib/app.py b/scripts/reviewer_bot_lib/app.py index a0cfe8209..eed373799 100644 --- a/scripts/reviewer_bot_lib/app.py +++ b/scripts/reviewer_bot_lib/app.py @@ -1,6 +1,7 @@ """Top-level reviewer-bot orchestration.""" import sys +from dataclasses import dataclass from . import maintenance, reconcile from .context import EventContext, ExecutionResult @@ -13,6 +14,107 @@ from .runtime_protocols import AppEventContextRuntime, AppExecutionRuntime +@dataclass(frozen=True) +class StateIssueWriteReceipt: + issue_number: int | None + mutation_kind: str + issue_body_hash_before: str | None + issue_body_hash_after: str | None + external_side_effects_recorded: tuple[str, ...] + state_save_attempted: bool + state_save_succeeded: bool + write_status: str + failure_kind: str | None + next_recovery_action: str | None + recorded_at: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "mutation_kind": self.mutation_kind, + "issue_body_hash_before": self.issue_body_hash_before, + "issue_body_hash_after": self.issue_body_hash_after, + "external_side_effects_recorded": sorted(self.external_side_effects_recorded), + "state_save_attempted": self.state_save_attempted, + "state_save_succeeded": self.state_save_succeeded, + "write_status": self.write_status, + "failure_kind": self.failure_kind, + "next_recovery_action": self.next_recovery_action, + "recorded_at": self.recorded_at, + } + + +def build_state_issue_write_receipt( + *, + issue_number: int | None, + mutation_kind: str, + issue_body_hash_before: str | None, + issue_body_hash_after: str | None, + external_side_effects_recorded: tuple[str, ...], + state_save_attempted: bool, + state_save_succeeded: bool, + failure_kind: str | None = None, + recorded_at: str | None = None, +) -> StateIssueWriteReceipt: + if not state_save_attempted: + write_status = "not_attempted_read_only" if mutation_kind.endswith("preview") else "not_attempted_no_state_change" + recovery = "none" + elif state_save_succeeded: + write_status = "persisted" + recovery = "none" + elif external_side_effects_recorded: + write_status = "failed_after_external_side_effect" + recovery = ( + "recover_from_live_github_receipt" + if any("comment" in item for item in external_side_effects_recorded) + else "retry_state_save_before_replay_closeout" + ) + else: + write_status = "failed_before_external_side_effect" + recovery = "retry_state_save_before_replay_closeout" + return StateIssueWriteReceipt( + issue_number=issue_number, + mutation_kind=mutation_kind, + issue_body_hash_before=issue_body_hash_before, + issue_body_hash_after=issue_body_hash_after, + external_side_effects_recorded=external_side_effects_recorded, + state_save_attempted=state_save_attempted, + state_save_succeeded=state_save_succeeded, + write_status=write_status, + failure_kind=failure_kind, + next_recovery_action=recovery, + recorded_at=recorded_at, + ) + + +def record_state_issue_write_receipt( + bot, + *, + issue_number: int | None, + mutation_kind: str, + issue_body_hash_before: str | None, + issue_body_hash_after: str | None, + external_side_effects_recorded: tuple[str, ...], + state_save_attempted: bool, + state_save_succeeded: bool, + failure_kind: str | None = None, +) -> StateIssueWriteReceipt: + receipt = build_state_issue_write_receipt( + issue_number=issue_number, + mutation_kind=mutation_kind, + issue_body_hash_before=issue_body_hash_before, + issue_body_hash_after=issue_body_hash_after, + external_side_effects_recorded=external_side_effects_recorded, + state_save_attempted=state_save_attempted, + state_save_succeeded=state_save_succeeded, + failure_kind=failure_kind, + recorded_at=bot.datetime.now(bot.timezone.utc).isoformat() if hasattr(bot, "datetime") else None, + ) + if hasattr(bot, "logger"): + bot.logger.event("info", "state issue write receipt", **receipt.to_output()) + return receipt + + def _log(bot: AppExecutionRuntime, level: str, message: str, **fields) -> None: bot.logger.event(level, message, **fields) @@ -86,7 +188,7 @@ def _classify_event_intent_from_context(bot: AppEventContextRuntime, context: Ev if event_name == "workflow_run": if event_action != "completed": return bot.EVENT_INTENT_NON_MUTATING_READONLY - if context.workflow_kind == "reconcile" and context.workflow_run_triggering_conclusion == "success": + if context.workflow_kind == "reconcile": return bot.EVENT_INTENT_MUTATING return bot.EVENT_INTENT_NON_MUTATING_READONLY @@ -163,7 +265,6 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes event_name == "workflow_run" and event_action == "completed" and context.workflow_kind == "reconcile" - and context.workflow_run_triggering_conclusion == "success" and reconcile.optional_router_payload_missing(bot, context) ): workflow_run_missing_optional_payload = True @@ -265,7 +366,7 @@ def execute_run(bot: AppExecutionRuntime, context: EventContext) -> ExecutionRes "info", "Skipping successful router workflow_run with no deferred artifact.", ) - elif context.workflow_kind == "reconcile" and context.workflow_run_triggering_conclusion == "success": + elif context.workflow_kind == "reconcile": workflow_run_result = reconcile.handle_workflow_run_event_result(bot, state) state_changed = workflow_run_result.state_changed else: diff --git a/scripts/reviewer_bot_lib/assignment_flow.py b/scripts/reviewer_bot_lib/assignment_flow.py index 4f8b0ebaf..6881f04be 100644 --- a/scripts/reviewer_bot_lib/assignment_flow.py +++ b/scripts/reviewer_bot_lib/assignment_flow.py @@ -2,6 +2,8 @@ from __future__ import annotations +from dataclasses import dataclass + from .config import CODING_GUIDELINE_LABEL from .guidance import ( get_assignment_failure_comment, @@ -18,6 +20,291 @@ ) +@dataclass(frozen=True) +class ReviewControlPlaneSnapshot: + issue_number: int + is_pull_request: bool + live_read_ok: bool + tracked_reviewer: str | None + requested_reviewers: tuple[str, ...] + assignees: tuple[str, ...] + author_login: str | None + review_decision: str | None + head_sha: str | None + snapshot_source: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "is_pull_request": self.is_pull_request, + "live_read_ok": self.live_read_ok, + "tracked_reviewer": self.tracked_reviewer, + "requested_reviewers": sorted(self.requested_reviewers), + "assignees": sorted(self.assignees), + "author_login": self.author_login, + "review_decision": self.review_decision, + "head_sha": self.head_sha, + "snapshot_source": self.snapshot_source, + "diagnostic_reason": self.diagnostic_reason, + } + + def to_legacy_dict(self) -> dict[str, object]: + return self.to_output() + + +@dataclass(frozen=True) +class ReviewerAuthorityResolution: + authority_status: str + tracked_reviewer: str | None + live_control_plane_reviewers: tuple[str, ...] + reason: str + is_pull_request: bool + live_read_ok: bool + + def to_output(self) -> dict[str, object]: + return { + "authority_status": self.authority_status, + "tracked_reviewer": self.tracked_reviewer, + "live_control_plane_reviewers": sorted(self.live_control_plane_reviewers), + "reason": self.reason, + "is_pull_request": self.is_pull_request, + "live_read_ok": self.live_read_ok, + } + + def to_legacy_dict(self) -> dict[str, object]: + return self.to_output() + + +@dataclass(frozen=True) +class ClaimCycleTransition: + issue_number: int + reviewer: str + previous_reviewer: str | None + cycle_started_at: str + assigned_at_before: str | None + assigned_at_after: str | None + same_reviewer_noop: bool + state_changed: bool + guidance_emitted: bool + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "previous_reviewer": self.previous_reviewer, + "cycle_started_at": self.cycle_started_at, + "assigned_at_before": self.assigned_at_before, + "assigned_at_after": self.assigned_at_after, + "same_reviewer_noop": self.same_reviewer_noop, + "state_changed": self.state_changed, + "guidance_emitted": self.guidance_emitted, + } + + +@dataclass(frozen=True) +class ReviewerAssignmentConfirmation: + issue_number: int + reviewer: str + is_pull_request: bool + assignment_method: str + previous_reviewer: str | None + live_before: tuple[str, ...] + live_after: tuple[str, ...] | None + assignment_attempt_status_code: int | None + assignment_attempt_failure_kind: str | None + assignment_write_status: str + state_tracking_allowed: bool + set_current_reviewer: bool + clear_current_reviewer: bool + same_reviewer_noop: bool + guidance_emitted: bool + failure_comment: str | None + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "is_pull_request": self.is_pull_request, + "assignment_method": self.assignment_method, + "previous_reviewer": self.previous_reviewer, + "live_before": sorted(self.live_before), + "live_after": sorted(self.live_after) if self.live_after is not None else None, + "assignment_attempt_status_code": self.assignment_attempt_status_code, + "assignment_attempt_failure_kind": self.assignment_attempt_failure_kind, + "assignment_write_status": self.assignment_write_status, + "state_tracking_allowed": self.state_tracking_allowed, + "set_current_reviewer": self.set_current_reviewer, + "clear_current_reviewer": self.clear_current_reviewer, + "same_reviewer_noop": self.same_reviewer_noop, + "guidance_emitted": self.guidance_emitted, + "failure_comment": self.failure_comment, + "diagnostic_reason": self.diagnostic_reason, + } + + +def _tuple_logins(values) -> tuple[str, ...]: + if not isinstance(values, (list, tuple, set)): + return () + return tuple(str(value) for value in values if isinstance(value, str) and value.strip()) + + +def build_review_control_plane_snapshot(issue_snapshot: dict, *, tracked_reviewer: str | None) -> ReviewControlPlaneSnapshot: + issue_number = int(issue_snapshot.get("number") or issue_snapshot.get("issue_number") or 0) + pull_request = issue_snapshot.get("pull_request") + is_pull_request = isinstance(pull_request, dict) + requested = issue_snapshot.get("requested_reviewers") or issue_snapshot.get("review_requests") or () + requested_reviewers = tuple( + str(item.get("login") if isinstance(item, dict) else item) + for item in requested + if isinstance(item, (dict, str)) + ) + assignees = tuple( + str(item.get("login") if isinstance(item, dict) else item) + for item in issue_snapshot.get("assignees", ()) + if isinstance(item, (dict, str)) + ) + author = issue_snapshot.get("user") + head = issue_snapshot.get("head") + return ReviewControlPlaneSnapshot( + issue_number=issue_number, + is_pull_request=is_pull_request, + live_read_ok=True, + tracked_reviewer=tracked_reviewer, + requested_reviewers=tuple(value for value in requested_reviewers if value.strip()), + assignees=tuple(value for value in assignees if value.strip()), + author_login=author.get("login") if isinstance(author, dict) and isinstance(author.get("login"), str) else None, + review_decision=issue_snapshot.get("reviewDecision") if isinstance(issue_snapshot.get("reviewDecision"), str) else None, + head_sha=head.get("sha") if isinstance(head, dict) and isinstance(head.get("sha"), str) else None, + snapshot_source="github_live_snapshot", + diagnostic_reason=None, + ) + + +def derive_claim_cycle_transition(review_data: dict | None, *, issue_number: int, reviewer: str, now: str) -> ClaimCycleTransition: + previous = review_data.get("current_reviewer") if isinstance(review_data, dict) else None + assigned_before = review_data.get("assigned_at") if isinstance(review_data, dict) and isinstance(review_data.get("assigned_at"), str) else None + same_reviewer = isinstance(previous, str) and previous.lower() == reviewer.lower() + return ClaimCycleTransition( + issue_number=issue_number, + reviewer=reviewer, + previous_reviewer=previous if isinstance(previous, str) else None, + cycle_started_at=now, + assigned_at_before=assigned_before, + assigned_at_after=assigned_before if same_reviewer else now, + same_reviewer_noop=same_reviewer, + state_changed=not same_reviewer, + guidance_emitted=not same_reviewer, + ) + + +def derive_reviewer_assignment_confirmation( + request, + *, + reviewer: str, + assignment_method: str, + previous_reviewer: str | None, + live_before: tuple[str, ...] | None, + live_after: tuple[str, ...] | None, + assignment_attempt: object | None, + removal_attempts: dict[str, object], + same_reviewer_noop: bool, + guidance_emitted: bool, +) -> ReviewerAssignmentConfirmation: + del removal_attempts + status_code = getattr(assignment_attempt, "status_code", None) + failure_kind = getattr(assignment_attempt, "failure_kind", None) + before = live_before or () + after = live_after + reviewer_key = reviewer.lower() + before_has = reviewer_key in {value.lower() for value in before} + after_has = reviewer_key in {value.lower() for value in after or ()} + if same_reviewer_noop or before_has and after is not None and after_has: + write_status = "already_live_assigned" + state_tracking_allowed = True + set_current = not same_reviewer_noop + diagnostic = None + elif after is None: + write_status = "blocked_live_read_unavailable" + state_tracking_allowed = False + set_current = False + diagnostic = "live_assignment_confirmation_unavailable" + elif after_has and len(after) == 1: + write_status = "live_assignment_confirmed" + state_tracking_allowed = True + set_current = True + diagnostic = None + elif status_code == 422 and request.is_pull_request: + write_status = "state_tracked_after_api_422" + state_tracking_allowed = True + set_current = True + diagnostic = "pr_reviewer_api_422_retained_tracked_state" + elif request.is_pull_request and not after: + write_status = "state_tracked_without_live_request" + state_tracking_allowed = True + set_current = True + diagnostic = "absent_live_pr_review_request_retained" + else: + write_status = "blocked_final_mismatch" + state_tracking_allowed = False + set_current = False + diagnostic = "live_control_plane_final_mismatch" + return ReviewerAssignmentConfirmation( + issue_number=request.issue_number, + reviewer=reviewer, + is_pull_request=bool(request.is_pull_request), + assignment_method=assignment_method, + previous_reviewer=previous_reviewer, + live_before=before, + live_after=after, + assignment_attempt_status_code=status_code, + assignment_attempt_failure_kind=failure_kind, + assignment_write_status=write_status, + state_tracking_allowed=state_tracking_allowed, + set_current_reviewer=set_current, + clear_current_reviewer=write_status == "blocked_final_mismatch" and not request.is_pull_request, + same_reviewer_noop=same_reviewer_noop, + guidance_emitted=guidance_emitted, + failure_comment=None, + diagnostic_reason=diagnostic, + ) + + +def apply_reviewer_assignment_confirmation( + bot, + state: dict, + request, + confirmation: ReviewerAssignmentConfirmation, + *, + pr_head_sha: str | None, + record_assignment: bool, + emit_guidance: bool, +) -> bool: + if confirmation.same_reviewer_noop or not confirmation.state_tracking_allowed: + return False + if confirmation.clear_current_reviewer: + return clear_current_reviewer(state, confirmation.issue_number) + if not confirmation.set_current_reviewer: + return False + set_current_reviewer( + state, + confirmation.issue_number, + confirmation.reviewer, + assignment_method=confirmation.assignment_method, + at=_now_iso(bot), + ) + review_data = ensure_review_entry(state, confirmation.issue_number, create=True) + if request.is_pull_request and isinstance(review_data, dict) and isinstance(pr_head_sha, str) and pr_head_sha: + review_data["active_head_sha"] = pr_head_sha + if record_assignment: + bot.adapters.queue.record_assignment(state, confirmation.reviewer, confirmation.issue_number, "pr" if request.is_pull_request else "issue") + if emit_guidance: + _post_assignment_guidance(bot, request, confirmation.reviewer) + bot.collect_touched_item(confirmation.issue_number) + return True + + def _log(bot, level: str, message: str, **fields) -> None: bot.logger.event(level, message, **fields) @@ -120,58 +407,72 @@ def resolve_reviewer_authority( result = bot.github.get_issue_assignees_result(issue_number, is_pull_request=is_pull_request) _hard_fail_if_permission_denied(result, action="reviewer authority read", issue_number=issue_number) if not result.ok or not isinstance(result.payload, list): - return { - "authority_status": "live_read_unavailable", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": [], - "reason": str(result.failure_kind or "live_control_plane_unavailable"), - } + return ReviewerAuthorityResolution( + authority_status="live_read_unavailable", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=(), + reason="live_read_unavailable", + is_pull_request=is_pull_request, + live_read_ok=False, + ).to_legacy_dict() live_control_plane_reviewers = [value for value in result.payload if isinstance(value, str) and value.strip()] if not isinstance(tracked_reviewer, str) or not tracked_reviewer.strip(): - return { - "authority_status": "no_tracked_reviewer", - "tracked_reviewer": None, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "no_tracked_reviewer", - } + return ReviewerAuthorityResolution( + authority_status="no_tracked_reviewer", + tracked_reviewer=None, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="retained_without_live_pr_review_request", + is_pull_request=is_pull_request, + live_read_ok=True, + ).to_legacy_dict() normalized_reviewers = {value.lower() for value in live_control_plane_reviewers} tracked_key = tracked_reviewer.lower() if is_pull_request: if normalized_reviewers and tracked_key not in normalized_reviewers: - return { - "authority_status": "control_plane_mismatch", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "tracked_reviewer_missing_from_live_control_plane", - } - return { - "authority_status": "tracked_reviewer_confirmed", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "tracked_reviewer_confirmed", - } + return ReviewerAuthorityResolution( + authority_status="control_plane_mismatch", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="tracked_reviewer_missing_from_live_control_plane", + is_pull_request=True, + live_read_ok=True, + ).to_legacy_dict() + return ReviewerAuthorityResolution( + authority_status="tracked_reviewer_confirmed", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="present_in_live_control_plane" if normalized_reviewers else "retained_without_live_pr_review_request", + is_pull_request=True, + live_read_ok=True, + ).to_legacy_dict() if len(live_control_plane_reviewers) != 1: - return { - "authority_status": "control_plane_mismatch", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "invalid_live_assignee_count", - } + return ReviewerAuthorityResolution( + authority_status="control_plane_mismatch", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="invalid_live_assignee_count", + is_pull_request=False, + live_read_ok=True, + ).to_legacy_dict() if tracked_key != live_control_plane_reviewers[0].lower(): - return { - "authority_status": "control_plane_mismatch", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "stored_reviewer_mismatch", - } - return { - "authority_status": "tracked_reviewer_confirmed", - "tracked_reviewer": tracked_reviewer, - "live_control_plane_reviewers": live_control_plane_reviewers, - "reason": "tracked_reviewer_confirmed", - } + return ReviewerAuthorityResolution( + authority_status="control_plane_mismatch", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="tracked_reviewer_missing_from_live_control_plane", + is_pull_request=False, + live_read_ok=True, + ).to_legacy_dict() + return ReviewerAuthorityResolution( + authority_status="tracked_reviewer_confirmed", + tracked_reviewer=tracked_reviewer, + live_control_plane_reviewers=tuple(live_control_plane_reviewers), + reason="present_in_live_control_plane", + is_pull_request=False, + live_read_ok=True, + ).to_legacy_dict() def resolve_reviewer_command_authority( @@ -335,6 +636,30 @@ def confirm_reviewer_assignment( "current_assignees": live_before, "diagnostic_changed": diagnostic_changed, } + if ( + isinstance(stored_reviewer, str) + and stored_reviewer.lower() == reviewer.lower() + and _normalize_logins(live_before) == [reviewer.lower()] + ): + return { + "confirmed": True, + "reviewer": reviewer, + "current_assignees": live_before, + "final_assignees": live_before, + "assignment_confirmation": derive_reviewer_assignment_confirmation( + request, + reviewer=reviewer, + assignment_method=assignment_method, + previous_reviewer=stored_reviewer, + live_before=tuple(live_before), + live_after=tuple(live_before), + assignment_attempt=None, + removal_attempts={}, + same_reviewer_noop=True, + guidance_emitted=False, + ).to_output(), + "diagnostic_changed": diagnostic_changed, + } removal_attempts = {} live_before_normalized = _normalize_logins(live_before) for assignee in live_before: diff --git a/scripts/reviewer_bot_lib/comment_application.py b/scripts/reviewer_bot_lib/comment_application.py index 9a009ae38..fae5d6d59 100644 --- a/scripts/reviewer_bot_lib/comment_application.py +++ b/scripts/reviewer_bot_lib/comment_application.py @@ -31,6 +31,32 @@ class CommandExecutionResult: state_changed: bool +@dataclass(frozen=True) +class ReviewerFeedbackHandoffDecision: + authorized: bool + issue_number: int + actor: str + source_event_key: str + timestamp: str + reviewed_head_sha: str | None + replace_existing: bool + response_state: str + reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "authorized": self.authorized, + "issue_number": self.issue_number, + "actor": self.actor, + "source_event_key": self.source_event_key, + "timestamp": self.timestamp, + "reviewed_head_sha": self.reviewed_head_sha, + "replace_existing": self.replace_existing, + "response_state": self.response_state, + "reason": self.reason, + } + + def _route_comment_application(classified: dict, *, comment_id: int) -> str: comment_class = str(classified.get("comment_class", "")) command_count = int(classified.get("command_count", 0)) @@ -74,7 +100,9 @@ def record_conversation_freshness( if authority.get("authority_status") == "tracked_reviewer_confirmed": confirmed_reviewer = authority.get("tracked_reviewer") effective_review_data["current_reviewer"] = confirmed_reviewer - decision = comment_freshness_policy.decide_comment_freshness(effective_review_data, request) + current_head_sha = review_data.get("active_head_sha") if isinstance(review_data.get("active_head_sha"), str) else None + event = comment_freshness_policy.build_comment_freshness_event(effective_review_data, request) + decision = comment_freshness_policy.decide_comment_freshness_event(event, current_head_sha=current_head_sha) if decision.kind != "accept_channel_event": return False changed = accept_channel_event( @@ -289,6 +317,38 @@ def _feedback_handoff_should_replace(existing: object, candidate: dict) -> bool: return candidate_source == existing_source +def build_reviewer_feedback_handoff_decision( + review_data: dict, + request: CommentEventRequest, + authority, + *, + current_head_sha: str | None, +) -> ReviewerFeedbackHandoffDecision: + source_event_key = request.comment_source_event_key or f"issue_comment:{request.comment_id}" + reviewed_head_sha = current_head_sha if request.is_pull_request else None + authorized = bool(authority.get("authorized") if isinstance(authority, dict) else False) + reason = None if authorized else str((authority or {}).get("reason") or "unauthorized") + handoff = { + "source_event_key": source_event_key, + "timestamp": request.comment_created_at, + "actor": request.comment_author, + "command_name": comment_command_policy.OrdinaryCommandId.FEEDBACK.value, + "reviewed_head_sha": reviewed_head_sha, + } + replace_existing = _feedback_handoff_should_replace(review_data.get("current_cycle_reviewer_handoff"), handoff) + return ReviewerFeedbackHandoffDecision( + authorized=authorized, + issue_number=request.issue_number, + actor=request.comment_author, + source_event_key=source_event_key, + timestamp=request.comment_created_at, + reviewed_head_sha=reviewed_head_sha, + replace_existing=replace_existing, + response_state="awaiting_contributor_response" if authorized else "projection_failed", + reason=reason, + ) + + def handle_feedback_command(bot, state: dict, request: CommentEventRequest, decision) -> CommandExecutionResult: if request.issue_state.lower() != "open": return CommandExecutionResult( @@ -321,15 +381,21 @@ def handle_feedback_command(bot, state: dict, request: CommentEventRequest, deci state_changed=False, ) reviewed_head_sha = active_head_sha + handoff_decision = build_reviewer_feedback_handoff_decision( + review_data, + request, + authority, + current_head_sha=reviewed_head_sha, + ) handoff = { - "source_event_key": request.comment_source_event_key or f"issue_comment:{request.comment_id}", - "timestamp": request.comment_created_at, - "actor": decision.actor, + "source_event_key": handoff_decision.source_event_key, + "timestamp": handoff_decision.timestamp, + "actor": handoff_decision.actor, "command_name": comment_command_policy.OrdinaryCommandId.FEEDBACK.value, - "reviewed_head_sha": reviewed_head_sha, + "reviewed_head_sha": handoff_decision.reviewed_head_sha, } before = review_data.get("current_cycle_reviewer_handoff") - if not _feedback_handoff_should_replace(before, handoff): + if not handoff_decision.replace_existing: return CommandExecutionResult( response="ℹ️ Ignored stale `/feedback` handoff because a newer or same-time reviewer handoff is already recorded.", success=True, diff --git a/scripts/reviewer_bot_lib/context.py b/scripts/reviewer_bot_lib/context.py index 2c589aab8..f4eabefaf 100644 --- a/scripts/reviewer_bot_lib/context.py +++ b/scripts/reviewer_bot_lib/context.py @@ -56,6 +56,8 @@ class CommentEventRequest: comment_installation_id: str | None comment_performed_via_github_app: bool comment_author_association: str = "" + comment_source_kind: str = "issue_comment" + reviewed_head_sha: str | None = None @dataclass(frozen=True) diff --git a/scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py b/scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py index 2b858f87d..96a08acf4 100644 --- a/scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py +++ b/scripts/reviewer_bot_lib/deferred_gap_bookkeeping.py @@ -3,9 +3,161 @@ from __future__ import annotations from copy import deepcopy +from dataclasses import dataclass from datetime import datetime, timedelta, timezone +@dataclass(frozen=True) +class DeferredGap: + source_event_key: str + workflow_name: str | None + workflow_file: str | None + run_id: str | None + run_attempt: str | None + issue_number: int | None + source_event_action: str | None + failure_kind: str + diagnostic_reason: str | None + replay_allowed: bool + recorded_at: str | None + + def to_output(self) -> dict[str, object]: + return { + "source_event_key": self.source_event_key, + "workflow_name": self.workflow_name, + "workflow_file": self.workflow_file, + "run_id": self.run_id, + "run_attempt": self.run_attempt, + "issue_number": self.issue_number, + "source_event_action": self.source_event_action, + "failure_kind": self.failure_kind, + "diagnostic_reason": self.diagnostic_reason, + "replay_allowed": self.replay_allowed, + "recorded_at": self.recorded_at, + } + + +@dataclass(frozen=True) +class ObserverWatermark: + workflow_name: str + workflow_file: str | None + run_id: str | None + run_attempt: str | None + conclusion: str | None + artifact_name: str | None + artifact_path: str | None + diagnostics_retained: bool + recorded_at: str | None + + def to_output(self) -> dict[str, object]: + return { + "workflow_name": self.workflow_name, + "workflow_file": self.workflow_file, + "run_id": self.run_id, + "run_attempt": self.run_attempt, + "conclusion": self.conclusion, + "artifact_name": self.artifact_name, + "artifact_path": self.artifact_path, + "diagnostics_retained": self.diagnostics_retained, + "recorded_at": self.recorded_at, + } + + +@dataclass(frozen=True) +class ReconciledSourceEvent: + source_event_key: str + issue_number: int | None + source_event_action: str | None + replay_decision: str + mark_reconciled: bool + clear_gap: bool + reconciled_at: str | None + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "source_event_key": self.source_event_key, + "issue_number": self.issue_number, + "source_event_action": self.source_event_action, + "replay_decision": self.replay_decision, + "mark_reconciled": self.mark_reconciled, + "clear_gap": self.clear_gap, + "reconciled_at": self.reconciled_at, + "diagnostic_reason": self.diagnostic_reason, + } + + +def _optional_str(value: object) -> str | None: + return value if isinstance(value, str) and value.strip() else None + + +def _optional_int(value: object) -> int | None: + return value if isinstance(value, int) else None + + +def from_deferred_gap_state(row: dict[str, object]) -> DeferredGap: + return DeferredGap( + source_event_key=str(row.get("source_event_key") or ""), + workflow_name=_optional_str(row.get("workflow_name") or row.get("source_workflow_name")), + workflow_file=_optional_str(row.get("workflow_file") or row.get("source_workflow_file")), + run_id=str(row.get("source_run_id")) if row.get("source_run_id") is not None else None, + run_attempt=str(row.get("source_run_attempt")) if row.get("source_run_attempt") is not None else None, + issue_number=_optional_int(row.get("pr_number") or row.get("issue_number")), + source_event_action=_optional_str(row.get("source_event_kind") or row.get("source_event_action")), + failure_kind=str(row.get("failure_kind") or row.get("reason") or "unknown"), + diagnostic_reason=_optional_str(row.get("diagnostic_summary") or row.get("diagnostic_reason")), + replay_allowed=False, + recorded_at=_optional_str(row.get("last_checked_at") or row.get("first_noted_at")), + ) + + +def validate_deferred_gap(gap: DeferredGap) -> DeferredGap: + if not gap.source_event_key: + raise RuntimeError("DeferredGap.source_event_key must be non-empty") + return gap + + +def from_observer_watermark_state(row: dict[str, object]) -> ObserverWatermark: + return ObserverWatermark( + workflow_name=str(row.get("workflow_name") or row.get("surface") or ""), + workflow_file=_optional_str(row.get("workflow_file")), + run_id=str(row.get("run_id")) if row.get("run_id") is not None else None, + run_attempt=str(row.get("run_attempt")) if row.get("run_attempt") is not None else None, + conclusion=_optional_str(row.get("conclusion")), + artifact_name=_optional_str(row.get("artifact_name")), + artifact_path=_optional_str(row.get("artifact_path")), + diagnostics_retained=bool(row.get("diagnostics_retained", False)), + recorded_at=_optional_str(row.get("last_scan_completed_at") or row.get("recorded_at")), + ) + + +def validate_observer_watermark(watermark: ObserverWatermark) -> ObserverWatermark: + if not watermark.workflow_name: + raise RuntimeError("ObserverWatermark.workflow_name must be non-empty") + return watermark + + +def from_reconciled_source_event_state(row: dict[str, object]) -> ReconciledSourceEvent: + return ReconciledSourceEvent( + source_event_key=str(row.get("source_event_key") or ""), + issue_number=_optional_int(row.get("issue_number")), + source_event_action=_optional_str(row.get("source_event_action")), + replay_decision=str(row.get("replay_decision") or "pass_replayed_and_persisted"), + mark_reconciled=bool(row.get("mark_reconciled", True)), + clear_gap=bool(row.get("clear_gap", True)), + reconciled_at=_optional_str(row.get("reconciled_at")), + diagnostic_reason=_optional_str(row.get("diagnostic_reason")), + ) + + +def validate_reconciled_source_event(event: ReconciledSourceEvent) -> ReconciledSourceEvent: + if not event.source_event_key: + raise RuntimeError("ReconciledSourceEvent.source_event_key must be non-empty") + if event.mark_reconciled and not event.reconciled_at: + raise RuntimeError("ReconciledSourceEvent.reconciled_at is required when marked reconciled") + return event + + def _sidecars(review_data: dict) -> dict: sidecars = review_data.get("sidecars") if not isinstance(sidecars, dict): diff --git a/scripts/reviewer_bot_lib/event_inputs.py b/scripts/reviewer_bot_lib/event_inputs.py index 5eedbfd9b..7ed436fe6 100644 --- a/scripts/reviewer_bot_lib/event_inputs.py +++ b/scripts/reviewer_bot_lib/event_inputs.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from dataclasses import dataclass from datetime import datetime from pathlib import Path @@ -29,6 +30,69 @@ def __init__(self, builder: str, problems: tuple[str, ...]): super().__init__(f"{builder}: {'; '.join(problems)}") +@dataclass(frozen=True) +class LifecycleEventTimestampEvidence: + event_name: str + event_action: str + source_created_at: str | None + source_updated_at: str | None + source_closed_at: str | None + selected_timestamp: str | None + selection_kind: str + selection_reason: str + is_authoritative: bool + + def to_output(self) -> dict[str, object]: + return { + "event_name": self.event_name, + "event_action": self.event_action, + "source_created_at": self.source_created_at, + "source_updated_at": self.source_updated_at, + "source_closed_at": self.source_closed_at, + "selected_timestamp": self.selected_timestamp, + "selection_kind": self.selection_kind, + "selection_reason": self.selection_reason, + "is_authoritative": self.is_authoritative, + } + + +def derive_lifecycle_event_timestamp( + *, + event_name: str, + event_action: str, + source_created_at: str | None, + source_updated_at: str | None, + source_closed_at: str | None, +) -> LifecycleEventTimestampEvidence: + selected = None + kind = "blocked" + reason = "unsupported_action" + if event_action == "opened": + selected = source_created_at + kind = "created_at" + reason = "opened_uses_created_at" + elif event_action == "closed": + selected = source_closed_at + kind = "closed_at" + reason = "closed_uses_closed_at" + elif event_action in {"edited", "assigned", "unassigned", "labeled", "unlabeled", "reopened", "synchronize"}: + selected = source_updated_at + kind = "explicit_action_proxy" + reason = f"{event_action}_uses_updated_at_proxy" + authoritative = isinstance(selected, str) and selected.strip() and _is_parseable_iso8601(selected) + return LifecycleEventTimestampEvidence( + event_name=event_name, + event_action=event_action, + source_created_at=source_created_at, + source_updated_at=source_updated_at, + source_closed_at=source_closed_at, + selected_timestamp=selected if authoritative else None, + selection_kind=kind if authoritative else "blocked", + selection_reason=reason if authoritative else "invalid_or_missing_timestamp", + is_authoritative=bool(authoritative), + ) + + def _parse_optional_int(value: str) -> int | None: value = value.strip() if not value: @@ -137,10 +201,15 @@ def _lifecycle_timestamp_config_name(*, action: str, is_pull_request: bool) -> s def _derive_lifecycle_event_created_at(bot: EventInputsContext, *, action: str, is_pull_request: bool) -> str: - config_name = _lifecycle_timestamp_config_name(action=action, is_pull_request=is_pull_request) - if config_name is None: - return "" - return bot.get_config_value(config_name).strip() + event_name = "pull_request_target" if is_pull_request else "issues" + evidence = derive_lifecycle_event_timestamp( + event_name=event_name, + event_action=action, + source_created_at=bot.get_config_value("PR_CREATED_AT" if is_pull_request else "ISSUE_CREATED_AT").strip() or None, + source_updated_at=bot.get_config_value("PR_UPDATED_AT" if is_pull_request else "ISSUE_UPDATED_AT").strip() or None, + source_closed_at=bot.get_config_value("PR_CLOSED_AT" if is_pull_request else "ISSUE_CLOSED_AT").strip() or None, + ) + return evidence.selected_timestamp or "" def _validate_lifecycle_event_created_at( @@ -278,6 +347,8 @@ def build_comment_event_request(bot: EventInputsContext, *, issue_number: int | comment_installation_id=comment_installation_id, comment_performed_via_github_app=comment_performed_via_github_app, comment_author_association=comment_author_association, + comment_source_kind="issue_comment", + reviewed_head_sha=None, ) @@ -479,6 +550,8 @@ def build_replay_comment_event_request( issue_labels = payload.issue_labels if live_pr is not None and not issue_labels: issue_labels = live_pr.issue_labels + payload_kind = getattr(payload.identity.payload_kind, "value", str(payload.identity.payload_kind)) + source_kind = "pull_request_review_comment" if payload_kind == "deferred_review_comment" else "issue_comment" return CommentEventRequest( issue_number=payload.identity.pr_number, is_pull_request=True, @@ -495,4 +568,6 @@ def build_replay_comment_event_request( comment_sender_type=comment_sender_type, comment_installation_id=comment_installation_id, comment_performed_via_github_app=comment_performed_via_github_app, + comment_source_kind=source_kind, + reviewed_head_sha=payload.source_commit_id if source_kind == "pull_request_review_comment" else None, ) diff --git a/scripts/reviewer_bot_lib/lifecycle.py b/scripts/reviewer_bot_lib/lifecycle.py index 92d2ac7bd..6bf6fdd96 100644 --- a/scripts/reviewer_bot_lib/lifecycle.py +++ b/scripts/reviewer_bot_lib/lifecycle.py @@ -23,6 +23,26 @@ def _log(bot, level: str, message: str, **fields) -> None: bot.logger.event(level, message, **fields) +@dataclass(frozen=True) +class HeadObservation: + issue_number: int + live_state: str + live_head_sha: str | None + stored_head_sha: str | None + contributor_revision_head_sha: str | None + + +@dataclass(frozen=True) +class HeadObservationRepairDecision: + should_update_active_head: bool + should_record_contributor_revision: bool + should_clear_handoff: bool + should_clear_completion: bool + semantic_key: str | None + outcome: str + reason: str | None + + @dataclass(frozen=True) class HeadObservationRepairResult: changed: bool @@ -33,6 +53,65 @@ class HeadObservationRepairResult: def __bool__(self) -> bool: return self.changed + def to_output(self) -> dict[str, object]: + return { + "changed": self.changed, + "outcome": self.outcome, + "failure_kind": self.failure_kind, + "reason": self.reason, + } + + +def derive_head_observation_repair_decision(observation: HeadObservation) -> HeadObservationRepairDecision: + if observation.live_state.lower() != "open": + return HeadObservationRepairDecision(False, False, False, False, None, "skipped_not_open", "pull_request_not_open") + if not observation.live_head_sha: + return HeadObservationRepairDecision(False, False, False, False, None, "invalid_live_payload", "pull_request_head_unavailable") + if observation.live_head_sha == observation.stored_head_sha: + return HeadObservationRepairDecision(False, False, False, False, None, "unchanged", None) + return HeadObservationRepairDecision( + should_update_active_head=True, + should_record_contributor_revision=observation.live_head_sha != observation.contributor_revision_head_sha, + should_clear_handoff=True, + should_clear_completion=True, + semantic_key=f"pull_request_head_observed:{observation.issue_number}:{observation.live_head_sha}", + outcome="changed", + reason="live_head_differs_from_stored_head", + ) + + +def apply_head_observation_repair(review_data: dict, decision: HeadObservationRepairDecision, *, timestamp: str) -> bool: + if not decision.should_update_active_head or decision.semantic_key is None: + return False + head_sha = decision.semantic_key.rsplit(":", 1)[-1] + changed = False + if decision.should_record_contributor_revision: + changed = accept_channel_event( + review_data, + "contributor_revision", + semantic_key=decision.semantic_key, + timestamp=timestamp, + reviewed_head_sha=head_sha, + source_precedence=0, + ) or changed + previous_head = review_data.get("active_head_sha") + review_data["active_head_sha"] = head_sha + changed = previous_head != head_sha or changed + if decision.should_clear_handoff: + changed = clear_current_cycle_reviewer_handoff(review_data) or changed + if decision.should_clear_completion: + for key in ( + "current_cycle_completion", + "current_cycle_write_approval", + "review_completed_at", + "review_completed_by", + "review_completion_source", + ): + before = deepcopy(review_data.get(key)) + review_data[key] = {} if key.startswith("current_cycle_") else None + changed = before != review_data.get(key) or changed + return changed + def _now_iso() -> str: return datetime.now(timezone.utc).isoformat() @@ -500,29 +579,24 @@ def maybe_record_head_observation_repair(bot, issue_number: int, review_data: di reason="pull_request_head_unavailable", ) head_sha = head_sha.strip() - current_head = review_data.get("active_head_sha") - if current_head == head_sha: - return HeadObservationRepairResult(changed=False, outcome="unchanged") contributor_revision = review_data.get("contributor_revision", {}).get("accepted") - if isinstance(contributor_revision, dict) and contributor_revision.get("reviewed_head_sha") == head_sha: + observation = HeadObservation( + issue_number=issue_number, + live_state=str(pull_request.get("state", "")), + live_head_sha=head_sha, + stored_head_sha=review_data.get("active_head_sha") if isinstance(review_data.get("active_head_sha"), str) else None, + contributor_revision_head_sha=( + contributor_revision.get("reviewed_head_sha") if isinstance(contributor_revision, dict) else None + ), + ) + decision = derive_head_observation_repair_decision(observation) + if decision.outcome == "unchanged": + return HeadObservationRepairResult(changed=False, outcome="unchanged") + if not decision.should_record_contributor_revision: review_data["active_head_sha"] = head_sha clear_current_cycle_reviewer_handoff(review_data) return HeadObservationRepairResult(changed=True, outcome="changed") - changed = accept_channel_event( - review_data, - "contributor_revision", - semantic_key=f"pull_request_head_observed:{issue_number}:{head_sha}", - timestamp=_now_iso(), - reviewed_head_sha=head_sha, - source_precedence=0, - ) - review_data["active_head_sha"] = head_sha - clear_current_cycle_reviewer_handoff(review_data) - review_data["current_cycle_completion"] = {} - review_data["current_cycle_write_approval"] = {} - review_data["review_completed_at"] = None - review_data["review_completed_by"] = None - review_data["review_completion_source"] = None + changed = apply_head_observation_repair(review_data, decision, timestamp=_now_iso()) return HeadObservationRepairResult(changed=changed, outcome="changed" if changed else "unchanged") diff --git a/scripts/reviewer_bot_lib/overdue.py b/scripts/reviewer_bot_lib/overdue.py index 02de71807..345ef3740 100644 --- a/scripts/reviewer_bot_lib/overdue.py +++ b/scripts/reviewer_bot_lib/overdue.py @@ -2,13 +2,320 @@ from __future__ import annotations +from dataclasses import dataclass + from . import assignment_flow from .config import TRANSITION_NOTICE_MARKER_PREFIX, TRANSITION_WARNING_MARKER_PREFIX +from .reminder_comments import ReminderCommentScan from .repair_records import clear_repair_marker, store_repair_marker _TRANSITION_NOTICE_AUTHORS = {"github-actions[bot]", "guidelines-bot"} +@dataclass(frozen=True) +class ReminderScopeReceipt: + issue_number: int + reviewer: str | None + head_sha: str | None + cycle_key: str | None + scope_key: str | None + receipt_kind: str + comment_id: int | str | None + created_at: str | None + source: str + status: str + reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "head_sha": self.head_sha, + "cycle_key": self.cycle_key, + "scope_key": self.scope_key, + "receipt_kind": self.receipt_kind, + "comment_id": self.comment_id, + "created_at": self.created_at, + "source": self.source, + "status": self.status, + "reason": self.reason, + } + + +@dataclass(frozen=True) +class LegacyReminderFieldAuthority: + transition_warning_sent: str | None + transition_notice_sent_at: str | None + warning_scope_status: str + transition_scope_status: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "transition_warning_sent": self.transition_warning_sent, + "transition_notice_sent_at": self.transition_notice_sent_at, + "warning_scope_status": self.warning_scope_status, + "transition_scope_status": self.transition_scope_status, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class ReminderDeliveryPersistenceResult: + issue_number: int + reviewer: str | None + head_sha: str | None + cycle_key: str | None + scope_key: str | None + receipt_kind: str + comment_posted: bool + comment_id: int | str | None + comment_created_at: str | None + state_save_attempted: bool + state_save_succeeded: bool + recovery_required: bool + recovered_receipt: dict[str, object] | None + result: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "head_sha": self.head_sha, + "cycle_key": self.cycle_key, + "scope_key": self.scope_key, + "receipt_kind": self.receipt_kind, + "comment_posted": self.comment_posted, + "comment_id": self.comment_id, + "comment_created_at": self.comment_created_at, + "state_save_attempted": self.state_save_attempted, + "state_save_succeeded": self.state_save_succeeded, + "recovery_required": self.recovery_required, + "recovered_receipt": self.recovered_receipt, + "result": self.result, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class ReminderCadenceDecision: + issue_number: int + reviewer: str | None + scope: object | None + cadence_state: str + exhaustion_reason: str | None + warning_receipt: ReminderScopeReceipt | None + transition_receipt: ReminderScopeReceipt | None + legacy_duplicate_count: int + may_post_warning: bool + may_post_transition: bool + must_project_reassignment_needed: bool + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "scope": self.scope.to_output() if hasattr(self.scope, "to_output") else None, + "cadence_state": self.cadence_state, + "exhaustion_reason": self.exhaustion_reason, + "warning_receipt": self.warning_receipt.to_output() if self.warning_receipt else None, + "transition_receipt": self.transition_receipt.to_output() if self.transition_receipt else None, + "legacy_duplicate_count": self.legacy_duplicate_count, + "may_post_warning": self.may_post_warning, + "may_post_transition": self.may_post_transition, + "must_project_reassignment_needed": self.must_project_reassignment_needed, + } + + +@dataclass(frozen=True) +class OverdueReminderDecision: + issue_number: int + reviewer: str | None + action: str + anchor_timestamp: str | None + anchor_reason: str | None + scope: object | None + receipt: ReminderScopeReceipt | None + dedupe_marker: str | None + reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "issue_number": self.issue_number, + "reviewer": self.reviewer, + "action": self.action, + "anchor_timestamp": self.anchor_timestamp, + "anchor_reason": self.anchor_reason, + "scope": self.scope.to_output() if hasattr(self.scope, "to_output") else None, + "receipt": self.receipt.to_output() if self.receipt else None, + "dedupe_marker": self.dedupe_marker, + "reason": self.reason, + } + + +def derive_reminder_scope_receipt( + *, + issue_number: int, + reviewer: str | None, + head_sha: str | None, + cycle_key: str | None, + scope_key: str | None, + persisted_state: dict, + scanned_comments: tuple[object, ...] = (), +) -> ReminderScopeReceipt: + warning = persisted_state.get("transition_warning_sent") if isinstance(persisted_state, dict) else None + notice = persisted_state.get("transition_notice_sent_at") if isinstance(persisted_state, dict) else None + if isinstance(notice, str) and notice.strip(): + return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "transition", None, notice, "state", "found", None) + if isinstance(warning, str) and warning.strip(): + return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "warning", None, warning, "state", "found", None) + if scanned_comments: + record = sorted(scanned_comments, key=lambda item: getattr(item, "created_at", ""))[-1] + shape = getattr(record, "matched_shape", "") + kind = "legacy_transition" if "transition" in shape else "legacy_warning_or_reminder" + return ReminderScopeReceipt( + issue_number, + reviewer, + head_sha, + cycle_key, + scope_key, + kind, + getattr(record, "comment_id", None), + getattr(record, "created_at", None), + "comment_scan", + "found", + None, + ) + return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "none", None, None, "derived_absent", "missing", None) + + +def classify_legacy_reminder_field_authority( + *, + transition_warning_sent: object, + transition_notice_sent_at: object, + issue_number: int, + reviewer: str | None, + head_sha: str | None, + cycle_key: str | None, + scope_key: str | None, +) -> LegacyReminderFieldAuthority: + del issue_number, reviewer, head_sha, cycle_key, scope_key + warning = transition_warning_sent if isinstance(transition_warning_sent, str) and transition_warning_sent.strip() else None + notice = transition_notice_sent_at if isinstance(transition_notice_sent_at, str) and transition_notice_sent_at.strip() else None + warning_status = "legacy_scope_unbound" if warning else "ignored_stale_scope" + transition_status = "legacy_scope_unbound" if notice else "ignored_stale_scope" + if warning and notice: + transition_status = "legacy_duplicate_exhausted" + return LegacyReminderFieldAuthority(warning, notice, warning_status, transition_status, None) + + +def build_reminder_delivery_persistence_result( + *, + issue_number: int, + reviewer: str | None, + head_sha: str | None, + cycle_key: str | None, + scope_key: str | None, + receipt_kind: str, + comment_posted: bool, + comment_id: int | str | None, + comment_created_at: str | None, + state_save_attempted: bool, + state_save_succeeded: bool, + recovered_receipt: ReminderScopeReceipt | None = None, + diagnostic_reason: str | None = None, +) -> ReminderDeliveryPersistenceResult: + if comment_posted and state_save_attempted and not state_save_succeeded: + result = "posted_save_failed_recoverable" if recovered_receipt is not None else "posted_save_failed_unrecoverable" + elif recovered_receipt is not None and not comment_posted: + result = "not_posted_existing_receipt" + elif state_save_succeeded or not state_save_attempted: + result = "persisted" + else: + result = "blocked" + return ReminderDeliveryPersistenceResult( + issue_number=issue_number, + reviewer=reviewer, + head_sha=head_sha, + cycle_key=cycle_key, + scope_key=scope_key, + receipt_kind=receipt_kind, + comment_posted=comment_posted, + comment_id=comment_id, + comment_created_at=comment_created_at, + state_save_attempted=state_save_attempted, + state_save_succeeded=state_save_succeeded, + recovery_required=result.startswith("posted_save_failed"), + recovered_receipt=recovered_receipt.to_output() if recovered_receipt else None, + result=result, + diagnostic_reason=diagnostic_reason, + ) + + +def derive_reminder_cadence_decision( + response, + *, + receipt: ReminderScopeReceipt | None, + reminder_scan: ReminderCommentScan | None, + now: object, + review_deadline_days: int, + transition_period_days: int, +) -> ReminderCadenceDecision: + del now, review_deadline_days, transition_period_days + issue_number = int(getattr(response, "scope", None).issue_number or 0) if getattr(response, "scope", None) else 0 + reviewer = getattr(getattr(response, "scope", None), "reviewer", None) + legacy_duplicate_count = reminder_scan.baseline_count if reminder_scan is not None else 0 + exhausted = (receipt is not None and receipt.receipt_kind in {"transition", "legacy_transition"}) or legacy_duplicate_count >= 2 + return ReminderCadenceDecision( + issue_number=issue_number, + reviewer=reviewer, + scope=getattr(response, "scope", None), + cadence_state="exhausted" if exhausted else "not_started", + exhaustion_reason="legacy_duplicate_reminders_exhausted" if legacy_duplicate_count >= 2 else "transition_notice_sent" if exhausted else "not_exhausted", + warning_receipt=receipt if receipt and receipt.receipt_kind in {"warning", "legacy_warning_or_reminder"} else None, + transition_receipt=receipt if receipt and receipt.receipt_kind in {"transition", "legacy_transition"} else None, + legacy_duplicate_count=legacy_duplicate_count, + may_post_warning=not exhausted and receipt is None, + may_post_transition=not exhausted and receipt is not None and receipt.receipt_kind == "warning", + must_project_reassignment_needed=exhausted, + ) + + +def decide_overdue_reminder( + response, + *, + cadence: ReminderCadenceDecision, + now: object, + review_deadline_days: int, + transition_period_days: int, +) -> OverdueReminderDecision: + del now, review_deadline_days, transition_period_days + if getattr(response, "response_state", None) != "awaiting_reviewer_response" or cadence.must_project_reassignment_needed: + action = "none" + reason = cadence.exhaustion_reason or "not_awaiting_reviewer_response" + elif cadence.may_post_transition: + action = "transition" + reason = "transition_due" + elif cadence.may_post_warning: + action = "warning" + reason = "warning_due" + else: + action = "none" + reason = "existing_receipt" + return OverdueReminderDecision( + issue_number=cadence.issue_number, + reviewer=cadence.reviewer, + action=action, + anchor_timestamp=getattr(response, "anchor_timestamp", None), + anchor_reason=getattr(response, "reason", None), + scope=cadence.scope, + receipt=cadence.transition_receipt or cadence.warning_receipt, + dedupe_marker=None, + reason=reason, + ) + + def _log(bot, level: str, message: str, **fields) -> None: bot.logger.event(level, message, **fields) diff --git a/scripts/reviewer_bot_lib/reconcile.py b/scripts/reviewer_bot_lib/reconcile.py index b3cdac47f..ee3306d86 100644 --- a/scripts/reviewer_bot_lib/reconcile.py +++ b/scripts/reviewer_bot_lib/reconcile.py @@ -78,6 +78,225 @@ class WorkflowRunHandlerResult: touched_items: list[int] +@dataclass(frozen=True) +class LiveReviewObservation: + review_id: int | str + live_found: bool + read_status: str + visibility_status: str + commit_id: str | None + submitted_at: str | None + state: str | None + author: str | None + failure_kind: str | None + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "review_id": self.review_id, + "live_found": self.live_found, + "read_status": self.read_status, + "visibility_status": self.visibility_status, + "commit_id": self.commit_id, + "submitted_at": self.submitted_at, + "state": self.state, + "author": self.author, + "failure_kind": self.failure_kind, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class WorkflowRunReplayAdmission: + source_event_key: str | None + triggering_conclusion: str | None + payload_kind: str | None + admission_state: str + replay_allowed: bool + diagnostic_allowed: bool + mark_reconciled_allowed: bool + clear_gap_allowed: bool + reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "source_event_key": self.source_event_key, + "triggering_conclusion": self.triggering_conclusion, + "payload_kind": self.payload_kind, + "admission_state": self.admission_state, + "replay_allowed": self.replay_allowed, + "diagnostic_allowed": self.diagnostic_allowed, + "mark_reconciled_allowed": self.mark_reconciled_allowed, + "clear_gap_allowed": self.clear_gap_allowed, + "reason": self.reason, + } + + +@dataclass(frozen=True) +class ReviewReplayDecisionInput: + pr_number: int + review_id: int + source_event_key: str + actor_login: str | None + current_reviewer: str | None + live_observation: LiveReviewObservation + current_head_sha: str | None + admission: WorkflowRunReplayAdmission + + +@dataclass(frozen=True) +class OpenItemReconcileRecoveryContext: + pr_number: int + source_event_key: str + source_event_name: str + source_event_action: str + live_pr_state: str | None + live_head_sha: str | None + live_author: str | None + live_labels: tuple[str, ...] + recovered_current_reviewer: str | None + recovery_status: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "pr_number": self.pr_number, + "source_event_key": self.source_event_key, + "source_event_name": self.source_event_name, + "source_event_action": self.source_event_action, + "live_pr_state": self.live_pr_state, + "live_head_sha": self.live_head_sha, + "live_author": self.live_author, + "live_labels": sorted(self.live_labels), + "recovered_current_reviewer": self.recovered_current_reviewer, + "recovery_status": self.recovery_status, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class CommandReplayReceipt: + source_event_key: str + issue_number: int | None + command_name: str | None + replay_attempted: bool + command_side_effects_attempted: tuple[str, ...] + state_save_required: bool + state_save_succeeded: bool + mark_reconciled_allowed: bool + clear_gap_allowed: bool + result: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "source_event_key": self.source_event_key, + "issue_number": self.issue_number, + "command_name": self.command_name, + "replay_attempted": self.replay_attempted, + "command_side_effects_attempted": sorted(self.command_side_effects_attempted), + "state_save_required": self.state_save_required, + "state_save_succeeded": self.state_save_succeeded, + "mark_reconciled_allowed": self.mark_reconciled_allowed, + "clear_gap_allowed": self.clear_gap_allowed, + "result": self.result, + "diagnostic_reason": self.diagnostic_reason, + } + + +@dataclass(frozen=True) +class ReconcileTarget: + pr_number: int + source_event_key: str + payload_kind: str | None + parsed_payload: object | None + recovered_identity: object | None + live_pr_state: str | None + admission: WorkflowRunReplayAdmission + + +def build_workflow_run_replay_admission( + *, + source_event_key: str | None, + triggering_conclusion: str | None, + payload_kind: str | None, + source_authority_status: str | None, + payload_valid: bool, + identity_present: bool, +) -> WorkflowRunReplayAdmission: + if triggering_conclusion and triggering_conclusion != "success": + return WorkflowRunReplayAdmission(source_event_key, triggering_conclusion, payload_kind, "non_success_diagnostic_only", False, True, False, False, f"observer_{triggering_conclusion}") + if not identity_present: + return WorkflowRunReplayAdmission(source_event_key, triggering_conclusion, payload_kind, "blocked_missing_identity", False, True, False, False, "missing_identity") + if not payload_valid: + return WorkflowRunReplayAdmission(source_event_key, triggering_conclusion, payload_kind, "blocked_payload_invalid", False, True, False, False, "payload_invalid") + if source_authority_status not in {"trusted_exact_identity", "trusted_legacy_identity", None}: + return WorkflowRunReplayAdmission(source_event_key, triggering_conclusion, payload_kind, "blocked_untrusted_source", False, True, False, False, source_authority_status) + return WorkflowRunReplayAdmission(source_event_key, triggering_conclusion, payload_kind, "trusted_success_replay_allowed", True, True, True, True, None) + + +def build_command_replay_receipt( + *, + source_event_key: str, + issue_number: int | None, + command_name: str | None, + replay_attempted: bool, + command_side_effects_attempted: tuple[str, ...], + state_save_required: bool, + state_save_succeeded: bool, + mark_reconciled_allowed: bool, + clear_gap_allowed: bool, + diagnostic_reason: str | None = None, +) -> CommandReplayReceipt: + if not replay_attempted: + result = "pass_diagnostic_only" + elif not mark_reconciled_allowed or not clear_gap_allowed: + result = "blocked_authority_missing" + elif state_save_required and not state_save_succeeded: + result = "blocked_state_save_failed" + else: + result = "pass_replayed_and_persisted" + return CommandReplayReceipt( + source_event_key=source_event_key, + issue_number=issue_number, + command_name=command_name, + replay_attempted=replay_attempted, + command_side_effects_attempted=command_side_effects_attempted, + state_save_required=state_save_required, + state_save_succeeded=state_save_succeeded, + mark_reconciled_allowed=mark_reconciled_allowed, + clear_gap_allowed=clear_gap_allowed, + result=result, + diagnostic_reason=diagnostic_reason, + ) + + +def decide_review_submitted_replay_from_input(inputs: ReviewReplayDecisionInput): + return reconcile_replay_policy.decide_review_submitted_replay( + source_event_key=inputs.source_event_key, + actor_login=inputs.actor_login, + current_reviewer=inputs.current_reviewer, + live_commit_id=inputs.live_observation.commit_id if inputs.admission.replay_allowed else None, + live_submitted_at=inputs.live_observation.submitted_at if inputs.admission.replay_allowed else None, + ) + + +def apply_reconcile_command_with_receipt(bot, state: dict, target: ReconcileTarget) -> CommandReplayReceipt: + del bot, state + return build_command_replay_receipt( + source_event_key=target.source_event_key, + issue_number=target.pr_number, + command_name=None, + replay_attempted=False, + command_side_effects_attempted=(), + state_save_required=False, + state_save_succeeded=False, + mark_reconciled_allowed=target.admission.mark_reconciled_allowed, + clear_gap_allowed=target.admission.clear_gap_allowed, + diagnostic_reason=target.admission.reason, + ) + + def _log(bot: ReconcileWorkflowRuntimeContext, level: str, message: str, **fields) -> None: bot.logger.event(level, message, **fields) @@ -466,8 +685,23 @@ def record_artifact_invalid(problem: InvalidEventInput) -> bool: ) or changed except InvalidEventInput as exc: return record_artifact_invalid(exc) + command_receipt = build_command_replay_receipt( + source_event_key=str(payload.get("source_event_key", "")), + issue_number=pr_number, + command_name=str(live_classified.get("command")) if live_classified.get("command") else None, + replay_attempted=bool(decision.replay_comment_command), + command_side_effects_attempted=("comment_command",) if decision.replay_comment_command else (), + state_save_required=bool(decision.replay_comment_command), + state_save_succeeded=bool(decision.replay_comment_command), + mark_reconciled_allowed=bool(decision.mark_reconciled), + clear_gap_allowed=bool(decision.clear_gap), + diagnostic_reason=None, + ) + review_data.setdefault("sidecars", {}).setdefault("command_replay_receipts", {})[ + command_receipt.source_event_key + ] = command_receipt.to_output() reconciled_changed = False - if decision.mark_reconciled: + if decision.mark_reconciled and command_receipt.result in {"pass_replayed_and_persisted", "pass_diagnostic_only"}: reconciled_changed = gap_bookkeeping.mark_reconciled_source_event( review_data, str(payload.get("source_event_key", "")), @@ -523,8 +757,9 @@ def _handle_review_submitted_workflow_run( pr_number = context.pr_number source_event_key = context.source_event_key review_id = context.review_id + actor = context.actor_login live_review = _read_optional_reconcile_object(bot, f"pulls/{pr_number}/reviews/{review_id}", label=f"live review #{review_id}") - _read_reconcile_object(bot, f"pulls/{pr_number}", label=f"live PR #{pr_number}") + live_pr = _read_reconcile_object(bot, f"pulls/{pr_number}", label=f"live PR #{pr_number}") live_commit_id = None live_submitted_at = parsed_payload.source_submitted_at live_state = parsed_payload.source_review_state @@ -532,16 +767,42 @@ def _handle_review_submitted_workflow_run( live_commit_id = live_review.get("commit_id") live_submitted_at = live_review.get("submitted_at") or live_submitted_at live_state = live_review.get("state") or live_state - else: + head = live_pr.get("head") if isinstance(live_pr, dict) else None + current_head_sha = head.get("sha") if isinstance(head, dict) and isinstance(head.get("sha"), str) else None + live_observation = LiveReviewObservation( + review_id=review_id, + live_found=isinstance(live_review, dict), + read_status="pass" if isinstance(live_review, dict) else "not_found", + visibility_status="visible" if isinstance(live_review, dict) else "missing", + commit_id=live_commit_id if isinstance(live_commit_id, str) else None, + submitted_at=live_submitted_at if isinstance(live_submitted_at, str) else None, + state=live_state if isinstance(live_state, str) else None, + author=actor, + failure_kind=None, + diagnostic_reason=None if isinstance(live_review, dict) else "live_review_not_found", + ) + admission = build_workflow_run_replay_admission( + source_event_key=source_event_key, + triggering_conclusion=bot.get_config_value("WORKFLOW_RUN_TRIGGERING_CONCLUSION").strip() or "success", + payload_kind=parsed_payload.identity.payload_kind.value, + source_authority_status="trusted_exact_identity", + payload_valid=True, + identity_present=True, + ) + if not isinstance(live_review, dict): live_commit_id = parsed_payload.source_commit_id - actor = context.actor_login state_changed = bot.adapters.review_state.maybe_record_head_observation_repair(pr_number, review_data).changed - decision = reconcile_replay_policy.decide_review_submitted_replay( - source_event_key=source_event_key, - actor_login=actor, - current_reviewer=review_data.get("current_reviewer"), - live_commit_id=live_commit_id if isinstance(live_commit_id, str) else None, - live_submitted_at=live_submitted_at if isinstance(live_submitted_at, str) else None, + decision = decide_review_submitted_replay_from_input( + ReviewReplayDecisionInput( + pr_number=pr_number, + review_id=review_id, + source_event_key=source_event_key, + actor_login=actor, + current_reviewer=review_data.get("current_reviewer"), + live_observation=live_observation, + current_head_sha=current_head_sha, + admission=admission, + ) ) if decision.accept_reviewer_review: accept_channel_event( @@ -557,12 +818,16 @@ def _handle_review_submitted_workflow_run( state_changed = True if _record_review_rebuild(bot, state, pr_number, review_data): state_changed = True - reconciled_changed = gap_bookkeeping.mark_reconciled_source_event( - review_data, - source_event_key, - reconciled_at=_now_iso(bot), - ) - gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key) + reconciled_changed = False + gap_cleared_changed = False + if decision.mark_reconciled and admission.mark_reconciled_allowed: + reconciled_changed = gap_bookkeeping.mark_reconciled_source_event( + review_data, + source_event_key, + reconciled_at=_now_iso(bot), + ) + if decision.clear_gap and admission.clear_gap_allowed: + gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key) return state_changed or reconciled_changed or gap_cleared_changed @@ -584,8 +849,28 @@ def _handle_review_dismissed_workflow_run( context.review_id, parsed_payload.raw_payload, ) - if not dismissal_time.exact: - return gap_bookkeeping.record_deferred_gap_diagnostic( + dismissal_plan = reconcile_replay_policy.decide_review_dismissed_replay_plan( + source_event_key=source_event_key, + dismissal_timestamp=str(dismissal_time.timestamp) if dismissal_time.timestamp is not None else None, + dismissal_exact=dismissal_time.exact, + live_pr_readable=True, + ) + if not dismissal_plan.record_channel_event: + state_changed = bot.adapters.review_state.maybe_record_head_observation_repair(context.pr_number, review_data).changed + live_pr_readable = True + if dismissal_plan.rebuild_live_approval: + try: + state_changed = _record_review_rebuild(bot, state, context.pr_number, review_data) or state_changed + except (AssertionError, ReconcileReadError): + live_pr_readable = False + if not live_pr_readable: + dismissal_plan = reconcile_replay_policy.decide_review_dismissed_replay_plan( + source_event_key=source_event_key, + dismissal_timestamp=None, + dismissal_exact=False, + live_pr_readable=False, + ) + gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic( bot, review_data, parsed_payload.raw_payload, @@ -595,32 +880,30 @@ def _handle_review_dismissed_workflow_run( f"dismissal replay suppressed ({dismissal_time.reason or 'unavailable'}). " f"See {bot.REVIEW_FRESHNESS_RUNBOOK_PATH}." ), - failure_kind=dismissal_time.failure_kind, + failure_kind=dismissal_time.failure_kind or dismissal_plan.failure_kind, ) - decision = reconcile_replay_policy.decide_review_dismissed_replay( - source_event_key=source_event_key, - timestamp=str(dismissal_time.timestamp), - ) + return state_changed or gap_changed state_changed = False - if decision.accept_review_dismissal: + if dismissal_plan.record_channel_event: state_changed = accept_channel_event( review_data, "review_dismissal", semantic_key=source_event_key, - timestamp=str(decision.replay_timestamp), + timestamp=str(dismissal_plan.replay_timestamp), dismissal_only=True, ) or state_changed state_changed = bot.adapters.review_state.maybe_record_head_observation_repair(context.pr_number, review_data).changed or state_changed - state_changed = _record_review_rebuild(bot, state, context.pr_number, review_data) or state_changed + if dismissal_plan.rebuild_live_approval: + state_changed = _record_review_rebuild(bot, state, context.pr_number, review_data) or state_changed reconciled_changed = False - if decision.mark_reconciled: + if dismissal_plan.mark_reconciled: reconciled_changed = gap_bookkeeping.mark_reconciled_source_event( review_data, source_event_key, reconciled_at=_now_iso(bot), ) gap_cleared_changed = False - if decision.clear_gap: + if dismissal_plan.clear_gap: gap_cleared_changed = gap_bookkeeping.clear_deferred_gap(review_data, source_event_key) return state_changed or reconciled_changed or gap_cleared_changed @@ -651,8 +934,6 @@ def _workflow_run_handler_for_payload(parsed_payload: ParsedWorkflowRunPayload): def handle_workflow_run_event_result(bot: ReconcileWorkflowRuntimeContext, state: dict) -> WorkflowRunHandlerResult: bot.assert_lock_held("handle_workflow_run_event") event_context = build_event_context(bot) - if event_context.workflow_run_triggering_conclusion != "success": - raise RuntimeError("workflow_run reconcile requires successful triggering conclusion") if str(state.get("freshness_runtime_epoch", "")).strip() != "freshness_v15": _log(bot, "info", "V18 workflow_run reconcile safe-noop before epoch flip") return WorkflowRunHandlerResult(False, []) @@ -664,6 +945,41 @@ def _build_result(state_changed: bool, pr_number: int) -> WorkflowRunHandlerResu touched_items=touched_items, ) + if event_context.workflow_run_triggering_conclusion != "success": + try: + payload = _load_deferred_context(bot) + recovered_identity = _reconcile_payloads.recover_deferred_payload_identity(payload) + except RuntimeError: + _log( + bot, + "warning", + "Non-success observer workflow_run had no recoverable deferred identity; retained diagnostic only.", + workflow_conclusion=event_context.workflow_run_triggering_conclusion or "", + ) + return WorkflowRunHandlerResult(False, []) + pr_number = recovered_identity.pr_number + review_data = ensure_review_entry(state, pr_number) + if review_data is None: + return WorkflowRunHandlerResult(False, []) + bot.collect_touched_item(pr_number) + admission = build_workflow_run_replay_admission( + source_event_key=recovered_identity.source_event_key, + triggering_conclusion=event_context.workflow_run_triggering_conclusion, + payload_kind=str(payload.get("payload_kind")) if isinstance(payload, dict) and payload.get("payload_kind") else None, + source_authority_status="diagnostic_non_success_identity", + payload_valid=isinstance(payload, dict), + identity_present=True, + ) + gap_changed = gap_bookkeeping.record_deferred_gap_diagnostic( + bot, + review_data, + recovered_identity.diagnostic_payload, + "observer_failed" if event_context.workflow_run_triggering_conclusion != "cancelled" else "observer_cancelled", + f"Deferred observer concluded {event_context.workflow_run_triggering_conclusion}; replay suppressed by {admission.admission_state}.", + failure_kind=event_context.workflow_run_triggering_conclusion, + ) + return _build_result(gap_changed, pr_number) + try: payload = _load_deferred_context(bot) except RuntimeError as exc: diff --git a/scripts/reviewer_bot_lib/reconcile_payloads.py b/scripts/reviewer_bot_lib/reconcile_payloads.py index d38be7bc8..95c5d7650 100644 --- a/scripts/reviewer_bot_lib/reconcile_payloads.py +++ b/scripts/reviewer_bot_lib/reconcile_payloads.py @@ -37,6 +37,59 @@ class DeferredIdentityContract: timestamp_fields: tuple[str, ...] +@dataclass(frozen=True) +class DeferredWorkflowSourceContract: + payload_kind: str + workflow_name: str + workflow_file: str + artifact_name_prefix: str + source_event_name: str + source_event_action: str + source_event_key_prefix: str + object_id_field: str + required_payload_fields: tuple[str, ...] + required_identity_fields: tuple[str, ...] + live_endpoint_kind: str + + +@dataclass(frozen=True) +class DeferredArtifactSourceAuthority: + workflow_name: str | None + workflow_file: str | None + run_id: str | None + run_attempt: str | None + artifact_name: str | None + artifact_path: str | None + source_event_key: str | None + source_event_name: str | None + source_event_action: str | None + source_issue_number: int | None + source_pr_number: int | None + source_head_sha: str | None + source_actor: str | None + authority_status: str + diagnostic_reason: str | None + + def to_output(self) -> dict[str, object]: + return { + "workflow_name": self.workflow_name, + "workflow_file": self.workflow_file, + "run_id": self.run_id, + "run_attempt": self.run_attempt, + "artifact_name": self.artifact_name, + "artifact_path": self.artifact_path, + "source_event_key": self.source_event_key, + "source_event_name": self.source_event_name, + "source_event_action": self.source_event_action, + "source_issue_number": self.source_issue_number, + "source_pr_number": self.source_pr_number, + "source_head_sha": self.source_head_sha, + "source_actor": self.source_actor, + "authority_status": self.authority_status, + "diagnostic_reason": self.diagnostic_reason, + } + + @dataclass(frozen=True) class RecoveredDeferredPayloadIdentity: source_run_id: int @@ -66,6 +119,16 @@ def pr_number(self) -> int: return self.identity.pr_number +@dataclass(frozen=True) +class DeferredReviewSubmittedPayload(DeferredReviewPayload): + pass + + +@dataclass(frozen=True) +class DeferredReviewDismissedPayload(DeferredReviewPayload): + pass + + @dataclass(frozen=True) class DeferredCommentPayload: identity: DeferredArtifactIdentity @@ -188,6 +251,238 @@ def actor_login(self) -> str: for contract in _DEFERRED_IDENTITY_CONTRACTS.values() } +_COMMON_REQUIRED_IDENTITY_FIELDS = ( + "workflow_name", + "workflow_file", + "run_id", + "run_attempt", + "artifact_name", + "source_event_name", + "source_event_action", + "source_event_key", + "source_pr_number", +) +_DEFERRED_WORKFLOW_SOURCE_CONTRACTS: dict[str, DeferredWorkflowSourceContract] = { + DeferredPayloadKind.DEFERRED_COMMENT.value: DeferredWorkflowSourceContract( + payload_kind=DeferredPayloadKind.DEFERRED_COMMENT.value, + workflow_name="Reviewer Bot PR Comment Router", + workflow_file=".github/workflows/reviewer-bot-pr-comment-router.yml", + artifact_name_prefix="reviewer-bot-comment-context-", + source_event_name="issue_comment", + source_event_action="created", + source_event_key_prefix="issue_comment:", + object_id_field="comment_id", + required_payload_fields=( + "payload_kind", + "schema_version", + "source_run_id", + "source_run_attempt", + "source_event_name", + "source_event_action", + "source_event_key", + "pr_number", + "comment_id", + "comment_body", + "comment_created_at", + "comment_author", + "comment_author_id", + "comment_user_type", + "comment_sender_type", + "comment_performed_via_github_app", + "issue_author", + "issue_state", + "issue_labels", + ), + required_identity_fields=_COMMON_REQUIRED_IDENTITY_FIELDS, + live_endpoint_kind="issue_comment", + ), + DeferredPayloadKind.DEFERRED_REVIEW_COMMENT.value: DeferredWorkflowSourceContract( + payload_kind=DeferredPayloadKind.DEFERRED_REVIEW_COMMENT.value, + workflow_name="Reviewer Bot PR Review Comment Observer", + workflow_file=".github/workflows/reviewer-bot-pr-review-comment-observer.yml", + artifact_name_prefix="reviewer-bot-review-comment-context-", + source_event_name="pull_request_review_comment", + source_event_action="created", + source_event_key_prefix="pull_request_review_comment:", + object_id_field="comment_id", + required_payload_fields=( + "payload_kind", + "schema_version", + "source_run_id", + "source_run_attempt", + "source_event_name", + "source_event_action", + "source_event_key", + "pr_number", + "comment_id", + "comment_body", + "comment_created_at", + "comment_author", + "comment_author_id", + "comment_user_type", + "comment_sender_type", + "comment_performed_via_github_app", + "issue_author", + "issue_state", + "issue_labels", + "source_commit_id", + ), + required_identity_fields=_COMMON_REQUIRED_IDENTITY_FIELDS, + live_endpoint_kind="review_comment", + ), + DeferredPayloadKind.DEFERRED_REVIEW_SUBMITTED.value: DeferredWorkflowSourceContract( + payload_kind=DeferredPayloadKind.DEFERRED_REVIEW_SUBMITTED.value, + workflow_name="Reviewer Bot PR Review Submitted Observer", + workflow_file=".github/workflows/reviewer-bot-pr-review-submitted-observer.yml", + artifact_name_prefix="reviewer-bot-review-submitted-context-", + source_event_name="pull_request_review", + source_event_action="submitted", + source_event_key_prefix="pull_request_review:", + object_id_field="review_id", + required_payload_fields=( + "payload_kind", + "schema_version", + "source_run_id", + "source_run_attempt", + "source_event_name", + "source_event_action", + "source_event_key", + "pr_number", + "review_id", + "source_submitted_at", + "source_review_state", + "source_commit_id", + "actor_login", + ), + required_identity_fields=_COMMON_REQUIRED_IDENTITY_FIELDS, + live_endpoint_kind="pull_request_review", + ), + DeferredPayloadKind.DEFERRED_REVIEW_DISMISSED.value: DeferredWorkflowSourceContract( + payload_kind=DeferredPayloadKind.DEFERRED_REVIEW_DISMISSED.value, + workflow_name="Reviewer Bot PR Review Dismissed Observer", + workflow_file=".github/workflows/reviewer-bot-pr-review-dismissed-observer.yml", + artifact_name_prefix="reviewer-bot-review-dismissed-context-", + source_event_name="pull_request_review", + source_event_action="dismissed", + source_event_key_prefix="pull_request_review_dismissed:", + object_id_field="review_id", + required_payload_fields=( + "payload_kind", + "schema_version", + "source_run_id", + "source_run_attempt", + "source_event_name", + "source_event_action", + "source_event_key", + "pr_number", + "review_id", + ), + required_identity_fields=_COMMON_REQUIRED_IDENTITY_FIELDS, + live_endpoint_kind="pull_request_review", + ), +} + + +def deferred_workflow_source_contract_for_payload_kind(payload_kind: str) -> DeferredWorkflowSourceContract: + try: + return _DEFERRED_WORKFLOW_SOURCE_CONTRACTS[payload_kind] + except KeyError as exc: + raise RuntimeError("Unsupported deferred workflow source contract payload kind") from exc + + +def _raw_identity_value(identity: DeferredArtifactIdentity | None, raw_payload: dict, field_name: str) -> object: + if identity is not None: + if field_name == "run_id": + return identity.source_run_id + if field_name == "run_attempt": + return identity.source_run_attempt + if field_name == "source_event_name": + return identity.source_event_name + if field_name == "source_event_action": + return identity.source_event_action + if field_name == "source_event_key": + return identity.source_event_key + if field_name == "source_pr_number": + return identity.pr_number + aliases = { + "workflow_name": ("workflow_name", "source_workflow_name"), + "workflow_file": ("workflow_file", "source_workflow_file"), + "run_id": ("run_id", "source_run_id"), + "run_attempt": ("run_attempt", "source_run_attempt"), + "artifact_name": ("artifact_name", "source_artifact_name"), + "source_pr_number": ("source_pr_number", "pr_number"), + }.get(field_name, (field_name,)) + for alias in aliases: + if alias in raw_payload: + return raw_payload.get(alias) + return None + + +def derive_deferred_artifact_source_authority( + identity: DeferredArtifactIdentity | None, + raw_payload: dict, + *, + triggering_conclusion: str | None = None, + contract: DeferredWorkflowSourceContract | None = None, +) -> DeferredArtifactSourceAuthority: + if not isinstance(raw_payload, dict): + raw_payload = {} + payload_kind = raw_payload.get("payload_kind") + if contract is None: + contract = deferred_workflow_source_contract_for_payload_kind(str(payload_kind)) + missing_payload = [field for field in contract.required_payload_fields if field not in raw_payload] + missing_identity = [ + field + for field in contract.required_identity_fields + if _raw_identity_value(identity, raw_payload, field) in {None, ""} + ] + workflow_name = _raw_identity_value(identity, raw_payload, "workflow_name") + workflow_file = _raw_identity_value(identity, raw_payload, "workflow_file") + artifact_name = _raw_identity_value(identity, raw_payload, "artifact_name") + source_event_key = _raw_identity_value(identity, raw_payload, "source_event_key") + object_id = raw_payload.get(contract.object_id_field) + expected_key = f"{contract.source_event_key_prefix}{object_id}" + status = "trusted_exact_identity" + reason = None + if identity is None or missing_identity: + status = "blocked_missing_identity" + reason = "missing_identity_fields:" + ",".join(missing_identity) + elif missing_payload: + status = "blocked_source_mismatch" + reason = "missing_payload_fields:" + ",".join(missing_payload) + elif triggering_conclusion and triggering_conclusion != "success": + status = "diagnostic_non_success_identity" + reason = f"triggering_conclusion:{triggering_conclusion}" + elif workflow_name != contract.workflow_name or workflow_file != contract.workflow_file: + status = "blocked_source_mismatch" + reason = "workflow_identity_mismatch" + elif not isinstance(artifact_name, str) or not artifact_name.startswith(contract.artifact_name_prefix): + status = "blocked_source_mismatch" + reason = "artifact_name_prefix_mismatch" + elif raw_payload.get("source_event_name") != contract.source_event_name or raw_payload.get("source_event_action") != contract.source_event_action: + status = "blocked_action_mismatch" + reason = "source_event_action_mismatch" + elif source_event_key != expected_key: + status = "blocked_source_mismatch" + reason = "source_event_key_object_mismatch" + return DeferredArtifactSourceAuthority( + workflow_name=str(workflow_name) if workflow_name is not None else None, + workflow_file=str(workflow_file) if workflow_file is not None else None, + run_id=str(_raw_identity_value(identity, raw_payload, "run_id")) if _raw_identity_value(identity, raw_payload, "run_id") is not None else None, + run_attempt=str(_raw_identity_value(identity, raw_payload, "run_attempt")) if _raw_identity_value(identity, raw_payload, "run_attempt") is not None else None, + artifact_name=str(artifact_name) if artifact_name is not None else None, + artifact_path=str(raw_payload.get("artifact_path")) if raw_payload.get("artifact_path") is not None else None, + source_event_key=str(source_event_key) if source_event_key is not None else None, + source_event_name=str(raw_payload.get("source_event_name")) if raw_payload.get("source_event_name") is not None else None, + source_event_action=str(raw_payload.get("source_event_action")) if raw_payload.get("source_event_action") is not None else None, + source_issue_number=int(raw_payload["issue_number"]) if isinstance(raw_payload.get("issue_number"), int) else None, + source_pr_number=int(raw_payload["pr_number"]) if isinstance(raw_payload.get("pr_number"), int) else None, + source_head_sha=str(raw_payload.get("source_commit_id")) if raw_payload.get("source_commit_id") is not None else None, + source_actor=str(raw_payload.get("comment_author") or raw_payload.get("actor_login") or raw_payload.get("source_actor_login") or "") or None, + authority_status=status, + diagnostic_reason=reason, + ) + def _contract_for_event(source_event_name: object, source_event_action: object) -> DeferredIdentityContract | None: if not isinstance(source_event_name, str) or not isinstance(source_event_action, str): @@ -572,7 +867,12 @@ def parse_deferred_context_payload(payload: dict) -> DeferredReviewPayload | Def _validate_deferred_review_artifact(payload) review_id = int(payload["review_id"]) _validate_identity_object_key(identity, review_id) - return DeferredReviewPayload( + payload_type = ( + DeferredReviewSubmittedPayload + if identity.payload_kind == DeferredPayloadKind.DEFERRED_REVIEW_SUBMITTED + else DeferredReviewDismissedPayload + ) + return payload_type( identity=identity, review_id=review_id, source_submitted_at=(str(payload["source_submitted_at"]) if payload.get("source_submitted_at") is not None else None), diff --git a/scripts/reviewer_bot_lib/reminder_comments.py b/scripts/reviewer_bot_lib/reminder_comments.py new file mode 100644 index 000000000..a90e4373e --- /dev/null +++ b/scripts/reviewer_bot_lib/reminder_comments.py @@ -0,0 +1,145 @@ +"""Reminder comment scan and diff helpers.""" + +from __future__ import annotations + +from dataclasses import dataclass + +from .config import TRANSITION_NOTICE_MARKER_PREFIX, TRANSITION_WARNING_MARKER_PREFIX + + +@dataclass(frozen=True) +class ReminderCommentRecord: + comment_id: int | str + author_login: str + created_at: str + body_first_line: str + matched_shape: str + url: str | None + + def to_output(self) -> dict[str, object]: + return { + "comment_id": self.comment_id, + "author_login": self.author_login, + "created_at": self.created_at, + "body_first_line": self.body_first_line, + "matched_shape": self.matched_shape, + "url": self.url, + } + + +@dataclass(frozen=True) +class ReminderCommentScan: + records: tuple[ReminderCommentRecord, ...] + baseline_count: int + baseline_latest_created_at: str | None + scan_status: str + + def to_output(self) -> dict[str, object]: + return { + "records": [record.to_output() for record in _sort_records(self.records)], + "baseline_count": self.baseline_count, + "baseline_latest_created_at": self.baseline_latest_created_at, + "scan_status": self.scan_status, + } + + +@dataclass(frozen=True) +class ReminderCommentDiff: + before_count: int + after_count: int + new_records: tuple[ReminderCommentRecord, ...] + diff_status: str + + def to_output(self) -> dict[str, object]: + return { + "before_count": self.before_count, + "after_count": self.after_count, + "new_records": [record.to_output() for record in _sort_records(self.new_records)], + "diff_status": self.diff_status, + } + + +_REMINDER_AUTHORS = {"github-actions[bot]", "guidelines-bot", "github-actions"} + + +def _sort_records(records: tuple[ReminderCommentRecord, ...]) -> tuple[ReminderCommentRecord, ...]: + return tuple(sorted(records, key=lambda record: (record.created_at, str(record.comment_id), record.matched_shape))) + + +def _first_line(body: str) -> str: + lines = body.splitlines() + return lines[0].strip() if lines else "" + + +def _matched_shape(first_line: str, body: str) -> str | None: + if first_line.startswith(f"", "").split(): + if part.startswith(prefix): + value = part[len(prefix) :].strip() + return value or None + return None + + +def _receipt_from_scanned_comments( + *, + issue_number: int, + reviewer: str | None, + head_sha: str | None, + cycle_key: str | None, + scope_key: str | None, + scanned_comments: tuple[object, ...], +) -> ReminderScopeReceipt | None: + records = sorted( + scanned_comments, + key=lambda item: (getattr(item, "created_at", ""), str(getattr(item, "comment_id", "")), getattr(item, "matched_shape", "")), + ) + legacy_records = [] + for record in records: + shape = getattr(record, "matched_shape", "") + first_line = getattr(record, "body_first_line", "") + if shape in {"markerized_warning", "markerized_transition_notice"}: + marker_issue = _marker_field(first_line, "issue") + marker_reviewer = _marker_field(first_line, "reviewer") + if marker_issue != str(issue_number) or not _same_login(marker_reviewer, reviewer): + continue + receipt_kind = "transition" if shape == "markerized_transition_notice" else "warning" + return ReminderScopeReceipt( + issue_number, + reviewer, + head_sha, + cycle_key, + scope_key, + receipt_kind, + getattr(record, "comment_id", None), + getattr(record, "created_at", None), + "comment_scan", + "found", + None, + ) + if shape in { + "legacy_unmarked_warning", + "legacy_unmarked_transition_notice", + "legacy_actions_warning_or_reminder", + }: + legacy_records.append(record) + + if len(legacy_records) >= 2: + latest = legacy_records[-1] + shape = getattr(latest, "matched_shape", "") + receipt_kind = "legacy_transition" if "transition" in shape else "legacy_warning_or_reminder" + return ReminderScopeReceipt( + issue_number, + reviewer, + head_sha, + cycle_key, + scope_key, + receipt_kind, + getattr(latest, "comment_id", None), + getattr(latest, "created_at", None), + "legacy_duplicate_comment_scan", + "found", + None, + ) + if legacy_records: + latest = legacy_records[-1] + return ReminderScopeReceipt( + issue_number, + reviewer, + head_sha, + cycle_key, + scope_key, + "none", + getattr(latest, "comment_id", None), + getattr(latest, "created_at", None), + "blocked", + "unavailable", + "ambiguous_legacy_reminder_scan", + ) + return None + + def derive_reminder_scope_receipt( *, issue_number: int, @@ -227,29 +318,36 @@ def derive_reminder_scope_receipt( ) if scoped_receipt is not None: return scoped_receipt - if scanned_comments: - record = sorted(scanned_comments, key=lambda item: getattr(item, "created_at", ""))[-1] - shape = getattr(record, "matched_shape", "") - kind = "legacy_transition" if "transition" in shape else "legacy_warning_or_reminder" + scanned_receipt = _receipt_from_scanned_comments( + issue_number=issue_number, + reviewer=reviewer, + head_sha=head_sha, + cycle_key=cycle_key, + scope_key=scope_key, + scanned_comments=scanned_comments, + ) + if scanned_receipt is not None: + return scanned_receipt + warning = persisted_state.get("transition_warning_sent") if isinstance(persisted_state, dict) else None + notice = persisted_state.get("transition_notice_sent_at") if isinstance(persisted_state, dict) else None + if isinstance(warning, str) and warning.strip() and isinstance(notice, str) and notice.strip(): return ReminderScopeReceipt( issue_number, reviewer, head_sha, cycle_key, scope_key, - kind, - getattr(record, "comment_id", None), - getattr(record, "created_at", None), - "comment_scan", - "found", + "legacy_transition", None, + notice, + "state", + "found", + "legacy_duplicate_exhausted", ) - warning = persisted_state.get("transition_warning_sent") if isinstance(persisted_state, dict) else None - notice = persisted_state.get("transition_notice_sent_at") if isinstance(persisted_state, dict) else None if isinstance(notice, str) and notice.strip(): - return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "transition", None, notice, "legacy_state_field", "found", "scope_unbound_legacy_field") + return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "none", None, notice, "blocked", "unavailable", "scope_unbound_legacy_field") if isinstance(warning, str) and warning.strip(): - return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "warning", None, warning, "legacy_state_field", "found", "scope_unbound_legacy_field") + return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "none", None, warning, "blocked", "unavailable", "scope_unbound_legacy_field") return ReminderScopeReceipt(issue_number, reviewer, head_sha, cycle_key, scope_key, "none", None, None, "derived_absent", "missing", None) @@ -292,7 +390,8 @@ def build_reminder_delivery_persistence_result( if comment_posted and state_save_attempted and not state_save_succeeded: result = "posted_save_failed_recoverable" if recovered_receipt is not None else "posted_save_failed_unrecoverable" elif comment_posted and not state_save_attempted: - result = "pending_state_save_with_receipt" if recovered_receipt is not None else "pending_state_save_missing_receipt" + result = "blocked" + diagnostic_reason = diagnostic_reason or "state_save_not_attempted_after_comment_post" elif recovered_receipt is not None and not comment_posted: result = "not_posted_existing_receipt" elif state_save_succeeded or not state_save_attempted: @@ -341,6 +440,20 @@ def derive_reminder_cadence_decision( ) -> ReminderCadenceDecision: issue_number = int(getattr(response, "scope", None).issue_number or 0) if getattr(response, "scope", None) else 0 reviewer = getattr(getattr(response, "scope", None), "reviewer", None) + if receipt is not None and (receipt.source == "blocked" or receipt.status in {"unavailable", "malformed"}): + return ReminderCadenceDecision( + issue_number=int(getattr(response, "scope", None).issue_number or 0) if getattr(response, "scope", None) else 0, + reviewer=getattr(getattr(response, "scope", None), "reviewer", None), + scope=getattr(response, "scope", None), + cadence_state="blocked", + exhaustion_reason="blocked", + warning_receipt=None, + transition_receipt=None, + legacy_duplicate_count=reminder_scan.baseline_count if reminder_scan is not None else 0, + may_post_warning=False, + may_post_transition=False, + must_project_reassignment_needed=False, + ) effective_receipt = receipt if receipt is not None and receipt.receipt_kind != "none" else None legacy_duplicate_count = reminder_scan.baseline_count if reminder_scan is not None else 0 exhausted = (effective_receipt is not None and effective_receipt.receipt_kind in {"transition", "legacy_transition"}) or legacy_duplicate_count >= 2 diff --git a/scripts/reviewer_bot_lib/reconcile.py b/scripts/reviewer_bot_lib/reconcile.py index 0e0e8d7ca..983230d4f 100644 --- a/scripts/reviewer_bot_lib/reconcile.py +++ b/scripts/reviewer_bot_lib/reconcile.py @@ -503,7 +503,7 @@ def _recover_missing_active_review_entry( live_author=None, live_labels=(), recovered_current_reviewer=None, - recovery_status="orphaned_deferred_event", + recovery_status="blocked_live_pr_unavailable", diagnostic_reason=str(exc), ) changed = _record_missing_row_orphan(state, context) @@ -522,7 +522,7 @@ def _recover_missing_active_review_entry( source_event_action, live_pr, recovered_current_reviewer=None, - recovery_status="orphaned_deferred_event", + recovery_status="diagnostic_open_item_orphaned_event", diagnostic_reason="reconstruction_not_allowed_for_diagnostic_admission", ) changed = _record_missing_row_orphan(state, context) @@ -537,7 +537,7 @@ def _recover_missing_active_review_entry( source_event_action, live_pr, recovered_current_reviewer=None, - recovery_status="orphaned_deferred_event", + recovery_status="diagnostic_open_item_orphaned_event", diagnostic_reason="live_pr_state_not_open", ) changed = _record_missing_row_orphan(state, context) @@ -560,7 +560,7 @@ def _recover_missing_active_review_entry( source_event_action, live_pr, recovered_current_reviewer=None, - recovery_status="orphaned_deferred_event", + recovery_status="diagnostic_open_item_orphaned_event", diagnostic_reason="missing_row_recovery_requires_exactly_one_live_reviewer", ) changed = _record_missing_row_orphan(state, context) diff --git a/scripts/reviewer_bot_lib/reconcile_payloads.py b/scripts/reviewer_bot_lib/reconcile_payloads.py index ce9c4f14c..b6cb5e178 100644 --- a/scripts/reviewer_bot_lib/reconcile_payloads.py +++ b/scripts/reviewer_bot_lib/reconcile_payloads.py @@ -445,7 +445,7 @@ def derive_deferred_artifact_source_authority( status = "trusted_exact_identity" reason = None if identity is not None and identity.schema_version == 2: - status = "diagnostic_legacy_identity" + status = "trusted_legacy_identity" reason = "legacy_payload_missing_workflow_artifact_authority" elif identity is None or missing_identity: status = "blocked_missing_identity" diff --git a/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py b/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py index ff5283a1d..c85cb9a18 100644 --- a/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py +++ b/tests/integration/reviewer_bot/test_app_closed_issue_cleanup.py @@ -185,6 +185,6 @@ def test_execute_run_late_workflow_run_reconcile_missing_row_records_orphan(monk assert result.exit_code == 0 assert state["active_reviews"] == {} - assert state["sidecars"]["orphaned_deferred_reconcile_events"]["issue_comment:210"]["recovery_status"] == "orphaned_deferred_event" + assert state["sidecars"]["orphaned_deferred_reconcile_events"]["issue_comment:210"]["recovery_status"] == "blocked_live_pr_unavailable" assert save_called["value"] is True assert sync_calls == [[42]] diff --git a/tests/integration/reviewer_bot/test_app_execution.py b/tests/integration/reviewer_bot/test_app_execution.py index 2a2e266ba..c798efe0a 100644 --- a/tests/integration/reviewer_bot/test_app_execution.py +++ b/tests/integration/reviewer_bot/test_app_execution.py @@ -366,6 +366,29 @@ def test_execute_run_returns_failure_when_save_state_fails(monkeypatch): assert receipt_logs[-1]["fields"]["next_recovery_action"] == "recover_from_live_github_receipt" +def test_execute_run_persists_recovery_receipt_after_initial_save_failure(monkeypatch): + harness = AppHarness(monkeypatch) + harness.set_event(EVENT_NAME="issue_comment", EVENT_ACTION="created") + harness.stub_lock(acquire=lambda: None, release=lambda: True) + harness.stub_load_state(lambda *, fail_on_unavailable=False: make_state()) + harness.stub_pass_until(lambda state: (state, [])) + harness.stub_sync_members(lambda state: (state, [])) + harness.stub_handler("handle_comment_event", lambda state: True) + save_results = iter([False, True]) + harness.stub_save_state(lambda state: next(save_results)) + + result = harness.run_execute() + + assert result.exit_code == 1 + assert len(harness.state_store.save_calls) == 2 + receipts = harness.state_store.save_calls[-1]["state_issue_write_receipts"] + assert any( + receipt["write_status"] == "failed_after_external_side_effect" + and receipt["next_recovery_action"] == "recover_from_live_github_receipt" + for receipt in receipts.values() + ) + + def test_execute_run_releases_lock_after_save_failure(monkeypatch): harness = AppHarness(monkeypatch) harness.set_event(EVENT_NAME="issue_comment", EVENT_ACTION="created") diff --git a/tests/integration/reviewer_bot/test_app_workflow_run_bookkeeping.py b/tests/integration/reviewer_bot/test_app_workflow_run_bookkeeping.py index 97f69f341..fbe017de0 100644 --- a/tests/integration/reviewer_bot/test_app_workflow_run_bookkeeping.py +++ b/tests/integration/reviewer_bot/test_app_workflow_run_bookkeeping.py @@ -323,7 +323,7 @@ def test_execute_run_workflow_run_missing_row_records_orphan_then_projects( assert result.exit_code == 0 assert result.state_changed is True assert state["active_reviews"] == {} - assert state["sidecars"]["orphaned_deferred_reconcile_events"]["pull_request_review_dismissed:12"]["recovery_status"] == "orphaned_deferred_event" + assert state["sidecars"]["orphaned_deferred_reconcile_events"]["pull_request_review_dismissed:12"]["recovery_status"] == "blocked_live_pr_unavailable" assert save_snapshots assert projected_issue_numbers == [42] diff --git a/tests/integration/reviewer_bot/test_reconcile_workflow_run.py b/tests/integration/reviewer_bot/test_reconcile_workflow_run.py index 2441e561e..a0160442d 100644 --- a/tests/integration/reviewer_bot/test_reconcile_workflow_run.py +++ b/tests/integration/reviewer_bot/test_reconcile_workflow_run.py @@ -200,7 +200,7 @@ def test_late_workflow_run_reconcile_missing_row_is_diagnostic_safe_noop(monkeyp assert result == reconcile.WorkflowRunHandlerResult(True, [42]) assert state["active_reviews"] == {} orphan = state["sidecars"]["orphaned_deferred_reconcile_events"]["issue_comment:210"] - assert orphan["recovery_status"] == "orphaned_deferred_event" + assert orphan["recovery_status"] == "blocked_live_pr_unavailable" @pytest.mark.parametrize( @@ -1315,7 +1315,7 @@ def test_deferred_legacy_review_comment_hydrates_source_commit_id_from_live_comm gap = _deferred_gaps(review)["pull_request_review_comment:305"] assert gap["reason"] == "artifact_invalid" assert gap["failure_kind"] == "blocked_untrusted_source" - assert "diagnostic_legacy_identity" in gap["diagnostic_summary"] + assert "trusted_legacy_identity" in gap["diagnostic_summary"] assert review["reviewer_comment"]["accepted"] is None @@ -1346,7 +1346,7 @@ def test_deferred_legacy_review_comment_without_live_commit_id_records_artifact_ gap = _deferred_gaps(review)["pull_request_review_comment:306"] assert gap["reason"] == "artifact_invalid" assert gap["failure_kind"] == "blocked_untrusted_source" - assert "diagnostic_legacy_identity" in gap["diagnostic_summary"] + assert "trusted_legacy_identity" in gap["diagnostic_summary"] assert "source_commit_id" not in gap diff --git a/tests/unit/reviewer_bot/test_overdue_reminder_scope.py b/tests/unit/reviewer_bot/test_overdue_reminder_scope.py index c527a324c..6802b7827 100644 --- a/tests/unit/reviewer_bot/test_overdue_reminder_scope.py +++ b/tests/unit/reviewer_bot/test_overdue_reminder_scope.py @@ -1,10 +1,19 @@ +from scripts.reviewer_bot_core.reviewer_response_policy import ( + ReviewCycleScope, + ReviewerResponseDecision, +) from scripts.reviewer_bot_lib.overdue import ( build_reminder_delivery_persistence_result, + derive_reminder_cadence_decision, derive_reminder_scope_receipt, ) +from scripts.reviewer_bot_lib.reminder_comments import ( + ReminderCommentRecord, + ReminderCommentScan, +) -def test_state_warning_field_becomes_scope_receipt_not_global_authority(): +def test_state_warning_field_blocks_without_scope_receipt_authority(): receipt = derive_reminder_scope_receipt( issue_number=264, reviewer="iglesias", @@ -14,9 +23,108 @@ def test_state_warning_field_becomes_scope_receipt_not_global_authority(): persisted_state={"transition_warning_sent": "2026-04-01T00:00:00Z"}, ) - assert receipt.receipt_kind == "warning" + assert receipt.receipt_kind == "none" + assert receipt.source == "blocked" + assert receipt.status == "unavailable" assert receipt.created_at == "2026-04-01T00:00:00Z" assert receipt.scope_key == "scope-a" + assert receipt.reason == "scope_unbound_legacy_field" + + +def test_state_delivery_receipt_requires_matching_scope_identity(): + persisted_state = { + "sidecars": { + "reminder_delivery_receipts": { + "warning:other": { + "receipt_kind": "warning", + "reviewer": "other", + "head_sha": "head-a", + "cycle_key": "cycle-a", + "scope_key": "scope-a", + "comment_id": 123, + "comment_created_at": "2026-04-01T00:00:00Z", + }, + "warning:matching": { + "receipt_kind": "warning", + "reviewer": "iglesias", + "head_sha": "head-a", + "cycle_key": "cycle-a", + "scope_key": "scope-a", + "comment_id": 124, + "comment_created_at": "2026-04-02T00:00:00Z", + }, + } + } + } + + receipt = derive_reminder_scope_receipt( + issue_number=264, + reviewer="iglesias", + head_sha="head-a", + cycle_key="cycle-a", + scope_key="scope-a", + persisted_state=persisted_state, + ) + + assert receipt.comment_id == 124 + assert receipt.source == "state" + + +def test_single_ambiguous_legacy_scan_blocks_posting_instead_of_reusing_scope(): + record = ReminderCommentRecord( + comment_id=123, + author_login="github-actions[bot]", + created_at="2026-04-01T00:00:00Z", + body_first_line="Review reminder", + matched_shape="legacy_actions_warning_or_reminder", + url=None, + ) + scan = ReminderCommentScan( + records=(record,), + baseline_count=1, + baseline_latest_created_at="2026-04-01T00:00:00Z", + scan_status="pass", + ) + receipt = derive_reminder_scope_receipt( + issue_number=264, + reviewer="iglesias", + head_sha="head-a", + cycle_key="cycle-a", + scope_key="scope-a", + persisted_state={}, + scanned_comments=scan.records, + ) + response = ReviewerResponseDecision( + response_state="awaiting_reviewer_response", + reason="assigned", + suppression_reason=None, + scope=ReviewCycleScope(264, "iglesias", "head-a", "cycle-a", "scope-a", "active_head", "2026-03-01T00:00:00Z"), + current_head_sha="head-a", + anchor_timestamp="2026-03-01T00:00:00Z", + reviewer_authority_outcome="tracked_reviewer_confirmed", + latest_reviewer_activity_kind=None, + latest_reviewer_activity_timestamp=None, + latest_contributor_handoff_timestamp=None, + suppresses_overdue_reminder=False, + suppresses_reassignment_followup=False, + completion_state="not_completed", + write_approval_authority=None, + ) + + cadence = derive_reminder_cadence_decision( + response, + receipt=receipt, + reminder_scan=scan, + now="2026-05-01T00:00:00Z", + review_deadline_days=7, + transition_period_days=7, + ) + + assert receipt.source == "blocked" + assert receipt.reason == "ambiguous_legacy_reminder_scan" + assert cadence.cadence_state == "blocked" + assert cadence.may_post_warning is False + assert cadence.may_post_transition is False def test_posted_comment_save_failure_requires_live_receipt_recovery(): @@ -26,7 +134,21 @@ def test_posted_comment_save_failure_requires_live_receipt_recovery(): head_sha="head-a", cycle_key="cycle-a", scope_key="scope-a", - persisted_state={"transition_warning_sent": "2026-04-01T00:00:00Z"}, + persisted_state={ + "sidecars": { + "reminder_delivery_receipts": { + "warning:scope-a:123": { + "receipt_kind": "warning", + "reviewer": "iglesias", + "head_sha": "head-a", + "cycle_key": "cycle-a", + "scope_key": "scope-a", + "comment_id": 123, + "comment_created_at": "2026-04-01T00:00:00Z", + } + } + } + }, ) result = build_reminder_delivery_persistence_result( diff --git a/tests/unit/reviewer_bot/test_reconcile_unit.py b/tests/unit/reviewer_bot/test_reconcile_unit.py index b7bbe7d6b..4e2012abf 100644 --- a/tests/unit/reviewer_bot/test_reconcile_unit.py +++ b/tests/unit/reviewer_bot/test_reconcile_unit.py @@ -793,7 +793,7 @@ def test_legacy_deferred_comment_identity_is_diagnostic_only(monkeypatch): assert result == reconcile.WorkflowRunHandlerResult(True, [42]) gap = review["sidecars"]["deferred_gaps"]["issue_comment:210"] assert gap["reason"] == "artifact_invalid" - assert "diagnostic_legacy_identity" in gap["diagnostic_summary"] + assert "trusted_legacy_identity" in gap["diagnostic_summary"] assert review["sidecars"]["reconciled_source_events"] == {} @@ -873,7 +873,7 @@ def test_non_success_missing_row_records_orphan_without_reconstruction(monkeypat assert result == reconcile.WorkflowRunHandlerResult(True, [42]) assert "42" not in state["active_reviews"] orphan = state["sidecars"]["orphaned_deferred_reconcile_events"]["issue_comment:210"] - assert orphan["recovery_status"] == "orphaned_deferred_event" + assert orphan["recovery_status"] == "diagnostic_open_item_orphaned_event" assert orphan["diagnostic_reason"] == "reconstruction_not_allowed_for_diagnostic_admission"