mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 20:44:16 +02:00
fix: preserve reused workspace sources and GitHub destination defaults
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
9b12a2e773
commit
5b3be0d651
7 files changed
+202
-22
No files matched your search
@@ -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);
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, unknown> | 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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<HTMLDivElement>(null);
|
||||
const more = useRef<HTMLDivElement>(null);
|
||||
@@ -144,6 +148,21 @@ export function GitHubRepositoryAccess({
|
||||
}
|
||||
onChange={(enabled) => onChange(resource.id, enabled)}
|
||||
/>
|
||||
<details className="text-sm">
|
||||
<summary className="cursor-pointer text-muted-foreground">Task defaults</summary>
|
||||
<div className="pt-3">
|
||||
<ChatExecutionDefaults
|
||||
companyId={companyId}
|
||||
value={resource.executionDefaults}
|
||||
resource
|
||||
onSave={async (executionDefaults) => {
|
||||
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] });
|
||||
}}
|
||||
/>
|
||||
</div>
|
||||
</details>
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
@@ -241,6 +260,7 @@ export function GitHubBotManagement({
|
||||
<>
|
||||
<GitHubRepositoryAccess
|
||||
endpointId={endpoint.id}
|
||||
companyId={endpoint.companyId}
|
||||
pending={pending}
|
||||
managementUrl={
|
||||
endpoint.setup?.github?.managementUrl ??
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { useLayoutEffect, useState, type ReactNode } from "react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { queryKeys } from "@/lib/queryKeys";
|
||||
import { buildSkillMentionHref } from "@paperclipai/shared";
|
||||
import { buildSkillMentionHref, type ChatExecutionDefaults } from "@paperclipai/shared";
|
||||
import { EditorAutocompleteProvider } from "@/context/EditorAutocompleteContext";
|
||||
import {
|
||||
endpoint,
|
||||
@@ -37,6 +37,8 @@ export function FixtureApi({
|
||||
? { ...endpoint, status: "draft", setup: { github: { stage: "setup" } } } : endpoint);
|
||||
client.setQueryData(["github-wizard", endpoint.id], { endpointId: endpoint.id, state: state === "setup" ? "create" : "connected" });
|
||||
client.setQueryData(queryKeys.chatEndpoints.list(endpoint.companyId), [endpoint]);
|
||||
client.setQueryData(queryKeys.projects.list(endpoint.companyId), []);
|
||||
client.setQueryData(["channel-workspace-choices", endpoint.companyId], []);
|
||||
client.setQueryData(["project-repositories", endpoint.companyId], {
|
||||
repositories: [
|
||||
{ id: "100", fullName: "acme/web", ownerType: "organization", url: "https://github.com/acme/web", connections: ["GitHub"] },
|
||||
@@ -118,14 +120,16 @@ export function FixtureApi({
|
||||
if (path.endsWith("/repositories/refresh")) return Response.json(repos);
|
||||
if (path.endsWith("/resources")) {
|
||||
if (init?.method === "PUT")
|
||||
repos = repos.map((r) => ({
|
||||
...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"
|
||||
? []
|
||||
|
||||
@@ -196,6 +196,7 @@ function RepositoryExample() {
|
||||
return (
|
||||
<GitHubRepositoryAccess
|
||||
endpointId={endpoint.id}
|
||||
companyId={endpoint.companyId}
|
||||
managementUrl="https://github.com/settings/installations"
|
||||
pending={pending}
|
||||
onRefresh={() => {}}
|
||||
|
||||
Reference in new issue
Block a user