From 417336f8be108e476196f7f4b72f9556f3d490e0 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:23:18 -0500 Subject: [PATCH] fix(workspaces): attach PR preparation to existing branches (#11703) 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. > - Execution workspaces isolate an agent task from the primary checkout. > - Pull request preparation can need a branch that already contains completed work. > - The workspace policy could not require an exact existing branch. > - Workspace cleanup also treated worktree creation as branch ownership. > - This pull request adds an exact existing-branch policy and separate branch ownership metadata. > - The benefit is safe pull request preparation that preserves every existing commit and operator-owned branch. ## Linked Issues or Issue Description **What happened?** A pull request preparation run could not pin its execution workspace to an exact existing branch. Workspace reuse and cleanup could also confuse worktree creation with branch ownership. **Expected behavior** The run must attach only to the requested branch in an isolated Git worktree. It must fail if the branch is missing, busy, or inconsistent. Cleanup must not delete a branch that Paperclip does not own. **Steps to reproduce** 1. Create a branch that contains completed work. 2. Configure a pull request preparation task to use that branch. 3. Start the task and observe that the prior policy cannot require the exact branch. **Paperclip version or commit** This behavior reproduces on the base revision before this pull request. **Deployment mode** Local development with isolated Git worktrees. ## What Changed - Add `existingBranch` to the execution workspace policy and shared validation contracts. - Require `existingBranch` to use an isolated Git worktree and reject conflicting branch templates. - Attach to the exact branch without creating, renaming, resetting, or deleting it. - Track branch ownership separately from worktree creation and use that ownership during cleanup. - Return HTTP 422 for invalid existing-branch settings on every issue-producing route. - Add a bounded repair script for existing pull request preparation tasks. - Add focused policy, route, heartbeat, runtime, and ready-comment tests. - Document the exact-branch behavior and safety rules. ## Verification - `pnpm exec vitest run server/src/__tests__/execution-workspace-policy.test.ts server/src/__tests__/heartbeat-workspace-session.test.ts server/src/__tests__/issue-existing-branch-validation-status.test.ts server/src/__tests__/workspace-runtime.test.ts server/src/services/workspace-runtime-exposure.test.ts server/src/services/workspace-runtime-ready-comment.test.ts` passed 335 tests. - `pnpm -r typecheck` passed for all workspace projects. - `pnpm test:run` passed 4,431 tests. Two unrelated embedded-Postgres setup hooks timed out under aggregate load. Their isolated rerun passed 74 tests. - `pnpm build` passed for all workspace projects. - The two review regressions passed with 139 unrelated tests skipped. - All latest-head CI gates passed after one unrelated timing-sensitive test passed on rerun. - Greptile scored the latest head 5/5 with no unresolved review threads. ## Risks - Invalid workspace settings now return HTTP 422 instead of a generic validation response. - The exact branch must already exist and must not be checked out by another worktree. - The new policy fails closed when it cannot prove branch identity or ownership. - This change has no database migration. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex from the GPT-5 family assisted with this change. The runtime did not expose its exact deployment ID or context window. The agent used high-reasoning mode, repository tools, shell execution, and code execution. ## 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 - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip Co-authored-by: Claude Fable 5 --- doc/DEVELOPING.md | 2 + .../shared/src/types/workspace-runtime.ts | 6 + packages/shared/src/validators/issue.ts | 46 +- .../repair-pr-prep-workspace-attachment.mjs | 87 +++ .../execution-workspace-policy.test.ts | 48 ++ .../execution-workspaces-service.test.ts | 13 +- ...tbeat-workspace-branch-containment.test.ts | 4 + .../heartbeat-workspace-session.test.ts | 107 ++++ ...-existing-branch-validation-status.test.ts | 212 +++++++ server/src/__tests__/openapi-routes.test.ts | 1 + .../src/__tests__/workspace-runtime.test.ts | 571 +++++++++++++++++- server/src/middleware/validate.ts | 44 +- server/src/routes/issues.ts | 10 +- server/src/routes/openapi.ts | 8 +- server/src/routes/projects.ts | 2 + server/src/routes/routines.ts | 4 +- .../execution-workspace-branch-ownership.ts | 14 + .../services/execution-workspace-policy.ts | 3 + server/src/services/execution-workspaces.ts | 5 +- server/src/services/heartbeat.ts | 49 +- server/src/services/issues.ts | 1 + server/src/services/workspace-realization.ts | 1 + .../workspace-runtime-exposure.test.ts | 1 + .../workspace-runtime-ready-comment.test.ts | 1 + server/src/services/workspace-runtime.ts | 219 ++++++- 25 files changed, 1399 insertions(+), 60 deletions(-) create mode 100644 scripts/repair-pr-prep-workspace-attachment.mjs create mode 100644 server/src/__tests__/issue-existing-branch-validation-status.test.ts create mode 100644 server/src/services/execution-workspace-branch-ownership.ts diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 995a2d9e93..20747f2681 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -693,6 +693,8 @@ eval "$(npx paperclipai worktree env)" For project execution worktrees, Paperclip can also run a project-defined provision command after it creates or reuses an isolated git worktree. Configure this on the project's execution workspace policy (`workspaceStrategy.provisionCommand`). The command runs inside the derived worktree and receives `PAPERCLIP_WORKSPACE_*`, `PAPERCLIP_PROJECT_ID`, `PAPERCLIP_AGENT_ID`, and `PAPERCLIP_ISSUE_*` environment variables so each repo can bootstrap itself however it wants. +An issue can pin its isolated worktree to an exact pre-existing branch instead of a template-derived one — the contract PR-preparation tasks use. Set the issue's `executionWorkspaceSettings` to `{ "mode": "isolated_workspace", "workspaceStrategy": { "type": "git_worktree", "existingBranch": "" } }`. The validator requires isolated mode plus a `git_worktree` strategy and rejects `branchTemplate` alongside `existingBranch`. At dispatch the runtime attaches (never creates, renames, fast-forwards, or resets) that branch: it reuses a registered worktree that already has the branch checked out (including legacy `.worktrees/` paths), otherwise it attaches the branch under the managed worktree parent. A missing branch, an occupied worktree path on another branch, or a non-worktree strategy fails closed with a `workspace_validation_failed` error instead of falling back to the shared checkout or a derived branch, and an inherited `reuse_existing` workspace binding on a different branch is ignored in favor of realizing the pinned branch. + Heavier setup that is only needed by a managed runtime service can use `workspaceStrategy.runtimeProvisionCommand`. Paperclip runs this command lazily before spawning the first service in a start batch, serializes concurrent provisioning for the same workspace, and records the attempt as `workspace_runtime_provision`. The command receives the same workspace environment as `provisionCommand` and should be idempotent because later service-start batches invoke it again. Managed runtime control actions (`start`, `stop`, `restart`, and job `run`) are mutually exclusive per execution workspace. An overlapping control is rejected with `409 workspace_runtime_control_in_progress` instead of racing the active operation, and authorization is still checked first, so the conflict never widens who may control a workspace. diff --git a/packages/shared/src/types/workspace-runtime.ts b/packages/shared/src/types/workspace-runtime.ts index fcde0ef0fc..a36fbaf162 100644 --- a/packages/shared/src/types/workspace-runtime.ts +++ b/packages/shared/src/types/workspace-runtime.ts @@ -83,6 +83,12 @@ export interface ExecutionWorkspaceStrategy { type: ExecutionWorkspaceStrategyType; baseRef?: string | null; branchTemplate?: string | null; + /** + * Pin the worktree to this exact pre-existing branch instead of rendering + * `branchTemplate`. Realization attaches (never creates) the branch and + * fails closed when the branch does not exist or is not safely attachable. + */ + existingBranch?: string | null; worktreeParentDir?: string | null; provisionCommand?: string | null; runtimeProvisionCommand?: string | null; diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 8cd4e7cd2a..01e688cf0c 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -112,17 +112,50 @@ export const ISSUE_EXECUTION_WORKSPACE_PREFERENCES = [ "agent_default", ] as const; +// Mirrors git check-ref-format for a single branch name. The runtime verifies +// the branch actually exists before attaching, so this only rejects values +// that could never be a branch (path escapes, option injection, ref syntax). +export function isValidExistingBranchName(value: string): boolean { + if (value.length === 0 || value.length > 255) return false; + if (value.startsWith("-") || value.startsWith("/") || value.startsWith(".")) return false; + if (value.endsWith("/") || value.endsWith(".") || value.endsWith(".lock")) return false; + if (value.includes("..") || value.includes("//") || value.includes("@{") || value.includes("/.")) return false; + // eslint-disable-next-line no-control-regex + if (/[\x00-\x20\x7f~^:?*[\\]/.test(value)) return false; + return true; +} + const executionWorkspaceStrategySchema = z .object({ type: z.enum(["project_primary", "git_worktree", "adapter_managed", "cloud_sandbox"]).optional(), baseRef: z.string().optional().nullable(), branchTemplate: z.string().optional().nullable(), + existingBranch: z.string().trim().refine(isValidExistingBranchName, { + message: "existingBranch must be a valid git branch name", + }).optional().nullable(), worktreeParentDir: z.string().optional().nullable(), provisionCommand: z.string().optional().nullable(), runtimeProvisionCommand: z.string().optional().nullable(), teardownCommand: z.string().optional().nullable(), }) - .strict(); + .strict() + .superRefine((strategy, ctx) => { + if (!strategy.existingBranch) return; + if (strategy.type !== "git_worktree") { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["existingBranch"], + message: "existingBranch requires workspaceStrategy.type \"git_worktree\"", + }); + } + if (strategy.branchTemplate) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["existingBranch"], + message: "existingBranch and branchTemplate are mutually exclusive", + }); + } + }); const ipv4CidrPattern = /^(?:(?:25[0-5]|2[0-4]\d|[01]?\d\d?)\.){3}(?:25[0-5]|2[0-4]\d|[01]?\d\d?)\/(?:3[0-2]|[12]?\d)$/; const protectedTaskEgressCidrs = [ @@ -176,7 +209,16 @@ export const issueExecutionWorkspaceSettingsSchema = z )).max(100).optional(), }).strict().optional().nullable(), }) - .strict(); + .strict() + .superRefine((settings, ctx) => { + if (settings.workspaceStrategy?.existingBranch && settings.mode !== "isolated_workspace") { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["workspaceStrategy", "existingBranch"], + message: "existingBranch requires mode \"isolated_workspace\"", + }); + } + }); export const issueAssigneeAdapterOverridesSchema = z .object({ diff --git a/scripts/repair-pr-prep-workspace-attachment.mjs b/scripts/repair-pr-prep-workspace-attachment.mjs new file mode 100644 index 0000000000..b442b80d32 --- /dev/null +++ b/scripts/repair-pr-prep-workspace-attachment.mjs @@ -0,0 +1,87 @@ +#!/usr/bin/env node +// Bounded repair for PR-preparation issues stranded on a shared or stale +// execution-workspace binding (PAP-17617). For each explicit issue=branch +// pair, this pins the issue's executionWorkspaceSettings to the exact-branch +// isolated-worktree contract: +// { mode: "isolated_workspace", +// workspaceStrategy: { type: "git_worktree", existingBranch: "" } } +// Dispatch then attaches a git worktree on exactly that branch and ignores a +// mismatched inherited reuse_existing binding. The script only PATCHes issues +// through the company-scoped API; it never runs git and never mutates or +// deletes any branch. +// +// Usage: +// node scripts/repair-pr-prep-workspace-attachment.mjs PAP-16555=PAP-14380-salvage-pap-9514 [more ISSUE=BRANCH ...] [--apply] +// +// Without --apply it reports the current binding and the patch it would send. +// Requires PAPERCLIP_API_URL and PAPERCLIP_API_KEY in the environment. + +const args = process.argv.slice(2); +const apply = args.includes("--apply"); +const pairs = args.filter((arg) => !arg.startsWith("--")).map((arg) => { + const separator = arg.indexOf("="); + if (separator <= 0 || separator === arg.length - 1) { + console.error(`Expected ISSUE=BRANCH, got "${arg}"`); + process.exit(1); + } + return { issue: arg.slice(0, separator), branch: arg.slice(separator + 1) }; +}); +if (pairs.length === 0) { + console.error("No ISSUE=BRANCH pairs given. Nothing to repair."); + process.exit(1); +} + +const apiUrl = process.env.PAPERCLIP_API_URL; +const apiKey = process.env.PAPERCLIP_API_KEY; +if (!apiUrl || !apiKey) { + console.error("PAPERCLIP_API_URL and PAPERCLIP_API_KEY are required."); + process.exit(1); +} +const apiBase = apiUrl.replace(/\/$/, "").replace(/\/api$/, ""); +const headers = { + Authorization: `Bearer ${apiKey}`, + "Content-Type": "application/json", + ...(process.env.PAPERCLIP_RUN_ID ? { "X-Paperclip-Run-Id": process.env.PAPERCLIP_RUN_ID } : {}), +}; + +let failed = false; +for (const { issue, branch } of pairs) { + const current = await fetch(`${apiBase}/api/issues/${issue}`, { headers }); + if (!current.ok) { + console.error(`${issue}: fetch failed (${current.status})`); + failed = true; + continue; + } + const existing = await current.json(); + const settings = { + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: branch }, + }; + console.log( + `${issue}: status=${existing.status} preference=${existing.executionWorkspacePreference ?? "null"} ` + + `workspace=${existing.executionWorkspaceId ?? "null"} settings=${JSON.stringify(existing.executionWorkspaceSettings)}`, + ); + if (!apply) { + console.log(`${issue}: would PATCH executionWorkspaceSettings=${JSON.stringify(settings)}`); + continue; + } + const response = await fetch(`${apiBase}/api/issues/${issue}`, { + method: "PATCH", + headers, + body: JSON.stringify({ + executionWorkspaceSettings: settings, + comment: + `Repair for PAP-17617/PAP-17618: pinned this PR-preparation task to an isolated git worktree on the exact ` + + `existing branch \`${branch}\` via workspaceStrategy.existingBranch. Dispatch will attach that branch ` + + `(never create or reset it) and will not reuse the stale inherited workspace binding.`, + }), + }); + const body = await response.text(); + if (!response.ok) { + console.error(`${issue}: PATCH failed (${response.status}): ${body.slice(0, 500)}`); + failed = true; + continue; + } + console.log(`${issue}: PATCHED — executionWorkspaceSettings now pins existingBranch=${branch}`); +} +process.exit(failed ? 1 : 0); diff --git a/server/src/__tests__/execution-workspace-policy.test.ts b/server/src/__tests__/execution-workspace-policy.test.ts index 3669dd4e2f..2509d02994 100644 --- a/server/src/__tests__/execution-workspace-policy.test.ts +++ b/server/src/__tests__/execution-workspace-policy.test.ts @@ -82,6 +82,54 @@ describe("execution workspace policy helpers", () => { }).success).toBe(false); }); + it("accepts an existing-branch pin only with isolated mode and a git_worktree strategy", () => { + expect(issueExecutionWorkspaceSettingsSchema.parse({ + mode: "isolated_workspace", + workspaceStrategy: { + type: "git_worktree", + existingBranch: "PAP-14380-salvage-pap-9514", + }, + }).workspaceStrategy?.existingBranch).toBe("PAP-14380-salvage-pap-9514"); + + // Fail closed at the contract layer: an exact-branch pin outside an + // isolated git worktree could silently land in the shared checkout. + expect(issueExecutionWorkspaceSettingsSchema.safeParse({ + workspaceStrategy: { type: "git_worktree", existingBranch: "some-branch" }, + }).success).toBe(false); + expect(issueExecutionWorkspaceSettingsSchema.safeParse({ + mode: "shared_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "some-branch" }, + }).success).toBe(false); + expect(issueExecutionWorkspaceSettingsSchema.safeParse({ + mode: "isolated_workspace", + workspaceStrategy: { type: "project_primary", existingBranch: "some-branch" }, + }).success).toBe(false); + expect(issueExecutionWorkspaceSettingsSchema.safeParse({ + mode: "isolated_workspace", + workspaceStrategy: { + type: "git_worktree", + existingBranch: "some-branch", + branchTemplate: "{{issue.identifier}}-{{slug}}", + }, + }).success).toBe(false); + + for (const invalidBranch of ["-leading-dash", "a..b", "has space", "ends/", "back\\slash", "a.lock", "../escape"]) { + expect(issueExecutionWorkspaceSettingsSchema.safeParse({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: invalidBranch }, + }).success).toBe(false); + } + }); + + it("carries the existing-branch pin through issue settings parsing", () => { + expect( + parseIssueExecutionWorkspaceSettings({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: " PAP-14754-run-redaction " }, + })?.workspaceStrategy, + ).toEqual({ type: "git_worktree", existingBranch: "PAP-14754-run-redaction" }); + }); + it("centralizes unrunnable isolated worktree detection", () => { expect( isUnrunnableWorktreeCombo({ diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index 9f3693e1b9..421525ab89 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -1740,7 +1740,10 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { it("holds Git index and ref locks across terminal cleanup", async () => { const seeded = await seedTerminalWorkspace({ mergedPr: true }); await db.update(executionWorkspaces).set({ - metadata: { createdByRuntime: true }, + metadata: { + createdByRuntime: true, + gitBranchOwnershipVersion: 1, + }, }).where(eq(executionWorkspaces.id, seeded.executionWorkspaceId)); let commitFailure = ""; let refUpdateFailure = ""; @@ -4657,6 +4660,7 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { baseRef: "main", metadata: { createdByRuntime: true, + gitBranchOwnershipVersion: 1, config: { cleanupCommand: "printf 'workspace cleanup\\n'", }, @@ -4695,5 +4699,12 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { "git_worktree_remove", "git_branch_delete", ])); + + await db.update(executionWorkspaces).set({ + metadata: { createdByRuntime: true }, + }).where(eq(executionWorkspaces.id, executionWorkspaceId)); + const legacyReadiness = await svc.getCloseReadiness(executionWorkspaceId); + expect(legacyReadiness?.git?.createdByRuntime).toBe(false); + expect(legacyReadiness?.plannedActions.map((action) => action.kind)).not.toContain("git_branch_delete"); }, 20_000); }); diff --git a/server/src/__tests__/heartbeat-workspace-branch-containment.test.ts b/server/src/__tests__/heartbeat-workspace-branch-containment.test.ts index 38394581ea..48e483b8ce 100644 --- a/server/src/__tests__/heartbeat-workspace-branch-containment.test.ts +++ b/server/src/__tests__/heartbeat-workspace-branch-containment.test.ts @@ -408,6 +408,10 @@ async function seedBranchContainmentRun( branchName: expectedBranch, providerType: "git_worktree", providerRef: worktreePath, + metadata: { + createdByRuntime: true, + gitBranchOwnershipVersion: 1, + }, lastUsedAt: now, openedAt: now, createdAt: now, diff --git a/server/src/__tests__/heartbeat-workspace-session.test.ts b/server/src/__tests__/heartbeat-workspace-session.test.ts index 963c10ad2f..a5e51e2f4b 100644 --- a/server/src/__tests__/heartbeat-workspace-session.test.ts +++ b/server/src/__tests__/heartbeat-workspace-session.test.ts @@ -26,6 +26,7 @@ import { parseSessionCompactionPolicy, provisionExecutionWorkspaceForFreshnessDecision, reconcileReusedExecutionWorkspaceProjectWorkspaceId, + resolveExecutionWorkspaceBranchOwnership, resolveExecutionWorkspaceConfigFreshness, resolveExecutionWorkspaceReuseRequestForIssue, resolveExecutionWorkspaceReuseProvisioningPolicy, @@ -1267,11 +1268,54 @@ describe("applyPersistedExecutionWorkspaceConfig", () => { }); describe("mergeExecutionWorkspaceMetadataForPersistence", () => { + it("persists branch ownership independently of fresh worktree creation", () => { + const executionWorkspace = { + created: true, + branchCreatedByRuntime: false, + }; + + const metadata = mergeExecutionWorkspaceMetadataForPersistence({ + existingMetadata: null, + source: "task_session", + createdByRuntime: resolveExecutionWorkspaceBranchOwnership(executionWorkspace), + strategyType: "git_worktree", + configSnapshot: null, + shouldReuseExisting: false, + baseRef: "origin/main", + baseRefSha: null, + }); + + expect(metadata.createdByRuntime).toBe(false); + expect(metadata.gitBranchOwnershipVersion).toBe(1); + }); + + it("does not downgrade recorded runtime ownership after worktree reuse", () => { + const executionWorkspace = { + created: false, + branchCreatedByRuntime: true, + }; + + const metadata = mergeExecutionWorkspaceMetadataForPersistence({ + existingMetadata: { createdByRuntime: true }, + source: "task_session", + createdByRuntime: resolveExecutionWorkspaceBranchOwnership(executionWorkspace), + strategyType: "git_worktree", + configSnapshot: null, + shouldReuseExisting: false, + baseRef: "origin/main", + baseRefSha: null, + }); + + expect(metadata.createdByRuntime).toBe(true); + expect(metadata.gitBranchOwnershipVersion).toBe(1); + }); + it("merges config snapshot for newly realized workspaces", () => { expect(mergeExecutionWorkspaceMetadataForPersistence({ existingMetadata: null, source: "task_session", createdByRuntime: true, + strategyType: "project_primary", configSnapshot: { environmentId: "env-new", provisionCommand: "bash ./scripts/provision.sh", @@ -1305,6 +1349,7 @@ describe("mergeExecutionWorkspaceMetadataForPersistence", () => { }, source: "task_session", createdByRuntime: false, + strategyType: "project_primary", configSnapshot: { environmentId: "env-new", provisionCommand: "bash ./scripts/new-provision.sh", @@ -1327,6 +1372,7 @@ describe("mergeExecutionWorkspaceMetadataForPersistence", () => { existingMetadata: null, source: "task_session", createdByRuntime: true, + strategyType: "project_primary", configSnapshot: null, shouldReuseExisting: false, baseRef: "origin/main", @@ -1452,6 +1498,7 @@ describe("effective run execution workspace config freshness", () => { }, source: "task_session", createdByRuntime: false, + strategyType: "project_primary", configSnapshot: { workspaceRuntime: { services: [{ name: "web", command: "pnpm dev -- --host 0.0.0.0", port: 3200 }], @@ -1583,6 +1630,7 @@ describe("effective run execution workspace config freshness", () => { }, source: "task_session", createdByRuntime: false, + strategyType: "project_primary", configSnapshot: { provisionCommand: "pnpm install --frozen-lockfile", }, @@ -1676,6 +1724,65 @@ describe("effective run execution workspace config freshness", () => { expect(realizeWorkspace).not.toHaveBeenCalled(); }); + it.each([ + { name: "a different branch", branchName: "PAP-9001-derived-child-branch" }, + { name: "no recorded branch", branchName: null }, + ])( + "realizes the pinned existing branch instead of reusing an inherited workspace on $name", + async ({ branchName }) => { + const reuseRequest = resolveExecutionWorkspaceReuseRequestForIssue({ + issueExecutionWorkspaceId: "workspace-old", + issueExecutionWorkspacePreference: "reuse_existing", + existingExecutionWorkspaceStatus: "active", + requestedExistingBranch: "PAP-14380-salvage-pap-9514", + existingExecutionWorkspaceBranchName: branchName, + }); + + expect(reuseRequest).toEqual({ + requestedExecutionWorkspaceId: "workspace-old", + requestedShouldReuseExisting: false, + existingExecutionWorkspaceAvailable: false, + }); + + const metadata = buildWorkspaceConfigMetadata(); + const decision = resolveExecutionWorkspaceConfigFreshness({ + hasExistingWorkspace: false, + existingWorkspaceMetadata: null, + nextMetadata: metadata, + }); + const realizeWorkspace = vi.fn(async () => ({ id: "pinned-branch-workspace", warnings: [] })); + const restoreExistingWorkspace = vi.fn(async () => ({ id: "workspace-old", warnings: [] })); + + const result = await provisionExecutionWorkspaceForFreshnessDecision({ + requestedShouldReuseExisting: reuseRequest.requestedShouldReuseExisting, + existingExecutionWorkspaceId: reuseRequest.requestedExecutionWorkspaceId, + issueRef: { id: "issue-1", identifier: "PAP-42" }, + runId: "run-1", + workspaceConfigFreshness: decision, + restoreExistingWorkspace, + realizeWorkspace, + }); + + expect(result.executionWorkspace).toEqual({ id: "pinned-branch-workspace", warnings: [] }); + expect(result.reusedExecutionWorkspace).toBeNull(); + expect(restoreExistingWorkspace).not.toHaveBeenCalled(); + }, + ); + + it("keeps reusing an inherited workspace whose branch matches the pinned existing branch", () => { + expect(resolveExecutionWorkspaceReuseRequestForIssue({ + issueExecutionWorkspaceId: "workspace-old", + issueExecutionWorkspacePreference: "reuse_existing", + existingExecutionWorkspaceStatus: "active", + requestedExistingBranch: "PAP-14380-salvage-pap-9514", + existingExecutionWorkspaceBranchName: "PAP-14380-salvage-pap-9514", + })).toEqual({ + requestedExecutionWorkspaceId: "workspace-old", + requestedShouldReuseExisting: true, + existingExecutionWorkspaceAvailable: true, + }); + }); + it("fails loudly when explicit reuse restore returns no workspace", async () => { const metadata = buildWorkspaceConfigMetadata(); const decision = resolveExecutionWorkspaceConfigFreshness({ diff --git a/server/src/__tests__/issue-existing-branch-validation-status.test.ts b/server/src/__tests__/issue-existing-branch-validation-status.test.ts new file mode 100644 index 0000000000..8c7a3184bb --- /dev/null +++ b/server/src/__tests__/issue-existing-branch-validation-status.test.ts @@ -0,0 +1,212 @@ +import express from "express"; +import { readFileSync } from "node:fs"; +import request from "supertest"; +import { describe, expect, it, vi } from "vitest"; +import { + createAcceptedPlanDecompositionSchema, + createIssueSchema, + runRoutineSchema, + updateIssueSchema, +} from "@paperclipai/shared"; +import { z } from "zod"; +import { errorHandler } from "../middleware/error-handler.js"; +import { validateIssueMutationBody } from "../middleware/validate.js"; + +const EXISTING_BRANCH_PATH = ["executionWorkspaceSettings", "workspaceStrategy", "existingBranch"]; + +// Mirrors the route-level schema in routes/issues.ts. +const updateIssueRouteSchema = updateIssueSchema.extend({ + interrupt: z.boolean().optional(), +}); + +function buildApp(schema: Parameters[0]) { + const handler = vi.fn((_req: express.Request, res: express.Response) => { + res.status(200).json({ ok: true }); + }); + const app = express(); + app.use(express.json()); + app.post("/target", validateIssueMutationBody(schema), handler); + app.use(errorHandler); + return { app, handler }; +} + +function settingsBody(settings: Record) { + return { executionWorkspaceSettings: settings }; +} + +describe("existingBranch issue create/update validation status", () => { + it("returns 422 with field details for invalid branch syntax on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + })); + expect(res.status).toBe(422); + expect(res.body.error).toBe("Validation error"); + expect(res.body.details).toHaveLength(1); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(res.body.details[0].message).toBe("existingBranch must be a valid git branch name"); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for placement outside isolated_workspace + git_worktree on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "shared_workspace", + workspaceStrategy: { type: "project_primary", existingBranch: "feature/pinned" }, + })); + expect(res.status).toBe(422); + const messages = res.body.details.map((detail: { message: string }) => detail.message).sort(); + expect(messages).toEqual([ + 'existingBranch requires mode "isolated_workspace"', + 'existingBranch requires workspaceStrategy.type "git_worktree"', + ]); + for (const detail of res.body.details) { + expect(detail.path).toEqual(EXISTING_BRANCH_PATH); + } + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 when existingBranch is combined with branchTemplate on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { + type: "git_worktree", + existingBranch: "feature/pinned", + branchTemplate: "agent/{issue}", + }, + })); + expect(res.status).toBe(422); + expect(res.body.details).toHaveLength(1); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(res.body.details[0].message).toBe("existingBranch and branchTemplate are mutually exclusive"); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for the same semantic failures on create", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ + title: "Pinned worktree issue", + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "-bad-leading-dash" }, + }), + }); + expect(res.status).toBe(422); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for an invalid branch on a routine-created issue", async () => { + const { app, handler } = buildApp(runRoutineSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + })); + expect(res.status).toBe(422); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for an invalid branch nested in an accepted plan child", async () => { + const { app, handler } = buildApp(createAcceptedPlanDecompositionSchema); + const res = await request(app).post("/target").send({ + acceptedPlanRevisionId: "11111111-1111-4111-8111-111111111111", + children: [{ + title: "Pinned child", + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + }), + }], + }); + expect(res.status).toBe(422); + expect(res.body.details[0].path).toEqual(["children", 0, ...EXISTING_BRANCH_PATH]); + expect(handler).not.toHaveBeenCalled(); + }); + + it("keeps 400 for validation failures unrelated to existingBranch", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ title: 42, status: "todo" }); + expect(res.status).toBe(400); + expect(res.body.error).toBe("Validation error"); + expect(res.body.details.length).toBeGreaterThan(0); + expect(handler).not.toHaveBeenCalled(); + }); + + it("keeps 400 when existingBranch and unrelated failures are mixed", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ + title: 42, + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + }), + }); + expect(res.status).toBe(400); + expect(handler).not.toHaveBeenCalled(); + }); + + it("keeps 400 when a nested existingBranch failure is mixed with an unrelated child failure", async () => { + const { app, handler } = buildApp(createAcceptedPlanDecompositionSchema); + const res = await request(app).post("/target").send({ + acceptedPlanRevisionId: "11111111-1111-4111-8111-111111111111", + children: [{ + title: 42, + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + }), + }], + }); + expect(res.status).toBe(400); + expect(handler).not.toHaveBeenCalled(); + }); + + it("passes a valid pinned existingBranch through to the handler", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "feature/pinned" }, + })); + expect(res.status).toBe(200); + expect(handler).toHaveBeenCalledTimes(1); + }); +}); + +describe("existingBranch issue mutation route registrations", () => { + const routes = [ + { + source: readFileSync(new URL("../routes/issues.ts", import.meta.url), "utf8"), + schemas: [ + "createIssueSchema", + "createChildIssueSchema", + "createAcceptedPlanDecompositionSchema", + "updateIssueRouteSchema", + ], + }, + { + source: readFileSync(new URL("../routes/routines.ts", import.meta.url), "utf8"), + schemas: ["runRoutineSchema"], + }, + ]; + + it("uses the semantic issue validator for every issue route schema with workspace settings", () => { + for (const { source, schemas } of routes) { + for (const schema of schemas) { + const validators = Array.from( + source.matchAll(new RegExp(`\\b(validate(?:IssueMutationBody)?)\\(${schema}\\)`, "g")), + (match) => match[1], + ); + expect(validators, `${schema} must use validateIssueMutationBody at every route registration`).toEqual([ + "validateIssueMutationBody", + ]); + } + } + }); +}); diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index d417e0b831..3d8c85a5f0 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -300,6 +300,7 @@ describe("openapi routes", () => { expect(spec.paths["/api/invites/{token}/accept"].post.responses["202"]).toBeDefined(); expect(spec.paths["/api/board-api-keys"].post.responses["201"]).toBeDefined(); expect(spec.paths["/api/companies/import"].post.responses["202"]).toBeDefined(); + expect(spec.paths["/api/routines/{id}/run"].post.responses["422"]).toBeDefined(); }); it("publishes the Claude browser-code grammar and strict setup-token response shapes", () => { diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index adf2db7574..795bdfc828 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -72,6 +72,11 @@ import { startEmbeddedPostgresTestDatabase, } from "./helpers/embedded-postgres.js"; +const RUNTIME_OWNED_GIT_BRANCH_METADATA = { + createdByRuntime: true, + gitBranchOwnershipVersion: 1, +} as const; + const execFileAsync = promisify(execFile); function stableStringifyForTest(value: unknown): string { @@ -190,6 +195,7 @@ async function expectPersistedBranchMismatchRejected(input: { repoUrl: null, baseRef: "HEAD", branchName: input.expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: input.issueId, @@ -738,8 +744,7 @@ describe("realizeExecutionWorkspace", () => { it("creates and reuses a git worktree for an issue-scoped branch", async () => { const repoRoot = await createTempRepo(); - - const first = await realizeExecutionWorkspace({ + const realizationInput = { base: { baseCwd: repoRoot, source: "project_primary", @@ -764,42 +769,27 @@ describe("realizeExecutionWorkspace", () => { name: "Codex Coder", companyId: "company-1", }, - }); + } satisfies Parameters[0]; + + const first = await realizeExecutionWorkspace(realizationInput); expect(first.strategy).toBe("git_worktree"); expect(first.created).toBe(true); + expect(first.branchCreatedByRuntime).toBe(true); expect(first.branchName).toBe("PAP-447-add-worktree-support"); expect(first.cwd).toContain(path.join(".paperclip", "worktrees")); await expect(fs.stat(path.join(first.cwd, ".git"))).resolves.toBeTruthy(); const second = await realizeExecutionWorkspace({ - base: { - baseCwd: repoRoot, - source: "project_primary", - projectId: "project-1", - workspaceId: "workspace-1", - repoUrl: null, - repoRef: "HEAD", - }, - 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", + ...realizationInput, + recordedBranchOwnership: { + branchName: first.branchName!, + createdByRuntime: first.branchCreatedByRuntime, }, }); expect(second.created).toBe(false); + expect(second.branchCreatedByRuntime).toBe(true); expect(second.cwd).toBe(first.cwd); expect(second.branchName).toBe(first.branchName); }); @@ -2695,6 +2685,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-1", @@ -2762,6 +2753,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-detached", @@ -2813,6 +2805,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-2", @@ -2965,6 +2958,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: initial.branchName, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-3", @@ -3026,6 +3020,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-diverged", @@ -3099,6 +3094,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-deleted-branch", @@ -3180,6 +3176,7 @@ describe("realizeExecutionWorkspace", () => { repoUrl: null, baseRef: "HEAD", branchName: expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: "issue-deleted-branch-flag-off", @@ -3655,6 +3652,8 @@ describe("realizeExecutionWorkspace", () => { }, }); + expect(workspace.branchCreatedByRuntime).toBe(true); + const cleanup = await cleanupExecutionWorkspaceArtifacts({ workspace: { id: "execution-workspace-1", @@ -3668,7 +3667,8 @@ describe("realizeExecutionWorkspace", () => { projectWorkspaceId: workspace.workspaceId, sourceIssueId: "issue-1", metadata: { - createdByRuntime: true, + createdByRuntime: workspace.branchCreatedByRuntime, + gitBranchOwnershipVersion: 1, }, }, projectWorkspace: { @@ -3687,6 +3687,83 @@ describe("realizeExecutionWorkspace", () => { }); }); + it("deletes a runtime-created branch at its verified tip after a reuse realization", async () => { + const repoRoot = await createTempRepo(); + const realizationInput = { + base: { + baseCwd: repoRoot, + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef: "HEAD", + }, + config: { + workspaceStrategy: { + type: "git_worktree", + branchTemplate: "{{issue.identifier}}-{{slug}}", + }, + }, + issue: { + id: "issue-reused-cleanup", + identifier: "PAP-17633", + title: "Preserve branch ownership across reuse", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + } satisfies Parameters[0]; + + const initial = await realizeExecutionWorkspace(realizationInput); + expect(initial.branchCreatedByRuntime).toBe(true); + const verifiedBranchTip = await readGit(initial.cwd, ["rev-parse", "HEAD"]); + + const reused = await realizeExecutionWorkspace({ + ...realizationInput, + recordedBranchOwnership: { + branchName: initial.branchName!, + createdByRuntime: initial.branchCreatedByRuntime, + }, + }); + expect(reused.created).toBe(false); + expect(reused.branchCreatedByRuntime).toBe(true); + + const cleanup = await cleanupExecutionWorkspaceArtifacts({ + workspace: { + id: "execution-workspace-reused-cleanup", + cwd: reused.cwd, + providerType: "git_worktree", + providerRef: reused.worktreePath, + branchName: reused.branchName, + repoUrl: reused.repoUrl, + baseRef: reused.repoRef, + projectId: reused.projectId, + projectWorkspaceId: reused.workspaceId, + sourceIssueId: "issue-reused-cleanup", + metadata: { + createdByRuntime: reused.branchCreatedByRuntime, + gitBranchOwnershipVersion: 1, + }, + }, + projectWorkspace: { + cwd: repoRoot, + cleanupCommand: null, + }, + expectedBranchHeadSha: verifiedBranchTip, + }); + + expect(cleanup.cleaned).toBe(true); + expect(cleanup.warnings).toEqual([]); + await expect(fs.stat(reused.cwd)).rejects.toThrow(); + await expect( + execFileAsync("git", ["rev-parse", "--verify", `refs/heads/${reused.branchName}`], { + cwd: repoRoot, + }), + ).rejects.toThrow(); + }); + it("keeps a runtime-created branch when its tip changes after guarded worktree removal", async () => { const repoRoot = await createTempRepo(); const workspace = await realizeExecutionWorkspace({ @@ -3733,7 +3810,7 @@ describe("realizeExecutionWorkspace", () => { projectId: workspace.projectId, projectWorkspaceId: workspace.workspaceId, sourceIssueId: "issue-1", - metadata: { createdByRuntime: true }, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, projectWorkspace: { cwd: repoRoot, @@ -3798,7 +3875,7 @@ describe("realizeExecutionWorkspace", () => { projectWorkspaceId: workspace.workspaceId, sourceIssueId: "issue-1", metadata: { - createdByRuntime: true, + ...RUNTIME_OWNED_GIT_BRANCH_METADATA, }, }, projectWorkspace: { @@ -3873,7 +3950,7 @@ describe("realizeExecutionWorkspace", () => { projectWorkspaceId: workspace.workspaceId, sourceIssueId: "issue-1", metadata: { - createdByRuntime: true, + ...RUNTIME_OWNED_GIT_BRANCH_METADATA, worktreeInstanceRoot: instanceRoot, }, }, @@ -5597,6 +5674,7 @@ describeEmbeddedPostgres("workspace dirty quarantine branch repair", () => { repoUrl: null, baseRef: "HEAD", branchName: input.expectedBranch, + metadata: RUNTIME_OWNED_GIT_BRANCH_METADATA, }, issue: { id: input.ids.sourceIssueId, @@ -8886,3 +8964,436 @@ describe("workspace realization request additionalSources", () => { expect(parsed?.runtimeOverlay.runtimeProvisionCommand).toBeNull(); }); }); + +describe("realizeExecutionWorkspace with an exact existing branch", () => { + function realizeExistingBranch( + repoRoot: string, + existingBranch: string, + strategyOverrides: Record = {}, + recordedBranchOwnership: Parameters[0]["recordedBranchOwnership"] = null, + ) { + return realizeExecutionWorkspace({ + base: { + baseCwd: repoRoot, + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef: "HEAD", + }, + config: { + workspaceStrategy: { + type: "git_worktree", + existingBranch, + ...strategyOverrides, + }, + }, + issue: { + id: "issue-pr-prep", + identifier: "PAP-9001", + title: "Prepare pull request for an existing branch", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + recordedBranchOwnership, + }); + } + + function restoreExistingBranch( + repoRoot: string, + workspace: RealizedExecutionWorkspace, + metadata: Record, + ) { + return ensurePersistedExecutionWorkspaceAvailable({ + base: { + baseCwd: repoRoot, + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef: workspace.repoRef, + }, + workspace: { + id: "execution-workspace-existing-branch", + mode: "isolated_workspace", + strategyType: "git_worktree", + cwd: workspace.cwd, + providerRef: workspace.worktreePath, + projectId: "project-1", + projectWorkspaceId: "workspace-1", + repoUrl: null, + baseRef: workspace.repoRef, + branchName: workspace.branchName, + metadata, + }, + issue: { + id: "issue-pr-prep", + identifier: "PAP-9001", + title: "Restore an existing-branch pull request workspace", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + }); + } + + async function createBranchWithCommit(repoRoot: string, branchName: string, fileName: string) { + const baseBranch = await readGit(repoRoot, ["rev-parse", "--abbrev-ref", "HEAD"]); + await runGit(repoRoot, ["checkout", "-b", branchName]); + await fs.writeFile(path.join(repoRoot, fileName), `${fileName}\n`, "utf8"); + await runGit(repoRoot, ["add", fileName]); + await runGit(repoRoot, ["commit", "-m", `Add ${fileName}`]); + await runGit(repoRoot, ["checkout", baseBranch]); + return readGit(repoRoot, ["rev-parse", branchName]); + } + + it("attaches an isolated worktree checked out on exactly the requested branch", async () => { + const repoRoot = await createTempRepo(); + const branchTip = await createBranchWithCommit(repoRoot, "feature/preexisting-work", "existing.txt"); + + const workspace = await realizeExistingBranch(repoRoot, "feature/preexisting-work"); + + 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"); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(branchTip); + expect(await readGit(repoRoot, ["rev-parse", "feature/preexisting-work"])).toBe(branchTip); + }); + + it("reuses a registered legacy worktree that already has the branch checked out", async () => { + const repoRoot = await createTempRepo(); + const branchTip = await createBranchWithCommit(repoRoot, "feature/legacy-checkout", "legacy.txt"); + const legacyPath = path.join(repoRoot, ".worktrees", "legacy-checkout"); + await runGit(repoRoot, ["worktree", "add", legacyPath, "feature/legacy-checkout"]); + + const workspace = await realizeExistingBranch(repoRoot, "feature/legacy-checkout"); + + expect(workspace.cwd).toBe(path.resolve(legacyPath)); + expect(workspace.branchName).toBe("feature/legacy-checkout"); + expect(workspace.created).toBe(false); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(branchTip); + expect(await readGit(repoRoot, ["rev-parse", "feature/legacy-checkout"])).toBe(branchTip); + }); + + it("does not let exact-branch reuse inherit runtime ownership", async () => { + const repoRoot = await createTempRepo(); + await createBranchWithCommit(repoRoot, "feature/operator-owned-reuse", "operator-reuse.txt"); + const legacyPath = path.join(repoRoot, ".worktrees", "operator-owned-reuse"); + await runGit(repoRoot, ["worktree", "add", legacyPath, "feature/operator-owned-reuse"]); + + const workspace = await realizeExistingBranch( + repoRoot, + "feature/operator-owned-reuse", + {}, + { + branchName: "feature/operator-owned-reuse", + createdByRuntime: true, + }, + ); + + expect(workspace.created).toBe(false); + expect(workspace.branchCreatedByRuntime).toBe(false); + }); + + it("fails closed when the requested branch does not exist and creates nothing", async () => { + const repoRoot = await createTempRepo(); + + await expect(realizeExistingBranch(repoRoot, "feature/never-created")).rejects.toMatchObject({ + code: "workspace_validation_failed", + resultJson: { + workspaceValidation: expect.objectContaining({ + reason: "existing_branch_not_found", + requestedExistingBranch: "feature/never-created", + }), + }, + }); + + expect(await readGit(repoRoot, ["branch", "--list", "feature/never-created"])).toBe(""); + expect( + existsSync(path.join(repoRoot, ".paperclip", "worktrees", "feature", "never-created")), + ).toBe(false); + }); + + it("fails closed instead of reconciling when the managed worktree path holds another branch", async () => { + const repoRoot = await createTempRepo(); + const branchTip = await createBranchWithCommit(repoRoot, "feature/pinned", "pinned.txt"); + const managedPath = path.join(repoRoot, ".paperclip", "worktrees", "feature/pinned"); + await runGit(repoRoot, ["worktree", "add", "-b", "stale-occupant", managedPath]); + + await expect(realizeExistingBranch(repoRoot, "feature/pinned")).rejects.toMatchObject({ + code: "workspace_validation_failed", + resultJson: { + workspaceValidation: expect.objectContaining({ + reason: "existing_branch_worktree_not_reusable", + requestedExistingBranch: "feature/pinned", + }), + }, + }); + + expect(await readGit(repoRoot, ["rev-parse", "feature/pinned"])).toBe(branchTip); + expect(await readGit(managedPath, ["branch", "--show-current"])).toBe("stale-occupant"); + }); + + it("never fast-forwards an unstarted exact branch to a newer base ref", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + const oldMaster = await readGit(repoRoot, ["rev-parse", "origin/master"]); + await runGit(repoRoot, ["branch", "release/frozen", oldMaster]); + + const first = await realizeExistingBranch(repoRoot, "release/frozen", { baseRef: "origin/master" }); + expect(await readGit(first.cwd, ["rev-parse", "HEAD"])).toBe(oldMaster); + + const newMaster = await advanceRemoteMaster(sourceRepo, remotePath, "advance.txt"); + const reused = await restoreExistingBranch(repoRoot, first, { + createdByRuntime: false, + gitBranchOwnershipVersion: 1, + }); + + expect(reused?.cwd).toBe(first.cwd); + expect(await readGit(first.cwd, ["rev-parse", "HEAD"])).toBe(oldMaster); + expect(await readGit(repoRoot, ["rev-parse", "release/frozen"])).toBe(oldMaster); + expect(newMaster).not.toBe(oldMaster); + }); + + it.each(["forward branch", "detached commit"] as const)( + "fails closed when an operator-owned persisted worktree moves to a %s", + async (mismatchKind) => { + const repoRoot = await createTempRepo(); + const branchName = `feature/operator-owned-${mismatchKind === "forward branch" ? "forward" : "detached"}`; + const branchTip = await createBranchWithCommit(repoRoot, branchName, "operator-owned-tip.txt"); + const workspace = await realizeExistingBranch(repoRoot, branchName); + + if (mismatchKind === "forward branch") { + await runGit(workspace.cwd, ["checkout", "-b", `${branchName}-other`]); + } else { + await runGit(workspace.cwd, ["checkout", "--detach"]); + } + await fs.writeFile(path.join(workspace.cwd, "unexpected-forward-work.txt"), "do not adopt\n", "utf8"); + await runGit(workspace.cwd, ["add", "unexpected-forward-work.txt"]); + await runGit(workspace.cwd, ["commit", "-m", "Unexpected forward work"]); + const mismatchedHead = await readGit(workspace.cwd, ["rev-parse", "HEAD"]); + + await expect(restoreExistingBranch(repoRoot, workspace, { + createdByRuntime: false, + gitBranchOwnershipVersion: 1, + })).rejects.toMatchObject({ + code: "workspace_validation_failed", + resultJson: { + workspaceValidation: expect.objectContaining({ + reason: "git_worktree_not_reusable", + reasonCode: "branch_mismatch", + }), + }, + }); + + expect(await readGit(repoRoot, ["rev-parse", `refs/heads/${branchName}`])).toBe(branchTip); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(mismatchedHead); + if (mismatchKind === "detached commit") { + expect(await readGit(workspace.cwd, ["branch", "--show-current"])).toBe(""); + } + }, + ); + + 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, + gitBranchOwnershipVersion: 1, + }, + }, + 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("treats unversioned legacy ownership as operator-owned during cleanup", async () => { + const repoRoot = await createTempRepo(); + const branchTip = await createBranchWithCommit(repoRoot, "feature/legacy-owned", "legacy-owned.txt"); + const workspace = await realizeExistingBranch(repoRoot, "feature/legacy-owned"); + const cleanupInput = cleanupWorkspaceInput(workspace, repoRoot, branchTip); + + const cleanup = await cleanupExecutionWorkspaceArtifacts({ + ...cleanupInput, + workspace: { + ...cleanupInput.workspace, + metadata: { createdByRuntime: true }, + }, + }); + + 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/legacy-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"); + + const operatorOwned = await restoreExistingBranch(repoRoot, first, { createdByRuntime: false }); + expect(operatorOwned?.branchCreatedByRuntime).toBe(false); + + const legacyOwned = await restoreExistingBranch(repoRoot, first, { createdByRuntime: true }); + expect(legacyOwned?.branchCreatedByRuntime).toBe(false); + + const runtimeOwned = await restoreExistingBranch(repoRoot, first, RUNTIME_OWNED_GIT_BRANCH_METADATA); + expect(runtimeOwned?.branchCreatedByRuntime).toBe(true); + }); + + it("fails closed instead of recreating a missing branch from unversioned legacy ownership", async () => { + const repoRoot = await createTempRepo(); + const branchName = "feature/missing-operator-owned"; + await createBranchWithCommit(repoRoot, branchName, "operator-owned.txt"); + const first = await realizeExistingBranch(repoRoot, branchName); + + await runGit(repoRoot, ["worktree", "remove", "--force", first.cwd]); + await runGit(repoRoot, ["branch", "-D", branchName]); + + await expect(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, + metadata: { createdByRuntime: true }, + }, + issue: { + id: "issue-pr-prep", + identifier: "PAP-9004", + title: "Restore a missing operator-owned branch", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + })).rejects.toThrow(`operator-owned branch "${branchName}" no longer exists`); + + await expect(fs.stat(first.cwd)).rejects.toThrow(); + await expect(readGit(repoRoot, ["rev-parse", "--verify", branchName])).rejects.toThrow(); + }); + + 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"); + + await expect( + realizeExistingBranch(repoRoot, "feature/wrong-mode", { type: "project_primary" }), + ).rejects.toMatchObject({ + code: "workspace_validation_failed", + resultJson: { + workspaceValidation: expect.objectContaining({ + reason: "existing_branch_requires_git_worktree", + }), + }, + }); + }); +}); diff --git a/server/src/middleware/validate.ts b/server/src/middleware/validate.ts index 4acb53d0da..8b141fcb19 100644 --- a/server/src/middleware/validate.ts +++ b/server/src/middleware/validate.ts @@ -1,5 +1,6 @@ import type { Request, Response, NextFunction } from "express"; -import type { ZodSchema } from "zod"; +import { ZodError, type ZodIssue, type ZodSchema } from "zod"; +import { unprocessable } from "../errors.js"; export function validate(schema: ZodSchema) { return (req: Request, _res: Response, next: NextFunction) => { @@ -7,3 +8,44 @@ export function validate(schema: ZodSchema) { next(); }; } + +// The issue create/update contract requires HTTP 422 (not the generic Zod 400) +// when a request pins an invalid executionWorkspaceSettings.workspaceStrategy +// .existingBranch: bad branch syntax, placement outside isolated_workspace + +// git_worktree, or combination with branchTemplate. All three semantic checks +// report this exact path suffix, including when an issue-creating route nests +// the settings (for example accepted-plan-decomposition children). Requests +// that also fail unrelated validation keep the long-standing 400. +const EXISTING_BRANCH_SETTINGS_PATH = [ + "executionWorkspaceSettings", + "workspaceStrategy", + "existingBranch", +] as const; + +export function isExistingBranchSemanticsZodIssue(issue: Pick): boolean { + const pathOffset = issue.path.length - EXISTING_BRANCH_SETTINGS_PATH.length; + return ( + pathOffset >= 0 && + EXISTING_BRANCH_SETTINGS_PATH.every((segment, index) => issue.path[pathOffset + index] === segment) + ); +} + +export function validateIssueMutationBody(schema: ZodSchema) { + return (req: Request, _res: Response, next: NextFunction) => { + try { + req.body = schema.parse(req.body); + } catch (err) { + if ( + err instanceof ZodError && + err.issues.length > 0 && + err.issues.every(isExistingBranchSemanticsZodIssue) + ) { + // Same body shape as the generic Zod 400 response, so the + // field-specific details are preserved verbatim. + throw unprocessable("Validation error", err.issues); + } + throw err; + } + next(); + }; +} diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 7872a17749..ec1604b950 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -109,7 +109,7 @@ import { trackAgentTaskCompleted } from "@paperclipai/shared/telemetry"; import { getTelemetryClient } from "../telemetry.js"; import { isUniqueViolation } from "../db-errors.js"; import type { StorageService } from "../storage/types.js"; -import { validate } from "../middleware/validate.js"; +import { validate, validateIssueMutationBody } from "../middleware/validate.js"; import * as serviceIndex from "../services/index.js"; import { accessService, @@ -8469,7 +8469,7 @@ export function issueRoutes( res.json({ ok: true }); }); - router.post("/companies/:companyId/issues", applyCreateIssueStatusDefault, validate(createIssueSchema), async (req, res) => { + router.post("/companies/:companyId/issues", applyCreateIssueStatusDefault, validateIssueMutationBody(createIssueSchema), async (req, res) => { const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); if (isSkillTestScopedActor(req)) { @@ -8806,7 +8806,7 @@ export function issueRoutes( }); }); - router.post("/issues/:id/children", applyCreateIssueStatusDefault, validate(createChildIssueSchema), async (req, res) => { + router.post("/issues/:id/children", applyCreateIssueStatusDefault, validateIssueMutationBody(createChildIssueSchema), async (req, res) => { const parentId = req.params.id as string; const parent = await getAccessibleResource(req, res, svc.getById(parentId), "Parent issue not found"); if (!parent) return; @@ -8988,7 +8988,7 @@ export function issueRoutes( res.json(decompositions); }); - router.post("/issues/:id/accepted-plan-decompositions", validate(createAcceptedPlanDecompositionSchema), async (req, res) => { + router.post("/issues/:id/accepted-plan-decompositions", validateIssueMutationBody(createAcceptedPlanDecompositionSchema), async (req, res) => { const sourceIssueId = req.params.id as string; const sourceIssue = await getAccessibleResource(req, res, svc.getById(sourceIssueId), "Issue not found"); if (!sourceIssue) return; @@ -9357,7 +9357,7 @@ export function issueRoutes( }, ); - router.patch("/issues/:id", validate(updateIssueRouteSchema), async (req, res) => { + router.patch("/issues/:id", validateIssueMutationBody(updateIssueRouteSchema), async (req, res) => { const id = req.params.id as string; const existing = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); if (!existing) return; diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 49c9699544..690c4883e3 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -2249,7 +2249,7 @@ registry.registerPath({ params: z.object({ companyId: z.string() }), body: jsonBody(createIssueSchema), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 422: r.unprocessable }, }); registry.registerPath({ @@ -2270,7 +2270,7 @@ registry.registerPath({ params: z.object({ id: z.string() }), body: jsonBody(updateIssueSchema.partial()), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound, 422: r.unprocessable }, }); registry.registerPath({ @@ -2904,7 +2904,7 @@ registry.registerPath({ params: z.object({ id: z.string() }), body: jsonBody(runRoutineSchema), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 422: r.unprocessable }, }); registry.registerPath({ @@ -4809,7 +4809,7 @@ registry.registerPath({ tags: ["issues"], summary: "Create child issues", request: { params: z.object({ id: z.string() }), body: jsonBody(createChildIssueSchema) }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 422: r.unprocessable }, }); registry.registerPath({ diff --git a/server/src/routes/projects.ts b/server/src/routes/projects.ts index 50b4be2785..c01a028ddb 100644 --- a/server/src/routes/projects.ts +++ b/server/src/routes/projects.ts @@ -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: {}, diff --git a/server/src/routes/routines.ts b/server/src/routes/routines.ts index a9f0e7eee6..c8c94335dd 100644 --- a/server/src/routes/routines.ts +++ b/server/src/routes/routines.ts @@ -12,7 +12,7 @@ import { updateRoutineTriggerSchema, } from "@paperclipai/shared"; import { trackRoutineCreated } from "@paperclipai/shared/telemetry"; -import { validate } from "../middleware/validate.js"; +import { validate, validateIssueMutationBody } from "../middleware/validate.js"; import { accessService, documentAnnotationService, logActivity, routineService } from "../services/index.js"; import { assertCompanyAccess, getAccessibleResource, getActorInfo, hasCompanyAccess } from "./authz.js"; import { forbidden, unauthorized } from "../errors.js"; @@ -626,7 +626,7 @@ export function routineRoutes( }, ); - router.post("/routines/:id/run", validate(runRoutineSchema), async (req, res) => { + router.post("/routines/:id/run", validateIssueMutationBody(runRoutineSchema), async (req, res) => { const routine = await assertCanManageExistingRoutine(req, req.params.id as string); if (!routine) { res.status(404).json({ error: "Routine not found" }); diff --git a/server/src/services/execution-workspace-branch-ownership.ts b/server/src/services/execution-workspace-branch-ownership.ts new file mode 100644 index 0000000000..8dda9c711e --- /dev/null +++ b/server/src/services/execution-workspace-branch-ownership.ts @@ -0,0 +1,14 @@ +export const GIT_BRANCH_OWNERSHIP_METADATA_VERSION = 1; +export const GIT_BRANCH_OWNERSHIP_METADATA_KEY = "gitBranchOwnershipVersion"; + +function hasCurrentGitBranchOwnershipMetadata( + metadata: Record | null | undefined, +) { + return metadata?.[GIT_BRANCH_OWNERSHIP_METADATA_KEY] === GIT_BRANCH_OWNERSHIP_METADATA_VERSION; +} + +export function isRuntimeOwnedGitBranch( + metadata: Record | null | undefined, +) { + return hasCurrentGitBranchOwnershipMetadata(metadata) && metadata?.createdByRuntime === true; +} diff --git a/server/src/services/execution-workspace-policy.ts b/server/src/services/execution-workspace-policy.ts index 1c6b3ccc93..0205950fc1 100644 --- a/server/src/services/execution-workspace-policy.ts +++ b/server/src/services/execution-workspace-policy.ts @@ -40,6 +40,9 @@ function parseExecutionWorkspaceStrategy(raw: unknown): ExecutionWorkspaceStrate type, ...(typeof parsed.baseRef === "string" ? { baseRef: parsed.baseRef } : {}), ...(typeof parsed.branchTemplate === "string" ? { branchTemplate: parsed.branchTemplate } : {}), + ...(typeof parsed.existingBranch === "string" && parsed.existingBranch.trim().length > 0 + ? { existingBranch: parsed.existingBranch.trim() } + : {}), ...(typeof parsed.worktreeParentDir === "string" ? { worktreeParentDir: parsed.worktreeParentDir } : {}), ...(typeof parsed.provisionCommand === "string" ? { provisionCommand: parsed.provisionCommand } : {}), ...(typeof parsed.runtimeProvisionCommand === "string" diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 54b0a562f1..b9a635346c 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -55,6 +55,7 @@ import { visibleIssueCondition } from "./issue-visibility.js"; import { createGitRemoteAuthProvider } from "./git-credentials.js"; import { readProjectWorkspaceRuntimeConfig } from "./project-workspace-runtime-config.js"; import { workspaceGitOperationScheduler } from "./workspace-git-operation-scheduler.js"; +import { isRuntimeOwnedGitBranch } from "./execution-workspace-branch-ownership.js"; import { listCurrentRuntimeServicesForExecutionWorkspaces, listCurrentRuntimeServicesForProjectWorkspaces, @@ -789,7 +790,9 @@ async function inspectGitCloseReadiness(workspace: ExecutionWorkspace): Promise< }> { const warnings: string[] = []; const workspacePath = readNullableString(workspace.providerRef) ?? readNullableString(workspace.cwd); - const createdByRuntime = workspace.metadata?.createdByRuntime === true; + const createdByRuntime = workspace.providerType === "git_worktree" + ? isRuntimeOwnedGitBranch(workspace.metadata) + : workspace.metadata?.createdByRuntime === true; const expectsGitInspection = workspace.providerType === "git_worktree" || Boolean(workspace.repoUrl || workspace.baseRef || workspace.branchName || workspacePath); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index e2ef031a56..dd8ba788f3 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -178,6 +178,11 @@ import { } from "./issue-continuation-summary.js"; import { buildDocumentReviewContext, buildPlanReviewContext } from "./plan-review-context.js"; import { executionWorkspaceService, mergeExecutionWorkspaceConfig } from "./execution-workspaces.js"; +import { + GIT_BRANCH_OWNERSHIP_METADATA_KEY, + GIT_BRANCH_OWNERSHIP_METADATA_VERSION, + isRuntimeOwnedGitBranch, +} from "./execution-workspace-branch-ownership.js"; import { workspaceOperationService, type WorkspaceOperationRecorder } from "./workspace-operations.js"; import { isProcessGroupAlive, terminateLocalService } from "./local-service-supervisor.js"; import { @@ -1426,6 +1431,7 @@ export function mergeExecutionWorkspaceMetadataForPersistence(input: { existingMetadata: Record | null | undefined; source: string; createdByRuntime: boolean; + strategyType: "project_primary" | "git_worktree"; configSnapshot: Record | null; shouldReuseExisting: boolean; shouldRefreshConfigSnapshot?: boolean; @@ -1438,6 +1444,11 @@ export function mergeExecutionWorkspaceMetadataForPersistence(input: { source: input.source, createdByRuntime: input.createdByRuntime, } as Record; + if (input.strategyType === "git_worktree") { + base[GIT_BRANCH_OWNERSHIP_METADATA_KEY] = GIT_BRANCH_OWNERSHIP_METADATA_VERSION; + } else { + delete base[GIT_BRANCH_OWNERSHIP_METADATA_KEY]; + } const existingSnapshot = parseObject(base.baseRefSnapshot); if ( @@ -1467,6 +1478,12 @@ export function mergeExecutionWorkspaceMetadataForPersistence(input: { return mergeExecutionWorkspaceConfig(base, input.configSnapshot); } +export function resolveExecutionWorkspaceBranchOwnership( + executionWorkspace: Pick, +) { + return executionWorkspace.branchCreatedByRuntime; +} + export function stripWorkspaceRuntimeFromExecutionRunConfig(config: Record) { const nextConfig = { ...config }; delete nextConfig.workspaceRuntime; @@ -4444,10 +4461,22 @@ export function resolveExecutionWorkspaceReuseRequestForIssue(input: { issueExecutionWorkspaceId?: string | null; issueExecutionWorkspacePreference?: string | null; existingExecutionWorkspaceStatus?: string | null; + requestedExistingBranch?: string | null; + existingExecutionWorkspaceBranchName?: string | null; }): ExecutionWorkspaceReuseRequestForIssue { const requestedExecutionWorkspaceId = readNonEmptyString(input.issueExecutionWorkspaceId); + // An explicitly pinned existing branch outranks an inherited reuse_existing + // binding: a persisted workspace on any other branch (or with no recorded + // branch) is stale for this issue, so dispatch realizes the pinned branch + // instead of restoring the mismatched workspace. + const requestedExistingBranch = readNonEmptyString(input.requestedExistingBranch); + const existingWorkspaceMatchesRequestedBranch = + requestedExistingBranch === null || + readNonEmptyString(input.existingExecutionWorkspaceBranchName) === requestedExistingBranch; const requestedShouldReuseExisting = - input.issueExecutionWorkspacePreference === "reuse_existing" && requestedExecutionWorkspaceId !== null; + input.issueExecutionWorkspacePreference === "reuse_existing" && + requestedExecutionWorkspaceId !== null && + existingWorkspaceMatchesRequestedBranch; return { requestedExecutionWorkspaceId, @@ -14478,6 +14507,8 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) issueExecutionWorkspaceId: requestedExecutionWorkspaceId, issueExecutionWorkspacePreference: issueRef?.executionWorkspacePreference ?? null, existingExecutionWorkspaceStatus: existingExecutionWorkspace?.status ?? null, + requestedExistingBranch: issueExecutionWorkspaceSettings?.workspaceStrategy?.existingBranch ?? null, + existingExecutionWorkspaceBranchName: existingExecutionWorkspace?.branchName ?? null, }); const requestedShouldReuseExisting = workspaceReuseRequest.requestedShouldReuseExisting; const reusableExistingExecutionWorkspace = workspaceReuseRequest.existingExecutionWorkspaceAvailable @@ -15053,6 +15084,16 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) name: agent.name, companyId: agent.companyId, }, + recordedBranchOwnership: + existingExecutionWorkspace?.status !== "archived" + && existingExecutionWorkspace?.branchName + ? { + branchName: existingExecutionWorkspace.branchName, + createdByRuntime: isRuntimeOwnedGitBranch( + existingExecutionWorkspace.metadata, + ), + } + : null, heartbeatRunId: run.id, enableWorkspaceBranchReconcileForward: resolvedInstanceSettings.experimental.enableWorkspaceBranchReconcileForward, @@ -15070,7 +15111,11 @@ 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: resolveExecutionWorkspaceBranchOwnership(executionWorkspace), + strategyType: executionWorkspace.strategy, configSnapshot, shouldReuseExisting: resolvedWorkspaceReusePolicy.shouldRestoreExistingWorkspace, shouldRefreshConfigSnapshot: resolvedWorkspaceReusePolicy.shouldRefreshWorkspaceConfigSnapshot, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 58092a36b1..39d2f2545b 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -338,6 +338,7 @@ function buildPreRealizationExecutionWorkspaceSettings(raw: unknown): Record; issue: ExecutionWorkspaceIssueRef | null; agent: ExecutionWorkspaceAgentRef; + recordedBranchOwnership?: { + branchName: string; + createdByRuntime: boolean; + } | null; heartbeatRunId?: string | null; enableWorkspaceBranchReconcileForward?: boolean; enableWorkspaceDirtyQuarantineRepair?: boolean; @@ -3196,7 +3208,20 @@ export async function realizeExecutionWorkspace(input: { }): Promise { const rawStrategy = parseObject(input.config.workspaceStrategy); const strategyType = asString(rawStrategy.type, "project_primary"); + const requestedExistingBranch = asString(rawStrategy.existingBranch, "").trim(); if (strategyType !== "git_worktree") { + if (requestedExistingBranch) { + throw new WorkspaceRuntimeValidationFailure( + `Workspace strategy pins existing branch "${requestedExistingBranch}" but has type "${strategyType}"; an exact-branch workspace requires strategy type "git_worktree". Set workspaceStrategy.type to "git_worktree" or remove existingBranch.`, + { + workspaceValidation: { + reason: "existing_branch_requires_git_worktree", + requestedExistingBranch, + strategyType, + }, + }, + ); + } return { ...input.base, strategy: "project_primary", @@ -3205,24 +3230,62 @@ export async function realizeExecutionWorkspace(input: { worktreePath: null, warnings: [], created: false, + branchCreatedByRuntime: false, baseRefSha: null, }; } const repoRoot = await resolveGitOwnerRepoRoot(input.base.baseCwd); - const branchTemplate = asString(rawStrategy.branchTemplate, "{{issue.identifier}}-{{slug}}"); - const renderedBranch = renderWorkspaceTemplate(branchTemplate, { - issue: input.issue, - agent: input.agent, - projectId: input.base.projectId, - repoRef: input.base.repoRef, - }); - let branchName = sanitizeBranchName(renderedBranch); + let branchName: string; + if (requestedExistingBranch) { + // Exact-branch mode: attach the requested pre-existing branch verbatim. + // The branch must already exist; realization never creates, renames, or + // resets it, and any mismatch below fails closed instead of falling back + // to a derived branch or the shared checkout. + const existingBranchSha = await runGit( + ["rev-parse", "--verify", "--quiet", `refs/heads/${requestedExistingBranch}`], + repoRoot, + ).catch(() => null); + if (!existingBranchSha) { + throw new WorkspaceRuntimeValidationFailure( + `Workspace strategy pins existing branch "${requestedExistingBranch}", but no local branch with that name exists in "${repoRoot}". Create or fetch the branch first, or remove workspaceStrategy.existingBranch; exact-branch realization never creates a branch.`, + { + workspaceValidation: { + reason: "existing_branch_not_found", + requestedExistingBranch, + repoRoot, + }, + }, + ); + } + branchName = requestedExistingBranch; + } else { + const branchTemplate = asString(rawStrategy.branchTemplate, "{{issue.identifier}}-{{slug}}"); + const renderedBranch = renderWorkspaceTemplate(branchTemplate, { + issue: input.issue, + agent: input.agent, + projectId: input.base.projectId, + repoRef: input.base.repoRef, + }); + branchName = sanitizeBranchName(renderedBranch); + } const configuredParentDir = asString(rawStrategy.worktreeParentDir, ""); const worktreeParentDir = configuredParentDir ? resolveConfiguredPath(configuredParentDir, repoRoot) : path.join(repoRoot, ".paperclip", "worktrees"); const worktreePath = path.join(worktreeParentDir, branchName); + if (path.relative(worktreeParentDir, worktreePath).startsWith("..")) { + throw new WorkspaceRuntimeValidationFailure( + `Workspace branch "${branchName}" resolves to a worktree path outside the managed worktree parent directory "${worktreeParentDir}".`, + { + workspaceValidation: { + reason: "worktree_path_escapes_parent_dir", + branchName, + worktreeParentDir, + }, + }, + ); + } let pendingForwardBranchReconcile: PendingForwardBranchReconcile | null = null; const configuredBaseRef = typeof rawStrategy.baseRef === "string" && rawStrategy.baseRef.length > 0 ? rawStrategy.baseRef @@ -3245,7 +3308,9 @@ export async function realizeExecutionWorkspace(input: { await fs.mkdir(worktreeParentDir, { recursive: true }); async function reuseExistingWorktree(reusablePath: string, effectiveBranchName = branchName, extraWarnings: string[] = []) { - const refresh = currentBaseRefSha + // An exact-branch attach must never move the requested branch, so skip + // the unstarted-worktree fast-forward that template-derived reuse gets. + const refresh = currentBaseRefSha && !requestedExistingBranch ? await refreshUnstartedWorktreeToBase({ repoRoot, worktreePath: reusablePath, @@ -3304,6 +3369,15 @@ export async function realizeExecutionWorkspace(input: { worktreePath: reusablePath, warnings: [...extraWarnings, ...baseRefreshWarnings, ...baseDrift.warnings], created: false, + // A fresh realization may still land on the worktree recorded by a + // previous heartbeat. Preserve that branch's ownership only when the + // recorded branch matches the checkout being reused. Exact-branch mode + // remains operator-owned by contract; every mismatch likewise fails + // safe and leaves the branch behind during terminal cleanup. + branchCreatedByRuntime: + !requestedExistingBranch + && input.recordedBranchOwnership?.branchName === effectiveBranchName + && input.recordedBranchOwnership.createdByRuntime === true, baseRefSha: refresh.baseRefSha ?? baseDrift.branchBaseRefSha ?? baseDrift.currentBaseRefSha, pendingForwardBranchReconcile, }; @@ -3316,6 +3390,11 @@ export async function realizeExecutionWorkspace(input: { expectedBranchName: branchName, }).catch(() => null); if (validation && !validation.valid && validation.reasonCode === "branch_mismatch") { + if (requestedExistingBranch) { + // Exact-branch mode never reconciles a mismatched checkout onto + // another branch; the caller fails closed with the mismatch reason. + return { validation, branchName, warnings: [] }; + } const coherence = await ensureGitWorktreeBranchCoherent({ db: input.db ?? null, repoRoot, @@ -3357,6 +3436,19 @@ export async function realizeExecutionWorkspace(input: { } const validation = reusable.validation; const reason = validation && !validation.valid ? ` (${validation.reason})` : ""; + if (requestedExistingBranch) { + throw new WorkspaceRuntimeValidationFailure( + `Workspace strategy pins existing branch "${requestedExistingBranch}", but the worktree path "${worktreePath}" already exists and is not a reusable checkout of that branch${reason}. Repair or remove that worktree, then retry; exact-branch realization never reconciles it onto another branch.`, + { + workspaceValidation: { + reason: "existing_branch_worktree_not_reusable", + reasonCode: validation && !validation.valid ? validation.reasonCode : null, + requestedExistingBranch, + worktreePath, + }, + }, + ); + } throw new Error(`Configured worktree path "${worktreePath}" already exists and is not a reusable git worktree${reason}.`); } @@ -3368,9 +3460,80 @@ export async function realizeExecutionWorkspace(input: { } const validation = reusable.validation; const reason = validation && !validation.valid ? ` (${validation.reason})` : ""; + if (requestedExistingBranch) { + throw new WorkspaceRuntimeValidationFailure( + `Workspace strategy pins existing branch "${requestedExistingBranch}", which is already checked out at "${registeredBranchWorktree}", but that worktree is not reusable${reason}. Repair or remove that worktree, then retry.`, + { + workspaceValidation: { + reason: "existing_branch_worktree_not_reusable", + reasonCode: validation && !validation.valid ? validation.reasonCode : null, + requestedExistingBranch, + worktreePath: registeredBranchWorktree, + }, + }, + ); + } throw new Error(`Registered worktree for branch "${branchName}" at "${registeredBranchWorktree}" is not reusable${reason}.`); } + if (requestedExistingBranch) { + try { + await recordGitOperation(input.recorder, { + phase: "worktree_prepare", + args: ["worktree", "add", worktreePath, branchName], + cwd: repoRoot, + metadata: { + repoRoot, + worktreePath, + branchName, + baseRef, + baseRefSha: currentBaseRefSha, + created: false, + attachedExistingBranch: true, + }, + successMessage: `Attached existing branch ${branchName} at ${worktreePath}\n`, + failureLabel: `git worktree add ${worktreePath}`, + }); + } catch (attachError) { + const message = attachError instanceof Error ? attachError.message : String(attachError); + throw new WorkspaceRuntimeValidationFailure( + `Could not attach existing branch "${requestedExistingBranch}" as a git worktree at "${worktreePath}": ${message}`, + { + workspaceValidation: { + reason: "existing_branch_attach_failed", + requestedExistingBranch, + worktreePath, + }, + }, + ); + } + await provisionExecutionWorktree({ + strategy: rawStrategy, + base: input.base, + repoRoot, + worktreePath, + branchName, + issue: input.issue, + agent: input.agent, + created: true, + recorder: input.recorder ?? null, + }); + return { + ...input.base, + repoRef: baseRef, + strategy: "git_worktree", + cwd: worktreePath, + 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, + }; + } + // No reusable worktree exists, so a fresh `git worktree add -b ` // must run next. An unresolved base ref would make git fail with // `fatal: invalid reference`. Stop here instead and raise a pre-dispatch @@ -3384,6 +3547,7 @@ export async function realizeExecutionWorkspace(input: { }); } + let branchCreatedByRuntime = true; try { await recordGitOperation(input.recorder, { phase: "worktree_prepare", @@ -3421,6 +3585,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; @@ -3453,6 +3620,7 @@ export async function realizeExecutionWorkspace(input: { worktreePath, warnings: baseRefreshWarnings, created: true, + branchCreatedByRuntime, baseRefSha: currentBaseRefSha, }; } @@ -3503,6 +3671,11 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { worktreePath: strategy === "git_worktree" ? (input.workspace.providerRef ?? cwd) : null, warnings: [], created: false, + // Only the versioned ownership record introduced with branch-level + // ownership semantics can authorize branch recreation or deletion. Older + // createdByRuntime=true rows described worktree ownership, so trusting + // them here could recreate or later delete an operator-owned branch. + branchCreatedByRuntime: isRuntimeOwnedGitBranch(input.workspace.metadata), baseRefSha: readRecordedBaseRefSha(input.workspace.metadata), }; const provisionCommand = asString(input.workspace.config?.provisionCommand, "").trim(); @@ -3536,7 +3709,12 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { const reuseBaseRef = input.workspace.baseRef ?? input.base.repoRef ?? null; const reuseWorktreePath = realized.worktreePath ?? cwd; const repairWarnings: string[] = []; - if (await isGitCheckout(reuseWorktreePath)) { + if (await isGitCheckout(reuseWorktreePath) && realized.branchCreatedByRuntime) { + // Branch-coherence repair may check out another branch, adopt a forward + // branch, or move the recorded ref from a detached HEAD. Those repairs + // are valid only for a branch that this runtime created. An attached + // operator-owned branch must retain its exact identity and tip; the + // validation below rejects any mismatch without mutating Git state. const coherence = await ensureGitWorktreeBranchCoherent({ db: input.db ?? null, repoRoot, @@ -3581,7 +3759,9 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { ? await refreshRemoteTrackingBaseRef(repoRoot, reuseBaseRef, input.resolveGitAuth) : []; const currentBaseRefSha = reuseBaseRef ? await resolveBaseRefSha(repoRoot, reuseBaseRef) : null; - const refresh = reuseBaseRef && currentBaseRefSha + // An unstarted-worktree refresh can fast-forward the checked-out branch. + // Never run it for an attached operator-owned ref. + const refresh = realized.branchCreatedByRuntime && reuseBaseRef && currentBaseRefSha ? await refreshUnstartedWorktreeToBase({ repoRoot, worktreePath: reuseWorktreePath, @@ -3658,6 +3838,11 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { ) { throw error; } + if (!realized.branchCreatedByRuntime) { + throw new Error( + `Execution workspace "${worktreePath}" cannot be restored because its operator-owned branch "${branchName}" no longer exists.`, + ); + } const baseRef = input.workspace.baseRef ?? await detectDefaultBranch(repoRoot) ?? "HEAD"; const recreatedBaseRefSha = await resolveBaseRefSha(repoRoot, baseRef); await recordGitOperation(input.recorder, { @@ -3709,6 +3894,7 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { worktreePath, warnings: [...restoreRefreshWarnings, ...baseDrift.warnings], created, + branchCreatedByRuntime: realized.branchCreatedByRuntime || created, baseRefSha: recordedBaseRefSha ?? (created ? restoreCurrentBaseRefSha : baseDrift.branchBaseRefSha) @@ -3869,7 +4055,12 @@ export async function cleanupExecutionWorkspaceArtifacts(input: { warnings.push(`Could not read worktree instance pointer: ${err instanceof Error ? err.message : String(err)}`); } } + // Local-directory ownership keeps the historical createdByRuntime signal. + // Git branch deletion additionally requires the version marker introduced + // with branch-level ownership semantics. Unmarked legacy rows fail closed: + // their worktrees are removable, but their branch refs are operator-owned. const createdByRuntime = input.workspace.metadata?.createdByRuntime === true; + const branchCreatedByRuntime = isRuntimeOwnedGitBranch(input.workspace.metadata); const cleanupCommands = input.runCleanupCommands === false ? [] : [ @@ -3956,7 +4147,7 @@ export async function cleanupExecutionWorkspaceArtifacts(input: { } } } - if (createdByRuntime && input.workspace.branchName) { + if (branchCreatedByRuntime && input.workspace.branchName) { if (!repoRoot) { warnings.push(`Could not resolve git repo root to delete branch "${input.workspace.branchName}".`); } else { @@ -8233,6 +8424,7 @@ export async function restartDesiredRuntimeServicesOnStartup(db: Db) { worktreePath: null, warnings: [], created: false, + branchCreatedByRuntime: false, }, config: { workspaceRuntime: runtimeConfig.workspaceRuntime, @@ -8287,6 +8479,9 @@ export async function restartDesiredRuntimeServicesOnStartup(db: Db) { worktreePath: row.strategyType === "git_worktree" ? row.cwd : null, warnings: [], created: false, + branchCreatedByRuntime: isRuntimeOwnedGitBranch( + row.metadata as Record | null, + ), }, executionWorkspaceId: row.id, config: {