diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c68409f79..a449971bb 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Isolated branch-mode task merges now preserve the agent's own commits (message and author) instead of collapsing every diff into a single AI-summarized commit. `commitToBranch` detects when the subagent moved HEAD past the baseline, transfers the new commit objects into the parent repo via `git fetch`, and `mergeTaskBranches` cherry-picks the inclusive range `baseSha..omp/task/` so each commit replays verbatim; any uncommitted leftover on top of the agent's last commit lands as one trailing AI-summarized commit ([#3842](https://github.com/can1357/oh-my-pi/issues/3842)). + ## [16.2.6] - 2026-06-29 ### Changed diff --git a/packages/coding-agent/src/task/isolation-runner.ts b/packages/coding-agent/src/task/isolation-runner.ts index 455bba052..3a7b9ab68 100644 --- a/packages/coding-agent/src/task/isolation-runner.ts +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -154,6 +154,7 @@ export async function runIsolatedSubprocess(opts: IsolatedRunOptions): Promise { export interface CommitToBranchResult { branchName?: string; nestedPatches: NestedRepoPatch[]; + /** + * SHA of the parent-repo commit the task branch was created on top of, so + * {@link mergeTaskBranches} can cherry-pick the range `baseSha..branchName` + * and preserve every agent commit's message and author. + */ + baseSha?: string; } /** - * Commit task-only changes to a new branch. - * Only root repo changes go on the branch. Nested repo patches are returned - * separately since the parent git can't track files inside gitlinks. + * Capture task-only changes from the isolation worktree onto a parent-repo + * branch named `omp/task/${taskId}`. Only root-repo changes go on the branch; + * nested-repo patches are returned separately because the parent git can't + * track files inside gitlinks. + * + * If the agent committed its own changes inside isolation (HEAD moved past + * `baseline.root.headCommit`), this transfers those commit objects into the + * parent repo via `git fetch` and points the branch at the agent's HEAD — + * later cherry-pick of `baseSha..branchName` replays every commit with its + * original message and author preserved. Any uncommitted leftover (staged, + * unstaged, untracked) on top of the agent's last commit becomes one + * additional commit with an AI-generated message. + * + * If the agent did not commit, the captured delta is collapsed onto a single + * branch commit with an AI-generated (or fallback) message — the legacy + * behaviour. + * + * Returns `null` when no root or nested changes exist. */ export async function commitToBranch( isolationDir: string, @@ -474,6 +495,10 @@ export async function commitToBranch( description: string | undefined, commitMessage?: (diff: string) => Promise, ): Promise { + const baselineSha = baseline.root.headCommit; + const isolationHead = (await git.head.sha(isolationDir)) ?? ""; + const agentCommitted = isolationHead !== "" && isolationHead !== baselineSha; + const { rootPatch, nestedPatches } = await captureDeltaPatch(isolationDir, baseline); if (!rootPatch.trim() && nestedPatches.length === 0) return null; @@ -481,14 +506,40 @@ export async function commitToBranch( const branchName = `omp/task/${taskId}`; const fallbackMessage = description || taskId; - // Only create a branch if the root repo has changes - if (rootPatch.trim()) { + let branchCreated = false; + let leftoverPatch = ""; + + if (agentCommitted) { + // Transfer the agent's commit objects (which live in isolation's `.git`, + // stranded once `cleanupIsolation` tears the overlay down) into the parent + // repo's object DB and create the branch at the agent's HEAD. `+HEAD:…` + // force-overwrites a stale branch from a prior run. + await git.fetch(repoRoot, isolationDir, "HEAD", `refs/heads/${branchName}`); + branchCreated = true; + + // Leftover = anything still uncommitted in isolation on top of the + // agent's last commit (staged, unstaged, untracked). The agent didn't + // commit it, so it goes in as one AI-summarized trailing commit. + leftoverPatch = await captureRepoDeltaPatch(isolationDir, { + repoRoot: isolationDir, + headCommit: isolationHead, + staged: "", + unstaged: "", + untracked: [], + untrackedPatch: "", + }); + } else if (rootPatch.trim()) { await git.branch.create(repoRoot, branchName); + branchCreated = true; + leftoverPatch = rootPatch; + } + + if (branchCreated && leftoverPatch.trim()) { const tmpDir = path.join(os.tmpdir(), `omp-branch-${Snowflake.next()}`); try { await git.worktree.add(repoRoot, tmpDir, branchName); try { - await git.patch.applyText(tmpDir, rootPatch); + await git.patch.applyText(tmpDir, leftoverPatch); } catch (err) { if (err instanceof git.GitCommandError) { const stderr = err.result.stderr.slice(0, 2000); @@ -496,15 +547,15 @@ export async function commitToBranch( taskId, exitCode: err.result.exitCode, stderr, - patchSize: rootPatch.length, - patchHead: rootPatch.slice(0, 500), + patchSize: leftoverPatch.length, + patchHead: leftoverPatch.slice(0, 500), }); throw new Error(`git apply failed for task ${taskId}: ${stderr}`); } throw err; } await git.stage.files(tmpDir); - const msg = (commitMessage && (await commitMessage(rootPatch))) || fallbackMessage; + const msg = (commitMessage && (await commitMessage(leftoverPatch))) || fallbackMessage; await git.commit(tmpDir, msg); } finally { await git.worktree.tryRemove(repoRoot, tmpDir); @@ -512,7 +563,11 @@ export async function commitToBranch( } } - return { branchName: rootPatch.trim() ? branchName : undefined, nestedPatches }; + return { + branchName: branchCreated ? branchName : undefined, + baseSha: baselineSha, + nestedPatches, + }; } export interface MergeBranchResult { @@ -524,13 +579,17 @@ export interface MergeBranchResult { } /** - * Cherry-pick task branch commits sequentially onto HEAD. - * Each branch has a single commit that gets replayed cleanly. - * Stops on first conflict and reports which branches succeeded. + * Cherry-pick task branch commits sequentially onto HEAD. When `baseSha` is + * provided the cherry-pick uses the inclusive range `baseSha..branchName`, + * replaying every commit individually and preserving each commit's message + * and author. When omitted, the branch is cherry-picked as a single commit + * (legacy callers). + * + * Stops on the first conflict and reports which branches succeeded. */ export async function mergeTaskBranches( repoRoot: string, - branches: Array<{ branchName: string; taskId: string; description?: string }>, + branches: Array<{ branchName: string; taskId: string; description?: string; baseSha?: string }>, ): Promise { // Serialize against other in-process git mutations on this repo: concurrent // background merges interleaving stash push/pop + cherry-pick would corrupt @@ -546,9 +605,10 @@ export async function mergeTaskBranches( let conflictResult: MergeBranchResult | undefined; try { - for (const { branchName } of branches) { + for (const { branchName, baseSha } of branches) { try { - await git.cherryPick(repoRoot, branchName); + const target = baseSha ? `${baseSha}..${branchName}` : branchName; + await git.cherryPick(repoRoot, target); } catch (err) { try { await git.cherryPick.abort(repoRoot); diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 3dcdf6d1b..17d18dfae 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -6,6 +6,7 @@ import { applyNestedPatches, captureBaseline, captureDeltaPatch, + commitToBranch, ensureIsolation, getGitNoIndexNullPath, getRepoRoot, @@ -426,3 +427,137 @@ describe("applyNestedPatches", () => { expect(stashList).toContain("omp-isolation-"); }); }); + +describe("commitToBranch preserves agent commits", () => { + let parent: string; + let isolation: string; + + async function gitr(repo: string, args: string[]): Promise { + return runGit(repo, args); + } + + beforeEach(async () => { + parent = await fs.mkdtemp(path.join(os.tmpdir(), "omp-commit-parent-")); + isolation = await fs.mkdtemp(path.join(os.tmpdir(), "omp-commit-iso-")); + await gitr(parent, ["init", "-q", "-b", "main"]); + await gitr(parent, ["config", "user.email", "user@example.com"]); + await gitr(parent, ["config", "user.name", "Parent User"]); + await fs.writeFile( + path.join(parent, "EXP_CLEAN_COMMIT.txt"), + "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n", + ); + await gitr(parent, ["add", "."]); + await gitr(parent, ["commit", "-q", "-m", "add clean test fixture"]); + + // Simulate copy-on-write isolation: a real local clone so the agent's + // commit objects live in `isolation/.git`, just like the overlay/rcopy + // isolation backends would arrange them at runtime. + 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"]); + }); + + afterEach(async () => { + await Promise.all([removeWithRetries(parent), removeWithRetries(isolation)]); + }); + + // Reproduces issue #3842: agent commits with a specific message inside + // isolation; the merged commit on the parent branch must keep that exact + // message instead of an AI-generated summary. + it("preserves the agent's commit message after merge", async () => { + const baseline = await captureBaseline(parent); + + await fs.writeFile( + path.join(isolation, "EXP_CLEAN_COMMIT.txt"), + "line1\nline2\nline3\nline4\nLINE5-AGENT-WITH-MESSAGE\nline6\nline7\nline8\nline9\nline10\n", + ); + await gitr(isolation, ["add", "EXP_CLEAN_COMMIT.txt"]); + const agentMessage = "fix(test): agent committed with specific message for preservation check"; + await gitr(isolation, ["commit", "-q", "-m", agentMessage]); + + const taskId = "preservation-check"; + const aiMessage = vi.fn(async () => "fix: update line5 in clean commit example"); + const result = await commitToBranch(isolation, baseline, taskId, undefined, aiMessage); + + expect(result?.branchName).toBe(`omp/task/${taskId}`); + expect(result?.baseSha).toBe(baseline.root.headCommit); + // commitMessage callback must NOT have been invoked — the agent's + // message is taken verbatim. + expect(aiMessage).not.toHaveBeenCalled(); + + const branchSubject = await gitr(parent, ["log", "-1", "--pretty=%s", result!.branchName!]); + expect(branchSubject).toBe(agentMessage); + + const merge = await mergeTaskBranches(parent, [ + { branchName: result!.branchName!, taskId, baseSha: result!.baseSha! }, + ]); + expect(merge.failed).toEqual([]); + expect(merge.merged).toEqual([result!.branchName!]); + + const headSubject = await gitr(parent, ["log", "-1", "--pretty=%s"]); + expect(headSubject).toBe(agentMessage); + }); + + it("preserves every message when the agent makes multiple commits", async () => { + const baseline = await captureBaseline(parent); + + await fs.writeFile(path.join(isolation, "a.txt"), "alpha\n"); + await gitr(isolation, ["add", "a.txt"]); + await gitr(isolation, ["commit", "-q", "-m", "feat: add alpha file"]); + await fs.writeFile(path.join(isolation, "b.txt"), "beta\n"); + await gitr(isolation, ["add", "b.txt"]); + await gitr(isolation, ["commit", "-q", "-m", "test: add beta coverage"]); + + const result = await commitToBranch(isolation, baseline, "multi", undefined); + expect(result?.branchName).toBe("omp/task/multi"); + + const merge = await mergeTaskBranches(parent, [ + { branchName: result!.branchName!, taskId: "multi", baseSha: result!.baseSha! }, + ]); + expect(merge).toEqual({ failed: [], merged: ["omp/task/multi"] }); + + const subjects = (await gitr(parent, ["log", "-2", "--pretty=%s"])).split("\n"); + expect(subjects).toEqual(["test: add beta coverage", "feat: add alpha file"]); + }); + + it("appends one trailing commit when the agent leaves uncommitted work after committing", async () => { + const baseline = await captureBaseline(parent); + + await fs.writeFile(path.join(isolation, "a.txt"), "alpha\n"); + await gitr(isolation, ["add", "a.txt"]); + await gitr(isolation, ["commit", "-q", "-m", "feat: add alpha file"]); + // Uncommitted change on top of the agent's commit — should land as one + // extra commit with the AI-generated message, NOT silently dropped. + await fs.writeFile(path.join(isolation, "b.txt"), "beta\n"); + + const aiMessage = vi.fn(async () => "chore: leftover beta wip"); + const result = await commitToBranch(isolation, baseline, "leftover", undefined, aiMessage); + expect(result?.branchName).toBe("omp/task/leftover"); + expect(aiMessage).toHaveBeenCalledTimes(1); + + const subjects = (await gitr(parent, ["log", "-2", "--pretty=%s", result!.branchName!])).split("\n"); + expect(subjects).toEqual(["chore: leftover beta wip", "feat: add alpha file"]); + }); + + it("falls back to the AI-generated message when the agent never committed", async () => { + const baseline = await captureBaseline(parent); + + await fs.writeFile(path.join(isolation, "a.txt"), "alpha\n"); + + const aiMessage = vi.fn(async () => "feat: add alpha"); + const result = await commitToBranch(isolation, baseline, "nocommit", undefined, aiMessage); + + expect(result?.branchName).toBe("omp/task/nocommit"); + expect(aiMessage).toHaveBeenCalledTimes(1); + + const branchSubject = await gitr(parent, ["log", "-1", "--pretty=%s", result!.branchName!]); + expect(branchSubject).toBe("feat: add alpha"); + }); + + it("returns null when nothing changed in isolation", async () => { + const baseline = await captureBaseline(parent); + const result = await commitToBranch(isolation, baseline, "empty", undefined); + expect(result).toBeNull(); + }); +});