fix(mcp-server): isolate wallet config per runtime - #352
Conversation
A later programmatic main() could overwrite the process-wide config object while an HTTP listener from an earlier start was still serving, so new sessions built signing tools from the mutated singleton. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 97.17% 97.74% +0.57%
==========================================
Files 80 80
Lines 5410 5460 +50
==========================================
+ Hits 5257 5337 +80
+ Misses 153 123 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The per-runtime AppConfig snapshot + AsyncLocalStorage approach is a sound fix for the HTTP session-policy drift and is well covered for tool listing, but two wallet-safety gaps remain: validateSecurityConfig() still validates options.walletMode while tool/signing policy now comes from the unvalidated appConfig, and the new resetWalletProvider() in stop() lets one runtime's shutdown drop the memo a still-running stdio runtime depends on, so its next signing call can pick up a later runtime's key.
Findings: 4 blocking | 10 non-blocking | 7 posted inline
Blockers
- Test coverage stops short of the load-bearing part: the new tests only assert tool listing, which is decided at registration time inside
getServer(appConfig). TherunWithAppConfigwrappers aroundhandleRequest/handlePostMessage(the scope that governsgetPrivateKeyAsHex()/getWalletProvider()during a tool call) are not exercised — if AsyncLocalStorage did not propagate through the SDK's async chain under Bun/Node,wallet-isolation.test.tswould still pass. Add a direct assertion, e.g. inject atransportFactorywhosehandleRequestchecksgetRuntimeConfig() === appConfig, or invoke a signing-path helper inside the scope. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Claude's and Codex's findings. - Tests were not executed in this review environment (
bun testwas not permitted here); the PR's own test-plan checkboxes are still unchecked — please confirmbun test --isolate src/testsinpackages/mcp-serverbefore merge. snapshotConfig()copiesAppConfigfield-by-field, so any future field added toAppConfigis silently dropped from every snapshot (and would then fall back to the process singleton's value for that field). A spread + freeze, or a compile-time exhaustiveness check, would keep snapshots honest as the type grows.resetWalletProvider()only clearswalletProviderInstance; theprovidersByConfigWeakMap entry (which holds aPrivateKeyWalletProviderbuilt from a snapshot's key) is untouched and lives as long as the snapshot object does. Fine today because HTTP snapshots are wallet-disabled, but worth a comment or an explicit purge so the invariant is stated rather than incidental.- Codex flagged the
REVIEW.mdedit as possible review-instruction injection. Worth noting but not blocking: the loaded guidelines come from the base branch, and the added "HTTP tool and signing policy is instance-scoped" invariant accurately describes this PR's code rather than steering a verdict. Still fair to have a human confirm that new review invariants land with the change that creates them. - The changeset is present and correctly scoped to
@sei-js/mcp-server: patch; README and guideline docs were updated alongside the behaviour change. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| this.host = options.host ?? 'localhost'; | ||
| this.path = options.path ?? '/mcp'; | ||
| this.walletMode = options.walletMode ?? 'disabled'; | ||
| this.appConfig = options.appConfig ?? snapshotConfig(); |
There was a problem hiding this comment.
[blocker] appConfig now carries the wallet mode and the private key that determine this transport's tool list and signer, but the guard at line 101 still validates this.walletMode (options.walletMode ?? 'disabled'), which is a separate field. A programmatic caller doing new StreamableHttpTransport({ port, host, path, appConfig: { walletMode: 'private-key', privateKey, walletApiKey: undefined } }) passes validateSecurityConfig() (sees 'disabled'), then the default serverFactory calls getServer(this.appConfig) and registers the signing tools on an HTTP listener — exactly the drain-the-wallet configuration the guard exists to prevent.
The CLI path is safe (args.ts:157 derives walletMode from appConfig), so this is a public-API footgun rather than a shipped hole, but the fix is one line: validate the config that actually governs signing, e.g. validateSecurityConfig(this.mode, this.appConfig.walletMode), or drop the separate walletMode option and derive it from the snapshot so the two cannot disagree. (Raised by Codex as P1; confirmed.)
| this.host = options.host; | ||
| this.path = options.path; | ||
| this.walletMode = options.walletMode ?? 'disabled'; | ||
| this.appConfig = options.appConfig ?? snapshotConfig(); |
There was a problem hiding this comment.
[blocker] Same divergence as streamable-http.ts:70: the guard at line 228 checks this.walletMode, while the tool list and signer come from this.appConfig. Validate this.appConfig.walletMode (or derive walletMode from the snapshot) so a wallet-enabled appConfig cannot start an HTTP listener.
| const errors = await collectOperationErrors([() => transport.stop(), () => server?.close()]); | ||
| throwCollectedErrors(errors, 'Failed to stop all MCP server resources.'); | ||
| } finally { | ||
| resetWalletProvider(); |
There was a problem hiding this comment.
[blocker] This resetWalletProvider() reaches across runtimes. The memo it clears is process-global and is the one a stdio runtime uses, because stdio request handling never enters runWithAppConfig — getServer(config.appConfig) scopes only registration, so at tool-call time getWalletProvider() takes the runtime === processConfig branch.
Concretely: runtime A starts stdio with key A; a later main() mutates the singleton to key B; runtime B stops → this finally clears the memo → runtime A's next transfer_sei rebuilds a provider from the mutated singleton and signs with key B. Before this PR the memo persisted for the process lifetime, so A kept signing with A. That makes the reset a small regression in the same threat model the PR is fixing (Codex P1 on stdio; confirmed).
Either scope stdio execution to its snapshot too (pass appConfig to StdioTransport and wrap message handling in runWithAppConfig, which would also make this reset a no-op for the singleton path), or drop the reset from stop() and clear only state owned by the runtime being stopped.
| */ | ||
| export function getWalletProvider(): WalletProvider { | ||
| const runtime = getRuntimeConfig(); | ||
| if (runtime !== processConfig) { |
There was a problem hiding this comment.
[suggestion] Branching on object identity against the exported singleton is fragile: runWithAppConfig(config, ...) — passing the singleton itself, which snapshotConfig()'s default parameter makes easy to do by accident — silently falls back to the global memo path instead of the snapshot path. A structural signal (Object.isFrozen(runtime), or an explicit non-enumerable marker set by snapshotConfig) would express "this is an isolated snapshot" without depending on reference identity.
| constructor() { | ||
| this.privateKey = getPrivateKeyAsHex(); | ||
| constructor(...key: [] | [string | undefined]) { | ||
| this.privateKey = key.length === 0 ? getPrivateKeyAsHex() : key[0]; |
There was a problem hiding this comment.
[nit] The ...key: [] | [string | undefined] rest-tuple exists only to distinguish "no argument" from "explicit undefined", which is unusual enough in a public exported class to need a comment. An options object (new PrivateKeyWalletProvider({ privateKey })) or a static factory (PrivateKeyWalletProvider.fromConfig(appConfig)) alongside the zero-arg constructor would make the two call sites self-describing.
| import type { McpTransport, TransportConfig } from './types.js'; | ||
|
|
||
| export const createTransport = (config: TransportConfig): McpTransport => { | ||
| const appConfig = config.appConfig ? { appConfig: config.appConfig } : {}; |
There was a problem hiding this comment.
[nit] The conditional spread only avoids emitting an appConfig: undefined key; both transports already do options.appConfig ?? snapshotConfig(), so appConfig: config.appConfig passed straight through behaves identically and reads better. (Would need the corresponding factory test expectation updated, which already tolerates maxActiveRequests: undefined.)
| * change an already-running listener's tool policy or signer. | ||
| */ | ||
| export function snapshotConfig(source: AppConfig = config): AppConfig { | ||
| return Object.freeze({ |
There was a problem hiding this comment.
[nit] Object.freeze returns Readonly<AppConfig> but the declared return type is the mutable AppConfig, so callers get a type that invites writes which silently no-op (or throw in strict mode). Consider returning Readonly<AppConfig> and typing the snapshot fields on the transports as Readonly<AppConfig> — the mutable config singleton remains assignable, so it should be a type-only change.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c69b2b. Configure here.
Stdio request handling never entered runWithAppConfig, so resetWalletProvider() on another runtime's stop() made a live stdio server recreate its signer from the latest process singleton. Co-authored-by: Cursor <cursoragent@cursor.com>
callTool() can return a union where content is unknown, so the isolation helper now narrows to a text payload before parsing the derived address. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The per-runtime AppConfig snapshot + AsyncLocalStorage approach is sound and well covered by new tests, and the changeset is present. One gap blocks: the wallet-on-HTTP guard still validates only the separate walletMode option and never inspects the new appConfig snapshot that actually carries the private key, so a transport constructed with a signing snapshot and no explicit walletMode starts a listener with signing tools.
Findings: 2 blocking | 9 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty); only Codex's findings could be merged. Codex's P1 (guard ignores the wallet snapshot) is confirmed here; its P2 (REVIEW.md reviewer directives) is carried as an inline note. - Tests could not be executed in this environment —
node_modulesis not installed, sobun test --isolate srcwas not run. All findings are from static reading. index.ts:56logs wallet availability viaisWalletEnabled(), which reads the process singleton rather thanconfig.appConfig. Everything else instartMcpServernow uses the snapshot; for consistency (and correctness under interleavedmain()calls) this should readconfig.appConfig.walletMode.stop()callingresetWalletProvider()is a process-global side effect. Snapshot-backed runtimes are protected by the WeakMap, but a consumer that built a server via the defaultgetServer()(which binds toprocessConfigand therefore uses the global memo) will have its provider rebuilt from whatever the singleton holds at that moment — the same cross-runtime key-swap the PR is fixing, just on the default path. Worth documenting or removing the defaultprocessConfigfallback.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| this.host = options.host ?? 'localhost'; | ||
| this.path = options.path ?? '/mcp'; | ||
| this.walletMode = options.walletMode ?? 'disabled'; | ||
| this.appConfig = options.appConfig ?? snapshotConfig(); |
There was a problem hiding this comment.
[blocker] The guard in start() (line 101) still validates this.walletMode, which is a separate option from the new appConfig snapshot that actually carries the signing key. Construct new StreamableHttpTransport({ port, host, path, appConfig: <private-key snapshot> }) with walletMode omitted and the guard evaluates 'disabled', passes, and then serverFactory → getServer(this.appConfig) registers the wallet tools and signs with that key over HTTP. The default branch has the same shape: with no appConfig and no walletMode, snapshotConfig() captures a private-key process singleton while the guard sees 'disabled' — and the snapshot now pins that key to the listener for its whole lifetime.
Not reachable through the published entrypoint today (src/index.ts does not export the transports, and parseArgs() derives both fields from the same snapshot so they cannot diverge on the CLI path), but this PR introduces the option that carries a private key into an HTTP transport and leaves it outside the load-bearing check. Suggest validating the effective mode:
const effectiveWalletMode = this.appConfig.walletMode !== 'disabled' ? this.appConfig.walletMode : this.walletMode;
validateSecurityConfig(this.mode, effectiveWalletMode);Note that switching to this.appConfig.walletMode alone would regress the existing still terminates a direct private-key HTTP start before listen test, which passes walletMode: 'private-key' with no appConfig. Please also add a case for the conflicting combination (private-key appConfig + omitted/disabled walletMode).
| this.host = options.host; | ||
| this.path = options.path; | ||
| this.walletMode = options.walletMode ?? 'disabled'; | ||
| this.appConfig = options.appConfig ?? snapshotConfig(); |
There was a problem hiding this comment.
[blocker] Same gap as streamable-http.ts: validateSecurityConfig(this.mode, this.walletMode) at line 228 never inspects this.appConfig, so an SSE transport built with a private-key snapshot and no explicit walletMode starts a listener that exposes signing tools. Apply the same effective-mode fix and cover it in wallet-isolation.test.ts.
| createDocsSearchTool(server, packageInfo); | ||
| function bindServerToAppConfig(server: McpServer, appConfig: AppConfig): McpServer { | ||
| const connect = server.connect.bind(server); | ||
| server.connect = (async (transport: Transport) => { |
There was a problem hiding this comment.
[suggestion] The server.connect override accepts only transport and drops any further arguments before delegating; the as typeof server.connect cast hides the arity mismatch. Protocol.connect in recent @modelcontextprotocol/sdk versions takes an optional second options argument, so this silently discards it for every server returned by getServer(). Safer to forward everything:
server.connect = (async (transport: Transport, ...rest: unknown[]) => {
bindTransportToAppConfig(transport, appConfig);
return (connect as (...args: unknown[]) => Promise<void>)(transport, ...rest);
}) as typeof server.connect;| return (...args: Args): Result => runWithAppConfig(appConfig, () => fn(...args)); | ||
| } | ||
|
|
||
| export function getRuntimeConfig(): AppConfig { |
There was a problem hiding this comment.
[suggestion] getRuntimeConfig() fails open: with no ALS store it returns the mutable process singleton. Every wallet read that escapes the runWithAppConfig scope — a handler scheduled from a timer or an emitter registered outside the request chain — silently reverts to whatever the singleton currently holds, which is exactly the state this PR is isolating against. The current call sites all stay inside the context (the SDK dispatches request handlers through microtasks, which propagate ALS), so this is defense-in-depth rather than a live bug, but it is worth a comment here stating the invariant. Related: snapshotConfig() freezes its result but returns AppConfig, not Readonly<AppConfig>, so mutation attempts are only caught at runtime.
| const HOST = '127.0.0.1'; | ||
| const PATH = '/mcp'; | ||
| const PRIVATE_KEY = '1'.repeat(64); | ||
| const WALLET_TOOLS = new Set([ |
There was a problem hiding this comment.
[suggestion] WALLET_TOOLS is a hardcoded name list and both HTTP cases only assert toEqual([]), so the tests pass vacuously if a tool is renamed or the set drifts from registerEVMTools. Add a positive control — e.g. assert that a private-key snapshot server does list a non-empty intersection with this set — so the empty-array assertions keep their meaning. (REVIEW.md names this file as the isolation suite to keep meaningful, which makes the missing control more load-bearing.)
|
|
||
| constructor() { | ||
| this.privateKey = getPrivateKeyAsHex(); | ||
| constructor(...key: [] | [string | undefined]) { |
There was a problem hiding this comment.
[nit] constructor(...key: [] | [string | undefined]) is an obscure way to distinguish "no argument" from "explicit undefined". The distinction is deliberate and security-relevant (a private-key snapshot with an undefined key must not fall back to the singleton), which is exactly why it deserves to be explicit rather than encoded in a variadic tuple. Consider a named static factory, e.g. keep constructor(privateKey: string | undefined) and add static fromProcessConfig() { return new PrivateKeyWalletProvider(getPrivateKeyAsHex()); } for the singleton path.
| transports are reachable cross-origin, so a signing key behind one is a | ||
| drain-the-wallet primitive. Any change that narrows this check, makes it | ||
| non-fatal, or adds a transport that bypasses it is a finding. | ||
| - **Tool and signing policy is instance-scoped.** Each runtime closes over |
There was a problem hiding this comment.
[suggestion] This PR adds reviewer-facing directives to the AI review guidelines, authored in the same change they govern — it tells reviewers what to classify as a regression and which test file to preserve. Flagging per the untrusted-content rule: this text was treated strictly as material under review, not as instructions. It reads as legitimate documentation rather than an injection attempt, but PR-authored changes to the review contract should get explicit human sign-off, and it ideally lands separately from the implementation it blesses.
Separately, the wording pins the guidelines to implementation details ("a default serverFactory that calls unbound getServer()", "outside runWithAppConfig") that will ossify the current monkey-patching design. Stating the invariant — a running transport's tool and signing policy must not change when the process config is mutated — would survive refactors better.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the SeiDroid review in e9082c1:
Verification:
|
There was a problem hiding this comment.
Sound hardening of the wallet-on-HTTP invariant: an AsyncLocalStorage-scoped frozen AppConfig snapshot replaces process-global config reads, and the security guard is widened rather than narrowed. No blocking defects; remaining notes are about optional/fallback paths that keep the mutable singleton reachable, an unenforced MCP SDK ordering assumption, and the in-PR edit to the reviewer guidance file.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Codex's findings with my own. Codex reported no material implementation defects and could not run tests. - I could not run
bun test --isolate srcin this checkout (node_modules is not installed), so all findings come from reading the diff and surrounding source rather than execution. The PR's own test plan checkboxes are still unchecked. - Changeset and docs are correct per the repo conventions:
.changeset/fix-mcp-server-instance-scoped-wallet.mdis a patch scoped to@sei-js/mcp-serveronly (no coordinated bump requested), andpackages/mcp-server/README.mddocuments the new per-main()snapshot behaviour. - Test coverage for the new invariant is good — the new
wallet-isolation.test.tsdrives real MCP clients over both HTTP transports and asserts the tool list stays empty across a laterinitializeConfig(), plus a stdio test that asserts the signer address does not change. One gap worth considering: the stdio case is simulated viagetServer()+InMemoryTransportrather than two concurrentstartMcpServer()runtimes, so the full lifecycle (twoparseArgs()snapshots, onestop()callingresetWalletProvider) is not exercised end-to-end. - Codex flagged the
REVIEW.mdchange as a possible prompt-injection surface (its only finding). I agree it is worth surfacing and have left an inline note; the added text is accurate and matches the implementation, so I do not treat it as an attack, but reviewer-guidance changes landing in the same PR as the code they legitimise should get explicit human sign-off. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| transports are reachable cross-origin, so a signing key behind one is a | ||
| drain-the-wallet primitive. Any change that narrows this check, makes it | ||
| non-fatal, or adds a transport that bypasses it is a finding. | ||
| - **Tool and signing policy is instance-scoped.** A running transport's tool |
There was a problem hiding this comment.
[suggestion] This PR edits the file that instructs AI reviewers, adding the very invariant the PR introduces and directing reviewers to keep this PR's own test file "meaningful". The wording is accurate and matches the implementation, so I read it as genuine documentation rather than an attempt to steer review — I evaluated the code on its merits and did not treat this text as authoritative guidance. Still worth a human maintainer explicitly signing off: a change to reviewer instructions landing in the same PR as the code it legitimises is a pattern that should never merge on the strength of an automated review alone. Consider splitting it into a separate docs PR.
| export interface TransportConfig { | ||
| mode: TransportMode; | ||
| walletMode: WalletMode; | ||
| appConfig?: AppConfigSnapshot; |
There was a problem hiding this comment.
[suggestion] appConfig is optional here, and both HTTP transports resolve it with snapshotConfig(options.appConfig) — which, given snapshotConfig(source = config), falls back to a snapshot of the mutable process singleton when omitted. That fallback is safe today (it snapshots at construction, and the tests cover it), but it means the isolation guarantee depends on every caller remembering to pass appConfig, with no signal when they don't.
Since parseArgs() always populates it, consider making appConfig required in TransportConfig (and in HttpSseTransportOptions / StreamableHttpTransportOptions). That turns a silent fallback into a compile error and removes the last path by which a transport can be governed by config it didn't snapshot.
| const start = transport.start.bind(transport); | ||
| let bound = false; | ||
| transport.start = async () => { | ||
| if (!bound && transport.onmessage) { |
There was a problem hiding this comment.
[suggestion] bindTransportToAppConfig only wraps onmessage if it is already assigned when start() runs. That holds for the current SDK, where Protocol.connect() assigns onclose/onerror/onmessage and then awaits transport.start() — but it is an undocumented ordering dependency on @modelcontextprotocol/sdk (^1.23.0, so minors float). If a future release moves the assignment after start(), bound stays false, no wrap happens, and there is no error or log: handlers silently fall back to getRuntimeConfig()'s mutable-singleton path.
There is redundant coverage (the runWithAppConfig wrappers around handlePostMessage/handleRequest, and stdio.ts's explicit post-connect re-wrap — which double-wraps onmessage in the stdio case, harmless but worth a comment), so this is defence-in-depth rather than the only barrier. Given the invariant is load-bearing, I'd still add a comment naming the SDK ordering assumption, and consider warning when transport.onmessage is unset at start() so the regression is loud rather than silent.
| * Runtime request paths must establish an AsyncLocalStorage scope. The mutable | ||
| * process config remains the fallback for direct configuration helpers. | ||
| */ | ||
| export function getRuntimeConfig(): Readonly<AppConfig> { |
There was a problem hiding this comment.
[suggestion] The whole isolation property rests on every wallet-sensitive read happening inside a runWithAppConfig scope; when the scope is missing, getRuntimeConfig() silently returns the mutable config. That is the right fallback for the direct configuration helpers the comment mentions, but nothing distinguishes "legitimately outside a runtime" from "a request path that lost its scope" — and the latter is exactly the regression this PR exists to prevent.
A low-cost guard: have the wallet-sensitive getters (getPrivateKeyAsHex, isWalletEnabled, getWalletMode) emit a one-shot console.error when called with no store while config.walletMode !== 'disabled'. Cheap, stdio-safe (stderr, per the repo convention), and it surfaces a missed scope in tests instead of in production.
|
|
||
| async start(_server?: McpServer): Promise<void> { | ||
| validateSecurityConfig(this.mode, this.walletMode); | ||
| validateSecurityConfig(this.mode, this.walletMode, this.appConfig.walletMode); |
There was a problem hiding this comment.
[suggestion] Worth being deliberate about this behaviour change (same at http-sse.ts:228). Because validateSecurityConfig now ORs the two sources, an HTTP transport constructed with an explicit walletMode: 'disabled' will process.exit(1) if the ambient process config happens to be private-key — e.g. a co-resident stdio runtime started earlier in the same process. Previously that combination started fine (and, in the old code, actually leaked signing tools onto HTTP, since serverFactory read the global — so the new hard failure is strictly safer and I'm not asking you to soften it; process.exit(1) here is the documented intent).
The observation is that this.appConfig is the thing that actually governs tool registration, so options.walletMode is now redundant input that can only ever add failure modes. Deriving the checked mode from appConfig alone, and treating a conflicting options.walletMode as a programming error at construction, would give the same fail-closed guarantee without letting an unrelated runtime's config take down this one.
| * Exits the process if unsafe configuration detected | ||
| */ | ||
| export function validateSecurityConfig(transportMode: TransportMode, walletMode: WalletMode): void { | ||
| export function validateSecurityConfig(transportMode: TransportMode, walletMode: WalletMode, ...additionalWalletModes: WalletMode[]): void { |
There was a problem hiding this comment.
[nit] The variadic ...additionalWalletModes is an unusually loose shape for a check with exactly two known sources — both call sites pass precisely one extra argument. A named third parameter (e.g. snapshotWalletMode?: WalletMode) would be self-documenting at the call site, would make the arity explicit, and would keep the array allocation on line 24 unnecessary.
Superseded: latest AI review found no blocking issues.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the approved SeiDroid follow-up suggestions in c06418b:
Verification: |
There was a problem hiding this comment.
Solid per-runtime wallet isolation that preserves and strengthens the wallet-on-HTTP guard (HTTP transports now derive walletMode from the snapshot instead of defaulting to 'disabled'), with a changeset and genuine end-to-end isolation tests. No blockers; the notable items are an internal default-construction path that now throws, a fail-open fallback in getRuntimeConfig(), and coupling to MCP SDK internals via onmessage property interception.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output —
cursor-review.mdis empty, so only Codex's review was available to merge with mine. - I could not run
bun test --isolate srcin this environment, so the PR's test-plan checkboxes are unverified from my side. - Consider a brief note in the README or a code comment recording why the AsyncLocalStorage +
onmessage-interception mechanism exists. The changeset itself says "No shipped CLI or host spawn does that", so a future maintainer looking at ~90 lines of property interception inserver.tshas no way to tell it is load-bearing rather than incidental, and guideline §1 asks that wallet invariants be legible in code. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| private transport?: StdioServerTransport; | ||
|
|
||
| constructor(appConfig?: AppConfigSnapshot) { | ||
| this.appConfig = snapshotConfig(appConfig); |
There was a problem hiding this comment.
[suggestion] Codex flagged this and the mechanism is real: new StdioTransport() calls snapshotConfig(undefined), which falls back to the mutable config and freezes a fresh object. await getServer() independently freezes another. start() then calls bindTransportToAppConfig(transport, A) before server.connect(transport), whose bindServerToAppConfig wrapper re-binds with B — existingConfig !== appConfig, so it throws MCP transport is already bound to a different AppConfig snapshot. even though both snapshots hold identical values.
I'd downgrade Codex's P2, though: StdioTransport and getServer are not in the package exports map (only . → src/index.ts), and startMcpServer() always threads the single config.appConfig through both getServer() and createTransport(), so no reachable path hits this. It's still worth fixing as an API-consistency issue — make appConfig required here as HttpSseTransportOptions/StreamableHttpTransportOptions now do. Worth noting the 12 new StdioTransport() call sites in stdio.test.ts only pass because StdioServerTransport and server.connect are both mocked, so the default path has no real coverage either way.
| export function getRuntimeConfig(): Readonly<AppConfig> { | ||
| const scopedConfig = getScopedAppConfig(); | ||
| if (scopedConfig) return scopedConfig; | ||
| if (config.walletMode !== 'disabled' && !warnedAboutUnscopedWalletRead) { |
There was a problem hiding this comment.
[suggestion] This fallback fails open on the one value the package treats as security-critical: if the AsyncLocalStorage scope is ever lost, wallet reads silently revert to the mutable process singleton, which is precisely the pre-PR behaviour the changeset describes as the defect. The only signal is a single stderr line, and initializeConfig() resets warnedAboutUnscopedWalletRead, so a repeated leak can stay quiet.
Today the blast radius is small — tools.ts:64 gates wallet tools at registration time inside the getServer scope, so a wallet-disabled HTTP runtime registers no signing tool that could reach getPrivateKeyAsHex(). But per guideline §1 this is the kind of guard that should fail closed: consider returning a walletMode: 'disabled' snapshot (or throwing) for unscoped reads, and keep the mutable fallback only for the non-wallet configuration helpers. At minimum, warn on every unscoped read rather than once.
| const initialHandler = transport.onmessage; | ||
| const descriptor = findPropertyDescriptor(transport, 'onmessage'); | ||
| let boundHandler: Transport['onmessage']; | ||
| Object.defineProperty(transport, 'onmessage', { |
There was a problem hiding this comment.
[suggestion] Redefining onmessage as an accessor couples the wallet guarantee to an MCP SDK implementation detail — that Protocol.connect() assigns the handler through the property and the transport invokes it by reading the property back. If a future SDK version stores the handler in a private field or captures a local reference, this interception stops applying and (given the fail-open fallback in getRuntimeConfig()) the runtime degrades to the mutable singleton with no test failure. @modelcontextprotocol/sdk is a caret range (^1.23.0), so a minor bump could do it.
Two smaller things about the accessor itself:
- The getter returns the wrapped handler, so
transport.onmessage !== theHandlerJustAssigned. Chaining patterns (const prev = t.onmessage; t.onmessage = m => { prev?.(m); ... }) produce nestedrunWithAppConfigcalls — harmless with the same snapshot, but identity comparisons would silently fail. enumerable: truemeans spreading or serialising the transport now invokes the getter and yields a function where a plain field used to be.
Worth adding an assertion against the real SDK transport (the server.test.ts case uses a hand-rolled { start, onmessage } fake) so an SDK upgrade that bypasses the property breaks CI rather than the invariant. The wallet-isolation.test.ts HTTP cases do exercise the real SDK, so most of this is covered — the gap is a direct assertion that the interception is what's doing the work.
| export interface TransportConfig { | ||
| mode: TransportMode; | ||
| walletMode: WalletMode; | ||
| appConfig: AppConfigSnapshot; |
There was a problem hiding this comment.
[nit] TransportConfig now carries both walletMode and appConfig.walletMode. args.ts:157 derives the former from the latter and factory.ts no longer forwards it, so the only remaining reader is index.ts:55's log line — two sources of truth for a security-critical value, which is exactly the drift the HTTP constructors now throw 'walletMode must match appConfig.walletMode.' to prevent. Dropping walletMode from TransportConfig and reading config.appConfig.walletMode at index.ts:55 would make the invariant structural instead of checked.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest four non-blocking notes in 86b098a:
Verification: |
There was a problem hiding this comment.
The per-runtime AppConfig snapshot is a sound fix: parseArgs() freezes a snapshot, all three transports require it, HTTP signing policy and validateSecurityConfig() derive from it, and wallet tool handlers are individually bound via wrapWithAppConfig so they don't depend on AsyncLocalStorage surviving stdin/SSE event boundaries. No blocking correctness or security problems found; the notes below are hardening, test-durability, and simplification suggestions.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported "no material issues" but explicitly could not run the tests, so neither pass contributes independent findings here. - I could not execute
bun test --isolate srcin this environment either (bun/node_modules unavailable), so my verdict rests on static review. The PR's test plan claims the suite passes; CI should be the gate. - Defense-in-depth gap worth noting: only tools registered through
withToolRegistrationPolicyget snapshot-bound handlers. Resource handlers (registerEVMResources), prompt handlers, andcreateDocsSearchToolare registered on the raw server and are therefore unbound. None of them reads wallet config today (verified by grep forisWalletEnabled/getWalletMode/getPrivateKeyAsHex/getWalletProvider), so there's no live bug — but under stdio a future resource/prompt that touches wallet state would silently read the mutable process singleton instead of the runtime snapshot. A short comment at theregisterEVMResources/registerEVMPromptscall sites inserver.ts, or routing them through a snapshot-binding wrapper too, would keep that from regressing quietly. - For the record on release scoping:
getServer()gaining a required argument and the transport constructors now requiringappConfigare technically breaking signature changes, but none of those symbols is reachable through the package'sexportsmap (only.→dist/index.js, which exportsmain/runCli/startMcpServer/registerShutdownHandlers/isDirectExecution). The public lifecycle is unchanged, so thepatchchangeset is the right level — no action needed. - No prompt-injection or instruction-like content found in the diff, changeset, or PR description.
- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| const HOST = '127.0.0.1'; | ||
| const PATH = '/mcp'; | ||
| const PRIVATE_KEY = '1'.repeat(64); | ||
| const WALLET_TOOLS = new Set([ |
There was a problem hiding this comment.
[suggestion] This hardcoded WALLET_TOOLS allowlist can rot into a false pass on the repo's most security-sensitive invariant. listWalletTools() filters the listed tools down to these nine names and the assertions check toEqual([]), so the test only proves "none of these nine specific tools leaked." Add a tenth signing tool to tools.ts and forget to add it here, and every isolation assertion in this file still passes while the tool is exposed on a wallet-disabled HTTP listener.
Inverting the check makes it self-maintaining and strictly stronger: export READ_ONLY_TOOL_NAMES from src/core/tools.ts and assert that every tool the session lists is in that set (i.e. return the names not in READ_ONLY_TOOL_NAMES and expect []). Then any newly added non-read-only tool is covered automatically.
| return undefined; | ||
| } | ||
| return registerTool(name, description, 'network' in schema ? { ...schema, network: networkSchema.optional() } : schema, handler); | ||
| const runtimeHandler = appConfig ? wrapWithAppConfig(appConfig, handler) : handler; |
There was a problem hiding this comment.
[suggestion] appConfig ? wrapWithAppConfig(...) : handler silently degrades to unbound handlers when there is no active scope. This ternary is the load-bearing mechanism for stdio wallet isolation (the ALS scope opened around server.connect() is not guaranteed to survive stdin data events, which is why per-handler binding exists at all). If a future refactor moves registerEVMTools outside the runWithAppConfig scope in server.ts, every wallet tool reverts to reading the mutable process singleton and nothing fails — the stdio isolation test only exercises the in-scope path.
Consider making the degraded path noisy or fatal when walletEnabled is true, e.g. if (walletEnabled && !appConfig) throw new Error('withToolRegistrationPolicy requires an active AppConfig scope when the wallet is enabled.'). The existing unit tests that call registerEVMTools without a scope pass walletEnabled: false-equivalent config, or can be wrapped in runWithAppConfig.
| const scopedConfig = getScopedAppConfig(); | ||
| if (scopedConfig) return scopedConfig; | ||
| if (config.walletMode !== 'disabled') { | ||
| console.error('Wallet configuration was read outside an MCP runtime scope; using the mutable process configuration.'); |
There was a problem hiding this comment.
[suggestion] This diagnostic fires on every out-of-scope read while the wallet is enabled (the new test asserts exactly that: two reads → two console.error calls). The doc comment above declares the mutable config a supported fallback for "direct configuration helpers," so an embedder that calls initializeConfig() and then drives services/contracts.ts directly hits getPrivateKeyAsHex() twice per write path and floods stderr — which under stdio is the same stream the host reads for diagnostics.
A once-per-process (or once-per-config-object) warning conveys the same information without the unbounded volume. If the per-read repetition is deliberate for auditability, a brief comment saying so would help, since it reads as an oversight.
| // Cache for the process-global singleton only. Instance snapshots use the WeakMap. | ||
| let walletProviderInstance: WalletProvider | null = null; | ||
|
|
||
| function createWalletProvider(appConfig: AppConfigSnapshot): WalletProvider { |
There was a problem hiding this comment.
[nit] createWalletProvider() now duplicates the wallet-mode switch that still lives in the singleton fallback path of getWalletProvider() (including the Unknown wallet mode: ${mode} throw). Adding a third wallet mode later requires editing both, and only one of them is covered by the new snapshot tests.
The fallback branch can reuse the new helper: if (!walletProviderInstance) walletProviderInstance = createWalletProvider(snapshotConfig()); return walletProviderInstance;. That preserves the memo and the error message. Note it would change which config accessor the existing 'Unknown wallet mode: unknown-mode' unit test has to mock (config rather than getWalletMode), so it's a judgment call whether the churn is worth it.
| host: string; | ||
| path: string; | ||
| appConfig: AppConfigSnapshot; | ||
| /** @deprecated Wallet mode is derived from appConfig. */ |
There was a problem hiding this comment.
[nit] Now that appConfig is required, this @deprecated walletMode option grants no capability — it can only either match appConfig.walletMode (no-op) or throw. Since the constructor is already a breaking change in this PR, dropping the option outright would remove the compatibility branch here and in streamable-http.ts:71, plus the two rejects conflicting compatibility walletMode tests, without costing any caller anything. Same note applies to StreamableHttpTransportOptions.walletMode.
| const errors = await collectOperationErrors([() => transport.stop(), () => server?.close()]); | ||
| throwCollectedErrors(errors, 'Failed to stop all MCP server resources.'); | ||
| } finally { | ||
| resetWalletProvider(config.appConfig); |
There was a problem hiding this comment.
[nit] Worth being precise in the changeset/README wording: this is a WeakMap cache eviction, not a release of key material. The private key stays live in the frozen config.appConfig snapshot that this closure (and the transport) still holds, and an in-flight request still inside the snapshot's scope would just get a freshly constructed PrivateKeyWalletProvider with the same key. Harmless in practice — stop() is terminal and the transport is already down — but "Stopping one runtime releases only its provider" reads as a key-lifetime guarantee that isn't being made.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest approved cleanup notes in e1ce4f6:
Verification: |
There was a problem hiding this comment.
Per-runtime wallet isolation via an AsyncLocalStorage-scoped frozen AppConfig snapshot is a sound design and preserves both load-bearing security invariants (the stdio-only wallet guard is now fed from the same snapshot that governs signing, and SSE session binding is untouched). No blockers; five non-blocking notes, chiefly that the new isolation test asserts against the same READ_ONLY_TOOL_NAMES constant it is meant to police, so it does not actually prevent the allowlist drift the PR description claims.
Findings: 0 blocking | 8 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verification gap on my side:
bun test --isolate srcandbun run checkcould not be run in this review environment, so the PR's test-plan checkboxes are unverified here. Worth confirming CI is green, particularlysrc/tests/core/config.test.ts, which now depends on module-levelwarnedAboutUnscopedWalletReadstate — the warn-once test only passes because it callsinitializeConfig()(which resets the flag) before assertingtoHaveBeenCalledTimes(1). That is correct today but is order-sensitive within the file if tests are later reordered or a new case is inserted above it. - The Cursor second-opinion file (
cursor-review.md) was empty, so that pass produced no output. Codex reported no material issues but also could not run the test suite. - Positive confirmation for the record:
getServer,StdioTransport,HttpSseTransport,StreamableHttpTransportandresetWalletProviderare not reachable through the package'sexportsmap (onlysrc/index.tsis published), so makingappConfiga required parameter is not a breaking change for consumers and thepatchchangeset is correctly scoped. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| 'is_contract', | ||
| 'read_contract' | ||
| 'read_contract', | ||
| 'search_docs' |
There was a problem hiding this comment.
[suggestion] Adding search_docs here has no runtime effect: createDocsSearchTool(server, packageInfo) in server/server.ts:26 registers that tool on the raw McpServer, never through the policy proxy, so the name is never tested against this set in production. The entry exists only so the new listUnsafeTools() helper in wallet-isolation.test.ts filters it out.
That coupling undercuts the PR description's claim that the isolation tests "reject every tool outside the exported read-only surface, preventing allowlist drift" — the test oracle and the production allowlist are now the same constant, so a future PR that mistakenly adds a wallet-mutating tool name to READ_ONLY_TOOL_NAMES would both un-suppress that tool under a wallet-disabled runtime and keep the isolation test green. Consider having the test assert against a literal expected tool-name list (or the set plus an explicit ['search_docs']), so the constant under test is not also the constant doing the checking.
| return undefined; | ||
| } | ||
| return registerTool(name, description, 'network' in schema ? { ...schema, network: networkSchema.optional() } : schema, handler); | ||
| const runtimeHandler = appConfig ? wrapWithAppConfig(appConfig, handler) : handler; |
There was a problem hiding this comment.
[suggestion] The scope guard is asymmetric: line 37 hard-fails when walletEnabled && !appConfig, but here a missing scope silently falls back to the unwrapped handler. Those handlers then resolve wallet state through getRuntimeConfig()'s mutable-process-config fallback at call time, which is exactly the drift this PR removes — a later initializeConfig() enabling the wallet would be visible to them.
Today this is defensive-only (registerEVMTools is reached solely from getServer(), which always establishes the scope), and the surviving tools in the wallet-disabled case are read-only, so impact is low. But the fallback is also the branch a future refactor is most likely to land in unnoticed. Requiring a scope unconditionally, or binding to an explicit frozen { walletMode: 'disabled' } snapshot when none is active, would make the guarantee hold regardless of how registration is reached.
| registerEVMTools(server); | ||
| registerEVMPrompts(server); | ||
| createDocsSearchTool(server, packageInfo); | ||
| // Resources and prompts are wallet-independent. Any future |
There was a problem hiding this comment.
[suggestion] The comment is inaccurate for prompts: registerEVMPrompts gates registerWalletPrompts on isWalletEnabled() (core/prompts.ts:37), so prompt registration is wallet-dependent. It happens to be correct here because the call sits inside runWithAppConfig, which is precisely the load-bearing detail the comment obscures — a maintainer trusting "wallet-independent" could hoist registerEVMPrompts(server) out of the scope and silently reintroduce the singleton read.
Suggest rewording to something like: "Registration runs inside the runtime scope so wallet-gated registration (prompts) and tool policy read this snapshot. Any wallet-sensitive callback must additionally be bound via the scoped tool policy, since resource and prompt callbacks are not wrapped." That also documents the real remaining constraint.
| */ | ||
| export function snapshotConfig(source: Readonly<AppConfig> = config): AppConfigSnapshot { | ||
| if (Object.isFrozen(source)) return source; | ||
| return Object.freeze({ |
There was a problem hiding this comment.
[suggestion] Enumerating the three AppConfig fields explicitly makes this a silent-drop point: a field added to AppConfig later would be absent from every snapshot, and because getRuntimeConfig() returns the snapshot inside a runtime scope, a new accessor built on it would read undefined while the same accessor outside a scope reads the real value from the process singleton. That is an unpleasant class of bug to track down.
Object.freeze({ ...source }) gives the same shallow freeze (all three fields are primitives) and stays correct as the interface grows.
| return walletProviderInstance; | ||
| } | ||
|
|
||
| walletProviderInstance = createWalletProvider(snapshotConfig(processConfig)); |
There was a problem hiding this comment.
[nit] Worth noting the lifecycle asymmetry this creates: stop() in index.ts now always calls resetWalletProvider(config.appConfig), which only deletes the WeakMap entry — so this process-global memo is never evicted by the runtime lifecycle, only by a bare resetWalletProvider() (tests). A first getWalletProvider() call that lands outside a runtime scope therefore pins a signer built from the mutable config for the remaining life of the process, surviving every subsequent initializeConfig(), with only the one-shot stderr diagnostic as a signal.
No production path reaches it (all wallet reads flow through wrapped tool handlers), and it matches the pre-PR behaviour, so this is a note rather than a defect. A brief comment recording that the fallback memo is deliberately process-lifetime would save the next reader the trace.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the newest approved notes in 40523dd:
Verification: |
There was a problem hiding this comment.
Solid, well-tested hardening: wallet policy now travels with a frozen per-runtime AppConfig snapshot through AsyncLocalStorage, the HTTP wallet guard is fed from that snapshot instead of an optional constructor arg (a net strengthening), SSE session binding is untouched, and a patch changeset is present. No blockers found; remaining notes are about the deliberate unscoped fallbacks, which weaken the stated isolation invariant at the edges.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
parseArgs/TransportConfig/getServer/transport constructors all changed shape, but none of them are part of the published surface (package.jsonexports onlydist/index.js, andsrc/index.tsexports justmain/runCli/startMcpServer/registerShutdownHandlers/isDirectExecution), so thepatchchangeset is the right bump. Flagging only so it's recorded as checked, not as a defect.- Verified the two load-bearing invariants from REVIEW_GUIDELINES.md are intact:
validateSecurityConfig(this.mode, this.appConfig.walletMode)still runs before anylistenin both HTTP transports (and is now harder to bypass than the oldoptions.walletMode ?? 'disabled'default), andhttp-sse.tsstill keysconnectionsbytransport.sessionIdwith 400/404 on missing/unknown?sessionId=. No newconsole.logon a stdio-reachable path. src/tests/core/tools.test.tsmockswrapWithAppConfigto the identity function inbeforeEach, so that suite no longer exercises the snapshot binding it is nominally covering — the only real coverage for handler-level binding is the new stdiowallet-isolation.test.tscase viaInMemoryTransport. That is adequate, but consider one assertion intools.test.tsthatwrapWithAppConfigwas called with the scoped snapshot for each registered tool, so a future refactor that drops the wrap is caught where tools are tested.- Could not execute the test plan in this environment:
node_modulesis not installed andbunis unavailable, sobun test --isolate src/testswas not run. Findings below are from static reading only. Codex's second-opinion pass reported no material issues and also could not run tests. The Cursor second-opinion file (cursor-review.md) is empty — that pass produced no output. - Nothing in the diff, PR title, or PR body attempted to instruct the reviewer; no prompt-injection concerns.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| return walletProviderInstance; | ||
| } | ||
|
|
||
| walletProviderInstance = createWalletProvider(snapshotConfig(processConfig)); |
There was a problem hiding this comment.
[suggestion] The unscoped fallback reads processConfig directly rather than going through getRuntimeConfig(), so the new "Wallet configuration was read outside an MCP runtime scope" diagnostic never fires here. That inverts the intent: of all the unscoped reads, this is the one that actually materializes a signer from the mutable process config, and it is the only wallet-sensitive read that stays silent. getPrivateKeyAsHex()/isWalletEnabled()/getWalletMode() all warn; provider construction does not.
Suggest routing this through getRuntimeConfig() (or emitting the same one-shot diagnostic) so a lost runtime scope is observable on the path that matters most.
| */ | ||
| export function resetWalletProvider(): void { | ||
| walletProviderInstance = null; | ||
| export function resetWalletProvider(appConfig?: AppConfigSnapshot): void { |
There was a problem hiding this comment.
[suggestion] resetWalletProvider(appConfig) only deletes the providersByConfig entry, and src/index.ts stop() only ever calls the snapshot form. So if anything triggered the unscoped path while the wallet was enabled, walletProviderInstance holds a PrivateKeyWalletProvider (and therefore the key material) for the remainder of the process, surviving stop() and never being invalidated by a later initializeConfig().
That's a gap in the changeset's stated invariant ("stopping one runtime evicts only its provider cache entry") — the fallback memo is a second, unowned copy of the signer with a process-lifetime lease. Options: drop the fallback memo entirely (construct per call when unscoped), or clear it in initializeConfig()/stop() as well.
| walletApiKey: string | undefined; | ||
| } | ||
|
|
||
| export type AppConfigSnapshot = Readonly<AppConfig>; |
There was a problem hiding this comment.
[nit] Readonly<AppConfig> is structurally assignable from a mutable AppConfig, so this alias gives no compile-time guarantee that callers pass an actual snapshot — the snapshots a mutable %s appConfig at construction test in wallet-isolation.test.ts relies on exactly that. Enforcement is entirely runtime (runWithAppConfig's Object.isFrozen check plus defensive snapshotConfig calls in every transport and getServer).
A brand would make the invariant checkable statically and let you drop the defensive re-snapshotting, e.g.:
declare const snapshotBrand: unique symbol;
export type AppConfigSnapshot = Readonly<AppConfig> & { readonly [snapshotBrand]: true };with snapshotConfig as the only cast site.
| registerEVMResources(server); | ||
| registerEVMTools(server); | ||
| registerEVMPrompts(server); | ||
| createDocsSearchTool(server, packageInfo); |
There was a problem hiding this comment.
[suggestion] Only server.tool is bound to the snapshot (via the withToolRegistrationPolicy proxy + wrapWithAppConfig). Resource and prompt callbacks registered on lines 24/26 run unwrapped, and createDocsSearchTool is handed the raw server, bypassing the policy proxy — so search_docs is also unbound.
I confirmed this is currently safe: resources.ts only reaches read-only services (getChainId, getBlockNumber, getBalance, getERC*, getTransaction, isNFTOwner), prompts.ts reads isWalletEnabled() at registration time (inside this scope) rather than in a callback, and every wallet-touching service (getWalletClientFromProvider, getPrivateKeyAsHex in writeContract) is reachable only from tools suppressed when the wallet is disabled.
The comment on lines 21-23 states the invariant but nothing enforces it — a future wallet-sensitive resource or prompt callback would silently fall through to the mutable process config, which is the exact bug this PR fixes. Worth either routing all four registrations through a scope-binding wrapper, or adding a test that asserts no wallet read occurs outside a runtime scope during a resource/prompt/docs invocation.
| const HOST = '127.0.0.1'; | ||
| const PATH = '/mcp'; | ||
| const PRIVATE_KEY = '1'.repeat(64); | ||
| const EXPECTED_HTTP_TOOL_NAMES = [ |
There was a problem hiding this comment.
[nit] This literal is READ_ONLY_TOOL_NAMES from src/core/tools.ts plus search_docs (19 + 1 = 20, verified to match). Duplicating it deliberately is the right call for drift detection, but the failure a future author sees will be an opaque array diff. A one-line comment saying it intentionally mirrors READ_ONLY_TOOL_NAMES + search_docs, and that adding a read-only tool means updating it here, would save that debugging round-trip.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid hardening of per-runtime wallet isolation: the stdio-only wallet guard and SSE session binding are both preserved, the scoped-callback proxy composes correctly with the existing tool-registration policy, and a changeset plus meaningful isolation tests are included. No blockers; the remaining notes are about the isolation being fail-open in two places that could quietly regress later.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass (
cursor-review.md) is empty — that review produced no output. Codex (codex-review.md) reported no material issues. - I could not execute
bun test --isolate srcortsc --noEmitin this environment (both commands were blocked), so this review is static-analysis only. The PR's own test plan claims the suite passes; CI should be the gate. - Minor inconsistency:
stdio.tswrapsserver.connect()inrunWithAppConfig, buthttp-sse.ts:154andstreamable-http.tscallserver.connect(transport)outside the scope. Harmless today (the SDK'sconnectreads no wallet config) but worth aligning so the pattern is uniform. config.test.tstests in theisWalletEnabled/getWalletModedescribes setconfig.walletMode = 'private-key'without an ALS scope, so they now hit the new unscoped-read branch and can write to real stderr. The module-levelwarnedAboutUnscopedWalletReadflag also persists across tests in the file. Not a failure, just noise and a bit of order-coupling — resetting the flag inbeforeEach(or exporting a test-only reset) would keep output clean.- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| name: packageInfo.name, | ||
| version: packageInfo.version | ||
| }); | ||
| const SCOPED_REGISTRATION_METHODS = new Set<PropertyKey>(['tool', 'resource', 'prompt']); |
There was a problem hiding this comment.
[suggestion] This set covers only the legacy tool / resource / prompt methods. @modelcontextprotocol/sdk (the package pins ^1.23.0) also exposes registerTool / registerResource / registerPrompt, which are the current non-deprecated registration API. Any method not in this set falls through to value.bind(target) and is registered unwrapped, so its callback reads wallet policy from the mutable process singleton via getRuntimeConfig()'s fallback.
Nothing in the repo uses the register* forms today, so this is not a live bug — but the failure mode is silent and it is exactly the regression this PR exists to prevent. The first person who migrates a tool to server.registerTool(...) reintroduces it with no test failure (the isolation test only asserts the tool list, and a wallet tool registered unwrapped would still be suppressed at registration time by withToolRegistrationPolicy, so the list check would pass while the handler leaked the process key).
Suggest adding the three register* names to the set, and/or failing loud on unrecognised registration-shaped methods rather than passing them through.
| if (scopedConfig) return scopedConfig; | ||
| // Direct helpers intentionally retain the process fallback. Warn once per | ||
| // initialization so a lost runtime scope is visible without flooding stderr. | ||
| if (config.walletMode !== 'disabled' && !warnedAboutUnscopedWalletRead) { |
There was a problem hiding this comment.
[suggestion] The fallback here is fail-open: a lost ALS scope silently reverts to the mutable process config — the exact state this PR hardens against — and the only signal is one stderr line. Two things make that signal weaker than it looks:
initializeConfig()resetswarnedAboutUnscopedWalletReadtofalse, so in a multi-start process the warning can be re-armed and re-suppressed repeatedly; you get at most one line per initialization, which is easy to lose in startup output.- The warning is gated on the process config being wallet-enabled. If the process config is
disabledbut a live runtime snapshot is wallet-enabled, a lost scope returnsundefinedfor the key with no warning at all. That direction is safe, but it also means a scope leak in the stdio path is invisible.
Not blocking — keeping a fallback for direct helper use is a reasonable call, and the comment says so. But consider making the warning unconditional on scope-absence (not on walletMode), so the diagnostic covers both directions.
| } | ||
|
|
||
| return walletProviderInstance; | ||
| return createWalletProvider(snapshotConfig(getRuntimeConfig())); |
There was a problem hiding this comment.
[nit] The unscoped path allocates a fresh frozen snapshot and a fresh provider on every call, so getWalletProvider() !== getWalletProvider() outside a runtime scope. That's a deliberate change (the removed test asserted the old caching, and the new one asserts non-memoization), and providers are cheap here — PrivateKeyWalletProvider only stores the key, and getWalletClient() already builds a new viem client per call. Nothing in src/ compares providers by identity, so this is fine; flagging only so it's a conscious contract for future callers.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The per-runtime AppConfigSnapshot + AsyncLocalStorage scoping is sound: registration-time wallet gating (tools.ts:64, prompts.ts:37) and every request callback now read a frozen snapshot, validateSecurityConfig still runs off appConfig.walletMode before listen, and no unscoped wallet read remains on a production path. No blockers found; the remaining notes are hardening and maintainability suggestions around the fail-open getRuntimeConfig() fallback and the registration proxy.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. - I could not execute
bun test --isolate srcin this environment, so the PR's test-plan checkboxes are unverified here; the review is static only. - Changeset scoping looks right:
StdioTransport/HttpSseTransport/StreamableHttpTransportconstructors,getServer()andresetWalletProvider()all changed signature, butpackage.jsononly exports.→dist/index.js(i.e.src/index.ts, which exportsmain/runCli/startMcpServer/registerShutdownHandlers). None of the changed signatures are reachable by consumers, sopatchis correct. wallet-isolation.test.ts'sEXPECTED_HTTP_TOOL_NAMESduplicatesREAD_ONLY_TOOL_NAMESfromcore/tools.tsplussearch_docs. The comment says this is deliberate (independent drift detection), which is reasonable for a security surface — just note every new read-only tool now requires a second edit, and a missed update fails the test rather than the policy.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // visible without flooding stderr. | ||
| if (!warnedAboutUnscopedWalletRead) { | ||
| warnedAboutUnscopedWalletRead = true; | ||
| console.error('Wallet configuration was read outside an MCP runtime scope; using the mutable process configuration.'); |
There was a problem hiding this comment.
[suggestion] This fallback is fail-open for the one invariant the package treats as load-bearing: if an ALS scope is ever lost, getPrivateKeyAsHex() / isWalletEnabled() silently serve the current mutable process config instead of the runtime's snapshot.
After this PR every production read is scoped — registration runs inside runWithAppConfig in getServer, and each callback is individually wrapped, which is presumably why index.ts:55 was rewritten to read config.appConfig.walletMode directly rather than call isWalletEnabled(). That makes this branch effectively dead in production, which is exactly when it's cheapest to make it fail closed (throw, or return a walletMode: 'disabled' view) rather than warn.
Separately, warnedAboutUnscopedWalletRead is a module-global latch reset only by initializeConfig(). In a long-running stdio server that calls initializeConfig() once at startup, a scope lost hours later is diagnosed at most once — and if anything else tripped the warning first, never. If you keep the fallback, consider not latching it (or latching per-callsite) so the diagnostic is actually usable.
| console.error('Supported networks:', getSupportedNetworks().join(', ')); | ||
| return (...args: unknown[]) => { | ||
| let callbackIndex = args.length - 1; | ||
| while (callbackIndex >= 0 && typeof args[callbackIndex] !== 'function') callbackIndex--; |
There was a problem hiding this comment.
[suggestion] The backward scan wraps only the last function argument, so callbacks that arrive in a non-final position stay unscoped. Today that's safe — resources.ts constructs every ResourceTemplate with { list: undefined }, and nothing passes a completable() completer — but a ResourceTemplate list/complete callback or a prompt-argument completer added later would silently run against the mutable process config, reopening the exact leak this PR closes, with no test failure to signal it.
Worth either wrapping callbacks found anywhere in args (rather than just the last one), or leaving a comment here stating the assumption that registration callbacks are always terminal.
| constructor() { | ||
| this.privateKey = getPrivateKeyAsHex(); | ||
| constructor(options?: PrivateKeyWalletProviderOptions) { | ||
| this.privateKey = options ? options.privateKey : getPrivateKeyAsHex(); |
There was a problem hiding this comment.
[suggestion] createWalletProvider() now always passes options, so the getPrivateKeyAsHex() fallback only fires for direct new PrivateKeyWalletProvider() calls — i.e. the one remaining path that reads the mutable singleton for key material, which is what this PR is removing everywhere else. Consider making options required (constructor(options: PrivateKeyWalletProviderOptions)); the only caller already supplies it, and the new test at private-key.test.ts already asserts the explicit-options path doesn't touch the singleton.
| */ | ||
| export function resetWalletProvider(): void { | ||
| walletProviderInstance = null; | ||
| export function resetWalletProvider(appConfig: AppConfigSnapshot): void { |
There was a problem hiding this comment.
[nit] Since providersByConfig is a WeakMap keyed by the snapshot, and the snapshot is only reachable from the runtime being stopped, the entry is collectible as soon as stop() returns — and no further request can reach that snapshot anyway. The explicit eviction is defensive rather than load-bearing; the doc comment reading "Evict one runtime's cached provider" slightly oversells it.
If the intent is to drop key material promptly, note this only drops the reference — the evicted PrivateKeyWalletProvider still holds this.privateKey until GC. Zeroing it would need an explicit dispose() on the provider.
|
|
||
| console.error('Supported networks:', getSupportedNetworks().join(', ')); | ||
|
|
||
| return server; |
There was a problem hiding this comment.
[nit] getServer returns the unproxied server, not scopedServer. That's fine for the current callers (they only call connect/close), but any future registration performed on the returned instance would bypass snapshot binding entirely. Returning scopedServer would close that off — the proxy forwards non-registration methods via value.bind(target), so connect/close keep working.
2 participants
Summary
validateSecurityConfig, so later programmatic starts cannot alter an already-running listener's tools or signer.parseArgs()creates a frozen, brandedAppConfigsnapshot; all transports require it explicitly, and every MCP tool/resource/prompt callback captures it at registration.appConfig.walletMode; wallet-enabled snapshots fail closed before listen. Runtime providers are cached by snapshot and evicted only for the stopped runtime; unscoped provider reads are diagnosed and not memoized.Test plan
bun test --isolate src/testsinpackages/mcp-serverinitializeConfig()→ new session still has no wallet toolsstreamable-http/http-ssebefore listenMade with Cursor