diff --git a/internal/archtest/baseline/no-direct-exec.txt b/internal/archtest/baseline/no-direct-exec.txt index d8965ae..eebede0 100644 --- a/internal/archtest/baseline/no-direct-exec.txt +++ b/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 diff --git a/internal/dotfiles/dotfiles.go b/internal/dotfiles/dotfiles.go index 6372ebf..76ae1d1 100644 --- a/internal/dotfiles/dotfiles.go +++ b/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) } diff --git a/internal/installer/installer.go b/internal/installer/installer.go index ab74765..7cd08ab 100644 --- a/internal/installer/installer.go +++ b/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 diff --git a/internal/installer/step_git.go b/internal/installer/step_git.go index fa3e091..639d572 100644 --- a/internal/installer/step_git.go +++ b/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 } diff --git a/internal/installer/step_packages.go b/internal/installer/step_packages.go index 48986b6..2a6d2bf 100644 --- a/internal/installer/step_packages.go +++ b/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() { diff --git a/internal/installer/step_shell.go b/internal/installer/step_shell.go index b08c09f..f4c20df 100644 --- a/internal/installer/step_shell.go +++ b/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)) } diff --git a/internal/installer/step_system.go b/internal/installer/step_system.go index dbef60e..b96667f 100644 --- a/internal/installer/step_system.go +++ b/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() diff --git a/internal/installer/steps_test.go b/internal/installer/steps_test.go new file mode 100644 index 0000000..96bd947 --- /dev/null +++ b/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"}, + })) +} diff --git a/internal/system/system.go b/internal/system/system.go index 902945d..e83e544 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -38,6 +38,17 @@ func RunCommandInDir(dir string, name string, args ...string) error { return cmd.Run() } +// RunCommandInDirSilent runs name with args in the given working directory and +// captures its combined output instead of forwarding it to the terminal, so a +// subprocess can't interleave its own chatter into a structured install log. +// Callers surface the output themselves when the command fails. +func RunCommandInDirSilent(dir string, name string, args ...string) (string, error) { + cmd := exec.CommandContext(context.Background(), name, args...) //nolint:gosec // intentional generic runner; callers are responsible for validating name and args + cmd.Dir = dir + output, err := cmd.CombinedOutput() + return strings.TrimSpace(string(output)), err +} + func RunCommandSilent(name string, args ...string) (string, error) { return RunCommandSilentContext(context.Background(), name, args...) } diff --git a/test/e2e/vm_interactive_test.go b/test/e2e/vm_interactive_test.go index fca652a..c57724c 100644 --- a/test/e2e/vm_interactive_test.go +++ b/test/e2e/vm_interactive_test.go @@ -13,7 +13,19 @@ import ( "github.com/openbootdotdev/openboot/testutil" ) -// TestVM_Interactive_InstallScript tests install.sh interactive prompts. +// TestVM_Interactive_InstallScript covers install.sh when openboot is already +// installed — the path every returning user takes. +// +// This file used to drive a "Reinstall? (y/N)" prompt with expect and assert +// that answering "n" kept the existing install. It passed, and it verified +// nothing: under `curl … | bash` the script's stdin is the pipe carrying the +// script, so `read` never saw expect's keystroke. It consumed the script's own +// bytes, failed to match ^[Yy]$, and took the No branch — precisely what the +// test asserted. Bug and assertion agreed, so the suite stayed green while +// every returning user was silently pinned to their first-installed version. +// +// The prompt is gone: running the installer means you want the current +// release. These tests pin that contract instead. func TestVM_Interactive_InstallScript(t *testing.T) { if testing.Short() { t.Skip("skipping VM interactive test in short mode") @@ -22,47 +34,51 @@ func TestVM_Interactive_InstallScript(t *testing.T) { vm := testutil.NewMacHost(t) vmInstallViaBrew(t, vm) // taps openbootdotdev/openboot and installs openboot - // expect is required for interactive tests. - if _, err := vm.Run(fmt.Sprintf("export PATH=%q && command -v expect", brewPath)); err != nil { - out, installErr := vm.Run(fmt.Sprintf("export PATH=%q && brew install expect", brewPath)) - t.Logf("install expect: %s", out) - require.NoError(t, installErr, "should install expect for interactive tests") - } + // Plain `curl … | bash` IS the hazard, so it needs no simulating: bash's + // stdin is the pipe carrying the script, which means there is no keyboard + // for a prompt to read from and no way to hand it one. + // + // Don't try to make that explicit by redirecting — `curl … | bash < /dev/null` + // under bash replaces the script itself, so nothing runs at all and every + // assertion sees empty output. (Under zsh the same line does run the script, + // which is a good way to convince yourself it works before CI proves it + // doesn't.) + // + // `-s -- --help` passes --help through to the `openboot install` the script + // exec's into at the end, so these assertions exercise the installer without + // kicking off a real install. + installOverExisting := fmt.Sprintf( + "export NONINTERACTIVE=1 PATH=%q && curl -fsSL https://openboot.dev/install.sh | bash -s -- --help", + brewPath, + ) - t.Run("reinstall_answer_no", func(t *testing.T) { - cmd := fmt.Sprintf( - "export NONINTERACTIVE=1 PATH=%q && curl -fsSL https://openboot.dev/install.sh | bash", - brewPath, - ) - output, err := vm.RunInteractive(cmd, []testutil.ExpectStep{ - {Expect: "Reinstall", Send: "n\r"}, - }, 30) - t.Logf("reinstall-no:\n%s", output) + t.Run("already_installed_updates_without_prompting", func(t *testing.T) { + output, err := vm.Run(installOverExisting) + t.Logf("install-over-existing:\n%s", output) if err != nil { - t.Logf("exited with: %v", err) + t.Logf("exited with: %v", err) // it exec's into `openboot install`, which wants a tty } - assert.True(t, - strings.Contains(output, "existing") || - strings.Contains(output, "Using"), - "should keep existing installation, got: %s", output) + + assert.NotContains(t, output, "Reinstall?", + "must not ask a question it cannot receive the answer to under curl|bash") + assert.NotContains(t, output, "Using existing installation", + "an existing install must be updated, not silently kept") + assert.Contains(t, output, "updating", + "the already-installed path reports that it is updating, got: %s", output) }) - t.Run("reinstall_answer_yes", func(t *testing.T) { - cmd := fmt.Sprintf( - "export NONINTERACTIVE=1 PATH=%q && curl -fsSL https://openboot.dev/install.sh | bash -s -- --help", - brewPath, - ) - output, err := vm.RunInteractive(cmd, []testutil.ExpectStep{ - {Expect: "Reinstall", Send: "y\r"}, - }, 120) - t.Logf("reinstall-yes:\n%s", output) - if err != nil { - t.Logf("exited with: %v", err) - } - assert.True(t, - strings.Contains(output, "reinstalled") || - strings.Contains(output, "Reinstalling") || - strings.Contains(output, "Usage:"), - "should reinstall, got: %s", output) + // The failure this path guards against is a successful-looking install that + // leaves yesterday's binary behind, so the version has to be stated where + // the user can see it. + t.Run("reports_the_version_it_installed", func(t *testing.T) { + output, _ := vm.Run(installOverExisting) + + version, err := vm.Run(fmt.Sprintf("export PATH=%q && openboot version", brewPath)) + require.NoError(t, err, "openboot must be runnable after the installer") + version = strings.TrimSpace(version) + require.NotEmpty(t, version) + + assert.Contains(t, output, version, + "installer must print the version it left behind (%q), got: %s", version, output) }) }