From 0c76531e06904acb483cfeb4270bb78834fd87da Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 12:45:58 +0000 Subject: [PATCH 1/6] feat(updater): add --version pin, rollback, and backup retention Introduces `openboot update` subcommand with three modes beyond the default latest-version upgrade: - `--version X.Y.Z` pins a specific release (URL-resolved, SHA-256 verified) - `--rollback` restores the newest pre-upgrade backup - `--list-backups` prints the backup directory Each direct-download upgrade now copies the running binary to ~/.openboot/backup/ before atomic rename; backupRetention=5. Homebrew- managed installs reject --version / --rollback with a clear hint. DownloadAndReplace now takes an explicit version (empty = /latest/, for back-compat with AutoUpgrade). fetchChecksums signature updated accordingly; test fixed. --- internal/cli/root.go | 1 + internal/cli/update.go | 179 ++++++++++++++ internal/updater/backup_test.go | 317 +++++++++++++++++++++++++ internal/updater/updater.go | 288 +++++++++++++++++++++- internal/updater/updater_extra_test.go | 2 +- 5 files changed, 775 insertions(+), 12 deletions(-) create mode 100644 internal/cli/update.go create mode 100644 internal/updater/backup_test.go diff --git a/internal/cli/root.go b/internal/cli/root.go index 34d0c17..e11ce70 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -94,6 +94,7 @@ func init() { rootCmd.AddCommand(snapshotCmd) rootCmd.AddCommand(loginCmd) rootCmd.AddCommand(logoutCmd) + rootCmd.AddCommand(updateCmd) rootCmd.SetUsageTemplate(usageTemplate) } diff --git a/internal/cli/update.go b/internal/cli/update.go new file mode 100644 index 0000000..e9b31b6 --- /dev/null +++ b/internal/cli/update.go @@ -0,0 +1,179 @@ +package cli + +import ( + "fmt" + + "github.com/spf13/cobra" + + "github.com/openbootdotdev/openboot/internal/ui" + "github.com/openbootdotdev/openboot/internal/updater" +) + +// update subcommand flags. Package-level so tests can reset them if needed. +var ( + updateVersion string + updateRollback bool + updateListBackups bool + updateDryRun bool +) + +var updateCmd = &cobra.Command{ + Use: "update", + Short: "Update OpenBoot, pin to a specific version, or roll back", + Long: `Manage the OpenBoot binary itself. + +Examples: + openboot update # upgrade to the latest release + openboot update --version 0.24.1 # pin to an exact version (must be X.Y.Z) + openboot update --rollback # restore the most recent pre-upgrade backup + openboot update --list-backups # print pre-upgrade backups under ~/.openboot/backup + +Backups are written to ~/.openboot/backup/ before each direct-download +upgrade. Only the 5 most recent backups are retained. + +If OpenBoot was installed via Homebrew, --version and --rollback are rejected; +use 'brew upgrade openboot' or 'brew' commands instead.`, + Args: cobra.NoArgs, + SilenceUsage: true, + RunE: runUpdateCmd, +} + +func init() { + updateCmd.Flags().SortFlags = false + updateCmd.Flags().StringVar(&updateVersion, "version", "", "pin to an exact version (e.g. 0.25.0)") + updateCmd.Flags().BoolVar(&updateRollback, "rollback", false, "restore the most recent backup") + updateCmd.Flags().BoolVar(&updateListBackups, "list-backups", false, "list backups under ~/.openboot/backup") + updateCmd.Flags().BoolVar(&updateDryRun, "dry-run", false, "print what would be done without making changes") +} + +func runUpdateCmd(_ *cobra.Command, _ []string) error { + // Mutually-exclusive modes: only one of --rollback / --list-backups / + // --version may be set (version-less upgrade is the default). + set := 0 + if updateRollback { + set++ + } + if updateListBackups { + set++ + } + if updateVersion != "" { + set++ + } + if set > 1 { + return fmt.Errorf("--version, --rollback, and --list-backups are mutually exclusive") + } + + switch { + case updateListBackups: + return runListBackups() + case updateRollback: + return runRollback() + case updateVersion != "": + return runPinnedUpgrade(updateVersion) + default: + return runLatestUpgrade() + } +} + +func runListBackups() error { + dir, err := updater.GetBackupDir() + if err != nil { + return fmt.Errorf("resolve backup dir: %w", err) + } + names, err := updater.ListBackups() + if err != nil { + return fmt.Errorf("list backups: %w", err) + } + if len(names) == 0 { + ui.Info(fmt.Sprintf("No backups in %s", dir)) + return nil + } + ui.Header(fmt.Sprintf("Backups in %s", dir)) + for _, n := range names { + fmt.Println(" " + n) + } + return nil +} + +func runRollback() error { + if updater.IsHomebrewInstall() { + ui.Warn("OpenBoot is managed by Homebrew — rollback is not supported.") + ui.Muted("Use 'brew' commands to change versions.") + return fmt.Errorf("rollback refused: Homebrew-managed install") + } + if updateDryRun { + names, err := updater.ListBackups() + if err != nil { + return fmt.Errorf("list backups: %w", err) + } + if len(names) == 0 { + ui.Info("Dry-run: no backups available to roll back to.") + return nil + } + ui.Info(fmt.Sprintf("Dry-run: would restore backup %s", names[0])) + return nil + } + if err := updater.Rollback(); err != nil { + return fmt.Errorf("rollback: %w", err) + } + ui.Success("Rollback complete.") + return nil +} + +func runPinnedUpgrade(v string) error { + if err := updater.ValidateSemver(v); err != nil { + return err + } + if updater.IsHomebrewInstall() { + ui.Warn("OpenBoot is managed by Homebrew — --version is not supported.") + ui.Muted("Run 'brew upgrade openboot' to update via Homebrew.") + return fmt.Errorf("version pin refused: Homebrew-managed install") + } + if updateDryRun { + ui.Info(fmt.Sprintf("Dry-run: would download and install OpenBoot v%s", trimV(v))) + return nil + } + ui.Info(fmt.Sprintf("Installing OpenBoot v%s...", trimV(v))) + // Let the backup record the running version. + updater.SetCurrentVersionForTesting(func() string { return version }) + defer updater.SetCurrentVersionForTesting(func() string { return "" }) + + if err := updater.DownloadAndReplace(v); err != nil { + return fmt.Errorf("update failed: %w", err) + } + ui.Success(fmt.Sprintf("Installed OpenBoot v%s.", trimV(v))) + return nil +} + +func runLatestUpgrade() error { + if updater.IsHomebrewInstall() { + ui.Warn("OpenBoot is managed by Homebrew.") + ui.Muted("Run 'brew upgrade openboot' to update.") + return fmt.Errorf("use 'brew upgrade openboot'") + } + if updateDryRun { + ui.Info("Dry-run: would check GitHub for the latest release and upgrade.") + return nil + } + latest, err := updater.GetLatestVersion() + if err != nil { + return fmt.Errorf("look up latest version: %w", err) + } + ui.Info(fmt.Sprintf("Installing OpenBoot %s...", latest)) + updater.SetCurrentVersionForTesting(func() string { return version }) + defer updater.SetCurrentVersionForTesting(func() string { return "" }) + + if err := updater.DownloadAndReplace(latest); err != nil { + return fmt.Errorf("update failed: %w", err) + } + ui.Success(fmt.Sprintf("Installed OpenBoot %s.", latest)) + return nil +} + +// trimV strips an optional leading "v" for display. +func trimV(v string) string { + if len(v) > 0 && v[0] == 'v' { + return v[1:] + } + return v +} diff --git a/internal/updater/backup_test.go b/internal/updater/backup_test.go new file mode 100644 index 0000000..392adbf --- /dev/null +++ b/internal/updater/backup_test.go @@ -0,0 +1,317 @@ +// NOTE: tests in this file must NOT use t.Parallel() due to shared +// package-level variables (backupDirOverride, currentBinaryVersionFn). +package updater + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- Semver validation --- + +func TestValidateSemver(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {"plain semver", "0.25.0", false}, + {"with v prefix", "v0.25.0", false}, + {"multi-digit components", "12.345.6789", false}, + {"empty", "", true}, + {"not semver word", "latest", true}, + {"incomplete two-part", "1.2", true}, + {"four parts", "1.2.3.4", true}, + {"non-numeric major", "a.1.1", true}, + {"prerelease suffix", "1.2.3-rc1", true}, + {"leading space", " 1.2.3", true}, + {"trailing newline", "1.2.3\n", true}, + {"double v prefix", "vv1.2.3", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateSemver(tt.input) + if tt.wantErr { + assert.Error(t, err, "ValidateSemver(%q) expected error", tt.input) + } else { + assert.NoError(t, err, "ValidateSemver(%q) unexpected error", tt.input) + } + }) + } +} + +// --- URL construction --- + +func TestChecksumsURL(t *testing.T) { + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/latest/download/checksums.txt", + checksumsURL(""), + "empty version should use /latest/", + ) + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/download/v0.25.0/checksums.txt", + checksumsURL("0.25.0"), + ) + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/download/v0.25.0/checksums.txt", + checksumsURL("v0.25.0"), + "leading v should be normalized", + ) +} + +func TestBinaryURL(t *testing.T) { + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/latest/download/openboot-darwin-arm64", + binaryURL("", "openboot-darwin-arm64"), + ) + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/download/v0.25.0/openboot-darwin-arm64", + binaryURL("0.25.0", "openboot-darwin-arm64"), + ) + assert.Equal(t, + "https://github.com/openbootdotdev/openboot/releases/download/v0.25.0/openboot-darwin-arm64", + binaryURL("v0.25.0", "openboot-darwin-arm64"), + ) +} + +// --- Backup directory / creation / pruning --- + +func TestGetBackupDir_DefaultsToHome(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + SetBackupDirForTesting("") + dir, err := GetBackupDir() + require.NoError(t, err) + assert.Contains(t, dir, ".openboot") + assert.Contains(t, dir, "backup") +} + +func TestGetBackupDir_Override(t *testing.T) { + tmp := t.TempDir() + SetBackupDirForTesting(tmp) + defer SetBackupDirForTesting("") + dir, err := GetBackupDir() + require.NoError(t, err) + assert.Equal(t, tmp, dir) +} + +func TestBackupCurrentBinary_CreatesFile(t *testing.T) { + tmp := t.TempDir() + backupDir := filepath.Join(tmp, "backup") + SetBackupDirForTesting(backupDir) + defer SetBackupDirForTesting("") + + origFn := currentBinaryVersionFn + currentBinaryVersionFn = func() string { return "1.2.3" } + defer func() { currentBinaryVersionFn = origFn }() + + bin := filepath.Join(tmp, "openboot") + require.NoError(t, os.WriteFile(bin, []byte("fake binary"), 0755)) + + require.NoError(t, backupCurrentBinary(bin)) + + entries, err := os.ReadDir(backupDir) + require.NoError(t, err) + require.Len(t, entries, 1, "expected one backup file") + name := entries[0].Name() + assert.Contains(t, name, "openboot-1.2.3-") + + // Contents must match the source. + data, err := os.ReadFile(filepath.Join(backupDir, name)) + require.NoError(t, err) + assert.Equal(t, []byte("fake binary"), data) + + // File mode must be 0755 so the restored binary is executable. + info, err := os.Stat(filepath.Join(backupDir, name)) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0755), info.Mode().Perm()) +} + +func TestBackupCurrentBinary_UnknownVersionWhenBlank(t *testing.T) { + tmp := t.TempDir() + backupDir := filepath.Join(tmp, "backup") + SetBackupDirForTesting(backupDir) + defer SetBackupDirForTesting("") + + origFn := currentBinaryVersionFn + currentBinaryVersionFn = func() string { return "" } + defer func() { currentBinaryVersionFn = origFn }() + + bin := filepath.Join(tmp, "openboot") + require.NoError(t, os.WriteFile(bin, []byte("x"), 0755)) + require.NoError(t, backupCurrentBinary(bin)) + + entries, err := os.ReadDir(backupDir) + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Contains(t, entries[0].Name(), "openboot-unknown-") +} + +// Writes n backup files with staggered, deterministic mtimes so prune order +// is unambiguous. Returns names in oldest→newest order. +func seedBackups(t *testing.T, dir string, n int) []string { + t.Helper() + require.NoError(t, os.MkdirAll(dir, 0700)) + names := make([]string, n) + base := time.Now().Add(-time.Duration(n) * time.Hour) + for i := 0; i < n; i++ { + name := filepath.Join(dir, "openboot-1.0.0-"+time.Now().Format("20060102T150405Z")+"-"+itoa(i)) + require.NoError(t, os.WriteFile(name, []byte("v"+itoa(i)), 0755)) + mt := base.Add(time.Duration(i) * time.Hour) + require.NoError(t, os.Chtimes(name, mt, mt)) + names[i] = name + } + return names +} + +func itoa(i int) string { + const digits = "0123456789" + if i == 0 { + return "0" + } + var buf [20]byte + pos := len(buf) + for i > 0 { + pos-- + buf[pos] = digits[i%10] + i /= 10 + } + return string(buf[pos:]) +} + +func TestPruneBackups_KeepsNewest(t *testing.T) { + tmp := t.TempDir() + names := seedBackups(t, tmp, 7) + + require.NoError(t, pruneBackups(tmp, 5)) + + entries, err := os.ReadDir(tmp) + require.NoError(t, err) + require.Len(t, entries, 5, "should retain 5 newest") + + // The two oldest should be gone. + _, err = os.Stat(names[0]) + assert.True(t, os.IsNotExist(err), "oldest backup should be pruned") + _, err = os.Stat(names[1]) + assert.True(t, os.IsNotExist(err), "second-oldest backup should be pruned") + // The newest should survive. + _, err = os.Stat(names[6]) + assert.NoError(t, err, "newest backup should survive prune") +} + +func TestPruneBackups_NoOpUnderLimit(t *testing.T) { + tmp := t.TempDir() + seedBackups(t, tmp, 3) + require.NoError(t, pruneBackups(tmp, 5)) + entries, err := os.ReadDir(tmp) + require.NoError(t, err) + assert.Len(t, entries, 3) +} + +func TestBackupCurrentBinary_PrunesAfterLimit(t *testing.T) { + tmp := t.TempDir() + backupDir := filepath.Join(tmp, "backup") + SetBackupDirForTesting(backupDir) + defer SetBackupDirForTesting("") + origFn := currentBinaryVersionFn + currentBinaryVersionFn = func() string { return "1.0.0" } + defer func() { currentBinaryVersionFn = origFn }() + + // Seed one-less-than-limit backups with old mtimes. + seedBackups(t, backupDir, backupRetention) + + bin := filepath.Join(tmp, "openboot") + require.NoError(t, os.WriteFile(bin, []byte("new"), 0755)) + require.NoError(t, backupCurrentBinary(bin)) + + entries, err := os.ReadDir(backupDir) + require.NoError(t, err) + assert.Len(t, entries, backupRetention, "should cap at retention limit after a new backup") +} + +// --- ListBackups --- + +func TestListBackups_EmptyDirReturnsNil(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + SetBackupDirForTesting("") + names, err := ListBackups() + require.NoError(t, err) + assert.Empty(t, names, "missing dir should return empty slice, not error") +} + +func TestListBackups_NewestFirst(t *testing.T) { + tmp := t.TempDir() + SetBackupDirForTesting(tmp) + defer SetBackupDirForTesting("") + + names := seedBackups(t, tmp, 3) + got, err := ListBackups() + require.NoError(t, err) + require.Len(t, got, 3) + // Newest mtime is the last name we seeded. + assert.Equal(t, filepath.Base(names[2]), got[0], "newest should be first") + assert.Equal(t, filepath.Base(names[0]), got[2], "oldest should be last") +} + +// --- Rollback --- + +func TestRollback_NoBackups(t *testing.T) { + tmp := t.TempDir() + SetBackupDirForTesting(tmp) + defer SetBackupDirForTesting("") + + err := Rollback() + require.Error(t, err) + assert.Contains(t, err.Error(), "no backups") +} + +func TestRollback_PicksNewest(t *testing.T) { + // Create a fake "current binary" in the temp dir so os.Executable() points + // somewhere we can safely overwrite. We can't override os.Executable() + // itself, so simulate rollback via the internals: exercise listBackupsSorted + // and copyFile, which together back Rollback(). + tmp := t.TempDir() + backupDir := filepath.Join(tmp, "backup") + require.NoError(t, os.MkdirAll(backupDir, 0700)) + + // Seed three backups with escalating mtimes + distinct contents. + base := time.Now().Add(-3 * time.Hour) + for i, contents := range []string{"oldest", "middle", "newest"} { + path := filepath.Join(backupDir, "openboot-1.0.0-"+itoa(i)) + require.NoError(t, os.WriteFile(path, []byte(contents), 0755)) + mt := base.Add(time.Duration(i) * time.Hour) + require.NoError(t, os.Chtimes(path, mt, mt)) + } + + files, err := listBackupsSorted(backupDir) + require.NoError(t, err) + require.Len(t, files, 3) + + // Newest first — "newest" content file should be picked. + data, err := os.ReadFile(filepath.Join(backupDir, files[0].Name())) + require.NoError(t, err) + assert.Equal(t, "newest", string(data)) + + // copyFile should produce a 0755 executable clone. + dst := filepath.Join(tmp, "restored") + require.NoError(t, copyFile(filepath.Join(backupDir, files[0].Name()), dst, 0755)) + info, err := os.Stat(dst) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0755), info.Mode().Perm()) + got, err := os.ReadFile(dst) + require.NoError(t, err) + assert.Equal(t, "newest", string(got)) +} + +func TestRollback_ErrorMessageMentionsBackupDir(t *testing.T) { + tmp := t.TempDir() + SetBackupDirForTesting(tmp) + defer SetBackupDirForTesting("") + err := Rollback() + require.Error(t, err) + assert.Contains(t, err.Error(), tmp, "error should mention the backup directory") +} diff --git a/internal/updater/updater.go b/internal/updater/updater.go index c8ea9b7..3b4a6ca 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -11,7 +11,9 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "runtime" + "sort" "strconv" "strings" "sync" @@ -229,7 +231,11 @@ func doDirectUpgrade(currentVersion, latestVersion string) { latestClean := trimVersionPrefix(latestVersion) currentClean := trimVersionPrefix(currentVersion) ui.Info(fmt.Sprintf("Updating OpenBoot v%s → v%s...", currentClean, latestClean)) - if err := DownloadAndReplace(); err != nil { + // Record the running version so backupCurrentBinary can label the backup. + prevFn := currentBinaryVersionFn + currentBinaryVersionFn = func() string { return currentVersion } + defer func() { currentBinaryVersionFn = prevFn }() + if err := DownloadAndReplace(latestVersion); err != nil { ui.Warn(fmt.Sprintf("Auto-update failed: %v", err)) ui.Muted("Run 'openboot update --self' to update manually") fmt.Println() @@ -322,10 +328,11 @@ func verifyChecksum(path, filename string, checksums map[string]string) error { } // fetchChecksums downloads and parses the checksums.txt file published -// alongside a GitHub release. Uses the /latest/ redirect so it matches the -// binary downloaded in DownloadAndReplace. -func fetchChecksums(client *http.Client) (map[string]string, error) { - url := "https://github.com/openbootdotdev/openboot/releases/latest/download/checksums.txt" +// alongside a GitHub release. If version is empty, the /latest/ redirect is +// used (back-compat with AutoUpgrade); otherwise the exact release path +// /releases/download/v/ is used. +func fetchChecksums(client *http.Client, version string) (map[string]string, error) { + url := checksumsURL(version) resp, err := client.Get(url) if err != nil { return nil, fmt.Errorf("download checksums: %w", err) @@ -338,7 +345,34 @@ func fetchChecksums(client *http.Client) (map[string]string, error) { return parseChecksumsFile(io.LimitReader(resp.Body, 1<<20)) } -func DownloadAndReplace() error { +// checksumsURL returns the correct checksums.txt URL for the given version. +// Empty version means use /latest/. +func checksumsURL(version string) string { + if version == "" { + return "https://github.com/openbootdotdev/openboot/releases/latest/download/checksums.txt" + } + return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/checksums.txt", trimVersionPrefix(version)) +} + +// binaryURL returns the correct binary download URL for the given version and +// filename. Empty version means use /latest/. +func binaryURL(version, filename string) string { + if version == "" { + return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/latest/download/%s", filename) + } + return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/%s", trimVersionPrefix(version), filename) +} + +// DownloadAndReplace downloads the openboot binary for the given version and +// atomically replaces the currently-running binary. +// +// If version is empty, the GitHub /latest/ redirect is used (back-compat with +// AutoUpgrade). If version is non-empty, the exact release URL is used. +// +// Before the atomic rename, the current binary is copied to the backup +// directory (see GetBackupDir) for rollback via Rollback(). The newest +// backupRetention backups are kept; older ones are pruned. +func DownloadAndReplace(version string) error { if IsHomebrewInstall() { return fmt.Errorf("openboot is managed by Homebrew — run 'brew upgrade openboot' instead") } @@ -354,15 +388,12 @@ func DownloadAndReplace() error { // Fetch checksums first so we can verify integrity before overwriting the // running binary. If the checksums file is missing or malformed, abort. - checksums, err := fetchChecksums(client) + checksums, err := fetchChecksums(client, version) if err != nil { return fmt.Errorf("verify update integrity: %w", err) } - // Uses /latest/ redirect — always downloads whatever GitHub considers the - // current latest release. The displayed version (from resolveLatestVersion) - // may differ by one patch if a release lands between the check and download. - url := fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/latest/download/%s", filename) + url := binaryURL(version, filename) resp, err := client.Get(url) if err != nil { @@ -417,6 +448,14 @@ func DownloadAndReplace() error { return fmt.Errorf("chmod: %w", err) } + // Back up the currently-running binary before we overwrite it. If backup + // fails, we still proceed — the user asked to update and integrity is + // already verified. Warn via UI so the user knows rollback is unavailable + // for this upgrade. + if err := backupCurrentBinary(binPath); err != nil { + ui.Warn(fmt.Sprintf("could not create backup (rollback unavailable for this upgrade): %v", err)) + } + if err := os.Rename(tmpPath, binPath); err != nil { return fmt.Errorf("replace binary: %w", err) } @@ -425,6 +464,233 @@ func DownloadAndReplace() error { return nil } +// --- Backup & rollback --- + +// backupDirOverride is set by tests to redirect the backup directory. +// When empty, GetBackupDir returns ~/.openboot/backup. +var backupDirOverride string + +// backupRetention is the maximum number of backups to keep. Older ones are +// pruned after each successful backup. +const backupRetention = 5 + +// currentBinaryVersion returns a version string to embed in backup filenames. +// Tests can override via currentBinaryVersionFn. +var currentBinaryVersionFn = func() string { return "" } + +// GetBackupDir returns the directory where pre-upgrade binary backups are +// stored. Defaults to ~/.openboot/backup; tests may override via +// SetBackupDirForTesting. +func GetBackupDir() (string, error) { + if backupDirOverride != "" { + return backupDirOverride, nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("home dir: %w", err) + } + return filepath.Join(home, ".openboot", "backup"), nil +} + +// SetBackupDirForTesting overrides the backup directory. Tests should defer a +// reset to "". It is exported for use by the cli package tests. +func SetBackupDirForTesting(dir string) { + backupDirOverride = dir +} + +// SetCurrentVersionForTesting overrides the value used in backup filenames. +// When unset, backups use "unknown" as the version tag. +func SetCurrentVersionForTesting(fn func() string) { + currentBinaryVersionFn = fn +} + +// backupCurrentBinary copies the binary at binPath into the backup directory +// with a timestamped filename and prunes old backups beyond backupRetention. +func backupCurrentBinary(binPath string) error { + dir, err := GetBackupDir() + if err != nil { + return err + } + if err := os.MkdirAll(dir, 0700); err != nil { + return fmt.Errorf("mkdir backup dir: %w", err) + } + + version := currentBinaryVersionFn() + if version == "" { + version = "unknown" + } + version = trimVersionPrefix(version) + + ts := time.Now().UTC().Format("20060102T150405Z") + dst := filepath.Join(dir, fmt.Sprintf("openboot-%s-%s", version, ts)) + + if err := copyFile(binPath, dst, 0755); err != nil { + return fmt.Errorf("copy binary: %w", err) + } + + if err := pruneBackups(dir, backupRetention); err != nil { + // Pruning failure is not fatal — backup succeeded. + ui.Muted(fmt.Sprintf("Warning: could not prune old backups: %v", err)) + } + return nil +} + +// copyFile copies src to dst, preserving the given permission mode on dst. +func copyFile(src, dst string, mode os.FileMode) error { + in, err := os.Open(src) + if err != nil { + return fmt.Errorf("open src: %w", err) + } + defer in.Close() //nolint:errcheck // read-only + + out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) //nolint:gosec // backup files must be executable + if err != nil { + return fmt.Errorf("create dst: %w", err) + } + if _, err := io.Copy(out, in); err != nil { + out.Close() //nolint:errcheck,gosec // returning the more descriptive error + return fmt.Errorf("copy: %w", err) + } + if err := out.Close(); err != nil { + return fmt.Errorf("close dst: %w", err) + } + return nil +} + +// listBackupsSorted returns backup file entries sorted by modification time, +// newest first. +func listBackupsSorted(dir string) ([]os.DirEntry, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + // Filter to files only and sort by modtime descending. + files := entries[:0] + for _, e := range entries { + if e.IsDir() { + continue + } + files = append(files, e) + } + sort.Slice(files, func(i, j int) bool { + ii, _ := files[i].Info() + jj, _ := files[j].Info() + if ii == nil || jj == nil { + return files[i].Name() > files[j].Name() + } + return ii.ModTime().After(jj.ModTime()) + }) + return files, nil +} + +// pruneBackups deletes the oldest backup files in dir so that at most keep +// files remain. +func pruneBackups(dir string, keep int) error { + files, err := listBackupsSorted(dir) + if err != nil { + return err + } + if len(files) <= keep { + return nil + } + for _, f := range files[keep:] { + if err := os.Remove(filepath.Join(dir, f.Name())); err != nil { + return fmt.Errorf("remove %s: %w", f.Name(), err) + } + } + return nil +} + +// ListBackups returns the names of backup files in the backup directory, +// newest first. Returns an empty slice (not an error) if the directory does +// not exist. +func ListBackups() ([]string, error) { + dir, err := GetBackupDir() + if err != nil { + return nil, err + } + files, err := listBackupsSorted(dir) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read backup dir: %w", err) + } + names := make([]string, 0, len(files)) + for _, f := range files { + names = append(names, f.Name()) + } + return names, nil +} + +// Rollback restores the most recent backup over the currently-running +// binary. Fails if no backup exists or openboot is managed by Homebrew. +func Rollback() error { + if IsHomebrewInstall() { + return fmt.Errorf("openboot is managed by Homebrew — rollback is not supported (use 'brew' commands)") + } + dir, err := GetBackupDir() + if err != nil { + return err + } + files, err := listBackupsSorted(dir) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("no backups found in %s", dir) + } + return fmt.Errorf("read backup dir: %w", err) + } + if len(files) == 0 { + return fmt.Errorf("no backups found in %s", dir) + } + + binPath, err := os.Executable() + if err != nil { + return fmt.Errorf("binary path: %w", err) + } + binPath, err = filepath.EvalSymlinks(binPath) + if err != nil { + return fmt.Errorf("resolve binary path: %w", err) + } + + src := filepath.Join(dir, files[0].Name()) + tmpPath := binPath + ".rollback.tmp" + if err := copyFile(src, tmpPath, 0755); err != nil { + return fmt.Errorf("stage rollback: %w", err) + } + needsCleanup := true + defer func() { + if needsCleanup { + os.Remove(tmpPath) //nolint:errcheck,gosec // best-effort cleanup + } + }() + + if err := os.Rename(tmpPath, binPath); err != nil { + return fmt.Errorf("replace binary: %w", err) + } + needsCleanup = false + return nil +} + +// --- Semver validation --- + +// semverRe accepts plain X.Y.Z form (with optional leading v). Pre-release and +// build metadata are intentionally not supported here — the releases this +// tool pins to use plain semver. +var semverRe = regexp.MustCompile(`^v?\d+\.\d+\.\d+$`) + +// ValidateSemver returns nil if v is a valid X.Y.Z version (with or without a +// leading v) and an error otherwise. +func ValidateSemver(v string) error { + if v == "" { + return fmt.Errorf("version is empty") + } + if !semverRe.MatchString(v) { + return fmt.Errorf("invalid version %q: must be X.Y.Z (e.g. 0.25.0)", v) + } + return nil +} + // --- Version comparison --- func isNewerVersion(latest, current string) bool { diff --git a/internal/updater/updater_extra_test.go b/internal/updater/updater_extra_test.go index 92e7b1c..ca2258b 100644 --- a/internal/updater/updater_extra_test.go +++ b/internal/updater/updater_extra_test.go @@ -119,7 +119,7 @@ func TestFetchChecksums_NonOKResponse(t *testing.T) { }, nil })) - _, err := fetchChecksums(client) + _, err := fetchChecksums(client, "") require.Error(t, err) assert.Contains(t, err.Error(), "HTTP 500") } From e224e28f004ed8c26d703e71ce654aba2f3e4ee4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 12:46:08 +0000 Subject: [PATCH 2/6] refactor(brew): add RunInteractive and route upgrade/update through Runner The Runner interface now exposes RunInteractive for subcommands that need /dev/tty attached (sudo prompts for `brew upgrade`). The two direct exec.Command call sites in Update() and PreInstallChecks() are migrated to currentRunner().RunInteractive, closing the L1 test boundary for upgrade and index-refresh flows. ResolveFormulaNames now also goes through Runner.Output. The remaining exec.Command sites (brewInstallCmd and its callers) stay exempt: they need HOMEBREW_NO_AUTO_UPDATE env + custom stdout pipe wiring for StickyProgress streaming that Runner cannot express cleanly. A Runner-exempt comment now flags them. Adds runner_test.go with a recordingRunner that asserts routing and error propagation for both Run and RunInteractive. --- internal/brew/brew.go | 21 ++--- internal/brew/brew_command_test.go | 18 +++- internal/brew/brew_install.go | 9 +- internal/brew/runner.go | 32 ++++++- internal/brew/runner_test.go | 140 +++++++++++++++++++++++++++++ internal/sync/execute_test.go | 4 + 6 files changed, 200 insertions(+), 24 deletions(-) create mode 100644 internal/brew/runner_test.go diff --git a/internal/brew/brew.go b/internal/brew/brew.go index ad2a95a..b93ec3d 100644 --- a/internal/brew/brew.go +++ b/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...") } diff --git a/internal/brew/brew_command_test.go b/internal/brew/brew_command_test.go index ae4c30b..224c289 100644 --- a/internal/brew/brew_command_test.go +++ b/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") } diff --git a/internal/brew/brew_install.go b/internal/brew/brew_install.go index edb0b6d..76c4e4e 100644 --- a/internal/brew/brew_install.go +++ b/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) } diff --git a/internal/brew/runner.go b/internal/brew/runner.go index 8456699..973056a 100644 --- a/internal/brew/runner.go +++ b/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{} diff --git a/internal/brew/runner_test.go b/internal/brew/runner_test.go new file mode 100644 index 0000000..c64a0cd --- /dev/null +++ b/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()) +} diff --git a/internal/sync/execute_test.go b/internal/sync/execute_test.go index 37076f5..bef6199 100644 --- a/internal/sync/execute_test.go +++ b/internal/sync/execute_test.go @@ -32,6 +32,10 @@ func (f fakeBrewRunner) Run(args ...string) error { return f.err } +func (f fakeBrewRunner) RunInteractive(args ...string) error { + return f.err +} + // fakeNpmRunner is a test double for the npm.Runner interface. type fakeNpmRunner struct { err error From 83272ddd74b0324bb1ecb6de04b31bf36dfa7e76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 12:47:23 +0000 Subject: [PATCH 3/6] feat(logging): add always-on structured file logs under ~/.openboot/logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds internal/logging that wires slog.SetDefault to a multi-handler: captures all debug-level records to a daily rotating JSON file in ~/.openboot/logs/ (0700 dir, 0600 file), and mirrors records to stderr at LevelWarn by default (LevelDebug with --verbose). - Retention: openboot-YYYY-MM-DD.log files older than 14 days are pruned in a background goroutine on startup. Only openboot-*.log is touched. - Fallback: if the log dir or file cannot be opened, Init never errors — it reports once on stderr and keeps stderr-only logging. - Session marker: one session_start record per process (version, pid, args). Wires logging.Init in PersistentPreRunE and flushes via a deferred closer in Execute. Adds a minimal install_started / install_completed log pair in the installer to prove file-sink wiring end-to-end. Tests cover dir/file permissions, session_start emission, append (not truncate) across invocations, retention pruning (with injected clock), fallback on unwritable dirs, and verbose/non-verbose level wiring. --- internal/cli/root.go | 18 ++- internal/installer/installer.go | 10 ++ internal/logging/logging.go | 211 ++++++++++++++++++++++++++++ internal/logging/logging_test.go | 228 +++++++++++++++++++++++++++++++ 4 files changed, 464 insertions(+), 3 deletions(-) create mode 100644 internal/logging/logging.go create mode 100644 internal/logging/logging_test.go diff --git a/internal/cli/root.go b/internal/cli/root.go index 34d0c17..c60a654 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -3,7 +3,6 @@ package cli import ( "context" "fmt" - "log/slog" "os" "os/signal" "syscall" @@ -11,6 +10,7 @@ import ( "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 @@ -138,8 +142,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) } diff --git a/internal/installer/installer.go b/internal/installer/installer.go index 8d5f96b..e462ce1 100644 --- a/internal/installer/installer.go +++ b/internal/installer/installer.go @@ -3,6 +3,7 @@ package installer import ( "errors" "fmt" + "log/slog" "strings" "github.com/openbootdotdev/openboot/internal/brew" @@ -37,6 +38,13 @@ func Run(cfg *config.Config) error { } func runInstall(opts *config.InstallOptions, st *config.InstallState) error { + slog.Info("install_started", + "version", opts.Version, + "preset", opts.Preset, + "user", opts.User, + "dry_run", opts.DryRun, + "silent", opts.Silent, + ) fmt.Println() ui.Header(fmt.Sprintf("OpenBoot Installer v%s", opts.Version)) fmt.Println() @@ -127,8 +135,10 @@ func Apply(plan InstallPlan, r Reporter) error { showCompletionFromPlan(plan, r, len(softErrs)) if len(softErrs) > 0 { + slog.Info("install_completed", "soft_errors", len(softErrs)) return errors.Join(softErrs...) } + slog.Info("install_completed", "soft_errors", 0) return nil } diff --git a/internal/logging/logging.go b/internal/logging/logging.go new file mode 100644 index 0000000..a2216a9 --- /dev/null +++ b/internal/logging/logging.go @@ -0,0 +1,211 @@ +// Package logging provides always-on structured logging to a rotating file +// under ~/.openboot/logs/, independent of the --verbose flag (which controls +// stderr verbosity only). +package logging + +import ( + "context" + "fmt" + "log/slog" + "os" + "path/filepath" + "sort" + "strings" + "sync" + "time" +) + +// retentionDays is the number of days to keep log files before deleting them +// during startup cleanup. +const retentionDays = 14 + +// logDir resolves the directory where log files are stored. It is a package +// variable so tests can override it without touching the user's home dir. +var logDir = func() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolve home dir: %w", err) + } + return filepath.Join(home, ".openboot", "logs"), nil +} + +// now returns the current time. It is a package variable so tests can inject +// a deterministic clock. +var now = time.Now + +// cleanupWG lets tests deterministically wait for the background retention +// goroutine to finish before asserting on file state. +var cleanupWG sync.WaitGroup + +// fallbackReported guards the "once per process" stderr message we emit when +// the log file cannot be opened. It's a pointer so tests can reset it between +// runs. +var fallbackReported = &sync.Once{} + +// multiHandler fans out slog records to a set of child handlers, letting us +// capture every record to the log file while filtering stderr separately. +type multiHandler struct { + handlers []slog.Handler +} + +func newMultiHandler(handlers ...slog.Handler) *multiHandler { + return &multiHandler{handlers: handlers} +} + +func (m *multiHandler) Enabled(ctx context.Context, level slog.Level) bool { + for _, h := range m.handlers { + if h.Enabled(ctx, level) { + return true + } + } + return false +} + +func (m *multiHandler) Handle(ctx context.Context, r slog.Record) error { + var firstErr error + for _, h := range m.handlers { + if !h.Enabled(ctx, r.Level) { + continue + } + if err := h.Handle(ctx, r.Clone()); err != nil && firstErr == nil { + firstErr = err + } + } + return firstErr +} + +func (m *multiHandler) WithAttrs(attrs []slog.Attr) slog.Handler { + next := make([]slog.Handler, len(m.handlers)) + for i, h := range m.handlers { + next[i] = h.WithAttrs(attrs) + } + return &multiHandler{handlers: next} +} + +func (m *multiHandler) WithGroup(name string) slog.Handler { + next := make([]slog.Handler, len(m.handlers)) + for i, h := range m.handlers { + next[i] = h.WithGroup(name) + } + return &multiHandler{handlers: next} +} + +// Init configures slog.SetDefault to write to a multi-handler that always +// captures debug-level records to a daily log file under ~/.openboot/logs/, +// and mirrors records to stderr at LevelDebug (when verbose) or LevelWarn +// otherwise. It returns a closer that flushes and closes the log file. +// +// If the log file cannot be opened (permissions, read-only FS, etc.), Init +// falls back to stderr-only logging and reports the failure once. +func Init(version string, verbose bool) (func(), error) { + stderrLevel := slog.LevelWarn + if verbose { + stderrLevel = slog.LevelDebug + } + stderrHandler := slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: stderrLevel}) + + dir, dirErr := logDir() + var ( + file *os.File + openErr error + ) + if dirErr == nil { + file, openErr = openLogFile(dir) + } else { + openErr = dirErr + } + + var handler slog.Handler + closer := func() {} + if openErr != nil || file == nil { + handler = stderrHandler + reportFallback(openErr) + } else { + fileHandler := slog.NewJSONHandler(file, &slog.HandlerOptions{Level: slog.LevelDebug}) + handler = newMultiHandler(fileHandler, stderrHandler) + closer = func() { + _ = file.Sync() + _ = file.Close() + } + } + + slog.SetDefault(slog.New(handler)) + slog.Info("session_start", + "version", version, + "pid", os.Getpid(), + "args", os.Args, + ) + + if openErr == nil && file != nil { + cleanupWG.Add(1) + go func() { + defer cleanupWG.Done() + pruneOldLogs(dir, now()) + }() + } + + return closer, nil +} + +// WaitForCleanup blocks until any pending retention goroutines have finished. +// Exported for tests; production callers don't need to call this. +func WaitForCleanup() { + cleanupWG.Wait() +} + +// openLogFile ensures dir exists with 0700 and opens today's log file with +// 0600 in append mode. +func openLogFile(dir string) (*os.File, error) { + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("create log dir: %w", err) + } + // Tighten permissions in case dir already existed with a wider mode. + if err := os.Chmod(dir, 0o700); err != nil { + return nil, fmt.Errorf("chmod log dir: %w", err) + } + + name := fmt.Sprintf("openboot-%s.log", now().Format("2006-01-02")) + path := filepath.Join(dir, name) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) + if err != nil { + return nil, fmt.Errorf("open log file: %w", err) + } + return f, nil +} + +// pruneOldLogs removes openboot-*.log files whose mtime is older than +// retentionDays before ref. Errors are swallowed — retention is best-effort. +func pruneOldLogs(dir string, ref time.Time) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + cutoff := ref.Add(-retentionDays * 24 * time.Hour) + sort.SliceStable(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasPrefix(name, "openboot-") || !strings.HasSuffix(name, ".log") { + continue + } + full := filepath.Join(dir, name) + info, err := os.Stat(full) + if err != nil { + continue + } + if info.ModTime().Before(cutoff) { + _ = os.Remove(full) + } + } +} + +func reportFallback(err error) { + if err == nil { + return + } + fallbackReported.Do(func() { + fmt.Fprintf(os.Stderr, "openboot: file logging disabled, using stderr only: %v\n", err) + }) +} diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go new file mode 100644 index 0000000..4b64e11 --- /dev/null +++ b/internal/logging/logging_test.go @@ -0,0 +1,228 @@ +package logging + +import ( + "encoding/json" + "io/fs" + "log/slog" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// withStubs swaps the package-level logDir/now vars for the duration of a +// test, restoring the originals on cleanup. Tests must always use this to +// avoid touching the real user home dir. +func withStubs(t *testing.T, dir string, clock time.Time) { + t.Helper() + origDir := logDir + origNow := now + origOnce := fallbackReported + logDir = func() (string, error) { return dir, nil } + now = func() time.Time { return clock } + fallbackReported = &sync.Once{} + t.Cleanup(func() { + logDir = origDir + now = origNow + fallbackReported = origOnce + }) +} + +func TestInit_CreatesDirAndFile(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + clock := time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) + withStubs(t, dir, clock) + + closer, err := Init("1.2.3", false) + require.NoError(t, err) + defer closer() + WaitForCleanup() + + // Directory was created with 0700. + info, err := os.Stat(dir) + require.NoError(t, err) + require.True(t, info.IsDir()) + if runtime.GOOS != "windows" { + assert.Equal(t, fs.FileMode(0o700), info.Mode().Perm(), "log dir must be 0700") + } + + // File was created with 0600, named by the injected clock. + expected := filepath.Join(dir, "openboot-2026-04-19.log") + finfo, err := os.Stat(expected) + require.NoError(t, err, "expected log file %s", expected) + if runtime.GOOS != "windows" { + assert.Equal(t, fs.FileMode(0o600), finfo.Mode().Perm(), "log file must be 0600") + } +} + +func TestInit_EmitsSessionStart(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + clock := time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) + withStubs(t, dir, clock) + + closer, err := Init("9.9.9", false) + require.NoError(t, err) + closer() // flush and close before we read the file + WaitForCleanup() + + path := filepath.Join(dir, "openboot-2026-04-19.log") + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NotEmpty(t, data, "log file should contain at least session_start") + + // Each line must be valid JSON (we use JSONHandler for the file sink). + var found bool + for _, line := range strings.Split(strings.TrimSpace(string(data)), "\n") { + var rec map[string]any + require.NoError(t, json.Unmarshal([]byte(line), &rec), "line must be valid JSON: %s", line) + if rec["msg"] == "session_start" { + assert.Equal(t, "9.9.9", rec["version"]) + assert.NotNil(t, rec["pid"]) + assert.NotNil(t, rec["args"]) + found = true + } + } + assert.True(t, found, "session_start record must be present") +} + +func TestInit_AppendsToExistingFile(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + clock := time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) + withStubs(t, dir, clock) + + // First invocation. + closer1, err := Init("1.0.0", false) + require.NoError(t, err) + closer1() + WaitForCleanup() + + // Second invocation should append, not truncate. + closer2, err := Init("1.0.0", false) + require.NoError(t, err) + closer2() + WaitForCleanup() + + path := filepath.Join(dir, "openboot-2026-04-19.log") + data, err := os.ReadFile(path) + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + // Two session_start records. + count := 0 + for _, ln := range lines { + if strings.Contains(ln, "session_start") { + count++ + } + } + assert.Equal(t, 2, count, "both sessions should be appended") +} + +func TestInit_Retention(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + require.NoError(t, os.MkdirAll(dir, 0o700)) + + clock := time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) + withStubs(t, dir, clock) + + // Create three fake log files: one fresh (today's, by mtime) and two old. + freshName := "openboot-2026-04-15.log" // 4 days old — within retention + oldAName := "openboot-2026-04-01.log" // 18 days old + oldBName := "openboot-2026-03-20.log" // 30 days old + for _, name := range []string{freshName, oldAName, oldBName} { + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte("x"), 0o600)) + } + + // Set mtimes explicitly so retention logic is deterministic. + mustChtimes(t, filepath.Join(dir, freshName), clock.AddDate(0, 0, -4)) + mustChtimes(t, filepath.Join(dir, oldAName), clock.AddDate(0, 0, -18)) + mustChtimes(t, filepath.Join(dir, oldBName), clock.AddDate(0, 0, -30)) + + // Also drop an unrelated file — retention must ignore it. + unrelated := filepath.Join(dir, "other.txt") + require.NoError(t, os.WriteFile(unrelated, []byte("keep"), 0o600)) + mustChtimes(t, unrelated, clock.AddDate(0, 0, -60)) + + closer, err := Init("test", false) + require.NoError(t, err) + defer closer() + WaitForCleanup() + + // Today's log should exist after Init. + todayLog := filepath.Join(dir, "openboot-2026-04-19.log") + _, err = os.Stat(todayLog) + assert.NoError(t, err) + + // Fresh (4d old) log should still be there. + _, err = os.Stat(filepath.Join(dir, freshName)) + assert.NoError(t, err, "file within retention window must be kept") + + // Old logs should be deleted. + _, err = os.Stat(filepath.Join(dir, oldAName)) + assert.True(t, os.IsNotExist(err), "file older than retention must be deleted") + _, err = os.Stat(filepath.Join(dir, oldBName)) + assert.True(t, os.IsNotExist(err), "file older than retention must be deleted") + + // Unrelated file must survive. + _, err = os.Stat(unrelated) + assert.NoError(t, err, "retention must only touch openboot-*.log files") +} + +func TestInit_FallsBackWhenDirUnwritable(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file-as-directory trick is POSIX-only") + } + tmp := t.TempDir() + // Point logDir at a file, so MkdirAll + OpenFile both fail. + blocker := filepath.Join(tmp, "not-a-dir") + require.NoError(t, os.WriteFile(blocker, []byte("x"), 0o600)) + + origDir := logDir + origNow := now + origOnce := fallbackReported + logDir = func() (string, error) { return blocker, nil } + now = func() time.Time { return time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) } + fallbackReported = &sync.Once{} + t.Cleanup(func() { + logDir = origDir + now = origNow + fallbackReported = origOnce + }) + + closer, err := Init("x", false) + require.NoError(t, err, "Init must never return an error for fallback") + require.NotNil(t, closer) + closer() // must be safe to call + + // slog.Default should still produce output (to stderr) without panicking. + slog.Warn("fallback_ok", "k", "v") +} + +func TestInit_VerboseSetsStderrLevel(t *testing.T) { + // This test is mostly structural: verify Init accepts the verbose flag + // and returns a valid closer. Level wiring is inspected indirectly. + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + withStubs(t, dir, time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC)) + + for _, verbose := range []bool{false, true} { + closer, err := Init("v", verbose) + require.NoError(t, err) + closer() + WaitForCleanup() + } +} + +func mustChtimes(t *testing.T, path string, mtime time.Time) { + t.Helper() + require.NoError(t, os.Chtimes(path, mtime, mtime)) +} From db78535d4b19bf1afa4436d7b96e6c29b0b241b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 13:00:52 +0000 Subject: [PATCH 4/6] chore(logging): silence gosec G302 on log dir chmod The log dir needs the owner-exec bit (0700) to be traversable; gosec's G302 rule flags any Chmod mode above 0600 since it can't distinguish files from dirs. Follows the existing nolint pattern used elsewhere. --- internal/logging/logging.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index a2216a9..a18e138 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -160,7 +160,7 @@ func openLogFile(dir string) (*os.File, error) { return nil, fmt.Errorf("create log dir: %w", err) } // Tighten permissions in case dir already existed with a wider mode. - if err := os.Chmod(dir, 0o700); err != nil { + if err := os.Chmod(dir, 0o700); err != nil { //nolint:gosec // log dir needs owner-exec bit to be traversable return nil, fmt.Errorf("chmod log dir: %w", err) } From 144d663e1a2a7db067d2d528d89b92cdbb707eae Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 13:03:33 +0000 Subject: [PATCH 5/6] chore(logging): silence gosec G706 on session_start args os.Args is the audit subject of the session_start record by design. slog handlers quote attribute values, so there is no injection path into the surrounding log line. --- internal/logging/logging.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index a18e138..0236567 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -130,7 +130,10 @@ func Init(version string, verbose bool) (func(), error) { } slog.SetDefault(slog.New(handler)) - slog.Info("session_start", + // os.Args is user-provided by design — this is an audit record of how the + // CLI was invoked. slog's JSON/text handlers quote attribute values, so + // there is no injection path into the surrounding log line. + slog.Info("session_start", //nolint:gosec // G706: os.Args is the audit subject, not untrusted taint "version", version, "pid", os.Getpid(), "args", os.Args, From 1beda79df1edd17e06c9d50eb82a22ad97f46629 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Apr 2026 13:16:37 +0000 Subject: [PATCH 6/6] =?UTF-8?q?refactor:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?drop=20ForTesting=20API,=20add=20cli/update=20tests,=20redact?= =?UTF-8?q?=20args?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on self-review of PR #34: - updater: DownloadAndReplace now takes (targetVersion, currentVersion). Drops the SetCurrentVersionForTesting / currentBinaryVersionFn pair that leaked a test-named setter into production CLI code. - updater: exports TrimVersionPrefix; removes the duplicated trimV helper from internal/cli/update.go. - cli/update: new test seams (updateIsHomebrewInstall, updateGetLatestVersion, updateDownloadAndReplace, updateRollbackFn, updateListBackupsFn, updateGetBackupDir) let L1 cover the cobra subcommand without fork/exec. - cli/update_test.go (new): 19 cases covering mutex-flag validation, Homebrew-refuse branches (pin, rollback, latest) plus allowed list-backups, semver validation, dry-run paths (pin/latest/rollback with+without backups), delegation + error wrapping for download and rollback, and the runListBackups branches (empty / populated / list-error / dir-error). - logging: session_start now passes os.Args through RedactArgs, replacing the value of "--flag=value" entries whose flag name contains any of {token, password, secret, key, credential} (case-insensitive) with "". Space-separated form is a documented non-goal. - logging: adds a multiHandler WithAttrs/WithGroup test covering the fan-out paths, plus a table-driven RedactArgs test. --- internal/cli/update.go | 49 ++-- internal/cli/update_test.go | 391 +++++++++++++++++++++++++ internal/logging/logging.go | 40 ++- internal/logging/logging_test.go | 85 ++++++ internal/updater/backup_test.go | 21 +- internal/updater/updater.go | 65 ++-- internal/updater/updater_extra_test.go | 2 +- internal/updater/updater_test.go | 2 +- 8 files changed, 572 insertions(+), 83 deletions(-) create mode 100644 internal/cli/update_test.go diff --git a/internal/cli/update.go b/internal/cli/update.go index e9b31b6..2a18050 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -17,6 +17,16 @@ var ( updateDryRun bool ) +// Test seams — real implementations by default; tests replace via t.Cleanup. +var ( + updateIsHomebrewInstall = updater.IsHomebrewInstall + updateGetLatestVersion = updater.GetLatestVersion + updateDownloadAndReplace = updater.DownloadAndReplace + updateRollbackFn = updater.Rollback + updateListBackupsFn = updater.ListBackups + updateGetBackupDir = updater.GetBackupDir +) + var updateCmd = &cobra.Command{ Use: "update", Short: "Update OpenBoot, pin to a specific version, or roll back", @@ -76,11 +86,11 @@ func runUpdateCmd(_ *cobra.Command, _ []string) error { } func runListBackups() error { - dir, err := updater.GetBackupDir() + dir, err := updateGetBackupDir() if err != nil { return fmt.Errorf("resolve backup dir: %w", err) } - names, err := updater.ListBackups() + names, err := updateListBackupsFn() if err != nil { return fmt.Errorf("list backups: %w", err) } @@ -96,13 +106,13 @@ func runListBackups() error { } func runRollback() error { - if updater.IsHomebrewInstall() { + if updateIsHomebrewInstall() { ui.Warn("OpenBoot is managed by Homebrew — rollback is not supported.") ui.Muted("Use 'brew' commands to change versions.") return fmt.Errorf("rollback refused: Homebrew-managed install") } if updateDryRun { - names, err := updater.ListBackups() + names, err := updateListBackupsFn() if err != nil { return fmt.Errorf("list backups: %w", err) } @@ -113,7 +123,7 @@ func runRollback() error { ui.Info(fmt.Sprintf("Dry-run: would restore backup %s", names[0])) return nil } - if err := updater.Rollback(); err != nil { + if err := updateRollbackFn(); err != nil { return fmt.Errorf("rollback: %w", err) } ui.Success("Rollback complete.") @@ -124,29 +134,26 @@ func runPinnedUpgrade(v string) error { if err := updater.ValidateSemver(v); err != nil { return err } - if updater.IsHomebrewInstall() { + if updateIsHomebrewInstall() { ui.Warn("OpenBoot is managed by Homebrew — --version is not supported.") ui.Muted("Run 'brew upgrade openboot' to update via Homebrew.") return fmt.Errorf("version pin refused: Homebrew-managed install") } if updateDryRun { - ui.Info(fmt.Sprintf("Dry-run: would download and install OpenBoot v%s", trimV(v))) + ui.Info(fmt.Sprintf("Dry-run: would download and install OpenBoot v%s", updater.TrimVersionPrefix(v))) return nil } - ui.Info(fmt.Sprintf("Installing OpenBoot v%s...", trimV(v))) - // Let the backup record the running version. - updater.SetCurrentVersionForTesting(func() string { return version }) - defer updater.SetCurrentVersionForTesting(func() string { return "" }) + ui.Info(fmt.Sprintf("Installing OpenBoot v%s...", updater.TrimVersionPrefix(v))) - if err := updater.DownloadAndReplace(v); err != nil { + if err := updateDownloadAndReplace(v, version); err != nil { return fmt.Errorf("update failed: %w", err) } - ui.Success(fmt.Sprintf("Installed OpenBoot v%s.", trimV(v))) + ui.Success(fmt.Sprintf("Installed OpenBoot v%s.", updater.TrimVersionPrefix(v))) return nil } func runLatestUpgrade() error { - if updater.IsHomebrewInstall() { + if updateIsHomebrewInstall() { ui.Warn("OpenBoot is managed by Homebrew.") ui.Muted("Run 'brew upgrade openboot' to update.") return fmt.Errorf("use 'brew upgrade openboot'") @@ -155,25 +162,15 @@ func runLatestUpgrade() error { ui.Info("Dry-run: would check GitHub for the latest release and upgrade.") return nil } - latest, err := updater.GetLatestVersion() + latest, err := updateGetLatestVersion() if err != nil { return fmt.Errorf("look up latest version: %w", err) } ui.Info(fmt.Sprintf("Installing OpenBoot %s...", latest)) - updater.SetCurrentVersionForTesting(func() string { return version }) - defer updater.SetCurrentVersionForTesting(func() string { return "" }) - if err := updater.DownloadAndReplace(latest); err != nil { + if err := updateDownloadAndReplace(latest, version); err != nil { return fmt.Errorf("update failed: %w", err) } ui.Success(fmt.Sprintf("Installed OpenBoot %s.", latest)) return nil } - -// trimV strips an optional leading "v" for display. -func trimV(v string) string { - if len(v) > 0 && v[0] == 'v' { - return v[1:] - } - return v -} diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go new file mode 100644 index 0000000..139278b --- /dev/null +++ b/internal/cli/update_test.go @@ -0,0 +1,391 @@ +package cli + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// resetUpdateFlags restores the package-level update flag vars to their zero +// values. Call via t.Cleanup to isolate each test case. +func resetUpdateFlags(t *testing.T) { + t.Helper() + t.Cleanup(func() { + updateVersion = "" + updateRollback = false + updateListBackups = false + updateDryRun = false + }) +} + +// stubSeams swaps the update-command test seams, returning a restore func +// for t.Cleanup. All args are optional — pass nil to keep the real impl. +func stubSeams(t *testing.T, + isHomebrew func() bool, + getLatest func() (string, error), + download func(target, current string) error, + rollback func() error, + listBackups func() ([]string, error), + getBackupDir func() (string, error), +) { + t.Helper() + origHB := updateIsHomebrewInstall + origLatest := updateGetLatestVersion + origDL := updateDownloadAndReplace + origRB := updateRollbackFn + origLB := updateListBackupsFn + origBD := updateGetBackupDir + if isHomebrew != nil { + updateIsHomebrewInstall = isHomebrew + } + if getLatest != nil { + updateGetLatestVersion = getLatest + } + if download != nil { + updateDownloadAndReplace = download + } + if rollback != nil { + updateRollbackFn = rollback + } + if listBackups != nil { + updateListBackupsFn = listBackups + } + if getBackupDir != nil { + updateGetBackupDir = getBackupDir + } + t.Cleanup(func() { + updateIsHomebrewInstall = origHB + updateGetLatestVersion = origLatest + updateDownloadAndReplace = origDL + updateRollbackFn = origRB + updateListBackupsFn = origLB + updateGetBackupDir = origBD + }) +} + +// --------------------------------------------------------------------------- +// Mutex-flag validation +// --------------------------------------------------------------------------- + +func TestRunUpdateCmd_MutexFlags(t *testing.T) { + cases := []struct { + name string + version string + rollback bool + listBackups bool + wantErrSub string + expectRouted bool + }{ + {name: "version+rollback rejected", version: "1.2.3", rollback: true, wantErrSub: "mutually exclusive"}, + {name: "rollback+list rejected", rollback: true, listBackups: true, wantErrSub: "mutually exclusive"}, + {name: "version+list rejected", version: "1.2.3", listBackups: true, wantErrSub: "mutually exclusive"}, + {name: "all three rejected", version: "1.2.3", rollback: true, listBackups: true, wantErrSub: "mutually exclusive"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resetUpdateFlags(t) + updateVersion = tc.version + updateRollback = tc.rollback + updateListBackups = tc.listBackups + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrSub) + }) + } +} + +// --------------------------------------------------------------------------- +// Homebrew refusal +// --------------------------------------------------------------------------- + +func TestRunUpdate_HomebrewRefusesPin(t *testing.T) { + resetUpdateFlags(t) + updateVersion = "1.2.3" + stubSeams(t, func() bool { return true }, nil, nil, nil, nil, nil) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "Homebrew") +} + +func TestRunUpdate_HomebrewRefusesRollback(t *testing.T) { + resetUpdateFlags(t) + updateRollback = true + stubSeams(t, func() bool { return true }, nil, nil, nil, nil, nil) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "Homebrew") +} + +func TestRunUpdate_HomebrewRefusesLatest(t *testing.T) { + resetUpdateFlags(t) + stubSeams(t, func() bool { return true }, nil, nil, nil, nil, nil) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "brew upgrade openboot") +} + +func TestRunUpdate_HomebrewAllowsListBackups(t *testing.T) { + // --list-backups is read-only and should be allowed on Homebrew too. + resetUpdateFlags(t) + updateListBackups = true + stubSeams(t, func() bool { return true }, nil, nil, nil, + func() ([]string, error) { return nil, nil }, + func() (string, error) { return "/tmp/backup", nil }, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) +} + +// --------------------------------------------------------------------------- +// Semver validation +// --------------------------------------------------------------------------- + +func TestRunUpdate_InvalidSemverRejected(t *testing.T) { + resetUpdateFlags(t) + updateVersion = "not-a-version" + stubSeams(t, func() bool { return false }, nil, nil, nil, nil, nil) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid version") +} + +// --------------------------------------------------------------------------- +// Dry-run paths +// --------------------------------------------------------------------------- + +func TestRunUpdate_DryRunPin_DoesNotDownload(t *testing.T) { + resetUpdateFlags(t) + updateVersion = "1.2.3" + updateDryRun = true + called := false + stubSeams(t, + func() bool { return false }, + nil, + func(target, current string) error { called = true; return nil }, + nil, nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.False(t, called, "dry-run must not call DownloadAndReplace") +} + +func TestRunUpdate_DryRunLatest_DoesNotNetwork(t *testing.T) { + resetUpdateFlags(t) + updateDryRun = true + latestCalled := false + stubSeams(t, + func() bool { return false }, + func() (string, error) { latestCalled = true; return "1.0.0", nil }, + nil, nil, nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.False(t, latestCalled, "dry-run must not call GetLatestVersion") +} + +func TestRunUpdate_DryRunRollback_ReportsBackup(t *testing.T) { + resetUpdateFlags(t) + updateRollback = true + updateDryRun = true + rollbackCalled := false + stubSeams(t, + func() bool { return false }, + nil, nil, + func() error { rollbackCalled = true; return nil }, + func() ([]string, error) { return []string{"openboot-1.0.0-20260101T000000Z"}, nil }, + nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.False(t, rollbackCalled, "dry-run must not actually roll back") +} + +func TestRunUpdate_DryRunRollback_NoBackups(t *testing.T) { + resetUpdateFlags(t) + updateRollback = true + updateDryRun = true + stubSeams(t, + func() bool { return false }, + nil, nil, nil, + func() ([]string, error) { return nil, nil }, + nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) +} + +// --------------------------------------------------------------------------- +// Delegation to updater functions +// --------------------------------------------------------------------------- + +func TestRunUpdate_PinDelegatesToDownload(t *testing.T) { + resetUpdateFlags(t) + updateVersion = "1.2.3" + var gotTarget, gotCurrent string + stubSeams(t, + func() bool { return false }, + nil, + func(target, current string) error { + gotTarget = target + gotCurrent = current + return nil + }, + nil, nil, nil, + ) + // Seed the package-level `version` so runPinnedUpgrade forwards it. + origVersion := version + version = "0.9.0" + t.Cleanup(func() { version = origVersion }) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.Equal(t, "1.2.3", gotTarget) + assert.Equal(t, "0.9.0", gotCurrent, "current running version must be forwarded for backup labeling") +} + +func TestRunUpdate_LatestDelegatesToDownload(t *testing.T) { + resetUpdateFlags(t) + var gotTarget string + stubSeams(t, + func() bool { return false }, + func() (string, error) { return "2.0.0", nil }, + func(target, current string) error { gotTarget = target; return nil }, + nil, nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.Equal(t, "2.0.0", gotTarget) +} + +func TestRunUpdate_LatestSurfacesLookupError(t *testing.T) { + resetUpdateFlags(t) + stubSeams(t, + func() bool { return false }, + func() (string, error) { return "", errors.New("github unreachable") }, + nil, nil, nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "github unreachable") +} + +func TestRunUpdate_DownloadErrorWrapped(t *testing.T) { + resetUpdateFlags(t) + updateVersion = "1.2.3" + stubSeams(t, + func() bool { return false }, + nil, + func(target, current string) error { return errors.New("checksum mismatch") }, + nil, nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "update failed") + assert.Contains(t, err.Error(), "checksum mismatch") +} + +func TestRunUpdate_RollbackDelegates(t *testing.T) { + resetUpdateFlags(t) + updateRollback = true + called := false + stubSeams(t, + func() bool { return false }, + nil, nil, + func() error { called = true; return nil }, + nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) + assert.True(t, called) +} + +func TestRunUpdate_RollbackErrorWrapped(t *testing.T) { + resetUpdateFlags(t) + updateRollback = true + stubSeams(t, + func() bool { return false }, + nil, nil, + func() error { return errors.New("no backups found") }, + nil, nil, + ) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "rollback") + assert.Contains(t, err.Error(), "no backups found") +} + +// --------------------------------------------------------------------------- +// runListBackups +// --------------------------------------------------------------------------- + +func TestRunListBackups_EmptyDir(t *testing.T) { + resetUpdateFlags(t) + updateListBackups = true + stubSeams(t, nil, nil, nil, nil, + func() ([]string, error) { return nil, nil }, + func() (string, error) { return "/tmp/backup", nil }, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) +} + +func TestRunListBackups_WithEntries(t *testing.T) { + resetUpdateFlags(t) + updateListBackups = true + stubSeams(t, nil, nil, nil, nil, + func() ([]string, error) { + return []string{ + "openboot-1.0.0-20260101T000000Z", + "openboot-0.9.0-20251201T000000Z", + }, nil + }, + func() (string, error) { return "/tmp/backup", nil }, + ) + + err := runUpdateCmd(nil, nil) + require.NoError(t, err) +} + +func TestRunListBackups_ListError(t *testing.T) { + resetUpdateFlags(t) + updateListBackups = true + stubSeams(t, nil, nil, nil, nil, + func() ([]string, error) { return nil, errors.New("permission denied") }, + func() (string, error) { return "/tmp/backup", nil }, + ) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "list backups") + assert.Contains(t, err.Error(), "permission denied") +} + +func TestRunListBackups_BackupDirError(t *testing.T) { + resetUpdateFlags(t) + updateListBackups = true + stubSeams(t, nil, nil, nil, nil, nil, + func() (string, error) { return "", errors.New("home dir unavailable") }, + ) + + err := runUpdateCmd(nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolve backup dir") +} diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 0236567..0aa83c6 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -136,7 +136,7 @@ func Init(version string, verbose bool) (func(), error) { slog.Info("session_start", //nolint:gosec // G706: os.Args is the audit subject, not untrusted taint "version", version, "pid", os.Getpid(), - "args", os.Args, + "args", RedactArgs(os.Args), ) if openErr == nil && file != nil { @@ -204,6 +204,44 @@ func pruneOldLogs(dir string, ref time.Time) { } } +// sensitiveFlagFragments are substrings that, when found in a flag name, +// cause the flag's value to be redacted in the session_start audit record. +// Matching is case-insensitive and done on the flag name only (the part +// before '='); values of positional args are never redacted here. +var sensitiveFlagFragments = []string{"token", "password", "secret", "key", "credential"} + +// RedactArgs returns a copy of args with values of sensitive-looking flags +// replaced by "". Only the "--flag=value" inline form is redacted; +// the space-separated form ("--flag value") is left alone because the value +// at position i+1 has no flag context at this layer. This is a best-effort +// hedge, not a guarantee. +func RedactArgs(args []string) []string { + out := make([]string, len(args)) + for i, a := range args { + eq := strings.IndexByte(a, '=') + if eq <= 0 { + out[i] = a + continue + } + name := strings.ToLower(a[:eq]) + if isSensitiveFlagName(name) { + out[i] = a[:eq] + "=" + continue + } + out[i] = a + } + return out +} + +func isSensitiveFlagName(name string) bool { + for _, frag := range sensitiveFlagFragments { + if strings.Contains(name, frag) { + return true + } + } + return false +} + func reportFallback(err error) { if err == nil { return diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index 4b64e11..e1b35df 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -226,3 +226,88 @@ func mustChtimes(t *testing.T, path string, mtime time.Time) { t.Helper() require.NoError(t, os.Chtimes(path, mtime, mtime)) } + +func TestRedactArgs(t *testing.T) { + cases := []struct { + name string + in []string + want []string + }{ + { + name: "plain args untouched", + in: []string{"openboot", "install", "--preset=developer", "--dry-run"}, + want: []string{"openboot", "install", "--preset=developer", "--dry-run"}, + }, + { + name: "token flag redacted", + in: []string{"openboot", "--token=abc123"}, + want: []string{"openboot", "--token="}, + }, + { + name: "password flag redacted", + in: []string{"openboot", "--password=hunter2"}, + want: []string{"openboot", "--password="}, + }, + { + name: "case-insensitive match on fragment", + in: []string{"openboot", "--API-KEY=xyz", "--Secret-Token=abc"}, + want: []string{"openboot", "--API-KEY=", "--Secret-Token="}, + }, + { + name: "space-separated form is NOT redacted (known limitation)", + in: []string{"openboot", "--token", "abc123"}, + want: []string{"openboot", "--token", "abc123"}, + }, + { + name: "positional arg with = passes through when name has no sensitive fragment", + in: []string{"openboot", "package=latest"}, + want: []string{"openboot", "package=latest"}, + }, + { + name: "empty args", + in: []string{}, + want: []string{}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := RedactArgs(tc.in) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestMultiHandler_WithAttrsAndWithGroup(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, "logs") + clock := time.Date(2026, 4, 19, 10, 30, 0, 0, time.UTC) + withStubs(t, dir, clock) + + closer, err := Init("1.0.0", false) + require.NoError(t, err) + defer closer() + WaitForCleanup() + + // Exercise both WithAttrs and WithGroup — they must return a handler + // that still writes to every child sink (file + stderr in this case). + logger := slog.Default().With("component", "test").WithGroup("sub") + logger.Info("grouped_event", "k", "v") + // Also call a direct slog fn to make sure Enabled gate works across the + // fanned-out tree. + slog.Default().Debug("debug_should_reach_file_not_stderr") + + // Force any buffered writes to flush before we read the file. + closer() + + path := filepath.Join(dir, "openboot-2026-04-19.log") + data, err := os.ReadFile(path) + require.NoError(t, err) + text := string(data) + // The grouped record must appear with both the With attr and a grouped + // key. JSONHandler renders groups as nested objects; assert by substring + // so we don't need to unmarshal every line. + assert.Contains(t, text, `"component":"test"`, "WithAttrs attribute must reach file sink") + assert.Contains(t, text, `grouped_event`, "grouped record must appear in file") + assert.Contains(t, text, `"sub":{`, "WithGroup must namespace attributes") + assert.Contains(t, text, `debug_should_reach_file_not_stderr`, "file sink must accept Debug") +} diff --git a/internal/updater/backup_test.go b/internal/updater/backup_test.go index 392adbf..255c392 100644 --- a/internal/updater/backup_test.go +++ b/internal/updater/backup_test.go @@ -1,5 +1,5 @@ -// NOTE: tests in this file must NOT use t.Parallel() due to shared -// package-level variables (backupDirOverride, currentBinaryVersionFn). +// NOTE: tests in this file must NOT use t.Parallel() due to the shared +// package-level backupDirOverride variable. package updater import ( @@ -105,14 +105,10 @@ func TestBackupCurrentBinary_CreatesFile(t *testing.T) { SetBackupDirForTesting(backupDir) defer SetBackupDirForTesting("") - origFn := currentBinaryVersionFn - currentBinaryVersionFn = func() string { return "1.2.3" } - defer func() { currentBinaryVersionFn = origFn }() - bin := filepath.Join(tmp, "openboot") require.NoError(t, os.WriteFile(bin, []byte("fake binary"), 0755)) - require.NoError(t, backupCurrentBinary(bin)) + require.NoError(t, backupCurrentBinary(bin, "1.2.3")) entries, err := os.ReadDir(backupDir) require.NoError(t, err) @@ -137,13 +133,9 @@ func TestBackupCurrentBinary_UnknownVersionWhenBlank(t *testing.T) { SetBackupDirForTesting(backupDir) defer SetBackupDirForTesting("") - origFn := currentBinaryVersionFn - currentBinaryVersionFn = func() string { return "" } - defer func() { currentBinaryVersionFn = origFn }() - bin := filepath.Join(tmp, "openboot") require.NoError(t, os.WriteFile(bin, []byte("x"), 0755)) - require.NoError(t, backupCurrentBinary(bin)) + require.NoError(t, backupCurrentBinary(bin, "")) entries, err := os.ReadDir(backupDir) require.NoError(t, err) @@ -217,16 +209,13 @@ func TestBackupCurrentBinary_PrunesAfterLimit(t *testing.T) { backupDir := filepath.Join(tmp, "backup") SetBackupDirForTesting(backupDir) defer SetBackupDirForTesting("") - origFn := currentBinaryVersionFn - currentBinaryVersionFn = func() string { return "1.0.0" } - defer func() { currentBinaryVersionFn = origFn }() // Seed one-less-than-limit backups with old mtimes. seedBackups(t, backupDir, backupRetention) bin := filepath.Join(tmp, "openboot") require.NoError(t, os.WriteFile(bin, []byte("new"), 0755)) - require.NoError(t, backupCurrentBinary(bin)) + require.NoError(t, backupCurrentBinary(bin, "1.0.0")) entries, err := os.ReadDir(backupDir) require.NoError(t, err) diff --git a/internal/updater/updater.go b/internal/updater/updater.go index 3b4a6ca..fb1565f 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -171,8 +171,8 @@ func resolveLatestVersion(currentVersion string) string { } func notifyUpdate(currentVersion, latestVersion string) { - latestClean := trimVersionPrefix(latestVersion) - currentClean := trimVersionPrefix(currentVersion) + latestClean := TrimVersionPrefix(latestVersion) + currentClean := TrimVersionPrefix(currentVersion) ui.Warn(fmt.Sprintf("New version available: v%s (current: v%s)", latestClean, currentClean)) if IsHomebrewInstall() { ui.Muted("Run 'brew upgrade openboot' to upgrade") @@ -211,8 +211,8 @@ var execBrewUpgrade = func(formula string) error { } func doBrewUpgrade(currentVersion, latestVersion string) { - latestClean := trimVersionPrefix(latestVersion) - currentClean := trimVersionPrefix(currentVersion) + latestClean := TrimVersionPrefix(latestVersion) + currentClean := TrimVersionPrefix(currentVersion) ui.Info(fmt.Sprintf("Updating OpenBoot v%s → v%s via Homebrew...", currentClean, latestClean)) if err := execBrewUpgrade(brewFormula); err != nil { ui.Warn(fmt.Sprintf("Auto-update failed: %v", err)) @@ -228,14 +228,10 @@ func doBrewUpgrade(currentVersion, latestVersion string) { } func doDirectUpgrade(currentVersion, latestVersion string) { - latestClean := trimVersionPrefix(latestVersion) - currentClean := trimVersionPrefix(currentVersion) + latestClean := TrimVersionPrefix(latestVersion) + currentClean := TrimVersionPrefix(currentVersion) ui.Info(fmt.Sprintf("Updating OpenBoot v%s → v%s...", currentClean, latestClean)) - // Record the running version so backupCurrentBinary can label the backup. - prevFn := currentBinaryVersionFn - currentBinaryVersionFn = func() string { return currentVersion } - defer func() { currentBinaryVersionFn = prevFn }() - if err := DownloadAndReplace(latestVersion); err != nil { + if err := DownloadAndReplace(latestVersion, currentVersion); err != nil { ui.Warn(fmt.Sprintf("Auto-update failed: %v", err)) ui.Muted("Run 'openboot update --self' to update manually") fmt.Println() @@ -351,7 +347,7 @@ func checksumsURL(version string) string { if version == "" { return "https://github.com/openbootdotdev/openboot/releases/latest/download/checksums.txt" } - return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/checksums.txt", trimVersionPrefix(version)) + return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/checksums.txt", TrimVersionPrefix(version)) } // binaryURL returns the correct binary download URL for the given version and @@ -360,19 +356,21 @@ func binaryURL(version, filename string) string { if version == "" { return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/latest/download/%s", filename) } - return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/%s", trimVersionPrefix(version), filename) + return fmt.Sprintf("https://github.com/openbootdotdev/openboot/releases/download/v%s/%s", TrimVersionPrefix(version), filename) } -// DownloadAndReplace downloads the openboot binary for the given version and -// atomically replaces the currently-running binary. +// DownloadAndReplace downloads the openboot binary for the given target +// version and atomically replaces the currently-running binary. // -// If version is empty, the GitHub /latest/ redirect is used (back-compat with -// AutoUpgrade). If version is non-empty, the exact release URL is used. +// targetVersion controls which release to fetch: empty → GitHub /latest/ +// redirect (back-compat with AutoUpgrade), non-empty → exact release URL. +// currentVersion is recorded in the backup filename for rollback UX; it may +// be empty (renders as "unknown"). // // Before the atomic rename, the current binary is copied to the backup // directory (see GetBackupDir) for rollback via Rollback(). The newest // backupRetention backups are kept; older ones are pruned. -func DownloadAndReplace(version string) error { +func DownloadAndReplace(targetVersion, currentVersion string) error { if IsHomebrewInstall() { return fmt.Errorf("openboot is managed by Homebrew — run 'brew upgrade openboot' instead") } @@ -388,12 +386,12 @@ func DownloadAndReplace(version string) error { // Fetch checksums first so we can verify integrity before overwriting the // running binary. If the checksums file is missing or malformed, abort. - checksums, err := fetchChecksums(client, version) + checksums, err := fetchChecksums(client, targetVersion) if err != nil { return fmt.Errorf("verify update integrity: %w", err) } - url := binaryURL(version, filename) + url := binaryURL(targetVersion, filename) resp, err := client.Get(url) if err != nil { @@ -452,7 +450,7 @@ func DownloadAndReplace(version string) error { // fails, we still proceed — the user asked to update and integrity is // already verified. Warn via UI so the user knows rollback is unavailable // for this upgrade. - if err := backupCurrentBinary(binPath); err != nil { + if err := backupCurrentBinary(binPath, currentVersion); err != nil { ui.Warn(fmt.Sprintf("could not create backup (rollback unavailable for this upgrade): %v", err)) } @@ -474,10 +472,6 @@ var backupDirOverride string // pruned after each successful backup. const backupRetention = 5 -// currentBinaryVersion returns a version string to embed in backup filenames. -// Tests can override via currentBinaryVersionFn. -var currentBinaryVersionFn = func() string { return "" } - // GetBackupDir returns the directory where pre-upgrade binary backups are // stored. Defaults to ~/.openboot/backup; tests may override via // SetBackupDirForTesting. @@ -498,15 +492,10 @@ func SetBackupDirForTesting(dir string) { backupDirOverride = dir } -// SetCurrentVersionForTesting overrides the value used in backup filenames. -// When unset, backups use "unknown" as the version tag. -func SetCurrentVersionForTesting(fn func() string) { - currentBinaryVersionFn = fn -} - // backupCurrentBinary copies the binary at binPath into the backup directory -// with a timestamped filename and prunes old backups beyond backupRetention. -func backupCurrentBinary(binPath string) error { +// with a timestamped filename labeled by currentVersion (or "unknown" if +// empty), and prunes old backups beyond backupRetention. +func backupCurrentBinary(binPath, currentVersion string) error { dir, err := GetBackupDir() if err != nil { return err @@ -515,11 +504,11 @@ func backupCurrentBinary(binPath string) error { return fmt.Errorf("mkdir backup dir: %w", err) } - version := currentBinaryVersionFn() + version := currentVersion if version == "" { version = "unknown" } - version = trimVersionPrefix(version) + version = TrimVersionPrefix(version) ts := time.Now().UTC().Format("20060102T150405Z") dst := filepath.Join(dir, fmt.Sprintf("openboot-%s-%s", version, ts)) @@ -700,8 +689,8 @@ func isNewerVersion(latest, current string) bool { if current == "dev" { return false } - latestClean := trimVersionPrefix(latest) - currentClean := trimVersionPrefix(current) + latestClean := TrimVersionPrefix(latest) + currentClean := TrimVersionPrefix(current) return compareSemver(latestClean, currentClean) > 0 } @@ -734,7 +723,7 @@ func parseSemver(v string) [3]int { return result } -func trimVersionPrefix(v string) string { +func TrimVersionPrefix(v string) string { if len(v) > 0 && v[0] == 'v' { return v[1:] } diff --git a/internal/updater/updater_extra_test.go b/internal/updater/updater_extra_test.go index ca2258b..fc22054 100644 --- a/internal/updater/updater_extra_test.go +++ b/internal/updater/updater_extra_test.go @@ -214,7 +214,7 @@ func TestNotifyUpdate_DirectInstall(t *testing.T) { func TestNotifyUpdate_VersionPrefix_Stripped(t *testing.T) { // Verify that both "v"-prefixed and bare versions produce the same - // display string (regression guard for trimVersionPrefix). + // display string (regression guard for TrimVersionPrefix). // We just exercise the code path without network. notifyUpdate("v1.0.0", "v2.0.0") notifyUpdate("1.0.0", "2.0.0") diff --git a/internal/updater/updater_test.go b/internal/updater/updater_test.go index 44775e1..274be20 100644 --- a/internal/updater/updater_test.go +++ b/internal/updater/updater_test.go @@ -138,7 +138,7 @@ func TestTrimVersionPrefix(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, trimVersionPrefix(tt.input)) + assert.Equal(t, tt.expected, TrimVersionPrefix(tt.input)) }) } }