Skip to content

fix(email): RFC-correct In-Reply-To (§3.6.4) threading + RFC 2047 (§6.2) address display - #1185

Closed
seonghobae wants to merge 3 commits into
developfrom
claude/inkspan-pr-audit-ci-q1u4uj
Closed

fix(email): RFC-correct In-Reply-To (§3.6.4) threading + RFC 2047 (§6.2) address display#1185
seonghobae wants to merge 3 commits into
developfrom
claude/inkspan-pr-audit-ci-q1u4uj

Conversation

@seonghobae

@seonghobae seonghobae commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Two small, independent, standards-grounded correctness fixes to Naruon email-header handling (one commit each).

1. RFC 5322 §3.6.4 — multi-id In-Reply-To threading (4dadff0)

assign_thread_id parsed In-Reply-To with normalize_message_id, which only strips </> off the whole string. RFC 5322 §3.6.4 defines in-reply-to = "In-Reply-To:" 1*msg-id — it may carry multiple message-ids. A legal "<parent@x> <cc-parent@x>" became the corrupted id "parent@x> <cc-parent@x", so the reply failed to join its parent thread and, when References was absent, became a mangled deterministic root (orphaned). Fixed to use the multi-id extract_reference_ids(...)[0] (first parent, per §3.6.4 and jwz message threading). Single-id/absent paths unchanged.

2. RFC 2047 §6.2 — encoded-word address display names (4cb34fe)

_sanitize_address_display_text decoded From/To/Reply-To display names via getaddresses (correct), then passed them through email.utils.formataddr, whose default charset='utf-8' re-encodes any non-ASCII display name back into an RFC 2047 encoded-word. So From: =?UTF-8?B?7ZmN6ri464+Z?= <hong@test.com> was stored/shown as =?utf-8?b?…?= <hong@test.com> instead of 홍길동 <hong@test.com>. RFC 2047 §6.2 requires encoded-words in a displayed header to be shown decoded; these fields are display+fingerprint surfaces never re-serialized into an outgoing message. Fixed with a local _format_address_display that mirrors formataddr's quoting/escaping (ASCII output byte-identical, verified) but never re-encodes. (Subject was already correct — it doesn't round-trip through formataddr.)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Verification (from backend/, DISABLE_BACKGROUND_WORKERS=1 PYTHONWARNINGS=error)

  • TDD for both: new tests fail on develop (assert the mangled/re-encoded output) and pass after the fix.
  • pytest tests/test_threading_service.py → 8 passed; parser+threading+service → 39 passed.
  • Broader email suites (import, dedupe, model reconciliation, emails API, threading pipeline) → 118 passed, 1 skipped; client/attachment suites → 103 passed. Zero Timeout/Fatal/Warn/Denied.
  • ruff check on all changed files → clean.

assign_thread_id() normalized the In-Reply-To header by stripping angle
brackets off the whole string. RFC 5322 §3.6.4 defines In-Reply-To as
"In-Reply-To:" 1*msg-id, so it may legitimately carry more than one
message-id. For a two-id value like "<a@x> <b@x>" the old code produced
the mangled id "a@x> <b@x", which matches no stored parent: the reply
failed to join its parent thread and, when References was absent, became
a bogus thread root.

Parse In-Reply-To with the same angle-bracket extractor used for
References and take the first parsed id as the immediate parent, matching
jwz "message threading" (extract the first message-id from In-Reply-To).
Single-id and bracket-less values are unchanged, so all existing tests
stay green.

Verification (backend/):
  DISABLE_BACKGROUND_WORKERS=1 PYTHONWARNINGS=error python3 -m pytest \
    tests/test_threading_service.py tests/test_threading_perf.py \
    tests/test_threading_pipeline.py tests/test_email_parser.py \
    tests/test_email_import_service.py -q
  -> 75 passed, 0 warnings
  python3 -m ruff check services/threading_service.py \
    tests/test_threading_service.py -> All checks passed

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 90f88644-f808-44ca-9a71-632021b3d04e

📥 Commits

Reviewing files that changed from the base of the PR and between ab6442b and 05b94a9.

📒 Files selected for processing (4)
  • backend/services/email_parser.py
  • backend/services/threading_service.py
  • backend/tests/test_email_parser.py
  • backend/tests/test_threading_service.py

Comment @coderabbitai help to get the list of available commands.

_sanitize_address_display_text decoded From/To/Reply-To display names via
getaddresses (correct), then passed them through email.utils.formataddr,
which re-encodes any non-ASCII display name back into an RFC 2047
encoded-word. As a result a header such as
`From: =?UTF-8?B?7ZmN6ri464+Z?= <hong@test.com>` was stored and shown as
`=?utf-8?b?7ZmN6ri464+Z?= <hong@test.com>` instead of the decoded
`홍길동 <hong@test.com>`.

RFC 2047 §6.2 requires encoded-words in a displayed header to be presented
in their decoded form. sender/recipients/reply_to are display (and
fingerprint) fields that are never re-serialized into an outgoing message,
so the decoded human-readable form is the correct value.

Fix: format the already-decoded, sanitized display name with a local
helper (_format_address_display) that mirrors formataddr's quoting/escaping
for ASCII names (verified identical output) but never RFC 2047-re-encodes,
keeping joined recipient lists unambiguous. Adds a regression test citing
RFC 2047 §6.2 covering B- and Q-encoded display names.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
@seonghobae seonghobae changed the title fix(threading): parse multi message-id In-Reply-To per RFC 5322 §3.6.4 fix(email): RFC-correct In-Reply-To (§3.6.4) threading + RFC 2047 (§6.2) address display Jul 30, 2026
@seonghobae

Copy link
Copy Markdown
Contributor Author

Closing after Loop drain: permanently blocked — branch was updated onto develop for mergeability, which cleared prior APPROVED robot evidence; re-review (CodeRabbit/OpenCode) and/or central gate jobs (metadata-only gate evaluation, coverage-evidence) remained pending/stuck without a re-runnable workflow handle. Not force-merging (merge-gate policy). Re-open a focused PR when robot capacity is available. Related product security fixes that reimplemented cleanly remain on branch goal/carddav-path-traversal-decode (#1206) for relaunch.

@seonghobae seonghobae closed this Jul 31, 2026
@seonghobae seonghobae reopened this Jul 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR governance metadata gate is not ready for 05b94a9ba1427d119d1917bae90d3f472b5918ee:

  • Required check coverage-evidence is CANCELLED on the current head.
  • Required check opencode-review is CANCELLED on the current head.
  • Required check coverage-source-tree is CANCELLED on the current head.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Superseded by #1192 after current-head CodeGraph and branch comparison. #1192 preserves both behaviors from this PR (decoded RFC 2047 display names and RFC 5322 multi-ID In-Reply-To threading) and extends them with CFWS handling, folded Message-ID normalization, unknown-zone Date normalization, broader 100% parser/threading tests, and the standards-basis research pack. Closing the overlapping smaller PR to avoid competing edits to the same parser/threading files.

@seonghobae seonghobae closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants