fix(gmail): stop drafts update referencing the draft's own revision - #944
Conversation
`gmail drafts update` fed the draft's existing threadId back in as a reply target, so rebuilding a draft resolved In-Reply-To/References from the newest message in that thread. For a draft that is not a reply, the only message in the thread is the draft itself, so every update anchored the draft to its own previous revision — a message that was never sent and that no recipient's mail client can thread against. Each further update chained onto the last. Thread continuity and reply lineage were riding the same wire: msg.ThreadId is set from the resolved reply info, so simply dropping the thread as a reply target would also have dropped threadId. Separate them. - Only a caller-supplied --reply-to-message-id/--thread-id is a reply target. - Carry threadId forward independently, as thread continuity. - Preserve genuine reply context by re-applying the draft's own stored In-Reply-To/References verbatim rather than re-deriving them, so repeated updates are idempotent and References does not accumulate. - Never reference an unsent message: filter DRAFT-labelled messages out of thread reply-target selection, and refuse a --reply-to-message-id naming a draft. The draft-safe selector already existed for --quote; the header path was using the unfiltered one. - Report the effective inReplyTo/references (and their provenance) on drafts create/update so callers can verify threading without a raw-header fetch, warning on stderr when the lineage was carried rather than caller-specified. - Add --clear-reply-context to strip reply headers in place, so repairing a mis-threaded draft no longer requires delete-and-recreate. Fixes openclaw#942
paulmiu
left a comment
paulmiu
left a comment
There was a problem hiding this comment.
Verified against a real production failure with the same shape: updating a legitimate Gmail reply draft caused the old implementation to select the draft revision as the newest thread message, then the replacement draft carried an In-Reply-To value whose target no longer existed. This PR fixes the underlying model by separating thread continuity from reply lineage, preserves genuine headers idempotently, excludes drafts as reply targets, and provides an explicit recovery path for malformed legacy drafts. I also ran all focused TestGmailDraftsUpdateCmd_ regression cases locally with Go 1.26.5; they pass. The complete upstream CI matrix is green. This is the right fix.
|
I added a follow-up contribution that turns reply-context handling into a permanent integrity guarantee: chrischall#9 This is broader than repairing drafts affected by an older gog release. The follow-up establishes these invariants for every reply-draft update:
The known legacy corruption is an important acceptance case, not the scope of the change. Correct threading becomes the default for human and automated callers without requiring knowledge of a recovery flag. Focused draft-update tests, build, formatting, lint, dead-code analysis, generated docs/coverage, and agent-skill generation all pass. |
paulmiu
left a comment
paulmiu
left a comment
There was a problem hiding this comment.
Clarification to my earlier review: the companion improvement in chrischall#9 is not primarily legacy cleanup. It makes stored reply headers a validated invariant for every drafts update rewrite.
In-Reply-To and References can become stale through any API client, imports, deleted messages, manual tooling, future edge cases, or the known older gog bug. The contribution validates healthy context, automatically repairs stale context using real messages in the Gmail thread, fails before mutation when repair is unsafe, and exposes explicit / preserved / repaired state to automation. Standalone drafts remain on the zero-fetch path.
The legacy corruption is one regression case proving the need for this ongoing guardrail; the long-term benefit is that gog no longer silently accepts invalid reply context from any source.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 2:30 AM ET / 06:30 UTC. ClawSweeper reviewWhat this changesThis PR prevents Gmail draft updates from creating self-referential reply headers, preserves thread continuity and genuine reply context, and adds explicit recovery and threading output. Merge readinessKeep this PR open for normal landing review. Current Priority: P2 Review scores
Verification
How this fits togetherThe Gmail drafts command fetches an existing draft, rebuilds its MIME message, and writes it back through Gmail. Reply lineage determines flowchart LR
A[Existing Gmail draft] --> B[Draft update command]
C[Caller reply flags] --> B
B --> D[Resolve reply lineage]
B --> E[Preserve thread continuity]
D --> F[Rebuilt MIME headers]
E --> G[Updated Gmail draft]
F --> G
G --> H[CLI JSON and warnings]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the focused repair after required checks complete, preserving existing thread placement while using Do we have a high-confidence way to reproduce the issue? Yes. The PR discussion provides a live Gmail reproduction against current main, and current source independently shows the existing draft thread being passed as reply lineage. Is this the best way to solve the issue? Yes. Separating stable Gmail thread placement from reply-header lineage is the narrowest maintainable fix; the explicit clear flag handles historical malformed drafts without silently rewriting ambiguous stored headers. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1b261244fc26. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (22 earlier review cycles; latest 8 shown)
|
AGENTS.md puts CHANGELOG.md in the maintainer's landing checklist
("update CHANGELOG.md (PR #/issue + thanks)"), so a contributor PR
should not carry the entry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WB8tgULwQHCtFbHwXhS2Cc
|
Both items addressed. ✅ Remove the release-owned changelog entryDropped ( ✅ Real behavior proofBuilt from The bug, reproduced on current Standalone draft on this branch — the first of the two behaviors you asked to see: No reply headers, and Gmail kept the thread — which is the specific thing you flagged as unproven: thread continuity survives while the self-referential lineage does not. Genuine reply on this branch — the second behavior. A draft replying to a real received message, then updated four times: The real parent is preserved verbatim, Recovery, live. Taking the draft All test drafts were deleted afterward.
|
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Maintainer follow-up: I fixed the remaining The command now rejects Proof:
This clears the concrete code-review blocker. My recommendation is LAND: the core bug and recovery path have direct live-Gmail proof in the existing discussion, and the branch is now internally consistent. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
3 participants
Fixes #942.
gmail drafts updaterebuilds the whole message, and it was feeding the draft's ownthreadIdback in as a reply target:That flows to
fetchReplyInfo, which anchorsIn-Reply-To/Referencesto the newest message in the thread. For a draft that is not a reply, the only message in its thread is the draft — so every update pointed the draft at its own previous revision, a message that was never sent. A second update chained onto the first.The two concepts were riding the same wire:
msg.ThreadIdis set from the resolved reply info inbuildGmailMessage, so simply not passing the thread as a reply target would also have droppedthreadId. This separates them.Changes
--reply-to-message-id/--thread-idis a reply target. The draft's own thread never is.ThreadContinuityIDondraftComposeInputapplied only when no reply target resolved a thread of its own.In-Reply-To/Referencesverbatim instead of re-deriving them. Repeated updates are therefore idempotent andReferencesdoes not accumulate.fetchReplyInfonow filtersDRAFT-labelled messages out of thread reply-target selection, and an explicit--reply-to-message-idpointing at a draft is refused. The draft-safe selector (selectLatestThreadReplyTarget) already existed but was wired only into the--quotepath; the header path was using the unfilteredselectLatestThreadMessage.drafts create/updatenow emitinReplyTo,references, andreplyContextSource(caller/carried/ null) so a caller can verify without a raw-header fetch. When lineage was carried rather than caller-specified, a warning goes to stderr.--clear-reply-contextstrips reply headers in place, so repairing a mis-threaded draft no longer requires delete-and-recreate (which loses the draft id).Compatibility
threadIdbehaviour is unchanged — a plain update keeps the draft in its thread, and a caller-supplied--thread-idstill overrides it. The JSON result gains three fields; existing fields are untouched. No existing test needed changing, which is worth noting: nothing in the suite asserted the old self-referencing behaviour.The
replyContextSourcefield reportsnullrather than a"none"string when a draft has no reply context, so all three threading fields are null together.Testing
make cigreen. New regression coverage ininternal/cmd/gmail_drafts_reply_context_test.go:In-Reply-To/References, keepsthreadId, and does not fetch its own thread at allReferencesdoes not drift across repeated updates of a real reply--clear-reply-contextstrips the headers and keeps the thread; it conflicts with an explicit reply target--reply-to-message-idnaming a draft is refused--thread-idwhose newest message is a draft anchors to the newest real message; a drafts-only thread is refusedinReplyTo/references, null when absentReview round 2 (updated after ClawSweeper review)
Dropped the CHANGELOG entry (
c9836084) —AGENTS.mdputs it in the maintainer's landing checklist.Added live Gmail proof for both behaviors flagged as unverified. Detail in this comment; summary:
main(1b261244): a standalone draft with no reply target gainedIn-Reply-To/Referencesafter one update, pointing at the revision that update replaced.threadIdunchanged — thread continuity survives while the self-referential lineage does not.Referencesdoes not accumulate,replyContextSourcereportscallerthencarried.mainkeeps its bad headers on this branch (they're indistinguishable from genuine context by inspection);--clear-reply-contextremoves them while keepingthreadId.Test drafts were deleted afterward.
make cigreen onc9836084.On the open scope question — whether
--clear-reply-contextand the threading JSON fields should land here or in a follow-up — I've argued the case on #942 and will take whichever split you prefer.