fix: address PR review feedback for batch install

- Show progress status before batch install starts (PrintLine instead of
  staying at 0% until completion)
- Remove dead code: installCaskWithProgress and printBrewOutput
- Capture brew output for specific error messages per failed package
  (extractPackageError) instead of generic "install failed"
- Add meaningful assertions to TestInstallWithProgress_BatchMode verifying
  batch command format in dry-run output
- Add TestExtractPackageError for the new helper

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: jerryjrxie

internal/brew/brew.go

@@ -259,13 +259,12 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm
 	var allFailed []failedJob
 
 	if len(newCli) > 0 {
-		ui.Info(fmt.Sprintf("Installing %d CLI packages...", len(newCli)))
+		progress.PrintLine("  Installing %d CLI packages via brew install...", len(newCli))
 
 		args := append([]string{"install"}, newCli...)
 		cmd := brewInstallCmd(args...)
-		cmd.Stdout = os.Stdout
-		cmd.Stderr = os.Stderr
-		cmdErr := cmd.Run()
+		cmdOutput, cmdErr := cmd.CombinedOutput()
+		cmdOutputStr := string(cmdOutput)
 
 		// Re-check installed packages to determine actual success
 		postFormulae, _, postErr := GetInstalledPackages()
@@ -278,7 +277,7 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm
 				} else {
 					allFailed = append(allFailed, failedJob{
 						installJob: installJob{name: pkg, isCask: false},
-						errMsg:     "install failed",
+						errMsg:     parseBrewError(cmdOutputStr),
 					})
 				}
 			}
@@ -292,26 +291,25 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm
 				} else {
 					allFailed = append(allFailed, failedJob{
 						installJob: installJob{name: pkg, isCask: false},
-						errMsg:     "install failed",
+						errMsg:     extractPackageError(cmdOutputStr, pkg),
 					})
 				}
 			}
 		}
 	}
 
 	if len(newCask) > 0 {
-		ui.Info(fmt.Sprintf("Installing %d GUI apps...", len(newCask)))
+		progress.PrintLine("  Installing %d GUI apps via brew install --cask...", len(newCask))
 
 		args := append([]string{"install", "--cask"}, newCask...)
 		cmd := brewInstallCmd(args...)
-		cmd.Stdout = os.Stdout
-		cmd.Stderr = os.Stderr
 		// Open TTY for password prompts
 		tty, opened := system.OpenTTY()
 		if opened {
 			cmd.Stdin = tty
 		}
-		cmdErr := cmd.Run()
+		cmdOutput, cmdErr := cmd.CombinedOutput()
+		cmdOutputStr := string(cmdOutput)
 		if opened {
 			tty.Close()
 		}
@@ -327,7 +325,7 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm
 				} else {
 					allFailed = append(allFailed, failedJob{
 						installJob: installJob{name: pkg, isCask: true},
-						errMsg:     "install failed",
+						errMsg:     parseBrewError(cmdOutputStr),
 					})
 				}
 			}
@@ -341,7 +339,7 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedForm
 				} else {
 					allFailed = append(allFailed, failedJob{
 						installJob: installJob{name: pkg, isCask: true},
-						errMsg:     "install failed",
+						errMsg:     extractPackageError(cmdOutputStr, pkg),
 					})
 				}
 			}
@@ -418,36 +416,6 @@ type failedJob struct {
 	errMsg string
 }
 
-func installCaskWithProgress(pkg string, progress *ui.StickyProgress) string {
-	progress.PauseForInteractive()
-
-	cmd := brewInstallCmd("install", "--cask", pkg)
-	tty, opened := system.OpenTTY()
-	if opened {
-		defer tty.Close()
-	}
-	cmd.Stdin = tty
-	cmd.Stdout = os.Stdout
-	cmd.Stderr = os.Stderr
-	err := cmd.Run()
-
-	progress.ResumeAfterInteractive()
-
-	if err != nil {
-		return "install failed"
-	}
-	return ""
-}
-
-func printBrewOutput(output string, progress *ui.StickyProgress) {
-	for _, line := range strings.Split(strings.TrimSpace(output), "\n") {
-		line = strings.TrimSpace(line)
-		if line != "" {
-			progress.PrintLine("    %s", line)
-		}
-	}
-}
-
 func brewInstallCmd(args ...string) *exec.Cmd {
 	cmd := exec.Command("brew", args...)
 	cmd.Env = append(os.Environ(), "HOMEBREW_NO_AUTO_UPDATE=1")
@@ -585,6 +553,31 @@ func parseBrewError(output string) string {
 	}
 }
 
+// extractPackageError tries to find an error message specific to pkg in the
+// combined brew output. Falls back to parseBrewError on the full output.
+func extractPackageError(output, pkg string) string {
+	// Scan for lines mentioning the package name near an error indicator.
+	lowerPkg := strings.ToLower(pkg)
+	for _, line := range strings.Split(output, "\n") {
+		lower := strings.ToLower(line)
+		if strings.Contains(lower, lowerPkg) && strings.Contains(lower, "error") {
+			line = strings.TrimSpace(line)
+			if len(line) > 80 {
+				return line[:77] + "..."
+			}
+			return line
+		}
+	}
+
+	// No package-specific line found; fall back to the general parser but
+	// indicate the package was not installed after the batch attempt.
+	parsed := parseBrewError(output)
+	if parsed == "unknown error" {
+		return "not installed after batch attempt"
+	}
+	return parsed
+}
+
 func Uninstall(packages []string, dryRun bool) error {
 	if len(packages) == 0 {
 		return nil

internal/brew/brew_test.go

@@ -1,10 +1,12 @@
 package brew
 
 import (
+	"os"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestParseBrewError(t *testing.T) {
@@ -198,17 +200,71 @@ func TestHandleFailedJobs_WithFailures(t *testing.T) {
 // commands (brew install pkg1 pkg2...) instead of individual commands.
 // This leverages Homebrew's native parallel download capability.
 func TestInstallWithProgress_BatchMode(t *testing.T) {
-	// Dry-run should show batch commands
-	formulae, casks, err := InstallWithProgress(
+	// Capture stdout to verify batch command format
+	oldStdout := os.Stdout
+	r, w, err := os.Pipe()
+	require.NoError(t, err)
+	os.Stdout = w
+
+	formulae, casks, runErr := InstallWithProgress(
 		[]string{"git", "curl", "wget"},
 		[]string{"firefox", "chrome"},
 		true,
 	)
-	assert.NoError(t, err)
-	assert.Empty(t, formulae)
-	assert.Empty(t, casks)
-	// In dry-run mode, we can't easily verify the command format without capturing output,
-	// but the function signature and behavior tests ensure batch mode is used
+
+	w.Close()
+	os.Stdout = oldStdout
+
+	var buf strings.Builder
+	_, copyErr := buf.ReadFrom(r)
+	require.NoError(t, copyErr)
+	output := buf.String()
+
+	assert.NoError(t, runErr)
+	assert.Empty(t, formulae, "dry-run should not report installed formulae")
+	assert.Empty(t, casks, "dry-run should not report installed casks")
+
+	// Verify batch command format: all CLI packages in a single brew install
+	assert.Contains(t, output, "brew install git curl wget",
+		"dry-run should show a single batch brew install command for all CLI packages")
+	// Verify batch cask command format
+	assert.Contains(t, output, "brew install --cask firefox chrome",
+		"dry-run should show a single batch brew install --cask command for all cask packages")
+}
+
+func TestExtractPackageError(t *testing.T) {
+	tests := []struct {
+		name     string
+		output   string
+		pkg      string
+		expected string
+	}{
+		{
+			name:     "package-specific error line",
+			output:   "==> Installing foo\nError: foo: no bottle available!\n==> Installing bar",
+			pkg:      "foo",
+			expected: "Error: foo: no bottle available!",
+		},
+		{
+			name:     "no package-specific line falls back to parser",
+			output:   "Error: No internet connection available",
+			pkg:      "baz",
+			expected: "no internet connection",
+		},
+		{
+			name:     "no useful output gives batch message",
+			output:   "some random output\nnothing useful here",
+			pkg:      "qux",
+			expected: "not installed after batch attempt",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := extractPackageError(tt.output, tt.pkg)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
 }
 
 // TestResolveFormulaName tests that formula aliases are resolved correctly.