diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index d32918cf45..a7feaa1144 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -88,7 +88,24 @@ concurrency: github.event_name == 'repository_dispatch' && github.event.client_payload.pr_number != '' && format('pr-{0}', github.event.client_payload.pr_number) || github.event_name == 'repository_dispatch' && format('repo-dispatch-{0}', github.repository) || github.ref }} - cancel-in-progress: ${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' || github.event_name == 'repository_dispatch' }} + cancel-in-progress: >- + ${{ + github.event_name == 'pull_request_target' || + github.event_name == 'repository_dispatch' || + ( + github.event_name == 'pull_request_review' && + ( + github.event.action == 'dismissed' || + ( + github.event.action == 'submitted' && + ( + github.event.review.state == 'approved' || + github.event.review.state == 'changes_requested' + ) + ) + ) + ) + }} # Scorecard Token-Permissions (alert #9): declare a least-privilege default at # the workflow level. The scan-pr-queue job that actually needs write access @@ -109,6 +126,17 @@ jobs: ( github.event_name != 'repository_dispatch' || github.event.client_payload.org_sweep != true + ) && + ( + github.event_name != 'pull_request_review' || + github.event.action == 'dismissed' || + ( + github.event.action == 'submitted' && + ( + github.event.review.state == 'approved' || + github.event.review.state == 'changes_requested' + ) + ) ) runs-on: ubuntu-24.04 # Bound scan-pr-queue to a wall-clock ceiling well short of GitHub's diff --git a/CHANGELOG.md b/CHANGELOG.md index 06b3dba425..5e96dfc703 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ - Raised `hourly-review-repair.yml`'s discovery ceiling from 50 to 200 while rotating deterministic 50-PR deep-inspection windows by hourly run number. The scheduler hydrates only the selected window and stops immediately after its single dispatch, preserving access to newer PRs without quadrupling expensive review/check/comment work. See `docs/doctoring/hourly-review-repair-single-file-consolidation.md`'s 2026-09-03 follow-up. ## [Unreleased] +- Stop `pull_request_review: submitted` events with state `commented` at the + merge scheduler's job-admission boundary, before a hosted runner is + requested. `approved`, `changes_requested`, and `dismissed` review + transitions retain their existing exact-PR scheduler path and permissions; + COMMENTED submissions also no longer cancel an already-running actionable + review transition through workflow-level concurrency. - Include merge-scheduler entrypoint, core, and regression-test changes in the existing runtime-quality workflow's trigger and suite selector. Scheduler workflow edits retain queue checks and also select the full review-repair diff --git a/docs/doctoring/pr-review-merge-scheduler-trigger-audit-20260903.md b/docs/doctoring/pr-review-merge-scheduler-trigger-audit-20260903.md index b81778cfef..3a5ff2c00e 100644 --- a/docs/doctoring/pr-review-merge-scheduler-trigger-audit-20260903.md +++ b/docs/doctoring/pr-review-merge-scheduler-trigger-audit-20260903.md @@ -1,5 +1,21 @@ # Doctoring record: pr-review-merge-scheduler.yml's "fires at every step" pattern is by-design, not a bug (2026-09-03) +> **2026-09-05 correction.** The broad claim below that every submitted review +> is an actionable approval-state change was incomplete. GitHub emits +> `pull_request_review: submitted` for `COMMENTED` reviews, which do not create +> an `APPROVED` or `CHANGES_REQUESTED` state. On PR #1885, CodeRabbit submitted +> `COMMENTED` reviews at 03:08:52Z, 04:27:31Z, and 05:30:37Z; the central +> scheduler admitted runner-backed runs 33941045179, 33944606701, and +> 33947394894 within seconds. The scheduler still needs the review trigger for +> `APPROVED`, `CHANGES_REQUESTED`, and `dismissed`, but `COMMENTED` is now +> rejected by the `scan-pr-queue` job-level `if` before runner acquisition. +> The executable truth-table contract is +> `tests/test_merge_scheduler_review_event_admission.py`. This correction does +> not reinterpret a bot comment as formal review evidence. It preserves the +> exact-PR concurrency group while narrowing cancellation so a COMMENTED +> submission cannot cancel an already-running APPROVED, CHANGES_REQUESTED, or +> dismissed transition; scheduler permissions remain unchanged. + - **Date:** 2026-09-03 - **Subject:** the user directly observed the scheduler workflow firing repeatedly ("왜 각 모든 단계마다 Trigger 되고 있죠?") after live evidence surfaced today of severe org-wide Actions thrashing (near-zero completion diff --git a/tests/test_current_head_coalescer_self_cancellation.py b/tests/test_current_head_coalescer_self_cancellation.py index e49193d4df..45ad4b469e 100644 --- a/tests/test_current_head_coalescer_self_cancellation.py +++ b/tests/test_current_head_coalescer_self_cancellation.py @@ -26,9 +26,10 @@ def test_current_head_coalescer_shares_pr_scoped_scheduler_admission() -> None: assert "github.repository == 'ContextualWisdomLab/.github'" in coalescer assert "github.event.pull_request.head.sha" not in concurrency_block assert "github.event.pull_request.number" in concurrency_block - assert any( - line.startswith("cancel-in-progress:") - and "github.event_name == 'pull_request_target'" in line - for line in active_lines - ) + normalized_concurrency = " ".join(active_lines) + assert "cancel-in-progress: >- ${{" in normalized_concurrency + assert "github.event_name == 'pull_request_target'" in normalized_concurrency + assert "github.event_name == 'repository_dispatch'" in normalized_concurrency + assert "github.event.review.state == 'approved'" in normalized_concurrency + assert "github.event.review.state == 'changes_requested'" in normalized_concurrency assert "queue: max" not in workflow_text diff --git a/tests/test_merge_scheduler_review_event_admission.py b/tests/test_merge_scheduler_review_event_admission.py new file mode 100644 index 0000000000..dd0d0e8b6c --- /dev/null +++ b/tests/test_merge_scheduler_review_event_admission.py @@ -0,0 +1,160 @@ +"""Executable admission contract for merge-scheduler review events.""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] +WORKFLOW = ROOT / ".github" / "workflows" / "pr-review-merge-scheduler.yml" + + +def scan_job_condition() -> str: + """Return the normalized pre-runner condition for ``scan-pr-queue``.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + scan_job = workflow.split("\n scan-pr-queue:\n", 1)[1] + condition = scan_job.split("\n runs-on:", 1)[0].split("\n if: >-\n", 1)[1] + return " ".join(line.strip() for line in condition.splitlines()) + + +def cancellation_condition() -> str: + """Return the normalized workflow-level cancellation expression.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + marker = "\n cancel-in-progress: " + raw = workflow.split(marker, 1)[1] + if raw.startswith(">-\n"): + condition = raw.split("\n\n", 1)[0].removeprefix(">-\n") + else: + condition = raw.splitlines()[0] + normalized = " ".join(line.strip() for line in condition.splitlines()) + assert normalized.startswith("${{ ") and normalized.endswith(" }}") + return normalized.removeprefix("${{ ").removesuffix(" }}") + + +def evaluate_review_expression( + *, expression: str, action: str, state: str, event_name: str = "pull_request_review" +) -> bool: + """Evaluate a workflow review-event expression for one trusted fixture.""" + expression = expression.replace("&&", " and ").replace("||", " or ") + expression = re.sub(r"\btrue\b", "True", expression) + values = { + "github.event_name": event_name, + "github.event.action": action, + "github.event.review.state": state, + "github.event.client_payload.org_sweep": False, + } + + def evaluate(node: ast.AST) -> object: + """Interpret only the boolean/comparison subset used by the job guard.""" + if isinstance(node, ast.Expression): + return evaluate(node.body) + if isinstance(node, ast.BoolOp): + operands = [bool(evaluate(value)) for value in node.values] + return all(operands) if isinstance(node.op, ast.And) else any(operands) + if isinstance(node, ast.Compare) and len(node.ops) == len(node.comparators) == 1: + left = evaluate(node.left) + right = evaluate(node.comparators[0]) + if isinstance(node.ops[0], ast.Eq): + return left == right + if isinstance(node.ops[0], ast.NotEq): + return left != right + if isinstance(node, ast.Constant): + return node.value + if isinstance(node, (ast.Attribute, ast.Name)): + key = ast.unparse(node) + if key in values: + return values[key] + raise AssertionError(f"unsupported scheduler expression node: {ast.dump(node)}") + + return bool(evaluate(ast.parse(expression, mode="eval"))) + + +def admits_review_event(*, action: str, state: str) -> bool: + """Evaluate the pre-runner admission condition for one review fixture.""" + return evaluate_review_expression( + expression=scan_job_condition(), action=action, state=state + ) + + +def cancels_predecessor( + *, action: str, state: str, event_name: str = "pull_request_review" +) -> bool: + """Evaluate whether one review event cancels the prior scheduler run.""" + return evaluate_review_expression( + expression=cancellation_condition(), + action=action, + state=state, + event_name=event_name, + ) + + +@pytest.mark.parametrize( + ("action", "state", "expected"), + [ + ("submitted", "commented", False), + ("submitted", "approved", True), + ("submitted", "changes_requested", True), + ("dismissed", "commented", True), + ], +) +def test_review_event_admission_truth_table( + action: str, state: str, expected: bool +) -> None: + """Admit only review transitions that can change merge eligibility.""" + assert admits_review_event(action=action, state=state) is expected + + +def test_review_filter_preserves_exact_pr_group_and_least_privilege() -> None: + """Filtering COMMENTED reviews must not weaken scheduler trust boundaries.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + assert ( + "github.event_name == 'pull_request_review' && " + "format('pr-{0}', github.event.pull_request.number)" in workflow + ) + assert "permissions:\n contents: read" in workflow + + scan_header = workflow.split("\n scan-pr-queue:\n", 1)[1].split( + "\n env:\n", 1 + )[0] + for permission in ( + "actions: write", + "checks: read", + "contents: write", + "id-token: write", + "pull-requests: write", + ): + assert permission in scan_header + + +def test_commented_review_does_not_cancel_approved_review_execution() -> None: + """A later COMMENTED review must preserve an approved scheduler execution.""" + assert cancels_predecessor(action="submitted", state="approved") is True + assert cancels_predecessor(action="submitted", state="commented") is False + assert cancels_predecessor(action="submitted", state="changes_requested") is True + assert cancels_predecessor(action="dismissed", state="commented") is True + + +@pytest.mark.parametrize( + ("event_name", "expected"), + [ + ("pull_request_target", True), + ("repository_dispatch", True), + ("push", False), + ("schedule", False), + ("workflow_call", False), + ], +) +def test_review_filter_preserves_non_review_cancellation_semantics( + event_name: str, expected: bool +) -> None: + """Narrow only review-event cancellation, preserving other trigger behavior.""" + assert ( + cancels_predecessor( + event_name=event_name, action="submitted", state="commented" + ) + is expected + ) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 37ec068db9..4784da541b 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2408,10 +2408,17 @@ def test_merge_scheduler_uses_escalating_mutation_credentials(): assert 'check_delay="$((check_attempt * 2))"' in workflow assert "steps.review_followup.outputs.proceed != 'false'" in workflow assert "Native events and the explicit org-sweep recovery remain authoritative." in workflow - assert ( - "github.event_name == 'pull_request_review' || " - "github.event_name == 'repository_dispatch'" in workflow - ) + concurrency_block = workflow.split("\nconcurrency:\n", 1)[1].split( + "\n# Scorecard", 1 + )[0] + assert "cancel-in-progress: >-" in concurrency_block + for required_transition in ( + "github.event.action == 'dismissed'", + "github.event.review.state == 'approved'", + "github.event.review.state == 'changes_requested'", + "github.event_name == 'repository_dispatch'", + ): + assert required_transition in concurrency_block def test_opencode_runs_merge_scheduler_after_review_without_repo_local_dispatch(): diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 803d43ab59..37d8d518a3 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -112,7 +112,7 @@ def test_merge_scheduler_uses_native_auto_merge_after_required_checks() -> None: assert "github.event_name == 'repository_dispatch' && github.run_id" not in ( concurrency_contract ) - assert "cancel-in-progress: ${{" in concurrency_contract + assert "cancel-in-progress: >-" in concurrency_contract assert "github.event_name == 'repository_dispatch'" in concurrency_contract