mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 21:59:38 +01:00
## Thinking Path > - Paperclip is the control plane people use to create, configure, and run AI agents for work. > - This change sits in the server-side agent lifecycle and secret-binding subsystem, where adapter config `env` entries can reference company secrets. > - An incident (while trying to configure a Novita sandbox) showed that an agent can reach a broken runtime state if `adapterConfig.env` contains `secret_ref` entries but the matching `company_secret_bindings` rows are missing. > - The immediate run-path guard and error-surfacing work made the failure diagnosable, but they did not fully prevent new broken agents from being created. > - The risk came from create and approval flows being responsible for remembering to sync bindings at each call site, which is easy to miss as new flows are added. > - This pull request moves the invariant into `agentService` create/update/activate paths, keeps the existing hire-flow fix, and adds regression coverage for create, update, and legacy pending-approval recovery. > - The benefit is that agent secret binding integrity is enforced closer to the data mutation point, so future callers inherit the protection automatically. ## Linked Issues or Issue Description Refs #8309 ### What happened? A Paperclip agent could persist `adapterConfig.env` `secret_ref` entries without matching agent-scoped `company_secret_bindings` rows. When that happened, the config UI could still look configured, but the real run path failed pre-dispatch because the secret was not actually bound to that agent. ### Expected behavior Every normal agent create, config-update, and pending-approval activation flow should leave the agent with secret bindings that match its persisted secret-ref env config. ### Steps to reproduce 1. Create or activate an agent through a flow that persists `adapterConfig.env` secret refs without synchronizing `company_secret_bindings`. 2. Observe that the config state can still appear populated. 3. Start a run for that agent. 4. Observe that pre-dispatch binding validation fails because the secret reference exists but the agent binding does not. ### Deployment mode Local dev (`pnpm dev`) ### Installation method Built from source (`pnpm dev` / `pnpm build`) ### Agent adapter(s) involved - Claude Code - Not adapter-specific (core bug) ### Database mode Embedded PGlite / embedded local dev database flow ### Access context Board (human operator) created or approved the agent; agent runtime later consumed the config. ### Additional context This PR focuses on preventing new broken states from normal service flows and on backfilling the covered legacy pending-approval activation path. ## What Changed - Kept the existing branch-local hire-flow fix that synchronized bindings for route and approval paths. - Moved the binding integrity invariant into `agentService.create()`, `agentService.update()` when `adapterConfig` changes, and `agentService.activatePendingApproval()`. - Added `server/src/__tests__/agents-service-secret-bindings.test.ts` covering create-time sync, update-time resync, and backfill for legacy pending-approval agents. - Removed now-redundant route-layer and approval-layer binding sync calls once the service layer became authoritative. - Simplified the affected unit tests so route/approval tests no longer assert service-owned binding writes directly. ## Verification - `pnpm --filter @paperclipai/server typecheck` - `pnpm exec vitest run server/src/__tests__/agents-service-secret-bindings.test.ts server/src/__tests__/approvals-service.test.ts server/src/__tests__/agent-skills-routes.test.ts` ## Risks - Low to medium risk. - This changes where secret-binding synchronization is enforced, so any unexpected caller that relied on upper-layer manual sync behavior could behave differently. - Agent create/update/activation flows now perform binding synchronization consistently, which adds binding-table writes at those mutation points. - This PR does not retroactively scan and heal every already-broken historical agent row; it prevents and backfills through the covered service flows. > 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 / GPT-5 Codex class model via `codex_local` - Session model family: GPT-5 Codex - Tool-assisted coding with shell, git, HTTP, and local test execution - Reasoning mode: medium interactive tool-use 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] 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 - [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>
239 lines
6.8 KiB
TypeScript
239 lines
6.8 KiB
TypeScript
import { randomUUID } from "node:crypto";
|
|
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
|
import { eq } from "drizzle-orm";
|
|
import {
|
|
agents,
|
|
agentRuntimeState,
|
|
companies,
|
|
createDb,
|
|
heartbeatRunEvents,
|
|
heartbeatRuns,
|
|
} from "@paperclipai/db";
|
|
import {
|
|
getEmbeddedPostgresTestSupport,
|
|
startEmbeddedPostgresTestDatabase,
|
|
} from "./helpers/embedded-postgres.js";
|
|
import { agentService } from "../services/agents.ts";
|
|
|
|
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
|
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
|
|
|
if (!embeddedPostgresSupport.supported) {
|
|
console.warn(
|
|
`Skipping embedded Postgres agent service tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
|
);
|
|
}
|
|
|
|
describeEmbeddedPostgres("agent service clearError", () => {
|
|
let db!: ReturnType<typeof createDb>;
|
|
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
|
|
|
beforeAll(async () => {
|
|
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-agent-clear-error-");
|
|
db = createDb(tempDb.connectionString);
|
|
}, 20_000);
|
|
|
|
afterEach(async () => {
|
|
await db.delete(heartbeatRunEvents);
|
|
await db.delete(agentRuntimeState);
|
|
await db.delete(heartbeatRuns);
|
|
await db.delete(agents);
|
|
await db.delete(companies);
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await tempDb?.cleanup();
|
|
});
|
|
|
|
it("moves an error agent to idle without deleting run history or runtime diagnostics", async () => {
|
|
const companyId = randomUUID();
|
|
const agentId = randomUUID();
|
|
const runId = randomUUID();
|
|
const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`;
|
|
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Paperclip",
|
|
issuePrefix,
|
|
requireBoardApprovalForNewAgents: false,
|
|
});
|
|
|
|
await db.insert(agents).values({
|
|
id: agentId,
|
|
companyId,
|
|
name: "CodexCoder",
|
|
role: "engineer",
|
|
status: "error",
|
|
pauseReason: "system",
|
|
pausedAt: new Date("2026-06-07T00:00:00.000Z"),
|
|
errorReason: "Secret is not bound to agent at env.ANTHROPIC_API_KEY",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
runtimeConfig: {},
|
|
permissions: {},
|
|
});
|
|
|
|
await db.insert(heartbeatRuns).values({
|
|
id: runId,
|
|
companyId,
|
|
agentId,
|
|
invocationSource: "on_demand",
|
|
status: "failed",
|
|
error: "Adapter exited with code 1",
|
|
stdoutExcerpt: "stdout stays inspectable",
|
|
stderrExcerpt: "stderr stays inspectable",
|
|
logStore: "local_disk",
|
|
logRef: "runs/failed.log",
|
|
resultJson: { sessionId: "codex-session-1" },
|
|
finishedAt: new Date("2026-06-07T00:01:00.000Z"),
|
|
});
|
|
|
|
await db.insert(heartbeatRunEvents).values({
|
|
companyId,
|
|
runId,
|
|
agentId,
|
|
seq: 1,
|
|
eventType: "transcript",
|
|
stream: "stderr",
|
|
message: "transcript stays inspectable",
|
|
payload: { itemType: "error" },
|
|
});
|
|
|
|
await db.insert(agentRuntimeState).values({
|
|
agentId,
|
|
companyId,
|
|
adapterType: "codex_local",
|
|
sessionId: "codex-session-1",
|
|
stateJson: { taskKey: "issue:test" },
|
|
lastRunId: runId,
|
|
lastRunStatus: "failed",
|
|
lastError: "Adapter exited with code 1",
|
|
});
|
|
|
|
const cleared = await agentService(db).clearError(agentId);
|
|
|
|
expect(cleared).toMatchObject({
|
|
id: agentId,
|
|
status: "idle",
|
|
pauseReason: null,
|
|
pausedAt: null,
|
|
errorReason: null,
|
|
});
|
|
|
|
const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId));
|
|
expect(run).toMatchObject({
|
|
id: runId,
|
|
status: "failed",
|
|
error: "Adapter exited with code 1",
|
|
stdoutExcerpt: "stdout stays inspectable",
|
|
stderrExcerpt: "stderr stays inspectable",
|
|
logStore: "local_disk",
|
|
logRef: "runs/failed.log",
|
|
});
|
|
|
|
const transcriptEvents = await db
|
|
.select()
|
|
.from(heartbeatRunEvents)
|
|
.where(eq(heartbeatRunEvents.runId, runId));
|
|
expect(transcriptEvents).toHaveLength(1);
|
|
expect(transcriptEvents[0]).toMatchObject({
|
|
runId,
|
|
eventType: "transcript",
|
|
stream: "stderr",
|
|
message: "transcript stays inspectable",
|
|
payload: { itemType: "error" },
|
|
});
|
|
|
|
const [runtimeState] = await db
|
|
.select()
|
|
.from(agentRuntimeState)
|
|
.where(eq(agentRuntimeState.agentId, agentId));
|
|
expect(runtimeState).toMatchObject({
|
|
agentId,
|
|
sessionId: "codex-session-1",
|
|
stateJson: { taskKey: "issue:test" },
|
|
lastRunId: runId,
|
|
lastRunStatus: "failed",
|
|
lastError: "Adapter exited with code 1",
|
|
});
|
|
});
|
|
|
|
it("rejects non-error agents with a 409 conflict", async () => {
|
|
const companyId = randomUUID();
|
|
const agentId = randomUUID();
|
|
const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`;
|
|
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Paperclip",
|
|
issuePrefix,
|
|
requireBoardApprovalForNewAgents: false,
|
|
});
|
|
|
|
await db.insert(agents).values({
|
|
id: agentId,
|
|
companyId,
|
|
name: "CodexCoder",
|
|
role: "engineer",
|
|
status: "idle",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
runtimeConfig: {},
|
|
permissions: {},
|
|
});
|
|
|
|
await expect(agentService(db).clearError(agentId)).rejects.toMatchObject({
|
|
status: 409,
|
|
message: "Only agents in error status can have their error cleared",
|
|
});
|
|
});
|
|
|
|
it("keeps resume-style terminal and pending-approval protections", async () => {
|
|
const companyId = randomUUID();
|
|
const terminatedAgentId = randomUUID();
|
|
const pendingAgentId = randomUUID();
|
|
const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`;
|
|
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Paperclip",
|
|
issuePrefix,
|
|
requireBoardApprovalForNewAgents: false,
|
|
});
|
|
|
|
await db.insert(agents).values([
|
|
{
|
|
id: terminatedAgentId,
|
|
companyId,
|
|
name: "Terminated",
|
|
role: "engineer",
|
|
status: "terminated",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
runtimeConfig: {},
|
|
permissions: {},
|
|
},
|
|
{
|
|
id: pendingAgentId,
|
|
companyId,
|
|
name: "Pending",
|
|
role: "engineer",
|
|
status: "pending_approval",
|
|
adapterType: "codex_local",
|
|
adapterConfig: {},
|
|
runtimeConfig: {},
|
|
permissions: {},
|
|
},
|
|
]);
|
|
|
|
await expect(agentService(db).clearError(terminatedAgentId)).rejects.toMatchObject({
|
|
status: 409,
|
|
message: "Cannot clear error on terminated agent",
|
|
});
|
|
await expect(agentService(db).clearError(pendingAgentId)).rejects.toMatchObject({
|
|
status: 409,
|
|
message: "Pending approval agents cannot have errors cleared",
|
|
});
|
|
});
|
|
});
|