From 15df4c741ce7233fd7b5f88b1530630b5eb1e3b0 Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sun, 30 Aug 2026 16:53:45 +0200 Subject: [PATCH] feat(mcp): supplier and date filters on list_supplier_invoices (#2035) * feat(mcp): supplier and date filters on list_supplier_invoices Add supplier_id, supplier_name (case-insensitive substring, resolved server-side against the suppliers table), date_from and date_to (inclusive, on invoice_date) to gnubok_list_supplier_invoices, mirroring the v1 REST supplier-invoices filter shapes (eq on supplier_id, gte/lte on invoice_date). Unknown supplier_name returns an empty result without touching the invoice table; malformed dates and non-uuid supplier_id fail loudly with pointer messages. Schema text is kept deliberately terse: the tools/list payload guard sits at near-zero headroom, so the date format hint lives once in the tool description and the redundant status enum prose was trimmed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NtvffGr6uVk2J2Skuz6L98 * fix(mcp): bound the supplier_name match set fed to the IN clause PR Agent review flagged the unbounded id list: a broad substring on a large supplier register could build an IN clause past PostgREST URL limits. Cap name matches at 200 (cap + 1 fetched so overflow is detected) and fail loudly with a refine hint instead of degrading. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NtvffGr6uVk2J2Skuz6L98 * fix(mcp): loud validation, literal name matching, truncation signal on list_supplier_invoices Skeptic review refuted three behaviors; all three fixed: 1. Silent truncation: date filters invite period reconciliation but the 50-row cap was unsignalled. The invoice query now uses count exact and returns total_count and has_more (same contract as list_invoices), with a unique-id order tiebreaker so identical calls return identical subsets. 2. Silent filter drop: non-string values for the new params (and blank supplier_name) fell through typeof guards and returned the UNFILTERED ledger presented as filtered, the same class arg-guard exists for. They now throw clear errors. Dates are also calendar-validated, so 2026-02-30 fails loudly instead of as a raw Postgres cast error. 3. Wildcard broadening: PostgREST rewrites * in ilike values to %, so Star*Mart matched Starke Martinsson AB. supplier_name is now matched as a literal case-insensitive substring in JS over the company's suppliers (fetchAllRows, parity with list_suppliers), cap unchanged. Two redundant property descriptions dropped to fund the outputSchema additions under the tools/list payload ceiling. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NtvffGr6uVk2J2Skuz6L98 --------- Co-authored-by: Claude Fable 5 --- .../__tests__/list-supplier-invoices.test.ts | 273 ++++++++++++++++++ extensions/general/mcp-server/server.ts | 117 +++++++- 2 files changed, 385 insertions(+), 5 deletions(-) create mode 100644 extensions/general/mcp-server/__tests__/list-supplier-invoices.test.ts diff --git a/extensions/general/mcp-server/__tests__/list-supplier-invoices.test.ts b/extensions/general/mcp-server/__tests__/list-supplier-invoices.test.ts new file mode 100644 index 00000000..d922b5c7 --- /dev/null +++ b/extensions/general/mcp-server/__tests__/list-supplier-invoices.test.ts @@ -0,0 +1,273 @@ +import { describe, expect, it } from 'vitest' +import { createQueuedMockSupabase } from '@/tests/helpers' +import { tools } from '../server' + +const tool = () => tools.find((candidate) => candidate.name === 'gnubok_list_supplier_invoices')! + +const COMPANY = 'company-1' +const USER = 'user-1' +const SUPPLIER_UUID = '11111111-2222-4333-8444-555555555555' + +const invoiceRow = (overrides: Record = {}) => ({ + id: 'si-1', + supplier_invoice_number: 'F-1001', + invoice_date: '2026-03-10', + due_date: '2026-04-09', + status: 'approved', + total: 1250, + total_sek: 1250, + currency: 'SEK', + vat_treatment: 'standard', + remaining_amount: 1250, + default_dimensions: null, + supplier: { id: SUPPLIER_UUID, name: 'Office Depot AB' }, + ...overrides, +}) + +type ListResult = { + invoices: Array> + count: number + total_count: number + has_more: boolean +} + +describe('gnubok_list_supplier_invoices: filters', () => { + it('filters by supplier_id alone (eq on supplier_id, no supplier lookup)', async () => { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ data: [invoiceRow()], count: 1 }) + + const result = (await tool().execute( + { supplier_id: SUPPLIER_UUID }, + COMPANY, + USER, + supabase as never, + )) as ListResult + + expect(result.count).toBe(1) + expect(result.total_count).toBe(1) + expect(result.has_more).toBe(false) + expect(result.invoices[0].id).toBe('si-1') + const eqCalls = findCalls('supplier_invoices', 'eq') + expect(eqCalls).toContainEqual(['supplier_id', SUPPLIER_UUID]) + // Resolving supplier_name is the only reason to touch suppliers. + expect(findCalls('suppliers', 'select')).toHaveLength(0) + expect(findCalls('supplier_invoices', 'gte')).toHaveLength(0) + expect(findCalls('supplier_invoices', 'lte')).toHaveLength(0) + }) + + it('rejects a non-uuid supplier_id with a pointer to supplier_name', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute({ supplier_id: 'Office Depot' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/supplier UUID.*supplier_name/) + }) + + it('rejects non-string filter values instead of silently returning the unfiltered list', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute({ date_from: 20260101 }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_from must be a string/) + await expect( + tool().execute({ supplier_name: ['Office Depot'] }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/supplier_name must be a string/) + await expect( + tool().execute({ supplier_id: 42 }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/supplier_id must be a string/) + await expect( + tool().execute({ date_to: null }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_to must be a string/) + }) + + it('rejects a blank supplier_name instead of dropping the filter', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute({ supplier_name: ' ' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/supplier_name must not be blank/) + }) + + it('filters by supplier_name alone: literal case-insensitive substring resolved to supplier ids', async () => { + const { supabase, enqueue, findCall, findCalls } = createQueuedMockSupabase() + enqueue({ + data: [ + { id: 'sup-1', name: 'Office Depot AB' }, + { id: 'sup-2', name: 'Nordic OFFICE Supplies' }, + { id: 'sup-3', name: 'Byggmax AB' }, + ], + }) // suppliers fetch + enqueue({ data: [invoiceRow()], count: 1 }) // invoice list + + const result = (await tool().execute( + { supplier_name: 'office' }, + COMPANY, + USER, + supabase as never, + )) as ListResult + + expect(result.count).toBe(1) + expect(findCall('suppliers', 'select')).toEqual(['id, name']) + expect(findCalls('suppliers', 'eq')).toContainEqual(['company_id', COMPANY]) + expect(findCalls('supplier_invoices', 'in')).toContainEqual([ + 'supplier_id', + ['sup-1', 'sup-2'], + ]) + }) + + it('matches supplier_name literally: * and % are not wildcards', async () => { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ + data: [ + { id: 'sup-1', name: 'Star*Mart' }, + { id: 'sup-2', name: 'Starke Martinsson AB' }, + { id: 'sup-3', name: '100%_AB' }, + ], + }) + enqueue({ data: [], count: 0 }) + + await tool().execute({ supplier_name: 'Star*Mart' }, COMPANY, USER, supabase as never) + + expect(findCalls('supplier_invoices', 'in')).toContainEqual(['supplier_id', ['sup-1']]) + }) + + it('unknown supplier_name yields an empty result without querying invoices', async () => { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ data: [{ id: 'sup-1', name: 'Byggmax AB' }] }) // no substring match + + const result = (await tool().execute( + { supplier_name: 'no such vendor' }, + COMPANY, + USER, + supabase as never, + )) as ListResult + + expect(result).toEqual({ invoices: [], count: 0, total_count: 0, has_more: false }) + expect(findCalls('supplier_invoices', 'select')).toHaveLength(0) + }) + + it('rejects a supplier_name matching more suppliers than the cap', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ + data: Array.from({ length: 201 }, (_, i) => ({ id: `sup-${i}`, name: `Byrå AB ${i}` })), + }) + + await expect( + tool().execute({ supplier_name: 'byrå' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/matches more than 200 suppliers/) + }) + + it('filters by date_from alone (gte on invoice_date)', async () => { + const { supabase, enqueue, findCall, findCalls } = createQueuedMockSupabase() + enqueue({ data: [], count: 0 }) + + await tool().execute({ date_from: '2026-01-01' }, COMPANY, USER, supabase as never) + + expect(findCall('supplier_invoices', 'gte')).toEqual(['invoice_date', '2026-01-01']) + expect(findCalls('supplier_invoices', 'lte')).toHaveLength(0) + }) + + it('filters by date_to alone (lte on invoice_date)', async () => { + const { supabase, enqueue, findCall, findCalls } = createQueuedMockSupabase() + enqueue({ data: [], count: 0 }) + + await tool().execute({ date_to: '2026-06-30' }, COMPANY, USER, supabase as never) + + expect(findCall('supplier_invoices', 'lte')).toEqual(['invoice_date', '2026-06-30']) + expect(findCalls('supplier_invoices', 'gte')).toHaveLength(0) + }) + + it('combines status, supplier_id, supplier_name and date range', async () => { + const { supabase, enqueue, findCall, findCalls } = createQueuedMockSupabase() + enqueue({ data: [{ id: SUPPLIER_UUID, name: 'Office Depot AB' }] }) // suppliers fetch + enqueue({ data: [invoiceRow()], count: 1 }) // invoice list + + const result = (await tool().execute( + { + status: 'to_pay', + supplier_id: SUPPLIER_UUID, + supplier_name: 'office', + date_from: '2026-01-01', + date_to: '2026-12-31', + }, + COMPANY, + USER, + supabase as never, + )) as ListResult + + expect(result.count).toBe(1) + const inCalls = findCalls('supplier_invoices', 'in') + expect(inCalls).toContainEqual(['status', ['approved', 'overdue']]) + expect(inCalls).toContainEqual(['supplier_id', [SUPPLIER_UUID]]) + expect(findCalls('supplier_invoices', 'eq')).toContainEqual(['supplier_id', SUPPLIER_UUID]) + expect(findCall('supplier_invoices', 'gte')).toEqual(['invoice_date', '2026-01-01']) + expect(findCall('supplier_invoices', 'lte')).toEqual(['invoice_date', '2026-12-31']) + // Deterministic order: due_date with the unique id as tiebreaker. + const orderCalls = findCalls('supplier_invoices', 'order') + expect(orderCalls).toContainEqual(['due_date', { ascending: true }]) + expect(orderCalls).toContainEqual(['id', { ascending: true }]) + }) + + it('rejects a malformed date', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute({ date_from: '01/02/2026' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_from must be a valid ISO date/) + await expect( + tool().execute({ date_to: '2026-6-1' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_to must be a valid ISO date/) + }) + + it('rejects an impossible calendar date that passes the shape regex', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute({ date_from: '2026-02-30' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_from must be a valid ISO date/) + await expect( + tool().execute({ date_to: '2026-13-01' }, COMPANY, USER, supabase as never), + ).rejects.toThrow(/date_to must be a valid ISO date/) + }) + + it('rejects a reversed date range', async () => { + const { supabase } = createQueuedMockSupabase() + await expect( + tool().execute( + { date_from: '2026-12-31', date_to: '2026-01-01' }, + COMPANY, + USER, + supabase as never, + ), + ).rejects.toThrow(/date_from must not be after date_to/) + }) + + it('signals truncation: total_count and has_more expose rows past the limit', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ + data: Array.from({ length: 50 }, (_, i) => invoiceRow({ id: `si-${i}` })), + count: 60, + }) + + const result = (await tool().execute( + { date_from: '2026-01-01', date_to: '2026-03-31' }, + COMPANY, + USER, + supabase as never, + )) as ListResult + + expect(result.count).toBe(50) + expect(result.total_count).toBe(60) + expect(result.has_more).toBe(true) + }) + + it('keeps the unfiltered path unchanged: no supplier lookup, no date filters', async () => { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ data: [invoiceRow(), invoiceRow({ id: 'si-2' })], count: 2 }) + + const result = (await tool().execute({}, COMPANY, USER, supabase as never)) as ListResult + + expect(result.count).toBe(2) + expect(result.total_count).toBe(2) + expect(result.has_more).toBe(false) + expect(findCalls('suppliers', 'select')).toHaveLength(0) + expect(findCalls('supplier_invoices', 'gte')).toHaveLength(0) + expect(findCalls('supplier_invoices', 'lte')).toHaveLength(0) + expect(findCalls('supplier_invoices', 'in')).toHaveLength(0) + }) +}) diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index faf7c431..0f87349f 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -7583,16 +7583,19 @@ export const tools: McpTool[] = [ { name: 'gnubok_list_supplier_invoices', title: 'List Supplier Invoices', - description: 'List supplier invoices (leverantörsfakturor), sorted by due date. Optional status filter; "to_pay" combines approved+overdue.', + description: 'List supplier invoices (leverantörsfakturor), sorted by due date. Filters: status (to_pay = approved+overdue), supplier_id, supplier_name (substring), date_from/date_to on invoice_date (YYYY-MM-DD).', inputSchema: { type: 'object', additionalProperties: false, properties: { status: { type: 'string', - description: 'Filter: registered, approved, overdue, paid, to_pay, all (default)', enum: ['registered', 'approved', 'overdue', 'paid', 'to_pay', 'all'], }, + supplier_id: { type: 'string' }, + supplier_name: { type: 'string' }, + date_from: { type: 'string' }, + date_to: { type: 'string' }, limit: { type: 'number', description: 'Max results 1-100 (default 50)' }, }, }, @@ -7602,6 +7605,8 @@ export const tools: McpTool[] = [ properties: { invoices: { type: 'array', items: { type: 'object' } }, count: { type: 'number' }, + total_count: { type: 'number' }, + has_more: { type: 'boolean' }, }, required: ['invoices', 'count'], }, @@ -7615,9 +7620,92 @@ export const tools: McpTool[] = [ const limit = Math.min(Math.max(1, Number(args.limit) || 50), 100) const status = (args.status as string) || 'all' + // Loud validation: a mistyped filter must never silently degrade to the + // UNFILTERED list (the silent-drop failure class arg-guard exists for, + // feedback seq 261545). Hosts do not reliably enforce inputSchema types, + // so non-string values reach execute() and are rejected here. + for (const key of ['supplier_id', 'supplier_name', 'date_from', 'date_to'] as const) { + if (args[key] !== undefined && typeof args[key] !== 'string') { + throw new Error(`${key} must be a string.`) + } + } + const supplierId = args.supplier_id as string | undefined + const supplierName = (args.supplier_name as string | undefined)?.trim() + const dateFrom = args.date_from as string | undefined + const dateTo = args.date_to as string | undefined + + if (supplierName === '') { + throw new Error('supplier_name must not be blank.') + } + // Clear message instead of a raw Postgres uuid cast error: an agent may + // well pass a supplier's name here; point it at supplier_name instead. + if (supplierId && !/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(supplierId)) { + throw new Error('supplier_id must be a supplier UUID (suppliers.id). To filter by name, use supplier_name.') + } + for (const [key, value] of [['date_from', dateFrom], ['date_to', dateTo]] as const) { + if (value === undefined) continue + // Shape AND calendar validity: '2026-02-30' must fail here with a + // clear message, not as a raw Postgres date-cast error downstream. + const parsed = new Date(`${value}T00:00:00Z`) + if ( + !ISO_DATE_RE.test(value) || + Number.isNaN(parsed.getTime()) || + parsed.toISOString().slice(0, 10) !== value + ) { + throw new Error(`${key} must be a valid ISO date (YYYY-MM-DD).`) + } + } + if (dateFrom && dateTo && dateFrom > dateTo) { + throw new Error('date_from must not be after date_to.') + } + + // supplier_name resolves server-side to supplier ids: a LITERAL + // case-insensitive substring matched in JS over the company's suppliers + // (fetchAllRows, same shape as gnubok_list_suppliers). Not ilike: + // PostgREST rewrites `*` in like/ilike values to `%`, so a DB-side + // pattern silently broadens names containing `*` and there is no + // server-side escape for it. Archived suppliers are included on + // purpose: their invoices still exist. The match set is bounded: an + // unbounded id list fed to .in() below could exceed PostgREST URL + // limits, so past the cap the tool fails loudly with a refine hint + // instead of degrading. + const NAME_MATCH_CAP = 200 + let nameMatchedSupplierIds: string[] | undefined + if (supplierName) { + const needle = supplierName.toLowerCase() + let suppliers: { id: string; name: string | null }[] + try { + suppliers = await fetchAllRows<{ id: string; name: string | null }>(({ from, to }) => + supabase + .from('suppliers') + .select('id, name') + .eq('company_id', companyId) + .order('id', { ascending: true }) + .range(from, to) + ) + } catch (error) { + throw dbError(error) + } + const matchedIds = suppliers + .filter((s) => (s.name ?? '').toLowerCase().includes(needle)) + .map((s) => s.id) + if (matchedIds.length === 0) { + return { invoices: [], count: 0, total_count: 0, has_more: false } + } + if (matchedIds.length > NAME_MATCH_CAP) { + throw new Error( + `supplier_name matches more than ${NAME_MATCH_CAP} suppliers; refine the name or use supplier_id.`, + ) + } + nameMatchedSupplierIds = matchedIds + } + + // count: 'exact' so truncation is SIGNALLED (total_count/has_more): the + // date filters invite period reconciliation, and a silently capped + // listing reads as complete (same contract as gnubok_list_invoices). let query = supabase .from('supplier_invoices') - .select('id, supplier_invoice_number, invoice_date, due_date, status, total, total_sek, currency, vat_treatment, remaining_amount, default_dimensions, supplier:suppliers(id, name)') + .select('id, supplier_invoice_number, invoice_date, due_date, status, total, total_sek, currency, vat_treatment, remaining_amount, default_dimensions, supplier:suppliers(id, name)', { count: 'exact' }) .eq('company_id', companyId) if (status !== 'all') { @@ -7628,11 +7716,30 @@ export const tools: McpTool[] = [ } } - const { data, error } = await query.order('due_date', { ascending: true }).limit(limit) + // Same filter shapes as the v1 supplier-invoices list: eq on + // supplier_id, gte/lte on invoice_date. + if (supplierId) query = query.eq('supplier_id', supplierId) + if (nameMatchedSupplierIds) query = query.in('supplier_id', nameMatchedSupplierIds) + if (dateFrom) query = query.gte('invoice_date', dateFrom) + if (dateTo) query = query.lte('invoice_date', dateTo) + + // Tiebreak on the unique id: due_date alone leaves which rows fall past + // the cap nondeterministic between identical calls. + const { data, error, count } = await query + .order('due_date', { ascending: true }) + .order('id', { ascending: true }) + .limit(limit) if (error) throw dbError(error) - return { invoices: data ?? [], count: data?.length ?? 0 } + const invoices = data ?? [] + const totalCount = count ?? invoices.length + return { + invoices, + count: invoices.length, + total_count: totalCount, + has_more: totalCount > invoices.length, + } }, },