Files
accounted/extensions/general/mcp-server/__tests__/query-journal.test.ts
T
Jakob Wennberg 6629964780 fix(mcp): unblock query_journal text search across line + entry descriptions (#605)
* fix(mcp): unblock query_journal text search across line+entry descriptions

`gnubok_query_journal` issued a single PostgREST `.or()` filter mixing a
base-table column (line_description on journal_entry_lines) with a column
on the embedded inner-joined resource (journal_entries.description).
PostgREST's flat comma OR syntax cannot span base and embedded resources
and returned 400 "failed to parse logic tree" on every free-text search,
so the agent could not look up history (e.g. "har vi bokfört Google
innan?") via the MCP tool.

Refactor: pull the common filter chain into a `buildBaseQuery()` helper
and issue two parallel `.ilike()` queries — one on `line_description`,
one on `journal_entries.description` — then merge by line id, re-sort,
and slice to `limit`. Same pattern as
`lib/invoices/duplicate-payment-candidates.ts`, with the same rationale
(LIKE-DSL injection risk + cross-embed unsupported).

Drops the obsolete comma-stripping pre-processing on the search term
(only needed because the value was being injected into PostgREST's OR
DSL); LIKE wildcard escaping (`%` and `_`) is preserved.

Adds four tests covering: merge across both legs, dedup of cross-leg
overlap, correct columns + escaped pattern sent to `.ilike`, and
LIKE-wildcard escape end-to-end. Existing amount-filter / accounts-cap /
truncation tests unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(mcp): query_journal PR review — honest truncation, wider leg, error sanitisation

Address PR #605 review (Greptile + compliance swarm).

- truncated math: previously dbMatched = byLine.count + byEntry.count
  inflated the counter every time a row matched both legs (e.g. an entry
  whose line description AND header description both contain "Google").
  total_lines was off by up to 2× and truncated was forced true even when
  every distinct match was already returned, sending the agent into
  unnecessary narrow-and-retry loops. dbMatched is now merged.size — the
  honest distinct-row count among what we fetched. A new legCapHit signal
  drives truncated when either leg's fetch window filled, so we never lie
  the other direction either.

- per-leg fetch widened to 2 × limit. Previously each leg was capped at
  `limit` independently, so when one column was much more selective than
  the other (e.g. 150 line matches vs 5 entry matches), the merge could
  drop globally-ranked rows from the chronologically newer tail. The
  final slice still caps at `limit`; the wider per-leg window just gives
  the merge a better tail to choose from.

- text input length cap (200 chars). Defence-in-depth against pathological
  inputs even though .ilike() parameterises the value (compliance A.8.28).

- error sanitisation in the text-search path. Raw PostgREST messages can
  surface internal schema details (table names, constraint names) to the
  caller. We now log details server-side via the existing module logger
  and throw a generic message (compliance V16).

- comma-stripping rationale documented inline so the next reviewer
  doesn't re-add it. The previous defence was needed because the value
  was injected into PostgREST's OR DSL where `,` is the separator; the
  parameterised .ilike() path treats `,` as a literal and stripping it
  would mangle real-comma searches (compliance V1.2.5).

Adds 4 regression tests: overlap-doesn't-falsely-truncate, leg-cap-hit
flags truncated, text>200 rejected, raw schema name never leaked. All
14 query_journal tests pass; 266/266 MCP suite green; build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(mcp): query_journal PR review round 2 — schema guards, leg cap, audit log

Address compliance swarm findings on commit 3dfab46c.

- text input: add `maxLength: 200` to the JSON Schema so the limit is
  enforced at the boundary by the MCP runtime, not only inside execute().
  Inline guard kept as belt-and-suspenders (ASVS V2.2, ISO 27001 A.8.28).
- limit input: add `minimum: 1, maximum: 500` to the JSON Schema (matches
  the existing Math.min cap inside execute()) so the bound is visible to
  agents and enforced declaratively.
- legLimit hard cap: `Math.min(limit * 2, 500)`. Previously legLimit
  scaled with limit unbounded, so a maxed-out limit triggered 2×1000-row
  fetches. Cap is independent of the caller-supplied limit (ASVS V2.3).
- audit log: include `userId` alongside `companyId` in both log.warn
  payloads so failed-query events can be correlated to the actor for
  incident investigation (ASVS V16, SOC 2 CC7.2).
- new test: assert both parallel ilike legs in the text-search path issue
  `.eq('journal_entries.company_id', companyId)` — defence-in-depth
  against a future refactor accidentally dropping tenant scoping from
  one leg (SOC 2 CC6.1).

Not addressed (with rationale):

- ASVS V1.2.5 / SOC 2 CC7.2 "log.warn body contains raw PostgREST error
  text". The swedish-accounting-compliance reviewer explicitly cleared
  this on the same commit ("the schema-leak is plugged in both paths").
  The thrown error is generic; raw detail lives only in internal
  structured logs (Sentry, etc.) where it is needed for incident triage.
  Same pattern is used by 20+ other tools in this file; scrubbing only
  this one is inconsistent. Defer to a project-wide logger sanitiser.

- Swedish reviewer's "reversed mixed with posted in status=all": false
  positive — `status` IS surfaced per line in the response shape (see
  the lines.map projection in server.ts).

- Swedish reviewer's "debit XOR credit not enforced": insert-time
  concern, not a read-tool concern. Out of scope.

Tests: 15 query_journal cases (4 new this round); 267 MCP suite; build
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-29 14:21:46 +02:00

468 lines
16 KiB
TypeScript

/**
* Unit tests for gnubok_query_journal.
*
* Verifies tool registration and the post-fetch amount filter + totals
* computation. The supabase query-builder chain is exercised by the live
* MCP smoke test; here we just check the result-shape pipeline.
*/
import { describe, it, expect, vi } from 'vitest'
import { tools } from '../server'
import { TOOL_SCOPE_MAP } from '@/lib/auth/api-keys'
describe('gnubok_query_journal — registration', () => {
it('is registered and read-only', () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')
expect(tool).toBeDefined()
expect(tool?.annotations.readOnlyHint).toBe(true)
expect(tool?.annotations.destructiveHint).toBe(false)
})
it('declares the expected output fields', () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const schema = tool.outputSchema as { required?: string[] }
expect(schema.required).toContain('lines')
expect(schema.required).toContain('totals')
expect(schema.required).toContain('total_lines')
})
it('is mapped to reports:read scope', () => {
expect(TOOL_SCOPE_MAP.gnubok_query_journal).toBe('reports:read')
})
})
/**
* Build a minimal supabase mock that returns a fixed line set when the chain
* is awaited. Uses a chainable proxy whose every method returns itself, with
* the terminal awaitable resolving to { data, error, count }.
*/
function makeChainMock(lines: unknown[], count: number) {
const result = { data: lines, error: null, count }
const buildChain = (): unknown => {
return new Proxy(
{},
{
get(_t, prop) {
if (prop === 'then') {
return (resolve: (v: unknown) => void) => resolve(result)
}
return () => buildChain()
},
},
)
}
return {
from: vi.fn().mockImplementation(() => buildChain()),
} as never
}
/**
* 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<Array<{ column: string; value: unknown }>> = []
let callIndex = 0
const buildChain = (
result: { data: unknown[]; error: null; count: number },
legEqCalls: Array<{ column: string; value: unknown }>,
): unknown => {
return new Proxy(
{},
{
get(_t, prop) {
if (prop === 'then') {
return (resolve: (v: unknown) => void) => resolve(result)
}
if (prop === 'ilike') {
return (column: string, pattern: string) => {
ilikeCalls.push({ column, pattern })
return buildChain(result, legEqCalls)
}
}
if (prop === 'eq') {
return (column: string, value: unknown) => {
legEqCalls.push({ column, value })
return buildChain(result, legEqCalls)
}
}
return () => buildChain(result, legEqCalls)
},
},
)
}
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)
}),
} as never
return { supabase, ilikeCalls, eqCallsByLeg, callCount: () => callIndex }
}
/** Build a LineRow fixture inline — keeps the per-test data dense and readable. */
function makeLineRow(opts: {
id: string
account_number?: string
debit_amount?: number
credit_amount?: number
line_description?: string | null
entry_description?: string
voucher_number?: number
entry_date?: string
}) {
return {
id: opts.id,
account_number: opts.account_number ?? '4010',
debit_amount: opts.debit_amount ?? 1000,
credit_amount: opts.credit_amount ?? 0,
currency: 'SEK',
line_description: opts.line_description ?? null,
project: null,
cost_center: null,
sort_order: 0,
journal_entries: {
id: `e-${opts.id}`,
voucher_number: opts.voucher_number ?? 1,
voucher_series: 'A',
entry_date: opts.entry_date ?? '2026-03-15',
description: opts.entry_description ?? '',
source_type: 'bank_transaction',
status: 'posted',
},
}
}
describe('gnubok_query_journal — execute', () => {
it('applies amount_min filter and computes totals on the filtered set', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const lines = [
// Line 1: large debit — should pass amount_min: 1000
{
id: 'l1', account_number: '4010',
debit_amount: 5000, credit_amount: 0,
currency: 'SEK', line_description: 'Hyra', project: null, cost_center: null, sort_order: 0,
journal_entries: {
id: 'e1', voucher_number: 1, voucher_series: 'A',
entry_date: '2026-03-15', description: 'Marshyra',
source_type: 'supplier_invoice', status: 'posted',
},
},
// Line 2: small debit — should fail amount_min: 1000
{
id: 'l2', account_number: '4010',
debit_amount: 50, credit_amount: 0,
currency: 'SEK', line_description: 'Småinköp', project: null, cost_center: null, sort_order: 0,
journal_entries: {
id: 'e2', voucher_number: 2, voucher_series: 'A',
entry_date: '2026-03-16', description: 'Reseutlägg',
source_type: 'bank_transaction', status: 'posted',
},
},
]
const supabase = makeChainMock(lines, 2)
const result = (await tool.execute(
{ account_from: '4000', account_to: '4999', amount_min: 1000, limit: 100 },
'company-1',
'user-1',
supabase,
)) as {
lines: { line_id: string }[]
totals: { debit: number; credit: number; net: number }
truncated: boolean
total_lines: number
returned_lines: number
}
// amount_min: 1000 should filter out the 50-line
expect(result.returned_lines).toBe(1)
expect(result.lines[0].line_id).toBe('l1')
expect(result.totals.debit).toBe(5000)
expect(result.totals.credit).toBe(0)
expect(result.totals.net).toBe(5000)
})
it('caps accounts list at 50', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const supabase = makeChainMock([], 0)
const accounts = Array.from({ length: 51 }, (_, i) => String(1000 + i))
await expect(
tool.execute({ accounts }, 'company-1', 'user-1', supabase),
).rejects.toThrow(/capped at 50/)
})
it('marks truncated=true when count exceeds returned', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const lines = [
{
id: 'l1', account_number: '1930',
debit_amount: 100, credit_amount: 0,
currency: 'SEK', line_description: null, project: null, cost_center: null, sort_order: 0,
journal_entries: {
id: 'e1', voucher_number: 1, voucher_series: 'A',
entry_date: '2026-01-01', description: 'Inbetalning',
source_type: 'bank_transaction', status: 'posted',
},
},
]
// count=999 simulates "many more matched than were returned"
const supabase = makeChainMock(lines, 999)
const result = (await tool.execute(
{ accounts: ['1930'], limit: 1 },
'company-1',
'user-1',
supabase,
)) as { truncated: boolean; total_lines: number; returned_lines: number }
expect(result.truncated).toBe(true)
expect(result.total_lines).toBe(999)
expect(result.returned_lines).toBe(1)
})
})
describe('gnubok_query_journal — free-text search', () => {
it('merges non-overlapping results from line_description and journal_entries.description', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const byLineHit = makeLineRow({
id: 'L1',
line_description: 'GOOGLE*CLOUD EMEA',
entry_description: 'Bank kostnad',
entry_date: '2026-05-10',
voucher_number: 42,
})
const byEntryHit = makeLineRow({
id: 'L2',
line_description: null,
entry_description: 'Google Workspace månadsavgift',
entry_date: '2026-05-12',
voucher_number: 43,
})
const { supabase, callCount } = makeQueueMock([
{ data: [byLineHit], count: 1 },
{ data: [byEntryHit], count: 1 },
])
const result = (await tool.execute(
{ text: 'Google', limit: 50 },
'company-1',
'user-1',
supabase,
)) as { lines: Array<{ line_id: string }>; returned_lines: number }
expect(callCount()).toBe(2)
expect(result.returned_lines).toBe(2)
const ids = result.lines.map((l) => l.line_id).sort()
expect(ids).toEqual(['L1', 'L2'])
})
it('deduplicates rows returned by both query legs', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const dupHit = makeLineRow({
id: 'LDUP',
line_description: 'Google Cloud',
entry_description: 'Google Cloud invoice',
entry_date: '2026-05-15',
voucher_number: 100,
})
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: Array<{ line_id: string }>; returned_lines: number }
expect(result.returned_lines).toBe(1)
expect(result.lines[0].line_id).toBe('LDUP')
})
it('issues .ilike against both line_description and journal_entries.description with escaped pattern', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const { supabase, ilikeCalls } = makeQueueMock([
{ data: [], count: 0 },
{ data: [], count: 0 },
])
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)
})
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 },
])
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%')
})
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.
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 { supabase } = makeQueueMock([
{ data: fullLeg, count: 4 },
{ data: [], count: 0 },
])
const result = (await tool.execute(
{ text: 'Google', limit: 2 },
'company-1',
'user-1',
supabase,
)) as { returned_lines: number; truncated: boolean }
expect(result.returned_lines).toBe(2)
expect(result.truncated).toBe(true)
})
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.
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const { supabase, eqCallsByLeg, callCount } = makeQueueMock([
{ data: [], count: 0 },
{ data: [], count: 0 },
])
await tool.execute(
{ text: 'Google', limit: 50 },
'company-xyz',
'user-1',
supabase,
)
expect(callCount()).toBe(2)
for (const legEqs of eqCallsByLeg) {
const scoped = legEqs.some(
(c) => c.column === 'journal_entries.company_id' && c.value === 'company-xyz',
)
expect(scoped).toBe(true)
}
})
it('rejects text longer than 200 characters', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
const { supabase } = makeQueueMock([])
const oversized = 'x'.repeat(201)
await expect(
tool.execute({ text: oversized, limit: 50 }, 'company-1', 'user-1', supabase),
).rejects.toThrow(/200 characters or shorter/)
})
it('does not surface raw PostgREST error text on text-search failure', async () => {
const tool = tools.find((t) => t.name === 'gnubok_query_journal')!
// 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
await expect(
tool.execute({ text: 'Google', limit: 50 }, 'company-1', 'user-1', supabase),
).rejects.toThrow(/Database error while running text search/)
// 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/)
})
})