fix(cli): allow positional name in any position for mcp add (#17)

Go's stdlib flag parser stops at the first non-flag argument, so
`sluice mcp add github --command X` would stop at `github` and never
see `--command`. This contradicted the usage message, which documented
`<name>` as the first positional.

Add reorderFlagsBeforePositional helper that moves flag args before
positional args, respecting both `--flag value` and `--flag=value`
forms, bool flags that don't consume the next arg, and the `--`
terminator. handleMCPAdd now calls it before fs.Parse so the upstream
name can appear at any position.

Author: nnemirovsky

cmd/sluice/flagutil.go

@@ -0,0 +1,73 @@
+package main
+
+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
+}

cmd/sluice/flagutil_test.go

@@ -0,0 +1,98 @@
+package main
+
+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) {
+	// Already in canonical order: should be unchanged.
+	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) {
+	// --verbose is a bool flag and does not consume the next arg.
+	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) {
+	// Everything after -- is positional, even if it looks like a flag.
+	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_Empty(t *testing.T) {
+	fs := newTestFlagSet()
+	got := reorderFlagsBeforePositional(nil, fs)
+	if len(got) != 0 {
+		t.Errorf("expected empty, got %v", got)
+	}
+}

cmd/sluice/mcp.go

@@ -264,7 +264,9 @@ func handleMCPAdd(args []string) error {
 		headers[parts[0]] = parts[1]
 		return nil
 	})
-	if err := fs.Parse(args); err != nil {
+	// Go's stdlib flag parser stops at the first non-flag argument. Reorder
+	// so positional args (upstream name) can come before flags too.
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/mcp_test.go

@@ -253,6 +253,53 @@ func TestHandleMCPAddAndList(t *testing.T) {
 	}
 }
 
+// TestHandleMCPAddNameBeforeFlags verifies that the upstream name can appear
+// before the flags, matching what the usage string documents. The stdlib
+// flag parser normally stops at the first non-flag arg; handleMCPAdd
+// reorders args via reorderFlagsBeforePositional to handle both orders.
+func TestHandleMCPAddNameBeforeFlags(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "test.db")
+
+	err := handleMCPAdd([]string{
+		"github", // name first
+		"--db", dbPath,
+		"--transport", "http",
+		"--command", "https://api.githubcopilot.com/mcp/",
+		"--header", "Authorization=Bearer token123",
+	})
+	if err != nil {
+		t.Fatalf("mcp add: %v", err)
+	}
+
+	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) != 1 {
+		t.Fatalf("expected 1 upstream, got %d", len(upstreams))
+	}
+	u := upstreams[0]
+	if u.Name != "github" {
+		t.Errorf("expected name github, got %q", u.Name)
+	}
+	if u.Transport != "http" {
+		t.Errorf("expected transport http, got %q", u.Transport)
+	}
+	if u.Command != "https://api.githubcopilot.com/mcp/" {
+		t.Errorf("expected command URL, got %q", u.Command)
+	}
+	if u.Headers["Authorization"] != "Bearer token123" {
+		t.Errorf("expected Authorization header, got %v", u.Headers)
+	}
+}
+
 // TestHandleMCPAddWithEnv verifies that environment variables are parsed correctly.
 func TestHandleMCPAddWithEnv(t *testing.T) {
 	dir := t.TempDir()