diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 00100fcfd..66d6aa29a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -27,6 +27,9 @@ ### Fixed - Fixed `/extensions` showing MCP servers as `active` when `/mcp list` reported them as `disabled`, and made `/extensions` re-enable work for every supported source. The dashboard now mirrors `/mcp list` by honoring the per-server `enabled: false` flag plus the user-level `disabledServers` denylist and a new `enabledServers` allowlist; the dashboard's MCP toggle writes through the canonical mcp.json — flipping `enabled` on the loaded source file for config-resident servers (including supported non-primary files such as `.omp/.mcp.json`), force-enabling tool-owned sources (such as `opencode.json`) via the user `enabledServers` allowlist without mutating the foreign config, and using the `disabledServers` denylist for purely discovered third-party servers, so `/mcp list`, the MCP runtime, and the dashboard stay in sync ([#3827](https://github.com/can1357/oh-my-pi/issues/3827)). +### 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 clean-baseline commit objects into the parent repo via `git fetch`, rewrites dirty-baseline commits against the captured baseline WIP so user staged/unstaged/untracked changes are filtered out, and `mergeTaskBranches` cherry-picks the inclusive range `baseSha..omp/task/` so each task commit replays with its original message; 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 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; +} + +function baselineHasRootWip(baseline: RepoBaseline): boolean { + return !!(baseline.staged.trim() || baseline.unstaged.trim() || baseline.untrackedPatch.trim()); +} + +async function commitPatchToBranchWorktree( + tmpDir: string, + taskId: string, + patchText: string, + message: string, + author?: git.CommitAuthor, +): Promise { + try { + await git.patch.applyText(tmpDir, patchText); + } catch (err) { + if (err instanceof git.GitCommandError) { + const stderr = err.result.stderr.slice(0, 2000); + logger.error("commitToBranch: git apply failed", { + taskId, + exitCode: err.result.exitCode, + stderr, + patchSize: patchText.length, + patchHead: patchText.slice(0, 500), + }); + throw new Error(`git apply failed for task ${taskId}: ${stderr}`); + } + throw err; + } + await git.stage.files(tmpDir); + await git.commit(tmpDir, message, author ? { author } : {}); +} + +interface FilteredAgentReplayOptions { + baseline: WorktreeBaseline; + branchName: string; + commitMessage?: (diff: string) => Promise; + fallbackMessage: string; + isolationDir: string; + isolationHead: string; + repoRoot: string; + rootPatch: string; + taskId: string; +} + +async function replayFilteredAgentCommits(opts: FilteredAgentReplayOptions): Promise { + const baselineSha = opts.baseline.root.headCommit; + await git.branch.create(opts.repoRoot, opts.branchName, baselineSha); + + const tmpDir = path.join(os.tmpdir(), `omp-branch-${Snowflake.next()}`); + try { + await git.worktree.add(opts.repoRoot, tmpDir, opts.branchName); + const agentCommits = await git.revList.range(opts.isolationDir, baselineSha, opts.isolationHead); + const dirtyBaselineTree = await writeSyntheticTree(opts.isolationDir, baselineSha, [ + opts.baseline.root.staged, + opts.baseline.root.unstaged, + opts.baseline.root.untrackedPatch, + ]); + let previousFilteredTree = baselineSha; + + for (const commitSha of agentCommits) { + const taskStatePatch = await git.diff.tree(opts.isolationDir, dirtyBaselineTree, `${commitSha}^{tree}`, { + allowFailure: true, + binary: true, + }); + const currentFilteredTree = await writeSyntheticTree(opts.repoRoot, baselineSha, [taskStatePatch]); + const commitPatch = await git.diff.tree(opts.repoRoot, previousFilteredTree, currentFilteredTree, { + allowFailure: true, + binary: true, + }); + if (commitPatch.trim()) { + const details = await git.commitDetails(opts.isolationDir, commitSha); + await commitPatchToBranchWorktree( + tmpDir, + opts.taskId, + commitPatch, + details.message || commitSha, + details.author, + ); + } + 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); + } + } finally { + await git.worktree.tryRemove(opts.repoRoot, tmpDir); + await fs.rm(tmpDir, { recursive: true, force: true }); + } } /** - * 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 inside isolation (HEAD moved past + * `baseline.root.headCommit`), clean-baseline runs fetch the raw commit range + * into the parent repo and later cherry-pick `baseSha..branchName`, preserving + * every message and author verbatim. Dirty-baseline runs rewrite each agent + * commit against the captured baseline WIP before committing it to the task + * branch, so user staged/unstaged/untracked changes present at isolation + * start are not replayed into the parent commit history. + * + * 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,45 +591,83 @@ 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; + if (!rootPatch.trim()) return { nestedPatches }; const repoRoot = baseline.root.repoRoot; const branchName = `omp/task/${taskId}`; const fallbackMessage = description || taskId; - // Only create a branch if the root repo has changes - if (rootPatch.trim()) { - await git.branch.create(repoRoot, branchName); + let branchCreated = false; + + if (agentCommitted) { + if (baselineHasRootWip(baseline.root)) { + await replayFilteredAgentCommits({ + baseline, + branchName, + commitMessage, + fallbackMessage, + isolationDir, + isolationHead, + repoRoot, + rootPatch, + taskId, + }); + } else { + // 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}`); + + // 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. + const leftoverPatch = await captureRepoDeltaPatch(isolationDir, { + repoRoot: isolationDir, + headCommit: isolationHead, + staged: "", + unstaged: "", + untracked: [], + untrackedPatch: "", + }); + if (leftoverPatch.trim()) { + const tmpDir = path.join(os.tmpdir(), `omp-branch-${Snowflake.next()}`); + try { + await git.worktree.add(repoRoot, tmpDir, branchName); + const msg = (commitMessage && (await commitMessage(leftoverPatch))) || fallbackMessage; + await commitPatchToBranchWorktree(tmpDir, taskId, leftoverPatch, msg); + } finally { + await git.worktree.tryRemove(repoRoot, tmpDir); + await fs.rm(tmpDir, { recursive: true, force: true }); + } + } + } + branchCreated = true; + } else if (rootPatch.trim()) { + await git.branch.create(repoRoot, branchName, baselineSha); + branchCreated = true; 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); - } catch (err) { - if (err instanceof git.GitCommandError) { - const stderr = err.result.stderr.slice(0, 2000); - logger.error("commitToBranch: git apply failed", { - taskId, - exitCode: err.result.exitCode, - stderr, - patchSize: rootPatch.length, - patchHead: rootPatch.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; - await git.commit(tmpDir, msg); + await commitPatchToBranchWorktree(tmpDir, taskId, rootPatch, msg); } finally { await git.worktree.tryRemove(repoRoot, tmpDir); await fs.rm(tmpDir, { recursive: true, force: true }); } } - return { branchName: rootPatch.trim() ? branchName : undefined, nestedPatches }; + return { + branchName: branchCreated ? branchName : undefined, + baseSha: baselineSha, + nestedPatches, + }; } export interface MergeBranchResult { @@ -524,13 +679,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 +705,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/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 86afc8bc0..c8de9e67c 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -74,8 +74,20 @@ export interface StatusOptions { readonly z?: boolean; } +export interface CommitAuthor { + readonly date?: string; + readonly email: string; + readonly name: string; +} + +export interface CommitDetails { + readonly author: CommitAuthor; + readonly message: string; +} + export interface CommitOptions { readonly allowEmpty?: boolean; + readonly author?: CommitAuthor; readonly files?: readonly string[]; readonly signal?: AbortSignal; } @@ -1122,6 +1134,10 @@ export const stage = { /** Create a commit with the given message (passed via stdin). */ export async function commit(cwd: string, message: string, options: CommitOptions = {}): Promise { const args = ["commit", "-F", "-"]; + if (options.author) { + args.push(`--author=${options.author.name} <${options.author.email}>`); + if (options.author.date) args.push(`--date=${options.author.date}`); + } if (options.allowEmpty) args.push("--allow-empty"); if (options.files?.length) args.push("--", ...options.files); return runChecked(cwd, args, { signal: options.signal, stdin: message }); @@ -1195,6 +1211,19 @@ export const show = Object.assign( }, ); +/** Read commit message and author metadata for replay/rewrite flows. */ +export async function commitDetails(cwd: string, revision: string, signal?: AbortSignal): Promise { + const raw = await runText(cwd, ["show", "-s", "--format=%an%x00%ae%x00%aI%x00%B", revision], { + readOnly: true, + signal, + }); + const [name = "", email = "", date = "", ...messageParts] = raw.split("\0"); + return { + author: { date, email, name }, + message: messageParts.join("\0").replace(/\n$/, ""), + }; +} + // ════════════════════════════════════════════════════════════════════════════ // API: log // ════════════════════════════════════════════════════════════════════════════ @@ -1212,6 +1241,13 @@ export const log = { }, }; +export const revList = { + /** Commits in `base..head`, oldest first. */ + async range(cwd: string, base: string, head: string, signal?: AbortSignal): Promise { + return splitLines(await runText(cwd, ["rev-list", "--reverse", `${base}..${head}`], { readOnly: true, signal })); + }, +}; + // ════════════════════════════════════════════════════════════════════════════ // API: branch // ════════════════════════════════════════════════════════════════════════════ diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 3dcdf6d1b..260fafa98 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,179 @@ 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("filters baseline WIP when the agent commits with git add -A", async () => { + await fs.writeFile(path.join(parent, "staged.txt"), "baseline staged wip\n"); + await gitr(parent, ["add", "staged.txt"]); + await fs.writeFile(path.join(parent, "user-wip.txt"), "baseline untracked wip\n"); + await fs.writeFile(path.join(isolation, "staged.txt"), "baseline staged wip\n"); + await gitr(isolation, ["add", "staged.txt"]); + await fs.writeFile(path.join(isolation, "user-wip.txt"), "baseline untracked wip\n"); + 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", "-A"]); + const agentMessage = "fix(test): preserve message without baseline wip"; + await gitr(isolation, ["commit", "-q", "-m", agentMessage]); + + const aiMessage = vi.fn(async () => "fix: generated fallback"); + const result = await commitToBranch(isolation, baseline, "dirty-baseline", undefined, aiMessage); + expect(result?.branchName).toBe("omp/task/dirty-baseline"); + expect(aiMessage).not.toHaveBeenCalled(); + + const branchFiles = (await gitr(parent, ["show", "--name-only", "--pretty=format:", result!.branchName!])) + .split("\n") + .filter(Boolean); + expect(branchFiles).toEqual(["EXP_CLEAN_COMMIT.txt"]); + + const merge = await mergeTaskBranches(parent, [ + { branchName: result!.branchName!, taskId: "dirty-baseline", baseSha: result!.baseSha! }, + ]); + expect(merge).toEqual({ failed: [], merged: ["omp/task/dirty-baseline"] }); + + const [headSubject, status, fixture] = await Promise.all([ + gitr(parent, ["log", "-1", "--pretty=%s"]), + gitr(parent, ["status", "--porcelain=v1"]), + fs.readFile(path.join(parent, "EXP_CLEAN_COMMIT.txt"), "utf8"), + ]); + expect(headSubject).toBe(agentMessage); + expect(status.split("\n").sort()).toEqual(["?? user-wip.txt", "A staged.txt"]); + expect(fixture).toContain("LINE5-AGENT-WITH-MESSAGE"); + }); + + 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(); + }); +});