HT-11: inbound email parser (postal-mime) - #7
Conversation
Pure raw-MIME -> ParsedEmail parser, the first mail-engine module. Built on postal-mime (MIT-0). Faithfully surfaces the threading-critical headers (messageId, inReplyTo, references-as-ordered-array) and captures HTML verbatim (unsanitized — sanitization is a later, separate step). Attachments carry bytes (blob storage happens at the store layer). Test-first: 8 hand-authored RFC 5322 fixtures + 17 Vitest cases; 94% coverage on parse.ts. Verified: typecheck, lint, tests all green. Adds @types/node (was absent; needed for fs + any future Node-touching module) and flips tsconfig types to ["node"]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults 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:
📝 WalkthroughWalkthroughAdds ChangesInbound email parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant parseInboundEmail
participant PostalMime
Caller->>parseInboundEmail: raw email input
parseInboundEmail->>PostalMime: parse(raw)
PostalMime-->>parseInboundEmail: parsed email data
parseInboundEmail-->>Caller: normalized ParsedEmail
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
src/mail/parse.test.ts (1)
12-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the binary parser inputs.
parseInboundEmailacceptsstring | Uint8Array | ArrayBuffer, but the tests only pass strings. Add equivalent fixture tests for both binary forms to protect the public input 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/parse.test.ts` around lines 12 - 31, Add equivalent tests for parseInboundEmail using both Uint8Array and ArrayBuffer inputs, reusing the plain-text-simple fixture bytes and asserting the same parsed fields as the existing string-input test. This should cover the complete public input contract without duplicating unrelated test setup.
🤖 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/mail/parse.ts`:
- Around line 169-182: Update toParsedAddress and the ParsedEmail.from type to
preserve RFC 6854 group-form senders instead of returning null when only
addr.group is present. Add a group representation compatible with existing
ParsedAddress consumers, retain the group name and member addresses (including
empty groups), and add fixtures/tests covering named groups and empty-group From
values.
- Around line 219-225: Update toHeaderRecord to initialize its accumulator with
Object.create(null) instead of a plain object, preserving reserved header names
such as __proto__ and constructor; add a regression test covering a reserved
header name and verifying its value is retained correctly.
- Around line 152-155: Update parseReferences to extract RFC 5322 msg-id values
rather than splitting on whitespace, so CFWS/comments between IDs are ignored
and only bracketed message IDs are returned. Add a fixture covering a References
header such as `<a@example.test> (legacy MUA) <b@example.test>` and verify the
result contains only both IDs.
---
Nitpick comments:
In `@src/mail/parse.test.ts`:
- Around line 12-31: Add equivalent tests for parseInboundEmail using both
Uint8Array and ArrayBuffer inputs, reusing the plain-text-simple fixture bytes
and asserting the same parsed fields as the existing string-input test. This
should cover the complete public input contract without duplicating unrelated
test setup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7151f5d5-dfc6-46f0-a4c8-8a50a4513400
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonsrc/mail/parse.test.tssrc/mail/parse.tstests/mail/fixtures/attachment.emltests/mail/fixtures/encoded-word-subject.emltests/mail/fixtures/html-script-body.emltests/mail/fixtures/missing-optional-headers.emltests/mail/fixtures/multipart-alternative.emltests/mail/fixtures/plain-text-simple.emltests/mail/fixtures/quoted-printable-body.emltests/mail/fixtures/threading-headers.emltsconfig.json
| /** | ||
| * postal-mime's `Address` type is a union of a plain `Mailbox` | ||
| * (`{name, address}`) and an RFC 5322 address GROUP | ||
| * (`{name, group: Mailbox[]}`, no top-level `address`) — group syntax is | ||
| * legal in `To`/`Cc`/`Bcc` (e.g. `undisclosed-recipients:;`) but not in | ||
| * `From`. `ParsedAddress` has no group concept, so a bare group here (no | ||
| * `address`) maps to `null` — this only matters for `From`, where a group | ||
| * would be non-conformant mail anyway. `to`/`cc` use `toParsedAddressList` | ||
| * below, which flattens groups into their member addresses instead of | ||
| * dropping them. | ||
| */ | ||
| function toParsedAddress(addr: Address | undefined): ParsedAddress | null { | ||
| if (!addr?.address) return null | ||
| return addr.name ? { address: addr.address, name: addr.name } : { address: addr.address } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' src/mail/parse.tsRepository: Helpthread/helpthread
Length of output: 9824
🏁 Script executed:
git ls-files | rg '(^|/)(test|spec|fixture|fixtures|__tests__|parse).*'Repository: Helpthread/helpthread
Length of output: 1016
🏁 Script executed:
sed -n '1,260p' src/mail/parse.test.ts && printf '\n---\n' && sed -n '1,260p' specs/mail/threading.md && printf '\n---\n' && rg -n "group|undisclosed|Sender|From:" tests/mail fixtures/mail src/mail specs/mail -SRepository: Helpthread/helpthread
Length of output: 22878
🌐 Web query:
RFC 6854 group syntax From Sender allow group syntax in From Sender RFC 5322
💡 Result:
RFC 6854 formally updates the Internet Message Format specification (RFC 5322) to allow the use of "group" syntax in the "From:" and "Sender:" header fields [1][2]. Key details regarding this update include: 1. Permissibility: While RFC 5322 previously restricted "group" syntax to fields such as "To:" and "Cc:", RFC 6854 explicitly relaxes this restriction to allow group syntax in "From:", "Sender:", "Resent-From:", and "Resent-Sender:" [1][3]. 2. Recommendations: Despite permitting this syntax, the RFC includes an applicability statement that recommends against the general use of group syntax in these originator fields [4]. It states that the "From:" and "Sender:" fields should ideally continue to use standard mailbox-list and mailbox syntax, respectively [1][3]. 3. Constraints: If group syntax is used in the "Sender:" field, the group must not contain more than one mailbox [1][3]. Additionally, if the "From:" field contains more than one mailbox (whether listed directly or contained within groups), the message must include a "Sender:" field with a single address [1][3]. This update effectively reconciles the syntax rules for originator fields with those used elsewhere in the Internet Message Format [2][4].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc6854
- 2: https://datatracker.ietf.org/doc/html/rfc6854
- 3: https://www.rfc-editor.org/rfc/rfc6854.txt
- 4: http://www.faqs.org/rfc/rfc6854.txt
Preserve RFC 6854 group-form From values. From: can legally use group syntax under RFC 6854, so toParsedAddress() shouldn’t collapse a group-form sender to null. A value like From: Automated Systems:alice@example.test; currently loses its members; make ParsedEmail.from group-aware (or add a separate group representation) and cover named/empty-group fixtures.
🤖 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/parse.ts` around lines 169 - 182, Update toParsedAddress and the
ParsedEmail.from type to preserve RFC 6854 group-form senders instead of
returning null when only addr.group is present. Add a group representation
compatible with existing ParsedAddress consumers, retain the group name and
member addresses (including empty groups), and add fixtures/tests covering named
groups and empty-group From values.
Sources: Coding guidelines, MCP tools
…ening Parser handles untrusted input (emails from anyone), so these matter: - 🔒 Security (Minor): toHeaderRecord now uses a null-prototype object. A header literally named __proto__ or constructor is stored as an ordinary own key instead of mutating the prototype (dropping the header) or reading an inherited value (garbage). + regression test. - 🎯 Correctness (Major, RFC 6854): a group-form From no longer collapses to null — it maps to the group's first member so the sender isn't lost. - 🎯 Correctness (Minor, threading-critical): References are extracted as angle-bracketed <...> message-ids via regex, so CFWS/comments legally interspersed between ids don't leak in as bogus 'message-ids'. Three regression tests added (parse.test.ts: 15 -> 18). Verified: lint clean, typecheck exit 0, all tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mail/parse.ts (1)
65-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc:
referencesfield comment describes the old whitespace-splitting behavior.
parseReferences(Lines 146-156) now extracts angle-bracketed<...>message-ids via regex to correctly ignore CFWS/comments, but this field-level doc still says "This function splits that string on runs of whitespace, which correctly handles both space-separated and folded/newline-separated ids". Since this field is documented as threading-critical, a stale description here could mislead future changes to threading logic.📝 Proposed doc fix
- * Threading-critical: postal-mime exposes `References` as a single raw - * string (already unfolded — folded continuation lines are merged with - * an embedded newline, not collapsed to a header object per id). This - * function splits that string on runs of whitespace, which correctly - * handles both space-separated and folded/newline-separated ids without - * losing or reordering any entry. specs/mail/threading.md §3 rule 1 + * Threading-critical: postal-mime exposes `References` as a single raw + * string (already unfolded — folded continuation lines are merged with + * an embedded newline, not collapsed to a header object per id). This + * function extracts angle-bracketed `<...>` message-id tokens (RFC 5322 + * §3.6.4), ignoring CFWS/comments interspersed between them, without + * losing or reordering any entry. specs/mail/threading.md §3 rule 1 * scans this array most-recent-first — i.e. the CONSUMER reverses it; * this parser preserves wire order and does not reverse. */🤖 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/parse.ts` around lines 65 - 79, Update the field-level documentation for references to describe parseReferences extracting angle-bracketed message-ids via regex while ignoring CFWS/comments, rather than splitting on whitespace. Preserve the documented wire order, empty-header behavior, and consumer-side most-recent-first reversal guidance.
🤖 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.
Outside diff comments:
In `@src/mail/parse.ts`:
- Around line 65-79: Update the field-level documentation for references to
describe parseReferences extracting angle-bracketed message-ids via regex while
ignoring CFWS/comments, rather than splitting on whitespace. Preserve the
documented wire order, empty-header behavior, and consumer-side
most-recent-first reversal guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50f2be7b-c3c0-4b4e-9909-1ec43fe6d745
📒 Files selected for processing (2)
src/mail/parse.test.tssrc/mail/parse.ts
The References field-doc still described the old whitespace-splitting behavior after the code moved to regex message-id extraction. Updated to match — a doc contradicting the code on threading-critical parsing is exactly what misleads future changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
The first mail-engine module — a pure function that turns a raw RFC822/MIME email into a normalized
ParsedEmail. No I/O, no storage, no side effects.Built on postal-mime (MIT-0, serverless MIME parser — verified at adoption). Faithfully surfaces the threading-critical headers the spec depends on:
messageId/inReplyTo— verbatimreferences— split into an ordered array of message-ids (threading depends on this — tested against folded multi-id headers)htmlcaptured verbatim / unsanitized (sanitization is a later, separate step — matches the observed 'inbound HTML stored raw' behavior)ParsedEmailis intentionally richer than the providers'NormalizedInboundEmail)Test-first: 8 hand-authored RFC 5322 fixtures (plain, multipart, script-tag HTML, threading headers, attachment, encoded-word subject, quoted-printable, missing-headers) + 17 Vitest cases. 94% coverage on
parse.ts; verified locally green (typecheck/lint/test).Also adds
@types/node(was absent — needed for fs and any future Node-touching module) and setstsconfigtypes to["node"].Jira: https://resonantiq.atlassian.net/browse/HT-11
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores