fix(gmail): stop sentinel strings from clobbering drafts - #206
Open
shoemoney wants to merge 1 commit into
Open
Conversation
_extract_body returned human-readable placeholder strings on failure instead of signalling it. gmail_edit_draft fed that straight into a real MIME message and PUT it back to Gmail, so editing only the subject of a draft with an undecodable body silently overwrote the body with the literal text "(Could not decode email body)" and reported success=True. The multipart recursion guard had the same root cause: it checked `nested != "(No email body found)"`, so a nested part whose own body.data failed to decode looked like a successful extraction and short-circuited the loop, discarding an already-collected valid plain_text from an earlier sibling. _extract_body now returns str | None with no sentinel, and raises ValueError only when body data is present but fails to decode - distinguishing "no body" from "couldn't recover the body". The sentinel strings are rendered exactly once, at the gmail_read_email display boundary, so existing read-path behavior is unchanged. gmail_edit_draft now returns success=False with a real error instead of writing a placeholder when it can't recover the existing body. Fixes mozilla-ai#203. Deferred scope from mozilla-ai#200 (base64url padding).
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #203.
The bug
_extract_bodyinsrc/apron_tools/providers/google/gmail/tools.pyreturns human-readable placeholder strings on failure instead of signalling failure to its callers:gmail_edit_draft(:451, now :468) feeds that straight into a real MIME message:which is then encoded and PUT to Gmail. So editing only the subject of a draft (
params.bodyisNone, the documented normal case) whose stored body cannot be decoded — non-UTF-8 or otherwise corrupt — overwrites the real body with the literal text(Could not decode email body)and reportssuccess=True. Unrecoverable data loss presented as a success.The multipart recursion guard (:121) has the same root cause:
It only excludes one of the two sentinels, so a nested part whose own
body.datafails to decode returns(Could not decode email body), which passes the guard and is treated as a successful extraction.return nestedfires immediately and discards an already-collected validplain_textfrom an earlier sibling.The fix
_extract_bodynow returnsstr | Nonewith no sentinel inside the recursion, and raisesValueErroronly when body data is present but fails to decode — that's the one case a caller must not treat as "no body". Multipart traversal collectsplain_text/html_text/nested_textinto the same preference resolution instead of short-circuiting on the first nested part with any content, and a nested decode failure is suppressed the same way a direct part decode failure already was (searches other siblings instead of aborting).The sentinel strings (
(Could not decode email body),(No email body found)) are now rendered exactly once, at thegmail_read_emaildisplay boundary, so existing read-path output is unchanged.gmail_edit_draftnow distinguishes the two cases:params.body is Noneand the existing body decodes fine → carries it forward as before.params.body is Noneand the existing body fails to decode → returnssuccess=Falsewith an error instead of writing a placeholder over it.Verification
The two new regression tests fail on
main:With the fix:
This is the scope deferred from #200 (base64url padding) to keep that PR focused, per the issue body.