From e441f9405904529b35938c2e563f8a322e390d69 Mon Sep 17 00:00:00 2001 From: a3chron Date: Fri, 10 Jul 2026 22:25:45 +0200 Subject: [PATCH 01/12] feat: add Windows support via file-copy apply mode Windows handles symlinks poorly (they need Developer Mode or admin), so on Windows stellar now copies the theme file over starship.toml instead of symlinking. The mode is selected by runtime.GOOS and overridable via STELLAR_APPLY_MODE=copy|symlink for testing. - symlink: rename CreateSymlink -> ApplyTheme, add IsCopyMode() and a shared copyFile helper; back up the user's original config safely in copy mode (never overwrite an existing backup, so a lost config.json can't clobber it) - current: make verification mode-aware (only readlink in symlink mode) - update: unblock Windows, append .exe, download next to the binary, and do a Windows-safe self-replace; clean up the leftover .old on the next run - goreleaser: build windows/amd64 + windows/arm64 - installer: add install.ps1 and point install.sh's Windows guard at it - tests: copy-mode unit and e2e coverage, incl. the backup-clobber safety net Co-Authored-By: Claude Opus 4.8 (1M context) --- .goreleaser.yaml | 11 ++- README.md | 38 +++++++--- cmd/apply.go | 8 +- cmd/current.go | 39 ++++++---- cmd/e2e_test.go | 91 +++++++++++++++++++++++ cmd/rollback.go | 10 +-- cmd/root.go | 2 + cmd/update.go | 70 +++++++++++++++--- install.ps1 | 47 ++++++++++++ install.sh | 5 +- internal/paths/paths.go | 3 + internal/symlink/symlink.go | 122 ++++++++++++++++++++++--------- internal/symlink/symlink_test.go | 119 ++++++++++++++++++++++++++++-- 13 files changed, 475 insertions(+), 90 deletions(-) create mode 100644 install.ps1 diff --git a/.goreleaser.yaml b/.goreleaser.yaml index e8eb4f7..01386f8 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -15,7 +15,7 @@ builds: goos: - linux - darwin - # No windows - symlinks not supported well + - windows # Windows applies themes by copying instead of symlinking goarch: - amd64 - arm64 @@ -33,7 +33,7 @@ archives: name_template: "stellar-{{ .Os }}-{{ .Arch }}" files: - stellar - format: binary + formats: [binary] # Checksums checksum: @@ -69,9 +69,14 @@ release: ```bash # Update stellar stellar update - # Install stellar + # Install stellar (Linux / macOS) curl -fsSL https://raw.githubusercontent.com/a3chron/stellar/main/install.sh | bash ``` + + ```powershell + # Install stellar (Windows) + irm https://raw.githubusercontent.com/a3chron/stellar/main/install.ps1 | iex + ``` **Full Changelog**: https://github.com/a3chron/stellar/compare/{{ .PreviousTag }}...{{ .Tag }} diff --git a/README.md b/README.md index 887b98d..fcc1434 100644 --- a/README.md +++ b/README.md @@ -14,12 +14,22 @@ Easily get and switch between starship configs ## Installation +### Linux / macOS + Just run the [install script](https://raw.githubusercontent.com/a3chron/stellar/main/install.sh) (which will download the binary and move it to `~/.local/bin`) ```bash curl -fsSL https://raw.githubusercontent.com/a3chron/stellar/main/install.sh | bash ``` +### Windows + +Run the [PowerShell install script](https://raw.githubusercontent.com/a3chron/stellar/main/install.ps1) +(which downloads the binary to `%LOCALAPPDATA%\stellar\bin` and adds it to your PATH) +```powershell +irm https://raw.githubusercontent.com/a3chron/stellar/main/install.ps1 | iex +``` + Check that stellar is installed with `stellar --version` or `stellar --help`, and search for a theme you like on the [stellar hub](https://stellar-hub.vercel.app) to apply. @@ -30,15 +40,15 @@ Some [basic usage](#basic-usage) covered here, for more info, run `stellar --hel > [!NOTE] -> stellar is not yet available for windows, because stellar uses symlinks, and windows is weird with symlinks. -> Windows users would need to either: -> - Run it with admin privileges -> - Enable Developer Mode in Windows 10+ -> - (Use WSL -> not really "Windows") -> -> which is not optimal, and means we will have to do a special case for windows, which may take some time +> On Linux and macOS, stellar applies a theme by **symlinking** `~/.config/starship.toml` to the cached +> config, which gives you hot-reload while editing local configs. +> +> Windows handles symlinks poorly (they need Developer Mode or admin privileges), so on Windows stellar +> **copies** the theme file over `starship.toml` instead. Everything works the same, with one caveat: +> editing a local theme file does **not** live-update `starship.toml` (it's a copy, not a link) — just +> re-run `stellar apply /` after editing. > -> In the meantime, you can try out the [starship theme switcher](https://github.com/a3chron/starship-theme-switcher), the first version of stellar, with a lot of features missing, but should be able to run on anything at least. +> You can force copy mode anywhere (e.g. for testing) by setting `STELLAR_APPLY_MODE=copy`. ## Why use @@ -143,6 +153,10 @@ and then switch to it using `stellar apply ...`. Because stellar is using a symlink to the currently selected config file, you get hot-reload as well for editing configs, just like with the usual `starship.toml`. +> [!NOTE] +> On Windows stellar copies the config instead of symlinking it (see the [Windows note](#windows)), +> so editing a local theme file does **not** hot-reload — re-run `stellar apply /` after editing. + ## Troubleshooting ### "Theme not found online, using local cache" @@ -222,8 +236,13 @@ When adding new CLI features, please add corresponding E2E tests in `cmd/e2e_tes - [x] Allow removing several themes at once: `stellar remove a3chron/ctp-green a3chron/ctp-red` - [x] Preview: maybe cache in /tmp, os not downloading two times, but also not saving previewed themes in stellar cache +- [x] Add tests +- [x] **Windows support**: apply themes by copying instead of symlinking (`STELLAR_APPLY_MODE`), Windows release binary, PowerShell installer, and `stellar update` - [ ] **Preview: fix bash formatting** +- [ ] **`stellar preview` on Windows**: `cmd/preview.go` only spawns terminals on macOS/Linux and returns "unsupported platform" on Windows. Needs a Windows Terminal / PowerShell branch that opens a shell with `STARSHIP_CONFIG` set. +- [ ] **Windows packaging**: consider scoop/winget packaging (leftover `stellar.exe.old` from self-update is already cleaned up on the next run). +- [ ] **CI test job**: the release workflow runs no `go test` today; add one (ideally with a `windows-latest` runner) to guard the copy path natively. - [ ] **`stellar publish` command**: Upload local themes directly to stellar-hub - Challenge: Need to implement CLI authentication (OAuth flow with browser redirect or API keys) - Would read from `~/.config/stellar///.toml` @@ -233,8 +252,7 @@ When adding new CLI features, please add corresponding E2E tests in `cmd/e2e_tes - Requires authentication (same challenge as publish) - Upload new version of already published theme - Interactive prompts for version notes, dependencies, etc. -- [ ] Add progress bars for downloads? -- [x] Add tests +- [ ] Add progress bars for downloads - [ ] Get into nix pckgs
diff --git a/cmd/apply.go b/cmd/apply.go index d06df1d..c064a1b 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -174,14 +174,14 @@ var applyCmd = &cobra.Command{ return err } - // 5. Create symlink FIRST (before saving config) - // This ensures that if symlink fails, config remains unchanged - backupPath, err := symlink.CreateSymlink(themePath) + // 5. Apply the theme FIRST (before saving config) + // This ensures that if applying fails, config remains unchanged + backupPath, err := symlink.ApplyTheme(themePath) if err != nil { return err } - // 6. Update config only AFTER symlink succeeds + // 6. Update config only AFTER applying succeeds cfg.PreviousTheme = cfg.CurrentTheme cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = t.String() diff --git a/cmd/current.go b/cmd/current.go index a0c6301..88728ac 100644 --- a/cmd/current.go +++ b/cmd/current.go @@ -27,22 +27,36 @@ var currentCmd = &cobra.Command{ return nil } - // Verify symlink is still valid - target, err := symlink.GetCurrentTarget() - if err != nil { - color.Red("Symlink broken or missing") + starshipConfig, _ := symlink.StarshipConfigPath() + + // Verify the applied starship config is still present. Use Lstat so a + // dangling symlink still counts as present and falls through to the + // symlink-specific diagnostics below rather than "config missing". + if _, err := os.Lstat(starshipConfig); os.IsNotExist(err) { + color.Red("Starship config missing") fmt.Printf("Config says: %s\n", cfg.CurrentTheme) fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) return nil } - // Check if target exists - if _, err := os.Stat(target); os.IsNotExist(err) { - color.Red("Theme file missing") - fmt.Printf("Theme: %s\n", cfg.CurrentTheme) - fmt.Printf("Expected at: %s\n", cfg.CurrentPath) - fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) - return nil + // In symlink mode, also verify the link points at an existing theme file. + // In copy mode there is no link to read, so the check above is sufficient. + if !symlink.IsCopyMode() { + target, err := symlink.GetCurrentTarget() + if err != nil { + color.Red("Symlink broken or missing") + fmt.Printf("Config says: %s\n", cfg.CurrentTheme) + fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + return nil + } + + if _, err := os.Stat(target); os.IsNotExist(err) { + color.Red("Theme file missing") + fmt.Printf("Theme: %s\n", cfg.CurrentTheme) + fmt.Printf("Expected at: %s\n", cfg.CurrentPath) + fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) + return nil + } } // All good - display current theme @@ -52,8 +66,7 @@ var currentCmd = &cobra.Command{ fmt.Printf(" Path: %s\n", cfg.CurrentPath) fmt.Println() - // Show symlink info - starshipConfig, _ := symlink.StarshipConfigPath() + // Show starship config path fmt.Printf(" Starship config: %s\n", starshipConfig) return nil diff --git a/cmd/e2e_test.go b/cmd/e2e_test.go index 760d06e..d60427e 100644 --- a/cmd/e2e_test.go +++ b/cmd/e2e_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "testing" + "github.com/a3chron/stellar/internal/paths" "github.com/a3chron/stellar/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -55,6 +56,26 @@ func TestE2E_Apply(t *testing.T) { assert.Equal(t, themePath, env.ReadSymlink(env.StarshipPath)) }) + t.Run("Apply in copy mode (Windows behavior)", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + resetFlags() + + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + + err := cmd.Execute() + require.NoError(t, err) + + // In copy mode starship.toml is a regular file with the theme's content + assert.True(t, env.FileExists(env.StarshipPath)) + assert.False(t, env.IsSymlink(env.StarshipPath)) + assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) + }) + t.Run("Apply previewed theme from tmp", func(t *testing.T) { env := testutil.SetupTestEnv(t) resetFlags() @@ -316,6 +337,47 @@ func TestE2E_Current(t *testing.T) { require.NoError(t, err) }) + t.Run("Shows applied theme (copy mode)", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + // Copy mode: starship.toml is a regular file, not a symlink + env.CreateStarshipConfig(testutil.SampleTOML()) + + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + themePath + `" +}` + env.CreateConfig(config) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + + err := cmd.Execute() + require.NoError(t, err) + }) + + t.Run("Missing starship.toml in copy mode", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + // No starship.toml on disk, but config claims a theme is applied + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + env.StellarDir + `/alice/rainbow/1.0.toml" +}` + env.CreateConfig(config) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + + err := cmd.Execute() + assert.NoError(t, err) + }) + t.Run("Broken symlink", func(t *testing.T) { env := testutil.SetupTestEnv(t) @@ -542,6 +604,35 @@ func TestE2E_Rollback(t *testing.T) { assert.Equal(t, previousPath, env.ReadSymlink(env.StarshipPath)) }) + t.Run("Swaps current and previous (copy mode)", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + currentPath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + previousPath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) + // Copy mode: starship.toml is a regular file with the current theme's content + env.CreateStarshipConfig(testutil.SampleTOML()) + + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + currentPath + `", + "previous_theme": "bob/sunset@2.0", + "previous_path": "` + previousPath + `" +}` + env.CreateConfig(config) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"rollback"}) + cmd.SetOut(new(bytes.Buffer)) + + err := cmd.Execute() + require.NoError(t, err) + + // starship.toml now holds the previous theme's content, still a regular file + assert.False(t, env.IsSymlink(env.StarshipPath)) + assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) + }) + t.Run("Double rollback returns to original", func(t *testing.T) { env := testutil.SetupTestEnv(t) diff --git a/cmd/rollback.go b/cmd/rollback.go index e6e5f92..104dd7f 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -68,14 +68,14 @@ var rollbackCmd = &cobra.Command{ previousTheme := cfg.PreviousTheme previousPath := cfg.PreviousPath - // Update symlink FIRST (before modifying config) - // This ensures that if symlink fails, config remains unchanged - _, err = symlink.CreateSymlink(previousPath) + // Apply the previous theme FIRST (before modifying config) + // This ensures that if applying fails, config remains unchanged + _, err = symlink.ApplyTheme(previousPath) if err != nil { - return fmt.Errorf("failed to update symlink: %w", err) + return fmt.Errorf("failed to apply previous theme: %w", err) } - // Only swap config after symlink succeeds + // Only swap config after applying succeeds cfg.PreviousTheme = cfg.CurrentTheme cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = previousTheme diff --git a/cmd/root.go b/cmd/root.go index b1efa4f..0e1f4de 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -19,6 +19,8 @@ func NewRootCmd() *cobra.Command { Short: "Starship theme manager", Long: `Stellar - Discover, preview, and apply Starship themes from the community`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + // Best-effort cleanup of a binary left behind by a previous Windows self-update + cleanupOldExecutable() // Initialize stellar directory structure before any command runs return stellarinit.EnsureStellarDir() }, diff --git a/cmd/update.go b/cmd/update.go index 9c956a7..d76529b 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "os" + "path/filepath" "runtime" "strings" @@ -133,10 +134,10 @@ var updateCmd = &cobra.Command{ color.Yellow("Updating to version %s...", latestVersion) - // Construct binary name based on OS/arch + // Construct binary name based on OS/arch (matches goreleaser artifact names) binary := fmt.Sprintf("stellar-%s-%s", runtime.GOOS, runtime.GOARCH) if runtime.GOOS == "windows" { - return fmt.Errorf("why would you use windows? Anyways, stellar does not yet support windows, but support is planned") + binary += ".exe" } // Step 1: Fetch checksums.txt for verification @@ -168,8 +169,16 @@ var updateCmd = &cobra.Command{ return fmt.Errorf("download failed (status: %d)", resp.StatusCode) } - // Write to temporary file - tmpFile, err := os.CreateTemp("", "stellar-update-*") + // Resolve the running binary's path up front so the downloaded file can be + // written next to it. Keeping them on the same volume means the final + // rename works on all OSes (system temp may be on a different volume). + execPath, err := os.Executable() + if err != nil { + return err + } + + // Write to a temporary file in the same directory as the current binary + tmpFile, err := os.CreateTemp(filepath.Dir(execPath), ".stellar-update-*") if err != nil { return err } @@ -181,6 +190,9 @@ var updateCmd = &cobra.Command{ } if _, err := io.Copy(tmpFile, resp.Body); err != nil { + // Close first so the temp file isn't left with an open handle + // (which would block removal on Windows). + _ = tmpFile.Close() cleanup() return err } @@ -205,18 +217,12 @@ var updateCmd = &cobra.Command{ color.Green("Checksum verified successfully") // Step 6: Replace current binary (only after checksum verified) - execPath, err := os.Executable() - if err != nil { - cleanup() - return err - } - if err := os.Chmod(tmpPath, 0755); err != nil { cleanup() return err } - if err := os.Rename(tmpPath, execPath); err != nil { + if err := replaceExecutable(tmpPath, execPath); err != nil { cleanup() return err } @@ -226,3 +232,45 @@ var updateCmd = &cobra.Command{ return nil }, } + +// cleanupOldExecutable removes the ".old" binary left behind by a previous +// Windows self-update. On Windows the old binary can't be deleted while it's +// still running, so replaceExecutable leaves it in place; it is cleaned up here +// on the next stellar run (when a different binary is executing). No-op on other +// platforms, and safe to call even when nothing is left over. +func cleanupOldExecutable() { + if runtime.GOOS != "windows" { + return + } + execPath, err := os.Executable() + if err != nil { + return + } + _ = os.Remove(execPath + ".old") +} + +// replaceExecutable swaps the running binary at execPath for the file at newPath. +// +// On Unix a plain rename over the target works. On Windows a running .exe cannot +// be overwritten, but it can be renamed: move the current binary aside, put the +// new one in its place, then best-effort remove the old one (it stays locked +// until this process exits, so a failure there is fine — it's cleaned up on the +// next run). +func replaceExecutable(newPath, execPath string) error { + if runtime.GOOS != "windows" { + return os.Rename(newPath, execPath) + } + + oldPath := execPath + ".old" + _ = os.Remove(oldPath) // clear any leftover from a previous update + if err := os.Rename(execPath, oldPath); err != nil { + return err + } + if err := os.Rename(newPath, execPath); err != nil { + // Restore the original binary so the user isn't left without one + _ = os.Rename(oldPath, execPath) + return err + } + _ = os.Remove(oldPath) + return nil +} diff --git a/install.ps1 b/install.ps1 new file mode 100644 index 0000000..3845238 --- /dev/null +++ b/install.ps1 @@ -0,0 +1,47 @@ +#Requires -Version 5.1 +# Installer for stellar on Windows. +# Usage: irm https://raw.githubusercontent.com/a3chron/stellar/main/install.ps1 | iex + +$ErrorActionPreference = "Stop" + +# Configurable install directory (override with $env:STELLAR_INSTALL_DIR) +$BinDir = if ($env:STELLAR_INSTALL_DIR) { $env:STELLAR_INSTALL_DIR } else { Join-Path $env:LOCALAPPDATA "stellar\bin" } + +# Detect architecture +switch ($env:PROCESSOR_ARCHITECTURE) { + "AMD64" { $Arch = "amd64" } + "ARM64" { $Arch = "arm64" } + default { + Write-Error "Unsupported architecture: $($env:PROCESSOR_ARCHITECTURE)" + exit 1 + } +} + +$Binary = "stellar-windows-$Arch.exe" +$Url = "https://github.com/a3chron/stellar/releases/latest/download/$Binary" +$Target = Join-Path $BinDir "stellar.exe" + +Write-Host "Installing stellar for windows-$Arch" +Write-Host "Target directory: $BinDir" + +New-Item -ItemType Directory -Force -Path $BinDir | Out-Null + +Invoke-WebRequest -Uri $Url -OutFile $Target -UseBasicParsing + +Write-Host "stellar installed successfully!" + +# Add install dir to the user PATH if it isn't already there +$UserPath = [Environment]::GetEnvironmentVariable("Path", "User") +if ($UserPath -notlike "*$BinDir*") { + Write-Host "" + Write-Host "Adding $BinDir to your user PATH" + $NewPath = if ([string]::IsNullOrEmpty($UserPath)) { $BinDir } else { "$UserPath;$BinDir" } + [Environment]::SetEnvironmentVariable("Path", $NewPath, "User") + # Make it available in the current session too + $env:Path = "$env:Path;$BinDir" + Write-Host "Restart your terminal for the PATH change to take effect everywhere." +} + +Write-Host "" +Write-Host "Run:" +Write-Host " stellar --help" diff --git a/install.sh b/install.sh index 004901e..6d7c447 100644 --- a/install.sh +++ b/install.sh @@ -1,9 +1,10 @@ #!/usr/bin/env bash set -euo pipefail -# Check for Windows +# Check for Windows - use the PowerShell installer instead of this bash script if [[ "$OSTYPE" == "msys" || "$OSTYPE" == "cygwin" || "$OSTYPE" == "win32" ]]; then - echo "Windows is not supported yet." + echo "On Windows, install stellar with PowerShell instead:" + echo " irm https://raw.githubusercontent.com/a3chron/stellar/main/install.ps1 | iex" echo "For more information, see: https://github.com/a3chron/stellar#windows" exit 1 fi diff --git a/internal/paths/paths.go b/internal/paths/paths.go index d4c2e6a..42651be 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -17,6 +17,9 @@ const ( EnvTmpDir = "STELLAR_TMP_DIR" // EnvAPIURL overrides the API base URL EnvAPIURL = "STELLAR_API_URL" + // EnvApplyMode overrides how themes are applied ("symlink" or "copy"). + // Defaults to symlink on Unix and copy on Windows. + EnvApplyMode = "STELLAR_APPLY_MODE" ) // StellarHome returns the stellar configuration directory. diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index b214131..57faf9b 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -6,7 +6,10 @@ import ( "os" "os/user" "path/filepath" + "runtime" + "strings" + "github.com/a3chron/stellar/internal/config" "github.com/a3chron/stellar/internal/paths" ) @@ -14,6 +17,19 @@ func StarshipConfigPath() (string, error) { return paths.StarshipConfig() } +// IsCopyMode reports whether themes are applied by copying the file instead of +// symlinking. This is the default on Windows (where symlinks require Developer +// Mode or admin privileges) and can be forced anywhere via STELLAR_APPLY_MODE. +func IsCopyMode() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(paths.EnvApplyMode))) { + case "copy": + return true + case "symlink": + return false + } + return runtime.GOOS == "windows" +} + // isSymlink checks if the given path is a symlink func isSymlink(path string) bool { info, err := os.Lstat(path) @@ -23,15 +39,53 @@ func isSymlink(path string) bool { return info.Mode()&os.ModeSymlink != 0 } +// copyFile copies the contents of src to dst, creating or truncating dst. +func copyFile(src, dst string) (err error) { + source, err := os.Open(src) + if err != nil { + return fmt.Errorf("failed to open source file: %w", err) + } + defer func() { + if cerr := source.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close source file: %w", cerr) + } + }() + + destination, err := os.Create(dst) + if err != nil { + return fmt.Errorf("failed to create destination file: %w", err) + } + defer func() { + if cerr := destination.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close destination file: %w", cerr) + } + }() + + if _, err := io.Copy(destination, source); err != nil { + return fmt.Errorf("failed to copy file: %w", err) + } + + return nil +} + // backupOriginalConfig backs up the user's original starship.toml to ~/.config/stellar//backup/1.0.toml // Returns the backup path if successful, empty string otherwise func backupOriginalConfig(configPath string) (backupPath string, err error) { - // Check if the file exists and is NOT a symlink + // Check if the file exists if _, err := os.Stat(configPath); os.IsNotExist(err) { return "", nil // No file to back up } - if isSymlink(configPath) { + if IsCopyMode() { + // Copy mode leaves starship.toml as a regular file, so there's no + // symlink to distinguish a user's original config from one stellar + // copied in. Fall back to stellar's own state: if a theme is already + // applied, the current file is our copy and must not be backed up + // (doing so would clobber the real original backup). + if cfg, cerr := config.Load(); cerr == nil && cfg.CurrentTheme != "" { + return "", nil + } + } else if isSymlink(configPath) { return "", nil // Already a symlink, no need to back up } @@ -50,46 +104,38 @@ func backupOriginalConfig(configPath string) (backupPath string, err error) { backupDir := filepath.Join(stellarHome, currentUser.Username, "backup") backupPath = filepath.Join(backupDir, "1.0.toml") + // Never overwrite an existing backup: the first one holds the user's real + // original config. This is a safety net for copy mode, where a lost or reset + // config.json could otherwise make stellar mistake its own copy for the + // original and clobber the genuine backup. + if _, err := os.Stat(backupPath); err == nil { + return "", nil + } + // Create backup directory if err := os.MkdirAll(backupDir, 0755); err != nil { return "", fmt.Errorf("failed to create backup directory: %w", err) } // Copy the original file to backup location - source, err := os.Open(configPath) - if err != nil { - return "", fmt.Errorf("failed to open original config: %w", err) - } - defer func() { - if cerr := source.Close(); cerr != nil && err == nil { - err = fmt.Errorf("failed to close source file: %w", cerr) - } - }() - - destination, err := os.Create(backupPath) - if err != nil { - return "", fmt.Errorf("failed to create backup file: %w", err) - } - defer func() { - if cerr := destination.Close(); cerr != nil && err == nil { - err = fmt.Errorf("failed to close destination file: %w", cerr) - } - }() - - if _, err := io.Copy(destination, source); err != nil { - return "", fmt.Errorf("failed to copy config to backup: %w", err) + if err := copyFile(configPath, backupPath); err != nil { + return "", fmt.Errorf("failed to back up config: %w", err) } return backupPath, nil } -// CreateSymlink creates a symlink from ~/.config/starship.toml to the target file. +// ApplyTheme points ~/.config/starship.toml at the target theme file. +// On Unix a symlink is created; on Windows (or when STELLAR_APPLY_MODE=copy) the +// theme file is copied over starship.toml instead, since Windows handles symlinks +// poorly. +// // If an original (non-symlink) starship.toml exists, it's backed up first. // Returns the backup path if a backup was created (empty string if no backup was needed). // -// Uses atomic symlink replacement (temp-then-rename) to prevent data loss: -// If symlink creation fails, the original config remains intact. -func CreateSymlink(target string) (backupPath string, err error) { +// Uses atomic replacement (temp-then-rename) to prevent data loss: +// if applying fails, the original config remains intact. +func ApplyTheme(target string) (backupPath string, err error) { configPath, err := StarshipConfigPath() if err != nil { return "", err @@ -101,20 +147,25 @@ func CreateSymlink(target string) (backupPath string, err error) { return "", fmt.Errorf("failed to backup original config: %w", err) } - // Use atomic symlink replacement to avoid data loss window - // Strategy: Create temp symlink, then rename over target + // Use atomic replacement to avoid a data-loss window. + // Strategy: write the new symlink/copy to a temp path, then rename over target. configDir := filepath.Dir(configPath) tempPath := filepath.Join(configDir, ".starship.toml.stellar-tmp") // Remove any stale temp file from a previous failed attempt _ = os.Remove(tempPath) - // Create new symlink at temp location - if err := os.Symlink(target, tempPath); err != nil { - return backupPath, fmt.Errorf("failed to create symlink: %w", err) + if IsCopyMode() { + if err := copyFile(target, tempPath); err != nil { + return backupPath, fmt.Errorf("failed to copy theme: %w", err) + } + } else { + if err := os.Symlink(target, tempPath); err != nil { + return backupPath, fmt.Errorf("failed to create symlink: %w", err) + } } - // Atomic rename over the target (atomic on POSIX systems) + // Atomic rename over the target (replaces the destination on POSIX and Windows) // If this fails, the original file/symlink is still intact if err := os.Rename(tempPath, configPath); err != nil { // Clean up temp file on failure @@ -125,6 +176,9 @@ func CreateSymlink(target string) (backupPath string, err error) { return backupPath, nil } +// GetCurrentTarget returns the theme file that starship.toml points at. +// Only meaningful in symlink mode; in copy mode there is no link to read, so +// callers should gate this behind !IsCopyMode() and rely on config state instead. func GetCurrentTarget() (string, error) { configPath, err := StarshipConfigPath() if err != nil { diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index d828b15..9647567 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -6,17 +6,18 @@ import ( "path/filepath" "testing" + "github.com/a3chron/stellar/internal/paths" "github.com/a3chron/stellar/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestCreateSymlink_NoExistingFile(t *testing.T) { +func TestApplyTheme_NoExistingFile(t *testing.T) { env := testutil.SetupTestEnv(t) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := CreateSymlink(themePath) + backupPath, err := ApplyTheme(themePath) require.NoError(t, err) assert.Empty(t, backupPath, "should not create backup when no existing file") @@ -24,7 +25,7 @@ func TestCreateSymlink_NoExistingFile(t *testing.T) { assert.Equal(t, themePath, env.ReadSymlink(env.StarshipPath)) } -func TestCreateSymlink_ExistingRegularFile(t *testing.T) { +func TestApplyTheme_ExistingRegularFile(t *testing.T) { env := testutil.SetupTestEnv(t) originalContent := "# Original config\n[character]\nsuccess_symbol = \"OLD\"" @@ -32,7 +33,7 @@ func TestCreateSymlink_ExistingRegularFile(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := CreateSymlink(themePath) + backupPath, err := ApplyTheme(themePath) require.NoError(t, err) assert.NotEmpty(t, backupPath) @@ -48,15 +49,15 @@ func TestCreateSymlink_ExistingRegularFile(t *testing.T) { assert.Equal(t, themePath, env.ReadSymlink(env.StarshipPath)) } -func TestCreateSymlink_ExistingSymlink(t *testing.T) { +func TestApplyTheme_ExistingSymlink(t *testing.T) { env := testutil.SetupTestEnv(t) themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - _, err := CreateSymlink(themePath1) + _, err := ApplyTheme(themePath1) require.NoError(t, err) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - backupPath, err := CreateSymlink(themePath2) + backupPath, err := ApplyTheme(themePath2) require.NoError(t, err) assert.Empty(t, backupPath, "should not create backup when replacing symlink") @@ -65,11 +66,101 @@ func TestCreateSymlink_ExistingSymlink(t *testing.T) { assert.Equal(t, themePath2, env.ReadSymlink(env.StarshipPath)) } +func TestApplyTheme_CopyMode(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + assert.True(t, IsCopyMode()) + + themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + + backupPath, err := ApplyTheme(themePath) + require.NoError(t, err) + assert.Empty(t, backupPath, "should not create backup when no existing file") + + // In copy mode starship.toml is a regular file, not a symlink + assert.True(t, env.FileExists(env.StarshipPath)) + assert.False(t, env.IsSymlink(env.StarshipPath)) + assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) +} + +func TestApplyTheme_CopyMode_ExistingRegularFile(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + originalContent := "# Original config\n[character]\nsuccess_symbol = \"OLD\"" + env.CreateStarshipConfig(originalContent) + + themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + + backupPath, err := ApplyTheme(themePath) + require.NoError(t, err) + + // Original is backed up before being overwritten by the copy + assert.NotEmpty(t, backupPath) + assert.True(t, env.FileExists(backupPath)) + assert.Equal(t, originalContent, env.ReadFile(backupPath)) + + assert.False(t, env.IsSymlink(env.StarshipPath)) + assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) +} + +func TestApplyTheme_CopyMode_ReplacesPreviousCopy(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + _, err := ApplyTheme(themePath1) + require.NoError(t, err) + + // Simulate the config state that cmd/apply.go would have saved after the + // first apply, so the second apply knows the current file is stellar's copy. + env.CreateConfig(`{"current_theme":"alice/rainbow@1.0","current_path":"` + themePath1 + `"}`) + + themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) + backupPath, err := ApplyTheme(themePath2) + require.NoError(t, err) + + // A stellar-managed copy should not be backed up when replaced + assert.Empty(t, backupPath, "should not back up a config stellar already manages") + assert.False(t, env.IsSymlink(env.StarshipPath)) + assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) +} + +// TestApplyTheme_CopyMode_DoesNotClobberExistingBackup guards the safety net for +// the case where stellar's config state is lost (CurrentTheme == "") but the +// current starship.toml is actually a stellar copy. Without the guard, stellar +// would mistake its own copy for the user's original and overwrite the genuine +// backup. +func TestApplyTheme_CopyMode_DoesNotClobberExistingBackup(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + env := testutil.SetupTestEnv(t) + + // A real original backup already exists from an earlier apply. + currentUser, err := user.Current() + require.NoError(t, err) + backupPath := filepath.Join(env.StellarDir, currentUser.Username, "backup", "1.0.toml") + require.NoError(t, os.MkdirAll(filepath.Dir(backupPath), 0755)) + require.NoError(t, os.WriteFile(backupPath, []byte("# PRECIOUS ORIGINAL"), 0644)) + + // starship.toml holds a stellar copy, but there is no config.json state + // (CurrentTheme == ""), so the config-based marker can't help here. + env.CreateStarshipConfig("# stellar copy of some theme") + + themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + newBackup, err := ApplyTheme(themePath) + require.NoError(t, err) + + assert.Empty(t, newBackup, "should not create a new backup when one already exists") + assert.Equal(t, "# PRECIOUS ORIGINAL", env.ReadFile(backupPath), "existing backup must be preserved") + assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) +} + func TestGetCurrentTarget(t *testing.T) { env := testutil.SetupTestEnv(t) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - _, err := CreateSymlink(themePath) + _, err := ApplyTheme(themePath) require.NoError(t, err) target, err := GetCurrentTarget() @@ -84,6 +175,18 @@ func TestGetCurrentTarget_NoSymlink(t *testing.T) { assert.Error(t, err) } +func TestIsCopyMode(t *testing.T) { + t.Run("forced copy", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "copy") + assert.True(t, IsCopyMode()) + }) + + t.Run("forced symlink", func(t *testing.T) { + t.Setenv(paths.EnvApplyMode, "symlink") + assert.False(t, IsCopyMode()) + }) +} + func TestIsSymlink(t *testing.T) { env := testutil.SetupTestEnv(t) From aa182c8eff9d38fbfb3219900087e5054fe83b51 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sat, 11 Jul 2026 21:17:26 +0200 Subject: [PATCH 02/12] fix: harden copy-mode backups, current diagnostics, installer, and self-update Address code-review findings on the Windows support work: - record applied_hash in config.json so stellar recognizes its own applied file mode-independently; the documented edit-and-reapply workflow, clean --all, and apply-mode switches no longer create junk versioned backups (legacy configs without the hash keep the old content-match fallback) - stellar current: branch on the observed on-disk state (Lstat) instead of STELLAR_APPLY_MODE, restore the lost "theme file missing" diagnostic for copied configs, and surface path-resolution errors - stellar rollback: print the backup notice instead of silently discarding the backup path; share the notice helper with apply and derive the restore hint from the written path - install.ps1: verify the download against checksums.txt before replacing stellar.exe (temp-then-move), preserve REG_EXPAND_SZ when updating the user PATH, detect WoW64 hosts via PROCESSOR_ARCHITEW6432 - self-update: gate leftover cleanup to Windows, remove only stellar's own .old/.stellar-update-* artifacts, drop the unreachable removal of the running old binary - tests: pin STELLAR_APPLY_MODE=symlink in SetupTestEnv and skip symlink tests where unsupported so the suite runs on Windows; make CaptureOutput restore stdout on test failure; add regression tests for the backup, rollback, current, and cleanup behavior Co-Authored-By: Claude Fable 5 --- README.md | 19 +- cmd/apply.go | 30 +-- cmd/current.go | 47 +++-- cmd/e2e_test.go | 329 +++++++++++++++++++++++++++++-- cmd/rollback.go | 10 +- cmd/root.go | 4 +- cmd/update.go | 70 +++---- cmd/update_test.go | 54 +++++ install.ps1 | 97 ++++++++- internal/config/config.go | 7 + internal/symlink/symlink.go | 182 ++++++++++++++--- internal/symlink/symlink_test.go | 260 +++++++++++++++++++++--- internal/testutil/testutil.go | 81 ++++++++ 13 files changed, 1042 insertions(+), 148 deletions(-) create mode 100644 cmd/update_test.go diff --git a/README.md b/README.md index fcc1434..aec834a 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,12 @@ Some [basic usage](#basic-usage) covered here, for more info, run `stellar --hel > re-run `stellar apply /` after editing. > > You can force copy mode anywhere (e.g. for testing) by setting `STELLAR_APPLY_MODE=copy`. +> +> **Release note:** the backup folder name is now the sanitized OS username (e.g. `john.doe` -> `john-doe`, +> spaces and other characters `starship.toml`'s theme-identifier parser rejects are replaced with `-`), so a +> raw Windows `DOMAIN\user`-style name always produces a valid, restorable `stellar apply /backup` +> hint. Backups created before this change under the old, unsanitized folder name are left in place at +> their original path and are not migrated automatically. ## Why use @@ -122,11 +128,22 @@ with beeing able to update either metadata like the theme name, description, pre When you first use `stellar apply`, if you have an existing `~/.config/starship.toml` that's not managed by stellar, it will be automatically backed up to `~/.config/stellar//backup/1.0.toml` before creating the symlink. -This ensures your carefully crafted config is never lost :) You can apply it anytime with: +Backups are versioned. If stellar later finds another unmanaged `starship.toml` (for example one you restored or hand-wrote), it backs that up too as `2.0.toml`, then `3.0.toml`, and so on, so an earlier backup is never overwritten. + +This also works in copy mode (the Windows default): if you edit the applied `starship.toml` directly, stellar notices the file no longer matches the theme it applied and backs up your edits before applying the next theme. + +Stellar recognizes its own applied file by a checksum recorded in `~/.config/stellar/config.json`, independent of apply mode (symlink or copy) or OS. This means editing a cached theme file directly and re-applying it, or running `stellar clean`/`stellar clean --all` and then applying another theme, never creates a spurious backup — only a config file you actually hand-edited yourself gets preserved. + +This ensures your carefully crafted config is never lost :) You can apply the newest backup anytime with: ```bash stellar apply /backup ``` +To restore a specific one — for instance your very first original config — pin the version: +```bash +stellar apply /backup@1.0 +``` + You can also rename the backup folder to give it a proper theme name: ```bash mv ~/.config/stellar//backup ~/.config/stellar//my-custom-theme diff --git a/cmd/apply.go b/cmd/apply.go index c064a1b..dee7eac 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "os" - "os/user" "strings" "github.com/a3chron/stellar/internal/api" @@ -20,14 +19,6 @@ import ( var forceApply bool var updateTheme bool -func getCurrentUsername() string { - currentUser, err := user.Current() - if err != nil { - return "local" - } - return currentUser.Username -} - // promptConfirmation asks for user confirmation, defaults to No func promptConfirmation(prompt string) bool { reader := bufio.NewReader(os.Stdin) @@ -42,6 +33,15 @@ func promptConfirmation(prompt string) bool { return response == "y" || response == "yes" } +// printBackupNotice tells the user their original starship.toml was preserved +// and how to restore it. Shared by apply and rollback so both surface the +// same notice whenever backupOriginalConfig actually creates a backup. +func printBackupNotice(backupPath string) { + color.Yellow("Your original starship.toml has been backed up to:") + color.Yellow(" %s", backupPath) + color.Cyan("\nYou can apply it later with: stellar apply %s \n", symlink.BackupIdentifier(backupPath)) +} + var applyCmd = &cobra.Command{ Use: "apply [author/theme[@version]]", Short: "Apply a Starship theme", @@ -176,7 +176,7 @@ var applyCmd = &cobra.Command{ // 5. Apply the theme FIRST (before saving config) // This ensures that if applying fails, config remains unchanged - backupPath, err := symlink.ApplyTheme(themePath) + backupPath, err := symlink.ApplyTheme(themePath, cfg) if err != nil { return err } @@ -186,6 +186,12 @@ var applyCmd = &cobra.Command{ cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = t.String() cfg.CurrentPath = themePath + // Best-effort: record the hash of what was actually applied so future + // runs can recognize this exact file as stellar-managed regardless of + // apply mode. Empty on error (e.g. copy mode where a read races with + // something external); that just means a future run falls back to the + // legacy CurrentPath comparison instead of failing outright. + cfg.AppliedHash, _ = symlink.HashFile(themePath) if err := cfg.Save(); err != nil { // Symlink succeeded but config save failed @@ -195,9 +201,7 @@ var applyCmd = &cobra.Command{ // Notify user if their original config was backed up if backupPath != "" { - color.Yellow("Your original starship.toml has been backed up to:") - color.Yellow(" %s", backupPath) - color.Cyan("\nYou can apply it later with: stellar apply %s/backup \n", getCurrentUsername()) + printBackupNotice(backupPath) } color.Green("Applied %s", t) diff --git a/cmd/current.go b/cmd/current.go index 88728ac..e0e1470 100644 --- a/cmd/current.go +++ b/cmd/current.go @@ -27,23 +27,28 @@ var currentCmd = &cobra.Command{ return nil } - starshipConfig, _ := symlink.StarshipConfigPath() + starshipConfig, err := symlink.StarshipConfigPath() + if err != nil { + return fmt.Errorf("failed to resolve starship config path: %w", err) + } - // Verify the applied starship config is still present. Use Lstat so a - // dangling symlink still counts as present and falls through to the - // symlink-specific diagnostics below rather than "config missing". - if _, err := os.Lstat(starshipConfig); os.IsNotExist(err) { + // Branch on what's actually on disk (via a single Lstat), not on + // STELLAR_APPLY_MODE/GOOS: a copy-mode config inspected with + // STELLAR_APPLY_MODE=symlink (or vice versa) must still get the right + // diagnostics, since the env var only affects how a *future* apply + // behaves, not what's currently on disk. + info, statErr := os.Lstat(starshipConfig) + switch { + case os.IsNotExist(statErr): color.Red("Starship config missing") fmt.Printf("Config says: %s\n", cfg.CurrentTheme) fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) return nil - } - // In symlink mode, also verify the link points at an existing theme file. - // In copy mode there is no link to read, so the check above is sufficient. - if !symlink.IsCopyMode() { - target, err := symlink.GetCurrentTarget() - if err != nil { + case statErr == nil && info.Mode()&os.ModeSymlink != 0: + // Symlink mode. Verify the link points at an existing theme file. + target, terr := symlink.GetCurrentTarget() + if terr != nil { color.Red("Symlink broken or missing") fmt.Printf("Config says: %s\n", cfg.CurrentTheme) fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) @@ -57,6 +62,26 @@ var currentCmd = &cobra.Command{ fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) return nil } + + case statErr == nil: + // Regular file (copy mode, or a symlink-mode config someone + // hand-edited into a plain file). There's no link to read, so the + // only diagnostic available is whether the cached theme file + // we'd re-download from is still around. + if cfg.CurrentPath != "" { + if _, err := os.Stat(cfg.CurrentPath); os.IsNotExist(err) { + color.Red("Theme file missing") + fmt.Printf("Theme: %s\n", cfg.CurrentTheme) + fmt.Printf("Cached theme file missing, expected at: %s\n", cfg.CurrentPath) + fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) + return nil + } + } + + default: + // Some other Lstat error (e.g. permission denied). Surface it + // rather than silently reporting a healthy state. + return fmt.Errorf("failed to inspect starship config: %w", statErr) } // All good - display current theme diff --git a/cmd/e2e_test.go b/cmd/e2e_test.go index d60427e..ae8b227 100644 --- a/cmd/e2e_test.go +++ b/cmd/e2e_test.go @@ -6,10 +6,13 @@ import ( "bytes" "os" "path/filepath" + "strings" "testing" "github.com/a3chron/stellar/internal/paths" + "github.com/a3chron/stellar/internal/symlink" "github.com/a3chron/stellar/internal/testutil" + "github.com/a3chron/stellar/internal/theme" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -20,6 +23,7 @@ import ( func TestE2E_Apply(t *testing.T) { t.Run("Download and apply remote theme", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -40,6 +44,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply cached theme", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -57,8 +62,8 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply in copy mode (Windows behavior)", func(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") resetFlags() env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) @@ -76,6 +81,188 @@ func TestE2E_Apply(t *testing.T) { assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) }) + t.Run("Backup hint is a valid identifier (copy mode)", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + // A pre-existing, unmanaged starship.toml that will be backed up. + env.CreateStarshipConfig("# my hand-written original config") + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + // Find the printed restore hint and pull the identifier out of it. + var hint string + for _, line := range strings.Split(output, "\n") { + if strings.Contains(line, "stellar apply ") { + hint = line + break + } + } + require.NotEmpty(t, hint, "expected a backup restore hint in output:\n%s", output) + + idx := strings.Index(hint, "stellar apply ") + identifier := strings.TrimSpace(hint[idx+len("stellar apply "):]) + + // The regression: on Windows a raw "DOMAIN\user/backup" hint would not + // parse. The sanitized identifier must both parse and target the backup. + parsed, perr := theme.ParseIdentifier(identifier) + require.NoError(t, perr, "backup hint %q must be a valid identifier", identifier) + assert.True(t, strings.HasSuffix(parsed.String(), "/backup@1.0"), + "backup hint %q should target /backup@1.0", identifier) + }) + + t.Run("Hand-edited config is backed up (copy mode)", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + env.CreateThemeFile("local", "other", "1.0", testutil.SampleTOMLWithCustom()) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cmd.Execute()) + + // The user edits the applied copy directly, then applies another theme. + editedContent := "# MY HAND-EDITED CONFIG" + env.CreateStarshipConfig(editedContent) + resetFlags() + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/other@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + // The edit was detected and preserved as a backup, and the user was told. + assert.Contains(t, output, "has been backed up") + backupPath := filepath.Join(env.StellarDir, symlink.BackupAuthor(), "backup", "1.0.toml") + assert.Equal(t, editedContent, env.ReadFile(backupPath)) + assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) + }) + + t.Run("Editing cached theme file then re-applying creates no backup (copy mode)", func(t *testing.T) { + // Regression for finding #1: the README-documented workflow of editing + // a cached theme file directly and re-applying it (for a copy-mode + // hot-reload equivalent) must not be mistaken for a hand-edited + // starship.toml. + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + themePath := env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cmd.Execute()) + + // The user edits the cached theme file itself, not starship.toml. + editedThemeContent := "# EDITED CACHED THEME\nformat = \"$all\"\n" + require.NoError(t, os.WriteFile(themePath, []byte(editedThemeContent), 0644)) + resetFlags() + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + assert.NotContains(t, output, "backed up", "re-applying an edited cached theme must not trigger a backup") + assert.Equal(t, editedThemeContent, env.ReadFile(env.StarshipPath)) + }) + + t.Run("Clean --all then applying another theme creates no backup (copy mode)", func(t *testing.T) { + // Regression for finding #1: "stellar clean --all" removes the cached + // theme file that stellar's copy came from, but the copy on disk (and + // its recorded applied_hash) don't change, so it must still be + // recognized as stellar's own file. + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cmd.Execute()) + resetFlags() + + cleanRunner := NewRootCmd() + cleanRunner.SetArgs([]string{"clean", "--all"}) + cleanRunner.SetOut(new(bytes.Buffer)) + require.NoError(t, cleanRunner.Execute()) + resetFlags() + + // The next theme becomes available only after the clean (e.g. a fresh + // download); what matters for this regression is that stellar's own + // existing copy on disk is still recognized despite its cache source + // being gone. + env.CreateThemeFile("local", "other", "1.0", testutil.SampleTOMLWithCustom()) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/other@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + assert.NotContains(t, output, "backed up", "clean --all followed by apply must not trigger a junk backup") + assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) + }) + + t.Run("Switching apply mode copy to symlink creates no backup", func(t *testing.T) { + // Regression for finding #2: the managed-file check must not be + // mode-gated. Applying in copy mode, then applying again in symlink + // mode, must recognize the copy-mode file via applied_hash instead of + // backing it up as if it were a foreign original. + testutil.RequireSymlinks(t) + env := testutil.SetupTestEnv(t) + resetFlags() + + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + env.CreateThemeFile("local", "other", "1.0", testutil.SampleTOMLWithCustom()) + + t.Setenv(paths.EnvApplyMode, "copy") + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cmd.Execute()) + resetFlags() + + t.Setenv(paths.EnvApplyMode, "symlink") + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"apply", "local/other@1.0"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + assert.NotContains(t, output, "backed up", "mode switch must not trigger a junk backup") + assert.True(t, env.IsSymlink(env.StarshipPath)) + }) + t.Run("Apply previewed theme from tmp", func(t *testing.T) { env := testutil.SetupTestEnv(t) resetFlags() @@ -99,6 +286,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply with update flag", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -120,6 +308,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply uses local cache without update", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -141,6 +330,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply specific version", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -160,6 +350,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply backs up original config", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -191,6 +382,7 @@ func TestE2E_Apply(t *testing.T) { }) t.Run("Apply updates config file", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) resetFlags() @@ -206,6 +398,7 @@ func TestE2E_Apply(t *testing.T) { configContent := env.ReadFile(filepath.Join(env.StellarDir, "config.json")) assert.Contains(t, configContent, "local/mytheme@1.0") assert.Contains(t, configContent, themePath) + assert.Contains(t, configContent, "applied_hash", "config should record the hash of the applied theme") }) t.Run("Apply nonexistent theme errors", func(t *testing.T) { @@ -318,6 +511,7 @@ func TestE2E_Current(t *testing.T) { }) t.Run("Shows applied theme", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) @@ -338,8 +532,8 @@ func TestE2E_Current(t *testing.T) { }) t.Run("Shows applied theme (copy mode)", func(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) // Copy mode: starship.toml is a regular file, not a symlink @@ -351,17 +545,20 @@ func TestE2E_Current(t *testing.T) { }` env.CreateConfig(config) - cmd := NewRootCmd() - cmd.SetArgs([]string{"current"}) - cmd.SetOut(new(bytes.Buffer)) - - err := cmd.Execute() - require.NoError(t, err) + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "alice/rainbow@1.0") }) t.Run("Missing starship.toml in copy mode", func(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") // No starship.toml on disk, but config claims a theme is applied config := `{ @@ -370,15 +567,78 @@ func TestE2E_Current(t *testing.T) { }` env.CreateConfig(config) - cmd := NewRootCmd() - cmd.SetArgs([]string{"current"}) - cmd.SetOut(new(bytes.Buffer)) + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + assert.NoError(t, execErr) + assert.Contains(t, output, "Starship config missing") + assert.Contains(t, output, "stellar apply alice/rainbow@1.0") + }) - err := cmd.Execute() - assert.NoError(t, err) + t.Run("Copy mode with cached theme file deleted reports missing", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + // starship.toml (the copy) is present and healthy, but the cached + // theme file it was copied from has since been removed (e.g. via + // "stellar clean --all"). current.go has no symlink to follow in + // copy mode, so cfg.CurrentPath is the only thing it can check. + env.CreateStarshipConfig(testutil.SampleTOML()) + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + env.StellarDir + `/alice/rainbow/1.0.toml" +}` + env.CreateConfig(config) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + assert.NoError(t, execErr) + assert.Contains(t, output, "Theme file missing") + assert.Contains(t, output, "Cached theme file missing", "wording should acknowledge the file is a standalone copy") + assert.Contains(t, output, "stellar apply alice/rainbow@1.0") + }) + + t.Run("Copy-applied config inspected with mode=symlink env still reports healthy", func(t *testing.T) { + // Regression for the mode-switch misclassification bug: current.go + // must branch on what's actually on disk (a regular file here), not + // on STELLAR_APPLY_MODE, which only describes how a *future* apply + // would behave. + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "symlink") + + themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + env.CreateStarshipConfig(testutil.SampleTOML()) + + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + themePath + `" +}` + env.CreateConfig(config) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "Current Theme") + assert.NotContains(t, output, "missing") + assert.NotContains(t, output, "broken") }) t.Run("Broken symlink", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) require.NoError(t, os.Symlink("/nonexistent/path.toml", env.StarshipPath)) @@ -579,6 +839,7 @@ func TestE2E_Rollback(t *testing.T) { }) t.Run("Swaps current and previous", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) currentPath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) @@ -605,8 +866,8 @@ func TestE2E_Rollback(t *testing.T) { }) t.Run("Swaps current and previous (copy mode)", func(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") currentPath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) previousPath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) @@ -633,7 +894,44 @@ func TestE2E_Rollback(t *testing.T) { assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) }) + t.Run("Rollback backup notice (copy mode, hand-edited config)", func(t *testing.T) { + // Regression for the silent-rollback-backup bug: rollback must print + // the same backup notice apply does whenever backupPath != "". + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + currentPath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + previousPath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) + + // The user hand-edited the applied copy; config has no applied_hash + // (fabricated directly), so this can't be recognized as stellar's own. + editedContent := "# MY HAND-EDITED CONFIG BEFORE ROLLBACK" + env.CreateStarshipConfig(editedContent) + + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + currentPath + `", + "previous_theme": "bob/sunset@2.0", + "previous_path": "` + previousPath + `" +}` + env.CreateConfig(config) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"rollback"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + + assert.Contains(t, output, "has been backed up") + backupPath := filepath.Join(env.StellarDir, symlink.BackupAuthor(), "backup", "1.0.toml") + assert.Equal(t, editedContent, env.ReadFile(backupPath)) + }) + t.Run("Double rollback returns to original", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) currentPath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) @@ -660,6 +958,7 @@ func TestE2E_Rollback(t *testing.T) { }) t.Run("Redownloads missing theme", func(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) mockAPI := testutil.CreateDefaultMockAPI() diff --git a/cmd/rollback.go b/cmd/rollback.go index 104dd7f..36c5ccd 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -70,7 +70,7 @@ var rollbackCmd = &cobra.Command{ // Apply the previous theme FIRST (before modifying config) // This ensures that if applying fails, config remains unchanged - _, err = symlink.ApplyTheme(previousPath) + backupPath, err := symlink.ApplyTheme(previousPath, cfg) if err != nil { return fmt.Errorf("failed to apply previous theme: %w", err) } @@ -80,6 +80,9 @@ var rollbackCmd = &cobra.Command{ cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = previousTheme cfg.CurrentPath = previousPath + // Best-effort, same as apply: record what we just wrote so future runs + // recognize this file as stellar-managed. + cfg.AppliedHash, _ = symlink.HashFile(previousPath) // Save config if err := cfg.Save(); err != nil { @@ -88,6 +91,11 @@ var rollbackCmd = &cobra.Command{ return fmt.Errorf("rollback applied but failed to save config: %w", err) } + // Notify user if their original config was backed up (previously silent) + if backupPath != "" { + printBackupNotice(backupPath) + } + color.Green("Rolled back to: %s", cfg.CurrentTheme) return nil diff --git a/cmd/root.go b/cmd/root.go index 0e1f4de..4d821db 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -19,8 +19,8 @@ func NewRootCmd() *cobra.Command { Short: "Starship theme manager", Long: `Stellar - Discover, preview, and apply Starship themes from the community`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - // Best-effort cleanup of a binary left behind by a previous Windows self-update - cleanupOldExecutable() + // Best-effort cleanup of files left behind by a previous self-update + cleanupUpdateLeftovers() // Initialize stellar directory structure before any command runs return stellarinit.EnsureStellarDir() }, diff --git a/cmd/update.go b/cmd/update.go index d76529b..748f5a8 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -2,8 +2,6 @@ package cmd import ( "bufio" - "crypto/sha256" - "encoding/hex" "fmt" "io" "net/http" @@ -12,6 +10,7 @@ import ( "runtime" "strings" + "github.com/a3chron/stellar/internal/symlink" "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -80,26 +79,6 @@ func parseChecksum(checksums, binaryName string) (string, error) { return "", fmt.Errorf("checksum not found for binary: %s", binaryName) } -// computeFileHash computes the SHA256 hash of a file and returns it as a hex string -func computeFileHash(filePath string) (hash string, err error) { - f, err := os.Open(filePath) - if err != nil { - return "", fmt.Errorf("failed to open file for hashing: %w", err) - } - defer func() { - if cerr := f.Close(); cerr != nil && err == nil { - err = cerr - } - }() - - h := sha256.New() - if _, err := io.Copy(h, f); err != nil { - return "", fmt.Errorf("failed to hash file: %w", err) - } - - return hex.EncodeToString(h.Sum(nil)), nil -} - // verifyChecksum compares expected and actual checksums func verifyChecksum(expected, actual, binaryName string) error { expected = strings.ToLower(strings.TrimSpace(expected)) @@ -203,7 +182,7 @@ var updateCmd = &cobra.Command{ // Step 4: Compute hash of downloaded file color.Yellow("Verifying checksum...") - actualHash, err := computeFileHash(tmpPath) + actualHash, err := symlink.HashFile(tmpPath) if err != nil { cleanup() return fmt.Errorf("failed to compute checksum: %w", err) @@ -233,29 +212,55 @@ var updateCmd = &cobra.Command{ }, } -// cleanupOldExecutable removes the ".old" binary left behind by a previous -// Windows self-update. On Windows the old binary can't be deleted while it's -// still running, so replaceExecutable leaves it in place; it is cleaned up here -// on the next stellar run (when a different binary is executing). No-op on other -// platforms, and safe to call even when nothing is left over. -func cleanupOldExecutable() { +// cleanupUpdateLeftovers is the PersistentPreRunE gate: it only does anything +// on Windows, since that's the only platform replaceExecutable leaves a ".old" +// binary behind on (a running .exe can't be deleted, only renamed out of the +// way) and the only platform a ".stellar-update-*" temp file can survive a +// crashed update (Unix error paths already remove their own temp file). Gating +// here — rather than inside removeUpdateLeftovers — keeps the glob/remove logic +// itself trivially testable on any OS. +func cleanupUpdateLeftovers() { if runtime.GOOS != "windows" { return } + execPath, err := os.Executable() if err != nil { return } + removeUpdateLeftovers(execPath) +} + +// removeUpdateLeftovers deletes exactly this binary's ".old" leftover and any +// ".stellar-update-*" temp files next to it. Only stellar's own artifacts are +// touched — the install dir may be a shared bin directory (STELLAR_INSTALL_DIR), +// so a broad "*.old" glob is off-limits. Every removal is best-effort and safe +// to call when nothing is left over. +// +// This runs from PersistentPreRunE, before the update command creates its own +// temp file, so it never deletes an in-progress download of the current +// process. A concurrent "stellar update" in another process could theoretically +// have its temp file removed here; that race is accepted, and is Windows-only +// per the gate in cleanupUpdateLeftovers. +func removeUpdateLeftovers(execPath string) { _ = os.Remove(execPath + ".old") + + matches, err := filepath.Glob(filepath.Join(filepath.Dir(execPath), ".stellar-update-*")) + if err != nil { + return + } + for _, m := range matches { + _ = os.Remove(m) + } } // replaceExecutable swaps the running binary at execPath for the file at newPath. // // On Unix a plain rename over the target works. On Windows a running .exe cannot // be overwritten, but it can be renamed: move the current binary aside, put the -// new one in its place, then best-effort remove the old one (it stays locked -// until this process exits, so a failure there is fine — it's cleaned up on the -// next run). +// new one in its place. The old binary stays locked until this process exits, +// so it is deliberately left in place rather than removed here — cleanup +// happens on the next stellar run via cleanupUpdateLeftovers. func replaceExecutable(newPath, execPath string) error { if runtime.GOOS != "windows" { return os.Rename(newPath, execPath) @@ -271,6 +276,5 @@ func replaceExecutable(newPath, execPath string) error { _ = os.Rename(oldPath, execPath) return err } - _ = os.Remove(oldPath) return nil } diff --git a/cmd/update_test.go b/cmd/update_test.go new file mode 100644 index 0000000..128eae6 --- /dev/null +++ b/cmd/update_test.go @@ -0,0 +1,54 @@ +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRemoveUpdateLeftovers covers the worker split out of +// cleanupUpdateLeftovers so the removal logic is testable on any OS, even +// though the gate that calls it in production only fires on Windows. +func TestRemoveUpdateLeftovers(t *testing.T) { + dir := t.TempDir() + + execPath := filepath.Join(dir, "stellar.exe") + oldBinary := execPath + ".old" + tmpUpdate := filepath.Join(dir, ".stellar-update-abc123") + unrelated := filepath.Join(dir, "keep-me.txt") + unrelatedOld := filepath.Join(dir, "other-tool.old") + + require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) + require.NoError(t, os.WriteFile(tmpUpdate, []byte("partial download"), 0644)) + require.NoError(t, os.WriteFile(unrelated, []byte("unrelated"), 0644)) + require.NoError(t, os.WriteFile(unrelatedOld, []byte("someone else's leftover"), 0644)) + + removeUpdateLeftovers(execPath) + + assert.NoFileExists(t, oldBinary, "leftover .old binary should be removed") + assert.NoFileExists(t, tmpUpdate, "leftover update temp file should be removed") + assert.FileExists(t, unrelated, "unrelated files must not be touched") + assert.FileExists(t, unrelatedOld, "another tool's .old file must not be touched") +} + +// TestRemoveUpdateLeftovers_NothingToRemove verifies the worker is a safe +// no-op when there's nothing left over from a previous update. +func TestRemoveUpdateLeftovers_NothingToRemove(t *testing.T) { + dir := t.TempDir() + + require.NotPanics(t, func() { + removeUpdateLeftovers(filepath.Join(dir, "stellar.exe")) + }) +} + +// TestCleanupUpdateLeftovers_GateIsSafe verifies the PersistentPreRunE gate +// itself never panics or errors when called directly (it early-returns on +// non-Windows platforms, which covers CI). +func TestCleanupUpdateLeftovers_GateIsSafe(t *testing.T) { + require.NotPanics(t, func() { + cleanupUpdateLeftovers() + }) +} diff --git a/install.ps1 b/install.ps1 index 3845238..f212b3a 100644 --- a/install.ps1 +++ b/install.ps1 @@ -7,40 +7,117 @@ $ErrorActionPreference = "Stop" # Configurable install directory (override with $env:STELLAR_INSTALL_DIR) $BinDir = if ($env:STELLAR_INSTALL_DIR) { $env:STELLAR_INSTALL_DIR } else { Join-Path $env:LOCALAPPDATA "stellar\bin" } -# Detect architecture -switch ($env:PROCESSOR_ARCHITECTURE) { +# Detect architecture. On a 32-bit process running under WoW64 (32-bit +# PowerShell on a 64-bit Windows host), PROCESSOR_ARCHITECTURE reports "x86" +# even though the OS is 64-bit; PROCESSOR_ARCHITEW6432 holds the real +# architecture in that case, so it must be checked first. +$RawArch = if ($env:PROCESSOR_ARCHITEW6432) { $env:PROCESSOR_ARCHITEW6432 } else { $env:PROCESSOR_ARCHITECTURE } +switch ($RawArch) { "AMD64" { $Arch = "amd64" } "ARM64" { $Arch = "arm64" } default { - Write-Error "Unsupported architecture: $($env:PROCESSOR_ARCHITECTURE)" - exit 1 + throw "Unsupported architecture: $RawArch" } } $Binary = "stellar-windows-$Arch.exe" $Url = "https://github.com/a3chron/stellar/releases/latest/download/$Binary" +$ChecksumsUrl = "https://github.com/a3chron/stellar/releases/latest/download/checksums.txt" $Target = Join-Path $BinDir "stellar.exe" +$DownloadPath = "$Target.download" Write-Host "Installing stellar for windows-$Arch" Write-Host "Target directory: $BinDir" New-Item -ItemType Directory -Force -Path $BinDir | Out-Null -Invoke-WebRequest -Uri $Url -OutFile $Target -UseBasicParsing +# Download to a temp path first (mirrors what "stellar update" already does): +# verify the checksum before ever touching the final "stellar.exe", so a +# corrupted or interrupted download never leaves a broken binary in place. +try { + Write-Host "Downloading $Binary..." + Invoke-WebRequest -Uri $Url -OutFile $DownloadPath -UseBasicParsing + + Write-Host "Fetching checksums..." + $Checksums = (Invoke-WebRequest -Uri $ChecksumsUrl -UseBasicParsing).Content + + $ExpectedHash = $null + foreach ($line in ($Checksums -split "`r?`n")) { + if ([string]::IsNullOrWhiteSpace($line)) { continue } + $fields = $line -split '\s+' | Where-Object { $_ } + if ($fields.Count -lt 2) { continue } + if ($fields[-1] -eq $Binary) { + $ExpectedHash = $fields[0] + break + } + } + + if (-not $ExpectedHash) { + throw "checksum not found for binary: $Binary" + } + + Write-Host "Verifying checksum..." + $ActualHash = (Get-FileHash -Path $DownloadPath -Algorithm SHA256).Hash + + if ($ActualHash.ToLower() -ne $ExpectedHash.ToLower()) { + throw "checksum verification failed for $Binary`n expected: $ExpectedHash`n got: $ActualHash`n`nThe downloaded file may be corrupted or tampered with. Please try again" + } + + Write-Host "Checksum verified successfully" + + Move-Item -Path $DownloadPath -Destination $Target -Force +} +catch { + if (Test-Path $DownloadPath) { + Remove-Item -Path $DownloadPath -Force -ErrorAction SilentlyContinue + } + throw +} Write-Host "stellar installed successfully!" -# Add install dir to the user PATH if it isn't already there -$UserPath = [Environment]::GetEnvironmentVariable("Path", "User") -if ($UserPath -notlike "*$BinDir*") { +# Add install dir to the user PATH if it isn't already there. +# Compare entry-by-entry (ignoring a trailing backslash) instead of a substring +# match, so an unrelated path that merely contains $BinDir as a prefix doesn't +# make us skip the update. +# +# Read/write the registry value directly via Microsoft.Win32.Registry (rather +# than [Environment]::GetEnvironmentVariable/SetEnvironmentVariable) so an +# existing REG_EXPAND_SZ "Path" entry (one containing unexpanded references +# like %SystemRoot%) is preserved as REG_EXPAND_SZ instead of being flattened +# to a REG_SZ, which would break those references for every other program +# reading the raw registry value. +$EnvKey = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey('Environment', $true) +$RawPath = $EnvKey.GetValue('Path', '', [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) + +$Normalized = $BinDir.TrimEnd('\') +$PathEntries = ($RawPath -split ';') | ForEach-Object { $_.TrimEnd('\') } | Where-Object { $_ } +if ($PathEntries -notcontains $Normalized) { Write-Host "" Write-Host "Adding $BinDir to your user PATH" - $NewPath = if ([string]::IsNullOrEmpty($UserPath)) { $BinDir } else { "$UserPath;$BinDir" } - [Environment]::SetEnvironmentVariable("Path", $NewPath, "User") + + # PowerShell 5.1 has no ternary operator, so use if/else explicitly. + if ($RawPath) { + $NewPath = "$RawPath;$BinDir" + } + else { + $NewPath = $BinDir + } + + if ($RawPath) { + $Kind = $EnvKey.GetValueKind('Path') + } + else { + $Kind = [Microsoft.Win32.RegistryValueKind]::ExpandString + } + + $EnvKey.SetValue('Path', $NewPath, $Kind) + # Make it available in the current session too $env:Path = "$env:Path;$BinDir" Write-Host "Restart your terminal for the PATH change to take effect everywhere." } +$EnvKey.Close() Write-Host "" Write-Host "Run:" diff --git a/internal/config/config.go b/internal/config/config.go index bd54fae..76134a7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,13 @@ type Config struct { PreviousTheme string `json:"previous_theme,omitempty"` PreviousPath string `json:"previous_path,omitempty"` DownloadedThemes []string `json:"downloaded_themes,omitempty"` // ["alice/rainbow", "bob/sunset"] + // AppliedHash is the SHA-256 hex hash of the content stellar last wrote to + // starship.toml. It lets stellar recognize its own applied file regardless + // of apply mode (symlink vs copy) or OS, without depending on how the file + // happens to be on disk right now. Absent on configs written before this + // field existed; callers must treat that as "unknown" rather than + // "mismatch". + AppliedHash string `json:"applied_hash,omitempty"` } func ConfigPath() (string, error) { diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index 57faf9b..c6d12e6 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -1,6 +1,9 @@ package symlink import ( + "bytes" + "crypto/sha256" + "encoding/hex" "fmt" "io" "os" @@ -68,50 +71,150 @@ func copyFile(src, dst string) (err error) { return nil } -// backupOriginalConfig backs up the user's original starship.toml to ~/.config/stellar//backup/1.0.toml -// Returns the backup path if successful, empty string otherwise -func backupOriginalConfig(configPath string) (backupPath string, err error) { - // Check if the file exists - if _, err := os.Stat(configPath); os.IsNotExist(err) { - return "", nil // No file to back up +// filesEqual reports whether both files exist and have identical content. +// Any read error counts as "not equal", which for backup decisions errs on +// the side of preserving the file. +func filesEqual(a, b string) bool { + contentA, err := os.ReadFile(a) + if err != nil { + return false + } + contentB, err := os.ReadFile(b) + if err != nil { + return false } + return bytes.Equal(contentA, contentB) +} - if IsCopyMode() { - // Copy mode leaves starship.toml as a regular file, so there's no - // symlink to distinguish a user's original config from one stellar - // copied in. Fall back to stellar's own state: if a theme is already - // applied, the current file is our copy and must not be backed up - // (doing so would clobber the real original backup). - if cfg, cerr := config.Load(); cerr == nil && cfg.CurrentTheme != "" { - return "", nil +// HashFile computes the SHA-256 hash of a file and returns it as a hex string. +func HashFile(path string) (hash string, err error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("failed to open file for hashing: %w", err) + } + defer func() { + if cerr := f.Close(); cerr != nil && err == nil { + err = cerr } - } else if isSymlink(configPath) { - return "", nil // Already a symlink, no need to back up + }() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", fmt.Errorf("failed to hash file: %w", err) } - // Get the current user's username - currentUser, err := user.Current() + return hex.EncodeToString(h.Sum(nil)), nil +} + +// hashOf returns the SHA-256 hex hash of path, or "" if it can't be read. +// Used for "is this managed" comparisons, where an unreadable file should +// simply fail to match rather than propagate an error. +func hashOf(path string) string { + h, err := HashFile(path) if err != nil { - return "", fmt.Errorf("failed to get current user: %w", err) + return "" + } + return h +} + +// sanitizeBackupAuthor turns a raw OS username into a valid theme-author +// segment. Windows usernames from user.Current() look like "DOMAIN\user" and +// may contain spaces or other characters the theme-identifier parser rejects +// (it allows only [a-zA-Z0-9_-]). We drop any domain prefix and replace every +// disallowed character with '-', falling back to "local" if nothing usable is +// left, so the printed restore hint always parses. +func sanitizeBackupAuthor(name string) string { + // Drop everything up to and including the last path separator so a + // "DOMAIN\user" (or "domain/user") input keeps only the "user" part. + if i := strings.LastIndexAny(name, `\/`); i >= 0 { + name = name[i+1:] + } + + var b strings.Builder + for _, r := range name { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '-' { + b.WriteRune(r) + } else { + b.WriteByte('-') + } } - // Construct backup path: ~/.config/stellar//backup/1.0.toml - stellarHome, err := paths.StellarHome() + sanitized := b.String() + if sanitized == "" { + return "local" + } + return sanitized +} + +// BackupAuthor returns the theme-author segment used for backups of the user's +// original config: the sanitized current username, or "local" if it can't be +// determined. Exported so callers can build a restore hint that matches where +// the backup was actually written. +func BackupAuthor() string { + u, err := user.Current() if err != nil { - return "", fmt.Errorf("failed to get stellar home directory: %w", err) + return "local" + } + return sanitizeBackupAuthor(u.Username) +} + +// backupOriginalConfig backs up the user's original starship.toml under +// ~/.config/stellar//backup/. Backups are versioned: the first one is +// 1.0.toml (the user's genuine original), and every later unmanaged +// starship.toml stellar finds gets the next major version (2.0.toml, 3.0.toml, +// …) so no earlier backup is ever clobbered. +// Returns the backup path if a backup was created, empty string otherwise. +// +// The "is this stellar's own file" check is mode-independent: it never asks +// whether we're in symlink or copy mode, only what's actually on disk and +// recorded in cfg. Each signal only ever *prevents* a backup; a false +// negative just means an extra (harmless) backup, so this fails safe: +// - configPath is a symlink (symlink mode's own marker), or +// - cfg.AppliedHash is set and matches configPath's current content, or +// - cfg.CurrentTheme is set and configPath's content matches the cached +// theme file byte-for-byte (legacy fallback for configs saved before +// AppliedHash existed). +// +// cfg may be nil, treated the same as an empty config (i.e. no known state, +// so nothing is recognized as managed and an unmanaged file always backs up). +func backupOriginalConfig(configPath string, cfg *config.Config) (backupPath string, err error) { + // Check if the file exists + if _, err := os.Stat(configPath); os.IsNotExist(err) { + return "", nil // No file to back up } - backupDir := filepath.Join(stellarHome, currentUser.Username, "backup") - backupPath = filepath.Join(backupDir, "1.0.toml") + if cfg == nil { + cfg = &config.Config{} + } - // Never overwrite an existing backup: the first one holds the user's real - // original config. This is a safety net for copy mode, where a lost or reset - // config.json could otherwise make stellar mistake its own copy for the - // original and clobber the genuine backup. - if _, err := os.Stat(backupPath); err == nil { + managed := isSymlink(configPath) || + (cfg.AppliedHash != "" && hashOf(configPath) == cfg.AppliedHash) || + (cfg.CurrentTheme != "" && filesEqual(configPath, cfg.CurrentPath)) + if managed { return "", nil } + // Construct backup directory: ~/.config/stellar//backup + backupDir, err := paths.ThemeCacheDir(BackupAuthor(), "backup") + if err != nil { + return "", fmt.Errorf("failed to resolve backup directory: %w", err) + } + + // Pick the first free version slot so an existing backup is never + // overwritten. The first backup (1.0.toml) holds the user's real original + // config; any later unmanaged starship.toml (e.g. a hand-written one) is + // preserved as the next version rather than clobbering the original. + for n := 1; ; n++ { + candidate, perr := paths.ThemeCachePath(BackupAuthor(), "backup", fmt.Sprintf("%d.0", n)) + if perr != nil { + return "", fmt.Errorf("failed to resolve backup path: %w", perr) + } + if _, statErr := os.Stat(candidate); os.IsNotExist(statErr) { + backupPath = candidate + break + } + } + // Create backup directory if err := os.MkdirAll(backupDir, 0755); err != nil { return "", fmt.Errorf("failed to create backup directory: %w", err) @@ -125,24 +228,39 @@ func backupOriginalConfig(configPath string) (backupPath string, err error) { return backupPath, nil } +// BackupIdentifier derives the theme identifier ("/backup@") +// for a backup file from its path alone, so a printed restore hint always +// matches where the backup was actually written, regardless of how BackupAuthor +// resolves at call time. +func BackupIdentifier(backupPath string) string { + version := strings.TrimSuffix(filepath.Base(backupPath), ".toml") + author := filepath.Base(filepath.Dir(filepath.Dir(backupPath))) + return fmt.Sprintf("%s/backup@%s", author, version) +} + // ApplyTheme points ~/.config/starship.toml at the target theme file. // On Unix a symlink is created; on Windows (or when STELLAR_APPLY_MODE=copy) the // theme file is copied over starship.toml instead, since Windows handles symlinks // poorly. // +// cfg is the caller's loaded config, used only to recognize stellar's own +// previously-applied file so it isn't mistaken for a user's original (see +// backupOriginalConfig). It may be nil. This function does not read or write +// cfg's persisted state itself; the caller is responsible for saving it. +// // If an original (non-symlink) starship.toml exists, it's backed up first. // Returns the backup path if a backup was created (empty string if no backup was needed). // // Uses atomic replacement (temp-then-rename) to prevent data loss: // if applying fails, the original config remains intact. -func ApplyTheme(target string) (backupPath string, err error) { +func ApplyTheme(target string, cfg *config.Config) (backupPath string, err error) { configPath, err := StarshipConfigPath() if err != nil { return "", err } - // Back up original config if it exists and is not a symlink - backupPath, err = backupOriginalConfig(configPath) + // Back up original config if it exists and isn't recognized as stellar's own + backupPath, err = backupOriginalConfig(configPath, cfg) if err != nil { return "", fmt.Errorf("failed to backup original config: %w", err) } diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index 9647567..b82a445 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -2,10 +2,10 @@ package symlink import ( "os" - "os/user" "path/filepath" "testing" + "github.com/a3chron/stellar/internal/config" "github.com/a3chron/stellar/internal/paths" "github.com/a3chron/stellar/internal/testutil" "github.com/stretchr/testify/assert" @@ -13,11 +13,12 @@ import ( ) func TestApplyTheme_NoExistingFile(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath) + backupPath, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) assert.Empty(t, backupPath, "should not create backup when no existing file") @@ -26,6 +27,7 @@ func TestApplyTheme_NoExistingFile(t *testing.T) { } func TestApplyTheme_ExistingRegularFile(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) originalContent := "# Original config\n[character]\nsuccess_symbol = \"OLD\"" @@ -33,16 +35,14 @@ func TestApplyTheme_ExistingRegularFile(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath) + backupPath, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) assert.NotEmpty(t, backupPath) assert.True(t, env.FileExists(backupPath)) assert.Equal(t, originalContent, env.ReadFile(backupPath)) - currentUser, err := user.Current() - require.NoError(t, err) - expectedBackupPath := filepath.Join(env.StellarDir, currentUser.Username, "backup", "1.0.toml") + expectedBackupPath := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") assert.Equal(t, expectedBackupPath, backupPath) assert.True(t, env.IsSymlink(env.StarshipPath)) @@ -50,14 +50,15 @@ func TestApplyTheme_ExistingRegularFile(t *testing.T) { } func TestApplyTheme_ExistingSymlink(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - _, err := ApplyTheme(themePath1) + _, err := ApplyTheme(themePath1, &config.Config{}) require.NoError(t, err) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath2) + backupPath, err := ApplyTheme(themePath2, &config.Config{}) require.NoError(t, err) assert.Empty(t, backupPath, "should not create backup when replacing symlink") @@ -67,14 +68,14 @@ func TestApplyTheme_ExistingSymlink(t *testing.T) { } func TestApplyTheme_CopyMode(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") assert.True(t, IsCopyMode()) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath) + backupPath, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) assert.Empty(t, backupPath, "should not create backup when no existing file") @@ -85,15 +86,15 @@ func TestApplyTheme_CopyMode(t *testing.T) { } func TestApplyTheme_CopyMode_ExistingRegularFile(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") originalContent := "# Original config\n[character]\nsuccess_symbol = \"OLD\"" env.CreateStarshipConfig(originalContent) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath) + backupPath, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) // Original is backed up before being overwritten by the copy @@ -106,19 +107,21 @@ func TestApplyTheme_CopyMode_ExistingRegularFile(t *testing.T) { } func TestApplyTheme_CopyMode_ReplacesPreviousCopy(t *testing.T) { - t.Setenv(paths.EnvApplyMode, "copy") env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - _, err := ApplyTheme(themePath1) + _, err := ApplyTheme(themePath1, &config.Config{}) require.NoError(t, err) // Simulate the config state that cmd/apply.go would have saved after the // first apply, so the second apply knows the current file is stellar's copy. env.CreateConfig(`{"current_theme":"alice/rainbow@1.0","current_path":"` + themePath1 + `"}`) + cfg, err := config.Load() + require.NoError(t, err) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) - backupPath, err := ApplyTheme(themePath2) + backupPath, err := ApplyTheme(themePath2, cfg) require.NoError(t, err) // A stellar-managed copy should not be backed up when replaced @@ -127,40 +130,209 @@ func TestApplyTheme_CopyMode_ReplacesPreviousCopy(t *testing.T) { assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) } -// TestApplyTheme_CopyMode_DoesNotClobberExistingBackup guards the safety net for -// the case where stellar's config state is lost (CurrentTheme == "") but the -// current starship.toml is actually a stellar copy. Without the guard, stellar -// would mistake its own copy for the user's original and overwrite the genuine -// backup. -func TestApplyTheme_CopyMode_DoesNotClobberExistingBackup(t *testing.T) { +// TestApplyTheme_CopyMode_BacksUpHandEditedConfig verifies that copy mode +// detects a hand-edited starship.toml: even though config.json says a theme is +// applied, the file no longer matches the cached theme (nor the recorded +// applied hash, since this fabricated config predates that field), so it must +// be backed up instead of being treated as stellar's own copy. +func TestApplyTheme_CopyMode_BacksUpHandEditedConfig(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + _, err := ApplyTheme(themePath1, &config.Config{}) + require.NoError(t, err) + env.CreateConfig(`{"current_theme":"alice/rainbow@1.0","current_path":"` + themePath1 + `"}`) + cfg, err := config.Load() + require.NoError(t, err) + + // The user edits the applied copy directly. + editedContent := "# MY HAND-EDITED CONFIG" + env.CreateStarshipConfig(editedContent) + + themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) + backupPath, err := ApplyTheme(themePath2, cfg) + require.NoError(t, err) + + expectedBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") + assert.Equal(t, expectedBackup, backupPath, "hand-edited config should be backed up") + assert.Equal(t, editedContent, env.ReadFile(backupPath)) + assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) +} + +// TestApplyTheme_CopyMode_MissingCachedThemeBacksUp covers the fallback when the +// cached theme referenced by config.json is gone (e.g. removed via +// "stellar clean --all"): the file can't be verified as stellar's copy via the +// legacy CurrentPath comparison, and there is no applied_hash recorded either +// (this fabricated config predates that field), so it is backed up rather than +// risking data loss. +func TestApplyTheme_CopyMode_MissingCachedThemeBacksUp(t *testing.T) { + env := testutil.SetupTestEnv(t) t.Setenv(paths.EnvApplyMode, "copy") + + // config.json points at a cached theme file that no longer exists. + env.CreateConfig(`{"current_theme":"alice/rainbow@1.0","current_path":"` + env.StellarDir + `/alice/rainbow/1.0.toml"}`) + env.CreateStarshipConfig("# unverifiable stellar copy") + cfg, err := config.Load() + require.NoError(t, err) + + themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + backupPath, err := ApplyTheme(themePath, cfg) + require.NoError(t, err) + + expectedBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") + assert.Equal(t, expectedBackup, backupPath, "unverifiable config should be backed up") + assert.Equal(t, "# unverifiable stellar copy", env.ReadFile(backupPath)) +} + +// TestApplyTheme_CopyMode_AppliedHashRecognizesCopy is the hash-present +// counterpart to TestApplyTheme_CopyMode_MissingCachedThemeBacksUp: when +// applied_hash is recorded and still matches what's on disk, stellar +// recognizes its own copy and skips the backup even though the cached theme +// file it originally came from is gone. +func TestApplyTheme_CopyMode_AppliedHashRecognizesCopy(t *testing.T) { env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") - // A real original backup already exists from an earlier apply. - currentUser, err := user.Current() + appliedContent := "# stellar copy, cached source now missing" + env.CreateStarshipConfig(appliedContent) + hash, err := HashFile(env.StarshipPath) + require.NoError(t, err) + + // config.json points at a cached theme file that no longer exists, but + // applied_hash matches the current starship.toml content. + env.CreateConfig(`{"current_theme":"alice/rainbow@1.0","current_path":"` + env.StellarDir + `/alice/rainbow/1.0.toml","applied_hash":"` + hash + `"}`) + cfg, err := config.Load() + require.NoError(t, err) + + themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + backupPath, err := ApplyTheme(themePath, cfg) require.NoError(t, err) - backupPath := filepath.Join(env.StellarDir, currentUser.Username, "backup", "1.0.toml") - require.NoError(t, os.MkdirAll(filepath.Dir(backupPath), 0755)) - require.NoError(t, os.WriteFile(backupPath, []byte("# PRECIOUS ORIGINAL"), 0644)) + + assert.Empty(t, backupPath, "applied_hash match should prevent a junk backup") +} + +// TestApplyTheme_CopyMode_VersionedBackupPreservesOriginal covers the case where +// stellar's config state is lost (CurrentTheme == "") but the current +// starship.toml is actually a stellar copy. Rather than skipping the backup (and +// risking silent data loss on a genuinely new original), stellar writes the file +// as the next backup version. The genuine original at 1.0.toml is never touched: +// the unrecognized file gets 2.0.toml instead. +func TestApplyTheme_CopyMode_VersionedBackupPreservesOriginal(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + // A real original backup already exists from an earlier apply. + origBackupPath := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") + require.NoError(t, os.MkdirAll(filepath.Dir(origBackupPath), 0755)) + require.NoError(t, os.WriteFile(origBackupPath, []byte("# PRECIOUS ORIGINAL"), 0644)) // starship.toml holds a stellar copy, but there is no config.json state // (CurrentTheme == ""), so the config-based marker can't help here. env.CreateStarshipConfig("# stellar copy of some theme") themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - newBackup, err := ApplyTheme(themePath) + newBackup, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) - assert.Empty(t, newBackup, "should not create a new backup when one already exists") - assert.Equal(t, "# PRECIOUS ORIGINAL", env.ReadFile(backupPath), "existing backup must be preserved") + // The unrecognized file is preserved as the next version (2.0.toml)... + expectedNewBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "2.0.toml") + assert.Equal(t, expectedNewBackup, newBackup, "new backup should use the next version") + assert.Equal(t, "# stellar copy of some theme", env.ReadFile(newBackup)) + + // ...and the genuine original at 1.0.toml is never clobbered. + assert.Equal(t, "# PRECIOUS ORIGINAL", env.ReadFile(origBackupPath), "original backup must be preserved") assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) } +// TestApplyTheme_VersionedBackup verifies that each unmanaged original starship +// config is preserved under its own version instead of overwriting an earlier +// backup. +func TestApplyTheme_VersionedBackup(t *testing.T) { + testutil.RequireSymlinks(t) + env := testutil.SetupTestEnv(t) + + // First original: backed up as 1.0.toml. + firstOriginal := "# FIRST ORIGINAL" + env.CreateStarshipConfig(firstOriginal) + + themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + firstBackup, err := ApplyTheme(themePath1, &config.Config{}) + require.NoError(t, err) + + expectedFirstBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") + assert.Equal(t, expectedFirstBackup, firstBackup) + assert.Equal(t, firstOriginal, env.ReadFile(firstBackup)) + + // The user later replaces starship.toml with a new hand-written config + // (a regular file, not the symlink stellar just created). + secondOriginal := "# SECOND ORIGINAL" + require.NoError(t, os.Remove(env.StarshipPath)) + env.CreateStarshipConfig(secondOriginal) + + themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + secondBackup, err := ApplyTheme(themePath2, &config.Config{}) + require.NoError(t, err) + + // The second original is preserved as 2.0.toml, and 1.0.toml is unchanged. + expectedSecondBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "2.0.toml") + assert.Equal(t, expectedSecondBackup, secondBackup) + assert.Equal(t, secondOriginal, env.ReadFile(secondBackup)) + assert.Equal(t, firstOriginal, env.ReadFile(firstBackup), "earlier backup must stay intact") +} + +func TestSanitizeBackupAuthor(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"windows domain prefix", `DESKTOP-ABC\kurt`, "kurt"}, + {"plain username", "kurt", "kurt"}, + {"spaces replaced", "John Doe", "John-Doe"}, + {"domain and spaces", `domain\John Doe`, "John-Doe"}, + {"empty falls back to local", "", "local"}, + {"only separator falls back to local", `\`, "local"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeBackupAuthor(tt.input)) + }) + } +} + +func TestBackupIdentifier(t *testing.T) { + tests := []struct { + name string + backupPath string + want string + }{ + { + name: "standard path", + backupPath: filepath.Join("/home/user/.config/stellar", "kurt", "backup", "1.0.toml"), + want: "kurt/backup@1.0", + }, + { + name: "higher version", + backupPath: filepath.Join("/home/user/.config/stellar", "john-doe", "backup", "3.0.toml"), + want: "john-doe/backup@3.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, BackupIdentifier(tt.backupPath)) + }) + } +} + func TestGetCurrentTarget(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - _, err := ApplyTheme(themePath) + _, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) target, err := GetCurrentTarget() @@ -188,6 +360,7 @@ func TestIsCopyMode(t *testing.T) { } func TestIsSymlink(t *testing.T) { + testutil.RequireSymlinks(t) env := testutil.SetupTestEnv(t) regularPath := filepath.Join(env.RootDir, "regular.txt") @@ -200,3 +373,30 @@ func TestIsSymlink(t *testing.T) { assert.True(t, isSymlink(symlinkPath)) assert.False(t, isSymlink("/nonexistent/path")) } + +func TestHashFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "sample.toml") + require.NoError(t, os.WriteFile(path, []byte(testutil.SampleTOML()), 0644)) + + hash1, err := HashFile(path) + require.NoError(t, err) + assert.NotEmpty(t, hash1) + + // Same content hashes the same. + path2 := filepath.Join(dir, "sample-copy.toml") + require.NoError(t, os.WriteFile(path2, []byte(testutil.SampleTOML()), 0644)) + hash2, err := HashFile(path2) + require.NoError(t, err) + assert.Equal(t, hash1, hash2) + + // Different content hashes differently. + path3 := filepath.Join(dir, "other.toml") + require.NoError(t, os.WriteFile(path3, []byte(testutil.SampleTOMLWithCustom()), 0644)) + hash3, err := HashFile(path3) + require.NoError(t, err) + assert.NotEqual(t, hash1, hash3) + + _, err = HashFile(filepath.Join(dir, "missing.toml")) + assert.Error(t, err) +} diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index 522f412..3da23b6 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -3,12 +3,15 @@ package testutil import ( + "bytes" + "io" "net/http/httptest" "os" "path/filepath" "testing" "github.com/a3chron/stellar/internal/paths" + "github.com/fatih/color" ) // TestEnv holds the test environment configuration @@ -56,11 +59,17 @@ func SetupTestEnv(t *testing.T) *TestEnv { env.origEnv[paths.EnvStarshipPath] = os.Getenv(paths.EnvStarshipPath) env.origEnv[paths.EnvTmpDir] = os.Getenv(paths.EnvTmpDir) env.origEnv[paths.EnvAPIURL] = os.Getenv(paths.EnvAPIURL) + env.origEnv[paths.EnvApplyMode] = os.Getenv(paths.EnvApplyMode) // Set test env variables _ = os.Setenv(paths.EnvStellarHome, env.StellarDir) _ = os.Setenv(paths.EnvStarshipPath, env.StarshipPath) _ = os.Setenv(paths.EnvTmpDir, env.TmpDir) + // Pin apply mode to symlink so the suite is deterministic regardless of + // the OS running it (IsCopyMode defaults to copy on Windows). Tests that + // specifically want copy-mode behavior call t.Setenv(paths.EnvApplyMode, + // "copy") themselves *after* calling SetupTestEnv, so their override wins. + _ = os.Setenv(paths.EnvApplyMode, "symlink") // Register cleanup t.Cleanup(func() { @@ -172,6 +181,78 @@ func (e *TestEnv) ReadSymlink(path string) string { return target } +// CaptureOutput runs fn with stdout redirected to a pipe and returns everything +// written to it. It swaps both os.Stdout and color.Output: fatih/color captures +// os.Stdout at package-init time, so reassigning os.Stdout alone would miss any +// color.* output. Both are restored before returning. The pipe is drained in a +// goroutine so large output can't deadlock on the pipe buffer. +// +// The restore is deferred and idempotent: if fn panics (e.g. a t.Fatal inside +// it), os.Stdout/color.Output are still put back before the panic propagates, +// instead of staying pointed at a pipe that no longer has a reader for every +// later test in the process. +func CaptureOutput(t *testing.T, fn func()) string { + t.Helper() + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + + origStdout := os.Stdout + origColorOutput := color.Output + os.Stdout = w + color.Output = w + + done := make(chan string, 1) + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + done <- buf.String() + }() + + restored := false + restore := func() { + if !restored { + restored = true + os.Stdout = origStdout + color.Output = origColorOutput + _ = w.Close() + } + } + defer restore() + + fn() + + // Restore first, then close the writer so the reader goroutine sees EOF. + restore() + + out := <-done + if cerr := r.Close(); cerr != nil { + t.Fatalf("failed to close pipe reader: %v", cerr) + } + return out +} + +// RequireSymlinks skips the test if the environment can't create symlinks +// (e.g. Windows without Developer Mode or admin privileges). Call it at the +// top of any test that exercises symlink-mode behavior directly. +func RequireSymlinks(t *testing.T) { + t.Helper() + + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "link") + + if err := os.WriteFile(target, []byte("probe"), 0644); err != nil { + t.Fatalf("failed to create symlink probe target: %v", err) + } + + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks not supported in this environment: %v", err) + } +} + // SampleTOML returns a valid sample starship config func SampleTOML() string { return `# Sample starship config From 83f67b4110835793a53f9155dfa383531a8aa8d3 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 10:59:32 +0200 Subject: [PATCH 03/12] refactor(symlink): record AppliedHash inside ApplyTheme and return structured backup info Move the AppliedHash bookkeeping out of the apply/rollback call sites into ApplyTheme itself, so no future caller can forget it and cause spurious backups. In copy mode the hash is computed while the file streams through copyFile instead of re-reading it afterward. Replace BackupIdentifier's path re-parsing with a BackupInfo{Path, Identifier} built at creation time from the same author/version values used to construct the path, formatted via theme.Theme.String() so the identifier wire format stays defined in one place. Co-Authored-By: Claude Fable 5 --- cmd/apply.go | 19 ++--- cmd/rollback.go | 10 +-- internal/symlink/symlink.go | 120 ++++++++++++++++++---------- internal/symlink/symlink_test.go | 130 +++++++++++++++++-------------- 4 files changed, 164 insertions(+), 115 deletions(-) diff --git a/cmd/apply.go b/cmd/apply.go index dee7eac..08d5c7e 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -36,10 +36,10 @@ func promptConfirmation(prompt string) bool { // printBackupNotice tells the user their original starship.toml was preserved // and how to restore it. Shared by apply and rollback so both surface the // same notice whenever backupOriginalConfig actually creates a backup. -func printBackupNotice(backupPath string) { +func printBackupNotice(info *symlink.BackupInfo) { color.Yellow("Your original starship.toml has been backed up to:") - color.Yellow(" %s", backupPath) - color.Cyan("\nYou can apply it later with: stellar apply %s \n", symlink.BackupIdentifier(backupPath)) + color.Yellow(" %s", info.Path) + color.Cyan("\nYou can apply it later with: stellar apply %s \n", info.Identifier) } var applyCmd = &cobra.Command{ @@ -176,22 +176,17 @@ var applyCmd = &cobra.Command{ // 5. Apply the theme FIRST (before saving config) // This ensures that if applying fails, config remains unchanged - backupPath, err := symlink.ApplyTheme(themePath, cfg) + backupInfo, err := symlink.ApplyTheme(themePath, cfg) if err != nil { return err } // 6. Update config only AFTER applying succeeds + // (ApplyTheme has already recorded cfg.AppliedHash for the applied file) cfg.PreviousTheme = cfg.CurrentTheme cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = t.String() cfg.CurrentPath = themePath - // Best-effort: record the hash of what was actually applied so future - // runs can recognize this exact file as stellar-managed regardless of - // apply mode. Empty on error (e.g. copy mode where a read races with - // something external); that just means a future run falls back to the - // legacy CurrentPath comparison instead of failing outright. - cfg.AppliedHash, _ = symlink.HashFile(themePath) if err := cfg.Save(); err != nil { // Symlink succeeded but config save failed @@ -200,8 +195,8 @@ var applyCmd = &cobra.Command{ } // Notify user if their original config was backed up - if backupPath != "" { - printBackupNotice(backupPath) + if backupInfo != nil { + printBackupNotice(backupInfo) } color.Green("Applied %s", t) diff --git a/cmd/rollback.go b/cmd/rollback.go index 36c5ccd..1b35666 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -70,19 +70,17 @@ var rollbackCmd = &cobra.Command{ // Apply the previous theme FIRST (before modifying config) // This ensures that if applying fails, config remains unchanged - backupPath, err := symlink.ApplyTheme(previousPath, cfg) + backupInfo, err := symlink.ApplyTheme(previousPath, cfg) if err != nil { return fmt.Errorf("failed to apply previous theme: %w", err) } // Only swap config after applying succeeds + // (ApplyTheme has already recorded cfg.AppliedHash for the applied file) cfg.PreviousTheme = cfg.CurrentTheme cfg.PreviousPath = cfg.CurrentPath cfg.CurrentTheme = previousTheme cfg.CurrentPath = previousPath - // Best-effort, same as apply: record what we just wrote so future runs - // recognize this file as stellar-managed. - cfg.AppliedHash, _ = symlink.HashFile(previousPath) // Save config if err := cfg.Save(); err != nil { @@ -92,8 +90,8 @@ var rollbackCmd = &cobra.Command{ } // Notify user if their original config was backed up (previously silent) - if backupPath != "" { - printBackupNotice(backupPath) + if backupInfo != nil { + printBackupNotice(backupInfo) } color.Green("Rolled back to: %s", cfg.CurrentTheme) diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index c6d12e6..1f5013a 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -14,6 +14,7 @@ import ( "github.com/a3chron/stellar/internal/config" "github.com/a3chron/stellar/internal/paths" + "github.com/a3chron/stellar/internal/theme" ) func StarshipConfigPath() (string, error) { @@ -43,10 +44,13 @@ func isSymlink(path string) bool { } // copyFile copies the contents of src to dst, creating or truncating dst. -func copyFile(src, dst string) (err error) { +// It returns the SHA-256 hex hash of the copied content, computed while the +// data streams through so callers that need the hash (e.g. ApplyTheme) don't +// have to re-read the file afterward. +func copyFile(src, dst string) (hash string, err error) { source, err := os.Open(src) if err != nil { - return fmt.Errorf("failed to open source file: %w", err) + return "", fmt.Errorf("failed to open source file: %w", err) } defer func() { if cerr := source.Close(); cerr != nil && err == nil { @@ -56,7 +60,7 @@ func copyFile(src, dst string) (err error) { destination, err := os.Create(dst) if err != nil { - return fmt.Errorf("failed to create destination file: %w", err) + return "", fmt.Errorf("failed to create destination file: %w", err) } defer func() { if cerr := destination.Close(); cerr != nil && err == nil { @@ -64,11 +68,12 @@ func copyFile(src, dst string) (err error) { } }() - if _, err := io.Copy(destination, source); err != nil { - return fmt.Errorf("failed to copy file: %w", err) + h := sha256.New() + if _, err := io.Copy(io.MultiWriter(destination, h), source); err != nil { + return "", fmt.Errorf("failed to copy file: %w", err) } - return nil + return hex.EncodeToString(h.Sum(nil)), nil } // filesEqual reports whether both files exist and have identical content. @@ -158,12 +163,24 @@ func BackupAuthor() string { return sanitizeBackupAuthor(u.Username) } +// BackupInfo describes a backup of the user's original config created by +// backupOriginalConfig/ApplyTheme. +type BackupInfo struct { + // Path is the on-disk location the original config was copied to. + Path string + // Identifier is the theme identifier ("/backup@") that + // targets this exact backup, built at creation time from the same + // author/version values used to construct Path (rather than re-derived + // from Path later), so it always matches where the backup actually lives. + Identifier string +} + // backupOriginalConfig backs up the user's original starship.toml under // ~/.config/stellar//backup/. Backups are versioned: the first one is // 1.0.toml (the user's genuine original), and every later unmanaged // starship.toml stellar finds gets the next major version (2.0.toml, 3.0.toml, // …) so no earlier backup is ever clobbered. -// Returns the backup path if a backup was created, empty string otherwise. +// Returns a *BackupInfo if a backup was created, nil otherwise. // // The "is this stellar's own file" check is mode-independent: it never asks // whether we're in symlink or copy mode, only what's actually on disk and @@ -177,10 +194,10 @@ func BackupAuthor() string { // // cfg may be nil, treated the same as an empty config (i.e. no known state, // so nothing is recognized as managed and an unmanaged file always backs up). -func backupOriginalConfig(configPath string, cfg *config.Config) (backupPath string, err error) { +func backupOriginalConfig(configPath string, cfg *config.Config) (info *BackupInfo, err error) { // Check if the file exists if _, err := os.Stat(configPath); os.IsNotExist(err) { - return "", nil // No file to back up + return nil, nil // No file to back up } if cfg == nil { @@ -191,23 +208,27 @@ func backupOriginalConfig(configPath string, cfg *config.Config) (backupPath str (cfg.AppliedHash != "" && hashOf(configPath) == cfg.AppliedHash) || (cfg.CurrentTheme != "" && filesEqual(configPath, cfg.CurrentPath)) if managed { - return "", nil + return nil, nil } + author := BackupAuthor() + // Construct backup directory: ~/.config/stellar//backup - backupDir, err := paths.ThemeCacheDir(BackupAuthor(), "backup") + backupDir, err := paths.ThemeCacheDir(author, "backup") if err != nil { - return "", fmt.Errorf("failed to resolve backup directory: %w", err) + return nil, fmt.Errorf("failed to resolve backup directory: %w", err) } // Pick the first free version slot so an existing backup is never // overwritten. The first backup (1.0.toml) holds the user's real original // config; any later unmanaged starship.toml (e.g. a hand-written one) is // preserved as the next version rather than clobbering the original. + var backupPath, version string for n := 1; ; n++ { - candidate, perr := paths.ThemeCachePath(BackupAuthor(), "backup", fmt.Sprintf("%d.0", n)) + version = fmt.Sprintf("%d.0", n) + candidate, perr := paths.ThemeCachePath(author, "backup", version) if perr != nil { - return "", fmt.Errorf("failed to resolve backup path: %w", perr) + return nil, fmt.Errorf("failed to resolve backup path: %w", perr) } if _, statErr := os.Stat(candidate); os.IsNotExist(statErr) { backupPath = candidate @@ -217,25 +238,20 @@ func backupOriginalConfig(configPath string, cfg *config.Config) (backupPath str // Create backup directory if err := os.MkdirAll(backupDir, 0755); err != nil { - return "", fmt.Errorf("failed to create backup directory: %w", err) + return nil, fmt.Errorf("failed to create backup directory: %w", err) } // Copy the original file to backup location - if err := copyFile(configPath, backupPath); err != nil { - return "", fmt.Errorf("failed to back up config: %w", err) + if _, err := copyFile(configPath, backupPath); err != nil { + return nil, fmt.Errorf("failed to back up config: %w", err) } - return backupPath, nil -} + // Build the identifier from the same author/version values used above, + // via the identifier format theme.Theme.String() already owns, instead of + // re-deriving it later by parsing backupPath. + identifier := (&theme.Theme{Author: author, Name: "backup", Version: version}).String() -// BackupIdentifier derives the theme identifier ("/backup@") -// for a backup file from its path alone, so a printed restore hint always -// matches where the backup was actually written, regardless of how BackupAuthor -// resolves at call time. -func BackupIdentifier(backupPath string) string { - version := strings.TrimSuffix(filepath.Base(backupPath), ".toml") - author := filepath.Base(filepath.Dir(filepath.Dir(backupPath))) - return fmt.Sprintf("%s/backup@%s", author, version) + return &BackupInfo{Path: backupPath, Identifier: identifier}, nil } // ApplyTheme points ~/.config/starship.toml at the target theme file. @@ -243,26 +259,29 @@ func BackupIdentifier(backupPath string) string { // theme file is copied over starship.toml instead, since Windows handles symlinks // poorly. // -// cfg is the caller's loaded config, used only to recognize stellar's own +// cfg is the caller's loaded config, used to recognize stellar's own // previously-applied file so it isn't mistaken for a user's original (see -// backupOriginalConfig). It may be nil. This function does not read or write -// cfg's persisted state itself; the caller is responsible for saving it. +// backupOriginalConfig). It may be nil. On a successful apply, ApplyTheme also +// records the hash of the applied content into cfg.AppliedHash (when cfg is +// non-nil) so future calls can recognize this exact file as stellar-managed; +// this mutates the in-memory cfg, but the caller is still responsible for +// persisting it via cfg.Save(). // // If an original (non-symlink) starship.toml exists, it's backed up first. -// Returns the backup path if a backup was created (empty string if no backup was needed). +// Returns a *BackupInfo if a backup was created (nil if no backup was needed). // // Uses atomic replacement (temp-then-rename) to prevent data loss: // if applying fails, the original config remains intact. -func ApplyTheme(target string, cfg *config.Config) (backupPath string, err error) { +func ApplyTheme(target string, cfg *config.Config) (info *BackupInfo, err error) { configPath, err := StarshipConfigPath() if err != nil { - return "", err + return nil, err } // Back up original config if it exists and isn't recognized as stellar's own - backupPath, err = backupOriginalConfig(configPath, cfg) + info, err = backupOriginalConfig(configPath, cfg) if err != nil { - return "", fmt.Errorf("failed to backup original config: %w", err) + return nil, fmt.Errorf("failed to backup original config: %w", err) } // Use atomic replacement to avoid a data-loss window. @@ -273,14 +292,27 @@ func ApplyTheme(target string, cfg *config.Config) (backupPath string, err error // Remove any stale temp file from a previous failed attempt _ = os.Remove(tempPath) + // Best-effort: compute the hash of what's actually being applied so it can + // be recorded into cfg below. A failure here just leaves appliedHash empty; + // that degrades a future run back to the legacy CurrentPath comparison + // instead of failing this apply outright. + var appliedHash string + if IsCopyMode() { - if err := copyFile(target, tempPath); err != nil { - return backupPath, fmt.Errorf("failed to copy theme: %w", err) + // copyFile already reads the file in full; compute the hash while the + // data streams through instead of re-reading it afterward. + h, err := copyFile(target, tempPath) + if err != nil { + return info, fmt.Errorf("failed to copy theme: %w", err) } + appliedHash = h } else { if err := os.Symlink(target, tempPath); err != nil { - return backupPath, fmt.Errorf("failed to create symlink: %w", err) + return info, fmt.Errorf("failed to create symlink: %w", err) } + // In symlink mode there's no copy to piggyback on; hash the target + // file once. + appliedHash = hashOf(target) } // Atomic rename over the target (replaces the destination on POSIX and Windows) @@ -288,10 +320,18 @@ func ApplyTheme(target string, cfg *config.Config) (backupPath string, err error if err := os.Rename(tempPath, configPath); err != nil { // Clean up temp file on failure _ = os.Remove(tempPath) - return backupPath, fmt.Errorf("failed to replace config: %w", err) + return info, fmt.Errorf("failed to replace config: %w", err) + } + + // Best-effort: record the hash of what was actually applied so future + // runs can recognize this exact file as stellar-managed regardless of + // apply mode. Empty on error; that just means a future run falls back to + // the legacy CurrentPath comparison instead of failing outright. + if cfg != nil { + cfg.AppliedHash = appliedHash } - return backupPath, nil + return info, nil } // GetCurrentTarget returns the theme file that starship.toml points at. diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index b82a445..ffbb41b 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -3,6 +3,7 @@ package symlink import ( "os" "path/filepath" + "strings" "testing" "github.com/a3chron/stellar/internal/config" @@ -18,9 +19,9 @@ func TestApplyTheme_NoExistingFile(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, &config.Config{}) + info, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) - assert.Empty(t, backupPath, "should not create backup when no existing file") + assert.Nil(t, info, "should not create backup when no existing file") assert.True(t, env.IsSymlink(env.StarshipPath)) assert.Equal(t, themePath, env.ReadSymlink(env.StarshipPath)) @@ -35,15 +36,16 @@ func TestApplyTheme_ExistingRegularFile(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, &config.Config{}) + info, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) - assert.NotEmpty(t, backupPath) - assert.True(t, env.FileExists(backupPath)) - assert.Equal(t, originalContent, env.ReadFile(backupPath)) + require.NotNil(t, info) + assert.True(t, env.FileExists(info.Path)) + assert.Equal(t, originalContent, env.ReadFile(info.Path)) expectedBackupPath := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") - assert.Equal(t, expectedBackupPath, backupPath) + assert.Equal(t, expectedBackupPath, info.Path) + assert.Equal(t, BackupAuthor()+"/backup@1.0", info.Identifier) assert.True(t, env.IsSymlink(env.StarshipPath)) assert.Equal(t, themePath, env.ReadSymlink(env.StarshipPath)) @@ -58,10 +60,10 @@ func TestApplyTheme_ExistingSymlink(t *testing.T) { require.NoError(t, err) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath2, &config.Config{}) + info, err := ApplyTheme(themePath2, &config.Config{}) require.NoError(t, err) - assert.Empty(t, backupPath, "should not create backup when replacing symlink") + assert.Nil(t, info, "should not create backup when replacing symlink") assert.True(t, env.IsSymlink(env.StarshipPath)) assert.Equal(t, themePath2, env.ReadSymlink(env.StarshipPath)) @@ -75,9 +77,9 @@ func TestApplyTheme_CopyMode(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, &config.Config{}) + info, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) - assert.Empty(t, backupPath, "should not create backup when no existing file") + assert.Nil(t, info, "should not create backup when no existing file") // In copy mode starship.toml is a regular file, not a symlink assert.True(t, env.FileExists(env.StarshipPath)) @@ -94,13 +96,13 @@ func TestApplyTheme_CopyMode_ExistingRegularFile(t *testing.T) { themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, &config.Config{}) + info, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) // Original is backed up before being overwritten by the copy - assert.NotEmpty(t, backupPath) - assert.True(t, env.FileExists(backupPath)) - assert.Equal(t, originalContent, env.ReadFile(backupPath)) + require.NotNil(t, info) + assert.True(t, env.FileExists(info.Path)) + assert.Equal(t, originalContent, env.ReadFile(info.Path)) assert.False(t, env.IsSymlink(env.StarshipPath)) assert.Equal(t, testutil.SampleTOML(), env.ReadFile(env.StarshipPath)) @@ -121,11 +123,11 @@ func TestApplyTheme_CopyMode_ReplacesPreviousCopy(t *testing.T) { require.NoError(t, err) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) - backupPath, err := ApplyTheme(themePath2, cfg) + info, err := ApplyTheme(themePath2, cfg) require.NoError(t, err) // A stellar-managed copy should not be backed up when replaced - assert.Empty(t, backupPath, "should not back up a config stellar already manages") + assert.Nil(t, info, "should not back up a config stellar already manages") assert.False(t, env.IsSymlink(env.StarshipPath)) assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) } @@ -151,12 +153,13 @@ func TestApplyTheme_CopyMode_BacksUpHandEditedConfig(t *testing.T) { env.CreateStarshipConfig(editedContent) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOMLWithCustom()) - backupPath, err := ApplyTheme(themePath2, cfg) + info, err := ApplyTheme(themePath2, cfg) require.NoError(t, err) expectedBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") - assert.Equal(t, expectedBackup, backupPath, "hand-edited config should be backed up") - assert.Equal(t, editedContent, env.ReadFile(backupPath)) + require.NotNil(t, info, "hand-edited config should be backed up") + assert.Equal(t, expectedBackup, info.Path) + assert.Equal(t, editedContent, env.ReadFile(info.Path)) assert.Equal(t, testutil.SampleTOMLWithCustom(), env.ReadFile(env.StarshipPath)) } @@ -177,12 +180,13 @@ func TestApplyTheme_CopyMode_MissingCachedThemeBacksUp(t *testing.T) { require.NoError(t, err) themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, cfg) + info, err := ApplyTheme(themePath, cfg) require.NoError(t, err) expectedBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") - assert.Equal(t, expectedBackup, backupPath, "unverifiable config should be backed up") - assert.Equal(t, "# unverifiable stellar copy", env.ReadFile(backupPath)) + require.NotNil(t, info, "unverifiable config should be backed up") + assert.Equal(t, expectedBackup, info.Path) + assert.Equal(t, "# unverifiable stellar copy", env.ReadFile(info.Path)) } // TestApplyTheme_CopyMode_AppliedHashRecognizesCopy is the hash-present @@ -206,10 +210,10 @@ func TestApplyTheme_CopyMode_AppliedHashRecognizesCopy(t *testing.T) { require.NoError(t, err) themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - backupPath, err := ApplyTheme(themePath, cfg) + info, err := ApplyTheme(themePath, cfg) require.NoError(t, err) - assert.Empty(t, backupPath, "applied_hash match should prevent a junk backup") + assert.Nil(t, info, "applied_hash match should prevent a junk backup") } // TestApplyTheme_CopyMode_VersionedBackupPreservesOriginal covers the case where @@ -232,13 +236,14 @@ func TestApplyTheme_CopyMode_VersionedBackupPreservesOriginal(t *testing.T) { env.CreateStarshipConfig("# stellar copy of some theme") themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - newBackup, err := ApplyTheme(themePath, &config.Config{}) + newInfo, err := ApplyTheme(themePath, &config.Config{}) require.NoError(t, err) // The unrecognized file is preserved as the next version (2.0.toml)... + require.NotNil(t, newInfo, "new backup should use the next version") expectedNewBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "2.0.toml") - assert.Equal(t, expectedNewBackup, newBackup, "new backup should use the next version") - assert.Equal(t, "# stellar copy of some theme", env.ReadFile(newBackup)) + assert.Equal(t, expectedNewBackup, newInfo.Path) + assert.Equal(t, "# stellar copy of some theme", env.ReadFile(newInfo.Path)) // ...and the genuine original at 1.0.toml is never clobbered. assert.Equal(t, "# PRECIOUS ORIGINAL", env.ReadFile(origBackupPath), "original backup must be preserved") @@ -257,12 +262,13 @@ func TestApplyTheme_VersionedBackup(t *testing.T) { env.CreateStarshipConfig(firstOriginal) themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) - firstBackup, err := ApplyTheme(themePath1, &config.Config{}) + firstInfo, err := ApplyTheme(themePath1, &config.Config{}) require.NoError(t, err) + require.NotNil(t, firstInfo) expectedFirstBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "1.0.toml") - assert.Equal(t, expectedFirstBackup, firstBackup) - assert.Equal(t, firstOriginal, env.ReadFile(firstBackup)) + assert.Equal(t, expectedFirstBackup, firstInfo.Path) + assert.Equal(t, firstOriginal, env.ReadFile(firstInfo.Path)) // The user later replaces starship.toml with a new hand-written config // (a regular file, not the symlink stellar just created). @@ -271,14 +277,15 @@ func TestApplyTheme_VersionedBackup(t *testing.T) { env.CreateStarshipConfig(secondOriginal) themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) - secondBackup, err := ApplyTheme(themePath2, &config.Config{}) + secondInfo, err := ApplyTheme(themePath2, &config.Config{}) require.NoError(t, err) // The second original is preserved as 2.0.toml, and 1.0.toml is unchanged. + require.NotNil(t, secondInfo) expectedSecondBackup := filepath.Join(env.StellarDir, BackupAuthor(), "backup", "2.0.toml") - assert.Equal(t, expectedSecondBackup, secondBackup) - assert.Equal(t, secondOriginal, env.ReadFile(secondBackup)) - assert.Equal(t, firstOriginal, env.ReadFile(firstBackup), "earlier backup must stay intact") + assert.Equal(t, expectedSecondBackup, secondInfo.Path) + assert.Equal(t, secondOriginal, env.ReadFile(secondInfo.Path)) + assert.Equal(t, firstOriginal, env.ReadFile(firstInfo.Path), "earlier backup must stay intact") } func TestSanitizeBackupAuthor(t *testing.T) { @@ -302,29 +309,38 @@ func TestSanitizeBackupAuthor(t *testing.T) { } } -func TestBackupIdentifier(t *testing.T) { - tests := []struct { - name string - backupPath string - want string - }{ - { - name: "standard path", - backupPath: filepath.Join("/home/user/.config/stellar", "kurt", "backup", "1.0.toml"), - want: "kurt/backup@1.0", - }, - { - name: "higher version", - backupPath: filepath.Join("/home/user/.config/stellar", "john-doe", "backup", "3.0.toml"), - want: "john-doe/backup@3.0", - }, - } +// TestBackupInfo_Identifier verifies that BackupInfo.Identifier is built from +// the same author/version values used to construct BackupInfo.Path (rather +// than re-derived by parsing the path afterward), and that it always targets +// the backup that was actually just written. +func TestBackupInfo_Identifier(t *testing.T) { + testutil.RequireSymlinks(t) + env := testutil.SetupTestEnv(t) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, BackupIdentifier(tt.backupPath)) - }) - } + // First unmanaged config: backed up as version 1.0. + env.CreateStarshipConfig("# FIRST ORIGINAL") + themePath1 := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + firstInfo, err := ApplyTheme(themePath1, &config.Config{}) + require.NoError(t, err) + + require.NotNil(t, firstInfo) + assert.Equal(t, BackupAuthor()+"/backup@1.0", firstInfo.Identifier) + + // A second, later unmanaged config gets the next version (2.0), and its + // identifier tracks that version rather than staying pinned at 1.0. + require.NoError(t, os.Remove(env.StarshipPath)) + env.CreateStarshipConfig("# SECOND ORIGINAL") + themePath2 := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + secondInfo, err := ApplyTheme(themePath2, &config.Config{}) + require.NoError(t, err) + + require.NotNil(t, secondInfo) + assert.Equal(t, BackupAuthor()+"/backup@2.0", secondInfo.Identifier) + + // The identifier must parse back into a theme that targets the file + // actually written to disk. + parsedVersion := strings.TrimSuffix(filepath.Base(secondInfo.Path), ".toml") + assert.True(t, strings.HasSuffix(secondInfo.Identifier, "/backup@"+parsedVersion)) } func TestGetCurrentTarget(t *testing.T) { From 607f6c9afaf3f46090f6e64d825495880f583e7e Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 10:59:43 +0200 Subject: [PATCH 04/12] fix(update): harden self-update failure paths and leftover cleanup - Preserve the checksum-verified download when the Windows binary swap double-fails (rename to .old succeeded, install and restore both failed): replaceExecutable now owns the temp file's fate and returns recovery instructions instead of letting the caller delete the only good binary. - Run update-leftover cleanup on every OS, not just Windows: temp files are created next to the binary everywhere, so an interrupted update on Linux/macOS no longer litters the bin dir forever. - Match leftovers with os.ReadDir + prefix instead of filepath.Glob, which misinterpreted glob metacharacters in the install path (e.g. tools[1]) and silently cleaned nothing. - Turn the GitHub release endpoints into package vars as an e2e test seam. Co-Authored-By: Claude Fable 5 --- cmd/update.go | 110 ++++++++++++++++++++++++++++++++------------- cmd/update_test.go | 53 ++++++++++++++++++++-- cmd/version.go | 8 +++- 3 files changed, 135 insertions(+), 36 deletions(-) diff --git a/cmd/update.go b/cmd/update.go index 748f5a8..1a1322e 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -15,9 +15,11 @@ import ( "github.com/spf13/cobra" ) -const ( - LatestReleaseURL = "https://github.com/a3chron/stellar/releases/latest/download" -) +// LatestReleaseURL is the base URL for downloading the latest release's +// assets (checksums.txt and the per-platform binaries). It is a var rather +// than a const so tests can point it at an httptest server; production +// behavior is unchanged since the default is assigned here. +var LatestReleaseURL = "https://github.com/a3chron/stellar/releases/latest/download" // fetchChecksums downloads checksums.txt from GitHub releases func fetchChecksums() (string, error) { @@ -201,29 +203,29 @@ var updateCmd = &cobra.Command{ return err } + // replaceExecutable owns the fate of tmpPath from here on: on success it + // has been renamed into place, and on failure it has either removed it + // (nothing of value lost) or deliberately preserved it for manual + // recovery (with instructions in the returned error). The caller must + // not call cleanup() itself, or it could delete the only verified copy + // of the new binary out from under a recovery path. if err := replaceExecutable(tmpPath, execPath); err != nil { - cleanup() return err } - // No cleanup needed - temp file was successfully moved color.Green("Successfully updated to version %s!", latestVersion) return nil }, } -// cleanupUpdateLeftovers is the PersistentPreRunE gate: it only does anything -// on Windows, since that's the only platform replaceExecutable leaves a ".old" -// binary behind on (a running .exe can't be deleted, only renamed out of the -// way) and the only platform a ".stellar-update-*" temp file can survive a -// crashed update (Unix error paths already remove their own temp file). Gating -// here — rather than inside removeUpdateLeftovers — keeps the glob/remove logic -// itself trivially testable on any OS. +// cleanupUpdateLeftovers is the PersistentPreRunE gate: it best-effort removes +// artifacts a previous "stellar update" run may have left behind — a ".old" +// binary (Windows renames the running .exe aside rather than overwriting it, +// since a running .exe can't be deleted) and any ".stellar-update-*" temp file +// that survived an interrupted update (e.g. Ctrl-C or SIGKILL before the +// update command's own cleanup ran). Temp files are created next to the +// binary on every OS, so this cleanup runs on every OS, not just Windows. func cleanupUpdateLeftovers() { - if runtime.GOOS != "windows" { - return - } - execPath, err := os.Executable() if err != nil { return @@ -237,44 +239,90 @@ func cleanupUpdateLeftovers() { // so a broad "*.old" glob is off-limits. Every removal is best-effort and safe // to call when nothing is left over. // +// Directory entries are listed with os.ReadDir and matched with +// strings.HasPrefix rather than filepath.Glob: Glob interprets "[", "]", "*" +// etc. in the directory path itself as pattern metacharacters, so an install +// dir containing one of those characters (e.g. "tools[1]") could silently +// corrupt the pattern or return ErrBadPattern, both of which left leftovers +// on disk forever. +// // This runs from PersistentPreRunE, before the update command creates its own // temp file, so it never deletes an in-progress download of the current // process. A concurrent "stellar update" in another process could theoretically -// have its temp file removed here; that race is accepted, and is Windows-only -// per the gate in cleanupUpdateLeftovers. +// have its temp file removed here; that race is accepted as unlikely. func removeUpdateLeftovers(execPath string) { _ = os.Remove(execPath + ".old") - matches, err := filepath.Glob(filepath.Join(filepath.Dir(execPath), ".stellar-update-*")) + dir := filepath.Dir(execPath) + entries, err := os.ReadDir(dir) if err != nil { return } - for _, m := range matches { - _ = os.Remove(m) + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".stellar-update-") { + _ = os.Remove(filepath.Join(dir, entry.Name())) + } } } -// replaceExecutable swaps the running binary at execPath for the file at newPath. +// replaceExecutable swaps the running binary at execPath for the checksum- +// verified file at newPath. It owns newPath's fate entirely: on success +// newPath has been renamed into place and no longer exists; on failure it has +// either removed newPath (nothing of value was lost) or deliberately +// preserved it with recovery instructions in the returned error. Callers must +// not remove newPath themselves after calling this. +// +// On Unix a plain rename over the target works, and a failure means execPath +// was never touched, so newPath is simply discarded. // -// On Unix a plain rename over the target works. On Windows a running .exe cannot -// be overwritten, but it can be renamed: move the current binary aside, put the -// new one in its place. The old binary stays locked until this process exits, -// so it is deliberately left in place rather than removed here — cleanup -// happens on the next stellar run via cleanupUpdateLeftovers. +// On Windows a running .exe cannot be overwritten, but it can be renamed: +// move the current binary aside, then put the new one in its place. If +// putting the new binary in place fails, we try to restore the original +// binary automatically. If even that restore fails (execPath now missing +// entirely), newPath is deliberately NOT removed — it is the only verified +// copy of the new binary — and the error tells the user exactly what to +// rename to recover. func replaceExecutable(newPath, execPath string) error { if runtime.GOOS != "windows" { - return os.Rename(newPath, execPath) + if err := os.Rename(newPath, execPath); err != nil { + _ = os.Remove(newPath) + return err + } + return nil } oldPath := execPath + ".old" _ = os.Remove(oldPath) // clear any leftover from a previous update + if err := os.Rename(execPath, oldPath); err != nil { + // execPath was never touched, so the verified download isn't needed. + _ = os.Remove(newPath) return err } + if err := os.Rename(newPath, execPath); err != nil { - // Restore the original binary so the user isn't left without one - _ = os.Rename(oldPath, execPath) - return err + if restoreErr := os.Rename(oldPath, execPath); restoreErr == nil { + // Original binary restored; the verified download isn't needed. + _ = os.Remove(newPath) + return err + } + + // Double failure: execPath is now missing entirely. Preserve newPath + // (the verified new binary) and oldPath (the previous binary) and + // tell the user exactly how to recover instead of silently deleting + // the one file that's known-good. + return fmt.Errorf( + "failed to install update and could not restore the previous binary: %w\n\n"+ + "Your files were not deleted, but manual recovery is needed:\n"+ + " - verified new binary: %s\n"+ + " - previous binary: %s\n\n"+ + "To finish the update yourself, run:\n"+ + " move %q %q", + err, newPath, oldPath, newPath, execPath, + ) } + + // New binary is now in place; the old one stays locked until this process + // exits, so it is left for cleanupUpdateLeftovers on the next stellar run. return nil } diff --git a/cmd/update_test.go b/cmd/update_test.go index 128eae6..f2bc2f5 100644 --- a/cmd/update_test.go +++ b/cmd/update_test.go @@ -44,11 +44,56 @@ func TestRemoveUpdateLeftovers_NothingToRemove(t *testing.T) { }) } -// TestCleanupUpdateLeftovers_GateIsSafe verifies the PersistentPreRunE gate -// itself never panics or errors when called directly (it early-returns on -// non-Windows platforms, which covers CI). -func TestCleanupUpdateLeftovers_GateIsSafe(t *testing.T) { +// TestRemoveUpdateLeftovers_BracketDirName proves the fix for the +// filepath.Glob metacharacter bug: Glob interprets "[", "]" etc. in the +// directory path itself as pattern syntax, so an install dir like +// "tools[1]" used to silently disable cleanup (either matching nothing, or +// hitting ErrBadPattern, which was swallowed). os.ReadDir + strings.HasPrefix +// has no such issue since the directory is opened directly rather than +// pattern-matched. +func TestRemoveUpdateLeftovers_BracketDirName(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "tools[1]") + require.NoError(t, os.Mkdir(dir, 0755)) + + execPath := filepath.Join(dir, "stellar.exe") + oldBinary := execPath + ".old" + tmpUpdate := filepath.Join(dir, ".stellar-update-xyz789") + + require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) + require.NoError(t, os.WriteFile(tmpUpdate, []byte("partial download"), 0644)) + + removeUpdateLeftovers(execPath) + + assert.NoFileExists(t, oldBinary, "leftover .old binary should be removed even when the dir name contains glob metacharacters") + assert.NoFileExists(t, tmpUpdate, "leftover update temp file should be removed even when the dir name contains glob metacharacters") +} + +// TestCleanupUpdateLeftovers_RemovesRealArtifacts verifies the +// PersistentPreRunE gate cleans up leftovers next to the actual running +// binary (os.Executable() in a test resolves to the compiled test binary). +// This now runs unconditionally on every OS: temp files are created next to +// the binary regardless of platform, so an interrupted update (Ctrl-C, +// SIGKILL) would litter the bin dir on Linux/macOS too if cleanup were still +// gated to Windows only. +func TestCleanupUpdateLeftovers_RemovesRealArtifacts(t *testing.T) { + execPath, err := os.Executable() + require.NoError(t, err) + + oldBinary := execPath + ".old" + tmpUpdate := filepath.Join(filepath.Dir(execPath), ".stellar-update-test123") + + require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) + require.NoError(t, os.WriteFile(tmpUpdate, []byte("partial download"), 0644)) + t.Cleanup(func() { + _ = os.Remove(oldBinary) + _ = os.Remove(tmpUpdate) + }) + require.NotPanics(t, func() { cleanupUpdateLeftovers() }) + + assert.NoFileExists(t, oldBinary, "leftover .old binary next to the real binary should be removed") + assert.NoFileExists(t, tmpUpdate, "leftover update temp file next to the real binary should be removed") } diff --git a/cmd/version.go b/cmd/version.go index d163f26..aa521b9 100644 --- a/cmd/version.go +++ b/cmd/version.go @@ -27,6 +27,12 @@ var ( commit: "none", date: "unknown", } + + // LatestReleaseAPIURL is the GitHub API endpoint used to fetch the latest + // release's metadata (tag, publish date, HTML URL). It is a var rather + // than a const so tests can point it at an httptest server; production + // behavior is unchanged since the default is assigned here. + LatestReleaseAPIURL = "https://api.github.com/repos/a3chron/stellar/releases/latest" ) // SetVersionInfo is called from main to set version information @@ -128,7 +134,7 @@ func getVersionAsciiArt() string { func GetLatestRelease() (*GitHubRelease, error) { client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get("https://api.github.com/repos/a3chron/stellar/releases/latest") + resp, err := client.Get(LatestReleaseAPIURL) if err != nil { return nil, fmt.Errorf("failed to check for updates: %w", err) } From e4c48287fc530fd05cddb8fc08f266fba546e0f4 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:00:01 +0200 Subject: [PATCH 05/12] fix(current): verify starship.toml content instead of reporting false-healthy The regular-file branch reported a healthy applied theme as long as the cached theme file existed, so a hand-modified or replaced starship.toml was misreported as the current theme (a regression from the symlink-only check on main). Compare the file's hash against cfg.AppliedHash (falling back to the cached theme file for legacy configs) and report 'modified or replaced' with a re-apply hint on mismatch. Also flag an empty CurrentPath and a directory at the config path as broken instead of skipping validation. Co-Authored-By: Claude Fable 5 --- cmd/current.go | 63 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/cmd/current.go b/cmd/current.go index e0e1470..3f6e9f5 100644 --- a/cmd/current.go +++ b/cmd/current.go @@ -63,21 +63,64 @@ var currentCmd = &cobra.Command{ return nil } + case statErr == nil && info.IsDir(): + // Something (not stellar) replaced starship.toml with a + // directory. Neither symlink nor copy mode ever produces this, + // so there's nothing further to check. + color.Red("Starship config path is a directory, not a file") + fmt.Printf("Config says: %s\n", cfg.CurrentTheme) + fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + return nil + case statErr == nil: // Regular file (copy mode, or a symlink-mode config someone - // hand-edited into a plain file). There's no link to read, so the - // only diagnostic available is whether the cached theme file - // we'd re-download from is still around. - if cfg.CurrentPath != "" { - if _, err := os.Stat(cfg.CurrentPath); os.IsNotExist(err) { - color.Red("Theme file missing") - fmt.Printf("Theme: %s\n", cfg.CurrentTheme) - fmt.Printf("Cached theme file missing, expected at: %s\n", cfg.CurrentPath) - fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) - return nil + // hand-edited into a plain file). There's no link to read, but + // unlike a plain existence check, AppliedHash (recorded by + // ApplyTheme/rollback whenever they write starship.toml) lets us + // verify the file's *content* still matches what was applied, + // not just that some file is present at cfg.CurrentPath. + if cfg.CurrentPath == "" { + color.Red("Current theme path is unknown") + fmt.Printf("Config says: %s\n", cfg.CurrentTheme) + fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + return nil + } + + if _, err := os.Stat(cfg.CurrentPath); os.IsNotExist(err) { + color.Red("Theme file missing") + fmt.Printf("Theme: %s\n", cfg.CurrentTheme) + fmt.Printf("Cached theme file missing, expected at: %s\n", cfg.CurrentPath) + fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) + return nil + } + + // Content check: does starship.toml still hold what was actually + // applied? Prefer AppliedHash, which is mode/OS independent and + // recorded at apply time; fall back to comparing directly + // against the cached theme file for legacy configs saved before + // AppliedHash existed. A hashing failure is treated as "can't + // tell" rather than "modified", so it fails open instead of + // reporting a false positive. + modified := false + if cfg.AppliedHash != "" { + if actualHash, herr := symlink.HashFile(starshipConfig); herr == nil { + modified = actualHash != cfg.AppliedHash + } + } else if currentHash, cerr := symlink.HashFile(starshipConfig); cerr == nil { + if cachedHash, cerr2 := symlink.HashFile(cfg.CurrentPath); cerr2 == nil { + modified = currentHash != cachedHash } } + if modified { + color.Red("Starship config was modified or replaced") + fmt.Printf("Theme: %s\n", cfg.CurrentTheme) + fmt.Println("starship.toml no longer matches the theme that was applied.") + fmt.Println("The next `stellar apply` will automatically back up your current starship.toml before applying.") + fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + return nil + } + default: // Some other Lstat error (e.g. permission denied). Surface it // rather than silently reporting a healthy state. From d88015a9a8af149ec120d9572982ed629cc381c3 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:00:14 +0200 Subject: [PATCH 06/12] fix(clean): never delete original-config backups from the cache Backups of the user's original starship.toml live at /backup inside the cache tree, so 'stellar clean' (and --all) swept them away and made the printed restore hint permanently unrecoverable ('backup' is not a real hub author, so there is nothing to re-download). Skip backup themes during cleaning; they remain removable explicitly via 'stellar remove'. Co-Authored-By: Claude Fable 5 --- README.md | 2 ++ internal/cache/manager.go | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/README.md b/README.md index aec834a..ae49e0d 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,8 @@ This also works in copy mode (the Windows default): if you edit the applied `sta Stellar recognizes its own applied file by a checksum recorded in `~/.config/stellar/config.json`, independent of apply mode (symlink or copy) or OS. This means editing a cached theme file directly and re-applying it, or running `stellar clean`/`stellar clean --all` and then applying another theme, never creates a spurious backup — only a config file you actually hand-edited yourself gets preserved. +`stellar clean` (with or without `--all`) never deletes your backups either — they're preserved automatically and only removed if you explicitly run `stellar remove /backup`. + This ensures your carefully crafted config is never lost :) You can apply the newest backup anytime with: ```bash stellar apply /backup diff --git a/internal/cache/manager.go b/internal/cache/manager.go index 83fc987..c52711e 100644 --- a/internal/cache/manager.go +++ b/internal/cache/manager.go @@ -107,6 +107,22 @@ func CleanCache(excludeCurrentPath string) error { continue } + // Backups of the user's original config live at /backup + // (see internal/symlink.backupOriginalConfig) and must survive + // cleaning: they're the only copy of whatever starship.toml the user + // had before ever running stellar. If clean swept them up, the + // restore hint stellar itself prints ("stellar apply + // /backup@") would become permanently + // unrecoverable, since "backup" isn't a real author on the hub and + // there's nothing to re-download. This applies to every author, and + // to every clean variant (CleanCache is the only removal path for + // both `stellar clean` and `stellar clean --all`; they differ only + // in excludeCurrentPath). Backups are still removable explicitly via + // `stellar remove /backup[@version]`. + if t.Name == "backup" { + continue + } + path, err := t.CachePath() if err != nil { continue From dc92b9ab039f598d6cf3121e58cb6f7dd8d50be8 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:00:29 +0200 Subject: [PATCH 07/12] fix(install): force TLS 1.2 capability and fix PATH membership check - OR TLS 1.2 into SecurityProtocol so Windows PowerShell 5.1 on pre-.NET-4.7 defaults (TLS 1.0 only) can reach GitHub instead of failing the download. - Expand registry PATH entries before comparing against the install dir, so an existing unexpanded entry (e.g. %LOCALAPPDATA%\stellar\bin) is recognized and no duplicate is appended; the raw REG_EXPAND_SZ value is still written back untouched. - Merge the duplicated if ($RawPath) blocks computing NewPath and Kind. Co-Authored-By: Claude Fable 5 --- install.ps1 | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/install.ps1 b/install.ps1 index f212b3a..634a6b3 100644 --- a/install.ps1 +++ b/install.ps1 @@ -4,6 +4,11 @@ $ErrorActionPreference = "Stop" +# 3072 = Tls12; needed on WinPS 5.1 with old .NET defaults (TLS 1.0 only), +# which otherwise fail Invoke-WebRequest against github.com with +# "Could not create SSL/TLS secure channel". -bor preserves existing protocols. +[Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor 3072 + # Configurable install directory (override with $env:STELLAR_INSTALL_DIR) $BinDir = if ($env:STELLAR_INSTALL_DIR) { $env:STELLAR_INSTALL_DIR } else { Join-Path $env:LOCALAPPDATA "stellar\bin" } @@ -91,7 +96,12 @@ $EnvKey = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey('Environment', $tru $RawPath = $EnvKey.GetValue('Path', '', [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames) $Normalized = $BinDir.TrimEnd('\') -$PathEntries = ($RawPath -split ';') | ForEach-Object { $_.TrimEnd('\') } | Where-Object { $_ } +# Compare the EXPANDED form of each raw entry against the expanded $BinDir, so +# an existing entry stored unexpanded (e.g. %LOCALAPPDATA%\stellar\bin) is +# still recognized as a match instead of getting a redundant duplicate +# appended. $RawPath itself stays untouched (see below) so REG_EXPAND_SZ +# entries are preserved as-is when we rewrite the value. +$PathEntries = ($RawPath -split ';') | ForEach-Object { [Environment]::ExpandEnvironmentVariables($_).TrimEnd('\') } | Where-Object { $_ } if ($PathEntries -notcontains $Normalized) { Write-Host "" Write-Host "Adding $BinDir to your user PATH" @@ -99,15 +109,10 @@ if ($PathEntries -notcontains $Normalized) { # PowerShell 5.1 has no ternary operator, so use if/else explicitly. if ($RawPath) { $NewPath = "$RawPath;$BinDir" - } - else { - $NewPath = $BinDir - } - - if ($RawPath) { $Kind = $EnvKey.GetValueKind('Path') } else { + $NewPath = $BinDir $Kind = [Microsoft.Win32.RegistryValueKind]::ExpandString } From d7d3eb4e743b6bf073a3cfe3f5e534a80eecdf7e Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:00:39 +0200 Subject: [PATCH 08/12] test(e2e): cover self-update, current diagnostics, and clean backup safety - TestE2E_Update: successful update, already-up-to-date, and checksum- mismatch paths against an httptest server via the release-URL seams, with the test binary snapshotted and restored around the swap. - stellar current: healthy untouched copy-mode apply, hand-edited config reported as modified, empty current path and directory-at-config-path reported as broken. - stellar clean / clean --all: original-config backups survive and the printed restore hint still applies. Co-Authored-By: Claude Fable 5 --- cmd/e2e_test.go | 356 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 356 insertions(+) diff --git a/cmd/e2e_test.go b/cmd/e2e_test.go index ae8b227..f482a69 100644 --- a/cmd/e2e_test.go +++ b/cmd/e2e_test.go @@ -4,8 +4,14 @@ package cmd import ( "bytes" + "crypto/sha256" + "encoding/hex" + "fmt" + "net/http" + "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" @@ -673,6 +679,108 @@ func TestE2E_Current(t *testing.T) { err := cmd.Execute() assert.NoError(t, err) }) + + t.Run("Copy-mode apply left untouched reports healthy", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + + applyCmd := NewRootCmd() + applyCmd.SetArgs([]string{"apply", "alice/rainbow@1.0"}) + applyCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, applyCmd.Execute()) + resetFlags() + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "Current Theme") + assert.NotContains(t, output, "modified") + }) + + t.Run("Copy-mode apply then hand-edit reports modified, not healthy", func(t *testing.T) { + // Regression: current.go used to only check that cfg.CurrentPath (the + // cached theme file) existed, never comparing starship.toml's actual + // content against cfg.AppliedHash, so a modified/replaced starship.toml + // was reported as a healthy applied theme. + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + resetFlags() + + env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + + applyCmd := NewRootCmd() + applyCmd.SetArgs([]string{"apply", "alice/rainbow@1.0"}) + applyCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, applyCmd.Execute()) + resetFlags() + + // Hand-edit the applied copy directly (no re-apply). + env.CreateStarshipConfig("# hand-edited after apply, no longer matches the theme") + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "modified or replaced") + assert.NotContains(t, output, "Current Theme", "a modified starship.toml must not be reported as healthy") + }) + + t.Run("Empty current path reports broken, not healthy", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + env.CreateStarshipConfig(testutil.SampleTOML()) + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "" +}` + env.CreateConfig(config) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "Current theme path is unknown") + assert.NotContains(t, output, "Current Theme") + }) + + t.Run("Starship config replaced by a directory reports broken", func(t *testing.T) { + env := testutil.SetupTestEnv(t) + + require.NoError(t, os.MkdirAll(env.StarshipPath, 0755)) + + config := `{ + "current_theme": "alice/rainbow@1.0", + "current_path": "` + env.StellarDir + `/alice/rainbow/1.0.toml" +}` + env.CreateConfig(config) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"current"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "directory, not a file") + }) } // ============================================================================= @@ -1132,6 +1240,70 @@ func TestE2E_Clean(t *testing.T) { assert.False(t, env.FileExists(filepath.Join(env.StellarDir, "alice", "rainbow", "1.0.toml"))) assert.False(t, env.FileExists(filepath.Join(env.StellarDir, "alice", "rainbow", "2.0.toml"))) }) + + t.Run("Preserves backups", func(t *testing.T) { + // Regression: CleanCache used to sweep /backup like any other + // cached theme, permanently destroying the user's original config and + // making the printed restore hint ("stellar apply /backup@1.0") + // unrecoverable, since "backup" isn't a real author on the hub. + testutil.RequireSymlinks(t) + env := testutil.SetupTestEnv(t) + resetFlags() + + // An unmanaged starship.toml gets backed up when a theme is applied. + originalContent := "# my hand-written original config" + env.CreateStarshipConfig(originalContent) + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + applyCmd := NewRootCmd() + applyCmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + applyCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, applyCmd.Execute()) + resetFlags() + + backupPath := filepath.Join(env.StellarDir, symlink.BackupAuthor(), "backup", "1.0.toml") + require.True(t, env.FileExists(backupPath), "precondition: backup should exist before cleaning") + + cleanCmd := NewRootCmd() + cleanCmd.SetArgs([]string{"clean"}) + cleanCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cleanCmd.Execute()) + + assert.True(t, env.FileExists(backupPath), "stellar clean must never delete backups of the user's original config") + resetFlags() + + // The restore hint stellar printed must still work. + restoreCmd := NewRootCmd() + restoreCmd.SetArgs([]string{"apply", symlink.BackupAuthor() + "/backup@1.0"}) + restoreCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, restoreCmd.Execute()) + assert.Equal(t, originalContent, env.ReadFile(env.StarshipPath)) + }) + + t.Run("--all also preserves backups", func(t *testing.T) { + testutil.RequireSymlinks(t) + env := testutil.SetupTestEnv(t) + resetFlags() + + env.CreateStarshipConfig("# another hand-written original config") + env.CreateThemeFile("local", "mytheme", "1.0", testutil.SampleTOML()) + + applyCmd := NewRootCmd() + applyCmd.SetArgs([]string{"apply", "local/mytheme@1.0"}) + applyCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, applyCmd.Execute()) + resetFlags() + + backupPath := filepath.Join(env.StellarDir, symlink.BackupAuthor(), "backup", "1.0.toml") + require.True(t, env.FileExists(backupPath), "precondition: backup should exist before cleaning") + + cleanCmd := NewRootCmd() + cleanCmd.SetArgs([]string{"clean", "--all"}) + cleanCmd.SetOut(new(bytes.Buffer)) + require.NoError(t, cleanCmd.Execute()) + + assert.True(t, env.FileExists(backupPath), "stellar clean --all must never delete backups either") + }) } // ============================================================================= @@ -1238,6 +1410,166 @@ func TestE2E_Preview(t *testing.T) { }) } +// ============================================================================= +// Update Tests +// ============================================================================= + +// TestE2E_Update exercises `stellar update` against a fake GitHub-shaped +// server, using the LatestReleaseAPIURL/LatestReleaseURL test seams (see +// cmd/version.go and cmd/update.go). versionInfo is pinned to a known +// non-dev version via SetVersionInfo (and restored afterward) so +// IsUpdateAvailable's comparison is deterministic. +// +// The "successful update" subtest deliberately lets the command rename a +// fresh file over os.Executable() - which in a test binary is the compiled +// go-test executable itself. Overwriting it is safe for *this* running +// process (Linux keeps the old, now-nameless inode mapped until the process +// exits), but backupBinaryForUpdateTest restores the original bytes +// afterward so later test runs (and any cached test binary on disk) aren't +// left corrupted. +func TestE2E_Update(t *testing.T) { + // binaryName mirrors the construction in updateCmd (cmd/update.go). + binaryName := fmt.Sprintf("stellar-%s-%s", runtime.GOOS, runtime.GOARCH) + if runtime.GOOS == "windows" { + binaryName += ".exe" + } + + // pinVersion sets versionInfo to a known non-dev version (IsDev() gates + // all update-checking on the version not being "dev") and restores the + // original values via t.Cleanup. versionInfo is an unexported package-level + // var in this same package, so the test can read/write it directly instead + // of needing an exported getter. + pinVersion := func(t *testing.T, version string) { + t.Helper() + origVersion, origCommit, origDate := versionInfo.version, versionInfo.commit, versionInfo.date + SetVersionInfo(version, "testcommit", "2024-01-01") + t.Cleanup(func() { + SetVersionInfo(origVersion, origCommit, origDate) + }) + } + + // pointAtServer starts an httptest server for mux, redirects both URL + // seams at it, and restores the real GitHub URLs afterward. + pointAtServer := func(t *testing.T, mux *http.ServeMux) { + t.Helper() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + origAPIURL, origReleaseURL := LatestReleaseAPIURL, LatestReleaseURL + LatestReleaseAPIURL = server.URL + "/api/latest" + LatestReleaseURL = server.URL + "/release" + t.Cleanup(func() { + LatestReleaseAPIURL = origAPIURL + LatestReleaseURL = origReleaseURL + }) + } + + releaseJSON := func(tag string) string { + return fmt.Sprintf( + `{"tag_name":%q,"name":%q,"published_at":"2024-01-01T00:00:00Z","html_url":"https://example.com/%s"}`, + tag, tag, tag, + ) + } + + t.Run("Already up to date", func(t *testing.T) { + _ = testutil.SetupTestEnv(t) + pinVersion(t, "1.2.3") + + mux := http.NewServeMux() + mux.HandleFunc("/api/latest", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(releaseJSON("v1.2.3"))) + }) + pointAtServer(t, mux) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"update"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "already on the latest version") + }) + + t.Run("Checksum mismatch aborts and leaves binary untouched", func(t *testing.T) { + _ = testutil.SetupTestEnv(t) + pinVersion(t, "1.0.0") + + fakeBody := []byte("fake stellar binary contents for checksum-mismatch test") + + execPath, err := os.Executable() + require.NoError(t, err) + originalContent, err := os.ReadFile(execPath) + require.NoError(t, err) + + mux := http.NewServeMux() + mux.HandleFunc("/api/latest", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(releaseJSON("v1.1.0"))) + }) + mux.HandleFunc("/release/checksums.txt", func(w http.ResponseWriter, r *http.Request) { + // A well-formed but wrong hash (64 hex chars). + wrongHash := strings.Repeat("a", 64) + _, _ = fmt.Fprintf(w, "%s %s\n", wrongHash, binaryName) + }) + mux.HandleFunc("/release/"+binaryName, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(fakeBody) + }) + pointAtServer(t, mux) + + cmd := NewRootCmd() + cmd.SetArgs([]string{"update"}) + cmd.SetOut(new(bytes.Buffer)) + err = cmd.Execute() + + require.Error(t, err) + assert.Contains(t, err.Error(), "checksum verification failed") + + // Nothing was ever renamed into place: the real binary is untouched. + currentContent, rerr := os.ReadFile(execPath) + require.NoError(t, rerr) + assert.Equal(t, originalContent, currentContent, "binary must be untouched when checksum verification fails") + }) + + t.Run("Successful update replaces the binary", func(t *testing.T) { + _ = testutil.SetupTestEnv(t) + pinVersion(t, "1.0.0") + backupBinaryForUpdateTest(t) + + fakeBody := []byte("fake stellar binary contents for successful-update test") + sum := sha256.Sum256(fakeBody) + hexSum := hex.EncodeToString(sum[:]) + + mux := http.NewServeMux() + mux.HandleFunc("/api/latest", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(releaseJSON("v1.1.0"))) + }) + mux.HandleFunc("/release/checksums.txt", func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprintf(w, "%s %s\n", hexSum, binaryName) + }) + mux.HandleFunc("/release/"+binaryName, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(fakeBody) + }) + pointAtServer(t, mux) + + var execErr error + output := testutil.CaptureOutput(t, func() { + cmd := NewRootCmd() + cmd.SetArgs([]string{"update"}) + cmd.SetOut(new(bytes.Buffer)) + execErr = cmd.Execute() + }) + require.NoError(t, execErr) + assert.Contains(t, output, "Successfully updated to version v1.1.0") + + execPath, err := os.Executable() + require.NoError(t, err) + content, err := os.ReadFile(execPath) + require.NoError(t, err) + assert.Equal(t, fakeBody, content, "the file at os.Executable() should now hold the downloaded content") + }) +} + // ============================================================================= // Helper functions // ============================================================================= @@ -1250,6 +1582,30 @@ func resetFlags() { cleanAll = false } +// backupBinaryForUpdateTest saves the current test binary's bytes and mode +// (os.Executable() in a test process resolves to the compiled test binary) +// and restores them via t.Cleanup. It must be called by any test that lets +// "stellar update" actually replace os.Executable(), so the on-disk test +// binary isn't left holding fake content for later test runs. +func backupBinaryForUpdateTest(t *testing.T) { + t.Helper() + + execPath, err := os.Executable() + require.NoError(t, err) + + original, err := os.ReadFile(execPath) + require.NoError(t, err) + + info, err := os.Stat(execPath) + require.NoError(t, err) + mode := info.Mode() + + t.Cleanup(func() { + _ = os.WriteFile(execPath, original, mode) + _ = os.Chmod(execPath, mode) + }) +} + // init ensures flags are reset at test start func init() { resetFlags() From 8e61418072ca67bd371fa771848095f0cf314e53 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:58:14 +0200 Subject: [PATCH 09/12] refactor: share managed-file predicate, backup name, and platform helpers - export symlink.IsManaged as the single "is starship.toml stellar's own" predicate; backupOriginalConfig and `stellar current` now use the same three-signal check, so current's modified diagnostic can no longer disagree with whether the next apply actually backs the file up - add theme.BackupThemeName so the backup writer (internal/symlink) and the clean-time guard (internal/cache) share one constant - add theme.IsValidIdentifierRune as the single [a-zA-Z0-9_-] definition, used by sanitizeBackupAuthor, with a test pinning it to the parser regex - collapse current.go's four duplicated diagnostic blocks into printReapplyHint / printThemeFileMissing (output unchanged) - extract platformBinaryName shared by updateCmd and the update E2E test Co-Authored-By: Claude Fable 5 --- cmd/current.go | 72 +++++++++++++++----------------- cmd/e2e_test.go | 7 +--- cmd/update.go | 19 ++++++--- internal/cache/manager.go | 2 +- internal/symlink/symlink.go | 56 +++++++++++++++---------- internal/symlink/symlink_test.go | 42 +++++++++++++++++++ internal/theme/parser.go | 18 ++++++++ internal/theme/parser_test.go | 17 ++++++++ 8 files changed, 160 insertions(+), 73 deletions(-) diff --git a/cmd/current.go b/cmd/current.go index 3f6e9f5..b7809a7 100644 --- a/cmd/current.go +++ b/cmd/current.go @@ -10,6 +10,26 @@ import ( "github.com/spf13/cobra" ) +// printReapplyHint prints the two-line diagnostic tail shown whenever +// starship.toml is in a state that a re-apply would fix (missing, broken +// symlink, replaced by a directory, unknown path): what config.json believes +// is applied, and the exact command to re-apply it. +func printReapplyHint(cfg *config.Config) { + fmt.Printf("Config says: %s\n", cfg.CurrentTheme) + fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) +} + +// printThemeFileMissing prints the "Theme file missing" diagnostic. expectedLabel +// is the only part that differs between call sites (the symlink branch points at +// the link target, the regular-file branch at the cached theme file), so it's +// passed in; everything else is identical. +func printThemeFileMissing(cfg *config.Config, expectedLabel string) { + color.Red("Theme file missing") + fmt.Printf("Theme: %s\n", cfg.CurrentTheme) + fmt.Printf("%s: %s\n", expectedLabel, cfg.CurrentPath) + fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) +} + var currentCmd = &cobra.Command{ Use: "current", Short: "Show the currently applied theme", @@ -41,8 +61,7 @@ var currentCmd = &cobra.Command{ switch { case os.IsNotExist(statErr): color.Red("Starship config missing") - fmt.Printf("Config says: %s\n", cfg.CurrentTheme) - fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + printReapplyHint(cfg) return nil case statErr == nil && info.Mode()&os.ModeSymlink != 0: @@ -50,16 +69,12 @@ var currentCmd = &cobra.Command{ target, terr := symlink.GetCurrentTarget() if terr != nil { color.Red("Symlink broken or missing") - fmt.Printf("Config says: %s\n", cfg.CurrentTheme) - fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + printReapplyHint(cfg) return nil } if _, err := os.Stat(target); os.IsNotExist(err) { - color.Red("Theme file missing") - fmt.Printf("Theme: %s\n", cfg.CurrentTheme) - fmt.Printf("Expected at: %s\n", cfg.CurrentPath) - fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) + printThemeFileMissing(cfg, "Expected at") return nil } @@ -68,49 +83,30 @@ var currentCmd = &cobra.Command{ // directory. Neither symlink nor copy mode ever produces this, // so there's nothing further to check. color.Red("Starship config path is a directory, not a file") - fmt.Printf("Config says: %s\n", cfg.CurrentTheme) - fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + printReapplyHint(cfg) return nil case statErr == nil: // Regular file (copy mode, or a symlink-mode config someone - // hand-edited into a plain file). There's no link to read, but - // unlike a plain existence check, AppliedHash (recorded by - // ApplyTheme/rollback whenever they write starship.toml) lets us - // verify the file's *content* still matches what was applied, - // not just that some file is present at cfg.CurrentPath. + // hand-edited into a plain file). There's no link to read, so the + // managed-file check below (IsManaged) verifies the file's + // *content* still matches what was applied, not just that some file + // is present at cfg.CurrentPath. if cfg.CurrentPath == "" { color.Red("Current theme path is unknown") - fmt.Printf("Config says: %s\n", cfg.CurrentTheme) - fmt.Println("\nRe-apply with: stellar apply " + cfg.CurrentTheme) + printReapplyHint(cfg) return nil } if _, err := os.Stat(cfg.CurrentPath); os.IsNotExist(err) { - color.Red("Theme file missing") - fmt.Printf("Theme: %s\n", cfg.CurrentTheme) - fmt.Printf("Cached theme file missing, expected at: %s\n", cfg.CurrentPath) - fmt.Println("\nRe-download with: stellar apply " + cfg.CurrentTheme) + printThemeFileMissing(cfg, "Cached theme file missing, expected at") return nil } - // Content check: does starship.toml still hold what was actually - // applied? Prefer AppliedHash, which is mode/OS independent and - // recorded at apply time; fall back to comparing directly - // against the cached theme file for legacy configs saved before - // AppliedHash existed. A hashing failure is treated as "can't - // tell" rather than "modified", so it fails open instead of - // reporting a false positive. - modified := false - if cfg.AppliedHash != "" { - if actualHash, herr := symlink.HashFile(starshipConfig); herr == nil { - modified = actualHash != cfg.AppliedHash - } - } else if currentHash, cerr := symlink.HashFile(starshipConfig); cerr == nil { - if cachedHash, cerr2 := symlink.HashFile(cfg.CurrentPath); cerr2 == nil { - modified = currentHash != cachedHash - } - } + // Report "modified" using the same predicate apply uses to decide + // whether it would back up this file, so `stellar current` never + // disagrees with what the next `stellar apply` actually does. + modified := !symlink.IsManaged(starshipConfig, cfg) if modified { color.Red("Starship config was modified or replaced") diff --git a/cmd/e2e_test.go b/cmd/e2e_test.go index f482a69..5dbee28 100644 --- a/cmd/e2e_test.go +++ b/cmd/e2e_test.go @@ -11,7 +11,6 @@ import ( "net/http/httptest" "os" "path/filepath" - "runtime" "strings" "testing" @@ -1428,11 +1427,7 @@ func TestE2E_Preview(t *testing.T) { // afterward so later test runs (and any cached test binary on disk) aren't // left corrupted. func TestE2E_Update(t *testing.T) { - // binaryName mirrors the construction in updateCmd (cmd/update.go). - binaryName := fmt.Sprintf("stellar-%s-%s", runtime.GOOS, runtime.GOARCH) - if runtime.GOOS == "windows" { - binaryName += ".exe" - } + binaryName := platformBinaryName() // pinVersion sets versionInfo to a known non-dev version (IsDev() gates // all update-checking on the version not being "dev") and restores the diff --git a/cmd/update.go b/cmd/update.go index 1a1322e..9a0e2e2 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -96,6 +96,18 @@ func verifyChecksum(expected, actual, binaryName string) error { return nil } +// platformBinaryName returns the release asset name for the running platform, +// matching the per-platform artifact names goreleaser produces +// ("stellar--", with a ".exe" suffix on Windows). It is the single +// definition of that name, shared by updateCmd and the update E2E test. +func platformBinaryName() string { + name := fmt.Sprintf("stellar-%s-%s", runtime.GOOS, runtime.GOARCH) + if runtime.GOOS == "windows" { + name += ".exe" + } + return name +} + var updateCmd = &cobra.Command{ Use: "update", Short: "Update stellar CLI to the latest version", @@ -115,11 +127,8 @@ var updateCmd = &cobra.Command{ color.Yellow("Updating to version %s...", latestVersion) - // Construct binary name based on OS/arch (matches goreleaser artifact names) - binary := fmt.Sprintf("stellar-%s-%s", runtime.GOOS, runtime.GOARCH) - if runtime.GOOS == "windows" { - binary += ".exe" - } + // Release asset name for this platform (matches goreleaser artifact names) + binary := platformBinaryName() // Step 1: Fetch checksums.txt for verification color.Yellow("Fetching checksums...") diff --git a/internal/cache/manager.go b/internal/cache/manager.go index c52711e..c44c1fc 100644 --- a/internal/cache/manager.go +++ b/internal/cache/manager.go @@ -119,7 +119,7 @@ func CleanCache(excludeCurrentPath string) error { // both `stellar clean` and `stellar clean --all`; they differ only // in excludeCurrentPath). Backups are still removable explicitly via // `stellar remove /backup[@version]`. - if t.Name == "backup" { + if t.Name == theme.BackupThemeName { continue } diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index 1f5013a..d73bf3a 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -137,7 +137,7 @@ func sanitizeBackupAuthor(name string) string { var b strings.Builder for _, r := range name { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '-' { + if theme.IsValidIdentifierRune(r) { b.WriteRune(r) } else { b.WriteByte('-') @@ -175,6 +175,31 @@ type BackupInfo struct { Identifier string } +// IsManaged reports whether the file at configPath is one stellar itself +// applied, as opposed to a user's own original config that must be preserved. +// +// The check is mode-independent: it never asks whether we're in symlink or copy +// mode, only what's actually on disk and recorded in cfg. Each signal only ever +// *confirms* management, so a false negative just means an unmanaged file is +// (harmlessly) backed up rather than data being lost — it fails safe toward +// preserving the file: +// - configPath is a symlink (symlink mode's own marker), or +// - cfg.AppliedHash is set and matches configPath's current content, or +// - cfg.CurrentTheme is set and configPath's content matches the cached +// theme file byte-for-byte (legacy fallback for configs saved before +// AppliedHash existed). +// +// cfg may be nil, treated the same as an empty config (i.e. no known state, so +// nothing is recognized as managed). +func IsManaged(configPath string, cfg *config.Config) bool { + if cfg == nil { + cfg = &config.Config{} + } + return isSymlink(configPath) || + (cfg.AppliedHash != "" && hashOf(configPath) == cfg.AppliedHash) || + (cfg.CurrentTheme != "" && filesEqual(configPath, cfg.CurrentPath)) +} + // backupOriginalConfig backs up the user's original starship.toml under // ~/.config/stellar//backup/. Backups are versioned: the first one is // 1.0.toml (the user's genuine original), and every later unmanaged @@ -182,39 +207,24 @@ type BackupInfo struct { // …) so no earlier backup is ever clobbered. // Returns a *BackupInfo if a backup was created, nil otherwise. // -// The "is this stellar's own file" check is mode-independent: it never asks -// whether we're in symlink or copy mode, only what's actually on disk and -// recorded in cfg. Each signal only ever *prevents* a backup; a false -// negative just means an extra (harmless) backup, so this fails safe: -// - configPath is a symlink (symlink mode's own marker), or -// - cfg.AppliedHash is set and matches configPath's current content, or -// - cfg.CurrentTheme is set and configPath's content matches the cached -// theme file byte-for-byte (legacy fallback for configs saved before -// AppliedHash existed). +// Whether the current file is stellar's own (and so must not be backed up) is +// decided by IsManaged; an unmanaged file always backs up. // -// cfg may be nil, treated the same as an empty config (i.e. no known state, -// so nothing is recognized as managed and an unmanaged file always backs up). +// cfg may be nil, treated the same as an empty config. func backupOriginalConfig(configPath string, cfg *config.Config) (info *BackupInfo, err error) { // Check if the file exists if _, err := os.Stat(configPath); os.IsNotExist(err) { return nil, nil // No file to back up } - if cfg == nil { - cfg = &config.Config{} - } - - managed := isSymlink(configPath) || - (cfg.AppliedHash != "" && hashOf(configPath) == cfg.AppliedHash) || - (cfg.CurrentTheme != "" && filesEqual(configPath, cfg.CurrentPath)) - if managed { + if IsManaged(configPath, cfg) { return nil, nil } author := BackupAuthor() // Construct backup directory: ~/.config/stellar//backup - backupDir, err := paths.ThemeCacheDir(author, "backup") + backupDir, err := paths.ThemeCacheDir(author, theme.BackupThemeName) if err != nil { return nil, fmt.Errorf("failed to resolve backup directory: %w", err) } @@ -226,7 +236,7 @@ func backupOriginalConfig(configPath string, cfg *config.Config) (info *BackupIn var backupPath, version string for n := 1; ; n++ { version = fmt.Sprintf("%d.0", n) - candidate, perr := paths.ThemeCachePath(author, "backup", version) + candidate, perr := paths.ThemeCachePath(author, theme.BackupThemeName, version) if perr != nil { return nil, fmt.Errorf("failed to resolve backup path: %w", perr) } @@ -249,7 +259,7 @@ func backupOriginalConfig(configPath string, cfg *config.Config) (info *BackupIn // Build the identifier from the same author/version values used above, // via the identifier format theme.Theme.String() already owns, instead of // re-deriving it later by parsing backupPath. - identifier := (&theme.Theme{Author: author, Name: "backup", Version: version}).String() + identifier := (&theme.Theme{Author: author, Name: theme.BackupThemeName, Version: version}).String() return &BackupInfo{Path: backupPath, Identifier: identifier}, nil } diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index ffbb41b..8c174d2 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -288,6 +288,48 @@ func TestApplyTheme_VersionedBackup(t *testing.T) { assert.Equal(t, firstOriginal, env.ReadFile(firstInfo.Path), "earlier backup must stay intact") } +// TestIsManaged exercises the three signals that mark a starship.toml as +// stellar's own, plus the nil-config and unmanaged fail-safe cases. +func TestIsManaged(t *testing.T) { + env := testutil.SetupTestEnv(t) + + themePath := env.CreateThemeFile("alice", "rainbow", "1.0", testutil.SampleTOML()) + + t.Run("nil config is treated as empty and unmanaged", func(t *testing.T) { + env.CreateStarshipConfig(testutil.SampleTOML()) + assert.False(t, IsManaged(env.StarshipPath, nil)) + }) + + t.Run("symlink is managed", func(t *testing.T) { + testutil.RequireSymlinks(t) + require.NoError(t, os.Remove(env.StarshipPath)) + require.NoError(t, os.Symlink(themePath, env.StarshipPath)) + assert.True(t, IsManaged(env.StarshipPath, &config.Config{})) + }) + + t.Run("matching applied hash is managed", func(t *testing.T) { + require.NoError(t, os.Remove(env.StarshipPath)) + env.CreateStarshipConfig(testutil.SampleTOML()) + hash, err := HashFile(env.StarshipPath) + require.NoError(t, err) + assert.True(t, IsManaged(env.StarshipPath, &config.Config{AppliedHash: hash})) + }) + + t.Run("legacy current-path content match is managed", func(t *testing.T) { + require.NoError(t, os.Remove(env.StarshipPath)) + env.CreateStarshipConfig(testutil.SampleTOML()) + cfg := &config.Config{CurrentTheme: "alice/rainbow@1.0", CurrentPath: themePath} + assert.True(t, IsManaged(env.StarshipPath, cfg)) + }) + + t.Run("hand-edited content matching no signal is unmanaged", func(t *testing.T) { + require.NoError(t, os.Remove(env.StarshipPath)) + env.CreateStarshipConfig("# hand-edited, matches nothing") + cfg := &config.Config{CurrentTheme: "alice/rainbow@1.0", CurrentPath: themePath, AppliedHash: "deadbeef"} + assert.False(t, IsManaged(env.StarshipPath, cfg)) + }) +} + func TestSanitizeBackupAuthor(t *testing.T) { tests := []struct { name string diff --git a/internal/theme/parser.go b/internal/theme/parser.go index 262657b..a00e6a0 100644 --- a/internal/theme/parser.go +++ b/internal/theme/parser.go @@ -18,6 +18,24 @@ type Theme struct { VersionExplicit bool // True if version was explicitly specified in the identifier } +// BackupThemeName is the reserved theme name used for backups of the user's +// original starship config (they live at /backup). It is not a real +// theme on the hub: internal/symlink writes backups under it, and +// internal/cache skips it at clean time so a backup is never swept away. +const BackupThemeName = "backup" + +// IsValidIdentifierRune reports whether r is allowed inside an author or name +// segment of a theme identifier. It is the single definition of that character +// class ([a-zA-Z0-9_-]); ParseIdentifier's regex below and any caller that +// sanitizes input (e.g. internal/symlink.sanitizeBackupAuthor) must agree with +// it, so the two can't silently drift. +func IsValidIdentifierRune(r rune) bool { + return (r >= 'a' && r <= 'z') || + (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || + r == '_' || r == '-' +} + // ParseIdentifier parses "alice/rainbow@1.2", "alice/rainbow@latest", or "alice/rainbow" func ParseIdentifier(identifier string) (*Theme, error) { // Normalize: remove leading/trailing whitespace diff --git a/internal/theme/parser_test.go b/internal/theme/parser_test.go index 543474f..7b22ff4 100644 --- a/internal/theme/parser_test.go +++ b/internal/theme/parser_test.go @@ -143,6 +143,23 @@ func TestParseIdentifier(t *testing.T) { } } +// TestIsValidIdentifierRune_MatchesParser guards against drift between +// IsValidIdentifierRune and the character class ParseIdentifier's regex +// accepts. For every rune across a broad sample range, building an identifier +// from that single rune must parse iff IsValidIdentifierRune allows it, so the +// two definitions stay in lockstep. +func TestIsValidIdentifierRune_MatchesParser(t *testing.T) { + for r := rune(0); r < 0x300; r++ { + identifier := string(r) + "/" + string(r) + _, err := ParseIdentifier(identifier) + parserAccepts := err == nil + + assert.Equal(t, IsValidIdentifierRune(r), parserAccepts, + "rune %q (U+%04X): IsValidIdentifierRune=%v but parser-accepts=%v", + r, r, IsValidIdentifierRune(r), parserAccepts) + } +} + func TestTheme_String(t *testing.T) { tests := []struct { name string From 407a117d66ce6d06e007ff3334f3849567afb298 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:58:41 +0200 Subject: [PATCH 10/12] fix(symlink): fail loudly when backup slot probing errors instead of hanging The backup version-slot search only exited on ENOENT, so any persistent non-ENOENT stat error (ENOTDIR because /backup exists as a plain file, EACCES on an unreadable dir) spun the loop forever and hung `stellar apply`/`stellar rollback`. Probe errors now abort the apply with a clear error, leaving the user's unmanaged starship.toml untouched. Co-Authored-By: Claude Fable 5 --- internal/symlink/symlink.go | 11 +++++++++- internal/symlink/symlink_test.go | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index d73bf3a..1124efc 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -240,10 +240,19 @@ func backupOriginalConfig(configPath string, cfg *config.Config) (info *BackupIn if perr != nil { return nil, fmt.Errorf("failed to resolve backup path: %w", perr) } - if _, statErr := os.Stat(candidate); os.IsNotExist(statErr) { + // A free slot (ENOENT) ends the search; a slot that already exists + // (statErr == nil) moves to the next version. Any other stat error + // (e.g. ENOTDIR because the backup dir is actually a plain file, or + // EACCES) is persistent and would otherwise spin this loop forever, so + // fail loudly instead of hanging. + _, statErr := os.Stat(candidate) + if os.IsNotExist(statErr) { backupPath = candidate break } + if statErr != nil { + return nil, fmt.Errorf("failed to probe backup slot %s: %w", candidate, statErr) + } } // Create backup directory diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index 8c174d2..ce7b445 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -5,10 +5,12 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/a3chron/stellar/internal/config" "github.com/a3chron/stellar/internal/paths" "github.com/a3chron/stellar/internal/testutil" + "github.com/a3chron/stellar/internal/theme" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -288,6 +290,39 @@ func TestApplyTheme_VersionedBackup(t *testing.T) { assert.Equal(t, firstOriginal, env.ReadFile(firstInfo.Path), "earlier backup must stay intact") } +// TestApplyTheme_BackupSlotAsFileFails is a regression test for a hang: the +// backup version-slot search used to loop forever if os.Stat returned a +// persistent non-ENOENT error. Here //backup exists +// as a plain FILE, so resolving any version slot beneath it fails with ENOTDIR; +// ApplyTheme must return that error promptly instead of spinning. +func TestApplyTheme_BackupSlotAsFileFails(t *testing.T) { + env := testutil.SetupTestEnv(t) + t.Setenv(paths.EnvApplyMode, "copy") + + // Create the backup dir path as a regular file, not a directory. + backupAsFile := filepath.Join(env.StellarDir, BackupAuthor(), theme.BackupThemeName) + require.NoError(t, os.MkdirAll(filepath.Dir(backupAsFile), 0755)) + require.NoError(t, os.WriteFile(backupAsFile, []byte("not a directory"), 0644)) + + // An unmanaged starship.toml so a backup is actually attempted. + env.CreateStarshipConfig("# unmanaged original config") + + themePath := env.CreateThemeFile("bob", "sunset", "2.0", testutil.SampleTOML()) + + done := make(chan error, 1) + go func() { + _, err := ApplyTheme(themePath, &config.Config{}) + done <- err + }() + + select { + case err := <-done: + require.Error(t, err, "backup slot resolution must fail loudly, not silently skip") + case <-time.After(10 * time.Second): + t.Fatal("ApplyTheme hung on the backup slot search instead of failing") + } +} + // TestIsManaged exercises the three signals that mark a starship.toml as // stellar's own, plus the nil-config and unmanaged fail-safe cases. func TestIsManaged(t *testing.T) { From aedf1814360762c1aff503b12f5b85fc324d42a5 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 11:59:07 +0200 Subject: [PATCH 11/12] fix(update): never delete a concurrent update's in-flight download cleanupUpdateLeftovers runs on every command start and removed any .stellar-update-* file next to the binary, including the temp file of a "stellar update" still running in another process, aborting that update at the final rename. Leftover temp files are now only removed once they are older than an hour; an in-flight download's mtime is always fresh. The .old removal stays unconditional since it is never in-flight. Co-Authored-By: Claude Fable 5 --- cmd/update.go | 24 ++++++++++++++++++++---- cmd/update_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/cmd/update.go b/cmd/update.go index 9a0e2e2..bac249c 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -9,6 +9,7 @@ import ( "path/filepath" "runtime" "strings" + "time" "github.com/a3chron/stellar/internal/symlink" "github.com/fatih/color" @@ -257,8 +258,13 @@ func cleanupUpdateLeftovers() { // // This runs from PersistentPreRunE, before the update command creates its own // temp file, so it never deletes an in-progress download of the current -// process. A concurrent "stellar update" in another process could theoretically -// have its temp file removed here; that race is accepted as unlikely. +// process. To also protect a concurrent "stellar update" in another process, a +// ".stellar-update-*" temp file is only removed once it is older than an hour: +// an in-flight download is touched continuously and always younger than that, +// while a genuine leftover from an interrupted update is not. This closes the +// race for any update that finishes in under an hour. The ".old" removal stays +// unconditional: it's the previous update's now-replaced binary, never a file +// an in-flight update is still writing. func removeUpdateLeftovers(execPath string) { _ = os.Remove(execPath + ".old") @@ -268,9 +274,19 @@ func removeUpdateLeftovers(execPath string) { return } for _, entry := range entries { - if strings.HasPrefix(entry.Name(), ".stellar-update-") { - _ = os.Remove(filepath.Join(dir, entry.Name())) + if !strings.HasPrefix(entry.Name(), ".stellar-update-") { + continue + } + info, ierr := entry.Info() + if ierr != nil { + // Can't tell how old it is; leave it rather than risk deleting a + // concurrent update's in-flight download. + continue + } + if time.Since(info.ModTime()) < time.Hour { + continue } + _ = os.Remove(filepath.Join(dir, entry.Name())) } } diff --git a/cmd/update_test.go b/cmd/update_test.go index f2bc2f5..bfe1619 100644 --- a/cmd/update_test.go +++ b/cmd/update_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -26,6 +27,10 @@ func TestRemoveUpdateLeftovers(t *testing.T) { require.NoError(t, os.WriteFile(unrelated, []byte("unrelated"), 0644)) require.NoError(t, os.WriteFile(unrelatedOld, []byte("someone else's leftover"), 0644)) + // Age the temp file past the 1h cutoff so it reads as a genuine leftover + // from an interrupted update, not a concurrent in-flight download. + ageFile(t, tmpUpdate) + removeUpdateLeftovers(execPath) assert.NoFileExists(t, oldBinary, "leftover .old binary should be removed") @@ -34,6 +39,35 @@ func TestRemoveUpdateLeftovers(t *testing.T) { assert.FileExists(t, unrelatedOld, "another tool's .old file must not be touched") } +// ageFile backdates path's mtime well past the 1h cutoff removeUpdateLeftovers +// uses, so a test temp file reads as a genuine interrupted-update leftover +// rather than a concurrent in-flight download. +func ageFile(t *testing.T, path string) { + t.Helper() + old := time.Now().Add(-2 * time.Hour) + require.NoError(t, os.Chtimes(path, old, old)) +} + +// TestRemoveUpdateLeftovers_FreshTempPreserved verifies the race fix: a +// recently-written ".stellar-update-*" temp file (younger than the 1h cutoff) +// is left alone, since it could be a concurrent "stellar update" still writing +// its download. The ".old" binary is still removed unconditionally. +func TestRemoveUpdateLeftovers_FreshTempPreserved(t *testing.T) { + dir := t.TempDir() + + execPath := filepath.Join(dir, "stellar.exe") + oldBinary := execPath + ".old" + freshTmp := filepath.Join(dir, ".stellar-update-inflight") + + require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) + require.NoError(t, os.WriteFile(freshTmp, []byte("in-flight download"), 0644)) + + removeUpdateLeftovers(execPath) + + assert.NoFileExists(t, oldBinary, "leftover .old binary should still be removed") + assert.FileExists(t, freshTmp, "a fresh temp file (possible in-flight update) must not be removed") +} + // TestRemoveUpdateLeftovers_NothingToRemove verifies the worker is a safe // no-op when there's nothing left over from a previous update. func TestRemoveUpdateLeftovers_NothingToRemove(t *testing.T) { @@ -62,6 +96,7 @@ func TestRemoveUpdateLeftovers_BracketDirName(t *testing.T) { require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) require.NoError(t, os.WriteFile(tmpUpdate, []byte("partial download"), 0644)) + ageFile(t, tmpUpdate) removeUpdateLeftovers(execPath) @@ -85,6 +120,7 @@ func TestCleanupUpdateLeftovers_RemovesRealArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(oldBinary, []byte("old binary"), 0644)) require.NoError(t, os.WriteFile(tmpUpdate, []byte("partial download"), 0644)) + ageFile(t, tmpUpdate) t.Cleanup(func() { _ = os.Remove(oldBinary) _ = os.Remove(tmpUpdate) From 367d113c88d95992ca43bbd1c11d04170b605347 Mon Sep 17 00:00:00 2001 From: a3chron Date: Sun, 12 Jul 2026 14:18:06 +0200 Subject: [PATCH 12/12] fix(update): retry transient Windows rename failures and shield recovery files from leftover cleanup On Windows, os.Rename intermittently fails with sharing violations while Defender or the Search Indexer briefly holds a freshly written file. The new symlink.RenameWithRetry rides that out with a bounded backoff (no-op single rename on other OSes) and is used for the theme-apply rename and the self-update binary swap. The self-update double-failure path now also renames the preserved, checksum-verified download to a .stellar-recovery-* name that removeUpdateLeftovers never matches, so a later stellar run can no longer delete the exact file the recovery instructions point at. Co-Authored-By: Claude Fable 5 --- cmd/update.go | 79 ++++++++++++++++++++++++++++---- cmd/update_test.go | 54 ++++++++++++++++++++++ internal/symlink/symlink.go | 69 +++++++++++++++++++++++++++- internal/symlink/symlink_test.go | 72 +++++++++++++++++++++++++++++ 4 files changed, 264 insertions(+), 10 deletions(-) diff --git a/cmd/update.go b/cmd/update.go index bac249c..f8b295d 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -22,6 +22,19 @@ import ( // behavior is unchanged since the default is assigned here. var LatestReleaseURL = "https://github.com/a3chron/stellar/releases/latest/download" +// updateTempPrefix is the filename prefix for the in-progress download temp +// file that "stellar update" writes next to the running binary. It is the +// single string removeUpdateLeftovers matches on to garbage-collect the +// leftovers of an interrupted update. +const updateTempPrefix = ".stellar-update-" + +// updateRecoveryPrefix is the filename prefix for a checksum-verified download +// that survived a double-failure in replaceExecutable and must be preserved for +// manual recovery. It deliberately does NOT begin with updateTempPrefix, so +// removeUpdateLeftovers never matches — and thus never deletes — the exact file +// the recovery instructions tell the user to move into place by hand. +const updateRecoveryPrefix = ".stellar-recovery-" + // fetchChecksums downloads checksums.txt from GitHub releases func fetchChecksums() (string, error) { checksumsURL := fmt.Sprintf("%s/checksums.txt", LatestReleaseURL) @@ -169,7 +182,7 @@ var updateCmd = &cobra.Command{ } // Write to a temporary file in the same directory as the current binary - tmpFile, err := os.CreateTemp(filepath.Dir(execPath), ".stellar-update-*") + tmpFile, err := os.CreateTemp(filepath.Dir(execPath), updateTempPrefix+"*") if err != nil { return err } @@ -265,6 +278,14 @@ func cleanupUpdateLeftovers() { // race for any update that finishes in under an hour. The ".old" removal stays // unconditional: it's the previous update's now-replaced binary, never a file // an in-flight update is still writing. +// +// Recovery files (updateRecoveryPrefix, ".stellar-recovery-*") are deliberately +// NOT matched here and are never auto-removed, regardless of age. They exist +// only after a replaceExecutable double-failure has preserved a checksum- +// verified download the user must move into place by hand; the recovery error +// points straight at one, and the user may not re-run stellar for days. Their +// distinct prefix keeps this cleanup from ever eating them out from under those +// instructions. func removeUpdateLeftovers(execPath string) { _ = os.Remove(execPath + ".old") @@ -274,7 +295,7 @@ func removeUpdateLeftovers(execPath string) { return } for _, entry := range entries { - if !strings.HasPrefix(entry.Name(), ".stellar-update-") { + if !strings.HasPrefix(entry.Name(), updateTempPrefix) { continue } info, ierr := entry.Info() @@ -305,8 +326,11 @@ func removeUpdateLeftovers(execPath string) { // putting the new binary in place fails, we try to restore the original // binary automatically. If even that restore fails (execPath now missing // entirely), newPath is deliberately NOT removed — it is the only verified -// copy of the new binary — and the error tells the user exactly what to -// rename to recover. +// copy of the new binary. Instead it is moved to a ".stellar-recovery-*" name +// (see preserveForRecovery) that removeUpdateLeftovers will never auto-delete, +// and the error tells the user exactly what to rename to recover. All the +// Windows renames go through symlink.RenameWithRetry to ride out the transient +// Defender/indexer locks that plague freshly written .exe files. func replaceExecutable(newPath, execPath string) error { if runtime.GOOS != "windows" { if err := os.Rename(newPath, execPath); err != nil { @@ -319,14 +343,14 @@ func replaceExecutable(newPath, execPath string) error { oldPath := execPath + ".old" _ = os.Remove(oldPath) // clear any leftover from a previous update - if err := os.Rename(execPath, oldPath); err != nil { + if err := symlink.RenameWithRetry(execPath, oldPath); err != nil { // execPath was never touched, so the verified download isn't needed. _ = os.Remove(newPath) return err } - if err := os.Rename(newPath, execPath); err != nil { - if restoreErr := os.Rename(oldPath, execPath); restoreErr == nil { + if err := symlink.RenameWithRetry(newPath, execPath); err != nil { + if restoreErr := symlink.RenameWithRetry(oldPath, execPath); restoreErr == nil { // Original binary restored; the verified download isn't needed. _ = os.Remove(newPath) return err @@ -336,6 +360,14 @@ func replaceExecutable(newPath, execPath string) error { // (the verified new binary) and oldPath (the previous binary) and // tell the user exactly how to recover instead of silently deleting // the one file that's known-good. + // + // Move the download to a ".stellar-recovery-*" name first: it currently + // sits under the ".stellar-update-*" temp name, which a later stellar + // run's removeUpdateLeftovers deletes once it is older than an hour. A + // recovery file has to outlive that window (the user may not re-run + // stellar for days), so it is renamed to the prefix cleanup never + // touches. The error then references wherever the file actually landed. + recoveryPath := preserveForRecovery(newPath) return fmt.Errorf( "failed to install update and could not restore the previous binary: %w\n\n"+ "Your files were not deleted, but manual recovery is needed:\n"+ @@ -343,7 +375,7 @@ func replaceExecutable(newPath, execPath string) error { " - previous binary: %s\n\n"+ "To finish the update yourself, run:\n"+ " move %q %q", - err, newPath, oldPath, newPath, execPath, + err, recoveryPath, oldPath, recoveryPath, execPath, ) } @@ -351,3 +383,34 @@ func replaceExecutable(newPath, execPath string) error { // exits, so it is left for cleanupUpdateLeftovers on the next stellar run. return nil } + +// recoveryPathFor maps a preserved ".stellar-update-" download to the +// ".stellar-recovery-" name it should be moved to before the recovery +// error is shown. The unique suffix is carried across unchanged so two +// concurrent double-failures can't collide on the same recovery filename. If +// tempPath somehow lacks the update prefix (it always has it in practice, being +// the os.CreateTemp name), the base name is used as the suffix so the result is +// still a well-formed recovery sibling in the same directory. +func recoveryPathFor(tempPath string) string { + suffix := strings.TrimPrefix(filepath.Base(tempPath), updateTempPrefix) + return filepath.Join(filepath.Dir(tempPath), updateRecoveryPrefix+suffix) +} + +// preserveForRecovery renames the checksum-verified download at tempPath to its +// ".stellar-recovery-*" sibling so a later removeUpdateLeftovers run can never +// delete it (that cleanup only matches ".stellar-update-*"). It returns the +// path the file actually ended up at, so the caller's recovery instructions +// always point at the real location. +// +// This only runs on replaceExecutable's Windows double-failure path, so it uses +// the same transient-lock retry as the renames there. If the rename still fails, +// it falls back to the original tempPath: keeping the file under its temp name +// is far better than losing it, and the worst case is a much-later cleanup +// removing it rather than the user recovering it in time. +func preserveForRecovery(tempPath string) string { + recoveryPath := recoveryPathFor(tempPath) + if err := symlink.RenameWithRetry(tempPath, recoveryPath); err != nil { + return tempPath + } + return recoveryPath +} diff --git a/cmd/update_test.go b/cmd/update_test.go index bfe1619..72839e2 100644 --- a/cmd/update_test.go +++ b/cmd/update_test.go @@ -3,6 +3,7 @@ package cmd import ( "os" "path/filepath" + "strings" "testing" "time" @@ -104,6 +105,59 @@ func TestRemoveUpdateLeftovers_BracketDirName(t *testing.T) { assert.NoFileExists(t, tmpUpdate, "leftover update temp file should be removed even when the dir name contains glob metacharacters") } +// TestRemoveUpdateLeftovers_RecoveryFilePreserved is the guard for Fix 2: a +// ".stellar-recovery-*" file (the checksum-verified download a double-failure +// preserved for manual recovery) must never be auto-removed, even when it is +// older than the 1h cutoff, while a stale ".stellar-update-*" temp file next to +// it is still cleaned up. Without the distinct prefix, cleanup would silently +// delete the exact file the recovery instructions point at. +func TestRemoveUpdateLeftovers_RecoveryFilePreserved(t *testing.T) { + dir := t.TempDir() + + execPath := filepath.Join(dir, "stellar.exe") + staleUpdate := filepath.Join(dir, ".stellar-update-abc123") + recovery := filepath.Join(dir, ".stellar-recovery-abc123") + + require.NoError(t, os.WriteFile(staleUpdate, []byte("partial download"), 0644)) + require.NoError(t, os.WriteFile(recovery, []byte("verified binary awaiting manual move"), 0644)) + + // Age both past the 1h cutoff: the stale update temp is fair game, the + // recovery file must survive regardless of age. + ageFile(t, staleUpdate) + ageFile(t, recovery) + + removeUpdateLeftovers(execPath) + + assert.NoFileExists(t, staleUpdate, "a stale update temp file should still be removed") + assert.FileExists(t, recovery, "a recovery file must never be auto-removed, even when old") +} + +// TestRecoveryPathFor verifies the temp-to-recovery name mapping keeps the +// unique suffix and switches to a prefix removeUpdateLeftovers never matches. +func TestRecoveryPathFor(t *testing.T) { + tempPath := filepath.Join("/some/bin dir", ".stellar-update-987654") + + got := recoveryPathFor(tempPath) + + assert.Equal(t, filepath.Join("/some/bin dir", ".stellar-recovery-987654"), got) + assert.False(t, strings.HasPrefix(filepath.Base(got), updateTempPrefix), + "recovery name must not carry the cleanup-matched update prefix") +} + +// TestPreserveForRecovery verifies the download is renamed to its recovery +// sibling and the returned path reflects where the file actually ended up. +func TestPreserveForRecovery(t *testing.T) { + dir := t.TempDir() + tempPath := filepath.Join(dir, ".stellar-update-555") + require.NoError(t, os.WriteFile(tempPath, []byte("verified binary"), 0644)) + + got := preserveForRecovery(tempPath) + + assert.Equal(t, filepath.Join(dir, ".stellar-recovery-555"), got) + assert.NoFileExists(t, tempPath, "the temp-named file should have been renamed away") + assert.FileExists(t, got, "the file should now live at its recovery path") +} + // TestCleanupUpdateLeftovers_RemovesRealArtifacts verifies the // PersistentPreRunE gate cleans up leftovers next to the actual running // binary (os.Executable() in a test resolves to the compiled test binary). diff --git a/internal/symlink/symlink.go b/internal/symlink/symlink.go index 1124efc..ea9d2f5 100644 --- a/internal/symlink/symlink.go +++ b/internal/symlink/symlink.go @@ -11,6 +11,7 @@ import ( "path/filepath" "runtime" "strings" + "time" "github.com/a3chron/stellar/internal/config" "github.com/a3chron/stellar/internal/paths" @@ -21,6 +22,68 @@ func StarshipConfigPath() (string, error) { return paths.StarshipConfig() } +// renameRetryEnabled gates RenameWithRetry's retry loop. It is true only on +// Windows, the one platform where os.Rename intermittently fails on a lock that +// clears by itself; everywhere else a rename either succeeds or fails for a +// real, persistent reason, so retrying would only add latency. It's a +// package-level var (rather than an inline runtime.GOOS check) purely so a unit +// test running on Linux can flip it on and exercise the retry path. +var renameRetryEnabled = runtime.GOOS == "windows" + +// renameRetryBackoff is the sequence of sleeps between rename attempts when the +// retry path is active. Four retries at 50/100/200/400ms cap the total added +// latency of a genuinely persistent failure at 750ms — comfortably under a +// second and a half — while giving a transient Defender/indexer lock ample time +// to clear. It's a package-level var so a test can shrink or replace it. +var renameRetryBackoff = []time.Duration{ + 50 * time.Millisecond, + 100 * time.Millisecond, + 200 * time.Millisecond, + 400 * time.Millisecond, +} + +// renameSleep is the sleep used between rename retries, injectable so a unit +// test can verify the retry/backoff behavior without actually sleeping. +var renameSleep = time.Sleep + +// RenameWithRetry renames oldpath to newpath, retrying a few times on Windows +// when the rename fails. +// +// Why: on Windows os.Rename intermittently fails with ERROR_SHARING_VIOLATION +// or ERROR_ACCESS_DENIED when another process holds a brief lock on the source +// or destination. Windows Defender's real-time scan and the Search Indexer both +// do exactly this to freshly written files and brand-new .exe binaries — which +// is precisely what stellar renames when applying a theme (a just-written temp +// file) or installing an update (a just-downloaded executable). These locks +// clear on their own within milliseconds, so a short, bounded retry turns an +// intermittent, user-visible failure into a silent success. +// +// On every other OS renameRetryEnabled is false, so this is a single os.Rename +// with no sleep and no retry — behavior there is identical to calling os.Rename +// directly. +// +// We retry on any error rather than trying to match specific Windows error +// codes: the sharing-violation codes don't map cleanly onto Go's portable error +// types, and retrying indiscriminately is safe here because a genuinely +// persistent failure still returns promptly — the total backoff is bounded by +// renameRetryBackoff. +func RenameWithRetry(oldpath, newpath string) error { + err := os.Rename(oldpath, newpath) + if err == nil || !renameRetryEnabled { + return err + } + + // Retry with growing backoff, returning the last error if none succeed. + for _, backoff := range renameRetryBackoff { + renameSleep(backoff) + if err = os.Rename(oldpath, newpath); err == nil { + return nil + } + } + + return err +} + // IsCopyMode reports whether themes are applied by copying the file instead of // symlinking. This is the default on Windows (where symlinks require Developer // Mode or admin privileges) and can be forced anywhere via STELLAR_APPLY_MODE. @@ -335,8 +398,10 @@ func ApplyTheme(target string, cfg *config.Config) (info *BackupInfo, err error) } // Atomic rename over the target (replaces the destination on POSIX and Windows) - // If this fails, the original file/symlink is still intact - if err := os.Rename(tempPath, configPath); err != nil { + // If this fails, the original file/symlink is still intact. RenameWithRetry + // rides out the transient Defender/indexer locks Windows takes on the + // freshly written temp file (a no-op single rename everywhere else). + if err := RenameWithRetry(tempPath, configPath); err != nil { // Clean up temp file on failure _ = os.Remove(tempPath) return info, fmt.Errorf("failed to replace config: %w", err) diff --git a/internal/symlink/symlink_test.go b/internal/symlink/symlink_test.go index ce7b445..7d6e900 100644 --- a/internal/symlink/symlink_test.go +++ b/internal/symlink/symlink_test.go @@ -467,6 +467,78 @@ func TestIsSymlink(t *testing.T) { assert.False(t, isSymlink("/nonexistent/path")) } +// TestRenameWithRetry exercises the Windows transient-lock retry path on Linux +// by flipping the renameRetryEnabled flag and injecting renameSleep, so no +// actual sleeping happens. os.Rename is driven to fail/succeed by controlling +// whether the source file exists at each attempt. +func TestRenameWithRetry(t *testing.T) { + t.Run("transient failure eventually succeeds", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + dst := filepath.Join(dir, "dst") + + // Force the retry path on even though the test runs on Linux. + oldFlag := renameRetryEnabled + renameRetryEnabled = true + defer func() { renameRetryEnabled = oldFlag }() + + // The source doesn't exist yet, so the first os.Rename fails. On the + // first backoff sleep we create it, standing in for a transient + // Defender/indexer lock clearing so the next attempt succeeds. + sleeps := 0 + oldSleep := renameSleep + renameSleep = func(time.Duration) { + sleeps++ + if sleeps == 1 { + require.NoError(t, os.WriteFile(src, []byte("binary"), 0644)) + } + } + defer func() { renameSleep = oldSleep }() + + require.NoError(t, RenameWithRetry(src, dst)) + assert.Equal(t, 1, sleeps, "should have retried exactly once before succeeding") + assert.FileExists(t, dst) + }) + + t.Run("persistent failure returns the last error", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "never-appears") + dst := filepath.Join(dir, "dst") + + oldFlag := renameRetryEnabled + renameRetryEnabled = true + defer func() { renameRetryEnabled = oldFlag }() + + sleeps := 0 + oldSleep := renameSleep + renameSleep = func(time.Duration) { sleeps++ } + defer func() { renameSleep = oldSleep }() + + err := RenameWithRetry(src, dst) + require.Error(t, err, "a source that never appears must ultimately fail") + assert.Equal(t, len(renameRetryBackoff), sleeps, "should exhaust every backoff step before giving up") + }) + + t.Run("no retry or sleep when the flag is off", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "missing") + dst := filepath.Join(dir, "dst") + + oldFlag := renameRetryEnabled + renameRetryEnabled = false + defer func() { renameRetryEnabled = oldFlag }() + + sleeps := 0 + oldSleep := renameSleep + renameSleep = func(time.Duration) { sleeps++ } + defer func() { renameSleep = oldSleep }() + + err := RenameWithRetry(src, dst) + require.Error(t, err, "single-shot rename of a missing source must fail") + assert.Zero(t, sleeps, "the non-Windows path must not sleep or retry") + }) +} + func TestHashFile(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "sample.toml")