mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 04:02:18 +01:00
fix(workspaces): key terminal branch cleanup on branch ownership, not worktree creation
Attaching an isolated worktree to a pre-existing branch (exact-branch mode, or a literal branchTemplate naming an existing branch) returned created: true because the worktree was new. Heartbeat persistence mapped that to metadata.createdByRuntime, and terminal cleanup then deleted the operator-owned branch at its verified tip via the detached-HEAD guarded delete, leaving its commits unreachable. Separate the two signals. RealizedExecutionWorkspace gains a required branchCreatedByRuntime flag that is true only when realization created the branch ref itself (worktree add -b). Worktree attaches to existing branches, reuse paths, and project_primary all report false; restoring a persisted workspace carries the recorded ownership forward instead of resetting it. Heartbeat now persists metadata.createdByRuntime from this ownership flag, so cleanup keeps deleting genuinely runtime-created branches (still guarded) while removing only the worktree for attached pre-existing branches. `created` keeps its worktree-freshness meaning for provision env and workspace-ready comments. Regression tests run terminal cleanup end-to-end for both attach paths and assert the branch ref survives at its exact tip; they fail against the previous commit. Co-authored-by: Paperclip <noreply@paperclip.ing> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
992d687608
commit
8758a8716e
7 files changed
+183
-2
No files matched your search
@@ -3469,6 +3469,8 @@ describe("realizeExecutionWorkspace", () => {
|
||||
},
|
||||
});
|
||||
|
||||
expect(workspace.branchCreatedByRuntime).toBe(true);
|
||||
|
||||
const cleanup = await cleanupExecutionWorkspaceArtifacts({
|
||||
workspace: {
|
||||
id: "execution-workspace-1",
|
||||
@@ -3482,7 +3484,7 @@ describe("realizeExecutionWorkspace", () => {
|
||||
projectWorkspaceId: workspace.workspaceId,
|
||||
sourceIssueId: "issue-1",
|
||||
metadata: {
|
||||
createdByRuntime: true,
|
||||
createdByRuntime: workspace.branchCreatedByRuntime,
|
||||
},
|
||||
},
|
||||
projectWorkspace: {
|
||||
@@ -8368,6 +8370,10 @@ describe("realizeExecutionWorkspace with an exact existing branch", () => {
|
||||
|
||||
expect(workspace.strategy).toBe("git_worktree");
|
||||
expect(workspace.branchName).toBe("feature/preexisting-work");
|
||||
// The worktree is freshly created, but the pinned branch pre-existed and
|
||||
// stays operator-owned so terminal cleanup never deletes it.
|
||||
expect(workspace.created).toBe(true);
|
||||
expect(workspace.branchCreatedByRuntime).toBe(false);
|
||||
expect(workspace.cwd).not.toBe(repoRoot);
|
||||
expect(workspace.cwd).toContain(path.join(".paperclip", "worktrees"));
|
||||
expect(await readGit(workspace.cwd, ["branch", "--show-current"])).toBe("feature/preexisting-work");
|
||||
@@ -8446,6 +8452,146 @@ describe("realizeExecutionWorkspace with an exact existing branch", () => {
|
||||
expect(newMaster).not.toBe(oldMaster);
|
||||
});
|
||||
|
||||
function cleanupWorkspaceInput(
|
||||
workspace: RealizedExecutionWorkspace,
|
||||
repoRoot: string,
|
||||
expectedBranchHeadSha: string,
|
||||
) {
|
||||
return {
|
||||
workspace: {
|
||||
id: "execution-workspace-1",
|
||||
cwd: workspace.cwd,
|
||||
providerType: "git_worktree",
|
||||
providerRef: workspace.worktreePath,
|
||||
branchName: workspace.branchName,
|
||||
repoUrl: workspace.repoUrl,
|
||||
baseRef: workspace.repoRef,
|
||||
projectId: workspace.projectId,
|
||||
projectWorkspaceId: workspace.workspaceId,
|
||||
sourceIssueId: "issue-pr-prep",
|
||||
// Persist ownership the way the heartbeat does: from the branch
|
||||
// ownership signal, never from worktree creation.
|
||||
metadata: { createdByRuntime: workspace.branchCreatedByRuntime },
|
||||
},
|
||||
projectWorkspace: {
|
||||
cwd: repoRoot,
|
||||
cleanupCommand: null,
|
||||
},
|
||||
expectedBranchHeadSha,
|
||||
};
|
||||
}
|
||||
|
||||
it("terminal cleanup removes the worktree but preserves the attached pre-existing branch at its tip", async () => {
|
||||
const repoRoot = await createTempRepo();
|
||||
const branchTip = await createBranchWithCommit(repoRoot, "feature/operator-owned", "operator.txt");
|
||||
|
||||
const workspace = await realizeExistingBranch(repoRoot, "feature/operator-owned");
|
||||
expect(workspace.created).toBe(true);
|
||||
expect(workspace.branchCreatedByRuntime).toBe(false);
|
||||
|
||||
const cleanup = await cleanupExecutionWorkspaceArtifacts(
|
||||
cleanupWorkspaceInput(workspace, repoRoot, branchTip),
|
||||
);
|
||||
|
||||
expect(cleanup.cleaned).toBe(true);
|
||||
expect(cleanup.warnings).toEqual([]);
|
||||
await expect(fs.stat(workspace.cwd)).rejects.toThrow();
|
||||
expect(await readGit(repoRoot, ["rev-parse", "refs/heads/feature/operator-owned"])).toBe(branchTip);
|
||||
});
|
||||
|
||||
it("terminal cleanup preserves a pre-existing branch pinned via a literal branchTemplate", async () => {
|
||||
const repoRoot = await createTempRepo();
|
||||
const branchTip = await createBranchWithCommit(repoRoot, "feature/literal-pin", "literal.txt");
|
||||
|
||||
const workspace = await realizeExecutionWorkspace({
|
||||
base: {
|
||||
baseCwd: repoRoot,
|
||||
source: "project_primary",
|
||||
projectId: "project-1",
|
||||
workspaceId: "workspace-1",
|
||||
repoUrl: null,
|
||||
repoRef: "HEAD",
|
||||
},
|
||||
config: {
|
||||
workspaceStrategy: {
|
||||
type: "git_worktree",
|
||||
branchTemplate: "feature/literal-pin",
|
||||
},
|
||||
},
|
||||
issue: {
|
||||
id: "issue-pr-prep",
|
||||
identifier: "PAP-9002",
|
||||
title: "Prepare pull request via a literal branch template",
|
||||
},
|
||||
agent: {
|
||||
id: "agent-1",
|
||||
name: "Codex Coder",
|
||||
companyId: "company-1",
|
||||
},
|
||||
});
|
||||
|
||||
expect(workspace.branchName).toBe("feature/literal-pin");
|
||||
expect(workspace.created).toBe(true);
|
||||
expect(workspace.branchCreatedByRuntime).toBe(false);
|
||||
expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(branchTip);
|
||||
|
||||
const cleanup = await cleanupExecutionWorkspaceArtifacts(
|
||||
cleanupWorkspaceInput(workspace, repoRoot, branchTip),
|
||||
);
|
||||
|
||||
expect(cleanup.cleaned).toBe(true);
|
||||
expect(cleanup.warnings).toEqual([]);
|
||||
await expect(fs.stat(workspace.cwd)).rejects.toThrow();
|
||||
expect(await readGit(repoRoot, ["rev-parse", "refs/heads/feature/literal-pin"])).toBe(branchTip);
|
||||
});
|
||||
|
||||
it("restoring a persisted workspace keeps the recorded branch ownership", async () => {
|
||||
const repoRoot = await createTempRepo();
|
||||
await createBranchWithCommit(repoRoot, "feature/persisted-ownership", "persisted-ownership.txt");
|
||||
const first = await realizeExistingBranch(repoRoot, "feature/persisted-ownership");
|
||||
|
||||
async function restoreWithMetadata(metadata: Record<string, unknown>) {
|
||||
return await ensurePersistedExecutionWorkspaceAvailable({
|
||||
base: {
|
||||
baseCwd: repoRoot,
|
||||
source: "project_primary",
|
||||
projectId: "project-1",
|
||||
workspaceId: "workspace-1",
|
||||
repoUrl: null,
|
||||
repoRef: "HEAD",
|
||||
},
|
||||
workspace: {
|
||||
mode: "isolated_workspace",
|
||||
strategyType: "git_worktree",
|
||||
cwd: first.cwd,
|
||||
providerRef: first.worktreePath,
|
||||
projectId: "project-1",
|
||||
projectWorkspaceId: "workspace-1",
|
||||
repoUrl: null,
|
||||
baseRef: "HEAD",
|
||||
branchName: first.branchName,
|
||||
metadata,
|
||||
},
|
||||
issue: {
|
||||
id: "issue-pr-prep",
|
||||
identifier: "PAP-9003",
|
||||
title: "Restore keeps branch ownership",
|
||||
},
|
||||
agent: {
|
||||
id: "agent-1",
|
||||
name: "Codex Coder",
|
||||
companyId: "company-1",
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
const operatorOwned = await restoreWithMetadata({ createdByRuntime: false });
|
||||
expect(operatorOwned?.branchCreatedByRuntime).toBe(false);
|
||||
|
||||
const runtimeOwned = await restoreWithMetadata({ createdByRuntime: true });
|
||||
expect(runtimeOwned?.branchCreatedByRuntime).toBe(true);
|
||||
});
|
||||
|
||||
it("fails closed when an existing branch is pinned on a non-worktree strategy", async () => {
|
||||
const repoRoot = await createTempRepo();
|
||||
await createBranchWithCommit(repoRoot, "feature/wrong-mode", "wrong-mode.txt");
|
||||
|
||||
@@ -489,6 +489,7 @@ export function projectRoutes(db: Db) {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
},
|
||||
command: workspaceCommand.rawConfig,
|
||||
adapterEnv: {},
|
||||
@@ -544,6 +545,7 @@ export function projectRoutes(db: Db) {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
},
|
||||
config: { workspaceRuntime: runtimeConfig },
|
||||
adapterEnv: {},
|
||||
|
||||
@@ -14951,7 +14951,10 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
? reusableExistingExecutionWorkspace?.metadata ?? null
|
||||
: null,
|
||||
source: executionWorkspace.source,
|
||||
createdByRuntime: executionWorkspace.created,
|
||||
// Branch ownership, not worktree freshness: attaching a worktree to a
|
||||
// pre-existing branch reports created=true but must never make terminal
|
||||
// cleanup delete that operator-owned branch.
|
||||
createdByRuntime: executionWorkspace.branchCreatedByRuntime,
|
||||
configSnapshot,
|
||||
shouldReuseExisting: resolvedWorkspaceReusePolicy.shouldRestoreExistingWorkspace,
|
||||
shouldRefreshConfigSnapshot: resolvedWorkspaceReusePolicy.shouldRefreshWorkspaceConfigSnapshot,
|
||||
|
||||
@@ -344,6 +344,7 @@ export function buildWorkspaceRealizationRecordFromDriverInput(input: {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
},
|
||||
workspaceConfig: null,
|
||||
});
|
||||
|
||||
@@ -241,6 +241,7 @@ function startInput(options?: {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
},
|
||||
executionWorkspaceId: EXECUTION_WORKSPACE_ID,
|
||||
config: {
|
||||
|
||||
@@ -23,6 +23,7 @@ function workspace(
|
||||
worktreePath: "/repo/.paperclip/worktrees/PAP-16051",
|
||||
warnings: [],
|
||||
created: true,
|
||||
branchCreatedByRuntime: true,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -163,6 +163,12 @@ export interface RealizedExecutionWorkspace extends ExecutionWorkspaceInput {
|
||||
worktreePath: string | null;
|
||||
warnings: string[];
|
||||
created: boolean;
|
||||
// Branch ownership, distinct from `created` (which reports a fresh worktree
|
||||
// checkout). True only when this realization created the branch ref itself;
|
||||
// attaching a worktree to a pre-existing branch keeps the branch
|
||||
// operator-owned. Persisted as metadata.createdByRuntime, which terminal
|
||||
// cleanup uses to decide whether the branch may be deleted.
|
||||
branchCreatedByRuntime: boolean;
|
||||
baseRefSha?: string | null;
|
||||
pendingForwardBranchReconcile?: PendingForwardBranchReconcile | null;
|
||||
}
|
||||
@@ -3063,6 +3069,7 @@ export async function realizeExecutionWorkspace(input: {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
baseRefSha: null,
|
||||
};
|
||||
}
|
||||
@@ -3197,6 +3204,7 @@ export async function realizeExecutionWorkspace(input: {
|
||||
worktreePath: reusablePath,
|
||||
warnings: [...extraWarnings, ...baseRefreshWarnings, ...baseDrift.warnings],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
baseRefSha: refresh.baseRefSha ?? baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha,
|
||||
pendingForwardBranchReconcile,
|
||||
};
|
||||
@@ -3345,11 +3353,15 @@ export async function realizeExecutionWorkspace(input: {
|
||||
branchName,
|
||||
worktreePath,
|
||||
warnings: baseRefreshWarnings,
|
||||
// The worktree is new, but the pinned branch pre-existed: it stays
|
||||
// operator-owned so terminal cleanup never deletes it.
|
||||
created: true,
|
||||
branchCreatedByRuntime: false,
|
||||
baseRefSha: currentBaseRefSha,
|
||||
};
|
||||
}
|
||||
|
||||
let branchCreatedByRuntime = true;
|
||||
try {
|
||||
await recordGitOperation(input.recorder, {
|
||||
phase: "worktree_prepare",
|
||||
@@ -3387,6 +3399,9 @@ export async function realizeExecutionWorkspace(input: {
|
||||
successMessage: `Attached existing branch ${branchName} at ${worktreePath}\n`,
|
||||
failureLabel: `git worktree add ${worktreePath}`,
|
||||
});
|
||||
// The template rendered to a branch that already existed, so this
|
||||
// attach did not create the branch and cleanup must not delete it.
|
||||
branchCreatedByRuntime = false;
|
||||
} catch (attachError) {
|
||||
if (!gitErrorIncludes(attachError, "already checked out")) {
|
||||
throw attachError;
|
||||
@@ -3419,6 +3434,7 @@ export async function realizeExecutionWorkspace(input: {
|
||||
worktreePath,
|
||||
warnings: baseRefreshWarnings,
|
||||
created: true,
|
||||
branchCreatedByRuntime,
|
||||
baseRefSha: currentBaseRefSha,
|
||||
};
|
||||
}
|
||||
@@ -3469,6 +3485,10 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: {
|
||||
worktreePath: strategy === "git_worktree" ? (input.workspace.providerRef ?? cwd) : null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
// Restoring a persisted workspace keeps its recorded branch ownership, so
|
||||
// a runtime-created branch stays cleanup-eligible across reuse heartbeats
|
||||
// and an attached pre-existing branch stays operator-owned.
|
||||
branchCreatedByRuntime: input.workspace.metadata?.createdByRuntime === true,
|
||||
baseRefSha: readRecordedBaseRefSha(input.workspace.metadata),
|
||||
};
|
||||
const provisionCommand = asString(input.workspace.config?.provisionCommand, "").trim();
|
||||
@@ -3677,6 +3697,7 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: {
|
||||
worktreePath,
|
||||
warnings: [...restoreRefreshWarnings, ...baseDrift.warnings],
|
||||
created,
|
||||
branchCreatedByRuntime: realized.branchCreatedByRuntime || created,
|
||||
baseRefSha:
|
||||
recordedBaseRefSha
|
||||
?? (created ? restoreCurrentBaseRefSha : baseDrift.branchBaseRefSha)
|
||||
@@ -3837,6 +3858,10 @@ export async function cleanupExecutionWorkspaceArtifacts(input: {
|
||||
warnings.push(`Could not read worktree instance pointer: ${err instanceof Error ? err.message : String(err)}`);
|
||||
}
|
||||
}
|
||||
// Ownership signal persisted from RealizedExecutionWorkspace.branchCreatedByRuntime:
|
||||
// true only when the runtime created the branch ref (or local_fs directory)
|
||||
// itself. A worktree attached to a pre-existing branch persists false, so
|
||||
// cleanup below removes the worktree but leaves the branch untouched.
|
||||
const createdByRuntime = input.workspace.metadata?.createdByRuntime === true;
|
||||
const cleanupCommands = input.runCleanupCommands === false
|
||||
? []
|
||||
@@ -7896,6 +7921,7 @@ export async function restartDesiredRuntimeServicesOnStartup(db: Db) {
|
||||
worktreePath: null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: false,
|
||||
},
|
||||
config: {
|
||||
workspaceRuntime: runtimeConfig.workspaceRuntime,
|
||||
@@ -7950,6 +7976,7 @@ export async function restartDesiredRuntimeServicesOnStartup(db: Db) {
|
||||
worktreePath: row.strategyType === "git_worktree" ? row.cwd : null,
|
||||
warnings: [],
|
||||
created: false,
|
||||
branchCreatedByRuntime: (row.metadata as Record<string, unknown> | null)?.createdByRuntime === true,
|
||||
},
|
||||
executionWorkspaceId: row.id,
|
||||
config: {
|
||||
|
||||
Reference in new issue
Block a user