feat(grw): mail send and drive domains by rianjs · Pull Request #21 · open-cli-collective/google-cli

rianjs · 2026-09-03T23:50:55Z

Summary

Lands the remaining grw write surfaces in one release. Each piece was reviewed on its own PR against this branch:

mail send (feat(grw): mail send <draft-id> #19): grw mail send <draft-id> previews a draft's headers and attachment count, refuses recipient-less drafts, and sends via drafts.send from internal/rw/gmail. --dry-run stops after the preview. gro still links no send path (architecture tests).
drive (feat(grw): drive domain #20): grw drive upload|mkdir|rename|move|trash|restore|delete backed by internal/rw/drive. delete defaults to Trash and gates --permanent behind the typed confirmation or --yes; bulk IDs from args, --stdin, or --query. grw adds the full drive scope beside the existing readonly/metadata scopes.
Golden principle 3 now states that confirmation is reserved for unrecoverable data loss and dry-run is the review step for irreversible-but-not-destructive actions.

Closes #8, closes #9

Test plan

make check, make test-cover-check (75.3%), go test ./internal/architecture/ on each stacked PR and on the branch tip
Dry-run smoke: grw mail send --help, grw drive upload go.mod --dry-run; gro mail/gro drive help unchanged
Live send and Drive mutations against a Google account (not run)

* 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

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

monit-reviewer

Automated PR Review

Reviewed commit: 935045f

Summary

Reviewer	Findings
harness-engineering:harness-self-documenting-code-reviewer	2
security:security-code-auditor	1

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

💡 Suggestion - internal/rwcmd/drive/bulk.go:9

resolveFileIDs has non-obvious control flow: it lazily builds a WriteClient only if query resolution needs one, skips client construction entirely on dry-run, and then constructs one afterward only if it wasn't already built. The reason for this dance (avoid constructing a client — and thus requiring credentials — during a dry-run or when the client from query resolution can be reused) isn't evident from the code itself, and there's a test (TestDryRunsDoNotConstructClient) asserting this behavior. Add a short comment explaining why client construction is deferred/conditional, or extract the reused-vs-fresh-client decision into a named helper so the intent reads without cross-referencing the test.

💡 Suggestion - internal/rw/drive/client.go:122

setTrashed sets metadata.ForceSendFields = []string{"Trashed"} only when trashed is false, which is a non-obvious workaround for the Drive API's JSON encoding omitting false (zero-value) booleans unless explicitly forced. Add a brief why comment (e.g., "Trashed:false is the zero value and would be silently omitted by the JSON encoder without ForceSendFields") rather than requiring the reader to already know the quirk of the generated API client.

security:security-code-auditor (1 findings)

⚠️ Should Fix - internal/rwcmd/drive/output.go:32

Terminal escape-sequence injection: printFile() writes file.Name (and ID/Parent) directly via fmt.Printf with no sanitization. Drive file names are not always user-supplied — e.g. grw drive move <file-id> <folder-id> prints the pre-existing name of a file that may have been created/renamed by another account (a collaborator on a shared drive or a file shared into the user's Drive), so the name is effectively externally-controlled input reaching a terminal-rendering sink. A malicious name containing ANSI/OSC escape sequences could rewrite terminal output, hide text, or spoof a success message. This same PR already recognizes this exact risk class for Gmail draft headers and routes them through mailcmd.SanitizeOutput() (internal/rwcmd/mail/send.go); the Drive output path should use the same (or an equivalent) sanitizer before printing file.Name/ID/Parent.

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

Completed in 1m 29s | $3.90 | sonnet | daemon 0.2.142 | 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	1m 29s wall · 1m 25s compute (Reviewers: 1m 04s · Synthesis: 21s)
Cost	$3.90 (estimated)
Tokens	748.5k in / 30.8k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	61.3k	1.9k	26.4k	34.9k (1h)	$0.25
documentation:docs-reviewer	sonnet	60.8k	4.3k	26.4k	34.3k (1h)	$0.28
harness-engineering:harness-architecture-reviewer	sonnet	126.3k	4.1k	26.4k	99.9k (1h)	$0.67
harness-engineering:harness-enforcement-reviewer	sonnet	128.5k	5.7k	26.4k	102.0k (1h)	$0.71
harness-engineering:harness-knowledge-reviewer	sonnet	127.4k	4.7k	26.4k	100.9k (1h)	$0.68
harness-engineering:harness-self-documenting-code-reviewer	sonnet	123.1k	5.5k	26.4k	96.6k (1h)	$0.67
security:security-code-auditor	sonnet	121.1k	4.7k	26.4k	94.7k (1h)	$0.65

_{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:52:32Z

+	"github.com/spf13/cobra"
+
+	"github.com/open-cli-collective/google-cli/internal/bulk"
+)


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): resolveFileIDs has non-obvious control flow: it lazily builds a WriteClient only if query resolution needs one, skips client construction entirely on dry-run, and then constructs one afterward only if it wasn't already built. The reason for this dance (avoid constructing a client — and thus requiring credentials — during a dry-run or when the client from query resolution can be reused) isn't evident from the code itself, and there's a test (TestDryRunsDoNotConstructClient) asserting this behavior. Add a short comment explaining why client construction is deferred/conditional, or extract the reused-vs-fresh-client decision into a named helper so the intent reads without cross-referencing the test.

Reply to this thread when addressed.

Addressed in d82e3fe: resolveFileIDs has a doc comment explaining when a client is built and why dry-run never touches credentials.

monit-reviewer · 2026-09-03T23:52:32Z

+	return driveapi.ParseFile(updated), nil
+}
+
+// TrashFiles moves files to Trash.


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): setTrashed sets metadata.ForceSendFields = []string{"Trashed"} only when trashed is false, which is a non-obvious workaround for the Drive API's JSON encoding omitting false (zero-value) booleans unless explicitly forced. Add a brief why comment (e.g., "Trashed:false is the zero value and would be silently omitted by the JSON encoder without ForceSendFields") rather than requiring the reader to already know the quirk of the generated API client.

Reply to this thread when addressed.

Addressed in d82e3fe: comment explains that false is the zero value and restore would silently no-op without ForceSendFields.

monit-reviewer · 2026-09-03T23:52:32Z

+
+func printFile(file *driveapi.File) {
+	fmt.Printf("ID: %s\n", file.ID)
+	fmt.Printf("Name: %s\n", file.Name)


🟡 Medium (security:security-code-auditor): Terminal escape-sequence injection: printFile() writes file.Name (and ID/Parent) directly via fmt.Printf with no sanitization. Drive file names are not always user-supplied — e.g. grw drive move <file-id> <folder-id> prints the pre-existing name of a file that may have been created/renamed by another account (a collaborator on a shared drive or a file shared into the user's Drive), so the name is effectively externally-controlled input reaching a terminal-rendering sink. A malicious name containing ANSI/OSC escape sequences could rewrite terminal output, hide text, or spoof a success message. This same PR already recognizes this exact risk class for Gmail draft headers and routes them through mailcmd.SanitizeOutput() (internal/rwcmd/mail/send.go); the Drive output path should use the same (or an equivalent) sanitizer before printing file.Name/ID/Parent.

Reply to this thread when addressed.

Addressed in d82e3fe: printFile routes ID, name, type, and parent through the mail sanitizers (SanitizeFilename for the name). The same gap exists in gro's own drive/contacts/calendar printers, which predate this PR; tracked as a follow-up issue to move the sanitizer to a shared package and apply it everywhere.

…ash quirks

monit-reviewer

Automated PR Review

Reviewed commit: d82e3fe | Previous: 935045f (incremental)

Summary

No issues found.

3 PR discussion threads considered.

Completed in 55s | $9.88 | 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	55s wall · 38s compute (Reviewers: 25s · Synthesis: 13s)
Cost	$9.88 (estimated)
Tokens	1982.2k in / 36.6k out
Turns	28

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	135.7k	2.7k	87.8k	47.9k (1h)	$0.35
documentation:docs-reviewer	sonnet	136.2k	4.6k	87.2k	49.0k (1h)	$0.39
harness-engineering:harness-architecture-reviewer	sonnet	337.3k	5.4k	54.3k	283.0k (1h)	$1.79
harness-engineering:harness-enforcement-reviewer	sonnet	343.9k	7.1k	54.3k	289.5k (1h)	$1.86
harness-engineering:harness-knowledge-reviewer	sonnet	339.6k	5.1k	54.3k	285.2k (1h)	$1.80
harness-engineering:harness-self-documenting-code-reviewer	sonnet	326.6k	5.8k	54.3k	272.2k (1h)	$1.74
security:security-code-auditor	sonnet	321.0k	5.3k	54.3k	266.6k (1h)	$1.70
discussion-summarizer	—	42.0k	582	20.3k	21.6k (1h)	$0.24

_{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.}

rianjs added 2 commits September 3, 2026 19:38

monit-reviewer suggested changes Sep 3, 2026

View reviewed changes

fix(drive): sanitize printed file names and explain the client and tr…

d82e3fe

…ash quirks

rianjs mentioned this pull request Sep 3, 2026

Move SanitizeOutput to a shared package and apply it to every printer #22

Closed

monit-reviewer approved these changes Sep 3, 2026

View reviewed changes

rianjs merged commit d4b7fec into main Sep 3, 2026
11 checks passed

rianjs deleted the feat/grw-write-surfaces branch September 3, 2026 23:56

Navigation Menu

Uh oh!

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

feat(grw): mail send and drive domains#21
rianjs merged 3 commits into
mainfrom
feat/grw-write-surfaces

rianjs commented Sep 3, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Sep 3, 2026

Uh oh!

rianjs Sep 3, 2026

Uh oh!

monit-reviewer Sep 3, 2026

Uh oh!

rianjs Sep 3, 2026

Uh oh!

monit-reviewer Sep 3, 2026

Uh oh!

rianjs Sep 3, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

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!

monit-reviewer Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 3, 2026

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants