Changes Overview

This PR rips out the aimx setup Step 6 drop-through that re-execed aimx agents setup as $SUDO_USER via runuser -l -s /bin/bash -- -ic <argv>. In its place, Step 6 now prints a static guidance block — header sentence, bullet list of every agent's display_name (driven by agents_setup::registry()), and a → aimx agents setup operator-action callout — then marks the step Handoff (new term::StepState::Handoff, ⎘ glyph / [*] ASCII) and exits. display_mcp_section loses its data_dir parameter (the picker handles per-data-dir dispatch now). Net 595 deletions / 116 additions across src/setup.rs and src/term.rs.

Scope Alignment

The PR's stated intent is "replace Step 6 drop-through with guidance + supported-agents list," and the code does exactly that. The rationale (bash job-control / SIGTTOU hangs from -i, doesn't scale across 8 agents) is sound, and the new shape matches the apt / gh auth login privilege-boundary idiom called out in the description. The supersedes/closes relationship with #221 is appropriate — #221 was a surgical fix for a single failure mode; this PR removes the entire class.

One real scope gap, addressed in Blockers below: the project's documented rule that user-facing book pages get updated alongside CLI-output changes was not followed.

Potential Bugs

None found in the production code. The new Step 6 is a few println!s plus checklist.set(6, Handoff) plus a single print_final_banner call — no state mutation, no error paths, no env-var dispatch, no subprocesses. display_mcp_section() is data-dir-agnostic; the only caller is run_setup Step 6. Deleted symbols (drop_through_to_agents_setup, build_agents_setup_argv, AgentsSetupOutcome, explicit_data_dir) have no remaining references in src/ or tests/ other than the negative-grep assertions that forbid re-introducing them.

The \n}\n regex in run_setup_step_six_uses_handoff_state to bound the run_setup body relies on rustfmt placing the closing brace at column 0 followed by a newline — brittle but the same shape the pre-existing source-grep guards already use, so consistent.

Test Coverage

The replacement tests are appropriate for what's changing:

• mcp_section_recommends_running_aimx_agents_setup — positive guard on the new copy + negative guards forbidding the old per-agent aimx agents setup <name> enumeration, the aimx agents list reference, and --data-dir threading.
• mcp_section_lists_every_registered_agent — registry-driven; adding a new agent automatically extends Step 6 output and the test asserts every display_name shows up. Good.
• run_setup_step_six_uses_handoff_state — source-greps for term::StepState::Handoff and forbids runuser, AgentsSetupOutcome, drop_through_to_agents_setup. This will false-negative if a future refactor aliases the module (use term::StepState as S etc.) but matches the project's existing source-grep regression-guard style.
• step_glyphs_use_unicode_on_tty / step_glyphs_use_ascii_fallback_on_non_tty extended to cover Handoff (⎘ / [*]).

Gap I noticed but classify as Non-blocker: no test asserts the line ordering in mcp_section_lines (header → blank → bullets → blank → "Run as your regular user…" → blank → callout). A future refactor that scrambles the order would slip through both new tests. The existing tests verify presence but not flow.

Code Quality

• StepState::Running is now reserved (no caller) and kept with #[allow(dead_code)] + a rationale comment pointing at "future long-running surfaces (aimx upgrade)." Per CLAUDE.md's "Don't design for hypothetical future requirements" this is borderline — the variant is still referenced by step_glyph (match arm) and the new tests (let running = step_glyph(StepState::Running).to_string()), so it's not technically dead and won't actually trigger the dead-code lint. The #[allow(dead_code)] is harmless but unnecessary, and the "kept for future" framing is the part that rubs against the convention. Easy to drop the attribute and the future-use comment without functional change. Non-blocker.
• The Step 6 inline comment block in run_setup (10 lines explaining why this is a handoff, not automation) is the kind of why-comment CLAUDE.md does endorse. Keep.
• display_mcp_section losing its data_dir parameter is a small but real API simplification — the inner if data_dir == Path::new("/var/lib/aimx") { … } else { "aimx --data-dir <path> agents setup …" } branch was already a code smell. Good removal.

Human Review Comments

No PR comments and no review history on #223 at the time of this review. PR #221 (the surgical-fix predecessor this supersedes) is referenced in the description but has no review comments worth carrying forward.

Summary and Recommended Actions

Overall verdict: Needs minor fixes.

The Rust changes are clean and the redesign is the right architectural call. The reason this isn't a clean approve is that the project explicitly documents "When making changes that affect CLI behavior, setup output, or MCP tools, update the corresponding guide files too" (CLAUDE.md), and the book pages still describe the drop-through that just got deleted. Anyone reading book/setup.md after this lands will get instructions that don't match what aimx setup does.

Blockers

1. Stale user-facing book documentation. The PR materially changes Step 6's visible behavior but doesn't update the corresponding book/ pages. Concrete stale passages:

   • book/setup.md:39 — checklist legend says "ticking ☐ → ☑ (or ☒ skipped)". Needs the new ⎘ Handoff terminal state added.
   • book/setup.md:46 — "Wire MCP for agents. Re-execs aimx agents setup as $SUDO_USER (via runuser) so the agent picker takes over the terminal. Skipped on direct root login or missing runuser … Skipped under AIMX_NONINTERACTIVE=1." — every clause is now factually wrong.
   • book/setup.md:86 — "Step 6 runs aimx agents setup as your regular user and presents an interactive picker." — Step 6 no longer spawns the picker.
   • book/setup.md:88 — "When you logged in directly as root (no sudo), step 6 marks ☒ skipped." — Step 6 now always marks ⎘ Handoff regardless of $SUDO_USER.
   • book/setup.md:92 — "AIMX_NONINTERACTIVE=1 … skips the agents-setup drop-through (no TTY assumed)." — there is no drop-through to skip; Step 6 prints the same guidance under AIMX_NONINTERACTIVE=1 as without it.
   • book/setup.md:100 — "only the preflight, DNS verification, and agents-setup steps run" — agents-setup is no longer a wizard-driven step.
   • book/agent-integration.md:64–66 — "When sudo aimx setup completes, the wizard drops through to aimx agents setup as the invoking user (via runuser -u $SUDO_USER -- /proc/self/exe agents setup) … Under AIMX_NONINTERACTIVE=1, the drop-through is skipped" — entire paragraph describes deleted behavior.
   • book/agent-integration.md:70 — "…and is never passed implicitly by aimx setup's drop-through." — drop-through gone, this caveat is meaningless.
   • book/installation.md:9 — "runs sudo aimx setup, then drops back to your user to wire the MCP server into your agent." — the install flow no longer drops back; the operator runs aimx agents setup themselves after setup exits.
   • book/installation.md:77 — "The drop-through to aimx agents setup also uses /proc/self/exe, so a non-default prefix works end-to-end without extra configuration." — paragraph is about a code path that no longer exists.

   Rewrite each of these to describe the new Step 6 shape (Handoff glyph, inline supported-agents list, explicit aimx agents setup follow-up by the operator), and update the checklist-legend line to enumerate ⎘.

Non-blockers

1. Stale comments in install.sh reference the deleted drop-through:
   • install.sh:322 (detect_invoker block comment) — "the wizard's drop-through reads $SUDO_USER directly so this is not on the live install path anymore." The helper is still present-but-unused; consider deleting detect_invoker outright or, at minimum, removing the drop-through reference.
   • install.sh:1394 — "the six-step checklist, the agents setup drop-through, and the closing message" in the exec aimx setup hand-off comment. Drop the middle clause.
2. StepState::Running kept for hypothetical "future long-running surfaces." Per CLAUDE.md, prefer to delete and re-introduce when a real caller materializes. The variant is still match-armed in step_glyph and asserted on by the existing tests, so leaving it doesn't cost much, but the #[allow(dead_code)] is unnecessary (it's not actually dead) and the future-use framing reads against the project's "no speculative API surface" convention.
3. No test pins the line ordering of mcp_section_lines. Both new tests check presence (substring matching). A future refactor that, say, moved the bullets below the callout would pass both tests but produce confusing output. Cheap to add an assertion that verifies the four landmark lines appear in the documented order.

Nice-to-haves

• The ⎘ (U+2398 NEXT PAGE) glyph isn't widely associated with "handoff" — terminal fonts handle it inconsistently. The ASCII fallback [*] is fine. Not worth changing for this PR, but worth tracking if operators report rendering oddities.
• Handoff rendered via truecolor(185, 83, 28) will emit 24-bit ANSI escapes on every TTY where colorize_active() is true; old/limited terminals without truecolor support may render the escape as visible artifacts. Most current terminals are fine; logging this for awareness only.

Re-review of commit 942bc6c on PR #223

The implementer addressed every issue from the previous review. I verified each one independently against the current tree.

Previously flagged issues — verification

[Blocker 1 — RESOLVED] Stale book/ documentation describing the deleted Step 6 drop-through. All 10 specific passages are rewritten:

• book/setup.md:39 — checklist legend now enumerates ☐ / ☑ / ☒ / ⎘.
• book/setup.md:46 — Step 6 rewritten to describe the guidance section, bullet list, → aimx agents setup callout, and ⎘ Handoff terminal state.
• book/setup.md:86 — "Wiring agents" section now explicitly says Step 6 prints the list and exits without spawning the picker.
• book/setup.md:88 — code fence shows aimx agents setup invocation as the operator follow-up.
• book/setup.md:92 — picker description moved to the operator-initiated follow-up step.
• book/setup.md:100 — non-interactive section no longer mentions a "drop-through that is skipped"; clarifies Step 6 is unaffected by AIMX_NONINTERACTIVE.
• book/agent-integration.md:64–66 — section renamed "Following aimx setup", body rewritten to describe the handoff model.
• book/agent-integration.md:70 — "and is never passed implicitly by aimx setup's drop-through" trailer removed.
• book/installation.md:9 — "drops back to your user" rephrased as a separate operator step.
• book/installation.md:77 — custom-prefix section rewritten ("/proc/self/exe drop-through" → "resolves itself from $PATH, or invoke by full path").

rg 'drop[ -]?through|drops back' across the repo returns no hits outside src/setup.rs:6211-6243 (the run_setup_step_six_uses_handoff_state test that forbids re-introduction — correct by design).

[Non-blocker 1 — RESOLVED] Stale install.sh comments.

• install.sh:322 (detect_invoker block) — "the wizard's drop-through reads $SUDO_USER directly" line replaced with "nothing in the live install path calls it today." The helper itself is preserved, which is correct: tests/install_sh.sh exercises it in 3 separate blocks (lines 145, 157, 297-326). Deleting detect_invoker would have broken out-of-scope tests.
• install.sh:1394 (the exec aimx setup hand-off comment) — "the agents setup drop-through" stripped from the comment.

[Non-blocker 2 — RESOLVED] StepState::Running doc comment speculative framing. The "future long-running surfaces" framing is replaced with concrete reasoning ("Kept for the step_glyph match-arm and the step_glyphs_* tests"). The #[allow(dead_code)] attribute is kept, which I independently verified is genuinely required:

• Removing it causes cargo clippy --bin aimx -- -D warnings to fail with error: variant 'Running' is never constructed at src/term.rs:151.
• It also fails with cargo clippy --all-targets -- -D warnings (which CI runs at .github/workflows/ci.yml:35,181), because Rust's dead-code lint does not credit #[cfg(test)] constructors against the non-test target.

So the implementer's pushback is correct. The compromise — tighten the doc, keep the attribute — is the right call.

[Non-blocker 3 — RESOLVED] No test asserts the ordering of mcp_section_lines. New test mcp_section_lines_are_in_documented_order at src/setup.rs:3933-3999 uses .position(...) to find each landmark (header, first bullet, run-as-user sentence, callout), asserts strict less-than ordering, and additionally verifies blank-line separators at exact relative indices. Not a weak presence-check — a refactor that scrambles the flow will fail loudly. Confirmed passing: setup::tests total is now 312 (up from 311).

Build / lint / test verification

• cargo clippy --all-targets -- -D warnings — clean (run locally on the head commit 942bc6c).
• cargo fmt -- --check — clean (no output).
• cargo test --bin aimx setup::tests:: — 312 passed.
• cargo test --bin aimx term::tests:: — 11 passed.

New issues introduced by the fix commit

None.

Items the implementer flagged as out-of-scope

• StepState::Error (lines 160–166 in src/term.rs) still carries similar speculative future-use framing ("future surfaces (e.g. aimx upgrade's rollback path) may use it"). This was not in the original review scope and the implementer didn't touch it. Not asking for it now — but worth a one-line follow-up if the project is enforcing "no speculative API surface" consistently.
• Nice-to-haves from the prior review (terminal-font coverage of ⎘, truecolor fallback on non-24-bit terminals) — intentionally deferred. Acknowledged.

Verdict

Ready to merge.

All previously-flagged issues are resolved or have correct, verified pushback. The fix commit 942bc6c is doc/comment-only with one new test addition; it introduces no new code paths, no new dependencies, and no regressions.

Recommended merge commit message

setup: replace Step 6 drop-through with operator-initiated agents setup

The Step 6 drop-through (sudo aimx setup re-execing `aimx agents setup`
as $SUDO_USER via runuser) produced two failure modes in three days
(silent CliMissing branch, then bash job-control deadlock on -i) and
did not scale across the 8 supported agents. Replace it with a
guidance section: print the supported-agents bullet list (driven by
agents_setup::registry() so adding an agent extends Step 6
automatically) and a `→ aimx agents setup` callout, then exit. The
closing checklist marks step 6 with a new ⎘ Handoff glyph (copper
accent on TTY, [*] ASCII fallback) distinct from ☑ Done and ☒ Skipped.

User-level agent wiring is now an operator-initiated follow-up — the
same idiom as `apt install` / `gh auth login`.

Net: 595 lines deleted, 116 added. Updated book/setup.md,
book/agent-integration.md, book/installation.md, and install.sh
comments to match. Added regression guards:

  - mcp_section_recommends_running_aimx_agents_setup
  - mcp_section_lists_every_registered_agent
  - mcp_section_lines_are_in_documented_order
  - run_setup_step_six_uses_handoff_state (forbids runuser /
    AgentsSetupOutcome / drop_through_to_agents_setup re-introduction)

Supersedes #221 (the surgical hang fix).