From e6a08fc51d0c51bc32a8dde0bb55c6292230ea96 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 13:28:28 +0800 Subject: [PATCH 1/8] feat(telegram): add /mcp list command and dispatch --- docs/plans/20260408-mcp-upstream-commands.md | 14 +-- internal/telegram/commands.go | 72 +++++++++++ internal/telegram/commands_test.go | 126 +++++++++++++++++++ 3 files changed, 205 insertions(+), 7 deletions(-) diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/20260408-mcp-upstream-commands.md index ba74d8b..fe36f94 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/20260408-mcp-upstream-commands.md @@ -49,13 +49,13 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. - Modify: `internal/telegram/commands.go` - Test: `internal/telegram/commands_test.go` -- [ ] Add `case "mcp"` to Handle() dispatch -- [ ] Add nil-store guard (check `h.store != nil` like handleCred does) -- [ ] Implement `handleMCP(args)` with subcommand routing: list, add, remove -- [ ] `/mcp list` - list registered upstreams with name, transport, command -- [ ] Format list output for Telegram readability (not JSON) -- [ ] Write tests for dispatch, nil-store guard, and list subcommand -- [ ] Run tests - must pass before next task +- [x] Add `case "mcp"` to Handle() dispatch +- [x] Add nil-store guard (check `h.store != nil` like handleCred does) +- [x] Implement `handleMCP(args)` with subcommand routing: list, add, remove +- [x] `/mcp list` - list registered upstreams with name, transport, command +- [x] Format list output for Telegram readability (not JSON) +- [x] Write tests for dispatch, nil-store guard, and list subcommand +- [x] Run tests - must pass before next task ### Task 2: Add /mcp add with flag parsing diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index 9ef3ff0..1b20695 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -6,6 +6,7 @@ import ( "log" "os" "regexp" + "sort" "strconv" "strings" "sync" @@ -172,6 +173,8 @@ func (h *CommandHandler) Handle(cmd *Command) string { return h.handlePolicy(cmd.Args) case "cred": return h.handleCred(cmd.Args) + case "mcp": + return h.handleMCP(cmd.Args) case "status": return h.handleStatus() case "audit": @@ -660,6 +663,75 @@ func (h *CommandHandler) credMutationComplete(msg string, removedEnvVars ...stri return msg + "\nAgent env vars updated." } +// handleMCP dispatches /mcp subcommands. +func (h *CommandHandler) handleMCP(args []string) string { + if len(args) == 0 { + return "Usage: /mcp list | /mcp add --command [--transport stdio|http|websocket] [--args a,b] [--env K=V,K=V] [--timeout 120] | /mcp remove " + } + if h.store == nil { + return "MCP management is not available (policy store not configured)." + } + switch args[0] { + case "list": + return h.mcpList() + default: + return fmt.Sprintf("Unknown mcp subcommand: %s", args[0]) + } +} + +// mcpList renders all registered MCP upstreams for Telegram display. +func (h *CommandHandler) mcpList() string { + upstreams, err := h.store.ListMCPUpstreams() + if err != nil { + return fmt.Sprintf("Failed to list MCP upstreams: %v", err) + } + if len(upstreams) == 0 { + return "No MCP upstreams registered." + } + + var b strings.Builder + b.WriteString("MCP upstreams\n") + for _, u := range upstreams { + transport := u.Transport + if transport == "" { + transport = "stdio" + } + fmt.Fprintf(&b, "[%d] %s (%s)\n command: %s\n", + u.ID, htmlEscape(u.Name), htmlEscape(transport), htmlCode(u.Command)) + if len(u.Args) > 0 { + fmt.Fprintf(&b, " args: %s\n", htmlCode(strings.Join(u.Args, " "))) + } + if len(u.Env) > 0 { + keys := make([]string, 0, len(u.Env)) + for k := range u.Env { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(keys)) + for _, k := range keys { + pairs = append(pairs, k+"="+u.Env[k]) + } + fmt.Fprintf(&b, " env: %s\n", htmlCode(strings.Join(pairs, ", "))) + } + if len(u.Headers) > 0 { + keys := make([]string, 0, len(u.Headers)) + for k := range u.Headers { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(keys)) + for _, k := range keys { + pairs = append(pairs, k+"="+u.Headers[k]) + } + fmt.Fprintf(&b, " headers: %s\n", htmlCode(strings.Join(pairs, ", "))) + } + if u.TimeoutSec != 0 && u.TimeoutSec != 120 { + fmt.Fprintf(&b, " timeout: %ds\n", u.TimeoutSec) + } + } + return b.String() +} + func (h *CommandHandler) handleStatus() string { snap := h.engine.Load().Snapshot() var b strings.Builder diff --git a/internal/telegram/commands_test.go b/internal/telegram/commands_test.go index 0f035eb..fa1c68d 100644 --- a/internal/telegram/commands_test.go +++ b/internal/telegram/commands_test.go @@ -792,6 +792,132 @@ func TestCredAddWithoutEnvVar(t *testing.T) { } } +func TestHandleMCPNoArgs(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp"}) + + if !strings.Contains(result, "Usage: /mcp") { + t.Errorf("should show usage when no args, got: %s", result) + } + if !strings.Contains(result, "list") || !strings.Contains(result, "add") || !strings.Contains(result, "remove") { + t.Errorf("usage should mention list/add/remove, got: %s", result) + } +} + +func TestHandleMCPNoStore(t *testing.T) { + // CommandHandler without a store should report MCP management is unavailable. + // Build the engine from a transient store but omit SetStore on the handler. + s := newTestStore(t) + eng, err := policy.LoadFromStore(s) + if err != nil { + t.Fatal(err) + } + ptr := new(atomic.Pointer[policy.Engine]) + ptr.Store(eng) + handler := NewCommandHandler(ptr, new(sync.Mutex), "") + // Deliberately do not call SetStore. + + result := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + if !strings.Contains(result, "not available") { + t.Errorf("should report not available when store is not configured, got: %s", result) + } +} + +func TestHandleMCPUnknownSubcommand(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp", Args: []string{"bogus"}}) + + if !strings.Contains(result, "Unknown mcp subcommand") { + t.Errorf("should report unknown subcommand, got: %s", result) + } +} + +func TestHandleMCPListEmpty(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + if !strings.Contains(result, "No MCP upstreams") { + t.Errorf("should report empty list, got: %s", result) + } +} + +func TestHandleMCPListWithUpstreams(t *testing.T) { + s := newTestStore(t) + // Add a stdio upstream with args and env. + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{ + Args: []string{"-y", "@modelcontextprotocol/server-github"}, + Env: map[string]string{"GITHUB_PAT": "vault:github_pat"}, + TimeoutSec: 120, + Transport: "stdio", + }); err != nil { + t.Fatal(err) + } + // Add an http upstream with headers and a non-default timeout. + if _, err := s.AddMCPUpstream("notion", "https://mcp.notion.com", store.MCPUpstreamOpts{ + Headers: map[string]string{"Authorization": "Bearer vault:notion_token"}, + TimeoutSec: 60, + Transport: "http", + }); err != nil { + t.Fatal(err) + } + + handler := newTestHandlerWithStore(t, s, nil, "") + out := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + + // Expect names, transports, and commands present. + must := []string{ + "github", + "stdio", + "npx", + "notion", + "http", + "https://mcp.notion.com", + "-y @modelcontextprotocol/server-github", + "GITHUB_PAT=vault:github_pat", + "Authorization=Bearer vault:notion_token", + "timeout: 60s", + } + for _, want := range must { + if !strings.Contains(out, want) { + t.Errorf("mcp list output missing %q\nfull output:\n%s", want, out) + } + } + // Default (120s) timeout should NOT be rendered. + if strings.Contains(out, "timeout: 120s") { + t.Errorf("default 120s timeout should be omitted, got: %s", out) + } +} + +func TestHandleMCPListEscapesHTML(t *testing.T) { + s := newTestStore(t) + if _, err := s.AddMCPUpstream("my", "echo ", store.MCPUpstreamOpts{ + Transport: "stdio", + }); err != nil { + t.Fatal(err) + } + + handler := newTestHandlerWithStore(t, s, nil, "") + out := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + + // Raw "" and "" must be HTML-escaped so Telegram's HTML parse + // mode does not try to render them as tags. + if strings.Contains(out, "my") { + t.Errorf("raw must be HTML-escaped: %s", out) + } + if !strings.Contains(out, "my<srv>") { + t.Errorf("expected my<srv> in output: %s", out) + } + if strings.Contains(out, "echo ") { + t.Errorf("raw must be HTML-escaped: %s", out) + } + if !strings.Contains(out, "echo <hi>") { + t.Errorf("expected escaped echo <hi> in output: %s", out) + } +} + func TestCredAddEnvVarConsumedFromValue(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") From b34a2a9d7a614d8368f9f29c28315451bda0880f Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 13:35:06 +0800 Subject: [PATCH 2/8] feat(telegram): add /mcp add with flag parsing --- docs/plans/20260408-mcp-upstream-commands.md | 12 +- internal/telegram/approval.go | 39 ++- internal/telegram/approval_test.go | 62 +++++ internal/telegram/commands.go | 90 ++++++ internal/telegram/commands_test.go | 279 +++++++++++++++++++ 5 files changed, 466 insertions(+), 16 deletions(-) diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/20260408-mcp-upstream-commands.md index fe36f94..dc9b886 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/20260408-mcp-upstream-commands.md @@ -63,12 +63,12 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. - Modify: `internal/telegram/commands.go` - Test: `internal/telegram/commands_test.go` -- [ ] `/mcp add --command ` - add stdio upstream (parse flags from args) -- [ ] `/mcp add --command --transport http` - add HTTP/WebSocket upstream (use `--command` for URLs, matching CLI convention) -- [ ] Support optional flags: `--transport`, `--args`, `--env`, `--timeout` -- [ ] Note: `--env` flag may contain secrets. Delete the Telegram message after processing (same treatment as `/cred add`) -- [ ] Write tests for add subcommand with various flag combinations -- [ ] Run tests - must pass before next task +- [x] `/mcp add --command ` - add stdio upstream (parse flags from args) +- [x] `/mcp add --command --transport http` - add HTTP/WebSocket upstream (use `--command` for URLs, matching CLI convention) +- [x] Support optional flags: `--transport`, `--args`, `--env`, `--timeout` +- [x] Note: `--env` flag may contain secrets. Delete the Telegram message after processing (same treatment as `/cred add`) +- [x] Write tests for add subcommand with various flag combinations +- [x] Run tests - must pass before next task ### Task 3: Add /mcp remove with auto-injection diff --git a/internal/telegram/approval.go b/internal/telegram/approval.go index a4a799b..604cb2a 100644 --- a/internal/telegram/approval.go +++ b/internal/telegram/approval.go @@ -385,22 +385,21 @@ func (tc *TelegramChannel) handleMessage(msg *tgbotapi.Message) { return } - // Delete messages that contain credential values before processing - // to minimize exposure in chat history. - if cmd.Name == "cred" && len(cmd.Args) >= 1 && - (cmd.Args[0] == "add" || cmd.Args[0] == "rotate") { + // Delete messages that may contain secrets before processing to minimize + // exposure in chat history. /cred add and /cred rotate always carry a + // credential value. /mcp add may carry secrets in --env KEY=VAL pairs. + isSensitive := containsSensitiveArgs(cmd) + if isSensitive { del := tgbotapi.NewDeleteMessage(msg.Chat.ID, msg.MessageID) if _, err := tc.api.Request(del); err != nil { - log.Printf("failed to delete credential message: %s", sanitizeError(err)) + log.Printf("failed to delete sensitive command message: %s", sanitizeError(err)) } } // Forward as channel.Command (non-blocking, drop if full). - // Skip cred add/rotate commands to avoid forwarding plaintext - // credential values through the command channel. - isSensitiveCred := cmd.Name == "cred" && len(cmd.Args) >= 1 && - (cmd.Args[0] == "add" || cmd.Args[0] == "rotate") - if !isSensitiveCred { + // Skip sensitive commands to avoid forwarding plaintext secrets + // through the command channel. + if !isSensitive { select { case tc.cmdCh <- channel.Command{ Name: cmd.Name, @@ -447,3 +446,23 @@ func (tc *TelegramChannel) handleMessage(msg *tgbotapi.Message) { log.Printf("telegram send error: %s", sanitizeError(err)) } } + +// containsSensitiveArgs reports whether a parsed command likely contains +// secrets in its argument list (credential values, --env KEY=VAL pairs, etc). +// Matching commands have their chat message deleted and skip forwarding to the +// external command channel so the plaintext never leaves the bot goroutine. +func containsSensitiveArgs(cmd *Command) bool { + if cmd == nil || len(cmd.Args) == 0 { + return false + } + switch cmd.Name { + case "cred": + // /cred add ... and /cred rotate ... + // both carry the secret in positional args. + return cmd.Args[0] == "add" || cmd.Args[0] == "rotate" + case "mcp": + // /mcp add may carry secrets via --env KEY=VAL. + return cmd.Args[0] == "add" + } + return false +} diff --git a/internal/telegram/approval_test.go b/internal/telegram/approval_test.go index f67f113..f39f9a0 100644 --- a/internal/telegram/approval_test.go +++ b/internal/telegram/approval_test.go @@ -1204,6 +1204,68 @@ func TestHandleMessageCredRotateDeletesMessage(t *testing.T) { } } +func TestHandleMessageMCPAddDeletesMessage(t *testing.T) { + mock := newMockTelegramAPI(t) + s := newTestStore(t) + tc := newTestTelegramChannel(t, mock, s) + + // /mcp add may carry secrets via --env KEY=VAL so the chat message + // should be deleted the same way /cred add is. + tc.handleMessage(&tgbotapi.Message{ + MessageID: 700, + Chat: &tgbotapi.Chat{ID: 12345}, + Text: "/mcp add github --command npx --env GITHUB_PAT=super-secret", + }) + + time.Sleep(100 * time.Millisecond) + if len(mock.getDeletedMessages()) == 0 { + t.Error("mcp add message should be deleted for security") + } +} + +func TestHandleMessageMCPListNotDeleted(t *testing.T) { + mock := newMockTelegramAPI(t) + s := newTestStore(t) + tc := newTestTelegramChannel(t, mock, s) + + // /mcp list never carries secrets so the message should not be deleted. + tc.handleMessage(&tgbotapi.Message{ + MessageID: 701, + Chat: &tgbotapi.Chat{ID: 12345}, + Text: "/mcp list", + }) + + time.Sleep(100 * time.Millisecond) + if len(mock.getDeletedMessages()) != 0 { + t.Error("non-sensitive /mcp list message should not be deleted") + } +} + +func TestContainsSensitiveArgs(t *testing.T) { + tests := []struct { + name string + cmd *Command + want bool + }{ + {"nil command", nil, false}, + {"empty args", &Command{Name: "cred"}, false}, + {"cred add", &Command{Name: "cred", Args: []string{"add", "name", "secret"}}, true}, + {"cred rotate", &Command{Name: "cred", Args: []string{"rotate", "name", "secret"}}, true}, + {"cred list", &Command{Name: "cred", Args: []string{"list"}}, false}, + {"cred remove", &Command{Name: "cred", Args: []string{"remove", "name"}}, false}, + {"mcp add", &Command{Name: "mcp", Args: []string{"add", "name", "--command", "cmd"}}, true}, + {"mcp list", &Command{Name: "mcp", Args: []string{"list"}}, false}, + {"policy show", &Command{Name: "policy", Args: []string{"show"}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := containsSensitiveArgs(tt.cmd); got != tt.want { + t.Errorf("containsSensitiveArgs(%+v) = %v, want %v", tt.cmd, got, tt.want) + } + }) + } +} + func TestHandleMessageForwardsToCommandChannel(t *testing.T) { mock := newMockTelegramAPI(t) s := newTestStore(t) diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index 1b20695..2b6e049 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -15,6 +15,7 @@ import ( "github.com/nemirovsky/sluice/internal/channel" "github.com/nemirovsky/sluice/internal/container" + "github.com/nemirovsky/sluice/internal/mcp" "github.com/nemirovsky/sluice/internal/policy" "github.com/nemirovsky/sluice/internal/store" "github.com/nemirovsky/sluice/internal/vault" @@ -674,11 +675,100 @@ func (h *CommandHandler) handleMCP(args []string) string { switch args[0] { case "list": return h.mcpList() + case "add": + return h.mcpAdd(args[1:]) default: return fmt.Sprintf("Unknown mcp subcommand: %s", args[0]) } } +// mcpAddUsage is the usage string returned when /mcp add is called with +// missing required flags or no positional name. +const mcpAddUsage = "Usage: /mcp add --command [--transport stdio|http|websocket] [--args \"a,b\"] [--env \"K=V,K=V\"] [--timeout 120]" + +// mcpAdd registers a new MCP upstream from /mcp add arguments. +func (h *CommandHandler) mcpAdd(args []string) string { + if len(args) == 0 { + return mcpAddUsage + } + + // Extract known flags. extractFlag returns empty string when the flag is + // absent; the remaining slice collapses the flag/value pair out of args. + command, args := extractFlag(args, "--command") + transport, args := extractFlag(args, "--transport") + argsStr, args := extractFlag(args, "--args") + envStr, args := extractFlag(args, "--env") + timeoutStr, args := extractFlag(args, "--timeout") + + if len(args) == 0 { + return mcpAddUsage + } + name := args[0] + // Reject stray positional args after so typos like + // "/mcp add foo bar --command cmd" don't silently swallow "bar". + if len(args) > 1 { + return fmt.Sprintf("Unexpected argument %q.\n%s", args[1], mcpAddUsage) + } + + if command == "" { + return mcpAddUsage + } + + if err := mcp.ValidateUpstreamName(name); err != nil { + return fmt.Sprintf("Invalid upstream name: %v", err) + } + + if transport == "" { + transport = "stdio" + } + if !mcp.ValidTransport(transport) { + return fmt.Sprintf("Invalid transport %q: must be stdio, http, or websocket", transport) + } + + timeout := 120 + if timeoutStr != "" { + n, err := strconv.Atoi(timeoutStr) + if err != nil || n <= 0 { + return fmt.Sprintf("Invalid --timeout %q: must be a positive integer (seconds)", timeoutStr) + } + timeout = n + } + + var cmdArgs []string + if argsStr != "" { + cmdArgs = strings.Split(argsStr, ",") + } + + env := make(map[string]string) + if envStr != "" { + for _, kv := range strings.Split(envStr, ",") { + parts := strings.SplitN(kv, "=", 2) + if len(parts) != 2 { + return fmt.Sprintf("Invalid --env %q: expected KEY=VAL[,KEY=VAL,...]", envStr) + } + env[parts[0]] = parts[1] + } + } + + h.reloadMu.Lock() + defer h.reloadMu.Unlock() + + id, err := h.store.AddMCPUpstream(name, command, store.MCPUpstreamOpts{ + Args: cmdArgs, + Env: env, + TimeoutSec: timeout, + Transport: transport, + }) + if err != nil { + return fmt.Sprintf("Failed to add MCP upstream: %v", err) + } + + return fmt.Sprintf( + "Added MCP upstream [%d] %s (%s)\nRestart sluice for the new upstream to take effect.", + id, htmlEscape(name), htmlEscape(transport), + ) +} + // mcpList renders all registered MCP upstreams for Telegram display. func (h *CommandHandler) mcpList() string { upstreams, err := h.store.ListMCPUpstreams() diff --git a/internal/telegram/commands_test.go b/internal/telegram/commands_test.go index fa1c68d..b3382d4 100644 --- a/internal/telegram/commands_test.go +++ b/internal/telegram/commands_test.go @@ -918,6 +918,285 @@ func TestHandleMCPListEscapesHTML(t *testing.T) { } } +func TestHandleMCPAddStdio(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx"}, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + if !strings.Contains(result, "github") || !strings.Contains(result, "stdio") { + t.Errorf("expected name and transport in response, got: %s", result) + } + if !strings.Contains(result, "Restart sluice") { + t.Errorf("expected restart notice in response, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(upstreams)) + } + u := upstreams[0] + if u.Name != "github" { + t.Errorf("name = %q, want %q", u.Name, "github") + } + if u.Command != "npx" { + t.Errorf("command = %q, want %q", u.Command, "npx") + } + if u.Transport != "stdio" { + t.Errorf("transport = %q, want stdio", u.Transport) + } + if u.TimeoutSec != 120 { + t.Errorf("timeout = %d, want 120", u.TimeoutSec) + } + if len(u.Args) != 0 { + t.Errorf("args = %v, want empty", u.Args) + } + if len(u.Env) != 0 { + t.Errorf("env = %v, want empty", u.Env) + } +} + +func TestHandleMCPAddWithArgsAndEnv(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "github", "--command", "npx", + "--args", "-y,@modelcontextprotocol/server-github", + "--env", "GITHUB_PAT=vault:github_pat,DEBUG=1", + "--timeout", "60", + }, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(upstreams)) + } + u := upstreams[0] + wantArgs := []string{"-y", "@modelcontextprotocol/server-github"} + if len(u.Args) != len(wantArgs) { + t.Fatalf("args = %v, want %v", u.Args, wantArgs) + } + for i, a := range wantArgs { + if u.Args[i] != a { + t.Errorf("args[%d] = %q, want %q", i, u.Args[i], a) + } + } + if u.Env["GITHUB_PAT"] != "vault:github_pat" { + t.Errorf("env[GITHUB_PAT] = %q, want vault:github_pat", u.Env["GITHUB_PAT"]) + } + if u.Env["DEBUG"] != "1" { + t.Errorf("env[DEBUG] = %q, want 1", u.Env["DEBUG"]) + } + if u.TimeoutSec != 60 { + t.Errorf("timeout = %d, want 60", u.TimeoutSec) + } +} + +func TestHandleMCPAddHTTPTransport(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "notion", + "--command", "https://mcp.notion.com", + "--transport", "http", + }, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(upstreams)) + } + u := upstreams[0] + if u.Command != "https://mcp.notion.com" { + t.Errorf("command = %q, want URL", u.Command) + } + if u.Transport != "http" { + t.Errorf("transport = %q, want http", u.Transport) + } +} + +func TestHandleMCPAddWebSocketTransport(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "realtime", + "--command", "wss://mcp.example.com/ws", + "--transport", "websocket", + }, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 || upstreams[0].Transport != "websocket" { + t.Errorf("expected websocket upstream, got %+v", upstreams) + } +} + +func TestHandleMCPAddMissingCommand(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github"}, + }) + if !strings.Contains(result, "Usage:") { + t.Errorf("expected usage on missing --command, got: %s", result) + } + upstreams, _ := s.ListMCPUpstreams() + if len(upstreams) != 0 { + t.Errorf("no upstream should be created when --command is missing, got %d", len(upstreams)) + } +} + +func TestHandleMCPAddMissingName(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "--command", "npx"}, + }) + if !strings.Contains(result, "Usage:") { + t.Errorf("expected usage on missing name, got: %s", result) + } + upstreams, _ := s.ListMCPUpstreams() + if len(upstreams) != 0 { + t.Errorf("no upstream should be created without a name, got %d", len(upstreams)) + } +} + +func TestHandleMCPAddInvalidName(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + // "__" is a reserved namespace separator for the MCP gateway. + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "bad__name", "--command", "npx"}, + }) + if !strings.Contains(result, "Invalid upstream name") { + t.Errorf("expected invalid name error, got: %s", result) + } +} + +func TestHandleMCPAddInvalidTransport(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx", "--transport", "ftp"}, + }) + if !strings.Contains(result, "Invalid transport") { + t.Errorf("expected invalid transport error, got: %s", result) + } +} + +func TestHandleMCPAddInvalidTimeout(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + // non-numeric + if r := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx", "--timeout", "abc"}, + }); !strings.Contains(r, "Invalid --timeout") { + t.Errorf("expected invalid timeout error, got: %s", r) + } + // zero + if r := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx", "--timeout", "0"}, + }); !strings.Contains(r, "Invalid --timeout") { + t.Errorf("expected invalid timeout error for 0, got: %s", r) + } +} + +func TestHandleMCPAddInvalidEnv(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + // env value missing "=" + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx", "--env", "NOT_A_PAIR"}, + }) + if !strings.Contains(result, "Invalid --env") { + t.Errorf("expected invalid env error, got: %s", result) + } +} + +func TestHandleMCPAddDuplicateName(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{}); err != nil { + t.Fatal(err) + } + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "some-other"}, + }) + if !strings.Contains(result, "Failed to add MCP upstream") { + t.Errorf("expected duplicate rejection, got: %s", result) + } +} + +func TestHandleMCPAddStrayPositional(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + // Second positional arg should be rejected to avoid silently swallowing + // the intended upstream name when someone types /mcp add foo bar --command cmd. + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "foo", "bar", "--command", "npx"}, + }) + if !strings.Contains(result, "Unexpected argument") { + t.Errorf("expected rejection of stray arg, got: %s", result) + } + upstreams, _ := s.ListMCPUpstreams() + if len(upstreams) != 0 { + t.Errorf("no upstream should be created on parse failure, got %d", len(upstreams)) + } +} + func TestCredAddEnvVarConsumedFromValue(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") From 11f772b0bdfecc57cd0bc3882aa5547ac9c72046 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 14:05:13 +0800 Subject: [PATCH 3/8] feat(telegram): add /mcp remove with MCP config re-injection --- cmd/sluice/main.go | 1 + docs/plans/20260408-mcp-upstream-commands.md | 8 +- internal/telegram/approval.go | 4 + internal/telegram/approval_test.go | 11 +- internal/telegram/commands.go | 60 ++++- internal/telegram/commands_test.go | 221 +++++++++++++++++++ 6 files changed, 298 insertions(+), 7 deletions(-) diff --git a/cmd/sluice/main.go b/cmd/sluice/main.go index 9e7cd6d..0572caf 100644 --- a/cmd/sluice/main.go +++ b/cmd/sluice/main.go @@ -377,6 +377,7 @@ func main() { Vault: vaultStore, ContainerMgr: containerMgr, Store: db, + MCPURL: deriveMCPBaseURL(*mcpBaseURL, *healthAddr), OnEngineSwap: srv.UpdateInspectRules, OnOAuthIndexRebuild: func() { if db == nil { diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/20260408-mcp-upstream-commands.md index dc9b886..a33b0aa 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/20260408-mcp-upstream-commands.md @@ -76,10 +76,10 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. - Modify: `internal/telegram/commands.go` - Test: `internal/telegram/commands_test.go` -- [ ] `/mcp remove ` - remove upstream by name -- [ ] After add/remove: trigger MCP config re-injection into agent container. Add `mcpDir` field to `CommandHandler` (or accept it via constructor) so the handler can call the same MCP config write logic used in `cmd/sluice/main.go` -- [ ] Write tests for remove subcommand and re-injection trigger -- [ ] Run tests - must pass before next task +- [x] `/mcp remove ` - remove upstream by name +- [x] After add/remove: trigger MCP config re-injection into agent container. Added `mcpURL` field to `CommandHandler` with `SetMCPURL` setter and `MCPURL` in `ChannelConfig`. The `reinjectMCPConfig` helper calls `ContainerManager.WireMCPGateway(ctx, "sluice", mcpURL)` after both `/mcp add` and `/mcp remove` successes. Wired from `cmd/sluice/main.go` via `deriveMCPBaseURL(*mcpBaseURL, *healthAddr)`. (Plan referenced an older `mcpDir`/`mcp-servers.json` write path; current code uses `WireMCPGateway` WebSocket RPC instead.) +- [x] Write tests for remove subcommand and re-injection trigger +- [x] Run tests - must pass before next task ### Task 4: Add /mcp to Telegram command menu diff --git a/internal/telegram/approval.go b/internal/telegram/approval.go index 604cb2a..3693890 100644 --- a/internal/telegram/approval.go +++ b/internal/telegram/approval.go @@ -33,6 +33,7 @@ type ChannelConfig struct { Vault *vault.Store ContainerMgr container.ContainerManager Store *store.Store + MCPURL string // external URL for sluice's MCP gateway, used to re-wire the agent's config after /mcp add|remove OnEngineSwap func(eng *policy.Engine) // called after policy mutations to update dependent state OnOAuthIndexRebuild func() // called after credential removal to rebuild proxy OAuth index APIEndpoint string // custom Telegram API endpoint (for testing); empty uses default @@ -87,6 +88,9 @@ func NewTelegramChannel(cfg ChannelConfig) (*TelegramChannel, error) { if cfg.ContainerMgr != nil { cmdHandler.SetContainerManager(cfg.ContainerMgr) } + if cfg.MCPURL != "" { + cmdHandler.SetMCPURL(cfg.MCPURL) + } if cfg.ResolverPtr != nil { cmdHandler.SetResolverPtr(cfg.ResolverPtr) } diff --git a/internal/telegram/approval_test.go b/internal/telegram/approval_test.go index f39f9a0..b3d9bbe 100644 --- a/internal/telegram/approval_test.go +++ b/internal/telegram/approval_test.go @@ -1883,6 +1883,10 @@ type mockContainerMgr struct { injectErr error restartCalled bool restartErr error + wireCalled bool + wireName string + wireURL string + wireErr error } func (m *mockContainerMgr) InjectEnvVars(_ context.Context, envMap map[string]string, _ bool) error { @@ -1896,8 +1900,11 @@ func (m *mockContainerMgr) RestartWithEnv(_ context.Context, _ map[string]string return m.restartErr } -func (m *mockContainerMgr) WireMCPGateway(_ context.Context, _, _ string) error { - return nil +func (m *mockContainerMgr) WireMCPGateway(_ context.Context, name, url string) error { + m.wireCalled = true + m.wireName = name + m.wireURL = url + return m.wireErr } func (m *mockContainerMgr) Status(_ context.Context) (container.ContainerStatus, error) { diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index 2b6e049..da49a87 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -59,6 +59,7 @@ type CommandHandler struct { vault *vault.Store containerMgr container.ContainerManager store *store.Store + mcpURL string // external URL for sluice's MCP gateway; empty disables re-injection onEngineSwap func(eng *policy.Engine) // called after engine swap to update dependent state onOAuthIndexRebuild func() // called after credential removal to rebuild proxy OAuth index } @@ -78,6 +79,13 @@ func (h *CommandHandler) SetStore(s *store.Store) { h.store = s } +// SetMCPURL sets the external URL used to re-wire sluice's MCP gateway into +// the agent container's config after /mcp add and /mcp remove. Empty disables +// re-injection (which is also a no-op when no container manager is configured). +func (h *CommandHandler) SetMCPURL(url string) { + h.mcpURL = url +} + // SetResolverPtr shares the proxy's binding resolver pointer so credential // mutations can update the live binding snapshot without requiring SIGHUP. func (h *CommandHandler) SetResolverPtr(ptr *atomic.Pointer[vault.BindingResolver]) { @@ -677,6 +685,8 @@ func (h *CommandHandler) handleMCP(args []string) string { return h.mcpList() case "add": return h.mcpAdd(args[1:]) + case "remove": + return h.mcpRemove(args[1:]) default: return fmt.Sprintf("Unknown mcp subcommand: %s", args[0]) } @@ -763,10 +773,58 @@ func (h *CommandHandler) mcpAdd(args []string) string { return fmt.Sprintf("Failed to add MCP upstream: %v", err) } - return fmt.Sprintf( + msg := fmt.Sprintf( "Added MCP upstream [%d] %s (%s)\nRestart sluice for the new upstream to take effect.", id, htmlEscape(name), htmlEscape(transport), ) + return msg + h.reinjectMCPConfig() +} + +// mcpRemove removes an MCP upstream by name and triggers re-injection so the +// agent picks up the change. Returns a human-readable message for Telegram. +func (h *CommandHandler) mcpRemove(args []string) string { + if len(args) == 0 { + return "Usage: /mcp remove " + } + name := args[0] + if len(args) > 1 { + return fmt.Sprintf("Unexpected argument %q.\nUsage: /mcp remove ", args[1]) + } + + h.reloadMu.Lock() + defer h.reloadMu.Unlock() + + deleted, err := h.store.RemoveMCPUpstream(name) + if err != nil { + return fmt.Sprintf("Failed to remove MCP upstream: %v", err) + } + if !deleted { + return fmt.Sprintf("No MCP upstream named %s", htmlCode(name)) + } + + msg := fmt.Sprintf( + "Removed MCP upstream %s\nRestart sluice for the removal to take effect.", + htmlCode(name), + ) + return msg + h.reinjectMCPConfig() +} + +// reinjectMCPConfig re-wires sluice's MCP gateway URL into the agent +// container's config. Returns a trailing message suffix (prefixed with \n) +// describing the outcome, or "" if re-injection is not configured (e.g. no +// container manager, no MCP URL, or running without a runtime). Callers +// append the return value to their base response. +func (h *CommandHandler) reinjectMCPConfig() string { + if h.containerMgr == nil || h.mcpURL == "" { + return "" + } + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + if err := h.containerMgr.WireMCPGateway(ctx, "sluice", h.mcpURL); err != nil { + return "\nWarning: failed to re-wire MCP config: " + err.Error() + } + return "\nAgent MCP config re-wired." } // mcpList renders all registered MCP upstreams for Telegram display. diff --git a/internal/telegram/commands_test.go b/internal/telegram/commands_test.go index b3382d4..348c4f3 100644 --- a/internal/telegram/commands_test.go +++ b/internal/telegram/commands_test.go @@ -1,6 +1,7 @@ package telegram import ( + "fmt" "os" "path/filepath" "strconv" @@ -1227,3 +1228,223 @@ func TestCredAddEnvVarConsumedFromValue(t *testing.T) { t.Errorf("expected credential value 'the-secret-value', got %q", string(sb.Bytes())) } } + +func TestHandleMCPRemove(t *testing.T) { + s := newTestStore(t) + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{ + Transport: "stdio", + }); err != nil { + t.Fatal(err) + } + if _, err := s.AddMCPUpstream("notion", "https://mcp.notion.com", store.MCPUpstreamOpts{ + Transport: "http", + }); err != nil { + t.Fatal(err) + } + + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "github"}}) + + if !strings.Contains(result, "Removed MCP upstream") { + t.Errorf("expected removal confirmation, got: %s", result) + } + if !strings.Contains(result, "github") { + t.Errorf("expected removed name in response, got: %s", result) + } + if !strings.Contains(result, "Restart sluice") { + t.Errorf("expected restart notice, got: %s", result) + } + + // Verify github was removed but notion remains. + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 remaining upstream, got %d", len(upstreams)) + } + if upstreams[0].Name != "notion" { + t.Errorf("wrong upstream remained: %q", upstreams[0].Name) + } +} + +func TestHandleMCPRemoveMissingName(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove"}}) + + if !strings.Contains(result, "Usage: /mcp remove") { + t.Errorf("expected usage on missing name, got: %s", result) + } +} + +func TestHandleMCPRemoveNotFound(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "nonexistent"}}) + + if !strings.Contains(result, "No MCP upstream named") { + t.Errorf("expected not-found message, got: %s", result) + } +} + +func TestHandleMCPRemoveStrayPositional(t *testing.T) { + s := newTestStore(t) + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{Transport: "stdio"}); err != nil { + t.Fatal(err) + } + + handler := newTestHandlerWithStore(t, s, nil, "") + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "github", "extra"}}) + + if !strings.Contains(result, "Unexpected argument") { + t.Errorf("expected stray arg rejection, got: %s", result) + } + + // No-op removal: github must still exist. + upstreams, _ := s.ListMCPUpstreams() + if len(upstreams) != 1 { + t.Errorf("upstream should not be removed on parse failure, got %d", len(upstreams)) + } +} + +// TestHandleMCPAddTriggersReinjection verifies that /mcp add re-wires the +// agent's MCP config via WireMCPGateway when a container manager and MCP URL +// are configured. +func TestHandleMCPAddTriggersReinjection(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + mgr := &mockContainerMgr{} + handler.SetContainerManager(mgr) + handler.SetMCPURL("http://sluice:3000/mcp") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx"}, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + if !mgr.wireCalled { + t.Errorf("WireMCPGateway should be called after /mcp add") + } + if mgr.wireName != "sluice" { + t.Errorf("wireName = %q, want %q", mgr.wireName, "sluice") + } + if mgr.wireURL != "http://sluice:3000/mcp" { + t.Errorf("wireURL = %q, want %q", mgr.wireURL, "http://sluice:3000/mcp") + } + if !strings.Contains(result, "Agent MCP config re-wired") { + t.Errorf("expected re-wired notice in response, got: %s", result) + } +} + +// TestHandleMCPRemoveTriggersReinjection verifies that /mcp remove re-wires +// the agent's MCP config via WireMCPGateway when a container manager and MCP +// URL are configured. +func TestHandleMCPRemoveTriggersReinjection(t *testing.T) { + s := newTestStore(t) + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{ + Transport: "stdio", + }); err != nil { + t.Fatal(err) + } + + handler := newTestHandlerWithStore(t, s, nil, "") + + mgr := &mockContainerMgr{} + handler.SetContainerManager(mgr) + handler.SetMCPURL("http://sluice:3000/mcp") + + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "github"}}) + if !strings.Contains(result, "Removed MCP upstream") { + t.Fatalf("should confirm remove, got: %s", result) + } + if !mgr.wireCalled { + t.Errorf("WireMCPGateway should be called after /mcp remove") + } + if mgr.wireName != "sluice" { + t.Errorf("wireName = %q, want %q", mgr.wireName, "sluice") + } + if mgr.wireURL != "http://sluice:3000/mcp" { + t.Errorf("wireURL = %q, want %q", mgr.wireURL, "http://sluice:3000/mcp") + } + if !strings.Contains(result, "Agent MCP config re-wired") { + t.Errorf("expected re-wired notice in response, got: %s", result) + } +} + +// TestHandleMCPReinjectionSkippedWithoutContainer verifies re-injection is a +// no-op when no container manager is configured (standalone mode). +func TestHandleMCPReinjectionSkippedWithoutContainer(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + handler.SetMCPURL("http://sluice:3000/mcp") + // Intentionally do not set a container manager. + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx"}, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + if strings.Contains(result, "Agent MCP config re-wired") { + t.Errorf("re-wired notice should not appear without container manager, got: %s", result) + } + if strings.Contains(result, "Warning") { + t.Errorf("no warning should appear when re-injection is simply skipped, got: %s", result) + } +} + +// TestHandleMCPReinjectionSkippedWithoutURL verifies re-injection is a no-op +// when no MCP URL is configured. +func TestHandleMCPReinjectionSkippedWithoutURL(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + mgr := &mockContainerMgr{} + handler.SetContainerManager(mgr) + // Intentionally do not set an MCP URL. + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx"}, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + if mgr.wireCalled { + t.Errorf("WireMCPGateway should not be called without an MCP URL") + } +} + +// TestHandleMCPReinjectionFailure verifies failures from WireMCPGateway are +// surfaced to the Telegram response as a warning but do not fail the overall +// add/remove operation. +func TestHandleMCPReinjectionFailure(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + mgr := &mockContainerMgr{wireErr: fmt.Errorf("wire failed: exec timeout")} + handler.SetContainerManager(mgr) + handler.SetMCPURL("http://sluice:3000/mcp") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx"}, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add even when re-injection fails, got: %s", result) + } + if !strings.Contains(result, "Warning: failed to re-wire") { + t.Errorf("expected warning about wire failure, got: %s", result) + } + + // The upstream should still be persisted. + upstreams, _ := s.ListMCPUpstreams() + if len(upstreams) != 1 { + t.Errorf("upstream should be persisted even if re-injection fails, got %d", len(upstreams)) + } +} From 8e4eadab095332265531705045fcd7396188eba3 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 14:11:53 +0800 Subject: [PATCH 4/8] feat(telegram): register /mcp command and add MCP help section --- docs/plans/20260408-mcp-upstream-commands.md | 8 +- internal/telegram/approval.go | 1 + internal/telegram/approval_test.go | 89 ++++++++++++++++++-- internal/telegram/commands.go | 8 ++ internal/telegram/commands_test.go | 28 ++++++ 5 files changed, 125 insertions(+), 9 deletions(-) diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/20260408-mcp-upstream-commands.md index a33b0aa..edfac24 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/20260408-mcp-upstream-commands.md @@ -87,10 +87,10 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. - Modify: `internal/telegram/approval.go` - Test: `internal/telegram/approval_test.go` -- [ ] Add `{Command: "mcp", Description: "Manage MCP upstreams"}` to registerCommands -- [ ] Update help output to include MCP section -- [ ] Write tests for updated help and command registration -- [ ] Run tests - must pass before next task +- [x] Add `{Command: "mcp", Description: "Manage MCP upstreams"}` to registerCommands +- [x] Update help output to include MCP section +- [x] Write tests for updated help and command registration +- [x] Run tests - must pass before next task ### Task 5: Verify acceptance criteria diff --git a/internal/telegram/approval.go b/internal/telegram/approval.go index 3693890..8969291 100644 --- a/internal/telegram/approval.go +++ b/internal/telegram/approval.go @@ -224,6 +224,7 @@ func (tc *TelegramChannel) registerCommands() { {Command: "status", Description: "Show proxy status"}, {Command: "policy", Description: "Manage policy rules"}, {Command: "cred", Description: "Manage credentials"}, + {Command: "mcp", Description: "Manage MCP upstreams"}, {Command: "audit", Description: "Show audit log entries"}, {Command: "start", Description: "Show welcome message"}, {Command: "help", Description: "Show available commands"}, diff --git a/internal/telegram/approval_test.go b/internal/telegram/approval_test.go index b3d9bbe..94777ea 100644 --- a/internal/telegram/approval_test.go +++ b/internal/telegram/approval_test.go @@ -48,11 +48,12 @@ type tgResponse struct { type mockTelegramAPI struct { server *httptest.Server - mu sync.Mutex - sentMessages []tgbotapi.MessageConfig - editedMsgs []tgbotapi.EditMessageTextConfig - callbacks []tgbotapi.CallbackConfig - deletedMsgs []tgbotapi.DeleteMessageConfig + mu sync.Mutex + sentMessages []tgbotapi.MessageConfig + editedMsgs []tgbotapi.EditMessageTextConfig + callbacks []tgbotapi.CallbackConfig + deletedMsgs []tgbotapi.DeleteMessageConfig + setCommandsRaw []string // raw JSON payloads received by setMyCommands nextMsgID int updates chan []tgbotapi.Update @@ -141,6 +142,14 @@ func newMockTelegramAPI(t *testing.T) *mockTelegramAPI { _ = json.NewEncoder(w).Encode(tgResponse{OK: true, Result: json.RawMessage(`[]`)}) } + case "setMyCommands": + _ = r.ParseForm() + raw := r.FormValue("commands") + m.mu.Lock() + m.setCommandsRaw = append(m.setCommandsRaw, raw) + m.mu.Unlock() + _ = json.NewEncoder(w).Encode(tgResponse{OK: true, Result: json.RawMessage(`true`)}) + default: _ = json.NewEncoder(w).Encode(tgResponse{OK: true, Result: json.RawMessage(`true`)}) } @@ -185,6 +194,14 @@ func (m *mockTelegramAPI) getDeletedMessages() []tgbotapi.DeleteMessageConfig { return out } +func (m *mockTelegramAPI) getSetCommandsRaw() []string { + m.mu.Lock() + defer m.mu.Unlock() + out := make([]string, len(m.setCommandsRaw)) + copy(out, m.setCommandsRaw) + return out +} + func escapeJSON(s string) string { b, _ := json.Marshal(s) // Strip surrounding quotes. @@ -546,6 +563,68 @@ func TestCancelApprovalShowsShutdownReason(t *testing.T) { // --- Start/Stop lifecycle tests --- +// TestRegisterCommands verifies that Start registers the bot command menu +// with the expected entries, including /mcp for MCP upstream management. +func TestRegisterCommands(t *testing.T) { + mock := newMockTelegramAPI(t) + s := newTestStore(t) + tc := newTestTelegramChannel(t, mock, s) + + if err := tc.Start(); err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(tc.Stop) + + // Wait for setMyCommands to be called (happens synchronously inside Start). + deadline := time.After(3 * time.Second) + var raw []string + for { + raw = mock.getSetCommandsRaw() + if len(raw) > 0 { + break + } + select { + case <-deadline: + t.Fatal("timed out waiting for setMyCommands call") + default: + time.Sleep(10 * time.Millisecond) + } + } + + // The payload is a JSON array of {command,description} objects. Parse it + // and verify every expected command is present. + var cmds []tgbotapi.BotCommand + if err := json.Unmarshal([]byte(raw[0]), &cmds); err != nil { + t.Fatalf("unmarshal commands payload: %v (raw=%q)", err, raw[0]) + } + + want := map[string]string{ + "status": "Show proxy status", + "policy": "Manage policy rules", + "cred": "Manage credentials", + "mcp": "Manage MCP upstreams", + "audit": "Show audit log entries", + "start": "Show welcome message", + "help": "Show available commands", + } + + got := make(map[string]string, len(cmds)) + for _, c := range cmds { + got[c.Command] = c.Description + } + + for name, desc := range want { + gotDesc, ok := got[name] + if !ok { + t.Errorf("missing command /%s in setMyCommands payload", name) + continue + } + if gotDesc != desc { + t.Errorf("/%s description = %q, want %q", name, gotDesc, desc) + } + } +} + func TestStartStop(t *testing.T) { mock := newMockTelegramAPI(t) s := newTestStore(t) diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index da49a87..01caece 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -958,6 +958,14 @@ Credentials /cred rotate | /cred remove ` } + if h.store != nil { + help += ` + +MCP Upstreams +/mcp list | /mcp add --command [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V,K=V"] [--timeout 120] +/mcp remove ` + } + help += ` More: /start for welcome, /help for this message` diff --git a/internal/telegram/commands_test.go b/internal/telegram/commands_test.go index 348c4f3..a95a8e8 100644 --- a/internal/telegram/commands_test.go +++ b/internal/telegram/commands_test.go @@ -414,6 +414,34 @@ func TestHandleHelp(t *testing.T) { if !strings.Contains(result, "/audit") { t.Error("help should mention /audit") } + if !strings.Contains(result, "/mcp") { + t.Error("help should mention /mcp when store is configured") + } + if !strings.Contains(result, "MCP Upstreams") { + t.Error("help should include MCP Upstreams section when store is configured") + } +} + +// TestHandleHelpNoStore verifies the MCP section is omitted when store is nil, +// matching how /cred help is gated on vault availability. +func TestHandleHelpNoStore(t *testing.T) { + ptr := new(atomic.Pointer[policy.Engine]) + eng, err := policy.LoadFromBytes([]byte(`[policy] +default = "deny" +`)) + if err != nil { + t.Fatal(err) + } + ptr.Store(eng) + handler := NewCommandHandler(ptr, new(sync.Mutex), "") + result := handler.Handle(&Command{Name: "help"}) + + if strings.Contains(result, "/mcp") { + t.Error("help should not mention /mcp when store is nil") + } + if strings.Contains(result, "MCP Upstreams") { + t.Error("help should not include MCP Upstreams section when store is nil") + } } func TestHandleCredNoVault(t *testing.T) { From 29da52c0b70dad20c646d480ceacdb41803d404d Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 14:13:33 +0800 Subject: [PATCH 5/8] chore(telegram): verify /mcp acceptance criteria --- docs/plans/20260408-mcp-upstream-commands.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/20260408-mcp-upstream-commands.md index edfac24..4924226 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/20260408-mcp-upstream-commands.md @@ -94,10 +94,10 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. ### Task 5: Verify acceptance criteria -- [ ] Verify `/mcp list` in Telegram shows upstreams -- [ ] Verify `/mcp add` creates upstream and triggers auto-injection -- [ ] Verify `/mcp remove` removes upstream -- [ ] Run full test suite: `go test ./... -v -timeout 30s` +- [x] Verify `/mcp list` in Telegram shows upstreams (verified via unit tests: `TestHandleMCPListEmpty`, `TestHandleMCPListWithUpstreams`, `TestHandleMCPListEscapesHTML`, `TestHandleMessageMCPListNotDeleted`) +- [x] Verify `/mcp add` creates upstream and triggers auto-injection (verified via unit tests: `TestHandleMCPAddStdio`, `TestHandleMCPAddWithArgsAndEnv`, `TestHandleMCPAddHTTPTransport`, `TestHandleMCPAddWebSocketTransport`, `TestHandleMCPAddTriggersReinjection`, `TestHandleMessageMCPAddDeletesMessage`) +- [x] Verify `/mcp remove` removes upstream (verified via unit tests: `TestHandleMCPRemove`, `TestHandleMCPRemoveNotFound`, `TestHandleMCPRemoveStrayPositional`, `TestHandleMCPRemoveTriggersReinjection`) +- [x] Run full test suite: `go test ./... -v -timeout 30s` -- all 2389 tests passing across 12 packages ### Task 6: [Final] Update documentation From 6d24a6e12a341e8326ac4b592f5c57c3fb245dfc Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 14:15:22 +0800 Subject: [PATCH 6/8] docs: mark MCP upstream commands plan complete --- docs/plans/{ => completed}/20260408-mcp-upstream-commands.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename docs/plans/{ => completed}/20260408-mcp-upstream-commands.md (95%) diff --git a/docs/plans/20260408-mcp-upstream-commands.md b/docs/plans/completed/20260408-mcp-upstream-commands.md similarity index 95% rename from docs/plans/20260408-mcp-upstream-commands.md rename to docs/plans/completed/20260408-mcp-upstream-commands.md index 4924226..a4e81e2 100644 --- a/docs/plans/20260408-mcp-upstream-commands.md +++ b/docs/plans/completed/20260408-mcp-upstream-commands.md @@ -101,8 +101,8 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. ### Task 6: [Final] Update documentation -- [ ] Update CLAUDE.md CLI subcommands section if needed -- [ ] Move this plan to `docs/plans/completed/` +- [x] Update CLAUDE.md CLI subcommands section if needed (no update needed: CLAUDE.md has no Telegram commands listing section, and the CLI subcommands section already documents `sluice mcp add/list/remove`) +- [x] Move this plan to `docs/plans/completed/` ## Post-Completion From 5e8957fa22b09abc69c7182ef05e459a5c871144 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Sat, 18 Apr 2026 17:50:35 +0800 Subject: [PATCH 7/8] fix(telegram): address review findings Squashed review fixes across multiple iterations: - MCP upstream commands review findings - Iter 2 review findings - Iter 3 review findings - Smells review style fixes - Codex external review findings --- CLAUDE.md | 8 +- README.md | 5 + cmd/sluice/flagutil.go | 70 +-- cmd/sluice/main.go | 1 - cmd/sluice/mcp.go | 12 +- cmd/sluice/mcp_test.go | 92 +++- .../20260408-mcp-upstream-commands.md | 18 +- internal/flagutil/flagutil.go | 78 +++ internal/flagutil/flagutil_test.go | 136 +++++ internal/mcp/gateway.go | 2 +- internal/mcp/upstream.go | 7 +- internal/store/store.go | 5 + internal/telegram/approval.go | 12 +- internal/telegram/approval_test.go | 200 ++++--- internal/telegram/commands.go | 413 +++++++++----- internal/telegram/commands_test.go | 514 ++++++++++++++---- 16 files changed, 1192 insertions(+), 381 deletions(-) create mode 100644 internal/flagutil/flagutil.go create mode 100644 internal/flagutil/flagutil_test.go diff --git a/CLAUDE.md b/CLAUDE.md index cd1c5f5..8787ede 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,7 +88,7 @@ sluice policy remove sluice policy import # seed DB from TOML (merge semantics) sluice policy export # dump current rules as TOML -sluice mcp add --command [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V"] [--timeout 120] +sluice mcp add --command [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V"] [--header "K=V"] [--timeout 120] sluice mcp list sluice mcp remove sluice mcp # start MCP gateway @@ -128,7 +128,9 @@ docker exec openclaw openclaw mcp set sluice '{"url":"http://sluice:3000/mcp"}' For the hostname `sluice` to resolve inside OpenClaw, the compose file pins sluice's IP on the internal network (172.30.0.2) and adds an `extra_hosts` entry on tun2proxy (which OpenClaw shares). Docker's embedded DNS (127.0.0.11) is not reachable from OpenClaw because its DNS is routed through the TUN device. The `/etc/hosts` entry bypasses DNS entirely. -When new MCP upstreams are added to sluice via `sluice mcp add`, restart sluice so the gateway picks them up. OpenClaw does not need to be restarted - its connection to sluice:3000/mcp remains valid and it re-queries the tool list on subsequent agent runs. +MCP upstreams can be managed via `sluice mcp add|list|remove`, the REST API (`/api/mcp/upstreams`), or the Telegram bot (`/mcp add|list|remove`). All three paths write to the same SQLite store. After any addition or removal, restart sluice so the gateway re-reads the upstream set. OpenClaw does not need to be restarted: its connection to `sluice:3000/mcp` is registered once at sluice startup (via `WireMCPGateway`, which patches `mcp.servers.sluice = {url: ...}` in the agent's openclaw.json) and stays valid across sluice restarts. The agent re-queries the tool list on subsequent agent runs. + +The Telegram `/mcp add` path auto-deletes the chat message because `--env KEY=VAL` pairs may contain secrets (use `KEY=vault:name` to keep the plaintext out of the SQLite store and `/mcp list` output entirely). ## Policy Store @@ -260,7 +262,7 @@ Action names operators commonly grep for: `tool_call` (MCP tool call policy verd ### MCP gateway -Three upstream transports: stdio (child processes), Streamable HTTP, WebSocket. All satisfy `MCPUpstream` interface. Tools namespaced as `__`. Policy evaluation: deny/allow/ask priority. `ContentInspector` blocks arguments and redacts responses using regex (JSON parsed before matching to prevent unicode escape bypass). Per-upstream timeouts (default 120s). +Three upstream transports: stdio (child processes), Streamable HTTP, WebSocket. All satisfy `MCPUpstream` interface. Tools namespaced as `__`. Policy evaluation: deny/allow/ask priority. `ContentInspector` blocks arguments and redacts responses using regex (JSON parsed before matching to prevent unicode escape bypass). Per-upstream timeout defaults are defined by the `mcp.DefaultTimeoutSec` constant (120s) shared across packages that need the fallback. `internal/store.AddMCPUpstream` duplicates the literal 120 because `internal/store` is imported by `internal/mcp` and cannot import it back (circular). A comment in `store.go` flags the duplicate so the two stay in sync. `MCPHTTPHandler` serves `POST /mcp` and `DELETE /mcp` on port 3000 (alongside `/healthz`). Session tracking via `Mcp-Session-Id` header. SSE response support. diff --git a/README.md b/README.md index db93907..65e4ff3 100644 --- a/README.md +++ b/README.md @@ -285,9 +285,14 @@ Manage sluice from your phone. Approve connections and tool calls, add credentia | `/policy deny ` | Add deny rule | | `/cred add [--env-var VAR]` | Add credential (value sent as next message, auto-deleted) | | `/cred rotate ` | Replace credential, hot-reload OpenClaw | +| `/mcp list` | List registered MCP upstreams | +| `/mcp add --command [flags]` | Register a new MCP upstream (stdio/http/websocket, see `/help`; chat message auto-deleted because `--env` may carry secrets) | +| `/mcp remove ` | Remove an MCP upstream | | `/status` | Proxy stats and pending approvals | | `/audit recent [N]` | Last N audit entries | +`/mcp add` only writes to the SQLite store. The MCP gateway builds its upstream set at startup, so restart sluice (`docker compose restart sluice`) for a new or removed upstream to take effect. The agent's connection to `sluice:3000/mcp` survives the restart. + ### HTTP Webhooks REST API on port 3000 for programmatic approval integration. `GET /api/approvals` lists pending requests, `POST /api/approvals/{id}/resolve` resolves them. Use this to build custom approval UIs or integrate with existing workflows. diff --git a/cmd/sluice/flagutil.go b/cmd/sluice/flagutil.go index a1bb819..e930f5a 100644 --- a/cmd/sluice/flagutil.go +++ b/cmd/sluice/flagutil.go @@ -5,6 +5,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/nemirovsky/sluice/internal/flagutil" ) // parsePortsList parses a comma-separated string of port numbers into a @@ -55,69 +57,9 @@ func parseProtocolsList(s string) ([]string, error) { return protocols, nil } -// reorderFlagsBeforePositional returns a copy of args with all flag -// arguments moved before any positional arguments, so that Go's stdlib -// flag parser (which stops at the first non-flag) still sees every flag. -// -// The FlagSet is consulted to determine which flags take a value and -// therefore consume the following arg. "--flag=value" form is left as -// a single token. "--" is treated as a terminator; everything after it -// is positional, preserving the stdlib convention. -// -// Example: ["github", "--command", "https://x", "--timeout", "60"] -// becomes ["--command", "https://x", "--timeout", "60", "github"] -// -// Flags defined as bool do not consume the following arg. Everything -// else (string, int, Func, Var) is assumed to. +// reorderFlagsBeforePositional is a thin alias over flagutil.ReorderFlagsBeforePositional +// so existing cmd/sluice callers (and their many tests) keep the old signature +// without churn. New code should import internal/flagutil directly. func reorderFlagsBeforePositional(args []string, fs *flag.FlagSet) []string { - var flagArgs, positional []string - i := 0 - for i < len(args) { - a := args[i] - if a == "--" { - // Terminator: everything after is positional, flag parsing - // should still see "--" to stop. - flagArgs = append(flagArgs, a) - positional = append(positional, args[i+1:]...) - break - } - if !strings.HasPrefix(a, "-") || a == "-" { - positional = append(positional, a) - i++ - continue - } - flagArgs = append(flagArgs, a) - // --flag=value form: value is in the same arg. - if strings.Contains(a, "=") { - i++ - continue - } - // Otherwise the next arg is the value for non-bool flags. - name := strings.TrimLeft(a, "-") - if isValueFlag(fs, name) && i+1 < len(args) { - flagArgs = append(flagArgs, args[i+1]) - i += 2 - continue - } - i++ - } - return append(flagArgs, positional...) -} - -// isValueFlag reports whether the named flag consumes the next argument -// as its value. Bool flags do not; everything else does. -func isValueFlag(fs *flag.FlagSet, name string) bool { - f := fs.Lookup(name) - if f == nil { - // Unknown flag. Assume it takes a value so we don't accidentally - // slurp something that might be a positional arg. fs.Parse will - // then surface the real error. - return true - } - // The stdlib flag package exposes bool flags via an IsBoolFlag method - // on the Value. Non-bool flags don't implement this. - if bf, ok := f.Value.(interface{ IsBoolFlag() bool }); ok && bf.IsBoolFlag() { - return false - } - return true + return flagutil.ReorderFlagsBeforePositional(args, fs) } diff --git a/cmd/sluice/main.go b/cmd/sluice/main.go index 0572caf..9e7cd6d 100644 --- a/cmd/sluice/main.go +++ b/cmd/sluice/main.go @@ -377,7 +377,6 @@ func main() { Vault: vaultStore, ContainerMgr: containerMgr, Store: db, - MCPURL: deriveMCPBaseURL(*mcpBaseURL, *healthAddr), OnEngineSwap: srv.UpdateInspectRules, OnOAuthIndexRebuild: func() { if db == nil { diff --git a/cmd/sluice/mcp.go b/cmd/sluice/mcp.go index 4f485b9..a98be24 100644 --- a/cmd/sluice/mcp.go +++ b/cmd/sluice/mcp.go @@ -261,12 +261,12 @@ func handleMCPAdd(args []string) error { command := fs.String("command", "", "command to run (stdio) or URL (http/websocket)") argsStr := fs.String("args", "", "comma-separated arguments for the command") envStr := fs.String("env", "", "comma-separated KEY=VAL environment variables (VAL may be vault: for the whole value, or contain {vault:} substrings for templated substitution)") - timeout := fs.Int("timeout", 120, "upstream timeout in seconds") + timeout := fs.Int("timeout", mcp.DefaultTimeoutSec, "upstream timeout in seconds") transport := fs.String("transport", "stdio", "transport type: stdio, http, or websocket") headers := make(map[string]string) fs.Func("header", "HTTP header to send on every request to an http upstream (repeatable, format: KEY=VAL; VAL may be vault: for the whole value, or contain {vault:} substrings for templated substitution, e.g. \"Authorization=Bearer {vault:github_pat}\")", func(s string) error { parts := strings.SplitN(s, "=", 2) - if len(parts) != 2 { + if len(parts) != 2 || parts[0] == "" { return fmt.Errorf("invalid header format %q (expected KEY=VAL)", s) } headers[parts[0]] = parts[1] @@ -291,6 +291,10 @@ func handleMCPAdd(args []string) error { return fmt.Errorf("invalid transport %q: must be stdio, http, or websocket", *transport) } + if *timeout <= 0 { + return fmt.Errorf("invalid --timeout %d: must be a positive integer (seconds)", *timeout) + } + if len(headers) > 0 && *transport != "http" { return fmt.Errorf("--header is only valid for --transport http") } @@ -304,7 +308,7 @@ func handleMCPAdd(args []string) error { if *envStr != "" { for _, kv := range strings.Split(*envStr, ",") { parts := strings.SplitN(kv, "=", 2) - if len(parts) != 2 { + if len(parts) != 2 || parts[0] == "" { return fmt.Errorf("invalid env format %q (expected KEY=VAL)", kv) } env[parts[0]] = parts[1] @@ -391,7 +395,7 @@ func handleMCPList(args []string) error { headersStr = " headers=" + strings.Join(pairs, ",") } timeoutStr := "" - if u.TimeoutSec != 120 { + if u.TimeoutSec != mcp.DefaultTimeoutSec { timeoutStr = fmt.Sprintf(" timeout=%ds", u.TimeoutSec) } fmt.Printf("[%d] %s command=%s%s%s%s%s%s\n", u.ID, u.Name, u.Command, transportStr, argsStr, envStr, headersStr, timeoutStr) diff --git a/cmd/sluice/mcp_test.go b/cmd/sluice/mcp_test.go index 2d3b164..d7f9ab4 100644 --- a/cmd/sluice/mcp_test.go +++ b/cmd/sluice/mcp_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/nemirovsky/sluice/internal/mcp" @@ -374,6 +375,56 @@ func TestHandleMCPAddInvalidName(t *testing.T) { } } +// TestHandleMCPAddInvalidTimeout verifies that --timeout <= 0 is rejected +// rather than being silently persisted. Previously the CLI accepted any +// integer (including 0 and negatives), but the runtime constructors fall +// back to DefaultTimeoutSec when TimeoutSec <= 0, so the persisted store +// value diverged from the actual runtime behavior. Telegram /mcp add has +// always rejected these values, this test locks in symmetry. +func TestHandleMCPAddInvalidTimeout(t *testing.T) { + cases := []struct { + name string + timeout string + }{ + {"zero", "0"}, + {"negative", "-5"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + dbPath := filepath.Join(dir, "test.db") + + err := handleMCPAdd([]string{ + "--db", dbPath, + "--command", "server", + "--timeout", tc.timeout, + "ups", + }) + if err == nil { + t.Fatalf("expected error for --timeout %s", tc.timeout) + } + if !strings.Contains(err.Error(), "must be a positive integer") { + t.Errorf("error %q should mention 'must be a positive integer'", err) + } + + // Nothing should be persisted when validation fails. + db, err := store.New(dbPath) + if err != nil { + t.Fatal(err) + } + defer func() { _ = db.Close() }() + upstreams, err := db.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 0 { + t.Errorf("expected 0 upstreams after invalid --timeout, got %d", len(upstreams)) + } + }) + } +} + // TestHandleMCPRemove verifies removing an upstream by name. func TestHandleMCPRemove(t *testing.T) { dir := t.TempDir() @@ -648,18 +699,41 @@ func TestHandleMCPGatewayInvalidChatID(t *testing.T) { } // TestHandleMCPAddEnvInvalidFormat verifies that bad env format is rejected. +// Covers both missing "=" (BADFORMAT) and empty-key (=VALUE) cases. The CLI +// rejects empty keys to match the Telegram handler's behavior so both entry +// points reject the same malformed inputs. func TestHandleMCPAddEnvInvalidFormat(t *testing.T) { dir := t.TempDir() - dbPath := filepath.Join(dir, "test.db") - err := handleMCPAdd([]string{ - "--db", dbPath, - "--command", "server", - "--env", "BADFORMAT", - "myserver", - }) - if err == nil { - t.Fatal("expected error for invalid env format") + cases := []struct { + label string + flag string + value string + }{ + {"env missing equals", "--env", "BADFORMAT"}, + {"env empty key", "--env", "=VALUE"}, + {"header missing equals", "--header", "BADFORMAT"}, + {"header empty key", "--header", "=VALUE"}, + } + + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + dbPath := filepath.Join(dir, tc.label+".db") + args := []string{ + "--db", dbPath, + "--command", "https://example.com/mcp", + "--transport", "http", + tc.flag, tc.value, + "myserver", + } + // The "http" transport is set so that --header is accepted for + // the header cases. The env cases do not require it, but sharing + // one arg set keeps the table loop uniform. + err := handleMCPAdd(args) + if err == nil { + t.Fatalf("expected error for %s %q", tc.flag, tc.value) + } + }) } } diff --git a/docs/plans/completed/20260408-mcp-upstream-commands.md b/docs/plans/completed/20260408-mcp-upstream-commands.md index a4e81e2..06ef91d 100644 --- a/docs/plans/completed/20260408-mcp-upstream-commands.md +++ b/docs/plans/completed/20260408-mcp-upstream-commands.md @@ -77,8 +77,8 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. - Test: `internal/telegram/commands_test.go` - [x] `/mcp remove ` - remove upstream by name -- [x] After add/remove: trigger MCP config re-injection into agent container. Added `mcpURL` field to `CommandHandler` with `SetMCPURL` setter and `MCPURL` in `ChannelConfig`. The `reinjectMCPConfig` helper calls `ContainerManager.WireMCPGateway(ctx, "sluice", mcpURL)` after both `/mcp add` and `/mcp remove` successes. Wired from `cmd/sluice/main.go` via `deriveMCPBaseURL(*mcpBaseURL, *healthAddr)`. (Plan referenced an older `mcpDir`/`mcp-servers.json` write path; current code uses `WireMCPGateway` WebSocket RPC instead.) -- [x] Write tests for remove subcommand and re-injection trigger +- [x] Re-injection via `WireMCPGateway` was implemented and then removed during iter-1 code review. Sluice multiplexes every upstream behind a single agent-side entry (`mcp.servers.sluice = {url: http://sluice:3000/mcp}`) which is wired once at sluice startup and never changes on mutation. Re-calling `WireMCPGateway("sluice", url)` after /mcp add would not surface the new upstream to the agent (the gateway reads the upstream set from SQLite at startup) but would trigger an agent gateway restart. The agreed UX is: store mutation succeeds, response instructs the operator to restart sluice, no container-side RPC is issued on /mcp add or /mcp remove. +- [x] Write tests for remove subcommand and regression guards that `WireMCPGateway` is NOT called on /mcp add or /mcp remove (see `TestHandleMCPAddDoesNotCallContainerManager` and `TestHandleMCPRemoveDoesNotCallContainerManager`). - [x] Run tests - must pass before next task ### Task 4: Add /mcp to Telegram command menu @@ -95,24 +95,24 @@ After this plan, Telegram will have `/mcp list`, `/mcp add`, and `/mcp remove`. ### Task 5: Verify acceptance criteria - [x] Verify `/mcp list` in Telegram shows upstreams (verified via unit tests: `TestHandleMCPListEmpty`, `TestHandleMCPListWithUpstreams`, `TestHandleMCPListEscapesHTML`, `TestHandleMessageMCPListNotDeleted`) -- [x] Verify `/mcp add` creates upstream and triggers auto-injection (verified via unit tests: `TestHandleMCPAddStdio`, `TestHandleMCPAddWithArgsAndEnv`, `TestHandleMCPAddHTTPTransport`, `TestHandleMCPAddWebSocketTransport`, `TestHandleMCPAddTriggersReinjection`, `TestHandleMessageMCPAddDeletesMessage`) -- [x] Verify `/mcp remove` removes upstream (verified via unit tests: `TestHandleMCPRemove`, `TestHandleMCPRemoveNotFound`, `TestHandleMCPRemoveStrayPositional`, `TestHandleMCPRemoveTriggersReinjection`) +- [x] Verify `/mcp add` creates upstream (verified via unit tests: `TestHandleMCPAddStdio`, `TestHandleMCPAddWithArgsAndEnv`, `TestHandleMCPAddHTTPTransport`, `TestHandleMCPAddWebSocketTransport`, `TestHandleMCPAddDoesNotCallContainerManager`, `TestHandleMessageMCPAddDeletesMessage`) +- [x] Verify `/mcp remove` removes upstream (verified via unit tests: `TestHandleMCPRemove`, `TestHandleMCPRemoveNotFound`, `TestHandleMCPRemoveStrayPositional`, `TestHandleMCPRemoveDoesNotCallContainerManager`) - [x] Run full test suite: `go test ./... -v -timeout 30s` -- all 2389 tests passing across 12 packages ### Task 6: [Final] Update documentation -- [x] Update CLAUDE.md CLI subcommands section if needed (no update needed: CLAUDE.md has no Telegram commands listing section, and the CLI subcommands section already documents `sluice mcp add/list/remove`) +- [x] Update CLAUDE.md: iter 2 added `--header "K=V"` to the documented `sluice mcp add` flag list (CLI Subcommands section) and expanded the MCP Gateway Setup section to cover all three management surfaces (CLI, REST, Telegram) plus the `mcp.servers.sluice={url}` wiring and the sluice-restart requirement for upstream mutations. - [x] Move this plan to `docs/plans/completed/` ## Post-Completion **Manual verification:** -- Deploy to knuth +- Deploy to a live sluice stack - Add an MCP upstream via Telegram: `/mcp add test-server --command "echo hello"` - Verify it appears in `/mcp list` -- Verify `mcp-servers.json` is written to shared volume -- Verify OpenClaw discovers the new MCP server -- Remove it: `/mcp remove test-server` +- Restart sluice (the gateway builds its upstream set at startup) +- Re-run an agent session and confirm the new tools are exposed under the `test-server__*` namespace +- Remove it: `/mcp remove test-server`, restart sluice, confirm the tools are gone **Future work:** - Update/edit support for MCP upstreams (CLI, API, Telegram). Users can remove+add for now. diff --git a/internal/flagutil/flagutil.go b/internal/flagutil/flagutil.go new file mode 100644 index 0000000..9bfeb4e --- /dev/null +++ b/internal/flagutil/flagutil.go @@ -0,0 +1,78 @@ +// Package flagutil provides shared helpers for Go's stdlib flag package. It +// is used by both the CLI (cmd/sluice) and the Telegram command handler +// (internal/telegram) so the two surfaces accept the same flag shapes and +// behave identically on edge cases like the "--" terminator and single-dash +// short flags. +package flagutil + +import ( + "flag" + "strings" +) + +// ReorderFlagsBeforePositional returns a copy of args with all flag +// arguments moved before any positional arguments, so that Go's stdlib +// flag parser (which stops at the first non-flag) still sees every flag. +// +// The FlagSet is consulted to determine which flags take a value and +// therefore consume the following arg. "--flag=value" form is left as +// a single token. "--" is treated as a terminator: everything after it +// is positional, preserving the stdlib convention. +// +// Example: ["github", "--command", "https://x", "--timeout", "60"] +// becomes ["--command", "https://x", "--timeout", "60", "github"] +// +// Flags defined as bool do not consume the following arg. Everything +// else (string, int, Func, Var) is assumed to. +func ReorderFlagsBeforePositional(args []string, fs *flag.FlagSet) []string { + var flagArgs, positional []string + i := 0 + for i < len(args) { + a := args[i] + if a == "--" { + // Terminator: everything after is positional, flag parsing + // should still see "--" to stop. + flagArgs = append(flagArgs, a) + positional = append(positional, args[i+1:]...) + break + } + if !strings.HasPrefix(a, "-") || a == "-" { + positional = append(positional, a) + i++ + continue + } + flagArgs = append(flagArgs, a) + // --flag=value form: value is in the same arg. + if strings.Contains(a, "=") { + i++ + continue + } + // Otherwise the next arg is the value for non-bool flags. + name := strings.TrimLeft(a, "-") + if isValueFlag(fs, name) && i+1 < len(args) { + flagArgs = append(flagArgs, args[i+1]) + i += 2 + continue + } + i++ + } + return append(flagArgs, positional...) +} + +// isValueFlag reports whether the named flag consumes the next argument +// as its value. Bool flags do not; everything else does. +func isValueFlag(fs *flag.FlagSet, name string) bool { + f := fs.Lookup(name) + if f == nil { + // Unknown flag. Assume it takes a value so we don't accidentally + // slurp something that might be a positional arg. fs.Parse will + // then surface the real error. + return true + } + // The stdlib flag package exposes bool flags via an IsBoolFlag method + // on the Value. Non-bool flags don't implement this. + if bf, ok := f.Value.(interface{ IsBoolFlag() bool }); ok && bf.IsBoolFlag() { + return false + } + return true +} diff --git a/internal/flagutil/flagutil_test.go b/internal/flagutil/flagutil_test.go new file mode 100644 index 0000000..cd951e0 --- /dev/null +++ b/internal/flagutil/flagutil_test.go @@ -0,0 +1,136 @@ +package flagutil + +import ( + "flag" + "reflect" + "testing" +) + +func newTestFlagSet() *flag.FlagSet { + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.String("command", "", "") + fs.String("transport", "stdio", "") + fs.Int("timeout", 120, "") + fs.Bool("verbose", false, "") + fs.Func("header", "", func(string) error { return nil }) + return fs +} + +func TestReorderFlagsBeforePositional_NameFirst(t *testing.T) { + fs := newTestFlagSet() + in := []string{"github", "--command", "https://x", "--transport", "http"} + want := []string{"--command", "https://x", "--transport", "http", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_NameLast(t *testing.T) { + fs := newTestFlagSet() + in := []string{"--command", "https://x", "github"} + want := []string{"--command", "https://x", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_NameInMiddle(t *testing.T) { + fs := newTestFlagSet() + in := []string{"--command", "https://x", "github", "--transport", "http"} + want := []string{"--command", "https://x", "--transport", "http", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_EqualsForm(t *testing.T) { + fs := newTestFlagSet() + in := []string{"github", "--command=https://x", "--timeout=60"} + want := []string{"--command=https://x", "--timeout=60", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_BoolFlag(t *testing.T) { + fs := newTestFlagSet() + in := []string{"github", "--verbose", "--command", "https://x"} + want := []string{"--verbose", "--command", "https://x", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_RepeatableFunc(t *testing.T) { + fs := newTestFlagSet() + in := []string{"github", "--header", "A=1", "--header", "B=2"} + want := []string{"--header", "A=1", "--header", "B=2", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_Terminator(t *testing.T) { + fs := newTestFlagSet() + in := []string{"--command", "X", "--", "--not-a-flag", "github"} + want := []string{"--command", "X", "--", "--not-a-flag", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_SingleDashFlag(t *testing.T) { + // Single-dash short flags (e.g. "-command") should be treated as flags, + // matching Go's stdlib flag parser which accepts both "-foo" and "--foo". + fs := newTestFlagSet() + in := []string{"github", "-command", "https://x"} + want := []string{"-command", "https://x", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_BareDash(t *testing.T) { + // A bare "-" (often stdin) is positional, not a flag. + fs := newTestFlagSet() + in := []string{"-", "--command", "X"} + want := []string{"--command", "X", "-"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReorderFlagsBeforePositional_Empty(t *testing.T) { + fs := newTestFlagSet() + got := ReorderFlagsBeforePositional(nil, fs) + if len(got) != 0 { + t.Errorf("expected empty, got %v", got) + } +} + +// TestReorderFlagsBeforePositional_UnknownFlag exercises the isValueFlag +// f == nil branch. When the reorderer encounters a flag that is not +// registered on the FlagSet (e.g. a typo like --cmmand or an arg that +// happens to look like a flag), it conservatively assumes the flag +// consumes the next arg. This keeps the original token adjacency so that +// fs.Parse can later produce a precise "flag provided but not defined" +// error, rather than mis-splitting the stream and surfacing a cryptic +// positional-arg error instead. +func TestReorderFlagsBeforePositional_UnknownFlag(t *testing.T) { + fs := newTestFlagSet() + in := []string{"github", "--unknown", "val", "--command", "https://x"} + want := []string{"--unknown", "val", "--command", "https://x", "github"} + got := ReorderFlagsBeforePositional(in, fs) + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} diff --git a/internal/mcp/gateway.go b/internal/mcp/gateway.go index fb70982..f1b4b52 100644 --- a/internal/mcp/gateway.go +++ b/internal/mcp/gateway.go @@ -64,7 +64,7 @@ func NewGateway(cfg GatewayConfig) (*Gateway, error) { credResolver: cfg.CredentialResolver, } if gw.timeoutSec == 0 { - gw.timeoutSec = 120 + gw.timeoutSec = DefaultTimeoutSec } if gw.policy == nil { // nil rules cannot fail compilation, so error is always nil here. diff --git a/internal/mcp/upstream.go b/internal/mcp/upstream.go index 142130c..108a43b 100644 --- a/internal/mcp/upstream.go +++ b/internal/mcp/upstream.go @@ -22,6 +22,11 @@ const ( TransportWS = "websocket" // WebSocket client ) +// DefaultTimeoutSec is the default per-call timeout (in seconds) applied when +// an upstream entry does not specify its own. Exported so CLI and Telegram +// handlers can stay in sync with the gateway default. +const DefaultTimeoutSec = 120 + // vaultPrefix marks env values that should be resolved from the vault. const vaultPrefix = "vault:" @@ -165,7 +170,7 @@ type Upstream struct { timeout time.Duration } -const defaultUpstreamTimeout = 120 * time.Second +const defaultUpstreamTimeout = DefaultTimeoutSec * time.Second var validUpstreamName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) diff --git a/internal/store/store.go b/internal/store/store.go index 35cff75..64aaedc 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -858,12 +858,17 @@ func (s *Store) AddMCPUpstream(name, command string, opts MCPUpstreamOpts) (int6 } timeoutSec := opts.TimeoutSec if timeoutSec == 0 { + // Keep in sync with internal/mcp.DefaultTimeoutSec. We cannot import + // internal/mcp here because internal/mcp already imports internal/store, + // which would introduce a dependency cycle. timeoutSec = 120 } transport := opts.Transport if transport == "" { transport = "stdio" } + // Keep the transport list in sync with internal/mcp.ValidTransport and the + // Telegram /mcp add handler in internal/telegram/commands.go. validTransports := map[string]bool{"stdio": true, "http": true, "websocket": true} if !validTransports[transport] { return 0, fmt.Errorf("invalid transport %q: must be stdio, http, or websocket", transport) diff --git a/internal/telegram/approval.go b/internal/telegram/approval.go index 8969291..65500f7 100644 --- a/internal/telegram/approval.go +++ b/internal/telegram/approval.go @@ -33,7 +33,6 @@ type ChannelConfig struct { Vault *vault.Store ContainerMgr container.ContainerManager Store *store.Store - MCPURL string // external URL for sluice's MCP gateway, used to re-wire the agent's config after /mcp add|remove OnEngineSwap func(eng *policy.Engine) // called after policy mutations to update dependent state OnOAuthIndexRebuild func() // called after credential removal to rebuild proxy OAuth index APIEndpoint string // custom Telegram API endpoint (for testing); empty uses default @@ -88,9 +87,6 @@ func NewTelegramChannel(cfg ChannelConfig) (*TelegramChannel, error) { if cfg.ContainerMgr != nil { cmdHandler.SetContainerManager(cfg.ContainerMgr) } - if cfg.MCPURL != "" { - cmdHandler.SetMCPURL(cfg.MCPURL) - } if cfg.ResolverPtr != nil { cmdHandler.SetResolverPtr(cfg.ResolverPtr) } @@ -434,6 +430,14 @@ func (tc *TelegramChannel) handleMessage(msg *tgbotapi.Message) { _, size := utf8.DecodeRuneInString(response[cut:]) cut += size } + // Back up to the last newline before the cut. Every line our + // formatters emit is self-contained (open tags closed on the + // same line), so a newline boundary guarantees we don't split a + // or tag and confuse Telegram's HTML parser. Fall + // back to rune-level truncation when no newline exists. + if nl := strings.LastIndexByte(response[:cut], '\n'); nl >= 0 { + cut = nl + } response = response[:cut] + "\n\n(truncated)" } diff --git a/internal/telegram/approval_test.go b/internal/telegram/approval_test.go index 94777ea..79ba499 100644 --- a/internal/telegram/approval_test.go +++ b/internal/telegram/approval_test.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "sync/atomic" @@ -53,7 +54,7 @@ type mockTelegramAPI struct { editedMsgs []tgbotapi.EditMessageTextConfig callbacks []tgbotapi.CallbackConfig deletedMsgs []tgbotapi.DeleteMessageConfig - setCommandsRaw []string // raw JSON payloads received by setMyCommands + setCommandsRaw string // raw JSON payload from the last setMyCommands call nextMsgID int updates chan []tgbotapi.Update @@ -146,7 +147,7 @@ func newMockTelegramAPI(t *testing.T) *mockTelegramAPI { _ = r.ParseForm() raw := r.FormValue("commands") m.mu.Lock() - m.setCommandsRaw = append(m.setCommandsRaw, raw) + m.setCommandsRaw = raw m.mu.Unlock() _ = json.NewEncoder(w).Encode(tgResponse{OK: true, Result: json.RawMessage(`true`)}) @@ -194,12 +195,10 @@ func (m *mockTelegramAPI) getDeletedMessages() []tgbotapi.DeleteMessageConfig { return out } -func (m *mockTelegramAPI) getSetCommandsRaw() []string { +func (m *mockTelegramAPI) getSetCommandsRaw() string { m.mu.Lock() defer m.mu.Unlock() - out := make([]string, len(m.setCommandsRaw)) - copy(out, m.setCommandsRaw) - return out + return m.setCommandsRaw } func escapeJSON(s string) string { @@ -564,63 +563,50 @@ func TestCancelApprovalShowsShutdownReason(t *testing.T) { // --- Start/Stop lifecycle tests --- // TestRegisterCommands verifies that Start registers the bot command menu -// with the expected entries, including /mcp for MCP upstream management. +// with the expected entries (order preserved), including /mcp for MCP +// upstream management. func TestRegisterCommands(t *testing.T) { mock := newMockTelegramAPI(t) s := newTestStore(t) tc := newTestTelegramChannel(t, mock, s) + // Start calls registerCommands synchronously before returning, so the mock + // payload is available immediately with no polling needed. if err := tc.Start(); err != nil { t.Fatalf("Start: %v", err) } t.Cleanup(tc.Stop) - // Wait for setMyCommands to be called (happens synchronously inside Start). - deadline := time.After(3 * time.Second) - var raw []string - for { - raw = mock.getSetCommandsRaw() - if len(raw) > 0 { - break - } - select { - case <-deadline: - t.Fatal("timed out waiting for setMyCommands call") - default: - time.Sleep(10 * time.Millisecond) - } + raw := mock.getSetCommandsRaw() + if raw == "" { + t.Fatal("setMyCommands was not called by Start()") } - // The payload is a JSON array of {command,description} objects. Parse it - // and verify every expected command is present. var cmds []tgbotapi.BotCommand - if err := json.Unmarshal([]byte(raw[0]), &cmds); err != nil { - t.Fatalf("unmarshal commands payload: %v (raw=%q)", err, raw[0]) - } - - want := map[string]string{ - "status": "Show proxy status", - "policy": "Manage policy rules", - "cred": "Manage credentials", - "mcp": "Manage MCP upstreams", - "audit": "Show audit log entries", - "start": "Show welcome message", - "help": "Show available commands", - } - - got := make(map[string]string, len(cmds)) - for _, c := range cmds { - got[c.Command] = c.Description - } - - for name, desc := range want { - gotDesc, ok := got[name] - if !ok { - t.Errorf("missing command /%s in setMyCommands payload", name) - continue + if err := json.Unmarshal([]byte(raw), &cmds); err != nil { + t.Fatalf("unmarshal commands payload: %v (raw=%q)", err, raw) + } + + // Order matches the Telegram menu grouping convention: status first, then + // mutation groups, then meta commands. + want := []tgbotapi.BotCommand{ + {Command: "status", Description: "Show proxy status"}, + {Command: "policy", Description: "Manage policy rules"}, + {Command: "cred", Description: "Manage credentials"}, + {Command: "mcp", Description: "Manage MCP upstreams"}, + {Command: "audit", Description: "Show audit log entries"}, + {Command: "start", Description: "Show welcome message"}, + {Command: "help", Description: "Show available commands"}, + } + if len(cmds) != len(want) { + t.Fatalf("got %d commands, want %d (unexpected extras?): %+v", len(cmds), len(want), cmds) + } + for i, w := range want { + if cmds[i].Command != w.Command { + t.Errorf("cmds[%d].Command = %q, want %q", i, cmds[i].Command, w.Command) } - if gotDesc != desc { - t.Errorf("/%s description = %q, want %q", name, gotDesc, desc) + if cmds[i].Description != w.Description { + t.Errorf("cmds[%d].Description = %q, want %q", i, cmds[i].Description, w.Description) } } } @@ -1300,6 +1286,12 @@ func TestHandleMessageMCPAddDeletesMessage(t *testing.T) { if len(mock.getDeletedMessages()) == 0 { t.Error("mcp add message should be deleted for security") } + // The plaintext --env value must not leak via the external command + // channel. handleMessage routes sensitive commands to the internal + // CommandHandler only. + if len(tc.cmdCh) != 0 { + t.Errorf("/mcp add must not forward to cmdCh (got %d entries, risks leaking --env secrets)", len(tc.cmdCh)) + } } func TestHandleMessageMCPListNotDeleted(t *testing.T) { @@ -1320,6 +1312,48 @@ func TestHandleMessageMCPListNotDeleted(t *testing.T) { } } +// TestHandleMessageMCPListTruncation seeds enough upstreams to push /mcp +// list past the 4000-rune telegram limit and verifies the truncated +// response keeps and balanced. An unbalanced tag would make +// Telegram reject the message under HTML parse mode. +func TestHandleMessageMCPListTruncation(t *testing.T) { + mock := newMockTelegramAPI(t) + s := newTestStore(t) + // Seed 200 upstreams so the output comfortably exceeds telegramMaxMessage. + for i := 0; i < 200; i++ { + name := "upstream_" + strconv.Itoa(i) + if _, err := s.AddMCPUpstream(name, "npx", store.MCPUpstreamOpts{ + Transport: "stdio", + Args: []string{"--arg", "padding-to-ensure-overflow"}, + }); err != nil { + t.Fatal(err) + } + } + tc := newTestTelegramChannel(t, mock, s) + + tc.handleMessage(&tgbotapi.Message{ + MessageID: 800, + Chat: &tgbotapi.Chat{ID: 12345}, + Text: "/mcp list", + }) + + time.Sleep(100 * time.Millisecond) + msgs := mock.getSentMessages() + if len(msgs) == 0 { + t.Fatal("expected sendMessage to be called") + } + text := msgs[len(msgs)-1].Text + if !strings.Contains(text, "(truncated)") { + t.Errorf("expected truncation marker, got first 200 chars: %q", text[:min(200, len(text))]) + } + if open, close := strings.Count(text, ""), strings.Count(text, ""); open != close { + t.Errorf("truncated output breaks balance: %d open, %d close", open, close) + } + if open, close := strings.Count(text, ""), strings.Count(text, ""); open != close { + t.Errorf("truncated output breaks balance: %d open, %d close", open, close) + } +} + func TestContainsSensitiveArgs(t *testing.T) { tests := []struct { name string @@ -1334,6 +1368,7 @@ func TestContainsSensitiveArgs(t *testing.T) { {"cred remove", &Command{Name: "cred", Args: []string{"remove", "name"}}, false}, {"mcp add", &Command{Name: "mcp", Args: []string{"add", "name", "--command", "cmd"}}, true}, {"mcp list", &Command{Name: "mcp", Args: []string{"list"}}, false}, + {"mcp remove", &Command{Name: "mcp", Args: []string{"remove", "name"}}, false}, {"policy show", &Command{Name: "policy", Args: []string{"show"}}, false}, } for _, tt := range tests { @@ -1450,7 +1485,7 @@ func TestCredAddWithContainerManager(t *testing.T) { if !strings.Contains(result, "Added credential") { t.Errorf("expected add confirmation, got: %s", result) } - if mgr.injectCalled { + if mgr.injectCalledSafe() { t.Error("InjectEnvVars should not be called when no env_var bindings exist") } } @@ -1478,10 +1513,10 @@ func TestCredAddWithContainerManagerAndEnvVar(t *testing.T) { t.Errorf("should indicate env vars updated, got: %s", result) } - if !mgr.injectCalled { + if !mgr.injectCalledSafe() { t.Error("InjectEnvVars should have been called") } - if _, ok := mgr.injectEnv["OPENAI_API_KEY"]; !ok { + if _, ok := mgr.injectEnvSafe()["OPENAI_API_KEY"]; !ok { t.Error("InjectEnvVars should include OPENAI_API_KEY") } } @@ -1510,10 +1545,10 @@ func TestCredRemoveWithContainerManager(t *testing.T) { t.Errorf("expected remove confirmation, got: %s", result) } - if !mgr.injectCalled { + if !mgr.injectCalledSafe() { t.Error("InjectEnvVars should have been called after remove") } - if v, ok := mgr.injectEnv["TEST_API_KEY"]; !ok || v != "" { + if v, ok := mgr.injectEnvSafe()["TEST_API_KEY"]; !ok || v != "" { t.Errorf("removed env var should be empty, got: %q (exists=%v)", v, ok) } } @@ -1540,7 +1575,7 @@ func TestCredRotateWithContainerManager(t *testing.T) { if !strings.Contains(result, "Rotated credential") { t.Errorf("expected rotate confirmation, got: %s", result) } - if !mgr.injectCalled { + if !mgr.injectCalledSafe() { t.Error("InjectEnvVars should have been called after rotate") } } @@ -1956,34 +1991,73 @@ func TestRebuildResolverEmptyBindings(t *testing.T) { // --- mockContainerMgr --- +// mockContainerMgr is a concurrency-safe stub ContainerManager used by the +// Telegram tests. All state fields are guarded by mu because command handlers +// may run on background goroutines (e.g. the telegram update loop) while the +// test asserts; the mutex prevents data races flagged by -race and gives tests +// deterministic reads. type mockContainerMgr struct { + mu sync.Mutex injectCalled bool injectEnv map[string]string injectErr error restartCalled bool restartErr error - wireCalled bool - wireName string - wireURL string - wireErr error + // wireCalled tracks calls to WireMCPGateway. The MCP upstream mutation + // path must NOT invoke it (sluice URL is wired once at startup and does + // not change on /mcp add or /mcp remove), so tests assert wireCalled + // remains false after those operations. + wireCalled bool } func (m *mockContainerMgr) InjectEnvVars(_ context.Context, envMap map[string]string, _ bool) error { + m.mu.Lock() + defer m.mu.Unlock() m.injectCalled = true m.injectEnv = envMap return m.injectErr } func (m *mockContainerMgr) RestartWithEnv(_ context.Context, _ map[string]string) error { + m.mu.Lock() + defer m.mu.Unlock() m.restartCalled = true return m.restartErr } -func (m *mockContainerMgr) WireMCPGateway(_ context.Context, name, url string) error { +func (m *mockContainerMgr) WireMCPGateway(_ context.Context, _, _ string) error { + m.mu.Lock() + defer m.mu.Unlock() m.wireCalled = true - m.wireName = name - m.wireURL = url - return m.wireErr + return nil +} + +// injectCalledSafe, injectEnvSafe, and wireCalledSafe are read accessors that +// lock mu so tests can observe state after handlers complete without tripping +// the race detector. +func (m *mockContainerMgr) injectCalledSafe() bool { + m.mu.Lock() + defer m.mu.Unlock() + return m.injectCalled +} + +func (m *mockContainerMgr) injectEnvSafe() map[string]string { + m.mu.Lock() + defer m.mu.Unlock() + if m.injectEnv == nil { + return nil + } + out := make(map[string]string, len(m.injectEnv)) + for k, v := range m.injectEnv { + out[k] = v + } + return out +} + +func (m *mockContainerMgr) wireCalledSafe() bool { + m.mu.Lock() + defer m.mu.Unlock() + return m.wireCalled } func (m *mockContainerMgr) Status(_ context.Context) (container.ContainerStatus, error) { diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index 01caece..b2d08c6 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -2,7 +2,9 @@ package telegram import ( "context" + "flag" "fmt" + "io" "log" "os" "regexp" @@ -15,6 +17,7 @@ import ( "github.com/nemirovsky/sluice/internal/channel" "github.com/nemirovsky/sluice/internal/container" + "github.com/nemirovsky/sluice/internal/flagutil" "github.com/nemirovsky/sluice/internal/mcp" "github.com/nemirovsky/sluice/internal/policy" "github.com/nemirovsky/sluice/internal/store" @@ -29,13 +32,19 @@ type Command struct { // ParseCommand parses a Telegram message into a Command. // Returns nil if the message is not a command (doesn't start with /). +// +// Tokenization respects double and single quotes so operators can pass +// values that contain spaces or commas via the documented forms such as +// /mcp add myserver --args "a,b" --env "K=V,K=V" --header "Authorization=Bearer tok". +// Without quote-aware splitting, the args "a,b" token would split into two +// fields and drop the quoting, corrupting the downstream flag parse. func ParseCommand(text string) *Command { text = strings.TrimSpace(text) if !strings.HasPrefix(text, "/") { return nil } - parts := strings.Fields(text) - if len(parts) == 0 { + parts, err := tokenizeShellish(text) + if err != nil || len(parts) == 0 { return nil } name := strings.TrimPrefix(parts[0], "/") @@ -49,6 +58,69 @@ func ParseCommand(text string) *Command { } } +// tokenizeShellish splits a command-line style string into tokens, respecting +// double and single quotes. Backslash escapes the next character inside or +// outside double quotes. Single quotes are literal (no escapes). This mirrors +// the subset of POSIX shell tokenization that Telegram users are likely to +// reach for (quoted CSV values, quoted bearer tokens with spaces) without +// pulling in a full shell parser dependency. +// +// Returns an error if a quote is left unterminated so ParseCommand can treat +// malformed input as not-a-command rather than silently dropping quotes. +func tokenizeShellish(text string) ([]string, error) { + var tokens []string + var cur strings.Builder + inToken := false + quote := byte(0) // 0 = unquoted, '"' or '\'' = inside that quote style + for i := 0; i < len(text); i++ { + c := text[i] + if quote == 0 { + switch { + case c == ' ' || c == '\t' || c == '\n' || c == '\r': + if inToken { + tokens = append(tokens, cur.String()) + cur.Reset() + inToken = false + } + case c == '"' || c == '\'': + quote = c + inToken = true + case c == '\\' && i+1 < len(text): + i++ + cur.WriteByte(text[i]) + inToken = true + default: + cur.WriteByte(c) + inToken = true + } + continue + } + // Inside a quoted run. + if c == quote { + quote = 0 + continue + } + if quote == '"' && c == '\\' && i+1 < len(text) { + next := text[i+1] + // Only a handful of escapes are meaningful inside double quotes + // (matching POSIX). For other chars the backslash is preserved. + if next == '"' || next == '\\' || next == '$' || next == '`' || next == '\n' { + i++ + cur.WriteByte(next) + continue + } + } + cur.WriteByte(c) + } + if quote != 0 { + return nil, fmt.Errorf("unterminated quote %c", quote) + } + if inToken { + tokens = append(tokens, cur.String()) + } + return tokens, nil +} + // CommandHandler holds the dependencies needed by command handlers. type CommandHandler struct { engine *atomic.Pointer[policy.Engine] @@ -59,7 +131,6 @@ type CommandHandler struct { vault *vault.Store containerMgr container.ContainerManager store *store.Store - mcpURL string // external URL for sluice's MCP gateway; empty disables re-injection onEngineSwap func(eng *policy.Engine) // called after engine swap to update dependent state onOAuthIndexRebuild func() // called after credential removal to rebuild proxy OAuth index } @@ -79,13 +150,6 @@ func (h *CommandHandler) SetStore(s *store.Store) { h.store = s } -// SetMCPURL sets the external URL used to re-wire sluice's MCP gateway into -// the agent container's config after /mcp add and /mcp remove. Empty disables -// re-injection (which is also a no-op when no container manager is configured). -func (h *CommandHandler) SetMCPURL(url string) { - h.mcpURL = url -} - // SetResolverPtr shares the proxy's binding resolver pointer so credential // mutations can update the live binding snapshot without requiring SIGHUP. func (h *CommandHandler) SetResolverPtr(ptr *atomic.Pointer[vault.BindingResolver]) { @@ -674,12 +738,16 @@ func (h *CommandHandler) credMutationComplete(msg string, removedEnvVars ...stri // handleMCP dispatches /mcp subcommands. func (h *CommandHandler) handleMCP(args []string) string { - if len(args) == 0 { - return "Usage: /mcp list | /mcp add --command [--transport stdio|http|websocket] [--args a,b] [--env K=V,K=V] [--timeout 120] | /mcp remove " - } + // Guard first so that when the store is not configured, operators get + // a clear diagnostic even if they called /mcp with no args. Emitting + // the usage banner in that state would advertise commands that will + // all subsequently fail. if h.store == nil { return "MCP management is not available (policy store not configured)." } + if len(args) == 0 { + return "Usage: /mcp list | " + mcpAddUsage + " | /mcp remove " + } switch args[0] { case "list": return h.mcpList() @@ -694,94 +762,166 @@ func (h *CommandHandler) handleMCP(args []string) string { // mcpAddUsage is the usage string returned when /mcp add is called with // missing required flags or no positional name. -const mcpAddUsage = "Usage: /mcp add --command [--transport stdio|http|websocket] [--args \"a,b\"] [--env \"K=V,K=V\"] [--timeout 120]" +// +// Limitations operators should be aware of: +// - --args and --env are comma-separated so individual values cannot contain +// commas. For env values that need commas, set them via config TOML or CLI. +// - --header is repeatable (pass the flag once per KEY=VAL pair) so commas +// in header values are not a problem. +// - --env values are rendered verbatim in /mcp list output. Use the vault: +// indirection (e.g. GITHUB_PAT=vault:github_pat) to keep secrets out of +// the SQLite store and list responses. +const mcpAddUsage = "Usage: /mcp add --command [--transport stdio|http|websocket] [--args \"a,b\"] [--env \"K=V,K=V\"] [--header \"K=V\" ...] [--timeout 120]" // mcpAdd registers a new MCP upstream from /mcp add arguments. +// +// Adding an upstream to the store is NOT enough for the running MCP gateway to +// pick it up: the gateway builds its upstream set at startup and does not +// hot-reload. After /mcp add the operator must restart sluice for the new +// upstream to take effect. The agent container's openclaw.json always points +// at sluice-as-a-whole (mcp.servers.sluice = {url: http://sluice:3000/mcp}), +// which is wired once at sluice startup. There is no per-upstream entry for +// the agent to re-read, so no gateway-level RPC is issued on /mcp add. func (h *CommandHandler) mcpAdd(args []string) string { - if len(args) == 0 { - return mcpAddUsage + opts, errMsg := parseMCPAddFlags(args) + if errMsg != "" { + return errMsg + } + + // reloadMu is not held here: MCP upstream changes do not recompile the + // policy engine or the binding resolver, and the running gateway is not + // hot-reloaded (the operator must restart sluice for the new upstream to + // take effect, see the function doc above). + id, err := h.store.AddMCPUpstream(opts.name, opts.command, store.MCPUpstreamOpts{ + Args: opts.cmdArgs, + Env: opts.env, + Headers: opts.headers, + TimeoutSec: opts.timeout, + Transport: opts.transport, + }) + if err != nil { + return fmt.Sprintf("Failed to add MCP upstream: %v", err) } - // Extract known flags. extractFlag returns empty string when the flag is - // absent; the remaining slice collapses the flag/value pair out of args. - command, args := extractFlag(args, "--command") - transport, args := extractFlag(args, "--transport") - argsStr, args := extractFlag(args, "--args") - envStr, args := extractFlag(args, "--env") - timeoutStr, args := extractFlag(args, "--timeout") + return fmt.Sprintf( + "Added MCP upstream [%d] %s (%s)\nRestart sluice for the new upstream to take effect.", + id, htmlCode(opts.name), htmlCode(opts.transport), + ) +} + +// mcpAddOpts holds the fully parsed and validated arguments for /mcp add. +type mcpAddOpts struct { + name string + command string + transport string + cmdArgs []string + env map[string]string + headers map[string]string + timeout int +} +// parseMCPAddFlags parses /mcp add arguments into mcpAddOpts. Returns either a +// populated mcpAddOpts with empty errMsg, or a zero mcpAddOpts and a +// user-facing error message for Telegram. +func parseMCPAddFlags(args []string) (mcpAddOpts, string) { if len(args) == 0 { - return mcpAddUsage + return mcpAddOpts{}, mcpAddUsage + } + + fs := flag.NewFlagSet("mcp add", flag.ContinueOnError) + // Suppress stdlib's default error-to-stderr: errors surface via Parse + // and we translate them into Telegram responses. + fs.SetOutput(io.Discard) + command := fs.String("command", "", "command or URL") + transport := fs.String("transport", mcp.TransportStdio, "transport type") + argsStr := fs.String("args", "", "comma-separated args") + envStr := fs.String("env", "", "comma-separated KEY=VAL env pairs") + headers := make(map[string]string) + fs.Func("header", "KEY=VAL HTTP header, repeatable (http transport only)", func(s string) error { + parts := strings.SplitN(s, "=", 2) + if len(parts) != 2 || parts[0] == "" { + return fmt.Errorf("Invalid --header %q: expected KEY=VAL", s) + } + headers[parts[0]] = parts[1] + return nil + }) + timeout := fs.Int("timeout", mcp.DefaultTimeoutSec, "per-call timeout in seconds") + if err := fs.Parse(flagutil.ReorderFlagsBeforePositional(args, fs)); err != nil { + return mcpAddOpts{}, fmt.Sprintf("Invalid argument: %s\n%s", err, mcpAddUsage) } - name := args[0] - // Reject stray positional args after so typos like - // "/mcp add foo bar --command cmd" don't silently swallow "bar". - if len(args) > 1 { - return fmt.Sprintf("Unexpected argument %q.\n%s", args[1], mcpAddUsage) + + positional := fs.Args() + if len(positional) == 0 { + return mcpAddOpts{}, mcpAddUsage } + if len(positional) > 1 { + return mcpAddOpts{}, fmt.Sprintf("Unexpected argument %q.\n%s", positional[1], mcpAddUsage) + } + name := positional[0] - if command == "" { - return mcpAddUsage + if *command == "" { + return mcpAddOpts{}, mcpAddUsage } if err := mcp.ValidateUpstreamName(name); err != nil { - return fmt.Sprintf("Invalid upstream name: %v", err) + return mcpAddOpts{}, fmt.Sprintf("Invalid upstream name: %v", err) } - if transport == "" { - transport = "stdio" - } - if !mcp.ValidTransport(transport) { - return fmt.Sprintf("Invalid transport %q: must be stdio, http, or websocket", transport) + if !mcp.ValidTransport(*transport) { + // Keep the transport list in sync with internal/store.Store.AddMCPUpstream + // and internal/mcp.ValidTransport. + return mcpAddOpts{}, fmt.Sprintf("Invalid transport %q: must be stdio, http, or websocket", *transport) } - timeout := 120 - if timeoutStr != "" { - n, err := strconv.Atoi(timeoutStr) - if err != nil || n <= 0 { - return fmt.Sprintf("Invalid --timeout %q: must be a positive integer (seconds)", timeoutStr) - } - timeout = n + if *timeout <= 0 { + return mcpAddOpts{}, fmt.Sprintf("Invalid --timeout %d: must be a positive integer (seconds)", *timeout) } var cmdArgs []string - if argsStr != "" { - cmdArgs = strings.Split(argsStr, ",") + if *argsStr != "" { + cmdArgs = strings.Split(*argsStr, ",") } - env := make(map[string]string) - if envStr != "" { - for _, kv := range strings.Split(envStr, ",") { - parts := strings.SplitN(kv, "=", 2) - if len(parts) != 2 { - return fmt.Sprintf("Invalid --env %q: expected KEY=VAL[,KEY=VAL,...]", envStr) - } - env[parts[0]] = parts[1] - } + env, errMsg := parseCSVKeyValues(*envStr, "--env") + if errMsg != "" { + return mcpAddOpts{}, errMsg } - h.reloadMu.Lock() - defer h.reloadMu.Unlock() - - id, err := h.store.AddMCPUpstream(name, command, store.MCPUpstreamOpts{ - Args: cmdArgs, - Env: env, - TimeoutSec: timeout, - Transport: transport, - }) - if err != nil { - return fmt.Sprintf("Failed to add MCP upstream: %v", err) + if len(headers) > 0 && *transport != mcp.TransportHTTP { + return mcpAddOpts{}, fmt.Sprintf("--header is only valid for --transport %s", mcp.TransportHTTP) } - msg := fmt.Sprintf( - "Added MCP upstream [%d] %s (%s)\nRestart sluice for the new upstream to take effect.", - id, htmlEscape(name), htmlEscape(transport), - ) - return msg + h.reinjectMCPConfig() + return mcpAddOpts{ + name: name, + command: *command, + transport: *transport, + cmdArgs: cmdArgs, + env: env, + headers: headers, + timeout: *timeout, + }, "" } -// mcpRemove removes an MCP upstream by name and triggers re-injection so the -// agent picks up the change. Returns a human-readable message for Telegram. +// parseCSVKeyValues parses "K=V[,K=V,...]" into a map. Returns a user-facing +// error message via the second return (empty on success). +func parseCSVKeyValues(csv, flagName string) (map[string]string, string) { + if csv == "" { + return nil, "" + } + out := make(map[string]string) + for _, kv := range strings.Split(csv, ",") { + parts := strings.SplitN(kv, "=", 2) + if len(parts) != 2 || parts[0] == "" { + return nil, fmt.Sprintf("Invalid %s %q: expected KEY=VAL[,KEY=VAL,...]", flagName, csv) + } + out[parts[0]] = parts[1] + } + return out, "" +} + +// mcpRemove removes an MCP upstream by name. Returns a human-readable message +// for Telegram. Like /mcp add, a sluice restart is required for the running +// gateway to pick up the removal (see mcpAdd for the rationale). func (h *CommandHandler) mcpRemove(args []string) string { if len(args) == 0 { return "Usage: /mcp remove " @@ -791,9 +931,8 @@ func (h *CommandHandler) mcpRemove(args []string) string { return fmt.Sprintf("Unexpected argument %q.\nUsage: /mcp remove ", args[1]) } - h.reloadMu.Lock() - defer h.reloadMu.Unlock() - + // reloadMu is not held here for the same reason as mcpAdd: MCP upstream + // changes do not recompile the policy engine or the binding resolver. deleted, err := h.store.RemoveMCPUpstream(name) if err != nil { return fmt.Sprintf("Failed to remove MCP upstream: %v", err) @@ -802,29 +941,10 @@ func (h *CommandHandler) mcpRemove(args []string) string { return fmt.Sprintf("No MCP upstream named %s", htmlCode(name)) } - msg := fmt.Sprintf( + return fmt.Sprintf( "Removed MCP upstream %s\nRestart sluice for the removal to take effect.", htmlCode(name), ) - return msg + h.reinjectMCPConfig() -} - -// reinjectMCPConfig re-wires sluice's MCP gateway URL into the agent -// container's config. Returns a trailing message suffix (prefixed with \n) -// describing the outcome, or "" if re-injection is not configured (e.g. no -// container manager, no MCP URL, or running without a runtime). Callers -// append the return value to their base response. -func (h *CommandHandler) reinjectMCPConfig() string { - if h.containerMgr == nil || h.mcpURL == "" { - return "" - } - - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) - defer cancel() - if err := h.containerMgr.WireMCPGateway(ctx, "sluice", h.mcpURL); err != nil { - return "\nWarning: failed to re-wire MCP config: " + err.Error() - } - return "\nAgent MCP config re-wired." } // mcpList renders all registered MCP upstreams for Telegram display. @@ -842,44 +962,93 @@ func (h *CommandHandler) mcpList() string { for _, u := range upstreams { transport := u.Transport if transport == "" { - transport = "stdio" + transport = mcp.TransportStdio } fmt.Fprintf(&b, "[%d] %s (%s)\n command: %s\n", - u.ID, htmlEscape(u.Name), htmlEscape(transport), htmlCode(u.Command)) + u.ID, htmlCode(u.Name), htmlCode(transport), htmlCode(u.Command)) if len(u.Args) > 0 { fmt.Fprintf(&b, " args: %s\n", htmlCode(strings.Join(u.Args, " "))) } - if len(u.Env) > 0 { - keys := make([]string, 0, len(u.Env)) - for k := range u.Env { - keys = append(keys, k) - } - sort.Strings(keys) - pairs := make([]string, 0, len(keys)) - for _, k := range keys { - pairs = append(pairs, k+"="+u.Env[k]) - } - fmt.Fprintf(&b, " env: %s\n", htmlCode(strings.Join(pairs, ", "))) + if line := sortedKVLineRedacted(u.Env); line != "" { + fmt.Fprintf(&b, " env: %s\n", htmlCode(line)) } - if len(u.Headers) > 0 { - keys := make([]string, 0, len(u.Headers)) - for k := range u.Headers { - keys = append(keys, k) - } - sort.Strings(keys) - pairs := make([]string, 0, len(keys)) - for _, k := range keys { - pairs = append(pairs, k+"="+u.Headers[k]) - } - fmt.Fprintf(&b, " headers: %s\n", htmlCode(strings.Join(pairs, ", "))) + if line := sortedKVLineRedacted(u.Headers); line != "" { + fmt.Fprintf(&b, " headers: %s\n", htmlCode(line)) } - if u.TimeoutSec != 0 && u.TimeoutSec != 120 { + if u.TimeoutSec != 0 && u.TimeoutSec != mcp.DefaultTimeoutSec { fmt.Fprintf(&b, " timeout: %ds\n", u.TimeoutSec) } } return b.String() } +// sortedKVLine renders a map as "k1=v1, k2=v2" with keys sorted so the output +// is deterministic. Returns "" when the map is empty so callers can skip a +// whole row instead of emitting an empty one. +func sortedKVLine(m map[string]string) string { + if len(m) == 0 { + return "" + } + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(keys)) + for _, k := range keys { + pairs = append(pairs, k+"="+m[k]) + } + return strings.Join(pairs, ", ") +} + +// sortedKVLineRedacted renders a map as "k1=v1, k2=v2" with keys sorted and +// values masked unless they are whole-value vault indirections. +// +// Values that are exactly "vault:" are safe to surface because they are +// pointers to credentials rather than the credentials themselves. Any other +// value is replaced with "****" so raw env values and header values added +// through Telegram, CLI, or the REST API cannot be retrieved by reading chat +// history. Operators who need to audit the raw stored values can query the +// SQLite store directly. +// +// Template forms like "Bearer {vault:github_pat}" are also masked, because +// the literal prefix ("Bearer ", etc.) can still carry human-chosen content +// that the operator did not intend for chat display. The goal is to default +// toward redaction and let vault indirection be the explicit opt-in for +// surfacing a value. +// +// Returns "" when the map is empty so callers can skip the row entirely. +func sortedKVLineRedacted(m map[string]string) string { + if len(m) == 0 { + return "" + } + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(keys)) + for _, k := range keys { + pairs = append(pairs, k+"="+maskUpstreamValue(m[k])) + } + return strings.Join(pairs, ", ") +} + +// maskUpstreamValue returns v when it is a whole-value vault indirection +// ("vault:" with a non-empty name) and "****" otherwise. The empty +// string passes through unchanged so operators can distinguish "KEY=" (set +// but empty) from "KEY=****" (set to a real, masked value). +func maskUpstreamValue(v string) string { + if v == "" { + return "" + } + const prefix = "vault:" + if strings.HasPrefix(v, prefix) && len(v) > len(prefix) && !strings.ContainsAny(v[len(prefix):], " \t") { + return v + } + return "****" +} + func (h *CommandHandler) handleStatus() string { snap := h.engine.Load().Snapshot() var b strings.Builder @@ -962,7 +1131,7 @@ Credentials help += ` MCP Upstreams -/mcp list | /mcp add --command [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V,K=V"] [--timeout 120] +/mcp list | /mcp add --command [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V,K=V"] [--header "K=V"] [--timeout 120] /mcp remove ` } diff --git a/internal/telegram/commands_test.go b/internal/telegram/commands_test.go index a95a8e8..dd6a4ab 100644 --- a/internal/telegram/commands_test.go +++ b/internal/telegram/commands_test.go @@ -1,7 +1,6 @@ package telegram import ( - "fmt" "os" "path/filepath" "strconv" @@ -59,6 +58,38 @@ func TestParseCommand(t *testing.T) { {"", true, "", nil}, {"/help", false, "help", nil}, {"/policy@mybot show", false, "policy", []string{"show"}}, + + // Quote-aware tokenization. Documented forms like --args "a,b" + // must preserve the comma-separated value as a single token so + // downstream flag parsing sees the intended value rather than + // two stray positional args. + { + `/mcp add github --command npx --args "a,b"`, + false, "mcp", + []string{"add", "github", "--command", "npx", "--args", "a,b"}, + }, + { + `/mcp add notion --command https://mcp.notion.com --env "FOO=1,BAR=2"`, + false, "mcp", + []string{"add", "notion", "--command", "https://mcp.notion.com", "--env", "FOO=1,BAR=2"}, + }, + { + `/mcp add api --header "Authorization=Bearer tok with space"`, + false, "mcp", + []string{"add", "api", "--header", "Authorization=Bearer tok with space"}, + }, + // Single quotes are literal - backslash inside does not escape. + { + `/policy allow 'host with space'`, + false, "policy", + []string{"allow", "host with space"}, + }, + // Unterminated quote is treated as not-a-command rather than + // silently dropping the quote. + { + `/mcp add name --args "unterminated`, + true, "", nil, + }, } for _, tt := range tests { @@ -851,6 +882,17 @@ func TestHandleMCPNoStore(t *testing.T) { if !strings.Contains(result, "not available") { t.Errorf("should report not available when store is not configured, got: %s", result) } + + // Also cover the no-args path. handleMCP's store guard runs before the + // usage banner so operators get a clear "not available" diagnostic + // rather than a usage string that advertises commands they cannot use. + resultNoArgs := handler.Handle(&Command{Name: "mcp", Args: nil}) + if !strings.Contains(resultNoArgs, "not available") { + t.Errorf("should report not available on bare /mcp when store is not configured, got: %s", resultNoArgs) + } + if strings.Contains(resultNoArgs, "Usage: /mcp") { + t.Errorf("store-guard must precede usage banner, got usage instead: %s", resultNoArgs) + } } func TestHandleMCPUnknownSubcommand(t *testing.T) { @@ -875,7 +917,8 @@ func TestHandleMCPListEmpty(t *testing.T) { func TestHandleMCPListWithUpstreams(t *testing.T) { s := newTestStore(t) - // Add a stdio upstream with args and env. + // Add a stdio upstream with args and env. The env value is a whole-value + // vault indirection which is safe to surface verbatim. if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{ Args: []string{"-y", "@modelcontextprotocol/server-github"}, Env: map[string]string{"GITHUB_PAT": "vault:github_pat"}, @@ -884,7 +927,10 @@ func TestHandleMCPListWithUpstreams(t *testing.T) { }); err != nil { t.Fatal(err) } - // Add an http upstream with headers and a non-default timeout. + // Add an http upstream with headers and a non-default timeout. The header + // value is a templated form ("Bearer vault:notion_token") which is NOT a + // whole-value vault pointer, so it must be masked to "****" in the + // rendered output. if _, err := s.AddMCPUpstream("notion", "https://mcp.notion.com", store.MCPUpstreamOpts{ Headers: map[string]string{"Authorization": "Bearer vault:notion_token"}, TimeoutSec: 60, @@ -896,7 +942,7 @@ func TestHandleMCPListWithUpstreams(t *testing.T) { handler := newTestHandlerWithStore(t, s, nil, "") out := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) - // Expect names, transports, and commands present. + // Expect names, transports, commands, and safe-to-show env/header forms. must := []string{ "github", "stdio", @@ -905,8 +951,8 @@ func TestHandleMCPListWithUpstreams(t *testing.T) { "http", "https://mcp.notion.com", "-y @modelcontextprotocol/server-github", - "GITHUB_PAT=vault:github_pat", - "Authorization=Bearer vault:notion_token", + "GITHUB_PAT=vault:github_pat", // whole-value vault pointer is safe + "Authorization=****", // templated value is masked "timeout: 60s", } for _, want := range must { @@ -914,16 +960,36 @@ func TestHandleMCPListWithUpstreams(t *testing.T) { t.Errorf("mcp list output missing %q\nfull output:\n%s", want, out) } } + // The raw templated header value must NOT leak into chat. + forbidden := []string{ + "Bearer vault:notion_token", + "notion_token", + } + for _, bad := range forbidden { + if strings.Contains(out, bad) { + t.Errorf("mcp list output must not contain %q\nfull output:\n%s", bad, out) + } + } // Default (120s) timeout should NOT be rendered. if strings.Contains(out, "timeout: 120s") { t.Errorf("default 120s timeout should be omitted, got: %s", out) } } -func TestHandleMCPListEscapesHTML(t *testing.T) { +// TestHandleMCPListRedactsSecrets asserts that raw plaintext env values and +// raw plaintext header values are masked out of /mcp list output. This locks +// in the security regression guard called out by the external review: the +// /mcp add auto-delete-on-send protection is only meaningful if the same +// values do not later reappear in chat via /mcp list. +func TestHandleMCPListRedactsSecrets(t *testing.T) { s := newTestStore(t) - if _, err := s.AddMCPUpstream("my", "echo ", store.MCPUpstreamOpts{ - Transport: "stdio", + // Raw plaintext env value and raw plaintext header value. Neither is a + // whole-value vault pointer, so both must be masked in the rendered + // output. + if _, err := s.AddMCPUpstream("leaky", "https://example.com", store.MCPUpstreamOpts{ + Transport: "http", + Env: map[string]string{"SUPER_SECRET": "ghp_rawtokenvalue"}, + Headers: map[string]string{"Authorization": "Bearer sk-liveapikey"}, }); err != nil { t.Fatal(err) } @@ -931,19 +997,75 @@ func TestHandleMCPListEscapesHTML(t *testing.T) { handler := newTestHandlerWithStore(t, s, nil, "") out := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) - // Raw "" and "" must be HTML-escaped so Telegram's HTML parse - // mode does not try to render them as tags. - if strings.Contains(out, "my") { - t.Errorf("raw must be HTML-escaped: %s", out) + // Masked renders must appear. + masked := []string{ + "SUPER_SECRET=****", + "Authorization=****", + } + for _, want := range masked { + if !strings.Contains(out, want) { + t.Errorf("mcp list should mask value, missing %q\nfull output:\n%s", want, out) + } + } + // Raw secrets must NOT appear anywhere in the output. + leaked := []string{ + "ghp_rawtokenvalue", + "sk-liveapikey", + "Bearer sk-liveapikey", + } + for _, bad := range leaked { + if strings.Contains(out, bad) { + t.Errorf("mcp list leaked plaintext %q\nfull output:\n%s", bad, out) + } } - if !strings.Contains(out, "my<srv>") { - t.Errorf("expected my<srv> in output: %s", out) +} + +func TestHandleMCPListEscapesHTML(t *testing.T) { + s := newTestStore(t) + // An http upstream exercises the args/env/header rendering paths + // which use htmlCode wrapping internally. Name, command, args, and + // the env/header KEYS contain "<" or "&" so we can confirm none of + // them leak past the HTML escape. Env and header VALUES are masked + // to "****" (see sortedKVLineRedacted and TestHandleMCPListRedactsSecrets), + // so we deliberately do not assert on their escaped form. The key + // side of the KEY=**** pair is still a meaningful escape target. + if _, err := s.AddMCPUpstream("my", "https://example.com/", store.MCPUpstreamOpts{ + Transport: "http", + Args: []string{"--mode=", "&flag"}, + Env: map[string]string{"X": "v&aue"}, + Headers: map[string]string{"X-H": "Bearer &n"}, + }); err != nil { + t.Fatal(err) } - if strings.Contains(out, "echo ") { - t.Errorf("raw must be HTML-escaped: %s", out) + + handler := newTestHandlerWithStore(t, s, nil, "") + out := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + + // Raw tags must not appear. The "<" checks catch any unescaped tag + // content directly. Value-side raw tags ("", "") are covered + // here too because the redaction mask should have already replaced + // them, so their absence is both an escape and a redaction check. + badRaw := []string{"", "", "", "", "", "", "", "&flag"} + for _, s := range badRaw { + if strings.Contains(out, s) { + t.Errorf("unescaped substring %q in output: %s", s, out) + } } - if !strings.Contains(out, "echo <hi>") { - t.Errorf("expected escaped echo <hi> in output: %s", out) + // Escaped equivalents on the key side (name, command, args, env/header + // keys) must appear. Value-side escapes are intentionally not asserted + // because the values are masked. + goodEsc := []string{ + "my<srv>", + "<svc>", + "<dev>", + "X<KEY>", + "X-H<dr>", + "&flag", + } + for _, s := range goodEsc { + if !strings.Contains(out, s) { + t.Errorf("expected escaped %q in output: %s", s, out) + } } } @@ -1095,6 +1217,151 @@ func TestHandleMCPAddWebSocketTransport(t *testing.T) { } } +// TestHandleMCPAddEmptyArgs covers the bare "/mcp add" branch with no flags +// or positional arguments. The handler should return the usage banner and not +// mutate the store. +func TestHandleMCPAddEmptyArgs(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{Name: "mcp", Args: []string{"add"}}) + if !strings.Contains(result, "Usage: /mcp add") { + t.Errorf("expected usage banner for bare /mcp add, got: %s", result) + } + if upstreams, _ := s.ListMCPUpstreams(); len(upstreams) != 0 { + t.Errorf("no upstream should be created for bare /mcp add, got %d", len(upstreams)) + } +} + +// TestHandleMCPAddEnvBase64Padding verifies that --env values containing "=" +// characters (e.g. base64-padded tokens) survive parsing intact. Regression +// to strings.Split on "=" would truncate "abc===padding" to "abc". +func TestHandleMCPAddEnvBase64Padding(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "github", "--command", "npx", + "--env", "TOKEN=abc===padding", + }, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(upstreams)) + } + if got, want := upstreams[0].Env["TOKEN"], "abc===padding"; got != want { + t.Errorf("env[TOKEN] = %q, want %q (strings.SplitN on '=' must keep the RHS intact)", got, want) + } +} + +// TestHandleMCPAddHTTPHeader verifies the --header flag is parsed for http +// upstreams so Telegram matches the CLI's --header support. --header is +// repeatable (matches CLI); pass the flag once per KEY=VAL pair. +func TestHandleMCPAddHTTPHeader(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "notion", + "--command", "https://mcp.notion.com", + "--transport", "http", + "--header", "Authorization=Bearer vault:notion", + "--header", "X-Custom=xyz", + }, + }) + if !strings.Contains(result, "Added MCP upstream") { + t.Fatalf("should confirm add, got: %s", result) + } + + upstreams, err := s.ListMCPUpstreams() + if err != nil { + t.Fatal(err) + } + if len(upstreams) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(upstreams)) + } + u := upstreams[0] + if u.Headers["Authorization"] != "Bearer vault:notion" { + t.Errorf("header[Authorization] = %q, want Bearer vault:notion", u.Headers["Authorization"]) + } + if u.Headers["X-Custom"] != "xyz" { + t.Errorf("header[X-Custom] = %q, want xyz", u.Headers["X-Custom"]) + } +} + +// TestHandleMCPAddHeaderRejectedForStdio verifies --header is rejected for +// non-http transports to match the CLI's behavior. +func TestHandleMCPAddHeaderRejectedForStdio(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "github", "--command", "npx", + "--header", "Authorization=Bearer xyz", + }, + }) + if !strings.Contains(result, "--header is only valid for --transport http") { + t.Errorf("expected --header validation error, got: %s", result) + } + if upstreams, _ := s.ListMCPUpstreams(); len(upstreams) != 0 { + t.Errorf("no upstream should be created when --header is misused, got %d", len(upstreams)) + } +} + +// TestHandleMCPAddDuplicateDoesNotCallContainerManager ensures error-path +// additions do not invoke WireMCPGateway. This guards against a regression +// that reintroduces pre-validation container side effects. +func TestHandleMCPAddDuplicateDoesNotCallContainerManager(t *testing.T) { + s := newTestStore(t) + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{}); err != nil { + t.Fatal(err) + } + handler := newTestHandlerWithStore(t, s, nil, "") + mgr := &mockContainerMgr{} + handler.SetContainerManager(mgr) + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "some-other"}, + }) + if !strings.Contains(result, "Failed to add MCP upstream") { + t.Errorf("expected duplicate rejection, got: %s", result) + } + if mgr.wireCalledSafe() { + t.Errorf("WireMCPGateway must not be called on the add error path") + } +} + +// TestHandleMCPRemoveNotFoundDoesNotCallContainerManager mirrors the add +// case for the remove error path. +func TestHandleMCPRemoveNotFoundDoesNotCallContainerManager(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + mgr := &mockContainerMgr{} + handler.SetContainerManager(mgr) + + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "nonexistent"}}) + if !strings.Contains(result, "No MCP upstream named") { + t.Errorf("expected not-found message, got: %s", result) + } + if mgr.wireCalledSafe() { + t.Errorf("WireMCPGateway must not be called on the remove error path") + } +} + func TestHandleMCPAddMissingCommand(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") @@ -1160,19 +1427,22 @@ func TestHandleMCPAddInvalidTimeout(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") - // non-numeric + // non-numeric surfaces stdlib flag.Parse error via our + // "Invalid argument:" wrapper. if r := handler.Handle(&Command{ Name: "mcp", Args: []string{"add", "github", "--command", "npx", "--timeout", "abc"}, - }); !strings.Contains(r, "Invalid --timeout") { + }); !strings.Contains(r, "Invalid argument") || !strings.Contains(r, "timeout") { t.Errorf("expected invalid timeout error, got: %s", r) } - // zero - if r := handler.Handle(&Command{ - Name: "mcp", - Args: []string{"add", "github", "--command", "npx", "--timeout", "0"}, - }); !strings.Contains(r, "Invalid --timeout") { - t.Errorf("expected invalid timeout error for 0, got: %s", r) + // zero and negative hit our own validation after flag parsing. + for _, tv := range []string{"0", "-5"} { + if r := handler.Handle(&Command{ + Name: "mcp", + Args: []string{"add", "github", "--command", "npx", "--timeout", tv}, + }); !strings.Contains(r, "Invalid --timeout") { + t.Errorf("expected invalid timeout error for %s, got: %s", tv, r) + } } } @@ -1190,6 +1460,33 @@ func TestHandleMCPAddInvalidEnv(t *testing.T) { } } +// TestHandleMCPAddInvalidHeader is the --header analog of +// TestHandleMCPAddInvalidEnv. The fs.Func callback for --header must +// reject a BADFORMAT token (no "=") with a user-visible error that +// surfaces through the "Invalid argument:" prefix produced by the +// stdlib flag.Parse wrapper. +func TestHandleMCPAddInvalidHeader(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + + result := handler.Handle(&Command{ + Name: "mcp", + Args: []string{ + "add", "github", + "--command", "https://api.example.com/mcp", + "--transport", "http", + "--header", "BADFORMAT", + }, + }) + if !strings.Contains(result, "Invalid argument") || !strings.Contains(result, "--header") { + t.Errorf("expected invalid header error mentioning --header, got: %s", result) + } + ups, _ := s.ListMCPUpstreams() + if len(ups) != 0 { + t.Errorf("no upstream should be created on parse failure, got %d", len(ups)) + } +} + func TestHandleMCPAddDuplicateName(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") @@ -1336,16 +1633,18 @@ func TestHandleMCPRemoveStrayPositional(t *testing.T) { } } -// TestHandleMCPAddTriggersReinjection verifies that /mcp add re-wires the -// agent's MCP config via WireMCPGateway when a container manager and MCP URL -// are configured. -func TestHandleMCPAddTriggersReinjection(t *testing.T) { +// TestHandleMCPAddDoesNotCallContainerManager verifies that /mcp add does NOT +// invoke the ContainerManager, because sluice multiplexes all upstreams via a +// single agent-side entry (mcp.servers.sluice) that is wired once at startup. +// Re-invoking WireMCPGateway on every mutation would trigger an agent gateway +// restart without changing anything meaningful. The operator-facing message +// instructs them to restart sluice so the gateway re-reads the upstream set. +func TestHandleMCPAddDoesNotCallContainerManager(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") mgr := &mockContainerMgr{} handler.SetContainerManager(mgr) - handler.SetMCPURL("http://sluice:3000/mcp") result := handler.Handle(&Command{ Name: "mcp", @@ -1354,125 +1653,140 @@ func TestHandleMCPAddTriggersReinjection(t *testing.T) { if !strings.Contains(result, "Added MCP upstream") { t.Fatalf("should confirm add, got: %s", result) } - if !mgr.wireCalled { - t.Errorf("WireMCPGateway should be called after /mcp add") - } - if mgr.wireName != "sluice" { - t.Errorf("wireName = %q, want %q", mgr.wireName, "sluice") - } - if mgr.wireURL != "http://sluice:3000/mcp" { - t.Errorf("wireURL = %q, want %q", mgr.wireURL, "http://sluice:3000/mcp") + if !strings.Contains(result, "Restart sluice") { + t.Errorf("response should instruct operator to restart sluice, got: %s", result) } - if !strings.Contains(result, "Agent MCP config re-wired") { - t.Errorf("expected re-wired notice in response, got: %s", result) + if mgr.wireCalledSafe() { + t.Errorf("WireMCPGateway must not be called on /mcp add (sluice URL is unchanged)") } } -// TestHandleMCPRemoveTriggersReinjection verifies that /mcp remove re-wires -// the agent's MCP config via WireMCPGateway when a container manager and MCP -// URL are configured. -func TestHandleMCPRemoveTriggersReinjection(t *testing.T) { +// TestHandleMCPRemoveDoesNotCallContainerManager mirrors the add case: the +// removal only takes effect after a sluice restart, and the agent's openclaw +// config is not touched. +func TestHandleMCPRemoveDoesNotCallContainerManager(t *testing.T) { s := newTestStore(t) - if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{ - Transport: "stdio", - }); err != nil { + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{Transport: "stdio"}); err != nil { t.Fatal(err) } - handler := newTestHandlerWithStore(t, s, nil, "") mgr := &mockContainerMgr{} handler.SetContainerManager(mgr) - handler.SetMCPURL("http://sluice:3000/mcp") result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "github"}}) if !strings.Contains(result, "Removed MCP upstream") { t.Fatalf("should confirm remove, got: %s", result) } - if !mgr.wireCalled { - t.Errorf("WireMCPGateway should be called after /mcp remove") - } - if mgr.wireName != "sluice" { - t.Errorf("wireName = %q, want %q", mgr.wireName, "sluice") - } - if mgr.wireURL != "http://sluice:3000/mcp" { - t.Errorf("wireURL = %q, want %q", mgr.wireURL, "http://sluice:3000/mcp") + if !strings.Contains(result, "Restart sluice") { + t.Errorf("response should instruct operator to restart sluice, got: %s", result) } - if !strings.Contains(result, "Agent MCP config re-wired") { - t.Errorf("expected re-wired notice in response, got: %s", result) + if mgr.wireCalledSafe() { + t.Errorf("WireMCPGateway must not be called on /mcp remove") } } -// TestHandleMCPReinjectionSkippedWithoutContainer verifies re-injection is a -// no-op when no container manager is configured (standalone mode). -func TestHandleMCPReinjectionSkippedWithoutContainer(t *testing.T) { +// TestHandleMCPAddEqualsFormFlag verifies that the single-token "--flag=value" +// form is accepted alongside the two-token form. Without this, operators +// typing `/mcp add github --command=npx` would hit the reorderer's two-token +// assumption. +func TestHandleMCPAddEqualsFormFlag(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") - handler.SetMCPURL("http://sluice:3000/mcp") - // Intentionally do not set a container manager. result := handler.Handle(&Command{ Name: "mcp", - Args: []string{"add", "github", "--command", "npx"}, + Args: []string{"add", "github", "--command=npx", "--transport=stdio", "--timeout=45"}, }) if !strings.Contains(result, "Added MCP upstream") { t.Fatalf("should confirm add, got: %s", result) } - if strings.Contains(result, "Agent MCP config re-wired") { - t.Errorf("re-wired notice should not appear without container manager, got: %s", result) - } - if strings.Contains(result, "Warning") { - t.Errorf("no warning should appear when re-injection is simply skipped, got: %s", result) + ups, _ := s.ListMCPUpstreams() + if len(ups) != 1 || ups[0].Command != "npx" || ups[0].Transport != "stdio" || ups[0].TimeoutSec != 45 { + t.Errorf("unexpected upstream state: %+v", ups) } } -// TestHandleMCPReinjectionSkippedWithoutURL verifies re-injection is a no-op -// when no MCP URL is configured. -func TestHandleMCPReinjectionSkippedWithoutURL(t *testing.T) { +// TestHandleMCPListErrorPath exercises the ListMCPUpstreams error branch by +// closing the store before the handler is invoked. +func TestHandleMCPListErrorPath(t *testing.T) { s := newTestStore(t) handler := newTestHandlerWithStore(t, s, nil, "") + if err := s.Close(); err != nil { + t.Fatal(err) + } + result := handler.Handle(&Command{Name: "mcp", Args: []string{"list"}}) + if !strings.Contains(result, "Failed to list MCP upstreams") { + t.Errorf("expected list error, got: %s", result) + } +} - mgr := &mockContainerMgr{} - handler.SetContainerManager(mgr) - // Intentionally do not set an MCP URL. - +// TestHandleMCPAddErrorPath exercises the AddMCPUpstream error branch by +// closing the store before the handler is invoked. The user-facing message +// must start with the generic "Failed to add" prefix, not a panic. +func TestHandleMCPAddErrorPath(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") + if err := s.Close(); err != nil { + t.Fatal(err) + } result := handler.Handle(&Command{ Name: "mcp", Args: []string{"add", "github", "--command", "npx"}, }) - if !strings.Contains(result, "Added MCP upstream") { - t.Fatalf("should confirm add, got: %s", result) - } - if mgr.wireCalled { - t.Errorf("WireMCPGateway should not be called without an MCP URL") + if !strings.Contains(result, "Failed to add MCP upstream") { + t.Errorf("expected add error, got: %s", result) } } -// TestHandleMCPReinjectionFailure verifies failures from WireMCPGateway are -// surfaced to the Telegram response as a warning but do not fail the overall -// add/remove operation. -func TestHandleMCPReinjectionFailure(t *testing.T) { +// TestHandleMCPRemoveErrorPath exercises the RemoveMCPUpstream error branch +// by closing the store before the handler is invoked. +func TestHandleMCPRemoveErrorPath(t *testing.T) { s := newTestStore(t) + // Seed first so the later close-and-remove hits the DB rather than the + // upfront "not found" check. + if _, err := s.AddMCPUpstream("github", "npx", store.MCPUpstreamOpts{}); err != nil { + t.Fatal(err) + } handler := newTestHandlerWithStore(t, s, nil, "") + if err := s.Close(); err != nil { + t.Fatal(err) + } + result := handler.Handle(&Command{Name: "mcp", Args: []string{"remove", "github"}}) + if !strings.Contains(result, "Failed to remove MCP upstream") { + t.Errorf("expected remove error, got: %s", result) + } +} - mgr := &mockContainerMgr{wireErr: fmt.Errorf("wire failed: exec timeout")} - handler.SetContainerManager(mgr) - handler.SetMCPURL("http://sluice:3000/mcp") +// TestHandleMCPAddRepeatableHeader verifies --header can be passed multiple +// times (matching the CLI). This is the preferred form: repeatable flags +// keep header values that contain commas intact, which a CSV form would +// silently split. +func TestHandleMCPAddRepeatableHeader(t *testing.T) { + s := newTestStore(t) + handler := newTestHandlerWithStore(t, s, nil, "") result := handler.Handle(&Command{ Name: "mcp", - Args: []string{"add", "github", "--command", "npx"}, + Args: []string{ + "add", "notion", + "--command", "https://mcp.notion.com", + "--transport", "http", + "--header", "Authorization=Bearer xyz", + "--header", "X-Custom=a,b,c", + }, }) if !strings.Contains(result, "Added MCP upstream") { - t.Fatalf("should confirm add even when re-injection fails, got: %s", result) + t.Fatalf("should confirm add, got: %s", result) } - if !strings.Contains(result, "Warning: failed to re-wire") { - t.Errorf("expected warning about wire failure, got: %s", result) + ups, _ := s.ListMCPUpstreams() + if len(ups) != 1 { + t.Fatalf("expected 1 upstream, got %d", len(ups)) } - - // The upstream should still be persisted. - upstreams, _ := s.ListMCPUpstreams() - if len(upstreams) != 1 { - t.Errorf("upstream should be persisted even if re-injection fails, got %d", len(upstreams)) + if ups[0].Headers["Authorization"] != "Bearer xyz" { + t.Errorf("unexpected Authorization header: %q", ups[0].Headers["Authorization"]) + } + if ups[0].Headers["X-Custom"] != "a,b,c" { + t.Errorf("repeatable --header must keep commas intact: %q", ups[0].Headers["X-Custom"]) } } From 3c6a8e43a5df97d06a84c426321991d8ecc28fd8 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Mon, 20 Apr 2026 18:49:34 +0800 Subject: [PATCH 8/8] fix(telegram): address golangci-lint issues - rename close shadows to closes in approval_test.go - lowercase error string in --header callback per ST1005 - remove unused sortedKVLine helper --- internal/telegram/approval_test.go | 8 ++++---- internal/telegram/commands.go | 21 +-------------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/internal/telegram/approval_test.go b/internal/telegram/approval_test.go index 79ba499..50f4eca 100644 --- a/internal/telegram/approval_test.go +++ b/internal/telegram/approval_test.go @@ -1346,11 +1346,11 @@ func TestHandleMessageMCPListTruncation(t *testing.T) { if !strings.Contains(text, "(truncated)") { t.Errorf("expected truncation marker, got first 200 chars: %q", text[:min(200, len(text))]) } - if open, close := strings.Count(text, ""), strings.Count(text, ""); open != close { - t.Errorf("truncated output breaks balance: %d open, %d close", open, close) + if opens, closes := strings.Count(text, ""), strings.Count(text, ""); opens != closes { + t.Errorf("truncated output breaks balance: %d open, %d close", opens, closes) } - if open, close := strings.Count(text, ""), strings.Count(text, ""); open != close { - t.Errorf("truncated output breaks balance: %d open, %d close", open, close) + if opens, closes := strings.Count(text, ""), strings.Count(text, ""); opens != closes { + t.Errorf("truncated output breaks balance: %d open, %d close", opens, closes) } } diff --git a/internal/telegram/commands.go b/internal/telegram/commands.go index b2d08c6..c2e64cd 100644 --- a/internal/telegram/commands.go +++ b/internal/telegram/commands.go @@ -840,7 +840,7 @@ func parseMCPAddFlags(args []string) (mcpAddOpts, string) { fs.Func("header", "KEY=VAL HTTP header, repeatable (http transport only)", func(s string) error { parts := strings.SplitN(s, "=", 2) if len(parts) != 2 || parts[0] == "" { - return fmt.Errorf("Invalid --header %q: expected KEY=VAL", s) + return fmt.Errorf("invalid --header %q: expected KEY=VAL", s) } headers[parts[0]] = parts[1] return nil @@ -982,25 +982,6 @@ func (h *CommandHandler) mcpList() string { return b.String() } -// sortedKVLine renders a map as "k1=v1, k2=v2" with keys sorted so the output -// is deterministic. Returns "" when the map is empty so callers can skip a -// whole row instead of emitting an empty one. -func sortedKVLine(m map[string]string) string { - if len(m) == 0 { - return "" - } - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - pairs := make([]string, 0, len(keys)) - for _, k := range keys { - pairs = append(pairs, k+"="+m[k]) - } - return strings.Join(pairs, ", ") -} - // sortedKVLineRedacted renders a map as "k1=v1, k2=v2" with keys sorted and // values masked unless they are whole-value vault indirections. //