Sprint Goal Assessment

The sprint goal is fully met. The 9 previously-broken MCP tools are rewritten to drop self.load_config() and route through the daemon's MAILBOX-LIST (and the new HOOK-LIST). The structural removal of AimxMcpServer::load_config from non-test code is in place — grep -n 'fn load_config' src/mcp.rs returns nothing, cargo build --release succeeds, and the symbol cannot be reintroduced by accident. The new HOOK-LIST verb mirrors MAILBOX-LIST line-for-line in framing, dispatch, and codec. Daemon-side hook_delete now collapses the not-owner reject into ENOENT "hook 'X' not found" so the MCP no longer needs a config-read pre-flight to honor NFR2 opacity.

Acceptance Criteria Checklist

[S4-1] HOOK-LIST UDS verb + handler

• Request::HookList added to Request enum (src/send_protocol.rs:211); parser arm at line 440; parse_hook_list_headers rejects any header (forged Owner / Content-Length).
• Client-side request builder write_hook_list_request (line 1233) mirrors write_mailbox_list_request.
• HookListRow defined exactly once in src/hook_list_handler.rs:35 (single source of truth).
• Handler returns Vec<HookListRow> via JsonAckResponse::Ok, filtered to caller-owned mailboxes; root sees every hook.
• Daemon dispatch in src/serve.rs:884.
• 5 unit tests cover root visibility, non-root owner, stranger (empty array), hooks on unowned mailboxes filtered, serde round-trip.
• cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt -- --check all clean (verified locally).

[S4-2] Email-read tools rewrite

• email_list, email_read, email_mark_read, email_mark_unread all replace self.load_config() with lookup_mailbox_row (which calls submit_mailbox_list_via_daemon).
• Row lookup by name; missing row → opaque "Mailbox '' does not exist." (NFR2-compatible — daemon's MAILBOX-LIST is already SO_PEERCRED-filtered).
• No caching; every tool invocation makes its own UDS call.
• Existing tests now spawn aimx serve to provide a daemon to talk to.

[S4-3] email_send / email_reply

• Both derive from_address from row.address (no config.domain read).
• row.address == None surfaces "Mailbox '' is not registered." — no unwrap.
• Catchall guard preserved: from_address.starts_with('*') rejects.
• No self.load_config() call in either tool.

[S4-4] Hook tools rewrite

• hook_create: lookup_mailbox_row pre-flight (opaque on missing row); daemon's HOOK-CREATE re-runs authorize().
• hook_list: routes through HOOK-LIST UDS verb; optional mailbox filter is post-filter only.
• hook_delete: thin pass-through to HOOK-DELETE; inline comment documents NFR2 + daemon-side authz.
• No self.load_config() call in any of the three.
• Daemon-side non_owner_cannot_delete_hook test (src/hook_handler.rs:805) verifies the new opacity collapse — wire reason is [ENOENT] hook '...' not found, never not authorized.

[S4-5] Per-tool production-perm regression tests

• 9 new tests at tests/integration.rs:3743-4139 (mcp_email_list_*, mcp_email_read_*, mcp_email_mark_read_*, mcp_email_mark_unread_*, mcp_email_send_*, mcp_email_reply_*, mcp_hook_create_*, mcp_hook_list_*, mcp_hook_delete_*).
• Each test chmod 0000s the on-disk config, spawns the daemon, asserts the MCP tool does not surface "Permission denied".
• No AIMX_INTEGRATION_SUDO=1 dependency — default-lane.

[S4-6] Structural removal of AimxMcpServer::load_config

• Method removed from non-test code; release build compiles clean.
• grep -n 'fn load_config' src/mcp.rs returns nothing.
• Test-only authorize_mailbox helper (#[cfg(test)]-gated, line 199) takes &Config directly — tests construct config inline rather than reading it.

Sprint 4 exit gate — all green: 1034 unit + 106 integration tests pass, clippy clean, fmt clean, release build clean.

Test Coverage

Coverage is solid:

• 5 new unit tests in hook_list_handler cover root, owner, stranger, filtering, and serde round-trip.
• 2 new codec tests pin HOOK-LIST round-trip and the parser's rejection of forged headers (Owner, Content-Length).
• 1 new daemon-side test pins the hook_delete ENOENT-collapse contract for non-owners.
• 9 new MCP-tool integration tests pin the EACCES-non-recurrence contract on 0000 config.
• 11 existing MCP integration tests now spawn aimx serve so the daemon-routed tools have someone to talk to.

One minor observation: S4-4's AC says "the daemon-trust shift in hook_delete is verified by an integration test that calls it against a hook the caller doesn't own." The verification lives at the daemon layer (non_owner_cannot_delete_hook in src/hook_handler.rs:805) rather than at the MCP-client layer. The contract is verified end-to-end via the wire-shape assertions; an MCP-level integration test on this exact path would add confidence but is not strictly required by the AC.

Potential Bugs

No blocker-level bugs found. The handler dispatch, lock hierarchy preservation, NFR1 (privilege escalation via SO_PEERCRED only), and NFR2 (opacity collapse) all hold.

Security Issues

None. Verified:

• HOOK-LIST parser rejects any client-supplied header — no path to forge an owner identity past the SO_PEERCRED filter (src/send_protocol.rs:803-817; pinned by hook_list_rejects_owner_header and hook_list_rejects_content_length_header codec tests).
• HOOK-LIST handler filters by caller_uid via mailbox::caller_owns, with root short-circuit (src/hook_list_handler.rs:104-109).
• hook_delete's daemon-side opacity collapse preserves NFR2: the wire reason for an unowned hook is now [ENOENT] hook '<X>' not found, indistinguishable from a nonexistent hook (src/hook_handler.rs:244-256; pinned by non_owner_cannot_delete_hook test asserting !reason.contains("not authorized")).
• enforce_mailbox_owner_or_root still logs the rejection for operator forensics before collapsing the wire shape — the comment at src/hook_handler.rs:250-251 is accurate; logging happens inside enforce_mailbox_owner_or_root (src/uds_authz.rs:346-353).
• The MCP hook_list post-filter calls lookup_mailbox_row(name)? to surface "not found" rather than silently returning [] for an unowned/missing mailbox — an extra UDS round-trip but no information leak (the HOOK-LIST listing is already filtered).

Code Quality

One non-blocker issue:

Stale doc comment + dead field (src/mcp.rs:18-35). The struct field data_dir_override is annotated #[allow(dead_code)] and its doc comment still says "Read by the test-only load_config helper" — but load_config was removed in this PR. Searching for actual readers:

$ grep -n 'self\.data_dir_override\|data_dir_override:' src/mcp.rs
24:    data_dir_override: Option<PathBuf>,
170:    pub fn new(data_dir_override: Option<PathBuf>) -> Self {
178:    pub fn with_caller_uid_for_test(data_dir_override: Option<PathBuf>, caller_uid: u32) -> Self {
182:    fn with_caller_uid(data_dir_override: Option<PathBuf>, caller_uid: u32) -> Self {
184:            data_dir_override,

The field is stored but never read (the test-only authorize_mailbox helper consumes only caller_uid; production tools delegate to the daemon). Two options: drop the field and the parameter from the public new constructor, or fix the doc comment to say "retained for compatibility with the public new signature; never read after load_config was removed." Either is fine; the doc-comment route is the safest squash.

Alignment with PRD

All P0 requirements (R29–R34, R43, R45) addressed in this PR:

• R29 (MAILBOX-LIST rewrite for the 4 email-read tools) — done.
• R30 (email_send/email_reply derive domain from row.address, no unwrap on None) — done.
• R31 (hook_create MAILBOX-LIST pre-flight, opaque on missing) — done.
• R32 (hook_list via new HOOK-LIST verb) — done.
• R33 (hook_delete thin pass-through; daemon-side opacity collapse) — done.
• R34 (HOOK-LIST schema with typed HookEvent) — done; serializes as snake_case.
• R43 (per-tool production-perm regression tests) — 9 new tests, one per tool.
• R45 (structural removal of load_config) — done; release build is the structural guard.

Phase-2 P1 items in scope for Sprint 5 (env var rename, format_auth_error consolidation finish) and Sprint 6 (doc drift, HOOK-LIST in book/cli.md, release-notes entry, final soak) are correctly deferred and tracked.

Summary and Recommended Actions

Overall verdict: Ready to merge.

Blockers: none.

Non-blockers:

• Fix stale doc comment on AimxMcpServer::data_dir_override (src/mcp.rs:20-22) which still references the now-removed load_config helper. Option A: drop the now-unused field + the new parameter. Option B: retitle the comment to reflect its current state ("retained for new signature compatibility; never read after load_config removal"). Either is fine; the doc-only fix can roll into Sprint 5 alongside the env-var rename.

Nice-to-haves:

• Consider adding an MCP-level integration test that calls hook_delete against a hook the caller doesn't own and asserts the wire-level "not found" shape (S4-4 verification is currently at the daemon layer only). The existing daemon-side non_owner_cannot_delete_hook test already pins the contract; this would just shift the verification one layer up.

Recommended merge commit message:

HOOK-LIST verb + MCP tool rewrites — close MCP EACCES bug class

- Add HOOK-LIST UDS verb (mirrors MAILBOX-LIST) with daemon-side handler
  and codec tests; HookListRow uses the typed HookEvent enum so client
  and server share one source of truth.
- Rewrite the 9 broken MCP tools (email_list/read/mark_read/mark_unread,
  email_send/reply, hook_create/list/delete) to drop self.load_config()
  and route through MAILBOX-LIST / HOOK-LIST. A non-root agent on a
  production-perm install (0640 root:root config) no longer fails with
  EACCES on tool entry.
- Structurally delete AimxMcpServer::load_config from non-test code.
  Release builds compile with no load_config symbol in src/mcp.rs, so
  a future tool cannot reintroduce the bug class by accident.
- Tighten daemon-side HOOK-DELETE so the not-owner reject collapses
  to ENOENT "hook 'X' not found" — preserves the NFR2 opacity contract
  without a config-read pre-flight on the MCP side.
- 9 new per-tool production-perm regression tests (chmod 0000 the config,
  daemon serves from Arc<Config>, every tool succeeds without EACCES).
- 11 existing MCP integration tests now spawn aimx serve so the
  tools have a daemon to talk to.

Re-review — Sprint 4 (commit 9e79448)

The non-blocker flagged in the previous review is resolved cleanly via the reviewer's recommended Option A.

Resolved

• Stale doc comment + dead data_dir_override field on AimxMcpServer (src/mcp.rs:18-35): the field is dropped entirely, and the parameter is removed from new(), with_caller_uid_for_test(), and the private with_caller_uid(). src/main.rs Command::Mcp no longer threads cli.data_dir into mcp::run() (signature is now pub async fn run() -> Result<...>). The doc-comment debt is gone with the field. Verified zero leftover data_dir_override references in src/mcp.rs; the only remaining hits in the tree are unrelated (config.rs parameter, agents_setup.rs test name string).

Verification

• Diff is 2 files, +15/-20 lines — surgical, exactly the scope advertised.
• All 5 callers of AimxMcpServer::with_caller_uid_for_test updated to the new single-arg form; one call site of AimxMcpServer::new() (in mcp::run) and one call site of mcp::run() (in main.rs) consistently updated.
• cargo test --bin aimx: 1034 passed, 0 failed, 8 ignored (matches claim).
• cargo test --test integration: 106 passed, 0 failed, 5 ignored (sudo-lane skips cleanly, matches claim).
• cargo clippy --all-targets -- -D warnings: clean. No clippy::new_without_default lint despite the new arity-0 new() (rmcp's tool macros likely keep clippy quiet here, or the field-set is small enough that clippy doesn't flag it; either way, zero warnings).
• cargo fmt -- --check: clean.
• Planning-doc cross-reference scan on the diff (the project's banned-tokens list): zero new hits in commit 9e79448. The pre-existing S2-1 / S3-5 / S2-3 / S2-4 comments in src/main.rs and src/mcp.rs are inherited from earlier sprints and outside this PR's scope.

Still unresolved

None.

New issues found

None.

Implementer's intentional skips (reviewed and accepted)

• The MCP-level hook_delete integration test against an unowned hook was a nice-to-have, not a non-blocker, in the previous review. The implementer's reasoning (the contract is already pinned end-to-end at the daemon layer via non_owner_cannot_delete_hook plus codec round-trip tests, and a copy at the MCP layer would shift the same assertion one layer up without strengthening it) is sound.
• The stale Each tool method loads config and operates on the filesystem directly. line in CLAUDE.md is doc drift outside this PR's scope and is correctly tracked for the Sprint 6 doc-drift sweep.

Summary

Overall verdict: Ready to merge.

Blockers: none.
Non-blockers: none.
Nice-to-haves: unchanged from the prior review (MCP-layer hook_delete opacity test).

Recommended merge commit message

HOOK-LIST verb + MCP tool rewrites — close MCP EACCES bug class

- Add HOOK-LIST UDS verb (mirrors MAILBOX-LIST) with daemon-side handler
  and codec tests; HookListRow uses the typed HookEvent enum so client
  and server share one source of truth.
- Rewrite the 9 broken MCP tools (email_list/read/mark_read/mark_unread,
  email_send/reply, hook_create/list/delete) to drop self.load_config()
  and route through MAILBOX-LIST / HOOK-LIST. A non-root agent on a
  production-perm install (0640 root:root config) no longer fails with
  EACCES on tool entry.
- Structurally delete AimxMcpServer::load_config from non-test code.
  Release builds compile with no load_config symbol in src/mcp.rs, so
  a future tool cannot reintroduce the bug class by accident.
- Drop the now-dead data_dir_override field from AimxMcpServer and the
  threaded --data-dir parameter through mcp::run().
- Tighten daemon-side HOOK-DELETE so the not-owner reject collapses to
  ENOENT "hook 'X' not found" — preserves the NFR2 opacity contract
  without a config-read pre-flight on the MCP side.
- 9 new per-tool production-perm regression tests (chmod 0000 the config,
  daemon serves from Arc<Config>, every tool succeeds without EACCES).
- 11 existing MCP integration tests now spawn aimx serve so the tools
  have a daemon to talk to.

Re-review — Sprint 4 (commit 37f0f3c)

The CI failure on the sudo-lane mailbox-dir-perms-isolation job is resolved cleanly. The fix scope matches the bug.

Resolved

• tests/uds_authz.rs::hook_delete_as_other_forbidden aligned with the new HOOK-DELETE opacity contract (tests/uds_authz.rs:723-730). The previous assertion assert_response_contains(&del, "EACCES") now reads:

  assert_response_contains(&del, "ENOENT");
  assert_response_contains(&del, "hook 'alice_owned_hook' not found");
  assert_response_does_not_contain(&del, "not authorized");
  assert_response_does_not_contain(&del, "EACCES");
  

  This mirrors the daemon-side non_owner_cannot_delete_hook test (src/hook_handler.rs:828-838) plus an extra wire-level guard that no EACCES substring leaks anywhere in the response frame. The handler at src/hook_handler.rs:252-255 literally returns code: ErrCode::Enoent, reason: format!("hook '{}' not found", req.name), and ErrCode::Enoent serializes as the string "ENOENT" (src/send_protocol.rs:283), so each substring assertion lines up exactly with the wire output.

Verification

• Diff is surgical: 1 file, +8/-1 lines (one assertion + four targeted guards), exactly the scope the implementer described.
• Audit of remaining EACCES assertions in tests/uds_authz.rs — only MARK-READ, MARK-UNREAD, and HOOK-CREATE cases. Verified at the source level that those handlers still surface reject.code directly (src/state_handler.rs:148-152, src/hook_handler.rs:74-81), so no other tests need updating. The implementer's audit claim ("only HOOK-DELETE had its opacity tightened") is accurate.
• cargo test: 1034 passed / 0 failed (unit), 106 passed / 0 failed (integration); 21 sudo-lane tests in tests/uds_authz.rs skip cleanly when AIMX_INTEGRATION_SUDO is unset.
• cargo clippy --all-targets -- -D warnings: clean.
• cargo fmt -- --check: clean.
• CI sudo-lane status — green: mailbox-dir-perms-isolation now passes (2m19s) on commit 37f0f3c. All 8 CI jobs are green: core-tests, mailbox-dir-perms-isolation, verifier-tests, docs-build, Analyze (rust|javascript-typescript|actions), CodeQL. The previously-failing sudo-lane CI is the authoritative signal we couldn't get locally (no aimx-test-alice / aimx-test-bob uids on the dev host), and it has now confirmed the fix.
• Planning-doc cross-reference scan on the diff: zero new hits.

Still unresolved

None.

New issues found

None. The fix is a pure test-assertion update — no production code changed, no new code paths added, no risk of regression.

Note on the opacity contract

The new test assertions are exactly what the contract requires:

1. ENOENT is asserted on the wire (the canonical opaque code).
2. hook 'alice_owned_hook' not found is asserted to confirm the canonical reason format.
3. not authorized and EACCES are explicitly asserted absent — the wire-level guards that prevent a future regression where the daemon accidentally re-leaks authz state through the response frame.

The pair of positive + negative assertions is stronger than the daemon-unit test alone, because it also pins the transport layer — even if a future refactor changed the AckResponse::Err serialization path, this test would catch any leak on the wire.

Summary

Overall verdict: Ready to merge.

Blockers: none.
Non-blockers: none.
Nice-to-haves: unchanged from the prior review (MCP-layer hook_delete opacity test — explicitly accepted as a nice-to-have, not a non-blocker).

Recommended merge commit message

HOOK-LIST verb + MCP tool rewrites — close MCP EACCES bug class

- Add HOOK-LIST UDS verb (mirrors MAILBOX-LIST) with daemon-side handler
  and codec tests; HookListRow uses the typed HookEvent enum so client
  and server share one source of truth.
- Rewrite the 9 broken MCP tools (email_list/read/mark_read/mark_unread,
  email_send/reply, hook_create/list/delete) to drop self.load_config()
  and route through MAILBOX-LIST / HOOK-LIST. A non-root agent on a
  production-perm install (0640 root:root config) no longer fails with
  EACCES on tool entry.
- Structurally delete AimxMcpServer::load_config from non-test code.
  Release builds compile with no load_config symbol in src/mcp.rs, so
  a future tool cannot reintroduce the bug class by accident.
- Drop the now-dead data_dir_override field from AimxMcpServer and the
  threaded --data-dir parameter through mcp::run().
- Tighten daemon-side HOOK-DELETE so the not-owner reject collapses to
  ENOENT "hook 'X' not found" — preserves the NFR2 opacity contract
  without a config-read pre-flight on the MCP side. Sudo-lane
  integration test (hook_delete_as_other_forbidden) updated to match.
- 9 new per-tool production-perm regression tests (chmod 0000 the config,
  daemon serves from Arc<Config>, every tool succeeds without EACCES).
- 11 existing MCP integration tests now spawn aimx serve so the tools
  have a daemon to talk to.