diff --git a/internal/http/mcp.go b/internal/http/mcp.go index 0d15eaee..9a180c98 100644 --- a/internal/http/mcp.go +++ b/internal/http/mcp.go @@ -18,9 +18,14 @@ import ( "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) -// MCPToolLister returns discovered tool names for a specific MCP server. +// MCPToolLister returns discovered tool names/info for a specific MCP server. type MCPToolLister interface { ServerToolNames(serverName string) []string + // ServerToolInfos returns the original (bare, unprefixed) tool name and + // real description for each tool of an already-connected server. Bare + // names match the shape returned by mcp.DiscoverTools, so callers don't + // have to special-case "already connected" vs. "on-demand discovery". + ServerToolInfos(serverName string) []mcp.ToolInfo } // MCPPoolEvictor evicts pooled connections for a tenant+server (called on credential rotation). diff --git a/internal/http/mcp_tools.go b/internal/http/mcp_tools.go index ef892a71..877c1459 100644 --- a/internal/http/mcp_tools.go +++ b/internal/http/mcp_tools.go @@ -89,14 +89,19 @@ func (h *MCPHandler) handleListServerTools(w http.ResponseWriter, r *http.Reques return } - // Try runtime Manager first — returns names only (no descriptions available). + // Try runtime Manager first — returns the original (bare, unprefixed) MCP + // tool names with their real descriptions, sourced from the live + // *BridgeTool for each registered tool. This must match the bare-name + // shape returned by the DiscoverTools fallback below: tool_allow grants + // saved from whichever shape this endpoint happens to return are matched + // against bare names by ListToolsForAgent's tool_cache lookups + // (internal/mcp/manager.go), so returning the registered/prefixed name + // here (as before) silently broke prompt-preview schemas for any server + // that was already live-connected when its grants were configured. var tools []mcpbridge.ToolInfo if h.mgr != nil { - if names := h.mgr.ServerToolNames(srv.Name); len(names) > 0 { - tools = make([]mcpbridge.ToolInfo, len(names)) - for i, n := range names { - tools[i] = mcpbridge.ToolInfo{Name: n} - } + if infos := h.mgr.ServerToolInfos(srv.Name); len(infos) > 0 { + tools = infos } } diff --git a/internal/mcp/list_tools_for_agent_test.go b/internal/mcp/list_tools_for_agent_test.go index 8aac2c76..9149853f 100644 --- a/internal/mcp/list_tools_for_agent_test.go +++ b/internal/mcp/list_tools_for_agent_test.go @@ -3,9 +3,12 @@ package mcp import ( "encoding/json" "sort" + "sync/atomic" "testing" "github.com/google/uuid" + mcpclient "github.com/mark3labs/mcp-go/client" + mcpgo "github.com/mark3labs/mcp-go/mcp" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" @@ -194,6 +197,119 @@ func TestListToolsForAgent_LegacyCacheShapeDegradesGracefully(t *testing.T) { } } +// TestListToolsForAgent_LegacyPrefixedToolAllowResolvesLiveDescriptions +// reproduces the production regression: an agent's MCP grant has a persisted +// tool_allow list containing OLD registered-name-shaped entries (e.g. +// "mcp_goclaw__goclaw_agent_get" instead of the bare "goclaw_agent_get"), +// captured before the grant-capture fix in mcp_tools.go. Before this fix, +// ListToolsForAgent looked up tool_cache/registry entries keyed by the raw +// stored name, which never matched the bare-name keys used elsewhere, +// silently producing empty descriptions/schemas for every tool. This test +// asserts descriptions and parameter schemas are still resolved correctly — +// both from a live-registered tool and from the settings tool_cache — by +// normalizing the legacy prefixed entries to bare names first. +func TestListToolsForAgent_LegacyPrefixedToolAllowResolvesLiveDescriptions(t *testing.T) { + serverID := uuid.New() + + // One tool ("goclaw_agent_get") is live-registered in the tool registry + // (simulating an already-connected self-hosted MCP server), and its + // description/schema must come from there, not from settings. + registry := tools.NewRegistry() + clientPtr := &atomic.Pointer[mcpclient.Client]{} + connected := &atomic.Bool{} + connected.Store(true) + liveTool := NewBridgeTool( + "goclaw", + mcpgo.Tool{ + Name: "goclaw_agent_get", + Description: "Fetch an agent by ID", + InputSchema: mcpgo.ToolInputSchema{ + Type: "object", + Properties: map[string]any{"agent_id": map[string]any{"type": "string"}}, + Required: []string{"agent_id"}, + }, + }, + clientPtr, "", 0, connected, uuid.Nil, nil, + ) + registry.Register(liveTool) + + // The other tool ("goclaw_agent_list") has no live registration but does + // have a settings tool_cache entry — description must come from there. + toolCache := map[string]store.CachedToolInfo{ + "goclaw_agent_list": { + Description: "List all agents", + Parameters: json.RawMessage(`{"type":"object","properties":{}}`), + }, + } + settings, err := json.Marshal(map[string]any{"tool_cache": toolCache}) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "goclaw", + Enabled: true, + Settings: settings, + }, + // Legacy-persisted entries: already-prefixed registered names, + // exactly the shape a pre-fix grant capture would have stored. + ToolAllow: []string{ + "mcp_goclaw__goclaw_agent_get", + "mcp_goclaw__goclaw_agent_list", + }, + ToolDeny: nil, + }, + }, + } + + mgr := NewManager(registry, WithStore(mockStore)) + + got, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 tool entries, got %d: %+v", len(got), got) + } + + sort.Slice(got, func(i, j int) bool { return got[i].RegisteredName < got[j].RegisteredName }) + + // Live-registered tool: description/schema resolved from the registry. + live := got[0] + if live.RegisteredName != "mcp_goclaw__goclaw_agent_get" { + t.Fatalf("unexpected registered name: %q", live.RegisteredName) + } + if live.Description != "Fetch an agent by ID" { + t.Fatalf("expected live description resolved from registry, got %q (has_desc=%v)", live.Description, live.Description != "") + } + if len(live.Parameters) == 0 { + t.Fatal("expected live parameter schema resolved from registry, got empty") + } + var liveSchema map[string]any + if err := json.Unmarshal(live.Parameters, &liveSchema); err != nil { + t.Fatalf("unmarshal live parameters: %v", err) + } + if _, ok := liveSchema["properties"].(map[string]any)["agent_id"]; !ok { + t.Errorf("expected 'agent_id' property in live schema, got %+v", liveSchema) + } + + // Cache-only tool: description/schema resolved from settings tool_cache. + cached := got[1] + if cached.RegisteredName != "mcp_goclaw__goclaw_agent_list" { + t.Fatalf("unexpected registered name: %q", cached.RegisteredName) + } + if cached.Description != "List all agents" { + t.Fatalf("expected cached description resolved despite legacy prefixed tool_allow entry, got %q", cached.Description) + } + if len(cached.Parameters) == 0 { + t.Fatal("expected cached parameter schema resolved despite legacy prefixed tool_allow entry, got empty") + } +} + // TestListToolsForAgent_EmptyToolAllowNoCacheFallsBackToPlaceholder verifies // that when a server has never been connected (no tool_cache present), the // existing single-placeholder behavior is preserved. diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index bdf51525..e1374a62 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "log/slog" + "strings" "sync" "sync/atomic" "time" @@ -674,6 +675,67 @@ type MCPToolPreviewInfo struct { Parameters json.RawMessage } +// mcpPreviewDiscoveryTimeout bounds the on-demand tool discovery +// ListToolsForAgent performs when a server has never been live-connected for +// this agent (empty tool_cache and empty registry). Kept short since it's on +// the hot path of a prompt-preview HTTP request, unlike the longer +// discoverToolsTimeout used by the dedicated admin "test connection"/"browse +// tools" endpoints. +const mcpPreviewDiscoveryTimeout = 5 * time.Second + +// bareMCPToolName strips a persisted "{effectivePrefix}__" prefix from a +// stored tool_allow/tool_deny entry, if present. +// +// Historically, some agent grants were captured while their MCP server was +// already live-connected and ended up storing the registered (prefixed) tool +// name instead of the bare original MCP tool name — see ServerToolInfos' +// doc comment for how that mismatch happened. Those legacy rows are never +// rewritten automatically, so ListToolsForAgent must accept both shapes +// going forward: bare names (the current, correct shape) and prefixed names +// (already-persisted legacy grants), normalizing to bare before matching +// against tool_cache keys or the live tool registry. +func bareMCPToolName(stored, effectivePrefix string) string { + if bare, ok := strings.CutPrefix(stored, effectivePrefix+"__"); ok { + return bare + } + return stored +} + +// resolveMCPToolInfo resolves the description and parameter schema for a bare +// MCP tool name, preferring the CURRENT live registry entry (if the server is +// connected right now) over the settings-persisted tool_cache snapshot, which +// can be stale or entirely absent. Admin-authored hints always take priority +// over both sources; the server's global hint is the last-resort fallback. +func (m *Manager) resolveMCPToolInfo(toolName, effectivePrefix string, hints ToolHints, toolCache map[string]store.CachedToolInfo) (string, json.RawMessage) { + desc := hints.HintFor(toolName) + var params json.RawMessage + + if tool, ok := m.registry.Get(effectivePrefix + "__" + toolName); ok { + if bridgeTool, isBridge := tool.(*BridgeTool); isBridge { + if desc == "" { + desc = bridgeTool.Description() + } + if schema := bridgeTool.Parameters(); schema != nil { + if schemaJSON, err := json.Marshal(schema); err == nil { + params = schemaJSON + } + } + } + } + + cached := toolCache[toolName] + if desc == "" { + desc = cached.Description + } + if params == nil { + params = cached.Parameters + } + if desc == "" && hints.Global != "" { + desc = hints.Global + } + return desc, params +} + // ListToolsForAgent returns a best-effort list of MCP tool names and descriptions // for a given agent+user based on store configuration only — no actual MCP // server connections are made. It is intended for prompt preview. @@ -737,10 +799,42 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user } } - // Build deny set + // Nothing persisted yet and no live registry entries either (the + // server has never actually been connected for this agent, e.g. a + // freshly added MCP server before the agent's first real chat turn + // triggers Manager.LoadForAgent). ListToolsForAgent is otherwise + // documented as "no actual MCP server connections are made", which + // left the prompt preview permanently showing empty descriptions and + // "{type: object}" schemas until a real session happened to connect. + // Do a single bounded on-demand discovery here (same live path the + // admin "browse tools" endpoint already uses, see handleListServerTools + // in internal/http/mcp_tools.go) so the preview reflects real tool + // data immediately, and persist it so subsequent calls hit the cache. + if len(toolCache) == 0 && len(m.ServerToolInfos(info.Server.Name)) == 0 { + if rs := m.resolveServerCredentials(ctx, info, userID); rs != nil { + discovered, err := discoverRawTools(ctx, info.Server.Transport, info.Server.Command, rs.args, rs.env, info.Server.URL, rs.headers, mcpPreviewDiscoveryTimeout) + if err != nil { + slog.Debug("mcp.ListToolsForAgent.on_demand_discovery_failed", "server", info.Server.Name, "error", err) + } else { + toolCache = buildCachedToolInfo(discovered) + if info.Server.ID != uuid.Nil && m.store != nil && len(toolCache) > 0 { + go func(sid uuid.UUID, cache map[string]store.CachedToolInfo) { + cacheCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if cacheErr := m.store.CacheToolDescriptions(cacheCtx, sid, cache); cacheErr != nil { + slog.Debug("mcp.ListToolsForAgent.cache_tool_descriptions_failed", "server_id", sid, "error", cacheErr) + } + }(info.Server.ID, toolCache) + } + } + } + } + + // Build deny set, normalizing legacy prefixed entries to bare names + // (see bareMCPToolName) so they match tool_cache/registry lookups. denySet := make(map[string]struct{}, len(info.ToolDeny)) for _, d := range info.ToolDeny { - denySet[d] = struct{}{} + denySet[bareMCPToolName(d, effectivePrefix)] = struct{}{} } if len(info.ToolAllow) == 0 { @@ -754,19 +848,12 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user continue } registeredName := effectivePrefix + "__" + toolName - cached := toolCache[toolName] - desc := hints.HintFor(toolName) - if desc == "" { - desc = cached.Description - } - if desc == "" && hints.Global != "" { - desc = hints.Global - } + desc, params := m.resolveMCPToolInfo(toolName, effectivePrefix, hints, toolCache) serverTools = append(serverTools, registeredName) result = append(result, MCPToolPreviewInfo{ RegisteredName: registeredName, Description: desc, - Parameters: cached.Parameters, + Parameters: params, }) } slog.Debug("mcp.ListToolsForAgent.server_tools_added_from_cache", "server", info.Server.Name, "tools", serverTools) @@ -788,25 +875,22 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user } var serverTools []string - for _, toolName := range info.ToolAllow { + for _, storedName := range info.ToolAllow { + // Normalize legacy prefixed entries (persisted before the grant-capture + // fix in mcp_tools.go) to the bare tool name so they resolve against + // tool_cache and the live registry the same as current bare entries. + toolName := bareMCPToolName(storedName, effectivePrefix) if _, denied := denySet[toolName]; denied { slog.Debug("mcp.ListToolsForAgent.tool_denied", "server", info.Server.Name, "tool", toolName) continue } registeredName := effectivePrefix + "__" + toolName - cached := toolCache[toolName] - desc := hints.HintFor(toolName) - if desc == "" { - desc = cached.Description - } - if desc == "" && hints.Global != "" { - desc = hints.Global - } + desc, params := m.resolveMCPToolInfo(toolName, effectivePrefix, hints, toolCache) serverTools = append(serverTools, registeredName) result = append(result, MCPToolPreviewInfo{ RegisteredName: registeredName, Description: desc, - Parameters: cached.Parameters, + Parameters: params, }) } slog.Debug("mcp.ListToolsForAgent.server_tools_added", "server", info.Server.Name, "tools", serverTools) diff --git a/internal/mcp/manager_tools.go b/internal/mcp/manager_tools.go index 513f1fe1..30d28adf 100644 --- a/internal/mcp/manager_tools.go +++ b/internal/mcp/manager_tools.go @@ -47,6 +47,57 @@ func (m *Manager) ServerToolNames(serverName string) []string { return nil } +// ServerToolInfos returns ToolInfo (original/bare tool name + real description) +// for a server that is already live-connected in this Manager, by looking up +// each registered (prefixed) tool name in the registry and reading its +// original MCP name and description off the live *BridgeTool. +// +// This exists so callers needing tool metadata for an already-connected +// server (e.g. handleListServerTools) get the SAME bare-name shape as the +// on-demand DiscoverTools fallback (manager_tools.go DiscoverTools), instead +// of the registered/prefixed names returned by ServerToolNames. Mixing the +// two shapes previously caused a server's tool_allow grant (built from +// whichever name shape the UI happened to see, depending on whether the +// server was already connected) to end up keyed by a prefixed name that +// never matches the bare-name keys used by buildCachedToolInfo +// (manager_connect.go) and ListToolsForAgent's tool_cache lookups +// (manager.go), silently producing empty description/parameters in the +// prompt preview for tools whose grant was captured while the server was +// live-connected — most commonly a server (like goclaw's own CRUD server) +// that self-connects and therefore tends to already be live by the time an +// operator configures its grants. +func (m *Manager) ServerToolInfos(serverName string) []ToolInfo { + m.mu.RLock() + var registeredNames []string + if _, isPool := m.poolServers[serverName]; isPool { + registeredNames = m.poolToolNames[serverName] + } else if ss, ok := m.servers[serverName]; ok { + registeredNames = ss.toolNames + } + m.mu.RUnlock() + + if len(registeredNames) == 0 { + return nil + } + + infos := make([]ToolInfo, 0, len(registeredNames)) + for _, registeredName := range registeredNames { + tool, ok := m.registry.Get(registeredName) + if !ok { + continue + } + bridgeTool, ok := tool.(*BridgeTool) + if !ok { + continue + } + infos = append(infos, ToolInfo{ + Name: bridgeTool.OriginalName(), + Description: bridgeTool.Description(), + }) + } + return infos +} + // updateMCPGroup rebuilds the "mcp" group with all MCP tool names across servers. // Must be called with m.mu NOT held (it acquires RLock). func (m *Manager) updateMCPGroup() { @@ -116,11 +167,33 @@ type ToolInfo struct { // DiscoverTools connects temporarily to an MCP server, lists its tools, and disconnects. // Used for on-demand discovery when no persistent Manager connection exists (DB-backed servers). func DiscoverTools(ctx context.Context, transportType, command string, args []string, env map[string]string, url string, headers map[string]string) ([]ToolInfo, error) { + mcpTools, err := discoverRawTools(ctx, transportType, command, args, env, url, headers, discoverToolsTimeout) + if err != nil { + return nil, err + } + + result := make([]ToolInfo, 0, len(mcpTools)) + for _, t := range mcpTools { + result = append(result, ToolInfo{Name: t.Name, Description: t.Description}) + } + return result, nil +} + +// discoverToolsTimeout bounds a full DiscoverTools/discoverRawTools round trip +// (connect + initialize + list tools) against a remote MCP server. +const discoverToolsTimeout = 15 * time.Second + +// discoverRawTools connects temporarily to an MCP server, lists its tools with +// their full mcpgo.Tool definitions (including input schema), and disconnects. +// Shared by DiscoverTools (name+description only, for the admin browse UI) and +// the ListToolsForAgent on-demand fallback (full schema, for prompt preview +// caching), so both callers see identical live tool data. +func discoverRawTools(ctx context.Context, transportType, command string, args []string, env map[string]string, url string, headers map[string]string, timeout time.Duration) ([]mcpgo.Tool, error) { if err := ValidateServerConfig(transportType, command, args, url); err != nil { return nil, fmt.Errorf("invalid MCP server config: %w", err) } - ctx, cancel := context.WithTimeout(ctx, 15*time.Second) + ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() client, err := createClient(transportType, command, args, env, url, headers) @@ -147,9 +220,5 @@ func DiscoverTools(ctx context.Context, transportType, command string, args []st return nil, fmt.Errorf("list tools: %w", err) } - result := make([]ToolInfo, 0, len(toolsResult.Tools)) - for _, t := range toolsResult.Tools { - result = append(result, ToolInfo{Name: t.Name, Description: t.Description}) - } - return result, nil + return toolsResult.Tools, nil } diff --git a/internal/mcp/server_tool_infos_test.go b/internal/mcp/server_tool_infos_test.go new file mode 100644 index 00000000..e5ea2c3c --- /dev/null +++ b/internal/mcp/server_tool_infos_test.go @@ -0,0 +1,125 @@ +package mcp + +import ( + "context" + "encoding/json" + "net/http/httptest" + "testing" + + "github.com/google/uuid" + mcpgo "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// TestSelfConnectedServer_CacheRetainsDescriptionAndSchema is a regression +// test for a reported bug where connecting to an already-connected MCP +// server as a client showed tools with an empty description and empty +// parameters in the system-prompt preview (`- function: {description: '', +// parameters: {type: object}}`), while newly-discovered tools came through +// with full schema. +// +// It builds a minimal generic MCP server directly with the underlying +// mark3labs/mcp-go library (no dependency on any goclaw-specific server such +// as the CRUD server), serves it over streamable-http via httptest +// (mirroring a goclaw gateway connecting to an MCP endpoint), connects to it +// with the exact client path used by the Manager (connectAndDiscover), and +// proves the resulting tool cache (buildCachedToolInfo) — and the full +// downstream pipeline through ListToolsForAgent with a bare-name tool_allow +// grant — retains the real description and JSON Schema. +func TestSelfConnectedServer_CacheRetainsDescriptionAndSchema(t *testing.T) { + srv := mcpserver.NewMCPServer("test-server", "1.0.0", mcpserver.WithToolCapabilities(false)) + + tool := mcpgo.NewTool("test_tool", + mcpgo.WithDescription("A simple test tool used to verify description/schema propagation."), + mcpgo.WithString("param", + mcpgo.Description("An example string parameter."), + mcpgo.Required(), + ), + ) + srv.AddTool(tool, func(_ context.Context, _ mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { + return mcpgo.NewToolResultText("ok"), nil + }) + + httpSrv := mcpserver.NewStreamableHTTPServer(srv) + ts := httptest.NewServer(httpSrv) + defer ts.Close() + + // "goclaw" is the same ClientInfo.Name used by connectAndDiscover in + // production (manager_connect.go). + ss, mcpTools, err := connectAndDiscover(context.Background(), "goclaw", "streamable-http", "", nil, nil, ts.URL, nil, 10) + if err != nil { + t.Fatalf("connectAndDiscover: %v", err) + } + defer func() { _ = ss.client.Close() }() + + cache := buildCachedToolInfo(mcpTools) + entry, ok := cache["test_tool"] + if !ok { + t.Fatalf("expected test_tool in cache, got %+v", cache) + } + if entry.Description == "" { + t.Fatal("expected non-empty description for test_tool, got empty (regression)") + } + if len(entry.Parameters) == 0 { + t.Fatal("expected non-empty parameters schema for test_tool, got empty (regression)") + } + + var schema map[string]any + if err := json.Unmarshal(entry.Parameters, &schema); err != nil { + t.Fatalf("unmarshal cached parameters: %v", err) + } + props, ok := schema["properties"].(map[string]any) + if !ok || len(props) == 0 { + t.Fatalf("expected non-empty properties in cached schema, got %+v", schema) + } + if _, ok := props["param"]; !ok { + t.Fatalf("expected 'param' property in cached schema, got %+v", props) + } + + // End-to-end: feed the cache into ListToolsForAgent exactly as it is + // persisted in production (settings.tool_cache), with an explicit + // tool_allow grant using the BARE tool name — the shape ListToolsForAgent + // expects (internal/mcp/manager.go) and the shape ServerToolInfos returns + // (internal/mcp/manager_tools.go). + settings, err := json.Marshal(map[string]any{"tool_cache": cache}) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + serverID := uuid.New() + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "goclaw", + Enabled: true, + Settings: settings, + }, + ToolAllow: []string{"test_tool"}, + }, + }, + } + mgr := NewManager(tools.NewRegistry(), WithStore(mockStore)) + previews, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + var found *MCPToolPreviewInfo + for i := range previews { + if previews[i].RegisteredName == "mcp_goclaw__test_tool" { + found = &previews[i] + } + } + if found == nil { + t.Fatalf("expected mcp_goclaw__test_tool in preview, got %+v", previews) + } + if found.Description == "" { + t.Fatal("preview: expected non-empty description (regression)") + } + if len(found.Parameters) == 0 { + t.Fatal("preview: expected non-empty parameters (regression)") + } +}