Skip to content

feat(mail): quote source message in reply drafts - #126

Merged
rianjs merged 5 commits into
mainfrom
feat/125-reply-draft-quoting
May 15, 2026
Merged

feat(mail): quote source message in reply drafts#126
rianjs merged 5 commits into
mainfrom
feat/125-reply-draft-quoting

Conversation

@rianjs

@rianjs rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor

Summary

gro mail draft --reply-to produced a blank body — it fetched the source with metadata only, so the prior message was never quoted (it behaved as if you hit Reply in Gmail and deleted everything). This makes reply drafts quote the source like Gmail's web UI, by default.

  • Quote by default; --no-quote opts out (fully blank, byte-identical to today; keeps the cheaper metadata-only fetch).
  • Attribution: fixed en-US On Mon, Jan 2, 2006 at 3:04 PM <From> wrote:, date in the source message's own timezone (never UTC/local). Deliberately not localized — Gmail's collapse is driven by gmail_quote markup, not this text, and the CLI has no reliable recipient locale. Raw-Date fallback on parse failure.
  • Plain replies: > line prefixes (empty line → >). HTML/markdown replies: wrapped in Gmail's gmail_quote div/blockquote so the Gmail UI collapses it behind its “…” affordance. Attribution and source are HTML-escaped (no markup injection via display name / <addr>).
  • Bare reply now valid: gro mail draft --reply-to <id> with no body source = quote-only draft; leading separator omitted when there's no authored text. Non-reply mode still requires exactly one body source.
  • Known limitation (by design): a source with only an HTML part (no plain-text alternative) is shown as escaped tags — avoids an HTML→text dependency. Empirically, Outlook/Exchange senders still expose text/plain, so this is rare.
  • Adds the "Gmail browser parity" design principle to CLAUDE.md — the rule this behavior embodies (folded into the PR that first honors it, by design).

Plan + 3-round Codex architect review converged at blockers=0/majors=0 on #125.

Test plan

  • go vet, go build ./..., golangci-lint (0 issues), gofmt clean
  • New unit tests (internal/cmd/mail/draft_quote_test.go): plain exact-bytes, HTML gmail_quote markup + escaping, markup-injection escape, bare quote-only, --no-quote body+fetch-mode spy, default-fetches-body spy, --no-quote bare blank, --no-quote requires --reply-to, empty-source-body skip, TZ-crossing attribution (offset where UTC would change weekday/day/hour), date-parse-failure fallback
  • Updated 3 existing assertions for the relaxed body-source error message
  • Live integration-tests T27/T29/T30/T31/T32 against a real inbox (run post-review, pre-merge)
  • Note: pre-existing Windows-only failures in TestSafeOutputPath* (untouched attachment path code; POSIX-abs paths aren't absolute on Windows) — unrelated; CI is Linux.

Closes #125

Reply drafts (--reply-to) now quote the source message below the authored
body, matching Gmail's web UI default, instead of producing a blank body.
The source is fetched with the full body only when quoting; --no-quote
keeps the cheaper metadata-only fetch and reproduces the prior blank draft.

- Attribution is a fixed en-US "On <date> <from> wrote:" line with the
  date rendered in the source message's own timezone (never UTC/local);
  deliberately not localized. Falls back to the raw Date on parse failure.
- Plain replies use "> " line prefixes (empty lines become ">"); HTML and
  markdown replies wrap the quote in Gmail's gmail_quote div/blockquote
  markup so the Gmail UI collapses it natively. Attribution and source are
  HTML-escaped to prevent markup injection via display name or address.
- Reply mode now allows zero body sources: a bare --reply-to yields a
  quote-only draft; the leading separator is omitted when there is no
  authored body. Non-reply mode still requires exactly one body source.
- A source with only an HTML part (no plain-text alternative) is shown as
  escaped tags, by design, to avoid an HTML-to-text dependency.

Adds the "Gmail browser parity" design principle to CLAUDE.md, the rule
this behavior embodies.

Closes #125
@rianjs

rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Major

  • internal/cmd/mail/draft.go:486 quotePlain assumes LF-only source bodies. Real email text/plain bodies commonly decode as CRLF; strings.Split(body, "\n") makes a CRLF blank line appear as "\r", so it becomes "> \r" instead of the canonical empty-line quote marker ">". Add a CRLF fixture and classify lines after stripping a terminal \r while preserving original line endings.

Minor

  • internal/cmd/mail/draft_quote_test.go:100 The “bare reply” unit test uses --plain, but the product path in the issue is literal gro mail draft --reply-to <id> which defaults to HTML/markdown. The implementation appears correct, but add an automated default-mode bare reply test asserting no leading newline and body starts with the gmail_quote block.

Confirmed

  • Conditional separator, both-escaped HTML attribution/body, source-timezone attribution, --no-quote metadata fetch, and command-layer-only quote tests all match the converged architecture.
  • CLAUDE.md is intentionally scoped and documented; no further objection.

quotePlain strips a terminal CR per line before classifying, so a CRLF
blank line quotes as ">" (not "> \r") and content lines carry no stray
CR; output is normalized to LF. Real email text/plain parts are CRLF.

Adds a CRLF-source test and a default-mode (markdown/HTML) bare-reply
test asserting no leading newline and a leading gmail_quote block.
@rianjs

rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

TDD coverage assessment

Overall the test suite is thorough and well-structured. All major happy-paths and guard-clauses have tests with real assertions. A few gaps are worth noting.

Major

None.

Minor

M1 — HTML body + quote: ordering and separator not asserted
TestDraftCommand_Reply_QuotesHTMLWithGmailQuoteMarkup (draft_quote_test.go:51) uses only Contains checks. It never asserts that the authored <p>…</p> block appears before the gmail_quote block, nor that exactly one \n separator separates them. A bug that appended the quote before the authored body, or omitted the separator, would pass the test. Compare to the plain-text equivalent (TestDraftCommand_Reply_QuotesPlainByDefault) which uses an exact Equal match.

M2 — CRLF in HTML source body is not tested
quoteHTML (draft.go:514) does html.EscapeString(body) → replace \n with <br>\n but never strips \r. A CRLF source body in HTML mode would emit \r<br> on each content line. TestDraftCommand_Reply_QuotesCRLFSourceBody covers the plain-text path only; there is no HTML-mode equivalent.

M3 — Raw --html flag + quoting path is untested
The quoting branch checks kind == gmailapi.DraftBodyHTML (draft.go:272), which is true for both markdown (the default) and --html (raw HTML pass-through). No test uses --reply-to … --html <raw> to confirm the HTML branch fires for that flag combination.

Nit

N1 — Doc comment / code mismatch in replyAttribution
The function doc (draft.go:472) says "On <date>, <from> wrote:" (comma after date) but the implementation (draft.go:485) emits no comma. No test catches this because tests assert the implementation's actual output — but it means the doc comment is wrong, not the test.

N2 — TestDraftCommand_Reply_BareReplyDefaultModeIsQuoteOnlyHTML relies on goldmark emitting empty bytes for empty input
The bare-reply HTML test (draft_quote_test.go:248) asserts strings.HasPrefix(body, '<div class="gmail_quote">'). This implicitly assumes renderMarkdown([]byte("")) returns zero bytes, not e.g. "\n". That assumption is currently correct but is fragile if the goldmark dependency changes. A unit test for renderMarkdown on empty input, or an explicit if len(bodyBytes) == 0 guard, would make this explicit.

STATUS: tdd_majors=0 tdd_minors=3

quoteHTML also leaked CRLF (a "\r<br>" before each break) — extract a
shared normalizeLF used by both quotePlain and quoteHTML so neither
emits stray CRs. Real text/html and text/plain parts decode as CRLF.

Tests: add HTML-CRLF coverage, a raw --html reply-quoting case, and an
ordering assertion (quote follows the authored body, preceded by the
"\n" separator). Fix the replyAttribution doc comment to match the
comma-free format the code actually emits.
@rianjs

rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

TDD assessment — resolution

All three minors + the nit addressed in 6d7ade7:

  • M1 (HTML body+quote ordering): added an ordering assertion in TestDraftCommand_Reply_QuotesHTMLWithGmailQuoteMarkup — quote follows the authored content and is immediately preceded by the \n separator.
  • M2 (CRLF in HTML mode): this was a real defect, not just a test gap — quoteHTML emitted \r<br> for CRLF sources. Extracted a shared normalizeLF used by both quotePlain and quoteHTML; added TestDraftCommand_Reply_QuotesHTMLCRLFSourceBody asserting no \r.
  • M3 (--html raw path): added TestDraftCommand_Reply_RawHTMLBodyStillQuotes exercising --reply-to … --html --body ….
  • Nit: replyAttribution doc comment corrected to the comma-free form the code emits.

Thanks — M2 caught a genuine correctness bug for real CRLF email bodies.

An HTML-only source message (no text/plain alternative — common for
alert/marketing/SaaS senders) was HTML-escaped into a wall of visible
tags in the quote. Gmail itself nests the original message's HTML inside
the gmail_quote blockquote on reply; mirror that.

- gmail: extractBodyWithKind reports whether Body came from text/html;
  Message gains BodyIsHTML. extractBody kept as a thin wrapper so the
  existing callers/tests are untouched.
- mail: quoteHTML nests an HTML source verbatim and only escapes a
  plain-text source. The attribution line is always escaped regardless
  (it carries the raw From header / display name).

Reverses the earlier "escape everything / documented limitation"
decision: the limitation hit a common real-world sender and produced
exactly the technically-valid-but-foreign output the Gmail-parity
principle rejects.
@rianjs

rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Major

  • internal/cmd/mail/draft.go:522 Raw HTML source nesting is acceptable for Gmail parity/XSS only if the source is contained as an HTML fragment. As written, a source body can include structural markup like </blockquote></div><p>looks like authored text</p> and break out of the gmail_quote wrapper before Gmail sanitizes/render-collapses it. Gmail may sanitize active script/event handlers, but it will not necessarily preserve your quote containment. Parse to a fragment/body-children first, or otherwise neutralize unmatched wrapper-closing tags; add a regression for blockquote/div breakout. The current test that expects a full <!DOCTYPE><html><body>... document verbatim inside a blockquote is also baking in invalid nested-document markup rather than Gmail-like body-fragment quoting.

Minor

  • internal/gmail/messages.go:25 BodyIsHTML is an internal routing bit for reply quoting, but the JSON tag exposes it in gro mail read --json for HTML-only messages. That is a public output surface change not needed for feat(mail): quote source message in reply drafts #125. Prefer json:"-" unless you intentionally want to document this as a new JSON field.
  • internal/gmail/messages_test.go:322 Helper coverage is good, but add one parseMessage(..., includeBody=true) test proving an HTML-only payload sets both Body and BodyIsHTML. The command tests manually set BodyIsHTML, so they do not exercise the real Gmail parse path.

Confirmed

  • Attribution escaping remains correct and should stay escaped.
  • Plain-source HTML replies still escape body + <br>, and --plain continues through quotePlain.
  • --no-quote fetch mode, conditional separator, source-TZ attribution, and layer placement remain aligned with the architecture.

Nesting raw source HTML risked container breakout: a source body with
"</blockquote></div>" could escape the gmail_quote wrapper before Gmail
renders it (Gmail sanitizes active content but does not re-balance our
quote containment). Parse the source with x/net/html and re-serialize
the <body> children, which (1) strips the DOCTYPE/<html>/<head> so we
no longer nest a full document inside a blockquote, and (2) makes
breakout structurally impossible — stray closers are dropped by the
parser since we emit a re-parsed tree, not raw bytes. Parse failure
degrades safely to escaped (plain-source) treatment. Attribution stays
escaped. x/net was already in the module graph (now direct).

Also: BodyIsHTML is json:"-" (internal routing bit, not a public --json
surface), and a parseMessage(includeBody=true) test proves an HTML-only
payload sets both Body and BodyIsHTML. Breakout regression added.
@rianjs

rianjs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Findings
None.

Assessment

  • Containment is now architecturally sound: parsing and re-serializing the source <body> children avoids full-document nesting and prevents raw </blockquote></div> bytes from escaping the gmail_quote wrapper.
  • Security posture is coherent for this CLI: attribution remains escaped, structural containment is owned here, and active HTML content is deliberately left to Gmail’s render-time sanitizer in a human-reviewed draft flow.
  • Plain-source HTML replies and --plain replies remain on the escaped/plain quote paths.
  • Coverage now hits the new branch through BodyIsHTML, parseMessage propagation, HTML-only rendering, breakout regression, CRLF normalization, raw --html, and bare default-mode reply.
  • Code Quality Lens: the new x/net/html dependency is justified and scoped; BodyIsHTML is kept off the public JSON surface; no new speculative abstraction or unrelated behavior change stands out.

@rianjs
rianjs merged commit eac6eec into main May 15, 2026
2 checks passed
@rianjs
rianjs deleted the feat/125-reply-draft-quoting branch May 15, 2026 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(mail): quote source message in reply drafts

1 participant