Files
Duy /zuey/andGitHub bc3bc25c98 fix(mcp): eliminate spurious grant-revoked errors (tool filter + session reset + system-user bypass) (#87)
* feat(mcp): filter tools at registration + detect FastMCP session reset with force-reconnect

Two foundational MCP reliability improvements:

(1) Tool allow/deny filtering at BridgeTool registration: Previously the runtime grant-check at execute time surfaced "grant revoked" errors when the LLM called a tool it wasn't allowed to call. Filter upfront in both the per-agent registration path (manager_connect.go) and per-user registration path (loop_mcp_user.go). Adds tool_filter.go with IsToolAllowed + tests. This eliminates the "registered then runtime-denied" loop.

(2) FastMCP session reset detection + force-reconnect: FastMCP/Python mcp servers reject tools/call as "invalid during session initialization" when the upstream session lifecycle resets but our pool still holds the old Mcp-Session-Id. Detector matches three known phrasings (FastMCP, mcp-go, mcp-go transport ErrSessionTerminated) in session_reset.go. On detection, BridgeTool.Execute requests a force-reconnect via atomic CAS dedup so N concurrent failing calls collapse to one reconnect. Health loops skip ping while pending so a server answering ping in "initializing" state cannot clobber connected=true before the fresh Initialize completes. Includes 30s timeout, structured slog telemetry, concurrent CAS dedup test.

Files: tool_filter.go + tool_filter_test.go (new), session_reset.go + session_reset_test.go (new), manager_connect.go (connectServer/connectViaPool signatures + registerBridgeTools/registerPoolBridgeTools filter logic + reconnPending skip), manager.go (connectAndFilter + connectServer call signature changes), loop_mcp_user.go (filter-at-register block + WithForceReconnect wiring), pool.go (reconnPending skip), bridge_tool.go (session reset detection + WithForceReconnect callback).

* fix(mcp): self-heal grant cache + bypass per-user grant for system/empty userID

Two production fixes for "MCP tool: grant revoked" recurring on song-nhi-v2.

(1) System-user bypass in ListAccessible: Registration uses LoadForAgent(ctx, agentID, "") while execute uses IsAllowed(ctx, agentID, "system", ...). The LEFT JOIN on mcp_user_grants could match a stale disabled row keyed user_id='system' and silently filter the server out only at execute. Skip the join entirely for synthetic owner identities (userID="" or "system") so registration and execute see the same set. Applied to both PostgreSQL (mcp_servers_access.go) and SQLite (mcp_servers_access.go).

(2) Grant-checker no-cache on empty allowByServer: grant_checker.loadEntry now skips the cache write when allowByServer is empty. Without this, a single transient empty result pinned permanent denial until a bus invalidate fired. Re-queries until the empty condition clears, then caches normally. Includes TestStoreGrantChecker_EmptyEntryNotCached.
2026-05-28 14:04:04 +07:00

152 lines
4.2 KiB
Go

package mcp
import (
"context"
"fmt"
"log/slog"
"time"
mcpgo "github.com/mark3labs/mcp-go/mcp"
"github.com/nextlevelbuilder/goclaw/internal/store"
)
// UserCredServers returns servers requiring per-user credentials.
// These are stored during LoadForAgent("") and used by the agent loop
// for per-request tool resolution via pool.AcquireUser().
func (m *Manager) UserCredServers() []store.MCPAccessInfo {
return m.userCredServers
}
// ToolNames returns all registered MCP tool names.
func (m *Manager) ToolNames() []string {
m.mu.RLock()
defer m.mu.RUnlock()
var names []string
for name, ss := range m.servers {
if _, isPool := m.poolServers[name]; isPool {
names = append(names, m.poolToolNames[name]...)
} else {
names = append(names, ss.toolNames...)
}
}
return names
}
// ServerToolNames returns tool names for a specific server.
func (m *Manager) ServerToolNames(serverName string) []string {
m.mu.RLock()
defer m.mu.RUnlock()
if _, isPool := m.poolServers[serverName]; isPool {
return append([]string(nil), m.poolToolNames[serverName]...)
}
if ss, ok := m.servers[serverName]; ok {
return append([]string(nil), ss.toolNames...)
}
return nil
}
// 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() {
allNames := m.ToolNames()
if len(allNames) > 0 {
m.registry.RegisterToolGroup("mcp", allNames)
} else {
m.registry.UnregisterToolGroup("mcp")
}
}
// unregisterAllTools removes all MCP tools from the registry.
func (m *Manager) unregisterAllTools() {
m.mu.Lock()
defer m.mu.Unlock()
for name := range m.servers {
if _, isPool := m.poolServers[name]; isPool {
// Pool-backed: unregister per-agent tools, release shared connection
for _, toolName := range m.poolToolNames[name] {
m.registry.Unregister(toolName)
}
if m.pool != nil {
if pkey, ok := m.poolKeys[name]; ok {
m.pool.Release(pkey)
}
}
} else {
// Standalone: close connection directly
ss := m.servers[name]
if ss.cancel != nil {
ss.cancel()
}
if ss.client != nil {
_ = ss.client.Close()
}
for _, toolName := range ss.toolNames {
m.registry.Unregister(toolName)
}
}
m.registry.UnregisterToolGroup("mcp:" + name)
slog.Debug("mcp.server.unregistered", "server", name)
}
// Clean up search mode state: unregister activated tools and clear deferred
if m.searchMode {
for name := range m.activatedTools {
m.registry.Unregister(name)
}
m.deferredTools = nil
m.activatedTools = nil
m.searchMode = false
}
m.servers = make(map[string]*serverState)
m.poolServers = nil
m.poolToolNames = nil
m.registry.UnregisterToolGroup("mcp")
}
// ToolInfo holds a tool's name and description for API responses.
type ToolInfo struct {
Name string `json:"name"`
Description string `json:"description,omitempty"`
}
// 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) {
ctx, cancel := context.WithTimeout(ctx, 15*time.Second)
defer cancel()
client, err := createClient(transportType, command, args, env, url, headers)
if err != nil {
return nil, fmt.Errorf("create client: %w", err)
}
defer client.Close()
if transportType != "stdio" {
if err := client.Start(ctx); err != nil {
return nil, fmt.Errorf("start transport: %w", err)
}
}
initReq := mcpgo.InitializeRequest{}
initReq.Params.ProtocolVersion = mcpgo.LATEST_PROTOCOL_VERSION
initReq.Params.ClientInfo = mcpgo.Implementation{Name: "goclaw-discovery", Version: "1.0.0"}
if _, err := client.Initialize(ctx, initReq); err != nil {
return nil, fmt.Errorf("initialize: %w", err)
}
toolsResult, err := client.ListTools(ctx, mcpgo.ListToolsRequest{})
if err != nil {
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
}