fix(gmail): never silently drop a requested quote - #996
Open
malob wants to merge 1 commit into
Open
Conversation
The thread-ID reply path fetched the quote source in full format but ignored fetch errors, so 'gmail send --thread-id X --quote' and 'gmail drafts create --thread-id X --quote' sent or saved the message without the requested quoted original. Fail closed instead: - propagate quote-source fetch errors on the thread path - treat an empty API response as an error on the reply-target and forward fetch paths (previously a nil message panicked these paths when composing with the original's body) - warn on stderr when the original yields no extractable quotable text, instead of silently composing without the quote; this also covers the default-on quoting of 'gmail reply' Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 15, 2026, 6:56 AM ET / 10:56 UTC. ClawSweeper reviewWhat this changesThe PR makes Gmail quote-enabled replies and forwards error instead of silently omitting a requested quote or dereferencing an empty API response, while warning on stderr for attachment-only originals. Merge readinessThis PR remains necessary: current main still silently continues after a failed full-message quote fetch. The patch is focused, proof-backed, and ready for maintainer merge review. Priority: P2 Review scores
Verification
How this fits togetherGog’s Gmail commands fetch a reply target, derive threading and quote content, then compose and send or save a message. This change sits between Gmail API reads and the compose/send paths, where it determines whether a requested quote is included or a failure is reported. flowchart LR
A[User Gmail command] --> B[Reply target lookup]
B --> C{Quote requested?}
C -->|yes| D[Fetch full original]
D --> E[Build quoted message]
E --> F[Send or save draft]
C -->|no| F
D -->|fetch failure| G[Return error]
E -->|no text found| H[Warn on stderr]
H --> F
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt the fail-closed source-fetch behavior and stderr-only no-text warning, retaining the focused regression tests for send and draft creation. Do we have a high-confidence way to reproduce the issue? Yes. The focused HTTP handler provides a high-confidence path: metadata thread lookup succeeds, the required full-message quote lookup fails, and the command must not POST a send or draft. Is this the best way to solve the issue? Yes. Propagating the required quote-source failure at the shared reply-information boundary is narrower and safer than allowing a quote-enabled command to continue unquoted. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 45b5d766e137. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
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
1 participant
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.
What
gmail send --thread-id X --quote(andgmail drafts create --thread-id X --quote) could send or save the message without the requested quoted original. The thread-ID reply path refetches the selected message in full format to get the quote body, but ignored the fetch error and fell back to the metadata-only message — which has no body parts — so the quote silently became a no-op and the mail went out anyway:This PR makes the quote pipeline fail closed end to end: a requested quote is either applied, or the command errors, or it says on stderr why it couldn't quote. Three layers:
fetch message <id> for quoting: <cause>), aborting before any send/draft POST.(nil, nil)for a200with a literalnullbody (decode goes through**Message, so JSONnullnils the pointer) — real Gmail won't produce this, but a broken proxy can, and previously it panicked (msg.Id/origMsg.Payloadnil dereference) when composing with the original's body.Why
The direct
--reply-to-message-idpath already failed closed (a fetch error aborts the compose); the thread path was the outlier, and the failure mode is the worst kind — the mail is already sent when you notice the quote is missing. The warning layer covers the remaining case where the fetch succeeds but there is genuinely nothing to quote: hard-failing there would break replying to attachment-only messages, so the compose proceeds but is no longer silent about it.User-facing changes
Warning: could not extract quotable text from the original message; composing without quote. Becausegmail reply/gmail drafts replyquote by default (--no-quoteto opt out), the warning can fire there too, not only under an explicit--quote. JSON/plain stdout is unchanged — the warning goes to stderr only (proven below).fetch message <id> for quoting: <cause>and… empty response(reply paths),fetch original message <id>: empty response(forward paths). Previously these situations sent without the quote or panicked.Changes
internal/cmd/gmail_reply.go— thread path: propagate the full-fetch error and reject a nil message; message-ID path: reject a nil message (previously panicked when quoting). NewreplyInfo.hasQuotableText()used by both the quoting no-op branch and the new warning, so the two conditions cannot drift apart.internal/cmd/gmail_compose.go—applyReplyQuotewarns on stderr when a requested quote has no quotable text (single chokepoint shared by send, drafts create/update, and reply/reply-all).internal/cmd/gmail_forward.go—buildForwardComposeMessageerrors on a nil fetched message instead of panicking (coversgmail forwardandgmail drafts forward).internal/cmd/gmail_quote_fetch_fail_test.go(new) — seven tests, detailed under Testing.Behavior proof
Live against a real account, redacted: the address appears as
<me>, message/thread ids as<src>/<reply>/<att-src>/<warn-reply>. Timestamps are literal.1. Happy path unchanged — a quoted thread reply still carries the quote:
2. The new warning — quoting an original with no text parts. The original is an attachment-only MIME message (single
application/pdfpart, notext/*parts), imported verbatim:The reply threads correctly and contains no quote block — and now says so, on stderr only (stdout above is the clean JSON).
3. The fail-closed core — not live-triggerable, proven by mutation. This path needs Gmail's API to fail mid-compose (a 5xx or a null response on the second fetch), which a real account can't produce on demand. The regression tests pin it: the mock serves the thread fine, 500s the full-format quote fetch, and fails the test on any POST. Restoring the pre-fix files reproduces the fail-open literally — the mock server receives the send POST:
(The panic is the pre-fix nil dereference the empty-response guard removes; the run aborts there before reaching the drafts-create test, which fails the same way when run alone.)
Testing
gmail_quote_fetch_fail_test.go, all passing with-race:TestFetchReplyInfo_ThreadIDQuote_FullFetchFailurePropagates— fetch error propagates, wrappedgoogleapi.Error(500) preserved through%w(checked viaerrors.As).TestGmailSendCmd_ThreadIDQuote_FullFetchFailureAbortsSend/TestGmailDraftsCreateCmd_ThreadIDQuote_FullFetchFailureAbortsCreate— command-level: the run errors and the mock receives zero POSTs (any POST fails the test).TestFetchReplyInfo_Quote_NullResponseFailsClosed—200 null→empty responseerror on both the message-ID and thread-ID paths (previously: silent metadata fallback / panic).TestBuildForwardComposeMessage_NullResponseFailsClosed— same for the forward fetch (previously: panic).TestGmailSendCmd_ThreadIDQuote_NoQuotableTextWarns— attachment-only original: warning on stderr, absent from stdout, message sent without a quote block.TestGmailSendCmd_ThreadIDQuote_QuotesWithoutWarning— quotable original: quote present in the sent raw, no warning.make fmt-check lint deadcode docs-check agent-skills-checkclean.docs/spec.mdneeds no sync (no flag/command changes).🤖 Generated with Claude Code