feat(gmail): warn when drafts update downgrades a rich-text draft to plain-only - #955
feat(gmail): warn when drafts update downgrades a rich-text draft to plain-only#955mcinteerj wants to merge 2 commits into
Conversation
…plain-only gmail drafts update rebuilds the whole message, so updating a draft that has a text/html part (e.g. one composed in Gmail's web UI) with only --body/--body-file silently produces a plain-text-only draft. Gmail then renders the stored hard-wrapped plain text literally, which reads as mangled formatting with no hint of what happened. Attachments (openclaw#680) and reply lineage (openclaw#942) are already carried forward on update; the body is the remaining silent-replacement surface. This adds a stderr-only warning when the existing draft has an HTML body part and the update supplies none. --quote is exempt since quoting regenerates an HTML part; output contracts (stdout/--json) untouched.
|
Codex review: needs changes before merge. Reviewed August 3, 2026, 6:23 PM ET / 22:23 UTC. ClawSweeper reviewWhat this changesAdds a stderr warning when Merge readinessKeep open: the warning works for fetched drafts, but misses a supported all-fields update path that still silently replaces rich text. Priority: P3 Review scores
Verification
How this fits togetherThe Gmail draft-update command may fetch the existing draft, rebuild its message, and send the replacement through Gmail’s drafts API. This warning inspects the fetched MIME tree before rebuilding so callers can detect an HTML-to-plain replacement. flowchart LR
A[CLI update options] --> B[Draft update command]
B --> C[Fetch existing draft]
C --> D[Inspect MIME body]
D --> E{HTML body and no HTML input?}
E -->|Yes| F[Write stderr warning]
E -->|No| G[Rebuild message]
F --> G
G --> H[Gmail drafts API]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Fetch and inspect the existing MIME payload for every plain-only, non-quote update path, then add regression coverage for the all-fields replacement case. Do we have a high-confidence way to reproduce the issue? Yes. Source establishes that Is this the best way to solve the issue? No. Reusing an existing fetch is efficient, but it leaves a supported rich-to-plain replacement path unwarned; extending the fetch predicate is the narrower complete repair. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4a1a6e94707. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
|
@clawsweeper re-review Added the requested real-behavior proof to the PR body: a redacted live Gmail run against a genuine |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…warning The existing-draft fetch is skipped when --to, --reply-to-message-id and --attach are all supplied, leaving existingPayload nil. That path still rebuilds the body, so a rich-text draft was downgraded to plain text without the warning firing. Extend the fetch predicate with the same condition the warning uses (no HTML body supplied and not --quote), so every plain-only, non-quote update inspects the stored MIME tree. The extra fetch is confined to updates that can actually drop an HTML body. Adds a regression test for the all-fields invocation; verified failing without the predicate change.
|
Fixed in 0682a12 — thanks, the finding was correct.
Added |
|
@clawsweeper re-review New head |
|
🦞👀 Command router queued. I will update this comment with the next step. |
Scope
gmail drafts updaterebuilds the whole message, so updating a draft that carries atext/htmlpart (e.g. a reply composed in Gmail's web UI) with only--body/--body-filesilently produces a plain-text-only draft. Gmail then renders the stored ~72-char hard-wrapped plain text literally — the draft looks mangled, with no hint of why.Update already carries forward attachments (#680/#681) and reply lineage (#942/#944); the body is the remaining silent-replacement surface. Since replacing the body is exactly what the caller asked for, this PR doesn't change behaviour — it adds a stderr-only warning when the existing draft has an HTML body part and the update supplies none:
--quoteis exempt: quoting regenerates an HTML part..htmlfile doesn't trigger it).--to+--reply-to-message-id+--attach) that previously skipped the fetch entirely (see review follow-up below).--json/--plainoutput contracts untouched.Motivation
Real-world agent workflow: an update passing only
--bodyon a Gmail-composed reply draft downgraded it to plain-only; the resulting "mangled wrapping" took a while to diagnose because nothing signalled the multipart → plain transition (2026-08-03, gogcli v0.17.0 — but the same applies on main).Real behavior proof (live Gmail, redacted)
Run against a real Gmail account through the real API — a throwaway rich-text draft (
multipart/alternative), updated with the unpatched v0.34.2 binary and then this branch's binary. Account address and draft id redacted; MIME trees printed fromgmail drafts get --json.mimetreeis just a shell helper that walkspayload.partsand printsmimeTypeper level. The test draft was deleted afterwards; no other drafts were touched, and nothing was sent.Review follow-up — the all-fields path (0682a12)
The first review correctly caught that
internal/cmd/gmail_drafts.goskips the existing-draft fetch when--to,--reply-to-message-idand--attachare all supplied, leavingexistingPayloadnil — that invocation still rebuilt the body, so it downgraded rich text unwarned.Fixed by extending the fetch predicate with the same condition the warning uses (
no HTML body supplied && !--quote), so the extra fetch is confined to updates that can actually drop an HTML body. Live re-verification of exactly that combination:The regression test was confirmed to fail without the predicate change (
expected downgrade warning on the all-fields update path, got:with empty stderr) and pass with it.Testing
make fmtclean,go vetclean;go test ./internal/cmd/ -count=1passes (full package, 77s).TestGmailDraftsUpdateCmd_WarnsWhenPlainBodyReplacesHTMLDraft(warning when a multipart/alternative draft is updated with--bodyonly),TestGmailDraftsUpdateCmd_NoWarnWhenHTMLBodyProvided(silent with--body-html), andTestGmailDraftsUpdateCmd_WarnsWhenAllFieldsUpdateSkipsFetchGuard(the--to+--reply-to-message-id+--attachpath).User-facing changes
New stderr warning only; no flags added or changed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CEKp3XY4d4ktX7LJBcgGVs