diff --git a/docs/storage.md b/docs/storage.md index 5d40d17070..799e01a73f 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -511,7 +511,7 @@ The `tasks.cumulativeActiveMs` and `tasks.executionCompletedAt` columns are the | `agents` | Agent registry/state/task assignment metadata. | | `agentHeartbeats` | Heartbeat run events linked to agents (`agentId` FK cascade). | | `approval_requests` | Durable approval request records: requester actor snapshot, target action payload (category/action/resource/context), lifecycle status (`pending`/`approved`/`denied`/`completed`), optional task/run context, and requested/decided/completed timestamps. | -| `approval_request_audit_events` | Append-only audit trail for approval requests. PostgreSQL uses the physical `(project_id, id)` identity, while public `ApprovalRequestStore.getAuditHistory` scopes bound audit-history reads; Command Center analytics remains intentionally unbound-tolerant. The ownership trigger normalizes only NULL/exact `''`, so a whitespace-only binding is stored literally. Rows store event type (`created`/`approved`/`denied`/`completed`), immutable actor snapshot, optional note, and deterministic per-request ordering by `(createdAt, rowid)`. | +| `approval_request_audit_events` | Append-only audit trail for approval requests. PostgreSQL uses the physical `(project_id, id)` identity, while public `ApprovalRequestStore.getAuditHistory` scopes bound audit-history reads; Command Center analytics remains intentionally unbound-tolerant. The ownership trigger normalizes only NULL/exact `''`, so a whitespace-only binding is stored literally. IDs are deterministic `aprevt---` values: request identity and lifecycle guards ensure one same-type event per request/timestamp within a partition, while different event types are distinct; history ties use lifecycle rank before ID. Rows store event type (`created`/`approved`/`denied`/`completed`), immutable actor snapshot, and optional note. | | `secrets` | Encrypted secret KV rows (`key` unique) with raw BLOB `value_ciphertext` + per-row random `nonce` (AES-256-GCM), per-secret `access_policy` CHECK (`auto`/`prompt`/`deny`), env-materialization metadata (`env_exportable`, `env_export_key`), and read-audit fields (`last_read_at`, `last_read_by`). Plaintext is never written to the database. | | `task_documents` | Task-scoped document metadata/content keyed by `(taskId, key)` with current revision pointer. | | `task_document_revisions` | Immutable revision history for task documents (content snapshots by revision). | diff --git a/packages/core/src/__tests__/approval-request-audit-id-race.test.ts b/packages/core/src/__tests__/approval-request-audit-id-race.test.ts new file mode 100644 index 0000000000..98869b42ca --- /dev/null +++ b/packages/core/src/__tests__/approval-request-audit-id-race.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it, vi } from "vitest"; +import { + decideApprovalRequest, + markApprovalRequestCompleted, +} from "../async-stores/async-approval-request-store.js"; +import type { AsyncDataLayer, DbTransaction } from "../postgres/data-layer.js"; +import type { ApprovalRequestStatus } from "../types.js"; + +const REQUESTER = { actorId: "agent-1", actorType: "agent" as const, actorName: "Requester" }; +const DECIDER = { actorId: "user-1", actorType: "user" as const, actorName: "Decider" }; +const NOW = "2026-08-16T23:22:00.000Z"; + +/** + * Build only the Drizzle chain surface exercised by the real transition helpers. + * The stale read followed by an empty guarded-update returning result is the + * production racer's state after another transaction commits between its read + * and conditional update. + */ +function createRacingLayer(status: ApprovalRequestStatus, updateWins: boolean) { + const auditInsert = vi.fn(async () => undefined); + const row = { + id: "apr-race", + status, + requesterActorId: REQUESTER.actorId, + requesterActorType: REQUESTER.actorType, + requesterActorName: REQUESTER.actorName, + targetActionCategory: "other", + targetActionOperation: "test", + targetActionSummary: "test", + targetResourceType: "task", + targetResourceId: "FN-9138", + targetContext: {}, + taskId: null, + runId: null, + requestedAt: "2026-08-16T23:00:00.000Z", + decidedAt: status === "approved" ? "2026-08-16T23:01:00.000Z" : null, + completedAt: null, + createdAt: "2026-08-16T23:00:00.000Z", + updatedAt: "2026-08-16T23:00:00.000Z", + }; + const tx = { + select: () => ({ from: () => ({ where: () => Promise.resolve([row]) }) }), + update: () => ({ + set: () => ({ + where: () => ({ returning: () => Promise.resolve(updateWins ? [{ id: row.id }] : []) }), + }), + }), + insert: () => ({ values: auditInsert }), + } as unknown as DbTransaction; + const layer = { + projectId: "fn-9138-project", + transactionImmediate: async (fn: (transaction: DbTransaction) => Promise) => fn(tx), + } as AsyncDataLayer; + return { layer, auditInsert }; +} + +/* +FNXC:ApprovalAuditIdentity 2026-08-16-23:22: +A same-millisecond same-type audit ID is safe only while the lifecycle writes at +most one successful transition of that type. This database-free seam simulates a +stale non-terminal racer: its guarded update returns no rows, so real helpers +throw the dashboard-mapped transition conflict before reaching the append-only +insert. Keep a successful control beside each losing case so this test fails if +its stub no longer exercises the production audit path. +*/ +describe("approval audit deterministic-ID guarded-update race", () => { + it.each([ + ["approved", "pending", "approved"] as const, + ["denied", "pending", "denied"] as const, + ["completed", "approved", "completed"] as const, + ])("rejects a stale %s racer before it mints a second audit row", async (eventType, staleStatus, targetStatus) => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date(NOW)); + try { + const { layer, auditInsert } = createRacingLayer(staleStatus, false); + + const operation = eventType === "completed" + ? markApprovalRequestCompleted(layer, "apr-race", { actor: DECIDER }) + : decideApprovalRequest(layer, "apr-race", targetStatus as "approved" | "denied", { actor: DECIDER }); + + await expect(operation).rejects.toThrow(`Invalid approval request transition: ${staleStatus} -> ${targetStatus}`); + expect(auditInsert).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it.each([ + ["approved", "pending", "approved"] as const, + ["denied", "pending", "denied"] as const, + ["completed", "approved", "completed"] as const, + ])("inserts one deterministic %s audit ID after a successful guarded update", async (eventType, staleStatus, targetStatus) => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date(NOW)); + try { + const { layer, auditInsert } = createRacingLayer(staleStatus, true); + if (eventType === "completed") { + await markApprovalRequestCompleted(layer, "apr-race", { actor: DECIDER }); + } else { + await decideApprovalRequest(layer, "apr-race", targetStatus as "approved" | "denied", { actor: DECIDER }); + } + + expect(auditInsert).toHaveBeenCalledTimes(1); + expect(auditInsert.mock.calls[0]?.[0]).toMatchObject({ + projectId: "fn-9138-project", + id: `aprevt-${eventType}-apr-race-${NOW}`, + requestId: "apr-race", + eventType, + createdAt: NOW, + }); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/packages/core/src/__tests__/postgres/approval-request-lifecycle.pg.test.ts b/packages/core/src/__tests__/postgres/approval-request-lifecycle.pg.test.ts index 71d9f65cf5..f7be2b1070 100644 --- a/packages/core/src/__tests__/postgres/approval-request-lifecycle.pg.test.ts +++ b/packages/core/src/__tests__/postgres/approval-request-lifecycle.pg.test.ts @@ -1,7 +1,9 @@ -import { it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; import { ApprovalRequestStore } from "../../agents/approval-request-store.js"; import type { AsyncDataLayer } from "../../postgres/data-layer.js"; +import * as schema from "../../postgres/schema/index.js"; import { + PG_AVAILABLE, pgDescribe, createSharedPgTaskStoreTestHarness, type SharedPgTaskStoreHarness, @@ -34,11 +36,19 @@ WHAT THESE DO AND DO NOT COVER, measured by reverting each guard in turn rather - requester-ownership on redemption -> 1 of 6 fails when removed - the `AND status = ?` guard on the UPDATE -> 0 fail when removed -That last line is the honest limit. The in-transaction re-read already rejects a replay single-threaded, -so the guard only earns its keep against a racer committing BETWEEN the read and the write — which needs -two concurrent transactions these tests do not create. The guard stays because the race is real; it is -simply not what is verified here. Do not read a green run as proof of it. +The guarded `AND status = ?` update is now pinned by the always-running +`approval-request-audit-id-race.test.ts` stale-read simulation and the barrier-overlapped +PostgreSQL double-decision/double-completion probes below. The pure test proves the exact +empty-returning branch; PostgreSQL may validly serialize its loser to the transition matrix. */ +describe("approval request lifecycle PostgreSQL availability", () => { + it("fails closed when a required PostgreSQL probe would otherwise be skipped", () => { + if (process.env.FUSION_PG_REQUIRED === "1") { + expect(PG_AVAILABLE).toBe(true); + } + }); +}); + pgDescribe("approval request lifecycle security (PostgreSQL)", () => { const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ prefix: "fusion_sat_test", @@ -184,4 +194,116 @@ pgDescribe("approval request lifecycle security (PostgreSQL)", () => { }); expect(completed.status).toBe("completed"); }); + + it("rejects an exact same-millisecond audit primary-key duplicate", async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + try { + const tiedAt = new Date("2026-08-16T23:22:00.000Z"); + vi.setSystemTime(tiedAt); + const store = await seed("apr-audit-primary-key"); + const [created] = await store.getApprovalAuditHistory(ctx.layer.db, "apr-audit-primary-key"); + expect(created).toBeDefined(); + const duplicateError = await h.adminDb().insert(schema.project.approvalRequestAuditEvents).values({ + projectId: "__legacy_unscoped__", + id: created!.id, + requestId: created!.requestId, + eventType: created!.eventType, + actorId: created!.actor.actorId, + actorType: created!.actor.actorType, + actorName: created!.actor.actorName, + note: created!.note ?? null, + createdAt: created!.createdAt, + }).then(() => null, (error: unknown) => error as { cause?: unknown }); + expect(duplicateError).toMatchObject({ cause: { code: "23505" } }); + } finally { + vi.useRealTimers(); + } + }); + + it.each([ + ["pending double approve", "approved", "approved"] as const, + ["pending double deny", "denied", "denied"] as const, + ["pending approve versus deny", "approved", "denied"] as const, + ])("runs overlapping %s decisions without duplicate audit IDs", async (_name, first, second) => { + const store = await seed(`apr-race-${first}-${second}`); + const requestId = `apr-race-${first}-${second}`; + vi.useFakeTimers({ toFake: ["Date"] }); + try { + vi.setSystemTime(new Date("2026-08-16T23:22:00.000Z")); + let release!: () => void; + const barrier = new Promise((resolve) => { release = resolve; }); + const windows: Array<{ started: number; settled: number }> = []; + const racer = async (status: "approved" | "denied") => { + await barrier; + const window = { started: performance.now(), settled: Number.NaN }; + windows.push(window); + try { + return await store.decideApprovalRequest(ctx.layer, requestId, status, { actor: DECIDER }); + } finally { + window.settled = performance.now(); + } + }; + const racers = [racer(first), racer(second)]; + await Promise.resolve(); + release(); + const outcomes = await Promise.allSettled(racers); + expect(windows).toHaveLength(2); + expect(Math.max(...windows.map((window) => window.started))).toBeLessThan( + Math.min(...windows.map((window) => window.settled)), + ); + expect(outcomes.filter((outcome) => outcome.status === "fulfilled")).toHaveLength(1); + expect(outcomes.filter((outcome) => outcome.status === "rejected")).toHaveLength(1); + const loser = outcomes.find((outcome) => outcome.status === "rejected") as PromiseRejectedResult; + expect(String(loser.reason)).toMatch(/Invalid approval request transition/); + expect(String(loser.reason)).not.toMatch(/duplicate key|unique constraint/i); + const winner = outcomes.find((outcome) => outcome.status === "fulfilled") as PromiseFulfilledResult<{ status: string }>; + expect((await store.getApprovalRequest(ctx.layer.db, requestId))?.status).toBe(winner.value.status); + const history = await store.getApprovalAuditHistory(ctx.layer.db, requestId); + expect(history.filter((event) => event.eventType === winner.value.status)).toHaveLength(1); + expect(new Set(history.map((event) => `${event.eventType}:${event.createdAt}`)).size).toBe(history.length); + } finally { + vi.useRealTimers(); + } + }); + + it("runs overlapping approved double completion without duplicate audit IDs", async () => { + const store = await seed("apr-race-completed"); + await store.decideApprovalRequest(ctx.layer, "apr-race-completed", "approved", { actor: DECIDER }); + vi.useFakeTimers({ toFake: ["Date"] }); + try { + vi.setSystemTime(new Date("2026-08-16T23:22:00.000Z")); + let release!: () => void; + const barrier = new Promise((resolve) => { release = resolve; }); + const windows: Array<{ started: number; settled: number }> = []; + const racer = async () => { + await barrier; + const window = { started: performance.now(), settled: Number.NaN }; + windows.push(window); + try { + return await store.markApprovalRequestCompleted(ctx.layer, "apr-race-completed", { actor: DECIDER }); + } finally { + window.settled = performance.now(); + } + }; + const outcomes = [racer(), racer()]; + await Promise.resolve(); + release(); + const settled = await Promise.allSettled(outcomes); + expect(Math.max(...windows.map((window) => window.started))).toBeLessThan( + Math.min(...windows.map((window) => window.settled)), + ); + expect(settled.filter((outcome) => outcome.status === "fulfilled")).toHaveLength(1); + expect(settled.filter((outcome) => outcome.status === "rejected")).toHaveLength(1); + const loser = settled.find((outcome) => outcome.status === "rejected") as PromiseRejectedResult; + expect(String(loser.reason)).toMatch(/Invalid approval request transition/); + expect(String(loser.reason)).not.toMatch(/duplicate key|unique constraint/i); + expect((await store.getApprovalRequest(ctx.layer.db, "apr-race-completed"))?.status).toBe("completed"); + const history = await store.getApprovalAuditHistory(ctx.layer.db, "apr-race-completed"); + expect(history.filter((event) => event.eventType === "completed")).toHaveLength(1); + expect(new Set(history.map((event) => `${event.eventType}:${event.createdAt}`)).size).toBe(history.length); + } finally { + vi.useRealTimers(); + } + }); + }); diff --git a/packages/core/src/async-stores/async-approval-request-store.ts b/packages/core/src/async-stores/async-approval-request-store.ts index 725a809f0b..3dde1c1da8 100644 --- a/packages/core/src/async-stores/async-approval-request-store.ts +++ b/packages/core/src/async-stores/async-approval-request-store.ts @@ -122,6 +122,16 @@ function rowToAuditEvent(row: ApprovalRequestAuditEventRow): ApprovalRequestAudi * * FNXC:ApprovalAnalyticsIsolation 2026-07-14-01:04: * Audit events must carry the bound layer's project ID at write time because request IDs alone do not provide a reliable tenant ownership join for Command Center intervention analytics. The live `(project_id, id)` key also permits deterministic audit IDs to collide safely across partitions; preserve the explicit value so the ownership trigger observes blank writes unchanged. + * + * FNXC:ApprovalAuditIdentity 2026-08-16-23:50: + * A deterministic ID collision needs one physical project partition, request ID, + * event type, and millisecond. `created` cannot repeat because the request-row + * primary key rejects ID reuse first; the transition matrix rejects same-state + * and terminal replays; and a concurrent non-terminal loser gets an empty + * status-guarded update before this append runs. The database-free + * `approval-request-audit-id-race.test.ts` pins that last branch, while the + * fail-closed PostgreSQL lifecycle overlap probes confirm it end to end. Keep + * this format unchanged unless a reachable writer defeats all three barriers. */ async function appendAuditEvent( tx: DbTransaction,