Changes Overview

This PR makes two related fixes to the aimx setup first-run experience:

1. install.sh gains a setup_completed helper (probes /etc/systemd/system/aimx.service and /etc/init.d/aimx, both overridable via env vars). The upgrade-vs-fresh decision in main() is gated on it so a Ctrl+C-interrupted setup re-enters cleanly instead of dying on systemctl start aimx.
2. src/setup.rs reworks the closing handoff: the 🐦‍⬛ AIMX setup — complete banner moves between Step 5 and Step 6, the aimx agents setup callout becomes a three-line box with hard-coded → and an equal-width invariant, the closing capability line is rewritten to stop overclaiming agent wiring, and the success line is prefixed with a glyph intended to mirror the per-step checklist's ☑.

Scope Alignment

The PR description, the linked issue body, and the diff line up on most points — but there is one inconsistency around the final success-line glyph that the implementation does not honor. See Potential Bugs below.

Banned planning-doc tokens check (\bSprint\b|\bS\d+[-.]\d+\b|\bFR-\d+[a-z]?\b|User Story|Acceptance criteria) against the PR's diff additions: zero hits. Pre-existing tokens in the repo are not touched.

Potential Bugs

Blocker 1 — Closing success line renders ✓, not ☑, despite explicit stated intent

The linked issue body (#229) explicitly promises:

Prefixes "AIMX has been set up successfully." with the same green ☑ glyph the checklist uses, so it reads as the final box checked.

…and its rendered example shows ☑ AIMX has been set up successfully.

The PR description's R11 claims the same: "line is prefixed with term::success_mark() (☑ on TTY, [OK] on non-TTY)".

The new doc comment on print_closing_message (src/setup.rs:2708-2710) reads:

The success line is prefixed with the same ☑ glyph the per-step checklist uses (via term::success_mark)

But term::success_mark() in src/term.rs:114-120 returns "✓".green() on TTY and "[OK]".normal() on non-TTY. The per-step checklist's ☑ glyph comes from term::step_glyph(StepState::Done) (src/term.rs:179), which is a different function returning a different glyph.

So the operator will see ✓ AIMX has been set up successfully. on TTY (not ☑) and [OK] AIMX has been set up successfully. on non-TTY (not [x]). The intended visual parity with the checklist row that just rendered ☑ Install AIMX is broken.

Two viable fixes:

• Swap term::success_mark() → term::step_glyph(term::StepState::Done) in print_closing_message. This delivers ☑/[x] to match the checklist exactly.
• Introduce a term::done_mark() helper and use it (if success_mark rendering elsewhere depends on ✓).

The test closing_message_carries_required_phrasing does not catch this because it only greps the source for the literal string term::success_mark(). It asserts the function is called, not that the resulting glyph matches the stated intent. After fixing, the test should also assert against the rendered glyph (e.g. compare against term::step_glyph(StepState::Done).to_string() so a future swap of success_mark to a non-☑ rendering does not silently revert this).

Classified as Blocker because it violates the explicitly stated scope/intent (issue #229 body + PR description R11 + inline doc comment).

Test Coverage

• mcp_section_callout_box_is_equal_width (new): solid. Checks top/body/bottom char-count equality (correctly using chars().count() for multi-byte box glyphs), asserts the body contains the literal →, AND source-greps inside mcp_section_lines for a regression of term::prompt_mark reintroduction. This catches the exact desync class the PR description calls out.
• setup_completed gate (a-e, new in tests/install_sh.sh): covers the systemd-only branch, openrc-only branch, neither-present branch, the end-to-end "binary at target tag + no unit → fall-through" path, and a regression assertion that genuine version-diff upgrades still take the upgrade branch. Good coverage.
• The new closing_message_carries_required_phrasing test is too weak in one place — it asserts source.contains("term::success_mark()") rather than asserting the rendered output mirrors term::step_glyph(StepState::Done). This is what let Blocker 1 slip through. Tightening this test is part of the recommended fix.
• The new closing_message_does_not_overclaim_agent_capability test correctly locks the old overclaim out by greping inside the print_closing_message function body only, so it cannot false-pass on a literal that survives elsewhere in the file. Good.
• The renamed final_banner_signals_completion test correctly asserts print_step_list is NOT called inside the banner body (function-body-bounded grep). Solid.

Code Quality (Non-blocker)

• print_final_banner(_checklist: &Checklist) now ignores its only parameter. Underscore prefix silences clippy, but the API is misleading — every caller now passes a value that is read once (the & borrow check) and then dropped. Consider either dropping the parameter or wiring it back into a single state-of-the-wizard summary (e.g. a one-liner "✓ all 5 install steps complete; 1 handoff remaining"). Keeping the param to "preserve API surface" is weak given this is a pub helper in a binary crate with no external consumers.
• The INNER_WIDTH = 29 constant in callout_box has a clear inline justification, and the test will fail loudly if the command is renamed. Acceptable, though pulling the constant onto the call site (callout_box(" → aimx agents setup", 29)) or computing it from the body length plus padding would make the coupling between body content and pad width more obvious. Non-blocker, design-taste call.
• The new setup_completed helper in install.sh mirrors the existing helper style (env-var overridable probe paths). Good fit with the rest of the file.

Pre-existing macOS test noise (verified unrelated)

The implementer flagged uds_authz (lookup_unknown_uid_returns_none, peer_username_does_not_panic_on_orphan_uid, peer_username_orphan_uid_returns_specific_error) and integration::concurrent_ingest_burst_and_mark_same_mailbox_no_torn_writes as failing on Darwin hosts but passing on Linux CI. I confirmed:

• The three uds_authz tests exercise lookup_username / peer_username for orphan uids (e.g. 4_294_967_294). macOS NSS resolves that uid to "nobody" while glibc returns None. None of the diff touches src/uds_authz.rs or the lookup path; the failures are host-level NSS behaviour, not regressions.
• The tests/install_sh.sh bash 3.2 + set -e quirk on Darwin is an artifact of /bin/sh being POSIX-mode bash 3.2 on macOS. Linux CI uses dash/bash. Verified by .github/workflows/ci.yml → all four runs-on: entries are ubuntu-latest. The harness's new section 9b runs on the same sh "${INSTALL_SH}" invocation pattern as the surrounding sections — no new Darwin-specific brittleness introduced.

These failures are not caused by this diff and CI will not see them.

Install.sh re-entry — verified clean

Trace through the Ctrl+C scenario (binary on disk at the same TAG, no service unit):

1. [ -x "${_install_path}" ] && ! setup_completed → true, _is_upgrade=0, emits "continuing setup".
2. Non-upgrade path: tarball downloaded, install -m 0755 overwrites the binary at the same path (effective no-op for the same tag).
3. backup_existing_config moves any partial /etc/aimx/config.toml aside.
4. exec ${SUDO} aimx setup.
5. run_setup calls is_already_configured (src/setup.rs:2286), which requires service_running && cert_exists && dkim_exists. Post-Ctrl+C, the service unit was never written → is_service_running returns false → already_configured is false → wizard re-enters from preflight. Clean.

One minor cosmetic in this flow: ui_success "AIMX ${TAG} installed to ${_install_path}" will print even when the binary on disk is already at TAG. Acceptable noise — it's a true statement about disk state and the operator just saw the "continuing setup" line.

Box geometry — verified

callout_box(" → aimx agents setup") produces three 33-char-wide lines (2-char indent + corner/pipe + 29-char inner + corner/pipe). Body content is 21 chars, so 8 chars of right padding. The equal-width test correctly uses chars().count() (not len()) so the multi-byte box glyphs and → arrow don't break the assertion. The test also defends against term::prompt_mark() being reintroduced — without this guard, a future refactor could silently desync the corners on non-TTY because [>] is wider than →.

Summary and Recommended Actions

• Overall verdict: Needs minor fixes
• Blockers:
  1. Closing success line uses term::success_mark() (renders ✓ / [OK]) instead of the ☑ / [x] glyph promised by both the linked issue body and PR description R11. Fix by routing through term::step_glyph(term::StepState::Done) and tightening closing_message_carries_required_phrasing to assert the rendered glyph, not just the function name.
• Non-blockers:
  • print_final_banner parameter is now unused; either drop or rewire.
  • Consider hoisting INNER_WIDTH to the call site for clearer coupling with the body literal.
• Nice-to-haves: none.

Re-review — cycle 2 (commit 2d5672d)

All three items from the cycle-1 review are verified fixed. Approving.

Resolved issues

Blocker — closing success line now uses the checklist glyph. Verified at src/setup.rs:2718 — print_closing_message now calls term::step_glyph(term::StepState::Done) instead of term::success_mark(). Rendering: ☑ on TTY, [x] on non-TTY, matching the per-step checklist row exactly. The in-function doc comment (lines 2710-2713) was updated to reference term::step_glyph and explicitly call out that success_mark would NOT match.

Test strengthening — closing_message_carries_required_phrasing is now genuinely glyph-aware (src/setup.rs:6144-6183). The cycle-1 source-grep-only failure mode is closed off by:

1. Bounding the grep to the print_closing_message body (extracts fn print_closing_message( → first \n}\n), not the whole file. Verified that the function body contains no nested braces that could cause premature termination.
2. Positive assertion: body contains term::step_glyph(term::StepState::Done).
3. Negative assertion: body does NOT contain term::success_mark — this is the exact regression class.
4. Runtime rendering assertion: actually calls term::step_glyph(term::StepState::Done).to_string() and asserts the rendered output contains '☑' or "[x]". This defends against the helper internals being swapped away from the checklist glyphs — not just against the source string changing.

Confirmed against src/term.rs:174-194: step_glyph(Done) returns ☑.green() on TTY or [x].normal() on non-TTY, so the assertion covers both real-world outputs.

Non-blocker — print_final_banner parameter dropped. Verified at src/setup.rs:2415 (signature is now pub fn print_final_banner()) and src/setup.rs:2684 (single call site updated to print_final_banner();). Repo-wide grep confirms no other callers — print_final_banner is referenced only inside src/setup.rs.

Non-blocker — inner_width hoisted to call site. Verified at src/setup.rs:1437 (call site is callout_box(" → aimx agents setup", 29) with a six-line comment above it explaining the 21-char body + 8-char right padding coupling) and src/setup.rs:1448-1458 (signature is fn callout_box(body: &str, inner_width: usize); the private INNER_WIDTH = 29 const is gone). The coupling between body content and pad width is now visible together at the call site.

Verification

• cargo test --bin aimx setup::tests:: → 313 passed, 0 failed.
• cargo test --bin aimx closing_message_carries_required_phrasing → 1 passed.
• cargo clippy --bin aimx -- -D warnings → clean.
• cargo fmt -- --check → clean.
• Banned planning-doc tokens (\bSprint\s*\d+, \bS\d+[-.]\d+\b, \bFR-\d+[a-z]?\b, User Story, Acceptance criteria) against the cycle-2 diff: zero hits.
• Files touched in cycle 2: src/setup.rs only. install.sh and tests/install_sh.sh untouched — the setup_completed gate side of the PR is unchanged and the pre-existing macOS-host test noise (uds_authz, integration concurrency, install.sh bash-3.2 quirk) is by definition not regressed.

No new issues

No regressions, no scope creep, no new bugs introduced. Cycle-2 is a tight, surgical fix.

Summary and recommended actions

• Overall verdict: Ready to merge.
• Blockers: none.
• Non-blockers: none.
• Nice-to-haves: none.

Recommended merge commit message

setup: fix install re-run after Ctrl+C + clearer Step 6 handoff (#231)

install.sh: add `setup_completed` helper (probing
/etc/systemd/system/aimx.service or /etc/init.d/aimx, with env-var
overrides for tests) and gate the upgrade-vs-fresh decision on it. A
Ctrl+C-interrupted `aimx setup` leaves the binary on disk but no
service unit; re-running the installer used to die on
`systemctl start aimx`. The gate forces the fresh-install path in
that state so the wizard re-enters cleanly. Genuine version-diff
upgrades are unchanged.

src/setup.rs: rework the closing handoff:
- Move the `🐦‍⬛ AIMX setup — complete` banner between Step 5 and
  Step 6 (no second checklist redraw underneath); drop the now-
  unused `&Checklist` parameter from `print_final_banner`.
- Reshape Step 6's `aimx agents setup` callout into a three-line
  box preceded by a "run as a regular user" sentence; pad width
  hoisted to the call site so body content and pad width sit next
  to each other.
- Rewrite the closing capability line to stop overclaiming agent
  wiring; the success line is now prefixed with
  `term::step_glyph(StepState::Done)` (☑ / [x]) to mirror the
  per-step checklist glyph.
- Lock the new behaviour with `mcp_section_callout_box_is_equal_width`,
  `closing_message_carries_required_phrasing` (now glyph-aware at
  runtime), `closing_message_does_not_overclaim_agent_capability`,
  and `final_banner_signals_completion`.

Closes #229.