Changes Overview

This PR auto-registers the aimx MCP server with Claude Code and Codex CLI as part of aimx agents setup <agent>, eliminating the manual claude mcp add / codex mcp add step that was Finding #7 from the 2026-04-17 manual test run. Secondary cleanup: Claude Code now installs to ~/.claude/skills/aimx/ (matching what the bundle is — a skill) instead of the previous ~/.claude/plugins/aimx/ plugin layout, with the installer wiping any pre-existing plugin tree on first run. A new agents_mcp module hosts an McpCli trait + RealMcpCli + NoopMcpCli so tests never spawn real CLIs against the developer's ~/.claude.json. The rewrite_plugin_args JSON-rewrite path and the bundled .claude-plugin/plugin.json are deleted; --data-dir flows through via the claude mcp add ... -- /usr/local/bin/aimx --data-dir <p> mcp argv directly.

Scope Alignment

The implementation matches the PR description exactly:

• Auto-registration of MCP for claude-code and codex via subprocess (try_auto_register_mcp in agents_setup.rs) with three classified outcomes (Registered / CliMissing / RegisterFailed).
• The fallback hint now includes the -- separator (claude mcp add --scope user aimx -- /usr/local/bin/aimx [--data-dir ...] mcp), addressing the previously-missing separator that broke --data-dir round-tripping.
• Layout migration to ~/.claude/skills/aimx/ with a cleanup_legacy_claude_plugin step that removes any pre-existing ~/.claude/plugins/aimx/ on setup.
• NoopMcpCli keeps cargo test from spawning real claude / codex against the developer's user-scope MCP registry; RealAgentEnv::mcp_cli() uses RealMcpCli.

No scope creep beyond what's described. Documentation (README.md, book/agent-integration.md, book/cli.md, agents/claude-code/README.md) is updated consistently.

Potential Bugs

Non-blocker: aimx agents remove claude-code does not clean up the legacy ~/.claude/plugins/aimx/ directory

agents_setup.rs::cleanup_legacy_claude_plugin removes the legacy plugin tree only on agents setup. agents_remove.rs and agents_cleanup.rs operate on spec.dest_template, which now resolves to ~/.claude/skills/aimx/. A user who installed with the old aimx, then runs aimx agents remove claude-code on the new aimx without first re-running agents setup, will be left with the orphaned ~/.claude/plugins/aimx/ directory.

Why non-blocker: Doesn't crash, lose data, or violate stated scope (the PR description focuses on agents setup, not agents remove). Users following the recommended setup → remove sequence are unaffected. Worth a follow-up commit but does not block merge.

Suggested fix path: Have agents_remove::do_remove also probe for and remove ~/.claude/plugins/aimx/ when spec.name == "claude-code", printing the same "Removed legacy plugin install at..." line for symmetry.

Non-blocker: error message after RegisterFailed may be misleading when stderr is empty

agents_setup.rs:932:

stderr.lines().next().unwrap_or("(no error output)")

If claude mcp add exits non-zero with empty stderr (e.g. some future Claude Code release), the user sees:

[!] MCP auto-registration failed: (no error output)

then the fallback hint. Fine, but the captured exit code is dropped — surfacing it in the message would help debugging. Trivial improvement; not blocking.

Security Issues

None observed. Subprocess argv is built from a fixed AIMX_BINARY = "/usr/local/bin/aimx" literal plus data_dir (which is operator-supplied via --data-dir and passed as a separate argv element, not interpolated into a shell string). NoopMcpCli defends against the developer-machine footgun of cargo test spawning real claude mcp add against the user's own registry.

Test Coverage

Strong coverage on the new agents_mcp module:

• register_claude_no_data_dir_uses_canonical_argv — exact argv assertion, including the -- separator.
• register_claude_with_data_dir_threads_path_into_argv — verifies --data-dir is inside the subprocess tail (not parsed by claude mcp add itself).
• register_claude_propagates_cli_missing / register_claude_propagates_register_failed_with_stderr — both error paths.
• register_ignores_remove_failure_when_add_succeeds — pins the documented "remove is best-effort" semantics.
• Codex variants mirror the Claude variants and cover the --scope-flag absence.

End-to-end tests in install_to_writer exercise the CliMissing branch via NoopMcpCli (which the MockEnv defaults to). Two new tests (install_claude_code_removes_legacy_plugins_dir, print_mode_skips_legacy_cleanup) cover the legacy cleanup behavior including the --print skip path.

Test gap (non-blocker): the Registered success branch in install_to_writer is not exercised end-to-end

install_to_writer has three observable code paths after try_auto_register_mcp:

• Registered → prints [ok] MCP server registered with <agent>.
• CliMissing → prints the manual hint
• RegisterFailed { stderr } → prints [!] MCP auto-registration failed: ... then the manual hint

NoopMcpCli always returns CliMissing, so no test in agents_setup::tests asserts the printed MCP server registered with Claude Code. line or the MCP auto-registration failed: warning string. Adding a MockCli-backed MockEnv variant in the agents_setup test module that returns ok() from RealMcpCli::run would close the gap. Unit-level coverage of register_claude / register_codex is already strong, so this is a non-blocker.

Code Quality

• The try_auto_register_mcp / cleanup_legacy_claude_plugin split is clean — both are short, single-purpose, and well-commented.
• NoopMcpCli doc comment is explicit about why it exists (preventing tests from mutating the developer's ~/.claude.json). Good.
• Legacy plugin.json rewrite path (rewrite_plugin_args) and its dedicated test are deleted, not just orphaned. Correct.
• One minor observation: try_auto_register_mcp matches on spec.name (a &str); future agents that grow registration CLIs would extend this match. Acceptable, but if more than 2 agents register this way, a per-spec function-pointer in AgentSpec would scale better. Not actionable for this PR.

Summary and Recommended Actions

• Overall verdict: Ready to merge
• Blockers: none
• Non-blockers:
  • aimx agents remove claude-code doesn't clean up ~/.claude/plugins/aimx/ — worth a follow-up commit for symmetry with agents setup.
  • RegisterFailed message drops the exit code; surface it for easier debugging.
  • Test gap: Registered and RegisterFailed branches in install_to_writer aren't asserted end-to-end (unit tests on register_claude / register_codex cover the underlying logic).
• Nice-to-haves:
  • Manual smoke checklist in the PR description still has unchecked items — validate against a real claude / codex on PATH before relying on this in production.

The blocking concern from the PR description (Finding #7 from the manual test run) is fully addressed: one command now wires up Claude Code / Codex with the MCP server, and the fallback hint correctly includes the -- separator so --data-dir round-trips. The layout migration to ~/.claude/skills/aimx/ is consistent across code, tests, docs, and the legacy cleanup path. CI is green across all checks.

Recommended merge commit message

feat(agents): auto-register MCP server for Claude Code & Codex

`aimx agents setup claude-code` (and `codex`) now auto-runs `claude mcp
add` / `codex mcp add` after writing the skill files, so the MCP server
is wired with one command. The fallback hint (printed when the agent CLI
isn't on PATH) gains the previously-missing `--` separator so
`--data-dir` round-trips through `claude mcp add` correctly.

Claude Code now installs to `~/.claude/skills/aimx/` (auto-discovered
under `~/.claude/skills/`) instead of the previous
`~/.claude/plugins/aimx/` plugin layout. The installer wipes any
pre-existing `~/.claude/plugins/aimx/` from older installs so the new
skills install isn't shadowed.

A new `agents_mcp` module hosts the `McpCli` trait, `RealMcpCli`, and a
`NoopMcpCli` used by `MockEnv` test harnesses so `cargo test` never
spawns a real CLI against the developer's `~/.claude.json`. The bundled
`.claude-plugin/plugin.json` and the now-dead `rewrite_plugin_args`
JSON-rewrite path are removed; `--data-dir` flows through to `claude mcp
add` directly.

Re-review (after be97208)

All three non-blockers from the previous review are addressed correctly. Verified independently by reading the diff, running the test suite, and checking each fix end-to-end.

Resolved

1. Symmetry gap on agents remove claude-code — fixed.
agents_remove::do_remove now calls a cleanup_legacy_claude_plugin helper that mirrors the setup-side helper exactly: probe ~/.claude/plugins/aimx/, remove_dir_all, print an info line on success or a warn line on failure. Cleanup is gated to spec.name == "claude-code", so agents remove codex against a host with an unrelated ~/.claude/plugins/aimx/ doesn't touch it. Two new tests pin both behaviours: agents_remove_claude_code_removes_legacy_plugins_dir (positive) and agents_remove_non_claude_code_does_not_touch_legacy_plugins_dir (negative).

2. RegisterFailed exit code — fixed.
McpRegistration::RegisterFailed now carries exit_code: Option<i32>. classify populates Some(out.status.code()) for non-zero exits and None for the io::Error path (other than NotFound, which still becomes CliMissing). The install_to_writer warn-line emits (exit N) after the stderr summary when Some, falls back to the original shape when None. Three test cases cover this: existing stderr-with-exit, new empty-stderr-with-exit, new PermissionDenied io::Error (no exit).

3. End-to-end coverage of Registered / RegisterFailed — fixed.
A MockMcpCli (queue of io::Result<Output> responses) plus a WithMcpEnv wrapper that overrides only AgentEnv::mcp_cli() lets the tests drive install_to_writer end-to-end against a programmable CLI without changing NoopMcpCli's safe default. install_claude_code_with_real_cli_prints_registered_line asserts the success confirmation appears AND that the manual fallback hint does NOT (a regression-prevention assertion the original review didn't even ask for, nice). install_claude_code_with_failed_cli_prints_warning_and_fallback asserts the warn-line includes both the stderr summary and (exit 1), plus the fallback hint follows.

New issues

None. The fixes are scoped tightly: only the three call sites flagged in the previous review are touched, no drive-by refactors or scope creep.

Verification

• cargo fmt -- --check — clean
• cargo clippy --all-targets -- -D warnings — clean
• cargo test --bin aimx — 1001 passed, 0 failed, 6 ignored (was 995; +6 new tests as claimed)
• cargo test --tests — 1001 unit + 92 integration pass; same root-only / two-uid suite stays ignored as before
• Targeted runs:
  • cargo test --bin aimx agents_remove — 7 passed (including both new ones)
  • cargo test --bin aimx agents_setup::tests::install_claude_code_with — 2 passed (both new ones)

Summary

All three non-blockers from the previous review are correctly fixed with strong test coverage, no regressions, and no new issues. Ready to merge.

Recommended merge commit message

feat(agents): auto-register MCP server for Claude Code & Codex

`aimx agents setup claude-code` (and `codex`) now auto-runs `claude mcp
add` / `codex mcp add` after writing the skill files, so the MCP server
is wired with one command. The fallback hint (printed when the agent CLI
isn't on PATH) gains the previously-missing `--` separator so
`--data-dir` round-trips through `claude mcp add` correctly.

Claude Code now installs to `~/.claude/skills/aimx/` (auto-discovered
under `~/.claude/skills/`) instead of the previous
`~/.claude/plugins/aimx/` plugin layout. Both `agents setup claude-code`
and `agents remove claude-code` now wipe any pre-existing
`~/.claude/plugins/aimx/` from older installs so the new skills install
isn't shadowed and remove leaves no orphaned tree.

A new `agents_mcp` module hosts the `McpCli` trait, `RealMcpCli`, and a
`NoopMcpCli` used by `MockEnv` test harnesses so `cargo test` never
spawns a real CLI against the developer's `~/.claude.json`.
`McpRegistration::RegisterFailed` carries the child exit code so
auto-registration warn lines remain debuggable when stderr is empty.
The bundled `.claude-plugin/plugin.json` and the now-dead
`rewrite_plugin_args` JSON-rewrite path are removed; `--data-dir` flows
through to `claude mcp add` directly.