From f90ea4dae4f4aca92c4772973a8687c2262f5cac Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 25 Jun 2026 13:31:59 -0700 Subject: [PATCH] Fix top-level secret ref binding sync (#8630) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Instance environments can store provider configuration fields as Paperclip secret references > - Environment save uses secret binding sync to keep persisted config refs aligned with `company_secret_bindings` > - Top-level secret-ref fields such as `apiKey` were not deleted during sync because the cleanup only matched child paths like `apiKey.*` > - Re-saving an environment with the same top-level secret ref could therefore hit the target/path unique constraint and return a 500 > - This pull request makes the sync cleanup include the exact top-level config path before reinserting current refs > - The benefit is that saved environment provider configs can be edited repeatedly without duplicate binding failures ## Linked Issues or Issue Description No public GitHub issue found in duplicate search for this exact environment secret-binding failure. ### Bug report #### Pre-submission checklist - I searched existing open and closed issues and this is not a duplicate. - I can reproduce this on the current `master` lineage. - I confirmed the error originates in Paperclip secret-binding sync, not the sandbox provider itself. #### What happened? Saving an instance environment whose provider config contains a top-level secret-ref field can fail with a duplicate key error on `company_secret_bindings_target_path_uq`. #### Expected behavior Saving the same environment config repeatedly should update/sync bindings idempotently. #### Steps to reproduce 1. Create or edit an instance environment with a provider config that has a top-level secret-ref field such as `apiKey`. 2. Save the environment. 3. Save the environment again without moving that field under a nested object. 4. The second save can attempt to insert a duplicate binding for the same target/path. #### Paperclip version or commit Observed on local dev from current `master` lineage before this fix. #### Deployment mode Local dev (pnpm dev), authenticated private mode. #### Installation method Built from source (pnpm dev / pnpm build). #### Agent adapter(s) involved Not adapter-specific (core bug). #### Database mode Embedded local Postgres. #### Access context Board (human operator) environment settings save. #### Relevant logs or output The server returned a 500 after Postgres rejected a duplicate `company_secret_bindings` row for the same environment target and `apiKey` config path. Secret values and local paths are intentionally omitted. #### Privacy checklist I reviewed the PR description for private instance links, local paths, API keys, tokens, and company-specific secrets. ## What Changed - Updated `syncSecretRefsForTarget()` so prefix cleanup removes both the exact top-level config path and nested child paths. - Added a regression test that syncs an environment top-level `apiKey` secret ref repeatedly, then replaces it and verifies only one binding remains. ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/secrets-service.test.ts` - `pnpm --filter @paperclipai/server typecheck` - `git diff --check` - Local diff scan for internal issue links, local paths, bearer/session tokens, and obvious secret literals returned no matches. - GitHub duplicate searches for related environment secret-binding issues/PRs returned no matches. ## Risks Low risk. The change only broadens the existing target/path cleanup used before reinserting secret refs. It preserves the existing child-path cleanup behavior and adds the missing exact-path case. ## Model Used OpenAI GPT-5 Codex via the `codex_local` Paperclip adapter. Tool-using coding-agent session with shell, git, and repository-edit capabilities. Context window size was not exposed by the runtime. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- server/src/__tests__/secrets-service.test.ts | 39 ++++++++++++++++++++ server/src/services/secrets.ts | 7 +++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index 4d7ffd5a68..3bc48eb908 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -145,6 +145,45 @@ describeEmbeddedPostgres("secretService", () => { ).rejects.toThrow(/already exists/i); }); + it("syncs top-level secret refs idempotently", async () => { + const companyId = await seedCompany(); + const svc = secretService(db); + const firstSecret = await svc.create(companyId, { + name: `top-level-first-${randomUUID()}`, + provider: "local_encrypted", + value: "one", + }); + const secondSecret = await svc.create(companyId, { + name: `top-level-second-${randomUUID()}`, + provider: "local_encrypted", + value: "two", + }); + const target = { targetType: "environment" as const, targetId: "env-1" }; + + await svc.syncSecretRefsForTarget(companyId, target, [ + { secretId: firstSecret.id, configPath: "apiKey" }, + ]); + await svc.syncSecretRefsForTarget(companyId, target, [ + { secretId: firstSecret.id, configPath: "apiKey" }, + ]); + await svc.syncSecretRefsForTarget(companyId, target, [ + { secretId: secondSecret.id, configPath: "apiKey" }, + ]); + + const bindings = await db + .select() + .from(companySecretBindings) + .where(eq(companySecretBindings.targetId, target.targetId)); + expect(bindings).toHaveLength(1); + expect(bindings[0]).toMatchObject({ + companyId, + targetType: "environment", + targetId: target.targetId, + configPath: "apiKey", + secretId: secondSecret.id, + }); + }); + it("reports reference counts and resolves binding target labels", async () => { const companyId = await seedCompany(); const svc = secretService(db); diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index 1f0bf36d65..25b1228d8e 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -1,4 +1,4 @@ -import { and, desc, eq, inArray, like, ne, notInArray, sql } from "drizzle-orm"; +import { and, desc, eq, inArray, like, ne, notInArray, or, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { agents, @@ -2190,7 +2190,10 @@ export function secretService(db: Db) { eq(companySecretBindings.companyId, companyId), eq(companySecretBindings.targetType, target.targetType), eq(companySecretBindings.targetId, target.targetId), - like(companySecretBindings.configPath, `${pathPrefix}.%`), + or( + eq(companySecretBindings.configPath, pathPrefix), + like(companySecretBindings.configPath, `${pathPrefix}.%`), + ), ), ); }