From a8f7fd945a4c3f20966a1ea5999542acac55546d Mon Sep 17 00:00:00 2001 From: Gil Gardosh Date: Wed, 5 Aug 2026 12:25:02 +0300 Subject: [PATCH 1/4] plan --- docs/mcp-extension/plan.md | 248 +++++++++++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 docs/mcp-extension/plan.md diff --git a/docs/mcp-extension/plan.md b/docs/mcp-extension/plan.md new file mode 100644 index 000000000..efbc37bca --- /dev/null +++ b/docs/mcp-extension/plan.md @@ -0,0 +1,248 @@ +# Accounter MCP — UX improvements from the agent-session feedback + +## Context + +`packages/mcp-server/docs/accounter_mcp_feedback.md` records a real agent session that answered +three questions ("how much did The Guild make this year?", "chart my money over time", "what +changed?") against the connector. It cost ~100 paginated calls, four subagents, and a large amount +of reverse-engineering — because the connector exposes **rows, not answers**, and omits fields the +GraphQL API already returns. + +The key finding from exploring the schema: **most top-ranked asks are already available upstream and +simply aren't selected or wrapped.** `incomeExpenseChart`, `profitAndLossReport`, `vatReport`, +`businessTransactionsSumFromLedgerRecords`, `financialAccountsByOwner`, `allDeposits`, and +`business(id)` all exist as `Query` fields. Currency conversion data rides on every transaction as +`eventExchangeRates` / `debitExchangeRates`. + +**One exception, which reshaped this plan.** `Transaction.balance` looks like the answer to §2 but is +not trustworthy today, so exposing it as-is would ship a confidently wrong number. See §1.1 — it is +now a server-side prerequisite, not a Phase 1 item. + +So the bulk of the work is inside `packages/mcp-server` with no schema or resolver changes. +Server-side gaps are catalogued at the end as follow-up. + +Decisions taken: **MCP-only scope**, and **one named tool per report** (precise input schemas beat a +fuzzy `reportType` union for model selection accuracy). + +--- + +## Phase 1 — Fields that already exist but aren't selected + +These are a handful of lines each and address §4, §5 and §6 of the feedback. Do these first; they +change every existing tool's output. §2 (the biggest accuracy gap) turns out **not** to be a +select-the-field job — see 1.1. + +### 1.1 `balanceAfter` — DEFERRED, needs a server fix first + +`Transaction.balance: FinancialAmount!` exists and reads like the answer to §2, but it is **not +safe to expose today**. `packages/server/src/modules/transactions/resolvers/common.ts:57` is a bare +passthrough of the `transactions.current_balance` column, and only some ingestion paths populate it +from the source feed. Per the trigger definitions in +`packages/migrations/src/actions/2026-02-19T17-00-00.update-scraper-triggers-according-to-rls-restrictions.ts`: + +| Ingestion trigger | `current_balance` value | +| --- | --- | +| `insert_poalim_ils_transaction_handler` | `new.current_balance` — real | +| `insert_poalim_foreign_transaction_handler` | `new.current_balance` — real | +| `insert_bank_discount_transaction_handler` | `NEW.balance_after_operation` — real | +| `insert_poalim_swift_transaction_handler` | **hardcoded `0`** | +| `insert_poalim_deposit_transaction_handler` | **hardcoded `0`** | +| `insert_creditcard_` / `_max_` / `_cal_` / `_amex_transaction_handler` | **hardcoded `0`** | + +A hardcoded `0` is indistinguishable from a genuine zero balance, the GraphQL field is non-null so +there is no `null` to signal "unknown", and `Transaction` exposes no `sourceOrigin`, so the MCP layer +**cannot tell trustworthy rows from placeholder ones**. Exposing `balanceAfter` would replace a +reconstructed-but-honest number with a confidently wrong one — strictly worse than the status quo the +feedback complained about. + +**Prerequisite (server, out of the agreed MCP-only scope — see follow-up item 0):** change the +placeholder triggers to insert `NULL`, make `Transaction.balance` nullable, and have the resolver +return `null` for a null column. Once `null` means "this source doesn't report balances", the MCP +change becomes the three-line edit originally planned: + +- add `balance { raw formatted currency }` to the `McpTransactionDetailsFields` fragment in + `transaction-details.ts`, and to the inline transaction selection in `charge-details.ts` + (selections are deliberately not shared as interpolated fragments — see the header comment in + `entity-shapes.ts`); +- map it to `balanceAfter: NormalizedAmount | null` in `normalizeTransaction` + (`entity-shapes.ts`) via the existing `normalizeAmount`. + +Until then, §2 is addressed by `accounter_list_accounts` (Phase 2) — knowing an account's +`type` is what actually resolves the card-double-count, sweep, and securities traps — and the +data-model guide in Phase 3, which must state plainly that per-transaction balances are unavailable. + +### 1.2 `amountLocal` + `exchangeRate` on transactions (§5) + +`Transaction.eventExchangeRates: ExchangeRates` carries per-date rates for all supported currencies +(`ils`, `usd`, `eur`, `gbp`, …). Select it, compute in the normalizer, and **do not** emit the raw +rates object (11 floats per row would eat the payload budget). + +- Select `eventExchangeRates { date ils usd eur gbp aud cad jpy sek eth usdc grt }` alongside the + existing amount, in both transaction selections. +- In `entity-shapes.ts`, add a helper `toLocalAmount(amount, rates)`: looks up the rate keyed by + `amount.currency` (lower-cased), returns `{ amountLocal: { value, currency: 'ILS' }, exchangeRate }` + or `null` when the rate is absent. Emit only those two derived keys. + +### 1.3 `chargeType` / `flowKind` on charges (§4) + +The schema's concrete charge types are exactly the classification the feedback asks for: +`CommonCharge`, `ConversionCharge`, `InternalTransferCharge`, `BankDepositCharge`, +`ForeignSecuritiesCharge`, `CreditcardBankCharge`, `SalaryCharge`, `MonthlyVatCharge`, +`DividendCharge`, `BusinessTripCharge`, `FinancialCharge`. + +- `packages/mcp-server/src/tools/charges.ts` — add `__typename` to `McpSearchCharges`, expose it as + `chargeType`, and derive `flowKind` (`income | expense | internal_transfer | conversion | + investment | tax | payroll`) from typename + amount sign. Put the mapping table in + `entity-shapes.ts` so `charge-details.ts` reuses it. +- `charge-details.ts` already fetches typed charges — expose the same two fields there. +- This makes "what changed my total" answerable by filtering `flowKind` client-side in one pass + instead of 400+ join calls. + +### 1.4 Documents: `direction` + `amountExVat` (§6) + +Both are derivable from fields already selected — `document-details.ts` already pulls +`charge { id owner { id } }`, `creditor`, `debtor`, `amount`, `vat`. + +- In `normalizeDocument` (`entity-shapes.ts`): `direction = creditor?.id === chargeOwnerId ? + 'issued' : debtor?.id === chargeOwnerId ? 'received' : null`, and + `amountExVat = amount.value - (vat?.value ?? 0)` with matching currency. +- `normalizeDocument` needs the owner id; pass it through (it is already on the raw shape). +- Credit invoices are the known edge case the feedback hit — keep `direction` `null` rather than + guessing when neither party matches the owner, and say so in the tool description. + +### 1.5 `get_charges` payload defaults (§8) + +`charge-details.ts:135–143` defaults `includeTransactions` and `includeDocuments` to `true`, which +is what forced truncation on nearly every call. Flip both defaults to `false` and state in the +descriptions that the nested collections are opt-in. Existing callers that want nesting pass it +explicitly. + +--- + +## Phase 2 — Report tools (wrapping existing upstream queries) + +New file `packages/mcp-server/src/tools/financial-reports.ts`, registered in +`registry-instance.ts`. Every tool follows the established `balanceReportTool` pattern in +`reports.ts`: required singular `businessId`, `SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX`, +`policy: { requiredRoles: ['business_owner','accountant'], requiresBusinessScope: true, +dataClassification: 'business' }`, membership re-check against `context.readScope.businessIds`, +output via `shapeListResult`. + +**Scoping note that matters:** `incomeExpenseChart`, `profitAndLossReport` and `vatReport` derive +their owner from `AdminContextProvider.getVerifiedAdminContext()`, which resolves through +`resolveWriteTargetBusinessId(tenant.businessId, activeReadScope)` — i.e. from the forwarded +`x-business-scope`. Because these tools take a required singular `businessId`, `execute.ts` narrows +the scope to that one business and the upstream resolver targets it correctly. `vatReport` and +`businessTransactionsSumFromLedgerRecords` additionally take explicit ids — pass them too, as +defense in depth (mirroring the `byOwners` comment in `charges.ts:130`). + +| Tool | Upstream query | Answers | +| --- | --- | --- | +| `accounter_income_expense_summary` | `incomeExpenseChart(filters: { fromDate, toDate, currency })` | "how much did we make this year", "chart my money over time" — monthly `income`/`expense`/running `balance`, already currency-converted server-side. This is the §5 `convertTo` ask, free. | +| `accounter_profit_and_loss` | `profitAndLossReport(reportYear, referenceYears)` | §1 P&L: revenue, cost of sales, gross profit, R&D / marketing / G&A, operating profit, financial expenses, tax, net profit — plus reference years for YoY. | +| `accounter_vat_report` | `vatReport(filters: { financialEntityId, monthDate, chargesType })` | §1 VAT. Return the aggregated income/expense totals plus per-record rows; drop the heavy `missingInfo`/`differentMonthDoc` charge collections. | +| `accounter_counterparty_totals` | `businessTransactionsSumFromLedgerRecords(filters: { ownerIds, fromDate, toDate, businessIDs, type })` | §1 revenue/expense `groupBy: counterparty`, §7 "top customers" — per-business `credit`/`debit`/`total` from the **ledger**, which is the authoritative source the feedback asked for. Union result — handle the `CommonError` member. | +| `accounter_ledger_records` | `businessTransactionsFromLedgerRecords(filters: …)` | §1 `ledger_query`: `invoiceDate`, `business`, `counterAccount`, `amount`, `foreignAmount`, `chargeId`, `details`. | +| `accounter_list_accounts` | `financialAccountsByOwner(ownerId)` | §2: id, name, number, `type` (`BANK_ACCOUNT` / `BANK_DEPOSIT_ACCOUNT` / `CREDIT_CARD` / `CRYPTO_WALLET` / `FOREIGN_SECURITIES`), `privateOrBusiness`. The `type` enum alone resolves the card-double-count and securities traps. | + +`accounter_list_accounts` deliberately ships **without** current balances. The obvious +implementation — one `transactionsByFilters` call over a trailing window, newest `balance` per +account — inherits the §1.1 defect exactly: it would report `0` for every credit card, deposit and +SWIFT account. Add `includeCurrentBalance` only after the §1.1 prerequisite lands, and have it +return `null` (not `0`) for accounts whose source reports no balance. + +Caveat to state in `accounter_income_expense_summary`'s description: `incomeExpenseChart` computes +its running `balance` by summing every transaction amount for the owner, so it carries the same +card-settlement double-count the feedback describes in §2. The monthly `income`/`expense` split is +sound and currency-converted server-side; the cumulative `balance` is a net cash-flow figure, not an +account balance. Say so in the tool description rather than letting the model infer otherwise. + +**Ordering in `registry-instance.ts` is a prompt-engineering lever** (see its header comment). +Register the report tools *before* the raw list tools so the model reaches for the answer before the +rows: memberships → accounts → income/expense summary → P&L → VAT → counterparty totals → +search_charges → get_charges → get_transactions → get_documents → ledger_records → lookups → +balance_report. + +--- + +## Phase 3 — The data-model guide (§8) + +The feedback says a static doc "would have saved the entire reverse-engineering phase." The MCP +handler advertises only `capabilities: { tools: { listChanged: false } }` +(`packages/mcp-server/src/mcp/handler.ts:80`) — implementing the resources protocol is more work +than the payoff. Instead add a pure, no-upstream-call tool (same shape as `businesses.ts`, whose +handler is already pure): + +`accounter_data_model_guide` → returns a static markdown string covering: + +- Account types and what each means; **credit-card rows are duplicated by the bank's settlement + rows** — use `accounter_list_accounts` types, don't sum both. +- The Poalim checking→deposit auto-sweep, and that deposits are `BANK_DEPOSIT_ACCOUNT`. +- `FOREIGN_SECURITIES` accounts are single-legged (cost basis, no mirror bank leg). +- `chargeType` / `flowKind` semantics and which are internal movements. +- Date-filter semantics: `fromDate`/`toDate` vs `fromAnyDate`/`toAnyDate` (the §8 confusion — the + charges tool maps `fromDate` → `fromAnyDate` at `charges.ts:145`, which is exactly why 2020 event + dates came back for a 2026 query; document this and consider exposing both). +- **That per-transaction balances are not available**, and why: only Poalim ILS/foreign and Discount + feeds carry one; cards, deposits and SWIFT rows store a placeholder. Point the reader at + `accounter_income_expense_summary` for flows and at account `type` for what to include or exclude. + +Keep the text under ~4KB. Source it from a `const` in the tool module so it ships with the bundle. + +--- + +## Explicitly out of scope (server-side follow-ups) + +Record these in `packages/mcp-server/docs/todo.md` rather than implementing: + +0. **Make `current_balance` honest — prerequisite for §1.1, and the highest-value server item.** + Change the six placeholder triggers to insert `NULL` instead of `0`, backfill existing + placeholder rows (careful: a genuine `0` balance is legitimate, so backfill must key off + `source_origin` / account type, not the value), make `Transaction.balance` nullable in + `packages/server/src/modules/transactions/typeDefs/transactions.graphql.ts`, and return `null` + from the resolver at `resolvers/common.ts:57`. Needs a migration. Longer term the credit-card and + deposit scrapers should capture the balance the source does expose. +1. **Transaction sorting + pagination** (§3) — `TransactionsFilters` has no `sortBy`, and + `transactionsByFilters` returns an unbounded `[Transaction!]!`. Real cursor pagination and bulk + export need `packages/server/src/modules/transactions` changes. This is why the session made ~100 + calls; it is the largest remaining item. +2. **`Charge.totalAmount` null on income charges** (§7) — verify against + `packages/server/src/modules/charges`; if computable, compute server-side. +3. **`myMemberships.businessName` is nullable** (§7) — this is the `name: null` the reviewer saw. + Either fix the resolver's join or add an MCP fallback through `businesses(ids:)` in + `upstream/memberships.ts`. +4. **Document → payment status / AR aging** (§6), **securities positions** (§7), **payroll + breakdown** (§7), **deposits as first-class** (`allDeposits` exists — a thin wrapper is cheap and + could be pulled forward if wanted). +5. **Field projection** (§3) — mitigated by Phase 1.5 and the report tools; revisit only if payloads + still truncate. + +--- + +## Verification + +1. `yarn generate` — every new `/* GraphQL */` operation must produce types in + `packages/mcp-server/src/gql/`. Codegen plucks from plain template literals only, so no `${}` + interpolation in the new query strings. +2. `yarn workspace @accounter/mcp-server test` — extend the existing suites: + - `src/tools/__tests__/scope-forwarding.test.ts` and `business-scope-forwarding.test.ts` iterate + registered tools; new tools must pass without modification (they assert every tool sends + `x-business-scope`). + - Add per-tool tests mirroring `lookups.test.ts`: fixture upstream response → asserted normalized + shape, plus the union/`CommonError` branch for the two ledger tools. + - Assert `amountLocal`, `chargeType`, `flowKind`, and document `direction` in the + `entity-shapes` normalizer tests. (No `balanceAfter` assertions — §1.1 is deferred.) +3. `yarn lint && yarn prettier:check`. +4. End-to-end against the live connector (per `docs/local-development.md`): re-run the three + questions from the feedback doc and confirm each is now one or two calls — + - "how much did The Guild make this year?" → `accounter_income_expense_summary` or + `accounter_profit_and_loss`, single call. + - "chart how much money I have" → `accounter_list_accounts` + + `accounter_income_expense_summary`, two calls. This one is only *partially* fixed until + follow-up 0 lands — confirm the agent reports the flow figure with its double-count caveat + rather than presenting it as an account balance. + - "what made the changes?" → `accounter_search_charges` filtered on `flowKind`, no charge-join + fan-out. +5. Spot-check `accounter_get_transactions` output size — the Phase 1 additions add bytes per row; + confirm `MAX_TOOL_RESULT_BYTES` (60KB, `output.ts:14`) still fits a useful page, and lower the + default page size if not. From 4536335201046f589fe9bc4b7a8155d1484e5155 Mon Sep 17 00:00:00 2001 From: Gil Gardosh Date: Wed, 5 Aug 2026 12:31:25 +0300 Subject: [PATCH 2/4] prettier --- docs/mcp-extension/plan.md | 110 ++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/docs/mcp-extension/plan.md b/docs/mcp-extension/plan.md index efbc37bca..aadfd7b1f 100644 --- a/docs/mcp-extension/plan.md +++ b/docs/mcp-extension/plan.md @@ -14,9 +14,9 @@ simply aren't selected or wrapped.** `incomeExpenseChart`, `profitAndLossReport` `business(id)` all exist as `Query` fields. Currency conversion data rides on every transaction as `eventExchangeRates` / `debitExchangeRates`. -**One exception, which reshaped this plan.** `Transaction.balance` looks like the answer to §2 but is -not trustworthy today, so exposing it as-is would ship a confidently wrong number. See §1.1 — it is -now a server-side prerequisite, not a Phase 1 item. +**One exception, which reshaped this plan.** `Transaction.balance` looks like the answer to §2 but +is not trustworthy today, so exposing it as-is would ship a confidently wrong number. See §1.1 — it +is now a server-side prerequisite, not a Phase 1 item. So the bulk of the work is inside `packages/mcp-server` with no schema or resolver changes. Server-side gaps are catalogued at the end as follow-up. @@ -34,26 +34,26 @@ select-the-field job — see 1.1. ### 1.1 `balanceAfter` — DEFERRED, needs a server fix first -`Transaction.balance: FinancialAmount!` exists and reads like the answer to §2, but it is **not -safe to expose today**. `packages/server/src/modules/transactions/resolvers/common.ts:57` is a bare +`Transaction.balance: FinancialAmount!` exists and reads like the answer to §2, but it is **not safe +to expose today**. `packages/server/src/modules/transactions/resolvers/common.ts:57` is a bare passthrough of the `transactions.current_balance` column, and only some ingestion paths populate it from the source feed. Per the trigger definitions in `packages/migrations/src/actions/2026-02-19T17-00-00.update-scraper-triggers-according-to-rls-restrictions.ts`: -| Ingestion trigger | `current_balance` value | -| --- | --- | -| `insert_poalim_ils_transaction_handler` | `new.current_balance` — real | -| `insert_poalim_foreign_transaction_handler` | `new.current_balance` — real | -| `insert_bank_discount_transaction_handler` | `NEW.balance_after_operation` — real | -| `insert_poalim_swift_transaction_handler` | **hardcoded `0`** | -| `insert_poalim_deposit_transaction_handler` | **hardcoded `0`** | -| `insert_creditcard_` / `_max_` / `_cal_` / `_amex_transaction_handler` | **hardcoded `0`** | +| Ingestion trigger | `current_balance` value | +| ---------------------------------------------------------------------- | ------------------------------------ | +| `insert_poalim_ils_transaction_handler` | `new.current_balance` — real | +| `insert_poalim_foreign_transaction_handler` | `new.current_balance` — real | +| `insert_bank_discount_transaction_handler` | `NEW.balance_after_operation` — real | +| `insert_poalim_swift_transaction_handler` | **hardcoded `0`** | +| `insert_poalim_deposit_transaction_handler` | **hardcoded `0`** | +| `insert_creditcard_` / `_max_` / `_cal_` / `_amex_transaction_handler` | **hardcoded `0`** | A hardcoded `0` is indistinguishable from a genuine zero balance, the GraphQL field is non-null so -there is no `null` to signal "unknown", and `Transaction` exposes no `sourceOrigin`, so the MCP layer -**cannot tell trustworthy rows from placeholder ones**. Exposing `balanceAfter` would replace a -reconstructed-but-honest number with a confidently wrong one — strictly worse than the status quo the -feedback complained about. +there is no `null` to signal "unknown", and `Transaction` exposes no `sourceOrigin`, so the MCP +layer **cannot tell trustworthy rows from placeholder ones**. Exposing `balanceAfter` would replace +a reconstructed-but-honest number with a confidently wrong one — strictly worse than the status quo +the feedback complained about. **Prerequisite (server, out of the agreed MCP-only scope — see follow-up item 0):** change the placeholder triggers to insert `NULL`, make `Transaction.balance` nullable, and have the resolver @@ -64,12 +64,12 @@ change becomes the three-line edit originally planned: `transaction-details.ts`, and to the inline transaction selection in `charge-details.ts` (selections are deliberately not shared as interpolated fragments — see the header comment in `entity-shapes.ts`); -- map it to `balanceAfter: NormalizedAmount | null` in `normalizeTransaction` - (`entity-shapes.ts`) via the existing `normalizeAmount`. +- map it to `balanceAfter: NormalizedAmount | null` in `normalizeTransaction` (`entity-shapes.ts`) + via the existing `normalizeAmount`. -Until then, §2 is addressed by `accounter_list_accounts` (Phase 2) — knowing an account's -`type` is what actually resolves the card-double-count, sweep, and securities traps — and the -data-model guide in Phase 3, which must state plainly that per-transaction balances are unavailable. +Until then, §2 is addressed by `accounter_list_accounts` (Phase 2) — knowing an account's `type` is +what actually resolves the card-double-count, sweep, and securities traps — and the data-model guide +in Phase 3, which must state plainly that per-transaction balances are unavailable. ### 1.2 `amountLocal` + `exchangeRate` on transactions (§5) @@ -80,8 +80,9 @@ rates object (11 floats per row would eat the payload budget). - Select `eventExchangeRates { date ils usd eur gbp aud cad jpy sek eth usdc grt }` alongside the existing amount, in both transaction selections. - In `entity-shapes.ts`, add a helper `toLocalAmount(amount, rates)`: looks up the rate keyed by - `amount.currency` (lower-cased), returns `{ amountLocal: { value, currency: 'ILS' }, exchangeRate }` - or `null` when the rate is absent. Emit only those two derived keys. + `amount.currency` (lower-cased), returns + `{ amountLocal: { value, currency: 'ILS' }, exchangeRate }` or `null` when the rate is absent. + Emit only those two derived keys. ### 1.3 `chargeType` / `flowKind` on charges (§4) @@ -91,9 +92,9 @@ The schema's concrete charge types are exactly the classification the feedback a `DividendCharge`, `BusinessTripCharge`, `FinancialCharge`. - `packages/mcp-server/src/tools/charges.ts` — add `__typename` to `McpSearchCharges`, expose it as - `chargeType`, and derive `flowKind` (`income | expense | internal_transfer | conversion | - investment | tax | payroll`) from typename + amount sign. Put the mapping table in - `entity-shapes.ts` so `charge-details.ts` reuses it. + `chargeType`, and derive `flowKind` + (`income | expense | internal_transfer | conversion | investment | tax | payroll`) from typename + + amount sign. Put the mapping table in `entity-shapes.ts` so `charge-details.ts` reuses it. - `charge-details.ts` already fetches typed charges — expose the same two fields there. - This makes "what changed my total" answerable by filtering `flowKind` client-side in one pass instead of 400+ join calls. @@ -103,9 +104,9 @@ The schema's concrete charge types are exactly the classification the feedback a Both are derivable from fields already selected — `document-details.ts` already pulls `charge { id owner { id } }`, `creditor`, `debtor`, `amount`, `vat`. -- In `normalizeDocument` (`entity-shapes.ts`): `direction = creditor?.id === chargeOwnerId ? - 'issued' : debtor?.id === chargeOwnerId ? 'received' : null`, and - `amountExVat = amount.value - (vat?.value ?? 0)` with matching currency. +- In `normalizeDocument` (`entity-shapes.ts`): + `direction = creditor?.id === chargeOwnerId ? 'issued' : debtor?.id === chargeOwnerId ? 'received' : null`, + and `amountExVat = amount.value - (vat?.value ?? 0)` with matching currency. - `normalizeDocument` needs the owner id; pass it through (it is already on the raw shape). - Credit invoices are the known edge case the feedback hit — keep `direction` `null` rather than guessing when neither party matches the owner, and say so in the tool description. @@ -121,12 +122,11 @@ explicitly. ## Phase 2 — Report tools (wrapping existing upstream queries) -New file `packages/mcp-server/src/tools/financial-reports.ts`, registered in -`registry-instance.ts`. Every tool follows the established `balanceReportTool` pattern in -`reports.ts`: required singular `businessId`, `SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX`, -`policy: { requiredRoles: ['business_owner','accountant'], requiresBusinessScope: true, -dataClassification: 'business' }`, membership re-check against `context.readScope.businessIds`, -output via `shapeListResult`. +New file `packages/mcp-server/src/tools/financial-reports.ts`, registered in `registry-instance.ts`. +Every tool follows the established `balanceReportTool` pattern in `reports.ts`: required singular +`businessId`, `SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX`, +`policy: { requiredRoles: ['business_owner','accountant'], requiresBusinessScope: true, dataClassification: 'business' }`, +membership re-check against `context.readScope.businessIds`, output via `shapeListResult`. **Scoping note that matters:** `incomeExpenseChart`, `profitAndLossReport` and `vatReport` derive their owner from `AdminContextProvider.getVerifiedAdminContext()`, which resolves through @@ -136,14 +136,14 @@ the scope to that one business and the upstream resolver targets it correctly. ` `businessTransactionsSumFromLedgerRecords` additionally take explicit ids — pass them too, as defense in depth (mirroring the `byOwners` comment in `charges.ts:130`). -| Tool | Upstream query | Answers | -| --- | --- | --- | -| `accounter_income_expense_summary` | `incomeExpenseChart(filters: { fromDate, toDate, currency })` | "how much did we make this year", "chart my money over time" — monthly `income`/`expense`/running `balance`, already currency-converted server-side. This is the §5 `convertTo` ask, free. | -| `accounter_profit_and_loss` | `profitAndLossReport(reportYear, referenceYears)` | §1 P&L: revenue, cost of sales, gross profit, R&D / marketing / G&A, operating profit, financial expenses, tax, net profit — plus reference years for YoY. | -| `accounter_vat_report` | `vatReport(filters: { financialEntityId, monthDate, chargesType })` | §1 VAT. Return the aggregated income/expense totals plus per-record rows; drop the heavy `missingInfo`/`differentMonthDoc` charge collections. | -| `accounter_counterparty_totals` | `businessTransactionsSumFromLedgerRecords(filters: { ownerIds, fromDate, toDate, businessIDs, type })` | §1 revenue/expense `groupBy: counterparty`, §7 "top customers" — per-business `credit`/`debit`/`total` from the **ledger**, which is the authoritative source the feedback asked for. Union result — handle the `CommonError` member. | -| `accounter_ledger_records` | `businessTransactionsFromLedgerRecords(filters: …)` | §1 `ledger_query`: `invoiceDate`, `business`, `counterAccount`, `amount`, `foreignAmount`, `chargeId`, `details`. | -| `accounter_list_accounts` | `financialAccountsByOwner(ownerId)` | §2: id, name, number, `type` (`BANK_ACCOUNT` / `BANK_DEPOSIT_ACCOUNT` / `CREDIT_CARD` / `CRYPTO_WALLET` / `FOREIGN_SECURITIES`), `privateOrBusiness`. The `type` enum alone resolves the card-double-count and securities traps. | +| Tool | Upstream query | Answers | +| ---------------------------------- | ------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `accounter_income_expense_summary` | `incomeExpenseChart(filters: { fromDate, toDate, currency })` | "how much did we make this year", "chart my money over time" — monthly `income`/`expense`/running `balance`, already currency-converted server-side. This is the §5 `convertTo` ask, free. | +| `accounter_profit_and_loss` | `profitAndLossReport(reportYear, referenceYears)` | §1 P&L: revenue, cost of sales, gross profit, R&D / marketing / G&A, operating profit, financial expenses, tax, net profit — plus reference years for YoY. | +| `accounter_vat_report` | `vatReport(filters: { financialEntityId, monthDate, chargesType })` | §1 VAT. Return the aggregated income/expense totals plus per-record rows; drop the heavy `missingInfo`/`differentMonthDoc` charge collections. | +| `accounter_counterparty_totals` | `businessTransactionsSumFromLedgerRecords(filters: { ownerIds, fromDate, toDate, businessIDs, type })` | §1 revenue/expense `groupBy: counterparty`, §7 "top customers" — per-business `credit`/`debit`/`total` from the **ledger**, which is the authoritative source the feedback asked for. Union result — handle the `CommonError` member. | +| `accounter_ledger_records` | `businessTransactionsFromLedgerRecords(filters: …)` | §1 `ledger_query`: `invoiceDate`, `business`, `counterAccount`, `amount`, `foreignAmount`, `chargeId`, `details`. | +| `accounter_list_accounts` | `financialAccountsByOwner(ownerId)` | §2: id, name, number, `type` (`BANK_ACCOUNT` / `BANK_DEPOSIT_ACCOUNT` / `CREDIT_CARD` / `CRYPTO_WALLET` / `FOREIGN_SECURITIES`), `privateOrBusiness`. The `type` enum alone resolves the card-double-count and securities traps. | `accounter_list_accounts` deliberately ships **without** current balances. The obvious implementation — one `transactionsByFilters` call over a trailing window, newest `balance` per @@ -158,7 +158,7 @@ sound and currency-converted server-side; the cumulative `balance` is a net cash account balance. Say so in the tool description rather than letting the model infer otherwise. **Ordering in `registry-instance.ts` is a prompt-engineering lever** (see its header comment). -Register the report tools *before* the raw list tools so the model reaches for the answer before the +Register the report tools _before_ the raw list tools so the model reaches for the answer before the rows: memberships → accounts → income/expense summary → P&L → VAT → counterparty totals → search_charges → get_charges → get_transactions → get_documents → ledger_records → lookups → balance_report. @@ -230,19 +230,19 @@ Record these in `packages/mcp-server/docs/todo.md` rather than implementing: `x-business-scope`). - Add per-tool tests mirroring `lookups.test.ts`: fixture upstream response → asserted normalized shape, plus the union/`CommonError` branch for the two ledger tools. - - Assert `amountLocal`, `chargeType`, `flowKind`, and document `direction` in the - `entity-shapes` normalizer tests. (No `balanceAfter` assertions — §1.1 is deferred.) + - Assert `amountLocal`, `chargeType`, `flowKind`, and document `direction` in the `entity-shapes` + normalizer tests. (No `balanceAfter` assertions — §1.1 is deferred.) 3. `yarn lint && yarn prettier:check`. -4. End-to-end against the live connector (per `docs/local-development.md`): re-run the three - questions from the feedback doc and confirm each is now one or two calls — +4. End-to-end against the live connector (per `packages/mcp-server/docs/local-development.md`): + re-run the three questions from the feedback doc and confirm each is now one or two calls — - "how much did The Guild make this year?" → `accounter_income_expense_summary` or `accounter_profit_and_loss`, single call. - - "chart how much money I have" → `accounter_list_accounts` + - `accounter_income_expense_summary`, two calls. This one is only *partially* fixed until - follow-up 0 lands — confirm the agent reports the flow figure with its double-count caveat - rather than presenting it as an account balance. + - "chart how much money I have" → `accounter_list_accounts` + `accounter_income_expense_summary`, + two calls. This one is only _partially_ fixed until follow-up 0 lands — confirm the agent + reports the flow figure with its double-count caveat rather than presenting it as an account + balance. - "what made the changes?" → `accounter_search_charges` filtered on `flowKind`, no charge-join fan-out. 5. Spot-check `accounter_get_transactions` output size — the Phase 1 additions add bytes per row; - confirm `MAX_TOOL_RESULT_BYTES` (60KB, `output.ts:14`) still fits a useful page, and lower the - default page size if not. + confirm `MAX_TOOL_RESULT_BYTES` (60KB, `packages/mcp-server/src/tools/output.ts:14`) still fits a + useful page, and lower the default page size if not. From 74fd9760231434d62f676efa0a58fbfbf1af93fb Mon Sep 17 00:00:00 2001 From: Gil Gardosh Date: Wed, 5 Aug 2026 12:32:10 +0300 Subject: [PATCH 3/4] Phase 1 --- docs/mcp-extension/plan.md | 5 +- .../src/tools/__tests__/charges.test.ts | 33 ++- .../src/tools/__tests__/detail-tools.test.ts | 79 ++++++- .../src/tools/__tests__/entity-shapes.test.ts | 212 +++++++++++++++++ .../tools/__tests__/schema-contract.test.ts | 17 ++ .../mcp-server/src/tools/charge-details.ts | 43 +++- packages/mcp-server/src/tools/charges.ts | 11 +- .../mcp-server/src/tools/document-details.ts | 2 +- .../mcp-server/src/tools/entity-shapes.ts | 222 ++++++++++++++++++ .../src/tools/transaction-details.ts | 16 +- 10 files changed, 627 insertions(+), 13 deletions(-) create mode 100644 packages/mcp-server/src/tools/__tests__/entity-shapes.test.ts diff --git a/docs/mcp-extension/plan.md b/docs/mcp-extension/plan.md index aadfd7b1f..d38772e7d 100644 --- a/docs/mcp-extension/plan.md +++ b/docs/mcp-extension/plan.md @@ -26,7 +26,10 @@ fuzzy `reportType` union for model selection accuracy). --- -## Phase 1 — Fields that already exist but aren't selected +## Phase 1 — Fields that already exist but aren't selected — ✅ DONE + +_Implemented in `packages/mcp-server/src/tools/` (1.2–1.5; 1.1 deferred as described below). 441 +tests pass; `yarn generate`, `yarn lint` and the mcp-server build are clean._ These are a handful of lines each and address §4, §5 and §6 of the feedback. Do these first; they change every existing tool's output. §2 (the biggest accuracy gap) turns out **not** to be a diff --git a/packages/mcp-server/src/tools/__tests__/charges.test.ts b/packages/mcp-server/src/tools/__tests__/charges.test.ts index d7df6de46..48ffdc6b9 100644 --- a/packages/mcp-server/src/tools/__tests__/charges.test.ts +++ b/packages/mcp-server/src/tools/__tests__/charges.test.ts @@ -70,13 +70,16 @@ describe('searchChargesTool — successful read', () => { totalCount: number; truncated: boolean; }; - // This fixture omits `owner` on purpose — it predates the field. Owner - // tagging must degrade to nulls rather than throwing, so older fixtures and - // any upstream that stops returning the field keep working. + // This fixture omits `owner` and `__typename` on purpose — it predates both + // fields. Owner tagging and charge classification must degrade to + // nulls/`unknown` rather than throwing, so older fixtures and any upstream + // that stops returning a field keep working. expect(structured.charges).toEqual([ { id: 'c1', description: 'Coffee', + chargeType: null, + flowKind: 'unknown', ownerId: null, ownerName: null, amount: { value: 12.5, formatted: '₪12.50', currency: 'ILS' }, @@ -88,6 +91,30 @@ describe('searchChargesTool — successful read', () => { expect(structured.pagination.hasNextPage).toBe(false); }); + it('tags each charge with its type and flow kind', async () => { + const client = clientReturning({ + allCharges: { + nodes: [ + { + __typename: 'InternalTransferCharge', + id: 'c1', + userDescription: 'Sweep to deposit', + owner: { id: 'b1', name: 'Acme' }, + totalAmount: { raw: -50_000, formatted: '₪-50,000.00', currency: 'ILS' }, + minEventDate: '2026-01-05', + }, + ], + pageInfo: { totalPages: 1, totalRecords: 1, currentPage: 1, pageSize: 25 }, + }, + }); + const result = await run(client, authContext(['b1']), {}); + const { charges } = result.structuredContent as { + charges: Array<{ chargeType: string; flowKind: string }>; + }; + // Negative amount, but it is not an expense — the money stayed in-house. + expect(charges[0]).toMatchObject({ chargeType: 'INTERNAL', flowKind: 'internal_transfer' }); + }); + it('tags each charge with its owning business', async () => { const client = clientReturning({ allCharges: { diff --git a/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts b/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts index 2054ee6d8..7ffd1fbc5 100644 --- a/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts +++ b/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts @@ -198,7 +198,7 @@ describe('getChargesTool', () => { const client = clientReturning(chargeFixture, body => (sentBody = body)); await run(getChargesTool, client, authContext([B1]), { chargeIds: ['c1', 'c2'], - includeDocuments: false, + includeTransactions: true, }); const variables = ( sentBody as { @@ -210,6 +210,19 @@ describe('getChargesTool', () => { expect(variables.includeDocuments).toBe(false); }); + // Nesting transactions/documents by default is what forced nearly every call + // to spill over the payload budget, so both are opt-in. + it('omits nested transactions and documents unless asked', async () => { + let sentBody: unknown; + const client = clientReturning(chargeFixture, body => (sentBody = body)); + await run(getChargesTool, client, authContext([B1]), { chargeIds: ['c1'] }); + const variables = ( + sentBody as { variables: { includeTransactions: boolean; includeDocuments: boolean } } + ).variables; + expect(variables.includeTransactions).toBe(false); + expect(variables.includeDocuments).toBe(false); + }); + it('forwards all available filters to allCharges', async () => { let sentBody: unknown; const client = clientReturning(filteredChargeFixture, body => (sentBody = body)); @@ -312,6 +325,32 @@ describe('getChargesTool', () => { expect(structured.charges[0]?.id).toBe('c1'); }); + it('classifies a charge from its typename', async () => { + const client = clientReturning({ + chargesByIDs: [ + { ...chargeFixture.chargesByIDs[0], __typename: 'CreditcardBankCharge' }, + { + ...chargeFixture.chargesByIDs[0], + id: 'c2', + __typename: 'CommonCharge', + totalAmount: { raw: 5000, formatted: '₪5,000.00', currency: 'ILS' }, + }, + ], + }); + const result = await run(getChargesTool, client, authContext([B1]), { + chargeIds: ['c1', 'c2'], + }); + const { charges } = result.structuredContent as { + charges: Array<{ chargeType: string | null; flowKind: string }>; + }; + // A card settlement moves money between the owner's own accounts. + expect(charges[0]).toMatchObject({ + chargeType: 'CREDITCARD_BANK', + flowKind: 'internal_transfer', + }); + expect(charges[1]).toMatchObject({ chargeType: 'COMMON', flowKind: 'income' }); + }); + it('drops a charge whose owner is outside the resolved scope (defense-in-depth)', async () => { const client = clientReturning(chargeFixture); // Caller is authorized for B2 only; the fixture charge is owned by B1. @@ -420,6 +459,31 @@ describe('getTransactionsTool', () => { expect(variables.transactionIDs).toEqual(['tx1', 'tx2']); }); + it('converts a foreign-currency row using its own event-date rate', async () => { + const client = clientReturning({ + transactionsByIDs: [ + { + ...fixture.transactionsByIDs[0], + amount: { raw: -100, formatted: '$-100.00', currency: 'USD' }, + eventExchangeRates: { date: '2026-01-05', ils: 1, usd: 3.5 }, + }, + ], + }); + const result = await run(getTransactionsTool, client, authContext([B1]), { + transactionIds: ['tx1'], + }); + const [transaction] = ( + result.structuredContent as { + transactions: Array<{ + amountLocal: { value: number; currency: string } | null; + exchangeRate: number | null; + }>; + } + ).transactions; + expect(transaction!.amountLocal).toEqual({ value: -350, currency: 'ILS' }); + expect(transaction!.exchangeRate).toBe(3.5); + }); + it('reports no matches for an empty upstream result', async () => { const client = clientReturning({ transactionsByIDs: [] }); const result = await run(getTransactionsTool, client, authContext([B1]), { @@ -605,6 +669,19 @@ describe('getDocumentsTool', () => { expect(structured.documents[0]!.vat).toEqual({ value: -17, formatted: '₪-17.00', currency: 'ILS' }); }); + it('exposes direction and amountExVat on each row', async () => { + const client = clientReturning({ documentsByIds: [doc()] }); + const result = await run(getDocumentsTool, client, authContext([B1]), { documentIds: ['d1'] }); + const [document] = ( + result.structuredContent as { + documents: Array<{ direction: string | null; amountExVat: { value: number } | null }>; + } + ).documents; + // B1 is the debtor on this fixture, so the business was billed. + expect(document!.direction).toBe('received'); + expect(document!.amountExVat?.value).toBe(-103); + }); + it('drops a document whose owning charge is outside scope', async () => { const client = clientReturning({ documentsByIds: [doc({ charge: { id: 'c9', owner: { id: B2 } } })], diff --git a/packages/mcp-server/src/tools/__tests__/entity-shapes.test.ts b/packages/mcp-server/src/tools/__tests__/entity-shapes.test.ts new file mode 100644 index 000000000..43a6f61e4 --- /dev/null +++ b/packages/mcp-server/src/tools/__tests__/entity-shapes.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from 'vitest'; +import { + chargeTypeFromTypename, + documentDirection, + flowKindForCharge, + LOCAL_CURRENCY, + normalizeDocument, + normalizeTransaction, + toLocalAmount, + type RawDocument, + type RawTransaction, +} from '../entity-shapes.js'; + +/** + * Unit coverage for the derived fields the detail tools expose: local-currency + * conversion, charge classification, and document direction. These are pure + * functions, so they are exercised directly rather than through the executor — + * the tool-level wiring is covered in `detail-tools.test.ts`. + */ + +// --------------------------------------------------------------------------- +// Currency conversion +// --------------------------------------------------------------------------- + +describe('toLocalAmount', () => { + const rates = { date: '2021-03-04', ils: 1, usd: 3.3, eur: 4, eth: null }; + + // Upstream quotes every field as "local currency per one unit", so the rate + // is a multiplier. Getting this inverted would silently scale six years of + // history by ~1/rate², which is exactly the class of error this replaces. + it('multiplies by the rate for the amount’s own currency', () => { + expect(toLocalAmount({ raw: 100, formatted: '$100', currency: 'USD' }, rates)).toEqual({ + amountLocal: { value: 330, currency: LOCAL_CURRENCY }, + exchangeRate: 3.3, + }); + }); + + it('matches the currency case-insensitively and keeps local amounts unchanged', () => { + expect(toLocalAmount({ raw: 50, formatted: '₪50', currency: 'ILS' }, rates)).toEqual({ + amountLocal: { value: 50, currency: LOCAL_CURRENCY }, + exchangeRate: 1, + }); + }); + + it('preserves the sign of a debit', () => { + const converted = toLocalAmount({ raw: -20, formatted: '-$20', currency: 'USD' }, rates); + expect(converted?.amountLocal.value).toBeCloseTo(-66); + }); + + // Returning null (rather than the origin-currency number) keeps an + // unconvertible row visibly unconverted instead of quietly mixing currencies. + it.each([ + ['a currency with no rate on file', { raw: 10, formatted: '₿10', currency: 'BTC' }, rates], + ['a null rate', { raw: 10, formatted: 'Ξ10', currency: 'ETH' }, rates], + ['missing rates entirely', { raw: 10, formatted: '$10', currency: 'USD' }, null], + ])('returns null for %s', (_label, amount, rateSet) => { + expect(toLocalAmount(amount, rateSet)).toBeNull(); + }); + + it('returns null when there is no amount', () => { + expect(toLocalAmount(null, rates)).toBeNull(); + }); +}); + +describe('normalizeTransaction', () => { + const base: RawTransaction = { + id: 'tx1', + chargeId: 'c1', + eventDate: '2021-03-04', + direction: 'CREDIT', + amount: { raw: 100, formatted: '$100', currency: 'USD' }, + sourceDescription: 'WIRE IN', + }; + + it('exposes the derived pair and not the raw rates object', () => { + const normalized = normalizeTransaction({ + ...base, + eventExchangeRates: { date: '2021-03-04', usd: 3.3 }, + }); + expect(normalized.amountLocal).toEqual({ value: 330, currency: LOCAL_CURRENCY }); + expect(normalized.exchangeRate).toBe(3.3); + expect(normalized).not.toHaveProperty('eventExchangeRates'); + }); + + it('degrades to nulls when the rates are absent', () => { + const normalized = normalizeTransaction(base); + expect(normalized.amountLocal).toBeNull(); + expect(normalized.exchangeRate).toBeNull(); + expect(normalized.amount).toEqual({ value: 100, formatted: '$100', currency: 'USD' }); + }); +}); + +// --------------------------------------------------------------------------- +// Charge classification +// --------------------------------------------------------------------------- + +describe('chargeTypeFromTypename', () => { + it('maps typenames onto the byChargeTypes filter vocabulary', () => { + expect(chargeTypeFromTypename('CommonCharge')).toBe('COMMON'); + expect(chargeTypeFromTypename('SalaryCharge')).toBe('PAYROLL'); + expect(chargeTypeFromTypename('MonthlyVatCharge')).toBe('VAT'); + expect(chargeTypeFromTypename('CreditcardBankCharge')).toBe('CREDITCARD_BANK'); + }); + + it('returns null for an absent or unrecognized typename', () => { + expect(chargeTypeFromTypename(undefined)).toBeNull(); + expect(chargeTypeFromTypename('SomeFutureCharge')).toBeNull(); + }); +}); + +describe('flowKindForCharge', () => { + const ils = (raw: number) => ({ raw, formatted: `₪${raw}`, currency: 'ILS' }); + + // The whole point of `flowKind`: these three move money between the owner's + // own accounts, so counting them as income/expense double-counts. + it.each(['INTERNAL', 'BANK_DEPOSIT', 'CREDITCARD_BANK'])( + 'classifies %s as an internal transfer regardless of sign', + chargeType => { + expect(flowKindForCharge(chargeType, ils(5000))).toBe('internal_transfer'); + expect(flowKindForCharge(chargeType, ils(-5000))).toBe('internal_transfer'); + }, + ); + + it('maps the remaining intrinsic types', () => { + expect(flowKindForCharge('CONVERSION', ils(-1))).toBe('conversion'); + expect(flowKindForCharge('FOREIGN_SECURITIES', ils(-1))).toBe('investment'); + expect(flowKindForCharge('PAYROLL', ils(-1))).toBe('payroll'); + expect(flowKindForCharge('VAT', ils(-1))).toBe('tax'); + expect(flowKindForCharge('DIVIDEND', ils(-1))).toBe('dividend'); + expect(flowKindForCharge('FINANCIAL', ils(-1))).toBe('financial'); + expect(flowKindForCharge('BUSINESS_TRIP', ils(1))).toBe('expense'); + }); + + it('falls back to the amount sign for the catch-all COMMON type', () => { + expect(flowKindForCharge('COMMON', ils(1200))).toBe('income'); + expect(flowKindForCharge('COMMON', ils(-1200))).toBe('expense'); + }); + + it('stays unknown rather than guessing without a type or amount', () => { + expect(flowKindForCharge(null, ils(100))).toBe('unknown'); + expect(flowKindForCharge('COMMON', null)).toBe('unknown'); + expect(flowKindForCharge('COMMON', ils(0))).toBe('unknown'); + }); +}); + +// --------------------------------------------------------------------------- +// Documents +// --------------------------------------------------------------------------- + +describe('documentDirection', () => { + const OWNER = 'owner-1'; + const OTHER = 'other-1'; + const doc = (creditor: string | null, debtor: string | null): RawDocument => ({ + id: 'd1', + creditor: creditor ? { id: creditor, name: creditor } : null, + debtor: debtor ? { id: debtor, name: debtor } : null, + }); + + it('reads issued when the owner is the creditor', () => { + expect(documentDirection(doc(OWNER, OTHER), OWNER)).toBe('issued'); + }); + + it('reads received when the owner is the debtor', () => { + expect(documentDirection(doc(OTHER, OWNER), OWNER)).toBe('received'); + }); + + // A wrong direction flips an expense into revenue, so the ambiguous cases the + // feedback hit — credit invoices, a missing creditor — must stay null. + it('returns null when the owner is neither party', () => { + expect(documentDirection(doc(OTHER, OTHER), OWNER)).toBeNull(); + }); + + it('returns null when a party is missing or the owner is unknown', () => { + expect(documentDirection(doc(null, OWNER), null)).toBeNull(); + expect(documentDirection(doc(null, null), OWNER)).toBeNull(); + }); +}); + +describe('normalizeDocument', () => { + it('derives direction from the owning charge and nets VAT off the amount', () => { + const normalized = normalizeDocument({ + id: 'd1', + __typename: 'Invoice', + amount: { raw: 1170, formatted: '₪1170', currency: 'ILS' }, + vat: { raw: 170, formatted: '₪170', currency: 'ILS' }, + creditor: { id: 'owner-1', name: 'Acme' }, + debtor: { id: 'cust-1', name: 'Customer' }, + charge: { id: 'c1', owner: { id: 'owner-1' } }, + }); + expect(normalized.direction).toBe('issued'); + expect(normalized.amountExVat).toEqual({ + value: 1000, + formatted: '1000 ILS', + currency: 'ILS', + }); + }); + + it('treats a document with no VAT as fully net', () => { + const normalized = normalizeDocument({ + id: 'd2', + amount: { raw: -500, formatted: '₪-500', currency: 'ILS' }, + charge: { id: 'c2', owner: { id: 'owner-1' } }, + debtor: { id: 'owner-1', name: 'Acme' }, + }); + expect(normalized.direction).toBe('received'); + expect(normalized.amountExVat?.value).toBe(-500); + }); + + it('leaves amountExVat null when there is no amount', () => { + expect(normalizeDocument({ id: 'd3' }).amountExVat).toBeNull(); + }); +}); diff --git a/packages/mcp-server/src/tools/__tests__/schema-contract.test.ts b/packages/mcp-server/src/tools/__tests__/schema-contract.test.ts index ee22f94a0..10d682e33 100644 --- a/packages/mcp-server/src/tools/__tests__/schema-contract.test.ts +++ b/packages/mcp-server/src/tools/__tests__/schema-contract.test.ts @@ -2,6 +2,7 @@ import { existsSync, readFileSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; +import { KNOWN_CHARGE_TYPENAMES } from '../entity-shapes.js'; /** * Phase 1 (`Tag.ownerId`) has no server-side unit test — the tags module has no @@ -49,4 +50,20 @@ describe('generated schema contract', () => { it('TaxCategory exposes ownerId', () => { expect(typeBlock(loadSchema(), 'TaxCategory')).toMatch(/ownerId: UUID!?/); }); + + // `chargeType` / `flowKind` are derived from `__typename`, so a charge type + // added upstream would quietly classify as `unknown` — data the model reads as + // "not categorized" rather than "the connector is out of date". + it('every charge implementation has a chargeType mapping', () => { + const typenames = [...loadSchema().matchAll(/^type (\w+) implements Charge\b/gm)].map( + match => match[1]!, + ); + expect(typenames.length).toBeGreaterThan(0); + expect([...typenames].sort()).toEqual([...KNOWN_CHARGE_TYPENAMES].sort()); + }); + + // Transactions carry `amountLocal`/`exchangeRate` derived from these fields. + it('Transaction exposes eventExchangeRates', () => { + expect(loadSchema()).toMatch(/^interface Transaction \{[^}]*eventExchangeRates: ExchangeRates/m); + }); }); diff --git a/packages/mcp-server/src/tools/charge-details.ts b/packages/mcp-server/src/tools/charge-details.ts index be311ec0f..82c961dfb 100644 --- a/packages/mcp-server/src/tools/charge-details.ts +++ b/packages/mcp-server/src/tools/charge-details.ts @@ -8,10 +8,13 @@ import type { import { UpstreamError } from '../upstream/graphql-client.js'; import { TIMELESS_DATE } from './dates.js'; import { + chargeTypeFromTypename, + flowKindForCharge, normalizeAmount, normalizeDocument, normalizeEntity, normalizeTransaction, + type FlowKind, type NormalizedDocument, type NormalizedTransaction, type RawDocument, @@ -135,13 +138,19 @@ const getChargesInput = z includeTransactions: z .boolean() .optional() - .default(true) - .describe('Include each charge’s linked transactions (default true).'), + .default(false) + .describe( + 'Include each charge’s linked transactions (default false — opt in only when you need the ' + + 'individual bank/card rows, since nesting them is what forces results to be truncated).', + ), includeDocuments: z .boolean() .optional() - .default(true) - .describe('Include each charge’s linked documents (default true).'), + .default(false) + .describe( + 'Include each charge’s linked documents (default false — opt in only when you need the ' + + 'individual invoices/receipts).', + ), }) .superRefine((value, context) => { const hasIds = value.chargeIds !== undefined && value.chargeIds.length > 0; @@ -182,6 +191,20 @@ const CHARGES_QUERY_DOCUMENT = /* GraphQL */ ` id name } + eventExchangeRates { + date + aud + cad + eur + gbp + ils + jpy + sek + usd + eth + grt + usdc + } } fragment McpChargeDetailDocumentFields on Document { @@ -259,10 +282,14 @@ const CHARGES_QUERY_DOCUMENT = /* GraphQL */ ` image charge { id + owner { + id + } } } fragment McpChargeDetailFields on Charge { + __typename id userDescription owner { @@ -413,6 +440,9 @@ type RawCharge = McpGetChargesQuery['chargesByIDs'][number]; interface NormalizedCharge { id: string; description: string | null; + /** Same vocabulary as `filters.byChargeTypes`, so it can be fed back as a filter. */ + chargeType: string | null; + flowKind: FlowKind; ownerId: string | null; ownerName: string | null; counterparty: { id: string; name: string | null } | null; @@ -435,9 +465,12 @@ interface NormalizedCharge { function normalizeCharge(charge: RawCharge): NormalizedCharge { const owner = normalizeEntity(charge.owner); + const chargeType = chargeTypeFromTypename(charge.__typename); return { id: charge.id, description: charge.userDescription ?? null, + chargeType, + flowKind: flowKindForCharge(chargeType, charge.totalAmount), ownerId: owner?.id ?? null, ownerName: owner?.name ?? null, counterparty: normalizeEntity(charge.counterparty), @@ -551,7 +584,7 @@ async function handler(input: GetChargesInput, context: ToolExecutionContext): P export const getChargesTool: ToolDefinition = { name: GET_CHARGES_TOOL_NAME, description: - 'Fetch charges by id and/or by filters (all ChargeFilter fields), with full detail: owner, counterparty, amounts (total, VAT, withholding), dates, tags, metadata counts, and — by default — linked transactions and documents. Read-only. ' + + 'Fetch charges by id and/or by filters (all ChargeFilter fields), with full detail: owner, counterparty, amounts (total, VAT, withholding), dates, tags, metadata counts, `chargeType`, and `flowKind` (income / expense / internal_transfer / conversion / investment / tax / payroll / dividend / financial) — use `flowKind` to tell real income and expense apart from money moving between your own accounts. Linked transactions and documents are opt-in via `includeTransactions` / `includeDocuments`. Read-only. ' + SCOPE_DESCRIPTION_SUFFIX, inputSchema: getChargesInput, policy: { requiresBusinessScope: true, dataClassification: 'business' }, diff --git a/packages/mcp-server/src/tools/charges.ts b/packages/mcp-server/src/tools/charges.ts index e8d66b1fd..70c6f12cc 100644 --- a/packages/mcp-server/src/tools/charges.ts +++ b/packages/mcp-server/src/tools/charges.ts @@ -1,6 +1,7 @@ import { z } from 'zod'; import type { McpSearchChargesQuery, McpSearchChargesQueryVariables } from '../gql/index.js'; import { DAY_MS, parseCalendarDate, TIMELESS_DATE } from './dates.js'; +import { chargeTypeFromTypename, flowKindForCharge, type FlowKind } from './entity-shapes.js'; import { ToolInputError } from './execute.js'; import { shapeListResult } from './output.js'; import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; @@ -41,6 +42,7 @@ const SEARCH_CHARGES_QUERY = /* GraphQL */ ` query McpSearchCharges($filters: ChargeFilter, $page: Int!, $limit: Int!) { allCharges(filters: $filters, page: $page, limit: $limit) { nodes { + __typename id userDescription owner { @@ -71,6 +73,10 @@ type RawCharge = McpSearchChargesQuery['allCharges']['nodes'][number]; export interface NormalizedCharge { id: string; description: string | null; + /** Same vocabulary as `filters.byChargeTypes`, so it can be fed back as a filter. */ + chargeType: string | null; + /** What the charge does to the money — see `flowKindForCharge`. */ + flowKind: FlowKind; /** Owning business, so multi-business results can be grouped by the model. */ ownerId: string | null; ownerName: string | null; @@ -150,9 +156,12 @@ function buildFilters( } function normalizeCharge(charge: RawCharge): NormalizedCharge { + const chargeType = chargeTypeFromTypename(charge.__typename); return { id: charge.id, description: charge.userDescription, + chargeType, + flowKind: flowKindForCharge(chargeType, charge.totalAmount), // Optional chaining: fixtures predating owner selection omit the field. ownerId: charge.owner?.id ?? null, ownerName: charge.owner?.name ?? null, @@ -217,7 +226,7 @@ async function handler( export const searchChargesTool: ToolDefinition = { name: SEARCH_CHARGES_TOOL_NAME, description: - 'Search and browse accounting charges within your authorized businesses. Supports date range, tag, free-text, and income/expense filters with bounded pagination. Read-only. ' + + 'Search and browse accounting charges within your authorized businesses. Supports date range, tag, free-text, and income/expense filters with bounded pagination. Each row carries `chargeType` and `flowKind` (income / expense / internal_transfer / conversion / investment / tax / payroll / dividend / financial), so money moving between your own accounts can be excluded without inspecting descriptions. Read-only. ' + SCOPE_DESCRIPTION_SUFFIX, inputSchema: searchChargesInput, policy: { requiresBusinessScope: true, dataClassification: 'business' }, diff --git a/packages/mcp-server/src/tools/document-details.ts b/packages/mcp-server/src/tools/document-details.ts index 475734580..49e682c2a 100644 --- a/packages/mcp-server/src/tools/document-details.ts +++ b/packages/mcp-server/src/tools/document-details.ts @@ -309,7 +309,7 @@ async function handler( export const getDocumentsTool: ToolDefinition = { name: GET_DOCUMENTS_TOOL_NAME, description: - 'Fetch documents (invoices, receipts, credit invoices, …) either by id or by filters (owners, charge ids, date range, type, unmatched/missing-info flags, and free-text), with type, serial number, date, amount, VAT, creditor/debtor, and file/image links. Read-only. ' + + 'Fetch documents (invoices, receipts, credit invoices, …) either by id or by filters (owners, charge ids, date range, type, unmatched/missing-info flags, and free-text), with type, serial number, date, amount, `amountExVat`, VAT, creditor/debtor, and file/image links. Each row carries `direction`: `issued` means the owning business raised it (revenue), `received` means it was billed. `direction` is null when the owner is neither party — credit invoices and documents with a missing creditor — so treat null as "undetermined" rather than assuming either way. Read-only. ' + SCOPE_DESCRIPTION_SUFFIX, inputSchema: getDocumentsInput, policy: { requiresBusinessScope: true, dataClassification: 'business' }, diff --git a/packages/mcp-server/src/tools/entity-shapes.ts b/packages/mcp-server/src/tools/entity-shapes.ts index 23c82e00d..1ca91d0ff 100644 --- a/packages/mcp-server/src/tools/entity-shapes.ts +++ b/packages/mcp-server/src/tools/entity-shapes.ts @@ -33,6 +33,72 @@ export function normalizeAmount(amount: RawAmount | null | undefined): Normalize : null; } +// --------------------------------------------------------------------------- +// Currency conversion +// --------------------------------------------------------------------------- + +/** + * `ExchangeRates` as returned by `Transaction.eventExchangeRates`: one nullable + * float per supported currency, plus the date the rates are quoted for. + */ +export interface RawExchangeRates { + date?: string | null; + aud?: number | null; + cad?: number | null; + eur?: number | null; + gbp?: number | null; + ils?: number | null; + jpy?: number | null; + sek?: number | null; + usd?: number | null; + eth?: number | null; + grt?: number | null; + usdc?: number | null; +} + +/** + * The bookkeeping currency every rate is quoted against. + * + * Upstream resolves the fiat fields straight from the Bank of Israel rates table + * (`getFiatExchangeRate`) and hardcodes `ils: () => 1`, so each field is + * "ILS per one unit of that currency" — the multiplier, not its reciprocal. + * (The crypto fields are quoted against the admin context's configurable + * `defaultLocalCurrency`; for this deployment that is also ILS.) + */ +export const LOCAL_CURRENCY = 'ILS'; + +/** Money converted to {@link LOCAL_CURRENCY}, with the historical rate used. */ +export interface LocalAmount { + amountLocal: { value: number; currency: string }; + exchangeRate: number; +} + +/** + * Convert an amount to the local bookkeeping currency using the transaction's + * own historical rates, so callers stop applying present-day rates to years of + * history. + * + * Returns `null` when the rate for that currency is absent (an unsupported + * currency, or a date the rates table does not cover) — never a guess, and + * never a silent fallback to the origin-currency number. + */ +export function toLocalAmount( + amount: RawAmount | null | undefined, + rates: RawExchangeRates | null | undefined, +): LocalAmount | null { + if (!amount || !rates) { + return null; + } + const rate = rates[amount.currency.toLowerCase() as keyof RawExchangeRates]; + if (typeof rate !== 'number' || !Number.isFinite(rate)) { + return null; + } + return { + amountLocal: { value: amount.raw * rate, currency: LOCAL_CURRENCY }, + exchangeRate: rate, + }; +} + /** A referenced financial entity (owner, counterparty, creditor, debtor). */ export interface RawEntityRef { id: string; @@ -64,6 +130,7 @@ export interface RawTransaction { isFee?: boolean | null; counterparty?: RawEntityRef | null; account?: { id: string; name: string } | null; + eventExchangeRates?: RawExchangeRates | null; } export interface NormalizedTransaction { @@ -72,6 +139,10 @@ export interface NormalizedTransaction { type: string | null; direction: string; amount: NormalizedAmount | null; + /** {@link amount} converted to {@link LOCAL_CURRENCY} at the event-date rate. */ + amountLocal: LocalAmount['amountLocal'] | null; + /** The historical rate behind `amountLocal`; `null` when unavailable. */ + exchangeRate: number | null; eventDate: string; effectiveDate: string | null; description: string; @@ -81,12 +152,18 @@ export interface NormalizedTransaction { } export function normalizeTransaction(transaction: RawTransaction): NormalizedTransaction { + const local = toLocalAmount(transaction.amount, transaction.eventExchangeRates); return { id: transaction.id, chargeId: transaction.chargeId, type: transaction.__typename ?? null, direction: transaction.direction, amount: normalizeAmount(transaction.amount), + // Only the derived pair is emitted — the raw `ExchangeRates` object is a + // dozen floats per row, nearly all of them for currencies the row does not + // use, and payload budget is the scarce resource here. + amountLocal: local?.amountLocal ?? null, + exchangeRate: local?.exchangeRate ?? null, eventDate: transaction.eventDate, effectiveDate: transaction.effectiveDate ?? null, description: transaction.sourceDescription, @@ -98,6 +175,101 @@ export function normalizeTransaction(transaction: RawTransaction): NormalizedTra }; } +// --------------------------------------------------------------------------- +// Charge classification +// --------------------------------------------------------------------------- + +/** + * Map a charge's GraphQL `__typename` to a stable `chargeType` token. + * + * The tokens are deliberately the *same* vocabulary as the upstream + * `ChargeFilter.byChargeTypes` enum, so a `chargeType` read off a result row can + * be handed straight back as a filter without translation. + */ +const CHARGE_TYPE_BY_TYPENAME: Record = { + CommonCharge: 'COMMON', + ConversionCharge: 'CONVERSION', + SalaryCharge: 'PAYROLL', + InternalTransferCharge: 'INTERNAL', + DividendCharge: 'DIVIDEND', + BusinessTripCharge: 'BUSINESS_TRIP', + MonthlyVatCharge: 'VAT', + BankDepositCharge: 'BANK_DEPOSIT', + ForeignSecuritiesCharge: 'FOREIGN_SECURITIES', + CreditcardBankCharge: 'CREDITCARD_BANK', + FinancialCharge: 'FINANCIAL', +}; + +/** + * Every charge `__typename` this module knows how to classify. Exported so the + * schema-contract suite can fail loudly when upstream adds a charge type — the + * runtime behaviour otherwise degrades silently to `chargeType: null` / + * `flowKind: 'unknown'`, which looks like missing data rather than a stale map. + */ +export const KNOWN_CHARGE_TYPENAMES = Object.keys(CHARGE_TYPE_BY_TYPENAME); + +export function chargeTypeFromTypename(typename: string | null | undefined): string | null { + return typename ? (CHARGE_TYPE_BY_TYPENAME[typename] ?? null) : null; +} + +/** + * What a charge *does* to the business's money, as opposed to what kind of + * record it is. + * + * This exists so "what changed my total?" is answerable by filtering rather + * than by regexing Hebrew bank descriptions. The critical distinction is + * `internal_transfer`: deposit sweeps and credit-card settlements move money + * between the owner's own accounts, so counting them as income/expense + * double-counts — which is exactly the trap the connector feedback hit. + */ +export type FlowKind = + | 'income' + | 'expense' + | 'internal_transfer' + | 'conversion' + | 'investment' + | 'tax' + | 'payroll' + | 'dividend' + | 'financial' + | 'unknown'; + +const FLOW_KIND_BY_CHARGE_TYPE: Record = { + CONVERSION: 'conversion', + // Both legs stay inside the owner's own accounts. + INTERNAL: 'internal_transfer', + BANK_DEPOSIT: 'internal_transfer', + CREDITCARD_BANK: 'internal_transfer', + FOREIGN_SECURITIES: 'investment', + PAYROLL: 'payroll', + VAT: 'tax', + DIVIDEND: 'dividend', + FINANCIAL: 'financial', + BUSINESS_TRIP: 'expense', +}; + +/** + * Derive the flow kind. `COMMON` — the catch-all charge type — carries no + * intrinsic direction, so it falls back to the sign of the total amount, and + * stays `unknown` when there is no amount to read rather than guessing. + */ +export function flowKindForCharge( + chargeType: string | null, + totalAmount: RawAmount | null | undefined, +): FlowKind { + if (chargeType === null) { + return 'unknown'; + } + const mapped = FLOW_KIND_BY_CHARGE_TYPE[chargeType]; + if (mapped) { + return mapped; + } + if (typeof totalAmount?.raw !== 'number' || totalAmount.raw === 0) { + return 'unknown'; + } + return totalAmount.raw > 0 ? 'income' : 'expense'; +} + // --------------------------------------------------------------------------- // Documents // --------------------------------------------------------------------------- @@ -118,6 +290,13 @@ export interface RawDocument { charge?: { id: string; owner?: { id: string } | null } | null; } +/** + * Which way the money flows relative to the owning business: `issued` is a + * document the business raised (revenue), `received` is one it was billed. + * `null` when neither party is the owner — see {@link documentDirection}. + */ +export type DocumentDirection = 'issued' | 'received'; + export interface NormalizedDocument { id: string; type: string | null; @@ -125,7 +304,10 @@ export interface NormalizedDocument { serialNumber: string | null; date: string | null; amount: NormalizedAmount | null; + /** `amount` net of VAT; `null` when there is no amount to net. */ + amountExVat: NormalizedAmount | null; vat: NormalizedAmount | null; + direction: DocumentDirection | null; description: string | null; creditor: EntityRef | null; debtor: EntityRef | null; @@ -134,6 +316,44 @@ export interface NormalizedDocument { imageUrl: string | null; } +/** + * Determine direction by matching the document's parties against the owning + * business, replacing the creditor-name / URL / serial-range heuristics the + * feedback describes. + * + * Returns `null` rather than guessing when the owner is unknown or appears as + * neither party — the known edge cases are credit invoices and documents with a + * missing creditor, and a wrong direction there silently flips an expense into + * revenue. + */ +export function documentDirection( + document: RawDocument, + ownerId: string | null | undefined, +): DocumentDirection | null { + if (!ownerId) { + return null; + } + if (document.creditor?.id === ownerId) { + return 'issued'; + } + if (document.debtor?.id === ownerId) { + return 'received'; + } + return null; +} + +/** VAT-exclusive amount. Both sides share a currency, so no conversion applies. */ +function amountExcludingVat( + amount: RawAmount | null | undefined, + vat: RawAmount | null | undefined, +): NormalizedAmount | null { + if (!amount) { + return null; + } + const net = amount.raw - (vat?.raw ?? 0); + return { value: net, formatted: `${net} ${amount.currency}`, currency: amount.currency }; +} + export function normalizeDocument(document: RawDocument): NormalizedDocument { return { id: document.id, @@ -142,7 +362,9 @@ export function normalizeDocument(document: RawDocument): NormalizedDocument { serialNumber: document.serialNumber ?? null, date: document.date ?? null, amount: normalizeAmount(document.amount), + amountExVat: amountExcludingVat(document.amount, document.vat), vat: normalizeAmount(document.vat), + direction: documentDirection(document, document.charge?.owner?.id), description: document.description ?? null, creditor: normalizeEntity(document.creditor), debtor: normalizeEntity(document.debtor), diff --git a/packages/mcp-server/src/tools/transaction-details.ts b/packages/mcp-server/src/tools/transaction-details.ts index 842a267e3..eeced9f71 100644 --- a/packages/mcp-server/src/tools/transaction-details.ts +++ b/packages/mcp-server/src/tools/transaction-details.ts @@ -138,6 +138,20 @@ const TRANSACTIONS_QUERY_DOCUMENT = /* GraphQL */ ` id name } + eventExchangeRates { + date + aud + cad + eur + gbp + ils + jpy + sek + usd + eth + grt + usdc + } } query McpGetTransactions($transactionIDs: [UUID!]!) { @@ -281,7 +295,7 @@ async function handler( export const getTransactionsTool: ToolDefinition = { name: GET_TRANSACTIONS_TOOL_NAME, description: - 'Fetch bank/card transactions either by id or by filters (owners, charge ids, date ranges, counterparties, missing-info flags, and free-text), with amount, dates, direction, counterparty, and account. Read-only. ' + + 'Fetch bank/card transactions either by id or by filters (owners, charge ids, date ranges, counterparties, missing-info flags, and free-text), with amount, dates, direction, counterparty, and account. Each row also carries `amountLocal` (the amount in ILS) and `exchangeRate` — the historical rate for that transaction’s own event date, so never apply present-day rates to past rows. Both are null when no rate is on file. Read-only. ' + SCOPE_DESCRIPTION_SUFFIX, inputSchema: getTransactionsInput, policy: { requiresBusinessScope: true, dataClassification: 'business' }, From 1a11f819468a72f17053df10e88c84fb9a736311 Mon Sep 17 00:00:00 2001 From: Gil Gardosh Date: Wed, 5 Aug 2026 15:40:44 +0300 Subject: [PATCH 4/4] =?UTF-8?q?Phase=202=20=E2=80=94=20Report=20tools=20(w?= =?UTF-8?q?rapping=20existing=20upstream=20queries)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/mcp-extension/plan.md | 6 +- packages/mcp-server/README.md | 83 ++- packages/mcp-server/docs/todo.md | 35 +- .../tools/__tests__/financial-reports.test.ts | 553 ++++++++++++++++ .../tools/__tests__/registry-instance.test.ts | 33 + .../tools/__tests__/scope-forwarding.test.ts | 59 +- packages/mcp-server/src/tools/accounts.ts | 160 +++++ .../mcp-server/src/tools/financial-reports.ts | 611 ++++++++++++++++++ .../mcp-server/src/tools/ledger-reports.ts | 367 +++++++++++ .../mcp-server/src/tools/registry-instance.ts | 20 + packages/mcp-server/src/tools/reports.ts | 29 +- packages/mcp-server/src/tools/scope-input.ts | 41 ++ 12 files changed, 1948 insertions(+), 49 deletions(-) create mode 100644 packages/mcp-server/src/tools/__tests__/financial-reports.test.ts create mode 100644 packages/mcp-server/src/tools/accounts.ts create mode 100644 packages/mcp-server/src/tools/financial-reports.ts create mode 100644 packages/mcp-server/src/tools/ledger-reports.ts diff --git a/docs/mcp-extension/plan.md b/docs/mcp-extension/plan.md index d38772e7d..8dd9d1b1d 100644 --- a/docs/mcp-extension/plan.md +++ b/docs/mcp-extension/plan.md @@ -123,7 +123,11 @@ explicitly. --- -## Phase 2 — Report tools (wrapping existing upstream queries) +## Phase 2 — Report tools (wrapping existing upstream queries) — ✅ DONE + +_Implemented in `src/tools/accounts.ts`, `financial-reports.ts` and `ledger-reports.ts`, registered +ahead of the row-level tools. 470 tests pass; codegen, lint, typecheck and build clean. The deferred +server-side items are now recorded as **I6** in `packages/mcp-server/docs/todo.md`._ New file `packages/mcp-server/src/tools/financial-reports.ts`, registered in `registry-instance.ts`. Every tool follows the established `balanceReportTool` pattern in `reports.ts`: required singular diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index 7c34295d5..5277eddb6 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -18,16 +18,18 @@ 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 nine read-only tools -(`accounter_list_business_memberships`, `accounter_search_charges`, `accounter_get_charges`, -`accounter_get_transactions`, `accounter_get_documents`, `accounter_list_tags`, -`accounter_list_tax_categories`, `accounter_list_businesses`, `accounter_balance_report`) each gated -by strict input validation, a per-tool authorization policy, and business-scope narrowing forwarded -upstream as `x-business-scope`; a hardened upstream GraphQL client (timeout, bounded retries, header -propagation, sanitized errors); a unified error taxonomy; per-`tools/call` rate limiting; in-process -operational metrics (request/outcome counters, a latency histogram, auth-failure counters) exposed -at `GET /metrics`; and OpenTelemetry tracing exported to Grafana Tempo (opt-in), correlated with the -backend via `traceparent` and `X-Correlation-Id`. +Accounter GraphQL server; a curated registry of fifteen read-only tools +(`accounter_list_business_memberships`, `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`, +`accounter_list_tags`, `accounter_list_tax_categories`, `accounter_list_businesses`, +`accounter_balance_report`) each gated by strict input validation, a per-tool authorization policy, +and business-scope narrowing forwarded upstream as `x-business-scope`; a hardened upstream GraphQL +client (timeout, bounded retries, header propagation, sanitized errors); a unified error taxonomy; +per-`tools/call` rate limiting; in-process operational metrics (request/outcome counters, a latency +histogram, auth-failure counters) exposed at `GET /metrics`; and OpenTelemetry tracing exported to +Grafana Tempo (opt-in), correlated with the backend via `traceparent` and `X-Correlation-Id`. Phase 2 (write scope) is **not** implemented — see [Known limitations & phase 2](#known-limitations--phase-2-write-scope). @@ -57,8 +59,8 @@ a `RATE_LIMIT_ERROR` with `retryAfterMs`. Limits are configured via `MCP_RATE_LI Every business-scoped tool follows one convention, so the model learns it once: - **Discover, then scope.** `accounter_list_business_memberships` returns - `{ businessId, name, role }`. Pass those ids back as `businessIds` (or, for the balance report, - the singular required `businessId`). + `{ businessId, name, role }`. Pass those ids back as `businessIds` (or, for the single-business + report tools, the singular required `businessId`). - **`businessIds` is optional and means "narrow".** Omitting it covers every business the caller belongs to. Any id outside the caller's memberships is **rejected**, never silently dropped. - **The resolved scope is forwarded upstream** as `x-business-scope`, so RLS on the Accounter server @@ -76,26 +78,63 @@ 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`. + +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 +five are single-business (required `businessId`) and require the `business_owner`/`accountant` role. + +- **`accounter_list_accounts`** — one business's financial accounts: id, name, number, `type` + (`BANK_ACCOUNT`, `BANK_DEPOSIT_ACCOUNT`, `CREDIT_CARD`, `CRYPTO_WALLET`, `FOREIGN_SECURITIES`), + currencies, and bank/card identifiers. `type` is what makes transaction data interpretable — + credit-card rows are settled again by a matching bank row, deposits receive checking-account + sweeps, and securities rows have no mirror bank leg. **Balances are deliberately not reported:** + upstream stores a placeholder `0` for card, deposit and SWIFT rows in a non-null column, so a + reported balance would be wrong more often than right. +- **`accounter_income_expense_summary`** — monthly income/expense totals over a date range (≤ 3660 + days, so a decade fits one call), converted to a single `currency` using historical rates, plus + period totals. `cumulativeNet` is a running sum of the period's own flows from zero — **not** an + account balance, and it counts card rows alongside the bank rows that settle them. +- **`accounter_profit_and_loss`** — the ledger-computed P&L for one calendar year: revenue, cost of + sales, gross profit, R&D/marketing/G&A, operating profit, financial expenses, other income, profit + before tax, tax, net profit. Optional `referenceYears` (≤ 5) for year-over-year. Line items are + flattened to totals; the per-sort-code breakdown is dropped. +- **`accounter_vat_report`** — monthly VAT: output VAT on income, input VAT on expenses, and net VAT + due (positive) or refundable (negative), in local currency. Per-document `records` are opt-in via + `includeRecords`; counts are always reported. +- **`accounter_counterparty_totals`** — per-counterparty credit/debit/net totals from the + double-entry ledger, ordered by absolute total, with a per-currency breakdown. This is the direct + answer to "who are our biggest customers/suppliers". Revaluation entries are excluded by default. +- **`accounter_ledger_records`** — individual ledger records (date, counterparty, counter account, + local and foreign amounts, `chargeId`, reference) over a bounded range (≤ 1096 days). The ledger, + not the raw bank feed, is authoritative about what a movement was. Both ledger tools surface an + upstream `CommonError` as a tool error rather than an empty list, which would misread as "no + activity". + - **`accounter_search_charges`** — read-only charges search/browse within the caller's authorized businesses. Optional `businessIds` (subset of memberships), `fromDate`/`toDate` (bounded to 366 days), `tags`, `freeText`, and `flow` (`ALL`/`INCOME`/`EXPENSE`), with bounded pagination - (`pageSize` ≤ 50). Returns normalized charges — each carrying `ownerId`/`ownerName` — plus - pagination metadata and the echoed `scope`. Scoping uses the `byOwners` predicate upstream (the - owner), never `byBusinesses` (the counterparty). + (`pageSize` ≤ 50). Returns normalized charges — each carrying `ownerId`/`ownerName`, plus + `chargeType` and `flowKind` — pagination metadata, and the echoed `scope`. Scoping uses the + `byOwners` predicate upstream (the owner), never `byBusinesses` (the counterparty). - **`accounter_get_charges`** — read-only charge **detail** by id (1–25 `chargeIds`). Returns each charge with owner, counterparty, amounts (total, VAT, withholding), the full set of dates, tags, - and `metadata` counts, plus — by default — its linked `transactions` and `documents` nested inline - (toggle with `includeTransactions` / `includeDocuments`). This is the drill-down for + `metadata` counts, `chargeType`, and `flowKind`. Linked `transactions` and `documents` are + **opt-in** via `includeTransactions` / `includeDocuments` (both default `false` — nesting them by + default is what forced nearly every response over the byte budget). This is the drill-down for `accounter_search_charges`. A charge whose `owner` falls outside the resolved scope is dropped as defense-in-depth on top of RLS. - **`accounter_get_transactions`** — read-only bank/card **transactions** by id (1–50 `transactionIds`). Each row carries direction, amount, event/effective dates, source description, - `isFee`, `chargeId`, counterparty, and account. Scope is enforced upstream by RLS (transactions - carry no owner field for a client-side filter). + `isFee`, `chargeId`, counterparty, and account, plus `amountLocal` and `exchangeRate` — the amount + converted to ILS at that transaction's own event-date rate, so historical rows are never valued at + today's rates. Both are `null` when no rate is on file. Scope is enforced upstream by RLS + (transactions carry no owner field for a client-side filter). - **`accounter_get_documents`** — read-only **documents** by id (1–50 `documentIds`). Each row - carries `documentType`, serial number, date, amount, VAT, creditor/debtor, `chargeId`, and - `file`/`image` links. A document whose owning charge falls outside the resolved scope is dropped - as defense-in-depth on top of RLS. + carries `documentType`, serial number, date, amount, `amountExVat`, VAT, creditor/debtor, + `chargeId`, `file`/`image` links, and `direction` (`issued` = the business raised it, `received` = + it was billed). `direction` is `null` when the owner is neither party — credit invoices and + documents with a missing creditor — rather than guessed. A document whose owning charge falls + outside the resolved scope is dropped as defense-in-depth on top of RLS. - **`accounter_list_tags`** — list tags for categorizing charges, optionally filtered by name and by `businessIds`. Rows carry `ownerId`. Deterministically sorted (name, then id) and size-capped (≤ 500). diff --git a/packages/mcp-server/docs/todo.md b/packages/mcp-server/docs/todo.md index db28cba84..fc61da878 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, five curated tools, business scope enforced by RLS upstream. Not +Auth0 client, read-only, fifteen curated tools, business scope enforced by RLS upstream. Not publishable — see B1. --- @@ -56,6 +56,39 @@ D3 (`MCP_TOOL_ALLOWLIST`) and D6 (rate-limit keying) are settled — see the clo Ranked by when they start to hurt, not by size. +### I6. Server-side gaps behind the connector-UX work — **high** + +Raised by the agent-session feedback (`accounter_mcp_feedback.md`) and scoped out of the MCP-only +Phases 1–2 (`docs/mcp-extension/plan.md`). These need `packages/server` / `packages/migrations` +changes. + +- [ ] **`transactions.current_balance` is a placeholder for most sources — the blocker for + `balanceAfter`.** `resolvers/common.ts:57` passes the column straight through, but only the + Poalim ILS/foreign and Discount triggers write a real value; the SWIFT, deposit, and all four + credit-card triggers insert a literal `0`. Since `Transaction.balance` is non-null, a + placeholder is indistinguishable from a genuine zero balance, and `Transaction` exposes no + `sourceOrigin` for the MCP layer to filter on — so per-transaction balances and + `accounter_list_accounts`' `includeCurrentBalance` both stay unshipped. Fix: write `NULL` + instead of `0`, make the GraphQL field nullable, return `null` from the resolver, and backfill + keyed off `source_origin` / account type (**not** off the value — a real `0` is legitimate). + Longer term, capture the balance the card and deposit sources do expose. +- [ ] **No sorting or pagination on `transactionsByFilters`.** `TransactionsFilters` has no `sortBy` + and the query returns an unbounded `[Transaction!]!`, which is why the feedback session + hand-rolled cursor pagination over ~100 calls. Wants `sortBy` + `limit`/`offset` (or a cursor) + and, ideally, a bulk export for the "fetch once, work locally" pattern. +- [ ] **`Charge.totalAmount` reported `null` on every income charge**, which is why revenue had to + be computed from documents. Verify against `packages/server/src/modules/charges`; compute + server-side if it is derivable. +- [ ] **`myMemberships.businessName` is nullable** and came back `null`, so the agent could not tell + which business was which without fetching a charge. Fix the resolver's join, or fall back + through `businesses(ids:)` in `src/upstream/memberships.ts`. +- [ ] **Not yet exposed, and asked for:** document → payment status (open/partially paid/paid with + matched transaction ids) for invoiced-vs-collected, securities positions (holdings, cost + basis, market value, realized/unrealized P&L), and a payroll breakdown. `allDeposits` already + exists — a thin wrapper is cheap if deposits become a priority. + +Source: `accounter_mcp_feedback.md` §§1–3, 6–7. + ### I5. Stable tunnel for local development — **low (friction)** Production is settled (see the closed list — `https://mcp.accounter.tax`). Local development still diff --git a/packages/mcp-server/src/tools/__tests__/financial-reports.test.ts b/packages/mcp-server/src/tools/__tests__/financial-reports.test.ts new file mode 100644 index 000000000..bcd517372 --- /dev/null +++ b/packages/mcp-server/src/tools/__tests__/financial-reports.test.ts @@ -0,0 +1,553 @@ +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 { listAccountsTool } from '../accounts.js'; +import { executeRegisteredTool } from '../execute.js'; +import { + incomeExpenseSummaryTool, + MAX_AGGREGATE_RANGE_DAYS, + profitAndLossTool, + vatReportTool, +} from '../financial-reports.js'; +import { counterpartyTotalsTool, ledgerRecordsTool } from '../ledger-reports.js'; +import type { ToolDefinition } from '../registry.js'; + +/** + * Unit coverage for the Phase 2 report tools. Mirrors the harness in + * `detail-tools.test.ts`: a fake upstream returns fixtures and the executor + * drives validation + policy + handler. + */ + +const B1 = 'aa000000-0000-4000-8000-000000000001'; +const B2 = 'aa000000-0000-4000-8000-000000000002'; + +function authContext(businessIds: string[], roleId = 'business_owner'): McpAuthContext { + const principal: AuthPrincipal = { + subject: 'user-1', + issuer: 'https://tenant.auth0.com/', + audience: 'aud', + scopes: [], + email: null, + expiresAt: undefined, + claims: { sub: 'user-1' }, + }; + return buildAuthContext( + principal, + businessIds.map(businessId => ({ businessId, roleId })), + ); +} + +function clientReturning(data: unknown, capture?: (body: unknown) => void) { + const fetchImpl = vi.fn(async (_url: string, init: RequestInit) => { + capture?.(JSON.parse(init.body as string)); + return { ok: true, status: 200, json: async () => ({ data }) } as unknown as Response; + }); + return new UpstreamGraphQLClient({ + endpoint: 'http://localhost:4000/graphql', + timeoutMs: 1000, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); +} + +function run( + tool: ToolDefinition, + client: UpstreamGraphQLClient, + auth: McpAuthContext, + rawArgs: unknown, +) { + return executeRegisteredTool({ + tool, + rawArgs, + auth, + correlationId: 'corr-1', + client, + authorization: 'Bearer tok', + }); +} + +const ils = (raw: number) => ({ raw, formatted: `₪${raw}`, currency: 'ILS' }); + +// --------------------------------------------------------------------------- +// list_accounts +// --------------------------------------------------------------------------- + +describe('listAccountsTool', () => { + const fixture = { + financialAccountsByOwner: [ + { + __typename: 'BankFinancialAccount', + id: 'acc1', + name: 'Poalim ILS', + number: '12345', + type: 'BANK_ACCOUNT', + privateOrBusiness: 'BUSINESS', + accountTaxCategories: [{ currency: 'ILS' }, { currency: 'USD' }, { currency: 'ILS' }], + bankNumber: 12, + branchNumber: 345, + iban: 'IL123', + swiftCode: 'POALILIT', + }, + { + __typename: 'CardFinancialAccount', + id: 'acc2', + name: 'Isracard', + number: '9999', + type: 'CREDIT_CARD', + privateOrBusiness: 'BUSINESS', + accountTaxCategories: [], + fourDigits: '9999', + }, + ], + }; + + it('normalizes accounts with their type-specific identifiers', async () => { + const client = clientReturning(fixture); + const result = await run(listAccountsTool, client, authContext([B1]), { businessId: B1 }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + accounts: Array>; + countsByType: Record; + }; + expect(structured.accounts[0]).toMatchObject({ + id: 'acc1', + type: 'BANK_ACCOUNT', + // De-duplicated and sorted. + currencies: ['ILS', 'USD'], + bank: { bankNumber: 12, branchNumber: 345, iban: 'IL123' }, + cardLastFour: null, + }); + expect(structured.accounts[1]).toMatchObject({ + id: 'acc2', + type: 'CREDIT_CARD', + bank: null, + cardLastFour: '9999', + }); + expect(structured.countsByType).toEqual({ BANK_ACCOUNT: 1, CREDIT_CARD: 1 }); + }); + + // Balances are deliberately absent: upstream stores a placeholder 0 for cards, + // deposits and SWIFT rows, so any balance we reported would be wrong more + // often than right. + it('never reports a balance', async () => { + const client = clientReturning(fixture); + const result = await run(listAccountsTool, client, authContext([B1]), { businessId: B1 }); + const { accounts } = result.structuredContent as { accounts: Array> }; + for (const account of accounts) { + expect(account).not.toHaveProperty('balance'); + expect(account).not.toHaveProperty('currentBalance'); + } + }); + + it('refuses a business outside the caller’s memberships', async () => { + const client = clientReturning(fixture); + const result = await run(listAccountsTool, client, authContext([B2]), { businessId: B1 }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('AUTHORIZATION_ERROR'); + }); +}); + +// --------------------------------------------------------------------------- +// income_expense_summary +// --------------------------------------------------------------------------- + +describe('incomeExpenseSummaryTool', () => { + const fixture = { + incomeExpenseChart: { + currency: 'USD', + fromDate: '2026-01-01', + toDate: '2026-02-28', + monthlyData: [ + { date: '2026-01-01', income: ils(1000), expense: ils(400), balance: ils(600) }, + { date: '2026-02-01', income: ils(500), expense: ils(100), balance: ils(1000) }, + ], + }, + }; + + it('returns per-month rows and period totals', async () => { + const client = clientReturning(fixture); + const result = await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2026-01-01', + toDate: '2026-02-28', + }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + months: Array<{ month: string; cumulativeNet: { value: number } }>; + totals: { income: number; expense: number; net: number; currency: string }; + }; + expect(structured.months).toHaveLength(2); + expect(structured.months[0]!.month).toBe('2026-01-01'); + expect(structured.totals).toMatchObject({ income: 1500, expense: 500, net: 1000 }); + expect(structured.totals.currency).toBe('USD'); + }); + + // Upstream calls this `balance`; renaming it is the point — it is a running + // sum of the period's flows, not an account balance. + it('renames the running sum to cumulativeNet', async () => { + const client = clientReturning(fixture); + const result = await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2026-01-01', + toDate: '2026-02-28', + }); + const { months } = result.structuredContent as { months: Array> }; + expect(months[0]).toHaveProperty('cumulativeNet'); + expect(months[0]).not.toHaveProperty('balance'); + }); + + it('forwards the requested currency to upstream', async () => { + let sentBody: unknown; + const client = clientReturning(fixture, body => (sentBody = body)); + await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2026-01-01', + toDate: '2026-02-28', + currency: 'USD', + }); + const { filters } = (sentBody as { variables: { filters: Record } }).variables; + expect(filters).toEqual({ fromDate: '2026-01-01', toDate: '2026-02-28', currency: 'USD' }); + }); + + it('omits currency entirely when not requested', async () => { + let sentBody: unknown; + const client = clientReturning(fixture, body => (sentBody = body)); + await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2026-01-01', + toDate: '2026-02-28', + }); + const { filters } = (sentBody as { variables: { filters: Record } }).variables; + expect('currency' in filters).toBe(false); + }); + + // A decade in one call is the "chart my money from the beginning" case. + it('accepts a multi-year range but rejects one beyond the cap', async () => { + const client = clientReturning(fixture); + const ok = await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2019-01-01', + toDate: '2026-01-01', + }); + expect(ok.isError).toBeUndefined(); + + const tooWide = await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2000-01-01', + toDate: '2026-01-01', + }); + expect(tooWide.isError).toBe(true); + expect((tooWide.structuredContent as { message: string }).message).toContain( + String(MAX_AGGREGATE_RANGE_DAYS), + ); + }); + + it('rejects an inverted range', async () => { + const client = clientReturning(fixture); + const result = await run(incomeExpenseSummaryTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2026-03-01', + toDate: '2026-01-01', + }); + expect(result.isError).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// profit_and_loss +// --------------------------------------------------------------------------- + +describe('profitAndLossTool', () => { + const year = (y: number) => ({ + year: y, + revenue: { amount: ils(1_000_000) }, + costOfSales: { amount: ils(-200_000) }, + grossProfit: ils(800_000), + researchAndDevelopmentExpenses: { amount: ils(-300_000) }, + marketingExpenses: { amount: ils(-50_000) }, + managementAndGeneralExpenses: { amount: ils(-100_000) }, + operatingProfit: ils(350_000), + financialExpenses: { amount: ils(-10_000) }, + otherIncome: { amount: ils(5_000) }, + profitBeforeTax: ils(345_000), + tax: ils(-79_350), + netProfit: ils(265_650), + }); + + const fixture = { + profitAndLossReport: { report: year(2026), reference: [year(2025)] }, + }; + + it('flattens each line item to its total and keeps reference years', async () => { + const client = clientReturning(fixture); + const result = await run(profitAndLossTool, client, authContext([B1]), { + businessId: B1, + year: 2026, + referenceYears: [2025], + }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + years: Array<{ year: number; revenue: { value: number }; netProfit: { value: number } }>; + reportYear: number; + }; + expect(structured.reportYear).toBe(2026); + // Report year first, then references. + expect(structured.years.map(y => y.year)).toEqual([2026, 2025]); + expect(structured.years[0]!.revenue.value).toBe(1_000_000); + expect(structured.years[0]!.netProfit.value).toBe(265_650); + // The per-sort-code `records` breakdown is intentionally dropped. + expect(structured.years[0]).not.toHaveProperty('records'); + }); + + it('rejects a report year that also appears in referenceYears', async () => { + const client = clientReturning(fixture); + const result = await run(profitAndLossTool, client, authContext([B1]), { + businessId: B1, + year: 2026, + referenceYears: [2026], + }); + expect(result.isError).toBe(true); + }); + + it('defaults referenceYears to empty', async () => { + let sentBody: unknown; + const client = clientReturning( + { profitAndLossReport: { report: year(2026), reference: [] } }, + body => (sentBody = body), + ); + await run(profitAndLossTool, client, authContext([B1]), { businessId: B1, year: 2026 }); + const { variables } = sentBody as { variables: { referenceYears: number[] } }; + expect(variables.referenceYears).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// vat_report +// --------------------------------------------------------------------------- + +describe('vatReportTool', () => { + const record = (vat: number, amount: number) => ({ + chargeId: 'c1', + documentId: 'd1', + documentSerial: 'INV-1', + documentDate: '2026-01-10', + chargeDate: '2026-01-10', + business: { id: 'cp1', name: 'Acme' }, + amount: ils(amount), + localAmount: ils(amount), + localVat: ils(vat), + localVatAfterDeduction: ils(vat), + vatNumber: '123', + isProperty: false, + }); + + const fixture = { + vatReport: { + income: [record(170, 1170), record(340, 2340)], + expenses: [record(85, 585)], + }, + }; + + it('reports net VAT due from output minus input VAT', async () => { + const client = clientReturning(fixture); + const result = await run(vatReportTool, client, authContext([B1]), { + businessId: B1, + month: '2026-01-01', + }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + totals: { income: { vat: number }; expenses: { vat: number }; netVatDue: number }; + recordCounts: { income: number; expenses: number }; + records: unknown[]; + }; + expect(structured.totals.income.vat).toBe(510); + expect(structured.totals.expenses.vat).toBe(85); + expect(structured.totals.netVatDue).toBe(425); + expect(structured.recordCounts).toEqual({ income: 2, expenses: 1 }); + // Records are opt-in; the counts are reported either way. + expect(structured.records).toEqual([]); + expect(result.content[0]!.text).toContain('net VAT due 425.00'); + }); + + it('returns individual records when asked', async () => { + const client = clientReturning(fixture); + const result = await run(vatReportTool, client, authContext([B1]), { + businessId: B1, + month: '2026-01-01', + includeRecords: true, + }); + const structured = result.structuredContent as { + records: Array<{ counterparty: { name: string } }>; + }; + expect(structured.records).toHaveLength(3); + expect(structured.records[0]!.counterparty).toEqual({ id: 'cp1', name: 'Acme' }); + }); + + it('describes a refund as negative net VAT', async () => { + const client = clientReturning({ + vatReport: { income: [record(10, 100)], expenses: [record(60, 600)] }, + }); + const result = await run(vatReportTool, client, authContext([B1]), { + businessId: B1, + month: '2026-01-01', + }); + expect((result.structuredContent as { totals: { netVatDue: number } }).totals.netVatDue).toBe( + -50, + ); + expect(result.content[0]!.text).toContain('refundable'); + }); + + it('passes the business id and month to upstream', async () => { + let sentBody: unknown; + const client = clientReturning(fixture, body => (sentBody = body)); + await run(vatReportTool, client, authContext([B1]), { businessId: B1, month: '2026-01-01' }); + const { filters } = (sentBody as { variables: { filters: Record } }).variables; + expect(filters).toEqual({ + financialEntityId: B1, + monthDate: '2026-01-01', + chargesType: 'ALL', + }); + }); +}); + +// --------------------------------------------------------------------------- +// counterparty_totals +// --------------------------------------------------------------------------- + +describe('counterpartyTotalsTool', () => { + const sum = (id: string, name: string, total: number) => ({ + business: { id, name }, + credit: ils(total > 0 ? total : 0), + debit: ils(total < 0 ? -total : 0), + total: ils(total), + foreignCurrenciesSum: [ + { currency: 'USD', credit: ils(1), debit: ils(0), total: ils(1) }, + ], + }); + + const fixture = { + businessTransactionsSumFromLedgerRecords: { + __typename: 'BusinessTransactionsSumFromLedgerRecordsSuccessfulResult', + businessTransactionsSum: [ + sum('small', 'Small Co', 100), + sum('big', 'Big Co', -9000), + sum('mid', 'Mid Co', 500), + ], + }, + }; + + it('orders counterparties by absolute total so the biggest survive truncation', async () => { + const client = clientReturning(fixture); + const result = await run(counterpartyTotalsTool, client, authContext([B1]), { + businessId: B1, + }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + counterparties: Array<{ counterparty: { id: string }; total: { value: number } }>; + }; + expect(structured.counterparties.map(row => row.counterparty.id)).toEqual([ + 'big', + 'mid', + 'small', + ]); + }); + + it('excludes revaluation entries unless asked', async () => { + let sentBody: unknown; + const client = clientReturning(fixture, body => (sentBody = body)); + await run(counterpartyTotalsTool, client, authContext([B1]), { businessId: B1 }); + const { filters } = (sentBody as { variables: { filters: Record } }).variables; + expect(filters).toEqual({ ownerIds: [B1], includeRevaluation: false }); + }); + + // An empty list would read as "no activity" — a materially different answer. + it('surfaces an upstream CommonError as an error, not an empty result', async () => { + const client = clientReturning({ + businessTransactionsSumFromLedgerRecords: { + __typename: 'CommonError', + message: 'Ledger unavailable', + }, + }); + const result = await run(counterpartyTotalsTool, client, authContext([B1]), { + businessId: B1, + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { message: string }).message).toContain( + 'Ledger unavailable', + ); + }); +}); + +// --------------------------------------------------------------------------- +// ledger_records +// --------------------------------------------------------------------------- + +describe('ledgerRecordsTool', () => { + const fixture = { + businessTransactionsFromLedgerRecords: { + __typename: 'BusinessTransactionsFromLedgerRecordsSuccessfulResult', + businessTransactions: [ + { + chargeId: 'c1', + invoiceDate: '2026-01-10', + business: { id: 'cp1', name: 'Acme' }, + counterAccount: { id: 'ta1', name: 'Income' }, + amount: ils(1000), + foreignAmount: { raw: 300, formatted: '$300', currency: 'USD' }, + details: 'Retainer', + reference: 'REF-1', + }, + ], + }, + }; + + it('normalizes ledger records', async () => { + const client = clientReturning(fixture); + const result = await run(ledgerRecordsTool, client, authContext([B1]), { businessId: B1 }); + + expect(result.isError).toBeUndefined(); + const structured = result.structuredContent as { + records: Array>; + }; + expect(structured.records[0]).toMatchObject({ + chargeId: 'c1', + date: '2026-01-10', + business: { id: 'cp1', name: 'Acme' }, + counterAccount: { id: 'ta1', name: 'Income' }, + details: 'Retainer', + reference: 'REF-1', + }); + expect(structured.records[0]!.foreignAmount).toEqual({ + value: 300, + formatted: '$300', + currency: 'USD', + }); + }); + + it('surfaces an upstream CommonError as an error', async () => { + const client = clientReturning({ + businessTransactionsFromLedgerRecords: { + __typename: 'CommonError', + message: 'Bad filter', + }, + }); + const result = await run(ledgerRecordsTool, client, authContext([B1]), { businessId: B1 }); + expect(result.isError).toBe(true); + }); + + // Row-level output keeps the tighter bound the other row tools use. + it('rejects a range wider than the row-level cap', async () => { + const client = clientReturning(fixture); + const result = await run(ledgerRecordsTool, client, authContext([B1]), { + businessId: B1, + fromDate: '2019-01-01', + toDate: '2026-01-01', + }); + expect(result.isError).toBe(true); + }); +}); diff --git a/packages/mcp-server/src/tools/__tests__/registry-instance.test.ts b/packages/mcp-server/src/tools/__tests__/registry-instance.test.ts index 49311d765..26044502e 100644 --- a/packages/mcp-server/src/tools/__tests__/registry-instance.test.ts +++ b/packages/mcp-server/src/tools/__tests__/registry-instance.test.ts @@ -26,4 +26,37 @@ describe('production tool registry', () => { // No required parameters — the model can always call it cold. expect(descriptor?.inputSchema.required).toBeUndefined(); }); + + // A model asked "how much did we make this year" reaches for whichever + // plausible tool it sees first. If `search_charges` led, it would rebuild + // bookkeeping from raw rows — the failure the report tools exist to prevent — + // so the ordering is behaviour, not cosmetics. + it('advertises the report tools ahead of the row-level ones', () => { + const order = toolRegistry.describe().map(tool => tool.name); + const positionOf = (name: string) => { + const index = order.indexOf(name); + expect(index, `${name} is not registered`).toBeGreaterThanOrEqual(0); + return index; + }; + + const lastReport = Math.max( + positionOf('accounter_list_accounts'), + positionOf('accounter_income_expense_summary'), + positionOf('accounter_profit_and_loss'), + positionOf('accounter_vat_report'), + positionOf('accounter_counterparty_totals'), + ); + const firstRowLevel = Math.min( + positionOf('accounter_search_charges'), + positionOf('accounter_get_charges'), + positionOf('accounter_get_transactions'), + positionOf('accounter_get_documents'), + ); + + expect(lastReport).toBeLessThan(firstRowLevel); + // Account structure is what makes any row-level answer interpretable. + expect(positionOf('accounter_list_accounts')).toBeLessThan( + positionOf('accounter_income_expense_summary'), + ); + }); }); 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 1c369ac3d..68e5a9b50 100644 --- a/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts +++ b/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts @@ -53,6 +53,37 @@ function dataFor(query: string): unknown { if (query.includes('taxCategories')) return { taxCategories: [] }; if (query.includes('allBusinesses')) return { allBusinesses: { nodes: [] } }; if (query.includes('transactionsForBalanceReport')) return { transactionsForBalanceReport: [] }; + if (query.includes('financialAccountsByOwner')) return { financialAccountsByOwner: [] }; + if (query.includes('incomeExpenseChart')) { + return { + incomeExpenseChart: { + currency: 'ILS', + fromDate: '2026-01-01', + toDate: '2026-03-01', + monthlyData: [], + }, + }; + } + if (query.includes('profitAndLossReport')) { + return { profitAndLossReport: { report: { year: 2026 }, reference: [] } }; + } + if (query.includes('vatReport')) return { vatReport: { income: [], expenses: [] } }; + if (query.includes('businessTransactionsSumFromLedgerRecords')) { + return { + businessTransactionsSumFromLedgerRecords: { + __typename: 'BusinessTransactionsSumFromLedgerRecordsSuccessfulResult', + businessTransactionsSum: [], + }, + }; + } + if (query.includes('businessTransactionsFromLedgerRecords')) { + return { + businessTransactionsFromLedgerRecords: { + __typename: 'BusinessTransactionsFromLedgerRecordsSuccessfulResult', + businessTransactions: [], + }, + }; + } return {}; } @@ -75,14 +106,36 @@ function capturingClient() { return { client, headersSeen }; } -/** Minimal valid arguments per tool; everything else is optional. */ +/** + * Minimal valid arguments per tool; everything else is optional. + * + * Any entry carrying `businessId` is a single-business tool, which is also how + * the expected scope below is derived — so a new report tool only has to be + * listed here once. + */ const ARGS_BY_TOOL: Record = { accounter_balance_report: { businessId: B1, fromDate: '2026-01-01', toDate: '2026-03-01' }, accounter_get_charges: { chargeIds: ['c1'] }, accounter_get_transactions: { transactionIds: ['t1'] }, accounter_get_documents: { documentIds: ['d1'] }, + accounter_list_accounts: { businessId: B1 }, + accounter_income_expense_summary: { + businessId: B1, + fromDate: '2026-01-01', + toDate: '2026-03-01', + }, + accounter_profit_and_loss: { businessId: B1, year: 2026 }, + accounter_vat_report: { businessId: B1, month: '2026-01-01' }, + accounter_counterparty_totals: { businessId: B1 }, + accounter_ledger_records: { businessId: B1 }, }; +/** 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; + return args?.businessId ? [args.businessId] : [B1, B2]; +} + describe('registry-wide business-scope forwarding', () => { const tools = toolRegistry.list(); @@ -114,8 +167,8 @@ describe('registry-wide business-scope forwarding', () => { } expect(headersSeen.length, `${name} should call upstream`).toBeGreaterThan(0); - // The balance report narrows to its single businessId; the rest keep both. - const expectedScope = name === 'accounter_balance_report' ? [B1] : [B1, B2]; + // Single-business tools narrow to their businessId; the rest keep both. + const expectedScope = expectedScopeFor(name); for (const headers of headersSeen) { expect(headers[BUSINESS_SCOPE_HEADER]).toBe(expectedScope.join(',')); } diff --git a/packages/mcp-server/src/tools/accounts.ts b/packages/mcp-server/src/tools/accounts.ts new file mode 100644 index 000000000..63d457331 --- /dev/null +++ b/packages/mcp-server/src/tools/accounts.ts @@ -0,0 +1,160 @@ +import { z } from 'zod'; +import type { McpListAccountsQuery, McpListAccountsQueryVariables } from '../gql/index.js'; +import { shapeListResult } from './output.js'; +import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; +import { + assertAuthorizedBusiness, + businessIdInput, + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, +} from './scope-input.js'; + +/** + * The account directory for one business. + * + * This is the answer to "how much money do I have" that the connector was + * missing — not because it reports balances (it deliberately does not; see + * below) but because `type` is what makes a transaction feed interpretable at + * all. Without it, summing every row double-counts credit cards against their + * bank settlement rows, loses deposit sweeps, and mixes securities cost basis + * into cash. Knowing which account a row belongs to, and what kind of account + * that is, resolves all three. + * + * **No balances, on purpose.** The obvious implementation — read the newest + * `Transaction.balance` per account — would be wrong for most accounts: + * upstream stores a hardcoded `0` for credit-card, deposit and SWIFT rows and + * the field is non-null, so a placeholder is indistinguishable from a real zero. + * Reporting `0` for every card would be worse than reporting nothing. Revisit + * once the column distinguishes "no balance reported" from zero. + */ + +export const LIST_ACCOUNTS_TOOL_NAME = 'accounter_list_accounts'; + +const listAccountsInput = z.object({ businessId: businessIdInput }); + +type ListAccountsInput = z.infer; + +const LIST_ACCOUNTS_QUERY = /* GraphQL */ ` + query McpListAccounts($ownerId: UUID!) { + financialAccountsByOwner(ownerId: $ownerId) { + __typename + id + name + number + type + privateOrBusiness + accountTaxCategories { + currency + } + ... on BankFinancialAccount { + bankNumber + branchNumber + iban + swiftCode + } + ... on CardFinancialAccount { + fourDigits + } + } + } +`; + +type RawAccount = McpListAccountsQuery['financialAccountsByOwner'][number]; + +export interface NormalizedAccount { + id: string; + name: string | null; + number: string | null; + /** BANK_ACCOUNT | BANK_DEPOSIT_ACCOUNT | CREDIT_CARD | CRYPTO_WALLET | FOREIGN_SECURITIES */ + type: string | null; + privateOrBusiness: string | null; + /** Currencies the account is configured for, derived from its tax categories. */ + currencies: string[]; + /** Bank-only identifiers; null on other account kinds. */ + bank: { bankNumber: number | null; branchNumber: number | null; iban: string | null } | null; + /** Card-only identifier; null on other account kinds. */ + cardLastFour: string | null; +} + +function normalizeAccount(account: RawAccount): NormalizedAccount { + const bank = + account.__typename === 'BankFinancialAccount' + ? { + bankNumber: account.bankNumber ?? null, + branchNumber: account.branchNumber ?? null, + iban: account.iban ?? null, + } + : null; + return { + id: account.id, + name: account.name ?? null, + number: account.number ?? null, + type: account.type ?? null, + privateOrBusiness: account.privateOrBusiness ?? null, + // De-duplicated: an account can carry several tax categories per currency, + // and the currency list is the useful part, not the mapping. + currencies: [ + ...new Set((account.accountTaxCategories ?? []).map(category => category.currency)), + ].sort(), + bank, + cardLastFour: account.__typename === 'CardFinancialAccount' ? account.fourDigits : null, + }; +} + +async function handler( + input: ListAccountsInput, + context: ToolExecutionContext, +): Promise { + const ownerId = assertAuthorizedBusiness(input.businessId, context); + + const variables: McpListAccountsQueryVariables = { ownerId }; + const data = await context.client.query( + { query: LIST_ACCOUNTS_QUERY, variables }, + context.upstream, + ); + + const accounts = (data.financialAccountsByOwner ?? []).map(normalizeAccount); + const byType = accounts.reduce>((counts, account) => { + const key = account.type ?? 'UNKNOWN'; + counts[key] = (counts[key] ?? 0) + 1; + return counts; + }, {}); + + return shapeListResult({ + items: accounts, + itemsKey: 'accounts', + total: accounts.length, + extra: { + businessId: ownerId, + countsByType: byType, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: (shown, total) => + total === 0 + ? 'This business has no financial accounts on file.' + : `${total} account(s)${shown < total ? ` (showing ${shown})` : ''}: ${Object.entries( + byType, + ) + .map(([type, count]) => `${count} ${type}`) + .join(', ')}.`, + }); +} + +export const listAccountsTool: ToolDefinition = { + name: LIST_ACCOUNTS_TOOL_NAME, + description: + 'List one business’s financial accounts: id, name, number, `type` (BANK_ACCOUNT, ' + + 'BANK_DEPOSIT_ACCOUNT, CREDIT_CARD, CRYPTO_WALLET, FOREIGN_SECURITIES), currencies, and ' + + 'bank/card identifiers. Call this before interpreting transaction data: CREDIT_CARD accounts ' + + 'carry merchant-level rows that are settled again by a matching bank row, so summing both ' + + 'double-counts; BANK_DEPOSIT_ACCOUNT holds sweeps out of the checking account; and ' + + 'FOREIGN_SECURITIES rows have no mirror leg in any bank account. Balances are not available — ' + + 'most sources do not report them. Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: listAccountsInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler, +}; diff --git a/packages/mcp-server/src/tools/financial-reports.ts b/packages/mcp-server/src/tools/financial-reports.ts new file mode 100644 index 000000000..e276c9fc4 --- /dev/null +++ b/packages/mcp-server/src/tools/financial-reports.ts @@ -0,0 +1,611 @@ +import { z } from 'zod'; +import type { + McpIncomeExpenseSummaryQuery, + McpIncomeExpenseSummaryQueryVariables, + McpProfitAndLossQuery, + McpProfitAndLossQueryVariables, + McpVatReportQuery, + McpVatReportQueryVariables, +} from '../gql/index.js'; +import { DAY_MS, parseCalendarDate, TIMELESS_DATE } from './dates.js'; +import { normalizeAmount, type NormalizedAmount } from './entity-shapes.js'; +import { ToolInputError } from './execute.js'; +import { shapeListResult } from './output.js'; +import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; +import { + assertAuthorizedBusiness, + businessIdInput, + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, +} from './scope-input.js'; + +/** + * Report-level tools (spec §8.2, feedback §1). + * + * The connector previously exposed only rows, so an agent asked "how much did we + * make this year" had to rebuild bookkeeping from raw transactions — the part it + * is most likely to get wrong. Accounter already computes these reports; these + * tools just expose them. + * + * All three are single-business. That is not only an authorization choice: the + * upstream resolvers behind `incomeExpenseChart` and `profitAndLossReport` take + * no owner argument and read it from the forwarded `x-business-scope`, which + * `execute.ts` narrows using the required `businessId` field. See + * `businessIdInput` in `scope-input.ts`. + */ + +export const INCOME_EXPENSE_SUMMARY_TOOL_NAME = 'accounter_income_expense_summary'; +export const PROFIT_AND_LOSS_TOOL_NAME = 'accounter_profit_and_loss'; +export const VAT_REPORT_TOOL_NAME = 'accounter_vat_report'; + +/** + * Aggregate reports return one row per month, not per transaction, so the + * ~3-year bound the row-level tools need would only force needless extra calls. + * Ten years covers "chart my money from the beginning" in a single request. + */ +export const MAX_AGGREGATE_RANGE_DAYS = 3660; + +/** Bound on the total years a single P&L call may span (report + references). */ +const MAX_REFERENCE_YEARS = 5; +const EARLIEST_REPORT_YEAR = 2000; + +const CURRENCIES = [ + 'ILS', + 'USD', + 'EUR', + 'GBP', + 'AUD', + 'CAD', + 'JPY', + 'SEK', + 'ETH', + 'GRT', + 'USDC', +] as const; + +/** Reject an invalid, inverted, or too-wide range before hitting upstream. */ +function assertAggregateRange(fromDate: string, toDate: string): void { + const from = parseCalendarDate(fromDate); + const to = parseCalendarDate(toDate); + if (from === null || to === null) { + throw new ToolInputError('Invalid fromDate/toDate'); + } + if (from > to) { + throw new ToolInputError('fromDate must be on or before toDate'); + } + if (Math.round((to - from) / DAY_MS) > MAX_AGGREGATE_RANGE_DAYS) { + throw new ToolInputError(`Date range must not exceed ${MAX_AGGREGATE_RANGE_DAYS} days`); + } +} + +// --------------------------------------------------------------------------- +// Income / expense summary +// --------------------------------------------------------------------------- + +const incomeExpenseSummaryInput = z.object({ + businessId: businessIdInput, + fromDate: TIMELESS_DATE.describe('Start of the reporting period (YYYY-MM-DD).'), + toDate: TIMELESS_DATE.describe('End of the reporting period (YYYY-MM-DD).'), + currency: z + .enum(CURRENCIES) + .optional() + .describe( + 'Convert every month to this currency using each transaction’s own historical rate. ' + + 'Defaults to the business’s configured currency.', + ), +}); + +type IncomeExpenseSummaryInput = z.infer; + +const INCOME_EXPENSE_SUMMARY_QUERY = /* GraphQL */ ` + query McpIncomeExpenseSummary($filters: IncomeExpenseChartFilters!) { + incomeExpenseChart(filters: $filters) { + currency + fromDate + toDate + monthlyData { + date + income { + raw + formatted + currency + } + expense { + raw + formatted + currency + } + balance { + raw + formatted + currency + } + } + } + } +`; + +async function incomeExpenseSummaryHandler( + input: IncomeExpenseSummaryInput, + context: ToolExecutionContext, +): Promise { + assertAggregateRange(input.fromDate, input.toDate); + const ownerId = assertAuthorizedBusiness(input.businessId, context); + + const variables: McpIncomeExpenseSummaryQueryVariables = { + filters: { + fromDate: input.fromDate, + toDate: input.toDate, + ...(input.currency ? { currency: input.currency } : {}), + }, + }; + const data = await context.client.query( + { query: INCOME_EXPENSE_SUMMARY_QUERY, variables }, + context.upstream, + ); + + const chart = data.incomeExpenseChart; + const months = (chart?.monthlyData ?? []).map(month => ({ + month: month.date, + income: normalizeAmount(month.income), + expense: normalizeAmount(month.expense), + // Named `cumulativeNet` rather than upstream's `balance`: it is a running + // sum of the period's own flows starting from zero, not an account balance. + cumulativeNet: normalizeAmount(month.balance), + })); + + const totals = months.reduce( + (sum, month) => ({ + income: sum.income + (month.income?.value ?? 0), + expense: sum.expense + (month.expense?.value ?? 0), + }), + { income: 0, expense: 0 }, + ); + const currency = chart?.currency ?? input.currency ?? null; + + return shapeListResult({ + items: months, + itemsKey: 'months', + total: months.length, + extra: { + businessId: ownerId, + currency, + period: { fromDate: input.fromDate, toDate: input.toDate }, + totals: { + income: totals.income, + expense: totals.expense, + net: totals.income - totals.expense, + currency, + }, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: (shown, total) => + total === 0 + ? `No transactions between ${input.fromDate} and ${input.toDate}.` + : `${total} month(s)${shown < total ? ` (showing ${shown})` : ''} from ${input.fromDate} to ` + + `${input.toDate}: income ${totals.income.toFixed(2)}, expense ${totals.expense.toFixed(2)}, ` + + `net ${(totals.income - totals.expense).toFixed(2)} ${currency ?? ''}`.trimEnd() + + '.', + }); +} + +export const incomeExpenseSummaryTool: ToolDefinition = { + name: INCOME_EXPENSE_SUMMARY_TOOL_NAME, + description: + 'Monthly income and expense totals for one business over a date range, each month converted ' + + 'to a single currency using the historical rates for that period — the fastest way to answer ' + + '“how much did we make this year” or to chart money over time. Returns one row per month plus ' + + 'period totals. IMPORTANT: `cumulativeNet` is a running sum of cash flows starting from zero ' + + 'for the requested period, NOT an account balance, and it counts credit-card rows alongside ' + + 'the bank rows that settle them, so it overstates movement — treat it as a trend, not a ' + + 'balance, and use `accounter_list_accounts` to understand the account structure. Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: incomeExpenseSummaryInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler: incomeExpenseSummaryHandler, +}; + +// --------------------------------------------------------------------------- +// Profit and loss +// --------------------------------------------------------------------------- + +const reportYearInput = z + .number() + .int() + .min(EARLIEST_REPORT_YEAR) + .max(2100) + .describe('Calendar year to report on, e.g. 2026.'); + +const profitAndLossInput = z.object({ + businessId: businessIdInput, + year: reportYearInput, + referenceYears: z + .array(reportYearInput) + .max(MAX_REFERENCE_YEARS) + .optional() + .default([]) + .describe( + `Earlier years to return alongside the report year for comparison (up to ${MAX_REFERENCE_YEARS}).`, + ), +}); + +type ProfitAndLossInput = z.infer; + +const PROFIT_AND_LOSS_QUERY = /* GraphQL */ ` + fragment McpProfitAndLossYearFields on ProfitAndLossReportYear { + year + revenue { + amount { + raw + formatted + currency + } + } + costOfSales { + amount { + raw + formatted + currency + } + } + grossProfit { + raw + formatted + currency + } + researchAndDevelopmentExpenses { + amount { + raw + formatted + currency + } + } + marketingExpenses { + amount { + raw + formatted + currency + } + } + managementAndGeneralExpenses { + amount { + raw + formatted + currency + } + } + operatingProfit { + raw + formatted + currency + } + financialExpenses { + amount { + raw + formatted + currency + } + } + otherIncome { + amount { + raw + formatted + currency + } + } + profitBeforeTax { + raw + formatted + currency + } + tax { + raw + formatted + currency + } + netProfit { + raw + formatted + currency + } + } + + query McpProfitAndLoss($reportYear: Int!, $referenceYears: [Int!]!) { + profitAndLossReport(reportYear: $reportYear, referenceYears: $referenceYears) { + report { + ...McpProfitAndLossYearFields + } + reference { + ...McpProfitAndLossYearFields + } + } + } +`; + +type RawProfitAndLossYear = McpProfitAndLossQuery['profitAndLossReport']['report']; + +interface NormalizedProfitAndLossYear { + year: number; + revenue: NormalizedAmount | null; + costOfSales: NormalizedAmount | null; + grossProfit: NormalizedAmount | null; + researchAndDevelopmentExpenses: NormalizedAmount | null; + marketingExpenses: NormalizedAmount | null; + managementAndGeneralExpenses: NormalizedAmount | null; + operatingProfit: NormalizedAmount | null; + financialExpenses: NormalizedAmount | null; + otherIncome: NormalizedAmount | null; + profitBeforeTax: NormalizedAmount | null; + tax: NormalizedAmount | null; + netProfit: NormalizedAmount | null; +} + +/** + * Flatten each line item to its total. + * + * Upstream models the expense lines as `ReportCommentary`, which nests a + * per-sort-code breakdown under `records`. That breakdown is large and is not + * what a P&L question asks for; a caller who needs it can drill in through the + * ledger tools. + */ +function normalizeProfitAndLossYear(year: RawProfitAndLossYear): NormalizedProfitAndLossYear { + return { + year: year.year, + revenue: normalizeAmount(year.revenue?.amount), + costOfSales: normalizeAmount(year.costOfSales?.amount), + grossProfit: normalizeAmount(year.grossProfit), + researchAndDevelopmentExpenses: normalizeAmount(year.researchAndDevelopmentExpenses?.amount), + marketingExpenses: normalizeAmount(year.marketingExpenses?.amount), + managementAndGeneralExpenses: normalizeAmount(year.managementAndGeneralExpenses?.amount), + operatingProfit: normalizeAmount(year.operatingProfit), + financialExpenses: normalizeAmount(year.financialExpenses?.amount), + otherIncome: normalizeAmount(year.otherIncome?.amount), + profitBeforeTax: normalizeAmount(year.profitBeforeTax), + tax: normalizeAmount(year.tax), + netProfit: normalizeAmount(year.netProfit), + }; +} + +async function profitAndLossHandler( + input: ProfitAndLossInput, + context: ToolExecutionContext, +): Promise { + const ownerId = assertAuthorizedBusiness(input.businessId, context); + if (input.referenceYears.includes(input.year)) { + throw new ToolInputError('referenceYears must not contain the report year'); + } + + const variables: McpProfitAndLossQueryVariables = { + reportYear: input.year, + referenceYears: [...input.referenceYears], + }; + const data = await context.client.query( + { query: PROFIT_AND_LOSS_QUERY, variables }, + context.upstream, + ); + + const report = normalizeProfitAndLossYear(data.profitAndLossReport.report); + const reference = (data.profitAndLossReport.reference ?? []).map(normalizeProfitAndLossYear); + + // A P&L is one object, not a list, so the items array holds the report year + // followed by its reference years — that keeps the byte guard meaningful + // (trailing reference years drop first) instead of all-or-nothing. + return shapeListResult({ + items: [report, ...reference], + itemsKey: 'years', + total: 1 + reference.length, + extra: { + businessId: ownerId, + reportYear: input.year, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: () => + `Profit and loss for ${input.year}: revenue ${report.revenue?.formatted ?? 'n/a'}, ` + + `operating profit ${report.operatingProfit?.formatted ?? 'n/a'}, ` + + `net profit ${report.netProfit?.formatted ?? 'n/a'}` + + (reference.length > 0 + ? ` (with ${reference.length} reference year(s): ${reference.map(y => y.year).join(', ')}).` + : '.'), + }); +} + +export const profitAndLossTool: ToolDefinition = { + name: PROFIT_AND_LOSS_TOOL_NAME, + description: + 'The accountant-grade profit and loss statement for one business and calendar year, computed ' + + 'from the double-entry ledger: revenue, cost of sales, gross profit, R&D / marketing / ' + + 'management-and-general expenses, operating profit, financial expenses, other income, profit ' + + 'before tax, tax, and net profit. Pass `referenceYears` for year-over-year comparison. Prefer ' + + 'this over deriving profitability from transactions or documents — the ledger is authoritative. ' + + 'Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: profitAndLossInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler: profitAndLossHandler, +}; + +// --------------------------------------------------------------------------- +// VAT report +// --------------------------------------------------------------------------- + +const vatReportInput = z.object({ + businessId: businessIdInput, + month: TIMELESS_DATE.describe( + 'Any date within the reporting month (YYYY-MM-DD); the report covers that whole month.', + ), + flow: z + .enum(['ALL', 'INCOME', 'EXPENSE']) + .optional() + .default('ALL') + .describe('Restrict to income (output VAT) or expense (input VAT) records.'), + includeRecords: z + .boolean() + .optional() + .default(false) + .describe( + 'Include the individual per-document records (default false — the totals answer most questions).', + ), +}); + +type VatReportInput = z.infer; + +const VAT_REPORT_QUERY = /* GraphQL */ ` + fragment McpVatRecordFields on VatReportRecord { + chargeId + documentId + documentSerial + documentDate + chargeDate + business { + id + name + } + amount { + raw + formatted + currency + } + localAmount { + raw + formatted + currency + } + localVat { + raw + formatted + currency + } + localVatAfterDeduction { + raw + formatted + currency + } + vatNumber + isProperty + } + + query McpVatReport($filters: VatReportFilter) { + vatReport(filters: $filters) { + income { + ...McpVatRecordFields + } + expenses { + ...McpVatRecordFields + } + } + } +`; + +type RawVatRecord = McpVatReportQuery['vatReport']['income'][number]; + +function normalizeVatRecord(record: RawVatRecord) { + return { + chargeId: record.chargeId, + documentId: record.documentId ?? null, + documentSerial: record.documentSerial ?? null, + documentDate: record.documentDate ?? null, + chargeDate: record.chargeDate ?? null, + counterparty: record.business ? { id: record.business.id, name: record.business.name } : null, + amount: normalizeAmount(record.amount), + localAmount: normalizeAmount(record.localAmount), + localVat: normalizeAmount(record.localVat), + localVatAfterDeduction: normalizeAmount(record.localVatAfterDeduction), + vatNumber: record.vatNumber ?? null, + isProperty: record.isProperty, + }; +} + +/** Sum the deductible local VAT and local amounts across a side of the report. */ +function sumVat(records: readonly ReturnType[]) { + return records.reduce( + (totals, record) => ({ + count: totals.count + 1, + amount: totals.amount + (record.localAmount?.value ?? 0), + vat: totals.vat + (record.localVatAfterDeduction?.value ?? record.localVat?.value ?? 0), + }), + { count: 0, amount: 0, vat: 0 }, + ); +} + +async function vatReportHandler( + input: VatReportInput, + context: ToolExecutionContext, +): Promise { + if (parseCalendarDate(input.month) === null) { + throw new ToolInputError('Invalid month'); + } + const ownerId = assertAuthorizedBusiness(input.businessId, context); + + const variables: McpVatReportQueryVariables = { + filters: { + // Passed explicitly even though `x-business-scope` already narrows + // upstream — this resolver takes the entity id, so state it (mirrors the + // `byOwners` reasoning in `charges.ts`). + financialEntityId: ownerId, + monthDate: input.month, + chargesType: input.flow, + }, + }; + const data = await context.client.query( + { query: VAT_REPORT_QUERY, variables }, + context.upstream, + ); + + const income = (data.vatReport?.income ?? []).map(normalizeVatRecord); + const expenses = (data.vatReport?.expenses ?? []).map(normalizeVatRecord); + const incomeTotals = sumVat(income); + const expenseTotals = sumVat(expenses); + const netVat = incomeTotals.vat - expenseTotals.vat; + + const totals = { + income: incomeTotals, + expenses: expenseTotals, + // Positive means VAT owed to the tax authority; negative means refundable. + netVatDue: netVat, + }; + + // The records are the bulk of the payload and are opt-in, so when they are + // omitted the tool still reports how many there were. + const records = input.includeRecords ? [...income, ...expenses] : []; + + return shapeListResult({ + items: records, + itemsKey: 'records', + total: input.includeRecords ? income.length + expenses.length : 0, + extra: { + businessId: ownerId, + month: input.month, + flow: input.flow, + totals, + recordCounts: { income: income.length, expenses: expenses.length }, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: () => + `VAT for ${input.month}: output VAT ${incomeTotals.vat.toFixed(2)} on ${incomeTotals.count} ` + + `income record(s), input VAT ${expenseTotals.vat.toFixed(2)} on ${expenseTotals.count} ` + + `expense record(s); net VAT ${netVat >= 0 ? 'due' : 'refundable'} ${Math.abs(netVat).toFixed(2)}.`, + }); +} + +export const vatReportTool: ToolDefinition = { + name: VAT_REPORT_TOOL_NAME, + description: + 'The monthly VAT report for one business: output VAT on income, input VAT on expenses, and ' + + 'the resulting net VAT due (positive) or refundable (negative), all in local currency. Set ' + + '`includeRecords` to also return the individual per-document records. Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: vatReportInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler: vatReportHandler, +}; diff --git a/packages/mcp-server/src/tools/ledger-reports.ts b/packages/mcp-server/src/tools/ledger-reports.ts new file mode 100644 index 000000000..0919b4af9 --- /dev/null +++ b/packages/mcp-server/src/tools/ledger-reports.ts @@ -0,0 +1,367 @@ +import { z } from 'zod'; +import type { + McpCounterpartyTotalsQuery, + McpCounterpartyTotalsQueryVariables, + McpLedgerRecordsQuery, + McpLedgerRecordsQueryVariables, +} from '../gql/index.js'; +import { DAY_MS, parseCalendarDate, TIMELESS_DATE } from './dates.js'; +import { normalizeAmount } from './entity-shapes.js'; +import { ToolInputError } from './execute.js'; +import { shapeListResult } from './output.js'; +import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; +import { + assertAuthorizedBusiness, + businessIdInput, + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, +} from './scope-input.js'; + +/** + * Ledger-backed tools (feedback §1). + * + * The double-entry ledger is the authoritative answer to "what kind of movement + * was this" and "who did we transact with" — raw bank rows are not. These two + * tools expose it at both levels: aggregated per counterparty, and record by + * record. + * + * Both upstream queries return a union with `CommonError`, so both handlers must + * branch on `__typename` rather than assuming success. + */ + +export const COUNTERPARTY_TOTALS_TOOL_NAME = 'accounter_counterparty_totals'; +export const LEDGER_RECORDS_TOOL_NAME = 'accounter_ledger_records'; + +/** Row-level output, so the same ~3-year bound the other row tools use. */ +export const MAX_LEDGER_RANGE_DAYS = 1096; +/** Aggregated per counterparty — a decade of totals is still a small payload. */ +export const MAX_TOTALS_RANGE_DAYS = 3660; + +const ID_LIST_CAP = 100; + +function optionalIdList(max: number) { + return z.preprocess( + value => (Array.isArray(value) && value.length === 0 ? undefined : value), + z.array(z.string().min(1)).min(1).max(max).optional(), + ); +} + +function assertRange(fromDate: string | undefined, toDate: string | undefined, maxDays: number) { + let from: number | undefined; + let to: number | undefined; + if (fromDate !== undefined) { + const parsed = parseCalendarDate(fromDate); + if (parsed === null) throw new ToolInputError('Invalid fromDate'); + from = parsed; + } + if (toDate !== undefined) { + const parsed = parseCalendarDate(toDate); + if (parsed === null) throw new ToolInputError('Invalid toDate'); + to = parsed; + } + if (from !== undefined && to !== undefined) { + if (from > to) { + throw new ToolInputError('fromDate must be on or before toDate'); + } + if (Math.round((to - from) / DAY_MS) > maxDays) { + throw new ToolInputError(`Date range must not exceed ${maxDays} days`); + } + } +} + +/** + * Surface an upstream `CommonError` as a tool input error rather than an empty + * result — an empty list would read as "this business had no activity", which is + * a materially different (and wrong) answer. + */ +function assertNotCommonError(result: { __typename?: string; message?: string }): void { + if (result.__typename === 'CommonError') { + throw new ToolInputError(result.message ?? 'Upstream rejected the ledger query'); + } +} + +// --------------------------------------------------------------------------- +// Counterparty totals +// --------------------------------------------------------------------------- + +const counterpartyTotalsInput = z.object({ + businessId: businessIdInput, + fromDate: TIMELESS_DATE.optional().describe('Only ledger records on/after this date.'), + toDate: TIMELESS_DATE.optional().describe('Only ledger records on/before this date.'), + counterpartyIds: optionalIdList(ID_LIST_CAP) + .optional() + .describe('Restrict to these counterparty (business) ids. Omit for all counterparties.'), + includeRevaluation: z + .boolean() + .optional() + .default(false) + .describe( + 'Include currency revaluation entries. These are bookkeeping adjustments, not real ' + + 'activity, so they are excluded by default.', + ), +}); + +type CounterpartyTotalsInput = z.infer; + +const COUNTERPARTY_TOTALS_QUERY = /* GraphQL */ ` + query McpCounterpartyTotals($filters: BusinessTransactionsFilter) { + businessTransactionsSumFromLedgerRecords(filters: $filters) { + __typename + ... on BusinessTransactionsSumFromLedgerRecordsSuccessfulResult { + businessTransactionsSum { + business { + id + name + } + credit { + raw + formatted + currency + } + debit { + raw + formatted + currency + } + total { + raw + formatted + currency + } + foreignCurrenciesSum { + currency + credit { + raw + formatted + currency + } + debit { + raw + formatted + currency + } + total { + raw + formatted + currency + } + } + } + } + ... on CommonError { + message + } + } + } +`; + +async function counterpartyTotalsHandler( + input: CounterpartyTotalsInput, + context: ToolExecutionContext, +): Promise { + assertRange(input.fromDate, input.toDate, MAX_TOTALS_RANGE_DAYS); + const ownerId = assertAuthorizedBusiness(input.businessId, context); + + const variables: McpCounterpartyTotalsQueryVariables = { + filters: { + ownerIds: [ownerId], + includeRevaluation: input.includeRevaluation, + ...(input.fromDate ? { fromDate: input.fromDate } : {}), + ...(input.toDate ? { toDate: input.toDate } : {}), + ...(input.counterpartyIds ? { businessIDs: [...input.counterpartyIds] } : {}), + }, + }; + const data = await context.client.query( + { query: COUNTERPARTY_TOTALS_QUERY, variables }, + context.upstream, + ); + + const result = data.businessTransactionsSumFromLedgerRecords; + assertNotCommonError(result); + const rows = + result.__typename === 'BusinessTransactionsSumFromLedgerRecordsSuccessfulResult' + ? result.businessTransactionsSum + : []; + + const counterparties = rows + .map(row => ({ + counterparty: { id: row.business.id, name: row.business.name }, + credit: normalizeAmount(row.credit), + debit: normalizeAmount(row.debit), + total: normalizeAmount(row.total), + foreignCurrencies: (row.foreignCurrenciesSum ?? []).map(sum => ({ + currency: sum.currency, + credit: normalizeAmount(sum.credit), + debit: normalizeAmount(sum.debit), + total: normalizeAmount(sum.total), + })), + })) + // Largest absolute total first: "top customers"/"biggest suppliers" is the + // question this answers, and the byte guard drops from the tail. + .sort((a, b) => Math.abs(b.total?.value ?? 0) - Math.abs(a.total?.value ?? 0)); + + return shapeListResult({ + items: counterparties, + itemsKey: 'counterparties', + total: counterparties.length, + extra: { + businessId: ownerId, + period: { fromDate: input.fromDate ?? null, toDate: input.toDate ?? null }, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: (shown, total) => + total === 0 + ? 'No ledger activity matched the given filters.' + : `${total} counterparty/counterparties${shown < total ? ` (showing top ${shown})` : ''}, ` + + 'ordered by absolute total.', + }); +} + +export const counterpartyTotalsTool: ToolDefinition = { + name: COUNTERPARTY_TOTALS_TOOL_NAME, + description: + 'Totals per counterparty from the double-entry ledger for one business — credit, debit and ' + + 'net total in local currency, plus a per-currency breakdown, ordered by largest absolute ' + + 'total. This is the direct way to answer “who are our biggest customers/suppliers” or to ' + + 'break revenue and expense down by counterparty, without aggregating charges yourself. ' + + 'Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: counterpartyTotalsInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler: counterpartyTotalsHandler, +}; + +// --------------------------------------------------------------------------- +// Ledger records +// --------------------------------------------------------------------------- + +const ledgerRecordsInput = z.object({ + businessId: businessIdInput, + fromDate: TIMELESS_DATE.optional().describe('Only ledger records on/after this date.'), + toDate: TIMELESS_DATE.optional().describe('Only ledger records on/before this date.'), + counterpartyIds: optionalIdList(ID_LIST_CAP) + .optional() + .describe('Restrict to these counterparty (business) ids.'), + includeRevaluation: z + .boolean() + .optional() + .default(false) + .describe('Include currency revaluation entries (excluded by default).'), +}); + +type LedgerRecordsInput = z.infer; + +const LEDGER_RECORDS_QUERY = /* GraphQL */ ` + query McpLedgerRecords($filters: BusinessTransactionsFilter) { + businessTransactionsFromLedgerRecords(filters: $filters) { + __typename + ... on BusinessTransactionsFromLedgerRecordsSuccessfulResult { + businessTransactions { + chargeId + invoiceDate + business { + id + name + } + counterAccount { + id + name + } + amount { + raw + formatted + currency + } + foreignAmount { + raw + formatted + currency + } + details + reference + } + } + ... on CommonError { + message + } + } + } +`; + +async function ledgerRecordsHandler( + input: LedgerRecordsInput, + context: ToolExecutionContext, +): Promise { + assertRange(input.fromDate, input.toDate, MAX_LEDGER_RANGE_DAYS); + const ownerId = assertAuthorizedBusiness(input.businessId, context); + + const variables: McpLedgerRecordsQueryVariables = { + filters: { + ownerIds: [ownerId], + includeRevaluation: input.includeRevaluation, + ...(input.fromDate ? { fromDate: input.fromDate } : {}), + ...(input.toDate ? { toDate: input.toDate } : {}), + ...(input.counterpartyIds ? { businessIDs: [...input.counterpartyIds] } : {}), + }, + }; + const data = await context.client.query( + { query: LEDGER_RECORDS_QUERY, variables }, + context.upstream, + ); + + const result = data.businessTransactionsFromLedgerRecords; + assertNotCommonError(result); + const rows = + result.__typename === 'BusinessTransactionsFromLedgerRecordsSuccessfulResult' + ? result.businessTransactions + : []; + + const records = rows.map(row => ({ + chargeId: row.chargeId, + date: row.invoiceDate, + business: { id: row.business.id, name: row.business.name }, + counterAccount: row.counterAccount + ? { id: row.counterAccount.id, name: row.counterAccount.name } + : null, + amount: normalizeAmount(row.amount), + foreignAmount: normalizeAmount(row.foreignAmount), + details: row.details ?? null, + reference: row.reference ?? null, + })); + + return shapeListResult({ + items: records, + itemsKey: 'records', + total: records.length, + extra: { + businessId: ownerId, + period: { fromDate: input.fromDate ?? null, toDate: input.toDate ?? null }, + scope: { businessIds: context.readScope.businessIds }, + }, + summarize: (shown, total) => + total === 0 + ? 'No ledger records matched the given filters.' + : `${total} ledger record(s)${shown < total ? ` (showing ${shown})` : ''}.`, + }); +} + +export const ledgerRecordsTool: ToolDefinition = { + name: LEDGER_RECORDS_TOOL_NAME, + description: + 'Individual double-entry ledger records for one business: date, counterparty, counter ' + + 'account, local and foreign amounts, charge id, and reference. Use this when you need to see ' + + 'how a movement was actually booked — the ledger, not the raw bank feed, is authoritative ' + + 'about what a movement was. For per-counterparty aggregates use ' + + '`accounter_counterparty_totals` instead. Read-only. ' + + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, + inputSchema: ledgerRecordsInput, + policy: { + requiredRoles: ['business_owner', 'accountant'], + requiresBusinessScope: true, + dataClassification: 'business', + }, + handler: ledgerRecordsHandler, +}; diff --git a/packages/mcp-server/src/tools/registry-instance.ts b/packages/mcp-server/src/tools/registry-instance.ts index e698e893a..c8af32ea6 100644 --- a/packages/mcp-server/src/tools/registry-instance.ts +++ b/packages/mcp-server/src/tools/registry-instance.ts @@ -1,7 +1,10 @@ +import { listAccountsTool } from './accounts.js'; import { listBusinessMembershipsTool } from './businesses.js'; import { getChargesTool } from './charge-details.js'; import { searchChargesTool } from './charges.js'; import { getDocumentsTool } from './document-details.js'; +import { incomeExpenseSummaryTool, profitAndLossTool, vatReportTool } from './financial-reports.js'; +import { counterpartyTotalsTool, ledgerRecordsTool } from './ledger-reports.js'; import { listBusinessesTool, listTagsTool, listTaxCategoriesTool } from './lookups.js'; import { ToolRegistry } from './registry.js'; import { balanceReportTool } from './reports.js'; @@ -21,12 +24,29 @@ 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); + +// Answers before rows. +// +// The report tools are registered ahead of the row-level ones because a model +// asked "how much did we make this year" will otherwise reach for +// `search_charges` and rebuild bookkeeping from raw rows — the exact failure +// this ordering is here to prevent. `list_accounts` leads them: knowing the +// account structure is what makes any transaction-level answer interpretable. +toolRegistry.register(listAccountsTool); +toolRegistry.register(incomeExpenseSummaryTool); +toolRegistry.register(profitAndLossTool); +toolRegistry.register(vatReportTool); +toolRegistry.register(counterpartyTotalsTool); + toolRegistry.register(searchChargesTool); // Charge detail (by id) sits next to search: the model searches, then drills // into specific charges — which nest their transactions and documents. toolRegistry.register(getChargesTool); toolRegistry.register(getTransactionsTool); toolRegistry.register(getDocumentsTool); +// Record-level ledger detail sits with the other drill-down tools, after the +// aggregate that answers most questions about it. +toolRegistry.register(ledgerRecordsTool); toolRegistry.register(listTagsTool); toolRegistry.register(listTaxCategoriesTool); // The full business directory sits with the other reference-data lookups. diff --git a/packages/mcp-server/src/tools/reports.ts b/packages/mcp-server/src/tools/reports.ts index ccd073d10..142f3e0bf 100644 --- a/packages/mcp-server/src/tools/reports.ts +++ b/packages/mcp-server/src/tools/reports.ts @@ -4,7 +4,11 @@ import { DAY_MS, parseCalendarDate, TIMELESS_DATE } from './dates.js'; import { ToolInputError } from './execute.js'; import { shapeListResult } from './output.js'; import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; -import { SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX } from './scope-input.js'; +import { + assertAuthorizedBusiness, + businessIdInput, + SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX, +} from './scope-input.js'; /** * Tool 3: a selected read-only report (spec §8.2). @@ -21,14 +25,7 @@ export const MAX_REPORT_DATE_RANGE_DAYS = 1096; // ~3 years export const MAX_REPORT_ROWS = 1000; const balanceReportInput = z.object({ - businessId: z - .string() - .min(1) - .describe( - 'The business (owner) id to report on — must be one of the businesses you belong to. ' + - 'Unlike the list tools this report covers exactly one business, so the id is required. ' + - 'Use accounter_list_business_memberships to discover ids.', - ), + businessId: businessIdInput, fromDate: TIMELESS_DATE.describe('Start of the reporting period (YYYY-MM-DD).'), toDate: TIMELESS_DATE.describe('End of the reporting period (YYYY-MM-DD).'), reportType: z @@ -77,19 +74,7 @@ async function handler( ): Promise { assertDateRange(input); - // Report on the business the caller actually asked for. Deriving the owner - // from the scope instead (`readScope.businessIds[0]`) happens to agree today - // only because the policy narrows the scope to exactly this one business — it - // would silently report on the wrong business the moment the scope can hold - // more than one entry. - // - // The membership check is defense in depth: the policy has already verified - // this business is in scope, so a mismatch means the two disagree, and a - // business-scoped tool must never reach upstream with an unauthorized owner. - const ownerId = input.businessId; - if (!context.readScope.businessIds.includes(ownerId)) { - throw new ToolInputError('No authorized business in scope for this report'); - } + const ownerId = assertAuthorizedBusiness(input.businessId, context); const variables: McpBalanceReportQueryVariables = { fromDate: input.fromDate, diff --git a/packages/mcp-server/src/tools/scope-input.ts b/packages/mcp-server/src/tools/scope-input.ts index 3748fc5de..5ee6445cc 100644 --- a/packages/mcp-server/src/tools/scope-input.ts +++ b/packages/mcp-server/src/tools/scope-input.ts @@ -1,4 +1,6 @@ import { z } from 'zod'; +import { ToolInputError } from './execute.js'; +import type { ToolExecutionContext } from './registry.js'; /** * Shared business-scope input fragment. @@ -49,3 +51,42 @@ export const SINGLE_BUSINESS_SCOPE_DESCRIPTION_SUFFIX = 'Scope: this covers exactly one business — pass its id as the required `businessId`. The response ' + 'echoes the effective `scope.businessIds` alongside it. If you have more than one business, call ' + '`accounter_list_business_memberships` first to choose.'; + +/** + * The required singular `businessId` taken by the single-business report tools. + * + * `requestedBusinessIds()` in `execute.ts` recognizes this field by name and + * narrows the resolved read scope — and therefore `x-business-scope` — to just + * this business. That matters beyond authorization: several upstream report + * resolvers (`incomeExpenseChart`, `profitAndLossReport`, `vatReport`) take no + * owner argument at all and derive it from the forwarded scope, so the name of + * this field is what points them at the right business. + */ +export const businessIdInput = z + .string() + .min(1) + .describe( + 'The business (owner) id to report on — must be one of the businesses you belong to. ' + + 'Unlike the list tools this report covers exactly one business, so the id is required. ' + + 'Use accounter_list_business_memberships to discover ids.', + ); + +/** + * Re-check a tool's requested business against the resolved read scope. + * + * Defense in depth: the policy has already verified this business is in scope, + * so a mismatch means the two disagree — and a business-scoped tool must never + * reach upstream with an unauthorized owner. Deriving the owner from + * `readScope.businessIds[0]` instead would agree today only because the policy + * narrows the scope to exactly this one business; it would silently report on + * the wrong business the moment the scope can hold more than one entry. + */ +export function assertAuthorizedBusiness( + businessId: string, + context: ToolExecutionContext, +): string { + if (!context.readScope.businessIds.includes(businessId)) { + throw new ToolInputError('No authorized business in scope for this report'); + } + return businessId; +}