diff --git a/packages/db/src/migrations/0212_approval_create_idempotency.sql b/packages/db/src/migrations/0212_approval_create_idempotency.sql new file mode 100644 index 000000000000..6cd9011fd5ff --- /dev/null +++ b/packages/db/src/migrations/0212_approval_create_idempotency.sql @@ -0,0 +1,17 @@ +ALTER TABLE "approvals" ADD COLUMN IF NOT EXISTS "idempotency_key" text; +--> statement-breakpoint +-- paperclip:migration-safety-ignore large-create-index-not-concurrently: Drizzle migrations run transactionally, so CONCURRENTLY is unavailable; the partial index initially contains no rows because the new column is null for every existing approval. +CREATE UNIQUE INDEX IF NOT EXISTS "approvals_company_agent_idempotency_idx" + ON "approvals" USING btree ("company_id", "requested_by_agent_id", "idempotency_key") + WHERE "idempotency_key" IS NOT NULL AND "requested_by_agent_id" IS NOT NULL AND "status" IN ('pending', 'revision_requested'); +--> statement-breakpoint +-- paperclip:migration-safety-ignore large-create-index-not-concurrently: Drizzle migrations run transactionally, so CONCURRENTLY is unavailable; the partial index initially contains no rows because the new column is null for every existing approval. +CREATE UNIQUE INDEX IF NOT EXISTS "approvals_company_user_idempotency_idx" + ON "approvals" USING btree ("company_id", "requested_by_user_id", "idempotency_key") + WHERE "idempotency_key" IS NOT NULL AND "requested_by_user_id" IS NOT NULL AND "status" IN ('pending', 'revision_requested'); +--> statement-breakpoint +-- Supports the cheap existence check: filtering pending approvals by requester without +-- reading the payload column. The list endpoint's summary view is the intended caller. +-- paperclip:migration-safety-ignore large-create-index-not-concurrently: Drizzle migrations run transactionally, so CONCURRENTLY is unavailable; the approvals table is small (low hundreds of rows). +CREATE INDEX IF NOT EXISTS "approvals_company_status_requested_by_agent_idx" + ON "approvals" USING btree ("company_id", "status", "requested_by_agent_id"); diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index 4917ed917858..5c4fd210a143 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -1429,6 +1429,13 @@ "when": 1786060800000, "tag": "0211_detached_queued_run_recovery_outbox", "breakpoints": true + }, + { + "idx": 212, + "version": "7", + "when": 1786060801000, + "tag": "0212_approval_create_idempotency", + "breakpoints": true } ] } diff --git a/packages/db/src/schema/approvals.ts b/packages/db/src/schema/approvals.ts index d6ccbc7990a3..e2d7c0bafbad 100644 --- a/packages/db/src/schema/approvals.ts +++ b/packages/db/src/schema/approvals.ts @@ -1,4 +1,5 @@ -import { pgTable, uuid, text, timestamp, jsonb, index } from "drizzle-orm/pg-core"; +import { pgTable, uuid, text, timestamp, jsonb, index, uniqueIndex } from "drizzle-orm/pg-core"; +import { sql } from "drizzle-orm"; import { companies } from "./companies.js"; import { agents } from "./agents.js"; @@ -13,6 +14,9 @@ export const approvals = pgTable( requestedByUserId: text("requested_by_user_id"), status: text("status").notNull().default("pending"), payload: jsonb("payload").$type>().notNull(), + // Requester-supplied dedupe token. Scoped to (company, requester, key) and only + // enforced while the approval is still undecided — see the partial indexes below. + idempotencyKey: text("idempotency_key"), decisionNote: text("decision_note"), decidedByUserId: text("decided_by_user_id"), decidedAt: timestamp("decided_at", { withTimezone: true }), @@ -26,5 +30,19 @@ export const approvals = pgTable( table.type, ), linkedAgentIdx: index("approvals_linked_agent_idx").on(table.linkedAgentId), + // Two indexes rather than one because the requester is stored in one of two + // mutually exclusive columns. Both are scoped to the undecided statuses: once the + // board has answered an ask, re-filing the same key is a legitimately new request + // (the answer may have changed the situation), so the key is released on decision. + companyAgentIdempotencyIdx: uniqueIndex("approvals_company_agent_idempotency_idx") + .on(table.companyId, table.requestedByAgentId, table.idempotencyKey) + .where( + sql`${table.idempotencyKey} IS NOT NULL AND ${table.requestedByAgentId} IS NOT NULL AND ${table.status} IN ('pending', 'revision_requested')`, + ), + companyUserIdempotencyIdx: uniqueIndex("approvals_company_user_idempotency_idx") + .on(table.companyId, table.requestedByUserId, table.idempotencyKey) + .where( + sql`${table.idempotencyKey} IS NOT NULL AND ${table.requestedByUserId} IS NOT NULL AND ${table.status} IN ('pending', 'revision_requested')`, + ), }), ); diff --git a/packages/mcp-server/src/tools.ts b/packages/mcp-server/src/tools.ts index 16cd9374932c..8d8bd0542350 100644 --- a/packages/mcp-server/src/tools.ts +++ b/packages/mcp-server/src/tools.ts @@ -509,11 +509,29 @@ export function createToolDefinitions(client: PaperclipApiClient): ToolDefinitio ), makeTool( "paperclipListApprovals", - "List approvals in a company", - z.object({ companyId: companyIdOptional, status: z.string().optional() }), - async ({ companyId, status }) => { + "List approvals in a company. Default view=full returns whole payload bodies and is expensive (hundreds of KB on a busy queue). Before filing a new approval, check for an existing one with view=count or view=summary — summary omits payload and returns a derived, always-populated `label` per row, so a duplicate check costs a fraction of a re-file. Filter by type, issueId, requestedByAgentId, or idempotencyKey to narrow further.", + z.object({ + companyId: companyIdOptional, + status: z.string().optional(), + type: z.string().optional(), + issueId: z.string().uuid().optional().describe("Only approvals linked to this issue"), + requestedByAgentId: z.string().uuid().optional(), + idempotencyKey: z + .string() + .optional() + .describe("Exact-match probe for a key you are about to reuse"), + view: z + .enum(["full", "summary", "count"]) + .optional() + .describe("full (default, includes payload) | summary (no payload, adds label) | count"), + }), + async ({ companyId, ...query }) => { const resolved = await client.resolveCompany({ override: companyId }); - const qs = status ? `?status=${encodeURIComponent(status)}` : ""; + const params = new URLSearchParams(); + for (const [key, value] of Object.entries(query)) { + if (value !== undefined && value !== null) params.set(key, String(value)); + } + const qs = params.size > 0 ? `?${params.toString()}` : ""; return client.requestJson("GET", `/companies/${resolved}/approvals${qs}`, { companyId: resolved, }); @@ -521,7 +539,7 @@ export function createToolDefinitions(client: PaperclipApiClient): ToolDefinitio ), makeTool( "paperclipCreateApproval", - "Create a board approval request, optionally linked to one or more issues", + "Create a board approval request, optionally linked to one or more issues. Pass idempotencyKey (a stable token derived from the ask itself, e.g. \"rotate-creds:BLO-18969\") so a retry replays the original instead of filing a duplicate: the response then carries deduplicated:true and a statusReadback line telling you the original is still pending. A pending approval emits nothing on its own, so use that readback — or paperclipListApprovals with view=count — instead of re-filing to find out.", createApprovalToolSchema, async ({ companyId, ...body }) => { const resolved = await client.resolveCompany({ override: companyId }); diff --git a/packages/shared/src/constants.ts b/packages/shared/src/constants.ts index fc59e3cc4f0f..bc4f9ba15161 100644 --- a/packages/shared/src/constants.ts +++ b/packages/shared/src/constants.ts @@ -642,6 +642,20 @@ export const APPROVAL_STATUSES = [ ] as const; export type ApprovalStatus = (typeof APPROVAL_STATUSES)[number]; +/** + * Statuses in which an approval has not yet been answered by the board. + * + * Single source of truth for three things that MUST agree, because a drift between + * them turns an idempotent replay into a raw unique-violation 500: + * 1. the create-side dedupe lookup (`approvalService.createWithIdempotency`), + * 2. the partial unique indexes on `approvals.idempotency_key`, + * 3. which approvals can still be resolved. + * Migration `0210_approval_create_idempotency.sql` hardcodes this set in SQL — it is + * frozen history and cannot import, so a change here needs a follow-up migration. + */ +export const APPROVAL_UNDECIDED_STATUSES = ["pending", "revision_requested"] as const; +export type ApprovalUndecidedStatus = (typeof APPROVAL_UNDECIDED_STATUSES)[number]; + export const SECRET_PROVIDERS = [ "local_encrypted", "aws_secrets_manager", diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 7b1910cb78e4..cd6168a50612 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -257,6 +257,7 @@ export { PROJECT_COLORS, APPROVAL_TYPES, APPROVAL_STATUSES, + APPROVAL_UNDECIDED_STATUSES, SECRET_PROVIDERS, SECRET_PROVIDER_CONFIG_STATUSES, SECRET_PROVIDER_CONFIG_HEALTH_STATUSES, @@ -437,6 +438,7 @@ export { type PauseReason, type ApprovalType, type ApprovalStatus, + type ApprovalUndecidedStatus, type SecretProvider, type SecretProviderConfigStatus, type SecretProviderConfigHealthStatus, @@ -1705,6 +1707,7 @@ export { export type { Milestone, CreateMilestoneInput, UpdateMilestoneInput } from "./types/milestone.js"; export { createApprovalSchema, + listApprovalsQuerySchema, upsertBudgetPolicySchema, resolveBudgetIncidentSchema, resolveApprovalSchema, @@ -1713,6 +1716,7 @@ export { withdrawApprovalSchema, addApprovalCommentSchema, type CreateApproval, + type ListApprovalsQuery, type UpsertBudgetPolicy, type ResolveBudgetIncident, type ResolveApproval, diff --git a/packages/shared/src/validators/approval.test.ts b/packages/shared/src/validators/approval.test.ts index b610ff97b6e2..9eb672ab283a 100644 --- a/packages/shared/src/validators/approval.test.ts +++ b/packages/shared/src/validators/approval.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { addApprovalCommentSchema, createApprovalSchema, + listApprovalsQuerySchema, requestApprovalRevisionSchema, resubmitApprovalSchema, resolveApprovalSchema, @@ -31,6 +32,51 @@ describe("approval validators", () => { expect(requestApprovalRevisionSchema.parse({ decisionNote: "Decision\\r\\nRevise." }).decisionNote) .toBe("Decision\nRevise."); }); + // BLO-19132: dedupe token + cheap existence check. + + it("accepts an idempotency key on create and trims it", () => { + const parsed = createApprovalSchema.parse({ + type: "request_board_approval", + payload: { title: "Rotate credentials" }, + idempotencyKey: " rotate-creds-blo-18969 ", + }); + expect(parsed.idempotencyKey).toBe("rotate-creds-blo-18969"); + }); + + it("keeps the idempotency key optional so existing callers are unaffected", () => { + const parsed = createApprovalSchema.parse({ + type: "request_board_approval", + payload: { title: "Rotate credentials" }, + }); + expect(parsed.idempotencyKey).toBeUndefined(); + }); + + it("rejects an empty or oversized idempotency key", () => { + const base = { type: "request_board_approval", payload: { title: "Rotate credentials" } }; + expect(createApprovalSchema.safeParse({ ...base, idempotencyKey: " " }).success).toBe(false); + expect(createApprovalSchema.safeParse({ ...base, idempotencyKey: "x".repeat(256) }).success).toBe(false); + expect(createApprovalSchema.safeParse({ ...base, idempotencyKey: "x".repeat(255) }).success).toBe(true); + }); + + it("defaults the listing view to full so the existing listing contract is unchanged", () => { + expect(listApprovalsQuerySchema.parse({}).view).toBe("full"); + }); + + it("accepts the cheap listing views and the narrowing filters", () => { + const parsed = listApprovalsQuerySchema.parse({ + view: "summary", + status: "pending", + type: "request_board_approval", + issueId: "00000000-0000-0000-0000-000000000001", + }); + expect(parsed).toMatchObject({ view: "summary", status: "pending" }); + expect(listApprovalsQuerySchema.parse({ view: "count" }).view).toBe("count"); + }); + + it("rejects an unknown view and an unknown status rather than coercing them", () => { + expect(listApprovalsQuerySchema.safeParse({ view: "everything" }).success).toBe(false); + expect(listApprovalsQuerySchema.safeParse({ status: "pendinggg" }).success).toBe(false); + }); }); describe("createApprovalSchema payload.title requirement", () => { diff --git a/packages/shared/src/validators/approval.ts b/packages/shared/src/validators/approval.ts index 6ac17e46b614..590ca54ac5ad 100644 --- a/packages/shared/src/validators/approval.ts +++ b/packages/shared/src/validators/approval.ts @@ -1,5 +1,5 @@ import { z } from "zod"; -import { APPROVAL_TYPES } from "../constants.js"; +import { APPROVAL_STATUSES, APPROVAL_TYPES } from "../constants.js"; import { multilineTextSchema } from "./text.js"; const approvalTitleMessage = @@ -18,10 +18,35 @@ export const createApprovalSchema = z.object({ requestedByAgentId: z.string().uuid().optional().nullable(), payload: approvalPayloadSchema, issueIds: z.array(z.string().uuid()).optional(), + idempotencyKey: z + .string() + .trim() + .min(1) + .max(255) + .describe( + "Dedupe token. A second create with the same key, from the same requester, while the first is still undecided replays the original approval instead of filing a duplicate.", + ) + .optional() + .nullable(), }); export type CreateApproval = z.infer; +/** + * Query parameters for listing approvals. `view=summary` omits the `payload` body, + * which is what makes a pre-file existence check cheap enough to be worth doing. + */ +export const listApprovalsQuerySchema = z.object({ + status: z.enum(APPROVAL_STATUSES).optional(), + type: z.enum(APPROVAL_TYPES).optional(), + issueId: z.string().uuid().optional(), + requestedByAgentId: z.string().uuid().optional(), + idempotencyKey: z.string().trim().min(1).max(255).optional(), + view: z.enum(["full", "summary", "count"]).optional().default("full"), +}); + +export type ListApprovalsQuery = z.infer; + export const resolveApprovalSchema = z.object({ decisionNote: multilineTextSchema.optional().nullable(), }); diff --git a/packages/shared/src/validators/index.ts b/packages/shared/src/validators/index.ts index 0b33732540cc..96c534340009 100644 --- a/packages/shared/src/validators/index.ts +++ b/packages/shared/src/validators/index.ts @@ -552,12 +552,14 @@ export { export { createApprovalSchema, + listApprovalsQuerySchema, resolveApprovalSchema, requestApprovalRevisionSchema, resubmitApprovalSchema, withdrawApprovalSchema, addApprovalCommentSchema, type CreateApproval, + type ListApprovalsQuery, type ResolveApproval, type RequestApprovalRevision, type ResubmitApproval, diff --git a/server/src/__tests__/approval-routes-idempotency.test.ts b/server/src/__tests__/approval-routes-idempotency.test.ts index 0077da1f210f..fab9e41cc650 100644 --- a/server/src/__tests__/approval-routes-idempotency.test.ts +++ b/server/src/__tests__/approval-routes-idempotency.test.ts @@ -4,8 +4,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const mockApprovalService = vi.hoisted(() => ({ list: vi.fn(), + listSummary: vi.fn(), + countBy: vi.fn(), getById: vi.fn(), create: vi.fn(), + createWithIdempotency: vi.fn(), approve: vi.fn(), reject: vi.fn(), requestRevision: vi.fn(), @@ -27,6 +30,7 @@ const mockSecretService = vi.hoisted(() => ({ normalizeHireApprovalPayloadForPersistence: vi.fn(), })); +const mockDeferredActivityPublish = vi.hoisted(() => vi.fn()); const mockLogActivity = vi.hoisted(() => vi.fn()); const mockAccessService = vi.hoisted(() => ({ decide: vi.fn(), @@ -119,8 +123,26 @@ describe("approval routes idempotent retries", () => { registerModuleMocks(); vi.clearAllMocks(); mockApprovalService.list.mockReset(); + mockApprovalService.listSummary.mockReset(); + mockApprovalService.countBy.mockReset(); mockApprovalService.getById.mockReset(); mockApprovalService.create.mockReset(); + mockApprovalService.createWithIdempotency.mockReset(); + // The route calls createWithIdempotency; the non-dedupe branch is behaviourally + // identical to the old create, so delegate. Existing assertions on `create` — the + // args the route builds — keep working unchanged, and tests that exercise a replay + // override this implementation. + mockApprovalService.createWithIdempotency.mockImplementation( + async ( + companyId: string, + data: Record, + options?: { afterCreate?: (txDb: unknown, approval: Record) => Promise }, + ) => { + const approval = await mockApprovalService.create(companyId, data); + await options?.afterCreate?.({ tx: true }, approval); + return { approval, deduplicated: false }; + }, + ); mockApprovalService.approve.mockReset(); mockApprovalService.reject.mockReset(); mockApprovalService.requestRevision.mockReset(); @@ -131,7 +153,9 @@ describe("approval routes idempotent retries", () => { mockIssueApprovalService.listIssuesForApproval.mockReset(); mockIssueApprovalService.linkManyForApproval.mockReset(); mockSecretService.normalizeHireApprovalPayloadForPersistence.mockReset(); + mockDeferredActivityPublish.mockReset(); mockLogActivity.mockReset(); + mockLogActivity.mockResolvedValue(mockDeferredActivityPublish); mockAccessService.decide.mockReset(); mockAccessService.decide.mockResolvedValue({ allowed: true, @@ -141,7 +165,6 @@ describe("approval routes idempotent retries", () => { }); mockHeartbeatService.wakeup.mockResolvedValue({ id: "wake-1" }); mockIssueApprovalService.listIssuesForApproval.mockResolvedValue([{ id: "issue-1" }]); - mockLogActivity.mockResolvedValue(undefined); }); it("does not emit duplicate approval side effects when approve is already resolved", async () => { @@ -391,29 +414,9 @@ describe("approval routes idempotent retries", () => { issueIds: ["00000000-0000-0000-0000-000000000001"], }), }), + { deferPublish: true }, ); - }); - - it("rejects an agent id on the generic hire approval route", async () => { - mockSecretService.normalizeHireApprovalPayloadForPersistence.mockResolvedValue({ - name: "Untrusted hire", - agentId: "00000000-0000-0000-0000-000000000002", - }); - - const res = await request(await createAgentApp()) - .post("/api/companies/company-1/approvals") - .send({ - type: "hire_agent", - payload: { - title: "Approve agent hire", - name: "Untrusted hire", - agentId: "00000000-0000-0000-0000-000000000002", - }, - }); - - expect(res.status, JSON.stringify(res.body)).toBe(422); - expect(res.body.error).toContain("cannot bind an existing agent"); - expect(mockApprovalService.create).not.toHaveBeenCalled(); + expect(mockDeferredActivityPublish).toHaveBeenCalledTimes(1); }); it("carries the payload `note` into details as description (note alias)", async () => { @@ -451,7 +454,9 @@ describe("approval routes idempotent retries", () => { description: "needs board sign-off", }), }), + { deferPublish: true }, ); + expect(mockDeferredActivityPublish).toHaveBeenCalledTimes(1); }); // `requestedByAgentId` is an attribution signal other subsystems reason about, so an agent must @@ -487,12 +492,12 @@ describe("approval routes idempotent retries", () => { ); }); - it("honours a body-supplied requestedByAgentId from a user actor", async () => { + it("ignores a body-supplied requestedByAgentId from a user actor", async () => { mockApprovalService.create.mockResolvedValue({ id: "approval-attr-user", companyId: "company-1", type: "request_board_approval", - requestedByAgentId: "00000000-0000-0000-0000-0000000000ff", + requestedByAgentId: null, requestedByUserId: "user-1", status: "pending", payload: { title: "Approve hosting spend" }, @@ -515,7 +520,7 @@ describe("approval routes idempotent retries", () => { expect(mockApprovalService.create).toHaveBeenCalledWith( "company-1", expect.objectContaining({ - requestedByAgentId: "00000000-0000-0000-0000-0000000000ff", + requestedByAgentId: null, requestedByUserId: "user-1", }), ); @@ -579,6 +584,324 @@ describe("approval routes idempotent retries", () => { expect(mockApprovalService.resubmit).not.toHaveBeenCalled(); }); + // --------------------------------------------------------------------------- + // BLO-19132: create-side dedupe and the cheap existence check. + // + // The defect these cover: filing a duplicate approval was cheaper than checking + // whether one already existed, and a pending approval emitted nothing back to its + // requester, so retrying was the only way to learn anything. Three asks for one PR + // review landed inside 73 minutes because of it. + // --------------------------------------------------------------------------- + + it("replays the original approval when an agent reuses an idempotency key", async () => { + const existing = { + id: "approval-original", + companyId: "company-1", + type: "request_board_approval", + requestedByAgentId: "agent-1", + requestedByUserId: null, + status: "pending", + payload: { title: "Trigger exact-head human review for MOQtail PR #312" }, + idempotencyKey: "moqtail-312-exact-head-review", + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + createdAt: new Date(Date.now() - 73 * 60 * 1000), + updatedAt: new Date(), + }; + mockApprovalService.createWithIdempotency.mockResolvedValue({ + approval: existing, + deduplicated: true, + }); + + const res = await request(await createAgentApp()) + .post("/api/companies/company-1/approvals") + .send({ + type: "request_board_approval", + issueIds: ["00000000-0000-0000-0000-000000000001"], + payload: { title: "Trigger exact-head human review for MOQtail PR #312" }, + idempotencyKey: "moqtail-312-exact-head-review", + }); + + // 200, not 201: nothing was created. + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ + id: "approval-original", + deduplicated: true, + deduplicationReason: "idempotency_key", + }); + // The readback is the signal that makes retrying unnecessary. + expect(res.body.statusReadback).toContain("still pending"); + expect(res.body.statusReadback).toContain("No duplicate was created"); + expect(res.body.pendingForMs).toBeGreaterThan(60 * 60 * 1000); + + // Issue links are idempotent (onConflictDoNothing), so applying the caller's links + // to the ORIGINAL approval is correct — a retry naming a new issue still attaches + // it. What must not repeat is the board notification. + expect(mockIssueApprovalService.linkManyForApproval).toHaveBeenCalledWith( + "approval-original", + ["00000000-0000-0000-0000-000000000001"], + { agentId: "agent-1", userId: null }, + ); + // Re-logging would put a second card in front of a human for an ask they have + // already been shown — the exact harm this ticket is about. + expect(mockLogActivity).not.toHaveBeenCalled(); + }); + + it("forwards the idempotency key to the service on a first filing", async () => { + mockApprovalService.create.mockResolvedValue({ + id: "approval-first", + companyId: "company-1", + type: "request_board_approval", + requestedByAgentId: "agent-1", + requestedByUserId: null, + status: "pending", + payload: { title: "Rotate credentials" }, + idempotencyKey: "rotate-creds-blo-18969", + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + createdAt: new Date("2026-08-02T00:00:00.000Z"), + updatedAt: new Date("2026-08-02T00:00:00.000Z"), + }); + + const res = await request(await createAgentApp()) + .post("/api/companies/company-1/approvals") + .send({ + type: "request_board_approval", + payload: { title: "Rotate credentials" }, + idempotencyKey: "rotate-creds-blo-18969", + }); + + expect([200, 201], JSON.stringify(res.body)).toContain(res.status); + expect(res.body.deduplicated).toBeUndefined(); + expect(mockApprovalService.createWithIdempotency).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ idempotencyKey: "rotate-creds-blo-18969" }), + expect.objectContaining({ afterCreate: expect.any(Function) }), + ); + // A genuinely new filing still notifies. + expect(mockLogActivity).toHaveBeenCalled(); + expect(mockDeferredActivityPublish).toHaveBeenCalledTimes(1); + }); + + it("flushes approval.created only after the create transaction returns", async () => { + const approval = { + id: "approval-deferred", + companyId: "company-1", + type: "request_board_approval", + requestedByAgentId: "agent-1", + requestedByUserId: null, + status: "pending", + payload: { title: "Review production deploy" }, + idempotencyKey: "deploy-review", + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + createdAt: new Date("2026-08-02T00:00:00.000Z"), + updatedAt: new Date("2026-08-02T00:00:00.000Z"), + }; + const publish = vi.fn(); + mockLogActivity.mockResolvedValueOnce(publish); + mockApprovalService.createWithIdempotency.mockImplementationOnce( + async ( + _companyId: string, + _data: Record, + options?: { afterCreate?: (txDb: unknown, approval: Record) => Promise }, + ) => { + await options?.afterCreate?.({ tx: true }, approval); + expect(publish).not.toHaveBeenCalled(); + return { approval, deduplicated: false }; + }, + ); + + const res = await request(await createAgentApp()) + .post("/api/companies/company-1/approvals") + .send({ + type: "request_board_approval", + payload: { title: "Review production deploy" }, + idempotencyKey: "deploy-review", + }); + + expect(res.status, JSON.stringify(res.body)).toBe(201); + expect(mockLogActivity).toHaveBeenCalledWith( + { tx: true }, + expect.objectContaining({ + action: "approval.created", + entityId: "approval-deferred", + }), + { deferPublish: true }, + ); + expect(publish).toHaveBeenCalledTimes(1); + }); + + it("does not flush deferred approval.created when the create transaction rejects", async () => { + const approval = { + id: "approval-rolled-back", + companyId: "company-1", + type: "request_board_approval", + requestedByAgentId: "agent-1", + requestedByUserId: null, + status: "pending", + payload: { title: "Review production deploy" }, + idempotencyKey: "deploy-review", + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + createdAt: new Date("2026-08-02T00:00:00.000Z"), + updatedAt: new Date("2026-08-02T00:00:00.000Z"), + }; + const publish = vi.fn(); + mockLogActivity.mockResolvedValueOnce(publish); + mockApprovalService.createWithIdempotency.mockImplementationOnce( + async ( + _companyId: string, + _data: Record, + options?: { afterCreate?: (txDb: unknown, approval: Record) => Promise }, + ) => { + await options?.afterCreate?.({ tx: true }, approval); + throw new Error("rollback after activity logging"); + }, + ); + + const res = await request(await createAgentApp()) + .post("/api/companies/company-1/approvals") + .send({ + type: "request_board_approval", + payload: { title: "Review production deploy" }, + idempotencyKey: "deploy-review", + }); + + expect(res.status).toBe(500); + expect(mockLogActivity).toHaveBeenCalledWith( + { tx: true }, + expect.objectContaining({ + action: "approval.created", + entityId: "approval-rolled-back", + }), + { deferPublish: true }, + ); + expect(publish).not.toHaveBeenCalled(); + }); + + it("serves a count-only listing with every accepted filter", async () => { + mockApprovalService.countBy.mockResolvedValue(63); + + const res = await request(await createApp()) + .get( + "/api/companies/company-1/approvals?view=count&status=pending&type=request_board_approval&issueId=00000000-0000-0000-0000-000000000001&requestedByAgentId=00000000-0000-0000-0000-000000000002&idempotencyKey=rotate-creds", + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toEqual({ count: 63 }); + expect(mockApprovalService.countBy).toHaveBeenCalledWith("company-1", { + status: "pending", + type: "request_board_approval", + issueId: "00000000-0000-0000-0000-000000000001", + requestedByAgentId: "00000000-0000-0000-0000-000000000002", + idempotencyKey: "rotate-creds", + }); + // The expensive path must not run. + expect(mockApprovalService.list).not.toHaveBeenCalled(); + }); + + it("serves a summary listing that omits payload and filters by linked issue", async () => { + mockApprovalService.listSummary.mockResolvedValue([ + { + id: "approval-1", + type: "request_board_approval", + status: "pending", + requestedByAgentId: "agent-1", + requestedByUserId: null, + idempotencyKey: "k1", + createdAt: new Date("2026-08-02T00:00:00.000Z"), + decidedAt: null, + label: "Rotate credentials", + }, + ]); + + const res = await request(await createApp()) + .get( + "/api/companies/company-1/approvals?view=summary&status=pending&issueId=00000000-0000-0000-0000-000000000001", + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toHaveLength(1); + expect(res.body[0].label).toBe("Rotate credentials"); + // The payload body is what makes the full listing expensive; it must be absent. + expect(res.body[0]).not.toHaveProperty("payload"); + expect(mockApprovalService.listSummary).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ + status: "pending", + issueId: "00000000-0000-0000-0000-000000000001", + type: undefined, + requestedByAgentId: undefined, + idempotencyKey: undefined, + }), + ); + expect(mockApprovalService.list).not.toHaveBeenCalled(); + }); + + it("rejects an unknown view rather than silently falling back to the expensive listing", async () => { + const res = await request(await createApp()) + .get("/api/companies/company-1/approvals?view=everything"); + + expect(res.status, JSON.stringify(res.body)).toBe(400); + expect(mockApprovalService.list).not.toHaveBeenCalled(); + expect(mockApprovalService.listSummary).not.toHaveBeenCalled(); + }); + + it("keeps the default listing unchanged when no view is given", async () => { + mockApprovalService.list.mockResolvedValue([ + { + id: "approval-1", + companyId: "company-1", + type: "request_board_approval", + status: "pending", + payload: { title: "Approve hosting spend" }, + requestedByAgentId: "agent-1", + requestedByUserId: null, + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + createdAt: new Date("2026-08-02T00:00:00.000Z"), + updatedAt: new Date("2026-08-02T00:00:00.000Z"), + }, + ]); + + const res = await request(await createApp()) + .get("/api/companies/company-1/approvals?status=pending"); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body[0]).toHaveProperty("payload"); + expect(mockApprovalService.list).toHaveBeenCalledWith("company-1", { + status: "pending", + type: undefined, + issueId: undefined, + requestedByAgentId: undefined, + idempotencyKey: undefined, + }); + }); + + it("applies every accepted filter to the full listing", async () => { + mockApprovalService.list.mockResolvedValue([]); + + const res = await request(await createApp()) + .get( + "/api/companies/company-1/approvals?view=full&status=pending&type=request_board_approval&issueId=00000000-0000-0000-0000-000000000001&requestedByAgentId=00000000-0000-0000-0000-000000000002&idempotencyKey=rotate-creds", + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockApprovalService.list).toHaveBeenCalledWith("company-1", { + status: "pending", + type: "request_board_approval", + issueId: "00000000-0000-0000-0000-000000000001", + requestedByAgentId: "00000000-0000-0000-0000-000000000002", + idempotencyKey: "rotate-creds", + }); + }); + it("blocks status-only recovery runs from creating approvals", async () => { const res = await request(await createAgentApp({ contextSnapshot: { diff --git a/server/src/__tests__/approvals-service.test.ts b/server/src/__tests__/approvals-service.test.ts index 06b1eecf6b2c..927dadcee58d 100644 --- a/server/src/__tests__/approvals-service.test.ts +++ b/server/src/__tests__/approvals-service.test.ts @@ -2,6 +2,10 @@ import { randomUUID } from "node:crypto"; import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { eq } from "drizzle-orm"; import { agents, approvals, companies, createDb } from "@paperclipai/db"; +import fs from "node:fs"; +import path from "node:path"; +import url from "node:url"; +import { APPROVAL_UNDECIDED_STATUSES } from "@paperclipai/shared"; import { approvalService } from "../services/approvals.js"; import { getEmbeddedPostgresTestSupport, @@ -36,6 +40,10 @@ type ApprovalRecord = { status: string; payload: Record; requestedByAgentId: string | null; + requestedByUserId: string | null; + idempotencyKey?: string | null; + createdAt?: Date; + decidedAt?: Date | null; }; function createApproval(status: string): ApprovalRecord { @@ -47,6 +55,7 @@ function createApproval(status: string): ApprovalRecord { status, payload: { agentId: "agent-1" }, requestedByAgentId: "requester-1", + requestedByUserId: null, }; } @@ -486,3 +495,249 @@ describeEmbeddedPostgres("approvalService.withdraw adversarial hire targets", () expect(mockAgentService.terminate).toHaveBeenCalledWith(targetAgentId); }); }); + +// --------------------------------------------------------------------------- +// BLO-19132: create-side dedupe. Two creates with the same key from the same +// requester, while the first is still undecided, must yield ONE approval. +// --------------------------------------------------------------------------- + +/** + * Transaction stub modelling the real table: an insert appends a row, a select + * returns whatever the pre-seeded lookup result is. `inserts` is the assertion + * surface — the whole claim is "one row, not two". + */ +function createTxStub(existingRows: ApprovalRecord[][]) { + const pending = [...existingRows]; + const inserts: Record[] = []; + + const tx = { + execute: vi.fn(async () => undefined), + select: vi.fn(() => ({ + from: vi.fn(() => ({ + where: vi.fn(() => ({ + limit: vi.fn(async () => pending.shift() ?? []), + })), + })), + })), + insert: vi.fn(() => ({ + values: vi.fn((row: Record) => { + inserts.push(row); + return { + returning: vi.fn(() => ({ + then: (resolve: (rows: unknown[]) => unknown) => + resolve([{ ...row, id: `approval-${inserts.length}` }]), + })), + }; + }), + })), + }; + + const db = { + transaction: vi.fn(async (fn: (t: typeof tx) => Promise) => fn(tx)), + insert: tx.insert, + }; + + return { db, tx, inserts }; +} + +describe("approvalService createWithIdempotency", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const baseInput = { + type: "request_board_approval", + payload: { title: "Trigger exact-head human review for MOQtail PR #312" }, + requestedByAgentId: "agent-1", + requestedByUserId: null, + status: "pending", + idempotencyKey: "moqtail-312-exact-head-review", + }; + + it("inserts on the first call and replays on the second — one approval, not two", async () => { + // First call: no existing row. Second call: the row the first one created. + const stub = createTxStub([[], []]); + const svc = approvalService(stub.db as any); + + const first = await svc.createWithIdempotency("company-1", baseInput as any); + expect(first.deduplicated).toBe(false); + expect(stub.inserts).toHaveLength(1); + + // Re-seed the lookup with the row that now exists, then retry the same ask. + const stub2 = createTxStub([[{ ...first.approval } as any]]); + const svc2 = approvalService(stub2.db as any); + const second = await svc2.createWithIdempotency("company-1", baseInput as any); + + expect(second.deduplicated).toBe(true); + expect(second.approval.id).toBe(first.approval.id); + // The claim that matters: the retry inserted nothing. + expect(stub2.inserts).toHaveLength(0); + }); + + it("takes an advisory lock before the lookup so concurrent retries cannot both insert", async () => { + const stub = createTxStub([[]]); + const svc = approvalService(stub.db as any); + + await svc.createWithIdempotency("company-1", baseInput as any); + + // Without the lock, two simultaneous first-filings both read "not found" and + // both insert; the partial unique index would then reject one with a raw 500 + // rather than replaying it. + expect(stub.tx.execute).toHaveBeenCalledTimes(1); + const lockCall = stub.tx.execute.mock.calls[0]?.[0] as { queryChunks?: unknown[] } | undefined; + expect(JSON.stringify(lockCall)).toContain("pg_advisory_xact_lock"); + }); + + it("rejects an idempotent create with both requester identities set", async () => { + const stub = createTxStub([[]]); + const svc = approvalService(stub.db as any); + + await expect( + svc.createWithIdempotency("company-1", { + ...baseInput, + requestedByUserId: "user-1", + } as any), + ).rejects.toThrow("either an agent or a user"); + + expect(stub.db.transaction).not.toHaveBeenCalled(); + expect(stub.inserts).toHaveLength(0); + }); + + it("runs first-filing side effects inside the idempotent create transaction", async () => { + const stub = createTxStub([[]]); + const svc = approvalService(stub.db as any); + const afterCreate = vi.fn(async () => undefined); + + await svc.createWithIdempotency("company-1", baseInput as any, { afterCreate }); + + expect(afterCreate).toHaveBeenCalledWith( + stub.tx, + expect.objectContaining({ id: "approval-1" }), + ); + }); + + it("does not rerun first-filing side effects when an idempotent create replays", async () => { + const existing = { ...baseInput, id: "approval-original", companyId: "company-1" }; + const stub = createTxStub([[existing as any]]); + const svc = approvalService(stub.db as any); + const afterCreate = vi.fn(async () => undefined); + + const result = await svc.createWithIdempotency("company-1", baseInput as any, { afterCreate }); + + expect(result.deduplicated).toBe(true); + expect(afterCreate).not.toHaveBeenCalled(); + expect(stub.inserts).toHaveLength(0); + }); + + it("does not dedupe when no idempotency key is supplied", async () => { + const stub = createTxStub([[]]); + const svc = approvalService(stub.db as any); + + const res = await svc.createWithIdempotency("company-1", { + ...baseInput, + idempotencyKey: null, + } as any); + + expect(res.deduplicated).toBe(false); + expect(stub.inserts).toHaveLength(1); + expect(stub.inserts[0]?.idempotencyKey).toBeNull(); + // No key means no lock and no lookup — the unkeyed path stays exactly as it was. + expect(stub.db.transaction).not.toHaveBeenCalled(); + }); + + it("treats a whitespace-only key as absent rather than as a dedupe token", async () => { + const stub = createTxStub([[]]); + const svc = approvalService(stub.db as any); + + const res = await svc.createWithIdempotency("company-1", { + ...baseInput, + idempotencyKey: " ", + } as any); + + expect(res.deduplicated).toBe(false); + expect(stub.inserts[0]?.idempotencyKey).toBeNull(); + }); +}); + +describe("approvalService listSummary", () => { + it("derives labels from redacted payload snippets instead of raw payload text", async () => { + const row = { + id: "approval-secret", + type: "request_board_approval", + status: "pending", + requestedByAgentId: "agent-1", + requestedByUserId: null, + idempotencyKey: "rotate-creds", + createdAt: new Date("2026-08-02T00:00:00.000Z"), + decidedAt: null, + title: "aaa.bbb.ccc", + summary: "Rotate credentials", + description: "fallback", + }; + const orderBy = vi.fn(async () => [row]); + const where = vi.fn(() => ({ orderBy })); + const from = vi.fn(() => ({ where })); + const select = vi.fn(() => ({ from })); + const svc = approvalService({ select } as any); + + const result = await svc.listSummary("company-1", { + status: "pending", + idempotencyKey: "rotate-creds", + }); + + expect(result).toEqual([ + { + id: "approval-secret", + type: "request_board_approval", + status: "pending", + requestedByAgentId: "agent-1", + requestedByUserId: null, + idempotencyKey: "rotate-creds", + createdAt: new Date("2026-08-02T00:00:00.000Z"), + decidedAt: null, + label: "Rotate credentials", + }, + ]); + expect(result[0]).not.toHaveProperty("title"); + expect(result[0]?.label).not.toBe("aaa.bbb.ccc"); + }); +}); + +describe("approval undecided-status scope stays bound across all three sites", () => { + // The migration is frozen history and the drizzle schema must mirror it verbatim, + // so neither can import the constant — an eager cross-package import at schema + // module scope would also take the whole db schema down if it ever failed to + // resolve, which is a worse failure than the drift it prevents. So the binding is + // asserted here instead: if someone widens APPROVAL_UNDECIDED_STATUSES without a + // follow-up migration, the partial index scope and the create-side dedupe lookup + // silently diverge, and a replay that should return the original becomes a raw + // unique-violation 500. + function repoFile(relative: string) { + const here = path.dirname(url.fileURLToPath(import.meta.url)); + return fs.readFileSync(path.resolve(here, "../../..", relative), "utf8"); + } + + function normalize(clause: string) { + return (clause.match(/'[^']+'/g) ?? []).sort().join(","); + } + + const expected = [...APPROVAL_UNDECIDED_STATUSES].map((s) => `'${s}'`).sort().join(","); + + it("matches the status set hardcoded in migration 0212", () => { + const migration = repoFile("packages/db/src/migrations/0212_approval_create_idempotency.sql"); + const clauses = migration.match(/"status" IN \(([^)]*)\)/g) ?? []; + expect(clauses.length, "migration 0212 no longer scopes its indexes by status").toBe(2); + for (const clause of clauses) { + expect(normalize(clause), `migration clause drifted: ${clause}`).toBe(expected); + } + }); + + it("matches the status set hardcoded in the drizzle schema indexes", () => { + const schema = repoFile("packages/db/src/schema/approvals.ts"); + const clauses = schema.match(/\$\{table\.status\} IN \(([^)]*)\)/g) ?? []; + expect(clauses.length, "approvals schema no longer scopes its indexes by status").toBe(2); + for (const clause of clauses) { + expect(normalize(clause), `schema clause drifted: ${clause}`).toBe(expected); + } + }); +}); diff --git a/server/src/routes/approvals.ts b/server/src/routes/approvals.ts index e892ebe44d89..be8d32e027d7 100644 --- a/server/src/routes/approvals.ts +++ b/server/src/routes/approvals.ts @@ -4,6 +4,7 @@ import { heartbeatRuns, type Db } from "@paperclipai/db"; import { addApprovalCommentSchema, createApprovalSchema, + listApprovalsQuerySchema, requestApprovalRevisionSchema, resolveApprovalSchema, resubmitApprovalSchema, @@ -119,8 +120,36 @@ export function approvalRoutes( const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); if (!(await assertApprovalAccessAllowed(req, res, companyId))) return; - const status = req.query.status as string | undefined; - const result = await svc.list(companyId, status); + + const parsed = listApprovalsQuerySchema.safeParse(req.query); + if (!parsed.success) { + res.status(400).json({ error: "Invalid query", details: parsed.error.flatten() }); + return; + } + const { view, status, type, issueId, requestedByAgentId, idempotencyKey } = parsed.data; + + // `count` and `summary` exist so that checking whether an ask is already filed is + // cheaper than filing it again. The default `full` view is unchanged. + const filters = { + status, + type, + issueId, + requestedByAgentId, + idempotencyKey, + }; + if (view === "count") { + const count = await svc.countBy(companyId, filters); + res.json({ count }); + return; + } + + if (view === "summary") { + const rows = await svc.listSummary(companyId, filters); + res.json(rows); + return; + } + + const result = await svc.list(companyId, filters); res.json(result.map((approval) => redactApprovalPayload(approval))); }); @@ -162,39 +191,8 @@ export function approvalRoutes( } const actor = getActorInfo(req); - const approval = await svc.create(companyId, { - ...approvalInput, - payload: normalizedPayload, - requestedByUserId: actor.actorType === "user" ? actor.actorId : null, - // An agent actor cannot nominate a different requester. The body field stays honoured for - // user actors (a human filing on an agent's behalf), but letting an agent set it would make - // `requestedByAgentId` unusable as an attribution signal — anything downstream that reasons - // about who asked for an approval could be pointed at an innocent agent. - requestedByAgentId: - actor.actorType === "agent" - ? actor.actorId - : (approvalInput.requestedByAgentId ?? null), - status: "pending", - decisionNote: null, - decidedByUserId: null, - decidedAt: null, - updatedAt: new Date(), - }); - - if (uniqueIssueIds.length > 0) { - await issueApprovalsSvc.linkManyForApproval(approval.id, uniqueIssueIds, { - agentId: actor.agentId, - userId: actor.actorType === "user" ? actor.actorId : null, - }); - } - - // Surface the approval's human-facing title/description in the activity - // details so the plugin domain event (built from `details` in logActivity) - // carries them to notifiers. Without this the Slack approval card renders - // only `Type` — every board approval looks identical (every card is just - // `request_board_approval`). `payload` is free-form (z.record), so accept - // either `description` or the common `note` alias. The Slack formatter reads - // `approvalId`, `title`, `description`. + const requestedByAgentId = actor.actorType === "agent" ? actor.actorId : null; + const requestedByUserId = actor.actorType === "user" ? actor.actorId : null; const payloadObj = typeof normalizedPayload === "object" && normalizedPayload !== null ? (normalizedPayload as Record) @@ -208,25 +206,85 @@ export function approvalRoutes( ? payloadObj.note : undefined; - await logActivity(db, { - companyId, - actorType: actor.actorType, - actorId: actor.actorId, - agentId: actor.agentId, - action: "approval.created", - entityType: "approval", - entityId: approval.id, - details: { - type: approval.type, - approvalId: approval.id, - issueIds: uniqueIssueIds, - ...(approvalTitle !== undefined ? { title: approvalTitle } : {}), - ...(approvalDescription !== undefined - ? { description: approvalDescription } - : {}), + const publishCreatedActivityRef: { current: (() => void) | null } = { current: null }; + const { approval, deduplicated } = await svc.createWithIdempotency(companyId, { + ...approvalInput, + payload: normalizedPayload, + // Requester identity is derived only from the authenticated actor, and exactly one + // requester column is populated. Letting a user also nominate `requestedByAgentId` + // makes the idempotency key ambiguous because both requester-scoped unique indexes + // would apply to the same row. + requestedByAgentId, + requestedByUserId, + status: "pending", + decisionNote: null, + decidedByUserId: null, + decidedAt: null, + updatedAt: new Date(), + }, { + afterCreate: async (txDb, createdApproval) => { + if (uniqueIssueIds.length > 0) { + await issueApprovalService(txDb).linkManyForApproval(createdApproval.id, uniqueIssueIds, { + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + }); + } + + publishCreatedActivityRef.current = await logActivity(txDb, { + companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + action: "approval.created", + entityType: "approval", + entityId: createdApproval.id, + details: { + type: createdApproval.type, + approvalId: createdApproval.id, + issueIds: uniqueIssueIds, + ...(approvalTitle !== undefined ? { title: approvalTitle } : {}), + ...(approvalDescription !== undefined + ? { description: approvalDescription } + : {}), + }, + }, { deferPublish: true }); }, }); + // Issue links are applied on both paths. The insert is onConflictDoNothing, so + // re-linking the same issues is a no-op, and a retry that names a new issue still + // gets it attached rather than silently losing it. New filings link inside the + // create transaction above, with the human-facing activity log; replays must not + // emit another activity card. + if (deduplicated && uniqueIssueIds.length > 0) { + await issueApprovalsSvc.linkManyForApproval(approval.id, uniqueIssueIds, { + agentId: actor.agentId, + userId: actor.actorType === "user" ? actor.actorId : null, + }); + } + + publishCreatedActivityRef.current?.(); + + // A replay is not a new filing. Answer with the original plus a readback so the + // requester learns it is still pending without having to file again to find out — + // silence is otherwise indistinguishable from "not yet decided", which is what + // makes retrying the only way to get information, and the queue flood downstream. + if (deduplicated) { + const pendingForMs = Date.now() - new Date(approval.createdAt).getTime(); + res.status(200).json({ + ...redactApprovalPayload(approval), + deduplicated: true, + deduplicationReason: "idempotency_key", + pendingSince: approval.createdAt, + pendingForMs, + statusReadback: + `Approval ${approval.id} (${approval.type}) is still ${approval.status}, filed ` + + `${new Date(approval.createdAt).toISOString()} (${Math.floor(pendingForMs / 60000)} min ago). ` + + `No duplicate was created.`, + }); + return; + } + res.status(201).json(redactApprovalPayload(approval)); }); diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index eda897640aa2..9b576f4a4626 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -73,6 +73,7 @@ import { updateUserSecretValueSchema, // Approval createApprovalSchema, + listApprovalsQuerySchema, resolveApprovalSchema, requestApprovalRevisionSchema, resubmitApprovalSchema, @@ -2841,8 +2842,14 @@ registry.registerPath({ path: "/api/companies/{companyId}/approvals", tags: ["approvals"], summary: "List approvals in a company", - request: { params: z.object({ companyId: z.string() }) }, - responses: { 200: r.ok(), 401: r.unauthorized }, + description: + "view=full (default) returns whole payload bodies. Use view=count or view=summary for a cheap " + + "existence check before filing a new approval — summary omits payload and adds a derived label.", + request: { + params: z.object({ companyId: z.string() }), + query: listApprovalsQuerySchema, + }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, }); registry.registerPath({ diff --git a/server/src/services/approvals.ts b/server/src/services/approvals.ts index 65ba756bab45..b862ee10e65b 100644 --- a/server/src/services/approvals.ts +++ b/server/src/services/approvals.ts @@ -1,21 +1,37 @@ -import { and, asc, eq, inArray } from "drizzle-orm"; +import { and, asc, eq, inArray, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; -import { agents, approvalComments, approvals } from "@paperclipai/db"; +import { agents, approvalComments, approvals, issueApprovals } from "@paperclipai/db"; +import { APPROVAL_UNDECIDED_STATUSES } from "@paperclipai/shared"; import { conflict, notFound, unprocessable } from "../errors.js"; import { redactCurrentUserText } from "../log-redaction.js"; import { logActivity, type LogActivityInput } from "./activity-log.js"; +import { REDACTED_EVENT_VALUE, redactApprovalPayloadByType } from "../redaction.js"; import { agentService } from "./agents.js"; import { budgetService } from "./budgets.js"; import { notifyHireApproved } from "./hire-hook.js"; import { instanceSettingsService } from "./instance-settings.js"; +type ApprovalListFilters = { + status?: string; + type?: string; + issueId?: string; + requestedByAgentId?: string; + idempotencyKey?: string; +}; + export function approvalService(db: Db) { const agentsSvc = agentService(db); const instanceSettings = instanceSettingsService(db); - const canResolveStatuses = new Set(["pending", "revision_requested"]); + // Single source of truth shared with the partial unique indexes on + // approvals.idempotency_key. If these drift, an idempotent replay becomes a raw + // unique-violation 500 instead of returning the original. + const canResolveStatuses = new Set(APPROVAL_UNDECIDED_STATUSES); const resolvableStatuses = Array.from(canResolveStatuses); type ApprovalRecord = typeof approvals.$inferSelect; type ResolutionResult = { approval: ApprovalRecord; applied: boolean }; + type CreateWithIdempotencyOptions = { + afterCreate?: (txDb: Db, approval: ApprovalRecord) => Promise; + }; function redactApprovalComment(comment: T, censorUsernameInLogs: boolean): T { return { @@ -24,6 +40,55 @@ export function approvalService(db: Db) { }; } + function normalizeListFilters(filters?: string | ApprovalListFilters): ApprovalListFilters { + return typeof filters === "string" ? { status: filters } : (filters ?? {}); + } + + function approvalListConditions(companyId: string, filters: ApprovalListFilters = {}) { + const conditions = [eq(approvals.companyId, companyId)]; + if (filters.status) conditions.push(eq(approvals.status, filters.status)); + if (filters.type) conditions.push(eq(approvals.type, filters.type)); + if (filters.requestedByAgentId) { + conditions.push(eq(approvals.requestedByAgentId, filters.requestedByAgentId)); + } + if (filters.idempotencyKey) { + conditions.push(eq(approvals.idempotencyKey, filters.idempotencyKey)); + } + if (filters.issueId) { + conditions.push( + sql`EXISTS (SELECT 1 FROM ${issueApprovals} WHERE ${issueApprovals.approvalId} = ${approvals.id} AND ${issueApprovals.issueId} = ${filters.issueId})`, + ); + } + return conditions; + } + + function readSafeLabel(value: unknown) { + if (typeof value !== "string") return null; + const trimmed = value.trim(); + if (!trimmed || trimmed === REDACTED_EVENT_VALUE) return null; + return trimmed; + } + + function deriveSummaryLabel(row: { + id: string; + type: string; + title: unknown; + summary: unknown; + description: unknown; + }) { + const redacted = redactApprovalPayloadByType(row.type, { + title: row.title, + summary: row.summary, + description: row.description, + }); + return ( + readSafeLabel(redacted.title) ?? + readSafeLabel(redacted.summary) ?? + readSafeLabel(redacted.description) ?? + `${row.type} ${row.id.slice(0, 8)}` + ); + } + async function reconcileApprovedBuiltInAgent( dbClient: Db, companyId: string, @@ -138,12 +203,61 @@ export function approvalService(db: Db) { } return { - list: (companyId: string, status?: string) => { - const conditions = [eq(approvals.companyId, companyId)]; - if (status) conditions.push(eq(approvals.status, status)); + list: (companyId: string, filters?: string | ApprovalListFilters) => { + const conditions = approvalListConditions(companyId, normalizeListFilters(filters)); return db.select().from(approvals).where(and(...conditions)); }, + /** + * Existence-check listing. Selects an explicit column set that excludes `payload` + * — the whole point is that checking for an already-filed ask must cost far less + * than re-filing it. `label` is derived server-side so the caller still gets + * something triageable without shipping the payload body. + * + * `issueId` filters through the issue_approvals join table. + */ + listSummary: async ( + companyId: string, + filters: ApprovalListFilters = {}, + ) => { + const conditions = approvalListConditions(companyId, filters); + + const rows = await db + .select({ + id: approvals.id, + type: approvals.type, + status: approvals.status, + requestedByAgentId: approvals.requestedByAgentId, + requestedByUserId: approvals.requestedByUserId, + idempotencyKey: approvals.idempotencyKey, + createdAt: approvals.createdAt, + decidedAt: approvals.decidedAt, + title: sql`${approvals.payload} ->> 'title'`, + summary: sql`${approvals.payload} ->> 'summary'`, + description: sql`${approvals.payload} ->> 'description'`, + }) + .from(approvals) + .where(and(...conditions)) + .orderBy(asc(approvals.createdAt)); + + return rows.map(({ title, summary, description, ...row }) => ({ + ...row, + label: deriveSummaryLabel({ ...row, title, summary, description }), + })); + }, + + countBy: async ( + companyId: string, + filters: ApprovalListFilters = {}, + ) => { + const conditions = approvalListConditions(companyId, filters); + const rows = await db + .select({ count: sql`count(*)::int` }) + .from(approvals) + .where(and(...conditions)); + return rows[0]?.count ?? 0; + }, + getById: (id: string) => db .select() @@ -173,6 +287,82 @@ export function approvalService(db: Db) { .returning() .then((rows) => rows[0]), + /** + * Create, replaying an existing undecided approval when the same requester reuses + * an idempotency key. Returns `{ approval, deduplicated }` so the route can answer + * 200-with-readback instead of 201, which is the signal a requester currently + * lacks — silence today is indistinguishable from "not yet decided", so retrying + * is the only way to find out, which is exactly what floods the queue. + * + * Race safety comes from the advisory lock, matching the issue-create path + * (server/src/services/issues.ts). The partial unique indexes are the backstop. + */ + createWithIdempotency: async ( + companyId: string, + data: Omit, + options: CreateWithIdempotencyOptions = {}, + ): Promise<{ approval: ApprovalRecord; deduplicated: boolean }> => { + const idempotencyKey = typeof data.idempotencyKey === "string" + ? data.idempotencyKey.trim() || null + : null; + const requestedByAgentId = data.requestedByAgentId ?? null; + const requestedByUserId = data.requestedByUserId ?? null; + + if (requestedByAgentId && requestedByUserId) { + throw unprocessable("Approval requester must be either an agent or a user, not both"); + } + + async function insertNew(client: Db, normalizedKey: string | null) { + const approval = await client + .insert(approvals) + .values({ ...data, companyId, idempotencyKey: normalizedKey }) + .returning() + .then((rows) => rows[0]); + await options.afterCreate?.(client, approval); + return { approval, deduplicated: false }; + } + + if (!idempotencyKey) { + if (options.afterCreate) { + return db.transaction(async (tx) => insertNew(tx as unknown as Db, null)); + } + return insertNew(db, null); + } + + // The requester identity that scopes the key. Exactly one of these is set by the + // route; scoping to the requester means two agents filing similar asks never + // collide, while one agent retrying always does. + const requesterColumn = requestedByAgentId + ? approvals.requestedByAgentId + : approvals.requestedByUserId; + const requesterValue = requestedByAgentId ?? requestedByUserId ?? null; + + if (!requesterValue) { + throw unprocessable("Approval idempotency key requires an authenticated requester"); + } + + return db.transaction(async (tx) => { + const guardKey = `approval-create:idempotency:${companyId}:${requesterValue ?? "anonymous"}:${idempotencyKey}`; + await tx.execute(sql`select pg_advisory_xact_lock(hashtextextended(${guardKey}, 0))`); + + const existing = await tx + .select() + .from(approvals) + .where( + and( + eq(approvals.companyId, companyId), + eq(approvals.idempotencyKey, idempotencyKey), + eq(requesterColumn, requesterValue), + inArray(approvals.status, resolvableStatuses), + ), + ) + .limit(1); + if (existing[0]) return { approval: existing[0], deduplicated: true }; + + return insertNew(tx as unknown as Db, idempotencyKey); + }); + }, + approve: async (id: string, decidedByUserId: string, decisionNote?: string | null) => { const now = new Date(); const result = await db.transaction(async (tx) => {