fix(installer): honest section numbering, and stop leaking subprocess output

internal/archtest/baseline/no-direct-exec.txt

@@ -6,12 +6,12 @@ internal/brew/brew_install.go:356
 internal/cli/snapshot.go:22
 internal/diff/compare.go:247
 internal/diff/compare.go:253
-internal/dotfiles/dotfiles.go:23
-internal/dotfiles/dotfiles.go:32
-internal/dotfiles/dotfiles.go:66
-internal/dotfiles/dotfiles.go:351
-internal/dotfiles/dotfiles.go:449
-internal/installer/step_system.go:132
+internal/dotfiles/dotfiles.go:27
+internal/dotfiles/dotfiles.go:41
+internal/dotfiles/dotfiles.go:79
+internal/dotfiles/dotfiles.go:376
+internal/dotfiles/dotfiles.go:474
+internal/installer/step_system.go:126
 internal/npm/npm.go:23
 internal/permissions/screen_recording_cgo.go:21
 internal/shell/shell.go:184

internal/dotfiles/dotfiles.go

@@ -17,13 +17,22 @@ var branchNameRe = regexp.MustCompile(`^[a-zA-Z0-9._/-]+$`)
 
 const defaultDotfilesDir = ".dotfiles"
 
-// gitExecFunc runs a git command with stdout/stderr forwarded to the terminal.
+// gitExecFunc runs a git command, capturing its output rather than forwarding
+// it to the terminal: these run inside a numbered section of the install log,
+// and git narrating itself there ("HEAD is now at 5a1dc8e feat: add Makefile…")
+// reads as noise the user can't act on. The output is attached to the error so
+// a failure is still diagnosable.
 // Replaced in tests to avoid forking real git processes.
 var gitExecFunc = func(args []string) error {
 	cmd := exec.Command("git", args...) //nolint:gosec // "git" is a hardcoded binary; args are validated by callers
-	cmd.Stdout = os.Stdout
-	cmd.Stderr = os.Stderr
-	return cmd.Run()
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		if trimmed := strings.TrimSpace(string(out)); trimmed != "" {
+			return fmt.Errorf("%w: %s", err, trimmed)
+		}
+		return err
+	}
+	return nil
 }
 
 // gitOutputFunc runs a git command and captures its stdout.
@@ -63,10 +72,19 @@ func Clone(repoURL string, dryRun bool) error {
 		return nil
 	}
 
+	// Captured like every other git call here: the clone's own progress
+	// ("Receiving objects: 100% (40/40)…") lands in the middle of a numbered
+	// install section and says nothing the surrounding lines don't. Its output
+	// is what diagnoses a failure, so it rides along with the error.
 	cmd := exec.Command("git", "clone", repoURL, dotfilesPath) //nolint:gosec // git binary is hardcoded; repoURL is validated by the caller
-	cmd.Stdout = os.Stdout
-	cmd.Stderr = os.Stderr
-	return cmd.Run()
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		if trimmed := strings.TrimSpace(string(out)); trimmed != "" {
+			return fmt.Errorf("%w: %s", err, trimmed)
+		}
+		return err
+	}
+	return nil
 }
 
 // handleExistingDotfiles manages the case where a dotfiles directory already
@@ -278,10 +296,17 @@ func linkWithMake(dotfilesPath string, dryRun bool) error {
 		}
 	}
 
-	if err := system.RunCommandInDir(dotfilesPath, "make", "install"); err != nil {
+	// Captured, not forwarded: this runs inside a numbered section of the
+	// install log, and the Makefile echoing its own recipe ("stow -v --target=…")
+	// there is noise. On failure the output is what diagnoses it, so it rides
+	// along with the error.
+	if out, err := system.RunCommandInDirSilent(dotfilesPath, "make", "install"); err != nil {
 		for _, pair := range allBacked {
 			restoreFile(pair[0], pair[1], false)
 		}
+		if out != "" {
+			return fmt.Errorf("make install: %w: %s", err, out)
+		}
 		return fmt.Errorf("make install: %w", err)
 	}
 

internal/installer/installer.go

@@ -114,50 +114,36 @@ func Apply(plan InstallPlan, r Reporter) error {
 }
 
 func ApplyContext(ctx context.Context, plan InstallPlan, r Reporter) error {
-	if !plan.PackagesOnly && !plan.SkipGit {
-		if err := applyGitConfig(plan, r); err != nil {
-			return fmt.Errorf("apply git config: %w", err)
-		}
-	}
-
+	steps := plannedSteps(plan)
 	var softErrs []error
 
-	if err := applyPackages(ctx, plan, r); err != nil {
-		softErrs = append(softErrs, fmt.Errorf("brew: %w", err))
-	}
-
-	// ctrl+c cancels ctx. brew/npm honour it (exec.CommandContext), but the
-	// config steps below take no ctx, so without these gates an aborted install
-	// would keep symlinking dotfiles and rewriting macOS defaults after the user
-	// asked to stop. Bail between phases so an abort skips not-yet-started work.
-	if err := ctx.Err(); err != nil {
-		return abortWith(softErrs, err)
-	}
+	for i, s := range steps {
+		// ctrl+c cancels ctx. brew/npm honour it (exec.CommandContext), but the
+		// config steps take no ctx, so without this gate an aborted install would
+		// keep symlinking dotfiles and rewriting macOS defaults after the user
+		// asked to stop. Bail between steps so an abort skips not-yet-started work.
+		if err := ctx.Err(); err != nil {
+			return abortWith(softErrs, err)
+		}
 
-	if err := applyNpm(ctx, plan, r); err != nil {
-		r.Error(fmt.Sprintf("npm package installation failed: %v", err))
-		softErrs = append(softErrs, fmt.Errorf("npm: %w", err))
-	}
+		r.Header(sectionTitle(i, len(steps), s.name))
 
-	if !plan.PackagesOnly {
-		configSteps := []struct {
-			name, failMsg string
-			apply         func(InstallPlan, Reporter) error
-		}{
-			{"shell", "Shell setup failed", applyShell},
-			{"dotfiles", "Dotfiles setup failed", applyDotfiles},
-			{"macos", "macOS configuration failed", applyMacOSPrefs},
-			{"post-install", "Post-install script failed", applyPostInstall},
+		err := s.run(ctx, plan, r)
+		if err == nil {
+			continue
 		}
-		for _, s := range configSteps {
-			if err := ctx.Err(); err != nil {
-				return abortWith(softErrs, err)
-			}
-			if err := s.apply(plan, r); err != nil {
-				r.Error(fmt.Sprintf("%s: %v", s.failMsg, err))
-				softErrs = append(softErrs, fmt.Errorf("%s: %w", s.name, err))
-			}
+		// Git is the one hard failure: everything after it authors commits or
+		// writes config on its behalf, so a broken identity is not something to
+		// carry on past. The rest are soft — a failed cask shouldn't cost you
+		// your dotfiles.
+		if s.name == "Git identity" {
+			return fmt.Errorf("apply git config: %w", err)
 		}
+		// Report here, once. Steps must not also announce their own failure:
+		// when both did, the same error printed twice in a row under its own
+		// section, which reads like two separate things went wrong.
+		r.Error(fmt.Sprintf("%s failed: %v", s.name, err))
+		softErrs = append(softErrs, fmt.Errorf("%s: %w", strings.ToLower(s.name), err))
 	}
 
 	showCompletionFromPlan(plan, r, len(softErrs))
@@ -170,6 +156,55 @@ func ApplyContext(ctx context.Context, plan InstallPlan, r Reporter) error {
 	return nil
 }
 
+// applyStep is one titled section of the apply.
+//
+// The section title used to be printed by the step function itself, with a
+// number written into the string: "Step 1: Git Configuration", "Step 4:
+// Installation", "Step 6: Dotfiles". Steps are conditional, so those numbers
+// could only ever be wrong — a real run printed 1, 4, 6, 7, with two
+// unnumbered sections wedged between. The numbering has to be derived from the
+// plan, because only the plan knows which steps exist.
+//
+// cond therefore mirrors each step function's own entry guard: a step that
+// won't run must not consume a number. The step functions keep their guards —
+// they're the ones that must not act — and this list keeps the titles honest.
+type applyStep struct {
+	name string
+	cond bool
+	run  func(context.Context, InstallPlan, Reporter) error
+}
+
+// noCtx adapts a step that takes no context to the uniform signature.
+func noCtx(f func(InstallPlan, Reporter) error) func(context.Context, InstallPlan, Reporter) error {
+	return func(_ context.Context, p InstallPlan, r Reporter) error { return f(p, r) }
+}
+
+// plannedSteps returns, in execution order, the sections this plan will run.
+func plannedSteps(plan InstallPlan) []applyStep {
+	sys := !plan.PackagesOnly
+	all := []applyStep{
+		{"Git identity", sys && !plan.SkipGit, noCtx(applyGitConfig)},
+		{"Packages", len(plan.Formulae)+len(plan.Casks)+len(plan.Taps) > 0, applyPackages},
+		{"npm globals", len(plan.Npm) > 0, applyNpm},
+		{"Shell", sys && plan.InstallOhMyZsh, noCtx(applyShell)},
+		{"Dotfiles", sys && plan.DotfilesURL != "", noCtx(applyDotfiles)},
+		{"macOS preferences", sys && (len(plan.MacOSPrefs) > 0 || plan.DockApps != nil || plan.LoginItems != nil), noCtx(applyMacOSPrefs)},
+		{"Post-install script", sys && len(plan.PostInstall) > 0, noCtx(applyPostInstall)},
+	}
+	out := make([]applyStep, 0, len(all))
+	for _, s := range all {
+		if s.cond {
+			out = append(out, s)
+		}
+	}
+	return out
+}
+
+// sectionTitle renders the header for step i of n.
+func sectionTitle(i, n int, name string) string {
+	return fmt.Sprintf("[%d/%d] %s", i+1, n, name)
+}
+
 // abortWith joins the soft errors accumulated so far with the context
 // cancellation cause, for when ApplyContext bails out early on a cancelled
 // context (ctrl+c). Returning here stops the remaining steps so an aborted

internal/installer/step_git.go

@@ -8,12 +8,9 @@ import (
 )
 
 func applyGitConfig(plan InstallPlan, r Reporter) error {
-	r.Header("Step 1: Git Configuration")
-	ui.Println()
-
 	existingName, existingEmail := system.GetExistingGitConfig()
 	if existingName != "" && existingEmail != "" {
-		r.Success(fmt.Sprintf("✓ Already configured: %s <%s>", existingName, existingEmail))
+		r.Success(fmt.Sprintf("Already configured: %s <%s>", existingName, existingEmail))
 		ui.Println()
 		return nil
 	}

internal/installer/step_packages.go

@@ -82,9 +82,6 @@ func categorizeSelectedPackages(opts *config.InstallOptions, st *config.InstallS
 }
 
 func applyPackages(ctx context.Context, plan InstallPlan, r Reporter) error { //nolint:gocyclo // orchestrates multiple package categories; splitting would obscure the install sequence
-	r.Header("Step 4: Installation")
-	ui.Println()
-
 	if len(plan.Taps) > 0 {
 		if err := brew.InstallTaps(plan.Taps, plan.DryRun); err != nil {
 			r.Warn(fmt.Sprintf("Some taps failed: %v", err))
@@ -156,10 +153,11 @@ func applyPackages(ctx context.Context, plan InstallPlan, r Reporter) error { //
 
 	brewCtx, cancel := packageInstallContext(ctx, len(cliPkgs)+len(caskPkgs))
 	defer cancel()
+	// brewErr is returned, not reported: ApplyContext announces a step's failure
+	// once, under its own section heading. The packages that did land are still
+	// recorded below, so a partial failure doesn't lose the state for the ones
+	// that worked.
 	installedCli, installedCask, brewErr := brew.InstallWithProgress(brewCtx, cliPkgs, caskPkgs, plan.DryRun)
-	if brewErr != nil {
-		r.Error(fmt.Sprintf("Some packages failed: %v", brewErr))
-	}
 
 	if !plan.DryRun {
 		for _, pkg := range installedCli {
@@ -236,9 +234,6 @@ func applyNpm(ctx context.Context, plan InstallPlan, r Reporter) error { //nolin
 		return nil
 	}
 
-	ui.Println()
-	r.Header("NPM Global Packages")
-	ui.Println()
 	r.Info(fmt.Sprintf("Installing %d npm packages...", len(npmPkgs)))
 	ui.Println()
 
@@ -252,7 +247,6 @@ func applyNpm(ctx context.Context, plan InstallPlan, r Reporter) error { //nolin
 			break
 		}
 		if attempt == maxAttempts {
-			r.Error(fmt.Sprintf("npm package installation failed after %d attempts: %v", maxAttempts, lastErr))
 			return fmt.Errorf("npm installation failed after %d attempts: %w", maxAttempts, lastErr)
 		}
 		if plan.Silent || !system.HasTTY() {

internal/installer/step_shell.go

@@ -14,9 +14,6 @@ var installOhMyZshFunc = shell.InstallOhMyZsh
 
 func applyShell(plan InstallPlan, r Reporter) error {
 	if plan.InstallOhMyZsh {
-		r.Header("Shell Configuration")
-		ui.Println()
-
 		if plan.ShellTheme != "" || len(plan.ShellPlugins) > 0 {
 			// Restore mode: install OMZ if missing, then write theme/plugins.
 			if err := shell.RestoreFromSnapshot(true, plan.ShellTheme, plan.ShellPlugins, plan.DryRun); err != nil {
@@ -55,9 +52,6 @@ func applyDotfiles(plan InstallPlan, r Reporter) error {
 		return nil // explicitly skipped via --dotfiles skip
 	}
 
-	r.Header("Step 6: Dotfiles")
-	ui.Println()
-
 	if plan.DotfilesURL == dotfiles.DefaultDotfilesURL {
 		r.Info(fmt.Sprintf("Using OpenBoot default dotfiles (%s)", plan.DotfilesURL))
 	}

internal/installer/step_system.go

@@ -20,9 +20,6 @@ func applyMacOSPrefs(plan InstallPlan, r Reporter) error {
 		return nil
 	}
 
-	r.Header("Step 7: macOS Preferences")
-	ui.Println()
-
 	var errs []error
 
 	if hasPrefs {
@@ -91,9 +88,6 @@ func applyPostInstall(plan InstallPlan, r Reporter) error {
 		return nil
 	}
 
-	r.Header("Step 8: Post-Install Script")
-	ui.Println()
-
 	if !plan.DryRun && (plan.Silent || !system.HasTTY()) && !plan.AllowPostInstall {
 		r.Warn("Skipping post-install script in silent mode (use --allow-post-install to enable)")
 		ui.Println()

internal/installer/steps_test.go

@@ -0,0 +1,83 @@
+package installer
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/openbootdotdev/openboot/internal/macos"
+)
+
+// Section numbers used to be written into each step's own header string —
+// "Step 1: Git Configuration", "Step 4: Installation", "Step 6: Dotfiles" —
+// while whether a step ran was decided separately, by its own guard. A real
+// run printed 1, 4, 6, 7 with two unnumbered sections wedged in. These tests
+// pin the invariant that makes that impossible: numbering is derived from the
+// plan, so it is always dense and always matches what actually runs.
+
+func stepNames(plan InstallPlan) []string {
+	var out []string
+	for _, s := range plannedSteps(plan) {
+		out = append(out, s.name)
+	}
+	return out
+}
+
+func TestPlannedStepsAreDenselyNumbered(t *testing.T) {
+	full := InstallPlan{
+		Formulae:       []string{"jq"},
+		Npm:            []string{"typescript"},
+		InstallOhMyZsh: true,
+		DotfilesURL:    "https://github.com/x/dotfiles",
+		MacOSPrefs:     make([]macos.Preference, 3),
+		PostInstall:    []string{"echo hi"},
+	}
+	steps := plannedSteps(full)
+	require.Len(t, steps, 7, "git, packages, npm, shell, dotfiles, macOS, post-install")
+
+	var titles []string
+	for i, s := range steps {
+		titles = append(titles, sectionTitle(i, len(steps), s.name))
+	}
+	assert.Equal(t, "[1/7] Git identity", titles[0])
+	assert.Equal(t, "[7/7] Post-install script", titles[6])
+	for i, title := range titles {
+		assert.Truef(t, strings.HasPrefix(title, sectionTitle(i, len(steps), "")[:4]),
+			"title %q must carry its own index — no gaps", title)
+	}
+}
+
+// A step that won't run must not consume a number: this is the exact failure
+// the old hardcoded strings had.
+func TestPlannedStepsSkipWhatWontRun(t *testing.T) {
+	packagesOnly := InstallPlan{
+		PackagesOnly:   true,
+		Formulae:       []string{"jq"},
+		InstallOhMyZsh: true,                            // ignored under PackagesOnly
+		DotfilesURL:    "https://github.com/x/dotfiles", // ignored
+		MacOSPrefs:     make([]macos.Preference, 3),     // ignored
+	}
+	assert.Equal(t, []string{"Packages"}, stepNames(packagesOnly),
+		"--packages-only runs exactly one section, numbered 1/1")
+
+	noGit := InstallPlan{SkipGit: true, Formulae: []string{"jq"}}
+	assert.Equal(t, []string{"Packages"}, stepNames(noGit), "a skipped git step takes no number")
+
+	npmOnly := InstallPlan{SkipGit: true, Npm: []string{"typescript"}}
+	assert.Equal(t, []string{"npm globals"}, stepNames(npmOnly),
+		"npm gets a real numbered section — it had no number at all before")
+
+	assert.Empty(t, stepNames(InstallPlan{SkipGit: true}), "an empty plan runs nothing")
+}
+
+// The step list is the execution order the user reads; lock it.
+func TestPlannedStepsOrder(t *testing.T) {
+	assert.Equal(t,
+		[]string{"Git identity", "Packages", "npm globals", "Shell", "Dotfiles", "macOS preferences", "Post-install script"},
+		stepNames(InstallPlan{
+			Formulae: []string{"jq"}, Npm: []string{"ts"}, InstallOhMyZsh: true,
+			DotfilesURL: "u", MacOSPrefs: make([]macos.Preference, 1), PostInstall: []string{"x"},
+		}))
+}