feat(installer): brand the curl|sh run with a mark, table, and card - #1122
Merged
norrietaylor merged 3 commits intoJul 31, 2026
Merged
Conversation
A successful install ended on whichever bookkeeping message fired last — the install-record summary, the shell-init note, the PATH advisory, or the AppArmor advisory — none of which tells a new user what to do next. Emit a getting-started block as the final stderr output of every successful run, after all existing notes, naming the first command to run. Failure and uninstall/dry-run paths are unchanged. Add harness assertions that the hint is the last line on a fresh install and on an up-to-date rerun, that it follows the AppArmor advisory, and that it is absent when an install fails.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe installer now uses terminal-aware progress presentation, structured install and uninstall output, first-install branding, receipts, and a successful-install closing card. Specifications and tests cover redirected output, card placement, failed installs, and uninstall recovery. ChangesInstaller presentation and lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant Terminal
participant InstallRecord
participant ShellIntegration
Installer->>Terminal: render component progress rows
Installer->>InstallRecord: persist install records
Installer->>ShellIntegration: generate completions and update shell hooks
Installer->>Terminal: print receipt and closing card
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
norrietaylor
approved these changes
Jul 31, 2026
norrietaylor
marked this pull request as ready for review
July 31, 2026 05:38
The installer is the product's first impression and read like a bookkeeping log: ragged `component: verb` lines, a successful run ending on the path of its own install record, and nothing anywhere that looked like Minimal. Add a presentation layer to install.sh: - Degradation first. Bold/dim and the in-place row rewrite need stderr to be a terminal, NO_COLOR unset, and TERM != dumb; UTF-8 glyphs need a UTF-8 locale, independently of that. MINIMAL_INSTALL_PLAIN=1 forces both off. A redirected run therefore emits no escape byte at all, which is what keeps CI logs and any future output parsing clean. - One aligned row per component (name, verb, size or path), rewritten in place on a terminal so a slow download narrates itself and still leaves exactly one line behind. Anything printing mid-row closes it first, so the active-sessions prompt and fatal errors never land mid-line. Uninstall prints the same table. - The mark, on a first install only: character-for-character the one in the README's session demo (docs/public/loadout-demo.cast). - A closing card as the last output of every successful install, superseding this branch's earlier getting-started hint. It names the first two commands and carries the PATH advisory inside it when the bin dir is not yet on PATH, since those commands would not resolve in this shell otherwise. Monochrome throughout — attributes, never color — like the CLI's prompt theme and the website. Record the layer as Unit 10 of 07-spec-installer. Its one contract is the degradation rule (R10.1); the terminal-only behaviors are marked not-automated, because the harness always redirects and cannot reach them. Drop the R-number citations from install.sh in the same pass: the spec is where those numbers belong, and in code they rot silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
norrietaylor
enabled auto-merge (squash)
July 31, 2026 07:09
norrietaylor
disabled auto-merge
July 31, 2026 07:09
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.sh (1)
527-547: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRetain the install record until shell teardown completes.
Line 536 removes
$recordbeforestrip_rc_blockruns. If a later rc-file rewrite fails or the process stops, the marker block remains. A later--uninstallexits at lines 441-444 because the record is gone, so it cannot retry cleanup.Move record deletion after shell teardown. Retain the record when any shell block remains unremoved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install.sh` around lines 527 - 547, Move the non-dry-run rm -f "$record" operation from the pre-teardown conditional to after the strip_rc_block loop. Track whether any rc-file marker block remains unremoved, and retain the install record with an appropriate message when teardown fails; only delete it after all shell cleanup succeeds, while preserving kept-entry and dry-run behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install_test.sh`:
- Around line 342-345: Update the redirected-output assertion around want_err to
invoke run with TERM=xterm-256color set, while keeping stderr redirected to OUT
and preserving the requirement that it contains no terminal escape bytes.
In `@scripts/install.sh`:
- Around line 84-90: Update the attribute-enabling condition in the
MINIMAL_INSTALL_PLAIN setup so NO_COLOR must be unset, not merely an empty
string; replace the current -z check with the appropriate variable-presence
check while preserving the existing terminal and TERM guards.
- Around line 165-175: Update wordmark so it returns immediately when glyph is
0, preventing the minimal branding line from rendering without UTF-8 glyph
support; retain the existing attr guard and full branding output when glyph is
enabled.
---
Outside diff comments:
In `@scripts/install.sh`:
- Around line 527-547: Move the non-dry-run rm -f "$record" operation from the
pre-teardown conditional to after the strip_rc_block loop. Track whether any
rc-file marker block remains unremoved, and retain the install record with an
appropriate message when teardown fails; only delete it after all shell cleanup
succeeds, while preserving kept-entry and dry-run behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24bfc443-49c6-4730-bbb7-d4aad637c635
📒 Files selected for processing (3)
docs/specs/07-spec-installer/07-spec-installer.mdscripts/install.shscripts/install_test.sh
Comment on lines
+84
to
+90
| if [ -z "${MINIMAL_INSTALL_PLAIN:-}" ]; then | ||
| if [ -t 2 ] && [ -z "${NO_COLOR:-}" ] && [ "${TERM:-dumb}" != dumb ]; then | ||
| attr=1 | ||
| fi | ||
| case "${LC_ALL:-${LC_CTYPE:-${LANG:-}}}" in | ||
| *UTF-8*|*utf-8*|*UTF8*|*utf8*) glyph=1 ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable attributes when NO_COLOR is set.
R10.1 requires NO_COLOR to be unset. The current -z check enables SGR attributes when the caller sets NO_COLOR=''.
Proposed fix
-if [ -z "${MINIMAL_INSTALL_PLAIN:-}" ]; then
- if [ -t 2 ] && [ -z "${NO_COLOR:-}" ] && [ "${TERM:-dumb}" != dumb ]; then
+if [ -z "${MINIMAL_INSTALL_PLAIN:-}" ]; then
+ if [ -t 2 ] && [ "${NO_COLOR+x}" != x ] && [ "${TERM:-dumb}" != dumb ]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -z "${MINIMAL_INSTALL_PLAIN:-}" ]; then | |
| if [ -t 2 ] && [ -z "${NO_COLOR:-}" ] && [ "${TERM:-dumb}" != dumb ]; then | |
| attr=1 | |
| fi | |
| case "${LC_ALL:-${LC_CTYPE:-${LANG:-}}}" in | |
| *UTF-8*|*utf-8*|*UTF8*|*utf8*) glyph=1 ;; | |
| esac | |
| if [ -z "${MINIMAL_INSTALL_PLAIN:-}" ]; then | |
| if [ -t 2 ] && [ "${NO_COLOR+x}" != x ] && [ "${TERM:-dumb}" != dumb ]; then | |
| attr=1 | |
| fi | |
| case "${LC_ALL:-${LC_CTYPE:-${LANG:-}}}" in | |
| *UTF-8*|*utf-8*|*UTF8*|*utf8*) glyph=1 ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 84 - 90, Update the attribute-enabling
condition in the MINIMAL_INSTALL_PLAIN setup so NO_COLOR must be unset, not
merely an empty string; replace the current -z check with the appropriate
variable-presence check while preserving the existing terminal and TERM guards.
Comment on lines
+165
to
+175
| wordmark() { | ||
| [ "$attr" -eq 1 ] || return 0 | ||
| printf '\n' >&2 | ||
| if [ "$glyph" -eq 1 ]; then | ||
| printf '%s%s\n%s\n%s%s\n\n' "$b" \ | ||
| ' ████ ████▄' \ | ||
| ' ▄▄▄ ▀███▄ ▀███▄' \ | ||
| ' ▀███ ▀███ ▀███' "$rst" >&2 | ||
| fi | ||
| printf ' %sminimal%s %s%s Build Software You Can Trust%s\n' \ | ||
| "$b" "$rst" "$dim" "$sep" "$rst" >&2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not render first-install branding without UTF-8 glyph support.
R10.3 requires the Minimal mark to be terminal-only and UTF-8-only. In an ASCII locale on a terminal, wordmark skips the block mark but still prints the minimal branding line.
Either return when glyph=0, or change R10.3 to specify this fallback.
Proposed fix
wordmark() {
- [ "$attr" -eq 1 ] || return 0
+ [ "$attr" -eq 1 ] && [ "$glyph" -eq 1 ] || return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wordmark() { | |
| [ "$attr" -eq 1 ] || return 0 | |
| printf '\n' >&2 | |
| if [ "$glyph" -eq 1 ]; then | |
| printf '%s%s\n%s\n%s%s\n\n' "$b" \ | |
| ' ████ ████▄' \ | |
| ' ▄▄▄ ▀███▄ ▀███▄' \ | |
| ' ▀███ ▀███ ▀███' "$rst" >&2 | |
| fi | |
| printf ' %sminimal%s %s%s Build Software You Can Trust%s\n' \ | |
| "$b" "$rst" "$dim" "$sep" "$rst" >&2 | |
| wordmark() { | |
| [ "$attr" -eq 1 ] && [ "$glyph" -eq 1 ] || return 0 | |
| printf '\n' >&2 | |
| if [ "$glyph" -eq 1 ]; then | |
| printf '%s%s\n%s\n%s%s\n\n' "$b" \ | |
| ' ████ ████▄' \ | |
| ' ▄▄▄ ▀███▄ ▀███▄' \ | |
| ' ▀███ ▀███ ▀███' "$rst" >&2 | |
| fi | |
| printf ' %sminimal%s %s%s Build Software You Can Trust%s\n' \ | |
| "$b" "$rst" "$dim" "$sep" "$rst" >&2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 165 - 175, Update wordmark so it returns
immediately when glyph is 0, preventing the minimal branding line from rendering
without UTF-8 glyph support; retain the existing attr guard and full branding
output when glyph is enabled.
Uninstall deleted the install record before stripping the marker-fenced shell-init block from the rc files. `strip_rc_block` returns non-zero on an unterminated block (the file is kept, block and all), and `do_uninstall` treats a missing record as "nothing to undo" and returns 0 — so a run that failed to strip the block left it uninstallable by any later run, with the inventory needed to retry already gone. Move the record teardown after the rc walk and the compinit-dump drop, and track a surviving block in `kept_rc` so retention is announced the same way a kept component already is. Also from review: - Set a terminal-like TERM in the test harness's `env -i`. The R10.1 redirected-output assertion ran with TERM unset, so it passed on the TERM guard alone and would not have caught a regression that dropped the `[ -t 2 ]` check. - Record the two presentation behaviours the spec stated more strictly than the implementation, both deliberate: NO_COLOR suppresses only when non-empty (no-color.org), and the mark's glyph block and its wordmark line degrade separately, so an ASCII-locale terminal opens on the wordmark alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
norrietaylor
enabled auto-merge (squash)
July 31, 2026 07:34
norrietaylor
deleted the
inbox-patch/installer-getting-started-hint-b25f03a06e45dc06
branch
July 31, 2026 07:48
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.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABK5q94Q
curl … | shis the product's first impression, and it read like a bookkeeping log: raggedcomponent: verblines, a successful run ending on the path of its own install record, and nothing anywhere that looked like Minimal.This started as a getting-started hint on the last line (the original commit on this branch) and grew into the presentation layer that hint belonged to.
What the run looks like now
A first install opens with the mark — character-for-character the one in the README's session demo (
docs/public/loadout-demo.cast) — then an aligned component table, then a closing card:NO_COLORunset, andTERM != dumb; UTF-8 glyphs require a UTF-8 locale, independently of that.MINIMAL_INSTALL_PLAIN=1forces both off. A redirected run emits no escape byte at all, which is what keeps CI logs and any future output parsing clean — and the one property here worth a regression test.PATHit carries that advisory inside the card, above the commands, because those commands would not resolve in this shell otherwise. No card on failure, none in uninstall mode.Monochrome throughout — attributes, never color — matching the CLI's prompt theme (
crates/minimal/src/theme.rs) and the website.Spec
The layer is recorded as Unit 10 of
07-spec-installer. Its one contract is the degradation rule (R10.1); R10.2–R10.4 record the shape the implementation takes so a rewrite does not have to rediscover it. The terminal-only behaviors are marked not automated — the harness always redirects and cannot reach them, so they are verified by eye against a mock bucket, not by CI.The same pass drops the R-number citations from
install.sh. The spec is where those numbers belong; in code they rot silently, and this PR's own diff is the proof (the oldUnit N:section headers no longer matched the units they named).Verification
install_test.sh— 270 passed / 0 failed, under bothshanddash. Gains: the card closes a fresh install, an up-to-date rerun, and a run whose last note is the AppArmor advisory; no card on a failed install; no ESC byte in redirected output. Four existing assertions were retargeted from the oldcomponent: verbwording to the table layout.shellcheck --shell=shclean oninstall.shandinstall_test.sh;just lint-shell— 26/26.cargo test -p common --locked— 46 passed / 0 failed, includingshell_lint::all_scripts_pass_shellcheckandinstaller_stop_signal.MINIMAL_INSTALL_PLAIN=1, piped-to-cat, an upgrade interrupted by the live-sessions prompt (the mid-row case), and uninstall.Note
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABK5q94Q
curl … | shis the product's first impression, and it read like a bookkeeping log: raggedcomponent: verblines, a successful run ending on the path of its own install record, and nothing anywhere that looked like Minimal.This started as a getting-started hint on the last line (the original commit on this branch) and grew into the presentation layer that hint belonged to.
What the run looks like now
A first install opens with the mark — character-for-character the one in the README's session demo (
docs/public/loadout-demo.cast) — then an aligned component table, then a closing card:NO_COLORunset, andTERM != dumb; UTF-8 glyphs require a UTF-8 locale, independently of that.MINIMAL_INSTALL_PLAIN=1forces both off. A redirected run emits no escape byte at all, which is what keeps CI logs and any future output parsing clean — and the one property here worth a regression test.PATHit carries that advisory inside the card, above the commands, because those commands would not resolve in this shell otherwise. No card on failure, none in uninstall mode.Monochrome throughout — attributes, never color — matching the CLI's prompt theme (
crates/minimal/src/theme.rs) and the website.Spec
The layer is recorded as Unit 10 of
07-spec-installer. Its one contract is the degradation rule (R10.1); R10.2–R10.4 record the shape the implementation takes so a rewrite does not have to rediscover it. The terminal-only behaviors are marked not automated — the harness always redirects and cannot reach them, so they are verified by eye against a mock bucket, not by CI.The same pass drops the R-number citations from
install.sh. The spec is where those numbers belong; in code they rot silently, and this PR's own diff is the proof (the oldUnit N:section headers no longer matched the units they named).Verification
install_test.sh— 270 passed / 0 failed, under bothshanddash. Gains: the card closes a fresh install, an up-to-date rerun, and a run whose last note is the AppArmor advisory; no card on a failed install; no ESC byte in redirected output. Four existing assertions were retargeted from the oldcomponent: verbwording to the table layout.shellcheck --shell=shclean oninstall.shandinstall_test.sh;just lint-shell— 26/26.cargo test -p common --locked— 46 passed / 0 failed, includingshell_lint::all_scripts_pass_shellcheckandinstaller_stop_signal.MINIMAL_INSTALL_PLAIN=1, piped-to-cat, an upgrade interrupted by the live-sessions prompt (the mid-row case), and uninstall.Changes since #1122 opened
human_sizeutility function to format byte counts andtildeutility to display HOME-relative paths [6a38bf7]do_uninstallshell function to retain the install record when shell-init blocks cannot be stripped or files remain [bebbfb5]TERM=xterm-256colorto test environment inrunhelper function [bebbfb5]NO_COLORenvironment variable handling to align with no-color.org convention [bebbfb5]Summary by CodeRabbit
New Features
mincommand after successful installation.Bug Fixes