From 79913951e8afcbe66d5e530e27686b8ce9ca4513 Mon Sep 17 00:00:00 2001 From: huangruiteng Date: Fri, 2 Oct 2026 13:08:23 +0800 Subject: [PATCH] fix(pr-review): read effective approvals across reviewer identities Signed-off-by: huangruiteng --- .../pr_review_queue/approval_closeout.py | 30 ++++---- .../pr_review_approval_closeout.ts | 39 ++++++++-- .../test_pr_review_approval_closeout.py | 72 +++++++++++++++++-- 3 files changed, 112 insertions(+), 29 deletions(-) diff --git a/loopx/capabilities/pr_review_queue/approval_closeout.py b/loopx/capabilities/pr_review_queue/approval_closeout.py index b84093fd5e..848d284182 100644 --- a/loopx/capabilities/pr_review_queue/approval_closeout.py +++ b/loopx/capabilities/pr_review_queue/approval_closeout.py @@ -78,25 +78,21 @@ def read_github_approval_closeout(*, repository: str, exact_head: str) -> dict[s reviews = [row for page in pages for row in page] if any(not isinstance(row, dict) for row in reviews): raise ValueError("review readback is malformed") - own_reviews = [{"state": row.get("state"), "body": row.get("body"), - "author": row.get("user"), "submittedAt": row.get("submitted_at"), - "commit": {"oid": row.get("commit_id")}} - for row in reviews if isinstance(row.get("user"), dict) - and str(row["user"].get("login", "")).casefold() == login.casefold()] - # Reuse the current standalone/exact-head body validator, not a new approval rule. - approval = _review_conclusion(pr | {"reviews": own_reviews}, reviewer_login=login, - behavior_bearing=bool({row["area"] for row in _files(pr)} & (CODE_AREAS | BEHAVIORAL_POLICY_AREAS))) - if approval["valid"]: - matching = [row for row in reviews if row.get("submitted_at") == approval["submitted_at"] - and row.get("state") == approval["state"] - and row.get("commit_id") == pr.get("headRefOid") - and str(row.get("user", {}).get("login", "")).casefold() == login.casefold()] - if len(matching) != 1: - raise ValueError("approval readback identity is ambiguous") - approval["review_id"] = matching[0].get("id") + # Validate bodies here; the typed owner selects the effective opinion from + # the complete history. Authentication permits the read, not reviewer scope. + behavior_bearing = bool({row["area"] for row in _files(pr)} & (CODE_AREAS | BEHAVIORAL_POLICY_AREAS)) + conclusions = [] + for row in reviews: + review = {"state": row.get("state"), "body": row.get("body"), + "author": row.get("user"), "submittedAt": row.get("submitted_at"), + "commit": {"oid": row.get("commit_id")}} + reviewer = row.get("user", {}).get("login") if isinstance(row.get("user"), dict) else None + conclusion = _review_conclusion(pr | {"reviews": [review]}, + reviewer_login=reviewer, behavior_bearing=behavior_bearing) + conclusions.append(conclusion | {"review_id": row.get("id")}) after = run_gh_json(args) if not isinstance(after, dict) or after.get("number") != number or after.get("state") != pr.get("state"): raise ValueError("pull-request identity or lifecycle changed during readback") return plan_approval_closeout({"repository": repository, "expected_exact_head": exact_head, "pull_request": pr, "readback_head": after.get("headRefOid"), "reviews": reviews, - "reviews_complete": True, "approval_conclusion": approval}) + "reviews_complete": True, "review_conclusions": conclusions}) diff --git a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts index 5c8764c562..cc1c64eb4a 100644 --- a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts +++ b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts @@ -7,7 +7,8 @@ import {parseIsoTimestamp} from "../runtime_timestamp.ts"; // GitHub owns these input states; closeout statuses are local to this read model. type ReviewState = "PENDING" | "COMMENTED" | "APPROVED" | "CHANGES_REQUESTED" | "DISMISSED"; -type Review = {id: number; login: string; state: ReviewState; head: string; time: number; url: string}; +type Review = {id: number; login: string; state: ReviewState; head: string; time: number; + submittedAt: string | null; url: string}; const states = new Set(["PENDING", "COMMENTED", "APPROVED", "CHANGES_REQUESTED", "DISMISSED"]); function fail(message: string): never { throw new EffectRuntimeRequestError(message); } function oid(value: unknown): string { @@ -21,13 +22,11 @@ export function planPrReviewApprovalCloseout(value: unknown): JsonObject { const match = /^([1-9][0-9]*)@([a-f0-9]{40}|[a-f0-9]{64})$/.exec(exact); if (!match) return fail("approval closeout requires NUMBER@full_HEAD_OID"); const pr = requireJsonObject(request.pull_request, "pull_request"); - const approval = requireJsonObject(request.approval_conclusion, "approval_conclusion"); const holds: string[] = []; if (pr.number !== Number(match[1])) holds.push("pull_request_identity_changed"); if (pr.state !== "OPEN") holds.push("pull_request_not_open"); if (pr.headRefOid !== match[2] || request.readback_head !== match[2]) holds.push("head_changed"); if (request.reviews_complete !== true) holds.push("review_source_incomplete"); - if (approval.valid !== true || approval.verdict !== "APPROVE") holds.push("exact_head_approval_missing"); if (!Array.isArray(request.reviews)) return fail("reviews must be a complete array"); const seen = new Set(); const reviews: Review[] = request.reviews.map(value => { @@ -45,12 +44,39 @@ export function planPrReviewApprovalCloseout(value: unknown): JsonObject { if (parsed === null) return fail("submitted_at must be an ISO timestamp"); const time = parsed.getTime(); return {id, login, state, head: oid(row.commit_id), time, + submittedAt: state === "PENDING" ? null : String(row.submitted_at), url: requireNonEmptyString(row.html_url, "review URL")}; }); + if (!Array.isArray(request.review_conclusions)) return fail("review_conclusions must be a complete array"); + const reviewsById = new Map(reviews.map(review => [review.id, review])); + const conclusions = new Map(); + for (const value of request.review_conclusions) { + const row = requireJsonObject(value, "review conclusion"); + const id = row.review_id; + if (typeof id !== "number" || !seen.has(id) || conclusions.has(id)) { + return fail("review conclusion must bind one unique review id"); + } + const review = reviewsById.get(id)!; + if (row.valid === true && (row.state !== review.state || row.reviewer !== review.login)) { + return fail("review conclusion identity does not match history"); + } + conclusions.set(id, row); + } + if (conclusions.size !== reviews.length) return fail("review conclusions are incomplete"); + const author = typeof pr.author === "object" && pr.author !== null + ? requireNonEmptyString(requireJsonObject(pr.author, "PR author").login, "PR author login").toLowerCase() : null; const latest = new Map(); for (const row of reviews.sort((a, b) => a.time - b.time || a.id - b.id)) { - if (row.state !== "PENDING" && row.state !== "COMMENTED") latest.set(row.login.toLowerCase(), row); + const conclusion = conclusions.get(row.id)!; + const authorFallback = row.state === "COMMENTED" && row.login.toLowerCase() === author + && conclusion.valid === true && ["APPROVE", "REQUEST_CHANGES"].includes(String(conclusion.verdict)); + if (row.state !== "PENDING" && (row.state !== "COMMENTED" || authorFallback)) latest.set(row.login.toLowerCase(), row); } + const approvalReview = [...latest.values()].filter(row => row.head === match[2] + && conclusions.get(row.id)!.valid === true && conclusions.get(row.id)!.verdict === "APPROVE" + && (row.state === "APPROVED" || (row.state === "COMMENTED" && row.login.toLowerCase() === author))) + .sort((a, b) => b.time - a.time || b.id - a.id)[0]; + if (!approvalReview) holds.push("exact_head_approval_missing"); const blockers = [...latest.values()].filter(row => row.state === "CHANGES_REQUESTED") .sort((a, b) => a.id - b.id).map(row => ({review_id: row.id, reviewer: row.login, review_head: row.head, review_url: row.url, on_approved_head: row.head === match[2]})); @@ -62,7 +88,8 @@ export function planPrReviewApprovalCloseout(value: unknown): JsonObject { status: holds.length ? "hold" : blockers.length ? "verification_required" : "clear", hold_reasons: holds, blocking_reviews: blockers, observed_review_decision: pr.reviewDecision ?? null, - approval_snapshot: {reviewer: approval.reviewer ?? null, review_id: approval.review_id ?? null, - state: approval.state ?? null, submitted_at: approval.submitted_at ?? null}, + approval_snapshot: {reviewer: approvalReview?.login ?? null, review_id: approvalReview?.id ?? null, + state: approvalReview?.state ?? null, + submitted_at: approvalReview?.submittedAt ?? null}, dismissal_authorized: false, merge_authorized: false, github_write_performed: false}; } diff --git a/tests/capabilities/test_pr_review_approval_closeout.py b/tests/capabilities/test_pr_review_approval_closeout.py index b74d60f1a0..3fafdeb67c 100644 --- a/tests/capabilities/test_pr_review_approval_closeout.py +++ b/tests/capabilities/test_pr_review_approval_closeout.py @@ -18,11 +18,18 @@ def review(identity, state, *, login="other", head=OLD): def request(reviews): + reviews = [*reviews, review(50, "APPROVED", login="maintainer", head=HEAD)] return {"expected_exact_head": f"42@{HEAD}", "repository": "owner/repo", "pull_request": {"number": 42, "headRefOid": HEAD, "state": "OPEN", "reviewDecision": "CHANGES_REQUESTED"}, "readback_head": HEAD, "reviews_complete": True, "reviews": reviews, - "approval_conclusion": {"valid": True, "verdict": "APPROVE"}} + "review_conclusions": [conclusion(row) for row in reviews]} + + +def conclusion(row): + return {"valid": row["state"] == "APPROVED", "verdict": "APPROVE", + "review_id": row["id"], "reviewer": row["user"].get("login"), + "state": row["state"], "submitted_at": row["submitted_at"]} def test_other_reviewer_blocker_survives_own_approval_and_comments(): @@ -70,12 +77,15 @@ def test_only_latest_blocker_per_reviewer_needs_reconciliation(): @pytest.mark.parametrize("field,value,reason", [ ("readback_head", OLD, "head_changed"), ("reviews_complete", False, "review_source_incomplete"), - ("approval_conclusion", {"valid": False, "verdict": "APPROVE"}, "exact_head_approval_missing"), - ("approval_conclusion", {"valid": True, "verdict": "REQUEST_CHANGES"}, "exact_head_approval_missing"), + ("approval_valid", False, "exact_head_approval_missing"), + ("approval_verdict", "REQUEST_CHANGES", "exact_head_approval_missing"), ]) def test_unverified_closeout_is_a_hold_not_clear(field, value, reason): data = request([review(1, "CHANGES_REQUESTED")]) - data[field] = value + if field.startswith("approval_"): + data["review_conclusions"][-1][field.removeprefix("approval_")] = value + else: + data[field] = value result = plan_approval_closeout(data) assert result["status"] == "hold" assert reason in result["hold_reasons"] @@ -92,6 +102,55 @@ def test_malformed_history_fails_closed(change): plan_approval_closeout(request([row])) +@pytest.mark.parametrize("superseding", ["CHANGES_REQUESTED", "DISMISSED"]) +def test_effective_foreign_opinion_not_authentication_selects_approval(superseding): + data = request([]) + foreign = review(1, "APPROVED", login="foreign", head=HEAD) + data["reviews"] = [foreign] + data["review_conclusions"] = [conclusion(foreign)] + data["pull_request"]["reviewDecision"] = "REVIEW_REQUIRED" + assert plan_approval_closeout(data)["approval_snapshot"]["reviewer"] == "foreign" + later = review(2, superseding, login="FOREIGN", head=HEAD) + data["reviews"].append(later) + data["review_conclusions"].append(conclusion(later)) + result = plan_approval_closeout(data) + assert result["status"] == "hold" + assert result["approval_snapshot"]["review_id"] is None + assert "exact_head_approval_missing" in result["hold_reasons"] + + +def test_ordinary_comment_does_not_retire_approval_or_override_another_blocker(): + data = request([review(1, "CHANGES_REQUESTED")]) + comment = review(51, "COMMENTED", login="maintainer", head=HEAD) + data["reviews"].append(comment) + data["review_conclusions"].append(conclusion(comment)) + result = plan_approval_closeout(data) + assert result["status"] == "verification_required" + assert result["approval_snapshot"]["review_id"] == 50 + assert [row["review_id"] for row in result["blocking_reviews"]] == [1] + + +def test_author_commented_fallback_can_be_read_by_another_operator(): + author = review(1, "COMMENTED", login="contributor", head=HEAD) + data = request([]) + data["pull_request"].update(author={"login": "contributor"}, reviewDecision=None) + data["reviews"] = [author] + data["review_conclusions"] = [conclusion(author) | {"valid": True}] + assert plan_approval_closeout(data)["approval_snapshot"]["state"] == "COMMENTED" + rejection = review(2, "COMMENTED", login="contributor", head=HEAD) + data["reviews"].append(rejection) + data["review_conclusions"].append(conclusion(rejection) | {"valid": True, "verdict": "REQUEST_CHANGES"}) + assert plan_approval_closeout(data)["status"] == "hold" + + +@pytest.mark.parametrize("change", [{"review_id": 99}, {"reviewer": "forged"}, {"state": "COMMENTED"}]) +def test_conclusion_cannot_rebind_history_identity(change): + data = request([]) + data["review_conclusions"][-1].update(change) + with pytest.raises(ValueError): + plan_approval_closeout(data) + + def test_capability_owns_closeout_and_preserves_authority_boundary(): closeout = build_agent_response_contract()["review_execution_contract"]["approval_closeout"] assert "--check-approval-closeout NUMBER@HEAD_OID" in closeout["readback_command"] @@ -103,7 +162,8 @@ def test_capability_owns_closeout_and_preserves_authority_boundary(): assert "target_dismissed_approval_preserved_head_unchanged" in closeout["readback_requires"] -def test_live_adapter_paginates_history_without_ci_or_mutations(monkeypatch, capsys): +@pytest.mark.parametrize("authenticated_login", ["maintainer", "observer"]) +def test_live_adapter_paginates_history_without_ci_or_mutations(monkeypatch, capsys, authenticated_login): from loopx.cli import main import loopx.pr_review as pr_module from loopx.capabilities.pr_review_queue import github_source @@ -124,7 +184,7 @@ def read(args, **_): "files": [{"path": "loopx/runtime.py"}], "changedFiles": 1} monkeypatch.setattr(github_source, "run_gh_json", read) - monkeypatch.setattr(pr_module, "resolve_current_github_login", lambda: "maintainer") + monkeypatch.setattr(pr_module, "resolve_current_github_login", lambda: authenticated_login) assert main(["--format", "json", "pr-review", "--repo", "owner/repo", "--check-approval-closeout", f"42@{HEAD}"]) == 0 result = json.loads(capsys.readouterr().out)