fix(cli): wrap fs.Parse with reorderFlagsBeforePositional in remaining commands (#25)

Seven CLI subcommands called fs.Parse(args) directly instead of going
through reorderFlagsBeforePositional, so any positional argument before
a flag stopped flag parsing and silently fell through to flag defaults.

For commands that take a --db flag this is a production hazard. Running

    sluice binding remove 2 --db /custom/path

stopped at "2", missed --db, and operated on the default
data/sluice.db instead of the requested path. The same shape applies to
cred remove, policy remove, policy import, mcp remove, channel update,
and channel remove.

Wrap each fs.Parse call with reorderFlagsBeforePositional like the other
sibling subcommands already do, and add a regression test per command
that exercises the positional-before-flags ordering.

Also fix the cred dispatcher's usage line to mention the "update"
subcommand that was added in v0.8.0.

Author: nnemirovsky

cmd/sluice/binding.go

@@ -277,7 +277,7 @@ func handleBindingUpdate(args []string) error {
 func handleBindingRemove(args []string) error {
 	fs := flag.NewFlagSet("binding remove", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/binding_test.go

@@ -582,6 +582,53 @@ func TestHandleBindingRemoveMissingArg(t *testing.T) {
 	}
 }
 
+// TestHandleBindingRemoveIDBeforeFlags is a regression for v0.8.0: the
+// positional id is allowed to appear before flags. The original
+// handleBindingRemove called fs.Parse(args) without
+// reorderFlagsBeforePositional, so an invocation like
+//
+//	sluice binding remove 1 --db /custom/path
+//
+// stopped flag parsing at "1" and silently fell through to the default
+// "data/sluice.db", deleting the wrong row in production. The fix is the
+// same reorderFlagsBeforePositional wrapper used by every other binding
+// subcommand. This test guards against the regression by passing the id
+// BEFORE --db and asserting the correct DB was modified.
+func TestHandleBindingRemoveIDBeforeFlags(t *testing.T) {
+	dbPath := setupBindingDB(t)
+
+	db, err := store.New(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.AddBinding("api.example.com", "mycred", store.BindingOpts{}); err != nil {
+		t.Fatal(err)
+	}
+	_ = db.Close()
+
+	_ = captureStdout(t, func() {
+		if err := handleBindingCommand([]string{
+			"remove", "1", "--db", dbPath,
+		}); err != nil {
+			t.Fatalf("remove with id-before-flags: %v", err)
+		}
+	})
+
+	db, err = store.New(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = db.Close() }()
+
+	bindings, err := db.ListBindings()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(bindings) != 0 {
+		t.Errorf("expected 0 bindings after remove with id-before-flags, got %d", len(bindings))
+	}
+}
+
 // TestHandleBindingAddWithEnvVar verifies that --env-var is stored on the
 // new binding when add is invoked.
 func TestHandleBindingAddWithEnvVar(t *testing.T) {

cmd/sluice/channel.go

@@ -121,7 +121,7 @@ func handleChannelUpdate(args []string) error {
 	enabled := fs.String("enabled", "", "set enabled state (true or false)")
 	url := fs.String("url", "", "update webhook URL")
 	secret := fs.String("secret", "", "update webhook HMAC secret")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 
@@ -174,7 +174,7 @@ func handleChannelUpdate(args []string) error {
 func handleChannelRemove(args []string) error {
 	fs := flag.NewFlagSet("channel remove", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/channel_test.go

@@ -413,6 +413,49 @@ func TestHandleChannelRemoveNonExistent(t *testing.T) {
 	}
 }
 
+// TestHandleChannelUpdateIDBeforeFlags is a regression for the v0.8.0
+// flag ordering bug: handleChannelUpdate called fs.Parse(args) directly,
+// so passing the channel id before --db caused the parser to stop and
+// silently fall through to the default "data/sluice.db", updating the
+// wrong channel. The fix wraps args with reorderFlagsBeforePositional.
+func TestHandleChannelUpdateIDBeforeFlags(t *testing.T) {
+	dbPath := setupChannelDB(t)
+
+	_ = captureChannelOutput(t, func() {
+		if err := handleChannelAdd([]string{
+			"--db", dbPath, "--type", "http", "--url", "https://example.com/hook",
+		}); err != nil {
+			t.Fatalf("add: %v", err)
+		}
+	})
+
+	_ = captureChannelOutput(t, func() {
+		if err := handleChannelUpdate([]string{"2", "--db", dbPath, "--enabled", "false"}); err != nil {
+			t.Fatalf("channel update with id-before-flags: %v", err)
+		}
+	})
+}
+
+// TestHandleChannelRemoveIDBeforeFlags is a regression for the same v0.8.0
+// flag ordering bug, applied to channel remove.
+func TestHandleChannelRemoveIDBeforeFlags(t *testing.T) {
+	dbPath := setupChannelDB(t)
+
+	_ = captureChannelOutput(t, func() {
+		if err := handleChannelAdd([]string{
+			"--db", dbPath, "--type", "http", "--url", "https://example.com/hook",
+		}); err != nil {
+			t.Fatalf("add: %v", err)
+		}
+	})
+
+	_ = captureChannelOutput(t, func() {
+		if err := handleChannelRemove([]string{"2", "--db", dbPath}); err != nil {
+			t.Fatalf("channel remove with id-before-flags: %v", err)
+		}
+	})
+}
+
 func TestChannelTypeName(t *testing.T) {
 	if got := channelTypeName(int(channel.ChannelTelegram)); got != "telegram" {
 		t.Errorf("telegram: got %q", got)

cmd/sluice/cred.go

@@ -19,7 +19,7 @@ import (
 
 func handleCredCommand(args []string) error {
 	if len(args) == 0 {
-		return fmt.Errorf("usage: sluice cred [add|list|remove]")
+		return fmt.Errorf("usage: sluice cred [add|list|update|remove]")
 	}
 
 	switch args[0] {
@@ -544,7 +544,7 @@ func handleCredList(args []string) error {
 func handleCredRemove(args []string) error {
 	fs := flag.NewFlagSet("cred remove", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/cred_test.go

@@ -275,6 +275,43 @@ func TestHandleCredRemove(t *testing.T) {
 	}
 }
 
+// TestHandleCredRemoveNameBeforeFlags is a regression for the v0.8.0 flag
+// ordering bug: handleCredRemove called fs.Parse(args) directly, so an
+// invocation like
+//
+//	sluice cred remove mycred --db /custom/path
+//
+// stopped flag parsing at "mycred" and silently fell through to the default
+// "data/sluice.db", removing the wrong credential. The fix wraps args with
+// reorderFlagsBeforePositional like every other CLI subcommand. This test
+// guards against the regression by passing the name BEFORE --db.
+func TestHandleCredRemoveNameBeforeFlags(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := setupVaultDB(t, dir)
+
+	vs, err := vault.NewStore(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := vs.Add("to_remove", "secret"); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := handleCredCommand([]string{"remove", "to_remove", "--db", dbPath}); err != nil {
+		t.Fatalf("cred remove with name-before-flags: %v", err)
+	}
+
+	names, err := vs.List()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, n := range names {
+		if n == "to_remove" {
+			t.Error("credential should have been removed via name-before-flags ordering")
+		}
+	}
+}
+
 // TestHandleCredListEmpty tests listing when no credentials exist.
 func TestHandleCredListEmpty(t *testing.T) {
 	dir := t.TempDir()

cmd/sluice/mcp.go

@@ -392,7 +392,7 @@ func handleMCPList(args []string) error {
 func handleMCPRemove(args []string) error {
 	fs := flag.NewFlagSet("mcp remove", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/mcp_test.go

@@ -414,6 +414,42 @@ func TestHandleMCPRemove(t *testing.T) {
 	}
 }
 
+// TestHandleMCPRemoveNameBeforeFlags is a regression for the v0.8.0 flag
+// ordering bug: handleMCPRemove called fs.Parse(args) directly, so the
+// positional name appearing before --db caused the parser to stop and
+// silently fall through to the default "data/sluice.db", removing the
+// wrong upstream.
+func TestHandleMCPRemoveNameBeforeFlags(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "test.db")
+
+	if err := handleMCPAdd([]string{
+		"--db", dbPath,
+		"--command", "server",
+		"myserver",
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := handleMCPRemove([]string{"myserver", "--db", dbPath}); err != nil {
+		t.Fatalf("mcp remove with name-before-flags: %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) != 0 {
+		t.Errorf("expected 0 upstreams after name-before-flags removal, got %d", len(upstreams))
+	}
+}
+
 // TestHandleMCPList verifies the list subcommand output.
 func TestHandleMCPList(t *testing.T) {
 	dir := t.TempDir()

cmd/sluice/policy.go

@@ -136,7 +136,7 @@ func handlePolicyAdd(args []string) error {
 func handlePolicyRemove(args []string) error {
 	fs := flag.NewFlagSet("policy remove", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 
@@ -169,7 +169,7 @@ func handlePolicyRemove(args []string) error {
 func handlePolicyImport(args []string) error {
 	fs := flag.NewFlagSet("policy import", flag.ContinueOnError)
 	dbPath := fs.String("db", "data/sluice.db", "path to SQLite database")
-	if err := fs.Parse(args); err != nil {
+	if err := fs.Parse(reorderFlagsBeforePositional(args, fs)); err != nil {
 		return err
 	}
 

cmd/sluice/policy_test.go

@@ -374,6 +374,41 @@ func TestHandlePolicyRemoveNoArgs(t *testing.T) {
 	}
 }
 
+// TestHandlePolicyRemoveIDBeforeFlags is a regression for the v0.8.0 flag
+// ordering bug: handlePolicyRemove called fs.Parse(args) directly, so
+// passing the id before --db caused the parser to stop and silently fall
+// through to the default "data/sluice.db", removing the wrong rule. The fix
+// wraps args with reorderFlagsBeforePositional. This test guards against
+// the regression.
+func TestHandlePolicyRemoveIDBeforeFlags(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "test.db")
+
+	db, err := store.New(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.AddRule("allow", store.RuleOpts{Destination: "example.com"}); err != nil {
+		t.Fatal(err)
+	}
+	_ = db.Close()
+
+	if err := handlePolicyRemove([]string{"1", "--db", dbPath}); err != nil {
+		t.Fatalf("policy remove with id-before-flags: %v", err)
+	}
+
+	db, err = store.New(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = db.Close() }()
+
+	rules, _ := db.ListRules(store.RuleFilter{})
+	if len(rules) != 0 {
+		t.Errorf("expected 0 rules after id-before-flags removal, got %d", len(rules))
+	}
+}
+
 // --- handlePolicyImport tests ---
 
 func TestHandlePolicyImportValid(t *testing.T) {
@@ -454,6 +489,40 @@ func TestHandlePolicyImportNoArgs(t *testing.T) {
 	}
 }
 
+// TestHandlePolicyImportPathBeforeFlags is a regression for the v0.8.0 flag
+// ordering bug: handlePolicyImport called fs.Parse(args) directly, so
+// passing the TOML path before --db caused the import to silently fall
+// through to the default "data/sluice.db". The fix wraps args with
+// reorderFlagsBeforePositional.
+func TestHandlePolicyImportPathBeforeFlags(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "test.db")
+	tomlPath := filepath.Join(dir, "config.toml")
+
+	tomlData := `[[allow]]
+destination = "api.example.com"
+ports = [443]
+`
+	if err := os.WriteFile(tomlPath, []byte(tomlData), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := handlePolicyImport([]string{tomlPath, "--db", dbPath}); err != nil {
+		t.Fatalf("policy import with path-before-flags: %v", err)
+	}
+
+	db, err := store.New(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = db.Close() }()
+
+	rules, _ := db.ListRules(store.RuleFilter{})
+	if len(rules) != 1 {
+		t.Errorf("expected 1 rule imported into custom db, got %d", len(rules))
+	}
+}
+
 // --- handlePolicyExport tests ---
 
 func TestHandlePolicyExportMatchesStore(t *testing.T) {