diff --git a/doc/architecture/SPEC/engine/steps/pr.md b/doc/architecture/SPEC/engine/steps/pr.md index 545857b4..a3cc050b 100644 --- a/doc/architecture/SPEC/engine/steps/pr.md +++ b/doc/architecture/SPEC/engine/steps/pr.md @@ -1,6 +1,6 @@ # engine/steps/pr/ —— 规范 - + `LOC ~1988（6 个文件） · step 库（PR） · refactor-status: ok` @@ -45,7 +45,9 @@ rebase，不修改配置 checkout 的分支或文件，不推送。该树是一 - `ci.push` 对 `pr_review` / `pr_quality` 无条件拒绝推送；其他任务仍委托给 `guard_push`（**C4**）。 - **`pr.fetch_diff`：一个 head 统治一切**（PR2 重构后）。head 由 `_resolve_pr_head` **恰好解析一次**，stale 门、fetch、diff、worktree 全部 - 从这一个答案推导（`_fetch_at_one_head`）。fetch 走 run 域强制目的 ref + 从这一个答案推导（`_fetch_at_one_head`）。解析请求明确包含 `headRefOid`， + 不能用可能截断为前 100 条的 `commits` 列表末项推断 head；权威字段缺失时 + 仍按既有未解析 head 路径处理，钉住的请求拒绝继续。fetch 走 run 域强制目的 ref `refs/imx//{base,head}`（取代旧的机会主义 tracking ref + `FETCH_HEAD` —— 对着可能陈旧的 `origin/` 取 merge-base 会把无关的 上游漂移当成 PR 的工作），且 `_fetch_pinned` 把取到的 head 与 API 报告的 @@ -64,6 +66,8 @@ rebase，不修改配置 checkout 的分支或文件，不推送。该树是一 - **`pr.post_review` 始终使用 `event="COMMENT"`**，模型 verdict 不授予批准或阻塞 PR 的权限。 - **`pr.post_review` 只发一条 GitHub review + inline thread**，且先把每条发现的位置 对照已抓取的 diff 校验过 —— **绝不是一串独立评论**。 + 写入前同样通过 `headRefOid` 验证当前 head，不能使用截断 commit 列表； + 缺少权威 head 时不提交 review。 - `diff_text` 和 `gate_report` 都可以经 state 注入，因此网络之下的每条路径都可离线测试； `gh` 不可用时降级为 BLOCKED，**绝不崩溃**。 - `pr.checkout_branch` 把推导出的 `PushPolicy` **序列化后发布**（**B2**）—— @@ -104,7 +108,7 @@ rebase，不修改配置 checkout 的分支或文件，不推送。该树是一 `..agent_runtime`。 ## 测试 -`test_pr_steps.py`（含钉 ref run 域隔离、head 移动检测、stale expected_head +`test_pr_steps.py`（含截断 commit 列表下的权威 head 解析、钉 ref run 域隔离、head 移动检测、stale expected_head BLOCK、worktree 分键/拒外来树、report-only rebase 的隔离与失效树拦截）、 `test_push_and_steps.py`、 `test_knowledge_harvest.py`（harvest step + executor crash-then-resume）、 diff --git a/src/infermatrix_copilot/engine/steps/pr/fetch.py b/src/infermatrix_copilot/engine/steps/pr/fetch.py index 88f3d5f0..a5010924 100644 --- a/src/infermatrix_copilot/engine/steps/pr/fetch.py +++ b/src/infermatrix_copilot/engine/steps/pr/fetch.py @@ -45,7 +45,7 @@ def _resolve_pr_head(repo: Path, pr: int) -> tuple[str, str, str]: diff` resolves the head server-side; `_pr_time_checkout` resolved it again via `gh pr view`), so a push landing between them produced a diff and a review tree from different commits.""" - code, out = _gh(["pr", "view", str(pr), "--json", "baseRefName,commits"], + code, out = _gh(["pr", "view", str(pr), "--json", "baseRefName,headRefOid"], cwd=repo) if code != 0: return "", "", f"pr view failed: {out[:200]}" @@ -54,10 +54,9 @@ def _resolve_pr_head(repo: Path, pr: int) -> tuple[str, str, str]: except json.JSONDecodeError: return "", "", "pr view returned non-JSON" base_ref = str(data.get("baseRefName") or "") - commits = data.get("commits") or [] - # `headRefOid` is not exposed by every gh version — the last commit's oid is - # the PR head and is universally available - head_sha = str(commits[-1].get("oid") or "") if commits else "" + # The commits connection can be truncated at 100 entries; its last entry + # need not be the PR head. Resolve the authoritative head directly. + head_sha = str(data.get("headRefOid") or "") if not head_sha: return base_ref, "", "PR head unresolvable from pr view" return base_ref, head_sha, "" diff --git a/src/infermatrix_copilot/engine/steps/pr/publish.py b/src/infermatrix_copilot/engine/steps/pr/publish.py index 9f44817c..f1b86e63 100644 --- a/src/infermatrix_copilot/engine/steps/pr/publish.py +++ b/src/infermatrix_copilot/engine/steps/pr/publish.py @@ -290,14 +290,13 @@ def _review_payload(state: dict, *, current_head: str = "") -> tuple[dict, int]: def _current_pr_head(repo, pr: int) -> tuple[str, str]: """Resolve the current PR head SHA immediately before the write.""" - code, out = _gh(["pr", "view", str(int(pr)), "--json", "commits"], cwd=repo) + code, out = _gh(["pr", "view", str(int(pr)), "--json", "headRefOid"], cwd=repo) if code != 0: return "", out[:400] try: - commits = json.loads(out or "{}").get("commits") or [] + sha = str(json.loads(out or "{}").get("headRefOid") or "") except (AttributeError, json.JSONDecodeError): - commits = [] - sha = str(commits[-1].get("oid") or "") if commits else "" + sha = "" return sha, "" if sha else "PR head SHA missing from gh response" diff --git a/test/test_e2e_strict_mock.py b/test/test_e2e_strict_mock.py index 34064435..a802f2cf 100644 --- a/test/test_e2e_strict_mock.py +++ b/test/test_e2e_strict_mock.py @@ -108,6 +108,8 @@ def gh(args, cwd=None): data: dict = {} if "baseRefName" in fields: data["baseRefName"] = "main" + if "headRefOid" in fields: + data["headRefOid"] = head if "commits" in fields: data["commits"] = [{"oid": head, "messageHeadline": "add feature"}] if "title" in fields: diff --git a/test/test_pr_steps.py b/test/test_pr_steps.py index 50748014..9d94bffa 100644 --- a/test/test_pr_steps.py +++ b/test/test_pr_steps.py @@ -509,8 +509,9 @@ def test_review_event_mapping(comments, pr_state, review_text, event): assert _event_for_review(comments, pr_state, review_text) == event +@pytest.mark.parametrize("commit_count", [1, 100]) def test_post_review_submits_one_review_with_inline_comments( - registry, settings, trace, tmp_path, monkeypatch): + registry, settings, trace, tmp_path, monkeypatch, commit_count): from infermatrix_copilot.engine.steps.pr import publish settings.allow_post = True @@ -533,7 +534,12 @@ def test_post_review_submits_one_review_with_inline_comments( def fake_gh(args, cwd=None): if args[:2] == ["pr", "view"]: - return 0, json.dumps({"commits": [{"oid": "b" * 40}]}) + # A longer PR's first 100 commits can end before the reviewed head. + data = {"commits": [{"oid": "b" * 40 if commit_count == 1 else "a" * 40} + for _ in range(commit_count)]} + if "headRefOid" in args[args.index("--json") + 1].split(","): + data["headRefOid"] = "b" * 40 + return 0, json.dumps(data) captured["args"] = args captured["cwd"] = cwd payload_path = Path(args[args.index("--input") + 1]) @@ -547,6 +553,7 @@ def fake_gh(args, cwd=None): assert result.ok, result.summary assert captured["args"][:3] == ["api", "--method", "POST"] assert captured["args"][3] == "repos/owner/repo/pulls/7/reviews" + assert captured["payload"]["commit_id"] == "b" * 40 assert len(captured["payload"]["comments"]) == 1 assert result.outputs["inline"] == 1 assert result.outputs["downgraded"] == 1 @@ -554,6 +561,31 @@ def fake_gh(args, cwd=None): assert result.outputs["url"].endswith("#review") +@pytest.mark.parametrize("head", [None, ""]) +def test_post_review_without_authoritative_head_does_not_write( + registry, settings, trace, tmp_path, monkeypatch, head): + from infermatrix_copilot.engine.steps.pr import publish + + settings.allow_post = True + state = {"repo_path": str(tmp_path), "task_spec": {"pr": 7, "post": True}, + "diff_text": _REVIEW_DIFF, "review_comments": [_finding("a.py", 10)], + "review_text": "Review complete.", "pr_head_sha": "b" * 40} + monkeypatch.setattr(publish, "_repo_full_name", lambda ctx, repo: "owner/repo") + + def fake_gh(args, cwd=None): + assert args[:2] == ["pr", "view"], "must not submit an unverified review" + assert "headRefOid" in args[args.index("--json") + 1].split(",") + return 0, json.dumps({"headRefOid": head, "commits": [{"oid": "b" * 40}]}) + + monkeypatch.setattr(publish, "_gh", fake_gh) + ctx = _ctx(settings, trace, tmp_path, state) + result = asyncio.run(registry.get("pr.post_review").handler(ctx)) + + assert not result.ok and result.failure is FailureKind.ESCALATE + assert "PR head SHA missing" in result.summary + assert not (ctx.run_dir / "github_review_payload.json").exists() + + def test_post_review_api_failure_escalates_with_payload( registry, settings, trace, tmp_path, monkeypatch): from infermatrix_copilot.engine.steps.pr import publish @@ -573,7 +605,7 @@ def test_post_review_api_failure_escalates_with_payload( def fake_gh(args, cwd=None): if args[:2] == ["pr", "view"]: - return 0, json.dumps({"commits": [{"oid": "c" * 40}]}) + return 0, json.dumps({"headRefOid": "c" * 40}) return 1, "HTTP 422 stale line" monkeypatch.setattr(publish, "_gh", fake_gh) @@ -789,6 +821,62 @@ def test_pinned_diff_ignores_a_stale_origin_tracking_ref(tmp_path): assert "pr.py" in text and "churn.py" not in text +def test_fetch_diff_uses_authoritative_head_with_truncated_commits( + registry, settings, trace, tmp_path, monkeypatch): + """The first 100 commits of a longer PR must not falsely trip the stale gate. + + The authoritative head governs the real fetch, diff and materialized tree, + even when the returned commit list ends at an earlier commit. + """ + from infermatrix_copilot.engine import worktrees as wt_mod + from infermatrix_copilot.engine.steps.pr import fetch as fetch_mod + + work, _base_tip, pr_head = _pr_upstream(tmp_path) + commits = [{"oid": f"{index:040x}"} for index in range(1, 101)] + assert commits[-1]["oid"] != pr_head + + def fake_gh(args, cwd=None): + if args[:2] == ["pr", "view"]: + fields = args[args.index("--json") + 1].split(",") + data = {"baseRefName": "main", "commits": commits} + if "headRefOid" in fields: + data["headRefOid"] = pr_head + return 0, json.dumps(data) + return 0, "{}" + + monkeypatch.setattr(fetch_mod, "_gh", fake_gh) + monkeypatch.setattr(wt_mod.Path, "home", staticmethod(lambda: tmp_path)) + state = {"task_spec": {"kind": "pr_review", "pr": 7, "repo": "vllm-omni", + "expected_head_sha": pr_head}, + "repo_path": str(work)} + result = asyncio.run(registry.get("pr.fetch_diff").handler( + _ctx(settings, trace, tmp_path, state))) + + assert result.ok, result.summary + updates = result.outputs["state_updates"] + assert updates["pr_head_sha"] == pr_head + assert "pr.py" in updates["diff_text"] + assert (Path(updates["repo_path"]) / "pr.py").read_text() == "PR = 1\n" + assert not list(trace.events("expected_head_mismatch")) + + +@pytest.mark.parametrize("head", [None, ""]) +def test_resolve_pr_head_does_not_infer_missing_head_from_commits( + tmp_path, monkeypatch, head): + from infermatrix_copilot.engine.steps.pr import fetch as fetch_mod + + def fake_gh(args, cwd=None): + assert "headRefOid" in args[args.index("--json") + 1].split(",") + return 0, json.dumps({"baseRefName": "main", "headRefOid": head, + "commits": [{"oid": "a" * 40}]}) + + monkeypatch.setattr(fetch_mod, "_gh", fake_gh) + base, actual, error = fetch_mod._resolve_pr_head(tmp_path, 7) + assert base == "main" + assert actual == "" + assert error == "PR head unresolvable from pr view" + + def test_merged_pr_falls_back_to_the_api_diff(settings, trace, tmp_path, monkeypatch): """A merged PR's head is an ancestor of the base, so the three-dot diff is @@ -812,8 +900,7 @@ def test_merged_pr_falls_back_to_the_api_diff(settings, trace, tmp_path, def fake_gh(args, cwd=None): if args[:2] == ["pr", "view"]: - return 0, json.dumps({"baseRefName": "main", - "commits": [{"oid": pr_head}]}) + return 0, json.dumps({"baseRefName": "main", "headRefOid": pr_head}) return 0, "diff --git a/pr.py b/pr.py\n+PR = 1\n" monkeypatch.setattr(fetch_mod, "_gh", fake_gh) @@ -882,8 +969,7 @@ def test_fetch_diff_blocks_on_a_stale_expected_head(settings, trace, tmp_path, def fake_gh(args, cwd=None): calls.append(list(args)) if args[:2] == ["pr", "view"]: - return 0, _json.dumps({"baseRefName": "main", - "commits": [{"oid": actual}]}) + return 0, _json.dumps({"baseRefName": "main", "headRefOid": actual}) return 0, "diff --git a/x b/x" monkeypatch.setattr(fetch_mod, "_gh", fake_gh)