Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions loopx/capabilities/pr_review_queue/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
102 changes: 102 additions & 0 deletions loopx/capabilities/pr_review_queue/approval_closeout.py
Original file line number Diff line number Diff line change
@@ -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})
5 changes: 4 additions & 1 deletion loopx/capabilities/pr_review_queue/review_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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."
Expand Down Expand Up @@ -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.",
],
}
17 changes: 17 additions & 0 deletions loopx/cli_commands/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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")
Expand Down
68 changes: 68 additions & 0 deletions loopx/control_plane/capabilities/pr_review_approval_closeout.ts
Original file line number Diff line number Diff line change
@@ -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<ReviewState>(["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<number>();
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<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 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};
}
2 changes: 2 additions & 0 deletions loopx/control_plane/effect_runtime_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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],
Expand Down
2 changes: 1 addition & 1 deletion loopx/semantics/project_registry_io_manifest_v1.json
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@
},
{
"site": "loopx/cli_commands/pr_review.py::<module>.handle_pr_review_command::codec_read:load_registry#1",
"line": 258,
"line": 262,
"column": 39,
"kind": "codec_read",
"api": "load_registry",
Expand Down
Loading
Loading