From 908b8cd8bc8465d71c76c687cd12baddb84ecb80 Mon Sep 17 00:00:00 2001 From: huangruiteng Date: Thu, 1 Oct 2026 23:14:54 +0800 Subject: [PATCH 1/2] fix(pr-review): reconcile obsolete blockers after exact-head approval Signed-off-by: huangruiteng --- .../pr_review_queue/approval_closeout.py | 102 ++++++++++++ .../pr_review_queue/review_contract.py | 5 +- loopx/cli_commands/pr_review.py | 17 ++ .../pr_review_approval_closeout.ts | 68 ++++++++ .../control_plane/effect_runtime_handlers.ts | 2 + .../project_registry_io_manifest_v1.json | 2 +- skills/loopx-pr-review/SKILL.md | 20 +-- .../test_pr_review_approval_closeout.py | 145 ++++++++++++++++++ 8 files changed, 349 insertions(+), 12 deletions(-) create mode 100644 loopx/capabilities/pr_review_queue/approval_closeout.py create mode 100644 loopx/control_plane/capabilities/pr_review_approval_closeout.ts create mode 100644 tests/capabilities/test_pr_review_approval_closeout.py diff --git a/loopx/capabilities/pr_review_queue/approval_closeout.py b/loopx/capabilities/pr_review_queue/approval_closeout.py new file mode 100644 index 0000000000..b84093fd5e --- /dev/null +++ b/loopx/capabilities/pr_review_queue/approval_closeout.py @@ -0,0 +1,102 @@ +"""GitHub read adapter for the typed post-approval reconciliation read model. + +No dismissal executor lives here: candidate age is not finding-resolution proof. +""" +from __future__ import annotations + +from typing import Any + +from ...control_plane.effect_runtime import EffectRuntimeRejected, effect_runtime_result + + +def approval_closeout_contract() -> dict[str, Any]: + return { + "required_after": "published_exact_head_approve_readback", + "readback_command": "loopx --format json pr-review --repo OWNER/REPO --check-approval-closeout NUMBER@HEAD_OID", + "old_commit_proves_resolution": False, + "grants_dismissal_or_merge_authority": False, + "dismissal_requires": [ + "all_prior_findings_verified_resolved", "github_permission_and_owner_authority", + "fresh_unchanged_head_approval_and_effective_target", + ], + "readback_requires": ["target_dismissed_approval_preserved_head_unchanged", "remaining_blockers_reported_truthfully"], + "procedure": [ + "After publishing/readback of APPROVE (including the author-owned COMMENTED fallback), run readback_command. An existing exact-head APPROVE may use this compact closeout without a duplicate audit or review.", + "For every effective blocking_reviews row, read its full review and inline comments. Independently map EVERY finding to current-head code and decisive validation. Old commit, resolved threads, another account's approval, or green CI alone never proves resolution; same-head findings may also need reconciliation.", + "Dismiss only findings verified resolved or independently disproven, with explicit owner authorization for review reconciliation and actual repository/branch dismissal permission. Preserve unresolved or unverified reviews. A COMMENTED self-approval is not GitHub approval or authority over another reviewer.", + "Immediately before each dismissal, re-read this plan and target review/comments; stop if the head, approval, target, or findings changed. Use GitHub's native review dismissal, never deletion: gh api --method PUT repos/OWNER/REPO/pulls/NUMBER/reviews/REVIEW_ID/dismissals -f message='PUBLIC_SAFE_FINDING_RESOLUTION_EVIDENCE'. Retain discussion and include evidence in the required dismissal message.", + "Read the target back as DISMISSED, verify the approval remains at the unchanged head, and rerun closeout. Report remaining blockers and raw reviewDecision (null is not APPROVED). On a hold, permission failure, or unresolved finding, preserve the earned APPROVE and report the separate closeout/merge hold; do not merge or erase dissent.", + ], + } + + +def plan_approval_closeout(request: dict[str, Any]) -> dict[str, Any]: + try: + result = effect_runtime_result("capabilities.pr_review.approval_closeout.plan", request) + except EffectRuntimeRejected as error: + raise ValueError(str(error)) from error + if not isinstance(result, dict) or result.get("schema_version") != "pull_request_review_approval_closeout_v0": + raise TypeError("typed approval closeout result mismatch") + result["execution_contract"] = approval_closeout_contract() + return result + + +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 ( + BEHAVIORAL_POLICY_AREAS, CODE_AREAS, _files, _review_conclusion, + resolve_current_github_login, + ) + from .selection_execution import normalize_fresh_audit_exact_heads + + targets = normalize_fresh_audit_exact_heads([exact_head]) + exact_head = next(iter(targets)) + number = int(exact_head.split("@", 1)[0]) + login = resolve_current_github_login() + if not login: + raise ValueError("approval closeout requires authenticated reviewer identity") + fields = "number,headRefOid,state,reviewDecision,author,files,changedFiles" + args = ["pr", "view", str(number), "--repo", repository, "--json", fields] + pr = run_gh_json(args) + if not isinstance(pr, dict): + raise ValueError("pull-request readback is incomplete") + expected_files = pr.get("changedFiles") + if not isinstance(expected_files, int) or expected_files < 0: + raise ValueError("changed-file readback is incomplete") + if not isinstance(pr.get("files"), list) or len(pr["files"]) != expected_files: + pr["files"] = _fetch_complete_pr_files(repository=repository, number=str(number), + expected_count=expected_files, cwd=None, run_gh_json=run_gh_json) + if pr["files"] is None: + raise ValueError("changed-file readback is incomplete") + if any(not isinstance(row, dict) or not isinstance(row.get("path"), str) + or not row["path"].strip() for row in pr["files"]): + raise ValueError("changed-file readback is malformed") + pages = run_gh_json(["api", "--paginate", "--slurp", + f"repos/{repository}/pulls/{number}/reviews?per_page=100"]) + if not isinstance(pages, list) or not pages or any(not isinstance(page, list) for page in pages): + raise ValueError("paginated review readback is incomplete") + 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") + 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}) diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index f42414e7c3..e627993a6f 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -5,9 +5,10 @@ from typing import Any from .review_body import REQUIRED_FINAL_SECTIONS, review_body_requirements +from .approval_closeout import approval_closeout_contract # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 12 +REVIEW_POLICY_REVISION = 13 # A red check is an observation, not evidence that the reviewed PR caused it. # This contract belongs to review judgment; merge readiness still owns whether @@ -245,6 +246,7 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An return { "schema_version": "pull_request_review_execution_contract_v2", "policy_revision": REVIEW_POLICY_REVISION, + "approval_closeout": approval_closeout_contract(), "purpose": ( "Define the evidence that must exist before a detailed review verdict; " "host skills route this contract but must not reimplement it." @@ -1344,6 +1346,7 @@ def build_agent_response_contract(*, wait_for_ci: bool = True) -> dict[str, Any] "Do not infer verified evidence from title, labels, changed-file counts, metadata_risk_hint, or green CI alone.", ("Observe final CI in addition to repository-native local validation, then attribute red checks before judging this PR; review approval and merge readiness are separate." if wait_for_ci else "Do not fetch, poll, or wait for CI for review or merge readiness. repository_required_checks means repository-native local validation; attribute base-equivalent failures and keep missing affected-invariant evidence blocking."), "Recheck the exact remote head before verdict and publication.", + "After publishing and reading back APPROVE, execute review_execution_contract.approval_closeout; approval alone does not clear another reviewer's effective blocking review.", "Render the verified result through a non-null pull_requests[].review_template; host skills must not maintain a competing depth checklist.", ], } diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index c58fe40efb..5edf818fe2 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -112,6 +112,10 @@ def register_pr_review_command( "--check-result", help="Check a saved review result for verdict/evidence consistency; no GitHub writes.", ) + parser.add_argument( + "--check-approval-closeout", metavar="NUMBER@HEAD_OID", + help="Read effective blocking reviews after exact-head approval; no GitHub writes or merge authority.", + ) parser.add_argument( "--check-merge-readiness", metavar="NUMBER@HEAD_OID", @@ -260,6 +264,19 @@ def handle_pr_review_command( raise ValueError("PR review Goal was not found: " + goal_id) review_configuration = resolve_configuration(goal, machine_configuration) wait_for_ci = review_configuration["wait_for_ci"] + if getattr(args, "check_approval_closeout", None): + from ..capabilities.pr_review_queue.approval_closeout import read_github_approval_closeout + if any((args.check_result, args.packet, args.check_merge_readiness, args.fixture, + args.autonomous_observation, args.observation_state_file, + args.previous_observation_json, args.handled_exact_head, + args.projected_exact_head, args.since, args.fresh_audit_exact_head, target_exact_heads)): + raise ValueError("approval closeout cannot be combined with scan, fixture, result, or readiness options") + repository = args.repo or resolve_current_github_repository() + if not repository: + raise ValueError("approval closeout requires a GitHub repository") + payload = read_github_approval_closeout(repository=repository, exact_head=args.check_approval_closeout) + print_payload(payload, output_format(args), lambda value: json.dumps(value, indent=2)) + return 1 if payload["status"] == "hold" else 0 if goal_id: if runtime_root is None: raise ValueError("--goal-id requires an available runtime root") diff --git a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts new file mode 100644 index 0000000000..5c8764c562 --- /dev/null +++ b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts @@ -0,0 +1,68 @@ +/** PR-review-owned read model. Finding resolution and GitHub write authority + * cannot be inferred from an approval, commit age, or this candidate list. */ +import type {JsonObject} from "../effect_program.ts"; +import {requireJsonObject, requireNonEmptyString} from "../runtime_decode.ts"; +import {EffectRuntimeRequestError} from "../effect_runtime_errors.ts"; +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}; +const states = new Set(["PENDING", "COMMENTED", "APPROVED", "CHANGES_REQUESTED", "DISMISSED"]); +function fail(message: string): never { throw new EffectRuntimeRequestError(message); } +function oid(value: unknown): string { + const result = requireNonEmptyString(value, "review commit").toLowerCase(); + return /^(?:[a-f0-9]{40}|[a-f0-9]{64})$/.test(result) ? result : fail("review commit must be a full SHA"); +} + +export function planPrReviewApprovalCloseout(value: unknown): JsonObject { + const request = requireJsonObject(value, "approval closeout"); + const exact = requireNonEmptyString(request.expected_exact_head, "expected_exact_head"); + 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 => { + const row = requireJsonObject(value, "review"); + const id = row.id; + if (typeof id !== "number" || !Number.isSafeInteger(id) || id <= 0 || seen.has(id)) { + return fail("review id must be unique and positive"); + } + seen.add(id); + const login = requireNonEmptyString(requireJsonObject(row.user, "review user").login, "reviewer login"); + const state = row.state as ReviewState; + if (!states.has(state)) return fail("unknown GitHub review state"); + // Pending reviews have no submitted_at and cannot clear a submitted opinion. + const parsed = state === "PENDING" ? new Date(0) : parseIsoTimestamp(requireNonEmptyString(row.submitted_at, "submitted_at")); + 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, + url: requireNonEmptyString(row.html_url, "review URL")}; + }); + 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 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]})); + if (request.reviews_complete === true && !blockers.length && pr.reviewDecision === "CHANGES_REQUESTED") { + holds.push("aggregate_review_decision_conflict"); + } + return {schema_version: "pull_request_review_approval_closeout_v0", + repository: requireNonEmptyString(request.repository, "repository"), exact_head: exact, + 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}, + dismissal_authorized: false, merge_authorized: false, github_write_performed: false}; +} diff --git a/loopx/control_plane/effect_runtime_handlers.ts b/loopx/control_plane/effect_runtime_handlers.ts index 9d99d39966..bca9a6e674 100644 --- a/loopx/control_plane/effect_runtime_handlers.ts +++ b/loopx/control_plane/effect_runtime_handlers.ts @@ -10,6 +10,7 @@ import {readCanonicalSnapshotPage} from "./coordination/canonical_snapshot_page. import {manageLocalAuthorityArchive} from "./coordination/local_authority_archive.ts"; import {selectPeriodicReportProgress, selectPeriodicReportApprovalRetry} from "./capabilities/periodic_report_progress.ts"; import {planIssueFixMonitorReconciliation} from "./capabilities/issue_fix_monitor_reconciliation.ts"; +import {planPrReviewApprovalCloseout} from "./capabilities/pr_review_approval_closeout.ts"; import {projectPeerOrchestration} from "./quota/peer_orchestration.ts"; import {inspectTaskLease} from "./work_items/task_lease_inspection.ts"; import {evaluateTodoPriority} from "./todos/priority.ts"; @@ -639,6 +640,7 @@ export function createEffectRuntimeHandlers( ["scheduler.monitor_successor.plan", planMonitorSuccessor], ["scheduler.monitor_target.select", selectMonitorTodoRequest], ["capabilities.issue_fix.monitor_reconciliation.plan", planIssueFixMonitorReconciliation], + ["capabilities.pr_review.approval_closeout.plan", planPrReviewApprovalCloseout], ["coordination.local_authority_shadow.record", recordLocalAuthorityShadow], ["coordination.runtime_shadow.commit_entry", deliverShadowEntry], ["coordination.runtime_shadow.outbox_read", readLocalAuthorityShadow], diff --git a/loopx/semantics/project_registry_io_manifest_v1.json b/loopx/semantics/project_registry_io_manifest_v1.json index e62770dae5..a23ea98670 100644 --- a/loopx/semantics/project_registry_io_manifest_v1.json +++ b/loopx/semantics/project_registry_io_manifest_v1.json @@ -743,7 +743,7 @@ }, { "site": "loopx/cli_commands/pr_review.py::.handle_pr_review_command::codec_read:load_registry#1", - "line": 258, + "line": 262, "column": 39, "kind": "codec_read", "api": "load_registry", diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index 440bd17d90..75c6df7ce3 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -89,8 +89,7 @@ When `review_action_kind` is null, the row stays in `pull_requests` inventory bu missing material evidence needs a concrete request-changes reason. 4. Publish the checked `review_body`; recheck after edits. Remote readback uses the same body rules. Headings and a verdict alone cannot certify a review. -5. Re-read the remote head immediately before verdict and publication. Restart - the evidence pass if it changed. +5. Re-read the remote head immediately before verdict and publication; restart the evidence pass if it changed. Each PR needs independent evidence and a standalone card; a queue table is only a preface. @@ -106,14 +105,15 @@ requested local-only/dry-run output or the finding is private or security-sensit when the account is the author and self-approval is rejected, record the same conclusion as a `COMMENTED` review titled `Approval conclusion (author-owned PR; GitHub blocks formal self-approval)`. - Non-blocking P2 suggestions: still `APPROVE`; keep them in the body. -- Merged PR: publish a post-merge audit comment only for a new actionable - finding; avoid duplicating an equivalent exact-head result. - -Build public text from the exact reviewed head. Remove local paths, private -context, raw logs, credentials, and internal-only links. Read the published -review back, verify its state and rendered body, and return its URL. Merge -still routes through `loopx-pr-merge`; an `APPROVE` is not merge authority. -Do not leave a public blocker only in chat. +- Merged PR: publish a post-merge audit comment only for a new actionable finding; + avoid duplicating an equivalent exact-head result. + +Build public text from the exact reviewed head; remove local paths, private context, +raw logs, credentials, and internal-only links. Read the published review back, +verify state/body and return its URL. After APPROVE (also existing approval), execute +`review_execution_contract.approval_closeout`; reconcile only verified obsolete +blockers with owner/GitHub authority, preserving discussion and unresolved reviews. +Merge routes through `loopx-pr-merge`; an `APPROVE` is not merge authority. Immediately before every merge, run `loopx --format json pr-review --goal-id GOAL --repo OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`; require `ready=true`. diff --git a/tests/capabilities/test_pr_review_approval_closeout.py b/tests/capabilities/test_pr_review_approval_closeout.py new file mode 100644 index 0000000000..b74d60f1a0 --- /dev/null +++ b/tests/capabilities/test_pr_review_approval_closeout.py @@ -0,0 +1,145 @@ +"""Approval is not proof that another reviewer's findings were resolved.""" +from __future__ import annotations + +import pytest +import json +from pathlib import Path + +from loopx.capabilities.pr_review_queue.approval_closeout import plan_approval_closeout +from loopx.capabilities.pr_review_queue import build_agent_response_contract + +HEAD, OLD = "a" * 40, "b" * 40 + + +def review(identity, state, *, login="other", head=OLD): + return {"id": identity, "state": state, "user": {"login": login}, + "commit_id": head, "submitted_at": f"2026-10-01T00:00:{identity:02d}Z", + "html_url": f"https://github.com/owner/repo/pull/42#pullrequestreview-{identity}"} + + +def request(reviews): + 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"}} + + +def test_other_reviewer_blocker_survives_own_approval_and_comments(): + result = plan_approval_closeout(request([ + review(1, "CHANGES_REQUESTED"), review(2, "COMMENTED"), + review(3, "APPROVED", login="maintainer", head=HEAD), + ])) + assert result["status"] == "verification_required" + assert [row["review_id"] for row in result["blocking_reviews"]] == [1] + assert result["blocking_reviews"][0]["on_approved_head"] is False + assert result["dismissal_authorized"] is False + assert result["github_write_performed"] is False + + +@pytest.mark.parametrize("superseding", ["APPROVED", "DISMISSED"]) +def test_superseded_history_is_not_resurrected(superseding): + data = request([ + review(1, "CHANGES_REQUESTED"), review(2, superseding), + ]) + data["pull_request"]["reviewDecision"] = None + result = plan_approval_closeout(data) + assert result["status"] == "clear" + assert result["blocking_reviews"] == [] + data["reviews"].reverse() + assert result == plan_approval_closeout(data) + + +def test_aggregate_history_conflict_is_not_a_clear_closeout(): + result = plan_approval_closeout(request([review(1, "DISMISSED")])) + assert result["status"] == "hold" + assert "aggregate_review_decision_conflict" in result["hold_reasons"] + + +def test_only_latest_blocker_per_reviewer_needs_reconciliation(): + result = plan_approval_closeout(request([ + review(1, "CHANGES_REQUESTED"), review(2, "CHANGES_REQUESTED", head=HEAD), + review(3, "PENDING"), review(4, "CHANGES_REQUESTED", login="third"), + ])) + assert [row["review_id"] for row in result["blocking_reviews"]] == [2, 4] + # A newer/same-head review cannot silently be classified as resolved either. + assert result["blocking_reviews"][0]["on_approved_head"] is True + assert result["dismissal_authorized"] is False + + +@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"), +]) +def test_unverified_closeout_is_a_hold_not_clear(field, value, reason): + data = request([review(1, "CHANGES_REQUESTED")]) + data[field] = value + result = plan_approval_closeout(data) + assert result["status"] == "hold" + assert reason in result["hold_reasons"] + assert result["dismissal_authorized"] is False + + +@pytest.mark.parametrize("change", [ + {"state": "UNKNOWN"}, {"user": {}}, {"id": None}, + {"submitted_at": "not-a-date"}, {"commit_id": ""}, +]) +def test_malformed_history_fails_closed(change): + row = review(1, "CHANGES_REQUESTED") | change + with pytest.raises(ValueError): + plan_approval_closeout(request([row])) + + +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"] + assert closeout["required_after"] == "published_exact_head_approve_readback" + assert closeout["old_commit_proves_resolution"] is False + assert closeout["grants_dismissal_or_merge_authority"] is False + assert "all_prior_findings_verified_resolved" in closeout["dismissal_requires"] + assert "github_permission_and_owner_authority" in closeout["dismissal_requires"] + assert "target_dismissed_approval_preserved_head_unchanged" in closeout["readback_requires"] + + +def test_live_adapter_paginates_history_without_ci_or_mutations(monkeypatch, capsys): + from loopx.cli import main + import loopx.pr_review as pr_module + from loopx.capabilities.pr_review_queue import github_source + + body = (Path(__file__).parents[2] / "examples/fixtures/pr-review.body.md").read_text() + body = body.replace("HEAD_OID", HEAD).replace("VERDICT", "APPROVE") + own = review(3, "APPROVED", login="maintainer", head=HEAD) | {"body": body} + calls = [] + + def read(args, **_): + calls.append(args) + if args[0] == "api": + assert "--paginate" in args and "--slurp" in args + return [[review(1, "CHANGES_REQUESTED")], [own]] + assert "statusCheckRollup" not in args[-1] + return {"number": 42, "headRefOid": HEAD, "state": "OPEN", + "author": {"login": "contributor"}, "reviewDecision": "CHANGES_REQUESTED", + "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") + assert main(["--format", "json", "pr-review", "--repo", "owner/repo", + "--check-approval-closeout", f"42@{HEAD}"]) == 0 + result = json.loads(capsys.readouterr().out) + assert result["status"] == "verification_required" + assert result["blocking_reviews"][0]["review_id"] == 1 + assert len(calls) == 3 + assert all("--method" not in args for args in calls) + assert "body" not in json.dumps(result) + + +def test_cli_rejects_mixed_modes_before_github_read(monkeypatch, capsys): + from loopx.cli import main + from loopx.capabilities.pr_review_queue import github_source + + monkeypatch.setattr(github_source, "run_gh_json", lambda *_: pytest.fail("unexpected GitHub read")) + assert main(["--format", "json", "pr-review", "--repo", "owner/repo", + "--check-approval-closeout", f"42@{HEAD}", "--autonomous-observation"]) == 1 + assert "cannot be combined" in capsys.readouterr().out From e0f81e6ccd5b06fb459b5a9e85a24c25c256003b Mon Sep 17 00:00:00 2001 From: huangruiteng Date: Thu, 1 Oct 2026 23:15:09 +0800 Subject: [PATCH 2/2] docs(pr-review): explain approval closeout and dismissal boundaries Signed-off-by: huangruiteng --- loopx/capabilities/pr_review_queue/README.md | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 0b89d59ebd..57d0a995af 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -591,6 +591,28 @@ state transition, author-owned conclusions use `COMMENTED` plus one exact title: The compact result is versioned as `pull_request_review_conclusion_v0` and reports a typed verdict and invalid-reason codes. +After a published exact-head `APPROVE` is read back, the capability-owned +`review_execution_contract.approval_closeout` requires effective-review +reconciliation. This also works for an existing approval without a duplicate audit: + +```bash +loopx --format json pr-review --repo OWNER/REPO --check-approval-closeout NUMBER@HEAD_OID +``` + +The read-only command paginates GitHub review history and uses the latest submitted +opinion per reviewer; a comment/pending review does not erase a blocker, and a +dismissed review does not resurrect older history. Its typed TS read model reports +`clear`, `verification_required`, or `hold`; these are not merge decisions. +Review age or a different commit only identifies a finding to inspect, never proof +of resolution. The host independently verifies every old finding and inline comment, +checks owner authorization and GitHub/branch dismissal permissions, rechecks the +head/approval/target immediately before GitHub's native dismissal, and reads back +`DISMISSED`, retained approval, unchanged head, and any remaining blockers. +Unresolved/unverified reviews stay intact; a failed closeout does not revoke an +earned approval. The command never dismisses, deletes, fetches CI, or merges, and +raw review bodies remain transient. No new setting, UI, or automatic GitHub authority +is introduced; normal review/merge policy is unchanged except this post-approval step. + `pull_request_merge_readiness_v0` is a separate, read-only last-mile gate. It re-reads the named PR instead of trusting a saved review packet. In particular, GitHub may retain or reassociate an approval after an update-from-base commit;