fix: Accept --session on plugin commands

README.md

@@ -73,7 +73,7 @@ datumctl get dnszones --session alice@example.com@api.staging.env.datum.net
 DATUM_SESSION=alice@example.com datumctl get projects
 ```
 
-datumctl also sets `DATUM_SESSION` for plugins it runs, so a plugin's own `datumctl` calls act as the same session. A stale `DATUM_SESSION` — left over from a previous logout, or inherited this way from a plugin — only affects commands that need a session; `datumctl version`, `datumctl plugin list`, and similar commands are unaffected.
+This works on plugin commands too (`datumctl dns zones list --session alice@example.com@api.datum.net`): datumctl takes the flag itself and sets `DATUM_SESSION` for the plugin it runs, so the plugin's own `datumctl` calls act as the same session. A stale `DATUM_SESSION` — left over from a previous logout, or inherited this way from a plugin — only affects commands that need a session; `datumctl version`, `datumctl plugin list`, and similar commands are unaffected.
 
 For machine-to-machine auth, see `datumctl login --credentials` for the machine-account flow.
 

internal/plugindispatch/forward.go

@@ -15,6 +15,7 @@ import (
 
 	"go.datum.net/datumctl/internal/client"
 	"go.datum.net/datumctl/internal/datumconfig"
+	customerrors "go.datum.net/datumctl/internal/errors"
 	"go.datum.net/datumctl/internal/pluginstore"
 )
 
@@ -35,12 +36,12 @@ import (
 // can correctly distinguish plugin names from registered subcommands.
 // On success this function does not return (process is replaced).
 //
-// A leading global --session flag ("datumctl --session X <plugin> ...") or the
-// DATUM_SESSION environment variable runs the plugin as that session. When the
-// session cannot be resolved, ForwardPlugin returns the UserError so the caller
-// can report it; the plugin is not run.
+// The global --session flag, wherever the user put it on the command line, or
+// the DATUM_SESSION environment variable runs the plugin as that session. When
+// the session cannot be resolved, ForwardPlugin returns the UserError so the
+// caller can report it; the plugin is not run.
 func ForwardPlugin(pluginsDir string, root *cobra.Command, factory *client.DatumCloudFactory) error {
-	sessionValue, args, hasSessionFlag := splitLeadingSessionFlag(os.Args[1:])
+	sessionValue, args, hasSessionFlag := splitSessionFlag(os.Args[1:])
 	if len(args) == 0 || strings.HasPrefix(args[0], "-") {
 		return nil
 	}
@@ -96,22 +97,35 @@ func ForwardPlugin(pluginsDir string, root *cobra.Command, factory *client.Datum
 	return Exec(binaryPath, args[1:], factory)
 }
 
-// splitLeadingSessionFlag removes global --session flags that appear before
-// the plugin name ("--session X" or "--session=X") and returns the last value
-// given, the remaining arguments, and whether the flag was present. Arguments
-// after the plugin name belong to the plugin and are left alone.
-func splitLeadingSessionFlag(args []string) (value string, rest []string, found bool) {
-	for len(args) > 0 {
+// splitSessionFlag removes datumctl's global --session flag ("--session X" or
+// "--session=X") from a plugin command line and returns the last value given,
+// the remaining arguments, and whether the flag was present.
+//
+// The flag is taken from anywhere in the command line, not just before the
+// plugin name: --session belongs to datumctl, and plugins do not define it, so
+// leaving one in place makes the plugin refuse the whole command with its own
+// "unknown flag: --session".
+//
+// Everything after a bare "--" is the plugin's to interpret and is left alone.
+func splitSessionFlag(args []string) (value string, rest []string, found bool) {
+	rest = make([]string, 0, len(args))
+	for i := 0; i < len(args); i++ {
 		switch {
-		case args[0] == "--session" && len(args) > 1:
-			value, found, args = args[1], true, args[2:]
-		case strings.HasPrefix(args[0], "--session="):
-			value, found, args = strings.TrimPrefix(args[0], "--session="), true, args[1:]
+		case args[i] == "--":
+			return value, append(rest, args[i:]...), found
+		case args[i] == "--session":
+			found = true
+			if i+1 < len(args) {
+				value = args[i+1]
+				i++
+			}
+		case strings.HasPrefix(args[i], "--session="):
+			value, found = strings.TrimPrefix(args[i], "--session="), true
 		default:
-			return value, args, found
+			rest = append(rest, args[i])
 		}
 	}
-	return value, args, found
+	return value, rest, found
 }
 
 // applyPluginSessionOverride installs the session a plugin runs as: the
@@ -126,6 +140,15 @@ func splitLeadingSessionFlag(args []string) (value string, rest []string, found
 // the unrelated real active session.
 func applyPluginSessionOverride(flagValue string, hasFlag bool) error {
 	if hasFlag {
+		// --session with nothing after it would otherwise clear the override and
+		// quietly run as the active session, which is the mistake the flag exists
+		// to prevent.
+		if strings.TrimSpace(flagValue) == "" {
+			return customerrors.NewUserErrorWithHint(
+				"--session needs a session name.",
+				"Run 'datumctl auth list' to see the available session names.",
+			)
+		}
 		_, err := datumconfig.ApplySessionOverride(flagValue, datumconfig.SessionOverrideFromFlag)
 		return err
 	}
@@ -220,7 +243,13 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err
 		return nil
 	}
 
-	name := os.Args[2]
+	// Take --session out of the completion request the same way the run path
+	// does, so it never reaches the plugin as an argument it does not know.
+	sessionValue, completionArgs, hasSessionFlag := splitSessionFlag(os.Args[2:])
+	if len(completionArgs) == 0 {
+		return nil
+	}
+	name := completionArgs[0]
 
 	// Find the plugin binary.
 	binaryPath, managed, err := FindPlugin(name, pluginsDir)
@@ -253,7 +282,7 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err
 	// Strip the plugin name from the forwarded args so the plugin sees
 	// ["__complete", <subargs...>] rather than ["__complete", "compute", <subargs...>].
 	// The plugin's own cobra tree has no knowledge of its name as a subcommand.
-	pluginArgs := append([]string{"__complete"}, os.Args[3:]...)
+	pluginArgs := append([]string{"__complete"}, completionArgs[1:]...)
 
 	cmd := exec.Command(binaryPath, pluginArgs...)
 	cmd.Stdin = os.Stdin
@@ -264,9 +293,10 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err
 	// completion. Non-fatal: if env construction fails we proceed without injection
 	// and the plugin will return empty candidates instead of erroring.
 	if factory != nil {
-		// Honor DATUM_SESSION so completion lists what the plugin will act on.
-		// An unresolvable value is ignored here; running the plugin reports it.
-		_ = applyPluginSessionOverride("", false)
+		// Honor --session/DATUM_SESSION so completion lists what the plugin will
+		// act on. An unresolvable value is ignored here; running the plugin
+		// reports it.
+		_ = applyPluginSessionOverride(sessionValue, hasSessionFlag)
 		if env, buildErr := BuildEnv(factory); buildErr == nil {
 			cmd.Env = overlayEnv(os.Environ(), env)
 		}
@@ -294,9 +324,9 @@ func ForwardCompletion(pluginsDir string, factory *client.DatumCloudFactory) err
 // Cobra intercepts --help before RunE fires, so this must be called before
 // cobra.Execute(), mirroring the ForwardCompletion pattern.
 func ForwardHelp(pluginsDir string) error {
-	// Strip "datumctl" and any leading --session, so help for
-	// "datumctl --session X <plugin> --help" reaches the plugin too.
-	_, args, _ := splitLeadingSessionFlag(os.Args[1:])
+	// Strip "datumctl" and any --session, so help for
+	// "datumctl <plugin> <cmd> --session X --help" reaches the plugin too.
+	_, args, _ := splitSessionFlag(os.Args[1:])
 	if len(args) == 0 {
 		return nil
 	}

internal/plugindispatch/session_override_test.go

@@ -2,6 +2,7 @@ package plugindispatch
 
 import (
 	"context"
+	"os"
 	"slices"
 	"strings"
 	"testing"
@@ -10,6 +11,7 @@ import (
 	"go.datum.net/datumctl/internal/datumconfig"
 	customerrors "go.datum.net/datumctl/internal/errors"
 	"go.datum.net/datumctl/internal/keyring"
+	"go.datum.net/datumctl/internal/pluginstore"
 )
 
 const (
@@ -169,7 +171,7 @@ func TestApplyPluginSessionOverride_StaleEnvDoesNotError(t *testing.T) {
 	}
 }
 
-func TestSplitLeadingSessionFlag(t *testing.T) {
+func TestSplitSessionFlag(t *testing.T) {
 	tests := []struct {
 		in        []string
 		wantValue string
@@ -179,15 +181,88 @@ func TestSplitLeadingSessionFlag(t *testing.T) {
 		{[]string{"ipam", "list"}, "", []string{"ipam", "list"}, false},
 		{[]string{"--session", "a@b@c", "ipam", "list", "-o", "wide"}, "a@b@c", []string{"ipam", "list", "-o", "wide"}, true},
 		{[]string{"--session=a@b", "ipam"}, "a@b", []string{"ipam"}, true},
-		// A --session after the plugin name belongs to the plugin.
-		{[]string{"ipam", "--session", "x"}, "", []string{"ipam", "--session", "x"}, false},
+		// Issue #304: --session after the plugin name is still datumctl's, and
+		// must not reach the plugin as an argument it does not recognize.
+		{[]string{"ipam", "--session", "x"}, "x", []string{"ipam"}, true},
+		{[]string{"assistant", "card", "--session", "a@b@c"}, "a@b@c", []string{"assistant", "card"}, true},
+		{[]string{"assistant", "card", "--session=a@b@c"}, "a@b@c", []string{"assistant", "card"}, true},
+		{[]string{"ipam", "list", "--session", "a@b", "-o", "wide"}, "a@b", []string{"ipam", "list", "-o", "wide"}, true},
+		// Missing value: reported as present with no value so the caller can say
+		// so, rather than silently running as the active session.
+		{[]string{"ipam", "list", "--session"}, "", []string{"ipam", "list"}, true},
+		// Everything after a bare "--" is the plugin's to interpret.
+		{[]string{"ipam", "--", "--session", "x"}, "", []string{"ipam", "--", "--session", "x"}, false},
 		{[]string{"--project", "p", "ipam"}, "", []string{"--project", "p", "ipam"}, false},
 	}
 	for _, tt := range tests {
-		v, rest, found := splitLeadingSessionFlag(tt.in)
+		v, rest, found := splitSessionFlag(tt.in)
 		if v != tt.wantValue || found != tt.wantFound || !slices.Equal(rest, tt.wantRest) {
-			t.Errorf("splitLeadingSessionFlag(%q) = %q, %q, %v; want %q, %q, %v",
+			t.Errorf("splitSessionFlag(%q) = %q, %q, %v; want %q, %q, %v",
 				tt.in, v, rest, found, tt.wantValue, tt.wantRest, tt.wantFound)
 		}
 	}
 }
+
+// Issue #304: "datumctl <plugin> <subcmd> --session X" must run the plugin as
+// that session. The flag is consumed by datumctl and turned into DATUM_SESSION
+// (plus the matching API host) instead of being handed to the plugin, which
+// would reject the whole command with "unknown flag: --session".
+func TestForwardPlugin_sessionFlagAfterPluginName(t *testing.T) {
+	// Not parallel — mutates os.Args, execPlatform, and environment.
+	factory := setupPluginOverrideEnv(t)
+	managedDir := t.TempDir()
+	// Isolate PATH so only the managed binary resolves the plugin name.
+	t.Setenv("PATH", t.TempDir())
+
+	binaryPath := writeFakeBinary(t, managedDir, "assistant")
+	writeManifest(t, managedDir, &pluginstore.Manifest{
+		Plugins: map[string]*pluginstore.InstalledPlugin{
+			"assistant": {SHA256: sha256HexFile(t, binaryPath)},
+		},
+	})
+
+	var gotArgs, gotEnv []string
+	execCalled := false
+	origExec := execPlatform
+	execPlatform = func(_ string, args []string, env []string) error {
+		execCalled, gotArgs, gotEnv = true, args, env
+		return nil
+	}
+	t.Cleanup(func() { execPlatform = origExec })
+
+	origArgs := os.Args
+	os.Args = []string{"datumctl", "assistant", "card", "--session", ovStaging}
+	t.Cleanup(func() { os.Args = origArgs })
+
+	if err := ForwardPlugin(managedDir, buildMinimalCobraTree(), factory); err != nil {
+		t.Fatalf("ForwardPlugin: %v", err)
+	}
+	if !execCalled {
+		t.Fatal("plugin was not exec'd")
+	}
+	if want := []string{"card"}; !slices.Equal(gotArgs, want) {
+		t.Errorf("forwarded args = %v, want %v (--session must not reach the plugin)", gotArgs, want)
+	}
+	if got := envValue(gotEnv, "DATUM_SESSION"); got != ovStaging {
+		t.Errorf("DATUM_SESSION = %q, want %q", got, ovStaging)
+	}
+	if got := envValue(gotEnv, "DATUM_API_HOST"); got != "api.staging.env.datum.net" {
+		t.Errorf("DATUM_API_HOST = %q, want the override session's host", got)
+	}
+}
+
+// A --session with no value must fail with datumctl's own message rather than
+// falling through and running the plugin as the active session.
+func TestApplyPluginSessionOverride_missingValue(t *testing.T) {
+	setupPluginOverrideEnv(t)
+	err := applyPluginSessionOverride("", true)
+	if err == nil {
+		t.Fatal("expected an error for --session with no value")
+	}
+	if _, ok := customerrors.IsUserError(err); !ok {
+		t.Errorf("error is not a UserError: %v", err)
+	}
+	if datumconfig.HasSessionOverride() {
+		t.Error("a failed override must not leave one installed")
+	}
+}