Skip to content

fix(self-hosted): unblock address ownership; stop reporting owned addresses as unowned#56

Merged
andrei-hasna merged 1 commit into
mainfrom
audit/address-ownership-selfhosted
Jul 25, 2026
Merged

fix(self-hosted): unblock address ownership; stop reporting owned addresses as unowned#56
andrei-hasna merged 1 commit into
mainfrom
audit/address-ownership-selfhosted

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Problem

The whole address-ownership capability was blocked in self_hosted by hand-written guards whose stated reason is factually false, even though a complete /v1-backed implementation already ships in the same tree.

CLIsrc/cli/commands/address.ts:137-181: address owner, set-owner, transfer-owner, unassign-owner and owner-history all threw unconditionally via serverOnly() (:29), and the comment at :24-28 asserted these "have no /v1 equivalent in this self-hosted-only client".

That is untrue:

  • src/db/owners.remote.ts implements listAddressOwnershipEvents (:230), assignAddressOwner (:245, PATCH /v1/addresses owner_id+administrator_id then POST /v1/address-ownership-events), transferAddressOwner (:269), unassignAddressOwner (:292) and getAddressOwnership (:311).
  • src/db/owners.ts routes to it whenever isSelfHostedMode().
  • src/server/self-hosted/resources.ts:451 serves the address-ownership-events resource.
  • The repo's own suite already proves both halves work remotely — parity.test.ts:530, :594, :615.
  • The refusal was self-contradictory: emails owner list / emails owner addresses <owner> (src/cli/commands/owner.ts) carry no guard and DO work in self_hosted through the same routed module.

MCP — the same false claim was duplicated: src/mcp/tools/domains-impl.ts:89-95 assertMcpLocalStateAllowed blocked get_address_owner (:408), set_address_owner (:431), transfer_address_owner (:456), unassign_address_owner (:479) and list_address_owner_history (:501) with the reason "it reads/writes local address ownership rows" — those rows are not local in self_hosted mode.

Worse than the refusalssrc/cli/commands/address.ts:52-58 hardcoded owner: null / administrator: null into every emails address list / emails addresses row, and :74 printed - in the Owner column. An owned address was reported as unowned in both the table and --json, with no error, while lib/address-ownership.ts:enrichAddress resolves owner and administrator through the same mode-routed modules and works fine.

Fix

  • Replace the five CLI guards with the existing lib/address-ownership.ts calls (getAddressOwnershipDetail, setAddressOwnerByRef, transferAddressOwnerByRef, unassignAddressOwnerByRef, getAddressOwnershipHistoryByRef). They are mode-routed, so both local and self_hosted work, and the output shape matches what the commands returned before the guards were introduced — scripts reading --json do not silently break.
  • Drop the ownership guards from the five MCP tools. They now return real data, or a real error (Address not found, A human-owned address requires an agent administrator), instead of a mode refusal.
  • Wire enrichAddresses into the list projection: owner, administrator and provider_name are real, and the Owner column shows owner[:admin].
  • --verbose now does what its help text promises (expanded owner/admin/quota) instead of being accepted and ignored. The quota field shows the configured limit only — never a client-side "used" count, because the daily send ledger is server-owned and countSendsTodayByAddress returns zeros in self_hosted.
  • Keep address provision server-only — the provisioning orchestration genuinely has no client-side equivalent — and rewrite the comment so it claims only that.
  • Delete the duplicate suggestAddressLocalParts copy in address.ts in favour of the lib/address-ownership.ts one.

Every user-facing string touched here was verified true afterwards. No misleading message replaces another.

Tests

New /v1-stub coverage (real curl transport, out-of-process stub — no module mocks):

  • src/cli/commands/address.test.ts — all five ownership subcommands end to end (assign persists owner_id/administrator_id on the /v1 address; transfer writes an audit event with its reason; unassign clears both columns; history is newest-first and honest when empty; a human owner without an agent administrator is refused), plus the list projection in table, --verbose and --json.
  • src/cli/commands/address.self-hosted.test.tsaddress owner answers from /v1; addresses surfaces the recorded owner. Only address provision is still asserted to block.
  • src/mcp/address-ownership-self-hosted.test.ts (new) — the five MCP tools against the stub, including "reports a real not-found error rather than a mode refusal".
  • src/mcp/domain-address-self-hosted.test.ts — the five ownership tools removed from the local-only blocked list.

Red before / green after (verified by stashing only the two source files):

  • src/mcp/address-ownership-self-hosted.test.ts0 pass, 6 fail before, 6 pass after.
  • address list reports real ownership2 fail before (received owner: null, and --verbose fell through to the compact table), pass after.
  • addresses surfaces the owner recorded on the /v1 record1 fail before, pass after.

Full suite: bun test2057 pass, 85 skip, 0 fail (2142 tests, 205 files). bunx tsc --noEmit → clean.

Notes for the reviewer

Adjacent findings left out of this PR to keep it to one cluster — same false-reason class, different surfaces:

  • suggest_address, remove_address, suspend_address, activate_address, set_address_quota are still blocked in MCP as "local rows", but their CLI equivalents (address suggest/remove/suspend/activate/quota) all work in self_hosted over /v1.
  • src/cli/commands/address.ts address add comment still says "the /v1 API exposes no /v1/providers", but resources.ts:102 serves providers and src/db/providers.remote.ts reads it.
  • countSendsToday/countSendsTodayByAddress return a hardcoded 0 in self_hosted; any surface that renders "used/limit" from them would be lying. That is why this PR shows the limit only.

Rebased on origin/main @ 1ac2d15. Not merging myself.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…addresses as unowned

`emails address owner`, `set-owner`, `transfer-owner`, `unassign-owner` and
`owner-history` threw unconditionally via serverOnly(), and the five matching
MCP tools were blocked by assertMcpLocalStateAllowed(). Both stated reasons were
false: ownership does have a /v1 implementation and those rows are not local in
self_hosted mode. src/db/owners.remote.ts already serves getAddressOwnership,
assignAddressOwner, transferAddressOwner, unassignAddressOwner and
listAddressOwnershipEvents from /v1/owners, owner_id/administrator_id on
/v1/addresses/<id>, and /v1/address-ownership-events; src/db/owners.ts routes to
it whenever isSelfHostedMode(); the server serves the address-ownership-events
resource; and `emails owner list` / `emails owner addresses` already used that
same path with no guard.

Worse than the refusals, `emails address list` / `emails addresses` hardcoded
owner: null / administrator: null into every row and printed "-" in the Owner
column, so an owned address was silently reported as unowned in the table AND in
--json, with no error.

- replace the five CLI guards with the existing lib/address-ownership.ts calls
  (mode-routed, so both local and self_hosted work), keeping the pre-guard output
  shape so scripts reading --json do not break
- drop the ownership guards from the five MCP tools; they now return real data or
  a real error (e.g. "Address not found") instead of a mode refusal
- hydrate the list projection through enrichAddresses so owner/administrator/
  provider_name are real; the Owner column shows owner[:admin]
- make --verbose do what its help text says (expanded owner/admin/quota) instead
  of being accepted and ignored; quota shows the configured LIMIT only, never a
  fabricated client-side "used" count, because the send ledger is server-owned
- keep `address provision` server-only (the provisioning orchestration genuinely
  has no client-side equivalent) and rewrite the comment to say only that
- delete the duplicate suggestAddressLocalParts copy in favour of the lib one

Tests: new /v1-stub coverage for all five CLI subcommands and all five MCP tools
plus the list projection; the old tests that asserted the refusals are replaced.
All six MCP ownership tests and the three list-ownership tests fail on the
pre-fix source and pass after. Full suite 2057 pass / 0 fail, tsc --noEmit clean.
@andrei-hasna
andrei-hasna merged commit 459f6bc into main Jul 25, 2026
1 of 3 checks passed
andrei-hasna added a commit that referenced this pull request Jul 25, 2026
Only CHANGELOG.md conflicted. Resolved by keeping BOTH sides:

- every send-path entry from main (#55, #54, #56) is preserved verbatim and
  kept first — part one wins on the send path;
- this branch's rename-revert and dead-scaffolding entries follow;
- the "BREAKING (aliased): rename @hasna/emails -> @hasna/mailery" entry is
  dropped rather than carried alongside its own revert. That rename never
  shipped — it exists only under [Unreleased] — so recording it and undoing it
  in the same unreleased section would describe a change no consumer can
  observe.

Verified on the merge result: tsc --noEmit clean, bun run build exit 0,
bun test 2101 pass / 0 fail / 108 skip, and the six Postgres suites against a
real PG 16 at 86 pass / 0 fail.
andrei-hasna added a commit that referenced this pull request Jul 25, 2026
Only CHANGELOG.md conflicted. Kept both sides: main's entries (part one #55/#54,
#56, and #57's rename revert) first, then this branch's dead-code removals. The
never-shipped 'BREAKING (aliased): rename' entry is dropped, as it was on #57 —
it is undone in the same unreleased section, so no consumer can observe it.

Verified on the merge result: tsc --noEmit clean, bun run build exit 0,
bun test 1994 pass / 0 fail / 108 skip, six Postgres suites 86 pass / 0 fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant