feat(grw): mail send <draft-id> by rianjs · Pull Request #19 · open-cli-collective/google-cli

rianjs · 2026-09-03T23:28:24Z

Summary

internal/rw/gmail: GetDraft (metadata-only fetch → DraftSummary with From/To/Cc/Bcc/Subject/attachment count) and SendDraft (users.drafts.send). DTOs live next to DraftResult in internal/api/gmail; the API calls stay in the write layer.
grw mail send <draft-id>: prints the draft headers, then sends and reports Sent message <id> in thread <thread-id>. --dry-run/-n stops after the preview (this leaf must build a client for dry-run since the preview needs the draft; documented in the help). Refuses drafts with no recipients.
No new scope: gmail.modify already covers drafts.send.
gro mail still has no send; TestGroNeverLinksWriteCode and TestNoDestructiveAPIMethodsInProductionCode keep it that way.

Closes #9

Test plan

make check, make test-cover-check (74.4%), go test ./internal/architecture/
grw mail send --help documents --dry-run; gro mail --help has no send
Live send against a Google account (not run)

Adds the product's only send path: grw mail send fetches a draft's headers, previews them, and sends via drafts.send from internal/rw/gmail. Dry-run stops after the preview; drafts with no recipients are refused. gro still links no send code, enforced by the architecture tests. Closes #9

monit-reviewer

Automated PR Review

Reviewed commit: 57c08b6

Summary

Reviewer	Findings
harness-engineering:harness-architecture-reviewer	1
harness-engineering:harness-enforcement-reviewer	2
harness-engineering:harness-knowledge-reviewer	2
security:security-code-auditor	3

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

💡 Suggestion - internal/rwcmd/mail/send.go:34

There is a TOCTOU gap between GetDraft (preview) and SendDraft (send): the two calls are not atomic, so the draft's content/recipients could change between preview and send (e.g. edited or deleted concurrently in another Gmail client). Given this command sends real, irreversible email, consider re-validating the fetched recipients are still consistent or documenting this limitation, similar to how other destructive commands (e.g. delete) guard risky irreversible actions.

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

💡 Suggestion - internal/rw/gmail/client.go:22

Client now stores its own service/userID copies pulled from the embedded gmailapi.Client at construction time, duplicating state already reachable through the embedded *gmailapi.Client. If the base client's identity is ever mutated or refreshed after construction (e.g. token/account switch), these cached copies would go stale independently of the embedded client. Consider calling through the embedded client's accessors directly in drafts.go instead of duplicating the fields.

💡 Suggestion - internal/rwcmd/mail/send_test.go:9

No test exercises the 'recipients present via Bcc only' path (draft.To == "" && draft.Cc == "" but Bcc set) to confirm the OR-based recipient check in send.go correctly allows sending. Current tests only cover all-empty (blocked) and all-populated (allowed) cases.

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

💡 Suggestion - internal/rw/gmail/drafts.go:58

countAttachments treats any MIME part with a non-empty Filename as an attachment without checking Content-Disposition. Inline parts (e.g. embedded signature images, typically marked Content-Disposition: inline but still carrying a Filename) will be counted toward 'Attachments: N' in the send preview, overstating what a recipient would actually see as an attachment.

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

No test covers GetDraft's early-return branches for draft.Message == nil or draft.Message.Payload == nil (drafts.go lines 21-23 and 26-28). Both are real code paths (e.g. a draft with no message body yet) and are currently untested.

security:security-code-auditor (3 findings)

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

GetDraft fetches with Format("metadata"), but Gmail's metadata format returns only message ID, labels, and headers — it does not populate payload.Parts. Since countAttachments walks draft.Message.Payload.Parts, AttachmentCount will silently report 0 against the real API even when the draft has attachments. This preview is the sole human-review gate before an irreversible, outward-facing send, so a wrong attachment count undermines that safety check. The unit test in drafts_test.go doesn't catch this because its fake server returns a parts array regardless of the requested format — a shape the real metadata-format API would never return — giving false confidence. Either request Format("full") or drop AttachmentCount from the preview, and update the test to mirror the real metadata-format response shape (no parts). Verify against the live API before merging, per the PR's own note that the live send test was not run.

💡 Suggestion - internal/rwcmd/mail/send.go:31

The --dry-run early return happens before the 'no recipients' guard, so grw mail send --dry-run on a recipient-less draft silently prints a blank To/Cc/Bcc preview with no warning, even though the real send would refuse it. Move the recipient check above the dry-run return (or run it regardless of dry-run) so the preview accurately reflects what a real send would do. Untested: existing dry-run tests only cover drafts with recipients present.

💡 Suggestion - internal/rwcmd/mail/send.go:37

Sending email is irreversible and outward-facing, yet unlike permanent delete (which the package docs say sits 'behind a guard') there is no confirmation or explicit --yes/--confirm flag — the command prints the preview and immediately sends. Since this CLI is the read-write surface intended for automated/agent use, consider requiring an explicit affirmative flag or interactive confirmation so a mistyped or stale draft ID cannot send mail to third parties with a single unguarded invocation. --dry-run helps but is opt-in.

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

Completed in 2m 43s | $2.20 | sonnet | daemon 0.2.142 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, security:security-code-auditor, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-architecture-reviewer
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 43s wall · 2m 39s compute (Reviewers: 1m 47s · Synthesis: 51s)
Cost	$2.20 (estimated)
Tokens	368.7k in / 33.7k out
Turns	10

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	68.4k	5.4k	27.9k	40.5k (1h)	$0.33
security:security-code-auditor	—	63.4k	3.1k	19.4k	43.9k (1h)	$0.53
harness-engineering:harness-knowledge-reviewer	sonnet	80.7k	10.6k	26.4k	54.2k (1h)	$0.49
harness-engineering:harness-enforcement-reviewer	sonnet	76.0k	5.9k	26.4k	49.6k (1h)	$0.39
harness-engineering:harness-architecture-reviewer	sonnet	80.3k	8.6k	27.9k	52.4k (1h)	$0.45

_{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-09-03T23:33:14Z

+
+// GetDraft returns the metadata needed to preview a draft before sending it.
+func (c *Client) GetDraft(ctx context.Context, draftID string) (*gmailapi.DraftSummary, error) {
+	draft, err := c.service.Users.Drafts.Get(c.userID, draftID).Format("metadata").Context(ctx).Do()


🟡 Medium (security:security-code-auditor): GetDraft fetches with Format("metadata"), but Gmail's metadata format returns only message ID, labels, and headers — it does not populate payload.Parts. Since countAttachments walks draft.Message.Payload.Parts, AttachmentCount will silently report 0 against the real API even when the draft has attachments. This preview is the sole human-review gate before an irreversible, outward-facing send, so a wrong attachment count undermines that safety check. The unit test in drafts_test.go doesn't catch this because its fake server returns a parts array regardless of the requested format — a shape the real metadata-format API would never return — giving false confidence. Either request Format("full") or drop AttachmentCount from the preview, and update the test to mirror the real metadata-format response shape (no parts). Verify against the live API before merging, per the PR's own note that the live send test was not run.

Reply to this thread when addressed.

Addressed in 167c48e: GetDraft now requests format=full (metadata omits payload parts), with the comment explaining why; the test asserts the format.

monit-reviewer · 2026-09-03T23:33:14Z

+			if err != nil {
+				return fmt.Errorf("getting draft: %w", err)
+			}
+			printDraftSummary(draft)


🔵 Low (security:security-code-auditor): The --dry-run early return happens before the 'no recipients' guard, so grw mail send --dry-run on a recipient-less draft silently prints a blank To/Cc/Bcc preview with no warning, even though the real send would refuse it. Move the recipient check above the dry-run return (or run it regardless of dry-run) so the preview accurately reflects what a real send would do. Untested: existing dry-run tests only cover drafts with recipients present.

Reply to this thread when addressed.

Addressed in 167c48e: the recipient check runs before the dry-run return, so a dry-run on a recipient-less draft fails the same way a real send would. Test added.

monit-reviewer · 2026-09-03T23:33:14Z

+			}
+			if draft.To == "" && draft.Cc == "" && draft.Bcc == "" {
+				return fmt.Errorf("draft has no recipients")
+			}


🔵 Low (security:security-code-auditor): Sending email is irreversible and outward-facing, yet unlike permanent delete (which the package docs say sits 'behind a guard') there is no confirmation or explicit --yes/--confirm flag — the command prints the preview and immediately sends. Since this CLI is the read-write surface intended for automated/agent use, consider requiring an explicit affirmative flag or interactive confirmation so a mistyped or stale draft ID cannot send mail to third parties with a single unguarded invocation. --dry-run helps but is opt-in.

Reply to this thread when addressed.

Declining by design: sending is not in the --permanent class (it is not data loss), the draft ID is an explicit human choice, and the preview prints exactly what goes out. Adding a typed confirmation would make the leaf useless for the piping/agent flows grw exists for. Dry-run remains the review step.

monit-reviewer · 2026-09-03T23:33:14Z

+
+func countAttachments(part *gmailv1.MessagePart) int {
+	count := 0
+	if part.Filename != "" {


🔵 Low (harness-engineering:harness-knowledge-reviewer): countAttachments treats any MIME part with a non-empty Filename as an attachment without checking Content-Disposition. Inline parts (e.g. embedded signature images, typically marked Content-Disposition: inline but still carrying a Filename) will be counted toward 'Attachments: N' in the send preview, overstating what a recipient would actually see as an attachment.

Reply to this thread when addressed.

Addressed in 167c48e: countAttachments now reads Content-Disposition; inline parts are skipped and only attachment-disposition or undisposed named parts count. Test covers an inline logo.

monit-reviewer · 2026-09-03T23:33:14Z

@@ -0,0 +1,105 @@
+package gmail


🔵 Low (harness-engineering:harness-knowledge-reviewer): No test covers GetDraft's early-return branches for draft.Message == nil or draft.Message.Payload == nil (drafts.go lines 21-23 and 26-28). Both are real code paths (e.g. a draft with no message body yet) and are currently untested.

Reply to this thread when addressed.

Addressed in 167c48e: TestGetDraftWithoutMessageOrPayload covers both early returns.

monit-reviewer · 2026-09-03T23:33:14Z

@@ -20,6 +20,8 @@ import (
 // *gmail.Client and adds the write operations below.
 type Client struct {
 	*gmailapi.Client


🔵 Low (harness-engineering:harness-enforcement-reviewer): Client now stores its own service/userID copies pulled from the embedded gmailapi.Client at construction time, duplicating state already reachable through the embedded *gmailapi.Client. If the base client's identity is ever mutated or refreshed after construction (e.g. token/account switch), these cached copies would go stale independently of the embedded client. Consider calling through the embedded client's accessors directly in drafts.go instead of duplicating the fields.

Reply to this thread when addressed.

Declining: this mirrors the calendar and contacts write clients, which capture the underlying service at construction the same way; the base client has no post-construction identity mutation, so there is nothing to go stale.

monit-reviewer · 2026-09-03T23:33:14Z

+	"strings"
+	"testing"
+
+	gmailapi "github.com/open-cli-collective/google-cli/internal/api/gmail"


🔵 Low (harness-engineering:harness-enforcement-reviewer): No test exercises the 'recipients present via Bcc only' path (draft.To == "" && draft.Cc == "" but Bcc set) to confirm the OR-based recipient check in send.go correctly allows sending. Current tests only cover all-empty (blocked) and all-populated (allowed) cases.

Reply to this thread when addressed.

Addressed in 167c48e: TestSend_BccOnlyRecipientSends.

monit-reviewer · 2026-09-03T23:33:14Z

+			printDraftSummary(draft)
+			if dryRun {
+				return nil
+			}


🔵 Low (harness-engineering:harness-architecture-reviewer): There is a TOCTOU gap between GetDraft (preview) and SendDraft (send): the two calls are not atomic, so the draft's content/recipients could change between preview and send (e.g. edited or deleted concurrently in another Gmail client). Given this command sends real, irreversible email, consider re-validating the fetched recipients are still consistent or documenting this limitation, similar to how other destructive commands (e.g. delete) guard risky irreversible actions.

Reply to this thread when addressed.

Addressed in 167c48e: the help text now states that Gmail sends the draft as it exists at send time. drafts.send takes only the draft ID, so the API offers no way to pin the previewed content.

Fetch drafts in full format so attachments are visible, skip inline parts when counting them, refuse recipient-less drafts in dry-run too, and note that Gmail sends the draft as it exists at send time.

monit-reviewer

Automated PR Review

Reviewed commit: 167c48e | Previous: 57c08b6 (incremental)

Approved with 1 non-blocking suggestion below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-knowledge-reviewer	1

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

💡 Suggestion - docs/golden-principles.md:9

The rationale for not requiring an explicit confirmation/--yes flag before grw mail send (send isn't in the --permanent data-loss class, the draft ID is an explicit human choice, the preview shows exactly what will go out, and a confirmation prompt would break piping/agent flows — dry-run is the review step) currently lives only in PR discussion. Capturing it near the existing 'permanent behind a guard' convention in docs/golden-principles.md would give future contributors a precedent to reference when deciding whether the next irreversible grw action needs confirmation, instead of re-litigating the question from scratch. This does not ask to revisit the design decision itself, which is already settled.

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

8 PR discussion threads considered.

Completed in 1m 55s | $4.67 | sonnet | daemon 0.2.142 | 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	1m 55s wall · 1m 29s compute (Reviewers: 55s · Synthesis: 34s)
Cost	$4.67 (estimated)
Tokens	1102.5k in / 49.6k out
Turns	24

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	157.6k	8.8k	54.3k	103.3k (1h)	$0.77
documentation:docs-reviewer	sonnet	52.5k	873	26.4k	26.0k (1h)	$0.18
harness-engineering:harness-architecture-reviewer	sonnet	201.7k	10.3k	108.2k	93.5k (1h)	$0.75
harness-engineering:harness-enforcement-reviewer	sonnet	191.1k	7.7k	102.4k	88.7k (1h)	$0.68
harness-engineering:harness-knowledge-reviewer	sonnet	207.4k	14.7k	107.1k	100.3k (1h)	$0.85
harness-engineering:harness-self-documenting-code-reviewer	sonnet	79.8k	2.5k	26.4k	53.3k (1h)	$0.37
security:security-code-auditor	sonnet	167.2k	3.5k	47.3k	119.8k (1h)	$0.79
discussion-summarizer	—	45.3k	1.3k	20.3k	24.9k (1h)	$0.29

_{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-09-03T23:37:46Z


 `gro` must have no dependency path into `internal/rw` or `internal/rwcmd`, its scopes must stay on the non-destructive allowlist, and its production graph must contain no forbidden destructive Google API calls.

+The only send path is `grw mail send` in `internal/rw/gmail`; it is unreachable from `gro` by the link-graph test.


🔵 Low (harness-engineering:harness-knowledge-reviewer): The rationale for not requiring an explicit confirmation/--yes flag before grw mail send (send isn't in the --permanent data-loss class, the draft ID is an explicit human choice, the preview shows exactly what will go out, and a confirmation prompt would break piping/agent flows — dry-run is the review step) currently lives only in PR discussion. Capturing it near the existing 'permanent behind a guard' convention in docs/golden-principles.md would give future contributors a precedent to reference when deciding whether the next irreversible grw action needs confirmation, instead of re-litigating the question from scratch. This does not ask to revisit the design decision itself, which is already settled.

Reply to this thread when addressed.

Addressed in e4832c3: golden principle 3 now states that confirmation is reserved for unrecoverable data loss and that dry-run is the review step for irreversible-but-not-destructive actions like send.

* feat(grw): mail send <draft-id> (#19) * feat(grw): mail send <draft-id> Adds the product's only send path: grw mail send fetches a draft's headers, previews them, and sends via drafts.send from internal/rw/gmail. Dry-run stops after the preview; drafts with no recipients are refused. gro still links no send code, enforced by the architecture tests. Closes #9 * fix(mail): make the send preview trustworthy Fetch drafts in full format so attachments are visible, skip inline parts when counting them, refuse recipient-less drafts in dry-run too, and note that Gmail sends the draft as it exists at send time. * docs: state when write leaves need confirmation * feat(grw): drive domain (#20) Adds grw drive upload/mkdir/rename/move/trash/restore/delete on top of the shared read command tree, backed by internal/rw/drive. delete defaults to Trash and gates --permanent behind the typed confirmation or --yes, like mail delete; bulk IDs come from args, --stdin, or --query. Closes #8 * fix(drive): sanitize printed file names and explain the client and trash quirks

monit-reviewer requested changes Sep 3, 2026

View reviewed changes

rianjs changed the base branch from main to feat/grw-write-surfaces September 3, 2026 23:34

fix(mail): make the send preview trustworthy

167c48e

Fetch drafts in full format so attachments are visible, skip inline parts when counting them, refuse recipient-less drafts in dry-run too, and note that Gmail sends the draft as it exists at send time.

monit-reviewer approved these changes Sep 3, 2026

View reviewed changes

docs: state when write leaves need confirmation

e4832c3

rianjs merged commit 27943b0 into feat/grw-write-surfaces Sep 3, 2026

rianjs deleted the feat/9-grw-mail-send branch September 3, 2026 23:38

rianjs mentioned this pull request Sep 3, 2026

feat(grw): mail send and drive domains #21

Merged

2 of 3 tasks


		`gro` must have no dependency path into `internal/rw` or `internal/rwcmd`, its scopes must stay on the non-destructive allowlist, and its production graph must contain no forbidden destructive Google API calls.

		The only send path is `grw mail send` in `internal/rw/gmail`; it is unreachable from `gro` by the link-graph test.

Conversation

rianjs commented Sep 3, 2026

Summary

Test plan

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

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

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!

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants