mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-14 08:26:46 +02:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
58bcb04bfd
commit
215219a5cb
8 files changed
+163
-8
No files matched your search
@@ -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" });
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -11157,6 +11157,12 @@ export function issueService(db: Db) {
|
||||
.for("update")
|
||||
.then((rows: Array<typeof issues.$inferSelect>) => 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,
|
||||
|
||||
@@ -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: [
|
||||
|
||||
@@ -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<ExecutionWorkspaceSummary[]>(
|
||||
|
||||
@@ -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" },
|
||||
});
|
||||
});
|
||||
@@ -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";
|
||||
|
||||
Reference in new issue
Block a user