mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 16:51:28 +02:00
fix(workspace-runtime): base fresh worktrees on origin/master and refresh unstarted reuses (#8412)
## 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/<branch>`), 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/<branch>` 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/<branch>`, 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/<branch>` 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/<branch>` 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
This commit is contained in:
Devin Foley authored and GitHub committed
1 parent
a937b89a47
commit
5ebbe67ebd
2 files changed
+328
-9
No files matched your search
@@ -628,6 +628,123 @@ export async function inspectExecutionWorkspaceBaseDrift(input: {
|
||||
return { warnings, currentBaseRefSha, branchBaseRefSha };
|
||||
}
|
||||
|
||||
async function localBranchExists(repoRoot: string, branch: string): Promise<boolean> {
|
||||
return runGit(["show-ref", "--verify", "--quiet", `refs/heads/${branch}`], repoRoot)
|
||||
.then(() => true)
|
||||
.catch(() => false);
|
||||
}
|
||||
|
||||
async function remoteExists(repoRoot: string, remote: string): Promise<boolean> {
|
||||
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/<branch>` 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: {
|
||||
|
||||
Reference in new issue
Block a user