fix(gmail): never silently drop a requested quote - #996
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>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed August 21, 2026, 6:59 PM ET / 22:59 UTC. ClawSweeper reviewWhat this changesThe PR makes quote-enabled Gmail replies and forwards fail or warn instead of silently omitting the requested quote or panicking on an empty API response. Merge readinessThis PR remains necessary: current main still ignores a full quote-source fetch failure and can then send or save without the requested quote. The patch is focused, has no actionable correctness finding, and supplies strong redacted real-behavior proof plus targeted no-POST regression coverage. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherGmail compose commands select an original message or thread, extract reply context and quote text, then send or save the resulting message through the Gmail API. The quote path must preserve parseable stdout while reporting non-fatal quote omissions on stderr. flowchart LR
A[CLI reply or send request] --> B[Select message or thread]
B --> C[Fetch original message]
C --> D{Quote text available?}
D -->|yes| E[Compose quoted message]
D -->|fetch failure| F[Return error before send]
D -->|no text| G[Warn on stderr]
E --> H[Send or save via Gmail API]
G --> H
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow fail-closed quote-fetch handling with its regression coverage, preserving stderr-only warning behavior when an original genuinely has no extractable text. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main ignores the full quote-fetch error in the thread path, while the added local HTTP-handler tests provide a deterministic failing-fetch/no-POST path. The PR also includes redacted live Gmail output for the successful quote and textless-original warning paths. Is this the best way to solve the issue? Yes: propagating the error before composition is the narrowest way to honor an explicitly requested quote, while a stderr warning preserves normal reply behavior for attachment-only originals. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ab7e0ed706f9. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
1 participant
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