Skip to content

Expose securities on the MCP connector - #4277

Open
gilgardosh wants to merge 8 commits into
mainfrom
feat/mcp-securities-tools
Open

Expose securities on the MCP connector#4277
gilgardosh wants to merge 8 commits into
mainfrom
feat/mcp-securities-tools

Conversation

@gilgardosh

@gilgardosh gilgardosh commented Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What & why

The securities domain (PRs #4194 → #4270) is reachable only through the web UI. Over MCP, an
assistant 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 sits behind a charge.

This adds that, in three pieces, plus the server work they need to be correct rather than
plausible.

Tools

accounter_list_security_holdings — 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.
includeClosed also returns securities traded but no longer held; search matches name (either
language), symbol, ISIN, exchange, currency and every source identifier.

Search, ordering and the row cap happen in the tool: upstream takes no search argument, a portfolio
is tens to low hundreds of rows, and mirroring the /securities screen's own match and sort rules is
what stops the two drifting. Ordering is by |quantity| descending — Math.abs on purpose, since a
negative quantity is a large position badly recorded, not a small one.

accounter_get_security_executions — the trade history. Buys, sales, dividends, interest,
redemptions, distributions and transfers, newest first, really paginated. Narrow by security, trade
date and kind. The three identity filters (securityBusinessIds, isins, symbols) union with
each other: they are three ways of naming one axis, so one ISIN plus one symbol means both
securities, not the empty overlap.

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, the trade lives in a separate ingested 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 — absent (not a securities charge, or not
asked for), empty (no key the feed knows), and referenceFound: false (a traded key whose reference
scrape is stale).

The numbers carry their own caveats

A position is arithmetic over a scraped trade history, and the arithmetic is easy to read as more
than it is. The bank reports no holding. There are no market prices anywhere in the system, so
current value and unrealized P&L are not computable. Pre-historyStartDate holdings and splits are
invisible. A negative quantity means a history that starts mid-life. A null amount means nothing was
ingested, not 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. Grouping is structural where a caveat is only advisory. Quantities
and average costs are never summed at all, even within one currency.

Two things found on the way

The securities tables were never in the multi-business RLS scope.
2026-05-25T10-00-00.rls-multi-business-scope switched 45 tables' read predicate to
owner_id = ANY(get_current_business_scope()). All four securities tables were created after it
(2026-08-11 / 08-13 / 08-20) and still read through the singular get_current_business_id(); no
later migration broadened them. Verified against a live database before fixing.

The failure was a silent narrowing, not a leak: the connector forwards its resolved scope as
x-business-scope and echoes that scope back, so a two-business caller would be told it had seen
both while the tables had served one. The web client's business switcher has the same bug today;
this fixes both. Predicates are byte-identical to the earlier migration's, so writes stay pinned to
the explicit target and the scraper ingestion path is unaffected.

Charge links and pagination do not compose. matchExecutionsToTransactions is greedy and
one-to-one over the sets it is handed, oldest-first. Hand it a page's slice and an execution on page 2
can claim the cash movement belonging to one on page 1 — the same execution reporting a different
charge at a different pageSize.

So Query.securityExecutions has two paths. Without includeCharges the whole filter pushes into
SQL and the page is a LIMIT/OFFSET slice. With it, each named security's complete history is
fetched and paired, then filtered and sliced in memory — capped at ten securities, since each costs a
full history plus a transactions query. Both paths order identically so they cannot disagree about
what page 1 is, and the tool refuses includeCharges unless securities are named (a
VALIDATION_ERROR that says what to add, rather than an UPSTREAM_ERROR that reads as a fault to
retry).

Commits
fix(migrations) scope the four securities tables to the request's business scope
feat(server) filtered, paginated securityExecutions; ownerId and the charge→security bridge
feat(mcp-server) the two securities tools
feat(mcp-server) includeSecurities on accounter_get_charges
docs(mcp-server) glossary entries for the securities vocabulary
test tool, query and RLS-scope coverage
docs(mcp-server) README, recorded findings, changeset

Verification

  • yarn test:integration against a local DB with the migration applied: 328 files, 4151 tests
    passing    , run four times. One run had a one-off suite-setup flake in
    rls-write-target.integration.test.ts (no assertion failed; it passes in isolation and in the
    other three runs).
  • yarn workspace @accounter/mcp-server test: 722 passing.
  • yarn lint: 0 errors. yarn prettier --check .: clean apart from three pre-existing
    untouched files.
  • Migration verified on a live database — pg_policies.qual now reads
    owner_id = ANY (accounter_schema.get_current_business_scope()) for all four tables, and the
    full-migration rls-all-tables suite passes with it applied.

Pre-existing failures, not from this PR

mcp-e2e.test.ts has two failing assertions (initializes and lists the curated tools, rejects a write tool as unknown while writes are disabled) — both expect the write tools to be hidden when
MCP_ENABLE_WRITE_TOOLS is unset. Confirmed identical on a clean stash of this branch's base
(same assertion, 14 tools instead of 16). Left alone as out of scope.

Not done

Left deliberately, and called out in the plan: the web client's /securities screen and Security tab
still use securityHoldings / securityBusinessHistory rather than migrating onto the new query; no
market-price or valuation work (no feed exists); the bank's per-execution nominalProfitLoss /
realProfitLoss are still not aggregated into a realized-gain report; no mutating securities tool;
and ledger generation for ForeignSecuritiesCharge remains unsupported.

🤖 Generated with Claude Code

gilgardosh and others added 5 commits August 23, 2026 17:03
@gilgardosh @claude
fix(migrations): scope the securities tables to the request's busines…
75c072e 
…s scope

`2026-05-25T10-00-00.rls-multi-business-scope` switched every `tenant_isolation`
read predicate to `owner_id = ANY(get_current_business_scope())`, leaving writes
pinned to `get_current_business_id()`. All four securities tables were created
after it (2026-08-11 / 08-13 / 08-20) and so were never in its list — they still
read through the singular helper, and no later migration broadens them.

The failure is a silent narrowing, not 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 it
would break the MCP connector harder — it forwards its resolved read scope as
`x-business-scope` and echoes that scope back to the caller, so the caller is
told it saw more than it did.

Predicates are byte-identical to the earlier migration's, so the scraper
ingestion path (a write, pinned to the explicit target) is unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh @claude
feat(server): filtered, paginated securities executions query
b10bcb6 
Everything read-side in the securities domain was addressed one security at a
time and unfiltered: `securityHoldings` returns the whole portfolio with no
filter, and `securityBusinessHistory` takes a single business id and always
returns its entire life. There was no way to ask "every sale of these two
securities last year".

Adds `Query.securityExecutions(filters, page, limit, includeCharges)` returning
`PaginatedSecurityExecutions`, reusing `SecurityHistoryExecution` and `PageInfo`
rather than inventing a second execution shape. Newest first — deliberately the
opposite of `securityBusinessHistory`, whose oldest-first order
`calculateSecurityPosition` depends on to pick a position's currency.

The three identity filters (`securityBusinessIds`, `isins`, `symbols`) union with
each other: they are three ways of naming the same axis, so asking for one ISIN
and one symbol means both securities, not the empty overlap. Dates and types
narrow on top. They resolve against the request-memoized
`getAllSecurityBusinesses()` instead of three more queries — which also means an
id that is not a security business of this tenant resolves to nothing rather than
reaching the executions feed.

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, and the same execution would report a
different charge at a different page size. So `includeCharges: false` pushes the
whole filter into SQL and slices with LIMIT/OFFSET, while `includeCharges: true`
reuses `getSecurityBusinessHistory` per security, unpaginated, and slices in
memory — capped at MAX_CHARGE_LINK_SECURITIES securities, since each one costs a
full history plus a transactions query. Both paths order identically so they
cannot disagree about what page 1 is.

`COUNT(*) OVER ()` rides on the returned rows, so 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.

Trade and transaction types filter on the bank's own Hebrew labels, translated
from the GraphQL enums by new inverses of the existing maps. The inverses are
built by inverting 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 — and
kept per-map, because the bank spells the same word two ways (`פדיון` as a trade
type, `פידיון` as a payment type) and one shared inverse would silently resolve
one to the other.

Also threads identity through so the new surface is usable across securities and
from a charge:

- `SecurityBusiness.ownerId`, so rows from several businesses stay
  distinguishable once a request's scope spans more than one.
- `SecurityHistoryExecution.securityBusiness`, so a flat cross-security list can
  be grouped. Carried on the proto rather than derived from the row: the
  executions table is keyed by Poalim's key, several of which can collapse onto
  one ISIN, and the caller already knows which business it asked for.
- `ChargeSecurity.securityBusiness`, closing the loop from a charge to the
  security's own identity through the key -> ISIN identifier bridge. Nullable: the
  reference feed can be ingested before the executions that create a security
  business.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh @claude
feat(mcp-server): securities holdings and executions tools
0e16a58 
The connector knew securities only as a charge *type* — a `FOREIGN_SECURITIES`
value in the charge filters and one glossary entry. There was no way to ask what
the tenant holds or what it traded.

Adds two read-only tools:

- `accounter_list_security_holdings` — the portfolio. One row per security with
  units held, weighted average cost, totals bought and sold, and the span of the
  ingested 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 web screen searches, so the
  two cannot drift. Upstream takes no search argument and a portfolio is tens to
  low hundreds of rows, so it filters here rather than growing a server filter
  nothing else needs. Rows sort by |quantity| descending, the screen's own
  default: biggest live position first, with `Math.abs` on purpose because a
  negative quantity is a large position badly recorded, not a small one.

- `accounter_get_security_executions` — the trade history, newest first, paged.
  Narrow by security (ids, ISINs or symbols, which union with each other), by
  trade date, and by kind. `includeCharges` also resolves the charge behind each
  trade, and is refused here unless securities are named: upstream caps it and
  would answer UPSTREAM_ERROR, which reads as a server fault to retry when in
  fact the call was malformed.

Both echo `scope.memberBusinessIds` and tag every row with `ownerId`, so a
multi-business answer cannot pass for a single-business one.

The holdings tool computes per-currency subtotals rather than leaving totalling
to the caller. Asked what a portfolio is worth, a model will add a shekel column
to a dollar one — these amounts are each security's own trade currency and are
never converted. Computing the sums that *are* valid, and only those, makes the
grouping structural where a caveat is only advisory; quantities and average costs
are deliberately absent, since units of different instruments and per-unit prices
do not add up even within one currency. Securities with nothing ingested have no
currency to report in and are counted separately rather than folded into an
arbitrary bucket.

Both responses also carry a `caveats` array on the wire: the position is derived
arithmetic over a scraped history, there is no bank balance and no market price,
pre-history holdings and splits are invisible, and a null amount means "nothing
ingested" rather than zero. A model that reads the rows and not the schema still
has to see that.

Registered between the charge/ledger drill-down and the reference-data lookups:
securities are their own drill-down, with the portfolio as the entry point and the
execution history as what it drills into.

Also extends the registry-wide contract suites, which are the real gate on a new
tool: upstream fixtures in scope-forwarding and usage-log, and both tools added to
scope-contract's hand-listed BUSINESS_SCOPED_TOOLS / MULTI_BUSINESS_TOOLS.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh @claude
feat(mcp-server): includeSecurities on accounter_get_charges
fcb60bc 
A foreign-securities charge is the one place where "what actually happened" is
not in the charge itself: the cash movement is a bank row, and the trade behind it
lives in the ingested portfolio feed. Asked about a securities charge, the model
could see the amount and the counterparty but not the security or the trade.

Adds `includeSecurities`, following `includeTransactions` / `includeDocuments`
exactly — one `@include(if:)` inline fragment on the `ForeignSecuritiesCharge`
member, threaded through both operations in the document.

Each security reports its `securityBusinessId`, which is what
`accounter_list_security_holdings` and `accounter_get_security_executions` are
addressed by, so a charge answer can be followed into the portfolio rather than
dead-ending. Descriptors come from the security business where both sources 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.

Three states are kept distinct rather than collapsed to an empty list, because
they mean different things:

- the field is absent for any charge that is not a securities one, and for a
  securities charge fetched without the flag — `@include(if:)` omits it entirely,
  so both read as "not asked for";
- an empty array means the transaction descriptions carried no key the ingested
  feed knows;
- `referenceFound: false` on a present security means the reference scrape is out
  of date for a key that *is* traded, which is worth seeing rather than dropping.

Unlike `transactions` and `additionalDocuments`, which the `Charge` interface
declares for every member, `securities` hangs off one union member — so it is
narrowed off the raw charge rather than read from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh @claude
docs(mcp-server): glossary entries for the securities vocabulary
b79c124 
The connector's glossary is what stops the model reading a tool result wrong
before it has seen the schema, and securities had exactly one entry in it — which
was also stale.

Corrects `foreign-securities-charge`: it said the charge is identified by "the
securities counterparty business configured for the owner", which stopped being
true when per-security businesses landed. The counterparty is now the traded
security itself; the general business is only the fallback. Also states that the
trade behind the cash leg has to be asked for, and that ledger generation for this
charge type is still unsupported.

Adds five entries, each written around the trap rather than the field name:

- `security-execution` — that it comes from the portfolio feed and not the bank
  statement, that the two have no link in the source so the pairing is derived and
  exact, and that dividends and interest are execution kinds here rather than a
  separate entity.
- `security-business` — that the ISIN is the identity, why that forces the
  identifier indirection, that two Poalim keys can collapse onto one security, and
  that it deliberately carries no suggestion phrases so a security can never win a
  description-based counterparty match.
- `derived-position` — the four ways these numbers mislead if quoted plainly:
  pre-history holdings uncounted, splits invisible, a negative quantity meaning a
  mid-life history rather than a short, and a null amount meaning nothing ingested
  rather than zero. Plus that there are no market prices at all, so current value
  and unrealized P&L are not computable.
- `poalim-security-key` — that it is parsed out of a description rather than
  reported, is not the ISIN, and is not comparable across brokers.
- `isin` — why identity and lookup are split across two tables, and that it is the
  stable way to name a security without a `securityBusinessId`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh gilgardosh self-assigned this Aug 23, 2026
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 14:39 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 14:39 — with GitHub Actions Active
@github-actions

github-actions Bot commented Aug 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@accounter/client 0.1.0-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/green-invoice-graphql 0.8.7-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/hashavshevet-mesh 0.2.13-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/israeli-vat-scraper 0.1.13-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/modern-poalim-scraper 0.11.0-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/payper-mesh 0.2.13-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/scraper-app 0.0.3-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/server 0.2.0-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/shaam-uniform-format-generator 0.2.7-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎
@accounter/shaam6111-generator 0.1.9-alpha-20260823155124-6a7748c3640ef5374111955447d72a188a0838f8 npm ↗︎ unpkg ↗︎

gilgardosh and others added 2 commits August 23, 2026 18:40
@gilgardosh @claude
test: cover the securities tools, query and RLS scope
88fb14d 
Server:

- The enum inverses round-trip every member of all three vocabularies. A forward
  map that grows a value without its label, or an inverse resolving into the wrong
  map, fails here rather than at query time as an empty result nobody can explain.
  Two cases pin the reason the inverses are per-map: the bank spells redemption
  `פדיון` as a trade type and `פידיון` as a payment type, while buy and sell are
  spelled identically across the two vocabularies.
- Integration coverage for `getSecurityExecutionsPage`: newest-first ordering,
  non-overlapping pages against a stable total, the identity filters unioning
  rather than intersecting, case-insensitive symbol matching, an id outside the
  tenant resolving to nothing, date and type pushdown, the empty-list-means-no-
  restriction guard, the `MAX_CHARGE_LINK_SECURITIES` refusal, and — the
  regression this design exists to prevent — the match path ordering identically
  to the SQL path.
- Integration coverage for the migration, run under the non-superuser role rather
  than at provider level: all four tables return every business in a multi-business
  scope, narrow to a single-business scope, and fall back to the single business
  when the GUC is unset.

MCP: a new suite for both tools — absolute-quantity ordering with a negative
position present, search across every field the web screen searches, per-currency
subtotals and the no-currency bucket, owner tagging and scope forwarding under two
memberships, 1-based-to-0-based page translation, the `includeCharges` validation
refusal, and a 400-security portfolio degrading to valid JSON inside the byte
budget. Plus `includeSecurities` cases on the charges detail tool covering the
three states it keeps distinct, and two end-to-end tool calls over real HTTP.

Two fixture-isolation fixes, both latent and exposed rather than introduced by the
above. The securities lookups carry no `owner_id` predicate — RLS scopes them in
production, and these suites connect as a superuser which bypasses it — so:

- ISINs in this suite are now synthetic (`ZZ…`). Sharing a real one with
  `security-businesses.integration.test.ts`, which runs concurrently and asserts on
  the row it gets back for a given ISIN, is a genuine cross-suite collision.
- Two `getExecutionsBySecurityBusiness` cases asserted on the whole map's size,
  which assumes exclusive access to the database. They now assert on this suite's
  own buckets. The second is also re-pointed at what it was really about: a
  security business with no key identifier still gets an empty bucket, which is
  what keeps "nothing ingested" distinguishable from "not a security".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh @claude
docs(mcp-server): document the securities tools and what was learned
e7045af 
README: the two new tools and the `includeSecurities` flag, each written around
what the numbers do not mean as much as what they do — derived positions, no
market prices, per-security currencies that are never converted, and why
`includeCharges` needs the securities named. Two smoke-test steps, including the
same-chargeId-at-any-pageSize check that is the whole reason that path exists.

Also corrects bounds this file had drifted on: the tool count (twelve, listing
eleven, now fourteen), the balance report's date range and row cap (366/500 →
1096/1000), the tags cap (500 → 1000), and the blanket "date ranges ≤ 366 days,
page size ≤ 50, list caps of 500" line. Every cap is an exported `MAX_*` constant
the suite asserts, so the file now says that rather than restating numbers it
cannot keep in step.

connector-gaps-and-decisions: a new "recorded findings" section for things worth
not re-deriving — the securities tables never having been in the multi-business
RLS scope and the general rule that a table created after that migration inherits
nothing from it; why a greedy one-to-one pairing cannot be computed from a page;
and why these integration suites, which share a database and connect as a
superuser that bypasses RLS, cannot assert on whole result sets.

Plus the changeset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 15:44 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 15:44 — with GitHub Actions Active
@gilgardosh gilgardosh changed the title mcp securities tools Expose securities on the MCP connector Aug 23, 2026
@gilgardosh
gilgardosh requested a lite review from Copilot August 23, 2026 15:46
@gilgardosh
gilgardosh marked this pull request as ready for review August 23, 2026 15:47
@gilgardosh @claude
fix(changeset): drop the ignored migrations package from the changeset
1a040f0 
`.changeset/config.json` ignores `@accounter-helper/*`, and changesets refuses a
changeset that names both ignored and non-ignored packages — so the snapshot
release failed with "Found mixed changeset".

The migrations package is not published and is not versioned this way; the
repo's own convention already reflects that, since `security-businesses-schema`
added a migration and declared only `@accounter/server`. The migration is still
described in the changeset body, which is where a reader looks for it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 15:49 — with GitHub Actions Active
@gilgardosh
gilgardosh deployed to accounter-fullstack August 23, 2026 15:49 — with GitHub Actions Active
Copilot AI reviewed Aug 23, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical multi-business ownership and DELETE-isolation findings, plus moderate tool correctness findings, remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR exposes securities holdings, executions, and charge-linked securities through MCP, with GraphQL, RLS, tests, and documentation updates.

Changes:

  • Adds holdings and paginated execution MCP tools.
  • Adds includeSecurities to charge details.
  • Adds multi-business RLS support and related coverage.
File summaries
File Reviewed change
packages/server/src/modules/foreign-securities/types.ts Execution filtering, pagination, and ownership types.
packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts Execution query and ownership schema.
packages/server/src/modules/foreign-securities/typeDefs/foreign-securities.graphql.ts Charge-to-security relationship schema.
packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts Execution page and owner resolvers.
packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts Charge security resolvers.
packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts Filtering, pagination, holdings, and matching logic.
packages/server/src/modules/foreign-securities/providers/__tests__/foreign-securities.integration.test.ts Integration coverage for executions and RLS.
packages/server/src/modules/foreign-securities/helpers/security-execution-enums.helper.ts Execution filter enum translation.
packages/server/src/modules/foreign-securities/helpers/__tests__/security-execution-enums.helper.test.ts Enum translation tests.
packages/migrations/src/run-pg-migrations.ts Registers the migration.
packages/migrations/src/actions/2026-08-23T10-00-00.rls-scope-securities-tables.ts Applies multi-business securities RLS scope.
packages/mcp-server/src/tools/terminology-data.ts Securities glossary entries.
packages/mcp-server/src/tools/securities.ts Holdings and execution MCP tools.
packages/mcp-server/src/tools/registry-instance.ts Registers the new tools.
packages/mcp-server/src/tools/charge-details.ts Adds securities to charge details.
packages/mcp-server/src/tools/__tests__/usage-log.test.ts Usage logging coverage.
packages/mcp-server/src/tools/__tests__/securities.test.ts Securities tool tests.
packages/mcp-server/src/tools/__tests__/scope-forwarding.test.ts Scope forwarding tests.
packages/mcp-server/src/tools/__tests__/scope-contract.test.ts Scope contract tests.
packages/mcp-server/src/tools/__tests__/detail-tools.test.ts Charge security detail tests.
packages/mcp-server/src/__tests__/mcp-e2e.test.ts MCP end-to-end coverage.
packages/mcp-server/README.md Documents the securities tools.
packages/mcp-server/docs/connector-gaps-and-decisions.md Records design decisions and caveats.
eslint.config.mjs Lint configuration updates.
codegen.ts GraphQL security type mappings.
.changeset/mcp-securities-tools.md Release notes.
Review details

Suppressed comments (7)

packages/mcp-server/src/tools/securities.ts:188

  • The /securities screen's default is numeric quantity descending (packages/client/src/components/screens/securities/index.tsx:88-90), whereas Math.abs makes a -80 position sort ahead of a +12 position. The PR and this function document that the tool mirrors the screen, so negative positions currently produce different ordering between the two views. Update the screen to use magnitude as well, or change this comparator and the parity claim.
    Math.abs(b.quantity) - Math.abs(a.quantity) ||

packages/mcp-server/src/tools/securities.ts:362

  • The input allows up to 50 values in each identity list, but includeCharges only supports 10 resolved securities. A valid call naming 11 matching securities therefore reaches upstream and becomes an UPSTREAM_ERROR, even though the handler's stated purpose is to classify malformed charge-link requests as validation errors. Enforce or otherwise surface the resolved-security cap before the upstream call instead of allowing this known input error through the upstream-error path.
  includeCharges: z
    .boolean()
    .optional()
    .default(false)
    .describe(

packages/mcp-server/src/tools/securities.ts:566

  • The summary string is missing the possessive in each security own trade currency, so the user-facing result is grammatically incorrect and inconsistent with the tool description. Use each security's own trade currency.
        : `${total} execution(s); showing ${shown} on page ${pagination.page} of ${pagination.totalPages}, newest first. Amounts are in each security own trade currency.`,

packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts:530

  • COUNT(*) OVER() is only available when the page contains a row. A request for a page after the last one therefore reports totalRecords: 0/totalPages: 0, and the MCP tool says “No executions matched” even when earlier pages contain matches. Preserve the count with an independent count query (or equivalent) when the slice is empty so pageInfo remains truthful.
      // `COUNT(*) OVER ()` is identical on every row of the page.
      totalRecords: rows.length ? Number(rows[0].total_count) : 0,
      currentPage: page,

packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts:557

  • ownerId is one request-level write target, but businessIds can contain securities from multiple owners in the read scope. If a caller asks for a B-owned security while the current target is A, this invokes getSecurityBusinessHistory(B, A) and filters candidate transactions to A, so every B execution loses its charge link; the history's key-only execution lookup can also mix same-key rows from other in-scope owners. Resolve the owner for each security and keep both execution and transaction matching owner-scoped.
    const histories = await Promise.all(
      businessIds.map(businessId => this.getSecurityBusinessHistory(businessId, ownerId)),

packages/server/src/modules/foreign-securities/resolvers/security-businesses.resolver.ts:103

  • These arguments are exposed to direct GraphQL callers without a positive/non-negative check, and the resolver passes them directly to LIMIT/OFFSET. A negative page reaches a negative offset, while limit: 0 yields no rows and makes Math.ceil(totalRecords / pageSize) NaN/Infinity; MCP's Zod validation does not protect other clients. Validate page >= 0 and limit > 0 in the GraphQL path (or enforce an equivalent schema contract).
        page: page ?? 0,
        limit: limit ?? DEFAULT_SECURITY_EXECUTIONS_LIMIT,

packages/server/src/modules/foreign-securities/typeDefs/security-businesses.graphql.ts:18

  • These new GraphQL arguments are unconstrained beyond Int. A direct GraphQL caller can pass limit: 0 (which makes totalPages = Math.ceil(totalRecords / pageSize) become NaN and fail Int serialization) or page: -1 (which reaches a negative SQL OFFSET); the MCP Zod schema does not protect other GraphQL clients. Validate page >= 0 and limit > 0 (and apply the intended maximum) in the resolver or schema before invoking the provider.
    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
  • Files reviewed: 26/26 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/mcp-server/src/tools/securities.ts
Comment on lines +220 to +224
const currency =
holding.totalBought?.currency ??
holding.totalSold?.currency ??
holding.averageCost?.currency ??
holding.currency;
Comment thread packages/mcp-server/src/tools/securities.ts
// 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.
const needle = input.search?.toLowerCase();
Comment thread packages/migrations/src/actions/2026-08-23T10-00-00.rls-scope-securities-tables.ts
Comment on lines +41 to +48
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())
`,
);
Comment thread packages/server/src/modules/foreign-securities/providers/foreign-securities.provider.ts
Comment on lines +510 to +513
const rows = await getFilteredSecurityExecutions.run(
{
securities: [...businessIdByKey.keys()],
isTradeTypes: rawTradeTypes.length ? 1 : 0,
Comment thread packages/server/src/modules/foreign-securities/resolvers/foreign-securities.resolver.ts
Comment on lines +55 to +58
(await injector.get(SecurityBusinessesProvider).getSecurityBusinessByIdentifierLoader.load({
type: 'POALIM_SECURITY_KEY',
value: chargeSecurity.securityKey,
})) ?? null,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

@gilgardosh gilgardosh

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gilgardosh