diff --git a/server/src/__tests__/environment-routes.test.ts b/server/src/__tests__/environment-routes.test.ts index a33d53326a..502db0c4b6 100644 --- a/server/src/__tests__/environment-routes.test.ts +++ b/server/src/__tests__/environment-routes.test.ts @@ -1036,6 +1036,31 @@ describe("environment routes", () => { ); }); + it("skips SSH secret cleanup gracefully when stored SSH config no longer parses", async () => { + const environment = { + ...createEnvironment(), + name: "SSH Fixture", + driver: "ssh" as const, + config: { + host: "", + username: "ssh-user", + }, + }; + mockEnvironmentService.getById.mockResolvedValue(environment); + mockEnvironmentService.remove.mockResolvedValue(environment); + const app = createApp({ + type: "board", + userId: "user-1", + source: "local_implicit", + }); + + const res = await request(app).delete(`/api/environments/${environment.id}`); + + expect(res.status).toBe(200); + expect(mockEnvironmentService.remove).toHaveBeenCalledWith(environment.id); + expect(mockSecretService.remove).not.toHaveBeenCalled(); + }); + it("returns 404 when deleting a missing environment", async () => { mockEnvironmentService.getById.mockResolvedValue(null); const app = createApp({ diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index 1e13d00f3b..f4c87e5f59 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -5,6 +5,7 @@ import path from "node:path"; import { randomUUID } from "node:crypto"; import { promisify } from "node:util"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { inArray } from "drizzle-orm"; import { companies, createDb, @@ -244,6 +245,104 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { ])); }); + it("clears matching environment selections transactionally without touching other workspaces", async () => { + const companyId = randomUUID(); + const projectId = randomUUID(); + const matchingWorkspaceId = randomUUID(); + const otherWorkspaceId = randomUUID(); + const untouchedWorkspaceId = randomUUID(); + const environmentId = randomUUID(); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: "PAP", + requireBoardApprovalForNewAgents: false, + }); + await db.insert(projects).values({ + id: projectId, + companyId, + name: "Workspace cleanup", + status: "in_progress", + executionWorkspacePolicy: { + enabled: true, + }, + }); + await db.insert(executionWorkspaces).values([ + { + id: matchingWorkspaceId, + companyId, + projectId, + mode: "isolated_workspace", + strategyType: "directory", + name: "Matching workspace", + status: "active", + providerType: "local_fs", + cwd: "/tmp/workspace-a", + metadata: { + source: "manual", + config: { + environmentId, + cleanupCommand: "echo clean", + }, + }, + }, + { + id: otherWorkspaceId, + companyId, + projectId, + mode: "isolated_workspace", + strategyType: "directory", + name: "Different environment", + status: "active", + providerType: "local_fs", + cwd: "/tmp/workspace-b", + metadata: { + source: "manual", + config: { + environmentId: randomUUID(), + }, + }, + }, + { + id: untouchedWorkspaceId, + companyId, + projectId, + mode: "isolated_workspace", + strategyType: "directory", + name: "No environment", + status: "active", + providerType: "local_fs", + cwd: "/tmp/workspace-c", + metadata: { + source: "manual", + }, + }, + ]); + + const cleared = await svc.clearEnvironmentSelection(companyId, environmentId); + + expect(cleared).toBe(1); + + const rows = await db + .select({ + id: executionWorkspaces.id, + metadata: executionWorkspaces.metadata, + }) + .from(executionWorkspaces) + .where(inArray(executionWorkspaces.id, [matchingWorkspaceId, otherWorkspaceId, untouchedWorkspaceId])); + + const byId = new Map(rows.map((row) => [row.id, row.metadata as Record | null])); + expect(readExecutionWorkspaceConfig(byId.get(matchingWorkspaceId) ?? null)).toMatchObject({ + environmentId: null, + cleanupCommand: "echo clean", + }); + expect(readExecutionWorkspaceConfig(byId.get(otherWorkspaceId) ?? null)).toMatchObject({ + environmentId: expect.any(String), + }); + expect(readExecutionWorkspaceConfig(byId.get(untouchedWorkspaceId) ?? null)).toBeNull(); + }); + it("warns about dirty and unmerged git worktrees and reports cleanup actions", async () => { const repoRoot = await createTempRepo(); tempDirs.add(repoRoot); diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index c34b930bf5..4e0bbcc4b1 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -22,6 +22,7 @@ import { normalizeEnvironmentConfigForPersistence, normalizeEnvironmentConfigForProbe, parseEnvironmentDriverConfig, + readSshEnvironmentPrivateKeySecretId, type ParsedEnvironmentConfig, } from "../services/environment-config.js"; import { probeEnvironment } from "../services/environment-probe.js"; @@ -319,14 +320,9 @@ export function environmentRoutes(db: Db) { res.status(404).json({ error: "Environment not found" }); return; } - if (existing.driver === "ssh") { - const parsed = parseEnvironmentDriverConfig(existing); - if (parsed.driver === "ssh") { - const secretId = parsed.config.privateKeySecretRef?.secretId; - if (secretId) { - await secrets.remove(secretId); - } - } + const secretId = readSshEnvironmentPrivateKeySecretId(existing); + if (secretId) { + await secrets.remove(secretId); } const actor = getActorInfo(req); await logActivity(db, { diff --git a/server/src/services/environment-config.ts b/server/src/services/environment-config.ts index e3fe447407..4fbb9fea86 100644 --- a/server/src/services/environment-config.ts +++ b/server/src/services/environment-config.ts @@ -206,6 +206,15 @@ export async function resolveEnvironmentDriverConfigForRuntime( return parsed; } +export function readSshEnvironmentPrivateKeySecretId( + environment: Pick, +): string | null { + if (environment.driver !== "ssh") return null; + const parsed = sshEnvironmentConfigSchema.safeParse(parseObject(environment.config)); + if (!parsed.success) return null; + return parsed.data.privateKeySecretRef?.secretId ?? null; +} + export function parseEnvironmentDriverConfig( environment: Pick, ): ParsedEnvironmentConfig { diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 7d5a7cbe70..e5fa36a1cb 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -745,31 +745,34 @@ export function executionWorkspaceService(db: Db) { }, clearEnvironmentSelection: async (companyId: string, environmentId: string) => { - const rows = await db - .select({ - id: executionWorkspaces.id, - metadata: executionWorkspaces.metadata, - }) - .from(executionWorkspaces) - .where(eq(executionWorkspaces.companyId, companyId)); - - let cleared = 0; - for (const row of rows) { - const metadata = (row.metadata as Record | null) ?? null; - const config = readExecutionWorkspaceConfig(metadata); - if (config?.environmentId !== environmentId) continue; - - await db - .update(executionWorkspaces) - .set({ - metadata: mergeExecutionWorkspaceConfig(metadata, { environmentId: null }), - updatedAt: new Date(), + return db.transaction(async (tx) => { + const rows = await tx + .select({ + id: executionWorkspaces.id, + metadata: executionWorkspaces.metadata, }) - .where(eq(executionWorkspaces.id, row.id)); - cleared += 1; - } + .from(executionWorkspaces) + .where(eq(executionWorkspaces.companyId, companyId)); - return cleared; + let cleared = 0; + const updatedAt = new Date(); + for (const row of rows) { + const metadata = (row.metadata as Record | null) ?? null; + const config = readExecutionWorkspaceConfig(metadata); + if (config?.environmentId !== environmentId) continue; + + await tx + .update(executionWorkspaces) + .set({ + metadata: mergeExecutionWorkspaceConfig(metadata, { environmentId: null }), + updatedAt, + }) + .where(eq(executionWorkspaces.id, row.id)); + cleared += 1; + } + + return cleared; + }); }, }; }