diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index fffdb1da..ef4cb066 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -132,6 +132,14 @@ The active-lock lifecycle is part of the checkout contract: Stale-lock recovery is crash recovery, not a retry loop. Paperclip must not clear or adopt locks held by non-terminal runs. After stale cleanup, a checkout `409` should mean a real live owner, status/assignee mismatch, unresolved blocker, or active gate still prevents checkout. Agents must treat that `409` as an ownership conflict and stop rather than retrying the same checkout. +### Pre-dispatch configuration validation + +Pre-dispatch configuration validation is a distinct gate that runs after ownership and checkout are resolved but before the control plane actually dispatches a run. + +> Before a run is dispatched, required secret/env bindings are validated; missing bindings produce a surfaced configuration-incomplete blocker, not a dispatched run. + +A configuration-incomplete result is a gate outcome, not a runtime failure. It is one of the active gates that a checkout-time or dispatch-time check can surface instead of starting a run, and it leaves the issue in an explicit waiting state that names the missing binding. Surfacing the blocker keeps the issue healthy under the liveness contract while preventing a run that is guaranteed to fail once it cannot resolve its required secret/env bindings. A dispatched-then-failed run is the wrong shape for missing configuration: the missing binding is a known pre-dispatch condition, so the control plane must surface it as a configuration-incomplete blocker rather than letting the run start and then fail. + ## 6. Parent/Sub-Issue vs Blockers Paperclip uses two different relationships for different jobs. diff --git a/packages/db/src/migrations/0103_agent_error_reason.sql b/packages/db/src/migrations/0103_agent_error_reason.sql new file mode 100644 index 00000000..2c952d65 --- /dev/null +++ b/packages/db/src/migrations/0103_agent_error_reason.sql @@ -0,0 +1 @@ +ALTER TABLE "agents" ADD COLUMN "error_reason" text; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index 55ff7687..49049cc6 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -722,6 +722,13 @@ "when": 1781490100000, "tag": "0102_managed_sandbox_dedup_index", "breakpoints": true + }, + { + "idx": 103, + "version": "7", + "when": 1781490200000, + "tag": "0103_agent_error_reason", + "breakpoints": true } ] } \ No newline at end of file diff --git a/packages/db/src/schema/agents.ts b/packages/db/src/schema/agents.ts index f7d06454..0f92f3b7 100644 --- a/packages/db/src/schema/agents.ts +++ b/packages/db/src/schema/agents.ts @@ -31,6 +31,7 @@ export const agents = pgTable( spentMonthlyCents: integer("spent_monthly_cents").notNull().default(0), pauseReason: text("pause_reason"), pausedAt: timestamp("paused_at", { withTimezone: true }), + errorReason: text("error_reason"), permissions: jsonb("permissions").$type>().notNull().default({}), lastHeartbeatAt: timestamp("last_heartbeat_at", { withTimezone: true }), metadata: jsonb("metadata").$type>(), diff --git a/packages/shared/src/constants.ts b/packages/shared/src/constants.ts index 2c873329..160713a5 100644 --- a/packages/shared/src/constants.ts +++ b/packages/shared/src/constants.ts @@ -272,6 +272,7 @@ export const ISSUE_RECOVERY_ACTION_KINDS = [ "missing_disposition", "stranded_assigned_issue", "workspace_validation", + "configuration_validation", "active_run_watchdog", "issue_graph_liveness", ] as const; diff --git a/packages/shared/src/types/agent.ts b/packages/shared/src/types/agent.ts index 7027f00f..8fc7ec5a 100644 --- a/packages/shared/src/types/agent.ts +++ b/packages/shared/src/types/agent.ts @@ -96,6 +96,7 @@ export interface Agent { spentMonthlyCents: number; pauseReason: PauseReason | null; pausedAt: Date | null; + errorReason?: string | null; permissions: AgentPermissions; lastHeartbeatAt: Date | null; metadata: Record | null; diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index c2f8266f..2c27a910 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -51,6 +51,7 @@ const mockCompanySkillService = vi.hoisted(() => ({ const mockSecretService = vi.hoisted(() => ({ resolveAdapterConfigForRuntime: vi.fn(), normalizeAdapterConfigForPersistence: vi.fn(async (_companyId: string, config: Record) => config), + syncEnvBindingsForTarget: vi.fn(), })); const mockLogActivity = vi.hoisted(() => vi.fn()); @@ -96,6 +97,10 @@ vi.mock("../services/index.js", () => ({ workspaceOperationService: () => mockWorkspaceOperationService, })); +vi.mock("../services/secrets.js", () => ({ + secretService: () => mockSecretService, +})); + vi.mock("../adapters/index.js", () => ({ findServerAdapter: vi.fn(() => mockAdapter), findActiveServerAdapter: vi.fn(() => mockAdapter), @@ -129,6 +134,10 @@ function registerModuleMocks() { workspaceOperationService: () => mockWorkspaceOperationService, })); + vi.doMock("../services/secrets.js", () => ({ + secretService: () => mockSecretService, + })); + vi.doMock("../adapters/index.js", () => ({ findServerAdapter: vi.fn(() => mockAdapter), findActiveServerAdapter: vi.fn(() => mockAdapter), @@ -249,6 +258,7 @@ describe.sequential("agent skill routes", () => { agent: makeAgent("claude_local"), }); mockSecretService.resolveAdapterConfigForRuntime.mockResolvedValue({ config: { env: {} } }); + mockSecretService.syncEnvBindingsForTarget.mockResolvedValue(undefined); mockCompanySkillService.listRuntimeSkillEntries.mockResolvedValue([ { key: "paperclipai/paperclip/paperclip", diff --git a/server/src/__tests__/agents-service-clear-error.test.ts b/server/src/__tests__/agents-service-clear-error.test.ts index 1b264160..0e58cb27 100644 --- a/server/src/__tests__/agents-service-clear-error.test.ts +++ b/server/src/__tests__/agents-service-clear-error.test.ts @@ -66,6 +66,7 @@ describeEmbeddedPostgres("agent service clearError", () => { 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: {}, @@ -116,6 +117,7 @@ describeEmbeddedPostgres("agent service clearError", () => { status: "idle", pauseReason: null, pausedAt: null, + errorReason: null, }); const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)); diff --git a/server/src/__tests__/agents-service-secret-bindings.test.ts b/server/src/__tests__/agents-service-secret-bindings.test.ts new file mode 100644 index 00000000..634086cf --- /dev/null +++ b/server/src/__tests__/agents-service-secret-bindings.test.ts @@ -0,0 +1,331 @@ +import { randomUUID } from "node:crypto"; +import { mkdirSync, rmSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { and, eq } from "drizzle-orm"; +import { + agents, + companies, + companySecretBindings, + companySecretProviderConfigs, + companySecretVersions, + companySecrets, + createDb, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { agentService } from "../services/agents.ts"; +import { secretService } from "../services/secrets.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres agent secret binding tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +describeEmbeddedPostgres("agent service secret binding sync", () => { + let stopDb: (() => Promise) | null = null; + let db!: ReturnType; + const previousKeyFile = process.env.PAPERCLIP_SECRETS_MASTER_KEY_FILE; + const secretsTmpDir = path.join(os.tmpdir(), `paperclip-agent-secret-bindings-${randomUUID()}`); + + beforeAll(async () => { + mkdirSync(secretsTmpDir, { recursive: true }); + process.env.PAPERCLIP_SECRETS_MASTER_KEY_FILE = path.join(secretsTmpDir, "master.key"); + const started = await startEmbeddedPostgresTestDatabase("agent-secret-bindings"); + stopDb = started.cleanup; + db = createDb(started.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(companySecretBindings); + await db.delete(companySecretVersions); + await db.delete(companySecrets); + await db.delete(companySecretProviderConfigs); + await db.delete(agents); + await db.delete(companies); + }); + + afterAll(async () => { + await stopDb?.(); + if (previousKeyFile === undefined) { + delete process.env.PAPERCLIP_SECRETS_MASTER_KEY_FILE; + } else { + process.env.PAPERCLIP_SECRETS_MASTER_KEY_FILE = previousKeyFile; + } + rmSync(secretsTmpDir, { recursive: true, force: true }); + }); + + async function seedCompany() { + const companyId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + return companyId; + } + + it("creates agent secret bindings when a new agent persists secret_ref env", async () => { + const companyId = await seedCompany(); + const secrets = secretService(db); + const secret = await secrets.create(companyId, { + name: `anthropic-${randomUUID()}`, + provider: "local_encrypted", + value: "sk-ant-123", + }); + + const created = await agentService(db).create(companyId, { + name: "Claude Novita", + role: "engineer", + status: "pending_approval", + adapterType: "claude_local", + adapterConfig: { + env: { + ANTHROPIC_API_KEY: { type: "secret_ref", secretId: secret.id, version: "latest" }, + }, + }, + runtimeConfig: {}, + spentMonthlyCents: 0, + lastHeartbeatAt: null, + }); + + const bindings = await db + .select() + .from(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, companyId), + eq(companySecretBindings.targetType, "agent"), + eq(companySecretBindings.targetId, created.id), + )); + + expect(bindings).toHaveLength(1); + expect(bindings[0]).toMatchObject({ + secretId: secret.id, + configPath: "env.ANTHROPIC_API_KEY", + versionSelector: "latest", + required: true, + }); + }); + + it("replaces agent secret bindings when adapterConfig env changes", async () => { + const companyId = await seedCompany(); + const secrets = secretService(db); + const oldSecret = await secrets.create(companyId, { + name: `old-${randomUUID()}`, + provider: "local_encrypted", + value: "old-value", + }); + const nextSecret = await secrets.create(companyId, { + name: `next-${randomUUID()}`, + provider: "local_encrypted", + value: "next-value", + }); + + const created = await agentService(db).create(companyId, { + name: "Binding Swapper", + role: "engineer", + adapterType: "codex_local", + adapterConfig: { + env: { + OLD_KEY: { type: "secret_ref", secretId: oldSecret.id, version: "latest" }, + }, + }, + runtimeConfig: {}, + spentMonthlyCents: 0, + lastHeartbeatAt: null, + }); + + await agentService(db).update(created.id, { + adapterConfig: { + env: { + NEW_KEY: { type: "secret_ref", secretId: nextSecret.id, version: "latest" }, + }, + }, + }); + + const bindings = await db + .select() + .from(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, companyId), + eq(companySecretBindings.targetType, "agent"), + eq(companySecretBindings.targetId, created.id), + )); + + expect(bindings).toHaveLength(1); + expect(bindings[0]).toMatchObject({ + secretId: nextSecret.id, + configPath: "env.NEW_KEY", + }); + }); + + it("backfills missing secret bindings when a legacy pending agent is approved", async () => { + const companyId = await seedCompany(); + const secrets = secretService(db); + const secret = await secrets.create(companyId, { + name: `legacy-${randomUUID()}`, + provider: "local_encrypted", + value: "legacy-value", + }); + const agentId = randomUUID(); + + await db.insert(agents).values({ + id: agentId, + companyId, + name: "Legacy Pending Agent", + role: "engineer", + status: "pending_approval", + adapterType: "claude_local", + adapterConfig: { + env: { + ANTHROPIC_API_KEY: { type: "secret_ref", secretId: secret.id, version: "latest" }, + }, + }, + runtimeConfig: {}, + permissions: {}, + }); + + const beforeBindings = await db + .select() + .from(companySecretBindings) + .where(eq(companySecretBindings.targetId, agentId)); + expect(beforeBindings).toHaveLength(0); + + const approved = await agentService(db).activatePendingApproval(agentId); + + expect(approved).toMatchObject({ + activated: true, + agent: { + id: agentId, + status: "idle", + }, + }); + + const afterBindings = await db + .select() + .from(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, companyId), + eq(companySecretBindings.targetType, "agent"), + eq(companySecretBindings.targetId, agentId), + )); + + expect(afterBindings).toHaveLength(1); + expect(afterBindings[0]).toMatchObject({ + secretId: secret.id, + configPath: "env.ANTHROPIC_API_KEY", + }); + }); + + it("rolls back create when binding sync fails", async () => { + const companyId = await seedCompany(); + const missingSecretId = randomUUID(); + + await expect( + agentService(db).create(companyId, { + name: "Broken Create", + role: "engineer", + adapterType: "claude_local", + adapterConfig: { + env: { + ANTHROPIC_API_KEY: { type: "secret_ref", secretId: missingSecretId, version: "latest" }, + }, + }, + runtimeConfig: {}, + spentMonthlyCents: 0, + lastHeartbeatAt: null, + }), + ).rejects.toBeTruthy(); + + const persistedAgents = await db + .select() + .from(agents) + .where(eq(agents.companyId, companyId)); + expect(persistedAgents).toHaveLength(0); + }); + + it("rolls back adapterConfig updates when binding sync fails", async () => { + const companyId = await seedCompany(); + const secrets = secretService(db); + const validSecret = await secrets.create(companyId, { + name: `valid-${randomUUID()}`, + provider: "local_encrypted", + value: "valid-value", + }); + const created = await agentService(db).create(companyId, { + name: "Transactional Update", + role: "engineer", + adapterType: "codex_local", + adapterConfig: { + env: { + API_KEY: { type: "secret_ref", secretId: validSecret.id, version: "latest" }, + }, + }, + runtimeConfig: {}, + spentMonthlyCents: 0, + lastHeartbeatAt: null, + }); + + await expect( + agentService(db).update(created.id, { + adapterConfig: { + env: { + API_KEY: { type: "secret_ref", secretId: randomUUID(), version: "latest" }, + }, + }, + }), + ).rejects.toBeTruthy(); + + const reloaded = await agentService(db).getById(created.id); + expect(reloaded?.adapterConfig).toMatchObject({ + env: { + API_KEY: { type: "secret_ref", secretId: validSecret.id, version: "latest" }, + }, + }); + + const bindings = await db + .select() + .from(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, companyId), + eq(companySecretBindings.targetType, "agent"), + eq(companySecretBindings.targetId, created.id), + )); + expect(bindings).toHaveLength(1); + expect(bindings[0]?.secretId).toBe(validSecret.id); + }); + + it("keeps pending approval status when activation binding sync fails", async () => { + const companyId = await seedCompany(); + const agentId = randomUUID(); + + await db.insert(agents).values({ + id: agentId, + companyId, + name: "Broken Pending Agent", + role: "engineer", + status: "pending_approval", + adapterType: "claude_local", + adapterConfig: { + env: { + ANTHROPIC_API_KEY: { type: "secret_ref", secretId: randomUUID(), version: "latest" }, + }, + }, + runtimeConfig: {}, + permissions: {}, + }); + + await expect(agentService(db).activatePendingApproval(agentId)).rejects.toBeTruthy(); + + const reloaded = await agentService(db).getById(agentId); + expect(reloaded?.status).toBe("pending_approval"); + }); +}); diff --git a/server/src/__tests__/approvals-service.test.ts b/server/src/__tests__/approvals-service.test.ts index b15298f0..abc45fb6 100644 --- a/server/src/__tests__/approvals-service.test.ts +++ b/server/src/__tests__/approvals-service.test.ts @@ -58,7 +58,7 @@ function createDbStub(selectResults: ApprovalRecord[][], updateResults: Approval describe("approvalService resolution idempotency", () => { beforeEach(() => { vi.clearAllMocks(); - mockAgentService.activatePendingApproval.mockResolvedValue(undefined); + mockAgentService.activatePendingApproval.mockResolvedValue({ agent: { id: "agent-1" }, activated: true }); mockAgentService.create.mockResolvedValue({ id: "agent-1" }); mockAgentService.terminate.mockResolvedValue(undefined); mockNotifyHireApproved.mockResolvedValue(undefined); @@ -104,4 +104,34 @@ describe("approvalService resolution idempotency", () => { expect(mockAgentService.activatePendingApproval).toHaveBeenCalledWith("agent-1"); expect(mockNotifyHireApproved).toHaveBeenCalledTimes(1); }); + + it("creates the agent from payload when approval does not reference a pending agent", async () => { + const approved = { + ...createApproval("approved"), + payload: { + name: "New Agent", + adapterConfig: { + env: { + API_KEY: { + type: "secret_ref", + secretId: "secret-1", + version: "latest", + }, + }, + }, + }, + }; + const dbStub = createDbStub([[{ ...createApproval("pending"), payload: approved.payload }]], [approved]); + + const svc = approvalService(dbStub.db as any); + const result = await svc.approve("approval-1", "board", "ship it"); + + expect(result.applied).toBe(true); + expect(mockAgentService.create).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ + adapterConfig: approved.payload.adapterConfig, + }), + ); + }); }); diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 723f8455..13679973 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -8,6 +8,8 @@ import { agentRuntimeState, agentWakeupRequests, budgetPolicies, + companySecretBindings, + companySecrets, companySkills, companies, costEvents, @@ -96,6 +98,7 @@ import { heartbeatService, redactDetectedSuccessfulRunProgressSummaryForBoard, } from "../services/heartbeat.ts"; +import { secretService } from "../services/secrets.ts"; import { SUCCESSFUL_RUN_HANDOFF_EXHAUSTED_NOTICE_BODY, SUCCESSFUL_RUN_HANDOFF_REQUIRED_NOTICE_BODY, @@ -411,6 +414,8 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { await db.delete(issueDocuments); await db.delete(documentRevisions); await db.delete(documents); + await db.delete(companySecretBindings); + await db.delete(companySecrets); try { await db.delete(companies); break; @@ -1432,6 +1437,90 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { expect(validationComment).toBeTruthy(); }); + it("blocks before dispatch when a declared secret ref has no binding instead of emitting an opaque setup failure", async () => { + const { companyId, agentId, runId, issueId } = await seedQueuedIssueRunFixture(); + const svc = secretService(db); + const secretName = `unbound-runtime-${randomUUID()}`; + const secret = await svc.create(companyId, { + name: secretName, + provider: "local_encrypted", + value: "never-resolved", + }); + // Declare the secret ref on the agent env WITHOUT creating a binding so the + // pre-dispatch gate short-circuits to a configuration-incomplete blocker. + await db + .update(agents) + .set({ + adapterConfig: { + env: { + UNBOUND_API_KEY: { type: "secret_ref", secretId: secret.id, version: "latest" }, + }, + }, + }) + .where(eq(agents.id, agentId)); + + const heartbeat = heartbeatService(db); + await heartbeat.resumeQueuedRuns(); + await waitForRunToSettle(heartbeat, runId, 5_000); + + expect(mockAdapterExecute).not.toHaveBeenCalled(); + + const failedRun = await db + .select() + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runId)) + .then((rows) => rows[0] ?? null); + expect(failedRun).toMatchObject({ + status: "failed", + errorCode: "configuration_incomplete", + }); + expect(failedRun?.error).toContain("configuration incomplete"); + expect(failedRun?.error).toContain(secretName); + expect(failedRun?.error).toContain("env.UNBOUND_API_KEY"); + expect(failedRun?.resultJson).toMatchObject({ + configurationIncomplete: { + reason: "secret_binding_missing", + missingBindings: [ + { + consumerType: "agent", + consumerId: agentId, + configPath: "env.UNBOUND_API_KEY", + envKey: "UNBOUND_API_KEY", + secretId: secret.id, + secretName, + }, + ], + }, + }); + // Value-free gate: no secret access events were recorded. + expect(await svc.listAccessEvents(companyId, secret.id)).toHaveLength(0); + + const issue = await waitForValue(async () => + db.select().from(issues).where(eq(issues.id, issueId)).then((rows) => { + const row = rows[0] ?? null; + return row?.status === "blocked" ? row : null; + }), + ); + expect(issue?.executionRunId).toBeNull(); + + const recoveryAction = await db + .select() + .from(issueRecoveryActions) + .where(and(eq(issueRecoveryActions.companyId, companyId), eq(issueRecoveryActions.sourceIssueId, issueId))) + .then((rows) => rows[0] ?? null); + expect(recoveryAction).toMatchObject({ + kind: "configuration_validation", + cause: "configuration_incomplete", + status: "active", + ownerAgentId: agentId, + recoveryIssueId: null, + }); + expect(recoveryAction?.nextAction).toContain("Bind the missing secret"); + + const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); + expect(comments.some((comment) => comment.body.includes("secret/env bindings are missing"))).toBe(true); + }); + it("queues one finish-handoff wake when a successful run leaves in-progress work without a next action", async () => { const { companyId, agentId, runId, issueId } = await seedQueuedIssueRunFixture(); mockAdapterExecute.mockImplementationOnce(async (ctx: { runId: string }) => { diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index 05607659..93ae8bcf 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -223,6 +223,46 @@ describeEmbeddedPostgres("secretService", () => { expect(JSON.stringify(events)).not.toContain("runtime-secret"); }); + it("collects declared secret refs that have no binding without resolving values", async () => { + const companyId = await seedCompany(); + const svc = secretService(db); + const secretName = `unbound-${randomUUID()}`; + const secret = await svc.create(companyId, { + name: secretName, + provider: "local_encrypted", + value: "runtime-secret", + }); + const env = { + API_KEY: { type: "secret_ref" as const, secretId: secret.id, version: "latest" as const }, + PLAIN_VALUE: "not-a-secret", + }; + + const missing = await svc.collectMissingRuntimeBindings(companyId, env, { + consumerType: "agent", + consumerId: "agent-1", + }); + + expect(missing).toHaveLength(1); + expect(missing[0]).toMatchObject({ + consumerType: "agent", + consumerId: "agent-1", + configPath: "env.API_KEY", + envKey: "API_KEY", + secretId: secret.id, + secretName, + }); + // Value-free validation: no access events recorded. + expect(await svc.listAccessEvents(companyId, secret.id)).toHaveLength(0); + + await svc.syncEnvBindingsForTarget(companyId, { targetType: "agent", targetId: "agent-1" }, env); + + const afterBinding = await svc.collectMissingRuntimeBindings(companyId, env, { + consumerType: "agent", + consumerId: "agent-1", + }); + expect(afterBinding).toEqual([]); + }); + it("denies runtime secret resolution outside the low-trust binding allowlist", async () => { const companyId = await seedCompany(); const svc = secretService(db); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 4d22fa3d..011992c5 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -2462,14 +2462,6 @@ export function agentRoutes( lastHeartbeatAt: null, }); const agent = await materializeDefaultInstructionsBundleForNewAgent(createdAgent, instructionsBundle); - const agentEnv = asRecord(agent.adapterConfig)?.env; - if (agentEnv) { - await secretsSvc.syncEnvBindingsForTarget?.( - companyId, - { targetType: "agent", targetId: agent.id }, - agentEnv, - ); - } const actor = getActorInfo(req); await logActivity(db, { @@ -2952,14 +2944,6 @@ export function agentRoutes( res.status(404).json({ error: "Agent not found" }); return; } - if (touchesAdapterConfiguration) { - const agentEnv = asRecord(agent.adapterConfig)?.env; - await secretsSvc.syncEnvBindingsForTarget?.( - agent.companyId, - { targetType: "agent", targetId: agent.id }, - agentEnv, - ); - } await logActivity(db, { companyId: agent.companyId, diff --git a/server/src/services/agent-secret-bindings.ts b/server/src/services/agent-secret-bindings.ts new file mode 100644 index 00000000..4afb096b --- /dev/null +++ b/server/src/services/agent-secret-bindings.ts @@ -0,0 +1,26 @@ +interface AgentSecretBindingSyncService { + syncEnvBindingsForTarget?: ( + companyId: string, + target: { targetType: "agent"; targetId: string; pathPrefix?: string }, + envValue: unknown, + ) => Promise; +} + +function asRecord(value: unknown): Record | null { + if (typeof value !== "object" || value === null || Array.isArray(value)) return null; + return value as Record; +} + +export async function syncAgentAdapterEnvBindings(input: { + secretsSvc: AgentSecretBindingSyncService; + companyId: string; + agentId: string; + adapterConfig: unknown; +}) { + const envValue = asRecord(asRecord(input.adapterConfig)?.env); + await input.secretsSvc.syncEnvBindingsForTarget?.( + input.companyId, + { targetType: "agent", targetId: input.agentId }, + envValue, + ); +} diff --git a/server/src/services/agents.ts b/server/src/services/agents.ts index f8d37aef..2c695589 100644 --- a/server/src/services/agents.ts +++ b/server/src/services/agents.ts @@ -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, @@ -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, - afterConfig: afterConfig as unknown as Record, - }); + 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, + afterConfig: afterConfig as unknown as Record, + }); + } + } + + 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); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index dbc8f56a..66e0d612 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -71,7 +71,7 @@ import { trackAgentFirstHeartbeat } from "@paperclipai/shared/telemetry"; import { getTelemetryClient } from "../telemetry.js"; import { companySkillService } from "./company-skills.js"; import { budgetService, type BudgetEnforcementScope } from "./budgets.js"; -import { secretService } from "./secrets.js"; +import { secretService, type MissingRuntimeBinding } from "./secrets.js"; import { resolveDefaultAgentWorkspaceDir, resolveManagedProjectWorkspaceDir } from "../home-paths.js"; import { buildHeartbeatRunIssueComment, @@ -254,6 +254,8 @@ const BOUNDED_TRANSIENT_HEARTBEAT_RETRY_WAKE_REASON = "transient_failure_retry"; const BOUNDED_TRANSIENT_HEARTBEAT_RETRY_MAX_ATTEMPTS = BOUNDED_TRANSIENT_HEARTBEAT_RETRY_DELAYS_MS.length; const WORKSPACE_VALIDATION_FAILURE_CODE = "workspace_validation_failed"; const WORKSPACE_VALIDATION_RECOVERY_CAUSE = "workspace_validation_failed"; +const CONFIGURATION_INCOMPLETE_FAILURE_CODE = "configuration_incomplete"; +const CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE = "configuration_incomplete"; // Keep this in sync with local adapters that require a git workspace before launch. const GIT_SENSITIVE_LOCAL_ADAPTER_TYPES = new Set([ "acpx_local", @@ -296,6 +298,20 @@ export class WorkspaceValidationFailure extends Error { } } +// Pre-dispatch gate outcome: required secret/env bindings are missing, so the +// run must not be dispatched. Surfaced as a configuration-incomplete blocker +// routed to a human owner instead of N opaque dispatched-then-failed runs. +export class ConfigurationIncompleteFailure extends Error { + code = CONFIGURATION_INCOMPLETE_FAILURE_CODE; + resultJson: Record; + + constructor(message: string, resultJson: Record) { + super(message); + this.name = "ConfigurationIncompleteFailure"; + this.resultJson = resultJson; + } +} + function resolveCodexTransientFallbackMode(attempt: number): CodexTransientFallbackMode { if (attempt <= 1) return "same_session"; if (attempt === 2) return "safer_invocation"; @@ -381,9 +397,16 @@ const INLINE_BASE64_IMAGE_DATA_RE = /("type":"image","source":\{"type":"base64", type RuntimeConfigSecretResolver = Pick< ReturnType, - "resolveAdapterConfigForRuntime" | "resolveEnvBindings" + "resolveAdapterConfigForRuntime" | "resolveEnvBindings" | "collectMissingRuntimeBindings" >; +function formatMissingBindingForOperator(missing: MissingRuntimeBinding): string { + const secretLabel = missing.secretName + ? `"${missing.secretName}"` + : missing.secretId; + return `secret ${secretLabel} not bound at ${missing.consumerType} ${missing.configPath}`; +} + const LOW_TRUST_SENSITIVE_ENV_KEY_RE = /(api[-_]?key|access[-_]?token|auth(?:_?token)?|authorization|bearer|secret|passwd|password|credential|jwt|private[-_]?key|cookie|connectionstring)/i; @@ -449,6 +472,54 @@ export async function resolveExecutionRunAdapterConfig(input: { assertLowTrustEnvConfigAllowed(projectEnv, "project.env"); assertLowTrustEnvConfigAllowed(routineEnv, "routine.env"); } + // Pre-dispatch binding-validation gate: detect declared secret refs that have + // no binding before resolving any secret value. Missing bindings short-circuit + // to a configuration-incomplete blocker routed to a human owner instead of a + // dispatched-then-failed run (which previously surfaced as opaque setup_failed). + if (typeof input.secretsSvc.collectMissingRuntimeBindings === "function") { + const missingBindings: MissingRuntimeBinding[] = []; + if (input.agentId) { + missingBindings.push( + ...(await input.secretsSvc.collectMissingRuntimeBindings( + input.companyId, + parseObject(executionRunConfig.env), + { consumerType: "agent", consumerId: input.agentId }, + )), + ); + } + if (projectEnv && input.projectId) { + missingBindings.push( + ...(await input.secretsSvc.collectMissingRuntimeBindings( + input.companyId, + projectEnv, + { consumerType: "project", consumerId: input.projectId }, + )), + ); + } + if (routineEnv && input.routineId) { + missingBindings.push( + ...(await input.secretsSvc.collectMissingRuntimeBindings( + input.companyId, + routineEnv, + { consumerType: "routine", consumerId: input.routineId }, + )), + ); + } + if (missingBindings.length > 0) { + const detail = missingBindings.map(formatMissingBindingForOperator).join("; "); + throw new ConfigurationIncompleteFailure(`configuration incomplete: ${detail}`, { + configurationIncomplete: { + reason: "secret_binding_missing", + companyId: input.companyId, + agentId: input.agentId ?? null, + issueId: input.issueId ?? null, + projectId: input.projectId ?? null, + routineId: input.routineId ?? null, + missingBindings, + }, + }); + } + } const { config: resolvedConfig, secretKeys, manifest } = await input.secretsSvc.resolveAdapterConfigForRuntime( input.companyId, executionRunConfig, @@ -939,6 +1010,16 @@ function isWorkspaceValidationFailedRun( return run?.errorCode === WORKSPACE_VALIDATION_FAILURE_CODE; } +function isConfigurationIncompleteFailure(error: unknown): error is ConfigurationIncompleteFailure { + return error instanceof ConfigurationIncompleteFailure; +} + +function isConfigurationIncompleteFailedRun( + run: Pick | null | undefined, +) { + return run?.errorCode === CONFIGURATION_INCOMPLETE_FAILURE_CODE; +} + async function hasGitMetadata(cwd: string | null | undefined) { const normalized = readNonEmptyString(cwd); if (!normalized) return false; @@ -7199,9 +7280,17 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) return cancelled; } + function truncateAgentErrorReason(reason: string | null | undefined): string | null { + if (!reason) return null; + const trimmed = reason.trim(); + if (!trimmed) return null; + return trimmed.length > 500 ? `${trimmed.slice(0, 499)}…` : trimmed; + } + async function finalizeAgentStatus( agentId: string, outcome: "succeeded" | "failed" | "cancelled" | "timed_out", + failureReason?: string | null, ) { const existing = await getAgent(agentId); if (!existing) return; @@ -7224,6 +7313,10 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) .update(agents) .set({ status: nextStatus, + // Persist a human-readable reason on the agent record when it enters + // error so operators see it on the agent page without digging into run + // events; clear it whenever the agent leaves error. + errorReason: nextStatus === "error" ? truncateAgentErrorReason(failureReason) : null, lastHeartbeatAt: new Date(), updatedAt: new Date(), }) @@ -7586,7 +7679,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) }, }); - await finalizeAgentStatus(run.agentId, "failed"); + await finalizeAgentStatus(run.agentId, "failed", baseMessage); await startNextQueuedRunForAgent(run.agentId); runningProcesses.delete(run.id); reaped.push(run.id); @@ -9439,14 +9532,20 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) } } } - await finalizeAgentStatus(agent.id, outcome); + await finalizeAgentStatus( + agent.id, + outcome, + outcome === "succeeded" ? null : (adapterResult.errorMessage ?? null), + ); } catch (err) { const message = redactCurrentUserText( err instanceof Error ? err.message : "Unknown adapter failure", await getCurrentUserRedactionOptions(), ); const workspaceValidationFailure = isWorkspaceValidationFailure(err) ? err : null; - const failureErrorCode = workspaceValidationFailure?.code ?? "adapter_failed"; + const configurationIncompleteFailure = isConfigurationIncompleteFailure(err) ? err : null; + const failureErrorCode = + workspaceValidationFailure?.code ?? configurationIncompleteFailure?.code ?? "adapter_failed"; logger.error({ err, runId }, "heartbeat execution failed"); let logSummary: { bytes: number; sha256?: string; compressed: boolean } | null = null; @@ -9472,7 +9571,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) resultJson: mergeRunStopMetadataForAgent(agent, "failed", { errorCode: failureErrorCode, errorMessage: message, - resultJson: workspaceValidationFailure?.resultJson ?? null, + resultJson: workspaceValidationFailure?.resultJson ?? configurationIncompleteFailure?.resultJson ?? null, }), stdoutExcerpt, stderrExcerpt, @@ -9507,7 +9606,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) }); const livenessRun = await classifyAndPersistRunLiveness(failedRun) ?? failedRun; await refreshContinuationSummaryForRun(livenessRun, agent); - if (!isWorkspaceValidationFailedRun(livenessRun)) { + if (!isWorkspaceValidationFailedRun(livenessRun) && !isConfigurationIncompleteFailedRun(livenessRun)) { await finalizeIssueCommentPolicy(livenessRun, agent); } await releaseIssueExecutionAndPromote(livenessRun); @@ -9535,22 +9634,31 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) } } - await finalizeAgentStatus(agent.id, "failed"); + await finalizeAgentStatus(agent.id, "failed", message); } } catch (outerErr) { // Setup code before adapter.execute threw (e.g. ensureRuntimeState, resolveWorkspaceForRun). // The inner catch did not fire, so we must record the failure here. - const message = outerErr instanceof Error ? outerErr.message : "Unknown setup failure"; + const message = redactCurrentUserText( + outerErr instanceof Error ? outerErr.message : "Unknown setup failure", + await getCurrentUserRedactionOptions(), + ); + // A missing secret/env binding is a known pre-dispatch configuration gap, + // not an opaque setup crash. Surface it with its own errorCode so the + // recovery path routes it to a human owner instead of looping retries. + const configurationIncompleteSetupFailure = isConfigurationIncompleteFailure(outerErr) ? outerErr : null; + const setupFailureErrorCode = configurationIncompleteSetupFailure?.code ?? "setup_failed"; logger.error({ err: outerErr, runId }, "heartbeat execution setup failed"); const setupFailureAgent = await getAgent(run.agentId).catch(() => null); const setupFailureWrite = await setRunStatusIfRunning(runId, "failed", { error: message, - errorCode: "setup_failed", + errorCode: setupFailureErrorCode, finishedAt: new Date(), ...(setupFailureAgent ? { resultJson: mergeRunStopMetadataForAgent(setupFailureAgent, "failed", { - errorCode: "setup_failed", + errorCode: setupFailureErrorCode, errorMessage: message, + resultJson: configurationIncompleteSetupFailure?.resultJson ?? null, }), } : {}), }).catch(() => ({ run: null, updated: false as const })); @@ -9583,7 +9691,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) const failedAgent = setupFailureAgent ?? await getAgent(run.agentId).catch(() => null); if (failedAgent) { await refreshContinuationSummaryForRun(livenessRun, failedAgent).catch(() => undefined); - if (!isWorkspaceValidationFailedRun(livenessRun)) { + if (!isWorkspaceValidationFailedRun(livenessRun) && !isConfigurationIncompleteFailedRun(livenessRun)) { await finalizeIssueCommentPolicy(livenessRun, failedAgent).catch(() => undefined); } } @@ -9593,7 +9701,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) // path owned the terminal transition. If another path already finalized // the run, keep that terminal outcome authoritative. if (setupFailureWrite.updated) { - await finalizeAgentStatus(run.agentId, "failed").catch(() => undefined); + await finalizeAgentStatus(run.agentId, "failed", message).catch(() => undefined); } } finally { const latestRun = await getRun(run.id).catch(() => null); @@ -9641,6 +9749,17 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) ); } + function buildConfigurationIncompleteRecoveryComment(input: { + latestRun: Pick | null | undefined; + }) { + const failureSummary = summarizeRunFailureForIssueComment(input.latestRun); + return ( + "Paperclip stopped before dispatching the adapter because required secret/env bindings are missing. " + + `Resolving them as a runtime failure would only produce repeated opaque setup failures.${failureSummary ?? ""} ` + + "Moving it to `blocked` with a source-scoped recovery action so an operator can bind the missing secret(s) before resuming." + ); + } + async function releaseIssueExecutionAndPromote(run: typeof heartbeatRuns.$inferSelect) { const runContext = parseObject(run.contextSnapshot); const contextIssueId = readNonEmptyString(runContext.issueId); @@ -9761,17 +9880,22 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) // Sibling lock cleanup is already done above; only the primary issue carries // the recovery surface because the comment is attached to a single issue. if ( - isWorkspaceValidationFailedRun(run) && + (isWorkspaceValidationFailedRun(run) || isConfigurationIncompleteFailedRun(run)) && (issue.status === "todo" || issue.status === "in_progress") && !issue.assigneeUserId && issue.assigneeAgentId === run.agentId ) { + const configurationIncomplete = isConfigurationIncompleteFailedRun(run); return { kind: "blocked" as const, issue, previousStatus: issue.status, - comment: buildWorkspaceValidationRecoveryComment({ latestRun: run }), - recoveryCause: WORKSPACE_VALIDATION_RECOVERY_CAUSE, + comment: configurationIncomplete + ? buildConfigurationIncompleteRecoveryComment({ latestRun: run }) + : buildWorkspaceValidationRecoveryComment({ latestRun: run }), + recoveryCause: configurationIncomplete + ? CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE + : WORKSPACE_VALIDATION_RECOVERY_CAUSE, }; } @@ -10057,21 +10181,29 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) !recoveryAgentInvokable || !recoveryAgent || isWorkspaceValidationFailedRun(run) || + isConfigurationIncompleteFailedRun(run) || didAutomaticRecoveryFail(run, issue.status === "todo" ? "assignment_recovery" : "issue_continuation_needed"); if (shouldBlockImmediately) { const workspaceValidationFailure = isWorkspaceValidationFailedRun(run); + const configurationIncompleteFailure = isConfigurationIncompleteFailedRun(run); const comment = workspaceValidationFailure ? buildWorkspaceValidationRecoveryComment({ latestRun: run }) - : buildImmediateExecutionPathRecoveryComment({ - status: issue.status as "todo" | "in_progress", - latestRun: run, - }); + : configurationIncompleteFailure + ? buildConfigurationIncompleteRecoveryComment({ latestRun: run }) + : buildImmediateExecutionPathRecoveryComment({ + status: issue.status as "todo" | "in_progress", + latestRun: run, + }); return { kind: "blocked" as const, issue, previousStatus: issue.status, comment, - recoveryCause: workspaceValidationFailure ? WORKSPACE_VALIDATION_RECOVERY_CAUSE : undefined, + recoveryCause: workspaceValidationFailure + ? WORKSPACE_VALIDATION_RECOVERY_CAUSE + : configurationIncompleteFailure + ? CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE + : undefined, }; } @@ -10157,7 +10289,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) recoveryCause: promotionResult.recoveryCause === WORKSPACE_VALIDATION_RECOVERY_CAUSE ? WORKSPACE_VALIDATION_RECOVERY_CAUSE - : undefined, + : promotionResult.recoveryCause === CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE + ? CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE + : undefined, }); return; } diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index f8655523..49c01851 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -119,6 +119,7 @@ type SuccessfulLatestIssueRun = NonNullable & { status: "succeed type StrandedRecoveryCause = | "stranded_assigned_issue" | "workspace_validation_failed" + | "configuration_incomplete" | typeof SUCCESSFUL_RUN_MISSING_STATE_REASON; type SuccessfulRunHandoffRecoveryEvidence = { @@ -2231,6 +2232,8 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) ? "missing_disposition" as const : cause === "workspace_validation_failed" ? "workspace_validation" as const + : cause === "configuration_incomplete" + ? "configuration_validation" as const : "stranded_assigned_issue" as const; } @@ -2306,11 +2309,13 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) ? "Choose and record a valid issue disposition without copying transcript content." : recoveryCause === "workspace_validation_failed" ? "Repair the source issue workspace link, project workspace cwd, or git checkout before resuming adapter execution." + : recoveryCause === "configuration_incomplete" + ? "Bind the missing secret(s) named in the run failure to the agent/project/routine env before resuming adapter execution." : "Restore a live execution path, fix the runtime/adapter failure, or record an intentional manual resolution.", - wakePolicy: recoveryCause === "workspace_validation_failed" + wakePolicy: recoveryCause === "workspace_validation_failed" || recoveryCause === "configuration_incomplete" ? { type: "manual_repair_required", - reason: "workspace_validation_failed", + reason: recoveryCause, ownerAgentId, } : ownerAgentId @@ -2338,6 +2343,7 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) recoveryCause: StrandedRecoveryCause; }) { if (input.recoveryCause === "workspace_validation_failed") return; + if (input.recoveryCause === "configuration_incomplete") return; if (!input.action.ownerAgentId) return; await deps.enqueueWakeup(input.action.ownerAgentId, { source: "assignment", @@ -2543,7 +2549,8 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) const shouldPostEscalationComment = recoveryAction.attemptCount === 1 || - input.recoveryCause === "workspace_validation_failed"; + input.recoveryCause === "workspace_validation_failed" || + input.recoveryCause === "configuration_incomplete"; if (shouldPostEscalationComment) { const escalationCommentMarker = `Recovery action: \`${recoveryAction.id}\``; @@ -2597,6 +2604,8 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) ? "recovery.reconcile_successful_run_handoff_missing_state" : input.recoveryCause === "workspace_validation_failed" ? "recovery.reconcile_workspace_validation_failed" + : input.recoveryCause === "configuration_incomplete" + ? "recovery.reconcile_configuration_incomplete" : "recovery.reconcile_stranded_assigned_issue", recoveryCause: input.recoveryCause ?? "stranded_assigned_issue", latestRunId: input.latestRun?.id ?? null, diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index a7f72928..a8605115 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -203,6 +203,15 @@ export type RuntimeSecretManifestEntry = { errorCode?: string | null; }; +export type MissingRuntimeBinding = { + consumerType: SecretBindingTargetType; + consumerId: string; + configPath: string; + envKey: string; + secretId: string; + secretName: string | null; +}; + type RuntimeSecretResolution = { value: string; manifestEntry: RuntimeSecretManifestEntry; @@ -2352,6 +2361,60 @@ export function secretService(db: Db) { return { env: resolved, secretKeys, manifest }; }, + // Pre-dispatch validation: list declared secret refs in an env-like config + // that have no binding for the given consumer, WITHOUT resolving any secret + // values. Callers use this to surface a configuration-incomplete blocker + // before a run is dispatched instead of letting resolution throw mid-setup. + collectMissingRuntimeBindings: async ( + companyId: string, + envValue: unknown, + context: Omit, + ): Promise => { + const record = asRecord(envValue); + if (!record) return []; + const secretRefs = Object.entries(record).flatMap(([key, rawBinding]) => { + if (!ENV_KEY_RE.test(key)) return []; + const parsed = envBindingSchema.safeParse(rawBinding); + if (!parsed.success) return []; + const binding = canonicalizeBinding(parsed.data as EnvBinding); + if (binding.type !== "secret_ref") return []; + return [{ key, configPath: `env.${key}`, secretId: binding.secretId }]; + }); + if (secretRefs.length === 0) return []; + + const bindingChecks = await Promise.all(secretRefs.map(async (entry) => ({ + entry, + found: await getBinding({ + companyId, + secretId: entry.secretId, + consumerType: context.consumerType, + consumerId: context.consumerId, + configPath: entry.configPath, + }), + }))); + const missingEntries = bindingChecks + .filter((check) => !check.found) + .map((check) => check.entry); + if (missingEntries.length === 0) return []; + + const secretRows = await Promise.all( + [...new Set(missingEntries.map((entry) => entry.secretId))].map(async (secretId) => [ + secretId, + await getById(secretId).catch(() => null), + ] as const), + ); + const secretsById = new Map(secretRows); + + return missingEntries.map((entry) => ({ + consumerType: context.consumerType, + consumerId: context.consumerId, + configPath: entry.configPath, + envKey: entry.key, + secretId: entry.secretId, + secretName: secretsById.get(entry.secretId)?.name ?? null, + })); + }, + resolveAdapterConfigForRuntime: async ( companyId: string, adapterConfig: Record, diff --git a/ui/src/components/AgentConfigForm.test.ts b/ui/src/components/AgentConfigForm.test.ts index 09905880..a17b05ae 100644 --- a/ui/src/components/AgentConfigForm.test.ts +++ b/ui/src/components/AgentConfigForm.test.ts @@ -1,6 +1,10 @@ -import { describe, expect, it } from "vitest"; -import type { Environment } from "@paperclipai/shared"; -import { supportsAdapterModelRefresh } from "./AgentConfigForm"; +import { describe, expect, it, vi } from "vitest"; +import type { AdapterEnvironmentTestResult, Environment } from "@paperclipai/shared"; +import { + getAgentConfigTestActionLabel, + runAgentConfigEnvironmentTest, + supportsAdapterModelRefresh, +} from "./AgentConfigForm"; import { resolveForcedKubernetesEnvironment } from "../lib/forced-kubernetes-environment"; describe("supportsAdapterModelRefresh", () => { @@ -16,6 +20,61 @@ describe("supportsAdapterModelRefresh", () => { }); }); +describe("agent config test action", () => { + it("labels dirty edit-mode tests as save-and-test", () => { + expect(getAgentConfigTestActionLabel({ isCreate: false, isDirty: true })).toBe("Save + Test"); + expect(getAgentConfigTestActionLabel({ isCreate: false, isDirty: false })).toBe("Test"); + expect(getAgentConfigTestActionLabel({ isCreate: true, isDirty: true })).toBe("Test"); + }); + + it("saves a dirty edit draft before running the environment test", async () => { + const callOrder: string[] = []; + const saveDraft = vi.fn(async () => { + callOrder.push("save"); + }); + const runTest = vi.fn(async (): Promise => { + callOrder.push("test"); + return { + adapterType: "claude_local", + status: "pass", + checks: [], + testedAt: new Date(0).toISOString(), + }; + }); + + await runAgentConfigEnvironmentTest({ + isCreate: false, + isDirty: true, + saveDraft, + runTest, + }); + + expect(saveDraft).toHaveBeenCalledTimes(1); + expect(runTest).toHaveBeenCalledTimes(1); + expect(callOrder).toEqual(["save", "test"]); + }); + + it("runs create-mode tests without saving first", async () => { + const saveDraft = vi.fn(async () => {}); + const runTest = vi.fn(async (): Promise => ({ + adapterType: "claude_local", + status: "pass", + checks: [], + testedAt: new Date(0).toISOString(), + })); + + await runAgentConfigEnvironmentTest({ + isCreate: true, + isDirty: true, + saveDraft, + runTest, + }); + + expect(saveDraft).not.toHaveBeenCalled(); + expect(runTest).toHaveBeenCalledTimes(1); + }); +}); + function makeEnvironment(overrides: Partial): Environment { return { id: "env-1", diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 3533e423..e123b6cc 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -95,7 +95,7 @@ type AgentConfigFormProps = { | { mode: "edit"; agent: Agent; - onSave: (patch: Record) => void; + onSave: (patch: Record) => void | Promise; isSaving?: boolean; } ); @@ -116,6 +116,22 @@ export function supportsAdapterModelRefresh(adapterType: string): boolean { return adapterType === "claude_local" || adapterType === "codex_local" || adapterType === "acpx_local"; } +export function getAgentConfigTestActionLabel(input: { isCreate: boolean; isDirty: boolean }): string { + return !input.isCreate && input.isDirty ? "Save + Test" : "Test"; +} + +export async function runAgentConfigEnvironmentTest(input: { + isCreate: boolean; + isDirty: boolean; + saveDraft?: () => void | Promise; + runTest: () => Promise; +}) { + if (!input.isCreate && input.isDirty) { + await input.saveDraft?.(); + } + return await input.runTest(); +} + function isOverlayDirty(o: AgentConfigOverlay): boolean { return ( Object.keys(o.identity).length > 0 || @@ -307,9 +323,9 @@ export function AgentConfigForm(props: AgentConfigFormProps) { setOverlay({ ...emptyOverlay }); }, []); - const handleSave = useCallback(() => { + const handleSave = useCallback(async () => { if (isCreate || !isDirty) return; - props.onSave(buildAgentUpdatePatch(props.agent, overlay)); + await props.onSave(buildAgentUpdatePatch(props.agent, overlay)); }, [isCreate, isDirty, overlay, props]); useEffect(() => { @@ -508,11 +524,36 @@ export function AgentConfigForm(props: AgentConfigFormProps) { }); }, }); - const testEnvironmentDisabled = testEnvironment.isPending || !selectedCompanyId; + const [testActionPending, setTestActionPending] = useState(false); + const [testActionError, setTestActionError] = useState(null); + const testActionLabel = getAgentConfigTestActionLabel({ isCreate, isDirty }); + const isSavePending = !isCreate && Boolean(props.isSaving); + const testEnvironmentDisabled = testActionPending || isSavePending || !selectedCompanyId; + const runEnvironmentTest = useCallback(async () => { + if (!selectedCompanyId) { + throw new Error("Select a company to test adapter environment"); + } + setTestActionPending(true); + setTestActionError(null); + testEnvironment.reset(); + try { + return await runAgentConfigEnvironmentTest({ + isCreate, + isDirty, + saveDraft: !isCreate ? handleSave : undefined, + runTest: () => testEnvironment.mutateAsync(), + }); + } catch (error) { + setTestActionError(error instanceof Error ? error.message : "Environment test failed"); + throw error; + } finally { + setTestActionPending(false); + } + }, [selectedCompanyId, isCreate, isDirty, handleSave, testEnvironment]); const triggerTestEnvironment = useCallback(() => { if (testEnvironmentDisabled) return; - testEnvironment.mutate(); - }, [testEnvironment.mutate, testEnvironmentDisabled]); + void runEnvironmentTest().catch(() => undefined); + }, [runEnvironmentTest, testEnvironmentDisabled]); useEffect(() => { if (!showAdapterTestEnvironmentButton || !props.onTestActionChange) return; @@ -526,7 +567,7 @@ export function AgentConfigForm(props: AgentConfigFormProps) { if (!showAdapterTestEnvironmentButton || !props.onTestActionStateChange) return; props.onTestActionStateChange({ disabled: testEnvironmentDisabled, - pending: testEnvironment.isPending, + pending: testActionPending, }); return () => { props.onTestActionStateChange?.({ disabled: true, pending: false }); @@ -535,23 +576,24 @@ export function AgentConfigForm(props: AgentConfigFormProps) { showAdapterTestEnvironmentButton, props.onTestActionStateChange, testEnvironmentDisabled, - testEnvironment.isPending, + testActionPending, ]); useEffect(() => { if (!props.onTestFeedbackChange) return; props.onTestFeedbackChange({ - errorMessage: testEnvironment.error instanceof Error - ? testEnvironment.error.message - : testEnvironment.error - ? "Environment test failed" - : null, + errorMessage: testActionError + ?? (testEnvironment.error instanceof Error + ? testEnvironment.error.message + : testEnvironment.error + ? "Environment test failed" + : null), result: testEnvironment.data ?? null, }); return () => { props.onTestFeedbackChange?.({ errorMessage: null, result: null }); }; - }, [props.onTestFeedbackChange, testEnvironment.data, testEnvironment.error]); + }, [props.onTestFeedbackChange, testActionError, testEnvironment.data, testEnvironment.error]); // Current model for display const currentModelId = isCreate @@ -895,7 +937,7 @@ export function AgentConfigForm(props: AgentConfigFormProps) { onClick={triggerTestEnvironment} disabled={testEnvironmentDisabled} > - {testEnvironment.isPending ? "Testing..." : "Test"} + {testActionPending ? `${testActionLabel}...` : testActionLabel} )} @@ -955,11 +997,12 @@ export function AgentConfigForm(props: AgentConfigFormProps) { )} - {showInlineAdapterTestEnvironmentFeedback && testEnvironment.error && ( + {showInlineAdapterTestEnvironmentFeedback && (testActionError || testEnvironment.error) && (
- {testEnvironment.error instanceof Error - ? testEnvironment.error.message - : "Environment test failed"} + {testActionError + ?? (testEnvironment.error instanceof Error + ? testEnvironment.error.message + : "Environment test failed")}
)} diff --git a/ui/src/components/AgentProperties.tsx b/ui/src/components/AgentProperties.tsx index 0b8dbe87..c0fdffd1 100644 --- a/ui/src/components/AgentProperties.tsx +++ b/ui/src/components/AgentProperties.tsx @@ -44,6 +44,13 @@ export function AgentProperties({ agent, runtimeState }: AgentPropertiesProps) { + {lastErrorIsActive && agent.errorReason && ( + + + {agent.errorReason} + + + )} {roleLabels[agent.role] ?? agent.role} diff --git a/ui/src/components/IssueRecoveryActionCard.tsx b/ui/src/components/IssueRecoveryActionCard.tsx index d086c3dc..d236b92f 100644 --- a/ui/src/components/IssueRecoveryActionCard.tsx +++ b/ui/src/components/IssueRecoveryActionCard.tsx @@ -47,6 +47,7 @@ const KIND_LABEL: Record = { missing_disposition: "Missing Disposition", stranded_assigned_issue: "Stranded Task", workspace_validation: "Workspace Validation", + configuration_validation: "Configuration Validation", active_run_watchdog: "Active Watchdog", issue_graph_liveness: "Graph Liveness", }; @@ -57,6 +58,8 @@ const KIND_HEADLINE: Record = { "Paperclip retried this task's last run and it still has no live execution path.", workspace_validation: "Paperclip stopped this run because the task's git workspace could not be validated.", + configuration_validation: + "Paperclip stopped before dispatching this run because required secret/env bindings are missing.", active_run_watchdog: "The active run has been silent. Recovery is observing without interrupting it.", issue_graph_liveness: diff --git a/ui/src/pages/AgentDetail.tsx b/ui/src/pages/AgentDetail.tsx index ba373445..2d446f5e 100644 --- a/ui/src/pages/AgentDetail.tsx +++ b/ui/src/pages/AgentDetail.tsx @@ -1659,7 +1659,7 @@ function ConfigurationTab({ updateAgent.mutate(patch)} + onSave={(patch) => updateAgent.mutateAsync(patch)} isSaving={isConfigSaving} adapterModels={adapterModels} onDirtyChange={onDirtyChange}