Skip to content

Commit 0aea970

Browse files
reilly3000claude
andcommitted
fix: tolerate empty toolNames on RemoteMCPServer refs (closes #1797)
When an Agent's `tools[*].mcpServer` references a RemoteMCPServer without an explicit `toolNames` filter, the controller serializes `http_tools[*].tools` (and `sse_tools[*].tools`) as JSON `null`, because Go's `encoding/json` marshals a nil `[]string` slice without `omitempty` as `null`. The Python ADK runtime then crashes on startup with a pydantic ValidationError, leaving the agent pod in CrashLoopBackOff: http_tools.0.tools Input should be a valid list [type=list_type, input_value=None, input_type=NoneType] Whether `toolNames` is intended to be required or optional is a call for maintainers (see #1797 for discussion); either way, the controller should not produce a config the runtime cannot load. This change is defensive on both sides: * Controller (Go): when `toolNames` is empty, expand to the RemoteMCPServer's `.status.discoveredTools` list so the rendered config is explicit and observable. Explicit `toolNames` continue to take precedence. Falls back to an empty array (rather than `null`) if the status has no discovered tools yet. * Runtime (Python): coerce `tools: None` to `[]` via a pydantic BeforeValidator on `HttpMcpServerConfig` / `SseMcpServerConfig`, so older controller versions don't crash newer runtimes. Adds a translator regression test asserting the rendered AgentConfig contains a non-null `tools` array, and a pydantic-level test for the runtime coercion. Happy to rework toward a different direction (e.g. CRD validation making `toolNames` required) if that better matches the intended semantics. Signed-off-by: Chris <chris@chris-reilly.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent cfcd66d commit 0aea970

4 files changed

Lines changed: 218 additions & 6 deletions

File tree

go/core/internal/controller/translator/agent/adk_api_translator.go

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,7 @@ func (a *adkApiTranslator) translateMCPServerTarget(ctx context.Context, agent *
909909
}
910910

911911
func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, agent *adk.AgentConfig, remoteMcpServer *v1alpha2.RemoteMCPServer, mcpServerTool *v1alpha2.McpServerTool, agentHeaders map[string]string, proxyURL string) error {
912+
toolNames := resolveToolNames(mcpServerTool.ToolNames, remoteMcpServer.Status.DiscoveredTools)
912913
switch remoteMcpServer.Spec.Protocol {
913914
case v1alpha2.RemoteMCPServerProtocolSse:
914915
tool, err := a.translateSseHttpTool(ctx, remoteMcpServer, agentHeaders, proxyURL)
@@ -917,7 +918,7 @@ func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, a
917918
}
918919
agent.SseTools = append(agent.SseTools, adk.SseMcpServerConfig{
919920
Params: *tool,
920-
Tools: mcpServerTool.ToolNames,
921+
Tools: toolNames,
921922
AllowedHeaders: mcpServerTool.AllowedHeaders,
922923
RequireApproval: mcpServerTool.RequireApproval,
923924
})
@@ -928,14 +929,32 @@ func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, a
928929
}
929930
agent.HttpTools = append(agent.HttpTools, adk.HttpMcpServerConfig{
930931
Params: *tool,
931-
Tools: mcpServerTool.ToolNames,
932+
Tools: toolNames,
932933
AllowedHeaders: mcpServerTool.AllowedHeaders,
933934
RequireApproval: mcpServerTool.RequireApproval,
934935
})
935936
}
936937
return nil
937938
}
938939

940+
// resolveToolNames returns the explicit toolNames filter when set; otherwise it
941+
// expands to the discovered tool list from the RemoteMCPServer status. The
942+
// returned slice is always non-nil so it serializes as a JSON array (the Python
943+
// ADK runtime rejects a null `tools` field with a pydantic validation error).
944+
func resolveToolNames(explicit []string, discovered []*v1alpha2.MCPTool) []string {
945+
if len(explicit) > 0 {
946+
return explicit
947+
}
948+
out := make([]string, 0, len(discovered))
949+
for _, t := range discovered {
950+
if t == nil {
951+
continue
952+
}
953+
out = append(out, t.Name)
954+
}
955+
return out
956+
}
957+
939958
// Helper functions
940959

941960
// isInternalK8sURL checks if a URL points to an internal Kubernetes service.

go/core/internal/controller/translator/agent/adk_api_translator_test.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ package agent_test
22

33
import (
44
"context"
5+
"encoding/json"
56
"fmt"
7+
"strings"
68
"testing"
79

810
"github.com/stretchr/testify/assert"
@@ -1413,3 +1415,83 @@ func Test_AdkApiTranslator_SandboxAgent_BYOEmitsSandbox(t *testing.T) {
14131415
require.False(t, sawDeploy)
14141416
require.False(t, sawService, "sandbox runtime must not include Service; agent-sandbox owns it")
14151417
}
1418+
1419+
// Test_AdkApiTranslator_RemoteMCPServer_NoToolNames asserts that a RemoteMCPServer
1420+
// reference without an explicit toolNames filter materializes to a non-null
1421+
// `tools` JSON array, expanded from the server's discovered tool list. The
1422+
// Python ADK runtime rejects `tools: null` with a pydantic validation error, so
1423+
// the controller must emit an explicit list (https://github.com/kagent-dev/kagent/issues/...).
1424+
func Test_AdkApiTranslator_RemoteMCPServer_NoToolNames(t *testing.T) {
1425+
scheme := schemev1.Scheme
1426+
require.NoError(t, v1alpha2.AddToScheme(scheme))
1427+
1428+
ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-ns"}}
1429+
modelConfig := &v1alpha2.ModelConfig{
1430+
ObjectMeta: metav1.ObjectMeta{Name: "m1", Namespace: "test-ns"},
1431+
Spec: v1alpha2.ModelConfigSpec{
1432+
Model: "gpt-4",
1433+
Provider: v1alpha2.ModelProviderOpenAI,
1434+
},
1435+
}
1436+
rms := &v1alpha2.RemoteMCPServer{
1437+
ObjectMeta: metav1.ObjectMeta{Name: "tools-server", Namespace: "test-ns"},
1438+
Spec: v1alpha2.RemoteMCPServerSpec{
1439+
Description: "tools server",
1440+
URL: "http://tools.example.com/mcp",
1441+
},
1442+
Status: v1alpha2.RemoteMCPServerStatus{
1443+
DiscoveredTools: []*v1alpha2.MCPTool{
1444+
{Name: "alpha"},
1445+
{Name: "beta"},
1446+
nil, // defensive: nil entry must be skipped, not panic
1447+
},
1448+
},
1449+
}
1450+
agent := &v1alpha2.Agent{
1451+
ObjectMeta: metav1.ObjectMeta{Name: "ag1", Namespace: "test-ns"},
1452+
Spec: v1alpha2.AgentSpec{
1453+
Type: v1alpha2.AgentType_Declarative,
1454+
Description: "agent",
1455+
Declarative: &v1alpha2.DeclarativeAgentSpec{
1456+
SystemMessage: "test",
1457+
ModelConfig: "m1",
1458+
Tools: []*v1alpha2.Tool{
1459+
{
1460+
Type: v1alpha2.ToolProviderType_McpServer,
1461+
McpServer: &v1alpha2.McpServerTool{
1462+
TypedReference: v1alpha2.TypedReference{
1463+
Kind: "RemoteMCPServer",
1464+
ApiGroup: "kagent.dev",
1465+
Name: "tools-server",
1466+
},
1467+
// NOTE: deliberately no ToolNames
1468+
},
1469+
},
1470+
},
1471+
},
1472+
},
1473+
}
1474+
1475+
kubeClient := fake.NewClientBuilder().
1476+
WithScheme(scheme).
1477+
WithObjects(ns, modelConfig, rms, agent).
1478+
Build()
1479+
1480+
defaultModel := types.NamespacedName{Namespace: "test-ns", Name: "m1"}
1481+
trans := translator.NewAdkApiTranslator(kubeClient, defaultModel, nil, "", nil)
1482+
1483+
outputs, err := translator.TranslateAgent(context.Background(), trans, agent)
1484+
require.NoError(t, err)
1485+
require.NotNil(t, outputs)
1486+
require.NotNil(t, outputs.Config)
1487+
require.Len(t, outputs.Config.HttpTools, 1)
1488+
assert.Equal(t, []string{"alpha", "beta"}, outputs.Config.HttpTools[0].Tools)
1489+
1490+
// JSON wire format must not contain `"tools":null` (the Python ADK runtime
1491+
// rejects it with a pydantic validation error).
1492+
raw, err := json.Marshal(outputs.Config)
1493+
require.NoError(t, err)
1494+
assert.NotContains(t, string(raw), `"tools":null`)
1495+
assert.True(t, strings.Contains(string(raw), `"tools":["alpha","beta"]`),
1496+
"expected tools to be inlined as a JSON array, got: %s", string(raw))
1497+
}

python/packages/kagent-adk/src/kagent/adk/types.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import logging
2-
from typing import Any, Callable, Literal, Optional, Union
2+
from typing import Annotated, Any, Callable, Literal, Optional, Union
33

44
import httpx
55
from agentsts.adk import ADKTokenPropagationPlugin
@@ -11,7 +11,7 @@
1111
from google.adk.models.anthropic_llm import Claude as ClaudeLLM
1212
from google.adk.models.google_llm import Gemini as GeminiLLM
1313
from google.adk.tools.mcp_tool import SseConnectionParams, StreamableHTTPConnectionParams
14-
from pydantic import AliasChoices, BaseModel, Field
14+
from pydantic import AliasChoices, BaseModel, BeforeValidator, Field
1515

1616
from kagent.adk._approval import make_approval_callback, strip_confirmation_parts_callback
1717
from kagent.adk._mcp_toolset import KAgentMcpToolset
@@ -140,16 +140,23 @@ def _convert_ollama_options(options: dict[str, str] | None) -> dict[str, Any]:
140140
return converted
141141

142142

143+
def _coerce_none_to_empty_list(v: Any) -> Any:
144+
# The kagent controller has historically emitted `tools: null` for
145+
# RemoteMCPServer refs without an explicit toolNames filter (kagent#TBD).
146+
# Tolerate it here so older controller versions don't crash the runtime.
147+
return [] if v is None else v
148+
149+
143150
class HttpMcpServerConfig(BaseModel):
144151
params: StreamableHTTPConnectionParams
145-
tools: list[str] = Field(default_factory=list)
152+
tools: Annotated[list[str], BeforeValidator(_coerce_none_to_empty_list)] = Field(default_factory=list)
146153
allowed_headers: list[str] | None = None # Headers to forward from A2A request to MCP calls
147154
require_approval: list[str] | None = None # Tools requiring human approval before execution
148155

149156

150157
class SseMcpServerConfig(BaseModel):
151158
params: SseConnectionParams
152-
tools: list[str] = Field(default_factory=list)
159+
tools: Annotated[list[str], BeforeValidator(_coerce_none_to_empty_list)] = Field(default_factory=list)
153160
allowed_headers: list[str] | None = None # Headers to forward from A2A request to MCP calls
154161
require_approval: list[str] | None = None # Tools requiring human approval before execution
155162

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
"""Regression test for kagent#1797: the controller has historically emitted
2+
``http_tools[*].tools = null`` for RemoteMCPServer refs without an explicit
3+
``toolNames`` filter. The runtime must tolerate that input rather than crashing
4+
with a pydantic ``list_type`` validation error at startup.
5+
"""
6+
7+
import json
8+
9+
import pytest
10+
11+
from kagent.adk.types import AgentConfig, HttpMcpServerConfig, SseMcpServerConfig
12+
13+
14+
_HTTP_PARAMS = {"url": "http://example/mcp", "headers": {}, "terminate_on_close": True}
15+
_SSE_PARAMS = {"url": "http://example/mcp", "headers": {}}
16+
17+
18+
@pytest.mark.parametrize(
19+
"model, params",
20+
[
21+
(HttpMcpServerConfig, _HTTP_PARAMS),
22+
(SseMcpServerConfig, _SSE_PARAMS),
23+
],
24+
)
25+
def test_tools_none_is_coerced_to_empty_list(model, params):
26+
cfg = model.model_validate({"params": params, "tools": None})
27+
assert cfg.tools == []
28+
29+
30+
@pytest.mark.parametrize(
31+
"model, params",
32+
[
33+
(HttpMcpServerConfig, _HTTP_PARAMS),
34+
(SseMcpServerConfig, _SSE_PARAMS),
35+
],
36+
)
37+
def test_tools_missing_defaults_to_empty_list(model, params):
38+
cfg = model.model_validate({"params": params})
39+
assert cfg.tools == []
40+
41+
42+
@pytest.mark.parametrize(
43+
"model, params",
44+
[
45+
(HttpMcpServerConfig, _HTTP_PARAMS),
46+
(SseMcpServerConfig, _SSE_PARAMS),
47+
],
48+
)
49+
def test_explicit_tools_list_is_preserved(model, params):
50+
cfg = model.model_validate({"params": params, "tools": ["alpha", "beta"]})
51+
assert cfg.tools == ["alpha", "beta"]
52+
53+
54+
def _make_agent_config_blob(http_tools_value, sse_tools_value):
55+
"""Reproduce the exact wire format the kagent controller writes to the
56+
agent's ``config.json`` Secret. Any field-level coercion in
57+
``HttpMcpServerConfig`` / ``SseMcpServerConfig`` must keep this top-level
58+
construction path working — that path is what crashes the runtime pod on
59+
startup."""
60+
return {
61+
"model": {
62+
"type": "openai",
63+
"model": "gpt-4o-mini",
64+
"api_key": "dummy",
65+
},
66+
"description": "repro for #1797",
67+
"instruction": "test",
68+
"http_tools": [
69+
{
70+
"params": {
71+
"url": "http://example/mcp",
72+
"headers": {},
73+
"terminate_on_close": True,
74+
},
75+
"tools": http_tools_value,
76+
}
77+
],
78+
"sse_tools": [
79+
{
80+
"params": {"url": "http://example/sse", "headers": {}},
81+
"tools": sse_tools_value,
82+
}
83+
],
84+
}
85+
86+
87+
def test_agent_config_load_with_null_tools_does_not_crash():
88+
"""End-to-end runtime startup path: load the exact JSON blob the controller
89+
writes when ``toolNames`` is omitted. Before the fix this raised
90+
``ValidationError: Input should be a valid list ... input_value=None``."""
91+
blob = _make_agent_config_blob(http_tools_value=None, sse_tools_value=None)
92+
cfg = AgentConfig.model_validate_json(json.dumps(blob))
93+
assert cfg.http_tools[0].tools == []
94+
assert cfg.sse_tools[0].tools == []
95+
96+
97+
def test_agent_config_load_with_explicit_tools_preserved():
98+
blob = _make_agent_config_blob(
99+
http_tools_value=["alpha", "beta"],
100+
sse_tools_value=["gamma"],
101+
)
102+
cfg = AgentConfig.model_validate_json(json.dumps(blob))
103+
assert cfg.http_tools[0].tools == ["alpha", "beta"]
104+
assert cfg.sse_tools[0].tools == ["gamma"]

0 commit comments

Comments
 (0)