fix(cli): accept the documented argument order on every command

go/internal/cli/argorder_test.go

@@ -0,0 +1,197 @@
+// go/internal/cli/argorder_test.go — the argument order our own help text
+// documents must parse. Go's flag.FlagSet stops at the first positional, so
+// `mir share box --ttl 1h` used to drop `--ttl 1h` into Args() and fail the
+// arity check (#112, after the same trap hit `mir attach` in #45). These tests
+// drive the real dispatch, one row per command that takes positionals AND
+// flags, so the class cannot come back a third time.
+package cli
+
+import (
+	"bytes"
+	"flag"
+	"io"
+	"strings"
+	"testing"
+	"time"
+)
+
+// TestParseArgsAcceptsAnyOrder pins the shared helper: flags before, after, and
+// between positionals all land on the same values, and "--" ends flag parsing.
+func TestParseArgsAcceptsAnyOrder(t *testing.T) {
+	cases := []struct {
+		name      string
+		args      []string
+		wantPos   []string
+		wantTTL   time.Duration
+		wantWrite bool
+	}{
+		{"flags first (Go's native order)", []string{"--ttl", "2h", "--write", "box"}, []string{"box"}, 2 * time.Hour, true},
+		{"documented order", []string{"box", "--ttl", "2h", "--write"}, []string{"box"}, 2 * time.Hour, true},
+		{"interleaved", []string{"--ttl", "2h", "box", "--write"}, []string{"box"}, 2 * time.Hour, true},
+		{"single dash spelling", []string{"box", "-ttl", "2h"}, []string{"box"}, 2 * time.Hour, false},
+		{"two positionals, trailing flag", []string{"box", "newbox", "--write"}, []string{"box", "newbox"}, time.Hour, true},
+		{"two positionals, flag between", []string{"box", "--ttl", "2h", "newbox"}, []string{"box", "newbox"}, 2 * time.Hour, false},
+		{"no positionals", nil, nil, time.Hour, false},
+		{"terminator keeps dashes positional", []string{"--write", "box", "--", "-weird", "--weirder"}, []string{"box", "-weird", "--weirder"}, time.Hour, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fs := flag.NewFlagSet("test", flag.ContinueOnError)
+			fs.SetOutput(io.Discard)
+			ttl := fs.Duration("ttl", time.Hour, "")
+			write := fs.Bool("write", false, "")
+			got := parseArgs(fs, tc.args)
+			if strings.Join(got, "|") != strings.Join(tc.wantPos, "|") {
+				t.Errorf("positionals = %q, want %q", got, tc.wantPos)
+			}
+			if *ttl != tc.wantTTL {
+				t.Errorf("--ttl = %v, want %v", *ttl, tc.wantTTL)
+			}
+			if *write != tc.wantWrite {
+				t.Errorf("--write = %v, want %v", *write, tc.wantWrite)
+			}
+		})
+	}
+}
+
+// TestDocumentedArgOrderParses drives every command that mixes positionals with
+// flags. Each row runs the human-friendly documented order AND Go flag's native
+// leading-flag order; both must get past parsing to the same real answer, and
+// neither may come back with a usage line.
+func TestDocumentedArgOrderParses(t *testing.T) {
+	t.Setenv("MIR_NO_UPDATE_CHECK", "1")
+	t.Setenv("MIR_SIGNAL", "http://127.0.0.1:1") // dead relay: discovery fails fast
+	withShareTTY(t, false)
+	withRevokeTTY(t, false)
+	dir := t.TempDir()
+
+	cases := []struct {
+		name string
+		// documented is the order our usage strings and help text show.
+		documented []string
+		// native is Go flag's own leading-flag order, which always worked.
+		native []string
+		// want is a fragment of the answer both orders must reach: an honest
+		// refusal about the real argument, never a usage line.
+		want string
+	}{
+		{
+			name:       "attach",
+			documented: []string{"attach", "box", "--dir", dir},
+			native:     []string{"attach", "--dir", dir, "box"},
+			want:       `unknown machine "box"`,
+		},
+		{
+			name:       "attach several machines",
+			documented: []string{"attach", "box", "other", "--dir", dir},
+			native:     []string{"attach", "--dir", dir, "box", "other"},
+			want:       `unknown machine "box"`,
+		},
+		{
+			// run is the deliberate exception: everything after <machine> is
+			// the remote command (`-n` below belongs to echo), so mir's own
+			// flags come first. Both spellings here are that documented order.
+			name:       "run",
+			documented: []string{"run", "--dir", dir, "box", "echo", "-n", "hi"},
+			native:     []string{"run", "--dir", dir, "--window", "1s", "box", "echo", "hi"},
+			want:       `unknown machine "box"`,
+		},
+		{
+			// The #112 bug, verbatim.
+			name:       "share",
+			documented: []string{"share", "box", "--ttl", "1h", "--dir", dir},
+			native:     []string{"share", "--ttl", "1h", "--dir", dir, "box"},
+			want:       "needs a person at the terminal",
+		},
+		{
+			name:       "share revoke",
+			documented: []string{"share", "revoke", "abcdef12", "--dir", dir},
+			native:     []string{"share", "revoke", "--dir", dir, "abcdef12"},
+			want:       `no share matches "abcdef12"`,
+		},
+		{
+			name:       "join",
+			documented: []string{"join", "not-a-code", "--dir", dir},
+			native:     []string{"join", "--dir", dir, "not-a-code"},
+			want:       "bad pairing code",
+		},
+		{
+			name:       "pair",
+			documented: []string{"pair", "not-a-code", "--dir", dir},
+			native:     []string{"pair", "--dir", dir, "not-a-code"},
+			want:       "bad pairing code",
+		},
+		{
+			name:       "machine rename",
+			documented: []string{"machine", "rename", "box", "newbox", "--dir", dir},
+			native:     []string{"machine", "rename", "--dir", dir, "box", "newbox"},
+			want:       `unknown machine "box"`,
+		},
+		{
+			name:       "machine revoke",
+			documented: []string{"machine", "revoke", "box", "--yes", "--dir", dir},
+			native:     []string{"machine", "revoke", "--yes", "--dir", dir, "box"},
+			want:       `unknown machine "box"`,
+		},
+	}
+
+	for _, tc := range cases {
+		for _, order := range []struct {
+			label string
+			argv  []string
+		}{{"documented", tc.documented}, {"native", tc.native}} {
+			t.Run(tc.name+"/"+order.label, func(t *testing.T) {
+				var out, errb bytes.Buffer
+				a := &app{in: strings.NewReader(""), out: &out, errOut: &errb, binary: "mir"}
+				if code := a.run(order.argv); code == 0 {
+					t.Fatalf("%v should have refused, stdout=%q", order.argv, out.String())
+				}
+				if strings.Contains(errb.String(), "usage:") {
+					t.Fatalf("%v was refused with a usage line — the documented order must parse:\n%s", order.argv, errb.String())
+				}
+				if !strings.Contains(errb.String(), tc.want) {
+					t.Fatalf("%v: stderr missing %q:\n%s", order.argv, tc.want, errb.String())
+				}
+			})
+		}
+	}
+}
+
+// Flag-only commands have nothing to interleave, but `identity show --dir X`
+// is the shape users type most, so pin that it still works end to end.
+func TestFlagOnlyCommandsUnchanged(t *testing.T) {
+	t.Setenv("MIR_NO_UPDATE_CHECK", "1")
+	t.Setenv("MIR_SIGNAL", "http://127.0.0.1:1")
+	dir := t.TempDir()
+	var out, errb bytes.Buffer
+	a := &app{in: strings.NewReader(""), out: &out, errOut: &errb, binary: "mir"}
+	if code := a.run([]string{"identity", "show", "--dir", dir}); code != 0 {
+		t.Fatalf("identity show exit = %d, stderr = %q", code, errb.String())
+	}
+	if strings.TrimSpace(out.String()) == "" {
+		t.Fatal("identity show printed no owner id")
+	}
+}
+
+// A share still refuses without a terminal, whichever order the arguments come
+// in: parsing was widened, the consent gate was not.
+func TestShareStillRefusesWithoutTTY(t *testing.T) {
+	t.Setenv("MIR_NO_UPDATE_CHECK", "1")
+	t.Setenv("MIR_SIGNAL", "http://127.0.0.1:1")
+	withShareTTY(t, false)
+	dir := t.TempDir()
+	for _, argv := range [][]string{
+		{"box", "--write", "--dir", dir},
+		{"--write", "--dir", dir, "box"},
+	} {
+		var out bytes.Buffer
+		a := &app{in: strings.NewReader("box\n"), out: &out, errOut: io.Discard, binary: "mir"}
+		err := a.cmdShare(argv)
+		if err == nil || !strings.Contains(err.Error(), "needs a person at the terminal") {
+			t.Fatalf("share %v: err = %v, want the no-TTY refusal", argv, err)
+		}
+		if strings.Contains(out.String(), "Type the machine name") {
+			t.Fatalf("share %v reached the write prompt before the TTY refusal:\n%s", argv, out.String())
+		}
+	}
+}

go/internal/cli/cli.go

@@ -154,7 +154,7 @@ func (a *app) guide() {
 	p("    " + b + " pair              add another owner later — prints a QR + safety number")
 	p("")
 	p("  Reach your machines (where you are):")
-	p("    " + b + "                   your machines, live — Enter attaches (Ctrl-O d comes back)")
+	p("    " + b + "                   your machines, live — Enter attaches (Ctrl-O then d comes back)")
 	p("    " + b + " pair <code>       pair to a machine (compare the safety numbers)")
 	p("    " + b + " attach            continue where you left off — short: " + b + " a")
 	p("    " + b + " attach <name>     open its shell, peer-to-peer — short: " + b + " a <name>")

go/internal/cli/client_cmds.go

@@ -100,10 +100,14 @@ func (a *app) cmdRun(args []string) error {
 	dir := fs.String("dir", defaultClientDir(), "client state directory")
 	ice := iceFlags(fs)
 	window := fs.Duration("window", 3*time.Second, "how long to stream output before exiting")
+	// run is the one command that does NOT accept flags after the positionals:
+	// everything past <machine> is the remote command, so `mir run box ls -la`
+	// must hand `-la` to ls, not to mir. Its own flags therefore come first,
+	// which is what the usage line says.
 	_ = fs.Parse(args)
 	rest := fs.Args()
 	if len(rest) < 2 {
-		return fmt.Errorf("usage: mir run <machine> <command...>")
+		return fmt.Errorf("usage: %s run [flags] <machine> <command...>   (flags come first; everything after <machine> is the remote command)", a.binary)
 	}
 	name := rest[0]
 	cmd := strings.Join(rest[1:], " ")
@@ -379,7 +383,7 @@ func sameRelay(a, b string) bool {
 
 func (a *app) cmdMachine(args []string) error {
 	if len(args) == 0 {
-		return fmt.Errorf("usage: mir machine rename <name> <new-name> | mir machine revoke <name> [--yes]")
+		return fmt.Errorf("usage: %s machine rename <name> <new-name> | %s machine revoke <name> [--yes]", a.binary, a.binary)
 	}
 	switch args[0] {
 	case "revoke":
@@ -401,10 +405,9 @@ func (a *app) cmdMachineRename(args []string) error {
 	fs := flag.NewFlagSet("machine rename", flag.ExitOnError)
 	dir := fs.String("dir", defaultClientDir(), "client state directory")
 	ice := iceFlags(fs)
-	_ = fs.Parse(args)
-	rest := fs.Args()
+	rest := parseArgs(fs, args)
 	if len(rest) != 2 {
-		return fmt.Errorf("usage: mir machine rename <name> <new-name>")
+		return fmt.Errorf("usage: %s machine rename <name> <new-name>", a.binary)
 	}
 	name, newName := rest[0], rest[1]
 	if !agent.ValidMachineName(newName) {
@@ -493,23 +496,13 @@ func (a *app) cmdMachineRevoke(args []string) error {
 	fs := flag.NewFlagSet("machine revoke", flag.ExitOnError)
 	dir := fs.String("dir", defaultClientDir(), "client state directory")
 	yes := fs.Bool("yes", false, "skip the interactive confirmation (scripts)")
-	// Accept the human-friendly documented form `revoke box --yes` as well as
-	// Go flag's native `revoke --yes box` ordering.
-	name := ""
-	if len(args) > 0 && !strings.HasPrefix(args[0], "-") {
-		name, args = args[0], args[1:]
-	}
-	_ = fs.Parse(args)
-	if name == "" {
-		if len(fs.Args()) == 1 {
-			name = fs.Args()[0]
-		}
-	} else if len(fs.Args()) != 0 {
-		return fmt.Errorf("usage: mir machine revoke <name> [--yes]")
-	}
-	if name == "" || len(fs.Args()) > 1 {
-		return fmt.Errorf("usage: mir machine revoke <name> [--yes]")
+	// Accepts the human-friendly documented form `revoke box --yes` as well as
+	// Go flag's native `revoke --yes box` ordering (parseArgs).
+	rest := parseArgs(fs, args)
+	if len(rest) != 1 {
+		return fmt.Errorf("usage: %s machine revoke <name> [--yes]", a.binary)
 	}
+	name := rest[0]
 	// Consent before any identity or network work. Interactive runs get the
 	// plain-words prompt; scripted runs must state --yes (fail closed).
 	if !*yes {
@@ -593,17 +586,16 @@ func (a *app) cmdAttach(args []string) error {
 	prefixFlag := fs.String("prefix", "ctrl-o", "multiplexer switch key (e.g. ctrl-o, ctrl-a, ctrl-space)")
 	relayOnly := fs.Bool("relay-only", false, "deprecated: no effect (one connection now carries direct and relayed)")
 	ice := iceFlags(fs)
-	_ = fs.Parse(args)
+	names := parseArgs(fs, args)
 	if *relayOnly {
 		fmt.Fprintln(a.errOut, "note: --relay-only no longer does anything and will go away — LAN-direct now rides the same connection (direct when possible, relayed when not)")
 	}
-	names := fs.Args()
 	if len(names) == 0 {
 		// A bare `mir attach` on a terminal means "continue": the last-used
 		// machine, else the only one there is, else the overview. Scripts
 		// (no TTY) keep the explicit usage error.
 		if !term.IsTerminal(int(os.Stdin.Fd())) || !term.IsTerminal(int(os.Stdout.Fd())) {
-			return fmt.Errorf("usage: mir attach <machine> [machine...]")
+			return fmt.Errorf("usage: %s attach <machine> [machine...]", a.binary)
 		}
 		name, err := a.defaultAttachTarget(*dir)
 		if err != nil {

go/internal/cli/overview.go

@@ -414,7 +414,10 @@ func (ov *overviewState) ice() []peer.ICEServer {
 func (ov *overviewState) attach(ctx context.Context, m client.Machine) error {
 	a := ov.app
 	fmt.Fprint(a.out, altScreenOff)
-	fmt.Fprintf(os.Stderr, "[%s] attached to %s — Ctrl-O then d comes back to your machines\r\n", a.binary, m.Name)
+	// Same opening as the bare-attach banner, then the gesture this entry path
+	// really has: the overview is still running behind the attach, so Ctrl-O
+	// then d returns to it.
+	fmt.Fprintf(os.Stderr, "[%s] %s; Ctrl-O then d comes back to your machines\r\n", a.binary, client.AttachHint(m.Name))
 
 	attachCtx, cancel := context.WithCancel(ctx)
 	defer cancel()

go/internal/cli/pair.go

@@ -119,9 +119,8 @@ func (a *app) cmdPair(args []string) error {
 	webURL := fs.String("web", defaults.WebURL(), "browser SPA base URL the QR opens (responder)")
 	confirmSAS := fs.String("confirm-sas", "", "non-interactive: the expected safety number; pairing is committed only if it matches the computed one")
 	yes := fs.Bool("yes", false, "non-interactive: commit pairing without comparing the safety number (only if you trust the channel out-of-band)")
-	_ = fs.Parse(args)
 
-	mode, code, err := classifyPair(fs.Args())
+	mode, code, err := classifyPair(parseArgs(fs, args))
 	if err != nil {
 		return err
 	}

go/internal/cli/share.go

@@ -51,11 +51,11 @@ func (a *app) cmdShare(args []string) error {
 	session := fs.String("session", "main", "tmux session the share covers")
 	webURL := fs.String("web", defaults.WebURL(), "browser SPA base URL the invite link opens")
 	ice := iceFlags(fs)
-	_ = fs.Parse(args)
-	if len(fs.Args()) != 1 {
-		return fmt.Errorf("usage: mir share <machine> [--ttl 1h] [--write] [--session main] | mir share ls | mir share revoke <id>")
+	rest := parseArgs(fs, args)
+	if len(rest) != 1 {
+		return fmt.Errorf("usage: %s share <machine> [--ttl 1h] [--write] [--session main] | %s share ls | %s share revoke <id>", a.binary, a.binary, a.binary)
 	}
-	name := fs.Arg(0)
+	name := rest[0]
 	if !shareIsTTY() {
 		return fmt.Errorf("sharing needs a person at the terminal to compare the safety number — there is no --yes; run `%s share` interactively", a.binary)
 	}
@@ -236,11 +236,11 @@ func (a *app) cmdShareRevoke(args []string) error {
 	fs := flag.NewFlagSet("share revoke", flag.ExitOnError)
 	dir := fs.String("dir", defaultClientDir(), "client state directory")
 	ice := iceFlags(fs)
-	_ = fs.Parse(args)
-	if len(fs.Args()) != 1 {
+	rest := parseArgs(fs, args)
+	if len(rest) != 1 {
 		return fmt.Errorf("usage: %s share revoke <id>   (ids: `%s share ls`)", a.binary, a.binary)
 	}
-	share, err := client.ResolveShareGID(*dir, fs.Arg(0))
+	share, err := client.ResolveShareGID(*dir, rest[0])
 	if err != nil {
 		return err
 	}
@@ -290,11 +290,11 @@ func modeWord(mode string) string {
 func (a *app) cmdJoin(args []string) error {
 	fs := flag.NewFlagSet("join", flag.ExitOnError)
 	dir := fs.String("dir", defaultClientDir(), "client state directory")
-	_ = fs.Parse(args)
-	if len(fs.Args()) != 1 {
-		return fmt.Errorf("usage: mir join <code>")
+	rest := parseArgs(fs, args)
+	if len(rest) != 1 {
+		return fmt.Errorf("usage: %s join <code>", a.binary)
 	}
-	signalURL, token, err := pairing.DecodeCode(fs.Arg(0))
+	signalURL, token, err := pairing.DecodeCode(rest[0])
 	if err != nil {
 		return err
 	}