cli: let flags be written anywhere on the line

README.md

@@ -276,10 +276,6 @@ account's trusted device, and each asking for confirmation unless you pass
 `--yes`. Remove the default and, if a single account is left, it takes over; if
 several remain, dwshell asks you to pick one rather than choosing for you.
 
-Flags follow their subcommand — `dwshell list --account …`, not
-`dwshell --account … list` — except in the shell shortcut, where
-`dwshell --account … myserver` works too.
-
 #### Agent name vs subcommand
 
 `dwshell <agent>` is a convenience shortcut: the first argument is treated as an

cmd/dwshell/account.go

@@ -15,18 +15,21 @@ import (
 // who log in with more than one account. With a single account there is nothing
 // here worth running.
 func cmdAccount(ctx context.Context, args []string) int {
-	if len(args) == 0 {
+	// The verb is the first positional, so flags may sit on either side of it.
+	flagArgs, pos := partitionArgs(args)
+	if len(pos) == 0 {
 		return fail("usage: dwshell account <list|default|rm>")
 	}
-	switch args[0] {
+	rest := append(append([]string{}, flagArgs...), pos[1:]...)
+	switch pos[0] {
 	case "list":
-		return cmdAccountList(args[1:])
+		return cmdAccountList(rest)
 	case "default":
-		return cmdAccountDefault(args[1:])
+		return cmdAccountDefault(rest)
 	case "rm":
-		return cmdAccountRemove(ctx, args[1:])
+		return cmdAccountRemove(ctx, rest)
 	default:
-		return fail("unknown account subcommand %q", args[0])
+		return fail("unknown account subcommand %q", pos[0])
 	}
 }
 
@@ -42,7 +45,8 @@ func cmdAccountList(args []string) int {
 	asJSON := false
 	fs.StringVar(&configPath, "config", "", "config file path")
 	fs.BoolVar(&asJSON, "json", false, "machine-readable output")
-	if err := fs.Parse(args); err != nil {
+	flagArgs, _ := partitionArgs(args)
+	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
 	cfg, err := config.Load(configPath)
@@ -80,23 +84,24 @@ func cmdAccountDefault(args []string) int {
 	fs := newFlags("account default")
 	var configPath string
 	fs.StringVar(&configPath, "config", "", "config file path")
-	if err := fs.Parse(args); err != nil {
+	flagArgs, pos := partitionArgs(args)
+	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
-	if fs.NArg() != 1 {
+	if len(pos) != 1 {
 		return fail("usage: dwshell account default <email>")
 	}
 	cfg, err := config.Load(configPath)
 	if err != nil {
 		return fail("%v", err)
 	}
-	if err := cfg.SetDefault(fs.Arg(0)); err != nil {
+	if err := cfg.SetDefault(pos[0]); err != nil {
 		return fail("%v", err)
 	}
 	if err := cfg.Save(); err != nil {
 		return fail("%v", err)
 	}
-	fmt.Fprintf(os.Stderr, "default account is now %s\n", fs.Arg(0))
+	fmt.Fprintf(os.Stderr, "default account is now %s\n", pos[0])
 	return 0
 }
 
@@ -106,10 +111,14 @@ func cmdAccountRemove(ctx context.Context, args []string) int {
 	assumeYes := false
 	fs.StringVar(&configPath, "config", "", "config file path")
 	fs.BoolVar(&assumeYes, "yes", false, "do not ask for confirmation")
-	email, flagArgs := extractPositional(args)
+	flagArgs, pos := partitionArgs(args)
 	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
+	email := ""
+	if len(pos) > 0 {
+		email = pos[0]
+	}
 	if email == "" {
 		return fail("usage: dwshell account rm <email> [--yes]")
 	}
@@ -130,28 +139,6 @@ func cmdAccountRemove(ctx context.Context, args []string) int {
 	return 0
 }
 
-// extractPositional pulls the first non-flag token out of a subcommand's args,
-// returning it with the flags that surrounded it. Go's flag package stops at
-// the first positional, so `account rm a@b --yes` would otherwise drop --yes;
-// this mirrors what extractAgent does for the shell shortcut.
-func extractPositional(args []string) (pos string, flagArgs []string) {
-	i := 0
-	for i < len(args) {
-		a := args[i]
-		if len(a) > 0 && a[0] == '-' {
-			flagArgs = append(flagArgs, a)
-			if !containsEq(a) && valueFlags[trimDashes(a)] && i+1 < len(args) {
-				i++
-				flagArgs = append(flagArgs, args[i])
-			}
-			i++
-			continue
-		}
-		return a, append(flagArgs, args[i+1:]...)
-	}
-	return "", flagArgs
-}
-
 // confirm gates a change that cannot be undone. With a terminal it asks; with
 // none — a script, a CI job — it refuses unless the caller passed --yes, so
 // automation cannot drop an account by accident.

cmd/dwshell/agent.go

@@ -65,22 +65,25 @@ func printAgentJSON(a agentJSON) {
 // cmdAgentManage dispatches the `dwshell agent <verb>` family. It is separate
 // from cmdAgent, which is the shell shortcut for `dwshell <agent>`.
 func cmdAgentManage(ctx context.Context, args []string) int {
-	if len(args) == 0 {
+	// The verb is the first positional, so flags may sit on either side of it.
+	flagArgs, pos := partitionArgs(args)
+	if len(pos) == 0 {
 		return fail("usage: dwshell agent <create|code|reinstall|rm|group> …")
 	}
-	switch args[0] {
+	rest := append(append([]string{}, flagArgs...), pos[1:]...)
+	switch pos[0] {
 	case "create":
-		return cmdAgentCreate(ctx, args[1:])
+		return cmdAgentCreate(ctx, rest)
 	case "code":
-		return cmdAgentCode(ctx, args[1:])
+		return cmdAgentCode(ctx, rest)
 	case "rm":
-		return cmdAgentLifecycle(ctx, args[1:], "rm")
+		return cmdAgentLifecycle(ctx, rest, "rm")
 	case "reinstall":
-		return cmdAgentLifecycle(ctx, args[1:], "reinstall")
+		return cmdAgentLifecycle(ctx, rest, "reinstall")
 	case "group":
-		return cmdAgentGroup(ctx, args[1:])
+		return cmdAgentGroup(ctx, rest)
 	default:
-		return fail("unknown agent subcommand %q", args[0])
+		return fail("unknown agent subcommand %q", pos[0])
 	}
 }
 
@@ -94,11 +97,15 @@ func cmdAgentCreate(ctx context.Context, args []string) int {
 	fs.StringVar(&description, "description", "", "free-text description")
 	fs.StringVar(&group, "group", "", "existing group to place the agent in")
 	fs.BoolVar(&asJSON, "json", false, "machine-readable output")
-	name, flagArgs := extractPositional(args)
+	flagArgs, pos := partitionArgs(args)
 	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
-	if name == "" || fs.NArg() != 0 {
+	name := ""
+	if len(pos) > 0 {
+		name = pos[0]
+	}
+	if name == "" || len(pos) != 1 {
 		return fail("usage: dwshell agent create <name> [--group G] [--description D] [--json]")
 	}
 
@@ -194,10 +201,14 @@ func cmdAgentCode(ctx context.Context, args []string) int {
 	fs.StringVar(&configPath, "config", "", "config file path")
 	fs.StringVar(&account, "account", "", "account to use when several are logged in")
 	fs.BoolVar(&asJSON, "json", false, "machine-readable output")
-	name, flagArgs := extractPositional(args)
+	flagArgs, pos := partitionArgs(args)
 	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
+	name := ""
+	if len(pos) > 0 {
+		name = pos[0]
+	}
 	if name == "" {
 		return fail("usage: dwshell agent code <agent> [--json]")
 	}
@@ -228,10 +239,14 @@ func cmdAgentLifecycle(ctx context.Context, args []string, verb string) int {
 	fs.StringVar(&configPath, "config", "", "config file path")
 	fs.StringVar(&account, "account", "", "account to use when several are logged in")
 	fs.BoolVar(&assumeYes, "yes", false, "do not ask for confirmation")
-	name, flagArgs := extractPositional(args)
+	flagArgs, pos := partitionArgs(args)
 	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
+	name := ""
+	if len(pos) > 0 {
+		name = pos[0]
+	}
 	if name == "" {
 		return fail("usage: dwshell agent %s <agent> [--yes]", verb)
 	}
@@ -284,16 +299,20 @@ func cmdAgentGroup(ctx context.Context, args []string) int {
 	fs.StringVar(&configPath, "config", "", "config file path")
 	fs.StringVar(&account, "account", "", "account to use when several are logged in")
 	fs.BoolVar(&none, "none", false, "remove the agent from its group")
-	name, flagArgs := extractPositional(args)
+	flagArgs, pos := partitionArgs(args)
 	if err := fs.Parse(flagArgs); err != nil {
 		return 2
 	}
+	name := ""
+	if len(pos) > 0 {
+		name = pos[0]
+	}
 	groupName := ""
 	if !none {
-		if fs.NArg() != 1 {
+		if len(pos) != 2 {
 			return fail("usage: dwshell agent group <agent> <group>   (or --none)")
 		}
-		groupName = fs.Arg(0)
+		groupName = pos[1]
 	}
 	if name == "" {
 		return fail("usage: dwshell agent group <agent> <group>   (or --none)")

cmd/dwshell/args.go

@@ -0,0 +1,50 @@
+package main
+
+import "strings"
+
+// valueFlags names every flag that consumes a following token as its value.
+//
+// This is what tells a value apart from a positional: given `--account a@b
+// list`, only this registry says that `a@b` belongs to `--account` and `list`
+// is the command. A value flag missing from here would make dwshell take the
+// value for a command name and quietly do the wrong thing, so a test walks the
+// package's own source and holds this list to the flags actually registered.
+var valueFlags = map[string]bool{
+	"account":     true,
+	"c":           true,
+	"config":      true,
+	"description": true,
+	"group":       true,
+	"term":        true,
+	"timeout":     true,
+	"user":        true,
+}
+
+// partitionArgs splits arguments into flags (each with its value) and
+// positionals, in their original relative order, so that flags may be written
+// anywhere: `dwshell list --account a@b` and `dwshell --account a@b list` are
+// the same command.
+//
+// Everything after a bare "--" is positional, which is how a path or an agent
+// name that begins with a dash can still be passed.
+func partitionArgs(args []string) (flags, positionals []string) {
+	for i := 0; i < len(args); i++ {
+		a := args[i]
+		if a == "--" {
+			positionals = append(positionals, args[i+1:]...)
+			return flags, positionals
+		}
+		if len(a) > 1 && a[0] == '-' {
+			flags = append(flags, a)
+			// --flag=value carries its value; a bare value flag takes the next
+			// token, which must not then be read as a positional.
+			if !strings.Contains(a, "=") && valueFlags[trimDashes(a)] && i+1 < len(args) {
+				i++
+				flags = append(flags, args[i])
+			}
+			continue
+		}
+		positionals = append(positionals, a)
+	}
+	return flags, positionals
+}

cmd/dwshell/args_test.go

@@ -0,0 +1,110 @@
+package main
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"path/filepath"
+	"reflect"
+	"testing"
+)
+
+func TestPartitionArgsSeparatesFlagsFromPositionals(t *testing.T) {
+	cases := []struct {
+		name  string
+		args  []string
+		flags []string
+		pos   []string
+	}{
+		{"flags first", []string{"--account", "a@b", "list"}, []string{"--account", "a@b"}, []string{"list"}},
+		{"flags last", []string{"list", "--account", "a@b"}, []string{"--account", "a@b"}, []string{"list"}},
+		{"flags around", []string{"-c", "ls", "GHE", "--term", "xterm"},
+			[]string{"-c", "ls", "--term", "xterm"}, []string{"GHE"}},
+		{"equals form needs no lookahead", []string{"--account=a@b", "list"}, []string{"--account=a@b"}, []string{"list"}},
+		{"boolean flags consume nothing", []string{"list", "--json"}, []string{"--json"}, []string{"list"}},
+		{"several positionals keep their order", []string{"get", "-r", "a:b", "c"},
+			[]string{"-r"}, []string{"get", "a:b", "c"}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			flags, pos := partitionArgs(tc.args)
+			if !reflect.DeepEqual(flags, tc.flags) {
+				t.Errorf("flags = %v, want %v", flags, tc.flags)
+			}
+			if !reflect.DeepEqual(pos, tc.pos) {
+				t.Errorf("positionals = %v, want %v", pos, tc.pos)
+			}
+		})
+	}
+}
+
+// Everything after "--" is positional, so a path or command that looks like a
+// flag can still be passed.
+func TestPartitionArgsStopsAtADoubleDash(t *testing.T) {
+	flags, pos := partitionArgs([]string{"rm", "--yes", "--", "-weird-name"})
+	if !reflect.DeepEqual(flags, []string{"--yes"}) {
+		t.Errorf("flags = %v", flags)
+	}
+	if !reflect.DeepEqual(pos, []string{"rm", "-weird-name"}) {
+		t.Errorf("positionals = %v", pos)
+	}
+}
+
+// valueFlags decides where a positional begins, so a flag that takes a value
+// and is missing from it makes dwshell mistake that value for a command name —
+// silently doing the wrong thing rather than failing. This walks the package's
+// own source and holds the registry to what is actually registered.
+func TestValueFlagsMatchesTheFlagsActuallyRegistered(t *testing.T) {
+	files, err := filepath.Glob("*.go")
+	if err != nil {
+		t.Fatal(err)
+	}
+	found := map[string]string{} // flag name -> the FlagSet method that registers it
+	fset := token.NewFileSet()
+	for _, f := range files {
+		file, err := parser.ParseFile(fset, f, nil, 0)
+		if err != nil {
+			t.Fatalf("parse %s: %v", f, err)
+		}
+		ast.Inspect(file, func(n ast.Node) bool {
+			call, ok := n.(*ast.CallExpr)
+			if !ok {
+				return true
+			}
+			sel, ok := call.Fun.(*ast.SelectorExpr)
+			if !ok || len(call.Args) < 2 {
+				return true
+			}
+			method := sel.Sel.Name
+			// Var methods take (ptr, name, default, usage); Bool ones consume no
+			// following token, every other kind does.
+			if len(method) < 4 || method[len(method)-3:] != "Var" {
+				return true
+			}
+			lit, ok := call.Args[1].(*ast.BasicLit)
+			if !ok || lit.Kind != token.STRING {
+				return true
+			}
+			name := lit.Value[1 : len(lit.Value)-1]
+			if method == "BoolVar" {
+				return true
+			}
+			found[name] = method
+			return true
+		})
+	}
+	if len(found) == 0 {
+		t.Fatal("no flags found: the walk is not seeing the source")
+	}
+	for name, method := range found {
+		if !valueFlags[name] {
+			t.Errorf("--%s takes a value (%s) but is missing from valueFlags: "+
+				"a positional after it would be mistaken for the flag's value", name, method)
+		}
+	}
+	for name := range valueFlags {
+		if _, ok := found[name]; !ok {
+			t.Errorf("valueFlags lists --%s, which no command registers as a value flag", name)
+		}
+	}
+}