feat: add MCP server config, session labels, and HTTP tools

[jeremyeder]

Summary

• MCP server configuration: Backend CRUD endpoints and frontend UI for managing MCP servers per workspace, including test connection, import/export (Claude Desktop and OpenCode formats)
• Session labels: Label editor component integrated into the create-session dialog and sessions list for tagging and filtering sessions
• HTTP tools: UI tab and API routes for managing HTTP-based tools per workspace
• Operator: Inject MCP config into session pods when project-level MCP servers are configured

Test plan

• Create/edit/delete MCP server configs from the workspace MCP Servers tab
• Test connection to an MCP server via the test button
• Import/export MCP config in Claude Desktop and OpenCode formats
• Add labels when creating a session, verify they appear in the sessions list
• Add/remove HTTP tools from the workspace HTTP Tools tab
• Verify MCP config is injected into session pods when configured

🤖 Generated with Claude Code

[github-actions]

Claude Code Review

Summary

This PR adds MCP (Model Context Protocol) server configuration, session labels, and HTTP tools management to the Ambient Code Platform. The implementation spans backend API handlers, frontend React components, and operator integration. Overall code quality is good with strong adherence to established patterns, but there are several security and architecture concerns that should be addressed.

Issues by Severity

🚫 Blocker Issues

None identified - No critical blockers that prevent merge.

🔴 Critical Issues

1. [Security] Command Injection Risk in MCP Test Handler (components/backend/handlers/mcp_test_server.go)

   • Location: Lines 166-174, allowedMcpCommands whitelist
   • Issue: While commands are whitelisted, Args and Env fields from user input are passed directly to Pod without validation
   • Risk: Malicious arguments like --exec, -c, or environment variable overrides could execute arbitrary code
   • Example Attack: {"command": "python3", "args": ["-c", "import os; os.system('malicious code')"]}
   • Recommendation:
     • Add strict validation for args (no -c, --eval, --exec patterns)
     • Sanitize environment variables (block PATH, LD_PRELOAD, etc.)
     • Consider using a sidecar container pattern instead of arbitrary command execution
   • Pattern Violation: Security Standards - Input Validation (security-standards.md:95-109)

2. [Security] No Token Validation in Backend Handlers (components/backend/handlers/mcp_config.go)

   • Location: Lines 20-27, 62-69
   • Issue: Uses user-scoped K8s clients (GetK8sClientsForRequest) but doesn't validate token length or check for service account tokens
   • Missing: No check like log.Printf("Processing request with token (len=%d)", len(token)) for audit trail
   • Recommendation: Add token length logging and validate token format before K8s client creation
   • Pattern Violation: Security Standards - Token Handling (security-standards.md:22-48)

3. [Architecture] Frontend API Routes Missing Error Handling (components/frontend/src/lib/api-route-helpers.ts)

   • Location: Lines 18, 35 - Generic console.error without structured logging
   • Issue: Errors logged to console but not captured for monitoring/debugging in production
   • Risk: Silent failures in production, no visibility into proxy errors
   • Recommendation:
     • Use structured error logging (consider Sentry or similar)
     • Return more specific error messages (distinguish network errors, backend errors, auth errors)
     • Add retry logic for transient failures
   • Pattern Violation: Error Handling Patterns (error-handling.md:49-66)

🟡 Major Issues

4. [Code Quality] No Type Safety for MCP Config JSON Parsing (components/backend/handlers/mcp_config.go)

   • Location: Lines 46-55, type assertion without validation
   • Issue: Unmarshals to map[string]interface{} without schema validation
   • Risk: Invalid JSON structure could crash handler or cause runtime panics
   • Recommendation:
     • Define proper Go structs for McpConfig (McpServerConfig, HttpToolConfig)
     • Validate structure before storing in ConfigMap
     • Return 400 Bad Request for invalid schemas
   • Example:

     type McpConfig struct {
         Servers map[string]McpServerConfig `json:"servers"`
     }
     type McpServerConfig struct {
         Command string            `json:"command" binding:"required"`
         Args    []string          `json:"args"`
         Env     map[string]string `json:"env"`
     }

   • Pattern Violation: Backend Development - Type-Safe Unstructured Access (backend-development.md:68-85)

5. [Performance] No Caching for MCP ConfigMap Reads (components/operator/internal/handlers/sessions.go)

   • Location: Lines 1228-1234
   • Issue: Fetches MCP ConfigMap on every session pod creation (could be 100s of sessions)
   • Risk: Unnecessary K8s API load, slower pod startup
   • Recommendation:
     • Cache ConfigMap data in operator memory with watch-based invalidation
     • Only refetch when ConfigMap changes (use informer pattern)
   • Impact: Medium - degrades performance at scale (10+ concurrent sessions)

6. [React Query] Missing Optimistic Updates in Label Editor (components/frontend/src/components/label-editor.tsx)

   • Issue: Label changes aren't persisted to backend, only stored in component state
   • Expected Pattern: Should use useMutation with onMutate for optimistic updates (react-query-usage.md:100-161)
   • Current State: Labels only exist in memory until parent form submission
   • Recommendation:
     • If labels should persist immediately: Add useUpdateSessionLabels mutation
     • If labels are form state: Document this clearly in component comments
   • Clarification Needed: Is label persistence immediate or deferred to session creation?

🔵 Minor Issues

7. [Code Style] Inconsistent Error Messages (components/backend/handlers/mcp_test_server.go)

   • Location: Lines 256, 244 - Mixed error message formats
   • Issue: Some errors return structured JSON, others return plain strings
   • Recommendation: Standardize on gin.H{"error": "message"} for all error responses
   • Pattern: Backend Development - Response Patterns (backend-development.md:51-66)

8. [Frontend] No Loading States in HttpToolsTab (components/frontend/src/components/http-tools-tab.tsx)

   • Location: Line 90 - Button missing disabled={updateMutation.isPending} during operations
   • Issue: User can click "Add Tool" multiple times during save
   • Recommendation: Add loading states to all action buttons
   • Pattern Violation: Frontend Development - All buttons have loading states (frontend-development.md:156)

9. [Documentation] Missing JSDoc for Public Functions (components/frontend/src/lib/api-route-helpers.ts)

   • Issue: createProxyRouteHandlers lacks documentation on return type and error behavior
   • Recommendation: Add JSDoc comments explaining proxy behavior and error handling

10. [Testing] No Unit Tests for MCP Test Script (components/backend/handlers/mcp_test_server.go)

    • Location: Lines 23-130, embedded Python script
    • Risk: Inline script is complex (100+ lines) and untested
    • Recommendation:
      • Extract script to separate file (scripts/mcp_test.py)
      • Add Python unit tests for JSON-RPC handshake logic
      • Add Go integration tests for pod creation and log parsing

Positive Highlights

✅ Excellent User Token Authentication - All backend handlers correctly use GetK8sClientsForRequest (mcp_config.go:22, mcp_test_server.go:177)

✅ SecurityContext Properly Set - MCP test pods have restrictive security settings (mcp_test_server.go:217-223)

✅ Clean React Query Integration - Frontend hooks follow established patterns (use-mcp-config.ts, use-http-tools.ts)

✅ Good Error Handling in Frontend - Skeleton loading states and error alerts implemented (mcp-servers-tab.tsx:177-196)

✅ Operator Integration Well-Designed - ConfigMap mount is optional and non-breaking (sessions.go:1228-1245)

✅ Export/Import Feature - Excellent UX for Claude Desktop and OpenCode format compatibility (mcp-servers-tab.tsx:134-175)

✅ Proper OwnerReferences Pattern - Although not shown in this PR, existing operator code follows best practices

Recommendations

High Priority (Address Before Merge)

1. Fix Command Injection Risk - Add args/env validation in TestMcpServer handler
2. Add Type Validation - Define structs for MCP config instead of map[string]interface{}
3. Improve Error Logging - Add structured logging to frontend proxy helpers

Medium Priority (Address in Follow-up PR)

4. Add Caching - Implement ConfigMap caching in operator for better performance
5. Add Tests - Unit tests for MCP test script and integration tests for backend handlers
6. Document Label Persistence - Clarify whether labels are immediate or deferred

Low Priority (Nice to Have)

7. Standardize Error Messages - Consistent error response format across all handlers
8. Add Loading States - Ensure all buttons have isPending checks
9. Improve JSDoc - Document public API functions

Testing Verification Needed

Before merge, please verify:

• MCP test handler rejects malicious args like ["-c", "malicious code"]
• Invalid JSON in MCP config returns 400 Bad Request (not 500)
• ConfigMap permissions work correctly (user can read/write in their namespace)
• Multiple concurrent MCP tests don't exceed namespace pod limits
• Label editor persists changes correctly (clarify expected behavior)
• Import/Export works for both Claude Desktop and OpenCode formats
• Operator gracefully handles missing MCP ConfigMap (optional mount)

Overall Assessment

Code Quality: 7.5/10

• Strong adherence to established patterns
• Good separation of concerns (API layer, queries, components)
• Missing critical input validation in security-sensitive areas

Security: 6/10

• Critical: Command injection risk in MCP test handler
• Good use of user token authentication
• Proper SecurityContext settings
• Missing input sanitization and validation

Performance: 8/10

• React Query patterns correctly implemented
• Minor optimization opportunity with ConfigMap caching
• No obvious bottlenecks

Recommendation: Approve with changes - Address critical security issues (#1, #2) before merge. Other issues can be addressed in follow-up PRs.

---

Generated by Claude Code Review - Loaded context: CLAUDE.md, backend-development.md, frontend-development.md, security-standards.md, k8s-client-usage.md, error-handling.md, react-query-usage.md

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.