Fix Codex API key authentication in tests and runs (#13260)
## 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 <noreply@paperclip.ing>
This commit is contained in:
parent
3bafac12f7
commit
ad4f0b5867
|
|
@ -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<string, string> }).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<string, string> }).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 () => {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -242,6 +242,8 @@ export const resetAgentSessionSchema = z.object({
|
|||
export type ResetAgentSession = z.infer<typeof resetAgentSessionSchema>;
|
||||
|
||||
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),
|
||||
|
|
|
|||
|
|
@ -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) =>
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -223,6 +223,7 @@ export const agentsApi = {
|
|||
type: string,
|
||||
data: {
|
||||
adapterConfig: Record<string, unknown>;
|
||||
agentId?: string;
|
||||
testCredentials?: Record<string, string>;
|
||||
environmentId?: string | null;
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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" },
|
||||
},
|
||||
|
|
|
|||
|
|
@ -9,11 +9,13 @@ export async function testAgentSetup(input: {
|
|||
adapterType: string;
|
||||
providerAdapter: string;
|
||||
adapterConfig: Record<string, unknown>;
|
||||
agentId?: string;
|
||||
testCredentials?: Record<string, string>;
|
||||
environmentId: string | null;
|
||||
}): Promise<AdapterEnvironmentTestResult> {
|
||||
const payload = {
|
||||
adapterConfig: input.adapterConfig,
|
||||
...(input.agentId ? { agentId: input.agentId } : {}),
|
||||
...(input.testCredentials ? { testCredentials: input.testCredentials } : {}),
|
||||
environmentId: input.environmentId,
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue