From a2c6f4ddf0fac1ff0893bb6053581a158f24617a Mon Sep 17 00:00:00 2001 From: immanuel-peter Date: Thu, 26 Feb 2026 10:40:23 -0600 Subject: [PATCH 1/8] feat(copy): use rsync by default with automatic scp fallback --- pkg/cmd/copy/copy.go | 102 +++++++++++++++++++++++++++++--------- pkg/cmd/copy/copy_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 24 deletions(-) create mode 100644 pkg/cmd/copy/copy_test.go diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index e0abb17eb..a3b4a0896 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -23,7 +23,7 @@ import ( ) var ( - copyLong = "Copy files and directories between your local machine and remote instance" + copyLong = "Copy files and directories between your local machine and remote instance (uses rsync by default and falls back to scp)" copyExample = "brev copy instance_name:/path/to/remote/file /path/to/local/file\nbrev copy /path/to/local/file instance_name:/path/to/remote/file\nbrev copy ./local-directory/ instance_name:/remote/path/" ) @@ -87,7 +87,7 @@ func runCopyCommand(t *terminal.Terminal, cstore CopyStore, source, dest string, _ = writeconnectionevent.WriteWCEOnEnv(cstore, workspace.DNS) - err = runSCP(t, sshName, localPath, remotePath, isUpload) + err = runCopyWithFallback(t, sshName, localPath, remotePath, isUpload) if err != nil { return breverrors.WrapAndTrace(err) } @@ -202,33 +202,23 @@ func parseWorkspacePath(path string) (workspace, filePath string, err error) { return parts[0], parts[1], nil } -func runSCP(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUpload bool) error { - var scpCmd *exec.Cmd - var source, dest string +type commandRunner func(name string, args ...string) ([]byte, error) - startTime := time.Now() +func combinedOutputRunner(name string, args ...string) ([]byte, error) { + cmd := exec.Command(name, args...) //nolint:gosec + return cmd.CombinedOutput() +} - scpArgs := []string{"scp"} +func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUpload bool) error { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) - if isUpload { - if isDirectory(localPath) { - scpArgs = append(scpArgs, "-r") - } - scpArgs = append(scpArgs, localPath, fmt.Sprintf("%s:%s", sshAlias, remotePath)) - source = localPath - dest = fmt.Sprintf("%s:%s", sshAlias, remotePath) - } else { - scpArgs = append(scpArgs, "-r") - scpArgs = append(scpArgs, fmt.Sprintf("%s:%s", sshAlias, remotePath), localPath) - source = fmt.Sprintf("%s:%s", sshAlias, remotePath) - dest = localPath + startTime := time.Now() + fellBack, err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner) + if fellBack { + t.Vprint(t.Yellow("rsync failed, falling back to scp...\n")) } - - scpCmd = exec.Command(scpArgs[0], scpArgs[1:]...) //nolint:gosec //sshAlias is validated workspace identifier - - output, err := scpCmd.CombinedOutput() if err != nil { - return breverrors.WrapAndTrace(fmt.Errorf("scp failed: %s\nOutput: %s", err.Error(), string(output))) + return breverrors.WrapAndTrace(err) } duration := time.Since(startTime) @@ -238,6 +228,70 @@ func runSCP(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUplo return nil } +func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) (bool, error) { + err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, runner) + if err == nil { + return false, nil + } + + scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) + if scpErr != nil { + return true, fmt.Errorf("rsync failed: %v\nscp fallback failed: %w", err, scpErr) + } + + return true, nil +} + +func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) error { + rsyncArgs := buildRsyncArgs(sshAlias, localPath, remotePath, isUpload) + output, err := runner("rsync", rsyncArgs...) + if err != nil { + return fmt.Errorf("rsync failed: %s\nOutput: %s", err.Error(), string(output)) + } + return nil +} + +func runSCPCommand(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) error { + scpArgs := buildSCPArgs(sshAlias, localPath, remotePath, isUpload) + output, err := runner("scp", scpArgs...) + if err != nil { + return fmt.Errorf("scp failed: %s\nOutput: %s", err.Error(), string(output)) + } + return nil +} + +func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload bool) []string { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) + + rsyncArgs := []string{"-z", "-e", "ssh"} + if !isUpload || isDirectory(localPath) { + rsyncArgs = append(rsyncArgs, "-r") + } + rsyncArgs = append(rsyncArgs, source, dest) + + return rsyncArgs +} + +func buildSCPArgs(sshAlias, localPath, remotePath string, isUpload bool) []string { + source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) + + scpArgs := []string{} + if !isUpload || isDirectory(localPath) { + scpArgs = append(scpArgs, "-r") + } + scpArgs = append(scpArgs, source, dest) + + return scpArgs +} + +func transferEndpoints(sshAlias, localPath, remotePath string, isUpload bool) (source, dest string) { + remoteTarget := fmt.Sprintf("%s:%s", sshAlias, remotePath) + if isUpload { + return localPath, remoteTarget + } + return remoteTarget, localPath +} + func waitForSSHToBeAvailable(sshAlias string, s *spinner.Spinner) error { counter := 0 s.Suffix = " waiting for SSH connection to be available" diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go new file mode 100644 index 000000000..23ca80c34 --- /dev/null +++ b/pkg/cmd/copy/copy_test.go @@ -0,0 +1,102 @@ +package copy + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBuildRsyncArgs(t *testing.T) { + t.Run("upload file", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", true) + assert.Equal(t, []string{"-z", "-e", "ssh", "/tmp/local.txt", "ws:/remote/path"}, args) + }) + + t.Run("upload directory", func(t *testing.T) { + tmpDir := t.TempDir() + localDir := filepath.Join(tmpDir, "mydir") + err := os.MkdirAll(localDir, 0o755) + assert.NoError(t, err) + + args := buildRsyncArgs("ws", localDir, "/remote/path", true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir, "ws:/remote/path"}, args) + }) + + t.Run("download path", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", false) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "ws:/remote/path", "/tmp/local.txt"}, args) + }) +} + +func TestBuildSCPArgs(t *testing.T) { + t.Run("upload file", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", true) + assert.Equal(t, []string{"/tmp/local.txt", "ws:/remote/path"}, args) + }) + + t.Run("upload directory", func(t *testing.T) { + tmpDir := t.TempDir() + localDir := filepath.Join(tmpDir, "mydir") + err := os.MkdirAll(localDir, 0o755) + assert.NoError(t, err) + + args := buildSCPArgs("ws", localDir, "/remote/path", true) + assert.Equal(t, []string{"-r", localDir, "ws:/remote/path"}, args) + }) + + t.Run("download path", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", false) + assert.Equal(t, []string{"-r", "ws:/remote/path", "/tmp/local.txt"}, args) + }) +} + +func TestTransferWithFallback(t *testing.T) { + t.Run("rsync success", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + return []byte("ok"), nil + } + + fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + assert.NoError(t, err) + assert.False(t, fellBack) + assert.Equal(t, []string{"rsync"}, calls) + }) + + t.Run("rsync fails and scp succeeds", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + if name == "rsync" { + return []byte("rsync failed"), errors.New("exit status 1") + } + return []byte("scp ok"), nil + } + + fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + assert.NoError(t, err) + assert.True(t, fellBack) + assert.Equal(t, []string{"rsync", "scp"}, calls) + }) + + t.Run("rsync fails and scp fails", func(t *testing.T) { + runner := func(name string, args ...string) ([]byte, error) { + if name == "rsync" { + return []byte("rsync output"), errors.New("exit status 1") + } + return []byte("scp output"), errors.New("exit status 1") + } + + fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + assert.Error(t, err) + assert.True(t, fellBack) + assert.Contains(t, err.Error(), "rsync failed") + assert.Contains(t, err.Error(), "scp fallback failed") + assert.Contains(t, err.Error(), "rsync output") + assert.Contains(t, err.Error(), "scp output") + }) +} From 0ce7e15da97d14e105c23cf0ffbccf8a279d3095 Mon Sep 17 00:00:00 2001 From: immanuel-peter Date: Thu, 26 Feb 2026 10:50:51 -0600 Subject: [PATCH 2/8] fix: fix linting issue in copy --- pkg/cmd/copy/copy.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index a3b4a0896..054ee3b25 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -206,7 +206,11 @@ type commandRunner func(name string, args ...string) ([]byte, error) func combinedOutputRunner(name string, args ...string) ([]byte, error) { cmd := exec.Command(name, args...) //nolint:gosec - return cmd.CombinedOutput() + output, err := cmd.CombinedOutput() + if err != nil { + return output, fmt.Errorf("run %s command: %w", name, err) + } + return output, nil } func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath string, isUpload bool) error { From 93f46285724782ea7100505904febbe7105a665b Mon Sep 17 00:00:00 2001 From: immanuel-peter Date: Thu, 26 Feb 2026 11:37:48 -0600 Subject: [PATCH 3/8] fix(copy): avoid duplicated rsync error prefix and add fallback callback hook --- pkg/cmd/copy/copy.go | 19 +++++++++++-------- pkg/cmd/copy/copy_test.go | 20 ++++++++++++-------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index 054ee3b25..6fb92f2a9 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -205,7 +205,7 @@ func parseWorkspacePath(path string) (workspace, filePath string, err error) { type commandRunner func(name string, args ...string) ([]byte, error) func combinedOutputRunner(name string, args ...string) ([]byte, error) { - cmd := exec.Command(name, args...) //nolint:gosec + cmd := exec.Command(name, args...) //nolint:gosec // Command and args come from internal call sites using fixed binaries/flags (rsync/scp). output, err := cmd.CombinedOutput() if err != nil { return output, fmt.Errorf("run %s command: %w", name, err) @@ -217,10 +217,9 @@ func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath s source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) startTime := time.Now() - fellBack, err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner) - if fellBack { + err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner, func() { t.Vprint(t.Yellow("rsync failed, falling back to scp...\n")) - } + }) if err != nil { return breverrors.WrapAndTrace(err) } @@ -232,18 +231,22 @@ func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath s return nil } -func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) (bool, error) { +func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner, onFallback func()) error { err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, runner) if err == nil { - return false, nil + return nil + } + + if onFallback != nil { + onFallback() } scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) if scpErr != nil { - return true, fmt.Errorf("rsync failed: %v\nscp fallback failed: %w", err, scpErr) + return fmt.Errorf("%v\nscp fallback failed: %w", err, scpErr) } - return true, nil + return nil } func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) error { diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go index 23ca80c34..548323ace 100644 --- a/pkg/cmd/copy/copy_test.go +++ b/pkg/cmd/copy/copy_test.go @@ -61,9 +61,12 @@ func TestTransferWithFallback(t *testing.T) { return []byte("ok"), nil } - fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + onFallbackCalled := false + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() { + onFallbackCalled = true + }) assert.NoError(t, err) - assert.False(t, fellBack) + assert.False(t, onFallbackCalled) assert.Equal(t, []string{"rsync"}, calls) }) @@ -77,10 +80,11 @@ func TestTransferWithFallback(t *testing.T) { return []byte("scp ok"), nil } - fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() { + calls = append(calls, "fallback") + }) assert.NoError(t, err) - assert.True(t, fellBack) - assert.Equal(t, []string{"rsync", "scp"}, calls) + assert.Equal(t, []string{"rsync", "fallback", "scp"}, calls) }) t.Run("rsync fails and scp fails", func(t *testing.T) { @@ -91,12 +95,12 @@ func TestTransferWithFallback(t *testing.T) { return []byte("scp output"), errors.New("exit status 1") } - fellBack, err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner) + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() {}) assert.Error(t, err) - assert.True(t, fellBack) - assert.Contains(t, err.Error(), "rsync failed") + assert.Contains(t, err.Error(), "rsync failed: exit status 1") assert.Contains(t, err.Error(), "scp fallback failed") assert.Contains(t, err.Error(), "rsync output") assert.Contains(t, err.Error(), "scp output") + assert.NotContains(t, err.Error(), "rsync failed: rsync failed:") }) } From 691d703eec01568b0713368118144ef546fbd806 Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Mon, 13 Jul 2026 17:52:04 -0700 Subject: [PATCH 4/8] feat(copy): detect missing rsync and hint how to get faster transfers Skip the rsync attempt entirely when rsync is not installed locally, and when the instance itself lacks rsync, say so in the fallback message. Both scp paths now tell the user to install rsync for faster transfers. --- pkg/cmd/copy/copy.go | 28 +++++++++++++++++++----- pkg/cmd/copy/copy_test.go | 45 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index 77d60c10e..326fcc69a 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -224,8 +224,8 @@ func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath s source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) startTime := time.Now() - err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner, func() { - t.Vprint(t.Yellow("rsync failed, falling back to scp...\n")) + err := transferWithFallback(sshAlias, localPath, remotePath, isUpload, combinedOutputRunner, rsyncInstalledLocally, func(reason string) { + t.Vprint(t.Yellow("%s\n", reason)) }) if err != nil { return breverrors.WrapAndTrace(err) @@ -238,14 +238,32 @@ func runCopyWithFallback(t *terminal.Terminal, sshAlias, localPath, remotePath s return nil } -func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner, onFallback func()) error { +func rsyncInstalledLocally() bool { + _, err := exec.LookPath("rsync") + return err == nil +} + +func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner, rsyncAvailable func() bool, onFallback func(reason string)) error { + notifyFallback := func(reason string) { + if onFallback != nil { + onFallback(reason) + } + } + + if !rsyncAvailable() { + notifyFallback("rsync not found on this machine, using scp. Install rsync for faster transfers.") + return runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) + } + err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, runner) if err == nil { return nil } - if onFallback != nil { - onFallback() + if strings.Contains(err.Error(), "command not found") { + notifyFallback("rsync is not installed on the instance, falling back to scp. Install rsync on the instance for faster transfers.") + } else { + notifyFallback("rsync failed, falling back to scp...") } scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go index ca2705da6..10692024f 100644 --- a/pkg/cmd/copy/copy_test.go +++ b/pkg/cmd/copy/copy_test.go @@ -54,6 +54,8 @@ func TestBuildSCPArgs(t *testing.T) { } func TestTransferWithFallback(t *testing.T) { + rsyncAvailable := func() bool { return true } + t.Run("rsync success", func(t *testing.T) { calls := []string{} runner := func(name string, args ...string) ([]byte, error) { @@ -62,7 +64,7 @@ func TestTransferWithFallback(t *testing.T) { } onFallbackCalled := false - err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() { + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { onFallbackCalled = true }) assert.NoError(t, err) @@ -70,6 +72,43 @@ func TestTransferWithFallback(t *testing.T) { assert.Equal(t, []string{"rsync"}, calls) }) + t.Run("rsync not installed locally skips straight to scp", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + return []byte("ok"), nil + } + + reasons := []string{} + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() bool { return false }, func(reason string) { + reasons = append(reasons, reason) + }) + assert.NoError(t, err) + assert.Equal(t, []string{"scp"}, calls) + assert.Len(t, reasons, 1) + assert.Contains(t, reasons[0], "Install rsync for faster transfers") + }) + + t.Run("rsync missing on instance falls back with install hint", func(t *testing.T) { + calls := []string{} + runner := func(name string, args ...string) ([]byte, error) { + calls = append(calls, name) + if name == "rsync" { + return []byte("bash: rsync: command not found"), errors.New("exit status 127") + } + return []byte("scp ok"), nil + } + + reasons := []string{} + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { + reasons = append(reasons, reason) + }) + assert.NoError(t, err) + assert.Equal(t, []string{"rsync", "scp"}, calls) + assert.Len(t, reasons, 1) + assert.Contains(t, reasons[0], "Install rsync on the instance for faster transfers") + }) + t.Run("rsync fails and scp succeeds", func(t *testing.T) { calls := []string{} runner := func(name string, args ...string) ([]byte, error) { @@ -80,7 +119,7 @@ func TestTransferWithFallback(t *testing.T) { return []byte("scp ok"), nil } - err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() { + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) { calls = append(calls, "fallback") }) assert.NoError(t, err) @@ -95,7 +134,7 @@ func TestTransferWithFallback(t *testing.T) { return []byte("scp output"), errors.New("exit status 1") } - err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, func() {}) + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/path", true, runner, rsyncAvailable, func(reason string) {}) assert.Error(t, err) assert.Contains(t, err.Error(), "rsync failed: exit status 1") assert.Contains(t, err.Error(), "scp fallback failed") From 3704b328059b83d316f85cf006152162aa9fd0fe Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Tue, 14 Jul 2026 09:32:21 -0700 Subject: [PATCH 5/8] chore(copy): drop rsync/scp implementation detail from help text --- pkg/cmd/copy/copy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index 326fcc69a..ee3d5b289 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -24,7 +24,7 @@ import ( ) var ( - copyLong = "Copy files and directories between your local machine and remote instance (uses rsync by default and falls back to scp)" + copyLong = "Copy files and directories between your local machine and remote instance" copyExample = "brev copy instance_name:/path/to/remote/file /path/to/local/file\nbrev copy /path/to/local/file instance_name:/path/to/remote/file\nbrev copy ./local-directory/ instance_name:/remote/path/" ) From d4fc34634660b5dc45236a1ba058a626d3b3de7b Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Tue, 14 Jul 2026 22:21:25 -0700 Subject: [PATCH 6/8] fix(copy): keep scp directory-upload semantics under rsync Without a trailing slash rsync nests the source directory inside the destination (dest/dir/...), while scp makes the destination the copy when it does not exist. Normalize directory-upload sources to trailing-slash form so the destination always mirrors the source. --- pkg/cmd/copy/copy.go | 6 ++++++ pkg/cmd/copy/copy_test.go | 12 +++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index ee3d5b289..6a2abb11d 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -299,6 +299,12 @@ func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload bool) []str if !isUpload || isDirectory(localPath) { rsyncArgs = append(rsyncArgs, "-r") } + // A trailing slash makes rsync copy the directory's contents so the + // destination becomes the copy, matching scp when the destination does + // not exist; without it rsync nests the source inside the destination. + if isUpload && isDirectory(localPath) && !strings.HasSuffix(source, "/") { + source += "/" + } rsyncArgs = append(rsyncArgs, source, dest) return rsyncArgs diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go index 10692024f..db8bef826 100644 --- a/pkg/cmd/copy/copy_test.go +++ b/pkg/cmd/copy/copy_test.go @@ -22,7 +22,17 @@ func TestBuildRsyncArgs(t *testing.T) { assert.NoError(t, err) args := buildRsyncArgs("ws", localDir, "/remote/path", true) - assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir, "ws:/remote/path"}, args) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir + "/", "ws:/remote/path"}, args) + }) + + t.Run("upload directory with trailing slash is not doubled", func(t *testing.T) { + tmpDir := t.TempDir() + localDir := filepath.Join(tmpDir, "mydir") + err := os.MkdirAll(localDir, 0o755) + assert.NoError(t, err) + + args := buildRsyncArgs("ws", localDir+"/", "/remote/path", true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir + "/", "ws:/remote/path"}, args) }) t.Run("download path", func(t *testing.T) { From 6058a98179fe0170fe545e60e8392955dd4356e0 Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Tue, 14 Jul 2026 22:23:29 -0700 Subject: [PATCH 7/8] docs(copy): note rsync/scp divergence when destination dir exists --- pkg/cmd/copy/copy.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index 6a2abb11d..41c946872 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -302,6 +302,9 @@ func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload bool) []str // A trailing slash makes rsync copy the directory's contents so the // destination becomes the copy, matching scp when the destination does // not exist; without it rsync nests the source inside the destination. + // Known divergence when the destination directory already exists: rsync + // merges the contents into it, while the scp fallback nests the source + // inside it (dest/dir/...). if isUpload && isDirectory(localPath) && !strings.HasSuffix(source, "/") { source += "/" } From 71d776010a1e7060bc290a9cedbb6050a2c2fd20 Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Tue, 14 Jul 2026 23:56:21 -0700 Subject: [PATCH 8/8] feat(copy): unify rsync/scp directory semantics so dest mirrors source Directory sources are normalized to contents-copy form (trailing slash for rsync, /. for scp) so the destination always becomes a mirror of the source, whether or not it already exists, and both transfer tools produce identical layouts. Downloads learn the remote path type via a single 'ssh test -d' probe; uploads stat the local source directly. This also makes repeated directory copies idempotent instead of nesting dir inside dir on the second run. --- pkg/cmd/copy/copy.go | 56 +++++--- pkg/cmd/copy/copy_test.go | 290 +++++++++++++++++++++++--------------- 2 files changed, 213 insertions(+), 133 deletions(-) diff --git a/pkg/cmd/copy/copy.go b/pkg/cmd/copy/copy.go index 41c946872..6a8c0e086 100644 --- a/pkg/cmd/copy/copy.go +++ b/pkg/cmd/copy/copy.go @@ -250,12 +250,21 @@ func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, } } + // Directory sources are normalized to contents-copy form so the + // destination always mirrors the source, whether or not it already + // exists, and rsync and scp produce identical layouts. Uploads can + // stat the local source; downloads probe the remote path type. + sourceIsDir := isUpload && isDirectory(localPath) + if !isUpload { + sourceIsDir = remotePathIsDir(sshAlias, remotePath, runner) + } + if !rsyncAvailable() { notifyFallback("rsync not found on this machine, using scp. Install rsync for faster transfers.") - return runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) + return runSCPCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) } - err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, runner) + err := runRsyncCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) if err == nil { return nil } @@ -266,7 +275,7 @@ func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, notifyFallback("rsync failed, falling back to scp...") } - scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, runner) + scpErr := runSCPCommand(sshAlias, localPath, remotePath, isUpload, sourceIsDir, runner) if scpErr != nil { return fmt.Errorf("%v\nscp fallback failed: %w", err, scpErr) } @@ -274,8 +283,14 @@ func transferWithFallback(sshAlias, localPath, remotePath string, isUpload bool, return nil } -func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) error { - rsyncArgs := buildRsyncArgs(sshAlias, localPath, remotePath, isUpload) +func remotePathIsDir(sshAlias, remotePath string, runner commandRunner) bool { + quoted := "'" + strings.ReplaceAll(remotePath, "'", `'\''`) + "'" + _, err := runner("ssh", sshAlias, "test", "-d", quoted) + return err == nil +} + +func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool, runner commandRunner) error { + rsyncArgs := buildRsyncArgs(sshAlias, localPath, remotePath, isUpload, sourceIsDir) output, err := runner("rsync", rsyncArgs...) if err != nil { return fmt.Errorf("rsync failed: %s\nOutput: %s", err.Error(), string(output)) @@ -283,8 +298,8 @@ func runRsyncCommand(sshAlias, localPath, remotePath string, isUpload bool, runn return nil } -func runSCPCommand(sshAlias, localPath, remotePath string, isUpload bool, runner commandRunner) error { - scpArgs := buildSCPArgs(sshAlias, localPath, remotePath, isUpload) +func runSCPCommand(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool, runner commandRunner) error { + scpArgs := buildSCPArgs(sshAlias, localPath, remotePath, isUpload, sourceIsDir) output, err := runner("scp", scpArgs...) if err != nil { return fmt.Errorf("scp failed: %s\nOutput: %s", err.Error(), string(output)) @@ -292,20 +307,17 @@ func runSCPCommand(sshAlias, localPath, remotePath string, isUpload bool, runner return nil } -func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload bool) []string { +func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool) []string { source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) rsyncArgs := []string{"-z", "-e", "ssh"} - if !isUpload || isDirectory(localPath) { + if !isUpload || sourceIsDir { rsyncArgs = append(rsyncArgs, "-r") } - // A trailing slash makes rsync copy the directory's contents so the - // destination becomes the copy, matching scp when the destination does - // not exist; without it rsync nests the source inside the destination. - // Known divergence when the destination directory already exists: rsync - // merges the contents into it, while the scp fallback nests the source - // inside it (dest/dir/...). - if isUpload && isDirectory(localPath) && !strings.HasSuffix(source, "/") { + // A trailing slash makes rsync copy the directory's contents, so the + // destination becomes the copy instead of having the source directory + // nested inside it. scp gets the same treatment via "/." below. + if sourceIsDir && !strings.HasSuffix(source, "/") { source += "/" } rsyncArgs = append(rsyncArgs, source, dest) @@ -313,13 +325,21 @@ func buildRsyncArgs(sshAlias, localPath, remotePath string, isUpload bool) []str return rsyncArgs } -func buildSCPArgs(sshAlias, localPath, remotePath string, isUpload bool) []string { +func buildSCPArgs(sshAlias, localPath, remotePath string, isUpload, sourceIsDir bool) []string { source, dest := transferEndpoints(sshAlias, localPath, remotePath, isUpload) scpArgs := []string{} - if !isUpload || isDirectory(localPath) { + if !isUpload || sourceIsDir { scpArgs = append(scpArgs, "-r") } + // "dir/." makes scp copy the directory's contents, mirroring rsync's + // trailing-slash behavior regardless of whether the destination exists. + if sourceIsDir { + if !strings.HasSuffix(source, "/") { + source += "/" + } + source += "." + } scpArgs = append(scpArgs, source, dest) return scpArgs diff --git a/pkg/cmd/copy/copy_test.go b/pkg/cmd/copy/copy_test.go index db8bef826..3767f5e93 100644 --- a/pkg/cmd/copy/copy_test.go +++ b/pkg/cmd/copy/copy_test.go @@ -2,8 +2,6 @@ package copy import ( "errors" - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -11,56 +9,203 @@ import ( func TestBuildRsyncArgs(t *testing.T) { t.Run("upload file", func(t *testing.T) { - args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", true) + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", true, false) assert.Equal(t, []string{"-z", "-e", "ssh", "/tmp/local.txt", "ws:/remote/path"}, args) }) - t.Run("upload directory", func(t *testing.T) { - tmpDir := t.TempDir() - localDir := filepath.Join(tmpDir, "mydir") - err := os.MkdirAll(localDir, 0o755) - assert.NoError(t, err) - - args := buildRsyncArgs("ws", localDir, "/remote/path", true) - assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir + "/", "ws:/remote/path"}, args) + t.Run("upload directory copies contents", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/mydir", "/remote/path", true, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "/tmp/mydir/", "ws:/remote/path"}, args) }) t.Run("upload directory with trailing slash is not doubled", func(t *testing.T) { - tmpDir := t.TempDir() - localDir := filepath.Join(tmpDir, "mydir") - err := os.MkdirAll(localDir, 0o755) - assert.NoError(t, err) - - args := buildRsyncArgs("ws", localDir+"/", "/remote/path", true) - assert.Equal(t, []string{"-z", "-e", "ssh", "-r", localDir + "/", "ws:/remote/path"}, args) + args := buildRsyncArgs("ws", "/tmp/mydir/", "/remote/path", true, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "/tmp/mydir/", "ws:/remote/path"}, args) }) - t.Run("download path", func(t *testing.T) { - args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", false) + t.Run("download file", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local.txt", "/remote/path", false, false) assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "ws:/remote/path", "/tmp/local.txt"}, args) }) + + t.Run("download directory copies contents", func(t *testing.T) { + args := buildRsyncArgs("ws", "/tmp/local", "/remote/path", false, true) + assert.Equal(t, []string{"-z", "-e", "ssh", "-r", "ws:/remote/path/", "/tmp/local"}, args) + }) } func TestBuildSCPArgs(t *testing.T) { t.Run("upload file", func(t *testing.T) { - args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", true) + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", true, false) assert.Equal(t, []string{"/tmp/local.txt", "ws:/remote/path"}, args) }) - t.Run("upload directory", func(t *testing.T) { - tmpDir := t.TempDir() - localDir := filepath.Join(tmpDir, "mydir") - err := os.MkdirAll(localDir, 0o755) - assert.NoError(t, err) + t.Run("upload directory copies contents", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/mydir", "/remote/path", true, true) + assert.Equal(t, []string{"-r", "/tmp/mydir/.", "ws:/remote/path"}, args) + }) - args := buildSCPArgs("ws", localDir, "/remote/path", true) - assert.Equal(t, []string{"-r", localDir, "ws:/remote/path"}, args) + t.Run("upload directory with trailing slash is not doubled", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/mydir/", "/remote/path", true, true) + assert.Equal(t, []string{"-r", "/tmp/mydir/.", "ws:/remote/path"}, args) }) - t.Run("download path", func(t *testing.T) { - args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", false) + t.Run("download file", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local.txt", "/remote/path", false, false) assert.Equal(t, []string{"-r", "ws:/remote/path", "/tmp/local.txt"}, args) }) + + t.Run("download directory copies contents", func(t *testing.T) { + args := buildSCPArgs("ws", "/tmp/local", "/remote/path", false, true) + assert.Equal(t, []string{"-r", "ws:/remote/path/.", "/tmp/local"}, args) + }) +} + +func TestParseCopyArguments_Upload(t *testing.T) { + ws, remotePath, localPath, isUpload, err := parseCopyArguments("./local.txt", "my-node:/tmp/dest") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ws != "my-node" { + t.Errorf("expected workspace my-node, got %s", ws) + } + if remotePath != "/tmp/dest" { + t.Errorf("expected remotePath /tmp/dest, got %s", remotePath) + } + if localPath != "./local.txt" { + t.Errorf("expected localPath ./local.txt, got %s", localPath) + } + if !isUpload { + t.Error("expected isUpload=true") + } +} + +func TestParseCopyArguments_Download(t *testing.T) { + ws, remotePath, localPath, isUpload, err := parseCopyArguments("my-node:/tmp/file", "./local.txt") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ws != "my-node" { + t.Errorf("expected workspace my-node, got %s", ws) + } + if remotePath != "/tmp/file" { + t.Errorf("expected remotePath /tmp/file, got %s", remotePath) + } + if localPath != "./local.txt" { + t.Errorf("expected localPath ./local.txt, got %s", localPath) + } + if isUpload { + t.Error("expected isUpload=false") + } +} + +func TestParseCopyArguments_BothLocal(t *testing.T) { + _, _, _, _, err := parseCopyArguments("./a", "./b") + if err == nil { + t.Fatal("expected error when both paths are local") + } +} + +func TestParseCopyArguments_BothRemote(t *testing.T) { + _, _, _, _, err := parseCopyArguments("ws1:/a", "ws2:/b") + if err == nil { + t.Fatal("expected error when both paths are remote") + } +} + +func TestParseWorkspacePath_Local(t *testing.T) { + ws, fp, err := parseWorkspacePath("/tmp/local/file") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ws != "" { + t.Errorf("expected empty workspace, got %s", ws) + } + if fp != "/tmp/local/file" { + t.Errorf("expected /tmp/local/file, got %s", fp) + } +} + +func TestParseWorkspacePath_Remote(t *testing.T) { + ws, fp, err := parseWorkspacePath("my-instance:/remote/path") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ws != "my-instance" { + t.Errorf("expected my-instance, got %s", ws) + } + if fp != "/remote/path" { + t.Errorf("expected /remote/path, got %s", fp) + } +} + +func TestParseWorkspacePath_InvalidMultipleColons(t *testing.T) { + _, _, err := parseWorkspacePath("ws:path:extra") + if err == nil { + t.Fatal("expected error for multiple colons") + } +} + +func TestRemotePathIsDir(t *testing.T) { + t.Run("directory", func(t *testing.T) { + var gotArgs []string + runner := func(name string, args ...string) ([]byte, error) { + gotArgs = append([]string{name}, args...) + return nil, nil + } + assert.True(t, remotePathIsDir("ws", "/remote/path", runner)) + assert.Equal(t, []string{"ssh", "ws", "test", "-d", "'/remote/path'"}, gotArgs) + }) + + t.Run("not a directory", func(t *testing.T) { + runner := func(name string, args ...string) ([]byte, error) { + return nil, errors.New("exit status 1") + } + assert.False(t, remotePathIsDir("ws", "/remote/file.txt", runner)) + }) + + t.Run("quotes are escaped", func(t *testing.T) { + var gotArgs []string + runner := func(name string, args ...string) ([]byte, error) { + gotArgs = append([]string{name}, args...) + return nil, nil + } + remotePathIsDir("ws", "/it's/a/path", runner) + assert.Equal(t, `'/it'\''s/a/path'`, gotArgs[4]) + }) +} + +func TestTransferWithFallbackDownloadNormalization(t *testing.T) { + rsyncAvailable := func() bool { return true } + + t.Run("download probes remote path type and copies directory contents", func(t *testing.T) { + commands := [][]string{} + runner := func(name string, args ...string) ([]byte, error) { + commands = append(commands, append([]string{name}, args...)) + return nil, nil + } + + err := transferWithFallback("ws", "/tmp/local", "/remote/path", false, runner, rsyncAvailable, nil) + assert.NoError(t, err) + assert.Len(t, commands, 2) + assert.Equal(t, "ssh", commands[0][0]) + assert.Equal(t, []string{"rsync", "-z", "-e", "ssh", "-r", "ws:/remote/path/", "/tmp/local"}, commands[1]) + }) + + t.Run("download of a file is not normalized", func(t *testing.T) { + commands := [][]string{} + runner := func(name string, args ...string) ([]byte, error) { + if name == "ssh" { + return nil, errors.New("exit status 1") + } + commands = append(commands, append([]string{name}, args...)) + return nil, nil + } + + err := transferWithFallback("ws", "/tmp/local.txt", "/remote/file.txt", false, runner, rsyncAvailable, nil) + assert.NoError(t, err) + assert.Equal(t, [][]string{{"rsync", "-z", "-e", "ssh", "-r", "ws:/remote/file.txt", "/tmp/local.txt"}}, commands) + }) } func TestTransferWithFallback(t *testing.T) { @@ -153,88 +298,3 @@ func TestTransferWithFallback(t *testing.T) { assert.NotContains(t, err.Error(), "rsync failed: rsync failed:") }) } - -func TestParseCopyArguments_Upload(t *testing.T) { - ws, remotePath, localPath, isUpload, err := parseCopyArguments("./local.txt", "my-node:/tmp/dest") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if ws != "my-node" { - t.Errorf("expected workspace my-node, got %s", ws) - } - if remotePath != "/tmp/dest" { - t.Errorf("expected remotePath /tmp/dest, got %s", remotePath) - } - if localPath != "./local.txt" { - t.Errorf("expected localPath ./local.txt, got %s", localPath) - } - if !isUpload { - t.Error("expected isUpload=true") - } -} - -func TestParseCopyArguments_Download(t *testing.T) { - ws, remotePath, localPath, isUpload, err := parseCopyArguments("my-node:/tmp/file", "./local.txt") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if ws != "my-node" { - t.Errorf("expected workspace my-node, got %s", ws) - } - if remotePath != "/tmp/file" { - t.Errorf("expected remotePath /tmp/file, got %s", remotePath) - } - if localPath != "./local.txt" { - t.Errorf("expected localPath ./local.txt, got %s", localPath) - } - if isUpload { - t.Error("expected isUpload=false") - } -} - -func TestParseCopyArguments_BothLocal(t *testing.T) { - _, _, _, _, err := parseCopyArguments("./a", "./b") - if err == nil { - t.Fatal("expected error when both paths are local") - } -} - -func TestParseCopyArguments_BothRemote(t *testing.T) { - _, _, _, _, err := parseCopyArguments("ws1:/a", "ws2:/b") - if err == nil { - t.Fatal("expected error when both paths are remote") - } -} - -func TestParseWorkspacePath_Local(t *testing.T) { - ws, fp, err := parseWorkspacePath("/tmp/local/file") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if ws != "" { - t.Errorf("expected empty workspace, got %s", ws) - } - if fp != "/tmp/local/file" { - t.Errorf("expected /tmp/local/file, got %s", fp) - } -} - -func TestParseWorkspacePath_Remote(t *testing.T) { - ws, fp, err := parseWorkspacePath("my-instance:/remote/path") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if ws != "my-instance" { - t.Errorf("expected my-instance, got %s", ws) - } - if fp != "/remote/path" { - t.Errorf("expected /remote/path, got %s", fp) - } -} - -func TestParseWorkspacePath_InvalidMultipleColons(t *testing.T) { - _, _, err := parseWorkspacePath("ws:path:extra") - if err == nil { - t.Fatal("expected error for multiple colons") - } -}