fix(pr-review): read effective approvals across reviewer identities

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})

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<ReviewState>(["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<number>();
   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<number, JsonObject>();
+  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<string, Review>();
   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};
 }

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)