fix: route aimx hooks CLI through daemon UDS for non-root callers

[uzyn]

Summary

aimx hooks list (and create / delete) failed for a non-root operator with Error: Permission denied (os error 13). They now work without sudo for a mailbox owner, just like aimx mailboxes already does.

Root cause: in src/main.rs, Command::Hooks fell into the catch-all other dispatch arm which eagerly ran Config::load_resolved_with_data_dir(...)? before dispatching to hooks::run. Because /etc/aimx/config.toml is 0640 root:root, a non-root caller hit EACCES before any hooks code ran — defeating the daemon-UDS path hooks.rs already implements. aimx mailboxes was already moved out of this path; hooks never got the same treatment even though the daemon-side machinery already existed (the HOOK-LIST verb, the ownership-filtered handler in hook_list_handler.rs, and the client helper in mcp.rs).

The fix wires the existing HOOK-LIST daemon path into the CLI and makes all three hooks subcommands tolerate an unreadable/absent config, mirroring src/mailbox.rs.

Requirements

• aimx hooks list run by a non-root mailbox owner (daemon running, unreadable 0640 root:root config) succeeds and lists only the caller's own hooks — no Permission denied, no sudo.
• aimx hooks create and aimx hooks delete run by a non-root mailbox owner (daemon running) succeed via the daemon UDS without reading the root-owned config — no Permission denied.
• When the daemon is stopped, a non-root caller gets the actionable "daemon must be running…" hint (not a raw EACCES); root retains its direct-config-edit fallback (which requires a readable config).
• Root behavior with a readable config is unchanged: list shows every hook, create/delete use the daemon then fall back to a direct config edit when the daemon is down.
• The local readable-config path still applies the per-caller ownership filter (non-root sees only owned hooks).
• Stale code/doc comments are corrected; new comments contain no planning-doc cross-references.

Out of scope

• The verifier service (services/verifier/).
• Any change to the daemon-side HOOK-LIST handler or the AIMX/1 wire protocol — both already exist and are correct.
• Changing the 0640 root:root config permission model or the SO_PEERCRED authz model.
• MCP tool behavior (already routes through the daemon correctly).

Technical approach

Mirrors src/mailbox.rs.

• src/main.rs — added a dedicated Command::Hooks(cmd) => hooks::run(cmd, cli.data_dir.as_deref()) arm next to Command::Mailboxes, removed the hooks arm from dispatch_with_config, and added Command::Hooks(_) to its unreachable! list. Config is no longer eager-loaded for hooks.
• src/mailbox.rs — load_config_optional is now pub(crate) (returns None on EACCES/ENOENT/parse error). caller_owns was already pub(crate).
• src/mcp.rs — added submit_hook_list_via_daemon_for_cli() -> Result<String, MailboxLifecycleFallback> calling the existing submit_hook_list_raw(), mirroring submit_mailbox_list_via_daemon_for_cli. The MCP tool's String-error variant is untouched.
• src/hooks.rs — run(cmd, data_dir: Option<&Path>) loads config optionally and passes loaded.as_ref() to each subcommand. Module doc rewritten. Extracted render_hook_rows so local and daemon list paths emit identical output. list_dispatch picks the local list (with the caller_owns euid filter) when config is readable, else list_via_daemon (parses the already-filtered+sorted HookListRows, re-applies the optional --mailbox filter, renders). create/delete take Option<&Config>: local authz pre-flight runs only with a readable config (daemon re-authorizes via SO_PEERCRED otherwise); the root-only direct-edit fallback on SocketMissing is guarded on a readable config + root (or the test escape hatch), else the canonical NotRoot + daemon-down hint. delete without a readable config resolves the hook for the confirmation prompt via the daemon's HOOK-LIST rows.
• Docs — rewrote the src/hooks.rs module doc, updated the hooks.rs / hook_list_handler.rs bullets in CLAUDE.md, and corrected the stale "hook creation requires root" claim in the --cmd clap help. book/hooks.md already documented the owner-without-sudo behavior, so it needed no change.

Test plan

• New tests/integration.rs regression tests, mirroring the MCP equivalents (skip when euid==0): cli_hooks_list_with_unreadable_config_does_not_surface_eacces, cli_hooks_create_with_unreadable_config_does_not_surface_eacces, cli_hooks_delete_with_unreadable_config_does_not_surface_eacces. Each starts the daemon, chmod 0000s the config, and asserts no Permission denied against the running daemon as the same uid. A new aimx_cmd_daemon helper wires the CLI to the daemon's runtime dir without the authz escape hatch.
• tests/hooks_list_filter.rs (the 0644 readable-config local-filter path) still compiles unchanged.
• Existing CLI round-trip tests in tests/integration.rs still pass under the new dispatch.

Results: all tests pass — 1151 lib unit tests, 119 integration tests, plus the isolation/uds_authz/release targets (0 failures). cargo clippy --all-targets -- -D warnings clean. cargo fmt -- --check clean. Banned planning-doc token scan on added lines: zero hits.

🤖 Generated with Claude Code

[uzyn]

Standalone Review — PR #254: route aimx hooks CLI through daemon UDS for non-root callers

Changes Overview

This PR fixes aimx hooks list|create|delete failing for a non-root mailbox owner with Permission denied (os error 13). The root cause was that Command::Hooks fell into main.rs's catch-all dispatch arm, which eagerly ran Config::load_resolved_with_data_dir(...)? against the 0640 root:root config before any hooks code ran. The fix moves Command::Hooks to a dedicated dispatch arm (no eager config load), makes hooks::run load config optionally via mailbox::load_config_optional, and routes all three subcommands through the existing daemon UDS verbs (HOOK-LIST / HOOK-CREATE / HOOK-DELETE) when the local config is unreadable — exactly mirroring the established src/mailbox.rs pattern.

Scope Alignment

Fully met. Every stated requirement is delivered:

• Command::Hooks is removed from dispatch_with_config and added to its unreachable! arm; the dedicated arm in dispatch does not pre-load config. Verified: the eager Config::load_resolved_with_data_dir(...)? is now reachable only by the other catch-all, which no longer includes Hooks.
• hooks::run(cmd, data_dir) loads config optionally and threads Option<&Config> into each subcommand.
• list uses the local euid ownership filter when config is readable, else the server-filtered HOOK-LIST rows.
• create / delete run a local authz pre-flight only with a readable config; otherwise rely on the daemon's SO_PEERCRED authz.
• Root-only direct-edit fallback on SocketMissing is gated on Some(cfg) + (is_root() or the test escape hatch).
• Stale doc comments and the --cmd clap help ("hook creation requires root") corrected; CLAUDE.md bullets updated.

No scope creep. The daemon-side HOOK-LIST handler, the wire protocol, and the permission model are untouched (confirmed: git diff of src/hook_list_handler.rs and src/send_protocol.rs is empty for this PR).

The single new helper submit_hook_list_via_daemon_for_cli() is a one-line wrapper over the shared submit_hook_list_raw() — the same body the MCP variant submit_hook_list_via_daemon() delegates to. No code duplication; it correctly mirrors the submit_mailbox_list_via_daemon / _for_cli pair.

Potential Bugs

I worked through each error branch the change introduces and found no bugs:

• create SocketMissing, non-root + readable config: match config { Some(cfg) if is_root() || skip_authz_check => ..., _ => Err(NotRoot + hint) }. For a non-root caller both guards are false, so the arm correctly falls to _ and hard-errors rather than attempting a write it can't perform. Correct.
• create SocketMissing, root + readable config: takes the direct-edit fallback. Correct.
• delete_via_daemon with --yes and daemon down: skips the confirm block, submits HOOK-DELETE, gets SocketMissing, returns the NotRoot + hint error. No false "deleted" message. Correct.
• delete_via_daemon without --yes, daemon up, name unknown/not-owned: HOOK-LIST rows are server-filtered to owned hooks; .find() → None → "Hook not found", opaque as documented. Correct.
• list_via_daemon ordering: relies on the daemon's (mailbox, event, name) sort and does not re-sort after the --mailbox filter — filtering preserves order, so output stays sorted and render_hook_rows yields output identical to the local path. Correct.
• is_none_or (used in list_via_daemon): stabilized in Rust 1.82; crate rust-version = 1.88.0. Fine.

The single hooks::run caller in main.rs is updated to the new signature; no stale callers.

Security Issues

None. The change is conservative on the privilege axis: the local authz pre-flight is preserved verbatim where the config is readable, and where it isn't, the daemon's kernel-validated SO_PEERCRED authz is authoritative (unchanged). A non-root caller can never reach apply_create_direct / apply_delete_direct (both guarded by is_root()), and the local ownership filter on the readable-config list path is retained, so a non-root caller still cannot see hooks on mailboxes it does not own. No secrets, no injection surface (argv is parsed as a JSON array and absolute-path validation is deferred to the shared validate_hooks).

Test Coverage

Solid for the regression being fixed. Three new integration tests (cli_hooks_{list,create,delete}_with_unreadable_config_does_not_surface_eacces) start a real daemon, chmod 0000 the config, and assert no Permission denied against the running daemon as the same uid — directly exercising the non-root UDS path with a genuinely unreadable config. A new aimx_cmd_daemon helper deliberately omits AIMX_TEST_SKIP_AUTHZ_CHECK, so the daemon's real SO_PEERCRED authz is exercised rather than bypassed. I ran the suite as a non-root user (so the tests did not early-skip): all 3 pass, plus the 7 hooks:: unit tests and the hooks_list_filter test (the latter ignored for lack of two host test uids — pre-existing).

Minor coverage gap (non-blocking): there is no test asserting the daemon-down + non-root path emits the canonical "daemon must be running…" hint rather than EACCES for the CLI (the MCP equivalents and the unit-level append_daemon_down_hint test exist, but no end-to-end CLI assertion of that specific message). The logic is straightforward and unit-covered, so this is low priority.

Code Quality

Good. The refactor extracts render_hook_rows, confirm_delete, print_hook_deleted_live, and hooks_ctx, removing the duplicated table-rendering / confirmation / AuthErrorContext-construction blocks that the old create and delete each carried inline. The split of delete into delete_with_config / delete_via_daemon reads cleanly and the doc comments accurately describe each path's contract.

One intentional, documented inconsistency worth noting (non-blocker): hooks list --mailbox <nonexistent> errors with "Mailbox '' does not exist" on the readable-config path but renders "No hooks configured." on the daemon path. The code comments call this out explicitly (the daemon listing is opaque between "no such mailbox" and "not owned"). It's a reasonable trade-off and harmless, but the two paths give different output for the same invocation depending on whether the caller can read the config.

Project-Rule Compliance

• Banned planning-doc tokens (Sprint N, S<n>-<n>, FR-<n>, User Story, Acceptance criteria, PRD §): scan of the PR's added lines returns zero hits. (Note: a pre-existing PRD R34 reference lives in src/hook_list_handler.rs, but that file is untouched by this PR, so it is out of scope here.)
• CLI color routing: scan of added lines for raw .red()/.green()/.yellow()/.blue()/.bold()/... returns zero hits — all output routes through term:: helpers.

Verification Performed

Built and tested the PR branch in an isolated worktree:

• cargo build — clean
• cargo clippy --all-targets -- -D warnings — clean
• cargo fmt -- --check — clean
• cargo test --test integration cli_hooks_ — 3 passed
• cargo test --bin aimx hooks:: — 7 passed
• cargo test --test hooks_list_filter — 1 ignored (host-uid requirement, pre-existing)

Summary and Recommended Actions

Overall verdict: Ready to merge — one optional cleanup.

This is a clean, well-scoped fix that faithfully mirrors the proven mailbox.rs pattern. All error branches are correct, the security posture is unchanged, tests exercise the real non-root UDS path, and both project lint rules pass.

• Blockers: none.
• Non-blockers:
  1. hooks list --mailbox <nonexistent> gives different output (hard error vs. "No hooks configured.") between the readable-config and daemon paths. Documented and harmless; align if you want strict parity.
• Nice-to-haves:
  1. Add a CLI-level integration test asserting the daemon-down + non-root path surfaces the "daemon must be running…" hint (not EACCES), to match the create/delete/list EACCES guards.

fix: route aimx hooks CLI through daemon UDS for non-root callers

aimx hooks list|create|delete failed for non-root mailbox owners with
Permission denied because Command::Hooks eagerly loaded the 0640
root:root config before dispatch. Move it to a dedicated dispatch arm,
load config optionally, and route through the existing HOOK-LIST /
HOOK-CREATE / HOOK-DELETE daemon UDS verbs when the config is
unreadable — mirroring the mailbox.rs pattern. Adds regression tests
covering the non-root unreadable-config path against a live daemon.

[uzyn]

Addressed the two review notes (both non-blockers):

Nice-to-have — added the missing CLI-level test. New cli_hooks_daemon_down_non_root_emits_hint_not_eacces in tests/integration.rs. It reproduces the daemon-down + non-root condition (no UDS socket, chmod 0000 config — the non-root default-install shape) and asserts aimx hooks list | create | delete each:

• exit non-zero,
• never surface a raw Permission denied / EACCES,
• emit the actionable hint the code actually produces — the canonical ... requires root (run with sudo) rendering plus the appended hint: if the daemon is running, hook CRUD over UDS would handle this without sudo. (asserts on the sudo + if the daemon is running substrings, not invented wording).

It guards the SocketMissing branches in hooks.rs (list_via_daemon / create / delete_via_daemon). Guarded to skip when euid == 0 (chmod 0000 is a no-op for root, and root falls back to a direct config edit), exactly like the existing unreadable-config tests. I verified the emitted output by hand to confirm the path is genuinely exercised and that no EACCES leaks — so this was a missing test, not a masked bug.

Full suite green on the branch: cargo build, cargo clippy --all-targets -- -D warnings, and cargo fmt -- --check all clean; cargo test = 1151 lib + 120 integration passing, 0 failures (integration went 119 -> 120 with this test). Banned planning-doc token scan on the added lines: zero hits.

Non-blocker — hooks list --mailbox <nonexistent> parity: intentional, not changing. The readable-config path hard-errors Mailbox '<name>' does not exist; the daemon path renders No hooks configured.. This divergence is deliberate and already documented inline in hooks.rs (the comment at the --mailbox filter in list_via_daemon): the daemon listing is intentionally opaque between "no such mailbox" and "exists but not owned by the caller", matching the MAILBOX-LIST opacity model. Harmonizing the daemon path to emit "does not exist" would leak mailbox-existence/ownership information to a non-owning caller, which is precisely what the opacity model prevents. The inline comment is clear, so the behavior is left as-is by design.

[uzyn]

Re-Review — PR #254: route aimx hooks CLI through daemon UDS for non-root callers

Re-reviewed after the implementer addressed the two non-blocking notes from the first pass (verdict was COMMENTED, 0 blockers). One new commit since: b40db70 (test-only, +82 lines to tests/integration.rs, no production change).

Previously flagged — both resolved

1. (Nice-to-have) Missing CLI-level test for daemon-down + non-root → hint, not EACCES — RESOLVED.

New cli_hooks_daemon_down_non_root_emits_hint_not_eacces reproduces the daemon-down + 0000-config shape (creates run/ but never starts aimx serve, so the UDS connect fails with SocketMissing) and asserts each of hooks list | create | delete --yes:

• exits non-zero,
• never surfaces Permission denied in stdout/stderr,
• emits both sudo and if the daemon is running substrings.

I verified this test is genuinely exercising the SocketMissing branches and is not a tautology:

• The asserted substring if the daemon is running has exactly one producer in the whole crate — append_daemon_down_hint (src/hooks.rs:618). Its only 5 call sites are the SocketMissing branches in list_via_daemon (line 109), create (line 319), delete_with_config (line 416), and delete_via_daemon (lines 436/471). No other error path (clap parse error, EACCES, daemon-side error) can produce that string, so the assertion cannot pass on a wrong-error path.
• The three test invocations reach: list → line 109; create (config None, so the local pre-flight is skipped) → line 319; delete --yes (skips the confirm block) → line 471.
• The eager-config-load EACCES that this PR removes for Command::Hooks would not produce the hint substring either, so a regression there would fail the test.
• Ran as a non-root user (euid 1000) so it did not early-skip: cli_hooks_daemon_down_non_root_emits_hint_not_eacces ... ok. All 4 cli_hooks_* integration tests + 7 hooks:: unit tests pass.

Minor note (not actionable): the test covers the --yes SocketMissing branch in delete_via_daemon (line 471), not the without---yes confirmation-prompt SocketMissing branch (line 436). Both are covered in aggregate across the suite; the test's doc comment and the PR reply accurately describe what it guards. No change needed.

2. (Non-blocker, intentional) hooks list --mailbox <nonexistent> output divergence — confirmed correct to leave as-is.

The readable-config path hard-errors Mailbox '<name>' does not exist; the daemon path renders No hooks configured.. This is a deliberate information-disclosure defense (matching the MAILBOX-LIST opacity model): harmonizing the daemon path to emit "does not exist" would leak mailbox existence/ownership to a non-owning caller. Documented inline at the --mailbox filter in list_via_daemon. Agreeing with the implementer — no harmonization required.

New-issue / regression scan (whole diff)

• Production code is byte-identical to what the first pass approved as ready — the only change since is the test file. git diff of src/hook_list_handler.rs and src/send_protocol.rs is empty (daemon-side HOOK-LIST + wire protocol untouched).
• main.rs dispatch move verified: Command::Hooks is in a dedicated arm with no eager config load, removed from dispatch_with_config's match, and added to its unreachable! list. No stale hooks::run(cmd, config) caller remains.
• cli.rs help text: the stale "Hook creation requires root" claim is replaced with accurate owner-without-sudo wording.
• Project rules (whole diff, added lines):
  • Banned planning-doc tokens (Sprint N, S<n>-<n>, FR-<n>, User Story, Acceptance criteria, PRD §): zero hits in added lines. (Pre-existing hits in tests/integration.rs / tests/uds_authz.rs live on main and are outside this PR's hunks — not introduced here.)
  • CLI color routing (raw .red()/.green()/.bold()/...): zero hits in added lines.
• cargo clippy --all-targets -- -D warnings — clean. cargo fmt -- --check — clean.

Summary and Recommended Actions

Overall verdict: Ready to merge.

Both previously flagged notes are resolved — the new regression test genuinely exercises the daemon-down + non-root SocketMissing branches (verified non-tautological and not auto-skipped under non-root), and the --mailbox opacity divergence is a correct, documented security trade-off. No blockers, no non-blockers, no new issues, no regressions.

• Blockers: none.
• Non-blockers: none.
• Nice-to-haves: none.

fix: route aimx hooks CLI through daemon UDS for non-root callers

aimx hooks list|create|delete failed for non-root mailbox owners with
Permission denied because Command::Hooks eagerly loaded the 0640
root:root config before dispatch. Move it to a dedicated dispatch arm,
load config optionally, and route through the existing HOOK-LIST /
HOOK-CREATE / HOOK-DELETE daemon UDS verbs when the config is
unreadable — mirroring the mailbox.rs pattern. Adds regression tests
covering the non-root unreadable-config and daemon-down paths against
a live daemon.