feat(mail): reply-to-thread drafts - #115
Conversation
Extend `gro mail draft` with --reply-to and --reply-all flags, building on the existing DraftMessage/buildMIME/CreateDraft pipeline rather than forking a new command. - gmail.Message: expose Cc, RFCMessageID, References, In-Reply-To from the source message so the command layer can derive defaults. - gmail.DraftMessage: add ThreadID, InReplyTo, References fields; buildMIME emits the two new headers; guardHeaderInjection covers them; CreateDraft sets Draft.Message.ThreadId. - mail draft command: --reply-to fetches the source via existing GetMessage, derives To/Subject/threading; --reply-all fetches the profile + reads any --from alias to filter self out of the source To/Cc. Local validation runs before any Gmail API call. deriveReplyDefaults is fallible and rejects empty/malformed From or Message-Id headers, so we never silently produce a broken reply. - Re: prefixing is case-insensitive and locale-naive (we don't recognise Aw/Sv/Sp variants — matches Gmail's own behaviour). - Tests: 19 new cases across both layers (derivation, overrides, alias filtering, missing/malformed headers, local-before-remote validation, injection guard for the new flag and headers). - Integration: T21-T26 verify wire-format headers via gro mail read --json rather than UI inspection. Closes #114
|
Major
Minor
Code Quality Lens The PR preserves the intended architecture: no new command, no |
… local guards - deriveReplyDefaults now takes a replyNeeds struct and only parses source headers for slots the caller actually consumes. An explicit --to override no longer fails on a malformed source From; an explicit --cc override no longer fails on malformed source To/Cc. - Restore the local 'len(toAddrs) == 0' guard in non-reply mode so empty --to fails before any Gmail API call (regression introduced by reply mode). - Treat --reply-to "" as non-reply mode (don't call GetMessage with empty ID, don't skip --to/--subject requirements). --reply-all --reply-to "" still errors with the existing requires-reply-to message. - New tests pin exact alias-filter expectations and cover the three new edge cases above.
TDD review — #115 (
|
Added 4 tests for the highest-value defensive branches flagged by TDD review: - TestDraftCommand_ReplyAll_GetProfileFails: error from GetProfile aborts before CreateDraft (without this test, a regression that swallows the error could leak the user's address into Cc). - TestDraftCommand_ReplyAll_EmptyProfileFallsBackToFrom: empty profile + --from alias still produces correct self-filtering. - TestDraftCommand_ReplyTo_FirstReplyEmptyReferences: source with no prior References produces a 1-element chain (just the source Message-Id). - TestDraftCommand_ReplyTo_WithAttachment: threading headers survive the multipart path when --attach is also supplied. Other suggestions in the TDD comment (addRePrefix edge cases, empty --subject "" override, srcReply() shared-pointer concern) are low-yield and skipped.
|
Major
Minor None. Nit The PR body is now stale: it still says “19 new tests” even though the updated diff adds more reply tests. Refresh the test-plan counts before merge. Code Quality Lens The lazy |
Address Codex re-review: --reply-to with an empty value (a common agentic-flow footgun when shell variables don't expand) now errors explicitly rather than silently falling back to non-reply mode. In reply mode, an explicit --to "" override is also rejected before any GetMessage call.
|
Blocker None. Major None. Minor None. Nit None. Code Quality Lens The latest revision closes the prior validation gaps: empty Architecturally this stays aligned: one |
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 538891d
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs-reviewer | 1 |
| harness-engineering:harness-architecture-reviewer | 3 |
| harness-engineering:harness-enforcement-reviewer | 1 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
documentation:docs-reviewer (1 findings)
💡 Suggestion - integration-tests.md:593
Test case T25 is malformed: the Command column contains descriptive text instead of an executable bash command. Should be something like
./bin/gro mail draft --reply-to "$SRC_ID_WITH_RE" --body "T25 test" --plain --json.
harness-engineering:harness-architecture-reviewer (3 findings)
💡 Suggestion - internal/cmd/mail/draft.go:248
In --reply-all mode, filterSelf only removes the authenticated user and --from alias from the derived Cc list, but does not deduplicate the derived To recipient from Cc. If the original sender also appears in the source To or Cc fields (common on mailing lists), they will appear in both To and Cc on the draft. Standard reply-all behavior excludes the derived To address from Cc.
💡 Suggestion - internal/cmd/mail/draft.go:416
addRePrefix checks only that the first 3 chars are 're:' (case-insensitive) but the comment states the check should require 're:' followed by a space or end-of-string. A subject like 're:nospace' would be incorrectly treated as already prefixed. The comment is misleading and the implementation diverges from stated intent.
💡 Suggestion - internal/cmd/mail/draft.go:364
buildReferences contains a redundant
if f != ""guard inside a strings.Fields loop. strings.Fields never returns empty tokens; the guard is dead code and misleads readers into thinking empty tokens are possible.
harness-engineering:harness-enforcement-reviewer (1 findings)
internal/cmd/mail/draft.go:308
Flag name
--reply-tocollides with the well-known RFC 5322 Reply-To header concept (which sets a redirect address, not a thread target). A user passing--reply-to email@example.comexpecting to set the Reply-To header will get a confusing API error. Consider--in-reply-toor--reply-to-messageto disambiguate. Since this is a new flag, the renaming cost is low.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - internal/gmail/drafts.go:123
The References header is emitted as a single unfolded line via strings.Join(msg.References, " "). RFC 2822 §2.1.1 limits physical lines to 998 characters. For deeply-threaded messages with many Message-Id tokens, this can produce a spec-violating line that may confuse strict MIME parsers. Header folding should be applied when the value exceeds the limit.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
💡 Suggestion - internal/cmd/mail/draft.go:372
buildReferences unconditionally appends msgID to the chain even when it already appears in rawRefs. A source message with a duplicate References header or where References already ends with the same Message-Id would produce a duplicate entry. RFC 5322 §3.6.4 requires unique message IDs in the chain; a simple dedup check before appending would fix this.
💡 Suggestion - internal/cmd/mail/draft_test.go:531
Missing test for the --reply-all + explicit --to override combination (needs.To=false but needs.Cc=true). This distinct branch in the replyNeeds/selfSet logic is not covered by existing tests. TestDraftCommand_ReplyTo_ExplicitOverrides overrides both --to AND --cc; TestDraftCommand_ReplyAll_AddsCc overrides neither.
4 info-level observations excluded. Run with --verbose to include.
4 PR discussion threads considered.
Completed in 5m 10s | $1.44 | 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 | 5m 10s wall · 14m 41s compute (Reviewers: 4m 07s · Synthesis: 1m 00s) |
| Cost | $1.44 |
| Tokens | 434.2k in / 58.6k out |
| Turns | 15 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 31.3k | 3.0k | 13.6k | 17.7k (1h) | $0.12 |
| documentation:docs-reviewer | haiku | 6.3k | 7.8k | 0 | 6.3k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 87.5k | 7.4k | 62.1k | 25.4k (1h) | $0.24 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 64.7k | 8.3k | 32.8k | 31.9k (1h) | $0.27 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 96.5k | 7.4k | 66.6k | 29.9k (1h) | $0.26 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 120.8k | 14.6k | 85.4k | 35.4k (1h) | $0.39 |
| security:security-code-auditor | haiku | 27.1k | 10.0k | 0 | 27.1k (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.
- buildReferences dedupes the trailing Message-Id when the source's References header already ends with it (some MUAs do this) and drops the redundant empty-token guard inside strings.Fields. - addRePrefix comment clarified to document that 're:foo' (no trailing space) is treated as already-prefixed. - New TestDraftCommand_ReplyAll_ExplicitToOverride pins the needs.To=false + needs.Cc=true combination. - T25 integration row now has an executable command instead of prose.
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8a747f8 | Previous: 538891d (incremental)
Approved with 4 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs-reviewer | 1 |
| harness-engineering:harness-architecture-reviewer | 1 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
documentation:docs-reviewer (1 findings)
💡 Suggestion - integration-tests.md:576
Integration test setup references
.rfcMessageIdJSON field (e.g.jq -r '.rfcMessageId'), and T21-T26 expectations reference.inReplyToand.references. If the actualgro mail read --jsonoutput uses different field names, all threading tests will fail silently during setup rather than with a clear assertion error. Should be verified against the live JSON schema before the manual integration run.
harness-engineering:harness-architecture-reviewer (1 findings)
💡 Suggestion - internal/cmd/mail/draft.go:256
Post-derivation
len(toAddrs) == 0guard can be bypassed by a sourceFromheader thatmail.ParseAddressparses without error but yields an emptyAddressfield (e.g. group syntax likeundisclosed-recipients:;). In that case the guard passes, the draft is created with an empty recipient, and no error is returned. The 'no From header' path is tested, but the empty-parsed-address path is not.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - internal/cmd/mail/draft.go:220
GetProfile is called unconditionally whenever
needs.Ccis true (--reply-all without explicit --cc), even when--fromalone is sufficient to populate selfSet. If the profile API is transiently unavailable, draft creation fails with no workaround — passing--ccexplicitly disables Cc derivation entirely, leaving the user stuck. This is a new failure dependency introduced by this PR.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - internal/cmd/mail/draft.go:114
When GetProfile returns
(nil, nil), the nil guard prevents a panic but selfSet contains only the--fromalias (if provided), so the authenticated account's address is not filtered from derived Cc. TestDraftCommand_ReplyAll_EmptyProfileFallsBackToFrom only covers the empty-EmailAddress case; the nil-profile case is a distinct code path with no test coverage.
4 info-level observations excluded. Run with --verbose to include.
12 PR discussion threads considered.
Completed in 7m 07s | $1.63 | 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 | 7m 07s wall · 16m 47s compute (Reviewers: 3m 52s · Synthesis: 2m 49s) |
| Cost | $1.63 |
| Tokens | 389.3k in / 66.7k out |
| Turns | 12 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 41.6k | 8.8k | 13.6k | 28.0k (1h) | $0.24 |
| documentation:docs-reviewer | haiku | 6.3k | 10.4k | 0 | 6.3k (1h) | $0.06 |
| harness-engineering:harness-architecture-reviewer | sonnet | 67.3k | 13.0k | 33.5k | 33.8k (1h) | $0.35 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 66.8k | 8.9k | 33.5k | 33.3k (1h) | $0.29 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 64.2k | 10.9k | 33.5k | 30.7k (1h) | $0.31 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 83.3k | 2.3k | 59.3k | 24.0k (1h) | $0.16 |
| security:security-code-auditor | haiku | 27.8k | 11.8k | 0 | 27.8k (1h) | $0.11 |
| discussion-summarizer | — | 32.0k | 816 | 18.9k | 13.1k (1h) | $0.11 |
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.
| # anything that resolves to a single recent message. | ||
| SRC_ID=$(./bin/gro mail search "in:inbox" --json | jq -r '.[0].id') | ||
| SRC_RFC=$(./bin/gro mail read "$SRC_ID" --json | jq -r '.rfcMessageId') | ||
| SRC_THREAD=$(./bin/gro mail read "$SRC_ID" --json | jq -r '.threadId') |
There was a problem hiding this comment.
🔵 Low (documentation:docs-reviewer): Integration test setup references .rfcMessageId JSON field (e.g. jq -r '.rfcMessageId'), and T21-T26 expectations reference .inReplyTo and .references. If the actual gro mail read --json output uses different field names, all threading tests will fail silently during setup rather than with a clear assertion error. Should be verified against the live JSON schema before the manual integration run.
Reply to this thread when addressed.
| return fmt.Errorf("--to must contain at least one address") | ||
| } | ||
|
|
||
| result, err := client.CreateDraft(cmd.Context(), gmailapi.DraftMessage{ |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): Post-derivation len(toAddrs) == 0 guard can be bypassed by a source From header that mail.ParseAddress parses without error but yields an empty Address field (e.g. group syntax like undisclosed-recipients:;). In that case the guard passes, the draft is created with an empty recipient, and no error is returned. The 'no From header' path is tested, but the empty-parsed-address path is not.
Reply to this thread when addressed.
| Cc: replyAll && !cmd.Flags().Changed("cc"), | ||
| Subject: !cmd.Flags().Changed("subject"), | ||
| } | ||
| selfSet := map[string]bool{} |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): GetProfile is called unconditionally whenever needs.Cc is true (--reply-all without explicit --cc), even when --from alone is sufficient to populate selfSet. If the profile API is transiently unavailable, draft creation fails with no workaround — passing --cc explicitly disables Cc derivation entirely, leaving the user stuck. This is a new failure dependency introduced by this PR.
Reply to this thread when addressed.
| @@ -98,6 +114,17 @@ Examples: | |||
| fromAddr = parsed.Address | |||
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): When GetProfile returns (nil, nil), the nil guard prevents a panic but selfSet contains only the --from alias (if provided), so the authenticated account's address is not filtered from derived Cc. TestDraftCommand_ReplyAll_EmptyProfileFallsBackToFrom only covers the empty-EmailAddress case; the nil-profile case is a distinct code path with no test coverage.
Reply to this thread when addressed.
Summary
Closes #114.
Extends
gro mail draftwith--reply-to <message-id>and--reply-allflags so it can compose threaded replies. Same command, same MIME pipeline, same client interface — strictly additive on #113.With
--reply-to:Todefaults to the source'sFrom;SubjectbecomesRe: <orig>(case-insensitive, no double prefix).In-Reply-To/Referencesheaders are emitted;Draft.Message.ThreadIdis set so the draft lands inside the source conversation.--to/--cc/--subjectoverride derived values (cmd.Flags().Changed()pattern). Lazy derivation: source headers are only parsed for slots the caller actually consumes.--reply-alladds the source To+Cc as Cc, filtered to remove the authenticated account and any--fromalias.Validation: all local-only checks run before any Gmail API call.
--reply-to ""is rejected explicitly (no silent fallback).deriveReplyDefaultsis fallible — empty/malformedFromorMessage-Idheaders fail beforeCreateDraft. Header-injection guard extended to cover--reply-to,In-Reply-To, andReferences.Reuse, no rebuild
Per the ticket's "reuse don't rebuild" mandate:
gro mail replycommand — two flags on the existingnewDraftCommand.ReplyMessagetype — three fields on the existingDraftMessage.MailClientmethods —GetMessageandGetProfileare already there.Test plan
make test)gro mail read --jsonrather than UI eyeballing