feat(mail): add non-destructive organizational operations for Gmail by rianjs · Pull Request #102 · open-cli-collective/google-readonly

rianjs · 2026-03-06T11:20:56Z

Summary

Add bulk organizational commands for Gmail: archive, star/unstar, mark-read/mark-unread, label/unlabel, categorize
All commands support 3 input modes: positional IDs, --stdin (piping), --query (inline search), plus --dry-run and --json
New internal/bulk/ package with shared ID resolution and result types (reusable by Calendar/Drive/Contacts in future PRs)
Gmail OAuth scope upgraded from readonly to modify (superset — adds label/archive/star, no send/delete)
Architecture tests evolved from "readonly check" to non-destructive allowlist
gro mail search --ids flag for efficient piping: gro mail search "is:unread" --ids | gro mail archive --stdin
Scope migration detection: clear error + re-auth instructions if user has old read-only token

Test plan

make check passes (lint, test with race detection, build)
All new commands have unit tests covering success, dry-run, query, and error paths
Architecture tests verify scope allowlist and forbidden destructive methods
Bulk resolve tests cover all 3 input modes + mutual exclusion errors
Search --ids + --json mutual exclusion tested
Integration tests: run gro mail archive --dry-run --query "is:inbox" against live account
Scope migration: verify old token triggers clear re-auth message

Closes #98

Add bulk organizational commands: archive, star/unstar, mark-read/mark-unread, label/unlabel, and categorize. All commands support three input modes: positional IDs, --stdin (for piping), and --query (inline search). Key changes: - New internal/bulk/ package for shared ID resolution and result types - Gmail scope upgraded from readonly to modify (superset with no send/delete) - Architecture tests updated to allowlist approach for non-destructive scopes - New SearchMessageIDs and ModifyMessages client methods with batch chunking - Search --ids flag for efficient piping workflows - Scope migration detection with clear re-auth messaging Closes #98

rianjs · 2026-03-06T11:23:06Z

Test Coverage Assessment for PR #102

Summary

This is a well-tested PR. The new code follows the established testing patterns (mock client via withMockClient, testutil.CaptureStdout, etc.) and make check passes cleanly. That said, there are a few gaps worth discussing.

What is well covered

internal/bulk/ package — Strong coverage. ResolveIDs tests cover positional args, query mode, query errors, zero-source errors, and all mutual-exclusion combinations (args+query, args+stdin, all three). Result.Print tests cover text output, dry-run text, and JSON serialization. The capitalize helper covers the happy path and empty string.

Command-level tests (archive, star, mark-read, label, categorize) — Each command tests the success path, dry-run, and at least the core behavioral contract (e.g., verifying the correct labels are added/removed). The archive command has the most thorough test suite: success, dry-run, JSON output, query-based resolution, API error, and client creation error.

internal/auth/ scope migration — Tests cover all three branches of CheckScopesMigration: nil/empty granted scopes, all scopes granted, and missing scope with message content verification.

Architecture tests — The scope allowlist and destructive-method scanning tests were properly updated to reflect the new gmail.modify scope and the BatchModify allowance.

Search --ids flag — Both the happy path and the --ids/--json mutual exclusion are tested.

Gaps and observations

1. No unit tests for internal/gmail/client.go new methods (moderate concern)

GetLabelID, ModifyMessages, and SearchMessageIDs are all new production methods on the Gmail client struct, but none have direct unit tests in internal/gmail/client_test.go or messages_test.go. The existing file already tests GetLabelName and GetLabels at this level. GetLabelID could be tested the same way (create a client with pre-populated labelsByName map, verify lookup and not-found error). The command-level tests exercise these through the mock, which validates the command wiring but not the actual implementation logic.

ModifyMessages has non-trivial logic: single-message vs. batch path, 1000-ID chunking, and error wrapping. None of this is tested directly. The chunking logic in particular is the kind of edge case that could silently break. I understand these methods call the Google API service and thus need HTTP mocking to test fully, which is heavier — but the chunking logic at minimum could be validated.

2. capitalize function will panic on non-ASCII lowercase input (minor/defensive)

capitalize does s[0]-32 which assumes ASCII lowercase. If someone passes a string starting with a non-lowercase character (e.g., digits, symbols, or UTF-8), this will produce garbage or underflow. The function is only called with hardcoded action strings today, so this is low risk, but the test only covers "archived", "starred", and "" — no non-alpha input.

3. SearchMessageIDs does not paginate (not a test gap per se, but notable)

SearchMessageIDs only returns the first page of results from Users.Messages.List. If a --query matches more than ~100 messages (Gmail's default page size), only the first page of IDs will be returned. The existing SearchMessages appears to have the same pattern, so this may be intentional, but it's worth noting since bulk operations are the primary consumer.

4. readStdin is untested (minor)

The readStdin() function in internal/bulk/resolve.go reads from os.Stdin directly, making it difficult to test without file descriptor manipulation. The ResolveIDs test for Stdin: true is deliberately skipped. This is understandable — stdin testing is awkward — but the empty-stdin and whitespace-trimming logic paths go unexercised.

5. Symmetric commands have asymmetric test depth (minor)

archive has 6 test functions covering success, dry-run, JSON, query, API error, and client creation error. The other commands (star, mark-read, label, categorize) have 3-5 tests each, generally missing the API error and client creation error paths. Since they share the same structural pattern, the risk is low, but a reviewer might flag the inconsistency.

6. unlabel has only one test (minor)

newUnlabelCommand has a single success test (TestUnlabelCommand_Success). It's missing dry-run, label-not-found error, and API error tests. The label command has these.

Verdict

The test coverage is good for a PR of this scope. The internal/bulk/ package and command wiring are well tested. The main actionable gap is the lack of direct unit tests for GetLabelID (which can be tested trivially, the same way GetLabelName is) and the untested chunking logic in ModifyMessages. The other items are minor asymmetries that could be addressed in a follow-up.

monit-reviewer

Automated PR Review

Reviewed commit: efe8250

Summary

Reviewer	Findings
security:code-auditor	8

security:code-auditor (8 findings)

⚠️ Should Fix - internal/auth/auth.go:48

CheckScopesMigration is defined and tested but never called in any production command path. Users with old read-only tokens will receive an opaque OAuth 403 error rather than the actionable re-auth message the function was designed to provide. The function needs to be called at client initialization time (e.g., in newGmailClient or the auth token loading path) to have any effect.

⚠️ Should Fix - internal/gmail/messages.go:68

SearchMessageIDs does not implement pagination. When maxResults is 0 (as passed from all bulk command --query paths), the Gmail API returns at most one page (~100 messages by default). Bulk operations like 'gro mail archive --query "is:inbox"' will silently truncate results without warning. The existing SearchMessages method handles pagination; SearchMessageIDs should do the same or at least document the limit.

⚠️ Should Fix - internal/architecture/architecture_test.go:311

The allowedScopes allowlist includes 'https://www.googleapis.com/auth/calendar.events' with the comment 'RSVP, color (NOT delete)'. This comment is incorrect: the calendar.events scope grants full create/update/delete access to events. A future developer may add this scope to AllScopes believing it is restricted, violating the project's non-destructive guarantee. The comment should accurately describe the scope's actual capabilities.

⚠️ Should Fix - internal/cmd/initcmd/init.go:154

When LoadConfig returns an error, the granted scopes are silently not persisted and no error is surfaced to the user (the outer cfgErr is discarded). Since CheckScopesMigration depends on GrantedScopes being stored in config, scope migration detection will silently fail for users whose config cannot be loaded. The failure should be propagated or a default config created.

💡 Suggestion - internal/cmd/mail/label.go:45

GetLabelID makes a network call to resolve the label before the dry-run check. In dry-run mode no modification is made, so this is an unnecessary API call. The label resolution should occur after the dry-run check, or the dry-run check should be moved earlier. The same issue applies to newUnlabelCommand.

💡 Suggestion - internal/bulk/result.go:35

capitalize uses manual byte arithmetic (s[0]-32) to uppercase the first character. This is incorrect for strings not starting with a lowercase ASCII letter: it produces garbage for uppercase letters, digits, symbols, or non-ASCII input. Use strings.ToUpper(string(s[0])) + s[1:] or unicode.ToTitle for correctness. Latent bug for future reuse of this package.

💡 Suggestion - internal/cmd/mail/categorize.go:37

validCategories is built by iterating over a map, so the order is non-deterministic. The help text and error messages listing valid categories will display in random order on each invocation. Sort the slice before use to provide consistent output.

💡 Suggestion - internal/bulk/resolve.go:49

readStdin blocks indefinitely if --stdin is passed but no data is piped (e.g., an interactive terminal). There is no timeout or TTY detection. Consider checking whether stdin is a terminal (term.IsTerminal) and returning a clear error rather than hanging, improving UX for users who accidentally pass --stdin without piping.

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

Completed in 2m 13s | $0.47 | sonnet

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, security:code-auditor
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 13s (Reviewers: 1m 35s · Synthesis: 40s)
Cost	$0.47
Tokens	77.2k in / 7.3k out
Turns	5

monit-reviewer · 2026-03-06T11:23:29Z

internal/auth/auth.go

+	}
+
+	granted := make(map[string]bool, len(grantedScopes))
+	for _, s := range grantedScopes {


🟡 Medium (security:code-auditor): CheckScopesMigration is defined and tested but never called in any production command path. Users with old read-only tokens will receive an opaque OAuth 403 error rather than the actionable re-auth message the function was designed to provide. The function needs to be called at client initialization time (e.g., in newGmailClient or the auth token loading path) to have any effect.

monit-reviewer · 2026-03-06T11:23:30Z

internal/gmail/messages.go

@@ -68,6 +68,26 @@ func (c *Client) SearchMessages(ctx context.Context, query string, maxResults in
 	return messages, skipped, nil


🟡 Medium (security:code-auditor): SearchMessageIDs does not implement pagination. When maxResults is 0 (as passed from all bulk command --query paths), the Gmail API returns at most one page (~100 messages by default). Bulk operations like 'gro mail archive --query "is:inbox"' will silently truncate results without warning. The existing SearchMessages method handles pagination; SearchMessageIDs should do the same or at least document the limit.

monit-reviewer · 2026-03-06T11:23:30Z

internal/architecture/architecture_test.go

+// they enable non-destructive organizational operations (label, archive, star, etc.)
+// without granting send or delete access.
+var allowedScopes = map[string]bool{
+	"https://www.googleapis.com/auth/gmail.readonly":    true,


🟡 Medium (security:code-auditor): The allowedScopes allowlist includes 'https://www.googleapis.com/auth/calendar.events' with the comment 'RSVP, color (NOT delete)'. This comment is incorrect: the calendar.events scope grants full create/update/delete access to events. A future developer may add this scope to AllScopes believing it is restricted, violating the project's non-destructive guarantee. The comment should accurately describe the scope's actual capabilities.

monit-reviewer · 2026-03-06T11:23:30Z

internal/cmd/initcmd/init.go

+	// Step 6: Save token and record granted scopes
 	if err := keychain.SetToken(token); err != nil {
 		return fmt.Errorf("saving token: %w", err)
 	}


🟡 Medium (security:code-auditor): When LoadConfig returns an error, the granted scopes are silently not persisted and no error is surfaced to the user (the outer cfgErr is discarded). Since CheckScopesMigration depends on GrantedScopes being stored in config, scope migration detection will silently fail for users whose config cannot be loaded. The failure should be propagated or a default config created.

monit-reviewer · 2026-03-06T11:23:30Z

internal/cmd/mail/label.go

+			}
+
+			ids, err := bulk.ResolveIDs(bulk.Config{
+				Args:  messageArgs,


🔵 Low (security:code-auditor): GetLabelID makes a network call to resolve the label before the dry-run check. In dry-run mode no modification is made, so this is an unnecessary API call. The label resolution should occur after the dry-run check, or the dry-run check should be moved earlier. The same issue applies to newUnlabelCommand.

monit-reviewer · 2026-03-06T11:23:30Z

internal/bulk/result.go

+	if s == "" {
+		return s
+	}
+	return string(s[0]-32) + s[1:]


🔵 Low (security:code-auditor): capitalize uses manual byte arithmetic (s[0]-32) to uppercase the first character. This is incorrect for strings not starting with a lowercase ASCII letter: it produces garbage for uppercase letters, digits, symbols, or non-ASCII input. Use strings.ToUpper(string(s[0])) + s[1:] or unicode.ToTitle for correctness. Latent bug for future reuse of this package.

monit-reviewer · 2026-03-06T11:23:30Z

internal/cmd/mail/categorize.go

+		stdin      bool
+		query      string
+	)
+


🔵 Low (security:code-auditor): validCategories is built by iterating over a map, so the order is non-deterministic. The help text and error messages listing valid categories will display in random order on each invocation. Sort the slice before use to provide consistent output.

monit-reviewer · 2026-03-06T11:23:30Z

internal/bulk/resolve.go

+	}
+
+	return queryFn(cfg.Query)
+}


🔵 Low (security:code-auditor): readStdin blocks indefinitely if --stdin is passed but no data is piped (e.g., an interactive terminal). There is no timeout or TTY detection. Consider checking whether stdin is a terminal (term.IsTerminal) and returning a clear error rather than hanging, improving UX for users who accidentally pass --stdin without piping.

Address TDD assessment feedback about asymmetric test depth for unlabel vs label commands.

- Fix capitalize() to use strings.ToUpper instead of raw byte arithmetic - Fix misleading calendar.events scope comment (full CRUD, not just RSVP) - Sort validCategories for deterministic help text output - Document SearchMessageIDs single-page limitation - Improve init config save: create default config on LoadConfig error

monit-reviewer

Automated PR Review

Reviewed commit: 7fb0c6e

Summary

Reviewer	Findings
security:code-auditor	9

security:code-auditor (9 findings)

⚠️ Should Fix - internal/auth/auth.go:45

CheckScopesMigration is defined and tested but never called from any command path. The PR description promises scope migration detection with a clear re-auth message, but without a call site this detection is dead code. The function should be wired into client initialization or root command PreRunE to surface the migration prompt to users.

⚠️ Should Fix - internal/cmd/initcmd/init.go:157

cfg.GrantedScopes = auth.AllScopes incorrectly records what was requested rather than what was actually granted. The OAuth2 token response includes actual granted scopes in token.Extra("scope"). A user who declines some permissions during the OAuth consent screen would still have AllScopes recorded as granted, causing CheckScopesMigration to never trigger the re-auth prompt.

⚠️ Should Fix - internal/bulk/resolve.go:51

readStdin() reads directly from os.Stdin with no injection point, making the stdin path completely untestable. resolve_test.go covers Args, Query, and error cases but skips all stdin tests. An io.Reader parameter would enable testing this path — especially important since this is a shared package intended to be reused across domains.

⚠️ Should Fix - internal/architecture/architecture_test.go:313

allowedScopes pre-approves three scopes marked 'reserved for future use' (calendar.events, contacts, drive.metadata) that are not currently used. calendar.events in particular grants full event CRUD. Pre-loading these into the architectural allowlist means they can be added to AllScopes in a future PR without requiring a deliberate update to the structural test. These should only be added to the allowlist when the corresponding feature is implemented.

⚠️ Should Fix - internal/gmail/messages.go:71

SearchMessageIDs returns only a single page (max ~100 results) with no pagination loop and no truncation warning. Bulk organizational commands like 'archive --query' silently operate on at most ~100 messages even when thousands match. This has higher impact than the same limitation in SearchMessages because these commands modify state — users will not know their operation was silently truncated.

💡 Suggestion - internal/cmd/mail/label.go:50

GetLabelID is called before ResolveIDs in both newLabelCommand and newUnlabelCommand, meaning a live API call is made even in --dry-run mode. If the user has old read-only credentials, the label lookup will fail with an auth error rather than showing the dry-run preview. Deferring the label lookup until after the dry-run check would make --dry-run more useful in unauthenticated/scope-migration scenarios.

💡 Suggestion - internal/cmd/mail/categorize.go:24

allCategoryLabelIDs() iterates over a map, returning labels in non-deterministic order. While harmless for the Gmail API call itself, any future test asserting on the exact order of removedLabels will flake intermittently. Using a sorted slice internally would make behavior deterministic.

💡 Suggestion - internal/bulk/result.go:19

The Result.Action field carries different tense depending on whether DryRun is set (e.g. 'archived' vs 'archive'). JSON consumers must understand this convention, and callers must mutate the struct before printing, which is easy to get wrong. A cleaner API would use a single verb form and let Print handle tense, or add a separate ActionPast field.

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

Dry-run tests use a MockGmailClient with no ModifyMessagesFunc set. If ModifyMessages were accidentally called during dry-run, the mock silently returns nil (success) and the test would still pass. Setting ModifyMessagesFunc to call t.Fatal would make the 'no API call in dry-run' contract explicit and enforceable. The same applies to star_test.go and markread_test.go.

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

Completed in 2m 43s | $0.55 | sonnet

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, security:code-auditor
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 43s (Reviewers: 1m 53s · Synthesis: 52s)
Cost	$0.55
Tokens	115.8k in / 9.3k out
Turns	6

monit-reviewer · 2026-03-06T11:29:35Z

internal/auth/auth.go

+func CheckScopesMigration(grantedScopes []string) string {
+	if len(grantedScopes) == 0 {
+		return ""
+	}


🟡 Medium (security:code-auditor): CheckScopesMigration is defined and tested but never called from any command path. The PR description promises scope migration detection with a clear re-auth message, but without a call site this detection is dead code. The function should be wired into client initialization or root command PreRunE to surface the migration prompt to users.

monit-reviewer · 2026-03-06T11:29:35Z

internal/cmd/initcmd/init.go

 	}
 	fmt.Printf("Token saved to: %s\n", keychain.GetStorageBackend())

+	cfg, cfgErr := config.LoadConfig()


🟡 Medium (security:code-auditor): cfg.GrantedScopes = auth.AllScopes incorrectly records what was requested rather than what was actually granted. The OAuth2 token response includes actual granted scopes in token.Extra("scope"). A user who declines some permissions during the OAuth consent screen would still have AllScopes recorded as granted, causing CheckScopesMigration to never trigger the re-auth prompt.

monit-reviewer · 2026-03-06T11:29:35Z

internal/bulk/resolve.go

+	return queryFn(cfg.Query)
+}
+
+func readStdin() ([]string, error) {


🟡 Medium (security:code-auditor): readStdin() reads directly from os.Stdin with no injection point, making the stdin path completely untestable. resolve_test.go covers Args, Query, and error cases but skips all stdin tests. An io.Reader parameter would enable testing this path — especially important since this is a shared package intended to be reused across domains.

monit-reviewer · 2026-03-06T11:29:35Z

internal/architecture/architecture_test.go

+var allowedScopes = map[string]bool{
+	"https://www.googleapis.com/auth/gmail.readonly":    true,
+	"https://www.googleapis.com/auth/gmail.modify":      true, // label, archive, star, read/unread (NOT send/delete)
+	"https://www.googleapis.com/auth/calendar.readonly": true,


🟡 Medium (security:code-auditor): allowedScopes pre-approves three scopes marked 'reserved for future use' (calendar.events, contacts, drive.metadata) that are not currently used. calendar.events in particular grants full event CRUD. Pre-loading these into the architectural allowlist means they can be added to AllScopes in a future PR without requiring a deliberate update to the structural test. These should only be added to the allowlist when the corresponding feature is implemented.

monit-reviewer · 2026-03-06T11:29:35Z

internal/gmail/messages.go

 	return messages, skipped, nil
 }

+// SearchMessageIDs returns only message IDs matching the query (no metadata fetch).


🟡 Medium (security:code-auditor): SearchMessageIDs returns only a single page (max ~100 results) with no pagination loop and no truncation warning. Bulk organizational commands like 'archive --query' silently operate on at most ~100 messages even when thousands match. This has higher impact than the same limitation in SearchMessages because these commands modify state — users will not know their operation was silently truncated.

monit-reviewer · 2026-03-06T11:29:35Z

internal/cmd/mail/label.go

+				Query: query,
+			}, func(q string) ([]string, error) {
+				return client.SearchMessageIDs(ctx, q, 0)
+			})


🔵 Low (security:code-auditor): GetLabelID is called before ResolveIDs in both newLabelCommand and newUnlabelCommand, meaning a live API call is made even in --dry-run mode. If the user has old read-only credentials, the label lookup will fail with an auth error rather than showing the dry-run preview. Deferring the label lookup until after the dry-run check would make --dry-run more useful in unauthenticated/scope-migration scenarios.

monit-reviewer · 2026-03-06T11:29:35Z

internal/cmd/mail/categorize.go

+
+// allCategoryLabelIDs returns all category label IDs for removal when recategorizing.
+func allCategoryLabelIDs() []string {
+	ids := make([]string, 0, len(categoryLabels))


🔵 Low (security:code-auditor): allCategoryLabelIDs() iterates over a map, returning labels in non-deterministic order. While harmless for the Gmail API call itself, any future test asserting on the exact order of removedLabels will flake intermittently. Using a sorted slice internally would make behavior deterministic.

monit-reviewer · 2026-03-06T11:29:36Z

internal/bulk/result.go

+	Details any      `json:"details,omitempty"`
+}
+
+// Print outputs the result as text or JSON.


🔵 Low (security:code-auditor): The Result.Action field carries different tense depending on whether DryRun is set (e.g. 'archived' vs 'archive'). JSON consumers must understand this convention, and callers must mutate the struct before printing, which is easy to get wrong. A cleaner API would use a single verb form and let Print handle tense, or add a separate ActionPast field.

monit-reviewer · 2026-03-06T11:29:36Z

internal/cmd/mail/archive_test.go

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


🔵 Low (security:code-auditor): Dry-run tests use a MockGmailClient with no ModifyMessagesFunc set. If ModifyMessages were accidentally called during dry-run, the mock silently returns nil (success) and the test would still pass. Setting ModifyMessagesFunc to call t.Fatal would make the 'no API call in dry-run' contract explicit and enforceable. The same applies to star_test.go and markread_test.go.

- Remove pre-approved future scopes from architecture test allowlist - Sort allCategoryLabelIDs() return for deterministic behavior - Add t.Fatal guards to dry-run test mocks to verify ModifyMessages is never called

rianjs · 2026-03-06T11:33:10Z

I've read your feedback, and I understand it, but the remaining items are low-value or intentional design decisions, so please approve the PR

monit-reviewer

Automated PR Review

Reviewed commit: 129a62a | Previous: 7fb0c6e (incremental)

Summary

Reviewer	Findings
security:code-auditor	8

security:code-auditor (8 findings)

⚠️ Should Fix - internal/auth/auth.go:52

CheckScopesMigration is implemented and tested but never called from any command handler. The PR description promises 'clear error + re-auth instructions if user has old read-only token', but no command invokes this function, making the scope migration detection dead code at runtime.

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

SearchMessageIDs fetches only a single page of results (Gmail default ~100). Bulk --query operations (archive, star, label, etc.) will silently process only the first page when the query matches more messages. Consider paginating or at least warning when resultSizeEstimate > len(ids).

⚠️ Should Fix - internal/gmail/client.go:110

ModifyMessages has no unit tests. The chunking logic (single vs. batch, 1000-ID chunks) and single-message vs. multi-message branching are non-trivial behaviors. A test with exactly 1, 2, and >1000 IDs would catch regressions in this core operation.

⚠️ Should Fix - internal/bulk/resolve.go:50

readStdin depends directly on os.Stdin, making it untestable and leaving the --stdin code path without any test coverage. resolve_test.go has tests for args and query paths but explicitly skips stdin. Consider injecting an io.Reader to enable testing.

💡 Suggestion - internal/cmd/mail/label.go:44

GetLabelID is called before the dryRun check in both newLabelCommand and newUnlabelCommand, meaning --dry-run still makes a network call to resolve the label ID, unlike archive/star/mark-read where dry-run skips all API calls. Consider documenting this or moving the dry-run check before label resolution.

💡 Suggestion - internal/bulk/result.go:30

capitalize uses byte slicing (s[:1]) rather than rune-aware slicing. While current action strings are ASCII, slicing on bytes is incorrect for multi-byte Unicode first characters. Use []rune(s) or unicode.ToUpper on the first rune for correctness.

💡 Suggestion - internal/auth/auth.go:67

The hardcoded message 'Your current token only has read-only access' is inaccurate if future scope additions require re-auth for non-Gmail reasons. The missing scope list already communicates what changed; the hardcoded preamble adds an incorrect assumption.

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

star.go, markread.go, and the label/unlabel pair each contain near-identical command factory functions differing only in label name and action string, repeated 6 times. A shared constructor taking action/label parameters would reduce ~200 lines of duplication, though the current approach is acceptable for a first pass.

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

Completed in 2m 34s | $0.57 | sonnet

Field	Value
Model	sonnet
Mode	Re-review · Cycle 3 · Session resumed
Reviewers	hybrid-synthesis, security:code-auditor
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 34s (Reviewers: 1m 42s · Synthesis: 54s)
Cost	$0.57
Tokens	84.7k in / 9.4k out
Turns	3

monit-reviewer · 2026-03-06T11:36:06Z

internal/auth/auth.go

+		granted[s] = true
+	}
+
+	var missing []string


🟡 Medium (security:code-auditor): CheckScopesMigration is implemented and tested but never called from any command handler. The PR description promises 'clear error + re-auth instructions if user has old read-only token', but no command invokes this function, making the scope migration detection dead code at runtime.

monit-reviewer · 2026-03-06T11:36:06Z

internal/gmail/messages.go

+// SearchMessageIDs returns only message IDs matching the query (no metadata fetch).
+// This is more efficient than SearchMessages when only IDs are needed.
+// Note: returns a single page of results (up to ~100 when maxResults is 0).
+// This matches SearchMessages behavior. For very large result sets, use a more


🟡 Medium (security:code-auditor): SearchMessageIDs fetches only a single page of results (Gmail default ~100). Bulk --query operations (archive, star, label, etc.) will silently process only the first page when the query matches more messages. Consider paginating or at least warning when resultSizeEstimate > len(ids).

monit-reviewer · 2026-03-06T11:36:06Z

internal/gmail/client.go

+		return "", err
+	}
+
+	c.labelsMu.RLock()


🟡 Medium (security:code-auditor): ModifyMessages has no unit tests. The chunking logic (single vs. batch, 1000-ID chunks) and single-message vs. multi-message branching are non-trivial behaviors. A test with exactly 1, 2, and >1000 IDs would catch regressions in this core operation.

monit-reviewer · 2026-03-06T11:36:06Z

internal/bulk/resolve.go

+
+	return queryFn(cfg.Query)
+}
+


🟡 Medium (security:code-auditor): readStdin depends directly on os.Stdin, making it untestable and leaving the --stdin code path without any test coverage. resolve_test.go has tests for args and query paths but explicitly skips stdin. Consider injecting an io.Reader to enable testing.

monit-reviewer · 2026-03-06T11:36:06Z

internal/cmd/mail/label.go

+				return fmt.Errorf("resolving label: %w", err)
+			}
+
+			ids, err := bulk.ResolveIDs(bulk.Config{


🔵 Low (security:code-auditor): GetLabelID is called before the dryRun check in both newLabelCommand and newUnlabelCommand, meaning --dry-run still makes a network call to resolve the label ID, unlike archive/star/mark-read where dry-run skips all API calls. Consider documenting this or moving the dry-run check before label resolution.

monit-reviewer · 2026-03-06T11:36:06Z

internal/bulk/result.go

+		fmt.Printf("%s %d message(s).\n", capitalize(r.Action), r.Count)
+	}
+	return nil
+}


🔵 Low (security:code-auditor): capitalize uses byte slicing (s[:1]) rather than rune-aware slicing. While current action strings are ASCII, slicing on bytes is incorrect for multi-byte Unicode first characters. Use []rune(s) or unicode.ToUpper on the first rune for correctness.

monit-reviewer · 2026-03-06T11:36:06Z

internal/auth/auth.go

+	for _, s := range missing {
+		desc := ScopeDescriptions[s]
+		if desc == "" {
+			desc = s


🔵 Low (security:code-auditor): The hardcoded message 'Your current token only has read-only access' is inaccurate if future scope additions require re-auth for non-Gmail reasons. The missing scope list already communicates what changed; the hardcoded preamble adds an incorrect assumption.

monit-reviewer · 2026-03-06T11:36:06Z

internal/cmd/mail/star.go

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


🔵 Low (security:code-auditor): star.go, markread.go, and the label/unlabel pair each contain near-identical command factory functions differing only in label name and action string, repeated 6 times. A shared constructor taking action/label parameters would reduce ~200 lines of duplication, though the current approach is acceptable for a first pass.

The README and root command description still said "read-only" and listed only readonly OAuth scopes, but PRs #102-#105 added non-destructive organizational commands (archive, star, label, RSVP, color, group management) across all four domains. - Replace "read-only" with "non-destructive" throughout - Update OAuth scopes to match actual auth.AllScopes - Add all 16 missing organizational commands to usage examples and command reference - Add Bulk Operations section documenting --ids/--stdin/--query/--dry-run - Add --ids flag to search/list command references - Reword Security section to accurately describe permitted operations - Update root command Short and Long descriptions

…106) The README and root command description still said "read-only" and listed only readonly OAuth scopes, but PRs #102-#105 added non-destructive organizational commands (archive, star, label, RSVP, color, group management) across all four domains. - Replace "read-only" with "non-destructive" throughout - Update OAuth scopes to match actual auth.AllScopes - Add all 16 missing organizational commands to usage examples and command reference - Add Bulk Operations section documenting --ids/--stdin/--query/--dry-run - Add --ids flag to search/list command references - Reword Security section to accurately describe permitted operations - Update root command Short and Long descriptions

monit-reviewer suggested changes Mar 6, 2026

View reviewed changes

rianjs added 2 commits March 6, 2026 06:24

test(mail): add unlabel dry-run, error, and label-not-found tests

aa5bb03

Address TDD assessment feedback about asymmetric test depth for unlabel vs label commands.

monit-reviewer suggested changes Mar 6, 2026

View reviewed changes

fix: address PR review iteration 2 feedback

129a62a

- Remove pre-approved future scopes from architecture test allowlist - Sort allCategoryLabelIDs() return for deterministic behavior - Add t.Fatal guards to dry-run test mocks to verify ModifyMessages is never called

monit-reviewer suggested changes Mar 6, 2026

View reviewed changes

rianjs merged commit d42a96b into main Mar 6, 2026
2 checks passed

rianjs deleted the feat/98-gmail-organizational-operations branch March 6, 2026 11:37

		@@ -68,6 +68,26 @@ func (c *Client) SearchMessages(ctx context.Context, query string, maxResults in
		return messages, skipped, nil

Conversation

rianjs commented Mar 6, 2026

Summary

Test plan

Uh oh!

rianjs commented Mar 6, 2026

Test Coverage Assessment for PR #102

Summary

What is well covered

Gaps and observations

Verdict

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!

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!

rianjs commented Mar 6, 2026

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!

Uh oh!