feat(telegram): add /mcp list, add, remove commands

CLAUDE.md

@@ -88,7 +88,7 @@ sluice policy remove <id>
 sluice policy import <path.toml>    # seed DB from TOML (merge semantics)
 sluice policy export                # dump current rules as TOML
 
-sluice mcp add <name> --command <cmd> [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V"] [--timeout 120]
+sluice mcp add <name> --command <cmd> [--transport stdio|http|websocket] [--args "a,b"] [--env "K=V"] [--header "K=V"] [--timeout 120]
 sluice mcp list
 sluice mcp remove <name>
 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 `<upstream>__<tool>`. 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 `<upstream>__<tool>`. 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.
 

README.md

@@ -285,9 +285,14 @@ Manage sluice from your phone. Approve connections and tool calls, add credentia
 | `/policy deny <dest>` | Add deny rule |
 | `/cred add <name> [--env-var VAR]` | Add credential (value sent as next message, auto-deleted) |
 | `/cred rotate <name>` | Replace credential, hot-reload OpenClaw |
+| `/mcp list` | List registered MCP upstreams |
+| `/mcp add <name> --command <cmd> [flags]` | Register a new MCP upstream (stdio/http/websocket, see `/help`; chat message auto-deleted because `--env` may carry secrets) |
+| `/mcp remove <name>` | 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.

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)
 }

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:<name> for the whole value, or contain {vault:<name>} 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:<name> for the whole value, or contain {vault:<name>} 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)

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)
+			}
+		})
 	}
 }