Harden MCP transport timeouts and expand external MCP e2e tests

pkg/tools/mcp.go

@@ -20,6 +20,7 @@ import (
 const (
 	defaultMCPStartupTimeout = 8 * time.Second
 	defaultMCPCallTimeout    = 30 * time.Second
+	defaultMCPTerminateWait  = 1 * time.Second
 	maxToolNameLength        = 64
 )
 
@@ -210,9 +211,7 @@ func (c *mcpClient) buildTransport() (mcp.Transport, error) {
 		tr := &mcp.CommandTransport{
 			Command: cmd,
 		}
-		if c.cfg.TerminateTimeoutMS > 0 {
+		tr.TerminateDuration = durationFromMS(c.cfg.TerminateTimeoutMS, defaultMCPTerminateWait)
-			tr.TerminateDuration = time.Duration(c.cfg.TerminateTimeoutMS) * time.Millisecond
-		}
 		return tr, nil
 	case "streamable_http":
 		if strings.TrimSpace(c.cfg.URL) == "" {

pkg/tools/mcp_external_integration_test.go

@@ -22,6 +22,15 @@ func TestMCPExternalPopularFilesystemCommand(t *testing.T) {
 	requireExternalMCPTests(t)
 
 	root := t.TempDir()
+	rootCanonical, err := filepath.EvalSymlinks(root)
+	if err != nil {
+		t.Fatalf("EvalSymlinks(root) error: %v", err)
+	}
+	testFile := filepath.Join(rootCanonical, "hello.txt")
+	if err := os.WriteFile(testFile, []byte("hello from filesystem mcp"), 0644); err != nil {
+		t.Fatalf("WriteFile(testFile) error: %v", err)
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
 	defer cancel()
 
@@ -33,7 +42,7 @@ func TestMCPExternalPopularFilesystemCommand(t *testing.T) {
 				Enabled:          true,
 				Transport:        "command",
 				Command:          "npx",
-				Args:             []string{"-y", "@modelcontextprotocol/server-filesystem", root},
+				Args:             []string{"-y", "@modelcontextprotocol/server-filesystem", rootCanonical},
 				ToolPrefix:       "mcp_fs",
 				StartupTimeoutMS: 30000,
 				CallTimeoutMS:    30000,
@@ -46,17 +55,29 @@ func TestMCPExternalPopularFilesystemCommand(t *testing.T) {
 		t.Fatalf("LoadMCPTools() error: %v", err)
 	}
 
-	tool := findToolByName(tools, "mcp_fs_list_allowed_directories")
+	listAllowedDirs := findToolByName(tools, "mcp_fs_list_allowed_directories")
-	if tool == nil {
+	if listAllowedDirs == nil {
 		t.Fatalf("missing tool mcp_fs_list_allowed_directories; got %v", toolNames(tools))
 	}
+	readFile := findToolByName(tools, "mcp_fs_read_file")
+	if readFile == nil {
+		t.Fatalf("missing tool mcp_fs_read_file; got %v", toolNames(tools))
+	}
 
-	out, err := tool.Execute(ctx, map[string]interface{}{})
+	out, err := listAllowedDirs.Execute(ctx, map[string]interface{}{})
 	if err != nil {
 		t.Fatalf("Execute(list_allowed_directories) error: %v", err)
 	}
-	if !strings.Contains(out, root) {
+	if !strings.Contains(out, rootCanonical) {
-		t.Fatalf("expected allowed directory %q in output: %s", root, out)
+		t.Fatalf("expected allowed directory %q in output: %s", rootCanonical, out)
+	}
+
+	readOut, err := readFile.Execute(ctx, map[string]interface{}{"path": testFile})
+	if err != nil {
+		t.Fatalf("Execute(read_file) error: %v", err)
+	}
+	if !strings.Contains(readOut, "hello from filesystem mcp") {
+		t.Fatalf("expected file content in output, got: %s", readOut)
 	}
 }
 
@@ -107,7 +128,7 @@ func TestMCPExternalPopularEverythingSSE(t *testing.T) {
 	requireExternalMCPTests(t)
 
 	port := pickFreePort(t)
-	cmd := startEverythingSSEServer(t, port)
+	cmd := startEverythingServer(t, port, "sse")
 	waitForTCPPort(t, fmt.Sprintf("127.0.0.1:%d", port), 15*time.Second)
 
 	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
@@ -149,6 +170,52 @@ func TestMCPExternalPopularEverythingSSE(t *testing.T) {
 	_ = cmd
 }
 
+func TestMCPExternalPopularEverythingStreamableHTTP(t *testing.T) {
+	requireExternalMCPTests(t)
+
+	port := pickFreePort(t)
+	cmd := startEverythingServer(t, port, "streamableHttp")
+	waitForTCPPort(t, fmt.Sprintf("127.0.0.1:%d", port), 15*time.Second)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+	defer cancel()
+
+	cfg := config.MCPToolsConfig{
+		Enabled: true,
+		Servers: []config.MCPServerConfig{
+			{
+				Name:             "everything-http",
+				Enabled:          true,
+				Transport:        "streamable_http",
+				URL:              fmt.Sprintf("http://127.0.0.1:%d/mcp", port),
+				ToolPrefix:       "mcp_http",
+				StartupTimeoutMS: 30000,
+				CallTimeoutMS:    30000,
+			},
+		},
+	}
+
+	tools, err := LoadMCPTools(ctx, cfg, "")
+	if err != nil {
+		t.Fatalf("LoadMCPTools() error: %v", err)
+	}
+
+	echoTool := findToolByName(tools, "mcp_http_echo")
+	if echoTool == nil {
+		t.Fatalf("missing tool mcp_http_echo; got %v", toolNames(tools))
+	}
+
+	out, err := echoTool.Execute(ctx, map[string]interface{}{"message": "hello from streamable-http"})
+	if err != nil {
+		t.Fatalf("Execute(echo) error: %v", err)
+	}
+	if !strings.Contains(out, "hello from streamable-http") {
+		t.Fatalf("unexpected echo output: %s", out)
+	}
+
+	_ = cmd
+}
+
 func requireExternalMCPTests(t *testing.T) {
 	t.Helper()
 	if os.Getenv("PICOCLAW_RUN_EXTERNAL_MCP_TESTS") != "1" {
@@ -197,16 +264,16 @@ func waitForTCPPort(t *testing.T, addr string, timeout time.Duration) {
 	t.Fatalf("port %s did not become ready within %v", addr, timeout)
 }
 
-func startEverythingSSEServer(t *testing.T, port int) *exec.Cmd {
+func startEverythingServer(t *testing.T, port int, mode string) *exec.Cmd {
 	t.Helper()
 
-	cmd := exec.Command("npx", "-y", "@modelcontextprotocol/server-everything", "sse")
+	cmd := exec.Command("npx", "-y", "@modelcontextprotocol/server-everything", mode)
 	cmd.Env = append(os.Environ(), fmt.Sprintf("PORT=%d", port))
 	cmd.Stdout = os.Stderr
 	cmd.Stderr = os.Stderr
 
 	if err := cmd.Start(); err != nil {
-		t.Fatalf("start everything sse server: %v", err)
+		t.Fatalf("start everything %s server: %v", mode, err)
 	}
 
 	t.Cleanup(func() {

pkg/tools/mcp_test.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/modelcontextprotocol/go-sdk/mcp"
 	"github.com/sipeed/picoclaw/pkg/config"
@@ -167,6 +168,53 @@ func TestLoadMCPTools_InvalidServerAggregatesError(t *testing.T) {
 	}
 }
 
+func TestBuildTransport_CommandTerminateDefaults(t *testing.T) {
+	client := newMCPClient(config.MCPServerConfig{
+		Name:      "default-terminate",
+		Enabled:   true,
+		Transport: "command",
+		Command:   "sleep",
+		Args:      []string{"1"},
+	}, "")
+
+	tr, err := client.buildTransport()
+	if err != nil {
+		t.Fatalf("buildTransport() error: %v", err)
+	}
+
+	cmdTr, ok := tr.(*mcp.CommandTransport)
+	if !ok {
+		t.Fatalf("buildTransport() returned %T, want *mcp.CommandTransport", tr)
+	}
+	if cmdTr.TerminateDuration != defaultMCPTerminateWait {
+		t.Fatalf("TerminateDuration = %v, want %v", cmdTr.TerminateDuration, defaultMCPTerminateWait)
+	}
+}
+
+func TestBuildTransport_CommandTerminateOverride(t *testing.T) {
+	client := newMCPClient(config.MCPServerConfig{
+		Name:               "override-terminate",
+		Enabled:            true,
+		Transport:          "command",
+		Command:            "sleep",
+		Args:               []string{"1"},
+		TerminateTimeoutMS: 2500,
+	}, "")
+
+	tr, err := client.buildTransport()
+	if err != nil {
+		t.Fatalf("buildTransport() error: %v", err)
+	}
+
+	cmdTr, ok := tr.(*mcp.CommandTransport)
+	if !ok {
+		t.Fatalf("buildTransport() returned %T, want *mcp.CommandTransport", tr)
+	}
+	if cmdTr.TerminateDuration != 2500*time.Millisecond {
+		t.Fatalf("TerminateDuration = %v, want %v", cmdTr.TerminateDuration, 2500*time.Millisecond)
+	}
+}
+
 func toolNames(tools []Tool) []string {
 	out := make([]string, 0, len(tools))
 	for _, tool := range tools {