Skip to content

feat(email): echo input message_id in triage result#1551

Merged
itomek merged 1 commit into
feat/email-llm-triage-api-1452from
feat/email-msgid-echo-1539
Jun 10, 2026
Merged

feat(email): echo input message_id in triage result#1551
itomek merged 1 commit into
feat/email-llm-triage-api-1452from
feat/email-msgid-echo-1539

Conversation

@itomek

@itomek itomek commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Closes #1539

Stacked on #1547 (base branch feat/email-llm-triage-api-1452). Review after #1547 — the diff here is only the message_id echo.

Why this matters

Before: the triage API accepted message_id on input but never echoed it in the response, so a consuming app couldn't correlate a result back to its input or dedupe — it would re-triage the same emails on every polling loop.

After: EmailTriageResult echoes the input's identifying id — message.message_id for a single email, thread_id for a thread — across both engines and both single/thread paths. The consumer caches by this id to dedupe. The field is Optional, so the frozen #1262 shape stays backward-compatible (existing callers that omit it still validate). Per the AC, consumer correlation is the documented echoed-id route; no stateful server-side cache is added.

Test plan

  • Unit: result echoes message_id for single (heuristic + llm) and thread; validates without the field (backward-compat); sample payloads parse. 374 passed.
  • Lint: black + isort pass.
  • Real-world (Linux + Windows): the live API echoes the id — evidence below.

Real-world evidence (live gaia api start, engine=heuristic; branch + field verified before testing)

OS HEAD single (message_id=rw-single-1) thread (thread_id=rw-thread-7)
Linux — t-nx-strx-halo 6181908 result.message_id = rw-single-1 result.message_id = rw-thread-7
Windows — t-win-radeon 6181908 result.message_id = rw-single-1 result.message_id = rw-thread-7

Both OSes: single echoes the input message.message_id, thread echoes thread_id. Boxes restored to their original branches after the run.

EmailTriageResult had no way to identify which input produced it: a
consuming app polling the triage endpoint could not correlate a result
back to its email or deduplicate across polling loops. The response now
echoes the input's identifying id — message.message_id for a single
email, thread_id for a thread — via a new optional message_id field.

The field is Optional (default None), preserving the frozen #1262
contract shape: existing callers that omit it keep validating unchanged.
The echoed id IS the dedup key — a consumer caches results by it; no
server-side result cache or GET-by-id endpoint is needed (keeps the
contract stateless).
@github-actions github-actions Bot added tests Test changes agents labels Jun 9, 2026
@itomek itomek self-assigned this Jun 10, 2026
@itomek itomek marked this pull request as ready for review June 10, 2026 14:42
@itomek itomek requested a review from kovtcharov-amd as a code owner June 10, 2026 14:42
@github-actions

Copy link
Copy Markdown
Contributor

Summary

Clean, well-scoped additive change: EmailTriageResult now echoes the input's identifying id (message.message_id for a single email, thread_id for a thread) so consumers can correlate and dedupe across polling loops. The field is Optional[str] defaulting to None, so the frozen #1262 contract stays backward-compatible. Wiring is consistent across all four assembly paths (heuristic/LLM × single/thread), the Gmail-API path deliberately leaves it None, and the field types line up — EmailMessage.message_id and ThreadInput.thread_id are both required str, so the echoed value is always present on the contract paths.

The single most important thing: there's nothing blocking here. Test coverage is genuinely good — both engines, both single/thread, and explicit backward-compat assertions — and the tests exercise the real EmailTriageService rather than over-mocking. No security concerns, no breaking changes, no LLM-affecting surface (the contract isn't used as a structured-output schema; the result is assembled in Python), so no eval is required.

Issues Found

🟢 Minor — history-tagged class docstring (contract.py:235)
The Amendment (#1539): block restates what the Field(...) description below it already says, and CLAUDE.md asks that the fix/issue history live in the commit/PR, not inline where it rots. The Field description (contract.py:258) is the right home for the consumer guidance — keep that, trim the class docstring back:

    """The structured analysis of a single email or thread."""

🟢 Minor — comment could be one line (email_routes.py:248)
The three-line block reads more like a changelog entry than a code comment. A single WHY line carries the same signal:

        # Gmail-API path is not a contract request — no id to echo.

🟢 Minor — backward-compat tests overlap test_contract_schema.py (test_msgid_echo.py:396)
TestContractBackwardCompatible duplicates assertions already covered by the new test_result_message_id_* cases in test_contract_schema.py. The file's docstring calls this out as intentional ("a superset to keep this file self-contained"), so this is a judgment call, not a defect — flagging only so it's a conscious choice. Fine to leave as-is.

Strengths

  • Backward compatibility is proven, not assumed. Frozen sample payloads from the existing schema tests are re-validated to confirm omitted message_id still parses to None (test_msgid_echo.py:441, test_contract_schema.py:142).
  • Tests hit the real service. EmailTriageService().triage_request(...) is exercised end-to-end with a lightweight chat double for the LLM path, instead of mocking the unit under test — and test_different_message_ids_echo_correctly guards against ids bleeding across calls.
  • Consistent wiring. The new message_id keyword threads through all four _build_result / _build_result_llm call sites identically, and keyword-only signatures keep the added param unambiguous.

Verdict

Approve with suggestions — only 🟢 minor doc/comment nits; the behavior, tests, and backward-compat guarantees are solid and safe to merge. (Note the PR is stacked on #1547; merge order applies.)

@itomek itomek merged commit 14c681c into feat/email-llm-triage-api-1452 Jun 10, 2026
16 checks passed
@itomek itomek deleted the feat/email-msgid-echo-1539 branch June 10, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant