Add backup/rollback support and structured logging

internal/brew/brew.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"net"
-	"os"
 	"os/exec"
 	"strings"
 	"sync"
@@ -217,16 +216,11 @@ func Update(dryRun bool) error {
 	}
 
 	ui.Info("Upgrading packages...")
-	// Upgrade may prompt for sudo; keep direct exec.Command so we can wire in a TTY.
-	cmd := exec.Command("brew", "upgrade")
-	cmd.Stdout = os.Stdout
-	cmd.Stderr = os.Stderr
-	tty, opened := system.OpenTTY()
-	if opened {
-		defer tty.Close() //nolint:errcheck // best-effort TTY cleanup
-	}
-	cmd.Stdin = tty
-	return cmd.Run()
+	// Upgrade may prompt for sudo; RunInteractive wires the current TTY.
+	if err := currentRunner().RunInteractive("upgrade"); err != nil {
+		return fmt.Errorf("brew upgrade: %w", err)
+	}
+	return nil
 }
 
 func Cleanup() error {
@@ -334,10 +328,7 @@ func PreInstallChecks(packageCount int) error {
 	}
 
 	ui.Info("Updating Homebrew index...")
-	updateCmd := exec.Command("brew", "update")
-	updateCmd.Stdout = nil
-	updateCmd.Stderr = nil
-	if err := updateCmd.Run(); err != nil {
+	if err := currentRunner().RunInteractive("update"); err != nil {
 		ui.Warn("brew update failed, continuing anyway...")
 	}
 

internal/brew/brew_command_test.go

@@ -44,6 +44,11 @@ func (f *fakeRunner) Run(args ...string) error {
 	return err
 }
 
+func (f *fakeRunner) RunInteractive(args ...string) error {
+	_, err := f.handler(args)
+	return err
+}
+
 // withFakeBrew installs a fakeRunner for the duration of the test and
 // restores the previous runner on cleanup.
 func withFakeBrew(t *testing.T, handler func(args []string) ([]byte, error)) {
@@ -107,10 +112,15 @@ func TestUpdateAndCleanup_UsesBrew(t *testing.T) {
 		return nil, nil
 	})
 
-	// Update now calls brew upgrade directly via exec.Command (TTY handling).
-	// Here we only verify that runner-routed calls (brew update, brew cleanup)
-	// were made. The brew upgrade path is exercised by integration tests.
-	err := Cleanup()
+	// Update now routes brew update + brew upgrade through the Runner
+	// (RunInteractive for upgrade to keep sudo-prompt TTY behavior). Both
+	// subcommands land in the fake handler.
+	err := Update(false)
+	assert.NoError(t, err)
+	assert.Contains(t, calls, "update")
+	assert.Contains(t, calls, "upgrade")
+
+	err = Cleanup()
 	assert.NoError(t, err)
 	assert.Contains(t, calls, "cleanup")
 }

internal/brew/brew_install.go

@@ -328,6 +328,12 @@ func installCaskWithProgress(pkg string, progress *ui.StickyProgress) string {
 	return ""
 }
 
+// Runner-exempt: this helper sets HOMEBREW_NO_AUTO_UPDATE=1 and returns a raw
+// *exec.Cmd so callers can wire custom stdout pipes (StickyProgress streaming)
+// and TTY stdin (sudo prompts for cask installs). The Runner interface cannot
+// express either of those cleanly, so Install / InstallCask /
+// installCaskWithProgress / brewCombinedOutputWithTTY / installFormulaWithError
+// / installSmartCaskWithError continue to use this helper directly.
 func brewInstallCmd(args ...string) *exec.Cmd {
 	cmd := exec.Command("brew", args...) //nolint:gosec // "brew" is a hardcoded binary; args are package names validated by caller
 	cmd.Env = append(os.Environ(), "HOMEBREW_NO_AUTO_UPDATE=1")
@@ -473,8 +479,7 @@ func ResolveFormulaNames(names []string) map[string]string {
 	}
 
 	args := append([]string{"info", "--json"}, names...)
-	cmd := exec.Command("brew", args...) //nolint:gosec // "brew" is a hardcoded binary; args are package names
-	output, err := cmd.Output()
+	output, err := currentRunner().Output(args...)
 	if err != nil {
 		return identityMap(names)
 	}

internal/brew/runner.go

@@ -4,14 +4,21 @@ import (
 	"os"
 	"os/exec"
 	"sync"
+
+	"github.com/openbootdotdev/openboot/internal/system"
 )
 
 // Runner is the swappable executor for brew subcommands. The package uses a
 // default implementation that invokes the real `brew` binary; tests replace it
 // with a fake runner to avoid fork/exec overhead.
 //
-// Only the common patterns are covered here. Complex cases (install progress
-// streaming, TTY-wrapped sudo prompts) still use exec.Command directly.
+// Coverage notes — the following call sites remain outside the Runner because
+// they need features Runner does not express cleanly:
+//   - progress-stream install path (brew_install.go: brewInstallCmd / Install /
+//     InstallCask / installCaskWithProgress / brewCombinedOutputWithTTY /
+//     installFormulaWithError / installSmartCaskWithError) — these rely on
+//     the HOMEBREW_NO_AUTO_UPDATE env var plus custom stdout pipe wiring for
+//     StickyProgress and TTY stdin for sudo prompts.
 type Runner interface {
 	// Output runs `brew args...` and returns stdout only.
 	Output(args ...string) ([]byte, error)
@@ -20,8 +27,15 @@ type Runner interface {
 	CombinedOutput(args ...string) ([]byte, error)
 
 	// Run runs `brew args...` with stdout/stderr attached to the process,
-	// so the user sees progress output. Returns the exit error.
+	// so the user sees progress output. Stdin is NOT attached. Returns the
+	// exit error.
 	Run(args ...string) error
+
+	// RunInteractive runs `brew args...` attached to the current TTY
+	// (stdin+stdout+stderr) so subcommands like `brew upgrade` that may
+	// prompt for a sudo password work correctly. If /dev/tty cannot be
+	// opened, falls back to os.Stdin. Returns the exit error.
+	RunInteractive(args ...string) error
 }
 
 type execRunner struct{}
@@ -41,6 +55,18 @@ func (execRunner) Run(args ...string) error {
 	return cmd.Run()
 }
 
+func (execRunner) RunInteractive(args ...string) error {
+	cmd := exec.Command("brew", args...) //nolint:gosec // "brew" is hardcoded; args are package names
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	tty, opened := system.OpenTTY()
+	if opened {
+		defer tty.Close() //nolint:errcheck // best-effort TTY cleanup
+	}
+	cmd.Stdin = tty
+	return cmd.Run()
+}
+
 var (
 	runnerMu sync.RWMutex
 	runner   Runner = execRunner{}

internal/brew/runner_test.go

@@ -0,0 +1,140 @@
+package brew
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// recordingRunner captures which Runner method was invoked and with which args
+// so tests can assert on routing (Run vs RunInteractive) without fork/exec.
+type recordingRunner struct {
+	outputCalls         [][]string
+	combinedOutputCalls [][]string
+	runCalls            [][]string
+	runInteractiveCalls [][]string
+	runErr              error
+	runInteractiveErr   error
+}
+
+func (r *recordingRunner) Output(args ...string) ([]byte, error) {
+	r.outputCalls = append(r.outputCalls, append([]string(nil), args...))
+	return nil, nil
+}
+
+func (r *recordingRunner) CombinedOutput(args ...string) ([]byte, error) {
+	r.combinedOutputCalls = append(r.combinedOutputCalls, append([]string(nil), args...))
+	return nil, nil
+}
+
+func (r *recordingRunner) Run(args ...string) error {
+	r.runCalls = append(r.runCalls, append([]string(nil), args...))
+	return r.runErr
+}
+
+func (r *recordingRunner) RunInteractive(args ...string) error {
+	r.runInteractiveCalls = append(r.runInteractiveCalls, append([]string(nil), args...))
+	return r.runInteractiveErr
+}
+
+func TestUpdate_RoutesUpgradeThroughRunInteractive(t *testing.T) {
+	rec := &recordingRunner{}
+	t.Cleanup(SetRunner(rec))
+
+	err := Update(false)
+	require.NoError(t, err)
+
+	// brew update is not sudo-gated, so Run is fine.
+	require.Len(t, rec.runCalls, 1)
+	assert.Equal(t, []string{"update"}, rec.runCalls[0])
+
+	// brew upgrade may prompt for sudo, so it must go through RunInteractive.
+	require.Len(t, rec.runInteractiveCalls, 1)
+	assert.Equal(t, []string{"upgrade"}, rec.runInteractiveCalls[0])
+}
+
+// TestUpdate_ErrorPropagation is table-driven: each case swaps in a recording
+// runner with a preset error on one of the two methods and verifies that
+// Update returns a wrapped error mentioning the offending subcommand.
+func TestUpdate_ErrorPropagation(t *testing.T) {
+	tests := []struct {
+		name              string
+		runErr            error
+		runInteractiveErr error
+		wantErrContains   string
+	}{
+		{
+			name:            "brew update fails",
+			runErr:          errors.New("network down"),
+			wantErrContains: "brew update",
+		},
+		{
+			name:              "brew upgrade fails",
+			runInteractiveErr: errors.New("sudo refused"),
+			wantErrContains:   "brew upgrade",
+		},
+		{
+			name: "both succeed",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rec := &recordingRunner{
+				runErr:            tt.runErr,
+				runInteractiveErr: tt.runInteractiveErr,
+			}
+			t.Cleanup(SetRunner(rec))
+
+			err := Update(false)
+			if tt.wantErrContains == "" {
+				assert.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), tt.wantErrContains)
+		})
+	}
+}
+
+func TestUpdate_DryRunDoesNotInvokeRunner(t *testing.T) {
+	rec := &recordingRunner{}
+	t.Cleanup(SetRunner(rec))
+
+	err := Update(true)
+	require.NoError(t, err)
+	assert.Empty(t, rec.runCalls)
+	assert.Empty(t, rec.runInteractiveCalls)
+}
+
+func TestPreInstallChecks_UsesRunInteractiveForUpdate(t *testing.T) {
+	rec := &recordingRunner{}
+	t.Cleanup(SetRunner(rec))
+
+	orig := checkNetworkFunc
+	checkNetworkFunc = func() error { return nil }
+	t.Cleanup(func() { checkNetworkFunc = orig })
+
+	err := PreInstallChecks(1)
+	require.NoError(t, err)
+
+	// The index-refresh `brew update` inside PreInstallChecks now routes
+	// through RunInteractive (so any prompts reach the user's TTY).
+	require.Len(t, rec.runInteractiveCalls, 1)
+	assert.Equal(t, []string{"update"}, rec.runInteractiveCalls[0])
+}
+
+func TestSetRunner_RestoreReinstatesPrevious(t *testing.T) {
+	before := currentRunner()
+
+	rec := &recordingRunner{}
+	restore := SetRunner(rec)
+	assert.Same(t, rec, currentRunner())
+
+	restore()
+	// `before` may hold a value-typed execRunner, so compare by type/value
+	// rather than pointer identity.
+	assert.IsType(t, before, currentRunner())
+}

internal/cli/root.go

@@ -3,14 +3,14 @@ package cli
 import (
 	"context"
 	"fmt"
-	"log/slog"
 	"os"
 	"os/signal"
 	"syscall"
 
 	"github.com/spf13/cobra"
 
 	"github.com/openbootdotdev/openboot/internal/config"
+	"github.com/openbootdotdev/openboot/internal/logging"
 	"github.com/openbootdotdev/openboot/internal/updater"
 )
 
@@ -38,9 +38,13 @@ shell configuration, and macOS preferences.`,
   # Capture your current environment
   openboot snapshot --json > my-setup.json`,
 	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
-		if verbose {
-			slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})))
+		// Install always-on file logging; --verbose controls stderr level.
+		// Failure here is never fatal — Init falls back to stderr internally.
+		closer, err := logging.Init(version, verbose)
+		if err != nil {
+			return fmt.Errorf("init logging: %w", err)
 		}
+		logCloser = closer
 
 		config.SetClientVersion(version)
 		installCfg.Version = version
@@ -94,6 +98,7 @@ func init() {
 	rootCmd.AddCommand(snapshotCmd)
 	rootCmd.AddCommand(loginCmd)
 	rootCmd.AddCommand(logoutCmd)
+	rootCmd.AddCommand(updateCmd)
 
 	rootCmd.SetUsageTemplate(usageTemplate)
 }
@@ -138,8 +143,16 @@ Learn more:
 // verbose is set by the --verbose persistent flag.
 var verbose bool
 
+// logCloser is set by PersistentPreRunE and flushed by Execute on return.
+var logCloser func()
+
 func Execute() error {
 	ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
 	defer cancel()
+	defer func() {
+		if logCloser != nil {
+			logCloser()
+		}
+	}()
 	return rootCmd.ExecuteContext(ctx)
 }