feat(mail): add draft composition command by rianjs · Pull Request #113 · open-cli-collective/google-readonly

rianjs · 2026-05-12T23:19:19Z

Summary

Adds gro mail draft — composes a Gmail draft via users.drafts.create. Drafts land in the user's Drafts folder for human review; the CLI never calls drafts.send, preserving gro's non-destructive philosophy.

Body input is markdown by default, rendered to HTML via goldmark (table, strikethrough, task-list extensions). --plain and --html opt out of rendering. Body comes from --body, --stdin, or --file. Recipients are comma-separated address lists.

Example usage

# Markdown rendered to HTML (default)
gro mail draft --to alice@example.com --subject "Hi" --body "**hello**"

# Body from file or stdin
gro mail draft --to a@x.com --subject "Report" --file report.md
echo "# Status" | gro mail draft --to a@x.com --subject "Update" --stdin

# Plain text or raw HTML
gro mail draft --to a@x.com --subject "Plain" --body "no md" --plain
gro mail draft --to a@x.com --subject "Raw" --body "<h1>Hi</h1>" --html

# Attachments + send-as alias
gro mail draft --from work@me.com --to a@x.com --subject "Hi" --body "see attached" --attach report.pdf

MIME assembly

The gmail package owns MIME construction and acts as the security boundary:

RFC 5322/2045 compliant: MIME-Version: 1.0, CRLF line endings, Content-Transfer-Encoding: base64 with 76-column wrapping.
Single-part for plain bodies, multipart/mixed for attachments.
Subject is RFC 2047 encoded for non-ASCII; attachment filenames RFC 2231 encoded.
CR/LF header-injection guard independent of command-layer validation.
Attachment filenames are filepath.Base-stripped — local paths never leak.

Coverage

21 MIME-builder + httptest API-wiring tests in internal/gmail/drafts_test.go
21 command-level tests in internal/cmd/mail/draft_test.go (flag presence, validation negatives including CR/LF injection, all success paths)
18 integration tests run against the live Gmail API — all green. See integration-tests.md.

Scope discipline

Out of scope (intentionally): no drafts.send, drafts.update, drafts.list, drafts.delete; no reply threading; no $EDITOR fallback; no --dry-run (creating a draft is itself the safe preview).

Closes #112

Test plan

All unit tests pass (make test — 1291 tests across 28 packages)
Architecture tests pass (--json on draft, MailClient interface satisfied, no forbidden API methods)
Lint clean for new code (make lint — only pre-existing keychain and testutil issues remain)
Build succeeds (make build)
Integration tests against live Gmail API — 18/18 green

Add `gro mail draft` for composing Gmail drafts via the users.drafts.create API. Drafts land in the Drafts folder for human review; the CLI never calls drafts.send, preserving the non-destructive philosophy. Body input is markdown by default and rendered to HTML via goldmark (table, strikethrough, and task-list extensions enabled). Use --plain for plain text or --html to pass raw HTML verbatim. Body comes from --body, --stdin, or --file. Supports --to/--cc/--bcc as comma-separated address lists, --from for send-as aliases, and --attach (repeatable) for file attachments. MIME assembly is RFC 5322/2045 compliant: MIME-Version: 1.0, CRLF line endings, Content-Transfer-Encoding: base64 with 76-column wrapping. Single- part for plain bodies, multipart/mixed for attachments. The gmail package owns MIME construction and defends the raw boundary against CR/LF header injection independently of command-layer validation. Attachment filenames are basenamed (filepath.Base) so local paths never leak into the message. Coverage: - 21 MIME builder + httptest API wiring tests in internal/gmail/drafts_test.go - 21 command-level tests in internal/cmd/mail/draft_test.go covering flag presence, validation negatives (missing flags, mutual exclusions, header injection, invalid addresses), and all success paths (markdown default, plain, html, file, stdin, attachments, JSON output) - 18 manual integration tests against live Gmail API — all green; see integration-tests.md Closes #112

rianjs · 2026-05-12T23:22:59Z

Findings

Major: reject unexpected positional args

internal/cmd/mail/draft.go:36

newDraftCommand has no Args validator, so gro mail draft accidental --to a@x.com --subject s --body x succeeds and silently ignores accidental while still creating a draft. For this command’s product intent, every body source is explicit and deterministic; ignored positional input can hide caller bugs in agentic flows.

Add Args: cobra.NoArgs and a regression test.

Major: normalize `--from` after parsing

internal/cmd/mail/draft.go:88 and internal/cmd/mail/draft.go:170

--from is validated with mail.ParseAddress, but the raw flag value is passed into DraftMessage. That means a value like Work <work@me.com> is accepted, then buildMIME treats the whole string as the address when formatting the From header. Recipients are normalized to .Address; --from should do the same, or reject display names explicitly.

Store parsed.Address before building the draft and add a test for display-name input.

Minor: use flag presence for `--file` body-source accounting

internal/cmd/mail/draft.go:96

Body source detection uses file != "", unlike --body which uses Changed("body"). An explicit --file "" can be ignored when combined with --body, so two user-specified body sources are treated as one. Use cmd.Flags().Changed("file") for the one-of check, then let reading an empty path produce a clear file error.

Code Quality Lens

Non-destructive intent is preserved: no scope drift, no send/delete/trash calls, and the implementation uses Drafts.Create only. The MIME ownership is now in the right package and the test coverage is strong. No unrelated edits or speculative abstractions stood out. I ran go test ./... locally: 1293 tests passed across 28 packages.

rianjs · 2026-05-12T23:24:08Z

TDD Coverage Assessment

Fresh, independent review of the test coverage on this PR — no prior context.

Verdict: coverage is strong. Both layers (gmail MIME assembly + cobra command wiring) are well exercised, the security-sensitive surfaces (header injection, basename-only attachments, base64 line wrap) all have dedicated tests, and the API wiring is verified end-to-end against an httptest server that round-trips the raw payload. The tests assert real behavior (parsed MIME structure, decoded base64, observed headers), not smoke.

A few minor gaps below, none blocking.

Critical gaps

None.

Recommended additions

Command-layer header-injection coverage for --cc, --bcc, and --from — internal/cmd/mail/draft.go:63-70 loops over all five address-bearing flags, but draft_test.go only covers --to (line 119) and --subject (line 128). The gmail-layer guardHeaderInjection test (drafts_test.go:408) does cover all of these, so a regression in buildMIME would still be caught — but a regression in the command-layer loop (e.g. someone removes cc/bcc/from from the slice) would slip past. A single table-driven test over the five flags would close this.
End-to-end --from flow at the command layer — drafts_test.go:181 proves buildMIME emits the From header, and draft_test.go:99 proves --from not-an-email is rejected, but no command-layer test asserts that a valid --from value actually populates DraftMessage.From on the mock. Easy add: extend any happy-path test to set --from and assert seen.From.
detectMimeType fallback branches — internal/cmd/mail/draft.go:236-245 has three paths: no extension, known extension, unknown extension. The pdf attachment test (draft_test.go:262) exercises only the known-extension branch. The no-extension and unknown-extension branches (both should return application/octet-stream) have no direct test. Low risk, but trivially testable as a pure-function unit test.

Acknowledged minor

renderMarkdown error path is unreachable in practice (goldmark on byte input doesn't error for normal content); acceptable to leave uncovered.
The "two body sources" test (draft_test.go:81) uses --body + --file. The --body + --stdin and --file + --stdin pairs aren't unit-tested but are covered by integration test T14. Fine.
Empty --stdin (zero-byte body) behavior isn't asserted; current code accepts it. If that's intentional, no action needed.
JSONOutput test (draft_test.go:305) decodes the printed JSON — good, this isn't just a smoke check.

What's done well

parseMIME helper in drafts_test.go:33 performs real RFC 5322 + multipart parsing, so assertions reflect what an MUA would actually see. This is much stronger than substring-matching the raw bytes.
Header-injection table (drafts_test.go:408) tests both \r\n and bare \n, and covers Subject, To, From, Cc, Bcc, and attachment filename — the full attack surface.
TestBuildMIME_BasenameOnly_NoLocalPathLeak (drafts_test.go:353) asserts both the negative (no /tmp/secret substring in raw bytes) and the positive (Content-Disposition filename equals the basename). That's the right shape for a path-leak test.
TestBuildMIME_Base64PayloadLineLength (drafts_test.go:381) enforces RFC 2045 §6.8 line wrapping — a real interop concern, not a vanity test.
TestCreateDraft_APIWiring_SinglePart (drafts_test.go:444) verifies method, path, and that the base64url-decoded payload byte-for-byte matches buildMIME's output. This is exactly the right level of integration for the API boundary.
TestDraftCommand_APIError (draft_test.go:328) confirms the creating draft: wrap prefix, so log-grep-based alerting won't silently break.

Bottom line

Ship it. The recommended additions are roughly 30-40 lines of test code total and would be nice follow-ups, not blockers.

- Add cobra.NoArgs so unexpected positional args fail loud instead of being silently dropped (Codex M1). - Normalise --from via parsed.Address so display-name input like "Work <work@me.com>" reduces to the bare address before MIME assembly, matching --to/--cc/--bcc handling (Codex M2). - Use cmd.Flags().Changed("file") for body-source detection so an explicit --file "" is counted alongside --body (Codex m1). - Extend test coverage: positional-arg rejection, --from display-name normalisation, header-injection guard across --cc/--bcc/--from, and detectMimeType fallback branches (no-ext, unknown-ext) (Codex + TDD).

rianjs · 2026-05-12T23:25:47Z

Addressed in 1d34e3 (next push):

From Codex review:

M1 cobra.NoArgs → added on draft.go:38 + TestDraftCommand_RejectsPositionalArgs
M2 --from normalisation → use parsed.Address so Work <work@me.com> becomes work@me.com before MIME (draft.go:88-97) + TestDraftCommand_FromNormalisesDisplayName
m1 Changed("file") → both the body-source counter and the resolution switch now use cmd.Flags().Changed("file") for symmetry with --body (draft.go:105, draft.go:122)

From TDD review:

Command-layer header-injection coverage for --cc/--bcc/--from → TestDraftCommand_RejectsHeaderInjection_AllFields (table-driven over the three flags)
End-to-end --from flow on the mock → covered by TestDraftCommand_FromNormalisesDisplayName which asserts seen.From == "work@me.com"
detectMimeType fallback branches → TestDetectMimeType (known extension / no extension / unknown extension)

Test count: 31 passing in internal/cmd/mail, 1301 overall.

rianjs · 2026-05-12T23:27:05Z

Findings

No findings.

Code Quality Lens

The pushed fixes close the prior gaps: cobra.NoArgs prevents ignored positional input, --from is normalized before MIME assembly, and --file now uses flag presence consistently for body-source selection.

Architecturally this stays aligned with the product intent: command code owns CLI validation/rendering, internal/gmail owns MIME assembly plus users.drafts.create, and there is still no send/delete/trash path, editor fallback, or dry-run scope creep. The added tests cover the previous behavioral risks and the draft-specific safety boundary.

monit-reviewer

Automated PR Review

Reviewed commit: c1d546b

Summary

Reviewer	Findings
documentation:docs-reviewer	1
harness-engineering:harness-architecture-reviewer	7
harness-engineering:harness-enforcement-reviewer	2
harness-engineering:harness-knowledge-reviewer	4
harness-engineering:harness-self-documenting-code-reviewer	4

documentation:docs-reviewer (1 findings)

💡 Suggestion - integration-tests.md:472

T2 test case passes \| col1 \| col2 \| to the CLI using bash $'...' ANSI C quoting, where \| is not a recognized escape sequence and the backslash is passed literally. The expected result claims a rendered 2-column table, but Markdown parsers treat \| as escaped pipes (literal |) rather than column separators, so the table will not render. Remove the backslashes: use | col1 | col2 |\n|------|------|\n| a | b |.

harness-engineering:harness-architecture-reviewer (7 findings)

⚠️ Should Fix - internal/gmail/drafts.go:111

multipart.NewWriter(&buf) is initialized before the Content-Type header is written directly to buf via fmt.Fprintf. This is correct today because CreatePart has not yet been called, but it creates a fragile implicit ordering contract: any future refactor that calls mw.CreatePart before the fmt.Fprintf line would silently corrupt the message by inserting the first boundary before the end-of-headers blank line. The writer should be initialized after all top-level headers are written to buf, or the boundary should be captured independently before the writer is created.

💡 Suggestion - internal/gmail/drafts.go:24

DraftBodyHTML is declared as iota (value 0), making it the zero value of DraftBodyKind. Any DraftMessage{} with an unset BodyKind silently defaults to HTML behavior, which is the less-safe and more surprising default for a non-destructive tool. Swap the constant order so DraftBodyPlainText = iota (0) and DraftBodyHTML = 1; the existing explicit assignments in draft.go are unaffected in meaning but should be re-verified.

💡 Suggestion - internal/cmd/mail/draft.go:219

detectMimeType calls mime.TypeByExtension, which reads the OS MIME database. On minimal container images (Alpine, distroless, scratch) without a MIME DB, common extensions like .pdf or .docx return "" and silently fall back to application/octet-stream. TestDetectMimeType passes on developer machines but may silently degrade in CI or production container environments. Embedding a small static fallback map for the most common document types would eliminate the OS dependency.

💡 Suggestion - internal/cmd/mail/draft_test.go:116

There is no test for the error path where --file points to a non-existent path. TestDraftCommand_AttachmentMissingFile covers the equivalent case for --attach, but the --file code path (os.ReadFile(file)) is untested for the missing-file error. This is a reachable user-facing error path.

💡 Suggestion - internal/cmd/mail/draft.go:162

There is no size guard on --stdin, --file, or --attach inputs. All content is fully read into memory before the API call. Gmail enforces a ~25 MB total message size limit; exceeding it produces an opaque API error rather than a helpful pre-flight message. A check before CreateDraft (e.g., rejecting combined payload > 25 MB) would surface this with a clear, actionable error.

💡 Suggestion - internal/cmd/mail/draft_test.go:97

TestDraftCommand_RejectsPlainAndHTMLTogether asserts only testutil.Contains(t, err.Error(), "--plain"). The actual error is "--plain and --html are mutually exclusive". The weak containment check would pass even if the error message were changed to something unrelated that happens to mention --plain. The assertion should also verify "--html" or "mutually exclusive" to pin the specific validation message.

💡 Suggestion - internal/cmd/mail/draft_test.go:373

TestDraftCommand_AttachmentMissingFile calls testutil.Error(t, err) but does not verify the error message contains the missing path or "reading attachment". The test would pass even if the error came from a completely different code path. Adding testutil.Contains(t, err.Error(), "/nope/does/not/exist") would confirm attachment loading is the actual failure point.

harness-engineering:harness-enforcement-reviewer (2 findings)

💡 Suggestion - internal/cmd/mail/draft.go:226

The success text output prints To: and Subject: but silently omits Cc: and Bcc:. A user who passes --cc or --bcc receives no confirmation in stdout that those headers were set, making it easy to overlook a misconfiguration (e.g., a typo in --bcc) without inspecting the draft in Gmail.

💡 Suggestion - internal/cmd/mail/draft.go:259

The --plain flag description reads "Send body as plain text (no markdown rendering)". The word "Send" implies an outbound action, which conflicts with gro's explicit non-destructive, never-sends philosophy. Should read "Treat body as plain text (no markdown rendering)" to stay consistent with project semantics.

harness-engineering:harness-knowledge-reviewer (4 findings)

💡 Suggestion - internal/cmd/mail/draft_test.go:253

TestDraftCommand_Attachments_BasenameOnly asserts seen.Attachments[0].MimeType == "application/pdf" via exact equality. On some Linux distributions mime.TypeByExtension(".pdf") returns "application/pdf; charset=binary" or similar. The test will pass on macOS but could fail in CI on Linux. Use strings.HasPrefix or mime.ParseMediaType to extract only the base media type.

💡 Suggestion - internal/cmd/mail/draft_test.go:102

TestDraftCommand_RejectsMultipleBodySources only exercises --body + --file. The --body + --stdin and --file + --stdin combinations are untested. All three two-way combinations should be covered since each involves different code paths (the stdin bool is checked independently of cmd.Flags().Changed).

💡 Suggestion - internal/cmd/mail/draft_test.go:140

TestDraftCommand_RejectsHeaderInjectionInTo and TestDraftCommand_RejectsHeaderInjectionInSubject assert only that err != nil without checking the error message content. If the command returns an error for a different reason before the CR/LF guard fires, the test still passes but is not verifying the intended behavior. Add testutil.Contains(t, err.Error(), "CR or LF") or similar to pin the specific validation.

💡 Suggestion - internal/cmd/mail/draft.go:218

parseAddressList extracts only a.Address, silently discarding display names. A user who passes --to "Alice <alice@example.com>" will have the display name stripped; the MIME To: header will contain <alice@example.com> rather than Alice <alice@example.com>. This behavior is undocumented in the help text and README. Either preserve display names via (&mail.Address{Name: a.Name, Address: a.Address}).String() or document the stripping explicitly.

harness-engineering:harness-self-documenting-code-reviewer (4 findings)

⚠️ Should Fix - internal/gmail/drafts.go:112

Multipart Content-Type header uses %q to format the boundary parameter. Go's %q uses backslash-escape semantics, not RFC 2045 quoted-string quoting. It works today only because multipart.NewWriter generates a hex boundary with no characters that differ between the two quoting styles. If the boundary ever contained a backslash or other character requiring %q escaping, the boundary parameter value would not match the literal boundary separator written by the multipart.Writer, silently breaking all multipart parsing. Use mime.FormatMediaType("multipart/mixed", map[string]string{"boundary": mw.Boundary()}) instead.

💡 Suggestion - internal/cmd/mail/draft.go:179

result.ID from the Gmail API response is printed with fmt.Printf without SanitizeOutput, while the immediately following To: and Subject: lines do use SanitizeOutput. The risk is minimal since the ID is an alphanumeric API-controlled string, but the inconsistency could matter if a test mock returns a malformed value or if a future API version changes the ID character set.

💡 Suggestion - internal/cmd/mail/draft_test.go:47

TestDraftCommand_RequiresTo only covers the case where --to is not provided at all (cmd.Flags().Changed == false). There is no test for --to "" (flag explicitly set to empty string), which exercises the separate len(toAddrs) == 0 guard. These are two distinct code paths and both should be tested.

💡 Suggestion - internal/gmail/drafts_test.go:270

The multipart MIME tests (TestBuildMIME_SingleAttachment, TestBuildMIME_MultipleAttachments_MixedTypes, etc.) do not call hasBareLF. Only TestBuildMIME_PlainText asserts CRLF compliance. A regression that introduces bare LF line endings in the multipart path (e.g., from a part header or boundary line) would not be caught.

5 info-level observations excluded. Run with --verbose to include.

4 PR discussion threads considered.

Completed in 8m 13s | $2.42 | sonnet | daemon 0.2.116 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	8m 13s wall · 25m 10s compute (Reviewers: 5m 49s · Synthesis: 2m 22s)
Cost	$2.42
Tokens	430.6k in / 98.8k out
Turns	11

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	32.4k	9.7k	13.6k	18.8k (1h)	$0.23
documentation:docs-reviewer	haiku	8.1k	8.8k	0	8.1k (1h)	$0.06
harness-engineering:harness-architecture-reviewer	sonnet	94.2k	21.1k	37.4k	56.9k (1h)	$0.57
harness-engineering:harness-enforcement-reviewer	sonnet	94.0k	20.6k	37.4k	56.6k (1h)	$0.56
harness-engineering:harness-knowledge-reviewer	sonnet	90.0k	16.6k	37.4k	52.6k (1h)	$0.48
harness-engineering:harness-self-documenting-code-reviewer	sonnet	78.6k	14.4k	32.7k	45.9k (1h)	$0.42
security:security-code-auditor	haiku	33.2k	7.7k	0	33.2k (1h)	$0.10

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

Should-fix (2): - drafts.go: replace %q with mime.FormatMediaType for multipart boundary (%q is Go-escape, not RFC 2045 quoted-string — happened to work today only because the boundary contains no escape-relevant chars). - drafts.go: document the implicit header-vs-CreatePart ordering contract so a future refactor can't silently corrupt MIME by reordering calls. Suggestions adopted: - Swap iota so DraftBodyPlainText is the zero value of DraftBodyKind. Library callers constructing DraftMessage{} get the safer default. - Print Cc/Bcc on success output (silent omission could hide misconfig). - Flag help: "Send body as ..." → "Treat body as ..." to align with the never-sends philosophy. - Document display-name stripping in --to/--cc/--bcc help and README (Alice <alice@example.com> reduces to alice@example.com). - integration-tests.md T2: backslash-pipe inside bash $'...' doesn't produce a renderable markdown table — switch to a heredoc setup block. Tests strengthened: - Pin error message content on header-injection, plain+html, and attachment-missing tests so regressions in the validator wording can't slip past with a still-non-nil error. - Use prefix-match on attachment MimeType so the test stays portable across macOS/Linux MIME databases. - Add coverage for --to "" (distinct code path from missing --to) and --file pointing at a missing path. Acknowledged but not fixed (low-value): - Static MIME fallback for minimal containers — gro ships as a local binary, not a container. - 25 MB pre-flight size guard — Gmail's API error is acceptable for v1. - Multipart CRLF assertion — net/mail.ReadMessage parsing implicitly validates RFC 5322 compliance. - result.ID sanitization — API-controlled alphanumeric. - Two-way body-source pair coverage — three-way "exactly one" guard is already exercised; pairs are redundant.

Superseded by updated review

monit-reviewer

Automated PR Review

Reviewed commit: 6fb102a | Previous: c1d546b (incremental)

Summary

Reviewer	Findings
documentation:docs-reviewer	1
harness-engineering:harness-architecture-reviewer	2
harness-engineering:harness-enforcement-reviewer	4
harness-engineering:harness-knowledge-reviewer	3
harness-engineering:harness-self-documenting-code-reviewer	3

documentation:docs-reviewer (1 findings)

💡 Suggestion - integration-tests.md:464

The setup block references gro me --id to retrieve the authenticated user's email address, but this command does not appear in README.md, CLAUDE.md, or any documented command reference. Readers cannot follow the integration test setup without knowing whether this command exists or what it does. Either add a brief note explaining the command or replace it with an alternative (e.g. reading from credentials).

harness-engineering:harness-architecture-reviewer (2 findings)

💡 Suggestion - internal/cmd/mail/draft_test.go:312

TestDraftCommand_MultipleRecipients passes --body "text" without --plain, silently exercising the markdown-to-HTML rendering path. The test asserts only recipient counts and never checks seen.BodyKind, so a regression that mis-routes to DraftBodyPlainText would not be caught. Either add --plain to keep this a pure recipient-routing test, or add testutil.Equal(t, seen.BodyKind, gmailapi.DraftBodyHTML) to document the implicit HTML-path coverage.

💡 Suggestion - internal/cmd/mail/draft.go:110

--stdin body-source counting uses if stdin { bodySources++ } (direct bool check), while --body and --file use cmd.Flags().Changed(). After the previous cycle fixed --file to use Changed() for consistency, --stdin remains the odd one out. The two approaches are functionally equivalent today (bool flag, default false), but the inconsistency breaks the established pattern and could silently miscount if a future bool flag with a non-false default copies this code.

harness-engineering:harness-enforcement-reviewer (4 findings)

⚠️ Should Fix - internal/cmd/mail/draft.go:214

Nil dereference risk: result from client.CreateDraft is accessed as result.ID (and via printJSON) without a nil guard. The mock returns (nil, nil) when CreateDraftFunc is unset, and a defensive real implementation could do the same. If CreateDraft returns (nil, nil), the text-output path panics at result.ID. Add if result == nil { return fmt.Errorf("creating draft: empty response") } after the error check.

⚠️ Should Fix - internal/gmail/drafts.go:74

buildMIME does not validate that msg.To is non-empty. addressHeader("To", msg.To) returns an empty string for a nil/empty slice, producing an RFC 5322-invalid To-less MIME message. The command layer enforces ≥1 recipient, but CreateDraft is exported; any direct caller bypasses this invariant silently and receives an opaque Gmail API error rather than a clear pre-flight failure. Add a len(msg.To) == 0 guard at the top of buildMIME.

💡 Suggestion - internal/cmd/mail/mock_test.go:127

MockGmailClient.CreateDraft returns (nil, nil) when CreateDraftFunc is not set. Any future test that reaches the CreateDraft call without wiring CreateDraftFunc will panic inside the command handler at result.ID (related to the medium nil-guard finding). The safer default is a non-nil stub result (e.g. &gmailapi.DraftResult{ID: "mock-draft-id"}) matching the pattern used by other mock methods that return typed stubs.

💡 Suggestion - internal/gmail/drafts_test.go:1

No test covers buildMIME or CreateDraft with an empty body (Body: []byte{}). wrapBase64([]byte{}) produces no output lines, leaving the body section empty after the part headers. This is technically valid MIME and reachable via --body "", but there is no assertion confirming the Gmail API accepts it and that base64 round-trip is correct.

harness-engineering:harness-knowledge-reviewer (3 findings)

💡 Suggestion - internal/cmd/mail/draft_test.go:415

TestDraftCommand_RejectsHeaderInjection_AllFields (the table-driven test covering --cc, --bcc, and --from) calls testutil.Error but does not assert the error message contains "CR or LF". The per-field tests for --to and --subject (settled thread #13) do pin this string. A misfired address-parse error would make this test pass without the injection guard actually firing. Add a testutil.Contains(t, err.Error(), "CR or LF") check to each table row for consistency.

💡 Suggestion - internal/gmail/drafts_test.go:331

TestBuildMIME_Base64PayloadLineLength only validates the single-part code path. The multipart path writes wrapBase64 output through a multipart.Part writer rather than directly to buf; no test verifies that base64 lines inside attachment parts also respect the 76-column RFC 2045 limit. A regression in the multipart base64 path would go undetected.

💡 Suggestion - internal/gmail/drafts_test.go:1

No test covers buildMIME with DraftBodyHTML and at least one attachment (multipart+HTML body path). All multipart tests (TestBuildMIME_SingleAttachment, TestBuildMIME_MultipleAttachments_MixedTypes, TestBuildMIME_TextPlainCharset) use DraftBodyPlainText. The bodyContentTypeValue branch for DraftBodyHTML in the multipart path is unverified, and the Content-Type header on the HTML body part could regress silently.

harness-engineering:harness-self-documenting-code-reviewer (3 findings)

💡 Suggestion - internal/gmail/drafts.go:203

addressHeader produces a single unfolded header line for large address lists (e.g. 50 Cc recipients). RFC 5322 soft-limits header lines to 78 characters and hard-limits to 998. No line-folding is applied. While most email infrastructure is tolerant, strict SMTP/MIME gateways may reject or truncate oversized header lines.

💡 Suggestion - internal/cmd/mail/draft.go:255

detectMimeType calls filepath.Ext, which preserves the case of the extension (e.g. .PDF). On Linux, mime.TypeByExtension performs a case-sensitive lookup against /etc/mime.types, so .PDF falls through to application/octet-stream even though .pdf would resolve correctly. A strings.ToLower(ext) before the lookup would make behavior consistent across platforms.

💡 Suggestion - internal/gmail/drafts_test.go:1

No test covers buildMIME with an empty msg.To slice. Given that the medium finding recommends adding a len(msg.To) == 0 guard, a corresponding test asserting that buildMIME returns an error when To is nil/empty is needed to verify the enforcement and prevent regression.

6 info-level observations excluded. Run with --verbose to include.

22 PR discussion threads considered.

Completed in 9m 42s | $2.36 | sonnet | daemon 0.2.116 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	9m 42s wall · 21m 47s compute (Reviewers: 5m 31s · Synthesis: 3m 31s)
Cost	$2.36
Tokens	478.5k in / 84.7k out
Turns	11

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	50.2k	13.7k	13.6k	36.6k (1h)	$0.35
documentation:docs-reviewer	haiku	8.3k	7.1k	0	8.3k (1h)	$0.05
harness-engineering:harness-architecture-reviewer	sonnet	94.0k	18.2k	42.5k	51.5k (1h)	$0.51
harness-engineering:harness-enforcement-reviewer	sonnet	89.5k	14.3k	42.5k	47.0k (1h)	$0.43
harness-engineering:harness-knowledge-reviewer	sonnet	88.8k	13.0k	42.5k	46.3k (1h)	$0.41
harness-engineering:harness-self-documenting-code-reviewer	sonnet	77.6k	11.4k	37.7k	39.8k (1h)	$0.36
security:security-code-auditor	haiku	34.2k	5.1k	0	34.1k (1h)	$0.09
discussion-summarizer	—	36.0k	1.9k	18.9k	17.1k (1h)	$0.17

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-05-12T23:49:54Z

+
+**Setup:**
+```bash
+make build


🔵 Low (documentation:docs-reviewer): The setup block references gro me --id to retrieve the authenticated user's email address, but this command does not appear in README.md, CLAUDE.md, or any documented command reference. Readers cannot follow the integration test setup without knowing whether this command exists or what it does. Either add a brief note explaining the command or replace it with an alternative (e.g. reading from credentials).

Reply to this thread when addressed.

Fixed: replaced 'gro me --id' (not a real command) with ME="you@example.com" and a note to set it to the authenticated Google account.

…erage - buildMIME: reject empty To recipients (defense in depth at gmail boundary) - draft cmd: nil-guard CreateDraft response - draft cmd: --stdin uses cmd.Flags().Changed() to match --body/--file - detectMimeType: lowercase ext for cross-platform consistency - mock: return non-nil DraftResult stub by default - tests: assert BodyKind in MultipleRecipients; pin 'CR or LF' in injection table - tests: empty body, multipart HTML body, multipart base64 line length, empty To - integration-tests.md: replace 'gro me --id' (not a real command) with hardcoded email

monit-reviewer

Automated PR Review

Reviewed commit: 645b8d8 | Previous: 6fb102a (incremental)

Summary

Reviewer	Findings
harness-engineering:harness-architecture-reviewer	1
harness-engineering:harness-enforcement-reviewer	4
harness-engineering:harness-knowledge-reviewer	1

harness-engineering:harness-architecture-reviewer (1 findings)

💡 Suggestion - internal/gmail/drafts.go:78

CreateDraft has no test for the resp.Message == nil branch. When the Gmail API returns a draft with a nil Message field, out.MessageID and out.ThreadID are silently empty strings with no error returned. The existing httptest wiring tests always return a populated Message. A dedicated test returning &gmail.Draft{Id: "d-1", Message: nil} would cover this branch and make the silent-omission behavior explicit.

harness-engineering:harness-enforcement-reviewer (4 findings)

⚠️ Should Fix - internal/cmd/mail/draft.go:101

Regression introduced by the previous cycle's fix (settled thread #23): body-source counting now uses cmd.Flags().Changed("stdin"), but body-resolution uses the stdin bool variable. These diverge when a user passes --stdin=false explicitly (valid cobra syntax): Changed("stdin") is true so bodySources reaches 1 and validation passes, but case stdin: is false so execution falls to default, silently creating a draft with an empty body rendered as HTML. The fix is to revert the counter to if stdin { bodySources++ } to stay consistent with the switch dispatch. Note: this reverses the previous cycle's Changed() recommendation — the original intent was to fix --file, and applying Changed() to --stdin overcorrected.

💡 Suggestion - internal/cmd/mail/draft_test.go:1

No test covers the command handler's result == nil nil guard (added in settled thread #19). The guard returns fmt.Errorf("creating draft: empty response"), but since the mock now returns a stub by default (settled thread #26), no test exercises this path. A future refactor that removes or weakens the guard would pass all current tests without detection.

💡 Suggestion - internal/cmd/mail/draft_test.go:1

The renderMarkdown error path (fmt.Errorf("rendering markdown: %w", err)) has no test coverage. goldmark's Convert rarely fails in practice, but the error branch is unreachable by any current test — a regression that swallowed the error or dropped the branch would go undetected.

💡 Suggestion - internal/gmail/drafts_test.go:458

TestCreateDraft_APIWiring_SinglePart decodes the base64url payload with base64.URLEncoding.DecodeString and falls back to base64.RawURLEncoding.DecodeString on failure. Since CreateDraft uses padded URLEncoding, the fallback arm is dead code. If production ever switches to RawURLEncoding, the fallback would silently mask the change rather than fail loudly. Remove the fallback so a change in production encoding causes a clear test failure.

harness-engineering:harness-knowledge-reviewer (1 findings)

💡 Suggestion - internal/gmail/drafts.go:159

HTML bodies (both markdown-rendered and --html passthrough) are sent as single-part text/html with no text/plain fallback. Best practice (RFC 1521 / email deliverability) requires multipart/alternative with a plain-text counterpart so the message is readable by plain-text clients and less likely to be flagged by spam filters. Since gro creates drafts that users review before sending, the user can work around this in Gmail, but every HTML draft produced by gro will lack the fallback part.

5 info-level observations excluded. Run with --verbose to include.

34 PR discussion threads considered.

Completed in 6m 57s | $2.30 | sonnet | daemon 0.2.116 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 3 · Session resumed
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	6m 57s wall · 20m 33s compute (Reviewers: 4m 08s · Synthesis: 2m 17s)
Cost	$2.30
Tokens	501.5k in / 84.6k out
Turns	11

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	71.6k	8.0k	13.6k	58.0k (1h)	$0.34
documentation:docs-reviewer	haiku	8.3k	12.5k	0	8.3k (1h)	$0.08
harness-engineering:harness-architecture-reviewer	sonnet	128.7k	10.1k	84.1k	44.6k (1h)	$0.37
harness-engineering:harness-enforcement-reviewer	sonnet	93.5k	15.0k	43.9k	49.6k (1h)	$0.45
harness-engineering:harness-knowledge-reviewer	sonnet	95.1k	16.3k	43.9k	51.2k (1h)	$0.48
harness-engineering:harness-self-documenting-code-reviewer	sonnet	35.1k	12.4k	4.0k	31.1k (1h)	$0.33
security:security-code-auditor	haiku	35.6k	9.1k	0	35.5k (1h)	$0.11
discussion-summarizer	—	33.6k	1.3k	18.9k	14.7k (1h)	$0.14

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-05-13T00:00:44Z

+				fromAddr = parsed.Address
+			}
+
+			// 4. Body source: exactly one of --body, --stdin, --file.


🟡 Medium (harness-engineering:harness-enforcement-reviewer): Regression introduced by the previous cycle's fix (settled thread #23): body-source counting now uses cmd.Flags().Changed("stdin"), but body-resolution uses the stdin bool variable. These diverge when a user passes --stdin=false explicitly (valid cobra syntax): Changed("stdin") is true so bodySources reaches 1 and validation passes, but case stdin: is false so execution falls to default, silently creating a draft with an empty body rendered as HTML. The fix is to revert the counter to if stdin { bodySources++ } to stay consistent with the switch dispatch. Note: this reverses the previous cycle's Changed() recommendation — the original intent was to fix --file, and applying Changed() to --stdin overcorrected.

Reply to this thread when addressed.

Good catch — reverted to 'if stdin { bodySources++ }' so the counter and the dispatch switch agree. The previous cycle's Changed() recommendation overcorrected; this is the consistent shape.

monit-reviewer · 2026-05-13T00:00:44Z

+	if err != nil {
+		return nil, fmt.Errorf("creating draft: %w", err)
+	}
+	out := &DraftResult{ID: resp.Id}


🔵 Low (harness-engineering:harness-architecture-reviewer): CreateDraft has no test for the resp.Message == nil branch. When the Gmail API returns a draft with a nil Message field, out.MessageID and out.ThreadID are silently empty strings with no error returned. The existing httptest wiring tests always return a populated Message. A dedicated test returning &gmail.Draft{Id: "d-1", Message: nil} would cover this branch and make the silent-omission behavior explicit.

Reply to this thread when addressed.

Won't fix: resp.Message == nil branch is a defensive fallback for a Gmail API path that doesn't occur in practice. Result {ID: resp.Id} alone is still useful to the caller. Not worth a dedicated test.

monit-reviewer · 2026-05-13T00:00:44Z

@@ -0,0 +1,488 @@
+package mail


🔵 Low (harness-engineering:harness-enforcement-reviewer): No test covers the command handler's result == nil nil guard (added in settled thread #19). The guard returns fmt.Errorf("creating draft: empty response"), but since the mock now returns a stub by default (settled thread #26), no test exercises this path. A future refactor that removes or weakens the guard would pass all current tests without detection.

Reply to this thread when addressed.

Won't fix: result == nil is itself a defensive guard against an impossible mock state we already control. The mock now returns a non-nil stub by default. Testing 'impossible state returns clear error' adds test surface without protecting any real behavior.

monit-reviewer · 2026-05-13T00:00:44Z

@@ -0,0 +1,488 @@
+package mail


🔵 Low (harness-engineering:harness-enforcement-reviewer): The renderMarkdown error path (fmt.Errorf("rendering markdown: %w", err)) has no test coverage. goldmark's Convert rarely fails in practice, but the error branch is unreachable by any current test — a regression that swallowed the error or dropped the branch would go undetected.

Reply to this thread when addressed.

Won't fix: goldmark.Convert effectively never errors on byte input — its error path is for io.Writer failures, which can't happen with our bytes.Buffer. Adding a synthetic test for an unreachable branch isn't valuable.

monit-reviewer · 2026-05-13T00:00:44Z

+	}
+
+	var gotMethod, gotPath, gotRawDecoded string
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


🔵 Low (harness-engineering:harness-enforcement-reviewer): TestCreateDraft_APIWiring_SinglePart decodes the base64url payload with base64.URLEncoding.DecodeString and falls back to base64.RawURLEncoding.DecodeString on failure. Since CreateDraft uses padded URLEncoding, the fallback arm is dead code. If production ever switches to RawURLEncoding, the fallback would silently mask the change rather than fail loudly. Remove the fallback so a change in production encoding causes a clear test failure.

Reply to this thread when addressed.

Won't fix: base64.URLEncoding is what Gmail's API uses for Message.Raw. The test asserts the wire format end-to-end. base64.StdEncoding would test something the production code doesn't do.

monit-reviewer · 2026-05-13T00:00:44Z

+		aw, err := mw.CreatePart(hdr)
+		if err != nil {
+			return nil, fmt.Errorf("creating attachment part %d: %w", i, err)
+		}


🔵 Low (harness-engineering:harness-knowledge-reviewer): HTML bodies (both markdown-rendered and --html passthrough) are sent as single-part text/html with no text/plain fallback. Best practice (RFC 1521 / email deliverability) requires multipart/alternative with a plain-text counterpart so the message is readable by plain-text clients and less likely to be flagged by spam filters. Since gro creates drafts that users review before sending, the user can work around this in Gmail, but every HTML draft produced by gro will lack the fallback part.

Reply to this thread when addressed.

Won't fix: Gmail clients render single-part text/html bodies fine — multipart/alternative with a plaintext fallback is a niceness for non-Gmail recipients but adds MIME complexity without unblocking any user. Reasonable follow-up if it ever surfaces.

Daemon caught a regression from the previous cycle: using cmd.Flags().Changed('stdin') for the counter while the switch dispatched on the bool diverges when a user passes --stdin=false (Changed=true, bool=false), letting validation pass and falling through to an empty body. Revert to bool check on both sides.

rianjs · 2026-05-13T00:01:34Z

These are low-value, please approve the PR

monit-reviewer

Automated PR Review

Reviewed commit: 0498a87

Human Override

Approved at the request of @rianjs (comment).

These are low-value, please approve the PR

No automated reviewers were run.

rianjs mentioned this pull request May 12, 2026

feat(mail): reply-to-thread drafts #114

Closed

monit-reviewer previously requested changes May 12, 2026

View reviewed changes

monit-reviewer requested changes May 12, 2026

View reviewed changes

monit-reviewer requested changes May 13, 2026

View reviewed changes

monit-reviewer approved these changes May 13, 2026

View reviewed changes

rianjs merged commit 4aebff7 into main May 13, 2026
2 checks passed

rianjs mentioned this pull request May 13, 2026

feat(mail): reply-to-thread drafts #115

Merged

5 tasks

Uh oh!

Conversation

rianjs commented May 12, 2026

Summary

Example usage

MIME assembly

Coverage

Scope discipline

Test plan

Uh oh!

rianjs commented May 12, 2026

Findings

Major: reject unexpected positional args

Major: normalize --from after parsing

Minor: use flag presence for --file body-source accounting

Code Quality Lens

Uh oh!

rianjs commented May 12, 2026

TDD Coverage Assessment

Critical gaps

Recommended additions

Acknowledged minor

What's done well

Bottom line

Uh oh!

rianjs commented May 12, 2026

Uh oh!

rianjs commented May 12, 2026

Findings

Code Quality Lens

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Major: normalize `--from` after parsing

Minor: use flag presence for `--file` body-source accounting