fix(server): enforce agent secret binding sync across lifecycle flows (#8307)
## 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>
This commit is contained in:
Devin Foley
@@ -24,8 +24,10 @@ import {
|
||||
type AgentEligibilityAgent,
|
||||
} from "@paperclipai/shared";
|
||||
import { conflict, notFound, unprocessable } from "../errors.js";
|
||||
import { syncAgentAdapterEnvBindings } from "./agent-secret-bindings.js";
|
||||
import { normalizeAgentPermissions } from "./agent-permissions.js";
|
||||
import { REDACTED_EVENT_VALUE, sanitizeRecord } from "../redaction.js";
|
||||
import { secretService } from "./secrets.js";
|
||||
|
||||
function hashToken(token: string) {
|
||||
return createHash("sha256").update(token).digest("hex");
|
||||
@@ -218,6 +220,8 @@ export function deduplicateAgentName(
|
||||
}
|
||||
|
||||
export function agentService(db: Db) {
|
||||
const secretsSvc = secretService(db);
|
||||
|
||||
function currentUtcMonthWindow(now = new Date()) {
|
||||
const year = now.getUTCFullYear();
|
||||
const month = now.getUTCMonth();
|
||||
@@ -371,6 +375,19 @@ export function agentService(db: Db) {
|
||||
}
|
||||
}
|
||||
|
||||
async function syncAgentSecretBindings(
|
||||
agent: { id: string; companyId: string; adapterConfig: unknown },
|
||||
dbClient: Db = db,
|
||||
) {
|
||||
const scopedSecretsSvc = dbClient === db ? secretsSvc : secretService(dbClient);
|
||||
await syncAgentAdapterEnvBindings({
|
||||
secretsSvc: scopedSecretsSvc,
|
||||
companyId: agent.companyId,
|
||||
agentId: agent.id,
|
||||
adapterConfig: agent.adapterConfig,
|
||||
});
|
||||
}
|
||||
|
||||
async function updateAgent(
|
||||
id: string,
|
||||
data: Partial<typeof agents.$inferInsert>,
|
||||
@@ -415,33 +432,45 @@ export function agentService(db: Db) {
|
||||
const shouldRecordRevision = Boolean(options?.recordRevision) && hasConfigPatchFields(normalizedPatch);
|
||||
const beforeConfig = shouldRecordRevision ? buildConfigSnapshot(existing) : null;
|
||||
|
||||
const updated = await db
|
||||
.update(agents)
|
||||
.set({ ...normalizedPatch, updatedAt: new Date() })
|
||||
.where(eq(agents.id, id))
|
||||
.returning()
|
||||
.then((rows) => rows[0] ?? null);
|
||||
const normalizedUpdated = updated ? await getById(updated.id) : null;
|
||||
return db.transaction(async (tx) => {
|
||||
const txDb = tx as unknown as Db;
|
||||
const updated = await tx
|
||||
.update(agents)
|
||||
.set({ ...normalizedPatch, updatedAt: new Date() })
|
||||
.where(eq(agents.id, id))
|
||||
.returning()
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!updated) return null;
|
||||
|
||||
if (normalizedUpdated && shouldRecordRevision && beforeConfig) {
|
||||
const afterConfig = buildConfigSnapshot(normalizedUpdated);
|
||||
const changedKeys = diffConfigSnapshot(beforeConfig, afterConfig);
|
||||
if (changedKeys.length > 0) {
|
||||
await db.insert(agentConfigRevisions).values({
|
||||
companyId: normalizedUpdated.companyId,
|
||||
agentId: normalizedUpdated.id,
|
||||
createdByAgentId: options?.recordRevision?.createdByAgentId ?? null,
|
||||
createdByUserId: options?.recordRevision?.createdByUserId ?? null,
|
||||
source: options?.recordRevision?.source ?? "patch",
|
||||
rolledBackFromRevisionId: options?.recordRevision?.rolledBackFromRevisionId ?? null,
|
||||
changedKeys,
|
||||
beforeConfig: beforeConfig as unknown as Record<string, unknown>,
|
||||
afterConfig: afterConfig as unknown as Record<string, unknown>,
|
||||
});
|
||||
if (Object.prototype.hasOwnProperty.call(normalizedPatch, "adapterConfig")) {
|
||||
await syncAgentSecretBindings(updated, txDb);
|
||||
}
|
||||
}
|
||||
|
||||
return normalizedUpdated;
|
||||
const normalizedUpdated = await agentService(txDb).getById(updated.id);
|
||||
if (!normalizedUpdated) {
|
||||
throw notFound("Agent not found");
|
||||
}
|
||||
|
||||
if (shouldRecordRevision && beforeConfig) {
|
||||
const afterConfig = buildConfigSnapshot(normalizedUpdated);
|
||||
const changedKeys = diffConfigSnapshot(beforeConfig, afterConfig);
|
||||
if (changedKeys.length > 0) {
|
||||
await tx.insert(agentConfigRevisions).values({
|
||||
companyId: normalizedUpdated.companyId,
|
||||
agentId: normalizedUpdated.id,
|
||||
createdByAgentId: options?.recordRevision?.createdByAgentId ?? null,
|
||||
createdByUserId: options?.recordRevision?.createdByUserId ?? null,
|
||||
source: options?.recordRevision?.source ?? "patch",
|
||||
rolledBackFromRevisionId: options?.recordRevision?.rolledBackFromRevisionId ?? null,
|
||||
changedKeys,
|
||||
beforeConfig: beforeConfig as unknown as Record<string, unknown>,
|
||||
afterConfig: afterConfig as unknown as Record<string, unknown>,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
return normalizedUpdated;
|
||||
});
|
||||
}
|
||||
|
||||
return {
|
||||
@@ -474,13 +503,20 @@ export function agentService(db: Db) {
|
||||
const role = data.role ?? "general";
|
||||
const normalizedPermissions = normalizeAgentPermissions(data.permissions, role);
|
||||
const runtimeConfig = normalizeRuntimeConfigForNewAgent(data.runtimeConfig);
|
||||
const created = await db
|
||||
.insert(agents)
|
||||
.values({ ...data, name: uniqueName, companyId, role, permissions: normalizedPermissions, runtimeConfig })
|
||||
.returning()
|
||||
.then((rows) => rows[0]);
|
||||
|
||||
return requireGetById(created.id);
|
||||
return db.transaction(async (tx) => {
|
||||
const txDb = tx as unknown as Db;
|
||||
const created = await tx
|
||||
.insert(agents)
|
||||
.values({ ...data, name: uniqueName, companyId, role, permissions: normalizedPermissions, runtimeConfig })
|
||||
.returning()
|
||||
.then((rows) => rows[0]);
|
||||
await syncAgentSecretBindings(created, txDb);
|
||||
const normalizedCreated = await agentService(txDb).getById(created.id);
|
||||
if (!normalizedCreated) {
|
||||
throw notFound("Agent not found");
|
||||
}
|
||||
return normalizedCreated;
|
||||
});
|
||||
},
|
||||
|
||||
update: updateAgent,
|
||||
@@ -496,6 +532,7 @@ export function agentService(db: Db) {
|
||||
status: "paused",
|
||||
pauseReason: reason,
|
||||
pausedAt: new Date(),
|
||||
errorReason: null,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(eq(agents.id, id))
|
||||
@@ -518,6 +555,7 @@ export function agentService(db: Db) {
|
||||
status: "idle",
|
||||
pauseReason: null,
|
||||
pausedAt: null,
|
||||
errorReason: null,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(eq(agents.id, id))
|
||||
@@ -543,6 +581,7 @@ export function agentService(db: Db) {
|
||||
status: "idle",
|
||||
pauseReason: null,
|
||||
pausedAt: null,
|
||||
errorReason: null,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(and(eq(agents.id, id), eq(agents.status, "error")))
|
||||
@@ -565,6 +604,7 @@ export function agentService(db: Db) {
|
||||
status: "terminated",
|
||||
pauseReason: null,
|
||||
pausedAt: null,
|
||||
errorReason: null,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(eq(agents.id, id));
|
||||
@@ -611,15 +651,25 @@ export function agentService(db: Db) {
|
||||
},
|
||||
|
||||
activatePendingApproval: async (id: string) => {
|
||||
const updated = await db
|
||||
.update(agents)
|
||||
.set({ status: "idle", updatedAt: new Date() })
|
||||
.where(and(eq(agents.id, id), eq(agents.status, "pending_approval")))
|
||||
.returning()
|
||||
.then((rows) => rows[0] ?? null);
|
||||
const activatedAgent = await db.transaction(async (tx) => {
|
||||
const txDb = tx as unknown as Db;
|
||||
const updated = await tx
|
||||
.update(agents)
|
||||
.set({ status: "idle", updatedAt: new Date() })
|
||||
.where(and(eq(agents.id, id), eq(agents.status, "pending_approval")))
|
||||
.returning()
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!updated) return null;
|
||||
await syncAgentSecretBindings(updated, txDb);
|
||||
const agent = await agentService(txDb).getById(updated.id);
|
||||
if (!agent) {
|
||||
throw notFound("Agent not found");
|
||||
}
|
||||
return agent;
|
||||
});
|
||||
|
||||
if (updated) {
|
||||
return { agent: await requireGetById(updated.id), activated: true };
|
||||
if (activatedAgent) {
|
||||
return { agent: activatedAgent, activated: true };
|
||||
}
|
||||
|
||||
const existing = await getById(id);
|
||||
|
||||
Reference in New Issue
Block a user