From 2a33291c18548d9aac7c8b9325c8168fa5395ed3 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg Date: Tue, 25 Aug 2026 10:44:30 +0200 Subject: [PATCH] fix(mcp): bulk-book titles that say what is approved, reject unknown parameters (#1856) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reports from the 08-24 feedback sweep (seq 261545): - The bulk-book queue title (Samlingsverifikation: 1 transaktioner 2026-07-22) carried no amount, direction or counterparty; the CEO approving from a phone could not tell what he authorised. Titles now read: Samlingsverifikation -1 000,00 SEK 2026-05-12: NORDNET UTTAG (+1 till). Same per-tx text the categorize titles already carry; preview_data stays aggregate-only. - gnubok_query_journal called with {query} instead of {text} silently returned the whole journal. tools/call now rejects unknown top-level parameters for every tool (all schemas declare additionalProperties: false) with a VALIDATION_ERROR that lists the valid keys. company_id stays tolerated everywhere. codedError is exported from company-routing for the dispatcher. The copy fixes this branch originally carried (scope-honest list_pending_operations, BFL 5 kap 6 § on create_voucher, bank-movement only on categorize/bulk_book) landed independently in #1844 and were dropped on rebase; no catalog token change remains. Claude-Session: https://claude.ai/code/session_01ScVhg6XsDtNXkiEQNV7LaZ Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- DECISIONS.md | 1 + .../mcp-server/__tests__/arg-guard.test.ts | 45 ++++++++ .../__tests__/dimension-tools.test.ts | 58 ++++++++++ .../__tests__/unknown-args-rejected.test.ts | 108 ++++++++++++++++++ extensions/general/mcp-server/arg-guard.ts | 28 +++++ .../general/mcp-server/company-routing.ts | 2 +- extensions/general/mcp-server/server.ts | 37 +++++- 7 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 extensions/general/mcp-server/__tests__/arg-guard.test.ts create mode 100644 extensions/general/mcp-server/__tests__/unknown-args-rejected.test.ts create mode 100644 extensions/general/mcp-server/arg-guard.ts diff --git a/DECISIONS.md b/DECISIONS.md index 6df50740..f1c3b382 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1201,3 +1201,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-08-25] A period klarmarkerad as closed in a previous system (closed_externally) no longer trips the trial balance's "closed without closing_entry_id" guard for statutory pre-closing balances: its closing verifikat never existed in these books, so the booked balances are the pre-closing balances and there is nothing to strip. The guard stays for periods our own engine closed, where a missing link is a real inconsistency. Found by Väla Redovisning: Klarmarkera + Årsredovisning = 500. [2026-08-24] fiscal_periods.previous_period_id is adjacency-only: findNextPeriod ignores a chained period that does not start the day after the current one, and SIE import only wires predecessor/successor links between date-adjacent periods (before: nearest period across any gap). A non-adjacent link is what sent a company's opening balances two years forward (feedback seq 249297); 40 such links exist on prod across 39 companies and are neutralized by the read-side guard, not repaired in this change. A gap in the chain means a missing räkenskapsår (BFL 3 kap), which reports should show as missing rather than bridge silently. [2026-08-24] Pending-operation authorization refusals (401/403 from an executor) release the claim back to 'pending' instead of consuming the op as 'rejected': the refusal happens before any side-effect and reflects the credential, not the booking, so the same op must survive for an authorized approver (/pending UI or a scoped key). Every CommitResult now carries operation_status so agents stop inferring "consumed" from status 'failed'. Deterministic content errors (400) still consume the op: re-staging is the only fix for those. +[2026-08-24] tools/call rejects unknown top-level parameters (VALIDATION_ERROR naming the valid keys) instead of ignoring them: hosts do not reliably enforce inputSchema, and a misspelled key silently widened gnubok_query_journal to the whole journal (feedback seq 261545). company_id stays tolerated on every tool because the routing layer owns it. Chose server-side enforcement over per-tool presence guards: every schema already declares additionalProperties:false, so the contract exists, it just was not enforced. diff --git a/extensions/general/mcp-server/__tests__/arg-guard.test.ts b/extensions/general/mcp-server/__tests__/arg-guard.test.ts new file mode 100644 index 00000000..0802f6af --- /dev/null +++ b/extensions/general/mcp-server/__tests__/arg-guard.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from 'vitest' +import { findUnknownArgKeys, listArgKeys } from '../arg-guard' +import { tools } from '../server' + +describe('findUnknownArgKeys', () => { + const schema = { + type: 'object', + additionalProperties: false, + properties: { text: { type: 'string' }, limit: { type: 'number' } }, + } + + it('flags keys the schema does not declare', () => { + expect(findUnknownArgKeys(schema, { query: 'moms', limit: 5 })).toEqual(['query']) + }) + + it('returns nothing for a well-formed call', () => { + expect(findUnknownArgKeys(schema, { text: 'moms' })).toEqual([]) + expect(findUnknownArgKeys(schema, {})).toEqual([]) + }) + + it('tolerates company_id everywhere (the routing layer owns it)', () => { + expect(findUnknownArgKeys(schema, { text: 'x', company_id: 'c-1' })).toEqual([]) + }) + + it('is inert for a schema that allows additional properties', () => { + expect(findUnknownArgKeys({ type: 'object', additionalProperties: true }, { anything: 1 })).toEqual([]) + }) + + it('lists the declared keys for the error message', () => { + expect(listArgKeys(schema)).toEqual(['text', 'limit']) + expect(listArgKeys({ type: 'object' })).toEqual([]) + }) + + it('would have caught the reported gnubok_query_journal misspelling', () => { + // Feedback seq 261545: {query: "..."} instead of {text: "..."} returned the + // whole journal (7321 rows) with applied_filters.text null. + const queryJournal = tools.find((t) => t.name === 'gnubok_query_journal')! + const unknown = findUnknownArgKeys( + queryJournal.inputSchema as Record, + { query: 'hyra' }, + ) + expect(unknown).toEqual(['query']) + expect(listArgKeys(queryJournal.inputSchema as Record)).toContain('text') + }) +}) diff --git a/extensions/general/mcp-server/__tests__/dimension-tools.test.ts b/extensions/general/mcp-server/__tests__/dimension-tools.test.ts index 7bdf6ed0..86925a68 100644 --- a/extensions/general/mcp-server/__tests__/dimension-tools.test.ts +++ b/extensions/general/mcp-server/__tests__/dimension-tools.test.ts @@ -955,3 +955,61 @@ describe('gnubok_bulk_book_transactions: dimensions bag', () => { expect(supabase.from).not.toHaveBeenCalled() }) }) + +describe('gnubok_bulk_book_transactions: approval-queue title', () => { + // Feedback seq 261545: "Samlingsverifikation: 1 transaktioner 2026-07-22" + // carried no amount, direction or counterparty, so the CEO approving from a + // phone could not tell what he was authorising. + it('carries direction, amount, currency, date and the lead counterparty', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + const inserts = captureInserts(supabase) + enqueue({ data: { dimensions_enabled: true }, error: null }) + enqueue({ data: null, error: null }) + enqueue({ data: REGISTRY_ROWS, error: null }) + enqueue({ data: VALUE_ROWS, error: null }) + enqueue({ + data: [ + { id: 'tx-1', amount: -400, currency: 'SEK', date: '2026-05-12', journal_entry_id: null, description: 'NORDNET UTTAG', merchant_name: null }, + { id: 'tx-2', amount: -600, currency: 'SEK', date: '2026-05-12', journal_entry_id: null, description: 'NORDNET UTTAG', merchant_name: null }, + ], + error: null, + }) + enqueue({ + data: [ + { account_number: '4010', account_name: 'Inköp material och varor' }, + { account_number: '1930', account_name: 'Företagskonto' }, + ], + error: null, + }) + enqueue({ data: null, error: null }) + enqueue({ data: null, error: null }) + enqueue({ data: { id: 'op-bulk-title' }, error: null }) + + await bulkBookTransactions.execute( + { + tx_ids: ['tx-1', 'tx-2'], + default_dimensions: { '6': 'villa almgren tak' }, + new_entry: { + description: 'Samlingsverifikation material', + lines: [ + { account_number: '4010', debit_amount: 1000, credit_amount: 0, currency: 'SEK', dimensions: { '1': 'KS01' } }, + { account_number: '1930', debit_amount: 0, credit_amount: 1000, currency: 'SEK' }, + ], + }, + }, + 'company-1', + 'user-1', + supabase as never, + ) + + const staged = inserts.find((i) => i.table === 'pending_operations') + expect(staged).toBeDefined() + const title = staged!.payload.title as string + expect(title).toContain('Samlingsverifikation') + expect(title).toContain('1 000,00 SEK') + expect(title).toMatch(/^Samlingsverifikation -/) + expect(title).toContain('2026-05-12') + expect(title).toContain('NORDNET UTTAG') + expect(title).toContain('(+1 till)') + }) +}) diff --git a/extensions/general/mcp-server/__tests__/unknown-args-rejected.test.ts b/extensions/general/mcp-server/__tests__/unknown-args-rejected.test.ts new file mode 100644 index 00000000..ca87f66c --- /dev/null +++ b/extensions/general/mcp-server/__tests__/unknown-args-rejected.test.ts @@ -0,0 +1,108 @@ +/** + * tools/call rejects unknown top-level parameters instead of dropping them. + * + * Feedback seq 261545: gnubok_query_journal called with {query} instead of + * {text} silently returned the whole journal. Hosts do not reliably enforce + * inputSchema, so the server does (arg-guard.ts), before execute() and as + * the structured VALIDATION_ERROR envelope. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { eventBus } from '@/lib/events/bus' + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(), + createServiceClient: vi.fn(), +})) + +vi.mock('@/lib/auth/api-keys', async (importOriginal) => { + const actual = await importOriginal() + const chain: unknown = new Proxy( + {}, + { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => resolve({ data: null, error: null }) + } + return () => chain + }, + }, + ) + const membershipChain: unknown = new Proxy( + {}, + { + get(_t, prop) { + if (prop === 'then') { + return (resolve: (v: unknown) => void) => + resolve({ + data: { company_id: '11111111-1111-4111-8111-111111111111', role: 'owner' }, + error: null, + }) + } + return () => membershipChain + }, + }, + ) + return { + ...actual, + extractBearerToken: vi.fn().mockReturnValue('test-token'), + validateApiKey: vi.fn().mockResolvedValue({ + userId: 'user-1', + companyId: '11111111-1111-4111-8111-111111111111', + scopes: ['reports:read', 'transactions:read'], + apiKeyId: 'key-live-1', + apiKeyName: 'Live Key', + mode: 'live', + }), + createServiceClientNoCookies: vi.fn(() => ({ + from: (table: string) => (table === 'company_members' ? membershipChain : chain), + rpc: () => chain, + })), + } +}) + +vi.mock('@/lib/entitlements/has-capability', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, hasCapability: vi.fn().mockResolvedValue(true) } +}) + +import { handleMcpRequest } from '../server' + +function mcpToolCall(name: string, args: Record = {}): Request { + return new Request('http://localhost:3000/api/extensions/ext/mcp-server/mcp', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer test-token' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name, arguments: args } }), + }) +} + +async function parsedToolResult(response: Response): Promise<{ isError: boolean; payload: Record }> { + const json = await response.json() + const result = json.result as { isError?: boolean; content: { text: string }[] } + return { isError: result.isError === true, payload: JSON.parse(result.content[0].text) } +} + +describe('MCP tools/call unknown-parameter guard', () => { + beforeEach(() => { + vi.clearAllMocks() + eventBus.clear() + }) + + it('rejects a misspelled parameter with a structured VALIDATION_ERROR naming the valid keys', async () => { + const response = await handleMcpRequest(mcpToolCall('gnubok_query_journal', { query: 'hyra' })) + const { isError, payload } = await parsedToolResult(response) + + expect(isError).toBe(true) + const error = payload.error as { code: string; message_en: string; retryable: boolean } + expect(error.code).toBe('VALIDATION_ERROR') + expect(error.retryable).toBe(false) + expect(error.message_en).toContain('"query"') + expect(error.message_en).toContain('text') + }) + + it('does not fire for a well-formed call (the tool itself runs)', async () => { + const response = await handleMcpRequest(mcpToolCall('gnubok_list_skills', {})) + const { payload } = await parsedToolResult(response) + const error = payload.error as { code?: string } | undefined + expect(error?.code).not.toBe('VALIDATION_ERROR') + }) +}) diff --git a/extensions/general/mcp-server/arg-guard.ts b/extensions/general/mcp-server/arg-guard.ts new file mode 100644 index 00000000..3625618d --- /dev/null +++ b/extensions/general/mcp-server/arg-guard.ts @@ -0,0 +1,28 @@ +/** + * Top-level argument allow-listing for tools/call. + * + * Every tool inputSchema declares `additionalProperties: false` (guarded by + * strict-schemas.test.ts), but hosts do not reliably enforce it, so a + * misspelled parameter used to be dropped silently: gnubok_query_journal + * called with {query} instead of {text} returned the whole journal with + * applied_filters.text null (feedback seq 261545). Unknown top-level keys are + * a caller error, never data, and are rejected before execute(). + * + * company_id is tolerated everywhere: the company-routing layer strips it for + * company-dependent tools, and a client that always sends it must not break + * on the few tools that ignore it. + */ +export function listArgKeys(inputSchema: Record): string[] { + const properties = inputSchema.properties + if (!properties || typeof properties !== 'object') return [] + return Object.keys(properties as Record) +} + +export function findUnknownArgKeys( + inputSchema: Record, + args: Record, +): string[] { + if (inputSchema.additionalProperties !== false) return [] + const allowed = new Set(listArgKeys(inputSchema)) + return Object.keys(args).filter((key) => key !== 'company_id' && !allowed.has(key)) +} diff --git a/extensions/general/mcp-server/company-routing.ts b/extensions/general/mcp-server/company-routing.ts index 2f153742..0de23dee 100644 --- a/extensions/general/mcp-server/company-routing.ts +++ b/extensions/general/mcp-server/company-routing.ts @@ -28,7 +28,7 @@ interface ToolSchemaSource { inputSchema: Record } -function codedError( +export function codedError( code: 'VALIDATION_ERROR' | 'NOT_FOUND' | 'FORBIDDEN' | 'INTERNAL_ERROR', message: string ) { diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 9a92c21c..4504ecc7 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -153,11 +153,13 @@ import { addCompanyToNextHint, addCompanyToTopLevelNext, assertMcpCompanyWriteAccess, + codedError, extractRequestedCompany, isCompanyDependentTool, projectToolInputSchema, resolveMcpCompanyContext, } from './company-routing' +import { findUnknownArgKeys, listArgKeys } from './arg-guard' import { findSupplierCandidates, type SupplierRow } from './supplier-candidates' import { matchSupplierByIdentity, @@ -9038,7 +9040,7 @@ export const tools: McpTool[] = [ const { data: txs, error: txError } = await supabase .from('transactions') - .select('id, amount, currency, date, journal_entry_id') + .select('id, amount, currency, date, journal_entry_id, description, merchant_name') .in('id', txIds) .eq('company_id', companyId) if (txError || !txs || txs.length !== txIds.length) { @@ -9138,10 +9140,24 @@ export const tools: McpTool[] = [ }) } + // The queue title is what a reviewer approves from (often on a phone): + // "Samlingsverifikation: 1 transaktioner 2026-07-22" carried no amount, + // direction or counterparty, so the approver could not tell what they + // were authorising (feedback seq 261545). Same per-tx text the + // categorize titles already carry; preview_data stays aggregate-only. + const batchTotal = txs.reduce((sum, t) => sum + Number(t.amount), 0) + const batchAmount = + `${direction === 'income' ? '+' : '-'}` + + `${Math.abs(batchTotal).toLocaleString('sv-SE', { minimumFractionDigits: 2, maximumFractionDigits: 2 })} ` + + `${txs[0]!.currency ?? 'SEK'}` + const batchLead = String(txs[0]!.merchant_name || txs[0]!.description || '').trim().slice(0, 40) + const batchRest = txIds.length > 1 ? ` (+${txIds.length - 1} till)` : '' + const batchTitle = existingJeId + ? `Länka ${txIds.length} transaktioner ${batchAmount} ${txDate} till verifikat: ${batchLead}${batchRest}` + : `Samlingsverifikation ${batchAmount} ${txDate}: ${batchLead}${batchRest}` + return stagePendingOperation(supabase, companyId, userId, 'bulk_book_transactions', - existingJeId - ? `Länka ${txIds.length} transaktioner till verifikat (${txDate})` - : `Samlingsverifikation: ${txIds.length} transaktioner ${txDate}`, + batchTitle, { tx_ids: txIds, existing_journal_entry_id: existingJeId, @@ -18482,6 +18498,19 @@ export async function handleMcpRequest(request: Request): Promise { const extracted = extractRequestedCompany(rawToolArgs) toolArgs = extracted.toolArgs + // Hosts do not reliably enforce inputSchema, so a misspelled + // parameter used to be dropped silently (see arg-guard.ts). Thrown + // inside this try so it reaches the caller as the structured + // VALIDATION_ERROR envelope, never as a half-applied call. + const unknownArgKeys = findUnknownArgKeys(tool.inputSchema as Record, toolArgs) + if (unknownArgKeys.length > 0) { + throw codedError( + 'VALIDATION_ERROR', + `Unknown parameter${unknownArgKeys.length > 1 ? 's' : ''} ${unknownArgKeys.map((k) => `"${k}"`).join(', ')} for ${requestedToolName}. ` + + `Valid parameters: ${listArgKeys(tool.inputSchema as Record).join(', ') || '(none)'}. Unknown keys are rejected, not ignored.`, + ) + } + if (isCompanyDependentTool(toolName)) { const companyContext = await resolveMcpCompanyContext({ supabase,