From 7c2827d33f217b4d765f3a301a35055ca438d88e Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 5 Jun 2026 04:13:49 +0000 Subject: [PATCH] Clean up failed import secrets --- .../src/__tests__/company-portability.test.ts | 70 +++++++++++++++++++ server/src/services/company-portability.ts | 45 ++++++++++++ 2 files changed, 115 insertions(+) diff --git a/server/src/__tests__/company-portability.test.ts b/server/src/__tests__/company-portability.test.ts index e3648b30988..7f3713316f5 100644 --- a/server/src/__tests__/company-portability.test.ts +++ b/server/src/__tests__/company-portability.test.ts @@ -64,6 +64,7 @@ const assetSvc = { const secretSvc = { create: vi.fn(async () => ({ id: "secret-created" })), + remove: vi.fn(async () => ({ id: "secret-created" })), normalizeAdapterConfigForPersistence: vi.fn(async (_companyId: string, config: Record) => config), normalizeEnvBindingsForPersistence: vi.fn(async (_companyId: string, env: Record) => env), syncEnvBindingsForTarget: vi.fn(async () => []), @@ -133,6 +134,7 @@ describe("company portability", () => { beforeEach(() => { vi.clearAllMocks(); secretSvc.create.mockResolvedValue({ id: "secret-created" }); + secretSvc.remove.mockResolvedValue({ id: "secret-created" }); secretSvc.normalizeAdapterConfigForPersistence.mockImplementation(async (_companyId, config) => config); secretSvc.normalizeEnvBindingsForPersistence.mockImplementation(async (_companyId, env) => env); secretSvc.syncEnvBindingsForTarget.mockResolvedValue([]); @@ -1501,6 +1503,74 @@ describe("company portability", () => { ); }); + it("cleans up unconsumed import secrets when agent import fails before persistence", async () => { + const portability = companyPortabilityService({} as any); + agentSvc.list.mockResolvedValue([]); + agentSvc.create.mockRejectedValueOnce(new Error("agent create failed")); + + await expect(portability.importBundle({ + source: { + type: "inline", + files: { + "COMPANY.md": [ + "---", + "name: Import", + "includes:", + " - agents/coder/AGENTS.md", + "---", + "", + ].join("\n"), + "agents/coder/AGENTS.md": [ + "---", + "name: Coder", + "slug: coder", + "kind: agent", + "---", + "", + "# Coder", + "", + ].join("\n"), + ".paperclip.yaml": [ + "schema: paperclip/v1", + "agents:", + " coder:", + " adapter:", + " type: codex_local", + " config: {}", + " inputs:", + " env:", + " OPENAI_API_KEY:", + " kind: secret", + " requirement: required", + "", + ].join("\n"), + }, + }, + include: { + company: false, + agents: true, + projects: false, + issues: false, + }, + target: { + mode: "existing_company", + companyId: "company-1", + }, + collisionStrategy: "rename", + secretValues: { + "agent:coder:OPENAI_API_KEY": "sk-imported", + }, + }, "user-1")).rejects.toThrow("agent create failed"); + + expect(secretSvc.create).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ value: "sk-imported" }), + { userId: "user-1", agentId: null }, + ); + expect(secretSvc.remove).toHaveBeenCalledWith("secret-created"); + expect(secretSvc.syncEnvBindingsForTarget).not.toHaveBeenCalled(); + }); + it("exports project env as portable inputs without concrete values", async () => { const portability = companyPortabilityService({} as any); diff --git a/server/src/services/company-portability.ts b/server/src/services/company-portability.ts index 131500c3d3a..c52d9c2f4ae 100644 --- a/server/src/services/company-portability.ts +++ b/server/src/services/company-portability.ts @@ -71,6 +71,7 @@ import { projectService } from "./projects.js"; import { routineService } from "./routines.js"; import { secretService } from "./secrets.js"; import { getConfiguredSecretProvider } from "../secrets/configured-provider.js"; +import { logger } from "../middleware/logger.js"; import { PORTABLE_CATALOG_PROVENANCE_STRING_KEYS, readCatalogStringList, @@ -2989,6 +2990,11 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { const strictSecretsMode = process.env.PAPERCLIP_SECRETS_STRICT_MODE === "true"; const defaultSecretProvider = getConfiguredSecretProvider(); + type ImportSecretTracker = { + createdSecretIds: string[]; + consumerPersisted: boolean; + }; + function assertKnownImportAdapterType(type: string | null | undefined): string { const adapterType = typeof type === "string" ? type.trim() : ""; if (!adapterType) { @@ -3052,6 +3058,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { envInputs: CompanyPortabilityEnvInput[], secretValues: Record | null | undefined, actorUserId: string | null | undefined, + secretTracker?: ImportSecretTracker, ) { if (envInputs.length === 0) return; const missingRequired = envInputs.filter((input) => { @@ -3088,6 +3095,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { }, { userId: actorUserId ?? null, agentId: null }, ); + secretTracker?.createdSecretIds.push(secret.id); writeManifestEnvBinding(manifest, input, { type: "secret_ref", secretId: secret.id, @@ -3096,6 +3104,20 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { } } + async function cleanupUnconsumedImportSecrets(secretTracker: ImportSecretTracker) { + if (secretTracker.consumerPersisted || secretTracker.createdSecretIds.length === 0) return; + for (const secretId of [...new Set(secretTracker.createdSecretIds)]) { + try { + await secrets.remove(secretId); + } catch (err) { + logger.warn( + { err, secretId }, + "failed to clean up unconsumed import secret after company import failure", + ); + } + } + } + function resolveImportedAssigneeAgentId( assigneeSlug: string | null | undefined, importedSlugToAgentId: Map, @@ -4261,6 +4283,24 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { input: CompanyPortabilityImport, actorUserId: string | null | undefined, options?: ImportBehaviorOptions, + ): Promise { + const secretTracker: ImportSecretTracker = { + createdSecretIds: [], + consumerPersisted: false, + }; + try { + return await importBundleInner(input, actorUserId, options, secretTracker); + } catch (err) { + await cleanupUnconsumedImportSecrets(secretTracker); + throw err; + } + } + + async function importBundleInner( + input: CompanyPortabilityImport, + actorUserId: string | null | undefined, + options: ImportBehaviorOptions | undefined, + secretTracker: ImportSecretTracker, ): Promise { const mode = resolveImportMode(options); const plan = await buildPreview(input, options); @@ -4391,6 +4431,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { importEnvInputs, input.secretValues, actorUserId, + secretTracker, ); if (include.company) { @@ -4565,6 +4606,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { }); continue; } + secretTracker.consumerPersisted = true; try { const materialized = await instructions.materializeManagedBundle(updated, bundleFiles, { clearLegacyPromptTemplate: true, @@ -4597,6 +4639,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { ...patch, status: createdStatus, }); + secretTracker.consumerPersisted = true; await access.ensureMembership(targetCompany.id, "agent", created.id, "member", "active"); await access.setPrincipalPermission( targetCompany.id, @@ -4724,6 +4767,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { }); continue; } + secretTracker.consumerPersisted = true; projectId = updated.id; importedSlugToProjectId.set(planProject.slug, updated.id); existingProjectSlugToId.set(updated.urlKey, updated.id); @@ -4736,6 +4780,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { }); } else { const created = await projects.create(targetCompany.id, projectPatch); + secretTracker.consumerPersisted = true; projectId = created.id; importedSlugToProjectId.set(planProject.slug, created.id); existingProjectSlugToId.set(created.urlKey, created.id);