Export MCP register tools functions #631
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoves import-time MCP server startup, adds a dedicated CLI entrypoint, re-exports language-specific MCP tool registration functions from the package entry, updates package.json packaging fields, and adds two changeset files for minor releases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant StdioTransport
participant ServerFactory
User->>CLI: run contracts-mcp
CLI->>StdioTransport: new StdioServerTransport()
CLI->>ServerFactory: createServer()
CLI->>ServerFactory: server.connect(transport)
StdioTransport-->>ServerFactory: exchange messages
sequenceDiagram
participant Consumer
participant Library
participant Server
Consumer->>Library: import { register*Tools }
Consumer->>Server: register*Tools(server)
Note over Consumer,Library: import has no side effects
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
User descriptionFixes #626 This allows consumers to import the functions as rather than PR TypeEnhancement Description
Diagram Walkthroughflowchart LR
A["CLI code"] --> B["Separate cli.ts file"]
C["index.ts"] --> D["Export register functions"]
E["package.json"] --> F["Update exports & bin config"]
|
| Relevant files | |||||
|---|---|---|---|---|---|
| Enhancement |
| ||||
| Miscellaneous |
| ||||
| Configuration changes |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.changeset/loud-clowns-divide.md (1)
1-6: Clarify the rationale for the minor bump (nit).If there’s no API change in wizard-common, consider adding a brief reason (e.g., alignment with MCP publishing cadence or dependency surface consistency) to aid future maintainers.
packages/mcp/package.json (1)
12-18: Consider adding an engines constraint.The MCP SDK typically targets modern Node versions. Add an "engines" field (e.g., Node >= 18) to make failures explicit during install if a user is on an unsupported runtime.
Add outside the shown block:
"engines": { "node": ">=18" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/famous-masks-cheat.md(1 hunks).changeset/loud-clowns-divide.md(1 hunks)packages/mcp/package.json(2 hunks)packages/mcp/src/cli.ts(1 hunks)packages/mcp/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/mcp/src/cli.ts (1)
packages/mcp/src/server.ts (1)
createServer(8-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - openzeppelin-contracts-wizard
- GitHub Check: Header rules - openzeppelin-contracts-wizard
- GitHub Check: Pages changed - openzeppelin-contracts-wizard
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
.changeset/famous-masks-cheat.md (1)
1-6: Release note is accurate and scoped.This changeset clearly communicates the minor bump to export the MCP tools registration functions. Good alignment with the PR’s objective.
packages/mcp/src/cli.ts (2)
3-4: No change required — package is CommonJS so explicit “.js” is not neededSummary: packages/mcp/tsconfig.json sets "module": "commonjs" and packages/mcp/package.json does not declare "type":"module" (it's null), so the current relative import without a ".js" extension is fine. If you later migrate this package to ESM/NodeNext, update the import to "./server.js" and adjust moduleResolution.
Files to note:
- packages/mcp/src/cli.ts
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; import { createServer } from './server';
1-1: Shebang preservation check — build not run; cannot verify dist/cli.jsThe compiled file packages/mcp/dist/cli.js was not found when I ran the verification script, so I couldn't confirm the shebang is preserved in the build output.
- packages/mcp/src/cli.ts — contains "#!/usr/bin/env node" (source).
- packages/mcp/dist/cli.js — missing; run the build and re-check.
Run locally to verify:
pnpm -w -r build if [ -f packages/mcp/dist/cli.js ]; then head -n 1 packages/mcp/dist/cli.js; else echo "dist/cli.js not found; build step likely not run."; fipackages/mcp/src/index.ts (1)
1-4: LGTM: clean, side‑effect‑free re‑exports.This meets the PR goal of exposing registration functions from the package root. Nice separation from the CLI bootstrap.
packages/mcp/package.json (1)
8-8: Top-level types exposure looks good.The "types" field correctly points at the declaration entry for root exports and aligns with the re-exports in src/index.ts.
packages/mcp/package.json
Outdated
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "require": "./dist/index.js", | ||
| "default": "./dist/index.js" | ||
| }, | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ESM named import compatibility: add an “import” export or ship an ESM build.
The PR’s usage example is import { registerSolidityTools } from "@openzeppelin/contracts-mcp". With the current exports map only specifying “require” and “default” (and likely emitting CJS), ESM consumers may not get named exports at runtime. Node ESM treats CJS as a default-only export.
Options:
- Preferred: dual publish with ESM and CJS, and wire exports accordingly.
- Minimal: add an "import" condition. This alone won’t enable named exports if the target is CJS, but it’s a step if you later emit ESM.
Apply at least this minimal change:
"exports": {
".": {
"types": "./dist/index.d.ts",
- "require": "./dist/index.js",
- "default": "./dist/index.js"
+ "import": "./dist/index.js",
+ "require": "./dist/index.js",
+ "default": "./dist/index.js"
},
"./package.json": "./package.json"
},If you can emit dual outputs, wire like this and update your build scripts:
"exports": {
".": {
"types": "./dist/index.d.ts",
- "require": "./dist/index.js",
- "default": "./dist/index.js"
+ "import": "./dist/index.js",
+ "require": "./dist/index.cjs"
},
"./package.json": "./package.json"
},Verification script to check module targets across the repo:
🏁 Script executed:
#!/bin/bash
# Show package type, exports, and bin for MCP
echo "packages/mcp/package.json:"
cat packages/mcp/package.json | jq '{ name, type, main, exports, bin }'
# Inspect TS configs to determine module target
fd -a --hidden --exclude node_modules 'tsconfig*.json' \
| while read -r f; do echo "==> $f"; cat "$f"; echo; done
# Find any relative imports without .js that would need updates for ESM NodeNext
rg -n "from '\\./[^']+'|from \\"\\./[^\\"]+\\"" packages/mcp/src | sed 's/^/RELATIVE IMPORT: /'Length of output: 4847
ESM named-imports will break for ESM consumers — emit ESM or add an import export
Verified: packages/mcp/package.json only exposes "require" and "default" and packages/mcp/tsconfig.json sets "module": "commonjs". The package is being built as CommonJS, so Node ESM consumers doing import { ... } from "@openzeppelin/contracts-mcp" will not get named exports at runtime.
Files to change:
- packages/mcp/package.json — exports map (currently missing "import")
- packages/mcp/tsconfig.json / build pipeline — currently emits CommonJS
Minimal change (adds an "import" condition; note: pointing it at a CJS file does NOT enable true named ESM exports at runtime, it's only preparatory):
"exports": {
".": {
"types": "./dist/index.d.ts",
-
"require": "./dist/index.js", -
"default": "./dist/index.js"
-
"import": "./dist/index.js", -
"require": "./dist/index.js", -
},
"default": "./dist/index.js"
"./package.json": "./package.json"
},
Preferred fix (publish dual outputs: ESM + CJS, update build to emit ESM and CJS and wire exports accordingly):
"exports": {
".": {
"types": "./dist/index.d.ts",
-
"require": "./dist/index.js", -
"default": "./dist/index.js"
-
"import": "./dist/index.js", # ESM build -
},
"require": "./dist/index.cjs" # CJS build
"./package.json": "./package.json"
},
Action required: produce an actual ESM build (or dual builds) and update exports as above; adding "import" to point at a CJS artifact alone will not fix named-export behavior for ESM consumers.
🤖 Prompt for AI Agents
packages/mcp/package.json lines 12-18: the exports map only exposes "require"
and "default" while the package is built as CommonJS, which breaks named ESM
imports; to fix, add an "import" condition and produce real ESM output
(preferred: dual ESM+CJS). Change the exports to include an "import" entry
pointing at the ESM build and keep "require" pointing at the CJS build, update
package "type" or file extensions accordingly, then update the build
pipeline/tsconfig to emit both module formats (e.g., add an ES module target in
tsconfig or add a separate rollup/tsc step to emit dist/index.mjs (or .js under
"module":"ESNext") and dist/index.cjs), adjust package.json build scripts to
produce both artifacts, and finally update exports to map
"import"->dist/index.mjs (ESM) and "require"->dist/index.cjs (CJS).
There was a problem hiding this comment.
removed the exports section instead. assess.
There was a problem hiding this comment.
Ask❓
removed the exports section instead. assess.
Answer:
Assessment:
- The PR did not remove exports; it refactored index.ts to export the MCP tool registration functions at the package root:
- Added named exports: registerSolidityTools, registerCairoTools, registerStellarTools, registerStylusTools.
- This enables consumers to import from "@openzeppelin/contracts-mcp" as intended in the PR description.
- The server startup code previously in src/index.ts was moved to a new src/cli.ts, and package.json’s "bin" now points to dist/cli.js. This cleanly separates the CLI entry point from the library exports.
- package.json adds "types": "dist/index.d.ts" to provide TypeScript typings for the new public API.
Impact:
- Breaking change for consumers who relied on the package default behavior of starting the MCP server when importing the module. Now, importing the package will only provide exports; it won’t start a server. However, CLI usage via
contracts-mcpremains intact via the new cli.js. - Positive change for downstream packages: they can import registration functions directly from the package root rather than deep paths.
Suggestions:
- Confirm there are no other symbols that should be exported at the root (e.g., any shared types used by the registration functions) to provide a complete public API.
- Add a brief README section:
- How to run the server via CLI (unchanged).
- How to programmatically register tools:
import { registerSolidityTools } from "@openzeppelin/contracts-mcp"; - Note the change in behavior: importing the package no longer boots a server.
- Ensure the build emits dist/cli.js and dist/index.d.ts. The tsconfig and build scripts should include src/cli.ts and generate type declarations for src/index.ts.
- Consider a deprecation note or minor version bump (already included via changesets) explaining the change in entrypoint behavior for users who might have imported the package expecting side effects.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of packages/mcp/package.json after exports removal
echo "=== Current packages/mcp/package.json ==="
cat packages/mcp/package.json
echo -e "\n=== Check if index.ts properly exports the functions ==="
cat packages/mcp/src/index.ts
echo -e "\n=== Verify TypeScript configuration ==="
cat packages/mcp/tsconfig.json | jq '{ compilerOptions: { module: .compilerOptions.module, target: .compilerOptions.target, moduleResolution: .compilerOptions.moduleResolution } }'
echo -e "\n=== Check what gets built in dist/ ==="
ls -la packages/mcp/dist/ 2>/dev/null || echo "dist/ directory not found"
Length of output: 2110
@ericglau Removing the exports section is actually a reasonable pragmatic approach that should work for your use case.
Assessment: ✅ This should work
Why this approach works:
- Node.js falls back to legacy
"main"field resolution (dist/index.js) - Your
src/index.tsproperly exports the functions with named exports - Modern Node.js versions handle named imports from CommonJS modules when using legacy resolution
- ESM consumers should be able to use
import { registerSolidityTools } from "@openzeppelin/contracts-mcp"
Tradeoffs vs. the original exports approach:
- ✅ Simpler: Avoids the complexity of dual ESM/CJS publishing
- ✅ Works now: Should handle the immediate need for named imports
⚠️ Less explicit: Relies on Node.js interop rather than explicit module resolution⚠️ Future-proofing: Less control over how different environments resolve the module
Alternative considered: The original suggestion to add proper ESM support would be more robust long-term, but this simplified approach should meet your current requirements without the additional build complexity.
Your approach prioritizes pragmatism over theoretical completeness, which makes sense for this PR's scope.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
|
@SocketSecurity ignore npm/base-x@3.0.10 |
|
@SocketSecurity ignore npm/cookie@0.4.2 |
3 participants
Fixes #626
This allows consumers to import the functions as
rather than