[Defilan]

What

Phase 1 of the Foreman MCP (Model Context Protocol) client: coder/reviewer
agents can now call read-only MCP tools (e.g. context7 docs lookup) at run
time. MCP tools appear in the agent's tool loop as ordinary tools named
mcp/<server>/<tool>, flowing through the same dispatch loop and transcript as
native tools. They are gated by their own per-server allowedTools list rather
than the Agent.spec.tools whitelist (which enumerates static native tools and
cannot name dynamically-discovered MCP tools).

New CRD surface (all optional, back-compatible):

kind: Agent
spec:
  mcp:
    enabled: true                 # per-agent opt-in; default off
    callTimeout: 30s
    maxResultBytes: 32768
    servers:
      - name: context7
        transport: http            # Phase 1: HTTP/streamable only
        url: https://mcp.context7.com/mcp
        headers:
          - name: Authorization
            valueFrom: { name: mcp-context7, key: api-key }
        allowedTools: ["*"]

Plus Workload.spec.mcpEnabled / AgenticTask.spec.mcpEnabled (a benchmark
opt-out: run the same coder with MCP off for an A/B control).

Why

Foreman agents hallucinate facts that live outside the model (metric names, API
groups, syntax): #409 invented a vLLM metric, #850 wrote the wrong API group,
#809 fabricated groundable behavior. An in-agent MCP client lets them consult
authoritative sources during a run and stop guessing.

Part of #1013 (this is Phase 1 of that epic; Brave search and native stdio
transport follow in later phases, so the epic stays open).

How

• New package pkg/foreman/agent/mcp: a client wrapper over the official
  github.com/modelcontextprotocol/go-sdk (v1.6.1, HTTP/streamable transport) —
  the SDK is imported by exactly one file; a tool adapter turning discovered MCP
  tools into tools.Tools; a Connect entry point (dial → discover → filter by
  allowedTools → adapt); and a Register/BuildServers wiring helper.
• Import-cycle-driven placement: mcp imports agent (for
  agent.ToolResult), so agent cannot import mcp. Registration therefore
  lives in the wiring layer cmd/foreman-agent/main.go (makeRegistryFactory),
  and only a workloadMCPEnabled bool + ctx are threaded through the
  RegistryFactory seam. The registry is assembled by filtering the native
  tools against Agent.spec.tools first, then adding the MCP tools onto the
  filtered set — so MCP tools bypass the native whitelist (they are gated by
  their own per-server allowedTools) and reach the model even when an agent's
  spec.tools lists only native tools.
• Best-effort by construction: an unreachable server, a failed secret
  resolution, or an erroring/oversized tool result is logged and skipped or
  returned as a soft tool error — it never fails an AgenticTask run.
• Safety: results are UTF-8-safe truncated to maxResultBytes; each call is
  bounded by callTimeout (and the dial handshake is bounded independently);
  auth headers come only from Secrets and are refused across cross-host
  redirects (so a redirect can't leak the token); MCP results are treated as
  untrusted model-facing data. Audit is the existing run transcript (MCP calls
  appear as normal mcp/<server>/<tool> tool calls).
• Lifecycle: MCP sessions are closed when the per-run context is done.

Built and reviewed task-by-task (11 commits) with a whole-branch review pass;
the one substantive finding (an inert callTimeout) is fixed in the final
commit. Fleet A/B validation on #409/#850 is tracked as the next step under
#1013.

Assisted-by: Claude Code (Foreman's authoring harness; generated the
implementation and tests task-by-task under my direction — I reviewed every
task, adjudicated the review findings, and ran make test / make lint
locally before submitting).

Checklist

• Tests added/updated
• make test passes locally
• make lint passes locally
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated (if user-facing change) — user-facing docs for the mcp config block follow once the feature is validated on the fleet (tracked in [FEATURE] MCP client for Foreman agents (docs + search grounding) #1013)

[codecov]

Codecov Report

❌ Patch coverage is 70.87087% with 97 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/foreman-agent/main.go	18.18%	34 Missing and 2 partials ⚠️
pkg/foreman/agent/mcp/client.go	61.76%	18 Missing and 8 partials ⚠️
api/foreman/v1alpha1/zz_generated.deepcopy.go	67.27%	18 Missing ⚠️
pkg/foreman/agent/mcp/registry.go	79.48%	5 Missing and 3 partials ⚠️
pkg/foreman/agent/mcp/session.go	75.00%	5 Missing and 2 partials ⚠️
pkg/foreman/agent/mcp/tool.go	97.14%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[joryirving]

Code review

Found 1 issue:

1. truncateUTF8 over-truncates and drops a whole rune whenever the byte cap lands on the last byte of a complete multibyte rune. The comment claims it cuts "to at most max bytes without splitting a multibyte rune," but it walks back past the full rune and then strips one more byte.

The first loop treats every trailing continuation byte as something to remove, but a continuation byte (0x80–0xBF) is also the last byte of a complete rune. So when s[:max] ends exactly on the final byte of a rune that fully fits, that rune is discarded, and the follow-up DecodeLastRuneInString then sees the orphaned leading byte and strips again. Verified against the implementation:

• truncateUTF8("ééé", 4) → "é" (2 bytes); correct is "éé" (4 bytes, fits exactly)
• truncateUTF8("ééé", 2) → "" (0 bytes); correct is "é" (2 bytes, fits exactly)
• truncateUTF8("𐍈𐍈", 4) → ""; correct is "𐍈" (4 bytes, fits exactly)

The existing test only exercises MaxResultBytes: 11 (lands mid-rune), so the boundary case never triggers it. MCP results routinely contain multibyte UTF-8 (code comments, non-ASCII text, emoji), and truncation at the default MaxResultBytes is common, so this silently loses data in practice rather than just in theory.

A minimal fix: after cut := s[:max], if utf8.Valid(cut) is already true, return cut unchanged — only walk back to the previous RuneStart when cut is actually invalid UTF-8. Worth adding a table-driven test case where the cap lands on a complete rune boundary.

LLMKube/pkg/foreman/agent/mcp/tool.go

Lines 199 to 218 in 697893f

	func truncateUTF8(s string, max int) string {
	if max <= 0 || len(s) <= max {
	return s
	}
	cut := s[:max]
	for len(cut) > 0 && !utf8.RuneStart(cut[len(cut)-1]) {
	cut = cut[:len(cut)-1]
	}
	// cut now ends either on a complete rune or (if the very start of a
	// multibyte rune was itself the byte we trimmed to) an incomplete
	// leading byte; utf8.RuneStart only confirms the byte *begins* a
	// rune, so verify the full rune actually fits before keeping it.
	if len(cut) > 0 {
	r, size := utf8.DecodeLastRuneInString(cut)
	if r == utf8.RuneError && size == 1 {
	cut = cut[:len(cut)-1]
	}
	}
	return cut
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}