Skip to content

Conversation

@khandrew1
Copy link
Collaborator

  • Fix Docker

@khandrew1 khandrew1 self-assigned this Dec 12, 2025
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Dec 12, 2025
Signed-off-by: Andrew Khadder <54488379+khandrew1@users.noreply.github.com>
@dosubot dosubot bot added the bug Something isn't working label Dec 12, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Walkthrough

The .dockerignore was replaced with compact, recursive pattern-based exclusions (including LICENSE and README.md) and wildcard Dockerfile patterns. The Dockerfile was rewritten as a multi-stage build (base, deps, build, final) on node:${NODE_VERSION}-alpine, using build-time mounts and npm ci (legacy-peer-deps), copying only build artifacts, running as non-root, and using CMD ["npm","start"]; dumb-init, health checks, and entrypoint wrappers were removed. compose.yaml removes explicit dockerfile/image settings, drops the env-file volume mount, and adjusts port mapping formatting.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c44d52f and 6ee1552.

📒 Files selected for processing (1)
  • compose.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • compose.yaml

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
Dockerfile (1)

63-63: Consider optimizing the CMD to reduce process indirection.

Running npm start adds a layer of process indirection. If npm start simply invokes a binary (e.g., node ./bin/inspector), consider using that directly in CMD to reduce overhead and simplify signal handling.

Example:

- CMD npm start
+ CMD ["node", "./bin/inspector"]

Verify what npm start does in package.json; if it performs essential setup, keep npm start. Otherwise, direct invocation is cleaner.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8891c73 and c44d52f.

⛔ Files ignored due to path filters (1)
  • sdk/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • .dockerignore (1 hunks)
  • Dockerfile (1 hunks)
  • compose.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:54:18.321Z
Learnt from: CR
Repo: MCPJam/inspector PR: 0
File: client/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:54:18.321Z
Learning: Use Vite + React for fast development and optimized builds

Applied to files:

  • Dockerfile
🪛 GitHub Actions: Prettier and Build Check
compose.yaml

[warning] 1-1: Code style issues found in compose.yaml. Run Prettier with --write to fix.


[error] 1-1: Prettier check failed. 'npx prettier --check .' reported that the file is not properly formatted. Run 'npm run prettier-fix' to fix.

🔍 Remote MCP

Summary: Docker Configuration PR Review Context

I've gathered comprehensive information to help review this Docker configuration pull request. Here are the key findings organized by file:

Multi-Stage Build Architecture

Multi-stage builds remove intermediate steps like downloading code, installing dependencies, testing, and building from the final image, reducing additional layers and the overall image size. The PR's shift to a 4-stage build (base, deps, build, and final) aligns with this best practice. Multi-stage builds with Alpine can achieve approximately 10x reduction in size compared to standard images.

Node.js Alpine Image Trade-offs

The switch from node:20-slim to node:${NODE_VERSION}-alpine requires careful consideration:

  • Pros: Alpine Linux is much smaller than most distribution base images (~5MB), leading to much slimmer images in general.

  • Cons: Node.js has no officially supported builds for Alpine Linux (experimental status), and since Alpine relies on musl instead of glibc, the behavior and/or performance of some applications may differ from glibc-based distributions like Debian. Alpine is considered experimental and not officially supported target platform for Node.JS.

Note: For most cases, the slimmer and more efficient choice is node:-slim, and if security vulnerabilities and image size aren't top priorities, node:-slim provides a smoother experience.

Docker Build Caching Strategy

The PR introduces build-time cache mounts, which is sound:

  • Cache mounts are a way to specify a persistent cache location during builds. The cache is cumulative across builds, so even if you need to rebuild a layer, you only download new or changed packages, with unchanged packages being reused from the cache mount.

  • Using the --mount flag with RUN instructions allows specifying cache mounts for locations like /root/.npm, which persists across builds.

.dockerignore Optimization

To increase docker build's performance, files and directories should be excluded by adding a .dockerignore file to the context directory. The PR's shift to pattern-based exclusions aligns with this.

npm ci vs npm install

If package or package-lock.json changes, npm ci will be executed; otherwise, commands execute almost instantly, supporting consistency.

Key Review Considerations

  1. Alpine compatibility risk: Test thoroughly for native module dependencies that may rely on glibc
  2. Caching strategy: Verify that build mounts are properly configured and will function in your CI/CD environment
  3. Non-root user: Running as the node user improves security by reducing attack surface
🔇 Additional comments (4)
.dockerignore (1)

7-32: Well-structured ignore patterns.

The shift to recursive pattern-based exclusions (**/) is cleaner and more maintainable than explicit file lists. This consolidation aligns well with the multi-stage build optimization and reduces unnecessary context in the build process.

Dockerfile (3)

9-26: Multi-stage build and caching strategy are well-designed.

The separation into base, deps, build, and final stages is clean. Using build-time cache mounts on /root/.npm and bind mounts for lockfiles optimizes rebuild speed without polluting the final image. The deps stage separation ensures dependency resolution is cached independently of source changes.


39-45: Security and image size improvements are solid.

Running as the non-root node user reduces the attack surface, and selective copying of only necessary artifacts (dist, bin, node_modules, sdk/dist) keeps the final image lean. Good security posture.

Also applies to: 48-57


15-23: Document the peer dependency conflicts and rationale for --legacy-peer-deps.

Using --legacy-peer-deps with npm ci suppresses npm v7+ peer dependency enforcement and indicates unresolved conflicts in the dependency tree. The flag appears in both the root and SDK installs, suggesting a systematic decision rather than an oversight.

If this is intentional:

  • Document the specific peer dependency conflicts in a comment or README
  • Verify the package-lock.json was generated with this flag to ensure reproducible installs
  • Confirm tests validate that the resulting dependency combination functions correctly

If unresolved, prioritize fixing the conflicts upstream before relying on this workaround.

# Stage 1: Dependencies installation
FROM node:20-slim AS deps
WORKDIR /app
FROM node:${NODE_VERSION}-alpine as base
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check for native C++ dependencies in package.json
if [ -f "package.json" ]; then
  echo "=== Checking package.json for native module indicators ==="
  cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null | head -100
fi

Repository: MCPJam/inspector

Length of output: 176


🌐 Web query:

Node.js official support for Alpine Linux 2024 2025 compatibility

💡 Result:

Short answer — as of Dec 12, 2025:

  • The Node.js project does not publish full official musl/glibc‑compatible Alpine binaries as a primary, fully supported platform; Alpine/musl support is treated as experimental and the project recommends Debian-based images or OS‑vendor binaries for production. [1][2]

  • The official Docker Node images do provide Alpine‑based tags (node:*-alpine) maintained by the Node.js Docker team, and the docker-node repo documents Alpine caveats (use libc6-compat for older Alpine, gcompat from Alpine 3.19+, and that musl can cause compatibility issues). Those Alpine images are maintained but you may hit musl-related runtime/build issues. [3][4]

  • Unofficial musl (Alpine) builds exist (unofficial-builds and community projects) if you need musl‑linked Node binaries, but they are not the Node.js project's primary official binary distribution. [5]

Sources:

  1. Node.js reference on versions & container images (notes Alpine is “experimental”). [2]
  2. Node Official Docker image overview on Docker Hub (Alpine tags listed). [1]
  3. nodejs/docker-node README (Alpine notes: libc6-compat, gcompat, musl caveats). [3]
  4. Docker docs referencing Node images (Alpine tags). [6]
  5. Unofficial Node.js musl builds discussion/repos. [7]

If you want, I can fetch exact lines from any of those sources or show which Node versions currently publish alpine tags. Which would you prefer?


Verify Alpine image compatibility with project dependencies.

Node.js support on Alpine is experimental (uses musl instead of glibc). While Docker maintains Alpine Node images, the Node.js project recommends Debian-based images (node:*-slim) for production. Native modules with glibc assumptions may fail or behave unpredictably on musl.

Before merging, verify:

  1. The application builds and runs without errors on Alpine.
  2. Any native C++ modules (node-gyp dependencies) are compatible with musl.
  3. All critical application paths are tested end-to-end on this image.

If compatibility issues arise, node:${NODE_VERSION}-slim is officially supported, still much smaller than standard images, and avoids musl-related risk.

🤖 Prompt for AI Agents
In Dockerfile around line 3 (FROM node:${NODE_VERSION}-alpine as base), the
Alpine-based Node image may be incompatible with some native modules and
musl-based behavior; verify the app builds and runs on this image by doing a
full container build, running unit/integration/e2e tests, and exercising any
code paths that use native node-gyp modules; if you encounter build or runtime
failures related to musl/glibc, switch the base image to
node:${NODE_VERSION}-slim, update CI workflows to use the same image, and re-run
the test matrix to confirm all native modules and production paths work.

@khandrew1 khandrew1 merged commit 1216262 into main Dec 12, 2025
3 checks passed
@mintlify mintlify bot mentioned this pull request Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants