feat(mail): connect an inbox with IMAP/SMTP and an app password (HT-101) - #165
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IMAP/SMTP mailbox connectivity, encrypted credentials, scheduled IMAP ingestion, mailbox-aware outbound routing, authenticated APIs, cron wiring, and admin mailbox-management/settings UI. Conversations persist the mailbox associated with their first inbound message. ChangesIMAP/SMTP mailbox transport
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…T-101) Stage 1 of HT-101: a bounded, cron-shaped IMAP fetch adapter (src/providers/adapters/imap/) and an SMTP EmailSender (src/providers/adapters/smtp/), per specs/mail/mailbox-connection.md §5/§7 items 2 and 4. Deliberately does not implement InboundEmailProvider (that spec flags the interface as webhook-shaped, not fetch-shaped) — the IMAP side exposes its own pure fetch function returning RawInboundMessage, mirroring src/mail/gmail-reconcile.ts's shape without any of the storage/ingest/lease wiring, which stays Stage 2. Built on imapflow (MIT) and nodemailer (MIT-0), each touched in exactly one file. Equivalence-fixture test proves the IMAP path hands parseInboundEmail the exact same bytes any other transport would. SMTP sender is wire-level tested against nodemailer's real (no-network) streamTransport, proving Message-ID/In-Reply-To/ References survive verbatim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adversarial review (Opus + independent Codex pass) found the per-invocation fetch bound was a UID *range* (sinceUid+1 : sinceUid+max). IMAP UIDs are not dense, so on any mailbox whose next live UID sits above that window (expunged history, or a UIDVALIDITY reset onto a high-UID epoch) the fetch returned nothing, the cursor never advanced, and new mail was silently never ingested — a violation of CHARTER §2's never-drop invariant. - client.ts: discover actual new UIDs via UID SEARCH (sinceUid+1):*, filter strictly > sinceUid (handles the N:* includes-highest quirk), take the lowest max, FETCH that explicit set; sort results oldest-UID-first. - client.ts: make the imapflow instance injectable so this logic is unit- tested without a network — the file that held the bug previously had none. - client.test.ts (new): sparse/high/gapped UIDs, the N:* quirk, count bound, out-of-order FETCH stream, empty mailbox, never-drop guards. - fetch.ts: enforce oldest-UID-first ordering independent of the client. Gates: typecheck clean, biome clean, 1540 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Migration 027 adds three 1:1-per-mailbox sidecar tables — imap_mailbox_config (host/port/username), imap_mailbox_credentials (encrypted app password), and imap_watch_state (UID cursor + fetch lease) — kept out of the mailboxes table exactly as the Gmail transport keeps its own state out. Plus the three stores. Security-critical elements reviewed by Opus + an independent Codex pass: - App password encrypted at rest via the shared token-crypto AES-256-GCM path, write-only (never returned over any read API), never logged. - Fetch lease mirrors the Gmail reconcile lease's ::text token precision-safety (a stale holder cannot clobber a live successor's lease). Review hardening applied: CHECK constraints enforcing the RFC 3501 unsigned- 32-bit range on uid_validity/last_uid and the 1-65535 range on ports; claimFetchLease rejects a non-positive lease duration. Gates: typecheck clean, biome clean, 1580 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stage 2a-ii: the IMAP/SMTP connect+check HTTP endpoints and the per-inbox scheduled-fetch cron that ingests mail via the Stage 1 adapters and Stage 2a-i stores. Mirrors src/mail/gmail-connect.ts and gmail-reconcile.ts's patterns: connect proves both legs usable before persisting anything atomically; the cron advances the cursor only once every ingest outcome is terminal, and pauses (never re-ingests) on a UIDVALIDITY reset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…T-101) Adversarial review (Opus + independent Codex pass) found a CRITICAL the test suite passed on because a test codified the bug: connect() always re-seeded the baseline cursor, so RECONNECTING an inbox (e.g. to rotate the app password) rewound the cursor and silently dropped any mail that had arrived since the last fetch (CHARTER §2 never-drop). - imap-watch-state: new seedBaselineIfAbsent (INSERT ... ON CONFLICT DO NOTHING). connect() uses it, so a reconnect preserves the existing cursor; a brand-new inbox still baselines at uidNext-1. A changed-UIDVALIDITY reconnect keeps the stale cursor, which the fetch cron then pauses on — fails closed, never silently reseeds. Flipped the test that codified the drop; added store-level coverage. - imap-connect: attemptImapConnection now constructs the client inside the try and guards close() in finally, so neither can throw out of the 'never-throws' leg contract or leak a raw error. Error sanitizer also redacts the base64 form of the password. - imap-fetch: the cron redacts the password from any thrown error before it reaches the logger. Codex re-review of the fix: no findings. Gates: typecheck clean, biome clean, 1656 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Foundation for per-inbox outbound (Stage 2b): a conversation now records which connected inbox its first message arrived at, so a reply can later be routed back out through that same inbox. - Migration 028: conversations.mailbox_id uuid REFERENCES mailboxes(id) ON DELETE SET NULL (nullable — pre-existing conversations read as null and the send path will treat that as the deployment default; SET NULL so deleting a mailbox never deletes customer conversations). - ingest: threads RawInboundMessage.mailboxId into writeParsedEmail, which stamps it ONLY when a genuinely new conversation is created (both the 'new' decision and the deleted/not-found fallback). The reply-to-existing path is untouched — a reply never overwrites a conversation's original inbox. Purely additive: no change to parsing, threading, dedup, the ledger, or event emission. - conversations store: NewConversation.mailboxId (write) + StoredConversation .mailboxId (read, null for pre-existing rows). Reviewed by Opus + independent Codex pass (no findings — confirmed stamp is new-conversation-only and ingest semantics unchanged). Gates: typecheck clean, biome clean, 1662 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add SenderResolver (src/mail/sender-resolver.ts): resolves the EmailSender + from-address for a conversation's mailboxId, branching on provider (gmail -> that mailbox's own OAuth token; imap -> its own SMTP config+credential); null mailboxId falls back to the deployment's default mailbox (config.supportAddress), preserving pre-2b-i behavior. Wire it into handleReply (replacing the fixed sender/supportAddress pair) and into the delivery worker's per-claimed-row retry (replacing the single global sender), so every reply and its retries go out through the same inbox the conversation arrived at. sendReply and attemptDeliveryOfClaimedThread in send.ts are untouched — only how callers supply sender/from changes.
…101) Adversarial review (Opus + independent Codex pass) of the outbound rework: - Codex HIGH: the delivery worker resolved a retry's sender from the conversation's CURRENT mailbox_id, but the row sends with its persisted fromAddress. If the mailbox was hard-deleted (mailbox_id -> null via ON DELETE SET NULL), the retry would fall to the deployment default transport while still using the deleted inbox's From — an SPF/DKIM misalignment. Fix: after resolving, refuse to send when resolved.from does not equal the row's persisted fromAddress (mark the row failed, sweep continues) — so transport can never drift from the From header. Normal rows always match (both are the mailbox's own address). Added a test. (The delivery worker isn't wired to run yet — HT-103 — so this is a latent-path fix ahead of that wiring.) - Codex LOW: handleReply now maps a SenderResolutionError to a clean 502 send_failed instead of the catch-all 500. - Codex MEDIUM (accepted, no change): a lease/maxSendMs MISCONFIGURATION now leaves one claimed row until lease expiry instead of failing pre-claim — only bites under a loud, self-healing deploy bug. Gates: typecheck clean, biome clean, 1676 tests pass. send.ts still byte-unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…SMTP form (HT-101) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e (HT-101)
The HT-101 connect-an-inbox increment hung a mailbox LIST + connect form
off SettingsScreen's "Inboxes" section — a scope violation under
specs/ui/admin-ia.md §1 (the mailbox list is Global-admin scope, per-mailbox
connection settings are Mailbox-scoped). This restores SettingsScreen to
General + Keyboard shortcuts and gives mailboxes their two real homes:
Manage ▾ → Mailboxes (list + "New inbox") and the folder rail's gear →
InboxSettingsShell (per-mailbox Connection section, read-only config via a
new GET /mailboxes/{id}/imap-config engine endpoint). The section menu is a
registry (inbox-settings-sections.ts), not hardcoded JSX, per Rule 2's
module-injection requirement.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`imapConnect` is optional on `InboxApiDeps`, so the dev server served no
`/inbound/imap/{connect,check}` routes and the connect screen 404'd against
it. Wire the real stores and adapters exactly as the composition root does,
plus a console-sender `SenderResolver` so the harness never transmits mail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two of the 2026-07-20 adversarial-review blockers are resolved (self-echo via our own signed Message-ID; the seam, by declining a webhook-shaped interface a cron fetch cannot satisfy). The third — no transport-stable providerMessageId for IMAP — is CONTAINED, not resolved: a UIDVALIDITY reset pauses the mailbox instead of re-ingesting, so nothing duplicates, but the identity problem is still owed an answer. Does not promote the spec to `accepted`; that decision is not made here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-101-imap-smtp-mailbox-connection
branch
from
July 25, 2026 16:33
ac6ee0b to
6443399
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
src/api/conversations.ts (1)
505-521: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed
SenderResolutionError.The 502 body is deliberately generic, and the error carries the only diagnostic (which mailbox, and whether config or credential is missing) — but it's discarded, so a misconfigured inbox produces a silent 502 with nothing in the logs. Other handlers in this API log before returning a shaped error.
🔭 Proposed change
if (err instanceof SenderResolutionError) { // The conversation's inbox can't send as configured (missing IMAP // config/credential, or its mailbox was deleted) — a clean send-domain // error, not an opaque 500. + console.error( + `[handleReply] no usable sender for conversation ${conversation.id}`, + err, + ) return apiError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/conversations.ts` around lines 505 - 521, In the SenderResolutionError branch of the conversation send flow, log the caught error with appropriate mailbox context before returning the existing 502 apiError response. Preserve the generic client-facing message and rethrow behavior for non-SenderResolutionError exceptions.src/mail/delivery-worker.ts (1)
174-196: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider memoizing resolution per mailbox within one sweep.
Each claimed row triggers
getConversationByThreadId+ a freshresolve()(which, for IMAP, is two more store reads plus a brand-new SMTP transport). A batch of retries for the same inbox pays that cost per row. A smallMap<string | null, {sender, from}>local to the sweep would collapse it without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/delivery-worker.ts` around lines 174 - 196, The delivery sweep should memoize sender resolution per mailbox to avoid repeating store reads and SMTP transport creation for rows sharing the same mailbox. Add a sweep-local Map keyed by the conversation mailbox ID (including null), reuse its cached `{sender, from}` in the resolution flow around `deps.senderResolver.resolve`, and preserve the existing `fromAddress` mismatch validation and `assertLeaseExceedsSenderBound` behavior.src/mail/sender-resolver.ts (1)
134-173: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake non-active mailbox resolution impossible-to-misread.
resolveForMailboxonly branches onprovider;disconnected,paused, andneeds_reconnectmailboxes still resolve tocreateGmailEmailSenderorcreateSmtpEmailSender. That is fine if replies should keep going out through configured dead mailboxes until credentials fail, but add a one-line module/docstring note so future devs don’t assumeMailboxRecord.statusis already being enforced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/sender-resolver.ts` around lines 134 - 173, Add a concise module or function docstring near resolveForMailbox explicitly stating that mailbox status values such as disconnected, paused, and needs_reconnect are not enforced during sender resolution; resolution currently branches only on provider and may return configured Gmail or SMTP senders. Do not change the existing provider or credential resolution behavior.src/mail/imap-fetch.ts (1)
331-377: 🚀 Performance & Scalability | 🔵 TrivialPer-mailbox IMAP fetch runs strictly sequentially.
runImapFetchprocesses each active IMAP mailbox one at a time viafor...of+await. With a 2-minute cron cadence, connect+fetch+ingest latency across mailboxes accumulates linearly; as the number of connected IMAP mailboxes grows, a single invocation may not keep up, and leases (a pure efficiency guard, not a correctness one) don't prevent backlog growth. Worth bounding concurrency (e.g. a small worker pool) once mailbox counts are non-trivial — not a blocker for this stage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.ts` around lines 331 - 377, Update runImapFetch to process active IMAP mailboxes with bounded concurrency instead of awaiting each fetchOneMailbox sequentially. Add a small worker-pool or equivalent concurrency-limited scheduling approach while preserving per-mailbox failure isolation, shared counts updates, and the existing ImapFetchReport totals.src/providers/adapters/smtp/sender.ts (1)
121-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
messageIdguard doesn't coverinReplyTo/references, which share the same verbatim contract.
hasControlOrNewlineis only applied toemail.messageId(Line 200), but the module doc's own contract (Lines 11-29) andOutboundEmail's doc (src/providers/email-sender.ts) requireinReplyTo/referencesto be transmitted verbatim too. nodemailer silently strips control/newline chars from those fields the same way it wouldmessageId— the exact silent-mangling failure mode this guard was added to prevent — so a malformedinReplyTo/referencesvalue would be quietly reformatted rather than rejected.🛡️ Proposed fix
if (hasControlOrNewline(email.messageId)) { throw new Error( 'createSmtpEmailSender: messageId contains a control or newline character — refusing to send (this would violate the verbatim Message-ID contract)', ) } + if (email.inReplyTo !== undefined && hasControlOrNewline(email.inReplyTo)) { + throw new Error( + 'createSmtpEmailSender: inReplyTo contains a control or newline character — refusing to send', + ) + } + if (email.references?.some(hasControlOrNewline)) { + throw new Error( + 'createSmtpEmailSender: references contains a control or newline character — refusing to send', + ) + }Also applies to: 195-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/sender.ts` around lines 121 - 127, Extend the validation in the SMTP send path, alongside the existing email.messageId check, to apply hasControlOrNewline to inReplyTo and references as well. Reject any value containing control or newline characters before passing the message to nodemailer, while preserving verbatim transmission for valid values.src/api/imap-connect.ts (1)
88-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo upper bounds on the string fields.
address/imapHost/smtpHost/username/passwordare only checked for non-emptiness, so a multi-megabyte value flows into the connection attempt and then into the encrypted-credential insert, where a column limit failure surfaces as a 500 rather than a 400. A cheaplength <= Ncap inrequireStringkeeps the failure in the validation layer.🛡️ Suggested bound
- const requireString = (field: string): string | null => { - const v = b[field] - return typeof v === 'string' && v.length > 0 ? v : null - } + const MAX_FIELD_LENGTH = 512 + const requireString = (field: string): string | null => { + const v = b[field] + return typeof v === 'string' && v.length > 0 && v.length <= MAX_FIELD_LENGTH ? v : null + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/imap-connect.ts` around lines 88 - 117, Update requireString in the input validation flow to reject strings exceeding the agreed maximum length while preserving the existing non-empty string check. Apply this bound to address, imapHost, smtpHost, username, and password so oversized values are added to problems and rejected before connection or credential persistence.web/src/components/MailboxListScreen.tsx (1)
72-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRow navigation via
<button>+router.pushloses link affordances.Each card is a real navigation to
/mailbox/{id}/settings/connection, so a styledLinkgives middle-click/new-tab, copy-link, prefetch, and correct role for assistive tech at no cost — the file already usesLinkfor the back nav. Same applies to the "New inbox" action at Line 60.♻️ Sketch
- <button + <Link key={mailbox.id} - type="button" - onClick={() => router.push(`/mailbox/${mailbox.id}/settings/connection`)} + href={`/mailbox/${mailbox.id}/settings/connection`} style={{ display: 'flex', + textDecoration: 'none',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/MailboxListScreen.tsx` around lines 72 - 107, Replace the mailbox row button and router.push navigation with a styled Link targeting the existing `/mailbox/${mailbox.id}/settings/connection` route, preserving the current card styling and content. Update the “New inbox” action similarly to use Link, reusing the existing Link import and removing any now-unused router dependency while keeping the back navigation unchanged.src/mail/imap-connect.test.ts (1)
184-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the base64 redaction branch.
sanitizeConnectionErrorredacts both the literal password and its base64 form, but only the literal path is exercised here. A case whose error message embedsBuffer.from(password).toString('base64')would pin the second branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-connect.test.ts` around lines 184 - 197, Add a test alongside the existing literal-password case for the base64 redaction path in sanitizeConnectionError. Create an error whose message includes Buffer.from(VALID_INPUT.password).toString('base64'), run it through createImapConnectService and checkConnection, and assert the result excludes the encoded password and contains “[redacted]”.web/src/components/NewMailboxScreen.tsx (1)
20-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBack affordance could be a
Linkfor navigation semantics.
MailboxListScreenuses<Link href="/inbox/open">for its equivalent back link; a<button>+router.pushloseshref, middle-click, and open-in-new-tab. Fine to leave ifNewAgentScreen(the shape this deliberately mirrors) does the same.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/NewMailboxScreen.tsx` around lines 20 - 34, Replace the back-navigation button in NewMailboxScreen with the established Link pattern used by MailboxListScreen, targeting /manage/mailboxes and preserving the existing visual styling and label so href, middle-click, and new-tab navigation work correctly.src/api/index.test.ts (1)
2947-3018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the third new dispatch branch too.
This block covers
imap-connectandimap-checkwiring, butmailbox-imap-config(GET /api/v1/mailboxes/{id}/imap-config, added insrc/api/index.tslines 608-615) has no wiring test here — notably thedeps.imapConnectabsent → 404 case and the acting-Agent threading, which is wiring-level behavior this suite owns rather than handler-level detail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/index.test.ts` around lines 2947 - 3018, Extend the IMAP wiring tests around apiWithImapConnect to cover GET /api/v1/mailboxes/{id}/imap-config, including a successful dispatch with deps.imapConnect present, propagation of the acting-Agent, and a 404 response when deps.imapConnect is absent. Reuse the existing authentication and fixture helpers, while keeping handler-specific behavior out of this suite.src/api/index.ts (1)
266-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc omits the third route this dep gates.
imapConnectalso gatesGET /api/v1/mailboxes/{id}/imap-config(lines 608-615), which is admin-acting-Agent-gated rather than plain Bearer-gated. Worth naming here so the "both routes 404" wording doesn't read as exhaustive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/index.ts` around lines 266 - 277, The imapConnect dependency documentation only names the connect and check routes, omitting the admin Agent-gated GET /api/v1/mailboxes/{id}/imap-config route. Update the comment near imapConnect to name all three gated routes and clarify that the two POST routes use ordinary Bearer authentication while the mailbox IMAP configuration route uses admin-acting-Agent authorization.web/src/app/(shell)/layout.tsx (1)
29-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize independent fetches in the shared layout.
loadFolderCountsandlistMailboxesdon't depend on each other (only onme), but are awaited sequentially. Since this layout runs on every navigation, running them concurrently would cut latency for admin users.⚡ Proposed fix
- const me = await getMe() - const counts = await loadFolderCounts(me.id) - const supportAddress = process.env.HELPTHREAD_SUPPORT_ADDRESS ?? 'support@dev.localhost' - - const mailboxes = me.role === 'admin' ? await listMailboxes() : [] + const me = await getMe() + const supportAddress = process.env.HELPTHREAD_SUPPORT_ADDRESS ?? 'support@dev.localhost' + + const [counts, mailboxes] = await Promise.all([ + loadFolderCounts(me.id), + me.role === 'admin' ? listMailboxes() : Promise.resolve([]), + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/app/`(shell)/layout.tsx around lines 29 - 33, Update the shared layout’s data-loading flow after getMe so loadFolderCounts and the admin-only listMailboxes call execute concurrently, while preserving the non-admin empty-mailboxes result and both existing returned values.web/src/components/FolderNav.tsx (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame
ChevronDownIconre-implemented three times in this PR. All three new files independently define a near-identical chevron-down SVG (differing only in size/opacity/strokeWidth); extracting a single shared icon component would remove the triplication.
web/src/components/FolderNav.tsx#L75-L88: replace the localChevronDownIconwith an import from a shared icon module (e.g.ds/core/icons), passing size/opacity as props if the visual variance is intentional.web/src/components/InboxSettingsShell.tsx#L38-L51: same — drop the local definition in favor of the shared icon.web/src/components/ConnectInboxForm.tsx#L182-L195: same — drop the local definition in favor of the shared icon.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/FolderNav.tsx` around lines 75 - 88, Replace the local ChevronDownIcon definitions with one shared icon component from the shared icon module, preserving each call site’s intentional size, opacity, and stroke-width differences through supported props. Update web/src/components/FolderNav.tsx:75-88, web/src/components/InboxSettingsShell.tsx:38-51, and web/src/components/ConnectInboxForm.tsx:182-195; remove the duplicated local implementations and import the shared component in each file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/dev-api.ts`:
- Around line 135-149: Update the createInboxApi call in scripts/dev-api.ts to
provide the required assistants, webhooks, and savedReplies dependencies using
the existing stores or providers available in the script. Ensure these
dependencies match InboxApiDeps so Assistant-auth requests can access
deps.assistants.store, and include the script in type-checking if needed to
catch missing wiring.
In `@src/api/assistants.test.ts`:
- Around line 305-307: Update senderResolver.resolve in the failingApi setup to
return the sender injected through deps.sender instead of creating a separate
sender with createFakeSender().sender. Preserve SUPPORT_ADDRESS as the from
value so the failure path exercises the injected transport.
In `@src/api/imap-connect.ts`:
- Around line 200-217: Update handleImapCheck to require the same
acting-Agent/Admin authorization used by the mailbox IMAP configuration endpoint
before invoking deps.service.checkConnection. Validate supplied IMAP/SMTP hosts
and ports against the established outbound allowlists and enforce the existing
rate/timeout protections, rejecting unauthorized or unsafe requests before any
network connection or leg outcome is returned.
In `@src/composition/root.ts`:
- Around line 598-601: Update the summary logging condition in runImapFetch to
also emit when report.skipped is greater than zero. Preserve the existing JSON
summary payload and return report behavior.
In `@src/mail/imap-connect.ts`:
- Around line 337-368: Before calling upsertConnectedMailbox in the transaction,
load the existing mailbox by input.address and reject the connection when it
exists with a provider different from 'imap'. Preserve the current upsert flow
for new mailboxes and existing IMAP mailboxes, requiring an explicit disconnect
before any provider change.
In `@src/providers/adapters/imap/client.ts`:
- Around line 302-311: Make ImapClient.close() in
src/providers/adapters/imap/client.ts lines 302-311 truly non-throwing by
independently guarding the flow.close() fallback after logout fails. In
src/providers/adapters/imap/fetch.ts lines 169-171, leave the finally block
unchanged because the root fix guarantees client.close() cannot reject; no
direct change is required there.
In `@src/providers/adapters/smtp/verify.ts`:
- Around line 78-94: Add the same outer timeout guard used by sender.ts around
transporter.verify() in verifySmtpConnection, using timeoutMs and preserving the
existing transporter creation and error propagation. Ensure the verification
promise settles when Nodemailer’s socket-level timeouts fail to fire, including
when a custom options.transporter is supplied.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 375-405: Update the locked address `<div>` in the `lockAddress`
branch of `ConnectInboxForm` to use the same `id` referenced by `FieldLabel`
(`ht-connect-inbox-address`), preserving the existing TextInput identifier and
address display behavior.
---
Nitpick comments:
In `@src/api/conversations.ts`:
- Around line 505-521: In the SenderResolutionError branch of the conversation
send flow, log the caught error with appropriate mailbox context before
returning the existing 502 apiError response. Preserve the generic client-facing
message and rethrow behavior for non-SenderResolutionError exceptions.
In `@src/api/imap-connect.ts`:
- Around line 88-117: Update requireString in the input validation flow to
reject strings exceeding the agreed maximum length while preserving the existing
non-empty string check. Apply this bound to address, imapHost, smtpHost,
username, and password so oversized values are added to problems and rejected
before connection or credential persistence.
In `@src/api/index.test.ts`:
- Around line 2947-3018: Extend the IMAP wiring tests around apiWithImapConnect
to cover GET /api/v1/mailboxes/{id}/imap-config, including a successful dispatch
with deps.imapConnect present, propagation of the acting-Agent, and a 404
response when deps.imapConnect is absent. Reuse the existing authentication and
fixture helpers, while keeping handler-specific behavior out of this suite.
In `@src/api/index.ts`:
- Around line 266-277: The imapConnect dependency documentation only names the
connect and check routes, omitting the admin Agent-gated GET
/api/v1/mailboxes/{id}/imap-config route. Update the comment near imapConnect to
name all three gated routes and clarify that the two POST routes use ordinary
Bearer authentication while the mailbox IMAP configuration route uses
admin-acting-Agent authorization.
In `@src/mail/delivery-worker.ts`:
- Around line 174-196: The delivery sweep should memoize sender resolution per
mailbox to avoid repeating store reads and SMTP transport creation for rows
sharing the same mailbox. Add a sweep-local Map keyed by the conversation
mailbox ID (including null), reuse its cached `{sender, from}` in the resolution
flow around `deps.senderResolver.resolve`, and preserve the existing
`fromAddress` mismatch validation and `assertLeaseExceedsSenderBound` behavior.
In `@src/mail/imap-connect.test.ts`:
- Around line 184-197: Add a test alongside the existing literal-password case
for the base64 redaction path in sanitizeConnectionError. Create an error whose
message includes Buffer.from(VALID_INPUT.password).toString('base64'), run it
through createImapConnectService and checkConnection, and assert the result
excludes the encoded password and contains “[redacted]”.
In `@src/mail/imap-fetch.ts`:
- Around line 331-377: Update runImapFetch to process active IMAP mailboxes with
bounded concurrency instead of awaiting each fetchOneMailbox sequentially. Add a
small worker-pool or equivalent concurrency-limited scheduling approach while
preserving per-mailbox failure isolation, shared counts updates, and the
existing ImapFetchReport totals.
In `@src/mail/sender-resolver.ts`:
- Around line 134-173: Add a concise module or function docstring near
resolveForMailbox explicitly stating that mailbox status values such as
disconnected, paused, and needs_reconnect are not enforced during sender
resolution; resolution currently branches only on provider and may return
configured Gmail or SMTP senders. Do not change the existing provider or
credential resolution behavior.
In `@src/providers/adapters/smtp/sender.ts`:
- Around line 121-127: Extend the validation in the SMTP send path, alongside
the existing email.messageId check, to apply hasControlOrNewline to inReplyTo
and references as well. Reject any value containing control or newline
characters before passing the message to nodemailer, while preserving verbatim
transmission for valid values.
In `@web/src/app/`(shell)/layout.tsx:
- Around line 29-33: Update the shared layout’s data-loading flow after getMe so
loadFolderCounts and the admin-only listMailboxes call execute concurrently,
while preserving the non-admin empty-mailboxes result and both existing returned
values.
In `@web/src/components/FolderNav.tsx`:
- Around line 75-88: Replace the local ChevronDownIcon definitions with one
shared icon component from the shared icon module, preserving each call site’s
intentional size, opacity, and stroke-width differences through supported props.
Update web/src/components/FolderNav.tsx:75-88,
web/src/components/InboxSettingsShell.tsx:38-51, and
web/src/components/ConnectInboxForm.tsx:182-195; remove the duplicated local
implementations and import the shared component in each file.
In `@web/src/components/MailboxListScreen.tsx`:
- Around line 72-107: Replace the mailbox row button and router.push navigation
with a styled Link targeting the existing
`/mailbox/${mailbox.id}/settings/connection` route, preserving the current card
styling and content. Update the “New inbox” action similarly to use Link,
reusing the existing Link import and removing any now-unused router dependency
while keeping the back navigation unchanged.
In `@web/src/components/NewMailboxScreen.tsx`:
- Around line 20-34: Replace the back-navigation button in NewMailboxScreen with
the established Link pattern used by MailboxListScreen, targeting
/manage/mailboxes and preserving the existing visual styling and label so href,
middle-click, and new-tab navigation work correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63eeb034-9255-4e74-b342-abde3039967b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (68)
package.jsonscripts/dev-api.tsspecs/mail/mailbox-connection.mdsrc/api/agents.test.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/saved-replies.test.tssrc/api/webauthn.test.tssrc/api/webhooks.test.tssrc/composition/app.test.tssrc/composition/app.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/imap-connect.test.tssrc/mail/imap-connect.tssrc/mail/imap-fetch.test.tssrc/mail/imap-fetch.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.test.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/imap/index.tssrc/providers/adapters/smtp/index.tssrc/providers/adapters/smtp/sender.test.tssrc/providers/adapters/smtp/sender.tssrc/providers/adapters/smtp/verify.test.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-config.test.tssrc/store/imap-config.tssrc/store/imap-credentials.test.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tsvercel.jsonweb/src/app/(shell)/layout.tsxweb/src/app/mailbox/[id]/settings/[section]/page.tsxweb/src/app/manage/mailboxes/new/page.tsxweb/src/app/manage/mailboxes/page.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/MailboxConnectionSection.tsxweb/src/components/MailboxListScreen.tsxweb/src/components/NewMailboxScreen.tsxweb/src/components/SettingsScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/inbox-settings-sections.tsweb/src/lib/mailbox-actions.ts
…ating (HT-101) Eleven findings from two independent reviews (CodeRabbit: 8; an adversarial Codex pass: 4, with zero overlap between them). The three that mattered: **Provider conversion double-ingested every message.** `upsertConnectedMailbox` rewrote `provider` on conflict, so connecting an already-Gmail address over IMAP flipped the row while its OAuth token and Gmail cursor stayed behind. The mailbox then satisfied both intake paths — `runImapFetch` selects on `provider`, `runGmailReconcileSweep` on `status` alone — and the two transports mint different `providerMessageId` values for one physical message, so the ledger could not dedupe across them. Now refused atomically (a WHERE on the DO UPDATE, not a read-then-write) as 409 `provider_conflict`. A test asserted the old behaviour as intended; it is replaced. **A stale lease holder could escape the UIDVALIDITY-reset quarantine.** The cursor write was unconditional, so a run whose lease expired mid-ingest could overwrite the state of a mailbox a successor had already paused. `setCursor` is now fenced on the lease. The token itself was the weakness: it was `claimed_until`, a timestamp, and two claims in one clock tick mint identical tokens — the test written to prove the fence instead proved it did not hold. `lease_token` is now a per-claim uuid. Migration 027 is amended in place; it has never run against production. **Both IMAP endpoints dialed operator-supplied hosts on the service Bearer alone.** No acting Agent was required, so any API-token holder could use `/imap/check` as a network probe. Both now require an admin Agent, checked before the body is parsed. This is authorization only — outbound host/port allowlisting is filed separately. Also: an outer timeout on `verifySmtpConnection` (it runs in the HTTP request path, where a hang holds the request open); `connect()` moved inside the try/finally so a rejected login cannot leak its socket; `close()` made truly throw-safe; `skipped` added to the sweep's log gate so a stuck lease is not a silent intake outage; `conversations.mailbox_id` changed to ON DELETE RESTRICT so deleting a mailbox cannot silently re-send its replies from another address; a label/control association fixed in the reconnect flow; and `scripts/**` added to tsconfig — the dev harness was missing three required deps precisely because nothing type-checked it, and including it immediately surfaced a second error. Deferred, filed as follow-ups: the §5 invocation clock budget, and the same timestamp-token weakness in `gmail_watch_state` (lower blast radius there — it guards only lease release, never a cursor advance). Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0; 87 files / 1716 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 25, 2026
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
src/api/index.test.ts (1)
3047-3056: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis password-echo test never reaches a 200 body.
Both requests use
post(...)without the acting-Agent header, sorequireAdminshort-circuits and the assertions run against 401 envelopes — the test would still pass if a successful connect/check response echoedpassword. UsepostAsAgentso the real success bodies are checked.♻️ Proposed fix
it('never echoes the password anywhere in either response body', async () => { const { db } = await freshApi() const api = apiWithImapConnect(db, fakeImapConnect()) + const agentId = await adminAgentId(db) - const connectRes = await api(post(CONNECT_PATH, VALID_BODY)) - const checkRes = await api(post(CHECK_PATH, VALID_BODY)) + const connectRes = await api(postAsAgent(CONNECT_PATH, VALID_BODY, agentId)) + const checkRes = await api(postAsAgent(CHECK_PATH, VALID_BODY, agentId)) expect(await connectRes.text()).not.toContain(VALID_BODY.password) expect(await checkRes.text()).not.toContain(VALID_BODY.password) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/index.test.ts` around lines 3047 - 3056, Update the password-echo test around the “never echoes the password anywhere in either response body” case to send both CONNECT_PATH and CHECK_PATH requests through postAsAgent instead of post, preserving the existing assertions against the successful response bodies.src/store/mailboxes.test.ts (1)
384-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
statustoo, since the test claims the row is untouched.
statusis selected but never asserted; the refusal should leave it atactive.♻️ Proposed tweak
expect(rows).toHaveLength(1) expect(rows[0].provider).toBe('gmail') + expect(rows[0].status).toBe('active')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/mailboxes.test.ts` around lines 384 - 402, Update the provider-conflict test for upsertConnectedMailbox to assert that the unchanged row’s status is also active, using the already selected rows[0].status value alongside the existing provider assertion.src/mail/delivery-worker.test.ts (1)
496-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the rethrow branch too.
The suite covers
SenderResolutionErrorisolation but not the documented counterpart: a non-SenderResolutionErrorfromresolve(or fromassertLeaseExceedsSenderBound) must abort the sweep rather than be swallowed as a per-row failure. A resolver fake that throws a plainErrorwould pin that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/delivery-worker.test.ts` around lines 496 - 550, Add a test alongside the existing SenderResolutionError isolation case that makes the senderResolver.resolve implementation throw a plain Error for a claimed candidate, then assert runDeliveryWorker rethrows that error and does not continue processing later candidates. Cover the documented rethrow behavior for non-SenderResolutionError failures, including errors from assertLeaseExceedsSenderBound if the existing test setup exposes that path.web/src/app/(shell)/layout.tsx (1)
29-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the two independent server-side fetches.
loadFolderCounts(me.id)andlistMailboxes()both depend only onme, not on each other, but are awaited sequentially. Since this layout wraps every page under(shell), the extra round trip adds latency on every navigation.⚡ Proposed fix
- const me = await getMe() - const counts = await loadFolderCounts(me.id) - const supportAddress = process.env.HELPTHREAD_SUPPORT_ADDRESS ?? 'support@dev.localhost' - - const mailboxes = me.role === 'admin' ? await listMailboxes() : [] + const me = await getMe() + const supportAddress = process.env.HELPTHREAD_SUPPORT_ADDRESS ?? 'support@dev.localhost' + + const [counts, mailboxes] = await Promise.all([ + loadFolderCounts(me.id), + me.role === 'admin' ? listMailboxes() : Promise.resolve([]), + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/app/`(shell)/layout.tsx around lines 29 - 39, Update the layout’s server-side loading flow around loadFolderCounts and listMailboxes to start both independent fetches concurrently after getMe resolves, then await their results together. Preserve the admin-only behavior for listMailboxes and the existing resolvedMailbox/mailbox selection logic.src/mail/imap-fetch.test.ts (1)
489-514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the seeded mailbox id over
listActiveMailboxes()[0].
seedImapMailboxalready returns the id; re-deriving it through a list query couples the test to list ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.test.ts` around lines 489 - 514, Use the mailbox ID returned by seedImapMailbox when calling claimFetchLease, instead of retrieving the first mailbox from listActiveMailboxes. Keep the lease and fetch assertions unchanged.src/mail/ingest.ts (1)
643-653: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo adjacent
stringpositional params (deliveryId,mailboxId) are easy to transpose.Both are ids of the same type with no compile-time protection at the call site. An options object would make a future swap impossible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/ingest.ts` around lines 643 - 653, Update storeAndMarkDelivered to accept deliveryId and mailboxId through a named options object instead of adjacent positional string parameters, then update every call site to pass the corresponding properties explicitly and preserve the existing behavior.src/mail/imap-fetch.ts (2)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedaction rewraps the error and drops the original stack/cause.
throw new Error(redacted)loses the stack and anycause, which is the only diagnostic left after the message is scrubbed. Pass{ cause: err }.♻️ Suggested change
- throw new Error(password !== null ? message.split(password).join('[redacted]') : message) + const redacted = password !== null ? message.split(password).join('[redacted]') : message + throw new Error(redacted, { cause: err })Note the
causeis not logged byrunImapFetch(onlyerr.message), so the secret still never reaches logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.ts` around lines 316 - 322, Update the error rethrow in the catch block of the IMAP fetch flow to preserve the original error as the new Error’s cause by passing err in the Error options. Keep the existing password redaction applied to the message and preserve the current behavior for non-Error thrown values.
379-393: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo per-invocation time budget across the mailbox loop.
Mailboxes are processed sequentially with no deadline; with N IMAP mailboxes each able to burn the client's socket timeouts, one tick can exceed the serverless invocation limit and be killed mid-batch (leases then linger until
DEFAULT_IMAP_FETCH_LEASE_MS).../providers/adapters/imap/fetch.tsdefers the "remaining invocation budget" scheme to "Stage 2/3 cron-wiring" — this is that wiring. A simple wall-clock check that stops starting new mailboxes past a deadline would bound it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.ts` around lines 379 - 393, Update the mailbox loop around fetchOneMailbox to enforce a per-invocation wall-clock deadline: compute the deadline before iteration from the configured invocation budget, check it before starting each mailbox, and stop launching additional mailboxes once the deadline is reached. Preserve existing per-mailbox error isolation and count/logging for mailboxes that have already started.src/providers/adapters/imap/fetch.ts (1)
174-176: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
close()in thefinallycan mask the real failure.If
connect()/uidFetchRawSince()throws andclose()also rejects (common when the socket is already broken), the close error replaces the original — which is whatimap-fetch.tsthen logs. Consider swallowing/logging the close failure instead.♻️ Suggested change
} finally { - await client.close() + try { + await client.close() + } catch { + // Never let a close failure mask the real fetch error. + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/imap/fetch.ts` around lines 174 - 176, Update the finally cleanup around client.close in the fetch flow so a close rejection is handled without replacing an error from connect or uidFetchRawSince. Swallow or log the cleanup failure while preserving propagation of the original operation error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/mail/mailbox-connection.md`:
- Around line 64-66: Update §7 in the mailbox connection specification to
describe building against the data-contract adapter model rather than the
declined InboundEmailProvider seam. Keep the corrected-status statement only if
§7’s instructions are brought into alignment; otherwise remove that claim.
In `@src/db/migrate.ts`:
- Around line 1597-1601: Update the migration documentation paragraph near the
foreign-key definition to describe the actual ON DELETE RESTRICT behavior,
removing the incorrect SET NULL claim while preserving the intended policy
explanation.
In `@src/mail/imap-fetch.test.ts`:
- Around line 159-179: Correct the empty-batch test around runImapFetch so its
title matches the actual no-advance behavior, or preferably capture the
mailboxId returned by seedImapMailbox and assert the stored cursor remains {
uidValidity: 100, lastUid: 10 }. Keep the existing fetched, messagesIngested,
and ingest assertions unchanged.
In `@src/providers/adapters/imap/fetch.ts`:
- Around line 55-64: Update the module documentation in the bounding section to
accurately describe ImapClient.uidFetchRawSince: maxPerInvocation is enforced by
limiting the message count through SEARCH, not by applying a UID range upper
bound or fetching then discarding. Remove the stale UID-range claim while
preserving the Stage 2/3 timeout-scope explanation.
In `@src/providers/adapters/smtp/sender.test.ts`:
- Around line 91-103: Remove the empty-string entry from the parametrized
badChar cases in the sender validation test, keeping only actual control or
newline characters so every iteration constructs a malformed messageId and
preserves the expected rejection assertion.
In `@src/store/conversations.ts`:
- Around line 363-371: The conversation mailbox documentation incorrectly
describes ON DELETE SET NULL; update conversations.ts around the mailboxId
documentation to state that null means a pre-028 row or no mailbox at ingest and
that the foreign key uses RESTRICT. In src/mail/sender-resolver.ts lines 17-27,
remove the hard-deleted-mailbox explanation from the null semantics. In
src/mail/delivery-worker.ts lines 178-188, describe the from/transport guard as
applying to pre-2b-i rows whose persisted fromAddress differs from the default
mailbox.
In `@src/store/imap-watch-state.ts`:
- Around line 34-68: Update the module documentation to match the current
implementation: distinguish seedBaseline from the lease-fenced setCursor SQL,
remove the outdated claimed_until text/precision and Gmail parity rationale, and
document the UUID lease token generated and checked by
claimFetchLease/releaseFetchLease. Revise the releaseFetchLease documentation
and the setCursor fence comment to identify lease_token—not claimed_until—as the
release and update fence.
In `@web/src/components/TopBar.tsx`:
- Around line 17-25: Replace the remaining Team/“Team management” terminology in
the TopBar documentation and corresponding menu label with Agents/“Agent
management,” preserving the existing menu structure and behavior.
In `@web/src/lib/mailbox-actions.ts`:
- Around line 42-69: Update checkMailboxConnection and connectMailbox to require
an authenticated user with the required admin role via getMe() before calling
imapCheckConnection or imapConnect; reject unauthorized Bearer-only requests and
ensure mailbox credentials are persisted only after authorization succeeds.
---
Nitpick comments:
In `@src/api/index.test.ts`:
- Around line 3047-3056: Update the password-echo test around the “never echoes
the password anywhere in either response body” case to send both CONNECT_PATH
and CHECK_PATH requests through postAsAgent instead of post, preserving the
existing assertions against the successful response bodies.
In `@src/mail/delivery-worker.test.ts`:
- Around line 496-550: Add a test alongside the existing SenderResolutionError
isolation case that makes the senderResolver.resolve implementation throw a
plain Error for a claimed candidate, then assert runDeliveryWorker rethrows that
error and does not continue processing later candidates. Cover the documented
rethrow behavior for non-SenderResolutionError failures, including errors from
assertLeaseExceedsSenderBound if the existing test setup exposes that path.
In `@src/mail/imap-fetch.test.ts`:
- Around line 489-514: Use the mailbox ID returned by seedImapMailbox when
calling claimFetchLease, instead of retrieving the first mailbox from
listActiveMailboxes. Keep the lease and fetch assertions unchanged.
In `@src/mail/imap-fetch.ts`:
- Around line 316-322: Update the error rethrow in the catch block of the IMAP
fetch flow to preserve the original error as the new Error’s cause by passing
err in the Error options. Keep the existing password redaction applied to the
message and preserve the current behavior for non-Error thrown values.
- Around line 379-393: Update the mailbox loop around fetchOneMailbox to enforce
a per-invocation wall-clock deadline: compute the deadline before iteration from
the configured invocation budget, check it before starting each mailbox, and
stop launching additional mailboxes once the deadline is reached. Preserve
existing per-mailbox error isolation and count/logging for mailboxes that have
already started.
In `@src/mail/ingest.ts`:
- Around line 643-653: Update storeAndMarkDelivered to accept deliveryId and
mailboxId through a named options object instead of adjacent positional string
parameters, then update every call site to pass the corresponding properties
explicitly and preserve the existing behavior.
In `@src/providers/adapters/imap/fetch.ts`:
- Around line 174-176: Update the finally cleanup around client.close in the
fetch flow so a close rejection is handled without replacing an error from
connect or uidFetchRawSince. Swallow or log the cleanup failure while preserving
propagation of the original operation error.
In `@src/store/mailboxes.test.ts`:
- Around line 384-402: Update the provider-conflict test for
upsertConnectedMailbox to assert that the unchanged row’s status is also active,
using the already selected rows[0].status value alongside the existing provider
assertion.
In `@web/src/app/`(shell)/layout.tsx:
- Around line 29-39: Update the layout’s server-side loading flow around
loadFolderCounts and listMailboxes to start both independent fetches
concurrently after getMe resolves, then await their results together. Preserve
the admin-only behavior for listMailboxes and the existing
resolvedMailbox/mailbox selection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f492f9b-9078-4536-9d52-7dccc64419a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonscripts/dev-api.tsspecs/mail/mailbox-connection.mdsrc/api/agents.test.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/saved-replies.test.tssrc/api/webauthn.test.tssrc/api/webhooks.test.tssrc/composition/app.test.tssrc/composition/app.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/imap-connect.test.tssrc/mail/imap-connect.tssrc/mail/imap-fetch.test.tssrc/mail/imap-fetch.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.test.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/imap/index.tssrc/providers/adapters/smtp/index.tssrc/providers/adapters/smtp/sender.test.tssrc/providers/adapters/smtp/sender.tssrc/providers/adapters/smtp/verify.test.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-config.test.tssrc/store/imap-config.tssrc/store/imap-credentials.test.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tstsconfig.jsonvercel.jsonweb/src/app/(shell)/layout.tsxweb/src/app/mailbox/[id]/settings/[section]/page.tsxweb/src/app/manage/mailboxes/new/page.tsxweb/src/app/manage/mailboxes/page.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/MailboxConnectionSection.tsxweb/src/components/MailboxListScreen.tsxweb/src/components/NewMailboxScreen.tsxweb/src/components/SettingsScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/inbox-settings-sections.tsweb/src/lib/mailbox-actions.ts
Comment on lines
+17
to
+25
| * whole trigger, not just its entries (a non-admin reaches per-mailbox | ||
| * settings via the folder rail's gear icon instead). It carries Settings, | ||
| * Team (Agents), and Mailboxes (HT-101: the connected-mailbox LIST — | ||
| * per-mailbox connection settings are Mailbox-scoped instead, the folder | ||
| * rail's gear, never here — specs/ui/admin-ia.md §1). Keyboard shortcuts | ||
| * lives under Settings now (`SettingsScreen`), not here. The avatar menu is | ||
| * **personal scope, and personal scope only** — Your Profile and Log out, | ||
| * nothing else, ever (the first draft wrongly hung Team management off it; | ||
| * that was the defect this correction fixes). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use “Agents” consistently.
Replace the Team/“Team management” terminology here and in the corresponding menu label with Agents/“Agent management.” As per coding guidelines, “call human support staff Agents … never conflate them.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/components/TopBar.tsx` around lines 17 - 25, Replace the remaining
Team/“Team management” terminology in the TopBar documentation and corresponding
menu label with Agents/“Agent management,” preserving the existing menu
structure and behavior.
Source: Coding guidelines
…e docs (HT-101) CodeRabbit's re-review of the fix commit: 9 findings, 8 real. **The connect UI was broken by the previous commit.** Admin-gating the two IMAP endpoints was right, but `web/src/lib/api.ts` sent neither call with the acting-Agent header, so the browser flow would 401 on every attempt. The sibling read (`getMailboxImapConfig`) already sent it; these two did not. The live smoke test could not catch this because it drove the service directly and never went through HTTP. **Six doc comments described the pre-fix design.** Changing the FK to RESTRICT and the lease token to a uuid left `imap-watch-state.ts`'s module doc, `conversations.ts`, `sender-resolver.ts`, migration 028's comment, the spec's §7 build order, and `imap/fetch.ts`'s bounding note all asserting things that are no longer true. On a path whose docs are this load-bearing, stale is worse than absent — each now describes what ships, and says what it used to claim. Also: a test titled "still advances the cursor" asserted no such thing and got no such advance (an empty batch deliberately does not move the watermark) — it now asserts the cursor stays put, and is retitled. **One finding rejected.** CodeRabbit read the fourth case of the Message-ID guard's parametrized list as an empty string that should fail the assertion. It is a literal BEL control character in the source, which renders as nothing — the test passes and always did. The literal is now written as a `` escape so it stops misleading readers; the tooling refused to accept an edit command containing the raw character, which rather made the point. **One rejected as wrong for this repo.** `Team` to `Agents` in `TopBar.tsx`: `specs/ui/admin-ia.md` §3 already records Agents/Team as a deliberate deviation, the file is untouched by this PR, and the vocabulary is under active revision in the other direction. Gates: typecheck, web typecheck, lint all exit 0; 87 files / 1716 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/TopBar.tsx (1)
249-256: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Teamlabel still not renamed toAgents.The doc comment was updated to "Team (Agents)", but the visible
MenuItemlabel is still literallyTeameven though it routes to/manage/agents. As per coding guidelines, "call human support staffAgents... never conflate them in schemas, code, documentation, or prose" — the UI text should sayAgents(or "Agent management"), matching a prior review comment on this exact spot that wasn't fully applied.📝 Proposed fix
<MenuItem onClick={() => { setOpenMenu(null) router.push('/manage/agents') }} > - Team + Agents </MenuItem>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/TopBar.tsx` around lines 249 - 256, The MenuItem in the TopBar navigation still displays “Team” despite linking to agent management. Update the visible label in this MenuItem to “Agents” (or “Agent management”), while preserving its existing setOpenMenu and router.push behavior.Source: Coding guidelines
🧹 Nitpick comments (8)
web/src/components/InboxSettingsShell.tsx (1)
21-26: 📐 Maintainability & Code Quality | 🔵 TrivialGet the maintainer's sign-off on this header treatment before merge.
The doc block records a deliberate departure (address as the bold header line, connection status in the mono line) that hasn't been signed off. As per coding guidelines, "The Agent Inbox UI and dogfood site must match the Claude Design prototype exactly across the whole designed surface, including visual details, copy, and interactions; deviations require the maintainer's explicit sign-off."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/InboxSettingsShell.tsx` around lines 21 - 26, Obtain the maintainer’s explicit sign-off for the mailbox header treatment documented in InboxSettingsShell.tsx before merging. Confirm approval for using the mailbox address as the bold header and connection status as the muted-mono line, or revise the implementation to match the Claude Design prototype if approval is not granted.Source: Coding guidelines
src/providers/adapters/smtp/sender.ts (1)
171-187: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse or close SMTP transporters
createSenderResolvercallscreateSmtpEmailSender()insideresolve(), and that factory allocates a fresh nodemailer SMTP transport on each path that does not supply an injectedtransporter. Nodemailer manages idle SMTP connections internally, but at scale reusing one transporter per resolved sender configuration or exposing an adapter lifecycleclose()is preferable to creating many short-lived transports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/sender.ts` around lines 171 - 187, Update createSenderResolver and createSmtpEmailSender so SMTP transporters are reused per resolved sender configuration instead of creating a new nodemailer transport on every resolve() call; preserve injected transporter behavior and ensure any adapter lifecycle close() path closes transports it owns.web/src/components/FolderNav.tsx (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFourth copy of an identical
ChevronDownIcon.The same inline
ChevronDownIconimplementation already exists inConnectInboxForm.tsx,InboxSettingsShell.tsx, andTopBar.tsx. Adding a fourth identical copy here compounds the duplication; extracting a shared icon component would keep them in sync automatically.♻️ Proposed extraction
-function ChevronDownIcon() { - return ( - <svg width="9" height="9" viewBox="0 0 24 24" aria-hidden="true" style={{ opacity: 0.75 }}> - <polyline - points="6 9 12 15 18 9" - fill="none" - stroke="currentColor" - strokeWidth="3" - strokeLinecap="round" - strokeLinejoin="round" - /> - </svg> - ) -} +import { ChevronDownIcon } from './ds/icons/ChevronDownIcon'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/FolderNav.tsx` around lines 75 - 88, Remove the local ChevronDownIcon definition from FolderNav.tsx and reuse a shared ChevronDownIcon component extracted from the existing identical implementations in ConnectInboxForm.tsx, InboxSettingsShell.tsx, and TopBar.tsx. Update those consumers to import the shared component while preserving the current SVG appearance and behavior.src/mail/imap-connect.ts (1)
312-322: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the two
checkConnectionlegs concurrently.Both legs always run and neither depends on the other, but they're awaited sequentially — with the adapter's 30s default
timeoutMson each, a fully unreachable host/port pair makes this request hang for ~60s before responding.Promise.allhalves the worst case and doesn't change the "both reported independently" contract (neither attempt function throws).♻️ Proposed change
- const imapAttempt = await attemptImapConnection(input, createImapClient) - const smtp = await attemptSmtpVerification(input, verifySmtp) + const [imapAttempt, smtp] = await Promise.all([ + attemptImapConnection(input, createImapClient), + attemptSmtpVerification(input, verifySmtp), + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-connect.ts` around lines 312 - 322, Update checkConnection to start attemptImapConnection and attemptSmtpVerification concurrently and await both results together with Promise.all. Preserve the existing independent result mapping, including conversion of imapAttempt into the returned imap LegResult and returning both imap and smtp outcomes.src/mail/imap-connect.test.ts (1)
202-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe new
provider_conflictpath is untested above the store layer.MailboxProviderConflictError(new insrc/store/mailboxes.ts) is translated toImapConnectError('provider_conflict')in the service and then to a 409 in the handler, but only the store-level guard has tests.
src/mail/imap-connect.test.ts#L202-L333: add aconnecttest that pre-seeds agmailmailbox row forVALID_INPUT.address, assertingImapConnectErrorwithcode: 'provider_conflict'and that nothing else was persisted.src/api/imap-connect.test.ts#L153-L168: add a case whereconnectthrowsImapConnectError('provider_conflict', ...)and assert the handler returns 409, not 422.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-connect.test.ts` around lines 202 - 333, The provider-conflict path lacks service- and handler-level coverage. In src/mail/imap-connect.test.ts lines 202-333, add a connect test that pre-seeds a gmail mailbox for VALID_INPUT.address, expects ImapConnectError with code provider_conflict, and verifies no additional configuration, credential, cursor, or mailbox data is persisted. In src/api/imap-connect.test.ts lines 153-168, add a case where connect throws ImapConnectError with provider_conflict and assert the handler responds with HTTP 409 rather than 422.src/api/imap-connect.ts (1)
283-316: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueWrap
handleGetMailboxImapConfigin a try/catch.A rejected
getMailboxById/getConfigis converted to a 500 by the dispatcher but is not logged or styled as a module-level 500 like the IMAP connect/check handlers. Add the same wrapper pattern here so store failures do not bypass this module’s error semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/imap-connect.ts` around lines 283 - 316, Wrap the body of handleGetMailboxImapConfig in the same try/catch pattern used by the module’s IMAP connect/check handlers. Keep the existing authorization, mailbox/config lookup, and success responses unchanged; catch rejected store operations from getMailboxById or getConfig and return the module-standard logged 500 error response.src/providers/adapters/imap/client.ts (1)
175-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the
new ImapFlow(...)expression structurally checked againstImapFlowLike.This is the only actual
imapflowinstantiation, andsearch/fetchoverloads can change across upgrades; theas unknown as ImapFlowLikebypasses the static adapter boundary. Use a narrowed factory return instead, e.g.as ImapFlowLike satisfies ImapFlowLike, or keep the realImapFlowreturn type and bridge from there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/imap/client.ts` around lines 175 - 197, Update defaultCreateFlow’s ImapFlow construction so the adapter boundary remains structurally type-checked against ImapFlowLike. Remove the as unknown as ImapFlowLike cast and use a direct satisfies-based narrowing or retain the concrete ImapFlow return type before safely adapting it to ImapFlowLike, preserving the existing options and behavior.src/mail/imap-fetch.ts (1)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the original error as
causewhen re-throwing the redacted message.The rewrap drops the stack and any structured fields of the underlying failure, leaving only a redacted string for the outer catch to log.
♻️ Keep the cause chain
const message = err instanceof Error ? err.message : String(err) - throw new Error(password !== null ? message.split(password).join('[redacted]') : message) + throw new Error(password !== null ? message.split(password).join('[redacted]') : message, { + cause: err, + })Note the outer handler only logs
err.message, so the cause stays out of the logs — no new leak path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.ts` around lines 316 - 322, Update the catch block that rethrows the redacted message to preserve the original err as the new Error’s cause. Keep the existing password redaction behavior and message selection unchanged, while retaining the underlying error for stack and structured failure details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/index.test.ts`:
- Around line 3047-3056: The password-echo test does not authenticate requests,
so it only validates unauthorized 401 responses. Update the test around
CONNECT_PATH and CHECK_PATH to use an acting admin Agent or equivalent
authenticated request setup, while preserving the existing assertions that
neither successful response body contains VALID_BODY.password.
In `@src/mail/delivery-worker.ts`:
- Around line 178-194: Update the comment above the resolved.from mismatch guard
in the delivery-worker flow to match migration 028’s ON DELETE RESTRICT
behavior: remove the unreachable hard-delete/NULL mailbox scenario and describe
only valid causes of a persisted fromAddress versus resolved transport mismatch,
while preserving the guard and its refusal to retry.
In `@src/mail/sender-resolver.ts`:
- Around line 151-162: Update the IMAP branch in the sender-resolution flow to
catch rejections from imapCredentialStore.getPassword and convert them into a
per-mailbox SenderResolutionError, preserving the existing missing-configuration
handling and error code contract. Extend SenderResolutionError’s constructor
only as needed to pass through the original failure as an optional cause, or
omit the cause if its API does not support one, so delivery-worker handling does
not rethrow the decrypt error and abort the sweep.
In `@src/providers/adapters/imap/fetch.ts`:
- Around line 179-181: Update the finally cleanup around client.close() so close
failures are swallowed rather than propagated, preserving any original fetch or
AUTH error and allowing successful fetches to resolve normally. Keep the client
cleanup attempt in place without changing the surrounding fetch behavior.
In `@src/store/mailboxes.ts`:
- Around line 319-339: Update the mailbox disconnect flow, centered on
markDisconnected, so disconnecting clears or resets the stored provider
alongside its Gmail tokens/watch state; then adjust upsertConnectedMailbox and
its SACRED invariant to allow reconnection with a different provider. Preserve
the existing provider-conflict behavior for still-connected mailboxes.
In `@web/src/components/InboxSettingsShell.tsx`:
- Around line 89-119: Update the mailbox switcher button in InboxSettingsShell
to include aria-haspopup identifying the menu and aria-expanded bound to the
setSwitcherOpen state, so assistive technology reflects whether the switcher is
open.
---
Outside diff comments:
In `@web/src/components/TopBar.tsx`:
- Around line 249-256: The MenuItem in the TopBar navigation still displays
“Team” despite linking to agent management. Update the visible label in this
MenuItem to “Agents” (or “Agent management”), while preserving its existing
setOpenMenu and router.push behavior.
---
Nitpick comments:
In `@src/api/imap-connect.ts`:
- Around line 283-316: Wrap the body of handleGetMailboxImapConfig in the same
try/catch pattern used by the module’s IMAP connect/check handlers. Keep the
existing authorization, mailbox/config lookup, and success responses unchanged;
catch rejected store operations from getMailboxById or getConfig and return the
module-standard logged 500 error response.
In `@src/mail/imap-connect.test.ts`:
- Around line 202-333: The provider-conflict path lacks service- and
handler-level coverage. In src/mail/imap-connect.test.ts lines 202-333, add a
connect test that pre-seeds a gmail mailbox for VALID_INPUT.address, expects
ImapConnectError with code provider_conflict, and verifies no additional
configuration, credential, cursor, or mailbox data is persisted. In
src/api/imap-connect.test.ts lines 153-168, add a case where connect throws
ImapConnectError with provider_conflict and assert the handler responds with
HTTP 409 rather than 422.
In `@src/mail/imap-connect.ts`:
- Around line 312-322: Update checkConnection to start attemptImapConnection and
attemptSmtpVerification concurrently and await both results together with
Promise.all. Preserve the existing independent result mapping, including
conversion of imapAttempt into the returned imap LegResult and returning both
imap and smtp outcomes.
In `@src/mail/imap-fetch.ts`:
- Around line 316-322: Update the catch block that rethrows the redacted message
to preserve the original err as the new Error’s cause. Keep the existing
password redaction behavior and message selection unchanged, while retaining the
underlying error for stack and structured failure details.
In `@src/providers/adapters/imap/client.ts`:
- Around line 175-197: Update defaultCreateFlow’s ImapFlow construction so the
adapter boundary remains structurally type-checked against ImapFlowLike. Remove
the as unknown as ImapFlowLike cast and use a direct satisfies-based narrowing
or retain the concrete ImapFlow return type before safely adapting it to
ImapFlowLike, preserving the existing options and behavior.
In `@src/providers/adapters/smtp/sender.ts`:
- Around line 171-187: Update createSenderResolver and createSmtpEmailSender so
SMTP transporters are reused per resolved sender configuration instead of
creating a new nodemailer transport on every resolve() call; preserve injected
transporter behavior and ensure any adapter lifecycle close() path closes
transports it owns.
In `@web/src/components/FolderNav.tsx`:
- Around line 75-88: Remove the local ChevronDownIcon definition from
FolderNav.tsx and reuse a shared ChevronDownIcon component extracted from the
existing identical implementations in ConnectInboxForm.tsx,
InboxSettingsShell.tsx, and TopBar.tsx. Update those consumers to import the
shared component while preserving the current SVG appearance and behavior.
In `@web/src/components/InboxSettingsShell.tsx`:
- Around line 21-26: Obtain the maintainer’s explicit sign-off for the mailbox header
treatment documented in InboxSettingsShell.tsx before merging. Confirm approval
for using the mailbox address as the bold header and connection status as the
muted-mono line, or revise the implementation to match the Claude Design
prototype if approval is not granted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d80fd64-1012-42d3-b49d-cd63bed172e6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonscripts/dev-api.tsspecs/mail/mailbox-connection.mdsrc/api/agents.test.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/saved-replies.test.tssrc/api/webauthn.test.tssrc/api/webhooks.test.tssrc/composition/app.test.tssrc/composition/app.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/imap-connect.test.tssrc/mail/imap-connect.tssrc/mail/imap-fetch.test.tssrc/mail/imap-fetch.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.test.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/imap/index.tssrc/providers/adapters/smtp/index.tssrc/providers/adapters/smtp/sender.test.tssrc/providers/adapters/smtp/sender.tssrc/providers/adapters/smtp/verify.test.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-config.test.tssrc/store/imap-config.tssrc/store/imap-credentials.test.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tstsconfig.jsonvercel.jsonweb/src/app/(shell)/layout.tsxweb/src/app/mailbox/[id]/settings/[section]/page.tsxweb/src/app/manage/mailboxes/new/page.tsxweb/src/app/manage/mailboxes/page.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/MailboxConnectionSection.tsxweb/src/components/MailboxListScreen.tsxweb/src/components/NewMailboxScreen.tsxweb/src/components/SettingsScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/inbox-settings-sections.tsweb/src/lib/mailbox-actions.ts
…rt, a vacuous test (HT-101) CodeRabbit's re-review of the previous commit: 16 findings, 3 stale re-reports, 1 rejected, 4 real and fixed here. **"Disconnect it first" was a lie.** The provider-conflict guard added last commit told the operator to disconnect and retry. `markDisconnected` only sets `status` and leaves `provider` untouched, so the retry hits the same conflict forever — and disconnect does not remove the old transport's sidecar rows either, so even if the predicate allowed it, the stale-sidecar double-intake the guard exists to prevent would come straight back. The message now states plainly that changing a connected inbox's transport is unsupported, rather than sending someone round a loop that cannot terminate. Supporting it means deciding what happens to the old transport's tokens, cursor, and in-flight mail — a product decision, deliberately not inferred here. **One undecryptable credential aborted every mailbox's outbound retries.** `ImapCredentialStore.getPassword` throws rather than returning null on a wrong `HELPTHREAD_TOKEN_ENC_KEY` or tampered ciphertext, and `runDeliveryWorker` rethrows anything that is not a `SenderResolutionError`, ending the whole sweep. Contained as a per-row `unreadable-imap-credential` so the blast radius is the one mailbox, restoring the worker's "one misconfigured mailbox fails only its own rows" contract. **The password-echo test was passing vacuously.** Admin-gating the routes last commit made both of its calls 401, so it asserted only that a 401 body does not contain the password. A handler echoing the password in a 200 would not have been caught. It now acts as an admin AND asserts 200 first, so it cannot go vacuous again without failing; a sibling test covers the 4xx body an operator actually sees. Also: `delivery-worker.ts`'s From/transport guard justified itself with `ON DELETE SET NULL`, which migration 028 no longer uses. The guard still earns its place for the cases that ARE reachable — a pre-028 conversation resolving to a deployment default that differs from the original send, or that default being reconfigured between attempts — so the rationale is corrected rather than the guard removed. And the inbox switcher gains `aria-haspopup`/`aria-expanded`. Rejected: `Team` to `Agents` in `TopBar.tsx`. `specs/ui/admin-ia.md` §3 records Agents/Team as a deliberate deviation, the file is untouched by this PR, and the vocabulary is under active revision in the other direction. Stale re-reports, already fixed in the previous commit: the `verifySmtpConnection` outer timeout and `ImapClient.close()` throw-safety. The latter also moots "a throwing close() masks the real failure" — close() can no longer throw. Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0; 87 files / 1717 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
src/mail/sender-resolver.test.ts (1)
180-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
unreadable-imap-credentialerror path.The test suite covers
missing-imap-connection(no config, no credential) but does not exerciseunreadable-imap-credential. That code path was added specifically to fix the previously flagged issue where a raw decrypt failure escapedsender-resolver.tsand aborted the whole delivery sweep. Add a test that stores a credential with one encryption key, then resolves through a store built with a different key, and asserts the error'scodeis'unreadable-imap-credential'.🧪 Proposed test
+ it('an imap mailbox with a credential encrypted under a DIFFERENT key throws SenderResolutionError (unreadable-imap-credential)', async () => { + const { mailboxStore, imapConfigStore, imapCredentialStore } = await freshDeps() + const mailbox = await mailboxStore.upsertConnectedMailbox({ + address: 'inbox-bad-key@example.test', + provider: 'imap', + }) + await imapConfigStore.upsertConfig(mailbox.id, { + imapHost: 'imap.example.test', + imapPort: 993, + smtpHost: 'smtp.example.test', + smtpPort: 465, + username: 'inbox-bad-key@example.test', + secure: true, + }) + await imapCredentialStore.upsertPassword(mailbox.id, 'super-secret-app-password') + + // Re-read through a store built with a DIFFERENT key — simulates a + // rotated/mismatched HELPTHREAD_TOKEN_ENC_KEY. + const wrongKeyCredentialStore = createImapCredentialStore(db!, Buffer.alloc(ENCRYPTION_KEY_BYTES, 1)) + const { factory: createGmailEmailSender } = fakeCreateGmailEmailSender() + const { factory: createSmtpEmailSender } = fakeCreateSmtpEmailSender() + const { tokenService } = fakeTokenService() + const resolver = createSenderResolver({ + mailboxStore, + tokenService, + imapConfigStore, + imapCredentialStore: wrongKeyCredentialStore, + createGmailEmailSender, + createSmtpEmailSender, + defaultAddress: DEFAULT_ADDRESS, + }) + + await expect(resolver.resolve(mailbox.id)).rejects.toMatchObject({ + code: 'unreadable-imap-credential', + }) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/sender-resolver.test.ts` around lines 180 - 241, Add a sender-resolver test alongside the existing IMAP credential cases that stores the mailbox password using one encryption key, recreates or configures the credential store with a different key, and resolves the mailbox through the resolver. Assert the rejection is a SenderResolutionError with code 'unreadable-imap-credential', reusing the existing freshDeps and resolver setup patterns.web/src/app/mailbox/[id]/settings/[section]/page.tsx (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBranch on the section key, not on
status !== 'planned'.The doc above states that
connectionis the only section with real content this pass, and that the registry is the injection point for new sections. The branch testsstatus, so the first additionalavailablesection added toresolveInboxSettingsSections()will silently render the connection card under that section's label. Branch on the key so a new section fails visibly instead.♻️ Proposed refactor
- {sectionDef.status === 'planned' ? ( - <PlannedSection label={sectionDef.label} /> - ) : ( - <ConnectionSection mailboxId={mailbox.id} address={mailbox.address} /> - )} + {sectionDef.key === 'connection' && sectionDef.status === 'available' ? ( + <ConnectionSection mailboxId={mailbox.id} address={mailbox.address} /> + ) : ( + <PlannedSection label={sectionDef.label} /> + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/app/mailbox/`[id]/settings/[section]/page.tsx around lines 40 - 44, Update the conditional rendering around PlannedSection and ConnectionSection to branch on the section key, rendering ConnectionSection only for the connection section and PlannedSection for other section keys. Do not use sectionDef.status to select the connection content, so newly available sections cannot silently reuse it.web/src/components/FolderNav.tsx (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared
ChevronDownIcon.This exact 9×9 chevron now exists in
web/src/components/FolderNav.tsx,web/src/components/InboxSettingsShell.tsx, andweb/src/components/TopBar.tsx, plus a 12×12 variant inweb/src/components/ConnectInboxForm.tsx. Move the shared version intods/coreand import it, so the stroke width and opacity stay identical across the rail, the shell nav, and the top bar. The rendered output does not change, so this needs no design sign-off.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/FolderNav.tsx` around lines 75 - 88, Extract the shared 9×9 ChevronDownIcon from FolderNav.tsx into the ds/core component area, then replace the duplicate implementations in FolderNav, InboxSettingsShell, and TopBar with imports of that shared component. Preserve its existing stroke width, opacity, dimensions, and SVG behavior; leave the distinct 12×12 ConnectInboxForm variant unchanged.web/src/components/ConnectInboxForm.tsx (1)
591-614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnounce the check result to assistive technology.
formErrorusesrole="alert", so failures of the request itself are announced. The per-leg result region is silent: afterCheck connectioncompletes, the IMAP/SMTP pills and the error lines appear with no live region, so a screen-reader user gets no confirmation that the check finished.♿ Proposed refactor
{checkResult !== null && checkResult.result !== undefined && ( - <div style={{ display: 'flex', flexDirection: 'column', gap: 8 }}> + <div + role="status" + aria-live="polite" + style={{ display: 'flex', flexDirection: 'column', gap: 8 }} + >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/ConnectInboxForm.tsx` around lines 591 - 614, Update the per-leg check-result container rendered when checkResult.result is available to use an appropriate assertive live-region mechanism, such as role="status" with aria-live="polite" or the existing accessibility convention, so screen readers announce completion and the IMAP/SMTP outcomes and errors. Keep the existing visual pills and error content unchanged.src/api/imap-connect.test.ts (1)
153-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
provider_conflictcase to the error-mapping test.
handleImapConnectmapsprovider_conflictto409and every otherImapConnectErrorcode to422(src/api/imap-connect.tsLine 213). The current table covers onlyimap_failedandsmtp_failed, so the409branch is untested.♻️ Proposed test addition
+ it('maps a caught ImapConnectError(provider_conflict) to a 409, not a 422', async () => { + const service = fakeService({ + connect: async () => { + throw new ImapConnectError('provider_conflict', 'already connected over another transport') + }, + }) + + const res = await handleImapConnect(postJson(CONNECT_URL, VALID_BODY), ADMIN, deps(service)) + + expect(res.status).toBe(409) + expect((await res.json()).error.code).toBe('provider_conflict') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/imap-connect.test.ts` around lines 153 - 168, Add a separate `provider_conflict` case to the `handleImapConnect` error-mapping tests, asserting that it returns HTTP 409 with the same error code and safe message; keep the existing `imap_failed` and `smtp_failed` cases asserting HTTP 422.src/api/imap-connect.ts (1)
283-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider wrapping this handler in the same try/catch the other two use.
handleImapConnectandhandleImapCheckeach contain their own try/catch so that every exit is aResponsethis module built (Line 185).handleGetMailboxImapConfighas none, so agetMailboxByIdorgetConfigrejection escapes to the dispatcher catch-all insrc/api/index.ts. The response shape stays controlled, but the module loses its own log line and its stated convention.♻️ Proposed refactor
- const mailbox = await deps.mailboxStore.getMailboxById(mailboxId) - if (mailbox === null) { - return apiError(404, 'not_found', 'No mailbox with that id.') - } - - const config = await deps.configStore.getConfig(mailboxId) - if (config === null) { - return apiError(404, 'not_found', 'This mailbox has no IMAP/SMTP configuration.') - } - - return json(200, { - imapHost: config.imapHost, - imapPort: config.imapPort, - smtpHost: config.smtpHost, - smtpPort: config.smtpPort, - username: config.username, - secure: config.secure, - }) + try { + const mailbox = await deps.mailboxStore.getMailboxById(mailboxId) + if (mailbox === null) { + return apiError(404, 'not_found', 'No mailbox with that id.') + } + + const config = await deps.configStore.getConfig(mailboxId) + if (config === null) { + return apiError(404, 'not_found', 'This mailbox has no IMAP/SMTP configuration.') + } + + return json(200, { + imapHost: config.imapHost, + imapPort: config.imapPort, + smtpHost: config.smtpHost, + smtpPort: config.smtpPort, + username: config.username, + secure: config.secure, + }) + } catch (err) { + console.error('[imap-connect] unhandled error reading mailbox imap config', err) + return apiError(500, 'server_error', 'Internal server error.') + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/imap-connect.ts` around lines 283 - 316, Wrap the body of handleGetMailboxImapConfig in the same try/catch pattern used by handleImapConnect and handleImapCheck, including the mailbox and config store calls. Preserve all existing validation and success responses, and ensure rejected dependencies are logged and converted into the module’s standard Response error rather than escaping to the dispatcher.src/store/mailboxes.test.ts (1)
396-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
statusas well, since the test claims the row is untouched.The query selects
status, but the assertions only checkprovider. Assert the originalstatusvalue so a conflict that still mutatesstatusfails this test.♻️ Proposed change
expect(rows).toHaveLength(1) expect(rows[0].provider).toBe('gmail') + expect(rows[0].status).toBe('active')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/mailboxes.test.ts` around lines 396 - 402, Extend the assertions in the mailbox conflict test to verify the selected row’s original status in addition to provider. Use the existing rows result from the query and assert rows[0].status matches the untouched status value expected by the test.src/store/imap-watch-state.test.ts (1)
54-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
NOT_A_LIVE_LEASEconstant for the literal uuid.Line 56 and line 386 repeat
'00000000-0000-4000-8000-000000000000'while the file already defines that value asNOT_A_LIVE_LEASEat line 7. Reuse the constant, or add a second named constant for the "mailbox that does not exist" id so the two roles stay distinct.Also applies to: 383-388
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/imap-watch-state.test.ts` around lines 54 - 57, Replace the repeated literal UUIDs in the “mailbox id does not exist” tests with the existing NOT_A_LIVE_LEASE constant, or introduce a clearly named separate constant if the two UUIDs represent distinct roles; update both affected test cases while preserving their assertions.web/src/components/MailboxConnectionSection.tsx (1)
145-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an accessible way to explain the unavailable action.
The
disabledbutton is not focusable, so itstitletext never reaches keyboard or screen reader users. The explanation is then visual-hover only. One option isaria-disabled="true"with ano-ophandler plus visible helper text next to the button. If the Claude Design prototype specifies the current treatment, keep it and get the maintainer's sign-off before changing the visual surface. As per coding guidelines, "The Agent Inbox UI and dogfood site must match the Claude Design prototype exactly across the whole designed surface".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/MailboxConnectionSection.tsx` around lines 145 - 152, The disabled “Send test email” action in the mailbox connection section is inaccessible because its availability explanation relies only on a non-focusable button title. Update the control to expose its unavailable state and explanation accessibly, using the established design treatment: retain the prototype’s visual appearance, add an accessible no-op interaction path such as aria-disabled when appropriate, and provide visible helper text associated with the button.Source: Coding guidelines
src/mail/imap-connect.test.ts (1)
247-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
provider_conflictmapping.
connectmapsMailboxProviderConflictErrortoImapConnectError('provider_conflict')(src/mail/imap-connect.tslines 355-365). That branch was added in response to an earlier review, and no test in this file exercises it. A regression there would silently convert a Gmail mailbox toprovider: 'imap'or surface a 500 instead of a 409.Seed a
provider: 'gmail'mailbox withcreateMailboxStore(db).upsertConnectedMailboxforVALID_INPUT.address, then assertconnectrejects with{ code: 'provider_conflict' }and that the mailbox row still reportsprovider: 'gmail'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-connect.test.ts` around lines 247 - 266, Add a test beside the existing failure cases that seeds VALID_INPUT.address as a connected Gmail mailbox via createMailboxStore(db).upsertConnectedMailbox, then verifies createImapConnectService(deps).connect rejects with code provider_conflict and the persisted mailbox still has provider gmail.src/mail/imap-fetch.test.ts (1)
524-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the lease-superseded cursor write.
fetchOneMailboxtreats afalsereturn fromwatchStateStore.setCursorasblockedand leaves the cursor to the live holder (src/mail/imap-fetch.tslines 287-306). The module doc marks that fence as SACRED, and no test in this file covers it.You can force it inside
ingest: overwrite the row'slease_tokenwith a freshgen_random_uuid()throughstores.db.query, then assertreport.failed === 1and that the cursor stays at its seeded value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.test.ts` around lines 524 - 551, Add a test in the fetch failure scenarios that supersedes the mailbox lease during ingest by updating its lease_token through stores.db.query with gen_random_uuid(). Assert runImapFetch reports one failure and verify the mailbox cursor remains at its seeded value, covering the false setCursor/blocked path in fetchOneMailbox without changing production behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/imap-connect.ts`:
- Around line 9-17: Update the module documentation heading around
handleImapConnect and handleImapCheck to state that both routes require an
authenticated admin acting Agent, not merely Bearer authentication. Keep the
surrounding explanation of their normal authenticated POST route flow accurate
and consistent with requireAdmin and the actingAgent request behavior.
In `@src/store/imap-watch-state.ts`:
- Around line 114-119: Update the stale SACRED fence documentation in
seedBaseline, releaseFetchLease, and setCursor to match the actual lease_token
predicates used by their SQL. Remove the claim that seedBaseline shares
setCursor’s SQL shape, describe releaseFetchLease as matching the lease token
rather than claimed_until, and identify lease_token—not claimed_until—as the
setCursor fence; change comments only.
In `@src/store/mailboxes.ts`:
- Around line 106-120: Update the constructor message of
MailboxProviderConflictError to remove the unsupported instruction to remove the
mailbox, and state only the currently supported resolution of connecting under a
different address. Do not add mailbox deletion capability or alter the error
fields and name.
In `@web/src/app/`(shell)/layout.tsx:
- Around line 34-35: Update the mailbox lookup in the resolvedMailbox selection
to compare entry.address and supportAddress case-insensitively, while preserving
the existing mailboxes[0] fallback when no address matches.
- Line 33: Update the mailbox-loading logic around listMailboxes so
non-authentication ApiError failures are caught and treated as no mailbox data,
allowing the shell to render with the existing null-mailbox behavior. Preserve
the 401/SESSION_ERROR path by rethrowing authentication errors, and add the
required ApiError import from the established API module.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 52-149: Update PROVIDER_PRESETS and the related connection flow so
IMAP and SMTP use independent TLS settings, ensuring port 993 presets such as
Outlook and iCloud use implicit IMAP TLS while SMTP retains its
provider-specific mode. Propagate the separate flags through ImapConnectInput,
adapter invocation, verification, persistence, and preset checks without
changing unrelated behavior. Obtain maintainer sign-off for the corrected SMTP hostnames
and secure values documented in the preset comments before merging.
In `@web/src/components/FolderNav.tsx`:
- Around line 171-199: Update the gear dropdown button in FolderNav to expose
its menu state by adding aria-haspopup="menu", aria-expanded={gearOpen}, and an
explicit aria-label describing mailbox settings; keep the existing title and
toggle behavior unchanged.
In `@web/src/lib/api.ts`:
- Around line 388-404: Update imapCheckConnection and imapConnect to pass an
explicit client timeout longer than the current 15-second default, allowing both
IMAP and SMTP connection checks to complete or reach their server-side/engine
timeout. Keep the existing request methods, paths, bodies, and actingAgent
behavior unchanged.
---
Nitpick comments:
In `@src/api/imap-connect.test.ts`:
- Around line 153-168: Add a separate `provider_conflict` case to the
`handleImapConnect` error-mapping tests, asserting that it returns HTTP 409 with
the same error code and safe message; keep the existing `imap_failed` and
`smtp_failed` cases asserting HTTP 422.
In `@src/api/imap-connect.ts`:
- Around line 283-316: Wrap the body of handleGetMailboxImapConfig in the same
try/catch pattern used by handleImapConnect and handleImapCheck, including the
mailbox and config store calls. Preserve all existing validation and success
responses, and ensure rejected dependencies are logged and converted into the
module’s standard Response error rather than escaping to the dispatcher.
In `@src/mail/imap-connect.test.ts`:
- Around line 247-266: Add a test beside the existing failure cases that seeds
VALID_INPUT.address as a connected Gmail mailbox via
createMailboxStore(db).upsertConnectedMailbox, then verifies
createImapConnectService(deps).connect rejects with code provider_conflict and
the persisted mailbox still has provider gmail.
In `@src/mail/imap-fetch.test.ts`:
- Around line 524-551: Add a test in the fetch failure scenarios that supersedes
the mailbox lease during ingest by updating its lease_token through
stores.db.query with gen_random_uuid(). Assert runImapFetch reports one failure
and verify the mailbox cursor remains at its seeded value, covering the false
setCursor/blocked path in fetchOneMailbox without changing production behavior.
In `@src/mail/sender-resolver.test.ts`:
- Around line 180-241: Add a sender-resolver test alongside the existing IMAP
credential cases that stores the mailbox password using one encryption key,
recreates or configures the credential store with a different key, and resolves
the mailbox through the resolver. Assert the rejection is a
SenderResolutionError with code 'unreadable-imap-credential', reusing the
existing freshDeps and resolver setup patterns.
In `@src/store/imap-watch-state.test.ts`:
- Around line 54-57: Replace the repeated literal UUIDs in the “mailbox id does
not exist” tests with the existing NOT_A_LIVE_LEASE constant, or introduce a
clearly named separate constant if the two UUIDs represent distinct roles;
update both affected test cases while preserving their assertions.
In `@src/store/mailboxes.test.ts`:
- Around line 396-402: Extend the assertions in the mailbox conflict test to
verify the selected row’s original status in addition to provider. Use the
existing rows result from the query and assert rows[0].status matches the
untouched status value expected by the test.
In `@web/src/app/mailbox/`[id]/settings/[section]/page.tsx:
- Around line 40-44: Update the conditional rendering around PlannedSection and
ConnectionSection to branch on the section key, rendering ConnectionSection only
for the connection section and PlannedSection for other section keys. Do not use
sectionDef.status to select the connection content, so newly available sections
cannot silently reuse it.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 591-614: Update the per-leg check-result container rendered when
checkResult.result is available to use an appropriate assertive live-region
mechanism, such as role="status" with aria-live="polite" or the existing
accessibility convention, so screen readers announce completion and the
IMAP/SMTP outcomes and errors. Keep the existing visual pills and error content
unchanged.
In `@web/src/components/FolderNav.tsx`:
- Around line 75-88: Extract the shared 9×9 ChevronDownIcon from FolderNav.tsx
into the ds/core component area, then replace the duplicate implementations in
FolderNav, InboxSettingsShell, and TopBar with imports of that shared component.
Preserve its existing stroke width, opacity, dimensions, and SVG behavior; leave
the distinct 12×12 ConnectInboxForm variant unchanged.
In `@web/src/components/MailboxConnectionSection.tsx`:
- Around line 145-152: The disabled “Send test email” action in the mailbox
connection section is inaccessible because its availability explanation relies
only on a non-focusable button title. Update the control to expose its
unavailable state and explanation accessibly, using the established design
treatment: retain the prototype’s visual appearance, add an accessible no-op
interaction path such as aria-disabled when appropriate, and provide visible
helper text associated with the button.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9be0dbd-e810-4c45-a109-6f514854dc78
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonscripts/dev-api.tsspecs/mail/mailbox-connection.mdsrc/api/agents.test.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/saved-replies.test.tssrc/api/webauthn.test.tssrc/api/webhooks.test.tssrc/composition/app.test.tssrc/composition/app.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/imap-connect.test.tssrc/mail/imap-connect.tssrc/mail/imap-fetch.test.tssrc/mail/imap-fetch.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.test.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/imap/index.tssrc/providers/adapters/smtp/index.tssrc/providers/adapters/smtp/sender.test.tssrc/providers/adapters/smtp/sender.tssrc/providers/adapters/smtp/verify.test.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-config.test.tssrc/store/imap-config.tssrc/store/imap-credentials.test.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tstsconfig.jsonvercel.jsonweb/src/app/(shell)/layout.tsxweb/src/app/mailbox/[id]/settings/[section]/page.tsxweb/src/app/manage/mailboxes/new/page.tsxweb/src/app/manage/mailboxes/page.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/MailboxConnectionSection.tsxweb/src/components/MailboxListScreen.tsxweb/src/components/NewMailboxScreen.tsxweb/src/components/SettingsScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/inbox-settings-sections.tsweb/src/lib/mailbox-actions.ts
…tale docs (HT-101) CodeRabbit's re-review of the previous commit: 9 open findings, 8 real and fixed, 1 rejected. **Every Outlook and iCloud preset was broken on arrival.** A single `secure` flag was fed to BOTH the IMAP and SMTP legs, but every provider preset pairs IMAP on 993 with SMTP on 465 or 587. An Outlook preset (`imapPort: 993`, `smtpPort: 587`, `secure: false`) therefore made the IMAP leg attempt STARTTLS against an implicit-TLS port, which cannot succeed. TLS mode is now derived per leg from that leg's own port (`imapImplicitTlsForPort`, RFC 8314 §3.3), with a regression test built from the exact preset shape that broke. NOT done here, deliberately: splitting `secure` into explicit per-leg fields across the API, schema, and connect form. That is the fuller fix, it needs a product call on how the form presents TLS, and the preset values themselves were already flagged for the maintainer in a code comment. Filed separately. **A mailbox-list failure took down the entire inbox.** `(shell)/layout.tsx` wraps every inbox and conversation route, and `listMailboxes()` throws on any non-2xx — so an engine error on a call that feeds ONE gear menu removed the folder rail, the counts, and the conversation view. Contained to an inert gear, which `FolderNav` already supports. A 401 still propagates on purpose: that means a stale session and must reach the re-login redirect. **Two instructions that named things that do not exist.** The provider-conflict message told the operator to "remove this one first" — `MailboxStore` has no delete. And the API module doc still described both IMAP routes as ordinary Bearer-gated, which is the exact misreading that left them un-gated in the first place. Also: the support-address match is now case-insensitive (a case difference silently deep-linked the gear into a different mailbox than the rail shows); three lease-token doc statements still described the pre-uuid design; `client.close()` is guarded at its call site in `fetch.ts` rather than relying on one implementation never throwing; and the folder-rail gear button gains `aria-haspopup`/`aria-expanded`/`aria-label`. Rejected, third time: `Team` to `Agents` in `TopBar.tsx`. `specs/ui/admin-ia.md` §3 records Agents/Team as a deliberate deviation, the file is untouched by this PR, and the vocabulary is under active revision in the other direction. Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0; 87 files / 1721 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 41 minutes. |
…26-07-31) Thirty-two doc comments, plus the spec, carried "(review, 2026-07-25)". The review happened on 2026-07-31; the wrong date came from reading an older commit's timestamp as today's. The dates are load-bearing here — they are how a future reader locates which review produced a given guard — so a wrong one sends them to the wrong conversation. No behaviour change; comments and one spec heading only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (HT-101) An adversarial Codex pass, run in place of CodeRabbit (rate-limited on this head), scoped deliberately to whether rounds 1-4 introduced NEW defects rather than re-reviewing the feature. Three findings, all real, all in fixes from those rounds. **The decrypt containment traded one blast radius for another.** Round three caught every rejection from `getPassword` and converted it to a `SenderResolutionError` so a bad encryption key could not abort the whole delivery sweep. But `runDeliveryWorker` marks a `SenderResolutionError` row `failed` — so a transient database fault, swept into the same bucket, would permanently fail outbound mail that only needed retrying. `ImapCredentialStore.getPassword` now throws a typed `ImapCredentialDecryptError` at the crypto boundary; the `await` on the query has already resolved by then, so anything thrown there is decryption and nothing else. `sender-resolver.ts` contains ONLY that type and rethrows everything else untouched, so a store fault still aborts the sweep for a retry. Tests pin both halves, including that the propagated error is the same object rather than a wrapper. **The per-leg TLS fix regressed the opposite case.** Round four derived implicit TLS from port 993 alone, which is right for every provider preset and wrong for an operator running implicit-TLS IMAP on a non-standard port — `imap.internal:1993` was previously served correctly by `secure: true` and would suddenly have STARTTLS forced on it. Now `port === 993 || secure === true`: the port answers the preset case, the explicit flag answers the unusual one, and neither overrides a working configuration. Deliberate asymmetry: STARTTLS cannot be forced onto 993. That combination does not exist in practice (RFC 8314 §3.3) and the failure mode of guessing wrong is a cleartext credential. **Two comments still told operators to "disconnect first"** — the instruction round three established can never terminate, because disconnect leaves `provider` set. `mailboxes.ts` was corrected then; `imap-connect.ts`'s own two copies were missed. Codex explicitly cleared the lease fencing (a `false` from `setCursor` causes retry work, not permanent re-ingestion), the provider guard's behaviour on fresh inserts, and the `close()` guards. Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0; 87 files / 1727 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes. |
A sixth adversarial pass, scoped to the previous commit's own three fixes,
found that the widened TLS predicate broke an ordinary configuration: IMAP on
143 (STARTTLS) alongside SMTP on 465 (implicit TLS) carries `secure: true`,
and `port === 993 || secure` then forced a TLS handshake against a server
expecting a plaintext IMAP greeting. Connect is rejected and every subsequent
fetch fails.
That is the THIRD rule tried in one review cycle, and the third to fix one real
configuration by breaking another:
pass `secure` to both legs → breaks 993 + secure:false (every Outlook and
iCloud preset)
port === 993 → breaks 1993 + secure:true (non-standard
implicit-TLS port)
port === 993 || secure → breaks 143 + secure:true (IMAP STARTTLS with
SMTPS, an ordinary self-hosted setup)
No fourth rule is attempted here. One boolean cannot describe two independent
connections; every candidate simply relocates which operator is broken. This
reverts to the second rule, whose failure is the rarest and least reachable —
no preset produces a non-standard implicit-TLS IMAP port, and reaching it needs
a deliberately unusual server, where the third rule's failure is one a
self-hosting operator meets on day one.
All three rules and their failure modes are now recorded in
`imapImplicitTlsForPort`'s doc, with a test pinning the known gap explicitly, so
the next person neither re-derives this nor "improves" it by adding another
operand. The real fix — per-leg TLS fields in the API, schema, and connect form
— is tracked separately and has been raised in priority with this evidence.
Every wrong answer here fails loudly at connect time with a TLS error. Nothing
is silently downgraded to cleartext.
Also confirmed clean by the same pass: the typed `ImapCredentialDecryptError`
separates a decrypt failure from a store fault correctly, and the delivery
worker's logging of a `SenderResolutionError` cause chain carries no ciphertext,
key, or password.
Gates: typecheck, web typecheck, web build, lint, gitleaks all exit 0;
87 files / 1726 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (18)
src/api/imap-connect.test.ts (1)
153-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for the
provider_conflict→ 409 mapping.The
it.eachcoversimap_failedandsmtp_failed, both of which map to 422.handleImapConnectmaps a third code differently:provider_conflictreturns 409 (src/api/imap-connect.tsLine 221). That branch is the one guarding against converting a Gmail-connected address to IMAP in place, and it has no handler-level test. A regression that collapses it back to 422 would pass this suite.💚 Proposed test addition
+ it('maps a caught ImapConnectError(provider_conflict) to a 409, not a 422', async () => { + const service = fakeService({ + connect: async () => { + throw new ImapConnectError('provider_conflict', 'already connected over another transport') + }, + }) + + const res = await handleImapConnect(postJson(CONNECT_URL, VALID_BODY), ADMIN, deps(service)) + + expect(res.status).toBe(409) + expect((await res.json()).error.code).toBe('provider_conflict') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/imap-connect.test.ts` around lines 153 - 168, Add a dedicated handler-level test near the existing ImapConnectError cases that throws ImapConnectError with provider_conflict from fakeService.connect, then assert handleImapConnect returns status 409 and preserves the error code and safe message in the response body. Keep the existing 422 it.each cases unchanged.web/src/components/ConnectInboxForm.tsx (1)
591-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid using
StatusPill status="spam"for IMAP/SMTP failures.
spamcurrently maps to the critical color, but it represents conversation classification and the documented allowed valuespam | conversationis unrelated to connection status. If the failed IMAP/SMTP pills must visually match this state, require explicit sign-off or extract a semantically named status token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/ConnectInboxForm.tsx` around lines 591 - 602, Update the IMAP and SMTP failure states in the StatusPill usages within ConnectInboxForm so they do not pass the semantically unrelated “spam” status. Introduce or reuse an explicitly named connection-failure status token that preserves the current critical visual treatment, and apply it to both failed checks while leaving successful “active” states unchanged.Source: Coding guidelines
web/src/app/mailbox/[id]/settings/[section]/page.tsx (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRender
ConnectionSectiononly for theconnectionkey.The branch treats every non-
plannedsection as the connection section. The section registry is documented as an injection point that Modules can extend (web/src/lib/inbox-settings-sections.ts, lines 91-99). If a Module adds a secondavailablesection, this page renders the IMAP/SMTP connection panel under that section's label.Key the branch on the section identity instead, and fall back to the placeholder for any section this page cannot render yet.
♻️ Proposed change
return ( <InboxSettingsShell mailbox={mailbox} mailboxes={mailboxes} activeSection={section}> - {sectionDef.status === 'planned' ? ( - <PlannedSection label={sectionDef.label} /> - ) : ( + {sectionDef.status === 'available' && sectionDef.key === 'connection' ? ( <ConnectionSection mailboxId={mailbox.id} address={mailbox.address} /> + ) : ( + <PlannedSection label={sectionDef.label} /> )} </InboxSettingsShell> )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/app/mailbox/`[id]/settings/[section]/page.tsx around lines 38 - 46, Update the conditional rendering in the page component to render ConnectionSection only when the section key is "connection"; render PlannedSection as the fallback for planned sections and any other section keys not handled by this page. Use the existing section identity value, such as section or sectionDef.key, rather than relying on status alone.src/mail/imap-fetch.test.ts (1)
497-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the lease-superseded cursor write.
The suite covers the lease-held skip and the lease release, but not the fence at
src/mail/imap-fetch.tslines 296-309:setCursorreturnsfalsewhen a successor reclaimed the expired lease, and the run must countfailedand leave the cursor untouched. That path is marked SACRED and is the one that protects the UIDVALIDITY-reset quarantine.You can force it with a short
leaseMsplus a competingclaimFetchLeaseinside theingestfake:💚 Suggested test
it('a superseded lease blocks the cursor write — the live holder keeps the cursor', async () => { const stores = await freshStores() const mailboxId = await seedImapMailbox(stores, { address: 'superseded@example.test', cursor: { uidValidity: 100, lastUid: 0 }, }) // Steal the lease while the first run is still ingesting. const ingest = vi.fn(async (raw: RawInboundMessage) => { await stores.watchStateStore.claimFetchLease(mailboxId, 60_000) return storedOutcome(raw) }) const report = await runImapFetch({ mailboxStore: stores.mailboxStore, configStore: stores.configStore, credentialStore: stores.credentialStore, watchStateStore: stores.watchStateStore, createImapClient: fakeCreateImapClient({ uidValidity: 100, uidNext: 2, messages: [rawMessage(1)], }), ingest, leaseMs: 1, }) expect(report).toMatchObject({ fetched: 0, failed: 1 }) expect(await stores.watchStateStore.getCursor(mailboxId)).toEqual({ uidValidity: 100, lastUid: 0, }) })The competing claim needs the first lease to be expired, so keep
leaseMsat 1 ms and add a small delay before the claim if PGlite's clock resolution requires it.A second gap: the
missing-config-credential-or-cursorskip branch (src/mail/imap-fetch.tslines 217-228) has no test either.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-fetch.test.ts` around lines 497 - 551, Add tests in the IMAP fetch suite for the lease-superseded cursor-write path: use a 1 ms lease and have the ingest mock reclaim the mailbox lease, then assert the run reports fetched: 0 and failed: 1 while getCursor retains the original cursor. Also add coverage for the missing-config-credential-or-cursor skip branch, asserting the mailbox is skipped without fetching or ingesting.web/src/components/MailboxConnectionSection.tsx (1)
149-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the unavailability reason out of
title.A disabled button does not receive keyboard focus, and most screen readers do not announce
titleon it. Keyboard and screen-reader users get the label "Send test email" with no reason for the disabled state. Render the reason as adjacent visible text, which also matches the honest-affordance intent stated in the module doc.♿ Proposed fix
- <Button variant="outline" disabled title="Not yet available — no send-test endpoint yet"> - Send test email - </Button> + <Button variant="outline" disabled> + Send test email + </Button> + <span style={{ fontSize: 12, color: 'var(--ht-ink-dim)' }}> + Not yet available — no send-test endpoint yet. + </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/MailboxConnectionSection.tsx` around lines 149 - 151, Update the disabled “Send test email” Button in MailboxConnectionSection to remove the title-only explanation and render “Not yet available — no send-test endpoint yet” as adjacent visible text, preserving the disabled state and clearly associating the reason with the control.src/store/imap-credentials.test.ts (1)
164-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate test.
This test repeats the assertion of the test at Line 33 exactly: insert a mailbox, then expect
getPasswordto returnnull. The added name states an intent ("absent and unreadable are different states") that the body does not verify, because it never creates an unreadable row.Either delete this test or make it contrast both states in one assertion pair.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/imap-credentials.test.ts` around lines 164 - 169, Remove the duplicate test around store.getPassword, or update it to create an unreadable credential row and assert that result differs from the missing-row null result; retain the existing missing-row assertion only if the test explicitly contrasts both states.src/db/migrate.test.ts (1)
1566-1568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the failure cause, not just any failure.
Both assertions use a bare
.rejects.toThrow(). Any error passes, including a typo in the SQL or a missing column. Match the foreign-key violation text so the test proves the constraint, not just a rejection.♻️ Proposed change
await expect( database.query('DELETE FROM mailboxes WHERE id = $1', [mailbox.id]), - ).rejects.toThrow() + ).rejects.toThrow(/violates foreign key constraint/i)await expect( database.query('UPDATE conversations SET mailbox_id = $1 WHERE id = $2', [ '00000000-0000-0000-0000-000000000000', existing.id, ]), - ).rejects.toThrow() + ).rejects.toThrow(/violates foreign key constraint/i)Also applies to: 1578-1583
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrate.test.ts` around lines 1566 - 1568, Update the DELETE constraint assertions in the migration tests to verify the rejection message identifies a foreign-key violation, rather than accepting any thrown error. Apply the same message-specific assertion to both affected cases while preserving their existing mailbox deletion scenarios.web/src/components/MailboxListScreen.tsx (1)
12-19: 📐 Maintainability & Code Quality | 🔵 TrivialGet the maintainer's sign-off before merge for the omitted
providercolumn.The card shows address and status only. The spec draft asked for address, provider, and status. The file documents this as an open decision. Confirm the sign-off, or add
providertoGET /api/v1/mailboxesand the card.As per coding guidelines: "The Agent Inbox UI and dogfood site must match the Claude Design prototype exactly across the whole designed surface, including visual details, copy, and interactions; deviations require the maintainer's explicit sign-off."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/MailboxListScreen.tsx` around lines 12 - 19, Resolve the documented provider-column decision before merging: obtain the maintainer’s explicit sign-off for the address-and-status-only card, or update the GET /api/v1/mailboxes response via toMailboxJson and the mailbox card in MailboxListScreen to include provider alongside address and status.Source: Coding guidelines
web/src/components/FolderNav.tsx (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared chevron glyph.
ChevronDownIconnow exists with the same body inweb/src/components/FolderNav.tsx,web/src/components/TopBar.tsx,web/src/components/ConnectInboxForm.tsx, andweb/src/components/InboxSettingsShell.tsx. Move one copy into theds/coreprimitives and import it, so the glyph stays consistent when the design changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/FolderNav.tsx` around lines 75 - 88, Extract the shared ChevronDownIcon glyph from the duplicated component implementations into the ds/core primitives, export it there, and replace the local definitions in FolderNav, TopBar, ConnectInboxForm, and InboxSettingsShell with imports. Preserve the existing SVG dimensions, styling, and accessibility attributes so all consumers render consistently.src/providers/adapters/imap/fetch.test.ts (1)
220-231: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a case for a throwing
close().
fetch.tslines 187-191 swallow close failures so cleanup never replaces the real outcome. No test covers that path. Add a fake whoseclose()rejects, then assert that a successful fetch still resolves and that a mid-fetch error still surfaces its own message.💚 Proposed test
+ it('swallows a close() failure — a successful fetch still resolves', async () => { + const calls = newCallLog() + const mailbox: ImapMailboxInfo = { uidValidity: 1, uidNext: 20 } + const base = createFakeClient(calls, mailbox, []) + const client: ImapClient = { + ...base, + async close() { + calls.close++ + throw new Error('close boom') + }, + } + + const result = await fetchImapInboundMessages( + 'mailbox-1', + { uidValidity: 1, lastUid: 5 }, + () => client, + 50, + ) + + expect(result.newCursor).toEqual({ uidValidity: 1, lastUid: 5 }) + expect(calls.close).toBe(1) + }) + + it('a close() failure never masks the original fetch error', async () => { + const calls = newCallLog() + const mailbox: ImapMailboxInfo = { uidValidity: 1, uidNext: 20 } + const base = createFakeClient(calls, mailbox, [], new Error('boom')) + const client: ImapClient = { + ...base, + async close() { + calls.close++ + throw new Error('close boom') + }, + } + + await expect( + fetchImapInboundMessages('mailbox-1', { uidValidity: 1, lastUid: 5 }, () => client, 50), + ).rejects.toThrow('boom') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/imap/fetch.test.ts` around lines 220 - 231, Add coverage in the fetchImapInboundMessages tests for a client whose close() rejects: verify a successful fetch still resolves, and verify a uidFetchRawSince failure still rejects with its original error message. Reuse the existing createFakeClient and call-log setup, and assert close was attempted so the swallowing behavior in fetch.ts is exercised.src/providers/adapters/imap/client.ts (2)
297-337: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftThe batch is bounded by message count, not by total bytes.
resultsholds the full RFC822 body of every message in the batch. With the defaultmaxof 50, a mailbox carrying large attachments can hold hundreds of megabytes in one cron invocation, and a serverless function has a fixed memory limit. Consider adding a cumulative byte budget that stops the loop early, and let the cursor advance to the last message actually collected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/imap/client.ts` around lines 297 - 337, Add a cumulative byte budget to the FETCH loop that tracks each message’s raw source size and stops collecting before the batch exceeds the configured memory limit. Ensure the cursor and returned results reflect only messages actually collected, while preserving the existing source and internalDate validation for accepted messages.
218-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow
defaultCreateFlowto the small imapflow surface this file actually uses.
new ImapFlow(...)is cast to the wholeImapFlowLikestruct viaas unknown as ImapFlowLike, not checked. If the npm package type is unavailable, replace the local hand-writtenImapFlowLikewith the imported npmImapFlow(whosemailboxOpen().uidValidityisbigint) and convert the value locally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/imap/client.ts` around lines 218 - 240, Update defaultCreateFlow to use the imported npm ImapFlow type instead of the hand-written ImapFlowLike and remove the unchecked whole-struct cast. Narrow the adapter-facing type to only the imapflow methods used in this file, and convert mailboxOpen().uidValidity from bigint at the local boundary where the existing code expects the current value type.src/providers/adapters/smtp/verify.test.ts (2)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test duplicates the first test and does not prove its own title.
The title states that the real nodemailer transport factory is never called. The body never observes
nodemailer.createTransport. The assertions are identical to the first test at lines 13-19. Spy on the module to make the claim real, or delete the case.♻️ Proposed change to actually assert the factory is untouched
+import nodemailer from 'nodemailer' + ... it('never calls the real nodemailer transport factory when a fake transporter is supplied', async () => { - // No assertion beyond "this resolves without touching the network" — - // the fake never delegates to a real transporter, and there is no - // network available in the test environment, so any accidental real - // network attempt would time out / hang, not silently pass. + const createTransport = vi.spyOn(nodemailer, 'createTransport') const verify = vi.fn(async () => true as const) await verifySmtpConnection({ ...baseOptions, transporter: { verify } }) expect(verify).toHaveBeenCalledOnce() + expect(createTransport).not.toHaveBeenCalled() + createTransport.mockRestore() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/verify.test.ts` around lines 31 - 39, Update the test “never calls the real nodemailer transport factory when a fake transporter is supplied” to spy on or otherwise observe nodemailer.createTransport, then assert it is not called after verifySmtpConnection receives the injected fake transporter. Keep the existing verify invocation assertion, and remove the test only if the factory cannot be observed reliably.
12-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
withTimeoutbackstop.
verify.tsnow racesverify()against a timer (lines 87-103, 125). No test exercises that path. A regression that removes the race would still pass this suite. Add a case with a transporter whoseverify()never settles and a smalltimeoutMs.💚 Proposed test
+ it('rejects when verify() outlives timeoutMs — the outer backstop settles the call', async () => { + const verify = vi.fn(() => new Promise<true>(() => {})) + await expect( + verifySmtpConnection({ ...baseOptions, transporter: { verify }, timeoutMs: 20 }), + ).rejects.toThrow(/timed out/i) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/verify.test.ts` around lines 12 - 40, Add a test in the verifySmtpConnection suite using a transporter whose verify method never settles and a small timeoutMs, then assert that verifySmtpConnection rejects when the withTimeout backstop expires. Keep the existing success and error propagation tests unchanged, and ensure the test uses the injected fake transporter rather than the real transport factory.src/mail/imap-connect.test.ts (1)
174-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the success-path
close()assertion.This case proves cleanup only when
connect()throws. The happy path is unasserted. A regression that skipsclose()after a successfulselectInbox()leaks an IMAP connection on every check and every connect, and this suite would stay green.💚 Proposed additional test
+ it('closes the IMAP client on the success path too', async () => { + const { factory, state } = fakeCreateImapClient() + const { deps } = await freshDeps({ createImapClient: factory }) + const service = createImapConnectService(deps) + + await service.checkConnection(VALID_INPUT) + + expect(state.closeCalls).toBe(1) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-connect.test.ts` around lines 174 - 182, Add a successful-connection test alongside the existing “connect() throws” case, using the fake IMAP client and valid input to let connect/selectInbox complete, then assert state.closeCalls is 1 after service.checkConnection. Keep the existing error-path assertion unchanged.src/providers/adapters/smtp/sender.test.ts (1)
133-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the
Referencesheader itself, not just substring presence.Each reference is also present in
In-Reply-Toor elsewhere in the composed bytes, so this loop can pass without aReferencesheader existing. Assert the joined header value to make the wire-level proof exact.♻️ Proposed change
- for (const ref of email.references ?? []) { - expect(wireText).toContain(ref) - } + expect(wireText).toContain(`References: ${(email.references ?? []).join(' ')}`)Nodemailer may fold long
Referencesvalues across lines, so confirm the composed formatting before adopting this exact assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/sender.test.ts` around lines 133 - 135, Update the References assertions in the sender test to extract and validate the actual References header value from wireText, rather than checking each reference as an unrestricted substring. Assert the joined references value while accounting for Nodemailer’s possible folded header formatting, so the test proves the header itself exists and contains the expected values.src/store/imap-config.test.ts (1)
171-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
txtest cannot distinguishtxfromdb.This case passes even if
upsertConfigignores the suppliedtxand writes on the bounddb. Add a rollback case to prove the write joins the caller's transaction.💚 Proposed additional test
+ it('a rolled-back tx leaves no config row — the write really joined the caller transaction', async () => { + const { db, store } = await freshStore() + const mailboxId = await insertMailbox(db) + + await expect( + db.transaction(async (tx) => { + await store.upsertConfig( + mailboxId, + { + imapHost: 'imap.example.test', + imapPort: 993, + smtpHost: 'smtp.example.test', + smtpPort: 587, + username: 'agent@example.test', + secure: true, + }, + tx, + ) + throw new Error('rollback') + }), + ).rejects.toThrow('rollback') + + expect(await store.getConfig(mailboxId)).toBeNull() + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/imap-config.test.ts` around lines 171 - 191, Strengthen the upsertConfig transaction test around the existing upsertConfig and db.transaction calls by making the caller-supplied transaction roll back after the write, then assert getConfig returns no configuration for the mailbox. Keep the commit-path coverage if needed, but ensure the rollback assertion proves upsertConfig uses tx rather than the bound db.src/providers/adapters/smtp/sender.ts (1)
145-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
withTimeoutis duplicated verbatim in./verify.ts.The same helper now exists in
sender.ts(lines 145-161) andverify.ts(lines 87-103). Only the error message differs. Extract it into one module in this directory and import it in both, so the two timeout semantics cannot drift apart.♻️ Proposed extraction
New file
src/providers/adapters/smtp/timeout.ts:/** Race `promise` against a rejecting timer. See sender.ts's send-bound doc. */ export function withTimeout<T>(promise: Promise<T>, timeoutMs: number, label: string): Promise<T> { return new Promise<T>((resolve, reject) => { const timer = setTimeout(() => { reject(new Error(`${label}: timed out after ${timeoutMs}ms`)) }, timeoutMs) promise.then( (value) => { clearTimeout(timer) resolve(value) }, (err) => { clearTimeout(timer) reject(err) }, ) }) }Then in
sender.ts:-function withTimeout<T>(promise: Promise<T>, timeoutMs: number): Promise<T> { - ... -} +import { withTimeout } from './timeout.js'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/adapters/smtp/sender.ts` around lines 145 - 161, Extract the duplicated withTimeout helper from sender.ts and verify.ts into a shared timeout module in the SMTP adapter directory, exporting a label-based API so each caller retains its existing timeout error context. Update both callers to import and use the shared helper, preserving the current timer cleanup, rejection, and resolution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/index.ts`:
- Around line 266-277: Update the imapConnect documentation to state that POST
/api/v1/inbound/imap/connect and POST /api/v1/inbound/imap/check require an
authenticated admin acting Agent, in addition to ordinary Bearer authentication.
Keep the existing absent-by-default and 404 behavior description unchanged,
aligning the wording with the requireAdmin enforcement in handleImapConnect and
handleImapCheck.
In `@src/mail/delivery-worker.test.ts`:
- Around line 579-583: Update the test name and adjacent setup comment in the
delivery-worker test to describe the reachable pre-028 scenario: a conversation
with null mailbox_id whose persisted fromAddress differs from the resolved
default address. Remove references to mailbox deletion, hard deletion, or
“mailbox deleted → default,” while preserving the test’s mismatch and no-send
behavior.
In `@src/mail/imap-connect.ts`:
- Around line 71-79: Correct the persist-step documentation to reference
ImapWatchStateStore.seedBaselineIfAbsent instead of
ImapWatchStateStore.seedBaseline, matching the call in connect and preserving
the documented reconnect behavior that retains an existing cursor. Do not change
the implementation or surrounding transaction description.
In `@src/mail/sender-resolver.ts`:
- Around line 61-66: Update the documentation immediately above
SenderResolutionErrorCode to state that SenderResolutionError can carry four
failure shapes, matching all members of the union; leave the error codes and
behavior unchanged.
In `@src/store/imap-credentials.ts`:
- Around line 40-51: Move the persistence-contract doc comment currently
immediately before ImapCredentialDecryptError so it directly documents the
ImapCredentialStore interface. Leave the decrypt-error documentation attached
only to ImapCredentialDecryptError, and ensure the interface no longer lacks its
intended documentation.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 540-586: Update the control copy in the ConnectInboxForm “Use TLS”
toggle to clarify that it applies only to SMTP, and indicate that IMAP TLS is
determined by the IMAP port (including STARTTLS behavior for port 587). Preserve
the existing toggle behavior and obtain the maintainer’s sign-off for the copy change to
maintain the required prototype match.
---
Nitpick comments:
In `@src/api/imap-connect.test.ts`:
- Around line 153-168: Add a dedicated handler-level test near the existing
ImapConnectError cases that throws ImapConnectError with provider_conflict from
fakeService.connect, then assert handleImapConnect returns status 409 and
preserves the error code and safe message in the response body. Keep the
existing 422 it.each cases unchanged.
In `@src/db/migrate.test.ts`:
- Around line 1566-1568: Update the DELETE constraint assertions in the
migration tests to verify the rejection message identifies a foreign-key
violation, rather than accepting any thrown error. Apply the same
message-specific assertion to both affected cases while preserving their
existing mailbox deletion scenarios.
In `@src/mail/imap-connect.test.ts`:
- Around line 174-182: Add a successful-connection test alongside the existing
“connect() throws” case, using the fake IMAP client and valid input to let
connect/selectInbox complete, then assert state.closeCalls is 1 after
service.checkConnection. Keep the existing error-path assertion unchanged.
In `@src/mail/imap-fetch.test.ts`:
- Around line 497-551: Add tests in the IMAP fetch suite for the
lease-superseded cursor-write path: use a 1 ms lease and have the ingest mock
reclaim the mailbox lease, then assert the run reports fetched: 0 and failed: 1
while getCursor retains the original cursor. Also add coverage for the
missing-config-credential-or-cursor skip branch, asserting the mailbox is
skipped without fetching or ingesting.
In `@src/providers/adapters/imap/client.ts`:
- Around line 297-337: Add a cumulative byte budget to the FETCH loop that
tracks each message’s raw source size and stops collecting before the batch
exceeds the configured memory limit. Ensure the cursor and returned results
reflect only messages actually collected, while preserving the existing source
and internalDate validation for accepted messages.
- Around line 218-240: Update defaultCreateFlow to use the imported npm ImapFlow
type instead of the hand-written ImapFlowLike and remove the unchecked
whole-struct cast. Narrow the adapter-facing type to only the imapflow methods
used in this file, and convert mailboxOpen().uidValidity from bigint at the
local boundary where the existing code expects the current value type.
In `@src/providers/adapters/imap/fetch.test.ts`:
- Around line 220-231: Add coverage in the fetchImapInboundMessages tests for a
client whose close() rejects: verify a successful fetch still resolves, and
verify a uidFetchRawSince failure still rejects with its original error message.
Reuse the existing createFakeClient and call-log setup, and assert close was
attempted so the swallowing behavior in fetch.ts is exercised.
In `@src/providers/adapters/smtp/sender.test.ts`:
- Around line 133-135: Update the References assertions in the sender test to
extract and validate the actual References header value from wireText, rather
than checking each reference as an unrestricted substring. Assert the joined
references value while accounting for Nodemailer’s possible folded header
formatting, so the test proves the header itself exists and contains the
expected values.
In `@src/providers/adapters/smtp/sender.ts`:
- Around line 145-161: Extract the duplicated withTimeout helper from sender.ts
and verify.ts into a shared timeout module in the SMTP adapter directory,
exporting a label-based API so each caller retains its existing timeout error
context. Update both callers to import and use the shared helper, preserving the
current timer cleanup, rejection, and resolution behavior.
In `@src/providers/adapters/smtp/verify.test.ts`:
- Around line 31-39: Update the test “never calls the real nodemailer transport
factory when a fake transporter is supplied” to spy on or otherwise observe
nodemailer.createTransport, then assert it is not called after
verifySmtpConnection receives the injected fake transporter. Keep the existing
verify invocation assertion, and remove the test only if the factory cannot be
observed reliably.
- Around line 12-40: Add a test in the verifySmtpConnection suite using a
transporter whose verify method never settles and a small timeoutMs, then assert
that verifySmtpConnection rejects when the withTimeout backstop expires. Keep
the existing success and error propagation tests unchanged, and ensure the test
uses the injected fake transporter rather than the real transport factory.
In `@src/store/imap-config.test.ts`:
- Around line 171-191: Strengthen the upsertConfig transaction test around the
existing upsertConfig and db.transaction calls by making the caller-supplied
transaction roll back after the write, then assert getConfig returns no
configuration for the mailbox. Keep the commit-path coverage if needed, but
ensure the rollback assertion proves upsertConfig uses tx rather than the bound
db.
In `@src/store/imap-credentials.test.ts`:
- Around line 164-169: Remove the duplicate test around store.getPassword, or
update it to create an unreadable credential row and assert that result differs
from the missing-row null result; retain the existing missing-row assertion only
if the test explicitly contrasts both states.
In `@web/src/app/mailbox/`[id]/settings/[section]/page.tsx:
- Around line 38-46: Update the conditional rendering in the page component to
render ConnectionSection only when the section key is "connection"; render
PlannedSection as the fallback for planned sections and any other section keys
not handled by this page. Use the existing section identity value, such as
section or sectionDef.key, rather than relying on status alone.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 591-602: Update the IMAP and SMTP failure states in the StatusPill
usages within ConnectInboxForm so they do not pass the semantically unrelated
“spam” status. Introduce or reuse an explicitly named connection-failure status
token that preserves the current critical visual treatment, and apply it to both
failed checks while leaving successful “active” states unchanged.
In `@web/src/components/FolderNav.tsx`:
- Around line 75-88: Extract the shared ChevronDownIcon glyph from the
duplicated component implementations into the ds/core primitives, export it
there, and replace the local definitions in FolderNav, TopBar, ConnectInboxForm,
and InboxSettingsShell with imports. Preserve the existing SVG dimensions,
styling, and accessibility attributes so all consumers render consistently.
In `@web/src/components/MailboxConnectionSection.tsx`:
- Around line 149-151: Update the disabled “Send test email” Button in
MailboxConnectionSection to remove the title-only explanation and render “Not
yet available — no send-test endpoint yet” as adjacent visible text, preserving
the disabled state and clearly associating the reason with the control.
In `@web/src/components/MailboxListScreen.tsx`:
- Around line 12-19: Resolve the documented provider-column decision before
merging: obtain the maintainer’s explicit sign-off for the address-and-status-only card, or
update the GET /api/v1/mailboxes response via toMailboxJson and the mailbox card
in MailboxListScreen to include provider alongside address and status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4810493-896b-4de5-8629-aae74028f277
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonscripts/dev-api.tsspecs/mail/mailbox-connection.mdsrc/api/agents.test.tssrc/api/assistants.test.tssrc/api/conversations.tssrc/api/drafts.test.tssrc/api/imap-connect.test.tssrc/api/imap-connect.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/api/saved-replies.test.tssrc/api/webauthn.test.tssrc/api/webhooks.test.tssrc/composition/app.test.tssrc/composition/app.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.tssrc/mail/delivery-worker.test.tssrc/mail/delivery-worker.tssrc/mail/imap-connect.test.tssrc/mail/imap-connect.tssrc/mail/imap-fetch.test.tssrc/mail/imap-fetch.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/mail/sender-resolver.test.tssrc/mail/sender-resolver.tssrc/providers/adapters/imap/client.test.tssrc/providers/adapters/imap/client.tssrc/providers/adapters/imap/fetch.test.tssrc/providers/adapters/imap/fetch.tssrc/providers/adapters/imap/index.tssrc/providers/adapters/smtp/index.tssrc/providers/adapters/smtp/sender.test.tssrc/providers/adapters/smtp/sender.tssrc/providers/adapters/smtp/verify.test.tssrc/providers/adapters/smtp/verify.tssrc/store/conversations.test.tssrc/store/conversations.tssrc/store/imap-config.test.tssrc/store/imap-config.tssrc/store/imap-credentials.test.tssrc/store/imap-credentials.tssrc/store/imap-watch-state.test.tssrc/store/imap-watch-state.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tstsconfig.jsonvercel.jsonweb/src/app/(shell)/layout.tsxweb/src/app/mailbox/[id]/settings/[section]/page.tsxweb/src/app/manage/mailboxes/new/page.tsxweb/src/app/manage/mailboxes/page.tsxweb/src/components/ConnectInboxForm.tsxweb/src/components/FolderNav.tsxweb/src/components/InboxSettingsShell.tsxweb/src/components/MailboxConnectionSection.tsxweb/src/components/MailboxListScreen.tsxweb/src/components/NewMailboxScreen.tsxweb/src/components/SettingsScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/inbox-settings-sections.tsweb/src/lib/mailbox-actions.ts
Comment on lines
+266
to
+277
| /** | ||
| * The IMAP/SMTP connect/check flow (HT-101 Stage 2a-ii; specs/mail/ | ||
| * mailbox-connection.md §5): ABSENT BY DEFAULT — a deployment that hasn't | ||
| * wired the IMAP/SMTP connect service simply never configures this. When | ||
| * present, `POST /api/v1/inbound/imap/connect` verifies + persists a | ||
| * per-inbox IMAP/SMTP connection and `POST /api/v1/inbound/imap/check` | ||
| * verifies without persisting — see `src/api/imap-connect.ts`. Both are | ||
| * ORDINARY Bearer-gated routes (no pre-auth carve-out — unlike Gmail's | ||
| * OAuth `/callback`). When absent, both routes 404 through the normal | ||
| * authenticated dispatch, exactly like `gmailConnect`'s own absent-case. | ||
| */ | ||
| imapConnect?: ImapConnectDeps |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State the admin requirement in the imapConnect doc.
Lines 273-275 describe both routes as "ORDINARY Bearer-gated routes" and stop there. handleImapConnect and handleImapCheck also require an admin acting Agent via requireAdmin (src/api/imap-connect.ts Line 168). The same omission in that module's own heading is what left the two dialing endpoints un-gated, and it was corrected there. Correct it here for the same reason.
📝 Proposed doc correction
* verifies without persisting — see `src/api/imap-connect.ts`. Both are
* ORDINARY Bearer-gated routes (no pre-auth carve-out — unlike Gmail's
- * OAuth `/callback`). When absent, both routes 404 through the normal
+ * OAuth `/callback`), and both additionally require an ADMIN acting Agent
+ * on top of that gate — both make the server dial an operator-supplied
+ * `host:port`. When absent, both routes 404 through the normal
* authenticated dispatch, exactly like `gmailConnect`'s own absent-case.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * The IMAP/SMTP connect/check flow (HT-101 Stage 2a-ii; specs/mail/ | |
| * mailbox-connection.md §5): ABSENT BY DEFAULT — a deployment that hasn't | |
| * wired the IMAP/SMTP connect service simply never configures this. When | |
| * present, `POST /api/v1/inbound/imap/connect` verifies + persists a | |
| * per-inbox IMAP/SMTP connection and `POST /api/v1/inbound/imap/check` | |
| * verifies without persisting — see `src/api/imap-connect.ts`. Both are | |
| * ORDINARY Bearer-gated routes (no pre-auth carve-out — unlike Gmail's | |
| * OAuth `/callback`). When absent, both routes 404 through the normal | |
| * authenticated dispatch, exactly like `gmailConnect`'s own absent-case. | |
| */ | |
| imapConnect?: ImapConnectDeps | |
| /** | |
| * The IMAP/SMTP connect/check flow (HT-101 Stage 2a-ii; specs/mail/ | |
| * mailbox-connection.md §5): ABSENT BY DEFAULT — a deployment that hasn't | |
| * wired the IMAP/SMTP connect service simply never configures this. When | |
| * present, `POST /api/v1/inbound/imap/connect` verifies + persists a | |
| * per-inbox IMAP/SMTP connection and `POST /api/v1/inbound/imap/check` | |
| * verifies without persisting — see `src/api/imap-connect.ts`. Both are | |
| * ORDINARY Bearer-gated routes (no pre-auth carve-out — unlike Gmail's | |
| * OAuth `/callback`), and both additionally require an ADMIN acting Agent | |
| * on top of that gate — both make the server dial an operator-supplied | |
| * `host:port`. When absent, both routes 404 through the normal | |
| * authenticated dispatch, exactly like `gmailConnect`'s own absent-case. | |
| */ | |
| imapConnect?: ImapConnectDeps |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/index.ts` around lines 266 - 277, Update the imapConnect
documentation to state that POST /api/v1/inbound/imap/connect and POST
/api/v1/inbound/imap/check require an authenticated admin acting Agent, in
addition to ordinary Bearer authentication. Keep the existing absent-by-default
and 404 behavior description unchanged, aligning the wording with the
requireAdmin enforcement in handleImapConnect and handleImapCheck.
Comment on lines
+579
to
+583
| it('refuses to retry a row whose persisted fromAddress no longer matches its resolved transport (mailbox deleted → default) — fails it, never sends mismatched', async () => { | ||
| const { store } = await freshStore() | ||
| // A conversation with no mailbox (mailbox_id null, as if its inbox was | ||
| // hard-deleted) but a row originally sent as a SPECIFIC inbox address. | ||
| const { conversationId } = await seedConversation(store) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name and comment reuse the retracted hard-delete explanation.
Migration 028 uses ON DELETE RESTRICT. A mailbox that owns conversations cannot be deleted, so "mailbox deleted → default" and "as if its inbox was hard-deleted" describe an unreachable state. src/store/conversations.ts Lines 369-376 and the guard comment in src/mail/delivery-worker.ts both retract this reasoning. Restate the scenario as the reachable one: a pre-028 conversation with a null mailbox_id whose persisted fromAddress differs from the resolved default.
📝 Proposed wording fix
- it('refuses to retry a row whose persisted fromAddress no longer matches its resolved transport (mailbox deleted → default) — fails it, never sends mismatched', async () => {
+ it('refuses to retry a row whose persisted fromAddress no longer matches its resolved transport (pre-028 null mailbox_id → default) — fails it, never sends mismatched', async () => {
const { store } = await freshStore()
- // A conversation with no mailbox (mailbox_id null, as if its inbox was
- // hard-deleted) but a row originally sent as a SPECIFIC inbox address.
+ // A pre-028 conversation with no recorded mailbox (mailbox_id null) but
+ // a row originally sent as a SPECIFIC inbox address.
const { conversationId } = await seedConversation(store)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('refuses to retry a row whose persisted fromAddress no longer matches its resolved transport (mailbox deleted → default) — fails it, never sends mismatched', async () => { | |
| const { store } = await freshStore() | |
| // A conversation with no mailbox (mailbox_id null, as if its inbox was | |
| // hard-deleted) but a row originally sent as a SPECIFIC inbox address. | |
| const { conversationId } = await seedConversation(store) | |
| it('refuses to retry a row whose persisted fromAddress no longer matches its resolved transport (pre-028 null mailbox_id → default) — fails it, never sends mismatched', async () => { | |
| const { store } = await freshStore() | |
| // A pre-028 conversation with no recorded mailbox (mailbox_id null) but | |
| // a row originally sent as a SPECIFIC inbox address. | |
| const { conversationId } = await seedConversation(store) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/delivery-worker.test.ts` around lines 579 - 583, Update the test
name and adjacent setup comment in the delivery-worker test to describe the
reachable pre-028 scenario: a conversation with null mailbox_id whose persisted
fromAddress differs from the resolved default address. Remove references to
mailbox deletion, hard deletion, or “mailbox deleted → default,” while
preserving the test’s mismatch and no-send behavior.
Comment on lines
+71
to
+79
| * ## Persist is ONE transaction — atomicity mirrors `./gmail-connect.ts` step 5 | ||
| * | ||
| * `MailboxStore.upsertConnectedMailbox` → `ImapConfigStore.upsertConfig` → | ||
| * `ImapCredentialStore.upsertPassword` → `ImapWatchStateStore.seedBaseline`, | ||
| * all against the SAME `tx` — a mid-persist failure (a constraint violation, | ||
| * a dropped connection) rolls back the WHOLE unit, never a partial connect | ||
| * (e.g. a mailbox row with no baseline cursor, which `./imap-fetch.ts`'s | ||
| * `runImapFetch` would then skip forever as "missing cursor," silently | ||
| * un-ingesting — worse than no mailbox row at all). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the persist-step doc: the code calls seedBaselineIfAbsent, not seedBaseline.
Line 74 names ImapWatchStateStore.seedBaseline, but connect calls watchStateStore.seedBaselineIfAbsent (Line 399). The difference is the reconnect invariant: seedBaseline overwrites an existing cursor, seedBaselineIfAbsent preserves it. The inline comment at Lines 392-398 documents that invariant correctly, so only this heading is wrong. A reader who trusts the module doc can conclude a reconnect re-baselines the cursor.
📝 Proposed doc correction
* `MailboxStore.upsertConnectedMailbox` → `ImapConfigStore.upsertConfig` →
- * `ImapCredentialStore.upsertPassword` → `ImapWatchStateStore.seedBaseline`,
+ * `ImapCredentialStore.upsertPassword` →
+ * `ImapWatchStateStore.seedBaselineIfAbsent` (a reconnect PRESERVES the
+ * existing cursor — see the persist step's own comment),
* all against the SAME `tx` — a mid-persist failure (a constraint violation,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * ## Persist is ONE transaction — atomicity mirrors `./gmail-connect.ts` step 5 | |
| * | |
| * `MailboxStore.upsertConnectedMailbox` → `ImapConfigStore.upsertConfig` → | |
| * `ImapCredentialStore.upsertPassword` → `ImapWatchStateStore.seedBaseline`, | |
| * all against the SAME `tx` — a mid-persist failure (a constraint violation, | |
| * a dropped connection) rolls back the WHOLE unit, never a partial connect | |
| * (e.g. a mailbox row with no baseline cursor, which `./imap-fetch.ts`'s | |
| * `runImapFetch` would then skip forever as "missing cursor," silently | |
| * un-ingesting — worse than no mailbox row at all). | |
| * ## Persist is ONE transaction — atomicity mirrors `./gmail-connect.ts` step 5 | |
| * | |
| * `MailboxStore.upsertConnectedMailbox` → `ImapConfigStore.upsertConfig` → | |
| * `ImapCredentialStore.upsertPassword` → | |
| * `ImapWatchStateStore.seedBaselineIfAbsent` (a reconnect PRESERVES the | |
| * existing cursor — see the persist step's own comment), | |
| * all against the SAME `tx` — a mid-persist failure (a constraint violation, | |
| * a dropped connection) rolls back the WHOLE unit, never a partial connect | |
| * (e.g. a mailbox row with no baseline cursor, which `./imap-fetch.ts`'s | |
| * `runImapFetch` would then skip forever as "missing cursor," silently | |
| * un-ingesting — worse than no mailbox row at all). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/imap-connect.ts` around lines 71 - 79, Correct the persist-step
documentation to reference ImapWatchStateStore.seedBaselineIfAbsent instead of
ImapWatchStateStore.seedBaseline, matching the call in connect and preserving
the documented reconnect behavior that retains an existing cursor. Do not change
the implementation or surrounding transaction description.
Comment on lines
+61
to
+66
| /** The two failure shapes {@link SenderResolutionError} can carry — every one is a "this reply cannot be sent as configured" condition, never a transient fault. */ | ||
| export type SenderResolutionErrorCode = | ||
| | 'mailbox-not-found' | ||
| | 'missing-imap-connection' | ||
| | 'unreadable-imap-credential' | ||
| | 'unknown-provider' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the count in the SenderResolutionErrorCode doc.
The comment says "two failure shapes". The union declares four: mailbox-not-found, missing-imap-connection, unreadable-imap-credential, unknown-provider.
📝 Proposed doc correction
-/** The two failure shapes {`@link` SenderResolutionError} can carry — every one is a "this reply cannot be sent as configured" condition, never a transient fault. */
+/** The failure shapes {`@link` SenderResolutionError} can carry — every one is a "this reply cannot be sent as configured" condition, never a transient fault. */
export type SenderResolutionErrorCode =📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** The two failure shapes {@link SenderResolutionError} can carry — every one is a "this reply cannot be sent as configured" condition, never a transient fault. */ | |
| export type SenderResolutionErrorCode = | |
| | 'mailbox-not-found' | |
| | 'missing-imap-connection' | |
| | 'unreadable-imap-credential' | |
| | 'unknown-provider' | |
| /** The failure shapes {`@link` SenderResolutionError} can carry — every one is a "this reply cannot be sent as configured" condition, never a transient fault. */ | |
| export type SenderResolutionErrorCode = | |
| | 'mailbox-not-found' | |
| | 'missing-imap-connection' | |
| | 'unreadable-imap-credential' | |
| | 'unknown-provider' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/sender-resolver.ts` around lines 61 - 66, Update the documentation
immediately above SenderResolutionErrorCode to state that SenderResolutionError
can carry four failure shapes, matching all members of the union; leave the
error codes and behavior unchanged.
Comment on lines
+40
to
+51
| /** Persistence for one mailbox's IMAP/SMTP app password. See the module doc for the encrypt/decrypt and write-only-internal-use contracts. */ | ||
| /** | ||
| * The stored ciphertext for `mailboxId` could not be decrypted — a wrong | ||
| * `HELPTHREAD_TOKEN_ENC_KEY`, or a tampered/corrupt row. Distinct from a | ||
| * database fault, deliberately: callers contain THIS (one mailbox is | ||
| * misconfigured; the others are fine) while letting a store fault propagate, | ||
| * because a transient database problem must be retried, not recorded as a | ||
| * permanent per-mailbox failure (review, 2026-07-31). | ||
| * | ||
| * Carries no ciphertext and no key material — only which mailbox. | ||
| */ | ||
| export class ImapCredentialDecryptError extends Error { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the orphaned ImapCredentialStore doc comment to the interface.
Line 40 documents ImapCredentialStore, but the declaration that follows it is ImapCredentialDecryptError. Two doc blocks now stack on the same class, and the interface at line 61 has none. Tooling attaches the first block to the wrong symbol.
♻️ Proposed fix
-/** Persistence for one mailbox's IMAP/SMTP app password. See the module doc for the encrypt/decrypt and write-only-internal-use contracts. */
/**
* The stored ciphertext for `mailboxId` could not be decrypted — a wrongThen above line 61:
+/** Persistence for one mailbox's IMAP/SMTP app password. See the module doc for the encrypt/decrypt and write-only-internal-use contracts. */
export interface ImapCredentialStore {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/imap-credentials.ts` around lines 40 - 51, Move the
persistence-contract doc comment currently immediately before
ImapCredentialDecryptError so it directly documents the ImapCredentialStore
interface. Leave the decrypt-error documentation attached only to
ImapCredentialDecryptError, and ensure the interface no longer lacks its
intended documentation.
Comment on lines
+540
to
+586
| <button | ||
| type="button" | ||
| aria-pressed={secure} | ||
| onClick={() => { | ||
| setSecure((current) => !current) | ||
| clearCheckState() | ||
| }} | ||
| style={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: 8, | ||
| border: '1px solid var(--ht-border)', | ||
| background: 'var(--ht-surface)', | ||
| borderRadius: 'var(--ht-radius-md)', | ||
| padding: '10px 12px', | ||
| cursor: 'pointer', | ||
| width: '100%', | ||
| textAlign: 'left', | ||
| }} | ||
| > | ||
| <span | ||
| aria-hidden="true" | ||
| style={{ | ||
| width: 32, | ||
| height: 18, | ||
| borderRadius: 999, | ||
| background: secure ? 'var(--ht-accent)' : 'var(--ht-surface-2)', | ||
| position: 'relative', | ||
| flexShrink: 0, | ||
| transition: 'background 0.15s', | ||
| }} | ||
| > | ||
| <span | ||
| style={{ | ||
| position: 'absolute', | ||
| top: 2, | ||
| left: secure ? 16 : 2, | ||
| width: 14, | ||
| height: 14, | ||
| borderRadius: '50%', | ||
| background: 'var(--ht-surface)', | ||
| transition: 'left 0.15s', | ||
| }} | ||
| /> | ||
| </span> | ||
| <span style={{ fontSize: 13, fontWeight: 600, color: 'var(--ht-ink)' }}>Use TLS</span> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The "Use TLS" toggle now controls the SMTP leg only. Correct the copy.
secure is sent as a single flag and reaches the engine as ImapConnectInput.secure. The engine applies it to the SMTP leg only: attemptSmtpVerification passes secure: input.secure ?? true, while attemptImapConnection ignores it and derives IMAP TLS from the port through imapImplicitTlsForPort (src/mail/imap-connect.ts Lines 261-266, 300). An operator who turns this toggle off expects both legs to change. For the port 587 presets (Outlook, iCloud) the label also reads as "TLS off" when the real meaning is STARTTLS.
Rename the control to name the SMTP leg, or add helper text stating that IMAP TLS follows the IMAP port. Copy changes on this surface need the maintainer's sign-off.
As per coding guidelines: "The Agent Inbox UI and dogfood site must match the Claude Design prototype exactly across the whole designed surface, including visual details, copy, and interactions; deviations require the maintainer's explicit sign-off."
📝 Proposed copy fix
- <span style={{ fontSize: 13, fontWeight: 600, color: 'var(--ht-ink)' }}>Use TLS</span>
+ <span style={{ fontSize: 13, fontWeight: 600, color: 'var(--ht-ink)' }}>
+ Use implicit TLS for SMTP
+ </span>
</button>
+ <p style={{ margin: 0, fontSize: 11.5, color: 'var(--ht-ink-dim)' }}>
+ Off means STARTTLS. IMAP TLS follows the IMAP port automatically.
+ </p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type="button" | |
| aria-pressed={secure} | |
| onClick={() => { | |
| setSecure((current) => !current) | |
| clearCheckState() | |
| }} | |
| style={{ | |
| display: 'flex', | |
| alignItems: 'center', | |
| gap: 8, | |
| border: '1px solid var(--ht-border)', | |
| background: 'var(--ht-surface)', | |
| borderRadius: 'var(--ht-radius-md)', | |
| padding: '10px 12px', | |
| cursor: 'pointer', | |
| width: '100%', | |
| textAlign: 'left', | |
| }} | |
| > | |
| <span | |
| aria-hidden="true" | |
| style={{ | |
| width: 32, | |
| height: 18, | |
| borderRadius: 999, | |
| background: secure ? 'var(--ht-accent)' : 'var(--ht-surface-2)', | |
| position: 'relative', | |
| flexShrink: 0, | |
| transition: 'background 0.15s', | |
| }} | |
| > | |
| <span | |
| style={{ | |
| position: 'absolute', | |
| top: 2, | |
| left: secure ? 16 : 2, | |
| width: 14, | |
| height: 14, | |
| borderRadius: '50%', | |
| background: 'var(--ht-surface)', | |
| transition: 'left 0.15s', | |
| }} | |
| /> | |
| </span> | |
| <span style={{ fontSize: 13, fontWeight: 600, color: 'var(--ht-ink)' }}>Use TLS</span> | |
| </button> | |
| <button | |
| type="button" | |
| aria-pressed={secure} | |
| onClick={() => { | |
| setSecure((current) => !current) | |
| clearCheckState() | |
| }} | |
| style={{ | |
| display: 'flex', | |
| alignItems: 'center', | |
| gap: 8, | |
| border: '1px solid var(--ht-border)', | |
| background: 'var(--ht-surface)', | |
| borderRadius: 'var(--ht-radius-md)', | |
| padding: '10px 12px', | |
| cursor: 'pointer', | |
| width: '100%', | |
| textAlign: 'left', | |
| }} | |
| > | |
| <span | |
| aria-hidden="true" | |
| style={{ | |
| width: 32, | |
| height: 18, | |
| borderRadius: 999, | |
| background: secure ? 'var(--ht-accent)' : 'var(--ht-surface-2)', | |
| position: 'relative', | |
| flexShrink: 0, | |
| transition: 'background 0.15s', | |
| }} | |
| > | |
| <span | |
| style={{ | |
| position: 'absolute', | |
| top: 2, | |
| left: secure ? 16 : 2, | |
| width: 14, | |
| height: 14, | |
| borderRadius: '50%', | |
| background: 'var(--ht-surface)', | |
| transition: 'left 0.15s', | |
| }} | |
| /> | |
| </span> | |
| <span style={{ fontSize: 13, fontWeight: 600, color: 'var(--ht-ink)' }}> | |
| Use implicit TLS for SMTP | |
| </span> | |
| </button> | |
| <p style={{ margin: 0, fontSize: 11.5, color: 'var(--ht-ink-dim)' }}> | |
| Off means STARTTLS. IMAP TLS follows the IMAP port automatically. | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/components/ConnectInboxForm.tsx` around lines 540 - 586, Update the
control copy in the ConnectInboxForm “Use TLS” toggle to clarify that it applies
only to SMTP, and indicate that IMAP TLS is determined by the IMAP port
(including STARTTLS behavior for port 587). Preserve the existing toggle
behavior and obtain the maintainer’s sign-off for the copy change to maintain the required
prototype match.
Source: Coding guidelines
…ables `lock_down_data_api` (#170) took migration id 27 on main while this branch was in review, and this branch already used 27 and 28. Two problems existed only in the combination — neither branch was wrong on its own. **The IMAP tables would never have been created.** `id` is the applied-once key. With main's 027 already recorded, shipping a second 027 would have been read as already-applied and SKIPPED: no tables, no error, IMAP intake simply dead on the next deploy. HT-101's migrations are now 028 (`imap_transport`) and 029 (`conversation_mailbox_id`), with a comment at the array explaining why the numbering must not be "tidied" back. **The three IMAP tables would have shipped without RLS.** Migration 027 enables row-level security on every table that existed when it ran; ours are created afterwards, so it cannot reach them. Without this they are queryable through the Supabase PostgREST Data API, and `imap_mailbox_credentials` holds encrypted app passwords. Migration 028 now enables RLS on all three, per the standing rule 027's own doc states: "a migration that adds a table MUST also ENABLE ROW LEVEL SECURITY on it." Main's own RLS test is what caught this — it asserts no table in `public` lacks RLS and failed against the merge. Kept as-is; it now covers HT-101's tables too. Conflicts resolved by taking main's `migrate.ts`/`migrate.test.ts` wholesale and re-applying HT-101's two migrations and its migration test on top, rather than hand-splicing the conflict regions — an earlier splice mangled a doc block and a template literal. Gates on the merged tree: typecheck, web typecheck, web build, lint, gitleaks all exit 0; 87 files / 1742 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
Adjudication — CodeRabbit review on
|
| # | Finding | Verdict |
|---|---|---|
| 1 | verify.ts's new withTimeout race has no test — "a regression that removes the race would still pass this suite" |
Real, accepted. Fair and the same class of gap this PR fixed elsewhere: a guard with no proof. Filed, not fixed here. |
| 2 | imap-fetch.ts's missing-config-credential-or-cursor skip branch has no test |
Real, accepted. Same. Filed. |
| 3 | Narrow defaultCreateFlow's as unknown as ImapFlowLike cast to the imapflow surface actually used |
Declined. The hand-written ImapFlowLike exists so client.test.ts can inject a fake without a real IMAP server; importing the npm type would reintroduce the bigint uidValidity this adapter deliberately narrows once at the boundary. Marked low-value by the reviewer, and it is. |
| 4–6 | Style and doc-phrasing nitpicks | Declined. No behaviour, no correctness. |
Items 1 and 2 are genuine coverage gaps in code added during review, and they go in the follow-up pile rather than extending a PR that has already been through six rounds.
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🟢 SAFE TO MERGE
Gates green. The five decisions this encodes were reviewed and approved by the maintainer. Six review rounds adjudicated.
CodeRabbit — 3 rounds, through
51b9faf: every finding adjudicated; the real ones fixed, 2 rejected with stated reasons, 3 stale re-reports. Codex (adversarial, in place of CodeRabbit — rate limited, then "incremental reviews are disabled") — 3 passes, 8 findings, all real, all fixed.Connect an inbox with an address and an app password instead of ~26 steps across four consoles. IMAP for intake on a 2-minute cron, SMTP for replies, per-inbox — no Google Cloud project, no Pub/Sub, no new DNS.
Decision provenance
Message-IDand ignoredacceptedis deliberately left to the maintainerAll five rules above were put to the maintainer in plain language and approved verbatim. The spec's promotion from
DRAFTtoacceptedwas not part of that approval and remains open.One-way door
Three new tables (
imap_mailbox_config,imap_mailbox_credentials,imap_watch_state) and a new cron entry every 2 minutes. Once these run against production they are migrations to live data, not a revert.App passwords are stored encrypted (AES-256-GCM,
HELPTHREAD_TOKEN_ENC_KEY), never returned by any read endpoint.The three findings that mattered
Provider conversion double-ingested every message.
upsertConnectedMailboxrewroteprovideron conflict, so connecting an already-Gmail address over IMAP flipped the row while its OAuth token and Gmail cursor stayed behind. The mailbox then satisfied both intake paths —runImapFetchselects onprovider,runGmailReconcileSweeponstatusalone — and the two transports mint differentproviderMessageIdvalues for one physical message, so the ledger could not dedupe across them. Now refused atomically as 409provider_conflict. A test asserted the old behaviour as intended; it is replaced.A stale lease holder could escape the UIDVALIDITY-reset quarantine. The cursor write was unconditional, so a run whose lease expired mid-ingest could overwrite the state of a mailbox a successor had already paused.
setCursoris now lease-fenced. The token itself was the weakness — it wasclaimed_until, a timestamp, and two claims in one clock tick mint identical tokens. The test written to prove the fence instead proved it did not hold.lease_tokenis now a per-claim uuid.Both IMAP endpoints dialed operator-supplied hosts on the service Bearer alone. Any API-token holder could use
/imap/checkas a network probe. Both now require an admin Agent, checked before the body is parsed.Still owed — read before calling IMAP intake done
The spec's blocking question #2 is contained, not resolved. IMAP has no transport-stable message id; the adapter mints
imap:{uidValidity}:{uid}, stable only within one epoch. Nothing duplicates today because a reset pauses the mailbox. The identity problem is real and still open, and the adapter says so in its own doc.Questions #1 (self-echo) and #3 (the seam) are resolved. #3 by declining the seam:
InboundEmailProvideris webhook-shaped and a cron fetch has noRequest. The spec was wrong; the code is right.Verification
npm run typechecknpm run -w web typechecknpm run -w web buildnpm run lintnpm testgitleaksLive smoke test
Against
help@resonantiq.app's real mail server over a throwaway PGlite database — real network, zero production rows touched, live Gmail intake on that address untouched throughout.checkConnection— both legsconnect— atomic persistactive, config stored, baseline cursor{uidValidity:1, lastUid:37}(=uidNext-1)imapover agmailrow → refused (provider_conflict)UID FETCHimap:1:35/36/37), cursor advanced 34 → 37Browser end-to-end
web/has no test infrastructure at all, so the acting-Agent header fix had no automated coverage — the same gap that let that bug through. Verified by driving the real UI: dev API + web dev server, first-admin setup, then the connect screen at/manage/mailboxes/newpointed at Gmail with a deliberately wrong password.Gmail's own rejection — not our 401. The request carried the acting-Agent header, passed the admin gate, and opened a real TLS connection. Both legs reported independently; the error is sanitized with no credential echoed. Before the fix this returned 401 and never left the process.
The real app password was never typed into the browser. A wrong one is the sharper test: it distinguishes "our API refused" from "the mail server refused," which is exactly the regression in question.
Rebase note
Rebased onto
mainafter #107 (HT-94) landed mid-review. That branch added a Gmailreconcile-sweepcron; this one adds animap-fetchcron. Every conflict resolved as keep-both — independent ticks over one shared ingest pipeline, and a test now pins that independence.Follow-ups filed, not fixed here
IMAP fetch: implement the §5 invocation clock budget #167 — the §5 invocation clock budget (spec'd, unbuilt; starvation, not loss)
Gmail reconcile lease: timestamp-derived token can collide #168 — the same timestamp-token collision in
gmail_watch_state's leaseIMAP/SMTP connect: no outbound host/port allowlist (SSRF surface) #169 — no outbound host/port allowlist for mail connections (authorization added here; target validation not)
Make TLS mode explicit per leg (IMAP and SMTP), and review the provider presets #173 — make TLS explicit per leg, and review the provider presets (partial fix landed here)
web/ has no test infrastructure at all #172 —
web/has zero test infrastructure. Filed because this PR shipped a browser-only bug that every engine gate passed straight through.Round six — the TLS rule was tried three times, so it stopped being derived
port === 993 || securebroke IMAP on 143 alongside SMTP on 465 — an ordinary self-hosted setup, wheresecure: truedescribes the SMTP leg and forced a TLS handshake against a plaintext IMAP greeting.That was the third rule in one cycle, each fixing one real configuration by breaking another:
secureto both legssecure:false— every Outlook/iCloud presetport === 993secure:true— non-standard implicit-TLS portport === 993 || securesecure:true— IMAP STARTTLS with SMTPSNo fourth rule was attempted. One boolean cannot describe two independent connections; each candidate just relocates which operator is broken. This ships the second rule — the rarest, least reachable failure, unreachable from any preset — with all three rules and their failures recorded in the function's doc and a test pinning the known gap. #173 carries the real fix and has been raised in priority with this evidence.
Every wrong answer fails loudly at connect time. Nothing downgrades silently to cleartext.
Round five — Codex in place of CodeRabbit, and it found defects in the fixes
CodeRabbit could not review
9af930f("Your included review limit is currently reached"), so per the substitution rule an adversarial Codex pass ran instead — scoped deliberately to whether rounds 1–4 introduced new defects rather than re-reviewing the feature. All three findings were in fixes from those rounds:getPassword, including database faults, and the delivery worker marks those rowsfailed— so a transient Postgres blip would have permanently failed mail that only needed retrying. One blast radius traded for another. Now a typedImapCredentialDecryptErroris contained and everything else propagates.secure: true, would have had STARTTLS forced on it.Codex explicitly cleared the lease fencing, the provider guard on fresh inserts, and the
close()guards.Round four, worth calling out
Every Outlook and iCloud preset was broken on arrival. One
secureflag fed both connections, but the presets pair IMAP 993 with SMTP 587 — so those presets attempted STARTTLS against an implicit-TLS IMAP port, which cannot succeed. TLS is now derived per leg from its own port, with a regression test built from the exact preset shape that failed. The fuller fix (explicit per-leg fields in API, schema, and form) needs a product call and is #173.A mailbox-list failure took down the entire inbox. The shell layout wraps every inbox and conversation route; an engine error on a call feeding one gear menu removed the folder rail, counts, and conversation view. Contained — 401 still propagates to re-login on purpose.
Round three, worth calling out
The provider-conflict guard's own error message said "disconnect it first." That instruction was false:
markDisconnectedonly changesstatus, leavingproviderset, so the retry hits the same conflict forever. The message now says changing an inbox's transport is unsupported rather than sending an operator round a loop that cannot terminate. Making it supported — deciding what happens to the old transport's tokens, cursor, and in-flight mail — is a product decision left open.Also fixed: one undecryptable credential aborted every mailbox's outbound retries (now contained per-row), and the password-echo test was passing vacuously against 401 bodies after the routes became admin-gated.
Before merge
51b9faf; rounds four to six covered by the Codex substitution (disclosed above).Open, and deliberately not blocking this merge: the spec stays
DRAFT. Promotingspecs/mail/mailbox-connection.mdtoacceptedis a separate call.🤖 Generated with Claude Code
Summary by CodeRabbit