Move email send to context #206
Conversation
WalkthroughEmail APIs were refactored to request/context-based async signatures returning Promise, centralizing MIME construction (MailComposer + attachments via fromDataType), adding OpenTelemetry spans, and moving send/sendReply responsibilities into Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EmailIO as Email IO
participant EmailAPI as Email API
participant MailComp as MailComposer
participant Telemetry as OpenTelemetry
participant Server as Email Server
Client->>EmailIO: send(req, ctx, to, email, from?)
EmailIO->>EmailAPI: context.email.send(req, ctx, to, email, from?)
EmailAPI->>Telemetry: start span (agentId)
EmailAPI->>MailComp: build RFC822 message (+attachments via fromDataType)
MailComp-->>EmailAPI: message/rfc822 (bytes)
EmailAPI->>Server: POST /email/send (message/rfc822, X-Agentuity-Message-Id)
Server-->>EmailAPI: 200 + messageId
EmailAPI->>Telemetry: record messageId, end span
EmailAPI-->>EmailIO: Promise<messageId>
EmailIO-->>Client: Promise<messageId>
alt Reply flow
Client->>EmailIO: sendReply(req, ctx, inReplyTo, reply, from?)
EmailIO->>EmailAPI: context.email.sendReply(...)
EmailAPI->>Server: POST /email/2025-03-17/{agentId}/reply
end
alt Error
Server-->>EmailAPI: error
EmailAPI->>Telemetry: record exception, set status
EmailAPI-->>EmailIO: throw Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)src/apis/**📄 CodeRabbit inference engine (AGENT.md)
Files:
{src,test}/**/!(*.d).ts📄 CodeRabbit inference engine (AGENT.md)
Files:
src/apis/**/*.ts📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
Files:
🧬 Code graph analysis (1)src/apis/email.ts (5)
🔇 Additional comments (6)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types.ts (1)
575-586: Fix type constraint mismatch in search() method signature.The constraint on
VectorSearchParams<T extends JsonObject>is incompatible with the unconstrained generic insearch<T = unknown>. The method signature must be updated to match:// src/apis/vector.ts line 222 async search<T extends JsonObject = JsonObject>( name: string, params: VectorSearchParams<T> ): Promise<VectorSearchResult[]>Without this fix, callers passing explicit type parameters with non-JsonObject types will encounter TypeScript compilation errors.
🧹 Nitpick comments (4)
src/io/email.ts (2)
470-489: Avoid shadowing OpenTelemetrycontextimport.Parameter
context: AgentContextshadowscontextfrom '@opentelemetry/api', inviting bugs if tracing is added later. Rename toctxfor clarity and consistency (other services usectx).- async send( - req: AgentRequest, - context: AgentContext, + async send( + req: AgentRequest, + ctx: AgentContext, to: string[], email: EmailReply, from?: { name?: string; email?: string } ): Promise<string> { - const fromAddress = from?.email ?? this.toEmail(); + const fromAddress = from?.email ?? this.toEmail(); if (!fromAddress) { throw new Error('a valid from email address is required'); } - return context.email.send(req, context, to, email, { - name: from?.name, - email: fromAddress, - }); + return ctx.email.send(req, ctx, to, email, { + name: from?.name, + email: fromAddress, + }); }
503-533: Drop unusedtoAddresscheck or document why it’s needed.
toAddressis computed only to throw if missing, but not used in the call. If reply routing doesn't require explicitto, remove this block to cut dead code; otherwise pass it through or document the rationale.- const toAddress = this.fromEmail(); - if (!toAddress) { - throw new Error('cannot reply to an email without a sender address'); - } + // If reply routing requires original sender, validate or pass through here.Also apply the param rename to avoid
contextshadowing as above.- async sendReply( - req: AgentRequest, - context: AgentContext, + async sendReply( + req: AgentRequest, + ctx: AgentContext, reply: EmailReply, from?: { name?: string; email?: string } ): Promise<string> { const messageId = this.messageId(); if (!messageId) { throw new Error('cannot reply to an email without a message ID'); } const fromAddress = from?.email ?? this.toEmail(); if (!fromAddress) { throw new Error('a valid from email address is required'); } - return context.email.sendReply( - req, context, messageId, + return ctx.email.sendReply( + req, ctx, messageId, { subject: this.makeReplySubject(reply.subject), text: reply.text, html: reply.html, attachments: reply.attachments, }, { name: from?.name, email: fromAddress } ); }src/apis/email.ts (2)
60-62: Error messages: standardize and include field name.Use a consistent message like "from.email is required" to aid telemetry/search.
- throw new Error('a valid from email address is required'); + throw new Error('from.email is required');Also applies to: 169-171
24-30: Use AgentRequest.get() for metadata access: safer than direct object indexing.The AgentRequest interface provides a dedicated
get()method for accessing metadata values. Usingreq.get('email-auth-token')instead of indexingreq.metadata?.['email-auth-token']follows the designed API contract and is more resilient to implementation changes.- const authToken = req.metadata?.['email-auth-token'] as string; + const authToken = req.get('email-auth-token') as string;Also applies to lines 138-144.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apis/email.ts(3 hunks)src/io/email.ts(3 hunks)src/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/io/**
📄 CodeRabbit inference engine (AGENT.md)
I/O handlers (Discord, Slack, Email, SMS, Telegram) live under src/io/
Files:
src/io/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/io/email.tssrc/apis/email.tssrc/types.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/email.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/email.ts
🧬 Code graph analysis (3)
src/io/email.ts (1)
src/router/data.ts (1)
src/apis/email.ts (5)
src/types.ts (3)
EmailService(671-699)AgentRequest(1069-1089)AgentContext(946-1064)src/io/email.ts (3)
to(288-299)EmailReply(222-242)attachments(371-443)src/router/router.ts (1)
getTracer(61-66)src/server/util.ts (1)
fromDataType(188-269)src/apis/api.ts (1)
POST(253-270)
src/types.ts (2)
src/io/email.ts (1)
to(288-299)src/router/data.ts (1)
🔇 Additional comments (3)
src/apis/email.ts (1)
64-76: Keep first location; remove incorrect second location reference.The refactor for lines 64-76 is valid—MailComposer accepts
toas either a comma-separated string or an array. However, lines 173-186 do not contain atofield (that's a reply operation usinginReplyToandreferences), so the "Also applies to" claim is incorrect.- to: normalizedTo.join(', '), + to: normalizedTo,Likely an incorrect or invalid review comment.
src/types.ts (2)
676-684: Breaking change verification complete — no alignment issues found.The interface signature change to
Promise<string>is properly implemented:
- EmailApi correctly implements the new return type
- The single call site (src/io/email.ts:485) properly returns the promise to its caller
- No orphaned implementations or mocks discovered
690-699: Test coverage needed: verify that Email.sendReply() return value is captured and used by handlers.The implementation is correct—both
Email.sendReply()(line 494) andEmail.send()(line 472) properly returnPromise<string>by propagating the result fromcontext.email.sendReply()andcontext.email.send()respectively. However, there are no tests intest/io/email.test.ts(or elsewhere) verifying that the returnedmessageIdis actually captured or used by handlers.Confirm that tests exist for the return value handling, or add tests to verify handlers correctly use the returned message ID.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/apis/email.ts (1)
39-53: Reduce memory usage by streaming large attachments.
resp.data.buffer()loads entire attachment content into memory. Per the past review,DataHandler.stream()returns aReadableStream, which can be passed to Nodemailer. However, Nodemailer expects Node.js streams, not Web ReadableStream.Consider this approach to stream attachments:
let attachments: Attachment[] = []; if (email.attachments) { attachments = await Promise.all( email.attachments.map(async (attachment) => { const resp = await fromDataType(attachment.data); + const webStream = await resp.data.stream(); + const nodeStream = webStream instanceof ReadableStream + ? Readable.fromWeb(webStream as any) + : webStream; return { filename: attachment.filename, - content: await resp.data.buffer(), + content: nodeStream, contentType: resp.data.contentType, contentDisposition: attachment.contentDisposition ?? ('attachment' as const), }; }) ); }You'll need to import
Readablefrom Node.jsstreammodule. This pattern also appears insendReply()at lines 153-167 and should be extracted to a shared helper to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apis/api.ts(1 hunks)src/apis/email.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/email.tssrc/apis/api.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/apis/email.tssrc/apis/api.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/email.tssrc/apis/api.ts
🧬 Code graph analysis (1)
src/apis/email.ts (5)
src/types.ts (3)
EmailService(671-699)AgentRequest(1069-1089)AgentContext(946-1064)src/io/email.ts (4)
to(288-299)EmailReply(222-242)attachments(371-443)messageId(272-274)src/router/router.ts (1)
getTracer(61-66)src/server/util.ts (1)
fromDataType(188-269)src/apis/api.ts (1)
POST(253-270)
🪛 GitHub Actions: Run Tests
src/apis/api.ts
[error] 167-167: TS2322: Type 'Body | undefined' is not assignable to type 'BodyInit | null | undefined'. Type 'Buffer' is not assignable to type 'BodyInit'.
🔇 Additional comments (5)
src/apis/email.ts (5)
55-62: LGTM! Validation logic is correct.The recipient normalization and validation ensure at least one valid recipient, and the from-email check prevents sending without a sender address.
64-75: LGTM! MailComposer configuration is correct.The email composition properly maps all fields, and the fallback to
ctx.agent.namefor the sender name is a sensible default.
88-97: LGTM! Message sent as Buffer (not string).The code correctly passes
message(a Buffer) directly toPOSTwithout callingtoString(), which would corrupt binary MIME content. This addresses the past review concern.
173-185: LGTM! Reply-specific MailComposer configuration is correct.The
inReplyToandreferencesfields are properly set for email threading, ensuring replies maintain conversation context.
198-207: LGTM! Message sent as Buffer with correct reply endpoint.The message Buffer is passed directly to
POSTwithout conversion, and the reply-specific endpoint path is used correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/apis/email.ts (2)
39-53: The past review comment about streaming attachments is still unresolved.The current implementation still buffers entire attachments via
resp.data.buffer()which can cause memory issues with large files. The past review comment correctly suggests usingresp.data.stream()instead to stream attachment content directly to Nodemailer.
153-167: The past review comment about extracting duplicate attachment processing remains unresolved.This attachment processing logic is identical to lines 39-53 in
send(). The past review comment correctly suggests extracting this to a shared private helper method to reduce duplication.
🧹 Nitpick comments (1)
src/apis/email.ts (1)
138-143: LGTM, but consider extracting auth token validation.The auth token extraction is correct and consistent with
send(). However, this validation logic is duplicated. Consider extracting to a private helper method:private extractAuthToken(req: AgentRequest): string { const authToken = req.metadata?.['email-auth-token'] as string; if (!authToken) { throw new Error( 'email authorization token is required but not found in metadata' ); } return authToken; }Then use:
const authToken = this.extractAuthToken(req);in both methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apis/email.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/email.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
src/apis/email.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/email.ts
🧬 Code graph analysis (1)
src/apis/email.ts (5)
src/types.ts (3)
EmailService(671-699)AgentRequest(1069-1089)AgentContext(946-1064)src/io/email.ts (4)
to(288-299)EmailReply(222-242)attachments(371-443)messageId(272-274)src/router/router.ts (1)
getTracer(61-66)src/server/util.ts (1)
fromDataType(188-269)src/apis/api.ts (1)
POST(253-270)
🔇 Additional comments (6)
src/apis/email.ts (6)
1-8: LGTM!The imports are well-organized and follow the coding guidelines. ESM syntax is used consistently, relative imports are used for internal modules, and type-only imports are properly applied.
24-29: LGTM!The auth token extraction and validation is correct. The error message is clear and the check prevents downstream issues.
55-62: LGTM!The validation logic is solid. Recipients are properly normalized and validated, and the from email requirement is clearly enforced with descriptive error messages.
169-171: LGTM!The from email validation is consistent with the
send()method and properly enforces the requirement.
173-187: LGTM!The MailComposer configuration for replies is correct. The
inReplyToandreferencesfields are properly set to maintain email threading, and the fallback forfrom.nameusingctx.agent.nameis consistent with thesend()method.
188-225: Message sending logic mirrorssend()method.The message building and POST logic is solid with proper OpenTelemetry tracing and error handling. However, the same issues flagged in the
send()method apply here:
- Line 200: Should pass
messagedirectly instead ofmessage.toString()(see earlier comment)- Line 216-217: Line length exceeds 80 characters (see earlier comment)
Summary by CodeRabbit
New Features
Improvements