From 8aa15512ec056df61d22604a6edb90f5052efc1f Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Sat, 28 Mar 2026 22:20:44 -0700 Subject: [PATCH 1/5] fix: resolve formula aliases before tracking installed packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Homebrew has formula aliases where the requested name differs from the actual installed name. Examples: - postgresql → installs as postgresql@18 - kubectl → installs as kubernetes-cli This caused packages to be reinstalled on every run because the state file tracked the alias name (e.g., postgresql) but brew list returns the actual name (e.g., postgresql@18), so they never matched during reconciliation. Fix: - Added ResolveFormulaName() function to resolve aliases using brew info --json - Updated all package tracking to use resolved names - Handles both CLI packages and casks Closes #17 (formula alias tracking issue) --- internal/brew/brew.go | 45 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/internal/brew/brew.go b/internal/brew/brew.go index b89c55a..8e74e3f 100644 --- a/internal/brew/brew.go +++ b/internal/brew/brew.go @@ -224,18 +224,22 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm var newCli []string for _, p := range cliPkgs { - if !alreadyFormulae[p] { + // Resolve the formula name to handle aliases before checking if installed + resolvedName := ResolveFormulaName(p) + if !alreadyFormulae[resolvedName] { newCli = append(newCli, p) } else { - installedFormulae = append(installedFormulae, p) + installedFormulae = append(installedFormulae, resolvedName) } } var newCask []string for _, p := range caskPkgs { - if !alreadyCasks[p] { + // Resolve the cask name to handle any aliases + resolvedName := ResolveFormulaName(p) + if !alreadyCasks[resolvedName] { newCask = append(newCask, p) } else { - installedCasks = append(installedCasks, p) + installedCasks = append(installedCasks, resolvedName) } } @@ -267,8 +271,10 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm failedSet[f.name] = true } for _, p := range newCli { + // Resolve the formula name to handle aliases like postgresql → postgresql@18 + resolvedName := ResolveFormulaName(p) if !failedSet[p] { - installedFormulae = append(installedFormulae, p) + installedFormulae = append(installedFormulae, resolvedName) } } allFailed = append(allFailed, failed...) @@ -283,13 +289,15 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm elapsed := time.Since(start) progress.IncrementWithStatus(errMsg == "") duration := ui.FormatDuration(elapsed) + // Resolve the cask name to handle any aliases + resolvedName := ResolveFormulaName(pkg) if errMsg == "" { progress.PrintLine(" %s %s", ui.Green("✔ "+pkg), ui.Cyan("("+duration+")")) - installedCasks = append(installedCasks, pkg) + installedCasks = append(installedCasks, resolvedName) } else { progress.PrintLine(" %s %s", ui.Red("✗ "+pkg+" ("+errMsg+")"), ui.Cyan("("+duration+")")) allFailed = append(allFailed, failedJob{ - installJob: installJob{name: pkg, isCask: true}, + installJob: installJob{name: resolvedName, isCask: true}, errMsg: errMsg, }) } @@ -598,6 +606,29 @@ func parseBrewError(output string) string { } } +// ResolveFormulaName resolves a formula alias to its canonical name. +// This handles cases like "postgresql" → "postgresql@18" or "kubectl" → "kubernetes-cli". +// Returns the original name if resolution fails. +func ResolveFormulaName(name string) string { + cmd := exec.Command("brew", "info", "--json", name) + output, err := cmd.Output() + if err != nil { + return name + } + + var result []struct { + Name string `json:"name"` + } + if err := json.Unmarshal(output, &result); err != nil { + return name + } + + if len(result) > 0 && result[0].Name != "" { + return result[0].Name + } + return name +} + func Uninstall(packages []string, dryRun bool) error { if len(packages) == 0 { return nil From 71afc6975cb400c16bbd91cbf6ccbc6b1956f950 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Sat, 28 Mar 2026 23:47:51 -0700 Subject: [PATCH 2/5] fix: isolate diff tests from local dotfiles repo state --- internal/diff/compare.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/diff/compare.go b/internal/diff/compare.go index 57c292f..c7f414c 100644 --- a/internal/diff/compare.go +++ b/internal/diff/compare.go @@ -171,6 +171,12 @@ func diffDotfiles(systemURL, referenceURL string) *DotfilesDiff { dd.RepoChanged = &ValueChange{System: systemURL, Reference: referenceURL} } + // Only check local dotfiles repo state if dotfiles are actually configured + // If both URLs are empty, there's no dotfiles setup to check + if sysNorm == "" && refNorm == "" { + return dd + } + // Check local dotfiles repo for dirty state home, err := os.UserHomeDir() if err != nil { From 1f9c695ebf0a59ff2c8f50627d9ca58434dc51b3 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Wed, 1 Apr 2026 00:33:59 -0700 Subject: [PATCH 3/5] fix: address review feedback on formula alias resolution - Batch brew info --json calls into single invocation with cached map - Fix retry path to use resolved alias names - Skip alias resolution for casks (no alias system) - Add unit tests for parseFormulaAliases with table-driven cases - Extract identityMap helper to deduplicate fallback logic Co-Authored-By: Claude Opus 4.6 --- internal/brew/brew.go | 94 ++++++++++++++++++++++------------- internal/brew/resolve_test.go | 82 ++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 35 deletions(-) create mode 100644 internal/brew/resolve_test.go diff --git a/internal/brew/brew.go b/internal/brew/brew.go index 8e74e3f..a0e7d6c 100644 --- a/internal/brew/brew.go +++ b/internal/brew/brew.go @@ -222,10 +222,13 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm return nil, nil, fmt.Errorf("list installed packages: %w", checkErr) } + // Batch-resolve formula aliases in a single brew info call. + // Casks don't have an alias system, so we skip resolution for them. + aliasMap := ResolveFormulaNames(cliPkgs) + var newCli []string for _, p := range cliPkgs { - // Resolve the formula name to handle aliases before checking if installed - resolvedName := ResolveFormulaName(p) + resolvedName := aliasMap[p] if !alreadyFormulae[resolvedName] { newCli = append(newCli, p) } else { @@ -234,12 +237,10 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm } var newCask []string for _, p := range caskPkgs { - // Resolve the cask name to handle any aliases - resolvedName := ResolveFormulaName(p) - if !alreadyCasks[resolvedName] { + if !alreadyCasks[p] { newCask = append(newCask, p) } else { - installedCasks = append(installedCasks, resolvedName) + installedCasks = append(installedCasks, p) } } @@ -271,10 +272,8 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm failedSet[f.name] = true } for _, p := range newCli { - // Resolve the formula name to handle aliases like postgresql → postgresql@18 - resolvedName := ResolveFormulaName(p) if !failedSet[p] { - installedFormulae = append(installedFormulae, resolvedName) + installedFormulae = append(installedFormulae, aliasMap[p]) } } allFailed = append(allFailed, failed...) @@ -289,15 +288,13 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm elapsed := time.Since(start) progress.IncrementWithStatus(errMsg == "") duration := ui.FormatDuration(elapsed) - // Resolve the cask name to handle any aliases - resolvedName := ResolveFormulaName(pkg) if errMsg == "" { progress.PrintLine(" %s %s", ui.Green("✔ "+pkg), ui.Cyan("("+duration+")")) - installedCasks = append(installedCasks, resolvedName) + installedCasks = append(installedCasks, pkg) } else { progress.PrintLine(" %s %s", ui.Red("✗ "+pkg+" ("+errMsg+")"), ui.Cyan("("+duration+")")) allFailed = append(allFailed, failedJob{ - installJob: installJob{name: resolvedName, isCask: true}, + installJob: installJob{name: pkg, isCask: true}, errMsg: errMsg, }) } @@ -321,28 +318,31 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm if f.isCask { installedCasks = append(installedCasks, f.name) } else { - installedFormulae = append(installedFormulae, f.name) + installedFormulae = append(installedFormulae, aliasMap[f.name]) } } else { fmt.Printf(" ✗ %s (still failed)\n", f.name) } } - type pkgKey struct { - name string - isCask bool - } - succeeded := make(map[pkgKey]bool) + succeededFormulae := make(map[string]bool, len(installedFormulae)) for _, p := range installedFormulae { - succeeded[pkgKey{p, false}] = true + succeededFormulae[p] = true } + succeededCasks := make(map[string]bool, len(installedCasks)) for _, p := range installedCasks { - succeeded[pkgKey{p, true}] = true + succeededCasks[p] = true } var stillFailed []failedJob for _, f := range allFailed { - if !succeeded[pkgKey{f.name, f.isCask}] { - stillFailed = append(stillFailed, f) + if f.isCask { + if !succeededCasks[f.name] { + stillFailed = append(stillFailed, f) + } + } else { + if !succeededFormulae[aliasMap[f.name]] { + stillFailed = append(stillFailed, f) + } } } allFailed = stillFailed @@ -606,27 +606,51 @@ func parseBrewError(output string) string { } } -// ResolveFormulaName resolves a formula alias to its canonical name. -// This handles cases like "postgresql" → "postgresql@18" or "kubectl" → "kubernetes-cli". -// Returns the original name if resolution fails. -func ResolveFormulaName(name string) string { - cmd := exec.Command("brew", "info", "--json", name) +// ResolveFormulaNames resolves formula aliases to their canonical names in a +// single batched `brew info --json` call. It returns a map from each input name +// to its canonical name. On any error it falls back to an identity mapping. +func ResolveFormulaNames(names []string) map[string]string { + if len(names) == 0 { + return make(map[string]string) + } + + args := append([]string{"info", "--json"}, names...) + cmd := exec.Command("brew", args...) output, err := cmd.Output() if err != nil { - return name + return identityMap(names) } - var result []struct { + return parseFormulaAliases(names, output) +} + +// parseFormulaAliases builds an alias map from the JSON response of +// `brew info --json`. The JSON array is positionally aligned with names. +func parseFormulaAliases(names []string, jsonData []byte) map[string]string { + var entries []struct { Name string `json:"name"` } - if err := json.Unmarshal(output, &result); err != nil { - return name + if err := json.Unmarshal(jsonData, &entries); err != nil { + return identityMap(names) + } + + resolved := make(map[string]string, len(names)) + for i, n := range names { + if i < len(entries) && entries[i].Name != "" { + resolved[n] = entries[i].Name + } else { + resolved[n] = n + } } + return resolved +} - if len(result) > 0 && result[0].Name != "" { - return result[0].Name +func identityMap(names []string) map[string]string { + m := make(map[string]string, len(names)) + for _, n := range names { + m[n] = n } - return name + return m } func Uninstall(packages []string, dryRun bool) error { diff --git a/internal/brew/resolve_test.go b/internal/brew/resolve_test.go new file mode 100644 index 0000000..2e7ac18 --- /dev/null +++ b/internal/brew/resolve_test.go @@ -0,0 +1,82 @@ +package brew + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveFormulaNames_EmptyInput(t *testing.T) { + assert.Empty(t, ResolveFormulaNames(nil)) + assert.Empty(t, ResolveFormulaNames([]string{})) +} + +func TestParseFormulaAliases(t *testing.T) { + tests := []struct { + name string + input []string + json interface{} + expected map[string]string + }{ + { + name: "resolves aliases to canonical names", + input: []string{"postgresql", "kubectl", "vim"}, + json: []struct { + Name string `json:"name"` + }{ + {Name: "postgresql@16"}, + {Name: "kubernetes-cli"}, + {Name: "vim"}, + }, + expected: map[string]string{ + "postgresql": "postgresql@16", + "kubectl": "kubernetes-cli", + "vim": "vim", + }, + }, + { + name: "falls back to identity when response is shorter than input", + input: []string{"foo", "bar", "baz"}, + json: []struct { + Name string `json:"name"` + }{ + {Name: "foo-canonical"}, + }, + expected: map[string]string{ + "foo": "foo-canonical", + "bar": "bar", + "baz": "baz", + }, + }, + { + name: "falls back to identity for empty name fields", + input: []string{"foo", "bar"}, + json: []struct { + Name string `json:"name"` + }{ + {Name: "foo-resolved"}, + {Name: ""}, + }, + expected: map[string]string{ + "foo": "foo-resolved", + "bar": "bar", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + data, err := json.Marshal(tc.json) + require.NoError(t, err) + assert.Equal(t, tc.expected, parseFormulaAliases(tc.input, data)) + }) + } +} + +func TestParseFormulaAliases_InvalidJSON(t *testing.T) { + names := []string{"pkg1", "pkg2"} + result := parseFormulaAliases(names, []byte("not json")) + assert.Equal(t, map[string]string{"pkg1": "pkg1", "pkg2": "pkg2"}, result) +} From 59c83c08c5feffdec7637fe22ee72d75fbcb124f Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Wed, 1 Apr 2026 00:57:27 -0700 Subject: [PATCH 4/5] test: cover alias install progress behavior --- internal/brew/brew.go | 12 +- .../brew/install_progress_behavior_test.go | 182 ++++++++++++++++++ 2 files changed, 191 insertions(+), 3 deletions(-) create mode 100644 internal/brew/install_progress_behavior_test.go diff --git a/internal/brew/brew.go b/internal/brew/brew.go index a0e7d6c..28e897f 100644 --- a/internal/brew/brew.go +++ b/internal/brew/brew.go @@ -18,6 +18,12 @@ import ( const maxWorkers = 1 +// Test seams for install behavior. Production uses the real implementations. +var ( + checkNetworkFunc = CheckNetwork + sleepFunc = time.Sleep +) + type OutdatedPackage struct { Name string Current string @@ -511,7 +517,7 @@ func installFormulaWithError(pkg string) string { errMsg := parseBrewError(outputStr) if attempt < maxAttempts && isRetryableError(errMsg) { delay := time.Duration(attempt) * 2 * time.Second - time.Sleep(delay) + sleepFunc(delay) continue } @@ -561,7 +567,7 @@ func installSmartCaskWithError(pkg string) string { if attempt < maxAttempts && isRetryableError(errMsg) { delay := time.Duration(attempt) * 2 * time.Second - time.Sleep(delay) + sleepFunc(delay) continue } @@ -883,7 +889,7 @@ func DoctorDiagnose() ([]string, error) { func PreInstallChecks(packageCount int) error { ui.Info("Checking network connectivity...") - if err := CheckNetwork(); err != nil { + if err := checkNetworkFunc(); err != nil { return fmt.Errorf("network check failed: %v\nPlease check your internet connection and try again", err) } diff --git a/internal/brew/install_progress_behavior_test.go b/internal/brew/install_progress_behavior_test.go new file mode 100644 index 0000000..642bdff --- /dev/null +++ b/internal/brew/install_progress_behavior_test.go @@ -0,0 +1,182 @@ +package brew + +import ( + "bytes" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func captureOutput(t *testing.T, fn func()) string { + t.Helper() + + oldStdout := os.Stdout + oldStderr := os.Stderr + + r, w, err := os.Pipe() + require.NoError(t, err) + + os.Stdout = w + os.Stderr = w + defer func() { + os.Stdout = oldStdout + os.Stderr = oldStderr + }() + + done := make(chan string, 1) + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + done <- buf.String() + }() + + fn() + + require.NoError(t, w.Close()) + + return <-done +} + +func TestInstallWithProgress_BatchesAliasResolutionAndSkipsCaskAliases(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "brew-calls.log") + + setupFakeBrew(t, fmt.Sprintf(`#!/bin/sh +log_file=%q +printf '%%s\n' "$*" >> "$log_file" + +if [ "$1" = "list" ] && [ "$2" = "--formula" ]; then + exit 0 +fi +if [ "$1" = "list" ] && [ "$2" = "--cask" ]; then + exit 0 +fi +if [ "$1" = "info" ]; then + shift 2 + for arg in "$@"; do + if [ "$arg" = "firefox" ]; then + echo "unexpected cask alias resolution" >&2 + exit 1 + fi + done + cat <<'EOF' +[{"name":"postgresql@16"},{"name":"kubernetes-cli"}] +EOF + exit 0 +fi +if [ "$1" = "update" ]; then + exit 0 +fi +if [ "$1" = "install" ] && [ "$2" = "--cask" ]; then + exit 0 +fi +if [ "$1" = "install" ]; then + exit 0 +fi +exit 0 +`, logPath)) + + originalCheckNetwork := checkNetworkFunc + checkNetworkFunc = func() error { return nil } + t.Cleanup(func() { checkNetworkFunc = originalCheckNetwork }) + + formulae, casks, err := InstallWithProgress([]string{"postgresql", "kubectl"}, []string{"firefox"}, false) + require.NoError(t, err) + assert.Equal(t, []string{"postgresql@16", "kubernetes-cli"}, formulae) + assert.Equal(t, []string{"firefox"}, casks) + + logContent, err := os.ReadFile(logPath) + require.NoError(t, err) + + var infoLines []string + for _, line := range strings.Split(strings.TrimSpace(string(logContent)), "\n") { + if strings.HasPrefix(line, "info --json") { + infoLines = append(infoLines, line) + } + } + + require.Len(t, infoLines, 1) + assert.Equal(t, "info --json postgresql kubectl", infoLines[0]) + assert.NotContains(t, infoLines[0], "firefox") +} + +func TestInstallWithProgress_RetrySuccessTracksCanonicalNames(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "brew-calls.log") + statePath := filepath.Join(tmpDir, "foo-attempts") + + setupFakeBrew(t, fmt.Sprintf(`#!/bin/sh +log_file=%q +state_file=%q +printf '%%s\n' "$*" >> "$log_file" + +if [ "$1" = "list" ] && [ "$2" = "--formula" ]; then + exit 0 +fi +if [ "$1" = "list" ] && [ "$2" = "--cask" ]; then + exit 0 +fi +if [ "$1" = "info" ]; then + cat <<'EOF' +[{"name":"foo-canonical"}] +EOF + exit 0 +fi +if [ "$1" = "update" ]; then + exit 0 +fi +if [ "$1" = "install" ] && [ "$2" = "foo" ]; then + attempt=0 + if [ -f "$state_file" ]; then + attempt=$(cat "$state_file") + fi + attempt=$((attempt + 1)) + echo "$attempt" > "$state_file" + if [ "$attempt" -eq 1 ]; then + echo "Error: Connection timed out while downloading" + exit 1 + fi + exit 0 +fi +if [ "$1" = "install" ] && [ "$2" = "--cask" ]; then + exit 0 +fi +if [ "$1" = "install" ]; then + exit 0 +fi +exit 0 +`, logPath, statePath)) + + originalCheckNetwork := checkNetworkFunc + originalSleep := sleepFunc + checkNetworkFunc = func() error { return nil } + sleepFunc = func(time.Duration) {} + t.Cleanup(func() { + checkNetworkFunc = originalCheckNetwork + sleepFunc = originalSleep + }) + + var formulae, casks []string + var err error + output := captureOutput(t, func() { + formulae, casks, err = InstallWithProgress([]string{"foo"}, nil, false) + }) + + require.NoError(t, err) + assert.Equal(t, []string{"foo-canonical"}, formulae) + assert.Empty(t, casks) + assert.Contains(t, output, "retry succeeded") + assert.NotContains(t, output, "packages failed to install") + + logContent, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.Equal(t, 1, strings.Count(string(logContent), "info --json foo")) + assert.Equal(t, 2, strings.Count(string(logContent), "install foo")) +} From cc1a83def79a98e0f72b2c818a729fdb252be2e9 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Wed, 1 Apr 2026 00:59:44 -0700 Subject: [PATCH 5/5] test: fix retry success expectation --- internal/brew/install_progress_behavior_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/brew/install_progress_behavior_test.go b/internal/brew/install_progress_behavior_test.go index 642bdff..cee4f00 100644 --- a/internal/brew/install_progress_behavior_test.go +++ b/internal/brew/install_progress_behavior_test.go @@ -172,8 +172,8 @@ exit 0 require.NoError(t, err) assert.Equal(t, []string{"foo-canonical"}, formulae) assert.Empty(t, casks) - assert.Contains(t, output, "retry succeeded") assert.NotContains(t, output, "packages failed to install") + assert.Contains(t, output, "✔ foo") logContent, err := os.ReadFile(logPath) require.NoError(t, err)