From 91c168db2042998fa90ec993febf0cc2d0b3f75c Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 15 Feb 2026 17:26:36 +0800 Subject: [PATCH 01/43] feat(mcp): add Model Context Protocol integration Implement comprehensive MCP support with stdio/HTTP/SSE transports, environment variable configuration (env and envFile), custom headers, tool registration, and automatic resource cleanup. Includes full test coverage and VSCode-compatible configuration. - Added pkg/mcp/manager.go for server lifecycle management - Added pkg/tools/mcp_tool.go for tool wrapping - Integrated into agent loop with cleanup - Support for envFile loading (.env format) - Headers injection for HTTP/SSE authentication - Example configs for filesystem, github, brave-search, postgres --- config/config.example.json | 62 ++++- go.mod | 6 +- go.sum | 10 + pkg/agent/loop.go | 46 +++- pkg/config/config.go | 61 +++++ pkg/mcp/manager.go | 432 +++++++++++++++++++++++++++++++++++ pkg/mcp/manager_test.go | 182 +++++++++++++++ pkg/tools/mcp_tool.go | 119 ++++++++++ pkg/tools/mcp_tool_test.go | 456 +++++++++++++++++++++++++++++++++++++ 9 files changed, 1366 insertions(+), 8 deletions(-) create mode 100644 pkg/mcp/manager.go create mode 100644 pkg/mcp/manager_test.go create mode 100644 pkg/tools/mcp_tool.go create mode 100644 pkg/tools/mcp_tool_test.go diff --git a/config/config.example.json b/config/config.example.json index aa75c8338..75478af4d 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -14,7 +14,9 @@ "enabled": false, "token": "YOUR_TELEGRAM_BOT_TOKEN", "proxy": "", - "allow_from": ["YOUR_USER_ID"] + "allow_from": [ + "YOUR_USER_ID" + ] }, "discord": { "enabled": false, @@ -115,6 +117,62 @@ "api_key": "YOUR_BRAVE_API_KEY", "max_results": 5 } + }, + "mcp": { + "enabled": false, + "servers": { + "filesystem": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-filesystem", + "/tmp" + ], + "env": {} + }, + "github": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-github" + ], + "env": { + "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_TOKEN" + }, + "envFile": ".env" + }, + "brave-search": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-brave-search" + ], + "env": { + "BRAVE_API_KEY": "YOUR_BRAVE_API_KEY" + } + }, + "postgres": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-postgres", + "postgresql://user:password@localhost/dbname" + ] + }, + "remote-http-example": { + "enabled": false, + "url": "https://mcp-server.example.com/stream", + "type": "sse", + "headers": { + "Authorization": "Bearer YOUR_TOKEN", + "X-Custom-Header": "custom-value" + } + } + } } }, "heartbeat": { @@ -129,4 +187,4 @@ "host": "0.0.0.0", "port": 18790 } -} +} \ No newline at end of file diff --git a/go.mod b/go.mod index 98aecd6ab..528bc40b9 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 github.com/larksuite/oapi-sdk-go/v3 v3.5.3 + github.com/modelcontextprotocol/go-sdk v1.3.0 github.com/mymmrac/telego v1.6.0 github.com/open-dingtalk/dingtalk-stream-sdk-go v0.9.1 github.com/openai/openai-go/v3 v3.22.0 @@ -19,7 +20,7 @@ require ( golang.org/x/oauth2 v0.35.0 ) - +require github.com/yosida95/uritemplate/v3 v3.0.2 // indirect require ( github.com/andybalholm/brotli v1.2.0 // indirect @@ -28,9 +29,9 @@ require ( github.com/bytedance/sonic/loader v0.5.0 // indirect github.com/cloudwego/base64x v0.1.6 // indirect github.com/github/copilot-sdk/go v0.1.23 - github.com/google/jsonschema-go v0.4.2 // indirect github.com/go-resty/resty/v2 v2.17.1 // indirect github.com/gogo/protobuf v1.3.2 // indirect + github.com/google/jsonschema-go v0.4.2 // indirect github.com/grbit/go-json v0.11.0 // indirect github.com/klauspost/compress v1.18.4 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect @@ -47,5 +48,4 @@ require ( golang.org/x/net v0.50.0 // indirect golang.org/x/sync v0.19.0 // indirect golang.org/x/sys v0.41.0 // indirect - ) diff --git a/go.sum b/go.sum index 6a565b93e..5469dd7dd 100644 --- a/go.sum +++ b/go.sum @@ -43,6 +43,8 @@ github.com/go-test/deep v1.1.1 h1:0r/53hagsehfO4bzD2Pgr/+RgHqhmf+k1Bpse2cTu1U= github.com/go-test/deep v1.1.1/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= +github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8= +github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.4.0-rc.1/go.mod h1:ceaxUfeHdC40wWswd/P6IGgMaK3YpKi5j83Wpe3EHw8= github.com/golang/protobuf v1.4.0-rc.1.0.20200221234624-67d41d38c208/go.mod h1:xKAWHe0F5eneWXFV3EuXVDTCmh+JuBKY0li0aMyXATA= @@ -58,6 +60,8 @@ github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/jsonschema-go v0.4.2 h1:tmrUohrwoLZZS/P3x7ex0WAVknEkBZM46iALbcqoRA8= github.com/google/jsonschema-go v0.4.2/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= @@ -84,6 +88,8 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/larksuite/oapi-sdk-go/v3 v3.5.3 h1:xvf8Dv29kBXC5/DNDCLhHkAFW8l/0LlQJimO5Zn+JUk= github.com/larksuite/oapi-sdk-go/v3 v3.5.3/go.mod h1:ZEplY+kwuIrj/nqw5uSCINNATcH3KdxSN7y+UxYY5fI= +github.com/modelcontextprotocol/go-sdk v1.3.0 h1:gMfZkv3DzQF5q/DcQePo5rahEY+sguyPfXDfNBcT0Zs= +github.com/modelcontextprotocol/go-sdk v1.3.0/go.mod h1:AnQ//Qc6+4nIyyrB4cxBU7UW9VibK4iOZBeyP/rF1IE= github.com/mymmrac/telego v1.6.0 h1:Zc8rgyHozvd/7ZgyrigyHdAF9koHYMfilYfyB6wlFC0= github.com/mymmrac/telego v1.6.0/go.mod h1:xt6ZWA8zi8KmuzryE1ImEdl9JSwjHNpM4yhC7D8hU4Y= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= @@ -141,6 +147,8 @@ github.com/valyala/fastjson v1.6.7 h1:ZE4tRy0CIkh+qDc5McjatheGX2czdn8slQjomexVpB github.com/valyala/fastjson v1.6.7/go.mod h1:CLCAqky6SMuOcxStkYQvblddUtoRxhYMGLrsQns1aXY= github.com/xyproto/randomstring v1.0.5 h1:YtlWPoRdgMu3NZtP45drfy1GKoojuR7hmRcnhZqKjWU= github.com/xyproto/randomstring v1.0.5/go.mod h1:rgmS5DeNXLivK7YprL0pY+lTuhNQW3iGxZ18UQApw/E= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= @@ -228,6 +236,8 @@ golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e/go.mod h1:emZCQorbCU4vsT4f golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= +golang.org/x/tools v0.34.0 h1:qIpSLOxeCYGg9TrcJokLBG4KFA6d795g0xkBkiESGlo= +golang.org/x/tools v0.34.0/go.mod h1:pAP9OwEaY1CAW3HOmg3hLZC5Z0CCmzjAF2UQMSqNARg= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index f3dd94090..742ea5496 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -22,6 +22,7 @@ import ( "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/constants" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/mcp" "github.com/sipeed/picoclaw/pkg/providers" "github.com/sipeed/picoclaw/pkg/session" "github.com/sipeed/picoclaw/pkg/state" @@ -40,6 +41,7 @@ type AgentLoop struct { state *state.Manager contextBuilder *ContextBuilder tools *tools.ToolRegistry + mcpManager *mcp.Manager // MCP server manager for resource cleanup running atomic.Bool summarizing sync.Map // Tracks which sessions are currently being summarized } @@ -58,7 +60,7 @@ type processOptions struct { // createToolRegistry creates a tool registry with common tools. // This is shared between main agent and subagents. -func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msgBus *bus.MessageBus) *tools.ToolRegistry { +func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msgBus *bus.MessageBus, mcpManager *mcp.Manager) *tools.ToolRegistry { registry := tools.NewToolRegistry() // File system tools @@ -99,6 +101,23 @@ func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msg }) registry.Register(messageTool) + // Register MCP tools from all connected servers + if mcpManager != nil { + servers := mcpManager.GetServers() + for serverName, conn := range servers { + for _, tool := range conn.Tools { + mcpTool := tools.NewMCPTool(mcpManager, serverName, tool) + registry.Register(mcpTool) + logger.DebugCF("agent", "Registered MCP tool", + map[string]interface{}{ + "server": serverName, + "tool": tool.Name, + "name": mcpTool.Name(), + }) + } + } + } + return registry } @@ -108,12 +127,22 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers restrict := cfg.Agents.Defaults.RestrictToWorkspace + // Initialize MCP Manager and load servers + mcpManager := mcp.NewManager() + ctx := context.Background() + if err := mcpManager.LoadFromConfig(ctx, cfg); err != nil { + logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", + map[string]interface{}{ + "error": err.Error(), + }) + } + // Create tool registry for main agent - toolsRegistry := createToolRegistry(workspace, restrict, cfg, msgBus) + toolsRegistry := createToolRegistry(workspace, restrict, cfg, msgBus, mcpManager) // Create subagent manager with its own tool registry subagentManager := tools.NewSubagentManager(provider, cfg.Agents.Defaults.Model, workspace, msgBus) - subagentTools := createToolRegistry(workspace, restrict, cfg, msgBus) + subagentTools := createToolRegistry(workspace, restrict, cfg, msgBus, mcpManager) // Subagent doesn't need spawn/subagent tools to avoid recursion subagentManager.SetTools(subagentTools) @@ -145,6 +174,7 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers state: stateManager, contextBuilder: contextBuilder, tools: toolsRegistry, + mcpManager: mcpManager, summarizing: sync.Map{}, } } @@ -193,6 +223,16 @@ func (al *AgentLoop) Run(ctx context.Context) error { func (al *AgentLoop) Stop() { al.running.Store(false) + + // Clean up MCP connections + if al.mcpManager != nil { + if err := al.mcpManager.Close(); err != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]interface{}{ + "error": err.Error(), + }) + } + } } func (al *AgentLoop) RegisterTool(tool tools.Tool) { diff --git a/pkg/config/config.go b/pkg/config/config.go index d76ec8095..237eade65 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -212,6 +212,35 @@ type WebToolsConfig struct { type ToolsConfig struct { Web WebToolsConfig `json:"web"` + MCP MCPConfig `json:"mcp"` +} + +// MCPServerConfig defines configuration for a single MCP server +type MCPServerConfig struct { + // Enabled indicates whether this MCP server is active + Enabled bool `json:"enabled"` + // Command is the executable to run (e.g., "npx", "python", "/path/to/server") + Command string `json:"command"` + // Args are the arguments to pass to the command + Args []string `json:"args,omitempty"` + // Env are environment variables to set for the server process (stdio only) + Env map[string]string `json:"env,omitempty"` + // EnvFile is the path to a file containing environment variables (stdio only) + EnvFile string `json:"envFile,omitempty"` + // Type is "stdio", "sse", or "http" (default: stdio if command is set, sse if url is set) + Type string `json:"type,omitempty"` + // URL is used for SSE/HTTP transport + URL string `json:"url,omitempty"` + // Headers are HTTP headers to send with requests (sse/http only) + Headers map[string]string `json:"headers,omitempty"` +} + +// MCPConfig defines configuration for all MCP servers +type MCPConfig struct { + // Enabled globally enables/disables MCP integration + Enabled bool `json:"enabled" env:"PICOCLAW_TOOLS_MCP_ENABLED"` + // Servers is a map of server name to server configuration + Servers map[string]MCPServerConfig `json:"servers,omitempty"` } func DefaultConfig() *Config { @@ -321,6 +350,38 @@ func DefaultConfig() *Config { MaxResults: 5, }, }, + MCP: MCPConfig{ + Enabled: false, + Servers: map[string]MCPServerConfig{ + "filesystem": { + Enabled: false, + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-filesystem", "/tmp"}, + Env: map[string]string{}, + }, + "github": { + Enabled: false, + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-github"}, + Env: map[string]string{ + "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_TOKEN", + }, + }, + "brave-search": { + Enabled: false, + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-brave-search"}, + Env: map[string]string{ + "BRAVE_API_KEY": "YOUR_BRAVE_API_KEY", + }, + }, + "postgres": { + Enabled: false, + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-postgres", "postgresql://user:password@localhost/dbname"}, + }, + }, + }, }, Heartbeat: HeartbeatConfig{ Enabled: true, diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go new file mode 100644 index 000000000..d6ca28f76 --- /dev/null +++ b/pkg/mcp/manager.go @@ -0,0 +1,432 @@ +package mcp + +import ( + "bufio" + "context" + "fmt" + "net/http" + "os" + "os/exec" + "strings" + "sync" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/logger" +) + +// headerTransport is an http.RoundTripper that adds custom headers to requests +type headerTransport struct { + base http.RoundTripper + headers map[string]string +} + +func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Clone the request to avoid modifying the original + req = req.Clone(req.Context()) + + // Add custom headers + for key, value := range t.headers { + req.Header.Set(key, value) + } + + // Use the base transport + base := t.base + if base == nil { + base = http.DefaultTransport + } + return base.RoundTrip(req) +} + +// loadEnvFile loads environment variables from a file in .env format +// Each line should be in the format: KEY=value +// Lines starting with # are comments +// Empty lines are ignored +func loadEnvFile(path string) (map[string]string, error) { + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("failed to open env file: %w", err) + } + defer file.Close() + + envVars := make(map[string]string) + scanner := bufio.NewScanner(file) + lineNum := 0 + + for scanner.Scan() { + lineNum++ + line := strings.TrimSpace(scanner.Text()) + + // Skip empty lines and comments + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + // Parse KEY=value + parts := strings.SplitN(line, "=", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("invalid format at line %d: %s", lineNum, line) + } + + key := strings.TrimSpace(parts[0]) + value := strings.TrimSpace(parts[1]) + + // Remove surrounding quotes if present + if len(value) >= 2 { + if (value[0] == '"' && value[len(value)-1] == '"') || + (value[0] == '\'' && value[len(value)-1] == '\'') { + value = value[1 : len(value)-1] + } + } + + envVars[key] = value + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading env file: %w", err) + } + + return envVars, nil +} + +// ServerConnection represents a connection to an MCP server +type ServerConnection struct { + Name string + Client *mcp.Client + Session *mcp.ClientSession + Tools []*mcp.Tool +} + +// Manager manages multiple MCP server connections +type Manager struct { + servers map[string]*ServerConnection + mu sync.RWMutex +} + +// NewManager creates a new MCP manager +func NewManager() *Manager { + return &Manager{ + servers: make(map[string]*ServerConnection), + } +} + +// LoadFromConfig loads MCP servers from configuration +func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error { + if !cfg.Tools.MCP.Enabled { + logger.InfoCF("mcp", "MCP integration is disabled", nil) + return nil + } + + if len(cfg.Tools.MCP.Servers) == 0 { + logger.InfoCF("mcp", "No MCP servers configured", nil) + return nil + } + + logger.InfoCF("mcp", "Initializing MCP servers", + map[string]interface{}{ + "count": len(cfg.Tools.MCP.Servers), + }) + + var wg sync.WaitGroup + errs := make(chan error, len(cfg.Tools.MCP.Servers)) + + for name, serverCfg := range cfg.Tools.MCP.Servers { + if !serverCfg.Enabled { + logger.DebugCF("mcp", "Skipping disabled server", + map[string]interface{}{ + "server": name, + }) + continue + } + + wg.Add(1) + go func(name string, serverCfg config.MCPServerConfig) { + defer wg.Done() + + if err := m.ConnectServer(ctx, name, serverCfg); err != nil { + logger.ErrorCF("mcp", "Failed to connect to MCP server", + map[string]interface{}{ + "server": name, + "error": err.Error(), + }) + errs <- fmt.Errorf("failed to connect to server %s: %w", name, err) + } + }(name, serverCfg) + } + + wg.Wait() + close(errs) + + // Collect errors + var allErrors []error + for err := range errs { + allErrors = append(allErrors, err) + } + + if len(allErrors) > 0 { + logger.WarnCF("mcp", "Some MCP servers failed to connect", + map[string]interface{}{ + "failed": len(allErrors), + "total": len(cfg.Tools.MCP.Servers), + }) + // Don't fail completely if some servers fail to connect + } + + connectedCount := len(m.GetServers()) + logger.InfoCF("mcp", "MCP server initialization complete", + map[string]interface{}{ + "connected": connectedCount, + "total": len(cfg.Tools.MCP.Servers), + }) + + return nil +} + +// ConnectServer connects to a single MCP server +func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCPServerConfig) error { + logger.InfoCF("mcp", "Connecting to MCP server", + map[string]interface{}{ + "server": name, + "command": cfg.Command, + "args": cfg.Args, + }) + + // Create client + client := mcp.NewClient(&mcp.Implementation{ + Name: "picoclaw", + Version: "1.0.0", + }, nil) + + // Create transport based on configuration + // Auto-detect transport type if not explicitly specified + var transport mcp.Transport + transportType := cfg.Type + + // Auto-detect: if URL is provided, use SSE; if command is provided, use stdio + if transportType == "" { + if cfg.URL != "" { + transportType = "sse" + } else if cfg.Command != "" { + transportType = "stdio" + } else { + return fmt.Errorf("either URL or command must be provided") + } + } + + switch transportType { + case "sse", "http": + if cfg.URL == "" { + return fmt.Errorf("URL is required for SSE/HTTP transport") + } + logger.DebugCF("mcp", "Using SSE/HTTP transport", + map[string]interface{}{ + "server": name, + "url": cfg.URL, + }) + + sseTransport := &mcp.StreamableClientTransport{ + Endpoint: cfg.URL, + } + + // Add custom headers if provided + if len(cfg.Headers) > 0 { + // Create a custom HTTP client with header-injecting transport + sseTransport.HTTPClient = &http.Client{ + Transport: &headerTransport{ + base: http.DefaultTransport, + headers: cfg.Headers, + }, + } + logger.DebugCF("mcp", "Added custom HTTP headers", + map[string]interface{}{ + "server": name, + "header_count": len(cfg.Headers), + }) + } + + transport = sseTransport + case "stdio": + if cfg.Command == "" { + return fmt.Errorf("command is required for stdio transport") + } + logger.DebugCF("mcp", "Using stdio transport", + map[string]interface{}{ + "server": name, + "command": cfg.Command, + }) + // Create command with context + cmd := exec.CommandContext(ctx, cfg.Command, cfg.Args...) + + // Set environment variables + env := cmd.Environ() + + // Load environment variables from file if specified + if cfg.EnvFile != "" { + envVars, err := loadEnvFile(cfg.EnvFile) + if err != nil { + return fmt.Errorf("failed to load env file %s: %w", cfg.EnvFile, err) + } + for k, v := range envVars { + env = append(env, fmt.Sprintf("%s=%s", k, v)) + } + logger.DebugCF("mcp", "Loaded environment variables from file", + map[string]interface{}{ + "server": name, + "envFile": cfg.EnvFile, + "var_count": len(envVars), + }) + } + + // Environment variables from config override those from file + if len(cfg.Env) > 0 { + for k, v := range cfg.Env { + env = append(env, fmt.Sprintf("%s=%s", k, v)) + } + } + + // Set environment if we added any variables + if len(env) > len(cmd.Environ()) { + cmd.Env = env + } + + transport = &mcp.CommandTransport{Command: cmd} + default: + return fmt.Errorf("unsupported transport type: %s (supported: stdio, sse, http)", transportType) + } + + // Connect to server + session, err := client.Connect(ctx, transport, nil) + if err != nil { + return fmt.Errorf("failed to connect: %w", err) + } + + // Get server info + initResult := session.InitializeResult() + logger.InfoCF("mcp", "Connected to MCP server", + map[string]interface{}{ + "server": name, + "serverName": initResult.ServerInfo.Name, + "serverVersion": initResult.ServerInfo.Version, + "protocol": initResult.ProtocolVersion, + }) + + // List available tools if supported + var tools []*mcp.Tool + if initResult.Capabilities.Tools != nil { + for tool, err := range session.Tools(ctx, nil) { + if err != nil { + logger.WarnCF("mcp", "Error listing tool", + map[string]interface{}{ + "server": name, + "error": err.Error(), + }) + continue + } + tools = append(tools, tool) + } + + logger.InfoCF("mcp", "Listed tools from MCP server", + map[string]interface{}{ + "server": name, + "toolCount": len(tools), + }) + } + + // Store connection + m.mu.Lock() + m.servers[name] = &ServerConnection{ + Name: name, + Client: client, + Session: session, + Tools: tools, + } + m.mu.Unlock() + + return nil +} + +// GetServers returns all connected servers +func (m *Manager) GetServers() map[string]*ServerConnection { + m.mu.RLock() + defer m.mu.RUnlock() + + result := make(map[string]*ServerConnection, len(m.servers)) + for k, v := range m.servers { + result[k] = v + } + return result +} + +// GetServer returns a specific server connection +func (m *Manager) GetServer(name string) (*ServerConnection, bool) { + m.mu.RLock() + defer m.mu.RUnlock() + + conn, ok := m.servers[name] + return conn, ok +} + +// CallTool calls a tool on a specific server +func (m *Manager) CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + conn, ok := m.GetServer(serverName) + if !ok { + return nil, fmt.Errorf("server %s not found", serverName) + } + + params := &mcp.CallToolParams{ + Name: toolName, + Arguments: arguments, + } + + result, err := conn.Session.CallTool(ctx, params) + if err != nil { + return nil, fmt.Errorf("failed to call tool: %w", err) + } + + return result, nil +} + +// Close closes all server connections +func (m *Manager) Close() error { + m.mu.Lock() + defer m.mu.Unlock() + + logger.InfoCF("mcp", "Closing all MCP server connections", + map[string]interface{}{ + "count": len(m.servers), + }) + + var errs []error + for name, conn := range m.servers { + if err := conn.Session.Close(); err != nil { + logger.ErrorCF("mcp", "Failed to close server connection", + map[string]interface{}{ + "server": name, + "error": err.Error(), + }) + errs = append(errs, err) + } + } + + m.servers = make(map[string]*ServerConnection) + + if len(errs) > 0 { + return fmt.Errorf("failed to close %d server(s)", len(errs)) + } + + return nil +} + +// GetAllTools returns all tools from all connected servers +func (m *Manager) GetAllTools() map[string][]*mcp.Tool { + m.mu.RLock() + defer m.mu.RUnlock() + + result := make(map[string][]*mcp.Tool) + for name, conn := range m.servers { + if len(conn.Tools) > 0 { + result[name] = conn.Tools + } + } + return result +} diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go new file mode 100644 index 000000000..e888d8764 --- /dev/null +++ b/pkg/mcp/manager_test.go @@ -0,0 +1,182 @@ +package mcp + +import ( + "os" + "path/filepath" + "testing" +) + +func TestLoadEnvFile(t *testing.T) { + tests := []struct { + name string + content string + expected map[string]string + expectErr bool + }{ + { + name: "basic env file", + content: `API_KEY=secret123 +DATABASE_URL=postgres://localhost/db +PORT=8080`, + expected: map[string]string{ + "API_KEY": "secret123", + "DATABASE_URL": "postgres://localhost/db", + "PORT": "8080", + }, + expectErr: false, + }, + { + name: "with comments and empty lines", + content: `# This is a comment +API_KEY=secret123 + +# Another comment +DATABASE_URL=postgres://localhost/db + +PORT=8080`, + expected: map[string]string{ + "API_KEY": "secret123", + "DATABASE_URL": "postgres://localhost/db", + "PORT": "8080", + }, + expectErr: false, + }, + { + name: "with quoted values", + content: `API_KEY="secret with spaces" +NAME='single quoted' +PLAIN=no-quotes`, + expected: map[string]string{ + "API_KEY": "secret with spaces", + "NAME": "single quoted", + "PLAIN": "no-quotes", + }, + expectErr: false, + }, + { + name: "with spaces around equals", + content: `API_KEY = secret123 +DATABASE_URL= postgres://localhost/db +PORT =8080`, + expected: map[string]string{ + "API_KEY": "secret123", + "DATABASE_URL": "postgres://localhost/db", + "PORT": "8080", + }, + expectErr: false, + }, + { + name: "invalid format - no equals", + content: `INVALID_LINE`, + expectErr: true, + }, + { + name: "empty file", + content: ``, + expected: map[string]string{}, + expectErr: false, + }, + { + name: "only comments", + content: `# Comment 1 +# Comment 2`, + expected: map[string]string{}, + expectErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + envFile := filepath.Join(tmpDir, ".env") + + if err := os.WriteFile(envFile, []byte(tt.content), 0644); err != nil { + t.Fatalf("Failed to create test file: %v", err) + } + + result, err := loadEnvFile(envFile) + + if tt.expectErr { + if err == nil { + t.Errorf("Expected error but got none") + } + return + } + + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } + + if len(result) != len(tt.expected) { + t.Errorf("Expected %d variables, got %d", len(tt.expected), len(result)) + } + + for key, expectedValue := range tt.expected { + if actualValue, ok := result[key]; !ok { + t.Errorf("Expected key %s not found", key) + } else if actualValue != expectedValue { + t.Errorf("For key %s: expected %q, got %q", key, expectedValue, actualValue) + } + } + }) + } +} + +func TestLoadEnvFileNotFound(t *testing.T) { + _, err := loadEnvFile("/nonexistent/file.env") + if err == nil { + t.Error("Expected error for nonexistent file") + } +} + +func TestEnvFilePriority(t *testing.T) { + // Create a temporary .env file + tmpDir := t.TempDir() + envFile := filepath.Join(tmpDir, ".env") + + envContent := `API_KEY=from_file +DATABASE_URL=from_file +SHARED_VAR=from_file` + + if err := os.WriteFile(envFile, []byte(envContent), 0644); err != nil { + t.Fatalf("Failed to create .env file: %v", err) + } + + // Load envFile + envVars, err := loadEnvFile(envFile) + if err != nil { + t.Fatalf("Failed to load env file: %v", err) + } + + // Verify envFile variables + if envVars["API_KEY"] != "from_file" { + t.Errorf("Expected API_KEY=from_file, got %s", envVars["API_KEY"]) + } + + // Simulate config.Env overriding envFile + configEnv := map[string]string{ + "SHARED_VAR": "from_config", + "NEW_VAR": "from_config", + } + + // Merge: envFile first, then config overrides + merged := make(map[string]string) + for k, v := range envVars { + merged[k] = v + } + for k, v := range configEnv { + merged[k] = v + } + + // Verify priority: config.Env should override envFile + if merged["SHARED_VAR"] != "from_config" { + t.Errorf("Expected SHARED_VAR=from_config (config should override file), got %s", merged["SHARED_VAR"]) + } + if merged["API_KEY"] != "from_file" { + t.Errorf("Expected API_KEY=from_file, got %s", merged["API_KEY"]) + } + if merged["NEW_VAR"] != "from_config" { + t.Errorf("Expected NEW_VAR=from_config, got %s", merged["NEW_VAR"]) + } +} diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go new file mode 100644 index 000000000..e08a526ca --- /dev/null +++ b/pkg/tools/mcp_tool.go @@ -0,0 +1,119 @@ +package tools + +import ( + "context" + "fmt" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + mcpPkg "github.com/sipeed/picoclaw/pkg/mcp" +) + +// MCPManager defines the interface for MCP manager operations +// This allows for easier testing with mock implementations +type MCPManager interface { + CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) +} + +// MCPTool wraps an MCP tool to implement the Tool interface +type MCPTool struct { + manager MCPManager + serverName string + tool *mcp.Tool +} + +// NewMCPTool creates a new MCP tool wrapper +func NewMCPTool(manager *mcpPkg.Manager, serverName string, tool *mcp.Tool) *MCPTool { + return &MCPTool{ + manager: manager, + serverName: serverName, + tool: tool, + } +} + +// Name returns the tool name, prefixed with the server name +func (t *MCPTool) Name() string { + // Prefix with server name to avoid conflicts + return fmt.Sprintf("mcp_%s_%s", t.serverName, t.tool.Name) +} + +// Description returns the tool description +func (t *MCPTool) Description() string { + desc := t.tool.Description + if desc == "" { + desc = fmt.Sprintf("MCP tool from %s server", t.serverName) + } + // Add server info to description + return fmt.Sprintf("[MCP:%s] %s", t.serverName, desc) +} + +// Parameters returns the tool parameters schema +func (t *MCPTool) Parameters() map[string]interface{} { + // The InputSchema is already a JSON Schema object + schema := t.tool.InputSchema + + // Convert to map[string]interface{} for compatibility + result := make(map[string]interface{}) + + // Use reflection to convert the schema + // The schema should already be in the correct format + if schema != nil { + // Attempt to convert directly + if schemaMap, ok := schema.(map[string]interface{}); ok { + return schemaMap + } + + // Otherwise, build it manually + result["type"] = "object" + result["properties"] = map[string]interface{}{} + result["required"] = []string{} + } else { + // Default schema when nil + result["type"] = "object" + result["properties"] = map[string]interface{}{} + result["required"] = []string{} + } + + return result +} + +// Execute executes the MCP tool +func (t *MCPTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult { + result, err := t.manager.CallTool(ctx, t.serverName, t.tool.Name, args) + if err != nil { + return ErrorResult(fmt.Sprintf("MCP tool execution failed: %v", err)).WithError(err) + } + + // Handle error result from server + if result.IsError { + errMsg := extractContentText(result.Content) + return ErrorResult(fmt.Sprintf("MCP tool returned error: %s", errMsg)). + WithError(fmt.Errorf("MCP tool error: %s", errMsg)) + } + + // Extract text content from result + output := extractContentText(result.Content) + + return &ToolResult{ + ForLLM: output, + IsError: false, + } +} + +// extractContentText extracts text from MCP content array +func extractContentText(content []mcp.Content) string { + var parts []string + for _, c := range content { + switch v := c.(type) { + case *mcp.TextContent: + parts = append(parts, v.Text) + case *mcp.ImageContent: + // For images, just indicate that an image was returned + parts = append(parts, fmt.Sprintf("[Image: %s]", v.MIMEType)) + default: + // For other content types, use string representation + parts = append(parts, fmt.Sprintf("[Content: %T]", v)) + } + } + return strings.Join(parts, "\n") +} diff --git a/pkg/tools/mcp_tool_test.go b/pkg/tools/mcp_tool_test.go new file mode 100644 index 000000000..3be5d95a0 --- /dev/null +++ b/pkg/tools/mcp_tool_test.go @@ -0,0 +1,456 @@ +package tools + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// MockMCPManager is a mock implementation of MCPManager interface for testing +type MockMCPManager struct { + callToolFunc func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) +} + +func (m *MockMCPManager) CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + if m.callToolFunc != nil { + return m.callToolFunc(ctx, serverName, toolName, arguments) + } + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "mock result"}, + }, + IsError: false, + }, nil +} + +// newMCPToolForTest creates an MCP tool for testing with mock manager +func newMCPToolForTest(manager MCPManager, serverName string, tool *mcp.Tool) *MCPTool { + return &MCPTool{ + manager: manager, + serverName: serverName, + tool: tool, + } +} + +// TestNewMCPTool verifies MCP tool creation +func TestNewMCPTool(t *testing.T) { + manager := &MockMCPManager{} + tool := &mcp.Tool{ + Name: "test_tool", + Description: "A test tool", + InputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "input": map[string]interface{}{ + "type": "string", + "description": "Test input", + }, + }, + }, + } + + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + if mcpTool == nil { + t.Fatal("NewMCPTool should not return nil") + } + // Verify tool properties we can access + if mcpTool.Name() != "mcp_test_server_test_tool" { + t.Errorf("Expected tool name with prefix, got '%s'", mcpTool.Name()) + } +} + +// TestMCPTool_Name verifies tool name with server prefix +func TestMCPTool_Name(t *testing.T) { + tests := []struct { + name string + serverName string + toolName string + expected string + }{ + { + name: "simple name", + serverName: "github", + toolName: "create_issue", + expected: "mcp_github_create_issue", + }, + { + name: "filesystem server", + serverName: "filesystem", + toolName: "read_file", + expected: "mcp_filesystem_read_file", + }, + { + name: "remote server", + serverName: "remote-api", + toolName: "fetch_data", + expected: "mcp_remote-api_fetch_data", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + manager := &MockMCPManager{} + tool := &mcp.Tool{Name: tt.toolName} + mcpTool := newMCPToolForTest(manager, tt.serverName, tool) + + result := mcpTool.Name() + if result != tt.expected { + t.Errorf("Expected name '%s', got '%s'", tt.expected, result) + } + }) + } +} + +// TestMCPTool_Description verifies tool description generation +func TestMCPTool_Description(t *testing.T) { + tests := []struct { + name string + serverName string + toolDescription string + expectContains []string + }{ + { + name: "with description", + serverName: "github", + toolDescription: "Create a GitHub issue", + expectContains: []string{"[MCP:github]", "Create a GitHub issue"}, + }, + { + name: "empty description", + serverName: "filesystem", + toolDescription: "", + expectContains: []string{"[MCP:filesystem]", "MCP tool from filesystem server"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + manager := &MockMCPManager{} + tool := &mcp.Tool{ + Name: "test_tool", + Description: tt.toolDescription, + } + mcpTool := newMCPToolForTest(manager, tt.serverName, tool) + + result := mcpTool.Description() + + for _, expected := range tt.expectContains { + if !strings.Contains(result, expected) { + t.Errorf("Description should contain '%s', got: %s", expected, result) + } + } + }) + } +} + +// TestMCPTool_Parameters verifies parameter schema conversion +func TestMCPTool_Parameters(t *testing.T) { + tests := []struct { + name string + inputSchema interface{} + expectType string + }{ + { + name: "map schema", + inputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "query": map[string]interface{}{ + "type": "string", + "description": "Search query", + }, + }, + "required": []string{"query"}, + }, + expectType: "object", + }, + { + name: "nil schema", + inputSchema: nil, + expectType: "object", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + manager := &MockMCPManager{} + tool := &mcp.Tool{ + Name: "test_tool", + InputSchema: tt.inputSchema, + } + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + params := mcpTool.Parameters() + + if params == nil { + t.Fatal("Parameters should not be nil") + } + + if params["type"] != tt.expectType { + t.Errorf("Expected type '%s', got '%v'", tt.expectType, params["type"]) + } + }) + } +} + +// TestMCPTool_Execute_Success tests successful tool execution +func TestMCPTool_Execute_Success(t *testing.T) { + manager := &MockMCPManager{ + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + // Verify correct parameters passed + if serverName != "github" { + t.Errorf("Expected serverName 'github', got '%s'", serverName) + } + if toolName != "search_repos" { + t.Errorf("Expected toolName 'search_repos', got '%s'", toolName) + } + + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "Found 3 repositories"}, + }, + IsError: false, + }, nil + }, + } + + tool := &mcp.Tool{ + Name: "search_repos", + Description: "Search GitHub repositories", + } + mcpTool := newMCPToolForTest(manager, "github", tool) + + ctx := context.Background() + args := map[string]interface{}{ + "query": "golang mcp", + } + + result := mcpTool.Execute(ctx, args) + + if result == nil { + t.Fatal("Result should not be nil") + } + if result.IsError { + t.Errorf("Expected no error, got error: %s", result.ForLLM) + } + if result.ForLLM != "Found 3 repositories" { + t.Errorf("Expected 'Found 3 repositories', got '%s'", result.ForLLM) + } +} + +// TestMCPTool_Execute_ManagerError tests execution when manager returns error +func TestMCPTool_Execute_ManagerError(t *testing.T) { + manager := &MockMCPManager{ + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + return nil, fmt.Errorf("connection failed") + }, + } + + tool := &mcp.Tool{Name: "test_tool"} + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + ctx := context.Background() + result := mcpTool.Execute(ctx, map[string]interface{}{}) + + if result == nil { + t.Fatal("Result should not be nil") + } + if !result.IsError { + t.Error("Expected IsError to be true") + } + if !strings.Contains(result.ForLLM, "MCP tool execution failed") { + t.Errorf("Error message should mention execution failure, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "connection failed") { + t.Errorf("Error message should include original error, got: %s", result.ForLLM) + } +} + +// TestMCPTool_Execute_ServerError tests execution when server returns error +func TestMCPTool_Execute_ServerError(t *testing.T) { + manager := &MockMCPManager{ + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "Invalid API key"}, + }, + IsError: true, + }, nil + }, + } + + tool := &mcp.Tool{Name: "test_tool"} + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + ctx := context.Background() + result := mcpTool.Execute(ctx, map[string]interface{}{}) + + if result == nil { + t.Fatal("Result should not be nil") + } + if !result.IsError { + t.Error("Expected IsError to be true") + } + if !strings.Contains(result.ForLLM, "MCP tool returned error") { + t.Errorf("Error message should mention server error, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "Invalid API key") { + t.Errorf("Error message should include server message, got: %s", result.ForLLM) + } +} + +// TestMCPTool_Execute_MultipleContent tests execution with multiple content items +func TestMCPTool_Execute_MultipleContent(t *testing.T) { + manager := &MockMCPManager{ + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "First line"}, + &mcp.TextContent{Text: "Second line"}, + &mcp.TextContent{Text: "Third line"}, + }, + IsError: false, + }, nil + }, + } + + tool := &mcp.Tool{Name: "multi_output"} + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + ctx := context.Background() + result := mcpTool.Execute(ctx, map[string]interface{}{}) + + if result.IsError { + t.Errorf("Expected no error, got: %s", result.ForLLM) + } + + expected := "First line\nSecond line\nThird line" + if result.ForLLM != expected { + t.Errorf("Expected '%s', got '%s'", expected, result.ForLLM) + } +} + +// TestExtractContentText_TextContent tests text content extraction +func TestExtractContentText_TextContent(t *testing.T) { + content := []mcp.Content{ + &mcp.TextContent{Text: "Hello World"}, + &mcp.TextContent{Text: "Second message"}, + } + + result := extractContentText(content) + expected := "Hello World\nSecond message" + + if result != expected { + t.Errorf("Expected '%s', got '%s'", expected, result) + } +} + +// TestExtractContentText_ImageContent tests image content extraction +func TestExtractContentText_ImageContent(t *testing.T) { + content := []mcp.Content{ + &mcp.ImageContent{ + Data: []byte("base64data"), + MIMEType: "image/png", + }, + } + + result := extractContentText(content) + + if !strings.Contains(result, "[Image:") { + t.Errorf("Expected image indicator, got: %s", result) + } + if !strings.Contains(result, "image/png") { + t.Errorf("Expected MIME type in output, got: %s", result) + } +} + +// TestExtractContentText_MixedContent tests mixed content types +func TestExtractContentText_MixedContent(t *testing.T) { + content := []mcp.Content{ + &mcp.TextContent{Text: "Description"}, + &mcp.ImageContent{ + Data: []byte("data"), + MIMEType: "image/jpeg", + }, + &mcp.TextContent{Text: "More text"}, + } + + result := extractContentText(content) + + if !strings.Contains(result, "Description") { + t.Errorf("Should contain text content, got: %s", result) + } + if !strings.Contains(result, "[Image:") { + t.Errorf("Should contain image indicator, got: %s", result) + } + if !strings.Contains(result, "More text") { + t.Errorf("Should contain second text, got: %s", result) + } +} + +// TestExtractContentText_EmptyContent tests empty content array +func TestExtractContentText_EmptyContent(t *testing.T) { + content := []mcp.Content{} + + result := extractContentText(content) + + if result != "" { + t.Errorf("Expected empty string for empty content, got: %s", result) + } +} + +// TestMCPTool_InterfaceCompliance verifies MCPTool implements Tool interface +func TestMCPTool_InterfaceCompliance(t *testing.T) { + manager := &MockMCPManager{} + tool := &mcp.Tool{Name: "test"} + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + // Verify it implements Tool interface + var _ Tool = mcpTool +} + +// TestMCPTool_Parameters_MapSchema tests schema that's already a map +func TestMCPTool_Parameters_MapSchema(t *testing.T) { + manager := &MockMCPManager{} + schema := map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "name": map[string]interface{}{ + "type": "string", + "description": "The name parameter", + }, + }, + "required": []string{"name"}, + } + + tool := &mcp.Tool{ + Name: "test_tool", + InputSchema: schema, + } + mcpTool := newMCPToolForTest(manager, "test_server", tool) + + params := mcpTool.Parameters() + + // Should return the schema as-is when it's already a map + if params["type"] != "object" { + t.Errorf("Expected type 'object', got '%v'", params["type"]) + } + + props, ok := params["properties"].(map[string]interface{}) + if !ok { + t.Error("Properties should be a map") + } + + nameParam, ok := props["name"].(map[string]interface{}) + if !ok { + t.Error("Name parameter should exist") + } + + if nameParam["type"] != "string" { + t.Errorf("Name type should be 'string', got '%v'", nameParam["type"]) + } +} From ce3fc4bc67ee7362ff536516d43f5af826e2f61f Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 14:48:40 +0800 Subject: [PATCH 02/43] feat(docker): add full-featured Docker image with MCP tools support Add Dockerfile.full with Debian-based runtime including git, nodejs, npm, python3, and uv for MCP servers. Add docker-compose.full.yml with npm cache optimization. Add Makefile targets for docker-build-full, docker-run-full, and docker-test. Add test script for MCP tools validation. --- Dockerfile.full | 47 +++++++++++++++++++++++++++++++++++++ Makefile | 40 +++++++++++++++++++++++++++++++ docker-compose.full.yml | 44 ++++++++++++++++++++++++++++++++++ scripts/test-docker-mcp.sh | 48 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+) create mode 100644 Dockerfile.full create mode 100644 docker-compose.full.yml create mode 100644 scripts/test-docker-mcp.sh diff --git a/Dockerfile.full b/Dockerfile.full new file mode 100644 index 000000000..5f805752a --- /dev/null +++ b/Dockerfile.full @@ -0,0 +1,47 @@ +# ============================================================ +# Stage 1: Build the picoclaw binary +# ============================================================ +FROM golang:1.26.0-alpine AS builder + +RUN apk add --no-cache git make + +WORKDIR /src + +# Cache dependencies +COPY go.mod go.sum ./ +RUN go mod download + +# Copy source and build +COPY . . +RUN make build + +# ============================================================ +# Stage 2: Debian-based runtime with full MCP support +# ============================================================ +FROM debian:bookworm-slim + +# Install runtime dependencies +RUN apt-get update && apt-get install -y --no-install-recommends \ + ca-certificates \ + curl \ + git \ + nodejs \ + npm \ + python3 \ + python3-pip \ + python3-venv \ + && rm -rf /var/lib/apt/lists/* \ + && npm install -g npm@latest + +# Install uv +RUN curl -LsSf https://astral.sh/uv/install.sh | sh +ENV PATH="/root/.cargo/bin:$PATH" + +# Copy binary +COPY --from=builder /src/build/picoclaw /usr/local/bin/picoclaw + +# Create picoclaw home directory +RUN /usr/local/bin/picoclaw onboard + +ENTRYPOINT ["picoclaw"] +CMD ["gateway"] diff --git a/Makefile b/Makefile index 058fdb790..00258a074 100644 --- a/Makefile +++ b/Makefile @@ -140,6 +140,44 @@ deps: run: build @$(BUILD_DIR)/$(BINARY_NAME) $(ARGS) +## docker-build: Build Docker image (minimal Alpine-based) +docker-build: + @echo "Building minimal Docker image (Alpine-based)..." + docker-compose build + +## docker-build-full: Build Docker image with full MCP support (Debian-based) +docker-build-full: + @echo "Building full-featured Docker image (Debian-based)..." + docker-compose -f docker-compose.full.yml build + +## docker-test: Test MCP tools in Docker container +docker-test: + @echo "Testing MCP tools in Docker..." + @chmod +x scripts/test-docker-mcp.sh + @./scripts/test-docker-mcp.sh + +## docker-run: Run picoclaw gateway in Docker (Alpine-based) +docker-run: + docker-compose --profile gateway up + +## docker-run-full: Run picoclaw gateway in Docker (full-featured) +docker-run-full: + docker-compose -f docker-compose.full.yml --profile gateway up + +## docker-run-agent: Run picoclaw agent in Docker (interactive, Alpine-based) +docker-run-agent: + docker-compose run --rm picoclaw-agent + +## docker-run-agent-full: Run picoclaw agent in Docker (interactive, full-featured) +docker-run-agent-full: + docker-compose -f docker-compose.full.yml run --rm picoclaw-agent + +## docker-clean: Clean Docker images and volumes +docker-clean: + docker-compose down -v + docker-compose -f docker-compose.full.yml down -v + docker rmi picoclaw:latest picoclaw:full 2>/dev/null || true + ## help: Show this help message help: @echo "picoclaw Makefile" @@ -155,6 +193,8 @@ help: @echo " make install # Install to ~/.local/bin" @echo " make uninstall # Remove from /usr/local/bin" @echo " make install-skills # Install skills to workspace" + @echo " make docker-build # Build minimal Docker image" + @echo " make docker-test # Test MCP tools in Docker" @echo "" @echo "Environment Variables:" @echo " INSTALL_PREFIX # Installation prefix (default: ~/.local)" diff --git a/docker-compose.full.yml b/docker-compose.full.yml new file mode 100644 index 000000000..ff2694173 --- /dev/null +++ b/docker-compose.full.yml @@ -0,0 +1,44 @@ +services: + # ───────────────────────────────────────────── + # PicoClaw Agent (one-shot query) - Full MCP Support + # docker compose -f docker-compose.full.yml run --rm picoclaw-agent -m "Hello" + # ───────────────────────────────────────────── + picoclaw-agent: + build: + context: . + dockerfile: Dockerfile.full + container_name: picoclaw-agent-full + profiles: + - agent + volumes: + - ./config/config.json:/root/.picoclaw/config.json:ro + - picoclaw-workspace:/root/.picoclaw/workspace + - picoclaw-npm-cache:/root/.npm # npm cache for faster MCP server installs + entrypoint: ["picoclaw", "agent"] + stdin_open: true + tty: true + + # ───────────────────────────────────────────── + # PicoClaw Gateway (Long-running Bot) - Full MCP Support + # docker compose -f docker-compose.full.yml --profile gateway up + # ───────────────────────────────────────────── + picoclaw-gateway: + build: + context: . + dockerfile: Dockerfile.full + container_name: picoclaw-gateway-full + restart: unless-stopped + profiles: + - gateway + volumes: + # Configuration file + - ./config/config.json:/root/.picoclaw/config.json:ro + # Persistent workspace (sessions, memory, logs) + - picoclaw-workspace:/root/.picoclaw/workspace + # NPM cache for faster MCP server installs + - picoclaw-npm-cache:/root/.npm + command: ["gateway"] + +volumes: + picoclaw-workspace: + picoclaw-npm-cache: # Cache npm packages to speed up MCP server installations diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh new file mode 100644 index 000000000..6ace832d2 --- /dev/null +++ b/scripts/test-docker-mcp.sh @@ -0,0 +1,48 @@ +#!/bin/bash +# Test script for MCP tools in Docker (full-featured image) + +set -e + +COMPOSE_FILE="docker-compose.full.yml" + +echo "🧪 Testing MCP tools in Docker container (full-featured image)..." +echo "" + +# Build the image +echo "📦 Building Docker image..." +docker-compose -f "$COMPOSE_FILE" build + +# Test npx +echo "✅ Testing npx..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npx --version' + +# Test npm +echo "✅ Testing npm..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npm --version' + +# Test node +echo "✅ Testing Node.js..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'node --version' + +# Test git +echo "✅ Testing git..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'git --version' + +# Test python +echo "✅ Testing Python..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'python3 --version' + +# Test uv +echo "✅ Testing uv..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'uv --version' + +# Test MCP server installation (quick) +echo "✅ Testing MCP server install with npx..." +docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npx -y cowsay "MCP works!"' + +echo "" +echo "🎉 All MCP tools are working correctly!" +echo "" +echo "Next steps:" +echo " 1. Configure MCP servers in config/config.json" +echo " 2. Run: docker-compose -f $COMPOSE_FILE --profile gateway up" From 51ed54a41410479ba9114acce4ee4d7bb66504ed Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:40:07 +0800 Subject: [PATCH 03/43] refactor(docker): switch to node:24-bookworm-slim base image Replace debian:bookworm-slim with node:24-bookworm-slim to: - Use latest Node.js 24 LTS and npm - Fix npm version compatibility issues (npm@11 requires node >=20.17) - Simplify Dockerfile by removing nodejs/npm installation - Better optimization from official Node.js image Changes: - Dockerfile.full: use node:24-bookworm-slim base - Remove nodejs, npm installation steps - Remove npm upgrade step (included in base image) - Update Makefile descriptions to reflect Node.js 24 --- Dockerfile.full | 9 +++------ Makefile | 4 ++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Dockerfile.full b/Dockerfile.full index 5f805752a..46a2bae90 100644 --- a/Dockerfile.full +++ b/Dockerfile.full @@ -16,22 +16,19 @@ COPY . . RUN make build # ============================================================ -# Stage 2: Debian-based runtime with full MCP support +# Stage 2: Node.js-based runtime with full MCP support # ============================================================ -FROM debian:bookworm-slim +FROM node:24-bookworm-slim # Install runtime dependencies RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ curl \ git \ - nodejs \ - npm \ python3 \ python3-pip \ python3-venv \ - && rm -rf /var/lib/apt/lists/* \ - && npm install -g npm@latest + && rm -rf /var/lib/apt/lists/* # Install uv RUN curl -LsSf https://astral.sh/uv/install.sh | sh diff --git a/Makefile b/Makefile index 00258a074..6122e4594 100644 --- a/Makefile +++ b/Makefile @@ -145,9 +145,9 @@ docker-build: @echo "Building minimal Docker image (Alpine-based)..." docker-compose build -## docker-build-full: Build Docker image with full MCP support (Debian-based) +## docker-build-full: Build Docker image with full MCP support (Node.js 24) docker-build-full: - @echo "Building full-featured Docker image (Debian-based)..." + @echo "Building full-featured Docker image (Node.js 24)..." docker-compose -f docker-compose.full.yml build ## docker-test: Test MCP tools in Docker container From b9c2b3555a946f93c9c1941403b68a7afe04a29d Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:49:14 +0800 Subject: [PATCH 04/43] fix(docker): override entrypoint in test script to avoid interactive mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add --entrypoint sh flag to docker-compose run commands in test script to bypass picoclaw agent's interactive mode. This allows direct command execution for testing MCP tools. Changes: - Add --entrypoint sh to all docker-compose run commands - Use SERVICE variable for better maintainability - Simplify command syntax: sh -c 'cmd' → -c 'cmd' --- scripts/test-docker-mcp.sh | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index 6ace832d2..eb8901f45 100644 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -4,6 +4,7 @@ set -e COMPOSE_FILE="docker-compose.full.yml" +SERVICE="picoclaw-agent" echo "🧪 Testing MCP tools in Docker container (full-featured image)..." echo "" @@ -14,31 +15,31 @@ docker-compose -f "$COMPOSE_FILE" build # Test npx echo "✅ Testing npx..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npx --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx --version' # Test npm echo "✅ Testing npm..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npm --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npm --version' # Test node echo "✅ Testing Node.js..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'node --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'node --version' # Test git echo "✅ Testing git..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'git --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'git --version' # Test python echo "✅ Testing Python..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'python3 --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'python3 --version' # Test uv echo "✅ Testing uv..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'uv --version' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'uv --version' # Test MCP server installation (quick) echo "✅ Testing MCP server install with npx..." -docker-compose -f "$COMPOSE_FILE" run --rm picoclaw-agent sh -c 'npx -y cowsay "MCP works!"' +docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y cowsay "MCP works!"' echo "" echo "🎉 All MCP tools are working correctly!" From c05742330dd7193eddd00b84d97ea226e0fafe83 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:50:46 +0800 Subject: [PATCH 05/43] refactor(docker): migrate to docker compose v2 syntax Replace docker-compose (v1) with docker compose (v2) command syntax across all files. Docker Compose v2 is now the default in modern Docker installations and uses 'docker compose' instead of 'docker-compose'. Changes: - scripts/test-docker-mcp.sh: update all 8 docker-compose commands - Makefile: update all 8 docker-compose commands in docker-* targets - No changes to file names (docker-compose.full.yml remains as-is) Compatibility: Requires Docker with Compose v2 plugin (Docker Desktop or docker-compose-plugin package) --- Makefile | 16 ++++++++-------- scripts/test-docker-mcp.sh | 18 +++++++++--------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 6122e4594..bf7e99bcd 100644 --- a/Makefile +++ b/Makefile @@ -143,12 +143,12 @@ run: build ## docker-build: Build Docker image (minimal Alpine-based) docker-build: @echo "Building minimal Docker image (Alpine-based)..." - docker-compose build + docker compose build ## docker-build-full: Build Docker image with full MCP support (Node.js 24) docker-build-full: @echo "Building full-featured Docker image (Node.js 24)..." - docker-compose -f docker-compose.full.yml build + docker compose -f docker-compose.full.yml build ## docker-test: Test MCP tools in Docker container docker-test: @@ -158,24 +158,24 @@ docker-test: ## docker-run: Run picoclaw gateway in Docker (Alpine-based) docker-run: - docker-compose --profile gateway up + docker compose --profile gateway up ## docker-run-full: Run picoclaw gateway in Docker (full-featured) docker-run-full: - docker-compose -f docker-compose.full.yml --profile gateway up + docker compose -f docker-compose.full.yml --profile gateway up ## docker-run-agent: Run picoclaw agent in Docker (interactive, Alpine-based) docker-run-agent: - docker-compose run --rm picoclaw-agent + docker compose run --rm picoclaw-agent ## docker-run-agent-full: Run picoclaw agent in Docker (interactive, full-featured) docker-run-agent-full: - docker-compose -f docker-compose.full.yml run --rm picoclaw-agent + docker compose -f docker-compose.full.yml run --rm picoclaw-agent ## docker-clean: Clean Docker images and volumes docker-clean: - docker-compose down -v - docker-compose -f docker-compose.full.yml down -v + docker compose down -v + docker compose -f docker-compose.full.yml down -v docker rmi picoclaw:latest picoclaw:full 2>/dev/null || true ## help: Show this help message diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index eb8901f45..a0c9e232a 100644 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -11,39 +11,39 @@ echo "" # Build the image echo "📦 Building Docker image..." -docker-compose -f "$COMPOSE_FILE" build +docker compose -f "$COMPOSE_FILE" build # Test npx echo "✅ Testing npx..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx --version' # Test npm echo "✅ Testing npm..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npm --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npm --version' # Test node echo "✅ Testing Node.js..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'node --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'node --version' # Test git echo "✅ Testing git..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'git --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'git --version' # Test python echo "✅ Testing Python..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'python3 --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'python3 --version' # Test uv echo "✅ Testing uv..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'uv --version' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'uv --version' # Test MCP server installation (quick) echo "✅ Testing MCP server install with npx..." -docker-compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y cowsay "MCP works!"' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y cowsay "MCP works!"' echo "" echo "🎉 All MCP tools are working correctly!" echo "" echo "Next steps:" echo " 1. Configure MCP servers in config/config.json" -echo " 2. Run: docker-compose -f $COMPOSE_FILE --profile gateway up" +echo " 2. Run: docker compose -f $COMPOSE_FILE --profile gateway up" From 1c9c32022ed5ca15c12b552db7336499be332016 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:52:59 +0800 Subject: [PATCH 06/43] fix(docker): ensure uv is accessible in system PATH Symlink uv from /root/.cargo/bin to /usr/local/bin to make it accessible without relying on ENV PATH setting. Add version check to verify successful installation during build. Changes: - Symlink uv to /usr/local/bin/uv - Add 'uv --version' validation step - Remove ENV PATH setting (no longer needed) Fixes: uv: not found error in test script --- Dockerfile.full | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Dockerfile.full b/Dockerfile.full index 46a2bae90..5014e879e 100644 --- a/Dockerfile.full +++ b/Dockerfile.full @@ -30,9 +30,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ python3-venv \ && rm -rf /var/lib/apt/lists/* -# Install uv -RUN curl -LsSf https://astral.sh/uv/install.sh | sh -ENV PATH="/root/.cargo/bin:$PATH" +# Install uv and symlink to system path +RUN curl -LsSf https://astral.sh/uv/install.sh | sh && \ + ln -s /root/.cargo/bin/uv /usr/local/bin/uv && \ + uv --version # Copy binary COPY --from=builder /src/build/picoclaw /usr/local/bin/picoclaw From 1764181e6f40370c6fd8e37bfd6f5ecb48fdffb9 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:57:30 +0800 Subject: [PATCH 07/43] fix(docker): correct uv installation path Fix uv symlink path from /root/.cargo/bin to /root/.local/bin. The uv installer puts binaries in ~/.local/bin, not ~/.cargo/bin. Changes: - Update uv symlink source: /root/.local/bin/uv - Add uvx symlink as well (installed alongside uv) Fixes: /bin/sh: 1: uv: not found error during build --- Dockerfile.full | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Dockerfile.full b/Dockerfile.full index 5014e879e..aebcdafe6 100644 --- a/Dockerfile.full +++ b/Dockerfile.full @@ -32,8 +32,9 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ # Install uv and symlink to system path RUN curl -LsSf https://astral.sh/uv/install.sh | sh && \ - ln -s /root/.cargo/bin/uv /usr/local/bin/uv && \ - uv --version + ln -s /root/.local/bin/uv /usr/local/bin/uv && \ + ln -s /root/.local/bin/uvx /usr/local/bin/uvx && \ + uv --version # Copy binary COPY --from=builder /src/build/picoclaw /usr/local/bin/picoclaw From fcedba1c9df3aa47b704145af864e00f074ddc33 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 15:59:34 +0800 Subject: [PATCH 08/43] fix(docker): add profiles to build commands Add --profile gateway --profile agent flags to docker build commands to ensure services are built even when using profiles in compose files. Without profiles specified, docker compose build skips all services that have a profile defined, resulting in 'No services to build' warning. Changes: - docker-build: add --profile flags - docker-build-full: add --profile flags Fixes: WARN[0000] No services to build --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index bf7e99bcd..4f7e65894 100644 --- a/Makefile +++ b/Makefile @@ -143,12 +143,12 @@ run: build ## docker-build: Build Docker image (minimal Alpine-based) docker-build: @echo "Building minimal Docker image (Alpine-based)..." - docker compose build + docker compose build --profile gateway --profile agent ## docker-build-full: Build Docker image with full MCP support (Node.js 24) docker-build-full: @echo "Building full-featured Docker image (Node.js 24)..." - docker compose -f docker-compose.full.yml build + docker compose -f docker-compose.full.yml build --profile gateway --profile agent ## docker-test: Test MCP tools in Docker container docker-test: From e91e7169587674c4f8d928e2675d68ec5642275e Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 16:02:10 +0800 Subject: [PATCH 09/43] fix(docker): use service names instead of --profile flag for build Replace --profile flags with explicit service names in build commands. The 'docker compose build' command does not support --profile flag; profiles are only used for runtime operations like 'up' and 'run'. Changes: - docker-build: specify picoclaw-agent picoclaw-gateway - docker-build-full: specify picoclaw-agent picoclaw-gateway Fixes: unknown flag: --profile error --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4f7e65894..a97f17799 100644 --- a/Makefile +++ b/Makefile @@ -143,12 +143,12 @@ run: build ## docker-build: Build Docker image (minimal Alpine-based) docker-build: @echo "Building minimal Docker image (Alpine-based)..." - docker compose build --profile gateway --profile agent + docker compose build picoclaw-agent picoclaw-gateway ## docker-build-full: Build Docker image with full MCP support (Node.js 24) docker-build-full: @echo "Building full-featured Docker image (Node.js 24)..." - docker compose -f docker-compose.full.yml build --profile gateway --profile agent + docker compose -f docker-compose.full.yml build picoclaw-agent picoclaw-gateway ## docker-test: Test MCP tools in Docker container docker-test: From 87e0336d6271bfd97d056234a8a626ca25974405 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 16:38:21 +0800 Subject: [PATCH 10/43] chore(deps): format go.mod Add blank line for better formatting consistency --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index 833093f7c..4118e0767 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) + require github.com/yosida95/uritemplate/v3 v3.0.2 // indirect require ( From 24610693e4a9e6df3fbf615eab31cb7363c38e48 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 16:40:35 +0800 Subject: [PATCH 11/43] chore(docker): add execute permission to test script Make scripts/test-docker-mcp.sh executable --- scripts/test-docker-mcp.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 scripts/test-docker-mcp.sh diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh old mode 100644 new mode 100755 From 77d26e5ce32adebf31eb773d1dcb9ebabf57ae65 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:33:31 +0800 Subject: [PATCH 12/43] fix(mcp): return aggregated error when all servers fail to connect - Add errors.Join to return aggregated error when all enabled MCP servers fail - Track enabled server count separately from total configured servers - Return error only when all servers fail, not for partial failures - Improve logging with accurate server counts (enabled vs connected) - Maintains fault tolerance: partial failures don't stop initialization --- pkg/mcp/manager.go | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index d6ca28f76..be941ec23 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -3,6 +3,7 @@ package mcp import ( "bufio" "context" + "errors" "fmt" "net/http" "os" @@ -24,12 +25,12 @@ type headerTransport struct { func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { // Clone the request to avoid modifying the original req = req.Clone(req.Context()) - + // Add custom headers for key, value := range t.headers { req.Header.Set(key, value) } - + // Use the base transport base := t.base if base == nil { @@ -129,6 +130,7 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error var wg sync.WaitGroup errs := make(chan error, len(cfg.Tools.MCP.Servers)) + enabledCount := 0 for name, serverCfg := range cfg.Tools.MCP.Servers { if !serverCfg.Enabled { @@ -139,6 +141,7 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error continue } + enabledCount++ wg.Add(1) go func(name string, serverCfg config.MCPServerConfig) { defer wg.Done() @@ -163,20 +166,32 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error allErrors = append(allErrors, err) } + connectedCount := len(m.GetServers()) + + // If all enabled servers failed to connect, return aggregated error + if enabledCount > 0 && connectedCount == 0 { + logger.ErrorCF("mcp", "All MCP servers failed to connect", + map[string]interface{}{ + "failed": len(allErrors), + "total": enabledCount, + }) + return errors.Join(allErrors...) + } + if len(allErrors) > 0 { logger.WarnCF("mcp", "Some MCP servers failed to connect", map[string]interface{}{ - "failed": len(allErrors), - "total": len(cfg.Tools.MCP.Servers), + "failed": len(allErrors), + "connected": connectedCount, + "total": enabledCount, }) - // Don't fail completely if some servers fail to connect + // Don't fail completely if some servers successfully connected } - connectedCount := len(m.GetServers()) logger.InfoCF("mcp", "MCP server initialization complete", map[string]interface{}{ "connected": connectedCount, - "total": len(cfg.Tools.MCP.Servers), + "total": enabledCount, }) return nil @@ -223,11 +238,11 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP "server": name, "url": cfg.URL, }) - + sseTransport := &mcp.StreamableClientTransport{ Endpoint: cfg.URL, } - + // Add custom headers if provided if len(cfg.Headers) > 0 { // Create a custom HTTP client with header-injecting transport @@ -243,7 +258,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP "header_count": len(cfg.Headers), }) } - + transport = sseTransport case "stdio": if cfg.Command == "" { @@ -259,7 +274,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP // Set environment variables env := cmd.Environ() - + // Load environment variables from file if specified if cfg.EnvFile != "" { envVars, err := loadEnvFile(cfg.EnvFile) @@ -276,14 +291,14 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP "var_count": len(envVars), }) } - + // Environment variables from config override those from file if len(cfg.Env) > 0 { for k, v := range cfg.Env { env = append(env, fmt.Sprintf("%s=%s", k, v)) } } - + // Set environment if we added any variables if len(env) > len(cmd.Environ()) { cmd.Env = env From a4265b3f163ef18cd664cffcdd5e18333fddc530 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:38:27 +0800 Subject: [PATCH 13/43] fix(mcp): resolve relative envFile paths against workspace directory - Resolve relative envFile paths relative to workspace instead of CWD - Add filepath import for path operations - Pass workspace path to goroutines for path resolution - Improves portability in Docker environments where CWD may vary - Absolute envFile paths continue to work as before --- pkg/mcp/manager.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index be941ec23..8449486f5 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "os/exec" + "path/filepath" "strings" "sync" @@ -128,6 +129,9 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error "count": len(cfg.Tools.MCP.Servers), }) + // Get workspace path for resolving relative envFile paths + workspacePath := cfg.WorkspacePath() + var wg sync.WaitGroup errs := make(chan error, len(cfg.Tools.MCP.Servers)) enabledCount := 0 @@ -143,9 +147,14 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error enabledCount++ wg.Add(1) - go func(name string, serverCfg config.MCPServerConfig) { + go func(name string, serverCfg config.MCPServerConfig, workspace string) { defer wg.Done() + // Resolve relative envFile paths relative to workspace + if serverCfg.EnvFile != "" && !filepath.IsAbs(serverCfg.EnvFile) { + serverCfg.EnvFile = filepath.Join(workspace, serverCfg.EnvFile) + } + if err := m.ConnectServer(ctx, name, serverCfg); err != nil { logger.ErrorCF("mcp", "Failed to connect to MCP server", map[string]interface{}{ @@ -154,7 +163,7 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error }) errs <- fmt.Errorf("failed to connect to server %s: %w", name, err) } - }(name, serverCfg) + }(name, serverCfg, workspacePath) } wg.Wait() From a026d56c0f7ce097e645693018ebff7965c32891 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:40:14 +0800 Subject: [PATCH 14/43] chore(deps): consolidate indirect require for uritemplate - Move standalone indirect require line into existing require block - Maintain alphabetical ordering of dependencies - Keep module file stable and avoid churn --- go.mod | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 4118e0767..705767f89 100644 --- a/go.mod +++ b/go.mod @@ -27,8 +27,6 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect ) -require github.com/yosida95/uritemplate/v3 v3.0.2 // indirect - require ( github.com/andybalholm/brotli v1.2.0 // indirect github.com/bytedance/gopkg v0.1.3 // indirect @@ -50,6 +48,7 @@ require ( github.com/valyala/bytebufferpool v1.0.0 // indirect github.com/valyala/fasthttp v1.69.0 // indirect github.com/valyala/fastjson v1.6.7 // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect golang.org/x/arch v0.24.0 // indirect golang.org/x/crypto v0.48.0 // indirect golang.org/x/net v0.50.0 // indirect From 20f8bb200b84a2350bc38387cad53816de281ef0 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:43:05 +0800 Subject: [PATCH 15/43] refactor(tools): use MCPManager interface in NewMCPTool constructor - Change NewMCPTool to accept MCPManager interface instead of concrete *mcp.Manager - Remove unused mcpPkg import from mcp_tool.go - Remove newMCPToolForTest helper function as NewMCPTool now accepts interface - Update all tests to use NewMCPTool directly with MockMCPManager - Improves testability and follows dependency inversion principle --- pkg/tools/mcp_tool.go | 3 +-- pkg/tools/mcp_tool_test.go | 33 ++++++++++++--------------------- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index e08a526ca..adc06ec23 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/modelcontextprotocol/go-sdk/mcp" - mcpPkg "github.com/sipeed/picoclaw/pkg/mcp" ) // MCPManager defines the interface for MCP manager operations @@ -23,7 +22,7 @@ type MCPTool struct { } // NewMCPTool creates a new MCP tool wrapper -func NewMCPTool(manager *mcpPkg.Manager, serverName string, tool *mcp.Tool) *MCPTool { +func NewMCPTool(manager MCPManager, serverName string, tool *mcp.Tool) *MCPTool { return &MCPTool{ manager: manager, serverName: serverName, diff --git a/pkg/tools/mcp_tool_test.go b/pkg/tools/mcp_tool_test.go index 3be5d95a0..597bbb52b 100644 --- a/pkg/tools/mcp_tool_test.go +++ b/pkg/tools/mcp_tool_test.go @@ -26,15 +26,6 @@ func (m *MockMCPManager) CallTool(ctx context.Context, serverName, toolName stri }, nil } -// newMCPToolForTest creates an MCP tool for testing with mock manager -func newMCPToolForTest(manager MCPManager, serverName string, tool *mcp.Tool) *MCPTool { - return &MCPTool{ - manager: manager, - serverName: serverName, - tool: tool, - } -} - // TestNewMCPTool verifies MCP tool creation func TestNewMCPTool(t *testing.T) { manager := &MockMCPManager{} @@ -48,11 +39,11 @@ func TestNewMCPTool(t *testing.T) { "type": "string", "description": "Test input", }, + }, }, - }, - } + } - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) if mcpTool == nil { t.Fatal("NewMCPTool should not return nil") @@ -95,7 +86,7 @@ func TestMCPTool_Name(t *testing.T) { t.Run(tt.name, func(t *testing.T) { manager := &MockMCPManager{} tool := &mcp.Tool{Name: tt.toolName} - mcpTool := newMCPToolForTest(manager, tt.serverName, tool) + mcpTool := NewMCPTool(manager, tt.serverName, tool) result := mcpTool.Name() if result != tt.expected { @@ -134,7 +125,7 @@ func TestMCPTool_Description(t *testing.T) { Name: "test_tool", Description: tt.toolDescription, } - mcpTool := newMCPToolForTest(manager, tt.serverName, tool) + mcpTool := NewMCPTool(manager, tt.serverName, tool) result := mcpTool.Description() @@ -182,7 +173,7 @@ func TestMCPTool_Parameters(t *testing.T) { Name: "test_tool", InputSchema: tt.inputSchema, } - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) params := mcpTool.Parameters() @@ -222,7 +213,7 @@ func TestMCPTool_Execute_Success(t *testing.T) { Name: "search_repos", Description: "Search GitHub repositories", } - mcpTool := newMCPToolForTest(manager, "github", tool) + mcpTool := NewMCPTool(manager, "github", tool) ctx := context.Background() args := map[string]interface{}{ @@ -251,7 +242,7 @@ func TestMCPTool_Execute_ManagerError(t *testing.T) { } tool := &mcp.Tool{Name: "test_tool"} - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() result := mcpTool.Execute(ctx, map[string]interface{}{}) @@ -284,7 +275,7 @@ func TestMCPTool_Execute_ServerError(t *testing.T) { } tool := &mcp.Tool{Name: "test_tool"} - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() result := mcpTool.Execute(ctx, map[string]interface{}{}) @@ -319,7 +310,7 @@ func TestMCPTool_Execute_MultipleContent(t *testing.T) { } tool := &mcp.Tool{Name: "multi_output"} - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() result := mcpTool.Execute(ctx, map[string]interface{}{}) @@ -407,7 +398,7 @@ func TestExtractContentText_EmptyContent(t *testing.T) { func TestMCPTool_InterfaceCompliance(t *testing.T) { manager := &MockMCPManager{} tool := &mcp.Tool{Name: "test"} - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) // Verify it implements Tool interface var _ Tool = mcpTool @@ -431,7 +422,7 @@ func TestMCPTool_Parameters_MapSchema(t *testing.T) { Name: "test_tool", InputSchema: schema, } - mcpTool := newMCPToolForTest(manager, "test_server", tool) + mcpTool := NewMCPTool(manager, "test_server", tool) params := mcpTool.Parameters() From 02c1792015baf4ed0e0f23c7f78e4373fa316959 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:50:00 +0800 Subject: [PATCH 16/43] fix(tools): preserve MCP tool InputSchema via JSON marshal/unmarshal - Handle json.RawMessage and []byte types by direct unmarshal - Use JSON marshal/unmarshal for struct types to preserve schema - Add test case for json.RawMessage schema - Fixes issue where non-map schemas returned empty object This fixes GitHub Copilot feedback that Parameters() was dropping tool schema when InputSchema wasn't already map[string]interface{} --- pkg/tools/mcp_tool.go | 72 ++++++++++++++++++++++++++++---------- pkg/tools/mcp_tool_test.go | 55 +++++++++++++++++++++++++---- 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index adc06ec23..c29ab8525 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/json" "fmt" "strings" @@ -51,26 +52,61 @@ func (t *MCPTool) Parameters() map[string]interface{} { // The InputSchema is already a JSON Schema object schema := t.tool.InputSchema - // Convert to map[string]interface{} for compatibility - result := make(map[string]interface{}) - - // Use reflection to convert the schema - // The schema should already be in the correct format - if schema != nil { - // Attempt to convert directly - if schemaMap, ok := schema.(map[string]interface{}); ok { - return schemaMap + // Handle nil schema + if schema == nil { + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + "required": []string{}, } + } - // Otherwise, build it manually - result["type"] = "object" - result["properties"] = map[string]interface{}{} - result["required"] = []string{} - } else { - // Default schema when nil - result["type"] = "object" - result["properties"] = map[string]interface{}{} - result["required"] = []string{} + // Try direct conversion first (fast path) + if schemaMap, ok := schema.(map[string]interface{}); ok { + return schemaMap + } + + // Handle json.RawMessage and []byte - unmarshal directly + var jsonData []byte + if rawMsg, ok := schema.(json.RawMessage); ok { + jsonData = rawMsg + } else if bytes, ok := schema.([]byte); ok { + jsonData = bytes + } + + if jsonData != nil { + var result map[string]interface{} + if err := json.Unmarshal(jsonData, &result); err == nil { + return result + } + // Fallback on error + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + "required": []string{}, + } + } + + // For other types (structs, etc.), convert via JSON marshal/unmarshal + var err error + jsonData, err = json.Marshal(schema) + if err != nil { + // Fallback to empty schema if marshaling fails + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + "required": []string{}, + } + } + + var result map[string]interface{} + if err := json.Unmarshal(jsonData, &result); err != nil { + // Fallback to empty schema if unmarshaling fails + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + "required": []string{}, + } } return result diff --git a/pkg/tools/mcp_tool_test.go b/pkg/tools/mcp_tool_test.go index 597bbb52b..92fb66234 100644 --- a/pkg/tools/mcp_tool_test.go +++ b/pkg/tools/mcp_tool_test.go @@ -141,9 +141,11 @@ func TestMCPTool_Description(t *testing.T) { // TestMCPTool_Parameters verifies parameter schema conversion func TestMCPTool_Parameters(t *testing.T) { tests := []struct { - name string - inputSchema interface{} - expectType string + name string + inputSchema interface{} + expectType string + checkProperty string + expectProperty bool }{ { name: "map schema", @@ -157,12 +159,35 @@ func TestMCPTool_Parameters(t *testing.T) { }, "required": []string{"query"}, }, - expectType: "object", + expectType: "object", + checkProperty: "query", + expectProperty: true, }, { - name: "nil schema", - inputSchema: nil, - expectType: "object", + name: "nil schema", + inputSchema: nil, + expectType: "object", + expectProperty: false, + }, + { + name: "json.RawMessage schema", + inputSchema: []byte(`{ + "type": "object", + "properties": { + "repo": { + "type": "string", + "description": "Repository name" + }, + "stars": { + "type": "integer", + "description": "Minimum stars" + } + }, + "required": ["repo"] + }`), + expectType: "object", + checkProperty: "repo", + expectProperty: true, }, } @@ -184,6 +209,22 @@ func TestMCPTool_Parameters(t *testing.T) { if params["type"] != tt.expectType { t.Errorf("Expected type '%s', got '%v'", tt.expectType, params["type"]) } + + // Check if property exists when expected + if tt.checkProperty != "" { + properties, ok := params["properties"].(map[string]interface{}) + if !ok && tt.expectProperty { + t.Errorf("Expected properties to be a map") + return + } + if ok { + _, hasProperty := properties[tt.checkProperty] + if hasProperty != tt.expectProperty { + t.Errorf("Expected property '%s' existence: %v, got: %v", + tt.checkProperty, tt.expectProperty, hasProperty) + } + } + } }) } } From aed7296c0d95ff88683cce0b6733510fd5895f3a Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:53:15 +0800 Subject: [PATCH 17/43] fix(agent): tie MCP connections to agent lifecycle context - Defer MCP server initialization to Run() using agent's context - Add mcpConfig and mcpInitOnce fields to AgentLoop - Use sync.Once to ensure MCP loads exactly once with proper context - Prevents orphaned subprocesses and resource leaks on cancellation This fixes GitHub Copilot feedback that MCP connections with context.Background() won't terminate when the agent stops, causing potential resource leaks and orphaned stdio/SSE connections. --- pkg/agent/loop.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 9349f86f3..afd8e8481 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -42,7 +42,9 @@ type AgentLoop struct { state *state.Manager contextBuilder *ContextBuilder tools *tools.ToolRegistry - mcpManager *mcp.Manager // MCP server manager for resource cleanup + mcpManager *mcp.Manager // MCP server manager for resource cleanup + mcpConfig *config.Config // Config for lazy MCP initialization + mcpInitOnce sync.Once // Ensures MCP is initialized only once running atomic.Bool summarizing sync.Map // Tracks which sessions are currently being summarized channelManager *channels.Manager @@ -129,15 +131,9 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers restrict := cfg.Agents.Defaults.RestrictToWorkspace - // Initialize MCP Manager and load servers + // Create MCP Manager (actual server loading deferred to Run()) + // This ensures MCP connections use the agent's lifecycle context mcpManager := mcp.NewManager() - ctx := context.Background() - if err := mcpManager.LoadFromConfig(ctx, cfg); err != nil { - logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", - map[string]interface{}{ - "error": err.Error(), - }) - } // Create tool registry for main agent toolsRegistry := createToolRegistry(workspace, restrict, cfg, msgBus, mcpManager) @@ -177,6 +173,7 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers contextBuilder: contextBuilder, tools: toolsRegistry, mcpManager: mcpManager, + mcpConfig: cfg, // Store config for lazy initialization in Run() summarizing: sync.Map{}, } } @@ -184,6 +181,17 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers func (al *AgentLoop) Run(ctx context.Context) error { al.running.Store(true) + // Initialize MCP servers using the agent's lifecycle context + // This ensures MCP connections are cancelled when the agent stops + al.mcpInitOnce.Do(func() { + if err := al.mcpManager.LoadFromConfig(ctx, al.mcpConfig); err != nil { + logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", + map[string]interface{}{ + "error": err.Error(), + }) + } + }) + for al.running.Load() { select { case <-ctx.Done(): From 2318232b71737ac100495ec86fdf0833fe379c4c Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 19:56:00 +0800 Subject: [PATCH 18/43] fix(agent): ensure MCP cleanup on all Run() exit paths - Add defer in Run() to guarantee MCP connection cleanup - Handles both normal termination and context cancellation - Prevents resource leaks when Run() exits via ctx.Done() - MCP Manager.Close() is idempotent, safe to call from both defer and Stop() This fixes GitHub Copilot feedback that MCP cleanup only happened in Stop() but Run() could return on ctx.Done() without cleanup, causing subprocess/session leaks on normal cancellation shutdown. --- pkg/agent/loop.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index afd8e8481..a18e962fa 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -42,9 +42,9 @@ type AgentLoop struct { state *state.Manager contextBuilder *ContextBuilder tools *tools.ToolRegistry - mcpManager *mcp.Manager // MCP server manager for resource cleanup - mcpConfig *config.Config // Config for lazy MCP initialization - mcpInitOnce sync.Once // Ensures MCP is initialized only once + mcpManager *mcp.Manager // MCP server manager for resource cleanup + mcpConfig *config.Config // Config for lazy MCP initialization + mcpInitOnce sync.Once // Ensures MCP is initialized only once running atomic.Bool summarizing sync.Map // Tracks which sessions are currently being summarized channelManager *channels.Manager @@ -192,6 +192,18 @@ func (al *AgentLoop) Run(ctx context.Context) error { } }) + // Ensure MCP connections are cleaned up on all exit paths + defer func() { + if al.mcpManager != nil { + if err := al.mcpManager.Close(); err != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]interface{}{ + "error": err.Error(), + }) + } + } + }() + for al.running.Load() { select { case <-ctx.Done(): From 0f6fadb445041aba56d04b764c6a3776b25427ff Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Mon, 16 Feb 2026 20:00:37 +0800 Subject: [PATCH 19/43] fix(agent): register MCP tools after server initialization Critical bug fix: - MCP tools were never registered because servers loaded in Run() but tool registration happened in NewAgentLoop() with empty manager - Move MCP tool registration from createToolRegistry to Run() - Register MCP tools for both main agent and subag after successful server loading - Add subagentManager field to AgentLoop for dynamic registration - Add tool_count logging for better observability This ensures MCP tools are properly available to both agent and subagents. --- pkg/agent/loop.go | 106 ++++++++++++++++++++++++++-------------------- 1 file changed, 61 insertions(+), 45 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index a18e962fa..c4d7c4ae7 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -32,22 +32,23 @@ import ( ) type AgentLoop struct { - bus *bus.MessageBus - provider providers.LLMProvider - workspace string - model string - contextWindow int // Maximum context window size in tokens - maxIterations int - sessions *session.SessionManager - state *state.Manager - contextBuilder *ContextBuilder - tools *tools.ToolRegistry - mcpManager *mcp.Manager // MCP server manager for resource cleanup - mcpConfig *config.Config // Config for lazy MCP initialization - mcpInitOnce sync.Once // Ensures MCP is initialized only once - running atomic.Bool - summarizing sync.Map // Tracks which sessions are currently being summarized - channelManager *channels.Manager + bus *bus.MessageBus + provider providers.LLMProvider + workspace string + model string + contextWindow int // Maximum context window size in tokens + maxIterations int + sessions *session.SessionManager + state *state.Manager + contextBuilder *ContextBuilder + tools *tools.ToolRegistry + mcpManager *mcp.Manager // MCP server manager for resource cleanup + mcpConfig *config.Config // Config for lazy MCP initialization + mcpInitOnce sync.Once // Ensures MCP is initialized only once + subagentManager *tools.SubagentManager // Subagent manager for MCP tool registration + running atomic.Bool + summarizing sync.Map // Tracks which sessions are currently being summarized + channelManager *channels.Manager } // processOptions configures how a message is processed @@ -105,22 +106,8 @@ func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msg }) registry.Register(messageTool) - // Register MCP tools from all connected servers - if mcpManager != nil { - servers := mcpManager.GetServers() - for serverName, conn := range servers { - for _, tool := range conn.Tools { - mcpTool := tools.NewMCPTool(mcpManager, serverName, tool) - registry.Register(mcpTool) - logger.DebugCF("agent", "Registered MCP tool", - map[string]interface{}{ - "server": serverName, - "tool": tool.Name, - "name": mcpTool.Name(), - }) - } - } - } + // Note: MCP tools are registered dynamically in Run() after servers are loaded + // This ensures we use the agent's lifecycle context for MCP connections return registry } @@ -162,19 +149,20 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers contextBuilder.SetToolsRegistry(toolsRegistry) return &AgentLoop{ - bus: msgBus, - provider: provider, - workspace: workspace, - model: cfg.Agents.Defaults.Model, - contextWindow: cfg.Agents.Defaults.MaxTokens, // Restore context window for summarization - maxIterations: cfg.Agents.Defaults.MaxToolIterations, - sessions: sessionsManager, - state: stateManager, - contextBuilder: contextBuilder, - tools: toolsRegistry, - mcpManager: mcpManager, - mcpConfig: cfg, // Store config for lazy initialization in Run() - summarizing: sync.Map{}, + bus: msgBus, + provider: provider, + workspace: workspace, + model: cfg.Agents.Defaults.Model, + contextWindow: cfg.Agents.Defaults.MaxTokens, // Restore context window for summarization + maxIterations: cfg.Agents.Defaults.MaxToolIterations, + sessions: sessionsManager, + state: stateManager, + contextBuilder: contextBuilder, + tools: toolsRegistry, + mcpManager: mcpManager, + mcpConfig: cfg, // Store config for lazy initialization in Run() + subagentManager: subagentManager, + summarizing: sync.Map{}, } } @@ -189,6 +177,34 @@ func (al *AgentLoop) Run(ctx context.Context) error { map[string]interface{}{ "error": err.Error(), }) + } else { + // Register for both main agent and subagents + servers := al.mcpManager.GetServers() + toolCount := 0 + for serverName, conn := range servers { + for _, tool := range conn.Tools { + mcpTool := tools.NewMCPTool(al.mcpManager, serverName, tool) + al.tools.Register(mcpTool) + + // Also register for subagent + if al.subagentManager != nil { + al.subagentManager.RegisterTool(tools.NewMCPTool(al.mcpManager, serverName, tool)) + } + + toolCount++ + logger.DebugCF("agent", "Registered MCP tool", + map[string]interface{}{ + "server": serverName, + "tool": tool.Name, + "name": mcpTool.Name(), + }) + } + } + logger.InfoCF("agent", "MCP tools registered successfully", + map[string]interface{}{ + "server_count": len(servers), + "tool_count": toolCount, + }) } }) From 6892d006d680902b32e07743434101db4cafec92 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Tue, 17 Feb 2026 10:39:39 +0800 Subject: [PATCH 20/43] perf(agent): reduce memory footprint by storing minimal MCP dependencies Replace full *config.Config reference with config.MCPConfig value type in AgentLoop to allow garbage collection of unused configuration data. Changes: - AgentLoop now stores only MCPConfig and workspacePath (minimal deps) - Add mcp.Manager.LoadFromMCPConfig() for minimal dependency version - Keep LoadFromConfig() for backward compatibility - Full Config object can be GC'd after NewAgentLoop() returns This optimization reduces memory usage by not holding references to unused channel, provider, gateway, and device configurations. --- pkg/agent/loop.go | 8 +++++--- pkg/mcp/manager.go | 19 +++++++++++-------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index c4d7c4ae7..69aa8759a 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -43,7 +43,8 @@ type AgentLoop struct { contextBuilder *ContextBuilder tools *tools.ToolRegistry mcpManager *mcp.Manager // MCP server manager for resource cleanup - mcpConfig *config.Config // Config for lazy MCP initialization + mcpConfig config.MCPConfig // MCP config for lazy initialization (minimal dependency) + workspacePath string // Workspace path for resolving relative envFile paths mcpInitOnce sync.Once // Ensures MCP is initialized only once subagentManager *tools.SubagentManager // Subagent manager for MCP tool registration running atomic.Bool @@ -160,7 +161,8 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers contextBuilder: contextBuilder, tools: toolsRegistry, mcpManager: mcpManager, - mcpConfig: cfg, // Store config for lazy initialization in Run() + mcpConfig: cfg.Tools.MCP, // Store only MCP config (minimal dependency) + workspacePath: workspace, // Store workspace path for envFile resolution subagentManager: subagentManager, summarizing: sync.Map{}, } @@ -172,7 +174,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { // Initialize MCP servers using the agent's lifecycle context // This ensures MCP connections are cancelled when the agent stops al.mcpInitOnce.Do(func() { - if err := al.mcpManager.LoadFromConfig(ctx, al.mcpConfig); err != nil { + if err := al.mcpManager.LoadFromMCPConfig(ctx, al.mcpConfig, al.workspacePath); err != nil { logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", map[string]interface{}{ "error": err.Error(), diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 8449486f5..833755cbd 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -114,29 +114,32 @@ func NewManager() *Manager { // LoadFromConfig loads MCP servers from configuration func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error { - if !cfg.Tools.MCP.Enabled { + return m.LoadFromMCPConfig(ctx, cfg.Tools.MCP, cfg.WorkspacePath()) +} + +// LoadFromMCPConfig loads MCP servers from MCP configuration and workspace path. +// This is the minimal dependency version that doesn't require the full Config object. +func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig, workspacePath string) error { + if !mcpCfg.Enabled { logger.InfoCF("mcp", "MCP integration is disabled", nil) return nil } - if len(cfg.Tools.MCP.Servers) == 0 { + if len(mcpCfg.Servers) == 0 { logger.InfoCF("mcp", "No MCP servers configured", nil) return nil } logger.InfoCF("mcp", "Initializing MCP servers", map[string]interface{}{ - "count": len(cfg.Tools.MCP.Servers), + "count": len(mcpCfg.Servers), }) - // Get workspace path for resolving relative envFile paths - workspacePath := cfg.WorkspacePath() - var wg sync.WaitGroup - errs := make(chan error, len(cfg.Tools.MCP.Servers)) + errs := make(chan error, len(mcpCfg.Servers)) enabledCount := 0 - for name, serverCfg := range cfg.Tools.MCP.Servers { + for name, serverCfg := range mcpCfg.Servers { if !serverCfg.Enabled { logger.DebugCF("mcp", "Skipping disabled server", map[string]interface{}{ From 4113190c2a981e9ac3f0ffd9738e6985797976c3 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Tue, 17 Feb 2026 10:41:29 +0800 Subject: [PATCH 21/43] chore(config): remove example MCP servers from default config Remove pre-populated example servers (filesystem, github, brave-search, postgres) from DefaultConfig() to reduce memory footprint per instance. Changes: - Set MCP.Servers to empty map instead of 4 example servers - Reduces default config size by ~500 bytes per instance - Users should add MCP servers via config.json or documentation Example configurations are still available in: - README.md MCP section - config.example.json - Official MCP documentation This optimization benefits deployments with many agent instances. --- pkg/config/config.go | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index ddc376645..d1dcffd03 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -353,35 +353,7 @@ func DefaultConfig() *Config { }, MCP: MCPConfig{ Enabled: false, - Servers: map[string]MCPServerConfig{ - "filesystem": { - Enabled: false, - Command: "npx", - Args: []string{"-y", "@modelcontextprotocol/server-filesystem", "/tmp"}, - Env: map[string]string{}, - }, - "github": { - Enabled: false, - Command: "npx", - Args: []string{"-y", "@modelcontextprotocol/server-github"}, - Env: map[string]string{ - "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_TOKEN", - }, - }, - "brave-search": { - Enabled: false, - Command: "npx", - Args: []string{"-y", "@modelcontextprotocol/server-brave-search"}, - Env: map[string]string{ - "BRAVE_API_KEY": "YOUR_BRAVE_API_KEY", - }, - }, - "postgres": { - Enabled: false, - Command: "npx", - Args: []string{"-y", "@modelcontextprotocol/server-postgres", "postgresql://user:password@localhost/dbname"}, - }, - }, + Servers: map[string]MCPServerConfig{}, }, }, Heartbeat: HeartbeatConfig{ From e38364b08a60f9d586c3191ebcdf208b3eef07db Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Tue, 17 Feb 2026 10:57:38 +0800 Subject: [PATCH 22/43] build(docker): migrate full image from Debian to Alpine base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace node:24-bookworm-slim with node:24-alpine3.23 to reduce image size and improve build efficiency. Changes: - Base image: node:24-bookworm-slim → node:24-alpine3.23 - Package manager: apt-get → apk - Package names: python3-pip → py3-pip - Remove python3-venv (included in Alpine Python3) - Use apk --no-cache for cleaner image layers Expected benefits: - Reduce base image size by ~100-200MB (30-40% reduction) - Faster image pulls and container startup - Full MCP support maintained (Node.js, Python, uv) Estimated final image size: ~600-700MB (vs ~800MB before) --- Dockerfile.full | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Dockerfile.full b/Dockerfile.full index aebcdafe6..30e1680d5 100644 --- a/Dockerfile.full +++ b/Dockerfile.full @@ -18,17 +18,15 @@ RUN make build # ============================================================ # Stage 2: Node.js-based runtime with full MCP support # ============================================================ -FROM node:24-bookworm-slim +FROM node:24-alpine3.23 # Install runtime dependencies -RUN apt-get update && apt-get install -y --no-install-recommends \ +RUN apk add --no-cache \ ca-certificates \ curl \ git \ python3 \ - python3-pip \ - python3-venv \ - && rm -rf /var/lib/apt/lists/* + py3-pip # Install uv and symlink to system path RUN curl -LsSf https://astral.sh/uv/install.sh | sh && \ From 47533a00cd5ca796aeb1a43d1ed80f93fdaf4d5e Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 18:53:24 +0800 Subject: [PATCH 23/43] style: format code with gofmt --- pkg/agent/loop.go | 4 ++-- pkg/mcp/manager_test.go | 14 +++++++------- pkg/tools/mcp_tool_test.go | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 69aa8759a..f150534b8 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -161,8 +161,8 @@ func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers contextBuilder: contextBuilder, tools: toolsRegistry, mcpManager: mcpManager, - mcpConfig: cfg.Tools.MCP, // Store only MCP config (minimal dependency) - workspacePath: workspace, // Store workspace path for envFile resolution + mcpConfig: cfg.Tools.MCP, // Store only MCP config (minimal dependency) + workspacePath: workspace, // Store workspace path for envFile resolution subagentManager: subagentManager, summarizing: sync.Map{}, } diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go index e888d8764..1c69e75f2 100644 --- a/pkg/mcp/manager_test.go +++ b/pkg/mcp/manager_test.go @@ -134,32 +134,32 @@ func TestEnvFilePriority(t *testing.T) { // Create a temporary .env file tmpDir := t.TempDir() envFile := filepath.Join(tmpDir, ".env") - + envContent := `API_KEY=from_file DATABASE_URL=from_file SHARED_VAR=from_file` - + if err := os.WriteFile(envFile, []byte(envContent), 0644); err != nil { t.Fatalf("Failed to create .env file: %v", err) } - + // Load envFile envVars, err := loadEnvFile(envFile) if err != nil { t.Fatalf("Failed to load env file: %v", err) } - + // Verify envFile variables if envVars["API_KEY"] != "from_file" { t.Errorf("Expected API_KEY=from_file, got %s", envVars["API_KEY"]) } - + // Simulate config.Env overriding envFile configEnv := map[string]string{ "SHARED_VAR": "from_config", "NEW_VAR": "from_config", } - + // Merge: envFile first, then config overrides merged := make(map[string]string) for k, v := range envVars { @@ -168,7 +168,7 @@ SHARED_VAR=from_file` for k, v := range configEnv { merged[k] = v } - + // Verify priority: config.Env should override envFile if merged["SHARED_VAR"] != "from_config" { t.Errorf("Expected SHARED_VAR=from_config (config should override file), got %s", merged["SHARED_VAR"]) diff --git a/pkg/tools/mcp_tool_test.go b/pkg/tools/mcp_tool_test.go index 92fb66234..580abc2ba 100644 --- a/pkg/tools/mcp_tool_test.go +++ b/pkg/tools/mcp_tool_test.go @@ -39,9 +39,9 @@ func TestNewMCPTool(t *testing.T) { "type": "string", "description": "Test input", }, - }, }, - } + }, + } mcpTool := NewMCPTool(manager, "test_server", tool) From ffa01986ce1513846f42d55bd8aab9d0bc3cd8fd Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 19:26:02 +0800 Subject: [PATCH 24/43] fix(agent): scope MCP manager cleanup to successful initialization Move defer cleanup inside else block to only clean up when MCP servers are successfully initialized. This prevents unnecessary cleanup attempts when LoadFromMCPConfig fails. Addresses Copilot code review feedback. --- pkg/agent/loop.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index d2d49e5b7..e5a1465eb 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -150,6 +150,16 @@ func (al *AgentLoop) Run(ctx context.Context) error { "error": err.Error(), }) } else { + // Ensure MCP connections are cleaned up on exit, only if initialization succeeded + defer func() { + if err := mcpManager.Close(); err != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]interface{}{ + "error": err.Error(), + }) + } + }() + // Register MCP tools for all agents servers := mcpManager.GetServers() toolCount := 0 @@ -179,16 +189,6 @@ func (al *AgentLoop) Run(ctx context.Context) error { "tool_count": toolCount, }) } - - // Ensure MCP connections are cleaned up on exit - defer func() { - if err := mcpManager.Close(); err != nil { - logger.ErrorCF("agent", "Failed to close MCP manager", - map[string]interface{}{ - "error": err.Error(), - }) - } - }() } for al.running.Load() { From dea381c38567fc242df0f3fd8d64172134712f67 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 19:31:13 +0800 Subject: [PATCH 25/43] improve(agent): clarify MCP tool registration logging Separate tool counting metrics for better clarity: - unique_tools: number of distinct MCP tools - total_registrations: total tool registrations across all agents - agent_count: number of agents receiving the tools Previously, tool_count was misleading as it showed total registrations, making it appear that more unique tools were registered than actually exist. Addresses Copilot code review feedback. --- pkg/agent/loop.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index e5a1465eb..87b47f4ad 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -162,8 +162,12 @@ func (al *AgentLoop) Run(ctx context.Context) error { // Register MCP tools for all agents servers := mcpManager.GetServers() - toolCount := 0 + uniqueTools := 0 + totalRegistrations := 0 + agentCount := len(al.registry.ListAgentIDs()) + for serverName, conn := range servers { + uniqueTools += len(conn.Tools) for _, tool := range conn.Tools { for _, agentID := range al.registry.ListAgentIDs() { agent, ok := al.registry.GetAgent(agentID) @@ -172,7 +176,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { } mcpTool := tools.NewMCPTool(mcpManager, serverName, tool) agent.Tools.Register(mcpTool) - toolCount++ + totalRegistrations++ logger.DebugCF("agent", "Registered MCP tool", map[string]interface{}{ "agent_id": agentID, @@ -185,8 +189,10 @@ func (al *AgentLoop) Run(ctx context.Context) error { } logger.InfoCF("agent", "MCP tools registered successfully", map[string]interface{}{ - "server_count": len(servers), - "tool_count": toolCount, + "server_count": len(servers), + "unique_tools": uniqueTools, + "total_registrations": totalRegistrations, + "agent_count": agentCount, }) } } From f0ce26ff2bfdf0f3a05743ca43594d022689760d Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 19:43:48 +0800 Subject: [PATCH 26/43] style(config): use snake_case for EnvFile JSON field name Change 'envFile' to 'env_file' to maintain consistency with the rest of the codebase which uses snake_case for JSON field names (e.g., 'api_key', 'api_base', 'max_results', 'exec_timeout_minutes'). Addresses Copilot code review feedback. --- pkg/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index bb3d6c3bc..53b0c6e1d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -333,7 +333,7 @@ type MCPServerConfig struct { // Env are environment variables to set for the server process (stdio only) Env map[string]string `json:"env,omitempty"` // EnvFile is the path to a file containing environment variables (stdio only) - EnvFile string `json:"envFile,omitempty"` + EnvFile string `json:"env_file,omitempty"` // Type is "stdio", "sse", or "http" (default: stdio if command is set, sse if url is set) Type string `json:"type,omitempty"` // URL is used for SSE/HTTP transport From 757741476181b6ed2b39cf1626c472eb069fe631 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 19:45:15 +0800 Subject: [PATCH 27/43] fix(mcp): ensure proper environment variable override semantics Use a map to merge environment variables with guaranteed override behavior. Config variables (cfg.Env) now properly override file variables (envFile), which in turn override parent process environment. Previously, simply appending to a slice could result in duplicate variables, and while most systems use the last occurrence, this behavior is not guaranteed and could lead to unexpected results. Addresses Copilot code review feedback. --- pkg/mcp/manager.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 833755cbd..923d01f3c 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -284,8 +284,16 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP // Create command with context cmd := exec.CommandContext(ctx, cfg.Command, cfg.Args...) - // Set environment variables - env := cmd.Environ() + // Build environment variables with proper override semantics + // Use a map to ensure config variables override file variables + envMap := make(map[string]string) + + // Start with parent process environment + for _, e := range cmd.Environ() { + if idx := strings.Index(e, "="); idx > 0 { + envMap[e[:idx]] = e[idx+1:] + } + } // Load environment variables from file if specified if cfg.EnvFile != "" { @@ -294,7 +302,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP return fmt.Errorf("failed to load env file %s: %w", cfg.EnvFile, err) } for k, v := range envVars { - env = append(env, fmt.Sprintf("%s=%s", k, v)) + envMap[k] = v } logger.DebugCF("mcp", "Loaded environment variables from file", map[string]interface{}{ @@ -305,16 +313,16 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP } // Environment variables from config override those from file - if len(cfg.Env) > 0 { - for k, v := range cfg.Env { - env = append(env, fmt.Sprintf("%s=%s", k, v)) - } + for k, v := range cfg.Env { + envMap[k] = v } - // Set environment if we added any variables - if len(env) > len(cmd.Environ()) { - cmd.Env = env + // Convert map to slice + env := make([]string, 0, len(envMap)) + for k, v := range envMap { + env = append(env, fmt.Sprintf("%s=%s", k, v)) } + cmd.Env = env transport = &mcp.CommandTransport{Command: cmd} default: From f1b798434d2f098d26b996a5f50995ffda9af1de Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 19:47:05 +0800 Subject: [PATCH 28/43] fix(mcp): prevent race condition between CallTool and Close Add a closed flag to the Manager struct to prevent CallTool from accessing server connections after Close has been called. The flag is checked within the RLock in CallTool to ensure thread-safety. Previously, CallTool could obtain a server reference using RLock, then that reference could be closed by Close() running concurrently, leading to use-after-close errors. Now: 1. CallTool checks the closed flag before accessing servers 2. Close sets the closed flag before closing connections 3. CallTool directly accesses m.servers within the lock instead of using GetServer() to avoid releasing the lock prematurely This ensures CallTool will not use a server connection that is being closed or has been closed. Addresses Copilot code review feedback. --- pkg/mcp/manager.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 923d01f3c..1fc253774 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -103,6 +103,7 @@ type ServerConnection struct { type Manager struct { servers map[string]*ServerConnection mu sync.RWMutex + closed bool } // NewManager creates a new MCP manager @@ -403,7 +404,14 @@ func (m *Manager) GetServer(name string) (*ServerConnection, bool) { // CallTool calls a tool on a specific server func (m *Manager) CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { - conn, ok := m.GetServer(serverName) + m.mu.RLock() + if m.closed { + m.mu.RUnlock() + return nil, fmt.Errorf("manager is closed") + } + conn, ok := m.servers[serverName] + m.mu.RUnlock() + if !ok { return nil, fmt.Errorf("server %s not found", serverName) } @@ -426,6 +434,11 @@ func (m *Manager) Close() error { m.mu.Lock() defer m.mu.Unlock() + if m.closed { + return nil + } + m.closed = true + logger.InfoCF("mcp", "Closing all MCP server connections", map[string]interface{}{ "count": len(m.servers), From a7a4e88fff1af11d8a675588ab7ab7cae830d222 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Thu, 19 Feb 2026 20:03:00 +0800 Subject: [PATCH 29/43] fix(agent): use fallback workspace path for MCP initialization Use cfg.WorkspacePath() as a fallback when defaultAgent is nil or its Workspace is empty. This ensures MCP servers with relative envFile paths can always resolve them correctly, even when agents haven't been fully initialized yet. Previously, workspacePath would be an empty string in these cases, causing relative envFile paths to fail to resolve. Now the fallback guarantees a valid workspace path is always provided to LoadFromMCPConfig. Addresses Copilot code review feedback. --- pkg/agent/loop.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 87b47f4ad..fc83007c2 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -140,8 +140,10 @@ func (al *AgentLoop) Run(ctx context.Context) error { mcpManager := mcp.NewManager() defaultAgent := al.registry.GetDefaultAgent() workspacePath := "" - if defaultAgent != nil { + if defaultAgent != nil && defaultAgent.Workspace != "" { workspacePath = defaultAgent.Workspace + } else { + workspacePath = al.cfg.WorkspacePath() } if err := mcpManager.LoadFromMCPConfig(ctx, al.cfg.Tools.MCP, workspacePath); err != nil { From fb2b5940600c03349787a4147264f32133de5e92 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:39:42 +0800 Subject: [PATCH 30/43] fix(scripts): specify service name in docker compose build Avoid building zero services when all services are gated behind profiles. Without an explicit service target, 'docker compose build' silently skips all profile-gated services, causing subsequent 'docker compose run' to use stale or missing images. --- scripts/test-docker-mcp.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index a0c9e232a..5c4e5cb56 100755 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -11,7 +11,7 @@ echo "" # Build the image echo "📦 Building Docker image..." -docker compose -f "$COMPOSE_FILE" build +docker compose -f "$COMPOSE_FILE" build "$SERVICE" # Test npx echo "✅ Testing npx..." From 246fdf3f33d48b7361f9623f1566a020c894a32c Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:40:55 +0800 Subject: [PATCH 31/43] fix(mcp): guard against nil result from CallTool CallTool can return (nil, nil) if the underlying MCP library misbehaves. Without a nil check, result.IsError would panic. Return an explicit error ToolResult instead. --- pkg/tools/mcp_tool.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index c29ab8525..6bd3d75e3 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -119,6 +119,11 @@ func (t *MCPTool) Execute(ctx context.Context, args map[string]interface{}) *Too return ErrorResult(fmt.Sprintf("MCP tool execution failed: %v", err)).WithError(err) } + if result == nil { + nilErr := fmt.Errorf("MCP tool returned nil result without error") + return ErrorResult("MCP tool execution failed: nil result").WithError(nilErr) + } + // Handle error result from server if result.IsError { errMsg := extractContentText(result.Content) From 59e9c5545413626d11e3628f981503ea16e8f82d Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:42:26 +0800 Subject: [PATCH 32/43] docs(config): restore MCP server examples in config.example.json Add back filesystem, github, brave-search, and postgres as example MCP server configurations. These were removed from DefaultConfig() to reduce memory footprint, but should remain in the example config as documentation for users setting up MCP servers. --- config/config.example.json | 45 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index 5e49de2a7..2ad5664d6 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -220,7 +220,48 @@ }, "mcp": { "enabled": false, - "servers": {} + "servers": { + "filesystem": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-filesystem", + "/tmp" + ] + }, + "github": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-github" + ], + "env": { + "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_TOKEN" + } + }, + "brave-search": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-brave-search" + ], + "env": { + "BRAVE_API_KEY": "YOUR_BRAVE_API_KEY" + } + }, + "postgres": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-postgres", + "postgresql://user:password@localhost/dbname" + ] + } + } }, "exec": { "enable_deny_patterns": false, @@ -250,4 +291,4 @@ "host": "0.0.0.0", "port": 18790 } -} +} \ No newline at end of file From 33058b534e50b6fe6507c1c2b5d53b1014893f09 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:45:00 +0800 Subject: [PATCH 33/43] fix(mcp): reject empty keys in loadEnvFile A line like '=value' would result in envVars[""] = "value", producing an invalid environment entry for the child process. Return an error instead when the key is empty. --- pkg/mcp/manager.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 1fc253774..bbc6925ea 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -73,6 +73,10 @@ func loadEnvFile(path string) (map[string]string, error) { key := strings.TrimSpace(parts[0]) value := strings.TrimSpace(parts[1]) + if key == "" { + return nil, fmt.Errorf("invalid format at line %d: empty key", lineNum) + } + // Remove surrounding quotes if present if len(value) >= 2 { if (value[0] == '"' && value[len(value)-1] == '"') || From d2b3fc1dd03abae11e1f2ab15258c8b31a5598a8 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:46:06 +0800 Subject: [PATCH 34/43] fix(mcp): include server name and cause in Close() errors Previously Close() discarded all underlying errors and returned only 'failed to close N server(s)', making debugging impossible. Now each error wraps the server name and original cause, and all errors are joined so callers can inspect the full failure list. --- pkg/mcp/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index bbc6925ea..8ff3af3dc 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -456,14 +456,14 @@ func (m *Manager) Close() error { "server": name, "error": err.Error(), }) - errs = append(errs, err) + errs = append(errs, fmt.Errorf("server %s: %w", name, err)) } } m.servers = make(map[string]*ServerConnection) if len(errs) > 0 { - return fmt.Errorf("failed to close %d server(s)", len(errs)) + return fmt.Errorf("failed to close %d server(s): %w", len(errs), errors.Join(errs...)) } return nil From 11dbc301f927ce7299df222996d5d83484647179 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 13:48:41 +0800 Subject: [PATCH 35/43] perf(agent): cache ListAgentIDs() result before MCP tool registration loop ListAgentIDs() was called on every iteration of the inner tool loop, causing repeated allocations. Capture the slice once and reuse it for both agentCount and the registration loop. --- pkg/agent/loop.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index e3efea17b..eef800921 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -185,12 +185,13 @@ func (al *AgentLoop) Run(ctx context.Context) error { servers := mcpManager.GetServers() uniqueTools := 0 totalRegistrations := 0 - agentCount := len(al.registry.ListAgentIDs()) + agentIDs := al.registry.ListAgentIDs() + agentCount := len(agentIDs) for serverName, conn := range servers { uniqueTools += len(conn.Tools) for _, tool := range conn.Tools { - for _, agentID := range al.registry.ListAgentIDs() { + for _, agentID := range agentIDs { agent, ok := al.registry.GetAgent(agentID) if !ok { continue From cfc29a1383a3cdc26cdd2a74c162fc0cd7dec6c7 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sat, 21 Feb 2026 14:10:48 +0800 Subject: [PATCH 36/43] fix(mcp): prevent use-after-close race between CallTool and Close A race could occur when Close() called conn.Session.Close() concurrently with an in-flight conn.Session.CallTool(), leading to undefined behavior. Fix by adding a sync.WaitGroup to Manager: - CallTool increments the WaitGroup while holding the read lock (after checking m.closed), ensuring no new calls are counted after Close sets the flag - Close sets m.closed=true, releases the write lock, then waits for all in-flight calls to finish via wg.Wait() before closing sessions --- pkg/mcp/manager.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 8ff3af3dc..b28ba4670 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -108,6 +108,7 @@ type Manager struct { servers map[string]*ServerConnection mu sync.RWMutex closed bool + wg sync.WaitGroup // tracks in-flight CallTool calls } // NewManager creates a new MCP manager @@ -414,11 +415,15 @@ func (m *Manager) CallTool(ctx context.Context, serverName, toolName string, arg return nil, fmt.Errorf("manager is closed") } conn, ok := m.servers[serverName] + if ok { + m.wg.Add(1) + } m.mu.RUnlock() if !ok { return nil, fmt.Errorf("server %s not found", serverName) } + defer m.wg.Done() params := &mcp.CallToolParams{ Name: toolName, @@ -436,12 +441,18 @@ func (m *Manager) CallTool(ctx context.Context, serverName, toolName string, arg // Close closes all server connections func (m *Manager) Close() error { m.mu.Lock() - defer m.mu.Unlock() - if m.closed { + m.mu.Unlock() return nil } m.closed = true + m.mu.Unlock() + + // Wait for all in-flight CallTool calls to finish before closing sessions + m.wg.Wait() + + m.mu.Lock() + defer m.mu.Unlock() logger.InfoCF("mcp", "Closing all MCP server connections", map[string]interface{}{ From 6aade43236e9d5a90c11871dd4d34c0d65bc351f Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 22 Feb 2026 15:03:20 +0800 Subject: [PATCH 37/43] docs: add MCP tool configuration documentation --- docs/tools_configuration.md | 141 +++++++++++++++++++++++++++--------- 1 file changed, 108 insertions(+), 33 deletions(-) diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index 8aba1aa91..6204fb0c8 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -8,6 +8,7 @@ PicoClaw's tools configuration is located in the `tools` field of `config.json`. { "tools": { "web": { ... }, + "mcp": { ... }, "exec": { ... }, "cron": { ... }, "skills": { ... } @@ -21,35 +22,35 @@ Web tools are used for web search and fetching. ### Brave -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `enabled` | bool | false | Enable Brave search | -| `api_key` | string | - | Brave Search API key | -| `max_results` | int | 5 | Maximum number of results | +| Config | Type | Default | Description | +| ------------- | ------ | ------- | ------------------------- | +| `enabled` | bool | false | Enable Brave search | +| `api_key` | string | - | Brave Search API key | +| `max_results` | int | 5 | Maximum number of results | ### DuckDuckGo -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `enabled` | bool | true | Enable DuckDuckGo search | -| `max_results` | int | 5 | Maximum number of results | +| Config | Type | Default | Description | +| ------------- | ---- | ------- | ------------------------- | +| `enabled` | bool | true | Enable DuckDuckGo search | +| `max_results` | int | 5 | Maximum number of results | ### Perplexity -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `enabled` | bool | false | Enable Perplexity search | -| `api_key` | string | - | Perplexity API key | -| `max_results` | int | 5 | Maximum number of results | +| Config | Type | Default | Description | +| ------------- | ------ | ------- | ------------------------- | +| `enabled` | bool | false | Enable Perplexity search | +| `api_key` | string | - | Perplexity API key | +| `max_results` | int | 5 | Maximum number of results | ## Exec Tool The exec tool is used to execute shell commands. -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `enable_deny_patterns` | bool | true | Enable default dangerous command blocking | -| `custom_deny_patterns` | array | [] | Custom deny patterns (regular expressions) | +| Config | Type | Default | Description | +| ---------------------- | ----- | ------- | ------------------------------------------ | +| `enable_deny_patterns` | bool | true | Enable default dangerous command blocking | +| `custom_deny_patterns` | array | [] | Custom deny patterns (regular expressions) | ### Functionality @@ -80,10 +81,7 @@ By default, PicoClaw blocks the following dangerous commands: "tools": { "exec": { "enable_deny_patterns": true, - "custom_deny_patterns": [ - "\\brm\\s+-r\\b", - "\\bkillall\\s+python" - ] + "custom_deny_patterns": ["\\brm\\s+-r\\b", "\\bkillall\\s+python"] } } } @@ -93,9 +91,84 @@ By default, PicoClaw blocks the following dangerous commands: The cron tool is used for scheduling periodic tasks. -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `exec_timeout_minutes` | int | 5 | Execution timeout in minutes, 0 means no limit | +| Config | Type | Default | Description | +| ---------------------- | ---- | ------- | ---------------------------------------------- | +| `exec_timeout_minutes` | int | 5 | Execution timeout in minutes, 0 means no limit | + +## MCP Tool + +The MCP tool enables integration with external Model Context Protocol servers. + +### Global Config + +| Config | Type | Default | Description | +| --------- | ------ | ------- | ----------------------------------- | +| `enabled` | bool | false | Enable MCP integration globally | +| `servers` | object | `{}` | Map of server name to server config | + +### Per-Server Config + +| Config | Type | Required | Description | +| ---------- | ------ | -------- | ------------------------------------------ | +| `enabled` | bool | yes | Enable this MCP server | +| `type` | string | no | Transport type: `stdio`, `sse`, `http` | +| `command` | string | stdio | Executable command for stdio transport | +| `args` | array | no | Command arguments for stdio transport | +| `env` | object | no | Environment variables for stdio process | +| `env_file` | string | no | Path to environment file for stdio process | +| `url` | string | sse/http | Endpoint URL for `sse`/`http` transport | +| `headers` | object | no | HTTP headers for `sse`/`http` transport | + +### Transport Behavior + +- If `type` is omitted, transport is auto-detected: + - `url` is set → `sse` + - `command` is set → `stdio` +- `http` and `sse` both use `url` + optional `headers`. +- `env` and `env_file` are only applied to `stdio` servers. + +### Configuration Examples + +#### 1) Stdio MCP server + +```json +{ + "tools": { + "mcp": { + "enabled": true, + "servers": { + "filesystem": { + "enabled": true, + "command": "npx", + "args": ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"] + } + } + } + } +} +``` + +#### 2) Remote SSE/HTTP MCP server + +```json +{ + "tools": { + "mcp": { + "enabled": true, + "servers": { + "remote-mcp": { + "enabled": true, + "type": "sse", + "url": "https://example.com/mcp", + "headers": { + "Authorization": "Bearer YOUR_TOKEN" + } + } + } + } + } +} +``` ## Skills Tool @@ -103,13 +176,13 @@ The skills tool configures skill discovery and installation via registries like ### Registries -| Config | Type | Default | Description | -|--------|------|---------|-------------| -| `registries.clawhub.enabled` | bool | true | Enable ClawHub registry | -| `registries.clawhub.base_url` | string | `https://clawhub.ai` | ClawHub base URL | -| `registries.clawhub.search_path` | string | `/api/v1/search` | Search API path | -| `registries.clawhub.skills_path` | string | `/api/v1/skills` | Skills API path | -| `registries.clawhub.download_path` | string | `/api/v1/download` | Download API path | +| Config | Type | Default | Description | +| ---------------------------------- | ------ | -------------------- | ----------------------- | +| `registries.clawhub.enabled` | bool | true | Enable ClawHub registry | +| `registries.clawhub.base_url` | string | `https://clawhub.ai` | ClawHub base URL | +| `registries.clawhub.search_path` | string | `/api/v1/search` | Search API path | +| `registries.clawhub.skills_path` | string | `/api/v1/skills` | Skills API path | +| `registries.clawhub.download_path` | string | `/api/v1/download` | Download API path | ### Configuration Example @@ -136,8 +209,10 @@ The skills tool configures skill discovery and installation via registries like All configuration options can be overridden via environment variables with the format `PICOCLAW_TOOLS_
_`: For example: + - `PICOCLAW_TOOLS_WEB_BRAVE_ENABLED=true` - `PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS=false` - `PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES=10` +- `PICOCLAW_TOOLS_MCP_ENABLED=true` -Note: Array-type environment variables are not currently supported and must be set via the config file. +Note: Nested map-style config (for example `tools.mcp.servers..*`) is configured in `config.json` rather than environment variables. From 16a3b96ddebe4b1d75db05a19c6f36f814823118 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 22 Feb 2026 15:06:57 +0800 Subject: [PATCH 38/43] fix(mcp): validate workspace before resolving relative env_file --- pkg/mcp/manager.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index b28ba4670..90f31f0c1 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -161,6 +161,17 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig // Resolve relative envFile paths relative to workspace if serverCfg.EnvFile != "" && !filepath.IsAbs(serverCfg.EnvFile) { + if workspace == "" { + err := fmt.Errorf("workspace path is empty while resolving relative envFile %q for server %s", serverCfg.EnvFile, name) + logger.ErrorCF("mcp", "Invalid MCP server configuration", + map[string]interface{}{ + "server": name, + "env_file": serverCfg.EnvFile, + "error": err.Error(), + }) + errs <- err + return + } serverCfg.EnvFile = filepath.Join(workspace, serverCfg.EnvFile) } From 4e330b297c99a3f5c1e44869b84028b8069ca433 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 22 Feb 2026 15:13:29 +0800 Subject: [PATCH 39/43] test(mcp): add manager behavior and lifecycle unit tests --- pkg/mcp/manager_test.go | 112 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go index 1c69e75f2..f9b8c07dd 100644 --- a/pkg/mcp/manager_test.go +++ b/pkg/mcp/manager_test.go @@ -1,9 +1,14 @@ package mcp import ( + "context" "os" "path/filepath" + "strings" "testing" + + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sipeed/picoclaw/pkg/config" ) func TestLoadEnvFile(t *testing.T) { @@ -180,3 +185,110 @@ SHARED_VAR=from_file` t.Errorf("Expected NEW_VAR=from_config, got %s", merged["NEW_VAR"]) } } + +func TestLoadFromMCPConfig_EmptyWorkspaceWithRelativeEnvFile(t *testing.T) { + mgr := NewManager() + + mcpCfg := config.MCPConfig{ + Enabled: true, + Servers: map[string]config.MCPServerConfig{ + "test-server": { + Enabled: true, + Command: "echo", + Args: []string{"ok"}, + EnvFile: ".env", + }, + }, + } + + err := mgr.LoadFromMCPConfig(context.Background(), mcpCfg, "") + if err == nil { + t.Fatal("expected error for relative env_file with empty workspace path, got nil") + } + + if !strings.Contains(err.Error(), "workspace path is empty") { + t.Fatalf("expected workspace path validation error, got: %v", err) + } +} + +func TestNewManager_InitialState(t *testing.T) { + mgr := NewManager() + if mgr == nil { + t.Fatal("expected manager instance, got nil") + } + if len(mgr.GetServers()) != 0 { + t.Fatalf("expected no servers on new manager, got %d", len(mgr.GetServers())) + } +} + +func TestLoadFromMCPConfig_DisabledOrEmptyServers(t *testing.T) { + mgr := NewManager() + + err := mgr.LoadFromMCPConfig(context.Background(), config.MCPConfig{Enabled: false}, "/tmp") + if err != nil { + t.Fatalf("expected nil error when MCP disabled, got: %v", err) + } + + err = mgr.LoadFromMCPConfig(context.Background(), config.MCPConfig{Enabled: true}, "/tmp") + if err != nil { + t.Fatalf("expected nil error when no servers configured, got: %v", err) + } +} + +func TestGetServers_ReturnsCopy(t *testing.T) { + mgr := NewManager() + mgr.servers["s1"] = &ServerConnection{Name: "s1"} + + servers := mgr.GetServers() + delete(servers, "s1") + + if _, ok := mgr.GetServer("s1"); !ok { + t.Fatal("expected internal manager state to remain unchanged") + } +} + +func TestGetAllTools_FiltersEmptyTools(t *testing.T) { + mgr := NewManager() + mgr.servers["empty"] = &ServerConnection{Name: "empty", Tools: nil} + mgr.servers["with-tools"] = &ServerConnection{Name: "with-tools", Tools: []*sdkmcp.Tool{{}}} + + all := mgr.GetAllTools() + if _, ok := all["empty"]; ok { + t.Fatal("expected server without tools to be excluded") + } + if _, ok := all["with-tools"]; !ok { + t.Fatal("expected server with tools to be included") + } +} + +func TestCallTool_ErrorsForClosedOrMissingServer(t *testing.T) { + t.Run("manager closed", func(t *testing.T) { + mgr := NewManager() + mgr.closed = true + + _, err := mgr.CallTool(context.Background(), "s1", "tool", nil) + if err == nil || !strings.Contains(err.Error(), "manager is closed") { + t.Fatalf("expected manager closed error, got: %v", err) + } + }) + + t.Run("server missing", func(t *testing.T) { + mgr := NewManager() + + _, err := mgr.CallTool(context.Background(), "missing", "tool", nil) + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("expected server not found error, got: %v", err) + } + }) +} + +func TestClose_IdempotentOnEmptyManager(t *testing.T) { + mgr := NewManager() + + if err := mgr.Close(); err != nil { + t.Fatalf("first close should succeed, got: %v", err) + } + if err := mgr.Close(); err != nil { + t.Fatalf("second close should be idempotent, got: %v", err) + } +} From 077d7c8d9b04d84c7c8072f3f07bc7a6f5e24a50 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 1 Mar 2026 08:53:13 +0800 Subject: [PATCH 40/43] chore: fix lint issues in mcp and agent packages - Fix gci import ordering in manager.go, manager_test.go - Fix gofmt formatting in loop.go, manager.go, mcp_tool.go, mcp_tool_test.go - Fix gofumpt formatting in manager_test.go - Fix golines line length issues in manager.go, mcp_tool_test.go - Fix wastedassign: replace redundant zero-value init with var declaration in loop.go --- pkg/agent/loop.go | 124 +++++++++++++++++++++++++++---------- pkg/mcp/manager.go | 64 ++++++++++++------- pkg/mcp/manager_test.go | 10 ++- pkg/tools/mcp_tool.go | 32 +++++----- pkg/tools/mcp_tool_test.go | 50 ++++++++------- 5 files changed, 186 insertions(+), 94 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 12ca35148..338ce9c58 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -58,7 +58,11 @@ type processOptions struct { const defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json." -func NewAgentLoop(cfg *config.Config, msgBus *bus.MessageBus, provider providers.LLMProvider) *AgentLoop { +func NewAgentLoop( + cfg *config.Config, + msgBus *bus.MessageBus, + provider providers.LLMProvider, +) *AgentLoop { registry := NewAgentRegistry(cfg, provider) // Register shared tools to all agents @@ -166,7 +170,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { if al.cfg.Tools.MCP.Enabled { mcpManager := mcp.NewManager() defaultAgent := al.registry.GetDefaultAgent() - workspacePath := "" + var workspacePath string if defaultAgent != nil && defaultAgent.Workspace != "" { workspacePath = defaultAgent.Workspace } else { @@ -175,7 +179,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { if err := mcpManager.LoadFromMCPConfig(ctx, al.cfg.Tools.MCP, workspacePath); err != nil { logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", - map[string]interface{}{ + map[string]any{ "error": err.Error(), }) } else { @@ -183,7 +187,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { defer func() { if err := mcpManager.Close(); err != nil { logger.ErrorCF("agent", "Failed to close MCP manager", - map[string]interface{}{ + map[string]any{ "error": err.Error(), }) } @@ -208,7 +212,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { agent.Tools.Register(mcpTool) totalRegistrations++ logger.DebugCF("agent", "Registered MCP tool", - map[string]interface{}{ + map[string]any{ "agent_id": agentID, "server": serverName, "tool": tool.Name, @@ -218,7 +222,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { } } logger.InfoCF("agent", "MCP tools registered successfully", - map[string]interface{}{ + map[string]any{ "server_count": len(servers), "unique_tools": uniqueTools, "total_registrations": totalRegistrations, @@ -367,7 +371,10 @@ func (al *AgentLoop) RecordLastChatID(chatID string) error { return al.state.SetLastChatID(chatID) } -func (al *AgentLoop) ProcessDirect(ctx context.Context, content, sessionKey string) (string, error) { +func (al *AgentLoop) ProcessDirect( + ctx context.Context, + content, sessionKey string, +) (string, error) { return al.ProcessDirectWithChannel(ctx, content, sessionKey, "cli", "direct") } @@ -388,7 +395,10 @@ func (al *AgentLoop) ProcessDirectWithChannel( // ProcessHeartbeat processes a heartbeat request without session history. // Each heartbeat is independent and doesn't accumulate context. -func (al *AgentLoop) ProcessHeartbeat(ctx context.Context, content, channel, chatID string) (string, error) { +func (al *AgentLoop) ProcessHeartbeat( + ctx context.Context, + content, channel, chatID string, +) (string, error) { agent := al.registry.GetDefaultAgent() if agent == nil { return "", fmt.Errorf("no default agent for heartbeat") @@ -413,13 +423,16 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) } else { logContent = utils.Truncate(msg.Content, 80) } - logger.InfoCF("agent", fmt.Sprintf("Processing message from %s:%s: %s", msg.Channel, msg.SenderID, logContent), + logger.InfoCF( + "agent", + fmt.Sprintf("Processing message from %s:%s: %s", msg.Channel, msg.SenderID, logContent), map[string]any{ "channel": msg.Channel, "chat_id": msg.ChatID, "sender_id": msg.SenderID, "session_key": msg.SessionKey, - }) + }, + ) // Route system messages to processSystemMessage if msg.Channel == "system" { @@ -480,9 +493,15 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) }) } -func (al *AgentLoop) processSystemMessage(ctx context.Context, msg bus.InboundMessage) (string, error) { +func (al *AgentLoop) processSystemMessage( + ctx context.Context, + msg bus.InboundMessage, +) (string, error) { if msg.Channel != "system" { - return "", fmt.Errorf("processSystemMessage called with non-system message channel: %s", msg.Channel) + return "", fmt.Errorf( + "processSystemMessage called with non-system message channel: %s", + msg.Channel, + ) } logger.InfoCF("agent", "Processing system message", @@ -540,14 +559,22 @@ func (al *AgentLoop) processSystemMessage(ctx context.Context, msg bus.InboundMe } // runAgentLoop is the core message processing logic. -func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opts processOptions) (string, error) { +func (al *AgentLoop) runAgentLoop( + ctx context.Context, + agent *AgentInstance, + opts processOptions, +) (string, error) { // 0. Record last channel for heartbeat notifications (skip internal channels) if opts.Channel != "" && opts.ChatID != "" { // Don't record internal channels (cli, system, subagent) if !constants.IsInternalChannel(opts.Channel) { channelKey := fmt.Sprintf("%s:%s", opts.Channel, opts.ChatID) if err := al.RecordLastChannel(channelKey); err != nil { - logger.WarnCF("agent", "Failed to record last channel", map[string]any{"error": err.Error()}) + logger.WarnCF( + "agent", + "Failed to record last channel", + map[string]any{"error": err.Error()}, + ) } } } @@ -629,7 +656,10 @@ func (al *AgentLoop) targetReasoningChannelID(channelName string) (chatID string return "" } -func (al *AgentLoop) handleReasoning(ctx context.Context, reasoningContent, channelName, channelID string) { +func (al *AgentLoop) handleReasoning( + ctx context.Context, + reasoningContent, channelName, channelID string, +) { if reasoningContent == "" || channelName == "" || channelID == "" { return } @@ -697,22 +727,33 @@ func (al *AgentLoop) runLLMIteration( callLLM := func() (*providers.LLMResponse, error) { if len(agent.Candidates) > 1 && al.fallback != nil { - fbResult, fbErr := al.fallback.Execute(ctx, agent.Candidates, + fbResult, fbErr := al.fallback.Execute( + ctx, + agent.Candidates, func(ctx context.Context, provider, model string) (*providers.LLMResponse, error) { - return agent.Provider.Chat(ctx, messages, providerToolDefs, model, map[string]any{ - "max_tokens": agent.MaxTokens, - "temperature": agent.Temperature, - "prompt_cache_key": agent.ID, - }) + return agent.Provider.Chat( + ctx, + messages, + providerToolDefs, + model, + map[string]any{ + "max_tokens": agent.MaxTokens, + "temperature": agent.Temperature, + "prompt_cache_key": agent.ID, + }, + ) }, ) if fbErr != nil { return nil, fbErr } if fbResult.Provider != "" && len(fbResult.Attempts) > 0 { - logger.InfoCF("agent", fmt.Sprintf("Fallback: succeeded with %s/%s after %d attempts", - fbResult.Provider, fbResult.Model, len(fbResult.Attempts)+1), - map[string]any{"agent_id": agent.ID, "iteration": iteration}) + logger.InfoCF( + "agent", + fmt.Sprintf("Fallback: succeeded with %s/%s after %d attempts", + fbResult.Provider, fbResult.Model, len(fbResult.Attempts)+1), + map[string]any{"agent_id": agent.ID, "iteration": iteration}, + ) } return fbResult.Response, nil } @@ -738,10 +779,14 @@ func (al *AgentLoop) runLLMIteration( strings.Contains(errMsg, "length") if isContextError && retry < maxRetries { - logger.WarnCF("agent", "Context window error detected, attempting compression", map[string]any{ - "error": err.Error(), - "retry": retry, - }) + logger.WarnCF( + "agent", + "Context window error detected, attempting compression", + map[string]any{ + "error": err.Error(), + "retry": retry, + }, + ) if retry == 0 && !constants.IsInternalChannel(opts.Channel) { al.bus.PublishOutbound(ctx, bus.OutboundMessage{ @@ -773,7 +818,12 @@ func (al *AgentLoop) runLLMIteration( return "", iteration, fmt.Errorf("LLM call failed after retries: %w", err) } - go al.handleReasoning(ctx, response.Reasoning, opts.Channel, al.targetReasoningChannelID(opts.Channel)) + go al.handleReasoning( + ctx, + response.Reasoning, + opts.Channel, + al.targetReasoningChannelID(opts.Channel), + ) logger.DebugCF("agent", "LLM response", map[string]any{ @@ -1075,7 +1125,11 @@ func formatMessagesForLog(messages []providers.Message) string { for _, tc := range msg.ToolCalls { fmt.Fprintf(&sb, " - ID: %s, Type: %s, Name: %s\n", tc.ID, tc.Type, tc.Name) if tc.Function != nil { - fmt.Fprintf(&sb, " Arguments: %s\n", utils.Truncate(tc.Function.Arguments, 200)) + fmt.Fprintf( + &sb, + " Arguments: %s\n", + utils.Truncate(tc.Function.Arguments, 200), + ) } } } @@ -1104,7 +1158,11 @@ func formatToolsForLog(toolDefs []providers.ToolDefinition) string { fmt.Fprintf(&sb, " [%d] Type: %s, Name: %s\n", i, tool.Type, tool.Function.Name) fmt.Fprintf(&sb, " Description: %s\n", tool.Function.Description) if len(tool.Function.Parameters) > 0 { - fmt.Fprintf(&sb, " Parameters: %s\n", utils.Truncate(fmt.Sprintf("%v", tool.Function.Parameters), 200)) + fmt.Fprintf( + &sb, + " Parameters: %s\n", + utils.Truncate(fmt.Sprintf("%v", tool.Function.Parameters), 200), + ) } } sb.WriteString("]") @@ -1201,7 +1259,9 @@ func (al *AgentLoop) summarizeBatch( existingSummary string, ) (string, error) { var sb strings.Builder - sb.WriteString("Provide a concise summary of this conversation segment, preserving core context and key points.\n") + sb.WriteString( + "Provide a concise summary of this conversation segment, preserving core context and key points.\n", + ) if existingSummary != "" { sb.WriteString("Existing context: ") sb.WriteString(existingSummary) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 90f31f0c1..e79c9d3f8 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -13,6 +13,7 @@ import ( "sync" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/logger" ) @@ -125,7 +126,11 @@ func (m *Manager) LoadFromConfig(ctx context.Context, cfg *config.Config) error // LoadFromMCPConfig loads MCP servers from MCP configuration and workspace path. // This is the minimal dependency version that doesn't require the full Config object. -func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig, workspacePath string) error { +func (m *Manager) LoadFromMCPConfig( + ctx context.Context, + mcpCfg config.MCPConfig, + workspacePath string, +) error { if !mcpCfg.Enabled { logger.InfoCF("mcp", "MCP integration is disabled", nil) return nil @@ -137,7 +142,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig } logger.InfoCF("mcp", "Initializing MCP servers", - map[string]interface{}{ + map[string]any{ "count": len(mcpCfg.Servers), }) @@ -148,7 +153,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig for name, serverCfg := range mcpCfg.Servers { if !serverCfg.Enabled { logger.DebugCF("mcp", "Skipping disabled server", - map[string]interface{}{ + map[string]any{ "server": name, }) continue @@ -162,9 +167,13 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig // Resolve relative envFile paths relative to workspace if serverCfg.EnvFile != "" && !filepath.IsAbs(serverCfg.EnvFile) { if workspace == "" { - err := fmt.Errorf("workspace path is empty while resolving relative envFile %q for server %s", serverCfg.EnvFile, name) + err := fmt.Errorf( + "workspace path is empty while resolving relative envFile %q for server %s", + serverCfg.EnvFile, + name, + ) logger.ErrorCF("mcp", "Invalid MCP server configuration", - map[string]interface{}{ + map[string]any{ "server": name, "env_file": serverCfg.EnvFile, "error": err.Error(), @@ -177,7 +186,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig if err := m.ConnectServer(ctx, name, serverCfg); err != nil { logger.ErrorCF("mcp", "Failed to connect to MCP server", - map[string]interface{}{ + map[string]any{ "server": name, "error": err.Error(), }) @@ -200,7 +209,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig // If all enabled servers failed to connect, return aggregated error if enabledCount > 0 && connectedCount == 0 { logger.ErrorCF("mcp", "All MCP servers failed to connect", - map[string]interface{}{ + map[string]any{ "failed": len(allErrors), "total": enabledCount, }) @@ -209,7 +218,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig if len(allErrors) > 0 { logger.WarnCF("mcp", "Some MCP servers failed to connect", - map[string]interface{}{ + map[string]any{ "failed": len(allErrors), "connected": connectedCount, "total": enabledCount, @@ -218,7 +227,7 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig } logger.InfoCF("mcp", "MCP server initialization complete", - map[string]interface{}{ + map[string]any{ "connected": connectedCount, "total": enabledCount, }) @@ -227,9 +236,13 @@ func (m *Manager) LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig } // ConnectServer connects to a single MCP server -func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCPServerConfig) error { +func (m *Manager) ConnectServer( + ctx context.Context, + name string, + cfg config.MCPServerConfig, +) error { logger.InfoCF("mcp", "Connecting to MCP server", - map[string]interface{}{ + map[string]any{ "server": name, "command": cfg.Command, "args": cfg.Args, @@ -263,7 +276,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP return fmt.Errorf("URL is required for SSE/HTTP transport") } logger.DebugCF("mcp", "Using SSE/HTTP transport", - map[string]interface{}{ + map[string]any{ "server": name, "url": cfg.URL, }) @@ -282,7 +295,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP }, } logger.DebugCF("mcp", "Added custom HTTP headers", - map[string]interface{}{ + map[string]any{ "server": name, "header_count": len(cfg.Headers), }) @@ -294,7 +307,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP return fmt.Errorf("command is required for stdio transport") } logger.DebugCF("mcp", "Using stdio transport", - map[string]interface{}{ + map[string]any{ "server": name, "command": cfg.Command, }) @@ -322,7 +335,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP envMap[k] = v } logger.DebugCF("mcp", "Loaded environment variables from file", - map[string]interface{}{ + map[string]any{ "server": name, "envFile": cfg.EnvFile, "var_count": len(envVars), @@ -343,7 +356,10 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP transport = &mcp.CommandTransport{Command: cmd} default: - return fmt.Errorf("unsupported transport type: %s (supported: stdio, sse, http)", transportType) + return fmt.Errorf( + "unsupported transport type: %s (supported: stdio, sse, http)", + transportType, + ) } // Connect to server @@ -355,7 +371,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP // Get server info initResult := session.InitializeResult() logger.InfoCF("mcp", "Connected to MCP server", - map[string]interface{}{ + map[string]any{ "server": name, "serverName": initResult.ServerInfo.Name, "serverVersion": initResult.ServerInfo.Version, @@ -368,7 +384,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP for tool, err := range session.Tools(ctx, nil) { if err != nil { logger.WarnCF("mcp", "Error listing tool", - map[string]interface{}{ + map[string]any{ "server": name, "error": err.Error(), }) @@ -378,7 +394,7 @@ func (m *Manager) ConnectServer(ctx context.Context, name string, cfg config.MCP } logger.InfoCF("mcp", "Listed tools from MCP server", - map[string]interface{}{ + map[string]any{ "server": name, "toolCount": len(tools), }) @@ -419,7 +435,11 @@ func (m *Manager) GetServer(name string) (*ServerConnection, bool) { } // CallTool calls a tool on a specific server -func (m *Manager) CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { +func (m *Manager) CallTool( + ctx context.Context, + serverName, toolName string, + arguments map[string]any, +) (*mcp.CallToolResult, error) { m.mu.RLock() if m.closed { m.mu.RUnlock() @@ -466,7 +486,7 @@ func (m *Manager) Close() error { defer m.mu.Unlock() logger.InfoCF("mcp", "Closing all MCP server connections", - map[string]interface{}{ + map[string]any{ "count": len(m.servers), }) @@ -474,7 +494,7 @@ func (m *Manager) Close() error { for name, conn := range m.servers { if err := conn.Session.Close(); err != nil { logger.ErrorCF("mcp", "Failed to close server connection", - map[string]interface{}{ + map[string]any{ "server": name, "error": err.Error(), }) diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go index f9b8c07dd..6dd71a3c2 100644 --- a/pkg/mcp/manager_test.go +++ b/pkg/mcp/manager_test.go @@ -8,6 +8,7 @@ import ( "testing" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sipeed/picoclaw/pkg/config" ) @@ -95,7 +96,7 @@ PORT =8080`, tmpDir := t.TempDir() envFile := filepath.Join(tmpDir, ".env") - if err := os.WriteFile(envFile, []byte(tt.content), 0644); err != nil { + if err := os.WriteFile(envFile, []byte(tt.content), 0o644); err != nil { t.Fatalf("Failed to create test file: %v", err) } @@ -144,7 +145,7 @@ func TestEnvFilePriority(t *testing.T) { DATABASE_URL=from_file SHARED_VAR=from_file` - if err := os.WriteFile(envFile, []byte(envContent), 0644); err != nil { + if err := os.WriteFile(envFile, []byte(envContent), 0o644); err != nil { t.Fatalf("Failed to create .env file: %v", err) } @@ -176,7 +177,10 @@ SHARED_VAR=from_file` // Verify priority: config.Env should override envFile if merged["SHARED_VAR"] != "from_config" { - t.Errorf("Expected SHARED_VAR=from_config (config should override file), got %s", merged["SHARED_VAR"]) + t.Errorf( + "Expected SHARED_VAR=from_config (config should override file), got %s", + merged["SHARED_VAR"], + ) } if merged["API_KEY"] != "from_file" { t.Errorf("Expected API_KEY=from_file, got %s", merged["API_KEY"]) diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index 6bd3d75e3..69615b4bc 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -12,7 +12,11 @@ import ( // MCPManager defines the interface for MCP manager operations // This allows for easier testing with mock implementations type MCPManager interface { - CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) + CallTool( + ctx context.Context, + serverName, toolName string, + arguments map[string]any, + ) (*mcp.CallToolResult, error) } // MCPTool wraps an MCP tool to implement the Tool interface @@ -48,21 +52,21 @@ func (t *MCPTool) Description() string { } // Parameters returns the tool parameters schema -func (t *MCPTool) Parameters() map[string]interface{} { +func (t *MCPTool) Parameters() map[string]any { // The InputSchema is already a JSON Schema object schema := t.tool.InputSchema // Handle nil schema if schema == nil { - return map[string]interface{}{ + return map[string]any{ "type": "object", - "properties": map[string]interface{}{}, + "properties": map[string]any{}, "required": []string{}, } } // Try direct conversion first (fast path) - if schemaMap, ok := schema.(map[string]interface{}); ok { + if schemaMap, ok := schema.(map[string]any); ok { return schemaMap } @@ -75,14 +79,14 @@ func (t *MCPTool) Parameters() map[string]interface{} { } if jsonData != nil { - var result map[string]interface{} + var result map[string]any if err := json.Unmarshal(jsonData, &result); err == nil { return result } // Fallback on error - return map[string]interface{}{ + return map[string]any{ "type": "object", - "properties": map[string]interface{}{}, + "properties": map[string]any{}, "required": []string{}, } } @@ -92,19 +96,19 @@ func (t *MCPTool) Parameters() map[string]interface{} { jsonData, err = json.Marshal(schema) if err != nil { // Fallback to empty schema if marshaling fails - return map[string]interface{}{ + return map[string]any{ "type": "object", - "properties": map[string]interface{}{}, + "properties": map[string]any{}, "required": []string{}, } } - var result map[string]interface{} + var result map[string]any if err := json.Unmarshal(jsonData, &result); err != nil { // Fallback to empty schema if unmarshaling fails - return map[string]interface{}{ + return map[string]any{ "type": "object", - "properties": map[string]interface{}{}, + "properties": map[string]any{}, "required": []string{}, } } @@ -113,7 +117,7 @@ func (t *MCPTool) Parameters() map[string]interface{} { } // Execute executes the MCP tool -func (t *MCPTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult { +func (t *MCPTool) Execute(ctx context.Context, args map[string]any) *ToolResult { result, err := t.manager.CallTool(ctx, t.serverName, t.tool.Name, args) if err != nil { return ErrorResult(fmt.Sprintf("MCP tool execution failed: %v", err)).WithError(err) diff --git a/pkg/tools/mcp_tool_test.go b/pkg/tools/mcp_tool_test.go index 580abc2ba..95bb0f992 100644 --- a/pkg/tools/mcp_tool_test.go +++ b/pkg/tools/mcp_tool_test.go @@ -11,10 +11,14 @@ import ( // MockMCPManager is a mock implementation of MCPManager interface for testing type MockMCPManager struct { - callToolFunc func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) + callToolFunc func(ctx context.Context, serverName, toolName string, arguments map[string]any) (*mcp.CallToolResult, error) } -func (m *MockMCPManager) CallTool(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { +func (m *MockMCPManager) CallTool( + ctx context.Context, + serverName, toolName string, + arguments map[string]any, +) (*mcp.CallToolResult, error) { if m.callToolFunc != nil { return m.callToolFunc(ctx, serverName, toolName, arguments) } @@ -32,10 +36,10 @@ func TestNewMCPTool(t *testing.T) { tool := &mcp.Tool{ Name: "test_tool", Description: "A test tool", - InputSchema: map[string]interface{}{ + InputSchema: map[string]any{ "type": "object", - "properties": map[string]interface{}{ - "input": map[string]interface{}{ + "properties": map[string]any{ + "input": map[string]any{ "type": "string", "description": "Test input", }, @@ -142,17 +146,17 @@ func TestMCPTool_Description(t *testing.T) { func TestMCPTool_Parameters(t *testing.T) { tests := []struct { name string - inputSchema interface{} + inputSchema any expectType string checkProperty string expectProperty bool }{ { name: "map schema", - inputSchema: map[string]interface{}{ + inputSchema: map[string]any{ "type": "object", - "properties": map[string]interface{}{ - "query": map[string]interface{}{ + "properties": map[string]any{ + "query": map[string]any{ "type": "string", "description": "Search query", }, @@ -212,7 +216,7 @@ func TestMCPTool_Parameters(t *testing.T) { // Check if property exists when expected if tt.checkProperty != "" { - properties, ok := params["properties"].(map[string]interface{}) + properties, ok := params["properties"].(map[string]any) if !ok && tt.expectProperty { t.Errorf("Expected properties to be a map") return @@ -232,7 +236,7 @@ func TestMCPTool_Parameters(t *testing.T) { // TestMCPTool_Execute_Success tests successful tool execution func TestMCPTool_Execute_Success(t *testing.T) { manager := &MockMCPManager{ - callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]any) (*mcp.CallToolResult, error) { // Verify correct parameters passed if serverName != "github" { t.Errorf("Expected serverName 'github', got '%s'", serverName) @@ -257,7 +261,7 @@ func TestMCPTool_Execute_Success(t *testing.T) { mcpTool := NewMCPTool(manager, "github", tool) ctx := context.Background() - args := map[string]interface{}{ + args := map[string]any{ "query": "golang mcp", } @@ -277,7 +281,7 @@ func TestMCPTool_Execute_Success(t *testing.T) { // TestMCPTool_Execute_ManagerError tests execution when manager returns error func TestMCPTool_Execute_ManagerError(t *testing.T) { manager := &MockMCPManager{ - callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]any) (*mcp.CallToolResult, error) { return nil, fmt.Errorf("connection failed") }, } @@ -286,7 +290,7 @@ func TestMCPTool_Execute_ManagerError(t *testing.T) { mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() - result := mcpTool.Execute(ctx, map[string]interface{}{}) + result := mcpTool.Execute(ctx, map[string]any{}) if result == nil { t.Fatal("Result should not be nil") @@ -305,7 +309,7 @@ func TestMCPTool_Execute_ManagerError(t *testing.T) { // TestMCPTool_Execute_ServerError tests execution when server returns error func TestMCPTool_Execute_ServerError(t *testing.T) { manager := &MockMCPManager{ - callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]any) (*mcp.CallToolResult, error) { return &mcp.CallToolResult{ Content: []mcp.Content{ &mcp.TextContent{Text: "Invalid API key"}, @@ -319,7 +323,7 @@ func TestMCPTool_Execute_ServerError(t *testing.T) { mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() - result := mcpTool.Execute(ctx, map[string]interface{}{}) + result := mcpTool.Execute(ctx, map[string]any{}) if result == nil { t.Fatal("Result should not be nil") @@ -338,7 +342,7 @@ func TestMCPTool_Execute_ServerError(t *testing.T) { // TestMCPTool_Execute_MultipleContent tests execution with multiple content items func TestMCPTool_Execute_MultipleContent(t *testing.T) { manager := &MockMCPManager{ - callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]interface{}) (*mcp.CallToolResult, error) { + callToolFunc: func(ctx context.Context, serverName, toolName string, arguments map[string]any) (*mcp.CallToolResult, error) { return &mcp.CallToolResult{ Content: []mcp.Content{ &mcp.TextContent{Text: "First line"}, @@ -354,7 +358,7 @@ func TestMCPTool_Execute_MultipleContent(t *testing.T) { mcpTool := NewMCPTool(manager, "test_server", tool) ctx := context.Background() - result := mcpTool.Execute(ctx, map[string]interface{}{}) + result := mcpTool.Execute(ctx, map[string]any{}) if result.IsError { t.Errorf("Expected no error, got: %s", result.ForLLM) @@ -448,10 +452,10 @@ func TestMCPTool_InterfaceCompliance(t *testing.T) { // TestMCPTool_Parameters_MapSchema tests schema that's already a map func TestMCPTool_Parameters_MapSchema(t *testing.T) { manager := &MockMCPManager{} - schema := map[string]interface{}{ + schema := map[string]any{ "type": "object", - "properties": map[string]interface{}{ - "name": map[string]interface{}{ + "properties": map[string]any{ + "name": map[string]any{ "type": "string", "description": "The name parameter", }, @@ -472,12 +476,12 @@ func TestMCPTool_Parameters_MapSchema(t *testing.T) { t.Errorf("Expected type 'object', got '%v'", params["type"]) } - props, ok := params["properties"].(map[string]interface{}) + props, ok := params["properties"].(map[string]any) if !ok { t.Error("Properties should be a map") } - nameParam, ok := props["name"].(map[string]interface{}) + nameParam, ok := props["name"].(map[string]any) if !ok { t.Error("Name parameter should exist") } From ef738f478741baeba5c8fe77d69a8b76a981a935 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 1 Mar 2026 10:56:02 +0800 Subject: [PATCH 41/43] fix: address PR review feedback for MCP tools support - Avoid logging sensitive cfg.Args in ConnectServer; log args_count instead - Sanitize server/tool name components in MCPTool.Name() to ensure valid identifiers for downstream providers (lowercase, [a-z0-9_-] only) - Add slack as 5th MCP server example in config.example.json - Move Dockerfile.full and docker-compose.full.yml into docker/ directory for consistency with existing docker/Dockerfile and docker/docker-compose.yml - Fix all Makefile docker-* targets to reference correct compose file paths - Fix docker/docker-compose.full.yml build context (.. ) and volume paths - Fix scripts/test-docker-mcp.sh compose file path and replace cowsay test with actual @modelcontextprotocol/server-filesystem MCP server test --- Makefile | 16 ++--- config/config.example.json | 12 ++++ Dockerfile.full => docker/Dockerfile.full | 0 .../docker-compose.full.yml | 16 ++--- pkg/mcp/manager.go | 6 +- pkg/tools/mcp_tool.go | 61 ++++++++++++++++++- scripts/test-docker-mcp.sh | 6 +- 7 files changed, 93 insertions(+), 24 deletions(-) rename Dockerfile.full => docker/Dockerfile.full (100%) rename docker-compose.full.yml => docker/docker-compose.full.yml (78%) diff --git a/Makefile b/Makefile index e2ef23e72..4da92a92b 100644 --- a/Makefile +++ b/Makefile @@ -207,12 +207,12 @@ run: build ## docker-build: Build Docker image (minimal Alpine-based) docker-build: @echo "Building minimal Docker image (Alpine-based)..." - docker compose build picoclaw-agent picoclaw-gateway + docker compose -f docker/docker-compose.yml build picoclaw-agent picoclaw-gateway ## docker-build-full: Build Docker image with full MCP support (Node.js 24) docker-build-full: @echo "Building full-featured Docker image (Node.js 24)..." - docker compose -f docker-compose.full.yml build picoclaw-agent picoclaw-gateway + docker compose -f docker/docker-compose.full.yml build picoclaw-agent picoclaw-gateway ## docker-test: Test MCP tools in Docker container docker-test: @@ -222,24 +222,24 @@ docker-test: ## docker-run: Run picoclaw gateway in Docker (Alpine-based) docker-run: - docker compose --profile gateway up + docker compose -f docker/docker-compose.yml --profile gateway up ## docker-run-full: Run picoclaw gateway in Docker (full-featured) docker-run-full: - docker compose -f docker-compose.full.yml --profile gateway up + docker compose -f docker/docker-compose.full.yml --profile gateway up ## docker-run-agent: Run picoclaw agent in Docker (interactive, Alpine-based) docker-run-agent: - docker compose run --rm picoclaw-agent + docker compose -f docker/docker-compose.yml run --rm picoclaw-agent ## docker-run-agent-full: Run picoclaw agent in Docker (interactive, full-featured) docker-run-agent-full: - docker compose -f docker-compose.full.yml run --rm picoclaw-agent + docker compose -f docker/docker-compose.full.yml run --rm picoclaw-agent ## docker-clean: Clean Docker images and volumes docker-clean: - docker compose down -v - docker compose -f docker-compose.full.yml down -v + docker compose -f docker/docker-compose.yml down -v + docker compose -f docker/docker-compose.full.yml down -v docker rmi picoclaw:latest picoclaw:full 2>/dev/null || true ## help: Show this help message diff --git a/config/config.example.json b/config/config.example.json index 762bb90aa..c4f901caf 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -279,6 +279,18 @@ "@modelcontextprotocol/server-postgres", "postgresql://user:password@localhost/dbname" ] + }, + "slack": { + "enabled": false, + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-slack" + ], + "env": { + "SLACK_BOT_TOKEN": "YOUR_SLACK_BOT_TOKEN", + "SLACK_TEAM_ID": "YOUR_SLACK_TEAM_ID" + } } } }, diff --git a/Dockerfile.full b/docker/Dockerfile.full similarity index 100% rename from Dockerfile.full rename to docker/Dockerfile.full diff --git a/docker-compose.full.yml b/docker/docker-compose.full.yml similarity index 78% rename from docker-compose.full.yml rename to docker/docker-compose.full.yml index ff2694173..6f34448c4 100644 --- a/docker-compose.full.yml +++ b/docker/docker-compose.full.yml @@ -1,17 +1,17 @@ services: # ───────────────────────────────────────────── # PicoClaw Agent (one-shot query) - Full MCP Support - # docker compose -f docker-compose.full.yml run --rm picoclaw-agent -m "Hello" + # docker compose -f docker/docker-compose.full.yml run --rm picoclaw-agent -m "Hello" # ───────────────────────────────────────────── picoclaw-agent: build: - context: . - dockerfile: Dockerfile.full + context: .. + dockerfile: docker/Dockerfile.full container_name: picoclaw-agent-full profiles: - agent volumes: - - ./config/config.json:/root/.picoclaw/config.json:ro + - ../config/config.json:/root/.picoclaw/config.json:ro - picoclaw-workspace:/root/.picoclaw/workspace - picoclaw-npm-cache:/root/.npm # npm cache for faster MCP server installs entrypoint: ["picoclaw", "agent"] @@ -20,19 +20,19 @@ services: # ───────────────────────────────────────────── # PicoClaw Gateway (Long-running Bot) - Full MCP Support - # docker compose -f docker-compose.full.yml --profile gateway up + # docker compose -f docker/docker-compose.full.yml --profile gateway up # ───────────────────────────────────────────── picoclaw-gateway: build: - context: . - dockerfile: Dockerfile.full + context: .. + dockerfile: docker/Dockerfile.full container_name: picoclaw-gateway-full restart: unless-stopped profiles: - gateway volumes: # Configuration file - - ./config/config.json:/root/.picoclaw/config.json:ro + - ../config/config.json:/root/.picoclaw/config.json:ro # Persistent workspace (sessions, memory, logs) - picoclaw-workspace:/root/.picoclaw/workspace # NPM cache for faster MCP server installs diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index e79c9d3f8..8b6d6d9aa 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -243,9 +243,9 @@ func (m *Manager) ConnectServer( ) error { logger.InfoCF("mcp", "Connecting to MCP server", map[string]any{ - "server": name, - "command": cfg.Command, - "args": cfg.Args, + "server": name, + "command": cfg.Command, + "args_count": len(cfg.Args), }) // Create client diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index 69615b4bc..1bdf248f7 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -35,10 +35,67 @@ func NewMCPTool(manager MCPManager, serverName string, tool *mcp.Tool) *MCPTool } } +// sanitizeIdentifierComponent normalizes a string so it can be safely used +// as part of a tool/function identifier for downstream providers. +// It: +// - lowercases the string +// - replaces any character not in [a-z0-9_-] with '_' +// - collapses multiple consecutive '_' into a single '_' +// - trims leading/trailing '_' +// - falls back to "unnamed" if the result is empty +// - truncates overly long components to a reasonable length +func sanitizeIdentifierComponent(s string) string { + const maxLen = 64 + + s = strings.ToLower(s) + var b strings.Builder + b.Grow(len(s)) + + prevUnderscore := false + for _, r := range s { + isAllowed := (r >= 'a' && r <= 'z') || + (r >= '0' && r <= '9') || + r == '_' || r == '-' + + if !isAllowed { + // Normalize any disallowed character to '_' + if !prevUnderscore { + b.WriteRune('_') + prevUnderscore = true + } + continue + } + + if r == '_' { + if prevUnderscore { + continue + } + prevUnderscore = true + } else { + prevUnderscore = false + } + + b.WriteRune(r) + } + + result := strings.Trim(b.String(), "_") + if result == "" { + result = "unnamed" + } + + if len(result) > maxLen { + result = result[:maxLen] + } + + return result +} + // Name returns the tool name, prefixed with the server name func (t *MCPTool) Name() string { - // Prefix with server name to avoid conflicts - return fmt.Sprintf("mcp_%s_%s", t.serverName, t.tool.Name) + // Prefix with server name to avoid conflicts, and sanitize components + sanitizedServer := sanitizeIdentifierComponent(t.serverName) + sanitizedTool := sanitizeIdentifierComponent(t.tool.Name) + return fmt.Sprintf("mcp_%s_%s", sanitizedServer, sanitizedTool) } // Description returns the tool description diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index 5c4e5cb56..7ad8640f2 100755 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -3,7 +3,7 @@ set -e -COMPOSE_FILE="docker-compose.full.yml" +COMPOSE_FILE="docker/docker-compose.full.yml" SERVICE="picoclaw-agent" echo "🧪 Testing MCP tools in Docker container (full-featured image)..." @@ -38,8 +38,8 @@ echo "✅ Testing uv..." docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'uv --version' # Test MCP server installation (quick) -echo "✅ Testing MCP server install with npx..." -docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y cowsay "MCP works!"' +echo "✅ Testing @modelcontextprotocol/server-filesystem MCP server install with npx..." +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y @modelcontextprotocol/server-filesystem --help' echo "" echo "🎉 All MCP tools are working correctly!" From 0eec640c3763a4e3691dfc940348f71da4e324a4 Mon Sep 17 00:00:00 2001 From: yuchou87 Date: Sun, 1 Mar 2026 11:24:12 +0800 Subject: [PATCH 42/43] fix: correct MCP server install test in test-docker-mcp.sh server-filesystem does not support --help; use timeout + /dev/null stdin to verify installation and startup without hanging the test script --- scripts/test-docker-mcp.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index 7ad8640f2..4eb77dceb 100755 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -39,7 +39,7 @@ docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'uv --v # Test MCP server installation (quick) echo "✅ Testing @modelcontextprotocol/server-filesystem MCP server install with npx..." -docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c 'npx -y @modelcontextprotocol/server-filesystem --help' +docker compose -f "$COMPOSE_FILE" run --rm --entrypoint sh "$SERVICE" -c ' Date: Sun, 1 Mar 2026 12:00:26 +0800 Subject: [PATCH 43/43] fix: improve MCP tool name collision safety and registry overwrite warning - MCPTool.Name(): append FNV-32a hash of original (unsanitized) server+tool names whenever sanitization is lossy or total length exceeds 64 chars, ensuring names that differ only in disallowed characters remain distinct - ToolRegistry.Register(): emit warn log when a tool registration overwrites an existing tool with the same name, making collisions observable - scripts/test-docker-mcp.sh: switch shebang from #/bin/bash /Users/yuchou/Work/klook-calendar/klook-google-cal-sync/src/googlecalconversrv/bin/start.sh to # for portability on minimal distros and Nix environments --- pkg/tools/mcp_tool.go | 30 ++++++++++++++++++++++++++++-- pkg/tools/registry.go | 7 ++++++- scripts/test-docker-mcp.sh | 2 +- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go index 1bdf248f7..6e53cf354 100644 --- a/pkg/tools/mcp_tool.go +++ b/pkg/tools/mcp_tool.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "hash/fnv" "strings" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -90,12 +91,37 @@ func sanitizeIdentifierComponent(s string) string { return result } -// Name returns the tool name, prefixed with the server name +// Name returns the tool name, prefixed with the server name. +// The total length is capped at 64 characters (OpenAI-compatible API limit). +// A short hash of the original (unsanitized) server and tool names is appended +// whenever sanitization is lossy or the name is truncated, ensuring that two +// names which differ only in disallowed characters remain distinct after sanitization. func (t *MCPTool) Name() string { // Prefix with server name to avoid conflicts, and sanitize components sanitizedServer := sanitizeIdentifierComponent(t.serverName) sanitizedTool := sanitizeIdentifierComponent(t.tool.Name) - return fmt.Sprintf("mcp_%s_%s", sanitizedServer, sanitizedTool) + full := fmt.Sprintf("mcp_%s_%s", sanitizedServer, sanitizedTool) + + // Check if sanitization was lossless (only lowercasing, no char replacement/truncation) + lossless := strings.ToLower(t.serverName) == sanitizedServer && + strings.ToLower(t.tool.Name) == sanitizedTool + + const maxTotal = 64 + if lossless && len(full) <= maxTotal { + return full + } + + // Sanitization was lossy or name too long: append hash of the ORIGINAL names + // (not the sanitized names) so different originals always yield different hashes. + h := fnv.New32a() + _, _ = h.Write([]byte(t.serverName + "\x00" + t.tool.Name)) + suffix := fmt.Sprintf("%08x", h.Sum32()) // 8 chars + + base := full + if len(base) > maxTotal-9 { + base = strings.TrimRight(full[:maxTotal-9], "_") + } + return base + "_" + suffix } // Description returns the tool description diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go index d37a093a8..0ba983e02 100644 --- a/pkg/tools/registry.go +++ b/pkg/tools/registry.go @@ -25,7 +25,12 @@ func NewToolRegistry() *ToolRegistry { func (r *ToolRegistry) Register(tool Tool) { r.mu.Lock() defer r.mu.Unlock() - r.tools[tool.Name()] = tool + name := tool.Name() + if _, exists := r.tools[name]; exists { + logger.WarnCF("tools", "Tool registration overwrites existing tool", + map[string]any{"name": name}) + } + r.tools[name] = tool } func (r *ToolRegistry) Get(name string) (Tool, bool) { diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh index 4eb77dceb..9d582ffa0 100755 --- a/scripts/test-docker-mcp.sh +++ b/scripts/test-docker-mcp.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh # Test script for MCP tools in Docker (full-featured image) set -e