Skip to content

refactor(helius-mcp): Thread the Request Context Through Tool Handlers - #185

Merged
0xIchigo merged 6 commits into
mainfrom
nathan(helius-mcp)/migrate-inline-call-sites
Oct 14, 2026
Merged

0xIchigo merged 6 commits into
mainfrom
nathan(helius-mcp)/migrate-inline-call-sites

Conversation

@nathanliow

@nathanliow nathanliow commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #182. Base is nathan(helius-mcp)/add-request-context; GitHub retargets this to main once that merges. Review #182 first — the diff shown here is only this change.

Converts every tool handler that resolves credentials to pass the per-request context #182 added. 72 handlers across 20 files; zk-compression.ts alone accounts for 23.

Behaviour is unchanged: contextFromExtra returns null under stdio and the resolvers fall back to module state exactly as before. This is the migrate step of expand → migrate → contract; the globals come out two PRs from now.

The plan for this PR was wrong twice, in ways worth knowing

It said extra was already in scope. It is passed — action-handlers.ts:172 has handed it to every handler as the second argument since the router landed — but no handler declared the parameter, so nothing could read it. Each site therefore takes two edits rather than a one-token substitution:

-    async ({ address, hash }) => {
-      if (!hasApiKey()) return noApiKeyResponse();
+    async ({ address, hash }, extra) => {
+      const ctx = contextFromExtra(extra);
+      if (!hasApiKey(ctx)) return noApiKeyResponse();

It deferred accounts.ts and transactions.ts as module-level helpers. All fourteen of their call sites are direct in-handler calls of exactly the shape above, so they are migrated here.

A bug this introduced, and the guard that now prevents it

Eight handlers registered with {} as their schema were first written async (extra) => .... Both the SDK and ActionHandlerCollector invoke handlers as handler(args, extra), so that single parameter bound to args and the context was silently dropped.

Nothing caught it. TypeScript accepts a 1-arity function where a 2-arity signature is expected, contextFromExtra takes unknown, and this suite mocks the resolvers — so a handler that never receives a context still produces correct output. Against the real dispatch path, getEnhancedWebSocketInfo returned the machine's own on-disk key while a caller key was supplied. getAllWebhooks is the sharpest case: hosted, it would have listed the deployment's webhooks rather than the caller's.

tests/router-action-handlers.test.ts now asserts that every handler binding a context declares at least two parameters. Arity is the only observable signal here, and the assertion catches the class rather than the eight instances. Negative-tested — reverting getAllWebhooks alone fails with getAllWebhooks (arity 1), naming the tool.

Two signature adjustments

Resolvers accept RequestContext | null. contextFromExtra returns null for "this request carries no identity", which is meaningful rather than absent. Widening beats making 72 call sites write ?? undefined.

restRequest takes the context second, ahead of RequestInit. With options last the single call site read restRequest(url, undefined, ctx). Nothing passes options today, and restRequest is not in the package's public surface.

Deliberately not migrated

setHeliusApiKey (config.ts) stays context-free, with a comment saying why. It validates the key it was just handed via setApiKey; with a context the client it reads back would carry the caller's transport key instead, so the tool would report success for an invalid key. It is a mutator and never fitted this substitution.

config.ts:9 resolves at registration time, outside any request.

utils/ows.ts:82 — the one genuine deferral. getOwsSolanaAddress reads the network from module state inside a module-level helper, so staking.ts and transfers.ts are half-applied: the Helius client follows the caller's network while the OWS signer follows the server's. A devnet caller on a mainnet-default server therefore derives the wrong CAIP-2 account key through resolveOwsOrKeypairSigner. That is the next PR.

Verification

tsc --noEmit clean, pnpm validate passes, 340 tests green, no async (extra) remains in src/.

Two assertions in router-action-handlers.test.ts were updated: they asserted an exact argument list against a signature that gained a parameter, and now assert the null a context-free call actually passes.

The suite cannot see a context failing to arrive, so this was checked through the real callActionHandler path, against the {}-schema shape the first check never exercised:

getEnhancedWebSocketInfo
  with authInfo : wss://atlas-devnet.helius-rpc.com/?api-key=key-AAAA-1111
  no authInfo   : wss://atlas-mainnet.helius-rpc.com/?api-key=deployment-key-ZZZZ
getLaserstreamInfo
  with authInfo : https://laserstream-devnet-ewr.helius-rpc.com
  no authInfo   : https://laserstream-mainnet-ewr.helius-rpc.com

The caller's key and network both flow, and the no-context fallback is byte-identical to before — the two halves of "per-caller isolation works, stdio is unchanged" in one frame.

Reviewing this efficiently

The diff is large but has only four shapes: a handler signature gaining , extra; a const ctx = contextFromExtra(extra); line; a resolver call gaining (ctx); and one import per file. Anything that does not match one of those four is worth a second look — the arity bug and the setHeliusApiKey revert were both exactly that.

🤖 Generated with Claude Code

Converts the call sites that resolve credentials inside a handler body to
pass the per-request context added in the previous change. 66 handlers across
18 files; `zk-compression.ts` alone accounts for 23.

Behaviour is unchanged. `contextFromExtra` returns null under stdio, and the
resolvers fall back to module state exactly as before — the two failing
assertions in `router-action-handlers.test.ts` were asserting an exact
argument list against a signature that gained a parameter, and now assert the
null that a context-free call actually passes.

The plan for this change said `extra` was already in scope and the edit was a
one-token substitution. It was not: `action-handlers.ts` has passed `extra` as
the second argument to every handler since the router landed, but no handler
declared it. So each site takes two edits — add the parameter, bind the
context once at the top of the body — which is why this diff adds lines rather
than only changing them.

Two signature adjustments fell out of the conversion:

- The resolvers now accept `RequestContext | null`, not just `undefined`.
  `contextFromExtra` returns null for "this request carries no identity", and
  widening the parameter beats making 66 call sites write `?? undefined`.
- `restRequest` takes the context as its second parameter, ahead of
  `RequestInit`. With options last, the single call site read
  `restRequest(url, undefined, ctx)`; nothing passes options today.

Left for the change that follows: `accounts.ts`, `transactions.ts` and
`ows.ts`, whose calls sit in module-level helpers that receive no `extra` and
need it threaded from their callers. `config.ts:10` resolves at registration
time, outside any request, and stays on module state.

Verified beyond the suite: two callers with different keys driven concurrently
through `callActionHandler` each get their own key back in the subscription URL
the tool returns.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nathanliow
nathanliow added this pull request to stack #186 September 24, 2026 15:02
…meter

Eight handlers registered with `{}` as their schema were written
`async (extra) => ...`. Both the SDK and `ActionHandlerCollector` invoke
handlers as `handler(args, extra)`, so that single parameter binds to *args*
and the context was silently discarded.

Nothing caught it. TypeScript accepts a 1-arity function where a 2-arity
signature is expected, and `contextFromExtra` takes `unknown`, so the wrong
value type-checks and returns null. The suite mocks the resolvers, so a
handler that never receives a context still produces correct output. The
end-to-end check I ran exercised `transactionSubscribe`, which has a real
schema and was correct; `getEnhancedWebSocketInfo`, 180 lines below it in the
same file, was not.

Demonstrated against the real dispatch path before the fix: passing
`key-AAAA-1111` as the caller's key, `getEnhancedWebSocketInfo` returned the
machine's own on-disk key in the subscription URL. `getAllWebhooks` is the
sharpest case — hosted, it would have listed the deployment's webhooks rather
than the caller's.

`tests/router-action-handlers.test.ts` now asserts that every handler binding
a context declares at least two parameters, which catches the whole class
rather than these eight instances. Arity is the only observable signal here,
since mocked resolvers hide everything else. Negative-tested: reverting
`getAllWebhooks` alone fails with `getAllWebhooks (arity 1)`.

Also from review:

- `setHeliusApiKey` is context-free again. It validates the key it was just
  handed by calling `setApiKey` and then reading a client back; with a context
  that client would carry the caller's transport key instead, so the tool would
  report success for an invalid key. Unreachable today, but it is a mutator and
  never fitted the substitution this change applies everywhere else.
- `accounts.ts` and `transactions.ts` are migrated here after all. The plan
  deferred them as module-level helpers; all fourteen call sites are direct
  in-handler calls of exactly the migrated shape. One had a multi-line
  parameter list my single-line pattern missed, which is the same blind spot
  that produced the arity bug.

That leaves `ows.ts` as the only genuine deferral: `getOwsSolanaAddress` reads
the network from module state inside a module-level helper. Until it is
threaded, a devnet caller on a mainnet-default server derives the wrong CAIP-2
account key through `resolveOwsOrKeypairSigner`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
},
async ({ recipientAddress, amount, sendMax, owsWallet }) => {
if (!hasApiKey()) return noApiKeyResponse();
async ({ recipientAddress, amount, sendMax, owsWallet }, extra) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The transaction is now submitted under the caller's key via getHeliusClient(ctx), but resolveOwsOrKeypairSigner still picks the signer based only on the HELIUS_MCP_SHARED_CREDENTIAL env flag. A per-request deployment that authenticates via authInfo without that flag, with a keypair on disk, signs every caller's send with the operator's wallet. The signer resolution needs to refuse the disk keypair whenever ctx is present, and not just when the env flag is set

'Lightweight pre-flight check: returns current plan, credit balance, and which MCP tools require an upgrade. 0 credits. Call before gated tools (transactionSubscribe, laserstreamSubscribe, etc.).',
{},
async () => {
async (_args, extra) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Further down on 215, hasApiKey(ctx) gates on the caller and then getJwt() reads the host's on-disk session and returns the operator's plan, credits, and project. The identity check and the data belong to different principals. The same shape is at auth.ts:340

We need to either thread a JWT through the context or refuse when ctx is present. Fixing the getAccountPlan label in #181 makes this consistent

// Resolve wallet address
let walletAddress = wallet;
if (!walletAddress) {
walletAddress = getSessionWalletAddress() ?? undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An omitted wallet still defaults to the host keypair's address even when a context is supplied. We should return missingParamError when ctx is present

// that renames the parameter drops out of the set and stops being
// checked. The size tripwire below catches wholesale breakage of this
// scan, not a single rename.
for (const block of source.split('server.tool(').slice(1)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Splitting on the literal server.tool( and matching a single-quoted name means a double-quoted name or a renamed extra silently drops out of threading. A registration-time handler.length >= 2 check in ActionHandlerCollector.tool does the same job without parsing source

const { readFileSync, readdirSync } = await import('node:fs');
const { join } = await import('node:path');

const toolsDir = new URL('../src/tools/', import.meta.url).pathname;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

new URL(...).pathname keeps percent-encoding. We should use fileURLToPath or a checkout under a path with a space fails with ENOENT

nathanliow and others added 3 commits October 1, 2026 12:45
…sent

Merges the updated base and addresses review. Four conflicts in `helius.ts`
resolved by keeping the new semantics from the base and re-applying the `| null`
widening this branch added.

Three findings share one shape: an identity check made against the caller,
answered with data belonging to the host.

**`getJwt` now refuses a context.** `hasApiKey(ctx)` gated on the caller, then
`getJwt()` read the dashboard session belonging to whoever ran `signup` on this
machine and returned the operator's plan, credits and projects. Fixed in
`getJwt` rather than at the two call sites named in review, because there are
nine; returning `undefined` keeps every one of them on the "no session" branch
it already handles.

**`getStakeAccounts` no longer defaults to the host wallet.** An omitted
`wallet` fell through to `getSessionWalletAddress()`, so a caller who supplied
no address got the operator's stake accounts. It now asks for the address.

The third — signer resolution gating only on `HELIUS_MCP_SHARED_CREDENTIAL`,
so a per-request deployment with a keypair on disk would sign every caller's
transaction with the operator's wallet — belongs on the branch that threads a
context into `ows.ts`, which this one does not. Carried there rather than
patched here against a parameter that does not exist yet.

**The arity guard moved into `ActionHandlerCollector`.** Scanning tool source
for `contextFromExtra(extra)` meant a renamed parameter or a double-quoted tool
name dropped out of coverage silently — the same failure mode as the bug it was
written to catch. Registration sees what the call site actually depends on.

Narrower than the review suggested, deliberately: `handler.length >= 2` for
everything would reject fourteen handlers that legitimately take no context,
like the docs and knowledge tools. The hazard is specifically an *empty schema*
with exactly one parameter — there `args` is always `{}`, so that parameter is
almost certainly meant to be `extra` and silently is not. Arity 0 is safe:
nothing binds, so nothing is shadowed.

The narrower check still caught `signup`, which the source scan had missed.

Also: `new URL(...).pathname` kept percent-encoding, so a checkout under a path
containing a space would have failed with ENOENT. Uses `fileURLToPath`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lers

A re-read of the combined stack found the `getJwt` guard incomplete. It refuses
only when a context is *passed*, and seven call sites still call it bare. Six
are module-level helpers or actions that need a JWT to run at all, but
`getStarted` had a context in scope and did not use it — checking the caller's
key on one line and reading the host's dashboard session on the next. That is
the exact mixed-principal shape the guard was written for, in a handler already
migrated.

`keypairExistsOnDisk()` beside it is the same thing: a request that carried its
own identity was told whether the operator has a keypair, and where.

Adds `tests/host-state-isolation.test.ts`, which drives real actions through
`callActionHandler` and asserts the host's session, keypair path and wallet
never reach an identified caller — plus the mirror case, that a local caller
still sees all three, so withholding stays conditional on identity rather than
becoming a blanket removal that breaks stdio.

Two things that made the first version of that file worthless, both caught by
reverting the fix and watching it still pass:

- It asserted against a machine with no JWT and no keypair, so there was
  nothing to leak. The host is now mocked as fully provisioned.
- It mocked `getJwt` itself, which replaced the guard under test. It mocks
  `loadConfig` instead, so the real `getJwt(ctx)` logic runs.

Reverting the fix now fails the suite by name.

Worth saying what this does not cover: nothing enforces hosted eligibility at
dispatch, so these actions remain reachable with a context. Until that lands,
these assertions are the boundary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s mean something

CI caught what I did not: the mirror case asserted a provisioned host while
running on a machine that has never been set up. It passed locally because this
machine has `~/.helius/config.json`, and failed on a runner that does not.

Fixing the environment was not enough. The mocks in that file never applied at
all: `config.ts` resolves its paths from `os.homedir()` at module load, and
`getJwt` calls its own module-local `loadConfig`, so mocking either export
leaves the real code path untouched. The suite was asserting against whatever
the machine happened to have.

It now writes a real config and keypair into a temp directory and points
`os.homedir` at it, so the actual loader reads an actual session. Three
consequences worth having:

- The assertions hold identically with `HOME` empty and `HELIUS_API_KEY` unset,
  which is the runner's situation.
- A first case asserts the fixture is provisioned — a local caller does see the
  session and keypair. Without it, everything below could pass because there was
  nothing to leak rather than because leaking was prevented.
- Reverting the guard in `getStarted` now fails by name under those same
  conditions, which was not true before.

That is twice in this file that a test passed for the wrong reason. Both times
the check that caught it was the same: revert the fix, confirm the test fails,
restore.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Base automatically changed from nathan(helius-mcp)/add-request-context to main October 14, 2026 21:38

@0xIchigo 0xIchigo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM :shipit:

@0xIchigo
0xIchigo merged commit 6470a43 into main Oct 14, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants