feat(cache): add top-level refresh subcommand - #146
Conversation
Adds `gro refresh [resources...]` and `gro refresh --status` per cli-common/docs/working-with-state.md §4.6, making the cache control surface a single discoverable command rather than per-command flags. `gro drive drives --refresh` is marked deprecated and continues to work through one release cycle. `--status` reads the cache envelope directly via the new Cache.DrivesStatus() helper, never constructing a Drive client. The ClientFactory seam in refreshcmd pins this invariant via a panicking factory test. Closes #142
|
Findings
Checks
|
Address Codex PR review minors on #146: - README points users at gro refresh / gro refresh --status; marks --refresh deprecated. - New TestRefresh_Status_Stale exercises the stale classification path via cache.SetNowFnForTest. - New TestDrivesCommand_RefreshFlagStillForcesFetch executes `gro drive drives --refresh` against a primed cache and asserts the Drive API is hit (deprecation contract preserved).
TDD coverage assessmentOverall: solid coverage. The test seam ( CriticalNone. Important
Nit
Strong points
|
Direct unit coverage for the three Status return paths (uninitialized / corrupt-as-miss / fresh), addressing the TDD-review gap on PR #146. The corrupt-JSON branch in particular was unreachable from the refreshcmd-level tests since they always go through Set+Get.
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 45d3406
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 3 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (3 findings)
💡 Suggestion - internal/cache/cache.go:197
NowFnForTest and SetNowFnForTest are exported from a production file (cache.go), making them part of the package's compiled API. Any non-test code can call SetNowFnForTest and silently corrupt time-based classification. The idiomatic Go approach for cross-package test seams is an export_test.go file, or a clock interface field on Cache set via a constructor option. Without build-tag protection, these symbols are reachable from production paths.
💡 Suggestion - internal/cmd/refreshcmd/refresh.go:80
runStatus captures
now := time.Now().UTC()for the Age column directly, while DrivesStatus() uses the internal nowFn seam for status classification. In tests that advance nowFn (e.g. TestRefresh_Status_Stale advances clock by 48h), the status column correctly shows 'stale' but the Age column shows ~0s real elapsed time — the two columns are computed from different clocks in the same row. No production bug, but the test output is misleading and undermines the stale-status test's diagnostic value.
💡 Suggestion - internal/cmd/refreshcmd/refresh.go:98
The default branches in runStatus's switch and in runRefresh's switch are dead code. Because the command uses cobra.OnlyValidArgs with ValidArgs set to validResources, Cobra rejects any unrecognized argument before RunE is called; and the zero-args path fills selected from validResources. These branches create a false impression of internal validation logic and will mislead future contributors adding new resources.
harness-engineering:harness-enforcement-reviewer (2 findings)
internal/cmd/refreshcmd/refresh.go:47
gro refresh has RunE (making it a leaf command) but no --json/-j flag. CLAUDE.md and golden-principles.md require every leaf subcommand to support --json/-j, and this is CI-enforced by internal/architecture/architecture_test.go. This structural invariant must be satisfied before merging.
internal/cache/cache_test.go:130
The generic I/O error branch in DrivesStatus (cache.go:172 —
return fmt.Errorf("reading drives cache: %w", err)) has zero test coverage. The PR discussion explicitly flagged this as 'Important'. The corrupt-JSON and uninitialized paths are covered, but a permission-denied or missing-parent-directory error is never exercised. A test that writes a valid file and then makes it unreadable (chmod 000 or pointing Root at a non-existent path) would close this gap.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
README.md:466
README still documents
gro drive drives --refreshas the primary cache-refresh surface and does not mentiongro refresh drivesorgro refresh --status. New users following the README will land on the deprecated flag path. The PR discussion identifies this as an open issue. Update the README to presentgro refreshas the recommended command and markgro drive drives --refreshas deprecated.
💡 Suggestion - internal/cache/cache.go:177
DrivesStatus returns (StatusUninitialized, non-nil error) for generic I/O failures (permission denied, missing directory). A caller that checks status before error — or that logs both — will see StatusUninitialized and infer the cache was never populated, when the real cause is a permissions or disk error. Returning a zero-value status or a dedicated StatusError sentinel alongside the non-nil error would prevent silent misdiagnosis.
2 info-level observations excluded. Run with --verbose to include.
2 PR discussion threads considered.
Completed in 5m 41s | $1.26 | sonnet | daemon 0.2.121 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 5m 41s wall · 12m 57s compute (Reviewers: 3m 31s · Synthesis: 2m 07s) |
| Cost | $1.26 |
| Tokens | 252.6k in / 52.0k out |
| Turns | 11 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 31.5k | 8.7k | 0 | 31.5k (1h) | $0.26 |
| documentation:docs-reviewer | haiku | 5.7k | 7.5k | 0 | 5.7k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 45.8k | 4.5k | 25.2k | 20.6k (1h) | $0.16 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 46.4k | 7.1k | 25.2k | 21.2k (1h) | $0.20 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 54.8k | 13.3k | 25.2k | 29.6k (1h) | $0.33 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 47.3k | 7.3k | 24.7k | 22.6k (1h) | $0.21 |
| security:security-code-auditor | haiku | 21.1k | 3.6k | 0 | 21.1k (1h) | $0.05 |
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.
Address pr-review-daemon findings on #146: - Add --json/-j to gro refresh (control-plane envelope, per the gro repo's leaf-command rule + cli-common §2 retention of JSON for control-plane surfaces). - DrivesStatus() now returns (fetchedAt, ttl, status, now, error) so the --status presenter renders the Age column from the same clock that classified the status — no test-only dual-clock skew. - Move the cross-package clock seam behind internal/cache/cachetest (SwapClock) and rename the production-package symbol to SwapClockForTest with a stronger warning. Production callers cannot reach it via cachetest without an obvious import. - Add direct unit coverage for DrivesStatus's I/O-error branch (envelope path is a directory). - Drop dead default branches in runStatus / runRefresh (cobra rejects unknown resources before RunE via OnlyValidArgs). - Refresh failure path now wraps the underlying error (refreshing drives: listing shared drives: boom) so errors.Is can match through the chain. Adds 2 envelope-shape tests for --status --json and --json success.
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6bbc3b4 | Previous: 45d3406 (incremental)
Approved with 2 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-enforcement-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-enforcement-reviewer (1 findings)
💡 Suggestion - README.md:1112
The new
gro refreshFlags block in the README lists only--statusbut omits--json/-j, which was added to the command in this revision. Every other command's Flags block in the README documents-j, --json; thegro refreshentry should match. The flag exists in the code — this is a documentation-only gap in the newly added section.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - internal/cmd/refreshcmd/refresh.go:171
runRefresh now calls refreshDrives unconditionally for every resource in
selected, usingnameonly to populateentry.Resource— it has no effect on which operation executes. This is correct whilevalidResourcescontains only"drives", but when a second resource is added,gro refresh <new-resource>will silently call the drives refresh function instead of the new one. The prior version'sswitch name { case "drives": }made dispatch explicit and safe by construction; the fix for the deaddefaultbranch should have dropped only that default while preserving thecase "drives":dispatch.runStatusat least carries a comment explaining the single-resource assumption;runRefreshhas neither the guard nor the comment.
9 PR discussion threads considered.
Completed in 4m 40s | $1.32 | sonnet | daemon 0.2.121 | 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 | 4m 40s wall · 10m 09s compute (Reviewers: 3m 06s · Synthesis: 1m 03s) |
| Cost | $1.32 |
| Tokens | 335.5k in / 37.2k out |
| Turns | 8 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 45.1k | 3.3k | 13.2k | 31.9k (1h) | $0.17 |
| documentation:docs-reviewer | haiku | 14.9k | 3.5k | 5.7k | 9.3k (1h) | $0.03 |
| harness-engineering:harness-architecture-reviewer | sonnet | 37.6k | 4.8k | 4.0k | 33.6k (1h) | $0.20 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 40.3k | 4.1k | 4.0k | 36.2k (1h) | $0.20 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 99.7k | 7.2k | 50.4k | 49.2k (1h) | $0.31 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 39.4k | 11.0k | 4.0k | 35.3k (1h) | $0.30 |
| security:security-code-auditor | haiku | 36.3k | 2.6k | 0 | 36.3k (1h) | $0.06 |
| discussion-summarizer | — | 22.1k | 753 | 13.2k | 8.9k (1h) | $0.05 |
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.
| Usage: gro refresh [resources...] [flags] | ||
|
|
||
| Flags: | ||
| --status Print cache freshness; no network calls |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): The new gro refresh Flags block in the README lists only --status but omits --json/-j, which was added to the command in this revision. Every other command's Flags block in the README documents -j, --json; the gro refresh entry should match. The flag exists in the code — this is a documentation-only gap in the newly added section.
Reply to this thread when addressed.
|
|
||
| entries := make([]refreshEntry, 0, len(selected)) | ||
| var firstErr error | ||
| for _, name := range selected { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): runRefresh now calls refreshDrives unconditionally for every resource in selected, using name only to populate entry.Resource — it has no effect on which operation executes. This is correct while validResources contains only "drives", but when a second resource is added, gro refresh <new-resource> will silently call the drives refresh function instead of the new one. The prior version's switch name { case "drives": } made dispatch explicit and safe by construction; the fix for the dead default branch should have dropped only that default while preserving the case "drives": dispatch. runStatus at least carries a comment explaining the single-resource assumption; runRefresh has neither the guard nor the comment.
Reply to this thread when addressed.
|
Findings
The ClientFactory seam still proves |
Address final Codex pass on #146: - gro refresh --json now uses internal/output.JSON, so the §1.8 _migration block splices into the envelope rather than leaking to stderr (matters when refresh constructs the Drive client and the keychain migration runs). - Remove the cross-package cachetest helper and the production-exported SwapClockForTest seam. The stale-classification case is now covered by an in-package cache_test.go subtest that mutates nowFn directly — no production-package test exports. - Pointer time.Time fields (FetchedAt, UpdatedAt) in the envelope so omitempty actually omits unset timestamps instead of serializing the Go zero value. - README's gro refresh flag block now lists -j, --json.
|
Findings
Checks
|
* feat!: remove per-command --json from resource leaves JSON is reserved for round-trip payloads and control-plane envelopes per cli-common docs/output-and-rendering.md §2. Today's surviving JSON surfaces are 'gro refresh --json' (§4.6, added in #146) and 'gro config show --json' (diagnostic). Every resource-surface leaf under mail/, calendar/, contacts/, drive/, me/ now emits text only; running 'gro mail search foo --json' returns cobra's 'unknown flag: --json'. The boundary is uniform across resource reads AND mutation-success renderers — mail archive/label/markread/star/categorize/draft, calendar color/rsvp, contacts group_manage/star, drive star all lose their --json flag along with the read commands. Architecture: inverts golden principle #4 + the structural test that enforced it. Pre-#144: every leaf had to declare --json/-j (with two binary-write exemptions). Post-#144: every resource leaf must NOT declare --json — caught by TestResourceLeavesHaveNoJSONFlag. The jsonExemptCommands allowlist is gone (empty after the inversion). Helper cleanup: bulk.Result.Print(jsonOutput bool) → Print() (text only); the four package-local printJSON helpers in mail/calendar/contacts/drive output.go are deleted; me/output.go's RenderJSON + json-shaped structs are deleted. shared internal/output helpers stay (refresh + config show consume them). Tests: per-package 'has json flag' subtests, every Test*_JSON function, and noleak's me --json variants are removed; config show --json no-leak coverage stays. BREAKING CHANGE: 'gro <resource> --json' now errors. Scripts that piped 'gro ... --json | jq' must move to text parsing or use one of the surviving JSON surfaces (gro refresh --json, gro config show --json). Closes #144 * docs(adding-a-domain): align with #144 — text-only resource leaves The pre-#144 instructions required output.go to contain a printJSON() helper and every leaf to declare --json/-j. Both are inverted by the new policy: resource-surface leaves emit text only; JSON is reserved for control-plane envelopes outside the domain resource packages (refresh, config show). Updated the section so a future domain author follows the §4 rule rather than the removed surface. * docs: drop stale printJSON() references after #144 Two doc tails referenced the removed printJSON() helper: - docs/architecture.md file-naming table - internal/output/output.go's JSON() docstring referring to 'per-domain printJSON() callers' (those are gone; the only surviving JSONStdout callers are control-plane refresh + config show). * test: end-to-end resource --json rejection + config show --json carve-out Two TDD-suggested coverage adds: - TestResourceLeaf_RejectsJSON_EndToEnd dispatches 'gro drive drives --json' through cobra and asserts the user-visible 'unknown flag' error. The static guard catches missing flag declarations; this one catches a hypothetical regression where the flag is declared but the structural test's domain set is bypassed. - TestConfigShowCommand: assert the --json control-plane flag is declared with -j shorthand. config show --json had no-leak coverage in internal/noleak but no shape coverage; this pins the carve-out surface itself. * docs+test: align stale references; consolidate no-json assertion Five daemon findings on PR #147: 1. The repo's agent-guidance markdown §Key Constraints still required '--json on all leaf commands' — directly contradicts the post-#144 inversion and would mislead future contributors. Updated to mirror the new text-only rule. 2. internal/architecture E2E test comment overstated protection (claimed to catch a domain-set bypass when it shares the same list). Reworded as a spot-check complement that explicitly notes neither test guards against domainPackages drift. 3. docs/golden-principles.md §4 grew a 'carve-out criteria' paragraph articulating what qualifies a command as a control-plane carve-out (outside domain resource packages + envelope/diagnostic). 4. Removed the two duplicate per-command 'no json flag' subtests (drives_test.go, archive_test.go) for consistency — global guard in TestResourceLeavesHaveNoJSONFlag covers all leaves uniformly.
Summary
gro refresh [resources...]andgro refresh --statusper cli-common/docs/working-with-state.md §4.6gro drive drives --refreshdeprecated (one-release cycle); behavior preservedCache.DrivesStatus()helper lets--statusread freshness without a Drive client; theClientFactoryseam inrefreshcmdpins the no-network-on-status invariant via a panicking-factory testTest plan
go test ./...green (1412 tests)make lintgreenmake checkgreengro refresh --statuson empty cache printsuninitializedgro refresh fooerrors with valid-args list (exit 1)gro refresh --helpshows expected examplesCloses #142