Skip to content

Commit 889f26f

Browse files
authored
Merge pull request #4876 from loopx-project/codex/pr-review-exact-target-clean
fix(pr-review): read explicit exact targets directly
2 parents f9bff24 + d5e207f commit 889f26f

7 files changed

Lines changed: 293 additions & 38 deletions

File tree

‎examples/pr-review-command-smoke.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ def main() -> int:
9090
"Never send the projection ACK before the Todo exists",
9191
"Generic `re-review`, `重新review`, and `复审` wording selects the named PR; it is not a force-refresh token.",
9292
"the row stays in `pull_requests` inventory but must not appear in `review_sequence`",
93+
"--target-exact-head NUMBER@HEAD_OID",
9394
):
9495
assert phrase in skill_text, phrase
9596
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:
229230
assert request["command"] == "/loopx-pr-review", request
230231
assert (
231232
request["cli_command"]
232-
== "loopx pr-review [--repo owner/repo] [--state open|merged|all] [--review-priority other-developers-first|owner-first] [--since ISO]"
233+
== "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]"
233234
), request
234235
assert request["privacy_mode"] == "public_safe_github_metadata", request
235236
assert request["dry_run"] is True, request
@@ -253,6 +254,18 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object:
253254
assert payload["summary"]["total_pr_count"] == 4, payload["summary"]
254255
assert payload["summary"]["open_pr_count"] == 3, payload["summary"]
255256
assert payload["summary"]["merged_pr_count"] == 1, payload["summary"]
257+
target = payload["pull_requests"][0]
258+
exact_target = f"{target['number']}@{target['head_oid']}"
259+
targeted = json.loads(
260+
run_cli(
261+
"--format", "json", "pr-review", "--fixture", str(FIXTURE),
262+
"--target-exact-head", exact_target,
263+
).stdout
264+
)
265+
assert targeted["request"]["target_exact_heads"] == [exact_target], targeted
266+
assert targeted["result_completeness"]["complete"] is True, targeted
267+
assert targeted["result_completeness"]["limit_scope"] == "exact_targets", targeted
268+
assert [item["number"] for item in targeted["pull_requests"]] == [target["number"]]
256269
assert payload["summary"]["post_merge_review_count"] == 1, payload["summary"]
257270
assert payload["summary"]["review_attention_count"] == 3, payload["summary"]
258271
assert payload["summary"]["draft_count"] == 1, payload["summary"]

‎loopx/capabilities/pr_review_queue/README.md‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ workflow or the merge-focused `loopx-pr-merge` skill.
4646

4747
| Command | CLI reference | Intent |
4848
| --- | --- | --- |
49-
| `/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. |
49+
| `/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. |
5050
| 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. |
5151

5252
The slash command must run the CLI first. Agentloop must not reconstruct the
@@ -62,6 +62,20 @@ templates enter the model context:
6262
loopx --format json pr-review --state all [--repo owner/repo] [--since ISO]
6363
```
6464

65+
When the user explicitly names one or a few PRs, resolve each current head and
66+
request only those exact heads. This direct path is complete for the named
67+
targets and must not be expanded into a historical queue merely to satisfy
68+
queue completeness:
69+
70+
```bash
71+
loopx --format json pr-review --repo owner/repo \
72+
--target-exact-head 4868@0123456789abcdef0123456789abcdef01234567
73+
```
74+
75+
The option is repeatable. A remote head mismatch fails closed. Use
76+
`--fresh-audit-exact-head` in addition only when an unchanged target already has
77+
a valid conclusion and the user explicitly requests new evidence.
78+
6579
The live source scan keeps the list query lightweight and enriches each PR's
6680
nested commits, reviews, and checks with a bounded pool of concurrent
6781
`gh pr view` reads (at most eight at a time). Results are reassembled in list

‎loopx/capabilities/pr_review_queue/github_source.py‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
from pathlib import Path
1010
from typing import Any, Callable
1111

12+
from .selection_execution import normalize_fresh_audit_exact_heads
13+
1214
GitHubJsonRunner = Callable[..., Any]
1315

1416
DETAIL_FIELDS = (
@@ -21,6 +23,26 @@
2123
"reviews",
2224
)
2325

26+
PR_LIST_FIELDS = (
27+
"number",
28+
"title",
29+
"url",
30+
"state",
31+
"isDraft",
32+
"headRefName",
33+
"headRefOid",
34+
"baseRefName",
35+
"author",
36+
"createdAt",
37+
"updatedAt",
38+
"closedAt",
39+
"mergedAt",
40+
"mergeCommit",
41+
"changedFiles",
42+
"additions",
43+
"deletions",
44+
)
45+
2446

2547
def run_gh_json(args: list[str], *, cwd: Path | None = None) -> Any:
2648
proc = subprocess.run(
@@ -140,6 +162,80 @@ def attach_pr_review_details(
140162
return True
141163

142164

165+
def scan_github_pull_request_targets(
166+
*,
167+
repository: str,
168+
exact_heads: Sequence[str],
169+
cwd: Path | None = None,
170+
run_gh_json: GitHubJsonRunner = run_gh_json,
171+
wait_for_ci: bool = True,
172+
) -> dict[str, Any]:
173+
"""Read only explicitly requested exact heads, without scanning a queue."""
174+
175+
targets = normalize_fresh_audit_exact_heads(exact_heads)
176+
if not targets:
177+
raise ValueError("at least one target exact head is required")
178+
179+
pull_requests: list[dict[str, Any]] = []
180+
for target in sorted(
181+
targets, key=lambda item: (int(item.split("@", 1)[0]), item)
182+
):
183+
number, expected_head = target.split("@", 1)
184+
try:
185+
row = run_gh_json(
186+
[
187+
"pr",
188+
"view",
189+
number,
190+
"--json",
191+
",".join(PR_LIST_FIELDS),
192+
"--repo",
193+
repository,
194+
],
195+
cwd=cwd,
196+
)
197+
except Exception as exc:
198+
raise RuntimeError(f"target PR #{number} metadata read failed") from exc
199+
if not isinstance(row, dict):
200+
raise RuntimeError(f"target PR #{number} metadata read was not an object")
201+
actual_head = str(row.get("headRefOid") or "").strip().lower()
202+
if actual_head != expected_head:
203+
raise ValueError(
204+
f"target PR #{number} head changed: expected {expected_head}, "
205+
f"remote is {actual_head or 'unavailable'}"
206+
)
207+
details_ok = attach_pr_review_details(
208+
row,
209+
repository=repository,
210+
cwd=cwd,
211+
**({"wait_for_ci": False} if not wait_for_ci else {}),
212+
run_gh_json=run_gh_json,
213+
)
214+
if not details_ok:
215+
raise RuntimeError(f"target PR #{number} detail read was incomplete")
216+
pull_requests.append(row)
217+
218+
return {
219+
"schema_version": "pr_review_source_scan_v0",
220+
"complete": True,
221+
"mode": "exact_targets",
222+
"requested_exact_heads": sorted(targets),
223+
"observed_exact_heads": sorted(targets),
224+
"pull_requests": pull_requests,
225+
"states": [
226+
{
227+
"state": "exact_targets",
228+
"fetch_limit": len(targets),
229+
"fetched_count": len(pull_requests),
230+
"included_after_window": len(pull_requests),
231+
"detail_read_failures": 0,
232+
"source_saturated": False,
233+
"source_read_valid": True,
234+
}
235+
],
236+
}
237+
238+
143239
PR_REVIEW_DETAIL_MAX_WORKERS = 8
144240

145241

‎loopx/cli_commands/pr_review.py‎

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@
1818
)
1919
from ..capabilities.machine_configuration.store import read_machine_configuration
2020
from ..capabilities.pr_review_queue.result_check import check_review_result
21+
from ..capabilities.pr_review_queue.github_source import (
22+
scan_github_pull_request_targets,
23+
)
2124
from ..file_lock import exclusive_file_lock
2225
from ..pr_review import (
2326
build_pr_review_packet,
@@ -148,6 +151,16 @@ def register_pr_review_command(
148151
"--fixture",
149152
help="Read public-safe PR metadata from a JSON fixture instead of live gh output.",
150153
)
154+
parser.add_argument(
155+
"--target-exact-head",
156+
action="append",
157+
default=[],
158+
metavar="NUMBER@HEAD_OID",
159+
help=(
160+
"Read only this exact PR head instead of scanning a lifecycle queue. "
161+
"Repeatable for a small explicit review batch."
162+
),
163+
)
151164
parser.add_argument(
152165
"--fresh-audit-exact-head",
153166
action="append",
@@ -208,6 +221,7 @@ def handle_pr_review_command(
208221
return None
209222
checkpoint_path: Path | None = None
210223
resolved_review_priority = DEFAULT_REVIEW_PRIORITY
224+
target_exact_heads = list(getattr(args, "target_exact_head", []) or [])
211225
try:
212226
machine_configuration = (read_machine_configuration(runtime_root, registry=build_builtin_machine_configuration_registry()) if runtime_root is not None else None)
213227
goal = None
@@ -233,6 +247,7 @@ def handle_pr_review_command(
233247
or args.repo
234248
or args.since
235249
or args.fresh_audit_exact_head
250+
or target_exact_heads
236251
or args.check_merge_readiness
237252
):
238253
raise ValueError(
@@ -263,6 +278,7 @@ def handle_pr_review_command(
263278
or args.projected_exact_head
264279
or args.since
265280
or args.fresh_audit_exact_head
281+
or target_exact_heads
266282
):
267283
raise ValueError(
268284
"merge readiness cannot be combined with queue or observation options"
@@ -349,6 +365,15 @@ def handle_pr_review_command(
349365
"--observation-state-file cannot be combined with "
350366
"--previous-observation-json"
351367
)
368+
if target_exact_heads and args.autonomous_observation:
369+
raise ValueError(
370+
"--target-exact-head cannot be combined with --autonomous-observation"
371+
)
372+
if target_exact_heads and args.since:
373+
raise ValueError(
374+
"--target-exact-head already defines the review window and "
375+
"cannot be combined with --since"
376+
)
352377
explicit_review_priority = getattr(args, "review_priority", None)
353378
if explicit_review_priority is not None:
354379
resolved_review_priority = normalize_review_priority(explicit_review_priority)
@@ -372,6 +397,19 @@ def handle_pr_review_command(
372397
Path(args.fixture).expanduser()
373398
)
374399
repository = repository or repository_from_fixture
400+
if target_exact_heads:
401+
requested_targets = normalize_fresh_audit_exact_heads(
402+
target_exact_heads
403+
)
404+
pull_requests = [
405+
item
406+
for item in pull_requests
407+
if (
408+
f"{item.get('number')}@"
409+
f"{str(item.get('headRefOid') or '').lower()}"
410+
in requested_targets
411+
)
412+
]
375413
source = "fixture"
376414
source_scan = None
377415
else:
@@ -382,13 +420,23 @@ def handle_pr_review_command(
382420
"authenticated GitHub reviewer identity is required for "
383421
"autonomous author-owned scheduling"
384422
)
385-
source_scan = scan_github_pull_requests(
386-
repo=repository,
387-
limit=max(1, args.limit) + 1,
388-
state_filter=normalize_pr_state_filter(args.state),
389-
since=args.since,
390-
**({"wait_for_ci": False} if not wait_for_ci else {}),
391-
)
423+
if target_exact_heads:
424+
if not repository:
425+
raise RuntimeError("GitHub repository could not be resolved")
426+
source_scan = scan_github_pull_request_targets(
427+
repository=repository,
428+
exact_heads=target_exact_heads,
429+
**({"wait_for_ci": False} if not wait_for_ci else {}),
430+
)
431+
source = "github_cli_exact_targets"
432+
else:
433+
source_scan = scan_github_pull_requests(
434+
repo=repository,
435+
limit=max(1, args.limit) + 1,
436+
state_filter=normalize_pr_state_filter(args.state),
437+
since=args.since,
438+
**({"wait_for_ci": False} if not wait_for_ci else {}),
439+
)
392440
pull_requests = source_scan["pull_requests"]
393441
if checkpoint_path is not None and previous_observation:
394442
checkpoint_repository = str(
@@ -411,6 +459,7 @@ def handle_pr_review_command(
411459
source_scan=source_scan,
412460
reviewer_login=reviewer_login,
413461
fresh_audit_exact_heads=args.fresh_audit_exact_head,
462+
target_exact_heads=target_exact_heads,
414463
review_priority=resolved_review_priority,
415464
wait_for_ci=wait_for_ci,
416465
)
@@ -465,13 +514,14 @@ def handle_pr_review_command(
465514
"request": {
466515
"schema_version": "loopx_pr_review_command_request_v0",
467516
"command": "/loopx-pr-review",
468-
"cli_command": "loopx pr-review [--repo owner/repo] [--state open|merged|all] [--review-priority other-developers-first|owner-first] [--since ISO]",
517+
"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]",
469518
"repository": args.repo,
470519
"limit": max(1, args.limit),
471520
"state_filter": normalize_pr_state_filter(args.state),
472521
"since": args.since,
473522
"review_priority": resolved_review_priority.value,
474523
"fresh_audit_exact_heads": list(args.fresh_audit_exact_head),
524+
"target_exact_heads": target_exact_heads,
475525
"source": "fixture" if args.fixture else "github_cli",
476526
"privacy_mode": "public_safe_github_metadata",
477527
"dry_run": True,

0 commit comments

Comments
 (0)