Skip to content

Fix draft_gmail_message: reply-threading crash, threadId not attaching, signature line breaks - #917

Open
erikholz wants to merge 2 commits into
taylorwilsdon:mainfrom
erikholz:fix/reply-threading-and-signature
Open

Fix draft_gmail_message: reply-threading crash, threadId not attaching, signature line breaks#917
erikholz wants to merge 2 commits into
taylorwilsdon:mainfrom
erikholz:fix/reply-threading-and-signature

Conversation

@erikholz

@erikholz erikholz commented Jul 11, 2026

Copy link
Copy Markdown

Three related bugs in draft_gmail_message's reply-threading path, all reproduced against a live Gmail account.

1. Crash when thread_id is set without explicit in_reply_to/references. _fetch_thread_reply_context did an unguarded message_contexts[-1] to pick a fallback target message. If every message in the thread carries the TRASH label, the earlier skip-trash filter empties message_contexts while the thread itself still has messages, so this raises IndexError: list index out of range. Fixed by only taking the fallback when the list is non-empty; a missing target already degrades gracefully everywhere it's consumed.

2. threadId not forwarded to the Gmail API — already fixed on main via #889, included here for completeness/testing, no additional change.

3. Signature line breaks lost when appended to a plain-text body. The general _html_to_text/_HTMLTextExtractor (built for reading arbitrary message bodies as flowing text) inserts no separator between block elements and collapses all whitespace to single spaces, so adjacent signature lines (e.g. one <p> per line) get concatenated with zero whitespace between them -- no amount of post-hoc whitespace collapsing can fix that since there was never a separating character to begin with. Added a dedicated _HTMLSignatureExtractor that treats <br> and block boundaries as real newlines, used only for signature-to-text conversion so the general message-reading path (and its test coverage) is untouched.

Reproduction/verification harness included at scripts/repro_bugs.py. 143 existing tests pass.

Summary by CodeRabbit

  • Bug Fixes
    • Gmail signatures now preserve readable line breaks and formatting when included in outgoing messages.
    • Improved plain-text conversion for HTML signatures.
    • Prevented errors when replying to threads with no available message context.

1. Crash on auto-derived reply headers (list index out of range)
   _fetch_thread_reply_context unconditionally did
   message_contexts[-1] to pick a fallback target message, but
   message_contexts can be empty even when the thread has messages
   if every message in it carries the TRASH label (all get filtered
   out by the earlier skip-trash loop). Guard the fallback so target
   stays None instead of raising IndexError; callers already handle
   a missing target by degrading gracefully to an unthreaded draft.

2. threadId not forwarded to the Gmail API
   Already fixed upstream on main (PR taylorwilsdon#889, merged after the v1.22.0
   release this bug was originally reproduced against, never
   published to PyPI) -- draft_body["message"]["threadId"] is now set
   whenever thread_id/in_reply_to/references are all present.
   Verified empirically against a live thread, no additional patch
   needed here.

3. Signature line breaks lost
   _HTMLTextExtractor (used for reading arbitrary message bodies)
   inserts no separator between block elements and its get_text()
   collapses all whitespace, including newlines, to single spaces --
   correct for flowing message-body text, but it silently
   concatenates adjacent signature lines with zero whitespace
   between them (e.g. "-Erik" immediately followed by "Erik
   Holzhauer" with no break), which no amount of post-hoc whitespace
   collapsing can repair. Added a dedicated _HTMLSignatureExtractor /
   _signature_html_to_text that treats <br> and block-container
   boundaries (div/p/tr/li) as real newlines, and switched the two
   signature-to-text call sites (_append_signature_to_body,
   _build_quoted_reply_body) to use it. The general _html_to_text
   path used for message-body display is untouched, so its existing
   test coverage still holds.

Includes scripts/repro_bugs.py, the harness used to reproduce and
verify all three against a live Gmail account (real traceback for
bug 1, threadId match check for bug 2, rendered signature diff for
bug 3). 143 existing gmail tests still pass.
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 016831d5-8367-4c7c-861b-a766cd37e325

📥 Commits

Reviewing files that changed from the base of the PR and between 30d5438 and 73b1477.

📒 Files selected for processing (2)
  • gmail/gmail_helpers.py
  • gmail/gmail_tools.py

📝 Walkthrough

Walkthrough

Gmail signature HTML now uses a dedicated parser-based conversion path in outgoing HTML and plain-text composition. Reply-context selection also avoids indexing an empty context list when the requested message is unavailable.

Changes

Gmail draft defect handling

Layer / File(s) Summary
Signature HTML conversion
gmail/gmail_helpers.py
Adds a specialized HTML parser that preserves block and <br> line breaks, skips script/style content, normalizes whitespace, and falls back to the original HTML on conversion errors.
Signature composition integration
gmail/gmail_tools.py
Uses the signature-specific converter in outgoing HTML and plain-text signature paths.
Reply-context fallback handling
gmail/gmail_tools.py
Prevents an empty message_contexts list from causing an invalid last-item lookup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: taylorwilsdon, jcwatson11

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description explains the changes, but it does not follow the required template sections or include the checklist and testing fields. Rewrite the PR description using the repository template: fill Description, Type of Change, Testing, Checklist, and Additional Notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main fixes: reply-threading crash and signature line-break handling, though it also mentions threadId forwarding that wasn't changed here.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@taylorwilsdon taylorwilsdon self-assigned this Jul 16, 2026
@taylorwilsdon taylorwilsdon added the bug Something isn't working label Jul 16, 2026
@taylorwilsdon

Copy link
Copy Markdown
Owner

Please move helpers to gmail_helpers.py and drop the bug test script thing and I will get this merged thanks!

…o script

Per maintainer review on the PR:
- Move _HTMLSignatureExtractor and _signature_html_to_text from
  gmail_tools.py into gmail_helpers.py alongside the other helpers,
  importing _signature_html_to_text back into gmail_tools.py at its
  two call sites. The function's parameter is renamed html ->
  signature_html so it does not shadow the html module that
  gmail_helpers.py imports at top level.
- Remove scripts/repro_bugs.py, the one-off live-account harness used
  to reproduce the original bugs.

All 143 gmail tests still pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants