From a2c6f4ddf0fac1ff0893bb6053581a158f24617a Mon Sep 17 00:00:00 2001 From: immanuel-peter Date: Thu, 26 Feb 2026 10:40:23 -0600 Subject: [PATCH 1/3] 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 e0abb17e..a3b4a089 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 00000000..23ca80c3 --- /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/3] 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 a3b4a089..054ee3b2 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/3] 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 054ee3b2..6fb92f2a 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 23ca80c3..548323ac 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:") }) }