diff --git a/DECISIONS.md b/DECISIONS.md index bc9a20e9..4aca68bb 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1511,6 +1511,7 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-09-02] Agent-triggerable bank sync shipped (v1 POST /bank-connections/{id}/sync + MCP gnubok_sync_bank), lifting the 2026-09-01 deferral: Emil chose to close every open F2 item in one PR. The cost worry is bounded structurally instead of by policy: the window is never caller-controlled (gap-aware 7 to 90 days, same helper as the cron), a connection synced within 15 minutes answers BANK_SYNC_COOLDOWN with next_allowed_at (MCP returns it in-band as synced=false so agents read on instead of retrying), and failures are throttled per process by attempt time. The web Synka-nu route is left untouched rather than refactored onto the shared runner: it carries UI-only behaviour (caller-chosen days_back up to 365, SIE sweep stamping) and a regression there would hit every user for a code-sharing win. [2026-09-02] Bank-sync cooldown is a durable lease column (bank_connections.sync_lease_until, migration 20260902150000) claimed with one conditional UPDATE, not a process-local attempt map: the security scan on PR #2165 showed the map is bypassed by a second serverless instance or a cold start, so two agent calls could each bill Enable Banking. A column add was chosen over reusing extension_data because PostgREST cannot express an atomic conditional upsert there; the nightly cron deliberately ignores the lease. [2026-09-02] Grok links carry auth=required like the claude.ai link (#2159), decided from a live test: on the lazy URL Grok's connector dialog listed all 150+ tools and never opened the sign-in, so it reads the 200 probe as an authless server exactly as claude.ai does. The flag lives in one helper (mcpServerUrl / sideDoorServerUrl in lib/onboarding/checklist.ts) so the settings row, the onboarding side door and the deep link cannot drift; ChatGPT stays lazy because its developer mode honours the 401 on the first protected call. +[2026-09-02] reconcile_match staging folds N:1 groups back together by journal_entry_id (links from the dry run that share a verifikat and carry no allocated_amount), mirroring the existing bank 1:N fold, instead of threading the caller's original pairs through: the dry run is the source of truth for what was validated, a verifikat can only be settled once so shared-JE links can only have come from one N:1 pair, and use_proposals has no caller pairs to thread. The remaining gap (dry run validates only pair shape for non-split pairs) is the engine's to close, not the tool's. [2026-09-02] parties children/roles reference parties(id, company_id) with composite FKs, not parties(id): a party UUID from another tenant is rejected by construction instead of relying on each writer to check; ON DELETE SET NULL (party_id) on customers/suppliers because a plain SET NULL would null company_id too (Superagent P2 on #2162) [2026-09-02] Party suggestions attach only by explicit party_id, org number or an exact ledger key already in alias_keys; same-core text is reported as similar_to for a person to decide and identities are withheld when a key mixes org numbers: the selection eval measured 9% false merges on trade names shared by distinct legal entities (Fortnox AB / Fortnox Finans), so text never merges [2026-09-02] Edited migration 20260902160000 after merge: its backfill failed on prod (ensure_party: name is required; 3 nameless rows) so it was never applied there, the Supabase main branch sat in MIGRATIONS_FAILED and every later migration was blocked behind it. An unapplied file is not a shipped schema; a follow-up migration could not run before it diff --git a/extensions/general/mcp-server/__tests__/categorize-already-booked.test.ts b/extensions/general/mcp-server/__tests__/categorize-already-booked.test.ts new file mode 100644 index 00000000..83153a97 --- /dev/null +++ b/extensions/general/mcp-server/__tests__/categorize-already-booked.test.ts @@ -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}\\)`)) + }) +}) diff --git a/extensions/general/mcp-server/__tests__/match-batch-allocate-kind.test.ts b/extensions/general/mcp-server/__tests__/match-batch-allocate-kind.test.ts new file mode 100644 index 00000000..88839a3b --- /dev/null +++ b/extensions/general/mcp-server/__tests__/match-batch-allocate-kind.test.ts @@ -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/) + }) +}) diff --git a/extensions/general/mcp-server/__tests__/reconciliation-tools.test.ts b/extensions/general/mcp-server/__tests__/reconciliation-tools.test.ts index 6b7e677d..750a5fa2 100644 --- a/extensions/general/mcp-server/__tests__/reconciliation-tools.test.ts +++ b/extensions/general/mcp-server/__tests__/reconciliation-tools.test.ts @@ -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 + const preview = out.preview as Record + // 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 () => { diff --git a/extensions/general/mcp-server/__tests__/voucher-tools.test.ts b/extensions/general/mcp-server/__tests__/voucher-tools.test.ts index 6b837b11..76f0db26 100644 --- a/extensions/general/mcp-server/__tests__/voucher-tools.test.ts +++ b/extensions/general/mcp-server/__tests__/voucher-tools.test.ts @@ -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 }) diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index f86653ad..9094cab3 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -1523,6 +1523,35 @@ async function categorizeTransactionCore( } } +/** + * A voucher line that names neither debit_amount nor credit_amount is a shape + * error, not a zero line. `Number(undefined) || 0` silently turned + * {debit: 500}, {amount, side} and {debitAmount: 500} into 0/0, and the + * balance check then reported "debits 0 SEK, credits 0 SEK" for four + * perfectly balanced formats (feedback seq 318571). No host validates + * inputSchema at runtime, so the guard lives here, and it names the keys it + * got so the agent can fix the shape in one turn instead of guessing at the + * amounts. + */ +function assertVoucherLineShape(line: Record, label: string): void { + const hasDebit = line.debit_amount !== undefined && line.debit_amount !== null + const hasCredit = line.credit_amount !== undefined && line.credit_amount !== null + if (!hasDebit && !hasCredit) { + const got = Object.keys(line).filter((k) => k !== 'account_number') + throw new Error( + `${label}: expected debit_amount and/or credit_amount (numbers in SEK)` + + (got.length ? `; got ${got.join(', ')}` : '; got neither') + + '. Example: {"account_number":"6110","debit_amount":400}, {"account_number":"1930","credit_amount":400}.', + ) + } + for (const key of ['debit_amount', 'credit_amount'] as const) { + const v = line[key] + if (v !== undefined && v !== null && !Number.isFinite(Number(v))) { + throw new Error(`${label}.${key} must be a number in SEK; got ${JSON.stringify(v)}`) + } + } +} + // ── Output schema helpers ──────────────────────────────────── const PAGINATION_PROPS = { @@ -5696,10 +5725,18 @@ export const tools: McpTool[] = [ false // preview mode: execution happens at approval time via gnubok_approve_pending_operation ) - // If already has a journal entry, pass through as-is + // Already booked: categorizeTransactionCore returns a success-shaped + // object here, which fails STAGED_OPERATION_SCHEMA on strict clients and + // reached the agent as "Structured content does not match the tool's + // output schema" with the real reason swallowed (feedback seq 288574). + // Throw instead: the dispatcher's isError path carries no + // structuredContent, so the message survives. if (result.success && result.journal_entry_created === false) { - const { transaction: _tx, ...publicResult } = result - return publicResult + throw new Error( + `Transaction is already booked (journal_entry_id ${result.journal_entry_id ?? 'unknown'}); ` + + 'nothing to categorize. Use gnubok_list_uncategorized_transactions to find unbooked ones, ' + + 'or gnubok_correct_entry / gnubok_reverse_journal_entry to change the existing verifikat.', + ) } // Fetch transaction description (and date for period_status) for the title @@ -10693,6 +10730,21 @@ export const tools: McpTool[] = [ if (!Array.isArray(allocations) || allocations.length === 0) { throw new Error('allocations is required (non-empty array)') } + // kind is the key every guard below branches on (direction, required + // id, tenant pre-check). 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 + // reject here, and say which id field goes with which kind. + for (const [i, a] of allocations.entries()) { + if (a.kind !== 'customer_invoice' && a.kind !== 'supplier_invoice') { + throw new Error( + `allocations[${i}].kind is required: "customer_invoice" (incoming payment, pass invoice_id) ` + + `or "supplier_invoice" (outgoing payment, pass supplier_invoice_id)` + + (a.kind === undefined ? '' : `; got ${JSON.stringify(a.kind)}`), + ) + } + } const { data: transaction, error: txError } = await supabase .from('transactions') @@ -12149,15 +12201,22 @@ export const tools: McpTool[] = [ { dryRun: true }, ) if (!preview) throw new Error(`Unknown account_key "${accountKey}" for this company`) - // Rebuild the staged pairs from the preview: 1:1 links stay one pair - // each; the links of a bank 1:N split (they carry allocated_amount, all - // on the same row) fold back into ONE pair with explicit allocations, so - // the executor re-validates the exact slices the reviewer approved. + // Rebuild the staged pairs from the preview. The dry run flattens every + // pair into one link per outside row, so the grouping must be put back: + // the links of an N:1 pair (several rows, one verifikat, no + // allocated_amount) fold into ONE pair on their verifikat, and the links + // of a bank 1:N split (one row, allocated_amount per verifikat) fold into + // ONE pair with explicit allocations. Staging them as N separate 1:1 + // pairs made the executor ask each Skatteverket row alone to settle the + // whole 1630 verifikat: PAIR_NOT_CLOSED on every "Avdragen skatt" + + // "Arbetsgivaravgift" pair whose sum matched exactly (feedback seq + // 292682, 36 rows / 18 verifikat). const resolvedPairs: Array<{ external_ids: string[] journal_entry_ids: string[] allocations?: Array<{ journal_entry_id: string; amount: number }> }> = [] + const rowsByEntry = new Map() const splitByRow = new Map>() for (const a of preview.applied) { if (typeof a.allocated_amount === 'number') { @@ -12166,7 +12225,12 @@ export const tools: McpTool[] = [ splitByRow.set(a.external_id, slices) continue } - resolvedPairs.push({ external_ids: [a.external_id], journal_entry_ids: [a.journal_entry_id] }) + const rows = rowsByEntry.get(a.journal_entry_id) ?? [] + if (!rows.includes(a.external_id)) rows.push(a.external_id) + rowsByEntry.set(a.journal_entry_id, rows) + } + for (const [journalEntryId, externalIds] of rowsByEntry) { + resolvedPairs.push({ external_ids: externalIds, journal_entry_ids: [journalEntryId] }) } for (const [externalId, slices] of splitByRow) { resolvedPairs.push({ @@ -12176,7 +12240,15 @@ export const tools: McpTool[] = [ }) } if (resolvedPairs.length === 0) { - throw new Error('No linkable pairs: nothing to stage') + // The dry run's skipped list holds the actual reason; without it the + // agent saw only "No linkable pairs" (feedback seq 292682). + const reasons = preview.skipped + .slice(0, 5) + .map((sk) => `${sk.code}: ${sk.message}`) + throw new Error( + 'No linkable pairs: nothing to stage' + + (reasons.length ? `. Skipped: ${reasons.join(' | ')}` : ''), + ) } return stagePendingOperation( @@ -19005,6 +19077,9 @@ export const tools: McpTool[] = [ } // Normalize so validateBalance + preview see consistent numeric types. + // Shape first: a line with neither amount key is a schema mismatch and + // must say so, never coerce to 0/0 and fail the balance check instead. + for (const [i, l] of rawLines.entries()) assertVoucherLineShape(l, `lines[${i}]`) const lines = rawLines.map((l, i) => ({ account_number: String(l.account_number ?? ''), debit_amount: Number(l.debit_amount) || 0, @@ -19320,6 +19395,7 @@ export const tools: McpTool[] = [ throw new Error('entry_id and at least two lines are required') } + for (const [i, l] of rawLines.entries()) assertVoucherLineShape(l, `lines[${i}]`) const lines = rawLines.map((l, i) => ({ account_number: String(l.account_number ?? ''), debit_amount: Number(l.debit_amount) || 0,