From c5a017408551cdb344d06abb4a2992638f8d50ac Mon Sep 17 00:00:00 2001 From: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Date: Tue, 22 Sep 2026 02:54:46 +0800 Subject: [PATCH] fix(pr-review): default queue scans to open PRs Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> --- examples/bootstrap-command-pack-smoke.py | 3 +- examples/install-local-smoke.py | 3 +- examples/pr-review-command-smoke.py | 31 ++++++++++--- loopx/capabilities/pr_review_queue/README.md | 12 +++-- loopx/cli_commands/pr_review.py | 31 ++++++++++--- loopx/doctor.py | 3 +- loopx/pr_review.py | 12 ++--- skills/loopx-pr-review/SKILL.md | 2 +- .../references/repair-patterns.md | 1 + tests/test_pr_review_github_scan.py | 45 +++++++++++++++++++ 10 files changed, 117 insertions(+), 26 deletions(-) diff --git a/examples/bootstrap-command-pack-smoke.py b/examples/bootstrap-command-pack-smoke.py index fef295914a..0987074019 100644 --- a/examples/bootstrap-command-pack-smoke.py +++ b/examples/bootstrap-command-pack-smoke.py @@ -710,7 +710,8 @@ def test_skill_slash_fallback_contract() -> None: assert "Do not handle `/loopx-pr-review` from this broader project skill" in normalized assert "do not route it to `loopx-pr-merge` unless" in normalized assert "loopx --format json pr-review --state all" not in skill_text - assert "loopx --format json pr-review --state all" in pr_review_skill_text + assert "keeps ordinary queue discovery open-only" in pr_review_skill_text + assert "explicit `--state merged|all`" in pr_review_skill_text assert "Save the full first JSON packet before printing a compact projection" in pr_review_normalized assert "agent_response_contract" in pr_review_skill_text assert "pull_requests[review_action_kind!=null].review_template" in pr_review_skill_text diff --git a/examples/install-local-smoke.py b/examples/install-local-smoke.py index 2eeae4d0c3..f8a0446a0a 100644 --- a/examples/install-local-smoke.py +++ b/examples/install-local-smoke.py @@ -418,7 +418,8 @@ def main() -> int: pr_review_skill = codex_home / "skills" / "loopx-pr-review" / "SKILL.md" pr_review_text = " ".join(pr_review_skill.read_text(encoding="utf-8").split()) for phrase in ( - "loopx --format json pr-review --state all", + "keeps ordinary queue discovery open-only", + "explicit `--state merged|all`", "thin host adapter", "agent_response_contract.review_execution_contract", "review_groups", diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 5e08669fac..275d6b390c 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -64,7 +64,8 @@ def main() -> int: skill_text = " ".join(skill_source.split()) for phrase in ( "This skill is a thin host adapter", - "loopx --format json pr-review --state all", + "keeps ordinary queue discovery open-only", + "explicit `--state merged|all`", "agent_response_contract.review_execution_contract", "pull_requests[review_action_kind!=null].review_plan", "pull_requests[review_action_kind!=null].review_template", @@ -222,7 +223,8 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: payload = json.loads( run_cli( - "--format", "json", "pr-review", "--fixture", str(FIXTURE), "--limit", "5" + "--format", "json", "pr-review", "--fixture", str(FIXTURE), + "--state", "all", "--limit", "5" ).stdout ) assert payload["schema_version"] == "loopx_pr_review_command_response_v0", payload @@ -254,7 +256,15 @@ 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] + default_open = json.loads( + run_cli( + "--format", "json", "pr-review", "--fixture", str(FIXTURE), "--limit", "5" + ).stdout + ) + assert default_open["request"]["state_filter"] == "open", default_open["request"] + assert default_open["summary"]["total_pr_count"] == 3, default_open["summary"] + assert default_open["summary"]["merged_pr_count"] == 0, default_open["summary"] + target = next(item for item in payload["pull_requests"] if item["number"] == 770) exact_target = f"{target['number']}@{target['head_oid']}" targeted = json.loads( run_cli( @@ -263,6 +273,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: ).stdout ) assert targeted["request"]["target_exact_heads"] == [exact_target], targeted + assert targeted["request"]["state_filter"] == "all", targeted["request"] 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"]] @@ -1152,7 +1163,8 @@ def approved_open_head( group_limited = json.loads( run_cli( - "--format", "json", "pr-review", "--fixture", str(FIXTURE), "--limit", "1" + "--format", "json", "pr-review", "--fixture", str(FIXTURE), + "--state", "all", "--limit", "1" ).stdout ) assert group_limited["summary"]["total_pr_count"] == 2, group_limited["summary"] @@ -1192,6 +1204,8 @@ def approved_open_head( "pr-review", "--fixture", str(FIXTURE), + "--state", + "all", "--since", "2026-06-27T12:20:00Z", "--limit", @@ -1206,7 +1220,14 @@ def approved_open_head( "review_sequence" ] - markdown = run_cli("pr-review", "--fixture", str(FIXTURE), "--limit", "1").stdout + default_markdown = run_cli( + "pr-review", "--fixture", str(FIXTURE), "--limit", "1" + ).stdout + assert "state_filter: `open`" in default_markdown, default_markdown + assert "#770" not in default_markdown, default_markdown + markdown = run_cli( + "pr-review", "--fixture", str(FIXTURE), "--state", "all", "--limit", "1" + ).stdout assert "# Project PR Review Queue" in markdown, markdown assert "current gh repository" not in markdown, markdown assert "state_filter: `all`" in markdown, markdown diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index df3226815a..574bf0427a 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -59,9 +59,13 @@ Use the JSON form for the first pass so the response contract and per-PR blank templates enter the model context: ```bash -loopx --format json pr-review --state all [--repo owner/repo] [--since ISO] +loopx --format json pr-review [--repo owner/repo] [--since ISO] ``` +The ordinary queue defaults to open PRs. Use `--state merged` or `--state all` +explicitly only for a lifecycle or post-merge audit; named exact targets remain +lifecycle-neutral when `--state` is omitted. + 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 @@ -844,10 +848,10 @@ A first implementation is acceptable when: - `loopx pr-review` returns `loopx_pr_review_command_response_v0`; - default live reads use the caller's current `gh` repository, while `--repo owner/repo` can review another GitHub project; -- `--state all` includes merged PRs in the same packet, applies `--limit` per +- omitted `--state` and explicit `--state open` keep the ordinary queue + open-only; `--state all` includes merged PRs in the same packet, applies `--limit` per lifecycle group, and keeps `review_groups.merged` non-empty when merged PRs - exist in the requested window; `--state open` preserves the old open-only - review queue; + exist in the requested window; - `pull_requests` remains the full bounded inventory while every `review_sequence` contains only rows with a non-null `review_action_kind`; valid concluded exact heads are never recommended for duplicate work and diff --git a/loopx/cli_commands/pr_review.py b/loopx/cli_commands/pr_review.py index 147ba65b3c..6cc8b9d590 100644 --- a/loopx/cli_commands/pr_review.py +++ b/loopx/cli_commands/pr_review.py @@ -3,7 +3,7 @@ import argparse import hashlib import json -from collections.abc import Callable +from collections.abc import Callable, Sequence from contextlib import suppress from pathlib import Path @@ -97,7 +97,7 @@ def register_pr_review_command( ) -> None: parser = subparsers.add_parser( "pr-review", - help="Build a public-safe /loopx-pr-review queue for the current project's open and merged pull requests.", + help="Build a public-safe /loopx-pr-review queue for the current project's pull requests.", ) add_subcommand_format(parser) parser.add_argument("--goal-id", help="Use this Goal PR review configuration; otherwise use machine defaults.") @@ -130,8 +130,11 @@ def register_pr_review_command( parser.add_argument( "--state", choices=("open", "merged", "all"), - default="all", - help="PR lifecycle state to include. Defaults to all so merged PRs remain reviewable.", + default=None, + help=( + "PR lifecycle state to include. Ordinary queues default to open; " + "use merged/all explicitly for lifecycle or post-merge audits." + ), ) parser.add_argument( "--review-priority", @@ -209,6 +212,16 @@ def register_pr_review_command( ) +def _resolve_pr_review_state_filter( + raw_state: object, + *, + target_exact_heads: Sequence[str], +) -> str: + if raw_state is None and target_exact_heads: + return "all" + return normalize_pr_state_filter(raw_state) + + def handle_pr_review_command( args: argparse.Namespace, *, @@ -222,6 +235,10 @@ 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 []) + resolved_state_filter = _resolve_pr_review_state_filter( + getattr(args, "state", None), + target_exact_heads=target_exact_heads, + ) try: machine_configuration = (read_machine_configuration(runtime_root, registry=build_builtin_machine_configuration_registry()) if runtime_root is not None else None) goal = None @@ -433,7 +450,7 @@ def handle_pr_review_command( source_scan = scan_github_pull_requests( repo=repository, limit=max(1, args.limit) + 1, - state_filter=normalize_pr_state_filter(args.state), + state_filter=resolved_state_filter, since=args.since, **({"wait_for_ci": False} if not wait_for_ci else {}), ) @@ -454,7 +471,7 @@ def handle_pr_review_command( repository=repository, limit=max(1, args.limit), source=source, - state_filter=normalize_pr_state_filter(args.state), + state_filter=resolved_state_filter, since=args.since, source_scan=source_scan, reviewer_login=reviewer_login, @@ -517,7 +534,7 @@ def handle_pr_review_command( "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), + "state_filter": resolved_state_filter, "since": args.since, "review_priority": resolved_review_priority.value, "fresh_audit_exact_heads": list(args.fresh_audit_exact_head), diff --git a/loopx/doctor.py b/loopx/doctor.py index 817ccdf046..1f2e486df7 100644 --- a/loopx/doctor.py +++ b/loopx/doctor.py @@ -43,7 +43,8 @@ "--delivery-outcome ", ), "loopx-pr-review": ( - "loopx --format json pr-review --state all", + "keeps ordinary queue discovery open-only", + "explicit `--state merged|all`", "thin host adapter", "agent_response_contract.review_execution_contract", "pull_requests[review_action_kind!=null].review_plan", diff --git a/loopx/pr_review.py b/loopx/pr_review.py index 698268a183..4350947588 100644 --- a/loopx/pr_review.py +++ b/loopx/pr_review.py @@ -193,8 +193,8 @@ def _include_pr_in_window(pr: dict[str, Any], *, since: object | None) -> bool: def normalize_pr_state_filter(value: object) -> str: - state = str(value or "all").strip().lower() - return state if state in {"open", "merged", "all"} else "all" + state = str(value or "open").strip().lower() + return state if state in {"open", "merged", "all"} else "open" def fetch_github_pull_requests( @@ -202,7 +202,7 @@ def fetch_github_pull_requests( repo: str | None, limit: int, cwd: Path | None = None, - state_filter: str = "all", + state_filter: str = "open", since: str | None = None, ) -> list[dict[str, Any]]: scan = scan_github_pull_requests( @@ -220,7 +220,7 @@ def scan_github_pull_requests( repo: str | None, limit: int, cwd: Path | None = None, - state_filter: str = "all", + state_filter: str = "open", since: str | None = None, wait_for_ci: bool = True, ) -> dict[str, Any]: @@ -1022,7 +1022,7 @@ def build_pr_review_packet( repository: str | None, limit: int, source: str, - state_filter: str = "all", + state_filter: str = "open", since: str | None = None, source_scan: Mapping[str, Any] | None = None, reviewer_login: str | None = None, @@ -1347,7 +1347,7 @@ def render_pr_review_markdown(payload: dict[str, Any]) -> str: "", f"- command: `{request.get('command')}`", f"- repository: `{request.get('repository') or 'current gh repository'}`", - f"- state_filter: `{request.get('state_filter') or 'all'}`", + f"- state_filter: `{request.get('state_filter') or 'open'}`", f"- since: `{request.get('since') or 'not set'}`", f"- headline: {summary.get('headline')}", f"- complete: `{completeness.get('complete')}`; truncated=`{completeness.get('truncated')}`; recommended_limit=`{completeness.get('recommended_limit')}`", diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index 130831e01d..fae045e5b5 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -17,7 +17,7 @@ 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. -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. +For named PRs, resolve heads and run repeatable `--target-exact-head NUMBER@HEAD_OID`. An omitted `--state` keeps ordinary queue discovery open-only while exact targets remain lifecycle-neutral; use explicit `--state merged|all` only for deliberate history or post-merge audit. Translate only explicit filters: diff --git a/skills/loopx-self-repair/references/repair-patterns.md b/skills/loopx-self-repair/references/repair-patterns.md index 659a897caf..80fb1182c5 100644 --- a/skills/loopx-self-repair/references/repair-patterns.md +++ b/skills/loopx-self-repair/references/repair-patterns.md @@ -152,6 +152,7 @@ teaches a reusable control-plane lesson. | `explore_visual_cross_role_postcondition_gap` | Each role reports a successful marker readback, but a later role overwrites the same whiteboard and makes the earlier success stale. | configured role/stage token fingerprints, publish order, batch readback target set, final role markers. | Stage tokens were validated only within each role and marker settlement ran before all role writes completed. | Reject cross-role stage-token reuse before writes, then settle every role/stage marker once after the final batch write and recompute all role and delivery statuses from that final postcondition. | | `nested_singleflight_self_contention` | A batch owns a sink singleflight lock, then a nested direct command or automatic sync for the same config returns `sync_busy` even though no independent writer exists. | outer batch lock scope, nested command call path, process/context identity, connector call count, cross-process contention smoke. | The file lock distinguished only acquired versus busy and could not recognize re-entry by its current execution context; a regression fixture may also have encoded self-rejection as expected behavior. | Track held lock targets in execution-local context, reuse only an exact same-target ownership, preserve cross-context/process fail-fast, and cover nested direct/decorated calls plus a competing process. | | `pr_review_lifecycle_group_truncation` | `loopx pr-review --state all` reports open PRs but `review_groups.merged` is empty even though `--state merged` or GitHub closed/merged search finds merged PRs in the same window. | `loopx --format json pr-review --state all --since ...`, `loopx --format json pr-review --state merged --since ...`, `gh pr list --state closed/merged`, and `examples/pr-review-command-smoke.py`. | The live fetch or packet builder applied one combined `--limit` before lifecycle grouping, so open PRs consumed the packet and merged post-merge review entries were truncated away. | Fetch open and closed/merged windows separately for `--state all`, apply `--limit` per lifecycle group, keep `review_groups.unmerged` and `review_groups.merged` authoritative, and cover a fixture where `--limit 1` still returns one merged PR. | +| `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_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. | diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index c7bc8299e6..abddbb2b59 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -23,6 +23,51 @@ HEAD_2 = "b" * 40 +def test_state_filter_defaults_open_but_exact_targets_are_lifecycle_neutral() -> None: + assert pr_review_module.normalize_pr_state_filter(None) == "open" + assert pr_review_module.normalize_pr_state_filter("unknown") == "open" + assert ( + pr_review_cli_module._resolve_pr_review_state_filter( + None, target_exact_heads=[] + ) + == "open" + ) + assert ( + pr_review_cli_module._resolve_pr_review_state_filter( + None, target_exact_heads=[f"1@{HEAD_1}"] + ) + == "all" + ) + assert ( + pr_review_cli_module._resolve_pr_review_state_filter( + "open", target_exact_heads=[f"1@{HEAD_1}"] + ) + == "open" + ) + + +def test_live_scan_omitted_state_queries_only_open( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[list[str]] = [] + + def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: + del cwd + calls.append(args) + return [] + + monkeypatch.setattr(pr_review_module, "_run_gh_json", fake_run_gh_json) + + scan = pr_review_module.scan_github_pull_requests( + repo="owner/repo", + limit=10, + ) + + assert [item["state"] for item in scan["states"]] == ["open"] + assert len(calls) == 1 + assert calls[0][calls[0].index("--state") + 1] == "open" + + def _rows() -> list[dict[str, object]]: return [ {