From ad4f0b58676ca6f807fa9a009c25615360b09c76 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 11 Sep 2026 13:58:29 -0700 Subject: [PATCH] Fix Codex API key authentication in tests and runs (#13260) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agent runtime settings can bind organization secrets to an adapter environment > - Paperclip redacts plain environment values when it returns a saved agent to the UI > - A saved-agent test sent the redacted `CODEX_HOME` value back to the server > - Codex ACP also received the API key without an ACP API-key authentication request > - This pull request restores saved environment values for tests and selects API-key authentication for Codex ACP runs > - The benefit is that Codex agents can test and run with an organization-scoped OpenAI API key ## Linked Issues or Issue Description **What happened?** Testing a saved Codex agent sent `***REDACTED***` as `CODEX_HOME`. Secret normalization rejected that placeholder. Remote Codex ACP runs received `OPENAI_API_KEY`, but session creation stopped with `Authentication required`. **Expected behavior** Paperclip must use the saved `CODEX_HOME` value when it tests an existing agent. Codex ACP must select API-key authentication when `OPENAI_API_KEY` is available. **Steps to reproduce** 1. Create an organization-scoped secret named `OPENAI_API_KEY`. 2. Give a Codex agent access to the secret. 3. Save the agent runtime settings. 4. Test the saved agent again. 5. Run the agent in a remote sandbox through ACP. **Paperclip version or commit** Reproduced on master before commit `68c17709d7c051a804a416263e2e08920f1dfcb1`. **Deployment mode** Self-hosted server with a remote sandbox environment. **Installation method** Built from source. **Agent adapter(s) involved** Codex. ## What Changed - Send the saved agent ID with adapter environment tests. - Restore redacted plain environment values from the saved agent before test-time secret resolution. - Select the Codex ACP `api-key` authentication method when `OPENAI_API_KEY` is present. - Add focused regression coverage for saved-agent tests and remote ACP launch configuration. ## Verification - `pnpm --filter @paperclipai/adapter-utils exec vitest run src/acpx-engine/execute.test.ts` - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/agent-adapter-validation-routes.test.ts` - `pnpm --filter @paperclipai/ui exec vitest run src/lib/test-agent-setup.test.ts` - `pnpm -r typecheck` - `pnpm test:run` - `pnpm build` - `git diff --check` ## Risks - Low risk. The test route reads saved configuration only when the request supplies a compatible agent ID and the caller can update that agent. - The Codex ACP change applies only when `OPENAI_API_KEY` exists and no explicit `DEFAULT_AUTH_REQUEST` exists. - There are no schema migrations or telemetry changes. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex with `gpt-5`. The context-window size is not exposed in this runtime. The model used reasoning, repository search, file editing, command execution, and test execution. ## 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 --- .../src/acpx-engine/execute.test.ts | 35 ++++++++++++-- .../adapter-utils/src/acpx-engine/execute.ts | 11 +++++ packages/shared/src/validators/agent.ts | 2 + .../agent-adapter-validation-routes.test.ts | 46 +++++++++++++++++++ server/src/routes/agents.ts | 25 +++++++++- ui/src/api/agents.ts | 1 + ui/src/components/AgentConfigForm.tsx | 5 +- ui/src/lib/test-agent-setup.test.ts | 2 + ui/src/lib/test-agent-setup.ts | 2 + 9 files changed, 122 insertions(+), 7 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index 5981cc79cf..1e3b8abd00 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -1548,6 +1548,30 @@ describe("shared ACPX engine runtime behavior", () => { expect(env.PAPERCLIP_CLOUD_PROVIDER_TOKEN).toBe("cloud-token"); }); + it.each(["OPENAI_API_KEY", "CODEX_API_KEY"] as const)( + "selects Codex ACP API-key authentication when %s is configured", + async (apiKeyName) => { + const root = await makeTempRoot(); + const codexHome = path.join(root, "codex-home"); + await fs.mkdir(codexHome, { recursive: true }); + + const { sessionInputs } = await runExecutor({ + agent: "codex", + stateDir: path.join(root, "state"), + env: { + CODEX_HOME: codexHome, + [apiKeyName]: "sk-acp-test-key", + }, + paperclipRuntimeSkills: [], + paperclipSkillSync: { desiredSkills: [] }, + }); + + const env = (sessionInputs[0]!.sessionOptions as { env: Record }).env; + expect(env[apiKeyName]).toBe("sk-acp-test-key"); + expect(env.DEFAULT_AUTH_REQUEST).toBe(JSON.stringify({ methodId: "api-key" })); + }, + ); + it("busts the session fingerprint when resolved adapter env changes but not across wakes", async () => { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); @@ -4854,13 +4878,13 @@ describe("ACPX engine sandbox-start spans (opt-in root + child parenting)", () = const { traceContext, spans } = createRecordingStartupTrace(); // A codex bring-up runs the codex-home.seed step, which nests skills.reconcile. - const { events } = await runExecutor( + const { events, sessionInputs } = await runExecutor( { agent: "codex", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd, - env: { CODEX_HOME: codexHome }, + env: { CODEX_HOME: codexHome, OPENAI_API_KEY: "sk-acp-test-key" }, }, { authToken: "real-run-jwt", executionTarget, startupTraceContext: traceContext }, ); @@ -5426,13 +5450,13 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => runner: createLocalSandboxRunner(), }; - const { events } = await runExecutor( + const { events, sessionInputs } = await runExecutor( { agent: "codex", agentCommand: "node ./fake-acp.js", stateDir, cwd: localCwd, - env: { CODEX_HOME: codexHome }, + env: { CODEX_HOME: codexHome, OPENAI_API_KEY: "sk-acp-test-key" }, }, { authToken: "real-run-jwt", executionTarget }, ); @@ -5454,6 +5478,9 @@ describe("ACPX engine per-step startup timing (run.startup.step events)", () => expect(typeof event!.payload?.durationMs).toBe("number"); expect(event!.payload?.durationMs as number).toBeGreaterThanOrEqual(0); } + const sessionEnv = (sessionInputs[0]?.sessionOptions as { env: Record }).env; + expect(sessionEnv.OPENAI_API_KEY).toBe("sk-acp-test-key"); + expect(sessionEnv.DEFAULT_AUTH_REQUEST).toBe(JSON.stringify({ methodId: "api-key" })); }); it("emits the 5 non-codex boundaries for a custom-agent sandbox bring-up (no codex steps)", async () => { diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index ae10093fcb..b4971bd718 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -1967,6 +1967,17 @@ async function buildRuntime(input: { // are absent from tempKeysApplied and keep their compatibility protection. if (!scratchKeys.has(key) || value !== scratch.dir) resolvedAdapterEnv[key] = value; } + // codex-acp supports both key names, but ACP clients must select its + // api-key authentication method during session creation. Without this + // request, the server advertises authentication and rejects session/new even + // though the credential is present in the launched process environment. + if ( + acpxAgent === "codex" && + (env.OPENAI_API_KEY || env.CODEX_API_KEY) && + !env.DEFAULT_AUTH_REQUEST + ) { + env.DEFAULT_AUTH_REQUEST = JSON.stringify({ methodId: "api-key" }); + } if (authToken) env.PAPERCLIP_API_KEY = authToken; // For the claude agent, set model via ANTHROPIC_MODEL at startup rather than // via session/set_config_option — the ACP server's set_config_option handler diff --git a/packages/shared/src/validators/agent.ts b/packages/shared/src/validators/agent.ts index c1fdf16c72..fbff34fa72 100644 --- a/packages/shared/src/validators/agent.ts +++ b/packages/shared/src/validators/agent.ts @@ -242,6 +242,8 @@ export const resetAgentSessionSchema = z.object({ export type ResetAgentSession = z.infer; export const testAdapterEnvironmentSchema = z.object({ + /** Saved agent whose redacted environment entries are restored for this probe. */ + agentId: z.string().guid().optional(), /** One-shot provider keys for a probe. Never persist these in agent config. */ testCredentials: z.object({ ANTHROPIC_API_KEY: z.string().max(16384), diff --git a/server/src/__tests__/agent-adapter-validation-routes.test.ts b/server/src/__tests__/agent-adapter-validation-routes.test.ts index 918560aaa1..8f1c2e11d3 100644 --- a/server/src/__tests__/agent-adapter-validation-routes.test.ts +++ b/server/src/__tests__/agent-adapter-validation-routes.test.ts @@ -536,6 +536,52 @@ describe("agent routes adapter validation", () => { expect(String(env.CODEX_HOME)).toContain(`/companies/company-1/agents/${agentId}/codex-home`); }); + it("restores a saved agent's redacted CODEX_HOME before testing its adapter", async () => { + const agentId = "11111111-1111-4111-8111-111111111111"; + const storedHome = "/paperclip/companies/company-1/agents/agent-1/codex-home"; + mockAgentService.getById.mockResolvedValue({ + ...(await mockAgentService.getById()), + id: agentId, + adapterType: "external_test", + adapterConfig: { env: { CODEX_HOME: storedHome } }, + }); + const { registerServerAdapter } = await import("../adapters/index.js"); + registerServerAdapter(externalAdapter); + const app = await createApp(); + const res = await requestApp(app, (baseUrl) => + request(baseUrl) + .post("/api/companies/company-1/adapters/external_test/test-environment") + .send({ + agentId, + adapterConfig: { env: { CODEX_HOME: { type: "plain", value: "***REDACTED***" } } }, + }), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockSecretService.normalizeAdapterConfigForPersistence).toHaveBeenCalledWith( + "company-1", + { env: { CODEX_HOME: storedHome } }, + expect.objectContaining({ adapterType: "external_test" }), + ); + }); + + it("rejects redacted-value restoration from an incompatible saved agent", async () => { + const { registerServerAdapter } = await import("../adapters/index.js"); + registerServerAdapter(externalAdapter); + const app = await createApp(); + const res = await requestApp(app, (baseUrl) => + request(baseUrl) + .post("/api/companies/company-1/adapters/external_test/test-environment") + .send({ + agentId: "11111111-1111-4111-8111-111111111111", + adapterConfig: { env: { CODEX_HOME: { type: "plain", value: "***REDACTED***" } } }, + }), + ); + + expect(res.status).toBe(422); + expect(mockSecretService.normalizeAdapterConfigForPersistence).not.toHaveBeenCalled(); + }); + it("rejects unknown adapter types even when schema accepts arbitrary strings", async () => { const app = await createApp(); const res = await requestApp(app, (baseUrl) => diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 6efa0888c9..9585da7cce 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -3139,9 +3139,32 @@ export function agentRoutes( if (requestedEnvironmentId) { await assertAdapterTestEnvironmentForCompany(companyId, requestedEnvironmentId); } + // Agent reads redact every plain environment value. When this is a saved + // agent test, restore those display-only placeholders from the + // server-side config before validating or resolving secrets; otherwise + // the probe treats "***REDACTED***" as a value to persist. + const savedAgentId = typeof req.body.agentId === "string" ? req.body.agentId : null; + let adapterConfigForTest = inputAdapterConfig; + if (savedAgentId) { + const savedAgent = await getAccessibleResource(req, res, svc.getById(savedAgentId), "Agent not found"); + if (!savedAgent) return; + if (savedAgent.companyId !== companyId) throw notFound("Agent not found"); + const providerAdapter = savedAgent.adapterType === "paperclip_runner" + ? inputAdapterConfig.provider === "codex" + ? "codex_local" + : inputAdapterConfig.provider === "acpx" && inputAdapterConfig.acpxAgent === "claude" + ? "claude_local" + : null + : null; + if (savedAgent.adapterType !== type && providerAdapter !== type) { + throw unprocessable("Saved agent is not compatible with the adapter being tested"); + } + await assertCanUpdateAgent(req, savedAgent); + adapterConfigForTest = restoreRedactedAgentEnv(inputAdapterConfig, savedAgent.adapterConfig); + } const normalizedAdapterConfig = await secretsSvc.normalizeAdapterConfigForPersistence( companyId, - inputAdapterConfig, + adapterConfigForTest, { strictMode: strictSecretsMode, adapterType: type }, ); // Prospective, non-persisted config: resolve the acting user's own user diff --git a/ui/src/api/agents.ts b/ui/src/api/agents.ts index 43ec821358..c55fe7c625 100644 --- a/ui/src/api/agents.ts +++ b/ui/src/api/agents.ts @@ -223,6 +223,7 @@ export const agentsApi = { type: string, data: { adapterConfig: Record; + agentId?: string; testCredentials?: Record; environmentId?: string | null; }, diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 1e22442fcc..91534ffb16 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -1054,15 +1054,16 @@ export function AgentConfigForm(props: AgentConfigFormProps) { visibleEnvironmentIds: environmentList.map((environment) => environment.id), }); const adapterConfig = buildAdapterConfigForTest(adapterConfigPatch); + const agentId = isCreate ? undefined : props.agent.id; if (props.compactTestFeedback) { const providerAdapter = adapterType === "paperclip_runner" ? adapterConfig.provider === "codex" ? "codex_local" : adapterConfig.provider === "acpx" && adapterConfig.acpxAgent === "claude" ? "claude_local" : adapterType : adapterType; - return testAgentSetup({ companyId: selectedCompanyId, adapterType, providerAdapter, adapterConfig, environmentId }); + return testAgentSetup({ companyId: selectedCompanyId, adapterType, providerAdapter, adapterConfig, agentId, environmentId }); } - return agentsApi.testEnvironment(selectedCompanyId, adapterType, { adapterConfig, environmentId }); + return agentsApi.testEnvironment(selectedCompanyId, adapterType, { adapterConfig, agentId, environmentId }); }, }); const [testActionPending, setTestActionPending] = useState(false); diff --git a/ui/src/lib/test-agent-setup.test.ts b/ui/src/lib/test-agent-setup.test.ts index 015d7ab27c..34e926fa4a 100644 --- a/ui/src/lib/test-agent-setup.test.ts +++ b/ui/src/lib/test-agent-setup.test.ts @@ -4,6 +4,7 @@ const testEnvironment = vi.hoisted(() => vi.fn()); vi.mock("../api/agents", () => ({ agentsApi: { testEnvironment } })); const input = { companyId: "company-1", + agentId: "agent-1", adapterType: "paperclip_runner", providerAdapter: "claude_local", environmentId: "sandbox-1", @@ -48,6 +49,7 @@ it("does not report a connection when runtime readiness passes but provider auth "company-1", "claude_local", { + agentId: "agent-1", environmentId: "sandbox-1", adapterConfig: { ...input.adapterConfig, engine: "cli" }, }, diff --git a/ui/src/lib/test-agent-setup.ts b/ui/src/lib/test-agent-setup.ts index 81b9f1aa80..8d0f2dd2e2 100644 --- a/ui/src/lib/test-agent-setup.ts +++ b/ui/src/lib/test-agent-setup.ts @@ -9,11 +9,13 @@ export async function testAgentSetup(input: { adapterType: string; providerAdapter: string; adapterConfig: Record; + agentId?: string; testCredentials?: Record; environmentId: string | null; }): Promise { const payload = { adapterConfig: input.adapterConfig, + ...(input.agentId ? { agentId: input.agentId } : {}), ...(input.testCredentials ? { testCredentials: input.testCredentials } : {}), environmentId: input.environmentId, };