Skip to content

feat(gmail): warn when drafts update downgrades a rich-text draft to plain-only - #955

Open
mcinteerj wants to merge 2 commits into
openclaw:mainfrom
mcinteerj:warn-plain-only-draft-update
Open

feat(gmail): warn when drafts update downgrades a rich-text draft to plain-only#955
mcinteerj wants to merge 2 commits into
openclaw:mainfrom
mcinteerj:warn-plain-only-draft-update

Conversation

@mcinteerj

@mcinteerj mcinteerj commented Aug 3, 2026

Copy link
Copy Markdown

Scope

gmail drafts update rebuilds the whole message, so updating a draft that carries a text/html part (e.g. a reply composed in Gmail's web UI) with only --body/--body-file silently 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:

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.
  • --quote is exempt: quoting regenerates an HTML part.
  • Attachment parts don't count as an HTML body (an attached .html file doesn't trigger it).
  • The existing-draft fetch predicate is extended with the same condition the warning uses, so every plain-only non-quote update inspects the stored MIME tree — including the all-fields path (--to + --reply-to-message-id + --attach) that previously skipped the fetch entirely (see review follow-up below).
  • stdout/--json/--plain output contracts untouched.

Motivation

Real-world agent workflow: an update passing only --body on 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 from gmail drafts get --json.

$ # (0) starting point — a genuine rich-text draft, as Gmail composes them
$ gog gmail drafts get r2430190…0882 --json | mimetree
multipart/alternative
  text/plain
  text/html

$ # (1) BEFORE — unpatched v0.34.2, update supplying only --body
$ gog-v0.34.2 gmail drafts update r2430190…0882 \
    --subject "test draft" --body "Downgraded silently." --json >/dev/null
exit=0   stderr bytes=0            <-- nothing said
$ gog gmail drafts get r2430190…0882 --json | mimetree
text/plain                          <-- HTML part gone

$ # (2) AFTER — this branch, same real draft restored to rich text, same flags
$ gog-patched gmail drafts update r2430190…0882 \
    --subject "test draft" --body "Hello there. Updated with plain text only." --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 -3 out.json                  <-- stdout still clean JSON
{
  "draftId": "r2430190…0882",
  "inReplyTo": null,
$ gog gmail drafts get r2430190…0882 --json | mimetree
text/plain                          <-- warning was truthful

$ # (3) AFTER — rich draft again, this time supplying --body-html
$ gog-patched gmail drafts update r2430190…0882 \
    --body "Hello there. Updated, rich text kept." \
    --body-html '<div dir="ltr"><div>Hello there. Updated, rich text kept.</div></div>' --json >out.json
exit=0   stderr bytes=0            <-- silent, as intended
$ gog gmail drafts get r2430190…0882 --json | mimetree
multipart/alternative               <-- rich text preserved
  text/plain
  text/html

mimetree is just a shell helper that walks payload.parts and prints mimeType per 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.go skips the existing-draft fetch when --to, --reply-to-message-id and --attach are all supplied, leaving existingPayload nil — 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:

$ gog gmail drafts get r7187264…6691 --json | mimetree
multipart/alternative
  text/plain
  text/html

$ gog-patched gmail drafts update r7187264…6691 \
    --to you@example.com --reply-to-message-id 19c208a…b345 --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

$ gog gmail drafts get r7187264…6691 --json | mimetree
multipart/mixed                     <-- body downgraded, warning now fires
  text/plain
  text/plain                        (the attachment part)

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 fmt clean, go vet clean; go test ./internal/cmd/ -count=1 passes (full package, 77s).
  • Tests: TestGmailDraftsUpdateCmd_WarnsWhenPlainBodyReplacesHTMLDraft (warning when a multipart/alternative draft is updated with --body only), TestGmailDraftsUpdateCmd_NoWarnWhenHTMLBodyProvided (silent with --body-html), and TestGmailDraftsUpdateCmd_WarnsWhenAllFieldsUpdateSkipsFetchGuard (the --to + --reply-to-message-id + --attach path).
  • Plus the two live Gmail runs above.

User-facing changes

New stderr warning only; no flags added or changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CEKp3XY4d4ktX7LJBcgGVs

…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 clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Aug 3, 2026
@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 3, 2026, 6:23 PM ET / 22:23 UTC.

ClawSweeper review

What this changes

Adds a stderr warning when gmail drafts update would replace a fetched rich-text Gmail draft body with plain text only.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Keep open: the warning works for fetched drafts, but misses a supported all-fields update path that still silently replaces rich text.

Priority: P3
Reviewed head: 14b2241ad86d14c5950e02c269b5bbf5c7bb8068

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Strong live proof supports the claimed path, but a supported replacement path bypasses the new warning.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): A redacted live Gmail API transcript in the PR body demonstrates the before/after warning behavior and the HTML-body control case; the remaining gap is a separate option combination.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): A redacted live Gmail API transcript in the PR body demonstrates the before/after warning behavior and the HTML-body control case; the remaining gap is a separate option combination.
Evidence reviewed 6 items Current fetch guard: Current main fetches the existing MIME payload only when recipients, reply context, or attachment preservation needs it; an all-fields replacement can skip this fetch.
Proposed warning: The branch warning depends on existingPayload, so it cannot fire when the existing-draft fetch is skipped.
Release check: v0.34.2 contains the same fetch guard and no HTML-body warning; the requested safeguard is not already shipped or on current main.
Findings 1 actionable finding [P2] Inspect all plain-only update paths
Security None None.

How this fits together

The 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]
Loading

Before merge

  • Inspect all plain-only update paths (P2) - When callers provide --to, --reply-to-message-id, and --attach, the existing fetch guard is false, leaving existingPayload nil. That invocation still rebuilds an HTML draft as plain text but never reaches this warning. Include warning inspection in the fetch predicate and add this regression case. Late finding: this head is unchanged since the previous review.
  • Resolve merge risk (P1) - An update supplying --to, --reply-to-message-id, and --attach skips the draft fetch, so a rich-text draft can still be silently downgraded despite this warning feature.
  • Complete next step (P2) - A narrow, mechanical repair can make the warning cover the supported path that currently skips MIME inspection.

Findings

  • [P2] Inspect all plain-only update paths — internal/cmd/gmail_drafts.go:899-900
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +26, tests +81 The focused helper and warning have substantially more targeted test coverage than production code.
Files affected 2 files The patch remains tightly scoped to Gmail draft updating and its command tests.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    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.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best 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 --to plus --reply-to-message-id plus --attach makes the existing-draft fetch guard false, leaving the new MIME inspection nil while the update still rebuilds the body.

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:

  • [P2] Inspect all plain-only update paths — internal/cmd/gmail_drafts.go:899-900
    When callers provide --to, --reply-to-message-id, and --attach, the existing fetch guard is false, leaving existingPayload nil. That invocation still rebuilds an HTML draft as plain text but never reaches this warning. Include warning inspection in the fetch predicate and add this regression case. Late finding: this head is unchanged since the previous review.
    Confidence: 0.95
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against d4a1a6e94707.

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): A redacted live Gmail API transcript in the PR body demonstrates the before/after warning behavior and the HTML-body control case; the remaining gap is a separate option combination.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P3: This is a low-blast-radius CLI warning improvement, although one supported option combination remains uncovered.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): A redacted live Gmail API transcript in the PR body demonstrates the before/after warning behavior and the HTML-body control case; the remaining gap is a separate option combination.
  • proof: sufficient: Contributor real behavior proof is sufficient. A redacted live Gmail API transcript in the PR body demonstrates the before/after warning behavior and the HTML-body control case; the remaining gap is a separate option combination.

Evidence

Acceptance criteria:

  • [P1] go test ./internal/cmd/ -run 'TestGmailDraftsUpdateCmd_(WarnsWhenPlainBodyReplacesHTMLDraft|NoWarnWhenHTMLBodyProvided)' -count=1.
  • [P1] go test ./internal/cmd/ -count=1.

What I checked:

  • Current fetch guard: Current main fetches the existing MIME payload only when recipients, reply context, or attachment preservation needs it; an all-fields replacement can skip this fetch. (internal/cmd/gmail_drafts.go:857, d4a1a6e94707)
  • Proposed warning: The branch warning depends on existingPayload, so it cannot fire when the existing-draft fetch is skipped. (internal/cmd/gmail_drafts.go:899, 14b2241ad86d)
  • Release check: v0.34.2 contains the same fetch guard and no HTML-body warning; the requested safeguard is not already shipped or on current main. (internal/cmd/gmail_drafts.go:857, 1c5a1ec15fa8)
  • Feature provenance: The current draft-update preservation and reply-context flow was recently refined by the Gmail draft self-reference fix. (internal/cmd/gmail_drafts.go:857, 4985e2681e31)
  • Real behavior proof: The PR body and follow-up comment provide a redacted live Gmail before/after run showing the warning, clean JSON stdout, and the HTML-input control case. (14b2241ad86d)
  • Re-review continuity: The prior completed review used the same head SHA; this finding is a late discovery on unchanged code. (internal/cmd/gmail_drafts.go:899, 14b2241ad86d)

Likely related people:

  • chrischall: Introduced the recent draft-update reply-context correction that established the surrounding fetch and preservation flow. (role: recent Gmail draft-update contributor; confidence: high; commits: 4985e2681e31; files: internal/cmd/gmail_drafts.go)
  • Peter Steinberger: Feature history shows the largest contribution count on the central command file and a recent dry-run adjustment in this path. (role: long-term Gmail command contributor; confidence: medium; commits: 6db9b084fd28; files: internal/cmd/gmail_drafts.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Extend the existing-draft fetch condition for plain-only updates and add a regression test using --to, --reply-to-message-id, and --attach.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-03T21:15:58.064Z sha 14b2241 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T22:15:28.807Z sha 14b2241 :: needs maintainer review before merge. :: none

@mcinteerj

Copy link
Copy Markdown
Author

@clawsweeper re-review

Added the requested real-behavior proof to the PR body: a redacted live Gmail run against a genuine multipart/alternative draft, showing (1) unpatched v0.34.2 downgrading it to text/plain with zero bytes on stderr, (2) this branch emitting the warning for the same update while stdout stays clean JSON, and (3) no warning when --body-html is supplied, with the rich body preserved. Test draft was deleted afterwards; nothing was sent.

@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 3, 2026
…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.
@mcinteerj

Copy link
Copy Markdown
Author

Fixed in 0682a12 — thanks, the finding was correct.

--to + --reply-to-message-id + --attach made all three fetch-guard conditions false, so existingPayload was nil and the warning couldn't fire on a path that still replaced the body. The fetch predicate now carries the same condition as the warning (no HTML body supplied and not --quote), so every plain-only non-quote update inspects the stored MIME tree while the extra fetch stays confined to updates that can actually drop an HTML body.

Added TestGmailDraftsUpdateCmd_WarnsWhenAllFieldsUpdateSkipsFetchGuard for that invocation; verified it fails without the predicate change and passes with it. PR body has a live Gmail transcript of the same combination — rich draft in, warning emitted, resulting MIME multipart/mixed with no text/html. Full internal/cmd suite, make fmt and go vet clean.

@mcinteerj

Copy link
Copy Markdown
Author

@clawsweeper re-review

New head 0682a12 addresses the P2 finding: the existing-draft fetch predicate now carries the same condition as the warning (no HTML body supplied and not --quote), so the --to + --reply-to-message-id + --attach path no longer skips MIME inspection. Added TestGmailDraftsUpdateCmd_WarnsWhenAllFieldsUpdateSkipsFetchGuard for that invocation (verified failing without the predicate change), plus a live Gmail transcript of the same combination in the PR body.

@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant