fix(mcp): reject unparseable voucher lines and allocation kinds; already-booked and unlinkable say why; N:1 reconcile groups survive staging (#2171)

* fix(mcp): reject unparseable voucher lines and allocation kinds; already-booked and unlinkable say why; N:1 reconcile groups survive staging Four MCP feedback reports about the same failure class: the server does no runtime validation of inputSchema, so a shape mistake was coerced into a wrong-but-well-formed call, and the error the agent finally saw pointed at the wrong thing. - create_voucher / correct_entry: a line naming neither debit_amount nor credit_amount was `Number(undefined) || 0`-ed into 0/0, and the balance check reported "debits 0 SEK, credits 0 SEK" for four perfectly balanced formats ({debit}, {amount, side}, {debitAmount}, signed amount). The line shape is now checked first, the error names the keys it got and shows a valid line, and a non-numeric amount is rejected as such (seq 318571). - match_batch_allocate: kind is the key every guard branches on (direction vs sign, required id per kind, tenant pre-check on the invoices). With kind absent none of them fired: an incoming +50 359 SEK payment against three kundfakturor staged as allocations_kind "supplier_invoice" with zero invoice checks. A missing or unknown kind is now rejected before any query, with the id field that goes with each kind (seq 319919). - categorize_transaction on an already-booked transaction returned the core's success-shaped object, which fails STAGED_OPERATION_SCHEMA on strict clients: the agent saw "Structured content does not match the tool's output schema" and never the reason. It now throws, naming the existing journal_entry_id (seq 288574). - reconcile_match: the dry run flattens a pair into one link per outside row, and the staging rebuild put each back as its own 1:1 pair, so an N:1 group (Skatteverket "Avdragen skatt" + "Arbetsgivaravgift" against one 1630 verifikat, sum exact) reached the executor as N pairs each asked to settle the whole verifikat: PAIR_NOT_CLOSED on all 18. Links sharing a verifikat now fold back into one pair, mirroring the existing 1:N fold, and "No linkable pairs" carries the dry run's skip reasons (seq 292682). No tools/list payload change: no schema text touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yw62FMXGSzo6icFDiBwP3 * docs(decisions): N:1 reconcile fold at staging Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yw62FMXGSzo6icFDiBwP3 --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-04 09:27:26 +02:00
parent f767977bf5
commit 67e878fb23
6 changed files with 339 additions and 10 deletions
@@ -0,0 +1,63 @@
+/**
+ * gnubok_categorize_transaction on a transaction that already has a
+ * journal entry.
+ *
+ * categorizeTransactionCore returns a success-shaped object for this case
+ * ({ success: true, journal_entry_created: false, journal_entry_error }),
+ * which the tool used to pass through as its result. STAGED_OPERATION_SCHEMA
+ * requires staged/risk_level/actor/message/preview, so strict clients
+ * rejected the structured content and the agent saw only "Structured content
+ * does not match the tool's output schema" with the real reason swallowed
+ * (feedback seq 288574). The dispatcher's isError path carries no
+ * structuredContent, so an Error is the only schema-conformant way out.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { createQueuedMockSupabase } from '@/tests/helpers'
+
+vi.mock('@/lib/transactions/booking-duplicate-detection', () => ({
+  detectBookingDuplicate: vi.fn().mockResolvedValue(null),
+}))
+
+import { tools } from '../server'
+
+const categorize = tools.find((t) => t.name === 'gnubok_categorize_transaction')!
+
+const TX_ID = '00000000-0000-4000-8000-0000000000aa'
+const JE_ID = '00000000-0000-4000-8000-0000000000bb'
+
+beforeEach(() => {
+  vi.clearAllMocks()
+})
+
+describe('gnubok_categorize_transaction: already booked', () => {
+  it('throws a plain error naming the existing verifikat instead of returning an off-schema object', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({
+      data: {
+        id: TX_ID,
+        date: '2026-08-26',
+        amount: 25000,
+        currency: 'SEK',
+        amount_sek: 25000,
+        exchange_rate: null,
+        description: 'Aktiekapital',
+        merchant_name: null,
+        cash_account_id: null,
+        document_id: null,
+        journal_entry_id: JE_ID,
+        is_business: true,
+      },
+      error: null,
+    }) // core: transactions select('*')
+
+    await expect(
+      categorize.execute(
+        { transaction_id: TX_ID, category: 'income_other' },
+        'company-1',
+        'user-1',
+        supabase as never,
+        { type: 'api_key' } as never,
+      ),
+    ).rejects.toThrow(new RegExp(`already booked \\(journal_entry_id ${JE_ID}\\)`))
+  })
+})
@@ -0,0 +1,70 @@
+/**
+ * gnubok_match_batch_allocate: allocations[].kind is load-bearing.
+ *
+ * Every staging guard branches on kind (direction vs transaction sign, the
+ * required id per kind, the tenant pre-check on the referenced invoices).
+ * With kind absent none of them fired: an incoming +50 359 SEK payment
+ * against three kundfakturor staged as allocations_kind "supplier_invoice"
+ * with zero invoice checks (feedback seq 319919). No host validates
+ * inputSchema at runtime, so the tool rejects a missing or unknown kind
+ * before touching the database.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { createQueuedMockSupabase } from '@/tests/helpers'
+import { tools } from '../server'
+
+const allocate = tools.find((t) => t.name === 'gnubok_match_batch_allocate')!
+
+const TX_ID = '11111111-1111-4111-8111-111111111111'
+const INV_ID = '22222222-2222-4222-8222-222222222222'
+
+beforeEach(() => {
+  vi.clearAllMocks()
+})
+
+describe('gnubok_match_batch_allocate: kind guard', () => {
+  it('rejects an allocation with no kind before any query runs, and says which id goes with which kind', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    await expect(
+      allocate.execute(
+        { transaction_id: TX_ID, allocations: [{ invoice_id: INV_ID, amount: 100 }] },
+        'company-1',
+        'user-1',
+        supabase as never,
+        { type: 'api_key' } as never,
+      ),
+    ).rejects.toThrow(/allocations\[0\]\.kind is required: "customer_invoice" \(incoming payment, pass invoice_id\)/)
+    expect(supabase.from).not.toHaveBeenCalled()
+  })
+
+  it('rejects an unknown kind and echoes it', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    await expect(
+      allocate.execute(
+        { transaction_id: TX_ID, allocations: [{ kind: 'customer', invoice_id: INV_ID, amount: 100 }] },
+        'company-1',
+        'user-1',
+        supabase as never,
+        { type: 'api_key' } as never,
+      ),
+    ).rejects.toThrow(/allocations\[0\]\.kind is required.*got "customer"/)
+    expect(supabase.from).not.toHaveBeenCalled()
+  })
+
+  it('still reaches the direction guard with a valid kind (the existing contract)', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({
+      data: { id: TX_ID, description: 'OPPY', merchant_name: null, amount: -100, currency: 'SEK', date: '2026-08-31', journal_entry_id: null },
+      error: null,
+    })
+    await expect(
+      allocate.execute(
+        { transaction_id: TX_ID, allocations: [{ kind: 'customer_invoice', invoice_id: INV_ID, amount: 100 }] },
+        'company-1',
+        'user-1',
+        supabase as never,
+        { type: 'api_key' } as never,
+      ),
+    ).rejects.toThrow(/Customer allocations require an income transaction/)
+  })
+})
@@ -204,7 +204,49 @@ describe('reconciliation MCP tools', () => {
    ])
  })

-  it('reconcile_match refuses an empty request and a request with nothing linkable', async () => {
+  it('reconcile_match keeps an N:1 group as ONE staged pair (feedback seq 292682: Skatteverket rows against one 1630 verifikat)', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    const row2 = '77777777-7777-4777-8777-777777777777'
+    const otherRow = '66666666-6666-4666-8666-666666666666'
+    const entry2 = '44444444-4444-4444-8444-444444444444'
+    // The dry run flattens a pair into one link per outside row and carries
+    // no allocated_amount for a non-split pair.
+    matchMock.mockResolvedValue({
+      dry_run: true,
+      considered: 2,
+      applied: [
+        { external_id: ROW, journal_entry_id: ENTRY },
+        { external_id: row2, journal_entry_id: ENTRY },
+        { external_id: otherRow, journal_entry_id: entry2 },
+      ],
+      skipped: [],
+    })
+    const out = (await tool('gnubok_reconcile_match').execute(
+      {
+        account_key: 'skattekonto',
+        pairs: [
+          { external_ids: [ROW, row2], journal_entry_ids: [ENTRY] },
+          { external_ids: [otherRow], journal_entry_ids: [entry2] },
+        ],
+        dry_run: true,
+      },
+      COMPANY,
+      USER,
+      supabase as never,
+      { type: 'api_key', id: 'key-1' } as never,
+    )) as Record<string, unknown>
+    const preview = out.preview as Record<string, unknown>
+    // Staging the group as two 1:1 pairs made the executor ask each row alone
+    // to settle the whole verifikat (PAIR_NOT_CLOSED on both). The group must
+    // reach the executor as the all-or-nothing pair the reviewer approved.
+    expect(preview.pair_count).toBe(2)
+    expect(preview.pairs).toEqual([
+      { external_ids: [ROW, row2], journal_entry_ids: [ENTRY] },
+      { external_ids: [otherRow], journal_entry_ids: [entry2] },
+    ])
+  })
+
+  it('reconcile_match refuses an empty request and a request with nothing linkable, naming the skip reason', async () => {
    const { supabase } = createQueuedMockSupabase()
    await expect(
      tool('gnubok_reconcile_match').execute({ account_key: 'skattekonto' }, COMPANY, USER, supabase as never),
@@ -218,6 +260,28 @@ describe('reconciliation MCP tools', () => {
        supabase as never,
      ),
    ).rejects.toThrow(/nothing to stage/i)
+    // A bank 1:N whose dry run legitimately failed used to surface as a bare
+    // "No linkable pairs"; the skipped list holds the actual reason.
+    matchMock.mockResolvedValue({
+      dry_run: true,
+      considered: 1,
+      applied: [],
+      skipped: [
+        {
+          pair: { external_ids: [ROW], journal_entry_ids: [ENTRY] },
+          code: 'PAIR_NOT_CLOSED',
+          message: 'Verifikationen saknar rad på 1940',
+        },
+      ],
+    })
+    await expect(
+      tool('gnubok_reconcile_match').execute(
+        { account_key: 'skattekonto', pairs: [{ external_ids: [ROW], journal_entry_ids: [ENTRY] }] },
+        COMPANY,
+        USER,
+        supabase as never,
+      ),
+    ).rejects.toThrow(/nothing to stage\. Skipped: PAIR_NOT_CLOSED: Verifikationen saknar rad på 1940/)
  })

  it('reconcile_unmatch dry-run returns the low-risk staging preview', async () => {
@@ -64,6 +64,43 @@ describe('gnubok_create_voucher: staging gates', () => {
    ).rejects.toThrow(/not balanced/i)
  })

+  it('names the missing amount keys instead of coercing an unknown line shape to 0/0 (feedback seq 318571)', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    // Four "balanced" formats an agent actually sent: none names
+    // debit_amount/credit_amount, so every one used to read as
+    // "debits 0 SEK, credits 0 SEK" from the balance check.
+    for (const lines of [
+      [{ account_number: '6110', debit: 500 }, { account_number: '1930', credit: 500 }],
+      [{ account_number: '6110', amount: 500, side: 'debit' }, { account_number: '1930', amount: 500, side: 'credit' }],
+      [{ account_number: '6110', debitAmount: 500 }, { account_number: '1930', creditAmount: 500 }],
+      [{ account_number: '6110', amount: 500 }, { account_number: '1930', amount: -500 }],
+    ]) {
+      await expect(
+        createVoucher.execute(
+          { entry_date: '2026-05-12', description: 'shape', lines },
+          'company-1',
+          'user-1',
+          supabase as never,
+        ),
+      ).rejects.toThrow(/lines\[0\]: expected debit_amount and\/or credit_amount.*got .*Example/)
+    }
+    await expect(
+      createVoucher.execute(
+        {
+          entry_date: '2026-05-12',
+          description: 'shape',
+          lines: [
+            { account_number: '6110', debit_amount: '500 kr' },
+            { account_number: '1930', credit_amount: 500 },
+          ],
+        },
+        'company-1',
+        'user-1',
+        supabase as never,
+      ),
+    ).rejects.toThrow(/lines\[0\]\.debit_amount must be a number in SEK; got "500 kr"/)
+  })
+
  it('rejects when an explicit fiscal_period_id is closed', async () => {
    const { supabase, enqueue } = createQueuedMockSupabase()
    // fiscal_periods fetch returns a closed period
@@ -655,6 +692,24 @@ describe('gnubok_correct_entry: registration', () => {
    ).rejects.toThrow(/not balanced/i)
  })

+  it('names the missing amount keys on replacement lines instead of coercing to 0/0', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    await expect(
+      correctEntry.execute(
+        {
+          entry_id: 'je-1',
+          lines: [
+            { account_number: '2645', debit: 250 },
+            { account_number: '2614', credit: 250 },
+          ],
+        },
+        'company-1',
+        'user-1',
+        supabase as never,
+      ),
+    ).rejects.toThrow(/lines\[0\]: expected debit_amount and\/or credit_amount.*got debit/)
+  })
+
  it('shows preserved currency, tax, and dimension metadata in the correction preview', async () => {
    const { supabase, enqueue } = createQueuedMockSupabase()
    enqueue({ data: { dimensions_enabled: false }, error: null })