feat: prepare npm publishing with doctor CLI - #9
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesCLI and release flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (4)
src/cli.ts (1)
192-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
runClirejects theservecommand it documents.USAGE at lines 13-14 advertises
mottainaiandmottainai serve, butrunClihas noservebranch, so it falls through tofail(USAGE).scripts/mcp.tsforwards every argument torunCli, soscripts/mcp.ts serveprints usage and exits 1. Onlysrc/index.tsinterceptsserve. Either handleserveinrunCliand let both entry points share one dispatch, or remove the server lines from USAGE.🤖 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/cli.ts` around lines 192 - 199, Update runCli to recognize the documented "serve" command instead of falling through to fail(USAGE), ensuring scripts/mcp.ts serve and the main entry point share the same dispatch behavior; preserve the existing doctor and unknown-command handling.src/index.ts (1)
7-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle
runServerfailures explicitly.
runServercan reject during config load or OAuth provider load. This entry point does not catch it, so Node prints an unhandled-rejection stack trace. This entry point runs the MCP stdio server, so a clean message on stderr plusprocess.exitCode = 1gives a better client-side diagnostic.♻️ Proposed structure
-if (args.length === 0) { - await runServer(); -} else if (args[0] === "serve") { - const configIndex = args.indexOf("--config", 1); - const configPath = configIndex === -1 ? undefined : args[configIndex + 1]; - if (configIndex !== -1 && configPath === undefined) { - console.error("missing value for --config"); - process.exitCode = 1; - } else { - await runServer(configPath); - } -} else { - process.exitCode = await runCli(args); -} +if (args.length === 0 || args[0] === "serve") { + const configIndex = args.indexOf("--config", 1); + const configPath = configIndex === -1 ? undefined : args[configIndex + 1]; + if (configIndex !== -1 && configPath === undefined) { + console.error("missing value for --config"); + process.exitCode = 1; + } else { + try { + await runServer(configPath); + } catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exitCode = 1; + } + } +} else { + process.exitCode = await runCli(args); +}🤖 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/index.ts` around lines 7 - 20, Wrap the `runServer` calls in the `src/index.ts` entry-point flow with explicit rejection handling, including both the no-argument path and the `"serve"` path. On failure, print a concise diagnostic to stderr and set `process.exitCode = 1`, preventing an unhandled-rejection stack trace while preserving the existing config-argument validation and CLI behavior.src/server.ts (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded server version can drift from the published package version.
The
Servermetadata setsversion: "0.1.0"directly. This PR preparespackage.jsonfor npm publishing with a real version number. If the hardcoded string is not kept in sync with the actual published version, clients that read the MCP server's reported version (for compatibility checks or diagnostics) get an incorrect value after every release.Read the version from
package.jsonat build or runtime instead of hardcoding it.♻️ Proposed fix
+import packageJson from "../package.json" with { type: "json" }; ... const server = new Server( - { name: "mottainai", version: "0.1.0" }, + { name: "mottainai", version: packageJson.version }, { capabilities: { tools: {} } }, );🤖 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 28 - 31, Update the Server metadata initialization to source the version from package.json instead of the hardcoded "0.1.0" value, ensuring the reported version remains synchronized with the published package while preserving the existing server name and capabilities..github/workflows/publish.yml (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the ad-hoc npm install to an exact version.
npm install --global npm@11installs whatever the latest11.xrelease is at run time, since only the major version is pinned. This is flagged by static analysis as an ad-hoc install outside the lockfile. Pin an exact version (for examplenpm@11.5.1or later, the minimum required for OIDC trusted publishing) for reproducible releases.🤖 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 @.github/workflows/publish.yml around lines 32 - 33, Update the “Require an OIDC-capable npm CLI” workflow step to install npm using an exact version, such as npm@11.5.1 or a later supported release, instead of the floating npm@11 range.Source: Linters/SAST tools
🤖 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/mcp-cli.test.ts`:
- Around line 228-238: The test in this file depends on ripgrep being installed,
but the publish workflow does not include a step to install it while the CI
workflow does. Either add a ripgrep installation step to the publish workflow
file (matching the approach in the CI workflow) to ensure the dependency is
available when pnpm test runs, or refactor the test to remove its dependency on
the ripgrep command.
In `@src/server.ts`:
- Around line 11-48: The runServer function creates an UpstreamRegistry instance
in the upstreams variable but never closes it during shutdown, leaving stdio
child processes alive. Add an idempotent shutdown path in runServer that awaits
upstreams.close() before the process exits, triggered by transport closure or
process signals (SIGINT/SIGTERM). Ensure this cleanup runs after
server.connect(transport) completes and before the function returns.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 32-33: Update the “Require an OIDC-capable npm CLI” workflow step
to install npm using an exact version, such as npm@11.5.1 or a later supported
release, instead of the floating npm@11 range.
In `@src/cli.ts`:
- Around line 192-199: Update runCli to recognize the documented "serve" command
instead of falling through to fail(USAGE), ensuring scripts/mcp.ts serve and the
main entry point share the same dispatch behavior; preserve the existing doctor
and unknown-command handling.
In `@src/index.ts`:
- Around line 7-20: Wrap the `runServer` calls in the `src/index.ts` entry-point
flow with explicit rejection handling, including both the no-argument path and
the `"serve"` path. On failure, print a concise diagnostic to stderr and set
`process.exitCode = 1`, preventing an unhandled-rejection stack trace while
preserving the existing config-argument validation and CLI behavior.
In `@src/server.ts`:
- Around line 28-31: Update the Server metadata initialization to source the
version from package.json instead of the hardcoded "0.1.0" value, ensuring the
reported version remains synchronized with the published package while
preserving the existing server name and capabilities.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8048bb1a-7715-493f-a19d-4ad51e8fff09
📒 Files selected for processing (10)
.github/workflows/publish.ymlREADME.mdpackage.jsonscripts/mcp.tssrc/cli.tssrc/commands/doctor.test.tssrc/commands/doctor.tssrc/index.tssrc/mcp-cli.test.tssrc/server.ts
| export async function runServer(configPath?: string, cwd: string = process.cwd()): Promise<void> { | ||
| const snapshot = loadConfigSnapshot(configPath, cwd); | ||
| const oauthCredentialProvider = await loadOAuthCredentialProvider( | ||
| snapshot.gatewayConfig.oauthProviderModule, | ||
| path.dirname(snapshot.configPath), | ||
| ); | ||
| const upstreams = new UpstreamRegistry( | ||
| Object.entries(snapshot.config.mcpServers).map(([name, upstream]) => ({ name, ...upstream })), | ||
| undefined, | ||
| oauthCredentialProvider, | ||
| ); | ||
| const logger = createLogger(); | ||
| const artifactStore = new InMemoryArtifactStore({ | ||
| ttlMs: snapshot.gatewayConfig.resultTtlMs, | ||
| maxEntries: snapshot.gatewayConfig.resultMaxEntries, | ||
| }); | ||
|
|
||
| const server = new Server( | ||
| { name: "mottainai", version: "0.1.0" }, | ||
| { capabilities: { tools: {} } }, | ||
| ); | ||
| const activeProfileName = snapshot.config.gateway?.activeProfile; | ||
| const activeProfile = activeProfileName === undefined | ||
| ? undefined | ||
| : snapshot.config.profiles?.[activeProfileName]; | ||
| registerProxyHandlers( | ||
| server, | ||
| upstreams, | ||
| logger, | ||
| artifactStore, | ||
| snapshot.gatewayConfig, | ||
| {}, | ||
| activeProfile, | ||
| ); | ||
|
|
||
| const transport = new StdioServerTransport(); | ||
| await server.connect(transport); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether upstream shutdown is wired to process signals or transport close events.
rg -n 'SIGINT|SIGTERM|\.close\(\)|onclose' src/proxy.ts src/server.ts src/index.ts src/upstream.tsRepository: yohn-jp/mottainai
Length of output: 408
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked files ---'
git ls-files | rg '(^|/)(src/(server|proxy|upstream|index)\.ts|package(-lock)?\.json|pnpm-lock\.yaml|yarn\.lock|README\.md)$'
printf '%s\n' '--- src/server.ts ---'
cat -n src/server.ts
printf '%s\n' '--- src/proxy.ts outline and shutdown-related code ---'
ast-grep outline src/proxy.ts
rg -n -C 5 'registerProxyHandlers|close|onclose|SIGINT|SIGTERM|transport|server\.' src/proxy.ts
printf '%s\n' '--- src/upstream.ts relevant sections ---'
cat -n src/upstream.ts | sed -n '1,235p'
printf '%s\n' '--- package metadata and SDK references ---'
rg -n -C 3 'StdioServerTransport|`@modelcontextprotocol`|Server|transport|close|SIGINT|SIGTERM' package.json package-lock.json pnpm-lock.yaml yarn.lock README.md src 2>/dev/null || trueRepository: yohn-jp/mottainai
Length of output: 50376
🌐 Web query:
@modelcontextprotocol/sdk 1.29.0 StdioServerTransport SIGINT SIGTERM close implementation server connect
💡 Result:
In the @modelcontextprotocol/sdk, StdioServerTransport is designed for communication over standard input (stdin) and standard output (stdout) [1][2]. Regarding handling shutdown signals (SIGINT/SIGTERM) and pipe closure, here is the current technical status: 1. Stdin EOF Handling: Modern versions of the SDK (specifically addressing issues where servers became "zombie" processes) have implemented logic within StdioServerTransport to detect when the client closes the stdin pipe [3]. The transport now listens for 'end' and 'close' events on stdin, which triggers the transport's close method [3]. This is considered the primary, portable mechanism for graceful shutdown in the MCP stdio specification, as it occurs when the client terminates or disconnects [1]. 2. SIGINT/SIGTERM Handling: The SDK's transport implementation does not automatically intercept process-level signals like SIGINT or SIGTERM [4][3]. These signals are operating system-level events directed at the entire process. It is the responsibility of the server application developer to handle these signals explicitly (e.g., via Node.js process.on('SIGINT',...)) and call the server's or transport's close method to ensure graceful cleanup [4]. Examples in the SDK have been updated to include these explicit handlers [4]. 3. Close Implementation: The close method in StdioServerTransport is designed to be idempotent [3]. When invoked—whether via explicit application logic (e.g., signal handler), stdin closure, or direct calls—it performs the following: - Removes event listeners from stdin and stdout [5][6]. - Optionally pauses stdin if no other listeners remain [5][6]. - Clears internal read buffers [5][6]. - Fires the onclose callback [5][3]. In summary, while stdin EOF is now automatically handled to trigger transport closure, application developers must still implement their own process-level signal handlers (SIGINT/SIGTERM) to trigger this shutdown flow gracefully when the process receives these signals [4][3].
Citations:
- 1: https://modelcontextprotocol.io/specification/2026-07-28/basic/transports/stdio
- 2: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_server.server_stdio.StdioServerTransport.html
- 3: fix(server): close StdioServerTransport when stdin ends or closes modelcontextprotocol/typescript-sdk#2494
- 4: fix: async onclose, stdin EOF detection, SIGTERM in examples modelcontextprotocol/typescript-sdk#1814
- 5: https://github.com/modelcontextprotocol/typescript-sdk/blob/2c0c481cb9dbfd15c8613f765c940a5f5bace94d/packages/server/src/server/stdio.ts
- 6: https://github.com/modelcontextprotocol/typescript-sdk/blob/327243ce/packages/server/src/server/stdio.ts
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- src/index.ts ---'
cat -n src/index.ts
printf '%s\n' '--- package scripts ---'
cat -n package.json | sed -n '1,55p'
printf '%s\n' '--- focused proxy lifecycle references ---'
rg -n -C 8 'registerProxyHandlers|UpstreamRegistry|upstreams\.close|server\.on|transport\.on|onclose|close\(' src/proxy.ts src/index.ts src/server.ts src/*.test.tsRepository: yohn-jp/mottainai
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
base='https://unpkg.com/@modelcontextprotocol/sdk@1.29.0/dist/esm'
for file in server/stdio.js server/index.js shared/transport.js; do
printf '\n--- %s ---\n' "$file"
curl -fsSL "$base/$file" | rg -n -C 8 'class StdioServerTransport|class Server|on\("end"|on\("close"|onclose|async close|connect\(' || true
done
printf '\n--- exact installed SDK version from lockfile ---\n'
rg -n -C 2 \"'`@modelcontextprotocol/sdk`@|version: 1\\.29\\.0\" pnpm-lock.yamlRepository: yohn-jp/mottainai
Length of output: 3448
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact SDK 1.29.0 stdio lifecycle ---'
curl -fsSL 'https://unpkg.com/@modelcontextprotocol/sdk@1.29.0/dist/esm/server/stdio.js' \
| sed -n '1,95p'
printf '%s\n' '--- exact SDK 1.29.0 server connect/close lifecycle ---'
curl -fsSL 'https://unpkg.com/@modelcontextprotocol/sdk@1.29.0/dist/esm/shared/protocol.js' \
| rg -n -C 10 'connect\(|close\(|onclose|transport\.on' || true
curl -fsSL 'https://unpkg.com/@modelcontextprotocol/sdk@1.29.0/dist/esm/server/index.js' \
| rg -n -C 10 'connect\(|close\(|onclose|transport\.on' || true
printf '%s\n' '--- exact lockfile entry ---'
rg -n -C 3 'modelcontextprotocol/sdk@1\.29\.0|version: 1\.29\.0' pnpm-lock.yamlRepository: yohn-jp/mottainai
Length of output: 8649
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact SDK 1.29.0 upstream stdio client close ---'
curl -fsSL 'https://unpkg.com/@modelcontextprotocol/sdk@1.29.0/dist/esm/client/stdio.js' \
| sed -n '1,220p' \
| rg -n -C 12 'class StdioClientTransport|async close|child|kill|AbortController|on\("exit"|on\("close"'
printf '%s\n' '--- repository shutdown wiring, excluding tests ---'
rg -n -C 4 'process\.on|once\(["'\"']SIG|SIGINT|SIGTERM|upstreams\.close|server\.close|transport\.close|onclose' src --glob '!*.test.ts' || trueRepository: yohn-jp/mottainai
Length of output: 4953
Wire UpstreamRegistry.close() into server shutdown.
registerProxyHandlers and SDK StdioServerTransport do not close upstreams on transport closure or SIGINT/SIGTERM. Add an idempotent shutdown path in runServer that awaits upstreams.close() before the process exits. This prevents stdio upstream child processes from remaining alive.
🤖 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 11 - 48, The runServer function creates an
UpstreamRegistry instance in the upstreams variable but never closes it during
shutdown, leaving stdio child processes alive. Add an idempotent shutdown path
in runServer that awaits upstreams.close() before the process exits, triggered
by transport closure or process signals (SIGINT/SIGTERM). Ensure this cleanup
runs after server.connect(transport) completes and before the function returns.
1 participant
Summary
mottainaiwith npm metadata, public access defaults, and release workflowmottainai doctorwith human-readable and JSON diagnosticsmottainai serveDoctor checks
.mottainaiwrite accessThe default diagnostic path remains static and does not start or connect to upstream servers.
Validation
pnpm run typecheckpnpm test— 459 passedpnpm run buildgit diff --checkmottainai@0.1.0outside the checkoutdoctor,doctor --json, andlist