fix(mcp): bulk-book titles that say what is approved, reject unknown parameters (#1856)
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 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Jakob Wennberg
Claude Fable 5
Jakob Wennberg
Claude Fable 5
parent
43a569d248
commit
2a33291c18
@@ -1201,3 +1201,4 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. 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.
|
||||
|
||||
@@ -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<string, unknown>,
|
||||
{ query: 'hyra' },
|
||||
)
|
||||
expect(unknown).toEqual(['query'])
|
||||
expect(listArgKeys(queryJournal.inputSchema as Record<string, unknown>)).toContain('text')
|
||||
})
|
||||
})
|
||||
@@ -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)')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<typeof import('@/lib/auth/api-keys')>()
|
||||
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<typeof import('@/lib/entitlements/has-capability')>()
|
||||
return { ...actual, hasCapability: vi.fn().mockResolvedValue(true) }
|
||||
})
|
||||
|
||||
import { handleMcpRequest } from '../server'
|
||||
|
||||
function mcpToolCall(name: string, args: Record<string, unknown> = {}): 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<string, unknown> }> {
|
||||
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')
|
||||
})
|
||||
})
|
||||
@@ -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, unknown>): string[] {
|
||||
const properties = inputSchema.properties
|
||||
if (!properties || typeof properties !== 'object') return []
|
||||
return Object.keys(properties as Record<string, unknown>)
|
||||
}
|
||||
|
||||
export function findUnknownArgKeys(
|
||||
inputSchema: Record<string, unknown>,
|
||||
args: Record<string, unknown>,
|
||||
): string[] {
|
||||
if (inputSchema.additionalProperties !== false) return []
|
||||
const allowed = new Set(listArgKeys(inputSchema))
|
||||
return Object.keys(args).filter((key) => key !== 'company_id' && !allowed.has(key))
|
||||
}
|
||||
@@ -28,7 +28,7 @@ interface ToolSchemaSource {
|
||||
inputSchema: Record<string, unknown>
|
||||
}
|
||||
|
||||
function codedError(
|
||||
export function codedError(
|
||||
code: 'VALIDATION_ERROR' | 'NOT_FOUND' | 'FORBIDDEN' | 'INTERNAL_ERROR',
|
||||
message: string
|
||||
) {
|
||||
|
||||
@@ -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<Response> {
|
||||
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<string, unknown>, 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<string, unknown>).join(', ') || '(none)'}. Unknown keys are rejected, not ignored.`,
|
||||
)
|
||||
}
|
||||
|
||||
if (isCompanyDependentTool(toolName)) {
|
||||
const companyContext = await resolveMcpCompanyContext({
|
||||
supabase,
|
||||
|
||||
Reference in New Issue
Block a user