fix(mcp): query_journal defaults to status 'all' so totals equal ledger balances (#1863)
* fix(mcp): query_journal defaults to status 'all' so totals equal ledger balances Posted-only was the default, but storno bookkeeping keeps both the reversed original and its posted storno on the account: one-leg sums are never balances. A customer's agent summed posted-only lines over a storno-heavy quarter, found phantom VAT residuals on 2614/2641/2645/2647 and asked support to revert correct books. - default status 'all' (posted + reversed), the same inclusion rule as trial balance, GL and SIE export - explicit 'posted'/'reversed' get status_filter_warning when the opposite leg exists in range (entry-level head count, advisory) - unknown status values now throw instead of matching nothing - sibling description trims fund the additions within the tools/list payload budget Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(mcp): make status_filter_warning claim strictly true, hint status:'posted' for tag preview Review feedback (PR Agent + skeptics): the entry-level opposite-status count cannot prove the line-filtered totals are wrong, so the warning now says one-leg totals CAN differ from balances; verb agrees with a count of one. tag_journal_lines preview hint tells agents to pass status:'posted' now that query_journal defaults to 'all'. Description trims keep the tools/list payload inside the budget guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(mcp): state the status_filter_warning count scope in the message CodeRabbit: the opposite-status count applies entry-level filters only, so say so in the warning instead of letting an account-scoped caller read the count as account-scoped. Line-level scoping declined as documented in DECISIONS.md (it would re-run the full line fetch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5
Claude Fable 5
parent
174f315d9c
commit
0a3cb1a31a
@@ -249,6 +249,7 @@ function makeLineRow(opts: {
|
||||
entry_notes?: string | null
|
||||
voucher_number?: number
|
||||
entry_date?: string
|
||||
entry_status?: string
|
||||
}) {
|
||||
return {
|
||||
id: opts.id,
|
||||
@@ -268,7 +269,7 @@ function makeLineRow(opts: {
|
||||
description: opts.entry_description ?? '',
|
||||
notes: opts.entry_notes ?? null,
|
||||
source_type: 'bank_transaction',
|
||||
status: 'posted',
|
||||
status: opts.entry_status ?? 'posted',
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -1006,3 +1007,102 @@ describe('gnubok_query_journal: accounts argument normalization', () => {
|
||||
).rejects.toThrow(/capped at 50/)
|
||||
})
|
||||
})
|
||||
|
||||
describe('gnubok_query_journal: status default and balance integrity', () => {
|
||||
const tool = () => tools.find((t) => t.name === 'gnubok_query_journal')!
|
||||
|
||||
// A storno pair on 2614: the reversed original (credit) and its posted
|
||||
// storno (debit). Under the ledger inclusion rule they net to zero; a
|
||||
// posted-only view sees just the storno leg and fabricates a +940.27
|
||||
// "balance". This is the exact shape that made a customer's agent find
|
||||
// phantom VAT residuals and demand a revert of correct books.
|
||||
const stornoPair = () => [
|
||||
makeLineRow({
|
||||
id: 'l-orig', account_number: '2614', debit_amount: 0, credit_amount: 940.27,
|
||||
entry_status: 'reversed', entry_date: '2026-05-31',
|
||||
}),
|
||||
makeLineRow({
|
||||
id: 'l-storno', account_number: '2614', debit_amount: 940.27, credit_amount: 0,
|
||||
entry_status: 'posted', entry_date: '2026-05-31', voucher_number: 2,
|
||||
}),
|
||||
]
|
||||
|
||||
it("declares status_filter_warning in the output schema and 'all' as the documented default", () => {
|
||||
const t = tool()
|
||||
const out = t.outputSchema as { properties?: Record<string, unknown> }
|
||||
expect(out.properties?.status_filter_warning).toBeDefined()
|
||||
const statusProp = (t.inputSchema as {
|
||||
properties: Record<string, { description?: string }>
|
||||
}).properties.status
|
||||
expect(statusProp.description).toMatch(/Default 'all'/)
|
||||
})
|
||||
|
||||
it('defaults to posted + reversed so a storno pair nets to zero (ledger rule)', async () => {
|
||||
const { supabase, entryQueries } = makeTwoStepTextMock(stornoPair())
|
||||
const result = (await tool().execute(
|
||||
{ accounts: ['2614'] },
|
||||
'company-1', 'user-1', supabase,
|
||||
)) as {
|
||||
lines: unknown[]
|
||||
totals: { net: number }
|
||||
status_filter_warning?: string
|
||||
applied_filters: { status: string }
|
||||
}
|
||||
|
||||
expect(result.applied_filters.status).toBe('all')
|
||||
expect(result.lines).toHaveLength(2)
|
||||
expect(result.totals.net).toBe(0)
|
||||
expect(result.status_filter_warning).toBeUndefined()
|
||||
// The entry pass filters on BOTH statuses; no separate opposite-status
|
||||
// count query runs on the default path.
|
||||
const statusFilters = entryQueries().flatMap((q) =>
|
||||
q.filters.filter((f) => f.column === 'status'),
|
||||
)
|
||||
expect(statusFilters).toEqual([
|
||||
{ op: 'in', column: 'status', value: ['posted', 'reversed'] },
|
||||
])
|
||||
})
|
||||
|
||||
it("explicit status 'posted' returns one leg and warns that totals are not balances", async () => {
|
||||
const { supabase, entryQueries } = makeTwoStepTextMock(stornoPair())
|
||||
const result = (await tool().execute(
|
||||
{ accounts: ['2614'], status: 'posted' },
|
||||
'company-1', 'user-1', supabase,
|
||||
)) as {
|
||||
lines: Array<{ line_id: string }>
|
||||
totals: { net: number }
|
||||
status_filter_warning?: string
|
||||
}
|
||||
|
||||
expect(result.lines.map((l) => l.line_id)).toEqual(['l-storno'])
|
||||
expect(result.totals.net).toBe(940.27)
|
||||
expect(result.status_filter_warning).toMatch(/can differ from account balances/)
|
||||
expect(result.status_filter_warning).toMatch(/status 'all'/)
|
||||
// Singular/plural agreement: one excluded entry reads "1 entry ... is".
|
||||
expect(result.status_filter_warning).toMatch(/1 entry with status 'reversed' in the filtered range is excluded/)
|
||||
// The warning is grounded in a real opposite-status count query.
|
||||
const oppositeCount = entryQueries().some((q) =>
|
||||
q.filters.some((f) => f.op === 'eq' && f.column === 'status' && f.value === 'reversed'),
|
||||
)
|
||||
expect(oppositeCount).toBe(true)
|
||||
})
|
||||
|
||||
it('omits the warning when the one-leg filter excluded nothing', async () => {
|
||||
const rows = [
|
||||
makeLineRow({ id: 'l1', account_number: '2614', debit_amount: 100, entry_status: 'posted' }),
|
||||
]
|
||||
const { supabase } = makeTwoStepTextMock(rows)
|
||||
const result = (await tool().execute(
|
||||
{ accounts: ['2614'], status: 'posted' },
|
||||
'company-1', 'user-1', supabase,
|
||||
)) as { status_filter_warning?: string }
|
||||
expect(result.status_filter_warning).toBeUndefined()
|
||||
})
|
||||
|
||||
it('rejects an unknown status value instead of matching nothing', async () => {
|
||||
const { supabase } = makeTwoStepTextMock([])
|
||||
await expect(
|
||||
tool().execute({ status: 'draft' }, 'company-1', 'user-1', supabase),
|
||||
).rejects.toThrow(/status must be/)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -7380,7 +7380,7 @@ export const tools: McpTool[] = [
|
||||
filters: {
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
description: 'Line selection: at least one filter required. Preview the match set with gnubok_query_journal (same filter fields) first.',
|
||||
description: "Line selection: at least one filter required. Preview the match set with gnubok_query_journal (same filter fields plus status:'posted') first.",
|
||||
properties: {
|
||||
account_from: { type: 'string', description: 'Lowest account number (inclusive), e.g. "4010".' },
|
||||
account_to: { type: 'string', description: 'Highest account number (inclusive).' },
|
||||
@@ -7907,7 +7907,7 @@ export const tools: McpTool[] = [
|
||||
{
|
||||
name: 'gnubok_query_journal',
|
||||
title: 'Query Journal Lines',
|
||||
description: "Flexible journal-line query for ad-hoc questions. Filters: account, date, amount, voucher, source, status, dimensions bag, free-text. group_by/group_by_dimension aggregation; include_dimensions returns each line's bag. Lines + totals over the full match set (totals_scope).",
|
||||
description: 'Flexible journal-line query for ad-hoc questions. Filters: account, date, amount, voucher, source, status, dimensions, free-text. group_by/group_by_dimension aggregation; include_dimensions returns line bags. Lines + totals over the full match set.',
|
||||
inputSchema: {
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
@@ -7924,7 +7924,7 @@ export const tools: McpTool[] = [
|
||||
voucher_number_from: { type: 'number', description: 'Lowest voucher number (inclusive)' },
|
||||
voucher_number_to: { type: 'number', description: 'Highest voucher number (inclusive)' },
|
||||
source_type: { type: 'string', description: 'Filter by source: bank_transaction, invoice_created, supplier_invoice, currency_revaluation, year_end, opening_balance, etc.' },
|
||||
status: { type: 'string', enum: ['posted', 'reversed', 'all'], description: 'Default: posted' },
|
||||
status: { type: 'string', enum: ['posted', 'reversed', 'all'], description: "Default 'all' (posted + reversed: totals equal ledger balances). One-leg totals are NOT balances." },
|
||||
project: { type: 'string', description: 'Filter by project code (SIE dim 6)' },
|
||||
cost_center: { type: 'string', description: 'Filter by cost center (SIE dim 1)' },
|
||||
dimensions: {
|
||||
@@ -7938,7 +7938,7 @@ export const tools: McpTool[] = [
|
||||
},
|
||||
group_by: { type: 'string', enum: ['account_number', 'voucher_series', 'source_type', 'cost_center', 'project'], description: 'Aggregate matching lines into groups by this field. Mutually exclusive with group_by_dimension.' },
|
||||
group_by_dimension: { type: 'string', description: 'Aggregate by SIE dimension number (e.g. "6" = projekt) from each line\'s dimensions bag; untagged → "(utan dimension)". Mutually exclusive with group_by.' },
|
||||
limit: { type: 'number', minimum: 1, maximum: 500, description: 'Max lines returned 1-500 (default 100). Totals/groups always cover the FULL match set even when truncated (free-text search included).' },
|
||||
limit: { type: 'number', minimum: 1, maximum: 500, description: 'Max lines returned 1-500 (default 100); totals/groups still cover the full match set.' },
|
||||
},
|
||||
},
|
||||
outputSchema: {
|
||||
@@ -7947,10 +7947,10 @@ export const tools: McpTool[] = [
|
||||
properties: {
|
||||
lines: { type: 'array', items: { type: 'object' } },
|
||||
truncated: { type: 'boolean', description: 'True if more matching lines exist than were returned' },
|
||||
total_lines: { type: 'number', description: 'Total lines matching ALL filters (incl. amount). When amount_min/amount_max is set this reflects the filtered set, not the wider DB-side match.' },
|
||||
total_lines: { type: 'number', description: 'Total lines matching ALL filters, amount filter included.' },
|
||||
returned_lines: { type: 'number' },
|
||||
amount_filter_applied_post_fetch: { type: 'boolean', description: 'True if amount_min/amount_max was applied client-side after the DB fetch.' },
|
||||
db_matched_pre_amount_filter: { type: ['number', 'null'], description: 'Pre-amount-filter DB match count when amount_filter_applied_post_fetch is true; null otherwise.' },
|
||||
db_matched_pre_amount_filter: { type: ['number', 'null'], description: 'DB match count before the post-fetch amount filter; null when not applied.' },
|
||||
totals: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
@@ -7962,7 +7962,11 @@ export const tools: McpTool[] = [
|
||||
totals_scope: {
|
||||
type: 'string',
|
||||
enum: ['full_match'],
|
||||
description: 'Always full_match: totals/groups aggregate ALL matching lines regardless of limit, on free-text searches too. (returned_slice is no longer emitted; the field stays for older clients.)',
|
||||
description: 'Always full_match: totals/groups cover ALL matching lines regardless of limit (field kept for older clients).',
|
||||
},
|
||||
status_filter_warning: {
|
||||
type: 'string',
|
||||
description: 'Set when a one-leg status filter excluded opposite-status entries: totals may not equal account balances.',
|
||||
},
|
||||
groups: {
|
||||
type: 'array',
|
||||
@@ -7976,7 +7980,7 @@ export const tools: McpTool[] = [
|
||||
line_count: { type: 'number' },
|
||||
},
|
||||
},
|
||||
description: 'Present when group_by/group_by_dimension is set; sorted by |net| desc. Scope follows totals_scope.',
|
||||
description: 'Present when grouping is set; sorted by |net| desc, full-match scope.',
|
||||
},
|
||||
applied_filters: { type: 'object' },
|
||||
...DIMENSION_FILTER_OUTPUT_PROPS,
|
||||
@@ -7991,7 +7995,16 @@ 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'
|
||||
// Default 'all' (posted + reversed): the same inclusion rule every
|
||||
// report uses (trial balance, GL, SIE export). Storno bookkeeping keeps
|
||||
// both the reversed original and its posted storno on the account, so a
|
||||
// one-leg filter produces sums that are not balances; the old 'posted'
|
||||
// default made a real customer's agent find phantom VAT residuals in a
|
||||
// storno-heavy quarter and demand a revert of correct books.
|
||||
const status = (args.status as string) || 'all'
|
||||
if (status !== 'all' && status !== 'posted' && status !== 'reversed') {
|
||||
throw new Error("status must be 'posted', 'reversed' or 'all'")
|
||||
}
|
||||
// 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
|
||||
@@ -8059,9 +8072,9 @@ export const tools: McpTool[] = [
|
||||
// journal_entry_lines. Every pass (plain and both text legs) applies
|
||||
// BOTH, so they always describe one match set; the text legs only add
|
||||
// their .ilike() on top.
|
||||
const filterEntries = (q: EntryLinesQuery): EntryLinesQuery => {
|
||||
const filterEntriesWithStatus = (q: EntryLinesQuery, st: string): EntryLinesQuery => {
|
||||
let e = q.eq('company_id', companyId)
|
||||
e = status === 'all' ? e.in('status', ['posted', 'reversed']) : e.eq('status', status)
|
||||
e = st === 'all' ? e.in('status', ['posted', 'reversed']) : e.eq('status', st)
|
||||
if (dateFrom) e = e.gte('entry_date', dateFrom)
|
||||
if (dateTo) e = e.lte('entry_date', dateTo)
|
||||
if (voucherSeries) e = e.eq('voucher_series', voucherSeries)
|
||||
@@ -8070,6 +8083,8 @@ export const tools: McpTool[] = [
|
||||
if (sourceType) e = e.eq('source_type', sourceType)
|
||||
return e
|
||||
}
|
||||
const filterEntries = (q: EntryLinesQuery): EntryLinesQuery =>
|
||||
filterEntriesWithStatus(q, status)
|
||||
|
||||
const filterLines = (q: EntryLinesQuery): EntryLinesQuery => {
|
||||
let l = q
|
||||
@@ -8334,6 +8349,34 @@ export const tools: McpTool[] = [
|
||||
.sort((a, b) => Math.abs(b.net) - Math.abs(a.net))
|
||||
}
|
||||
|
||||
// One-leg status filters get a balance-integrity warning when the
|
||||
// opposite leg exists in range. Entry-level head count only: cheap, and
|
||||
// exact for what the warning claims (excluded entries exist), without
|
||||
// re-running the line fetch. A failed count never fails the query; the
|
||||
// warning is advisory.
|
||||
let statusFilterWarning: string | undefined
|
||||
if (status !== 'all') {
|
||||
const opposite = status === 'posted' ? 'reversed' : 'posted'
|
||||
const { count, error: countError } = await filterEntriesWithStatus(
|
||||
supabase.from('journal_entries').select('id', { count: 'exact', head: true }),
|
||||
opposite
|
||||
)
|
||||
if (countError) {
|
||||
log.warn('query_journal opposite-status count failed', {
|
||||
companyId,
|
||||
userId,
|
||||
error: countError.message,
|
||||
})
|
||||
} else if ((count ?? 0) > 0) {
|
||||
statusFilterWarning =
|
||||
`${count} entr${count === 1 ? 'y' : 'ies'} with status '${opposite}' in the filtered range ` +
|
||||
`${count === 1 ? 'is' : 'are'} excluded by status='${status}' (count scoped by date/voucher/` +
|
||||
`source filters only, not by account or dimension filters). Storno bookkeeping keeps both ` +
|
||||
`the reversed original and its storno on the account, so one-leg totals can differ from ` +
|
||||
`account balances. Re-run with status 'all' (the default) for ledger-accurate sums.`
|
||||
}
|
||||
}
|
||||
|
||||
// The full match set anchors total_lines / truncated / pre-amount count
|
||||
// on every path; `lines` is the first `limit` of it in display order.
|
||||
return {
|
||||
@@ -8349,6 +8392,7 @@ export const tools: McpTool[] = [
|
||||
net: Math.round((totalDebit - totalCredit) * 100) / 100,
|
||||
},
|
||||
totals_scope: 'full_match',
|
||||
...(statusFilterWarning ? { status_filter_warning: statusFilterWarning } : {}),
|
||||
...(groups ? { groups } : {}),
|
||||
applied_filters: {
|
||||
account_from: accountFrom ?? null,
|
||||
|
||||
Reference in New Issue
Block a user