1 Commits

Author	SHA1	Message	Date
shankar0123	fbe053aa0c	refactor(mcp): split tools.go by tool domain — Option B sibling-files (Phase 9, 10 of N) ... Phase 9 ARCH-M2 closure Sprint 10. Splits internal/mcp/tools.go (was 1867 LOC, the second-largest backend hotspot after the service/acme.go cuts in Sprints 9 + 9b) via the Option B sibling- file pattern — new files stay in `package mcp` so every external caller of `mcp.RegisterTools(...)` resolves the same way. Pure mechanical relocation; no signature, no behavior, no import-graph change. Why this is naturally suited to Option B ======================================== The mcp package already follows the sibling-file convention: tools_audit_fix.go (registerAuditFixTools), tools_auth.go (registerAuthTools), tools_auth_bundle2.go (registerAuthBundle2Tools), and tools_est.go (registerESTTools) each carry a single register-function each, all in the same `mcp` package. Sprint 10 extends that pattern to the 22 register-functions still inside tools.go. The structure of tools.go is unusually clean for a refactor: every domain has its own `// ── DomainName ──` banner above its register-function, and every register-function ends with a `}` + blank line before the next domain's banner. The RegisterTools dispatcher stayed in tools.go and still invokes each registerXxxTools(...) in the same order — calls cross a file boundary but stay in `package mcp`, so same-package resolution makes them zero-cost. What moved ========== New `internal/mcp/tools_certificates.go` (404 LOC) — certificate- lifecycle domain: - registerCertificateTools (cert CRUD + revocation) - registerCRLOCSPTools - registerRenewalPolicyTools (Phase C P1-1..P1-5) - registerVerificationTools (Phase G P1-32/P1-34/P1-35) New `internal/mcp/tools_agents.go` (266 LOC) — agent-management domain: - registerAgentTools (per-agent CRUD + lifecycle) - registerAgentGroupTools New `internal/mcp/tools_resources.go` (565 LOC) — resource- management / configuration surface: - registerIssuerTools, registerTargetTools - registerPolicyTools, registerProfileTools - registerTeamTools, registerOwnerTools - registerNotificationTools - registerIntermediateCATools (Phase F P1-6..P1-9) New `internal/mcp/tools_jobs.go` (170 LOC) — workflow domain: - registerJobTools - registerApprovalTools + approvalDecisionPayload struct (Phase A P1-28..P1-31) New `internal/mcp/tools_discovery.go` (169 LOC) — discovery domain: - registerNetworkScanTools (Phase D P1-14..P1-19) - registerDiscoveryReadTools (Phase E P1-10..P1-13) New `internal/mcp/tools_admin.go` (369 LOC) — observability / admin domain: - registerAuditTools, registerStatsTools, registerDigestTools, registerMetricsTools, registerHealthTools - registerHealthCheckTools (Phase B P1-20..P1-27) What stays in tools.go (109 LOC, down from 1867) ================================================ - The RegisterTools dispatcher (still owns the canonical registration order; calls cross-file but stay in-package). - The three Bundle-3 wrappers + helper that every register function consumes: textResult (the json.RawMessage success-path fence), errorResult (the failure-path fence), paginationQuery (the URL helper). The unused `context` import is dropped from tools.go as a clean side effect — none of the four surviving functions take a context.Context. Per-import audit on every new file: - tools_certificates.go: context, fmt, gomcp - tools_agents.go: context, fmt, net/url, gomcp - tools_resources.go: context, gomcp - tools_jobs.go: context, gomcp - tools_discovery.go: context, gomcp - tools_admin.go: context, net/url, strconv, gomcp None of the moved code touched encoding/json directly — that import stays inside tools.go for textResult's json.RawMessage param. Bundle-3 fence guardrail update =============================== The existing TestFenceGuardrail_NoBareCallToolResult guardrail in fence_guardrail_test.go fails any file that constructs gomcp.CallToolResult{...} literals outside the tools.go allowlist. registerCRLOCSPTools — which moved to tools_certificates.go — has two pre-existing literal CallToolResult constructions: each returns a server-built status string of the form "DER CRL retrieved (%d bytes, content-type: %s)" or "OCSP response retrieved (...)". The byte count is `len(raw)` (server-controlled) and the content-type comes from the HTTP header on the upstream PKI endpoint (server-controlled in self-hosted deployments). Both predate Bundle-3 fencing. Two options to keep CI green: (a) Route through textResult — but that changes behavior (adds the UNTRUSTED MCP_RESPONSE fence around the response), which breaks the "mechanical relocation, no behavior change" rule Sprint 10 commits to. (b) Add tools_certificates.go to the allowlist with a comment explaining the carve-out is pre-existing and Sprint 10 preserves byte-exact behavior. This commit takes option (b). The allowlist comment in fence_guardrail_test.go documents the carve-out, points at the specific tools (CRL + OCSP binary-pass-through with server-built status descriptions), and flags tightening these two sites through textResult as a follow-up concern (open question: does the format break MCP consumers that parse the description text). Net effect ========== tools.go: 1867 → 109 LOC (-1758 = -94.2%). Six new sibling files at 1943 LOC total (109 LOC of header + Phase 9 doc-comment overhead per file = ~185 LOC of added documentation; the rest is moved code). The biggest pre-Sprint-10 hotspot in the mcp package is now smaller than tools_test.go (435 LOC). Cumulative Phase 9 progress =========================== config.go 3403 → 1342 (-60.6%, Sprints 1-7) cmd/server/main.go 2966 → 2260 (-23.8%, Sprints 8 + 8b) service/acme.go 1965 → 1162 (-40.9%, Sprints 9 + 9b) mcp/tools.go 1867 → 109 (-94.2%, Sprint 10) TOTAL across 4 files: 10,201 → 4,873 LOC = -5,328 (-52.2%) Behavior preservation contract ============================== 1. gofmt -l clean across all 8 affected files. 2. go vet ./internal/mcp/... — no findings. 3. staticcheck ./internal/mcp/... ./cmd/mcp-server/... — no findings. 4. go test -short -count=1 ./internal/mcp/... — green (includes the TestFenceGuardrail_NoBareCallToolResult guardrail post-allowlist- update, the tools_per_tool_test.go suite that exercises every moved register function, and the injection_regression_test.go suite that pins Bundle-3 fencing behavior on the wrapper layer). 5. Broader-importer build green: go build ./... . 6. Broader-importer tests green: go test -short ./cmd/mcp-server/... ./internal/api/handler/... ./cmd/server/... . Same-package resolution means the RegisterTools dispatcher's 13-line call list in tools.go reaches each registerXxxTools across six new sibling files via compile-time-resolved package-level names; the public mcp.RegisterTools entry point + its (s, client) signature is unchanged. What remains for Phase 9 ======================== Two sibling-file splits queued: - Sprint 11: internal/api/handler/auth_session_oidc.go (1577 LOC) split per handler verb (login / callback / refresh / logout / backchannel). - Sprint 12: cmd/agent/main.go (1489 LOC) mirroring the cmd/server pattern from Sprints 8 + 8b. Refs: ARCH-M2 (god-files), Phase 9 audit. Sprint 10 closes the MCP hotspot from the audit's top-6 list.	2026-05-14 10:15:21 +00:00