smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

merge PR #4140: fix(coding-agent): stopped isolated task merges failing when working tree carries WIP for files the agent also modifies (@roboomp)

Browse Source
This commit is contained in:
can1357
2026-07-02 10:30:06 +02:00
parent ebf41281e7 87e1b03db1
commit a23d1d6554
4 changed files with 319 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -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
packages/coding-agent/src/task/worktree.ts
+144 -16
View File
@@ -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<void> {
// 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<void> {
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<string>();
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 });
packages/coding-agent/src/utils/git.ts
+8
View File
@@ -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<string[]> {
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<string[]> {
const output = await git(cwd, ["submodule", "--quiet", "foreach", "--recursive", "echo $sm_path"], {
packages/coding-agent/test/task/worktree.test.ts
+164
View File
@@ -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<void> {
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");
});
});
});