From cd60892a93a3a0b7e9a9b3d45e388023e36d10e2 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Sat, 26 Sep 2026 18:05:39 +0800 Subject: [PATCH] Isolate report-only PR rebase in private worktrees --- doc/architecture/SPEC/engine/steps/pr.md | 20 ++- doc/architecture/SPEC/engine/worktrees.md | 24 ++- doc/architecture/SPEC/task_spec.md | 3 + .../engine/steps/pr/rebase.py | 164 +++++++++++++++--- src/infermatrix_copilot/engine/worktrees.py | 73 +++++++- test/test_idempotency.py | 11 +- test/test_pr_steps.py | 143 ++++++++++++++- 7 files changed, 388 insertions(+), 50 deletions(-) diff --git a/doc/architecture/SPEC/engine/steps/pr.md b/doc/architecture/SPEC/engine/steps/pr.md index c104694b4..545857b45 100644 --- a/doc/architecture/SPEC/engine/steps/pr.md +++ b/doc/architecture/SPEC/engine/steps/pr.md @@ -2,7 +2,7 @@ -`LOC ~1390（6 个文件） · step 库（PR） · refactor-status: ok` +`LOC ~1988（6 个文件） · step 库（PR） · refactor-status: ok` ## 职责 受守卫的推送、只读的 PR 抓取/门禁、PR rebase、PR debug、受门禁的评审发布。 @@ -20,8 +20,9 @@ - `utils.py` —— 纯函数 `extract_signature`（及其正则）。 ## Steps（12 个） -`ci.push`（script/push）；`pr.fetch_diff`、`pr.gate_check`、`pr.checkout_branch`、 +`ci.push`（script/push）；`pr.fetch_diff`、`pr.gate_check`、 `pr.analyze_diff`、`pr.fetch_ci_failures`、`pr.group_failures`（deterministic/read）； +`pr.checkout_branch`（deterministic/write_workspace）； `pr.harvest_debug_knowledge`（deterministic/knowledge）； `pr.rebase_onto_base`、`agent.debug_group`（agent/write_workspace）； `agent.verify_module`（validation/read）；`pr.post_review`（script/push）。 @@ -29,6 +30,9 @@ `pr-debug` 的 report-only 路径不 checkout PR 分支。诊断组数受 playbook 参数和 settings 安全上限共同约束；执行组共享 checkout 时必须顺序修改。`agent.debug_group` 只有在观察到新 commit、干净的 tracked checkout、根因和验证证据时才算成功。 +`pr-rebase` 的 report-only 路径在按 run 分键的私有 detached worktree 内本地 +rebase，不修改配置 checkout 的分支或文件，不推送。该树是一次 run 的可变 scratch， +不是 `pr.fetch_diff` 使用的、按 head 分键的不可变评审快照。 `agent.verify_module` 的咨询结论是 `verified | problem | inconclusive`，未知文本 不能标成已验证。`ci.push` 对 PR debug/rebase 还需同一 HEAD 的 patch-gate 批准。 知识 intake 的 canonical 记录使用 schema v2、完整仓库名和稳定 event ID；缺少 @@ -64,7 +68,13 @@ settings 安全上限共同约束；执行组共享 checkout 时必须顺序修 `gh` 不可用时降级为 BLOCKED，**绝不崩溃**。 - `pr.checkout_branch` 把推导出的 `PushPolicy` **序列化后发布**（**B2**）—— 在推送处 resume 时**绝不能**看到那个 deny-all 的默认值。 +- `pr.checkout_branch` 的 report-only 模式只在私有树里 checkout，并经 + `state_updates` 发布私有 `repo_path` 和配置 checkout 路径；resume 时所有后续 + rebase/analyze/verify step 核对私有树身份、仓库归属及 detached HEAD，树若已被 reaper 清理就 + BLOCKED，绝不回退到配置 checkout。 - `pr.rebase_onto_base`：由受治理 agent 解冲突，或 abort+升级（**工作区始终被还原**）。 + 判断 rebase 是否完成时通过 `git rev-parse --git-path` 定位每棵 worktree + 自己的元数据；不能假设 `.git` 是目录。抓取 base 或生成 diff 失败时 BLOCKED。 - `pr.fetch_ci_failures` 经 profile 选定的 CI provider 富化日志，否则记一条 `capability_gap`（**E2**）；`pr.group_failures` 按**归一化后**的签名分组。 - `pr.post_review` 是**双闸**的（**C5**）。 @@ -89,12 +99,14 @@ settings 安全上限共同约束；执行组共享 checkout 时必须顺序修 不含 agent 治理（那是 `agent_runtime`）。 ## 依赖（允许） -`scopes`、`push`、`ci/*`、`adapters/base`（analyze）、`engine/step`、`.._common`、 +`scopes`、`push`、`ci/*`、`adapters/base`（analyze）、`engine/step`、 +`engine/worktrees`、`.._common`、 `..agent_runtime`。 ## 测试 `test_pr_steps.py`（含钉 ref run 域隔离、head 移动检测、stale expected_head -BLOCK、worktree 分键/拒外来树）、`test_push_and_steps.py`、 +BLOCK、worktree 分键/拒外来树、report-only rebase 的隔离与失效树拦截）、 +`test_push_and_steps.py`、 `test_knowledge_harvest.py`（harvest step + executor crash-then-resume）、 `test_ci_and_repo_map.py`（注意：`test_ci_and_repo_map` monkeypatch 的是 `pr.debug._gh`，即 `pr.fetch_ci_failures` 绑定 `gh` 的那个子模块）； diff --git a/doc/architecture/SPEC/engine/worktrees.md b/doc/architecture/SPEC/engine/worktrees.md index f279dccfd..c9f9e25e3 100644 --- a/doc/architecture/SPEC/engine/worktrees.md +++ b/doc/architecture/SPEC/engine/worktrees.md @@ -1,8 +1,8 @@ # engine/worktrees.py —— 规范 - + -`LOC ~231 · PR-time worktree：身份、物化、持有 · refactor-status: ok` +`LOC ~294 · PR-time worktree：身份、物化、持有 · refactor-status: ok` ## 职责 PR 快照 worktree 的三条性质，每条都对应一个修掉的真实事故： @@ -20,9 +20,14 @@ checkout 之间碰撞，force-remove 曾删掉一个活评审的树）、 `owned_by(repo, dest, sha, git)`；`materialize(repo, sha, dest, git, timeout=300)`；`hold(dest, run_dir)`；`held_paths()`；`GitRunner` 类型别名 （git 以可调用注入 —— 本模块不含 subprocess 策略，可测）。 +`mutable_dest_for(repo, pr, run_dir)` 和 `materialize_mutable(repo, sha, dest, +git, timeout=300)` 专供 report-only PR rebase：每个 run 独占一棵可变的 detached +worktree，允许本地 rebase，又不会改配置的 checkout 或复用评审的不可变快照。 +`owned_by_repository(repo, dest, git)` 在可变树的 HEAD 改动后核对归属。 ## 不变量 -- **reaper 只删匹配 `_MANAGED_DEST`（`-hex8-prN-hex12` 后缀）的树**： +- **reaper 只删匹配 `_MANAGED_DEST`（`-hex8-prN-hex12` 后缀）或 + `_MUTABLE_DEST`（`-hex8-prN-runhex12` 后缀）的树**： worktrees 根是共享 scratch，长期存着其他工具的树 —— 一次"见什么删什么" 的扫描曾经毁掉过别人的工作，这个守卫因此存在。 - `materialize` 对 git 级失败**绝不抛**（调用方决定 block 还是降级）， @@ -33,6 +38,11 @@ timeout=300)`；`hold(dest, run_dir)`；`held_paths()`；`GitRunner` 类型别 恰是 head 门要防的"评错树"。 - 复用验证：`--git-common-dir` 必须解析回请求方仓库自己的 git dir **且** HEAD == sha；force-remove 只对外来/撕裂的树成立，绝不对同身份的活树。 +- 可变树按 run_dir 分键，复用时只核对仓库归属（rebase 会改变 HEAD）， + 拒绝 symlink、重定向到配置 checkout 的路径及外来仓库；调用方仅在 checkout + step 重试时可重置此树。后续 step 经 checkpoint 恢复路径，并在树被清理后 + fail closed，绝不退回 live checkout。调用方还要确认 HEAD 保持 detached， + 防止重置或 rebase 意外移动共享仓库里的命名分支。 - `hold()` 幂等（进程内 memo），经 `lifecycle.register_finalizer` 在每条 退出路径释放，进程死亡时内核兜底 —— 崩掉的 run 绝不永久钉住一棵树。 - **持有挂在"使用"而不是"创建"上**（`engine/steps/_common.py::repo_path` @@ -42,7 +52,7 @@ timeout=300)`；`hold(dest, run_dir)`；`held_paths()`；`GitRunner` 类型别 ## 边界 —— 不属于这里 无 subprocess 策略（git runner 注入）；无 run 生命周期逻辑（持有原语之外 —— 那是 `engine/lifecycle.py`）；何时物化/何时 block 是 -`engine/steps/pr/fetch.py` 的决定。 +`engine/steps/pr/fetch.py` 和 `engine/steps/pr/rebase.py` 的决定。 ## 依赖（允许） stdlib（hashlib/os/re/time/pathlib）+ `.lifecycle` @@ -50,9 +60,9 @@ stdlib（hashlib/os/re/time/pathlib）+ `.lifecycle` ## 测试 `test_pr_steps.py`（materialize 创建与复用、同 basename 仓库分键、 -拒绝外来树、fetch_diff 端到端钉住）；`test_idempotency.py` -（reaper 只碰自己键出的树、放过被持有的树）。 +拒绝外来树、fetch_diff 端到端钉住、report-only rebase 的隔离与 resume）； +`test_idempotency.py`（reaper 只碰自己键出的不可变及可变树、放过被持有的树）。 ## 重构备注 -新模块（PR2）。键格式与 `_MANAGED_DEST` 必须同步演进 —— 改其一忘其二， +新模块（PR2）。键格式与 `_MANAGED_DEST` / `_MUTABLE_DEST` 必须同步演进 —— 改其一忘其二， reaper 就会开始放过（或误删）新格式的树。 diff --git a/doc/architecture/SPEC/task_spec.md b/doc/architecture/SPEC/task_spec.md index 66a10d6b0..8861f7b20 100644 --- a/doc/architecture/SPEC/task_spec.md +++ b/doc/architecture/SPEC/task_spec.md @@ -23,6 +23,9 @@ expected_head_sha?, repo_path?)`；property `tier`、`read_only`、 - **C1**：**不存在可设置的 tier 字段**；`tier = KIND_TIER[kind]` —— 文本无法把它扩大。 - 只读 kind 的 `read_only` = `not post`，其余为 `report_only`； `confirm_required = not read_only`。 +- 这里的 `read_only` 是**任务权限/对外发布**的推导，不表示执行期间零文件写入： + `pr_rebase` 的 report-only 模式允许在按 run 隔离的 disposable worktree + 本地 rebase，但不能修改配置 checkout 的分支/文件或推送 PR head。 - **快照绑定字段只收窄，绝不扩权**（C1 完整无损）：`expected_head_sha` （field_validator 强制 `FULL_SHA_RE`；设置后 run 只准评审这个 head， 否则以 stale 停下）与 `repo_path`（预约时冻结、由 diff --git a/src/infermatrix_copilot/engine/steps/pr/rebase.py b/src/infermatrix_copilot/engine/steps/pr/rebase.py index cb897bfc1..7e6b1407d 100644 --- a/src/infermatrix_copilot/engine/steps/pr/rebase.py +++ b/src/infermatrix_copilot/engine/steps/pr/rebase.py @@ -13,6 +13,8 @@ from dataclasses import asdict from pathlib import Path +from ... import worktrees +from ...lifecycle import FileLockingUnavailable from ....push import PushPolicy from ....scopes import post_plan_scope from ...step import FailureKind, StepContext, StepResult @@ -23,11 +25,67 @@ from .._common import task_spec as _task_spec -@step("pr.checkout_branch", "deterministic", "read", +def _checked_rebase_repo(ctx: StepContext) -> Path | StepResult: + """Resolve the checkout used after checkout_branch, failing closed on resume. + + A report-only checkpoint may outlive its disposable worktree. Never fall + back to the configured checkout or follow a substituted worktree path. + """ + repo = _repo_path(ctx) + if repo is None or not repo.is_dir(): + return StepResult(False, FailureKind.BLOCKED, + "PR rebase checkout is missing; restart in a new run") + spec = _task_spec(ctx) + if spec.get("report_only"): + source = ctx.state.get("pr_rebase_source_repo") + try: + expected = worktrees.mutable_dest_for(Path(source), int(spec["pr"]), + ctx.run_dir) + except (TypeError, ValueError, KeyError): + return StepResult(False, FailureKind.BLOCKED, + "private PR rebase checkout identity is missing") + if repo.is_symlink() or repo != expected: + return StepResult(False, FailureKind.BLOCKED, + "private PR rebase checkout identity changed") + if not worktrees.hold(repo, ctx.run_dir) or not repo.is_dir(): + return StepResult(False, FailureKind.BLOCKED, + "private PR rebase checkout is unavailable") + try: + owned, why = worktrees.owned_by_repository(Path(source), repo, _git) + except OSError: + owned, why = False, "worktree disappeared" + if not owned: + return StepResult(False, FailureKind.BLOCKED, + f"private PR rebase checkout is unavailable ({why})") + rc, _ = _git(repo, "symbolic-ref", "--quiet", "HEAD") + if rc != 1: + return StepResult(False, FailureKind.BLOCKED, + "private PR rebase checkout is no longer detached") + return repo + + +def _rebase_in_progress(repo: Path) -> bool: + """Use Git's per-worktree metadata path, which is not always a .git dir.""" + for marker in ("rebase-merge", "rebase-apply"): + rc, location = _git(repo, "rev-parse", "--git-path", marker) + if rc != 0: + return True + path = Path(location) + if not path.is_absolute(): + path = repo / path + if path.exists(): + return True + return False + + +@step("pr.checkout_branch", "deterministic", "write_workspace", "Checkout PR head (fork-aware); derive PushPolicy.") async def _pr_checkout(ctx: StepContext) -> StepResult: - """Fetch PR metadata, add the fork remote if needed, check out a local - pr-- branch, and derive the PushPolicy for later steps.""" + """Fetch the PR head and derive PushPolicy for later steps. + + Report-only work runs in a private detached worktree keyed to this run; + it may rebase there but cannot change the configured checkout or push. + """ repo = _repo_path(ctx) spec = _task_spec(ctx) pr = spec.get("pr") @@ -60,34 +118,73 @@ async def _pr_checkout(ctx: StepContext) -> StepResult: rc, out = _git(repo, "fetch", remote, head_ref) if rc != 0: return StepResult(False, FailureKind.BLOCKED, f"fetch {remote}/{head_ref} failed: {out[:400]}") - local = f"pr-{pr}-{head_ref}".replace("/", "-") - rc, out = _git(repo, "checkout", "-B", local, "FETCH_HEAD") + rc, fetched_head = _git(repo, "rev-parse", "FETCH_HEAD") if rc != 0: - return StepResult(False, FailureKind.BLOCKED, f"checkout failed: {out[:400]}") - rc, initial_head_sha = _git(repo, "rev-parse", "HEAD") - if rc != 0: - return StepResult(False, FailureKind.BLOCKED, f"cannot identify PR head: {initial_head_sha[:400]}") + return StepResult(False, FailureKind.BLOCKED, + f"cannot identify fetched PR head: {fetched_head[:400]}") + + report_only = bool(spec.get("report_only", False)) + local = f"pr-{pr}-{head_ref}".replace("/", "-") + checkout = repo + if report_only: + checkout = worktrees.mutable_dest_for(repo, int(pr), ctx.run_dir) + try: + ok, detail = worktrees.materialize_mutable( + repo, fetched_head, checkout, _git) + except FileLockingUnavailable as exc: + return StepResult(False, FailureKind.BLOCKED, str(exc)) + if not ok: + return StepResult(False, FailureKind.BLOCKED, detail) + if not worktrees.hold(checkout, ctx.run_dir): + return StepResult(False, FailureKind.BLOCKED, + "could not hold private rebase worktree") + rc, _ = _git(checkout, "symbolic-ref", "--quiet", "HEAD") + if rc != 1: + return StepResult(False, FailureKind.BLOCKED, + "private checkout is no longer detached") + # This step can be retried after a crash before its checkpoint. Its + # private tree is safe to reset; a completed step is replayed instead, + # preserving in-progress work for a later resume. + _git(checkout, "rebase", "--abort") + if _rebase_in_progress(checkout): + return StepResult(False, FailureKind.BLOCKED, + "private checkout still has a rebase in progress") + for args in (("reset", "--hard", fetched_head), ("clean", "-fd")): + rc, out = _git(checkout, *args) + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, + f"private checkout reset failed: {out[:400]}") + local = "(detached private worktree)" + else: + rc, out = _git(repo, "checkout", "-B", local, "FETCH_HEAD") + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, f"checkout failed: {out[:400]}") + + rc, initial_head_sha = _git(checkout, "rev-parse", "HEAD") + if rc != 0 or initial_head_sha != fetched_head: + return StepResult(False, FailureKind.BLOCKED, + f"checkout head differs from fetched PR head: {initial_head_sha[:400]}") force = bool(ctx.params.get("force_push", False)) policy = PushPolicy( - allowed=not spec.get("report_only", False), + allowed=not report_only, remote=remote, branch=head_ref, force_with_lease=force, ) - ctx.state.update( + updates = dict( pr_head_ref=head_ref, pr_head_remote=remote, pr_base_branch=base, pr_local_branch=local, pr_initial_head_sha=initial_head_sha, - push_policy=policy, + push_policy=asdict(policy), ) + if report_only: + updates["repo_path"] = str(checkout) + updates["pr_rebase_source_repo"] = str(repo) + ctx.state.update(updates) + ctx.state["push_policy"] = policy return StepResult(True, summary=f"checked out PR #{pr} ({remote}/{head_ref} -> {local})", outputs={"local_branch": local, "base": base, "remote": remote, # push_policy serialized JSON-simple; ci.push # rehydrates dicts back into a PushPolicy - "state_updates": { - "pr_head_ref": head_ref, "pr_head_remote": remote, - "pr_base_branch": base, "pr_local_branch": local, - "pr_initial_head_sha": initial_head_sha, - "push_policy": asdict(policy), - }}) + "state_updates": updates}) _CONFLICT_GUIDANCE = """You are resolving git rebase conflicts. Work only inside the repository. @@ -115,13 +212,18 @@ async def _pr_rebase_onto_base(ctx: StepContext) -> StepResult: sides, `rebase --continue`); success requires no rebase left in progress. If the agent can't finish, or when there is no LLM, `rebase --abort` restores the workspace and the step returns ESCALATE with the conflict file list.""" - repo = _repo_path(ctx) + repo = _checked_rebase_repo(ctx) + if isinstance(repo, StepResult): + return repo base = ctx.state.get("pr_base_branch", "main") base_remote = ctx.params.get("base_remote", "origin") rc, out = _git(repo, "fetch", base_remote, base) if rc != 0: return StepResult(False, FailureKind.BLOCKED, f"fetch {base_remote}/{base} failed: {out[:400]}") rc, base_sha = _git(repo, "rev-parse", "FETCH_HEAD") + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot identify fetched base: {base_sha[:400]}") ctx.state["rebase_base_sha"] = base_sha rc, out = _git(repo, "rebase", "FETCH_HEAD") @@ -150,8 +252,7 @@ async def _pr_rebase_onto_base(ctx: StepContext) -> StepResult: scope=post_plan_scope(repo), # write the workspace; run_shell allowed max_iters=ctx.settings.max_agent_iters, ) - in_progress = (Path(repo) / ".git" / "rebase-merge").exists() or \ - (Path(repo) / ".git" / "rebase-apply").exists() + in_progress = _rebase_in_progress(repo) if result.ok and not in_progress: outputs = {**result.outputs} outputs.setdefault("state_updates", {})["rebase_base_sha"] = base_sha @@ -191,9 +292,14 @@ async def _pr_analyze_diff(ctx: StepContext) -> StepResult: Publishes to state (B2 `state_updates`): `affected_modules` / `touched_modules` (the module list) and `primary_files` (the changed paths, as `*`-globs for the diff-summary scope check).""" - repo = _repo_path(ctx) + repo = _checked_rebase_repo(ctx) + if isinstance(repo, StepResult): + return repo base_sha = ctx.state.get("rebase_base_sha", "HEAD~1") rc, out = _git(repo, "diff", "--numstat", f"{base_sha}..HEAD") + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot analyze rebased diff: {out[:400]}") changed: list[str] = [] for line in out.splitlines(): parts = line.split("\t") @@ -204,8 +310,11 @@ async def _pr_analyze_diff(ctx: StepContext) -> StepResult: adapter = None try: from ....adapters.base import AdapterRegistry + # The adapter is registered for the configured checkout; report-only + # steps run in a private path with the same repository content. + adapter_repo = ctx.state.get("pr_rebase_source_repo") or repo adapter = AdapterRegistry(ctx.settings.adapters_dir).resolve( - repo_path=str(repo)) if repo else None + repo_path=str(adapter_repo)) if adapter_repo else None except Exception: adapter = None modules: list[str] = [] @@ -230,6 +339,9 @@ async def _pr_analyze_diff(ctx: StepContext) -> StepResult: async def _verify_module(ctx: StepContext) -> StepResult: """Read-only per-module sanity check of the rebased diff. Advisory: the fail-closed gate is review.patch_gate before push.""" + repo = _checked_rebase_repo(ctx) + if isinstance(repo, StepResult): + return repo module = ctx.item or "all" if ctx.llm is None or not ctx.llm.available: return no_llm_gap(ctx, "agent.verify_module", @@ -237,9 +349,11 @@ async def _verify_module(ctx: StepContext) -> StepResult: "remains fail-closed", summary=f"{module}: verification skipped (no LLM); patch " "gate before push remains fail-closed") - repo = _repo_path(ctx) base_sha = ctx.state.get("rebase_base_sha", "HEAD~1") - _, diff = _git(repo, "diff", f"{base_sha}..HEAD") + rc, diff = _git(repo, "diff", f"{base_sha}..HEAD") + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot verify rebased diff: {diff[:400]}") reply = ctx.llm.create( system=("You verify a rebased PR branch. Check the diff for rebase damage: " "dropped hunks, duplicated code, mis-merged imports, references to " diff --git a/src/infermatrix_copilot/engine/worktrees.py b/src/infermatrix_copilot/engine/worktrees.py index 21e6db86e..c0d6ec062 100644 --- a/src/infermatrix_copilot/engine/worktrees.py +++ b/src/infermatrix_copilot/engine/worktrees.py @@ -82,17 +82,34 @@ def dest_for(repo: Path | str, pr: int, sha: str, *, return base / f"{Path(repo).name}-{owner_tag(repo)}-pr{int(pr)}-{sha[:12]}" -# A destination this module produced: `--pr-`. The +def mutable_dest_for(repo: Path | str, pr: int, run_dir: Path | str, *, + root: Path | None = None) -> Path: + """A private mutable PR worktree, keyed to one run rather than one head. + + Report-only rebase changes HEAD locally, so the immutable review-tree key + cannot be reused: another reader at the original head could see edits. + The run-directory identity remains stable across resume and distinct from + every other run of the same PR. + """ + base = Path(root) if root is not None else worktree_root() + run_tag = hashlib.sha256(canonical(run_dir).encode("utf-8")).hexdigest()[:12] + return base / f"{Path(repo).name}-{owner_tag(repo)}-pr{int(pr)}-run{run_tag}" + + +# Destinations this module produced: immutable `--pr-` +# or mutable `--pr-run`. The # reaper removes ONLY these. The worktrees root is a shared scratch directory # that has held other tooling's trees under other naming schemes for a long # time, and a sweep that deleted anything it found there would destroy work it # knows nothing about — as one did, before this guard existed. _MANAGED_DEST = re.compile(r"-[0-9a-f]{8}-pr\d+-[0-9a-f]{12}\Z") +_MUTABLE_DEST = re.compile(r"-[0-9a-f]{8}-pr\d+-run[0-9a-f]{12}\Z") def is_managed_dest(dest: Path | str) -> bool: """Whether `dest` is a worktree this module keys, and may therefore reap.""" - return bool(_MANAGED_DEST.search(Path(dest).name)) + name = Path(dest).name + return bool(_MANAGED_DEST.search(name) or _MUTABLE_DEST.search(name)) def lock_path(dest: Path | str) -> Path: @@ -132,6 +149,18 @@ def owned_by(repo: Path, dest: Path, sha: str, git: GitRunner) -> tuple[bool, st Both halves matter: the git-dir check catches a tree belonging to a different clone that landed on this path, and the HEAD check catches a torn or interrupted materialization.""" + same_repo, reason = owned_by_repository(repo, dest, git) + if not same_repo: + return False, reason + code, head = git(dest, "rev-parse", "HEAD") + if code != 0 or head.strip() != sha: + return False, f"head {head.strip()[:12] or '?'} != {sha[:12]}" + return True, "owned" + + +def owned_by_repository(repo: Path, dest: Path, + git: GitRunner) -> tuple[bool, str]: + """Check Git ownership without requiring an immutable HEAD.""" code, common = git(dest, "rev-parse", "--git-common-dir") if code != 0: return False, "not a git worktree" @@ -147,12 +176,46 @@ def owned_by(repo: Path, dest: Path, sha: str, git: GitRunner) -> tuple[bool, st mine_path = Path(repo) / mine_path if canonical(common_path) != canonical(mine_path): return False, "worktree belongs to a different repository" - code, head = git(dest, "rev-parse", "HEAD") - if code != 0 or head.strip() != sha: - return False, f"head {head.strip()[:12] or '?'} != {sha[:12]}" return True, "owned" +def materialize_mutable(repo: Path, sha: str, dest: Path, git: GitRunner, *, + timeout: float = 300.0) -> tuple[bool, str]: + """Create or reuse one run's private worktree without touching a foreign path. + + A reused tree may have a rebased HEAD; the caller resets it only when the + checkout step itself is being retried. A completed checkout is replayed + from the checkpoint on resume and keeps its in-progress work. + """ + require_file_locking() + fd = _open_lock(dest) + try: + if not _flock_blocking(fd, fcntl.LOCK_EX, timeout): + return False, f"worktree lock busy for {timeout:.0f}s" + try: + # A symlink to the source checkout passes the common-git-dir check, + # but resetting it would rewrite the user's live files. Never + # follow a redirected private destination, including broken links. + if dest.is_symlink() or canonical(dest) == canonical(repo): + return False, "private worktree path is redirected" + if dest.exists(): + ok, why = owned_by_repository(repo, dest, git) + if not ok: + return False, f"private worktree path is not owned ({why})" + return True, "reused private worktree" + dest.parent.mkdir(parents=True, exist_ok=True) + code, out = git(repo, "worktree", "add", "--detach", str(dest), sha) + if code != 0: + return False, f"worktree add failed: {out[:300]}" + return True, "created private worktree" + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + except OSError as exc: + return False, f"worktree error: {exc}" + finally: + os.close(fd) + + def materialize(repo: Path, sha: str, dest: Path, git: GitRunner, *, timeout: float = 300.0) -> tuple[bool, str]: """Create (or reuse) a detached worktree of `repo` at `sha` under `dest`, diff --git a/test/test_idempotency.py b/test/test_idempotency.py index 3ae537b5d..430558c29 100644 --- a/test/test_idempotency.py +++ b/test/test_idempotency.py @@ -371,17 +371,22 @@ def test_reaper_only_touches_worktrees_it_keys(settings, tmp_path, git_repo): capture_output=True, text=True).stdout.strip() ours = wt.dest_for(git_repo, 1, sha, root=root) assert wt.materialize(git_repo, sha, ours, _git)[0] + mutable = wt.mutable_dest_for(git_repo, 1, tmp_path / "run", root=root) + assert wt.materialize_mutable(git_repo, sha, mutable, _git)[0] + (mutable / "scratch.txt").write_text("disposable rebase scratch") # a foreign tree, named the way earlier tooling named them foreign = root / "vllm-omni-eval-pr4762" assert wt.materialize(git_repo, sha, foreign, _git)[0] - for dest in (ours, foreign): + for dest in (ours, mutable, foreign): os.utime(dest, (0, 0)) - assert wt.is_managed_dest(ours) and not wt.is_managed_dest(foreign) + assert wt.is_managed_dest(ours) and wt.is_managed_dest(mutable) + assert not wt.is_managed_dest(foreign) counts = idem.reap_stale(settings.run_root, worktree_root=root) assert not ours.exists() # ours, aged out + assert not mutable.exists() # disposable mutable tree, aged out assert foreign.exists() # never ours to remove - assert counts["worktrees"] == 1 + assert counts["worktrees"] == 2 def test_reaper_leaves_a_held_worktree_and_removes_an_unheld_one(settings, diff --git a/test/test_pr_steps.py b/test/test_pr_steps.py index a24bc5555..507480145 100644 --- a/test/test_pr_steps.py +++ b/test/test_pr_steps.py @@ -69,13 +69,130 @@ def test_checkout_sets_branch_and_push_policy(registry, settings, trace, tmp_pat assert policy.allowed and policy.branch == "feature" and policy.force_with_lease -def test_checkout_report_only_disallows_push(registry, settings, trace, tmp_path, pr_repos): +def test_checkout_report_only_disallows_push(registry, settings, trace, tmp_path, + pr_repos, monkeypatch): + from infermatrix_copilot.engine import worktrees + from infermatrix_copilot.engine.lifecycle import finalize + + monkeypatch.setattr(worktrees, "worktree_root", lambda: tmp_path / "private") _, work = pr_repos + original_head = _git(work, "rev-parse", "HEAD") state = {"repo_path": str(work), "task_spec": {"pr": 7, "report_only": True}, "pr_meta": {"headRefName": "feature", "remote": "origin"}} step = registry.get("pr.checkout_branch") - assert asyncio.run(step.handler(_ctx(settings, trace, tmp_path, state))).ok + result = asyncio.run(step.handler(_ctx(settings, trace, tmp_path, state))) + assert result.ok, result.summary assert state["push_policy"].allowed is False + private = Path(state["repo_path"]) + assert private != work and private.is_dir() + assert _git(private, "rev-parse", "HEAD") == _git(work, "rev-parse", "origin/feature") + assert _git(work, "rev-parse", "HEAD") == original_head + assert _git(work, "branch", "--show-current") == "main" + assert _git(work, "status", "--porcelain") == "" + assert "pr-7-feature" not in _git(work, "branch", "--list") + assert result.outputs["state_updates"]["repo_path"] == str(private) + asyncio.run(finalize(tmp_path / "rundir", None)) + + +def test_report_only_rebase_stays_private_across_resume( + registry, settings, trace, tmp_path, pr_repos, monkeypatch, +): + from infermatrix_copilot.engine import worktrees + from infermatrix_copilot.engine.lifecycle import finalize + from infermatrix_copilot.engine.steps.pr.rebase import _rebase_in_progress + + monkeypatch.setattr(worktrees, "worktree_root", lambda: tmp_path / "private") + origin, work = pr_repos + (origin / "other.py").write_text("o = 1\n") + _git(origin, "add", ".") + _git(origin, "commit", "-q", "-m", "main moves on") + original_head = _git(work, "rev-parse", "HEAD") + state = {"repo_path": str(work), "task_spec": {"pr": 7, "report_only": True}, + "pr_meta": {"headRefName": "feature", "remote": "origin"}} + checkout = asyncio.run(registry.get("pr.checkout_branch").handler( + _ctx(settings, trace, tmp_path, state))) + assert checkout.ok, checkout.summary + private = Path(state["repo_path"]) + # A linked worktree has a .git file. Rebase state lives in its Git metadata. + marker = Path(_git(private, "rev-parse", "--git-path", "rebase-merge")) + if not marker.is_absolute(): + marker = private / marker + marker.mkdir() + assert _rebase_in_progress(private) + marker.rmdir() + assert not _rebase_in_progress(private) + # Executor resume replays only JSON-simple state_updates, not the handler. + resumed = {"task_spec": state["task_spec"], + **checkout.outputs["state_updates"]} + for name in ("pr.rebase_onto_base", "pr.analyze_diff"): + result = asyncio.run(registry.get(name).handler( + _ctx(settings, trace, tmp_path, resumed))) + assert result.ok, f"{name}: {result.summary}" + assert (private / "other.py").exists() and (private / "feature.py").exists() + assert resumed["affected_modules"] == ["root"] + assert _git(private, "rev-parse", "HEAD") != checkout.outputs["state_updates"]["pr_initial_head_sha"] + assert _git(work, "rev-parse", "HEAD") == original_head + assert _git(work, "branch", "--show-current") == "main" + assert _git(work, "status", "--porcelain") == "" + assert not (work / "other.py").exists() + assert not resumed["push_policy"]["allowed"] + + # A second run of the same PR gets a distinct mutable checkout. + second_root = tmp_path / "second" + second_state = {"repo_path": str(work), "task_spec": state["task_spec"], + "pr_meta": state["pr_meta"]} + second = asyncio.run(registry.get("pr.checkout_branch").handler( + _ctx(settings, trace, second_root, second_state))) + assert second.ok and second_state["repo_path"] != str(private) + asyncio.run(finalize(tmp_path / "rundir", None)) + asyncio.run(finalize(second_root / "rundir", None)) + + +def test_report_only_rebase_rejects_redirected_or_missing_private_tree( + registry, settings, trace, tmp_path, pr_repos, monkeypatch, +): + from infermatrix_copilot.engine import worktrees + from infermatrix_copilot.engine.lifecycle import finalize + + monkeypatch.setattr(worktrees, "worktree_root", lambda: tmp_path / "private") + _, work = pr_repos + original_head = _git(work, "rev-parse", "HEAD") + state = {"repo_path": str(work), "task_spec": {"pr": 7, "report_only": True}, + "pr_meta": {"headRefName": "feature", "remote": "origin"}} + dest = worktrees.mutable_dest_for(work, 7, tmp_path / "rundir") + dest.parent.mkdir(parents=True) + dest.symlink_to(work, target_is_directory=True) + refused = asyncio.run(registry.get("pr.checkout_branch").handler( + _ctx(settings, trace, tmp_path, state))) + assert not refused.ok and "redirected" in refused.summary + assert _git(work, "rev-parse", "HEAD") == original_head + assert _git(work, "branch", "--show-current") == "main" + dest.unlink() + + checkout = asyncio.run(registry.get("pr.checkout_branch").handler( + _ctx(settings, trace, tmp_path, state))) + assert checkout.ok, checkout.summary + resumed = {"task_spec": state["task_spec"], + **checkout.outputs["state_updates"]} + _git(dest, "checkout", "-q", "-b", "unexpected") + attached = asyncio.run(registry.get("pr.rebase_onto_base").handler( + _ctx(settings, trace, tmp_path, resumed))) + assert not attached.ok and "detached" in attached.summary + # A real retry starts after run teardown releases the prior shared hold. + asyncio.run(finalize(tmp_path / "rundir", None)) + retry_state = {"repo_path": str(work), "task_spec": state["task_spec"], + "pr_meta": state["pr_meta"]} + retry = asyncio.run(registry.get("pr.checkout_branch").handler( + _ctx(settings, trace, tmp_path, retry_state))) + assert not retry.ok and "detached" in retry.summary + assert _git(work, "branch", "--show-current") == "main" + _git(dest, "checkout", "-q", "--detach") + asyncio.run(finalize(tmp_path / "rundir", None)) + _git(work, "worktree", "remove", "--force", str(dest)) + missing = asyncio.run(registry.get("pr.rebase_onto_base").handler( + _ctx(settings, trace, tmp_path, resumed))) + assert not missing.ok and "missing" in missing.summary + assert _git(work, "rev-parse", "HEAD") == original_head def test_patch_gate_reviews_committed_range_and_push_binds_head( @@ -179,8 +296,15 @@ def test_clean_rebase_and_analyze(registry, settings, trace, tmp_path, pr_repos) assert state["primary_files"] == ["*feature.py"] +@pytest.mark.parametrize("report_only", [False, True]) def test_conflict_without_llm_aborts_and_escalates(registry, settings, trace, - tmp_path, pr_repos): + tmp_path, pr_repos, + report_only, monkeypatch): + from infermatrix_copilot.engine import worktrees + from infermatrix_copilot.engine.lifecycle import finalize + from infermatrix_copilot.engine.steps.pr.rebase import _rebase_in_progress + + monkeypatch.setattr(worktrees, "worktree_root", lambda: tmp_path / "private") origin, work = pr_repos # conflicting change on main touching the same line as a new feature commit (origin / "core.py").write_text("x = 2\n") @@ -192,7 +316,7 @@ def test_conflict_without_llm_aborts_and_escalates(registry, settings, trace, _git(origin, "commit", "-q", "-m", "feature edits core") _git(origin, "checkout", "-q", "main") - state = {"repo_path": str(work), "task_spec": {"pr": 7}, + state = {"repo_path": str(work), "task_spec": {"pr": 7, "report_only": report_only}, "pr_meta": {"headRefName": "feature", "remote": "origin"}} assert asyncio.run(registry.get("pr.checkout_branch").handler( _ctx(settings, trace, tmp_path, state))).ok @@ -201,9 +325,16 @@ def test_conflict_without_llm_aborts_and_escalates(registry, settings, trace, assert not result.ok and result.failure is FailureKind.ESCALATE assert "core.py" in result.outputs["conflicts"] # rebase aborted -> workspace clean, no rebase in progress - assert _git(work, "status", "--porcelain") == "" - assert not (work / ".git" / "rebase-merge").exists() + checkout = Path(state["repo_path"]) + assert _git(checkout, "status", "--porcelain") == "" + assert not _rebase_in_progress(checkout) + if report_only: + assert checkout != work + assert _git(work, "branch", "--show-current") == "main" + assert (work / "core.py").read_text() == "x = 1\n" + assert _git(work, "status", "--porcelain") == "" assert list(trace.events("rebase_conflict")) + asyncio.run(finalize(tmp_path / "rundir", None)) def test_extract_signature_prefers_root_cause_over_symptom():