feat: provision a mailbox straight from the request (close the loop)

[marcelxpfeifer]

What & why

Part of the next-layer UX plan — workstream A: an admin can set up everything in-app, locked principle #1 NO CLI CLIFFS ("every setup action is reachable from the dashboard") and #5 HONESTY ("never mark a step done that isn't real").

Today a mailbox request could only be acknowledged — the admin still had to go provision the mailbox elsewhere and then click "Mark done", a step that lied about what happened. This closes the loop: an admin provisions the hosted mailbox straight from the request row, and it resolves as FULFILLED, not merely acknowledged.

• New admin-only mutation mail.mailboxRequest.provisionFromRequest stands the hosted mailbox up through the existing shared provisioning path (createProvisionedMailbox / provisionMailbox) — admin scope is never bypassed.
• Reservation/claim machinery honoured: if a hosted mailbox was already reserved for the requester (via an invitation), we provision that reserved address and consume the reservation rather than orphaning it.
• Requester notified in-app: their onboarding flips to mailboxReady and their fresh-start guard (freshStartStatus) now reports hasMailbox, so PostboxMailboxGuard admits them to the inbox live.
• Idempotent / redelivery-safe: a second call returns the same mailbox; a request whose requester already has a live mailbox is fulfilled against it instead of standing up a second one.
• UI: each row gets a primary Provision now and keeps Mark done (the plain acknowledge/decline for external/other cases). Provision is disabled with a one-line reason ("Verify a sending domain first") when hosted mail isn't configured (no verified sending domain).

Acceptance criteria

• "Provision now" on each request row provisions the hosted mailbox inline via the existing authed provisioning path (no admin-scope bypass, no shortcut around reservation/claim).
• Request resolves as FULFILLED (distinct fulfilled status + fulfilledMailboxId), not merely acknowledged.
• Requester notified in-app (onboarding mailboxReady; fresh-start guard admits them).
• Plain acknowledge/decline retained via "Mark done".
• Action disabled with a one-line reason when hosted mail isn't configured.
• Idempotent (double-click / redelivery safe).
• Admin-gated; org-scoped (cross-org request rejected).
• FF tokens only, dark+light, focus-visible via UiButton; human copy.

Preserved behavior

• request / freshStartStatus / listPending / resolve unchanged in behavior; resolve remains the acknowledge/decline path.
• Schema status union widened open | resolved → open | fulfilled | resolved; sweep/erasure/tenant-table consumers only delete rows (no exhaustive switch), so no break.
• Existing provisioning path (createProvisionedMailbox / provisionMailbox) reused unchanged; no new mailbox insert path.

Tests (vitest / convex-test, CI-gated)

apps/api/convex/__tests__/mailboxRequest.integration.test.ts:

• provisions a fresh hosted mailbox, fulfils the request (audit-stamped), notifies the requester (onboarding + freshStartStatus.hasMailbox).
• idempotent: redelivered provision returns the same mailbox, no second one.
• fulfils against an existing live mailbox instead of standing up a second.
• honours a reservation: provisions the reserved address and consumes it.
• admin-gated (member fails closed); cross-org request rejected.

Auto-merge pipeline: squash-merges on reviewer approval + green CI.

[marcelxpfeifer]

Review — round 1

Verdict: REQUEST_CHANGES

CI is green and the admin-gate/org-scope/idempotency work is solid (secure-by-default wrapper, requireAdminContext, org check, convex-test coverage incl. fail-closed member + cross-org). Two blocking defects remain in what actually gets provisioned, plus improvements.

Blocking

• apps/api/convex/mail/mailboxRequest.ts:281 — no-reservation provisioning uses the requester's LOGIN email as the hosted address, at whatever domain that is. In the fresh-start flow the requester's profile email is normally an external address (they were invited at it), so "Provision now" stands up a kind: 'hosted' mailbox at e.g. jordan@gmail.com: it is pushed to the MTA cache (pushMailboxToCache), so the local MTA claims routing for a domain this deployment does not host (an internal send to that address gets swallowed locally instead of reaching the real inbox); inbound mail can never arrive (MX doesn't point here), yet the request is marked FULFILLED and the requester is admitted to a dead inbox — exactly the "marked done that isn't real" the brief's principle Bump @octokit/rest from 21.1.1 to 22.0.1 #5 forbids. The existing add-mailbox flow (apps/web/app/pages/dashboard/postbox/settings/add-account.vue:176) only ever builds localpart@<verified domain>. Your own test proves the hole: it provisions member-a@example.com with no domains seeded at all — the "Verify a sending domain first" gate exists only in the client; the server neither enforces a verified domain nor uses one. Fix: in the no-reservation branch provision at an address on a verified sending domain (localpart derived from the requester email + a verified domain, mirroring the reservation shape — or let the admin choose, as add-account does), and enforce the verified-domain floor server-side in provisionFromRequest (the UI disable is an affordance, not the fence). Update the tests to seed a verified domain and assert the refusal without one.

• apps/api/convex/mail/mailboxRequest.ts:237 — the "already has a live mailbox" short-circuit mis-fulfils move-raised requests. mailboxRequests rows are also created by mailboxMove.start (apps/api/convex/mail/mailboxMove.ts:183) for non-admin movers, whose mailbox stays live throughout — so getActiveMailboxForUser matches their ACTIVE EXTERNAL mailbox, and "Provision now" marks the request FULFILLED against the very mailbox they're moving away from, provisions nothing, strands the move at stage provisioning, and removes the ask from the admin list (mailboxMove.provisionHosted's own resolve is then skipped by its status === 'open' guard). Fix: the short-circuit must only fulfil against a hosted mailbox (kind !== 'external'), and a move-linked request (mailboxMoves.provisionRequestId) needs deliberate handling — delegate to mailboxMove.provisionHosted (which correctly bypasses the address dup-check and advances the move) or refuse with a pointer to the move flow; note that with only the kind filter the fresh branch would throwAlreadyExists on the mover's address, an unactionable dead-end for the admin. Add a convex-test covering a move-raised request.

Improvements

• apps/api/convex/mail/mailboxRequest.ts:309-327 — resolve has no status guard: a stale "Mark done" click racing another admin's "Provision now" (subscription hasn't removed the row yet) downgrades fulfilled → resolved, erasing the fulfilment distinction this PR introduces. mailboxMove.provisionHosted (mailboxMove.ts:268) already guards status === 'open' before patching — do the same here (idempotent no-op on non-open rows).
• apps/api/convex/mail/mailboxRequest.ts:261-278 — Duplicated Code: the reserved branch re-implements pendingMailbox.claimForInvitation's claim shape (by_address collision guard → provisionMailbox → delete reservation → markOnboardingStep; apps/api/convex/mail/pendingMailbox.ts:169-190). Extract one shared claim helper in pendingMailbox.ts that both call so collision/consume semantics cannot drift.
• apps/api/convex/mail/mailboxRequest.ts:239-247 vs :288-300 — Duplicated Code inside the handler: the fulfil stamp (patch {status, fulfilledMailboxId, resolvedByUserId, resolvedAt} + markOnboardingStep + return) appears twice. Extract a local fulfil(mailboxId) helper.
• apps/api/convex/mail/mailboxRequest.ts:253-259 — the reservation lookup takes .first() on by_invitee_email and only then org-checks: a foreign-org reservation sitting first in the index silently shadows a same-org one and drops the admin into fresh provisioning. Constrain the query to the caller's org (filter on organizationId) so the guard can't pick the wrong row.
• apps/web/app/components/dashboard/MailboxRequests.vue:115 — while "Provision now" spins on a row, that row's "Mark done" renders enabled but its click is silently swallowed by the busy guard. Disable the sibling action on the busy row too: busy !== null && !(busy.id === req.id && busy.action === 'resolve') (and mirror for the provision button).
• apps/web/app/components/dashboard/MailboxRequests.vue:34,107 — hostedConfigured treats "query not yet loaded" (undefined) as "not configured", so the disabled button + "Verify a sending domain first" flash on every mount even when domains exist. Only show the disabled state/reason once verifiedDomains.value !== undefined.

[marcelxpfeifer]

Author response — round 2

All findings addressed.

Blocking

• No-reservation branch provisioned at the external login address (dead inbox / dishonest fulfilment). The fresh branch now stands up localpart@<verified sending domain> (localpart derived from the requester email, sanitised to the mailbox charset), mirroring the reservation / add-mailbox shape — never the requester's external address. The verified-domain floor is now enforced server-side in provisionFromRequest (domains.by_status = 'verified'), so the UI disable is just an affordance. Tests updated to seed a verified domain and assert the address lands on it; added a test asserting refusal when no domain is verified (no mailbox stood up, row stays open).
• "Already has a live mailbox" short-circuit mis-fulfilled move-raised requests. Two changes: (1) the short-circuit now matches hosted mailboxes only (kind !== 'external'), so a mover's live external mailbox is never mistaken for a fulfilled hosted one; (2) a move-linked request (looked up via mailboxMoves.by_user + provisionRequestId) is refused with a pointer to the move flow rather than provisioned here — provisioning would strand the move at provisioning. Chose refusal over delegating into mailboxMove.provisionHosted to avoid duplicating the move state machine (stage transition + audit log); the move card is the correct place to advance it. Added a convex-test covering a move-raised request (request stays open, move untouched, external mailbox intact).

Improvements

• resolve status guard — now a no-op on non-open rows, so a stale "Mark done" can't downgrade fulfilled → resolved. Test added.
• Shared claim helper — extracted claimReservedMailbox() in pendingMailbox.ts; both claimForInvitation and provisionFromRequest call it, so the collision-guard → provision → delete-reservation → mark-ready sequence can't drift.
• Duplicated fulfil stamp — extracted a local fulfil(mailboxId) helper; the patch + markOnboardingStep + return now lives in one place.
• Reservation lookup org-scoped — the by_invitee_email query now filters organizationId, so a foreign-org reservation first in the index can't shadow a same-org one.
• Vue busy sibling — the sibling action on a busy row is now disabled too (keyed on id and action), no more silently-swallowed clicks.
• Vue loading flash — the disabled state + 'Verify a sending domain first' only render once verifiedDomains has loaded (domainsLoaded), so they no longer flash on mount when domains exist.

[marcelxpfeifer]

Review — round 2

Verdict: APPROVE

Re-reviewed against round 1. Both blocking findings and all six improvements are addressed, no new defects introduced by the fix commits, and GitHub CI is fully green (Lint & Typecheck, Test apps/api + apps/web, SAST, secret scan all pass).

Blocking

none

• Dead-inbox / dishonest fulfilment (mailboxRequest.ts no-reservation branch) — resolved. The fresh branch now stands up localpart@<verified sending domain> (localpart derived + sanitised via localpartFromEmail), never the requester's external login address, and the verified-domain floor is enforced server-side (domains.by_status = 'verified'), so the client disable is only an affordance. Tests seed a verified domain, assert the address lands on it, and assert refusal (row stays open, no mailbox) when none is verified.
• Move-raised requests mis-fulfilled against the mover's live external mailbox — resolved. The already-live short-circuit now matches hosted mailboxes only (kind !== 'external'), and a move-linked request (mailboxMoves.by_user + provisionRequestId) is refused with a pointer to the move flow before any provisioning. Covered by a new convex-test (request stays open, move untouched at provisioning, external mailbox intact).

Improvements

none

• resolve status guard (no-op on non-open) — done, tested.
• Shared claimReservedMailbox() in pendingMailbox.ts, called by both claimForInvitation and provisionFromRequest; the extraction is faithful (claimForInvitation still clears the stale reservation on collision) — done.
• Local fulfil(mailboxId) helper collapses the duplicated fulfil stamp — done.
• Reservation lookup org-scoped (.filter(organizationId)) so a foreign-org row can't shadow — done.
• Vue busy-sibling disable keyed on id AND action — done.
• Vue loading-flash gated on domainsLoaded — done.

Spec delivered: admin-gated (requireAdminContext), org-scoped, idempotent, reservation/claim machinery honoured through the shared provisioning path (no scope bypass), requester notified in-app (onboarding + freshStartStatus), FULFILLED distinct from resolved, plain acknowledge/decline retained, disabled-with-reason when hosted mail isn't configured. FF tokens only, both themes, focus-visible via UiButton, human copy. No new v-html/secret/external-asset/process.env surface.

[marcelxpfeifer]

Review — round 1 (independent re-verification; PR was already merged after the prior round-2 approve)

Verdict: APPROVE

Full independent pass over the merged head (b080665): security, spec, brief conformance, code smells, best practices, tests. CI 32/32 green.

Blocking

none

• Admin gate (requireAdminContext) + org scope verified; no authedIdentityMutation, no env reads, no v-html, no secrets returned (domains.listVerified exposes only _id/domain/verifiedAt).
• Provisioning goes through the shared createProvisionedMailbox/provisionMailbox path — canonicalization, by_address dup-check, folder/member provisioning, and the hosted-only MTA cache push all intact; no reservation/claim shortcut (claimReservedMailbox extraction is faithful, incl. claimForInvitation's stale-reservation delete on collision).
• Verified-domain floor enforced server-side; fresh branch builds localpart@<verified domain>, never the requester's external login address.
• Move-raised requests refused with a pointer to the move flow; confirmed mailboxMove.provisionHosted's own status === 'open' resolve guard still fires afterwards.
• Widened status union is safe for every consumer (memberErasure, org-deletion walker, tenantTables only delete rows; listPending reads by_org_and_status = 'open').
• Tests cover all acceptance criteria (fulfil+notify, idempotent, existing-hosted, reservation, no-verified-domain refusal, move-raised refusal, resolve no-downgrade, admin gate fails closed, cross-org). FF tokens only, human copy, atomic conventional commits.

Improvements

• apps/api/convex/mail/mailboxRequest.ts (provisionFromRequest, after the fulfilled short-circuit) — no guard for row.status === 'resolved': a stale "Provision now" racing another admin's "Mark done" (subscription hasn't removed the row yet) provisions anyway, flips the decided row to fulfilled, and overwrites the first admin's audit stamp (resolvedByUserId/resolvedAt). This is the mirror image of the race this PR guarded in resolve(). Fix (one line, follow-up-sized since the PR is merged): if (row.status !== 'open') throwInvalidState('This request was already handled'); placed after the fulfilled idempotency return.