diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 3054b781b1..255a042d9c 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -1071,7 +1071,7 @@ def approved_open_head( "review_sequence_membership": "review_action_kind_non_null_only", "no_action_inventory_location": "pull_requests", "generic_rereview_terms_force_fresh_audit": False, - "no_action_behavior": "compact_exact_head_conclusion_readback_only", + "no_action_behavior": "compact_exact_head_review_or_lifecycle_readback_only", "no_action_execution_artifacts": "plan_and_template_null_commands_empty", "force_fresh_audit_requires": ( "An explicit request to rerun evidence despite the unchanged/no-action " diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 67f3ff7198..1de69ff2b2 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -842,6 +842,38 @@ relaxes body qualification nor grants dismissal or merge authority. 不改变已有正式意见,head 变化仍需新证据。本修复改变默认队列和 readiness 分类,不放宽正文校验,也不授予撤回评审或合并权限。 +The default queue also avoids duplicating a submitted independent review. A +complete, paginated GitHub history can establish `independent_review` when a +non-author, non-bot account with `OWNER`, `MEMBER`, `COLLABORATOR` or `CONTRIBUTOR` +association submitted `COMMENTED`, `APPROVED` or `CHANGES_REQUESTED` on the current +head. These are formal review events, including inline-only reviews, not issue +comments. The typed history owner uses each reviewer's latest submitted event; +pending reviews do not erase prior activity, and dismissal retires that actor's +coverage. Missing actor/association metadata, incomplete reads or a new head +cannot establish coverage. The live provider rechecks head/base after history. + +Covered rows remain in inventory with the reviewer, review URL/state and exact +head, a null action, null plan/template and no evidence commands. Read that +record once and continue the queue; the contributor need not write this agent's +bilingual format. This is work coverage, not a qualified approval or merge +permission. Existing dissent, strict approval-body qualification, closeout, +validation and merge-readiness gates remain. An explicit authorized +`--fresh-audit-exact-head NUMBER@HEAD_OID` can request new evidence despite +coverage. No reviewer-name whitelist, new configuration or automatic write is +introduced. + +默认队列也跳过已有独立评审覆盖的重复全文审查:完整分页的 GitHub 记录证明, +当前提交已由非作者、非机器人且身份为 OWNER、MEMBER、COLLABORATOR 或 +CONTRIBUTOR 的账号提交 COMMENTED、APPROVED 或 CHANGES_REQUESTED。这里指正式 +review(包括只有行内意见的评审),不包括普通 issue 评论。复用同一 TypeScript +历史 owner,按每个评审者最近的已提交记录判断;待提交不会抹去旧记录,撤销只 +撤回该账号的覆盖。身份未知、来源不完整或换头时不跳过,读取后还须核对 head/base。 +载荷的 `independent_review` 给出评审者、原记录链接、状态和精确提交;该行仍在 +inventory,但不生成可执行计划或证据命令。第三方无需使用本 agent 的双语格式。 +跳过重复工作不授予批准或合并资格,不清除阻塞意见;原批准正文、closeout、验证 +和 merge-readiness 门保留。明确要求重新取证时,沿原精确头 fresh-audit 入口执行。 +不新增人名白名单、配置或自动外部写入。 + 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: diff --git a/loopx/capabilities/pr_review_queue/approval_closeout.py b/loopx/capabilities/pr_review_queue/approval_closeout.py index f6324aa435..629bef44ed 100644 --- a/loopx/capabilities/pr_review_queue/approval_closeout.py +++ b/loopx/capabilities/pr_review_queue/approval_closeout.py @@ -41,8 +41,8 @@ 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.""" +def select_review_history(request: dict[str, Any]) -> dict[str, Any]: + """Transport bodies and provider provenance to the shared history owner.""" try: result = effect_runtime_result("capabilities.pr_review.conclusion.select", request) except EffectRuntimeRejected as error: @@ -50,11 +50,11 @@ def select_review_conclusion(request: dict[str, Any]) -> int: 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 + 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 .github_source import _fetch_complete_pr_files, fetch_complete_pr_reviews, run_gh_json from ...pr_review import ( BEHAVIORAL_POLICY_AREAS, CODE_AREAS, _files, _review_conclusion, resolve_current_github_login, @@ -83,13 +83,8 @@ def read_github_approval_closeout(*, repository: str, exact_head: str) -> dict[s 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") + reviews = fetch_complete_pr_reviews(repository=repository, number=str(number), + cwd=None, run_gh_json=run_gh_json) # 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)) diff --git a/loopx/capabilities/pr_review_queue/github_source.py b/loopx/capabilities/pr_review_queue/github_source.py index a4eaf8e897..6a41fb21a0 100644 --- a/loopx/capabilities/pr_review_queue/github_source.py +++ b/loopx/capabilities/pr_review_queue/github_source.py @@ -222,6 +222,63 @@ def same_snapshot() -> bool: return None +def fetch_complete_pr_reviews(*, repository: str, number: str, cwd: Path | None, + run_gh_json: GitHubJsonRunner) -> list[dict[str, Any]]: + """One complete provider history for queue coverage and approval closeout.""" + pages = run_gh_json(["api", "--paginate", "--slurp", + f"repos/{repository}/pulls/{number}/reviews?per_page=100"], + **({"cwd": cwd} if cwd is not None else {})) + 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") + return reviews + + +def _attach_review_provenance(row: dict[str, Any], details: dict[str, Any], *, + repository: str, number: str, cwd: Path | None, + run_gh_json: GitHubJsonRunner) -> bool: + author = row.get("author") + author = author.get("login") if isinstance(author, dict) else None + # This only avoids unnecessary provider reads. Eligibility belongs to TS; + # no association, state or actor-type policy is duplicated here. + if not author or not any(isinstance(review, dict) + and isinstance(review.get("author"), dict) and review["author"].get("login") + and review["author"]["login"].casefold() != author.casefold() + for review in details["reviews"]): + return True + try: + reviews = fetch_complete_pr_reviews(repository=repository, number=number, + cwd=cwd, run_gh_json=run_gh_json) + normalized = [] + for review in reviews: + user = review.get("user") or {} + if (not isinstance(user, dict) or not isinstance(user.get("login"), str) + or type(review.get("id")) is not int or any( + not isinstance(review.get(key), str) + for key in ("state", "commit_id", "html_url"))): + raise ValueError("review provenance readback is malformed") + normalized.append({"id": review.get("id"), "url": review.get("html_url"), + "author": {"login": user.get("login"), "is_bot": {"User": False, "Bot": True}.get(user.get("type"))}, + "authorAssociation": review.get("author_association"), "state": review.get("state"), + "submittedAt": review.get("submitted_at"), "commit": {"oid": review.get("commit_id")}, + "body": review.get("body")}) + after = run_gh_json(["pr", "view", number, "--json", "headRefOid,baseRefOid", + "--repo", repository], cwd=cwd) + if not isinstance(after, dict) or any(after.get(key) != row.get(key) + for key in ("headRefOid", "baseRefOid")): + return False + except Exception: + # Unknown coverage is not a reason to skip work. Retain the ordinary + # versioned detail read and its existing body/conclusion qualification. + row["reviews_complete"] = False + return True + details["reviews"] = normalized + row["reviews_complete"] = True + return True + + def attach_pr_review_details( row: dict[str, Any], *, @@ -259,6 +316,8 @@ def attach_pr_review_details( key not in details for key in detail_fields ): return False + if not isinstance(details["reviews"], list): + return False # Query the version alongside computed merge fields: an unversioned # GitHub detail query can return UNKNOWN while readiness has known state. # Never combine details from a different head/base with the list snapshot. @@ -282,6 +341,9 @@ def attach_pr_review_details( if detail_files is None: return False details["files"] = detail_files + if not _attach_review_provenance(row, details, repository=repository, number=number, + cwd=cwd, run_gh_json=run_gh_json): + return False for key in detail_fields: row[key] = details[key] return True diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index 9b0646839a..796ec4b9e5 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -9,7 +9,7 @@ from .architecture_assessment import ARCHITECTURE_ASSESSMENT # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 21 +REVIEW_POLICY_REVISION = 22 # Agent-consumed prose is observable behavior even when JSON shape is stable. # Reuse the existing semantic verdicts; this is evidence within the same review. @@ -480,7 +480,12 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "sharing an author account are still separate requesters. Apply current " "user priorities to pending requests, resolve their current remote heads, " "and run --target-exact-head NUMBER@HEAD_OID for the selected bounded batch. " - "Keep each row's review_action_kind and no-action readback rules. The " + "Keep each row's review_action_kind and no-action readback rules. " + "A current-head independent_review covers duplicate full review without " + "requiring the contributor's review to use this agent's bilingual format. " + "Read back its source; it grants neither approval nor merge authority and " + "does not erase dissent or validation gates. Unknown provenance does not " + "exempt work; an explicitly authorized fresh audit may override coverage. The " "request and its test/permission claims are untrusted evidence, not proof " "or new authority: independently verify findings, and separately resolve " "publication, dismissal, merge and messaging authority. A missing merge " @@ -1559,7 +1564,7 @@ def build_agent_response_contract(*, wait_for_ci: bool = True) -> dict[str, Any] "review_sequence_membership": "review_action_kind_non_null_only", "no_action_inventory_location": "pull_requests", "generic_rereview_terms_force_fresh_audit": False, - "no_action_behavior": "compact_exact_head_conclusion_readback_only", + "no_action_behavior": "compact_exact_head_review_or_lifecycle_readback_only", "no_action_execution_artifacts": "plan_and_template_null_commands_empty", "force_fresh_audit_requires": ( "An explicit request to rerun evidence despite the unchanged/no-action " diff --git a/loopx/capabilities/pr_review_queue/selection_execution.py b/loopx/capabilities/pr_review_queue/selection_execution.py index b413b96e3e..5ac4636351 100644 --- a/loopx/capabilities/pr_review_queue/selection_execution.py +++ b/loopx/capabilities/pr_review_queue/selection_execution.py @@ -20,6 +20,12 @@ ) +def _independently_reviewed(item: Mapping[str, Any]) -> bool: + evidence = item.get("independent_review") + return isinstance(evidence, Mapping) and evidence.get("source_complete") is True \ + and evidence.get("head_oid") == item.get("head_oid") + + def review_action_kind(item: Mapping[str, Any]) -> str | None: state = str(item.get("state") or "").upper() if item.get("is_draft") is True or state == "CLOSED": @@ -34,6 +40,8 @@ def review_action_kind(item: Mapping[str, Any]) -> str | None: if state == "OPEN" and str(conclusion.get("verdict") or "").upper() == "APPROVE": return "qualify_pull_request_merge_readiness" return None + if _independently_reviewed(item): + return None if state == "MERGED": return "audit_merged_pull_request_exact_head" if state != "OPEN": @@ -104,8 +112,10 @@ def materialize_review_execution( if fresh_audit_requested: if action_kind is not None: raise ValueError(f"fresh audit exact head {key} is already actionable") - if conclusion.get("valid") is not True: - raise ValueError(f"fresh audit exact head {key} requires a valid prior conclusion") + peer_coverage = _independently_reviewed(item) and item.get("is_draft") is not True \ + and str(item.get("state") or "").upper() in {"OPEN", "MERGED"} + if conclusion.get("valid") is not True and not peer_coverage: + raise ValueError(f"fresh audit exact head {key} requires a valid prior conclusion or independent review") action_kind = "audit_pull_request_exact_head" result: dict[str, Any] = { @@ -123,6 +133,11 @@ def materialize_review_execution( instruction = "This PR is closed: read back lifecycle state and any recorded conclusion." elif conclusion.get("valid") is True: instruction = "Read back the existing exact-head conclusion." + elif _independently_reviewed(item): + peer = item["independent_review"] + instruction = (f"The current exact head has an independent contributor/maintainer review by " + f"{peer['reviewer']} ({peer['state']}): {peer['url']}. Read it back and skip duplicate " + "full review. This grants no approval or merge authority; preserve blockers and validation.") else: instruction = "Read back lifecycle state; no valid exact-head conclusion is established." return result | { diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index c4435ff713..3722b372db 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -185,7 +185,7 @@ def register_pr_review_command( metavar="NUMBER@HEAD_OID", help=( "Explicitly request a fresh evidence audit for one unchanged exact head " - "that already has a valid conclusion. Repeatable." + "that already has a valid conclusion or independent review. Repeatable." ), ) parser.add_argument( diff --git a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts index 7958601332..a168684624 100644 --- a/loopx/control_plane/capabilities/pr_review_approval_closeout.ts +++ b/loopx/control_plane/capabilities/pr_review_approval_closeout.ts @@ -10,6 +10,8 @@ type ReviewState = "PENDING" | "COMMENTED" | "APPROVED" | "CHANGES_REQUESTED" | 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"]); +// GitHub associations, not a reviewer-name allowlist or evidence of approval. +const repositoryReviewers = new Set(["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR"]); function fail(message: string): never { throw new EffectRuntimeRequestError(message); } function oid(value: unknown): string { const result = requireNonEmptyString(value, "review commit").toLowerCase(); @@ -30,6 +32,40 @@ function effectiveOpinions(reviews: readonly T[], author: str return [...latest.values()]; } +/** Submitted review activity covers duplicate work, independently of whether + * its body earns a qualified approval. Comments here are formal review events; + * issue comments do not enter this history. Dismissal retires only that actor. */ +function independentReview(reviews: readonly (Opinion & {source: JsonObject})[], + complete: boolean, author: string | null, head: string): JsonObject | undefined { + if (!complete || !author) return; + const latest = new Map(); + const seen = new Set(); + for (const row of reviews) { + const source = row.source; + const id = source.review_id; + if (typeof id !== "number" || !Number.isSafeInteger(id) || id <= 0 || seen.has(id) + || !row.login || row.login !== row.login.trim()) return; + seen.add(id); + if (row.state === "PENDING") continue; + const submitted = typeof source.submitted_at === "string" ? parseIsoTimestamp(source.submitted_at) : null; + if (!submitted || !/^(?:[a-f0-9]{40}|[a-f0-9]{64})$/.test(row.head)) return; + const candidate = {...row, id, time: submitted.getTime()}; + const previous = latest.get(row.login); + if (!previous || candidate.time > previous.time || candidate.time === previous.time && id > previous.id) { + latest.set(row.login, candidate); + } + } + const selected = [...latest.values()].filter(row => row.login !== author && row.head === head + && row.state !== "DISMISSED" && row.source.is_bot === false + && typeof row.source.association === "string" && repositoryReviewers.has(row.source.association) + && typeof row.source.url === "string" + && row.source.url.trim()).sort((a, b) => b.time - a.time || b.id - a.id)[0]; + if (selected) return {reviewer: selected.login, association: selected.source.association, + state: selected.state, head_oid: head, review_id: selected.id, url: selected.source.url, + submitted_at: selected.source.submitted_at, source_complete: true, + grants_approval: false, grants_merge_authority: false}; +} + /** 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 { @@ -37,7 +73,7 @@ export function selectPrReviewConclusion(value: unknown): JsonObject { 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 reviews = 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"); @@ -46,7 +82,7 @@ export function selectPrReviewConclusion(value: unknown): JsonObject { // 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}; + return {id, login: row.login.toLowerCase(), state, head: row.head.toLowerCase(), time: row.time, source: row}; }); const author = typeof request.author === "string" ? request.author.toLowerCase() : null; const head = requireNonEmptyString(request.head, "current head").toLowerCase(); @@ -58,7 +94,8 @@ export function selectPrReviewConclusion(value: unknown): JsonObject { || 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}; + const independent = independentReview(reviews, request.reviews_complete === true, author, head); + return {index: history.length - selected.id, ...(independent ? {independent_review: independent} : {})}; } export function planPrReviewApprovalCloseout(value: unknown): JsonObject { diff --git a/loopx/pr_review.py b/loopx/pr_review.py index a3ec8e4fe7..65e5df599e 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -861,17 +861,23 @@ def evaluate(review: Mapping[str, Any]) -> dict[str, Any]: } conclusions = [evaluate(review) for review in reviews] - if len(reviews) == 1: + if len(reviews) == 1 and pr.get("reviews_complete") is not True: return conclusions[0] - from .capabilities.pr_review_queue.approval_closeout import select_review_conclusion - index = select_review_conclusion({"head": head_oid, "author": pr_author, + from .capabilities.pr_review_queue.approval_closeout import select_review_history + selection = select_review_history({"head": head_oid, "author": pr_author, + "reviews_complete": pr.get("reviews_complete") is True, "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")), + "review_id": review.get("id"), "url": review.get("url"), + "association": review.get("authorAssociation"), + "is_bot": _as_dict(review.get("author")).get("is_bot"), + "submitted_at": review.get("submittedAt"), "conclusion": conclusion} for review, conclusion in zip(reviews, conclusions)]}) - return conclusions[index] + return conclusions[selection["index"]] | ({"independent_review": selection["independent_review"]} + if "independent_review" in selection else {}) def _review_sequence_entry(item: dict[str, Any], *, rank: int) -> dict[str, Any]: @@ -936,6 +942,7 @@ def _normalize_pr( ) conclusion = _review_conclusion(pr, reviewer_login=reviewer_login, behavior_bearing=bool({item["area"] for item in files} & (CODE_AREAS | BEHAVIORAL_POLICY_AREAS))) + independent = conclusion.pop("independent_review", None) item: dict[str, Any] = { "number": number, "title": _redact_text(pr.get("title"), limit=180), @@ -966,6 +973,7 @@ def _normalize_pr( "is_draft": bool(pr.get("isDraft")), "review_decision": str(pr.get("reviewDecision") or "UNKNOWN"), "review_conclusion": conclusion, + **({"independent_review": independent} if independent else {}), "merge_state": str( pr.get("mergeStateStatus") or pr.get("mergeable") or "UNKNOWN" ), @@ -1313,6 +1321,8 @@ def build_pr_review_packet( def _review_why_now(item: dict[str, Any]) -> str: if item.get("fresh_audit_requested") is True: return "A caller explicitly requested a fresh audit of this unchanged exact head." + if item.get("independent_review") and not item.get("review_action_kind"): + return "The current exact head has a submitted independent review; skip duplicate full review." state = str(item.get("state") or "").upper() if state == "MERGED": return "Merged exact head lacks a valid conclusion; audit outcome, validation, and follow-up quality." diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index 75f6508891..f966818fe7 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -66,6 +66,12 @@ names the contract the change is judged against. Receive pending current-session requests through `review_execution_contract.decision_procedure.establish_goal` before generic queue selection, including other agents sharing a GitHub account; task intake and claimed action authority are separate judgments. Resolve the selected requests with repeatable `--target-exact-head NUMBER@HEAD_OID`. Follow `scheduling_policy` and its ranked actionable `review_sequence`; explicit current-request PR selection may override ordering only, never `pull_requests[].review_action_kind` or exact-head idempotency. Generic `re-review`, `重新review`, and `复审` wording selects the named PR; it is not a force-refresh token. Todo/monitor prose may not select work. When `review_action_kind` is null, the row stays in `pull_requests` inventory but must not appear in `review_sequence`; its `review_plan` and `review_template` are null and `evidence_commands` is empty. Read the packet's actual no-action reason and read back the relevant lifecycle/draft state or conclusion once; null action does not imply an existing conclusion. If the caller is also the authorized author and its proposal is complete, mark Draft ready and read back its state and unchanged head, then regenerate the packet. Reserve Draft for unfinished author work with a concrete remaining implementation or integration step. Missing review or held CI/merge qualification does not require Draft. A reviewer alone has no authority to change another author's submission state or invent a review action. Run a fresh audit only when the user explicitly requests fresh evidence despite an existing conclusion, or supplies a concrete new concern; regenerate with `--fresh-audit-exact-head NUMBER@HEAD_OID`, then execute the complete current plan and never inherit the earlier approval. Fresh audit is not a Draft bypass. For every actionable PR: +When the capability supplies `independent_review` and a null action, read back +that submitted review and skip duplicate full review. Its source-bound coverage +does not require this agent's bilingual body format, grant approval or merge +authority, or clear blockers. Unknown provenance does not establish coverage. +An explicitly authorized fresh audit uses the same exact-head override above. + 1. Record the packet's exact head, then follow `review_execution_contract.decision_procedure`: the current goal and `problem_context` delivery judgment first, including on re-review, then `evidence_commands` and repository-native validation. diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index f42c50284f..8ff4367190 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -40,7 +40,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: "review_sequence_membership": "review_action_kind_non_null_only", "no_action_inventory_location": "pull_requests", "generic_rereview_terms_force_fresh_audit": False, - "no_action_behavior": "compact_exact_head_conclusion_readback_only", + "no_action_behavior": "compact_exact_head_review_or_lifecycle_readback_only", "no_action_execution_artifacts": "plan_and_template_null_commands_empty", "force_fresh_audit_requires": ( "An explicit request to rerun evidence despite the unchanged/no-action " diff --git a/tests/capabilities/test_pr_review_independent_review.py b/tests/capabilities/test_pr_review_independent_review.py new file mode 100644 index 0000000000..f2dabe02db --- /dev/null +++ b/tests/capabilities/test_pr_review_independent_review.py @@ -0,0 +1,227 @@ +"""An independent submitted review avoids duplicate work, never earns approval.""" +from __future__ import annotations + +from copy import deepcopy + +import pytest + +from loopx.pr_review import build_pr_review_packet, render_pr_review_markdown + +HEAD, OLD = "a" * 40, "b" * 40 + + +def review(identity=1, *, login="contributor", state="COMMENTED", head=HEAD, + association="CONTRIBUTOR", is_bot=False): + return {"id": identity, "url": f"https://github.com/owner/repo/pull/42#pullrequestreview-{identity}", + "author": {"login": login, "is_bot": is_bot}, "authorAssociation": association, + "state": state, "commit": {"oid": head}, "body": "See inline feedback.", + "submittedAt": f"2026-10-01T00:00:{identity:02d}Z"} + + +def raw_pr(reviews, *, complete=True, head=HEAD, state="OPEN", draft=False): + return { + "number": 42, "title": "Preserve the runtime contract", "state": state, + "body": "Keep the original runtime behavior.", "changedFiles": 1, + "author": {"login": "author"}, "headRefOid": head, "baseRefOid": "c" * 40, + "isDraft": draft, "createdAt": "2026-10-01T00:00:00Z", + "files": [{"path": "src/runtime.py", "additions": 2, "deletions": 1}], + "reviews": reviews, "reviews_complete": complete, + "reviewDecision": "CHANGES_REQUESTED", "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", "commits": [], + } + + +def packet(reviews, *, complete=True, head=HEAD, state="OPEN", draft=False, fresh=False): + return build_pr_review_packet(pull_requests=[raw_pr(reviews, complete=complete, + head=head, state=state, draft=draft)], + repository="owner/repo", source="fixture", limit=10, state_filter="all", + reviewer_login="agent", wait_for_ci=False, + fresh_audit_exact_heads=[f"42@{head}"] if fresh else []) + + +@pytest.mark.parametrize("state", ["COMMENTED", "APPROVED", "CHANGES_REQUESTED"]) +@pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR"]) +def test_submitted_independent_review_covers_work_without_our_body_format(state, association): + data = packet([review(state=state, association=association)]) + row = data["pull_requests"][0] + assert row["review_action_kind"] is None + assert data["review_sequence"] == [] and data["summary"]["review_attention_count"] == 0 + assert row["review_plan"] is row["review_template"] is None + assert row["evidence_commands"] == [] + evidence = row["independent_review"] + assert evidence["reviewer"] == "contributor" and evidence["head_oid"] == HEAD + assert evidence["association"] == association and evidence["state"] == state + assert evidence["review_id"] == 1 and "pullrequestreview-1" in evidence["url"] + assert evidence["grants_approval"] is evidence["grants_merge_authority"] is False + assert row["review_conclusion"]["valid"] is False + assert row["review_decision"] == "CHANGES_REQUESTED" + assert "independent" in row["review_goal"].lower() + assert "contributor" in render_pr_review_markdown(data) + + +@pytest.mark.parametrize("change", [ + {"author": {"login": "AUTHOR", "is_bot": False}}, + {"author": {"login": "contributor", "is_bot": True}}, + {"author": {"login": "contributor"}}, + {"authorAssociation": "NONE"}, {"authorAssociation": "FIRST_TIME_CONTRIBUTOR"}, + {"authorAssociation": None}, {"state": "PENDING"}, {"state": "DISMISSED"}, + {"authorAssociation": {"role": "OWNER"}}, + {"author": {"login": " author ", "is_bot": False}}, + {"commit": {"oid": OLD}}, {"submittedAt": None}, {"submittedAt": "not-a-date"}, +]) +def test_unqualified_review_does_not_suppress_full_review(change): + row = packet([review() | change])["pull_requests"][0] + assert row["review_action_kind"] == "rereview_pull_request_exact_head" + assert row["review_plan"] is not None and row["evidence_commands"] + assert "independent_review" not in row + + +def test_incomplete_review_source_does_not_suppress_or_invent_a_conclusion(): + data = packet([review()], complete=False) + row = data["pull_requests"][0] + assert row["review_action_kind"] == "rereview_pull_request_exact_head" + assert row["review_conclusion"]["valid"] is False + assert "independent_review" not in row + + +def test_dismissal_reopens_work_and_new_head_requires_new_review(): + histories = [[review(), review(2, state="DISMISSED")], + [review(), review(2, state="PENDING")]] + for reviews, expected in zip(histories, ["rereview_pull_request_exact_head", None]): + forward = packet(reviews)["pull_requests"][0] + reverse = packet(list(reversed(reviews)))["pull_requests"][0] + assert forward["review_action_kind"] == reverse["review_action_kind"] == expected + assert packet([review()], head=OLD)["pull_requests"][0]["review_action_kind"] is not None + assert packet([review(), review(2, head=OLD)], head=OLD)["review_sequence"] == [] + + +def test_another_reviewers_dismissal_does_not_erase_independent_coverage(): + data = packet([review(), review(2, login="other", state="DISMISSED")]) + assert data["pull_requests"][0]["review_action_kind"] is None + assert data["pull_requests"][0]["independent_review"]["reviewer"] == "contributor" + + +def test_explicit_fresh_audit_can_override_peer_coverage_without_granting_approval(): + row = packet([review()], fresh=True)["pull_requests"][0] + assert row["review_action_kind"] == "audit_pull_request_exact_head" + assert row["fresh_audit_requested"] and row["review_plan"] is not None + assert row["review_conclusion"]["valid"] is False + + +def test_merged_head_is_inventory_only_when_independently_reviewed(): + assert packet([review()], state="MERGED")["review_sequence"] == [] + + +def test_unknown_identity_parity_does_not_change_old_execution_or_strict_body_rules(): + original = review() + original["author"].pop("is_bot") + before = packet([deepcopy(original)], complete=False)["pull_requests"][0] + after = packet([deepcopy(original)], complete=True)["pull_requests"][0] + assert before == after + + +@pytest.mark.parametrize("state,draft", [("OPEN", True), ("CLOSED", False)]) +def test_fresh_audit_is_not_a_draft_or_closed_lifecycle_bypass(state, draft): + with pytest.raises(ValueError, match="requires a valid prior conclusion"): + packet([review()], state=state, draft=draft, fresh=True) + + +@pytest.mark.parametrize("state", ["COMMENTED", "APPROVED", "CHANGES_REQUESTED"]) +def test_work_coverage_cannot_satisfy_approval_closeout_or_erase_dissent(state): + from loopx.capabilities.pr_review_queue.approval_closeout import plan_approval_closeout + + data = packet([review(state=state)])["pull_requests"][0] + result = plan_approval_closeout({"expected_exact_head": f"42@{HEAD}", + "repository": "owner/repo", "pull_request": {"number": 42, "headRefOid": HEAD, + "state": "OPEN", "author": {"login": "author"}, "reviewDecision": data["review_decision"]}, + "readback_head": HEAD, "reviews_complete": True, + "reviews": [rest_review(review(state=state))], + "review_conclusions": [data["review_conclusion"] | {"review_id": 1}]}) + assert result["status"] == "hold" + assert "exact_head_approval_missing" in result["hold_reasons"] + assert result["dismissal_authorized"] is result["merge_authorized"] is False + if state == "CHANGES_REQUESTED": + assert [row["review_id"] for row in result["blocking_reviews"]] == [1] + + +def rest_review(row): + return {"id": row["id"], "html_url": row["url"], "user": { + "login": row["author"]["login"], "type": "Bot" if row["author"].get("is_bot") else "User"}, + "author_association": row["authorAssociation"], "state": row["state"], + "submitted_at": row["submittedAt"], "commit_id": row["commit"]["oid"], "body": row["body"]} + + +def provider(pages, *, unavailable=False, after_head=HEAD): + """Metadata substitutes GitHub IO, never the typed policy under test.""" + snapshot = review() + snapshot["author"].pop("is_bot") # Actual gh view omits actor type. + pr = raw_pr([snapshot]) + pr.pop("reviews_complete") + calls = [] + + def run(args, *, cwd=None): + del cwd + calls.append(args) + assert "statusCheckRollup" not in ",".join(args) + if args[:2] == ["pr", "view"]: + if args[args.index("--json") + 1] == "headRefOid,baseRefOid": + return {"headRefOid": after_head, "baseRefOid": pr["baseRefOid"]} + return deepcopy(pr) + assert args[:4] == ["api", "--paginate", "--slurp", "repos/owner/repo/pulls/42/reviews?per_page=100"] + if unavailable: + raise OSError("Review history temporarily unavailable") + return deepcopy(pages) + + return run, calls + + +def source_packet(run): + from loopx.capabilities.pr_review_queue.github_source import scan_github_pull_request_targets + + scan = scan_github_pull_request_targets(repository="owner/repo", exact_heads=[f"42@{HEAD}"], + wait_for_ci=False, run_gh_json=run) + return build_pr_review_packet(pull_requests=scan["pull_requests"], repository="owner/repo", + source="github_cli_exact_targets", source_scan=scan, limit=10, state_filter="all", + reviewer_login="agent", wait_for_ci=False) + + +def test_complete_provider_history_sees_late_dismissal_then_another_review(): + pages = [[rest_review(review(state="APPROVED"))], + [rest_review(review(2, state="DISMISSED"))]] + run, calls = provider(pages) + assert source_packet(run)["pull_requests"][0]["review_action_kind"] is not None + pages[-1].append(rest_review(review(3, login="maintainer", association="MEMBER"))) + data = source_packet(run) + assert data["pull_requests"][0]["review_action_kind"] is None + assert data["pull_requests"][0]["independent_review"]["reviewer"] == "maintainer" + assert data["result_completeness"]["complete"] + assert len([call for call in calls if call[0] == "api"]) == 2 + + +def test_source_failure_keeps_review_runnable_and_recovery_can_cover_it(): + pages = [[rest_review(review())]] + unavailable, _ = provider(pages, unavailable=True) + failed = source_packet(unavailable) + assert failed["result_completeness"]["complete"] # Ordinary versioned details are intact. + assert failed["pull_requests"][0]["review_action_kind"] is not None + assert "independent_review" not in failed["pull_requests"][0] + recovered, _ = provider(pages) + assert source_packet(recovered)["review_sequence"] == [] + + +def test_provider_rechecks_head_after_history_before_suppressing_work(): + run, _ = provider([[rest_review(review())]], after_head=OLD) + with pytest.raises(RuntimeError, match="detail read was incomplete"): + source_packet(run) + + +@pytest.mark.parametrize("pages", [[{}], [[{}]], [[rest_review(review()) | {"user": None}]]]) +def test_malformed_provider_history_retains_ordinary_review(pages): + run, _ = provider(pages) + row = source_packet(run)["pull_requests"][0] + assert row["review_action_kind"] is not None and "independent_review" not in row + + +def test_inline_only_submitted_review_does_not_need_an_agent_body(): + row = packet([review() | {"body": ""}])["pull_requests"][0] + assert row["review_action_kind"] is None and not row["review_conclusion"]["valid"]