install.sh: prompt to re-run setup when AIMX is already installed + capitalize brand prose#204
Conversation
…pitalize brand prose
Re-running `curl ... | install.sh` on a host whose binary already matches
the latest tag used to hard-exit with `aimx 0.0.7 is already installed`,
stranding operators who had aborted partway through `aimx setup` (e.g. to
grab a different domain) and now wanted to redo configuration. They had
to know about `--force`, which implies a binary swap they don't need and
isn't discoverable from the failure message.
Now: when the equal-version branch fires and `--force` was not passed,
ask "AIMX is already installed. Re-run setup to (re)configure it? [y/N]".
On yes, skip the download/extract/install (binary is already correct)
and fall through to backup_existing_config + `exec aimx setup`, exactly
the same handoff fresh installs use. On no (or no usable TTY) keep the
old exit-0 behavior so CI / fully-scripted callers don't break.
While here, capitalize the brand name "AIMX" in operator-facing prose.
Lowercase `aimx` stays for command literals (`aimx setup`, `aimx doctor`),
service unit references (`aimx.service`), paths (`/etc/aimx/...`), and
the GitHub repo identifier (`uzyn/aimx`). Lines touched: install banner,
port-check banner, help heading, "is Linux-only" error, port-25 occupancy
warn, the "already installed" message, the post-upgrade success line, the
post-install success line, and the "upgrade complete" sentence.
Tests: 6 new prompt cases (y/Yes/n/empty/no-TTY) via an overridable
_prompt_read stub, plus 3 regex regressions catching the canonical brand
violations ("installing/installed/upgraded aimx", "aimx is", and
"aimx <semver>"). Updated 3 existing assertions to match the new capital
strings.
The two `shellcheck install.sh` / `shellcheck tests/install_sh.sh` failures
in `sh tests/install_sh.sh` are pre-existing (SC2024 on `(sudo -v </dev/tty)`
and SC2010/SC2016 in the test harness); unrelated to this change and not
gated by CI.
uzyn
left a comment
There was a problem hiding this comment.
Changes Overview
This PR makes two related changes to install.sh:
- When the binary already matches the target tag and
--forcewas not passed, promptAIMX is already installed. Re-run setup to (re)configure it? [y/N]instead of hard-exiting. Ony, skip the download/extract/install step entirely and fall through tobackup_existing_config+exec aimx setup. Onn/ Enter / no usable TTY, retain the existing exit-0 behavior. - Capitalize the brand name "AIMX" in operator-facing prose strings (install banner, port-check banner, help heading, "Linux-only" error, port-25 occupancy warn, "already installed" message, post-install/upgrade success lines, "Upgrade complete" sentence). Lowercase
aimxis preserved for command literals, paths, service unit names, and the GitHub repo identifier.
Scope Alignment
Implementation matches the PR description exactly. The new prompt_reinstall and _prompt_read helpers are appropriately scoped, the _skip_swap=1 plumbing through the download/extract/install gates is consistent, and the equal-version yes-branch falls through to the same backup_existing_config + exec aimx setup handoff that fresh installs use. The brand sweep is bounded to user-facing prose strings — none of the touched lines contain command literals, paths, or repo identifiers.
Potential Bugs
No correctness issues. I traced the equal-version flow end-to-end:
prompt_reinstallreturns 0 →_skip_swap=1,_is_upgrade=0→ download/extract gate at install.sh:1306 skipped → upgrade gate at install.sh:1318 skipped → fresh-install gate at install.sh:1377 skipped (withRe-running AIMX setupinfo line) →backup_existing_config→exec aimx setup. Correct.prompt_reinstallreturns 1 →say "AIMX ... is already installed"→exit 0. Correct.--forceshort-circuits to_is_upgrade=1beforeprompt_reinstallis called, so the prompt never fires for--force. Correct.
One minor cosmetic discrepancy worth flagging:
Non-blocker — PR description claim about non-TTY is slightly incorrect. The PR description's test plan says curl ... | sh </dev/null (no TTY) "exits 0 with the 'already installed' line, no prompt printed." But prompt_reinstall prints the prompt question to stderr (install.sh:270) before calling _prompt_read, which is what does the TTY check. So in the no-TTY case, stderr will show:
AIMX is already installed. Re-run setup to (re)configure it? [y/N] install: AIMX 0.0.7 is already installed (pass --force to re-install)
Functionally fine (still exits 0, no install), but cosmetically the prompt question appears in CI logs even though no answer is read. Either move the printf after the TTY-availability check inside _prompt_read, or update the PR description / commit message to remove the "no prompt printed" claim.
Non-blocker — Dry-run now prompts interactively. Pre-PR, the equal-version + no-force branch in dry-run mode exit 0'd immediately (at the old say "aimx ... is already installed" line). Post-PR, it calls prompt_reinstall first, which means AIMX_DRY_RUN=1 sh install.sh on a host where the binary is already at the target tag will now prompt the operator. Dry-run is conventionally non-interactive ("audit before running for real"). Consider gating the prompt on [ "${DRY_RUN}" != "1" ], or accept the new behavior and document it. Not blocking — the dry-run still exits without touching the filesystem either way.
Security Issues
None. _ans is constrained to literal case-matching (y | Y | yes | YES | Yes); no command interpolation, no path use, no shell expansion of the input. read -r correctly disables backslash interpretation. Prompt content is hardcoded.
Test Coverage
The five new prompt assertions (y, Yes, n, empty, no-TTY) cover the core decision points. The overridable-stub design (_prompt_read() { _ans="y"; }) is a clean test seam — no /dev/tty mocking required. The three branding regressions are correctly anchored regexes that allow legitimate uses (paths like aimx-0.0.7-..., redirects like aimx 2>/dev/null).
Non-blocker — three case-statement branches are untested. The accept-list is y | Y | yes | YES | Yes. Tests cover y and Yes. The other three (Y, yes, YES) are matched by the same case arm so the risk of regression is low, but a single test asserting all five accept-list values would prevent a future contributor from quietly narrowing the match.
Non-blocker — no integration test for the equal-version + yes path through main(). All five new tests probe prompt_reinstall in isolation. The plumbing (_skip_swap=1 skipping the download, skipping the install, falling through to backup + handoff) is verified by code inspection only. A INSTALL_SH_TEST=1 test that stubs _prompt_read, compare_tags, parse_installed_tag, and download to drive the end-to-end equal-version yes branch and assert that no download call happened would close this. Not required given the code is straightforward, but would make the PR's intent regression-proof.
Code Quality
Implementation is clean. Good separation: _prompt_read owns the TTY/stdin selection and is stubbed in tests; prompt_reinstall owns the prompt text and case match. POSIX-compliant throughout. Function comments explain the "why" (TTY logic, default-no rationale, CI compatibility). The ${_skip_swap:-0} parameter expansion handles the unset case correctly.
The _is_upgrade=0 line at install.sh:1264 is technically redundant (it's already initialized at install.sh:1249), but keeping it makes the branch's intent self-documenting. Fine either way.
Documentation Gaps
Non-blocker — book/installation.md:38 documents the old behavior. The line currently reads:
"If an older
aimxis already installed, the upgrade path runs instead: stop the service, swap the binary atomically, restart. No wizard re-run. If the running version matches the target, it exits without touching anything (pass--forceto reinstall)."
This is now incorrect. The new behavior is: prompt to re-run setup (default no), with --force and non-TTY callers preserving the old exit-0 path. Operators reading the install guide will be misled. Suggested rewrite of the bolded sentence:
"If the running version matches the target, the script asks whether to re-run
aimx setupto (re)configure (default no, so CI / scripted callers keep exit-0 semantics). Pass--forceto skip the prompt and reinstall the binary."
Non-blocker — no release-notes entry. book/release-notes.md has an active "Unreleased" section. This PR is an operator-visible behavior change (curl | sh re-runs now offer a recovery handle) and warrants an entry there, ideally as a sub-section of the existing "install.sh upgrade path is louder and self-healing" block at line 87 or as its own short block.
Summary and Recommended Actions
-
Overall verdict: Needs minor fixes — implementation is correct and well-tested, but the install-flow documentation in
book/installation.mdnow diverges from the script's behavior, and the release notes don't reflect the operator-visible change. -
Blockers: none.
-
Non-blockers (should fix before merge):
- Update
book/installation.md:38to describe the new prompt-on-equal-version behavior. - Add a release-notes entry for the operator-visible behavior change.
- Update
-
Nice-to-haves (low priority):
3. Move the promptprintfinside_prompt_readafter the TTY check (or update the PR description to drop the "no prompt printed" claim).
4. Decide whether dry-run should be non-interactive on the equal-version branch and either gateprompt_reinstallonDRY_RUN != 1or accept the new behavior explicitly.
5. Add explicit test cases for theY,yes,YESaccept-list values.
6. Add an integration-style test driving the equal-version + yes path throughmain()to assert nodownloadcall happens.
Non-blocker fixes: - book/installation.md: rewrite the equal-version step in the install-flow numbered list so the docs match the new prompt-on-equal-version behavior (default no, --force bypass, non-TTY exits 0). - book/release-notes.md: add an Unreleased entry under "install.sh" for the recovery prompt + brand-prose capitalization, so operators reading the changelog see the behavior change. Nice-to-haves: - Move the prompt printf inside _prompt_read after the TTY-availability check so `curl | sh </dev/null` (no usable TTY) stays quiet — no stray "Re-run setup to (re)configure" line in CI logs. Aligns with the PR description's "no prompt printed" claim. - Gate prompt_reinstall on DRY_RUN != 1 so AIMX_DRY_RUN=1 + equal-version + no-force preserves pre-PR exit-0 behavior. Auditing the script must stay non-interactive. - Tests now cover all five accept-list values (y/Y/yes/YES/Yes) via a single loop, plus a regression assertion that the prompt question does not print when _prompt_read signals no-TTY, plus a dry-run+equal-version test that confirms prompt_reinstall is never called in dry-run mode. Test counts: 107/109 in `sh tests/install_sh.sh` (+5 vs pre-fix). The two failures are the same pre-existing shellcheck issues already disclosed on the original PR description and not gated by CI.
Re-review responses (commit 33336bd)Pushed a follow-up commit addressing the review feedback. Summary of what changed and what was deliberately skipped. Non-blockers — both fixed
Nice-to-haves — addressed (3, 4, 5, 6)
Also added
Implementation note on prompt placementThe cleanest fix was moving the Test results
Ready for re-review. |
…+ tests
`sh tests/install_sh.sh` previously reported 2 failures (`shellcheck
install.sh`, `shellcheck tests/install_sh.sh`) — pre-existing on main
and not gated by CI. Investigation confirmed all 24 diagnostics are
false positives or controlled-context cases:
- SC2024 (install.sh, 2 sites): `(sudo -v </dev/tty)` provides a tty
for sudo's password prompt under `curl|sh`, not redirecting a
privileged file *through* sudo. Per-site `# shellcheck disable=`
with a brief inline reason.
- SC2016 (tests/install_sh.sh, 20 sites): single-quoted printf strings
carry literal backticks / ${...} for human display, and grep patterns
carry escaped \${...} that must stay literal. Expanding any of them
would corrupt output or regex matches. File-level disable since the
idiom is consistent across the file.
- SC2010 (tests/install_sh.sh, 2 sites): `ls | grep` operates on a
test-controlled tmpdir whose filenames are guaranteed to match
`config.toml.bak-<utc>-<pid>`. Per-site disable with a reason.
Also future-proofed two structural assertions in tests/install_sh.sh
(redirected `exec` handoff and `sudo -v </dev/tty` gating) so that
intervening shellcheck directives or clarifying comments above the
matched line no longer break the prev-line check. New helper
`_nearest_code_line` walks back over comments and blanks within a
10-line window.
Result: `sh tests/install_sh.sh` now reports 109/109 passing.
`shellcheck install.sh` and `shellcheck -s sh tests/install_sh.sh`
both exit 0 with no output.
Follow-up: clean shellcheck (commit c096374)Pushed a small follow-up addressing the 2 shellcheck failures the original PR description had disclosed as pre-existing. After investigating I confirmed all 24 diagnostics are false positives or controlled-context cases:
Annotation strategy:
Test-side future-proofingInserting the SC2024 directive line above Verification
PR description updated to reflect the new 109/109 baseline and to drop the "two failures are pre-existing" disclaimer. |
uzyn
left a comment
There was a problem hiding this comment.
Re-review (commits 33336bd + c096374)
Verified each previously-flagged item against the current code at c096374. All addressed; no new issues introduced.
Previous non-blockers — both resolved
book/installation.md:38— Step 9 of the install-flow numbered list now describes the recovery prompt, default-no semantics, non-TTY exit-0 path, and--forcebypass. Matches the script's behavior exactly.book/release-notes.md— New Unreleased sub-section after the existinginstall.shupgrade-path block covers both the recovery prompt and the brand capitalization sweep. Operator-visible behavior change is now documented in the changelog.
Previous nice-to-haves — all addressed
- Prompt placement — The
printfmoved into_prompt_read(install.sh:262, 265), gated behind the same TTY-availability checks that governread. The no-TTY return path at line 268 short-circuits before any output, socurl | sh </dev/nullno longer leaks the prompt into stderr. Cleaner factoring than duplicating the predicate. The new regression assertion (tests/install_sh.sh:1292assert_not_contains "prompt_reinstall stays quiet on no TTY") locks this in. - Dry-run gate —
[ "${DRY_RUN}" != "1" ] && prompt_reinstallatinstall.sh:1264short-circuits the prompt, falling straight through to the canonicalelsearm (exit 0). I traced this:AIMX_DRY_RUN=1+ equal version + no--forcereaches thesay "AIMX ... is already installed"branch and exits without ever callingprompt_reinstallor printing the dry-run plan. That matches the intent (auditing must stay non-interactive). - Accept-list coverage —
for _accept in y Y yes YES Yesloop replaces the two case-arm tests. A future contributor narrowing the accept list will now break a test rather than silently ship. - Equal-version + dry-run integration test — New test at
tests/install_sh.sh:1216-1237fakes a target-tagaimxat${PREFIX}/aimx, setsAIMX_DRY_RUN=1, and asserts both that "is already installed" prints and thatprompt_reinstalldoes NOT fire. Exercises the gate end-to-end throughmain(), not justprompt_reinstallin isolation.
Bonus: shellcheck cleanup (commit c096374)
The implementer also took care of the two pre-existing shellcheck failures the original PR description had disclosed:
- SC2024 × 2 (
install.sh) —(sudo -v </dev/tty)annotated per-site with the rationale (subshell +</dev/ttyis providing a TTY for sudo's password prompt undercurl|sh, not the shellcheck-targeted antipattern of redirecting a privileged file through sudo into the inner command). Reasoning is correct. - SC2016 × 20 (
tests/install_sh.sh) — file-level disable with a top-of-file comment explaining the idiom (single-quotedprintfliterals carrying${...}for human display, grep patterns carrying escaped\${...}). Per-line directives would have been noisy for an idiom this consistent — file-level is the right call. - SC2010 × 2 (
tests/install_sh.sh) — per-site disable on the twols "${_tmp}/etc/aimx/" | grep '^config.toml.bak-'sites. Tmpdir is test-controlled and filenames follow the predictableconfig.toml.bak-<utc>-<pid>pattern, so the SC2010 hazard cannot fire here.
Verification on my end:
sh tests/install_sh.sh→ 109/109 passing.shellcheck install.sh→ exit 0, no output.shellcheck -s sh tests/install_sh.sh→ exit 0, no output.
The new _nearest_code_line helper (tests/install_sh.sh:222-238) is a sensible response to the brittleness in the previous structural assertions: walking back over comments and blanks within a 10-line window means future shellcheck directives or clarifying comments above either matched line won't break the gating check. Pattern matching is correct (''|[[:space:]]*'#'*|'#'* cleanly identifies blanks, indented comments, and top-level comments without false-matching ordinary indented code).
End-to-end flow verification (re-traced from the current code)
--forceset →_is_upgrade=1→ upgrade path → binary swap + service restart.prompt_reinstallnever called.--forceunset,DRY_RUN=1, equal version →[ "${DRY_RUN}" != "1" ]short-circuits →elsearm →exit 0. No prompt, no dry-run plan output (which is fine — pre-PR exited 0 here too).--forceunset,DRY_RUN=0, equal version, prompt yes →_skip_swap=1,_is_upgrade=0→ download/extract gate atinstall.sh:1314skipped → upgrade gate atinstall.sh:1326skipped → fresh-install gate atinstall.sh:1385skipped (printsRe-running AIMX setup (binary unchanged)) →backup_existing_config→exec aimx setup.--forceunset,DRY_RUN=0, equal version, prompt no / no TTY →prompt_reinstallreturns 1 →elsearm →exit 0.--forceunset, older version →_is_upgrade=1→ upgrade path. Unaffected by this PR.- Binary missing entirely → outer
if [ -x "${_install_path}" ]skipped →_is_upgrade=0,_skip_swapunset → fresh install +exec aimx setup. Unaffected.
${_skip_swap:-0} parameter expansion handles the unset case under set -u. All branches verified.
No new issues
- No regressions in the non-equal paths (older / newer / unparseable / missing-binary / dry-run-with-force).
- Brand sweep is consistent:
grep -nE '(installing|installed|upgraded) aimx\b|\baimx is\b|aimx [0-9]+\.[0-9]'againstinstall.shreturns zero hits (asserted by the three branding tests). - Help text capitalization (
AIMX install scriptat install.sh:170) matches the test assertion attests/install_sh.sh:729. - No banned planning-doc cross-references introduced (verified
Sprint,FR-,S\d-\dpatterns).
Summary and Recommended Actions
- Overall verdict: Ready to merge.
- Blockers: none.
- Non-blockers: none.
- Nice-to-haves: none.
Recommended merge commit message
install.sh: prompt to re-run setup when AIMX is already installed + capitalize brand prose
When `curl ... | install.sh` finds the binary already at the target tag and `--force` was not passed, ask "AIMX is already installed. Re-run setup to (re)configure it? [y/N]" instead of hard-exiting. On `y`, skip the download/extract/install entirely and fall through to backup_existing_config + `exec aimx setup` (the same handoff fresh installs use). On `N` / Enter / no usable TTY (CI, scripted callers) / `AIMX_DRY_RUN=1`, keep the existing exit-0 semantics. `--force` continues to bypass the prompt.
Capitalize the brand name "AIMX" in operator-facing prose (install banner, port-check banner, help heading, "Linux-only" error, port-25 warn, "already installed" message, post-install/upgrade success lines, "Upgrade complete" sentence). Lowercase `aimx` is preserved for command literals, paths, service unit names, and the GitHub repo identifier.
Also silence pre-existing shellcheck false positives in `install.sh` and `tests/install_sh.sh` (SC2024, SC2016, SC2010) so the test suite reaches 109/109 with `shellcheck install.sh` and `shellcheck -s sh tests/install_sh.sh` both clean.
Docs:
- book/installation.md: rewrote step 9 of the install-flow list to describe the new prompt-on-equal-version behavior.
- book/release-notes.md: added an Unreleased entry covering both the recovery prompt and the brand capitalization sweep.
Summary
curl -fsSL https://aimx.email/install.sh | shis re-run and the binary already matches the target tag, ask "AIMX is already installed. Re-run setup to (re)configure it? [y/N]" instead of hard-exiting. Lets an operator who aborted partway throughaimx setup(e.g. to grab a different domain) resume without knowing about--force. Default is No so non-interactive callers (CI, scripted upgrades) keep today's exit-0 semantics;--forcestill bypasses the prompt.AIMX_DRY_RUN=1also bypasses the prompt — auditing the script stays non-interactive.backup_existing_config+exec aimx setup— the same handoff fresh installs already use.aimxstays for command literals (aimx setup,aimx doctor), service unit references (aimx.service), paths (/etc/aimx/...), and the repo identifier (uzyn/aimx).install.sh+tests/install_sh.sh(SC2024 on(sudo -v </dev/tty), SC2016 on intentional single-quoted printf/grep literals, SC2010 onls | grepover a test-controlled tmpdir) so the test suite reaches a clean 109/109.Why
Re-running
install.shafter aborting the wizard mid-flow is the natural "I got the wrong domain, let me start over" recovery path, but today it dead-ends onaimx 0.0.7 is already installed (pass --force to re-install)— a message that both implies a binary swap the operator doesn't need and isn't discoverable as a recovery handle. The existingbackup_existing_configrename already protects partial config from clobbering, so the right fix is to ask and fall through.The brand sweep was raised in the same conversation: the shell wrapper's prose had drifted from the wizard, which already uses
AIMX setup/Install AIMX/AIMX has been set up successfullycorrectly.Docs
book/installation.md— rewrote step 9 of the install-flow numbered list to describe the new prompt-on-equal-version behavior, default-no semantics, non-TTY exit-0 path, and--forcebypass.book/release-notes.md— added an Unreleased entry under the existinginstall.shblock covering both the recovery prompt and the brand-prose capitalization sweep.Test plan
sh tests/install_sh.sh— 109/109 passing.shellcheck install.shandshellcheck -s sh tests/install_sh.shboth exit 0 with no warnings.sh -n install.sh && sh -n tests/install_sh.sh— POSIX-clean._prompt_readstub._prompt_readsignalling no usable TTY, the prompt question itself does NOT appear in stderr.${PREFIX}/aimxreporting the target tag andAIMX_DRY_RUN=1, the recovery prompt is never invoked and the script exits 0 with the "AIMX X.Y.Z is already installed" line.(installing|installed|upgraded) aimx\b,\baimx is\b, andaimx [0-9]+\.[0-9]all return zero hits.install.sh→ confirm prompt appears, Enter exits 0,yskips the download and re-entersaimx setup.--force→ bypasses the prompt and re-installs.curl ... | sh </dev/null(no TTY) → exits 0 with the "already installed" line, no prompt printed.