From 3e1ea29d02ded29a74e1288ea121c92a8a97ce6e Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:33:49 +0200 Subject: [PATCH] fix(pending-ops): record posted ids and land failed_partial instead of clean rejected after partial commits (#842) (#1110) Multi-step executors (match_transaction_invoice, credit_invoice) post an irreversible voucher or persist a credit note and then run later fallible steps. A failure there previously marked the whole op status=rejected, hiding the posted entity and its id from operators. - new migration 20260722134114: add failed_partial to the pending_operations status CHECK and treat it as terminal in both immutability triggers (immutable, undeletable, never re-claimable) - PartialCommitError + ExecutorResult.partialPostedIds carry the posted ids; the dispatcher writes status=failed_partial with result_data.posted_ids and returns code=partial_commit - instrument only the two named executors; hoist the read-only settlement-account resolution above the storno in the match executor - consumer sweep: status union + query schema widened, failed_partial folds into the Avvisade tab with a badge and posted-ids detail line, bulk/reject routes and MCP tools message it explicitly, worklist and expiry sweep intentionally untouched (not pending work) - tests: pg-real coverage for the new terminal semantics, dispatcher unit tests for both partial paths plus byte-for-byte regression guards Fixes #842 Co-authored-by: Claude Fable 5 --- DECISIONS.md | 1 + app/(dashboard)/pending/page.tsx | 26 ++ .../pending-operations/[id]/reject/route.ts | 10 +- .../pending-operations/bulk-commit/route.ts | 1 + .../pending-operations/bulk-reject/route.ts | 1 + app/api/pending-operations/route.ts | 14 +- extensions/general/mcp-server/server.ts | 10 +- lib/api/schemas.ts | 4 +- .../__tests__/failed-partial.test.ts | 343 ++++++++++++++++++ lib/pending-operations/commit.ts | 134 ++++++- lib/pending-operations/errors.ts | 35 ++ messages/en.json | 4 +- messages/sv.json | 4 +- ...nding_operations_failed_partial_status.sql | 86 +++++ ...nding-operations-failed-partial.pg.test.ts | 139 +++++++ types/index.ts | 6 +- 16 files changed, 795 insertions(+), 23 deletions(-) create mode 100644 lib/pending-operations/__tests__/failed-partial.test.ts create mode 100644 lib/pending-operations/errors.ts create mode 100644 supabase/migrations/20260722134114_pending_operations_failed_partial_status.sql create mode 100644 tests/pg/pending-operations-failed-partial.pg.test.ts diff --git a/DECISIONS.md b/DECISIONS.md index f3b30e93..00a71066 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -275,3 +275,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-22] LEGACY_DISCOVERY_HOSTS drift guard (#1093) is an exported validateLegacyDiscoveryHosts() returning a violations list, exercised only by a unit test that pins the registered prod config (app.accounted.se canonical + app.gnubok.se SKV pin), not a startup assertion: CI does not set the prod env vars, so a runtime assertion would either no-op in CI or crash self-hosted deploys with different domains; the test-pinned constants make any allowlist or pin change a deliberate, reviewed edit. [2026-07-22] Stuck-committing recovery sweep (#843) rejects rows without positive evidence instead of reverting to pending, and only three op types (categorize_transaction, link_transaction_journal_entry, match_transaction_invoice) can recover to committed: no generic side-effect -> pending_op linkage exists yet (that is #842's posted-ids work), so evidence is limited to types whose params identify a target row with an unambiguous posted state; reverting to pending risks re-executing side-effects that posted without a trace (duplicate entries/emails). [2026-07-22] MCP briefing recommended_tools (#1098) ships as a STATIC per-workflow loadout list, not state-gated: the briefing does not query workflow state (unbooked counts, open periods) today, so gating would add reads to the session-bootstrap hot path for marginal honesty; drift protection is a module-init assert against the tool registry + workflow-skill slugs, pinned by tests. +[2026-07-22] failed_partial (#842) is a TERMINAL, immutable pending_operations status, never released back to pending: the executor already posted an irreversible voucher/credit note, so a retry would double-post and a status rewrite would violate BFL 7 kap.; recovery is a manual storno guided by result_data.posted_ids. Exception kept: AccountsNotInChartError in match_transaction_invoice still releases to pending because that executor is re-entrant past the storno. diff --git a/app/(dashboard)/pending/page.tsx b/app/(dashboard)/pending/page.tsx index 77447e6f..dd9c7bc4 100644 --- a/app/(dashboard)/pending/page.tsx +++ b/app/(dashboard)/pending/page.tsx @@ -275,6 +275,19 @@ function isAutoExpired(op: PendingOperation): boolean { return op.status === 'rejected' && rd?.auto_rejected === true && rd?.reason === 'expired' } +/** + * failed_partial rows (issue #842): the executor posted an irreversible + * voucher/credit note and then failed a later step. The dispatcher persisted + * the ids of what WAS posted in result_data.posted_ids; render them so the + * reviewer can locate the orphaned entity. + */ +function failedPartialPostedIds(op: PendingOperation): string | null { + const rd = op.result_data as { posted_ids?: Record } | null + const entries = Object.entries(rd?.posted_ids ?? {}) + if (entries.length === 0) return null + return entries.map(([key, value]) => `${key}: ${value}`).join(', ') +} + function formatRelativeTime(dateStr: string): string { const now = new Date() const date = new Date(dateStr) @@ -1253,6 +1266,11 @@ export default function PendingOperationsPage() { {t('badge_auto_expired')} )} + {op.status === 'failed_partial' && ( + + {t('badge_failed_partial')} + + )} {showHighRiskWarning && (

@@ -1273,6 +1291,14 @@ export default function PendingOperationsPage() { {t('auto_expired_detail')}

)} + {op.status === 'failed_partial' && ( +

+ {t('failed_partial_detail')} + {failedPartialPostedIds(op) && ( + ({failedPartialPostedIds(op)}) + )} +

+ )} ) }) diff --git a/app/api/pending-operations/[id]/reject/route.ts b/app/api/pending-operations/[id]/reject/route.ts index ac742f9e..10b2043c 100644 --- a/app/api/pending-operations/[id]/reject/route.ts +++ b/app/api/pending-operations/[id]/reject/route.ts @@ -70,9 +70,13 @@ export const POST = withRouteContext<{ params: Promise<{ id: string }> }>( ? 'Operation already rejected.' : op.status === 'expired' ? 'Operation already expired and can no longer be rejected.' - : `Operation already ${op.status}: it was approved explicitly (most likely via the ` + - 'Att göra / pending UI in parallel), not auto-committed. It can no longer be rejected; ' + - 'reverse or correct the resulting verifikat instead.' + : op.status === 'failed_partial' + ? 'Operation already resolved as failed_partial: it failed after posting an ' + + 'irreversible voucher. It can no longer be rejected; see result_data.posted_ids ' + + 'for what was posted and correct it with a storno if needed.' + : `Operation already ${op.status}: it was approved explicitly (most likely via the ` + + 'Att göra / pending UI in parallel), not auto-committed. It can no longer be rejected; ' + + 'reverse or correct the resulting verifikat instead.' return NextResponse.json( { error: explained, status: op.status }, { status: 409 } diff --git a/app/api/pending-operations/bulk-commit/route.ts b/app/api/pending-operations/bulk-commit/route.ts index 6af10854..611038a8 100644 --- a/app/api/pending-operations/bulk-commit/route.ts +++ b/app/api/pending-operations/bulk-commit/route.ts @@ -21,6 +21,7 @@ const STATUS_LABELS_SV: Record = { committing: 'godkänns just nu', committed: 'godkänd', rejected: 'avvisad', + failed_partial: 'delvis genomförd', } export const POST = withRouteContext( diff --git a/app/api/pending-operations/bulk-reject/route.ts b/app/api/pending-operations/bulk-reject/route.ts index f1a556bc..6f2034c8 100644 --- a/app/api/pending-operations/bulk-reject/route.ts +++ b/app/api/pending-operations/bulk-reject/route.ts @@ -16,6 +16,7 @@ const STATUS_LABELS_SV: Record = { committed: 'godkänd', rejected: 'avvisad', expired: 'utgången', + failed_partial: 'delvis genomförd', } /** diff --git a/app/api/pending-operations/route.ts b/app/api/pending-operations/route.ts index e7cd60c2..1da5d86d 100644 --- a/app/api/pending-operations/route.ts +++ b/app/api/pending-operations/route.ts @@ -23,11 +23,19 @@ export const GET = withRouteContext( // newer rejections and the "Utgick automatiskt" context would never be seen. const orderColumn = status === 'pending' ? 'created_at' : 'resolved_at' + // 'failed_partial' rows (issue #842: op failed AFTER posting an + // irreversible voucher) surface inside the Avvisade tab rather than a + // fourth tab: they are resolved-with-failure and must stay visible to the + // operator, but a dedicated tab for a rare state would bury it. A direct + // ?status=failed_partial query still returns only those rows. + const statusesFor = (candidate: string): string[] => + candidate === 'rejected' ? ['rejected', 'failed_partial'] : [candidate] + const listPromise = supabase .from('pending_operations') .select('*', { count: 'exact' }) .eq('company_id', companyId) - .eq('status', status) + .in('status', statusesFor(status)) .order(orderColumn, { ascending: false, nullsFirst: false }) .range(offset, offset + limit - 1) @@ -45,7 +53,7 @@ export const GET = withRouteContext( .from('pending_operations') .select('id', { count: 'exact', head: true }) .eq('company_id', companyId) - .eq('status', candidate), + .in('status', statusesFor(candidate)), ), ]) @@ -55,7 +63,7 @@ export const GET = withRouteContext( return NextResponse.json({ error: getUserErrorMessage(error) }, { status: 500 }) } - const counts: Partial> = { + const counts: Partial> = { [status]: count ?? 0, } otherStatuses.forEach((candidate, index) => { diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index d271b89f..d03bc9d1 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -13168,7 +13168,7 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - status: { type: 'string', enum: ['pending', 'committing', 'committed', 'rejected'], description: 'Default: pending' }, + status: { type: 'string', enum: ['pending', 'committing', 'committed', 'rejected', 'failed_partial'], description: 'Default: pending' }, risk_level: { type: 'string', enum: ['low', 'medium', 'high'] }, operation_type: { type: 'string', description: 'Filter to a single operation_type (e.g. "create_invoice")' }, limit: { type: 'number', minimum: 1, maximum: 200, description: 'Default 50' }, @@ -13408,8 +13408,12 @@ export const tools: McpTool[] = [ throw new Error( op.status === 'rejected' ? 'Operation already rejected.' - : `Operation already ${op.status}: approved explicitly (likely via the pending UI), ` + - 'not auto-committed. Reverse or correct the resulting verifikat instead.', + : op.status === 'failed_partial' + ? 'Operation already resolved as failed_partial: it failed after posting an ' + + 'irreversible voucher. See result_data.posted_ids for what was posted and ' + + 'correct it with a storno if needed.' + : `Operation already ${op.status}: approved explicitly (likely via the pending UI), ` + + 'not auto-committed. Reverse or correct the resulting verifikat instead.', ) } diff --git a/lib/api/schemas.ts b/lib/api/schemas.ts index d6e398e6..9b262adf 100644 --- a/lib/api/schemas.ts +++ b/lib/api/schemas.ts @@ -1905,7 +1905,9 @@ export const EventsQuerySchema = z.object({ // ============================================================ export const PendingOperationsQuerySchema = z.object({ - status: z.enum(['pending', 'committed', 'rejected']).default('pending'), + // 'failed_partial' is queryable directly; the UI folds it into the + // rejected tab (see app/api/pending-operations/route.ts). + status: z.enum(['pending', 'committed', 'rejected', 'failed_partial']).default('pending'), limit: z.coerce.number().int().min(1).max(100).default(50), offset: z.coerce.number().int().nonnegative().default(0), }) diff --git a/lib/pending-operations/__tests__/failed-partial.test.ts b/lib/pending-operations/__tests__/failed-partial.test.ts new file mode 100644 index 00000000..688cc274 --- /dev/null +++ b/lib/pending-operations/__tests__/failed-partial.test.ts @@ -0,0 +1,343 @@ +/** + * failed_partial dispatcher coverage (issue #842). + * + * Multi-step executors post an irreversible voucher (or persist a credit + * note) and then run later fallible steps. When such a later step fails, the + * dispatcher must land the op in the terminal 'failed_partial' status with + * the posted ids in result_data.posted_ids, instead of a clean-looking + * 'rejected' that hides the orphaned voucher. Clean failures (nothing posted + * yet) must keep today's 'rejected' behavior byte-for-byte. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { eventBus } from '@/lib/events/bus' +import { createQueuedMockSupabase } from '@/tests/helpers' +import { JournalEntryNotBalancedError } from '@/lib/bookkeeping/errors' +import type { PendingOperation } from '@/types' + +const mockCreatePaymentEntry = vi.fn() +const mockCreateCashEntry = vi.fn() +const mockCreateCreditNoteEntry = vi.fn() +vi.mock('@/lib/bookkeeping/invoice-entries', async () => { + const actual = await vi.importActual( + '@/lib/bookkeeping/invoice-entries', + ) + return { + ...actual, + createInvoicePaymentJournalEntry: (...args: unknown[]) => mockCreatePaymentEntry(...args), + createInvoiceCashEntry: (...args: unknown[]) => mockCreateCashEntry(...args), + createCreditNoteJournalEntry: (...args: unknown[]) => mockCreateCreditNoteEntry(...args), + } +}) + +const mockReverseEntry = vi.fn() +vi.mock('@/lib/bookkeeping/engine', async () => { + const actual = await vi.importActual( + '@/lib/bookkeeping/engine', + ) + return { + ...actual, + reverseEntry: (...args: unknown[]) => mockReverseEntry(...args), + } +}) + +import { commitPendingOperation } from '../commit' + +function makePendingOp(overrides: Partial): PendingOperation { + return { + id: 'op-1', + user_id: 'user-1', + company_id: 'company-1', + operation_type: 'match_transaction_invoice', + status: 'pending', + title: 'test', + params: {}, + preview_data: {}, + result_data: null, + actor_type: 'user', + actor_id: null, + actor_label: null, + risk_level: 'medium', + created_at: '2026-07-22T00:00:00Z', + resolved_at: null, + updated_at: '2026-07-22T00:00:00Z', + ...overrides, + } as PendingOperation +} + +/** + * Wraps the queued mock so every .update(payload) is recorded per table: + * the queued helper drops call arguments, but these tests must assert WHAT + * the dispatcher wrote to pending_operations, not only the returned status. + */ +function recordUpdates(supabase: { from: ReturnType }) { + const updates: Array<{ table: string; payload: Record }> = [] + const original = supabase.from.getMockImplementation() as (table: string) => unknown + supabase.from.mockImplementation((table: string) => { + const chain = original(table) as object + return new Proxy(chain, { + get(target, prop, receiver) { + if (prop === 'update') { + return (payload: Record) => { + updates.push({ table, payload }) + return (Reflect.get(target, 'update', receiver) as (p: unknown) => unknown)(payload) + } + } + return Reflect.get(target, prop, receiver) + }, + }) + }) + return updates +} + +function pendingOpUpdates(updates: Array<{ table: string; payload: Record }>) { + return updates.filter((u) => u.table === 'pending_operations') +} + +const baseTransaction = { + id: 'tx-1', + company_id: 'company-1', + amount: 500, + currency: 'SEK', + date: '2026-05-12', + invoice_id: null, + journal_entry_id: null, + cash_account_id: null, +} + +const baseInvoice = { + id: 'inv-1', + invoice_number: 'F-2026001', + status: 'sent', + total: 500, + remaining_amount: 500, + paid_amount: 0, + currency: 'SEK', + exchange_rate: null, + journal_entry_id: null, + credited_invoice_id: null, + customer: { name: 'Kund AB' }, +} + +beforeEach(() => { + vi.clearAllMocks() + eventBus.clear() + mockCreatePaymentEntry.mockResolvedValue({ id: 'je-pay' }) + mockCreateCashEntry.mockResolvedValue({ id: 'je-pay' }) + mockCreateCreditNoteEntry.mockResolvedValue({ id: 'je-credit' }) + mockReverseEntry.mockResolvedValue({ id: 'je-storno' }) +}) + +describe('match_transaction_invoice: partial commit after the storno', () => { + it('lands failed_partial with the reversal voucher id when the payment JE throws after the storno', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: { ...baseTransaction, journal_entry_id: 'je-old' }, error: null }) // transaction fetch + enqueue({ data: baseInvoice, error: null }) // invoice fetch + enqueue({ data: { accounting_method: 'accrual', entity_type: 'aktiebolag' }, error: null }) // settings + enqueue({ data: null, error: null }) // transactions unlink after storno + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + mockCreatePaymentEntry.mockRejectedValue(new JournalEntryNotBalancedError(500, 400)) + + const op = makePendingOp({ params: { transaction_id: 'tx-1', invoice_id: 'inv-1' } }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(mockReverseEntry).toHaveBeenCalledWith(expect.anything(), 'company-1', 'user-1', 'je-old') + expect(result.status).toBe('failed') + expect(result.http_status).toBe(500) + expect(result.code).toBe('partial_commit') + expect(result.data).toEqual({ posted_ids: { reversal_journal_entry_id: 'je-storno' } }) + + const opUpdates = pendingOpUpdates(updates) + // First write is the atomic claim, second is the terminal status. + expect(opUpdates[0]?.payload).toEqual({ status: 'committing' }) + expect(opUpdates[1]?.payload).toMatchObject({ + status: 'failed_partial', + result_data: { + threw: true, + posted_ids: { reversal_journal_entry_id: 'je-storno' }, + }, + }) + expect(opUpdates[1]?.payload.resolved_at).toBeTruthy() + }) + + it('lands failed_partial with the payment JE id when the invoice CAS update matches zero rows', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: baseTransaction, error: null }) // transaction fetch (no prior JE: no storno) + enqueue({ data: baseInvoice, error: null }) // invoice fetch + enqueue({ data: { accounting_method: 'accrual', entity_type: 'aktiebolag' }, error: null }) // settings + enqueue({ data: [], error: null }) // invoice CAS update: zero rows (raced fully-paid) + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + const op = makePendingOp({ params: { transaction_id: 'tx-1', invoice_id: 'inv-1' } }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + // A 409 AFTER the payment voucher posted must NOT auto-reject: it is a + // partial commit and the posted JE id must be surfaced. + expect(result.status).toBe('failed') + expect(result.http_status).toBe(409) + expect(result.auto_rejected).toBeUndefined() + expect(result.code).toBe('partial_commit') + expect(result.data).toEqual({ posted_ids: { payment_journal_entry_id: 'je-pay' } }) + + const opUpdates = pendingOpUpdates(updates) + expect(opUpdates[1]?.payload).toMatchObject({ + status: 'failed_partial', + result_data: { + http_status: 409, + posted_ids: { payment_journal_entry_id: 'je-pay' }, + }, + }) + }) + + it('keeps the clean rejected path when the failure happens BEFORE anything is posted', async () => { + // The settlement-account lookup now runs before the storno: an infra + // failure there must reject the op with nothing posted and must NOT be + // labeled a partial commit. + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ + data: { ...baseTransaction, journal_entry_id: 'je-old', cash_account_id: 'ca-broken' }, + error: null, + }) // transaction fetch + enqueue({ data: baseInvoice, error: null }) // invoice fetch + enqueue({ data: { accounting_method: 'accrual', entity_type: 'aktiebolag' }, error: null }) // settings + enqueue({ data: null, error: { message: 'connection reset' } }) // cash_accounts lookup errors + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + const op = makePendingOp({ params: { transaction_id: 'tx-1', invoice_id: 'inv-1' } }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(mockReverseEntry).not.toHaveBeenCalled() + expect(result.status).toBe('failed') + expect(result.code).toBeUndefined() + + const opUpdates = pendingOpUpdates(updates) + expect(opUpdates[1]?.payload).toMatchObject({ + status: 'rejected', + result_data: { threw: true }, + }) + }) + + it('keeps the 409 auto-reject when the CAS update races and no voucher was posted', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: baseTransaction, error: null }) // transaction fetch + enqueue({ data: baseInvoice, error: null }) // invoice fetch + enqueue({ data: { accounting_method: 'accrual', entity_type: 'aktiebolag' }, error: null }) // settings + enqueue({ data: [], error: null }) // invoice CAS update: zero rows + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + // JE creation "succeeded" with null (e.g. no open fiscal period): + // nothing was posted, so today's auto-reject semantics must survive. + mockCreatePaymentEntry.mockResolvedValue(null) + + const op = makePendingOp({ params: { transaction_id: 'tx-1', invoice_id: 'inv-1' } }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(result.status).toBe('rejected') + expect(result.auto_rejected).toBe(true) + expect(result.http_status).toBe(409) + + const opUpdates = pendingOpUpdates(updates) + expect(opUpdates[1]?.payload).toMatchObject({ + status: 'rejected', + result_data: { auto_rejected: true }, + }) + }) +}) + +describe('credit_invoice: partial commit after the credit note persisted', () => { + const originalInvoice = { + id: 'inv-1', + invoice_number: 'F-2026001', + status: 'sent', + document_type: 'invoice', + customer_id: 'cust-1', + delivery_date: null, + currency: 'SEK', + exchange_rate: null, + exchange_rate_date: null, + subtotal: 400, + subtotal_sek: 400, + vat_amount: 100, + vat_amount_sek: 100, + total: 500, + total_sek: 500, + vat_treatment: 'standard_25', + vat_rate: 25, + moms_ruta: null, + reverse_charge_text: null, + your_reference: null, + our_reference: null, + journal_entry_id: null, + default_dimensions: {}, + items: [], + } + + it('lands failed_partial with the credit note id when the credit-note JE throws', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: originalInvoice, error: null }) // original invoice fetch + enqueue({ data: { id: 'cn-1', invoice_date: '2026-07-22' }, error: null }) // credit note insert + enqueue({ data: null, error: null }) // invoice_items insert + enqueue({ data: null, error: null }) // original invoice -> credited + enqueue({ + data: { id: 'cn-1', invoice_date: '2026-07-22', items: [], customer: { name: 'Kund AB' } }, + error: null, + }) // complete credit note fetch + enqueue({ data: { entity_type: 'aktiebolag', accounting_method: 'accrual' }, error: null }) // settings + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + mockCreateCreditNoteEntry.mockRejectedValue(new JournalEntryNotBalancedError(500, 400)) + + const op = makePendingOp({ + operation_type: 'credit_invoice', + params: { invoice_id: 'inv-1' }, + }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(result.status).toBe('failed') + expect(result.code).toBe('partial_commit') + expect(result.data).toEqual({ + posted_ids: { credit_note_id: 'cn-1', original_invoice_id: 'inv-1' }, + }) + + const opUpdates = pendingOpUpdates(updates) + expect(opUpdates[1]?.payload).toMatchObject({ + status: 'failed_partial', + result_data: { + threw: true, + posted_ids: { credit_note_id: 'cn-1', original_invoice_id: 'inv-1' }, + }, + }) + }) + + it('keeps the clean rejected path when the credit note itself fails to persist', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const updates = recordUpdates(supabase) + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: originalInvoice, error: null }) // original invoice fetch + enqueue({ data: null, error: { message: 'insert failed' } }) // credit note insert fails + enqueue({ data: null, error: null }) // dispatcher pending_operations update + + const op = makePendingOp({ + operation_type: 'credit_invoice', + params: { invoice_id: 'inv-1' }, + }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(result.status).toBe('failed') + expect(result.code).toBeUndefined() + expect(mockCreateCreditNoteEntry).not.toHaveBeenCalled() + + const opUpdates = pendingOpUpdates(updates) + expect(opUpdates[1]?.payload).toMatchObject({ status: 'rejected' }) + }) +}) diff --git a/lib/pending-operations/commit.ts b/lib/pending-operations/commit.ts index 0ef0d244..21507951 100644 --- a/lib/pending-operations/commit.ts +++ b/lib/pending-operations/commit.ts @@ -60,6 +60,7 @@ import { type SkatteverketCommitServices, type SkvSubmitResult, } from '@/lib/pending-operations/skatteverket-commit' +import { PartialCommitError } from '@/lib/pending-operations/errors' import { getEmailService } from '@/lib/email/service' import { hasCapability, CAPABILITY_BLOCKED_MESSAGE_SV } from '@/lib/entitlements/has-capability' import { PAID_OPERATION_CAPABILITY_MAP } from '@/lib/entitlements/keys' @@ -192,7 +193,16 @@ async function recordSkippedInvoiceJournalEntry( // ── Executors ──────────────────────────────────────────────────── -type ExecutorResult = { data?: Record; error?: string; status?: number } +type ExecutorResult = { + data?: Record + error?: string + status?: number + // Set when the executor already performed an irreversible side-effect + // (posted voucher, persisted credit note) before the failure in `error`: + // the dispatcher then lands the op in 'failed_partial' instead of + // 'rejected' and persists these ids in result_data.posted_ids (issue #842). + partialPostedIds?: Record +} async function commitCategorizeTransaction( supabase: SupabaseClient, @@ -1722,13 +1732,13 @@ async function commitMatchTransactionInvoice( } const { newPaidAmount, newRemaining, isFullyPaid, newStatus } = payment.plan - if (transaction.journal_entry_id) { - await reverseEntry(supabase, companyId, userId, transaction.journal_entry_id) - await supabase.from('transactions').update({ journal_entry_id: null }).eq('id', transactionId) - } - const now = new Date().toISOString() + // Read-only prevalidation, deliberately hoisted ABOVE the irreversible + // storno below (issue #842): resolveSettlementAccount can throw + // (BookkeepingDatabaseError on a failed cash_accounts lookup), and a throw + // here must reject the op with NOTHING posted. Behavior-preserving on the + // happy path: these are pure reads. const { data: settings } = await supabase .from('company_settings').select('accounting_method, entity_type').eq('company_id', companyId).single() @@ -1746,6 +1756,17 @@ async function commitMatchTransactionInvoice( // transaction settled into. Mirrors the match-invoice route fix. const paymentAccount = await resolveSettlementAccount(supabase, companyId, transaction.cash_account_id, log) + // From here on the executor posts irreversible vouchers. Track their ids so + // a later failure can land the op in 'failed_partial' carrying them + // (issue #842) instead of a clean-looking 'rejected'. + const postedIds: Record = {} + + if (transaction.journal_entry_id) { + const reversal = await reverseEntry(supabase, companyId, userId, transaction.journal_entry_id) + postedIds.reversal_journal_entry_id = reversal.id + await supabase.from('transactions').update({ journal_entry_id: null }).eq('id', transactionId) + } + let journalEntryId: string | null = null try { if (useCashEntry) { @@ -1762,9 +1783,26 @@ async function commitMatchTransactionInvoice( journalEntryId = je?.id ?? null } } catch (err) { - if (isBookkeepingError(err)) throw err + // Recoverable: the dispatcher releases the op back to 'pending' and this + // executor is re-entrant past the storno (the transaction was unlinked + // above, so a retry does not post a second storno). Keep that path even + // when a reversal voucher was already posted. + if (err instanceof AccountsNotInChartError) throw err + if (isBookkeepingError(err)) { + // A reversal voucher already posted makes this a partial commit, not a + // clean failure: surface the storno id (issue #842). + if (Object.keys(postedIds).length > 0) { + throw new PartialCommitError( + `match_transaction_invoice failed after posting a reversal voucher: ${err instanceof Error ? err.message : 'journal entry creation failed'}`, + postedIds, + err, + ) + } + throw err + } log.error('Failed to create match journal entry:', err) } + if (journalEntryId) postedIds.payment_journal_entry_id = journalEntryId const { data: updatedRows, error: updateInvError } = await supabase .from('invoices') @@ -1778,9 +1816,19 @@ async function commitMatchTransactionInvoice( .in('status', ['sent', 'overdue', 'partially_paid']) .select('id') - if (updateInvError) return { error: 'Failed to update invoice status', status: 500 } + if (updateInvError) { + return { + error: 'Failed to update invoice status', + status: 500, + ...(Object.keys(postedIds).length > 0 ? { partialPostedIds: postedIds } : {}), + } + } if (!updatedRows || updatedRows.length === 0) { - return { error: 'Invoice has already been fully paid or is no longer matchable', status: 409 } + return { + error: 'Invoice has already been fully paid or is no longer matchable', + status: 409, + ...(Object.keys(postedIds).length > 0 ? { partialPostedIds: postedIds } : {}), + } } const paymentNotes = (accountingMethod === 'cash' && !isFullyPaid) @@ -3034,7 +3082,19 @@ async function commitCreditInvoice( .eq('id', creditNote.id) } } catch (err) { - if (isBookkeepingError(err)) throw err + if (isBookkeepingError(err)) { + // The credit note row and the original's 'credited' flip are already + // persisted: a clean 'rejected' would hide them. Land the op in + // 'failed_partial' carrying the ids (issue #842). This intentionally + // covers AccountsNotInChartError too: the release-to-pending retry + // path cannot recover a credit_invoice op (re-running the executor + // auto-rejects with 409 because the original is already 'credited'). + throw new PartialCommitError( + `credit_invoice failed after persisting the credit note: ${err instanceof Error ? err.message : 'journal entry creation failed'}`, + { credit_note_id: creditNote.id, original_invoice_id: id }, + err, + ) + } log.error('Failed to create credit note journal entry:', err) } @@ -4615,6 +4675,30 @@ async function commitPendingOperationInner( } } } catch (err) { + // Partial commit (issue #842): the executor already posted an + // irreversible side-effect (storno voucher, credit note) before a later + // step failed. 'rejected' would misrepresent reality and hide the posted + // entity, so land the op in the terminal 'failed_partial' status with the + // posted ids in result_data so an operator can locate the orphan. Checked + // FIRST: a wrapped recoverable cause must NOT release the claim back to + // 'pending' (the side-effect already exists). + if (err instanceof PartialCommitError) { + await supabase + .from('pending_operations') + .update({ + status: 'failed_partial', + resolved_at: new Date().toISOString(), + result_data: { error: err.message, threw: true, posted_ids: err.postedIds }, + }) + .eq('id', pendingOp.id) + return { + status: 'failed', + error: err.message, + http_status: 500, + code: 'partial_commit', + data: { posted_ids: err.postedIds }, + } + } // Accounts-not-in-chart is RECOVERABLE: the booking itself is valid; the // company's chart just lacks the (standard BAS) accounts it posts to. Do // NOT consume the op: release the atomic claim back to 'pending' so the @@ -4670,6 +4754,36 @@ async function commitPendingOperationInner( } if (result.error) { + // Structured partial marker (issue #842): same semantics as the + // PartialCommitError branch above, for executors that report the failure + // via the ExecutorResult contract instead of throwing. Must run before + // the auto-reject branch: a 409 AFTER a voucher was posted is a partial + // commit, not a re-stageable rejection. + const partialPostedIds = + result.partialPostedIds && Object.keys(result.partialPostedIds).length > 0 + ? result.partialPostedIds + : null + if (partialPostedIds) { + await supabase + .from('pending_operations') + .update({ + status: 'failed_partial', + resolved_at: new Date().toISOString(), + result_data: { + error: result.error, + http_status: result.status, + posted_ids: partialPostedIds, + }, + }) + .eq('id', pendingOp.id) + return { + status: 'failed', + error: result.error, + http_status: result.status ?? 500, + code: 'partial_commit', + data: { posted_ids: partialPostedIds }, + } + } const isAutoReject = result.status === 404 || result.status === 409 await supabase .from('pending_operations') diff --git a/lib/pending-operations/errors.ts b/lib/pending-operations/errors.ts new file mode 100644 index 00000000..e3b0571a --- /dev/null +++ b/lib/pending-operations/errors.ts @@ -0,0 +1,35 @@ +/** + * Typed errors for the pending-operations commit path. + * + * PartialCommitError is thrown by multi-step executors when an irreversible + * side-effect (posted voucher, persisted credit note) already exists and a + * LATER step fails. The dispatcher (commitPendingOperationInner) detects it + * and lands the op in the terminal status 'failed_partial' instead of + * 'rejected', persisting the posted ids in result_data.posted_ids so an + * operator can locate the orphaned voucher/entity (issue #842). + * + * Do NOT throw this before the first irreversible step: a clean failure with + * nothing posted must keep today's 'rejected' semantics. + */ +export class PartialCommitError extends Error { + readonly name = 'PartialCommitError' + /** + * Ids of the entities that were irreversibly created before the failure, + * keyed by a stable snake_case label (e.g. reversal_journal_entry_id, + * payment_journal_entry_id, credit_note_id). Persisted verbatim into + * pending_operations.result_data.posted_ids. + */ + readonly postedIds: Record + /** The underlying failure that interrupted the executor. */ + readonly cause: unknown + + constructor(message: string, postedIds: Record, cause?: unknown) { + super(message) + this.postedIds = postedIds + this.cause = cause + } +} + +export function isPartialCommitError(err: unknown): err is PartialCommitError { + return err instanceof PartialCommitError +} diff --git a/messages/en.json b/messages/en.json index 07ab9b67..f761b5c2 100644 --- a/messages/en.json +++ b/messages/en.json @@ -505,7 +505,9 @@ "origin_api": "Suggested via API integration", "origin_cron": "Created by a scheduled job", "badge_auto_expired": "Expired automatically", - "auto_expired_detail": "Expired automatically after 30 days without action. Nothing was booked." + "auto_expired_detail": "Expired automatically after 30 days without action. Nothing was booked.", + "badge_failed_partial": "Partially completed", + "failed_partial_detail": "The action was interrupted after an irreversible posting had already been made. What was posted remains and may need manual correction." }, "deadlines": { "title": "Deadlines", diff --git a/messages/sv.json b/messages/sv.json index 07ced225..b33f12be 100644 --- a/messages/sv.json +++ b/messages/sv.json @@ -505,7 +505,9 @@ "origin_api": "Föreslaget via API-integration", "origin_cron": "Skapat av automatiskt jobb", "badge_auto_expired": "Utgick automatiskt", - "auto_expired_detail": "Utgick automatiskt efter 30 dagar utan åtgärd. Inget bokfördes." + "auto_expired_detail": "Utgick automatiskt efter 30 dagar utan åtgärd. Inget bokfördes.", + "badge_failed_partial": "Delvis genomförd", + "failed_partial_detail": "Åtgärden avbröts efter att en oåterkallelig bokföring redan hade skett. Det som bokfördes står kvar och kan behöva rättas manuellt." }, "deadlines": { "title": "Deadlines", diff --git a/supabase/migrations/20260722134114_pending_operations_failed_partial_status.sql b/supabase/migrations/20260722134114_pending_operations_failed_partial_status.sql new file mode 100644 index 00000000..debcfcb2 --- /dev/null +++ b/supabase/migrations/20260722134114_pending_operations_failed_partial_status.sql @@ -0,0 +1,86 @@ +-- Migration: add 'failed_partial' terminal status to pending_operations. +-- +-- Issue #842 (follow-up deferred from PR #841): several multi-step executors +-- post an irreversible voucher and then perform a later fallible step (e.g. +-- match_transaction_invoice posts a storno before building the payment JE; +-- credit_invoice persists the credit note before posting its JE). When the +-- later step failed, the dispatcher marked the WHOLE op 'rejected', which +-- misrepresents reality: an immutable voucher/credit note already exists. +-- +-- 'failed_partial' is the honest terminal state for that case: the operation +-- did NOT complete, but side-effects were posted and their ids are recorded +-- in result_data.posted_ids so an operator can find the orphaned voucher. +-- +-- Semantics: +-- - terminal: rows are immutable and undeletable once in this state, same +-- as 'committed'/'rejected' (BFL 7 kap.: the posted underlag and the +-- record of what happened must be unalterable) +-- - NOT re-committable: the CAS claim only picks up status = 'pending', +-- which already excludes it; the immutability trigger below additionally +-- blocks any status rewrite +-- - NOT pending work: worklist/pending counts filter on 'pending' only + +-- ============================================================================= +-- 1. pending_operations_status_check: add 'failed_partial' +-- ============================================================================= +-- Same drop + re-add pattern as 20260504100000 used when adding 'committing'. +ALTER TABLE public.pending_operations + DROP CONSTRAINT IF EXISTS pending_operations_status_check; + +ALTER TABLE public.pending_operations + ADD CONSTRAINT pending_operations_status_check + CHECK (status IN ('pending', 'committing', 'committed', 'rejected', 'failed_partial')); + +-- ============================================================================= +-- 2. terminal-state immutability: treat 'failed_partial' as terminal +-- ============================================================================= +-- Replaces the functions from 20260504100000 (never edit that migration) so +-- the UPDATE/DELETE blockers cover the new terminal state too. + +CREATE OR REPLACE FUNCTION public.enforce_pending_operations_immutability() +RETURNS TRIGGER AS $$ +BEGIN + IF OLD.status IN ('committed', 'rejected', 'failed_partial') THEN + RAISE EXCEPTION + 'pending_operations row % is in terminal state % and cannot be modified (BFL 7 kap.)', + OLD.id, OLD.status + USING ERRCODE = 'check_violation'; + END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS pending_operations_immutability ON public.pending_operations; +CREATE TRIGGER pending_operations_immutability + BEFORE UPDATE ON public.pending_operations + FOR EACH ROW + EXECUTE FUNCTION public.enforce_pending_operations_immutability(); + +CREATE OR REPLACE FUNCTION public.enforce_pending_operations_no_delete() +RETURNS TRIGGER AS $$ +BEGIN + IF OLD.status IN ('committed', 'rejected', 'failed_partial') THEN + RAISE EXCEPTION + 'pending_operations row % is in terminal state % and cannot be deleted (BFL 7 kap.)', + OLD.id, OLD.status + USING ERRCODE = 'check_violation'; + END IF; + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS pending_operations_no_delete ON public.pending_operations; +CREATE TRIGGER pending_operations_no_delete + BEFORE DELETE ON public.pending_operations + FOR EACH ROW + EXECUTE FUNCTION public.enforce_pending_operations_no_delete(); + +-- Note: pending_ops_auto_commit_status (20260430120000) constrained +-- auto_committed_at to status = 'committed', but it was dropped together with +-- the auto-commit feature in 20260505190027, so there is nothing to expand +-- for the new status. + +-- ============================================================================= +-- 3. PostgREST schema reload +-- ============================================================================= +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/pending-operations-failed-partial.pg.test.ts b/tests/pg/pending-operations-failed-partial.pg.test.ts new file mode 100644 index 00000000..648c09b2 --- /dev/null +++ b/tests/pg/pending-operations-failed-partial.pg.test.ts @@ -0,0 +1,139 @@ +/** + * pg-real coverage for migration 20260722134114_pending_operations_failed_partial_status. + * + * 'failed_partial' (issue #842) is the terminal state for a pending_operation + * whose executor posted an irreversible side-effect (storno voucher, credit + * note) and then failed a later step. These tests prove the schema semantics + * the dispatcher relies on: + * - the CHECK constraint accepts the new status + * - committing -> failed_partial is a legal transition (the dispatcher + * always claims to 'committing' first) + * - failed_partial rows are immutable and undeletable (terminal, BFL 7 kap.) + * - the CAS claim (UPDATE ... WHERE status = 'pending') can never pick up a + * failed_partial row, so the op is not re-committable + */ +import { describe, expect, it } from 'vitest' +import { getPool } from './setup' +import { seedCompany } from './fixtures' + +async function insertOp( + userId: string, + companyId: string, + status: string, + extra: { resolvedAt?: boolean; resultData?: Record } = {}, +): Promise { + const result = await getPool().query<{ id: string }>( + `INSERT INTO public.pending_operations + (user_id, company_id, operation_type, status, title, params, preview_data, resolved_at, result_data) + VALUES ($1, $2, 'match_transaction_invoice', $3, 'failed-partial pg test', '{}', '{}', + ${extra.resolvedAt ? 'now()' : 'NULL'}, $4) + RETURNING id`, + [userId, companyId, status, JSON.stringify(extra.resultData ?? {})], + ) + return result.rows[0]!.id +} + +describe('pending_operations: failed_partial status (migration 20260722134114)', () => { + it('CHECK constraint accepts failed_partial with posted ids in result_data', async () => { + const { userId, companyId } = await seedCompany() + const id = await insertOp(userId, companyId, 'failed_partial', { + resolvedAt: true, + resultData: { + error: 'payment JE failed after storno', + posted_ids: { reversal_journal_entry_id: '00000000-0000-0000-0000-000000000001' }, + }, + }) + + const row = await getPool().query<{ status: string; result_data: { posted_ids?: Record } }>( + `SELECT status, result_data FROM public.pending_operations WHERE id = $1`, + [id], + ) + expect(row.rows[0]?.status).toBe('failed_partial') + expect(row.rows[0]?.result_data.posted_ids?.reversal_journal_entry_id).toBe( + '00000000-0000-0000-0000-000000000001', + ) + }) + + it('CHECK constraint still rejects unknown statuses', async () => { + const { userId, companyId } = await seedCompany() + await expect(insertOp(userId, companyId, 'failed')).rejects.toThrow( + /pending_operations_status_check|check constraint/i, + ) + }) + + it('allows the committing -> failed_partial transition the dispatcher performs', async () => { + const { userId, companyId } = await seedCompany() + const id = await insertOp(userId, companyId, 'committing') + + const upd = await getPool().query( + `UPDATE public.pending_operations + SET status = 'failed_partial', + resolved_at = now(), + result_data = '{"error":"second step failed","posted_ids":{"credit_note_id":"cn-1"}}' + WHERE id = $1 RETURNING id, status`, + [id], + ) + expect(upd.rowCount).toBe(1) + expect(upd.rows[0]?.status).toBe('failed_partial') + }) + + it('blocks UPDATE on failed_partial rows (terminal-state immutability trigger)', async () => { + const { userId, companyId } = await seedCompany() + const id = await insertOp(userId, companyId, 'failed_partial', { resolvedAt: true }) + + await expect( + getPool().query( + `UPDATE public.pending_operations SET title = 'tampered' WHERE id = $1`, + [id], + ), + ).rejects.toThrow(/terminal state|BFL 7/i) + + // Including status rewrites: failed_partial can never go back to pending. + await expect( + getPool().query( + `UPDATE public.pending_operations SET status = 'pending' WHERE id = $1`, + [id], + ), + ).rejects.toThrow(/terminal state|BFL 7/i) + }) + + it('blocks DELETE on failed_partial rows', async () => { + const { userId, companyId } = await seedCompany() + const id = await insertOp(userId, companyId, 'failed_partial', { resolvedAt: true }) + + await expect( + getPool().query(`DELETE FROM public.pending_operations WHERE id = $1`, [id]), + ).rejects.toThrow(/terminal state|BFL 7/i) + }) + + it('CAS claim (status = pending guard) never picks up a failed_partial row', async () => { + const { userId, companyId } = await seedCompany() + const id = await insertOp(userId, companyId, 'failed_partial', { resolvedAt: true }) + + const claim = await getPool().query( + `UPDATE public.pending_operations SET status = 'committing' + WHERE id = $1 AND status = 'pending' RETURNING id`, + [id], + ) + expect(claim.rowCount).toBe(0) + }) + + it('keeps committed and rejected terminal behavior intact (regression)', async () => { + const { userId, companyId } = await seedCompany() + const committedId = await insertOp(userId, companyId, 'committed', { resolvedAt: true }) + const rejectedId = await insertOp(userId, companyId, 'rejected', { resolvedAt: true }) + + await expect( + getPool().query( + `UPDATE public.pending_operations SET title = 'tampered' WHERE id = $1`, + [committedId], + ), + ).rejects.toThrow(/terminal state|BFL 7/i) + await expect( + getPool().query( + `UPDATE public.pending_operations SET title = 'tampered' WHERE id = $1`, + [rejectedId], + ), + ).rejects.toThrow(/terminal state|BFL 7/i) + }) +}) diff --git a/types/index.ts b/types/index.ts index 1ffeb7de..2ea702dc 100644 --- a/types/index.ts +++ b/types/index.ts @@ -2092,7 +2092,11 @@ export type PendingOperationType = // Semesterårsavslut: rolls vacation balances into the next year and may // post a 2920/2940 drift-adjustment verifikation (Phase 3). | 'vacation_year_close' -export type PendingOperationStatus = 'pending' | 'committing' | 'committed' | 'rejected' +// 'failed_partial' (issue #842, DB CHECK widened in 20260722134114): terminal +// state for ops whose executor posted an irreversible side-effect (voucher, +// credit note) and then failed a later step. Not re-committable, not pending +// work; result_data.posted_ids carries the ids of what WAS posted. +export type PendingOperationStatus = 'pending' | 'committing' | 'committed' | 'rejected' | 'failed_partial' // 'agent_chat' = the in-app AI chat (DB CHECK widened in migration // 20260519090000_actor_type_agent_chat).