diff --git a/docs/cron.md b/docs/cron.md index 6483fa1370..b570d5df31 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -36,6 +36,8 @@ This is the default for the cron tool. When the job fires, PicoClaw sends the saved message back through the agent loop as a new agent turn. Use this for scheduled work that may need reasoning, tools, or a generated reply. +Because the saved `message` is replayed as a new user-style input, write it from the user's perspective or as direct instructions to the agent. Prefer wording such as `check the repo every hour and tell me if there is a new release` over third-person wording such as `check the repo and notify the user`. + ### `deliver: true` When the job fires, PicoClaw publishes the saved message directly to the target channel and recipient without agent processing. @@ -50,6 +52,23 @@ For command jobs, `deliver` is forced to `false` when the job is created. The sa The current CLI `picoclaw cron add` command does not expose a `command` flag. +## Writing Job Messages + +For normal cron jobs without `command`, the saved `payload.message` becomes the next input sent to the agent when the job fires. In practice, that means the job message should read like something the user would say to the agent. + +Recommended style: + +- Use first-person or direct-address wording such as `tell me`, `remind me`, `reply in Chinese`, `do not reply if nothing changed` +- Be explicit about the quiet case if needed, for example `If there is no update, do not reply` +- Avoid third-person wording such as `notify the user`, because the model may continue replying in third person + +Examples: + +```text +Good: Check gdsfactory/gdsfactory every hour. If there is a new release, tell me in Chinese and summarize the changes. If nothing changed, do not reply. +Bad: Check gdsfactory/gdsfactory every hour. If there is a new release, notify the user and summarize the changes. +``` + ## Config and Security Gates ### `tools.cron` diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index adee9244a0..69380b23a8 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -252,6 +252,8 @@ The cron tool is used for scheduling periodic tasks. | `allow_command` | bool | true | Allow command jobs without extra confirmation | | `exec_timeout_minutes` | int | 5 | Execution timeout in minutes, 0 means no limit | +For normal cron jobs without `command`, the saved `message` is later replayed into the agent loop as a new user-style message. Write it from the user's perspective, for example `check the repo every hour and tell me if there is a new release`, not `notify the user`. + For schedule types, execution modes (`deliver`, agent turn, and command jobs), persistence, and the current command-security gates, see [Scheduled Tasks and Cron Jobs](cron.md). ## MCP Tool diff --git a/docs/zh/tools_configuration.md b/docs/zh/tools_configuration.md index 63ac5000b3..f6ccd5605f 100644 --- a/docs/zh/tools_configuration.md +++ b/docs/zh/tools_configuration.md @@ -234,6 +234,8 @@ Cron 工具用于调度周期性任务。 | `exec_timeout_minutes` | int | 5 | 执行超时时间(分钟),0 表示无限制 | | `allow_command` | bool | false | 允许 cron 任务执行 shell 命令 | +对于不带 `command` 的普通 cron 任务,保存下来的 `message` 会在任务触发时重新作为一条新的“用户消息”送回 agent。编写时应使用用户视角/直接对 agent 说话的口吻,例如 `每小时检查仓库更新,如果有新版本告诉我`,而不是 `如果有更新就通知用户`。 + ## MCP 工具 MCP 工具支持与外部 Model Context Protocol 服务器集成。 diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index ac230aa868..245774030d 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -1033,6 +1033,47 @@ func (al *AgentLoop) ReloadProviderAndConfig( // Ensure shared tools are re-registered on the new registry registerSharedTools(al, cfg, al.bus, registry, provider) + var ( + newMCPManager mcpController + mcpSummary mcpRegistrationSummary + ) + if cfg.Tools.IsToolEnabled("mcp") && countEnabledMCPServers(cfg.Tools.MCP.Servers) > 0 { + newMCPManager = newMCPController() + + workspacePath := cfg.WorkspacePath() + if defaultAgent := registry.GetDefaultAgent(); defaultAgent != nil && defaultAgent.Workspace != "" { + workspacePath = defaultAgent.Workspace + } + + if err := newMCPManager.LoadFromMCPConfig(ctx, cfg.Tools.MCP, workspacePath); err != nil { + if closeErr := newMCPManager.Close(); closeErr != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]any{ + "error": closeErr.Error(), + }) + } + return fmt.Errorf("failed to initialize MCP during reload: %w", err) + } + + mcpSummary = registerMCPToolsOnRegistry(registry, cfg, newMCPManager, newMCPManager.GetServers()) + if err := registerMCPDiscoveryToolsOnRegistry(registry, cfg); err != nil { + if closeErr := newMCPManager.Close(); closeErr != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]any{ + "error": closeErr.Error(), + }) + } + return fmt.Errorf("failed to restore MCP discovery tools during reload: %w", err) + } + } + if al.mediaStore != nil { + for _, agentID := range registry.ListAgentIDs() { + if agent, ok := registry.GetAgent(agentID); ok { + agent.Tools.SetMediaStore(al.mediaStore) + } + } + } + // Atomically swap the config and registry under write lock // This ensures readers see a consistent pair al.mu.Lock() @@ -1054,6 +1095,7 @@ func (al *AgentLoop) ReloadProviderAndConfig( al.mu.Unlock() + oldMCPManager := al.mcp.replaceForReload(newMCPManager, newMCPManager != nil) al.hookRuntime.reset(al) configureHookManagerFromConfig(al.hooks, cfg) @@ -1074,10 +1116,21 @@ func (al *AgentLoop) ReloadProviderAndConfig( } } } + if oldMCPManager != nil { + if err := oldMCPManager.Close(); err != nil { + logger.ErrorCF("agent", "Failed to close previous MCP manager", + map[string]any{ + "error": err.Error(), + }) + } + } logger.InfoCF("agent", "Provider and config reloaded successfully", map[string]any{ - "model": cfg.Agents.Defaults.GetModelName(), + "model": cfg.Agents.Defaults.GetModelName(), + "mcp_server_count": mcpSummary.serverCount, + "mcp_unique_tools": mcpSummary.uniqueTools, + "mcp_total_registrations": mcpSummary.totalRegistrations, }) return nil @@ -3454,6 +3507,9 @@ func (al *AgentLoop) buildCommandsRuntime(agent *AgentInstance, opts *processOpt Config: cfg, ListAgentIDs: registry.ListAgentIDs, ListDefinitions: al.cmdRegistry.Definitions, + GetMCPStatus: func() string { + return formatMCPStatus(cfg, al.mcp.statusSnapshot()) + }, GetEnabledChannels: func() []string { if al.channelManager == nil { return nil diff --git a/pkg/agent/loop_mcp.go b/pkg/agent/loop_mcp.go index b9c844d1a3..15f819df47 100644 --- a/pkg/agent/loop_mcp.go +++ b/pkg/agent/loop_mcp.go @@ -9,6 +9,8 @@ package agent import ( "context" "fmt" + "sort" + "strings" "sync" "github.com/sipeed/picoclaw/pkg/config" @@ -18,22 +20,54 @@ import ( ) type mcpRuntime struct { - initOnce sync.Once - mu sync.Mutex - manager *mcp.Manager - initErr error + initOnce sync.Once + mu sync.Mutex + attempted bool + manager mcpController + caller tools.MCPManager + servers map[string]*mcp.ServerConnection + initErr error + lastErr error } -func (r *mcpRuntime) setManager(manager *mcp.Manager) { +type mcpController interface { + tools.MCPManager + LoadFromMCPConfig(ctx context.Context, mcpCfg config.MCPConfig, workspacePath string) error + GetServers() map[string]*mcp.ServerConnection + Close() error +} + +var newMCPController = func() mcpController { + return mcp.NewManager() +} + +func (r *mcpRuntime) setManager(manager mcpController) { r.mu.Lock() + r.attempted = true r.manager = manager + r.caller = manager + if manager != nil { + r.servers = manager.GetServers() + } else { + r.servers = nil + } r.initErr = nil + r.lastErr = nil r.mu.Unlock() } func (r *mcpRuntime) setInitErr(err error) { r.mu.Lock() + r.attempted = true r.initErr = err + r.lastErr = err + r.mu.Unlock() +} + +func (r *mcpRuntime) setStatusErr(err error) { + r.mu.Lock() + r.attempted = true + r.lastErr = err r.mu.Unlock() } @@ -43,20 +77,301 @@ func (r *mcpRuntime) getInitErr() error { return r.initErr } -func (r *mcpRuntime) takeManager() *mcp.Manager { +func (r *mcpRuntime) takeManager() mcpController { r.mu.Lock() defer r.mu.Unlock() manager := r.manager r.manager = nil + r.caller = nil + r.servers = nil return manager } +func (r *mcpRuntime) replaceForReload(manager mcpController, attempted bool) mcpController { + r.mu.Lock() + defer r.mu.Unlock() + + oldManager := r.manager + r.initOnce = sync.Once{} + r.attempted = attempted + r.manager = manager + r.initErr = nil + r.lastErr = nil + if manager != nil { + r.caller = manager + r.servers = manager.GetServers() + } else { + r.caller = nil + r.servers = nil + } + if attempted { + r.initOnce.Do(func() {}) + } + return oldManager +} + func (r *mcpRuntime) hasManager() bool { r.mu.Lock() defer r.mu.Unlock() return r.manager != nil } +type mcpStatusSnapshot struct { + attempted bool + lastErr error + servers map[string]*mcp.ServerConnection +} + +func (r *mcpRuntime) statusSnapshot() mcpStatusSnapshot { + r.mu.Lock() + defer r.mu.Unlock() + + servers := make(map[string]*mcp.ServerConnection, len(r.servers)) + for name, conn := range r.servers { + servers[name] = conn + } + + return mcpStatusSnapshot{ + attempted: r.attempted, + lastErr: r.lastErr, + servers: servers, + } +} + +func (r *mcpRuntime) registrationSnapshot() (tools.MCPManager, map[string]*mcp.ServerConnection) { + r.mu.Lock() + defer r.mu.Unlock() + + if r.caller == nil || len(r.servers) == 0 { + return nil, nil + } + + servers := make(map[string]*mcp.ServerConnection, len(r.servers)) + for name, conn := range r.servers { + servers[name] = conn + } + return r.caller, servers +} + +func formatMCPStatus(cfg *config.Config, snap mcpStatusSnapshot) string { + if cfg == nil { + return "MCP status unavailable: config not loaded." + } + + if !cfg.Tools.IsToolEnabled("mcp") || !cfg.Tools.MCP.Enabled { + return "MCP is disabled." + } + + configured := cfg.Tools.MCP.Servers + if len(configured) == 0 { + return "MCP is enabled, but no servers are configured." + } + + lines := []string{ + fmt.Sprintf("MCP Enabled: yes"), + fmt.Sprintf("Initialization Attempted: %s", yesNo(snap.attempted)), + fmt.Sprintf("Connected Servers: %d/%d", len(snap.servers), countEnabledMCPServers(configured)), + } + + if snap.lastErr != nil { + lines = append(lines, fmt.Sprintf("Last Init Error: %s", snap.lastErr.Error())) + } + + names := make([]string, 0, len(configured)) + for name, serverCfg := range configured { + if !serverCfg.Enabled { + continue + } + names = append(names, name) + } + sort.Strings(names) + + if len(names) == 0 { + lines = append(lines, "No enabled MCP servers.") + return strings.Join(lines, "\n") + } + + lines = append(lines, "", "Servers:") + for _, name := range names { + serverCfg := configured[name] + conn, connected := snap.servers[name] + toolCount := 0 + if conn != nil { + toolCount = len(conn.Tools) + } + lines = append(lines, fmt.Sprintf( + "- %s: %s, transport=%s, tools=%d%s", + name, + connectionStatusLabel(connected), + mcpTransportLabel(serverCfg), + toolCount, + mcpEndpointSummary(serverCfg), + )) + } + + return strings.Join(lines, "\n") +} + +func countEnabledMCPServers(servers map[string]config.MCPServerConfig) int { + count := 0 + for _, serverCfg := range servers { + if serverCfg.Enabled { + count++ + } + } + return count +} + +func yesNo(v bool) string { + if v { + return "yes" + } + return "no" +} + +func connectionStatusLabel(connected bool) string { + if connected { + return "connected" + } + return "not connected" +} + +func mcpTransportLabel(serverCfg config.MCPServerConfig) string { + transportType := strings.TrimSpace(serverCfg.Type) + if transportType != "" { + return transportType + } + if strings.TrimSpace(serverCfg.URL) != "" { + return "sse" + } + if strings.TrimSpace(serverCfg.Command) != "" { + return "stdio" + } + return "unknown" +} + +func mcpEndpointSummary(serverCfg config.MCPServerConfig) string { + if url := strings.TrimSpace(serverCfg.URL); url != "" { + return fmt.Sprintf(", url=%s", url) + } + if cmd := strings.TrimSpace(serverCfg.Command); cmd != "" { + return fmt.Sprintf(", command=%s", cmd) + } + return "" +} + +type mcpRegistrationSummary struct { + serverCount int + uniqueTools int + totalRegistrations int + agentCount int +} + +func registerMCPToolsOnRegistry( + registry *AgentRegistry, + cfg *config.Config, + caller tools.MCPManager, + servers map[string]*mcp.ServerConnection, +) mcpRegistrationSummary { + if registry == nil || cfg == nil || caller == nil || len(servers) == 0 { + return mcpRegistrationSummary{} + } + if !cfg.Tools.IsToolEnabled("mcp") || !cfg.Tools.MCP.Enabled { + return mcpRegistrationSummary{} + } + + agentIDs := registry.ListAgentIDs() + summary := mcpRegistrationSummary{ + serverCount: len(servers), + agentCount: len(agentIDs), + } + + for serverName, conn := range servers { + serverCfg, ok := cfg.Tools.MCP.Servers[serverName] + if !ok || !serverCfg.Enabled { + continue + } + + summary.uniqueTools += len(conn.Tools) + registerAsHidden := serverIsDeferred(cfg.Tools.MCP.Discovery.Enabled, serverCfg) + + for _, tool := range conn.Tools { + for _, agentID := range agentIDs { + agent, ok := registry.GetAgent(agentID) + if !ok { + continue + } + + mcpTool := tools.NewMCPTool(caller, serverName, tool) + mcpTool.SetWorkspace(agent.Workspace) + mcpTool.SetMaxInlineTextRunes(cfg.Tools.MCP.GetMaxInlineTextChars()) + + if registerAsHidden { + agent.Tools.RegisterHidden(mcpTool) + } else { + agent.Tools.Register(mcpTool) + } + + summary.totalRegistrations++ + logger.DebugCF("agent", "Registered MCP tool", + map[string]any{ + "agent_id": agentID, + "server": serverName, + "tool": tool.Name, + "name": mcpTool.Name(), + "deferred": registerAsHidden, + }) + } + } + } + + return summary +} + +func registerMCPDiscoveryToolsOnRegistry(registry *AgentRegistry, cfg *config.Config) error { + if registry == nil || cfg == nil || !cfg.Tools.MCP.Enabled || !cfg.Tools.MCP.Discovery.Enabled { + return nil + } + + useBM25 := cfg.Tools.MCP.Discovery.UseBM25 + useRegex := cfg.Tools.MCP.Discovery.UseRegex + if !useBM25 && !useRegex { + return fmt.Errorf( + "tool discovery is enabled but neither 'use_bm25' nor 'use_regex' is set to true in the configuration", + ) + } + + ttl := cfg.Tools.MCP.Discovery.TTL + if ttl <= 0 { + ttl = 5 + } + + maxSearchResults := cfg.Tools.MCP.Discovery.MaxSearchResults + if maxSearchResults <= 0 { + maxSearchResults = 5 + } + + logger.InfoCF("agent", "Initializing tool discovery", map[string]any{ + "bm25": useBM25, "regex": useRegex, "ttl": ttl, "max_results": maxSearchResults, + }) + + for _, agentID := range registry.ListAgentIDs() { + agent, ok := registry.GetAgent(agentID) + if !ok { + continue + } + + if useRegex { + agent.Tools.Register(tools.NewRegexSearchTool(agent.Tools, ttl, maxSearchResults)) + } + if useBM25 { + agent.Tools.Register(tools.NewBM25SearchTool(agent.Tools, ttl, maxSearchResults)) + } + } + + return nil +} + // ensureMCPInitialized loads MCP servers/tools once so both Run() and direct // agent mode share the same initialization path. func (al *AgentLoop) ensureMCPInitialized(ctx context.Context) error { @@ -81,7 +396,7 @@ func (al *AgentLoop) ensureMCPInitialized(ctx context.Context) error { } al.mcp.initOnce.Do(func() { - mcpManager := mcp.NewManager() + mcpManager := newMCPController() defaultAgent := al.registry.GetDefaultAgent() workspacePath := al.cfg.WorkspacePath() @@ -90,6 +405,7 @@ func (al *AgentLoop) ensureMCPInitialized(ctx context.Context) error { } if err := mcpManager.LoadFromMCPConfig(ctx, al.cfg.Tools.MCP, workspacePath); err != nil { + al.mcp.setStatusErr(err) logger.WarnCF("agent", "Failed to load MCP servers, MCP tools will not be available", map[string]any{ "error": err.Error(), @@ -103,104 +419,25 @@ func (al *AgentLoop) ensureMCPInitialized(ctx context.Context) error { return } - // Register MCP tools for all agents servers := mcpManager.GetServers() - uniqueTools := 0 - totalRegistrations := 0 - agentIDs := al.registry.ListAgentIDs() - agentCount := len(agentIDs) - - for serverName, conn := range servers { - uniqueTools += len(conn.Tools) - - // Determine whether this server's tools should be deferred (hidden). - // Per-server "deferred" field takes precedence over the global Discovery.Enabled. - serverCfg := al.cfg.Tools.MCP.Servers[serverName] - registerAsHidden := serverIsDeferred(al.cfg.Tools.MCP.Discovery.Enabled, serverCfg) - - for _, tool := range conn.Tools { - for _, agentID := range agentIDs { - agent, ok := al.registry.GetAgent(agentID) - if !ok { - continue - } - - mcpTool := tools.NewMCPTool(mcpManager, serverName, tool) - mcpTool.SetWorkspace(agent.Workspace) - mcpTool.SetMaxInlineTextRunes(al.cfg.Tools.MCP.GetMaxInlineTextChars()) - - if registerAsHidden { - agent.Tools.RegisterHidden(mcpTool) - } else { - agent.Tools.Register(mcpTool) - } - - totalRegistrations++ - logger.DebugCF("agent", "Registered MCP tool", - map[string]any{ - "agent_id": agentID, - "server": serverName, - "tool": tool.Name, - "name": mcpTool.Name(), - "deferred": registerAsHidden, - }) - } - } - } + summary := registerMCPToolsOnRegistry(al.registry, al.cfg, mcpManager, servers) logger.InfoCF("agent", "MCP tools registered successfully", map[string]any{ - "server_count": len(servers), - "unique_tools": uniqueTools, - "total_registrations": totalRegistrations, - "agent_count": agentCount, - }) - - // Initializes Discovery Tools only if enabled by configuration - if al.cfg.Tools.MCP.Enabled && al.cfg.Tools.MCP.Discovery.Enabled { - useBM25 := al.cfg.Tools.MCP.Discovery.UseBM25 - useRegex := al.cfg.Tools.MCP.Discovery.UseRegex - - // Fail fast: If discovery is enabled but no search method is turned on - if !useBM25 && !useRegex { - al.mcp.setInitErr(fmt.Errorf( - "tool discovery is enabled but neither 'use_bm25' nor 'use_regex' is set to true in the configuration", - )) - if closeErr := mcpManager.Close(); closeErr != nil { - logger.ErrorCF("agent", "Failed to close MCP manager", - map[string]any{ - "error": closeErr.Error(), - }) - } - return - } - - ttl := al.cfg.Tools.MCP.Discovery.TTL - if ttl <= 0 { - ttl = 5 // Default value - } - - maxSearchResults := al.cfg.Tools.MCP.Discovery.MaxSearchResults - if maxSearchResults <= 0 { - maxSearchResults = 5 // Default value - } - - logger.InfoCF("agent", "Initializing tool discovery", map[string]any{ - "bm25": useBM25, "regex": useRegex, "ttl": ttl, "max_results": maxSearchResults, + "server_count": summary.serverCount, + "unique_tools": summary.uniqueTools, + "total_registrations": summary.totalRegistrations, + "agent_count": summary.agentCount, }) - for _, agentID := range agentIDs { - agent, ok := al.registry.GetAgent(agentID) - if !ok { - continue - } - - if useRegex { - agent.Tools.Register(tools.NewRegexSearchTool(agent.Tools, ttl, maxSearchResults)) - } - if useBM25 { - agent.Tools.Register(tools.NewBM25SearchTool(agent.Tools, ttl, maxSearchResults)) - } + if err := registerMCPDiscoveryToolsOnRegistry(al.registry, al.cfg); err != nil { + al.mcp.setInitErr(err) + if closeErr := mcpManager.Close(); closeErr != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]any{ + "error": closeErr.Error(), + }) } + return } al.mcp.setManager(mcpManager) diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index a67c8d040c..2f14634596 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -14,9 +14,12 @@ import ( "testing" "time" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" "github.com/sipeed/picoclaw/pkg/config" + mcppkg "github.com/sipeed/picoclaw/pkg/mcp" "github.com/sipeed/picoclaw/pkg/media" "github.com/sipeed/picoclaw/pkg/providers" "github.com/sipeed/picoclaw/pkg/routing" @@ -75,6 +78,66 @@ type recordingProvider struct { lastMessages []providers.Message } +type fakeMCPController struct { + closed bool + servers map[string]*mcppkg.ServerConnection +} + +func (m *fakeMCPController) LoadFromMCPConfig( + _ context.Context, + mcpCfg config.MCPConfig, + _ string, +) error { + m.servers = make(map[string]*mcppkg.ServerConnection) + for serverName, serverCfg := range mcpCfg.Servers { + if !serverCfg.Enabled { + continue + } + m.servers[serverName] = &mcppkg.ServerConnection{ + Name: serverName, + Tools: []*sdkmcp.Tool{ + { + Name: "ping", + Description: "Remote ping", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{}, + }, + }, + }, + } + } + return nil +} + +func (m *fakeMCPController) GetServers() map[string]*mcppkg.ServerConnection { + servers := make(map[string]*mcppkg.ServerConnection, len(m.servers)) + for name, conn := range m.servers { + servers[name] = conn + } + return servers +} + +func (m *fakeMCPController) CallTool( + ctx context.Context, + serverName, toolName string, + arguments map[string]any, +) (*sdkmcp.CallToolResult, error) { + if m.closed { + return nil, fmt.Errorf("manager is closed") + } + return &sdkmcp.CallToolResult{ + Content: []sdkmcp.Content{ + &sdkmcp.TextContent{Text: fmt.Sprintf("%s:%s", serverName, toolName)}, + }, + }, nil +} + +func (m *fakeMCPController) Close() error { + m.closed = true + return nil +} + func (r *recordingProvider) Chat( ctx context.Context, messages []providers.Message, @@ -2365,6 +2428,216 @@ func TestProcessDirectWithChannel_TriggersMCPInitialization(t *testing.T) { } } +func TestReloadProviderAndConfig_RebuildsMCPToolsFromNewConfig(t *testing.T) { + tmpDir := t.TempDir() + + oldFactory := newMCPController + newMCPController = func() mcpController { + return &fakeMCPController{} + } + t.Cleanup(func() { + newMCPController = oldFactory + }) + + oldCfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + Tools: config.ToolsConfig{ + MCP: config.MCPConfig{ + ToolConfig: config.ToolConfig{ + Enabled: true, + }, + Servers: map[string]config.MCPServerConfig{ + "stale": {Enabled: true}, + }, + }, + }, + } + newCfg := &config.Config{ + Agents: oldCfg.Agents, + Tools: config.ToolsConfig{ + MCP: config.MCPConfig{ + ToolConfig: config.ToolConfig{ + Enabled: true, + }, + Servers: map[string]config.MCPServerConfig{ + "fresh": {Enabled: true}, + }, + }, + }, + } + + msgBus := bus.NewMessageBus() + al := NewAgentLoop(oldCfg, msgBus, &mockProvider{}) + defer al.Close() + + al.mcp.setManager(&fakeMCPController{ + servers: map[string]*mcppkg.ServerConnection{ + "stale": { + Name: "stale", + Tools: []*sdkmcp.Tool{ + { + Name: "ping", + Description: "Stale ping", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{}, + }, + }, + }, + }, + }, + }) + + if err := al.ReloadProviderAndConfig(context.Background(), &mockProvider{}, newCfg); err != nil { + t.Fatalf("ReloadProviderAndConfig() error = %v", err) + } + + agent := al.GetRegistry().GetDefaultAgent() + if agent == nil { + t.Fatal("expected default agent after reload") + } + + if _, ok := agent.Tools.Get("mcp_stale_ping"); ok { + t.Fatal("expected stale MCP tool to be removed after reload") + } + + toolName := "mcp_fresh_ping" + if _, ok := agent.Tools.Get(toolName); !ok { + t.Fatalf("expected MCP tool %q to be registered from reloaded config", toolName) + } + + result := agent.Tools.Execute(context.Background(), toolName, map[string]any{}) + if result == nil || result.IsError { + t.Fatalf("expected MCP tool %q to execute successfully after reload, got %#v", toolName, result) + } + if !strings.Contains(result.ContentForLLM(), "fresh:ping") { + t.Fatalf("unexpected MCP tool result after reload: %q", result.ContentForLLM()) + } +} + +func TestReloadProviderAndConfig_RemovesMCPToolsWhenServersDeleted(t *testing.T) { + tmpDir := t.TempDir() + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + Tools: config.ToolsConfig{ + MCP: config.MCPConfig{ + ToolConfig: config.ToolConfig{ + Enabled: true, + }, + Servers: map[string]config.MCPServerConfig{ + "remote": {Enabled: true}, + }, + }, + }, + } + reloadedCfg := &config.Config{ + Agents: cfg.Agents, + Tools: config.ToolsConfig{ + MCP: config.MCPConfig{ + ToolConfig: config.ToolConfig{ + Enabled: true, + }, + }, + }, + } + + msgBus := bus.NewMessageBus() + al := NewAgentLoop(cfg, msgBus, &mockProvider{}) + defer al.Close() + + al.mcp.setManager(&fakeMCPController{ + servers: map[string]*mcppkg.ServerConnection{ + "remote": { + Name: "remote", + Tools: []*sdkmcp.Tool{ + { + Name: "ping", + Description: "Remote ping", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{}, + }, + }, + }, + }, + }, + }) + + if err := al.ReloadProviderAndConfig(context.Background(), &mockProvider{}, reloadedCfg); err != nil { + t.Fatalf("ReloadProviderAndConfig() error = %v", err) + } + + agent := al.GetRegistry().GetDefaultAgent() + if agent == nil { + t.Fatal("expected default agent after reload") + } + if _, ok := agent.Tools.Get("mcp_remote_ping"); ok { + t.Fatal("expected MCP tool to be removed when no servers are configured after reload") + } + if al.mcp.hasManager() { + t.Fatal("expected MCP manager to be cleared when reload removes all servers") + } +} + +func TestFormatMCPStatus_IncludesConfiguredServers(t *testing.T) { + cfg := &config.Config{ + Tools: config.ToolsConfig{ + MCP: config.MCPConfig{ + ToolConfig: config.ToolConfig{Enabled: true}, + Servers: map[string]config.MCPServerConfig{ + "remote-http": { + Enabled: true, + Type: "http", + URL: "http://127.0.0.1:8080/mcp", + }, + "local-stdio": { + Enabled: true, + Command: "npx", + }, + }, + }, + }, + } + + status := formatMCPStatus(cfg, mcpStatusSnapshot{ + attempted: true, + servers: map[string]*mcppkg.ServerConnection{ + "remote-http": { + Name: "remote-http", + Tools: []*sdkmcp.Tool{{Name: "ping"}, {Name: "echo"}}, + }, + }, + }) + + if !strings.Contains(status, "Initialization Attempted: yes") { + t.Fatalf("status missing initialization state:\n%s", status) + } + if !strings.Contains(status, "Connected Servers: 1/2") { + t.Fatalf("status missing connected server count:\n%s", status) + } + if !strings.Contains(status, "remote-http: connected, transport=http, tools=2") { + t.Fatalf("status missing connected http server details:\n%s", status) + } + if !strings.Contains(status, "local-stdio: not connected, transport=stdio, tools=0, command=npx") { + t.Fatalf("status missing disconnected stdio server details:\n%s", status) + } +} + func TestTargetReasoningChannelID_AllChannels(t *testing.T) { tmpDir, err := os.MkdirTemp("", "agent-test-*") if err != nil { diff --git a/pkg/commands/builtin_test.go b/pkg/commands/builtin_test.go index 5fd8dd9bc8..e7432d34c7 100644 --- a/pkg/commands/builtin_test.go +++ b/pkg/commands/builtin_test.go @@ -39,7 +39,7 @@ func TestBuiltinHelpHandler_ReturnsFormattedMessage(t *testing.T) { if !strings.Contains(reply, "/show [model|channel|agents]") { t.Fatalf("/help reply missing /show usage, got %q", reply) } - if !strings.Contains(reply, "/list [models|channels|agents|skills]") { + if !strings.Contains(reply, "/list [models|channels|agents|skills|mcp]") { t.Fatalf("/help reply missing /list usage, got %q", reply) } if !strings.Contains(reply, "/use ") { @@ -174,6 +174,31 @@ func TestBuiltinListSkills_UsesRuntimeSkillNames(t *testing.T) { } } +func TestBuiltinListMCP_UsesRuntimeStatus(t *testing.T) { + rt := &Runtime{ + GetMCPStatus: func() string { + return "MCP Enabled: yes\nConnected Servers: 1/1\n\nServers:\n- remote: connected, transport=http, tools=3, url=http://127.0.0.1:8080/mcp" + }, + } + defs := BuiltinDefinitions() + ex := NewExecutor(NewRegistry(defs), rt) + + var reply string + res := ex.Execute(context.Background(), Request{ + Text: "/list mcp", + Reply: func(text string) error { + reply = text + return nil + }, + }) + if res.Outcome != OutcomeHandled { + t.Fatalf("/list mcp: outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !strings.Contains(reply, "Connected Servers: 1/1") || !strings.Contains(reply, "remote: connected") { + t.Fatalf("/list mcp reply=%q, want MCP status summary", reply) + } +} + func TestBuiltinUseCommand_PassthroughsToAgentLogic(t *testing.T) { defs := BuiltinDefinitions() ex := NewExecutor(NewRegistry(defs), nil) diff --git a/pkg/commands/cmd_list.go b/pkg/commands/cmd_list.go index 7186a6c256..869cc13973 100644 --- a/pkg/commands/cmd_list.go +++ b/pkg/commands/cmd_list.go @@ -64,6 +64,16 @@ func listCommand() Definition { )) }, }, + { + Name: "mcp", + Description: "MCP server status", + Handler: func(_ context.Context, req Request, rt *Runtime) error { + if rt == nil || rt.GetMCPStatus == nil { + return req.Reply(unavailableMsg) + } + return req.Reply(rt.GetMCPStatus()) + }, + }, }, } } diff --git a/pkg/commands/runtime.go b/pkg/commands/runtime.go index 5ba6a1bd27..a8fa085f58 100644 --- a/pkg/commands/runtime.go +++ b/pkg/commands/runtime.go @@ -12,6 +12,7 @@ type Runtime struct { ListDefinitions func() []Definition ListSkillNames func() []string GetEnabledChannels func() []string + GetMCPStatus func() string GetActiveTurn func() any // Returning any to avoid circular dependency with agent package SwitchModel func(value string) (oldModel string, err error) SwitchChannel func(value string) error diff --git a/pkg/commands/show_list_handlers_test.go b/pkg/commands/show_list_handlers_test.go index 28d481b67c..5d856ae0ea 100644 --- a/pkg/commands/show_list_handlers_test.go +++ b/pkg/commands/show_list_handlers_test.go @@ -64,6 +64,9 @@ func TestShowListHandlers_ListHandledOnAllChannels(t *testing.T) { ListSkillNames: func() []string { return []string{"shell"} }, + GetMCPStatus: func() string { + return "MCP Enabled: yes\nConnected Servers: 0/1" + }, } ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt) @@ -101,4 +104,20 @@ func TestShowListHandlers_ListHandledOnAllChannels(t *testing.T) { if !strings.Contains(reply, "shell") { t.Fatalf("whatsapp /list skills reply=%q, expected installed skills content", reply) } + + reply = "" + res = ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/list mcp", + Reply: func(text string) error { + reply = text + return nil + }, + }) + if res.Outcome != OutcomeHandled { + t.Fatalf("whatsapp /list mcp outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !strings.Contains(reply, "Connected Servers: 0/1") { + t.Fatalf("whatsapp /list mcp reply=%q, expected mcp status content", reply) + } } diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index c6ac3a129a..60d701ae17 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -73,7 +73,7 @@ func (t *CronTool) Name() string { // Description returns the tool description func (t *CronTool) Description() string { - return "Schedule reminders, tasks, or system commands. IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). Use 'cron_expr' for complex recurring schedules. Use 'command' to execute shell commands directly." + return "Schedule reminders, tasks, or system commands. IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. For normal reminder/task jobs, write 'message' as the user's request to the agent in first-person/direct-address style (for example, 'check the repo every hour and tell me if there is a new release'), not third-person text like 'notify the user'. Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). Use 'cron_expr' for complex recurring schedules. Use 'command' to execute shell commands directly." } // Parameters returns the tool parameters schema @@ -88,7 +88,7 @@ func (t *CronTool) Parameters() map[string]any { }, "message": map[string]any{ "type": "string", - "description": "The reminder/task message to display when triggered. If 'command' is used, this describes what the command does.", + "description": "The reminder/task message saved with the job. For normal cron jobs, this will be sent back into the agent loop as a new user-style message, so phrase it from the user's perspective (e.g. 'check the repo and tell me if there is an update', not 'notify the user'). If 'command' is used, this describes what the command does.", }, "command": map[string]any{ "type": "string",