From 7473272bc8b1f51964c0e2115fe8384fb8fee1ba Mon Sep 17 00:00:00 2001 From: Mike Date: Fri, 7 Aug 2026 20:43:55 +0300 Subject: [PATCH 1/5] fix(SHARK-3629): slot 0 is the account's Default project key, not the MFA-gated synthetic one findKeyBySlot read index 0 as the account's own synthetic key whenever no team account was selected, so every key-addressed tool refused the slot a user can see in their own listing, explaining itself with a second factor that has nothing to do with it. Measured on prod: mgmt_list_api_keys shows `index 0: Default`, and mgmt_get_api_key_status(index 0) answers "Slot 0 is not a project key". Two different routes were being treated as one. GET /auth/jwt/all is the project listing and carries slot 0; GET /auth/jwt/getMySyntheticJwt is the account's own key, MFA-gated and deliberately never wrapped here. A personal account's slot 0 now resolves through the listing like slots 1 and up, and the synthetic route is still not reached on any path -- asserted, not assumed. mgmt_reveal_api_key keeps its refusal for this slot. It shares the resolver, so without an explicit guard this change would have silently reopened what SHARK-3567 closed on purpose: a tool whose job is handing over a credential makes a different trade from one that operates on a key without disclosing it. One existing pin flipped rather than being deleted: the fixture it used has no slot 0, so it now pins the refusal that stays true for it, an empty slot naming the slots that exist, with no claim about second factors. Gates: typecheck, lint, format, 1644 tests. Co-Authored-By: Claude Opus 5 (1M context) --- src/mgmt/tools/keyAddressing.ts | 15 +- src/mgmt/tools/revealApiKey.ts | 13 ++ test/mgmt-key-addressing.test.ts | 18 +- test/mgmt-slot-zero-default-key.test.ts | 246 ++++++++++++++++++++++++ 4 files changed, 287 insertions(+), 5 deletions(-) create mode 100644 test/mgmt-slot-zero-default-key.test.ts diff --git a/src/mgmt/tools/keyAddressing.ts b/src/mgmt/tools/keyAddressing.ts index 4756187..bdece95 100644 --- a/src/mgmt/tools/keyAddressing.ts +++ b/src/mgmt/tools/keyAddressing.ts @@ -131,8 +131,19 @@ export async function findKeyBySlot( ): Promise { if (index === 0) { const selected = scopeOf(gateway)?.selected(); - if (!selected) return { ok: false, reason: "account-level" }; - return findTeamAccountKey(gateway, selected.address); + if (selected) return findTeamAccountKey(gateway, selected.address); + // SHARK-3629 — a PERSONAL account's slot 0 is the "Default" PROJECT key. It + // is in `GET /auth/jwt/all` alongside every other slot, which is why + // mgmt_list_api_keys shows it, so it resolves the same way they do and the + // MFA-gated `GET /auth/jwt/getMySyntheticJwt` is not reached for it. + // + // This is NOT the account's synthetic key. Treating the two as one made + // every key tool refuse the slot a user can see in their own listing, with + // a message about a second factor that had nothing to do with it. + // + // mgmt_reveal_api_key keeps its own refusal for this slot (SHARK-3567): a + // tool whose job is handing over a credential makes a different trade from + // one that operates on a key without disclosing it. } const keys = await gateway.listJwtTokens(); const listed = keys ?? []; diff --git a/src/mgmt/tools/revealApiKey.ts b/src/mgmt/tools/revealApiKey.ts index e8c9832..87ae644 100644 --- a/src/mgmt/tools/revealApiKey.ts +++ b/src/mgmt/tools/revealApiKey.ts @@ -34,6 +34,7 @@ import { } from "./confirmation.js"; import { labelKeySlot } from "./listApiKeys.js"; import { type KeyLookup, findKeyBySlot } from "./keyAddressing.js"; +import { scopeOf } from "../gateway/groupScope.js"; import { accountAddressForDisplay } from "./whoami.js"; import { MGMT_ADDITIVE_NON_IDEMPOTENT } from "./annotations.js"; import { describeEndpointToken } from "./endpointToken.js"; @@ -185,6 +186,18 @@ export function registerRevealApiKey({ .strict(), }, async ({ index, confirmToken }) => { + // SHARK-3567 kept HERE on purpose, now that findKeyBySlot resolves a + // personal account's slot 0 like any other project key (SHARK-3629). + // + // The two tools make different trades and must not share this answer. An + // operating tool acts on a key without disclosing it; this one hands the + // credential to the caller. Routing that around the gateway's second + // factor is the wrong trade, so the refusal stays on the disclosing tool + // even though the slot is now addressable everywhere else. + if (index === 0 && !scopeOf(gateway)?.selected()) { + return errorResult(ACCOUNT_LEVEL_REFUSAL); + } + // One list read per invocation, shared by the pre-flight check, the // approval page and the exchange. Memoised rather than re-fetched so the // page and the reveal cannot disagree about which key this is. diff --git a/test/mgmt-key-addressing.test.ts b/test/mgmt-key-addressing.test.ts index 65c08b0..84fa7ca 100644 --- a/test/mgmt-key-addressing.test.ts +++ b/test/mgmt-key-addressing.test.ts @@ -540,7 +540,18 @@ test("SHARK-3612: an ENCRYPTED key is refused with the reason and the way round } }); -test("SHARK-3612: slot 0 on a personal account is refused with the reason, not as a range error", async () => { +test("SHARK-3629: slot 0 on a personal account that has none is refused as an EMPTY slot, not as an account-level one", async () => { + // FLIPPED from SHARK-3612, deliberately. This used to assert that slot 0 on a + // personal account is always the account's own MFA-gated key and therefore + // always refused. That conflated two routes: the project listing + // (`GET /auth/jwt/all`, which carries slot 0 and is what the console shows) + // and `GET /auth/jwt/getMySyntheticJwt` (MFA-gated, never wrapped here). + // Slot 0 is now resolved through the listing like any other slot — see + // test/mgmt-slot-zero-default-key.test.ts. + // + // This fixture's listing has slots 4 and 7 and no slot 0, so what is pinned + // here is the refusal that remains TRUE for it: an empty slot, naming the + // slots that do exist, with no claim about second factors. const { gateway, calls } = makeStubGateway(); const { deps } = depsWithStore(); const client = await connect(gateway, deps); @@ -550,8 +561,9 @@ test("SHARK-3612: slot 0 on a personal account is refused with the reason, not a arguments: { index: 0 }, }); assert.equal((r as { isError?: boolean }).isError, true); - assert.match(textOf(r), /account-level key/); - assert.match(textOf(r), /mgmt_select_account/); + assert.match(textOf(r), /4/); + assert.match(textOf(r), /7/); + assert.doesNotMatch(textOf(r), /second factor/i); assert.equal(calls.filter((c) => c.method === "getJwtStatus").length, 0); } finally { await client.close(); diff --git a/test/mgmt-slot-zero-default-key.test.ts b/test/mgmt-slot-zero-default-key.test.ts new file mode 100644 index 0000000..c79795c --- /dev/null +++ b/test/mgmt-slot-zero-default-key.test.ts @@ -0,0 +1,246 @@ +// SHARK-3629 — slot 0 is the account's DEFAULT project key, and it is addressable. +// +// THE CONFUSION THIS FILE ENDS. Two different things were treated as one: +// +// GET /auth/jwt/all the project-key listing. On a personal account it +// carries slot 0, named "Default" in the console, and +// it is what mgmt_list_api_keys shows. +// GET /auth/jwt/getMySyntheticJwt the account's own synthetic key. MFA-gated, +// deliberately NOT wrapped by this server (SHARK-3557). +// +// findKeyBySlot read index 0 as the SECOND one whenever no team account was +// selected, so every key-addressed tool refused the slot the user can see in the +// listing, with a message about a second factor that has nothing to do with it. +// Measured on prod 2026-08-07: mgmt_list_api_keys shows `index 0: Default`, and +// mgmt_get_api_key_status(index 0) answers "Slot 0 is not a project key". +// +// WHAT IS PINNED HERE: +// 1. On a personal account, slot 0 resolves through the LISTING, exactly like +// slots 1 and up, and the tools operate on it. +// 2. The MFA-gated synthetic route is never called. Not before the change and +// not after: this ticket does not reach for that key at all. +// 3. On a SELECTED TEAM account, slot 0 still resolves through the team route. +// That path was correct and stays untouched. +// 4. When the listing genuinely has no slot 0, the refusal is the empty-slot +// one, naming the slots that exist, NOT a claim about second factors. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { createMgmtServer } from "../src/mgmt/server.js"; +import type { GatewayClient } from "../src/mgmt/gateway/client.js"; +import { + type MgmtDeps, + createConfirmationStore, +} from "../src/mgmt/tools/confirmation.js"; +import { createAccountScope } from "../src/mgmt/gateway/groupScope.js"; + +type Call = { method: string; args: unknown }; + +/** The slot every account has, and the one the console calls "Default". */ +const KEY_0 = { + index: 0, + jwt_data: "DEFAULT.JWT.VALUE", + is_encrypted: false, + name: "Default", + description: "", + config: "", +}; + +const KEY_4 = { + index: 4, + jwt_data: "SECRET.JWT.VALUE", + is_encrypted: false, + name: "agent-key", + description: "", + config: '{"blockchains":["eth"]}', +}; + +const TOKEN_FOR_SLOT_0 = "defaulttokenforslot0"; +const TOKEN_FOR_SLOT_4 = "premiumtokenforslot4"; + +const ISSUER = "http://localhost:3100"; + +function deps(): MgmtDeps { + return { + confirmations: createConfirmationStore(ISSUER), + sub: "test-subject", + issuerUrl: ISSUER, + mfaEnforced: true, + worker: { + importJwtToken: (jwtData: string) => + Promise.resolve({ + token: + jwtData === KEY_0.jwt_data ? TOKEN_FOR_SLOT_0 : TOKEN_FOR_SLOT_4, + }), + }, + } as unknown as MgmtDeps; +} + +/** A personal-account gateway: no team selected, and the listing carries slot 0. */ +function personalGateway(listing: (typeof KEY_0)[]): { + gateway: GatewayClient; + calls: Call[]; +} { + const calls: Call[] = []; + const rec = + (method: string, ret: unknown) => + (args?: unknown): Promise => { + calls.push({ method, args }); + return Promise.resolve(ret); + }; + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: rec("listJwtTokens", listing), + getJwtStatus: rec("getJwtStatus", { + frozen: false, + suspended: false, + freemium: false, + }), + getUserProfile: rec("getUserProfile", { + address: "0xabc0000000000000000000000000000000000001", + }), + } as unknown as GatewayClient; + return { gateway, calls }; +} + +async function connect( + gateway: GatewayClient, + d: MgmtDeps = deps() +): Promise { + const server = createMgmtServer(gateway, d); + const [clientT, serverT] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "0" }); + await server.connect(serverT); + await client.connect(clientT); + return client; +} + +function textOf(r: unknown): string { + return ((r as { content?: { text?: string }[] }).content ?? []) + .map((c) => c.text ?? "") + .join("\n"); +} + +// --------------------------------------------------------------------------- +// 1. Given a personal account whose listing carries slot 0, +// when a key-addressed tool names index 0, +// then it operates on that key. +// --------------------------------------------------------------------------- + +test("SHARK-3629: slot 0 on a personal account is a project key and reads like any other slot", async () => { + const { gateway, calls } = personalGateway([KEY_0, KEY_4]); + const client = await connect(gateway); + try { + const r = await client.callTool({ + name: "mgmt_get_api_key_status", + arguments: { index: 0 }, + }); + + assert.notEqual( + (r as { isError?: boolean }).isError, + true, + `slot 0 was refused: ${textOf(r)}` + ); + assert.match(textOf(r), /frozen: false/); + + // It resolved through the listing, and the status was read for the token + // that slot 0's material exchanges into. + assert.equal(calls.filter((c) => c.method === "listJwtTokens").length, 1); + // getJwtStatus takes the endpoint token positionally, so the recorded arg + // IS the token slot 0's material exchanged into. + const status = calls.find((c) => c.method === "getJwtStatus"); + assert.ok(status, "the status route was never reached"); + assert.equal(status.args, TOKEN_FOR_SLOT_0); + } finally { + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 2. The MFA-gated synthetic route is never called, on any path. +// --------------------------------------------------------------------------- + +test("SHARK-3629: resolving slot 0 never reaches for the MFA-gated synthetic key", async () => { + const { gateway, calls } = personalGateway([KEY_0, KEY_4]); + const client = await connect(gateway); + try { + await client.callTool({ + name: "mgmt_get_api_key_status", + arguments: { index: 0 }, + }); + const reachedForSynthetic = calls.filter((c) => + /synthetic/i.test(c.method) + ); + assert.deepEqual( + reachedForSynthetic, + [], + "the synthetic-JWT route is MFA-gated and out of scope for this ticket" + ); + // Nor did it try the team route on an account with no team selected. + assert.equal(calls.filter((c) => c.method === "getGroupJwt").length, 0); + } finally { + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 3. Given a SELECTED TEAM account, slot 0 still resolves through the team route. +// --------------------------------------------------------------------------- + +test("SHARK-3629: slot 0 on a selected TEAM account still resolves through the team route", async () => { + const calls: Call[] = []; + const scope = createAccountScope(); + scope.select({ address: "0xteam0000000000000000000000000000000000001" }); + const gateway = { + accountScope: scope, + getGroupJwt: (args?: unknown) => { + calls.push({ method: "getGroupJwt", args }); + return Promise.resolve({ jwt_data: KEY_0.jwt_data }); + }, + getJwtStatus: (args?: unknown) => { + calls.push({ method: "getJwtStatus", args }); + return Promise.resolve({ frozen: false, suspended: false }); + }, + getUserProfile: () => Promise.resolve({ address: "0xteam" }), + listJwtTokens: (args?: unknown) => { + calls.push({ method: "listJwtTokens", args }); + return Promise.resolve([KEY_4]); + }, + } as unknown as GatewayClient; + const client = await connect(gateway); + try { + const r = await client.callTool({ + name: "mgmt_get_api_key_status", + arguments: { index: 0 }, + }); + assert.notEqual((r as { isError?: boolean }).isError, true, textOf(r)); + assert.equal(calls.filter((c) => c.method === "getGroupJwt").length, 1); + // The team route answers for slot 0; the project listing is not consulted. + assert.equal(calls.filter((c) => c.method === "listJwtTokens").length, 0); + } finally { + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 4. Given a personal listing with no slot 0, the refusal is the empty-slot one. +// --------------------------------------------------------------------------- + +test("SHARK-3629: a personal account with no slot 0 is refused as an empty slot, not as a second-factor problem", async () => { + const { gateway } = personalGateway([KEY_4]); + const client = await connect(gateway); + try { + const r = await client.callTool({ + name: "mgmt_get_api_key_status", + arguments: { index: 0 }, + }); + assert.equal((r as { isError?: boolean }).isError, true); + const said = textOf(r); + assert.match(said, /4/, "the refusal must name the slots that do exist"); + assert.doesNotMatch(said, /second factor/i); + assert.doesNotMatch(said, /not a project key/i); + } finally { + await client.close(); + } +}); From d860d75cf6c46cc5ef14e409399c1e1ba9beba51 Mon Sep 17 00:00:00 2001 From: Mike Date: Fri, 7 Aug 2026 23:59:35 +0300 Subject: [PATCH 2/5] feat(SHARK-3629): serve the chain reads from the management endpoint, by default The advertised OAuth endpoint served 75 account-administration tools and not one chain read. Asked for an address balance, an agent connected to /mcp had no tool for it: the data tools lived on a second server behind a raw key in a header, and reaching them cost a second MCP entry, a hand-pasted credential and a client restart. The restart is what actually stopped people. /mcp now registers the same sixteen data tools /rpc does -- registerDataTools is imported rather than copied, so the two endpoints cannot drift into different surfaces -- and the key they spend is resolved server-side from the account the session is already signed in as: slot 0, the Default project key, or whichever slot mgmt_select_key names. Nothing is pasted and no credential enters the conversation. The default selection moves from `core` to `core` plus `data`, which only ever adds tools; the entry cost goes from ~2.4k to 8,823 o200k tokens against an 8,900 ceiling the test asserts. TWO CLASSIFICATION SUITES HAD TO NARROW THEIR SCOPE RATHER THAN GROW THEIR LISTS. mgmt-annotations (SHARK-3540) and mgmt-role-capabilities (SHARK-3553) enumerate everything registered on the management server and partition it, and both rules are about acting on an ACCOUNT. A chain read is read-only and answers to a different contract, held in test/annotations.test.ts -- so getAccountBalance was being told its readOnlyHint should be false, which is the wrong complaint about the right code. The boundary is now DATA_TOOL_NAMES in src/server.ts, exported beside the registrar and held equal to the live /rpc surface by test/data-tool-surface.test.ts. Verified rather than asserted: dropping one name from that list fires all four gates at once, so a data tool cannot slip past either classification by claiming to belong to the other. mgmt_select_key is a management tool that rides with the group, so it stays in both partitions. It registers through the account-scope wrapper -- naming slot 4 while the session is aimed somewhere you did not expect is exactly the mistake worth refusing -- and takes JwtManagerRead, the capability the key LISTING takes: a seat that may not see which projects exist has no business naming one by index. FOUR DEFECTS FOUND IN REVIEW BEFORE THIS SHIPPED, all pinned by a test: - The resolved token was cached by SLOT ALONE. mgmt_select_account moves a session between the accounts a login holds a seat on, and slot 4 of one team is a different key from slot 4 of another. The first resolution won, so every later chain read went out on the previous account's key -- including the slot-0 default nobody selects. The session would report one account through mgmt_whoami and the account line every wrapped tool prints, while the reads were billed to another. The cache key is now the account plus the slot. - The chain tools' own contracts were not delivered on this endpoint. SHARK-3599 lifted that prose OUT of the 16 tool descriptions because instructions carry it, which is only true where the instructions do; here they did not, and the RAW BASE UNITS rule lives nowhere else, so an agent decoding a transfer would have reported an amount wrong by 10^decimals with nothing contradicting it. Contracts 2 to 5 are now a shared DATA_TOOL_CONTRACTS both planes compose from; contract 1 stays per-endpoint, because the bound key is the one thing the two genuinely disagree about. - mgmt_select_key could name a key the account no longer has in that slot: a session can delete and recreate a slot without leaving, and the cache sees neither write. It now always re-reads, which is one gateway read and one worker exchange, the same as the equivalent key tool pays. - The deferred client Proxy answered every property with a function, which made it THENABLE. One `await` or `Promise.resolve` near it would call `then(resolve, reject)`, and the handler would resolve the real client, find no `then`, return undefined and never call either callback -- a permanent hang on a request path with nothing in a log. `then` is now absent. MUTATION TESTING FOUND WHAT COVERAGE COULD NOT, on both files it ran over. The new key session scored 68% on its first pass with fourteen survivors, and they were not noise: nothing proved the cache was a cache, nothing proved a failed resolution was retried rather than remembered as broken, nothing drove the refusal path of a switch at all, and the account-switch test above turned out to be passing through `select`, which always re-reads -- so it would have passed against a cache keyed on the wrong thing. It also found that the `provider` accessor, which hands the AAPI client to eight registered tools, was reachable by no test: replacing it with one that yields undefined survived the whole suite. Six tests later the file is at 95.45%, with two survivors that are genuinely equivalent. Over toolsets.ts (97.17%) two more real gaps: listPhrase was untested at three names, where slice(0, -1) and slice(0, 1) stop agreeing, and the guarantee that a session carries `core` however it was built was never exercised on a core-less input. KNOWN, NOT FIXED, AND NOW STATED WHERE IT WAS PREVIOUSLY DENIED. Importing the data plane here pulls gpt-tokenizer into the management binary at boot: measured RSS 79 -> 189 MB and 1.04 s for src/mgmt/server.ts, the tokenizer being 65 MB and 386 ms of it, against a 512Mi pod. Two comments asserted the opposite ("the management binary carries no tokenizer on purpose") and are corrected rather than left to be believed. Deferring the import is not cheap -- `data` is in the default and the group thunks run inside registerAsOneChange's synchronous window -- so the two real options, a real token count in mgmt_list_toolsets and a lazy tokenizer in torpc/tokens.ts, are recorded as open decisions. Gates: typecheck, lint, format, 1662 tests, coverage (global 90/80/85 and mgmt-scoped 80/75/80), build, mutation (toolsets.ts 97.17%, keySession.ts 95.45%). Co-Authored-By: Claude Opus 5 (1M context) --- DEPLOY-MGMT.md | 63 ++- USER-STORIES.md | 16 +- src/mgmt/data/keySession.ts | 174 +++++++ src/mgmt/server.ts | 36 +- src/mgmt/tools/index.ts | 59 ++- src/mgmt/tools/listToolsets.ts | 52 +- src/mgmt/tools/rolePermissions.ts | 9 + src/mgmt/tools/selectKey.ts | 75 +++ src/mgmt/toolsets.ts | 76 ++- src/server.ts | 128 ++++- test/data-tool-surface.test.ts | 44 +- test/helpers/mgmtToolSurface.ts | 38 +- test/mgmt-annotations.test.ts | 99 +++- test/mgmt-data-plane-in-session.test.ts | 616 ++++++++++++++++++++++++ test/mgmt-load-toolset.test.ts | 24 +- test/mgmt-role-capabilities.test.ts | 18 +- test/mgmt-toolsets.test.ts | 128 +++-- 17 files changed, 1527 insertions(+), 128 deletions(-) create mode 100644 src/mgmt/data/keySession.ts create mode 100644 src/mgmt/tools/selectKey.ts create mode 100644 test/mgmt-data-plane-in-session.test.ts diff --git a/DEPLOY-MGMT.md b/DEPLOY-MGMT.md index 9d394cc..54b17fd 100644 --- a/DEPLOY-MGMT.md +++ b/DEPLOY-MGMT.md @@ -78,21 +78,29 @@ client shim (mgmt-mcp) UAuth / gateway - `DELETE /mcp` — session teardown. **`?toolsets=` on the connection URL (SHARK-3600).** Which groups of tools the -session registers: `core`, `keys`, `usage`, `billing`, `notifications`, `team`, -`identity`, or `all`, comma-separated. `core` is always registered and cannot be -dropped; **with no parameter a session gets `core` only** (10 tools, roughly -2.3k o200k tokens, against ~27.7k for all 77). Callers who want everything must say -`?toolsets=all`. Both figures are printed by `test/mgmt-toolsets.test.ts` on -every run rather than being maintained here; read that output, not this sentence, -when the number has to be exact. +session registers: `core`, `data`, `keys`, `usage`, `billing`, `notifications`, +`team`, `identity`, or `all`, comma-separated. `core` is always registered and +cannot be dropped; **with no parameter a session gets `core` plus `data`** +(27 tools, roughly 8.8k o200k tokens, against ~34.6k for all 94). Callers who +want everything must say `?toolsets=all`; callers who want the account tools +WITHOUT the chain reads say `?toolsets=core` (10 tools, ~2.4k). Every figure is +printed by `test/mgmt-toolsets.test.ts` on every run rather than being maintained +here; read that output, not this sentence, when the number has to be exact. + +**The default changed in SHARK-3629**, and it is the one behavioural change on +this endpoint that an existing caller can notice: a connection that names no +`?toolsets=` used to get `core` alone and now also gets the sixteen chain reads +plus `mgmt_select_key`. It only ever ADDS tools, so nothing a caller already +depended on moved, but the entry cost went from ~2.4k to ~8.8k tokens. A caller +that wants the old listing asks for it by name. Those are REAL o200k counts. `mgmt_list_toolsets` prints slightly larger numbers -for the same two listings (~2.5k and ~30.1k) because the served process carries -no tokenizer and estimates at four characters per token. The estimate runs 5.6% -to 10.7% high across the eight selections, measured on every test run and gated -at 15%. Same quantity, two measurement methods, and the estimate is deliberately -the one that overshoots: a caller is never surprised by a listing that costs more -than it was told. +for the same listings (~9.0k for the default and ~37.1k for all) because the +served process carries no tokenizer and estimates at four characters per token. +The estimate runs 2.6% to 10.9% high across the nine selections, measured on +every test run and gated at 15%. Same quantity, two measurement methods, and the +estimate is deliberately the one that overshoots: a caller is never surprised by +a listing that costs more than it was told. Four properties this parameter has, and each one is a test: @@ -132,7 +140,9 @@ cost was measured rather than assumed: building every group costs 1.006 ms and register-everything-disabled would have charged every default session ~0.87 ms and ~510 KB for tools it never lists. Against a 512Mi pod with a bounded session registry that is a real bill. The tool itself costs `core` one extra entry: 223 -o200k tokens, 2039 → 2262, still inside the 2400 budget the test asserts. +o200k tokens, 2039 → 2262 when SHARK-3609 measured it. The budget the test +asserts is on the DEFAULT selection rather than on `core`, and since SHARK-3629 +that is `core` plus `data`: 8,823 measured against an 8,900 ceiling. Any session can call `mgmt_list_toolsets` (it is in `core`) for each group's tool count, approximate token cost and reconnect URL; the same catalogue is one line @@ -173,13 +183,24 @@ with the session store when that is externalized. The `/mcp` data path is ## Tools (PoC) -**77 tools are registered** on the management server (`?toolsets=all`; 75 before -SHARK-3600 added `mgmt_list_toolsets` to `core` and SHARK-3609 added -`mgmt_load_toolset` beside it), of which **32 are HITL-gated**. -Both counts are held by `test/mgmt-annotations.test.ts`, which asserts the -classified sets partition the registered surface exactly, so a new tool cannot -land unclassified; `test/helpers/mgmtToolSurface.ts` is where the 77 are written -out by name. The bullets below are the operationally interesting families, not +**94 tools are registered** on the management server (`?toolsets=all`; 75 before +SHARK-3600 added `mgmt_list_toolsets` to `core`, SHARK-3609 added +`mgmt_load_toolset` beside it and SHARK-3629 added the `data` group's sixteen +chain reads plus `mgmt_select_key`), of which **32 are HITL-gated**. +The HITL count and the classification of the **78 management** tools are held by +`test/mgmt-annotations.test.ts`, which asserts the classified sets partition the +MANAGEMENT surface exactly, so a new tool cannot land unclassified. The **16** +chain-read tools are governed by `test/annotations.test.ts` instead — the whole +chain plane is read-only and open-world, which is the opposite of what the +management rules assume — and the boundary between the two is `DATA_TOOL_NAMES` +in `src/server.ts`, pinned against the live `/rpc` surface by +`test/data-tool-surface.test.ts`. + +Note the two counts do not split the `data` GROUP down the middle by accident: +the group has seventeen members, and the seventeenth is `mgmt_select_key`, which +travels with the chain tools but acts on the session and stays in the management +partition (`isDataToolName("mgmt_select_key") === false`, asserted). `test/helpers/mgmtToolSurface.ts` is where all +94 are written out by name, split into their groups. The bullets below are the operationally interesting families, not the inventory; `tools/list` on a live pod is. - `mgmt_get_usage` (SHARK-3375) — read-only; `GET /auth/intervalUsage`. diff --git a/USER-STORIES.md b/USER-STORIES.md index b78e03b..ba2b9b2 100644 --- a/USER-STORIES.md +++ b/USER-STORIES.md @@ -109,14 +109,14 @@ reason. ## 7. Data plane (the RPC itself) -| # | Story | Status | Serving tool / note | -| --- | ------------------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| 7.1 | Read chain data with compressed, decoded output | **DONE** | 16 tools, TORPC tier 2 where the proxy applies it. **Correction (SHARK-3598): this row said 17, and the earlier SHARK-3570 edit moved it from 16 UP to 17 against stale code on this branch rather than against the rolled-out data plane, which served 16.** The registered count is 16 because `getChainStats` is gone: the AAPI method behind it, `ankr_getBlockchainStats`, was removed from the Advanced API entirely (live probe `-32075 Method disabled, restricted by blockchain schema` recorded in SHARK-3527; removal in SHARK-3524, and on this branch in SHARK-3598), so the tool could not succeed on any key. The number is no longer maintained by hand: `test/data-tool-surface.test.ts` reads this row and `README.md` and fails when either disagrees with the live `tools/list` | -| 7.2 | Know when compression degraded | **DONE** | `tier_degraded` in the body plus `_meta.tier` (SHARK-3524) | -| 7.3 | Call any read method not covered by a routed tool | **DONE** | **Status corrected (SHARK-3570): this row carried `YES`, which the legend at the top of this file does not define.** The four defined statuses are DONE, PARTIAL, GAP and N/A; an undefined fifth one cannot be read as "verified by test or live run" or as anything else, so it read as a gap that was not filed. It is DONE on the legend's own terms: pinned by `test/rpcCall.test.ts` and by the live-probe result recorded per method at the call site. **Correction (SHARK-3393 / SHARK-3560, 2026-08-05): this row described the guard that was REMOVED, and described it as the shipped behaviour.** It said `rpcCall` is a default-deny read allowlist and that ten legitimate reads (`web3_sha3`, `net_listening`, `net_peerCount`, `eth_mining`, `eth_hashrate`, `eth_coinbase`, `eth_createAccessList`, `debug_storageRangeAt`, `txpool_content`, `txpool_inspect`) had been re-admitted as exact-match entries. There is no read allowlist any more. The guard is a WRITE DENYLIST only, and everything it does not refuse is FORWARDED to the endpoint. What still refuses locally is the class the proxy forwards rather than judges: transaction broadcast and signing, transaction construction (including Sui's `unsafe_*` builders, which `unsafe_moveCall` used to slip past on the "call" substring), node administration, named node and wallet state mutation, mutating verbs, and the operational half of geth's `debug_` namespace. Which READS exist is decided by the two layers that are current by construction and that a list in this repository can never match: the per-chain blockchain schema in the proxy, which answers `-32075 Method disabled, restricted by blockchain schema`, and the tenant the caller's authenticated session resolves to. So the ten methods above are no longer refused locally, and availability is the proxy's per-chain answer exactly as before (six of the ten answer `-32075` on eth/bsc, as `txpool_status` always has). SHARK-3560 is dissolved along with the mechanism that created it rather than fixed; the test that pinned its refusals now pins the forwarding. The behaviour change and its risk are stated in `REVIEW-READY.md` section 4.5 | -| 7.4 | Broadcast a transaction | **N/A** | Out of scope by design: custody belongs to a wallet / AgentKit, not to an RPC MCP | -| 7.5 | Use the key I just created for these calls | **PARTIAL** | Decided (SHARK-3545): keep the session binding, state the limit. A per-call key would ride in the request body, which the per-request key check does not inspect, so a session could be driven with a credential it was never opened with, and the data plane has no principal to scope an override against. So the token is returned and usable over plain HTTPS at once (1.1), and the one step that remains is stated where it is met: the create/reveal reply says a new session is what makes the data tools use this key, the data server's instructions say the same at `initialize`, and a wrong-key follow-up is refused with the remedy, not a bare 401 | -| 7.6 | Machine-readable results | **GAP** | No `outputSchema` / `structuredContent` yet. SHARK-3540 part 2, decided: structured payload with a compact text summary | +| # | Story | Status | Serving tool / note | +| --- | ------------------------------------------------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 7.1 | Read chain data with compressed, decoded output | **DONE** | 16 tools, TORPC tier 2 where the proxy applies it. **Correction (SHARK-3598): this row said 17, and the earlier SHARK-3570 edit moved it from 16 UP to 17 against stale code on this branch rather than against the rolled-out data plane, which served 16.** The registered count is 16 because `getChainStats` is gone: the AAPI method behind it, `ankr_getBlockchainStats`, was removed from the Advanced API entirely (live probe `-32075 Method disabled, restricted by blockchain schema` recorded in SHARK-3527; removal in SHARK-3524, and on this branch in SHARK-3598), so the tool could not succeed on any key. The number is no longer maintained by hand: `test/data-tool-surface.test.ts` reads this row and `README.md` and fails when either disagrees with the live `tools/list` | +| 7.2 | Know when compression degraded | **DONE** | `tier_degraded` in the body plus `_meta.tier` (SHARK-3524) | +| 7.3 | Call any read method not covered by a routed tool | **DONE** | **Status corrected (SHARK-3570): this row carried `YES`, which the legend at the top of this file does not define.** The four defined statuses are DONE, PARTIAL, GAP and N/A; an undefined fifth one cannot be read as "verified by test or live run" or as anything else, so it read as a gap that was not filed. It is DONE on the legend's own terms: pinned by `test/rpcCall.test.ts` and by the live-probe result recorded per method at the call site. **Correction (SHARK-3393 / SHARK-3560, 2026-08-05): this row described the guard that was REMOVED, and described it as the shipped behaviour.** It said `rpcCall` is a default-deny read allowlist and that ten legitimate reads (`web3_sha3`, `net_listening`, `net_peerCount`, `eth_mining`, `eth_hashrate`, `eth_coinbase`, `eth_createAccessList`, `debug_storageRangeAt`, `txpool_content`, `txpool_inspect`) had been re-admitted as exact-match entries. There is no read allowlist any more. The guard is a WRITE DENYLIST only, and everything it does not refuse is FORWARDED to the endpoint. What still refuses locally is the class the proxy forwards rather than judges: transaction broadcast and signing, transaction construction (including Sui's `unsafe_*` builders, which `unsafe_moveCall` used to slip past on the "call" substring), node administration, named node and wallet state mutation, mutating verbs, and the operational half of geth's `debug_` namespace. Which READS exist is decided by the two layers that are current by construction and that a list in this repository can never match: the per-chain blockchain schema in the proxy, which answers `-32075 Method disabled, restricted by blockchain schema`, and the tenant the caller's authenticated session resolves to. So the ten methods above are no longer refused locally, and availability is the proxy's per-chain answer exactly as before (six of the ten answer `-32075` on eth/bsc, as `txpool_status` always has). SHARK-3560 is dissolved along with the mechanism that created it rather than fixed; the test that pinned its refusals now pins the forwarding. The behaviour change and its risk are stated in `REVIEW-READY.md` section 4.5 | +| 7.4 | Broadcast a transaction | **N/A** | Out of scope by design: custody belongs to a wallet / AgentKit, not to an RPC MCP | +| 7.5 | Use the key I just created for these calls | **DONE** | Ships in SHARK-3629, on the MANAGEMENT endpoint. `/mcp` now serves the data tools itself, and the key they spend is resolved SERVER-SIDE from the account the session is already signed in as: slot 0, the Default project key, or whichever slot `mgmt_select_key` names. So a key created a moment ago is reachable by naming its slot, on the same connection, with no reconnection, nothing pasted and no credential in the conversation. The 2026-08-07 measurement this ticket started from is what made it worth doing: reaching the chain tools used to cost a second MCP entry, a hand-pasted key and a CLIENT RESTART, and it was the restart that stopped people. **The RAW-key plane at `/rpc` is unchanged and still PARTIAL in the SHARK-3545 sense** — it binds one key at connect, on purpose: a per-call key would ride in the request body, which the per-request key check does not inspect, so a session could be driven with a credential it was never opened with, and that plane has no principal to scope an override against. What changed is that there is now an endpoint WITH a principal, which is the thing that makes switching safe rather than a hole | +| 7.6 | Machine-readable results | **GAP** | No `outputSchema` / `structuredContent` yet. SHARK-3540 part 2, decided: structured payload with a compact text summary | ## 8. Teams and roles diff --git a/src/mgmt/data/keySession.ts b/src/mgmt/data/keySession.ts new file mode 100644 index 0000000..67cbc2b --- /dev/null +++ b/src/mgmt/data/keySession.ts @@ -0,0 +1,174 @@ +// SHARK-3629 — the key the data tools use, for a session that has an account. +// +// THE PROBLEM THIS SOLVES. buildProvider and buildTorpcClient bake the key into +// the URL at construction, which is right for /rpc (one key, presented at +// connect) and wrong here: a management session knows the account, so it can +// resolve the key itself, and a user must be able to point the data tools at a +// different key without tearing the connection down. Rebuilding the clients is +// cheap; re-initializing an OAuth-protected MCP session is not. +// +// HOW. The tools are handed PROXIES rather than clients. Every call goes +// through the holder, which resolves the selected slot to an endpoint token on +// first use, caches it per slot, and swaps the underlying clients when the +// selection changes. Nothing the tools see changes, so the seventeen data tools +// are registered exactly as they are on /rpc. +// +// THE CREDENTIAL NEVER SURFACES. resolveKeyTarget hands back the token for the +// gateway and a label for humans; only the label leaves this module. +import type { GatewayClient } from "../gateway/client.js"; +import type { WorkerClient } from "../gateway/worker.js"; +import { scopeOf } from "../gateway/groupScope.js"; +import { resolveKeyTarget } from "../tools/keyAddressing.js"; +import { buildProvider } from "../../provider.js"; +import { buildTorpcClient } from "../../torpc/client.js"; + +/** The slot every account has: the key the console calls "Default". */ +export const DEFAULT_KEY_SLOT = 0; + +type Provider = ReturnType; +type Torpc = ReturnType; + +export type KeySelection = + { ok: true; index: number; label: string } | { ok: false; text: string }; + +export type DataKeySession = { + /** Clients that follow the selection. Safe to register tools with once. */ + provider: Provider; + torpc: Torpc; + /** Point the data tools at another slot, for the rest of this session. */ + select: (index: number) => Promise; + /** The slot in force, for a reply that has to name it. */ + currentIndex: () => number; +}; + +type Resolved = { + token: string; + label: string; + provider: Provider; + torpc: Torpc; +}; + +/** + * A stand-in that defers every call until the key is known. + * + * Deliberately narrow: it forwards METHOD calls, which is all the data tools + * make of these clients. A property read would have to be answered before the + * token exists, and answering it with a promise would be a lie, so it is not + * supported rather than faked. + */ +function following(current: () => Promise): T { + return new Proxy({} as T, { + get: (_target, prop) => { + // `then` MUST be absent, and this is not defensive tidying. A `get` that + // answers every name with a function makes this object THENABLE, so the + // first `await` or `Promise.resolve` anywhere near it calls `then(resolve, + // reject)` — and the handler below would resolve the real client, find no + // `then` on it, return undefined and never touch either callback. The + // await would hang forever, on a request path, with no error to see. The + // client is not a promise, so it says so. + if (prop === "then") return undefined; + return async (...args: unknown[]): Promise => { + const real = await current(); + const value = Reflect.get(real, prop) as unknown; + if (typeof value !== "function") return value; + return (value as (...a: unknown[]) => unknown).apply(real, args); + }; + }, + }); +} + +export function createDataKeySession({ + gateway, + worker, +}: { + gateway: GatewayClient; + worker?: WorkerClient; +}): DataKeySession { + let index = DEFAULT_KEY_SLOT; + // Cached, and the cache key is the ACCOUNT plus the slot rather than the slot + // alone. + // + // A slot number means nothing on its own: `mgmt_select_account` can move this + // session between the accounts the login holds a seat on, and slot 4 of one + // team is a different key from slot 4 of another. Cached by slot alone, the + // first resolution won and every later chain read went out on the PREVIOUS + // account's key — including the slot-0 default, which is resolved on the first + // data call and would then have outlived any number of account switches. The + // session would say it was acting on one account (mgmt_whoami, the account + // line every wrapped tool prints) while the reads were billed to another. + // + // Keyed this way, an account switch simply misses the cache and re-resolves + // through the gateway, which is already scoped to the account in force. The + // SLOT survives a switch on purpose: "use slot 4" is a statement about the + // account the session is on, so after moving it means slot 4 of the new one. + const resolved = new Map>(); + + // The personal account has no selection, and `undefined` is a perfectly good + // cache identity for it — it is one specific account, the login's own. + const cacheKey = (slot: number): string => + `${scopeOf(gateway)?.current() ?? "personal"}#${String(slot)}`; + + const build = async (slot: number): Promise => { + const target = await resolveKeyTarget({ gateway, worker, index: slot }); + if (!target.ok) throw new Error(target.text); + return { + token: target.token, + label: target.label, + provider: buildProvider(target.token), + torpc: buildTorpcClient(target.token), + }; + }; + + /** + * The clients for `slot` on the account in force, resolving at most once. + * + * The PROMISE is cached rather than its value, so two data calls that arrive + * before the first resolution finishes share one gateway read and one worker + * exchange instead of racing two. A rejection is evicted: a key that failed to + * resolve once must be retryable, not cached as broken for the session's life. + * + * `fresh` bypasses the cache and replaces the entry. A session can DELETE the + * key in a slot and create another in the same slot without leaving, and the + * cache cannot see either write — so mgmt_select_key, the one place that + * NAMES the key back to a human, always re-reads rather than reporting a + * label the account no longer agrees with. That is bounded: one extra gateway + * read and one worker exchange, on an explicit tool call, which is exactly + * what the equivalent key tool pays. It does not close the case where the + * slot is rebuilt and nothing selects again — those reads keep the stale + * token until the upstream refuses it, which is visible rather than silent — + * and closing that properly means invalidating from the key-write tools. + */ + const resolveFor = (slot: number, fresh = false): Promise => { + const key = cacheKey(slot); + const hit = resolved.get(key); + if (hit && !fresh) return hit; + const made = build(slot).catch((e: unknown) => { + resolved.delete(key); + throw e; + }); + resolved.set(key, made); + return made; + }; + + const currentResolved = (): Promise => resolveFor(index); + + return { + provider: following( + async () => (await currentResolved()).provider + ), + torpc: following(async () => (await currentResolved()).torpc), + currentIndex: () => index, + select: async (next: number): Promise => { + let made: Resolved; + try { + made = await resolveFor(next, true); + } catch (e: unknown) { + // Only after the slot is known to resolve: a failed switch must leave + // the session on the key it was working with, not on a broken one. + return { ok: false, text: e instanceof Error ? e.message : String(e) }; + } + index = next; + return { ok: true, index: next, label: made.label }; + }, + }; +} diff --git a/src/mgmt/server.ts b/src/mgmt/server.ts index dcbbb23..10a6618 100644 --- a/src/mgmt/server.ts +++ b/src/mgmt/server.ts @@ -18,6 +18,7 @@ import { buildVersion } from "../buildInfo.js"; import type { GatewayClient } from "./gateway/client.js"; import { registerMgmtTools } from "./tools/index.js"; import { type MgmtDeps, defaultMgmtDeps } from "./tools/confirmation.js"; +import { DATA_TOOL_CONTRACTS } from "../server.js"; import type { ToolsetName } from "./toolsets.js"; /** @@ -90,13 +91,44 @@ export const MGMT_INSTRUCTIONS = // the agent does the expensive thing the old sentence taught it to do. "5. TOOL GROUPS. This connection registers only the groups it asked for, so a " + "tool you expect may simply not be loaded. Groups: core (always on, cannot be " + - "dropped), keys, usage, billing, notifications, team, identity. If a tool you " + + "dropped), data (blockchain reads: balances, blocks, logs, transactions, " + + "token prices, contract resolution and a generic rpcCall), keys, usage, " + + "billing, notifications, team, identity. If a tool you " + "need is missing, call mgmt_load_toolset with the group that holds it: it is " + "registered in THIS session, your tool list is updated and no reconnection or " + "re-authentication happens. Do not reconnect for this. `?toolsets=` on the " + "MCP URL still sets what a NEW connection starts with, comma-separated (for " + "example `?toolsets=core,keys,billing`) or `all`; with no parameter you get " + - "core. Call mgmt_list_toolsets for each group's size and cost."; + "core and data. Call mgmt_list_toolsets for each group's size and cost.\n\n" + + // SHARK-3629. Contract 1 of the data plane's own instructions, restated for an + // endpoint where it is different: /rpc binds one key at connect, this one + // resolves the account's and can be repointed. An agent that does not know the + // key is resolved FOR it goes looking for a credential to paste and finds + // nothing to paste it into. + "6. THE CHAIN TOOLS' KEY. The reads in the `data` group are billed to this " + + "account's own API key, which this server resolves itself: slot 0, the " + + "account's Default project key, unless mgmt_select_key names another slot as " + + "mgmt_list_api_keys shows it. Nothing is pasted, no credential is shown, and " + + "a change takes effect on the next data call over this same connection. This " + + "replaces the bound-key contract the raw-key endpoint at rpc.ankr.com states " + + "as its contract 1; the rest of that endpoint's contracts hold here unchanged " + + "and follow, numbered as they are there.\n\n" + + // Shared with src/server.ts rather than copied. SHARK-3599 lifted this prose + // OUT of the 16 tool descriptions because instructions carry it — an argument + // that only holds on an endpoint whose instructions actually do. This one's + // did not, and the RAW BASE UNITS rule lives nowhere else, so an agent here + // decoding a transfer would have reported an amount wrong by 10^decimals with + // nothing in the response to contradict it. + // + // UNCONDITIONAL, rather than appended only when `data` is in the selection, + // and the choice is deliberate. Instructions are delivered ONCE, at + // initialize, and mgmt_load_toolset can add `data` to a live session + // afterwards — so a selection-dependent block would be absent exactly when a + // core-only session went and loaded the chain tools, which is the silent + // version of the defect this fixes. The cost of being wrong the other way is + // that a session which never loads `data` reads ~600 tokens of prose about + // tools it does not have, once. + DATA_TOOL_CONTRACTS; export const createMgmtServer = ( gateway: GatewayClient, diff --git a/src/mgmt/tools/index.ts b/src/mgmt/tools/index.ts index 99dd17a..6c12a4e 100644 --- a/src/mgmt/tools/index.ts +++ b/src/mgmt/tools/index.ts @@ -28,6 +28,22 @@ import { registerPaymentWrites } from "./paymentWrites.js"; import { registerBundles } from "./bundles.js"; import { registerPinAccount, withAccountScope } from "./accountScope.js"; import { scopeOf } from "../gateway/groupScope.js"; +// SHARK-3629. A STATIC import, and the cost is stated rather than left to be +// discovered: it pulls the data plane, and through it gpt-tokenizer, into the +// management binary at boot. Measured on this tree, importing src/mgmt/server.ts +// moves RSS 79 -> 189 MB and takes 1.04 s, the tokenizer being 65 MB and 386 ms +// of that, against the 512Mi pod and 5 s HEALTHCHECK in DEPLOY-MGMT.md. +// +// It is static because the alternative is not free either. `data` is in the +// DEFAULT selection, so nearly every session loads it anyway, and the group +// thunks run inside registerAsOneChange, whose batching of +// notifications/tools/list_changed depends on a strictly SYNCHRONOUS window that +// an `await import()` would break. Deferring the cost properly means making the +// tokenizer itself lazy in torpc/tokens.ts, which is a change to the data +// plane's hot path and wants its own measurement. Flagged, not smuggled. +import { registerDataTools } from "../../server.js"; +import { createDataKeySession } from "../data/keySession.js"; +import { registerSelectKey } from "./selectKey.js"; import { registerAccountSelection } from "./accountSelection.js"; import { createTwoFactorProbe, registerTwoFactorStatus } from "./twoFactor.js"; import { registerSessions } from "./sessions.js"; @@ -128,6 +144,37 @@ export function registerMgmtTools({ // Nothing else moved. Each thunk holds exactly the registrar calls and the // comments its `if` block held before. const groups: Record, () => void> = { + // === data ============================================================== + // + // SHARK-3629. The chain-read tools, the reason people integrate Ankr at + // all, on the endpoint that already knows the account. The key is resolved + // server-side from slot 0 and can be moved with mgmt_select_key without a + // reconnect; nothing is pasted and no credential enters the conversation. + // + // On the RAW server: a chain read is not an account-scoped answer, and the + // scope wrapper would append the selected account to every block and + // balance it returns. + // + // registerDataTools is the SAME registrar /rpc uses, imported rather than + // copied, so the two endpoints cannot drift into different surfaces. + data: () => { + const keys = createDataKeySession({ + gateway, + worker: sessionDeps.worker, + }); + registerDataTools({ + server: rawServer, + provider: keys.provider, + torpc: keys.torpc, + }); + // mgmt_select_key, unlike the tools above it, goes on the WRAPPED server. + // Its answer IS about the account — which of THIS account's key slots the + // session will spend — so the scope wrapper's `expectAccount` and its + // account line are the point rather than noise: naming slot 4 while the + // session is aimed somewhere you did not expect is exactly the mistake + // worth refusing. + registerSelectKey({ server, keys }); + }, // === identity ========================================================== identity: () => { registerPinAccount({ server: rawServer, gateway }); @@ -429,11 +476,13 @@ const measureToolsets = async ( tools: tools.length, // chars/4. NOT what `_meta.token_count` uses — that is a real o200k_base // count (src/torpc/tokens.ts) and this comment claimed otherwise until - // SHARK-3524's review round. The management binary carries no tokenizer on - // purpose (RSS 42 -> 111 MB for one advisory number), so this is an - // estimate, measured 5.6-10.7% HIGH across the eight selections and gated - // at 15% in test/mgmt-toolsets.test.ts. See listToolsets.ts for the full - // note. + // SHARK-3524's review round. It then claimed the management binary carries + // no tokenizer, which SHARK-3629 falsified by importing the data plane + // here: the tokenizer is in this process at boot either way. So this stays + // an estimate by choice rather than by constraint, measured 2.6-10.9% HIGH + // across the nine selections and gated at 15% in + // test/mgmt-toolsets.test.ts. See listToolsets.ts for the full note and + // the two decisions it leaves open. tokens: Math.ceil(JSON.stringify(tools).length / 4), }); } diff --git a/src/mgmt/tools/listToolsets.ts b/src/mgmt/tools/listToolsets.ts index c701909..08d4d52 100644 --- a/src/mgmt/tools/listToolsets.ts +++ b/src/mgmt/tools/listToolsets.ts @@ -29,25 +29,45 @@ // that SHARK-3525 removed chars/4 from the data plane precisely because it // UNDERSTATES real usage. chars/4 survives in exactly one place in src/: here. // -// WHY IT SURVIVES. The management binary carries no tokenizer, and importing one -// is measured in tokens.ts at RSS 42 -> 111 MB steady against a 512Mi pod — a -// real cost for one advisory number in one tool. So the estimate stays and the -// claim about it is now measured rather than remembered. +// WHY IT SURVIVED, AND WHY THAT REASON EXPIRED IN SHARK-3629. The argument was +// that the management binary carries no tokenizer and importing one costs RSS +// 42 -> 111 MB steady against a 512Mi pod, which is a lot for one advisory +// number in one tool. That is no longer the situation: tools/index.ts imports +// registerDataTools from src/server.ts, which reaches torpc/tokens.ts, so the +// tokenizer is loaded at boot whether or not a session ever asks for a chain. +// Measured on this tree: importing src/mgmt/server.ts moves RSS 79 -> 189 MB and +// takes 1.04 s, of which the tokenizer alone is 82 -> 147 MB and 386 ms. // -// MEASURED on this tree, chars/4 against o200k_base for all eight selections: -// between 5.6% and 10.7% HIGH (core 9.2%, keys 5.6%, usage 8.8%, billing 9.6%, -// notifications 10.7%, team 10.2%, identity 9.5%, all 8.5%). High is the safe -// direction for a budget — a caller is never surprised by a listing that costs -// more than it was told — but it is a 5-11% band, not the "about 25%" this -// comment used to assert. test/mgmt-toolsets.test.ts computes the real o200k -// number next to the estimate and fails past 15%, so the band cannot drift away -// from this paragraph again. +// So chars/4 is now a CHOICE rather than a constraint, and it is left as it is +// pending a decision rather than changed on the way past. Two things follow, and +// both are open: whether this tool should simply report the real count now that +// the tokenizer is in the process anyway (which would retire the band test +// below), and whether the data plane's import belongs behind the `data` thunk so +// a core-only session stops paying ~107 MB and ~1 s for tools it never lists. +// Neither is decided here; what is fixed here is the comment, which asserted a +// property of the binary that its own imports contradict. +// +// MEASURED on this tree, chars/4 against o200k_base for all nine selections: +// between 2.6% and 10.9% HIGH (core 9.9%, data 2.6%, keys 6.8%, usage 9.3%, +// billing 9.9%, notifications 10.9%, team 10.4%, identity 10.0%, all 7.3%). High +// is the safe direction for a budget — a caller is never surprised by a listing +// that costs more than it was told — but it is a 3-11% band, not the "about 25%" +// this comment used to assert. test/mgmt-toolsets.test.ts computes the real +// o200k number next to the estimate and fails past 15%, so the band cannot drift +// away from this paragraph again. +// +// SHARK-3629 widened the band at the bottom rather than the top: `data` is the +// closest row at 2.6%, because chars/4 tracks real tokenization better on the +// chain tools' prose than on the management tools'. Nothing about the ceiling +// moved. // // One consequence to know when reading numbers about this surface: DEPLOY-MGMT.md -// quotes the REAL o200k counts (~2.3k for core, ~27.7k for all), because that is -// what the test prints, while the tool a caller runs prints the estimate (~2.5k -// and ~30.1k). Same quantity, two measurement methods, both stated as such. -// (SHARK-3609 moved both pairs by one tool: mgmt_load_toolset joined `core`.) +// quotes the REAL o200k counts (~8.8k for the default, ~34.6k for all), because +// that is what the test prints, while the tool a caller runs prints the estimate +// (~9.0k and ~37.1k). Same quantity, two measurement methods, both stated as +// such. (SHARK-3609 moved both pairs by one tool: mgmt_load_toolset joined +// `core`. SHARK-3629 moved them again, by putting the sixteen chain reads into +// the default and into `all`; core alone is still ~2.4k real, ~2.6k estimated.) import { z } from "zod"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { MGMT_READ } from "./annotations.js"; diff --git a/src/mgmt/tools/rolePermissions.ts b/src/mgmt/tools/rolePermissions.ts index 1a9ef6a..21101c2 100644 --- a/src/mgmt/tools/rolePermissions.ts +++ b/src/mgmt/tools/rolePermissions.ts @@ -201,6 +201,15 @@ export const TOOL_CAPABILITY: Readonly> = { // console a DEV opens the project page and sees its endpoints. The annotation // is about what the reply puts in the world; the capability is about what the // gateway is being asked for. + // SHARK-3629 — naming which key slot the session's data tools spend. Mapped to + // the same READ the listing is, and for the same reason the reveal is: the + // capability is about what the gateway is asked for, and this asks it for the + // account's key list. A seat that may not see which projects exist has no + // business naming one of them by index. It is not a write — no row changes and + // the credential is never shown — so it takes JwtManagerRead rather than the + // write, and a seat that holds the read can switch keys as freely as it can + // list them. + mgmt_select_key: "JwtManagerRead", mgmt_reveal_api_key: "JwtManagerRead", mgmt_get_allowlist: "JwtManagerRead", mgmt_get_allowlist_mode: "JwtManagerRead", diff --git a/src/mgmt/tools/selectKey.ts b/src/mgmt/tools/selectKey.ts new file mode 100644 index 0000000..9643a6e --- /dev/null +++ b/src/mgmt/tools/selectKey.ts @@ -0,0 +1,75 @@ +// SHARK-3629 — choose which key the data tools use, inside a live session. +// +// The analogue of mgmt_select_account, one level down: that one picks the +// ACCOUNT a session acts on, this one picks which of that account's keys the +// chain tools spend. Both change only what this connection does next, which is +// why both are annotated as reads: nothing on the account is modified, no +// credential is disclosed, and closing the session forgets it. +// +// It exists because the alternative is a reconnect. The raw-key plane binds one +// key at connect and cannot be repointed; a management session must not inherit +// that limit, since it knows the account and can resolve any of its keys. +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { MGMT_READ } from "./annotations.js"; +import type { DataKeySession } from "../data/keySession.js"; + +export function registerSelectKey({ + server, + keys, +}: { + server: McpServer; + keys: DataKeySession; +}): void { + server.registerTool( + "mgmt_select_key", + { + title: "Choose which API key the data tools use", + annotations: MGMT_READ, + description: + "Point the blockchain data tools at one of this account's API keys, " + + "for the rest of this session. The key is named by its slot `index`, " + + "as mgmt_list_api_keys shows it, and this server resolves the slot to " + + "the key's endpoint token itself: the credential is never shown here. " + + "Takes effect on the NEXT data call, on this same connection — no " + + "reconnection and no new sign-in. Without it the data tools use slot " + + "0, the account's Default key. Nothing on the account is changed.", + // `.strict()` for the same reason every other tool here has it: an + // undeclared argument is a caller believing something this tool does not + // do, and silently dropping it would let that belief survive. + inputSchema: z + .object({ + index: z + .number() + .int() + .min(0) + .max(128) + .describe( + "Slot index of the key to use, as mgmt_list_api_keys shows it." + ), + }) + .strict(), + }, + async ({ index }) => { + const chosen = await keys.select(index); + if (!chosen.ok) { + return { + isError: true, + content: [{ type: "text" as const, text: chosen.text }], + }; + } + return { + content: [ + { + type: "text" as const, + text: + `Done: the data tools in this session now use API key ` + + `${chosen.label}. It took effect immediately, on this ` + + `connection, and applies to every data call until it is changed ` + + `again or the session ends.`, + }, + ], + }; + } + ); +} diff --git a/src/mgmt/toolsets.ts b/src/mgmt/toolsets.ts index 699babc..ac38ce7 100644 --- a/src/mgmt/toolsets.ts +++ b/src/mgmt/toolsets.ts @@ -1,12 +1,19 @@ // SHARK-3600 — which groups of management tools a session registers. // -// WHY THIS EXISTS. The management plane advertises 75 tools whose `tools/list` -// is about 27,260 o200k tokens. A client pays that before it has asked anything, -// and almost no session needs keys, usage, billing, notifications, teams and -// login identity at once. `?toolsets=` on the connection URL lets a caller say -// which concerns it came for; `core` is always registered, so a session that -// asks for nothing still knows who it is, what its keys and usage are, and how -// to come back for more (mgmt_list_toolsets). +// WHY THIS EXISTS. The management plane advertised 75 tools whose `tools/list` +// was about 27,260 o200k tokens. A client pays that before it has asked +// anything, and almost no session needs keys, usage, billing, notifications, +// teams and login identity at once. `?toolsets=` on the connection URL lets a +// caller say which concerns it came for; `core` is always registered, so a +// session that asks for nothing still knows who it is, what its keys and usage +// are, and how to come back for more (mgmt_list_toolsets). +// +// The surface has grown since — SHARK-3629 put the sixteen chain reads on this +// endpoint too, and `all` is 94 tools and about 34,600 tokens — which is the +// argument for this module rather than against it: the whole listing costs more +// than ever, and a default connection pays 8,823 of it. The live numbers are +// printed by test/mgmt-toolsets.test.ts on every run; the figures in this +// paragraph are the shape, not the source. // // THREE PROPERTIES THIS MODULE IS RESPONSIBLE FOR, all of them security ones: // @@ -61,11 +68,16 @@ * * These match the grouping the registrars in tools/index.ts already followed; * the module comments there are the argument for each boundary. `core` is in the - * list because it is a valid thing to ASK for (`?toolsets=core` is the default - * spelled out), not because it can be left out. + * list because it is a valid thing to ASK for — since SHARK-3629 it is how a + * caller asks for the account tools WITHOUT the chain reads the default carries + * — not because it can be left out. */ export const TOOLSET_NAMES = [ "core", + // SHARK-3629 — the chain-read tools. Second in the list, and in the DEFAULT + // set, because reading chains is what people come here for; account + // administration is what they do around it. + "data", "keys", "usage", "billing", @@ -157,9 +169,22 @@ const immutable = (names: Iterable): ReadonlySet => { /** Every set. What `?toolsets=all` resolves to. */ export const ALL_TOOLSETS: ReadonlySet = immutable(TOOLSET_NAMES); -/** The default when the URL carries no `toolsets` parameter at all. */ +/** `core` alone. Still a valid thing to ask for, no longer the default. */ export const CORE_ONLY: ReadonlySet = immutable(["core"]); +/** + * SHARK-3629 — the default when the URL carries no `toolsets` parameter. + * + * It used to be `core` alone, which meant the advertised endpoint answered + * account questions and could not read a single chain. A connection that names + * nothing now gets the chain tools plus the session core, and the heavier + * administration groups stay one in-session `mgmt_load_toolset` away. + */ +export const DEFAULT_TOOLSETS: ReadonlySet = immutable([ + "core", + "data", +]); + /** * SHARK-3609 — the sets a LIVE session has loaded, which can grow. * @@ -207,13 +232,33 @@ export type ToolsetResolution = | { ok: true; toolsets: ReadonlySet } | { ok: false; message: string }; +/** + * "a", "a and b", "a, b and c" — for naming a set to a human. + * + * Exported for its own test rather than only through the refusal it composes. + * The two-name case, which is all DEFAULT_TOOLSETS exercises today, cannot tell + * `slice(0, -1)` from `slice(0, 1)`: both yield the first element. The day a + * third group joins the default, one of those is right and the other silently + * drops a name from the sentence that tells a caller what they will get. + */ +export const listPhrase = (names: readonly string[]): string => + names.length < 2 + ? names.join("") + : `${names.slice(0, -1).join(", ")} and ${names.slice(-1).join("")}`; + // Stated once, appended to every refusal. It names the whole valid set, because // the caller cannot see the allowlist and guessing is what got them here. +// +// SHARK-3629: the last sentence is DERIVED from DEFAULT_TOOLSETS rather than +// written out. It used to say "to get core" and that stayed true only for as +// long as nobody changed the default — which this ticket then did. A refusal +// that misstates the default sends the caller to a URL that does not do what +// they were just told it does. const VALID_VALUES = `Valid values are ${TOOLSET_NAMES.join(", ")} and ` + `${ALL_TOOLSETS_KEYWORD}, comma-separated, lower-case. ` + `The core set is always registered and cannot be dropped. ` + - `Omit the parameter entirely to get core.`; + `Omit the parameter entirely to get ${listPhrase([...DEFAULT_TOOLSETS])}.`; const refuse = (why: string): ToolsetResolution => ({ ok: false, @@ -230,12 +275,13 @@ const refuse = (why: string): ToolsetResolution => ({ * being coerced at the call site, where "take the last one" would quietly turn a * duplicated parameter into a widening. * - * Absent (`undefined`) is the DEFAULT and resolves to core. An EMPTY value is - * not absent: `?toolsets=` is a caller asking for something and getting it - * wrong, so it is refused like any other unusable value. + * Absent (`undefined`) is the DEFAULT and resolves to core plus data + * (SHARK-3629). An EMPTY value is not absent: `?toolsets=` is a caller asking + * for something and getting it wrong, so it is refused like any other unusable + * value. */ export const resolveToolsets = (raw: unknown): ToolsetResolution => { - if (raw === undefined) return { ok: true, toolsets: CORE_ONLY }; + if (raw === undefined) return { ok: true, toolsets: DEFAULT_TOOLSETS }; if (typeof raw !== "string") { return refuse("The toolsets parameter must be given at most once."); } diff --git a/src/server.ts b/src/server.ts index 3270421..932abe3 100644 --- a/src/server.ts +++ b/src/server.ts @@ -58,17 +58,24 @@ import { registerGetInteractions } from "./tools/getInteractions.js"; * the one paragraph every session reads at initialize. What contract 5 promises * now is only what the code still does, which is the write refusal. */ -export const DATA_INSTRUCTIONS = - "Blockchain READ tools for ONE Ankr API key: the key presented when this " + - "session was opened. Five contracts apply across every tool here, so they are " + - "stated once instead of in each description.\n\n" + - "1. THE BOUND KEY. That binding is fixed for the life of the session, so a key " + - "obtained later, for example one created through the Ankr management MCP " + - "server, is not reachable from these tools until a NEW session is opened " + - "presenting it. There is deliberately no per-call key argument: the bound key " + - "is part of this session's identity and is re-checked on every request, so a " + - "session that could be repointed mid-flight could also be driven with a " + - "credential it was never opened with.\n\n" + +/** + * SHARK-3629 — the four contracts that are about the TOOLS rather than about how + * this session got its key, split out so both endpoints can deliver them. + * + * WHY IT IS SHARED RATHER THAN COPIED. The management endpoint now serves this + * same tool surface, and SHARK-3599 deliberately LIFTED this prose out of the 16 + * tool descriptions on the argument that instructions carry it. That argument + * only holds where the instructions actually do. On /mcp they did not, and the + * gap was not cosmetic: the RAW BASE UNITS rule exists nowhere else, so an agent + * decoding a transfer there would report an amount wrong by a factor of + * 10^decimals and nothing in the response would contradict it. + * + * Contract 1 is NOT in here, because it is the one thing the two endpoints + * genuinely disagree about: /rpc binds one key for the session's life, /mcp + * resolves the account's own and can be repointed with mgmt_select_key. Each + * states its own, and this block is what they share. + */ +export const DATA_TOOL_CONTRACTS = "2. TORPC TIER, negotiated PER CALL and NOT guaranteed. Where the proxy " + "supports the method and the response fits its compression budget, tier 2 " + "applies: contract calls and event logs are ABI-decoded into named `args` and " + @@ -102,6 +109,19 @@ export const DATA_INSTRUCTIONS = "Chain coverage is deliberately not enumerated in these tools' descriptions, " + "because the set changes whenever Ankr adds a chain: call listChains."; +export const DATA_INSTRUCTIONS = + "Blockchain READ tools for ONE Ankr API key: the key presented when this " + + "session was opened. Five contracts apply across every tool here, so they are " + + "stated once instead of in each description.\n\n" + + "1. THE BOUND KEY. That binding is fixed for the life of the session, so a key " + + "obtained later, for example one created through the Ankr management MCP " + + "server, is not reachable from these tools until a NEW session is opened " + + "presenting it. There is deliberately no per-call key argument: the bound key " + + "is part of this session's identity and is re-checked on every request, so a " + + "session that could be repointed mid-flight could also be driven with a " + + "credential it was never opened with.\n\n" + + DATA_TOOL_CONTRACTS; + export const createServer = (apiKey: string) => { const server = new McpServer( { @@ -111,9 +131,89 @@ export const createServer = (apiKey: string) => { { instructions: DATA_INSTRUCTIONS } ); - const provider = buildProvider(apiKey); - const torpc = buildTorpcClient(apiKey); + registerDataTools({ + server, + provider: buildProvider(apiKey), + torpc: buildTorpcClient(apiKey), + }); + + return server; +}; + +/** + * SHARK-3629 — the names registerDataTools registers, as data rather than as a + * fact you can only learn by connecting a server. + * + * WHY IT HAS TO EXIST. Since this ticket the two planes share a surface, and the + * MANAGEMENT plane's own suites enumerate everything registered on its server + * and classify it: test/mgmt-annotations.test.ts partitions it into read and + * three flavours of write, test/mgmt-role-capabilities.test.ts partitions it + * into capability-gated and capability-free. Both rules are about acting on an + * ACCOUNT, and neither is true of a chain read — getAccountBalance is genuinely + * read-only, which the management partition reads as an unclassified write. The + * data plane has its own annotation contract in test/annotations.test.ts, and + * this list is what lets each suite claim the surface it actually governs + * instead of the whole server. + * + * WHY IT IS A LIST AND NOT A FILTER ON A PREFIX. `mgmt_select_key` is a + * management tool that happens to be registered alongside these, and a rule like + * "names without the mgmt_ prefix are data" would swallow anything else that + * lands here later. Names, so that adding a tool is a decision. + * + * The drift this could introduce is closed by test/data-tool-surface.test.ts asserting + * this list against the surface createServer actually advertises: a tool added to + * registerDataTools and not to this list fails there, and until it is added it is + * also unclassified on the management side, so it cannot slip past either + * partition. + */ +export const DATA_TOOL_NAMES: readonly string[] = [ + "expandResult", + "getAccountBalance", + "getBalances", + "getBlock", + "getInteractions", + "getLogs", + "getNFTs", + "getTokenHolders", + "getTokenPrice", + "getTokenPriceHistory", + "getTransaction", + "getWalletActivity", + "listChains", + "resolveContract", + "rpcCall", + "searchChain", +]; + +/** Membership test for the above, for callers that only ask "is this one?". */ +const DATA_TOOL_NAME_SET: ReadonlySet = new Set(DATA_TOOL_NAMES); + +/** True when `name` is served by the data plane rather than by management. */ +export const isDataToolName = (name: string): boolean => + DATA_TOOL_NAME_SET.has(name); +/** + * SHARK-3629 — the chain-read surface, registered onto a server someone else + * owns. + * + * Extracted from createServer so BOTH planes register the identical set: the + * raw-key server at /rpc, which binds one key at connect, and a management + * session at /mcp, which resolves the account's key itself and can swap it + * mid-session. A second copy of this list would drift, and the drift would be + * invisible until a user found a tool on one endpoint and not the other. + * + * `provider` and `torpc` are taken as VALUES rather than built here, which is + * what lets the management plane hand in clients that follow the selected key. + */ +export const registerDataTools = ({ + server, + provider, + torpc, +}: { + server: McpServer; + provider: ReturnType; + torpc: ReturnType; +}) => { // Kept AAPI tools (unchanged behavior) registerGetAccountBalance({ server, provider }); registerGetTokenPrice({ server, provider }); @@ -141,6 +241,4 @@ export const createServer = (apiKey: string) => { // Discoverability registerListChains({ server }); - - return server; }; diff --git a/test/data-tool-surface.test.ts b/test/data-tool-surface.test.ts index 0808639..453f0fd 100644 --- a/test/data-tool-surface.test.ts +++ b/test/data-tool-surface.test.ts @@ -18,7 +18,11 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; -import { createServer } from "../src/server.js"; +import { + DATA_TOOL_NAMES, + createServer, + isDataToolName, +} from "../src/server.js"; import { EXPECTED_DATA_TOOLS } from "./helpers/dataToolSurface.js"; type ToolResult = { @@ -54,6 +58,44 @@ test("tools/list advertises exactly the expected data tool names, no more, no fe } }); +// SHARK-3629 — and the SAME comparison against the list src exports, which is a +// different thing from the pin above rather than a second copy of it. +// +// EXPECTED_DATA_TOOLS is a TEST-side expectation: its whole job is to be an +// independent transcription that fails when the surface moves. DATA_TOOL_NAMES +// is shipped code, and two management suites now filter by it to decide which +// tools their classification rules govern (see src/server.ts). If it drifted +// from the registered surface, a data tool would go missing from BOTH +// partitions at once — unclassified on the management side because it is a +// chain read, and excluded from the mgmt suites' scope because the filter +// claimed it belonged to this plane. Asserting it here is what makes the filter +// safe to apply there. +test("the exported DATA_TOOL_NAMES is exactly what this plane registers", async () => { + const client = await connectData(); + try { + const { tools } = await client.listTools(); + assert.deepEqual( + [...DATA_TOOL_NAMES].sort(), + tools.map((t) => t.name).sort(), + "DATA_TOOL_NAMES and the registered data surface disagree, so the " + + "management suites are filtering by a stale list" + ); + for (const name of tools.map((t) => t.name)) { + assert.ok( + isDataToolName(name), + `${name} is not recognised as a data tool` + ); + } + assert.equal( + isDataToolName("mgmt_select_key"), + false, + "a management tool must not be filtered out of the management partition" + ); + } finally { + await client.close(); + } +}); + // "isError is true" is NOT a usable assertion here. While the tool still existed, // calling it ALSO came back isError, because its AAPI request goes out over axios // and fails on a dummy key: a test that only checked for an error passed against diff --git a/test/helpers/mgmtToolSurface.ts b/test/helpers/mgmtToolSurface.ts index 8ce8e1c..2b14884 100644 --- a/test/helpers/mgmtToolSurface.ts +++ b/test/helpers/mgmtToolSurface.ts @@ -10,8 +10,9 @@ // // The list below is the 75 tools the management plane served before SHARK-3600, // PLUS mgmt_list_toolsets, which this ticket adds to `core` (it is how a session -// that defaults to core discovers what it is missing). Nothing else was added, -// renamed or removed. +// that defaults to core discovers what it is missing), PLUS the `data` group +// SHARK-3629 added: the sixteen chain reads and mgmt_select_key, which names the +// key they spend. Nothing else was added, renamed or removed. // // The per-set lists are the same partition, split. They are DISJOINT and their // union is exactly EXPECTED_MGMT_TOOLS; both properties are asserted, so a tool @@ -37,6 +38,39 @@ export const CORE_TOOLS = [ /** The optional sets, each WITHOUT the core tools. */ export const OPTIONAL_TOOLSETS: Record = { + // SHARK-3629 — the chain-read plane, served from the management endpoint. + // + // These sixteen names are also src/server.ts's DATA_TOOL_NAMES and are pinned + // there against the surface /rpc advertises. They are written out AGAIN here + // rather than imported for the reason the header states: this file is the list + // that decides what `?toolsets=` may serve, so importing the answer would make + // the two endpoints agree by construction and prove nothing about either. + // Divergence between the two lists means the endpoints diverged, which is the + // finding, not a maintenance nuisance. + // + // mgmt_select_key is here rather than in `keys` because it is useless without + // the tools it aims: a session that loads the chain reads must be able to say + // which key pays for them, and a session that does not load them has nothing + // to point. + data: [ + "expandResult", + "getAccountBalance", + "getBalances", + "getBlock", + "getInteractions", + "getLogs", + "getNFTs", + "getTokenHolders", + "getTokenPrice", + "getTokenPriceHistory", + "getTransaction", + "getWalletActivity", + "listChains", + "mgmt_select_key", + "resolveContract", + "rpcCall", + "searchChain", + ], keys: [ "mgmt_add_allowlist_item", "mgmt_create_api_key", diff --git a/test/mgmt-annotations.test.ts b/test/mgmt-annotations.test.ts index dbd3cca..3a0f47f 100644 --- a/test/mgmt-annotations.test.ts +++ b/test/mgmt-annotations.test.ts @@ -7,12 +7,19 @@ // that would dim or double-check a mutating tool has nothing to go on. These // hints are the declaration; the gate stays the enforcement. // -// WHAT IS PINNED. The classification of every registered tool, by name, in four sets, and -// the two consistency rules that make the sets trustworthy: the sets must -// partition the registered surface exactly (a new tool cannot land +// WHAT IS PINNED. The classification of every MANAGEMENT tool, by name, in four +// sets, and the two consistency rules that make the sets trustworthy: the sets +// must partition the management surface exactly (a new tool cannot land // unclassified), and every HITL-gated tool must be declared not read-only. The // wording of a title is not pinned; its presence and uniqueness are. // +// SHARK-3629 narrowed "registered" to "management". The same server now also +// carries the chain-read plane, which has its own annotation contract in +// test/annotations.test.ts, and the two contracts genuinely disagree: a chain +// read is read-only, which is exactly what the rules below forbid an unclassified +// tool from claiming. See mgmtToolsOf, and the test above it that keeps the +// narrowing from quietly excluding more than it should. +// // destructiveHint follows the specification's binary, not intuition: a tool is // additive when it can only ADD, and destructive otherwise. So freeze (reversible // but not additive) is destructive, while create (additive, idempotent by slot @@ -22,6 +29,7 @@ import assert from "node:assert/strict"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { createMgmtServer } from "../src/mgmt/server.js"; +import { DATA_TOOL_NAMES, isDataToolName } from "../src/server.js"; import type { GatewayClient } from "../src/mgmt/gateway/client.js"; /** Reads. Nothing on the account changes, so a host may call them freely. */ @@ -132,6 +140,13 @@ const READ_TOOLS = [ // confirmation would prevent here. The tool it makes safe, not this tool, is // where the gate belongs. "mgmt_select_account", + // SHARK-3629: choosing which of the account's keys the data tools spend is + // the same kind of act one level down. It changes SESSION state only: no row + // is written, no key is created, changed or disabled, the credential is never + // disclosed, and a repeat lands on the same state. Closing the session forgets + // it. Annotated read for the same reason mgmt_select_account is — a gate here + // would be confirmation fatigue on the tool that makes the others safe. + "mgmt_select_key", "mgmt_whoami", ]; @@ -331,11 +346,82 @@ async function connect(): Promise { return client; } +/** + * SHARK-3629 — the MANAGEMENT tools on this server, which since that ticket is + * no longer the same thing as every tool on it. + * + * The `data` group registers the chain-read plane onto this same server, and + * those tools answer to a DIFFERENT annotation contract, held in + * test/annotations.test.ts: the whole data plane is read-only and open-world, + * so getAccountBalance declares readOnlyHint true and is right to. Run the + * management rules over it and it reads as a write that forgot to say so, which + * is the wrong complaint about the right code. + * + * So the scope is narrowed rather than the lists extended. The filter is + * src/server.ts's own DATA_TOOL_NAMES, asserted against the live data surface in + * test/data-tool-surface.test.ts, which is what stops a data tool being dropped + * from this partition by claiming to be something it is not. `mgmt_select_key` + * is deliberately NOT in it: it rides along with the data group but acts on the + * session, so it stays classified here. + */ +const mgmtToolsOf = (tools: T[]): T[] => + tools.filter((t) => !isDataToolName(t.name)); + +test("SHARK-3629: the data plane is on this server, and it is not what this file governs", async () => { + // Without this, the two partitions below would still pass if DATA_TOOL_NAMES + // were empty or if the data group stopped registering: an exclusion that + // excludes nothing is invisible in a green run. So assert both halves — the + // data tools ARE here, and the filter takes exactly them out. + const client = await connect(); + try { + const { tools } = await client.listTools(); + const all = tools.map((t) => t.name); + const removed = all.filter(isDataToolName).sort(); + assert.deepEqual( + removed, + [...DATA_TOOL_NAMES].sort(), + "the management server no longer registers the whole data surface, so " + + "the scope this file narrows to is not the one it thinks" + ); + assert.ok( + mgmtToolsOf(tools).some((t) => t.name === "mgmt_select_key"), + "mgmt_select_key is a management tool and must stay in this partition" + ); + + // Titles are checked for uniqueness WITHIN each plane below and in + // test/annotations.test.ts, and neither of those sees the other's list. On + // this endpoint they are one list shown to one client, which is exactly + // where a duplicate label confuses someone, so the cross-plane check has to + // live here — the only place that holds both. + const titles = new Map(); + const clashes: string[] = []; + for (const tool of tools) { + const title = tool.title ?? tool.annotations?.title; + if (!title) { + clashes.push(`${tool.name}: no title`); + continue; + } + const first = titles.get(title); + if (first) clashes.push(`${tool.name} reuses the title of ${first}`); + else titles.set(title, tool.name); + } + assert.deepEqual( + clashes, + [], + "the combined surface this endpoint serves has duplicate or missing titles" + ); + } finally { + await client.close(); + } +}); + test("SHARK-3540: the four classified sets partition the registered surface exactly", async () => { const client = await connect(); try { const { tools } = await client.listTools(); - const registered = tools.map((t) => t.name).sort(); + const registered = mgmtToolsOf(tools) + .map((t) => t.name) + .sort(); const classified = [ ...READ_TOOLS, ...ADDITIVE_TOOLS, @@ -366,7 +452,10 @@ test("SHARK-3540: every mgmt tool declares its hints and a distinct title", asyn const titles = new Map(); const problems: string[] = []; - for (const tool of tools) { + // Titles are checked for uniqueness within the management surface only. A + // data tool's title is held distinct by test/annotations.test.ts, over the + // plane where a clash would actually confuse someone. + for (const tool of mgmtToolsOf(tools)) { const a = tool.annotations; if (!a) { problems.push(`${tool.name}: no annotations`); diff --git a/test/mgmt-data-plane-in-session.test.ts b/test/mgmt-data-plane-in-session.test.ts new file mode 100644 index 0000000..841ffe7 --- /dev/null +++ b/test/mgmt-data-plane-in-session.test.ts @@ -0,0 +1,616 @@ +// SHARK-3629 — the management endpoint serves the DATA tools, by default. +// +// WHY. People integrate Ankr to read chains. Until this ticket the advertised +// OAuth endpoint served 75 account-administration tools and NOT ONE chain read: +// asked for an address balance, an agent connected to /mcp had no tool for it. +// The data tools existed, on a second server, behind a raw key in a header, and +// reaching them cost the user a second MCP entry, a credential pasted by hand +// and a client restart. Measured end to end on 2026-08-07; the restart is what +// finally blocked it. +// +// WHAT IS PINNED HERE: +// 1. A session that asks for `data` gets the chain tools. +// 2. The DEFAULT connection carries them, without asking. That is the whole +// product point of the ticket and it is why the default set is asserted +// rather than the group alone. +// 3. The key is resolved SERVER-SIDE from slot 0, the account's Default key. +// Nothing is pasted, and no credential appears in the conversation. +// 4. The key can be changed inside a LIVE session: after mgmt_select_key the +// very next data call goes out with the other key, on the same connection, +// with no re-initialize and no re-authentication. +// 4b. And the resolved key follows the ACCOUNT, not just the slot number. +// mgmt_select_account moves a session between the accounts a login holds a +// seat on, and a slot means a different key on each, so a resolution cached +// by slot alone would bill the previous account's key for reads the session +// reports as the new account's. +// 5. /rpc is untouched: createServer still binds one key given to it. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { createMgmtServer } from "../src/mgmt/server.js"; +import { DATA_TOOL_CONTRACTS, createServer } from "../src/server.js"; +import type { GatewayClient } from "../src/mgmt/gateway/client.js"; +import { + type MgmtDeps, + createConfirmationStore, +} from "../src/mgmt/tools/confirmation.js"; +import { createAccountScope } from "../src/mgmt/gateway/groupScope.js"; +import { createDataKeySession } from "../src/mgmt/data/keySession.js"; +import { DEFAULT_TOOLSETS } from "../src/mgmt/toolsets.js"; + +const KEY_0 = { + index: 0, + jwt_data: "DEFAULT.JWT.VALUE", + is_encrypted: false, + name: "Default", + description: "", + config: "", +}; + +const KEY_4 = { + index: 4, + jwt_data: "OTHER.JWT.VALUE", + is_encrypted: false, + name: "agent-key", + description: "", + config: "", +}; + +const TOKEN_0 = "defaulttokenforslot0"; +const TOKEN_4 = "othertokenforslot4"; + +const ISSUER = "http://localhost:3100"; + +/** A representative sample of the data surface, not the whole list. */ +const DATA_TOOLS = ["rpcCall", "getBalances", "getBlock", "getLogs"]; + +function deps(): MgmtDeps { + return { + confirmations: createConfirmationStore(ISSUER), + sub: "test-subject", + issuerUrl: ISSUER, + mfaEnforced: true, + worker: { + importJwtToken: (jwtData: string) => + Promise.resolve({ + token: jwtData === KEY_0.jwt_data ? TOKEN_0 : TOKEN_4, + }), + }, + } as unknown as MgmtDeps; +} + +function gatewayWithKeys(): GatewayClient { + return { + accountScope: createAccountScope(), + listJwtTokens: () => Promise.resolve([KEY_0, KEY_4]), + getUserProfile: () => + Promise.resolve({ + address: "0xabc0000000000000000000000000000000000001", + }), + } as unknown as GatewayClient; +} + +/** Records every outbound URL and answers a valid JSON-RPC result. */ +function stubFetch(): { urls: string[]; restore: () => void } { + const urls: string[] = []; + const original = globalThis.fetch; + globalThis.fetch = (async (input: unknown) => { + urls.push(String(input)); + return { + ok: true, + status: 200, + headers: new Headers({ "content-type": "application/json" }), + json: async () => ({ jsonrpc: "2.0", id: 1, result: "0x1" }), + text: async () => '{"jsonrpc":"2.0","id":1,"result":"0x1"}', + }; + }) as unknown as typeof fetch; + return { urls, restore: () => (globalThis.fetch = original) }; +} + +/** A worker whose token names the material it came from, so a URL identifies a key. */ +const tokenPerJwt = (): Parameters[0]["worker"] => + ({ + importJwtToken: (jwt: string) => + Promise.resolve({ token: `token-for-${jwt}` }), + }) as unknown as Parameters[0]["worker"]; + +/** Run `fn` with fetch recorded, and hand back the URLs it reached for. */ +async function recordFetch(fn: () => Promise): Promise { + const stub = stubFetch(); + try { + await fn(); + } finally { + stub.restore(); + } + return stub.urls; +} + +async function connectMgmt(toolsets?: ReadonlySet): Promise { + const server = createMgmtServer(gatewayWithKeys(), deps(), toolsets as never); + const [clientT, serverT] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "0" }); + await server.connect(serverT); + await client.connect(clientT); + return client; +} + +async function toolNames(client: Client): Promise { + const { tools } = await client.listTools(); + return tools.map((t) => t.name); +} + +// --------------------------------------------------------------------------- +// 1 and 2: the data tools are there, and they are there BY DEFAULT +// --------------------------------------------------------------------------- + +test("SHARK-3629: a session that loads the data group gets the chain tools", async () => { + const client = await connectMgmt(new Set(["core", "data"])); + try { + const names = await toolNames(client); + for (const tool of DATA_TOOLS) { + assert.ok(names.includes(tool), `${tool} is missing from the data group`); + } + } finally { + await client.close(); + } +}); + +test("SHARK-3629: the DEFAULT connection carries the data tools without being asked", () => { + // The product claim of the ticket, pinned on the constant the HTTP entry point + // uses when a connection names no toolsets at all. + assert.ok( + DEFAULT_TOOLSETS.has("data"), + "a default connection must be able to read a chain" + ); + assert.ok(DEFAULT_TOOLSETS.has("core")); +}); + +// --------------------------------------------------------------------------- +// 3: the key comes from slot 0, resolved server-side +// --------------------------------------------------------------------------- + +test("SHARK-3629: a data call goes out with the key from slot 0, and no credential is asked for", async () => { + const fetchStub = stubFetch(); + const client = await connectMgmt(new Set(["core", "data"])); + try { + const r = await client.callTool({ + name: "rpcCall", + arguments: { chain: "eth", method: "eth_blockNumber", params: [] }, + }); + assert.notEqual((r as { isError?: boolean }).isError, true); + const used = fetchStub.urls.filter((u) => u.includes(TOKEN_0)); + assert.ok( + used.length > 0, + `no request carried slot 0's token; saw ${JSON.stringify(fetchStub.urls)}` + ); + } finally { + fetchStub.restore(); + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 4: switching the key inside a live session +// --------------------------------------------------------------------------- + +test("SHARK-3629: mgmt_select_key changes which key the NEXT data call uses, on the same connection", async () => { + const fetchStub = stubFetch(); + const client = await connectMgmt(new Set(["core", "data"])); + try { + await client.callTool({ + name: "rpcCall", + arguments: { chain: "eth", method: "eth_blockNumber", params: [] }, + }); + const before = fetchStub.urls.length; + + const sel = await client.callTool({ + name: "mgmt_select_key", + arguments: { index: 4 }, + }); + assert.notEqual((sel as { isError?: boolean }).isError, true); + + await client.callTool({ + name: "rpcCall", + arguments: { chain: "eth", method: "eth_blockNumber", params: [] }, + }); + + const after = fetchStub.urls.slice(before); + assert.ok( + after.some((u) => u.includes(TOKEN_4)), + `the call after the switch did not use slot 4's token; saw ${JSON.stringify(after)}` + ); + assert.ok( + !after.some((u) => u.includes(TOKEN_0)), + "the old key was still in use after the switch" + ); + } finally { + fetchStub.restore(); + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 4b: the resolved key follows the ACCOUNT, not only the slot +// --------------------------------------------------------------------------- + +// The defect this pins, found in review before it shipped: the resolution was +// cached by slot alone, and `mgmt_select_account` can move a session between the +// accounts a login holds a seat on. Slot 4 of one team is a different key from +// slot 4 of another, and slot 0 — the default nobody selects, resolved on the +// first data call — would have outlived every switch. The session would report +// one account through mgmt_whoami and through the account line every wrapped +// tool prints, while the chain reads went out on, and were billed to, another. +// +// Driven at the key session rather than through the tool surface on purpose: the +// thing under test is which token a resolution yields after the scope moves, and +// a stubbed McpServer round trip would only add ways for the assertion to pass +// for the wrong reason. +test("SHARK-3629: moving the session to another account re-resolves the key, it does not reuse the old one", async () => { + const TEAM_A = "0xaaa0000000000000000000000000000000000001"; + const TEAM_B = "0xbbb0000000000000000000000000000000000002"; + // The SAME slot number on both accounts, holding different material. This is + // what makes caching by slot alone wrong rather than merely imprecise. + // + // Slot 4 rather than slot 0: on a TEAM account slot 0 takes its own gateway + // route (findTeamAccountKey), and the point here is the ordinary project slot + // every account has several of. Both routes read through the same + // account-scoped gateway, so the cache key is what decides either way. + const A_SLOT_4 = { ...KEY_4, jwt_data: "TEAM.A.SLOT4" }; + const B_SLOT_4 = { ...KEY_4, jwt_data: "TEAM.B.SLOT4" }; + + const scope = createAccountScope(); + const gateway = { + accountScope: scope, + listJwtTokens: () => + Promise.resolve([scope.current() === TEAM_B ? B_SLOT_4 : A_SLOT_4]), + } as unknown as GatewayClient; + + scope.select({ address: TEAM_A }); + const keys = createDataKeySession({ gateway, worker: tokenPerJwt() }); + + // Point the session at slot 4 while it is on team A, and spend it once so the + // resolution is genuinely CACHED rather than merely computed. + assert.equal((await keys.select(4)).ok, true); + const onA = await recordFetch(() => + keys.torpc.call("eth", "eth_blockNumber") + ); + assert.ok( + onA.some((u) => u.includes("token-for-TEAM.A.SLOT4")), + onA.join() + ); + + // The console's account switch, mid-session. NOTHING re-selects afterwards: + // the read below goes through the same cached path a real session uses, which + // is the path the cache key has to get right. Asserting this after another + // select would prove nothing, because a select always re-reads and would mask + // a cache keyed on the wrong thing. + scope.select({ address: TEAM_B }); + const onB = await recordFetch(() => + keys.torpc.call("eth", "eth_blockNumber") + ); + + assert.ok( + onB.some((u) => u.includes("token-for-TEAM.B.SLOT4")), + `the read did not use team B's key; saw ${JSON.stringify(onB)}` + ); + assert.ok( + !onB.some((u) => u.includes("token-for-TEAM.A.SLOT4")), + "a chain read was billed to the account the session had LEFT" + ); +}); + +// --------------------------------------------------------------------------- +// 4c: the cache itself, and the three ways it has to behave +// --------------------------------------------------------------------------- + +// It has to BE a cache. Without this, "resolve every time" passes every other +// test in this file while paying a gateway read and a worker exchange on every +// single chain call — the thing the cache exists to prevent, invisible in green. +test("SHARK-3629: repeated data calls on one slot resolve the key once", async () => { + let reads = 0; + let exchanges = 0; + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: () => { + reads += 1; + return Promise.resolve([KEY_0, KEY_4]); + }, + } as unknown as GatewayClient; + const keys = createDataKeySession({ + gateway, + worker: { + importJwtToken: (jwt: string) => { + exchanges += 1; + return Promise.resolve({ token: `token-for-${jwt}` }); + }, + } as unknown as Parameters[0]["worker"], + }); + + await recordFetch(async () => { + await keys.torpc.call("eth", "eth_blockNumber"); + await keys.torpc.call("eth", "eth_blockNumber"); + await keys.torpc.call("eth", "eth_chainId"); + }); + + assert.equal(reads, 1, "the key listing was read more than once"); + assert.equal(exchanges, 1, "the worker exchange ran more than once"); +}); + +// And it must not cache a FAILURE. A slot that could not be resolved because the +// gateway was briefly unreachable would otherwise be broken for the life of the +// session, with no way back short of reconnecting — which is the cost this whole +// ticket exists to remove. +test("SHARK-3629: a resolution that failed is retried, not remembered as broken", async () => { + let attempt = 0; + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: () => { + attempt += 1; + return attempt === 1 + ? Promise.reject(new Error("gateway unreachable")) + : Promise.resolve([KEY_0, KEY_4]); + }, + } as unknown as GatewayClient; + const keys = createDataKeySession({ gateway, worker: tokenPerJwt() }); + + await assert.rejects(() => keys.torpc.call("eth", "eth_blockNumber")); + + const used = await recordFetch(() => + keys.torpc.call("eth", "eth_blockNumber") + ); + assert.ok( + used.some((u) => u.includes("token-for-DEFAULT.JWT.VALUE")), + `the retry did not go out; saw ${JSON.stringify(used)}` + ); + assert.equal(attempt, 2, "the second call did not re-read the key listing"); +}); + +// A slot that does not resolve leaves the session on the key it was using. +// +// The alternative is worse than an error: moving `index` first and failing +// second would point the data tools at a slot that cannot produce a key, so the +// NEXT chain read fails too, on a session that was working a moment ago. +test("SHARK-3629: selecting a slot that cannot resolve refuses, and changes nothing", async () => { + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: () => Promise.resolve([KEY_0, KEY_4]), + } as unknown as GatewayClient; + const keys = createDataKeySession({ gateway, worker: tokenPerJwt() }); + + assert.equal((await keys.select(4)).ok, true); + assert.equal(keys.currentIndex(), 4); + + const refused = await keys.select(9); + assert.equal(refused.ok, false, "an empty slot must not be selectable"); + assert.match( + refused.ok ? "" : refused.text, + /9/, + "the refusal must name the slot that failed" + ); + assert.equal( + keys.currentIndex(), + 4, + "a failed switch moved the session off the key it was working with" + ); + + const used = await recordFetch(() => + keys.torpc.call("eth", "eth_blockNumber") + ); + assert.ok( + used.some((u) => u.includes("token-for-OTHER.JWT.VALUE")), + `the reads after a failed switch changed key; saw ${JSON.stringify(used)}` + ); +}); + +// BOTH deferred clients follow the selection, not just the one the tests above +// happen to drive. +// +// Everything else here goes through `torpc`, so the `provider` accessor — the +// one line that hands the AAPI client to eight registered tools — was exercised +// by nothing. A mutation run found it: replacing that accessor with one that +// yields undefined survived the entire suite, which is the shape of a defect +// that would take out getAccountBalance, getNFTs, getTokenHolders and five more +// in production while every gate stayed green. +// +// The assertion is on the SEAM rather than on a response, and it reaches no +// network: a real provider method would go out over axios to rpc.ankr.com, and +// this suite must not depend on a host being up. Calling a name the client does +// NOT define drives the identical path — resolve the key, look the name up on +// the resolved client — and stops at the lookup. It therefore also pins the +// deferred client's other documented behaviour, that a non-method name yields +// its value rather than a call. +test("SHARK-3629: the provider clients follow the selection too, not only torpc", async () => { + let reads = 0; + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: () => { + reads += 1; + return Promise.resolve([KEY_0, KEY_4]); + }, + } as unknown as GatewayClient; + const keys = createDataKeySession({ gateway, worker: tokenPerJwt() }); + + assert.notEqual(keys.provider, undefined, "there is no provider to hand out"); + assert.equal(reads, 0, "nothing should resolve before a call is made"); + + const notAMethod = await ( + keys.provider as unknown as { noSuchMember: () => Promise } + ).noSuchMember(); + + assert.equal( + reads, + 1, + "touching the provider did not resolve the account's key, so the AAPI " + + "tools are not following the session's selection" + ); + assert.equal( + notAMethod, + undefined, + "a name the client does not define must come back as its value, not as a " + + "call into something that is not there" + ); +}); + +// mgmt_select_key is the one place that names the key back to a human, so it +// re-reads instead of trusting the cache. +// +// A session can delete the key in a slot and create another in the same slot +// without leaving, and nothing tells the cache. Reporting "Done: … now use API +// key " is precisely the class of +// untruth SHARK-3619/3622 spent this branch removing from the key-lifecycle +// writes, so it does not get reintroduced by a cache one layer down. +test("SHARK-3629: selecting a slot re-reads it, so the key it names is the one the account has now", async () => { + let material = "FIRST.JWT"; + let name = "old-key"; + const gateway = { + accountScope: createAccountScope(), + listJwtTokens: () => + Promise.resolve([{ ...KEY_4, jwt_data: material, name }]), + } as unknown as GatewayClient; + const keys = createDataKeySession({ + gateway, + worker: { + importJwtToken: (jwt: string) => + Promise.resolve({ token: `token-for-${jwt}` }), + } as unknown as Parameters[0]["worker"], + }); + + const first = await keys.select(4); + assert.equal(first.ok, true, JSON.stringify(first)); + assert.match(first.ok ? first.label : "", /old-key/); + + // The slot is rebuilt underneath the session: same index, different key. + material = "SECOND.JWT"; + name = "new-key"; + + const second = await keys.select(4); + assert.equal(second.ok, true, JSON.stringify(second)); + assert.match( + second.ok ? second.label : "", + /new-key/, + "mgmt_select_key named a key the account no longer has in that slot" + ); + + // And the reads that follow use the new material, not the cached token. + const used: string[] = []; + const original = globalThis.fetch; + globalThis.fetch = (async (input: unknown) => { + used.push(String(input)); + return { + ok: true, + status: 200, + headers: new Headers({ "content-type": "application/json" }), + json: () => Promise.resolve({ jsonrpc: "2.0", id: 1, result: "0x1" }), + text: () => Promise.resolve('{"jsonrpc":"2.0","id":1,"result":"0x1"}'), + }; + }) as unknown as typeof fetch; + try { + await keys.torpc.call("eth", "eth_blockNumber", []); + } finally { + globalThis.fetch = original; + } + assert.ok( + used.some((u) => u.includes("token-for-SECOND.JWT")), + `the read did not use the re-resolved key; saw ${JSON.stringify(used)}` + ); +}); + +// The deferred clients are objects, not promises, and the distinction is not +// cosmetic. They are Proxies whose `get` answers with a function, so without an +// explicit `then` of undefined they would be THENABLE: `await` or +// `Promise.resolve` on one calls `then(resolve, reject)`, the handler resolves +// the real client, finds no `then` there, returns undefined and never calls +// either callback. The await hangs forever, with no error and nothing in a log. +// +// Asserted with a real `await` under a timeout rather than by inspecting the +// property, because "does not hang" is the property that matters and reading +// `.then` would pass on a proxy that hangs anyway. +test("SHARK-3629: the deferred data clients are not thenable, so awaiting one cannot hang", async () => { + const keys = createDataKeySession({ + gateway: gatewayWithKeys(), + worker: { + importJwtToken: () => Promise.resolve({ token: TOKEN_0 }), + } as unknown as Parameters[0]["worker"], + }); + assert.equal((keys.torpc as { then?: unknown }).then, undefined); + assert.equal((keys.provider as { then?: unknown }).then, undefined); + + const hung = Symbol("hung"); + const raced = await Promise.race([ + Promise.resolve(keys.torpc).then(() => "settled"), + new Promise((resolve) => setTimeout(() => resolve(hung), 250)), + ]); + assert.equal(raced, "settled", "awaiting the deferred client hung"); +}); + +// The contracts that make a chain answer readable travel with the tools. +// +// SHARK-3599 lifted this prose OUT of the 16 tool descriptions on the argument +// that the session instructions carry it. That argument is only true on an +// endpoint whose instructions do, and when the tools first landed here they did +// not. The concrete miss, and the reason this is a test rather than a note: the +// RAW BASE UNITS rule ("args.value 41695680 on a 6-decimal token is 41.69568, +// not 41 million") exists in no tool description at all, so an agent on /mcp +// decoding a transfer would report an amount wrong by a factor of 10^decimals +// and nothing in the response would contradict it. +// +// Asserted against the SHARED constant, not against a copy of the sentences: a +// second transcription here would agree with itself while both drifted from what +// /rpc serves. +test("SHARK-3629: the management endpoint delivers the chain tools' own contracts", async () => { + const server = createMgmtServer(gatewayWithKeys(), deps(), DEFAULT_TOOLSETS); + const [clientT, serverT] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "0" }); + await server.connect(serverT); + await client.connect(clientT); + try { + const instructions = client.getInstructions() ?? ""; + assert.ok( + instructions.includes(DATA_TOOL_CONTRACTS), + "the data plane's contracts are not delivered on the endpoint that now " + + "serves its tools" + ); + // The two facts an agent cannot recover from a response on its own, named + // so a future edit that guts the shared block still fails here. + assert.match(instructions, /RAW BASE UNITS/); + assert.match(instructions, /tier_degraded/); + // And contract 1 is NOT carried over: this endpoint does not bind one key + // for the session's life, and saying so would send a user to open a new + // session for something mgmt_select_key does in place. + assert.doesNotMatch(instructions, /THE BOUND KEY/); + assert.match(instructions, /THE CHAIN TOOLS' KEY/); + } finally { + await client.close(); + } +}); + +// --------------------------------------------------------------------------- +// 5: /rpc is untouched +// --------------------------------------------------------------------------- + +test("SHARK-3629: the raw-key data server still binds the one key it is given", async () => { + const fetchStub = stubFetch(); + const server = createServer("rawkeygivenatconnect"); + const [clientT, serverT] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "0" }); + await server.connect(serverT); + await client.connect(clientT); + try { + await client.callTool({ + name: "rpcCall", + arguments: { chain: "eth", method: "eth_blockNumber", params: [] }, + }); + assert.ok( + fetchStub.urls.some((u) => u.includes("rawkeygivenatconnect")), + "the raw-key plane must keep using the key it was constructed with" + ); + // And it does not grow a key-switching tool: that belongs to the session + // that has an account behind it. + const names = (await client.listTools()).tools.map((t) => t.name); + assert.ok(!names.includes("mgmt_select_key")); + } finally { + fetchStub.restore(); + await client.close(); + } +}); diff --git a/test/mgmt-load-toolset.test.ts b/test/mgmt-load-toolset.test.ts index b06f394..fbada35 100644 --- a/test/mgmt-load-toolset.test.ts +++ b/test/mgmt-load-toolset.test.ts @@ -450,6 +450,18 @@ test("SHARK-3609: the session selection grows only through load(), and load() is assert.deepEqual(session.names(), ["core", "keys"]); }); +// Every caller today hands in a resolution, and every resolution already +// contains `core`, so the `loaded.add("core")` that guarantees it is never +// observed to do anything — a mutation run confirmed it: replacing the added +// name with an empty string survives the whole suite. The guarantee is real and +// this file's header states it, so it is asserted on the one input that can see +// it rather than left to the resolver's good behaviour. +test("SHARK-3609: a session has core however it was built, not only when asked for it", () => { + const session = createSessionToolsets(["keys"]); + assert.equal(session.has("core"), true, "core was dropped at construction"); + assert.deepEqual(session.names(), ["core", "keys"]); +}); + test("SHARK-3609: a resolved selection is unchanged by the session built from it", () => { const resolution = resolveToolsets("keys"); assert.equal(resolution.ok, true); @@ -518,13 +530,16 @@ test("SHARK-3609: over the real app, a load widens the SAME session with no new assert.ok(shimToken, "the harness login must mint a shim token"); const cred: Credential = { kind: "oauth", shimToken }; - // A default connection: no ?toolsets at all. + // A default connection: no ?toolsets at all. Since SHARK-3629 that is `core` + // plus `data`, which is what makes this the right starting point for the + // ticket's claim — it is the surface a real client actually lands on, not a + // selection a test asked for. const { status, sid } = await initSession(world, cred, null); assert.equal(status, 200); assert.ok(sid, "initialize must mint a session id"); assert.deepEqual( await listToolsOverHttp(world, cred, sid), - [...CORE_TOOLS].sort() + expectedFor("data") ); const loaded = await callTool(world, cred, sid, "mgmt_load_toolset", { @@ -535,10 +550,11 @@ test("SHARK-3609: over the real app, a load widens the SAME session with no new assert.match(loaded.text, /no reconnection or re-authentication is needed/); // The SAME session id, the SAME bearer, no second initialize — and the - // tools are there. + // tools are there, ON TOP of what the session already had rather than + // instead of it. assert.deepEqual( await listToolsOverHttp(world, cred, sid), - expectedFor("keys"), + expectedFor("data", "keys"), "the tools must appear on the session that was already open" ); } finally { diff --git a/test/mgmt-role-capabilities.test.ts b/test/mgmt-role-capabilities.test.ts index 8f87391..f115ba1 100644 --- a/test/mgmt-role-capabilities.test.ts +++ b/test/mgmt-role-capabilities.test.ts @@ -28,6 +28,7 @@ import assert from "node:assert/strict"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { createMgmtServer } from "../src/mgmt/server.js"; +import { isDataToolName } from "../src/server.js"; import { type GatewayClient, GatewayError, @@ -576,7 +577,20 @@ test("SHARK-3553: every registered tool is either mapped to a capability or expl const client = await connect(gateway); try { const { tools } = await client.listTools(); - const registered = tools.map((t) => t.name).sort(); + // SHARK-3629 — MANAGEMENT tools only. The `data` group puts the chain-read + // plane on this same server, and a role is the wrong instrument for it: the + // capability map transcribes the console's per-account permissions, and a + // chain read is not an account action at all. Whether the session may read a + // chain is decided by the key it holds, one layer down, not by a seat. + // Filtered by src/server.ts's DATA_TOOL_NAMES, which + // test/data-tool-surface.test.ts holds equal to the live data surface. + const managed = tools.filter((t) => !isDataToolName(t.name)); + assert.ok( + tools.length > managed.length, + "the data plane is no longer on this server, so this filter now hides " + + "nothing and the partition below is narrower than it reads" + ); + const registered = managed.map((t) => t.name).sort(); const mapped = Object.keys(TOOL_CAPABILITY); const free = [...CAPABILITY_FREE_TOOLS]; const overlap = mapped.filter((t) => CAPABILITY_FREE_TOOLS.has(t)); @@ -596,7 +610,7 @@ test("SHARK-3553: every registered tool is either mapped to a capability or expl // `expectAccount`, so its presence is an exact proxy for "this tool is // wrapped". Without this, a mapped tool moved onto the raw server would be // silently ungated while every name-level assertion above still passed. - for (const tool of tools) { + for (const tool of managed) { if (!Object.prototype.hasOwnProperty.call(TOOL_CAPABILITY, tool.name)) { continue; } diff --git a/test/mgmt-toolsets.test.ts b/test/mgmt-toolsets.test.ts index 0f53271..6860285 100644 --- a/test/mgmt-toolsets.test.ts +++ b/test/mgmt-toolsets.test.ts @@ -10,9 +10,13 @@ // // WHAT THESE TESTS HOLD, and why each one is here rather than being obvious: // -// 1. The DEFAULT is `core` and it fits a stated budget. A saving nobody -// measured is a saving nobody made, so the budget is asserted, not -// described, and the measured number is printed next to it. +// 1. The DEFAULT fits a stated budget. A saving nobody measured is a saving +// nobody made, so the budget is asserted, not described, and the measured +// number is printed next to it. SHARK-3629 moved that default from `core` +// to `core` plus `data` — an endpoint that could not read a chain was the +// wrong thing to serve a connection that asked for nothing — and moved the +// budget with it. The property being held is unchanged: a connection that +// names nothing pays a bounded, measured entry cost, not the whole surface. // 2. `all` is EXACTLY the pinned surface. The parameter may only ever // SUBTRACT: if a future tool could join `all` without the pinned list // moving, the whole security argument below is unenforced. @@ -39,6 +43,8 @@ import { createToolsetInventory } from "../src/mgmt/tools/index.js"; import { ALL_TOOLSETS, CORE_ONLY, + DEFAULT_TOOLSETS, + listPhrase, MAX_TOOLSETS_PARAM_LENGTH, TOOLSET_NAMES, type ToolsetName, @@ -69,7 +75,19 @@ import { // The entry cost a default session may pay, in o200k_base tokens over the // serialized `tools/list` tool array. Measured the same way the 27,260-token // baseline was, so the two numbers are comparable. -const CORE_TOKEN_BUDGET = 2400; +// +// SHARK-3629 moved the default from `core` to `core` plus `data`, so the budget +// moved with it: the ceiling is a statement about what a connection that asks +// for nothing costs, and that connection now carries the chain reads. It is +// still far under the 27,260 the whole surface costs, which is the saving +// SHARK-3600 exists for; what changed is which tools the saving keeps. +// Measured at 8,823 over 27 tools on this tree. The ceiling is deliberately +// close to the measurement, the way the 2,400 that stood against core's 2,388 +// was: a budget with room in it is a budget that notices nothing. +const DEFAULT_TOKEN_BUDGET = 8900; + +/** Exactly what a connection carrying no `?toolsets=` must serve. */ +const DEFAULT_TOOLS = expectedFor("data"); const tokensOf = (tools: unknown): number => encode(JSON.stringify(tools)).length; @@ -210,7 +228,7 @@ const withWorld = async ( // 1. The default, and its budget // --------------------------------------------------------------------------- -test("SHARK-3600: no ?toolsets on the URL registers exactly core, inside the token budget", async () => { +test("SHARK-3600: no ?toolsets on the URL registers exactly the default, inside the token budget", async () => { await withWorld(async (world, cred) => { // `null` = no query string at all, which is what a client that has never // heard of the parameter sends. @@ -219,19 +237,22 @@ test("SHARK-3600: no ?toolsets on the URL registers exactly core, inside the tok const tools = await listToolsOverHttp(world, cred, sid); assert.deepEqual( tools.map((t) => t.name).sort(), - CORE_TOOLS.slice().sort(), - "a session that asked for nothing must get core, no more and no less" + DEFAULT_TOOLS, + "a session that asked for nothing must get the default set, no more and " + + "no less" ); const measured = tokensOf(tools); console.log( `[SHARK-3600] default tools/list: ${String(tools.length)} tools, ` + - `${String(measured)} o200k tokens (budget ${String(CORE_TOKEN_BUDGET)})` + `${String(measured)} o200k tokens (budget ${String( + DEFAULT_TOKEN_BUDGET + )})` ); assert.ok( - measured <= CORE_TOKEN_BUDGET, - `core tools/list is ${String(measured)} o200k tokens, over the ` + - `${String(CORE_TOKEN_BUDGET)} budget` + measured <= DEFAULT_TOKEN_BUDGET, + `the default tools/list is ${String(measured)} o200k tokens, over the ` + + `${String(DEFAULT_TOKEN_BUDGET)} budget` ); }); }); @@ -526,16 +547,13 @@ test("SHARK-3600: ?toolsets on a follow-up POST cannot widen a live session", as const widened = await listToolsOverHttp(world, cred, sid, "toolsets=all"); assert.deepEqual( widened.map((t) => t.name).sort(), - CORE_TOOLS.slice().sort(), + DEFAULT_TOOLS, "the selection is fixed at initialize; a later URL must not move it" ); // And an unusable value on a follow-up is not an error either: the // parameter is simply not read after initialize. const still = await listToolsOverHttp(world, cred, sid, "toolsets=wallets"); - assert.deepEqual( - still.map((t) => t.name).sort(), - CORE_TOOLS.slice().sort() - ); + assert.deepEqual(still.map((t) => t.name).sort(), DEFAULT_TOOLS); }); }); @@ -680,8 +698,33 @@ const bad = (raw: unknown): string => { return r.ok ? "" : r.message; }; -test("SHARK-3600 resolver: an absent parameter is core, and only core", () => { - assert.deepEqual([...ok(undefined)].sort(), ["core"]); +test("SHARK-3600 resolver: an absent parameter is the default set, and only that", () => { + // SHARK-3629 moved the default from `core` alone to `core` plus `data`: an + // endpoint that answered account questions and could not read a single chain + // was the wrong thing to hand someone who connected without asking for + // anything. What the assertion holds is unchanged — absent resolves to a + // FIXED set, not to whatever happens to be registered — so it is pinned + // against the constant the HTTP entry point uses rather than against a + // literal, and the literal is asserted once, next to it. + assert.deepEqual([...ok(undefined)].sort(), [...DEFAULT_TOOLSETS].sort()); + assert.deepEqual([...DEFAULT_TOOLSETS].sort(), ["core", "data"]); +}); + +// The refusal's last sentence is built by this, and DEFAULT_TOOLSETS exercises +// only the two-name case — where `slice(0, -1)` and `slice(0, 1)` are the same +// thing. A mutation run confirmed the gap: swapping one for the other survived +// the whole suite. Three names is the shortest input that can tell them apart, +// and it is the input this function will actually see the day a third group +// joins the default. +test("SHARK-3629: a set is named to a human at every length", () => { + assert.equal(listPhrase([]), ""); + assert.equal(listPhrase(["core"]), "core"); + assert.equal(listPhrase(["core", "data"]), "core and data"); + assert.equal(listPhrase(["core", "data", "keys"]), "core, data and keys"); + assert.equal( + listPhrase(["core", "data", "keys", "usage"]), + "core, data, keys and usage" + ); }); test("SHARK-3600 resolver: `all` expands to every named set", () => { @@ -768,9 +811,10 @@ test("SHARK-3600 resolver: nothing the caller sent comes back in the refusal", ( // wrong URL, so every clause of it is pinned rather than sampled: which way the // value was unusable, the complete list of names, and how to get the default. const VALID_TAIL = - "Valid values are core, keys, usage, billing, notifications, team, identity " + - "and all, comma-separated, lower-case. The core set is always registered and " + - "cannot be dropped. Omit the parameter entirely to get core."; + "Valid values are core, data, keys, usage, billing, notifications, team, " + + "identity and all, comma-separated, lower-case. The core set is always " + + "registered and cannot be dropped. Omit the parameter entirely to get core " + + "and data."; test("SHARK-3600 resolver: every refusal names the reason AND the whole valid set", () => { const cases: [unknown, string][] = [ @@ -849,21 +893,33 @@ test("SHARK-3600 resolver: a selection cannot be widened through the set forEach // selection and bound to the raw underlying Set handed that raw Set, with a // working `.add`, to any caller who asked for it. // - // It lands on a SINGLETON. resolveToolsets returns CORE_ONLY itself for the - // no-parameter default, so one such call would not widen one session: it would - // widen the default for every LATER session in the process, and `keys` there - // means mgmt_reveal_api_key, mgmt_create_platform_api_key and + // It lands on a SINGLETON. resolveToolsets returns DEFAULT_TOOLSETS itself for + // the no-parameter case, so one such call would not widen one session: it + // would widen the default for every LATER session in the process, and `keys` + // there means mgmt_reveal_api_key, mgmt_create_platform_api_key and // mgmt_delete_api_key on connections that asked for nothing. - const core = ok(undefined); + const byDefault = ok(undefined); assert.throws( () => - core.forEach((_value, _value2, handedOver) => { + byDefault.forEach((_value, _value2, handedOver) => { (handedOver as Set).add("billing"); }), /cannot be changed/, "forEach must hand the callback something that cannot be added to" ); - assert.deepEqual([...ok(undefined)].sort(), ["core"], "the default widened"); + assert.deepEqual( + [...ok(undefined)].sort(), + ["core", "data"], + "the default widened" + ); + // Every exported singleton, not only the one this call went through: they are + // separate objects and a hole in the freezing would show on whichever one the + // widening route happened to touch. + assert.deepEqual( + [...DEFAULT_TOOLSETS].sort(), + ["core", "data"], + "the default singleton itself widened" + ); assert.deepEqual([...CORE_ONLY], ["core"], "the singleton itself widened"); assert.deepEqual([...ALL_TOOLSETS].sort(), [...TOOLSET_NAMES].sort()); @@ -1099,14 +1155,22 @@ test("SHARK-3524: the catalogue's estimate stays inside the band its comment sta pct > 0, `${name}: the estimate must not UNDERSTATE the real cost (${pct.toFixed(1)}%)` ); - // And within the band the comments now state. Measured 5.6-10.7% across the - // eight selections; 15% is the ceiling those comments promise, so a change - // that pushes past it has to update the prose too. + // And within the band the comments now state. 15% is the ceiling those + // comments promise, so a change that pushes past it has to update the prose + // too. The band is re-measured whenever the surface moves — SHARK-3629's + // `data` group is the ninth selection — and the run prints it above. assert.ok( pct < 15, `${name}: the estimate is ${pct.toFixed(1)}% high, past the 15% ceiling ` + `the comments in listToolsets.ts and tools/index.ts state` ); } - assert.equal(overstatement.length, 8, "every selection must be measured"); + // One row per selection a caller can ask for: every named set, plus `all`. + // Derived rather than written out, so adding a group cannot leave a selection + // silently unmeasured while the count still reads as deliberate. + assert.equal( + overstatement.length, + TOOLSET_NAMES.length + 1, + "every selection must be measured" + ); }); From 1961d4f05d68f4392d6ab17a89ef1147848b0aa6 Mon Sep 17 00:00:00 2001 From: Mike Date: Sat, 8 Aug 2026 00:54:01 +0300 Subject: [PATCH 3/5] perf(SHARK-3635): load the tokenizer when chain tools appear, not when the module does SHARK-3629 made the management server import registerDataTools, which reaches torpc/tokens.ts, which imported gpt-tokenizer at the top level. So every management process paid 65 MB and 386 ms at boot for a tokenizer it might never reach: `import src/mgmt/server.ts` went RSS 79 -> 189 MB and took 1.04 s, against a 512Mi pod and a 5 s HEALTHCHECK, and a `?toolsets=core` session -- which has no chain tool in it at all -- paid the same as one serving the whole data plane. The load now happens on first use, and the warm-up moved to registerDataTools. That is the honest place for it: it is the function that puts chain tools on a server, so it is exactly the event after which a token count becomes reachable. Measured per process, one scenario each: import src/mgmt/server.ts 118 MB (was 177) not loaded ?toolsets=core session 104 MB (was 176) not loaded default core+data session 176 MB loaded /rpc createServer 170 MB loaded, as before A cold process is not at the 82 MB management-only baseline, and that is the change's boundary rather than a shortfall: the AAPI client and the sixteen tool modules are still statically imported. The tokenizer is the single largest piece and the only one a chain-free session provably never needs. createRequire RATHER THAN `await import()`. countTokensDetailed is called synchronously from every tool's response path, so making the deferral async would ripple through tokenMeta and all fourteen call sites for no behavioural gain. In an ESM package createRequire is how a synchronous deferral is spelled. There is deliberately NO fallback: if the module cannot be loaded this throws rather than quietly reverting to chars/4, which is the 40-60% understatement SHARK-3525 removed and would be worse arriving silently. AND THIS IS WHY mgmt_list_toolsets KEEPS ITS chars/4 ESTIMATE. The two halves are one decision. mgmt_list_toolsets is in `core`, i.e. on every session including the narrowest, so counting for real would pull those 65 MB straight back into exactly the connections this relieved -- undoing the change through the one tool that reports the numbers. Two comments claimed the binary "carries no tokenizer on purpose", which SHARK-3629 had falsified and d860d75 corrected to say so; they now state the posture that is actually true again. Pinned in test/tokenizer-lazy.test.ts, one CHILD PROCESS per scenario. A module registry is per process and write-once, so in-process the answer to "was it loaded?" would depend on test order -- the shape of a test that passes for the wrong reason. Both directions are asserted: the cold cases prove the deferral, and the warm ones prove it is a deferral and not a removal. Verified by hand mutation: deleting the warmTokenizer() call fails both warm tests. Also closes a vacuous assertion the mutation run exposed in the pre-existing >256 KB path. The extrapolation test asserted `meta.token_count === d.tokens`, which compares the computation with itself, so replacing `(tokens / counted) * text.length` with `/ text.length` or `tokens * counted` survived the whole suite. A uniform payload of twice the limit must extrapolate to about twice the count of one at the limit, which is a reference the function did not produce. Gates: typecheck, lint, format, 1668 tests, coverage (global 90/80/85 and mgmt-scoped 80/75/80), build, mutation (tokens.ts 82.35% before the added test; remaining survivors are the Math.min cost bound, which only a timing assertion could kill, and two equivalents). Co-Authored-By: Claude Opus 5 (1M context) --- DEPLOY-MGMT.md | 19 +++ src/mgmt/tools/index.ts | 32 ++--- src/mgmt/tools/listToolsets.ts | 35 ++--- src/server.ts | 13 ++ src/torpc/tokens.ts | 60 ++++++++- test/fixtures/tokenizer-load-child.ts | 74 +++++++++++ test/tokenizer-lazy.test.ts | 185 ++++++++++++++++++++++++++ test/tokens.test.ts | 30 +++++ 8 files changed, 414 insertions(+), 34 deletions(-) create mode 100644 test/fixtures/tokenizer-load-child.ts create mode 100644 test/tokenizer-lazy.test.ts diff --git a/DEPLOY-MGMT.md b/DEPLOY-MGMT.md index 54b17fd..0867b96 100644 --- a/DEPLOY-MGMT.md +++ b/DEPLOY-MGMT.md @@ -144,6 +144,25 @@ o200k tokens, 2039 → 2262 when SHARK-3609 measured it. The budget the test asserts is on the DEFAULT selection rather than on `core`, and since SHARK-3629 that is `core` plus `data`: 8,823 measured against an 8,900 ceiling. +**Process memory, and what a chain-free session costs (SHARK-3635).** SHARK-3629 +made this binary import the data plane, which brought `gpt-tokenizer` with it: +65 MB and 386 ms, paid at boot by every pod whether or not a session ever read a +chain. The tokenizer now loads on FIRST USE and is warmed by `registerDataTools`, +so it arrives with the chain tools instead of with the import statement. +Measured per process, one scenario each: + +| Process | RSS | tokenizer | +| ------------------------------------ | ---------------- | ----------------- | +| `import src/mgmt/server.ts` | 118 MB (was 177) | not loaded | +| `?toolsets=core` session | 104 MB (was 176) | not loaded | +| default (`core` plus `data`) session | 176 MB | loaded | +| `/rpc` `createServer` | 170 MB | loaded, as before | + +This is why `mgmt_list_toolsets` still reports a chars/4 estimate rather than a +real count: it is in `core`, so counting for real would pull those 65 MB back +into exactly the sessions this relieved. `test/tokenizer-lazy.test.ts` holds all +four rows, one child process per row. + Any session can call `mgmt_list_toolsets` (it is in `core`) for each group's tool count, approximate token cost and reconnect URL; the same catalogue is one line of the server instructions. diff --git a/src/mgmt/tools/index.ts b/src/mgmt/tools/index.ts index 6c12a4e..886767f 100644 --- a/src/mgmt/tools/index.ts +++ b/src/mgmt/tools/index.ts @@ -29,18 +29,21 @@ import { registerBundles } from "./bundles.js"; import { registerPinAccount, withAccountScope } from "./accountScope.js"; import { scopeOf } from "../gateway/groupScope.js"; // SHARK-3629. A STATIC import, and the cost is stated rather than left to be -// discovered: it pulls the data plane, and through it gpt-tokenizer, into the -// management binary at boot. Measured on this tree, importing src/mgmt/server.ts -// moves RSS 79 -> 189 MB and takes 1.04 s, the tokenizer being 65 MB and 386 ms -// of that, against the 512Mi pod and 5 s HEALTHCHECK in DEPLOY-MGMT.md. +// discovered: it pulls the data plane's modules into the management binary at +// boot, whether or not a session ever reads a chain. // -// It is static because the alternative is not free either. `data` is in the +// It stays static because the alternative is not free either. `data` is in the // DEFAULT selection, so nearly every session loads it anyway, and the group // thunks run inside registerAsOneChange, whose batching of // notifications/tools/list_changed depends on a strictly SYNCHRONOUS window that -// an `await import()` would break. Deferring the cost properly means making the -// tokenizer itself lazy in torpc/tokens.ts, which is a change to the data -// plane's hot path and wants its own measurement. Flagged, not smuggled. +// an `await import()` would break. +// +// SHARK-3635 took the expensive part out of it instead. gpt-tokenizer was 65 MB +// and 386 ms of a 79 -> 189 MB, 1.04 s import, and it is now loaded on first use +// and warmed by registerDataTools — so it arrives with the chain tools rather +// than with this import statement. Measured after: a `?toolsets=core` session is +// 104 MB, down from 176 MB. What remains here is the AAPI client and the sixteen +// tool modules, which are the surface itself. import { registerDataTools } from "../../server.js"; import { createDataKeySession } from "../data/keySession.js"; import { registerSelectKey } from "./selectKey.js"; @@ -476,13 +479,12 @@ const measureToolsets = async ( tools: tools.length, // chars/4. NOT what `_meta.token_count` uses — that is a real o200k_base // count (src/torpc/tokens.ts) and this comment claimed otherwise until - // SHARK-3524's review round. It then claimed the management binary carries - // no tokenizer, which SHARK-3629 falsified by importing the data plane - // here: the tokenizer is in this process at boot either way. So this stays - // an estimate by choice rather than by constraint, measured 2.6-10.9% HIGH - // across the nine selections and gated at 15% in - // test/mgmt-toolsets.test.ts. See listToolsets.ts for the full note and - // the two decisions it leaves open. + // SHARK-3524's review round. It stays an estimate because this tool is in + // `core`, so counting for real would pull the tokenizer into every + // session including the ones with no chain tool in them — which is the + // 65 MB SHARK-3635 just took off them. Measured 2.6-10.9% HIGH across the + // nine selections and gated at 15% in test/mgmt-toolsets.test.ts. See + // listToolsets.ts for the full note. tokens: Math.ceil(JSON.stringify(tools).length / 4), }); } diff --git a/src/mgmt/tools/listToolsets.ts b/src/mgmt/tools/listToolsets.ts index 08d4d52..65a56e8 100644 --- a/src/mgmt/tools/listToolsets.ts +++ b/src/mgmt/tools/listToolsets.ts @@ -29,23 +29,26 @@ // that SHARK-3525 removed chars/4 from the data plane precisely because it // UNDERSTATES real usage. chars/4 survives in exactly one place in src/: here. // -// WHY IT SURVIVED, AND WHY THAT REASON EXPIRED IN SHARK-3629. The argument was -// that the management binary carries no tokenizer and importing one costs RSS -// 42 -> 111 MB steady against a 512Mi pod, which is a lot for one advisory -// number in one tool. That is no longer the situation: tools/index.ts imports -// registerDataTools from src/server.ts, which reaches torpc/tokens.ts, so the -// tokenizer is loaded at boot whether or not a session ever asks for a chain. -// Measured on this tree: importing src/mgmt/server.ts moves RSS 79 -> 189 MB and -// takes 1.04 s, of which the tokenizer alone is 82 -> 147 MB and 386 ms. +// WHY IT SURVIVES. A management session that never reads a chain carries no +// tokenizer, and loading one costs RSS 82 -> 147 MB and 386 ms — a lot for one +// advisory number in one tool. // -// So chars/4 is now a CHOICE rather than a constraint, and it is left as it is -// pending a decision rather than changed on the way past. Two things follow, and -// both are open: whether this tool should simply report the real count now that -// the tokenizer is in the process anyway (which would retire the band test -// below), and whether the data plane's import belongs behind the `data` thunk so -// a core-only session stops paying ~107 MB and ~1 s for tools it never lists. -// Neither is decided here; what is fixed here is the comment, which asserted a -// property of the binary that its own imports contradict. +// THAT SENTENCE WAS BRIEFLY FALSE, and the repair is the reason to state the +// history rather than just the rule. SHARK-3629 made tools/index.ts import +// registerDataTools from src/server.ts, which reaches torpc/tokens.ts, and that +// import was static — so the tokenizer landed in every management process at +// boot and the justification above described a property the binary no longer +// had. SHARK-3635 made the load happen on first use, warmed by registerDataTools +// rather than by module evaluation, which puts the cost on sessions that serve +// chain reads and nowhere else. Measured after: a `?toolsets=core` session is +// 104 MB against 176 MB before. +// +// So this tool must NOT switch to a real count, and the reason is now sharper +// than "it would cost memory". mgmt_list_toolsets is in `core`, i.e. on every +// session including the narrowest — the very sessions SHARK-3635 exists to +// spare. Counting for real here would load 65 MB for an advisory number on +// exactly the connections that were just relieved of it, and undo the ticket +// through the one tool that reports the numbers. // // MEASURED on this tree, chars/4 against o200k_base for all nine selections: // between 2.6% and 10.9% HIGH (core 9.9%, data 2.6%, keys 6.8%, usage 9.3%, diff --git a/src/server.ts b/src/server.ts index 932abe3..3644711 100644 --- a/src/server.ts +++ b/src/server.ts @@ -2,6 +2,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { buildVersion } from "./buildInfo.js"; import { buildProvider } from "./provider.js"; import { buildTorpcClient } from "./torpc/client.js"; +import { warmTokenizer } from "./torpc/tokens.js"; import { registerGetAccountBalance } from "./tools/getAccountBalance.js"; import { registerGetTokenPrice } from "./tools/getTokenPrice.js"; import { registerGetTransaction } from "./tools/getTransaction.js"; @@ -214,6 +215,18 @@ export const registerDataTools = ({ provider: ReturnType; torpc: ReturnType; }) => { + // SHARK-3635 — pull the tokenizer in HERE, at the moment chain tools appear on + // a server, rather than at module import. + // + // This function is the exact event that makes a token count reachable, so it + // is where the 65 MB and 386 ms belong: /rpc pays it at construction, as it + // always did, a default management session pays it when its `data` group + // registers, and a `?toolsets=core` session — which has no tool here at all — + // pays nothing. Kept eager rather than left to the first tool call for the + // reason torpc/tokens.ts has always given: a one-time module load inside a + // request is latency a user did not ask for. + warmTokenizer(); + // Kept AAPI tools (unchanged behavior) registerGetAccountBalance({ server, provider }); registerGetTokenPrice({ server, provider }); diff --git a/src/torpc/tokens.ts b/src/torpc/tokens.ts index f3267bb..4340c08 100644 --- a/src/torpc/tokens.ts +++ b/src/torpc/tokens.ts @@ -38,9 +38,60 @@ // module import, RSS 42 -> 111 MB steady (146 MB peak while encoding a 700 KB // payload), and ~32-45 ms per MB of text. Against a 200-660 ms upstream RPC call // that is under 3% added latency, and it fits the pod's 512Mi limit with room to -// spare. Imported EAGERLY (below) so the 99 ms lands at server start rather than -// inside the first tool call. -import { encode } from "gpt-tokenizer/model/gpt-4o"; +// spare. +// +// SHARK-3635 — LOADED ON FIRST USE, WARMED BY WHOEVER SERVES CHAIN READS. +// +// It used to be a plain top-level import, and the argument for that was sound +// while this module had one consumer: every /rpc session reads chains, so the +// one-time cost belongs at server start rather than inside the first tool call. +// SHARK-3629 gave it a second consumer with a different shape. The management +// server imports registerDataTools, so a STATIC import here put 65 MB and 386 ms +// into every management process at boot — including one serving `?toolsets=core`, +// which has no chain tool in it at all. Measured: src/mgmt/server.ts went 79 -> +// 189 MB and 1.04 s, against a 512Mi pod and a 5 s HEALTHCHECK. +// +// So the load moved to first use and the WARM-UP moved to registerDataTools, +// which is the honest place for it: it is the function that puts chain tools on +// a server, so it is exactly the event after which a token count becomes +// reachable. /rpc warms at construction as before, a default management session +// warms when its `data` group registers, and a core-only one never does. +// +// WHY createRequire AND NOT `await import()`. countTokensDetailed is called +// synchronously from every tool's response path. Making it async would ripple +// through tokenMeta and all fourteen call sites for no behavioural gain, so the +// deferral has to be synchronous, and in an ESM package createRequire is how +// that is spelled. +// +// THERE IS NO FALLBACK, deliberately. If the module cannot be loaded this throws +// rather than quietly reverting to chars/4 — which is the 40-60% understatement +// SHARK-3525 removed, and a silent return to it would be worse than a loud +// failure. +import { createRequire } from "node:module"; + +type Encode = (text: string) => number[]; + +let encoder: Encode | undefined; + +const loadEncoder = (): Encode => { + encoder ??= ( + createRequire(import.meta.url)("gpt-tokenizer/model/gpt-4o") as { + encode: Encode; + } + ).encode; + return encoder; +}; + +/** + * Load the tokenizer NOW, so the one-time cost lands where it can be afforded. + * + * Called by registerDataTools (src/server.ts). Idempotent and cheap after the + * first call, so a process that registers the chain tools more than once pays + * once. + */ +export const warmTokenizer = (): void => { + loadEncoder(); +}; // Name of the encoding the reported token_count is measured in. Exposed so // _meta can say what the number means: it is an o200k_base count (the same @@ -102,6 +153,9 @@ export const countTokensDetailed = ( text: string ): { tokens: number; exact: boolean } => { if (text.length === 0) return { tokens: 0, exact: true }; + // Resolved ONCE per call rather than per slice: a 256 KB payload is 64 slices, + // and paying a memoised lookup 64 times for one answer is pure waste. + const encode = loadEncoder(); const counted = Math.min(text.length, EXACT_COUNT_LIMIT); let tokens = 0; for (let i = 0; i < counted; i += COUNT_CHUNK) { diff --git a/test/fixtures/tokenizer-load-child.ts b/test/fixtures/tokenizer-load-child.ts new file mode 100644 index 0000000..475fed9 --- /dev/null +++ b/test/fixtures/tokenizer-load-child.ts @@ -0,0 +1,74 @@ +// Child process for the tokenizer-laziness tests in tokenizer-lazy.test.ts +// (SHARK-3635). +// +// Not a *.test.ts file on purpose: `pnpm test` globs `test/*.test.ts`, so this is +// only ever run by the parent test spawning it. +// +// WHY A CHILD PROCESS AT ALL. "Has gpt-tokenizer been loaded?" is a property of +// a MODULE REGISTRY, and a registry is per process and write-once: the first +// test in a file that touches the data plane loads it, and every later assertion +// in that file then reads a state some earlier test created. In-process the +// answer would depend on test ORDER, which is exactly the kind of test that +// passes for the wrong reason. One scenario per fresh process, and the answer is +// whatever that process alone did. +// +// WHY THE MODULE REGISTRY AND NOT RSS. RSS is the number the ticket quotes, +// because it is the number that matters against a 512Mi pod. It is also noisy — +// GC timing, the tsx transform, whatever the OS feels like — so a threshold on +// it would be flaky in one direction and blind in the other. Whether the module +// is in `require.cache` is exact, and it is the CAUSE of the RSS the ticket +// measured, so pinning it pins the thing that produces the number. +import { createRequire } from "node:module"; + +const SCENARIO = process.env.TOKENIZER_SCENARIO ?? ""; + +const loaded = (): boolean => + Object.keys(createRequire(import.meta.url).cache).some((p) => + p.includes("gpt-tokenizer") + ); + +// Read BEFORE anything is imported, so a scenario can prove the baseline is +// clean rather than assuming it. +const before = loaded(); + +switch (SCENARIO) { + case "mgmt-import": { + // Importing the management server must not drag the tokenizer in. + await import("../../src/mgmt/server.js"); + break; + } + case "mgmt-core": { + const { createMgmtServer } = await import("../../src/mgmt/server.js"); + const { CORE_ONLY } = await import("../../src/mgmt/toolsets.js"); + createMgmtServer({} as never, undefined, CORE_ONLY); + break; + } + case "mgmt-default": { + const { createMgmtServer } = await import("../../src/mgmt/server.js"); + const { DEFAULT_TOOLSETS } = await import("../../src/mgmt/toolsets.js"); + createMgmtServer({} as never, undefined, DEFAULT_TOOLSETS); + break; + } + case "data-server": { + const { createServer } = await import("../../src/server.js"); + createServer("dummy-key-not-used"); + break; + } + case "count-tokens": { + // The number itself must be unaffected by where the module came from. + const { tokenMeta, toolText } = await import("../../src/torpc/tokens.js"); + const text = toolText({ hello: "world", n: 41695680 }); + process.stdout.write(`META ${JSON.stringify(tokenMeta(text))}\n`); + break; + } + default: + throw new Error(`unknown TOKENIZER_SCENARIO: ${SCENARIO}`); +} + +process.stdout.write( + `RESULT ${JSON.stringify({ + before, + after: loaded(), + rssMb: Math.round(process.memoryUsage().rss / 1024 / 1024), + })}\n` +); diff --git a/test/tokenizer-lazy.test.ts b/test/tokenizer-lazy.test.ts new file mode 100644 index 0000000..8dfc044 --- /dev/null +++ b/test/tokenizer-lazy.test.ts @@ -0,0 +1,185 @@ +// SHARK-3635 — the tokenizer is loaded by whoever needs it, not by whoever +// happens to be in the same process. +// +// WHAT WENT WRONG. SHARK-3629 put the chain-read tools on the management +// endpoint, which meant src/mgmt/tools/index.ts importing registerDataTools from +// src/server.ts. That import is static, so it pulled the whole data plane — and +// through torpc/tokens.ts, gpt-tokenizer — into the management process at boot, +// for every session including one that never reads a chain. Measured: importing +// src/mgmt/server.ts moved RSS 79 -> 189 MB and took 1.04 s, the tokenizer alone +// being 65 MB and 386 ms of it, against a 512Mi pod and a 5 s HEALTHCHECK. +// +// It also falsified the reason mgmt_list_toolsets reports a chars/4 ESTIMATE +// rather than a real count: "the management binary carries no tokenizer on +// purpose". Deferring the load is what makes that sentence true again, which is +// why the two are one decision and not two. +// +// WHAT IS PINNED HERE, and each one is a separate process: +// +// 1. Importing the management server does not load it. +// 2. Building a `?toolsets=core` session does not load it. This is the case +// the whole change exists for: an account-administration session pays +// nothing for a tokenizer it will never reach. +// 3. Building the DEFAULT session (core plus data) DOES load it — proving the +// deferral is a deferral and not a removal, and that the cost lands at +// registration rather than inside the first tool call. +// 4. The raw-key data plane loads it at construction, exactly as before. Its +// own header argues for eager loading and that argument is still right +// there: every /rpc session reads chains. +// 5. The count itself is unchanged, and exact. +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import path from "node:path"; + +const HERE = import.meta.dirname; +const REPO = path.join(HERE, ".."); +const FIXTURE = path.join(HERE, "fixtures", "tokenizer-load-child.ts"); + +type Verdict = { before: boolean; after: boolean; rssMb: number }; + +// Measured on this tree, one process per scenario, after the change: +// +// mgmt-import 118 MB not loaded (was 177 MB, loaded) +// mgmt-core 104 MB not loaded (was 176 MB, loaded) +// mgmt-default 176 MB loaded +// data-server 170 MB loaded +// +// The primary assertion is the module registry, which is exact. RSS is asserted +// too, on the COLD cases only, because it is the number the ticket is about and +// because a registry probe alone would be satisfied by a load that arrived by +// some other route. The ceiling is set well clear of both sides: 40 MB of head +// room over the highest cold reading and 36 MB below the lowest warm one. +// +// A cold process is NOT at the 82 MB management-only baseline, and that is +// expected rather than a shortfall: the rest of the data plane (the AAPI client, +// the sixteen tool modules) is still statically imported. This change is about +// the tokenizer, which is the single largest piece and the only one a +// chain-free session provably never needs. +const COLD_MAX_RSS_MB = 140; + +// `node --import tsx` rather than the `tsx` bin, for the reason +// data-http-hotpath.test.ts records: the bin runs the script in a grandchild and +// its stdout does not reach us. +const run = async ( + scenario: string +): Promise<{ verdict: Verdict; out: string }> => { + const child = spawn(process.execPath, ["--import", "tsx", FIXTURE], { + cwd: REPO, + env: { ...process.env, TOKENIZER_SCENARIO: scenario, NODE_ENV: "test" }, + stdio: ["ignore", "pipe", "pipe"], + }); + let out = ""; + let err = ""; + child.stdout?.on("data", (c: Buffer) => (out += c.toString())); + child.stderr?.on("data", (c: Buffer) => (err += c.toString())); + const code = await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + child.kill("SIGKILL"); + reject(new Error(`${scenario}: child never exited; stderr: ${err}`)); + }, 30_000); + child.once("exit", (c) => { + clearTimeout(timer); + resolve(c ?? -1); + }); + }); + assert.equal(code, 0, `${scenario} exited ${String(code)}; stderr: ${err}`); + const m = /RESULT (.+)/.exec(out); + assert.ok(m?.[1], `${scenario} printed no verdict; stdout: ${out}`); + return { verdict: JSON.parse(m[1]) as Verdict, out }; +}; + +test("SHARK-3635: importing the management server does not load the tokenizer", async () => { + const { verdict } = await run("mgmt-import"); + assert.equal( + verdict.before, + false, + "the module was resident before we began" + ); + assert.equal( + verdict.after, + false, + `importing src/mgmt/server.ts loaded gpt-tokenizer (RSS ${String( + verdict.rssMb + )} MB)` + ); + assert.ok( + verdict.rssMb <= COLD_MAX_RSS_MB, + `importing src/mgmt/server.ts cost ${String(verdict.rssMb)} MB, over the ` + + `${String(COLD_MAX_RSS_MB)} MB ceiling a tokenizer-free import has` + ); +}); + +test("SHARK-3635: a core-only management session never loads the tokenizer", async () => { + const { verdict } = await run("mgmt-core"); + assert.equal( + verdict.after, + false, + `a ?toolsets=core session loaded gpt-tokenizer (RSS ${String( + verdict.rssMb + )} MB), so it paid ~65 MB for a tool surface with no chain reads in it` + ); + assert.ok( + verdict.rssMb <= COLD_MAX_RSS_MB, + `a core-only session cost ${String(verdict.rssMb)} MB, over the ` + + `${String(COLD_MAX_RSS_MB)} MB ceiling` + ); +}); + +// The other direction, and it is what stops the two tests above from being +// satisfied by simply deleting the tokenizer. A deferral that never fires is +// indistinguishable from a removal until a chain read returns a wrong number. +test("SHARK-3635: the DEFAULT session does load it, at registration rather than on first use", async () => { + const { verdict } = await run("mgmt-default"); + assert.equal( + verdict.after, + true, + "the default session registers the chain tools, so their tokenizer must be " + + "resident before the first tool call rather than inside it" + ); +}); + +test("SHARK-3635: the raw-key data plane still loads it at construction", async () => { + const { verdict } = await run("data-server"); + assert.equal( + verdict.after, + true, + "createServer must keep warming the tokenizer: every /rpc session reads " + + "chains, so the 386 ms belongs at construction and not in a request" + ); +}); + +// And the number is the same number. A lazy load that silently fell back to an +// estimate would pass every assertion above while reintroducing the 40-60% +// understatement SHARK-3525 exists to remove, so the value and its exactness +// flag are both read back out of a process that loaded the module lazily. +test("SHARK-3635: a lazily loaded tokenizer produces the same exact count", async () => { + const { out } = await run("count-tokens"); + const m = /META (.+)/.exec(out); + assert.ok(m?.[1], `no token meta printed; stdout: ${out}`); + const meta = JSON.parse(m[1]) as { + token_count: number; + token_count_estimated?: boolean; + }; + assert.ok( + meta.token_count > 0, + `a real count is a positive number, got ${JSON.stringify(meta)}` + ); + // tokenMeta omits the flag entirely when the count is exact, so its ABSENCE is + // the assertion: a lazily loaded tokenizer that had fallen back to an estimate + // would have to say so here, and saying nothing is the exact case. + assert.equal( + meta.token_count_estimated, + undefined, + "a short payload must be counted exactly, never estimated" + ); + // The same string, counted here, in a process that has the tokenizer resident + // by whatever route this test file loaded it. Two routes, one answer. + const { tokenMeta, toolText } = await import("../src/torpc/tokens.js"); + const local = tokenMeta(toolText({ hello: "world", n: 41695680 })); + assert.deepEqual( + meta, + local, + "the lazily loaded tokenizer disagreed with this process's own count" + ); +}); diff --git a/test/tokens.test.ts b/test/tokens.test.ts index a639291..a070caa 100644 --- a/test/tokens.test.ts +++ b/test/tokens.test.ts @@ -157,3 +157,33 @@ test("the exactness boundary is where the limit actually is", () => { "one char past the limit" ); }); + +// SHARK-3635 (mutation round): the extrapolated NUMBER, not just its flag. +// +// The test above asserts `token_count_estimated` and then checks +// `meta.token_count === d.tokens` — which compares tokenMeta with +// countTokensDetailed, i.e. the same computation with itself. A mutation run +// showed what that misses: replacing `(tokens / counted) * text.length` with +// `tokens / counted / text.length` or `tokens * counted` survived the whole +// suite, because both sides of that equality move together. The scale factor is +// the entire content of the estimate, so it needs a reference the function did +// not produce. +// +// A uniform payload is that reference. "a" repeated tokenizes at a constant rate, +// so a string of exactly twice the limit must extrapolate to about twice the +// count of a string of exactly the limit — which is measurable here without +// re-implementing the estimator. +test("the extrapolated count scales with the payload, and is not some other arithmetic", () => { + const atLimit = countTokensDetailed("a".repeat(262_144)); + const twiceLimit = countTokensDetailed("a".repeat(524_288)); + + assert.equal(atLimit.exact, true); + assert.equal(twiceLimit.exact, false); + + const ratio = twiceLimit.tokens / atLimit.tokens; + assert.ok( + ratio > 1.9 && ratio < 2.1, + `doubling a uniform payload must roughly double the estimate, got ` + + `${atLimit.tokens} -> ${twiceLimit.tokens} (x${ratio.toFixed(3)})` + ); +}); From e7809bda307cfa37e69214a6f6ebbef5e8e735c8 Mon Sep 17 00:00:00 2001 From: Mike Date: Sat, 8 Aug 2026 09:39:00 +0300 Subject: [PATCH 4/5] docs(SHARK-3629): record that tool-call metrics do not cover the management plane instrumentToolCalls is applied only in createServer, so mcp_ankr_tool_calls_total and mcp_ankr_tool_call_duration_seconds carry nothing from /mcp. That was invisible while the planes served disjoint surfaces; SHARK-3629 put the sixteen chain reads on both, so the same tool name is now counted from /rpc and not from /mcp, and a per-tool rate read off those families understates real usage silently. Not fixed here on purpose: widening a metric's coverage changes what every existing dashboard and alert on those names means, which belongs to whoever owns them rather than to a merge. Co-Authored-By: Claude Opus 5 (1M context) --- DEPLOY-MGMT.md | 20 ++++++++++++++++++++ src/server.ts | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/DEPLOY-MGMT.md b/DEPLOY-MGMT.md index 475443e..17cfe4b 100644 --- a/DEPLOY-MGMT.md +++ b/DEPLOY-MGMT.md @@ -230,6 +230,26 @@ this section records only what is specific to the management plane. Nothing here logs a credential: the log field set is a closed allowlist, and the UAuth bearer, the shim JWT, a TOTP and a `confirmToken` are all outside it. +**KNOWN GAP: tool calls on this plane are not counted.** `instrumentToolCalls` +patches `registerTool` once, before the tools register, and it is applied only in +`createServer` (`src/server.ts`) — the raw-key data plane. The management server +never instruments, so `mcp_ankr_tool_calls_total` and +`mcp_ankr_tool_call_duration_seconds` carry nothing from `/mcp`. + +That was invisible while the two planes served disjoint surfaces. It stopped +being invisible in SHARK-3629, which put the sixteen chain reads on this plane +too: the same tool name is now counted when it is served from `/rpc` and not +counted when it is served from `/mcp`, so a per-tool rate read off these metrics +UNDERSTATES real usage by whatever share the management endpoint carries, and +does so silently. Read those two families as "data plane only" until this is +closed. + +Closing it is a one-line application of the same helper to the management +server's raw McpServer, before `registerMgmtTools` runs; it is left out of +SHARK-3629 because widening a metric's coverage changes what every existing +dashboard and alert on those names means, and that is a decision for whoever owns +them rather than a merge artefact. + ## Tools (PoC) **94 tools are registered** on the management server (`?toolsets=all`; 75 before diff --git a/src/server.ts b/src/server.ts index ec93c14..359d20a 100644 --- a/src/server.ts +++ b/src/server.ts @@ -194,8 +194,8 @@ export const createServer = (apiKey: string, metricsOverride?: Metrics) => { // SHARK-3629 note for whoever reads this next: the OTHER caller of // registerDataTools is the management plane, and it does not instrument its // server, so the chain tools it serves are not counted by - // mcp_tool_calls_total. That is a gap in the metric's coverage, not in this - // function, and it is recorded in DEPLOY-MGMT.md rather than fixed here. + // mcp_ankr_tool_calls_total. That is a gap in the metric's coverage, not in + // this function, and it is recorded in DEPLOY-MGMT.md rather than fixed here. instrumentToolCalls(server, metricsOverride ?? installedMetrics()); registerDataTools({ From b625c482409a5ecd15e7e0a30cccac6f62c729e7 Mon Sep 17 00:00:00 2001 From: Mike Date: Sat, 8 Aug 2026 09:47:03 +0300 Subject: [PATCH 5/5] fix(SHARK-3635): assert the module load, not the megabytes CI failed the tokenizer test at 149 MB against a 140 MB ceiling, for a cold process that is 107-118 MB here, while the module registry correctly reported the tokenizer absent. The threshold was measuring the runner, not the change. The file's own comment already said RSS is noisy and the registry probe is exact, and the ceiling was added anyway on the argument that RSS is the number the ticket is about. It is, and that is an argument for reporting it, not for gating on it: baseline RSS moves with the Node build, the GC and the transform cache, and at 149 cold against 176 warm the bands overlap across environments, so no portable ceiling separates them. Locally the same scenario read 118 MB and then 107 MB on consecutive runs. The registry assertion, which is exact and environment independent, is unchanged and is what proved the change works. Each scenario now prints its RSS instead, so the figure stays visible without the gate depending on the machine. Gates: typecheck, lint, format, 1742 tests, both coverage scripts including the one CI failed on. Co-Authored-By: Claude Opus 5 (1M context) --- DEPLOY-MGMT.md | 10 ++++++-- test/tokenizer-lazy.test.ts | 46 ++++++++++++++++++------------------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/DEPLOY-MGMT.md b/DEPLOY-MGMT.md index 17cfe4b..8b7a885 100644 --- a/DEPLOY-MGMT.md +++ b/DEPLOY-MGMT.md @@ -158,10 +158,16 @@ Measured per process, one scenario each: | default (`core` plus `data`) session | 176 MB | loaded | | `/rpc` `createServer` | 170 MB | loaded, as before | +RSS figures are one run on one machine and move with the Node build, the GC and +the transform cache: the same cold process read 118 MB locally and 149 MB on a CI +runner. Read the column as an order of magnitude, not a budget. +`test/tokenizer-lazy.test.ts` holds all four rows, one child process per row, and +asserts the MODULE LOAD rather than the megabytes, which is exact and the same +everywhere. + This is why `mgmt_list_toolsets` still reports a chars/4 estimate rather than a real count: it is in `core`, so counting for real would pull those 65 MB back -into exactly the sessions this relieved. `test/tokenizer-lazy.test.ts` holds all -four rows, one child process per row. +into exactly the sessions this relieved. Any session can call `mgmt_list_toolsets` (it is in `core`) for each group's tool count, approximate token cost and reconnect URL; the same catalogue is one line diff --git a/test/tokenizer-lazy.test.ts b/test/tokenizer-lazy.test.ts index 8dfc044..a2dddfe 100644 --- a/test/tokenizer-lazy.test.ts +++ b/test/tokenizer-lazy.test.ts @@ -38,25 +38,28 @@ const FIXTURE = path.join(HERE, "fixtures", "tokenizer-load-child.ts"); type Verdict = { before: boolean; after: boolean; rssMb: number }; -// Measured on this tree, one process per scenario, after the change: +// One run on this machine, one process per scenario, after the change (the +// figures move between runs, which is the point of the note below): // // mgmt-import 118 MB not loaded (was 177 MB, loaded) // mgmt-core 104 MB not loaded (was 176 MB, loaded) // mgmt-default 176 MB loaded // data-server 170 MB loaded // -// The primary assertion is the module registry, which is exact. RSS is asserted -// too, on the COLD cases only, because it is the number the ticket is about and -// because a registry probe alone would be satisfied by a load that arrived by -// some other route. The ceiling is set well clear of both sides: 40 MB of head -// room over the highest cold reading and 36 MB below the lowest warm one. +// RSS IS REPORTED AND NOT ASSERTED, and the first version of this file got that +// wrong. It carried a 140 MB ceiling on the cold cases on the argument that RSS +// is the number the ticket is about. CI then read 149 MB for the same cold +// process that is 118 MB here, while the module registry said, correctly, that +// the tokenizer was absent. The threshold was measuring the runner, not the +// change: baseline RSS moves with the Node build, the GC and the transform +// cache, and at 149 cold against 176 warm the two bands overlap across +// environments, so no portable ceiling separates them. // -// A cold process is NOT at the 82 MB management-only baseline, and that is -// expected rather than a shortfall: the rest of the data plane (the AAPI client, -// the sixteen tool modules) is still statically imported. This change is about -// the tokenizer, which is the single largest piece and the only one a -// chain-free session provably never needs. -const COLD_MAX_RSS_MB = 140; +// The registry probe has none of that. It is exact, it is environment +// independent, and it is the CAUSE of the RSS the ticket measured, so pinning it +// pins the thing that produces the number. The RSS each scenario reached is +// printed instead, which keeps the figure visible without making the gate depend +// on the machine it runs on. // `node --import tsx` rather than the `tsx` bin, for the reason // data-http-hotpath.test.ts records: the bin runs the script in a grandchild and @@ -86,7 +89,14 @@ const run = async ( assert.equal(code, 0, `${scenario} exited ${String(code)}; stderr: ${err}`); const m = /RESULT (.+)/.exec(out); assert.ok(m?.[1], `${scenario} printed no verdict; stdout: ${out}`); - return { verdict: JSON.parse(m[1]) as Verdict, out }; + const verdict = JSON.parse(m[1]) as Verdict; + // Printed, not asserted. The number is what SHARK-3635 is about, so a run + // should show it; see the note above for why it must not gate. + console.log( + `[SHARK-3635] ${scenario}: ${String(verdict.rssMb)} MB RSS, tokenizer ` + + `${verdict.after ? "loaded" : "not loaded"}` + ); + return { verdict, out }; }; test("SHARK-3635: importing the management server does not load the tokenizer", async () => { @@ -103,11 +113,6 @@ test("SHARK-3635: importing the management server does not load the tokenizer", verdict.rssMb )} MB)` ); - assert.ok( - verdict.rssMb <= COLD_MAX_RSS_MB, - `importing src/mgmt/server.ts cost ${String(verdict.rssMb)} MB, over the ` + - `${String(COLD_MAX_RSS_MB)} MB ceiling a tokenizer-free import has` - ); }); test("SHARK-3635: a core-only management session never loads the tokenizer", async () => { @@ -119,11 +124,6 @@ test("SHARK-3635: a core-only management session never loads the tokenizer", asy verdict.rssMb )} MB), so it paid ~65 MB for a tool surface with no chain reads in it` ); - assert.ok( - verdict.rssMb <= COLD_MAX_RSS_MB, - `a core-only session cost ${String(verdict.rssMb)} MB, over the ` + - `${String(COLD_MAX_RSS_MB)} MB ceiling` - ); }); // The other direction, and it is what stops the two tests above from being