diff --git a/DECISIONS.md b/DECISIONS.md index 437dcd13..ac9ed27c 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1167,6 +1167,7 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-20] Invoice detail hydration addresses the endpoint by the resource config's own `idField` read off the raw payload, not by `dto.id`. Björn Lundén's sales config names `invoiceNumber` while its mapper builds `dto.id` from `entityId`, so `dto.id` would have requested a different invoice or none; every other provider/resource pair happens to agree, which is exactly why the mismatch was easy to miss. [2026-08-21] A migrated mixed-rate invoice stores `vat_rate: null` while keeping a treatment, matching what buildInvoiceWriteData already does for a natively created one (`isMixedRate ? null : theRate`). Labelling the header with the first line's rate would assert 25 % on an invoice that is 25 % and 6 %, and dividing the rate out of the totals gives a blended figure matching no statutory rate. The money is unaffected either way: generatePerRateLines groups per ITEM rate, which is why the per-line vat_rate/vat_amount are the part that has to be right. [2026-08-21] Invoice detail hydration stops the whole pass on a 401/403 and bounds every in-flight call against the budget deadline. The provider clients retry 429s and 5xx with backoff (Fortnox: 6 attempts, up to 60 s apart), so a call starting one millisecond inside the budget can still be retrying minutes later, and three concurrent ones could hold the migration past its 300 s ceiling; racing each against the deadline returns control even though the socket is not cancelled. A rejected token fails identically for every remaining invoice, so continuing would spend the Fortnox rate-limit budget for nothing: note that limiter keys on the literal string 'global', making 4 req/s a PLATFORM-WIDE budget shared by every company and every concurrent migration, not a per-token one. +[2026-08-23] Reversed the 2026-07-26 call and moved the two free-text ilike legs of MCP query_journal off the journal_entries!inner embed onto fetchEntryLines: the embed's correlated LATERAL was the query behind the daily statement timeouts (57014) in prod, and leg B (line_description) is bounded by the same entry-side scope the plain query already fetches in full, so the two-step pass is never more expensive than the non-text query with the same filters. Cost: legLimit/legCapHit and totals_scope='returned_slice' are gone (text totals/truncated are now exact; the field stays, always 'full_match'). No trigram index this round; if the ilike scan itself proves slow on staging that becomes a separate RPC PR. [2026-08-22] Per-company invoice sending domains are gated by a manually granted capability (custom_sender_domain), deliberately NOT in PAID_CAPABILITIES: the opt-in must not be trial-seeded or written by the Stripe subscription sync, and non-grantees must see an unchanged invoicing settings page (the section hides on the 403 capability_blocked envelope). The sending-domain module has no Resend orphan-adoption path (a name that already exists is a 409), because the same Resend account holds the platform's own outbound domain. The delivery log was left untouched (no from_address column): adding it would re-open the hardened invoice_deliveries evidence triggers/redaction paths for a nice-to-have, and the log already measures delivered/bounced per send. [2026-08-22] company_sending_domains verification state (domain, status, resend_domain_id, dns_records, verified_at, last_checked_at) is service-role only via a BEFORE trigger keyed on the JWT role claim; tenant JWTs may only open a pending claim and edit sender_local_part/sender_name/enabled. Skeptic refutation: RLS alone let a granted admin insert {domain: platform sender domain, status: verified} through PostgREST and send invoice mail as the platform. The claim/verify helpers therefore take a separate service-role writer for those columns. Second refutation: a domain Resend later flips to failed made every invoice send for that company fail; the Resend adapter now retries once as the platform sender when an explicit company From is rejected (nothing was sent on the rejected attempt, so the retry cannot double-send). [2026-08-22] Sending-domain verification writes bind by (id, company_id, domain, resend_domain_id IS NULL) and verify/webhook compare Resend's domain name with the row before writing verified; resolveInvoiceSender additionally refuses reserved platform domains and non-hostnames at send time. Skeptic re-check: a tenant could delete and re-insert its pending row under the same id with a reserved domain during the claim's Resend round-trip (TOCTOU), and the service-role writer updated by id alone. Defense in depth over a single gate. diff --git a/extensions/general/mcp-server/__tests__/query-journal.test.ts b/extensions/general/mcp-server/__tests__/query-journal.test.ts index 83cd95d2..d70afb7a 100644 --- a/extensions/general/mcp-server/__tests__/query-journal.test.ts +++ b/extensions/general/mcp-server/__tests__/query-journal.test.ts @@ -2,15 +2,17 @@ * Unit tests for gnubok_query_journal. * * Verifies tool registration, the post-fetch amount filter, the full-match - * aggregate pass (totals/groups over ALL matching lines via fetchAllRows, - * totals_scope='full_match'), and the slice-scoped free-text path - * (totals_scope='returned_slice'). The supabase query-builder chain is - * exercised by the live MCP smoke test; here we check the result-shape - * pipeline. + * aggregate pass (totals/groups over ALL matching lines via the two-step + * entry-lines fetch, totals_scope='full_match'), and the free-text path, + * which runs the same two-step fetch once per leg (entry description, line + * description) instead of a `journal_entries!inner` embed. The supabase + * query-builder chain is exercised by the live MCP smoke test; here we + * check the filters sent and the result-shape pipeline. */ import { describe, it, expect, vi } from 'vitest' import { tools } from '../server' import { TOOL_SCOPE_MAP } from '@/lib/auth/api-keys' +import { getStructuredError } from '@/lib/errors/get-structured-error' describe('gnubok_query_journal: registration', () => { it('is registered and read-only', () => { @@ -113,62 +115,127 @@ function makeEntryLinesMock(rows: Array>) { return { supabase, tables } } -/** - * Richer mock for the text-search path: returns queued results across - * successive .from() calls and records every .ilike(column, pattern) call so - * tests can assert what was actually sent to PostgREST. - * - * The text branch issues TWO parallel .from('journal_entry_lines') queries: - * one filtered by line_description, one by journal_entries.description. The - * first .from() call gets `results[0]`, the second gets `results[1]`. - */ -function makeQueueMock(results: Array<{ data: unknown[]; count: number }>) { - const ilikeCalls: Array<{ column: string; pattern: string }> = [] - // Each entry is one leg's recorded .eq calls. Index lines up with - // .from() invocation order, so tests can assert per-leg tenant scoping. - const eqCallsByLeg: Array> = [] - let callIndex = 0 +type FakeFilter = { op: string; column: string; value: unknown } +type FakeQuery = { table: string; filters: FakeFilter[] } - const buildChain = ( - result: { data: unknown[]; error: null; count: number }, - legEqCalls: Array<{ column: string; value: unknown }>, - ): unknown => { +/** Translate a LIKE pattern (with `\`-escaped `%`, `_`, `\`) into a regex. */ +function likeToRegex(pattern: string): RegExp { + const escapeRe = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + let re = '' + for (let i = 0; i < pattern.length; i++) { + const ch = pattern[i] + if (ch === '\\' && i + 1 < pattern.length) { + re += escapeRe(pattern[++i]) + } else if (ch === '%') { + re += '.*' + } else if (ch === '_') { + re += '.' + } else { + re += escapeRe(ch) + } + } + return new RegExp(`^${re}$`, 'is') +} + +/** + * Filter-aware fake for the free-text path. Both text legs run the two-step + * entry-lines fetch (journal_entries first, then journal_entry_lines by + * parent id) with the .ilike() on the entry side (leg A: description) or + * the line side (leg B: line_description). The legs run in parallel, so + * .from() call ORDER is not something a test should pin; instead this fake + * evaluates the recorded filters (eq/in/gte/lte/ilike) against embed-shaped + * fixtures like a tiny PostgREST, and records every query (table + filters) + * so tests can assert on scoping and on what was sent. + * + * `failLinesWith` makes every journal_entry_lines page fail with that raw + * message (the helper re-throws it as a plain Error), for the error-path + * tests. + */ +function makeTwoStepTextMock( + rows: Array>, + opts: { failLinesWith?: string } = {}, +) { + const entries = [ + ...new Map( + rows.map((r) => { + const e = r.journal_entries as { id: string } + return [e.id, e as Record] + }), + ).values(), + ] + const bareLines = rows.map((r) => { + const { journal_entries: parent, ...line } = r + return { ...line, journal_entry_id: (parent as { id: string }).id } as Record + }) + const queries: FakeQuery[] = [] + + const passes = (row: Record, f: FakeFilter): boolean => { + // Columns the fixture does not carry (company_id, status defaults, ...) + // are unconstrained: the tests that care assert on the recorded filters. + if (!(f.column in row)) return true + const v = row[f.column] + switch (f.op) { + case 'eq': + return v === f.value + case 'in': + return (f.value as unknown[]).includes(v) + case 'gte': + return typeof v === typeof f.value && (v as string | number) >= (f.value as string | number) + case 'lte': + return typeof v === typeof f.value && (v as string | number) <= (f.value as string | number) + case 'ilike': + return typeof v === 'string' && likeToRegex(f.value as string).test(v) + default: + return true + } + } + + const chain = (query: FakeQuery, data: Record[]): unknown => { + const evaluate = () => { + if (query.table === 'journal_entry_lines' && opts.failLinesWith) { + return { data: null, error: { message: opts.failLinesWith }, count: null } + } + const out = data.filter((row) => query.filters.every((f) => passes(row, f))) + return { data: out, error: null, count: out.length } + } return new Proxy( {}, { get(_t, prop) { if (prop === 'then') { - return (resolve: (v: unknown) => void) => resolve(result) + return (resolve: (v: unknown) => void) => resolve(evaluate()) } - if (prop === 'ilike') { - return (column: string, pattern: string) => { - ilikeCalls.push({ column, pattern }) - return buildChain(result, legEqCalls) - } - } - if (prop === 'eq') { + if (prop === 'range') return () => evaluate() + if (prop === 'eq' || prop === 'in' || prop === 'gte' || prop === 'lte' || prop === 'ilike' || prop === 'contains') { return (column: string, value: unknown) => { - legEqCalls.push({ column, value }) - return buildChain(result, legEqCalls) + query.filters.push({ op: prop, column, value }) + return chain(query, data) } } - return () => buildChain(result, legEqCalls) + return () => chain(query, data) }, }, ) } const supabase = { - from: vi.fn().mockImplementation(() => { - const next = results[callIndex] ?? { data: [], count: 0 } - callIndex += 1 - const legEqCalls: Array<{ column: string; value: unknown }> = [] - eqCallsByLeg.push(legEqCalls) - return buildChain({ data: next.data, error: null, count: next.count }, legEqCalls) + from: vi.fn().mockImplementation((table: string) => { + const query: FakeQuery = { table, filters: [] } + queries.push(query) + return chain(query, table === 'journal_entries' ? entries : bareLines) }), } as never - return { supabase, ilikeCalls, eqCallsByLeg, callCount: () => callIndex } + const ilikeCalls = () => + queries.flatMap((q) => + q.filters + .filter((f) => f.op === 'ilike') + .map((f) => ({ table: q.table, column: f.column, pattern: f.value as string })), + ) + const entryQueries = () => queries.filter((q) => q.table === 'journal_entries') + const lineQueries = () => queries.filter((q) => q.table === 'journal_entry_lines') + + return { supabase, queries, ilikeCalls, entryQueries, lineQueries } } /** Build a LineRow fixture inline: keeps the per-test data dense and readable. */ @@ -506,8 +573,42 @@ describe('gnubok_query_journal: execute', () => { }) }) +describe('gnubok_query_journal: amount filter vs limit', () => { + it('applies the amount filter before the display slice so `limit` returns matching lines', async () => { + // Old behaviour sliced the first `limit` rows and THEN dropped the ones + // failing amount_min, so limit=1 could return zero lines while matches + // existed. Now the full match set is filtered first. + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const rows = [ + makeLineRow({ id: 'l1', debit_amount: 50, voucher_number: 3 }), + makeLineRow({ id: 'l2', debit_amount: 5000, voucher_number: 2 }), + makeLineRow({ id: 'l3', debit_amount: 6000, voucher_number: 1 }), + ] + const { supabase } = makeEntryLinesMock(rows) + + const result = (await tool.execute( + { amount_min: 1000, limit: 1 }, + 'company-1', + 'user-1', + supabase, + )) as { + lines: Array<{ line_id: string }> + returned_lines: number + total_lines: number + truncated: boolean + db_matched_pre_amount_filter: number | null + } + + expect(result.returned_lines).toBe(1) + expect(result.lines[0].line_id).toBe('l2') + expect(result.total_lines).toBe(2) + expect(result.truncated).toBe(true) + expect(result.db_matched_pre_amount_filter).toBe(3) + }) +}) + describe('gnubok_query_journal: free-text search', () => { - it('merges non-overlapping results from line_description and journal_entries.description', async () => { + it('merges results from the entry-description leg and the line-description leg', async () => { const tool = tools.find((t) => t.name === 'gnubok_query_journal')! const byLineHit = makeLineRow({ id: 'L1', @@ -523,28 +624,36 @@ describe('gnubok_query_journal: free-text search', () => { entry_date: '2026-05-12', voucher_number: 43, }) + const noise = makeLineRow({ + id: 'L3', + line_description: 'Hyra maj', + entry_description: 'Lokalhyra', + entry_date: '2026-05-01', + voucher_number: 40, + }) - const { supabase, callCount } = makeQueueMock([ - { data: [byLineHit], count: 1 }, - { data: [byEntryHit], count: 1 }, - ]) + const { supabase, entryQueries, lineQueries } = makeTwoStepTextMock([byLineHit, byEntryHit, noise]) const result = (await tool.execute( { text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase, - )) as { lines: Array<{ line_id: string }>; returned_lines: number; totals_scope: string } + )) as { lines: Array<{ line_id: string }>; returned_lines: number; total_lines: number; totals_scope: string } - expect(callCount()).toBe(2) + // Two legs, each a two-step fetch: two entry-side queries, and one line + // chunk per leg (both legs found at least one entry in scope). + expect(entryQueries()).toHaveLength(2) + expect(lineQueries()).toHaveLength(2) expect(result.returned_lines).toBe(2) - const ids = result.lines.map((l) => l.line_id).sort() - expect(ids).toEqual(['L1', 'L2']) - // Free-text path never runs the full aggregate pass: the output says so. - expect(result.totals_scope).toBe('returned_slice') + expect(result.total_lines).toBe(2) + // Newest first. + expect(result.lines.map((l) => l.line_id)).toEqual(['L2', 'L1']) + // Free-text search now aggregates the full match set like every other path. + expect(result.totals_scope).toBe('full_match') }) - it('deduplicates rows returned by both query legs', async () => { + it('deduplicates a line hit by both legs', async () => { const tool = tools.find((t) => t.name === 'gnubok_query_journal')! const dupHit = makeLineRow({ id: 'LDUP', @@ -554,58 +663,82 @@ describe('gnubok_query_journal: free-text search', () => { voucher_number: 100, }) - const { supabase } = makeQueueMock([ - { data: [dupHit], count: 1 }, - { data: [dupHit], count: 1 }, - ]) + const { supabase } = makeTwoStepTextMock([dupHit]) const result = (await tool.execute( { text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase, - )) as { lines: Array<{ line_id: string }>; returned_lines: number } + )) as { lines: Array<{ line_id: string }>; returned_lines: number; total_lines: number; truncated: boolean } expect(result.returned_lines).toBe(1) + expect(result.total_lines).toBe(1) + expect(result.truncated).toBe(false) expect(result.lines[0].line_id).toBe('LDUP') }) - it('issues .ilike against both line_description and journal_entries.description with escaped pattern', async () => { + it('never queries journal_entry_lines through a journal_entries embed', async () => { + // The `journal_entries!inner(...)` embed compiled to a correlated LATERAL + // join over every tenant's lines (statement timeouts in production). + // Both legs must drive from journal_entries and hit lines by parent id. const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase, ilikeCalls } = makeQueueMock([ - { data: [], count: 0 }, - { data: [], count: 0 }, + const { supabase, queries, lineQueries } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'Google Cloud' }), + ]) + const selects: string[] = [] + const originalFrom = (supabase as { from: (t: string) => unknown }).from + ;(supabase as { from: unknown }).from = vi.fn().mockImplementation((table: string) => { + const chain = originalFrom(table) as Record unknown> + return new Proxy(chain, { + get(target, prop) { + if (prop === 'select') { + return (cols: string) => { + selects.push(cols) + return target.select(cols) + } + } + return target[prop as string] + }, + }) + }) + + await tool.execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase) + + expect(queries.length).toBeGreaterThan(0) + expect(selects.length).toBe(queries.length) + expect(selects.some((s) => s.includes('journal_entries!inner'))).toBe(false) + for (const q of lineQueries()) { + expect(q.filters.some((f) => f.op === 'in' && f.column === 'journal_entry_id')).toBe(true) + } + }) + + it('issues .ilike on journal_entries.description (entry side) and journal_entry_lines.line_description (line side) with the escaped pattern', async () => { + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const { supabase, ilikeCalls } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'x', entry_description: 'y' }), ]) - await tool.execute( - { text: 'Google', limit: 50 }, - 'company-1', - 'user-1', - supabase, - ) + await tool.execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase) - const columns = ilikeCalls.map((c) => c.column).sort() - expect(columns).toEqual(['journal_entries.description', 'line_description']) - expect(ilikeCalls.every((c) => c.pattern === '%Google%')).toBe(true) + const calls = ilikeCalls() + expect(calls.filter((c) => c.table === 'journal_entries' && c.column === 'description')).toHaveLength(1) + expect(calls.filter((c) => c.table === 'journal_entry_lines' && c.column === 'line_description')).toHaveLength(1) + expect(calls.every((c) => c.pattern === '%Google%')).toBe(true) }) it('escapes LIKE wildcards (% and _) in the search pattern', async () => { const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase, ilikeCalls } = makeQueueMock([ - { data: [], count: 0 }, - { data: [], count: 0 }, + const { supabase, ilikeCalls } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'x', entry_description: 'y' }), ]) - await tool.execute( - { text: '2_441%foo', limit: 50 }, - 'company-1', - 'user-1', - supabase, - ) + await tool.execute({ text: '2_441%foo', limit: 50 }, 'company-1', 'user-1', supabase) // Both legs see the same escaped pattern. - expect(new Set(ilikeCalls.map((c) => c.pattern)).size).toBe(1) - expect(ilikeCalls[0].pattern).toBe('%2\\_441\\%foo%') + const patterns = new Set(ilikeCalls().map((c) => c.pattern)) + expect(patterns.size).toBe(1) + expect([...patterns][0]).toBe('%2\\_441\\%foo%') }) it('escapes a literal backslash so it does not swallow the next character', async () => { @@ -614,15 +747,15 @@ describe('gnubok_query_journal: free-text search', () => { // filter silently matched rows containing `ab` and missed the ones the user // actually asked for. Flagged by CodeQL as js/incomplete-sanitization. const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase, ilikeCalls } = makeQueueMock([ - { data: [], count: 0 }, - { data: [], count: 0 }, + const { supabase, ilikeCalls } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'x', entry_description: 'y' }), ]) await tool.execute({ text: 'a\\b', limit: 50 }, 'company-1', 'user-1', supabase) - expect(new Set(ilikeCalls.map((c) => c.pattern)).size).toBe(1) - expect(ilikeCalls[0].pattern).toBe('%a\\\\b%') + const patterns = new Set(ilikeCalls().map((c) => c.pattern)) + expect(patterns.size).toBe(1) + expect([...patterns][0]).toBe('%a\\\\b%') }) it('escapes backslash before the wildcard rules, not after', async () => { @@ -630,101 +763,104 @@ describe('gnubok_query_journal: free-text search', () => { // % / _ rules just introduced, turning `50%` into `50\\%` (a literal // backslash followed by a wildcard) instead of `50\%` (a literal percent). const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase, ilikeCalls } = makeQueueMock([ - { data: [], count: 0 }, - { data: [], count: 0 }, + const { supabase, ilikeCalls } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'x', entry_description: 'y' }), ]) await tool.execute({ text: '50%', limit: 50 }, 'company-1', 'user-1', supabase) - expect(ilikeCalls[0].pattern).toBe('%50\\%%') + expect(ilikeCalls()[0].pattern).toBe('%50\\%%') }) - it('does NOT flag truncated when an overlap row is hit by both legs and merged set fits limit', async () => { - // Greptile / Compliance V2.3 regression: previously, dbMatched = sum of - // leg counts and a row matching both legs would inflate the count and - // force truncated=true even though every distinct match was returned. + it('computes totals, total_lines and truncated over the FULL text match set', async () => { + // The per-leg window (legLimit/legCapHit) is gone: both legs pull their + // whole match set, so the text path reports exact counts and totals and + // `truncated` is simply "more matches than returned". const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const dupHit = makeLineRow({ - id: 'LDUP', - line_description: 'Google Cloud', - entry_description: 'Google Cloud invoice', - }) - - const { supabase } = makeQueueMock([ - { data: [dupHit], count: 1 }, - { data: [dupHit], count: 1 }, - ]) - - const result = (await tool.execute( - { text: 'Google', limit: 50 }, - 'company-1', - 'user-1', - supabase, - )) as { lines: unknown[]; truncated: boolean; total_lines: number; returned_lines: number } - - expect(result.returned_lines).toBe(1) - expect(result.total_lines).toBe(1) - expect(result.truncated).toBe(false) - }) - - it('flags truncated when a leg fills its per-leg fetch window', async () => { - // Per-leg cap is limit*2. With limit=2 → legLimit=4. Returning 4 rows on - // one leg signals "this leg's window filled, more may exist DB-side". - const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const fullLeg = [ - makeLineRow({ id: 'L1', entry_date: '2026-05-10', voucher_number: 4 }), - makeLineRow({ id: 'L2', entry_date: '2026-05-09', voucher_number: 3 }), - makeLineRow({ id: 'L3', entry_date: '2026-05-08', voucher_number: 2 }), - makeLineRow({ id: 'L4', entry_date: '2026-05-07', voucher_number: 1 }), + const rows = [ + makeLineRow({ id: 'L1', line_description: 'Google a', debit_amount: 100, entry_date: '2026-05-10', voucher_number: 4 }), + makeLineRow({ id: 'L2', line_description: 'Google b', debit_amount: 200, entry_date: '2026-05-09', voucher_number: 3 }), + makeLineRow({ id: 'L3', line_description: null, entry_description: 'Google c', debit_amount: 300, entry_date: '2026-05-08', voucher_number: 2 }), + makeLineRow({ id: 'L4', line_description: null, entry_description: 'Google d', debit_amount: 400, entry_date: '2026-05-07', voucher_number: 1 }), + makeLineRow({ id: 'L5', line_description: 'Hyra', entry_description: 'Hyra', debit_amount: 9999, entry_date: '2026-05-06', voucher_number: 0 }), ] - - const { supabase } = makeQueueMock([ - { data: fullLeg, count: 4 }, - { data: [], count: 0 }, - ]) + const { supabase } = makeTwoStepTextMock(rows) const result = (await tool.execute( { text: 'Google', limit: 2 }, 'company-1', 'user-1', supabase, - )) as { returned_lines: number; truncated: boolean } + )) as { + lines: Array<{ line_id: string }> + returned_lines: number + total_lines: number + truncated: boolean + totals: { debit: number; credit: number; net: number } + totals_scope: string + } expect(result.returned_lines).toBe(2) + expect(result.lines.map((l) => l.line_id)).toEqual(['L1', 'L2']) + expect(result.total_lines).toBe(4) expect(result.truncated).toBe(true) + expect(result.totals).toEqual({ debit: 1000, credit: 0, net: 1000 }) + expect(result.totals_scope).toBe('full_match') }) - it('scopes BOTH parallel legs to the caller company_id (tenant isolation)', async () => { - // Defence-in-depth against a future refactor that splits the legs and - // accidentally drops .eq('journal_entries.company_id', companyId) from - // one of them. RLS would still block cross-tenant reads, but losing the - // app-level filter would mean a wider scan than intended. + it('scopes BOTH legs to the caller company_id on the entry side (tenant isolation)', async () => { + // Defence-in-depth against a future refactor that drops + // .eq('company_id', companyId) from one leg's entry query. RLS would + // still block cross-tenant reads, but losing the app-level filter would + // mean a wider scan than intended. const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase, eqCallsByLeg, callCount } = makeQueueMock([ - { data: [], count: 0 }, - { data: [], count: 0 }, + const { supabase, entryQueries } = makeTwoStepTextMock([ + makeLineRow({ id: 'L1', line_description: 'Google' }), ]) - await tool.execute( - { text: 'Google', limit: 50 }, - 'company-xyz', + await tool.execute({ text: 'Google', limit: 50 }, 'company-xyz', 'user-1', supabase) + + const legs = entryQueries() + expect(legs).toHaveLength(2) + for (const leg of legs) { + expect( + leg.filters.some((f) => f.op === 'eq' && f.column === 'company_id' && f.value === 'company-xyz'), + ).toBe(true) + } + }) + + it('applies date_from/date_to on the entry side of both legs and excludes out-of-range matches', async () => { + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const rows = [ + makeLineRow({ id: 'IN1', line_description: 'DLE Sverige', entry_date: '2026-03-15', voucher_number: 2 }), + makeLineRow({ id: 'IN2', entry_description: 'DLE faktura', entry_date: '2026-03-20', voucher_number: 3 }), + makeLineRow({ id: 'OUT1', line_description: 'DLE Sverige', entry_date: '2025-11-02', voucher_number: 1 }), + makeLineRow({ id: 'OUT2', entry_description: 'DLE faktura', entry_date: '2026-07-01', voucher_number: 9 }), + ] + const { supabase, entryQueries } = makeTwoStepTextMock(rows) + + const result = (await tool.execute( + { text: 'DLE', date_from: '2026-01-01', date_to: '2026-06-30', limit: 50 }, + 'company-1', 'user-1', supabase, - ) + )) as { lines: Array<{ line_id: string }>; total_lines: number } - expect(callCount()).toBe(2) - for (const legEqs of eqCallsByLeg) { - const scoped = legEqs.some( - (c) => c.column === 'journal_entries.company_id' && c.value === 'company-xyz', + for (const leg of entryQueries()) { + expect(leg.filters).toEqual( + expect.arrayContaining([ + { op: 'gte', column: 'entry_date', value: '2026-01-01' }, + { op: 'lte', column: 'entry_date', value: '2026-06-30' }, + ]), ) - expect(scoped).toBe(true) } + expect(result.total_lines).toBe(2) + expect(result.lines.map((l) => l.line_id).sort()).toEqual(['IN1', 'IN2']) }) it('rejects text longer than 200 characters', async () => { const tool = tools.find((t) => t.name === 'gnubok_query_journal')! - const { supabase } = makeQueueMock([]) + const { supabase } = makeTwoStepTextMock([]) const oversized = 'x'.repeat(201) await expect( @@ -734,39 +870,45 @@ describe('gnubok_query_journal: free-text search', () => { it('does not surface raw PostgREST error text on text-search failure', async () => { const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const { supabase } = makeTwoStepTextMock( + [makeLineRow({ id: 'L1', line_description: 'Google' })], + { failLinesWith: 'relation "journal_entries" does not exist in schema "private_internal"' }, + ) - // Custom mock that returns an error from the first leg. - const supabase = { - from: vi.fn().mockImplementation(() => { - const result = { - data: null, - error: { message: 'relation "journal_entries" does not exist in schema "private_internal"' }, - count: null, - } - const buildChain = (): unknown => - new Proxy( - {}, - { - get(_t, prop) { - if (prop === 'then') { - return (resolve: (v: unknown) => void) => resolve(result) - } - return () => buildChain() - }, - }, - ) - return buildChain() - }), - } as never + const thrown = await tool + .execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase) + .then(() => null, (e: unknown) => e as Error & { code?: string }) + expect(thrown).toBeInstanceOf(Error) + expect(thrown!.message).toMatch(/Database error while running text search/) + // The schema-leak text never reaches the caller, and a non-transient + // failure carries no retry hint. + expect(thrown!.message).not.toMatch(/private_internal/) + expect(thrown!.code).toBeUndefined() + }) - await expect( - tool.execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase), - ).rejects.toThrow(/Database error while running text search/) + it('surfaces a statement timeout as a retryable TRANSIENT_ERROR instead of a generic failure', async () => { + // Production symptom: SQLSTATE 57014 on the text legs reached the agent + // as UNKNOWN_ERROR / "Något gick fel". The sanitised error must still + // carry the transient code so the structured-error layer maps it to the + // retryable envelope, and tell the agent how to narrow the query. + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const { supabase } = makeTwoStepTextMock( + [makeLineRow({ id: 'L1', line_description: 'Google' })], + { failLinesWith: 'canceling statement due to statement timeout' }, + ) - // And the schema-leak text never reaches the caller. - await expect( - tool.execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase), - ).rejects.not.toThrow(/private_internal/) + const thrown = await tool + .execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase) + .then(() => null, (e: unknown) => e as Error & { code?: string }) + expect(thrown).toBeInstanceOf(Error) + expect(thrown!.code).toBe('TRANSIENT_ERROR') + expect(thrown!.message).toMatch(/Database error while running text search/) + expect(thrown!.message).toMatch(/date_from\/date_to/) + expect(thrown!.message).not.toMatch(/canceling statement/) + + const structured = getStructuredError(thrown) + expect(structured.code).toBe('TRANSIENT_ERROR') + expect(structured.retryable).toBe(true) }) }) diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index a1716523..0a0e2744 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -27,6 +27,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 { getStructuredError } from '@/lib/errors/get-structured-error' import { applySettlementAccount } from '@/lib/bookkeeping/mapping-engine' import { resolveSettlementAccount } from '@/lib/bookkeeping/settlement-account' import { buildTransactionEntryLines, createTransactionJournalEntry } from '@/lib/bookkeeping/transaction-entries' @@ -7846,7 +7847,7 @@ export const tools: McpTool[] = [ }, group_by: { type: 'string', enum: ['account_number', 'voucher_series', 'source_type', 'cost_center', 'project'], description: 'Aggregate matching lines into groups by this field. Mutually exclusive with group_by_dimension.' }, group_by_dimension: { type: 'string', description: 'Aggregate by SIE dimension number (e.g. "6" = projekt) from each line\'s dimensions bag; untagged → "(utan dimension)". Mutually exclusive with group_by.' }, - limit: { type: 'number', minimum: 1, maximum: 500, description: 'Max lines returned 1-500 (default 100). Totals/groups cover the FULL match set even when truncated, except under free-text search (see totals_scope).' }, + limit: { type: 'number', minimum: 1, maximum: 500, description: 'Max lines returned 1-500 (default 100). Totals/groups always cover the FULL match set even when truncated (free-text search included).' }, }, }, outputSchema: { @@ -7869,8 +7870,8 @@ export const tools: McpTool[] = [ }, totals_scope: { type: 'string', - enum: ['full_match', 'returned_slice'], - description: 'full_match: totals/groups aggregate ALL matching lines regardless of limit. returned_slice: free-text search aggregates only the returned window.', + enum: ['full_match'], + description: 'Always full_match: totals/groups aggregate ALL matching lines regardless of limit, on free-text searches too. (returned_slice is no longer emitted; the field stays for older clients.)', }, groups: { type: 'array', @@ -7951,62 +7952,22 @@ export const tools: McpTool[] = [ // dimension group, the bag filter's echo, or include_dimensions): it is // the widest column on the line and the aggregate pass fetches ALL rows. const dimsSelect = groupByDimension || includeDimensions || dimFilter.filter ? ', dimensions' : '' - // Free-text legs only. The embed survives here on purpose: each leg is - // capped at `legLimit` rows, and that cap (which drives legCapHit and - // the `truncated` signal) has no equivalent in the two-step fetch, - // which would have to pull the whole ilike match set unbounded. Every - // other pass uses fetchEntryLines: see ENTRY_COLUMNS/LINE_COLUMNS. - const DISPLAY_SELECT = `id, account_number, debit_amount, credit_amount, currency, line_description, project, cost_center${dimsSelect}, sort_order, journal_entries!inner(id, voucher_number, voucher_series, entry_date, description, notes, source_type, status, company_id)` - // Column lists for the two-step entry-lines fetch (the non-text path). - // Same fields as DISPLAY_SELECT, split across the two queries the - // helper issues; company_id is implied by the entry-side filter. + // Column lists for the two-step entry-lines fetch + // (lib/bookkeeping/entry-lines.ts), which EVERY pass uses: the plain + // query and both free-text legs. The old `journal_entries!inner` embed + // is gone on purpose: PostgREST compiled it into a correlated LATERAL + // join that walked every tenant's journal_entry_lines, and the + // free-text legs (the last users of it) were the query behind the + // daily statement timeouts (SQLSTATE 57014) in production. company_id + // is implied by the entry-side filter. const ENTRY_COLUMNS = 'id, voucher_number, voucher_series, entry_date, description, notes, source_type, status' const LINE_COLUMNS = `id, account_number, debit_amount, credit_amount, currency, line_description, project, cost_center${dimsSelect}, sort_order` - // Each query pass needs its own builder instance: PostgREST query - // builders are not reusable across awaits. The factory closes over the - // resolved filter values above and applies IDENTICAL filters for every - // projection, so display, text legs, and the aggregate pass always see - // the same match set. - const buildFilteredQuery = (select: string) => { - let q = supabase - .from('journal_entry_lines') - .select(select) - .eq('journal_entries.company_id', companyId) - - if (status === 'all') { - q = q.in('journal_entries.status', ['posted', 'reversed']) - } else { - q = q.eq('journal_entries.status', status) - } - - if (accounts && accounts.length > 0) { - q = q.in('account_number', accounts) - } else { - if (accountFrom) q = q.gte('account_number', accountFrom) - if (accountTo) q = q.lte('account_number', accountTo) - } - - if (dateFrom) q = q.gte('journal_entries.entry_date', dateFrom) - if (dateTo) q = q.lte('journal_entries.entry_date', dateTo) - - if (voucherSeries) q = q.eq('journal_entries.voucher_series', voucherSeries) - if (typeof vnFrom === 'number') q = q.gte('journal_entries.voucher_number', vnFrom) - if (typeof vnTo === 'number') q = q.lte('journal_entries.voucher_number', vnTo) - - if (sourceType) q = q.eq('journal_entries.source_type', sourceType) - - if (project) q = q.eq('project', project) - if (costCenter) q = q.eq('cost_center', costCenter) - if (dimFilter.filter) q = q.contains('dimensions', dimFilter.filter) - - return q - } - - // Same filter set as buildFilteredQuery, split for the two-step - // entry-lines fetch: entry-level predicates become plain column filters - // on journal_entries, line-level ones stay on journal_entry_lines. Keep - // the three in sync: they must always describe one match set. + // Filter set for the two-step entry-lines fetch: entry-level predicates + // are plain column filters on journal_entries, line-level ones stay on + // journal_entry_lines. Every pass (plain and both text legs) applies + // BOTH, so they always describe one match set; the text legs only add + // their .ilike() on top. const filterEntries = (q: EntryLinesQuery): EntryLinesQuery => { let e = q.eq('company_id', companyId) e = status === 'all' ? e.in('status', ['posted', 'reversed']) : e.eq('status', status) @@ -8073,25 +8034,35 @@ export const tools: McpTool[] = [ return a.id < b.id ? -1 : a.id > b.id ? 1 : 0 } - // Free-text search runs as two parallel .ilike() queries: one against - // line_description (base table) and one against journal_entries.description - // (embedded resource). PostgREST's flat .or() filter cannot span a base - // column and an embedded-resource column ("failed to parse logic tree"), - // so we issue two queries and merge by line id. Same pattern as - // lib/invoices/duplicate-payment-candidates.ts. + // Wrap a failed DB pass. The raw PostgREST/Postgres message never + // reaches the agent (it can name schemas and relations), but a + // transient failure (statement timeout 57014, connection drop) must + // still be dispatchable: the structured-error layer maps `code: + // 'TRANSIENT_ERROR'` to the registry's retryable envelope instead of + // the generic "Något gick fel" UNKNOWN_ERROR, and the message tells the + // agent what to do about it. + const sanitizeDbError = (err: unknown, safeMessage: string): Error => { + if (getStructuredError(err).code === 'TRANSIENT_ERROR') { + const out = new Error( + `${safeMessage}: the query timed out or the database was temporarily unavailable. Retry, or narrow the search with date_from/date_to.` + ) as Error & { code: string } + out.code = 'TRANSIENT_ERROR' + return out + } + return new Error(safeMessage) + } + + // Free-text search runs as two parallel two-step fetches: one matching + // journal_entries.description (entry side), one matching + // line_description (line side). PostgREST's flat .or() cannot span the + // two tables, so we issue two passes over the SAME filter set and merge + // by line id. Each pass pulls its full ilike match set (paginated by the + // helper), so totals/groups are exact for text queries too; the old + // per-leg window (legLimit/legCapHit) is gone with the embed. const text = (args.text as string | undefined)?.trim() - let data: LineRow[] = [] - let dbMatched = 0 - // Full match set (non-text path only) so totals and groups are exact - // regardless of `limit`. The free-text path stays slice-scoped (its - // per-leg windows make a full pass unbounded) and says so via - // totals_scope='returned_slice'. - let fullRows: LineRow[] | null = null - // True when at least one text-search leg filled its per-leg fetch - // window: i.e. more matches probably exist on the DB side that didn't - // make it into the merge. Drives the `truncated` signal honestly even - // when the merged distinct set fits inside `limit`. - let legCapHit = false + // Full match set for every path, so totals and groups are exact + // regardless of `limit`. + let fullRows: LineRow[] if (text) { // Length guard: defence in depth against pathological inputs even @@ -8119,58 +8090,55 @@ export const tools: McpTool[] = [ .replace(/_/g, '\\_') const pattern = `%${escaped}%` - // Fetch up to 2× limit per leg to reduce global-ordering loss when - // one leg is much more selective than the other (e.g. 150 line - // matches vs 5 entry matches with limit=100). Hard-capped at 500 - // rows per leg so a caller-supplied `limit` near its own ceiling - // can't fan out to 2× very large queries. The final post-merge - // slice still caps at `limit`; the wider per-leg window just gives - // the merge a better tail to choose from. - const legLimit = Math.min(limit * 2, 500) - - const buildLeg = (column: 'line_description' | 'journal_entries.description') => - buildFilteredQuery(DISPLAY_SELECT) - .ilike(column, pattern) - .order('entry_date', { foreignTable: 'journal_entries', ascending: false }) - .order('voucher_number', { foreignTable: 'journal_entries', ascending: false }) - .order('sort_order', { ascending: true }) - .limit(legLimit) - - const [byLine, byEntry] = await Promise.all([ - buildLeg('line_description'), - buildLeg('journal_entries.description'), - ]) - if (byLine.error || byEntry.error) { + // Leg A: entries whose description matches, then all of their lines + // (that pass the line filters). Leg B: every entry in scope, then only + // the lines whose line_description matches. Both legs are bounded by + // the entry-side filters (company, status, dates, series, source), so + // the scan is tenant-scoped and driven from journal_entries; leg B + // fetches the same entry id set the plain query would, but only the + // matching lines, so it is never more expensive than the equivalent + // query without `text`. + let byEntry: LineRow[] + let byLine: LineRow[] + try { + ;[byEntry, byLine] = await Promise.all([ + fetchEntryLines({ + supabase, + entryColumns: ENTRY_COLUMNS, + lineColumns: LINE_COLUMNS, + filterEntries: (q) => filterEntries(q).ilike('description', pattern), + filterLines, + }), + fetchEntryLines({ + supabase, + entryColumns: ENTRY_COLUMNS, + lineColumns: LINE_COLUMNS, + filterEntries, + filterLines: (q) => filterLines(q).ilike('line_description', pattern), + }), + ]) + } catch (err) { log.warn('query_journal text-search failed', { companyId, userId, - byLine: byLine.error?.message ?? null, - byEntry: byEntry.error?.message ?? null, + error: err instanceof Error ? err.message : String(err), }) - throw new Error('Database error while running text search') + throw sanitizeDbError(err, 'Database error while running text search') } + // Merge by line id: a line whose entry description AND line + // description both match comes back from both legs exactly once. const merged = new Map() - for (const row of (byLine.data ?? []) as unknown as LineRow[]) merged.set(row.id, row) - for (const row of (byEntry.data ?? []) as unknown as LineRow[]) { + for (const row of byEntry) merged.set(row.id, row) + for (const row of byLine) { if (!merged.has(row.id)) merged.set(row.id, row) } - data = Array.from(merged.values()).sort(byDisplayOrder).slice(0, limit) - - // Honest distinct-row count among what we fetched. If a leg hit its - // window cap, more distinct matches may exist; `legCapHit` carries - // that signal downstream so `truncated` isn't faked false. - dbMatched = merged.size - legCapHit = - (byLine.data?.length ?? 0) >= legLimit || - (byEntry.data?.length ?? 0) >= legLimit + fullRows = Array.from(merged.values()) } else { - // Non-text path: ONE two-step fetch (lib/bookkeeping/entry-lines.ts) - // feeds both the display slice and the full-match aggregate pass. - // The old code ran two `journal_entries!inner` embed queries here (a - // display one and a lean aggregate one), each of which PostgREST - // compiled into a correlated LATERAL join that walked every tenant's - // journal_entry_lines. The display projection is a superset of the + // Plain path: ONE two-step fetch feeds both the display slice and + // the full-match aggregate pass. The old code ran two + // `journal_entries!inner` embed queries here (a display one and a + // lean aggregate one); the display projection is a superset of the // aggregate one, so one pass over the same match set replaces both. try { fullRows = await fetchEntryLines({ @@ -8186,16 +8154,15 @@ export const tools: McpTool[] = [ userId, error: err instanceof Error ? err.message : String(err), }) - throw new Error('Database error while running journal query') + throw sanitizeDbError(err, 'Database error while running journal query') } - data = [...fullRows].sort(byDisplayOrder).slice(0, limit) - dbMatched = data.length } // Apply amount filter post-fetch: PostgREST can't OR an abs(debit) >= n // with abs(credit) >= n cleanly. Lines are debit XOR credit, so checking - // max(debit, credit) works. The SAME predicate runs over the display - // slice and the full aggregate set so both describe one match set. + // max(debit, credit) works. It runs over the full match set BEFORE the + // display slice is cut, so a limit of N returns N matching lines (not + // N minus whatever the amount filter removed from the first N). const amountMin = args.amount_min as number | undefined const amountMax = args.amount_max as number | undefined const amountFilterApplied = typeof amountMin === 'number' || typeof amountMax === 'number' @@ -8205,17 +8172,15 @@ export const tools: McpTool[] = [ if (typeof amountMax === 'number' && lineAmount > amountMax) return false return true } - const filtered = data.filter(passesAmountFilter) - const fullFiltered = fullRows ? fullRows.filter(passesAmountFilter) : null + const fullFiltered = fullRows.filter(passesAmountFilter) + const filtered = [...fullFiltered].sort(byDisplayOrder).slice(0, limit) - // Totals aggregate over the full match set when available (non-text), - // else over the returned slice (free-text): totals_scope tells the - // agent which one it got. - const totalsSource: Array<{ debit_amount: number; credit_amount: number }> = - fullFiltered ?? filtered + // Totals aggregate over the full match set on every path (text + // included): totals_scope is always 'full_match' and stays in the + // output for clients that learned to read it. let totalDebit = 0 let totalCredit = 0 - for (const r of totalsSource) { + for (const r of fullFiltered) { totalDebit += Number(r.debit_amount) || 0 totalCredit += Number(r.credit_amount) || 0 } @@ -8248,7 +8213,6 @@ export const tools: McpTool[] = [ | Array<{ key: string; debit: number; credit: number; net: number; line_count: number }> | undefined if (wantsGroups) { - const groupSource: LineRow[] = fullFiltered ?? filtered const keyOf = (r: LineRow): string => { if (groupByDimension) return r.dimensions?.[groupByDimension] ?? '(utan dimension)' switch (groupBy) { @@ -8260,7 +8224,7 @@ export const tools: McpTool[] = [ } } const bucketMap = new Map() - for (const r of groupSource) { + for (const r of fullFiltered) { const key = keyOf(r) const bucket = bucketMap.get(key) ?? { debit: 0, credit: 0, count: 0 } bucket.debit += Number(r.debit_amount) || 0 @@ -8279,36 +8243,21 @@ export const tools: McpTool[] = [ .sort((a, b) => Math.abs(b.net) - Math.abs(a.net)) } - // Non-text path: the aggregate pass IS the full match set, so - // total_lines / truncated / pre-amount count all anchor to it. Text - // path: no full pass exists: total_lines stays slice-anchored exactly - // as before (amount filter → post-filter slice; otherwise the merged - // distinct count), and legCapHit keeps `truncated` honest. - const total_lines = fullFiltered - ? fullFiltered.length - : amountFilterApplied - ? lines.length - : dbMatched - const truncated = fullFiltered - ? fullFiltered.length > lines.length - : amountFilterApplied - ? data.length >= limit && lines.length === limit - : dbMatched > lines.length || legCapHit + // The full match set anchors total_lines / truncated / pre-amount count + // on every path; `lines` is the first `limit` of it in display order. return { lines, - truncated, - total_lines, + truncated: fullFiltered.length > lines.length, + total_lines: fullFiltered.length, returned_lines: lines.length, amount_filter_applied_post_fetch: amountFilterApplied, - db_matched_pre_amount_filter: amountFilterApplied - ? (fullRows ? fullRows.length : dbMatched) - : null, + db_matched_pre_amount_filter: amountFilterApplied ? fullRows.length : null, totals: { debit: Math.round(totalDebit * 100) / 100, credit: Math.round(totalCredit * 100) / 100, net: Math.round((totalDebit - totalCredit) * 100) / 100, }, - totals_scope: fullFiltered ? 'full_match' : 'returned_slice', + totals_scope: 'full_match', ...(groups ? { groups } : {}), applied_filters: { account_from: accountFrom ?? null,