Skip to content

Commit 1f472eb

Browse files
authored
feat(state): port gro to cli-common state components [MON-5371] (#134)
* feat(state): port gro to cli-common state components Adopt cli-common/statedir for the config-dir resolver (native per OS: $XDG_CONFIG_HOME→Linux, ~/Library/Application Support→macOS, %APPDATA%→Windows; relative $XDG_CONFIG_HOME now errors per the §1.1 intentional tightening). Make SaveConfig atomic via os.CreateTemp + rename at 0600 under a 0700 dir. Add a macOS/Windows config-relocation gate (copy-leave-old; fail loud on divergence; mutation-free runtime read-fallback via the new LoadConfigForRuntime wrapper used by non-init callers). The gate runs ahead of EnsureMigrated in `gro init` so divergent old/new can't be silently papered over. Whole-dir copy excludes token.json (a secret); the existing §1.8 token migrator gains the old-hand-rolled token path as an additional legacy candidate with full conflict semantics (equal→migrate+delete-both; divergent→fail loud). Replace internal/cache with a thin wrapper over cli-common/cache: atomic temp+rename envelope, version-mismatch-as-miss, hard-coded per-resource TTL (§4.4). The pre-MON-5371 `cache_ttl_hours` config field, `DefaultCacheTTLHours`, GetCacheTTL{,Hours}, the AskCacheTTL prompt and `gro config cache {show,clear,ttl}` subcommand are removed (older config.yml that still carries the field loads cleanly — yaml ignores unknowns; clearing the cache folds into `gro config clear --all`). Switch internal/credtest to delegate state-dir isolation to cli-common/statedirtest (full 7-var set) — closes a pre-existing Windows real-dir leak (AppData/USERPROFILE/XDG_DATA_HOME were missing). Tests that hand-built `<tmp>/xdgconfig/<DirName>/…` paths now resolve via the resolver so they're correct on every OS. §3.2 acceptance matrix coverage: 8 cases × 2 surfaces (config in internal/config/relocate_test.go; cache in internal/cache/cache_test.go). Init reconcile coverage: detect-runs-before-migrate, divergent-aborts- mutates-nothing, copy-needed-triggers-apply. Closes #131 [MON-5371] * fix(state): address Codex PR review blockers + TDD gap - relocate detector now treats `old/config.json` as a valid legacy source (was YAML-only); LoadConfig falls back to old/config.json when new/yml is absent. A pre-MON-5371 macOS/Windows user on the legacy JSON form is no longer treated as a fresh install. - configsMaterialEqual now compares `oauth_client_path` via oauthClientPathEquiv: each-side-at-its-own-default → equal (location artifact); explicit non-default divergence → fail loud. - migrateLegacyCacheDir now probes both the new statedir LegacyCacheDir and the old hand-rolled `<old>/google-readonly/cache/` (new export config.OldHandRolledLegacyCacheDir). Dedupes on Linux. - Replace the stubbed TestLoadConfigForRuntime_SoftConflict with two real assertions (soft-conflict returns canonical+nil-err; no-conflict pass-through) via a new testable seam. - Add TestMigrateTokenFile_OldHandRolledPath and TestMigrateTokenFile_OldAndNewDivergent for the new old-token enumeration; covers §1.8 equal-migrate and divergent-fail-loud paths. [MON-5371] * fix(state): malformed old-only config fails loud before any copy Codex caught: detectRelocation's oldPresent+!newPresent branch was returning CopyNeeded=true without parsing the old file, so init's ApplyConfigRelocation would carry a malformed YAML/JSON into the new dir before LoadConfig later choked on it — that's a §3.2 row-5 (malformed old) regression: the contract is fail loud, mutate nothing. Parse the old file in the old-only branch and surface ErrRelocationConflict ahead of any copy. Add TestRelocate_OldOnlyMalformedYAML_FailLoud_MutatesNothing and TestRelocate_OldOnlyMalformedJSON_FailLoud_MutatesNothing asserting the new dir is not created and CopyNeeded is false. [MON-5371] * fix(state): address pr-review-daemon findings - configsMaterialEqual now uses reflect.DeepEqual on the whole Keyring sub-struct, not just Backend, so any future KeyringConfig field is auto-covered as a divergence. - LoadConfig skips the new-dir re-parse and legacy-JSON re-read when relErr is already a detect-time ErrRelocationConflict (which includes the malformed-new-wrapped-in-conflict case). The wrapped conflict error wins, so LoadConfigForRuntime soft-degrades cleanly instead of hard-failing on a bare parse error. - Add cache_test subtest seeding a warm cache exclusively at the old hand-rolled legacy cache subdir and asserting the dual-probe carry + legacy-dir removal. - Delete configExistedBefore plumbing (variable + threading through tryExistingToken / finishExisting); the AskCacheTTL gate it guarded was removed in this same PR so the suppressor was dead code. [MON-5371] * fix(state): daemon round 2 — real regression + nits - LoadConfig: attempt new-dir read unconditionally so callers soft-degrading via LoadConfigForRuntime get the user's actual settings (OAuthClientPath, GrantedScopes, …) alongside ErrRelocationConflict instead of an empty Config. The relErr-set branch still suppresses bare parse-error propagation so the soft-degrade contract holds. - oauthClientPathEquiv: ExpandPath both sides before comparing so tilde vs fully-expanded forms don't false-positive as divergent. - Cache.Clear: scope to <Root>/<InstanceKey> instead of the tool-level Root so a future multi-instance Locator can't have one instance's Clear evict the others. [MON-5371] * fix(state): malformed config under conflict hard-fails at runtime Codex final-pass blocker: LoadConfig was suppressing canonical read failures whenever relErr was set, returning all-defaults cfg + nil through LoadConfigForRuntime. A malformed new config (under a both-present detect-time conflict) would therefore silently warn-and- default, swapping CredentialRef back to google-readonly/default for non-init commands and masking the corrupt file. LoadConfig now tracks whether a canonical config was actually read; if relErr is set AND no read succeeded, return (nil, relErr) instead of (empty cfg, relErr). LoadConfigForRuntime then only soft-degrades when cfg is non-nil, so the runtime command hard-fails on a malformed canonical config and the user sees the conflict instead of silent default swap. Added TestLoadConfigForRuntime_MalformedCanonicalUnderConflict_HardFails pinning the new contract via the testable seam (which now mirrors the real LoadConfig + LoadConfigForRuntime branching). [MON-5371]
1 parent 7eac0dc commit 1f472eb

23 files changed

Lines changed: 1615 additions & 969 deletions

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ require (
66
github.com/atotto/clipboard v0.1.4
77
github.com/charmbracelet/huh v0.8.0
88
github.com/charmbracelet/lipgloss v1.1.0
9-
github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14
9+
github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d
1010
github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c
1111
github.com/spf13/cobra v1.8.0
1212
github.com/yuin/goldmark v1.8.2

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,8 @@ github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIf
112112
github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc=
113113
github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk=
114114
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno=
115-
github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0=
116-
github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds=
115+
github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d h1:fiyfxc/Wvpuen/u6Mk3bCUG0DLyOylF0K+eol4y9C64=
116+
github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds=
117117
github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c h1:+mdjkGKdHQG3305AYmdv1U2eRNDiU2ErMBj1gwrq8eQ=
118118
github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c/go.mod h1:7rwL4CYBLnjLxUqIJNnCWiEdr3bn6IUYi15bNlnbCCU=
119119
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=

internal/auth/auth.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func CheckScopesMigration(grantedScopes []string) string {
8888
// OAuth client JSON referenced by config.yml's oauth_client_path (§1.2 — not
8989
// a secret; lives on disk, never the keyring), with all scopes.
9090
func GetOAuthConfig() (*oauth2.Config, error) {
91-
cfg, err := config.LoadConfig()
91+
cfg, err := config.LoadConfigForRuntime()
9292
if err != nil {
9393
return nil, err
9494
}

internal/cache/cache.go

Lines changed: 103 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1,196 +1,175 @@
1-
// Package cache provides TTL-based caching for API metadata.
1+
// Package cache wraps cli-common/cache for gro's Drive metadata cache.
2+
//
3+
// Per cli-common/docs/working-with-state.md §4, gro's cache is disposable
4+
// state at os.UserCacheDir()/google-readonly (via statedir.Cache). Writes are
5+
// atomic via cli-common/cache's temp+rename envelope; TTL is hard-coded per
6+
// resource (no user-configurable cache_ttl_hours — §4.4); reads classify a
7+
// version/identity mismatch as a miss so schema bumps self-heal. The pre-B2b
8+
// "<configdir>/cache/" relocation is retained for installs that pre-date the
9+
// B2b cache move.
210
package cache
311

412
import (
513
"encoding/json"
14+
"errors"
15+
"fmt"
616
"os"
717
"path/filepath"
818
"time"
919

20+
clicache "github.com/open-cli-collective/cli-common/cache"
21+
1022
"github.com/open-cli-collective/google-readonly/internal/config"
1123
)
1224

1325
const (
14-
// DefaultTTLHours is the default cache TTL if not configured
15-
DefaultTTLHours = 24
16-
// DrivesFile is the cache file for shared drives
26+
// DrivesFile is the cache file for shared drives. Retained as the legacy
27+
// pre-B2b file name so migrateLegacyCacheDir can find and relocate it.
1728
DrivesFile = "drives.json"
29+
// drivesResource is the cli-common cache resource name. The on-disk file
30+
// becomes <cachedir>/<instanceKey>/drives.json.
31+
drivesResource = "drives"
32+
// instanceKey is gro's single-instance discriminator (Locator requires
33+
// one). Per cli-common docs: single-instance CLIs use "default".
34+
instanceKey = "default"
35+
// drivesTTL is the §4.4 hard-coded per-resource TTL for shared drives —
36+
// same 24-hour default the user-configurable knob previously defaulted to.
37+
drivesTTL = "24h"
1838
)
1939

20-
// CachedDrive represents a cached shared drive entry
40+
// CachedDrive represents a cached shared drive entry. Public so callers
41+
// (drives.go) can populate it directly.
2142
type CachedDrive struct {
2243
ID string `json:"id"`
2344
Name string `json:"name"`
2445
}
2546

26-
// DriveCache represents the cached shared drives data
27-
type DriveCache struct {
28-
CachedAt time.Time `json:"cached_at"`
29-
TTLHours int `json:"ttl_hours"`
30-
Drives []*CachedDrive `json:"drives"`
31-
}
32-
33-
// Cache provides TTL-based caching for API metadata
47+
// Cache is gro's wrapper around the cli-common envelope cache.
3448
type Cache struct {
35-
dir string
36-
ttlHours int
49+
loc clicache.Locator
3750
}
3851

39-
// New creates a new Cache instance rooted at the OS cache dir (B2b). It also
40-
// runs a transparent, best-effort one-time relocation of a pre-B2b cache that
41-
// lived inside the config dir; relocation never fails New (the cache is
42-
// disposable — it simply repopulates).
43-
func New(ttlHours int) (*Cache, error) {
52+
// New creates a new Cache instance rooted at the OS cache dir (B2b via
53+
// cli-common/statedir). It also runs a transparent, best-effort one-time
54+
// relocation of a pre-B2b cache that lived inside the config dir; relocation
55+
// never fails New (the cache is disposable — it simply repopulates).
56+
func New() (*Cache, error) {
4457
cacheDir, err := config.GetCacheDir()
4558
if err != nil {
4659
return nil, err
4760
}
48-
4961
migrateLegacyCacheDir(cacheDir)
50-
51-
if ttlHours <= 0 {
52-
ttlHours = DefaultTTLHours
53-
}
54-
5562
return &Cache{
56-
dir: cacheDir,
57-
ttlHours: ttlHours,
63+
loc: clicache.Locator{Root: cacheDir, InstanceKey: instanceKey},
5864
}, nil
5965
}
6066

6167
// migrateLegacyCacheDir relocates a pre-B2b cache (a "cache" subdir of the
6268
// config dir) into newDir, then removes the legacy dir. Strictly silent and
6369
// best-effort: any failure is abandoned without touching New's result. If the
64-
// warm-cache carry fails the legacy dir is left intact (don't delete data we
65-
// could not carry); a stuck legacy dir is force-cleaned by `config clear
66-
// --all`. Idempotent: once the legacy dir is gone this is a single stat.
70+
// warm-cache carry fails the legacy dir is left intact; a stuck legacy dir is
71+
// force-cleaned by `config clear --all`. The carry is a byte copy and does
72+
// NOT parse the envelope; a stale shape arriving in newDir is harmless
73+
// because cli-common/cache's identity/version check + this package's
74+
// parse-error-mapped-to-miss demote it on the next read.
6775
func migrateLegacyCacheDir(newDir string) {
68-
legacy, err := config.LegacyCacheDir()
69-
if err != nil {
70-
return
76+
// Two possible legacy locations on macOS/Windows: (a) the pre-B2b
77+
// "cache" subdir under the NEW (statedir-resolved) config dir — what a
78+
// user reached if they already ran post-port code once — and (b) the
79+
// same subdir under the OLD hand-rolled config dir, which is where a
80+
// pure pre-MON-5371 install lived. Both are byte-carried best-effort;
81+
// any failure abandons the migration without touching New's result. On
82+
// Linux the two paths collapse, so the second probe is a stat-only no-op.
83+
probes := []func() (string, error){
84+
config.LegacyCacheDir,
85+
config.OldHandRolledLegacyCacheDir,
86+
}
87+
tried := map[string]bool{}
88+
for _, p := range probes {
89+
legacy, err := p()
90+
if err != nil || tried[legacy] {
91+
continue
92+
}
93+
tried[legacy] = true
94+
tryCarryLegacyCache(legacy, newDir)
7195
}
96+
}
97+
98+
func tryCarryLegacyCache(legacy, newDir string) {
7299
if _, err := os.Stat(legacy); err != nil {
73-
return // absent (steady state) or unreadable — nothing safe to do
100+
return
74101
}
75102

76103
legacyDrives := filepath.Join(legacy, DrivesFile)
77-
newDrives := filepath.Join(newDir, DrivesFile)
104+
newDrives := filepath.Join(newDir, instanceKey, DrivesFile)
78105
switch _, serr := os.Stat(newDrives); {
79106
case serr == nil:
80107
// New cache already present: nothing to carry, safe to drop legacy.
81108
case os.IsNotExist(serr):
82-
// New cache absent: try to carry the warm legacy file.
83-
data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // G304: path from config dir, not user input
109+
data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // path from legacy config dir
84110
switch {
85111
case rerr == nil:
86-
if werr := os.WriteFile(newDrives, data, config.TokenPerm); werr != nil { //nolint:gosec // G703: config-derived cache path, user's own disposable data
87-
return // carry failed: keep legacy intact, retry next run
112+
if mkErr := os.MkdirAll(filepath.Dir(newDrives), 0o700); mkErr != nil {
113+
return
114+
}
115+
if werr := os.WriteFile(newDrives, data, config.TokenPerm); werr != nil { //nolint:gosec // disposable cache
116+
return
88117
}
89118
case os.IsNotExist(rerr):
90119
// No warm file to carry — fine, fall through to cleanup.
91120
default:
92-
return // legacy drives file exists but unreadable: do NOT delete it
121+
return
93122
}
94123
default:
95-
return // ambiguous stat on the new path: do not risk deleting un-carried legacy
124+
return
96125
}
97-
_ = os.RemoveAll(legacy) // best-effort; cosmetic if it lingers
126+
_ = os.RemoveAll(legacy)
98127
}
99128

100-
// GetDrives returns cached shared drives, or nil if cache is stale or missing
129+
// GetDrives returns cached shared drives, or nil if cache is stale, missing,
130+
// or corrupt. Corrupt-as-miss preserves the pre-MON-5371 behavior: caches
131+
// are disposable, so a JSON parse error self-heals on the next API call. I/O
132+
// errors (read failure, permission denied) propagate.
101133
func (c *Cache) GetDrives() ([]*CachedDrive, error) {
102-
path := filepath.Join(c.dir, DrivesFile)
103-
104-
data, err := os.ReadFile(path) //nolint:gosec // Path constructed from known config directory
105-
if err != nil {
106-
if os.IsNotExist(err) {
107-
return nil, nil // Cache miss, not an error
108-
}
109-
return nil, err
110-
}
111-
112-
var cache DriveCache
113-
if err := json.Unmarshal(data, &cache); err != nil {
114-
// Corrupted cache, treat as miss
134+
env, err := clicache.ReadResource[[]*CachedDrive](c.loc, drivesResource)
135+
switch {
136+
case errors.Is(err, clicache.ErrCacheMiss):
115137
return nil, nil
138+
case err != nil:
139+
var syn *json.SyntaxError
140+
var ute *json.UnmarshalTypeError
141+
if errors.As(err, &syn) || errors.As(err, &ute) {
142+
return nil, nil // corrupt → miss (self-heals on next write)
143+
}
144+
return nil, fmt.Errorf("reading drives cache: %w", err)
116145
}
117146

118-
// Check if cache is stale
119-
ttl := time.Duration(cache.TTLHours) * time.Hour
120-
if time.Since(cache.CachedAt) > ttl {
121-
return nil, nil // Cache expired
147+
if clicache.Classify(env.FetchedAt, env.TTL, nowFn()) == clicache.StatusStale {
148+
return nil, nil // stale → miss
122149
}
123-
124-
return cache.Drives, nil
150+
return env.Data, nil
125151
}
126152

127-
// SetDrives updates the cached shared drives
153+
// SetDrives atomically writes the drives cache with the §4.4 hard-coded TTL.
128154
func (c *Cache) SetDrives(drives []*CachedDrive) error {
129-
cache := DriveCache{
130-
CachedAt: time.Now(),
131-
TTLHours: c.ttlHours,
132-
Drives: drives,
133-
}
134-
135-
data, err := json.MarshalIndent(cache, "", " ")
136-
if err != nil {
137-
return err
155+
if err := clicache.WriteResource(c.loc, drivesResource, drivesTTL, drives); err != nil {
156+
return fmt.Errorf("writing drives cache: %w", err)
138157
}
139-
140-
path := filepath.Join(c.dir, DrivesFile)
141-
return os.WriteFile(path, data, config.TokenPerm)
158+
return nil
142159
}
143160

144-
// Clear removes all cached data
161+
// Clear removes all cached data for this instance. Scoped to
162+
// <Root>/<InstanceKey> rather than the tool-level Root so a future move to a
163+
// multi-instance Locator can't have one instance's Clear() silently evict
164+
// every other instance's cache.
145165
func (c *Cache) Clear() error {
146-
return os.RemoveAll(c.dir)
147-
}
148-
149-
// Status returns information about the cache state
150-
type Status struct {
151-
Dir string `json:"dir"`
152-
TTLHours int `json:"ttl_hours"`
153-
DrivesCache *FileInfo `json:"drives_cache,omitempty"`
154-
}
155-
156-
// FileInfo contains information about a cache file
157-
type FileInfo struct {
158-
Path string `json:"path"`
159-
CachedAt time.Time `json:"cached_at"`
160-
ExpiresAt time.Time `json:"expires_at"`
161-
IsStale bool `json:"is_stale"`
162-
Count int `json:"count"`
163-
}
164-
165-
// GetStatus returns the current cache status
166-
func (c *Cache) GetStatus() (*Status, error) {
167-
status := &Status{
168-
Dir: c.dir,
169-
TTLHours: c.ttlHours,
170-
}
171-
172-
// Check drives cache
173-
drivesPath := filepath.Join(c.dir, DrivesFile)
174-
data, err := os.ReadFile(drivesPath) //nolint:gosec // Path constructed from known config directory
175-
if err == nil {
176-
var cache DriveCache
177-
if json.Unmarshal(data, &cache) == nil {
178-
ttl := time.Duration(cache.TTLHours) * time.Hour
179-
expiresAt := cache.CachedAt.Add(ttl)
180-
status.DrivesCache = &FileInfo{
181-
Path: drivesPath,
182-
CachedAt: cache.CachedAt,
183-
ExpiresAt: expiresAt,
184-
IsStale: time.Now().After(expiresAt),
185-
Count: len(cache.Drives),
186-
}
187-
}
188-
}
189-
190-
return status, nil
166+
return os.RemoveAll(filepath.Join(c.loc.Root, c.loc.InstanceKey))
191167
}
192168

193-
// GetDir returns the cache directory path
169+
// GetDir returns the cache directory path.
194170
func (c *Cache) GetDir() string {
195-
return c.dir
171+
return c.loc.Root
196172
}
173+
174+
// nowFn is a testing seam for cache classification.
175+
var nowFn = func() time.Time { return time.Now() }

0 commit comments

Comments
 (0)