From 10c566d731b53172f4e3c94ec67b77ae1415bc2b Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Sun, 30 Aug 2026 16:35:18 +0200 Subject: [PATCH] fix(cli): accept the documented argument order on every command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mir share --ttl 1h` — the order the command's own usage string shows — was refused. Go's flag.FlagSet stops at the first positional, so `--ttl 1h` landed in fs.Args() and tripped the arity check; only the flags-first spelling parsed. The same trap hit `mir attach --dir X` (#45), and G1d had fixed it for `machine revoke` alone with a bespoke argument peel. One mechanism now, not one per command: parseArgs (cli/shared.go) parses flags and positionals in any order and every command that takes both goes through it — attach, share, share revoke, join, pair, machine rename, machine revoke. `mir run` keeps flags-first on purpose: everything after is the remote command, so `mir run box ls -la` must hand `-la` to ls; its usage line now says so. Usage strings print the running binary instead of a hardcoded "mir", so the mir-agent shim reads right. Consent gates are untouched: `share --write` still makes you type the machine name, `share` still refuses without a TTY, `machine revoke` still asks or wants --yes. Copy: the two attach banners described the detach gesture differently and each omitted what the other said. They now share an opening — client.AttachHint, "attached to X — Ctrl-C goes to the shell" — and each adds only the way out it really has: closing the client for a bare `mir attach`, Ctrl-O then d from the overview, which is the one place that gesture exists. Tests: a table over the real dispatch runs both orders for every command touched, so this class cannot regress a third time. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KeiotDVE94wEzvc7wcvm1y --- go/internal/cli/argorder_test.go | 197 +++++++++++++++++++++++++++++++ go/internal/cli/cli.go | 2 +- go/internal/cli/client_cmds.go | 40 +++---- go/internal/cli/overview.go | 5 +- go/internal/cli/pair.go | 3 +- go/internal/cli/share.go | 22 ++-- go/internal/cli/shared.go | 39 ++++++ go/internal/client/term.go | 12 +- go/internal/client/term_test.go | 24 ++++ 9 files changed, 304 insertions(+), 40 deletions(-) create mode 100644 go/internal/cli/argorder_test.go create mode 100644 go/internal/client/term_test.go diff --git a/go/internal/cli/argorder_test.go b/go/internal/cli/argorder_test.go new file mode 100644 index 0000000..765d674 --- /dev/null +++ b/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 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()) + } + } +} diff --git a/go/internal/cli/cli.go b/go/internal/cli/cli.go index 5b95fd3..f1d4d1c 100644 --- a/go/internal/cli/cli.go +++ b/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 pair to a machine (compare the safety numbers)") p(" " + b + " attach continue where you left off — short: " + b + " a") p(" " + b + " attach open its shell, peer-to-peer — short: " + b + " a ") diff --git a/go/internal/cli/client_cmds.go b/go/internal/cli/client_cmds.go index ac2a059..902e4d8 100644 --- a/go/internal/cli/client_cmds.go +++ b/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 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 ") + return fmt.Errorf("usage: %s run [flags] (flags come first; everything after 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 | mir machine revoke [--yes]") + return fmt.Errorf("usage: %s machine rename | %s machine revoke [--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 ") + return fmt.Errorf("usage: %s machine rename ", 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 [--yes]") - } - if name == "" || len(fs.Args()) > 1 { - return fmt.Errorf("usage: mir machine revoke [--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 [--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...]") + return fmt.Errorf("usage: %s attach [machine...]", a.binary) } name, err := a.defaultAttachTarget(*dir) if err != nil { diff --git a/go/internal/cli/overview.go b/go/internal/cli/overview.go index 964d4b1..817cb32 100644 --- a/go/internal/cli/overview.go +++ b/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() diff --git a/go/internal/cli/pair.go b/go/internal/cli/pair.go index 74fe97b..cc69683 100644 --- a/go/internal/cli/pair.go +++ b/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 } diff --git a/go/internal/cli/share.go b/go/internal/cli/share.go index be06bcc..69e6fe1 100644 --- a/go/internal/cli/share.go +++ b/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 [--ttl 1h] [--write] [--session main] | mir share ls | mir share revoke ") + rest := parseArgs(fs, args) + if len(rest) != 1 { + return fmt.Errorf("usage: %s share [--ttl 1h] [--write] [--session main] | %s share ls | %s share revoke ", 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 (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 ") + rest := parseArgs(fs, args) + if len(rest) != 1 { + return fmt.Errorf("usage: %s join ", a.binary) } - signalURL, token, err := pairing.DecodeCode(fs.Arg(0)) + signalURL, token, err := pairing.DecodeCode(rest[0]) if err != nil { return err } diff --git a/go/internal/cli/shared.go b/go/internal/cli/shared.go index ce01172..dafdd08 100644 --- a/go/internal/cli/shared.go +++ b/go/internal/cli/shared.go @@ -83,6 +83,45 @@ func hostname() string { return h } +// parseArgs parses fs against args and returns the positionals, accepting flags +// and positionals in ANY order — both the human-friendly form our usage strings +// document (`mir share box --ttl 1h`) and Go flag's native leading-flag form +// (`mir share --ttl 1h box`). +// +// Go's flag.FlagSet stops at the first positional, so without this a documented +// invocation drops its flags into fs.Args(), where they trip the arity check and +// the user is refused for following our own help text (#45, #112). Every command +// that takes positionals AND flags parses through here, so the trap cannot come +// back one command at a time. +// +// A literal "--" ends flag parsing: everything after it is a positional, dashes +// and all. The flag sets here are all flag.ExitOnError, so a bad flag exits +// inside fs.Parse exactly as before. +func parseArgs(fs *flag.FlagSet, args []string) []string { + var literal []string + for i, a := range args { + if a == "--" { + args, literal = args[:i], args[i+1:] + break + } + } + var positional []string + for { + if err := fs.Parse(args); err != nil { + return nil + } + rest := fs.Args() + if len(rest) == 0 { + break + } + // Keep the first positional, then parse what follows it: repeat until + // the flags on both sides of every positional are consumed. + positional = append(positional, rest[0]) + args = rest[1:] + } + return append(positional, literal...) +} + // iceFlags registers --stun/--turn/--turn-user/--turn-pass on fs and returns a // closure building the ICE server list (call after fs.Parse). TURN is the opt-in // symmetric-NAT fallback; Noise keeps it blind to content. diff --git a/go/internal/client/term.go b/go/internal/client/term.go index bb43588..5892c35 100644 --- a/go/internal/client/term.go +++ b/go/internal/client/term.go @@ -14,6 +14,14 @@ import ( "github.com/srcful/terminal-relay/go/internal/peer" ) +// AttachHint is the half of the attach banner every entry path shares: which +// machine you are on, and where Ctrl-C goes (to the shell — it is not a detach +// key). Each caller appends the way out it actually offers, so no banner can +// promise a gesture that does not exist there. +func AttachHint(machineName string) string { + return fmt.Sprintf("attached to %s — Ctrl-C goes to the shell", machineName) +} + // RunInteractive puts the real terminal into raw mode, wires SIGWINCH to RESIZE, // and runs the bridge against stdin/stdout. Restores the terminal on exit. func RunInteractive(ctx context.Context, mc peer.MsgConn, sess *noise.Session, machineName string) error { @@ -26,7 +34,9 @@ func RunInteractive(ctx context.Context, mc peer.MsgConn, sess *noise.Session, m return err } defer func() { _ = term.Restore(fd, old) }() - fmt.Fprintf(os.Stderr, "[mir] attached to %s — Ctrl-C goes to the shell; close the client to detach\r\n", machineName) + // One attach, no picker behind it: closing the client is the way out, and + // Ctrl-O then d belongs to the overview, so it is not claimed here. + fmt.Fprintf(os.Stderr, "[mir] %s; close the client to detach (the session keeps running)\r\n", AttachHint(machineName)) cols, rows, err := term.GetSize(fd) if err != nil { diff --git a/go/internal/client/term_test.go b/go/internal/client/term_test.go new file mode 100644 index 0000000..dc9c489 --- /dev/null +++ b/go/internal/client/term_test.go @@ -0,0 +1,24 @@ +package client + +import ( + "strings" + "testing" +) + +// The attach banner's shared half names the machine and says where Ctrl-C goes. +// It must NOT name a way out: each entry path has a different one (a bare +// `mir attach` closes the client; the overview takes Ctrl-O then d), and a +// banner that promises the wrong gesture is worse than one that promises none. +func TestAttachHint(t *testing.T) { + got := AttachHint("box") + for _, want := range []string{"attached to box", "Ctrl-C goes to the shell"} { + if !strings.Contains(got, want) { + t.Errorf("AttachHint = %q, want it to contain %q", got, want) + } + } + for _, unwanted := range []string{"Ctrl-O", "detach", "comes back"} { + if strings.Contains(got, unwanted) { + t.Errorf("AttachHint = %q — the shared half must leave %q to the caller", got, unwanted) + } + } +}