feat(tools/maildir): land the local-archive read side by sharpca · Pull Request #1811 · apache/magpie

sharpca · 2026-10-25T06:57:56Z

Summary

contract:mail-source had no working local-files backend: the mbox adapter
documented three read operations and shipped no executable. tools/maildir/
— already named as the vendor home for this backend in its own README, and
already carrying the outbound half — now implements them:
maildir-source-threads, maildir-source-read, maildir-source-thread-url.
One reader, three shapes sniffed per call: a single mbox file, a Maildir, or
a directory of .eml files. Read-only by construction; there is no draft or
send path here, that is maildir-draft-create's half of the vendor.
Docs follow: tools/maildir/README.md goes from "reader remains the
documented stub" to an Operations table, and the mbox adapter's own README
drops its stub status and points at the implementation.

Type of change

Python package (tools/*/ with pyproject.toml)
Tool / bridge contract (tools/<system>/*.md)
Documentation (docs/, README.md, CONTRIBUTING.md)

Test plan

pytest tests/test_read_archive.py: 16 passed — covers all three archive
shapes, threading by In-Reply-To and by References, the
orphan-becomes-its-own-root case, whole-thread retention across the
--since-days boundary, newest-first ordering, oldest-first thread
bodies, the missing-thread exit code, and both thread_url branches.
mypy: no issues in 8 source files
ruff check and ruff format --check: clean
markdownlint-cli2 on both changed READMEs: 0 issues

RFC-AI-0004 compliance

Write-access discipline — the new code is read-only. It opens the
archive, reads it, and returns JSON; nothing writes, sends, or drafts.

Linked issues

Closes #304

Notes for reviewers

Two judgement calls are in the commit body rather than repeated here: a
thread is returned whole when any message is inside the window, and
thread_url returns a file:// link rather than inventing a public address
when the adopter declared no template.
tools/maildir's existing tests fail on Windows because Maildir names carry
:2,D, which Windows rejects. That is a platform limit, unrelated to this
change, and CI runs on Linux.
The mbox row of the capability matrix in tools/mail-source/contract.md
already claimed these three operations; it now has an implementation behind
the claim.

Generative AI disclosure

This change was authored with an AI coding agent (WorkBuddy AI, DeepSeek-V4.1-Flash)
under human review. The commit carries the Generated-by: trailer; the
contributor has reviewed the diff and owns the change.

Why: `fetch_issues.py` ran `gh` once and turned any non-zero exit into a `SystemExit`, so a single transient `HTTP 502` aborted the whole dashboard run. Because this fetch is the first of six steps, the operator had to restart the pipeline by hand. `run_gh` now retries the failures that can clear on their own (5xx, gateway timeouts, connection resets) with exponential backoff, and still fails fast on the ones that cannot: an auth error, a bad repo slug, or an unknown flag never fixes itself. The default cache also moves off a hardcoded `/tmp`. A sandboxed run is granted its own scratch directory and cannot write `/tmp` at all, so the first fetch step failed before it ever reached the API. The default is now `<tempfile.gettempdir()>/tracker-stats-cache`, which honours `$TMPDIR` on POSIX and `%TEMP%` on Windows; an explicit `TRACKER_STATS_CACHE` still wins. The default output path moves with it, for the same reason. Closes apache#1744. Generated-by: WorkBuddy AI (DeepSeek-V4.1-Flash)

Why: Step 2 told the model to pick exactly one triage class from a table that listed `STALE-CANDIDATE` as one of the five rows, while the same row called staleness "orthogonal". Both instructions cannot be followed, so the model picked one: Sonnet took the table literally and returned `triage_class: STALE-CANDIDATE` for the stale fixture, while Opus and Fable followed the "orthogonal" half and returned `UNTRIAGED`. The fixture expects `UNTRIAGED`, so Sonnet failed `step-2-classify/case-4-stale-candidate` on 10 of 10 runs. Staleness is a flag, not a class. The class table now has four rows, and the paragraph under it says plainly that an issue keeps its real class while `is_stale_candidate` carries staleness: an issue untriaged for 216 days is `UNTRIAGED` with `is_stale_candidate: true`, never `STALE-CANDIDATE`. Step 3's `stale_candidate` bucket and the untriaged funnel already read the boolean, so only the two places that still named a class had to move. The eval output spec drops `STALE-CANDIDATE` from the allowed `triage_class` values and states the rule explicitly. No fixture changes: `case-4-stale-candidate/expected.json` already expected `UNTRIAGED` plus `is_stale_candidate: true`. This makes the skill agree with the fixture it was already being graded against. Closes apache#1678. Generated-by: WorkBuddy AI (DeepSeek-V4.1-Flash)

Why: `contract:mail-source` had no working local-files backend. The mbox adapter documented the three read operations but shipped no executable, so the forensics / air-gapped path every `security-issue-import` run falls back to was unexercised code in a README. `tools/maildir/` was already named as the vendor home for that backend in its own README, and it already carries the outbound half of the vendor. This lands the inbound half there: `maildir-source-threads`, `maildir-source-read` and `maildir-source-thread-url`, over the three shapes an adopter can point at (one mbox file, a Maildir, a directory of `.eml` files), sniffed per call. Two decisions worth stating: - A thread is returned whole when any of its messages falls inside the `--since-days` window. A report filed months ago whose last reply is from yesterday is still an active thread, and trimming the older messages would hide the original report from the import skills that asked for the thread. - `thread_url` needs either an adopter-declared `--archive-template` or the archive path. It refuses to invent a public address: without a template the URL is a `file://` link to the snapshot, which is honest about being unreachable to anyone else. Thread identity is the root's RFC-5322 `Message-ID`, the `rfc5322-message-id` kind the backend declares, so an ID recorded against another backend does not dedupe across kinds. An ancestor missing from the archive makes the message its own root rather than dropping it — that is what a snapshot truncated at the start of a thread looks like. Closes apache#304. Generated-by: WorkBuddy AI (DeepSeek-V4.1-Flash)

Kaap10

Thanks for working on this @sharpca! Landing the read-side for contract:mail-source closes a key gap for forensics/air-gapped triage (#304). The format sniffing, whole-thread retention across --since-days, and orphan-root handling are well-designed.

A few items to address before merge:

Rebase needed (Commit hygiene): The branch is based on an older commit and bundles two commits (9f1da7d9 and f432930e) that were already merged via PR #1752 and PR #1753. This makes the PR show 20 files changed across other packages. Please rebase onto main to keep only the single commit for #304:
```
git fetch upstream main
git rebase --onto upstream/main HEAD~1
git push --force-with-lease origin feat/maildir-archive-read
```
thread_url file URI: In read_archive.py:267, f"file://{root.as_posix()}#{wanted}" produces file://C:/... on Windows (which is missing the third slash for drive letters). Consider using f"{root.as_uri()}#{wanted}" from pathlib.Path for cross-platform RFC-compliant file URIs.
CLI FileNotFoundError handling: In threads_main and read_main, catch FileNotFoundError and print to stderr with exit code 1 so missing archive paths exit cleanly rather than dumping an unhandled Python traceback.
.eml parsing: In iter_messages (line 156), consider email.message_from_bytes(item.read_bytes()) instead of mailbox.mboxMessage(...) since standalone .eml files lack mbox From envelope delimiters.

Once rebased and updated, this should be ready to land!

potiuk

Thanks for landing the read side — the overall shape fits the mail-source contract well. I probed read_archive.py against small synthetic mbox/Maildir archives, and a few things need fixing before this can merge; details are in the inline comments. Most are cheap to cover with tests. (Kaap10's rebase point still stands; that also takes care of the tracker-stats files, which are already on main via #1752.)

One point has no line in this diff: tools/spec-loop/specs/adapters.md (:46–52) still describes tools/maildir/ as drafting-only and points at a separate tools/mail-archive/ reader for mbox/Maildir. AGENTS.md asks for the spec update in this PR or a companion sync-specs PR.

potiuk

Inline versions of the points in my review above, on the lines they refer to. The spec point (tools/spec-loop/specs/adapters.md still describes tools/maildir/ as drafting-only) has no line in this diff, so it stays in the review body.

potiuk · 2026-10-25T15:07:52Z

+
+def _normalise_id(raw: str) -> str:
+    """Strip the angle brackets and surrounding space from a Message-ID."""
+    return raw.strip().strip("<>").strip()


Blocking — thread IDs don't match the IMAP backend. _normalise_id strips the angle brackets and emits root@host, but imap_source emits <root@host>, and both declare thread_id_kind: rfc5322-message-id. The same thread then has two different strings under one kind, so the "already tracked?" check misses it when an adopter switches backends or uses one as a fallback, and a duplicate tracker gets proposed. Please emit the bracketed form, or normalise both backends and pin the format in the contract.

potiuk · 2026-10-25T15:07:52Z

+        while True:
+            parents = [ref for ref in _reference_ids(current) if ref in by_id]
+            if not parents:
+                break
+            # `References` is ordered root-first, so its first in-archive entry
+            # is the closest thing to a root the archive can prove.
+            parent = parents[0]
+            if parent in seen:
+                break
+            seen.add(parent)
+            root = parent
+            current = by_id[parent][1]
+        roots.setdefault(root, []).append((key, msg))


Blocking — orphaned replies split a thread. When the root isn't in the archive, each direct reply becomes its own thread: two replies to an absent <R@h> come back as threads r1@h and r2@h. IMAP keys on the first References ID even when that message is absent, so the two backends also disagree on the thread ID. Keying on the first References/In-Reply-To ID would keep the replies together and match IMAP.

potiuk · 2026-10-25T15:07:52Z

+        if dates and max(dates) < cutoff:
+            continue
+        summaries.append(_thread_summary(thread_id, members))
+    summaries.sort(key=lambda item: item["last_date"] or "", reverse=True)


Newest-first order across time zones: sorting compares isoformat() strings that keep each message's UTC offset. A message at 12:00+00:00 sorts after one at 13:00+05:00 (08:00Z). Sort on the aware datetime, or convert to UTC first.

potiuk · 2026-10-25T15:07:52Z

+def _body_text(msg: Message) -> str:
+    """The first ``text/plain`` part, decoded; ``""`` when the message has none."""
+    if not msg.is_multipart():
+        return _decode_payload(msg)
+    for part in msg.walk():
+        if part.get_content_type() != "text/plain" or part.get_filename():
+            continue
+        text = _decode_payload(part)
+        if text:
+            return text
+    return ""
+
+
+def _decode_payload(part: Message) -> str:
+    """A single part's payload as text; ``""`` for a non-bytes or undecodable part."""
+    try:
+        payload = part.get_payload(decode=True)
+    except (AttributeError, LookupError):
+        return ""
+    if not isinstance(payload, bytes):
+        return ""
+    try:
+        return payload.decode(part.get_content_charset() or "utf-8", "replace")
+    except (LookupError, UnicodeDecodeError):
+        return ""


Bodies: an HTML-only multipart message returns "", a single-part text/html message returns raw HTML, and an unknown charset hits LookupError and also returns "". For a security report, an empty body is the worst outcome. Fall back to decode('utf-8', 'replace'), and to the HTML part (tags stripped) when there's no text/plain.

potiuk · 2026-10-25T15:07:52Z

+        "from": str(msg.get("From", "")),
+        "to": str(msg.get("To", "")),
+        "date": when.isoformat() if when else None,
+        "subject": str(msg.get("Subject", "")),


Encoded headers aren't decoded (also the Subject at L222 and L268): a subject comes back as =?utf-8?q?Caf=C3=A9_vuln?=, which breaks [SECURITY] prefix matching and reply subjects. Use make_header(decode_header(v)), or parse with policy=email.policy.default. Note that message_from_bytes alone still uses compat32.

potiuk · 2026-10-25T15:07:52Z

+            seen.add(parent)
+            root = parent
+            current = by_id[parent][1]
+        roots.setdefault(root, []).append((key, msg))


Duplicate Message-IDs: by_id (L177) deduplicates, but every copy is still appended to the thread here, so two copies of one report give message_count 2 and the body twice. Skip members already added.

potiuk · 2026-10-25T15:07:52Z

+    return {
+        "thread_id": thread_id,
+        "thread_id_kind": THREAD_ID_KIND,
+        "subject": str(members[0][1].get("Subject", "")),


Thread subject from a reply (same in read_thread, L268): for a Maildir the keys come back in directory-listing order, so members[0] can be Re: …. Take the subject from the root message, or the earliest one, and fix the "file order" docstring (L142).

potiuk · 2026-10-25T15:07:52Z

+    """
+    wanted = _normalise_id(thread_id)
+    if archive_template:
+        return archive_template.replace("{message_id}", wanted)


The template URL isn't encoded: the raw Message-ID goes into the template, so +, /, ?, # and spaces break the link written to the tracker. Use urllib.parse.quote(id, safe="") as imap_source/archive_url.py does.

potiuk · 2026-10-25T15:07:52Z

+        if msgid is None:
+            # Without a Message-ID a message cannot be threaded; keep it as a
+            # single-message thread keyed by its archive key so it stays visible.
+            roots.setdefault(f"urn:archive-key:{key}", []).append((key, msg))


Nit: a message without a Message-ID gets urn:archive-key:<n> under the rfc5322-message-id kind. For mbox, <n> is a position that shifts between snapshots. Use a distinct kind or a content-derived ID.

potiuk · 2026-10-25T15:07:52Z

+        help=f"How far back to look, in days (default {DEFAULT_SINCE_DAYS}).",
+    )
+    args = parser.parse_args(argv)
+    json.dump(list_recent_threads(args.archive, since_days=args.since_days), sys.stdout, indent=2)


Nit, beyond the FileNotFoundError point: a negative --since-days (and url_main without --archive/--archive-template) raises a ValueError traceback. parser.error would be cleaner.

sharpca added 3 commits October 21, 2026 17:57

Kaap10 reviewed Oct 25, 2026

View reviewed changes

potiuk requested changes Oct 25, 2026

View reviewed changes

potiuk reviewed Oct 25, 2026

View reviewed changes

Conversation

sharpca commented Oct 25, 2026

Summary

Type of change

Test plan

RFC-AI-0004 compliance

Linked issues

Notes for reviewers

Generative AI disclosure

Uh oh!

Kaap10 left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

potiuk left a comment •

edited

Loading