diff --git a/AGENTS.md b/AGENTS.md index 4910de668b..37d416abc0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,7 +122,8 @@ merged commit and correcting the rule that let it through. - the change is single-purpose and easy to review from the diff; - required checks or focused smokes have passed; - the exact head carries a published self-review, and - `loopx pr-review --check-merge-readiness NUMBER@HEAD_OID` returned ready for + `loopx pr-review --goal-id GOAL --check-merge-readiness NUMBER@HEAD_OID` + returned ready for that unchanged head. GitHub blocks formal self-approval, so on an author-owned PR the record is a `COMMENTED` review on the exact head that states the approval conclusion and an English verdict; a green CI run, a diff diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 275d6b390c..5ab4bdecf7 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -59,6 +59,33 @@ def assert_public_safe(payload: dict[str, object]) -> None: ) +# `--check-merge-readiness` now mandates a Goal id. Every authoritative +# invocation an agent may read has to carry it, or the canonical self-merge +# gate fails deterministically before it can record its observation. +MERGE_READINESS_GUIDANCE_PATHS = ( + REPO_ROOT / "AGENTS.md", + REPO_ROOT / "loopx" / "capabilities" / "pr_review_queue" / "README.md", + REPO_ROOT / "loopx" / "capabilities" / "pr_review_queue" / "catalog_entry.py", + PR_REVIEW_SKILL, + PR_MERGE_SKILL, +) + + +def assert_merge_readiness_invocations_require_goal_id() -> None: + for path in MERGE_READINESS_GUIDANCE_PATHS: + source = path.read_text(encoding="utf-8") + for span in re.findall(r"`[^`]*--check-merge-readiness[^`]*`", source): + assert "--goal-id" in span, ( + f"{path.name} must pass --goal-id to --check-merge-readiness: {span}" + ) + for line in source.splitlines(): + if "--check-merge-readiness" in line and "`" not in line: + assert "--goal-id" in line, ( + f"{path.name} must pass --goal-id to --check-merge-readiness: " + f"{line.strip()}" + ) + + def main() -> int: skill_source = PR_REVIEW_SKILL.read_text(encoding="utf-8") skill_text = " ".join(skill_source.split()) @@ -149,6 +176,7 @@ def main() -> int: "A merge decision without this evidence is not authorized", ): assert phrase in merge_text, phrase + assert_merge_readiness_invocations_require_goal_id() assert _github_search_date("2026-06-28T00:00:00+08:00") == "2026-06-27" assert _github_search_date("2026-06-28T00:00:00Z") == "2026-06-28" @@ -316,6 +344,12 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: merge_head = "e" * 40 with tempfile.TemporaryDirectory() as temp_dir: + runtime_root = Path(temp_dir) / "runtime" + registry_path = Path(temp_dir) / "registry.json" + registry_path.write_text( + json.dumps({"goals": [{"id": "test-goal", "repo": temp_dir}]}), + encoding="utf-8", + ) merge_fixture_path = Path(temp_dir) / "merge-readiness.json" merge_fixture = { "repository": "owner/repo", @@ -372,9 +406,15 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: merge_fixture_path.write_text(json.dumps(merge_fixture), encoding="utf-8") ready = json.loads( run_cli( + "--runtime-root", + str(runtime_root), + "--registry", + str(registry_path), "--format", "json", "pr-review", + "--goal-id", + "test-goal", "--fixture", str(merge_fixture_path), "--check-merge-readiness", @@ -383,6 +423,29 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: ) assert ready["ready"] is True, ready assert ready["blocking_reasons"] == [], ready + unchanged_queue = json.loads( + run_cli( + "--runtime-root", + str(runtime_root), + "--registry", + str(registry_path), + "--format", + "json", + "pr-review", + "--goal-id", + "test-goal", + "--fixture", + str(merge_fixture_path), + "--state", + "open", + ).stdout + ) + unchanged_item = unchanged_queue["pull_requests"][0] + assert unchanged_item["review_action_kind"] is None, unchanged_item + assert ( + unchanged_item["merge_readiness_observation"]["observation_state"] + == "observed_unchanged" + ), unchanged_item merge_fixture["pull_requests"][0]["reviews"][0]["body"] = merge_fixture[ "pull_requests" @@ -392,9 +455,15 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: ) merge_fixture_path.write_text(json.dumps(merge_fixture), encoding="utf-8") blocked_run = run_cli( + "--runtime-root", + str(runtime_root), + "--registry", + str(registry_path), "--format", "json", "pr-review", + "--goal-id", + "test-goal", "--fixture", str(merge_fixture_path), "--check-merge-readiness", @@ -471,6 +540,12 @@ def approved_open_head( } with tempfile.TemporaryDirectory() as temp_dir: + runtime_root = Path(temp_dir) / "runtime" + registry_path = Path(temp_dir) / "registry.json" + registry_path.write_text( + json.dumps({"goals": [{"id": "test-goal", "repo": temp_dir}]}), + encoding="utf-8", + ) approval_fixture_path = Path(temp_dir) / "approved-open-heads.json" approval_fixture = { "repository": "owner/repo", @@ -522,9 +597,15 @@ def approved_open_head( ): readiness = json.loads( run_cli( + "--runtime-root", + str(runtime_root), + "--registry", + str(registry_path), "--format", "json", "pr-review", + "--goal-id", + "test-goal", "--fixture", str(approval_fixture_path), "--check-merge-readiness", diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 574bf0427a..493be080e1 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -47,7 +47,7 @@ workflow or the merge-focused `loopx-pr-merge` skill. | Command | CLI reference | Intent | | --- | --- | --- | | `/loopx-pr-review` | `loopx pr-review [--repo owner/repo] [--target-exact-head NUMBER@HEAD_OID] [--state open\|merged\|all] [--review-priority other-developers-first\|owner-first] [--since ISO] [--fresh-audit-exact-head NUMBER@HEAD_OID]` | Review a small explicit batch with repeatable `--target-exact-head`, or list a lifecycle queue when no target is supplied. Both paths provide concrete main-regression analysis and the five-block review contract. The default queue prioritizes non-owner developer PRs; `owner-first` opts into owner priority. `--fresh-audit-exact-head` separately forces new evidence for an unchanged concluded head. | -| pre-merge readback | `loopx pr-review --repo owner/repo --check-merge-readiness NUMBER@HEAD_OID` | Immediately before merge, fail closed unless the remote PR is still open at the reviewed head, its standalone conclusion approves that head, all checks are successful or skipped, review-thread pagination is complete with no unresolved thread, and merge state is compatible. This read grants no merge authority. | +| pre-merge readback | `loopx pr-review --goal-id GOAL --repo owner/repo --check-merge-readiness NUMBER@HEAD_OID` | Immediately before merge, fail closed unless the remote PR is still open at the reviewed head, its standalone conclusion approves that head, all checks are successful or skipped, review-thread pagination is complete with no unresolved thread, and merge state is compatible. The Goal-scoped command records a compact public-safe readiness observation; this read grants no merge authority. | The slash command must run the CLI first. Agentloop must not reconstruct the review window by manually calling `gh pr view` / `gh pr list` for every PR. The @@ -232,15 +232,18 @@ contain only rows whose `review_action_kind` is non-null. A merged exact head without a valid conclusion receives `audit_merged_pull_request_exact_head`; a merged or open exact head whose valid conclusion is not an approval remains inventory-only and cannot become the recommended first PR. An open exact head -with a valid approval keeps owing `qualify_pull_request_merge_readiness`, -because merge readiness is decided by the typed verdict rather than by GitHub's +with a valid approval owes `qualify_pull_request_merge_readiness` until the Goal +has observed its current readiness material state. An unchanged observation +suppresses duplicate qualification work; exact-head, base, review conclusion, +configured CI, review-thread, merge-state, or draft-state changes reopen it. +This preserves the typed readiness verdict rather than relying on GitHub's review state: the platform blocks self-approval, so an author-owned approval is -recorded as `COMMENTED` and a state-based rule would count that still-unmerged -head as concluded, even after it goes behind, conflicts, loses its checks or is -blocked. The summary's attention counts are derived from this same actionable -set. Inventory-only rows set `review_plan` and `review_template` to null and -`evidence_commands` to an empty list so hosts cannot mistake readback metadata -for execution authority. +recorded as `COMMENTED` and a state-based rule could otherwise count a +still-unmerged head as concluded even after it goes behind, conflicts, loses +its checks or is blocked. The summary's attention counts are derived from this +same actionable set. Inventory-only rows set `review_plan` and +`review_template` to null and `evidence_commands` to an empty list so hosts +cannot mistake readback metadata for execution authority. It emits a `pull_request_review_todo_preview_v0` bound to its exact head. The preview may @@ -526,6 +529,15 @@ bypass may satisfy GitHub's author-owned self-review limitation, but it never overrides this capability gate or supplies user merge authority. +`pull_request_merge_readiness_observation_v0` is the Goal-scoped scheduling +receipt for that gate. It persists only the exact-head material fingerprint and +compact public-safe readiness result under the local Goal runtime. It excludes +review bodies, raw logs, credentials, private payloads, and local paths. Queue +construction consumes the observation only when every readiness input still +matches; a changed head, base, review conclusion, CI policy/result, review +thread, draft flag, merge state, or PR state fails open to a fresh +qualification. + They must not include raw logs, private connector payloads, credentials, local absolute paths, private source bodies, or hidden CI artifacts. @@ -829,7 +841,8 @@ The packet should let a reviewer move through PRs in order: copied as the final risk judgement. 8. Recheck the exact head, then decide `approve`, `request changes`, `defer`, or `merge after checks`. Immediately before merge, require - `--check-merge-readiness NUMBER@HEAD_OID` to return `ready=true`. + `loopx pr-review --goal-id GOAL --check-merge-readiness NUMBER@HEAD_OID` to + return `ready=true`. A response that only lists `Open` and `Merged` PRs, scale, and recommended next order is incomplete for `/loopx-pr-review`; it should continue into the @@ -859,9 +872,10 @@ A first implementation is acceptable when: - `--fresh-audit-exact-head NUMBER@HEAD_OID` is the only packet-level way to turn an unchanged valid conclusion into an actionable fresh audit, and malformed, absent, or already-actionable targets fail closed; -- `--check-merge-readiness NUMBER@HEAD_OID` rejects head drift, stale review +- Goal-scoped `--goal-id GOAL --check-merge-readiness NUMBER@HEAD_OID` rejects head drift, stale review prose, non-approval conclusions, red/pending/unknown checks, incomplete or - unresolved review-thread evidence, and incompatible merge state; + unresolved review-thread evidence, and incompatible merge state, then records + the compact observation used to suppress only unchanged requalification; - the default limit is 100, and exhaustive requests only proceed when `result_completeness.complete=true`; truncated packets provide a larger `recommended_limit` for the next read; diff --git a/loopx/capabilities/pr_review_queue/catalog_entry.py b/loopx/capabilities/pr_review_queue/catalog_entry.py index 4da645b7d2..f4efe1d785 100644 --- a/loopx/capabilities/pr_review_queue/catalog_entry.py +++ b/loopx/capabilities/pr_review_queue/catalog_entry.py @@ -33,9 +33,9 @@ ), "commands": [ { - "command": "loopx pr-review --repo --check-merge-readiness NUMBER@HEAD_OID --format json", + "command": "loopx pr-review --goal-id --repo --check-merge-readiness NUMBER@HEAD_OID --format json", "purpose": "Fail closed on exact-head, approval-body, configured CI, thread, or merge-state drift immediately before merge.", - "write_boundary": "live public GitHub read only; does not approve, merge, bypass policy, or grant merge authority", + "write_boundary": "reads live public GitHub state and writes one compact public-safe Goal observation; does not approve, merge, bypass policy, or grant merge authority", }, { "command": "loopx pr-review --check-result --packet --format json", @@ -82,6 +82,11 @@ "module": "loopx.capabilities.pr_review_queue.merge_readiness", "doc": "loopx/capabilities/pr_review_queue/README.md", }, + { + "schema_version": "pull_request_merge_readiness_observation_v0", + "module": "loopx.capabilities.pr_review_queue.readiness_observation", + "doc": "loopx/capabilities/pr_review_queue/README.md", + }, { "schema_version": "pull_request_review_result_check_v0", "module": "loopx.capabilities.pr_review_queue.result_check", @@ -151,7 +156,8 @@ "Only rows with a non-null review_action_kind enter review_sequence and carry review plans, templates, or evidence commands; valid exact-head conclusions remain artifact-free inventory-only rows, and only --fresh-audit-exact-head NUMBER@HEAD_OID can explicitly reopen one.", "Todo prose, monitor notes, and one-off author filters are not scheduling authority.", "A complete exact-head conclusion requires the five Chinese sections, a state-aligned English verdict, and formal state or the verdict-specific titled author-owned fallback.", - "Every merge must rerun the read-only merge-readiness gate for the reviewed exact head; admin bypass cannot override stale review text, required CI when wait_for_ci is true, incomplete thread evidence, or head drift.", + "Every merge must rerun the Goal-scoped merge-readiness gate for the reviewed exact head; the resulting compact observation suppresses only unchanged qualification work, and any head, base, review, CI, thread, draft, merge-state, or PR-state change reopens it.", + "Readiness observations contain only public-safe material fingerprints and compact verdicts; they exclude review bodies, raw logs, credentials, private payloads, and local paths.", "One observation emits at most one exact-head advancement Todo preview; unchanged observations replay it until explicit durable Todo-projection ACK, then rotate across acknowledged exact heads.", "The capability reuses the existing pr-review GitHub scan and normalized packet; review bodies are inspected for format but never emitted or checkpointed.", "Candidate selection grants no GitHub review, comment, push, merge, quota, or Todo-write authority; those remain with their existing policy surfaces.", diff --git a/loopx/capabilities/pr_review_queue/github_source.py b/loopx/capabilities/pr_review_queue/github_source.py index 1d4de6633b..6938cecfc9 100644 --- a/loopx/capabilities/pr_review_queue/github_source.py +++ b/loopx/capabilities/pr_review_queue/github_source.py @@ -32,6 +32,7 @@ "headRefName", "headRefOid", "baseRefName", + "baseRefOid", "author", "createdAt", "updatedAt", diff --git a/loopx/capabilities/pr_review_queue/merge_readiness.py b/loopx/capabilities/pr_review_queue/merge_readiness.py index 3299bb6927..3e0da371ac 100644 --- a/loopx/capabilities/pr_review_queue/merge_readiness.py +++ b/loopx/capabilities/pr_review_queue/merge_readiness.py @@ -5,6 +5,11 @@ from collections.abc import Mapping from typing import Any +from .readiness_observation import ( + readiness_material_fingerprint, + readiness_material_state, +) + SCHEMA_VERSION = "pull_request_merge_readiness_v0" @@ -89,6 +94,12 @@ def build_merge_readiness( blockers.append("repository_merge_state_blocked") blockers = list(dict.fromkeys(blockers)) + material_state = readiness_material_state( + repository=repository, + item=item, + review_threads=review_threads, + wait_for_ci=wait_for_ci, + ) return { "ok": True, "schema_version": SCHEMA_VERSION, @@ -111,6 +122,8 @@ def build_merge_readiness( "ci_policy": "required" if wait_for_ci else "not_consulted", "wait_for_ci": wait_for_ci, "blocking_reasons": blockers, + "material_state": material_state, + "material_fingerprint": readiness_material_fingerprint(material_state), "authority": { "grants_merge_authority": False, "admin_bypass_overrides_this_gate": False, diff --git a/loopx/capabilities/pr_review_queue/readiness_observation.py b/loopx/capabilities/pr_review_queue/readiness_observation.py new file mode 100644 index 0000000000..52d26fff86 --- /dev/null +++ b/loopx/capabilities/pr_review_queue/readiness_observation.py @@ -0,0 +1,248 @@ +"""Goal-scoped observations for exact-head merge-readiness qualification.""" + +from __future__ import annotations + +import hashlib +import json +import re +from collections.abc import Mapping +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +from ...file_lock import exclusive_file_lock +from ...registry import atomic_write_json +from ...runtime import validate_goal_id_path_segment + + +MERGE_READINESS_OBSERVATION_SCHEMA_VERSION = ( + "pull_request_merge_readiness_observation_v0" +) +MERGE_READINESS_OBSERVATION_STORE_SCHEMA_VERSION = ( + "pull_request_merge_readiness_observation_store_v0" +) +MAX_OBSERVATIONS = 200 +EXACT_HEAD_PATTERN = re.compile(r"^[1-9][0-9]*@[0-9a-f]{40}$") + + +def _mapping(value: object) -> Mapping[str, Any]: + return value if isinstance(value, Mapping) else {} + + +def _text(value: object) -> str: + return str(value or "").strip() + + +def _string_list(value: object) -> list[str]: + if not isinstance(value, list): + return [] + return sorted({_text(item) for item in value if _text(item)}) + + +def readiness_material_state( + *, + repository: str, + item: Mapping[str, Any], + review_threads: Mapping[str, Any], + wait_for_ci: bool, +) -> dict[str, Any]: + """Return only facts whose change requires a new readiness decision.""" + + conclusion = _mapping(item.get("review_conclusion")) + checks = _mapping(item.get("checks")) + counts = _mapping(checks.get("counts")) + number = item.get("number") + head_oid = _text(item.get("head_oid")).casefold() + exact_head = f"{number}@{head_oid}" if isinstance(number, int) and head_oid else None + return { + "repository": _text(repository).casefold(), + "exact_head": exact_head, + "base_oid": _text(item.get("base_oid")).casefold() or None, + "state": _text(item.get("state")).upper(), + "is_draft": item.get("is_draft") is True, + "merge_state": _text(item.get("merge_state")).upper(), + "review_decision": _text(item.get("review_decision")).upper(), + "review_conclusion": { + "valid": conclusion.get("valid") is True, + "status": _text(conclusion.get("status")), + "state": _text(conclusion.get("state")).upper(), + "verdict": _text(conclusion.get("verdict")).upper(), + "review_commit": _text(conclusion.get("review_commit")).casefold() + or None, + "invalid_reasons": _string_list(conclusion.get("invalid_reasons")), + }, + "checks": { + "consulted": wait_for_ci, + "total": checks.get("total") if type(checks.get("total")) is int else None, + "counts": { + key: int(counts.get(key) or 0) + for key in ("success", "failure", "pending", "unknown") + }, + "failures": _string_list(checks.get("failures")), + "pending": _string_list(checks.get("pending")), + }, + "review_threads": { + "complete": review_threads.get("complete") is True, + "total_count": ( + review_threads.get("total_count") + if type(review_threads.get("total_count")) is int + else None + ), + "unresolved_count": ( + review_threads.get("unresolved_count") + if type(review_threads.get("unresolved_count")) is int + else None + ), + "failure_code": _text(review_threads.get("failure_code")) or None, + }, + "wait_for_ci": wait_for_ci, + } + + +def readiness_material_fingerprint(material_state: Mapping[str, Any]) -> str: + encoded = json.dumps( + material_state, + ensure_ascii=True, + sort_keys=True, + separators=(",", ":"), + ) + return hashlib.sha256(encoded.encode("utf-8")).hexdigest() + + +def observation_key(repository: str, exact_head: str) -> str: + return f"{_text(repository).casefold()}#{_text(exact_head).casefold()}" + + +def build_readiness_observation( + *, goal_id: str, readiness: Mapping[str, Any] +) -> dict[str, Any]: + material_state = _mapping(readiness.get("material_state")) + fingerprint = _text(readiness.get("material_fingerprint")) + if not material_state or len(fingerprint) != 64: + raise ValueError("merge readiness payload lacks a material observation") + repository = _text(readiness.get("repository")) + exact_head = _text(readiness.get("expected_exact_head")).casefold() + if not repository or not EXACT_HEAD_PATTERN.fullmatch(exact_head): + raise ValueError("merge readiness observation requires repository and exact head") + return { + "schema_version": MERGE_READINESS_OBSERVATION_SCHEMA_VERSION, + "goal_id": validate_goal_id_path_segment(goal_id), + "repository": repository, + "exact_head": exact_head, + "material_fingerprint": fingerprint, + "material_state": dict(material_state), + "ready": readiness.get("ready") is True, + "blocking_reasons": _string_list(readiness.get("blocking_reasons")), + "observed_at": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), + } + + +def readiness_observation_path(runtime_root: Path, goal_id: str) -> Path: + safe_goal = validate_goal_id_path_segment(goal_id) + return ( + runtime_root.expanduser().resolve() + / "goals" + / safe_goal + / "pr_review" + / "merge_readiness_observations.json" + ) + + +def read_readiness_observations( + *, runtime_root: Path, goal_id: str +) -> dict[str, dict[str, Any]]: + path = readiness_observation_path(runtime_root, goal_id) + with exclusive_file_lock(path, operation="pr_review_readiness_observation_read"): + if not path.exists(): + return {} + payload = json.loads(path.read_text(encoding="utf-8")) + if ( + not isinstance(payload, dict) + or payload.get("schema_version") + != MERGE_READINESS_OBSERVATION_STORE_SCHEMA_VERSION + ): + raise ValueError("merge readiness observation store has an unsupported schema") + if payload.get("goal_id") != validate_goal_id_path_segment(goal_id): + raise ValueError("merge readiness observation store belongs to another Goal") + rows = payload.get("observations") + if not isinstance(rows, list): + raise TypeError("merge readiness observation store observations must be a list") + result: dict[str, dict[str, Any]] = {} + for row in rows: + if ( + not isinstance(row, dict) + or row.get("schema_version") + != MERGE_READINESS_OBSERVATION_SCHEMA_VERSION + ): + raise ValueError("merge readiness observation store contains an invalid row") + key = observation_key(str(row.get("repository") or ""), str(row.get("exact_head") or "")) + result[key] = dict(row) + return result + + +def record_readiness_observation( + *, runtime_root: Path, goal_id: str, readiness: Mapping[str, Any] +) -> dict[str, Any]: + observation = build_readiness_observation(goal_id=goal_id, readiness=readiness) + path = readiness_observation_path(runtime_root, goal_id) + key = observation_key(observation["repository"], observation["exact_head"]) + repository_number = key.rsplit("@", 1)[0] + with exclusive_file_lock(path, operation="pr_review_readiness_observation_write"): + current: dict[str, dict[str, Any]] = {} + if path.exists(): + payload = json.loads(path.read_text(encoding="utf-8")) + if ( + not isinstance(payload, dict) + or payload.get("schema_version") + != MERGE_READINESS_OBSERVATION_STORE_SCHEMA_VERSION + or payload.get("goal_id") != observation["goal_id"] + ): + raise ValueError("merge readiness observation store is invalid") + for row in payload.get("observations") or []: + if isinstance(row, dict): + row_key = observation_key( + str(row.get("repository") or ""), + str(row.get("exact_head") or ""), + ) + if row_key.rsplit("@", 1)[0] != repository_number: + current[row_key] = dict(row) + current[key] = observation + rows = sorted( + current.values(), + key=lambda row: str(row.get("observed_at") or ""), + reverse=True, + )[:MAX_OBSERVATIONS] + atomic_write_json( + path, + { + "schema_version": MERGE_READINESS_OBSERVATION_STORE_SCHEMA_VERSION, + "goal_id": observation["goal_id"], + "observations": rows, + }, + preserve_mode=True, + ) + return observation + + +def observation_matches_material_state( + observation: Mapping[str, Any], material_state: Mapping[str, Any] +) -> bool: + return bool( + observation.get("schema_version") + == MERGE_READINESS_OBSERVATION_SCHEMA_VERSION + and _text(observation.get("material_fingerprint")) + == readiness_material_fingerprint(material_state) + ) + + +__all__ = [ + "MERGE_READINESS_OBSERVATION_SCHEMA_VERSION", + "MERGE_READINESS_OBSERVATION_STORE_SCHEMA_VERSION", + "build_readiness_observation", + "observation_key", + "observation_matches_material_state", + "read_readiness_observations", + "readiness_material_fingerprint", + "readiness_material_state", + "record_readiness_observation", +] diff --git a/loopx/capabilities/pr_review_queue/selection_execution.py b/loopx/capabilities/pr_review_queue/selection_execution.py index ee03a66a3d..2c23d21466 100644 --- a/loopx/capabilities/pr_review_queue/selection_execution.py +++ b/loopx/capabilities/pr_review_queue/selection_execution.py @@ -7,6 +7,12 @@ from typing import Any from .review_contract import build_review_plan, build_review_template +from .readiness_observation import ( + observation_key, + observation_matches_material_state, + readiness_material_fingerprint, + readiness_material_state, +) EXACT_HEAD_PATTERN = re.compile( @@ -62,10 +68,36 @@ def exact_head_key(item: Mapping[str, Any]) -> str | None: def materialize_review_execution( - item: Mapping[str, Any], *, fresh_audit_exact_heads: set[str] + item: Mapping[str, Any], + *, + fresh_audit_exact_heads: set[str], + readiness_observations: Mapping[str, Mapping[str, Any]] | None = None, + repository: str | None = None, + review_threads: Mapping[str, Any] | None = None, ) -> dict[str, Any]: action_kind = review_action_kind(item) key = exact_head_key(item) + readiness_observation: dict[str, Any] | None = None + if action_kind == "qualify_pull_request_merge_readiness" and key and repository: + observed = (readiness_observations or {}).get(observation_key(repository, key)) + if observed: + material_state = readiness_material_state( + repository=repository, + item=item, + review_threads=review_threads or {}, + wait_for_ci=item.get("wait_for_ci") is not False, + ) + matched = observation_matches_material_state(observed, material_state) + readiness_observation = { + "schema_version": "pull_request_merge_readiness_queue_match_v0", + "observation_state": ( + "observed_unchanged" if matched else "material_transition" + ), + "material_fingerprint": readiness_material_fingerprint(material_state), + "previous_material_fingerprint": observed.get("material_fingerprint"), + } + if matched: + action_kind = None fresh_audit_requested = key in fresh_audit_exact_heads conclusion = item.get("review_conclusion") conclusion = conclusion if isinstance(conclusion, Mapping) else {} @@ -79,6 +111,7 @@ def materialize_review_execution( result: dict[str, Any] = { "review_action_kind": action_kind, "fresh_audit_requested": fresh_audit_requested, + "merge_readiness_observation": readiness_observation, } if not action_kind: return result | { diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index 6cc8b9d590..c581087fb1 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -4,6 +4,7 @@ import hashlib import json from collections.abc import Callable, Sequence +from concurrent.futures import ThreadPoolExecutor from contextlib import suppress from pathlib import Path @@ -18,6 +19,11 @@ ) from ..capabilities.machine_configuration.store import read_machine_configuration from ..capabilities.pr_review_queue.result_check import check_review_result +from ..capabilities.pr_review_queue.readiness_observation import ( + observation_key, + read_readiness_observations, + record_readiness_observation, +) from ..capabilities.pr_review_queue.github_source import ( scan_github_pull_request_targets, ) @@ -110,7 +116,8 @@ def register_pr_review_command( metavar="NUMBER@HEAD_OID", help=( "Re-read one open PR and fail closed unless this exact reviewed head, " - "its configured CI policy, approval, and review threads are ready immediately before merge." + "its configured CI policy, approval, and review threads are ready immediately before merge; " + "requires --goal-id and records a compact local Goal readiness observation." ), ) parser.add_argument( @@ -235,6 +242,7 @@ def handle_pr_review_command( checkpoint_path: Path | None = None resolved_review_priority = DEFAULT_REVIEW_PRIORITY target_exact_heads = list(getattr(args, "target_exact_head", []) or []) + readiness_observations: dict[str, dict[str, object]] = {} resolved_state_filter = _resolve_pr_review_state_filter( getattr(args, "state", None), target_exact_heads=target_exact_heads, @@ -251,6 +259,13 @@ 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 goal_id: + if runtime_root is None: + raise ValueError("--goal-id requires an available runtime root") + readiness_observations = read_readiness_observations( + runtime_root=runtime_root, + goal_id=goal_id, + ) if args.check_result or args.packet: if not (args.check_result and args.packet): raise ValueError("--check-result and --packet must be used together") @@ -287,6 +302,8 @@ def handle_pr_review_command( ) return 0 if payload["ok"] else 1 if args.check_merge_readiness: + if not goal_id: + raise ValueError("merge readiness requires --goal-id") if ( args.autonomous_observation or args.observation_state_file @@ -359,6 +376,25 @@ def handle_pr_review_command( source=source, wait_for_ci=wait_for_ci, ) + observation = record_readiness_observation( + runtime_root=runtime_root, + goal_id=goal_id, + readiness=payload, + ) + payload["readiness_observation"] = { + key: observation[key] + for key in ( + "schema_version", + "goal_id", + "repository", + "exact_head", + "material_fingerprint", + "ready", + "blocking_reasons", + "observed_at", + ) + } + payload["local_goal_observation_write_performed"] = True print_payload( payload, output_format(args), @@ -455,6 +491,41 @@ def handle_pr_review_command( **({"wait_for_ci": False} if not wait_for_ci else {}), ) pull_requests = source_scan["pull_requests"] + if readiness_observations: + observed_rows = [] + for row in pull_requests: + number = row.get("number") + head_oid = str(row.get("headRefOid") or "").strip().casefold() + exact_head = ( + f"{number}@{head_oid}" + if isinstance(number, int) and head_oid + else "" + ) + if observation_key(str(repository or ""), exact_head) not in readiness_observations: + continue + if args.fixture: + raw_threads = row.get("review_thread_summary") + if not isinstance(raw_threads, dict): + row["review_thread_summary"] = { + "schema_version": "github_review_thread_summary_v0", + "complete": False, + "total_count": 0, + "unresolved_count": 0, + "failure_code": "fixture_review_thread_summary_missing", + } + else: + observed_rows.append(row) + if observed_rows: + with ThreadPoolExecutor(max_workers=min(8, len(observed_rows))) as pool: + summaries = pool.map( + lambda row: fetch_github_review_thread_summary( + repo=str(repository or ""), + number=row["number"], + ), + observed_rows, + ) + for row, summary in zip(observed_rows, summaries, strict=True): + row["review_thread_summary"] = summary if checkpoint_path is not None and previous_observation: checkpoint_repository = str( previous_observation.get("repository") or "" @@ -479,9 +550,13 @@ def handle_pr_review_command( target_exact_heads=target_exact_heads, review_priority=resolved_review_priority, wait_for_ci=wait_for_ci, + readiness_observations=readiness_observations, ) payload["request"]["goal_id"] = goal_id payload["request"]["review_configuration"] = review_configuration + payload["request"]["readiness_observation_count"] = len( + readiness_observations + ) if args.autonomous_observation: autonomous_review = build_pull_request_review_queue_observation( repository=repository, diff --git a/loopx/pr_review.py b/loopx/pr_review.py index 4350947588..29263d0ee8 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -928,6 +928,8 @@ def _normalize_pr( generated_at: datetime, fresh_audit_exact_heads: set[str], wait_for_ci: bool = True, + readiness_observations: Mapping[str, Mapping[str, object]] | None = None, + repository: str | None = None, ) -> dict[str, Any]: files = _files(pr) checks = _checks(pr) @@ -975,6 +977,7 @@ def _normalize_pr( if merge_commit_oid else None, "base_ref": _redact_text(pr.get("baseRefName"), limit=80), + "base_oid": _redact_text(pr.get("baseRefOid"), limit=80), "head_ref": _redact_text(pr.get("headRefName"), limit=120), "head_oid": _redact_text(pr.get("headRefOid"), limit=80), "is_draft": bool(pr.get("isDraft")), @@ -1001,7 +1004,11 @@ def _normalize_pr( } item.update( materialize_review_execution( - item, fresh_audit_exact_heads=fresh_audit_exact_heads + item, + fresh_audit_exact_heads=fresh_audit_exact_heads, + readiness_observations=readiness_observations or {}, + repository=repository, + review_threads=_as_dict(pr.get("review_thread_summary")), ) ) item["community_feedback_ready"] = bool( @@ -1030,6 +1037,7 @@ def build_pr_review_packet( target_exact_heads: Sequence[str] = (), review_priority: object = DEFAULT_REVIEW_PRIORITY, wait_for_ci: bool = True, + readiness_observations: Mapping[str, Mapping[str, object]] | None = None, ) -> dict[str, Any]: normalized_state_filter = normalize_pr_state_filter(state_filter) normalized_priority = normalize_review_priority(review_priority) @@ -1045,6 +1053,8 @@ def build_pr_review_packet( generated_at=generated_at, fresh_audit_exact_heads=requested_fresh_audits, wait_for_ci=wait_for_ci, + readiness_observations=readiness_observations, + repository=repository, ) for item in pull_requests ] diff --git a/loopx/pr_review_merge_readiness.py b/loopx/pr_review_merge_readiness.py index ca74d85f40..34d0127072 100644 --- a/loopx/pr_review_merge_readiness.py +++ b/loopx/pr_review_merge_readiness.py @@ -27,7 +27,7 @@ def fetch_github_pull_request( ) -> dict[str, Any]: fields = ( "number,title,url,state,isDraft,reviewDecision,mergeStateStatus," - "headRefName,headRefOid,baseRefName,author,createdAt,updatedAt," + "headRefName,headRefOid,baseRefName,baseRefOid,author,createdAt,updatedAt," "closedAt,mergedAt,mergeCommit,body,files,changedFiles,additions," "deletions,commits,reviews" ) diff --git a/skills/loopx-pr-merge/SKILL.md b/skills/loopx-pr-merge/SKILL.md index 2e427d098b..63485d8421 100644 --- a/skills/loopx-pr-merge/SKILL.md +++ b/skills/loopx-pr-merge/SKILL.md @@ -28,9 +28,10 @@ authorized: a diff read, green CI, or pull-request metadata is not a substitute. surface. 2. Apply `completion_gate` literally, and re-read the remote head immediately before the decision. -3. Immediately before the merge, run `loopx --format json pr-review --repo - OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`. Merge only when it - returns `ready=true` for that unchanged head. +3. Immediately before the merge, run `loopx --format json pr-review --goal-id + GOAL --repo OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`. Merge only + when it returns `ready=true` for that unchanged head; this records the + compact Goal readiness observation consumed by future review queues. A rebase or head update restarts review, and admin bypass never overrides this gate: it needs explicit owner authorization and never substitutes for the diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index fae045e5b5..bf6a382e29 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -115,10 +115,10 @@ 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. -Immediately before every merge, run `loopx --format json pr-review --repo -OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`; merge only when it returns -`ready=true` for that unchanged head. A rebase/update restarts review, admin -bypass never overrides this gate, and author-owned fallback needs user 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`. +Its compact Goal observation suppresses only unchanged requalification; material +change reopens it, admin bypass never overrides this gate, and author fallback needs user authority. ## Full PR Review And Bilingual Format diff --git a/skills/loopx-self-repair/references/repair-patterns.md b/skills/loopx-self-repair/references/repair-patterns.md index b62b5b8e6a..00362932fb 100644 --- a/skills/loopx-self-repair/references/repair-patterns.md +++ b/skills/loopx-self-repair/references/repair-patterns.md @@ -156,6 +156,7 @@ teaches a reusable control-plane lesson. | `pr_review_default_lifecycle_overreach` | An ordinary `loopx pr-review` queue scans historical merged PRs, saturates a closed-PR window, or diverts reviewers into post-merge audits even though the user asked to continue current reviews. | CLI parser defaults, internal scan and packet defaults, installed skill first-pass command, ordinary no-state packet, explicit all-state packet, and a merged exact-target packet. | Queue discovery and deliberate lifecycle/post-merge audit shared an `all` default, and the host adapter reinforced it with an explicit `--state all` command. | Default ordinary discovery and internal APIs to `open`; require explicit `--state merged|all` for history; keep an omitted-state exact-target request lifecycle-neutral so a named merged head remains reviewable; cover all three routes in the command smoke. | | `pr_review_response_duplicate_replan` | A PR still reports `CHANGES_REQUESTED` after the author pushed a newer repair and resolved every review thread, so lifecycle polling repeatedly proposes another patch successor while the actual next step is reviewer re-approval. | Compact PR lifecycle metadata, complete review-thread counts, latest changes-requested timestamp, head commit timestamp, CI rollup, and current monitor todo. | Lifecycle routing treated the aggregate review decision as a current unhandled action and ignored the response state encoded by newer commits plus resolved threads. | On explicit metadata fetch, read only compact review-response evidence and route to a quiet re-review monitor when at least one thread exists, every fetched thread is resolved, pagination is complete, and the head commit is newer than the review. Missing, partial, empty-thread, or older evidence must fail closed to the actionable replan route. | | `pr_review_explicit_selection_idempotency_gap` | A named PR receives another full evidence pass even though the packet marks its exact-head conclusion valid or its merged row has `review_action_kind=null`; generic `re-review` wording repeatedly reopens completed work, a null-action row remains in the ranked `review_sequence`, or its inventory row still carries an executable-looking review plan/template/commands that a host follows. | Selected packet row, exact head, `review_conclusion`, `review_action_kind`, plan/template/command presence, top-level and group `review_sequence`, summary attention counts, current-request wording, and the evidence-command tool trace. | The host adapter conflated explicit selection authority over queue ordering with authority to override the capability's exact-head execution decision, while the packet mixed bounded inventory with executable ranking and artifacts and treated generic re-review wording as a force-refresh token. | Project a typed selection-execution contract: explicit selection changes ordering only; null action is compact readback-only with null plan/template and empty evidence commands; every `review_sequence` and attention count derives exclusively from non-null actions; and a fresh audit on an unchanged/no-action row must be regenerated through the typed `--fresh-audit-exact-head NUMBER@HEAD_OID` option after an explicit request or concrete new concern/evidence invalidation. Keep valid concluded rows only in inventory; assign an explicit audit action to merged heads that genuinely lack a valid conclusion. Cover the packet contract, installed skill text, exact valid-merged fixture, and result-check rejection of inventory-only rows with focused smokes. | +| `pr_review_readiness_observation_gap` | An open exact head with a valid approval is repeatedly selected for merge-readiness qualification even though the Goal already observed the same behind, blocked, or ready state and no material input changed. | Exact head and base OIDs, approval conclusion fingerprint, configured CI policy and effective checks, complete review-thread summary, draft/merge/PR state, prior Goal-scoped readiness observation, and current `review_action_kind`. | Queue selection derived qualification only from the durable approval and discarded the last readiness result, so each heartbeat reconstructed the same action with no monotonic observation boundary. | Require `--goal-id` for merge-readiness checks, persist one compact public-safe material fingerprint under the Goal runtime, and suppress only an exact unchanged match. Reopen qualification on any head, base, approval, CI policy/result, thread, draft, merge-state, or PR-state change; missing or partial evidence fails open to fresh qualification. Cover record, unchanged replay, material transition, and unscoped-command rejection through the real CLI path. | | `pr_review_key_code_explanation_gap` | A PR review names changed files and symbols, summarizes intent, and lists passing checks, but a reader still cannot reconstruct the critical branch, state transition, side effect, consumer, or failure path from the review. | Exact reviewed head, changed production symbols, surrounding definitions and active call sites, published five-block review, and packet explanation-depth contract. | The review contract asked for mechanism-rich prose but did not make exact-head key-code explanation a required subsection, so agents could satisfy the headings with a polished file inventory. | Require `关键代码讲解` under `具体改动` for code-changing PRs; select 2-5 behavior-bearing symbols, cite exact-head lines, use short excerpts or equivalent pseudocode, and explain inputs/pre-state, branch/invariant, calls/side effects, outputs/consumers, and failure ownership. Keep docs-only reviews on a parallel key-content path and cover both the skill text and CLI packet in the PR-review smoke. | | `pr_review_proportionality_goal_drift` | A narrow or infrequent failure attracts a large production mechanism; repeated reviews find real implementation gaps, each requested fix grows the PR, and the final review approves because the mechanism is coherent and CI is green even though the maintenance cost no longer fits the original benefit. | Original user-visible problem and recovery cost, first and current exact-head diff shapes, production/state/schema/CLI/caller growth, review findings over time, alternatives and smallest viable fix, and the final proportionality verdict. | Scope-fit proved the code had active callers and code-volume evidence remained descriptive, while verdict policy had no blocking benefit-to-complexity gate. Re-review optimized closure of the latest finding and inherited an approval trajectory instead of resetting from the original problem. | Require typed `change_proportionality` evidence for every code change. Compare verified frequency/severity/blast radius/recovery with production mechanism and long-term maintenance; make `disproportionate` and `not_yet_proven` blocking regardless of correctness or green CI. After material scope growth, reset the whole exact-head review from the original problem and prefer the smallest viable fix, deletion, split, or hold. Cover the projected contract and code-change/docs-only applicability with focused tests. | | `pr_review_feature_gate_counterfactual_gap` | A review proves the feature-on path and approves an opt-in or default-off change, but users who never enable it still receive new schema requirements, prompt instructions, accepted inputs, persisted projections, scheduling decisions, or effects; scoped sub-agent behavior may also be published under a broader multi-agent protocol name. | Authoritative gate and default, identical disabled/enabled fixtures, pre-change disabled output, every shared changed builder or serializer, emitted schema/prompt/journal/effect differences, public protocol ids, actual actor lifecycle, and granted or excluded authority. | The review inferred whole-change isolation from the absence of one enabled topology or operation list, and treated broad protocol terminology as cosmetic even when it implied registered peers or durable coordination that the implementation did not grant. Feature-on tests then encoded the default-off leakage as expected behavior. | Require typed `default_off_isolation` and `authority_semantics` evidence for every code change, with explicit `not_applicable` only after checking scope. Trace all shared surfaces, run a paired feature-off/feature-on counterfactual, compare the disabled side with the pre-change contract, and block `not_isolated`, `misleading`, or `not_yet_proven`. Prefer established sub-agent/child terminology for ephemeral scoped execution and rename unshipped v0 protocols before compatibility cost accumulates. | diff --git a/tests/capabilities/test_pr_review_queue.py b/tests/capabilities/test_pr_review_queue.py index a8b45a4100..53f334e899 100644 --- a/tests/capabilities/test_pr_review_queue.py +++ b/tests/capabilities/test_pr_review_queue.py @@ -10,6 +10,12 @@ materialize_review_execution, scheduling_tier, ) +from loopx.capabilities.pr_review_queue.readiness_observation import ( + MERGE_READINESS_OBSERVATION_SCHEMA_VERSION, + observation_key, + readiness_material_fingerprint, + readiness_material_state, +) def _concluded_item( @@ -445,8 +451,8 @@ def test_approved_transition_routes_to_merge_policy_without_granting_it() -> Non assert approved["write_authority_granted"] is False -def test_open_head_merge_readiness_follows_typed_verdict() -> None: - """An approval keeps owing the pre-merge gate while the PR is open. +def test_open_head_without_observation_routes_to_typed_merge_readiness() -> None: + """An approval owes the pre-merge gate until its material state is observed. GitHub blocks self-approval, so an author-owned approval is stored as a COMMENTED review. Keying the queue on the formal review state counted such a @@ -489,6 +495,98 @@ def test_open_head_merge_readiness_follows_typed_verdict() -> None: assert _action_kind(changes_requested) == "rereview_pull_request_exact_head" +def test_readiness_observation_suppresses_only_an_exact_material_match() -> None: + repository = "owner/repo" + threads = { + "complete": True, + "total_count": 1, + "unresolved_count": 0, + } + item = _concluded_item( + conclusion={ + "valid": True, + "status": "valid", + "state": "APPROVED", + "verdict": "APPROVE", + "review_commit": "1" * 40, + "invalid_reasons": [], + }, + decision="APPROVED", + ) | { + "base_oid": "a" * 40, + "merge_state": "CLEAN", + "checks": { + "total": 1, + "counts": {"success": 1, "failure": 0, "pending": 0, "unknown": 0}, + "failures": [], + "pending": [], + }, + "wait_for_ci": True, + } + material = readiness_material_state( + repository=repository, + item=item, + review_threads=threads, + wait_for_ci=True, + ) + exact_head = f"1@{item['head_oid']}" + observations = { + observation_key(repository, exact_head): { + "schema_version": MERGE_READINESS_OBSERVATION_SCHEMA_VERSION, + "material_fingerprint": readiness_material_fingerprint(material), + } + } + + unchanged = materialize_review_execution( + item, + fresh_audit_exact_heads=set(), + readiness_observations=observations, + repository=repository, + review_threads=threads, + ) + assert unchanged["review_action_kind"] is None + assert ( + unchanged["merge_readiness_observation"]["observation_state"] + == "observed_unchanged" + ) + + mutations = [] + changed_head = deepcopy(item) + changed_head["head_oid"] = "2" * 40 + mutations.append((changed_head, threads)) + changed_base = deepcopy(item) + changed_base["base_oid"] = "b" * 40 + mutations.append((changed_base, threads)) + changed_review = deepcopy(item) + changed_review["review_conclusion"]["review_commit"] = "2" * 40 + mutations.append((changed_review, threads)) + changed_checks = deepcopy(item) + changed_checks["checks"]["counts"]["success"] = 0 + changed_checks["checks"]["counts"]["failure"] = 1 + changed_checks["checks"]["failures"] = ["merge-gate"] + mutations.append((changed_checks, threads)) + changed_threads = dict(threads, unresolved_count=1) + mutations.append((deepcopy(item), changed_threads)) + changed_merge_state = deepcopy(item) + changed_merge_state["merge_state"] = "BEHIND" + mutations.append((changed_merge_state, threads)) + + for changed_item, changed_thread_state in mutations: + execution = materialize_review_execution( + changed_item, + fresh_audit_exact_heads=set(), + readiness_observations=observations, + repository=repository, + review_threads=changed_thread_state, + ) + assert execution["review_action_kind"] == "qualify_pull_request_merge_readiness" + if changed_item["head_oid"] == item["head_oid"]: + assert ( + execution["merge_readiness_observation"]["observation_state"] + == "material_transition" + ) + + def test_review_backlog_keeps_active_cadence_until_all_handled() -> None: first = _observe([_pr(1), _pr(2), _pr(3)]) diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index abddbb2b59..f1b624262b 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -536,11 +536,25 @@ def _merge_readiness_args(**overrides: object) -> SimpleNamespace: "repo": "owner/repo", "since": None, "fresh_audit_exact_head": [], + "target_exact_head": [], + "goal_id": "test-goal", + "limit": 100, + "state": "open", + "review_priority": None, } values.update(overrides) return SimpleNamespace(**values) +def _goal_registry(tmp_path: Path) -> Path: + path = tmp_path / "registry.json" + path.write_text( + json.dumps({"goals": [{"id": "test-goal", "repo": str(tmp_path)}]}), + encoding="utf-8", + ) + return path + + def _capture_payload(out: list[dict[str, object]]): def capture( payload: dict[str, object], @@ -571,6 +585,8 @@ def test_merge_readiness_cli_qualifies_one_fixture_exact_head( result = pr_review_cli_module.handle_pr_review_command( _merge_readiness_args(fixture=str(fixture_path), repo=None), + runtime_root=tmp_path, + registry_path=_goal_registry(tmp_path), output_format=lambda _args: "json", print_payload=_capture_payload(out), ) @@ -583,6 +599,7 @@ def test_merge_readiness_cli_qualifies_one_fixture_exact_head( def test_merge_readiness_cli_reads_live_pr_and_threads( monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, ) -> None: monkeypatch.setattr( pr_review_cli_module, @@ -603,6 +620,8 @@ def test_merge_readiness_cli_reads_live_pr_and_threads( result = pr_review_cli_module.handle_pr_review_command( _merge_readiness_args(), + runtime_root=tmp_path, + registry_path=_goal_registry(tmp_path), output_format=lambda _args: "json", print_payload=_capture_payload(out), ) @@ -613,6 +632,146 @@ def test_merge_readiness_cli_reads_live_pr_and_threads( assert out[0]["review_conclusion"]["reviewer"] == "maintainer" +def test_goal_readiness_observation_suppresses_only_unchanged_exact_head( + tmp_path: Path, +) -> None: + fixture_path = tmp_path / "pull-request.json" + pull_request = _merge_ready_pr() + pull_request["review_thread_summary"] = _complete_review_threads() + fixture_path.write_text( + json.dumps( + { + "repository": "owner/repo", + "reviewer_login": "maintainer", + "pull_requests": [pull_request], + } + ), + encoding="utf-8", + ) + registry_path = _goal_registry(tmp_path) + readiness: list[dict[str, object]] = [] + assert pr_review_cli_module.handle_pr_review_command( + _merge_readiness_args(fixture=str(fixture_path), repo=None), + runtime_root=tmp_path, + registry_path=registry_path, + output_format=lambda _args: "json", + print_payload=_capture_payload(readiness), + ) == 0 + assert readiness[0]["local_goal_observation_write_performed"] is True + + def queue_args() -> SimpleNamespace: + return _merge_readiness_args( + check_merge_readiness=None, + fixture=str(fixture_path), + repo=None, + ) + + unchanged: list[dict[str, object]] = [] + assert pr_review_cli_module.handle_pr_review_command( + queue_args(), + runtime_root=tmp_path, + registry_path=registry_path, + output_format=lambda _args: "json", + print_payload=_capture_payload(unchanged), + ) == 0 + item = unchanged[0]["pull_requests"][0] + assert item["review_action_kind"] is None + assert ( + item["merge_readiness_observation"]["observation_state"] + == "observed_unchanged" + ) + + pull_request["baseRefOid"] = "c" * 40 + fixture_path.write_text( + json.dumps( + { + "repository": "owner/repo", + "reviewer_login": "maintainer", + "pull_requests": [pull_request], + } + ), + encoding="utf-8", + ) + changed: list[dict[str, object]] = [] + assert pr_review_cli_module.handle_pr_review_command( + queue_args(), + runtime_root=tmp_path, + registry_path=registry_path, + output_format=lambda _args: "json", + print_payload=_capture_payload(changed), + ) == 0 + changed_item = changed[0]["pull_requests"][0] + assert changed_item["review_action_kind"] == "qualify_pull_request_merge_readiness" + assert ( + changed_item["merge_readiness_observation"]["observation_state"] + == "material_transition" + ) + + +def test_head_drift_observation_does_not_suppress_the_new_remote_head( + tmp_path: Path, +) -> None: + fixture_path = tmp_path / "pull-request.json" + pull_request = _merge_ready_pr() + pull_request["review_thread_summary"] = _complete_review_threads() + current_head = str(pull_request["headRefOid"]) + fixture_path.write_text( + json.dumps( + { + "repository": "owner/repo", + "reviewer_login": "maintainer", + "pull_requests": [pull_request], + } + ), + encoding="utf-8", + ) + registry_path = _goal_registry(tmp_path) + readiness: list[dict[str, object]] = [] + stale_head = "d" * 40 + assert pr_review_cli_module.handle_pr_review_command( + _merge_readiness_args( + fixture=str(fixture_path), + repo=None, + check_merge_readiness=f"4110@{stale_head}", + ), + runtime_root=tmp_path, + registry_path=registry_path, + output_format=lambda _args: "json", + print_payload=_capture_payload(readiness), + ) == 1 + assert "remote_head_mismatch" in readiness[0]["blocking_reasons"] + assert readiness[0]["readiness_observation"]["exact_head"] == f"4110@{stale_head}" + + queue: list[dict[str, object]] = [] + assert pr_review_cli_module.handle_pr_review_command( + _merge_readiness_args( + fixture=str(fixture_path), + repo=None, + check_merge_readiness=None, + ), + runtime_root=tmp_path, + registry_path=registry_path, + output_format=lambda _args: "json", + print_payload=_capture_payload(queue), + ) == 0 + item = queue[0]["pull_requests"][0] + assert item["head_oid"] == current_head + assert item["review_action_kind"] == "qualify_pull_request_merge_readiness" + assert item["merge_readiness_observation"] is None + + +def test_merge_readiness_requires_goal_scope() -> None: + out: list[dict[str, object]] = [] + result = pr_review_cli_module.handle_pr_review_command( + _merge_readiness_args(goal_id=None), + output_format=lambda _args: "json", + print_payload=_capture_payload(out), + ) + + assert result == 1 + assert out[0]["error"] == "merge readiness requires --goal-id" + + def test_pr_review_cli_uses_machine_capability_priority_when_flag_is_omitted( tmp_path: Path, ) -> None: