diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 06797054a..45b09313b 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -94,6 +94,9 @@ - Fixed `models.yml` remote compaction schema support for V2 streaming endpoint fields. ([#4146](https://github.com/can1357/oh-my-pi/issues/4146)) - Fixed the SSH tool to reject `cwd` values of `~` and `~/...` before sending guaranteed-bad quoted tilde paths to remote POSIX shells. ([#4002](https://github.com/can1357/oh-my-pi/issues/4002)) +### Fixed + +- Fixed isolated task merges failing when the parent working tree carried WIP for a file the isolated subagent also touched. `commitPatchToBranchWorktree` now tries plain apply and `git apply --3way` first (agent-only outcome when the WIP-side blob is tracked in HEAD), then falls back to seeding the temp worktree with the baseline WIP so the delta patch's HEAD+WIP context matches, and rewinds WIP-only files afterward so they don't leak into the branch commit. Covers untracked WIP files, staged-new WIP files, and overlaps `--3way` cannot resolve. ([#4136](https://github.com/can1357/oh-my-pi/issues/4136)) ## [16.2.12] - 2026-07-01 diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index f6bac2d5d..4f62313d9 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -481,41 +481,148 @@ function baselineHasRootWip(baseline: RepoBaseline): boolean { return !!(baseline.staged.trim() || baseline.unstaged.trim() || baseline.untrackedPatch.trim()); } +/** + * Baseline WIP context needed to safely apply a delta patch whose hunks were + * captured against `HEAD + WIP` (see {@link captureRepoDeltaPatch}). Passed + * whenever {@link baselineHasRootWip} is true so + * {@link commitPatchToBranchWorktree} can replay the WIP into the temp + * worktree first, then rewind WIP-only files after applying the delta. + */ +interface BaselineWipContext { + readonly staged: string; + readonly unstaged: string; + readonly untrackedPatch: string; + /** Untracked file paths present in the baseline (never in HEAD). */ + readonly untracked: readonly string[]; +} + +function collectWipPatches(wip: BaselineWipContext | undefined): string[] { + if (!wip) return []; + return [wip.staged, wip.unstaged, wip.untrackedPatch].filter(p => p.trim()); +} + async function commitPatchToBranchWorktree( tmpDir: string, taskId: string, patchText: string, message: string, author?: git.CommitAuthor, + baselineWip?: BaselineWipContext, ): Promise { + // Try the two clean paths first — they yield an agent-only commit and are + // the happy case when the temp worktree can resolve the patch against + // HEAD directly: + // + // 1. Plain apply — works when WIP context happens to match HEAD (e.g. + // WIP-touched files that the delta patch doesn't reference). + // 2. `--3way` — works when the WIP-side blob is tracked in HEAD and + // lives in the shared ODB (captureDeltaPatch seeded it while writing + // the synthetic baseline tree). The 3-way merge subtracts WIP, + // producing an agent-only commit even when WIP and agent modify the + // same tracked file at unrelated lines. + // + // If both fail (untracked WIP files, staged-new WIP files, or overlap that + // --3way can't resolve — see #4136), replay the WIP into the worktree + // first so the delta's context lines match, then rewind WIP-only files so + // they don't leak into the commit. Files touched by BOTH WIP and delta + // keep their combined state; the parent's stash-pop reconciles the WIP + // side via 3-way merge on merge-back. + let plainErr: git.GitCommandError | undefined; try { await git.patch.applyText(tmpDir, patchText); } catch (err) { if (!(err instanceof git.GitCommandError)) throw err; - // Plain apply rejects when the parent checkout carries unrelated dirty - // context near the task's edits; retry with --3way before giving up. + plainErr = err; + } + if (plainErr) { + let threeWayErr: git.GitCommandError | undefined; try { await git.patch.applyText(tmpDir, patchText, { threeWay: true }); - } catch (threeWayErr) { - if (threeWayErr instanceof git.GitCommandError) { + } catch (err) { + if (!(err instanceof git.GitCommandError)) throw err; + threeWayErr = err; + } + if (threeWayErr) { + const wipPatches = collectWipPatches(baselineWip); + if (wipPatches.length === 0 || !baselineWip) { const stderr = threeWayErr.result.stderr.slice(0, 2000); logger.error("commitToBranch: git apply --3way failed", { taskId, exitCode: threeWayErr.result.exitCode, stderr, - initialStderr: err.result.stderr.slice(0, 2000), + initialStderr: plainErr.result.stderr.slice(0, 2000), patchSize: patchText.length, patchHead: patchText.slice(0, 500), }); throw new Error(`git apply --3way failed for task ${taskId}: ${stderr}`); } - throw threeWayErr; + try { + // `git apply --3way` leaves conflict markers in `U` files when + // it can't resolve; reset the worktree so the WIP-seeded retry + // starts from a clean HEAD tree. + await git.reset(tmpDir, { hard: true, target: "HEAD" }); + await applyDeltaOverBaselineWip(tmpDir, taskId, patchText, wipPatches, baselineWip); + } catch (wipErr) { + if (!(wipErr instanceof git.GitCommandError)) throw wipErr; + const stderr = wipErr.result.stderr.slice(0, 2000); + logger.error("commitToBranch: git apply with baseline WIP failed", { + taskId, + exitCode: wipErr.result.exitCode, + stderr, + threeWayStderr: threeWayErr.result.stderr.slice(0, 2000), + initialStderr: plainErr.result.stderr.slice(0, 2000), + patchSize: patchText.length, + patchHead: patchText.slice(0, 500), + }); + throw new Error(`git apply with baseline WIP failed for task ${taskId}: ${stderr}`); + } } } + await git.stage.files(tmpDir); await git.commit(tmpDir, message, author ? { author } : {}); } +/** + * Replay baseline WIP into the temp worktree so the delta patch's HEAD+WIP + * context matches, apply the delta, then rewind files WIP touched but the + * delta didn't — HEAD-tracked files are restored via `git restore`, untracked + * or staged-new WIP files are removed from the worktree. The commit that + * follows reflects agent's delta plus any overlap with WIP; parent's + * stash-pop reconciles the WIP side on merge-back. + */ +async function applyDeltaOverBaselineWip( + tmpDir: string, + _taskId: string, + patchText: string, + wipPatches: readonly string[], + baselineWip: BaselineWipContext, +): Promise { + for (const wip of wipPatches) { + await git.patch.applyText(tmpDir, wip); + } + await git.patch.applyText(tmpDir, patchText); + + const wipFiles = new Set(wipPatches.flatMap(patchTouchedFiles)); + const deltaFiles = new Set(patchTouchedFiles(patchText)); + const wipOnly = [...wipFiles].filter(f => !deltaFiles.has(f)); + if (wipOnly.length === 0) return; + + // Any wipOnly file baselined as untracked cannot be in HEAD. + // Everything else may or may not — verify against HEAD's tree. + const untrackedSet = new Set(baselineWip.untracked); + const candidates = wipOnly.filter(f => !untrackedSet.has(f)); + const inHead = candidates.length > 0 ? new Set(await git.ls.tree(tmpDir, "HEAD", candidates)) : new Set(); + const toRestore = candidates.filter(f => inHead.has(f)); + const toRemove = wipOnly.filter(f => !toRestore.includes(f)); + if (toRestore.length > 0) { + await git.restore(tmpDir, { source: "HEAD", staged: true, worktree: true, files: toRestore }); + } + for (const rel of toRemove) { + await fs.rm(path.join(tmpDir, rel), { force: true }); + } +} + interface FilteredAgentReplayOptions { baseline: WorktreeBaseline; branchName: string; @@ -542,6 +649,7 @@ async function replayFilteredAgentCommits(opts: FilteredAgentReplayOptions): Pro opts.baseline.root.untrackedPatch, ]); let previousFilteredTree = baselineSha; + let filteredCommitsApplied = 0; for (const commitSha of agentCommits) { const taskStatePatch = await git.diff.tree(opts.isolationDir, dirtyBaselineTree, `${commitSha}^{tree}`, { @@ -562,18 +670,37 @@ async function replayFilteredAgentCommits(opts: FilteredAgentReplayOptions): Pro details.message || commitSha, details.author, ); + filteredCommitsApplied++; } previousFilteredTree = currentFilteredTree; } - - const finalFilteredTree = await writeSyntheticTree(opts.repoRoot, baselineSha, [opts.rootPatch]); - const leftoverPatch = await git.diff.tree(opts.repoRoot, previousFilteredTree, finalFilteredTree, { - allowFailure: true, - binary: true, - }); - if (leftoverPatch.trim()) { - const msg = (opts.commitMessage && (await opts.commitMessage(leftoverPatch))) || opts.fallbackMessage; - await commitPatchToBranchWorktree(tmpDir, opts.taskId, leftoverPatch, msg); + if (filteredCommitsApplied === 0) { + // No filtered commit landed — tmpDir is still pinned at baselineSha. + // The `finalFilteredTree = writeSyntheticTree(HEAD, [rootPatch])` + // path here fails hard whenever rootPatch's WIP-context can't be + // applied to a HEAD-only index (untracked WIP + agent modifies, + // staged-new WIP + agent modifies — see #4136). Bypass the synthesis + // entirely and collapse the isolation output onto a single commit + // with WIP seed, matching the no-agent-commit path in commitToBranch. + // This also handles the "agent committed only baseline WIP" corner + // case where every filtered patch collapsed to empty. + if (opts.rootPatch.trim()) { + const msg = (opts.commitMessage && (await opts.commitMessage(opts.rootPatch))) || opts.fallbackMessage; + await commitPatchToBranchWorktree(tmpDir, opts.taskId, opts.rootPatch, msg, undefined, opts.baseline.root); + } + } else { + // A filtered commit landed; tmpDir has advanced past baselineSha and + // previousFilteredTree is HEAD-derived, so writeSyntheticTree + + // leftoverPatch stay HEAD-based and no WIP seed is needed. + const finalFilteredTree = await writeSyntheticTree(opts.repoRoot, baselineSha, [opts.rootPatch]); + const leftoverPatch = await git.diff.tree(opts.repoRoot, previousFilteredTree, finalFilteredTree, { + allowFailure: true, + binary: true, + }); + if (leftoverPatch.trim()) { + const msg = (opts.commitMessage && (await opts.commitMessage(leftoverPatch))) || opts.fallbackMessage; + await commitPatchToBranchWorktree(tmpDir, opts.taskId, leftoverPatch, msg); + } } } finally { await git.worktree.tryRemove(opts.repoRoot, tmpDir); @@ -674,7 +801,8 @@ export async function commitToBranch( await git.worktree.add(repoRoot, tmpDir, branchName); const msg = (commitMessage && (await commitMessage(rootPatch))) || fallbackMessage; - await commitPatchToBranchWorktree(tmpDir, taskId, rootPatch, msg); + const wip = baselineHasRootWip(baseline.root) ? baseline.root : undefined; + await commitPatchToBranchWorktree(tmpDir, taskId, rootPatch, msg, undefined, wip); } finally { await git.worktree.tryRemove(repoRoot, tmpDir); await fs.rm(tmpDir, { recursive: true, force: true }); diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 91d45df06..d6272509e 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -1919,6 +1919,14 @@ export const ls = { return ls.files(cwd, { others: true, excludeStandard: true, signal }); }, + /** List paths present in a ref, optionally filtered to specific paths. */ + async tree(cwd: string, ref: string, files: readonly string[] = [], signal?: AbortSignal): Promise { + const args = ["ls-tree", "--name-only", "-r", "-z", ref]; + if (files.length > 0) args.push("--", ...files); + const raw = await runText(cwd, args, { readOnly: true, signal }); + return raw.split("\0").filter(entry => entry.length > 0); + }, + /** List submodule paths (recursive). */ async submodules(cwd: string, signal?: AbortSignal): Promise { const output = await git(cwd, ["submodule", "--quiet", "foreach", "--recursive", "echo $sm_path"], { diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 216c6b401..0d6f32f98 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -744,4 +744,168 @@ describe("commitToBranch preserves agent commits", () => { const result = await commitToBranch(isolation, baseline, "empty", undefined); expect(result).toBeNull(); }); + + // Regression: #4136. Baseline WIP made captureRepoDeltaPatch record hunks + // against `HEAD + WIP`; commitPatchToBranchWorktree then failed to apply + // those hunks to a fresh worktree pinned at HEAD when the WIP-side file was + // missing from HEAD's index (untracked WIP files, staged-new WIP files) or + // when --3way couldn't resolve the overlap. Each scenario below reproduced + // the failure before the fix. + describe("with baseline WIP overlapping the agent's changes (#4136)", () => { + async function seedWipFileFromParent(destRoot: string, relPath: string): Promise { + await fs.mkdir(path.join(destRoot, path.dirname(relPath)), { recursive: true }); + await fs.copyFile(path.join(parent, relPath), path.join(destRoot, relPath)); + } + + it("commits an agent-only delta via --3way when WIP and agent modify unrelated hunks of the same tracked file", async () => { + const fixture = "src/foo.py"; + const head = Array.from({ length: 40 }, (_, i) => `# line ${i + 1}\n`).join(""); + await fs.mkdir(path.join(parent, "src"), { recursive: true }); + await fs.writeFile(path.join(parent, fixture), head); + await gitr(parent, ["add", "."]); + await gitr(parent, ["commit", "-q", "-m", "add fixture"]); + + // Isolation must be re-cloned so the fixture is present in HEAD. + await fs.rm(isolation, { recursive: true, force: true }); + await gitr(parent, ["clone", "-q", "--no-hardlinks", "--local", parent, isolation]); + await gitr(isolation, ["config", "user.email", "agent@example.com"]); + await gitr(isolation, ["config", "user.name", "Agent User"]); + + // Parent WIP: change line 10 (unstaged edit to an existing tracked file). + const wipLines = head.split("\n"); + wipLines[9] = "# line 10 thinkingLevel: medium"; + await fs.writeFile(path.join(parent, fixture), wipLines.join("\n")); + await seedWipFileFromParent(isolation, fixture); + + // Agent modifies line 30, far from the WIP change. + const agentLines = wipLines.slice(); + agentLines[29] = "# line 30 def new_func()"; + await fs.writeFile(path.join(isolation, fixture), agentLines.join("\n")); + + const baseline = await captureBaseline(parent); + const result = await commitToBranch(isolation, baseline, "wip-tracked-file", undefined); + expect(result?.branchName).toBe("omp/task/wip-tracked-file"); + + const branchDiff = await gitr(parent, ["show", "--pretty=format:", result!.branchName!]); + expect(branchDiff).toContain("+# line 30 def new_func()"); + // --3way must subtract the WIP change from the commit; only the + // agent's line 30 edit belongs on the task branch. + expect(branchDiff).not.toContain("thinkingLevel: medium"); + }); + + it("commits an untracked WIP file that the agent modifies inside isolation", async () => { + // Parent WIP: add a new untracked file the agent also touches. + await fs.mkdir(path.join(parent, "src"), { recursive: true }); + await fs.writeFile(path.join(parent, "src/new.py"), "WIP header\nunchanged\n"); + await fs.mkdir(path.join(isolation, "src"), { recursive: true }); + await fs.copyFile(path.join(parent, "src/new.py"), path.join(isolation, "src/new.py")); + + // Agent extends the file inside isolation. + await fs.writeFile(path.join(isolation, "src/new.py"), "WIP header\nagent-edit\n"); + + const baseline = await captureBaseline(parent); + expect(baseline.root.untracked).toContain("src/new.py"); + const result = await commitToBranch(isolation, baseline, "wip-untracked", undefined); + expect(result?.branchName).toBe("omp/task/wip-untracked"); + + const branchDiff = await gitr(parent, ["show", "--pretty=format:", result!.branchName!]); + expect(branchDiff).toContain("new file mode"); + expect(branchDiff).toContain("src/new.py"); + expect(branchDiff).toContain("+WIP header"); + expect(branchDiff).toContain("+agent-edit"); + }); + + it("commits a staged-new WIP file that the agent modifies inside isolation", async () => { + // Parent WIP: stage a new file that isn't yet in HEAD. + await fs.writeFile(path.join(parent, "notes.md"), "l1\nl2\nl3\n"); + await gitr(parent, ["add", "notes.md"]); + await fs.copyFile(path.join(parent, "notes.md"), path.join(isolation, "notes.md")); + await gitr(isolation, ["add", "notes.md"]); + + // Agent edits the staged-new file. + await fs.writeFile(path.join(isolation, "notes.md"), "l1\nl2 agent\nl3\n"); + + const baseline = await captureBaseline(parent); + expect(baseline.root.staged).toContain("new file mode"); + const result = await commitToBranch(isolation, baseline, "wip-staged-new", undefined); + expect(result?.branchName).toBe("omp/task/wip-staged-new"); + + const branchDiff = await gitr(parent, ["show", "--pretty=format:", result!.branchName!]); + expect(branchDiff).toContain("new file mode"); + expect(branchDiff).toContain("notes.md"); + expect(branchDiff).toContain("+l2 agent"); + }); + + it("does not leak WIP-only files into the branch commit when the agent leaves them untouched", async () => { + // Parent WIP: touch two files. The agent only edits `wanted.py`; + // `wip-only.py` must not appear in the branch commit at all. + await fs.mkdir(path.join(parent, "src"), { recursive: true }); + await fs.writeFile(path.join(parent, "src/wanted.py"), "unchanged\n"); + await fs.writeFile(path.join(parent, "src/wip-only.py"), "unchanged\n"); + await gitr(parent, ["add", "."]); + await gitr(parent, ["commit", "-q", "-m", "seed"]); + await fs.rm(isolation, { recursive: true, force: true }); + await gitr(parent, ["clone", "-q", "--no-hardlinks", "--local", parent, isolation]); + await gitr(isolation, ["config", "user.email", "agent@example.com"]); + await gitr(isolation, ["config", "user.name", "Agent User"]); + + await fs.writeFile(path.join(parent, "src/wip-only.py"), "wip edit\n"); + await fs.writeFile(path.join(parent, "src/wanted.py"), "wip mixed\n"); + await fs.copyFile(path.join(parent, "src/wip-only.py"), path.join(isolation, "src/wip-only.py")); + await fs.copyFile(path.join(parent, "src/wanted.py"), path.join(isolation, "src/wanted.py")); + // Untracked WIP file the agent also does not touch. + await fs.writeFile(path.join(parent, "user-wip.txt"), "user wip\n"); + await fs.copyFile(path.join(parent, "user-wip.txt"), path.join(isolation, "user-wip.txt")); + + // Agent only extends `wanted.py`; leaves `wip-only.py` and + // `user-wip.txt` alone. + await fs.writeFile(path.join(isolation, "src/wanted.py"), "wip mixed\nagent line\n"); + + const baseline = await captureBaseline(parent); + const result = await commitToBranch(isolation, baseline, "wip-filter", undefined); + expect(result?.branchName).toBe("omp/task/wip-filter"); + + const files = (await gitr(parent, ["show", "--name-only", "--pretty=format:", result!.branchName!])) + .split("\n") + .filter(Boolean); + // Only the agent-touched file lands on the branch — no WIP-only files. + expect(files).toEqual(["src/wanted.py"]); + }); + + it("still seeds WIP when the agent commits only baseline WIP and leaves the real edit uncommitted", async () => { + // Regression for the review on #4140: `agentCommits.length` alone + // hid this case — the agent had commits, but every filtered patch + // collapsed to empty (they only replayed baseline WIP via `git + // add -A`), so tmpDir was still pinned at baselineSha and the + // leftover patch still carried HEAD+WIP context. + await fs.mkdir(path.join(parent, "src"), { recursive: true }); + // Parent WIP: untracked file the agent will also modify. The + // no-tracked-in-HEAD trigger from #4136 is the sharpest way to + // prove the WIP-seeded fallback fires. + await fs.writeFile(path.join(parent, "src/new.py"), "WIP header\nunchanged\n"); + + // Agent replays baseline WIP as a commit (`git add -A`), then makes + // the real edit uncommitted. `agentCommits.length` = 1, but the + // filtered commit patch is empty because it's identical to the + // baseline dirty tree. + await fs.mkdir(path.join(isolation, "src"), { recursive: true }); + await fs.copyFile(path.join(parent, "src/new.py"), path.join(isolation, "src/new.py")); + await gitr(isolation, ["add", "-A"]); + await gitr(isolation, ["commit", "-q", "-m", "chore: capture baseline"]); + + // Real, uncommitted agent edit on top of the WIP file. + await fs.writeFile(path.join(isolation, "src/new.py"), "WIP header\nagent-edit\n"); + + const baseline = await captureBaseline(parent); + expect(baseline.root.untracked).toContain("src/new.py"); + const result = await commitToBranch(isolation, baseline, "wip-only-commit", undefined); + expect(result?.branchName).toBe("omp/task/wip-only-commit"); + + const branchDiff = await gitr(parent, ["show", "--pretty=format:", result!.branchName!]); + expect(branchDiff).toContain("new file mode"); + expect(branchDiff).toContain("src/new.py"); + expect(branchDiff).toContain("+WIP header"); + expect(branchDiff).toContain("+agent-edit"); + }); + }); });