From 72a415b7eed05f2cf56ba6c475fbc849b16b8b61 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Fri, 17 Jul 2026 21:17:57 +0800 Subject: [PATCH 1/2] fix(installer): derive section numbers from the plan; stop leaking subprocess output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The apply log read as: === Step 1: Git Configuration === === Step 4: Installation === === Shell Configuration === === Step 6: Dotfiles === === Step 7: macOS Preferences === 1, 4, 6, 7, with two unnumbered sections wedged in. The numbers were written into each step's own header string, while whether a step ran was decided separately by its own guard — two sets of logic with no way to agree. Any conditional step made the numbering wrong, and every plan has conditional steps. ApplyContext now derives the section list from the plan and prints [i/n] itself; the steps keep their guards and lose their headers. The numbering is dense by construction and matches what actually runs: --packages-only prints [1/1], a plan with no dotfiles doesn't leave a hole where dotfiles would have been, and npm — which never had a number at all — gets a real section. Also visible in the same log, now fixed: - errors printed twice. applyPackages and applyNpm announced their own failure and the caller announced it again, so one failure read as two. ApplyContext is the single reporter; steps return. - "✓ ✓ Already configured" — ui.Success already prefixes the tick. - git narrating itself mid-section ("HEAD is now at 5a1dc8e feat: add Makefile…", "Receiving objects: 100% (40/40)"), and the dotfiles Makefile echoing its own recipe ("stow -v --target=…"). Both are captured now and attached to the error instead, so a failure is still diagnosable — the message it produces is strictly better than the raw stream was: ✗ Dotfiles failed: link dotfiles: make install: exit status 2: /bin/sh: stow: command not found system.RunCommandInDirSilent is the captured counterpart of RunCommandInDir, added because the convention is that subprocesses go through internal/system. These warts predate the wizard, but they were buried in an alt-screen log that dropped Info/Header lines. Moving the apply back to the scrollback made this output the thing you actually read, which is what surfaced them. --- internal/archtest/baseline/no-direct-exec.txt | 12 +- internal/dotfiles/dotfiles.go | 41 +++++-- internal/installer/installer.go | 111 ++++++++++++------ internal/installer/step_git.go | 5 +- internal/installer/step_packages.go | 14 +-- internal/installer/step_shell.go | 6 - internal/installer/step_system.go | 6 - internal/installer/steps_test.go | 83 +++++++++++++ internal/system/system.go | 11 ++ test/e2e/vm_interactive_test.go | 82 +++++++------ 10 files changed, 255 insertions(+), 116 deletions(-) create mode 100644 internal/installer/steps_test.go 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..bee1a25 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,41 @@ 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") - } + // `< /dev/null` reproduces the real hazard: under curl|bash there is no + // keyboard on stdin. A prompt here must not block, and must not silently + // answer itself from the script's own bytes. + installOverExisting := fmt.Sprintf( + "export NONINTERACTIVE=1 PATH=%q && curl -fsSL https://openboot.dev/install.sh | bash < /dev/null", + 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) }) } From b45590a68fbb931d7ae772cdd06c62ceb55840d8 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Fri, 17 Jul 2026 21:24:44 +0800 Subject: [PATCH 2/2] test(e2e): drive the installer the way users do, not through a redirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rewritten test used `curl … | bash < /dev/null` to make "there is no keyboard on stdin" explicit. Under bash that redirect replaces the script itself — bash reads its commands from stdin, so it got /dev/null, ran nothing, and every assertion saw empty output. (Under zsh the same line runs the script, which is how it looked fine locally.) Plain `curl … | bash` already is the no-keyboard case; that's the whole point of the bug being fixed. Use it, with -s -- --help so the exec'd `openboot install` prints usage instead of installing for real. --- test/e2e/vm_interactive_test.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/test/e2e/vm_interactive_test.go b/test/e2e/vm_interactive_test.go index bee1a25..c57724c 100644 --- a/test/e2e/vm_interactive_test.go +++ b/test/e2e/vm_interactive_test.go @@ -34,11 +34,21 @@ func TestVM_Interactive_InstallScript(t *testing.T) { vm := testutil.NewMacHost(t) vmInstallViaBrew(t, vm) // taps openbootdotdev/openboot and installs openboot - // `< /dev/null` reproduces the real hazard: under curl|bash there is no - // keyboard on stdin. A prompt here must not block, and must not silently - // answer itself from the script's own bytes. + // 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 < /dev/null", + "export NONINTERACTIVE=1 PATH=%q && curl -fsSL https://openboot.dev/install.sh | bash -s -- --help", brewPath, )