diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index f7aad0de70..8cb89eb6a3 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -799,7 +799,21 @@ state transition, author-owned conclusions use `COMMENTED` plus one exact title: `Approval conclusion (author-owned PR; GitHub blocks formal self-approval)` or `Request changes conclusion (author-owned PR; GitHub blocks formal self-review)`. The compact result is versioned as `pull_request_review_conclusion_v0` and -reports a typed verdict and invalid-reason codes. +reports a typed verdict and invalid-reason codes. Queue selection and approval +closeout share the typed owner for each reviewer's latest submitted opinion. +A brief approval by another reviewer cannot hide a complete exact-head approval. +A reviewer's newer formal opinion retires their own previous opinion, including +when it is dismissed or lacks the required body. Current-head requests for +changes remain visible even without that format, and ordinary comments or +pending reviews cannot restore a retired approval. A changed head still requires +fresh evidence. This corrects default queue/readiness classification; it neither +relaxes body qualification nor grants dismissal or merge authority. + +队列与 closeout 复用同一 TypeScript 历史选择规则：另一评审者的简短批准不会 +遮住完整的精确 head 批准；同一评审者的新正式意见会替换自己的旧意见，撤回或 +格式不完整也不会复活旧批准。当前 head 的阻塞意见保留，普通评论及待提交评审 +不改变已有正式意见，head 变化仍需新证据。本修复改变默认队列和 readiness +分类，不放宽正文校验，也不授予撤回评审或合并权限。 After a published exact-head `APPROVE` is read back, the capability-owned `review_execution_contract.approval_closeout` requires effective-review diff --git a/loopx/capabilities/pr_review_queue/approval_closeout.py b/loopx/capabilities/pr_review_queue/approval_closeout.py index 20df0c00cb..f6324aa435 100644 --- a/loopx/capabilities/pr_review_queue/approval_closeout.py +++ b/loopx/capabilities/pr_review_queue/approval_closeout.py @@ -41,6 +41,18 @@ def plan_approval_closeout(request: dict[str, Any]) -> dict[str, Any]: return result +def select_review_conclusion(request: dict[str, Any]) -> int: + """Transport qualified bodies to the shared typed history owner.""" + try: + result = effect_runtime_result("capabilities.pr_review.conclusion.select", request) + except EffectRuntimeRejected as error: + raise ValueError(str(error)) from error + index = result.get("index") if isinstance(result, dict) else None + if type(index) is not int or not 0 <= index < len(request["reviews"]): + raise TypeError("typed review conclusion index mismatch") + return index + + def read_github_approval_closeout(*, repository: str, exact_head: str) -> dict[str, Any]: from .github_source import _fetch_complete_pr_files, run_gh_json from ...pr_review import ( diff --git a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts index cc1c64eb4a..7958601332 100644 --- a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts +++ b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts @@ -7,8 +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; - submittedAt: string | null; url: string}; +type Opinion = {id: number; login: string; state: ReviewState; head: string; time: number}; +type Review = Opinion & {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 { @@ -16,6 +16,51 @@ function oid(value: unknown): string { return /^(?:[a-f0-9]{40}|[a-f0-9]{64})$/.test(result) ? result : fail("review commit must be a full SHA"); } +/** A submitted formal opinion retires only that reviewer's prior opinion. + * Ordinary comments and pending reviews cannot retire an earned conclusion. */ +function effectiveOpinions(reviews: readonly T[], author: string | null, + conclusions: ReadonlyMap): T[] { + const latest = new Map(); + for (const row of [...reviews].sort((a, b) => a.time - b.time || a.id - b.id)) { + 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); + } + return [...latest.values()]; +} + +/** Bodies are qualified by the adapter; history precedence has one typed owner. + * Queue indices are local positions, never GitHub review identifiers. */ +export function selectPrReviewConclusion(value: unknown): JsonObject { + const request = requireJsonObject(value, "review conclusion selection"); + if (!Array.isArray(request.reviews) || !request.reviews.length) return fail("review history must be nonempty"); + const history = request.reviews; + const conclusions = new Map(); + const reviews: Opinion[] = history.map((value, index) => { + const row = requireJsonObject(value, "review opinion"); + const state = row.state as ReviewState; + if (!states.has(state)) return fail("unknown GitHub review state"); + if (typeof row.time !== "number" || !Number.isFinite(row.time)) return fail("review time must be finite"); + if (typeof row.head !== "string" || typeof row.login !== "string") return fail("review identity must be strings"); + // The adapter supplies newest-first rows; equal-time ties retain its order. + const id = history.length - index; + conclusions.set(id, requireJsonObject(row.conclusion, "qualified review conclusion")); + return {id, login: row.login.toLowerCase(), state, head: row.head.toLowerCase(), time: row.time}; + }); + const author = typeof request.author === "string" ? request.author.toLowerCase() : null; + const head = requireNonEmptyString(request.head, "current head").toLowerCase(); + const effective = effectiveOpinions(reviews, author, conclusions) + .sort((a, b) => b.time - a.time || b.id - a.id); + // Unformatted dissent remains dissent. A different reviewer's later approval + // cannot erase it; complete closeout still reconciles blockers on older heads. + const selected = effective.find(row => row.head === head && (row.state === "CHANGES_REQUESTED" + || conclusions.get(row.id)!.valid === true && conclusions.get(row.id)!.verdict === "REQUEST_CHANGES")) + ?? effective.find(row => row.head === head && conclusions.get(row.id)!.valid === true) + ?? effective[0] ?? reviews[0]; + return {index: history.length - selected.id}; +} + export function planPrReviewApprovalCloseout(value: unknown): JsonObject { const request = requireJsonObject(value, "approval closeout"); const exact = requireNonEmptyString(request.expected_exact_head, "expected_exact_head"); @@ -65,19 +110,13 @@ export function planPrReviewApprovalCloseout(value: unknown): JsonObject { 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)) { - 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] + const effective = effectiveOpinions(reviews, author, conclusions); + const approvalReview = effective.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") + const blockers = effective.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]})); if (request.reviews_complete === true && !blockers.length && pr.reviewDecision === "CHANGES_REQUESTED") { diff --git a/loopx/control_plane/effect_runtime_handlers.ts b/loopx/control_plane/effect_runtime_handlers.ts index fd09e7404a..884bb7451d 100644 --- a/loopx/control_plane/effect_runtime_handlers.ts +++ b/loopx/control_plane/effect_runtime_handlers.ts @@ -462,6 +462,7 @@ export function createEffectRuntimeHandlers( ["capabilities.issue_fix.monitor_reconciliation.plan", lazyHandler(() => import("./capabilities/issue_fix_monitor_reconciliation.ts"), ({planIssueFixMonitorReconciliation}) => planIssueFixMonitorReconciliation)], ["capabilities.change_quality.validation_gate", lazyHandler(() => import("./capabilities/change_quality_validation.ts"), ({qualifyChangeQualityValidation}) => qualifyChangeQualityValidation)], ["capabilities.pr_review.approval_closeout.plan", lazyHandler(() => import("./capabilities/pr_review_approval_closeout.ts"), ({planPrReviewApprovalCloseout}) => planPrReviewApprovalCloseout)], + ["capabilities.pr_review.conclusion.select", lazyHandler(() => import("./capabilities/pr_review_approval_closeout.ts"), ({selectPrReviewConclusion}) => selectPrReviewConclusion)], ["capabilities.pr_review.configuration", lazyHandler(() => import("./capabilities/pr_review_order.ts"), ({prReviewConfiguration}) => prReviewConfiguration)], ["capabilities.pr_review.order", lazyHandler(() => import("./capabilities/pr_review_order.ts"), ({orderPrReviewQueue}) => orderPrReviewQueue)], ["coordination.local_authority_shadow.record", lazyHandler(() => import("./coordination/local_authority_shadow.ts"), ({recordLocalAuthorityShadow}) => recordLocalAuthorityShadow)], diff --git a/loopx/pr_review.py b/loopx/pr_review.py index 37e9dd6002..f9b8003abc 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -860,16 +860,18 @@ def evaluate(review: Mapping[str, Any]) -> dict[str, Any]: "invalid_reasons": reasons, } - latest_result = evaluate(reviews[0]) - if ( - str(reviews[0].get("state") or "").upper() == "COMMENTED" - and not latest_result["valid"] - ): - for earlier in reviews[1:]: - earlier_result = evaluate(earlier) - if earlier_result["valid"]: - return earlier_result - return latest_result + conclusions = [evaluate(review) for review in reviews] + if len(reviews) == 1: + return conclusions[0] + from .capabilities.pr_review_queue.approval_closeout import select_review_conclusion + index = select_review_conclusion({"head": head_oid, "author": pr_author, + "reviews": [{"state": conclusion["state"], + "login": str(_as_dict(review.get("author")).get("login") or ""), + "head": str(_as_dict(review.get("commit")).get("oid") or ""), + "time": _parse_updated_epoch(review.get("submittedAt")), + "conclusion": conclusion} + for review, conclusion in zip(reviews, conclusions)]}) + return conclusions[index] def _review_sequence_entry(item: dict[str, Any], *, rank: int) -> dict[str, Any]: diff --git a/tests/capabilities/test_pr_review_approval_closeout.py b/tests/capabilities/test_pr_review_approval_closeout.py index 3fafdeb67c..f24682e6dd 100644 --- a/tests/capabilities/test_pr_review_approval_closeout.py +++ b/tests/capabilities/test_pr_review_approval_closeout.py @@ -63,6 +63,17 @@ def test_aggregate_history_conflict_is_not_a_clear_closeout(): assert "aggregate_review_decision_conflict" in result["hold_reasons"] +def test_brief_approval_by_another_reviewer_does_not_hide_qualified_approval(): + data = request([]) + brief = review(51, "APPROVED", login="reporter", head=HEAD) + data["reviews"].append(brief) + data["review_conclusions"].append(conclusion(brief) | {"valid": False}) + data["pull_request"]["reviewDecision"] = "APPROVED" + result = plan_approval_closeout(data) + assert result["status"] == "clear" + assert result["approval_snapshot"]["review_id"] == 50 + + def test_only_latest_blocker_per_reviewer_needs_reconciliation(): result = plan_approval_closeout(request([ review(1, "CHANGES_REQUESTED"), review(2, "CHANGES_REQUESTED", head=HEAD), diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index d556899870..1382ca6479 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -1691,6 +1691,83 @@ def test_latest_review_and_author_owned_fallback_are_enforced(monkeypatch) -> No ) +def test_other_reviewers_brief_approval_preserves_full_exact_head_conclusion() -> None: + row = _queue_pr(32, author="community", ready_at="2026-08-18T07:00:00Z", + updated_at="2026-08-18T11:00:00Z") + head = row["headRefOid"] + row["reviews"] = [ + {"author": {"login": "maintainer"}, "body": _full_review_body(head), + "commit": {"oid": head}, "state": "APPROVED", + "submittedAt": "2026-08-18T09:00:00Z"}, + {"author": {"login": "reporter"}, "body": "Native platform reproduction passes.", + "commit": {"oid": head}, "state": "APPROVED", + "submittedAt": "2026-08-18T10:00:00Z"}, + ] + item = pr_review_module.build_pr_review_packet(pull_requests=[row], + repository="owner/repo", limit=10, source="fixture", reviewer_login="maintainer", + wait_for_ci=False)["pull_requests"][0] + assert item["review_conclusion"]["valid"] is True + assert item["review_conclusion"]["reviewer"] == "maintainer" + assert item["review_action_kind"] == "qualify_pull_request_merge_readiness" + + +@pytest.mark.parametrize("superseding", ["DISMISSED", "CHANGES_REQUESTED", "APPROVED"]) +def test_same_reviewers_new_opinion_cannot_resurrect_approval(superseding) -> None: + head = "a" * 40 + reviews = [ + {"author": {"login": "maintainer"}, "body": _full_review_body(head), + "commit": {"oid": head}, "state": "APPROVED", + "submittedAt": "2026-08-18T09:00:00Z"}, + {"author": {"login": "MAINTAINER"}, "body": "New formal opinion.", + "commit": {"oid": head}, "state": superseding, + "submittedAt": "2026-08-18T10:00:00Z"}, + {"author": {"login": "observer"}, "body": "Supplemental note.", + "commit": {"oid": head}, "state": "COMMENTED", + "submittedAt": "2026-08-18T11:00:00Z"}, + ] + result = pr_review_module._review_conclusion( + {"headRefOid": head, "author": {"login": "community"}, "reviews": reviews}, + reviewer_login="maintainer") + assert result["valid"] is False + assert result["state"] == superseding + + +def test_unformatted_current_head_blocker_survives_other_approval() -> None: + head = "a" * 40 + reviews = [ + {"author": {"login": "dissent"}, "body": "Please fix the reproduced failure.", + "commit": {"oid": head}, "state": "CHANGES_REQUESTED", + "submittedAt": "2026-08-18T09:00:00Z"}, + {"author": {"login": "maintainer"}, "body": _full_review_body(head), + "commit": {"oid": head}, "state": "APPROVED", + "submittedAt": "2026-08-18T10:00:00Z"}, + ] + result = pr_review_module._review_conclusion( + {"headRefOid": head, "author": {"login": "community"}, "reviews": reviews}, + reviewer_login="maintainer") + assert result["valid"] is False + assert result["state"] == "CHANGES_REQUESTED" + assert result["reviewer"] == "dissent" + + +@pytest.mark.parametrize("head", ["a" * 40, "b" * 40]) +def test_pending_review_cannot_retire_opinion_or_transfer_it_to_new_head(head) -> None: + approved_head = "a" * 40 + result = pr_review_module._review_conclusion({"headRefOid": head, + "author": {"login": "community"}, "reviews": [ + {"author": {"login": "maintainer"}, "body": _full_review_body(approved_head), + "commit": {"oid": approved_head}, "state": "APPROVED", + "submittedAt": "2026-08-18T09:00:00Z"}, + {"author": {"login": "maintainer"}, "body": "Pending draft.", + "commit": {"oid": head}, "state": "PENDING", + "submittedAt": "2026-08-18T10:00:00Z"}, + ]}, reviewer_login="maintainer") + assert result["valid"] is (head == approved_head) + assert result["state"] == "APPROVED" + if head != approved_head: + assert "review_not_bound_to_current_head" in result["invalid_reasons"] + + def test_actionable_sequence_excludes_valid_merged_exact_head(monkeypatch) -> None: monkeypatch.setattr(pr_review_module, "_now_iso", lambda: "2026-08-18T12:00:00Z") reviewed = _queue_pr(