From 215219a5cbe0e8fe32dfa6bbe409684ebd44f074 Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 9 Oct 2026 18:07:37 -0500 Subject: [PATCH] fix: enforce explicit workspace choices across update paths Fail closed for an unavailable configured source, invalidate superseded workspace intent atomically on ordinary task updates, and reuse canonical workspace eligibility in channel settings. Co-Authored-By: Paperclip --- .../__tests__/task-workspace-binding.test.ts | 87 ++++++++++++++++++- server/src/services/execution-workspaces.ts | 33 ++++++- server/src/services/heartbeat/workspaces.ts | 16 +++- server/src/services/issues.ts | 6 ++ ui/src/api/execution-workspaces.test.ts | 7 ++ ui/src/api/execution-workspaces.ts | 2 + .../apps/chat/ChatExecutionDefaults.test.tsx | 18 ++++ .../pages/apps/chat/ChatExecutionDefaults.tsx | 2 +- 8 files changed, 163 insertions(+), 8 deletions(-) diff --git a/server/src/__tests__/task-workspace-binding.test.ts b/server/src/__tests__/task-workspace-binding.test.ts index 368e37b236..5f7277e5b7 100644 --- a/server/src/__tests__/task-workspace-binding.test.ts +++ b/server/src/__tests__/task-workspace-binding.test.ts @@ -8,11 +8,12 @@ import path from "node:path"; import { randomUUID } from "node:crypto"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { eq } from "drizzle-orm"; -import { agents, companies, createDb, executionWorkspaces, issues, projects } from "@paperclipai/db"; +import { agents, companies, createDb, executionWorkspaces, heartbeatRuns, issues, projects, projectWorkspaces } from "@paperclipai/db"; import { executionWorkspaceService } from "../services/execution-workspaces.js"; import { canActorReadExecutionWorkspace } from "../services/authorization.js"; import { deleteCompany } from "../services/company-deletion.js"; import { issueService } from "../services/issues.js"; +import { instanceSettingsService } from "../services/instance-settings.js"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; const support = await getEmbeddedPostgresTestSupport(); @@ -68,6 +69,50 @@ const support = await getEmbeddedPostgresTestSupport(); await expect(svc.selectTaskWorkspace({ ...request, requestKey: "stale" })).rejects.toThrow("binding changed"); }); + it.each(["binding", "configured_source"] as const)("supersedes an older queued selection when an ordinary update changes %s", async kind => { + const f = await fixture(), svc = executionWorkspaceService(db), tasks = issueService(db); + const settings = instanceSettingsService(db), previous = await settings.getExperimental(); + await settings.updateExperimental({ enableIsolatedWorkspaces: true }); + try { + await svc.bindTaskWorkspace(f.companyId, f.issueId, f.workspaceId); + const request = { ...f, selection: { kind: "task_directory" as const }, expectedBindingRevision: 1, requestKey: "older-choice" }; + await svc.selectTaskWorkspace(request); + const [project] = await db.insert(projects).values({ companyId: f.companyId, name: "New source" }).returning(); + const [source] = await db.insert(projectWorkspaces).values({ companyId: f.companyId, projectId: project.id, name: "New files", cwd: `/tmp/new-source-${f.issueId}` }).returning(); + const [workspace] = await db.insert(executionWorkspaces).values({ companyId: f.companyId, projectId: null, + mode: "shared_workspace", strategyType: "project_primary", name: "New binding", cwd: `/tmp/new-binding-${f.issueId}` }).returning(); + const patch = kind === "binding" ? { executionWorkspaceId: workspace.id } : { executionWorkspaceId: null, projectWorkspaceId: source.id }; + await tasks.update(f.issueId, patch); + expect(await svc.applyPendingTaskWorkspaceSelection({ ...f, runId: randomUUID() })).toBe(false); + const view = await svc.inspectTaskWorkspace(f.companyId, f.issueId, f.actor); + expect(view).toMatchObject({ bindingRevision: 2, pendingSelection: null, selection: null }); + expect(view.workspace?.id ?? null).toBe(kind === "binding" ? workspace.id : null); + const [task] = await db.select().from(issues).where(eq(issues.id, f.issueId)); + expect(task.projectWorkspaceId).toBe(kind === "configured_source" ? source.id : null); + await expect(svc.selectTaskWorkspace(request)).rejects.toThrow("binding changed"); + await tasks.update(f.issueId, { projectId: project.id, ...patch }); + expect((await svc.inspectTaskWorkspace(f.companyId, f.issueId, f.actor)).bindingRevision).toBe(2); + } finally { await settings.updateExperimental({ enableIsolatedWorkspaces: previous.enableIsolatedWorkspaces }); } + }); + + it("rejects an ordinary source annotation on bound files and preserves active run ownership", async () => { + const f = await fixture(), svc = executionWorkspaceService(db), tasks = issueService(db); + await svc.bindTaskWorkspace(f.companyId, f.issueId, f.workspaceId); + const [project] = await db.insert(projects).values({ companyId: f.companyId, name: "Other source" }).returning(); + const [source] = await db.insert(projectWorkspaces).values({ companyId: f.companyId, projectId: project.id, name: "Other files", cwd: `/tmp/other-${f.issueId}` }).returning(); + await expect(tasks.update(f.issueId, { projectWorkspaceId: source.id })).rejects.toThrow("already has bound files"); + const [agent] = await db.insert(agents).values({ companyId: f.companyId, name: "Running worker", adapterType: "process", status: "active" }).returning(); + await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: agent.id, status: "running", invocationSource: "on_demand", nativeIssueId: f.issueId, contextSnapshot: { issueId: f.issueId } }); + const settings = instanceSettingsService(db), previous = await settings.getExperimental(); + await settings.updateExperimental({ enableIsolatedWorkspaces: true }); + try { + await expect(tasks.update(f.issueId, { executionWorkspaceId: null, projectWorkspaceId: source.id })).rejects.toThrow("still owns its workspace"); + expect(await svc.inspectTaskWorkspace(f.companyId, f.issueId, f.actor)).toMatchObject({ bindingRevision: 1, workspace: { id: f.workspaceId } }); + await expect(svc.selectTaskWorkspace({ ...f, selection: { kind: "configured_source", projectWorkspaceId: source.id, mode: "shared" }, + expectedBindingRevision: 1, requestKey: "next-admission" })).resolves.toMatchObject({ kind: "scheduled" }); + } finally { await settings.updateExperimental({ enableIsolatedWorkspaces: previous.enableIsolatedWorkspaces }); } + }); + it("defaults app and chat tasks to independent directories without hydrating legacy agent files", async () => { const f = await fixture(); const root = await fs.mkdtemp(path.join(os.tmpdir(), "task-workspace-default-")); @@ -105,6 +150,46 @@ const support = await getEmbeddedPostgresTestSupport(); } finally { vi.unstubAllEnvs(); await fs.rm(root, { recursive: true, force: true }); } }); + it.each(["missing_path", "clone_failure", "deleted_source"] as const)("refuses sibling and session fallbacks for an explicit configured source: %s", async (failure) => { + const f = await fixture(); + const root = await fs.mkdtemp(path.join(os.tmpdir(), "task-source-selection-")); + vi.stubEnv("PAPERCLIP_HOME", root); + try { + const projectId = randomUUID(), sourceId = randomUUID(), siblingId = randomUUID(); + const siblingCwd = path.join(root, "usable-sibling"); + await fs.mkdir(siblingCwd); + await fs.writeFile(path.join(siblingCwd, "source.txt"), "Other repository"); + await db.insert(projects).values({ id: projectId, companyId: f.companyId, name: "Multiple sources" }); + await db.insert(projectWorkspaces).values([ + { id: sourceId, companyId: f.companyId, projectId, name: "Selected", isPrimary: true, + sourceType: failure === "clone_failure" ? "git_repo" : "local_path", + cwd: failure === "clone_failure" ? null : path.join(root, "missing-source"), + // A nonexistent local remote exercises clone failure without network access. + repoUrl: failure === "clone_failure" ? path.join(root, "missing-remote.git") : null }, + { id: siblingId, companyId: f.companyId, projectId, name: "Other", sourceType: "local_path", cwd: siblingCwd }, + ]); + await db.update(issues).set({ projectId, projectWorkspaceId: sourceId, + workspaceSelection: { version: 1, source: "explicit", selection: { kind: "configured_source", projectWorkspaceId: sourceId, mode: "shared" } }, + }).where(eq(issues.id, f.issueId)); + if (failure === "deleted_source") await db.delete(projectWorkspaces).where(eq(projectWorkspaces.id, sourceId)); + const [agent] = await db.insert(agents).values({ companyId: f.companyId, name: "Worker", adapterType: "process", + adapterConfig: { cwd: siblingCwd } }).returning(); + await expect(createHeartbeatWorkspaceResolver(db).resolveWorkspaceForRun(agent!, { issueId: f.issueId }, { cwd: siblingCwd })) + .rejects.toMatchObject({ code: "workspace_validation_failed", resultJson: { workspaceValidation: { + reason: failure === "clone_failure" ? "git_worktree_base_materialization_failed" : "configured_workspace_unavailable", + issueProjectWorkspaceId: sourceId, baseCwdFallback: false, + materializationFailures: failure === "clone_failure" ? [expect.objectContaining({ projectWorkspaceId: sourceId })] : [], + } } }); + expect(await fs.readFile(path.join(siblingCwd, "source.txt"), "utf8")).toBe("Other repository"); + expect((await db.select().from(issues).where(eq(issues.id, f.issueId)))[0].executionWorkspaceId).toBeNull(); + if (failure === "missing_path") { + await fs.mkdir(path.join(root, "missing-source")); + const repaired = await createHeartbeatWorkspaceResolver(db).resolveWorkspaceForRun(agent!, { issueId: f.issueId }, { cwd: siblingCwd }); + expect(repaired).toMatchObject({ workspaceId: sourceId, cwd: path.join(root, "missing-source"), baseCwdFallback: false }); + } + } finally { vi.unstubAllEnvs(); await fs.rm(root, { recursive: true, force: true }); } + }); + it("retains every task privacy source when simultaneous tasks bind the same folder", async () => { const f = await fixture(), secondIssueId = randomUUID(), svc = executionWorkspaceService(db); await db.insert(issues).values({ id: secondIssueId, companyId: f.companyId, title: "Second task" }); diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index a654ee140d..e9b1757070 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -11,7 +11,7 @@ import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; -import { promisify } from "node:util"; +import { isDeepStrictEqual, promisify } from "node:util"; import { and, asc, desc, eq, gt, inArray, isNull, lte, ne, or, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { @@ -1401,6 +1401,35 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic }); } + /** Ordinary issue edits participate in the same binding revision transaction. */ + async function prepareTaskWorkspaceUpdate(input: { + companyId: string; issueId: string; patch: TaskWorkspaceBindingPatch; + }, tx: DbTransaction) { + const [task] = await tx.select().from(issues).where(and( + eq(issues.id, input.issueId), eq(issues.companyId, input.companyId), + )).for("no key update"); + if (!task) throw notFound("Task not found"); + const fields = ["executionWorkspaceId", "projectWorkspaceId", "executionWorkspacePreference", "executionWorkspaceSettings"] as const; + if (!fields.some(field => input.patch[field] !== undefined && !isDeepStrictEqual(input.patch[field], task[field]))) return {}; + if (input.patch.projectWorkspaceId !== undefined && input.patch.projectWorkspaceId !== task.projectWorkspaceId && + task.executionWorkspaceId && input.patch.executionWorkspaceId !== null) { + throw conflict("Task already has bound files; schedule a workspace selection or explicitly clear its binding when changing the source"); + } + const [holder] = await tx.select({ id: heartbeatRuns.id }).from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, input.companyId), eq(heartbeatRuns.status, "running"), + or(eq(heartbeatRuns.nativeIssueId, input.issueId), + sql`coalesce(${heartbeatRuns.contextSnapshot}->>'issueId', ${heartbeatRuns.contextSnapshot}->>'taskId') = ${input.issueId}`, + task.executionRunId ? eq(heartbeatRuns.id, task.executionRunId) : undefined), + )).limit(1); + const [unsettled] = await tx.select({ runId: nativeRunFinalizations.runId }).from(nativeRunFinalizations).where(and( + eq(nativeRunFinalizations.companyId, input.companyId), eq(nativeRunFinalizations.issueId, input.issueId), + ne(nativeRunFinalizations.phase, "committed"), + )).limit(1); + if (holder || unsettled) throw conflict("Task run still owns its workspace; schedule a workspace selection for the next admission"); + return { workspaceBindingRevision: task.workspaceBindingRevision + 1, + workspacePendingSelection: null, workspaceSelection: null }; + } + const inspectDisplay = opts.inspectGitCloseReadiness ? createWorkspaceGitInspectionCache(opts.inspectGitCloseReadiness) : inspectGitForDisplay; @@ -1999,7 +2028,7 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic requestTaskRepository: executionWorkspaceRepositoryService(db).request, listTaskRepositories: executionWorkspaceRepositoryService(db).list, prepareTaskRepositoriesForAdmission: executionWorkspaceRepositoryService(db).prepareForAdmission, - validateSelection, inspectTaskWorkspace, selectTaskWorkspace, applyPendingTaskWorkspaceSelection, bindTaskWorkspace, + validateSelection, inspectTaskWorkspace, selectTaskWorkspace, applyPendingTaskWorkspaceSelection, bindTaskWorkspace, prepareTaskWorkspaceUpdate, listOverview: async ( companyId: string, filters: WorkspaceOverviewQuery, diff --git a/server/src/services/heartbeat/workspaces.ts b/server/src/services/heartbeat/workspaces.ts index 6214663fca..c98f53dca7 100644 --- a/server/src/services/heartbeat/workspaces.ts +++ b/server/src/services/heartbeat/workspaces.ts @@ -3351,12 +3351,15 @@ export function createHeartbeatWorkspaceResolver(db: Db) { .then((rows) => rows[0] ?? null) : null; const issueProjectId = issueProjectRef?.projectId ?? null; + const workspaceSelection = issueProjectRef?.workspaceSelection?.selection; + const configuredSourceId = workspaceSelection?.kind === "configured_source" + ? workspaceSelection.projectWorkspaceId : null; const preferredProjectWorkspaceId = - issueProjectRef?.projectWorkspaceId ?? contextProjectWorkspaceId ?? null; + configuredSourceId ?? issueProjectRef?.projectWorkspaceId ?? contextProjectWorkspaceId ?? null; const [selectedSource] = preferredProjectWorkspaceId ? await db.select({ projectId: projectWorkspaces.projectId }).from(projectWorkspaces) .where(and(eq(projectWorkspaces.id, preferredProjectWorkspaceId), eq(projectWorkspaces.companyId, agent.companyId))) : []; const resolvedProjectId = selectedSource?.projectId ?? issueProjectId ?? contextProjectId; - const forceTaskDirectory = issueProjectRef?.workspaceSelection?.selection.kind === "task_directory"; + const forceTaskDirectory = workspaceSelection?.kind === "task_directory"; const useProjectWorkspace = opts?.useProjectWorkspace !== false; const workspaceProjectId = useProjectWorkspace && !forceTaskDirectory ? resolvedProjectId : null; @@ -3372,10 +3375,15 @@ export function createHeartbeatWorkspaceResolver(db: Db) { ) .orderBy(asc(projectWorkspaces.createdAt), asc(projectWorkspaces.id)) : []; - const projectWorkspaceRows = prioritizeProjectWorkspaceCandidatesForRun( + const prioritizedWorkspaceRows = prioritizeProjectWorkspaceCandidatesForRun( unorderedProjectWorkspaceRows, preferredProjectWorkspaceId, ); + // A durable source choice names the filesystem authority, not a preference + // among the project's repositories. Never label another checkout with it. + const projectWorkspaceRows = configuredSourceId + ? prioritizedWorkspaceRows.filter((workspace) => workspace.id === configuredSourceId) + : prioritizedWorkspaceRows; const workspaceHints = projectWorkspaceRows.map((workspace) => ({ workspaceId: workspace.id, @@ -3384,7 +3392,7 @@ export function createHeartbeatWorkspaceResolver(db: Db) { repoRef: readNonEmptyString(workspace.repoRef), })); - if (projectWorkspaceRows.length > 0) { + if (projectWorkspaceRows.length > 0 || configuredSourceId) { const preferredWorkspace = preferredProjectWorkspaceId ? (projectWorkspaceRows.find( (workspace) => workspace.id === preferredProjectWorkspaceId, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 702353b4ce..53cb97494e 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -11157,6 +11157,12 @@ export function issueService(db: Db) { .for("update") .then((rows: Array) => rows[0] ?? null); if (!receiptExisting) return null; + if (issueData.executionWorkspaceId !== undefined || issueData.projectWorkspaceId !== undefined || + issueData.executionWorkspacePreference !== undefined || issueData.executionWorkspaceSettings !== undefined) { + Object.assign(patch, await executionWorkspaceService(db).prepareTaskWorkspaceUpdate({ + companyId: receiptExisting.companyId, issueId: receiptExisting.id, patch, + }, tx)); + } if (expectedExecutionPolicy !== undefined && !isDeepStrictEqual( receiptExisting.executionPolicy ?? null, expectedExecutionPolicy, diff --git a/ui/src/api/execution-workspaces.test.ts b/ui/src/api/execution-workspaces.test.ts index a247d0ac45..6a018701aa 100644 --- a/ui/src/api/execution-workspaces.test.ts +++ b/ui/src/api/execution-workspaces.test.ts @@ -28,6 +28,13 @@ describe("executionWorkspacesApi.listSummaries", () => { ); }); + it("delegates task selection eligibility to the server", async () => { + await executionWorkspacesApi.listSummaries("company-1", { selectableForTask: true }); + expect(mockApi.get).toHaveBeenCalledWith( + "/companies/company-1/execution-workspaces?selectableForTask=true&summary=true", + ); + }); + it("requests and normalizes the bounded overview payload", async () => { mockApi.get.mockResolvedValue({ items: [ diff --git a/ui/src/api/execution-workspaces.ts b/ui/src/api/execution-workspaces.ts index 05ee341c62..7f90504fd2 100644 --- a/ui/src/api/execution-workspaces.ts +++ b/ui/src/api/execution-workspaces.ts @@ -59,6 +59,7 @@ export const executionWorkspacesApi = { issueId?: string; status?: string; reuseEligible?: boolean; + selectableForTask?: boolean; }, ) => { const params = new URLSearchParams(); @@ -67,6 +68,7 @@ export const executionWorkspacesApi = { if (filters?.issueId) params.set("issueId", filters.issueId); if (filters?.status) params.set("status", filters.status); if (filters?.reuseEligible) params.set("reuseEligible", "true"); + if (filters?.selectableForTask) params.set("selectableForTask", "true"); params.set("summary", "true"); const qs = params.toString(); return api.get( diff --git a/ui/src/pages/apps/chat/ChatExecutionDefaults.test.tsx b/ui/src/pages/apps/chat/ChatExecutionDefaults.test.tsx index 5847ba7221..2d6f204811 100644 --- a/ui/src/pages/apps/chat/ChatExecutionDefaults.test.tsx +++ b/ui/src/pages/apps/chat/ChatExecutionDefaults.test.tsx @@ -38,3 +38,21 @@ it("keeps edits visible after a save failure and permits retry", async () => { expect(mocks.save).toHaveBeenLastCalledWith({ projectId: null, workspace: { kind: "task_directory" } }); expect(container.textContent).toContain("Saved for new tasks."); }); + +it("offers and saves an idle workspace from the shared task selection filter", async () => { + mocks.workspaces.mockResolvedValue([ + { id: "active-workspace", name: "Active files", status: "active", closedAt: null }, + { id: "idle-workspace", name: "Idle files", status: "idle", closedAt: null }, + ]); + await render(); + expect(mocks.workspaces).toHaveBeenCalledWith("company", { selectableForTask: true }); + const options = [...container.querySelectorAll("select")[1].options].map((option) => option.value); + expect(options).toContain("existing:active-workspace"); + expect(options).toContain("existing:idle-workspace"); + await choose("existing:idle-workspace"); + await save(); + expect(mocks.save).toHaveBeenCalledWith({ + projectId: null, + workspace: { kind: "existing", workspaceId: "idle-workspace" }, + }); +}); diff --git a/ui/src/pages/apps/chat/ChatExecutionDefaults.tsx b/ui/src/pages/apps/chat/ChatExecutionDefaults.tsx index bf5cdc6bcc..4af5d358e6 100644 --- a/ui/src/pages/apps/chat/ChatExecutionDefaults.tsx +++ b/ui/src/pages/apps/chat/ChatExecutionDefaults.tsx @@ -30,7 +30,7 @@ export function ChatExecutionDefaults({ companyId, value, resource = false, onSa const projects = useQuery({ queryKey: queryKeys.projects.list(companyId), queryFn: () => projectsApi.list(companyId) }); const workspaces = useQuery({ queryKey: ["channel-workspace-choices", companyId], - queryFn: () => executionWorkspacesApi.listSummaries(companyId, { status: "active" }), + queryFn: () => executionWorkspacesApi.listSummaries(companyId, { selectableForTask: true }), }); const current = draft ?? value ?? {}; const projectValue = current.projectId === undefined ? "inherit" : current.projectId ?? "none";