From 5b3be0d65133be3e79a8f5d309f8b2fb9f098f00 Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 9 Oct 2026 22:26:23 -0500 Subject: [PATCH] fix: preserve reused workspace sources and GitHub destination defaults Co-Authored-By: Paperclip --- .../__tests__/task-workspace-binding.test.ts | 76 +++++++++++++++++- server/src/services/execution-workspaces.ts | 9 ++- server/src/services/issues.ts | 17 ++-- .../apps/chat/GitHubBotManagement.test.tsx | 79 ++++++++++++++++++- .../pages/apps/chat/GitHubBotManagement.tsx | 20 +++++ .../github-bot-management/fixture-api.tsx | 22 +++--- .../management.stories.tsx | 1 + 7 files changed, 202 insertions(+), 22 deletions(-) diff --git a/server/src/__tests__/task-workspace-binding.test.ts b/server/src/__tests__/task-workspace-binding.test.ts index b90775c0a46..b3688986702 100644 --- a/server/src/__tests__/task-workspace-binding.test.ts +++ b/server/src/__tests__/task-workspace-binding.test.ts @@ -1,6 +1,6 @@ import { findNativeChatWorkspaceScope } from "../services/native-runtime/native-chat-workspace.js"; import { EMBEDDED_POSTGRES_TEST_TIMEOUT_MS } from "@paperclipai/db"; -import { createHeartbeatWorkspaceResolver } from "../services/heartbeat/workspaces.js"; +import { assertGitSensitiveAdapterWorkspaceValid, createHeartbeatWorkspaceResolver } from "../services/heartbeat/workspaces.js"; import { resolveDefaultAgentWorkspaceDir } from "../home-paths.js"; import fs from "node:fs/promises"; import os from "node:os"; @@ -96,6 +96,80 @@ const support = await getEmbeddedPostgresTestSupport(); } }); + it.each(["explicit", "channel", "inherited", "legacy"] as const)("keeps a reused task folder's null source authoritative for %s creation", async (source) => { + 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 [project] = await db.insert(projects).values({ companyId: f.companyId, name: "Organization with unrelated source" }).returning(); + const [projectWorkspace] = await db.insert(projectWorkspaces).values({ companyId: f.companyId, projectId: project.id, + name: "Unrelated repository", cwd: "/tmp/unrelated-source", isPrimary: true }).returning(); + let parentId: string | undefined; + if (source === "inherited") { + const [parent] = await db.insert(issues).values({ companyId: f.companyId, projectId: project.id, + title: "Parent sharing task files", executionWorkspaceId: f.workspaceId }).returning(); + parentId = parent.id; + } + const created = await tasks.create(f.companyId, { + title: "Reuse files under an organizational project", projectId: project.id, parentId, + workspaceSelectionActor: f.actor, + ...(source === "legacy" + ? { executionWorkspaceId: f.workspaceId, projectWorkspaceId: projectWorkspace.id, executionWorkspacePreference: "reuse_existing" } + : source === "inherited" ? {} + : { workspaceSelection: { kind: "existing" as const, workspaceId: f.workspaceId }, workspaceSelectionSource: source }), + }); + expect(created).toMatchObject({ projectId: project.id, executionWorkspaceId: f.workspaceId, projectWorkspaceId: null }); + const [original] = await db.select().from(issues).where(eq(issues.id, f.issueId)); + expect(original).toMatchObject({ projectId: null, projectWorkspaceId: null, executionWorkspaceId: f.workspaceId }); + const { workspace } = await svc.inspectTaskWorkspace(f.companyId, f.issueId, f.actor); + expect(workspace).toMatchObject({ projectId: null, projectWorkspaceId: null, cwd: `/tmp/task-${f.issueId}` }); + // Both tasks remain eligible for the same local adapter launch; the + // organization did not turn the shared task folder into a Git source. + for (const task of [created, original]) { + await expect(assertGitSensitiveAdapterWorkspaceValid({ + adapterType: "codex_local", agentId: randomUUID(), issue: task, + resolvedWorkspace: { cwd: workspace!.cwd!, source: "task_session", projectId: null, workspaceId: null, + repoUrl: null, repoRef: null, workspaceHints: [], warnings: [] }, + executionWorkspace: { cwd: workspace!.cwd!, baseCwd: workspace!.cwd!, source: "task_session", projectId: null, + workspaceId: null, repoUrl: null, repoRef: null, strategy: "project_primary", branchName: null, + worktreePath: null, warnings: [], created: false, branchCreatedByRuntime: false }, + persistedExecutionWorkspace: workspace, executionTarget: { kind: "local" }, + })).resolves.toBeUndefined(); + } + } finally { + await settings.updateExperimental({ enableIsolatedWorkspaces: previous.enableIsolatedWorkspaces }); + } + }); + + it("replaces an old source with the selected binding's null source on an ordinary update", async () => { + const f = await fixture(), svc = executionWorkspaceService(db), settings = instanceSettingsService(db); + const previous = await settings.getExperimental(); + await settings.updateExperimental({ enableIsolatedWorkspaces: true }); + try { + await svc.bindTaskWorkspace(f.companyId, f.issueId, f.workspaceId); + const [project] = await db.insert(projects).values({ companyId: f.companyId, name: "Organizational project" }).returning(); + const [source] = await db.insert(projectWorkspaces).values({ companyId: f.companyId, projectId: project.id, + name: "Previous source", cwd: "/tmp/previous-source" }).returning(); + const [oldWorkspace] = await db.insert(executionWorkspaces).values({ companyId: f.companyId, projectId: project.id, + projectWorkspaceId: source.id, name: "Previous folder", cwd: source.cwd, mode: "shared_workspace", strategyType: "project_primary" }).returning(); + const [task] = await db.insert(issues).values({ companyId: f.companyId, projectId: project.id, + projectWorkspaceId: source.id, executionWorkspaceId: oldWorkspace.id, title: "Switch to task files" }).returning(); + await svc.selectTaskWorkspace({ ...f, issueId: task.id, selection: { kind: "task_directory" }, + expectedBindingRevision: 0, requestKey: "superseded-before-raw-binding" }); + const updated = await issueService(db).update(task.id, { executionWorkspaceId: f.workspaceId }); + expect(updated).toMatchObject({ projectId: project.id, executionWorkspaceId: f.workspaceId, projectWorkspaceId: null, + workspaceBindingRevision: 1, workspacePendingSelection: null }); + expect(await svc.applyPendingTaskWorkspaceSelection({ ...f, issueId: task.id, runId: randomUUID() })).toBe(false); + expect((await svc.inspectTaskWorkspace(f.companyId, f.issueId, f.actor)).workspace) + .toMatchObject({ id: f.workspaceId, projectId: null, projectWorkspaceId: null, cwd: `/tmp/task-${f.issueId}` }); + const [previousWorkspace] = await db.select().from(executionWorkspaces).where(eq(executionWorkspaces.id, oldWorkspace.id)); + expect(previousWorkspace).toMatchObject({ projectId: project.id, projectWorkspaceId: source.id, cwd: source.cwd }); + } finally { + await settings.updateExperimental({ enableIsolatedWorkspaces: previous.enableIsolatedWorkspaces }); + } + }); + it("queues explicit root changes without changing the current binding and applies at admission", async () => { const f = await fixture(), svc = executionWorkspaceService(db); await svc.bindTaskWorkspace(f.companyId, f.issueId, f.workspaceId); diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index e9b17570701..fc6132ab28d 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -1426,8 +1426,15 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic 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"); + const [selectedWorkspace] = input.patch.executionWorkspaceId + ? await tx.select({ projectWorkspaceId: executionWorkspaces.projectWorkspaceId }).from(executionWorkspaces).where(and( + eq(executionWorkspaces.id, input.patch.executionWorkspaceId), eq(executionWorkspaces.companyId, input.companyId), + )).limit(1) + : []; + if (input.patch.executionWorkspaceId && !selectedWorkspace) throw notFound("Execution workspace not found"); return { workspaceBindingRevision: task.workspaceBindingRevision + 1, - workspacePendingSelection: null, workspaceSelection: null }; + workspacePendingSelection: null, workspaceSelection: null, + ...(selectedWorkspace ? { projectWorkspaceId: selectedWorkspace.projectWorkspaceId } : {}) }; } const inspectDisplay = opts.inspectGitCloseReadiness diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 8be76b994e3..3bc7cc1950e 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -7304,6 +7304,7 @@ export function issueService(db: Db) { id: executionWorkspaces.id, companyId: executionWorkspaces.companyId, projectId: executionWorkspaces.projectId, + projectWorkspaceId: executionWorkspaces.projectWorkspaceId, }) .from(executionWorkspaces) .where(eq(executionWorkspaces.id, executionWorkspaceId)) @@ -10171,6 +10172,12 @@ export function issueService(db: Db) { executionWorkspaceSettings = projection.executionWorkspaceSettings; workspaceIntent = { version: 1, selection: workspaceSelection, source: workspaceSelectionSource }; } + if (executionWorkspaceId) { + // A durable binding owns its source, including a deliberately absent + // source. Organizational defaults and legacy hints cannot replace it. + const workspace = await assertValidExecutionWorkspace(companyId, null, executionWorkspaceId, tx); + projectWorkspaceId = workspace.projectWorkspaceId; + } const projectGoalId = await getProjectDefaultGoalId( tx, companyId, @@ -10217,7 +10224,7 @@ export function issueService(db: Db) { ), ) as Record | null; } - if (!projectWorkspaceId && issueData.projectId && workspaceIntent?.selection.kind !== "task_directory") { + if (!executionWorkspaceId && !projectWorkspaceId && issueData.projectId && workspaceIntent?.selection.kind !== "task_directory") { const project = await tx .select({ executionWorkspacePolicy: projects.executionWorkspacePolicy, @@ -10273,14 +10280,6 @@ export function issueService(db: Db) { tx, ); } - if (executionWorkspaceId) { - await assertValidExecutionWorkspace( - companyId, - null, - executionWorkspaceId, - tx, - ); - } if ( isolatedWorkspacesEnabled && issueData.executionWorkspaceSettings !== undefined diff --git a/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx b/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx index eb7001e6dd0..78c713ff86f 100644 --- a/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx +++ b/ui/src/pages/apps/chat/GitHubBotManagement.test.tsx @@ -8,6 +8,7 @@ import { buildSkillMentionHref, type GitHubChatConfiguration, type GitHubTaskReview, + type ChatExecutionDefaults, } from "@paperclipai/shared"; import { ChatEndpointDetail } from "./ChatEndpointDetail"; import { GitHubReviewList, orderedGitHubReviews } from "./GitHubBotManagement"; @@ -28,6 +29,8 @@ const mocks = vi.hoisted(() => ({ repositoryPage: vi.fn(), toggleAll: vi.fn(), updateResources: vi.fn(), + projects: vi.fn(), + workspaces: vi.fn(), reviews: vi.fn(), review: vi.fn(), members: vi.fn(), @@ -52,8 +55,8 @@ vi.mock("@/api/chatEndpoints", () => ({ listPrincipals: mocks.links, }, })); -vi.mock("@/api/projects", () => ({ projectsApi: { list: vi.fn().mockResolvedValue([]) } })); -vi.mock("@/api/execution-workspaces", () => ({ executionWorkspacesApi: { listSummaries: vi.fn().mockResolvedValue([]) } })); +vi.mock("@/api/projects", () => ({ projectsApi: { list: mocks.projects } })); +vi.mock("@/api/execution-workspaces", () => ({ executionWorkspacesApi: { listSummaries: mocks.workspaces } })); vi.mock("@/api/access", () => ({ accessApi: { listMembers: mocks.members } })); vi.mock("@/api/agents", () => ({ agentsApi: { get: vi.fn() } })); vi.mock("@/context/BreadcrumbContext", () => ({ @@ -138,6 +141,8 @@ describe("GitHub bot management", () => { mocks.tab = "settings"; mocks.reviewId = undefined; mocks.get.mockResolvedValue(endpoint); + mocks.projects.mockResolvedValue([]); + mocks.workspaces.mockResolvedValue([]); mocks.config.mockResolvedValue({ revision: 4, configuration: structuredClone(base), @@ -287,6 +292,76 @@ describe("GitHub bot management", () => { await vi.waitFor(() => expect(container.querySelectorAll('[role="region"] [role="switch"]')).toHaveLength(1)); expect(container.textContent).not.toContain("Could not load repositories"); }); + it("saves repository task defaults with explicit, cleared, and inherited values without changing access", async () => { + let executionDefaults: ChatExecutionDefaults | null = null; + mocks.projects.mockResolvedValue([{ id: "project", name: "Website", workspaces: [] }]); + mocks.workspaces.mockResolvedValue([{ id: "workspace", name: "Shared website" }]); + mocks.resources.mockImplementation(async () => [{ + id: "repo", label: "acme/web", type: "repository", enabled: false, + availability: "available", executionDefaults, + }]); + mocks.updateResources.mockImplementation(async (_id, [update]) => { + executionDefaults = update.executionDefaults; + return mocks.resources(); + }); + await render("access"); + await vi.waitFor(() => expect(container.querySelector('[role="region"] summary')).not.toBeNull()); + await act(async () => (container.querySelector('[role="region"] summary') as HTMLElement).click()); + const editor = container.querySelector('[role="region"] form')!; + const [project, workspace] = [...editor.querySelectorAll("select")]; + await vi.waitFor(() => expect(workspace.disabled).toBe(false)); + expect(mocks.projects).toHaveBeenCalledWith("company"); + expect(mocks.workspaces).toHaveBeenCalledWith("company", { selectableForTask: true }); + expect(project.value).toBe("inherit"); + expect(workspace.value).toBe("inherit"); + expect(editor.textContent).toContain("Use connection defaults"); + + const save = async (expected: ChatExecutionDefaults | null) => { + const calls = mocks.updateResources.mock.calls.length; + await click("Save defaults"); + await vi.waitFor(() => { + expect(mocks.updateResources).toHaveBeenCalledTimes(calls + 1); + expect(editor.textContent).toContain("Saved for new tasks."); + }); + expect(mocks.updateResources).toHaveBeenLastCalledWith("bot", [{ id: "repo", executionDefaults: expected }]); + expect(container.querySelector('[role="region"] [role="switch"]')?.getAttribute("aria-checked")).toBe("false"); + }; + await input(project, "project"); + await input(workspace, "existing:workspace"); + await save({ projectId: "project", workspace: { kind: "existing", workspaceId: "workspace" } }); + expect(project.value).toBe("project"); + expect(workspace.value).toBe("existing:workspace"); + await input(project, "none"); + await input(workspace, "clear"); + await save({ projectId: null, workspace: null }); + await input(project, "inherit"); + await save({ workspace: null }); + await input(workspace, "inherit"); + await save(null); + await vi.waitFor(() => { + expect(project.value).toBe("inherit"); + expect(workspace.value).toBe("inherit"); + }); + expect(mocks.save).not.toHaveBeenCalled(); + expect(mocks.toggleAll).not.toHaveBeenCalled(); + }); + it("keeps a rejected repository task-defaults draft visible for retry", async () => { + mocks.updateResources.mockRejectedValueOnce(new Error("Workspace access denied")); + await render("access"); + await vi.waitFor(() => expect(container.querySelector('[role="region"] summary')).not.toBeNull()); + await act(async () => (container.querySelector('[role="region"] summary') as HTMLElement).click()); + const editor = container.querySelector('[role="region"] form')!; + const workspace = editor.querySelectorAll("select")[1]; + await vi.waitFor(() => expect(workspace.disabled).toBe(false)); + await input(workspace, "task"); + await click("Save defaults"); + await vi.waitFor(() => expect(editor.querySelector('[role="alert"]')?.textContent).toBe("Workspace access denied")); + expect(workspace.value).toBe("task"); + expect(container.querySelector('[role="region"] [role="switch"]')?.getAttribute("aria-checked")).toBe("true"); + await click("Cancel"); + expect(workspace.value).toBe("inherit"); + expect(editor.querySelector('[role="alert"]')).toBeNull(); + }); it("automatically loads the next 20 repositories when the scroll sentinel becomes visible", async () => { let intersect!: IntersectionObserverCallback; vi.stubGlobal("IntersectionObserver", class { diff --git a/ui/src/pages/apps/chat/GitHubBotManagement.tsx b/ui/src/pages/apps/chat/GitHubBotManagement.tsx index 694a2d671ab..564869fed47 100644 --- a/ui/src/pages/apps/chat/GitHubBotManagement.tsx +++ b/ui/src/pages/apps/chat/GitHubBotManagement.tsx @@ -36,9 +36,11 @@ import { GitHubPolicyEditor, GitHubToggle, } from "./GitHubBotConfiguration"; +import { ChatExecutionDefaults } from "./ChatExecutionDefaults"; export function GitHubRepositoryAccess({ endpointId, + companyId, managementUrl, pending, onRefresh, @@ -46,12 +48,14 @@ export function GitHubRepositoryAccess({ onToggleAll, }: { endpointId: string; + companyId: string; managementUrl: string; pending: boolean; onRefresh: () => void; onChange: (id: string, enabled: boolean) => void; onToggleAll: (enabled: boolean) => void; }) { + const client = useQueryClient(); const [search, setSearch] = useState(""); const scroller = useRef(null); const more = useRef(null); @@ -144,6 +148,21 @@ export function GitHubRepositoryAccess({ } onChange={(enabled) => onChange(resource.id, enabled)} /> +
+ Task defaults +
+ { + await chatEndpointsApi.updateResources(endpointId, [{ id: resource.id, executionDefaults }]); + await client.invalidateQueries({ queryKey: ["github-bot-repositories", endpointId] }); + await client.invalidateQueries({ queryKey: ["github-bot-repository-pages", endpointId] }); + }} + /> +
+
))} @@ -241,6 +260,7 @@ export function GitHubBotManagement({ <> ({ - ...r, - enabled: - body.resources?.find( - (change: { id: string; enabled: boolean }) => - change.id === r.id, - )?.enabled ?? r.enabled, - })); + repos = repos.map((r) => { + const change = body.resources?.find( + (entry: { id: string; enabled?: boolean; executionDefaults?: ChatExecutionDefaults | null }) => entry.id === r.id, + ); + return { + ...r, + enabled: change?.enabled ?? r.enabled, + executionDefaults: change?.executionDefaults === undefined ? r.executionDefaults : change.executionDefaults, + }; + }); const rows = state === "empty" ? [] diff --git a/ui/storybook/stories/github-bot-management/management.stories.tsx b/ui/storybook/stories/github-bot-management/management.stories.tsx index 0d32d841935..86b8e63d696 100644 --- a/ui/storybook/stories/github-bot-management/management.stories.tsx +++ b/ui/storybook/stories/github-bot-management/management.stories.tsx @@ -196,6 +196,7 @@ function RepositoryExample() { return ( {}}