From afea682b7fb5fb8b3f23aeb6388aeeb3e348cfd4 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 18 Jul 2026 16:48:09 +0000 Subject: [PATCH] fix(task): detach isolated worktree git dir from parent checkout Copy isolation backends (reflink/apfs/btrfs/zfs/block-clone/rcopy) materialise the worktree by duplicating its `.git` verbatim. When the parent is a linked git worktree its `.git` is a pointer file, so the isolation shared the parent's HEAD/index/ref namespace: a task's `git checkout`/`commit` moved the parent's branch, and the rcopy `git worktree add` path stacked task branches in the shared namespace. `ensureIsolation` now runs `git.detachGitDir` after `isoStart`, turning each isolation into a standalone repo with a frozen HEAD/refs/index snapshot that borrows the source object database via `objects/info/alternates`. Isolated git ops stay private, every task branch is parented on the requested base, and patch/branch capture (`git fetch `) still resolves objects. Fixes #6003 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/task/worktree.ts | 10 ++ packages/coding-agent/src/utils/git.ts | 131 ++++++++++++++++++ .../coding-agent/test/task/worktree.test.ts | 128 +++++++++++++++++ 4 files changed, 273 insertions(+) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4522715df..cccc0c989 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed isolated `task` subagents mutating the parent checkout and stacking parallel task branches. Copy isolation backends (reflink/apfs/btrfs/zfs/block-clone/rcopy) materialise the worktree by duplicating its `.git` verbatim; when the parent is a linked git worktree its `.git` is a pointer file, so the isolation shared the parent's HEAD/index/ref namespace and a task's `git checkout`/`commit` moved the parent's branch (and the rcopy `git worktree add` path leaked task branches into the shared namespace so a second task committed on top of the first). `ensureIsolation` now runs a new `git.detachGitDir` after `isoStart`: each isolation becomes a standalone repo with a frozen HEAD/refs/index snapshot that borrows the source object database through `objects/info/alternates`, so isolated git operations stay private, every task branch is parented on the requested base, and patch/branch capture (`git fetch `) still resolves objects. ([#6003](https://github.com/can1357/oh-my-pi/issues/6003)) + ## [17.0.4] - 2026-07-18 ### Fixed diff --git a/packages/coding-agent/src/task/worktree.ts b/packages/coding-agent/src/task/worktree.ts index 86cdf7314..66f37aa2b 100644 --- a/packages/coding-agent/src/task/worktree.ts +++ b/packages/coding-agent/src/task/worktree.ts @@ -414,6 +414,8 @@ export async function ensureIsolation( preferred?: IsoBackendKind, ): Promise { const repoRoot = await getRepoRoot(baseCwd); + const repository = await git.repo.resolve(repoRoot); + const sourceCommonDir = repository?.commonDir ?? path.join(repoRoot, ".git"); const baseDir = getWorktreeDir(getTaskIsolationSegment(repoRoot, id)); const mergedDir = path.join(baseDir, TASK_ISOLATION_MOUNT_DIR); const resolution = natives.isoResolve(preferred ?? null); @@ -424,6 +426,14 @@ export async function ensureIsolation( await fs.rm(baseDir, { recursive: true, force: true }); try { await natives.isoStart(candidate, repoRoot, mergedDir); + // Sever the isolation's git metadata from the source checkout. Copy + // backends duplicate `repoRoot`'s `.git` verbatim — a linked-worktree + // pointer file (or the rcopy `git worktree add` registration) leaves + // the isolation sharing the source's HEAD/index/ref namespace, so a + // task's git operations would mutate the parent checkout and stack + // parallel task branches. Detaching gives each isolation a private, + // frozen repo that still borrows the source object DB via alternates. + await git.detachGitDir(mergedDir, sourceCommonDir); return { mergedDir, backend: candidate, diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 2974b0c68..c6deadf73 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -1379,6 +1379,137 @@ export async function writeTree(cwd: string, options: Pick`), so + * the copy still resolves HEAD/index/refs through the source repo — a task's + * `git checkout`/`commit` inside the isolation then mutates the *parent* + * checkout. The rcopy `git worktree add` path leaks the other way: task + * branches land in the shared ref namespace and stack on each other. + * + * After detaching, the working tree keeps its files verbatim while: + * - HEAD, refs, and the index are frozen to the snapshot at call time; + * - all commits/branches the task creates stay private to the isolation; + * - objects resolve against `sourceCommonDir` via alternates, so history reads + * and later `git fetch ` object transfer keep working; + * - the source checkout's HEAD, branch, index, and working tree are untouched. + * + * A full-copy `.git` (non-worktree source) already owns its object DB and is + * returned as `"independent"` without modification. `worktreeRoot` without a + * `.git` yields `"no-git"`. + */ +export async function detachGitDir(worktreeRoot: string, sourceCommonDir: string): Promise { + ensureAvailable(); + const gitEntry = path.join(worktreeRoot, ".git"); + let entryStat: fs.Stats; + try { + entryStat = await fs.promises.lstat(gitEntry); + } catch (err) { + if (isEnoent(err)) return "no-git"; + throw err; + } + const parentCommon = path.resolve(sourceCommonDir); + const isoCommon = path.resolve( + ( + await runText(worktreeRoot, ["rev-parse", "--path-format=absolute", "--git-common-dir"], { + readOnly: true, + }) + ).trim(), + ); + // A full-copy `.git` already resolves to its own object DB — leave it alone. + if (isoCommon !== parentCommon) return "independent"; + + // Snapshot the state the standalone repo must preserve. HEAD may be a branch + // ref (normal checkout) or detached; refs are frozen so `baseSha..branch` + // ranges and history reads keep resolving after the source moves on. + const headSha = (await tryText(worktreeRoot, ["rev-parse", "HEAD"], { readOnly: true }))?.trim() ?? ""; + if (!headSha) return "independent"; // unborn HEAD: no shared state to sever + const headRef = (await tryText(worktreeRoot, ["symbolic-ref", "-q", "HEAD"], { readOnly: true }))?.trim() ?? ""; + const stagedTree = (await runText(worktreeRoot, ["write-tree"], {})).trim(); + const refDump = ( + await runText(worktreeRoot, ["for-each-ref", "--format=%(objectname) %(refname)"], { + readOnly: true, + }) + ).trim(); + const objectFormat = + (await tryText(worktreeRoot, ["rev-parse", "--show-object-format"], { readOnly: true }))?.trim() || "sha1"; + const userName = await config.get(worktreeRoot, "user.name"); + const userEmail = await config.get(worktreeRoot, "user.email"); + + // A pointer `.git` file whose worktree-admin dir back-references this exact + // tree is the rcopy `git worktree add` registration. Remove that admin entry + // so the source repo's worktree list stops tracking the isolation. A pointer + // referencing the *source's* admin (a copied linked-worktree `.git`) is not + // ours to delete — only the local pointer file is discarded. + let ownWorktreeAdmin: string | undefined; + if (entryStat.isFile()) { + const pointer = parseGitDirPointer((await readOptionalText(gitEntry)) ?? ""); + if (pointer) { + const adminDir = path.resolve(path.dirname(gitEntry), pointer); + const backRef = (await readOptionalText(path.join(adminDir, "gitdir")))?.trim(); + if (backRef && path.resolve(backRef) === path.resolve(gitEntry)) ownWorktreeAdmin = adminDir; + } + } + + await fs.promises.rm(gitEntry, { recursive: true, force: true }); + if (ownWorktreeAdmin) await fs.promises.rm(ownWorktreeAdmin, { recursive: true, force: true }); + + await runEffect(worktreeRoot, ["init", "--object-format", objectFormat, "-q"]); + const objectsInfo = path.join(gitEntry, "objects", "info"); + await fs.promises.mkdir(objectsInfo, { recursive: true }); + const alternates = [path.join(parentCommon, "objects")]; + const chained = await readOptionalText(path.join(parentCommon, "objects", "info", "alternates")); + if (chained) { + for (const line of chained.split("\n")) { + const entry = line.trim(); + if (!entry) continue; + alternates.push(path.isAbsolute(entry) ? entry : path.resolve(parentCommon, "objects", entry)); + } + } + await Bun.write(path.join(objectsInfo, "alternates"), `${alternates.join("\n")}\n`); + + // Freeze refs. Point HEAD at the raw SHA first so `update-ref` writes land + // even for the branch HEAD currently names, then restore the symbolic HEAD. + await Bun.write(path.join(gitEntry, "HEAD"), `${headSha}\n`); + if (refDump) { + const commands = refDump + .split("\n") + .filter(Boolean) + .map(line => { + const sep = line.indexOf(" "); + return `create ${line.slice(sep + 1)} ${line.slice(0, sep)}`; + }) + .join("\n"); + await runEffect(worktreeRoot, ["update-ref", "--stdin"], { stdin: `${commands}\n` }); + } + if (headRef) await Bun.write(path.join(gitEntry, "HEAD"), `ref: ${headRef}\n`); + + // Carry the source identity so isolated commits have an author. + if (userName) await config.set(worktreeRoot, "user.name", userName); + if (userEmail) await config.set(worktreeRoot, "user.email", userEmail); + // Restore the staged index without touching the working tree. + await readTree(worktreeRoot, stagedTree); + return "detached"; +} + // ════════════════════════════════════════════════════════════════════════════ // API: show // ════════════════════════════════════════════════════════════════════════════ diff --git a/packages/coding-agent/test/task/worktree.test.ts b/packages/coding-agent/test/task/worktree.test.ts index 3ac79ef9d..3176dc095 100644 --- a/packages/coding-agent/test/task/worktree.test.ts +++ b/packages/coding-agent/test/task/worktree.test.ts @@ -556,6 +556,134 @@ describe("getRepoRoot", () => { }); }); +describe("detachGitDir", () => { + // Build a source checkout whose `.git` is a linked-worktree pointer file — + // the exact shape (`gitdir: …/worktrees/`) that makes copy backends + // leak into the parent. Returns the linked worktree root plus its shared + // common dir and base SHA. + async function makeLinkedWorktree(): Promise<{ main: string; wt: string; commonDir: string; baseSha: string }> { + const main = await fs.mkdtemp(path.join(os.tmpdir(), "omp-detach-main-")); + tempDirs.push(main); + await runGit(main, ["init", "-q", "-b", "main"]); + await runGit(main, ["config", "user.email", "src@example.com"]); + await runGit(main, ["config", "user.name", "Source User"]); + await fs.writeFile(path.join(main, "file.txt"), "base\n"); + await runGit(main, ["add", "file.txt"]); + await runGit(main, ["commit", "-q", "-m", "base"]); + const wt = path.join(main, "..", `${path.basename(main)}-wt`); + tempDirs.push(wt); + await runGit(main, ["worktree", "add", "-q", wt, "-b", "feature/parent", "HEAD"]); + const commonDir = path.resolve( + (await runGit(main, ["rev-parse", "--path-format=absolute", "--git-common-dir"])).trim(), + ); + const baseSha = await runGit(wt, ["rev-parse", "HEAD"]); + return { main, wt, commonDir, baseSha }; + } + + // Mimic a copy isolation backend (reflink/apfs/rcopy): a verbatim tree copy, + // including the `.git` pointer file, into a fresh isolation directory. + async function copyTree(source: string): Promise { + const iso = await fs.mkdtemp(path.join(os.tmpdir(), "omp-detach-iso-")); + tempDirs.push(iso); + await fs.cp(source, iso, { recursive: true }); + return iso; + } + + it("severs a copied linked-worktree from the parent so task git ops stay isolated", async () => { + const { wt, commonDir, baseSha } = await makeLinkedWorktree(); + // Dirty the source worktree: staged, unstaged, and untracked changes. + await fs.writeFile(path.join(wt, "staged.txt"), "staged\n"); + await runGit(wt, ["add", "staged.txt"]); + await fs.writeFile(path.join(wt, "file.txt"), "unstaged\n"); + await fs.writeFile(path.join(wt, "untracked.txt"), "untracked\n"); + const iso = await copyTree(wt); + const statusBefore = await runGit(iso, ["status", "--porcelain=v1"]); + + const result = await git.detachGitDir(iso, commonDir); + + expect(result).toBe("detached"); + // Working tree (staged/unstaged/untracked) is preserved verbatim. + expect(await runGit(iso, ["status", "--porcelain=v1"])).toBe(statusBefore); + // The isolation now owns an independent common dir. + const isoCommon = path.resolve( + (await runGit(iso, ["rev-parse", "--path-format=absolute", "--git-common-dir"])).trim(), + ); + expect(isoCommon).not.toBe(commonDir); + + // A task creates its own branch from the requested base and commits. + await runGit(iso, ["checkout", "-q", "-b", "feature/a", baseSha]); + await fs.writeFile(path.join(iso, "a.txt"), "task a\n"); + await runGit(iso, ["add", "a.txt"]); + await runGit(iso, ["commit", "-q", "-m", "task a"]); + const taskCommit = await runGit(iso, ["rev-parse", "HEAD"]); + const taskParent = await runGit(iso, ["rev-parse", "HEAD^"]); + + // The parent worktree is untouched: same branch, no leaked task branch. + expect(await runGit(wt, ["rev-parse", "--abbrev-ref", "HEAD"])).toBe("feature/parent"); + expect(await runGit(wt, ["branch", "--format=%(refname:short)"])).not.toContain("feature/a"); + // The task commit is parented on the requested base, not on parent state. + expect(taskParent).toBe(baseSha); + // Objects still resolve through the borrowed source ODB: the parent can + // fetch the task branch (proving the alternates link is intact). + await runGit(wt, ["fetch", iso, "feature/a:refs/heads/omp-fetched"]); + expect(await runGit(wt, ["rev-parse", "omp-fetched"])).toBe(taskCommit); + }); + + it("leaves an already-independent full-copy checkout untouched", async () => { + const src = await fs.mkdtemp(path.join(os.tmpdir(), "omp-detach-src-")); + tempDirs.push(src); + await runGit(src, ["init", "-q", "-b", "main"]); + await runGit(src, ["config", "user.email", "src@example.com"]); + await runGit(src, ["config", "user.name", "Source User"]); + await fs.writeFile(path.join(src, "file.txt"), "base\n"); + await runGit(src, ["add", "file.txt"]); + await runGit(src, ["commit", "-q", "-m", "base"]); + const srcCommon = path.resolve( + (await runGit(src, ["rev-parse", "--path-format=absolute", "--git-common-dir"])).trim(), + ); + const iso = await copyTree(src); // full `.git` directory copied — its own ODB + + expect(await git.detachGitDir(iso, srcCommon)).toBe("independent"); + // Its objects are self-contained: no alternates file was written. + expect(await Bun.file(path.join(iso, ".git", "objects", "info", "alternates")).exists()).toBe(false); + }); + + it("keeps ensureIsolation from mutating a linked-worktree parent (rcopy backend)", async () => { + const { wt, baseSha } = await makeLinkedWorktree(); + vi.spyOn(natives, "isoResolve").mockReturnValue({ + kind: natives.IsoBackendKind.Rcopy, + candidates: [natives.IsoBackendKind.Rcopy], + fellBack: false, + reason: undefined, + }); + const worktreeBase = await fs.mkdtemp(path.join(os.tmpdir(), "omp-detach-wtbase-")); + tempDirs.push(worktreeBase); + const originalWorktreeDir = process.env.OMP_WORKTREE_DIR; + delete process.env.OMP_WORKTREE_DIR; + setWorktreesDir(worktreeBase); + try { + const handle = await ensureIsolation(wt, "parent-isolation-guard"); + await runGit(handle.mergedDir, ["checkout", "-q", "-b", "feature/a", baseSha]); + await fs.writeFile(path.join(handle.mergedDir, "a.txt"), "task a\n"); + await runGit(handle.mergedDir, ["add", "a.txt"]); + await runGit(handle.mergedDir, ["commit", "-q", "-m", "task a"]); + + // Parent branch, HEAD, and worktree list are all unchanged. + expect(await runGit(wt, ["rev-parse", "--abbrev-ref", "HEAD"])).toBe("feature/parent"); + expect(await runGit(wt, ["branch", "--format=%(refname:short)"])).not.toContain("feature/a"); + const worktrees = (await runGit(wt, ["worktree", "list", "--porcelain"])) + .split("\n") + .filter(line => line.startsWith("worktree ")); + expect(worktrees).toHaveLength(2); // main + the linked parent only + expect(await runGit(handle.mergedDir, ["rev-parse", "HEAD^"])).toBe(baseSha); + } finally { + setWorktreesDir(undefined); + if (originalWorktreeDir === undefined) delete process.env.OMP_WORKTREE_DIR; + else process.env.OMP_WORKTREE_DIR = originalWorktreeDir; + } + }); +}); + describe("applyNestedPatches", () => { let parentRepo: string; let nestedRel: string;