feat: Milestone 3 - production HTTP server with dual-mode transport#3
Conversation
Milestone 3 implementation: dual-mode server supporting both stdio (Claude Desktop) and HTTP transports, with a standard MCP Streamable HTTP endpoint for Claude Code and other MCP clients. - Add Fastify HTTP server with dual-mode support (stdio/http/both via SERVER_MODE env) - Add MCP-over-HTTP transport layer with custom REST API (/mcp/tools) - Add standard MCP Streamable HTTP endpoint (/mcp) using StreamableHTTPServerTransport from @modelcontextprotocol/sdk, enabling Claude Code type:http .mcp.json integration - Add per-session McpServer instances with API key propagation via AsyncLocalStorage - Add IP-based rate limiting for /mcp/* endpoints - Add Docker deployment configuration with docker-compose
📝 WalkthroughWalkthroughAdds HTTP and streamable-HTTP transports, request-scoped API-key AsyncLocalStorage, per‑IP dual-window rate limiting, an HTTP tool registry/routes, extracts inline MCP tool handlers into exported handlers, makes server mode configurable, and adds Docker/compose, TS-for-Docker config, env example, and README updates. ChangesHTTP transport, rate limiting, API-key context, and tool HTTP surface
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant HTTP as Fastify HTTP Server
participant RL as RateLimiter
participant AKC as APIKeyContext
participant Tool as Tool Handler
participant Vespr as Vespr API
Client->>HTTP: POST /mcp/tools/:toolName (x-api-key header)
HTTP->>RL: incr/check(clientIp)
alt rate limit exceeded
RL-->>HTTP: blocked + retryAfter
HTTP-->>Client: 429 Too Many Requests
else allowed
RL-->>HTTP: allowed
HTTP->>AKC: apiKeyContext.run(x-api-key, handler)
AKC->>Tool: execute handler (getCurrentApiKey() available)
Tool->>Vespr: call Vespr API with resolved key
Vespr-->>Tool: response
Tool-->>AKC: result (content/structuredContent)
AKC-->>HTTP: return result
HTTP-->>Client: 200 OK + result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Cardano MCP server's architecture by adding support for HTTP and Streamable HTTP transports, allowing it to serve web-based clients like Claude Code. Key implementations include a Fastify server, a custom rate-limiting middleware, and a dynamic API key management system using AsyncLocalStorage. The review feedback identifies a critical security risk regarding permissive CORS settings in production, as well as potential memory and resource leaks within the session management and rate-limiter cleanup logic.
| apiKey?: string; | ||
| } | ||
|
|
||
| const sessions = new Map<string, Session>(); |
There was a problem hiding this comment.
The sessions map is a memory leak risk in a production HTTP server. As sessions are created, they are added to this map but only removed when the transport closes. If a client initiates a session but never closes it, the session will remain in memory indefinitely. Consider implementing a TTL (Time-To-Live) or a cleanup mechanism for idle sessions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
src/tools/get_token_chart.ts (2)
10-10: ⚡ Quick win
CHART_PERIODSis defined but never used.The inputSchema validates period via
ChartPeriodSchema, but the handler itself never referencesCHART_PERIODS. Either use it to validateperiodin the handler (so HTTP-path calls also get proper validation), or remove it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/get_token_chart.ts` at line 10, CHART_PERIODS is unused; either remove it or use it to validate incoming HTTP requests — update the handler (get_token_chart or the function that processes the route) to check the requested period against CHART_PERIODS (or derive ChartPeriodSchema from CHART_PERIODS) before using it, so runtime HTTP-path calls get the same validation as the inputSchema (ChartPeriodSchema); make sure the checked/derived type matches the existing ChartPeriodSchema and update error handling to return a 400 for invalid periods.
70-75: ⚡ Quick winMissing period/currency validation — same gap as
getTokenInfoHandler.
effectivePeriodandeffectiveCurrencyare used without validation. For MCP callers the Zod schema enforces valid values, but for direct/HTTP callers an invalid period or currency is silently forwarded to the repository.CHART_PERIODSalready exists in this file and could be reused for the period check, consistent with the validation pattern ingetWalletBalanceHandler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/get_token_chart.ts` around lines 70 - 75, Validate and normalize the period and currency before calling VesprApiRepository.getTokenChart: check that effectivePeriod is one of CHART_PERIODS (reuse that constant) and that effectiveCurrency is a known SupportedCurrency/CryptoCurrency value; if either is invalid, handle it the same way as getWalletBalanceHandler (return/throw a 400-style validation error or use the same defaulting logic pattern used in getTokenInfoHandler), then pass the validated values into VesprApiRepository.getTokenChart(unit, effectivePeriod as ChartPeriod, effectiveCurrency); update references to effectivePeriod, effectiveCurrency, CHART_PERIODS, getTokenChart and VesprApiRepository.getTokenChart to implement this check.src/tools/get_token_info.ts (1)
64-66: ⚡ Quick winMissing currency validation — inconsistent with
getWalletBalanceHandler.
currencyis cast toSupportedCurrencywithout a whitelist check.getWalletBalanceHandlervalidatesSUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)and returns a clear user error before hitting the repository.getTokenInfoHandler(andgetTokenChartHandler) silently pass an invalid string to the repo, producing an opaque API error instead of a meaningful validation message. Since these handlers are exported and reachable from the HTTP transport, pre-validation matters.♻️ Proposed fix
+ // Validate currency if provided + if (currency && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) { + return { + content: [{ type: "text" as const, text: "Error: Invalid currency. Must be one of the supported currencies." }], + isError: true, + }; + } + // Use default currency if not specified const effectiveCurrency = (currency as SupportedCurrency) ?? FiatCurrency.USD;You'll also need to add
SUPPORTED_CURRENCIESto the import on Line 6.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/get_token_info.ts` around lines 64 - 66, getTokenInfoHandler (and similarly getTokenChartHandler) currently casts currency to SupportedCurrency without whitelisting, causing opaque repo errors; update get_token_info.ts to import SUPPORTED_CURRENCIES and validate the incoming currency string the same way getWalletBalanceHandler does (e.g., if (!SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) return a user-facing validation error), then compute effectiveCurrency = (currency as SupportedCurrency) ?? FiatCurrency.USD only after validation so invalid values are rejected before calling the repository.src/tools/index.ts (1)
44-201: ⚖️ Poor tradeoffHandler type casts to
Record<string, unknown>suppress TypeScript's ability to detect field-name mismatches.The casts such as
getTransactionHistoryHandler as (args: Record<string, unknown>) => Promise<HttpToolResult>are what prevented the compiler from flagging thecount/pagevslimit/offsetmismatch above. Consider definingHttpToolDefinition.handlerwith a typed overload, or using a narrow adapter wrapper per tool rather than a blanket cast, so the type system remains a safety net for future schema changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/index.ts` around lines 44 - 201, Handlers are being cast to (args: Record<string, unknown>) => Promise<HttpToolResult> (e.g., getTransactionHistoryHandler, getWalletBalanceHandler, getTokenInfoHandler), which hides schema/handler shape mismatches; remove the blanket casts and either (A) make HttpToolDefinition.handler generic (e.g., handler<T>(args: T) => Promise<HttpToolResult>) and instantiate the tool with the concrete input type inferred from each z.object schema, or (B) keep handlers strongly typed and register them via a small adapter that runs the zod inputSchema.parse/parseAsync and then calls the typed handler (adapter<T>(schema, handler) => (raw) => handler(schema.parse(raw)) ), then update each registerTool call to use the adapter instead of casting to Record<string, unknown>.src/transports/http.ts (1)
20-23: ⚡ Quick winTool errors are always returned as HTTP 200 —
isErroris not surfaced to HTTP clients.
HttpToolResulthas noisErrorfield, so when a handler returns{ isError: true, content: [{type:"text", text:"Error: ..."}] }, the route at line 209–211 still sends HTTP 200 withresult: [{type:"text",...}]. HTTP clients cannot distinguish a successful result from a tool-level error without inspecting the content text.Consider adding
isErrortoHttpToolResultand mapping it to an appropriate HTTP status (e.g., 502 for upstream failures):♻️ Proposed change
export interface HttpToolResult { content: Array<{ type: string; text: string }>; structuredContent?: Record<string, unknown>; + isError?: boolean; }- return reply.send({ - result: result.structuredContent ?? result.content, - }); + if (result.isError) { + return reply.status(502).send({ + error: { + code: "TOOL_ERROR", + message: result.content.map((c) => c.text).join("\n"), + }, + }); + } + return reply.send({ + result: result.structuredContent ?? result.content, + });Also applies to: 208-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transports/http.ts` around lines 20 - 23, Add an optional isError:boolean to the HttpToolResult interface (export interface HttpToolResult { content: Array<{ type: string; text: string }>; structuredContent?: Record<string, unknown>; isError?: boolean; }) and update the HTTP route handler that returns the tool result (the code that currently always does res.status(200).json({ result })) to map tool errors to an appropriate HTTP status (e.g., if result.isError === true then send res.status(502).json({ error: result, result: result.content }) else send 200). Ensure the handler reads isError from the HttpToolResult returned by tools so HTTP clients can distinguish upstream/tool failures from successful responses.src/transports/streamableHttp.ts (1)
55-61: ⚡ Quick winDead code:
transport.sessionIdis alwaysundefinedat construction time.
StreamableHTTPServerTransportgenerates and assigns its session ID only during the firsthandleRequestinvocation (through theonsessioninitializedcallback, already wired at line 28–31). At the point lines 57–59 execute,transport.sessionIdis invariablyundefined, sosessions.set(…)there is never reached. Theonsessioninitializedcallback is the sole registration path. Remove the redundant block to avoid misleading future readers.♻️ Proposed cleanup
const session = { transport, apiKey }; - if (transport.sessionId) { - sessions.set(transport.sessionId, session); - } - return { session, isNew: true };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transports/streamableHttp.ts` around lines 55 - 61, The check-and-set block that tests transport.sessionId and calls sessions.set(...) is dead because StreamableHTTPServerTransport only assigns transport.sessionId during handleRequest via the onsessioninitialized callback; remove the redundant lines that reference transport.sessionId and sessions.set from the function that returns { session, isNew: true } so the only session registration path remains the onsessioninitialized handler in StreamableHTTPServerTransport.src/transports/http.test.ts (1)
161-165: 💤 Low valueLGTM — test isolation via internal field access is acceptable for now.
The
httpToolRegistry["tools"].delete(…)approach works, but it couples tests to the private implementation. IfHttpToolRegistrygrows or its internals change, tests will silently break. Consider adding aclear()/reset()method toHttpToolRegistryin a follow-up to make test teardown contract-based rather than implementation-coupled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transports/http.test.ts` around lines 161 - 165, Tests currently clear the registry by accessing the private internal field via httpToolRegistry["tools"].delete(...), coupling tests to implementation; add a public method (e.g., HttpToolRegistry.clear() or reset()) on the HttpToolRegistry class that removes all registered tools (internally clearing the same collection used by getAllTools()), then update the test teardown to call httpToolRegistry.clear() instead of touching the private "tools" property so tests rely on the class contract rather than its internals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 8-16: The docker-compose environment block is missing
VESPR_API_KEY so the container won't receive the host-provided API key; add an
entry to the environment list (same block with the other vars) that forwards the
host variable into the container (e.g., VESPR_API_KEY=${VESPR_API_KEY}) so the
container can access the key at runtime; update the environment section where
SERVER_MODE/HTTP_PORT etc. are declared to include the new VESPR_API_KEY entry.
In `@Dockerfile`:
- Around line 7-9: ARG NPM_TOKEN is leaking the secret into the image history;
remove the ARG and switch to BuildKit secret mounts for npm auth instead: stop
writing a token into .npmrc via build-time ARG, and update the RUN that performs
dependency install (the RUN invoking "npm ci --ignore-scripts && rm -f .npmrc")
to use a BuildKit secret mount for the .npmrc (or credentials) so the token
never appears in image layers; apply the same change to the runtime stage's
copy/install steps referenced around the COPY/package*.json and the later lines
handling .npmrc (the occurrences of COPY, ARG NPM_TOKEN, and RUN npm ci) and
ensure builds use BuildKit (DOCKER_BUILDKIT=1).
In `@src/index.ts`:
- Around line 38-41: startHttpTransport currently discards the FastifyInstance
returned by startServer(), so signal handlers cannot gracefully close
connections; change startHttpTransport to capture the returned FastifyInstance
(e.g., const server = await startServer()), register SIGINT/SIGTERM handlers
that await server.close() (or call server.close(cb) wrapped in a promise) and
only then exit the process, and replicate the same pattern for the other startup
block (lines ~62-70) that also starts a server; also add a short timeout
fallback to force exit if close hangs and ensure any errors from server.close()
are logged by the process logger.
In `@src/middleware/rateLimit.test.ts`:
- Around line 34-45: The test currently hits maxRequestsPerMinute before
exercising the maxRequestsPerDay path: when calling limiter.checkLimit
repeatedly the per-minute limit (maxRequestsPerMinute) blocks further increments
so the daily counter never reaches maxRequestsPerDay; update the test in
rateLimit.test.ts to perform the calls in two groups separated by advancing the
minute window (e.g., use Jest fake timers and advance time by >60,000ms or
otherwise reset the per-minute window) so that the first group's calls consume
the minute quota, time is advanced so the minute window resets, then additional
calls push the daily counter past maxRequestsPerDay and trigger the daily-limit
branch in limiter.checkLimit; reference limiter.checkLimit, maxRequestsPerMinute
and maxRequestsPerDay when making the change.
In `@src/middleware/rateLimit.ts`:
- Around line 93-100: getClientIp currently trusts the x-forwarded-for header
unconditionally (request.headers["x-forwarded-for"]) which allows spoofing;
update getClientIp to only read x-forwarded-for when the incoming connection is
from a trusted proxy (e.g., check request.ip against a configured trusted CIDR
list or use Fastify's `@fastify/forwarded-proxy`) and otherwise return request.ip;
ensure the trusted-proxy check is configurable and documented, and keep
getClientIp as the single place enforcing this logic.
In `@src/server.ts`:
- Around line 33-37: Update the CORS registration on the Fastify server: add
"DELETE" to the methods array used in server.register(cors), add "x-api-key" and
"mcp-session-id" to the allowedHeaders array so preflight requests with those
headers are permitted, and add an exposedHeaders array containing
"mcp-session-id" so browser clients can read that response header; locate this
change in the server.register(cors, {...}) block in src/server.ts.
- Around line 69-73: The rate-limiting hook only matches paths starting with
"/mcp/" and thus misses exact "/mcp" requests; update the onRequest guard in
server.addHook to use request.url.startsWith("/mcp") (instead of "/mcp/") so
rateLimitHook(request, reply) runs for "/mcp", "/mcp/", "/mcp/tools", "/mcp?..."
etc.; ensure this change is made where request.url.startsWith is called so all
MCP endpoints invoke rateLimitHook.
In `@src/tools/get_wallet_balance.ts`:
- Around line 118-126: Remove the debug JSON.stringify call from the returned
human-readable content: in the return object (where properties content and
structuredContent are set) stop appending JSON.stringify(output, null, 2) to the
text field and instead set the content text to just textSummary (or textSummary
+ "\n\n" if spacing is desired); keep structuredContent: output intact so LLM
clients receive the structured data via structuredContent. This change affects
the return in get_wallet_balance (variables textSummary, output, and the
content/structuredContent properties).
In `@src/tools/index.ts`:
- Around line 136-156: The HTTP tool registration for get_trending_tokens is
missing the optional period input; update the inputSchema in the
httpToolRegistry.registerTool call for "get_trending_tokens" to include a period
field that mirrors the MCP registration and handler: use the same preprocess
pattern as limit/currency (coerce null/"" to undefined) and validate against the
existing period enum/constant used by
registerGetTrendingTokens/getTrendingTokensHandler (e.g., SUPPORTED_PERIODS or
the Period enum), with the same default/optional behavior as the MCP variant so
HTTP callers can filter by time window.
- Around line 65-83: The HTTP schema for the get_transaction_history tool
exposes count/page but getTransactionHistoryHandler expects limit/offset, so
requests end up with undefined pagination; fix this by adding a thin wrapper
around getTransactionHistoryHandler in the tool registration that maps validated
args.count and args.page to the handler's expected args.limit and args.offset
(compute offset = (page - 1) * limit, defaulting to handler defaults when
count/page are absent), and ensure the registered handler signature still
returns Promise<HttpToolResult>; alternatively rename the schema fields to
limit/offset to match getTransactionHistoryHandler if you prefer a field rename
instead of a wrapper.
In `@src/transports/streamableHttp.ts`:
- Line 9: The VERSION constant in streamableHttp.ts is hardcoded to "0.1.1" and
is out of sync with the stdio-mode server; replace the hardcoded const VERSION
with a single source of truth (either import the version from package.json or
import a shared exported constant used by src/index.ts/src/server.ts) so both
stdio McpServer and per-session HTTP McpServer use the same value; locate the
const VERSION in streamableHttp.ts and change it to read the version dynamically
(e.g., from package.json.version or from the shared export) and update any
related exports/usage to reference that shared symbol instead of the literal.
---
Nitpick comments:
In `@src/tools/get_token_chart.ts`:
- Line 10: CHART_PERIODS is unused; either remove it or use it to validate
incoming HTTP requests — update the handler (get_token_chart or the function
that processes the route) to check the requested period against CHART_PERIODS
(or derive ChartPeriodSchema from CHART_PERIODS) before using it, so runtime
HTTP-path calls get the same validation as the inputSchema (ChartPeriodSchema);
make sure the checked/derived type matches the existing ChartPeriodSchema and
update error handling to return a 400 for invalid periods.
- Around line 70-75: Validate and normalize the period and currency before
calling VesprApiRepository.getTokenChart: check that effectivePeriod is one of
CHART_PERIODS (reuse that constant) and that effectiveCurrency is a known
SupportedCurrency/CryptoCurrency value; if either is invalid, handle it the same
way as getWalletBalanceHandler (return/throw a 400-style validation error or use
the same defaulting logic pattern used in getTokenInfoHandler), then pass the
validated values into VesprApiRepository.getTokenChart(unit, effectivePeriod as
ChartPeriod, effectiveCurrency); update references to effectivePeriod,
effectiveCurrency, CHART_PERIODS, getTokenChart and
VesprApiRepository.getTokenChart to implement this check.
In `@src/tools/get_token_info.ts`:
- Around line 64-66: getTokenInfoHandler (and similarly getTokenChartHandler)
currently casts currency to SupportedCurrency without whitelisting, causing
opaque repo errors; update get_token_info.ts to import SUPPORTED_CURRENCIES and
validate the incoming currency string the same way getWalletBalanceHandler does
(e.g., if (!SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) return
a user-facing validation error), then compute effectiveCurrency = (currency as
SupportedCurrency) ?? FiatCurrency.USD only after validation so invalid values
are rejected before calling the repository.
In `@src/tools/index.ts`:
- Around line 44-201: Handlers are being cast to (args: Record<string, unknown>)
=> Promise<HttpToolResult> (e.g., getTransactionHistoryHandler,
getWalletBalanceHandler, getTokenInfoHandler), which hides schema/handler shape
mismatches; remove the blanket casts and either (A) make
HttpToolDefinition.handler generic (e.g., handler<T>(args: T) =>
Promise<HttpToolResult>) and instantiate the tool with the concrete input type
inferred from each z.object schema, or (B) keep handlers strongly typed and
register them via a small adapter that runs the zod inputSchema.parse/parseAsync
and then calls the typed handler (adapter<T>(schema, handler) => (raw) =>
handler(schema.parse(raw)) ), then update each registerTool call to use the
adapter instead of casting to Record<string, unknown>.
In `@src/transports/http.test.ts`:
- Around line 161-165: Tests currently clear the registry by accessing the
private internal field via httpToolRegistry["tools"].delete(...), coupling tests
to implementation; add a public method (e.g., HttpToolRegistry.clear() or
reset()) on the HttpToolRegistry class that removes all registered tools
(internally clearing the same collection used by getAllTools()), then update the
test teardown to call httpToolRegistry.clear() instead of touching the private
"tools" property so tests rely on the class contract rather than its internals.
In `@src/transports/http.ts`:
- Around line 20-23: Add an optional isError:boolean to the HttpToolResult
interface (export interface HttpToolResult { content: Array<{ type: string;
text: string }>; structuredContent?: Record<string, unknown>; isError?: boolean;
}) and update the HTTP route handler that returns the tool result (the code that
currently always does res.status(200).json({ result })) to map tool errors to an
appropriate HTTP status (e.g., if result.isError === true then send
res.status(502).json({ error: result, result: result.content }) else send 200).
Ensure the handler reads isError from the HttpToolResult returned by tools so
HTTP clients can distinguish upstream/tool failures from successful responses.
In `@src/transports/streamableHttp.ts`:
- Around line 55-61: The check-and-set block that tests transport.sessionId and
calls sessions.set(...) is dead because StreamableHTTPServerTransport only
assigns transport.sessionId during handleRequest via the onsessioninitialized
callback; remove the redundant lines that reference transport.sessionId and
sessions.set from the function that returns { session, isNew: true } so the only
session registration path remains the onsessioninitialized handler in
StreamableHTTPServerTransport.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86b4b0cf-dc69-48fa-80a8-612f11e3c72a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.dockerignore.env.example.npmrc.prettierignoreDockerfileREADME.mddocker-compose.ymlpackage.jsonscripts/stress-test.tssrc/api/VesprApiClient.test.tssrc/api/VesprApiClient.tssrc/config.tssrc/index.tssrc/middleware/rateLimit.test.tssrc/middleware/rateLimit.tssrc/server.tssrc/tools/get_asset_metadata.tssrc/tools/get_asset_summary.tssrc/tools/get_pool_info.tssrc/tools/get_staking_info.tssrc/tools/get_supported_currencies.tssrc/tools/get_token_chart.tssrc/tools/get_token_info.tssrc/tools/get_transaction_history.tssrc/tools/get_trending_tokens.tssrc/tools/get_wallet_balance.tssrc/tools/index.tssrc/tools/resolve_ada_handle.tssrc/transports/http.test.tssrc/transports/http.tssrc/transports/streamableHttp.tssrc/utils/apiKeyContext.tstsconfig.docker.json
| // get_transaction_history | ||
| httpToolRegistry.registerTool({ | ||
| name: "get_transaction_history", | ||
| title: "Get Transaction History", | ||
| description: "Query Cardano wallet transaction history with pagination support", | ||
| inputSchema: z.object({ | ||
| address: z.string().describe("Cardano wallet address (bech32 format, addr1...)"), | ||
| count: z | ||
| .preprocess( | ||
| (val) => (val === null || val === "" ? undefined : val), | ||
| z.number().int().min(1).max(100).default(20), | ||
| ) | ||
| .describe("Number of transactions to return (1-100)"), | ||
| page: z | ||
| .preprocess((val) => (val === null || val === "" ? undefined : val), z.number().int().min(1).default(1)) | ||
| .describe("Page number for pagination"), | ||
| }), | ||
| handler: getTransactionHistoryHandler as (args: Record<string, unknown>) => Promise<HttpToolResult>, | ||
| }); |
There was a problem hiding this comment.
get_transaction_history HTTP schema fields count/page don't match handler parameters limit/offset — pagination is silently broken.
getTransactionHistoryHandler destructures { address, limit, offset } (see src/tools/get_transaction_history.ts:53–61), but the HTTP input schema exposes count and page. At runtime, validated args { address, count, page } are passed to the handler, where limit and offset resolve to undefined — the handler ignores the caller's page size and page number and always uses its own defaults.
The type cast on line 82 (as (args: Record<string, unknown>) => Promise<HttpToolResult>) silences the TypeScript error that would have caught this mismatch.
🐛 Option A — rename schema fields to match the handler
inputSchema: z.object({
address: z.string().describe("Cardano wallet address (bech32 format, addr1...)"),
- count: z
+ limit: z
.preprocess(
(val) => (val === null || val === "" ? undefined : val),
z.number().int().min(1).max(100).default(20),
)
- .describe("Number of transactions to return (1-100)"),
- page: z
+ .describe("Max transactions to return (1-100)"),
+ offset: z
.preprocess((val) => (val === null || val === "" ? undefined : val), z.number().int().min(1).default(1))
- .describe("Page number for pagination"),
+ .describe("Offset for pagination"),
}),🐛 Option B — map count/page to limit/offset in a wrapper
- handler: getTransactionHistoryHandler as (args: Record<string, unknown>) => Promise<HttpToolResult>,
+ handler: ({ address, count, page }: Record<string, unknown>) =>
+ getTransactionHistoryHandler({
+ address: address as string,
+ limit: count as number | undefined,
+ offset: page as number | undefined,
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // get_transaction_history | |
| httpToolRegistry.registerTool({ | |
| name: "get_transaction_history", | |
| title: "Get Transaction History", | |
| description: "Query Cardano wallet transaction history with pagination support", | |
| inputSchema: z.object({ | |
| address: z.string().describe("Cardano wallet address (bech32 format, addr1...)"), | |
| count: z | |
| .preprocess( | |
| (val) => (val === null || val === "" ? undefined : val), | |
| z.number().int().min(1).max(100).default(20), | |
| ) | |
| .describe("Number of transactions to return (1-100)"), | |
| page: z | |
| .preprocess((val) => (val === null || val === "" ? undefined : val), z.number().int().min(1).default(1)) | |
| .describe("Page number for pagination"), | |
| }), | |
| handler: getTransactionHistoryHandler as (args: Record<string, unknown>) => Promise<HttpToolResult>, | |
| }); | |
| // get_transaction_history | |
| httpToolRegistry.registerTool({ | |
| name: "get_transaction_history", | |
| title: "Get Transaction History", | |
| description: "Query Cardano wallet transaction history with pagination support", | |
| inputSchema: z.object({ | |
| address: z.string().describe("Cardano wallet address (bech32 format, addr1...)"), | |
| limit: z | |
| .preprocess( | |
| (val) => (val === null || val === "" ? undefined : val), | |
| z.number().int().min(1).max(100).default(20), | |
| ) | |
| .describe("Max transactions to return (1-100)"), | |
| offset: z | |
| .preprocess((val) => (val === null || val === "" ? undefined : val), z.number().int().min(1).default(1)) | |
| .describe("Offset for pagination"), | |
| }), | |
| handler: getTransactionHistoryHandler as (args: Record<string, unknown>) => Promise<HttpToolResult>, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/index.ts` around lines 65 - 83, The HTTP schema for the
get_transaction_history tool exposes count/page but getTransactionHistoryHandler
expects limit/offset, so requests end up with undefined pagination; fix this by
adding a thin wrapper around getTransactionHistoryHandler in the tool
registration that maps validated args.count and args.page to the handler's
expected args.limit and args.offset (compute offset = (page - 1) * limit,
defaulting to handler defaults when count/page are absent), and ensure the
registered handler signature still returns Promise<HttpToolResult>;
alternatively rename the schema fields to limit/offset to match
getTransactionHistoryHandler if you prefer a field rename instead of a wrapper.
- docker-compose.yml: propagate VESPR_API_KEY into container environment - Dockerfile: replace ARG NPM_TOKEN with BuildKit secret mount to avoid leaking token in image history - src/index.ts: capture FastifyInstance and close it gracefully on SIGINT/SIGTERM - src/server.ts: add DELETE to CORS methods, expose x-api-key/mcp-session-id headers, fix rate limit hook to match exact /mcp path - src/tools/get_wallet_balance.ts: remove debug JSON.stringify from content text - src/tools/get_token_info.ts: add currency validation matching getWalletBalanceHandler pattern - src/tools/get_token_chart.ts: remove unused CHART_PERIODS constant - src/tools/index.ts: fix get_transaction_history HTTP schema (count/page → to_block), fix get_token_chart HTTP schema (range → period + add currency), add period to get_trending_tokens - src/transports/http.ts: add isError to HttpToolResult, return 502 on tool errors, add clear() to HttpToolRegistry - src/transports/streamableHttp.ts: remove dead transport.sessionId block, sync VERSION to 0.1.0 - src/transports/http.test.ts: use httpToolRegistry.clear() instead of private field access - src/middleware/rateLimit.test.ts: fix per-day limit test to actually exercise daily code path
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Dockerfile (3)
3-3: 💤 Low value
npm install -g npm@11.7.0is duplicated across both stages and inflates the runtime image.The global npm upgrade is needed in the
builderstage fornpm ci+npx tsc, but in the runtime stagenpmis only used for the singlenpm cicall. After that it is never invoked again (the CMD isnode, notnpm). Removing the global upgrade in the runtime stage, or relying on the version already bundled withnode:22.22.0-alpine, would shrink the final image and remove one network call.Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 3, The Dockerfile contains a duplicated global npm upgrade (RUN npm install -g npm@11.7.0) in both the builder and runtime stages; remove the RUN npm install -g npm@11.7.0 from the runtime stage so only the builder performs the global npm upgrade (keep it in the builder stage where npx/tsc/npm ci need it) and rely on the node:22.22.0-alpine bundled npm for the runtime stage's single npm ci, which will shrink the final image and eliminate the extra network call.
15-17: 💤 Low valueAdd an
EXPOSEinstruction to document the HTTP port.The runtime stage has no
EXPOSE. Without it, tooling (docker-compose, Kubernetes, IDE integrations) cannot auto-discover the service port, anddocker run -Pwon't map anything.📄 Proposed addition
FROM node:22.22.0-alpine RUN npm install -g npm@11.7.0 WORKDIR /app + +EXPOSE 3000🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 15 - 17, The Dockerfile is missing an EXPOSE instruction documenting the container HTTP port; update the Dockerfile (runtime stage) to add an EXPOSE line for your service port (e.g., EXPOSE 3000 or EXPOSE 8080) so tooling and `docker run -P` can auto-discover and map the port—add the EXPOSE statement alongside the existing base image and npm install lines in the Dockerfile.
29-29: ⚡ Quick winAdd
ENV SERVER_MODE=httpto Dockerfile for proper containerized defaults.The
parseServerMode()function defaults to"stdio"for backward compatibility (src/config.ts:17). When the container runs without explicit environment variables (plaindocker run), it will start in stdio mode—no HTTP server, no/healthendpoint. Whiledocker-compose.ymlexplicitly setsSERVER_MODE=http, a direct Docker invocation would fail the healthcheck or lack the HTTP listener. Set a sensible default in the Dockerfile for standard containerized deployments.Proposed fix
USER node + +ENV SERVER_MODE=http CMD ["node", "dist/index.js"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 29, The Dockerfile should set SERVER_MODE to http so the container defaults to running the HTTP server; add an ENV SERVER_MODE=http declaration above the existing CMD ["node","dist/index.js"] to override the code's parseServerMode() default of "stdio" when no env is supplied, ensuring the app (parseServerMode()) runs the HTTP mode and exposes the /health endpoint in plain docker run scenarios.src/transports/http.test.ts (1)
350-382: ⚡ Quick winAdd a test case for the
result.isError === true/ 502 response path.The existing execution-error test (
"should handle tool execution errors") covers the thrown-exception path (→ 500EXECUTION_ERROR), but the distinctresult.isError === truebranch inhttp.ts(lines 216–223) that returns 502TOOL_ERRORhas no coverage. A regression in that branch would go undetected.✅ Suggested test case to add
+ it("should return 502 when tool handler returns isError: true", async () => { + httpToolRegistry.registerTool({ + name: "failing_tool", + title: "Failing Tool", + description: "Returns an isError result", + inputSchema: {}, + handler: async () => ({ + content: [{ type: "text", text: "Upstream API unavailable" }], + isError: true, + }), + }); + + const route = registeredRoutes.get("POST /mcp/tools/:toolName"); + + const mockRequest = { + params: { toolName: "failing_tool" }, + body: { arguments: {} }, + }; + const mockReply = { + send: jest.fn(), + status: jest.fn().mockReturnThis(), + }; + + await route!.handler(mockRequest, mockReply); + + expect(mockReply.status).toHaveBeenCalledWith(502); + expect(mockReply.send).toHaveBeenCalledWith({ + error: { + code: "TOOL_ERROR", + message: "Upstream API unavailable", + }, + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transports/http.test.ts` around lines 350 - 382, Add a unit test that covers the code path where a tool handler returns a Result-like object with result.isError === true so the HTTP handler returns 502 TOOL_ERROR; specifically, register a tool via httpToolRegistry.registerTool (e.g., name "tool_returns_error") whose handler returns { isError: true, error: { message: "tool failed" } }, then locate the POST /mcp/tools/:toolName route from registeredRoutes and invoke its handler with params { toolName: "tool_returns_error" } and a mock reply; assert mockReply.status was called with 502 and mockReply.send was called with the TOOL_ERROR payload (code: "TOOL_ERROR", message matching the returned error) to cover the result.isError branch in http.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/get_token_info.ts`:
- Around line 64-71: The currency check currently treats empty string as falsy
and bypasses validation, and the nullish coalescing (??) then preserves ""
instead of defaulting to FiatCurrency.USD; update the validation to explicitly
treat empty string as missing (e.g., check currency !== undefined && currency
!== null && currency !== "") before membership testing against
SUPPORTED_CURRENCIES and compute effectiveCurrency by using a conditional that
falls back to FiatCurrency.USD when currency is ""/null/undefined; apply this
change where effectiveCurrency is derived and ensure callers (and
VesprApiRepository.getTokenInfo) always receive a valid SupportedCurrency.
- Around line 119-133: The error paths in get_token_info omit structuredContent
while the tool declares outputSchema (tokenInfoOutputSchema), causing SDK
validation to fail; either remove tokenInfoOutputSchema from the tool
registration, or update every error return (all places that return { content,
isError: true } — e.g., the catch block in getTokenInfo and the earlier
error-return sites) to include a minimal structuredContent object that satisfies
tokenInfoOutputSchema so the MCP SDK validation passes even for isError
responses.
In `@src/transports/http.ts`:
- Around line 65-77: The GET /mcp/tools response currently exposes raw Zod
internals because listTools() returns tool.inputSchema.shape; change listTools()
to convert the Zod schemas to JSON Schema using z.toJSONSchema() instead of
exposing .shape: for each tool, detect if tool.inputSchema is a Zod schema and
call tool.inputSchema.toJSONSchema({ unrepresentable: "any" }) (or if
inputSchema is a ZodObject you can call toJSONSchema on it) and return that JSON
Schema object as inputSchema; keep the rest of the returned fields (name, title,
description) unchanged so consumers receive valid JSON Schema with field
descriptions.
In `@src/transports/streamableHttp.ts`:
- Around line 44-59: Change getOrCreateSession to be async and await
McpServer.connect(transport) inside a try/catch: construct McpServer, call
registerTools, then await mcpServer.connect(transport); on success create the
Session object, store it into the sessions map, and return { session, isNew:
true }; on connect failure log the error and rethrow or return a failure so the
caller won't proceed. Update the POST handler to await getOrCreateSession and
only call StreamableHTTPServerTransport.handleRequest(...) if a valid connected
session was returned (handle/propagate errors from connect instead of
fire-and-forget). Use the function/class names mcpServer, getOrCreateSession,
McpServer.connect, StreamableHTTPServerTransport.handleRequest and the sessions
map to locate and modify the code.
---
Nitpick comments:
In `@Dockerfile`:
- Line 3: The Dockerfile contains a duplicated global npm upgrade (RUN npm
install -g npm@11.7.0) in both the builder and runtime stages; remove the RUN
npm install -g npm@11.7.0 from the runtime stage so only the builder performs
the global npm upgrade (keep it in the builder stage where npx/tsc/npm ci need
it) and rely on the node:22.22.0-alpine bundled npm for the runtime stage's
single npm ci, which will shrink the final image and eliminate the extra network
call.
- Around line 15-17: The Dockerfile is missing an EXPOSE instruction documenting
the container HTTP port; update the Dockerfile (runtime stage) to add an EXPOSE
line for your service port (e.g., EXPOSE 3000 or EXPOSE 8080) so tooling and
`docker run -P` can auto-discover and map the port—add the EXPOSE statement
alongside the existing base image and npm install lines in the Dockerfile.
- Line 29: The Dockerfile should set SERVER_MODE to http so the container
defaults to running the HTTP server; add an ENV SERVER_MODE=http declaration
above the existing CMD ["node","dist/index.js"] to override the code's
parseServerMode() default of "stdio" when no env is supplied, ensuring the app
(parseServerMode()) runs the HTTP mode and exposes the /health endpoint in plain
docker run scenarios.
In `@src/transports/http.test.ts`:
- Around line 350-382: Add a unit test that covers the code path where a tool
handler returns a Result-like object with result.isError === true so the HTTP
handler returns 502 TOOL_ERROR; specifically, register a tool via
httpToolRegistry.registerTool (e.g., name "tool_returns_error") whose handler
returns { isError: true, error: { message: "tool failed" } }, then locate the
POST /mcp/tools/:toolName route from registeredRoutes and invoke its handler
with params { toolName: "tool_returns_error" } and a mock reply; assert
mockReply.status was called with 502 and mockReply.send was called with the
TOOL_ERROR payload (code: "TOOL_ERROR", message matching the returned error) to
cover the result.isError branch in http.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8037e18-54d8-436f-88e6-98fb76bdec0a
📒 Files selected for processing (15)
.env.exampleDockerfiledocker-compose.ymlsrc/config.tssrc/index.tssrc/middleware/rateLimit.test.tssrc/middleware/rateLimit.tssrc/server.tssrc/tools/get_token_chart.tssrc/tools/get_token_info.tssrc/tools/get_wallet_balance.tssrc/tools/index.tssrc/transports/http.test.tssrc/transports/http.tssrc/transports/streamableHttp.ts
✅ Files skipped from review due to trivial changes (1)
- src/middleware/rateLimit.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/index.ts
- src/middleware/rateLimit.test.ts
- docker-compose.yml
- src/server.ts
- src/tools/index.ts
| if (currency && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) { | ||
| return { | ||
| content: [{ type: "text" as const, text: "Error: Invalid currency. Must be one of the supported currencies." }], | ||
| isError: true, | ||
| }; | ||
| } | ||
|
|
||
| const effectiveCurrency = (currency as SupportedCurrency) ?? FiatCurrency.USD; |
There was a problem hiding this comment.
Empty-string currency bypasses validation and ?? won't default it.
if (currency && ...) short-circuits when currency = "" (falsy), skipping SUPPORTED_CURRENCIES membership check. Then "" ?? FiatCurrency.USD stays "" because ?? only falls back on null/undefined — so "" is forwarded to VesprApiRepository.getTokenInfo.
The MCP path is safe (the z.preprocess in inputSchema converts "" → undefined → default(FiatCurrency.USD)), but direct callers of the exported handler can trigger this.
🐛 Proposed fix
- if (currency && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) {
+ if (currency != null && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) {- const effectiveCurrency = (currency as SupportedCurrency) ?? FiatCurrency.USD;
+ const effectiveCurrency = (currency || undefined as SupportedCurrency | undefined) ?? FiatCurrency.USD;Or more concisely, collapse both steps:
- if (currency && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) {
- return {
- content: [{ type: "text" as const, text: "Error: Invalid currency. Must be one of the supported currencies." }],
- isError: true,
- };
- }
-
- const effectiveCurrency = (currency as SupportedCurrency) ?? FiatCurrency.USD;
+ if (currency !== undefined && currency !== "" && !SUPPORTED_CURRENCIES.includes(currency as SupportedCurrency)) {
+ return {
+ content: [{ type: "text" as const, text: "Error: Invalid currency. Must be one of the supported currencies." }],
+ isError: true,
+ };
+ }
+
+ const effectiveCurrency = (currency as SupportedCurrency) || FiatCurrency.USD;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/get_token_info.ts` around lines 64 - 71, The currency check
currently treats empty string as falsy and bypasses validation, and the nullish
coalescing (??) then preserves "" instead of defaulting to FiatCurrency.USD;
update the validation to explicitly treat empty string as missing (e.g., check
currency !== undefined && currency !== null && currency !== "") before
membership testing against SUPPORTED_CURRENCIES and compute effectiveCurrency by
using a conditional that falls back to FiatCurrency.USD when currency is
""/null/undefined; apply this change where effectiveCurrency is derived and
ensure callers (and VesprApiRepository.getTokenInfo) always receive a valid
SupportedCurrency.
| return { | ||
| content: [{ type: "text" as const, text: summary }], | ||
| structuredContent: output, | ||
| }; | ||
| } catch (error) { | ||
| if (error instanceof VesprApiError) { | ||
| return { | ||
| content: [{ type: "text" as const, text: `Error: ${error.message}` }], | ||
| isError: true, | ||
| }; | ||
| } | ||
| return { | ||
| content: [{ type: "text" as const, text: `Error: ${error instanceof Error ? error.message : "Unknown error"}` }], | ||
| isError: true, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check the installed SDK version
cat node_modules/@modelcontextprotocol/sdk/package.json | jq '.version'
# 2. Search for the fix: the SDK should skip structuredContent validation when isError is true
rg -n "isError" node_modules/@modelcontextprotocol/sdk/dist --include="*.js" -A 3 -B 3 | head -80Repository: vespr-wallet/cardano_mcp
Length of output: 594
🏁 Script executed:
# Check package.json for SDK version
cat package.json | grep -A 5 "@modelcontextprotocol/sdk"Repository: vespr-wallet/cardano_mcp
Length of output: 212
🏁 Script executed:
# Read the actual file mentioned in the review
cat -n src/tools/get_token_info.ts | head -150Repository: vespr-wallet/cardano_mcp
Length of output: 6214
🏁 Script executed:
# Look for any documentation or configuration related to SDK version
fd -e json -e md | grep -E "(package|README|sdk)" | head -20Repository: vespr-wallet/cardano_mcp
Length of output: 109
🏁 Script executed:
# Read the rest of get_token_info.ts
cat -n src/tools/get_token_info.ts | tail -n +150Repository: vespr-wallet/cardano_mcp
Length of output: 485
🏁 Script executed:
# Search for any references to issue `#654` or SDK structuredContent validation in the repo
rg -i "#654|structuredContent.*validation" --type ts --type mdRepository: vespr-wallet/cardano_mcp
Length of output: 50
🌐 Web query:
modelcontextprotocol SDK 1.0.0 structuredContent validation isError
💡 Result:
In the Model Context Protocol (MCP) SDK version 1.0.0 (confirmed as the initial stable release for TypeScript, Python, and Go SDKs [1][2][3]), the CallToolResult interface includes optional fields: content (ContentBlock[]), structuredContent ({ [key: string]: unknown }), and isError (boolean, default false indicating tool call error) [4]. Tools may define an outputSchema (JSON Schema) for validating structuredContent in successful responses [5][6]. Per spec, servers MUST conform structured results to outputSchema; clients SHOULD validate [5]. Tool execution errors (e.g., API failures, invalid input) are reported via isError: true with descriptive content, without requiring structuredContent validation [5][6][7]. A known issue in TypeScript SDK (and derivatives): client.callTool validates structuredContent against outputSchema even if isError: true, potentially throwing McpError and blocking error delivery (asymmetric with server-side validateToolOutput, which skips on isError) [8][9][10]. Fixes via PRs (e.g., add!result.isError guard [8], skip validation [9]) exist but issues persist into 2026 [2 published 2026-04-22]. Server-side properly skips validation on isError [8][9]. For error reporting, set isError: true and use content for text; avoid structuredContent or ensure it passes schema if present [6 example]. No SDK-specific 1.0.0 changes to this behavior found; aligns with spec since initial release [1].
Citations:
- 1: https://newreleases.io/project/github/modelcontextprotocol/typescript-sdk/release/1.0.0
- 2: https://newreleases.io/project/github/modelcontextprotocol/python-sdk/release/v1.0.0
- 3: https://github.com/modelcontextprotocol/go-sdk/releases/tag/v1.0.0
- 4: https://modelcontextprotocol.io/specification/2025-06-18/schema
- 5: https://modelcontextprotocol.io/specification/draft/server/tools
- 6: https://modelcontextprotocol.io/specification/2025-06-18/server/tools
- 7: https://csharp.sdk.modelcontextprotocol.io/api/ModelContextProtocol.Protocol.ToolResultContentBlock.html
- 8: client.callTool validates structuredContent against outputSchema even when isError is true (asymmetric with server) modelcontextprotocol/typescript-sdk#1943
- 9: Missing or wrong structuredContent blocks tools from reporting error modelcontextprotocol/typescript-sdk#654
- 10: [BUG]: outputSchema validation not skipped for isError:true tool results, hiding actual error messages IBM/mcp-context-forge#4135
🌐 Web query:
MCP SDK issue 654 structuredContent outputSchema validation
💡 Result:
MCP SDK issue #654 in the modelcontextprotocol/typescript-sdk repository describes a bug where server-side validation of structuredContent against a tool's outputSchema prevents tools from reporting errors properly. When a tool returns { content: [...], isError: true }, the SDK throws a validation error before checking isError, blocking the error from reaching the client.[1] The issue was reported on 2025-06-18. It references PR #655 ("fix: skip validation if tool reports error"), which likely resolves it on the server side.[1] Related client-side asymmetry issues exist (e.g., #1943, #699, #26), where validation occurs even for isError: true responses, but #654 focuses on server blocking.[1][2][3] Per the MCP spec, tools with outputSchema must return conforming structuredContent, but error responses (isError: true) should skip validation.[4] Workarounds include omitting structuredContent or outputSchema for error-prone tools, or handling errors outside the tool handler.[1][4] Status: Open as of latest data (mentions in 2026 issues/PRs like #2429, PR #1690), but fix likely in recent SDK versions post-PR #655. Update to latest @modelcontextprotocol/typescript-sdk and test error flows.[1][2]
Citations:
- 1: Missing or wrong structuredContent blocks tools from reporting error modelcontextprotocol/typescript-sdk#654
- 2: client.callTool validates structuredContent against outputSchema even when isError is true (asymmetric with server) modelcontextprotocol/typescript-sdk#1943
- 3: McpServer class using an outputSchema gobbles thrown errors from tool calls modelcontextprotocol/typescript-sdk#699
- 4: https://modelcontextprotocol.io/specification/draft/server/tools
Error paths omit structuredContent — the SDK validates against outputSchema even when isError: true is set.
When a tool declares outputSchema, the MCP SDK validates structuredContent against that schema even for error responses. All four error-return sites (lines 57–61, 64–68, 124–128, 130–133) return { content, isError: true } without structuredContent, but line 156 declares outputSchema: tokenInfoOutputSchema. This causes the SDK to throw a validation error before checking isError, blocking error reporting to the client.
The documented MCP SDK issue #654 confirms this behavior. Correct it by either:
- Omitting
outputSchemafrom the tool registration, or - Returning a valid (possibly minimal)
structuredContentobject on all error paths that satisfies the schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/get_token_info.ts` around lines 119 - 133, The error paths in
get_token_info omit structuredContent while the tool declares outputSchema
(tokenInfoOutputSchema), causing SDK validation to fail; either remove
tokenInfoOutputSchema from the tool registration, or update every error return
(all places that return { content, isError: true } — e.g., the catch block in
getTokenInfo and the earlier error-return sites) to include a minimal
structuredContent object that satisfies tokenInfoOutputSchema so the MCP SDK
validation passes even for isError responses.
| listTools(): Array<{ | ||
| name: string; | ||
| title: string; | ||
| description: string; | ||
| inputSchema: Record<string, unknown>; | ||
| }> { | ||
| return this.getAllTools().map((tool) => ({ | ||
| name: tool.name, | ||
| title: tool.title, | ||
| description: tool.description, | ||
| inputSchema: tool.inputSchema instanceof z.ZodObject ? tool.inputSchema.shape : {}, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual code in the file and surrounding context
cat -n src/transports/http.ts | head -100Repository: vespr-wallet/cardano_mcp
Length of output: 3179
🏁 Script executed:
# Check Zod version
cat package.json | grep -A 5 -B 5 '"zod"'Repository: vespr-wallet/cardano_mcp
Length of output: 363
🏁 Script executed:
# Search for HttpToolDefinition type to understand inputSchema type
grep -n "HttpToolDefinition\|inputSchema" src/transports/http.ts | head -20Repository: vespr-wallet/cardano_mcp
Length of output: 655
🏁 Script executed:
# Check if there are any imports or usage of z.toJSONSchema
rg "toJSONSchema" .Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Search for where listTools() is called
rg "listTools" src/Repository: vespr-wallet/cardano_mcp
Length of output: 489
🏁 Script executed:
# Search for the /mcp/tools endpoint
rg "/mcp/tools\|listTools" src/transports/Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Check if there are tests related to listTools
fd ".*test\|.*spec" | xargs rg -l "listTools" 2>/dev/nullRepository: vespr-wallet/cardano_mcp
Length of output: 119
🏁 Script executed:
# Let's look for what endpoints are registered and how listTools is used
cat -n src/transports/http.ts | sed -n '100,200p'Repository: vespr-wallet/cardano_mcp
Length of output: 3725
🏁 Script executed:
# Let's check the tests to see what the current expected behavior is
cat -n src/transports/http.test.ts | head -150Repository: vespr-wallet/cardano_mcp
Length of output: 5818
🏁 Script executed:
# Look at the listTools test specifically
rg -A 30 "describe.*listTools" src/transports/http.test.tsRepository: vespr-wallet/cardano_mcp
Length of output: 1199
🏁 Script executed:
# Let's check if z.toJSONSchema exists in Zod and how it works
# First, let's create a small test to verify the behavior
python3 << 'EOF'
# Check Zod v4.3.5 documentation/API
# Let's search for information about toJSONSchema availability in Zod
# First, let's check if we can install zod and inspect it
import subprocess
import json
result = subprocess.run(
["npm", "list", "zod", "--json"],
cwd="/tmp",
capture_output=True,
text=True
)
print("NPM list zod output:")
print(result.stdout[:500] if result.stdout else "No output")
print(result.stderr[:500] if result.stderr else "No error")
EOFRepository: vespr-wallet/cardano_mcp
Length of output: 102
🏁 Script executed:
# Let me search the repo for any existing JSON schema conversion attempts
rg "toJSON|toJson|JSON.*[Ss]chema|[Ss]chema.*JSON" src/Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Check the actual shape output structure by looking at how it's used in tests
rg -A 10 "inputSchema.*shape\|\.shape" src/transports/http.test.tsRepository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Let's check what Zod version documentation says about schema serialization
# by looking at imports and what's available from zod package
rg "from ['\"]zod['\"]" src/Repository: vespr-wallet/cardano_mcp
Length of output: 1035
🏁 Script executed:
# Let's check Zod v4.3.5 API - search for toJSONSchema or jsonSchema
rg "jsonSchema\|toJSON\|toJson" --iglob="*.ts" --iglob="*.js" src/Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Let's look for how the HTTP response is actually used - check if there are integration tests
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs rg -l "mcp/tools\|GET.*tools" 2>/dev/null | head -5Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Let's examine what actually happens when we serialize the shape object
# by checking test expectations more thoroughly
cat -n src/transports/http.test.ts | sed -n '116,200p'Repository: vespr-wallet/cardano_mcp
Length of output: 3626
🏁 Script executed:
# Check if there's any mention of JSON serialization issues or schema format expectations
rg "JSON\|serialize\|format" src/transports/http.test.ts | head -20Repository: vespr-wallet/cardano_mcp
Length of output: 50
🌐 Web query:
Zod v4.3.5 toJSONSchema API documentation
💡 Result:
Zod v4.3.5 JSON Schema conversion API: z.toJSONSchema Authoritative docs (Zod JSON Schema conversion): https://v4.zod.dev/json-schema?id=configuration [1] 1) How to use - Convert a Zod schema to JSON Schema with the function: z.toJSONSchema(schema, params?). [1] - Zod 4 adds first-party JSON Schema conversion via z.toJSONSchema. [2] 2) Signature / parameters (ToJSONSchemaParams) From the official docs, the supported options are: [1] - target?: which JSON Schema dialect to target: - "draft-2020-12" (default) - "draft-07" - "draft-04" - "openapi-3.0" (OpenAPI 3.0 Schema Object) [1] - metadata?: registry used to attach metadata to generated schema. - Any schema with an id property will be extracted as a $def. [1] - unrepresentable?: controls how Zod handles types that cannot be represented in JSON Schema: - "throw" (default) throws an error - "any" converts them to {} [1] - cycles?: controls how cycles are handled: - "ref" (default): cycles are broken using $defs - "throw": cycles throw if encountered [1] - reused?: controls handling of reused sub-schemas: - "inline" (default): inline reused schemas - "ref": extract reused schemas into $defs [1] - uri?: function used to convert schema id values into URIs for external $refs (default is (id) => id). [1] 3) Defaults called out in the docs - Default JSON Schema target is Draft 2020-12. [1] - unrepresentable default is "throw". [1] - cycles default is "ref". [1] - reused default is "inline". [1] 4) Input vs output types (io option) The docs also note that some Zod types have different input and output types, and that by default z.toJSONSchema represents the output type; you can use io: "input" to extract the input type instead. [1] 5) Instance method Zod v4 also provides an instance method: schema.toJSONSchema(params?). This is implemented by wiring JSON Schema conversion methods onto the schema instance. [3] 6) Notes about version 4.3.5 itself The commit that bumps Zod to 4.3.5 only shows version/package metadata changes (packages/zod/package.json, packages/zod/jsr.json, packages/zod/src/v4/core/versions.ts). It does not change the documented toJSONSchema parameter API in the sources surfaced here. [4] Example (from the official Zod release/docs) - const mySchema = z.object({ name: z.string, points: z.number }); - z.toJSONSchema(mySchema); produces a JSON Schema object with type, properties, and required. [2] If you want, tell me what exact part you need (e.g., how to set target: "draft-07" or how to control $defs/$ref behavior), and I’ll provide a minimal code example tailored to that.
Citations:
- 1: https://v4.zod.dev/json-schema?id=configuration
- 2: https://zod.dev/v4
- 3: Add
.toJSONSchema()method colinhacks/zod#5477 - 4: colinhacks/zod@0cdc0b8
🏁 Script executed:
# Let me check more of the GET /mcp/tools handler test to see if it verifies the actual JSON response
cat -n src/transports/http.test.ts | sed -n '200,250p'Repository: vespr-wallet/cardano_mcp
Length of output: 1825
🏁 Script executed:
# Let's see what happens when we actually serialize a Zod schema shape to JSON
# Create a quick test to show the problem
node << 'EOF'
const { z } = require("zod");
// Create a schema like in the tests
const schema = z.object({ input: z.string() });
const shape = schema.shape;
console.log("Shape object:", shape);
console.log("Serialized shape:", JSON.stringify(shape));
console.log("\nShape.input:", shape.input);
console.log("Serialized shape.input:", JSON.stringify(shape.input));
// Try toJSONSchema if available
try {
const jsonSchema = z.toJSONSchema(schema, { unrepresentable: "any" });
console.log("\nz.toJSONSchema result:", JSON.stringify(jsonSchema, null, 2));
} catch (e) {
console.log("z.toJSONSchema not available or errored:", e.message);
}
EOFRepository: vespr-wallet/cardano_mcp
Length of output: 920
🏁 Script executed:
# Let me check if there are any integration tests that actually hit the HTTP endpoint
# and verify the JSON response
rg -A 20 "describe.*GET.*mcp.*tools\|describe.*registerHttpRoutes" src/transports/http.test.ts | tail -50Repository: vespr-wallet/cardano_mcp
Length of output: 50
🏁 Script executed:
# Let's look for any tools that have .describe() calls to understand the point about descriptions
rg "\.describe\(" src/tools/ | head -10Repository: vespr-wallet/cardano_mcp
Length of output: 1087
🏁 Script executed:
# Check what the actual Zod instance looks like when logged/stringified
# Look at any debugging or error handling that might show what the shape contains
rg "console\|log\|shape" src/transports/http.tsRepository: vespr-wallet/cardano_mcp
Length of output: 50
listTools() exposes raw Zod schema instances that serialize poorly as JSON in the GET /mcp/tools response.
tool.inputSchema.shape returns a ZodRawShape — a map of field names to Zod schema instances (ZodString, ZodEnum, etc.). When Fastify serializes this in the GET /mcp/tools response via reply.send(), these instances do not produce usable JSON Schema; instead, they serialize to their internal representation or empty objects. Tool consumers calling this endpoint for schema discovery receive garbled data instead of valid schemas.
Zod v4 provides first-party JSON Schema conversion via z.toJSONSchema(), which is the correct approach here. Since tools use .describe() on their fields, z.toJSONSchema() correctly includes those descriptions in the output.
🛠️ Proposed fix
listTools(): Array<{
name: string;
title: string;
description: string;
inputSchema: Record<string, unknown>;
}> {
return this.getAllTools().map((tool) => ({
name: tool.name,
title: tool.title,
description: tool.description,
- inputSchema: tool.inputSchema instanceof z.ZodObject ? tool.inputSchema.shape : {},
+ inputSchema: tool.inputSchema instanceof z.ZodObject
+ ? z.toJSONSchema(tool.inputSchema, { unrepresentable: "any" })
+ : {},
}));
}The unrepresentable: "any" option prevents throws for schema types like z.preprocess(...) that don't map cleanly to JSON Schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transports/http.ts` around lines 65 - 77, The GET /mcp/tools response
currently exposes raw Zod internals because listTools() returns
tool.inputSchema.shape; change listTools() to convert the Zod schemas to JSON
Schema using z.toJSONSchema() instead of exposing .shape: for each tool, detect
if tool.inputSchema is a Zod schema and call tool.inputSchema.toJSONSchema({
unrepresentable: "any" }) (or if inputSchema is a ZodObject you can call
toJSONSchema on it) and return that JSON Schema object as inputSchema; keep the
rest of the returned fields (name, title, description) unchanged so consumers
receive valid JSON Schema with field descriptions.
| const mcpServer = new McpServer({ | ||
| name: "@vespr/cardano-mcp", | ||
| version: VERSION, | ||
| }); | ||
|
|
||
| registerTools(mcpServer); | ||
|
|
||
| mcpServer.connect(transport).catch((error) => { | ||
| logger.error("mcp_server_connect_error", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| }); | ||
|
|
||
| const session: Session = { transport, apiKey, lastActiveAt: Date.now() }; | ||
|
|
||
| return { session, isNew: true }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @modelcontextprotocol/sdk McpServer.connect() need to resolve before StreamableHTTPServerTransport.handleRequest() is safe to call?
💡 Result:
Yes—your server-side flow should await McpServer.connect(transport) before calling StreamableHTTPServerTransport.handleRequest, because handleRequest assumes the transport has already been connected to the server. Evidence: 1) The SDK’s server examples explicitly connect the server to the transport (await server.connect(transport)) before handling any HTTP requests; the sample code comment says “Connect the transport to the MCP server BEFORE handling the request so responses can flow back through the same transport”, and then it calls transport.handleRequest afterward [1]. 2) The transport implementation routes responses through the connection established by connect: WebStandardStreamableHTTPServerTransport has a handleRequest method that “delegates to the underlying WebStandardStreamableHTTPServerTransport” [2], and that underlying transport’s connect/start lifecycle is part of setting up request/response handling state used by handleRequest (including protocol version support passed during connect) [2]. 3) Community reports indicate that the correct ordering avoids failures; for example, implementations that create the transport and connect it prior to request handling are the documented pattern, while incorrect reuse/initialization order leads to protocol/session initialization errors [3]. Practical guidance (TypeScript): - During the first request that establishes a new session/transport, do: - await server.connect(transport) - then call await transport.handleRequest(req, res, req.body) - For subsequent requests that reuse an already-connected transport, you can call handleRequest without reconnecting (the example code does this: “no need to reconnect—The existing transport is already connected to the server”) [4]. Answer to your exact question: If you don’t await McpServer.connect, calling handleRequest “soon” is not guaranteed safe; do await to ensure the transport is fully connected before it processes requests.
Citations:
- 1: https://github.com/modelcontextprotocol/servers/blob/e6b0b0f5/src/everything/transports/streamableHttp.ts
- 2: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_server.server_streamableHttp.WebStandardStreamableHTTPServerTransport.html
- 3: MCP server broken: StreamableHTTPServerTransport stateless mode incompatible with SDK 1.26.0 slopus/happy#623
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/v1.x/src/examples/server/simpleStreamableHttp.ts
🏁 Script executed:
rg -n "async function getOrCreateSession|getOrCreateSession\(" src/transports/streamableHttp.ts -A 2 -B 1Repository: vespr-wallet/cardano_mcp
Length of output: 296
🏁 Script executed:
sed -n '78,109p' src/transports/streamableHttp.tsRepository: vespr-wallet/cardano_mcp
Length of output: 1063
🏁 Script executed:
rg "getOrCreateSession" src/transports/streamableHttp.tsRepository: vespr-wallet/cardano_mcp
Length of output: 162
mcpServer.connect(transport) is fire-and-forget — this creates a race condition where handleRequest may be called before the server is connected.
The web search confirms that McpServer.connect() must be awaited before calling StreamableHTTPServerTransport.handleRequest(). If connect rejects (e.g., transport already closed, version mismatch), the error is logged but getOrCreateSession still returns the local session as valid. The POST handler immediately calls handleRequest on line 96, which will fail because the transport is not yet connected. The session is also never stored in the sessions map, making it impossible to retry.
Make getOrCreateSession async and await connect:
♻️ Proposed refactor
-function getOrCreateSession(
+async function getOrCreateSession(
sessionId: string | undefined,
apiKey: string | undefined,
): Promise<{ session: Session; isNew: boolean }> {
if (sessionId && sessions.has(sessionId)) {
return { session: sessions.get(sessionId)!, isNew: false };
}
const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: () => randomUUID(),
onsessioninitialized: (sid: string) => {
sessions.set(sid, { transport, apiKey, lastActiveAt: Date.now() });
logger.info("mcp_session_created", { sessionId: sid });
},
});
transport.onclose = () => {
const sid = transport.sessionId;
if (sid) {
sessions.delete(sid);
logger.info("mcp_session_closed", { sessionId: sid });
}
};
const mcpServer = new McpServer({
name: "@vespr/cardano-mcp",
version: VERSION,
});
registerTools(mcpServer);
- mcpServer.connect(transport).catch((error) => {
- logger.error("mcp_server_connect_error", {
- error: error instanceof Error ? error.message : String(error),
- });
- });
+ await mcpServer.connect(transport);
const session: Session = { transport, apiKey, lastActiveAt: Date.now() };
return { session, isNew: true };
}And update the POST handler call to:
- const { session } = getOrCreateSession(sessionId, apiKey);
+ const { session } = await getOrCreateSession(sessionId, apiKey);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transports/streamableHttp.ts` around lines 44 - 59, Change
getOrCreateSession to be async and await McpServer.connect(transport) inside a
try/catch: construct McpServer, call registerTools, then await
mcpServer.connect(transport); on success create the Session object, store it
into the sessions map, and return { session, isNew: true }; on connect failure
log the error and rethrow or return a failure so the caller won't proceed.
Update the POST handler to await getOrCreateSession and only call
StreamableHTTPServerTransport.handleRequest(...) if a valid connected session
was returned (handle/propagate errors from connect instead of fire-and-forget).
Use the function/class names mcpServer, getOrCreateSession, McpServer.connect,
StreamableHTTPServerTransport.handleRequest and the sessions map to locate and
modify the code.
| return request.ip; | ||
| } | ||
|
|
||
| export function createRateLimitHook(config: RateLimitConfig) { |
There was a problem hiding this comment.
Ask AI to replace this custom implementation with this npm package: @fastify/rate-limit
| const rateLimitHook = createRateLimitHook({ | ||
| maxRequestsPerMinute: config.rateLimitPerMinute, | ||
| maxRequestsPerDay: config.rateLimitPerDay, | ||
| trustedProxies: config.trustedProxies, | ||
| }); | ||
| server.addHook("onRequest", async (request, reply) => { | ||
| if (request.url.startsWith("/mcp")) { | ||
| await rateLimitHook(request, reply); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Using https://www.npmjs.com/package/@fastify/rate-limit
I think this would be replaced with sth along the lines of
await server.register(fastifyRateLimit, {
max: maxRequestsPerMinute,
timeWindow: "1 minute",
hook: "preValidation",
keyGenerator: function (request) {
return request.headers['x-real-ip'] // nginx
|| request.headers['x-client-ip'] // apache
|| request.headers['cf-connecting-ip'] // cloudflare
|| request.headers['do-connecting-ip'] // digitalocean
|| request.headers['x-forwarded-for'] // cloudflare
|| request.ip // fallback to default
}
errorResponseBuilder: () =>
new MessageError({
devMessage: "Rate limit exceeded, retry in 1 minute",
statusCode: 429,
}),
});
await server.register(fastifyRateLimit, {
max: maxRequestsPerDay,
timeWindow: "1 day", (or whatever is accepted here, not sure)
hook: "preValidation",
keyGenerator: function (request) {
return request.headers['x-real-ip'] // nginx
|| request.headers['x-client-ip'] // apache
|| request.headers['cf-connecting-ip'] // cloudflare
|| request.headers['do-connecting-ip'] // digitalocean
|| request.headers['x-forwarded-for'] // cloudflare
|| request.ip // fallback to default
}
errorResponseBuilder: () =>
new MessageError({
devMessage: "Rate limit exceeded, retry later",
statusCode: 429,
}),
});
Swap the hand-rolled in-memory RateLimiter for @fastify/rate-limit, using a custom dual-window store that enforces both per-minute and per-day limits. IP resolution now uses a priority-header chain (x-real-ip, x-client-ip, cf-connecting-ip, do-connecting-ip, x-forwarded-for) instead of the trustedProxies config.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server.ts (1)
85-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't trust client-supplied forwarding headers for the rate-limit key.
This reintroduces the spoofable-IP problem: a caller can set
x-real-ip,x-client-ip,cf-connecting-ip,do-connecting-ip, orx-forwarded-forarbitrarily and sidestep the per-IP limiter. Preferrequest.iphere, and if this service sits behind proxies, let Fastify resolve the real client IP viatrustProxyinstead of parsing headers manually.Safe stopgap
- const rateLimitKeyGenerator = (request: FastifyRequest): string => - (request.headers["x-real-ip"] as string | undefined) || - (request.headers["x-client-ip"] as string | undefined) || - (request.headers["cf-connecting-ip"] as string | undefined) || - (request.headers["do-connecting-ip"] as string | undefined) || - (typeof request.headers["x-forwarded-for"] === "string" - ? request.headers["x-forwarded-for"].split(",")[0].trim() - : "") || - request.ip; + const rateLimitKeyGenerator = (request: FastifyRequest): string => request.ip;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 85 - 93, The rateLimitKeyGenerator currently trusts client-supplied forwarding headers (x-real-ip, x-client-ip, cf-connecting-ip, do-connecting-ip, x-forwarded-for) which allows IP spoofing; change rateLimitKeyGenerator to use only request.ip for the key and remove parsing of those headers, and instead configure Fastify’s trustProxy setting so Fastify resolves the real client IP from upstream proxies; update any tests or documentation that referenced header-based behavior to reflect the new use of request.ip and trustProxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 12-17: The parseServerMode function and the ALLOWED_ORIGINS
parsing need to trim environment input before processing: in parseServerMode,
call .trim() on process.env.SERVER_MODE (and then .toLowerCase()) so values like
"http " match correctly and return the proper ServerMode instead of falling back
to "stdio"; likewise, when building ALLOWED_ORIGINS (the code that reads
process.env.ALLOWED_ORIGINS and splits on commas), trim each origin entry after
splitting (and ignore empty entries) so spaced CSV values are handled correctly
during CORS checks.
---
Duplicate comments:
In `@src/server.ts`:
- Around line 85-93: The rateLimitKeyGenerator currently trusts client-supplied
forwarding headers (x-real-ip, x-client-ip, cf-connecting-ip, do-connecting-ip,
x-forwarded-for) which allows IP spoofing; change rateLimitKeyGenerator to use
only request.ip for the key and remove parsing of those headers, and instead
configure Fastify’s trustProxy setting so Fastify resolves the real client IP
from upstream proxies; update any tests or documentation that referenced
header-based behavior to reflect the new use of request.ip and trustProxy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e9ce3ce-5e29-4cca-9fd5-4a378e6c1cf0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/config.tssrc/middleware/rateLimit.test.tssrc/middleware/rateLimit.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| function parseServerMode(): ServerMode { | ||
| const mode = process.env.SERVER_MODE?.toLowerCase(); | ||
| if (mode === "http" || mode === "both") { | ||
| return mode; | ||
| } | ||
| return "stdio"; // Default for backward compatibility |
There was a problem hiding this comment.
Trim env values before parsing.
A trailing space in SERVER_MODE currently falls back to "stdio", and spaced CSV values in ALLOWED_ORIGINS will miss valid origins during CORS checks. Trimming both inputs makes deployment config much less brittle.
Proposed fix
function parseServerMode(): ServerMode {
- const mode = process.env.SERVER_MODE?.toLowerCase();
+ const mode = process.env.SERVER_MODE?.trim().toLowerCase();
if (mode === "http" || mode === "both") {
return mode;
}
return "stdio"; // Default for backward compatibility
}
@@
- allowedOrigins: (process.env.ALLOWED_ORIGINS ?? "").split(",").filter(Boolean),
+ allowedOrigins: (process.env.ALLOWED_ORIGINS ?? "")
+ .split(",")
+ .map((origin) => origin.trim())
+ .filter(Boolean),Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 12 - 17, The parseServerMode function and the
ALLOWED_ORIGINS parsing need to trim environment input before processing: in
parseServerMode, call .trim() on process.env.SERVER_MODE (and then
.toLowerCase()) so values like "http " match correctly and return the proper
ServerMode instead of falling back to "stdio"; likewise, when building
ALLOWED_ORIGINS (the code that reads process.env.ALLOWED_ORIGINS and splits on
commas), trim each origin entry after splitting (and ignore empty entries) so
spaced CSV values are handled correctly during CORS checks.
- min-release-age=7 days - ignore-scripts=true - Include .npmrc in both builder and runtime Docker stages
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.npmrc:
- Line 3: The inline comment on the min-release-age setting is breaking parsing;
remove the inline comment so the value is exactly "7" and place any explanatory
text on its own line above using a leading "#" (e.g., add a line "# days" or a
brief comment) so the .npmrc min-release-age entry reads simply
min-release-age=7 and is correctly parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f258cc99-3cee-438b-ae7b-3bec8edb64b8
📒 Files selected for processing (2)
.npmrcDockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
2 participants
Summary
SERVER_MODEenv var/mcp) using@modelcontextprotocol/sdk, enabling Claude Codetype:http.mcp.jsonintegration with no local install requiredMcpServerinstances with API key propagation viaAsyncLocalStorage/mcp/*endpoints with comprehensive testsDockerfile,docker-compose.yml,.dockerignore)Test plan
npm testpasses (rate limiter tests + HTTP transport tests insrc/middleware/rateLimit.test.tsandsrc/transports/http.test.ts)SERVER_MODE=stdiostarts correctly for Claude Desktop usageSERVER_MODE=httpstarts Fastify and responds atPOST /mcp.mcp.jsonwithtype: "http"connects successfully from Claude Codedocker compose up --buildSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation