Repository navigation
fix(imap): agree on a thread whose root sits outside the window (#1925) - #1965
Conversation
github-actions
Bot
added
family:tools
26230ad to
2d6b831
Compare
…he#1925) `list_recent_threads`, `read_thread` and `create_draft` disagreed about a thread the fetched window only partly holds. `create_draft` refused it outright, so a thread that could be listed and read could not be replied to; `list_recent_threads` titled the thread from the first message it saw rather than from the root, so a reply dated before the root - or a window that starts inside the thread - listed the same thread under a different subject than the one `read_thread` reported. All three operations now take the subject - and `started_by`, and `create_draft`'s default recipient - from the root when the window holds it, and otherwise from the earliest dated message (undated last), which is the message `maildir` already uses for the same purpose. `create_draft` attaches to the chronologically last message as before and, when the caller names no recipient, addresses the earliest sender that is not the adapter's own account, falling back to the earliest message; that heuristic is documented in the adapter README and the mail-source contract. Closes apache#1925 Generated-by: Codar agent
- _first_foreign_sender compares the From address with IMAP_SOURCE_USER case-insensitively, so a project reply that differs only in case is not taken for the reporter and the draft does not default to the project's own address; with a test. - Remove thread_subject_of, which nothing calls. Generated-by: Claude Opus 5
2fbd511 to
a24076c
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The shared thread_start_message helper is the right shape and matches maildir's _thread_subject; two small things before this merges: the default-recipient helper picks a message with no parseable From, and the contract sentence on default recipients is broader than the code.
GitHub has not yet computed whether this branch merges cleanly with its base.
Smaller observations
- See inline comments on
tools/mail-source/imap/src/imap_source/create_draft.py:138,tools/mail-source/contract.md:105,tools/mail-source/imap/src/imap_source/create_draft.py:80.
Worth a second look from
Folks with the most context on these paths:
@liwenjie200543— 3 recent commit(s) on the changed paths (frequent contributor, not a committer)
None of them have been notified — asking any of them for an
extra pass is the maintainer's call, and optional.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| """ | ||
| own = own_address.casefold() | ||
| return next( | ||
| (message for message in messages if (message.position.from_address or "").casefold() != own), |
There was a problem hiding this comment.
correctness: a message with no parseable From is taken as the default recipient
A message with no parseable From has from_address is None, and "" != own holds, so _first_foreign_sender returns it.
create_draft then exits with cannot address the draft: the thread has no parseable From and no --to given, even when a later message in the window is from the reporter.
I reproduced it against the fake backend: a window holding a From-less reply followed by a reporter@example.org message refuses the draft instead of addressing reporter@example.org.
Skip messages without a sender, and add a regression test for that window:
| (message for message in messages if (message.position.from_address or "").casefold() != own), | |
| ( | |
| message | |
| for message in messages | |
| if message.position.from_address is not None and message.position.from_address.casefold() != own | |
| ), |
| `create_draft`'s default recipient when the caller names none - comes from | ||
| the root when the window holds it, and otherwise from the earliest message | ||
| the window can prove (undated last), the message `maildir` uses for the same | ||
| purpose. A default recipient is the earliest message whose sender is not the |
There was a problem hiding this comment.
shared contract states an IMAP-only default unconditionally
Read on its own, this sentence applies even when the window holds the root, but the IMAP code addresses the root in that case.
It also puts into the shared contract a default that only the IMAP adapter has: maildir's create_draft always takes an explicit to.
Scope it to the case and the backends it covers, e.g. "When the root is missing, a backend that defaults the recipient uses the earliest message whose sender is not its own account, falling back to the earliest message."
| # this account - the one address the adapter knows is the project's own. | ||
| # The earliest fetched message is not necessarily the reporter: a thread the | ||
| # project answered first would otherwise address the project itself. | ||
| start = root if root is not None else thread_start_message(ordered, thread_id) |
There was a problem hiding this comment.
redundant branch
thread_start_message already returns the root whenever the window holds it, so the conditional is redundant.
Likewise _first_foreign_sender always returns a Message, so the if addressee is not None guard at line 93 can never be false.
start = thread_start_message(ordered, thread_id), and drop the dead addressee is not None guard.
|
I've opened a PR against your branch with the fixes for the review points: sharpca#1. Merge it there and this PR picks it up, or fold the changes into your own commit if you prefer. |
Summary
list_recent_threads,read_threadandcreate_draftnow agree on athread the fetched window only partly holds: the subject (and
started_by, andcreate_draft's default recipient) comes from the rootwhen the window holds it, else from the earliest dated message - the rule
maildiralready applies.create_draftno longer refuses such a thread withthread root ... not found, so a thread that can be listed and read can also be replied to.list_recent_threadswhile
read_threadkeeps reporting the root's subject.Type of change
tools/<system>/*.md)tools/*/withpyproject.toml)Test plan
pytest,ruff check,ruff format --check,mypypass intools/mail-source/imap(
PYTHONPATH="src;tests" python -m pytest -q-> 41 passed;mypy->no issues in 19 source files)
list_recent_threadsvsread_threadon a root-less window and on areply dated before the root;
create_drafton a root-less window; andthe default recipient when the earliest message is the project's own
reply.
create_draftraised
SystemExit: thread root <report-1@lists.example.org> not foundbefore the change and appends the thread-attached draft after it;
list_recent_threadsreportedRe: Bug report: connector crashwhileread_threadreportedBug report: connector crashbefore, and bothreport the root's subject after.
prek run --all-files- not run here (noprekin this environment);the package-level gates above were run directly.
RFC-AI-0004 compliance
draft to the Drafts folder; nothing is sent.
Linked issues
Closes #1925
Notes for reviewers
The default-recipient rule is a heuristic, as the issue anticipated: a thread
the project answered before the window begins still drafts to whoever replied
first, because the adapter only knows its own account address
(
IMAP_SOURCE_USER). The chosen rule is documented intools/mail-source/imap/README.md(Threading model) and intools/mail-source/contract.md(Thread identity).Gen-AI disclosure: An AI agent running on the Codar runtime reproduced the
issue and prepared the change, its regression tests and this description; the
package gates listed above were executed on the result, and the diff is
reviewable as an ordinary hand-written change.