From e53b478833618897fdfdbd24a5c0b50c44cc794b Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sun, 23 Aug 2026 00:13:30 +0200 Subject: [PATCH] fix(mcp): query_journal accepts scalar/number accounts instead of silently dropping the filter (#1803) * fix(mcp): query_journal accepts scalar/number accounts instead of silently dropping the filter Hosts don't always enforce inputSchema. `accounts: 1630` (JSON number) has no `.length`, so the account filter was skipped while applied_filters still echoed it, returning every account's lines as if filtered. A bare string "1630" was spread into its digits by postgrest-js `.in()` and matched nothing. Normalize accounts (array / bare string / comma list / integer) to string[] and account_from/account_to to strings; reject values that aren't account numbers with a clear error instead of ignoring them. Tests assert the actual `.in('account_number', [...])` predicate, not just the echo. Co-Authored-By: Claude Fable 5 * fix(mcp): reuse shared ACCOUNT_NUMBER_RE, drop null entries, normalize tag_journal_lines filters Review fixes: the local ACCOUNT_NUMBER_RE collided with the import from lib/invariants/account-number (build break); null/undefined array entries now drop out instead of reaching .in(); gnubok_tag_journal_lines (a bulk write path) gets the same accounts/account_from/account_to normalization so a mangled filter fails fast instead of widening the retag scope. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .../__tests__/query-journal.test.ts | 95 +++++++++++++++++++ .../__tests__/tag-journal-lines.test.ts | 37 ++++++++ extensions/general/mcp-server/server.ts | 62 ++++++++++-- 3 files changed, 188 insertions(+), 6 deletions(-) diff --git a/extensions/general/mcp-server/__tests__/query-journal.test.ts b/extensions/general/mcp-server/__tests__/query-journal.test.ts index 118f91b6..83cd95d2 100644 --- a/extensions/general/mcp-server/__tests__/query-journal.test.ts +++ b/extensions/general/mcp-server/__tests__/query-journal.test.ts @@ -769,3 +769,98 @@ describe('gnubok_query_journal: free-text search', () => { ).rejects.not.toThrow(/private_internal/) }) }) + +/** + * Hosts don't always enforce inputSchema, so `accounts` arrives as a bare + * number or string in the wild. Before the normalizer, `accounts: 1630` + * silently dropped the filter (a number has no `.length`) while + * applied_filters still echoed it, and `accounts: "1630"` was spread into its + * digits by `.in()`. Record every `.in()` call on journal_entry_lines so the + * assertion is on the actual predicate, not just the echo. + */ +function makeRecordingEntryLinesMock() { + const inCalls: Array<{ table: string; column: string; values: unknown }> = [] + // One parent entry so the two-step fetch actually issues the lines query + // (fetchEntryLines short-circuits on zero entries); the lines page is empty. + const rowsFor = (table: string) => + table === 'journal_entries' + ? [{ id: 'je-1', entry_date: '2026-01-15', voucher_series: 'A', voucher_number: 1, status: 'posted' }] + : [] + const chain = (table: string): unknown => + new Proxy( + {}, + { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => + resolve({ data: rowsFor(table), error: null, count: rowsFor(table).length }) + } + if (prop === 'range') return () => ({ data: rowsFor(table), error: null, count: rowsFor(table).length }) + if (prop === 'in') { + return (column: string, values: unknown) => { + inCalls.push({ table, column, values }) + return chain(table) + } + } + return () => chain(table) + }, + }, + ) + const supabase = { from: vi.fn().mockImplementation((table: string) => chain(table)) } as never + return { supabase, inCalls } +} + +describe('gnubok_query_journal: accounts argument normalization', () => { + const tool = () => tools.find((t) => t.name === 'gnubok_query_journal')! + const lineAccountFilters = (calls: Array<{ table: string; column: string; values: unknown }>) => + calls.filter((c) => c.table === 'journal_entry_lines' && c.column === 'account_number') + + it('applies a bare number `accounts: 1630` as ["1630"] and echoes the normalized value', async () => { + const { supabase, inCalls } = makeRecordingEntryLinesMock() + const result = (await tool().execute( + { accounts: 1630 }, + 'company-1', 'user-1', supabase, + )) as { applied_filters: { accounts: unknown } } + + const filters = lineAccountFilters(inCalls) + expect(filters.length).toBeGreaterThan(0) + for (const f of filters) expect(f.values).toEqual(['1630']) + expect(result.applied_filters.accounts).toEqual(['1630']) + }) + + it('applies a bare string `accounts: "1630"` as ["1630"], not as its digits', async () => { + const { supabase, inCalls } = makeRecordingEntryLinesMock() + await tool().execute({ accounts: '1630' }, 'company-1', 'user-1', supabase) + const filters = lineAccountFilters(inCalls) + expect(filters.length).toBeGreaterThan(0) + for (const f of filters) expect(f.values).toEqual(['1630']) + }) + + it('accepts a comma-separated string and integer array items', async () => { + const { supabase, inCalls } = makeRecordingEntryLinesMock() + await tool().execute({ accounts: '1930, 1940' }, 'company-1', 'user-1', supabase) + expect(lineAccountFilters(inCalls)[0]?.values).toEqual(['1930', '1940']) + + const second = makeRecordingEntryLinesMock() + await tool().execute({ accounts: [1930, '1940'] }, 'company-1', 'user-1', second.supabase) + expect(lineAccountFilters(second.inCalls)[0]?.values).toEqual(['1930', '1940']) + }) + + it('rejects non-account values instead of dropping the filter', async () => { + const { supabase } = makeRecordingEntryLinesMock() + await expect( + tool().execute({ accounts: 'kontorsmaterial' }, 'company-1', 'user-1', supabase), + ).rejects.toThrow(/accounts must be an account number/) + await expect( + tool().execute({ account_from: 4000 as unknown as string, account_to: { x: 1 } }, 'company-1', 'user-1', supabase), + ).rejects.toThrow(/account_to must be an account number/) + }) + + it('still caps accounts at 50 after normalization', async () => { + const { supabase } = makeRecordingEntryLinesMock() + const accounts = Array.from({ length: 51 }, (_, i) => 1000 + i).join(',') + await expect( + tool().execute({ accounts }, 'company-1', 'user-1', supabase), + ).rejects.toThrow(/capped at 50/) + }) +}) diff --git a/extensions/general/mcp-server/__tests__/tag-journal-lines.test.ts b/extensions/general/mcp-server/__tests__/tag-journal-lines.test.ts index dba1ce26..8796b36d 100644 --- a/extensions/general/mcp-server/__tests__/tag-journal-lines.test.ts +++ b/extensions/general/mcp-server/__tests__/tag-journal-lines.test.ts @@ -132,6 +132,43 @@ describe('gnubok_tag_journal_lines: filter gates', () => { ).rejects.toThrow(/Inga bokförda rader matchade filtret[\s\S]*gnubok_query_journal/) }) + it('rejects non-account filter values instead of silently widening the retag scope', async () => { + // Hosts don't always enforce inputSchema. A mangled account filter on a + // bulk WRITE path must fail fast, never fall through to "no account + // filter" and retag more lines than asked. + const { supabase } = createQueuedMockSupabase() + await expect( + tagJournalLines.execute( + { dimensions: { '6': 'P01' }, reason: 'Retro-taggning', filters: { accounts: 'kontorsmaterial' } }, + 'company-1', + 'user-1', + supabase as never, + ), + ).rejects.toThrow(/accounts must be an account number/) + await expect( + tagJournalLines.execute( + { dimensions: { '6': 'P01' }, reason: 'Retro-taggning', filters: { account_from: { x: 1 } } }, + 'company-1', + 'user-1', + supabase as never, + ), + ).rejects.toThrow(/filters\.account_from must be an account number/) + }) + + it('accepts a bare number for filters.accounts and applies it as a filter', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: { dimensions_enabled: false }, error: null }) + enqueue({ data: [], error: null }) // entry match query runs, so normalization passed + await expect( + tagJournalLines.execute( + { dimensions: { '6': 'P01' }, reason: 'Retro-taggning', filters: { accounts: 4010 } }, + 'company-1', + 'user-1', + supabase as never, + ), + ).rejects.toThrow(/Inga bokförda rader matchade filtret/) + }) + it('throws asking to narrow the filter when more than 500 lines match', async () => { const { supabase, enqueue } = createQueuedMockSupabase() enqueue({ data: { dimensions_enabled: false }, error: null }) diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 9184669d..a1716523 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -590,6 +590,46 @@ const AUTO_PERIOD_DATE_KEYS = [ const ISO_DATE_RE = /^\d{4}-\d{2}-\d{2}$/ +/** + * Coerce a single account-number argument to a trimmed string (validated + * against ACCOUNT_NUMBER_RE from lib/invariants/account-number). Accepts a + * string or a finite integer (hosts that skip inputSchema validation send + * `1930` as a JSON number); everything else is a validation error rather than + * a silently dropped filter. + */ +function normalizeAccountNumber(value: unknown, field: string): string | undefined { + if (value === undefined || value === null || value === '') return undefined + const str = + typeof value === 'number' && Number.isInteger(value) + ? String(value) + : typeof value === 'string' + ? value.trim() + : null + if (str === null || !ACCOUNT_NUMBER_RE.test(str)) { + throw new Error(`${field} must be an account number string like "1930" (got ${JSON.stringify(value)})`) + } + return str +} + +/** + * Coerce an `accounts` argument to string[]. Accepts an array of + * strings/integers, a bare string ("1930" or comma-separated "1930,1940"), or + * a bare integer. Returns undefined for null/empty so callers fall through to + * account_from/account_to. + */ +function normalizeAccountList(value: unknown): string[] | undefined { + if (value === undefined || value === null) return undefined + const raw: unknown[] = Array.isArray(value) + ? value + : typeof value === 'string' + ? value.split(',') + : [value] + const list = raw + .filter((v) => v != null && !(typeof v === 'string' && v.trim() === '')) + .map((v) => normalizeAccountNumber(v, 'accounts') as string) + return list.length > 0 ? list : undefined +} + function autoExtractDateForPeriodCheck(params: Record): string | undefined { for (const key of AUTO_PERIOD_DATE_KEYS) { const value = params[key] @@ -7291,12 +7331,16 @@ export const tools: McpTool[] = [ // ── Filters: validated before any DB work so bad input fails fast. const filters = (args.filters && typeof args.filters === 'object' ? args.filters : {}) as Record - const accounts = Array.isArray(filters.accounts) ? (filters.accounts as string[]) : undefined + // Same normalization as gnubok_query_journal: a bare number or string + // must narrow the match set, never be silently dropped. This is a bulk + // WRITE path (retags up to 500 posted lines), so a dropped account + // filter would widen the scope of the retag. + const accounts = normalizeAccountList(filters.accounts) if (accounts && accounts.length > 50) { throw new Error('filters.accounts is capped at 50: use account_from/account_to for ranges') } - const accountFrom = typeof filters.account_from === 'string' ? filters.account_from : undefined - const accountTo = typeof filters.account_to === 'string' ? filters.account_to : undefined + const accountFrom = normalizeAccountNumber(filters.account_from, 'filters.account_from') + const accountTo = normalizeAccountNumber(filters.account_to, 'filters.account_to') const dateFrom = typeof filters.date_from === 'string' ? filters.date_from : undefined const dateTo = typeof filters.date_to === 'string' ? filters.date_to : undefined const text = typeof filters.text === 'string' ? filters.text.trim() : '' @@ -7856,9 +7900,15 @@ export const tools: McpTool[] = [ async execute(args, companyId, userId, supabase) { const limit = Math.min(Math.max(1, Number(args.limit) || 100), 500) const status = (args.status as string) || 'posted' - const accounts = args.accounts as string[] | undefined - const accountFrom = args.account_from as string | undefined - const accountTo = args.account_to as string | undefined + // Hosts don't always enforce inputSchema: `accounts: 1630` (a bare + // number) or `accounts: "1630"` both reached us as-is. A number has no + // `.length`, so the filter was silently skipped while applied_filters + // still echoed it; a bare string was spread into its digits by + // postgrest-js `.in()` and matched nothing. Normalize every shape to a + // string[] and reject anything that isn't an account number. + const accounts = normalizeAccountList(args.accounts) + const accountFrom = normalizeAccountNumber(args.account_from, 'account_from') + const accountTo = normalizeAccountNumber(args.account_to, 'account_to') if (accounts && accounts.length > 50) { throw new Error('accounts list capped at 50: use account_from/account_to for ranges')