fix(installer): honest section numbering, and stop leaking subprocess output#152
Merged
Conversation
…bprocess output
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Makes the apply log readable: section numbers are derived from the plan instead of hardcoded, each failure is reported once, and subprocesses stop narrating themselves into the middle of a section.
Also fixes the v0.66.1 release, which failed its
vm-e2egate — see below.Why?
Reported from a real run. The log read:
The numbers lived in each step's own header string; whether a step ran was decided separately by its own guard. Two sets of logic, no way to agree — and every plan has conditional steps, so the numbers could only ever be wrong.
ApplyContextnow derives the section list from the plan and prints[i/n]. Steps keep their guards and lose their headers, so the numbering is dense by construction:--packages-onlyprints[1/1], a plan without dotfiles leaves no hole, and npm gets a real section instead of none.Same log, same run, also fixed:
applyPackages/applyNpmannounced their own failure and the caller announced it again.ApplyContextis now the single reporter.✓ ✓ Already configured—ui.Successalready prefixes the tick.HEAD is now at 5a1dc8e feat: add Makefile…,Receiving objects: 100% (40/40),stow -v --target=…. Captured now, and attached to the error, which reads better than the raw stream did:system.RunCommandInDirSilentis the captured counterpart ofRunCommandInDir, added because subprocesses go throughinternal/systemby convention.These warts predate the wizard. They were buried in an alt-screen log that dropped Info/Header lines; moving the apply back to the scrollback (#150) made this output the thing you actually read.
The v0.66.1 release failure
release.ymlfor v0.66.1 failed atTestVM_Interactive_InstallScript/reinstall_answer_no, so no binaries were published — the installer fix (#151) went live on openboot.dev but never reached the tap.The test is worth reading carefully. It drove the
Reinstall? (y/N)prompt with expect, sentn, and asserted "should keep existing installation". It passed, and it verified nothing. Undercurl … | bashthereadnever saw expect's keystroke — it consumed the script's own bytes, failed to match^[Yy]$, and took the No branch. Which is exactly what the test asserted. The bug and the assertion agreed, so the suite stayed green while every returning user was pinned to their first-installed version.#151 removed the prompt, so the test broke — after merge, not on the PR. That's the second thing worth noting: the test
curls the livehttps://openboot.dev/install.sh, so it tests production, not the PR. #151 was green because openboot.dev still served the old script at the time; the deploy on merge is what broke the release job.Rewritten to pin the new contract: an existing install is updated without prompting, and the resolved version is printed. It drives
bash < /dev/nullso the no-keyboard-on-stdin hazard is reproduced rather than papered over.Testing
go vet ./...passesinternal/installer/steps_test.gopins dense numbering, skip-what-won't-run, and order;vm_interactive_test.gorewritten for the new installer contractNotes for reviewer
Worth a follow-up decision, recorded in HARNESS.md rather than fixed here: the e2e installer test hitting the live URL means it can only fail after a merge has already shipped. Pointing it at the PR's own
scripts/install.shwould make it a real gate. Related:curl-bash-smokeis still gatedif: github.event_name != 'pull_request'and shows as skipping on every PR — including #151, which changed nothing but that script.