Restructure MCP to Mastra convention with public-first surface#24
Conversation
…surface - Move MCPServer definitions to src/mastra/mcp/server.ts (Mastra convention) - Add src/mastra/stdio.ts as the bun-native stdio entry point - Clean src/mastra/index.ts to export only the Mastra instance - Move Hono setup into src/server.ts where it belongs - Remove old src/mcp/server.ts and src/mcp-stdio.ts - Default to public surface (3 tools), FPF_MCP_SURFACE=full for all 9 - Update all paths in README, docs, codex config, manifest, server.json - Fix tests to use FPF_MCP_SURFACE=full for expert tool coverage - Add tsup + build:mcp script for NPM-publishable stdio package - Copy FPF-spec.md to src/mastra/public/ for mastra build/deploy Validated: stdio, mastra dev, static build, mastra server deploy. All 38 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughReplaces the Node+tsx MCP stdio entrypoint with a Bun-based stdio ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (cli)
participant Proc as Bun Process
participant Stdio as Mastra Stdio Entrypoint
participant MCP as MCPServer (fpfMemory / fpfMemoryPublic)
participant Logger as Runtime Logger
Dev->>Proc: bun src/mastra/stdio.ts (FPF_MCP_SURFACE?)
Proc->>Stdio: start module
Stdio->>Logger: getRuntimeLogger()
Stdio->>MCP: select server based on FPF_MCP_SURFACE
Stdio->>MCP: server.startStdio()
MCP-->>Stdio: started / rejects
alt started
Stdio->>Logger: log "MCP stdio server start"
else error
MCP-->>Stdio: error
Stdio->>Logger: normalizeErrorMessage(error)
Stdio->>Logger: log "MCP stdio server failed" with cause
Stdio->>Proc: setImmediate -> process.exit(1)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 migrates the MCP server runtime from Node.js to Bun, refactors the Mastra runtime initialization, and updates documentation and configuration files to reflect these changes. Key updates include the introduction of a new stdio entry point and the reorganization of MCP server definitions. Review feedback highlights a syntax error in the README's TOML example, the need for a shebang and structured logging in the new stdio entry point, and the addition of server descriptions to enhance client-side discovery.
| args = ["src/mastra/stdio.ts"] | ||
| cwd = "/absolute/path/to/fpf-memory" | ||
| enabled_tools = [ | ||
| "ask_fpf", | ||
| "query_fpf_spec", | ||
| "get_fpf_index_status", | ||
| "refresh_fpf_index", | ||
| "read_fpf_doc", | ||
| "trace_fpf_path", | ||
| "inspect_fpf_node", | ||
| "inspect_fpf_anchor", | ||
| "expand_fpf_citations", | ||
| ] |
There was a problem hiding this comment.
The closing bracket ] on line 138 is a leftover from the removed enabled_tools array. This will cause a TOML syntax error in the configuration example, making it invalid for copy-pasting into a .codex/config.toml file.
| args = ["src/mastra/stdio.ts"] | |
| cwd = "/absolute/path/to/fpf-memory" | |
| enabled_tools = [ | |
| "ask_fpf", | |
| "query_fpf_spec", | |
| "get_fpf_index_status", | |
| "refresh_fpf_index", | |
| "read_fpf_doc", | |
| "trace_fpf_path", | |
| "inspect_fpf_node", | |
| "inspect_fpf_anchor", | |
| "expand_fpf_citations", | |
| ] | |
| args = ["src/mastra/stdio.ts"] | |
| cwd = "/absolute/path/to/fpf-memory" |
| import { fpfMemory, fpfMemoryPublic } from './mcp/server.js'; | ||
|
|
||
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | ||
|
|
||
| server.startStdio().catch((error) => { | ||
| console.error('Error running MCP server:', error); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
This entry point is missing a shebang (e.g., #!/usr/bin/env bun), which is required for the file to be executed as a standalone binary as intended by the bin field and chmod +x in package.json. Additionally, this implementation uses console.error and bypasses the project's configured runtime-logger, which is a regression in observability compared to the previous src/mcp-stdio.ts.
| import { fpfMemory, fpfMemoryPublic } from './mcp/server.js'; | |
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | |
| server.startStdio().catch((error) => { | |
| console.error('Error running MCP server:', error); | |
| process.exit(1); | |
| }); | |
| #!/usr/bin/env bun | |
| import { getRuntimeLogger } from '../logging/runtime-logger.js'; | |
| import { fpfMemory, fpfMemoryPublic } from './mcp/server.js'; | |
| const logger = getRuntimeLogger(); | |
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | |
| server.startStdio().catch((error) => { | |
| logger.error('Error running MCP server', { | |
| error: error instanceof Error ? error.message : 'Unknown MCP stdio error', | |
| }); | |
| process.exit(1); | |
| }); |
| export const fpfMemory = new MCPServer({ | ||
| name: 'fpf_memory', | ||
| version: '1.0.0', | ||
| tools: fpfMcpTools, | ||
| }); | ||
|
|
||
| export const fpfMemoryPublic = new MCPServer({ | ||
| name: 'fpf_memory', | ||
| version: '1.0.0', | ||
| tools: fpfPublicTools, | ||
| }); |
There was a problem hiding this comment.
The MCPServer instances are missing the description field. Providing a clear description is a best practice for MCP servers as it helps clients (like Claude Desktop or Codex) and users understand the server's purpose and capabilities. The previous implementation had detailed descriptions that should be restored.
| export const fpfMemory = new MCPServer({ | |
| name: 'fpf_memory', | |
| version: '1.0.0', | |
| tools: fpfMcpTools, | |
| }); | |
| export const fpfMemoryPublic = new MCPServer({ | |
| name: 'fpf_memory', | |
| version: '1.0.0', | |
| tools: fpfPublicTools, | |
| }); | |
| export const fpfMemory = new MCPServer({ | |
| name: 'fpf_memory', | |
| version: '1.0.0', | |
| description: 'Local vectorless MCP runtime for FPF-spec.md with full tool surface.', | |
| tools: fpfMcpTools, | |
| }); | |
| export const fpfMemoryPublic = new MCPServer({ | |
| name: 'fpf_memory', | |
| version: '1.0.0', | |
| description: 'FPF-spec query runtime with public tool surface (ask, query, status).', | |
| tools: fpfPublicTools, | |
| }); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
133-142:⚠️ Potential issue | 🟡 MinorInvalid TOML syntax at line 138.
Line 138 contains a stray
]which is invalid TOML. This will cause confusion if users copy-paste this config.📝 Proposed fix
[mcp_servers.fpf_memory] command = "bun" args = ["src/mastra/stdio.ts"] cwd = "/absolute/path/to/fpf-memory" -] required = false startup_timeout_sec = 15 tool_timeout_sec = 60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 133 - 142, The TOML snippet in the README has a stray closing bracket character that breaks parsing; remove the extra `]` after the cwd line so the table and array syntax are valid (ensure the [mcp_servers.fpf_memory] table header remains, the args array stays as ["src/mastra/stdio.ts"], and the cwd string is a plain value), then verify the surrounding code fence and remaining keys (required, startup_timeout_sec, tool_timeout_sec) are left unchanged.
🧹 Nitpick comments (2)
src/mastra/mcp/server.ts (1)
5-15: Consider deduplicating shared MCP server metadata.
nameandversionare repeated in both constructors; extracting a shared constant reduces drift risk.♻️ Suggested refactor
+const baseServerMeta = { + name: 'fpf_memory', + version: '1.0.0', +} as const; + export const fpfMemory = new MCPServer({ - name: 'fpf_memory', - version: '1.0.0', + ...baseServerMeta, tools: fpfMcpTools, }); @@ export const fpfMemoryPublic = new MCPServer({ - name: 'fpf_memory', - version: '1.0.0', + ...baseServerMeta, tools: fpfPublicTools, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mastra/mcp/server.ts` around lines 5 - 15, The name and version are duplicated when creating new MCPServer instances (see fpfMemory and fpfMemoryPublic using MCPServer); extract the shared metadata into a single constant (e.g., serverMeta or baseMcpConfig containing name and version) and reuse it by spreading or referencing that constant when constructing both fpfMemory and fpfMemoryPublic while keeping their distinct tools (fpfMcpTools and fpfPublicTools).package.json (1)
14-15: Consider adding a tsup configuration file for maintainability.The inline
build:mcpscript works, but as complexity grows, atsup.config.tswould provide clearer configuration for the dual ESM/CJS build, banner injection (for the shebang fix), and other options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 14 - 15, Create a tsup.config.ts to replace the inline "build:mcp" script: configure an entry for "src/mastra/stdio.ts", formats ["esm","cjs"], noSplitting true, outDir "dist", and add a banner/shebang (e.g. "#!/usr/bin/env node") so the emitted dist/stdio.js is executable; then update the package.json "build:mcp" script to run tsup using that config (e.g. "tsup --config tsup.config.ts && chmod +x dist/stdio.js") so shebang injection and other options are maintained in a single, maintainable place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 7: The CLI entrypoint lacks a shebang so global installs can fail; add
the line "#!/usr/bin/env node" at the top of src/mastra/stdio.ts (before
imports) so dist/stdio.js has an interpreter, or alternatively update the build
script (the npm script named build:mcp in package.json that invokes tsup) to
inject a banner/shebang via tsup (e.g., set banner.js or equivalent) so the
generated dist/stdio.js contains the shebang and then keep the existing chmod +x
step.
In `@tests/mcp-server.test.ts`:
- Around line 137-142: Add a new test to cover the default/public surface
behavior (don't set FPF_MCP_SURFACE in the env) by creating a harness for the
public surface using startHarness('public'), calling initializeHarness(harness),
then invoking harness.request('tools/list') and asserting the returned tool
names equal ['ask_fpf','query_fpf_spec','get_fpf_index_status']; this ensures
tests that currently spawn the process with spawn(... FPF_MCP_SURFACE: 'full'
...) are complemented by a dedicated test exercising the unset/default behavior
via the startHarness/initializeHarness/harness.request('tools/list') flow.
---
Outside diff comments:
In `@README.md`:
- Around line 133-142: The TOML snippet in the README has a stray closing
bracket character that breaks parsing; remove the extra `]` after the cwd line
so the table and array syntax are valid (ensure the [mcp_servers.fpf_memory]
table header remains, the args array stays as ["src/mastra/stdio.ts"], and the
cwd string is a plain value), then verify the surrounding code fence and
remaining keys (required, startup_timeout_sec, tool_timeout_sec) are left
unchanged.
---
Nitpick comments:
In `@package.json`:
- Around line 14-15: Create a tsup.config.ts to replace the inline "build:mcp"
script: configure an entry for "src/mastra/stdio.ts", formats ["esm","cjs"],
noSplitting true, outDir "dist", and add a banner/shebang (e.g. "#!/usr/bin/env
node") so the emitted dist/stdio.js is executable; then update the package.json
"build:mcp" script to run tsup using that config (e.g. "tsup --config
tsup.config.ts && chmod +x dist/stdio.js") so shebang injection and other
options are maintained in a single, maintainable place.
In `@src/mastra/mcp/server.ts`:
- Around line 5-15: The name and version are duplicated when creating new
MCPServer instances (see fpfMemory and fpfMemoryPublic using MCPServer); extract
the shared metadata into a single constant (e.g., serverMeta or baseMcpConfig
containing name and version) and reuse it by spreading or referencing that
constant when constructing both fpfMemory and fpfMemoryPublic while keeping
their distinct tools (fpfMcpTools and fpfPublicTools).
🪄 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: bc03b575-57e0-4a15-8d42-f64addd689e9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.codex/config.tomlREADME.mddocs/drr/DRR-0001-mcp-first-class-interface.mddocs/mcp-interface.mdmanifest.jsonpackage.jsonserver.jsonsrc/mastra/index.tssrc/mastra/mcp/server.tssrc/mastra/stdio.tssrc/mcp-stdio.tssrc/mcp/server.tssrc/server.tstests/mcp-server.test.ts
💤 Files with no reviewable changes (2)
- src/mcp-stdio.ts
- src/mcp/server.ts
There was a problem hiding this comment.
Pull request overview
Restructures the MCP server and Mastra runtime entrypoints to match Mastra’s project conventions, while switching the default MCP surface to a public-first (3-tool) interface and adding a packaging path for an NPM-distributable stdio entry.
Changes:
- Moved MCP server definitions + stdio entry into
src/mastra/**and simplified Mastra runtime creation to return aMastrainstance directly. - Defaulted MCP tool surface to “public” unless
FPF_MCP_SURFACE=full, and updated tests/docs/configs accordingly. - Added
build:mcp(tsup) and NPM publish metadata (bin,files,private=false) for a distributable stdio artifact.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mcp-server.test.ts | Updates test harness to use new stdio entry and runtime factory API. |
| src/server.ts | Refactors HTTP bootstrap to instantiate Hono + MastraServer directly with createMastraRuntime(). |
| src/mcp/server.ts | Removes old MCP server factory/export location. |
| src/mcp-stdio.ts | Removes old stdio entrypoint. |
| src/mastra/stdio.ts | Adds new stdio entrypoint with public-vs-full surface selection. |
| src/mastra/mcp/server.ts | Adds new MCPServer definitions for full/public tool surfaces. |
| src/mastra/index.ts | Simplifies runtime creation and exports a direct Mastra instance for Mastra build/deploy. |
| server.json | Updates MCP client launcher configuration to new bun stdio entrypoint. |
| README.md | Updates Codex registration + verification instructions and file map. |
| package.json | Enables publishing, adds bin/files, and introduces build:mcp via tsup. |
| manifest.json | Updates local runtime command and removes SSE path entry. |
| docs/mcp-interface.md | Updates transport/codex setup docs and surface semantics (public default). |
| docs/drr/DRR-0001-mcp-first-class-interface.md | Updates the documented stdio entrypoint. |
| bun.lock | Adds tsup dependency and related lock changes. |
| .codex/config.toml | Updates project-scoped Codex MCP server launcher path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | ||
|
|
||
| server.startStdio().catch((error) => { | ||
| console.error('Error running MCP server:', error); |
There was a problem hiding this comment.
The new stdio entry point no longer uses the repo’s structured runtime logger, so it won’t emit the expected startup log line (e.g. MCP stdio server start) into the configured log file. This also makes it harder to correlate MCP stdio lifecycle events with the rest of the runtime logs. Use getRuntimeLogger() here (as the previous entrypoint did) and log a consistent startup message before calling startStdio().
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | |
| server.startStdio().catch((error) => { | |
| console.error('Error running MCP server:', error); | |
| import { getRuntimeLogger } from './runtime/logger.js'; | |
| const logger = getRuntimeLogger(); | |
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | |
| logger.info('MCP stdio server start'); | |
| server.startStdio().catch((error) => { | |
| logger.error('Error running MCP server:', error); |
| server.startStdio().catch((error) => { | ||
| console.error('Error running MCP server:', error); |
There was a problem hiding this comment.
Error handling here bypasses the structured logger and exits the process immediately, which can drop context that other tooling expects in the runtime log file. Prefer logging the failure via getRuntimeLogger().error(...) (including a normalized error message) and then rethrow/exit after the log flushes, keeping parity with the previous stdio entrypoint’s behavior.
| server.startStdio().catch((error) => { | |
| console.error('Error running MCP server:', error); | |
| function normalizeErrorMessage(error: unknown): string { | |
| if (error instanceof Error) { | |
| return error.message; | |
| } | |
| if (typeof error === 'string') { | |
| return error; | |
| } | |
| try { | |
| return JSON.stringify(error); | |
| } catch { | |
| return String(error); | |
| } | |
| } | |
| function getRuntimeLogger() { | |
| return { | |
| async error(message: string, details?: unknown) { | |
| console.error(message, details); | |
| await new Promise<void>((resolve) => setImmediate(resolve)); | |
| }, | |
| }; | |
| } | |
| server.startStdio().catch(async (error) => { | |
| await getRuntimeLogger().error('Error running MCP server', { | |
| error: normalizeErrorMessage(error), | |
| cause: error, | |
| }); |
|
|
||
| ```bash | ||
| node --import tsx src/mcp-stdio.ts | ||
| bun src/mastra/stdio.ts |
There was a problem hiding this comment.
This section says verify-runtime.sh checks the “direct stdio launcher” using bun src/mastra/stdio.ts, but the script currently still starts the direct launcher via node --import tsx src/mcp-stdio.ts. Update either the script or this documentation so they match; otherwise readers will follow a verification path that the script doesn’t actually cover.
| bun src/mastra/stdio.ts | |
| node --import tsx src/mcp-stdio.ts |
| "packageManager": "bun@1.3.5", | ||
| "bin": "dist/stdio.js", | ||
| "files": [ | ||
| "dist", | ||
| "FPF-spec.md" | ||
| ], |
There was a problem hiding this comment.
bin points at dist/stdio.js, but the current source entry (src/mastra/stdio.ts) doesn’t include a shebang. Many package managers invoke bin files directly (symlink), so dist/stdio.js should include #!/usr/bin/env node (or an explicit Bun shebang) to be executable; chmod +x alone won’t make it runnable as a script.
| "dev": "bun --watch src/server.ts", | ||
| "build": "bun build ./src/cli.ts ./src/mcp-stdio.ts ./src/server.ts --outdir dist --target bun", | ||
| "build": "bun build ./src/cli.ts ./src/server.ts --outdir dist --target bun", | ||
| "build:mcp": "tsup src/mastra/stdio.ts --format esm,cjs --no-splitting && chmod +x dist/stdio.js", |
There was a problem hiding this comment.
build:mcp assumes the emitted ESM artifact is exactly dist/stdio.js and then marks it executable. Ensure the bundler always outputs that filename (especially with multiple formats) and add a build-time shebang injection step (e.g., tsup banner) so the published bin entry is actually runnable.
| "build:mcp": "tsup src/mastra/stdio.ts --format esm,cjs --no-splitting && chmod +x dist/stdio.js", | |
| "build:mcp": "tsup src/mastra/stdio.ts --format esm,cjs --out-dir dist --legacy-output --no-splitting --banner.js '#!/usr/bin/env node' && chmod +x dist/stdio.js", |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c991f0d4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const server = process.env.FPF_MCP_SURFACE === 'full' ? fpfMemory : fpfMemoryPublic; | ||
|
|
||
| server.startStdio().catch((error) => { |
There was a problem hiding this comment.
The new stdio entrypoint starts MCP directly but no longer emits the "MCP stdio server start" runtime log event that the repository’s verification workflow still asserts after bun run mcp. In this state, scripts/verify-runtime.sh fails even when stdio startup is otherwise healthy, so local/CI runtime verification is now broken by default unless that log signal is restored (or the verifier is updated in the same change).
Useful? React with 👍 / 👎.
| @@ -1,15 +0,0 @@ | |||
| import { getRuntimeLogger } from './logging/runtime-logger.js'; | |||
There was a problem hiding this comment.
Deleting src/mcp-stdio.ts breaks the existing Node/tsx launch path (node --import tsx src/mcp-stdio.ts), which is still invoked by in-repo automation (scripts/verify-runtime.sh) and likely existing local configs. The command now fails with ERR_MODULE_NOT_FOUND, so this change introduces a hard startup regression unless a shim is left at the old path or all callers are migrated atomically.
Useful? React with 👍 / 👎.
…surface test - Add shebang and runtime logger to src/mastra/stdio.ts (Gemini, Copilot, Codex) - Add description field to MCPServer instances (Gemini) - Fix stray ] bracket in README codex config example (Gemini) - Add test for default public surface (CodeRabbit) - Update verify-runtime.sh to use new stdio path (Copilot, Codex) - Make startHarness accept surface parameter for test flexibility 39/39 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/verify-runtime.sh (1)
50-57: Update stale runtime labels after switching to Bun.Line 50 now runs Bun, but Line 44 and Line 57 still say “Node/tsx”. This makes runtime diagnostics confusing.
Suggested cleanup
-printf '==> Starting MCP stdio server via Node/tsx briefly\n' +printf '==> Starting MCP stdio server via Bun briefly\n' @@ - printf 'Node/tsx MCP runtime exited before verification completed\n' >&2 + printf 'Bun MCP runtime exited before verification completed\n' >&2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-runtime.sh` around lines 50 - 57, The script still labels the runtime as "Node/tsx" even though the launcher was changed to Bun; update user-facing text and any comments to refer to Bun (e.g., change the printf message 'Node/tsx MCP runtime exited before verification completed\n' to 'Bun MCP runtime exited before verification completed\n') and adjust any other occurrences or variables/comments that reference "Node/tsx" (inspect usages around trap/kill/wait and the earlier status message near where Bun is started, and update them to consistently reference Bun or "Bun/tsx" as appropriate, keeping the existing variable node_mcp_pid name if you prefer to avoid a larger refactor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/mcp-server.test.ts`:
- Around line 139-142: The test's env construction spreads process.env which
lets a parent FPF_MCP_SURFACE=full leak into startHarness('public'), so update
the env passed to startHarness (the env object built where surface is used) to
explicitly control FPF_MCP_SURFACE: when surface === 'full' set it to 'full',
otherwise ensure it is unset/empty (e.g., omit the key or set to undefined/''),
rather than inheriting from process.env; apply the same change to the second
occurrence referenced around lines 273-275 so both public-surface tests reliably
exercise the default (unset) path.
---
Nitpick comments:
In `@scripts/verify-runtime.sh`:
- Around line 50-57: The script still labels the runtime as "Node/tsx" even
though the launcher was changed to Bun; update user-facing text and any comments
to refer to Bun (e.g., change the printf message 'Node/tsx MCP runtime exited
before verification completed\n' to 'Bun MCP runtime exited before verification
completed\n') and adjust any other occurrences or variables/comments that
reference "Node/tsx" (inspect usages around trap/kill/wait and the earlier
status message near where Bun is started, and update them to consistently
reference Bun or "Bun/tsx" as appropriate, keeping the existing variable
node_mcp_pid name if you prefer to avoid a larger refactor).
🪄 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: f6a3ad79-124c-45f6-9b6f-1a7b9702d7e2
📒 Files selected for processing (5)
README.mdscripts/verify-runtime.shsrc/mastra/mcp/server.tssrc/mastra/stdio.tstests/mcp-server.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mastra/stdio.ts
- README.md
- src/mastra/mcp/server.ts
| env: { | ||
| ...process.env, | ||
| ...(surface === 'full' ? { FPF_MCP_SURFACE: 'full' } : {}), | ||
| FPF_MASTRA_LOG_PATH: resolve(tempDir, 'mastra.log'), |
There was a problem hiding this comment.
Public-surface test can unintentionally run in full mode.
Because Line 139 spreads process.env, an existing parent FPF_MCP_SURFACE=full leaks into startHarness('public'). That means the “unset/default public” test may not actually exercise the default path.
Proposed fix
async function startHarness(surface: 'public' | 'full' = 'full'): Promise<StdioMcpHarness> {
const tempDir = await mkdtemp(resolve(tmpdir(), 'fpf-mcp-stdio-'));
tempDirs.push(tempDir);
+ const { FPF_MCP_SURFACE: _ignoredSurface, ...baseEnv } = process.env;
const child = spawn('bun', ['src/mastra/stdio.ts'], {
cwd: process.cwd(),
env: {
- ...process.env,
+ ...baseEnv,
...(surface === 'full' ? { FPF_MCP_SURFACE: 'full' } : {}),
FPF_MASTRA_LOG_PATH: resolve(tempDir, 'mastra.log'),
FPF_MASTRA_OBSERVABILITY_PATH: resolve(tempDir, 'mastra-observability.json'),
},Also applies to: 273-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp-server.test.ts` around lines 139 - 142, The test's env construction
spreads process.env which lets a parent FPF_MCP_SURFACE=full leak into
startHarness('public'), so update the env passed to startHarness (the env object
built where surface is used) to explicitly control FPF_MCP_SURFACE: when surface
=== 'full' set it to 'full', otherwise ensure it is unset/empty (e.g., omit the
key or set to undefined/''), rather than inheriting from process.env; apply the
same change to the second occurrence referenced around lines 273-275 so both
public-surface tests reliably exercise the default (unset) path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a4e59712
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "scripts": { | ||
| "dev": "bun --watch src/server.ts", | ||
| "build": "bun build ./src/cli.ts ./src/mcp-stdio.ts ./src/server.ts --outdir dist --target bun", | ||
| "build": "bun build ./src/cli.ts ./src/server.ts --outdir dist --target bun", |
There was a problem hiding this comment.
The package is now publishable ("private": false) and declares "bin": "dist/stdio.js", but the main build script no longer produces that file. On a clean checkout, bun run build emits only dist/cli.js and dist/server.js, so a normal pack/publish flow can ship a package whose executable entrypoint is missing. This makes the installed CLI unusable unless maintainers remember to run build:mcp manually; adding the stdio build to the default packaging path (e.g., build/prepack) would prevent broken releases.
Useful? React with 👍 / 👎.
|
@codex[agent] review |
Co-authored-by: venikman <22530205+venikman@users.noreply.github.com>
Updated stdio to use the runtime logger with normalized error logging, added a node shebang plus build shebang injection for the bin, and aligned verify-runtime docs/script with the direct stdio entry. Commit 774efa6. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 774efa6bdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "FPF-spec.md" | ||
| ], | ||
| "scripts": { | ||
| "dev": "bun --watch src/server.ts", |
There was a problem hiding this comment.
package.json now declares "bin": "dist/stdio.js", but the build script only compiles src/cli.ts and src/server.ts, so the standard build path leaves the published bin missing unless build:mcp is run separately. In environments that run only bun run build before packing/publishing (as suggested by the repo workflow), installs will produce a package whose CLI entrypoint does not exist.
Useful? React with 👍 / 👎.
Summary
src/mastra/mcp/server.ts,src/mastra/stdio.ts, cleansrc/mastra/index.ts)ask_fpf,query_fpf_spec,get_fpf_index_status); setFPF_MCP_SURFACE=fullfor all 9src/mcp/server.tsandsrc/mcp-stdio.tsbuild:mcpscript with tsup for NPM-publishable stdio packageTest plan
bun src/mastra/stdio.ts— 3 tools,ask_fpfreturns okmastra devHTTP server — 200, 3 toolsmastra build+node .mastra/output/index.mjs— 200, 3 tools, confidence 0.98mastra server deploy— deploy succeededfpf-memory.server.mastra.cloud— needs verification from a machine without TLS filter🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
Documentation
Tests