diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 860266c53..11fd94dfe 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed task-branch merges aborting the whole cherry-pick range when a single commit was empty against HEAD (redundant with an already-applied change, or 3-way merged to HEAD); the sequencer now `--skip`s the empty commit and continues so later non-overlapping work still lands ([#4438](https://github.com/can1357/oh-my-pi/issues/4438)). + ## [16.3.4] - 2026-07-03 ### Fixed diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index 4f62313d9..86cdf7314 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -855,18 +855,40 @@ export async function mergeTaskBranches( try { const target = baseSha ? `${baseSha}..${branchName}` : branchName; await git.cherryPick(repoRoot, target); - } catch (err) { + } catch (initialErr) { + // Empty cherry-picks are not conflicts: a commit whose net + // effect is already on HEAD (redundant change, or 3-way + // merge auto-resolved to HEAD) leaves the sequencer stopped + // with a "The previous cherry-pick is now empty" message. + // Advance past every consecutive empty with `--skip` so the + // remaining non-redundant commits in the range still land. + // A genuine conflict (unmerged files, no "now empty" + // message) falls through to the abort path below. + let cursor: unknown = initialErr; + while (git.cherryPick.isEmptyError(cursor)) { + try { + await git.cherryPick.skip(repoRoot); + cursor = undefined; + break; + } catch (skipErr) { + cursor = skipErr; + } + } + if (cursor === undefined) { + merged.push(branchName); + continue; + } try { await git.cherryPick.abort(repoRoot); } catch { /* no state to abort */ } const stderr = - err instanceof git.GitCommandError - ? err.result.stderr.trim() - : err instanceof Error - ? err.message - : String(err); + cursor instanceof git.GitCommandError + ? cursor.result.stderr.trim() + : cursor instanceof Error + ? cursor.message + : String(cursor); failed.push(branchName); conflictResult = { merged, diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index d6272509e..0425140af 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -1726,6 +1726,29 @@ export const cherryPick = Object.assign( async abort(cwd: string, signal?: AbortSignal): Promise { await runEffect(cwd, ["cherry-pick", "--abort"], { signal }); }, + /** + * Skip the current commit of an in-progress cherry-pick sequence and + * continue with the rest of the range. Use after {@link isEmptyError} + * reports the current attempt collapsed to a no-op — the alternative, + * `--abort`, throws away every remaining commit in the range. + */ + async skip(cwd: string, signal?: AbortSignal): Promise { + await runEffect(cwd, ["cherry-pick", "--skip"], { signal }); + }, + /** + * True when a cherry-pick failure was caused by the current commit + * being empty against HEAD — either redundant with an already-applied + * change, or auto-resolved to HEAD by a 3-way merge. Callers should + * `--skip` in this case to advance the sequencer rather than aborting + * the whole range: an empty commit is not a merge conflict, and any + * later commits in the range still deserve to land. + */ + isEmptyError(err: unknown): boolean { + return ( + err instanceof GitCommandError && + /the previous cherry-pick is now empty/i.test(err.result.stderr) + ); + }, }, ); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 0d6f32f98..3ac79ef9d 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -398,6 +398,104 @@ describe("worktree isolation helpers", () => { expect(delta.rootPatch).not.toContain("baseline dirty change"); expect(delta.rootPatch).not.toContain("preexisting.txt"); }); + + // Regression for #4438: cherry-picking a range where an intermediate + // commit becomes empty (redundant with HEAD, or 3-way merged to HEAD) + // used to abort the whole range and mark the branch failed, dropping + // every remaining commit. The fixup here restores merged.txt to the + // content the shared fixture branch already established on HEAD, so + // the sequencer stops with "The previous cherry-pick is now empty". + // The follow-up commit contains real, non-overlapping work that MUST + // still land. + it("auto-skips empty cherry-picks so remaining commits in the range still land", async () => { + const REDUNDANT_BRANCH = "task/redundant-then-real"; + await runGit(repo, ["checkout", "-q", "-b", REDUNDANT_BRANCH, initialSha]); + const branchBase = await runGit(repo, ["rev-parse", "HEAD"]); + // This commit sets merged.txt to the exact content TASK_BRANCH + // establishes on HEAD. Once TASK_BRANCH is cherry-picked, the + // 3-way merge for this commit sees theirs == ours == target, + // resolves to HEAD, and stops the sequencer with the "The + // previous cherry-pick is now empty" message. + await fs.writeFile(path.join(repo, "merged.txt"), "task branch change\n"); + await runGit(repo, ["commit", "-q", "-am", "redundant: same as task branch tip"]); + // A non-overlapping follow-up that MUST land even though the + // preceding commit collapsed to empty. + await fs.writeFile(path.join(repo, "downstream.txt"), "unrelated follow-up\n"); + await runGit(repo, ["add", "downstream.txt"]); + await runGit(repo, ["commit", "-q", "-m", "unrelated follow-up commit"]); + await runGit(repo, ["checkout", "-q", BASE_BRANCH]); + try { + const result = await mergeTaskBranches(repo, [ + { branchName: TASK_BRANCH, taskId: "task-1" }, + { branchName: REDUNDANT_BRANCH, taskId: "task-2", baseSha: branchBase }, + ]); + + const [status, unmerged, mergedContent, downstreamContent, log] = await Promise.all([ + runGit(repo, ["status", "--porcelain=v1"]), + runGit(repo, ["ls-files", "--unmerged"]), + fs.readFile(path.join(repo, "merged.txt"), "utf8"), + fs.readFile(path.join(repo, "downstream.txt"), "utf8"), + runGit(repo, ["log", "--pretty=%s", `${initialSha}..HEAD`]), + ]); + + expect(result).toEqual({ failed: [], merged: [TASK_BRANCH, REDUNDANT_BRANCH] }); + // No cherry-pick sequencer state, no unmerged entries: the + // skip advanced cleanly. + expect(status).toBe(""); + expect(unmerged).toBe(""); + // TASK_BRANCH landed and the empty commit did NOT re-land it + // as a duplicate. + expect(mergedContent).toBe("task branch change\n"); + expect(downstreamContent).toBe("unrelated follow-up\n"); + const subjects = log.split("\n").filter(Boolean); + expect(subjects).toContain("unrelated follow-up commit"); + expect(subjects).toContain("task-change"); + // The redundant commit MUST NOT survive in history as a + // duplicate no-op. + expect(subjects).not.toContain("redundant: same as task branch tip"); + } finally { + await cleanupTaskBranches(repo, [REDUNDANT_BRANCH]); + } + }); + + // A genuine content conflict (unmerged files, no "now empty" hint) + // must NOT be misclassified as empty. The branch stays in `failed` + // and the sequencer aborts cleanly — no lingering cherry-pick state, + // no unmerged index entries. + it("still aborts on genuine cherry-pick conflicts", async () => { + const CONFLICT_BRANCH = "task/genuine-conflict"; + await runGit(repo, ["checkout", "-q", "-b", CONFLICT_BRANCH, initialSha]); + const branchBase = await runGit(repo, ["rev-parse", "HEAD"]); + await fs.writeFile(path.join(repo, "merged.txt"), "incompatible edit\n"); + await runGit(repo, ["commit", "-q", "-am", "incompatible merged.txt edit"]); + await runGit(repo, ["checkout", "-q", BASE_BRANCH]); + try { + const result = await mergeTaskBranches(repo, [ + { branchName: TASK_BRANCH, taskId: "task-1" }, + { branchName: CONFLICT_BRANCH, taskId: "task-2", baseSha: branchBase }, + ]); + + const [status, unmerged, cherryPickHeadExit] = await Promise.all([ + runGit(repo, ["status", "--porcelain=v1"]), + runGit(repo, ["ls-files", "--unmerged"]), + runGit(repo, ["rev-parse", "--verify", "--quiet", "CHERRY_PICK_HEAD"]).then( + () => 0, + () => 1, + ), + ]); + + expect(result.merged).toEqual([TASK_BRANCH]); + expect(result.failed).toEqual([CONFLICT_BRANCH]); + expect(result.conflict).toContain(CONFLICT_BRANCH); + // No stuck sequencer, no unmerged entries: the abort cleaned + // up after itself. + expect(status).toBe(""); + expect(unmerged).toBe(""); + expect(cherryPickHeadExit).toBe(1); + } finally { + await cleanupTaskBranches(repo, [CONFLICT_BRANCH]); + } + }); }); }); });