[file-diet] Refactor pkg/cli/mcp_server.go (1372 lines) into focused modules - #17176
Conversation
…lines each Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the 1,372-line pkg/cli/mcp_server.go file into 7 focused modules to improve maintainability and testability. The massive createMCPServer function (886 lines) that registered all 8 MCP tools inline has been split into separate registration functions organized by tool category.
Changes:
- Split monolithic
mcp_server.gointo 7 files organized by responsibility (coordinator, helpers, command setup, HTTP transport, and tool registrations grouped by type) - Introduced
execCmdFunctype for cleaner dependency injection of command execution logic to tool handlers - Added comprehensive unit tests for helper functions (
mcpErrorData,boolPtr,hasWriteAccess,validateWorkflowName) and HTTP utilities (sanitizeForLog,responseWriter,loggingHandler)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/mcp_server.go |
Thin coordinator with execCmdFunc type and createMCPServer function that delegates to registration helpers |
pkg/cli/mcp_server_helpers.go |
Helper functions for error marshaling, repository lookup, permission checking, and workflow validation |
pkg/cli/mcp_server_command.go |
Cobra command setup, server lifecycle management, and mcpLog declaration |
pkg/cli/mcp_server_http.go |
HTTP/SSE transport with request logging and security sanitization |
pkg/cli/mcp_tools_readonly.go |
Registration for read-only tools: status, compile, mcp-inspect |
pkg/cli/mcp_tools_privileged.go |
Registration for privileged tools requiring write+ access: logs, audit |
pkg/cli/mcp_tools_management.go |
Registration for workflow management tools: add, update, fix |
pkg/cli/mcp_server_helpers_test.go |
Unit tests for helper functions |
pkg/cli/mcp_server_http_test.go |
Unit tests for HTTP utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| http.ResponseWriter | ||
| statusCode int | ||
| } | ||
|
|
There was a problem hiding this comment.
The responseWriter struct is missing a WriteHeader method implementation. Without this method, when handlers call WriteHeader(), the statusCode field will never be updated from its default value of http.StatusOK, making the logged status code in loggingHandler always show 200 even for error responses.
Add a WriteHeader method:
func (w *responseWriter) WriteHeader(statusCode int) {
w.statusCode = statusCode
w.ResponseWriter.WriteHeader(statusCode)
}Note: The test TestResponseWriter_EmbeddedResponseWriter on line 70 calls WriteHeader but this test will pass even without the method because it only checks the embedded ResponseWriter's Code field, not the wrapper's statusCode field.
| func (w *responseWriter) WriteHeader(statusCode int) { | |
| w.statusCode = statusCode | |
| w.ResponseWriter.WriteHeader(statusCode) | |
| } |
| rw.WriteHeader(http.StatusCreated) | ||
| if rec.Code != http.StatusCreated { | ||
| t.Errorf("embedded ResponseWriter code = %d, want %d", rec.Code, http.StatusCreated) | ||
| } |
There was a problem hiding this comment.
This test doesn't verify that the responseWriter's statusCode field is updated when WriteHeader is called. It only checks the embedded ResponseWriter (rec.Code), which will be updated even without a WriteHeader method override.
The test should also verify:
if rw.statusCode != http.StatusCreated {
t.Errorf("responseWriter statusCode = %d, want %d", rw.statusCode, http.StatusCreated)
}This would catch the missing WriteHeader method implementation.
| } | |
| } | |
| if rw.statusCode != http.StatusCreated { | |
| t.Errorf("responseWriter statusCode = %d, want %d", rw.statusCode, http.StatusCreated) | |
| } |
pkg/cli/mcp_server.gohad grown to 1,372 lines with a single 886-linecreateMCPServerfunction registering all 8 MCP tools inline — no test file, high cognitive load, hard to extend.Split into 7 focused files (all ≤ 350 lines)
mcp_server.goexecCmdFunctype + thincreateMCPServercoordinatormcp_server_helpers.gomcpErrorData,boolPtr,getRepository,queryActorRole,hasWriteAccess,validateWorkflowName,checkActorPermissionmcp_server_command.gomcpLog,NewMCPServerCommand,checkAndLogGHVersion,runMCPServermcp_server_http.gosanitizeForLog,responseWriter,loggingHandler,runHTTPServermcp_tools_readonly.goregisterStatusTool,registerCompileTool,registerMCPInspectToolmcp_tools_privileged.goregisterLogsTool,registerAuditTool(write+ access checks)mcp_tools_management.goregisterAddTool,registerUpdateTool,registerFixToolKey structural change
createMCPServeris now a thin coordinator that delegates toregisterXToolhelpers, each accepting anexecCmdFunc:New unit tests
mcp_server_helpers_test.go— coversmcpErrorData,boolPtr,hasWriteAccess,validateWorkflowNamemcp_server_http_test.go— coverssanitizeForLog,responseWriter,loggingHandlerNo behavior changes; all existing tests continue to pass.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/graphql/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw GO111MODULE 64/bin/go git remo�� add origin /usr/bin/git -json GO111MODULE 64/bin/go git(http block)https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha jpq_JbW5I GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE ortcfg env g/timeutil/format.go g/timeutil/format_test.go ache/go/1.25.0/x64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE ache/go/1.25.0/x64/pkg/tool/linux_amd64/compile(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v3/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha -json GO111MODULE ache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha vaScript3151156433/001/test-complex-frontmatter-with-tools.md GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha lidations passed" GO111MODULE /opt/hostedtoolcache/go/1.25.0/x-nilfunc GOINSECURE GOMOD GOMODCACHE go env 0652-26638/test-1781840908 GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go l GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha GOMODCACHE resolved$ /usr/bin/git 2Qwr/27yQGRCRYoegit GO111MODULE 64/bin/go git rev-�� --show-toplevel 4333678/b394/imp-tests /usr/bin/git che/go-build/2b/git GOPROXY 64/bin/go git(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v5/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha 1014195/b416/_pkg_.a GO111MODULE 1014195/b416=> GOINSECURE GOMOD GOMODCACHE git rev-�� 1r4Y/WaYG5ENJPsn0nfHI1r4Y GOPROXY /usr/bin/git GOSUMDB GOWORK 64/bin/go git(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha .github/workflows/test.md node /usr/bin/git prettier --check 64/bin/go git rev-�� --git-dir /opt/hostedtoolcache/go/1.25.0/xGO111MODULE /usr/bin/git(http block)https://api.github.com/repos/actions/github-script/git/ref/tags/v8/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha GOSUMDB GOWORK 64/bin/go GOINSECURE GOMOD GOMODCACHE go ache�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha g/parser/import_GOINSECURE g/parser/import_GOMOD 64/bin/go GOINSECURE GOMOD GOMODCACHE ache/go/1.25.0/xGO111MODULE env 4333678/b399/_pkGOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD erignore go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/actions/setup-go/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha brave.md GO111MODULE /opt/hostedtoolcache/go/1.25.0/x/tmp/gh-aw-git-clone-864601210 GOINSECURE GOMOD GOMODCACHE go env 0652-26638/test-1781840908 GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/actions/setup-node/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha bot-detection.md GO111MODULE /opt/hostedtoolcache/go/1.25.0/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 0/x64/bin/node GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts/usr/bin/gh gh run download 1 --dir test-logs/run-1 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts/usr/bin/gh gh run download 12345 --dir test-logs/run-12345 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts/usr/bin/gh gh run download 12346 --dir test-logs/run-12346 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts/usr/bin/gh gh run download 2 --dir test-logs/run-2 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go estl�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts/usr/bin/gh gh run download 3 --dir test-logs/run-3 GO111MODULE x_amd64/compile GOINSECURE GOMOD GOMODCACHE x_amd64/compile env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts/usr/bin/gh gh run download 4 --dir test-logs/run-4 GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts/usr/bin/gh gh run download 5 --dir test-logs/run-5 GO111MODULE x_amd64/link GOINSECURE GOMOD GOMODCACHE x_amd64/link env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE cx/AERBTSiwFBTdGDiIa9qx/H4W_iTHyremote.origin.url(http block)https://api.github.com/repos/github/gh-aw/actions/workflows/usr/bin/gh gh workflow list --json name,state,path GOSUMDB GOWORK 64/bin/go GOINSECURE GOMOD ode-gyp-bin/sh go m/_n�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100 GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE sh(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6 GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 2230823790/.github/workflows GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999/usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq .object.sha ty-test.md GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 3366899114/.github/workflows GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/nonexistent/repo/actions/runs/12345/usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/owner/repo/actions/workflows/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo 64/bin/go GOINSECURE GOMOD GOMODCACHE go m/_n�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo 64/bin/go GOINSECURE GOMOD GOMODCACHE go env GOPATH); \ if coGOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/owner/repo/contents/file.md/tmp/go-build4201014195/b380/cli.test /tmp/go-build4201014195/b380/cli.test -test.testlogfile=/tmp/go-build4201014195/b380/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true GOINSECURE GOMOD GOMODCACHE go k/gh�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD erignore go(http block)https://api.github.com/repos/test-owner/test-repo/actions/secrets/usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name GOSUMDB GOWORK 64/bin/go GOINSECURE GOMOD GOMODCACHE go m/_n�� -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>[file-diet] Refactor
pkg/cli/mcp_server.go(1372 lines) into focused modules</issue_title><issue_description>### Overview
The file
pkg/cli/mcp_server.gohas grown to 1,372 lines — nearly double the healthy threshold of 800 lines — with no corresponding test file. The entire file lives inside a singlecreateMCPServerfunction that registers all 8 MCP tool handlers inline, making it hard to navigate, test, or extend without touching unrelated logic.Current State
pkg/cli/mcp_server.gomcp_server_test.godoes not exist)createMCPServer) owns all tool registrationsFull File Analysis
Functional Breakdown
mcpErrorData,boolPtr,getRepository,queryActorRole,hasWriteAccess,validateWorkflowNameNewMCPServerCommand,checkAndLogGHVersion,runMCPServer,checkActorPermissioncreateMCPServer(885 lines)status,compile,logs,audit,mcp-inspect,add,update,fixsanitizeForLog,responseWriter,loggingHandler,runHTTPServerComplexity Hotspots
createMCPServer(lines 406–1291) — single function containing 8 distinct tool handler closures, 8 privatetype xArgs structdeclarations, schema generation calls, and duplicated arg-building + error-handling patternsselect { case <-ctx.Done(): ... }), repeatedexec.ExitErrorstderr extraction, and areturn &mcp.CallToolResult{...}response constructionRefactoring Strategy
Proposed File Splits
mcp_server_helpers.go(~175 lines)mcpErrorData,boolPtr,getRepository,queryActorRole,hasWriteAccess,validateWorkflowNamemcp_server_command.go(~230 lines)NewMCPServerCommand,checkAndLogGHVersion,runMCPServer,checkActorPermissionmcpLogvar heremcp_server_tools.go(~300 lines)createMCPServer(coordinator only — delegates toregisterXToolhelpers), tool arg typesmcp_tools_readonly.go(~280 lines)registerStatusTool,registerCompileTool,registerMCPInspectToolmcp_tools_privileged.go(~280 lines)registerLogsTool,registerAuditToolmcp_tools_management.go(~250 lines)registerAddTool,registerUpdateTool,registerFixToolmcp_server_http.go(~80 lines)sanitizeForLog,responseWriter,loggingHandler,runHTTPServerShared Utilities
Extract the duplicated cancellation pattern and exec error helper into shared private helpers in
mcp_server_helpers.go:Interface Abstractions
The
execCmdclosure passed intocreateMCPServeris good — keep it as-is so tool files can accept it as afunc(ctx, ...string) *exec.Cmdparameter without referencing the outer closure.Test Coverage Plan
Add
mcp_server_test.gowith at minimum:mcp_server_helpers_test.goTestMcpErrorData— nil input, valid input, marshal errorTestHasWriteAccess— all permission levelsTestValidateWorkflowName— valid names, injection attempts, empty stringTestSanitizeForLog— newlines, carriage returns, clean inputmcp_server_command_test.goTestCheckActorPermission— with/without validateActor, missing actormcp_tools_readonly_test.goTestStatusTool— mockGetWorkflowStatuses, verify JSON o...pkg/cli/mcp_server.go(1372 lines) into focused modules #17163💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.