fix: prevent .mcp.json auto-discovery during plugin development#1
fix: prevent .mcp.json auto-discovery during plugin development#1clintecker wants to merge 3 commits intomainfrom
Conversation
Problem: Having .mcp.json in project root caused Claude Code to auto-discover the MCP server config during development, creating false tool availability and conflicts with properly installed plugin. Solution: Renamed to .mcp.json.example to prevent auto-discovery while still providing documentation. Added .mcp.local.json.example for local development (already gitignored). Changes: - Rename .mcp.json → .mcp.json.example (production config template) - Add .mcp.local.json.example (localhost config template) - Add npm scripts: setup:dev and setup:check - Update all documentation to reference example files - Document the "why" behind this pattern Benefits: - No auto-discovery pollution during development - Clear separation between docs and active configs - Follows .env.example convention - Easy dev setup via npm run setup:dev 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a new "detox" tool with tests and state API to clear active drugs, registers the tool in the MCP HTTP server (with OAuth metadata refactor), introduces local MCP config example and setup scripts, and updates extensive documentation and developer guides. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MCP
participant HTTP_Server as "HTTP Server"
participant DetoxTool as "detoxTool"
participant StateMgr as "StateManager"
participant Logger
User->>MCP: invoke "detox" tool
MCP->>HTTP_Server: tool call (detox)
HTTP_Server->>DetoxTool: detoxTool(stateManager)
activate DetoxTool
DetoxTool->>Logger: log start
DetoxTool->>StateMgr: getActiveDrugs()
alt drugs found
StateMgr-->>DetoxTool: list of drugs
DetoxTool->>StateMgr: clearAllDrugs()
StateMgr-->>DetoxTool: confirmation
DetoxTool->>Logger: log success + elapsed
DetoxTool-->>HTTP_Server: ToolResult (success + removed list)
else no drugs
StateMgr-->>DetoxTool: empty list
DetoxTool->>Logger: log no-op
DetoxTool-->>HTTP_Server: ToolResult (info: nothing to clear)
else error
StateMgr-->>DetoxTool: throws/error
DetoxTool->>Logger: log error + elapsed
DetoxTool-->>HTTP_Server: ToolResult (error + message)
end
deactivate DetoxTool
HTTP_Server-->>MCP: Return ToolResult
MCP-->>User: Display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Added extensive minimalist mermaid diagrams throughout documentation to help developers understand system architecture, data flows, and component interactions. Changes: - CLAUDE.md: Added 3 mermaid diagrams (two-level effects, persistent effect sequence, system overview, OAuth flow) - README.md: Added 3 mermaid diagrams (how it works, architecture components, dev config flow) - PLUGIN_ARCHITECTURE.md: Added 2 mermaid diagrams (two-level effects, plugin components) - docs/LOCAL_TESTING.md: Replaced ASCII diagram with 3 mermaid diagrams (setup, OAuth flow, dev testing) - DEVELOPMENT.md: New comprehensive development guide with 4 mermaid diagrams and troubleshooting All diagrams use consistent styling: - Minimalist and focused on key concepts - Color-coded components (green=local, blue=MCP, gold=production, etc.) - Clear labels and flow directions - Match existing documentation patterns Documentation now provides clear visual guides for: - Immediate vs persistent drug effects - OAuth authentication flows - MCP configuration patterns - Local development setup - Component interactions - Development workflows 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Documentation UpdatesAdded comprehensive visual documentation with mermaid diagrams: New Content
Enhanced Existing Docs
Visual Guides Now Cover:✅ Immediate vs persistent drug effects All diagrams use consistent minimalist styling with color-coding for easy comprehension by run-of-the-mill agentic developers. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/http-server.ts (1)
349-361: Return the local metadata URL in the auth hint.
Point clients at the server’s own metadata endpoint to keep flows local as documented.- oauthMetadataUrl: 'https://us-central1-agent-drugs.cloudfunctions.net/oauthMetadata' + oauthMetadataUrl: `${req.protocol}://${req.get('host')}/.well-known/oauth-authorization-server`docs/LOCAL_TESTING.md (1)
120-132: Add"type": "http"to the manual token example for consistency and correctness.The first two MCP configuration examples in the file correctly include
"type": "http", but the manual token example (lines 120-132) omits it. Since all three examples use the same HTTP endpoint (http://localhost:3000/mcp), the transport type must be explicitly specified for HTTP-based connections per MCP specification.{ "mcpServers": { "agent-drugs-local": { + "type": "http", "url": "http://localhost:3000/mcp", "headers": { "Authorization": "Bearer agdrug_xxx..." } } } }
🧹 Nitpick comments (8)
docs/LOCAL_TESTING.md (2)
72-80: Suggest adding a quick verification step after setup.
After npm run setup:dev (or manual copy), advise running setup:check to fail fast if the file wasn’t created.Apply this small addition:
# Copy the example to create your local config npm run setup:dev # Or manually: cp .mcp.local.json.example .mcp.local.json + +# Verify it exists: +npm run setup:check
84-85: Fix MD lint: avoid bold as a heading.
Use a heading or blockquote to satisfy MD036.Example:
-**Note:** The `.mcp.local.json` file is gitignored and won't be committed... +#### Note +The `.mcp.local.json` file is gitignored and won't be committed...src/tools/detox.ts (3)
14-16: Skip the write when there’s nothing to clear.
Avoid an unnecessary Firestore write in the no‑op case by returning before calling clearAllDrugs.- // Clear all active drugs - await state.clearAllDrugs(); - logger.info('Tool: detox succeeded', { count: drugs.length, drugs: drugNames, elapsed: Date.now() - startTime }); if (drugs.length === 0) { return { content: [{ type: 'text', text: '✨ No active drugs to clear. You\'re already clean!' }], }; } + // Clear only when there were active drugs + await state.clearAllDrugs();Also applies to: 23-31
17-21: Minimize potentially sensitive data in info logs.
Logging full drug names can leak user‑specific behavior. Log counts at INFO and names at DEBUG if needed.-logger.info('Tool: detox succeeded', { count: drugs.length, drugs: drugNames, elapsed: ... }); +logger.info('Tool: detox succeeded', { count: drugs.length, elapsed: Date.now() - startTime }); +logger.debug?.('Tool: detox details', { drugs: drugNames });
49-55: Don’t echo raw error details to the user.
Return a generic error and keep specifics in logs.- text: `Error clearing drugs: ${error instanceof Error ? error.message : 'Unknown error'}`, + text: 'Failed to clear drugs. Please try again.',src/tools/detox.test.ts (1)
55-66: Strengthen the error‑path test.
Verify we don’t attempt a clear when discovery fails.it('should handle errors gracefully', async () => { const mockState = { getActiveDrugs: jest.fn().mockRejectedValue(new Error('Firestore error')), clearAllDrugs: jest.fn(), } as any; const result = await detoxTool(mockState); expect(result.isError).toBe(true); expect(result.content[0].text).toContain('Error'); expect(result.content[0].text).toContain('Firestore error'); + expect(mockState.clearAllDrugs).not.toHaveBeenCalled(); });If you adopt the no‑op optimization in detoxTool, also update the “no drugs” test to expect not.toHaveBeenCalled() for clearAllDrugs.
PLUGIN_ARCHITECTURE.md (1)
73-87: Great call moving to .example configs to prevent auto‑discovery.
Consider linking the setup scripts here for discoverability (npm run setup:dev and setup:check).src/http-server.ts (1)
319-335: Align protocolVersion with logged MCP version.
Docs/logs say MCP 2025‑03‑26; initialize returns 2024‑11‑05. Align to avoid client capability mismatches.- protocolVersion: '2024-11-05', + protocolVersion: '2025-03-26',Confirm this matches the client you’re targeting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.mcp.local.json.example(1 hunks)CLAUDE.md(4 hunks)PLUGIN_ARCHITECTURE.md(2 hunks)README.md(2 hunks)commands/detox.md(1 hunks)docs/LOCAL_TESTING.md(1 hunks)docs/plans/2025-10-13-custom-auth-tokens.md(1 hunks)package.json(1 hunks)src/http-server.ts(4 hunks)src/state-manager.ts(1 hunks)src/tools/detox.test.ts(1 hunks)src/tools/detox.ts(1 hunks)src/tools/take-drug.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/http-server.ts (3)
functions/lib/oauthMetadata.js (2)
req(51-51)metadata(42-42)src/logger.ts (3)
logger(110-110)response(86-94)error(66-78)src/tools/detox.ts (1)
detoxTool(5-57)
src/tools/detox.ts (3)
src/state-manager.ts (1)
StateManager(19-129)src/tools/list-drugs.ts (1)
ToolResult(4-7)src/logger.ts (2)
logger(110-110)error(66-78)
src/tools/detox.test.ts (1)
src/tools/detox.ts (1)
detoxTool(5-57)
🪛 markdownlint-cli2 (0.18.1)
docs/LOCAL_TESTING.md
79-79: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (11)
src/tools/take-drug.ts (1)
4-4: LGTM!The import path change from
../logger.jsto../loggeris consistent with TypeScript conventions and has no functional impact..mcp.local.json.example (1)
1-9: LGTM!The local MCP configuration example is well-structured and serves its purpose for local plugin development testing against localhost:3000.
README.md (1)
111-141: LGTM! Clear and comprehensive local development guidance.The new plugin development section effectively explains:
- Purpose of the example config files
- Local development workflow with
setup:dev- Rationale for avoiding auto-discovery conflicts
The documentation aligns well with the PR's main objective of preventing .mcp.json auto-discovery issues.
Note: The markdownlint warning about Line 113 using emphasis instead of heading is a false positive—the bold text
**For Plugin Development:**is intentionally used for emphasis within the section rather than as a heading.docs/plans/2025-10-13-custom-auth-tokens.md (1)
1151-1165: LGTM!The documentation updates correctly reflect the new local MCP configuration pattern using
.mcp.local.jsonas an alternative to global configuration.CLAUDE.md (4)
11-11: LGTM!The detox tool/command additions to the quick reference are clear and consistent with the rest of the documentation.
Also applies to: 17-17
100-108: LGTM!The detox usage documentation is clear and provides helpful examples of alternative phrasings users might naturally use.
157-183: Excellent documentation of the MCP configuration pattern.This section clearly explains:
- The purpose of each example file
- The development workflow
- The rationale for avoiding auto-discovery conflicts
The explanation directly addresses the core problem described in the PR objectives.
206-206: LGTM!Updated reference to
.mcp.json.exampleis consistent with the new configuration pattern.src/state-manager.ts (1)
113-128: LGTM! Clean implementation of the clearAllDrugs method.The implementation correctly:
- Clears the drugs array
- Maintains userId and agentId metadata
- Uses server timestamp for consistency
- Uses
set()which will create the document if it doesn't exist (appropriate for detox)commands/detox.md (1)
1-14: LGTM! Clear command documentation.The documentation effectively describes:
- The command's purpose and behavior
- Expected confirmation output
- OAuth authentication handling and fallback guidance
The instructions align well with the detox tool implementation and user experience expectations.
docs/LOCAL_TESTING.md (1)
82-83: LGTM — clear explanation of auto‑discovery behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PLUGIN_ARCHITECTURE.md (1)
248-266: Add language specifications to code blocks.Code blocks for TypeScript, JavaScript, and JSON examples lack language identifiers, which violates markdown standards and impacts syntax highlighting.
Apply language identifiers to these code blocks:
-``` +```typescript const boxWidth = 62;-``` +```javascript // Load agent's active drugs from Firestore-``` +```javascript { userId: "user_123",-``` +```json { "name": "2389 Research Plugins",Also applies to: 273-281, 297-309, 391-405
🧹 Nitpick comments (1)
CLAUDE.md (1)
289-295: Consider referencing npm setup scripts.The "Local Testing" section mentions
npm run dev:httpbut could explicitly reference thenpm run setup:devandnpm run setup:checkscripts mentioned in the PR objectives for improved developer guidance on initial setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CLAUDE.md(6 hunks)DEVELOPMENT.md(1 hunks)PLUGIN_ARCHITECTURE.md(3 hunks)README.md(3 hunks)docs/LOCAL_TESTING.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- DEVELOPMENT.md
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/LOCAL_TESTING.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
PLUGIN_ARCHITECTURE.md
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
PLUGIN_ARCHITECTURE.md (1)
128-147: Clear rationale for example file pattern.The MCP Server Configuration section effectively explains the auto-discovery conflict and the solution using
.examplefiles. The documentation clearly contrasts production vs. development templates and lists the available tools including the newdetoxcapability.CLAUDE.md (2)
139-147: Well-documented detox feature.The "Remove All Drugs (Detox)" section clearly explains the feature's purpose and provides consistent usage examples. The note that detox removes drugs from both current and future sessions aligns well with the two-level effect system architecture.
262-287: Excellent explanation of MCP configuration pattern.The MCP Configuration Files section clearly documents the problem, solution, and developer workflow. The rationale for using
.examplefiles is well-explained, and the four-step process provides a clear path for local development. Cross-reference to.env.examplepattern strengthens credibility.
Critical Fixes: - OAuth metadata endpoint now returns local URLs for dev testing - Honor upstream status codes in metadata endpoint - Cross-platform npm scripts (setup:dev, setup:check) using Node.js - Make clearAllDrugs transactional to prevent race conditions Improvements: - Optimize detox to skip write when no drugs active - Move drug names to debug logging (reduce sensitive data exposure) - Return generic error messages to users (keep details in logs) - Add missing "type": "http" to LOCAL_TESTING.md example Documentation: - Add language tags to code blocks in PLUGIN_ARCHITECTURE.md - Add language tags to usage examples in CLAUDE.md - Update detox tests to match new behavior All tests passing. Changes improve reliability, security, and cross-platform support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/setup-check.js (1)
1-15: Simple and effective setup validation script.The script correctly checks for the existence of
.mcp.local.jsonand provides clear feedback. Exit codes are appropriate for use in npm scripts or CI workflows.Optional enhancement: Consider validating that the file contains valid JSON, not just that it exists:
try { fs.statSync(target); + // Optional: validate JSON structure + const content = fs.readFileSync(target, 'utf8'); + JSON.parse(content); console.log('✓ Dev config exists'); process.exit(0); } catch {However, the current implementation is perfectly adequate for a simple existence check.
scripts/setup-dev.js (1)
1-16: Clean setup script with appropriate error handling.The script correctly copies the example configuration to the local development config with clear success/failure messaging.
Optional enhancement: Consider checking if the target already exists to avoid accidentally overwriting local changes:
try { + if (fs.existsSync(target)) { + console.log('✓ .mcp.local.json already exists (skipping)'); + process.exit(0); + } fs.copyFileSync(source, target); console.log('✓ Created .mcp.local.json for local development'); process.exit(0);However, since this is a gitignored file and the script is explicitly named
setup-dev, overwriting is likely acceptable behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CLAUDE.md(7 hunks)PLUGIN_ARCHITECTURE.md(4 hunks)docs/LOCAL_TESTING.md(3 hunks)package.json(1 hunks)scripts/setup-check.js(1 hunks)scripts/setup-dev.js(1 hunks)src/http-server.ts(5 hunks)src/state-manager.ts(1 hunks)src/tools/detox.test.ts(1 hunks)src/tools/detox.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tools/detox.test.ts
- package.json
- src/state-manager.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/tools/detox.ts (3)
src/state-manager.ts (1)
StateManager(19-137)src/tools/list-drugs.ts (1)
ToolResult(4-7)src/logger.ts (2)
logger(110-110)error(66-78)
src/http-server.ts (3)
functions/lib/oauthMetadata.js (2)
req(51-51)metadata(42-42)src/logger.ts (3)
logger(110-110)response(86-94)error(66-78)src/tools/detox.ts (1)
detoxTool(5-64)
scripts/setup-dev.js (1)
scripts/setup-check.js (3)
fs(3-3)path(4-4)target(6-6)
🔇 Additional comments (10)
src/tools/detox.ts (1)
5-63: LGTM! Clean implementation with good privacy practices.The detox tool implementation is well-structured:
- Early return for no-op case avoids unnecessary Firestore writes
- Separates count logging (INFO) from drug names (DEBUG) to minimize sensitive data exposure
- Error handling properly logs details while returning generic user messages
- Follows the established pattern from other tools
CLAUDE.md (3)
11-11: Documentation properly reflects the new detox tool.The detox command is correctly added to both the tools list and available commands, maintaining consistency with the implementation.
Also applies to: 17-17
25-95: Excellent visual explanation of the two-level effect system.The mermaid diagrams and accompanying explanation clearly distinguish between immediate (current session) and persistent (future sessions) effects, making the architecture much easier to understand.
262-287: Well-justified rationale for example file pattern.The explanation of why
.mcp.json.exampleand.mcp.local.json.exampleare used instead of direct config files is clear and addresses the auto-discovery problem mentioned in the PR objectives.docs/LOCAL_TESTING.md (2)
72-84: Setup instructions properly reference the new workflow.The updated instructions correctly point users to
npm run setup:devand clarify that.mcp.local.jsonis gitignored, aligning with the new example file pattern.
158-235: Visual diagrams significantly improve local testing guidance.The three mermaid diagrams (Setup Overview, OAuth Flow, Development Testing Flow) provide clear, color-coded visual representations that will help developers understand the local testing architecture.
src/http-server.ts (2)
32-78: OAuth metadata endpoint correctly implements local rewriting and upstream status handling.The refactored endpoint properly:
- Fetches metadata from upstream Cloud Functions
- Honors upstream HTTP status (returns non-OK status if upstream fails)
- Rewrites endpoints to local URLs for dev testing
- Includes comprehensive logging
This addresses the concerns from the past review comment about returning local endpoints and honoring upstream status.
11-11: Detox tool properly integrated into MCP server.The detox tool is correctly:
- Imported from the tools module
- Added to the tools list with appropriate description
- Wired into the tool invocation switch
Integration follows the established pattern used by other tools.
Also applies to: 441-448, 480-482
PLUGIN_ARCHITECTURE.md (2)
23-89: Architecture diagrams provide excellent visual documentation.The mermaid diagrams clearly illustrate both the two-level effect system and the plugin component structure, making the architecture much more accessible to developers.
128-147: MCP configuration documentation properly explains the example file pattern.The expanded section clearly documents both production and local development templates, explains their purposes, and provides strong rationale for using example files to prevent auto-discovery conflicts during development.
1 participant
Summary
Fixes the MCP configuration auto-discovery conflict that occurs during plugin development. When developing the agent-drugs plugin, the
.mcp.jsonfile in the project root was being auto-discovered by Claude Code, creating false tool availability and conflicts with the properly installed plugin.Problem
Root Cause: Claude Code prioritizes project-local
.mcp.jsonfiles over globally installed plugins. During development:This is especially problematic for plugin developers who face a triple namespace collision:
Solution
Adopted the well-established
.env.examplepattern:.mcp.json→.mcp.json.example(production config template).mcp.local.json.example(localhost config template)setup:devandsetup:checkBenefits
npm run setup:devUsage
For plugin developers:
For plugin users:
.mcp.json.exampleis just documentationChanges
.mcp.json→.mcp.json.example(production template).mcp.local.json.example(development template)package.jsonwith dev setup scriptsREADME.md,CLAUDE.md,PLUGIN_ARCHITECTURE.mddocs/LOCAL_TESTING.mdwith new workflowdocs/plans/2025-10-13-custom-auth-tokens.mdTesting
.mcp.local.jsonis in.gitignorenpm run setup:devcreates correct file.mcp.json.exampleis not auto-discoveredAdditional Context
Also includes work on the
detoxtool (separate feature):commands/detox.md- New slash commandsrc/tools/detox.ts- Tool implementationsrc/tools/detox.test.ts- Testssrc/http-server.ts,src/state-manager.ts,src/tools/take-drug.ts(The detox work was in progress when this fix was needed, so it's included in this PR. Can split if needed.)
Summary by CodeRabbit
New Features
Documentation
Tests
Chores