diff --git a/packages/shared/src/validators/approval.ts b/packages/shared/src/validators/approval.ts index 10946e37a3f8..993c9fcca0e2 100644 --- a/packages/shared/src/validators/approval.ts +++ b/packages/shared/src/validators/approval.ts @@ -86,11 +86,12 @@ const approvalPayloadSchema = z.object({ "`budget_override_required`: such a card carrying no parseable assertion is refused with " + "`budget_approval_missing_enforcement_assertion` (BLO-34008), whose `details.remediation` " + "gives the full shape — approving one writes nothing to `budget_policies`, so an undeclared " + - "figure is unverifiable forever. Optionally add the figure the change starts from as " + - "`from_usd` / `from_amount_cents` when you already know it: the approval-enforcement " + - "reconciler reads it as the recorded prior, which is what lets it tell a decision that was " + - "never applied from one that was later superseded; without it a mismatch can only be " + - "reported as `unverifiable_mismatch`. Never invent one. For every other type it stays " + + "figure is unverifiable forever. You do not need to supply the figure the change starts " + + "from: the server reads it off the policy the assertion names and records it as " + + "`from_amount_cents`, which is what lets the approval-enforcement reconciler tell a decision " + + "that was never applied from one that was later superseded. Send `from_usd` / " + + "`from_amount_cents` yourself only if you know a prior the policy row no longer shows — " + + "never invent one. For every other type it stays " + "optional; today only `budget_policy_amount` is " + "checked, and unknown kinds are ignored.", ); diff --git a/server/src/__tests__/approval-budget-assertion-required.test.ts b/server/src/__tests__/approval-budget-assertion-required.test.ts index 1c2f5f1005a0..5961c903d7a0 100644 --- a/server/src/__tests__/approval-budget-assertion-required.test.ts +++ b/server/src/__tests__/approval-budget-assertion-required.test.ts @@ -53,20 +53,37 @@ function registerModuleMocks() { })); } -function createRouteDb() { +/** + * Two readers reach `select()` on these routes: the run-context lookup, and + * `loadEnforcedBudgetPolicies` (BLO-34008). They are told apart by the projection, + * because that is the only thing this stub sees — distinguishing on the table + * would mean reimplementing enough of drizzle to read `.from()`. `policyId` is + * the discriminator rather than `amount`: `amount` is a plausible column for any + * future select on these routes to project, and a misroute would surface as a + * confusing run-context failure instead of a clear one. + * + * The `where` clause is ignored, so the company/policy filter inside + * `loadEnforcedBudgetPolicies` is not exercised here — these cases are about the + * routes' stamping behaviour, and that filter is covered where the function is + * tested directly. + */ +function createRouteDb(policyRows: Array> = []) { + const runContextRow = { id: "run-1", companyId: "company-1", agentId: "agent-1", contextSnapshot: {} }; return { - select: vi.fn(() => ({ - from: vi.fn(() => ({ - where: vi.fn(() => ({ - then: async (resolve: (rows: unknown[]) => unknown) => - resolve([{ id: "run-1", companyId: "company-1", agentId: "agent-1", contextSnapshot: {} }]), + select: vi.fn((projection?: Record) => { + const rows = projection && "policyId" in projection ? policyRows : [runContextRow]; + return { + from: vi.fn(() => ({ + where: vi.fn(() => ({ + then: async (resolve: (rows: unknown[]) => unknown) => resolve(rows), + })), })), - })), - })), + }; + }), } as any; } -async function createAgentApp() { +async function createAgentApp(policyRows: Array> = []) { const [{ errorHandler }, { approvalRoutes }] = await Promise.all([ import("../middleware/index.js"), import("../routes/approvals.js"), @@ -84,7 +101,7 @@ async function createAgentApp() { }; next(); }); - app.use("/api", approvalRoutes(createRouteDb())); + app.use("/api", approvalRoutes(createRouteDb(policyRows))); app.use(errorHandler); return app; } @@ -390,8 +407,60 @@ describe("budget_override_required requires an enforcement assertion (BLO-34008) expect(mockApprovalService.resubmit).toHaveBeenCalledTimes(1); }); - it("leaves resubmit of every other approval type alone", async () => { - mockApprovalService.getById.mockResolvedValue({ + // The cohort the creation stamp cannot reach: a card filed before it existed + // carries an assertion with no prior. It passes the refusal above — that asks + // for *an* assertion, not a prior — so stamping only the supplied payload let + // it return to `pending` classifying as `unverifiable_mismatch` forever, which + // is the state BLO-34008 exists to eliminate. Ally's review of `b8d4f5e`. + const RESUBMIT_POLICY_AT_19K = { + policyId: POLICY_ID, + amount: 1900000, + isActive: true, + amountUpdatedAt: new Date("2026-08-01T00:00:00.000Z"), + }; + + it("stamps the stored payload on an empty resubmit when the card predates the creation stamp", async () => { + const priorless = { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000, label: "CTO" }; + mockApprovalService.getById.mockResolvedValue( + revisionRequestedBudgetCard({ title: "Raise the CTO cap", enforcement_assertions: [priorless] }), + ); + mockApprovalService.resubmit.mockImplementation(async (id: string, payload?: Record) => ({ + ...revisionRequestedBudgetCard(payload ?? {}), + id, + status: "pending", + })); + + const res = await resubmit(await createAgentApp([RESUBMIT_POLICY_AT_19K]), {}); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + const [, written] = mockApprovalService.resubmit.mock.calls.at(-1) ?? []; + expect((written as any)?.enforcement_assertions?.[0]).toMatchObject({ + from_amount_cents: 1900000, + from_source: "server_policy_read", + }); + }); + + it("still keeps the stored payload on an empty resubmit when there is no prior to add", async () => { + // The other half of the same branch: stamping must not turn every empty + // resubmit into an explicit payload write. `stampAssertionPriors` returns its + // argument by reference when it changes nothing, and that is what the route + // tests to decide whether to send one at all. + mockApprovalService.getById.mockResolvedValue( + revisionRequestedBudgetCard({ title: "Raise the CTO cap", enforcement_assertions: [VALID_ASSERTION] }), + ); + mockApprovalService.resubmit.mockImplementation(async (id: string, payload?: Record) => ({ + ...revisionRequestedBudgetCard(payload ?? {}), + id, + status: "pending", + })); + + const res = await resubmit(await createAgentApp([RESUBMIT_POLICY_AT_19K]), {}); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockApprovalService.resubmit).toHaveBeenCalledWith("approval-budget-1", undefined); + }); + + it("leaves resubmit of every other approval type alone", async () => { mockApprovalService.getById.mockResolvedValue({ ...revisionRequestedBudgetCard({ title: "Approve hosting spend" }), type: "request_board_approval", }); @@ -518,3 +587,147 @@ describe("the budget watcher's own threshold cards are exempt by construction (B expect(extractEnforcementAssertions(watcherPayload)).toEqual([]); }); }); + +/** + * BLO-34008 — the server records the figure the change starts from. + * + * `classifyEnforcementAssertion` needs three numbers (prior, decided, enforced) + * to tell a decision that never landed from one a later decision superseded. + * With only two it answers `unverifiable_mismatch` for every disagreement: + * still reported as drift, but not auto-appliable, and it cannot distinguish the + * real gap from the false-positive class that filed BLO-33160, BLO-33397, + * BLO-33416 and BLO-33772. + * + * Requiring the caller to supply it is not available: the refusal's own + * remediation says never to invent a starting figure, and its example payload + * deliberately omits one. So the server reads it from the policy the assertion + * already names. That is a database read, not a guess — the distinction the + * whole refusal exists to protect. + */ +describe("the starting figure is stamped from the enforcing policy (BLO-34008)", () => { + const POLICY_AT_19K = { policyId: POLICY_ID, amount: 1900000, isActive: true, amountUpdatedAt: new Date("2026-08-01T00:00:00.000Z") }; + + beforeEach(() => { + vi.resetModules(); + vi.doUnmock("../services/index.js"); + registerModuleMocks(); + vi.clearAllMocks(); + + mockApprovalService.createWithIdempotency.mockImplementation( + async (companyId: string, data: Record) => ({ + approval: { id: "approval-1", companyId, status: "pending", ...data }, + deduplicated: false, + }), + ); + mockLogActivity.mockResolvedValue(mockDeferredActivityPublish); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + action: "company_scope:read", + reason: "allow_test", + explanation: "Allowed by test mock.", + }); + mockIssueApprovalService.listIssuesForApproval.mockResolvedValue([]); + }); + + /** The `enforcement_assertions` array as it was actually persisted. */ + function persistedAssertions() { + const [, data] = mockApprovalService.createWithIdempotency.mock.calls.at(-1) ?? []; + return (data as any)?.payload?.enforcement_assertions ?? []; + } + + function fileCard(app: express.Express, assertion: Record) { + return postApproval(app, { + type: "budget_override_required", + payload: { title: "Raise the CTO cap", enforcement_assertions: [assertion] }, + }); + } + + it("fills the prior from the policy when the caller states none", async () => { + const app = await createAgentApp([POLICY_AT_19K]); + + const res = await fileCard(app, { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000, label: "CTO" }); + + expect([200, 201], JSON.stringify(res.body)).toContain(res.status); + expect(persistedAssertions()[0]).toMatchObject({ + from_amount_cents: 1900000, + from_source: "server_policy_read", + }); + }); + + it("does not overwrite a prior the caller stated", async () => { + // The caller may know a pre-decision figure the current row no longer shows + // (a card filed after a manual edit). Silently replacing it would also hide + // a wrong one, which `policyAmountChangedAfterDecision` is there to catch. + const app = await createAgentApp([{ ...POLICY_AT_19K, amount: 2500000 }]); + + await fileCard(app, { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000, from_usd: 19000 }); + + expect(persistedAssertions()[0]).toMatchObject({ from_usd: 19000 }); + expect(persistedAssertions()[0]).not.toHaveProperty("from_source"); + }); + + it("leaves an unresolvable policy unstamped so it still reports as missing_policy", async () => { + // Stamping nothing is the point: a card naming a policy that does not exist + // in this company must keep reading as broken, not acquire a figure that + // makes it look serviceable. + const app = await createAgentApp([]); + + await fileCard(app, { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000 }); + + expect(persistedAssertions()[0]).not.toHaveProperty("from_amount_cents"); + expect(persistedAssertions()[0]).not.toHaveProperty("from_source"); + }); + + it("turns the 0-of-8 shape from unverifiable_mismatch into never_applied", async () => { + // The whole reason the stamp exists, asserted end to end rather than on the + // field: file the card, take the payload that was actually persisted, and + // classify it against a policy still sitting at the pre-approval amount. + // That is card 304ea443's shape — approved, never applied. + const app = await createAgentApp([POLICY_AT_19K]); + await fileCard(app, { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000, label: "CTO" }); + + const { extractEnforcementAssertions, classifyEnforcementAssertion } = await import( + "../services/approval-enforcement-reconciler.js" + ); + const [, data] = mockApprovalService.createWithIdempotency.mock.calls.at(-1) ?? []; + const [assertion] = extractEnforcementAssertions((data as any).payload); + const decidedAt = new Date("2026-08-04T10:04:45.877Z"); + + expect(assertion?.priorAmountCents).toBe(1900000); + expect(classifyEnforcementAssertion(assertion!, POLICY_AT_19K, decidedAt)).toBe("never_applied"); + }); + + it("still classifies a genuine supersession as superseded, not drift", async () => { + // The other half: the stamp must not convert the legitimate case into a + // false positive. Enforced figure is neither prior nor decided, and the + // amount moved after the decision. + const app = await createAgentApp([POLICY_AT_19K]); + await fileCard(app, { kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000 }); + + const { extractEnforcementAssertions, classifyEnforcementAssertion } = await import( + "../services/approval-enforcement-reconciler.js" + ); + const [, data] = mockApprovalService.createWithIdempotency.mock.calls.at(-1) ?? []; + const [assertion] = extractEnforcementAssertions((data as any).payload); + + const movedLater = { ...POLICY_AT_19K, amount: 2500000, amountUpdatedAt: new Date("2026-08-20T00:00:00.000Z") }; + expect(classifyEnforcementAssertion(assertion!, movedLater, new Date("2026-08-04T10:04:45.877Z"))).toBe("superseded"); + }); + + it("writes the stamp back to the key it read, not the key that merely exists", async () => { + // `??` falls through a present-but-null `enforcement_assertions` to the + // camelCase array. Choosing the write key with `in` would then stamp the + // snake_case key and leave the camelCase array this actually read in place + // unstamped — two divergent assertion arrays on one money payload. + const { stampAssertionPriors } = await import("../services/approval-enforcement-reconciler.js"); + const payload = { + enforcement_assertions: null, + enforcementAssertions: [{ kind: "budget_policy_amount", policyId: POLICY_ID, expected_usd: 32000 }], + }; + + const out = stampAssertionPriors(payload, new Map([[POLICY_ID, POLICY_AT_19K]])) as any; + + expect(out.enforcementAssertions[0]).toMatchObject({ from_amount_cents: 1900000 }); + expect(out.enforcement_assertions).toBeNull(); + }); +}); diff --git a/server/src/routes/approvals.ts b/server/src/routes/approvals.ts index dc0721960cb3..67f5e33b4453 100644 --- a/server/src/routes/approvals.ts +++ b/server/src/routes/approvals.ts @@ -24,6 +24,8 @@ import { redactApprovalPayloadForDisplay, withholdAgentConfigFromApprovalPayload import { BUDGET_POLICY_AMOUNT_ASSERTION, extractEnforcementAssertions, + loadEnforcedBudgetPolicies, + stampAssertionPriors, } from "../services/approval-enforcement-reconciler.js"; import type { PluginWorkerManager } from "../services/plugin-worker-manager.js"; import { resolveApprovalWithSideEffects } from "../services/approval-resolution.js"; @@ -154,10 +156,11 @@ function budgetAssertionRefusal(type: string, payload: unknown) { "policy that enforces the cap. Give the target as `expected_usd` (dollars) or " + "`expected_amount_cents` (integer cents). `label` is printed into the drift report this " + "assertion raises, so set it to the agent or scope this policy actually caps — a label " + - "left over from the example misattributes the drift. If you have the figure the change " + - "starts from, record it as `from_usd` / `from_amount_cents`: it is retained on the card " + - "so a later reader can tell 'never applied' from 'applied and then superseded'. Nothing " + - "reads it yet, so never invent one — only the target is required. On resubmit, send the " + + "left over from the example misattributes the drift. You do not need the figure the change " + + "starts from: the server reads it off the policy you named and records it as " + + "`from_amount_cents`, which is what lets a later reader tell 'never applied' from " + + "'applied and then superseded'. State `from_usd` / `from_amount_cents` yourself only if " + + "you know a prior the policy row no longer shows — never invent one. On resubmit, send the " + "corrected assertions in the resubmit body: the check runs against the payload that will " + "end up pending, and a card filed before this guard existed has none stored.", }, @@ -212,6 +215,20 @@ export function approvalRoutes( }); const strictSecretsMode = process.env.PAPERCLIP_SECRETS_STRICT_MODE === "true"; + // Stamp the pre-decision figure onto any canonical assertion that omits one, + // reading it from the policy the assertion names (BLO-34008). Runs on both + // routes that can leave a card `pending`, for the same reason the refusal + // does: what matters is the payload that ends up decided. + // + // Called only after `budgetAssertionRefusal` has passed, so a payload with no + // usable assertion never reaches the lookup. + async function stampPriors(companyId: string, type: string, payload: unknown) { + if (type !== "budget_override_required") return payload; + const policyIds = extractEnforcementAssertions(payload).map((a) => a.policyId); + if (policyIds.length === 0) return payload; + return stampAssertionPriors(payload, await loadEnforcedBudgetPolicies(db, companyId, policyIds)); + } + async function requireApprovalAccess(req: Request, id: string) { const approval = await svc.getById(id); if (!approval || !hasCompanyAccess(req, approval.companyId)) { @@ -529,13 +546,14 @@ export function approvalRoutes( res.status(422).json(budgetRefusal); return; } + const persistedPayload = await stampPriors(companyId, approvalInput.type, normalizedPayload); const actor = getActorInfo(req); 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) + typeof persistedPayload === "object" && persistedPayload !== null + ? (persistedPayload as Record) : {}; const approvalTitle = typeof payloadObj.title === "string" ? payloadObj.title : undefined; @@ -554,7 +572,7 @@ export function approvalRoutes( const coalesceKey = runContextDecision.boardEscalationCoalesceKey; const { approval, deduplicated } = await svc.createWithIdempotency(companyId, { ...approvalInput, - payload: normalizedPayload, + payload: persistedPayload, // 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 @@ -788,7 +806,29 @@ export function approvalRoutes( return; } - const approval = await svc.resubmit(id, normalizedPayload); + // Stamp whichever payload will actually end up `pending`, for the same reason + // the refusal checks that one. Stamping only a *supplied* payload left the + // motivating cohort uncovered: a card filed before the creation stamp existed + // carries an assertion with no prior, which passes the refusal above (that + // only requires *an* assertion, not a prior) and walks back to `pending` + // classifying as `unverifiable_mismatch` — the exact state this exists to + // eliminate. Found by Ally reviewing `b8d4f5e`. + // + // `stampAssertionPriors` returns its argument by reference when it changes + // nothing, so keep-vs-overwrite semantics are untouched: an empty resubmit + // still sends `undefined` and lets `svc.resubmit()` keep the stored payload + // unless there was genuinely a prior to add. + const stamped = await stampPriors( + existing.companyId, + existing.type, + normalizedPayload ?? existing.payload, + ); + const resubmitPayload = + normalizedPayload === undefined && stamped === existing.payload + ? undefined + : (stamped as Record | undefined); + + const approval = await svc.resubmit(id, resubmitPayload); const actor = getActorInfo(req); await logActivity(db, { companyId: approval.companyId, diff --git a/server/src/services/approval-enforcement-reconciler.ts b/server/src/services/approval-enforcement-reconciler.ts index 63cd5c50c04f..15ed72f9b96b 100644 --- a/server/src/services/approval-enforcement-reconciler.ts +++ b/server/src/services/approval-enforcement-reconciler.ts @@ -312,6 +312,75 @@ export function extractEnforcementAssertions(payload: unknown): EnforcementAsser return [...byPolicyId.values()]; } +/** Marks a prior the server read from `budget_policies`, not one the caller stated. */ +export const SERVER_POLICY_READ_PRIOR = "server_policy_read"; + +/** + * Fill in the starting figure on canonical assertions that omit one, reading it + * from the enforcing policy row (BLO-34008). + * + * Without a prior, `classifyEnforcementAssertion` cannot run its three-way split + * and answers `unverifiable_mismatch` for every disagreement — so a never-applied + * decision and a legitimately superseded one look identical. Both get reported, + * which is the safe direction, but the second is the false-positive class that + * filed BLO-33160, BLO-33397, BLO-33416 and BLO-33772, and the first is not + * auto-appliable. Recording the prior is what separates them. + * + * The obvious alternative — require the caller to state `from_usd` — is the one + * thing the refusal's own remediation forbids ("never invent one"), and the + * example payload deliberately ships without it. So callers will keep omitting + * it, correctly. The server does not have to guess: it holds the authoritative + * number already, because the assertion names the `policyId`. + * + * Deliberately narrow: + * - **Only fills what is absent.** A caller who stated a prior has stated + * something we should not silently overwrite; `readPriorAmountCents` already + * treats a wrong one as suspect and splits on `amountUpdatedAt` instead. + * - **Canonical shape only.** `exact_changes` is the legacy shape on card + * `6f45844e`, which already carries `from_usd`. Rewriting a shape we are not + * encouraging buys nothing. + * - **Unresolvable policy is left alone.** A missing or cross-company id stays + * unstamped so the reconciler still reports it as `missing_policy`, rather + * than the stamp quietly making a bad id look serviceable. + * + * `from_source` records that the figure is a database read, so a later reader can + * tell an authoritative prior from an agent-authored one on a path that writes money. + */ +export function stampAssertionPriors( + payload: unknown, + policies: ReadonlyMap, +): unknown { + const root = asRecord(payload); + if (!root) return payload; + // Read and write the same key. Deriving the write key with `in` diverges from + // this read: `??` falls through a present-but-`null` `enforcement_assertions` + // to the camelCase array, but `in` would then pick the snake_case key — so the + // stamped array lands under snake_case while the camelCase one this actually + // read stays in place unstamped. Two arrays, one payload, on a money path. + const snake = root.enforcement_assertions; + const usesSnake = snake !== undefined && snake !== null; + const entries = usesSnake ? snake : root.enforcementAssertions; + if (!Array.isArray(entries)) return payload; + + let changed = false; + const stamped = entries.map((raw) => { + const entry = asRecord(raw); + if (!entry) return raw; + if (asNonEmptyString(entry.kind) !== BUDGET_POLICY_AMOUNT_ASSERTION) return raw; + if (readPriorAmountCents(entry) !== null) return raw; + const policyId = asNonEmptyString(entry.policyId ?? entry.policy_id); + if (!policyId) return raw; + const amount = policies.get(policyId)?.amount; + if (typeof amount !== "number") return raw; + changed = true; + return { ...entry, from_amount_cents: amount, from_source: SERVER_POLICY_READ_PRIOR }; + }); + + if (!changed) return payload; + const key = usesSnake ? "enforcement_assertions" : "enforcementAssertions"; + return { ...root, [key]: stamped }; +} + /** * What the enforcing row says happened to one decided assertion. *