Skip to content

fix(tools/mail-source/imap): accept bare Message-IDs - #1953

Closed
xiaobai0529 wants to merge 2 commits into
apache:mainfrom
xiaobai0529:fix/imap-read-bare-message-id-1924
Closed

xiaobai0529 wants to merge 2 commits into
apache:mainfrom
xiaobai0529:fix/imap-read-bare-message-id-1924

Conversation

@xiaobai0529

@xiaobai0529 xiaobai0529 commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Normalize caller-supplied bare RFC 5322 Message-IDs separately from header parsing, preserving the contract rule that mail-header thread tokens must stay bracketed.
  • Make read_thread return the documented bracketed identifier and make thread_url report the same canonical value for either accepted input form.
  • Add regression coverage for bare/bracketed equivalence, archive URLs, and preserving rejection of malformed non-Message-ID input.

Type of change

  • Python package (tools/*/ with pyproject.toml)
  • Skill change
  • Tool / bridge contract
  • Groovy reference impl
  • Cross-cutting
  • Documentation
  • Project template
  • CI / dev loop
  • Other

Test plan

  • Package tests: uv run pytest -q (34 passed)
  • uv run ruff check src tests
  • uv run ruff format --check src tests
  • uv run mypy src tests
  • prek run --all-files --skip workspace-pytest
  • prek run workspace-pytest --from-ref origin/main --to-ref HEAD

The new regression failed before the fix because read_thread compared the bare caller input verbatim. The same test passes after normalization. thread_url also now returns the canonical bracketed ID instead of echoing the bare spelling.

RFC-AI-0004 compliance

No RFC-AI-0004 surface changes: the patch adds no new mutation, network access, skill behavior, or write-access path.

Linked issues

Closes #1924

Generative AI disclosure

Generative AI was used. Codex (GPT-6) helped implement the patch, tests, and this description. The contributor reviewed the full diff, confirmed understanding, and takes ownership of responding to review feedback.

Normalize caller-supplied thread IDs at the read and archive-link boundaries so the documented bare and bracketed forms resolve to the same thread.

Generated-by: Codex (GPT-6)
Keep caller-side bare Message-ID normalization separate from the bracketed-token rule used for mail headers.

Generated-by: Codex (GPT-6)

@Kaap10 Kaap10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean fix @xiaobai0529!

Separating caller input normalization (canonical_thread_id) from mail header parsing (normalise_message_id) is a very thoughtful design choice that keeps RFC 5322 header extraction strict while accepting bare inputs per contract:mail-source. The regression tests in test_operations.py, test_threads.py, and test_config_and_urls.py cover the equivalence nicely.

There is just one pre-commit check to address so CI can turn green:

Line Endings (CRLF to LF)

The 6 modified files were committed with Windows CRLF (\r\n) line endings. Apache Magpie's mixed-line-ending pre-commit hook enforces LF (\n).

You can normalize line endings and amend your commit with:

git add --renormalize .
git commit --amend --no-edit
git push --force-with-lease

Alternatively, running prek run mixed-line-ending --all-files will automatically convert them to LF.

(Left one minor nit inline in read_thread.py as well). Once line endings are normalized, this is good to go!

@github-actions github-actions Bot added family:tools tools/* contract:mail-source Tool capability: inbound-mail ingestion (mbox / IMAP / ...) labels Nov 1, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes read_thread and thread_url accept a bare Message-ID and return the bracketed form, as the mail-source contract asks; the other three operations that take a thread id still need the same treatment.

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/threads.py:49.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

@potiuk

potiuk commented Nov 1, 2026

Copy link
Copy Markdown
Member

The remaining operations (create_draft, list_drafts, list_sent_since) are in #1962, on top of this branch, since I couldn't push to yours. That also takes care of the original_thread_id nit. This PR can go in as it is.

@potiuk
potiuk enabled auto-merge (squash) November 1, 2026 23:58

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes read_thread and thread_url for bare Message-IDs as #1924 asks, with tests for both spellings. The same fix for create_draft, list_drafts and list_sent_since follows in #1962, so this can go in as it is. Approving.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Magpie handles maintainer review:
Contributing guide.

@potiuk

potiuk commented Nov 2, 2026

Copy link
Copy Markdown
Member

Thanks! Your two commits landed in main as part of #1962, together with the remaining operations, and you are credited as co-author on that commit, so there is nothing left in this PR. Closing it as superseded.

@potiuk potiuk closed this Nov 2, 2026
auto-merge was automatically disabled November 2, 2026 00:04

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contract:mail-source Tool capability: inbound-mail ingestion (mbox / IMAP / ...) family:tools tools/*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

imap: read_thread rejects a bare Message-ID the contract says it accepts

3 participants