From aed37a11ef1d609e43d834376da4e305667463f9 Mon Sep 17 00:00:00 2001 From: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:15:14 +0800 Subject: [PATCH 1/3] fix(pr-review): recover capped file inventories from exact Git objects Signed-off-by: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> --- .../pr_review_queue/github_source.py | 163 +++++++++++++++--- 1 file changed, 138 insertions(+), 25 deletions(-) diff --git a/loopx/capabilities/pr_review_queue/github_source.py b/loopx/capabilities/pr_review_queue/github_source.py index 11606144b6..a4eaf8e897 100644 --- a/loopx/capabilities/pr_review_queue/github_source.py +++ b/loopx/capabilities/pr_review_queue/github_source.py @@ -3,6 +3,8 @@ from __future__ import annotations import json +import os +import re import subprocess from concurrent.futures import ThreadPoolExecutor from collections.abc import Sequence @@ -62,6 +64,100 @@ def run_gh_json(args: list[str], *, cwd: Path | None = None) -> Any: return json.loads(proc.stdout or "null") +def _read_git(args: list[str], *, cwd: Path) -> bytes: + # Local objects only. Lazy fetching, external diff and textconv must not turn + # a read-only inventory into provider execution or a network operation. + return subprocess.run( + ["git", "--no-replace-objects", *args], cwd=cwd, check=True, + env={**os.environ, "GIT_NO_LAZY_FETCH": "1", "GIT_OPTIONAL_LOCKS": "0"}, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=30, + ).stdout + + +def _recover_git_pr_files( + *, repository: str, snapshot: dict[str, Any], observed: list[dict[str, Any]], + cwd: Path | None, +) -> list[dict[str, Any]] | None: + """Reconcile local immutable objects with API-confirmed rename pairs.""" + cwd = cwd if cwd is not None else Path.cwd() + head, base = snapshot.get("headRefOid"), snapshot.get("baseRefOid") + if any(not isinstance(oid, str) or not re.fullmatch(r"[0-9a-f]{40}", oid) + for oid in (head, base)): + return None + try: + remote = _read_git(["remote", "get-url", "origin"], cwd=cwd).decode().strip() + match = re.fullmatch( + r"(?:https://github\.com/|ssh://git@github\.com/|git@github\.com:)" + r"([^/]+/[^/]+?)(?:\.git)?/?", remote, + ) + if not match or match[1].casefold() != repository.casefold(): + return None + for oid in (head, base): + _read_git(["cat-file", "-e", f"{oid}^{{commit}}"], cwd=cwd) + bases = _read_git(["merge-base", "--all", base, head], cwd=cwd).splitlines() + if len(bases) != 1: + return None + merge_base = bases[0].decode("ascii") + args = ["diff", "--no-ext-diff", "--no-textconv", "--no-renames"] + stats = _read_git([*args, "--numstat", "-z", merge_base, head, "--"], cwd=cwd) + names = _read_git([*args, "--name-status", "-z", merge_base, head, "--"], cwd=cwd) + if not stats.endswith(b"\0") or not names.endswith(b"\0"): + return None + inventory: dict[str, dict[str, Any]] = {} + for entry in stats[:-1].split(b"\0"): + added, deleted, raw_path = entry.split(b"\t", 2) + binary = added == deleted == b"-" + if not binary and (not added.isdigit() or not deleted.isdigit()): + return None + path = raw_path.decode("utf-8", errors="strict") + if not path or path in inventory: + return None + inventory[path] = {"path": path, "additions": None if binary else int(added), + "deletions": None if binary else int(deleted), "source": "git"} + tokens = names[:-1].split(b"\0") + if len(tokens) % 2: + return None + statuses = {tokens[i + 1].decode("utf-8", errors="strict"): + tokens[i].decode("ascii") for i in range(0, len(tokens), 2)} + if len(statuses) != len(inventory) or statuses.keys() != inventory.keys(): + return None + api_paths: set[str] = set() + rename_endpoints: set[str] = set() + for item in observed: + path = item["path"] + if path in api_paths: + return None + api_paths.add(path) + if item.get("status") != "renamed": + continue + old = item.get("previous_filename") + if (not isinstance(old, str) or old == path + or {old, path} & rename_endpoints + or statuses.get(old) != "D" or statuses.get(path) != "A"): + return None + rename_endpoints.update((old, path)) + del inventory[old] + inventory[path] = {"path": path, "additions": item["additions"], + "deletions": item["deletions"], "source": "github"} + if not api_paths <= inventory.keys(): + return None + # Verify Git whole-diff totals with only API-confirmed rename folding. + # Ordinary API rows may report 0/0 for generated/omitted diffs: replacing + # their Git statistics before this check would conceal disagreements. + # Git's valid -/- binary rows contribute no textual lines; retain None + # for unknown per-file counts, rather than inventing API 0/0 values. + if (len(inventory) != snapshot["changedFiles"] + or sum(row["additions"] or 0 for row in inventory.values()) != snapshot["additions"] + or sum(row["deletions"] or 0 for row in inventory.values()) != snapshot["deletions"]): + return None + for item in observed: + inventory[item["path"]] = {key: item[key] for key in + ("path", "additions", "deletions")} | {"source": "github"} + return [inventory[path] for path in sorted(inventory)] + except (OSError, subprocess.SubprocessError, UnicodeError, ValueError, KeyError): + return None + + def _fetch_complete_pr_files( *, repository: str, @@ -69,45 +165,61 @@ def _fetch_complete_pr_files( expected_count: int, cwd: Path | None, run_gh_json: GitHubJsonRunner, + snapshot: dict[str, Any] | None = None, ) -> list[dict[str, Any]] | None: + # The ordinary REST path and closeout callers retain their existing contract. + # Only a declared API-cap-sized PR with a versioned snapshot may use Git. + recover = snapshot is not None and expected_count > 3000 + version_fields = ("headRefOid", "baseRefOid", "changedFiles", "additions", "deletions") + + def same_snapshot() -> bool: + current = run_gh_json(["pr", "view", number, "--json", ",".join(version_fields), + "--repo", repository], cwd=cwd) + return isinstance(current, dict) and all( + current.get(key) == snapshot.get(key) for key in version_fields + ) + try: + if recover and (any(type(snapshot.get(key)) is not int or snapshot[key] < 0 + for key in version_fields[2:]) or not same_snapshot()): + return None payload = run_gh_json( - [ - "api", - "--paginate", - "--slurp", - f"repos/{repository}/pulls/{number}/files?per_page=100", - ], - cwd=cwd, + ["api", "--paginate", "--slurp", + f"repos/{repository}/pulls/{number}/files?per_page=100"], cwd=cwd, ) - except Exception: - return None - if not isinstance(payload, list): - return None - pages = payload if all(isinstance(page, list) for page in payload) else [payload] - files: list[dict[str, Any]] = [] - try: + if not isinstance(payload, list): + return None + pages = payload if all(isinstance(page, list) for page in payload) else [payload] + files: list[dict[str, Any]] = [] + observed: list[dict[str, Any]] = [] for page in pages: for item in page: if not isinstance(item, dict): return None - path = str(item.get("filename") or item.get("path") or "").strip() - if not path: + path = item.get("filename") or item.get("path") + if not isinstance(path, str) or not path: + return None + if recover and any(type(item.get(key)) is not int for key in ("additions", "deletions")): return None additions = int(item.get("additions") or 0) deletions = int(item.get("deletions") or 0) if additions < 0 or deletions < 0: return None - files.append( - { - "path": path, - "additions": additions, - "deletions": deletions, - } - ) - except (TypeError, ValueError): + row = {"path": path, "additions": additions, "deletions": deletions} + files.append(row) + observed.append(row | {"status": item.get("status"), + "previous_filename": item.get("previous_filename")}) + if len(files) == expected_count: + return files if not recover or same_snapshot() else None + if not recover or not files or len(files) > expected_count: + return None + recovered = _recover_git_pr_files( + repository=repository, snapshot=snapshot, observed=observed, cwd=cwd, + ) + return recovered if recovered is not None and same_snapshot() else None + except Exception: + # This provider's failed read remains an explicit incomplete source. return None - return files if len(files) == expected_count else None def attach_pr_review_details( @@ -163,6 +275,7 @@ def attach_pr_review_details( repository=repository, number=number, expected_count=expected_file_count, + snapshot=row, cwd=cwd, run_gh_json=run_gh_json, ) From ad292c6fb3657b8d0d2180d56a410388ed2c84aa Mon Sep 17 00:00:00 2001 From: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:15:14 +0800 Subject: [PATCH 2/3] test(pr-review): cover inventory recovery and version boundaries Signed-off-by: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> --- .../test_pr_review_git_file_inventory.py | 251 ++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 tests/capabilities/test_pr_review_git_file_inventory.py diff --git a/tests/capabilities/test_pr_review_git_file_inventory.py b/tests/capabilities/test_pr_review_git_file_inventory.py new file mode 100644 index 0000000000..685ce8cf42 --- /dev/null +++ b/tests/capabilities/test_pr_review_git_file_inventory.py @@ -0,0 +1,251 @@ +"""A capped GitHub inventory is complete only with independent versioned evidence.""" +from __future__ import annotations + +import copy +import subprocess +from pathlib import Path + +import pytest + +from loopx.capabilities.pr_review_queue import github_source as source + + +def git(repo: Path, *args: str) -> str: + return subprocess.check_output(["git", *args], cwd=repo, text=True).strip() + + +@pytest.fixture(scope="module") +def inventory(tmp_path_factory): + repo = tmp_path_factory.mktemp("inventory") + git(repo, "init", "-q") + git(repo, "config", "user.name", "Fixture") + git(repo, "config", "user.email", "fixture@example.test") + git(repo, "remote", "add", "origin", "https://github.com/owner/repo.git") + (repo / "old.txt").write_text("one\n") + (repo / "deleted.txt").write_text("gone\n") + git(repo, "add", "old.txt", "deleted.txt") + git(repo, "commit", "-qm", "base") + base = git(repo, "rev-parse", "HEAD") + (repo / "old.txt").unlink() + (repo / "deleted.txt").unlink() + (repo / "new.txt").write_text("one\ntwo\n") + for index in range(3000): + (repo / f"file-{index:04d}.txt").write_bytes(b"") + special = " space\nwith\ttabs.txt " + (repo / special).write_text("content\n") + git(repo, "add", "-A") + git(repo, "commit", "-qm", "head") + head = git(repo, "rev-parse", "HEAD") + # Branch advancement must not change the PR's merge-base comparison. + git(repo, "checkout", "-q", base) + (repo / "base-only.txt").write_text("unrelated\n") + git(repo, "add", "base-only.txt") + git(repo, "commit", "-qm", "base advanced") + advanced_base = git(repo, "rev-parse", "HEAD") + git(repo, "checkout", "-q", head) + (repo / "binary.bin").write_bytes(b"\0binary") + git(repo, "add", "binary.bin") + git(repo, "commit", "-qm", "binary head") + binary_head = git(repo, "rev-parse", "HEAD") + snapshot = {"number": 7, "headRefOid": head, "baseRefOid": advanced_base, + "changedFiles": 3003, "additions": 2, "deletions": 1} + api = [ + {"filename": "new.txt", "status": "renamed", "previous_filename": "old.txt", + "additions": 1, "deletions": 0}, + {"filename": "deleted.txt", "status": "removed", "additions": 0, "deletions": 1}, + # API-omitted per-file stats must be retained, separately from Git totals. + {"filename": special, "status": "added", "additions": 0, "deletions": 0}, + ] + # Keep the extra head separate so ordinary recovery still exercises 3003. + (repo / ".git" / "fixture-binary-head").write_text(binary_head) + return repo, snapshot, api, special + + +def attach(inventory, *, mutate_api=None, mutate_snapshot=None, drift=None, cwd=None): + repo, original, api, _ = inventory + row = copy.deepcopy(original) + if mutate_snapshot: + mutate_snapshot(row) + pages = [copy.deepcopy(api)] + if mutate_api: + mutate_api(pages[0]) + calls = [] + snapshots = 0 + + def gh(args, *, cwd=None): + nonlocal snapshots + calls.append(args) + assert "statusCheckRollup" not in ",".join(args) + if args[:2] == ["api", "--paginate"]: + return copy.deepcopy(pages) + fields = args[args.index("--json") + 1].split(",") + if "body" in fields: + return {**{field: [] for field in fields}, **row, "body": "author text", + "files": [{"path": "incomplete.txt"}]} + snapshots += 1 + current = copy.deepcopy(row) + if drift and snapshots == drift[0]: + current[drift[1]] = drift[2] + return current + + ok = source.attach_pr_review_details(row, repository="owner/repo", + cwd=cwd or repo, run_gh_json=gh, wait_for_ci=False) + return ok, row, calls + + +def test_capped_source_recovers_real_merge_base_and_preserves_api_stats(inventory): + ok, row, calls = attach(inventory) + assert ok + files = {entry["path"]: entry for entry in row["files"]} + assert len(files) == 3003 + assert "old.txt" not in files and "base-only.txt" not in files + assert files["new.txt"] == {"path": "new.txt", "additions": 1, "deletions": 0, + "source": "github"} + assert files[inventory[3]]["additions"] == 0 + assert files[inventory[3]]["source"] == "github" + assert files["file-2999.txt"]["source"] == "git" + # Mixed per-file display values need not sum to the verified remote totals. + assert sum(item["additions"] for item in files.values()) == 1 + assert len(calls) == 4 # details, version fence, REST inventory, version readback + + +@pytest.mark.parametrize("field,value", [ + ("changedFiles", 3004), ("additions", 3), ("deletions", 0), + ("headRefOid", "a" * 40), ("baseRefOid", "b" * 40), + ("headRefOid", "--all"), ("baseRefOid", "HEAD"), + ("additions", None), ("additions", True), +]) +def test_count_totals_and_exact_objects_are_decisive(inventory, field, value): + ok, row, _ = attach(inventory, mutate_snapshot=lambda row: row.update({field: value})) + assert not ok + assert "files" not in row + + +@pytest.mark.parametrize("fence", [1, 2]) +@pytest.mark.parametrize("field,value", [ + ("headRefOid", "c" * 40), ("baseRefOid", "d" * 40), + ("changedFiles", 3004), ("additions", 3), ("deletions", 2), +]) +def test_drift_around_api_and_git_read_never_attaches(inventory, fence, field, value): + assert not attach(inventory, drift=(fence, field, value))[0] + + +@pytest.mark.parametrize("mutation", [ + lambda rows: rows[0].update(previous_filename="missing.txt"), + lambda rows: rows[0].update(previous_filename="new.txt"), + lambda rows: rows[0].update(status="added"), + lambda rows: rows[0].update(additions=2), + lambda rows: rows.append(dict(rows[0])), + lambda rows: rows.append({"filename": "extra.txt", "status": "renamed", + "previous_filename": "old.txt", "additions": 0}), + lambda rows: rows.append({"filename": "not-in-diff.txt"}), + lambda rows: rows[0].update(filename=42), + lambda rows: rows[0].update(additions=1.5), + lambda rows: rows[0].update(deletions=True), +]) +def test_ambiguous_or_unobserved_renames_and_unknown_paths_fail(inventory, mutation): + assert not attach(inventory, mutate_api=mutation)[0] + + +def test_wrong_repository_and_absent_checkout_fail(inventory, tmp_path): + repo = tmp_path / "other" + repo.mkdir() + git(repo, "init", "-q") + git(repo, "remote", "add", "origin", "https://github.com/other/repo.git") + assert not attach(inventory, cwd=repo)[0] + assert not attach(inventory, cwd=tmp_path)[0] + + +@pytest.mark.parametrize("fault", ["hybrid_binary", "malformed", "invalid_utf8", "command_failure"]) +def test_invalid_local_diff_fails_closed(inventory, monkeypatch, fault): + original = source._read_git + + def read(args, *, cwd): + if "--numstat" in args: + if fault == "command_failure": + raise subprocess.CalledProcessError(1, ["git"]) + return {"hybrid_binary": b"-\t0\tbinary\0", "malformed": b"1\t0\tpath", + "invalid_utf8": b"1\t0\t\xff\0"}[fault] + return original(args, cwd=cwd) + + monkeypatch.setattr(source, "_read_git", read) + assert not attach(inventory)[0] + + +def test_ordinary_complete_rest_and_legacy_closeout_never_invoke_git(monkeypatch): + def forbidden(*args, **kwargs): + raise AssertionError("ordinary inventory must not touch Git") + + monkeypatch.setattr(source, "_read_git", forbidden) + expected = [{"path": "one", "additions": 1, "deletions": 0}, + {"path": "two", "additions": 0, "deletions": 2}] + calls = [] + + def gh(args, **kwargs): + calls.append(args) + return [[{"filename": "one", "additions": 1}], + [{"filename": "two", "deletions": 2}]] + + assert source._fetch_complete_pr_files(repository="owner/repo", number="7", + expected_count=2, cwd=None, run_gh_json=gh) == expected + assert len(calls) == 1 + assert source._fetch_complete_pr_files(repository="owner/repo", number="7", + expected_count=3003, cwd=None, run_gh_json=gh) is None + + +def test_api_failure_does_not_trigger_git_guess(inventory, monkeypatch): + repo, snapshot, _, _ = inventory + def failure(*args, **kwargs): + raise RuntimeError("API unavailable") + def forbidden(*args, **kwargs): + raise AssertionError("must not guess renames") + monkeypatch.setattr(source, "_read_git", forbidden) + assert source._fetch_complete_pr_files(repository="owner/repo", number="7", + expected_count=3003, cwd=repo, run_gh_json=failure, snapshot=snapshot) is None + + +def test_local_git_reads_disable_fetch_and_external_execution(monkeypatch, tmp_path): + calls = [] + def run(args, **kwargs): + calls.append((args, kwargs)) + return subprocess.CompletedProcess(args, 0, stdout=b"local", stderr=b"") + monkeypatch.setattr(source.subprocess, "run", run) + assert source._read_git(["diff", "--no-ext-diff", "--no-textconv"], cwd=tmp_path) == b"local" + args, options = calls[0] + assert args[:2] == ["git", "--no-replace-objects"] + assert options["env"]["GIT_NO_LAZY_FETCH"] == "1" + assert options["env"]["GIT_OPTIONAL_LOCKS"] == "0" + assert options["timeout"] == 30 + + +def test_production_target_scan_recovers_default_current_checkout(inventory, monkeypatch): + repo, snapshot, api, _ = inventory + monkeypatch.chdir(repo) + calls = [] + def gh(args, **kwargs): + calls.append(args) + if args[0] == "api": + return [api] + fields = args[args.index("--json") + 1].split(",") + if "body" in fields: + return {**{field: [] for field in fields}, **snapshot, + "body": "motivation", "files": []} + return copy.deepcopy(snapshot) + packet = source.scan_github_pull_request_targets(repository="owner/repo", + exact_heads=[f"7@{snapshot['headRefOid']}"], run_gh_json=gh, wait_for_ci=False) + assert packet["complete"] is True + assert len(packet["pull_requests"][0]["files"]) == 3003 + assert len(calls) == 5 + assert not any("statusCheckRollup" in ",".join(call) for call in calls) + + +def test_real_binary_row_has_unknown_counts_and_verified_text_totals(inventory): + repo, _, _, _ = inventory + binary_head = (repo / ".git" / "fixture-binary-head").read_text() + ok, row, _ = attach(inventory, mutate_snapshot=lambda row: + row.update(headRefOid=binary_head, changedFiles=3004)) + assert ok + binary = next(item for item in row["files"] if item["path"] == "binary.bin") + assert binary == {"path": "binary.bin", "additions": None, "deletions": None, + "source": "git"} + assert row["additions"] == 2 and row["deletions"] == 1 From d747192a0526120c4ed2ceefee0363d4fc827c9c Mon Sep 17 00:00:00 2001 From: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:15:14 +0800 Subject: [PATCH 3/3] docs(pr-review): explain local inventory proof and authority limits Signed-off-by: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> --- loopx/capabilities/pr_review_queue/README.md | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 734cb462cb..f7aad0de70 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -299,6 +299,36 @@ scan, so the latency improvement does not change queue ordering or freshness semantics. The scan does not use a stale cache: rerunning the command always re-reads the requested GitHub window. +When GitHub's paginated file API is capped at 3000 entries, a PR declaring +more files can recover its inventory from already available exact Git objects +in the caller's checkout. The `origin` must identify the requested GitHub +repository. LoopX compares the unique merge base to the exact head, folds only +API-confirmed rename pairs, and requires the resulting file count and whole-diff +addition/deletion totals to match fresh GitHub metadata. Head, base and totals +are fenced before pagination and after recovery. This is source completeness, +not review evidence or approval of the recovered PR. + +The source provider's recovered file rows identify `source` as `github` or +`git`. Known API per-file statistics are retained, including 0/0 for omitted +or generated diffs. Unobserved Git binary rows retain unknown per-file counts +(`null`); Git's textual totals exclude binary lines. Mixed display values need +not sum to the independently verified whole-diff totals. Ordinary complete GraphQL/REST reads keep their +existing shape. No object fetch, clone, checkout, external diff or textconv is +performed. A wrong repository, missing objects, ambiguous rename or +malformed statistics, total mismatch or remote version change remains an +incomplete source. Callers without a versioned snapshot (including approval +closeout) retain their complete-API requirement; inventory recovery grants no +additional closeout, publication or merge authority. + +GitHub 文件 API 达到 3000 项上限时,调用方当前 checkout 中已有的精确 Git 对象 +可用于恢复更大 PR 的文件清单。必须核对 origin 仓库、唯一 merge base 与精确 head, +仅折叠 API 明确确认的 rename,并让文件数和全 diff 增删量与远端 metadata 一致。 +分页前及恢复后校验 head、base 和总量。保留 API 已知行的统计值;恢复行的 Git 来源 +不等于已完成 review。Git 二进制行不贡献文本行数,未被 API 观测的逐行统计保留 null, +不会假定为 API 的 0/0。不会自动抓取对象、切分支或执行外部 diff/textconv;缺对象、 +统计格式损坏、rename 歧义、总量不符或版本变化继续明确返回 incomplete。没有精确快照 +的 approval closeout 仍要求完整 API 读回,不扩大撤回 review 或合并权限。 + For an autonomous maintainer monitor, request the complete open queue while persisting its compact cursor in an ignored local checkpoint: