fix(mcp-server): restore HTTP startup - #337
Conversation
Use the supported environment-based transport configuration and keep package guidance and runtime validation in sync. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
=======================================
Coverage 99.69% 99.69%
=======================================
Files 64 64
Lines 4293 4293
=======================================
Hits 4280 4280
Misses 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core fix is correct: parseArgs() uses commander with no --streamable-http/--http-sse options, so the old start scripts died on an unknown flag, and switching to SERVER_TRANSPORT matches args.ts/config.ts exactly. Docs, .env.example, and the changeset all line up with the code; remaining notes are hardening of the new smoke check and a couple of doc gaps.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the PR diff";
cursor-review.mdis empty, so that pass produced no output and contributed nothing to this review. - Doc-link swap: the README/
resources/page.tsxlinks move offdocs.sei.io/ai/mcp-serverto the package README. Per guideline §2's "ask rather than assert" posture on canonical sources — worth confirming the docs page is genuinely retired rather than temporarily broken; if it still exists, keeping a pointer alongside the README anchor would be better for generated apps. getAvailablePort()(scripts/check-mcp-cli.ts:24) closes the probe socket before the child binds, so the port can be taken in between. Low risk on CI, but a bind failure would surface as an opaquehealthy=falserather than "port in use"; including the child's stderr already helps, so this is only a nit.- Verification note: I could not execute
bun run --cwd packages/mcp-server buildorbun scripts/check-mcp-cli.tsin this sandbox, so the new smoke check is reviewed statically — the PR description reports bothbun run lint:pack:runtimeand the 502-test suite passing. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| const healthy = await waitForHealth(child, port); | ||
|
|
||
| await stopChild(child); | ||
| const [stdout, stderr] = await Promise.all([stdoutPromise, stderrPromise]); |
There was a problem hiding this comment.
[suggestion] This awaits the pipes to EOF after killing only the bun run wrapper. If the wrapper dies without the node grandchild going with it (guaranteed in the kill(9) path at line 63, and dependent on bun's signal forwarding even in the SIGTERM path), the grandchild keeps the stdout/stderr write ends open and Promise.all never settles — lint:pack:runtime then hangs with no internal timeout in both checks.yml and release.yml, so a release would block on a job timeout rather than a readable failure. Consider racing the reads with a timeout (Promise.race([Promise.all([...]), Bun.sleep(2_000).then(() => ['', ''])])) so the check always reports, and/or killing the process group so the server can't outlive the wrapper.
| cwd: root, | ||
| env: { | ||
| ...process.env, | ||
| SERVER_TRANSPORT: 'stdio', |
There was a problem hiding this comment.
[nit] Worth a one-line comment: setting SERVER_TRANSPORT: 'stdio' here is load-bearing, not a copy-paste slip — it proves the package script's own SERVER_TRANSPORT=... assignment wins over the inherited env, which is exactly the regression this PR fixes. Without the comment a future cleanup is likely to "fix" it to the target transport and silently defang the check.
| npx -y @sei-js/mcp-server | ||
| ``` | ||
|
|
||
| `http-sse` is retained for older clients; use `streamable-http` for new integrations. |
There was a problem hiding this comment.
[suggestion] This section now actively recommends the HTTP transports, but neither http-sse.ts nor streamable-http.ts authenticates callers or validates Origin/Host, and createCorsMiddleware() only answers preflights (it protects browsers that honour a missing Access-Control-Allow-Origin, not non-browser clients or DNS-rebinding). Since these docs are what operators will follow, a short note — bind to 127.0.0.1 and put any public exposure behind an authenticating proxy — would pair well with the existing wallet-mode warning at line 60.
| # Recommended: Streamable HTTP at http://localhost:8080/mcp | ||
| SERVER_TRANSPORT=streamable-http npx -y @sei-js/mcp-server | ||
|
|
||
| # Legacy HTTP/SSE at http://localhost:8080/mcp |
There was a problem hiding this comment.
[nit] For http-sse the URL is only half the story: {SERVER_PATH} is the GET stream, and clients must POST to {SERVER_PATH}/message?sessionId=<id> (400 without the param, 404 on unknown session — see http-sse.ts:61). Since SERVER_PATH is documented as configurable at line 101, spelling out the derived message endpoint would save legacy-client integrators a round trip.
2 participants
Summary
start:httpandstart:http-sseselect the supported environment-based transportsTest plan
bun run checkbun run --filter '@sei-js/mcp-server' test(502 tests)bun run lint:pack:runtimebun run --filter '@sei-js/create-sei' buildbun run --filter '@sei-js/create-sei' testMade with Cursor