php-mcp/changelog/2026/jan/code-review.md

Mcp Module Review

Updated: 2026-01-21 - Security improvements: DataRedactor service for sensitive data redaction in tool/API logs, UUID v4 for session IDs (~122 bits entropy). Previous: rate limiting, cache TTL config, input validation, database indexes, cleanup command, server consolidation, circuit breaker, tool call migrations

Overview

The MCP (Model Context Protocol) module provides infrastructure for AI agent integration with the Host Hub platform. It offers:

1. STDIO MCP Server - Single command-based server (mcp:agent-server) for Claude/AI integration via JSON-RPC using registry pattern with extracted tools
2. HTTP API - API key authenticated access to MCP tools
3. Agent Tooling - Comprehensive tool suite for plan/phase/task management, session handling, and multi-agent handoff
4. Monitoring - Metrics collection, Prometheus export, health checks, and alerting
5. Admin UI - Livewire components for API key management, playground, and request logging

The module bridges Laravel's MCP package with a custom agent orchestration system (Agentic module) and provides extensive analytics/monitoring capabilities.

Production Readiness Score: 94/100 (was 92/100 - Security hardening 2026-01-21)

Breakdown:

• Code Quality: 88/100 (clean patterns, good separation, single server)
• Test Coverage: 65/100 (core services covered, tools undertested)
• Security: 95/100 (solid auth, SQL injection fixed, rate limiting, data redaction, UUID session IDs)
• Documentation: 60/100 (good inline docs, missing user docs)
• Error Handling: 90/100 (workspace fallback fixed, input validation added, circuit breaker for fault tolerance)
• Production Hardening: 92/100 (rate limits, indexes, cleanup jobs, configurable TTLs, circuit breaker, all migrations present, PII redaction)

Critical Issues (Must Fix)

• Missing migration for mcp_tool_calls and mcp_tool_call_stats tables - FIXED 2026-01-21: Migration 2026_01_21_000002_create_mcp_tool_call_tables.php now exists with both tables, proper indexes, and foreign keys.

• QueryDatabase SQL injection risk - FIXED: Added comprehensive validation including semicolon check (prevents stacked queries), table allowlist (19 safe tables), 14 forbidden keywords (INSERT, UPDATE, DELETE, DROP, etc.), and UNION attack prevention.

• Server implementations consolidated - FIXED: The monolithic V1 server (~2000 lines) has been removed and replaced by the clean V2 implementation using the registry pattern with extracted tools. Content generation tools were V1-only features not yet migrated to the tool pattern - out of scope for this consolidation.

• Hardcoded workspace_id fallback - FIXED: SessionStart.php now returns error if workspace_id cannot be determined instead of falling back to ID 1. Validates workspace_id is required from context or plan.

Recommended Improvements

• Add rate limiting to STDIO servers - VERIFIED: ToolRateLimiter service at Mcp/Services/ToolRateLimiter.php provides cache-based rate limiting with configurable per-tool limits. McpAgentServerCommand uses $rateLimiter->check() and $rateLimiter->hit() before tool execution (lines 216-228).

• Implement scheduled cleanup for tool call logs - VERIFIED: CleanupToolCallLogsCommand at Mcp/Console/Commands/CleanupToolCallLogsCommand.php prunes mcp_tool_calls, mcp_api_requests, and mcp_tool_call_stats with configurable retention (default 90 days for logs, 365 days for stats). Supports --dry-run and chunk-based deletion.

• Add input validation to agent tools - VERIFIED: AgentTool base class at Mcp/Tools/Agent/AgentTool.php provides comprehensive validation helpers: requireString(), requireInt(), requireArray(), requireEnum(), optionalString(), optionalInt(), optionalEnum() with min/max bounds, length limits, and enum validation. Plan tools like PlanCreate use these (e.g., $this->requireString($args, 'title', 255)).

• Consolidate server implementations - VERIFIED: Only McpAgentServerCommand.php exists (no V2 file). Uses AgentToolRegistry pattern with 22 extracted tool classes. Content generation tools out of scope.

• Add circuit breaker for external dependencies - IMPLEMENTED 2026-01-21: CircuitBreaker service at Mcp/Services/CircuitBreaker.php provides fault tolerance with configurable thresholds, reset timeouts, and half-open state recovery. AgentTool base class exposes withCircuitBreaker() helper for wrapping external calls. Tools like PlanGet and SessionStart now use circuit breaker pattern with graceful fallbacks. Config in mcp.circuit_breaker.

• Add missing database indexes - VERIFIED: Migration 2026_01_21_000003_add_indexes_to_agent_sessions.php adds indexes on agent_sessions table:

  • last_active_at (stale session cleanup)
  • agent_type (filtering)
  • (workspace_id, last_active_at) (active session queries)
  • (status, last_active_at) (cleanup queries)

• Configuration extraction - PARTIALLY COMPLETED: Created config/mcp.php with:

  • Rate limit settings (moved from hardcoded values in McpApiKeyAuth)
  • Cache TTL settings (moved from hardcoded 86400 in AgentSessionService)
  • Protocol version setting
  • Monitoring thresholds now configurable
  • Note: Some values still hardcoded in V1 server pending deprecation

• Fix DEFAULT_TTL bug - VERIFIED: AgentSessionService at lines 28-31 uses getCacheTtl() method that reads from config('mcp.session.cache_ttl', 86400) instead of hardcoded constant. Used in setState() and cacheActiveSession() methods.

• Add OpenTelemetry/distributed tracing - The monitoring service exports to Prometheus but lacks trace context propagation for debugging complex multi-agent flows.

Missing Features (Future)

• WebSocket support for real-time updates - Currently all communication is request/response. Real-time session status would improve agent UX.

• Tool execution audit log - While McpToolCall logs calls, there's no immutable audit trail for compliance. Consider event sourcing for sensitive tools.

• Multi-server federation - The HostHub and Marketing servers are registered but there's no mechanism to route requests between them or aggregate tool listings.

• Session recovery/replay - No mechanism to replay a session from its work log. Would be valuable for debugging and resuming failed sessions.

• Tool versioning - No version tracking on tools. Breaking changes to tool schemas would affect running agents with no fallback.

• Usage quotas per workspace - API keys have rate limiting but no workspace-level quotas for tool calls or token consumption.

• Tool dependency graph - Some tools depend on others (e.g., task_update requires a valid plan). No validation that prerequisites are met.

Test Coverage Assessment

Current Coverage:

Area	Coverage	Notes
McpApiKeyAuth middleware	Good	Comprehensive tests for auth, rate limiting, server scopes
AgentToolRegistry	Good	Registration, permissions, execution tested
AgentSessionService	Good	Session lifecycle, handoff, statistics covered
McpMonitoringService	Good	Health status, Prometheus export, anomaly detection
Core MCP Tools	Partial	ListSites, GetStats, QueryDatabase tested
Agent Tools	Missing	22 extracted tools have no direct tests
Server Command	Partial	Core server loop tested via integration, tool execution via registry tests
Admin Livewire Components	Missing	ApiKeyManager, Playground, RequestLog untested
Resources/Prompts	Missing	No tests for AppConfig, DatabaseSchema, ContentResource, or prompts

Test File Inventory:

• Tests/Feature/McpApiKeyAuthTest.php - 370 lines, thorough
• Tests/Feature/AgentToolRegistryTest.php - 196 lines, good
• Tests/Feature/AgentSessionServiceTest.php - 203 lines, good
• Tests/Feature/McpToolsTest.php - 289 lines, partial
• Tests/Feature/McpMonitoringServiceTest.php - 171 lines, good
• Tests/UseCase/ApiKeyManagerBasic.php - Unclear purpose, possibly incomplete

Estimated Coverage: ~40% of module code is tested.

Security Concerns

1. SQL Injection via QueryDatabase (Critical)

   • Regex check insufficient
   • Allows UNION attacks, stacked queries
   • Recommendation: Use read-only database user + parameterised views or whitelist specific queries

2. API Key exposure in error responses (Low)

   • The toCurl() method on McpApiRequest uses placeholder but could log actual keys if misused

3. Session ID predictability (Low) - FIXED 2026-01-21

   • Session IDs now use UUID v4 (~122 bits entropy) via Uuid::uuid4() in AgentSession::start()
   • Format: sess_ prefix + UUID v4 (e.g., sess_550e8400-e29b-41d4-a716-446655440000)

4. Missing CSRF protection on Livewire components (Medium)

   • Admin components could be vulnerable if session hijacked
   • Verify Livewire's built-in CSRF is enabled

5. No tool call signing (Low)

   • Tool calls aren't cryptographically signed
   • Replay attacks possible within rate limit window

6. Sensitive data in tool call logs (Medium) - FIXED 2026-01-21

   • New DataRedactor service at Mcp/Services/DataRedactor.php provides automatic redaction
   • McpToolCall::log() now redacts input_params and summarizes result_summary
   • McpApiRequest::log() now redacts request_body and summarizes response_body
   • Redacts: passwords, tokens, API keys, credit cards, NI numbers, JWTs, PII fields
   • Partially redacts PII like emails (shows joh***@e***om)

Notes

Architecture Observations

The module now uses a clean registry-based architecture:

1. Single server command (mcp:agent-server) using AgentToolRegistry
2. Tools are extracted classes implementing AgentToolInterface
3. Content generation tools were removed during consolidation (out of scope, can be added as extracted tools later)

The dependency on the Agentic module (Mod\Agentic\Models\*) is extensive. Consider:

• Moving shared models to a Core package
• Or defining interfaces in Mcp and having Agentic implement them

Positive Patterns

• Clean use of PHP 8 features (enums, named arguments, union types)
• Consistent error response structure across tools
• Good use of Eloquent scopes for query building
• Prometheus metrics export is well-implemented
• Rate limiting with automatic clear on success
• Circuit breaker pattern for external module fault tolerance
• DataRedactor service for automatic PII/credential sanitisation in logs
• UUID v4 session IDs providing cryptographic-strength unpredictability

Technical Debt

1. onMcpTools event handler in Boot.php is empty (commented as "once extracted")
2. The UseCase/ApiKeyManagerBasic.php test file appears incomplete
3. Content generation tools (content_status, content_brief_*, content_generate, etc.) were removed during consolidation - add as extracted tools if needed

Dependencies

External module dependencies:

• Mod\Agentic - AgentPlan, AgentPhase, AgentSession, AgentWorkspaceState, PlanTemplateService
• Mod\Api - ApiKey, ApiKeyFactory
• Mod\Tenant - Workspace, User, EntitlementService
• Mod\Content - ContentBrief, AIUsage, GenerateContentJob, AIGatewayService
• Mod\Web - BioLink tools (BioResource, various Mcp\Tools)
• Mod\Trust - TrustHost tools
• Laravel\Mcp - Server, Tool, Request, Response
• Website\Mcp - Dashboard, McpRegistryController

This deep coupling may cause issues during module extraction or independent deployment.