-
Notifications
You must be signed in to change notification settings - Fork 238
Description
Overview
The file pkg/cli/mcp_server.go has grown to 1088 lines, making it difficult to maintain and test. This task involves refactoring it into smaller, focused files with improved test coverage and clearer separation of concerns.
Current State
- File:
pkg/cli/mcp_server.go - Size: 1088 lines
- Test Coverage: 3147 test lines across 9 test files (~2.9:1 test-to-source ratio - excellent coverage)
- Complexity: High - Single monolithic file handles 8 MCP tools plus server infrastructure
Full File Analysis
Detailed Breakdown
Current Structure:
- Functions: 9 total (3 exported, 6 internal)
NewMCPServerCommand()- Cobra command setuprunMCPServer()- Main server orchestrationcreateMCPServer()- Server creation and tool registration (~850 lines, 78% of file)checkAndLogGHVersion()- Version validationmcpErrorData()- Error marshaling helpersanitizeForLog()- Log injection preventionloggingHandler()- HTTP middlewarerunHTTPServer()- HTTP server setup
MCP Tool Handlers (8 total, embedded in createMCPServer):
- status (~70 lines) - Workflow status listing
- compile (~120 lines) - Workflow compilation with validation
- logs (~210 lines) - Log download and analysis
- audit (~105 lines) - Workflow run investigation
- mcp-inspect (~85 lines) - MCP server inspection
- add (~60 lines) - Workflow addition from remote repos
- update (~70 lines) - Workflow updates from source
- fix (~70 lines) - Automatic codemod application
Infrastructure Components:
- Tool argument structs (8 types, ~100 lines)
- Schema generation and defaults (~50 lines)
- Command execution helpers (~20 lines)
- HTTP server setup (~70 lines)
- Error handling and logging (~40 lines)
Complexity Hotspots:
- Lines 157-1006:
createMCPServer()function (850 lines) - Primary refactoring target - Lines 422-608:
logstool handler (186 lines) - Complex timeout and guardrail logic - Lines 258-420:
compiletool handler (162 lines) - Multiple validation options - Duplicate error handling patterns across all 8 tool handlers
- Schema generation repeated for each tool with similar patterns
Areas with High Coupling:
- All tool handlers share similar structure but are inline in
createMCPServer - Error handling duplicated across handlers (8x
ctx.Done()checks, 8x error marshaling) - Command execution logic repeated with slight variations
- Test files organized by tool but source code is monolithic
Refactoring Strategy
Proposed File Splits
Based on functional boundaries and the repository's preference for many small files:
-
mcp_server_command.go(~150 lines)- Functions:
NewMCPServerCommand(),runMCPServer(),checkAndLogGHVersion() - Responsibility: Command setup, server lifecycle, validation
- Rationale: Entry point and orchestration logic
- Functions:
-
mcp_server_factory.go(~100 lines)- Functions:
createMCPServer(),execCmd()helper - Responsibility: Server creation, configuration, tool registration
- Rationale: Server factory pattern, delegates to tool modules
- Functions:
-
mcp_server_tools_status.go(~90 lines)- Functions:
addStatusTool(server, execCmd) - Types:
statusArgs - Responsibility: Status tool implementation
- Rationale: Self-contained tool module
- Functions:
-
mcp_server_tools_compile.go(~180 lines)- Functions:
addCompileTool(server, execCmd) - Types:
compileArgs - Responsibility: Compile tool implementation
- Rationale: Largest tool, deserves dedicated file
- Functions:
-
mcp_server_tools_logs.go(~230 lines)- Functions:
addLogsTool(server, execCmd),checkLogsOutputSize() - Types:
logsArgs - Responsibility: Logs tool with guardrail logic
- Rationale: Second-largest tool with complex logic
- Functions:
-
mcp_server_tools_audit.go(~110 lines)- Functions:
addAuditTool(server, execCmd) - Types:
auditArgs - Responsibility: Audit tool implementation
- Rationale: Self-contained investigation logic
- Functions:
-
mcp_server_tools_inspect.go(~100 lines)- Functions:
addMCPInspectTool(server, execCmd) - Types:
mcpInspectArgs - Responsibility: MCP inspect tool implementation
- Rationale: Server introspection logic
- Functions:
-
mcp_server_tools_management.go(~200 lines)- Functions:
addAddTool(),addUpdateTool(),addFixTool() - Types:
addArgs,updateArgs,fixArgs - Responsibility: Workflow management tools (add/update/fix)
- Rationale: Related workflow lifecycle operations
- Functions:
-
mcp_server_http.go(~90 lines)- Functions:
runHTTPServer(),loggingHandler(),sanitizeForLog() - Types:
responseWriter - Responsibility: HTTP/SSE transport implementation
- Rationale: HTTP-specific infrastructure
- Functions:
-
mcp_server_helpers.go(~50 lines)- Functions:
mcpErrorData(), common error handling utilities - Responsibility: Shared helper functions
- Rationale: Utilities used across multiple tool modules
- Functions:
Shared Utilities
Extract common functionality into:
mcp_server_helpers.go: Error marshaling, context cancellation checks, jq filter applicationmcp_server_schema.go: Schema generation and default value helpers (if needed separately)
Interface Abstractions
Consider introducing interfaces to reduce coupling:
// ToolRegistrar allows registering MCP tools with common infrastructure
type ToolRegistrar interface {
Register(server *mcp.Server)
}
// CommandExecutor abstracts command execution for testing
type CommandExecutor interface {
Exec(ctx context.Context, args ...string) *exec.Cmd
}Test Coverage Plan
Existing test files (maintain and extend):
mcp_server_test.go- General server testsmcp_server_tools_test.go- Tool behavior testsmcp_server_operations_test.go- Operation testsmcp_server_defaults_test.go- Schema default testsmcp_server_add_test.go- Add tool testsmcp_server_compile_test.go- Compile tool testsmcp_server_inspect_test.go- Inspect tool testsmcp_server_error_codes_test.go- Error handling testsmcp_server_fix_test.go- Fix tool testsmcp_server_json_integration_test.go- JSON integration tests
New test files to add:
-
mcp_server_command_test.go- Test command creation and flag parsing
- Test server startup with different transports (stdio/HTTP)
- Test gh CLI version checking
- Target coverage: >80%
-
mcp_server_factory_test.go- Test server creation with different cmdPath configurations
- Test tool registration
- Test execCmd helper with custom paths
- Target coverage: >80%
-
mcp_server_tools_status_test.go(merge from existing)- Test status tool argument parsing
- Test pattern filtering
- Test jq filter application
- Target coverage: >80%
-
mcp_server_tools_logs_test.go(merge from existing)- Test logs guardrail logic
- Test timeout handling
- Test continuation response
- Target coverage: >80%
-
mcp_server_http_test.go- Test HTTP server setup
- Test logging middleware
- Test log injection prevention
- Target coverage: >80%
-
mcp_server_helpers_test.go- Test error data marshaling
- Test context cancellation handling
- Target coverage: >80%
Test organization strategy:
- Keep existing tests organized by tool (status, compile, logs, audit, inspect, add, fix)
- Add new tests for extracted infrastructure (command, factory, HTTP, helpers)
- Maintain current high test coverage ratio (>2.5:1 test-to-source)
- Use table-driven tests for tool handlers with common patterns
Implementation Guidelines
- Preserve Behavior: Ensure all existing MCP tools work identically
- Maintain Exports: Keep
NewMCPServerCommand()as the only exported function (current API) - Add Tests First: Ensure existing tests pass, then add tests for new boundaries
- Incremental Changes: Split one tool module at a time, starting with simplest (status, add, update, fix)
- Run Tests Frequently: Verify
make test-unitpasses after each split - Update Imports: Ensure all import paths remain in
pkg/cli - Document Changes: Add package comments explaining module organization
Acceptance Criteria
- Original file is split into 10 focused files (~100-230 lines each)
- Each new file is under 300 lines (target: 50-230 lines)
- All tests pass (
make test-unit) - Test coverage remains ≥80% for all tool modules
- No breaking changes to public API (
NewMCPServerCommandunchanged) - Code passes linting (
make lint) - Build succeeds (
make build) - All 8 MCP tools function identically (status, compile, logs, audit, mcp-inspect, add, update, fix)
- HTTP and stdio transports both work correctly
- Error handling preserves context cancellation checks
- Debug logging (
DEBUG=mcp:*) works as before
Additional Context
Repository Guidelines: Follow patterns in .github/agents/developer.instructions.agent.md
Code Organization Philosophy (from developer instructions):
"Prefer many smaller files grouped by functionality. Add new files for new features rather than extending existing ones."
Existing Tool File Patterns: See pkg/cli/logs_*.go for inspiration - the logs command is split into:
logs_command.go- Command setuplogs_orchestrator.go- Orchestrationlogs_download.go- Download logiclogs_display.go- Display logiclogs_parsing_*.go- Various parsing moduleslogs_report.go- Report generation
Testing Patterns: Match existing patterns in pkg/cli/mcp_server_*_test.go
- Use
require.*for critical setup - Use
assert.*for test validations - Table-driven tests with descriptive names
- Helpful assertion messages
Logger Usage: Maintain mcpLog = logger.New("mcp:server") for all modules
Console Formatting: Continue using console.Format*Message() for user-facing output
Priority: Medium
Effort: Large (~8-12 hours) - Complex refactoring with 8 tool modules and extensive test coverage
Expected Impact: Significantly improved maintainability, easier testing, reduced file complexity, clearer module boundaries
AI generated by Daily File Diet
- expires on Feb 12, 2026, 1:38 PM UTC