diff --git a/go/core/internal/controller/translator/agent/adk_api_translator.go b/go/core/internal/controller/translator/agent/adk_api_translator.go index b11d030aa..c57b1fbe1 100644 --- a/go/core/internal/controller/translator/agent/adk_api_translator.go +++ b/go/core/internal/controller/translator/agent/adk_api_translator.go @@ -909,6 +909,7 @@ func (a *adkApiTranslator) translateMCPServerTarget(ctx context.Context, agent * } func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, agent *adk.AgentConfig, remoteMcpServer *v1alpha2.RemoteMCPServer, mcpServerTool *v1alpha2.McpServerTool, agentHeaders map[string]string, proxyURL string) error { + toolNames := resolveToolNames(mcpServerTool.ToolNames, remoteMcpServer.Status.DiscoveredTools) switch remoteMcpServer.Spec.Protocol { case v1alpha2.RemoteMCPServerProtocolSse: tool, err := a.translateSseHttpTool(ctx, remoteMcpServer, agentHeaders, proxyURL) @@ -917,7 +918,7 @@ func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, a } agent.SseTools = append(agent.SseTools, adk.SseMcpServerConfig{ Params: *tool, - Tools: mcpServerTool.ToolNames, + Tools: toolNames, AllowedHeaders: mcpServerTool.AllowedHeaders, RequireApproval: mcpServerTool.RequireApproval, }) @@ -928,7 +929,7 @@ func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, a } agent.HttpTools = append(agent.HttpTools, adk.HttpMcpServerConfig{ Params: *tool, - Tools: mcpServerTool.ToolNames, + Tools: toolNames, AllowedHeaders: mcpServerTool.AllowedHeaders, RequireApproval: mcpServerTool.RequireApproval, }) @@ -936,6 +937,24 @@ func (a *adkApiTranslator) translateRemoteMCPServerTarget(ctx context.Context, a return nil } +// resolveToolNames returns the explicit toolNames filter when set; otherwise it +// expands to the discovered tool list from the RemoteMCPServer status. The +// returned slice is always non-nil so it serializes as a JSON array (the Python +// ADK runtime rejects a null `tools` field with a pydantic validation error). +func resolveToolNames(explicit []string, discovered []*v1alpha2.MCPTool) []string { + if len(explicit) > 0 { + return explicit + } + out := make([]string, 0, len(discovered)) + for _, t := range discovered { + if t == nil { + continue + } + out = append(out, t.Name) + } + return out +} + // Helper functions // isInternalK8sURL checks if a URL points to an internal Kubernetes service. diff --git a/go/core/internal/controller/translator/agent/adk_api_translator_test.go b/go/core/internal/controller/translator/agent/adk_api_translator_test.go index 6f0dbb80f..408a4df84 100644 --- a/go/core/internal/controller/translator/agent/adk_api_translator_test.go +++ b/go/core/internal/controller/translator/agent/adk_api_translator_test.go @@ -2,7 +2,9 @@ package agent_test import ( "context" + "encoding/json" "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -1413,3 +1415,83 @@ func Test_AdkApiTranslator_SandboxAgent_BYOEmitsSandbox(t *testing.T) { require.False(t, sawDeploy) require.False(t, sawService, "sandbox runtime must not include Service; agent-sandbox owns it") } + +// Test_AdkApiTranslator_RemoteMCPServer_NoToolNames asserts that a RemoteMCPServer +// reference without an explicit toolNames filter materializes to a non-null +// `tools` JSON array, expanded from the server's discovered tool list. The +// Python ADK runtime rejects `tools: null` with a pydantic validation error, so +// the controller must emit an explicit list (https://github.com/kagent-dev/kagent/issues/...). +func Test_AdkApiTranslator_RemoteMCPServer_NoToolNames(t *testing.T) { + scheme := schemev1.Scheme + require.NoError(t, v1alpha2.AddToScheme(scheme)) + + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-ns"}} + modelConfig := &v1alpha2.ModelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "m1", Namespace: "test-ns"}, + Spec: v1alpha2.ModelConfigSpec{ + Model: "gpt-4", + Provider: v1alpha2.ModelProviderOpenAI, + }, + } + rms := &v1alpha2.RemoteMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "tools-server", Namespace: "test-ns"}, + Spec: v1alpha2.RemoteMCPServerSpec{ + Description: "tools server", + URL: "http://tools.example.com/mcp", + }, + Status: v1alpha2.RemoteMCPServerStatus{ + DiscoveredTools: []*v1alpha2.MCPTool{ + {Name: "alpha"}, + {Name: "beta"}, + nil, // defensive: nil entry must be skipped, not panic + }, + }, + } + agent := &v1alpha2.Agent{ + ObjectMeta: metav1.ObjectMeta{Name: "ag1", Namespace: "test-ns"}, + Spec: v1alpha2.AgentSpec{ + Type: v1alpha2.AgentType_Declarative, + Description: "agent", + Declarative: &v1alpha2.DeclarativeAgentSpec{ + SystemMessage: "test", + ModelConfig: "m1", + Tools: []*v1alpha2.Tool{ + { + Type: v1alpha2.ToolProviderType_McpServer, + McpServer: &v1alpha2.McpServerTool{ + TypedReference: v1alpha2.TypedReference{ + Kind: "RemoteMCPServer", + ApiGroup: "kagent.dev", + Name: "tools-server", + }, + // NOTE: deliberately no ToolNames + }, + }, + }, + }, + }, + } + + kubeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ns, modelConfig, rms, agent). + Build() + + defaultModel := types.NamespacedName{Namespace: "test-ns", Name: "m1"} + trans := translator.NewAdkApiTranslator(kubeClient, defaultModel, nil, "", nil) + + outputs, err := translator.TranslateAgent(context.Background(), trans, agent) + require.NoError(t, err) + require.NotNil(t, outputs) + require.NotNil(t, outputs.Config) + require.Len(t, outputs.Config.HttpTools, 1) + assert.Equal(t, []string{"alpha", "beta"}, outputs.Config.HttpTools[0].Tools) + + // JSON wire format must not contain `"tools":null` (the Python ADK runtime + // rejects it with a pydantic validation error). + raw, err := json.Marshal(outputs.Config) + require.NoError(t, err) + assert.NotContains(t, string(raw), `"tools":null`) + assert.True(t, strings.Contains(string(raw), `"tools":["alpha","beta"]`), + "expected tools to be inlined as a JSON array, got: %s", string(raw)) +} diff --git a/python/packages/kagent-adk/src/kagent/adk/types.py b/python/packages/kagent-adk/src/kagent/adk/types.py index 635fca9c9..27b09c8a0 100644 --- a/python/packages/kagent-adk/src/kagent/adk/types.py +++ b/python/packages/kagent-adk/src/kagent/adk/types.py @@ -1,5 +1,5 @@ import logging -from typing import Any, Callable, Literal, Optional, Union +from typing import Annotated, Any, Callable, Literal, Optional, Union import httpx from agentsts.adk import ADKTokenPropagationPlugin @@ -11,7 +11,7 @@ from google.adk.models.anthropic_llm import Claude as ClaudeLLM from google.adk.models.google_llm import Gemini as GeminiLLM from google.adk.tools.mcp_tool import SseConnectionParams, StreamableHTTPConnectionParams -from pydantic import AliasChoices, BaseModel, Field +from pydantic import AliasChoices, BaseModel, BeforeValidator, Field from kagent.adk._approval import make_approval_callback, strip_confirmation_parts_callback from kagent.adk._mcp_toolset import KAgentMcpToolset @@ -140,16 +140,23 @@ def _convert_ollama_options(options: dict[str, str] | None) -> dict[str, Any]: return converted +def _coerce_none_to_empty_list(v: Any) -> Any: + # The kagent controller has historically emitted `tools: null` for + # RemoteMCPServer refs without an explicit toolNames filter (kagent#TBD). + # Tolerate it here so older controller versions don't crash the runtime. + return [] if v is None else v + + class HttpMcpServerConfig(BaseModel): params: StreamableHTTPConnectionParams - tools: list[str] = Field(default_factory=list) + tools: Annotated[list[str], BeforeValidator(_coerce_none_to_empty_list)] = Field(default_factory=list) allowed_headers: list[str] | None = None # Headers to forward from A2A request to MCP calls require_approval: list[str] | None = None # Tools requiring human approval before execution class SseMcpServerConfig(BaseModel): params: SseConnectionParams - tools: list[str] = Field(default_factory=list) + tools: Annotated[list[str], BeforeValidator(_coerce_none_to_empty_list)] = Field(default_factory=list) allowed_headers: list[str] | None = None # Headers to forward from A2A request to MCP calls require_approval: list[str] | None = None # Tools requiring human approval before execution diff --git a/python/packages/kagent-adk/tests/unittests/test_http_tool_null_tools.py b/python/packages/kagent-adk/tests/unittests/test_http_tool_null_tools.py new file mode 100644 index 000000000..488986a8d --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_http_tool_null_tools.py @@ -0,0 +1,104 @@ +"""Regression test for kagent#1797: the controller has historically emitted +``http_tools[*].tools = null`` for RemoteMCPServer refs without an explicit +``toolNames`` filter. The runtime must tolerate that input rather than crashing +with a pydantic ``list_type`` validation error at startup. +""" + +import json + +import pytest + +from kagent.adk.types import AgentConfig, HttpMcpServerConfig, SseMcpServerConfig + + +_HTTP_PARAMS = {"url": "http://example/mcp", "headers": {}, "terminate_on_close": True} +_SSE_PARAMS = {"url": "http://example/mcp", "headers": {}} + + +@pytest.mark.parametrize( + "model, params", + [ + (HttpMcpServerConfig, _HTTP_PARAMS), + (SseMcpServerConfig, _SSE_PARAMS), + ], +) +def test_tools_none_is_coerced_to_empty_list(model, params): + cfg = model.model_validate({"params": params, "tools": None}) + assert cfg.tools == [] + + +@pytest.mark.parametrize( + "model, params", + [ + (HttpMcpServerConfig, _HTTP_PARAMS), + (SseMcpServerConfig, _SSE_PARAMS), + ], +) +def test_tools_missing_defaults_to_empty_list(model, params): + cfg = model.model_validate({"params": params}) + assert cfg.tools == [] + + +@pytest.mark.parametrize( + "model, params", + [ + (HttpMcpServerConfig, _HTTP_PARAMS), + (SseMcpServerConfig, _SSE_PARAMS), + ], +) +def test_explicit_tools_list_is_preserved(model, params): + cfg = model.model_validate({"params": params, "tools": ["alpha", "beta"]}) + assert cfg.tools == ["alpha", "beta"] + + +def _make_agent_config_blob(http_tools_value, sse_tools_value): + """Reproduce the exact wire format the kagent controller writes to the + agent's ``config.json`` Secret. Any field-level coercion in + ``HttpMcpServerConfig`` / ``SseMcpServerConfig`` must keep this top-level + construction path working — that path is what crashes the runtime pod on + startup.""" + return { + "model": { + "type": "openai", + "model": "gpt-4o-mini", + "api_key": "dummy", + }, + "description": "repro for #1797", + "instruction": "test", + "http_tools": [ + { + "params": { + "url": "http://example/mcp", + "headers": {}, + "terminate_on_close": True, + }, + "tools": http_tools_value, + } + ], + "sse_tools": [ + { + "params": {"url": "http://example/sse", "headers": {}}, + "tools": sse_tools_value, + } + ], + } + + +def test_agent_config_load_with_null_tools_does_not_crash(): + """End-to-end runtime startup path: load the exact JSON blob the controller + writes when ``toolNames`` is omitted. Before the fix this raised + ``ValidationError: Input should be a valid list ... input_value=None``.""" + blob = _make_agent_config_blob(http_tools_value=None, sse_tools_value=None) + cfg = AgentConfig.model_validate_json(json.dumps(blob)) + assert cfg.http_tools[0].tools == [] + assert cfg.sse_tools[0].tools == [] + + +def test_agent_config_load_with_explicit_tools_preserved(): + blob = _make_agent_config_blob( + http_tools_value=["alpha", "beta"], + sse_tools_value=["gamma"], + ) + cfg = AgentConfig.model_validate_json(json.dumps(blob)) + assert cfg.http_tools[0].tools == ["alpha", "beta"] + assert cfg.sse_tools[0].tools == ["gamma"]