fix(mcp,bookkeeping): FX-safe mapping-rule VAT + honest nullable output schemas (#1846)

Follow-up sweep to #1842 for sibling bugs of both classes. FX denomination (same defect as categorize's vat_amount): - mapping-engine buildResult computed input VAT, reverse-charge fiktiv moms AND the rutor 20-24 basbelopp pair from the transaction-currency amount while journal lines are SEK. Now resolved through the same lenient SEK ladder buildTransactionEntryLines uses for the gross. Reachable at the ledger only via env-gated auto-book (dev/test) and the mapping-rules evaluate API, but one flag from production. - categorize-core's header doc claimed transaction.amount already IS SEK; rewritten, it was license for exactly this bug class. Output-schema nullability (same defect as matched_supplier_id): - get_payslip.calculation_breakdown: returned as explicit null before calculation (the schema description even said so) but typed object. - get_document_content.mime_type/size_bytes: verbatim nullable columns. - export_sie: company_name nullable, and org_number was returned but never declared under additionalProperties:false, so strict clients failed EVERY successful export. - currency (two transaction listings) and voucher_series (verifikat listing): nullable columns with defaults, loosened defensively. - New declaration-pinning test in output-schema.test.ts. Known and deliberately not fixed here: skatteverket bank-counterpart hint compares a foreign amount to SEK (display-only, missed hint); own-account-detector cross-currency magnitude tie-break (heuristic); invoice-inbox still withholds rule proposals on foreign rows (guard can be lifted separately now that the defect it guards is fixed). Claude-Session: https://claude.ai/code/session_01ScVhg6XsDtNXkiEQNV7LaZ Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-24 15:14:21 +02:00
parent 0676f5a564
commit 0ed26c2eca
6 changed files with 188 additions and 19 deletions
@@ -1183,3 +1183,4 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. Appended by agents and
 [2026-08-23] Reconciliation sign-off (PR 4) is an append-only attestation table (account_reconciliations) with a reopen stamp, not a flag on the account: who signed what through which date, with the numbers as they stood, is the thing an auditor and the Hem row read, so it must survive a later change of mind. Sign-off is refused with an unexplained difference unless forced with a note (the note is what the next reader sees). Separate scope reconciliation:signoff (write is not enough): an integration that links rows should not be able to attest. The worklist category reconciliation_due is gated on adoption (zero until the company has signed anything off) so the nudge reaches the people who reconcile monthly without becoming a new chore for everyone. Webhook events added additively without bumping API_V1_VERSION: the dated version is reserved for breaking changes; a new event type breaks no existing subscriber.
 [2026-08-23] Reconciliation agent surfaces (PR 5): the Hem notice for a skattekonto that disagrees with the ledger reads a summary the sync persists (extension_data skattekonto_reconciliation_latest) instead of recomputing the bridge on every render; the same persisted summary feeds nothing else yet. The attention resource's reconciliation_due category and the Hem row share lib/worklist countReconciliationDue (one predicate). Manual N:M matching with residual booking, dropping the local MatchDialog on /skattekonto, restyling the bank view and eval scenarios are deferred to PR 6: they need the two-pane UI and a visual pass, and none of the agent surfaces depend on them. The skattekonto sync cron now orders eligible companies by stalest sync before its 50-per-run cap (never-synced first) instead of raising the cap: a fixed order plus a cap starved the tail.
 [2026-08-24] vat_amount (categorize/bulk_book) is transaction-currency, converted to SEK at booking: the validation bound already read it in transaction currency (the underlag's denomination), and the gross line already converts through resolveSekAmount, so converting the VAT the same way was the only coherent option. Documenting it as SEK instead (the reporter's first suggestion, feedback seq 254607) would force agents to pre-convert with a settlement rate they cannot see.
+[2026-08-24] Declared currency/voucher_series nullable in three MCP listing schemas on column-nullability alone (no traced null producer): loosening an output schema can only stop false validation failures, never cause one, and legacy rows predate the columns' defaults. Declined (for now) a full Ajv execute-vs-schema round-trip harness in output-schema.test.ts: right long-term answer to this bug class, but a session-sized project of its own; the audit's seven confirmed sites are pinned by a targeted declaration test instead.
@@ -35,3 +35,44 @@ describe('annotation correctness', () => {
    expect(feedback?.annotations?.readOnlyHint).toBe(false)
  })
 })
+
+describe('nullable output properties stay declared nullable', () => {
+  // 2026-08-24 audit (sibling of the matched_supplier_id fix, PR #1842):
+  // these properties are emitted with null on real code paths (explicit
+  // `?? null`, or verbatim passthrough of a nullable column). A bare
+  // { type: 'string' } makes strict clients fail SUCCESSFUL calls, and the
+  // repeated "failures" trip the caller's circuit breaker. This test pins
+  // the declarations so a schema edit cannot silently regress them.
+  const NULLABLE: Array<[tool: string, prop: string]> = [
+    ['gnubok_get_payslip', 'calculation_breakdown'],
+    ['gnubok_get_document_content', 'mime_type'],
+    ['gnubok_get_document_content', 'size_bytes'],
+    ['gnubok_export_sie', 'company_name'],
+    ['gnubok_export_sie', 'org_number'],
+    ['gnubok_complete_document_upload', 'matched_supplier_id'],
+    ['gnubok_upload_document', 'matched_supplier_id'],
+  ]
+
+  it.each(NULLABLE)('%s.%s allows null and is not required', (toolName, prop) => {
+    const tool = tools.find((t) => t.name === toolName)
+    expect(tool, toolName).toBeDefined()
+    const schema = tool!.outputSchema as {
+      properties: Record<string, { type: unknown }>
+      required?: string[]
+    }
+    const declared = schema.properties[prop]
+    expect(declared, `${toolName}.${prop} must be declared (additionalProperties is false)`).toBeDefined()
+    expect(declared.type, `${toolName}.${prop}`).toContain('null')
+    expect(schema.required ?? []).not.toContain(prop)
+  })
+
+  it('gnubok_export_sie declares every key its execute returns', () => {
+    // The tool returned an undeclared org_number under
+    // additionalProperties: false, failing strict validation on EVERY call.
+    const tool = tools.find((t) => t.name === 'gnubok_export_sie')!
+    const props = Object.keys((tool.outputSchema as { properties: Record<string, unknown> }).properties)
+    for (const key of ['content', 'byte_size', 'fiscal_period_id', 'company_name', 'org_number', 'generated_at']) {
+      expect(props, key).toContain(key)
+    }
+  })
+})
@@ -4205,7 +4205,7 @@ export const tools: McpTool[] = [
        date: { type: 'string' },
        description: { type: 'string' },
        amount: { type: 'number' },
-        currency: { type: 'string' },
+        currency: { type: ['string', 'null'] },
        merchant_name: { type: ['string', 'null'] },
        reference: { type: ['string', 'null'] },
        is_business: { type: ['boolean', 'null'] },
@@ -4319,7 +4319,7 @@ export const tools: McpTool[] = [
        date: { type: 'string' },
        description: { type: 'string' },
        amount: { type: 'number' },
-        currency: { type: 'string' },
+        currency: { type: ['string', 'null'] },
        merchant_name: { type: ['string', 'null'] },
        reference: { type: ['string', 'null'] },
        is_business: { type: ['boolean', 'null'] },
@@ -4396,7 +4396,7 @@ export const tools: McpTool[] = [
      additionalProperties: false,
      properties: {
        journal_entry_id: { type: 'string' },
-        voucher_series: { type: 'string' },
+        voucher_series: { type: ['string', 'null'] },
        voucher_number: { type: 'number' },
        entry_date: { type: 'string' },
        description: { type: 'string' },
@@ -11161,8 +11161,8 @@ export const tools: McpTool[] = [
      properties: {
        document_id: { type: 'string' },
        file_name: { type: 'string' },
-        mime_type: { type: 'string' },
-        size_bytes: { type: 'number' },
+        mime_type: { type: ['string', 'null'] },
+        size_bytes: { type: ['number', 'null'] },
        signed_url: { type: 'string' },
        expires_at: { type: 'string' },
      },
@@ -12742,7 +12742,7 @@ export const tools: McpTool[] = [
        overrides: { type: 'object', description: 'Manual tax/avgifter overrides + reason (effective = override ?? calculated)' },
        absence_days: { type: 'object', description: 'Sick/vab/parental/vacation day counts' },
        line_items: { type: 'array', items: { type: 'object' }, description: 'Each with salary_line_item_id' },
-        calculation_breakdown: { type: 'object', description: 'Step-by-step engine breakdown; null until calculated' },
+        calculation_breakdown: { type: ['object', 'null'], description: 'Step-by-step engine breakdown; null until calculated' },
      },
      required: ['salary_run_employee_id', 'salary_run_id', 'employee_id', 'employee_name', 'personnummer_masked', 'amounts', 'line_items'],
    },
@@ -13852,7 +13852,8 @@ export const tools: McpTool[] = [
        content: { type: 'string' },
        byte_size: { type: 'number' },
        fiscal_period_id: { type: 'string' },
-        company_name: { type: 'string' },
+        company_name: { type: ['string', 'null'] },
+        org_number: { type: ['string', 'null'] },
        generated_at: { type: 'string' },
      },
    },
@@ -559,6 +559,123 @@ describe('mapping-engine', () => {
      expect(result.vat_lines[0].account_number).toBe('2645')
      expect(result.vat_lines[1].account_number).toBe('2614')
    })
+
+    // Journal entry lines are SEK; VAT from a rule match must be derived from
+    // the transaction's SEK value, not the raw foreign amount. Sibling of the
+    // buildMappingResultFromCategory FX fix (PR #1842).
+    it('derives rule input VAT from the SEK value for a foreign transaction', async () => {
+      const { evaluateMappingRules } = await import('../mapping-engine')
+
+      const tx = makeTransaction({
+        amount: -100,
+        currency: 'EUR',
+        exchange_rate: 11,
+        merchant_name: 'Adobe',
+        description: 'ADOBE SYSTEMS',
+      })
+
+      mockResult({
+        data: [
+          {
+            id: 'rule-fx-vat',
+            user_id: 'user-1',
+            rule_name: 'Adobe standard VAT',
+            rule_type: 'merchant_name',
+            priority: 10,
+            mcc_codes: null,
+            merchant_pattern: 'Adobe',
+            description_pattern: null,
+            amount_min: null,
+            amount_max: null,
+            debit_account: '5420',
+            credit_account: '1930',
+            vat_treatment: 'standard_25',
+            vat_debit_account: '2641',
+            vat_credit_account: null,
+            risk_level: 'LOW',
+            default_private: false,
+            requires_review: false,
+            confidence_score: 0.9,
+            capitalization_threshold: null,
+            capitalized_debit_account: null,
+            is_active: true,
+            source: 'system',
+            user_description: null,
+            template_id: null,
+            created_at: '2024-01-01',
+            updated_at: '2024-01-01',
+          },
+        ],
+        error: null,
+      })
+
+      const result = await evaluateMappingRules(mockSupabase as never, 'user-1', tx)
+
+      // 100 EUR at 11 = 1100 kr gross; 25% extraction = 220 kr, not 20
+      expect(result.vat_lines).toHaveLength(1)
+      expect(result.vat_lines[0].account_number).toBe('2641')
+      expect(result.vat_lines[0].debit_amount).toBe(220)
+    })
+
+    it('derives reverse-charge fiktiv moms and basbelopp from the SEK value (amount_sek wins)', async () => {
+      const { evaluateMappingRules } = await import('../mapping-engine')
+
+      const tx = makeTransaction({
+        amount: -100,
+        currency: 'USD',
+        // bank settlement wins over exchange_rate, matching the gross line
+        amount_sek: -950,
+        exchange_rate: 10,
+        merchant_name: 'AWS',
+        description: 'AWS EU-WEST-1',
+      })
+
+      mockResult({
+        data: [
+          {
+            id: 'rule-rc-fx',
+            user_id: 'user-1',
+            rule_name: 'AWS reverse charge',
+            rule_type: 'merchant_name',
+            priority: 10,
+            mcc_codes: null,
+            merchant_pattern: 'AWS',
+            description_pattern: null,
+            amount_min: null,
+            amount_max: null,
+            debit_account: '5421',
+            credit_account: '1930',
+            vat_treatment: 'reverse_charge',
+            vat_debit_account: null,
+            vat_credit_account: null,
+            risk_level: 'LOW',
+            default_private: false,
+            requires_review: false,
+            confidence_score: 0.9,
+            capitalization_threshold: null,
+            capitalized_debit_account: null,
+            is_active: true,
+            source: 'system',
+            user_description: null,
+            template_id: null,
+            created_at: '2024-01-01',
+            updated_at: '2024-01-01',
+          },
+        ],
+        error: null,
+      })
+
+      const result = await evaluateMappingRules(mockSupabase as never, 'user-1', tx)
+
+      // 950 kr gross: fiktiv moms 237.50, basbelopp 950 (not 25/100 off USD)
+      expect(result.vat_lines).toHaveLength(4)
+      expect(result.vat_lines[0].account_number).toBe('2645')
+      expect(result.vat_lines[0].debit_amount).toBe(237.5)
+      expect(result.vat_lines[1].account_number).toBe('2614')
+      expect(result.vat_lines[1].credit_amount).toBe(237.5)
+      expect(result.vat_lines[2].debit_amount).toBe(950)
+      expect(result.vat_lines[3].credit_amount).toBe(950)
+    })
  })

  // Both the halva-prisbasbeloppet threshold (IL 18 kap 4 §) and a rule's
@@ -4,6 +4,9 @@ import {
  generateReverseChargeLines,
  generateReverseChargeBasisLines,
 } from './vat-entries'
+// Aliased: this file already has a local resolveSekAmountOrNull(transaction)
+// for account-selection decisions, which refuses when unconvertible.
+import { resolveSekAmount as resolveSekAmountLenient } from './currency-utils'
 import { findMatchingTemplates, buildMappingResultFromTemplate } from './booking-templates'
 import {
  findCounterpartyTemplate,
@@ -296,12 +299,15 @@ function matchesRule(rule: MappingRule, transaction: Transaction): boolean {
 * Build a MappingResult from a matched rule
 */
 function buildResult(rule: MappingRule, transaction: Transaction, entityType?: EntityType): MappingResult {
-  // NOTE: this is the amount in the transaction's own currency. It still feeds
-  // the VAT line generation below, which understates ingående moms on non-SEK
-  // rows (a known separate defect, tracked on its own: fixing it changes
-  // posted VAT amounts). The capitalization check below deliberately does not
-  // use it: an account-selection decision must run on a SEK value.
-  const absAmount = Math.abs(transaction.amount)
+  // VAT figures land on journal entry lines, which are always SEK, so they
+  // are derived from the SEK value of the transaction. The LENIENT resolver
+  // is deliberate: buildTransactionEntryLines resolves the gross with the
+  // same ladder, so the VAT lines and the bank leg can never disagree (a
+  // rateless legacy row degrades to today's behavior on both sides instead
+  // of unbalancing the net line). Mirrors buildMappingResultFromCategory.
+  const absSekAmount = Math.abs(resolveSekAmountLenient(
+    transaction.amount, transaction.amount_sek, transaction.currency, transaction.exchange_rate
+  ))
  const isExpense = transaction.amount < 0

  let debitAccount = rule.debit_account || (isExpense ? '6991' : '1930')
@@ -355,7 +361,7 @@ function buildResult(rule: MappingRule, transaction: Transaction, entityType?: E
      // FK004. Mapping rules don't carry supplier-country today, so we
      // default to EU services: the most common reverse-charge scenario.
      const rcRate = 0.25
-      const rcLines = generateReverseChargeLines(absAmount, rcRate, false)
+      const rcLines = generateReverseChargeLines(absSekAmount, rcRate, false)
      for (const rcl of rcLines) {
        vatLines.push({
          account_number: rcl.account_number,
@@ -367,7 +373,7 @@ function buildResult(rule: MappingRule, transaction: Transaction, entityType?: E

      // Skip basbelopp emission if the rule already books to a basis account.
      if (!/^4[45]\d{2}$/.test(debitAccount)) {
-        const basisLines = generateReverseChargeBasisLines(absAmount, rcRate, 'eu_business')
+        const basisLines = generateReverseChargeBasisLines(absSekAmount, rcRate, 'eu_business')
        for (const bl of basisLines) {
          vatLines.push({
            account_number: bl.account_number,
@@ -382,7 +388,7 @@ function buildResult(rule: MappingRule, transaction: Transaction, entityType?: E
        rule.vat_treatment === 'standard_25' ? 0.25
        : rule.vat_treatment === 'reduced_12' ? 0.12
        : 0.06
-      const vatLine = generateInputVatLine(absAmount, vatRate)
+      const vatLine = generateInputVatLine(absSekAmount, vatRate)
      if (vatLine) {
        vatLines.push({
          account_number: vatLine.account_number,
@@ -17,9 +17,12 @@
 * reverse-charge VAT, and the propagation step below attaches the underlag to
 * the new verifikation (BFL 7 kap) and stamps the inbox item resolved.
 *
- * Booking is always in SEK off the bank transaction's own amount (BFL 5 kap
- * 2§), so the foreign-currency underlag never needs an FX step here: the bank
- * already settled it.
+ * Journal entry lines are always SEK (BFL 5 kap 2§), but transaction.amount is
+ * denominated in transaction.currency: the SEK resolution happens inside the
+ * mapping builders and buildTransactionEntryLines (amount_sek / exchange_rate,
+ * see lib/bookkeeping/currency-utils.ts), never off the raw amount. The
+ * foreign-currency underlag needs no extra FX step here because those two
+ * resolve it, not because the amount already is kronor.
 */
 import type { SupabaseClient } from '@supabase/supabase-js'
 import { eventBus } from '@/lib/events'