From dfecb2849fde779d1d95aa3a7e2b63a662dd30e4 Mon Sep 17 00:00:00 2001 From: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Date: Sun, 20 Sep 2026 08:20:59 +0800 Subject: [PATCH] fix(pr-review): owe merge readiness for every approved open head GitHub blocks self-approval, so an author-owned approval is necessarily recorded as a `COMMENTED` review with the titled fallback. The queue keyed the merge-readiness qualification on the formal `APPROVED` state, so those heads counted as `current_head_concluded`: of 39 open heads on 2026-09-20, five held a valid approval and were behind or blocked, and the queue reported three actionable rows instead of eight. `pull_request_merge_readiness_v0` already decides this from the typed verdict, so the queue now agrees with the gate it routes to and an approved open head keeps owing `qualify_pull_request_merge_readiness`. An offline fixture may now declare `reviewer_login`, because a fixture without a reviewer identity cannot express an author-owned conclusion at all; the public smoke covers an approved head that degraded, one that is still clean, and a request-changes fallback that stays inventory-only. Validation: the queue packet on the live window (attention 3 -> 8, the five approved heads now in `review_sequence`), `--check-merge-readiness` for a degraded and a clean approved head, `examples/pr-review-command-smoke.py`, and `pytest -k pr_review` (164 passed, 12 skipped). Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> --- examples/pr-review-command-smoke.py | 130 +++++++++++++++++- loopx/capabilities/pr_review_queue/README.md | 16 ++- .../pr_review_queue/selection_execution.py | 6 +- loopx/cli_commands/pr_review.py | 4 +- loopx/pr_review.py | 18 ++- tests/capabilities/test_pr_review_queue.py | 71 ++++++++++ tests/test_pr_review_github_scan.py | 16 ++- 7 files changed, 246 insertions(+), 15 deletions(-) diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 258832bf60..2fea1b84a1 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -387,6 +387,134 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: "current_head_review_missing_or_invalid" in blocked["blocking_reasons"] ), blocked assert "status_checks_failed" in blocked["blocking_reasons"], blocked + + # Merge readiness follows the typed approval verdict, not GitHub's review + # state. The platform blocks self-approval, so an author-owned approval is + # stored as COMMENTED; a state-based rule counted these still-open heads as + # concluded and hid approved heads that had gone behind, conflicted, lost + # checks, or become blocked. + approval_head = "f" * 40 + + def approved_open_head( + number: int, + *, + merge_state: str, + review_state: str = "COMMENTED", + verdict: str = "APPROVE", + ) -> dict[str, object]: + title = ( + "Approval conclusion (author-owned PR; GitHub blocks formal self-approval)" + if verdict == "APPROVE" + else "Request changes conclusion (author-owned PR; GitHub blocks formal self-review)" + ) + return { + "number": number, + "title": f"Approved open head {number}", + "url": f"https://github.com/owner/repo/pull/{number}", + "state": "OPEN", + "author": {"login": "maintainer"}, + "headRefOid": approval_head, + "baseRefName": "main", + "isDraft": False, + "reviewDecision": "REVIEW_REQUIRED", + "mergeStateStatus": merge_state, + "files": [{"path": "src/runtime.py", "additions": 1, "deletions": 1}], + "reviews": [ + { + "state": review_state, + "body": ( + f"{title}\n\n" + "## 动机\n动机。\n\n## 改动思路\n思路。\n\n" + "## 具体改动\n改动。\n\n## 对主干的风险\n风险。\n\n" + "## 我的整体评价\n通过。\n\n" + f"English verdict: {verdict} at exact head {approval_head}." + ), + "author": {"login": "maintainer"}, + "commit": {"oid": approval_head}, + "submittedAt": "2026-09-09T11:14:01Z", + } + ], + "statusCheckRollup": [ + {"name": "Sign-off", "status": "COMPLETED", "conclusion": "SUCCESS"}, + { + "name": "merge-gate", + "status": "COMPLETED", + "conclusion": "SUCCESS" if merge_state == "CLEAN" else "FAILURE", + }, + ], + "review_thread_summary": { + "schema_version": "github_review_thread_summary_v0", + "complete": True, + "total_count": 0, + "unresolved_count": 0, + }, + } + + with tempfile.TemporaryDirectory() as temp_dir: + approval_fixture_path = Path(temp_dir) / "approved-open-heads.json" + approval_fixture = { + "repository": "owner/repo", + "reviewer_login": "maintainer", + "pull_requests": [ + approved_open_head(4111, merge_state="BEHIND"), + approved_open_head(4112, merge_state="CLEAN"), + approved_open_head( + 4113, merge_state="CLEAN", verdict="REQUEST_CHANGES" + ), + ], + } + approval_fixture_path.write_text( + json.dumps(approval_fixture), encoding="utf-8" + ) + approval_packet = json.loads( + run_cli( + "--format", + "json", + "pr-review", + "--fixture", + str(approval_fixture_path), + "--state", + "open", + ).stdout + ) + approvals = { + item["number"]: item for item in approval_packet["pull_requests"] + } + for number in (4111, 4112): + assert approvals[number]["review_conclusion"]["verdict"] == "APPROVE", approvals + assert ( + approvals[number]["review_action_kind"] + == "qualify_pull_request_merge_readiness" + ), approvals[number] + assert approvals[4111]["merge_state"] == "BEHIND", approvals[4111] + assert approvals[4112]["merge_state"] == "CLEAN", approvals[4112] + assert approvals[4113]["review_conclusion"]["verdict"] == "REQUEST_CHANGES", approvals + assert approvals[4113]["review_action_kind"] is None, approvals[4113] + assert approval_packet["summary"]["review_attention_count"] == 2, ( + approval_packet["summary"] + ) + assert sorted( + item["number"] for item in approval_packet["review_sequence"] + ) == [4111, 4112], approval_packet["review_sequence"] + for number, expected_blockers in ( + (4111, ("merge_state_requires_update", "status_checks_failed")), + (4112, ()), + ): + readiness = json.loads( + run_cli( + "--format", + "json", + "pr-review", + "--fixture", + str(approval_fixture_path), + "--check-merge-readiness", + f"{number}@{approval_head}", + check=not expected_blockers, + ).stdout + ) + for blocker in expected_blockers: + assert blocker in readiness["blocking_reasons"], readiness + assert readiness["ready"] is (not expected_blockers), readiness assert sequence[0]["risk_hint_level"] == "medium", sequence[0] assert sequence[0]["main_risk_level"] == "medium", sequence[0] merged_sequence = next(item for item in sequence if item["number"] == 770) @@ -767,7 +895,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: ) assert incomplete_observation["candidate"] is None, incomplete_observation - repository, fixture_prs = load_pr_fixture(FIXTURE) + repository, fixture_prs, _fixture_reviewer = load_pr_fixture(FIXTURE) merged_fixture = next(item for item in fixture_prs if item.get("state") == "MERGED") busy_window = [] for offset in range(105): diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 27dee8fe23..3996ed27b1 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -212,11 +212,17 @@ The command packet keeps inventory and execution queues distinct. window for compact conclusion readback. Top-level and group `review_sequence` contain only rows whose `review_action_kind` is non-null. A merged exact head without a valid conclusion receives `audit_merged_pull_request_exact_head`; a -merged or open exact head with a valid non-action conclusion remains -inventory-only and cannot become the recommended first PR. The summary's -attention counts are derived from this same actionable set. Inventory-only rows -set `review_plan` and `review_template` to null and `evidence_commands` to an -empty list so hosts cannot mistake readback metadata for execution authority. +merged or open exact head whose valid conclusion is not an approval remains +inventory-only and cannot become the recommended first PR. An open exact head +with a valid approval keeps owing `qualify_pull_request_merge_readiness`, +because merge readiness is decided by the typed verdict rather than by GitHub's +review state: the platform blocks self-approval, so an author-owned approval is +recorded as `COMMENTED` and a state-based rule would count that still-unmerged +head as concluded, even after it goes behind, conflicts, loses its checks or is +blocked. The summary's attention counts are derived from this same actionable +set. Inventory-only rows set `review_plan` and `review_template` to null and +`evidence_commands` to an empty list so hosts cannot mistake readback metadata +for execution authority. It emits a `pull_request_review_todo_preview_v0` bound to its exact head. The preview may diff --git a/loopx/capabilities/pr_review_queue/selection_execution.py b/loopx/capabilities/pr_review_queue/selection_execution.py index 2e260a5017..ee03a66a3d 100644 --- a/loopx/capabilities/pr_review_queue/selection_execution.py +++ b/loopx/capabilities/pr_review_queue/selection_execution.py @@ -21,7 +21,11 @@ def review_action_kind(item: Mapping[str, Any]) -> str | None: conclusion = item.get("review_conclusion") conclusion = conclusion if isinstance(conclusion, Mapping) else {} if conclusion.get("valid") is True: - if state == "OPEN" and str(conclusion.get("state") or "").upper() == "APPROVED": + # Merge readiness is owed by the typed verdict, not by GitHub's review + # state: the platform blocks self-approval, so an author-owned approval + # is recorded as COMMENTED and would otherwise sit in the concluded + # lane forever, even after the head stops being mergeable. + if state == "OPEN" and str(conclusion.get("verdict") or "").upper() == "APPROVE": return "qualify_pull_request_merge_readiness" return None if state == "MERGED": diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index d0a04a56fe..22ef96b9a5 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -275,7 +275,7 @@ def handle_pr_review_command( repository = args.repo reviewer_login = None if args.fixture: - fixture_repository, pull_requests = load_pr_fixture( + fixture_repository, pull_requests, reviewer_login = load_pr_fixture( Path(args.fixture).expanduser() ) repository = repository or fixture_repository @@ -368,7 +368,7 @@ def handle_pr_review_command( source = "github_cli" reviewer_login = None if args.fixture: - repository_from_fixture, pull_requests = load_pr_fixture( + repository_from_fixture, pull_requests, reviewer_login = load_pr_fixture( Path(args.fixture).expanduser() ) repository = repository or repository_from_fixture diff --git a/loopx/pr_review.py b/loopx/pr_review.py index c634620130..b38ab12670 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -338,16 +338,26 @@ def append_state(state: str) -> None: } -def load_pr_fixture(path: Path) -> tuple[str | None, list[dict[str, Any]]]: +def load_pr_fixture( + path: Path, +) -> tuple[str | None, list[dict[str, Any]], str | None]: + """Load an offline PR window, including the reviewer identity it declares. + + Author-owned conclusions depend on the reviewer identity: GitHub records an + author-owned approval as `COMMENTED`, so offline use must be able to name + the authenticated reviewer or it cannot represent that real case. + """ + payload = json.loads(path.read_text(encoding="utf-8")) if isinstance(payload, list): - return None, [item for item in payload if isinstance(item, dict)] + return None, [item for item in payload if isinstance(item, dict)], None if not isinstance(payload, dict): - return None, [] + return None, [], None items = payload.get("pull_requests") or payload.get("prs") or [] return ( str(payload.get("repository") or "") or None, [item for item in _as_list(items) if isinstance(item, dict)], + str(payload.get("reviewer_login") or "") or None, ) @@ -1302,7 +1312,7 @@ def _review_why_now(item: dict[str, Any]) -> str: return "Draft PR; skim for early direction but do not treat as merge-ready." conclusion = _as_dict(item.get("review_conclusion")) if conclusion.get("valid") is True: - if str(conclusion.get("state") or "").upper() == "APPROVED": + if str(conclusion.get("verdict") or "").upper() == "APPROVE": return "The current exact head has a complete approval; qualify merge readiness." return "The current exact head already has a complete standalone conclusion." if item.get("author_owned"): diff --git a/tests/capabilities/test_pr_review_queue.py b/tests/capabilities/test_pr_review_queue.py index dc679fece1..a8b45a4100 100644 --- a/tests/capabilities/test_pr_review_queue.py +++ b/tests/capabilities/test_pr_review_queue.py @@ -7,10 +7,37 @@ PullRequestReviewPriority, build_pull_request_review_queue_observation, build_scheduling_policy, + materialize_review_execution, scheduling_tier, ) +def _concluded_item( + *, + number: int = 1, + state: str = "OPEN", + conclusion: dict[str, object] | None = None, + draft: bool = False, + decision: str = "REVIEW_REQUIRED", +) -> dict[str, object]: + return { + "number": number, + "state": state, + "head_oid": f"{number:040d}", + "is_draft": draft, + "review_decision": decision, + "review_conclusion": conclusion + if conclusion is not None + else {"valid": True, "state": "COMMENTED", "verdict": "APPROVE"}, + } + + +def _action_kind(item: dict[str, object]) -> str | None: + return materialize_review_execution( + item, fresh_audit_exact_heads=set() + )["review_action_kind"] + + def _pr( number: int, *, @@ -418,6 +445,50 @@ def test_approved_transition_routes_to_merge_policy_without_granting_it() -> Non assert approved["write_authority_granted"] is False +def test_open_head_merge_readiness_follows_typed_verdict() -> None: + """An approval keeps owing the pre-merge gate while the PR is open. + + GitHub blocks self-approval, so an author-owned approval is stored as a + COMMENTED review. Keying the queue on the formal review state counted such a + head as concluded and hid approved heads that had gone behind, conflicted, + lost checks, or become blocked. + """ + + assert ( + _action_kind(_concluded_item()) + == "qualify_pull_request_merge_readiness" + ) + assert ( + _action_kind( + _concluded_item( + conclusion={"valid": True, "state": "APPROVED", "verdict": "APPROVE"} + ) + ) + == "qualify_pull_request_merge_readiness" + ) + + author_owned_request_changes = _concluded_item( + conclusion={ + "valid": True, + "state": "COMMENTED", + "verdict": "REQUEST_CHANGES", + } + ) + assert _action_kind(author_owned_request_changes) is None + assert _action_kind(_concluded_item(state="MERGED")) is None + assert _action_kind(_concluded_item(draft=True)) is None + + invalid = _concluded_item( + conclusion={"valid": False, "state": None, "verdict": None} + ) + assert _action_kind(invalid) == "review_pull_request_exact_head" + changes_requested = _concluded_item( + conclusion={"valid": False, "state": None, "verdict": None}, + decision="CHANGES_REQUESTED", + ) + assert _action_kind(changes_requested) == "rereview_pull_request_exact_head" + + def test_review_backlog_keeps_active_cadence_until_all_handled() -> None: first = _observe([_pr(1), _pr(2), _pr(3)]) diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index 0339083518..86496bc363 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -1185,7 +1185,13 @@ def test_latest_review_and_author_owned_fallback_are_enforced(monkeypatch) -> No reviewer_login="maintainer", )["pull_requests"][0] assert valid_fallback["review_conclusion"]["status"] == "valid" - assert valid_fallback["review_action_kind"] is None + # Merge readiness follows the typed verdict, not GitHub's review state: an + # author-owned approval is COMMENTED because the platform blocks + # self-approval, and it still owes the pre-merge gate while the PR is open. + assert ( + valid_fallback["review_action_kind"] + == "qualify_pull_request_merge_readiness" + ) row["reviews"] = [ { @@ -1281,7 +1287,13 @@ def test_latest_review_and_author_owned_fallback_are_enforced(monkeypatch) -> No reviewer_login="maintainer", )["pull_requests"][0] assert ordinary_comment["review_conclusion"]["status"] == "valid" - assert ordinary_comment["review_action_kind"] is None + # The later COMMENTED note is not a conclusion, so the earlier valid + # author-owned approval still binds the current head and still owes the + # pre-merge gate. + assert ( + ordinary_comment["review_action_kind"] + == "qualify_pull_request_merge_readiness" + ) def test_actionable_sequence_excludes_valid_merged_exact_head(monkeypatch) -> None: