d1e6662ed8
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `codex_local` adapter spawns the Codex CLI for an agent, and `server/src/routes/agents.ts` normalizes each agent's `adapterConfig.env` on create/hire/update > - PR #8272 added an isolation guard that, on every codex_local agent, force-set a per-agent `CODEX_HOME` and injected `OPENAI_API_KEY = ""`, and rejected any "shared" home > - This broke the common case: operators who deleted `CODEX_HOME` / `OPENAI_API_KEY` in the UI saw them silently re-appear on save, and every agent was forced into an isolated home instead of sharing the device's existing Codex login (`~/.codex` / `$CODEX_HOME`) > - This pull request replaces the always-on isolation guard with key-scoped isolation: a keyless agent gets no env overrides and inherits the host Codex login at runtime; we only carve out an isolated per-agent `CODEX_HOME` when the agent sets its own `OPENAI_API_KEY` > - The benefit is that env-var deletion now persists, agents on one device share the host login by default, and per-account isolation is still available by setting a per-agent key ## Linked Issues or Issue Description No public GitHub issue exists. Bug report (following `bug_report.yml`): **What happened?** In a `codex_local` agent's configuration, removing the `CODEX_HOME` and `OPENAI_API_KEY` env vars via the UI (clicking the X) appears to work, but on save they instantly re-appear. The persisted config never loses the slots. **Root cause.** `applyCodexLocalIsolationGuard` in `server/src/routes/agents.ts` (added in #8272) re-injected `CODEX_HOME` (defaulting to a per-agent home) and `OPENAI_API_KEY = ""` on every create/hire/update, and rejected shared homes outright. So a PATCH that omitted those keys had them written back server-side. **Expected behavior.** Deleting these env vars should persist. A codex_local agent with no key should inherit whatever Codex login already exists on the device. **Steps to reproduce.** 1. Open a `codex_local` agent's configuration with `CODEX_HOME` and `OPENAI_API_KEY` set. 2. Remove both env vars and save. 3. Re-open the config — both slots are back. Related (different approach): Refs #8399 (keeps per-agent isolation, fixes only the empty `OPENAI_API_KEY` slot), Refs #8272 (introduced the guard), Refs #8403 (managed-auth seeding into isolated homes). ## What Changed - `server/src/routes/agents.ts` — replaced `applyCodexLocalIsolationGuard` (+ `assertCodexLocalHomeIsNotShared` / `normalizeCodexLocalHomePath`) with `applyCodexLocalKeyIsolation`. A codex_local agent now receives **no** env overrides unless it explicitly sets `OPENAI_API_KEY`; only then is an isolated per-agent `CODEX_HOME` injected (and only if the agent has not set its own `CODEX_HOME`). Keyless agents fall back to the host Codex login at runtime. Dropped the now-unused `node:os` import and the shared-home rejection. - `server/src/__tests__/agent-adapter-validation-routes.test.ts` — updated the validation-route tests to assert env-var deletion persists, keyless agents get no overrides, and key-bearing agents still get an isolated `CODEX_HOME`. ## Verification ```bash cd server && npx vitest run src/__tests__/agent-adapter-validation-routes.test.ts ``` All 6 tests pass locally. Manually verified in the live UI: removing `CODEX_HOME` and `OPENAI_API_KEY` from a codex_local agent and saving now persists the deletion (slots no longer re-appear on reload). ## Risks - **Host-key leak for keyless agents on a host with `OPENAI_API_KEY` set.** Low/intended: the product direction here is that codex_local agents inherit the host's Codex login by default; per-account isolation is opt-in via a per-agent `OPENAI_API_KEY`, which then gets its own `CODEX_HOME`. - **Divergence from #8399.** That PR retains the per-agent isolation default and only stops injecting the empty key, so it does not address the `CODEX_HOME` re-injection half of this bug. This PR intentionally changes the default to host-login inheritance. Reviewers should pick one direction. - **No migration.** Existing agents that already store these slots are not auto-cleaned, but operators can now delete them and the deletion sticks. ## Model Used - Provider: Anthropic (Claude) - Model ID: `claude-opus-4-8` - Capabilities: tool use, code execution, extended reasoning, agentic workflow ## 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 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 - [ ] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge
425 lines
14 KiB
TypeScript
425 lines
14 KiB
TypeScript
import express from "express";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import request from "supertest";
|
|
import { beforeEach, afterEach, describe, expect, it, vi } from "vitest";
|
|
import type { ServerAdapterModule } from "../adapters/index.js";
|
|
|
|
const mockAgentService = vi.hoisted(() => ({
|
|
create: vi.fn(),
|
|
getById: vi.fn(),
|
|
update: vi.fn(),
|
|
}));
|
|
|
|
const mockAccessService = vi.hoisted(() => ({
|
|
canUser: vi.fn(),
|
|
decide: vi.fn(),
|
|
hasPermission: vi.fn(),
|
|
ensureMembership: vi.fn(),
|
|
setPrincipalPermission: vi.fn(),
|
|
}));
|
|
|
|
const mockCompanySkillService = vi.hoisted(() => ({
|
|
listRuntimeSkillEntries: vi.fn(),
|
|
resolveRequestedSkillKeys: vi.fn(),
|
|
}));
|
|
|
|
const mockSecretService = vi.hoisted(() => ({
|
|
normalizeAdapterConfigForPersistence: vi.fn(async (_companyId: string, config: Record<string, unknown>) => config),
|
|
resolveAdapterConfigForRuntime: vi.fn(async (_companyId: string, config: Record<string, unknown>) => ({ config })),
|
|
syncEnvBindingsForTarget: vi.fn(),
|
|
}));
|
|
|
|
const mockAgentInstructionsService = vi.hoisted(() => ({
|
|
materializeManagedBundle: vi.fn(),
|
|
getBundle: vi.fn(),
|
|
readFile: vi.fn(),
|
|
updateBundle: vi.fn(),
|
|
writeFile: vi.fn(),
|
|
deleteFile: vi.fn(),
|
|
exportFiles: vi.fn(),
|
|
ensureManagedBundle: vi.fn(),
|
|
}));
|
|
|
|
const mockBudgetService = vi.hoisted(() => ({
|
|
upsertPolicy: vi.fn(),
|
|
}));
|
|
|
|
const mockHeartbeatService = vi.hoisted(() => ({
|
|
cancelActiveForAgent: vi.fn(),
|
|
}));
|
|
|
|
const mockIssueApprovalService = vi.hoisted(() => ({
|
|
linkManyForApproval: vi.fn(),
|
|
}));
|
|
|
|
const mockApprovalService = vi.hoisted(() => ({
|
|
create: vi.fn(),
|
|
getById: vi.fn(),
|
|
}));
|
|
|
|
const mockInstanceSettingsService = vi.hoisted(() => ({
|
|
getGeneral: vi.fn(async () => ({ censorUsernameInLogs: false })),
|
|
}));
|
|
|
|
const mockLogActivity = vi.hoisted(() => vi.fn());
|
|
|
|
vi.mock("../services/index.js", () => ({
|
|
agentService: () => mockAgentService,
|
|
agentInstructionsService: () => mockAgentInstructionsService,
|
|
accessService: () => mockAccessService,
|
|
approvalService: () => mockApprovalService,
|
|
companySkillService: () => mockCompanySkillService,
|
|
budgetService: () => mockBudgetService,
|
|
heartbeatService: () => mockHeartbeatService,
|
|
issueApprovalService: () => mockIssueApprovalService,
|
|
issueService: () => ({}),
|
|
logActivity: mockLogActivity,
|
|
secretService: () => mockSecretService,
|
|
syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config),
|
|
workspaceOperationService: () => ({}),
|
|
}));
|
|
|
|
vi.mock("../services/instance-settings.js", () => ({
|
|
instanceSettingsService: () => mockInstanceSettingsService,
|
|
}));
|
|
|
|
vi.mock("../services/secrets.js", () => ({
|
|
secretService: () => mockSecretService,
|
|
}));
|
|
|
|
function registerModuleMocks() {
|
|
vi.doMock("../services/index.js", () => ({
|
|
agentService: () => mockAgentService,
|
|
agentInstructionsService: () => mockAgentInstructionsService,
|
|
accessService: () => mockAccessService,
|
|
approvalService: () => mockApprovalService,
|
|
companySkillService: () => mockCompanySkillService,
|
|
budgetService: () => mockBudgetService,
|
|
heartbeatService: () => mockHeartbeatService,
|
|
issueApprovalService: () => mockIssueApprovalService,
|
|
issueService: () => ({}),
|
|
logActivity: mockLogActivity,
|
|
secretService: () => mockSecretService,
|
|
syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config),
|
|
workspaceOperationService: () => ({}),
|
|
}));
|
|
|
|
vi.doMock("../services/instance-settings.js", () => ({
|
|
instanceSettingsService: () => mockInstanceSettingsService,
|
|
}));
|
|
|
|
vi.doMock("../services/secrets.js", () => ({
|
|
secretService: () => mockSecretService,
|
|
}));
|
|
}
|
|
|
|
const externalAdapter: ServerAdapterModule = {
|
|
type: "external_test",
|
|
execute: async () => ({ exitCode: 0, signal: null, timedOut: false }),
|
|
testEnvironment: async () => ({
|
|
adapterType: "external_test",
|
|
status: "pass",
|
|
checks: [],
|
|
testedAt: new Date(0).toISOString(),
|
|
}),
|
|
};
|
|
|
|
const missingAdapterType = "missing_adapter_validation_test";
|
|
|
|
async function createApp() {
|
|
const [{ agentRoutes }, { errorHandler }] = await Promise.all([
|
|
vi.importActual<typeof import("../routes/agents.js")>("../routes/agents.js"),
|
|
vi.importActual<typeof import("../middleware/index.js")>("../middleware/index.js"),
|
|
]);
|
|
const app = express();
|
|
app.use(express.json());
|
|
app.use((req, _res, next) => {
|
|
(req as any).actor = {
|
|
type: "board",
|
|
userId: "local-board",
|
|
companyIds: ["company-1"],
|
|
source: "local_implicit",
|
|
isInstanceAdmin: false,
|
|
};
|
|
next();
|
|
});
|
|
const db = {
|
|
select: vi.fn(() => ({
|
|
from: vi.fn(() => ({
|
|
where: vi.fn(async () => [
|
|
{
|
|
id: "company-1",
|
|
requireBoardApprovalForNewAgents: false,
|
|
},
|
|
]),
|
|
})),
|
|
})),
|
|
};
|
|
app.use("/api", agentRoutes(db as any));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
async function requestApp(
|
|
app: express.Express,
|
|
buildRequest: (baseUrl: string) => request.Test,
|
|
) {
|
|
const { createServer } = await vi.importActual<typeof import("node:http")>("node:http");
|
|
const server = createServer(app);
|
|
try {
|
|
await new Promise<void>((resolve) => {
|
|
server.listen(0, "127.0.0.1", resolve);
|
|
});
|
|
const address = server.address();
|
|
if (!address || typeof address === "string") {
|
|
throw new Error("Expected HTTP server to listen on a TCP port");
|
|
}
|
|
return await buildRequest(`http://127.0.0.1:${address.port}`);
|
|
} finally {
|
|
if (server.listening) {
|
|
await new Promise<void>((resolve, reject) => {
|
|
server.close((error) => {
|
|
if (error) reject(error);
|
|
else resolve();
|
|
});
|
|
});
|
|
}
|
|
}
|
|
}
|
|
|
|
async function unregisterTestAdapter(type: string) {
|
|
const { unregisterServerAdapter } = await import("../adapters/index.js");
|
|
unregisterServerAdapter(type);
|
|
}
|
|
|
|
describe("agent routes adapter validation", () => {
|
|
beforeEach(async () => {
|
|
vi.resetModules();
|
|
vi.doUnmock("../routes/agents.js");
|
|
vi.doUnmock("../routes/authz.js");
|
|
vi.doUnmock("../middleware/index.js");
|
|
vi.doUnmock("../routes/agents.js");
|
|
registerModuleMocks();
|
|
vi.clearAllMocks();
|
|
mockCompanySkillService.listRuntimeSkillEntries.mockResolvedValue([]);
|
|
mockCompanySkillService.resolveRequestedSkillKeys.mockResolvedValue([]);
|
|
mockAccessService.canUser.mockResolvedValue(true);
|
|
mockAccessService.decide.mockResolvedValue({
|
|
allowed: true,
|
|
reason: "allow_explicit_grant",
|
|
explanation: "Allowed by test grant",
|
|
});
|
|
mockAccessService.hasPermission.mockResolvedValue(true);
|
|
mockAccessService.ensureMembership.mockResolvedValue(undefined);
|
|
mockAccessService.setPrincipalPermission.mockResolvedValue(undefined);
|
|
mockLogActivity.mockResolvedValue(undefined);
|
|
mockSecretService.syncEnvBindingsForTarget.mockResolvedValue(undefined);
|
|
mockAgentInstructionsService.materializeManagedBundle.mockImplementation(async (agent: { adapterConfig: unknown }) => ({
|
|
adapterConfig: agent.adapterConfig,
|
|
}));
|
|
mockAgentService.create.mockImplementation(async (_companyId: string, input: Record<string, unknown>) => ({
|
|
id: String(input.id ?? "11111111-1111-4111-8111-111111111111"),
|
|
companyId: "company-1",
|
|
name: String(input.name ?? "Agent"),
|
|
urlKey: "agent",
|
|
role: String(input.role ?? "general"),
|
|
title: null,
|
|
icon: null,
|
|
status: "idle",
|
|
reportsTo: null,
|
|
capabilities: null,
|
|
adapterType: String(input.adapterType ?? "process"),
|
|
adapterConfig: (input.adapterConfig as Record<string, unknown> | undefined) ?? {},
|
|
runtimeConfig: (input.runtimeConfig as Record<string, unknown> | undefined) ?? {},
|
|
budgetMonthlyCents: 0,
|
|
spentMonthlyCents: 0,
|
|
pauseReason: null,
|
|
pausedAt: null,
|
|
permissions: { canCreateAgents: false },
|
|
lastHeartbeatAt: null,
|
|
metadata: null,
|
|
createdAt: new Date(),
|
|
updatedAt: new Date(),
|
|
}));
|
|
mockAgentService.getById.mockResolvedValue({
|
|
id: "11111111-1111-4111-8111-111111111111",
|
|
companyId: "company-1",
|
|
name: "Codex",
|
|
urlKey: "codex",
|
|
role: "engineer",
|
|
title: null,
|
|
icon: null,
|
|
status: "idle",
|
|
reportsTo: null,
|
|
capabilities: null,
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
runtimeConfig: {},
|
|
budgetMonthlyCents: 0,
|
|
spentMonthlyCents: 0,
|
|
pauseReason: null,
|
|
pausedAt: null,
|
|
permissions: { canCreateAgents: false },
|
|
lastHeartbeatAt: null,
|
|
metadata: null,
|
|
createdAt: new Date(),
|
|
updatedAt: new Date(),
|
|
});
|
|
mockAgentService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
|
...(await mockAgentService.getById()),
|
|
...patch,
|
|
}));
|
|
await unregisterTestAdapter("external_test");
|
|
await unregisterTestAdapter(missingAdapterType);
|
|
});
|
|
|
|
afterEach(async () => {
|
|
await unregisterTestAdapter("external_test");
|
|
await unregisterTestAdapter(missingAdapterType);
|
|
});
|
|
|
|
it("creates agents for dynamically registered external adapter types", 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/agents")
|
|
.send({
|
|
name: "External Agent",
|
|
adapterType: "external_test",
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(201);
|
|
expect(res.body.adapterType).toBe("external_test");
|
|
});
|
|
|
|
it("does not inject CODEX_HOME or OPENAI_API_KEY when creating a keyless codex_local agent", async () => {
|
|
const app = await createApp();
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "Codex Agent",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(201);
|
|
const createInput = mockAgentService.create.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
|
const adapterConfig = createInput.adapterConfig as Record<string, unknown>;
|
|
const env = (adapterConfig.env as Record<string, unknown> | undefined) ?? {};
|
|
expect(env.OPENAI_API_KEY).toBeUndefined();
|
|
expect(env.CODEX_HOME).toBeUndefined();
|
|
});
|
|
|
|
it("does not re-inject CODEX_HOME or OPENAI_API_KEY when updating a keyless codex_local agent", async () => {
|
|
const app = await createApp();
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.patch("/api/agents/11111111-1111-4111-8111-111111111111")
|
|
.send({
|
|
adapterConfig: { model: "gpt-5.4" },
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
|
const patch = mockAgentService.update.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
|
const adapterConfig = patch.adapterConfig as Record<string, unknown>;
|
|
const env = (adapterConfig.env as Record<string, unknown> | undefined) ?? {};
|
|
expect(env.OPENAI_API_KEY).toBeUndefined();
|
|
expect(env.CODEX_HOME).toBeUndefined();
|
|
});
|
|
|
|
it("isolates CODEX_HOME when updating a codex_local agent to set its own OPENAI_API_KEY", async () => {
|
|
const agentId = "11111111-1111-4111-8111-111111111111";
|
|
const app = await createApp();
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.patch(`/api/agents/${agentId}`)
|
|
.send({
|
|
adapterConfig: {
|
|
env: {
|
|
OPENAI_API_KEY: "sk-test-key",
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
|
const patch = mockAgentService.update.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
|
const adapterConfig = patch.adapterConfig as Record<string, unknown>;
|
|
const env = adapterConfig.env as Record<string, unknown>;
|
|
expect(env.OPENAI_API_KEY).toBe("sk-test-key");
|
|
expect(String(env.CODEX_HOME)).toContain(`/companies/company-1/agents/${agentId}/codex-home`);
|
|
});
|
|
|
|
it("allows codex_local agents to share the host Codex home", async () => {
|
|
const app = await createApp();
|
|
const sharedHome = path.join(os.homedir(), ".codex");
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "Shared Codex",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {
|
|
env: {
|
|
CODEX_HOME: sharedHome,
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(201);
|
|
const createInput = mockAgentService.create.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
|
const adapterConfig = createInput.adapterConfig as Record<string, unknown>;
|
|
const env = adapterConfig.env as Record<string, unknown>;
|
|
expect(env.CODEX_HOME).toBe(sharedHome);
|
|
});
|
|
|
|
it("isolates CODEX_HOME when a codex_local agent sets its own OPENAI_API_KEY", async () => {
|
|
const app = await createApp();
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "Keyed Codex",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {
|
|
env: {
|
|
OPENAI_API_KEY: "sk-test-key",
|
|
},
|
|
},
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(201);
|
|
const createInput = mockAgentService.create.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
|
const agentId = String(createInput.id);
|
|
const adapterConfig = createInput.adapterConfig as Record<string, unknown>;
|
|
const env = adapterConfig.env as Record<string, unknown>;
|
|
expect(env.OPENAI_API_KEY).toBe("sk-test-key");
|
|
expect(String(env.CODEX_HOME)).toContain(`/companies/company-1/agents/${agentId}/codex-home`);
|
|
});
|
|
|
|
it("rejects unknown adapter types even when schema accepts arbitrary strings", async () => {
|
|
const app = await createApp();
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post("/api/companies/company-1/agents")
|
|
.send({
|
|
name: "Missing Adapter",
|
|
adapterType: missingAdapterType,
|
|
}),
|
|
);
|
|
|
|
expect(res.status, JSON.stringify(res.body)).toBe(422);
|
|
expect(String(res.body.error ?? res.body.message ?? "")).toContain(`Unknown adapter type: ${missingAdapterType}`);
|
|
});
|
|
});
|