Migrate the project to PNPM #4
Conversation
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
[WIP] Migrate project to use pnpm with latest version
|
Warning Rate limit exceeded@warengonzaga has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 23 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded pnpm as the package manager, Railway deployment configuration, documentation updates, dependency upgrades, and a new Express-based HTTP/SSE MCP server (src/server.ts) that provides an SSE transport alongside the existing stdio transport. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressServer as SSE Server
participant MCP
participant Relay as Relay API
Note over Client,ExpressServer: Establish SSE session
Client->>ExpressServer: GET /sse
ExpressServer->>Client: 200 OK (SSE open, sessionId)
ExpressServer->>MCP: connectTransport(sessionId)
MCP-->>ExpressServer: transport registered
Note over Client,ExpressServer: Invoke tool
Client->>ExpressServer: POST /message (sessionId + payload)
ExpressServer->>MCP: deliverMessage(sessionId, payload)
alt MCP calls external Relay
MCP->>Relay: execute tool / Relay request
Relay-->>MCP: result / error
end
MCP-->>ExpressServer: execution result
ExpressServer-->>Client: SSE event or HTTP response with result
Note over ExpressServer: Disconnect cleanup
Client-->>ExpressServer: close connection
ExpressServer->>MCP: disconnectTransport(sessionId)
MCP-->>ExpressServer: transport removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @package.json:
- Line 23: The package.json currently lists "@types/express" under
"dependencies"; move "@types/express": "^5.0.3" into "devDependencies" instead
(remove it from "dependencies" and add the same entry under "devDependencies")
so type definitions are only installed at dev time; after editing package.json
run your package manager install (npm/yarn/pnpm) to update the lockfile
(package-lock.json / yarn.lock / pnpm-lock.yaml).
In @src/server.ts:
- Around line 174-179: The response currently includes error.stack in the JSON
body (in the error response details object) which can leak implementation
internals; remove the stack property from the response payload and instead log
the full error server‑side (e.g., call console.error('Unexpected tool error:',
error) or use the app logger) while keeping details.message as before; update
the error response construction where the object with error: 'Unexpected error'
and details is created so it no longer assigns stack but still returns a safe
message to the client.
- Around line 231-235: The /message POST currently returns 200 without
forwarding the request to the SSE transport; update the app.post('/message',
...) handler to pass the incoming request and response to your
SSEServerTransport instance (e.g., call the transport's request/message handling
method such as handleMessage(req, res) or handleRequest(req, res) depending on
the MCP SDK API) so the transport can read the body and respond appropriately,
await the handler if it's async, and only end/return the response after the
transport has processed the message.
🧹 Nitpick comments (3)
railway.json (1)
5-5: Consider using--frozen-lockfilefor reproducible builds.Adding
--frozen-lockfileensures the lockfile is not modified during CI/deployment, catching dependency mismatches early.🔎 Proposed fix
- "buildCommand": "pnpm install && pnpm build" + "buildCommand": "pnpm install --frozen-lockfile && pnpm build"src/server.ts (2)
52-68: Consider adding CORS middleware for cross-origin clients.If the SSE server will be accessed from web-based MCP clients or different origins, CORS headers will be required.
🔎 Example CORS setup
import cors from 'cors'; // ... app.use(cors({ origin: process.env.ALLOWED_ORIGINS?.split(',') || '*', credentials: true, }));
237-244: Consider adding graceful shutdown handling.For production deployments, graceful shutdown ensures active connections are closed properly before the process exits.
🔎 Proposed enhancement
const httpServer = app.listen(PORT, () => { console.log(`🚀 Relay Protocol MCP Server running on port ${PORT}`); // ... existing logs }); // Graceful shutdown process.on('SIGTERM', () => { console.log('SIGTERM received, shutting down gracefully...'); httpServer.close(() => { console.log('HTTP server closed'); process.exit(0); }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.github/copilot-instructions.md.gitignore.railwayignoreREADME.mdpackage.jsonrailway.jsonsrc/server.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/server.ts (4)
src/config.ts (1)
config(10-25)src/client/RelayClient.ts (1)
RelayClient(45-424)src/tools/index.ts (1)
createAllTools(63-73)src/client/errors.ts (2)
RelayAPIError(18-51)RelayConnectionError(62-73)
🪛 LanguageTool
README.md
[grammar] ~49-~49: Ensure spelling is correct
Context: ...loyment guide. ## MCP Integration ### Stdio Transport (Local - Claude Desktop) Add...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (10)
.github/copilot-instructions.md (1)
118-121: LGTM!Development workflow commands correctly updated to use
pnpminstead ofyarn, consistent with the package manager migration..gitignore (2)
6-6: LGTM!Adding
pnpm-debug.log*is appropriate for the package manager migration.
50-52: LGTM!Ignoring auto-generated Snyk rules and context directory is reasonable.
.railwayignore (1)
1-37: LGTM!Railway ignore configuration is well-structured, excluding development files while retaining the
dist/build output needed for deployment.README.md (3)
17-45: LGTM!Quick Start section is well-updated with clear instructions for both local setup using
pnpmand cloud deployment using SSE transport.
70-103: LGTM!Railway deployment guide is comprehensive with clear steps, Claude Desktop configuration example, and helpful deployment information including resource estimates.
156-158: LGTM!Project structure documentation clearly distinguishes between the Stdio transport (local) and SSE transport (cloud) entry points.
package.json (2)
49-49: pnpm version 10.18.2 is valid and available—it was released on October 9, 2025.
25-25: Express 5.x is now stable and production-ready. Express v5.0.0 was released in September 2024, and v5.1.0 (March 2025) is marked as the recommended default on npm. Using^5.1.0is appropriate.src/server.ts (1)
218-229: This review concern is incorrect. The pattern of creating a newSSEServerTransportper SSE connection and callingserver.connect(transport)for each is exactly how the MCP SDK is designed to work. According to the SDK specification,SSEServerTransport.Connectis single-use (one connect per transport instance), so supporting multiple concurrent clients requires instantiating a new transport for each GET request and callingserver.connect(transport)separately—which is what this code does.Likely an incorrect or invalid review comment.
|
@copilot can you apply the code suggestion? |
|
@warengonzaga I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
…, remove stack traces, fix /message endpoint Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Apply code review suggestions: fix SSE transport, remove stack traces, move type deps
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @src/server.ts:
- Around line 122-143: The error response includes sensitive request data via
error.requestData; remove requestData from the object returned to clients and
only keep non-sensitive fields (e.g., error.message, statusCode, responseData).
Instead, log the full requestData server-side for debugging using the server
logger or error reporting path (e.g., when handling RelayAPIError). Update the
RelayAPIError handling block to omit request/requestData from the returned JSON
but still record it in process logs or error monitoring.
- Around line 244-271: The sessionId query param is unsafely cast to string in
the app.post('/message') handler; validate and normalize req.query.sessionId
which can be string | string[] | undefined: if it's an array, pick the first
element or reject with 400, and ensure you handle undefined safely before
calling transports.get(sessionId) and transport.handlePostMessage. Update the
handler to coerce/validate sessionId (e.g., const sid =
Array.isArray(req.query.sessionId) ? req.query.sessionId[0] :
req.query.sessionId) and use sid for transports.get and subsequent checks,
returning 400 if no valid single sessionId is provided.
- Around line 10-12: Update the top-of-file comment in server.ts that currently
lists only two endpoints; change it to list all exposed routes (/sse, /message,
/health, /) or rephrase to say "primary MCP endpoints: /sse and /message" and
add a second line that mentions auxiliary endpoints (/health and root). Modify
the comment block where the endpoints are documented so the text and bullet list
accurately reflect the implemented routes (/sse, /message, /health, /).
- Around line 225-242: Wrap the SSE endpoint body in a try-catch and handle
errors from new SSEServerTransport('/message', res) and await
server.connect(transport) by logging the error, sending an appropriate 500
response and closing/cleaning up the transport if it exposes a close/end method;
after creating transport, assert transport.sessionId is non-null/non-undefined
before calling transports.set(sessionId, transport) and before registering
req.on('close') (if sessionId is missing, log and respond with 500 and ensure
transport is closed), and ensure the req.on('close') handler deletes transports
only if the captured sessionId is truthy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonsrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/server.ts (4)
src/config.ts (1)
config(10-25)src/client/RelayClient.ts (1)
RelayClient(45-424)src/tools/index.ts (1)
createAllTools(63-73)src/client/errors.ts (2)
RelayAPIError(18-51)RelayConnectionError(62-73)
🔇 Additional comments (8)
src/server.ts (8)
21-34: LGTM!Imports are well-organized and include all necessary dependencies for the SSE transport server.
36-36: LGTM!PORT configuration follows best practices with environment variable override and safe parsing.
52-74: LGTM!Server initialization is well-structured: Express setup, MCP server creation with proper configuration, Relay client initialization, and tool registration all follow best practices.
80-86: LGTM!ListTools handler correctly maps tool metadata for client discovery.
145-191: LGTM!Error handling for connection errors and unexpected errors is well-implemented. Stack traces are properly logged server-side only (addressing previous review feedback), and only safe error messages are returned to clients.
195-202: LGTM!Health check endpoint provides appropriate status information for monitoring.
205-219: LGTM!Root endpoint provides helpful server information and endpoint discovery for developers.
274-291: LGTM!Server startup and entry point error handling follow best practices. The startup logs provide helpful information about available endpoints and tools.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @src/server.ts:
- Around line 10-12: Update the top-of-file server comment that currently says
"The server exposes two endpoints" to list all four endpoints exposed by the
code: GET /sse (SSE for MCP protocol), POST /message (HTTP message endpoint),
GET /health (healthcheck), and GET / (root). Locate the header comment or the
comment block describing endpoints (the lines containing "/sse" and "/message")
and change the wording and bullet list so the documentation accurately reflects
the four routes.
- Around line 229-246: Wrap the SSE connection logic in a try-catch: construct
SSEServerTransport inside the try, then check transport.sessionId for
null/undefined before using it (do not call transports.set with a falsy
sessionId); if sessionId is missing, respond with a 400/500 error and close the
response and transport. Also catch errors from server.connect and on any caught
exception send an appropriate HTTP error (and close/res.end) and ensure any
partially created transport is cleaned up from transports and closed; keep the
existing req.on('close') handler to remove transports when the client
disconnects.
- Around line 126-147: The RelayAPIError handling currently includes
error.requestData in the returned response which may leak sensitive info; update
the handler (the RelayAPIError branch in src/server.ts) to remove or sanitize
requestData before returning—either omit requestData entirely from the object
you stringify or replace it with a safe subset (e.g., HTTP method and URL only)
and ensure only non-sensitive fields (statusCode, responseData or a sanitized
response summary) are included in the returned payload; keep the structure
(content/isError) but do not include error.requestData.
- Around line 248-275: The code unsafely asserts sessionId as string; replace
that assertion with a type-safe check: read const rawSessionId =
req.query.sessionId; if (Array.isArray(rawSessionId)) return
res.status(400).json({ error: 'Multiple sessionId values not allowed' }) (or
alternatively pick the first element with const sessionId = rawSessionId[0]); if
(rawSessionId == null) return the existing 400 for missing sessionId; then use
the validated sessionId when calling transports.get(sessionId) and
transport.handlePostMessage to avoid runtime errors from arrays or undefined
values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonsrc/index.tssrc/server.tssrc/tools/currencies.tssrc/tools/swap.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/config.ts (1)
config(10-25)
src/server.ts (4)
src/config.ts (1)
config(10-25)src/client/RelayClient.ts (1)
RelayClient(45-424)src/tools/index.ts (1)
createAllTools(63-73)src/client/errors.ts (2)
RelayAPIError(18-51)RelayConnectionError(62-73)
🔇 Additional comments (7)
src/tools/currencies.ts (1)
49-49: LGTM! Correct Zod v4 migration.The change from
error.errorstoerror.issuescorrectly aligns with Zod v4's standardized error structure. As per the Zod v4 documentation, the newissuesarray provides the standardized issue formats under thez.corenamespace.src/tools/swap.ts (1)
73-73: LGTM! Consistent Zod v4 migration.The error handling has been correctly updated to use
error.issuesinstead oferror.errors, maintaining consistency with the Zod v4 migration applied in other tool files.src/server.ts (1)
173-186: LGTM! Stack trace exposure fixed.The unexpected error handler now correctly logs stack traces server-side only (line 174) while excluding them from client responses (line 185). This addresses the previous security concern about leaking implementation details.
package.json (3)
13-15: LGTM! SSE scripts correctly configured.The new
dev:sseandstart:ssescripts properly support the SSE transport entry point atsrc/server.ts, with development usingtsxand production using the compiled output.
28-35: LGTM! Development dependencies correctly organized.The
@types/expresspackage is now properly placed indevDependencies(addressing previous review feedback), and all development tooling packages have been updated to recent versions.
22-25: Verify major version upgrades are tested and compatible before merging.The specified versions exist and are published without visible security warnings. However, ensure these major upgrades are tested:
- Zod v4.3.5: Major version with breaking changes (error.issues API change evident in code)
- Express v5.2.1: Major version requiring compatibility verification
- @modelcontextprotocol/sdk v1.25.1: API changes require validation
- axios v1.13.2: Minor bump with existing type definitions (@types/express^5.0.6 present)
Confirm that the application works correctly with these versions together before release.
src/index.ts (1)
61-65: Server constructor icons array is the correct API format for @modelcontextprotocol/sdk.The update from a single
iconproperty to anicons: [{ src: string }]array is consistent with the current SDK API. Each Icon object requires asrcfield (HTTPS URL or data: URI) and optionally supportsmimeType,sizes, andthemeproperties. The implementation at lines 61-65 is correct.
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.