fix: Remove npm aliases and conflicting dependencies to resolve registerTools errors #90
Conversation
WalkthroughThe changes address a runtime failure caused by npm alias resolution issues when upgrading dependencies. The solution involves replacing the aliased dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant MCP_SDK as @camsoft/mcp-sdk
participant Server
App->>MCP_SDK: Import MCP types/classes
App->>Server: Initialize with MCP_SDK
Server->>MCP_SDK: Call registerTools (available in fork)
MCP_SDK-->>Server: Register tools
Server-->>App: Ready for runtime operations
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @cameroncooke's task —— View job Comprehensive PR ReviewThis PR addresses a critical npm alias resolution failure that was causing 🎯 Overall Assessment: EXCELLENTThis is a well-executed fix with thorough analysis, proper implementation, and comprehensive documentation. The solution directly addresses the root cause while improving the overall dependency management. ✅ Strengths1. Root Cause Analysis Quality
2. Implementation Strategy
3. Code Quality
📋 Technical Review by CategoryCode Quality: 9/10
Security: 10/10
Performance: 9/10
Testing Coverage: 8/10
🔍 Specific Code Reviewpackage.json (Excellent)// Before (Broken)
"@modelcontextprotocol/sdk": "npm:@camsoft/mcp-sdk@^1.17.1"
// After (Working)
"@camsoft/mcp-sdk": "^1.17.1"
Import Updates (Perfect)All 9 source files correctly updated:
All imports follow the pattern: Test Updates (Good)
🚨 Potential Issues & RecommendationsMinor Issues
Long-term Considerations
🔒 Security AssessmentPositive Changes
No Security Risks Introduced
🚀 Performance ImpactPositive Performance Changes
📊 Test Coverage AssessmentComprehensive Coverage
Validation Evidence# Confirms correct package installation
grep -n "registerTools" node_modules/@camsoft/mcp-sdk/dist/esm/server/mcp.js
# Returns: 604: registerTools(tools) {🏆 Final Recommendation: APPROVEDThis PR should be merged because:
Post-Merge Actions Recommended:
📈 Impact Summary
This is exemplary work that combines thorough analysis, proper implementation, and comprehensive testing. The RCA alone is a valuable contribution to the development community's understanding of npm alias limitations. |
|
Cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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 (8)
src/core/resources.ts (2)
14-15: Swap to forked SDK is correct; prefer type-only imports to avoid runtime cost.These are used as types in this file. Use
import typeto prevent unnecessary runtime imports and improve tree-shaking.-import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; -import { ReadResourceResult } from '@camsoft/mcp-sdk/types.js'; +import type { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { ReadResourceResult } from '@camsoft/mcp-sdk/types.js';
73-83: Optional: pass an executor into resource handlers to adhere to injectable executor pattern.Your
ResourceMeta.handlersupports an optionalexecutor, butreadCallbackcalls it without one. If any resource needs shell/filesystem access, pass a default executor to keep external interactions injectable.Example:
import { getDefaultCommandExecutor } from '../utils/command.js'; // ... const readCallback = async (resourceUri: URL): Promise<ReadResourceResult> => { const result = await resource.handler(resourceUri, getDefaultCommandExecutor()); return { contents: result.contents.map((content) => ({ uri: resourceUri.toString(), text: content.text, mimeType: resource.mimeType, })), }; };src/core/dynamic-tools.ts (1)
6-6: Import change is correct; use a type-only import for McpServer.
McpServeris only used for typing function parameters; useimport typeto avoid pulling a runtime binding.-import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { McpServer } from '@camsoft/mcp-sdk/server/mcp.js';Note: The
wrapHandlerWithExecutorpattern correctly injects the default CommandExecutor. Good.src/index.ts (1)
21-21: Use a type-only import for McpServer to avoid unnecessary runtime import.Since
McpServeris used purely for typing the global, preferimport type.-import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { McpServer } from '@camsoft/mcp-sdk/server/mcp.js';src/utils/tool-registry.ts (1)
1-1: Switch to type-only import for McpServer and RegisteredTool.Both are used for typing; using
import typereduces bundle size and avoids runtime bindings.-import { McpServer, RegisteredTool } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { McpServer, RegisteredTool } from '@camsoft/mcp-sdk/server/mcp.js';Bulk
registerToolsusage and tracking logic look solid with the forked SDK.src/core/__tests__/resources.test.ts (1)
2-2: Use a type-only import to avoid runtime dependency in testsMcpServer is used purely for typing the mock; prefer a type-only import to reduce runtime coupling and speed up tests.
-import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { McpServer } from '@camsoft/mcp-sdk/server/mcp.js';Note: Test otherwise complies with our no-vitest-mocking rule and keeps logic isolated. ✔️
src/mcp/tools/discovery/discover_tools.ts (1)
13-13: Prefer a type-only import for McpServer (typing only in this file)This file uses McpServer as a type (interfaces, global cast) and doesn't instantiate it. Switch to a type-only import to avoid unnecessary runtime linkage.
-import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; +import type { McpServer } from '@camsoft/mcp-sdk/server/mcp.js';RCA-npm-alias-resolution-failure.md (1)
25-29: Fix markdown lint issues: code block languages, table spacing, and headingsAddress markdownlint findings to improve readability and tooling compliance.
@@ -``` +```text TypeError: server.registerTools is not a function at Object.handler (/path/to/xcodebuildmcp/build/index.js)@@
-+text
/Volumes/Developer/XcodeBuildMCP/customer-issue-test/
├── package.json # Contains correct xcodebuildmcp@1.11.1
├── package-lock.json # Shows official registry resolution
└── node_modules/
├── xcodebuildmcp/package.json # Shows correct alias specification
└── @modelcontextprotocol/
└── sdk/
├── package.json # Shows official package (BUG)
└── dist/esm/server/mcp.js # Missing registerTools method@@ -### B. File Size Evidence -| Package | File Size | registerTools | Source | -|---------|-----------|---------------|--------| -| Official SDK | 29,473 bytes | ❌ Missing | @modelcontextprotocol/sdk@1.17.1 | -| Fork SDK | 33,842 bytes | ✅ Present | @camsoft/mcp-sdk@1.17.1 | -| **Size Difference** | **+4,369 bytes** | | **Fork adds bulk API** | +### B. File Size Evidence + +| Package | File Size | registerTools | Source | +|---------|-----------|---------------|--------| +| Official SDK | 29,473 bytes | ❌ Missing | @modelcontextprotocol/sdk@1.17.1 | +| Fork SDK | 33,842 bytes | ✅ Present | @camsoft/mcp-sdk@1.17.1 | +| **Size Difference** | **+4,369 bytes** | | **Fork adds bulk API** | + @@ -**Option 1: Maintain Fork Indefinitely** +### Option 1: Maintain Fork Indefinitely @@ -**Option 2: Upstream Integration** +### Option 2: Upstream Integration @@ -**Option 3: Hybrid Approach** +### Option 3: Hybrid ApproachOptional style tweak (line ~182): “completely different” -> “entirely different” for tighter language.
-**Key Insight**: Version ranges are completely different (`^1.6.1` vs `^1.17.1`), ruling out version overlap theories. +**Key Insight**: Version ranges are entirely different (`^1.6.1` vs `^1.17.1`), ruling out version overlap theories.Also applies to: 332-342, 416-421, 587-602
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
RCA-npm-alias-resolution-failure.md(1 hunks)package.json(1 hunks)src/core/__tests__/resources.test.ts(1 hunks)src/core/dynamic-tools.ts(1 hunks)src/core/resources.ts(1 hunks)src/index.ts(1 hunks)src/mcp/tools/discovery/discover_tools.ts(1 hunks)src/server/server.ts(1 hunks)src/utils/tool-registry.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All external interactions (like running commands or accessing the file system) must be handled through injectable executors (
CommandExecutor,FileSystemExecutor).
Files:
src/server/server.tssrc/core/dynamic-tools.tssrc/mcp/tools/discovery/discover_tools.tssrc/utils/tool-registry.tssrc/core/resources.tssrc/core/__tests__/resources.test.tssrc/index.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.test.{ts,tsx,js,jsx}: Do not usevi.mock(),vi.fn(),vi.spyOn(), etc. in tests; Vitest mocking is completely banned.
Tests should import the core...Logicfunction from a tool file and pass in a mock executor (createMockExecutororcreateMockFileSystemExecutor) to simulate different outcomes.
Files:
src/core/__tests__/resources.test.ts
🧠 Learnings (3)
📚 Learning: 2025-08-06T11:44:09.150Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T11:44:09.150Z
Learning: To add a new tool or resource, create a new file in the appropriate directory; it will be automatically loaded and exposed to MCP clients.
Applied to files:
src/mcp/tools/discovery/discover_tools.tssrc/utils/tool-registry.tssrc/core/resources.ts
📚 Learning: 2025-08-06T11:44:09.150Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T11:44:09.150Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should import the core `...Logic` function from a tool file and pass in a mock executor (`createMockExecutor` or `createMockFileSystemExecutor`) to simulate different outcomes.
Applied to files:
src/core/__tests__/resources.test.ts
📚 Learning: 2025-08-06T11:44:09.150Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T11:44:09.150Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Do not use `vi.mock()`, `vi.fn()`, `vi.spyOn()`, etc. in tests; Vitest mocking is completely banned.
Applied to files:
src/core/__tests__/resources.test.ts
🪛 LanguageTool
RCA-npm-alias-resolution-failure.md
[style] ~182-~182: Consider using a different adverb to strengthen your wording.
Context: ...k) Key Insight: Version ranges are completely different (^1.6.1 vs ^1.17.1), ruli...
(COMPLETELY_ENTIRELY)
🪛 markdownlint-cli2 (0.17.2)
RCA-npm-alias-resolution-failure.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
332-332: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
416-416: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
587-587: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
592-592: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
package.json (1)
61-61: Verification complete: SDK alias and dependencies are cleanAll checks passed—no further action required.
- No remaining imports of
@modelcontextprotocol/sdkin source or tests (only in docs/RCA).- No npm alias syntax (e.g.
"npm:@camsoft/mcp-sdk") in code.reloaderoodoes not appear inpackage.jsonor lockfile (only used in docs and tooling scripts).package-lock.jsonhas a single entry for"@camsoft/mcp-sdk": "^1.17.1"and no official SDK orreloaderooentries.
| ```bash | ||
| npm install @modelcontextprotocol/sdk@1.17.2 # Latest official with registerTools | ||
| ``` | ||
|
|
||
| 2. **Import Reversion**: | ||
| ```typescript | ||
| import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; | ||
| ``` |
There was a problem hiding this comment.
Correct rollback note: official SDK does NOT provide registerTools
This contradicts earlier evidence. The official @modelcontextprotocol/sdk lacks registerTools. Adjust the comment to avoid misleading readers.
- npm install @modelcontextprotocol/sdk@1.17.2 # Latest official with registerTools
+ npm install @modelcontextprotocol/sdk@1.17.2 # Official SDK (does NOT include registerTools bulk API)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| npm install @modelcontextprotocol/sdk@1.17.2 # Latest official with registerTools | |
| ``` | |
| 2. **Import Reversion**: | |
| ```typescript | |
| import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; | |
| ``` |
🤖 Prompt for AI Agents
In RCA-npm-alias-resolution-failure.md around lines 529 to 536, the comment
incorrectly states that the official @modelcontextprotocol/sdk includes
registerTools, which it does not. Update the comment to clearly state that the
official SDK version 1.17.2 does not provide registerTools to prevent confusion
and ensure accuracy.
| import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; | ||
| import { StdioServerTransport } from '@camsoft/mcp-sdk/server/stdio.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import migration to @camsoft/mcp-sdk looks correct; add a fast-fail guard to detect wrong SDK at runtime
The switch aligns with the dependency strategy and should resolve alias ambiguity. To harden against future regressions (e.g., lockfile drift pulling the official SDK), add a startup assertion that validates the fork-only API is present.
Apply this diff to fail fast when the wrong SDK is installed:
export async function startServer(server: McpServer): Promise<void> {
+ // Sanity check: ensure fork SDK with bulk registration is present
+ // Avoids subtle runtime failures if the official SDK is accidentally resolved.
+ if (typeof (server as any).registerTools !== 'function') {
+ throw new Error(
+ 'registerTools method missing. Likely incorrect SDK resolved. Expected @camsoft/mcp-sdk.',
+ );
+ }
const transport = new StdioServerTransport();
await server.connect(transport);
log('info', 'XcodeBuildMCP Server running on stdio');
}To ensure no residual imports remain and dependencies are clean, run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning for any remaining '@modelcontextprotocol/sdk' references..."
rg -n --hidden --glob '!.git' --glob '!node_modules' "@modelcontextprotocol/sdk"
echo "Confirming all imports reference '@camsoft/mcp-sdk'..."
rg -n --hidden --glob '!.git' --glob '!node_modules' "@camsoft/mcp-sdk"
echo "Checking package.json dependency declarations..."
if command -v jq >/dev/null 2>&1; then
jq '{deps: .dependencies, devDeps: .devDependencies, peerDeps: .peerDependencies}' package.json
else
cat package.json
fi | sed -n '1,200p'
echo "Verifying 'reloaderoo' is not present..."
rg -n --hidden --glob '!.git' --glob '!node_modules' "reloaderoo" || echo "OK: no reloaderoo found"
echo "Lockfile sanity (if present): ensure camsoft appears and official sdk does not override it unexpectedly..."
rg -n --hidden --glob '!.git' "camsoft/mcp-sdk" package-lock.json || true
rg -n --hidden --glob '!.git' "@modelcontextprotocol/sdk" package-lock.json || trueLength of output: 31463
Add startup assertion to fail fast if the official SDK is loaded
All code‐level imports now reference @camsoft/mcp-sdk, and both package.json and package-lock.json are free of @modelcontextprotocol/sdk aliases. To guard against any future alias drift or npm alias resolution bugs, please add a runtime check at the top of your startServer function in src/server/server.ts:
export async function startServer(server: McpServer): Promise<void> {
+ // Sanity check: ensure fork SDK with bulk registration is present
+ if (typeof (server as any).registerTools !== 'function') {
+ throw new Error(
+ 'registerTools method missing. Likely incorrect SDK resolved. Expected @camsoft/mcp-sdk.',
+ );
+ }
const transport = new StdioServerTransport();
await server.connect(transport);
log('info', 'XcodeBuildMCP Server running on stdio');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { McpServer } from '@camsoft/mcp-sdk/server/mcp.js'; | |
| import { StdioServerTransport } from '@camsoft/mcp-sdk/server/stdio.js'; | |
| export async function startServer(server: McpServer): Promise<void> { | |
| // Sanity check: ensure fork SDK with bulk registration is present | |
| if (typeof (server as any).registerTools !== 'function') { | |
| throw new Error( | |
| 'registerTools method missing. Likely incorrect SDK resolved. Expected @camsoft/mcp-sdk.', | |
| ); | |
| } | |
| const transport = new StdioServerTransport(); | |
| await server.connect(transport); | |
| log('info', 'XcodeBuildMCP Server running on stdio'); | |
| } |
🤖 Prompt for AI Agents
In src/server/server.ts around lines 15 to 16, add a runtime assertion at the
start of the startServer function to verify that the official SDK package
'@camsoft/mcp-sdk' is loaded and not '@modelcontextprotocol/sdk'. Implement a
check that throws an error immediately if the wrong SDK is detected, ensuring
the server fails fast on startup to prevent alias drift or resolution bugs.
1 participant
Summary
This PR completely resolves the npm alias resolution failure that was causing
registerTools is not a functionerrors for customers upgrading from xcodebuildmcp@1.10.4 to 1.11.1. The solution eliminates npm's broken alias mechanism and removes conflicting dependencies that were pulling in the wrong SDK package.Root Cause Analysis
As documented in RCA-npm-alias-resolution-failure.md, the issue was caused by:
@modelcontextprotocol/sdkpackage despite explicitnpm:@camsoft/mcp-sdk@^1.17.1alias specificationreloaderoowas accidentally included as a production dependency, pulling in the official SDK without theregisterToolsbulk API methodEvidence of the Problem
Before Fix - Wrong Package Installed:
After Fix - Correct Package with registerTools:
Solution Implemented
1. Direct Fork Dependency (No Aliases)
Before (Broken):
{ "dependencies": { "@modelcontextprotocol/sdk": "npm:@camsoft/mcp-sdk@^1.17.1" } }After (Working):
{ "dependencies": { "@camsoft/mcp-sdk": "^1.17.1" } }2. Removed Conflicting Dependencies
Eliminated
reloaderoofrom production dependencies which was pulling in the official SDK package and causing resolution conflicts.3. Clean Package Resolution
Ensured single, unambiguous path to the correct fork with
registerToolsbulk API method.Testing Results
Comprehensive Black Box Testing via Reloaderoo:
Key Validation:
Benefits
Customer Impact
Before Fix:
TypeError: server.registerTools is not a functionAfter Fix:
Files Changed
Deployment Notes
This fix addresses the exact scenario documented in the RCA:
The solution provides a robust, long-term fix that eliminates dependency on npm's broken alias resolution mechanism while ensuring all customers get the correct SDK with the registerTools bulk API method.
References
Summary by CodeRabbit