chore: MCP Dockerfile improvements and native Bun runtime#738
Open
phuongvm wants to merge 2 commits into
Open
Conversation
- Multi-stage Docker build (Bun builder → Node 20 runtime) - Non-root user for security (Trivy DS-0002) - Native Bun runtime instead of wrangler dev in CMD - Tightened .dockerignore (root + mcp/) - Bun mock server (run-mocked.ts) for testing Fixes security and deployment anti-patterns in MCP container.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Bun-based containerization and runtime for a mock MCP server: Dockerfile and dockerignore updates, a ChangesMock MCP Server Containerization
Sequence DiagramsequenceDiagram
participant Docker as Docker runtime
participant Bun as Bun runtime (entrypoint)
participant Plugin as cloudflare-mock plugin
participant MCP as ./src/index.ts module
participant HTTP as Bun.serve HTTP server
Docker->>Bun: start container (CMD bun run run-mocked.ts)
Bun->>Plugin: register cloudflare-mock plugin
Bun->>MCP: dynamic import ./src/index.ts
MCP-->>Bun: export default.fetch handler
Bun->>HTTP: Bun.serve(start on HONCHO_MCP_PORT, handler = fetch)
HTTP->>HTTP: serve requests using MCP fetch handler
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
mcp/run-mocked.ts (1)
25-25: 💤 Low valueType assertion bypasses safety checks.
Using
as anydisables TypeScript's type checking. Ifmodule.default.fetchdoesn't match Bun.serve's expected signature, errors will only surface at runtime.♻️ Proposed improvement: Add proper type guard
+ if (typeof module.default?.fetch !== 'function') { + throw new Error('MCP module does not export a fetch handler'); + } Bun.serve({ - fetch: module.default.fetch as any, + fetch: module.default.fetch, port: port, });🤖 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 `@mcp/run-mocked.ts` at line 25, Replace the unsafe "as any" on module.default.fetch with a runtime type guard and safe wrapper: add a small type-guard function (e.g., isFetchHandler) that checks module.default.fetch is a function and accepts a Request (or returns a Response/Promise<Response>), then assign fetch using that guard (e.g., const fetch = isFetchHandler(module.default.fetch) ? module.default.fetch : (() => { throw new Error("module.default.fetch has incompatible signature"); })); reference the property module.default.fetch and the assigned fetch used for Bun.serve so the code fails early with a clear error instead of bypassing TypeScript checks..dockerignore (1)
15-17: 💤 Low valueRedundant pattern:
mcp/.wrangleris already matched by**/.wrangler.Line 16's glob pattern
**/.wranglermatches.wranglerin any subdirectory, making line 17'smcp/.wranglerredundant.♻️ Proposed simplification
.wrangler **/.wrangler -mcp/.wrangler mcp/node_modules🤖 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 @.dockerignore around lines 15 - 17, The .dockerignore contains a redundant pattern: the glob "**/.wrangler" already matches ".wrangler" in any subdirectory so the explicit "mcp/.wrangler" entry should be removed; update the file by deleting the "mcp/.wrangler" line and keep only the general "**/.wrangler" (and the top-level ".wrangler" if intended) to avoid duplication.
🤖 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 `@mcp/Dockerfile`:
- Around line 13-23: The runtime stage currently uses FROM node:20 but then
invokes bun in CMD ["bun","run","run-mocked.ts"], causing a missing-binary
failure; update the runtime stage base image to a Bun-enabled image (e.g., use
the same Bun-based builder image or an oven/bun image) so the Bun binary is
present, keeping WORKDIR /app and the COPY --from=builder /app /app and leaving
CMD unchanged.
- Around line 1-23: The Dockerfile currently runs the container as root; create
a non-root user and switch to it before CMD to satisfy Trivy DS-0002: add steps
in the Dockerfile to create a user/group (e.g., adduser or groupadd/useradd like
"appuser"), chown /app (the copied files from builder) to that user so Bun can
access files, set USER to that non-root user (place USER appuser just before CMD
["bun","run","run-mocked.ts"]), and ensure any directories that need write
access (e.g., /app, /tmp if used) have appropriate ownership/permissions so the
container runs without root privileges.
- Around line 6-7: The Dockerfile currently copies only package.json before
running RUN bun install, causing non-deterministic installs; modify the COPY
instruction(s) so the bun.lock (or bun.lockb) is copied into the image prior to
RUN bun install (e.g., COPY package.json bun.lock ./ or a separate COPY bun.lock
./) so bun can use the lockfile for deterministic dependency resolution; ensure
the COPY lines preceding RUN bun install are updated accordingly.
In `@mcp/run-mocked.ts`:
- Line 23: The port assignment using Number(process.env.HONCHO_MCP_PORT) can
produce NaN for non-numeric values; validate and normalize the environment value
before passing it to Bun.serve by parsing the string (e.g., parseInt) and
checking Number.isFinite/Number.isInteger and that the value is within the valid
TCP port range (1–65535), then fall back to the default 8787 if validation
fails; update the code that sets the port (the port constant using
process.env.HONCHO_MCP_PORT) so Bun.serve always receives a valid numeric port.
---
Nitpick comments:
In @.dockerignore:
- Around line 15-17: The .dockerignore contains a redundant pattern: the glob
"**/.wrangler" already matches ".wrangler" in any subdirectory so the explicit
"mcp/.wrangler" entry should be removed; update the file by deleting the
"mcp/.wrangler" line and keep only the general "**/.wrangler" (and the top-level
".wrangler" if intended) to avoid duplication.
In `@mcp/run-mocked.ts`:
- Line 25: Replace the unsafe "as any" on module.default.fetch with a runtime
type guard and safe wrapper: add a small type-guard function (e.g.,
isFetchHandler) that checks module.default.fetch is a function and accepts a
Request (or returns a Response/Promise<Response>), then assign fetch using that
guard (e.g., const fetch = isFetchHandler(module.default.fetch) ?
module.default.fetch : (() => { throw new Error("module.default.fetch has
incompatible signature"); })); reference the property module.default.fetch and
the assigned fetch used for Bun.serve so the code fails early with a clear error
instead of bypassing TypeScript checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16597fa-090c-4e9a-a378-164afe7d4823
⛔ Files ignored due to path filters (1)
mcp/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.dockerignoremcp/.dockerignoremcp/Dockerfilemcp/package.jsonmcp/run-mocked.ts
- Add bun.lock copy before bun install for deterministic builds (Dockerfile) - Use frozen-lockfile for reproducible installs (Dockerfile) - Change stage 2 base from node:20 to oven/bun:1 (CMD requires bun binary) - Run container as non-root USER bun (Dockerfile) - Validate HONCHO_MCP_PORT to prevent NaN port (run-mocked.ts)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves the MCP server Docker setup with better build performance and native Bun runtime.
Key Changes
npx wrangler devwithbun run run-mocked.ts— bypasses Wrangler's anti-Bun blockerrun-mocked.tsprovides a Cloudflare-free execution path using Bun's plugin system to stubcloudflare:email.wrangler/directories andnode_modulesfrom build contextFiles Changed
mcp/Dockerfile— multi-stage build, native Bun CMDmcp/run-mocked.ts— Bun mock server with Cloudflare stubmcp/.dockerignore— MCP-specific ignoresmcp/package.json/mcp/bun.lock— dependency updates.dockerignore— root-level wrangler exclusionsSplit from #692 per maintainer request to separate MCP infrastructure from LLM fallback.
Summary by CodeRabbit