diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index cdd8195617..5e08669fac 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -90,6 +90,7 @@ def main() -> int: "Never send the projection ACK before the Todo exists", "Generic `re-review`, `重新review`, and `复审` wording selects the named PR; it is not a force-refresh token.", "the row stays in `pull_requests` inventory but must not appear in `review_sequence`", + "--target-exact-head NUMBER@HEAD_OID", ): assert phrase in skill_text, phrase assert len(skill_source.splitlines()) <= 180, len(skill_source.splitlines()) @@ -229,7 +230,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert request["command"] == "/loopx-pr-review", request assert ( request["cli_command"] - == "loopx pr-review [--repo owner/repo] [--state open|merged|all] [--review-priority other-developers-first|owner-first] [--since ISO]" + == "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]" ), request assert request["privacy_mode"] == "public_safe_github_metadata", request assert request["dry_run"] is True, request @@ -253,6 +254,18 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert payload["summary"]["total_pr_count"] == 4, payload["summary"] assert payload["summary"]["open_pr_count"] == 3, payload["summary"] assert payload["summary"]["merged_pr_count"] == 1, payload["summary"] + target = payload["pull_requests"][0] + exact_target = f"{target['number']}@{target['head_oid']}" + targeted = json.loads( + run_cli( + "--format", "json", "pr-review", "--fixture", str(FIXTURE), + "--target-exact-head", exact_target, + ).stdout + ) + assert targeted["request"]["target_exact_heads"] == [exact_target], targeted + assert targeted["result_completeness"]["complete"] is True, targeted + assert targeted["result_completeness"]["limit_scope"] == "exact_targets", targeted + assert [item["number"] for item in targeted["pull_requests"]] == [target["number"]] assert payload["summary"]["post_merge_review_count"] == 1, payload["summary"] assert payload["summary"]["review_attention_count"] == 3, payload["summary"] assert payload["summary"]["draft_count"] == 1, payload["summary"] diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 3996ed27b1..df3226815a 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -46,7 +46,7 @@ workflow or the merge-focused `loopx-pr-merge` skill. | Command | CLI reference | Intent | | --- | --- | --- | -| `/loopx-pr-review` | `loopx pr-review [--repo owner/repo] [--state open\|merged\|all] [--review-priority other-developers-first\|owner-first] [--since ISO] [--fresh-audit-exact-head NUMBER@HEAD_OID]` | List open and merged PRs for the current project or explicit repository, provide concrete main-regression analysis for each actionable PR, and include a blank five-block template that agentloop fills after reading the selected PR body/diff. The default prioritizes non-owner developer PRs; `owner-first` opts into owner priority. A typed exact-head option is required to re-audit an unchanged concluded head. | +| `/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. | The slash command must run the CLI first. Agentloop must not reconstruct the @@ -62,6 +62,20 @@ templates enter the model context: loopx --format json pr-review --state all [--repo owner/repo] [--since ISO] ``` +When the user explicitly names one or a few PRs, resolve each current head and +request only those exact heads. This direct path is complete for the named +targets and must not be expanded into a historical queue merely to satisfy +queue completeness: + +```bash +loopx --format json pr-review --repo owner/repo \ + --target-exact-head 4868@0123456789abcdef0123456789abcdef01234567 +``` + +The option is repeatable. A remote head mismatch fails closed. Use +`--fresh-audit-exact-head` in addition only when an unchanged target already has +a valid conclusion and the user explicitly requests new evidence. + The live source scan keeps the list query lightweight and enriches each PR's nested commits, reviews, and checks with a bounded pool of concurrent `gh pr view` reads (at most eight at a time). Results are reassembled in list diff --git a/loopx/capabilities/pr_review_queue/github_source.py b/loopx/capabilities/pr_review_queue/github_source.py index ca1045fa3f..1d4de6633b 100644 --- a/loopx/capabilities/pr_review_queue/github_source.py +++ b/loopx/capabilities/pr_review_queue/github_source.py @@ -9,6 +9,8 @@ from pathlib import Path from typing import Any, Callable +from .selection_execution import normalize_fresh_audit_exact_heads + GitHubJsonRunner = Callable[..., Any] DETAIL_FIELDS = ( @@ -21,6 +23,26 @@ "reviews", ) +PR_LIST_FIELDS = ( + "number", + "title", + "url", + "state", + "isDraft", + "headRefName", + "headRefOid", + "baseRefName", + "author", + "createdAt", + "updatedAt", + "closedAt", + "mergedAt", + "mergeCommit", + "changedFiles", + "additions", + "deletions", +) + def run_gh_json(args: list[str], *, cwd: Path | None = None) -> Any: proc = subprocess.run( @@ -140,6 +162,80 @@ def attach_pr_review_details( return True +def scan_github_pull_request_targets( + *, + repository: str, + exact_heads: Sequence[str], + cwd: Path | None = None, + run_gh_json: GitHubJsonRunner = run_gh_json, + wait_for_ci: bool = True, +) -> dict[str, Any]: + """Read only explicitly requested exact heads, without scanning a queue.""" + + targets = normalize_fresh_audit_exact_heads(exact_heads) + if not targets: + raise ValueError("at least one target exact head is required") + + pull_requests: list[dict[str, Any]] = [] + for target in sorted( + targets, key=lambda item: (int(item.split("@", 1)[0]), item) + ): + number, expected_head = target.split("@", 1) + try: + row = run_gh_json( + [ + "pr", + "view", + number, + "--json", + ",".join(PR_LIST_FIELDS), + "--repo", + repository, + ], + cwd=cwd, + ) + except Exception as exc: + raise RuntimeError(f"target PR #{number} metadata read failed") from exc + if not isinstance(row, dict): + raise RuntimeError(f"target PR #{number} metadata read was not an object") + actual_head = str(row.get("headRefOid") or "").strip().lower() + if actual_head != expected_head: + raise ValueError( + f"target PR #{number} head changed: expected {expected_head}, " + f"remote is {actual_head or 'unavailable'}" + ) + details_ok = attach_pr_review_details( + row, + repository=repository, + cwd=cwd, + **({"wait_for_ci": False} if not wait_for_ci else {}), + run_gh_json=run_gh_json, + ) + if not details_ok: + raise RuntimeError(f"target PR #{number} detail read was incomplete") + pull_requests.append(row) + + return { + "schema_version": "pr_review_source_scan_v0", + "complete": True, + "mode": "exact_targets", + "requested_exact_heads": sorted(targets), + "observed_exact_heads": sorted(targets), + "pull_requests": pull_requests, + "states": [ + { + "state": "exact_targets", + "fetch_limit": len(targets), + "fetched_count": len(pull_requests), + "included_after_window": len(pull_requests), + "detail_read_failures": 0, + "source_saturated": False, + "source_read_valid": True, + } + ], + } + + PR_REVIEW_DETAIL_MAX_WORKERS = 8 diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index 22ef96b9a5..147ba65b3c 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -18,6 +18,9 @@ ) 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.github_source import ( + scan_github_pull_request_targets, +) from ..file_lock import exclusive_file_lock from ..pr_review import ( build_pr_review_packet, @@ -148,6 +151,16 @@ def register_pr_review_command( "--fixture", help="Read public-safe PR metadata from a JSON fixture instead of live gh output.", ) + parser.add_argument( + "--target-exact-head", + action="append", + default=[], + metavar="NUMBER@HEAD_OID", + help=( + "Read only this exact PR head instead of scanning a lifecycle queue. " + "Repeatable for a small explicit review batch." + ), + ) parser.add_argument( "--fresh-audit-exact-head", action="append", @@ -208,6 +221,7 @@ def handle_pr_review_command( return None checkpoint_path: Path | None = None resolved_review_priority = DEFAULT_REVIEW_PRIORITY + target_exact_heads = list(getattr(args, "target_exact_head", []) or []) try: machine_configuration = (read_machine_configuration(runtime_root, registry=build_builtin_machine_configuration_registry()) if runtime_root is not None else None) goal = None @@ -233,6 +247,7 @@ def handle_pr_review_command( or args.repo or args.since or args.fresh_audit_exact_head + or target_exact_heads or args.check_merge_readiness ): raise ValueError( @@ -263,6 +278,7 @@ def handle_pr_review_command( or args.projected_exact_head or args.since or args.fresh_audit_exact_head + or target_exact_heads ): raise ValueError( "merge readiness cannot be combined with queue or observation options" @@ -349,6 +365,15 @@ def handle_pr_review_command( "--observation-state-file cannot be combined with " "--previous-observation-json" ) + if target_exact_heads and args.autonomous_observation: + raise ValueError( + "--target-exact-head cannot be combined with --autonomous-observation" + ) + if target_exact_heads and args.since: + raise ValueError( + "--target-exact-head already defines the review window and " + "cannot be combined with --since" + ) explicit_review_priority = getattr(args, "review_priority", None) if explicit_review_priority is not None: resolved_review_priority = normalize_review_priority(explicit_review_priority) @@ -372,6 +397,19 @@ def handle_pr_review_command( Path(args.fixture).expanduser() ) repository = repository or repository_from_fixture + if target_exact_heads: + requested_targets = normalize_fresh_audit_exact_heads( + target_exact_heads + ) + pull_requests = [ + item + for item in pull_requests + if ( + f"{item.get('number')}@" + f"{str(item.get('headRefOid') or '').lower()}" + in requested_targets + ) + ] source = "fixture" source_scan = None else: @@ -382,13 +420,23 @@ def handle_pr_review_command( "authenticated GitHub reviewer identity is required for " "autonomous author-owned scheduling" ) - source_scan = scan_github_pull_requests( - repo=repository, - limit=max(1, args.limit) + 1, - state_filter=normalize_pr_state_filter(args.state), - since=args.since, - **({"wait_for_ci": False} if not wait_for_ci else {}), - ) + if target_exact_heads: + if not repository: + raise RuntimeError("GitHub repository could not be resolved") + source_scan = scan_github_pull_request_targets( + repository=repository, + exact_heads=target_exact_heads, + **({"wait_for_ci": False} if not wait_for_ci else {}), + ) + source = "github_cli_exact_targets" + else: + source_scan = scan_github_pull_requests( + repo=repository, + limit=max(1, args.limit) + 1, + state_filter=normalize_pr_state_filter(args.state), + since=args.since, + **({"wait_for_ci": False} if not wait_for_ci else {}), + ) pull_requests = source_scan["pull_requests"] if checkpoint_path is not None and previous_observation: checkpoint_repository = str( @@ -411,6 +459,7 @@ def handle_pr_review_command( source_scan=source_scan, reviewer_login=reviewer_login, fresh_audit_exact_heads=args.fresh_audit_exact_head, + target_exact_heads=target_exact_heads, review_priority=resolved_review_priority, wait_for_ci=wait_for_ci, ) @@ -465,13 +514,14 @@ def handle_pr_review_command( "request": { "schema_version": "loopx_pr_review_command_request_v0", "command": "/loopx-pr-review", - "cli_command": "loopx pr-review [--repo owner/repo] [--state open|merged|all] [--review-priority other-developers-first|owner-first] [--since ISO]", + "cli_command": "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]", "repository": args.repo, "limit": max(1, args.limit), "state_filter": normalize_pr_state_filter(args.state), "since": args.since, "review_priority": resolved_review_priority.value, "fresh_audit_exact_heads": list(args.fresh_audit_exact_head), + "target_exact_heads": target_exact_heads, "source": "fixture" if args.fixture else "github_cli", "privacy_mode": "public_safe_github_metadata", "dry_run": True, diff --git a/loopx/pr_review.py b/loopx/pr_review.py index b38ab12670..698268a183 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -23,6 +23,7 @@ scheduling_tier, ) from .capabilities.pr_review_queue.github_source import ( + PR_LIST_FIELDS, attach_pr_review_details as _attach_pr_review_details, ) from .capabilities.pr_review_queue.github_source import run_gh_json as _run_gh_json @@ -230,25 +231,6 @@ def scan_github_pull_requests( search_date = _github_search_date(since) if search_date: search_args = ["--search", f"updated:>={search_date}"] - list_fields = [ - "number", - "title", - "url", - "state", - "isDraft", - "headRefName", - "headRefOid", - "baseRefName", - "author", - "createdAt", - "updatedAt", - "closedAt", - "mergedAt", - "mergeCommit", - "changedFiles", - "additions", - "deletions", - ] fetch_limit = max(1, limit) if since: fetch_limit = max(fetch_limit, min(100, fetch_limit * 3)) @@ -267,7 +249,7 @@ def append_state(state: str) -> None: "--limit", str(fetch_limit), "--json", - ",".join(list_fields), + ",".join(PR_LIST_FIELDS), *search_args, *repo_args, ], @@ -1045,6 +1027,7 @@ def build_pr_review_packet( source_scan: Mapping[str, Any] | None = None, reviewer_login: str | None = None, fresh_audit_exact_heads: Sequence[str] = (), + target_exact_heads: Sequence[str] = (), review_priority: object = DEFAULT_REVIEW_PRIORITY, wait_for_ci: bool = True, ) -> dict[str, Any]: @@ -1053,6 +1036,7 @@ def build_pr_review_packet( generated_at_text = _now_iso() generated_at = _parse_timestamp(generated_at_text) or datetime.now(timezone.utc) requested_fresh_audits = normalize_fresh_audit_exact_heads(fresh_audit_exact_heads) + requested_targets = normalize_fresh_audit_exact_heads(target_exact_heads) normalized_all = [ _normalize_pr( item, @@ -1075,10 +1059,14 @@ def build_pr_review_packet( item, review_priority=normalized_priority ) ) - packet_limit = max(1, limit) + packet_limit = len(requested_targets) if requested_targets else max(1, limit) unmerged_all = [item for item in normalized_all if str(item.get("state") or "").upper() != "MERGED"] merged_all = [item for item in normalized_all if str(item.get("state") or "").upper() == "MERGED"] - if normalized_state_filter == "all": + if requested_targets: + normalized = normalized_all + unmerged_items = unmerged_all + merged_items = merged_all + elif normalized_state_filter == "all": unmerged_items = unmerged_all[:packet_limit] merged_items = merged_all[:packet_limit] normalized = unmerged_items + merged_items @@ -1089,6 +1077,12 @@ def build_pr_review_packet( observed_exact_heads = { key for item in normalized if (key := exact_head_key(item)) } + missing_targets = requested_targets - observed_exact_heads + if missing_targets: + raise ValueError( + "target exact head is absent from the current result: " + + ", ".join(sorted(missing_targets)) + ) missing_fresh_audits = requested_fresh_audits - observed_exact_heads if missing_fresh_audits: raise ValueError( @@ -1132,7 +1126,13 @@ def build_pr_review_packet( "complete": complete, "truncated": not complete, "limit": packet_limit, - "limit_scope": "per_group" if normalized_state_filter == "all" else "filtered_queue", + "limit_scope": ( + "exact_targets" + if requested_targets + else "per_group" + if normalized_state_filter == "all" + else "filtered_queue" + ), "source_scan_complete": source_scan_complete, "observed_count_is_lower_bound": not source_scan_complete, "observed_pr_count": len(normalized_all), @@ -1222,7 +1222,7 @@ def build_pr_review_packet( "request": { "schema_version": "loopx_pr_review_command_request_v0", "command": COMMAND, - "cli_command": "loopx pr-review [--repo owner/repo] [--state open|merged|all] [--review-priority other-developers-first|owner-first] [--since ISO]", + "cli_command": "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]", "repository": repository, "limit": max(1, limit), "state_filter": normalized_state_filter, @@ -1235,6 +1235,7 @@ def build_pr_review_packet( "reviewer_login": reviewer_login, "review_priority": normalized_priority.value, "fresh_audit_exact_heads": sorted(requested_fresh_audits), + "target_exact_heads": sorted(requested_targets), "include": [ "pull_request_list", "result_completeness", diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index f2576d12ca..130831e01d 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -17,14 +17,14 @@ state or time window. Route approval, merge, self-merge, and admin bypass to `loopx-pr-merge` (optional repo-kept workflow, not installed by default) after the evidence review is complete; it never replaces this skill's exact-head gate. -Run `loopx --format json pr-review --state all` before ad hoc GitHub reads. +For named PRs, resolve heads and run repeatable `--target-exact-head NUMBER@HEAD_OID`; run `loopx --format json pr-review --state all` only for queue intent, never to expand explicit targets into historical inventory. Translate only explicit filters: - `--repo owner/repo` - `--since ISO` - `--state open|merged|all` -- `--limit N` +- `--limit N`; `--target-exact-head NUMBER@HEAD_OID` (repeatable direct read) - `--review-priority other-developers-first|owner-first` (default `other-developers-first`; use `owner-first` to opt into owner priority) When omitted, the CLI resolves `pull_request_review` from the standard machine capability editor; an absent namespace keeps the default `other-developers-first`. @@ -43,9 +43,9 @@ paths named by `agent_response_contract.required_packet_fields_to_preserve`: - `pull_requests[review_action_kind!=null].review_template` - `pull_requests[review_action_kind!=null].evidence_commands` -Do not pipe the only copy through `jq`. When an exhaustive request has +Do not pipe the only copy through `jq`. When an exhaustive queue request has `result_completeness.complete=false`, rerun with its `recommended_limit` before -reviewing. +reviewing; `limit_scope=exact_targets` is already complete for the named targets. Require the revision the installed capability declares, not a literal this file pins: read `review_execution_contract.policy_revision` from the packet and require the result's `review_policy_revision` to equal it. diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index 86496bc363..c7bc8299e6 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -276,6 +276,48 @@ def fake(args: list[str], *, cwd: Path | None = None): assert "files" not in scan["pull_requests"][0] +def test_exact_target_read_skips_lifecycle_queue_scan(monkeypatch) -> None: + calls: list[list[str]] = [] + + def fake(args: list[str], *, cwd: Path | None = None): + calls.append(args) + assert args[:2] == ["pr", "view"] + requested_fields = set(args[args.index("--json") + 1].split(",")) + if "number" in requested_fields: + return _rows()[int(args[2]) - 1] + return _fake_run_gh_json(args, cwd=cwd) + + scan = github_source_module.scan_github_pull_request_targets( + repository="owner/repo", + exact_heads=[f"1@{HEAD_1}", f"2@{HEAD_2}"], + run_gh_json=fake, + ) + + assert scan["complete"] is True + assert scan["mode"] == "exact_targets" + assert scan["requested_exact_heads"] == [f"1@{HEAD_1}", f"2@{HEAD_2}"] + assert [row["number"] for row in scan["pull_requests"]] == [1, 2] + assert len(calls) == 4 + assert all(call[:2] == ["pr", "view"] for call in calls) + assert not any(call[:2] == ["pr", "list"] for call in calls) + + +def test_exact_target_read_fails_closed_when_remote_head_changed( + monkeypatch, +) -> None: + def fake(args: list[str], *, cwd: Path | None = None): + row = _rows()[0] + row["headRefOid"] = HEAD_2 + return row + + with pytest.raises(ValueError, match="head changed"): + github_source_module.scan_github_pull_request_targets( + repository="owner/repo", + exact_heads=[f"1@{HEAD_1}"], + run_gh_json=fake, + ) + + def test_security_policy_keeps_public_entry_classification_after_move() -> None: assert pr_review_module._file_area(".github/SECURITY.md") == ( "public_entry_or_policy" @@ -984,6 +1026,45 @@ def test_queue_prioritizes_other_developers_by_default(monkeypatch) -> None: assert packet["scheduling_policy"]["other_developers_first_active"] is True +def test_exact_target_packet_is_complete_without_queue_inventory( + monkeypatch, +) -> None: + monkeypatch.setattr(pr_review_module, "_now_iso", lambda: "2026-08-18T12:00:00Z") + row = _queue_pr( + 31, + author="maintainer", + ready_at="2026-08-17T06:00:00Z", + updated_at="2026-08-18T11:58:00Z", + ) + exact_head = f"31@{row['headRefOid']}" + + packet = pr_review_module.build_pr_review_packet( + pull_requests=[row], + repository="owner/repo", + limit=100, + source="github_cli_exact_targets", + target_exact_heads=[exact_head], + reviewer_login="maintainer", + ) + + assert packet["request"]["target_exact_heads"] == [exact_head] + assert packet["result_completeness"]["complete"] is True + assert packet["result_completeness"]["limit_scope"] == "exact_targets" + assert packet["result_completeness"]["limit"] == 1 + assert packet["result_completeness"]["recommended_limit"] is None + assert [item["number"] for item in packet["pull_requests"]] == [31] + + with pytest.raises(ValueError, match="target exact head is absent"): + pr_review_module.build_pr_review_packet( + pull_requests=[row], + repository="owner/repo", + limit=100, + source="fixture", + target_exact_heads=[f"32@{HEAD_2}"], + reviewer_login="maintainer", + ) + + def test_community_feedback_and_aged_backlog_precede_remaining_queue( monkeypatch, ) -> None: