Add x402 Agent Mail service and autonomous domain activation - #86
Conversation
|
Failed to generate code suggestions for PR |
|
AI review audit completed: this PR has no actionable review threads. The current branch and review state were checked, so no review-driven code changes were required. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
hyrule-cloud/hyrule_cloud/domains/service.py
Lines 1231 to 1235 in 44d390e
Reject customer changes to service-owned RRsets
After a combined domain-and-mailbox buyer claims the domain into an account, the normal DNS changeset API loads agent_mail records into desired and allows the customer to delete or overwrite them exactly like customer-owned records. This defeats the newly added managed_by ownership boundary and can remove MX, DKIM, or other records required by an active paid mailbox. Refuse changes when the existing row is not customer-managed.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| password = self._decrypt(self._fernet(), account.backend_credential_ciphertext) | ||
| try: | ||
| message_id = await self.backend.send_message( |
There was a problem hiding this comment.
If the process stops or the database commit fails after backend.send_message() accepts the message, no MailSendRow or consumed-quote state is persisted. Retrying the same paid quote therefore calls the backend again and sends a duplicate message; the in-memory authorization guard cannot cover crashes or other API processes. Persist a recoverable send intent before the external submission and reconcile its outcome idempotently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 68623ab. The send path now commits a unique pending MailSendRow and reserves the quote before the first JMAP write. Each submission carries a stable X-Hyrule-Send-ID; request retries and the worker query that header and atomically finalize the existing intent. A crash-after-acceptance regression proves reconciliation completes with exactly one backend submission.
| if account is None or account.status == MailboxStatus.AWAITING_PAYMENT.value: | ||
| row.status = MailQuoteStatus.EXPIRED.value |
There was a problem hiding this comment.
When an activation payment settles near or after the reserved quote deadline but the API-to-database handoff fails, the durable payment event can coexist with an awaiting_payment account. This expiration path treats every such account as unpaid, marks it failed, and removes its recoverable capability; recover_x402_handoffs() then skips it because it is no longer awaiting payment. Check the settled ledger before expiring the account, or leave it recoverable so a paid customer is not charged without activation or a refund.
Useful? React with 👍 / 👎.
| if row is None or row.status in { | ||
| MailboxStatus.REFUND_DUE.value, | ||
| MailboxStatus.FAILED.value, | ||
| MailboxStatus.DELETED.value, | ||
| }: |
There was a problem hiding this comment.
On a paid provisioning failure, _fail_activation() commits refund_due before calling the best-effort refund ledger writer. A process interruption or ledger write failure in that gap permanently loses the refund obligation because subsequent calls return immediately for refund_due accounts and provision_pending() never selects them. Add the refund event to the same transaction as the terminal status, or retain a retryable state until the obligation is durably recorded.
Useful? React with 👍 / 👎.
| if backend_id: | ||
| query = query.where(MailAccountRow.backend_id == backend_id) | ||
| elif addresses: | ||
| query = query.where(MailAccountRow.address.in_(addresses)) | ||
| else: | ||
| continue | ||
| account = await session.scalar(query) |
There was a problem hiding this comment.
When Stalwart omits accountId, this query matches every managed address appearing anywhere in the event and arbitrarily selects one row. For mail exchanged between two Hyrule mailboxes, both sender and recipient match, so an inbound message, complaint, or delivery event can be indexed, webhook-delivered, or used to suspend the wrong customer's mailbox. Select the mailbox according to the event direction/type, or reject ambiguous address-only events.
Useful? React with 👍 / 👎.
| active_count = await session.scalar( | ||
| select(func.count()) | ||
| .select_from(MailAccountRow) | ||
| .where( | ||
| MailAccountRow.status.in_( | ||
| [ | ||
| MailboxStatus.AWAITING_PAYMENT.value, | ||
| MailboxStatus.PENDING_DOMAIN.value, | ||
| MailboxStatus.PROVISIONING.value, | ||
| MailboxStatus.ACTIVE.value, | ||
| MailboxStatus.SUSPENDED.value, | ||
| MailboxStatus.GRACE.value, | ||
| ] | ||
| ) | ||
| ) | ||
| ) | ||
| if int(active_count or 0) >= self.mail_config.max_active_mailboxes: |
There was a problem hiding this comment.
The global mailbox cap is checked only while issuing an unpaid quote. A caller can obtain many quotes while the count is below the cap and then reserve and pay all of them, because prepare_activation() never repeats this check transactionally; even sequential activation of pre-created quotes can exceed max_active_mailboxes. Enforce the capacity invariant when inserting the awaiting_payment account rather than only at quote creation.
Useful? React with 👍 / 👎.
| verified = await gate.verify_only( | ||
| request, | ||
| amount=service.config.payment.price_mail_send, | ||
| description="Send one Agent Mail message", |
There was a problem hiding this comment.
If price_mail_send changes during a deployment while an existing send quote remains valid, payment verification uses the new configuration value rather than the immutable amount returned by that quote. A client paying the advertised quote can therefore be rejected, or be asked to authorize a different amount. Convert quote.amount_usd back to Decimal and use it for verification and the charged-amount response.
Useful? React with 👍 / 👎.
| order = await service.mark_x402_paid( | ||
| order.order_id, | ||
| payer=paid, | ||
| tx_hash=getattr(request.state, "payment_tx", None), | ||
| payment_network=getattr(request.state, "payment_network", None), | ||
| payment_asset=getattr(request.state, "payment_asset", None), | ||
| ) |
There was a problem hiding this comment.
On real x402 EVM payments, request.state.payment_asset is the token contract address (for example, the 42-character USDC address), but DomainOrderRow.payment_asset remains VARCHAR(16). This newly enabled agent checkout passes that address into mark_x402_paid(), so PostgreSQL rejects the handoff after payment has already settled; ledger recovery repeats the same oversized write, leaving standalone domain and combined domain-plus-mail purchases stuck. Migrate the domain-order column to the same address-sized width used for mail accounts.
Useful? React with 👍 / 👎.
| if lower == "store.ingest" or lower in { | ||
| "message-ingest.ham", | ||
| "message-ingest.spam", | ||
| "message-ingest.imap-append", | ||
| "message-ingest.jmap-append", | ||
| }: | ||
| return "message.received" |
There was a problem hiding this comment.
A message-ingest.jmap-append event can be generated by this service's own Email/set draft creation during outbound submission, but this mapping labels every JMAP or IMAP append as message.received. That can place an outbound draft in the inbox index and emit a false received-message webhook; if it races the send transaction, both paths also try to insert the same message ID. Only SMTP/inbound delivery events should become message.received, or the event data must be checked for inbound direction and mailbox role.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 68623ab. Only store.ingest and ham/spam inbound events map to message.received now. IMAP/JMAP append events map to mail.system, require backend account ownership, and never create an inbox index. The regression verifies a JMAP append produces a system event without an inbox message and rejects an address-only ambiguous append.
|
Addressed the review-body DNS ownership finding in 68623ab: customer DNS changesets now return service_dns_record_managed for any existing non-customer RRset, preventing deletion or overwrite of Agent Mail MX/DKIM/service records. The regression exercises both DELETE and UPSERT and verifies the service-owned row and authoritative zone remain unchanged. Full validation: 508 pytest tests, Ruff, and mypy (102 source files) all pass. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68623abf88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if row.plan != MailboxMode.HOSTED.value: | ||
| await self.domains.replace_service_records( | ||
| str(row.domain), records, managed_by="agent_mail" | ||
| ) |
There was a problem hiding this comment.
When Stalwart returns no Domain/get item, an empty dnsZoneFile, or a temporarily incomplete generated zone, ensure_domain() returns an empty or partial record list. This call accepts that list—an empty list even removes existing Agent Mail RRsets—and the mailbox is subsequently marked active, so paid custom and domain_and_mailbox activations can succeed without MX/DKIM and be unable to receive or authenticate mail. Validate the required mail RRsets and leave provisioning retryable before applying the zone or activating the mailbox.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b379f9b. Stalwart DNS zones are now rejected unless apex MX and SPF plus DKIM TXT are present. Incomplete zones raise a retryable backend error before DNS replacement or mailbox activation; provisioning leaves the paid mailbox queued without a refund. Regressions cover valid and incomplete generated zones plus retry-to-success.
| MailAccountRow.address == address, | ||
| MailAccountRow.status != MailboxStatus.DELETED.value, |
There was a problem hiding this comment.
After lifecycle cleanup leaves a deleted mailbox tombstone, this query deliberately treats its address as available and issues a new quote, but mail_accounts.address remains globally unique. The later activation insert therefore raises an integrity conflict, so a customer can never reactivate the same hosted or custom address after deletion. Recycle the deleted row or change uniqueness so only non-deleted addresses conflict.
Useful? React with 👍 / 👎.
| if in_reply_to: | ||
| email["inReplyTo"] = [in_reply_to] |
There was a problem hiding this comment.
For replies, the supplied value is validated against MailMessageIndexRow.message_id, which is the JMAP Email object id later passed to Email/get, but JMAP's inReplyTo property represents RFC Message-ID header values rather than Email object ids. Forwarding the object id here can produce an invalid or unthreaded In-Reply-To header, breaking the advertised conversational reply behavior; load the original email's messageId value and submit that instead.
Useful? React with 👍 / 👎.
| secret = "hyr_whsec_" + secrets.token_urlsafe(32) | ||
| now = _now() | ||
| row = MailWebhookRow( | ||
| webhook_id=generate_mail_id("wh"), |
There was a problem hiding this comment.
A caller can obtain the mailbox token from the initial unpaid 402, and _authorized_account permits awaiting_payment accounts here; every subsequent POST then inserts another unique webhook with no count limit, uniqueness rule, idempotency requirement, or mail-route rate limiter. One unpaid reservation can therefore create unbounded persistent rows and later delivery work. Require an active mailbox and enforce a small per-mailbox webhook cap or equivalent bounded policy before insertion.
Useful? React with 👍 / 👎.
| # The paid send performs a non-idempotent external action before deferred x402 | ||
| # settlement. Serialize reuse of the same authorization in this process; a | ||
| # retry of the same quote remains idempotent at the database layer. | ||
| _mail_inflight_authorizations: set[str] = set() |
There was a problem hiding this comment.
This set only serializes a payment header inside one Python process, while the service is designed to run multiple API workers. An attacker can submit the same valid authorization concurrently to different workers with different one-cent send quotes; both workers can verify and deliver before either settles, after which at most one authorization is actually consumed. Store the authorization fingerprint in a database-backed reservation with a uniqueness constraint so verification-to-settlement reuse is excluded across workers.
Useful? React with 👍 / 👎.
| row.status = MailboxStatus.GRACE.value | ||
| row.grace_ends_at = now + timedelta(days=self.mail_config.grace_days) |
There was a problem hiding this comment.
Provisioning already sets grace_ends_at relative to the contractual mailbox expiry, but this transition overwrites it relative to whenever the worker happens to notice expiration. After a worker outage or paused deployment, mailbox reads, inbound delivery, and retained data therefore remain available for an additional full grace period beyond the promised seven days. Preserve the existing deadline or derive it from expires_at, and immediately purge mailboxes whose fixed grace deadline has already elapsed.
Useful? React with 👍 / 👎.
| rows = ( | ||
| await session.execute( | ||
| select(MailSendRow.status, func.count()).group_by(MailSendRow.status) | ||
| ) |
There was a problem hiding this comment.
hyrule_mail_messages_total is declared as a Prometheus counter but is computed from current MailSendRow counts, while lifecycle cleanup deletes every send row when a mailbox is purged. The series therefore decreases or disappears after mailbox expiry, causing counter resets and incorrect rates/alerts. Retain a monotonic aggregate or payment/event ledger for this counter, or expose the current row count as a gauge instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b379f9b. The nonmonotonic retained-row series is now hyrule_mail_messages_current and is declared as a Prometheus gauge. The metrics regression inserts an accepted send, asserts the gauge output, and confirms the old counter name is absent.
| if session.get_bind().dialect.name == "postgresql": | ||
| await session.execute(select(func.pg_advisory_xact_lock(_MAIL_CAPACITY_LOCK_ID))) | ||
| active_count = int( | ||
| await session.scalar( | ||
| select(func.count()) | ||
| .select_from(MailAccountRow) | ||
| .where(MailAccountRow.status.in_(_MAIL_CAPACITY_STATUSES)) |
There was a problem hiding this comment.
The one-mailbox-per-custom-domain check runs only while creating a quote. Two quotes for different local parts can be created before either account exists, and this reservation path rechecks only global capacity, so both quotes can later create and activate mailboxes on the same custom domain. When either mailbox fails or expires, its cleanup removes every agent_mail RRset for that domain and breaks the surviving mailbox. Recheck the domain invariant under the activation lock or enforce an appropriate database uniqueness rule for live custom mailboxes.
Useful? React with 👍 / 👎.
| async with self.db() as session: | ||
| rows = list( | ||
| await session.scalars( | ||
| select(MailMessageIndexRow) | ||
| .where(MailMessageIndexRow.mailbox_id == mailbox_id) | ||
| .order_by(MailMessageIndexRow.created_at.desc()) | ||
| .limit(min(max(limit, 1), 100)) | ||
| ) |
There was a problem hiding this comment.
This endpoint reads only the webhook-populated local index and never queries the mailbox backend. If a Stalwart ingest event is delayed or lost, the message remains stored in JMAP but is absent from listings; get_message() also rejects it when the index row is missing, so a paid mailbox silently hides legitimate inbound mail indefinitely. List from JMAP directly or run a durable reconciliation that inserts and removes index rows from authoritative mailbox state.
Useful? React with 👍 / 👎.
| idempotency_key=f"mail:{idempotency_key}", | ||
| additional_amount_usd=self.config.payment.price_mail_activation, | ||
| management_token=token, |
There was a problem hiding this comment.
domain_and_mailbox deliberately passes the hyr_identity_… mailbox token into the anonymous domain order, making it the domain's management capability. The only route that converts an anonymous purchased domain into an account-owned, fully manageable domain validates LegacyDomainClaimRequest.token against ^hyr_dom_, so this returned combined token is rejected before it can claim the domain; the buyer consequently cannot reach account-authenticated DNS, renewal, or transfer operations. Accept the combined token prefix for claims or return a separate compatible domain capability.
Useful? React with 👍 / 👎.
|
Addressed all 12 findings from the review of 68623ab in b379f9b. This pass adds retryable DNS completeness validation, deleted-address reactivation, RFC reply translation, active webhook limits, durable cross-worker payment authorization binding, paid-only capacity reservations, combined-payment recovery and refunds, fixed grace deadlines, gauge semantics, custom-domain exclusivity, authoritative JMAP reconciliation, and combined identity domain claims. Validation: 522 pytest tests, Ruff, mypy across 102 source files, Alembic head 018, and git diff checks all pass. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b379f9be05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| extra = event.extra if isinstance(event.extra, dict) else {} | ||
| mailbox_id = str(extra.get("mailbox_id") or "") | ||
| if not mailbox_id or mailbox_id in seen: | ||
| continue |
There was a problem hiding this comment.
When a deleted address is reactivated, prepare_activation overwrites the existing account row but retains its mailbox_id; this recovery scan then accepts any historical settled event carrying that ID and checks only whether the current row awaits payment. Consequently, the previous activation's payment can immediately mark the new quote paid without a new charge, and it may even apply an obsolete amount to a domain bundle. Persist the current activation quote or generation in payment metadata and require it to match before recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. Activation settlement metadata now carries the quote ID, capacity reservation and payment handoff fence on that ID, and recovery requires the current mailbox row to match both the quote ID and settled amount. A regression test covers deletion/reactivation so a historical payment cannot activate the reused mailbox ID.
| current = await session.get(MailAccountRow, mailbox_id) | ||
| if current is None or current.status != MailboxStatus.PROVISIONING.value: | ||
| await self.backend.delete_account(backend_id) |
There was a problem hiding this comment.
When two worker processes select the same PROVISIONING mailbox, both can call ensure_account; after the first worker activates the row, the second receives the same existing Stalwart account ID, observes that the database row is no longer provisioning, and deletes that now-active backend account here. Claim or lease the provisioning row before external writes, and do not delete an account merely because another worker completed the same operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. Provisioning now acquires a durable atomic lease/token before any Stalwart write, persists the credential with that lease, and fences every later database transition on the token. A stale worker never deletes an account when another lease wins; concurrent and expired-lease regressions cover both paths.
| MailSendRow.status.in_(("pending", "submitting")), | ||
| ) | ||
| ) | ||
| ) | ||
| new_count += len(pending_recipients) |
There was a problem hiding this comment.
When an established recipient has a pending/submitting send, this set includes that address even though it already exists in mail_recipients, and its size is added to the daily new-recipient count. It can also double-count an address first accepted today that has another pending send. Near the configured limit, legitimate sends to a new recipient are therefore rejected early; count the union of genuinely new addresses after excluding existing recipient rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. The limit now counts the union of recipients first accepted today and pending recipients that are absent from the known-recipient table. Existing and duplicate pending addresses are excluded, with a regression at the configured limit.
| quote_id=str(payload["domain_quote_id"]), | ||
| terms_version=str(payload["domain_terms_version"]), | ||
| idempotency_key=f"mail:{idempotency_key}", | ||
| additional_amount_usd=self.config.payment.price_mail_activation, |
There was a problem hiding this comment.
If price_mail_activation changes while an activation quote remains valid across a deployment, the x402 gate charges the quote's original total but the combined domain order is constructed with the new configured activation price here. The domain ledger can therefore record a different amount as paid, and a later provisioning failure may refund more or less than the customer was charged. Snapshot the activation component in the mail quote and use that value for the domain order, account row, and refund.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. The activation component is snapshotted in the mail quote and reused for the combined domain order, mailbox ledger row, quote response, and refund obligation. The regression changes live configuration after quoting and verifies the original 1.00/13.00 components remain authoritative.
| if ( | ||
| quote.status != MailQuoteStatus.ACTIVE.value | ||
| or quote_expires_at is None | ||
| or quote_expires_at <= _now() | ||
| ): |
There was a problem hiding this comment.
If the configured mail terms change after a quote is issued but before checkout, this validation accepts the still-unexpired quote without comparing quote.terms_version to the current terms, and the resulting account is later stamped with the new terms version. Hosted and custom activations can therefore be charged under terms the caller never accepted; reject the quote when its terms no longer match and record the quote's accepted version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. Both first consumption and an unpaid idempotent replay now reject a quote whose accepted terms differ from the current terms, and the account records the quote's terms version. Tests cover terms changing before either path.
| "hyrule_mail_activation_outcomes_total", | ||
| "Durable Agent Mail activation outcomes inferred from retained mailbox rows.", | ||
| "counter", |
There was a problem hiding this comment.
This metric is computed from mutable current rows rather than append-only events, so its values can decrease—for example, deleted-address reactivation clears activated_at and replaces a failed status. Declaring it as a Prometheus counter with a _total suffix produces invalid counter resets and misleading rates; expose the retained-row snapshot as a gauge or derive a true counter from immutable outcome events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. The retained-row snapshot is now exported as the hyrule_mail_activation_outcomes gauge; the _total counter name was removed. The metrics contract test asserts the gauge type and absence of the old counter.
| session.add( | ||
| MailMessageIndexRow( | ||
| message_id=message_id, | ||
| mailbox_id=account.mailbox_id, | ||
| folder="inbox", |
There was a problem hiding this comment.
When polling has already inserted this message into mail_message_index, or Stalwart emits another receive event for the same message, this unconditional insert violates the message primary key. The broad IntegrityError handler then rolls back the event and every webhook-delivery row in the transaction, so a delayed receive notification is silently lost. Upsert or reuse the existing index row while still committing the distinct event and its deliveries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. Receive ingestion now uses dialect-specific ON CONFLICT DO NOTHING for the message index while committing the distinct event and webhook deliveries. A delayed-event regression starts with an existing polled index row and verifies the event and delivery survive unchanged.
| except MailDNSIncompleteError: | ||
| log.warning( | ||
| "mailbox_dns_not_ready", | ||
| mailbox_id=row.mailbox_id, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
If Stalwart persistently returns an incomplete zone—for example because DKIM generation is misconfigured—every worker pass takes this branch and leaves the paid mailbox in provisioning forever. No error is exposed on the account and no refund is recorded, so the customer has neither a usable mailbox nor a terminal recovery path. Track retry state and eventually fail/refund the activation, or at least persist a visible actionable error with a bounded operator deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. DNS-incomplete attempts now persist the retry count, visible error, and next-attempt timestamp. Retries are bounded by configuration (15 attempts by default); exhaustion transitions to the durable failure/refund path. Tests cover deferral, later success, and terminal refund at the bound.
| if row.backend_id: | ||
| try: | ||
| await self.backend.delete_account(row.backend_id) | ||
| backend_deleted = True | ||
| except MailBackendError: |
There was a problem hiding this comment.
If deleting the Stalwart account fails transiently here, the mailbox has already been moved to failed or refund_due, and no worker path selects either status for another cleanup attempt. Its backend ID and encrypted credential therefore remain indefinitely; because read authorization does not reject refund_due, the capability holder can continue reading a hosted mailbox after refund while retention sweeps also skip it. Persist cleanup-pending state and retry deletion before clearing the credentials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 938323b. Failed/refund-due mailboxes now return 410 for capability reads, terminal rows retaining a backend ID are retried by the worker, and credentials are cleared only after deletion succeeds. A transient-delete regression verifies access closes immediately and cleanup completes on retry.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 938323b59b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| order, _domain_token, _created = await self.domains.create_agent_order( | ||
| quote_id=str(payload["domain_quote_id"]), | ||
| terms_version=str(payload["domain_terms_version"]), | ||
| idempotency_key=f"mail:{idempotency_key}", | ||
| additional_amount_usd=activation_amount, | ||
| management_token=token, |
There was a problem hiding this comment.
When a domain-and-mailbox activation retries after the domain order was committed but before the mailbox row was inserted, create_agent_order returns the original capability in _domain_token, but this code ignores it and stores the newly generated token in the mailbox instead. The paid domain ultimately hashes the original token, while every subsequent mail activation retry returns the new token, so the advertised hyr_identity_… capability cannot manage or claim the purchased domain. Use the returned domain token as the mailbox token on replay.
Useful? React with 👍 / 👎.
| order, _domain_token, _created = await self.domains.create_agent_order( | ||
| quote_id=str(payload["domain_quote_id"]), | ||
| terms_version=str(payload["domain_terms_version"]), | ||
| idempotency_key=f"mail:{idempotency_key}", |
There was a problem hiding this comment.
For domain_and_mailbox, activation accepts an Idempotency-Key of up to 128 characters, but prefixing it with mail: produces 129–133 characters for otherwise valid 124–128-character keys. DomainService.create_agent_order then rejects the derived key because its maximum is also 128, so only bundled activations fail with idempotency_key_required. Derive a fixed-size hash or account for the prefix in the accepted activation limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 19364ae. Bundled activations now derive the domain idempotency key as mail:<sha256-of-activation-key>, a deterministic 69-character value, while the mailbox continues to bind the original key's digest. A regression uses the full supported 128-character activation key and verifies the derived domain key remains within its 128-character contract.
| account.status = MailboxStatus.FAILED.value | ||
| account.provision_error = "payment_window_expired" | ||
| account.capacity_reserved_at = None |
There was a problem hiding this comment.
When a caller obtains an activation challenge but never pays, this permanently changes the account to failed with payment_window_expired. Nothing later deletes or recycles a failed row without a backend account, while create_account_quote rejects every non-deleted row for the same address. An unauthenticated caller can therefore reserve arbitrary hosted local parts for free and leave them unavailable forever, without consuming the active-mailbox capacity limit. After the handoff grace and settlement-ledger check, close or recycle the unpaid account so the address becomes available again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 19364ae. After the one-hour handoff grace and the settled/dev-bypass ledger check, a still-unpaid activation now becomes a closed tombstone: its capability ciphertext and reservation/lease state are cleared, while the old idempotency attempt remains closed. deleted rows no longer block address quoting, and the regression proves the same address can be quoted and activated again after expiry.
| async def delete_account(self, account_id: str) -> None: | ||
| await self._manage([["x:Account/set", {"destroy": [account_id]}, "delete-account"]]) |
There was a problem hiding this comment.
x:Account/set can return an HTTP-successful method response with the requested account in notDestroyed, but delete_account ignores the method data and always reports success. Lifecycle and failed-activation cleanup then clear the backend ID and encrypted credential, preventing any retry while the account and its messages remain in Stalwart beyond the promised deletion deadline. Inspect notDestroyed here, as delete_message already does, and raise for failures other than notFound before local state is discarded.
Useful? React with 👍 / 👎.
| result = await service.deliver_send(body.quote_id, _token(request)) | ||
| if not await gate.settle_verified(request, verified, extra_body=payment_metadata): |
There was a problem hiding this comment.
If a successful send response is lost and the client retries the same quote with a newly signed payment authorization, get_quote still returns the consumed quote, deliver_send idempotently returns the existing accepted send, and this line settles the new authorization anyway. The authorization table only prevents one fingerprint from being used for different quotes; it does not prevent multiple distinct authorizations from paying the same quote. Check the send's recorded settlement or ledger state before verifying/settling another authorization so a retry cannot charge twice for one message.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19364aeaa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| global_count = int( | ||
| await session.scalar( | ||
| select(func.count()) | ||
| .select_from(MailSendRow) | ||
| .where( | ||
| MailSendRow.created_at >= day_start, | ||
| MailSendRow.status.in_(_SEND_RESERVED_STATUSES), | ||
| ) | ||
| ) | ||
| or 0 |
There was a problem hiding this comment.
When concurrent paid sends target different mailboxes near global_send_limit_per_day, each transaction locks only its own mailbox/quote, so they can all read the same global_count below the limit and then insert reservations that exceed the launch-wide safety cap. Protect the global count-and-reserve operation with a shared PostgreSQL advisory lock or another database-level serialization mechanism.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9552e9a. Every new send reservation now takes a transaction-scoped PostgreSQL advisory lock before the global daily count and intent insert. After waiting, it still locks and rechecks the quote, so concurrent sends for different mailboxes cannot cross the launch-wide cap. The regression asserts that serialization occurs before global_count is read.
| def _mail_payment_authorization_fingerprint(request: Request) -> str | None: | ||
| supplied = request.headers.get("payment-signature") or request.headers.get("x-payment") | ||
| if not supplied: | ||
| return None | ||
| return hashlib.sha256(supplied.encode()).hexdigest() |
There was a problem hiding this comment.
When the same signed x402 payload is serialized with different JSON key ordering, whitespace, or another accepted base64 representation, hashing the raw header produces distinct fingerprints even though the facilitator verifies the same underlying authorization. An attacker can therefore bind those variants to multiple send quotes; the first settlement consumes the authorization, but the later messages have already been submitted before their settlements fail. Derive the binding key from the decoded authorization's immutable signed fields instead of its transport encoding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9552e9a. The route now decodes the x402 header and fingerprints normalized immutable EIP-3009 authorization fields plus network and asset, ignoring JSON ordering/whitespace, base64 transport differences, payload extras, and signature variants for the same authorization. A distinct nonce produces a distinct fingerprint. The durable binding is shared by activation and send quotes.
| and config.domain.dns_control_url | ||
| and config.domain.dns_control_secret | ||
| and config.domain.agent_order_fernet_key | ||
| and provider.username |
There was a problem hiding this comment.
When all agent-domain launch flags and provider settings are present but agent_order_fernet_key is nonempty and malformed, this gate advertises the operation in OpenAPI, the x402 manifest, and the request-challenge middleware, while require_agent_purchase_launch() rejects every checkout with agent_token_storage_not_ready. Validate the key here using the same Fernet construction as the service so an unusable paid route remains absent from discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9552e9a. Agent-domain discovery now constructs Fernet with the configured key and handles the same invalid-key exceptions as the domain service. A malformed but nonempty key therefore removes the route from OpenAPI/x402 discovery. The regression covers malformed and generated-valid keys.
| if row.plan != MailboxMode.HOSTED.value and row.domain: | ||
| try: | ||
| await self.domains.remove_service_records(row.domain, managed_by="agent_mail") | ||
| except Exception: | ||
| log.exception( | ||
| "mailbox_failed_activation_dns_cleanup_failed", | ||
| mailbox_id=row.mailbox_id, | ||
| ) |
There was a problem hiding this comment.
When custom-domain provisioning has already installed Agent Mail RRsets and then fails, a transient failure in remove_service_records() is only logged here. Unlike normal mailbox deletion, this path records no dns_cleanup_pending marker and no worker routine revisits the cleanup, so a refunded/failed mailbox can leave MX, SPF, and DKIM records pointing at a backend account that has been deleted. Persist the cleanup obligation and retry it until the service-owned records are removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9552e9a. Migration 021 adds an indexed, durable dns_cleanup_pending flag. Failed activation cleanup sets it without overwriting the provisioning/refund reason; the lifecycle worker revisits failed, refund-due, and deleted rows and clears the flag only after service-owned DNS records are removed successfully. The regression forces a transient DNS failure and verifies the next worker pass completes cleanup.
| if not await gate.settle_verified( | ||
| request, verified, extra_body=payment_metadata | ||
| ): | ||
| await service.release_activation_capacity(account.mailbox_id) |
There was a problem hiding this comment.
When settlement succeeds but the payment-ledger write times out or fails, settle_verified() still returns true because _record() deliberately swallows ledger errors; if mark_activation_paid() then also fails or the API process exits, the worker has no settlement event to recover and the paid mailbox remains awaiting_payment until it is tombstoned. The activation flow needs a durable recovery record that is guaranteed after settlement, rather than treating the best-effort metrics ledger as proof of payment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9552e9a. Activation now commits a pre-settlement intent before calling the facilitator, so expiry cannot tombstone an in-flight or unknown settlement. After success it durably records the settlement timestamp/transaction with bounded retry before provisioning handoff, and the worker replays that mailbox-local marker independently of the best-effort metrics ledger. A no-ledger regression forces the post-settlement activation handoff to fail and verifies worker recovery.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9552e9a01f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paid = await gate.check_payment( | ||
| request, | ||
| amount=order.amount_usd, | ||
| description=f"Hyrule wallet-native domain order for {order.fqdn}", | ||
| extra_body=challenge_metadata, |
There was a problem hiding this comment.
When the facilitator settles successfully but the database becomes unavailable before mark_x402_paid commits, this flow can charge the customer without ever queueing the domain. PaymentGate._record is explicitly best-effort and drops ledger failures, while recover_x402_handoffs relies exclusively on those PaymentEventRow records, so the 503 described as “pending durable recovery” may have no durable settlement marker to recover from. Persist a settlement intent on the order before calling the facilitator, as the mail activation flow does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. Both domain checkout routes now verify without settling, commit an order-local settlement intent (payer/network/asset/timestamp), and only then call the facilitator. A successful settlement is durably stamped in paid_at/payment_tx with bounded retry before fulfillment handoff; the worker replays that marker independently of PaymentEventRow, while expiry preserves pending/settled handoffs. Migration 022 and a no-ledger recovery regression cover the path.
| text: str = Field(default="", max_length=100_000) | ||
| html: str | None = Field(default=None, max_length=100_000) |
There was a problem hiding this comment.
When MAIL_MAX_TEXT_CHARS or MAIL_MAX_HTML_CHARS is configured above 100,000—which MailConfig permits up to 1,000,000—Pydantic rejects the request here before create_send_quote can apply the configured limits. Consequently, increasing either documented configuration value has no effect; these request bounds should be derived from configuration or should allow the full configured range before the service-level check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. MailSendQuoteRequest now accepts the full configured maximum of 1,000,000 characters for text and HTML, while MailService.create_send_quote remains the authority for the operator's actual MAIL_MAX_TEXT_CHARS and MAIL_MAX_HTML_CHARS values. The regression proves a 100,001-character body succeeds at a 150,000 limit and is rejected at 100,000.
| if body.mode is MailboxMode.CUSTOM: | ||
| await self._assert_managed_domain_token(domain, body.domain_management_token or "") |
There was a problem hiding this comment.
When Agent Mail is launch-ready but the domain DNS control URL or secret is unset, a customer who already has a valid managed-domain token can still obtain and pay this custom-domain activation quote. Neither this check nor prepare_activation verifies domains.dns.configured; provisioning only discovers the known-impossible configuration after settlement when replace_service_records raises DNSControlError, leaving the paid activation in refund_due. Fail this mode before issuing a payable quote unless managed DNS is configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. Every non-hosted Agent Mail quote now requires domains.dns.configured before validating a custom-domain token or creating a bundled domain quote. Missing managed DNS returns managed_dns_not_ready with retry guidance, so an impossible custom activation cannot become payable. Added a launch-gate regression.
| MailAccountRow.mailbox_id != mailbox_id, | ||
| MailAccountRow.domain == row.domain, | ||
| _mailbox_occupies_capacity(), | ||
| ) |
There was a problem hiding this comment.
If a custom-domain activation fails while removing its agent_mail DNS records also fails, the row becomes failed or refund_due with dns_cleanup_pending, but this predicate no longer counts it as occupying the domain. A new paid mailbox can therefore provision on the same domain; the lifecycle retry for the old row will subsequently call remove_service_records(..., managed_by="agent_mail") and delete the new mailbox's records as well. Treat pending-cleanup rows as domain occupants or bind cleanup ownership to a specific activation generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. Custom-domain occupancy now includes both the durable dns_cleanup_pending flag and its legacy error marker in addition to live/capacity states. The predicate is enforced at quote issuance and the serialized activation reservation, so no replacement can provision until the old service-owned RRsets are removed. The transient-cleanup regression proves replacement is blocked before retry and allowed only after successful cleanup.
| indexed = await session.get(MailMessageIndexRow, message_id) | ||
| if indexed is None: | ||
| indexed = MailMessageIndexRow( | ||
| message_id=message_id, | ||
| mailbox_id=mailbox_id, |
There was a problem hiding this comment.
When two JMAP accounts expose the same object ID, the second mailbox can fetch its message successfully from the backend but is forced into this 404 because mail_message_index uses message_id as a global primary key. JMAP IDs are scoped to an account rather than guaranteed globally unique, and list_messages has the same global lookup behavior, so the index key must include mailbox_id (or use a separate surrogate key).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. Migration 022 changes mail_message_index to a composite (mailbox_id, message_id) primary key. Every cache lookup, send finalization, reply lookup, event upsert, list, and detail path now uses the mailbox-scoped key. The regression exposes the same JMAP object id from two accounts and verifies both can list and fetch their independent entries.
| if item[0].endswith("/error"): | ||
| raise MailBackendError(str(item[1].get("description") or item[1].get("type"))) |
There was a problem hiding this comment.
When Stalwart returns a standard HTTP-200 JMAP method error, the response name is error, not a name ending in /error, so this condition accepts the failure as a successful method response. This is especially destructive for delete_account, delete_message, and retention deletion: their callers can clear credentials, mark a mailbox deleted, or remove the local index even though the backend object was never deleted. The identical check in _jmap should also reject item[0] == "error".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16ebe5a. Both management and ordinary JMAP response validators now reject the standard method name error as well as extension-style names ending in /error. An HTTP-200 regression feeds the standard error form through both parsers and verifies destructive calls raise MailBackendError instead of being treated as success.
|
Addressed all six findings from the review of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16ebe5a515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not await gate.settle_verified( | ||
| request, verified, extra_body=payment_metadata |
There was a problem hiding this comment.
If settlement succeeds while PostgreSQL is unavailable, PaymentGate._record() silently drops the best-effort settled event and the subsequent record_activation_settlement() retries can all fail. The mailbox then retains only payment_settlement_pending_at; recover_x402_handoffs() requires either payment_settled_at or a ledger event, while expire_quotes() skips pending settlements. The customer is charged, but the mailbox remains awaiting_payment and reserves launch capacity indefinitely, so this path needs an authoritative way to reconcile the settlement rather than relying on the best-effort ledger.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Before settlement, mailbox activations now persist the verified EIP-3009 authorization alongside the order-local pending marker. Indeterminate facilitator outcomes keep that recoverable state, and the worker replays the single-use authorization (with an on-chain authorization-state fallback), durably records the settlement, then resumes provisioning without relying on payment_events. The new no-ledger regression exercises this exact recovery path.
| if not await gate.settle_verified( | ||
| request, | ||
| verified, | ||
| extra_body=payment_metadata, | ||
| ): |
There was a problem hiding this comment.
When the facilitator settles successfully during a database outage, its ledger write is best-effort and may be dropped before record_x402_settlement() also exhausts its retries. The order is left with payment_settlement_pending_at but no paid_at or settled ledger event; the worker recovers only those latter signals, and quote expiry explicitly protects pending orders. Consequently a charged domain order can remain unpaid and reserved forever, so persist enough settlement authorization or add an authoritative reconciliation path for pending intents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Domain orders now persist the verified EIP-3009 authorization before settlement, retain it when the facilitator outcome is indeterminate, and have the worker authoritatively reconcile it through the payment gate before recording paid_at and queuing fulfillment. Recovery no longer depends on the best-effort metrics ledger; the regression recovers and queues an order with no PaymentEventRow.
| op.create_primary_key( | ||
| "mail_message_index_pkey", | ||
| "mail_message_index", | ||
| ["message_id"], |
There was a problem hiding this comment.
After this migration has allowed two mailboxes to store the same account-scoped JMAP message_id, recreating a primary key on message_id alone fails with a uniqueness violation. That is normal post-upgrade data rather than corruption, so a rollback from 022 becomes unavailable once such IDs collide; resolve or otherwise migrate these duplicates before restoring the old key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Migration 022's downgrade now deterministically keeps the oldest (created_at, mailbox_id) row for each colliding message_id and deletes the other account-scoped duplicates before restoring the legacy single-column primary key. The PostgreSQL downgrade SQL from 022 to 021 compiles successfully.
| "terms_changed", | ||
| "The Agent Mail terms changed; review and re-quote.", | ||
| ) | ||
| payload = dict(quote.request_payload) |
There was a problem hiding this comment.
A custom-domain capability is checked only when the quote is created, while the persisted quote payload contains no ownership or token-hash binding and activation later trusts that payload. If the domain is claimed through claim_legacy_domain() during the quote TTL, that operation clears anon_management_token_hash, but the holder of the now-revoked token can still pay this quote and cause Agent Mail to install DNS records and provision an address on the newly account-owned domain. Bind the quote to the validated capability state and verify that state again before reserving or charging the activation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Custom-domain quotes now bind the validated anonymous capability hash into the signed quote payload. That exact authority is revalidated during activation preparation, under the serialized capacity reservation before payment, and once more before provisioning writes. Legacy claims are also blocked while a payment-authorized activation is awaiting settlement or provisioning. The regression revokes the capability after preparation and verifies reservation is rejected before charge.
| constraints=( | ||
| ["one recipient", "no CC/BCC", "no outbound attachments"] | ||
| if row.kind == "send" | ||
| else ["30 days", "1 GB", "no auto-renew"] | ||
| ), |
There was a problem hiding this comment.
When an operator uses the supported MAIL_ACTIVE_DAYS or MAIL_STORAGE_QUOTA_BYTES overrides, provisioning applies those configured values, but every activation quote still advertises 30 days and 1 GB; /v1/mail/pricing likewise leaves storage_gb at its hardcoded default. Clients can therefore accept and pay against terms that differ from the mailbox actually provisioned, so derive these advertised constraints from MailConfig and use a representation that preserves quotas below or between whole GiB values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Activation quote constraints, product terms, and /v1/mail/pricing now derive duration and storage from MailConfig. Pricing exposes fractional storage_gb plus exact storage_bytes, while quote text reports precise GiB and byte values. A regression uses 45 days and 1.5 GiB and verifies the advertised terms exactly match configuration.
| order.payment_settlement_pending_at = ( | ||
| order.payment_settlement_pending_at or _now() | ||
| ) | ||
| await session.commit() |
There was a problem hiding this comment.
Two concurrent retries of the same awaiting domain order can carry distinct valid payment authorizations: both requests verify first, and begin_x402_settlement() accepts the second request even after the first has set payment_settlement_pending_at. Both callers can therefore reach the facilitator and settle, charging twice for one order; the later record_x402_settlement() also overwrites the stored transaction, leaving no automatic refund for the other charge. Claim or durably bind the order to exactly one authorization before either caller can settle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. Each domain order now stores a canonical authorization fingerprint under its row lock before either caller can settle, rejects a different authorization on the same order, and enforces a database-wide unique constraint so one authorization cannot bind to another order. The fingerprint survives successful settlement; the raw authorization is retained only while reconciliation may be needed. Concurrency regressions cover both rebinding and cross-order reuse.
| service: MailService = Depends(get_mail), | ||
| gate: PaymentGate = Depends(get_gate), | ||
| ) -> MailSendResponse | Response: | ||
| quote = await service.get_quote(body.quote_id) |
There was a problem hiding this comment.
After an active mailbox exists, neither send-quote creation nor this paid send path calls require_launch(). If an operator restarts with MAIL_ENABLED=false, withdraws legal or abuse approval, or removes required control-plane configuration, discovery hides Agent Mail but anyone holding an existing mailbox token can still create quotes, submit messages, and be charged through the direct API. Enforce mail.public_ready when creating a send quote and again before honoring a previously issued quote so the advertised fail-closed/abuse switch actually stops outbound mail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4529e24. require_launch() now gates both MailService.create_send_quote() and MailService.deliver_send(), with the route retaining its own fail-closed check. Disabling any public_ready prerequisite therefore blocks new quotes and previously issued unpaid deliveries before backend submission or charge. The regression verifies both paths and confirms no message is sent.
|
Addressed all seven findings from the review of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4529e24811
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = payload.get("result") if isinstance(payload, dict) else None | ||
| return isinstance(result, str) and int(result, 16) != 0 |
There was a problem hiding this comment.
When a settlement attempt has an indeterminate result, the payer can submit cancelAuthorization for the verified EIP-3009 nonce before the worker retries. authorizationState becomes true for both transferred and canceled authorizations, so this boolean check reports a recovered payment even though no funds moved; the mailbox and domain recovery paths then record the order as settled and provision the paid resource. Recovery needs evidence of the transfer itself, such as a matching finalized transfer receipt/event, rather than merely a consumed nonce.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82faaae. Recovery no longer treats authorizationState=true as payment. It waits 12 blocks, queries the nonce's indexed AuthorizationUsed/AuthorizationCanceled event, and for a used authorization requires a successful receipt containing the exact USDC Transfer from the signed payer to the configured receiver for the signed value. Cancellation or nonce consumption without that transfer is terminally unpaid. Regressions cover cancellation detection and exact receipt matching.
| payment_payload = decode_payment_signature_header(payment_header) | ||
| if not isinstance(payment_payload, PaymentPayload): | ||
| return None | ||
| requirements = self._build_requirements(amount) |
There was a problem hiding this comment.
The worker constructs a fresh PaymentGate and calls the mailbox/domain recovery methods without initializing it, but x402 2.10's build_payment_requirements raises RuntimeError until initialize() has run. Because _build_requirements executes before _ensure_initialized() and this exception is not caught here, every stored-authorization recovery attempt is deferred without ever initializing the gate, leaving indeterminate paid activations stuck indefinitely after a process restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82faaae. reconcile_settlement() now calls _ensure_initialized() before _build_requirements(), so a worker's fresh PaymentGate initializes x402 2.10 before requirement construction. Requirement errors remain retryable rather than escaping the recovery path. The regression uses a server that deliberately raises if requirements are built before initialization.
| if not await self._authorization_consumed_onchain(authorization): | ||
| return None |
There was a problem hiding this comment.
If the original settlement call was indeterminate but replay later returns a definitive failure and the authorization remains unconsumed—especially after validBefore expires—this path returns None without communicating a terminal outcome to the caller. Both mailbox and domain expiry logic permanently excludes rows with payment_settlement_pending_at, so such an authorization leaves its quote/order pending forever and, for mail, permanently consumes one of the limited activation-capacity slots. Recovery needs to clear or terminally fail intents once an unused authorization can no longer settle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82faaae. Reconciliation now returns an explicit terminal-unsettled outcome when a confirmed authorization is canceled, consumed without the matching payment, or still unused after validBefore. Mail terminal handling closes the activation/quote and releases capacity; domain handling expires the order/quote and releases any bundled VM quote state. Regressions verify both resource types lose their pending marker and cannot remain reserved forever.
| f"{self.mail_config.active_days} days; " | ||
| "domain quote plus activation; no auto-renew" | ||
| ), | ||
| available=ready and self.config.domain.agent_purchases_enabled, |
There was a problem hiding this comment.
When Agent Mail is ready and a caller already holds an anonymous managed-domain token, mode=custom can quote, activate, and provision without purchasing a domain, even if agent_purchases_enabled is false. This availability expression nevertheless reports the custom-mail product as unavailable in that configuration, so agents following /v1/mail/products incorrectly skip a working offering; only the domain-and-mailbox bundle should depend on the domain-purchase switch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82faaae. The custom-mail product now depends on Agent Mail readiness plus managed DNS readiness, not domain.agent_purchases_enabled. Only the domain-and-mailbox bundle depends on the sales switch. The regression disables agent domain purchases and verifies custom mail remains advertised while the bundle becomes unavailable.
| url, addresses = await validate_webhook_url(hook.url) | ||
| await self._post_pinned(url, addresses[0], raw, signature, event.event_id) |
There was a problem hiding this comment.
For a webhook hostname with multiple global addresses, delivery is pinned only to the lexicographically first result. If that address is unreachable from the worker—for example, a dual-stack hostname whose sorted AAAA address is selected on a host without working IPv6—each retry chooses the same dead address even when another validated A/AAAA address is healthy, and the webhook is eventually disabled. Iterate over the validated addresses before counting the attempt as failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82faaae. Webhook delivery now iterates every address returned by the already global-address-only validator, pinning and attempting each address before the delivery attempt is counted as failed. A dual-stack regression makes the first (IPv6) address unreachable, verifies the second address succeeds in the same worker pass, and confirms the delivery is recorded once as delivered.
|
Addressed all five findings from the exact-head review of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82faaae3cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not await gate.settle_verified(request, verified, extra_body=payment_metadata): | ||
| raise MailProblem( | ||
| 402, | ||
| "mail_payment_settlement_pending", | ||
| "The message was accepted, but payment did not settle; retry this same quote.", |
There was a problem hiding this comment.
When the facilitator broadcasts the payment but settle_verified() raises before returning its result, this branch reports a pending settlement without durably storing the authorization header. recover_x402_handoffs() only reconciles mailbox activations, while reconcile_send_intents() only recovers Stalwart submission state, so the accepted send can never be attributed to the confirmed payment. A retry with the original authorization may be rejected as already consumed, and a new authorization is rejected by the quote's unique fingerprint binding; persist a send-level settlement intent/header before settlement and reconcile it like account activations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. Accepted sends now persist payer, network, asset, the raw authorization, and a pending timestamp before settlement. Indeterminate retries return 503 without replaying or clearing that intent; the worker reconciles the stored authorization, records confirmed payments, and closes terminal unpaid outcomes as accepted but uncharged. Migration 024 and the send-payment recovery regression cover the full handoff.
| row.provision_claim_token = None | ||
| row.provision_claimed_at = None | ||
| row.provision_next_attempt_at = None | ||
| await session.commit() |
There was a problem hiding this comment.
If the worker exits after this commit but before remove_service_records(), a failed custom-domain activation is already terminal while dns_cleanup_pending is still false. process_lifecycle() only retries DNS cleanup for explicitly flagged rows, and _mailbox_occupies_domain() no longer reserves this failed/refund_due row, so Agent Mail-owned MX/SPF/DKIM records can remain indefinitely and cannot be removed through the customer DNS API. Persist the cleanup obligation atomically with the terminal status, then clear it only after DNS removal succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. A custom-domain activation now commits dns_cleanup_pending=true in the same transaction as its terminal status, before the DNS control-plane call. Successful removal clears the flag; failure leaves it for lifecycle retry. The regression asserts the flag is already durable from inside the external removal call.
| "created_at": event.created_at.isoformat(), | ||
| } | ||
| raw = json.dumps(payload, sort_keys=True, separators=(",", ":")).encode() | ||
| secret = self._decrypt(self._fernet(), hook.secret_ciphertext) |
There was a problem hiding this comment.
When a selected webhook has a missing or undecryptable secret_ciphertext, this call raises before entering the per-delivery try block, aborting the entire batch on every worker pass instead of failing that delivery and continuing. This is reachable after upgrading existing mail_webhooks rows because migration 017 adds the ciphertext as nullable while marking those rows active, and it also occurs after an uncoordinated Fernet-key rotation; once such a delivery reaches the front of the queue, all later webhooks are starved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. Secret decryption and signature creation now run inside the per-delivery exception boundary, so an undecryptable row follows the existing retry/disable policy and the batch continues. The regression queues a corrupt-secret webhook ahead of a healthy webhook and verifies the healthy delivery completes.
| charged_amount_usd=amount( | ||
| Decimal(row.total_amount_usd or row.activation_amount_usd or 0) | ||
| ), |
There was a problem hiding this comment.
For an awaiting_payment account created during the initial 402 round trip—or after a definitive settlement failure—payment_settled_at and payment_tx are both unset, but GET /v1/mail/accounts/{mailbox_id} still reports the full quote total as charged_amount_usd. The management token and status URL are deliberately returned in the first challenge, so clients can observe this state and incorrectly record money as charged; derive this field from settlement state and return zero until payment is durably confirmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. Account responses now report the quote total only when payment_settled_at is present (or payment_tx exists for legacy rows); awaiting-payment activations report 0.00. A dedicated regression covers the observable pre-payment activation response.
| "filter": {"header": ["X-Hyrule-Send-ID", send_id]}, | ||
| "limit": 1, |
There was a problem hiding this comment.
If Email/set creates the draft but EmailSubmission/set returns notCreated, send_message() raises while leaving an email carrying this header in the Drafts mailbox. On the next retry or worker reconciliation, this header-only query finds that unsent draft and _submit_send_intent() finalizes it as accepted; the route can then settle payment even though no submission occurred. Recovery must verify a successful EmailSubmission or otherwise exclude draft-only messages before marking the send accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. Recovery now uses the stable header to find candidate Email ids, then requires a matching EmailSubmission/query plus EmailSubmission/get record before treating any candidate as submitted. A matching draft with no submission now returns no recovery result, with both submitted and draft-only cases covered by tests.
| or not network.rpc_url | ||
| or authorization.pay_to != self.config.receiver_address.lower() | ||
| ): | ||
| return _AuthorizationChainOutcome("unknown") |
There was a problem hiding this comment.
An enabled PaymentNetwork is allowed to have an empty rpc_url, and normal verification and settlement can still work through the facilitator, but any indeterminate settlement on that network reaches this branch and returns unknown forever. Both domain and mailbox quote expiry deliberately skip rows with payment_settlement_pending_at, so the order or mailbox capacity remains permanently reserved even after the authorization expires. Reject payable networks without a recovery RPC (or provide another terminal-outcome source) before accepting payments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 061ca98. PaymentGate now fails construction and lists every enabled payment network whose recovery RPC URL is blank, so the service cannot accept a payment it cannot reconcile to a terminal outcome. The regression verifies an enabled Base configuration without an RPC is rejected.
|
Addressed all six exact-head findings in 061ca98:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 061ca9861f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MailPaymentAuthorizationRow( | ||
| fingerprint=fingerprint, | ||
| quote_id=quote_id, | ||
| created_at=_now(), | ||
| ) |
There was a problem hiding this comment.
When the same EIP-3009 authorization is concurrently verified against another equal-priced endpoint—defaults include the $0.01 mail send, direct proxy, BGP query, and threat/VoIP checks—this insert enforces uniqueness only within mail_payment_authorizations; domain bindings and generic settlements live elsewhere. If the mail request becomes indeterminate after the other endpoint settles, recover_x402_handoffs observes that authorization's on-chain transfer and marks the mail send paid too, granting two resources for one transfer. Enforce a service-wide authorization or transaction binding, while explicitly representing the intentional domain-and-mail bundle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. Every verified x402 authorization is now atomically claimed in a service-wide payment_authorizations table before settlement or delivery attribution; retries are allowed only for the same logical resource. Mail sends, activations, domain orders, and the intentional domain+mail bundle have stable keys, and durable recovery enforces the same ownership. Migration 025 backfills existing domain/mail bindings, including bundle identity. Regression coverage proves a mail authorization cannot be reused by BGP and that recovery fails closed when another resource owns it.
| if row is not None and row.quote_id == quote_id and row.payment_settled_at is None: | ||
| row.owner_wallet = None | ||
| row.payment_network = None | ||
| row.payment_asset = None | ||
| row.payment_authorization_header = None | ||
| row.payment_settlement_pending_at = None | ||
| await session.commit() |
There was a problem hiding this comment.
When activation settlement returns a definitive failure, the route calls this method and tells the client that payment failed, but the MailPaymentAuthorizationRow created by bind_payment_authorization remains uniquely attached to the quote. A replacement authorization can verify successfully but is then rejected with mail_quote_payment_bound, while the original authorization may be permanently unusable due to expiry or nonce failure, stranding the activation and reserved address until expiry cleanup. Remove the quote binding atomically when clearing a definitively failed authorization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. A definitive activation or send settlement failure now deletes the quote-scoped MailPaymentAuthorizationRow in the same transaction that clears the stored settlement intent, so a replacement authorization can bind without releasing anything while the outcome is indeterminate. The new activation regression binds a failed authorization, clears it, then successfully binds a replacement.
| if domain is None or str(domain.status) not in { | ||
| DomainStatus.ACTIVE.value, | ||
| DomainStatus.RENEWAL_DUE.value, | ||
| }: | ||
| raise DomainProblem(409, "domain_not_active", "The managed domain is not active.") |
There was a problem hiding this comment.
When a custom domain becomes expired or transferred before its mailbox reaches the end of grace, process_lifecycle calls remove_service_records, which delegates here with an empty record list and is rejected by this active-only status check. Every cleanup retry encounters the same 409, leaving the Agent Mail RRsets and dns_cleanup_pending obligation indefinitely and preventing reuse of the domain for another mailbox. Keep additions active-only, but allow removal of service-owned records from terminal domain states.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. Service-record additions remain restricted to active/renewal-due domains, while an empty replacement is permitted for terminal domain states. Cleanup removes the owned rows transactionally and updates the authoritative zone only when the domain is still in managed-nameserver mode. The regression expires a domain and verifies Agent Mail records can still be removed.
| .order_by(MailAccountRow.created_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
When the oldest limit mailboxes are pending_domain and their domain orders remain queued or provider-pending, the loop simply continues for each of them, but this query returns those same rows on every worker poll because their status and provision_next_attempt_at never change. Ten slow bundled registrations can therefore prevent every later hosted or otherwise ready provisioning mailbox from being processed until the registrar completes. Select ready provisioning rows independently or paginate past unresolved domain orders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. provision_pending now scans pending domain registrations separately, promotes only active orders, and then independently selects ready provisioning mailboxes. Unresolved registrar orders therefore cannot consume the ready batch. The regression places an older provider-pending domain order ahead of a hosted mailbox and verifies the hosted mailbox is provisioned with limit=1.
| select(MailSendRow.send_id) | ||
| .where(MailSendRow.status == "submitting") | ||
| .order_by(MailSendRow.created_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
A backend submission failure leaves its row in submitting, and when find_message_by_send_id keeps returning no submission this reconciliation pass makes no state change. Once the oldest limit rows are in that condition, every worker pass scans the same rows and never reaches later sends that were actually accepted before a process interruption; with the default daily global limit, 100 abandoned failures can produce this state on the following day. Paginate through candidates or move stale no-submission rows back to a retryable/terminal state after their lease.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. Send reconciliation now processes only expired submission leases, keyset-paginates by creation time/send ID, finalizes backend-confirmed sends, and moves stale no-submission intents back to pending for retry. Exceptions advance the cursor without rewriting ambiguous state, so one unavailable mailbox cannot starve later recoverable sends. Regression coverage exercises an older missing submission followed by a later accepted submission.
| type=row.type.value, | ||
| ttl=row.ttl, | ||
| values=row.values, | ||
| managed_by=managed_by, |
There was a problem hiding this comment.
For an account-owned custom domain with an active mailbox, these RRsets are protected from ordinary DNS changes through managed_by="agent_mail", but enqueue_nameserver_update and _assert_external_delegation_allowed only check VM attachment state. The owner can therefore switch to external nameservers, causing _apply_nameservers to delete the managed zone while the paid mailbox remains active and still reports normal service status; MX, SPF, and DKIM disappear unless the customer happens to reproduce them externally. Reject external delegation and transfer-out while Agent Mail occupies the domain, or explicitly tear down the mailbox first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6586. External nameserver changes and transfer-out now reject domains occupied by Agent Mail at enqueue time and recheck at worker execution. The guard covers pending/provisioning/active/suspended/grace mailboxes, paid capacity reservations, and outstanding DNS cleanup; the existing VM guard remains intact. Regression coverage checks both operation types and a mailbox activated after enqueue but before worker execution.
|
Addressed all six exact-head findings from the review of This round adds:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39d6586731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| row is not None | ||
| and str(row.status) in {DomainStatus.ACTIVE.value, DomainStatus.RENEWAL_DUE.value} | ||
| and expected_hash | ||
| and row.anon_management_token_hash | ||
| and secrets.compare_digest(row.anon_management_token_hash, expected_hash) |
There was a problem hiding this comment.
Require nameserver_mode == managed as part of this authority check. A domain can remain active with the same management-token hash after being switched to external nameservers, so a custom-mail quote and payment currently pass this guard. Provisioning then calls replace_service_records, which only applies the zone when delegation is managed, but still marks the mailbox active; the customer is therefore charged for a mailbox whose MX/SPF/DKIM records were never published.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cce67a. Managed-domain token validation, stored authority revalidation, capacity reservation, and provisioning now all require nameserver_mode=managed in addition to active status and the capability hash. The regression creates a valid custom activation, switches the domain to external delegation before payment reservation, and verifies the activation is rejected before settlement.
| row.payment_authorization_header = None | ||
| row.payment_settlement_pending_at = None | ||
| row.error = f"payment_authorization_{reason}"[:2000] | ||
| await session.commit() |
There was a problem hiding this comment.
When an indeterminate send settlement is later proven terminally unsettled, this error marker makes settled_send_response return the accepted send immediately with charged_amount_usd == 0.00. This path also leaves the quote's MailPaymentAuthorizationRow intact, unlike clear_send_settlement, so removing the short-circuit alone would make a fresh authorization fail with mail_quote_payment_bound. Release the quote binding and allow the same accepted send to retry settlement rather than permanently converting it into an uncharged success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cce67a. A terminally unsettled accepted send now clears its settlement intent and deletes the quote-scoped MailPaymentAuthorizationRow atomically, while settled_send_response no longer treats the retryable diagnostic marker as paid success. The same accepted message remains payable without being submitted again. The regression proves it reports no settlement, accepts a replacement authorization, and retains accepted send state.
| .order_by(MailAccountRow.created_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
Paginate these pending-domain rows or select only rows whose associated domain order is actionable. With more than limit domain-and-mail activations, if the oldest ten registrations remain queued, every worker poll selects those same ten and does not update their next-attempt time; an eleventh order that has already become active or failed is never inspected, and the independent ready-mailbox query cannot help because its mailbox remains in pending_domain. This can leave a paid activation stuck indefinitely behind unrelated slow registrations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cce67a. The pending-domain scan now outer-joins domain orders and selects only actionable active/terminal/missing orders; unresolved queued/provider-pending registrations are excluded rather than consuming the limit. Ready provisioning remains an independent query. The regression places an old provider-pending order ahead of both a hosted mailbox and a later active domain order, then verifies both actionable mailboxes progress with limit=1.
There was a problem hiding this comment.
Follow-up in ecdc9cb: the actionable and terminal sets now also include DomainOrderStatus.REFUNDED, covering the case where refund processing completes before the mail worker observes refund_due. The expanded starvation regression advances the formerly blocked order to refunded and verifies the mailbox transitions to failed instead of remaining pending.
| f"{self.mail_config.active_days} days; " | ||
| "live domain quote plus activation; no auto-renew" | ||
| ), | ||
| available=custom_ready and self.config.domain.agent_purchases_enabled, |
There was a problem hiding this comment.
The bundle is advertised as available whenever mail, DNS, and agent_purchases_enabled are set, but create_agent_order additionally requires domain legal/tax approval, valid agent-token encryption, and complete registrar credentials. In any partially configured rollout missing one of those prerequisites, /v1/mail/products tells autonomous clients the bundle is available and the documented workflow proceeds through a live registrar quote only to fail at activation with 503. Use the same complete agent-domain readiness predicate here that protects checkout and marketplace discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cce67a. HyruleConfig now exposes one shared agent_domain_purchases_ready predicate covering the agent flag, legal/tax approval, managed DNS URL+secret, a valid Fernet token key, and every registrar credential/contact. Both x402 marketplace discovery and the Agent Mail bundle product use that same predicate; hosted/custom mail availability remains independent. The regression verifies the bundle stays unavailable until all checkout prerequisites are present.
| if quote is not None: | ||
| quote.status = MailQuoteStatus.EXPIRED.value | ||
| quote.request_payload = {"redacted": True} |
There was a problem hiding this comment.
After recovery proves an activation authorization expired or was canceled, this replaces the activation payload with only {"redacted": true}. A subsequent valid GET /v1/mail/quotes/{quote_id} still calls _quote_response, which unconditionally reads payload["mode"] and payload["activation_amount_usd"], so the terminal quote now produces an unhandled KeyError and HTTP 500. Either retain the non-secret activation fields or make quote rendering explicitly handle redacted terminal quotes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cce67a. Terminal activation failure now redacts the request while preserving only the non-secret mode and activation amount required to render the immutable quote split. Quote rendering also tolerates older fully redacted rows instead of indexing missing keys. The terminal-recovery regression now calls get_quote and verifies the expired quote's mode, activation amount, and total remain available without a 500.
|
Addressed all five exact-head findings from the review of This round adds:
Validation:
@codex review |
|
Follow-up hardening in Please review the current exact head @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecdc9cbd98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = await service.deliver_send(body.quote_id, token) | ||
| await service.begin_send_settlement( |
There was a problem hiding this comment.
If the API process exits after Stalwart accepts the message but before begin_send_settlement commits, the accepted send has neither the signed authorization header nor payment_settlement_pending_at. reconcile_send_intents can recover the accepted message, but recover_x402_handoffs only selects sends with those settlement fields, so this authorization can never be settled unless the client retries the same quote before it expires. Persist the recoverable payment intent before the external submission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1f56f5. The send route now calls prepare_send_settlement before deliver_send; that step durably reserves the send and commits the payer, network, asset, signed authorization, and settlement-pending timestamp before the first Stalwart submission. A regression inspects the row from inside the backend call and proves the intent is already committed and survives an ambiguous submission failure.
| needs_dns_cleanup = row.plan != MailboxMode.HOSTED.value and bool(row.domain) | ||
| row.status = MailboxStatus.REFUND_DUE.value if refund else MailboxStatus.FAILED.value | ||
| row.provision_error = reason[:2000] | ||
| row.dns_cleanup_pending = needs_dns_cleanup |
There was a problem hiding this comment.
When a combined domain/mail order becomes terminal before mailbox provisioning starts—for example, mark_x402_paid moves the order directly to refund_due because its quote is no longer usable—no agent_mail records or even a DomainRow may exist. This condition nevertheless sets dns_cleanup_pending solely because the plan is non-hosted; remove_service_records then fails with domain_not_found, lifecycle retries cannot clear the flag, and _mailbox_occupies_domain permanently rejects a replacement activation for that domain. Track whether mail DNS was actually installed or attempted before requiring cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1f56f5. Failed non-hosted activations now require the durable backend_id marker before scheduling DNS cleanup. Provisioning commits that marker immediately before the external DNS write, so a combined order that becomes terminal before provisioning neither sets dns_cleanup_pending nor calls remove_service_records; the regression covers the refunded-before-provisioning path.
| mailbox_id=account.mailbox_id, | ||
| quote_id=quote_id, | ||
| recipient=recipient, | ||
| in_reply_to=payload.get("in_reply_to"), |
There was a problem hiding this comment.
For replies, payload["in_reply_to"] is no longer the client-supplied JMAP id: _resolve_reply_reference replaces it with the inbound RFC Message-ID without a length check. A valid inbound Message-ID longer than 128 characters therefore reaches the MailSendRow.in_reply_to VARCHAR(128) column here and PostgreSQL raises a data-length error, returning a 500 and preventing replies to that message. Widen the persisted field or reject unsupported references before issuing the send quote.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1f56f5. MailSendRow.in_reply_to is now TEXT, and migration 026 drops the old index and widens the PostgreSQL column (with a safe downgrade that truncates before restoring VARCHAR(128)). The reply regression now resolves, persists, and submits an RFC Message-ID over 300 characters; both 025→026 and 026→025 SQL compile.
| cursor: tuple[datetime, str] | None = None | ||
| while True: |
There was a problem hiding this comment.
On every 15-second worker pass, this cursor is reset and the loop pages through every historical settled /v1/mail/accounts event until it reaches the oldest row, even when there are no recoverable accounts; it also performs a separate account query for nearly every event. The analogous domain recovery loop has the same behavior, so payment-history growth makes each pass O(total historical settlements) and can eventually monopolize the lifecycle worker. Persist a checkpoint, constrain the fallback window, or drive the lookup from currently awaiting resource IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1f56f5. Mail and domain ledger fallback are now driven from currently recoverable resource rows: each pass selects at most limit awaiting/unsettled accounts or orders having an exact matching settlement event, then fetches only that resource's latest matching event. The resettable full-history cursors and per-historical-event resource queries are gone. Regressions place unrelated historical events ahead of the target and recover it with limits of 1/2.
commented
Jul 22, 2026
|
Addressed all four exact-head findings from the review of This round adds:
Validation:
Please review exact head @codex review |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f56f544d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .order_by(MailAccountRow.created_at, MailAccountRow.mailbox_id) | ||
| .limit(limit) |
There was a problem hiding this comment.
If the oldest limit ledger-backed accounts repeatedly raise DomainProblem in mark_activation_paid—for example, because one bundled domain order remains unrecoverable—those accounts remain eligible and are selected again on every worker pass. Since the query applies a fixed oldest-first limit without a cursor or retry deferral, newer customers with settled payments are never reached; paginate past failed candidates or move them out of the immediate candidate set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3d88e15. Activation recovery now uses a durable 30-second retry deferral in provision_next_attempt_at: pending-authorization, durable-settlement, and ledger-fallback account queries all exclude deferred rows, failures/no-result reconciliation set the deferral, and a successful durable settlement clears it. The regression keeps the oldest ledger-backed account failing, runs with limit=1, and proves the next settled customer is selected on the following pass. Full validation: 582 tests, Ruff/format checks, and mypy across 102 source files pass.
commented
Jul 22, 2026
|
Addressed the exact-head P1 from the review of Failed activation-payment handoffs now receive a durable retry deferral, and every account recovery source skips deferred rows so an unrecoverable oldest candidate cannot consume the batch indefinitely. The new Validation:
Please review exact head @codex review |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d88e15ce9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _SEND_RESERVED_STATUSES = ("pending", "submitting", "accepted") | ||
| _SEND_SUBMISSION_LEASE = timedelta(minutes=5) | ||
| _PAYMENT_HANDOFF_GRACE = timedelta(hours=1) | ||
| _PAYMENT_HANDOFF_RETRY_DELAY = timedelta(seconds=30) |
There was a problem hiding this comment.
With two permanently failing older candidates and limit=1, this 30-second delay does not prevent starvation: the worker invokes recover_x402_handoffs every 15 seconds, so it defers candidate A at t=0 and B at t=15, then A is due again at t=30; because every query still orders by the original settlement/creation timestamp, A and B alternate forever and a later settled customer is never selected. This also occurs with larger limits once there are at least twice as many blockers as the limit. Order never-attempted rows ahead of retry-due rows or otherwise persist a fair cursor rather than relying only on a fixed deferral interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c12182. All three activation-recovery queries now sort never-attempted rows (provision_next_attempt_at IS NULL) ahead of retries, then order retries by their durable due timestamp before the original settlement/creation tiebreaker. The regression uses two permanently failing older accounts with limit=1, makes the oldest retry due again, and proves the untouched third settlement is still selected first.
| row.payment_authorization_header = None | ||
| row.payment_settlement_pending_at = None | ||
| row.payment_settled_at = row.payment_settled_at or _now() | ||
| row.provision_next_attempt_at = None |
There was a problem hiding this comment.
When a stale concurrent recovery reaches record_activation_settlement after another worker has already moved the account to provisioning, the existing guard permits the call because payment_settled_at is non-null, and this new assignment clears the DNS retry deadline stored in the same column. provision_pending can then immediately retry an incomplete DNS setup instead of waiting provision_dns_retry_seconds, incrementing the bounded retry counter early and potentially refunding an activation after rapid retries. Only clear this field while the row is still in an activation-handoff state, rather than on an idempotent settlement replay for an already-provisioning row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c12182. record_activation_settlement now clears provision_next_attempt_at only when the row is still awaiting_payment; an idempotent replay after another worker advances it to provisioning preserves the DNS retry deadline. The regression transitions an account to provisioning, installs a five-minute retry deadline, replays the settlement, and verifies the deadline remains exact.
commented
Jul 22, 2026
|
Addressed both exact-head findings from the review of This pass:
Validation:
Please review exact head @codex review |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c12182592
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if awaiting_handoff: | ||
| row.provision_next_attempt_at = None |
There was a problem hiding this comment.
When settlement is replayed while an activation remains AWAITING_PAYMENT after _defer_activation_recovery assigned a future retry time, awaiting_handoff is still true and this clears that durable backoff. This can occur when concurrent recovery workers both fetched an unsettled row: one records settlement and defers a failed handoff before the other records the same settlement, making the row immediately eligible for another retry. Determine whether this is the first settlement from the preexisting payment_settled_at value rather than from the status so idempotent replays do not bypass the 30-second retry delay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96eef86. record_activation_settlement now derives first_settlement from the preexisting payment_settled_at value, so only the first durable settlement clears provision_next_attempt_at. An idempotent replay while the activation is still awaiting payment preserves the exact future handoff retry deadline. Added a regression covering that concurrent-recovery sequence.
| .order_by(DomainOrderRow.payment_settlement_pending_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
When at least limit old domain authorizations keep producing an unknown reconciliation result, these oldest rows are selected on every worker poll because neither their pending timestamp nor their eligibility changes when reconciliation.payment is None. Consequently, every newer pending domain order is permanently excluded from reconciliation even if its facilitator settlement is immediately recoverable, potentially leaving paid registrations stuck in awaiting_payment. Defer attempted rows or use durable cursor/fairness ordering so an inconclusive authorization cannot monopolize the bounded batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96eef86. Migration 027 adds a durable payment_recovery_next_attempt_at to domain orders. Unknown results and reconciliation exceptions defer the attempted authorization for 30 seconds, while the bounded query ranks never-attempted orders ahead of due retries; terminal and successful outcomes clear the deadline. The regression uses limit=1 and proves an untouched newer order is recovered before the inconclusive older order.
| .order_by(MailSendRow.payment_settlement_pending_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
When the oldest limit accepted sends repeatedly receive an unknown reconciliation outcome, their unchanged payment_settlement_pending_at values cause this query to return the same rows on every poll. Later accepted sends are therefore never offered to reconcile_settlement, even when their payments are recoverable, leaving delivered messages indefinitely unattributed and potentially uncharged. Add a durable retry time or fair cursor/rotation rather than permanently ordering every bounded batch by the original pending timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96eef86. Mail sends now persist payment_recovery_next_attempt_at through migration 027. Unknown results and reconciliation exceptions defer the attempted authorization, and recovery orders never-attempted sends before due retries; clear, terminal, and settled paths remove the deadline. The limit=1 regression proves an untouched newer send is reconciled instead of being starved by the oldest inconclusive send.
| account = await session.scalar( | ||
| select(MailAccountRow).where(MailAccountRow.quote_id == row.quote_id) | ||
| ) |
There was a problem hiding this comment.
At the payment-window boundary, this account is read and checked without a row lock, so a concurrent activation request can persist or complete settlement after the expiry transaction has observed all payment fields as empty but before it commits the tombstone. The expiry write then sets the account to DELETED and erases its capability without rechecking; even if payment_settled_at or the authorization intent survives as a concurrently updated column, every recovery query excludes deleted accounts, stranding the payment without activation or refund. Lock the account and evaluate the pending/settled fields only after acquiring that lock before releasing the address.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96eef86. expire_quotes now acquires a FOR UPDATE lock on the activation account before evaluating or writing the tombstone, refreshes the quote after any lock wait, and rechecks quote status and expiry plus the pending, settled, and durable-ledger evidence while holding that lock. A regression asserts the lock-and-refresh ordering before the settlement checks, alongside the paid-expiry behavior coverage.
| account.status = MailboxStatus.FAILED.value | ||
| account.provision_error = "payment_window_expired" | ||
| account.capacity_reserved_at = None |
There was a problem hiding this comment.
Fixed in 19364ae. After the one-hour handoff grace and the settled/dev-bypass ledger check, a still-unpaid activation now becomes a closed tombstone: its capability ciphertext and reservation/lease state are cleared, while the old idempotency attempt remains closed. deleted rows no longer block address quoting, and the regression proves the same address can be quoted and activated again after expiry.
| global_count = int( | ||
| await session.scalar( | ||
| select(func.count()) | ||
| .select_from(MailSendRow) | ||
| .where( | ||
| MailSendRow.created_at >= day_start, | ||
| MailSendRow.status.in_(_SEND_RESERVED_STATUSES), | ||
| ) | ||
| ) | ||
| or 0 |
There was a problem hiding this comment.
Fixed in 9552e9a. Every new send reservation now takes a transaction-scoped PostgreSQL advisory lock before the global daily count and intent insert. After waiting, it still locks and rechecks the quote, so concurrent sends for different mailboxes cannot cross the launch-wide cap. The regression asserts that serialization occurs before global_count is read.
| def _mail_payment_authorization_fingerprint(request: Request) -> str | None: | ||
| supplied = request.headers.get("payment-signature") or request.headers.get("x-payment") | ||
| if not supplied: | ||
| return None | ||
| return hashlib.sha256(supplied.encode()).hexdigest() |
There was a problem hiding this comment.
Fixed in 9552e9a. The route now decodes the x402 header and fingerprints normalized immutable EIP-3009 authorization fields plus network and asset, ignoring JSON ordering/whitespace, base64 transport differences, payload extras, and signature variants for the same authorization. A distinct nonce produces a distinct fingerprint. The durable binding is shared by activation and send quotes.
| and config.domain.dns_control_url | ||
| and config.domain.dns_control_secret | ||
| and config.domain.agent_order_fernet_key | ||
| and provider.username |
There was a problem hiding this comment.
Fixed in 9552e9a. Agent-domain discovery now constructs Fernet with the configured key and handles the same invalid-key exceptions as the domain service. A malformed but nonempty key therefore removes the route from OpenAPI/x402 discovery. The regression covers malformed and generated-valid keys.
| if not await gate.settle_verified( | ||
| request, verified, extra_body=payment_metadata | ||
| ): | ||
| await service.release_activation_capacity(account.mailbox_id) |
There was a problem hiding this comment.
Fixed in 9552e9a. Activation now commits a pre-settlement intent before calling the facilitator, so expiry cannot tombstone an in-flight or unknown settlement. After success it durably records the settlement timestamp/transaction with bounded retry before provisioning handoff, and the worker replays that mailbox-local marker independently of the best-effort metrics ledger. A no-ledger regression forces the post-settlement activation handoff to fail and verifies worker recovery.
| row.payment_authorization_header = None | ||
| row.payment_settlement_pending_at = None | ||
| row.payment_settled_at = row.payment_settled_at or _now() | ||
| row.provision_next_attempt_at = None |
There was a problem hiding this comment.
Fixed in 8c12182. record_activation_settlement now clears provision_next_attempt_at only when the row is still awaiting_payment; an idempotent replay after another worker advances it to provisioning preserves the DNS retry deadline. The regression transitions an account to provisioning, installs a five-minute retry deadline, replays the settlement, and verifies the deadline remains exact.
| if awaiting_handoff: | ||
| row.provision_next_attempt_at = None |
There was a problem hiding this comment.
Fixed in 96eef86. record_activation_settlement now derives first_settlement from the preexisting payment_settled_at value, so only the first durable settlement clears provision_next_attempt_at. An idempotent replay while the activation is still awaiting payment preserves the exact future handoff retry deadline. Added a regression covering that concurrent-recovery sequence.
| .order_by(DomainOrderRow.payment_settlement_pending_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
Fixed in 96eef86. Migration 027 adds a durable payment_recovery_next_attempt_at to domain orders. Unknown results and reconciliation exceptions defer the attempted authorization for 30 seconds, while the bounded query ranks never-attempted orders ahead of due retries; terminal and successful outcomes clear the deadline. The regression uses limit=1 and proves an untouched newer order is recovered before the inconclusive older order.
| .order_by(MailSendRow.payment_settlement_pending_at) | ||
| .limit(limit) |
There was a problem hiding this comment.
Fixed in 96eef86. Mail sends now persist payment_recovery_next_attempt_at through migration 027. Unknown results and reconciliation exceptions defer the attempted authorization, and recovery orders never-attempted sends before due retries; clear, terminal, and settled paths remove the deadline. The limit=1 regression proves an untouched newer send is reconciled instead of being starved by the oldest inconclusive send.
| account = await session.scalar( | ||
| select(MailAccountRow).where(MailAccountRow.quote_id == row.quote_id) | ||
| ) |
There was a problem hiding this comment.
Fixed in 96eef86. expire_quotes now acquires a FOR UPDATE lock on the activation account before evaluating or writing the tombstone, refreshes the quote after any lock wait, and rechecks quote status and expiry plus the pending, settled, and durable-ledger evidence while holding that lock. A regression asserts the lock-and-refresh ordering before the settlement checks, alongside the paid-expiry behavior coverage.
commented
Jul 22, 2026
|
@codex review |
commented
Jul 22, 2026
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Why
The customer campaign needs a concrete agent-native email identity outcome: an agent can acquire a domain, activate a mailbox, prove controlled send/receive, and diagnose delivery failures without a human account or subscription checkout.
Product impact
Rollout
The API is fail-closed until the related infrastructure and product launch gates are approved. Production customer-result examples remain pending real canaries.
Related PRs
Validation
uv run pytest -q— 500 passeduvx ruff check .uv run mypy hyrule_cloud— 102 source files