mcp-adr-analysis-server

# MCP Best Practices Violations - Newer Tools Audit **Date**: 2025-10-07 **Issue Type**: ✅ **RESOLVED** - Progress & Logging Notifications Now Implemented **Affected Tools**: 6 newer tools (4 critical fixes completed, 2 optional remaining) --- ## Executive Summary The 6 newer tools **violate MCP best practices** from the course notes regarding **Progress and Logging Notifications**. According to the notes: > "Tool functions automatically receive context argument as last parameter. Context object provides methods: info() for logging, report_progress() for progress updates." **Violation**: None of the newer tools accept or use a `context` parameter, preventing them from providing real-time feedback during long-running operations. **Impact**: Users experience poor UX with no visibility into tool execution progress, making long-running operations appear stalled. --- ## The MCP Best Practice (From Course Notes) ### **What Should Happen:** ```typescript // ✅ CORRECT - Following MCP best practices export async function myTool( args: ToolArgs, context: ToolContext // ← Context as last parameter ): Promise<CallToolResult> { context.info('Starting operation...'); // ← Logging context.report_progress(0, 100); // ← Progress updates // Do work... context.info('Processing files...'); context.report_progress(50, 100); // More work... context.info('Finalizing...'); context.report_progress(100, 100); return result; } ``` **Benefits** (from course notes): - ✅ Prevents user confusion about stalled/failed tool calls - ✅ Provides visibility into long-running operations - ✅ Real-time feedback during tool execution - ✅ Optional feature for UX enhancement --- ## Affected Tools Analysis ### **1. conversation-memory-tool.ts** 🔴 **Current Signature:** ```typescript export async function expandMemory( args: { expandableId: string; section?: string; includeContext?: boolean; }, memoryManager: ConversationMemoryManager // ← No context parameter ): Promise<CallToolResult> ``` **Problem**: ❌ No context parameter, no progress/logging **Duration**: Fast (memory lookup) **Severity**: 🟢 **LOW** - Operation is quick, progress not critical **Fix Needed:** ```typescript export async function expandMemory( args: { expandableId: string; section?: string; includeContext?: boolean; }, memoryManager: ConversationMemoryManager, context?: ToolContext // ← Add context parameter ): Promise<CallToolResult> { context?.info('Retrieving expanded content...'); // ... rest of implementation } ``` --- ### **2. expand-analysis-tool.ts** 🔴 **Current Signature:** ```typescript export async function expandAnalysisSection( params: ExpandAnalysisParams // ← No context parameter ): Promise<{ content: Array<{ type: 'text'; text: string }>; isError?: boolean }> ``` **Problem**: ❌ No context parameter, no progress/logging **Duration**: Fast (tiered response lookup) **Severity**: 🟢 **LOW** - Operation is quick **Fix Needed:** ```typescript export async function expandAnalysisSection( params: ExpandAnalysisParams, context?: ToolContext // ← Add context parameter ): Promise<{ content: Array<{ type: 'text'; text: string }>; isError?: boolean }> { context?.info(`Expanding section: ${params.section || 'full analysis'}`); // ... rest of implementation } ``` --- ### **3. perform-research-tool.ts** 🔴 **Current Signature:** ```typescript export async function performResearch(args: { question: string; projectPath?: string; adrDirectory?: string; confidenceThreshold?: number; performWebSearch?: boolean; }): Promise<any> // ← No context parameter ``` **Problem**: ❌ No context parameter in tool function **Partial Fix**: Wrapper method in `index.ts` HAS context but doesn't pass it down **Duration**: **SLOW** (multi-source research: files → knowledge graph → web) **Severity**: 🔴 **CRITICAL** - Long-running operation needs progress feedback **Current Wrapper (index.ts:6586):** ```typescript private async performResearch( args: Record<string, unknown>, context?: ToolContext // ← Has context here ): Promise<CallToolResult> { const ctx = context || createNoOpContext(); ctx.info('📊 Phase 1: Initializing research framework'); ctx.report_progress(10, 100); // ... more progress updates in wrapper ... // ❌ BUT: Calls tool function WITHOUT context! const { performResearch } = await import('./tools/perform-research-tool.js'); return await performResearch(args); // ← Context NOT passed! } ``` **Fix Needed:** ```typescript // In perform-research-tool.ts export async function performResearch( args: { question: string; projectPath?: string; adrDirectory?: string; confidenceThreshold?: number; performWebSearch?: boolean; }, context?: ToolContext // ← Add context parameter ): Promise<any> { context?.info('🔍 Searching project files...'); context?.report_progress(25, 100); // Research from files... context?.info('📊 Querying knowledge graph...'); context?.report_progress(50, 100); // Query knowledge graph... context?.info('🌐 Performing web search...'); context?.report_progress(75, 100); // Web search... context?.info('✅ Research complete'); context?.report_progress(100, 100); return results; } // In index.ts wrapper - PASS context down return await performResearch(args, ctx); // ← Pass context! ``` --- ### **4. llm-cloud-management-tool.ts** 🔴 **Current Signature:** ```typescript export async function llmCloudManagement(args: { provider: 'aws' | 'azure' | 'gcp' | 'redhat' | 'ubuntu' | 'macos'; action: string; parameters?: Record<string, any>; llmInstructions: string; researchFirst?: boolean; projectPath?: string; adrDirectory?: string; }): Promise<any> // ← No context parameter ``` **Problem**: ❌ No context parameter, no progress/logging **Duration**: **SLOW** (research + command generation + execution) **Severity**: 🔴 **CRITICAL** - Cloud operations are slow and critical **Current Wrapper (index.ts):** ```typescript private async llmCloudManagement( args: Record<string, unknown> ): Promise<CallToolResult> { // ❌ No context parameter AT ALL! const { llmCloudManagement } = await import('./tools/llm-cloud-management-tool.js'); return await llmCloudManagement(args); } ``` **Fix Needed:** ```typescript // In llm-cloud-management-tool.ts export async function llmCloudManagement( args: { /* ... */ }, context?: ToolContext // ← Add context parameter ): Promise<any> { context?.info(`🔧 Initializing ${args.provider} management...`); context?.report_progress(0, 100); if (args.researchFirst) { context?.info('📚 Researching best practices...'); context?.report_progress(20, 100); // Research phase... } context?.info('🤖 Generating commands with LLM...'); context?.report_progress(50, 100); // LLM generation... context?.info('☁️ Executing cloud operation...'); context?.report_progress(80, 100); // Execute... context?.info('✅ Operation complete'); context?.report_progress(100, 100); return results; } // In index.ts - Add context parameter and pass it down private async llmCloudManagement( args: Record<string, unknown>, context?: ToolContext // ← Add context ): Promise<CallToolResult> { const ctx = context || createNoOpContext(); const { llmCloudManagement } = await import('./tools/llm-cloud-management-tool.js'); return await llmCloudManagement(args, ctx); // ← Pass context! } ``` --- ### **5. llm-database-management-tool.ts** 🔴 **Current Signature:** ```typescript export async function llmDatabaseManagement(args: { database: string; action: string; parameters?: Record<string, any>; llmInstructions: string; // ... }): Promise<any> // ← No context parameter ``` **Problem**: ❌ No context parameter, no progress/logging **Duration**: **SLOW** (database operations can be lengthy) **Severity**: 🔴 **CRITICAL** - Database operations need progress feedback **Fix**: Same pattern as llm-cloud-management-tool --- ### **6. llm-web-search-tool.ts** 🔴 **Current Signature:** ```typescript export async function llmWebSearch(args: { query: string; maxResults?: number; includeContent?: boolean; llmInstructions?: string; // ... }): Promise<any> // ← No context parameter ``` **Problem**: ❌ No context parameter, no progress/logging **Duration**: **SLOW** (web search with Firecrawl + LLM analysis) **Severity**: 🔴 **CRITICAL** - Web search is slow, needs progress updates **Fix**: Same pattern as llm-cloud-management-tool --- ## Severity Summary | Tool | Operation Speed | Severity | Priority | |------|-----------------|----------|----------| | `conversation-memory-tool` | Fast | 🟢 Low | P3 - Optional | | `expand-analysis-tool` | Fast | 🟢 Low | P3 - Optional | | `perform-research-tool` | **Slow** | 🔴 Critical | **P1 - Must Fix** | | `llm-cloud-management-tool` | **Slow** | 🔴 Critical | **P1 - Must Fix** | | `llm-database-management-tool` | **Slow** | 🔴 Critical | **P1 - Must Fix** | | `llm-web-search-tool` | **Slow** | 🔴 Critical | **P1 - Must Fix** | --- ## Impact Analysis ### **User Experience Impact** 🔴 **Without Progress/Logging (Current State):** ``` User: "Research how authentication works in this project" [30 seconds pass with no feedback] [User thinks it's broken] [60 seconds pass] [User cancels operation] ``` **With Progress/Logging (Correct Implementation):** ``` User: "Research how authentication works in this project" 🔍 Searching project files... [Progress: 25%] 📊 Querying knowledge graph... [Progress: 50%] 🌐 Performing web search... [Progress: 75%] ✅ Research complete [Progress: 100%] [Results displayed] ``` ### **Production Impact** (HTTP Transport) From course notes: > "Setting certain flags to true breaks the workaround, making StreamableHTTP complex to understand and use properly" **Critical Point**: If deploying with `stateless=true` or `jsonResponse=true`: - ❌ Progress notifications won't work anyway - ❌ Logging statements will be lost - ❌ Users get NO feedback at all **Current Risk**: - These tools will have **ZERO feedback** in stateless HTTP deployment - Even worse than stdio transport (which would at least log to stdout) --- ## Comparison with Older Tools ### **✅ Older Tools That Got It Right** ```typescript // Example: analyzeProjectEcosystem (index.ts:4033) private async analyzeProjectEcosystem( args: AnalyzeProjectEcosystemArgs, context?: ToolContext // ← Has context parameter ): Promise<CallToolResult> { const ctx = context || createNoOpContext(); ctx.info('🌐 Starting comprehensive ecosystem analysis'); ctx.report_progress(0, 100); ctx.info('📊 Phase 1: Analyzing project structure'); ctx.report_progress(10, 100); ctx.info('🧠 Phase 2: Generating architectural knowledge'); ctx.report_progress(25, 100); // ... continues with detailed progress updates ... } ``` **Why It Works:** - ✅ Context parameter present - ✅ Progress updates at each phase - ✅ Informative logging messages - ✅ Users see exactly what's happening --- ## Root Cause Analysis ### **Why This Happened** 1. **Timeline**: Newer tools added after Phase 1-3 context decay work 2. **Pattern Inconsistency**: Older tools follow MCP pattern, newer tools don't 3. **No Enforcement**: TypeScript doesn't enforce context parameter 4. **Missing Documentation**: No guidelines on tool creation patterns 5. **Code Review Gap**: Pattern violation not caught in review ### **Why It Matters** From course notes: > "Key benefits: Prevents user confusion about stalled/failed tool calls, Provides visibility into long-running operations, Real-time feedback during tool execution" Long-running tools **MUST** have progress feedback or users will: - ❌ Think the tool is broken - ❌ Cancel operations prematurely - ❌ Have poor experience with the MCP server --- ## Action Plan ### **Priority 1: Critical Long-Running Tools** 🔴 Fix these 4 tools that MUST have progress/logging: 1. **perform-research-tool.ts** - Add `context?: ToolContext` parameter - Add progress updates for: file search, knowledge graph query, web search - Update wrapper in index.ts to pass context down - **Estimated effort**: 1 hour 2. **llm-cloud-management-tool.ts** - Add `context?: ToolContext` parameter - Add progress for: research phase, LLM generation, cloud execution - Update wrapper in index.ts to accept and pass context - **Estimated effort**: 1 hour 3. **llm-database-management-tool.ts** - Add `context?: ToolContext` parameter - Add progress for: research, query generation, database execution - Update wrapper in index.ts - **Estimated effort**: 1 hour 4. **llm-web-search-tool.ts** - Add `context?: ToolContext` parameter - Add progress for: query optimization, web search, LLM analysis - Update wrapper in index.ts - **Estimated effort**: 1 hour **Total P1 Effort**: 4 hours ### **Priority 2: Fast Tools** 🟢 Optional improvements for fast tools: 5. **conversation-memory-tool.ts** - Add context parameter (for consistency) - Single info message: "Retrieving expanded content..." - **Estimated effort**: 15 minutes 6. **expand-analysis-tool.ts** - Add context parameter (for consistency) - Single info message: "Expanding analysis section..." - **Estimated effort**: 15 minutes **Total P2 Effort**: 30 minutes ### **Priority 3: Documentation & Prevention** 📚 7. **Create Tool Development Guidelines** - Document MCP context parameter requirement - Provide template for new tools - Add to CLAUDE.md - **Estimated effort**: 30 minutes 8. **Add TypeScript Helper Type** ```typescript // types/tool-function.ts export type MCPToolFunction<TArgs, TResult = CallToolResult> = ( args: TArgs, context?: ToolContext // ← Enforces context parameter ) => Promise<TResult>; ``` - **Estimated effort**: 15 minutes **Total P3 Effort**: 45 minutes --- ## Implementation Example ### **Before (Violates MCP Best Practices)** ```typescript // ❌ BAD: No context parameter export async function performResearch(args: { question: string; projectPath?: string; }): Promise<any> { // Silent execution - user has no idea what's happening const files = await searchFiles(args.projectPath); const kgResults = await queryKnowledgeGraph(args.question); const webResults = await webSearch(args.question); return combineResults(files, kgResults, webResults); } ``` ### **After (Follows MCP Best Practices)** ```typescript // ✅ GOOD: Context parameter with progress/logging export async function performResearch( args: { question: string; projectPath?: string; }, context?: ToolContext // ← Add context parameter ): Promise<any> { context?.info('🔍 Starting research: ' + args.question); context?.report_progress(0, 100); context?.info('📁 Searching project files...'); const files = await searchFiles(args.projectPath); context?.report_progress(33, 100); context?.info('📊 Querying knowledge graph...'); const kgResults = await queryKnowledgeGraph(args.question); context?.report_progress(66, 100); context?.info('🌐 Performing web search...'); const webResults = await webSearch(args.question); context?.report_progress(90, 100); context?.info('✅ Research complete!'); context?.report_progress(100, 100); return combineResults(files, kgResults, webResults); } ``` --- ## Testing Checklist After implementing fixes, test each tool: - [ ] **Stdio Transport** (local development) - [ ] Progress updates appear in real-time - [ ] Logging messages display correctly - [ ] No errors with context parameter - [ ] **HTTP Transport** (production simulation) - [ ] Progress works with StreamableHTTP - [ ] Test with `stateless=false` (should work) - [ ] Test with `stateless=true` (progress disabled, should not error) - [ ] Logging messages stream correctly via SSE - [ ] **Context Handling** - [ ] Works with context parameter provided - [ ] Works with context=undefined (graceful degradation) - [ ] No crashes if context is null --- ## Conclusion **Severity**: 🔴 **CRITICAL** for 4 long-running tools **Impact**: Poor user experience, appears broken during execution **Fix Complexity**: **LOW** - Simple parameter addition **Total Effort**: ~5 hours for all fixes + documentation **Recommendation**: **Fix immediately** before deploying to production with HTTP transport. Without progress/logging, long-running operations will create terrible user experience. **Next Steps:** 1. Implement Priority 1 fixes (4 hours) 2. Test with both stdio and HTTP transports 3. Add tool development guidelines to prevent future violations 4. Consider Priority 2 fixes for consistency (30 mins) --- **References:** - MCP Course Notes: "Log and Progress Notifications" - MCP Course Notes: "StreamableHTTP Transport" (stateless limitations) - Existing implementation: `analyzeProjectEcosystem` (index.ts:4033) ← Good example --- ## ✅ IMPLEMENTATION COMPLETE **Completion Date**: 2025-10-07 ### Priority 1 Critical Fixes - ✅ COMPLETED (4/4 tools) All 4 critical long-running tools now have MCP progress/logging support: 1. ✅ **perform-research-tool.ts** - COMPLETED - Added `context?: ToolContext` parameter - Added progress updates at 0%, 25%, 50%, 75%, 100% - Added logging for: research start, file search, knowledge graph query, web search, completion - Updated wrapper in index.ts to pass context (line 6616) - **Files Modified**: - `src/tools/perform-research-tool.ts` (lines 8-9, 58-67, 76-98, 217-218) - `src/index.ts` (lines 6605-6619) 2. ✅ **llm-cloud-management-tool.ts** - COMPLETED - Added `context?: ToolContext` parameter - Added progress updates at 0%, 20%, 50%, 80%, 100% - Added logging for: initialization, research phase, LLM generation, cloud execution, completion - Updated wrapper in index.ts to accept and pass context - **Files Modified**: - `src/tools/llm-cloud-management-tool.ts` (lines 7-9, 57-68, 86-128) - `src/index.ts` (lines 6654-6683) 3. ✅ **llm-database-management-tool.ts** - COMPLETED - Added `context?: ToolContext` parameter - Added progress updates at 0%, 20%, 50%, 80%, 100% - Added logging for: initialization, research phase, query generation, database execution, completion - Updated wrapper in index.ts to accept and pass context - **Files Modified**: - `src/tools/llm-database-management-tool.ts` (lines 7-9, 57-68, 86-129) - `src/index.ts` (lines 6688-6717) 4. ✅ **llm-web-search-tool.ts** - COMPLETED - Added `context?: ToolContext` parameter - Added progress updates at 0%, 25%, 50%, 75%, 100% - Added logging for: initialization, query optimization, web search execution, LLM analysis, completion - Updated wrapper in index.ts to accept and pass context - **Files Modified**: - `src/tools/llm-web-search-tool.ts` (lines 7-9, 55-65, 79-115) - `src/index.ts` (lines 6634-6659) ### Verification ✅ **TypeScript Type Checking**: All changes compile without errors ✅ **MCP Best Practices**: All 4 tools now follow the course notes pattern ✅ **Backwards Compatible**: Context parameter is optional, existing code continues to work ✅ **Progress Feedback**: Users will see real-time updates for long-running operations (30-120s) ### User Experience Impact **Before (without progress/logging)**: ``` User: "Research how authentication works in this project" [60 seconds pass with no feedback - user thinks it's broken] ``` **After (with progress/logging)**: ``` User: "Research how authentication works in this project" 🔍 Starting research: how authentication works in this project [0%] 📁 Searching project files... [25%] 📊 Querying knowledge graph and environment resources... [50%] 🌐 Analyzing results and preparing response... [75%] ✅ Research complete! [100%] [Results displayed] ``` ### Remaining Work (Optional - Priority 2) Two fast tools remain without context (optional improvements for consistency): - 🟢 **conversation-memory-tool.ts** (<1s operations) - Low priority - 🟢 **expand-analysis-tool.ts** (<1s operations) - Low priority These are fast operations where progress feedback is less critical, but could be added for consistency. ### Testing Recommendations Before production deployment, test: 1. ✅ Stdio transport (local development) - Progress updates appear in real-time 2. 🔲 HTTP transport (production simulation) - Progress works with StreamableHTTP 3. 🔲 Context handling - Works with context=undefined (graceful degradation) **Status**: Ready for production deployment with stdio transport. HTTP transport testing recommended before production release.