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 3 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.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed August 5, 2026, 7:21 AM ET / 11:21 UTC. ClawSweeper reviewWhat this changesAdds a stderr warning when Merge readinessKeep open for a maintainer UX decision. Current main still silently rebuilds a rich-text draft as plain-only when no HTML body is supplied; this focused advisory patch is technically sound and has real Gmail proof. Priority: P2 Review scores
Verification
How this fits togetherThe Gmail draft-update command reads stored draft state when needed, rebuilds the MIME message from CLI inputs, and writes the replacement through Gmail’s API. The added MIME inspection warns on stderr before a plain-only rebuild removes an existing HTML body. flowchart LR
A[Draft update flags] --> B[Draft update command]
B --> C[Existing Gmail draft]
C --> D[MIME body inspection]
D --> E[Warning decision]
B --> F[Rebuilt draft message]
E --> G[stderr advisory]
F --> H[Gmail draft update]
Decision needed
Why: The patch is correct, but adding a new user-visible warning is a product and CLI-UX choice rather than a repair to an established output contract. Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: If maintainers want this extra user guidance, retain the existing replacement semantics and merge the MIME-aware stderr advisory with its regression coverage. Do we have a high-confidence way to reproduce the issue? Yes. Current main rebuilds the outgoing MIME message from supplied bodies without preserving or inspecting an existing HTML body; the PR also supplies redacted live Gmail before-and-after terminal evidence. Is this the best way to solve the issue? Unclear pending product direction: an stderr-only warning is technically the narrowest approach, but maintainers must choose whether this new CLI guidance belongs in the default experience. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4a1a6e94707. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (16 earlier review cycles; latest 8 shown)
|
|
@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:
|
clawsweeper
Bot
added
proof: sufficient
…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 |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
rating: 🦪 silver shellfish
The plain-only downgrade warning added the existing-draft fetch to updates that previously needed no read at all (--to + --reply-to-message-id + --attach). That fetch kept the original `return fetchErr`, so a transient Drafts.Get failure aborted an update main would have performed. Split the predicate: a read the rebuilt message depends on (stored recipients, reply lineage, attachment carry-forward) stays fatal; a read wanted only for the warning is advisory and a failure costs the warning, not the update. Tests: AdvisoryFetchFailureStillUpdates (failing GET, PUT still sent, no warning) and RequiredFetchFailureAborts (no --to, failing GET, no PUT).
|
@clawsweeper re-review New head The previous head added Predicate and error handling are now separate: requireExistingDraft := (!toWasSet && !c.ReplyAll) || strings.TrimSpace(replyToMessageID) == "" || preserveAttachments
inspectForHTMLDowngrade := strings.TrimSpace(htmlBody) == "" && !c.Quote
if requireExistingDraft || inspectForHTMLDowngrade {
existing, fetchErr := svc.Users.Drafts.Get("me", draftID).Format("full").Do()
// Advisory whenever nothing but the warning wants it: a failed inspection
// costs the warning, never the update the caller asked for.
if fetchErr != nil && requireExistingDraft {
return fetchErr
}
if fetchErr == nil && existing != nil && existing.Message != nil {
...
}
}A read the rebuilt message depends on — stored recipients (no Both halves have regression tests, on an
Live re-verification that the user-facing behaviour is unchanged by the split — same all-fields path, patched binary, real Gmail account (ids redacted): $ gog gmail drafts get r6691291…3717 --json | mimetree
multipart/alternative
text/plain
text/html
$ gog-patched gmail drafts update r6691291…3717 \
--to me@example.com --reply-to-message-id 19fce745…1210 --attach note.txt \
--subject "test draft" --body "Downgraded via the all-fields path." --json >out.json
Warning: draft has an HTML body; this update replaces it with plain text only. Pass --body-html/--body-html-file to keep rich-text formatting.
exit=0
$ head -4 out.json <-- stdout still clean JSON
{
"attachments": [
{
"filename": "note.txt",
$ gog gmail drafts get r6691291…3717 --json | mimetree
multipart/mixed <-- warning was truthful
text/plain
text/plain (the attachment part)Test draft deleted afterwards; nothing was sent. Checks on this head: |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Landed in #957 after maintainer review as commit c68441a, with the contributor credit preserved. The landed version includes the original warning behavior and regression coverage, keeps advisory MIME inspection non-blocking, adds a guard so HTML nested inside an attached message does not trigger a false warning, and adds the changelog credit. Local |
2 participants
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
Review follow-up — the advisory read (8dbbdfc)
The second review was right that the previous head over-reached: adding
inspectForHTMLDowngradeto the fetch predicate made the existing-draftDrafts.Gethappen on invocations that need nothing from it (--to+--reply-to-message-id+--attach), while the originalreturn fetchErrwas left in place. A transient read failure would then abort an update that main performs — a warning-only read blocking the write it was meant to annotate.Fixed by splitting the predicate from the error handling:
A read the rebuilt message depends on — stored recipients (no
--to), reply lineage (no--reply-to-message-id), attachment carry-forward — still returns the error, because proceeding there would silently drop data. A read wanted only for the warning does not.Two regression tests cover both halves, on an
httptestserver that fails the draftGETand accepts thePUT:TestGmailDraftsUpdateCmd_AdvisoryFetchFailureStillUpdates— all-fields invocation,GETreturns 503: command exits 0, exactly onePUTis sent, and no warning is printed (none is possible without the payload). Confirmed to fail on the previous head:advisory draft read failure must not abort the update: googleapi: got HTTP response code 503 with body: transient draft read failure.TestGmailDraftsUpdateCmd_RequiredFetchFailureAborts— no--to, same failingGET: the command errors and zeroPUTs are sent, so the advisory relaxation didn't leak into reads that matter.Live re-verification that the user-facing behaviour is unchanged by the error-handling split — same all-fields path, patched binary, real Gmail account (redacted):
The test draft was deleted afterwards; nothing was sent.
Checks on this head:
goimports/gofumptclean,go vet ./internal/cmd/clean,golangci-lint run ./internal/cmd/...0 issues,go test ./internal/cmd/ -count=1passes (139s).