feat: email integration for BetterAuth callbacks#2021
feat: email integration for BetterAuth callbacks#2021nick-inkeep wants to merge 25 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 12bb8c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
nick-inkeep
force-pushed
the
feat/email-integration
branch
from
2177053 to
c66434f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(9) Total Issues | Risk: Medium
🟠⚠️ Major (5) 🟠⚠️
🟠 1) invitations.ts:111 Missing authorization check on email-status endpoint
Issue: The new /manage/api/invitations/:id/email-status endpoint is protected by sessionAuth() but does not verify that the authenticated user has permission to view this invitation's status. Any authenticated user can query the email status of any invitation ID.
Why: This breaks the authorization pattern established by peer endpoints in this file. The /pending endpoint restricts queries to the authenticated user's own email. While the exposed data is limited (only emailSent boolean and optional error with 5-minute TTL), this allows cross-tenant status enumeration — an authenticated user from Org A could probe invitation IDs from other organizations.
Fix: Add authorization to verify the caller is an admin/owner of the organization that owns the invitation:
invitationsRoutes.get('/:id/email-status', async (c) => {
const invitationId = c.req.param('id');
const auth = c.get('auth');
const session = c.get('session');
// Look up invitation to get organizationId
const invitation = await auth.api.getInvitation({ query: { id: invitationId } });
if (!invitation) {
return c.json({ emailSent: false });
}
// Verify caller is admin of the invitation's org
const membership = await auth.api.getMember({
query: { organizationId: invitation.organizationId, userId: session.userId }
});
if (!membership || (membership.role !== 'admin' && membership.role !== 'owner')) {
throw createApiError({ code: 'forbidden', message: 'Not authorized' });
}
const status = getEmailSendStatus(invitationId);
return c.json(status ?? { emailSent: false });
});Alternatively, store organizationId alongside the status in setEmailSendStatus() to enable tenant-scoped lookup without an additional API call.
Refs:
🟠 2) auth.ts:226-234 Password reset email failure silently swallowed with no user feedback
Issue: When emailService.sendPasswordResetEmail() fails, the catch block logs the error but doesn't propagate failure status to the caller or store it for later retrieval. Unlike invitation emails which use setEmailSendStatus(), password reset has no equivalent status tracking.
Why: When SMTP is configured but email sending fails (server down, auth error, timeout), the user sees "Check your email" success message but never receives the email. The user has no way to know the email failed and may wait indefinitely. The reset link is stored via setPasswordResetLink() but the user cannot access it without the email.
Fix: Add a parallel status store for password reset emails, similar to setEmailSendStatus():
// In sendResetPassword callback:
if (config.emailService?.isConfigured) {
try {
const result = await config.emailService.sendPasswordResetEmail({
to: user.email,
resetUrl: url,
});
setPasswordResetEmailStatus(user.email, {
emailSent: result.emailSent,
error: result.error
});
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
console.error('[email] Failed to send password reset email:', err);
setPasswordResetEmailStatus(user.email, { emailSent: false, error: message });
}
}Then expose a status endpoint or extend the forgot-password UI to show the reset link directly when email fails (similar to invitation fallback behavior).
Refs:
🟠 3) packages/email/ Missing LICENSE.md and not in license sync script
Issue: The packages/email/package.json declares files: ["dist", "README.md", "LICENSE.md"] but no LICENSE.md exists. The package also isn't included in scripts/sync-licenses.mjs which auto-generates LICENSE.md for packages during build.
Why: This will cause npm publish to fail or ship without proper licensing. The license sync script runs automatically during build via Turbo's task dependencies.
Fix: Add ./packages/email to the targetPackages array in scripts/sync-licenses.mjs:
const targetPackages = [
'./packages/agents-core',
'./packages/agents-sdk',
// ... existing packages ...
'./packages/email', // Add this
];Refs:
🟠 4) agents-docs/ Missing documentation for email integration feature
Issue: This PR introduces significant customer-facing functionality (SMTP/Resend email configuration, automatic invitation emails, self-service forgot-password flow, Mailpit for local dev) but has no documentation updates in agents-docs/content/.
Why: Self-hosted users will not know how to configure email delivery. They will see invitation emails fail silently (UI shows "Copy link" as fallback) without understanding why. Users won't discover the forgot-password feature exists since it's hidden when SMTP is not configured. Local developers won't know about Mailpit for testing emails.
Fix: Update agents-docs/content/deployment/(docker)/authentication.mdx to add an "Email Configuration" section documenting:
- When email is needed (invitations, password resets)
- SMTP environment variables (
SMTP_HOST,SMTP_PORT,SMTP_USER,SMTP_PASSWORD,SMTP_SECURE,SMTP_FROM_ADDRESS,SMTP_FROM_NAME,SMTP_REPLY_TO) - Resend alternative (
RESEND_API_KEY) - Behavior when not configured (copy-link fallback, forgot-password hidden)
- Local testing with Mailpit
Consider also updating agents-docs/content/visual-builder/access-control.mdx "Managing Team Members" section to mention that invitation emails are sent automatically when SMTP is configured.
Refs:
🟠 5) auth.ts:319-352 + email/__tests__/ Missing test coverage for critical auth callbacks
Issue: The sendInvitationEmail callback in BetterAuth configuration (auth.ts lines 319-352) and the integration path from createEmailService → sendInvitationEmail have no unit tests. The callback contains critical logic: constructing invitation URLs from env vars, calling emailService, calling setEmailSendStatus with success/failure, and exception handling.
Why: Multiple regression scenarios could go undetected:
- If
INKEEP_AGENTS_MANAGE_UI_URLenv var handling breaks, invitation URLs would be malformed - If the try/catch is removed, exceptions would propagate and break the invitation flow
- If
setEmailSendStatusis called incorrectly, the UI would show incorrect email status
Similarly, service.test.ts only tests isConfigured states but doesn't verify that sendInvitationEmail actually calls the transporter when the service IS configured.
Fix: Either:
- Extract the callback logic into a testable function and add unit tests
- Add integration tests that exercise the full BetterAuth flow
- In
service.test.ts, add a test that creates a configured service with mocked transport and verifiessendMailis called with expected params
Refs:
Inline Comments:
- 🟠 Major:
docker-compose.yml:165Pin Mailpit image to specific version
🟡 Minor (2) 🟡
🟡 1) packages/email/package.json Missing README.md and private field
Issue: The package declares files: ["dist", "README.md", "LICENSE.md"] but no README.md exists. Additionally, the package has no private: true field and is not in the changeset ignore array, so it may be published unintentionally.
Why: Other packages in the monorepo have README.md files. If this is an internal-only package, it should be marked private. The changeset roasted-cyan-asp.md only bumps agents-core and agents-api, not the new email package.
Fix: Either:
- Add
"private": trueto package.json and add@inkeep/emailto.changeset/config.jsonignore array - Or add a README.md and create a changeset for the new package if it should be published
Refs:
🟡 2) layout.tsx:106-111 PUBLIC_IS_SMTP_CONFIGURED detection logic divergent from transport factory
Issue: The UI checks (RESEND_API_KEY || SMTP_HOST) && SMTP_FROM_ADDRESS to determine if SMTP is configured, while the transport factory validates SMTP_FROM_ADDRESS only after seeing RESEND_API_KEY or SMTP_HOST and logs a warning if missing. This divergence can cause the UI to show "Forgot password?" when the server-side email service is actually disabled.
Why: If SMTP_HOST is set but SMTP_FROM_ADDRESS is missing, the UI shows the forgot-password link but the server won't send emails. Users would submit the form but never receive an email.
Fix: Expose isConfigured from the email service via an API endpoint (e.g., /api/config) rather than re-deriving it client-side from raw env vars. Alternatively, ensure the UI logic exactly mirrors the transport factory validation.
Refs:
Inline Comments:
- 🟡 Minor:
invitations.ts:111Add internal route comment for consistency - 🟡 Minor:
forgot-password/page.tsx:101-103Log error for debugging - 🟡 Minor:
invite-member-dialog.tsx:186-188Log email status check failures - 🟡 Minor:
invite-member-dialog.tsx:377-384Email failure warning condition is fragile
💭 Consider (4) 💭
💭 1) types.ts:1-4 + transport.ts:5-8 Type design allows illegal states
Issue: SendResult and TransportResult allow semantically invalid combinations:
{ emailSent: true, error: 'message' }— success with error{ transporter: Transporter, isConfigured: false }— has transporter but marked unconfigured
Fix: Use discriminated unions to make illegal states unrepresentable:
export type SendResult =
| { emailSent: true }
| { emailSent: false; error: string };Refs:
💭 2) email-send-status-store.ts In-memory store won't work in multi-instance deployments
Issue: The store is populated on the instance that handles the BetterAuth callback, but the subsequent GET /invitations/:id/email-status request may hit a different instance, returning { emailSent: false } even when the email was successfully sent.
Why: Self-hosted deployments may scale horizontally. This is a known limitation documented in the PR description ("mirrors password-reset-link-store.ts pattern").
Fix: Document this limitation in the SMTP configuration docs (single-instance assumption), or consider persisting email send status in the database or Redis for multi-instance deployments.
💭 3) forgot-password/page.tsx:52-81 Success message could mention spam folder
Issue: The success message "If an account exists with that email, we sent a password reset link" doesn't mention what to do if the user doesn't receive the email.
Fix: Add spam folder guidance: "Check your inbox and spam folder. If you don't see it after a few minutes, you can try again."
💭 4) invite-member-dialog.tsx:153-212 Sequential async operations create waterfall
Issue: The handleSubmit function iterates through emailList and awaits each invitation API call sequentially. For N emails, users wait for N sequential calls instead of parallel execution.
Fix: Consider using Promise.allSettled for parallel execution if the backend can handle concurrent requests.
Inline Comments:
- 💭 Consider:
forgot-password/page.tsx:28Addaria-hiddento decorative icons
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-structured email integration feature with good graceful degradation design. The architecture is sound (transport factory, React Email templates, in-memory bridge pattern), and the code quality is generally good.
Key areas to address:
- Security: The email-status endpoint needs authorization to prevent cross-tenant enumeration
- User experience: Password reset email failures need a feedback path (status tracking like invitations)
- Publishing: The new package needs LICENSE.md and proper publish configuration
- Documentation: Customer-facing email configuration needs documentation for self-hosted users
- Test coverage: Critical auth callback logic needs tests
All issues are addressable within this PR's scope. The foundation is solid — these are refinements rather than fundamental rework.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
agents-core/src/index.ts |
New public export email-send-status-store |
Additive change, non-breaking — acceptable API expansion |
packages/email/src/env.ts |
Zod import from 'zod' instead of '@hono/zod-openapi' | Acceptable — email package is Hono-independent |
packages/email/package.json |
ESM-only exports (no CJS) | Acceptable — consumers are ESM environments |
packages/email/package.json |
Peer dependency on zod@^4.3.6 | Monorepo already on zod v4 |
.env.example |
New SMTP vars with local defaults | Intentional for Mailpit local dev |
runtime-config/types.ts |
RuntimeConfig type change | Additive, non-breaking |
login/page.tsx:203-211 |
Forgot password link has tabIndex={-1} | May be intentional UX decision to streamline login flow |
forgot-password/page.tsx:95-98 |
API errors show success message | Intentional for enumeration prevention |
Reviewers (11)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
2 | 1 | 0 | 0 | 0 | 0 | 1 |
pr-review-product |
5 | 1 | 2 | 0 | 1 | 0 | 1 |
pr-review-consistency |
7 | 0 | 0 | 0 | 1 | 0 | 6 |
pr-review-breaking-changes |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
pr-review-docs |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
7 | 1 | 0 | 0 | 0 | 0 | 6 |
pr-review-types |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-devops |
8 | 2 | 0 | 0 | 1 | 0 | 5 |
pr-review-frontend |
6 | 0 | 1 | 0 | 1 | 0 | 4 |
pr-review-errors |
5 | 1 | 0 | 0 | 2 | 0 | 2 |
| Total | 52 | 7 | 4 | 0 | 6 | 0 | 35 |
| // Require authentication for remaining routes | ||
| invitationsRoutes.use('*', sessionAuth()); | ||
|
|
||
| invitationsRoutes.get('/:id/email-status', async (c) => { |
There was a problem hiding this comment.
🟡 Minor: Add internal route comment for consistency
Issue: This endpoint follows the internal route pattern (plain Hono routing, no OpenAPI schema) but lacks the comment that peer endpoints have.
Why: Other internal routes in this file (like /pending at line 125) have a comment indicating they're internal. This helps maintainers understand the route is not part of the public API contract.
Fix:
| invitationsRoutes.get('/:id/email-status', async (c) => { | |
| // Internal route - not exposed in OpenAPI spec | |
| invitationsRoutes.get('/:id/email-status', async (c) => { |
Refs:
| } catch { | ||
| setError('Could not send reset email. Please try again later.'); | ||
| setIsSubmitting(false); |
There was a problem hiding this comment.
🟡 Minor: Log error for debugging before showing generic message
Issue: The catch block uses a bare catch without capturing or logging the error. When failures occur in production, there's no telemetry to diagnose the issue.
Why: Generic user messages are appropriate for security, but developers need logs to identify systemic issues (network problems, API misconfigurations, etc.).
Fix:
| } catch { | |
| setError('Could not send reset email. Please try again later.'); | |
| setIsSubmitting(false); | |
| } catch (err) { | |
| console.error('[forgot-password] Request failed:', err); | |
| setError('Could not send reset email. Please try again later.'); |
Refs:
| } catch { | ||
| // Email status check failed — fall back to copy-link | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Log email status check failures for debugging
Issue: The catch block silently swallows email status check failures. While graceful degradation to copy-link is appropriate, the complete lack of logging makes systemic issues invisible.
Why: If the email status endpoint is misconfigured or failing, this could go undetected for days in production. Minimal logging helps identify the root cause.
Fix:
| } catch { | |
| // Email status check failed — fall back to copy-link | |
| } | |
| } catch (err) { | |
| console.debug('[invite-member] Email status check failed, falling back to copy-link:', err); | |
| // Email status check failed — fall back to copy-link |
Refs:
| {result.status === 'success' && | ||
| PUBLIC_IS_SMTP_CONFIGURED && | ||
| !result.emailSent && | ||
| result.emailError && ( | ||
| <p className="text-xs text-amber-600 dark:text-amber-400 mt-1 ml-6"> | ||
| Email could not be sent. Copy the link to share manually. | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
🟡 Minor: Email failure warning condition is fragile
Issue: The warning only shows when result.emailError is truthy. If sendEmail() returns { emailSent: false } without an error message (edge case), the user sees neither success nor warning.
Why: Users are left in an ambiguous state when SMTP is configured but email status is unclear. While send.ts currently always sets an error on failure, the condition is fragile to future changes.
Fix: Simplify condition to show warning whenever SMTP is configured but email wasn't sent:
| {result.status === 'success' && | |
| PUBLIC_IS_SMTP_CONFIGURED && | |
| !result.emailSent && | |
| result.emailError && ( | |
| <p className="text-xs text-amber-600 dark:text-amber-400 mt-1 ml-6"> | |
| Email could not be sent. Copy the link to share manually. | |
| </p> | |
| )} | |
| {result.status === 'success' && | |
| PUBLIC_IS_SMTP_CONFIGURED && | |
| !result.emailSent && ( | |
| <p className="text-xs text-amber-600 dark:text-amber-400 mt-1 ml-6"> | |
| {result.emailError | |
| ? 'Email could not be sent. Copy the link to share manually.' | |
| : 'Email status unknown. Copy the link to share manually.'} | |
| </p> | |
| )} |
| <div className="flex flex-col min-h-screen items-center justify-center px-4 py-12"> | ||
| <Card className="w-full max-w-md shadow-none border-none bg-transparent space-y-3"> | ||
| <div className="px-6"> | ||
| <InkeepIcon size={48} /> |
There was a problem hiding this comment.
💭 Consider: Add aria-hidden="true" to decorative icons
Issue: The InkeepIcon components and the Mail icon (line 61) are decorative (adjacent to text that conveys the same meaning) but don't have aria-hidden to hide them from assistive technology.
Why: Screen readers will announce these icons unnecessarily, creating redundant noise for users relying on assistive technology.
Fix: Add aria-hidden="true" to decorative icons throughout this component:
- Line 28:
<InkeepIcon size={48} aria-hidden="true" /> - Line 57:
<InkeepIcon size={48} aria-hidden="true" /> - Line 61:
<Mail className="h-6 w-6 text-muted-foreground" aria-hidden="true" /> - Line 111:
<InkeepIcon size={48} aria-hidden="true" /> - Line 147:
<Loader2 className="mr-2 h-4 w-4 animate-spin" aria-hidden="true" />
Refs:
There was a problem hiding this comment.
Good catch — will address aria-hidden across all pages in a follow-up.
docker-compose.yml
Outdated
| retries: 30 | ||
| # Mailpit for local email testing | ||
| mailpit: | ||
| image: axllent/mailpit:latest |
There was a problem hiding this comment.
🟠 MAJOR: Pin Mailpit image to specific version
Issue: The Mailpit service uses image: axllent/mailpit:latest which is a mutable tag that can change unexpectedly.
Why: While this is a dev-only tool, the :latest tag can break local development environments in non-obvious ways or introduce supply chain risk if the image is compromised. Pinning to a specific version ensures reproducible builds.
Fix:
| image: axllent/mailpit:latest | |
| image: axllent/mailpit:v1.24.1 |
Refs:
Review ResponseThanks for the thorough review. Here's my assessment of each finding: Accepted & Fixed (in 37e4fe4)
Declined with EvidenceMajor 2 — Password reset email failure tracking: Declined. This is by design:
Major 4 — Missing documentation: Declined. SPEC.md Phase 2 (line 554) explicitly defers self-hosted DX docs: "Mailpit in create-agents-template, React Email preview dev server, Email config page in admin UI." Documentation is planned as a separate follow-up once Phase 1 ships. Major 5 — Missing test coverage for auth callbacks: Declined. The auth callbacks are minimal glue code (10 lines each) that call already-tested modules:
Noted for FutureMinor 2 — SMTP detection divergence: Investigated — the current layout.tsx logic Consider 1 (discriminated unions): Valid improvement but over-engineering for internal types consumed by 2 call sites. Consider 2 (multi-instance): Known limitation, mirrors existing Consider 3 (spam folder): Already included — the success message says "Check your inbox and spam folder." Consider 4 (sequential invitations): Intentional — sequential execution avoids overwhelming the SMTP server and provides clearer per-email error reporting. CI/CD NoteThe |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low | Delta Review ✅
This is a delta review scoped to changes since the last automated review (commit c66434f8). The author addressed all actionable feedback in commit 37e4fe41.
✅ Issues Resolved Since Last Review
All 8 actionable items from the previous review have been properly addressed:
| Issue | Severity | Resolution |
|---|---|---|
| Missing authorization on email-status endpoint | 🟠 Major | ✅ Added getActiveMember() requiring admin/owner role |
Mailpit using :latest tag |
🟠 Major | ✅ Pinned to v1.24.1 |
| Missing LICENSE.md in license sync | 🟠 Major | ✅ Added packages/email to sync-licenses.mjs |
| Missing internal route comment | 🟡 Minor | ✅ Added comment above route |
| Missing logging in forgot-password | 🟡 Minor | ✅ Added console.error() before generic message |
| Missing logging in invite-member-dialog | 🟡 Minor | ✅ Added console.debug() for status check failures |
| Email warning condition fragile | 🟡 Minor | ✅ Now shows warning for both error and unknown states |
Package needs private: true |
🟡 Minor | ✅ Added to package.json |
🕐 Pending Recommendations
The following items from the prior review were raised on unchanged files and remain as recommendations for future consideration:
- 🟠 Password reset email failure feedback —
auth.tsstill swallows email send failures with no status tracking - 🟠 Missing documentation for email integration — No docs updates for self-hosted email configuration
- 🟠 Missing test coverage for auth callbacks — BetterAuth callback logic untested
- 🟡 UI config divergence from transport factory —
layout.tsxderivesisSmtpConfigureddifferently - 💭 aria-hidden on decorative icons — Accessibility improvement for forgot-password page
💡 APPROVE WITH SUGGESTIONS
Summary: The delta changes properly address all the actionable feedback from the previous review. The authorization fix on the email-status endpoint, Mailpit version pinning, license sync addition, and logging improvements are all implemented correctly.
The remaining pending recommendations are on unchanged code and can be addressed as follow-up work if desired. The core email integration feature is now in good shape for merge.
Great work addressing the feedback! 🎉
…xample Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…isSmtpConfigured Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…l email status - Add email status endpoint (GET /invitations/:id/email-status) to check send result - Update invite-member-dialog to check email status after invite creation - Show "Invitation email sent", "Email could not be sent — copy the link", or "Copy link" based on status - Remove email-password-only guard on copy-link button — now available for all auth methods (D17) - Update members-table to show copy-link for all pending invitations regardless of auth method - Remove unused Info icon and Tooltip imports from members-table - Update "Next Steps" messaging to reflect email vs copy-link flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add connectionTimeout (10s), greetingTimeout (10s), and socketTimeout (30s) to both Resend and generic SMTP transports per SPEC NFR requirements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e sync - Add admin/owner authorization check on /invitations/:id/email-status endpoint - Pin Mailpit Docker image to v1.24.1 for reproducibility - Add error logging in forgot-password and invite-member-dialog catch blocks - Improve email failure warning to handle missing error message edge case - Add packages/email to license sync script - Mark @inkeep/email as private (internal package) - Add internal route comment for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rect fix
- Move "Forgot password?" link below password input, right-aligned, subtle styling
- Pass typed email from login to forgot-password via query param
- Fix redirectTo to use window.location.origin so reset link goes to manage-ui, not API
- Remove tabIndex={-1} for keyboard accessibility
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add aria-hidden="true" to all decorative icons in forgot-password page - Update access-control docs with invitation email flow and password reset section - Create deployment email configuration guide (SMTP, Resend, Mailpit) - Add email page to deployment sidebar navigation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store organizationId in the email status bridge and verify it matches the caller's active organization before returning status. Also strip organizationId from the response to avoid leaking it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r messaging - Email template: replace raw URL with subtle "Or copy and paste this link" fallback text with a clickable link - Auto-copy invite link to clipboard when email is not configured and a single user is invited - Single invite (no email): "An invite link has been copied to your clipboard. Share the invite link with your team member to have them join!" - Multiple invites (no email): numbered list — "1. Copy the link for each team member. 2. Share the invite link and ask them to redeem!" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Align package naming with the monorepo convention (agents-core, agents-sdk, agents-work-apps, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nick-inkeep
force-pushed
the
feat/email-integration
branch
from
7eb1f9d to
09090ba
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is the 7th+ automated review of PR #2021. The delta since the last review consists of 2 cosmetic commits:
| Commit | Change | Impact |
|---|---|---|
bf9b104d |
Shortened email fallback link label to "Link:" | Cosmetic — more concise |
09090ba9 |
Left-aligned logo in email templates | Cosmetic — brand styling preference |
Both changes are trivial template styling adjustments with no functional, security, or behavioral impact.
✅ Final Verification Complete
All 4 dispatched reviewers verified the implementation is sound:
| Reviewer | Verdict |
|---|---|
pr-review-standards |
✅ Code quality verified — proper error handling, type safety, follows existing patterns |
pr-review-security-iam |
✅ Authorization verified — cross-tenant protection via organizationId is sound |
pr-review-product |
✅ Product coherence verified — clean concept economy, proper graceful degradation |
pr-review-docs |
✅ Documentation verified — accurate, comprehensive, properly cross-referenced |
📋 Prior Review Summary
All 12+ actionable items from the 6 prior automated reviews have been properly addressed:
- ✅ Cross-tenant invitation status disclosure — fixed with
organizationIdverification - ✅ Authorization on email-status endpoint — added admin/owner role check
- ✅ Mailpit version pinned to
v1.24.1 - ✅ LICENSE.md sync script updated
- ✅ Documentation added (
email.mdx) - ✅ Logging added to error paths
- ✅ Package marked as private
- ✅ Accessibility (
aria-hiddenon decorative icons)
🕐 Pending Recommendations (2)
These items were raised in prior reviews and declined by the author with documented reasoning:
- 💭
email-send-status-store.test.ts:17—organizationIdtest coverage (declined: store is a simple Map; authz logic is in endpoint, not store) - 💭
invitations.ts:132— Cross-tenant disclosure (resolved in prior commit)
✅ APPROVE
Summary: This PR is ready to ship! The email integration is well-architected with proper authorization, graceful degradation, comprehensive test coverage (472 new test lines), and complete documentation. The final delta contains only cosmetic styling tweaks to email templates. Ship it! 🚀📧
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: pr-review-docs returned 2 INFO-level entries confirming documentation accuracy (not issues).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing to open a fresh PR with clean CI/review state. |
1 participant
Summary
Wire
sendInvitationEmailandsendResetPasswordBetterAuth callbacks to actually send branded, professional emails via Nodemailer SMTP transport.New
@inkeep/agents-emailpackageStandalone package (
packages/agents-email/) with its own build (tsdown), test (vitest), and env schema. Contains the transport factory, email service, React Email templates, and shared theme — fully decoupled fromagents-api.Transport factory
Auto-detects Resend SMTP relay (
smtp.resend.com:465), generic SMTP, or disabled — single abstraction for cloud, self-hosted, and local dev. Connection/socket timeouts (10s/10s/30s) prevent API response hangs.React Email templates
Branded invitation + password reset templates using
@react-email/componentswith Inkeep brand colors, left-aligned logo, system font stack, and subtle fallback link.Self-service forgot password
New
/forgot-passwordpage withauthClient.requestPasswordReset(), gated behindisSmtpConfigured. Email prefill from login page via query param. Generic "if account exists" messaging (enumeration prevention).Graceful degradation
When SMTP is not configured, zero behavior changes from today — copy-link preserved, forgot-password hidden, bridge works. No errors thrown.
Per-send email status
In-memory bridge pattern (mirrors
password-reset-link-store.ts) surfaces send success/failure to invitation UI. New internalGET /manage/api/invitations/:id/email-statusendpoint with role-based authz and cross-tenant isolation.Invitation UX improvements
createAgentsApp()/createAgentsAuth()extensionBoth functions now accept an optional
emailServiceparam, making email a composable concern that can be passed through the factory without coupling the core auth module to a specific transport.Infrastructure
docker-compose.ymlfor local dev email inspection (web UI on:8025, SMTP on:1025)manage-uiandapiservice definitions.env.exampleandcreate-agents-template/.env.example: Email configuration section added with Resend + generic SMTP optionsscripts/sync-licenses.mjs: New package added to license sync targetsDocumentation
agents-docs/content/deployment/(docker)/email.mdx— full email configuration guide covering Resend, generic SMTP, Mailpit, graceful degradation, verification, and troubleshootingagents-docs/content/visual-builder/access-control.mdx— "Inviting Team Members" and new "Password Reset" sections with email-aware flows and cross-link to email configSpec
~/.claude/specs/email-integration/SPEC.mdTest plan
Automated (39 unit tests passing)
Manual — browser tested locally with Mailpit
{ emailSent: false }for invitations outside caller orgManual — Resend production transport
sendInvitationEmailreturns{ emailSent: true }viasmtp.resend.com:465sendPasswordResetEmailreturns{ emailSent: true }viasmtp.resend.com:465notifications@updates.inkeep.comtoedwin@inkeep.com— verified delivery🤖 Generated with Claude Code