From aa515d19314bae9eb9fd4e41a0338d3a16e547aa Mon Sep 17 00:00:00 2001 From: Gil Gardosh Date: Wed, 5 Aug 2026 15:55:38 +0300 Subject: [PATCH] =?UTF-8?q?Phase=203=20=E2=80=94=20The=20data-model=20guid?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/mcp-extension/plan.md | 9 +- packages/mcp-server/README.md | 13 +- packages/mcp-server/docs/todo.md | 2 +- .../tools/__tests__/data-model-guide.test.ts | 152 +++++++++++++++++ .../tools/__tests__/scope-forwarding.test.ts | 13 +- .../mcp-server/src/tools/data-model-guide.ts | 158 ++++++++++++++++++ .../mcp-server/src/tools/registry-instance.ts | 5 + 7 files changed, 346 insertions(+), 6 deletions(-) create mode 100644 packages/mcp-server/src/tools/__tests__/data-model-guide.test.ts create mode 100644 packages/mcp-server/src/tools/data-model-guide.ts diff --git a/docs/mcp-extension/plan.md b/docs/mcp-extension/plan.md index 8dd9d1b1d..22e2964fe 100644 --- a/docs/mcp-extension/plan.md +++ b/docs/mcp-extension/plan.md @@ -172,7 +172,14 @@ balance_report. --- -## Phase 3 — The data-model guide (§8) +## Phase 3 — The data-model guide (§8) — ✅ DONE + +_Implemented as `src/tools/data-model-guide.ts`, registered second (right after membership +discovery). 480 tests pass; codegen, lint, typecheck and build clean. Two deviations from the plan +below, both deliberate: the guide is ~5.8KB rather than ~4KB, and the card-settlement claim was +corrected against the code — `CreditcardBankCharge` **does** link the settlement transaction to the +card rows it covers (`creditCardTransactions`, `validCreditCardAmount`), so the guide points at that +grouping instead of repeating the feedback's "no link between them"._ The feedback says a static doc "would have saved the entire reverse-engineering phase." The MCP handler advertises only `capabilities: { tools: { listChanged: false } }` diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index 5277eddb6..4b927e944 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -18,8 +18,8 @@ Phase 1 (read-only) is feature-complete. The server provides: strict startup env transport with `/health`, `/metrics`, the OAuth protected-resource metadata endpoint, and the MCP route (`POST /mcp`, JSON-RPC 2.0) with graceful shutdown; Auth0 bearer-token verification; identity mapping to an internal user + business-membership context with memberships resolved from the -Accounter GraphQL server; a curated registry of fifteen read-only tools -(`accounter_list_business_memberships`, `accounter_list_accounts`, +Accounter GraphQL server; a curated registry of sixteen read-only tools +(`accounter_list_business_memberships`, `accounter_data_model_guide`, `accounter_list_accounts`, `accounter_income_expense_summary`, `accounter_profit_and_loss`, `accounter_vat_report`, `accounter_counterparty_totals`, `accounter_search_charges`, `accounter_get_charges`, `accounter_get_transactions`, `accounter_get_documents`, `accounter_ledger_records`, @@ -78,6 +78,15 @@ scope, because it _is_ the scope. businesses last. Pure: memberships are already on the auth context, so it makes no upstream call. A caller with no memberships gets an empty list, not an error. This is the scope-discovery entry point; to browse the full business directory use `accounter_list_businesses`. +- **`accounter_data_model_guide`** — a static markdown explanation of how accounts, charges, + documents, currency, and date filters relate, and of the traps that make naive aggregation wrong. + Pure (no upstream call) and **needs no business scope**, so it can be read cold, before a business + is chosen — which is the point: the connector feedback identified this missing context as the + single most expensive gap, since an agent otherwise reverse-engineers the model from Hebrew bank + descriptions. It is a tool rather than an MCP resource because the transport advertises only + `tools` capabilities, and `tools/list` is where a model actually looks. `data-model-guide.test.ts` + pins its content against the code — every tool it names must be registered, and every charge and + account type must be documented — so a rename cannot leave it silently stale. The **report tools** are registered ahead of the row-level ones on purpose: a model asked "how much did we make this year" should assemble and explain a report, not derive one from raw bank rows. All diff --git a/packages/mcp-server/docs/todo.md b/packages/mcp-server/docs/todo.md index fc61da878..ce5746786 100644 --- a/packages/mcp-server/docs/todo.md +++ b/packages/mcp-server/docs/todo.md @@ -10,7 +10,7 @@ Last consolidated 2026-08-03, after I1/I2/I4 (#4103–#4105) merged into `main`. owner-scoping work (#4089–#4097) is also in. **Status of the connector today:** working end-to-end against Claude Desktop with a pre-registered -Auth0 client, read-only, fifteen curated tools, business scope enforced by RLS upstream. Not +Auth0 client, read-only, sixteen curated tools, business scope enforced by RLS upstream. Not publishable — see B1. --- diff --git a/packages/mcp-server/src/tools/__tests__/data-model-guide.test.ts b/packages/mcp-server/src/tools/__tests__/data-model-guide.test.ts new file mode 100644 index 000000000..56ca88333 --- /dev/null +++ b/packages/mcp-server/src/tools/__tests__/data-model-guide.test.ts @@ -0,0 +1,152 @@ +import { describe, expect, it, vi } from 'vitest'; +import { buildAuthContext, type McpAuthContext } from '../../auth/identity.js'; +import type { AuthPrincipal } from '../../auth/token.js'; +import { UpstreamGraphQLClient } from '../../upstream/graphql-client.js'; +import { + DATA_MODEL_GUIDE, + DATA_MODEL_GUIDE_TOOL_NAME, + dataModelGuideTool, +} from '../data-model-guide.js'; +import { KNOWN_CHARGE_TYPENAMES, chargeTypeFromTypename } from '../entity-shapes.js'; +import { executeRegisteredTool } from '../execute.js'; +import { toolRegistry } from '../registry-instance.js'; + +/** + * The guide is static text, so the interesting failure mode is not "does it + * render" but **drift**: it names tools, charge types and fields, and a rename + * elsewhere would silently leave the model reading instructions for a surface + * that no longer exists. These tests pin it to the code it describes. + */ + +const PRINCIPAL: AuthPrincipal = { + subject: 'user-1', + issuer: 'https://tenant.auth0.com/', + audience: 'aud', + scopes: [], + email: null, + expiresAt: undefined, + claims: { sub: 'user-1' }, +}; + +function authContext(businessIds: string[]): McpAuthContext { + return buildAuthContext( + PRINCIPAL, + businessIds.map(businessId => ({ businessId, roleId: 'accountant' })), + ); +} + +/** Fails the test if anything reaches upstream — the handler must be pure. */ +function forbiddenClient() { + const fetchImpl = vi.fn(async () => { + throw new Error('the data-model guide must not call upstream'); + }); + return new UpstreamGraphQLClient({ + endpoint: 'http://localhost:4000/graphql', + timeoutMs: 1000, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); +} + +function run(auth: McpAuthContext, rawArgs: unknown = {}) { + return executeRegisteredTool({ + tool: dataModelGuideTool, + rawArgs, + auth, + correlationId: 'corr-1', + client: forbiddenClient(), + authorization: 'Bearer tok', + }); +} + +describe('dataModelGuideTool', () => { + it('returns the guide as text and structured content without calling upstream', async () => { + const result = await run(authContext(['b1'])); + + expect(result.isError).toBeUndefined(); + expect(result.content[0]!.text).toBe(DATA_MODEL_GUIDE); + const structured = result.structuredContent as { guide: string; version: string }; + expect(structured.guide).toBe(DATA_MODEL_GUIDE); + expect(structured.version).toMatch(/^\d+\.\d+\.\d+$/); + }); + + // This is the point of the tool: it has to be readable at cold start, before + // the model knows which business it is working with — or whether it has one. + it('works for a caller with no memberships at all', async () => { + const result = await run(authContext([])); + expect(result.isError).toBeUndefined(); + expect(result.content[0]!.text).toBe(DATA_MODEL_GUIDE); + }); + + it('rejects unknown arguments', async () => { + const result = await run(authContext(['b1']), { businessId: 'b1' }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('VALIDATION_ERROR'); + }); + + it('is registered ahead of every data tool', () => { + const order = toolRegistry.describe().map(tool => tool.name); + const guideIndex = order.indexOf(DATA_MODEL_GUIDE_TOOL_NAME); + expect(guideIndex).toBeGreaterThanOrEqual(0); + expect(guideIndex).toBeLessThan(order.indexOf('accounter_list_accounts')); + expect(guideIndex).toBeLessThan(order.indexOf('accounter_search_charges')); + }); +}); + +describe('data-model guide content stays in sync', () => { + // A tool rename would leave the guide telling the model to call something that + // does not exist — the single most likely way this text goes stale. + it('only references tools that are actually registered', () => { + const registered = new Set(toolRegistry.list().map(tool => tool.name)); + const referenced = [...DATA_MODEL_GUIDE.matchAll(/accounter_[a-z_]+/g)].map(match => match[0]); + + expect(referenced.length).toBeGreaterThan(0); + for (const name of new Set(referenced)) { + expect(registered.has(name), `guide references unregistered tool ${name}`).toBe(true); + } + }); + + it('documents every charge type the connector can emit', () => { + const chargeTypes = KNOWN_CHARGE_TYPENAMES.map(typename => chargeTypeFromTypename(typename)!); + expect(chargeTypes.length).toBeGreaterThan(0); + for (const chargeType of chargeTypes) { + expect(DATA_MODEL_GUIDE, `guide omits chargeType ${chargeType}`).toContain(chargeType); + } + }); + + it('documents every account type', () => { + for (const accountType of [ + 'BANK_ACCOUNT', + 'BANK_DEPOSIT_ACCOUNT', + 'CREDIT_CARD', + 'CRYPTO_WALLET', + 'FOREIGN_SECURITIES', + ]) { + expect(DATA_MODEL_GUIDE).toContain(accountType); + } + }); + + // The traps the feedback session actually fell into. If a future edit trims + // the guide, these are the sentences that must survive. + it('keeps the warnings that cost the most to rediscover', () => { + // Card rows are settled again by a bank row. + expect(DATA_MODEL_GUIDE).toMatch(/double-count/i); + // Internal transfers are neither income nor expense. + expect(DATA_MODEL_GUIDE).toContain('internal_transfer'); + // Securities rows have no mirror leg. + expect(DATA_MODEL_GUIDE).toMatch(/single-legged/i); + // Per-transaction balances are unavailable, and cumulativeNet is not one. + expect(DATA_MODEL_GUIDE).toContain('cumulativeNet'); + expect(DATA_MODEL_GUIDE).toMatch(/balances are not available/i); + // Historical, not present-day, exchange rates. + expect(DATA_MODEL_GUIDE).toContain('exchangeRate'); + // Overlap-vs-containment date semantics. + expect(DATA_MODEL_GUIDE).toContain('fromAnyDate'); + }); + + // The whole guide is prompt budget the model pays on every read, and it is + // cheap to keep appending to. Currently ~5.8KB; this is the ceiling before an + // edit should be trading content out rather than adding. + it('stays within its prompt budget', () => { + expect(Buffer.byteLength(DATA_MODEL_GUIDE, 'utf8')).toBeLessThan(6_500); + }); +}); diff --git a/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts b/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts index 68e5a9b50..27f087109 100644 --- a/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts +++ b/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts @@ -3,6 +3,7 @@ import { buildAuthContext, type McpAuthContext } from '../../auth/identity.js'; import type { AuthPrincipal } from '../../auth/token.js'; import { BUSINESS_SCOPE_HEADER, UpstreamGraphQLClient } from '../../upstream/graphql-client.js'; import { LIST_BUSINESS_MEMBERSHIPS_TOOL_NAME } from '../businesses.js'; +import { DATA_MODEL_GUIDE_TOOL_NAME } from '../data-model-guide.js'; import { executeRegisteredTool } from '../execute.js'; import { toolRegistry } from '../registry-instance.js'; @@ -130,6 +131,12 @@ const ARGS_BY_TOOL: Record = { accounter_ledger_records: { businessId: B1 }, }; +/** + * Tools with pure handlers: discovery (memberships are already on the auth + * context) and the static data-model guide. Neither talks upstream. + */ +const PURE_TOOLS = new Set([LIST_BUSINESS_MEMBERSHIPS_TOOL_NAME, DATA_MODEL_GUIDE_TOOL_NAME]); + /** A tool that took a singular `businessId` must have narrowed the scope to it. */ function expectedScopeFor(name: string): string[] { const args = ARGS_BY_TOOL[name] as { businessId?: string } | undefined; @@ -159,8 +166,10 @@ describe('registry-wide business-scope forwarding', () => { expect(result.isError, `${name} should succeed`).toBeUndefined(); const structured = result.structuredContent as Record; - if (name === LIST_BUSINESS_MEMBERSHIPS_TOOL_NAME) { - // Discovery is pure and *is* the scope — no upstream call, no echo. + if (PURE_TOOLS.has(name)) { + // These make no upstream call, so there is nothing to forward and no + // scope to echo. Asserted rather than skipped: if one of them ever grows + // an upstream call, it must come back through the scope contract above. expect(headersSeen).toHaveLength(0); expect(structured).not.toHaveProperty('scope'); return; diff --git a/packages/mcp-server/src/tools/data-model-guide.ts b/packages/mcp-server/src/tools/data-model-guide.ts new file mode 100644 index 000000000..e190e4f14 --- /dev/null +++ b/packages/mcp-server/src/tools/data-model-guide.ts @@ -0,0 +1,158 @@ +import { z } from 'zod'; +import type { ToolDefinition, ToolResult } from './registry.js'; + +/** + * The data-model guide (feedback §8). + * + * The connector feedback singled out one missing thing as costing more than any + * other: nothing explains how Accounter's accounts and charges actually relate, + * so an agent reverse-engineers it from Hebrew bank descriptions and gets it + * subtly wrong. This tool is that explanation. + * + * It is a **tool, not an MCP resource**, deliberately. The handler advertises + * only `capabilities: { tools: { listChanged: false } }` (`mcp/handler.ts`), and + * implementing the resources protocol to serve one static document is more + * moving parts than payoff. As a tool it also shows up in `tools/list`, where a + * model actually looks. + * + * The handler is **pure** — no upstream call, no auth beyond being a registered + * caller — so it is safe to call first, before any business is chosen. + * + * Every claim below is checked against the code, not against the feedback's + * inferences; where the two differ (notably the card-settlement link, which + * *does* exist upstream) the code wins. + */ + +export const DATA_MODEL_GUIDE_TOOL_NAME = 'accounter_data_model_guide'; + +const dataModelGuideInput = z.object({}); +type DataModelGuideInput = z.infer; + +/** + * Bumped whenever the guide's content changes materially, so a client that + * caches it can tell. Not the package version — this tracks the text. + */ +export const DATA_MODEL_GUIDE_VERSION = '1.0.0'; + +export const DATA_MODEL_GUIDE = `# Accounter data model — read this before interpreting the numbers + +## Accounts (\`accounter_list_accounts\`) + +Every transaction belongs to an account, and the account's \`type\` determines whether you should +count it. Summing all transaction rows without checking \`type\` gives a wrong answer. + +- **\`BANK_ACCOUNT\`** — a real bank account. Its rows are actual cash movements. +- **\`CREDIT_CARD\`** — one row per merchant purchase. **These are not separate cash movements.** + The bank later debits a single settlement amount covering many card rows, and that settlement is + its own \`BANK_ACCOUNT\` row. Counting both double-counts every card purchase. +- **\`BANK_DEPOSIT_ACCOUNT\`** — deposits. Money is swept here out of the checking account, so a + checking account can show a near-zero running total by design while the money simply sits in a + deposit. Sweeps are transfers, not expenses. +- **\`FOREIGN_SECURITIES\`** — securities activity. These rows are **single-legged**: a buy or sell + appears here with no mirror row in a bank account. Summing everything therefore gives cash only; + excluding them gives cost basis. Neither is market value — unrealized gains are not in this data. +- **\`CRYPTO_WALLET\`** — crypto holdings, valued through crypto exchange rates. + +## Charges group the legs + +A **charge** is the unit of bookkeeping: it groups the transactions and documents that belong to one +economic event. \`chargeType\` (and the derived \`flowKind\`) tells you what kind of event it was — +this is the reliable signal. Do **not** classify by matching Hebrew strings in \`description\`. + +| \`chargeType\` | \`flowKind\` | Meaning | +| --- | --- | --- | +| \`COMMON\` | \`income\` / \`expense\` | Ordinary business activity; direction from the amount sign. | +| \`CREDITCARD_BANK\` | \`internal_transfer\` | A bank settlement paying off card purchases. **It groups both legs** — the settlement transaction and the card transactions it covers — so the pairing you need is here. | +| \`BANK_DEPOSIT\` | \`internal_transfer\` | A sweep between checking and a deposit. | +| \`INTERNAL\` | \`internal_transfer\` | A transfer between the business's own accounts. | +| \`CONVERSION\` | \`conversion\` | One currency exchanged for another; two legs, no net change in value. | +| \`FOREIGN_SECURITIES\` | \`investment\` | Securities buy/sell. | +| \`PAYROLL\` | \`payroll\` | Salary payments. | +| \`VAT\` | \`tax\` | A monthly VAT settlement with the tax authority. | +| \`DIVIDEND\` | \`dividend\` | Dividend distribution. | +| \`BUSINESS_TRIP\` | \`expense\` | Travel expenses. | +| \`FINANCIAL\` | \`financial\` | Fees, interest, and similar financial items. | + +**\`flowKind: internal_transfer\` is the one to watch.** Those charges move money between accounts +the business already owns. They are neither income nor expense, and including them inflates both. + +## Answering "how much did we make?" + +Prefer the report tools over deriving numbers from rows — they use the double-entry ledger, which is +authoritative; the raw bank feed is not. \`accounter_profit_and_loss\` (yearly P&L), +\`accounter_income_expense_summary\` (monthly, currency-converted), \`accounter_counterparty_totals\` +(who money came from or went to), \`accounter_vat_report\` (output vs input VAT). + +## Balances are not available per transaction + +Only some bank feeds report a running balance; card, deposit and SWIFT rows store a placeholder +indistinguishable from a real zero, so the connector does not expose it rather than hand you a wrong +number. So: \`accounter_list_accounts\` returns **no balance field**, and +\`accounter_income_expense_summary\`'s \`cumulativeNet\` is a running sum of that period's own flows +from zero — a **trend, not a balance**, and it counts card rows alongside the bank rows settling +them. Reason about cash position from account \`type\` and flows, and say plainly that exact balances +are unavailable. + +## Currency + +Transactions carry \`amount\` in origin currency plus \`amountLocal\` and \`exchangeRate\` — the +historical rate for that transaction's own event date. **Use those.** Applying today's rate to years +of history is a common and material error. Both are \`null\` when no rate is on file; that means +"unknown", not "1". Report tools convert server-side and take a \`currency\` argument. + +## Documents + +\`direction\`: \`issued\` = the business raised the document (revenue), \`received\` = it was billed +(expense). It is \`null\` when the business is neither party — credit invoices, or a missing creditor +— and null means **undetermined**, not "either". \`amountExVat\` is the amount net of VAT. + +## Date filters — the subtle one + +A charge draws dates from several sources (documents, event dates, debit dates, ledger dates), and +the two filter families ask different questions: + +- \`fromDate\` / \`toDate\` — **containment**: the charge's earliest *and* latest dates must fall + inside the window. A charge straddling the boundary is excluded. +- \`fromAnyDate\` / \`toAnyDate\` — **overlap**: the charge has *some* activity in the window. + +\`accounter_search_charges\` uses **overlap**, so its results can legitimately include charges with +older event dates — expected, not a bug. For containment use \`accounter_get_charges\`, which exposes +all four directly. Transactions are simpler: \`fromEventDate\`/\`toEventDate\`, +\`fromDebitDate\`/\`toDebitDate\`, or \`fromAnyDate\`/\`toAnyDate\` for either. + +## Reading responses + +List responses report \`returnedCount\`, \`totalCount\` and \`truncated\`, and echo the effective +\`scope.businessIds\`. When \`truncated\` is true you are not seeing everything — narrow the filters +rather than assuming the total. Call \`accounter_list_business_memberships\` first to learn which +businesses you can query. +`; + +function handler(_input: DataModelGuideInput): ToolResult { + return { + content: [{ type: 'text', text: DATA_MODEL_GUIDE }], + structuredContent: { + guide: DATA_MODEL_GUIDE, + version: DATA_MODEL_GUIDE_VERSION, + }, + }; +} + +export const dataModelGuideTool: ToolDefinition = { + name: DATA_MODEL_GUIDE_TOOL_NAME, + description: + 'Explains how Accounter models accounts, charges, documents, currency, and date filters — ' + + 'including the traps that make naive aggregation wrong (credit-card rows are settled again by ' + + 'a bank row, deposit sweeps and internal transfers are not expenses, securities rows have no ' + + 'mirror leg, and per-transaction balances are unavailable). Read this BEFORE summing ' + + 'transactions, computing revenue, or reporting how much money the business has. Takes no ' + + 'parameters and needs no business scope, so it is safe to call first. Read-only.', + inputSchema: dataModelGuideInput, + policy: { + // No scope required: this is documentation, not business data, and gating it + // would make it unavailable in exactly the cold-start moment it is for. + requiresBusinessScope: false, + dataClassification: 'public', + }, + handler, +}; diff --git a/packages/mcp-server/src/tools/registry-instance.ts b/packages/mcp-server/src/tools/registry-instance.ts index c8af32ea6..e6576dd0f 100644 --- a/packages/mcp-server/src/tools/registry-instance.ts +++ b/packages/mcp-server/src/tools/registry-instance.ts @@ -2,6 +2,7 @@ import { listAccountsTool } from './accounts.js'; import { listBusinessMembershipsTool } from './businesses.js'; import { getChargesTool } from './charge-details.js'; import { searchChargesTool } from './charges.js'; +import { dataModelGuideTool } from './data-model-guide.js'; import { getDocumentsTool } from './document-details.js'; import { incomeExpenseSummaryTool, profitAndLossTool, vatReportTool } from './financial-reports.js'; import { counterpartyTotalsTool, ledgerRecordsTool } from './ledger-reports.js'; @@ -24,6 +25,10 @@ export const toolRegistry = new ToolRegistry(); // `listedTools` is now empty (the internal smoke tool is dispatchable but no // longer advertised), so this is genuinely the first tool the model sees. toolRegistry.register(listBusinessMembershipsTool); +// The data-model guide sits with discovery, ahead of every data tool: it is the +// one thing that has to be read *before* interpreting results, it needs no +// scope, and it costs no upstream call. +toolRegistry.register(dataModelGuideTool); // Answers before rows. //