diff --git a/.changeset/mcp-securities-tools.md b/.changeset/mcp-securities-tools.md new file mode 100644 index 000000000..7bd192b96 --- /dev/null +++ b/.changeset/mcp-securities-tools.md @@ -0,0 +1,71 @@ +--- +'@accounter/server': minor +'@accounter/mcp-server': minor +--- + +Expose securities over the MCP connector. + +The securities domain was reachable only through the web UI. An assistant connected over MCP knew +securities as a charge *type* and nothing more: it could not say what the tenant holds, what it paid, +what it traded, or which security is behind a charge. + +**Two new tools.** `accounter_list_security_holdings` is the portfolio — one row per security with +units held, weighted average cost per unit bought, totals bought and sold, and the span of the +ingested history, with a closed-position toggle and free-text search over name, symbol, ISIN, +exchange, currency and every source identifier. Search, ordering (biggest live position first) and +the row cap happen in the tool: upstream takes no search argument, a portfolio is tens to low +hundreds of rows, and matching the `/securities` screen's own rules is what stops the two drifting. + +`accounter_get_security_executions` is the trade history behind it — buys, sales, dividends, +interest, redemptions and transfers, newest first and really paginated, narrowed by security, trade +date and kind. The three identity filters union with each other, since ids, ISINs and symbols are +three ways of naming one axis; asking for one ISIN and one symbol means both securities, not the +empty overlap. + +**The numbers carry their own caveats.** A position is arithmetic over a scraped trade history: the +bank reports no holding, there are no market prices anywhere in the system, pre-history holdings and +splits are invisible, a negative quantity means a history that starts mid-life, and a null amount +means nothing was ingested rather than zero. Amounts are each security's own trade currency and are +never converted. Asked what a portfolio is worth, a model will otherwise add a shekel column to a +dollar one — so the holdings tool computes the sums that *are* valid, per currency, and emits a +machine-readable `caveats` array alongside them. Quantities and average costs are never summed at +all. + +**`includeSecurities` on `accounter_get_charges`**, following the existing `includeTransactions` / +`includeDocuments` idiom. A foreign-securities charge is the one place where what happened is not in +the charge: the cash leg is a bank row and the trade lives in a separate feed. Each security reports +the `securityBusinessId` the other two tools are addressed by, so a charge answer can be followed +into the portfolio. Three states stay distinct — not asked for, no key the feed knows, and a traded +key whose reference scrape is stale. + +**Server:** a new `Query.securityExecutions(filters, page, limit, includeCharges)` with SQL pushdown, +reusing the existing execution and page-info types. It has two paths, because charge links and +pagination do not compose: `matchExecutionsToTransactions` is greedy and one-to-one over the sets it +is handed, so pairing a page's slice would let an execution on page 2 claim the cash movement +belonging to one on page 1 — the same execution reporting a different charge at a different page +size. Requesting links therefore switches to an unpaginated match per security, capped at ten of +them, and both paths order identically so they cannot disagree about what page 1 is. Also adds +`SecurityBusiness.ownerId`, `SecurityHistoryExecution.securityBusiness` and +`ChargeSecurity.securityBusiness`, so rows are owner-tagged, a flat cross-security list can be +grouped, and a charge reaches the security's own identity through the key-to-ISIN bridge. + +**Migration:** the four securities tables' read predicates were still pinned to the singular +`get_current_business_id()`. They were all created after `rls-multi-business-scope`, whose 45-table +list they were never in, and no later migration broadened them. The consequence was a silent +narrowing rather than a leak: a request whose scope spanned several businesses saw securities for one +of them, with nothing in the response saying so. That broke the web client's business switcher, and +it would have broken the connector harder — it forwards its resolved scope upstream and echoes that +scope back, so the caller was told it had seen more than it had. Reads now follow +`get_current_business_scope()` while **writes stay single-tenant**: `USING` is what selects the rows +a statement may act on, and Postgres consults it for DELETE and UPDATE as well as SELECT, so +widening it alone would authorize deleting another in-scope business's row — or updating one into +the write target's ownership, moving it between businesses. Two restrictive per-command policies +pin both back to the explicit target, and the scraper ingestion path is unaffected. + +Widening the read scope also changed what "unique" means underneath it. A Poalim security key is +unique only *within* an owner, so two businesses that both trade one security carry it under the +same key — ordinary for a multi-business tenant. While reads were pinned to one business a key-only +lookup could not go wrong; spanning owners, it files one business's trades under the other's +security. The execution queries now resolve the relation in SQL by joining the identifier bridge on +`(owner_id, identifier_value)` and returning the security business per row, and the two lookups that +remain in memory take an owner-qualified key. diff --git a/codegen.ts b/codegen.ts index 2fb4af122..4e1da52f0 100644 --- a/codegen.ts +++ b/codegen.ts @@ -163,6 +163,8 @@ const config: CodegenConfig = { SecurityHolding: '../modules/foreign-securities/types.js#SecurityHoldingProto', SecurityPosition: '../modules/foreign-securities/types.js#SecurityPositionWithIdProto', SecurityExecution: '../modules/foreign-securities/types.js#SecurityExecutionRow', + PaginatedSecurityExecutions: + '../modules/foreign-securities/types.js#PaginatedSecurityExecutionsProto', SecurityIdentifier: '../modules/foreign-securities/types.js#SecurityIdentifierRow', Shaam6111Report: '../modules/reports/types.js#Shaam6111ReportProto', SortCode: '../modules/sort-codes/types.js#IGetSortCodesByIdsResult', diff --git a/eslint.config.mjs b/eslint.config.mjs index 05a5c95e2..90692e15c 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -207,6 +207,7 @@ export default [ 'PaginatedCharges', 'PaginatedBusinesses', 'PaginatedFinancialEntities', + 'PaginatedSecurityExecutions', 'PCNFileResult', 'PCNRawData', 'ReportCommentary', diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index c779a7121..7ce8e4066 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -18,10 +18,11 @@ 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 twelve read-only tools -(`accounter_list_business_memberships`, `accounter_search_charges`, `accounter_get_charges`, -`accounter_get_transactions`, `accounter_get_documents`, `accounter_get_ledger_records`, -`accounter_get_contracts`, `accounter_list_tags`, `accounter_list_tax_categories`, +Accounter GraphQL server; a curated registry of fourteen read-only tools +(`accounter_list_business_memberships`, `accounter_explain_terminology`, `accounter_search_charges`, +`accounter_get_charges`, `accounter_get_transactions`, `accounter_get_documents`, +`accounter_get_ledger_records`, `accounter_get_contracts`, `accounter_list_security_holdings`, +`accounter_get_security_executions`, `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 @@ -127,6 +128,18 @@ scope, because it _is_ the scope. `accounter_search_charges`. A charge whose `owner` falls outside the resolved scope is dropped as defense-in-depth on top of RLS. + A **foreign-securities** charge additionally carries the security traded and the portfolio + executions behind the cash movement, opt-in via `includeSecurities`. That charge type is the one + place where what happened is not in the charge itself: the cash leg is a bank row, and the trade + lives in a separate ingested feed. Each security reports the `securityBusinessId` that + `accounter_list_security_holdings` and `accounter_get_security_executions` are addressed by, so + the answer can be followed into the portfolio. Three states stay distinct: the field is **absent** + for a charge that is not a securities one and for one fetched without the flag, an **empty array** + means the transaction descriptions carried no key the ingested feed knows, and + `referenceFound: false` on a present security means the reference scrape is stale for a key that + _is_ traded. Nesting executions multiplies the payload, so pair it with explicit `chargeIds` or a + small `pageSize`. + Both charge tools build their filter from one definition (`tools/charge-filters.ts`), and `schema-contract.test.ts` checks that definition against `input ChargeFilter` in `schema.graphql`, so a field added upstream fails the suite instead of quietly becoming unreachable. Three fields — @@ -162,9 +175,32 @@ scope, because it _is_ the scope. owner filter and is forwarded as the upstream `filters.ownerIds`; there is no separate owner input to drift from it. Each row reports its `ownerId` plus the client, period, amount, billing cycle, document type, product/plan, and purchase orders. +- **`accounter_list_security_holdings`** — the **securities portfolio**: one row per security with + units held, weighted average cost per unit bought, totals bought and sold, and the span of the + ingested trade history. `includeClosed` also returns securities traded but no longer held; + `search` matches name (either language), symbol, ISIN, exchange, currency and every source + identifier — the same fields the `/securities` screen searches, so the two cannot drift. Upstream + takes no search argument and a portfolio is tens to low hundreds of rows, so the filtering, + ordering (by |quantity| descending, biggest live position first) and row cap all happen in the + tool. Two things about the numbers are load-bearing: the position is **derived** by adding up + scraped executions rather than read from a bank balance, and amounts are in each security's own + trade currency and are never converted. The response therefore carries `byCurrency` subtotals — + the only valid aggregation — plus a machine-readable `caveats` array, rather than leaving a model + to add a shekel column to a dollar one. Quantities and average costs are never summed at all. +- **`accounter_get_security_executions`** — the **trade history** behind that portfolio: buys, + sales, dividends, interest, redemptions, distributions and transfers, newest first, with dates, + direction, quantity, unit price, net value, commission and Israeli tax. Narrow by security + (`securityBusinessIds`, `isins` or `symbols` — three ways of naming one axis, so they union with + each other), by trade date, and by `tradeTypes`/`transactionTypes`. Really paginated upstream + (1-based `page` here, 0-based there) with `pagination` echoed. `includeCharges` additionally + resolves the charge each trade's cash movement landed on, and **requires naming the securities**: + the pairing is greedy and one-to-one over a security's whole history, so it cannot be computed + from a page — see the note in `docs/connector-gaps-and-decisions.md`. Asking for it unnarrowed is + refused here as a `VALIDATION_ERROR` rather than upstream as an `UPSTREAM_ERROR`, so the failure + says what to add. - **`accounter_list_tags`** — list tags for categorizing charges, optionally filtered by name and by `memberBusinessIds`. Rows carry `ownerId`. Deterministically sorted (name, then id) and - size-capped (≤ 500). + size-capped (≤ 1000). - **`accounter_list_tax_categories`** — list tax categories (id, name, `ownerId`, IRS code, bookkeeping sort code, active flag), optionally filtered by name, active status, or `memberBusinessIds`. Same deterministic sort + cap. @@ -177,8 +213,8 @@ scope, because it _is_ the scope. short. Use `accounter_list_business_memberships` instead for just the caller's own memberships and roles. - **`accounter_balance_report`** — read-only balance report (transactions) for **exactly one** of - your businesses over a bounded date range (≤ 366 days), selected by the required singular - `memberBusinessId`. Requires `business_owner`/`accountant` role; rows are capped at 500 with a + your businesses over a bounded date range (≤ 1096 days), selected by the required singular + `memberBusinessId`. Requires `business_owner`/`accountant` role; rows are capped at 1000 with a `truncated` flag. Every row carries `ownerId` — the one business the report ran for, which the response also reports once alongside the echoed `scope`. @@ -479,9 +515,23 @@ curl -s -X POST http://localhost:3100/mcp -H 'Content-Type: application/json' \ curl -s -X POST http://localhost:3100/mcp -H 'Content-Type: application/json' \ -H "Authorization: Bearer $TOKEN" \ -d '{"jsonrpc":"2.0","id":4,"method":"tools/call","params":{"name":"accounter_list_tags","arguments":{"memberBusinessIds":["00000000-0000-4000-8000-000000000000"]}}}' + +# 8. Securities: the portfolio, then the trades behind one row. +# Expect byCurrency subtotals and a caveats array, and a securityBusinessId to +# carry into step 9. +curl -s -X POST http://localhost:3100/mcp -H 'Content-Type: application/json' \ + -H "Authorization: Bearer $TOKEN" \ + -d '{"jsonrpc":"2.0","id":5,"method":"tools/call","params":{"name":"accounter_list_security_holdings","arguments":{}}}' + +# 9. Executions for one security, newest first, with charge links. +# Run it twice with pageSize 5 and 100: the same execution must report the same +# chargeId either way — that invariant is why includeCharges needs the securities named. +curl -s -X POST http://localhost:3100/mcp -H 'Content-Type: application/json' \ + -H "Authorization: Bearer $TOKEN" \ + -d '{"jsonrpc":"2.0","id":6,"method":"tools/call","params":{"name":"accounter_get_security_executions","arguments":{"securityBusinessIds":[""],"includeCharges":true,"pageSize":5}}}' ``` -The automated equivalent of steps 1–7 (with the Auth0 verifier and upstream mocked) lives in +The automated equivalent of steps 1–9 (with the Auth0 verifier and upstream mocked) lives in `src/__tests__/mcp-e2e.test.ts` and runs with `yarn workspace @accounter/mcp-server test`. ## Troubleshooting @@ -588,11 +638,13 @@ apply. - There is no generic "run any query" surface: every capability is a curated tool with a strict input schema, and the upstream client's read and write paths are separately guarded. -- Responses are **bounded** (date ranges ≤ 366 days, page size ≤ 50, list caps of 500, a - payload-size guard) — very large result sets are truncated with a `truncated`/`continuation` hint - rather than streamed in full. Inline uploads are bounded too: ≤ 10 documents, 256KB per file and - 512KB per call once decoded, against a MIME allowlist. Inline base64 is only viable for small - files at all — see [Why inline upload is small](#why-inline-upload-is-small). +- Responses are **bounded** (date ranges ≤ 1096 days, page sizes ≤ 500, list caps of 200–1000 + depending on the tool, a 60KB payload-size guard — every cap is an exported `MAX_*` constant so + the suite asserts it rather than this file being the record) — very large result sets are + truncated with a `truncated`/`continuation` hint rather than streamed in full. Inline uploads are + bounded too: ≤ 10 documents, 256KB per file and 512KB per call once decoded, against a MIME + allowlist. Inline base64 is only viable for small files at all — see + [Why inline upload is small](#why-inline-upload-is-small). - Rate limiting and metrics are **in-process** (per replica); there is no shared/Redis-backed limiter or Prometheus exposition yet (the limiter and metrics are behind swappable seams). - Tracing is exported to OpenTelemetry/Grafana Tempo (opt-in via `OTEL_ENABLED=1`), but metrics diff --git a/packages/mcp-server/docs/connector-gaps-and-decisions.md b/packages/mcp-server/docs/connector-gaps-and-decisions.md index 6f2d1bd9d..30e8c2b25 100644 --- a/packages/mcp-server/docs/connector-gaps-and-decisions.md +++ b/packages/mcp-server/docs/connector-gaps-and-decisions.md @@ -88,6 +88,105 @@ user-delegated grant on the Accounter API to the new app, update the connector's Claude Desktop, and restore the test application's original name. Best paired with any other change that already requires re-granting API access. +## Recorded findings (not gaps) + +Things learned while building a tool that are worth not re-deriving. + +### Securities tables were never in the multi-business RLS scope (2026-08-23, fixed) + +`2026-05-25T10-00-00.rls-multi-business-scope` switched every `tenant_isolation` read predicate to +`owner_id = ANY(accounter_schema.get_current_business_scope())`, leaving writes on +`get_current_business_id()`. Its table list covered 45 tables. All four securities tables — +`poalim_securities`, `poalim_securities_transactions`, `businesses_securities`, +`security_identifiers` — were created _after_ it (2026-08-11 / 08-13 / 08-20) and so still read +through the singular helper, verified against a live database before the fix. + +The failure mode was a **silent narrowing, not a leak**: the connector forwards its resolved read +scope as `x-business-scope` and echoes that scope back to the caller, so a two-business caller was +told it had seen both while the securities tables had served one. The web client's business switcher +had the same bug. Fixed by `2026-08-23T10-00-00.rls-scope-securities-tables`, with predicates +byte-identical to the earlier migration's. + +**The general lesson:** a table added after that migration does not inherit its predicate, and +nothing fails loudly when it doesn't. Any new owner-scoped table needs +`owner_id = ANY(get_current_business_scope())` written into its own creating migration. When adding +a tool over tables you did not create, check `pg_policies.qual` for them before trusting +`x-business-scope` to have narrowed anything. + +### A Poalim security key is unique only within an owner (2026-08-24, fixed) + +Found in review of the securities tools, and reachable _because_ the read scope was widened above. + +`accounter_schema.security_identifiers` is unique on +`(owner_id, identifier_type, identifier_value)`, and `poalim_securities` dedupes per owner too. So +two of a tenant's businesses that both trade one security each carry it under the same Poalim key — +the ordinary case for a multi-business tenant, not an exotic one. While reads were pinned to a +single business a key-only lookup could not go wrong; once they follow a scope that spans owners, +both rows are visible at once and a `Map` keeps whichever was written last, filing +one business's trades under the other's security. + +Three lookups had it: the executions-to-business mapping (twice), the charge-to-security bridge, and +the reference-details loader. All now carry the owner — the execution queries resolve it in SQL by +joining `security_identifiers` on `(owner_id, identifier_value)` and returning `business_id` per +row, so the relation is expressed once rather than rebuilt from a map that cannot hold it; the two +DataLoaders take an owner-qualified key. + +**The general lesson:** widening a read scope silently changes what "unique" means for every lookup +underneath it. A natural key that was unambiguous under single-business reads may only be unique +_per owner_ — check the unique index, not the intuition. + +### Writes stay single-tenant even when reads do not + +`USING` selects the rows a statement may act on, and Postgres consults it for DELETE and UPDATE as +well as SELECT; `WITH CHECK` constrains only the _new_ values an INSERT or UPDATE writes. A +permissive policy whose `USING` spans the read scope therefore authorizes deleting another in-scope +business's row — and updating one, since the `WITH CHECK` will happily accept the result once the +new value names the write target, which is to say the row gets _moved_ between businesses. + +Every tenant-isolated table needs two RESTRICTIVE per-command policies alongside the permissive one: + +```sql +CREATE POLICY tenant_isolation_delete ON … AS RESTRICTIVE FOR DELETE + USING (owner_id = accounter_schema.get_current_business_id()); +CREATE POLICY tenant_isolation_update ON … AS RESTRICTIVE FOR UPDATE + USING (owner_id = accounter_schema.get_current_business_id()); +``` + +They must be per-command: a restrictive `FOR ALL` would apply to SELECT and undo the multi-business +read scope entirely. INSERT needs none, having no `USING` at all. + +> **Open item.** `2026-05-26T10-00-00.rls-delete-write-target` added the DELETE half for the 45 +> tables it covered, but not the UPDATE half. Those tables still allow an in-scope cross-business +> UPDATE. Out of scope for the securities work, and worth its own change. + +### Charge links and pagination do not compose (`accounter_get_security_executions`) + +`matchExecutionsToTransactions` pairs a securities execution with the bank row behind it. There is +no link in the source — the scrape has no per-execution id — so the pairing is derived, exact, and +**greedy and one-to-one over the sets it is handed**, consuming executions oldest-first. + +Hand it a page's slice and an execution on page 2 can claim the cash movement that belongs to one on +page 1, so the _same_ execution reports a _different_ charge at a different page size. A paginated +query therefore cannot resolve charge links from its own page. + +`Query.securityExecutions` splits into two paths for this reason: without `includeCharges` the +filter pushes into SQL and the page is a `LIMIT`/`OFFSET` slice; with it, each named security's +whole history is fetched and paired, then filtered and sliced in memory — which is why that path +caps how many securities the filter may resolve to, and why the tool refuses `includeCharges` unless +the securities are named. Both paths order identically so they cannot disagree about what page 1 is. + +**The general lesson:** before paginating a result whose fields are computed across rows, check +whether the computation is order- or set-dependent. A greedy one-to-one assignment is. + +### These integration suites share one database and bypass RLS + +`foreign-securities.integration.test.ts` and `security-businesses.integration.test.ts` run +concurrently against the same database, connect as a superuser (which bypasses +`FORCE ROW LEVEL SECURITY`), and the securities lookups carry no `owner_id` predicate because RLS is +what scopes them in production. So a lookup by ISIN sees every tenant's securities, and an assertion +on a whole result set's size assumes exclusive access to the database. Use synthetic per-suite ISINs +and assert on your own fixtures' buckets, not on totals. + ## Open decisions 1. **Audience strategy.** Accept the shared `https://api.accounter.com` audience for MCP and GraphQL diff --git a/packages/mcp-server/src/__tests__/mcp-e2e.test.ts b/packages/mcp-server/src/__tests__/mcp-e2e.test.ts index 0ef0259a3..37d41cf88 100644 --- a/packages/mcp-server/src/__tests__/mcp-e2e.test.ts +++ b/packages/mcp-server/src/__tests__/mcp-e2e.test.ts @@ -107,6 +107,64 @@ function upstreamData(query: string, authorization?: string): unknown { ], }; } + if (query.includes('securityHoldings')) { + return { + securityHoldings: [ + { + id: 'sec-biz-1', + security: { + ownerId: AUTHORIZED_BUSINESS, + isin: 'US67066G1040', + symbol: 'NVDA', + engName: 'NVIDIA Corp', + hebName: null, + exchange: 'NASDAQ', + currencyCode: 'USD', + isEtf: false, + identifiers: [{ type: 'POALIM_SECURITY_KEY', value: '1177423' }], + }, + position: { + quantity: 400, + averageCost: { raw: 100, formatted: '100.00', currency: 'USD' }, + totalBought: { raw: 40000, formatted: '40,000.00', currency: 'USD' }, + totalSold: { raw: 0, formatted: '0.00', currency: 'USD' }, + historyStartDate: '2023-02-02', + lastExecutionDate: '2026-06-01', + }, + }, + ], + }; + } + if (query.includes('securityExecutions')) { + return { + securityExecutions: { + pageInfo: { totalPages: 1, totalRecords: 1 }, + nodes: [ + { + securityBusiness: { + id: 'sec-biz-1', + ownerId: AUTHORIZED_BUSINESS, + isin: 'US67066G1040', + symbol: 'NVDA', + }, + execution: { + id: 'exec-1', + tradeDate: '2026-05-01', + valueDate: '2026-05-03', + tradeType: 'BUY', + transactionType: 'BUY', + paymentType: null, + quantity: 10, + tradePrice: 100, + netValue: { raw: -1000, formatted: '-1,000.00', currency: 'USD' }, + tradeCommission: { raw: 5, formatted: '5.00', currency: 'USD' }, + israelTaxValue: null, + }, + }, + ], + }, + }; + } if (query.includes('transactionsForBalanceReport')) { return { transactionsForBalanceReport: [ @@ -288,6 +346,8 @@ describe('authenticated tool invocation', () => { 'accounter_list_tags', 'accounter_list_tax_categories', 'accounter_balance_report', + 'accounter_list_security_holdings', + 'accounter_get_security_executions', ]), ); // Discovery leads the list, and the internal smoke tool is not advertised. @@ -329,6 +389,52 @@ describe('authenticated tool invocation', () => { expect(scope).toEqual({ memberBusinessIds: [AUTHORIZED_BUSINESS] }); }); + it('runs the securities portfolio over the wire, subtotalled and caveated', async () => { + const result = await callTool('accounter_list_security_holdings', {}, 'owner-token'); + expect(result.isError).toBeUndefined(); + const { holdings, byCurrency, caveats, scope } = result.structuredContent as { + holdings: Array<{ securityBusinessId: string; ownerId: string; isin: string }>; + byCurrency: Array<{ currency: string; securityCount: number; totalBought: number }>; + caveats: string[]; + scope: { memberBusinessIds: string[] }; + }; + + expect(holdings).toHaveLength(1); + expect(holdings[0].securityBusinessId).toBe('sec-biz-1'); + expect(holdings[0].ownerId).toBe(AUTHORIZED_BUSINESS); + expect(holdings[0].isin).toBe('US67066G1040'); + // The only valid total, and the reason it is computed server-side. + expect(byCurrency).toEqual([ + { currency: 'USD', securityCount: 1, totalBought: 40000, totalSold: 0 }, + ]); + expect(caveats.length).toBeGreaterThan(0); + expect(scope).toEqual({ memberBusinessIds: [AUTHORIZED_BUSINESS] }); + }); + + it('runs the securities execution history over the wire, 1-based', async () => { + const result = await callTool( + 'accounter_get_security_executions', + { isins: ['US67066G1040'] }, + 'owner-token', + ); + expect(result.isError).toBeUndefined(); + const { executions, pagination, scope } = result.structuredContent as { + executions: Array<{ executionId: string; securityBusinessId: string; tradeType: string }>; + pagination: { page: number; totalPages: number; hasNextPage: boolean }; + scope: { memberBusinessIds: string[] }; + }; + + expect(executions).toHaveLength(1); + expect(executions[0]).toMatchObject({ + executionId: 'exec-1', + securityBusinessId: 'sec-biz-1', + tradeType: 'BUY', + }); + // The tool speaks 1-based pages even though upstream is 0-based. + expect(pagination).toMatchObject({ page: 1, totalPages: 1, hasNextPage: false }); + expect(scope).toEqual({ memberBusinessIds: [AUTHORIZED_BUSINESS] }); + }); + it('forwards x-business-scope on tool calls but never on the membership bootstrap', async () => { forwardedScopes.length = 0; await callTool('accounter_list_tags', {}, 'owner-token'); 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 92403a019..eca2329af 100644 --- a/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts +++ b/packages/mcp-server/src/tools/__tests__/detail-tools.test.ts @@ -218,10 +218,171 @@ describe('getChargesTool', () => { const client = clientReturning(chargeFixture, body => (sentBody = body)); await run(getChargesTool, client, authContext([B1]), { chargeIds: ['c1'] }); const variables = ( - sentBody as { variables: { includeTransactions: boolean; includeDocuments: boolean } } + sentBody as { + variables: { + includeTransactions: boolean; + includeDocuments: boolean; + includeSecurities: boolean; + }; + } ).variables; expect(variables.includeTransactions).toBe(false); expect(variables.includeDocuments).toBe(false); + expect(variables.includeSecurities).toBe(false); + }); + + describe('includeSecurities', () => { + /** + * A foreign-securities charge carrying one security: a key whose reference row + * was ingested, and one buy execution. + */ + const securitiesChargeFixture = { + chargesByIDs: [ + { + ...chargeFixture.chargesByIDs[0], + id: 'sec-charge', + __typename: 'ForeignSecuritiesCharge', + securities: [ + { + securityKey: '1177423', + securityBusiness: { + id: 'sb-big', + ownerId: B1, + isin: 'US67066G1040', + symbol: 'NVDA', + engName: 'NVIDIA Corp', + }, + details: { + key: '1177423', + engName: 'NVIDIA CORP', + symbol: 'NVDA', + itemType: 'STOCK', + exchange: 'NASDAQ', + currencyCode: 'דולר ארה"ב', + asOfDate: '2026-06-01T00:00:00.000Z', + }, + executions: [ + { + id: 'ex-1', + tradeDate: '2026-05-01', + valueDate: '2026-05-03', + tradeType: 'BUY', + transactionType: 'BUY', + paymentType: null, + quantity: 10, + tradePrice: 100, + netValue: { raw: -1000, formatted: '$-1,000.00', currency: 'USD' }, + tradeCommission: { raw: 5, formatted: '$5.00', currency: 'USD' }, + israelTaxValue: null, + }, + ], + }, + ], + }, + ], + }; + + it('is off by default and adds nothing to the payload', async () => { + const result = await run(getChargesTool, clientReturning(chargeFixture), authContext([B1]), { + chargeIds: ['c1'], + }); + const charge = (result.structuredContent as { charges: Array> }) + .charges[0]!; + expect(charge).not.toHaveProperty('securities'); + }); + + it('selects the securities block when asked', async () => { + let sentBody: unknown; + const client = clientReturning(securitiesChargeFixture, body => (sentBody = body)); + await run(getChargesTool, client, authContext([B1]), { + chargeIds: ['sec-charge'], + includeSecurities: true, + }); + const sent = sentBody as { variables: { includeSecurities: boolean }; query: string }; + expect(sent.variables.includeSecurities).toBe(true); + expect(sent.query).toContain('... on ForeignSecuritiesCharge'); + expect(sent.query).toContain('securities @include(if: $includeSecurities)'); + }); + + it('reports the security, its business id and its executions', async () => { + const result = await run( + getChargesTool, + clientReturning(securitiesChargeFixture), + authContext([B1]), + { chargeIds: ['sec-charge'], includeSecurities: true }, + ); + const charge = ( + result.structuredContent as { + charges: Array<{ securities: Array> }>; + } + ).charges[0]!; + + expect(charge.securities).toHaveLength(1); + expect(charge.securities[0]).toMatchObject({ + securityKey: '1177423', + // The id the other two securities tools are addressed by, which is the + // whole point of carrying the security business here. + securityBusinessId: 'sb-big', + isin: 'US67066G1040', + symbol: 'NVDA', + name: 'NVIDIA Corp', + referenceFound: true, + }); + expect( + (charge.securities[0]!.executions as Array>)[0], + ).toMatchObject({ + executionId: 'ex-1', + tradeType: 'BUY', + quantity: 10, + }); + }); + + it('says so when the reference feed has no row for a traded key', async () => { + const fixture = structuredClone(securitiesChargeFixture); + (fixture.chargesByIDs[0]!.securities[0] as Record).details = null; + + const result = await run(getChargesTool, clientReturning(fixture), authContext([B1]), { + chargeIds: ['sec-charge'], + includeSecurities: true, + }); + const security = ( + result.structuredContent as { + charges: Array<{ securities: Array> }>; + } + ).charges[0]!.securities[0]!; + + // A stale scrape must be visible rather than looking like an absent + // security: the key and the security business survive. + expect(security.referenceFound).toBe(false); + expect(security.securityKey).toBe('1177423'); + expect(security.symbol).toBe('NVDA'); + expect(security.exchange).toBeNull(); + }); + + it('distinguishes "no keys resolved" from "not a securities charge"', async () => { + const fixture = structuredClone(securitiesChargeFixture); + fixture.chargesByIDs[0]!.securities = []; + + const result = await run(getChargesTool, clientReturning(fixture), authContext([B1]), { + chargeIds: ['sec-charge'], + includeSecurities: true, + }); + const charge = (result.structuredContent as { charges: Array> }) + .charges[0]!; + // Present and empty, not absent. + expect(charge.securities).toEqual([]); + }); + + it('leaves a non-securities charge untouched even when asked', async () => { + const result = await run(getChargesTool, clientReturning(chargeFixture), authContext([B1]), { + chargeIds: ['c1'], + includeSecurities: true, + }); + const charge = (result.structuredContent as { charges: Array> }) + .charges[0]!; + expect(charge).not.toHaveProperty('securities'); + expect(charge.id).toBe('c1'); + }); }); it('forwards all available filters to allCharges', async () => { diff --git a/packages/mcp-server/src/tools/__tests__/scope-contract.test.ts b/packages/mcp-server/src/tools/__tests__/scope-contract.test.ts index b0fc47013..2088a0fed 100644 --- a/packages/mcp-server/src/tools/__tests__/scope-contract.test.ts +++ b/packages/mcp-server/src/tools/__tests__/scope-contract.test.ts @@ -12,6 +12,7 @@ import { getLedgerRecordsTool } from '../ledger.js'; import { listBusinessesTool, listTagsTool, listTaxCategoriesTool } from '../lookups.js'; import type { ToolExecutionContext, ToolResult } from '../registry.js'; import { balanceReportTool } from '../reports.js'; +import { getSecurityExecutionsTool, listSecurityHoldingsTool } from '../securities.js'; import { getTransactionsTool } from '../transaction-details.js'; import { SCOPE_DESCRIPTION_SUFFIX, @@ -56,6 +57,8 @@ const BUSINESS_SCOPED_TOOLS = [ searchChargesTool, getLedgerRecordsTool, getContractsTool, + listSecurityHoldingsTool, + getSecurityExecutionsTool, listTagsTool, listTaxCategoriesTool, listBusinessesTool, @@ -70,6 +73,8 @@ const MULTI_BUSINESS_TOOLS = [ searchChargesTool, getLedgerRecordsTool, getContractsTool, + listSecurityHoldingsTool, + getSecurityExecutionsTool, listTagsTool, listTaxCategoriesTool, ]; 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 a3d8ad782..53eb67018 100644 --- a/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts +++ b/packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts @@ -88,6 +88,10 @@ function dataFor(query: string): unknown { if (query.includes('transactionsByIDs')) return { transactionsByIDs: [] }; if (query.includes('documentsByIds')) return { documentsByIds: [] }; if (query.includes('allTags')) return { allTags: [] }; + if (query.includes('securityHoldings')) return { securityHoldings: [] }; + if (query.includes('securityExecutions')) { + return { securityExecutions: { nodes: [], pageInfo: { totalPages: 0, totalRecords: 0 } } }; + } if (query.includes('taxCategories')) return { taxCategories: [] }; if (query.includes('allBusinesses')) return { allBusinesses: { nodes: [] } }; if (query.includes('transactionsForBalanceReport')) return { transactionsForBalanceReport: [] }; diff --git a/packages/mcp-server/src/tools/__tests__/securities.test.ts b/packages/mcp-server/src/tools/__tests__/securities.test.ts new file mode 100644 index 000000000..cb0ceb3c9 --- /dev/null +++ b/packages/mcp-server/src/tools/__tests__/securities.test.ts @@ -0,0 +1,675 @@ +import { describe, expect, it, vi } from 'vitest'; +import { buildAuthContext, type McpAuthContext } from '../../auth/identity.js'; +import type { AuthPrincipal } from '../../auth/token.js'; +import { BUSINESS_SCOPE_HEADER, UpstreamGraphQLClient } from '../../upstream/graphql-client.js'; +import { executeRegisteredTool } from '../execute.js'; +import { MAX_TOOL_RESULT_BYTES } from '../output.js'; +import type { ToolDefinition } from '../registry.js'; +import { + DEFAULT_SECURITY_HOLDINGS, + getSecurityExecutionsTool, + listSecurityHoldingsTool, + SECURITIES_CAVEATS, +} from '../securities.js'; + +const B1 = 'aa000000-0000-4000-8000-000000000001'; +const B2 = 'aa000000-0000-4000-8000-000000000002'; + +function authContext(memberBusinessIds: string[] = [B1]): 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, + memberBusinessIds.map(memberBusinessId => ({ memberBusinessId, roleId: 'accountant' })), + ); +} + +function clientReturning(data: unknown, capture?: (init: RequestInit) => void) { + const fetchImpl = vi.fn(async (_url: string, init: RequestInit) => { + capture?.(init); + 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, + }); +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const runTool = (tool: ToolDefinition, client: UpstreamGraphQLClient, rawArgs: unknown, auth = authContext()) => + executeRegisteredTool({ + tool, + rawArgs, + auth, + correlationId: 'c', + client, + authorization: 'Bearer t', + }); + +const amount = (raw: number, currency: string) => ({ + raw, + formatted: `${raw} ${currency}`, + currency, +}); + +/** + * Three securities chosen so ordering, search and currency grouping all have + * something to bite on: a big USD position, a small USD one, and an ILS one whose + * quantity is negative (a history that starts mid-life). + */ +function holdingsPayload() { + return { + securityHoldings: [ + { + id: 'sb-small', + security: { + ownerId: B1, + isin: 'US0378331005', + symbol: 'AAPL', + engName: 'Apple Inc', + hebName: null, + exchange: 'NASDAQ', + currencyCode: 'USD', + isEtf: false, + identifiers: [{ type: 'POALIM_SECURITY_KEY', value: '5129523' }], + }, + position: { + quantity: 12, + averageCost: amount(150, 'USD'), + totalBought: amount(1800, 'USD'), + totalSold: amount(0, 'USD'), + historyStartDate: '2024-01-05', + lastExecutionDate: '2026-05-01', + }, + }, + { + id: 'sb-big', + security: { + ownerId: B1, + isin: 'US67066G1040', + symbol: 'NVDA', + engName: 'NVIDIA Corp', + hebName: null, + exchange: 'NASDAQ', + currencyCode: 'USD', + isEtf: false, + identifiers: [{ type: 'POALIM_SECURITY_KEY', value: '1177423' }], + }, + position: { + quantity: 400, + averageCost: amount(100, 'USD'), + totalBought: amount(40_000, 'USD'), + totalSold: amount(2500, 'USD'), + historyStartDate: '2023-02-02', + lastExecutionDate: '2026-06-01', + }, + }, + { + id: 'sb-negative', + security: { + ownerId: B2, + isin: 'IL0010811143', + symbol: 'TEVA', + engName: null, + hebName: 'טבע', + exchange: 'TASE', + currencyCode: 'ILS', + isEtf: false, + identifiers: [{ type: 'POALIM_SECURITY_KEY', value: '6290011' }], + }, + position: { + quantity: -80, + averageCost: null, + totalBought: null, + totalSold: amount(9000, 'ILS'), + historyStartDate: '2025-03-03', + lastExecutionDate: '2026-01-01', + }, + }, + ], + }; +} + +interface HoldingsStructured { + holdings: Array<{ + securityBusinessId: string; + ownerId: string; + isin: string; + name: string; + quantity: number; + totalBought: { value: number; currency: string } | null; + }>; + byCurrency: Array<{ + currency: string; + securityCount: number; + totalBought: number; + totalSold: number; + }>; + securitiesWithNoCurrency: number; + caveats: readonly string[]; + totalCount: number; + returnedCount: number; + truncated: boolean; + continuation?: { reason: string }; + scope: { memberBusinessIds: string[] }; +} + +describe('accounter_list_security_holdings', () => { + it('orders by absolute quantity, so the biggest live position leads', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), {}); + const structured = result.structuredContent as unknown as HoldingsStructured; + + // 400, then |-80|, then 12 — the negative position sorts on its magnitude + // rather than being pushed to the bottom by its sign. + expect(structured.holdings.map(holding => holding.securityBusinessId)).toEqual([ + 'sb-big', + 'sb-negative', + 'sb-small', + ]); + }); + + it('tags every row with its owner, so a two-business answer is visible as one', async () => { + const result = await runTool( + listSecurityHoldingsTool, + clientReturning(holdingsPayload()), + {}, + authContext([B1, B2]), + ); + const structured = result.structuredContent as unknown as HoldingsStructured; + + expect(structured.holdings.map(holding => holding.ownerId).sort()).toEqual([B1, B1, B2]); + expect(structured.scope).toEqual({ memberBusinessIds: [B1, B2] }); + }); + + it('forwards the resolved scope as x-business-scope', async () => { + let seen: Record | undefined; + const client = clientReturning(holdingsPayload(), init => { + seen = init.headers as Record; + }); + await runTool(listSecurityHoldingsTool, client, {}, authContext([B1, B2])); + expect(seen?.[BUSINESS_SCOPE_HEADER]).toBe(`${B1},${B2}`); + }); + + it('narrows to a requested business', async () => { + let seen: Record | undefined; + const client = clientReturning(holdingsPayload(), init => { + seen = init.headers as Record; + }); + const result = await runTool( + listSecurityHoldingsTool, + client, + { memberBusinessIds: [B2] }, + authContext([B1, B2]), + ); + expect(seen?.[BUSINESS_SCOPE_HEADER]).toBe(B2); + expect( + (result.structuredContent as unknown as HoldingsStructured).scope.memberBusinessIds, + ).toEqual([B2]); + }); + + it('rejects a business the caller is not a member of', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + memberBusinessIds: ['aa000000-0000-4000-8000-000000000009'], + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('AUTHORIZATION_ERROR'); + }); + + it.each([ + ['english name', 'nvidia', ['sb-big']], + ['hebrew name', 'טבע', ['sb-negative']], + ['symbol', 'aapl', ['sb-small']], + ['isin', 'US67066G1040', ['sb-big']], + ['exchange', 'tase', ['sb-negative']], + ['currency', 'usd', ['sb-big', 'sb-small']], + ['poalim security key', '6290011', ['sb-negative']], + ])('searches by %s', async (_what, search, expected) => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + search, + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + expect(structured.holdings.map(holding => holding.securityBusinessId)).toEqual(expected); + expect(structured.totalCount).toBe(expected.length); + }); + + it('passes includeClosed through rather than filtering locally', async () => { + let body: string | undefined; + const client = clientReturning(holdingsPayload(), init => { + body = init.body as string; + }); + await runTool(listSecurityHoldingsTool, client, { includeClosed: true }); + expect(JSON.parse(body!).variables).toEqual({ includeClosed: true }); + }); + + it('subtotals per currency and never across them', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), {}); + const structured = result.structuredContent as unknown as HoldingsStructured; + + expect(structured.byCurrency).toEqual([ + { currency: 'USD', securityCount: 2, totalBought: 41_800, totalSold: 2500 }, + { currency: 'ILS', securityCount: 1, totalBought: 0, totalSold: 9000 }, + ]); + // No cross-currency total exists anywhere in the payload. + expect(structured).not.toHaveProperty('totalBought'); + }); + + it('counts securities with nothing ingested rather than bucketing them', async () => { + const payload = holdingsPayload(); + payload.securityHoldings.push({ + id: 'sb-empty', + security: { + ownerId: B1, + isin: 'US0000000000', + symbol: null, + engName: 'Never Traded', + hebName: null, + exchange: null, + currencyCode: null, + isEtf: false, + identifiers: [], + }, + position: { + quantity: 0, + averageCost: null, + totalBought: null, + totalSold: null, + historyStartDate: null, + lastExecutionDate: null, + }, + } as (typeof payload.securityHoldings)[number]); + + const result = await runTool(listSecurityHoldingsTool, clientReturning(payload), { + includeClosed: true, + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + + expect(structured.securitiesWithNoCurrency).toBe(1); + expect(structured.byCurrency.map(bucket => bucket.currency)).toEqual(['USD', 'ILS']); + }); + + /** + * The reference currency is known for a security nothing was ever traded of, so treating it as + * evidence of an amount would put that security in a bucket with a 0/0 subtotal — the exact + * null-means-nothing-ingested-not-zero confusion the caveats warn about. + */ + it('does not bucket a security by its reference currency alone', async () => { + const payload = holdingsPayload(); + payload.securityHoldings.push({ + id: 'sb-never-traded', + security: { + ownerId: B1, + isin: 'US1111111111', + symbol: 'NONE', + engName: 'Never Traded', + hebName: null, + exchange: 'NASDAQ', + // Known from the reference feed even though nothing was ever bought or sold. + currencyCode: 'USD', + isEtf: false, + identifiers: [], + }, + position: { + quantity: 0, + averageCost: null, + totalBought: null, + totalSold: null, + historyStartDate: null, + lastExecutionDate: null, + }, + } as (typeof payload.securityHoldings)[number]); + + const result = await runTool(listSecurityHoldingsTool, clientReturning(payload), { + includeClosed: true, + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + + expect(structured.securitiesWithNoCurrency).toBe(1); + const usd = structured.byCurrency.find(bucket => bucket.currency === 'USD')!; + // Still just the two securities that actually traded in USD. + expect(usd.securityCount).toBe(2); + expect(usd.totalBought).toBe(41_800); + }); + + it('trims the search, as the web screen does', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + search: ' nvidia ', + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + expect(structured.holdings.map(holding => holding.securityBusinessId)).toEqual(['sb-big']); + }); + + it('treats an all-whitespace search as no search', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + search: ' ', + }); + expect((result.structuredContent as unknown as HoldingsStructured).totalCount).toBe(3); + }); + + it('falls back through the name to the ISIN, so a row is never nameless', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + search: 'טבע', + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + // No engName on this one, so the Hebrew name is used. + expect(structured.holdings[0]!.name).toBe('טבע'); + }); + + it('carries the caveats on the wire, not only in the description', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), {}); + const structured = result.structuredContent as unknown as HoldingsStructured; + expect(structured.caveats).toEqual(SECURITIES_CAVEATS); + expect(structured.caveats.join(' ')).toMatch(/never sum across currencies/i); + }); + + it('caps rows at the limit while reporting the full match count', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + limit: 2, + }); + const structured = result.structuredContent as unknown as HoldingsStructured; + expect(structured.returnedCount).toBe(2); + expect(structured.totalCount).toBe(3); + expect(structured.truncated).toBe(true); + }); + + it('rejects a limit above the cap rather than silently clamping', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + limit: 10_000, + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('VALIDATION_ERROR'); + }); + + it('rejects unknown input fields', async () => { + const result = await runTool(listSecurityHoldingsTool, clientReturning(holdingsPayload()), { + includeClosedPositions: true, + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('VALIDATION_ERROR'); + }); + + /** + * The byte guard is what stands between a large portfolio and an invalid + * response. A default-limit page of the widest plausible rows must still come + * back as valid JSON with the shortfall declared. + */ + it('degrades a large portfolio to valid JSON with a continuation hint', async () => { + const securityHoldings = Array.from({ length: 400 }, (_unused, index) => ({ + id: `sb-${index}`.padEnd(36, '0'), + security: { + ownerId: B1, + isin: `IL00108111${String(index).padStart(4, '0')}`, + symbol: `SYMBOL${index}`, + engName: `A Rather Long Security Name Number ${index}`, + hebName: `שם ארוך של נייר ערך מספר ${index}`, + exchange: 'NASDAQ', + currencyCode: 'USD', + isEtf: false, + identifiers: [{ type: 'POALIM_SECURITY_KEY', value: String(1_000_000 + index) }], + }, + position: { + quantity: index + 1, + averageCost: amount(123.456, 'USD'), + totalBought: amount(98_765.43, 'USD'), + totalSold: amount(1234.56, 'USD'), + historyStartDate: '2023-01-01', + lastExecutionDate: '2026-06-01', + }, + })); + + const result = await runTool( + listSecurityHoldingsTool, + clientReturning({ securityHoldings }), + {}, + ); + const structured = result.structuredContent as unknown as HoldingsStructured; + + expect(() => JSON.stringify(structured)).not.toThrow(); + expect(JSON.stringify(structured).length).toBeLessThanOrEqual(MAX_TOOL_RESULT_BYTES); + expect(structured.totalCount).toBe(400); + expect(structured.truncated).toBe(true); + expect(structured.continuation?.reason).toBeTruthy(); + // The row cap bites before the byte guard does, so rows are dropped by the + // limit rather than mid-object. + expect(structured.returnedCount).toBeLessThanOrEqual(DEFAULT_SECURITY_HOLDINGS); + // Subtotals describe every match, not just the returned page. + expect(structured.byCurrency[0]!.securityCount).toBe(400); + }); +}); + +// --------------------------------------------------------------------------- + +function executionsPayload(overrides: { totalRecords?: number; totalPages?: number } = {}) { + return { + securityExecutions: { + pageInfo: { + totalPages: overrides.totalPages ?? 3, + totalRecords: overrides.totalRecords ?? 250, + }, + nodes: [ + { + securityBusiness: { id: 'sb-big', ownerId: B1, isin: 'US67066G1040', symbol: 'NVDA' }, + execution: { + id: 'ex-2', + tradeDate: '2026-06-01', + valueDate: '2026-06-03', + tradeType: 'SELL', + transactionType: 'SELL', + paymentType: null, + quantity: 10, + tradePrice: 250, + netValue: amount(2500, 'USD'), + tradeCommission: amount(5, 'USD'), + israelTaxValue: amount(90, 'ILS'), + }, + }, + { + securityBusiness: { id: 'sb-big', ownerId: B1, isin: 'US67066G1040', symbol: 'NVDA' }, + execution: { + id: 'ex-1', + tradeDate: '2026-05-01', + valueDate: '2026-05-03', + tradeType: 'BUY', + transactionType: 'BUY', + paymentType: null, + quantity: 10, + tradePrice: 100, + netValue: amount(-1000, 'USD'), + tradeCommission: amount(5, 'USD'), + israelTaxValue: null, + }, + }, + ], + }, + }; +} + +interface ExecutionsStructured { + executions: Array>; + pagination: { page: number; pageSize: number; totalPages: number; hasNextPage: boolean }; + caveats: readonly string[]; + totalCount: number; + scope: { memberBusinessIds: string[] }; +} + +describe('accounter_get_security_executions', () => { + const variablesOf = (body: string) => JSON.parse(body).variables; + + it('translates its 1-based page to the 0-based upstream page', async () => { + let body: string | undefined; + const client = clientReturning(executionsPayload(), init => { + body = init.body as string; + }); + await runTool(getSecurityExecutionsTool, client, { page: 3, pageSize: 100 }); + expect(variablesOf(body!).page).toBe(2); + expect(variablesOf(body!).limit).toBe(100); + }); + + it('reports pagination in the tool 1-based terms', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + page: 2, + pageSize: 100, + }); + const structured = result.structuredContent as unknown as ExecutionsStructured; + expect(structured.pagination).toEqual({ + page: 2, + pageSize: 100, + totalPages: 3, + hasNextPage: true, + }); + expect(structured.totalCount).toBe(250); + }); + + it('knows when it is on the last page', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + page: 3, + }); + expect( + (result.structuredContent as unknown as ExecutionsStructured).pagination.hasNextPage, + ).toBe(false); + }); + + it('forwards the identity filters as a union, untouched', async () => { + let body: string | undefined; + const client = clientReturning(executionsPayload(), init => { + body = init.body as string; + }); + await runTool(getSecurityExecutionsTool, client, { + isins: ['US67066G1040'], + symbols: ['AAPL'], + tradeTypes: ['SELL'], + fromTradeDate: '2026-01-01', + toTradeDate: '2026-12-31', + }); + expect(variablesOf(body!).filters).toMatchObject({ + isins: ['US67066G1040'], + symbols: ['AAPL'], + tradeTypes: ['SELL'], + fromTradeDate: '2026-01-01', + toTradeDate: '2026-12-31', + }); + }); + + it('tags rows with the security and its owner', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), {}); + const structured = result.structuredContent as unknown as ExecutionsStructured; + expect(structured.executions[0]).toMatchObject({ + executionId: 'ex-2', + securityBusinessId: 'sb-big', + ownerId: B1, + isin: 'US67066G1040', + tradeType: 'SELL', + }); + }); + + it('omits the charge fields entirely when links were not requested', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), {}); + const structured = result.structuredContent as unknown as ExecutionsStructured; + // Absent, not null: "not asked for" must stay distinguishable from + // "asked for, nothing matched". + expect(structured.executions[0]).not.toHaveProperty('chargeId'); + expect(structured.executions[0]).not.toHaveProperty('transactionId'); + }); + + it('reports a null chargeId when links were requested and nothing matched', async () => { + const payload = executionsPayload(); + const nodes = payload.securityExecutions.nodes as Array>; + nodes[0]!.charge = { id: 'charge-1' }; + nodes[0]!.transaction = { id: 'tx-1' }; + nodes[1]!.charge = null; + nodes[1]!.transaction = null; + + const result = await runTool(getSecurityExecutionsTool, clientReturning(payload), { + isins: ['US67066G1040'], + includeCharges: true, + }); + const structured = result.structuredContent as unknown as ExecutionsStructured; + expect(structured.executions[0]).toMatchObject({ chargeId: 'charge-1', transactionId: 'tx-1' }); + expect(structured.executions[1]).toMatchObject({ chargeId: null, transactionId: null }); + }); + + /** + * Upstream also refuses this, but as an UPSTREAM_ERROR — which reads as a + * transient server fault worth retrying, when the call was simply malformed. + * Catching it here makes the failure say what to add. + */ + it('refuses includeCharges without naming securities, as a validation error', async () => { + const client = clientReturning(executionsPayload()); + const result = await runTool(getSecurityExecutionsTool, client, { includeCharges: true }); + expect(result.isError).toBe(true); + const structured = result.structuredContent as { code: string; message: string }; + expect(structured.code).toBe('VALIDATION_ERROR'); + expect(structured.message).toMatch(/securityBusinessIds, isins or symbols/); + }); + + it.each([['securityBusinessIds'], ['isins'], ['symbols']])( + 'accepts includeCharges when %s names the securities', + async field => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + [field]: ['x'], + includeCharges: true, + }); + expect(result.isError).toBeUndefined(); + }, + ); + + it('rejects an impossible calendar date that passes the format check', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + fromTradeDate: '2026-02-31', + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { message: string }).message).toMatch(/fromTradeDate/); + }); + + it('rejects an inverted date range', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + fromTradeDate: '2026-06-01', + toTradeDate: '2026-01-01', + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { message: string }).message).toMatch( + /on or before toTradeDate/, + ); + }); + + it('rejects a trade type outside the bank vocabulary', async () => { + const result = await runTool(getSecurityExecutionsTool, clientReturning(executionsPayload()), { + tradeTypes: ['SHORT_SELL'], + }); + expect(result.isError).toBe(true); + expect((result.structuredContent as { code: string }).code).toBe('VALIDATION_ERROR'); + }); + + it('carries the caveats and the echoed scope', async () => { + const result = await runTool( + getSecurityExecutionsTool, + clientReturning(executionsPayload()), + {}, + authContext([B1, B2]), + ); + const structured = result.structuredContent as unknown as ExecutionsStructured; + expect(structured.caveats).toEqual(SECURITIES_CAVEATS); + expect(structured.scope).toEqual({ memberBusinessIds: [B1, B2] }); + }); + + it('reports an empty result without inventing a page', async () => { + const result = await runTool( + getSecurityExecutionsTool, + clientReturning({ + securityExecutions: { pageInfo: { totalPages: 0, totalRecords: 0 }, nodes: [] }, + }), + {}, + ); + const structured = result.structuredContent as unknown as ExecutionsStructured; + expect(structured.totalCount).toBe(0); + expect(structured.pagination.hasNextPage).toBe(false); + expect(result.content[0]!.text).toMatch(/No executions matched/); + }); +}); diff --git a/packages/mcp-server/src/tools/__tests__/usage-log.test.ts b/packages/mcp-server/src/tools/__tests__/usage-log.test.ts index 5c6dfa3c7..2cd62fc1c 100644 --- a/packages/mcp-server/src/tools/__tests__/usage-log.test.ts +++ b/packages/mcp-server/src/tools/__tests__/usage-log.test.ts @@ -56,6 +56,10 @@ function dataFor(query: string): unknown { if (query.includes('transactionsByIDs')) return { transactionsByIDs: [] }; if (query.includes('documentsByIds')) return { documentsByIds: [] }; if (query.includes('allTags')) return { allTags: [] }; + if (query.includes('securityHoldings')) return { securityHoldings: [] }; + if (query.includes('securityExecutions')) { + return { securityExecutions: { nodes: [], pageInfo: { totalPages: 0, totalRecords: 0 } } }; + } if (query.includes('batchUpdateChargesTags')) { return { batchUpdateChargesTags: { diff --git a/packages/mcp-server/src/tools/charge-details.ts b/packages/mcp-server/src/tools/charge-details.ts index 9f7e51e9d..e4f341534 100644 --- a/packages/mcp-server/src/tools/charge-details.ts +++ b/packages/mcp-server/src/tools/charge-details.ts @@ -101,6 +101,18 @@ const getChargesInput = z 'Include each charge’s linked documents (default false — opt in only when you need the ' + 'individual invoices/receipts).', ), + includeSecurities: z + .boolean() + .optional() + .default(false) + .describe( + 'For foreign-securities charges, include the security traded and the portfolio executions ' + + 'behind the cash movement (default false). Only such charges carry it; every other type is ' + + 'unaffected. Each security also reports its `securityBusinessId`, which is what ' + + 'accounter_list_security_holdings and accounter_get_security_executions are addressed by. ' + + 'Nesting executions multiplies the payload, so pair it with explicit `chargeIds` or a small ' + + '`pageSize`.', + ), }) .superRefine((value, context) => { const hasIds = value.chargeIds !== undefined && value.chargeIds.length > 0; @@ -279,12 +291,59 @@ const CHARGES_QUERY_DOCUMENT = /* GraphQL */ ` additionalDocuments @include(if: $includeDocuments) { ...McpChargeDetailDocumentFields } + ... on ForeignSecuritiesCharge { + securities @include(if: $includeSecurities) { + securityKey + securityBusiness { + id + ownerId + isin + symbol + engName + } + details { + key + engName + symbol + itemType + exchange + currencyCode + asOfDate + } + executions { + id + tradeDate + valueDate + tradeType + transactionType + paymentType + quantity + tradePrice + netValue { + raw + formatted + currency + } + tradeCommission { + raw + formatted + currency + } + israelTaxValue { + raw + formatted + currency + } + } + } + } } query McpGetCharges( $chargeIDs: [UUID!]! $includeTransactions: Boolean! $includeDocuments: Boolean! + $includeSecurities: Boolean! ) { chargesByIDs(chargeIDs: $chargeIDs) { ...McpChargeDetailFields @@ -297,6 +356,7 @@ const CHARGES_QUERY_DOCUMENT = /* GraphQL */ ` $limit: Int! $includeTransactions: Boolean! $includeDocuments: Boolean! + $includeSecurities: Boolean! ) { allCharges(filters: $filters, page: $page, limit: $limit) { nodes { @@ -323,6 +383,22 @@ function isNotFoundByIdUpstreamError(error: unknown): boolean { } type RawCharge = McpGetChargesQuery['chargesByIDs'][number]; +/** + * The securities block hangs off the `ForeignSecuritiesCharge` member of the union + * only — unlike `transactions` and `additionalDocuments`, which the `Charge` + * interface declares for every member — so it has to be narrowed rather than read. + */ +type RawSecuritiesCharge = Extract; +type RawChargeSecurity = NonNullable[number]; + +/** + * Undefined for any charge that is not a securities one, and for a securities + * charge fetched without `includeSecurities` — `@include(if:)` omits the field + * entirely, so both read as "not asked for", which is what the payload says. + */ +function rawChargeSecurities(charge: RawCharge): readonly RawChargeSecurity[] | undefined { + return 'securities' in charge ? charge.securities : undefined; +} interface NormalizedCharge { id: string; @@ -347,10 +423,50 @@ interface NormalizedCharge { metadata: Record | null; transactions: NormalizedTransaction[]; documents: NormalizedDocument[]; + /** Present only for a securities charge fetched with `includeSecurities`. */ + securities?: Array>; +} + +/** + * One security a charge's transaction descriptions referenced, plus the trades + * behind the cash movement. + * + * `details` is the ingested reference row and is legitimately null: the feed can + * be out of date, and saying so is more useful than dropping the key. Descriptors + * come from the security *business* where both have them — its `currencyCode` is + * the resolved `Currency` enum, while the reference feed's is a raw source string + * that can still be a Hebrew label. + */ +function normalizeChargeSecurity(security: RawChargeSecurity) { + return { + securityKey: security.securityKey, + securityBusinessId: security.securityBusiness?.id ?? null, + isin: security.securityBusiness?.isin ?? null, + symbol: security.securityBusiness?.symbol ?? security.details?.symbol ?? null, + name: security.securityBusiness?.engName ?? security.details?.engName ?? null, + exchange: security.details?.exchange ?? null, + itemType: security.details?.itemType ?? null, + referenceAsOf: security.details?.asOfDate ?? null, + referenceFound: security.details != null, + executions: security.executions.map(execution => ({ + executionId: execution.id, + tradeDate: execution.tradeDate, + valueDate: execution.valueDate, + tradeType: execution.tradeType, + transactionType: execution.transactionType, + paymentType: execution.paymentType, + quantity: execution.quantity, + tradePrice: execution.tradePrice, + netValue: normalizeAmount(execution.netValue), + tradeCommission: normalizeAmount(execution.tradeCommission), + israelTaxValue: normalizeAmount(execution.israelTaxValue), + })), + }; } function normalizeCharge(charge: RawCharge): NormalizedCharge { const owner = normalizeEntity(charge.owner); + const securities = rawChargeSecurities(charge); return { id: charge.id, description: charge.userDescription ?? null, @@ -375,6 +491,11 @@ function normalizeCharge(charge: RawCharge): NormalizedCharge { normalizeTransaction(raw as RawTransaction), ), documents: (charge.additionalDocuments ?? []).map(raw => normalizeDocument(raw as RawDocument)), + // Absent unless asked for and unless this charge is a securities one, so + // "not a securities charge" stays distinguishable from "an empty list of + // securities" — which is itself a real state, meaning the descriptions + // carried no key the ingested feed knows. + ...(securities === undefined ? {} : { securities: securities.map(normalizeChargeSecurity) }), }; } @@ -393,6 +514,7 @@ async function handler(input: GetChargesInput, context: ToolExecutionContext): P chargeIDs: input.chargeIds!, includeTransactions: input.includeTransactions, includeDocuments: input.includeDocuments, + includeSecurities: input.includeSecurities, } satisfies McpGetChargesQueryVariables, }, context.upstream, @@ -422,6 +544,7 @@ async function handler(input: GetChargesInput, context: ToolExecutionContext): P limit: input.pageSize, includeTransactions: input.includeTransactions, includeDocuments: input.includeDocuments, + includeSecurities: input.includeSecurities, } satisfies McpGetChargesByFiltersQueryVariables, }, context.upstream, @@ -487,7 +610,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 `chargeType`. Linked transactions and documents are opt-in via `includeTransactions` / `includeDocuments`. 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 and `chargeType`. Linked transactions and documents are opt-in via `includeTransactions` / `includeDocuments`, and for a foreign-securities charge the security traded and the portfolio executions behind it are opt-in via `includeSecurities` (each reporting a `securityBusinessId` that accounter_list_security_holdings and accounter_get_security_executions accept). Read-only. ' + SCOPE_DESCRIPTION_SUFFIX, inputSchema: getChargesInput, policy: { requiresBusinessScope: true, dataClassification: 'business' }, diff --git a/packages/mcp-server/src/tools/registry-instance.ts b/packages/mcp-server/src/tools/registry-instance.ts index 34ebec5b4..67b5361a4 100644 --- a/packages/mcp-server/src/tools/registry-instance.ts +++ b/packages/mcp-server/src/tools/registry-instance.ts @@ -8,6 +8,7 @@ import { getLedgerRecordsTool } from './ledger.js'; import { listBusinessesTool, listTagsTool, listTaxCategoriesTool } from './lookups.js'; import { ToolRegistry } from './registry.js'; import { balanceReportTool } from './reports.js'; +import { getSecurityExecutionsTool, listSecurityHoldingsTool } from './securities.js'; import { updateChargesTagsTool } from './tags-write.js'; import { explainTerminologyTool } from './terminology.js'; import { getTransactionsTool } from './transaction-details.js'; @@ -41,6 +42,11 @@ toolRegistry.register(getDocumentsTool); // charge/transaction/document drill-down rather than sitting with the lookups. toolRegistry.register(getLedgerRecordsTool); toolRegistry.register(getContractsTool); +// Securities are their own drill-down, not reference data: the portfolio is the +// entry point and the execution history is what it drills into, so the pair sits +// together after the charge/ledger chain and ahead of the lookups. +toolRegistry.register(listSecurityHoldingsTool); +toolRegistry.register(getSecurityExecutionsTool); 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/securities.ts b/packages/mcp-server/src/tools/securities.ts new file mode 100644 index 000000000..b185bcabd --- /dev/null +++ b/packages/mcp-server/src/tools/securities.ts @@ -0,0 +1,587 @@ +import { z } from 'zod'; +import { ToolInputError } from '../errors/taxonomy.js'; +import type { McpGetSecurityExecutionsQuery, McpListSecurityHoldingsQuery } from '../gql/index.js'; +import { parseCalendarDate, TIMELESS_DATE } from './dates.js'; +import { normalizeAmount } from './entity-shapes.js'; +import { shapeListResult } from './output.js'; +import type { ToolDefinition, ToolExecutionContext, ToolResult } from './registry.js'; +import { memberBusinessIdsInput, SCOPE_DESCRIPTION_SUFFIX } from './scope-input.js'; + +/** + * Securities: what the tenant holds, and the trades behind it. + * + * Two tools over the same domain. `accounter_list_security_holdings` is the + * portfolio — one row per security with the position its executions add up to. + * `accounter_get_security_executions` is the trade history, filtered and paged. + * + * Everything here is derived rather than reported, and the derivation has real + * limits (no bank balance, no market prices, per-security currencies that must + * never be added together). Those limits ride on every response as `caveats` + * rather than living only in the tool description: a model that reads the rows + * and not the schema still has to see them. + */ + +export const LIST_SECURITY_HOLDINGS_TOOL_NAME = 'accounter_list_security_holdings'; +export const GET_SECURITY_EXECUTIONS_TOOL_NAME = 'accounter_get_security_executions'; + +/** Hard cap on holdings rows returned in one call. */ +export const MAX_SECURITY_HOLDINGS = 300; +/** Default holdings rows — roughly what fits the result byte budget untruncated. */ +export const DEFAULT_SECURITY_HOLDINGS = 150; +/** Hard cap on executions per page. */ +export const MAX_SECURITY_EXECUTIONS_PAGE_SIZE = 200; +export const DEFAULT_SECURITY_EXECUTIONS_PAGE_SIZE = 100; +/** Hard cap on how many securities one identity filter may name. */ +export const MAX_REQUESTED_SECURITIES = 50; + +/** + * What is true of every number these tools return. + * + * Emitted on the wire, not just documented. The position is arithmetic over a + * scraped trade history, and each of these is a way that arithmetic can be read + * as more than it is. + */ +export const SECURITIES_CAVEATS = [ + 'Positions are derived by adding up ingested executions. The bank does not report a holding, so these are not a reported balance.', + 'Anything held before historyStartDate is not counted, and splits or corporate actions with no execution row are invisible.', + "Amounts are in each security's own trade currency and are never converted. Never sum across currencies, and never sum quantities or average costs at all.", + 'No market prices are available: current value and unrealized profit or loss are unknown.', + 'A negative quantity means the scraped history starts mid-life - a data-quality signal, not a short position. A null amount means nothing was ingested, not zero.', +] as const; + +// --------------------------------------------------------------------------- +// Holdings +// --------------------------------------------------------------------------- + +const listSecurityHoldingsInput = z.object({ + memberBusinessIds: memberBusinessIdsInput, + includeClosed: z + .boolean() + .optional() + .default(false) + .describe( + 'Include securities that were traded but are no longer held (sold out, or with nothing ingested against them). Off by default: closed positions are the bulk of a long-lived portfolio.', + ), + search: z + .string() + .min(1) + .max(120) + .optional() + .describe( + 'Case-insensitive substring matched against the security name (English or Hebrew), symbol, ISIN, exchange, currency and every source identifier.', + ), + limit: z + .number() + .int() + .positive() + .max(MAX_SECURITY_HOLDINGS) + .optional() + .default(DEFAULT_SECURITY_HOLDINGS) + .describe(`Maximum rows to return (capped at ${MAX_SECURITY_HOLDINGS}).`), +}); +type ListSecurityHoldingsInput = z.infer; + +const LIST_SECURITY_HOLDINGS_QUERY = /* GraphQL */ ` + query McpListSecurityHoldings($includeClosed: Boolean!) { + securityHoldings(includeClosed: $includeClosed) { + id + security { + ownerId + isin + symbol + engName + hebName + exchange + currencyCode + isEtf + identifiers { + type + value + } + } + position { + quantity + averageCost { + raw + formatted + currency + } + totalBought { + raw + formatted + currency + } + totalSold { + raw + formatted + currency + } + historyStartDate + lastExecutionDate + } + } + } +`; + +type RawHolding = McpListSecurityHoldingsQuery['securityHoldings'][number]; + +/** + * Every string the search matches, mirroring the web screen's own search so the + * two cannot drift: name in both languages, symbol, ISIN, exchange, currency and + * every source identifier (a Poalim key is how a trade is named in a bank + * statement, so it is a thing a caller will paste in). + */ +function searchableText(holding: RawHolding): string { + const { security } = holding; + return [ + security.engName, + security.hebName, + security.symbol, + security.isin, + security.exchange, + security.currencyCode, + ...security.identifiers.map(identifier => identifier.value), + ] + .filter(Boolean) + .join(' ') + .toLowerCase(); +} + +/** A name is never empty: the ISIN is the identity, so it is the last resort. */ +function holdingName(holding: RawHolding): string { + return holding.security.engName ?? holding.security.hebName ?? holding.security.isin; +} + +function normalizeHolding(holding: RawHolding) { + const { security, position } = holding; + return { + securityBusinessId: holding.id, + ownerId: security.ownerId, + isin: security.isin, + symbol: security.symbol, + name: holdingName(holding), + exchange: security.exchange, + currency: security.currencyCode, + isEtf: security.isEtf, + quantity: position.quantity, + averageCost: normalizeAmount(position.averageCost), + totalBought: normalizeAmount(position.totalBought), + totalSold: normalizeAmount(position.totalSold), + historyStartDate: position.historyStartDate, + lastExecutionDate: position.lastExecutionDate, + }; +} +type NormalizedHolding = ReturnType; + +const NAME_COLLATOR = new Intl.Collator('en', { sensitivity: 'base' }); + +/** + * Biggest live position first — the reason to ask for a portfolio at all, and the + * web screen's own default. + * + * `Math.abs` on purpose: a negative quantity is a history that starts mid-life, + * which is a large position badly recorded rather than a small one. Ties break + * on name then id so the order is stable across calls. + */ +function bySizeThenName(a: NormalizedHolding, b: NormalizedHolding): number { + return ( + Math.abs(b.quantity) - Math.abs(a.quantity) || + NAME_COLLATOR.compare(a.name, b.name) || + (a.securityBusinessId < b.securityBusinessId + ? -1 + : a.securityBusinessId > b.securityBusinessId + ? 1 + : 0) + ); +} + +/** Money summed to the cent; a float sum of trade values otherwise trails noise. */ +function roundMoney(value: number): number { + return Math.round(value * 100) / 100; +} + +/** + * The only aggregation these numbers support: a subtotal per trade currency. + * + * Asked what a portfolio is worth, a model will add a shekel column to a dollar + * one. Computing the sums that *are* valid, and only those, leaves nothing to + * mis-add — grouping is structural where a caveat is advisory. Quantities and + * average costs are deliberately absent: units of different instruments and + * per-unit prices do not add up even within one currency. + * + * A security with nothing ingested has no currency to report in, so it is + * counted separately rather than folded into an arbitrary bucket. + */ +function subtotalsByCurrency(holdings: readonly NormalizedHolding[]) { + const byCurrency = new Map(); + let securitiesWithNoCurrency = 0; + + for (const holding of holdings) { + // Only an actual amount counts. `holding.currency` is the security's static + // reference currency, which is known even for a security nothing was ever + // traded of — folding it in here would put that security in a bucket with a + // 0/0 subtotal, which is exactly the null-vs-zero confusion the caveats warn + // against. + const currency = + holding.totalBought?.currency ?? holding.totalSold?.currency ?? holding.averageCost?.currency; + if (!currency) { + securitiesWithNoCurrency += 1; + continue; + } + const bucket = byCurrency.get(currency) ?? { securityCount: 0, bought: 0, sold: 0 }; + bucket.securityCount += 1; + bucket.bought += holding.totalBought?.value ?? 0; + bucket.sold += holding.totalSold?.value ?? 0; + byCurrency.set(currency, bucket); + } + + return { + securitiesWithNoCurrency, + byCurrency: [...byCurrency.entries()] + .map(([currency, bucket]) => ({ + currency, + securityCount: bucket.securityCount, + totalBought: roundMoney(bucket.bought), + totalSold: roundMoney(bucket.sold), + })) + .sort((a, b) => b.securityCount - a.securityCount || a.currency.localeCompare(b.currency)), + }; +} + +async function listSecurityHoldingsHandler( + input: ListSecurityHoldingsInput, + context: ToolExecutionContext, +): Promise { + const data = await context.client.query( + { + query: LIST_SECURITY_HOLDINGS_QUERY, + variables: { includeClosed: input.includeClosed }, + }, + context.upstream, + ); + + // Upstream takes no search argument and a portfolio is tens to low hundreds of + // rows, so filtering here costs nothing and keeps the match rules identical to + // the web screen's. + // Trimmed as well as lowercased, matching the web screen's own normalization + // (`screens/securities/index.tsx`) — a pasted `" NVDA "` has to behave the same + // in both places for the parity this mirrors to be worth anything. + const needle = input.search?.trim().toLowerCase(); + const matched = needle + ? data.securityHoldings.filter(holding => searchableText(holding).includes(needle)) + : data.securityHoldings; + + const holdings = matched.map(normalizeHolding).sort(bySizeThenName); + // Subtotals cover every match, not just the page: a total that silently shrank + // with the row cap would be worse than no total. + const { byCurrency, securitiesWithNoCurrency } = subtotalsByCurrency(holdings); + + return shapeListResult({ + items: holdings.slice(0, input.limit), + itemsKey: 'holdings', + total: holdings.length, + extra: { + byCurrency, + securitiesWithNoCurrency, + caveats: SECURITIES_CAVEATS, + scope: { memberBusinessIds: context.readScope.memberBusinessIds }, + }, + summarize: (shown, total, truncated) => + total === 0 + ? 'No securities matched.' + : `${total} ${total === 1 ? 'security' : 'securities'}${input.includeClosed ? ' (including closed positions)' : ' currently held'}; showing ${shown}${truncated ? ' (truncated)' : ''}. Amounts are per-security trade currency - see byCurrency for the only valid subtotals.`, + }); +} + +export const listSecurityHoldingsTool: ToolDefinition = { + name: LIST_SECURITY_HOLDINGS_TOOL_NAME, + description: + 'List the securities portfolio: one row per security with units held, weighted average cost per unit bought, totals bought and sold, and the dates the ingested trade history spans. ' + + 'Positions are DERIVED by adding up scraped executions, not read from a bank balance, and there are no market prices - so current value and unrealized profit/loss are unavailable. ' + + "Amounts are in each security's own trade currency and are never converted: use the response's `byCurrency` subtotals rather than adding rows up, and never sum quantities or average costs. " + + 'Every response carries a `caveats` array stating the limits of the derivation. ' + + 'Set `includeClosed` to also see securities traded but no longer held. Use accounter_get_security_executions for the trades behind a row. Read-only. ' + + SCOPE_DESCRIPTION_SUFFIX, + inputSchema: listSecurityHoldingsInput, + policy: { requiresBusinessScope: true, dataClassification: 'business' }, + handler: listSecurityHoldingsHandler, +}; + +// --------------------------------------------------------------------------- +// Executions +// --------------------------------------------------------------------------- + +/** + * The bank's closed vocabularies, mirrored from the upstream enums. Listed rather + * than free strings so the model is told what it may ask for, and so an unknown + * value fails as input validation instead of as an empty result. + */ +export const SECURITY_TRADE_TYPES = [ + 'BUY', + 'SELL', + 'DIVIDEND_PAYMENT', + 'INTEREST_PAYMENT', + 'REDEMPTION', + 'STOCK_DISTRIBUTION', + 'TRANSFER_IN', + 'TRANSFER_OUT', + 'TRANSFER_IN_TWO_SIDED', + 'TRANSFER_OUT_TWO_SIDED', +] as const; + +export const SECURITY_TRANSACTION_TYPES = [ + 'BUY', + 'SELL', + 'PAYMENTS_AND_CORPORATE_ACTIONS', + 'TRANSFERS', +] as const; + +const securityIdList = (what: string) => + z.array(z.string().min(1)).min(1).max(MAX_REQUESTED_SECURITIES).optional().describe(what); + +const getSecurityExecutionsInput = z.object({ + memberBusinessIds: memberBusinessIdsInput, + securityBusinessIds: securityIdList( + 'Securities to include, by the `securityBusinessId` accounter_list_security_holdings returns.', + ), + isins: securityIdList('Securities to include, by ISIN.'), + symbols: securityIdList('Securities to include, by ticker symbol (case-insensitive).'), + fromTradeDate: TIMELESS_DATE.optional().describe('Earliest trade date to include (YYYY-MM-DD).'), + toTradeDate: TIMELESS_DATE.optional().describe('Latest trade date to include (YYYY-MM-DD).'), + tradeTypes: z + .array(z.enum(SECURITY_TRADE_TYPES)) + .min(1) + .optional() + .describe( + 'Restrict to these kinds of execution. Note dividends and interest are execution kinds here (DIVIDEND_PAYMENT, INTEREST_PAYMENT), not a separate entity.', + ), + transactionTypes: z + .array(z.enum(SECURITY_TRANSACTION_TYPES)) + .min(1) + .optional() + .describe('Restrict to these coarser buckets.'), + includeCharges: z + .boolean() + .optional() + .default(false) + .describe( + 'Also resolve the charge and transaction behind each execution. Requires naming securities (securityBusinessIds, isins or symbols) and is limited to a small number of them, because the pairing has to be computed over each security whole history rather than a page.', + ), + page: z + .number() + .int() + .positive() + .optional() + .default(1) + .describe('1-based page number, newest executions first.'), + pageSize: z + .number() + .int() + .positive() + .max(MAX_SECURITY_EXECUTIONS_PAGE_SIZE) + .optional() + .default(DEFAULT_SECURITY_EXECUTIONS_PAGE_SIZE) + .describe(`Rows per page (capped at ${MAX_SECURITY_EXECUTIONS_PAGE_SIZE}).`), +}); +type GetSecurityExecutionsInput = z.infer; + +const GET_SECURITY_EXECUTIONS_QUERY = /* GraphQL */ ` + query McpGetSecurityExecutions( + $filters: SecurityExecutionsFilter + $page: Int! + $limit: Int! + $includeCharges: Boolean! + ) { + securityExecutions( + filters: $filters + page: $page + limit: $limit + includeCharges: $includeCharges + ) { + pageInfo { + totalPages + totalRecords + } + nodes { + securityBusiness { + id + ownerId + isin + symbol + } + charge @include(if: $includeCharges) { + id + } + transaction @include(if: $includeCharges) { + id + } + execution { + id + tradeDate + valueDate + tradeType + transactionType + paymentType + quantity + tradePrice + netValue { + raw + formatted + currency + } + tradeCommission { + raw + formatted + currency + } + israelTaxValue { + raw + formatted + currency + } + } + } + } + } +`; + +type RawExecutionNode = McpGetSecurityExecutionsQuery['securityExecutions']['nodes'][number]; + +/** + * Each date is validated on its own even when only one is given: a value can pass + * the format regex and still not be a calendar date (2026-02-31). No width cap — + * pagination already bounds the result, and a trade history is meant to be asked + * about across years. + */ +function assertTradeDateRange(input: GetSecurityExecutionsInput): void { + let from: number | undefined; + if (input.fromTradeDate !== undefined) { + const parsed = parseCalendarDate(input.fromTradeDate); + if (parsed === null) { + throw new ToolInputError('Invalid fromTradeDate'); + } + from = parsed; + } + if (input.toTradeDate !== undefined) { + const parsed = parseCalendarDate(input.toTradeDate); + if (parsed === null) { + throw new ToolInputError('Invalid toTradeDate'); + } + if (from !== undefined && from > parsed) { + throw new ToolInputError('fromTradeDate must be on or before toTradeDate'); + } + } +} + +/** + * Refuse an unnamed `includeCharges` here rather than upstream. + * + * Upstream caps how many securities it will pair and rejects the rest, but that + * arrives as an UPSTREAM_ERROR — which reads as a server fault the model should + * retry, when in fact the call was malformed. Checking first turns it into the + * VALIDATION_ERROR it is, and says what to add. + */ +function assertChargeLinksAreNarrowed(input: GetSecurityExecutionsInput): void { + if (!input.includeCharges) { + return; + } + if (!input.securityBusinessIds && !input.isins && !input.symbols) { + throw new ToolInputError( + 'includeCharges requires naming the securities: pass securityBusinessIds, isins or symbols. Charge links are computed over a security whole history, so they cannot be resolved for the entire portfolio at once.', + ); + } +} + +function normalizeExecutionNode(node: RawExecutionNode) { + const { execution, securityBusiness } = node; + return { + executionId: execution.id, + securityBusinessId: securityBusiness.id, + ownerId: securityBusiness.ownerId, + isin: securityBusiness.isin, + symbol: securityBusiness.symbol, + tradeDate: execution.tradeDate, + valueDate: execution.valueDate, + tradeType: execution.tradeType, + transactionType: execution.transactionType, + paymentType: execution.paymentType, + quantity: execution.quantity, + tradePrice: execution.tradePrice, + netValue: normalizeAmount(execution.netValue), + tradeCommission: normalizeAmount(execution.tradeCommission), + israelTaxValue: normalizeAmount(execution.israelTaxValue), + // Absent rather than null when not asked for, so "no charge matched" stays + // distinguishable from "charge links were not requested". `@include(if:)` + // omits the field entirely, which is what makes the distinction possible. + ...(node.charge === undefined ? {} : { chargeId: node.charge?.id ?? null }), + ...(node.transaction === undefined ? {} : { transactionId: node.transaction?.id ?? null }), + }; +} + +async function getSecurityExecutionsHandler( + input: GetSecurityExecutionsInput, + context: ToolExecutionContext, +): Promise { + assertTradeDateRange(input); + assertChargeLinksAreNarrowed(input); + + const data = await context.client.query( + { + query: GET_SECURITY_EXECUTIONS_QUERY, + variables: { + filters: { + securityBusinessIds: input.securityBusinessIds, + isins: input.isins, + symbols: input.symbols, + fromTradeDate: input.fromTradeDate, + toTradeDate: input.toTradeDate, + tradeTypes: input.tradeTypes, + transactionTypes: input.transactionTypes, + }, + // Upstream pages from zero; the tool's page is 1-based, as everywhere. + page: input.page - 1, + limit: input.pageSize, + includeCharges: input.includeCharges, + }, + }, + context.upstream, + ); + + const { nodes, pageInfo } = data.securityExecutions; + const executions = nodes.map(normalizeExecutionNode); + const pagination = { + page: input.page, + pageSize: input.pageSize, + totalPages: pageInfo.totalPages, + hasNextPage: input.page < pageInfo.totalPages, + }; + + return shapeListResult({ + items: executions, + itemsKey: 'executions', + total: pageInfo.totalRecords, + extra: { + pagination, + caveats: SECURITIES_CAVEATS, + scope: { memberBusinessIds: context.readScope.memberBusinessIds }, + }, + summarize: (shown, total) => + total === 0 + ? 'No executions matched the given filters.' + : `${total} execution(s); showing ${shown} on page ${pagination.page} of ${pagination.totalPages}, newest first. Amounts are in each security own trade currency.`, + }); +} + +export const getSecurityExecutionsTool: ToolDefinition = { + name: GET_SECURITY_EXECUTIONS_TOOL_NAME, + description: + 'Fetch securities trade history - buys, sales, dividends, interest, redemptions and transfers - newest first, with dates, direction, quantity, unit price, net value, commission and Israeli tax. ' + + 'Narrow by security (securityBusinessIds, isins or symbols - these three union with each other, since they are three ways of naming the same thing), by trade date, and by kind. ' + + "Amounts are in each security's own trade currency and are never converted, so do not add rows from different securities together. " + + 'Set `includeCharges` to also get the charge each trade cash movement landed on; that requires naming the securities, because the pairing is computed over a security whole history rather than a page. ' + + 'If a call fails with an upstream error naming an unknown trade type, the bank has used a label the server does not yet translate - that is a data bug worth reporting, not something to retry; passing explicit `tradeTypes` filters such rows out and works around it. Read-only. ' + + SCOPE_DESCRIPTION_SUFFIX, + inputSchema: getSecurityExecutionsInput, + policy: { requiresBusinessScope: true, dataClassification: 'business' }, + handler: getSecurityExecutionsHandler, +}; diff --git a/packages/mcp-server/src/tools/terminology-data.ts b/packages/mcp-server/src/tools/terminology-data.ts index f5d43406e..a96571334 100644 --- a/packages/mcp-server/src/tools/terminology-data.ts +++ b/packages/mcp-server/src/tools/terminology-data.ts @@ -16,7 +16,7 @@ * result or an input schema and needs to know what it means and what it is * *not*. Prefer stating the trap over restating the field name. * - * Budget: at 62 entries the index is ~10 KB and the full glossary ~40 KB against + * Budget: at 67 entries the index is ~11 KB and the full glossary ~40 KB against * the 60 KB `MAX_TOOL_RESULT_BYTES` guard, so there is room for roughly 30 more * before a request for every topic starts truncating. `terminology.test.ts` * asserts the whole glossary still fits in one response, so overshooting fails @@ -173,9 +173,10 @@ const CHARGE_ENTRIES: readonly GlossaryEntry[] = [ summary: 'FOREIGN_SECURITIES / ForeignSecuritiesCharge — activity in a securities portfolio held at the bank.', detail: - 'Buys, sells, dividends and fees on a foreign securities portfolio, identified by the securities counterparty business configured for the owner.', + 'Buys, sells, dividends, redemptions and fees on a securities portfolio held at the bank. A charge is typed this way when any of its businesses is on the securities side, which since the per-security businesses landed means the traded security itself — the general "Foreign Securities" business configured for the owner is now only the fallback for a movement no specific security was resolved for. The cash leg is a bank transaction like any other; the trade behind it lives in the ingested portfolio feed and is only reachable by asking for it (see security-execution). Ledger generation for this charge type is not supported yet.', aliases: ['FOREIGN_SECURITIES', 'ForeignSecuritiesCharge'], - seeAlso: ['charge-type'], + seeAlso: ['charge-type', 'security-business', 'security-execution'], + tools: ['accounter_get_charges'], }, { term: 'creditcard-bank-charge', @@ -370,6 +371,24 @@ const TRANSACTION_ENTRIES: readonly GlossaryEntry[] = [ aliases: ['ConversionTransaction', 'QUOTE', 'BASE'], seeAlso: ['conversion-charge', 'transaction'], }, + { + term: 'security-execution', + topic: 'transaction', + summary: + 'One executed action in a securities portfolio — a buy, sale, dividend, interest payment, redemption, distribution or transfer.', + detail: + "Scraped from the bank's portfolio feed, NOT from the bank statement: an execution is what the portfolio reports happened to the security, while the matching bank transaction is what happened to the cash. They are separate records with no link in the source — the scrape has no per-execution id — so the pairing is derived, and it is exact: same account, the execution's value date equal to the transaction's effective debit date, and the net value in the transaction's own currency matching with the sign the trade type implies. Pairing is one-to-one, so a security executed several times in a day for the same amount still maps each execution to its own cash movement. Dividends and interest are execution KINDS here (DIVIDEND_PAYMENT, INTEREST_PAYMENT), not a separate entity, and they move cash without changing the unit count. Amounts are in the security's own trade currency and are never converted.", + aliases: [ + 'SecurityExecution', + 'execution', + 'executions', + 'trade', + 'SecurityTradeType', + 'SecurityTransactionType', + ], + seeAlso: ['security-business', 'derived-position', 'foreign-securities-charge', 'transaction'], + tools: ['accounter_get_security_executions', 'accounter_get_charges'], + }, ]; const DOCUMENT_ENTRIES: readonly GlossaryEntry[] = [ @@ -682,6 +701,58 @@ const ENTITY_ENTRIES: readonly GlossaryEntry[] = [ aliases: ['adminContext', 'AdminContextInfo'], seeAlso: ['tax-category', 'ledger-record', 'ledger-lock', 'owner'], }, + { + term: 'security-business', + topic: 'entity', + summary: + 'A traded security modelled as a business of its own, identified by its ISIN, so it can be a counterparty and have a page.', + detail: + "The presence of a businesses_securities row is what makes a business a security — the same shape as businesses_admin or clients. Identity is the ISIN, which forces an indirection: the rest of the system addresses a security by Poalim's proprietary key, and the reference feed that key joins to carries no ISIN at all, so an identifier table bridges them. That is also how two Poalim keys collapse onto one security. A security business inherits sort code, IRS code, country and tax category from the tenant's general foreign-securities business, and deliberately carries no suggestion phrases: a security must never win a description-based counterparty match. Its id is the security's identity everywhere — the `securityBusinessId` on a holding, an execution and a charge's securities block are all the same id.", + aliases: ['SecurityBusiness', 'securityInfo', 'securityBusinessId', 'security'], + seeAlso: ['isin', 'poalim-security-key', 'derived-position', 'counterparty'], + tools: ['accounter_list_security_holdings', 'accounter_get_security_executions'], + }, + { + term: 'derived-position', + topic: 'entity', + summary: + "What a security's ingested executions add up to: units held, average cost, totals bought and sold. Derived, never reported.", + detail: + "The bank does not report a holding, so a position is arithmetic over the scraped trade history and is only as complete as that history. Four consequences worth stating before quoting any of these numbers: anything held before historyStartDate is not counted; splits and corporate actions that change the unit count with no execution row are invisible; a NEGATIVE quantity means the history starts mid-life (a data-quality signal, not a short position); and a NULL amount means nothing was ingested rather than zero, because with no execution there is no currency to state an amount in. There are no market prices anywhere in the system, so current value and unrealized profit or loss cannot be computed at all. Amounts are each security's own trade currency and are never converted — never sum them across securities, and never sum quantities or average costs even within one currency.", + aliases: [ + 'SecurityPosition', + 'SecurityHolding', + 'position', + 'holding', + 'holdings', + 'averageCost', + 'historyStartDate', + ], + seeAlso: ['security-business', 'security-execution'], + tools: ['accounter_list_security_holdings'], + }, + { + term: 'poalim-security-key', + topic: 'entity', + summary: + "The bank's proprietary id for a security, parsed out of transaction descriptions rather than reported as a field.", + detail: + "A securities transaction's description carries the key as a zero-padded run of digits, which is extracted and unpadded — it is the only link between a bank cash movement and a security. It is NOT the ISIN and is not comparable across brokers: the identifier table maps POALIM_SECURITY_KEY to a security business, and several keys may point at one security. A key with no ingested reference row is reported with null details rather than dropped, because a stale scrape should be visible instead of looking like an absent security.", + aliases: ['securityKey', 'POALIM_SECURITY_KEY', 'security key'], + seeAlso: ['security-business', 'isin', 'foreign-securities-charge'], + tools: ['accounter_get_charges'], + }, + { + term: 'isin', + topic: 'entity', + summary: + 'The international securities identification number — the identity a security business is keyed by.', + detail: + "Unique per owner, and the thing that distinguishes two share classes of one issuer where a name or symbol would not, which is why pickers label a security by name AND ISIN. It appears on execution rows but not on the bank's reference feed, which is the reason identity and lookup are split across two tables. Use it as the stable way to name a security to a tool when you do not have its `securityBusinessId`.", + aliases: ['ISIN', 'isins'], + seeAlso: ['security-business', 'poalim-security-key'], + tools: ['accounter_get_security_executions'], + }, ]; const SCOPE_ENTRIES: readonly GlossaryEntry[] = [ diff --git a/packages/migrations/src/actions/2026-08-23T10-00-00.rls-scope-securities-tables.ts b/packages/migrations/src/actions/2026-08-23T10-00-00.rls-scope-securities-tables.ts new file mode 100644 index 000000000..437ec7fe8 --- /dev/null +++ b/packages/migrations/src/actions/2026-08-23T10-00-00.rls-scope-securities-tables.ts @@ -0,0 +1,102 @@ +import { sql } from 'slonik'; +import { type MigrationExecutor } from '../pg-migrator.js'; + +/** + * Multi-business read scope for the securities tables. + * + * `2026-05-25T10-00-00.rls-multi-business-scope.sql` switched every + * `tenant_isolation` read predicate to + * `owner_id = ANY (accounter_schema.get_current_business_scope())`, leaving writes pinned to + * the single `get_current_business_id()` target. All four securities tables were created + * *after* that migration (2026-08-11 / 08-13 / 08-20) and so were never in its list — they + * still read through the singular helper. + * + * The consequence is a silent narrowing rather than a leak: a request whose authorized scope + * spans several businesses sees securities for only one of them, with nothing in the response + * saying so. That breaks the web client's business switcher and, more sharply, the MCP + * connector, which forwards its resolved read scope as `x-business-scope` and echoes it back + * to the caller (`docs/coherent-owner-scoping-for-mcp/plan.md`). + * + * Predicates are deliberately byte-identical to the earlier migration's: reads follow the + * request's scope, writes stay on the explicit write target, so the scraper ingestion path is + * unaffected. + * + * Widening the permissive `USING` is not sufficient on its own: **every write stays pinned to the + * single write target, and only reads follow the scope.** + * + * `USING` is what selects the rows a statement may act on, and Postgres consults it for DELETE and + * UPDATE as well as SELECT. `WITH CHECK` constrains only the *new* values an INSERT or UPDATE + * writes. So a permissive policy whose `USING` spans the whole read scope would let a session + * delete another in-scope business's row, or update one — the `WITH CHECK` would then happily + * accept the result, since the new value names the write target, which is to say the row would be + * *moved* from one business to another. + * + * Two RESTRICTIVE policies close that. A restrictive policy ANDs with the permissive one, so a row + * must satisfy both: in the read scope (permissive) *and* owned by the write target (restrictive). + * They are per-command on purpose — a restrictive `FOR ALL` would apply to SELECT too and undo the + * widening this migration exists for. INSERT needs no such policy, having no `USING` at all; the + * permissive `WITH CHECK` is the whole of its authorization. + * + * `2026-05-26T10-00-00.rls-delete-write-target.sql` established the DELETE half of this for the + * tables it covered. The securities tables were not in its list either, and until now did not need + * it because their `USING` was still the singular helper — widening the read scope is what creates + * the need, so both halves belong in this migration. + */ +export default { + name: '2026-08-23T10-00-00.rls-scope-securities-tables.sql', + run: async ({ connection }) => { + const tables = [ + 'poalim_securities', + 'poalim_securities_transactions', + 'businesses_securities', + 'security_identifiers', + ]; + + for (const table of tables) { + await connection.query( + sql.unsafe`DROP POLICY IF EXISTS tenant_isolation ON accounter_schema.${sql.identifier([table])}`, + ); + + // Reads (USING): any business in the request's authorized scope. + // Writes (WITH CHECK): strictly the single explicit write-target business. + await connection.query( + sql.unsafe` + CREATE POLICY tenant_isolation ON accounter_schema.${sql.identifier([table])} + FOR ALL + USING (owner_id = ANY (accounter_schema.get_current_business_scope())) + WITH CHECK (owner_id = accounter_schema.get_current_business_id()) + `, + ); + + // Writes stay single-tenant. Both restrictive policies AND with the permissive one above, + // so a row must be in the read scope *and* owned by the write target to be written. + for (const command of ['delete', 'update'] as const) { + await connection.query( + sql.unsafe`DROP POLICY IF EXISTS ${sql.identifier([`tenant_isolation_${command}`])} ON accounter_schema.${sql.identifier([table])}`, + ); + } + + // DELETE is authorized by USING alone. + await connection.query( + sql.unsafe` + CREATE POLICY tenant_isolation_delete ON accounter_schema.${sql.identifier([table])} + AS RESTRICTIVE + FOR DELETE + USING (owner_id = accounter_schema.get_current_business_id()) + `, + ); + + // UPDATE consults USING to pick the row and WITH CHECK to validate the new value. Without + // this, a row owned by another business in the read scope could be updated into the write + // target's ownership — a cross-tenant move that the permissive WITH CHECK would accept. + await connection.query( + sql.unsafe` + CREATE POLICY tenant_isolation_update ON accounter_schema.${sql.identifier([table])} + AS RESTRICTIVE + FOR UPDATE + USING (owner_id = accounter_schema.get_current_business_id()) + `, + ); + } + }, +} satisfies MigrationExecutor; diff --git a/packages/migrations/src/run-pg-migrations.ts b/packages/migrations/src/run-pg-migrations.ts index d35338c6f..9063d9b25 100644 --- a/packages/migrations/src/run-pg-migrations.ts +++ b/packages/migrations/src/run-pg-migrations.ts @@ -200,6 +200,7 @@ import migration_2026_08_12T10_00_00_poalim_securities_tenant_scoped_dedup from import migration_2026_08_13T12_00_00_add_poalim_securities_transactions_table from './actions/2026-08-13T12-00-00.add-poalim-securities-transactions-table.js'; import migration_2026_08_14T10_00_00_poalim_securities_transactions_calendar_dates from './actions/2026-08-14T10-00-00.poalim-securities-transactions-calendar-dates.js'; import migration_2026_08_20T10_00_00_add_security_businesses from './actions/2026-08-20T10-00-00.add-security-businesses.js'; +import migration_2026_08_23T10_00_00_rls_scope_securities_tables from './actions/2026-08-23T10-00-00.rls-scope-securities-tables.js'; import { runMigrations } from './pg-migrator.js'; export const MIGRATIONS = [ @@ -404,6 +405,7 @@ export const MIGRATIONS = [ migration_2026_08_13T12_00_00_add_poalim_securities_transactions_table, migration_2026_08_14T10_00_00_poalim_securities_transactions_calendar_dates, migration_2026_08_20T10_00_00_add_security_businesses, + migration_2026_08_23T10_00_00_rls_scope_securities_tables, ] as const; export const LATEST_MIGRATION_NAME = MIGRATIONS[MIGRATIONS.length - 1]?.name; diff --git a/packages/server/src/modules/foreign-securities/helpers/__tests__/security-execution-enums.helper.test.ts b/packages/server/src/modules/foreign-securities/helpers/__tests__/security-execution-enums.helper.test.ts index 19e485e83..c04b976f0 100644 --- a/packages/server/src/modules/foreign-securities/helpers/__tests__/security-execution-enums.helper.test.ts +++ b/packages/server/src/modules/foreign-securities/helpers/__tests__/security-execution-enums.helper.test.ts @@ -5,9 +5,12 @@ import { SecurityTransactionType, } from '../../../../shared/enums.js'; import { + paymentTypeToRaw, toSecurityPaymentType, toSecurityTradeType, toSecurityTransactionType, + tradeTypeToRaw, + transactionTypeToRaw, } from '../security-execution-enums.helper.js'; describe('toSecurityTradeType', () => { @@ -70,3 +73,64 @@ describe('toSecurityPaymentType', () => { expect(() => toSecurityPaymentType('פדיון')).toThrow(/PAYMENT_TYPES/); }); }); + +/** + * The inverses exist so a filter can push a GraphQL enum into SQL without + * hand-writing the bank's Hebrew. Round-tripping every enum member is what makes + * that safe: a forward map that grows a member without its label, or an inverse + * that resolves to the wrong map, fails here rather than at query time as an + * empty result nobody can explain. + */ +describe('the reverse maps', () => { + it.each(Object.values(SecurityTradeType))('round-trips trade type %s', tradeType => { + const raw = tradeTypeToRaw[tradeType]; + expect(raw, `${tradeType} has no label`).toBeTruthy(); + expect(toSecurityTradeType(raw)).toBe(tradeType); + }); + + it.each(Object.values(SecurityTransactionType))( + 'round-trips transaction type %s', + transactionType => { + const raw = transactionTypeToRaw[transactionType]; + expect(raw, `${transactionType} has no label`).toBeTruthy(); + expect(toSecurityTransactionType(raw)).toBe(transactionType); + }, + ); + + it.each(Object.values(SecurityPaymentType))('round-trips payment type %s', paymentType => { + const raw = paymentTypeToRaw[paymentType]; + expect(raw, `${paymentType} has no label`).toBeTruthy(); + expect(toSecurityPaymentType(raw)).toBe(paymentType); + }); + + /** + * The reason the inverses are per-map rather than one shared table: the bank + * spells redemption two ways, and a shared inverse would silently resolve a + * trade-type redemption to the payment-type spelling (or the reverse), which + * would then match nothing in the column being filtered. + */ + it('keeps the two spellings of redemption apart', () => { + expect(tradeTypeToRaw[SecurityTradeType.Redemption]).toBe('פדיון'); + expect(paymentTypeToRaw[SecurityPaymentType.Redemption]).toBe( + 'פידיון', + ); + expect(tradeTypeToRaw[SecurityTradeType.Redemption]).not.toBe( + paymentTypeToRaw[SecurityPaymentType.Redemption], + ); + }); + + /** + * Buy and sell are spelled identically as a trade type and a transaction type, + * so an inverse keyed only by the enum member's *name* would be ambiguous. These + * are separate enums, and the labels agreeing is a fact about the source rather + * than a collision. + */ + it('shares the buy/sell labels across the two vocabularies', () => { + expect(tradeTypeToRaw[SecurityTradeType.Buy]).toBe( + transactionTypeToRaw[SecurityTransactionType.Buy], + ); + expect(tradeTypeToRaw[SecurityTradeType.Sell]).toBe( + transactionTypeToRaw[SecurityTransactionType.Sell], + ); + }); +}); diff --git a/packages/server/src/modules/foreign-securities/helpers/security-execution-enums.helper.ts b/packages/server/src/modules/foreign-securities/helpers/security-execution-enums.helper.ts index 10d68653d..5ee6ea81f 100644 --- a/packages/server/src/modules/foreign-securities/helpers/security-execution-enums.helper.ts +++ b/packages/server/src/modules/foreign-securities/helpers/security-execution-enums.helper.ts @@ -77,3 +77,28 @@ export const toSecurityTransactionType = (raw: string): SecurityTransactionType export const toSecurityPaymentType = (raw: string | null): SecurityPaymentType | null => raw === null ? null : translate(PAYMENT_TYPES, raw, 'payment type', 'PAYMENT_TYPES'); + +/** + * The translation run backwards, so a filter can push a GraphQL enum into SQL without + * hand-writing the bank's Hebrew. + * + * Built by inverting the maps above rather than written out again — a value added to one and + * forgotten in the other is exactly the drift these maps exist to make loud. The inverses are + * kept per-map for the same reason the forward maps are: the bank spells the same word two ways + * (`פדיון` as a trade type, `פידיון` as a payment type), so a single shared inverse would + * silently resolve one to the other. + * + * Every enum member is covered because each forward map is a bijection today; the + * `Record` assertion is what will fail the build if a new member arrives without + * its label. + */ +function invert(map: Record): Record { + return Object.fromEntries(Object.entries(map).map(([raw, value]) => [value, raw])) as Record< + T, + string + >; +} + +export const tradeTypeToRaw = invert(TRADE_TYPES); +export const transactionTypeToRaw = invert(TRANSACTION_TYPES); +export const paymentTypeToRaw = invert(PAYMENT_TYPES); diff --git a/packages/server/src/modules/foreign-securities/providers/__tests__/foreign-securities.integration.test.ts b/packages/server/src/modules/foreign-securities/providers/__tests__/foreign-securities.integration.test.ts index 2da046b6e..cf45e2c5a 100644 --- a/packages/server/src/modules/foreign-securities/providers/__tests__/foreign-securities.integration.test.ts +++ b/packages/server/src/modules/foreign-securities/providers/__tests__/foreign-securities.integration.test.ts @@ -9,7 +9,8 @@ import { TenantAwareDBClient } from '../../../app-providers/tenant-db-client.js' import type { FinancialAccountsProvider } from '../../../financial-accounts/providers/financial-accounts.provider.js'; import type { FinancialBankAccountsProvider } from '../../../financial-accounts/providers/financial-bank-accounts.provider.js'; import type { TransactionsProvider } from '../../../transactions/providers/transactions.provider.js'; -import { ForeignSecuritiesProvider } from '../foreign-securities.provider.js'; +import { dateToTimelessDateString } from '../../../../shared/helpers/misc.js'; +import { ForeignSecuritiesProvider, MAX_CHARGE_LINK_SECURITIES } from '../foreign-securities.provider.js'; import { SecurityBusinessesProvider } from '../security-businesses.provider.js'; let pool: Pool; @@ -27,7 +28,31 @@ const ACCOUNT_ID = '00000000-0000-0000-0000-00000000a001'; const APPLE_BUSINESS_ID = '00000000-0000-0000-0000-0000000005a1'; const MSFT_BUSINESS_ID = '00000000-0000-0000-0000-0000000005a2'; const ISIN_ONLY_BUSINESS_ID = '00000000-0000-0000-0000-0000000005a3'; -const SECURITY_BUSINESS_IDS = [APPLE_BUSINESS_ID, MSFT_BUSINESS_ID, ISIN_ONLY_BUSINESS_ID]; +/** + * Filler security businesses, used only to push a tenant past + * `MAX_CHARGE_LINK_SECURITIES`. Declared here rather than inline so the shared + * cleanup covers them — a leaked security business is visible to every other case + * in the file, since this suite connects as a superuser and so is not scoped by RLS. + */ +const OVERFLOW_BUSINESS_IDS = Array.from( + { length: MAX_CHARGE_LINK_SECURITIES }, + (_unused, index) => `00000000-0000-0000-0000-0000000007${String(index).padStart(2, '0')}`, +); +const SECURITY_BUSINESS_IDS = [ + APPLE_BUSINESS_ID, + MSFT_BUSINESS_ID, + ISIN_ONLY_BUSINESS_ID, + ...OVERFLOW_BUSINESS_IDS, +]; +/** + * ISINs here are synthetic (`ZZ…`) rather than real ones. + * + * The securities lookups carry no `owner_id` predicate — RLS does that, and this + * suite connects as a superuser which bypasses it — so every security business in + * the database is visible to a lookup by ISIN. Sharing a real ISIN with + * `security-businesses.integration.test.ts`, which runs concurrently and asserts + * on the row it gets back for one, is a genuine cross-suite collision. + */ const BANK_NUMBER = 12; const BRANCH_NUMBER = 615; const ACCOUNT_NUMBER = 100000; @@ -74,22 +99,27 @@ const VALUE_DATE = '2024-03-12'; function createStubTransactionsProvider(transactions: StubTransaction[]): TransactionsProvider { return { transactionsByChargeIDLoader: { - load: (chargeId: string) => - Promise.resolve( - transactions.map(transaction => ({ - charge_id: chargeId, - amount: '-1000.00', - currency: 'USD', - debit_date: new Date(`${VALUE_DATE}T00:00:00`), - debit_date_override: null, - account_id: ACCOUNT_ID, - ...transaction, - })), - ), + load: (chargeId: string) => Promise.resolve(transactions.map(row => withDefaults(chargeId, row))), }, + // The reverse direction, used by `getSecurityBusinessHistory`: the candidate cash + // movements are the security business's own transactions. + getTransactionsByFilters: () => + Promise.resolve(transactions.map(row => withDefaults(CHARGE_ID, row))), } as unknown as TransactionsProvider; } +function withDefaults(chargeId: string, transaction: StubTransaction) { + return { + charge_id: chargeId, + amount: '-1000.00', + currency: 'USD', + debit_date: new Date(`${VALUE_DATE}T00:00:00`), + debit_date_override: null, + account_id: ACCOUNT_ID, + ...transaction, + }; +} + /** * The account lookups are pure id → row maps in the real providers; stubbing them keeps this * suite off the financial_accounts fixtures while still exercising the tuple resolution @@ -179,6 +209,8 @@ type ExecutionFixture = { tradeDate?: string; valueDate?: string | null; tradeType?: string; + /** Defaults to `tradeType`, which is the invariant on a plain buy or sale. */ + transactionType?: string; netValueTradeCurrency?: string; /** Units moved. Fractional on purpose in the holdings cases — ETFs trade that way. */ nv?: string; @@ -193,6 +225,7 @@ async function insertExecution({ tradeDate = '2024-03-10', valueDate = VALUE_DATE, tradeType = 'קניה', + transactionType, netValueTradeCurrency = '1000.00', nv = '10', }: ExecutionFixture) { @@ -202,7 +235,7 @@ async function insertExecution({ `INSERT INTO accounter_schema.poalim_securities_transactions ( owner_id, bank_number, branch_number, account_number, security, trade_date, value_date, trade_type, transaction_type, nv, trade_price, net_value_trade_currency, trade_currency - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $8, $10, 100, $9, 'דולר ארה"ב')`, + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $11, $10, 100, $9, 'דולר ארה"ב')`, [ ownerId, BANK_NUMBER, @@ -214,6 +247,7 @@ async function insertExecution({ tradeType, netValueTradeCurrency, nv, + transactionType ?? tradeType, ], ); } @@ -281,10 +315,12 @@ beforeAll(async () => { await ensureRlsRole(pool, { grants: [ - { table: 'poalim_securities', privileges: 'SELECT' }, - { table: 'poalim_securities_transactions', privileges: 'SELECT' }, - { table: 'businesses_securities', privileges: 'SELECT' }, - { table: 'security_identifiers', privileges: 'SELECT' }, + // DELETE/UPDATE as well as SELECT: the write-target cases below have to reach the policy + // check rather than failing on a missing privilege, which would pass for the wrong reason. + { table: 'poalim_securities', privileges: 'SELECT, UPDATE, DELETE' }, + { table: 'poalim_securities_transactions', privileges: 'SELECT, UPDATE, DELETE' }, + { table: 'businesses_securities', privileges: 'SELECT, UPDATE, DELETE' }, + { table: 'security_identifiers', privileges: 'SELECT, UPDATE, DELETE' }, ], }); }); @@ -338,7 +374,7 @@ describe('getChargeSecurities', () => { { id: 't1', source_description: 'ניע"ז מכירה 0005129523' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities).toHaveLength(1); expect(securities[0].securityKey).toBe('5129523'); @@ -352,7 +388,7 @@ describe('getChargeSecurities', () => { { id: 't1', source_description: 'ניע"ז קניה 0077774297' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities).toHaveLength(1); expect(securities[0].securityKey).toBe('77774297'); @@ -367,7 +403,7 @@ describe('getChargeSecurities', () => { { id: 'fee', source_description: 'ניע"ז עמ קניה 0005129523' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities).toHaveLength(1); expect(securities[0].transactionIds).toEqual(['trade', 'fee']); @@ -380,7 +416,7 @@ describe('getChargeSecurities', () => { { id: 't2', source_description: null }, ]); - expect(await provider.getChargeSecurities(CHARGE_ID)).toEqual([]); + expect(await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID)).toEqual([]); }); it('returns one entry per distinct key on a merged charge', async () => { @@ -395,7 +431,7 @@ describe('getChargeSecurities', () => { { id: 't2', source_description: 'ניע"ז קניה 0077774297' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities.map(s => s.securityKey)).toEqual(['5129523', '77774297']); expect(securities.map(s => s.details?.eng_name)).toEqual(['Example Corp', 'Other Corp']); @@ -419,7 +455,7 @@ describe('getChargeSecurities', () => { { id: 't1', source_description: 'ניע"ז מכירה 0005129523' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities).toHaveLength(1); expect(securities[0].details?.eng_name).toBe('Fresh Name'); @@ -465,7 +501,7 @@ describe('getChargeSecurities — matched executions', () => { { id: 't1', source_description: 'ניע"ז קניה 0005129523', amount: '-1000.00' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions).toHaveLength(1); expect(securities[0].executions[0].trade_type).toBe('קניה'); @@ -484,7 +520,7 @@ describe('getChargeSecurities — matched executions', () => { { id: 't1', source_description: 'ניע"ז מכירה 0005129523', amount: '2500.50' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions.map(e => e.net_value_trade_currency)).toEqual(['2500.50']); }); @@ -496,7 +532,7 @@ describe('getChargeSecurities — matched executions', () => { { id: 't1', source_description: 'ניע"ז קניה 0005129523', amount: '-1000.00' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions).toEqual([]); }); @@ -508,7 +544,7 @@ describe('getChargeSecurities — matched executions', () => { { id: 't1', source_description: 'ניע"ז קניה 0005129523', amount: '-1000.00' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions).toEqual([]); }); @@ -520,7 +556,7 @@ describe('getChargeSecurities — matched executions', () => { { id: 't1', source_description: 'ניע"ז קניה 0005129523', amount: '-1000.00' }, ]); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions).toEqual([]); }); @@ -534,7 +570,7 @@ describe('getChargeSecurities — matched executions', () => { 'IL12-3456', ); - const securities = await provider.getChargeSecurities(CHARGE_ID); + const securities = await provider.getChargeSecurities(CHARGE_ID, TEST_OWNER_ID); expect(securities[0].executions).toEqual([]); }); @@ -564,17 +600,24 @@ describe('getChargeSecurities — matched executions', () => { }); }); +/** This suite's own buckets out of a tenant-wide result. See the note on ISINs above. */ +function ownedBuckets(byBusinessId: Map): T[][] { + return SECURITY_BUSINESS_IDS.map(id => byBusinessId.get(id)).filter( + (bucket): bucket is T[] => bucket !== undefined, + ); +} + describe('getExecutionsBySecurityBusiness', () => { it('gives every security business its own executions, chronologically', async () => { await insertSecurityBusiness({ id: APPLE_BUSINESS_ID, - isin: 'US0378331005', + isin: 'ZZ0000000601', engName: 'APPLE INC', securityKeys: ['1097'], }); await insertSecurityBusiness({ id: MSFT_BUSINESS_ID, - isin: 'US5949181045', + isin: 'ZZ0000000602', engName: 'MICROSOFT CORP', securityKeys: ['2044'], }); @@ -596,7 +639,7 @@ describe('getExecutionsBySecurityBusiness', () => { it('collapses several Poalim keys onto the one security business they name', async () => { await insertSecurityBusiness({ id: APPLE_BUSINESS_ID, - isin: 'US0378331005', + isin: 'ZZ0000000601', securityKeys: ['1097', '1098'], }); await insertExecution({ security: '1097' }); @@ -625,7 +668,7 @@ describe('getExecutionsBySecurityBusiness', () => { it('ignores executions whose key belongs to no security business', async () => { await insertSecurityBusiness({ id: APPLE_BUSINESS_ID, - isin: 'US0378331005', + isin: 'ZZ0000000601', securityKeys: ['1097'], }); await insertExecution({ security: '1097' }); @@ -633,16 +676,26 @@ describe('getExecutionsBySecurityBusiness', () => { const executionsByBusinessId = await createProvider([]).getExecutionsBySecurityBusiness(); - expect(executionsByBusinessId.size).toBe(1); + // Asserted on this suite's own buckets rather than on the map's size: the + // query carries no owner predicate — RLS is what scopes it in production, and + // this suite connects as a superuser which bypasses it — so a concurrently + // running suite's security businesses are legitimately in the result. expect(executionsByBusinessId.get(APPLE_BUSINESS_ID)).toHaveLength(1); + // '9999' belongs to no security business, so it is nowhere in the map. + expect(ownedBuckets(executionsByBusinessId).flat()).toHaveLength(1); }); - it('has no entries at all when the tenant has no security businesses', async () => { + it('gives a security business with nothing ingested an empty bucket, not none', async () => { + await insertSecurityBusiness({ id: APPLE_BUSINESS_ID, isin: 'ZZ0000000601' }); await insertExecution({ security: '1097' }); const executionsByBusinessId = await createProvider([]).getExecutionsBySecurityBusiness(); - expect(executionsByBusinessId.size).toBe(0); + // Apple carries no POALIM_SECURITY_KEY identifier, so the execution cannot + // resolve to it — but the business still gets an entry, which is what keeps + // "nothing ingested" distinguishable from "not a security". + expect(executionsByBusinessId.has(APPLE_BUSINESS_ID)).toBe(true); + expect(executionsByBusinessId.get(APPLE_BUSINESS_ID)).toEqual([]); }); /** @@ -653,7 +706,7 @@ describe('getExecutionsBySecurityBusiness', () => { it("does not expose another tenant's security businesses under tenant_isolation", async () => { await insertSecurityBusiness({ id: APPLE_BUSINESS_ID, - isin: 'US0378331005', + isin: 'ZZ0000000601', securityKeys: ['1097'], ownerId: OTHER_OWNER_ID, }); @@ -680,3 +733,523 @@ describe('getExecutionsBySecurityBusiness', () => { } }); }); + +/** + * `Query.securityExecutions` behind the provider: the SQL-pushdown path, the + * unpaginated match path behind `includeCharges`, and the filter resolution both + * share. + */ +describe('getSecurityExecutionsPage', () => { + const APPLE_ISIN = 'ZZ0000000601'; + const MSFT_ISIN = 'ZZ0000000602'; + + /** + * Every case narrows to this suite's own two securities by default. + * + * Omitting the identity filter means "every security this tenant has", and the + * suite connects as a superuser — so RLS does not scope the read and a + * concurrently-running suite's fixtures would land in the result. Only the one + * case that is actually about the unfiltered behaviour leaves this out, and it + * asserts containment rather than an exact count. + */ + const page = ( + overrides: Partial[0]> = {}, + ) => + createProvider([]).getSecurityExecutionsPage({ + page: 0, + limit: 50, + includeCharges: false, + ownerId: TEST_OWNER_ID, + ...overrides, + filters: { isins: [APPLE_ISIN, MSFT_ISIN], ...overrides.filters }, + }); + + beforeEach(async () => { + await insertSecurityBusiness({ + id: APPLE_BUSINESS_ID, + isin: APPLE_ISIN, + engName: 'Apple', + securityKeys: ['1097'], + }); + await insertSecurityBusiness({ + id: MSFT_BUSINESS_ID, + isin: MSFT_ISIN, + engName: 'Microsoft', + securityKeys: ['2098'], + }); + }); + + it('is empty, without erroring, when the tenant has no matching security', async () => { + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { isins: ['ZZ0000000699'] }, + page: 0, + limit: 50, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + + expect(result.nodes).toEqual([]); + expect(result.totalRecords).toBe(0); + }); + + it('covers every security when no identity filter is given', async () => { + await insertExecution({ security: '1097' }); + await insertExecution({ security: '2098' }); + + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: {}, + page: 0, + limit: 500, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + + // Containment, not equality: with no identity filter and no RLS under a + // superuser connection, a concurrent suite's fixtures can be in here too. + const businessIds = new Set(result.nodes.map(node => node.securityBusinessId)); + expect(businessIds).toContain(APPLE_BUSINESS_ID); + expect(businessIds).toContain(MSFT_BUSINESS_ID); + }); + + it('orders newest first — the opposite of the history query', async () => { + await insertExecution({ security: '1097', tradeDate: '2024-01-01' }); + await insertExecution({ security: '1097', tradeDate: '2024-06-01' }); + await insertExecution({ security: '1097', tradeDate: '2024-03-01' }); + + const result = await page(); + + expect(result.nodes.map(node => dateToTimelessDateString(node.execution.trade_date))).toEqual([ + '2024-06-01', + '2024-03-01', + '2024-01-01', + ]); + }); + + it('reports the full match count alongside a page of it', async () => { + for (const day of ['01', '02', '03', '04', '05']) { + await insertExecution({ security: '1097', tradeDate: `2024-03-${day}` }); + } + + const first = await page({ limit: 2, page: 0 }); + const second = await page({ limit: 2, page: 1 }); + + expect(first.totalRecords).toBe(5); + expect(first.nodes).toHaveLength(2); + expect(second.totalRecords).toBe(5); + // Consecutive pages must not overlap or skip. + expect(second.nodes.map(node => node.id)).not.toEqual(first.nodes.map(node => node.id)); + }); + + /** + * The three identity filters name the same axis three ways, so they union. + * Intersecting them would make "this ISIN and that symbol" mean the empty + * overlap, which is never what a caller means. + */ + it('unions the identity filters rather than intersecting them', async () => { + await insertExecution({ security: '1097' }); + await insertExecution({ security: '2098' }); + + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { isins: [APPLE_ISIN], securityBusinessIds: [MSFT_BUSINESS_ID] }, + page: 0, + limit: 50, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + + expect(result.totalRecords).toBe(2); + }); + + it('matches symbols case-insensitively', async () => { + await insertExecution({ security: '1097' }); + + const businessesFor = async (symbol: string) => { + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { symbols: [symbol] }, + page: 0, + limit: 500, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + return new Set(result.nodes.map(node => node.securityBusinessId)); + }; + + // insertSecurityBusiness writes the symbol as 'EXMP', so the lowercase spelling + // has to reach it. Asserted by which security came back rather than by a count: + // with no ISIN filter and no RLS under a superuser connection, a concurrent + // suite's fixtures can share the result. + expect(await businessesFor('exmp')).toContain(APPLE_BUSINESS_ID); + expect(await businessesFor('nope')).not.toContain(APPLE_BUSINESS_ID); + }); + + it('ignores an id that is not one of this tenant security businesses', async () => { + await insertExecution({ security: '1097' }); + + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { securityBusinessIds: [OTHER_OWNER_ID] }, + page: 0, + limit: 50, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + + expect(result.totalRecords).toBe(0); + }); + + it('pushes the trade-date range into SQL', async () => { + await insertExecution({ security: '1097', tradeDate: '2024-01-15' }); + await insertExecution({ security: '1097', tradeDate: '2024-05-15' }); + + const bounded = await page({ + filters: { fromTradeDate: '2024-04-01', toTradeDate: '2024-06-30' }, + }); + + expect(bounded.totalRecords).toBe(1); + expect(dateToTimelessDateString(bounded.nodes[0]!.execution.trade_date)).toBe('2024-05-15'); + }); + + it('filters on the bank own labels for trade and transaction type', async () => { + await insertExecution({ security: '1097', tradeType: 'קניה' }); + await insertExecution({ security: '1097', tradeType: 'מכירה' }); + + expect((await page({ filters: { rawTradeTypes: ['מכירה'] } })).totalRecords).toBe(1); + expect((await page({ filters: { rawTransactionTypes: ['קניה'] } })).totalRecords).toBe(1); + // An empty list is "no restriction", not "match nothing" — the `is*` flag guard. + expect((await page({ filters: { rawTradeTypes: [] } })).totalRecords).toBe(2); + expect((await page()).totalRecords).toBe(2); + }); + + it('carries the security business on every row, so a flat list can be grouped', async () => { + await insertExecution({ security: '2098' }); + + const result = await page(); + + expect(result.nodes[0]!.securityBusinessId).toBe(MSFT_BUSINESS_ID); + // Not asked for, so no pairing was attempted. + expect(result.nodes[0]!.transaction).toBeNull(); + }); + + it('refuses to pair charge links across more securities than it can', async () => { + // Two are seeded already; take the tenant past the cap so an unnarrowed + // request has to be refused. + for (const [index, id] of OVERFLOW_BUSINESS_IDS.entries()) { + await insertSecurityBusiness({ + id, + isin: `ZZ9${String(index).padStart(9, '0')}`, + securityKeys: [`90${index}`], + }); + } + await insertExecution({ security: '1097' }); + + await expect( + createProvider([]).getSecurityExecutionsPage({ + filters: {}, + page: 0, + limit: 50, + includeCharges: true, + ownerId: TEST_OWNER_ID, + }), + ).rejects.toThrow(/more than the \d+ it can pair at once/); + }); + + it('pairs charge links when the filter names few enough securities', async () => { + await insertExecution({ security: '1097' }); + + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { isins: [APPLE_ISIN] }, + page: 0, + limit: 50, + includeCharges: true, + ownerId: TEST_OWNER_ID, + }); + + expect(result.totalRecords).toBe(1); + expect(result.nodes[0]!.securityBusinessId).toBe(APPLE_BUSINESS_ID); + }); + + /** + * The two paths must agree about what page 1 is, or paging with and without + * charge links would return different rows for the same request. + */ + it('orders the match path identically to the SQL path', async () => { + await insertExecution({ security: '1097', tradeDate: '2024-01-01' }); + await insertExecution({ security: '1097', tradeDate: '2024-06-01' }); + await insertExecution({ security: '1097', tradeDate: '2024-03-01' }); + + const pushdown = await page({ filters: { isins: [APPLE_ISIN] } }); + const matched = await createProvider([]).getSecurityExecutionsPage({ + filters: { isins: [APPLE_ISIN] }, + page: 0, + limit: 50, + includeCharges: true, + ownerId: TEST_OWNER_ID, + }); + + expect(matched.nodes.map(node => node.id)).toEqual(pushdown.nodes.map(node => node.id)); + }); + + it('applies the date and type filters on the match path too', async () => { + await insertExecution({ security: '1097', tradeDate: '2024-01-15', tradeType: 'קניה' }); + await insertExecution({ security: '1097', tradeDate: '2024-05-15', tradeType: 'מכירה' }); + + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { + isins: [APPLE_ISIN], + fromTradeDate: '2024-04-01', + rawTradeTypes: ['מכירה'], + }, + page: 0, + limit: 50, + includeCharges: true, + ownerId: TEST_OWNER_ID, + }); + + expect(result.totalRecords).toBe(1); + expect(result.nodes[0]!.execution.trade_type).toBe('מכירה'); + }); + + /** + * Tenant isolation is deliberately NOT asserted here: this suite connects as a + * superuser, who bypasses RLS, so a green provider-level assertion would prove + * nothing. The `multi-business read scope` cases below run the same tables under + * the non-superuser role the server actually operates behind. + */ +}); + +/** + * A Poalim security key is unique only *within* an owner. + * + * `security_identifiers` is unique on `(owner_id, identifier_type, identifier_value)`, so two + * businesses that both trade one security each carry it under the same key — the ordinary case for + * a tenant with more than one business, not an exotic one. Reads follow the request's whole + * business scope, so both rows are visible at once and a key-only lookup has nothing to tell them + * apart: it keeps whichever was seen last and files one business's trades under the other's + * security. + * + * These cases run as the suite's superuser, which bypasses RLS — which is exactly the widest + * version of the situation, and what makes them a regression test for the join rather than for the + * policy. + */ +describe('two businesses trading the same security', () => { + const SHARED_KEY = '1097'; + + beforeEach(async () => { + await insertSecurityBusiness({ + id: APPLE_BUSINESS_ID, + isin: 'ZZ0000000621', + engName: 'Shared Security (mine)', + securityKeys: [SHARED_KEY], + }); + await insertSecurityBusiness({ + id: MSFT_BUSINESS_ID, + isin: 'ZZ0000000622', + engName: 'Shared Security (theirs)', + securityKeys: [SHARED_KEY], + ownerId: OTHER_OWNER_ID, + }); + await insertExecution({ security: SHARED_KEY, tradeDate: '2024-03-01' }); + await insertExecution({ + ownerId: OTHER_OWNER_ID, + security: SHARED_KEY, + tradeDate: '2024-03-02', + }); + }); + + it('gives each business only its own executions', async () => { + const executionsByBusinessId = await createProvider([]).getExecutionsBySecurityBusiness(); + + expect(executionsByBusinessId.get(APPLE_BUSINESS_ID)).toHaveLength(1); + expect(executionsByBusinessId.get(MSFT_BUSINESS_ID)).toHaveLength(1); + expect(executionsByBusinessId.get(APPLE_BUSINESS_ID)![0]!.owner_id).toBe(TEST_OWNER_ID); + expect(executionsByBusinessId.get(MSFT_BUSINESS_ID)![0]!.owner_id).toBe(OTHER_OWNER_ID); + }); + + it('keeps a filtered page to the business it named', async () => { + const result = await createProvider([]).getSecurityExecutionsPage({ + filters: { securityBusinessIds: [APPLE_BUSINESS_ID] }, + page: 0, + limit: 50, + includeCharges: false, + ownerId: TEST_OWNER_ID, + }); + + expect(result.totalRecords).toBe(1); + expect(result.nodes[0]!.securityBusinessId).toBe(APPLE_BUSINESS_ID); + expect(result.nodes[0]!.execution.owner_id).toBe(TEST_OWNER_ID); + }); + + it("keeps one business's history out of the other's", async () => { + const { executions } = await createProvider([]).getSecurityBusinessHistory( + MSFT_BUSINESS_ID, + OTHER_OWNER_ID, + ); + + expect(executions).toHaveLength(1); + expect(executions[0]!.owner_id).toBe(OTHER_OWNER_ID); + }); + + it('resolves the reference details per owner', async () => { + await insertSecurity({ securityKey: SHARED_KEY, engName: 'Mine' }); + await insertSecurity({ + ownerId: OTHER_OWNER_ID, + securityKey: SHARED_KEY, + engName: 'Theirs', + }); + + const provider = createProvider([]); + const [mine, theirs] = await Promise.all([ + provider.securityByKeyLoader.load({ ownerId: TEST_OWNER_ID, securityKey: SHARED_KEY }), + provider.securityByKeyLoader.load({ ownerId: OTHER_OWNER_ID, securityKey: SHARED_KEY }), + ]); + + expect(mine?.eng_name).toBe('Mine'); + expect(theirs?.eng_name).toBe('Theirs'); + }); +}); + +/** + * The read predicate on all four securities tables was pinned to the singular + * `get_current_business_id()` until this was fixed, so a request whose authorized + * scope spanned several businesses silently saw only one of them. A + * provider-level assertion proves nothing — this suite connects as a superuser, + * who bypasses RLS — so this runs under the non-superuser role the server + * actually operates behind. + */ +describe('multi-business read scope', () => { + async function readUnderScope( + table: string, + scope: string[] | null, + currentBusinessId = TEST_OWNER_ID, + ) { + const client = await pool.connect(); + try { + await client.query('BEGIN'); + // Session variables must be set as superuser, before privileges are dropped. + await client.query(`SELECT set_config('app.current_business_id', $1, true)`, [ + currentBusinessId, + ]); + await client.query(`SELECT set_config('app.current_business_scope', $1, true)`, [ + scope ? `{${scope.join(',')}}` : '', + ]); + + return await runAsRlsRole(client, async () => { + const result = await client.query( + `SELECT owner_id FROM accounter_schema.${table} ORDER BY owner_id`, + ); + return result.rows.map((row: { owner_id: string }) => row.owner_id); + }); + } finally { + await client.query('ROLLBACK'); + client.release(); + } + } + + beforeEach(async () => { + await insertSecurityBusiness({ + id: APPLE_BUSINESS_ID, + isin: 'ZZ0000000611', + securityKeys: ['1097'], + }); + await insertSecurityBusiness({ + id: MSFT_BUSINESS_ID, + isin: 'ZZ0000000612', + securityKeys: ['2098'], + ownerId: OTHER_OWNER_ID, + }); + await insertSecurity({ securityKey: '1097' }); + await insertSecurity({ ownerId: OTHER_OWNER_ID, securityKey: '2098' }); + await insertExecution({ security: '1097' }); + await insertExecution({ ownerId: OTHER_OWNER_ID, security: '2098' }); + }); + + const TABLES = [ + 'poalim_securities', + 'poalim_securities_transactions', + 'businesses_securities', + 'security_identifiers', + ]; + + it.each(TABLES)('%s returns every business in the scope', async table => { + const owners = await readUnderScope(table, [TEST_OWNER_ID, OTHER_OWNER_ID]); + + expect(new Set(owners)).toEqual(new Set([TEST_OWNER_ID, OTHER_OWNER_ID])); + }); + + it.each(TABLES)('%s narrows to a single-business scope', async table => { + const owners = await readUnderScope(table, [TEST_OWNER_ID]); + + expect(new Set(owners)).toEqual(new Set([TEST_OWNER_ID])); + }); + + /** + * `get_current_business_scope()` falls back to `ARRAY[get_current_business_id()]` + * when the GUC is unset, so a caller that never sets a scope behaves exactly as + * it did before the predicate was widened. + */ + it.each(TABLES)('%s falls back to the single business when no scope is set', async table => { + const owners = await readUnderScope(table, null); + + expect(new Set(owners)).toEqual(new Set([TEST_OWNER_ID])); + }); + + /** + * Reads follow the scope; writes do not. + * + * `USING` selects the rows a statement may act on, and Postgres consults it for DELETE and + * UPDATE as well as SELECT — `WITH CHECK` only constrains the *new* values. So the permissive + * scope-wide policy on its own would let a session delete another in-scope business's row, or + * update one into its own ownership. The restrictive per-command policies are what stop that, + * and these cases are the proof: the row is readable, and neither writable. + */ + async function writeUnderScope( + statement: 'delete' | 'update', + table: string, + scope: string[], + targetOwnerId: string, + currentBusinessId = TEST_OWNER_ID, + ): Promise { + const client = await pool.connect(); + try { + await client.query('BEGIN'); + await client.query(`SELECT set_config('app.current_business_id', $1, true)`, [ + currentBusinessId, + ]); + await client.query(`SELECT set_config('app.current_business_scope', $1, true)`, [ + `{${scope.join(',')}}`, + ]); + + return await runAsRlsRole(client, async () => { + const sql = + statement === 'delete' + ? `DELETE FROM accounter_schema.${table} WHERE owner_id = $1` + : `UPDATE accounter_schema.${table} SET owner_id = owner_id WHERE owner_id = $1`; + const result = await client.query(sql, [targetOwnerId]); + return result.rowCount ?? 0; + }); + } finally { + await client.query('ROLLBACK'); + client.release(); + } + } + + it.each(TABLES)('%s refuses to delete another business in the scope', async table => { + const scope = [TEST_OWNER_ID, OTHER_OWNER_ID]; + + // Visible... + expect(await readUnderScope(table, scope)).toContain(OTHER_OWNER_ID); + // ...and still not deletable, because the write target is the other business. + expect(await writeUnderScope('delete', table, scope, OTHER_OWNER_ID)).toBe(0); + }); + + it.each(TABLES)('%s refuses to update another business in the scope', async table => { + const scope = [TEST_OWNER_ID, OTHER_OWNER_ID]; + + expect(await writeUnderScope('update', table, scope, OTHER_OWNER_ID)).toBe(0); + }); + + it.each(TABLES)('%s still lets the write target delete its own rows', async table => { + const scope = [TEST_OWNER_ID, OTHER_OWNER_ID]; + + expect(await writeUnderScope('delete', table, scope, TEST_OWNER_ID)).toBeGreaterThan(0); + }); +}); diff --git a/packages/server/src/modules/foreign-securities/providers/__tests__/security-businesses.integration.test.ts b/packages/server/src/modules/foreign-securities/providers/__tests__/security-businesses.integration.test.ts index ea7b8ed94..68a4d7824 100644 --- a/packages/server/src/modules/foreign-securities/providers/__tests__/security-businesses.integration.test.ts +++ b/packages/server/src/modules/foreign-securities/providers/__tests__/security-businesses.integration.test.ts @@ -296,6 +296,7 @@ describe('linkIdentifier', () => { await provider.linkIdentifier(security.id, 'POALIM_SECURITY_KEY', '5129523'); const found = await provider.getSecurityBusinessByIdentifierLoader.load({ + ownerId: TEST_OWNER_ID, type: 'POALIM_SECURITY_KEY', value: '5129523', }); @@ -321,6 +322,7 @@ describe('linkIdentifier', () => { const provider = createProvider(); const found = await provider.getSecurityBusinessByIdentifierLoader.load({ + ownerId: TEST_OWNER_ID, type: 'POALIM_SECURITY_KEY', value: '0000001', }); diff --git a/packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts b/packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts index ad534c959..be2882844 100644 --- a/packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts +++ b/packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts @@ -1,6 +1,8 @@ import DataLoader from 'dataloader'; +import { GraphQLError } from 'graphql'; import { Injectable, Scope } from 'graphql-modules'; import { sql } from '@pgtyped/runtime'; +import { dateToTimelessDateString } from '../../../shared/helpers/misc.js'; import { TenantAwareDBClient } from '../../app-providers/tenant-db-client.js'; import { FinancialAccountsProvider } from '../../financial-accounts/providers/financial-accounts.provider.js'; import { FinancialBankAccountsProvider } from '../../financial-accounts/providers/financial-bank-accounts.provider.js'; @@ -14,27 +16,35 @@ import { import { extractSecurityKeys } from '../helpers/security-key.helper.js'; import type { ChargeSecurityProto, + IGetFilteredSecurityExecutionsQuery, IGetSecuritiesByKeysQuery, - IGetSecurityExecutionsByKeysQuery, + IGetSecurityExecutionsByBusinessIdsQuery, IGetSecurityExecutionsQuery, + PaginatedSecurityExecutionsProto, SecurityExecutionRow, + SecurityExecutionsFilterInput, + SecurityHistoryExecutionProto, SecurityRow, } from '../types.js'; import { SecurityBusinessesProvider } from './security-businesses.provider.js'; /** - * No owner_id predicate: accounter_schema.poalim_securities is FORCE RLS with a - * tenant_isolation policy, so going through TenantAwareDBClient scopes this to the - * acting tenant. The dedup key includes branch/account, so one tenant can hold the - * same security in several accounts — DISTINCT ON keeps the freshest scrape. + * Deduped per owner as well as per key, and returning the owner. + * + * RLS scopes this to the request's business *scope*, which can span several businesses, and the + * bank's key is only unique within one of them — two businesses trading the same security each + * have a reference row for it. Deduping on the key alone would hand one business the other's + * reference details. The dedup key also includes branch/account in the table itself, so one owner + * can hold the same security in several accounts — `DISTINCT ON` keeps the freshest scrape. */ const getSecuritiesByKeys = sql` - SELECT DISTINCT ON (security_key) - id, security_key, eng_name, heb_name, symbol, eng_symbol, heb_symbol, + SELECT DISTINCT ON (owner_id, security_key) + id, owner_id, security_key, eng_name, heb_name, symbol, eng_symbol, heb_symbol, item_type, stock_type, exchange, currency_code, is_etf, is_foreign, as_of_date FROM accounter_schema.poalim_securities - WHERE security_key = ANY($securityKeys!) - ORDER BY security_key, as_of_date DESC;`; + WHERE owner_id = $ownerId! + AND security_key = ANY($securityKeys!) + ORDER BY owner_id, security_key, as_of_date DESC;`; /** * A prefilter, not the match itself: the ANY(...) predicates form a cross-product over the @@ -47,6 +57,7 @@ const getSecuritiesByKeys = sql` const getSecurityExecutions = sql` SELECT id, + owner_id, security, bank_number, branch_number, @@ -81,45 +92,151 @@ const getSecurityExecutions = sql` AND value_date = ANY($valueDates!);`; /** - * Every ingested execution of the given securities, unbounded by charge or date — the whole - * life of an instrument, which is what its business page shows. RLS scopes it to the tenant. + * Every ingested execution of the given security businesses, unbounded by charge or date — the + * whole life of an instrument, which is what its page shows. + * + * Addressed by security *business* rather than by Poalim key, through a join on the identifier + * bridge. A key is only unique within an owner — `security_identifiers` is unique on + * `(owner_id, identifier_type, identifier_value)`, and the same security traded by two of a + * tenant's businesses carries the same key under each. Reads follow the request's whole business + * scope rather than one business, so filtering on the key alone would pull one business's + * executions into another's history and position. The join carries the owner on both sides and + * hands back the business each row belongs to, resolving the relation once, in SQL, rather than + * rebuilding it from a map that cannot express it. + * + * Tenant scoping is still RLS, on both tables. */ -const getSecurityExecutionsByKeys = sql` +const getSecurityExecutionsByBusinessIds = sql` SELECT - id, - security, - bank_number, - branch_number, - account_number, - trade_date, - value_date, - settlement_date, - payment_date, - trade_type, - transaction_type, - nv, - trade_price, - trade_gross_value_trade_currency, - net_value_trade_currency, - net_value_settlement_currency, - net_value_nis, - trade_currency, - settlement_currency, - trade_commission_value_trade_currency, - management_fees_value_trade_currency, - israe_tax_value, - nominal_profit_loss_nis, - real_profit_loss_nis, - payment_type, - symbol, - isin - FROM accounter_schema.poalim_securities_transactions - WHERE security = ANY($securities!) - ORDER BY trade_date, id;`; + t.id, + t.owner_id, + t.security, + t.bank_number, + t.branch_number, + t.account_number, + t.trade_date, + t.value_date, + t.settlement_date, + t.payment_date, + t.trade_type, + t.transaction_type, + t.nv, + t.trade_price, + t.trade_gross_value_trade_currency, + t.net_value_trade_currency, + t.net_value_settlement_currency, + t.net_value_nis, + t.trade_currency, + t.settlement_currency, + t.trade_commission_value_trade_currency, + t.management_fees_value_trade_currency, + t.israe_tax_value, + t.nominal_profit_loss_nis, + t.real_profit_loss_nis, + t.payment_type, + t.symbol, + t.isin, + si.business_id AS security_business_id + FROM accounter_schema.poalim_securities_transactions t + INNER JOIN accounter_schema.security_identifiers si + ON si.owner_id = t.owner_id + AND si.identifier_type = 'POALIM_SECURITY_KEY' + AND si.identifier_value = t.security + WHERE si.business_id IN $$businessIds + ORDER BY t.trade_date, t.id;`; + +/** + * The paginated, filtered slice `Query.securityExecutions` serves. + * + * Newest first, deliberately the opposite of `getSecurityExecutionsByBusinessIds`: that one + * feeds `calculateSecurityPosition`, which reads a position's currency off the *first* execution + * and so depends on chronological order. This one is read by a human asking what happened lately. + * + * Addressed by security business through the same identifier join, for the same reason — see the + * note there. Resolving the relation in SQL rather than after the fact also keeps + * `COUNT(*) OVER ()` honest: a row dropped in memory for belonging to another owner would leave a + * total that no longer matches what pagination can reach. + * + * Trade and transaction types are filtered on the bank's own labels — the caller passes GraphQL + * enums and the resolver translates them through `tradeTypeToRaw` / `transactionTypeToRaw`, so + * the Hebrew never appears outside the enum helper. + * + * `COUNT(*) OVER ()` rides on the returned rows, which means a request for a page past the end + * reports a total of 0 rather than the real count. That is the only inaccuracy, it costs a + * second round trip to fix, and it only misreports a page the caller invented — so it stands. + * + * Tenant scoping is RLS, as everywhere in this file. + */ +const getFilteredSecurityExecutions = sql` + SELECT + t.id, + t.owner_id, + t.security, + t.bank_number, + t.branch_number, + t.account_number, + t.trade_date, + t.value_date, + t.settlement_date, + t.payment_date, + t.trade_type, + t.transaction_type, + t.nv, + t.trade_price, + t.trade_gross_value_trade_currency, + t.net_value_trade_currency, + t.net_value_settlement_currency, + t.net_value_nis, + t.trade_currency, + t.settlement_currency, + t.trade_commission_value_trade_currency, + t.management_fees_value_trade_currency, + t.israe_tax_value, + t.nominal_profit_loss_nis, + t.real_profit_loss_nis, + t.payment_type, + t.symbol, + t.isin, + si.business_id AS security_business_id, + COUNT(*) OVER () AS total_count + FROM accounter_schema.poalim_securities_transactions t + INNER JOIN accounter_schema.security_identifiers si + ON si.owner_id = t.owner_id + AND si.identifier_type = 'POALIM_SECURITY_KEY' + AND si.identifier_value = t.security + WHERE si.business_id IN $$businessIds + AND ($isTradeTypes = 0 OR t.trade_type IN $$tradeTypes) + AND ($isTransactionTypes = 0 OR t.transaction_type IN $$transactionTypes) + AND ($fromTradeDate::DATE IS NULL OR t.trade_date >= $fromTradeDate) + AND ($toTradeDate::DATE IS NULL OR t.trade_date <= $toTradeDate) + ORDER BY t.trade_date DESC, t.id DESC + LIMIT $limit! OFFSET $offset!;`; + +/** + * How many securities `includeCharges` will pair at once. + * + * Each one costs its whole execution history plus a transactions query, because the pairing is + * only correct over a complete set (see `matchExecutionsToTransactions`). The cap is what keeps + * "every trade I ever made, with charges" from turning into a portfolio-wide fan-out. + */ +export const MAX_CHARGE_LINK_SECURITIES = 10; /** What the reverse match needs off a transaction, charge included so a row can link out. */ type MatchedTransaction = MatchableTransaction & { charge_id: string }; +/** + * A Poalim security key, qualified by the owner it belongs to. + * + * The bank's key is only unique within an owner, and reads follow the request's whole business + * scope rather than a single business — so the key alone cannot identify a security once a tenant + * has two businesses trading the same one. + */ +export type OwnedSecurityKey = { ownerId: string; securityKey: string }; + +function ownedKey(ownerId: string, securityKey: string): string { + return `${ownerId}:${securityKey}`; +} + @Injectable({ scope: Scope.Operation, global: true, @@ -133,15 +250,39 @@ export class ForeignSecuritiesProvider { private securityBusinessesProvider: SecurityBusinessesProvider, ) {} - private async batchSecuritiesByKeys(securityKeys: readonly string[]) { - const securities = await getSecuritiesByKeys.run({ securityKeys: [...securityKeys] }, this.db); - // DISTINCT ON in the query guarantees one row per key, so a plain Map is enough. - const securityByKey = new Map(securities.map(security => [security.security_key, security])); - return securityKeys.map(key => securityByKey.get(key) ?? null); + private async batchSecuritiesByKeys(keys: readonly OwnedSecurityKey[]) { + // One query per owner; in practice a batch carries a single one, since a charge belongs to + // exactly one business. + const keysByOwner = new Map>(); + for (const key of keys) { + const values = keysByOwner.get(key.ownerId); + if (values) { + values.add(key.securityKey); + } else { + keysByOwner.set(key.ownerId, new Set([key.securityKey])); + } + } + + const securityByOwnedKey = new Map(); + await Promise.all( + [...keysByOwner].map(async ([ownerId, securityKeys]) => { + const securities = await getSecuritiesByKeys.run( + { ownerId, securityKeys: [...securityKeys] }, + this.db, + ); + // DISTINCT ON guarantees one row per (owner, key), so a plain Map is enough. + for (const security of securities) { + securityByOwnedKey.set(ownedKey(security.owner_id, security.security_key), security); + } + }), + ); + + return keys.map(key => securityByOwnedKey.get(ownedKey(key.ownerId, key.securityKey)) ?? null); } - public securityByKeyLoader = new DataLoader((keys: readonly string[]) => - this.batchSecuritiesByKeys(keys), + public securityByKeyLoader = new DataLoader( + (keys: readonly OwnedSecurityKey[]) => this.batchSecuritiesByKeys(keys), + { cacheKeyFn: key => ownedKey(key.ownerId, key.securityKey) }, ); /** @@ -231,18 +372,8 @@ export class ForeignSecuritiesProvider { * the other end. */ public async getSecurityBusinessHistory(businessId: string, ownerId: string) { - const identifiers = - await this.securityBusinessesProvider.getIdentifiersByBusinessIdLoader.load(businessId); - const securityKeys = identifiers - .filter(identifier => identifier.identifier_type === 'POALIM_SECURITY_KEY') - .map(identifier => identifier.identifier_value); - - if (securityKeys.length === 0) { - return { executions: [], transactionByExecutionId: new Map() }; - } - const [executions, transactions] = await Promise.all([ - getSecurityExecutionsByKeys.run({ securities: securityKeys }, this.db), + getSecurityExecutionsByBusinessIds.run({ businessIds: [businessId] }, this.db), this.transactionsProvider.getTransactionsByFilters({ businessIDs: [businessId], ownerIDs: [ownerId], @@ -267,9 +398,10 @@ export class ForeignSecuritiesProvider { * it belongs to. Each business gets an entry, so "nothing ingested" is distinguishable from * "not a security". * - * One query for the whole portfolio: `getSecurityExecutionsByKeys` already filters on - * `security = ANY(...)` and returns the key on every row, so the union of every business's - * Poalim keys can be asked for at once and split back up in memory. + * One query for the whole portfolio: `getSecurityExecutionsByBusinessIds` joins the identifier + * bridge, so every business can be asked for at once and each row already knows which one it + * belongs to — including when two businesses trade the same security under the same Poalim key, + * which the key alone cannot tell apart. * * Unlike `getSecurityBusinessHistory` this never looks at transactions or accounts — those * exist only to pair an execution with the cash movement behind it, which a position does not @@ -285,51 +417,199 @@ export class ForeignSecuritiesProvider { return executionsByBusinessId; } - // One batched query behind the loader, not one per business. - const identifierLists = - await this.securityBusinessesProvider.getIdentifiersByBusinessIdLoader.loadMany( - securityBusinesses.map(securityBusiness => securityBusiness.id), - ); - - // The executions table is keyed by Poalim's security key, and one business can carry several - // of them — that is what the identifiers bridge is for — so invert into key -> business. The - // unique index on (owner_id, identifier_type, identifier_value) is what makes one key resolve - // to exactly one business, so a plain Map is enough. - const businessIdByKey = new Map(); - for (const identifiers of identifierLists) { - // loadMany reports a rejected key as an Error rather than throwing; one bad business must - // not blank the whole list. - if (identifiers instanceof Error) { - continue; - } - for (const identifier of identifiers) { - if (identifier.identifier_type === 'POALIM_SECURITY_KEY') { - businessIdByKey.set(identifier.identifier_value, identifier.business_id); - } - } + // ORDER BY trade_date, id is global to the result, so each business's slice stays + // chronological — which is what `calculateSecurityPosition` reads its currency off. + const executions = await getSecurityExecutionsByBusinessIds.run( + { businessIds: securityBusinesses.map(securityBusiness => securityBusiness.id) }, + this.db, + ); + + for (const execution of executions) { + // The bucket comes from the join, so an execution can only ever land under the business + // whose owner *and* key it matches. + executionsByBusinessId.get(execution.security_business_id)?.push(execution); } - if (businessIdByKey.size === 0) { - return executionsByBusinessId; + return executionsByBusinessId; + } + + /** + * Which securities a filter names. + * + * `securityBusinessIds`, `isins` and `symbols` are three ways of naming the same axis, so they + * union with each other rather than intersecting — asking for one ISIN and one symbol means + * both securities, not the empty overlap. Naming none of them means every security. + * + * Resolved against the request-memoized `getAllSecurityBusinesses()` rather than with three + * more queries: it is one round trip already paid for, and going through it means an id that + * is not a security business of this tenant resolves to nothing instead of reaching the + * executions feed. + */ + private async resolveFilterSecurityBusinessIds( + filters: SecurityExecutionsFilterInput, + ): Promise { + const securityBusinesses = await this.securityBusinessesProvider.getAllSecurityBusinesses(); + + const requestedIds = new Set(filters.securityBusinessIds?.filter(Boolean) ?? []); + const requestedIsins = new Set(filters.isins?.filter(Boolean) ?? []); + // The bank is inconsistent about symbol case across its two feeds; match case-insensitively. + const requestedSymbols = new Set( + (filters.symbols?.filter(Boolean) ?? []).map(symbol => symbol.toLowerCase()), + ); + + if (requestedIds.size === 0 && requestedIsins.size === 0 && requestedSymbols.size === 0) { + return securityBusinesses.map(securityBusiness => securityBusiness.id); + } + + return securityBusinesses + .filter( + securityBusiness => + requestedIds.has(securityBusiness.id) || + requestedIsins.has(securityBusiness.isin) || + (securityBusiness.symbol != null && + requestedSymbols.has(securityBusiness.symbol.toLowerCase())), + ) + .map(securityBusiness => securityBusiness.id); + } + + /** + * A page of executions across securities, newest first. + * + * Two paths, because charge links and pagination do not compose. Without them the filter + * pushes straight into SQL and the page is a `LIMIT`/`OFFSET` slice. With them the pairing has + * to see a security's *whole* history — `matchExecutionsToTransactions` is greedy and + * one-to-one over the sets it is handed, so pairing a page's slice would let an execution on + * page 2 claim the cash movement that belongs to one on page 1, and the same execution would + * report a different charge at a different page size. So that path reuses + * `getSecurityBusinessHistory` per security, unpaginated, and slices in memory — which is why + * it is capped at {@link MAX_CHARGE_LINK_SECURITIES} securities. + */ + public async getSecurityExecutionsPage(params: { + filters: SecurityExecutionsFilterInput; + page: number; + limit: number; + includeCharges: boolean; + ownerId: string; + }): Promise { + const { filters, page, limit, includeCharges, ownerId } = params; + const empty: PaginatedSecurityExecutionsProto = { + nodes: [], + totalRecords: 0, + currentPage: page, + pageSize: limit, + }; + + const businessIds = await this.resolveFilterSecurityBusinessIds(filters); + if (businessIds.length === 0) { + return empty; + } + + if (includeCharges) { + if (businessIds.length > MAX_CHARGE_LINK_SECURITIES) { + throw new GraphQLError( + `includeCharges resolves to ${businessIds.length} securities, more than the ${MAX_CHARGE_LINK_SECURITIES} it can pair at once — narrow securityBusinessIds, isins or symbols, or drop includeCharges.`, + ); + } + return this.chargeLinkedExecutionsPage(businessIds, filters, page, limit, ownerId); } - // ORDER BY trade_date, id is global to the result, so each key's slice stays chronological — - // which is what `calculateSecurityPosition` reads its currency off. - const executions = await getSecurityExecutionsByKeys.run( - { securities: [...businessIdByKey.keys()] }, + const rawTradeTypes = filters.rawTradeTypes?.filter(Boolean) ?? []; + const rawTransactionTypes = filters.rawTransactionTypes?.filter(Boolean) ?? []; + + const rows = await getFilteredSecurityExecutions.run( + { + businessIds: [...businessIds], + isTradeTypes: rawTradeTypes.length ? 1 : 0, + isTransactionTypes: rawTransactionTypes.length ? 1 : 0, + // pgtyped requires a non-empty array for `IN $$list`; the matching `is*` flag + // short-circuits the predicate, so the placeholder is never compared. + tradeTypes: rawTradeTypes.length ? [...rawTradeTypes] : [null], + transactionTypes: rawTransactionTypes.length ? [...rawTransactionTypes] : [null], + fromTradeDate: filters.fromTradeDate ?? null, + toTradeDate: filters.toTradeDate ?? null, + limit, + offset: page * limit, + }, this.db, ); - for (const execution of executions) { - const businessId = businessIdByKey.get(execution.security); - if (!businessId) { - continue; + return { + // `COUNT(*) OVER ()` is identical on every row of the page. + totalRecords: rows.length ? Number(rows[0].total_count) : 0, + currentPage: page, + pageSize: limit, + // The security business comes off the join, so it is known for every row rather than + // looked up afterwards. + nodes: rows.map(row => ({ + id: row.id, + execution: row, + securityBusinessId: row.security_business_id, + transaction: null, + })), + }; + } + + /** + * The `includeCharges` path: every named security's complete history, paired, then filtered, + * ordered and sliced in memory. See {@link getSecurityExecutionsPage} for why it cannot be + * done in SQL. + */ + private async chargeLinkedExecutionsPage( + businessIds: readonly string[], + filters: SecurityExecutionsFilterInput, + page: number, + limit: number, + ownerId: string, + ): Promise { + const histories = await Promise.all( + businessIds.map(businessId => this.getSecurityBusinessHistory(businessId, ownerId)), + ); + + const rawTradeTypes = new Set(filters.rawTradeTypes?.filter(Boolean) ?? []); + const rawTransactionTypes = new Set(filters.rawTransactionTypes?.filter(Boolean) ?? []); + + const matched: SecurityHistoryExecutionProto[] = []; + for (const [index, { executions, transactionByExecutionId }] of histories.entries()) { + const securityBusinessId = businessIds[index]!; + for (const execution of executions) { + // Both sides are calendar dates; the timeless string compares as the day, which the raw + // Date does not once a DST boundary is between them. + const tradeDate = dateToTimelessDateString(execution.trade_date); + if (filters.fromTradeDate && tradeDate < filters.fromTradeDate) { + continue; + } + if (filters.toTradeDate && tradeDate > filters.toTradeDate) { + continue; + } + if (rawTradeTypes.size && !rawTradeTypes.has(execution.trade_type)) { + continue; + } + if (rawTransactionTypes.size && !rawTransactionTypes.has(execution.transaction_type)) { + continue; + } + matched.push({ + id: execution.id, + execution, + securityBusinessId, + transaction: transactionByExecutionId.get(execution.id) ?? null, + }); } - // An identifier can outlive the business it pointed at; skip rather than invent a bucket. - executionsByBusinessId.get(businessId)?.push(execution); } - return executionsByBusinessId; + // Newest first, matching the SQL path's ORDER BY exactly so the two paths cannot disagree + // about what page 1 is. + matched.sort( + (a, b) => + b.execution.trade_date.getTime() - a.execution.trade_date.getTime() || + b.execution.id.localeCompare(a.execution.id), + ); + + return { + nodes: matched.slice(page * limit, (page + 1) * limit), + totalRecords: matched.length, + currentPage: page, + pageSize: limit, + }; } /** @@ -337,7 +617,10 @@ export class ForeignSecuritiesProvider { * description carries. Keys with no ingested row are still returned, with a null * `details`, so a stale or missing scrape is visible instead of silently dropping data. */ - public async getChargeSecurities(chargeId: string): Promise { + public async getChargeSecurities( + chargeId: string, + ownerId: string, + ): Promise { const transactions = await this.transactionsProvider.transactionsByChargeIDLoader.load(chargeId); @@ -359,7 +642,7 @@ export class ForeignSecuritiesProvider { const keys = [...transactionIdsByKey.keys()].sort(); const [details, executionsByKey] = await Promise.all([ - this.securityByKeyLoader.loadMany(keys), + this.securityByKeyLoader.loadMany(keys.map(securityKey => ({ ownerId, securityKey }))), this.getMatchedExecutions(transactions, keys), ]); @@ -367,6 +650,7 @@ export class ForeignSecuritiesProvider { const detail = details[index]; return { id: `${chargeId}-${key}`, + ownerId, securityKey: key, // loadMany surfaces a rejected key as an Error rather than throwing; treat it // the same as "not ingested" so one bad key can't blank the whole section. diff --git a/packages/server/src/modules/foreign-securities/providers/security-businesses.provider.ts b/packages/server/src/modules/foreign-securities/providers/security-businesses.provider.ts index 0f3c98254..877f69456 100644 --- a/packages/server/src/modules/foreign-securities/providers/security-businesses.provider.ts +++ b/packages/server/src/modules/foreign-securities/providers/security-businesses.provider.ts @@ -42,12 +42,23 @@ const getSecurityBusinessesByIsins = sql` FROM accounter_schema.businesses_securities WHERE isin = ANY($isins!);`; +/** + * An explicit `owner_id` predicate, unlike the rest of this file. + * + * RLS narrows to the request's *scope*, which can span several businesses, while this lookup has + * to answer for one owner: `security_identifiers` is unique on + * `(owner_id, identifier_type, identifier_value)`, so the same Poalim key legitimately exists + * under two of a tenant's businesses when both trade the security. Without the predicate the + * batch would see both rows and keep whichever was written last, silently attaching one + * business's trade to the other's security. + */ const getSecurityBusinessesByIdentifiers = sql` - SELECT si.identifier_value, bs.* + SELECT si.identifier_value, si.owner_id AS identifier_owner_id, bs.* FROM accounter_schema.security_identifiers si INNER JOIN accounter_schema.businesses_securities bs ON bs.id = si.business_id - WHERE si.identifier_type = $identifierType! + WHERE si.owner_id = $ownerId! + AND si.identifier_type = $identifierType! AND si.identifier_value = ANY($identifierValues!);`; const getSecurityIdentifiersByBusinessIds = sql` @@ -89,11 +100,34 @@ function toCurrency(rawCurrency: string | null | undefined): Currency | null { return label ? formatCurrency(label, true) : null; } +/** + * A source's name for a security, *within an owner*. + * + * The owner is not optional: `(owner_id, identifier_type, identifier_value)` is what the relation + * is unique on, and reads span the request's whole business scope. A key without it cannot + * identify one security business. + */ export type IdentifierKey = { + ownerId: string; type: SecurityIdentifierType; value: string; }; +/** Cache/lookup key for the identifier relation, which is unique on all three parts. */ +function identifierCacheKey(ownerId: string, type: SecurityIdentifierType, value: string): string { + return `${ownerId}:${type}:${value}`; +} + +/** Batch grouping key — one query per (owner, identifier type). */ +function identifierGroupKey(ownerId: string, type: SecurityIdentifierType): string { + return `${ownerId}:${type}`; +} + +function splitIdentifierGroupKey(group: string): [string, SecurityIdentifierType] { + const separator = group.indexOf(':'); + return [group.slice(0, separator), group.slice(separator + 1) as SecurityIdentifierType]; +} + @Injectable({ scope: Scope.Operation, global: true, @@ -143,36 +177,41 @@ export class SecurityBusinessesProvider { } private async batchSecurityBusinessesByIdentifiers(keys: readonly IdentifierKey[]) { - // One query per identifier type; in practice a batch carries a single type. - const valuesByType = new Map>(); + // One query per (owner, identifier type); in practice a batch carries a single pair. + const valuesByOwnerAndType = new Map>(); for (const key of keys) { - const values = valuesByType.get(key.type); + const group = identifierGroupKey(key.ownerId, key.type); + const values = valuesByOwnerAndType.get(group); if (values) { values.add(key.value); } else { - valuesByType.set(key.type, new Set([key.value])); + valuesByOwnerAndType.set(group, new Set([key.value])); } } const found = new Map(); await Promise.all( - [...valuesByType].map(async ([identifierType, values]) => { + [...valuesByOwnerAndType].map(async ([group, values]) => { + const [ownerId, identifierType] = splitIdentifierGroupKey(group); const rows = await getSecurityBusinessesByIdentifiers.run( - { identifierType, identifierValues: [...values] }, + { ownerId, identifierType, identifierValues: [...values] }, this.db, ); - for (const { identifier_value, ...securityBusiness } of rows) { - found.set(`${identifierType}:${identifier_value}`, securityBusiness); + for (const { identifier_value, identifier_owner_id, ...securityBusiness } of rows) { + found.set( + identifierCacheKey(identifier_owner_id, identifierType, identifier_value), + securityBusiness, + ); } }), ); - return keys.map(key => found.get(`${key.type}:${key.value}`) ?? null); + return keys.map(key => found.get(identifierCacheKey(key.ownerId, key.type, key.value)) ?? null); } public getSecurityBusinessByIdentifierLoader = new DataLoader( (keys: readonly IdentifierKey[]) => this.batchSecurityBusinessesByIdentifiers(keys), - { cacheKeyFn: key => `${key.type}:${key.value}` }, + { cacheKeyFn: key => identifierCacheKey(key.ownerId, key.type, key.value) }, ); private async batchIdentifiersByBusinessIds(businessIds: readonly string[]) { @@ -383,6 +422,7 @@ export class SecurityBusinessesProvider { this.db, ); this.getSecurityBusinessByIdentifierLoader.clear({ + ownerId, type: identifierType, value: identifierValue, }); diff --git a/packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts b/packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts index dbab5ee2e..2e95bd198 100644 --- a/packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts +++ b/packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts @@ -11,6 +11,7 @@ import { toSecurityTransactionType, } from '../helpers/security-execution-enums.helper.js'; import { ForeignSecuritiesProvider } from '../providers/foreign-securities.provider.js'; +import { SecurityBusinessesProvider } from '../providers/security-businesses.provider.js'; import type { ForeignSecuritiesModule, SecurityExecutionRow } from '../types.js'; /** @@ -36,7 +37,9 @@ export const foreignSecuritiesResolvers: ForeignSecuritiesModule.Resolvers = { ForeignSecuritiesCharge: { securities: async (dbCharge, _, { injector }) => { try { - return await injector.get(ForeignSecuritiesProvider).getChargeSecurities(dbCharge.id); + return await injector + .get(ForeignSecuritiesProvider) + .getChargeSecurities(dbCharge.id, dbCharge.owner_id); } catch (e) { throw errorSimplifier(`Error fetching securities for charge ${dbCharge.id}`, e); } @@ -46,6 +49,19 @@ export const foreignSecuritiesResolvers: ForeignSecuritiesModule.Resolvers = { id: chargeSecurity => chargeSecurity.id, securityKey: chargeSecurity => chargeSecurity.securityKey, details: chargeSecurity => chargeSecurity.details, + // The bridge from the bank's key to the security's own identity: `security_identifiers` maps + // POALIM_SECURITY_KEY -> the ISIN-keyed security business, which is what the holdings list + // and the executions query are addressed by. Null is a real answer — the reference feed can + // be ingested before the executions that create a security business. + securityBusiness: async (chargeSecurity, _, { injector }) => + (await injector.get(SecurityBusinessesProvider).getSecurityBusinessByIdentifierLoader.load({ + // Scoped to the charge's owner: the key is unique only within one, so a request whose + // scope spans two businesses trading the same security would otherwise resolve to + // whichever row won the batch. + ownerId: chargeSecurity.ownerId, + type: 'POALIM_SECURITY_KEY', + value: chargeSecurity.securityKey, + })) ?? null, // Transaction concrete types are mapped to their id (see codegen.ts mappers). transactions: chargeSecurity => chargeSecurity.transactionIds, executions: chargeSecurity => chargeSecurity.executions, diff --git a/packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts b/packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts index 48085afc4..6423a9000 100644 --- a/packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts +++ b/packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts @@ -4,11 +4,21 @@ import { formatFinancialAmount } from '../../../shared/helpers/amount.js'; import { AdminContextProvider } from '../../admin-context/providers/admin-context.provider.js'; import { ChargesProvider } from '../../charges/providers/charges.provider.js'; import { BusinessesProvider } from '../../financial-entities/providers/businesses.provider.js'; +import { + tradeTypeToRaw, + transactionTypeToRaw, +} from '../helpers/security-execution-enums.helper.js'; import { calculateSecurityPosition, isOpenPosition } from '../helpers/security-position.helper.js'; import { ForeignSecuritiesProvider } from '../providers/foreign-securities.provider.js'; import { SecurityBusinessesProvider } from '../providers/security-businesses.provider.js'; import type { ForeignSecuritiesModule } from '../types.js'; +/** + * Falls back to the schema default, which graphql-js applies for an omitted argument but not for + * an explicit `null`. + */ +const DEFAULT_SECURITY_EXECUTIONS_LIMIT = 100; + /** An amount the executions imply, or null when they imply nothing. */ const positionAmount = (value: number | null, currency: string | null) => value == null || currency == null ? null : formatFinancialAmount(value, currency); @@ -79,14 +89,58 @@ export const securityBusinessesResolvers: ForeignSecuritiesModule.Resolvers = { executions: executions.map(execution => ({ id: execution.id, execution, + securityBusinessId: businessId, transaction: transactionByExecutionId.get(execution.id) ?? null, })), }; }, + securityExecutions: async (_, { filters, page, limit, includeCharges }, { injector }) => { + const { ownerId } = await injector.get(AdminContextProvider).getVerifiedAdminContext(); + + return injector.get(ForeignSecuritiesProvider).getSecurityExecutionsPage({ + ownerId, + page: page ?? 0, + limit: limit ?? DEFAULT_SECURITY_EXECUTIONS_LIMIT, + includeCharges: includeCharges ?? false, + filters: { + securityBusinessIds: filters?.securityBusinessIds, + isins: filters?.isins, + symbols: filters?.symbols, + fromTradeDate: filters?.fromTradeDate, + toTradeDate: filters?.toTradeDate, + // Translated here rather than in the provider so the bank's Hebrew stays confined to + // the enum helper — the provider filters on labels and never learns what they say. + rawTradeTypes: filters?.tradeTypes?.map(tradeType => tradeTypeToRaw[tradeType]), + rawTransactionTypes: filters?.transactionTypes?.map( + transactionType => transactionTypeToRaw[transactionType], + ), + }, + }); + }, + }, + PaginatedSecurityExecutions: { + nodes: executionsPage => executionsPage.nodes, + pageInfo: executionsPage => ({ + totalRecords: executionsPage.totalRecords, + totalPages: Math.ceil(executionsPage.totalRecords / executionsPage.pageSize), + currentPage: executionsPage.currentPage, + pageSize: executionsPage.pageSize, + }), }, SecurityHistoryExecution: { id: historyExecution => historyExecution.id, execution: historyExecution => historyExecution.execution, + securityBusiness: async (historyExecution, _, { injector }) => { + const securityBusiness = await injector + .get(SecurityBusinessesProvider) + .getSecurityBusinessByIdLoader.load(historyExecution.securityBusinessId); + if (!securityBusiness) { + throw new GraphQLError( + `Business ID="${historyExecution.securityBusinessId}" is not a security`, + ); + } + return securityBusiness; + }, // Transaction concrete types are mapped to their id (see codegen.ts mappers); Charge is // mapped to its row, so it has to be loaded. transaction: historyExecution => historyExecution.transaction?.id ?? null, @@ -118,6 +172,7 @@ export const securityBusinessesResolvers: ForeignSecuritiesModule.Resolvers = { }, SecurityBusiness: { id: securityBusiness => securityBusiness.id, + ownerId: securityBusiness => securityBusiness.owner_id, business: async (securityBusiness, _, { injector }) => { const business = await injector .get(BusinessesProvider) diff --git a/packages/server/src/modules/foreign-securities/typeDefs/foreign-securities.graphql.ts b/packages/server/src/modules/foreign-securities/typeDefs/foreign-securities.graphql.ts index ff8f9c769..814870a18 100644 --- a/packages/server/src/modules/foreign-securities/typeDefs/foreign-securities.graphql.ts +++ b/packages/server/src/modules/foreign-securities/typeDefs/foreign-securities.graphql.ts @@ -13,6 +13,8 @@ export default gql` securityKey: String! " Reference details; null when no matching security was ingested for this owner " details: Security + " The security's own business, reached through the key -> ISIN identifier bridge. Null when the key has no security business yet — the reference feed can be ingested before the executions that create one " + securityBusiness: SecurityBusiness " The charge's transactions whose description carries this key " transactions: [Transaction!]! " Ingested portfolio executions matched to those transactions by account, date and amount " diff --git a/packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts b/packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts index e618bcdcd..a11ca55aa 100644 --- a/packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts +++ b/packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts @@ -8,6 +8,33 @@ export default gql` securityBusinessHistory(businessId: UUID!): SecurityBusinessHistory! @requiresAuth " Every security the tenant holds, with the position its executions add up to. Closed positions — sold out, or never ingested — are left out unless asked for " securityHoldings(includeClosed: Boolean = false): [SecurityHolding!]! @requiresAuth + " Ingested executions across securities, newest first — deliberately the opposite of securityBusinessHistory, whose oldest-first order the position calculation depends on " + securityExecutions( + filters: SecurityExecutionsFilter + page: Int = 0 + limit: Int = 100 + " Resolve the cash movement, and so the charge, behind each execution. The pairing is greedy and one-to-one over a security's whole history, so it cannot be computed from a page — the same execution would report a different charge at a different page size. Asking for it therefore switches to an unpaginated match per security, and caps how many securities the filter may resolve to " + includeCharges: Boolean = false + ): PaginatedSecurityExecutions! @requiresAuth + } + + " Which securities, over what period, of what kind. The three identity filters (securityBusinessIds, isins, symbols) union with each other — they are three ways of naming the same axis — while dates and types narrow on top. Naming none of them means every security the tenant has " + input SecurityExecutionsFilter { + " Security business ids — the same ids securityHoldings returns " + securityBusinessIds: [UUID!] + isins: [String!] + " Matched case-insensitively against the security's cached symbol " + symbols: [String!] + fromTradeDate: TimelessDate + toTradeDate: TimelessDate + tradeTypes: [SecurityTradeType!] + transactionTypes: [SecurityTransactionType!] + } + + " A page of executions, each carrying the security it belongs to " + type PaginatedSecurityExecutions { + nodes: [SecurityHistoryExecution!]! + pageInfo: PageInfo! } " One security and what is held of it, without the execution history behind it " @@ -46,6 +73,8 @@ export default gql` type SecurityHistoryExecution { id: UUID! execution: SecurityExecution! + " The security this execution belongs to — what a cross-security list groups by " + securityBusiness: SecurityBusiness! " The charge the matched cash movement belongs to; null when no movement was matched " charge: Charge transaction: Transaction @@ -59,6 +88,8 @@ export default gql` " A business that stands for one traded security, identified by its ISIN " type SecurityBusiness { id: UUID! + " The tenant this security belongs to, so rows from several businesses stay distinguishable " + ownerId: UUID! " The business this security is represented by " business: LtdFinancialEntity! isin: String! diff --git a/packages/server/src/modules/foreign-securities/types.ts b/packages/server/src/modules/foreign-securities/types.ts index afd7d83bf..45649f1c3 100644 --- a/packages/server/src/modules/foreign-securities/types.ts +++ b/packages/server/src/modules/foreign-securities/types.ts @@ -66,10 +66,36 @@ export type SecurityPositionWithIdProto = SecurityPositionProto & { id: string } export type SecurityHistoryExecutionProto = { id: string; execution: SecurityExecutionRow; + /** + * The security business the execution belongs to. Carried rather than resolved from the row + * because the executions table is keyed by Poalim's security key, and several keys can + * collapse onto one ISIN — the caller already knows which business it asked for. + */ + securityBusinessId: string; /** The matched transaction, carrying the charge it belongs to. Null when nothing matched. */ transaction: { id: string; charge_id: string } | null; }; +/** A page of executions plus the total the filter matched, for `Query.securityExecutions`. */ +export type PaginatedSecurityExecutionsProto = { + nodes: SecurityHistoryExecutionProto[]; + totalRecords: number; + currentPage: number; + pageSize: number; +}; + +/** What `Query.securityExecutions` narrows on, normalized off the GraphQL input. */ +export type SecurityExecutionsFilterInput = { + securityBusinessIds?: readonly string[] | null; + isins?: readonly string[] | null; + symbols?: readonly string[] | null; + fromTradeDate?: string | null; + toTradeDate?: string | null; + /** The bank's own labels, already translated from the GraphQL enums by the resolver. */ + rawTradeTypes?: readonly string[] | null; + rawTransactionTypes?: readonly string[] | null; +}; + /** * One security plus the position its executions imply, for the tenant-wide holdings list. * The execution list itself is deliberately absent — see the `SecurityHolding` type. @@ -90,6 +116,11 @@ export type SecurityBusinessHistoryProto = { export type ChargeSecurityProto = { /** Scoped to the charge so the client cache keeps a key's entries distinct per charge. */ id: string; + /** + * The charge's owner. Carried because the Poalim key is only unique within one — resolving the + * key to a security without it can attach a charge to another business's security. + */ + ownerId: string; securityKey: string; details: SecurityRow | null; transactionIds: string[]; diff --git a/packages/server/src/modules/transactions/resolvers/transaction-suggestions.resolver.ts b/packages/server/src/modules/transactions/resolvers/transaction-suggestions.resolver.ts index 80b972ed1..d16dce61d 100644 --- a/packages/server/src/modules/transactions/resolvers/transaction-suggestions.resolver.ts +++ b/packages/server/src/modules/transactions/resolvers/transaction-suggestions.resolver.ts @@ -135,6 +135,10 @@ const missingInfoSuggestions = async ( const securityBusiness = await injector .get(SecurityBusinessesProvider) .getSecurityBusinessByIdentifierLoader.load({ + // The transaction's own owner, not the session's: a Poalim key is unique only within an + // owner, so the same security traded by two of a tenant's businesses would otherwise + // resolve to whichever row won the batch. + ownerId: transaction.owner_id, type: 'POALIM_SECURITY_KEY', value: securityKeys[0], });