From 5ebbe67ebd9da015eb44e30cf802ae18e1c56fd3 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sat, 20 Jun 2026 18:51:28 -0700 Subject: [PATCH] fix(workspace-runtime): base fresh worktrees on origin/master and refresh unstarted reuses (#8412) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agent work runs inside git worktrees created by the workspace runtime, each branched from a configured base ref (typically the repo's default branch) > - When the local `master` is stale or ahead of `origin/master` (committed but unpushed work, or local-only commits), a freshly created worktree inherits that divergence — so an unrelated task branch silently carries commits it never intended to touch > - This surfaced as a docs-only task whose PR accidentally pulled in unrelated changes from a diverged local master > - The base for a fresh worktree should be resolved authoritatively to the remote-tracking ref (`origin/`), and an idle/unstarted reused worktree should be safely fast-forwarded — without ever destroying in-progress work > - This pull request makes both behaviors explicit in the workspace runtime > - The benefit is that task branches start from a clean, authoritative base, eliminating accidental inclusion of unrelated local changes ## Linked Issues or Issue Description This is a bug fix. No public GitHub issue exists, so describing it inline following the Bug Report template: **What happened** A task intended to change only docs produced a PR that also contained unrelated changes pulled in from `master`. The root cause: when a new worktree is created from a configured local branch (e.g. `master`), the worktree inherits whatever that local branch points at. If the local `master` has committed divergence from `origin/master` (unpushed or local-only commits), that divergence leaks into the new task branch. The leak comes from committed local-vs-`origin/master` ref drift, not uncommitted working-tree changes (each worktree has its own working tree). **Expected behavior** A fresh worktree should be based on the authoritative `origin/master` head so unrelated local commits never seed a task branch. **Steps to reproduce** 1. Have a local `master` that is ahead of `origin/master` (committed but unpushed work). 2. Create a new worktree/task branched from `master` via the workspace runtime. 3. Open a PR from that branch — it carries the unrelated local commits. **Deployment mode** Self-hosted / local workspace runtime. ## What Changed - Fresh worktrees now resolve their base ref authoritatively: a configured local branch (e.g. `"master"`) is mapped to its `origin/` remote-tracking counterpart so unpushed/ahead local commits can never seed a task branch. Remote-tracking refs, SHAs, and tags are used verbatim; an unset/`HEAD` base falls back to the detected default branch. The resolved ref is recorded (`repoRef`) so downstream drift checks stay accurate. - If a configured local branch has no matching `origin/`, the runtime warns and falls back to the local ref rather than failing. - On reuse, a *provably unstarted* worktree (no commits past base + clean tree including untracked files) is fast-forwarded to the latest `origin/master`. Started or dirty worktrees keep the prior warn-only behavior, so in-progress work is never reset. Only remote-tracking bases are eligible for the refresh. ## Verification - `cd server && npx vitest run src/__tests__/workspace-runtime.test.ts` - 5 new tests cover: local-branch→`origin/` mapping, no-remote fallback warning, unstarted-reuse fast-forward, and that started/dirty worktrees are left untouched. - Result: 65 passed. 1 pre-existing failure (`auto-detects the default branch via symbolic-ref when origin/HEAD is set`) is unrelated to this change and fails only due to the test host's git default-branch config (test setup runs `git push -u origin main master` but the local default branch is `main`); it also fails on `master`. ## Risks - Low risk. The refresh path is intentionally conservative: it only fast-forwards worktrees that are provably unstarted (zero commits past base and a fully clean tree, including untracked files) and only when the base is a remote-tracking ref. Started or dirty worktrees fall through to the existing warn-only drift behavior, so no in-progress work can be destroyed. - Behavioral shift: fresh worktrees configured against a local branch will now base on `origin/` instead of the local ref. This is the intended fix; the only case it changes is when local and remote have diverged. ## Model Used Claude — `claude-opus-4-8` (extended thinking, tool use / code execution via Claude Code). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — no UI changes) - [x] I have updated relevant documentation to reflect my changes (N/A — no doc changes needed) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending CI run) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge --- .../src/__tests__/workspace-runtime.test.ts | 168 +++++++++++++++++ server/src/services/workspace-runtime.ts | 169 +++++++++++++++++- 2 files changed, 328 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index 51bad01f..b0f9ac01 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -94,6 +94,57 @@ async function createTempRepo(defaultBranch = "main") { return repoRoot; } +async function createClonedRepoWithRemote() { + const sourceRepo = await createTempRepo("master"); + const remoteDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-remote-")); + const remotePath = path.join(remoteDir, "paperclip.git"); + await execFileAsync("git", ["clone", "--bare", sourceRepo, remotePath]); + + const cloneRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-clone-")); + const repoRoot = path.join(cloneRoot, "paperclip"); + await execFileAsync("git", ["clone", remotePath, repoRoot]); + await runGit(repoRoot, ["config", "user.email", "paperclip@example.com"]); + await runGit(repoRoot, ["config", "user.name", "Paperclip Test"]); + return { sourceRepo, remotePath, repoRoot }; +} + +async function advanceRemoteMaster(sourceRepo: string, remotePath: string, fileName: string) { + await fs.writeFile(path.join(sourceRepo, fileName), `${fileName}\n`, "utf8"); + await runGit(sourceRepo, ["add", fileName]); + await runGit(sourceRepo, ["commit", "-m", `Add ${fileName}`]); + await runGit(sourceRepo, ["push", remotePath, "master"]); + return readGit(sourceRepo, ["rev-parse", "master"]); +} + +function realizeWorktreeForTest(repoRoot: string, repoRef: string | null) { + return realizeExecutionWorkspace({ + base: { + baseCwd: repoRoot, + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef, + }, + config: { + workspaceStrategy: { + type: "git_worktree", + branchTemplate: "{{issue.identifier}}-{{slug}}", + }, + }, + issue: { + id: "issue-1", + identifier: "PAP-447", + title: "Add Worktree Support", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + }); +} + function buildWorkspace(cwd: string): RealizedExecutionWorkspace { return { baseCwd: cwd, @@ -508,6 +559,123 @@ describe("realizeExecutionWorkspace", () => { ]); }); + it("bases a fresh worktree on origin/master even when local master has unpushed commits", async () => { + const { repoRoot } = await createClonedRepoWithRemote(); + const originHead = await readGit(repoRoot, ["rev-parse", "origin/master"]); + + await fs.writeFile(path.join(repoRoot, "unpushed.txt"), "local only\n", "utf8"); + await runGit(repoRoot, ["add", "unpushed.txt"]); + await runGit(repoRoot, ["commit", "-m", "Unpushed local work"]); + const localHead = await readGit(repoRoot, ["rev-parse", "master"]); + expect(localHead).not.toBe(originHead); + + const workspace = await realizeWorktreeForTest(repoRoot, null); + + expect(workspace.baseRefSha).toBe(originHead); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(originHead); + }); + + it("maps a configured local branch base ref to origin/ for fresh worktrees", async () => { + const { repoRoot } = await createClonedRepoWithRemote(); + const originHead = await readGit(repoRoot, ["rev-parse", "origin/master"]); + + await fs.writeFile(path.join(repoRoot, "unpushed.txt"), "local only\n", "utf8"); + await runGit(repoRoot, ["add", "unpushed.txt"]); + await runGit(repoRoot, ["commit", "-m", "Unpushed local work"]); + const localHead = await readGit(repoRoot, ["rev-parse", "master"]); + expect(localHead).not.toBe(originHead); + + const workspace = await realizeWorktreeForTest(repoRoot, "master"); + + expect(workspace.repoRef).toBe("origin/master"); + expect(workspace.baseRefSha).toBe(originHead); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(originHead); + }); + + it("fast-forwards an unstarted reused worktree to the advanced origin/master", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + + const initial = await realizeWorktreeForTest(repoRoot, null); + const initialHead = await readGit(initial.cwd, ["rev-parse", "HEAD"]); + + const advancedHead = await advanceRemoteMaster(sourceRepo, remotePath, "auth-fix.txt"); + expect(advancedHead).not.toBe(initialHead); + + const reused = await realizeWorktreeForTest(repoRoot, null); + + expect(reused.created).toBe(false); + expect(reused.cwd).toBe(initial.cwd); + expect(await readGit(reused.cwd, ["rev-parse", "HEAD"])).toBe(advancedHead); + expect(reused.baseRefSha).toBe(advancedHead); + expect(reused.warnings).toEqual([]); + }); + + it("does not reset a reused worktree that already has task commits", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + + const initial = await realizeWorktreeForTest(repoRoot, null); + await fs.writeFile(path.join(initial.cwd, "task-work.txt"), "in progress\n", "utf8"); + await runGit(initial.cwd, ["add", "task-work.txt"]); + await runGit(initial.cwd, ["commit", "-m", "Task work in progress"]); + const taskHead = await readGit(initial.cwd, ["rev-parse", "HEAD"]); + + await advanceRemoteMaster(sourceRepo, remotePath, "auth-fix.txt"); + + const reused = await realizeWorktreeForTest(repoRoot, null); + + expect(reused.created).toBe(false); + expect(await readGit(reused.cwd, ["rev-parse", "HEAD"])).toBe(taskHead); + expect(reused.warnings).toEqual([ + expect.stringContaining("is behind origin/master by 1 commit"), + ]); + }); + + it("does not reset a reused worktree with untracked changes", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + + const initial = await realizeWorktreeForTest(repoRoot, null); + const initialHead = await readGit(initial.cwd, ["rev-parse", "HEAD"]); + await fs.writeFile(path.join(initial.cwd, "scratch.txt"), "uncommitted scratch\n", "utf8"); + + await advanceRemoteMaster(sourceRepo, remotePath, "auth-fix.txt"); + + const reused = await realizeWorktreeForTest(repoRoot, null); + + expect(reused.created).toBe(false); + expect(await readGit(reused.cwd, ["rev-parse", "HEAD"])).toBe(initialHead); + await expect(fs.readFile(path.join(reused.cwd, "scratch.txt"), "utf8")).resolves.toBe( + "uncommitted scratch\n", + ); + expect(reused.warnings).toEqual([ + expect.stringContaining("is behind origin/master by 1 commit"), + ]); + }); + + it("does not reset a reused worktree with untracked changes when status.showUntrackedFiles=no", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + + const initial = await realizeWorktreeForTest(repoRoot, null); + const initialHead = await readGit(initial.cwd, ["rev-parse", "HEAD"]); + // Without `--untracked-files=all`, this config hides untracked files from + // `git status --porcelain`, which would let the clean-tree guard pass and a + // `reset --hard` destroy the scratch file below. + await readGit(initial.cwd, ["config", "status.showUntrackedFiles", "no"]); + await fs.writeFile(path.join(initial.cwd, "scratch.txt"), "uncommitted scratch\n", "utf8"); + + await advanceRemoteMaster(sourceRepo, remotePath, "auth-fix.txt"); + + const reused = await realizeWorktreeForTest(repoRoot, null); + + expect(reused.created).toBe(false); + expect(await readGit(reused.cwd, ["rev-parse", "HEAD"])).toBe(initialHead); + await expect(fs.readFile(path.join(reused.cwd, "scratch.txt"), "utf8")).resolves.toBe( + "uncommitted scratch\n", + ); + expect(reused.warnings).toEqual([ + expect.stringContaining("is behind origin/master by 1 commit"), + ]); + }); + it("rejects reusing an empty directory that only looks like a worktree because it sits inside the repo", async () => { const repoRoot = await createTempRepo(); const branchName = "PAP-447-add-worktree-support"; diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index dc63868a..6d486a92 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -628,6 +628,123 @@ export async function inspectExecutionWorkspaceBaseDrift(input: { return { warnings, currentBaseRefSha, branchBaseRefSha }; } +async function localBranchExists(repoRoot: string, branch: string): Promise { + return runGit(["show-ref", "--verify", "--quiet", `refs/heads/${branch}`], repoRoot) + .then(() => true) + .catch(() => false); +} + +async function remoteExists(repoRoot: string, remote: string): Promise { + return runGit(["remote", "get-url", remote], repoRoot) + .then(() => true) + .catch(() => false); +} + +// Resolve the authoritative base ref for a fresh worktree. A configured local +// branch is mapped to its `origin/` counterpart so unpushed local +// divergence never leaks into the task branch; remote-tracking refs, SHAs, and +// tags are used verbatim, and an unset/`HEAD` base falls back to the detected +// default branch (which already prefers `origin/master`). +async function resolveAuthoritativeBaseRef( + repoRoot: string, + configuredBaseRef: string | null, +): Promise<{ baseRef: string; warnings: string[]; refreshed: boolean }> { + const warnings: string[] = []; + const detectOrHead = async () => (await detectDefaultBranch(repoRoot)) ?? "HEAD"; + + const configured = configuredBaseRef?.trim(); + if (!configured || configured === "HEAD") { + return { baseRef: await detectOrHead(), warnings, refreshed: false }; + } + + if (parseRemoteTrackingRef(configured)) { + return { baseRef: configured, warnings, refreshed: false }; + } + + if (await localBranchExists(repoRoot, configured)) { + const remoteCandidate = `origin/${configured}`; + // Refresh here and keep the warnings; the caller skips its own refresh of + // the returned ref (see `refreshed`) so we never fetch the same ref twice. + warnings.push(...await refreshRemoteTrackingBaseRef(repoRoot, remoteCandidate)); + if (await resolveBaseRefSha(repoRoot, remoteCandidate)) { + return { baseRef: remoteCandidate, warnings, refreshed: true }; + } + if (await remoteExists(repoRoot, "origin")) { + warnings.push( + `Configured base ref "${configured}" is a local branch with no matching origin/${configured}; basing the execution workspace on the local ref, which may include unpushed commits.`, + ); + } + return { baseRef: configured, warnings, refreshed: false }; + } + + return { baseRef: configured, warnings, refreshed: false }; +} + +// Auto-refresh a reused worktree to the latest base only when it is provably +// unstarted: no task commits past the base and a clean tree (including untracked +// files). This pulls an idle worktree forward to the freshest `origin/master` +// after a long planning phase without ever destroying in-progress work. Only +// remote-tracking bases are eligible; local-only bases keep warn-only drift. +async function refreshUnstartedWorktreeToBase(input: { + repoRoot: string; + worktreePath: string; + branchName: string | null; + baseRef: string; + currentBaseRefSha: string; + recorder?: WorkspaceOperationRecorder | null; +}): Promise<{ refreshed: boolean; baseRefSha: string | null }> { + if (!parseRemoteTrackingRef(input.baseRef)) { + return { refreshed: false, baseRefSha: null }; + } + + const headSha = await runGit(["rev-parse", "HEAD"], input.worktreePath).catch(() => null); + if (!headSha) { + return { refreshed: false, baseRefSha: null }; + } + if (headSha === input.currentBaseRefSha) { + return { refreshed: false, baseRefSha: input.currentBaseRefSha }; + } + + const commitsPastBaseRaw = await runGit( + ["rev-list", "--count", `${input.currentBaseRefSha}..HEAD`], + input.worktreePath, + ).catch(() => null); + const commitsPastBase = commitsPastBaseRaw === null ? null : Number.parseInt(commitsPastBaseRaw, 10); + if (commitsPastBase === null || !Number.isFinite(commitsPastBase) || commitsPastBase > 0) { + return { refreshed: false, baseRefSha: null }; + } + + // Force `--untracked-files=all` so untracked files are counted regardless of a + // local `status.showUntrackedFiles=no`; otherwise the clean-tree guard could + // pass and the `reset --hard` below would destroy untracked work. + const status = await runGit( + ["status", "--porcelain", "--untracked-files=all"], + input.worktreePath, + ).catch(() => null); + if (status === null || status.trim().length > 0) { + return { refreshed: false, baseRefSha: null }; + } + + await recordGitOperation(input.recorder, { + phase: "worktree_prepare", + args: ["reset", "--hard", input.currentBaseRefSha], + cwd: input.worktreePath, + metadata: { + repoRoot: input.repoRoot, + worktreePath: input.worktreePath, + branchName: input.branchName, + baseRef: input.baseRef, + previousHeadSha: headSha, + baseRefSha: input.currentBaseRefSha, + refreshedUnstartedWorktree: true, + }, + successMessage: `Refreshed unstarted git worktree at ${input.worktreePath} to ${input.baseRef} (${formatShortSha(input.currentBaseRefSha)})\n`, + failureLabel: `git reset --hard ${input.currentBaseRefSha}`, + }); + + return { refreshed: true, baseRefSha: input.currentBaseRefSha }; +} + type GitWorktreeListEntry = { worktree: string; @@ -1133,15 +1250,30 @@ export async function realizeExecutionWorkspace(input: { const configuredBaseRef = typeof rawStrategy.baseRef === "string" && rawStrategy.baseRef.length > 0 ? rawStrategy.baseRef : input.base.repoRef ?? null; - const baseRef = configuredBaseRef - ?? await detectDefaultBranch(repoRoot) - ?? "HEAD"; - const baseRefreshWarnings = await refreshRemoteTrackingBaseRef(repoRoot, baseRef); + const { + baseRef, + warnings: baseRefResolutionWarnings, + refreshed: baseRefAlreadyRefreshed, + } = await resolveAuthoritativeBaseRef(repoRoot, configuredBaseRef); + const baseRefreshWarnings = [ + ...baseRefResolutionWarnings, + ...(baseRefAlreadyRefreshed ? [] : await refreshRemoteTrackingBaseRef(repoRoot, baseRef)), + ]; const currentBaseRefSha = await resolveBaseRefSha(repoRoot, baseRef); await fs.mkdir(worktreeParentDir, { recursive: true }); async function reuseExistingWorktree(reusablePath: string) { + const refresh = currentBaseRefSha + ? await refreshUnstartedWorktreeToBase({ + repoRoot, + worktreePath: reusablePath, + branchName, + baseRef, + currentBaseRefSha, + recorder: input.recorder ?? null, + }) + : { refreshed: false, baseRefSha: null }; const baseDrift = await inspectExecutionWorkspaceBaseDrift({ repoRoot, worktreePath: reusablePath, @@ -1184,13 +1316,14 @@ export async function realizeExecutionWorkspace(input: { }); return { ...input.base, + repoRef: baseRef, strategy: "git_worktree" as const, cwd: reusablePath, branchName, worktreePath: reusablePath, warnings: [...baseRefreshWarnings, ...baseDrift.warnings], created: false, - baseRefSha: baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha, + baseRefSha: refresh.baseRefSha ?? baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha, }; } @@ -1284,6 +1417,7 @@ export async function realizeExecutionWorkspace(input: { return { ...input.base, + repoRef: baseRef, strategy: "git_worktree", cwd: worktreePath, branchName, @@ -1342,15 +1476,32 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { const repoRoot = await runGit(["rev-parse", "--show-toplevel"], input.base.baseCwd); const recordedBaseRefSha = readRecordedBaseRefSha(input.workspace.metadata); if (await directoryExists(cwd)) { + const reuseBaseRef = input.workspace.baseRef ?? input.base.repoRef ?? null; + const reuseWorktreePath = realized.worktreePath ?? cwd; + const baseRefreshWarnings = reuseBaseRef + ? await refreshRemoteTrackingBaseRef(repoRoot, reuseBaseRef) + : []; + const currentBaseRefSha = reuseBaseRef ? await resolveBaseRefSha(repoRoot, reuseBaseRef) : null; + const refresh = reuseBaseRef && currentBaseRefSha + ? await refreshUnstartedWorktreeToBase({ + repoRoot, + worktreePath: reuseWorktreePath, + branchName: realized.branchName, + baseRef: reuseBaseRef, + currentBaseRefSha, + recorder: input.recorder ?? null, + }) + : { refreshed: false, baseRefSha: null }; const baseDrift = await inspectExecutionWorkspaceBaseDrift({ repoRoot, - worktreePath: realized.worktreePath ?? cwd, + worktreePath: reuseWorktreePath, branchName: realized.branchName, - baseRef: input.workspace.baseRef ?? input.base.repoRef ?? null, + baseRef: reuseBaseRef, recordedBaseRefSha, + skipRefresh: true, }); - realized.warnings = baseDrift.warnings; - realized.baseRefSha = recordedBaseRefSha ?? baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha; + realized.warnings = [...baseRefreshWarnings, ...baseDrift.warnings]; + realized.baseRefSha = refresh.baseRefSha ?? recordedBaseRefSha ?? baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha; if (provisionCommand) { await provisionExecutionWorktree({ strategy: {