From e1cb580eea55a757db97b0bdd5e5af23209ad19b Mon Sep 17 00:00:00 2001 From: pratikbin <68642400+pratikbin@users.noreply.github.com> Date: Tue, 30 Jun 2026 18:33:50 +0530 Subject: [PATCH] fix(sandbox): parse sync flags placed after the sandbox arg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit urfave/cli v2 stops flag parsing at the first positional, so `sandbox sync my-box --mode mirror` silently dropped every flag after the sandbox argument — the new --exclude/--mode/--quiet/--no-ignore-vcs plus the pre-existing --local/--remote/-i (the latter masked by the interactive prompts on a TTY). - add syncOptions + parseSyncArgs to recover flags regardless of position, matching the parseDiskCreateArgs / splitForceFlag pattern - extract pure mutagenCreateArgs so the destructive mirror mapping is unit-tested - make runMutagen take output sinks; --quiet now captures sync create output and surfaces it only on failure instead of leaking to stderr - add regression tests for the parser, mirror mapping, and mode mapping Addresses review feedback on #59. --- cmd/sandbox/sync.go | 213 +++++++++++++++++++++++++++++++++------ cmd/sandbox/sync_test.go | 158 +++++++++++++++++++++++++++++ 2 files changed, 338 insertions(+), 33 deletions(-) create mode 100644 cmd/sandbox/sync_test.go diff --git a/cmd/sandbox/sync.go b/cmd/sandbox/sync.go index 663c2f2..0fc8b17 100644 --- a/cmd/sandbox/sync.go +++ b/cmd/sandbox/sync.go @@ -1,6 +1,7 @@ package sandbox import ( + "bytes" "context" "encoding/pem" "errors" @@ -125,8 +126,14 @@ func runSync(c *cli.Context) error { } tty := terminal.IsInteractive() + // urfave/cli v2 stops parsing flags at the first positional, so + // `sync my-box --mode mirror` would otherwise drop every flag after + // the sandbox argument. parseSyncArgs recovers them regardless of + // position (same workaround as parseDiskCreateArgs / splitForceFlag). + opts := parseSyncArgs(c) + // 1. Pick / resolve the sandbox. - ref := strings.TrimSpace(c.Args().First()) + ref := opts.ref var id string if ref == "" { if !tty { @@ -152,7 +159,7 @@ func runSync(c *cli.Context) error { // 2. Local + remote paths. Prompt on TTY when missing; default the // local side to the user's current directory (the common case — // "sync this project I'm sitting in"). - localArg := strings.TrimSpace(c.String("local")) + localArg := opts.local if localArg == "" { if !tty { return errors.New("--local is required (no terminal for interactive prompt)") @@ -170,7 +177,7 @@ func runSync(c *cli.Context) error { localArg = cwd } } - remote := strings.TrimSpace(c.String("remote")) + remote := opts.remote if remote == "" { if !tty { return errors.New("--remote is required (no terminal for interactive prompt)") @@ -185,7 +192,7 @@ func runSync(c *cli.Context) error { remote = strings.TrimSpace(v) } - local, err := validateLocalSyncPath(localArg, c.Bool("force")) + local, err := validateLocalSyncPath(localArg, opts.force) if err != nil { return err } @@ -195,7 +202,7 @@ func runSync(c *cli.Context) error { // Resolve --mode up front so a typo fails before we touch the // sandbox or download Mutagen. - syncMode, err := syncModeToMutagen(c.String("mode")) + syncMode, err := syncModeToMutagen(opts.mode) if err != nil { return err } @@ -206,7 +213,7 @@ func runSync(c *cli.Context) error { } // 3. Resolve the SSH identity. Same auto-detect as `shell --ssh`. - privPath, pubPath, err := resolveIdentity(c.String("identity")) + privPath, pubPath, err := resolveIdentity(opts.identity) if err != nil { return err } @@ -228,14 +235,14 @@ func runSync(c *cli.Context) error { defer cleanup() privPath = unlocked - user := strings.TrimSpace(c.String("user")) + user := opts.user if user == "" { user = "root" } // 4. Install authorized_keys (with consent) + start sshd. Mirror of // the SSH-shell path so sync gets the same modes/sshd setup. - if err = ensureAuthorizedKey(c, client, id, user, ref, pubBytes, keyConsentGiven(c)); err != nil { + if err = ensureAuthorizedKey(c, client, id, user, ref, pubBytes, opts.assumeYes); err != nil { return err } authPath := authorizedKeysPath(user) @@ -274,7 +281,7 @@ fi return fmt.Errorf("could not open tunnel to the sandbox: %w", err) } defer bridge.close() - if err = waitForTCP(ctx, bridge.localAddr, c.Duration("sshd-wait")); err != nil { + if err = waitForTCP(ctx, bridge.localAddr, opts.sshdWait); err != nil { return fmt.Errorf("sshd did not start in time: %w", err) } _, port, _ := net.SplitHostPort(bridge.localAddr) //nolint:errcheck @@ -294,35 +301,35 @@ fi // Mutagen runs ssh from its long-lived daemon, not from this // process. Stop the daemon so the next `create` auto-starts it // under our env, picking up the wrapper PATH. - _ = runMutagen(ctx, mutagenBin, wrapperEnv, "daemon", "stop") //nolint:errcheck + _ = runMutagen(ctx, mutagenBin, wrapperEnv, io.Discard, io.Discard, "daemon", "stop") //nolint:errcheck - quiet := c.Bool("quiet") + quiet := opts.quiet if !quiet { pterm.Println(pterm.Gray(fmt.Sprintf(" syncing %s ⇄ %s:%s", local, refLabel(ref, id), remote))) } - createArgs := []string{ - "sync", "create", - "--name=" + sessionName, - "--sync-mode=" + syncMode, - } - if !c.Bool("no-ignore-vcs") { - createArgs = append(createArgs, "--ignore-vcs") - } - for _, pat := range c.StringSlice("exclude") { - if p := strings.TrimSpace(pat); p != "" { - createArgs = append(createArgs, "--ignore="+p) + createArgs := mutagenCreateArgs(sessionName, syncMode, local, remoteSpec, !opts.noIgnoreVCS, opts.exclude) + // On --quiet, capture create output and surface it only if the + // command fails, so a real error isn't swallowed by the quiet flag. + var createOut io.Writer = os.Stderr + var createBuf *bytes.Buffer + if quiet { + createBuf = &bytes.Buffer{} + createOut = createBuf + } + if err := runMutagen(ctx, mutagenBin, wrapperEnv, createOut, createOut, createArgs...); err != nil { + detail := "" + if createBuf != nil { + if s := strings.TrimSpace(createBuf.String()); s != "" { + detail = "\n" + s + } } - } - // Source and target must come last as positional args. - createArgs = append(createArgs, local, remoteSpec) - if err := runMutagen(ctx, mutagenBin, wrapperEnv, createArgs...); err != nil { - return fmt.Errorf("mutagen sync create failed: %w", err) + return fmt.Errorf("mutagen sync create failed: %w%s", err, detail) } // Best-effort cleanup on exit. We can't always rely on context // cancellation propagating before we exit. defer func() { bg := context.Background() - _ = runMutagen(bg, mutagenBin, wrapperEnv, "sync", "terminate", sessionName) //nolint:errcheck + _ = runMutagen(bg, mutagenBin, wrapperEnv, io.Discard, io.Discard, "sync", "terminate", sessionName) //nolint:errcheck }() if !quiet { @@ -371,16 +378,156 @@ func syncModeToMutagen(mode string) (string, error) { } } -// runMutagen runs `mutagen ` with our shadowed PATH env. -// stdout/stderr are forwarded so the user sees mutagen's progress. -func runMutagen(ctx context.Context, bin string, env []string, args ...string) error { +// runMutagen runs `mutagen ` with our shadowed PATH env, sending +// stdout/stderr to the supplied writers. Callers decide the output +// policy (forward to the terminal, discard, or capture) rather than this +// helper hardwiring it — e.g. --quiet captures create output and only +// surfaces it on failure. +func runMutagen(ctx context.Context, bin string, env []string, stdout, stderr io.Writer, args ...string) error { cmd := exec.CommandContext(ctx, bin, args...) // #nosec G204 -- bin is our managed mutagen binary; args are internally constructed cmd.Env = env - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr + cmd.Stdout = stdout + cmd.Stderr = stderr return cmd.Run() } +// mutagenCreateArgs assembles the argument list for `mutagen sync +// create`. Kept pure (no cli.Context, no I/O) so the mode/ignore/exclude +// mapping — especially the destructive `mirror` → one-way-replica path — +// is unit-testable. Source and target must be the final two positionals. +func mutagenCreateArgs(sessionName, syncMode, local, remoteSpec string, ignoreVCS bool, exclude []string) []string { + args := []string{ + "sync", "create", + "--name=" + sessionName, + "--sync-mode=" + syncMode, + } + if ignoreVCS { + args = append(args, "--ignore-vcs") + } + for _, pat := range exclude { + if p := strings.TrimSpace(pat); p != "" { + args = append(args, "--ignore="+p) + } + } + return append(args, local, remoteSpec) +} + +// syncOptions holds every resolved input to `sandbox sync`. It exists so +// flags work whether they appear before or after the sandbox argument +// (urfave/cli v2 stops flag parsing at the first positional). +type syncOptions struct { + ref string + local string + remote string + identity string + user string + mode string + sshdWait time.Duration + exclude []string + force bool + assumeYes bool + quiet bool + noIgnoreVCS bool +} + +// parseSyncArgs merges the flags urfave already parsed (those placed +// before the sandbox argument) with a manual scan of the positional tail +// (flags placed after it). The first bare token is the sandbox ref; for +// scalars the last value wins, and --exclude accumulates. Mirrors the +// parseDiskCreateArgs / splitForceFlag workaround used elsewhere here. +func parseSyncArgs(c *cli.Context) syncOptions { + opts := syncOptions{ + local: strings.TrimSpace(c.String("local")), + remote: strings.TrimSpace(c.String("remote")), + identity: strings.TrimSpace(c.String("identity")), + user: strings.TrimSpace(c.String("user")), + mode: c.String("mode"), + sshdWait: c.Duration("sshd-wait"), + exclude: append([]string{}, c.StringSlice("exclude")...), + force: c.Bool("force"), + assumeYes: c.Bool("yes"), + quiet: c.Bool("quiet"), + noIgnoreVCS: c.Bool("no-ignore-vcs"), + } + + // Map short aliases to their canonical flag names. + canon := func(k string) string { + switch k { + case "i": + return "identity" + case "u": + return "user" + case "y": + return "yes" + case "q": + return "quiet" + } + return k + } + + args := c.Args().Slice() + for i := 0; i < len(args); i++ { + a := strings.TrimSpace(args[i]) + if a == "" { + continue + } + if !strings.HasPrefix(a, "-") { + if opts.ref == "" { + opts.ref = a + } + continue + } + raw := strings.TrimLeft(a, "-") + key, inline, hasInline := raw, "", false + if eq := strings.IndexByte(raw, '='); eq >= 0 { + key, inline, hasInline = raw[:eq], raw[eq+1:], true + } + key = canon(key) + + switch key { + case "force": + opts.force = true + case "yes": + opts.assumeYes = true + case "quiet": + opts.quiet = true + case "no-ignore-vcs": + opts.noIgnoreVCS = true + case "local", "remote", "identity", "user", "mode", "sshd-wait", "exclude": + val := inline + if !hasInline && i+1 < len(args) { + val = args[i+1] + i++ + } + val = strings.TrimSpace(val) + switch key { + case "local": + opts.local = val + case "remote": + opts.remote = val + case "identity": + opts.identity = val + case "user": + opts.user = val + case "mode": + opts.mode = val + case "sshd-wait": + if d, derr := time.ParseDuration(val); derr == nil { + opts.sshdWait = d + } + case "exclude": + if val != "" { + opts.exclude = append(opts.exclude, val) + } + } + default: + // Unknown flag after the positional. Ignore it rather than + // guess whether it consumes the following token as a value. + } + } + return opts +} + // makeSSHWrapper creates a tempdir with `ssh` AND `scp` shims that // forward to the real binaries while injecting `-i ` and the // right host-key flags. Returns (dir, env) where env's PATH has dir diff --git a/cmd/sandbox/sync_test.go b/cmd/sandbox/sync_test.go new file mode 100644 index 0000000..cefdd45 --- /dev/null +++ b/cmd/sandbox/sync_test.go @@ -0,0 +1,158 @@ +package sandbox + +import ( + "reflect" + "testing" + "time" + + "github.com/urfave/cli/v2" +) + +func TestSyncModeToMutagen(t *testing.T) { + ok := map[string]string{ + "": "two-way-safe", + "two-way": "two-way-safe", + "TWO-WAY": "two-way-safe", + " mirror ": "one-way-replica", + "one-way": "one-way-safe", + "mirror": "one-way-replica", + } + for in, want := range ok { + got, err := syncModeToMutagen(in) + if err != nil { + t.Errorf("syncModeToMutagen(%q) unexpected err: %v", in, err) + } + if got != want { + t.Errorf("syncModeToMutagen(%q) = %q, want %q", in, got, want) + } + } + if _, err := syncModeToMutagen("sideways"); err == nil { + t.Error(`syncModeToMutagen("sideways") expected error, got nil`) + } +} + +func TestMutagenCreateArgs(t *testing.T) { + // mirror mode + ignore-vcs + exclude (blank patterns dropped). + got := mutagenCreateArgs("sess1", "one-way-replica", "/local", "u@h:22:/r", true, []string{"*.log", " ", "node_modules"}) + want := []string{ + "sync", "create", + "--name=sess1", + "--sync-mode=one-way-replica", + "--ignore-vcs", + "--ignore=*.log", + "--ignore=node_modules", + "/local", "u@h:22:/r", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("mutagenCreateArgs mirror:\n got=%v\nwant=%v", got, want) + } + + // no ignore-vcs => flag omitted; source/target stay the final two args. + got = mutagenCreateArgs("s", "two-way-safe", "/l", "/r", false, nil) + want = []string{"sync", "create", "--name=s", "--sync-mode=two-way-safe", "/l", "/r"} + if !reflect.DeepEqual(got, want) { + t.Errorf("mutagenCreateArgs no-vcs:\n got=%v\nwant=%v", got, want) + } + if got[len(got)-2] != "/l" || got[len(got)-1] != "/r" { + t.Errorf("source/target must be last two args, got %v", got) + } +} + +// runParse builds a cli context with the real sync command's flags, runs +// it with argv, and returns the parsed syncOptions. +func runParse(t *testing.T, argv ...string) syncOptions { + t.Helper() + var got syncOptions + app := &cli.App{ + Commands: []*cli.Command{{ + Name: "sync", + Flags: newSyncCommand().Flags, + Action: func(c *cli.Context) error { + got = parseSyncArgs(c) + return nil + }, + }}, + } + if err := app.Run(append([]string{"app", "sync"}, argv...)); err != nil { + t.Fatalf("run %v: %v", argv, err) + } + return got +} + +// The blocker: urfave/cli v2 drops flags after the first positional. +// parseSyncArgs must recover every flag placed after the sandbox ref. +func TestParseSyncArgs_FlagsAfterPositional(t *testing.T) { + o := runParse(t, "my-box", + "--mode", "mirror", + "--exclude", "*.log", "--exclude", "node_modules", + "--quiet", "--no-ignore-vcs", + "--remote", "/root/work") + + if o.ref != "my-box" { + t.Errorf("ref = %q, want my-box", o.ref) + } + if o.mode != "mirror" { + t.Errorf("mode = %q, want mirror", o.mode) + } + if !o.quiet { + t.Error("quiet = false, want true") + } + if !o.noIgnoreVCS { + t.Error("noIgnoreVCS = false, want true") + } + if o.remote != "/root/work" { + t.Errorf("remote = %q, want /root/work", o.remote) + } + if !reflect.DeepEqual(o.exclude, []string{"*.log", "node_modules"}) { + t.Errorf("exclude = %v, want [*.log node_modules]", o.exclude) + } +} + +// Short aliases and --flag=value form, also after the positional. +func TestParseSyncArgs_ShortAndInlineAfterPositional(t *testing.T) { + o := runParse(t, "my-box", "-i", "/k/id", "--mode=one-way", "-q", "-y", "-u", "app") + if o.ref != "my-box" { + t.Errorf("ref = %q, want my-box", o.ref) + } + if o.identity != "/k/id" { + t.Errorf("identity = %q, want /k/id", o.identity) + } + if o.user != "app" { + t.Errorf("user = %q, want app", o.user) + } + if o.mode != "one-way" { + t.Errorf("mode = %q, want one-way", o.mode) + } + if !o.quiet || !o.assumeYes { + t.Errorf("quiet=%v assumeYes=%v, want both true", o.quiet, o.assumeYes) + } +} + +// Flags before the positional (the only form urfave handles natively) +// must keep working. +func TestParseSyncArgs_FlagsBeforePositional(t *testing.T) { + o := runParse(t, "--mode", "mirror", "--exclude", "*.tmp", "my-box") + if o.ref != "my-box" { + t.Errorf("ref = %q, want my-box", o.ref) + } + if o.mode != "mirror" { + t.Errorf("mode = %q, want mirror", o.mode) + } + if !reflect.DeepEqual(o.exclude, []string{"*.tmp"}) { + t.Errorf("exclude = %v, want [*.tmp]", o.exclude) + } +} + +// Defaults survive when no flags are given. +func TestParseSyncArgs_Defaults(t *testing.T) { + o := runParse(t, "my-box") + if o.mode != "two-way" { + t.Errorf("mode = %q, want two-way (flag default)", o.mode) + } + if o.sshdWait != 5*time.Second { + t.Errorf("sshdWait = %v, want 5s (flag default)", o.sshdWait) + } + if o.quiet || o.force || o.assumeYes || o.noIgnoreVCS { + t.Errorf("bool flags should default false, got %+v", o) + } +}