diff --git a/DECISIONS.md b/DECISIONS.md index 2e5121b6..81861706 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1337,3 +1337,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-28] /migrate SIE guard skips company-info-only runs (all entity flags false) and the wizard derives "SIE already imported" from the preview OR this session's successful /import-sie results: company info writes no accounts, balances or subledger rows, so the BFL rationale does not apply; and the one-shot preview went stale after phase 1 succeeded and phase 2 failed, falsely blocking an entities-only retry (#2000 review). [2026-08-28] get_vat_ruta_source_lines (the VAT ruta drill-down) now applies the same four exclusions as get_vat_declaration_totals (the filed figure): posted closing entries, source_type 'vat_settlement', the two kontantmetod year-end reversals, and settlement-SHAPED entries (a line on a ruta account plus a line on 2650/1650). It previously filtered on company, status and date only, so expanding a ruta listed verifikat that are not in the number it claims to explain, with no total on the panel to reveal the mismatch. Measured on prod 2026-08-28: 322 posted/reversed entries carrying 26xx lines across 214 companies sit in those excluded classes. A momsdeklaration is räkenskapsinformation (BFL 5 kap.) and this drill-down is what substantiates a filed figure, so the two must agree exactly. The exclusion CTEs are lifted VERBATIM from the figure rather than re-derived: any divergence reintroduces exactly this bug, and an identical copy is easy to diff when the figure changes. Settlement-shape is detected against journal_entry_lines directly instead of through the figure's vat_lines CTE, which is EQUIVALENT not a shortcut (p_ruta_accounts = VAT_ACCOUNTS and p_net_accounts = ['2650','1650'] are both strict subsets of the figure's p_accounts, so restricting to vat_lines first cannot change which entries match); that keeps p_accounts meaning "the accounts of the ruta being expanded" without a fourth account parameter. opening_balance entries are deliberately NOT excluded: the figure exempts them from `shaped`, which keeps their lines IN the totals, so dropping them here would break the equality in the other direction (pinned by its own test). VAT_ACCOUNTS is now exported from lib/reports/vat-declaration.ts so the route detects shape from the same list the figure uses; a second copy is what let the two disagree. DROP + CREATE OR REPLACE, not CREATE OR REPLACE alone: the signature gains p_ruta_accounts/p_net_accounts and adding parameters registers a second overload PostgREST cannot choose between (trap documented in 20260421140000); OR REPLACE on the new arity keeps the file re-runnable. Verified the new pg test actually catches the bug by reinstalling the old body and watching 3 of 4 tests fail with the real misreporting (2611: drill-down 250/240 vs figure 0/200), then restoring. [2026-08-28] Bankavstamning NULL-link fix scoped to transfer legs with contradicting sign (20260828220000): the naive rule (NULL counts only for the primary account) and the formula-only variant (drop far-leg-settled vouchers from unexplained) were both simulated against prod and rejected; the naive rule worsened 4 of 11 affected cards (worst -37 000 kr false alarm on single-leg vouchers with no user action available), the formula variant blew up healthy cards by up to 474 550 kr. The shipped three-condition rule changes 24 vouchers on 7 cards in 6 companies, all verified per-card. +[2026-08-29] Database errors now keep their SQLSTATE: new lib/errors/db-error.ts (dbError/errorCauseTag), applied at the 54 `throw new Error(\`Database error: ${err.message}\`)` sites in the MCP server AND, far more importantly, at lib/supabase/fetch-all.ts:74 where `throw new Error(error.message)` was the single highest-traffic strip point in the codebase (31 callers; every paginated read). isTransientFailure() checks the driver code FIRST and 57014 (statement timeout) is already in TRANSIENT_SQLSTATES, so discarding it turned a retryable timeout into UNKNOWN_ERROR ("Något gick fel. Försök igen."), which an agent cannot dispatch on. Traced end to end: gnubok_query_journal -> fetchEntryLines -> fetchAllRows (code stripped here) -> the tool's own sanitizeDbError, which ALREADY had a correct TRANSIENT_ERROR branch with a "retry or narrow with date_from/date_to" hint that could never fire because getStructuredError saw an anonymous Error. Measured on prod over 60 days with bot actors excluded: 1 024 real-agent failures, 645 UNKNOWN_ERROR across 60 actors and 57 companies; query_journal failed 164 times at p50 8 110 ms while every other failing tool sat at 1-315 ms; 82 retry streaks, 462 wasted repeat calls, 53.1% of error calls inside a streak. fetch-all passes context=null so the driver message stays VERBATIM (sanitizeDbError and other callers match on the existing text; this change adds the code, it does not reword). Attaching `code` is safe because extractCode() only accepts /^[A-Z_]+$/ and every SQLSTATE/PostgREST code contains digits, so it cannot hijack the application error registry (pinned by a test). dbError also never renders the literal "undefined": a driver-level failure with no message produced "Database error: undefined", the string that made these unsearchable. errorCauseTag() returns a PII-safe SQLSTATE for telemetry; the raw driver message can quote row values in a constraint violation and belongs in the server log, never in event_log. NOT ratcheted: check:types reports 538 vs baseline 539 because main fixed an unrelated error in own-account-detector.test.ts after the baseline was set; the gate only fails on an INCREASE, so the baseline is left alone rather than adding unrelated churn to this diff. diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 2903bc1b..6068006b 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -47,6 +47,7 @@ import { applyAccountOverride } from '@/lib/bookkeeping/account-override' import { ACCOUNT_NUMBER_RE } from '@/lib/invariants/account-number' import { isSlpPensionAccount } from '@/lib/bookkeeping/slp-lines' import { getErrorEntry } from '@/lib/errors/structured-errors' +import { dbError } from '@/lib/errors/db-error' import { getStructuredError } from '@/lib/errors/get-structured-error' import { applySettlementAccount } from '@/lib/bookkeeping/mapping-engine' import { resolveSettlementAccount } from '@/lib/bookkeeping/settlement-account' @@ -1055,7 +1056,7 @@ async function resolveJournalEntryRef( .order('entry_date', { ascending: false }) if (error) { - throw new Error(`Database error resolving voucher "${series}-${number}": ${error.message}`) + throw dbError(error, `Database error resolving voucher "${series}-${number}"`) } const matches = (data ?? []) as Array<{ id: string; entry_date: string; description: string }> @@ -3799,7 +3800,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!data) throw new Error('Company settings not found.') return { @@ -3916,7 +3917,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!current) throw new Error('Company settings not found.') const currentPreview = { @@ -5041,7 +5042,7 @@ export const tools: McpTool[] = [ if (cashAccountId) countQuery = countQuery.eq('cash_account_id', cashAccountId) const { count: totalCount, error: countError } = await countQuery - if (countError) throw new Error(`Database error: ${countError.message}`) + if (countError) throw dbError(countError) let listQuery = supabase .from('transactions') @@ -5055,7 +5056,7 @@ export const tools: McpTool[] = [ .order('date', { ascending: false }) .range(offset, offset + limit - 1) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) // Resolve the bank account's BAS ledger for the rows on this page so a // per-account reconciliation can be driven from outside (customer @@ -5070,7 +5071,7 @@ export const tools: McpTool[] = [ .select('id, ledger_account') .eq('company_id', companyId) .in('id', cashAccountIds) - if (cashError) throw new Error(`Database error: ${cashError.message}`) + if (cashError) throw dbError(cashError) for (const c of (cashRows ?? []) as Array<{ id: string; ledger_account: string }>) { ledgerByCashAccount.set(c.id, c.ledger_account) } @@ -5150,7 +5151,7 @@ export const tools: McpTool[] = [ p_limit: limit, p_offset: offset, }) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const result = data as { ok: boolean @@ -5228,7 +5229,7 @@ export const tools: McpTool[] = [ p_limit: limit, p_offset: offset, }) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const result = data as { ok: boolean @@ -5490,7 +5491,7 @@ export const tools: McpTool[] = [ .order('date', { ascending: false }) .limit(limit) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) return { transactions: data ?? [], @@ -5551,7 +5552,7 @@ export const tools: McpTool[] = [ .range(from, to) }) } catch (error) { - throw new Error(`Database error: ${error instanceof Error ? error.message : 'unknown error'}`) + throw dbError(error) } rows.sort((a, b) => a.name.localeCompare(b.name, 'sv') || a.id.localeCompare(b.id)) @@ -5843,7 +5844,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!current) throw new Error('Customer not found.') // Same guard as gnubok_create_customer and the REST PATCH route: only @@ -5971,7 +5972,7 @@ export const tools: McpTool[] = [ return q.order('id', { ascending: true }).range(from, to) }) } catch (error) { - throw new Error(`Database error: ${error instanceof Error ? error.message : 'unknown error'}`) + throw dbError(error) } articles.sort((a, b) => a.name.localeCompare(b.name, 'sv') || a.id.localeCompare(b.id)) @@ -6157,7 +6158,7 @@ export const tools: McpTool[] = [ .order('id', { ascending: false }) .range(offset, offset + limit) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const rows = data ?? [] const invoices = rows.slice(0, limit).map((inv: Record) => ({ @@ -6285,7 +6286,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!invoice) throw new Error('Invoice not found. Use gnubok_list_invoices to find valid IDs.') type InvoiceLineRow = { @@ -7258,7 +7259,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (invoiceError) throw new Error(`Database error: ${invoiceError.message}`) + if (invoiceError) throw dbError(invoiceError) if (!invoice) throw new Error('Invoice not found') // Never read invoice_deliveries directly: the row carries the exact @@ -7270,7 +7271,7 @@ export const tools: McpTool[] = [ 'list_invoice_delivery_summaries_for_service', { p_company_id: companyId, p_user_id: userId, p_invoice_id: invoiceId }, ) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) // Mirrors the RETURNS TABLE of the RPC. body_html, body_text and // bcc_addresses are absent by construction, not filtered here. @@ -7411,7 +7412,7 @@ export const tools: McpTool[] = [ .range(from, to) }) } catch (error) { - throw new Error(`Database error: ${error instanceof Error ? error.message : 'unknown error'}`) + throw dbError(error) } suppliers.sort((a, b) => a.name.localeCompare(b.name, 'sv') || a.id.localeCompare(b.id)) @@ -7605,7 +7606,7 @@ export const tools: McpTool[] = [ const { data, error } = await query.order('due_date', { ascending: true }).limit(limit) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) return { invoices: data ?? [], count: data?.length ?? 0 } }, @@ -7650,7 +7651,7 @@ export const tools: McpTool[] = [ .order('occurrence_count', { ascending: false }) .limit(limit) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) return { templates: (data ?? []).map((t) => ({ @@ -7710,7 +7711,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .in('id', limitedIds) - if (txError) throw new Error(`Database error: ${txError.message}`) + if (txError) throw dbError(txError) if (!transactions || transactions.length === 0) throw new Error('No transactions found') // Fetch mapping rules @@ -7846,7 +7847,7 @@ export const tools: McpTool[] = [ return query.order('account_number', { ascending: true }).range(from, to) }) } catch (error) { - throw new Error(`Database error: ${error instanceof Error ? error.message : 'unknown error'}`) + throw dbError(error) } // Postgres ordered by sort_order ascending with nulls last; keep that @@ -7917,7 +7918,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .eq('account_number', accountNumber) .maybeSingle() - if (existingErr) throw new Error(`Database error: ${existingErr.message}`) + if (existingErr) throw dbError(existingErr) if (existing) { throw new Error( existing.is_active @@ -8038,7 +8039,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .eq('account_number', accountNumber) .maybeSingle() - if (fetchErr) throw new Error(`Database error: ${fetchErr.message}`) + if (fetchErr) throw dbError(fetchErr) if (!current) { throw new Error(`Konto ${accountNumber} finns inte i kontoplanen. Skapa det med gnubok_create_account.`) } @@ -8240,7 +8241,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .eq('sie_dim_no', sieDimNo) .maybeSingle() - if (dimError) throw new Error(`Database error: ${dimError.message}`) + if (dimError) throw dbError(dimError) if (!dimension) { throw new Error( `Dimension ${sieDimNo} finns inte i registret. Anropa gnubok_list_dimensions för att se registrerade dimensioner.`, @@ -8256,7 +8257,7 @@ export const tools: McpTool[] = [ if (!includeInactive) valuesQuery = valuesQuery.eq('is_active', true) const { data: rows, error: valuesError } = await valuesQuery - if (valuesError) throw new Error(`Database error: ${valuesError.message}`) + if (valuesError) throw dbError(valuesError) const all = (rows ?? []) as Array<{ id: string code: string @@ -8350,7 +8351,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .eq('sie_dim_no', params.sie_dim_no) .maybeSingle() - if (dimError) throw new Error(`Database error: ${dimError.message}`) + if (dimError) throw dbError(dimError) if (!dimension) { throw new Error( `Okänd dimension ${params.sie_dim_no}. Endast registrerade dimensioner kan få nya värden: ` + @@ -8370,7 +8371,7 @@ export const tools: McpTool[] = [ .eq('dimension_id', dimension.id) .eq('code', params.code) .maybeSingle() - if (existingError) throw new Error(`Database error: ${existingError.message}`) + if (existingError) throw dbError(existingError) if (existing?.is_active) { throw new Error( `Värdet "${params.code}" (${existing.name}) finns redan i ${dimension.name}: använd koden direkt i dimensions-baggen.`, @@ -10925,7 +10926,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .order('period_start', { ascending: false }) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const periods = (data ?? []).map((p) => ({ id: p.id, @@ -11719,7 +11720,7 @@ export const tools: McpTool[] = [ } const { data, error } = await query - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const mapped = (data || []).map((item) => { const extracted = item.extracted_data as Record | null @@ -11819,7 +11820,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .single() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!data) throw new Error('Inbox item not found') return data @@ -12275,7 +12276,7 @@ export const tools: McpTool[] = [ } const { data: inboxRows, error: inboxError } = await inboxQuery - if (inboxError) throw new Error(`Database error: ${inboxError.message}`) + if (inboxError) throw dbError(inboxError) if (!inboxRows || inboxRows.length === 0) { return { items: [], count: 0 } } @@ -12287,7 +12288,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .in('document_id', docIds) - if (txError) throw new Error(`Database error: ${txError.message}`) + if (txError) throw dbError(txError) const matchedDocIds = new Set((txMatches || []).map((t) => t.document_id)) const unmatched = inboxRows @@ -12402,7 +12403,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (docError) throw new Error(`Database error: ${docError.message}`) + if (docError) throw dbError(docError) if (!doc) throw new Error('Document not found') const ttlSeconds = 300 @@ -12717,7 +12718,7 @@ export const tools: McpTool[] = [ .from('fiscal_periods') .select('id, period_start, period_end') .eq('company_id', companyId) - if (periodsError) throw new Error(`Database error: ${periodsError.message}`) + if (periodsError) throw dbError(periodsError) const periodsByYear = new Map>() for (const p of periods ?? []) { @@ -12734,7 +12735,7 @@ export const tools: McpTool[] = [ .select('id, file_name, mime_type, journal_entry_id') .in('id', documentIds) .eq('company_id', companyId) - if (docsError) throw new Error(`Database error: ${docsError.message}`) + if (docsError) throw dbError(docsError) const docsById = new Map((docs ?? []).map((d) => [d.id as string, d as { id: string; file_name: string; mime_type: string; journal_entry_id: string | null }])) @@ -12820,7 +12821,7 @@ export const tools: McpTool[] = [ .select('id, status') .eq('company_id', companyId) .in('id', existingJeIds) - if (existingErr) throw new Error(`Database error: ${existingErr.message}`) + if (existingErr) throw dbError(existingErr) for (const je of existingJes ?? []) { if ((je as { status: string }).status === 'posted') postedExistingJeIds.add(je.id as string) } @@ -13185,7 +13186,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) if (activeOnly) query = query.eq('is_active', true) const { data, error } = await query.order('last_name') - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) // Shared masking helper: strips personnummer (ciphertext) AND // personnummer_last4, exposing only personnummer_masked: same shape as // the app routes and the other MCP payroll tools. @@ -13370,7 +13371,7 @@ export const tools: McpTool[] = [ .eq('id', id) .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!run) throw new Error('Salary run not found') if (run.status === 'booked') throw new Error('Salary run is already booked') if (!['draft', 'review', 'approved', 'paid'].includes(run.status as string)) { @@ -13383,7 +13384,7 @@ export const tools: McpTool[] = [ .from('salary_run_employees') .select('id, calculation_breakdown') .eq('salary_run_id', id) - if (rosterError) throw new Error(`Database error: ${rosterError.message}`) + if (rosterError) throw dbError(rosterError) const rosterRows = roster ?? [] const uncalculated = rosterRows.filter((r) => !r.calculation_breakdown).length if (uncalculated > 0) { @@ -13880,7 +13881,7 @@ export const tools: McpTool[] = [ .eq('id', employeeId) .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!e) throw new Error('Employee not found') // LLM context is a leak surface: personnummer is ALWAYS masked on MCP, // there is no full-value drill-in on this surface. @@ -13977,7 +13978,7 @@ export const tools: McpTool[] = [ .eq('employee_id', employeeId) .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!sre) throw new Error('Employee not found in this salary run') const emp = sre.employee as { first_name: string; last_name: string; personnummer: string } | null const lineItems = ((sre.line_items ?? []) as Array>) @@ -14593,7 +14594,7 @@ export const tools: McpTool[] = [ .eq('id', employee_id) .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!existing) throw new Error('Employee not found') const changes = Object.entries(patch).map(([field, to]) => ({ @@ -14714,7 +14715,7 @@ export const tools: McpTool[] = [ .select(`employee_id, ${MERGEABLE_FIELDS.join(', ')}`) .eq('company_id', companyId) .in('employee_id', employeeIds) - if (storedErr) throw new Error(`Database error: ${storedErr.message}`) + if (storedErr) throw dbError(storedErr) const storedByEmployee = new Map( ((storedRows ?? []) as unknown as Array>).map((r) => [ r.employee_id as string, @@ -14837,7 +14838,7 @@ export const tools: McpTool[] = [ .order('vacation_year_start', { ascending: false }) .limit(1) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!balance) throw new Error('No vacation balance exists for the employee yet (the ledger seeds on first booking)') const { id, ...rest } = balance as { id: string } & Record const entitled = (rest.entitled_days as number) ?? 0 @@ -14855,7 +14856,7 @@ export const tools: McpTool[] = [ .eq('id', employeeId) .eq('company_id', companyId) .maybeSingle() - if (empErr) throw new Error(`Database error: ${empErr.message}`) + if (empErr) throw dbError(empErr) if (!employee) throw new Error(`Employee ${employeeId} not found for this company`) const savedTotal = Object.values((rest.saved_days as Record | null) ?? {}) .reduce((s, d) => s + (Number(d) || 0), 0) @@ -16121,7 +16122,7 @@ export const tools: McpTool[] = [ .eq('id', journalEntryId) .eq('company_id', companyId) .maybeSingle() - if (fetchErr) throw new Error(`Database error: ${fetchErr.message}`) + if (fetchErr) throw dbError(fetchErr) if (!entry) throw new Error('Verifikationen hittades inte.') const voucherLabel = entry.voucher_number @@ -16555,7 +16556,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!invoice) throw new Error('Invoice not found. Use gnubok_list_invoices to find valid IDs.') // Editable drafts only: the shared predicate the web PATCH route gates @@ -16700,7 +16701,7 @@ export const tools: McpTool[] = [ .select('line_type, description, quantity, unit, unit_price, line_total, vat_rate, revenue_account, article_id, deduction_type, accrual_period_start, accrual_period_end') .eq('invoice_id', invoice.id) .order('sort_order', { ascending: true }) - if (currentError) throw new Error(`Database error: ${currentError.message}`) + if (currentError) throw dbError(currentError) currentItems = (currentRows ?? []).map((row: Record) => ({ line_type: row.line_type ?? 'product', description: row.description, @@ -17715,7 +17716,7 @@ export const tools: McpTool[] = [ const original = data as OriginalRow | null if (origErr) { - throw new Error(`Database error looking up journal entry ${entryId}: ${origErr.message}`) + throw dbError(origErr, `Database error looking up journal entry ${entryId}`) } if (!original) { throw new Error( @@ -17883,7 +17884,7 @@ export const tools: McpTool[] = [ const original = data as OriginalRow | null if (origErr) { - throw new Error(`Database error looking up journal entry ${entryId}: ${origErr.message}`) + throw dbError(origErr, `Database error looking up journal entry ${entryId}`) } if (!original) { throw new Error( @@ -18897,7 +18898,7 @@ export const tools: McpTool[] = [ .order('id', { ascending: false }) .range(offset, offset + limit) - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) const rows = data ?? [] const schedules = rows.slice(0, limit).map((row: Record) => { @@ -19095,7 +19096,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!customer) throw new Error('Customer not found. Use gnubok_list_customers to find IDs.') if (params.auto_send && !customer.email) { throw new Error('Customer has no email address: auto_send requires one. Stage with auto_send=false or add an email first.') @@ -19290,7 +19291,7 @@ export const tools: McpTool[] = [ .eq('company_id', companyId) .maybeSingle() - if (error) throw new Error(`Database error: ${error.message}`) + if (error) throw dbError(error) if (!current) throw new Error('Recurring schedule not found. Use gnubok_list_recurring_schedules to find IDs.') // Turning auto_send on, or moving the schedule to another customer, @@ -19305,7 +19306,7 @@ export const tools: McpTool[] = [ .eq('id', parsedChanges.customer_id) .eq('company_id', companyId) .maybeSingle() - if (targetError) throw new Error(`Database error: ${targetError.message}`) + if (targetError) throw dbError(targetError) if (!target) throw new Error('Customer not found. Use gnubok_list_customers to find IDs.') if (effectiveAutoSend && !target.email) { throw new Error('Customer has no email address: auto_send requires one.') diff --git a/lib/errors/__tests__/db-error.test.ts b/lib/errors/__tests__/db-error.test.ts new file mode 100644 index 00000000..ae1367e5 --- /dev/null +++ b/lib/errors/__tests__/db-error.test.ts @@ -0,0 +1,110 @@ +/** + * The database-error wrapper, and the classification it exists to unlock. + * + * The idiom `throw new Error(\`Database error: ${error.message}\`)` kept the + * prose and dropped `code`. `isTransientFailure()` checks that SQLSTATE FIRST, + * and 57014 (statement timeout) is already in its transient set, so stripping + * it turned a retryable timeout into UNKNOWN_ERROR: "Något gick fel. Försök + * igen." Agents cannot dispatch on that, so they retried: on production over + * 60 days, 462 wasted repeat calls, with 53.1% of all real-agent error calls + * sitting inside a repeat streak. + * + * The headline test is the last one: it asserts the OLD shape still resolves + * to UNKNOWN_ERROR and the new one resolves to TRANSIENT_ERROR, so it fails if + * the wrapper ever stops preserving the code. + */ +import { describe, it, expect } from 'vitest' +import { dbError, errorCauseTag } from '../db-error' +import { getStructuredError } from '../get-structured-error' + +/** What supabase-js hands back when Postgres cancels on statement_timeout. */ +const TIMEOUT_ERROR = { + message: 'canceling statement due to statement timeout', + code: '57014', + details: null, + hint: null, +} + +describe('dbError', () => { + it('preserves the SQLSTATE, which is the whole point', () => { + const wrapped = dbError(TIMEOUT_ERROR) + expect(wrapped.code).toBe('57014') + }) + + it('preserves details and hint for the server log', () => { + const wrapped = dbError({ + message: 'duplicate key value violates unique constraint', + code: '23505', + details: 'Key (company_id, account_number) already exists.', + hint: 'Use upsert.', + }) + expect(wrapped.details).toContain('already exists') + expect(wrapped.hint).toBe('Use upsert.') + }) + + it('never renders the literal "undefined"', () => { + // A driver-level failure (aborted fetch, gateway timeout) can arrive with + // no message at all. "Database error: undefined" is the string that made + // these unsearchable in production. + for (const shape of [{}, { message: undefined }, { message: '' }, { message: ' ' }, null]) { + expect(dbError(shape).message).not.toContain('undefined') + } + }) + + it('prefixes with the historical context by default', () => { + expect(dbError({ message: 'boom' }).message).toBe('Database error: boom') + }) + + it('keeps the driver message verbatim when context is null', () => { + // fetchAllRows passes null: callers such as query_journal's + // sanitizeDbError already match on the exact text, and this change is + // meant to add the code, not reword anything. + expect(dbError({ message: 'boom' }, null).message).toBe('boom') + }) + + it('accepts a custom context', () => { + expect(dbError({ message: 'boom' }, 'Database error resolving voucher "A-7"').message).toBe( + 'Database error resolving voucher "A-7": boom', + ) + }) +}) + +describe('errorCauseTag', () => { + it('returns the SQLSTATE, which carries no tenant data', () => { + expect(errorCauseTag(dbError(TIMEOUT_ERROR))).toBe('57014') + }) + + it('falls back to the error name when there is no code', () => { + expect(errorCauseTag(new TypeError('nope'))).toBe('TypeError') + }) + + it('returns null rather than inventing a tag', () => { + expect(errorCauseTag(new Error('plain'))).toBeNull() + expect(errorCauseTag(null)).toBeNull() + }) +}) + +describe('classification: the regression this prevents', () => { + it('classifies a wrapped statement timeout as retryable', () => { + expect(getStructuredError(dbError(TIMEOUT_ERROR)).code).toBe('TRANSIENT_ERROR') + }) + + it('would classify the OLD bare-Error shape as UNKNOWN_ERROR', () => { + // The bug, pinned. `new Error(error.message)` is what fetchAllRows threw, + // and PostgREST does not always put the word "timeout" in the message it + // returns, so message-pattern matching cannot be relied on. Only the code + // is durable, which is why the wrapper must carry it. + const stripped = new Error('some driver text that names no timeout') + expect(getStructuredError(stripped).code).toBe('UNKNOWN_ERROR') + + // Same failure, code intact: now dispatchable. + const preserved = dbError({ message: 'some driver text that names no timeout', code: '57014' }) + expect(getStructuredError(preserved).code).toBe('TRANSIENT_ERROR') + }) + + it('does not mistake a SQLSTATE for one of our own stable codes', () => { + // extractCode() only accepts /^[A-Z_]+$/. Every SQLSTATE contains digits, + // so attaching one cannot hijack the application error registry. + expect(getStructuredError(dbError({ message: 'x', code: '23505' })).code).not.toBe('23505') + }) +}) diff --git a/lib/errors/db-error.ts b/lib/errors/db-error.ts new file mode 100644 index 00000000..74eb82ff --- /dev/null +++ b/lib/errors/db-error.ts @@ -0,0 +1,97 @@ +/** + * Wrap a Supabase/PostgREST error without throwing away its identity. + * + * ## The bug this exists to prevent + * + * The idiom across the MCP tools was: + * + * if (error) throw new Error(`Database error: ${error.message}`) + * + * which keeps the prose and discards `code`. That matters because `code` is + * the SQLSTATE, and `isTransientFailure()` in lib/errors/get-structured-error.ts + * checks it FIRST: `57014` (statement timeout), `40001`, `40P01`, `53300` and + * friends are already in its TRANSIENT_SQLSTATES set. Strip the code and a + * retryable timeout arrives as an anonymous Error, misses every transient + * check, and resolves to UNKNOWN_ERROR: "Något gick fel. Försök igen." + * + * Measured on production over 60 days (bot actors excluded): 1 024 real-agent + * tool failures, 645 of them UNKNOWN_ERROR across 60 actors and 57 companies, + * 537 carrying that exact generic string. `gnubok_query_journal` alone failed + * 164 times at a p50 of 8 110 ms while every other failing tool sat between 1 + * and 315 ms: a timeout signature that should have been TRANSIENT_ERROR all + * along. Agents cannot dispatch on "something went wrong", so they retried: + * 82 streaks of three or more identical failures, 462 wasted repeat calls, + * 53.1% of all real-agent error calls sitting inside a streak. + * + * ## Why attaching `code` is safe + * + * `extractCode()` only treats a code as an application error code when it + * matches /^[A-Z_]+$/. Every SQLSTATE contains digits (`57014`, `42P01`, + * `23505`), and so does every PostgREST code (`PGRST200`), so none of them can + * be mistaken for one of our own stable codes. The only behaviour this unlocks + * is the transient check that was always meant to run. + */ + +/** The shape of a PostgrestError, narrowed to what we read. */ +interface DatabaseErrorLike { + message?: string | null + code?: string | null + details?: string | null + hint?: string | null +} + +/** + * An Error carrying the driver's SQLSTATE and diagnostics. + * + * `details` and `hint` are preserved for the server log, not for the agent: + * `getStructuredError` never reads them, so they cannot leak into a tool + * result. They are what makes a production failure debuggable after the fact. + */ +export interface DatabaseError extends Error { + code?: string + details?: string + hint?: string +} + +/** + * @param error the `error` half of a supabase-js `{ data, error }` result + * @param context prefix for the message; defaults to the historical + * "Database error" so existing message-pattern matching in + * `inferCode()` keeps working unchanged. Pass `null` to keep + * the driver's message verbatim, for call sites whose exact + * text callers already depend on. + */ +export function dbError(error: unknown, context: string | null = 'Database error'): DatabaseError { + const source = (error ?? {}) as DatabaseErrorLike + const raw = typeof source.message === 'string' && source.message.trim() ? source.message.trim() : null + + // Never render the literal "undefined". A driver-level failure (an aborted + // fetch, a gateway timeout) can arrive with no `message` at all, and + // "Database error: undefined" is the string that made these unsearchable in + // the first place. + const prefix = context ?? '' + const message = raw + ? (prefix ? `${prefix}: ${raw}` : raw) + : `${prefix || 'Database error'}: no message from the database driver` + + const wrapped = new Error(message) as DatabaseError + if (typeof source.code === 'string' && source.code) wrapped.code = source.code + if (typeof source.details === 'string' && source.details) wrapped.details = source.details + if (typeof source.hint === 'string' && source.hint) wrapped.hint = source.hint + return wrapped +} + +/** + * A PII-safe identifier for what failed, for telemetry. + * + * A SQLSTATE is five characters of protocol vocabulary and carries no tenant + * data. A raw driver message can quote row values in a constraint violation, + * so it belongs in the server log, never in `event_log`. + */ +export function errorCauseTag(error: unknown): string | null { + if (error === null || error === undefined) return null + const source = error as DatabaseErrorLike & { name?: unknown } + if (typeof source.code === 'string' && source.code) return source.code + if (error instanceof Error && error.name && error.name !== 'Error') return error.name + return null +} diff --git a/lib/supabase/__tests__/fetch-all.test.ts b/lib/supabase/__tests__/fetch-all.test.ts index ba655043..2d9fd0ac 100644 --- a/lib/supabase/__tests__/fetch-all.test.ts +++ b/lib/supabase/__tests__/fetch-all.test.ts @@ -41,6 +41,31 @@ describe('fetchAllRows', () => { ).rejects.toThrow('boom') }) + it('propagates the driver SQLSTATE so a timeout stays dispatchable', async () => { + // This is the highest-traffic error strip point in the codebase: every + // paginated read goes through it. It used to throw `new Error(msg)`, which + // dropped `code`, so a statement timeout (57014) reached the structured + // error layer anonymous and resolved to UNKNOWN_ERROR instead of the + // retryable TRANSIENT_ERROR. gnubok_query_journal failed this way 164 + // times in 60 days at a p50 of 8 110 ms. + const failure = fetchAllRows(() => + Promise.resolve({ + data: null, + error: { message: 'canceling statement due to statement timeout', code: '57014' }, + }), + ) + await expect(failure).rejects.toMatchObject({ code: '57014' }) + // And the message stays verbatim: query_journal's sanitizeDbError matches + // on the existing text. + await expect(failure).rejects.toThrow('canceling statement due to statement timeout') + }) + + it('does not render "undefined" when the driver sends no message', async () => { + await expect( + fetchAllRows(() => Promise.resolve({ data: null, error: { code: '08006' } as never })), + ).rejects.toThrow(/^(?!.*undefined).*$/) + }) + it('returns [] when the first page is empty', async () => { const out = await fetchAllRows(pagedQuery({ 0: [] })) expect(out).toEqual([]) diff --git a/lib/supabase/fetch-all.ts b/lib/supabase/fetch-all.ts index 10ca479a..12968f33 100644 --- a/lib/supabase/fetch-all.ts +++ b/lib/supabase/fetch-all.ts @@ -1,4 +1,5 @@ import { createLogger } from '@/lib/logger' +import { dbError } from '@/lib/errors/db-error' const log = createLogger('fetch-all') @@ -71,7 +72,19 @@ export async function fetchAllRows( while (true) { const { data, error } = await queryFn({ from, to: from + PAGE_SIZE - 1 }) - if (error) throw new Error(error.message) + // dbError, not `new Error(error.message)`: the bare form discarded the + // driver's SQLSTATE, and `isTransientFailure()` checks that code FIRST. + // A statement timeout (57014) therefore arrived as an anonymous Error and + // resolved to UNKNOWN_ERROR ("Något gick fel. Försök igen.") instead of + // the retryable TRANSIENT_ERROR. This is the single highest-traffic strip + // point in the codebase: every paginated read goes through it, including + // gnubok_query_journal, which failed 164 times in 60 days at a p50 of + // 8 110 ms while every other failing tool sat between 1 and 315 ms. + // + // The message is kept VERBATIM (context: null). Callers such as + // query_journal's sanitizeDbError already match on it, and this change is + // meant to add the code, not to reword anything. + if (error) throw dbError(error, null) if (!data || data.length === 0) break allRows.push(...data) pages += 1