refactor: build gro on the shared google-cli-common module - #168
Merged
Conversation
Move gro's OAuth/credential/config/cache infrastructure, the Google API clients (gmail/calendar/contacts/drive/people), rendering and bulk helpers, and the shared cobra command packages (mail/init/config/setcred/refresh + root scaffolding) into github.com/open-cli-collective/google-cli-common, and consume them here. gro is now a thin CLI: its own domain commands (calendar/contacts/ drive/me), the read-only mail composition via the shared mailcmd, its identity (internal/appidentity) + config.Register in main, and its architecture tests. Behavior is unchanged and the non-destructive guarantee is preserved: the architecture test asserts appidentity.Scopes stays on the non-destructive allowlist and gro's tree contains no destructive API calls. Internal-only refactor; not a release.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: da7efb3ee39c
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Major - internal/appidentity/appidentity.go:1
The repo's declared source-of-truth docs (docs/architecture.md, docs/golden-principles.md, docs/adding-a-domain.md — all linked from AGENTS.md/STANDARDS.md as the durable package-graph and onboarding reference) still describe the pre-refactor tree: internal/auth, internal/keychain, internal/gmail, internal/calendar, internal/contacts, internal/drive, internal/people, internal/cache, and internal/config, all of which this PR deletes in favor of google-cli-common. adding-a-domain.md even tells the next contributor to edit internal/auth/auth.go, which no longer exists. Since this PR's own stated goal is establishing the pattern gro/grw will follow going forward, leaving these docs unrefreshed means the next domain addition or the next agent onboarding read will follow dead instructions and get a confusing package-not-found result instead of the new config.Identity/appidentity seam. Fix: update docs/architecture.md's dependency graph and package table to show cmd/gro/main.go -> config.Register(appidentity.Identity()) -> root -> shared google-cli-common command packages, and update adding-a-domain.md's scope-registration step to point at internal/appidentity/appidentity.go instead of the deleted internal/auth/auth.go.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | cmd/gro/main.go, go.mod, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/calendar/get.go, internal/cmd/contacts/get.go, internal/cmd/drive/download.go, internal/cmd/me/main_test.go, internal/cmd/me/me.go, internal/cmd/me/me_test.go, internal/cmd/me/output.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go, internal/noleak/noleak_test.go | go.sum, internal/auth/auth.go, internal/auth/auth_test.go, internal/auth/config.go, internal/bulk/resolve.go, internal/bulk/resolve_test.go, internal/bulk/result.go, internal/bulk/result_test.go, internal/cache/cache.go, internal/cache/cache_test.go, internal/calendar/client.go, internal/calendar/client_test.go, internal/calendar/events.go, internal/calendar/events_test.go, internal/cmd/calendar/calendar_test.go, internal/cmd/calendar/color_test.go, internal/cmd/calendar/dates_test.go, internal/cmd/calendar/events_helper.go, internal/cmd/calendar/handlers_test.go, internal/cmd/calendar/list.go, internal/cmd/calendar/output.go, internal/cmd/calendar/output_test.go, internal/cmd/calendar/rsvp_test.go, internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/contacts/contacts_test.go, internal/cmd/contacts/group_manage.go, internal/cmd/contacts/group_manage_test.go, internal/cmd/contacts/groups.go, internal/cmd/contacts/handlers_test.go, internal/cmd/contacts/list.go, internal/cmd/contacts/output.go, internal/cmd/contacts/output_test.go, internal/cmd/contacts/search.go, internal/cmd/contacts/star.go, internal/cmd/contacts/star_test.go, internal/cmd/drive/download_test.go, internal/cmd/drive/drives.go, internal/cmd/drive/drives_test.go, internal/cmd/drive/get.go, internal/cmd/drive/get_test.go, internal/cmd/drive/handlers_test.go, internal/cmd/drive/list.go, internal/cmd/drive/list_test.go, internal/cmd/drive/mock_test.go, internal/cmd/drive/output.go, internal/cmd/drive/search_test.go, internal/cmd/drive/star.go, internal/cmd/drive/star_test.go, internal/cmd/drive/tree.go, internal/cmd/drive/tree_test.go, internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go, internal/cmd/mail/archive.go, internal/cmd/mail/archive_test.go, internal/cmd/mail/attachments.go, internal/cmd/mail/attachments_download.go, internal/cmd/mail/attachments_download_test.go, internal/cmd/mail/attachments_list.go, internal/cmd/mail/attachments_test.go, internal/cmd/mail/categorize.go, internal/cmd/mail/categorize_test.go, internal/cmd/mail/draft.go, internal/cmd/mail/draft_quote_test.go, internal/cmd/mail/draft_test.go, internal/cmd/mail/handlers_test.go, internal/cmd/mail/label.go, internal/cmd/mail/label_test.go, internal/cmd/mail/labels.go, internal/cmd/mail/labels_test.go, internal/cmd/mail/mail.go, internal/cmd/mail/mail_test.go, internal/cmd/mail/markread.go, internal/cmd/mail/markread_test.go, internal/cmd/mail/mock_test.go, internal/cmd/mail/output.go, internal/cmd/mail/output_test.go, internal/cmd/mail/read.go, internal/cmd/mail/read_test.go, internal/cmd/mail/sanitize.go, internal/cmd/mail/sanitize_test.go, internal/cmd/mail/search.go, internal/cmd/mail/search_test.go, internal/cmd/mail/star.go, internal/cmd/mail/star_test.go, internal/cmd/mail/thread.go, internal/cmd/mail/thread_test.go, internal/cmd/refreshcmd/refresh.go, internal/cmd/refreshcmd/refresh_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/contacts/client.go, internal/contacts/client_test.go, internal/contacts/contacts.go, internal/contacts/contacts_test.go, internal/credtest/credtest.go, internal/drive/client.go, internal/drive/files.go, internal/drive/files_test.go, internal/errors/errors.go, internal/errors/errors_test.go, internal/format/format.go, internal/format/format_test.go, internal/gmail/attachments.go, internal/gmail/attachments_test.go, internal/gmail/client.go, internal/gmail/client_test.go, internal/gmail/drafts.go, internal/gmail/drafts_test.go, internal/gmail/messages.go, internal/gmail/messages_test.go, internal/keychain/credref_test.go, internal/keychain/keychain.go, internal/keychain/keychain_metadata_darwin_test.go, internal/keychain/keychain_test.go, internal/keychain/migrate.go, internal/keychain/passphrase.go, internal/keychain/token_source.go, internal/keychain/token_source_test.go, internal/keychain/wire.go, internal/keychain/wire_test.go, internal/log/log.go, internal/log/log_test.go, internal/migrationsink/migrationsink.go, internal/output/migration.go, internal/output/output.go, internal/output/output_test.go, internal/people/client.go, internal/people/client_test.go, internal/testutil/assert.go, internal/testutil/assert_test.go, internal/testutil/fixtures.go, internal/testutil/helpers.go, internal/version/version.go, internal/view/view.go, internal/view/view_test.go, internal/zip/extract.go, internal/zip/extract_test.go, internal/zip/fs.go | The vast majority of assigned files are deletions (code moved verbatim into the external google-cli-common module) or mechanical import-path-only edits (+1/-1); these were spot-checked rather than line-by-line diffed since they carry no implementation-quality risk in this repo.; google-cli-common's own implementation and tests are out of scope for this review; only gro's remaining/added code (appidentity, main, root wiring, architecture and noleak tests) was reviewed in depth. |
| policies:conventions | incomplete_skipped | cmd/gro/main.go, go.mod, go.sum, internal/appidentity/appidentity.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/keychain/keychain.go, internal/keychain/migrate.go, internal/keychain/wire.go | Shared cli-common/google-cli-common docs were not present in review context, so drift against those source-of-truth standards could not be checked. |
| structure:repo-health | incomplete_skipped | cmd/gro/main.go, go.mod, go.sum, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | internal/config/config.go, internal/config/relocate.go | Findings limited to the 9 assigned files; docs/architecture.md, docs/golden-principles.md, and docs/adding-a-domain.md are not in changed_files, so staleness there is reported via an anchor on an assigned file rather than directly on the doc. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 06s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 6m 06s wall · 11m 56s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 49s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 31s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 4m 25s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 4m 00s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 10s |
| @@ -0,0 +1,64 @@ | |||
| // Package appidentity declares google-readonly's CLI identity: the config/ | |||
There was a problem hiding this comment.
File-level note: internal/appidentity/appidentity.go
The repo's declared source-of-truth docs (docs/architecture.md, docs/golden-principles.md, docs/adding-a-domain.md — all linked from AGENTS.md/STANDARDS.md as the durable package-graph and onboarding reference) still describe the pre-refactor tree: internal/auth, internal/keychain, internal/gmail, internal/calendar, internal/contacts, internal/drive, internal/people, internal/cache, and internal/config, all of which this PR deletes in favor of google-cli-common. adding-a-domain.md even tells the next contributor to edit internal/auth/auth.go, which no longer exists. Since this PR's own stated goal is establishing the pattern gro/grw will follow going forward, leaving these docs unrefreshed means the next domain addition or the next agent onboarding read will follow dead instructions and get a confusing package-not-found result instead of the new config.Identity/appidentity seam. Fix: update docs/architecture.md's dependency graph and package table to show cmd/gro/main.go -> config.Register(appidentity.Identity()) -> root -> shared google-cli-common command packages, and update adding-a-domain.md's scope-registration step to point at internal/appidentity/appidentity.go instead of the deleted internal/auth/auth.go.
Reply inline to this comment.
…cli-common split Address the review finding on #168: docs/architecture.md, adding-a-domain.md, golden-principles.md, and development.md still described the pre-refactor tree (internal/auth, internal/gmail, internal/config, ...) and told contributors to add scopes in the deleted internal/auth/auth.go. Update them to the new seam: scopes in internal/appidentity, API clients + shared command packages in google-cli-common, dependency direction guaranteed by the module boundary.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a6c9b3a80dee
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
| documentation:docs | 0 |
structure:repo-health (1 finding)
Major - internal/architecture/architecture_test.go:296
Invariant at risk: gro's non-destructive guarantee (golden-principles.md #5) used to be code-enforced by scanning the gmail client implementation itself for .Send/.Trash/.BatchDelete. After this refactor, TestNoDestructiveAPIMethodsInProductionCode only walks findModuleRoot(t) (this repo's own tree), and the actual gmail/calendar/drive/contacts API client code now lives entirely in google-cli-common, outside that walk. The only remaining structural check is TestAllScopesAreNonDestructive, which validates OAuth scopes, not API call sites. The comment in appidentity.go and docs/golden-principles.md #5 both state 'the shared google-cli-common/gmail client is likewise non-destructive' as an assertion, but nothing in this repo's CI verifies it — a future google-cli-common release that adds a destructive call (e.g. via a shared bulk/batch helper) would ship into gro undetected until a human notices. Consider either vendoring an equivalent destructive-method scan against the resolved google-cli-common module cache in CI, or documenting in cli-common's own repo that this check has moved there and confirming it actually runs in that repo's pipeline — otherwise the non-destructive contract has silently become prose-only for the largest part of the surface it used to cover.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_failed | unavailable | unavailable | structured output invalid after retry: first: llm: constraints entry length out of bounds; second: llm: constraints entry length out of bounds |
| policies:conventions | incomplete_skipped | cmd/gro/main.go, docs/adding-a-domain.md, docs/architecture.md, go.mod, go.sum, internal/appidentity/appidentity.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/keychain/keychain.go, internal/keychain/migrate.go, internal/keychain/wire.go | Reviewed against the head checkout only; no diff hunks or sibling google-cli-common docs were available in context, so dependency-management conventions were checked only against what is visible in go.mod/go.sum and this repo's own docs. |
| structure:repo-health | incomplete_skipped | cmd/gro/main.go, docs/architecture.md, docs/development.md, docs/golden-principles.md, go.mod, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | go.sum, internal/config/config.go, internal/config/relocate.go | google-cli-common (v0.1.1) itself is an external dependency and was not checked out for this review; its own structural tests and CI enforcement could not be inspected, only inferred from go.mod pinning. |
| documentation:docs | complete_broad | docs/adding-a-domain.md, docs/architecture.md, docs/development.md, docs/golden-principles.md | unavailable | unavailable |
Reviewer Diagnostics
| Reviewer | Status | Diagnostic |
|---|---|---|
| go:implementation-tests | failed | structured output invalid after retry: first: llm: constraints entry length out of bounds; second: llm: constraints entry length out of bounds |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 7m 14s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 7m 14s wall · 11m 22s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 49s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 3m 08s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 4m 07s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 40s |
| documentation:docs | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 09s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 26s |
| // (which would permit filters or permanent deletion). | ||
| func TestAllScopesAreNonDestructive(t *testing.T) { | ||
| t.Parallel() | ||
|
|
There was a problem hiding this comment.
Invariant at risk: gro's non-destructive guarantee (golden-principles.md #5) used to be code-enforced by scanning the gmail client implementation itself for .Send/.Trash/.BatchDelete. After this refactor, TestNoDestructiveAPIMethodsInProductionCode only walks findModuleRoot(t) (this repo's own tree), and the actual gmail/calendar/drive/contacts API client code now lives entirely in google-cli-common, outside that walk. The only remaining structural check is TestAllScopesAreNonDestructive, which validates OAuth scopes, not API call sites. The comment in appidentity.go and docs/golden-principles.md #5 both state 'the shared google-cli-common/gmail client is likewise non-destructive' as an assertion, but nothing in this repo's CI verifies it — a future google-cli-common release that adds a destructive call (e.g. via a shared bulk/batch helper) would ship into gro undetected until a human notices. Consider either vendoring an equivalent destructive-method scan against the resolved google-cli-common module cache in CI, or documenting in cli-common's own repo that this check has moved there and confirming it actually runs in that repo's pipeline — otherwise the non-destructive contract has silently become prose-only for the largest part of the surface it used to cover.
Reply inline to this comment.
Address the review finding on #168: after moving the Google API clients into google-cli-common, TestNoDestructiveAPIMethodsInProductionCode only scanned gro's own tree, so the shared clients' non-destructive contract became prose-only. Add TestSharedGoogleClientsAreNonDestructive, which resolves the pinned google-cli-common module in the local cache (go list -m) and scans its gmail/calendar/contacts/drive/people packages for the same forbidden methods (.Send/.Trash/.Untrash/.BatchDelete). A future common release that adds one now fails gro's CI when gro bumps to it. Doc #5 updated to name the check.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 4c87329a2b42
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| documentation:docs | 0 |
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | cmd/gro/main.go, go.mod, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/calendar/get.go, internal/cmd/contacts/get.go, internal/cmd/drive/download.go, internal/cmd/me/main_test.go, internal/cmd/me/me.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go, internal/noleak/noleak_test.go | go.sum, internal/auth/auth.go, internal/auth/auth_test.go, internal/auth/config.go, internal/bulk/resolve.go, internal/bulk/resolve_test.go, internal/bulk/result.go, internal/bulk/result_test.go, internal/cache/cache.go, internal/cache/cache_test.go, internal/calendar/client.go, internal/calendar/client_test.go, internal/calendar/events.go, internal/calendar/events_test.go, internal/cmd/calendar/calendar_test.go, internal/cmd/calendar/color_test.go, internal/cmd/calendar/dates_test.go, internal/cmd/calendar/events_helper.go, internal/cmd/calendar/handlers_test.go, internal/cmd/calendar/list.go, internal/cmd/calendar/output.go, internal/cmd/calendar/output_test.go, internal/cmd/calendar/rsvp_test.go, internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/contacts/contacts_test.go, internal/cmd/contacts/group_manage.go, internal/cmd/contacts/group_manage_test.go, internal/cmd/contacts/groups.go, internal/cmd/contacts/handlers_test.go, internal/cmd/contacts/list.go, internal/cmd/contacts/output.go, internal/cmd/contacts/output_test.go, internal/cmd/contacts/search.go, internal/cmd/contacts/star.go, internal/cmd/contacts/star_test.go, internal/cmd/drive/download_test.go, internal/cmd/drive/drives.go, internal/cmd/drive/drives_test.go, internal/cmd/drive/get.go, internal/cmd/drive/get_test.go, internal/cmd/drive/handlers_test.go, internal/cmd/drive/list.go, internal/cmd/drive/list_test.go, internal/cmd/drive/mock_test.go, internal/cmd/drive/output.go, internal/cmd/drive/search_test.go, internal/cmd/drive/star.go, internal/cmd/drive/star_test.go, internal/cmd/drive/tree.go, internal/cmd/drive/tree_test.go, internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go, internal/cmd/mail/archive.go, internal/cmd/mail/archive_test.go, internal/cmd/mail/attachments.go, internal/cmd/mail/attachments_download.go, internal/cmd/mail/attachments_download_test.go, internal/cmd/mail/attachments_list.go, internal/cmd/mail/attachments_test.go, internal/cmd/mail/categorize.go, internal/cmd/mail/categorize_test.go, internal/cmd/mail/draft.go, internal/cmd/mail/draft_quote_test.go, internal/cmd/mail/draft_test.go, internal/cmd/mail/handlers_test.go, internal/cmd/mail/label.go, internal/cmd/mail/label_test.go, internal/cmd/mail/labels.go, internal/cmd/mail/labels_test.go, internal/cmd/mail/mail.go, internal/cmd/mail/mail_test.go, internal/cmd/mail/markread.go, internal/cmd/mail/markread_test.go, internal/cmd/mail/mock_test.go, internal/cmd/mail/output.go, internal/cmd/mail/output_test.go, internal/cmd/mail/read.go, internal/cmd/mail/read_test.go, internal/cmd/mail/sanitize.go, internal/cmd/mail/sanitize_test.go, internal/cmd/mail/search.go, internal/cmd/mail/search_test.go, internal/cmd/mail/star.go, internal/cmd/mail/star_test.go, internal/cmd/mail/thread.go, internal/cmd/mail/thread_test.go, internal/cmd/me/me_test.go, internal/cmd/me/output.go, internal/cmd/refreshcmd/refresh.go, internal/cmd/refreshcmd/refresh_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/contacts/client.go, internal/contacts/client_test.go, internal/contacts/contacts.go, internal/contacts/contacts_test.go, internal/credtest/credtest.go, internal/drive/client.go, internal/drive/files.go, internal/drive/files_test.go, internal/errors/errors.go, internal/errors/errors_test.go, internal/format/format.go, internal/format/format_test.go, internal/gmail/attachments.go, internal/gmail/attachments_test.go, internal/gmail/client.go, internal/gmail/client_test.go, internal/gmail/drafts.go, internal/gmail/drafts_test.go, internal/gmail/messages.go, internal/gmail/messages_test.go, internal/keychain/credref_test.go, internal/keychain/keychain.go, internal/keychain/keychain_metadata_darwin_test.go, internal/keychain/keychain_test.go, internal/keychain/migrate.go, internal/keychain/passphrase.go, internal/keychain/token_source.go, internal/keychain/token_source_test.go, internal/keychain/wire.go, internal/keychain/wire_test.go, internal/log/log.go, internal/log/log_test.go, internal/migrationsink/migrationsink.go, internal/output/migration.go, internal/output/output.go, internal/output/output_test.go, internal/people/client.go, internal/people/client_test.go, internal/testutil/assert.go, internal/testutil/assert_test.go, internal/testutil/fixtures.go, internal/testutil/helpers.go, internal/version/version.go, internal/view/view.go, internal/view/view_test.go, internal/zip/extract.go, internal/zip/extract_test.go, internal/zip/fs.go | Most assigned files are pure deletions (moved into the external google-cli-common module) or one-line import-path edits; these were spot-checked rather than exhaustively read line-by-line since they carry no new logic.; Sandbox permissions blocked running git and go build/go test in the review checkout, so this review relied on static reading of the checked-out files rather than a compiled/tested verification. |
| policies:conventions | incomplete_skipped | cmd/gro/main.go, docs/adding-a-domain.md, docs/architecture.md, go.mod, go.sum, internal/appidentity/appidentity.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/keychain/keychain.go, internal/keychain/migrate.go, internal/keychain/wire.go | Reviewed against the checked-out PR-head working tree rather than a line-level unified diff; findings reflect current file state at 4c87329.; google-cli-common source is not present in the review context, so only the parts of the shared-module contract visible from this repo (imports, go.mod pin, docs) could be checked. |
| structure:repo-health | incomplete_skipped | cmd/gro/main.go, docs/architecture.md, docs/development.md, docs/golden-principles.md, go.mod, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go | go.sum, internal/config/config.go, internal/config/relocate.go | go.sum not separately inspected beyond confirming go.mod pins google-cli-common v0.1.1 as a direct (non-indirect) requirement.; internal/config/config.go and internal/config/relocate.go are assigned but were deleted by this PR (moved into google-cli-common) and no longer exist at the head SHA; nothing to inspect there. |
| documentation:docs | complete_broad | docs/adding-a-domain.md, docs/architecture.md, docs/development.md, docs/golden-principles.md | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 56s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 4m 56s wall · 7m 57s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 43s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 08s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 44s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 46s |
| documentation:docs | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 24s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 10s |
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 4c87329a2b42
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| documentation:docs | 0 |
| structure:repo-health | 0 |
| automation:ci-release | 0 |
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | incomplete_skipped | cmd/gro/main.go, go.mod, go.sum, internal/appidentity/appidentity.go, internal/architecture/architecture_test.go, internal/cmd/me/main_test.go, internal/cmd/me/me.go, internal/cmd/root/backend_wire_test.go, internal/cmd/root/credref_wire_test.go, internal/cmd/root/main_test.go, internal/cmd/root/root.go, internal/cmd/root/root_test.go, internal/noleak/noleak_test.go | internal/auth/auth.go, internal/auth/auth_test.go, internal/auth/config.go, internal/bulk/resolve.go, internal/bulk/resolve_test.go, internal/bulk/result.go, internal/bulk/result_test.go, internal/cache/cache.go, internal/cache/cache_test.go, internal/calendar/client.go, internal/calendar/client_test.go, internal/calendar/events.go, internal/calendar/events_test.go, internal/cmd/calendar/calendar_test.go, internal/cmd/calendar/color_test.go, internal/cmd/calendar/dates_test.go, internal/cmd/calendar/events_helper.go, internal/cmd/calendar/get.go, internal/cmd/calendar/handlers_test.go, internal/cmd/calendar/list.go, internal/cmd/calendar/output.go, internal/cmd/calendar/output_test.go, internal/cmd/calendar/rsvp_test.go, internal/cmd/config/behavior_test.go, internal/cmd/config/clear_legacy_test.go, internal/cmd/config/config.go, internal/cmd/config/config_test.go, internal/cmd/contacts/contacts_test.go, internal/cmd/contacts/get.go, internal/cmd/contacts/group_manage.go, internal/cmd/contacts/group_manage_test.go, internal/cmd/contacts/groups.go, internal/cmd/contacts/handlers_test.go, internal/cmd/contacts/list.go, internal/cmd/contacts/output.go, internal/cmd/contacts/output_test.go, internal/cmd/contacts/search.go, internal/cmd/contacts/star.go, internal/cmd/contacts/star_test.go, internal/cmd/drive/download.go, internal/cmd/drive/download_test.go, internal/cmd/drive/drives.go, internal/cmd/drive/drives_test.go, internal/cmd/drive/get.go, internal/cmd/drive/get_test.go, internal/cmd/drive/handlers_test.go, internal/cmd/drive/list.go, internal/cmd/drive/list_test.go, internal/cmd/drive/mock_test.go, internal/cmd/drive/output.go, internal/cmd/drive/search_test.go, internal/cmd/drive/star.go, internal/cmd/drive/star_test.go, internal/cmd/drive/tree.go, internal/cmd/drive/tree_test.go, internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go, internal/cmd/mail/archive.go, internal/cmd/mail/archive_test.go, internal/cmd/mail/attachments.go, internal/cmd/mail/attachments_download.go, internal/cmd/mail/attachments_download_test.go, internal/cmd/mail/attachments_list.go, internal/cmd/mail/attachments_test.go, internal/cmd/mail/categorize.go, internal/cmd/mail/categorize_test.go, internal/cmd/mail/draft.go, internal/cmd/mail/draft_quote_test.go, internal/cmd/mail/draft_test.go, internal/cmd/mail/handlers_test.go, internal/cmd/mail/label.go, internal/cmd/mail/label_test.go, internal/cmd/mail/labels.go, internal/cmd/mail/labels_test.go, internal/cmd/mail/mail.go, internal/cmd/mail/mail_test.go, internal/cmd/mail/markread.go, internal/cmd/mail/markread_test.go, internal/cmd/mail/mock_test.go, internal/cmd/mail/output.go, internal/cmd/mail/output_test.go, internal/cmd/mail/read.go, internal/cmd/mail/read_test.go, internal/cmd/mail/sanitize.go, internal/cmd/mail/sanitize_test.go, internal/cmd/mail/search.go, internal/cmd/mail/search_test.go, internal/cmd/mail/star.go, internal/cmd/mail/star_test.go, internal/cmd/mail/thread.go, internal/cmd/mail/thread_test.go, internal/cmd/me/me_test.go, internal/cmd/me/output.go, internal/cmd/refreshcmd/refresh.go, internal/cmd/refreshcmd/refresh_test.go, internal/cmd/setcred/setcred.go, internal/cmd/setcred/setcred_test.go, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go, internal/contacts/client.go, internal/contacts/client_test.go, internal/contacts/contacts.go, internal/contacts/contacts_test.go, internal/credtest/credtest.go, internal/drive/client.go, internal/drive/files.go, internal/drive/files_test.go, internal/errors/errors.go, internal/errors/errors_test.go, internal/format/format.go, internal/format/format_test.go, internal/gmail/attachments.go, internal/gmail/attachments_test.go, internal/gmail/client.go, internal/gmail/client_test.go, internal/gmail/drafts.go, internal/gmail/drafts_test.go, internal/gmail/messages.go, internal/gmail/messages_test.go, internal/keychain/credref_test.go, internal/keychain/keychain.go, internal/keychain/keychain_metadata_darwin_test.go, internal/keychain/keychain_test.go, internal/keychain/migrate.go, internal/keychain/passphrase.go, internal/keychain/token_source.go, internal/keychain/token_source_test.go, internal/keychain/wire.go, internal/keychain/wire_test.go, internal/log/log.go, internal/log/log_test.go, internal/migrationsink/migrationsink.go, internal/output/migration.go, internal/output/output.go, internal/output/output_test.go, internal/people/client.go, internal/people/client_test.go, internal/testutil/assert.go, internal/testutil/assert_test.go, internal/testutil/fixtures.go, internal/testutil/helpers.go, internal/version/version.go, internal/view/view.go, internal/view/view_test.go, internal/zip/extract.go, internal/zip/extract_test.go, internal/zip/fs.go | Deep-diffed only the files unique to this migration (appidentity, root wiring, main.go, architecture_test.go); the bulk of the assigned list is straight file deletion/relocation to google-cli-common with mechanical import-path updates, so those were spot-checked rather than line-by-line reviewed. |
| policies:conventions | incomplete_skipped | cmd/gro/main.go, docs/adding-a-domain.md, docs/architecture.md, docs/development.md, docs/golden-principles.md, go.mod, go.sum | internal/cmd/initcmd/init.go, internal/cmd/refreshcmd/refresh.go, internal/cmd/root/root.go, internal/cmd/setcred/setcred.go, internal/config/config.go, internal/config/relocate.go | Both unresolved threads (stale docs, structural non-destructive test) were resolved in the latest commit; verified directly against current file contents. |
| documentation:docs | complete_broad | docs/adding-a-domain.md, docs/architecture.md, docs/development.md, docs/golden-principles.md | unavailable | unavailable |
| structure:repo-health | complete_broad | cmd/gro/main.go, docs/development.md, go.mod, go.sum, internal/config/config.go, internal/config/config_test.go, internal/config/relocate.go, internal/config/relocate_test.go | unavailable | Scope limited to assigned files; the docs-drift and non-destructive-coverage-gap threads point at internal/appidentity/appidentity.go and internal/architecture/architecture_test.go, which are outside this assignment and were left to the reviewer(s) covering those files. |
| automation:ci-release | complete_broad | go.mod, go.sum | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 04s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, documentation:docs, structure:repo-health, automation:ci-release |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 5m 04s wall · 11m 07s compute |
| Cost | unavailable |
| Tokens | 122 in / 31.3k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 10.0k | 53.7k | 41.1k | unavailable | 1m 16s |
| go:implementation-tests | claude-sonnet-5 | 22 | 6.4k | 456.0k | 66.0k | unavailable | 3m 04s |
| policies:conventions | claude-sonnet-5 | 10 | 2.0k | 103.5k | 56.1k | unavailable | 1m 32s |
| documentation:docs | claude-sonnet-5 | 42 | 6.5k | 891.4k | 66.4k | unavailable | 1m 49s |
| structure:repo-health | claude-sonnet-5 | 22 | 2.8k | 346.3k | 53.1k | unavailable | 2m 17s |
| automation:ci-release | claude-sonnet-5 | 14 | 3.2k | 239.8k | 59.9k | unavailable | 56s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 525 | 73.1k | 78.6k | unavailable | 10s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Migrates gro onto the new
google-cli-commonshared module (v0.1.1), eliminating the duplication between gro and its new read-write siblinggrw.Moved into google-cli-common and now consumed as a dependency:
config,keychain,auth(OAuth flow),cache,log,version,view,errors,format,bulk,output,zip,testutil,credtest,migrationsinkgmail,calendar,contacts,drive,peoplemailcmd,initcmd,configcmd,setcred,refreshcmd,rootutilgro keeps only what's gro-specific: its domain commands (
calendar/contacts/drive/me), its read-onlymailcomposition,internal/appidentity(its identity + scopes),config.Registerinmain, andinternal/architecturestructural tests.Seam
Everything CLI-specific is funneled through
config.Identity(dir name, default credential ref, product name, scopes), registered once at startup.DirNamealone drives the config/cache dir, keyring service, and derived env vars.Non-destructive guarantee preserved
TestAllScopesAreNonDestructivenow assertsappidentity.Scopesstays on the non-destructive allowlist.TestNoDestructiveAPIMethodsInProductionCodestill scans gro's tree (no.Trash/.Send/.BatchDelete). The sharedgmailclient is non-destructive too; the destructive surface lives only in grw.Verification
GOWORK=off(as CI runs): build + 7 test packages +golangci-lint+ gofmt all green against the publishedgoogle-cli-common@v0.1.1.Behavior is unchanged; internal-only refactor (not a release).