From 4e330b297c99a3f5c1e44869b84028b8069ca433 Mon Sep 17 00:00:00 2001
From: yuchou87 <yuchou87@gmail.com>
Date: Sun, 22 Feb 2026 15:13:29 +0800
Subject: [PATCH] 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)
+	}
+}