From b8d4f5ebf8cf6ceef1e5452fa468bc2ed8a7803a Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sat, 19 Sep 2026 04:18:17 +0000 Subject: [PATCH 1/2] feat(approvals): record the pre-decision figure on budget assertions (BLO-34008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 indistinguishable from the false-positive class that filed BLO-33160, BLO-33397, BLO-33416 and BLO-33772. Requiring the caller to supply the prior is not available. The refusal's own remediation says never to invent a starting figure and its example payload deliberately omits one, so callers will keep omitting it — correctly. The server does not have to guess: the assertion already names the `policyId`, so it reads `budget_policies.amount` and stamps `from_amount_cents`. A database read, not a regex over prose, which is the distinction the whole refusal exists to protect. Narrow on purpose: fills only what is absent (a caller-stated prior may be one the row no longer shows), canonical shape only (`exact_changes` already carries `from_usd`), and leaves an unresolvable policy unstamped so it still reports as `missing_policy` rather than acquiring a figure that makes it look serviceable. `from_source` marks a server-read prior so provenance is legible on a money path. 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. Also corrects the route's "Nothing reads it yet" guidance, stale since #1846 made the reconciler read the prior, and now wrong in the other direction too. Tests: 5 cases, each with a verified failing mutation — the stamp, the don't-overwrite guard, the missing-policy guard, and both classifier outcomes end to end (never_applied for the 0-of-8 shape, superseded for a real one). --- packages/shared/src/validators/approval.ts | 11 +- ...approval-budget-assertion-required.test.ts | 156 ++++++++++++++++-- server/src/routes/approvals.ts | 44 ++++- .../approval-enforcement-reconciler.ts | 62 +++++++ 4 files changed, 250 insertions(+), 23 deletions(-) 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..633758a49a07 100644 --- a/server/src/__tests__/approval-budget-assertion-required.test.ts +++ b/server/src/__tests__/approval-budget-assertion-required.test.ts @@ -53,20 +53,29 @@ 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()`. + */ +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 && "amount" 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 +93,7 @@ async function createAgentApp() { }; next(); }); - app.use("/api", approvalRoutes(createRouteDb())); + app.use("/api", approvalRoutes(createRouteDb(policyRows))); app.use(errorHandler); return app; } @@ -518,3 +527,130 @@ 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"); + }); +}); diff --git a/server/src/routes/approvals.ts b/server/src/routes/approvals.ts index dc0721960cb3..ce0754b9e250 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,17 @@ export function approvalRoutes( return; } - const approval = await svc.resubmit(id, normalizedPayload); + // Only a supplied payload is stamped. An empty resubmit keeps the stored one, + // which was already stamped when it was created — and a card old enough to + // predate that is refused above for having no assertion at all. + const resubmitPayload = + normalizedPayload === undefined + ? undefined + : ((await stampPriors(existing.companyId, existing.type, normalizedPayload)) 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..86ab8d2eb966 100644 --- a/server/src/services/approval-enforcement-reconciler.ts +++ b/server/src/services/approval-enforcement-reconciler.ts @@ -312,6 +312,68 @@ 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; + const entries = root.enforcement_assertions ?? 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 = "enforcement_assertions" in root ? "enforcement_assertions" : "enforcementAssertions"; + return { ...root, [key]: stamped }; +} + /** * What the enforcing row says happened to one decided assertion. * From 9e3952bb848e79c3057f95dc3f20c05d16481f9b Mon Sep 17 00:00:00 2001 From: Staff Engineer Date: Sat, 19 Sep 2026 10:41:03 +0000 Subject: [PATCH 2/2] fix(approvals): stamp the stored payload on an empty resubmit (BLO-34008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's review of `b8d4f5e` found the comment justifying the no-payload resubmit branch asserts coverage the branch does not have. Both halves were wrong: a card created before this merges was never stamped at creation, and `budgetAssertionRefusal` refuses only a card with *no parseable assertion* — one carrying a valid `expected_usd` and no prior passes it cleanly. So such a card resubmitted through the board's own affordance (which sends no payload) walked back to `pending` with no prior and classified `unverifiable_mismatch` forever, which is the state this PR exists to eliminate. Stamp whichever payload will end up `pending`, for the same reason the refusal checks that one. `stampAssertionPriors` returns its argument by reference when it changes nothing, so keep-vs-overwrite semantics are untouched: an empty resubmit still sends `undefined` unless there was genuinely a prior to add. Ally's two suggestions, both taken: - The stamp's write-back key was chosen with `in` while the read used `??`, so a present-but-null `enforcement_assertions` beside a real `enforcementAssertions` array stamped the snake_case key and left the camelCase array this actually read in place unstamped — two divergent arrays on a money payload. Derive the key from the branch the read took. - The route stub's `select()` discriminator is `policyId` rather than `amount`: specific to this query, where `amount` is a plausible column for any future select on these routes to project. Also records that the stub ignores `where`, so `loadEnforcedBudgetPolicies`'s company filter is not exercised here. Note for the record: the cohort is forward-looking, not live. Both `budget_override_required` cards in `revision_requested` today (`29015e50`, `170097eb`) are server-filed watcher cards, and `304ea443` — named in the review — is `approved`, so `resubmit`'s status guard puts it out of reach regardless. The defect is real; that particular card is not in it. Tests: 3 cases, each with a verified failing mutation — the stored-payload stamp, the reference check that preserves keep-semantics, and the key divergence. --- ...approval-budget-assertion-required.test.ts | 85 ++++++++++++++++++- server/src/routes/approvals.ts | 26 ++++-- .../approval-enforcement-reconciler.ts | 11 ++- 3 files changed, 109 insertions(+), 13 deletions(-) diff --git a/server/src/__tests__/approval-budget-assertion-required.test.ts b/server/src/__tests__/approval-budget-assertion-required.test.ts index 633758a49a07..5961c903d7a0 100644 --- a/server/src/__tests__/approval-budget-assertion-required.test.ts +++ b/server/src/__tests__/approval-budget-assertion-required.test.ts @@ -57,13 +57,21 @@ function registerModuleMocks() { * 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()`. + * 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((projection?: Record) => { - const rows = projection && "amount" in projection ? policyRows : [runContextRow]; + const rows = projection && "policyId" in projection ? policyRows : [runContextRow]; return { from: vi.fn(() => ({ where: vi.fn(() => ({ @@ -399,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", }); @@ -653,4 +713,21 @@ describe("the starting figure is stamped from the enforcing policy (BLO-34008)", 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 ce0754b9e250..67f5e33b4453 100644 --- a/server/src/routes/approvals.ts +++ b/server/src/routes/approvals.ts @@ -806,15 +806,27 @@ export function approvalRoutes( return; } - // Only a supplied payload is stamped. An empty resubmit keeps the stored one, - // which was already stamped when it was created — and a card old enough to - // predate that is refused above for having no assertion at all. + // 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 + normalizedPayload === undefined && stamped === existing.payload ? undefined - : ((await stampPriors(existing.companyId, existing.type, normalizedPayload)) as - | Record - | undefined); + : (stamped as Record | undefined); const approval = await svc.resubmit(id, resubmitPayload); const actor = getActorInfo(req); diff --git a/server/src/services/approval-enforcement-reconciler.ts b/server/src/services/approval-enforcement-reconciler.ts index 86ab8d2eb966..15ed72f9b96b 100644 --- a/server/src/services/approval-enforcement-reconciler.ts +++ b/server/src/services/approval-enforcement-reconciler.ts @@ -352,7 +352,14 @@ export function stampAssertionPriors( ): unknown { const root = asRecord(payload); if (!root) return payload; - const entries = root.enforcement_assertions ?? root.enforcementAssertions; + // 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; @@ -370,7 +377,7 @@ export function stampAssertionPriors( }); if (!changed) return payload; - const key = "enforcement_assertions" in root ? "enforcement_assertions" : "enforcementAssertions"; + const key = usesSnake ? "enforcement_assertions" : "enforcementAssertions"; return { ...root, [key]: stamped }; }