From 9f0419d82d462c704c317d17e2e7e549641ee385 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 18:17:46 -0400 Subject: [PATCH 1/6] feat(state): port gro to cli-common state components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/xdgconfig//…` 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] --- go.mod | 2 +- go.sum | 4 +- internal/auth/auth.go | 2 +- internal/cache/cache.go | 200 +++++++---------- internal/cache/cache_test.go | 208 +++++------------ internal/cmd/config/behavior_test.go | 9 +- internal/cmd/config/cache.go | 150 ------------- internal/cmd/config/config.go | 3 +- internal/cmd/config/config_test.go | 117 +--------- internal/cmd/drive/drives.go | 7 +- internal/cmd/initcmd/init.go | 124 ++++------- internal/cmd/initcmd/init_test.go | 108 ++++++--- internal/cmd/me/me.go | 4 +- internal/cmd/me/me_test.go | 18 +- internal/config/config.go | 158 +++++++------ internal/config/config_test.go | 274 +++++++++-------------- internal/config/relocate.go | 284 ++++++++++++++++++++++++ internal/config/relocate_test.go | 320 +++++++++++++++++++++++++++ internal/credtest/credtest.go | 59 +++-- internal/keychain/keychain.go | 4 +- internal/keychain/keychain_test.go | 24 +- internal/keychain/migrate.go | 17 ++ internal/noleak/noleak_test.go | 12 +- 23 files changed, 1161 insertions(+), 947 deletions(-) delete mode 100644 internal/cmd/config/cache.go create mode 100644 internal/config/relocate.go create mode 100644 internal/config/relocate_test.go diff --git a/go.mod b/go.mod index 14c1f37..4e1c2ea 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/atotto/clipboard v0.1.4 github.com/charmbracelet/huh v0.8.0 github.com/charmbracelet/lipgloss v1.1.0 - github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 + github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/spf13/cobra v1.8.0 github.com/yuin/goldmark v1.8.2 diff --git a/go.sum b/go.sum index 6ff7542..91833ba 100644 --- a/go.sum +++ b/go.sum @@ -112,8 +112,8 @@ github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIf github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0= -github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d h1:fiyfxc/Wvpuen/u6Mk3bCUG0DLyOylF0K+eol4y9C64= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c h1:+mdjkGKdHQG3305AYmdv1U2eRNDiU2ErMBj1gwrq8eQ= github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c/go.mod h1:7rwL4CYBLnjLxUqIJNnCWiEdr3bn6IUYi15bNlnbCCU= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= diff --git a/internal/auth/auth.go b/internal/auth/auth.go index f542c7d..9b500db 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -88,7 +88,7 @@ func CheckScopesMigration(grantedScopes []string) string { // OAuth client JSON referenced by config.yml's oauth_client_path (§1.2 — not // a secret; lives on disk, never the keyring), with all scopes. func GetOAuthConfig() (*oauth2.Config, error) { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return nil, err } diff --git a/internal/cache/cache.go b/internal/cache/cache.go index b9dd2c2..d3dc55f 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -1,196 +1,154 @@ -// Package cache provides TTL-based caching for API metadata. +// Package cache wraps cli-common/cache for gro's Drive metadata cache. +// +// Per cli-common/docs/working-with-state.md §4, gro's cache is disposable +// state at os.UserCacheDir()/google-readonly (via statedir.Cache). Writes are +// atomic via cli-common/cache's temp+rename envelope; TTL is hard-coded per +// resource (no user-configurable cache_ttl_hours — §4.4); reads classify a +// version/identity mismatch as a miss so schema bumps self-heal. The pre-B2b +// "/cache/" relocation is retained for installs that pre-date the +// B2b cache move. package cache import ( "encoding/json" + "errors" + "fmt" "os" "path/filepath" "time" + clicache "github.com/open-cli-collective/cli-common/cache" + "github.com/open-cli-collective/google-readonly/internal/config" ) const ( - // DefaultTTLHours is the default cache TTL if not configured - DefaultTTLHours = 24 - // DrivesFile is the cache file for shared drives + // DrivesFile is the cache file for shared drives. Retained as the legacy + // pre-B2b file name so migrateLegacyCacheDir can find and relocate it. DrivesFile = "drives.json" + // drivesResource is the cli-common cache resource name. The on-disk file + // becomes //drives.json. + drivesResource = "drives" + // instanceKey is gro's single-instance discriminator (Locator requires + // one). Per cli-common docs: single-instance CLIs use "default". + instanceKey = "default" + // drivesTTL is the §4.4 hard-coded per-resource TTL for shared drives — + // same 24-hour default the user-configurable knob previously defaulted to. + drivesTTL = "24h" ) -// CachedDrive represents a cached shared drive entry +// CachedDrive represents a cached shared drive entry. Public so callers +// (drives.go) can populate it directly. type CachedDrive struct { ID string `json:"id"` Name string `json:"name"` } -// DriveCache represents the cached shared drives data -type DriveCache struct { - CachedAt time.Time `json:"cached_at"` - TTLHours int `json:"ttl_hours"` - Drives []*CachedDrive `json:"drives"` -} - -// Cache provides TTL-based caching for API metadata +// Cache is gro's wrapper around the cli-common envelope cache. type Cache struct { - dir string - ttlHours int + loc clicache.Locator } -// New creates a new Cache instance rooted at the OS cache dir (B2b). It also -// runs a transparent, best-effort one-time relocation of a pre-B2b cache that -// lived inside the config dir; relocation never fails New (the cache is -// disposable — it simply repopulates). -func New(ttlHours int) (*Cache, error) { +// New creates a new Cache instance rooted at the OS cache dir (B2b via +// cli-common/statedir). It also runs a transparent, best-effort one-time +// relocation of a pre-B2b cache that lived inside the config dir; relocation +// never fails New (the cache is disposable — it simply repopulates). +func New() (*Cache, error) { cacheDir, err := config.GetCacheDir() if err != nil { return nil, err } - migrateLegacyCacheDir(cacheDir) - - if ttlHours <= 0 { - ttlHours = DefaultTTLHours - } - return &Cache{ - dir: cacheDir, - ttlHours: ttlHours, + loc: clicache.Locator{Root: cacheDir, InstanceKey: instanceKey}, }, nil } // migrateLegacyCacheDir relocates a pre-B2b cache (a "cache" subdir of the // config dir) into newDir, then removes the legacy dir. Strictly silent and // best-effort: any failure is abandoned without touching New's result. If the -// warm-cache carry fails the legacy dir is left intact (don't delete data we -// could not carry); a stuck legacy dir is force-cleaned by `config clear -// --all`. Idempotent: once the legacy dir is gone this is a single stat. +// warm-cache carry fails the legacy dir is left intact; a stuck legacy dir is +// force-cleaned by `config clear --all`. The carry is a byte copy and does +// NOT parse the envelope; a stale shape arriving in newDir is harmless +// because cli-common/cache's identity/version check + this package's +// parse-error-mapped-to-miss demote it on the next read. func migrateLegacyCacheDir(newDir string) { legacy, err := config.LegacyCacheDir() if err != nil { return } if _, err := os.Stat(legacy); err != nil { - return // absent (steady state) or unreadable — nothing safe to do + return } legacyDrives := filepath.Join(legacy, DrivesFile) - newDrives := filepath.Join(newDir, DrivesFile) + // New cache lives at //drives.json now. + newDrives := filepath.Join(newDir, instanceKey, DrivesFile) switch _, serr := os.Stat(newDrives); { case serr == nil: // New cache already present: nothing to carry, safe to drop legacy. case os.IsNotExist(serr): - // New cache absent: try to carry the warm legacy file. - data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // G304: path from config dir, not user input + data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // path from config dir switch { case rerr == nil: - if werr := os.WriteFile(newDrives, data, config.TokenPerm); werr != nil { //nolint:gosec // G703: config-derived cache path, user's own disposable data - return // carry failed: keep legacy intact, retry next run + if mkErr := os.MkdirAll(filepath.Dir(newDrives), 0o700); mkErr != nil { + return + } + if werr := os.WriteFile(newDrives, data, config.TokenPerm); werr != nil { //nolint:gosec // disposable cache + return } case os.IsNotExist(rerr): // No warm file to carry — fine, fall through to cleanup. default: - return // legacy drives file exists but unreadable: do NOT delete it + return } default: - return // ambiguous stat on the new path: do not risk deleting un-carried legacy + return } - _ = os.RemoveAll(legacy) // best-effort; cosmetic if it lingers + _ = os.RemoveAll(legacy) } -// GetDrives returns cached shared drives, or nil if cache is stale or missing +// GetDrives returns cached shared drives, or nil if cache is stale, missing, +// or corrupt. Corrupt-as-miss preserves the pre-MON-5371 behavior: caches +// are disposable, so a JSON parse error self-heals on the next API call. I/O +// errors (read failure, permission denied) propagate. func (c *Cache) GetDrives() ([]*CachedDrive, error) { - path := filepath.Join(c.dir, DrivesFile) - - data, err := os.ReadFile(path) //nolint:gosec // Path constructed from known config directory - if err != nil { - if os.IsNotExist(err) { - return nil, nil // Cache miss, not an error - } - return nil, err - } - - var cache DriveCache - if err := json.Unmarshal(data, &cache); err != nil { - // Corrupted cache, treat as miss + env, err := clicache.ReadResource[[]*CachedDrive](c.loc, drivesResource) + switch { + case errors.Is(err, clicache.ErrCacheMiss): return nil, nil + case err != nil: + var syn *json.SyntaxError + var ute *json.UnmarshalTypeError + if errors.As(err, &syn) || errors.As(err, &ute) { + return nil, nil // corrupt → miss (self-heals on next write) + } + return nil, fmt.Errorf("reading drives cache: %w", err) } - // Check if cache is stale - ttl := time.Duration(cache.TTLHours) * time.Hour - if time.Since(cache.CachedAt) > ttl { - return nil, nil // Cache expired + if clicache.Classify(env.FetchedAt, env.TTL, nowFn()) == clicache.StatusStale { + return nil, nil // stale → miss } - - return cache.Drives, nil + return env.Data, nil } -// SetDrives updates the cached shared drives +// SetDrives atomically writes the drives cache with the §4.4 hard-coded TTL. func (c *Cache) SetDrives(drives []*CachedDrive) error { - cache := DriveCache{ - CachedAt: time.Now(), - TTLHours: c.ttlHours, - Drives: drives, + if err := clicache.WriteResource(c.loc, drivesResource, drivesTTL, drives); err != nil { + return fmt.Errorf("writing drives cache: %w", err) } - - data, err := json.MarshalIndent(cache, "", " ") - if err != nil { - return err - } - - path := filepath.Join(c.dir, DrivesFile) - return os.WriteFile(path, data, config.TokenPerm) + return nil } -// Clear removes all cached data +// Clear removes all cached data. func (c *Cache) Clear() error { - return os.RemoveAll(c.dir) + return os.RemoveAll(c.loc.Root) } -// Status returns information about the cache state -type Status struct { - Dir string `json:"dir"` - TTLHours int `json:"ttl_hours"` - DrivesCache *FileInfo `json:"drives_cache,omitempty"` -} - -// FileInfo contains information about a cache file -type FileInfo struct { - Path string `json:"path"` - CachedAt time.Time `json:"cached_at"` - ExpiresAt time.Time `json:"expires_at"` - IsStale bool `json:"is_stale"` - Count int `json:"count"` -} - -// GetStatus returns the current cache status -func (c *Cache) GetStatus() (*Status, error) { - status := &Status{ - Dir: c.dir, - TTLHours: c.ttlHours, - } - - // Check drives cache - drivesPath := filepath.Join(c.dir, DrivesFile) - data, err := os.ReadFile(drivesPath) //nolint:gosec // Path constructed from known config directory - if err == nil { - var cache DriveCache - if json.Unmarshal(data, &cache) == nil { - ttl := time.Duration(cache.TTLHours) * time.Hour - expiresAt := cache.CachedAt.Add(ttl) - status.DrivesCache = &FileInfo{ - Path: drivesPath, - CachedAt: cache.CachedAt, - ExpiresAt: expiresAt, - IsStale: time.Now().After(expiresAt), - Count: len(cache.Drives), - } - } - } - - return status, nil -} - -// GetDir returns the cache directory path +// GetDir returns the cache directory path. func (c *Cache) GetDir() string { - return c.dir + return c.loc.Root } + +// nowFn is a testing seam for cache classification. +var nowFn = func() time.Time { return time.Now() } diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index c5b7f39..8a73556 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -1,60 +1,46 @@ package cache import ( - "encoding/json" "os" "path/filepath" "testing" "time" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/google-readonly/internal/config" "github.com/open-cli-collective/google-readonly/internal/testutil" ) -// hermetic isolates every cache path resolver from the developer's real -// dirs (also fixes a pre-existing pollution bug). Covers all OSes -// os.UserCacheDir / config dir resolution can consult: HOME (macOS -// ~/Library/Caches), XDG_CACHE_HOME (Linux/CI), LOCALAPPDATA (Windows), -// XDG_CONFIG_HOME (legacy-cache-dir resolution). +// hermetic isolates the §3.1 7-var env set so os.UserCacheDir / +// os.UserConfigDir resolve under a per-test temp root on every OS. func hermetic(t *testing.T) { t.Helper() - d := t.TempDir() - t.Setenv("HOME", d) - t.Setenv("XDG_CACHE_HOME", filepath.Join(d, "xcache")) - t.Setenv("LOCALAPPDATA", filepath.Join(d, "localappdata")) - t.Setenv("XDG_CONFIG_HOME", filepath.Join(d, "xconfig")) + statedirtest.Hermetic(t) } func TestNew(t *testing.T) { hermetic(t) - t.Run("creates cache with default TTL", func(t *testing.T) { - c, err := New(0) + t.Run("creates cache", func(t *testing.T) { + c, err := New() testutil.NoError(t, err) testutil.NotNil(t, c) - testutil.Equal(t, c.ttlHours, DefaultTTLHours) - defer c.Clear() - }) - - t.Run("creates cache with custom TTL", func(t *testing.T) { - c, err := New(12) - testutil.NoError(t, err) - testutil.Equal(t, c.ttlHours, 12) defer c.Clear() }) t.Run("creates cache directory", func(t *testing.T) { - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() - _, err = os.Stat(c.dir) + _, err = os.Stat(c.loc.Root) testutil.NoError(t, err) }) } func TestCache_GetSetDrives(t *testing.T) { hermetic(t) - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() @@ -85,49 +71,25 @@ func TestCache_GetSetDrives(t *testing.T) { func TestCache_Expiration(t *testing.T) { hermetic(t) - c, err := New(1) // 1 hour TTL + c, err := New() testutil.NoError(t, err) defer c.Clear() - t.Run("returns nil for expired cache", func(t *testing.T) { - // Write cache with expired timestamp - expiredCache := DriveCache{ - CachedAt: time.Now().Add(-2 * time.Hour), // 2 hours ago - TTLHours: 1, - Drives: []*CachedDrive{ - {ID: "drive1", Name: "Test"}, - }, - } - - data, err := json.Marshal(expiredCache) - testutil.NoError(t, err) + t.Run("classifies stale envelope as miss", func(t *testing.T) { + testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "x", Name: "y"}})) - path := filepath.Join(c.dir, DrivesFile) - err = os.WriteFile(path, data, 0600) - testutil.NoError(t, err) + // Pretend "now" is two days ahead of the envelope's FetchedAt (TTL is 24h). + origNow := nowFn + nowFn = func() time.Time { return time.Now().Add(48 * time.Hour) } + defer func() { nowFn = origNow }() drives, err := c.GetDrives() testutil.NoError(t, err) testutil.Nil(t, drives) }) - t.Run("returns drives for valid cache", func(t *testing.T) { - // Write fresh cache - freshCache := DriveCache{ - CachedAt: time.Now(), - TTLHours: 1, - Drives: []*CachedDrive{ - {ID: "drive1", Name: "Test"}, - }, - } - - data, err := json.Marshal(freshCache) - testutil.NoError(t, err) - - path := filepath.Join(c.dir, DrivesFile) - err = os.WriteFile(path, data, 0600) - testutil.NoError(t, err) - + t.Run("returns drives for fresh envelope", func(t *testing.T) { + testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "drive1", Name: "Test"}})) drives, err := c.GetDrives() testutil.NoError(t, err) testutil.Len(t, drives, 1) @@ -137,14 +99,30 @@ func TestCache_Expiration(t *testing.T) { func TestCache_CorruptedCache(t *testing.T) { hermetic(t) - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() - t.Run("returns nil for corrupted JSON", func(t *testing.T) { - path := filepath.Join(c.dir, DrivesFile) - err := os.WriteFile(path, []byte("not valid json"), 0600) + t.Run("malformed JSON treated as miss (disposable state)", func(t *testing.T) { + // Write a malformed envelope at the same path cli-common would use: + // //.json. + path := filepath.Join(c.loc.Root, c.loc.InstanceKey, drivesResource+".json") + testutil.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + testutil.NoError(t, os.WriteFile(path, []byte("not valid json"), 0o600)) + + drives, err := c.GetDrives() testutil.NoError(t, err) + testutil.Nil(t, drives) + }) + + t.Run("old pre-MON-5371 DriveCache shape treated as miss", func(t *testing.T) { + // cli-common envelope expects Version=1, Resource="drives", + // Instance="default"; the pre-MON-5371 shape supplied none of those, + // so ReadResource classifies it as ErrCacheMiss. + path := filepath.Join(c.loc.Root, c.loc.InstanceKey, drivesResource+".json") + testutil.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + oldShape := `{"cached_at":"2026-01-01T00:00:00Z","ttl_hours":24,"drives":[{"id":"d1","name":"x"}]}` + testutil.NoError(t, os.WriteFile(path, []byte(oldShape), 0o600)) drives, err := c.GetDrives() testutil.NoError(t, err) @@ -154,83 +132,22 @@ func TestCache_CorruptedCache(t *testing.T) { func TestCache_Clear(t *testing.T) { hermetic(t) - c, err := New(24) - testutil.NoError(t, err) - - // Add some data - err = c.SetDrives([]*CachedDrive{{ID: "test", Name: "Test"}}) - testutil.NoError(t, err) - - // Verify file exists - path := filepath.Join(c.dir, DrivesFile) - _, err = os.Stat(path) + c, err := New() testutil.NoError(t, err) - // Clear cache - err = c.Clear() - testutil.NoError(t, err) + testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "test", Name: "Test"}})) - // Verify directory is gone - _, err = os.Stat(c.dir) + testutil.NoError(t, c.Clear()) + _, err = os.Stat(c.loc.Root) testutil.True(t, os.IsNotExist(err)) } -func TestCache_GetStatus(t *testing.T) { - hermetic(t) - c, err := New(24) - testutil.NoError(t, err) - defer c.Clear() - - t.Run("returns status with no cache", func(t *testing.T) { - status, err := c.GetStatus() - testutil.NoError(t, err) - testutil.Equal(t, status.Dir, c.dir) - testutil.Equal(t, status.TTLHours, 24) - testutil.Nil(t, status.DrivesCache) - }) - - t.Run("returns status with drives cache", func(t *testing.T) { - err := c.SetDrives([]*CachedDrive{ - {ID: "drive1", Name: "Test1"}, - {ID: "drive2", Name: "Test2"}, - }) - testutil.NoError(t, err) - - status, err := c.GetStatus() - testutil.NoError(t, err) - testutil.NotNil(t, status.DrivesCache) - testutil.Equal(t, status.DrivesCache.Count, 2) - testutil.False(t, status.DrivesCache.IsStale) - testutil.True(t, status.DrivesCache.ExpiresAt.After(time.Now())) - }) - - t.Run("marks stale cache as stale", func(t *testing.T) { - // Write expired cache - expiredCache := DriveCache{ - CachedAt: time.Now().Add(-48 * time.Hour), - TTLHours: 24, - Drives: []*CachedDrive{{ID: "test", Name: "Test"}}, - } - data, _ := json.Marshal(expiredCache) - path := filepath.Join(c.dir, DrivesFile) - os.WriteFile(path, data, 0600) - - status, err := c.GetStatus() - testutil.NoError(t, err) - testutil.NotNil(t, status.DrivesCache) - testutil.True(t, status.DrivesCache.IsStale) - }) -} - func TestCache_GetDir(t *testing.T) { hermetic(t) - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() - // Assert the exact resolved path, not a substring: with no Windows - // \Cache suffix and macOS ~/Library/Caches there is no portable - // "cache" substring (Codex Minor-2). want, err := config.CacheDirPath() testutil.NoError(t, err) testutil.NotEmpty(t, c.GetDir()) @@ -238,9 +155,6 @@ func TestCache_GetDir(t *testing.T) { } func TestMigrateLegacyCacheDir(t *testing.T) { - // Each subtest fully isolates its own fixtures: a fresh hermetic env + - // freshly-resolved legacy/newDir, so there is no ordered-cleanup coupling - // between subtests. setup := func(t *testing.T) (legacy, newDir string) { t.Helper() hermetic(t) @@ -260,43 +174,42 @@ func TestMigrateLegacyCacheDir(t *testing.T) { legacy, newDir := setup(t) seedLegacy(t, legacy, `{"cached_at":"2026-01-01T00:00:00Z","ttl_hours":24,"drives":[{"id":"d1","name":"Eng"}]}`) - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() - got, err := os.ReadFile(filepath.Join(newDir, DrivesFile)) + got, err := os.ReadFile(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) testutil.NoError(t, err) testutil.Contains(t, string(got), `"d1"`) _, statErr := os.Stat(legacy) - testutil.True(t, os.IsNotExist(statErr)) // legacy removed + testutil.True(t, os.IsNotExist(statErr)) // Idempotent: a second New is a no-op and keeps the new cache. - c2, err := New(24) + c2, err := New() testutil.NoError(t, err) defer c2.Clear() - _, err = os.Stat(filepath.Join(newDir, DrivesFile)) + _, err = os.Stat(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) testutil.NoError(t, err) }) t.Run("does not overwrite an existing new cache; still removes legacy", func(t *testing.T) { legacy, _ := setup(t) - // New cache already warm with distinct content. - c, err := New(24) + c, err := New() testutil.NoError(t, err) defer c.Clear() testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "new", Name: "Keep"}})) seedLegacy(t, legacy, `{"drives":[{"id":"old","name":"Stale"}]}`) - c2, err := New(24) + c2, err := New() testutil.NoError(t, err) defer c2.Clear() drives, err := c2.GetDrives() testutil.NoError(t, err) testutil.Len(t, drives, 1) - testutil.Equal(t, drives[0].ID, "new") // not overwritten by legacy + testutil.Equal(t, drives[0].ID, "new") _, statErr := os.Stat(legacy) - testutil.True(t, os.IsNotExist(statErr)) // legacy still removed + testutil.True(t, os.IsNotExist(statErr)) }) t.Run("unreadable legacy drives file: legacy preserved, no partial carry", func(t *testing.T) { @@ -306,20 +219,21 @@ func TestMigrateLegacyCacheDir(t *testing.T) { // and nothing must be written into the new cache. testutil.NoError(t, os.MkdirAll(filepath.Join(legacy, DrivesFile), 0o700)) - c, err := New(24) - testutil.NoError(t, err) // migration never fails New + c, err := New() + testutil.NoError(t, err) defer c.Clear() _, statErr := os.Stat(legacy) - testutil.NoError(t, statErr) // legacy left intact (not deleted) - _, newErr := os.Stat(filepath.Join(newDir, DrivesFile)) - testutil.True(t, os.IsNotExist(newErr)) // no partial/corrupt carry + testutil.NoError(t, statErr) + _, newErr := os.Stat(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) + testutil.True(t, os.IsNotExist(newErr)) }) t.Run("no legacy is a clean no-op", func(t *testing.T) { setup(t) - c, err := New(24) - testutil.NoError(t, err) // migration never fails New + c, err := New() + testutil.NoError(t, err) defer c.Clear() }) + } diff --git a/internal/cmd/config/behavior_test.go b/internal/cmd/config/behavior_test.go index 3d13f84..0774b0e 100644 --- a/internal/cmd/config/behavior_test.go +++ b/internal/cmd/config/behavior_test.go @@ -45,11 +45,8 @@ func capture(t *testing.T, f func()) string { func seedTokenAndClient(t *testing.T) string { t.Helper() - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", appconfig.DirName) - if err := os.MkdirAll(dir, 0o700); err != nil { - t.Fatal(err) - } + credtest.Setup(t) + dir := credtest.ConfigDir(t) if err := os.WriteFile(filepath.Join(dir, appconfig.OAuthClientFile), []byte(clientJSON), 0o644); err != nil { t.Fatal(err) } @@ -165,7 +162,7 @@ func TestRunClearSemantics(t *testing.T) { // Seed both: a real new cache and a legacy dir whose drives.json is // itself a directory (a state the migration shim refuses to carry). // --all must remove BOTH directly (no cache.New / no migration). - c, err := cache.New(24) + c, err := cache.New() if err != nil { t.Fatalf("cache.New: %v", err) } diff --git a/internal/cmd/config/cache.go b/internal/cmd/config/cache.go deleted file mode 100644 index 716a35c..0000000 --- a/internal/cmd/config/cache.go +++ /dev/null @@ -1,150 +0,0 @@ -package config - -import ( - "fmt" - "strconv" - - "github.com/spf13/cobra" - - "github.com/open-cli-collective/google-readonly/internal/cache" - configpkg "github.com/open-cli-collective/google-readonly/internal/config" - "github.com/open-cli-collective/google-readonly/internal/output" -) - -func newCacheCommand() *cobra.Command { - cmd := &cobra.Command{ - Use: "cache", - Short: "Manage cache settings", - Long: `Manage cache settings for Drive metadata. - -gro caches Drive metadata (like shared drive lists) to speed up repeated commands. -The cache TTL is configured during 'gro init' (default: 24 hours).`, - } - - cmd.AddCommand(newCacheShowCommand()) - cmd.AddCommand(newCacheClearCommand()) - cmd.AddCommand(newCacheTTLCommand()) - - return cmd -} - -func newCacheShowCommand() *cobra.Command { - var jsonOutput bool - - cmd := &cobra.Command{ - Use: "show", - Short: "Display cache status", - Long: `Display the current cache status including: -- Cache directory location -- Configured TTL -- Cached data status (when last updated, expiration)`, - Args: cobra.NoArgs, - RunE: func(_ *cobra.Command, _ []string) error { - cfg, err := configpkg.LoadConfig() - if err != nil { - return fmt.Errorf("loading config: %w", err) - } - - c, err := cache.New(cfg.CacheTTLHours) - if err != nil { - return fmt.Errorf("initializing cache: %w", err) - } - - status, err := c.GetStatus() - if err != nil { - return fmt.Errorf("getting cache status: %w", err) - } - - if jsonOutput { - return output.JSONStdout(status) - } - - fmt.Printf("Cache directory: %s\n", configpkg.ShortenPath(status.Dir)) - fmt.Printf("Cache TTL: %d hours\n", status.TTLHours) - fmt.Println() - - if status.DrivesCache != nil { - fmt.Println("Shared Drives Cache:") - fmt.Printf(" File: %s\n", configpkg.ShortenPath(status.DrivesCache.Path)) - fmt.Printf(" Cached: %s\n", status.DrivesCache.CachedAt.Local().Format("2006-01-02 15:04:05")) - fmt.Printf(" Expires: %s\n", status.DrivesCache.ExpiresAt.Local().Format("2006-01-02 15:04:05")) - if status.DrivesCache.IsStale { - fmt.Printf(" Status: Stale (will refresh on next use)\n") - } else { - fmt.Printf(" Status: Valid (%d drives cached)\n", status.DrivesCache.Count) - } - } else { - fmt.Println("Shared Drives Cache: Not populated") - } - - return nil - }, - } - - cmd.Flags().BoolVarP(&jsonOutput, "json", "j", false, "Output as JSON") - - return cmd -} - -func newCacheClearCommand() *cobra.Command { - return &cobra.Command{ - Use: "clear", - Short: "Clear all cached data", - Long: `Remove all cached data. Cache will be repopulated on next use.`, - Args: cobra.NoArgs, - RunE: func(_ *cobra.Command, _ []string) error { - cfg, err := configpkg.LoadConfig() - if err != nil { - return fmt.Errorf("loading config: %w", err) - } - - c, err := cache.New(cfg.CacheTTLHours) - if err != nil { - return fmt.Errorf("initializing cache: %w", err) - } - - if err := c.Clear(); err != nil { - return fmt.Errorf("clearing cache: %w", err) - } - - fmt.Println("Cache cleared.") - return nil - }, - } -} - -func newCacheTTLCommand() *cobra.Command { - return &cobra.Command{ - Use: "ttl ", - Short: "Set cache TTL", - Long: `Set the cache time-to-live in hours. - -This affects how long cached data (like shared drive lists) is considered valid. -After the TTL expires, data will be refreshed from the API on next use. - -Examples: - gro config cache ttl 12 # Set TTL to 12 hours - gro config cache ttl 48 # Set TTL to 48 hours`, - Args: cobra.ExactArgs(1), - RunE: func(_ *cobra.Command, args []string) error { - ttl, err := strconv.Atoi(args[0]) - if err != nil || ttl <= 0 { - return fmt.Errorf("invalid TTL value: must be a positive integer (hours)") - } - - cfg, err := configpkg.LoadConfig() - if err != nil { - return fmt.Errorf("loading config: %w", err) - } - - cfg.CacheTTLHours = ttl - - if err := configpkg.SaveConfig(cfg); err != nil { - return fmt.Errorf("saving config: %w", err) - } - - fmt.Printf("Cache TTL set to %d hours.\n", ttl) - return nil - }, - } -} diff --git a/internal/cmd/config/config.go b/internal/cmd/config/config.go index 16029d5..1f7b825 100644 --- a/internal/cmd/config/config.go +++ b/internal/cmd/config/config.go @@ -28,7 +28,6 @@ func NewCommand() *cobra.Command { cmd.AddCommand(newShowCommand()) cmd.AddCommand(newTestCommand()) cmd.AddCommand(newClearCommand()) - cmd.AddCommand(newCacheCommand()) return cmd } @@ -97,7 +96,7 @@ type showStatus struct { } func runShow(jsonOut, verbose bool) error { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return err } diff --git a/internal/cmd/config/config_test.go b/internal/cmd/config/config_test.go index 76a2670..fda3d59 100644 --- a/internal/cmd/config/config_test.go +++ b/internal/cmd/config/config_test.go @@ -19,7 +19,7 @@ func TestConfigCommand(t *testing.T) { t.Run("has subcommands", func(t *testing.T) { subcommands := cmd.Commands() - testutil.GreaterOrEqual(t, len(subcommands), 4) + testutil.GreaterOrEqual(t, len(subcommands), 3) var names []string for _, sub := range subcommands { @@ -28,7 +28,6 @@ func TestConfigCommand(t *testing.T) { testutil.SliceContains(t, names, "show") testutil.SliceContains(t, names, "test") testutil.SliceContains(t, names, "clear") - testutil.SliceContains(t, names, "cache") }) } @@ -104,117 +103,3 @@ func TestConfigClearCommand(t *testing.T) { testutil.Contains(t, cmd.Long, "token") }) } - -func TestCacheCommand(t *testing.T) { - cmd := newCacheCommand() - - t.Run("has correct use", func(t *testing.T) { - testutil.Equal(t, cmd.Use, "cache") - }) - - t.Run("has short description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Short) - }) - - t.Run("has long description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Long) - testutil.Contains(t, cmd.Long, "cache") - }) - - t.Run("has subcommands", func(t *testing.T) { - subcommands := cmd.Commands() - testutil.Equal(t, len(subcommands), 3) - - var names []string - for _, sub := range subcommands { - names = append(names, sub.Name()) - } - testutil.SliceContains(t, names, "show") - testutil.SliceContains(t, names, "clear") - testutil.SliceContains(t, names, "ttl") - }) -} - -func TestCacheShowCommand(t *testing.T) { - cmd := newCacheShowCommand() - - t.Run("has correct use", func(t *testing.T) { - testutil.Equal(t, cmd.Use, "show") - }) - - t.Run("requires no arguments", func(t *testing.T) { - err := cmd.Args(cmd, []string{}) - testutil.NoError(t, err) - - err = cmd.Args(cmd, []string{"extra"}) - testutil.Error(t, err) - }) - - t.Run("has short description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Short) - }) - - t.Run("has long description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Long) - testutil.Contains(t, cmd.Long, "cache") - }) - - t.Run("has json flag", func(t *testing.T) { - flag := cmd.Flags().Lookup("json") - testutil.NotNil(t, flag) - testutil.Equal(t, flag.Shorthand, "j") - testutil.Equal(t, flag.DefValue, "false") - }) -} - -func TestCacheClearCommand(t *testing.T) { - cmd := newCacheClearCommand() - - t.Run("has correct use", func(t *testing.T) { - testutil.Equal(t, cmd.Use, "clear") - }) - - t.Run("requires no arguments", func(t *testing.T) { - err := cmd.Args(cmd, []string{}) - testutil.NoError(t, err) - - err = cmd.Args(cmd, []string{"extra"}) - testutil.Error(t, err) - }) - - t.Run("has short description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Short) - }) - - t.Run("has long description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Long) - }) -} - -func TestCacheTTLCommand(t *testing.T) { - cmd := newCacheTTLCommand() - - t.Run("has correct use", func(t *testing.T) { - testutil.Equal(t, cmd.Use, "ttl ") - }) - - t.Run("requires exactly one argument", func(t *testing.T) { - err := cmd.Args(cmd, []string{}) - testutil.Error(t, err) - - err = cmd.Args(cmd, []string{"24"}) - testutil.NoError(t, err) - - err = cmd.Args(cmd, []string{"24", "extra"}) - testutil.Error(t, err) - }) - - t.Run("has short description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Short) - }) - - t.Run("has long description", func(t *testing.T) { - testutil.NotEmpty(t, cmd.Long) - testutil.Contains(t, cmd.Long, "TTL") - }) -} diff --git a/internal/cmd/drive/drives.go b/internal/cmd/drive/drives.go index 11d3f91..37bbb1a 100644 --- a/internal/cmd/drive/drives.go +++ b/internal/cmd/drive/drives.go @@ -10,7 +10,6 @@ import ( "github.com/spf13/cobra" "github.com/open-cli-collective/google-readonly/internal/cache" - "github.com/open-cli-collective/google-readonly/internal/config" "github.com/open-cli-collective/google-readonly/internal/drive" ) @@ -39,8 +38,7 @@ Examples: } // Initialize cache - ttl := config.GetCacheTTLHours() - c, err := cache.New(ttl) + c, err := cache.New() if err != nil { return fmt.Errorf("initializing cache: %w", err) } @@ -141,8 +139,7 @@ func resolveDriveScope(ctx context.Context, client DriveClient, myDrive bool, dr } // Try to resolve name via cache - ttl := config.GetCacheTTLHours() - c, err := cache.New(ttl) + c, err := cache.New() if err != nil { return drive.DriveScope{}, fmt.Errorf("initializing cache: %w", err) } diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index b723c1d..9824ea9 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -11,7 +11,6 @@ import ( "net/url" "os" "path/filepath" - "strconv" "strings" "github.com/atotto/clipboard" @@ -99,6 +98,14 @@ type initDeps struct { // Browser opener. OpenBrowser func(url string) error + // DetectConfigRelocation / ApplyConfigRelocation are the MON-5371 init + // relocation gate. Injected so parallel tests (which cannot t.Setenv the + // hermetic env) can stub them to a no-op; production wires them to the + // real config functions. Strict semantics: divergent old/new aborts init + // before any mutation. + DetectConfigRelocation func() (config.SharedRelocation, error) + ApplyConfigRelocation func(config.SharedRelocation) error + // EnsureMigrated runs/resolves the one-time §1.8 migration up front (a // legacy-vs-keyring conflict aborts init loudly). EnsureMigrated func() error @@ -139,7 +146,6 @@ type prompter interface { ConfirmOpenBrowser() (bool, error) PasteRedirectURL() (string, error) ConfirmReauth() (bool, error) - AskCacheTTL(defaultHours int) (int, error) } // defaultDeps wires up production collaborators. @@ -149,27 +155,29 @@ func defaultDeps() initDeps { // The OAuth client JSON is deployment material (§1.2): the wizard // writes it to oauth_client_path, not the legacy credentials.json. GetCredentialsPath: func() (string, error) { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return "", err } return config.ExpandPath(cfg.OAuthClientPath), nil }, - ReadFile: os.ReadFile, - WriteFile: os.WriteFile, - Chmod: os.Chmod, - Stat: os.Stat, - ClipboardSupported: func() bool { return !clipboard.Unsupported }, - ClipboardReadAll: clipboard.ReadAll, - OpenBrowser: browser.OpenURL, - EnsureMigrated: ensureMigrated, - HasStoredToken: storeHasToken, - SetToken: storeSetToken, - DeleteToken: storeDeleteToken, - GetStorageBackend: storeBackendLabel, - StdinReadAll: readAllStdin, - ExchangeAuthCode: auth.ExchangeAuthCode, - GetOAuthConfig: auth.GetOAuthConfig, + ReadFile: os.ReadFile, + WriteFile: os.WriteFile, + Chmod: os.Chmod, + Stat: os.Stat, + ClipboardSupported: func() bool { return !clipboard.Unsupported }, + ClipboardReadAll: clipboard.ReadAll, + OpenBrowser: browser.OpenURL, + DetectConfigRelocation: config.DetectConfigRelocation, + ApplyConfigRelocation: config.ApplyConfigRelocation, + EnsureMigrated: ensureMigrated, + HasStoredToken: storeHasToken, + SetToken: storeSetToken, + DeleteToken: storeDeleteToken, + GetStorageBackend: storeBackendLabel, + StdinReadAll: readAllStdin, + ExchangeAuthCode: auth.ExchangeAuthCode, + GetOAuthConfig: auth.GetOAuthConfig, GmailVerify: func(ctx context.Context) (string, error) { c, err := gmail.NewClient(ctx) if err != nil { @@ -188,7 +196,7 @@ func defaultDeps() initDeps { } return c.GetMe(ctx) }, - LoadConfig: config.LoadConfig, + LoadConfig: config.LoadConfigForRuntime, SaveConfig: config.SaveConfig, GetConfigPath: config.GetConfigPath, Prompter: huhPrompter{}, @@ -272,6 +280,25 @@ func readAllStdin() (string, error) { // runWith is the testable entry point for the wizard. NewCommand wraps it. func runWith(ctx context.Context, d initDeps, opts *initOptions) error { + // Step -1 (must precede the §1.8 migration): the MON-5371 config-dir + // relocation gate. If the old hand-rolled dir and the new statedir- + // resolved dir both exist with divergent settings, abort BEFORE + // EnsureMigrated runs — otherwise that path would soft-read one side via + // the runtime wrapper, migrate, then SaveConfig, papering over the + // conflict. On only-old we copy old→new (atomic per-file, skipping + // token.json which the §1.8 migrator handles). + if d.DetectConfigRelocation != nil { + reloc, err := d.DetectConfigRelocation() + if err != nil { + return fmt.Errorf("detecting config relocation: %w", err) + } + if reloc.CopyNeeded && d.ApplyConfigRelocation != nil { + if err := d.ApplyConfigRelocation(reloc); err != nil { + return fmt.Errorf("relocating config from %s to %s: %w", reloc.OldPath, reloc.NewPath, err) + } + } + } + // Step 0: run/resolve the one-time §1.8 migration up front. A legacy // token is migrated into the keyring (its original removed) before the // wizard decides anything; a legacy-vs-keyring conflict aborts loudly @@ -367,24 +394,13 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { // Step 5: persist granted scopes (creates config.json if missing). cfg, cfgErr := d.LoadConfig() if cfgErr != nil { - cfg = &config.Config{CacheTTLHours: config.DefaultCacheTTLHours} + cfg = &config.Config{} } cfg.GrantedScopes = auth.AllScopes if saveErr := d.SaveConfig(cfg); saveErr != nil { d.View.Error("Warning: saving granted scopes: %v", saveErr) } - - // Step 6: cache TTL prompt (only on first-run, per the snapshot). - if !configExistedBefore { - ttl, err := d.Prompter.AskCacheTTL(config.DefaultCacheTTLHours) - if err != nil { - return err - } - cfg.CacheTTLHours = ttl - if saveErr := d.SaveConfig(cfg); saveErr != nil { - d.View.Error("Warning: saving cache TTL: %v", saveErr) - } - } + _ = configExistedBefore // pre-MON-5371 snapshot-before-save gate for the AskCacheTTL prompt; the prompt is gone (TTL is hard-coded per resource, §4.4) so the snapshot has no remaining consumer. // Step 7: verify + render `gro me` one-liner. People failure is fatal — // init's success contract is that you can immediately run `gro me`. @@ -508,21 +524,7 @@ func finishExisting(d initDeps, configExistedBefore bool, profile *people.Profil mecmd.RenderOneLiner(d.View.Out, profile) } - if !configExistedBefore { - // Edge case: token in keychain but no config file. Ask for TTL only. - ttl, err := d.Prompter.AskCacheTTL(config.DefaultCacheTTLHours) - if err != nil { - return err - } - cfg, cfgErr := d.LoadConfig() - if cfgErr != nil { - cfg = &config.Config{} - } - cfg.CacheTTLHours = ttl - if saveErr := d.SaveConfig(cfg); saveErr != nil { - d.View.Error("Warning: saving config: %v", saveErr) - } - } + _ = configExistedBefore // pre-MON-5371 TTL prompt gate (now gone). return nil } @@ -806,34 +808,6 @@ func (huhPrompter) ConfirmReauth() (bool, error) { return ok, err } -func (huhPrompter) AskCacheTTL(defaultHours int) (int, error) { - var s string - err := huh.NewInput(). - Title("Cache TTL for Drive metadata (hours)"). - Placeholder(strconv.Itoa(defaultHours)). - Value(&s). - Validate(func(in string) error { - in = strings.TrimSpace(in) - if in == "" { - return nil - } - n, err := strconv.Atoi(in) - if err != nil || n <= 0 { - return errors.New("must be a positive integer") - } - return nil - }). - Run() - if err != nil { - return 0, err - } - s = strings.TrimSpace(s) - if s == "" { - return defaultHours, nil - } - return strconv.Atoi(s) -} - // validateOAuthJSON is exposed so tests can poke it directly. func validateOAuthJSON(s string) error { if strings.TrimSpace(s) == "" { diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index fd6f030..1585d31 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -143,7 +143,6 @@ type stubPrompter struct { openBrowser bool redirectURL string reauth bool - ttl int pasteJSONErr error filePathErr error @@ -183,13 +182,6 @@ func (s *stubPrompter) ConfirmReauth() (bool, error) { s.calls = append(s.calls, "reauth") return s.reauth, nil } -func (s *stubPrompter) AskCacheTTL(d int) (int, error) { - s.calls = append(s.calls, "ttl") - if s.ttl == 0 { - return d, nil - } - return s.ttl, nil -} // fakeFS captures filesystem interactions across writeCredentials and Stat. type fakeFS struct { @@ -537,7 +529,10 @@ func TestEnsureCredentialsTightensPermsOnOverwrite(t *testing.T) { } } -func TestRunWithFreshSetupSavesScopesAndAsksTTL(t *testing.T) { +func TestRunWithFreshSetupSavesScopesNoTTLPrompt(t *testing.T) { + // MON-5371: TTL is hard-coded per resource (§4.4); the AskCacheTTL prompt + // is gone. Init still saves granted scopes — exactly once now (no second + // save for TTL). t.Parallel() fs := newFakeFS() d := baseDeps(t, fs) @@ -552,24 +547,18 @@ func TestRunWithFreshSetupSavesScopesAndAsksTTL(t *testing.T) { cfgSeen = append(cfgSeen, &cp) return nil } - stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC", ttl: 12} + stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC"} d.Prompter = stub if err := runWith(context.Background(), d, &initOptions{credentialsFile: src}); err != nil { t.Fatalf("runWith: %v", err) } - // The TTL prompt should have been called even though we save scopes - // (which creates config.json in real life). The snapshot-before-save - // fix means it is gated on the pre-run state, not the live one. - if !contains(stub.calls, "ttl") { - t.Errorf("expected TTL prompt, calls=%v", stub.calls) - } - if len(cfgSeen) < 2 { - t.Fatalf("expected at least two config saves (scopes, then TTL), got %d", len(cfgSeen)) + if contains(stub.calls, "ttl") { + t.Errorf("TTL prompt must not be called post-MON-5371, calls=%v", stub.calls) } - if cfgSeen[len(cfgSeen)-1].CacheTTLHours != 12 { - t.Errorf("expected CacheTTLHours=12, got %d", cfgSeen[len(cfgSeen)-1].CacheTTLHours) + if len(cfgSeen) < 1 { + t.Fatalf("expected at least one config save (scopes), got %d", len(cfgSeen)) } } @@ -595,7 +584,7 @@ func TestRunWithExpiredTokenPromptsReauth(t *testing.T) { t.Fatal(err) } - stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC", reauth: true, ttl: 6} + stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC", reauth: true} d.Prompter = stub if err := runWith(context.Background(), d, &initOptions{credentialsFile: src}); err != nil { @@ -627,7 +616,6 @@ func TestRunWithConfirmOpenBrowserTrueInvokesOpener(t *testing.T) { pasteJSON: validOAuthJSON, openBrowser: true, redirectURL: "http://localhost/?code=ABC", - ttl: 24, } d.Prompter = stub @@ -675,7 +663,6 @@ func TestRunWithRecordedStaleScopesReauths(t *testing.T) { // Recorded scopes are deliberately incomplete — only gmail.modify. d.LoadConfig = func() (*config.Config, error) { return &config.Config{ - CacheTTLHours: 24, GrantedScopes: []string{"https://www.googleapis.com/auth/gmail.modify"}, }, nil } @@ -697,7 +684,7 @@ func TestRunWithRecordedStaleScopesReauths(t *testing.T) { return &oauth2.Token{AccessToken: "tok"}, nil } - stub := &stubPrompter{redirectURL: "http://localhost/?code=ABC", reauth: true, ttl: 24} + stub := &stubPrompter{redirectURL: "http://localhost/?code=ABC", reauth: true} d.Prompter = stub if err := runWith(context.Background(), d, &initOptions{}); err != nil { @@ -711,10 +698,9 @@ func TestRunWithRecordedStaleScopesReauths(t *testing.T) { if !contains(stub.calls, "redirect") { t.Errorf("expected fresh OAuth flow after re-auth, calls=%v", stub.calls) } - // Fresh OAuth should also re-ask for TTL on first-run (config didn't - // exist before — we used the override which doesn't actually write). - if !contains(stub.calls, "ttl") { - t.Errorf("expected TTL prompt after fresh OAuth, calls=%v", stub.calls) + // MON-5371: AskCacheTTL prompt is gone (TTL hard-coded per resource). + if contains(stub.calls, "ttl") { + t.Errorf("TTL prompt must not be called post-MON-5371, calls=%v", stub.calls) } } @@ -782,14 +768,13 @@ func TestRunWithExistingTokenNoVerifyStillCatchesStaleScopes(t *testing.T) { d.HasStoredToken = func() bool { return true } d.LoadConfig = func() (*config.Config, error) { return &config.Config{ - CacheTTLHours: 24, GrantedScopes: []string{"https://www.googleapis.com/auth/gmail.modify"}, }, nil } deleteCalled := false d.DeleteToken = func() error { deleteCalled = true; return nil } - stub := &stubPrompter{redirectURL: "http://localhost/?code=ABC", reauth: true, ttl: 24} + stub := &stubPrompter{redirectURL: "http://localhost/?code=ABC", reauth: true} d.Prompter = stub if err := runWith(context.Background(), d, &initOptions{noVerify: true}); err != nil { @@ -844,7 +829,7 @@ func TestRunWithFreshSetupPeopleFailureIsFatal(t *testing.T) { d.PeopleGetMe = func(_ context.Context) (*people.Profile, error) { return nil, &googleapi.Error{Code: 403, Message: "People API has not been used in project before"} } - stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC", ttl: 12} + stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC"} d.Prompter = stub err := runWith(context.Background(), d, &initOptions{credentialsFile: src}) @@ -899,6 +884,67 @@ func TestRunWith_AuthCodeStdin(t *testing.T) { } } +// TestRunWith_RelocationGateRunsBeforeMigrate proves the MON-5371 ordering +// invariant: DetectConfigRelocation runs ahead of EnsureMigrated, so a +// divergent old/new config aborts init before any keyring migration / token +// write / SaveConfig can paper over the conflict. +func TestRunWith_RelocationGateRunsBeforeMigrate(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + order := []string{} + d.DetectConfigRelocation = func() (config.SharedRelocation, error) { + order = append(order, "detect") + return config.SharedRelocation{}, errors.New("old/new diverge") + } + d.EnsureMigrated = func() error { order = append(order, "migrate"); return nil } + d.SetToken = func(_ *oauth2.Token) error { order = append(order, "set"); return nil } + d.SaveConfig = func(_ *config.Config) error { order = append(order, "save"); return nil } + + err := runWith(context.Background(), d, &initOptions{noVerify: true}) + if err == nil || !strings.Contains(err.Error(), "detecting config relocation") { + t.Fatalf("init must abort on relocation divergence, got %v", err) + } + if len(order) != 1 || order[0] != "detect" { + t.Fatalf("detect must run first; nothing else should run on conflict; order=%v", order) + } +} + +// TestRunWith_RelocationGate_CopyNeededTriggersApply proves that on relocOldOnly +// the init wizard calls ApplyConfigRelocation before EnsureMigrated. +func TestRunWith_RelocationGate_CopyNeededTriggersApply(t *testing.T) { + t.Parallel() + fs := newFakeFS() + d := baseDeps(t, fs) + srcDir := t.TempDir() + src := filepath.Join(srcDir, "downloaded.json") + if err := os.WriteFile(src, []byte(validOAuthJSON), 0644); err != nil { + t.Fatal(err) + } + order := []string{} + d.DetectConfigRelocation = func() (config.SharedRelocation, error) { + order = append(order, "detect") + return config.SharedRelocation{Kind: 1, OldPath: "/old", NewPath: "/new", CopyNeeded: true}, nil + } + d.ApplyConfigRelocation = func(_ config.SharedRelocation) error { + order = append(order, "apply") + return nil + } + originalMigrated := d.EnsureMigrated + d.EnsureMigrated = func() error { order = append(order, "migrate"); return originalMigrated() } + stub := &stubPrompter{credChoice: "paste", pasteJSON: validOAuthJSON, redirectURL: "http://localhost/?code=ABC"} + d.Prompter = stub + + if err := runWith(context.Background(), d, &initOptions{credentialsFile: src, noVerify: true}); err != nil { + t.Fatalf("runWith: %v", err) + } + + // detect → apply → migrate, in that exact order. + if len(order) < 3 || order[0] != "detect" || order[1] != "apply" || order[2] != "migrate" { + t.Fatalf("expected detect → apply → migrate, got %v", order) + } +} + // TestRunWith_EnsureMigratedRunsFirst proves init resolves the one-time // migration before any token write, and surfaces a §1.8 conflict as a hard // abort (not a silent fresh-write alongside a stale legacy original — the diff --git a/internal/cmd/me/me.go b/internal/cmd/me/me.go index 68ae5ce..bbfa828 100644 --- a/internal/cmd/me/me.go +++ b/internal/cmd/me/me.go @@ -68,7 +68,7 @@ Data comes from the People API people/me endpoint.`, func run(ctx context.Context, out, errOut io.Writer, idOnly, extended, jsonOutput bool) error { // Loud-and-early stale-scope check (only fires when scopes were recorded). - if cfg, err := config.LoadConfig(); err == nil { + if cfg, err := config.LoadConfigForRuntime(); err == nil { if msg := auth.CheckScopesMigration(cfg.GrantedScopes); msg != "" { _, _ = fmt.Fprintln(errOut, msg) return errReauth @@ -123,7 +123,7 @@ func run(ctx context.Context, out, errOut io.Writer, idOnly, extended, jsonOutpu // (no config file, or empty list) we return nil — claiming auth.AllScopes // would overstate what an older token actually consented to. func grantedScopes() []string { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return nil } diff --git a/internal/cmd/me/me_test.go b/internal/cmd/me/me_test.go index 93f291a..123a1ff 100644 --- a/internal/cmd/me/me_test.go +++ b/internal/cmd/me/me_test.go @@ -10,6 +10,8 @@ import ( "google.golang.org/api/googleapi" + "github.com/open-cli-collective/cli-common/statedirtest" + "github.com/open-cli-collective/google-readonly/internal/auth" "github.com/open-cli-collective/google-readonly/internal/config" "github.com/open-cli-collective/google-readonly/internal/people" @@ -250,7 +252,6 @@ func TestRunExtendedJSONEndToEnd(t *testing.T) { // surface reflects the recorded scopes, not auth.AllScopes (which // would overstate consent). if err := config.SaveConfig(&config.Config{ - CacheTTLHours: 24, GrantedScopes: auth.AllScopes, }); err != nil { t.Fatal(err) @@ -367,12 +368,16 @@ func TestNewCommandIDExtendedMutuallyExclusive(t *testing.T) { } } -// withConfigDir points config.LoadConfig at a clean tempdir so stale-scope -// state can be injected per-test via real config.json. +// withConfigDir isolates state-dir resolution (§3.1 7-var set) so config.LoadConfig +// resolves under a per-test temp root, then returns the resolved config dir +// (created) — tests plant config.json/yml there. func withConfigDir(t *testing.T) string { t.Helper() - dir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", dir) + statedirtest.Hermetic(t) + dir, err := config.GetConfigDir() + if err != nil { + t.Fatalf("GetConfigDir: %v", err) + } return dir } @@ -382,7 +387,6 @@ func TestRunStaleRecordedScopesTriggersReauthMessage(t *testing.T) { // Write a config.json with a deliberately incomplete scopes list — only // gmail.modify, missing People/Drive/etc. CheckScopesMigration should fire. cfg := &config.Config{ - CacheTTLHours: 24, GrantedScopes: []string{"https://www.googleapis.com/auth/gmail.modify"}, } if err := config.SaveConfig(cfg); err != nil { @@ -430,7 +434,7 @@ func TestRunEmptyGrantedScopesDoesNotShortCircuit(t *testing.T) { withConfigDir(t) // Config exists but granted_scopes is explicitly empty — same semantics // as missing-config per CheckScopesMigration's len-0 early return. - cfg := &config.Config{CacheTTLHours: 24, GrantedScopes: []string{}} + cfg := &config.Config{GrantedScopes: []string{}} if err := config.SaveConfig(cfg); err != nil { t.Fatal(err) } diff --git a/internal/config/config.go b/internal/config/config.go index 50e9b1e..c0b4f4e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,11 +12,12 @@ package config import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" - "time" + "github.com/open-cli-collective/cli-common/statedir" "gopkg.in/yaml.v3" ) @@ -43,9 +44,6 @@ const ( // credential_ref. Callers still resolve it via credstore.ParseRef — the // service/profile are never assumed structurally (§1.3). DefaultCredentialRef = "google-readonly/default" - - // DefaultCacheTTLHours is the default cache TTL in hours. - DefaultCacheTTLHours = 24 ) // File and directory permission constants for consistent security settings. @@ -66,6 +64,12 @@ const ( // Config is google-readonly's config.yml. Everything here is safe for an org // to ship via MDM (§1.2); none of it is an access secret. JSON tags are // retained so a legacy config.json is read transparently for one upgrade. +// +// The pre-MON-5371 `cache_ttl_hours` field is gone — cache TTL is now +// hard-coded per resource per cli-common/docs/working-with-state.md §4.4. An +// older config.yml that still contains `cache_ttl_hours: N` continues to +// load cleanly (yaml.v3 silently ignores unknown fields); the value is just +// inert post-port. type Config struct { // CredentialRef is the authoritative / keyring ref // (§1.3). Resolved via credstore.ParseRef; never hard-coded. @@ -74,8 +78,6 @@ type Config struct { // (deployment material). Stored expanded + absolute; `~` is display-only // via ShortenPath. An org may override the default location here. OAuthClientPath string `yaml:"oauth_client_path" json:"oauth_client_path,omitempty"` - // CacheTTLHours is preserved pre-existing tuning (not a secret). - CacheTTLHours int `yaml:"cache_ttl_hours" json:"cache_ttl_hours"` // GrantedScopes is preserved: detects when a token's scopes drift from // what init granted. Not a secret. GrantedScopes []string `yaml:"granted_scopes,omitempty" json:"granted_scopes,omitempty"` @@ -96,32 +98,23 @@ type KeyringConfig struct { // internal/cache. const legacyCacheSubdir = "cache" +// configScope is the cli-common state-scope for gro's config dir. The scope +// name is gro's `DirName`; the resolver returns the native per-OS user config +// dir (Linux $XDG_CONFIG_HOME or ~/.config; macOS ~/Library/Application +// Support; Windows %APPDATA%) plus that scope. A relative $XDG_CONFIG_HOME on +// Linux now yields an error (intentional tightening per +// cli-common/docs/working-with-state.md §1.1). +var configScope = statedir.Scope{Name: DirName} + // configDirPath resolves the configuration directory WITHOUT creating it. -// Uses $XDG_CONFIG_HOME if set, else ~/.config/google-readonly. Identical on -// Linux, macOS, and Windows — matches the released layout (no %APPDATA% -// branch), so config.yml sits beside the legacy files it supersedes. +// Delegated to cli-common/statedir so the per-OS dir is native everywhere. func configDirPath() (string, error) { - configHome := os.Getenv("XDG_CONFIG_HOME") - if configHome == "" { - home, err := os.UserHomeDir() - if err != nil { - return "", err - } - configHome = filepath.Join(home, ".config") - } - return filepath.Join(configHome, DirName), nil + return configScope.ConfigDir() } // GetConfigDir returns the configuration directory, creating it if needed. func GetConfigDir() (string, error) { - configDir, err := configDirPath() - if err != nil { - return "", err - } - if err := os.MkdirAll(configDir, DirPerm); err != nil { //nolint:gosec // not user-controlled - return "", err - } - return configDir, nil + return configScope.ConfigDirEnsured() } // CacheDirPath resolves the OS-designated cache directory WITHOUT creating it @@ -226,33 +219,65 @@ func ShortenPath(path string) string { return path } -// LoadConfig loads config.yml. If config.yml is absent but a legacy -// config.json exists, it is read transparently (the user keeps working); the -// one-time migration rewrites it as config.yml and removes the JSON. An -// absent config entirely yields defaults. Defaults are always applied. +// LoadConfig loads config.yml. The strict variant — used by `gro init`'s +// relocation gate and by tests. Returns ErrRelocationConflict (with a wrapped +// detail message) when both the old hand-rolled and new statedir-resolved +// dirs contain materially-different config.yml files; on conflict, the +// canonical new-dir config is still returned alongside the error so callers +// can choose to soft-degrade. Runtime call sites should use +// LoadConfigForRuntime instead — see relocate.go. +// +// If new/config.yml is absent but old-only is present (the MON-5371 +// macOS/Windows pre-init steady state), the old file is transparently read. +// If neither YAML is present, a legacy config.json is read once at the new +// dir (post-init); if that is also absent, defaults are returned. +// +// Defaults are always applied to the returned *Config. func LoadConfig() (*Config, error) { - cfg := &Config{} + relErr := error(nil) + reloc, derr := DetectConfigRelocation() + if derr != nil && errors.Is(derr, ErrRelocationConflict) { + // Both-present-divergent — still return the canonical new-dir cfg so + // callers can soft-degrade via LoadConfigForRuntime. + relErr = derr + } else if derr != nil && !errors.Is(derr, ErrRelocationConflict) { + return nil, derr + } - ymlPath, err := GetConfigPath() - if err != nil { - return nil, err + // Priority: new/config.yml → old/config.yml (only if new is absent) → + // legacy config.json at the new dir. + cfg := &Config{} + read := false + if reloc.NewPath != "" { + newYML := filepath.Join(reloc.NewPath, ConfigFileYAML) + if data, err := os.ReadFile(newYML); err == nil { //nolint:gosec // path from validated dir + if uerr := yaml.Unmarshal(data, cfg); uerr != nil { + return nil, fmt.Errorf("parse config %s: %w", newYML, uerr) + } + read = true + } else if !os.IsNotExist(err) { + return nil, fmt.Errorf("read config %s: %w", newYML, err) + } } - data, err := os.ReadFile(ymlPath) //nolint:gosec // path from config dir - switch { - case err == nil: - if uerr := yaml.Unmarshal(data, cfg); uerr != nil { - return nil, fmt.Errorf("parse config %s: %w", ymlPath, uerr) + if !read && reloc.Kind == relocOldOnly && reloc.OldPath != "" { + oldYML := filepath.Join(reloc.OldPath, ConfigFileYAML) + if data, err := os.ReadFile(oldYML); err == nil { //nolint:gosec // path from hand-rolled legacy dir + if uerr := yaml.Unmarshal(data, cfg); uerr != nil { + return nil, fmt.Errorf("parse config %s: %w", oldYML, uerr) + } + read = true + } else if !os.IsNotExist(err) { + return nil, fmt.Errorf("read config %s: %w", oldYML, err) } - case os.IsNotExist(err): + } + if !read { if jerr := loadLegacyJSON(cfg); jerr != nil { return nil, jerr } - default: - return nil, fmt.Errorf("read config %s: %w", ymlPath, err) } cfg.applyDefaults() - return cfg, nil + return cfg, relErr } // loadLegacyJSON reads a pre-migration config.json if present (absent is not @@ -286,13 +311,14 @@ func (c *Config) applyDefaults() { } else { c.OAuthClientPath = ExpandPath(c.OAuthClientPath) } - if c.CacheTTLHours <= 0 { - c.CacheTTLHours = DefaultCacheTTLHours - } } -// SaveConfig writes config.yml at 0600 under a 0700 directory. OAuthClientPath -// is persisted expanded + absolute so os.ReadFile never sees a literal ~. +// SaveConfig writes config.yml at 0600 under a 0700 directory using an atomic +// temp-file-in-same-dir → chmod 0600 → rename (§3 standard). The unique temp +// name from os.CreateTemp means a same-process concurrent save and a +// crash-leftover from a prior run can never collide; a hard-crash orphan tmp +// is harmless (never read as config). OAuthClientPath is persisted expanded + +// absolute so os.ReadFile never sees a literal ~. func SaveConfig(cfg *Config) error { dir, err := GetConfigDir() if err != nil { @@ -309,23 +335,29 @@ func SaveConfig(cfg *Config) error { if err != nil { return err } - return os.WriteFile(filepath.Join(dir, ConfigFileYAML), data, TokenPerm) -} -// GetCacheTTL returns the configured cache TTL duration. -func GetCacheTTL() time.Duration { - cfg, err := LoadConfig() + final := filepath.Join(dir, ConfigFileYAML) + tmp, err := os.CreateTemp(dir, "config-*.yml.tmp") if err != nil { - return time.Duration(DefaultCacheTTLHours) * time.Hour + return fmt.Errorf("creating temp config file: %w", err) } - return time.Duration(cfg.CacheTTLHours) * time.Hour -} - -// GetCacheTTLHours returns the configured cache TTL in hours. -func GetCacheTTLHours() int { - cfg, err := LoadConfig() - if err != nil { - return DefaultCacheTTLHours + tmpPath := tmp.Name() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return fmt.Errorf("writing temp config file: %w", err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("closing temp config file: %w", err) } - return cfg.CacheTTLHours + if err := os.Chmod(tmpPath, TokenPerm); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("setting config file mode: %w", err) + } + if err := os.Rename(tmpPath, final); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("finalizing config file: %w", err) + } + return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6e3181f..02b0358 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -5,23 +5,36 @@ import ( "path/filepath" "strings" "testing" - "time" + + "github.com/open-cli-collective/cli-common/statedirtest" ) +// hermeticConfig isolates the §3.1 7-var env set so os.UserConfigDir / +// os.UserCacheDir never resolve to the developer's real dirs. Helper-local so +// these in-package tests stay independent of the credtest leaf used by tests +// in sibling packages. +func hermeticConfig(t *testing.T) string { + t.Helper() + return statedirtest.Hermetic(t) +} + func TestGetConfigDir(t *testing.T) { - t.Run("uses XDG_CONFIG_HOME if set", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + t.Run("creates the resolved config dir", func(t *testing.T) { + hermeticConfig(t) + base, err := os.UserConfigDir() + if err != nil { + t.Fatalf("UserConfigDir: %v", err) + } + want := filepath.Join(base, DirName) dir, err := GetConfigDir() if err != nil { t.Fatalf("unexpected error: %v", err) } - if dir != filepath.Join(tmpDir, DirName) { - t.Errorf("got %v, want %v", dir, filepath.Join(tmpDir, DirName)) + if dir != want { + t.Errorf("got %v, want %v", dir, want) } - // Verify directory was created info, err := os.Stat(dir) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -31,24 +44,8 @@ func TestGetConfigDir(t *testing.T) { } }) - t.Run("uses ~/.config if XDG_CONFIG_HOME not set", func(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", "") - - dir, err := GetConfigDir() - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - home, _ := os.UserHomeDir() - expected := filepath.Join(home, ".config", DirName) - if dir != expected { - t.Errorf("got %v, want %v", dir, expected) - } - }) - t.Run("creates directory with correct permissions", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) dir, err := GetConfigDir() if err != nil { @@ -66,28 +63,30 @@ func TestGetConfigDir(t *testing.T) { } func TestGetCredentialsPath(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) + base, _ := os.UserConfigDir() path, err := GetCredentialsPath() if err != nil { t.Fatalf("unexpected error: %v", err) } - if path != filepath.Join(tmpDir, DirName, CredentialsFile) { - t.Errorf("got %v, want %v", path, filepath.Join(tmpDir, DirName, CredentialsFile)) + want := filepath.Join(base, DirName, CredentialsFile) + if path != want { + t.Errorf("got %v, want %v", path, want) } } func TestGetTokenPath(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) + base, _ := os.UserConfigDir() path, err := GetTokenPath() if err != nil { t.Fatalf("unexpected error: %v", err) } - if path != filepath.Join(tmpDir, DirName, TokenFile) { - t.Errorf("got %v, want %v", path, filepath.Join(tmpDir, DirName, TokenFile)) + want := filepath.Join(base, DirName, TokenFile) + if path != want { + t.Errorf("got %v, want %v", path, want) } } @@ -155,95 +154,65 @@ func TestConstants(t *testing.T) { if ConfigFile != "config.json" { t.Errorf("got %v, want %v", ConfigFile, "config.json") } - if DefaultCacheTTLHours != 24 { - t.Errorf("got %v, want %v", DefaultCacheTTLHours, 24) - } } func TestGetConfigPath(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) + base, _ := os.UserConfigDir() path, err := GetConfigPath() if err != nil { t.Fatalf("unexpected error: %v", err) } - if path != filepath.Join(tmpDir, DirName, ConfigFileYAML) { - t.Errorf("got %v, want %v", path, filepath.Join(tmpDir, DirName, ConfigFileYAML)) + want := filepath.Join(base, DirName, ConfigFileYAML) + if path != want { + t.Errorf("got %v, want %v", path, want) } } func TestLoadConfig(t *testing.T) { t.Run("returns default config when file does not exist", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) cfg, err := LoadConfig() if err != nil { t.Fatalf("unexpected error: %v", err) } - if cfg.CacheTTLHours != DefaultCacheTTLHours { - t.Errorf("got %v, want %v", cfg.CacheTTLHours, DefaultCacheTTLHours) + if cfg.CredentialRef != DefaultCredentialRef { + t.Errorf("got %v, want %v", cfg.CredentialRef, DefaultCredentialRef) } }) t.Run("config.yml wins over legacy config.json when both present", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - dir := filepath.Join(tmpDir, DirName) - if err := os.MkdirAll(dir, DirPerm); err != nil { + hermeticConfig(t) + dir, err := GetConfigDir() + if err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, ConfigFile), []byte(`{"cache_ttl_hours": 99}`), TokenPerm); err != nil { + if err := os.WriteFile(filepath.Join(dir, ConfigFile), []byte(`{"credential_ref":"google-readonly/legacy"}`), TokenPerm); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, ConfigFileYAML), []byte("cache_ttl_hours: 7\n"), TokenPerm); err != nil { + if err := os.WriteFile(filepath.Join(dir, ConfigFileYAML), []byte("credential_ref: google-readonly/yml\n"), TokenPerm); err != nil { t.Fatal(err) } cfg, err := LoadConfig() if err != nil { t.Fatalf("unexpected error: %v", err) } - if cfg.CacheTTLHours != 7 { - t.Errorf("config.yml must win: got %v, want 7", cfg.CacheTTLHours) + if cfg.CredentialRef != "google-readonly/yml" { + t.Errorf("config.yml must win: got %v, want google-readonly/yml", cfg.CredentialRef) } }) - t.Run("loads config from file", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - // Create config directory and file - configDir := filepath.Join(tmpDir, DirName) - if err := os.MkdirAll(configDir, DirPerm); err != nil { - t.Fatalf("unexpected error: %v", err) - } - - configData := `{"cache_ttl_hours": 48}` - if err := os.WriteFile(filepath.Join(configDir, ConfigFile), []byte(configData), TokenPerm); err != nil { - t.Fatalf("unexpected error: %v", err) - } - - cfg, err := LoadConfig() + t.Run("loads config from legacy JSON", func(t *testing.T) { + hermeticConfig(t) + dir, err := GetConfigDir() if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if cfg.CacheTTLHours != 48 { - t.Errorf("got %v, want %v", cfg.CacheTTLHours, 48) - } - }) - - t.Run("applies default for zero or negative TTL", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - configDir := filepath.Join(tmpDir, DirName) - if err := os.MkdirAll(configDir, DirPerm); err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatal(err) } - configData := `{"cache_ttl_hours": 0}` - if err := os.WriteFile(filepath.Join(configDir, ConfigFile), []byte(configData), TokenPerm); err != nil { + configData := `{"credential_ref":"google-readonly/from-json"}` + if err := os.WriteFile(filepath.Join(dir, ConfigFile), []byte(configData), TokenPerm); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -251,25 +220,23 @@ func TestLoadConfig(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if cfg.CacheTTLHours != DefaultCacheTTLHours { - t.Errorf("got %v, want %v", cfg.CacheTTLHours, DefaultCacheTTLHours) + if cfg.CredentialRef != "google-readonly/from-json" { + t.Errorf("got %v, want google-readonly/from-json", cfg.CredentialRef) } }) t.Run("returns error for invalid JSON", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - configDir := filepath.Join(tmpDir, DirName) - if err := os.MkdirAll(configDir, DirPerm); err != nil { - t.Fatalf("unexpected error: %v", err) + hermeticConfig(t) + dir, err := GetConfigDir() + if err != nil { + t.Fatal(err) } - if err := os.WriteFile(filepath.Join(configDir, ConfigFile), []byte("not json"), TokenPerm); err != nil { + if err := os.WriteFile(filepath.Join(dir, ConfigFile), []byte("not json"), TokenPerm); err != nil { t.Fatalf("unexpected error: %v", err) } - _, err := LoadConfig() + _, err = LoadConfig() if err == nil { t.Fatal("expected error, got nil") } @@ -278,124 +245,77 @@ func TestLoadConfig(t *testing.T) { func TestSaveConfig(t *testing.T) { t.Run("saves config to file", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) - cfg := &Config{CacheTTLHours: 12} + cfg := &Config{CredentialRef: "google-readonly/test"} err := SaveConfig(cfg) if err != nil { t.Fatalf("unexpected error: %v", err) } - // Verify file was created path, _ := GetConfigPath() data, err := os.ReadFile(path) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(string(data), "cache_ttl_hours: 12") { - t.Errorf("expected %q to contain %q", string(data), "cache_ttl_hours: 12") + if !strings.Contains(string(data), "credential_ref: google-readonly/test") { + t.Errorf("expected %q to contain credential_ref", string(data)) } }) t.Run("overwrites existing config", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) + hermeticConfig(t) - // Save initial config - cfg1 := &Config{CacheTTLHours: 12} + cfg1 := &Config{CredentialRef: "google-readonly/a"} if err := SaveConfig(cfg1); err != nil { t.Fatalf("unexpected error: %v", err) } - // Save new config - cfg2 := &Config{CacheTTLHours: 36} + cfg2 := &Config{CredentialRef: "google-readonly/b"} if err := SaveConfig(cfg2); err != nil { t.Fatalf("unexpected error: %v", err) } - // Verify new value loaded, err := LoadConfig() if err != nil { t.Fatalf("unexpected error: %v", err) } - if loaded.CacheTTLHours != 36 { - t.Errorf("got %v, want %v", loaded.CacheTTLHours, 36) + if loaded.CredentialRef != "google-readonly/b" { + t.Errorf("got %v, want google-readonly/b", loaded.CredentialRef) } }) -} - -func TestGetCacheTTL(t *testing.T) { - t.Run("returns configured TTL as duration", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - cfg := &Config{CacheTTLHours: 12} - if err := SaveConfig(cfg); err != nil { - t.Fatalf("unexpected error: %v", err) + t.Run("writes atomically with correct perms and no stale tmp", func(t *testing.T) { + hermeticConfig(t) + if err := SaveConfig(&Config{}); err != nil { + t.Fatalf("save: %v", err) } - - ttl := GetCacheTTL() - if ttl != 12*time.Hour { - t.Errorf("got %v, want %v", ttl, 12*time.Hour) + final, _ := GetConfigPath() + info, err := os.Stat(final) + if err != nil { + t.Fatalf("stat: %v", err) } - }) - - t.Run("returns default TTL when no config exists", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - ttl := GetCacheTTL() - if ttl != time.Duration(DefaultCacheTTLHours)*time.Hour { - t.Errorf("got %v, want %v", ttl, time.Duration(DefaultCacheTTLHours)*time.Hour) + if info.Mode().Perm() != TokenPerm { + t.Errorf("config file mode = %v, want %v", info.Mode().Perm(), TokenPerm) } - }) -} - -func TestGetCacheTTLHours(t *testing.T) { - t.Run("returns configured TTL in hours", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - cfg := &Config{CacheTTLHours: 48} - if err := SaveConfig(cfg); err != nil { - t.Fatalf("unexpected error: %v", err) - } - - hours := GetCacheTTLHours() - if hours != 48 { - t.Errorf("got %v, want %v", hours, 48) + // No stale *.tmp in the dir after a successful save. + dir := filepath.Dir(final) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("readdir: %v", err) } - }) - - t.Run("returns default TTL when no config exists", func(t *testing.T) { - tmpDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", tmpDir) - - hours := GetCacheTTLHours() - if hours != DefaultCacheTTLHours { - t.Errorf("got %v, want %v", hours, DefaultCacheTTLHours) + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("stale temp file left in %s: %s", dir, e.Name()) + } } }) } func TestCacheDirResolvers(t *testing.T) { - // Each subtest gets its OWN hermetic env + fresh temp dir: GetCacheDir is - // creating, so a shared env would let one subtest's mkdir defeat - // another's "non-creating" assertion. - hermeticCfg := func(t *testing.T) string { - t.Helper() - d := t.TempDir() - t.Setenv("HOME", d) - t.Setenv("XDG_CACHE_HOME", filepath.Join(d, "xcache")) - t.Setenv("LOCALAPPDATA", filepath.Join(d, "localappdata")) - t.Setenv("XDG_CONFIG_HOME", filepath.Join(d, "xconfig")) - return d - } - t.Run("GetCacheDir is under os.UserCacheDir()/DirName, not the config tree", func(t *testing.T) { - hermeticCfg(t) - base, err := os.UserCacheDir() // computed in-test — no hard-coded per-OS strings + hermeticConfig(t) + base, err := os.UserCacheDir() if err != nil { t.Fatalf("UserCacheDir: %v", err) } @@ -421,7 +341,7 @@ func TestCacheDirResolvers(t *testing.T) { }) t.Run("CacheDirPath and LegacyCacheDir are non-creating", func(t *testing.T) { - d := hermeticCfg(t) + hermeticConfig(t) cp, err := CacheDirPath() if err != nil { t.Fatalf("CacheDirPath: %v", err) @@ -436,7 +356,13 @@ func TestCacheDirResolvers(t *testing.T) { if _, serr := os.Stat(lp); !os.IsNotExist(serr) { t.Errorf("LegacyCacheDir must not create %q (stat err=%v)", lp, serr) } - if want := filepath.Join(filepath.Join(d, "xconfig"), DirName, legacyCacheSubdir); lp != want { + // Expected legacy cache dir is computed from the resolver — no + // hard-coded per-OS paths. LegacyCacheDir = configDirPath() + "/cache". + cfgDir, err := configDirPath() + if err != nil { + t.Fatalf("configDirPath: %v", err) + } + if want := filepath.Join(cfgDir, legacyCacheSubdir); lp != want { t.Errorf("LegacyCacheDir = %q, want %q", lp, want) } @@ -447,13 +373,13 @@ func TestCacheDirResolvers(t *testing.T) { if _, serr := os.Stat(filepath.Dir(gp)); !os.IsNotExist(serr) { t.Errorf("GetConfigPathNoCreate must not create the config dir %q (stat err=%v)", filepath.Dir(gp), serr) } - if want := filepath.Join(filepath.Join(d, "xconfig"), DirName, ConfigFileYAML); gp != want { + if want := filepath.Join(cfgDir, ConfigFileYAML); gp != want { t.Errorf("GetConfigPathNoCreate = %q, want %q", gp, want) } }) t.Run("GetConfigDir still creates", func(t *testing.T) { - hermeticCfg(t) + hermeticConfig(t) cd, err := GetConfigDir() if err != nil { t.Fatalf("GetConfigDir: %v", err) diff --git a/internal/config/relocate.go b/internal/config/relocate.go new file mode 100644 index 0000000..dd39ace --- /dev/null +++ b/internal/config/relocate.go @@ -0,0 +1,284 @@ +package config + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "reflect" + "sort" + "sync" + + "gopkg.in/yaml.v3" +) + +// ErrRelocationConflict is returned by LoadConfig (and surfaced through +// LoadConfigForRuntime) when both the old hand-rolled config dir and the new +// statedir-resolved config dir contain a config.yml with materially different +// user settings. Mutation-free: nothing is copied, nothing is overwritten. +// The user reconciles by running `gro init` (which fails the same way at its +// pre-write gate) or by manually deleting one side. +var ErrRelocationConflict = errors.New("config: shared old/new config diverge") + +// relocKind is the four-way classification used by the relocation detector. +// Linux always collapses to relocNone because old and new paths are identical +// (os.UserConfigDir on Linux ≡ $XDG_CONFIG_HOME else $HOME/.config). +type relocKind int + +const ( + relocNone relocKind = iota // old path absent OR old==new (Linux short-circuit) + relocOldOnly // only the old hand-rolled config.yml exists + relocBothEqual // both exist with materially-equal Configs + relocBothDivergent // both exist with materially-different Configs +) + +// SharedRelocation is the result of DetectConfigRelocation. Paths are filled +// even on relocNone so callers can log/diagnose; CopyNeeded is true iff a +// gated ApplyConfigRelocation would actually do work. +type SharedRelocation struct { + Kind relocKind + OldPath string // old hand-rolled config dir; "" on Linux short-circuit + NewPath string // statedir-resolved config dir (always set) + CopyNeeded bool // relocOldOnly only +} + +// oldHandRolledConfigDir reproduces the prior pre-MON-5371 resolver: +// $XDG_CONFIG_HOME if set, else $HOME/.config; then "/google-readonly". Same +// shape on Linux/macOS/Windows (the deliberate "no %APPDATA% branch"). A +// missing HOME is an error (matches the original behavior). +func oldHandRolledConfigDir() (string, error) { + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + configHome = filepath.Join(home, ".config") + } + return filepath.Join(configHome, DirName), nil +} + +// OldHandRolledTokenPath is the pre-MON-5371 legacy token.json location. +// Exported so keychain.migrate's token-source enumeration can probe it with +// full §1.8 conflict semantics (per the MON-5371 plan: token.json is excluded +// from ApplyConfigRelocation and handled exclusively through the existing +// migrator). +func OldHandRolledTokenPath() (string, error) { + dir, err := oldHandRolledConfigDir() + if err != nil { + return "", err + } + return filepath.Join(dir, TokenFile), nil +} + +// DetectConfigRelocation classifies the old/new pair without touching disk +// beyond stats and reads. Never copies, never writes. On Linux (old==new) it +// short-circuits to relocNone. On macOS/Windows it returns one of the four +// kinds and the named paths. +func DetectConfigRelocation() (SharedRelocation, error) { + newDir, err := configDirPath() + if err != nil { + return SharedRelocation{}, err + } + return detectRelocation(newDir) +} + +// detectRelocation is the testable core: the new-dir path is injected so +// macOS/Windows divergence can be exercised on Linux CI (the same pattern +// MON-5370 used). +func detectRelocation(newDir string) (SharedRelocation, error) { + oldDir, err := oldHandRolledConfigDir() + if err != nil { + // Old path unresolvable (no HOME): treat as relocNone with new-only. + // LoadConfig still works against new; the gate is harmless. + return SharedRelocation{Kind: relocNone, NewPath: newDir}, nil + } + if oldDir == newDir { + // Linux: identical paths — nothing to relocate. + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + } + + oldYML := filepath.Join(oldDir, ConfigFileYAML) + newYML := filepath.Join(newDir, ConfigFileYAML) + oldPresent := fileExists(oldYML) + newPresent := fileExists(newYML) + switch { + case !oldPresent && !newPresent: + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + case oldPresent && !newPresent: + return SharedRelocation{Kind: relocOldOnly, OldPath: oldDir, NewPath: newDir, CopyNeeded: true}, nil + case !oldPresent && newPresent: + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + } + + // Both present — load and compare comparable subset. + oldCfg, oerr := loadConfigFromFile(oldYML) + newCfg, nerr := loadConfigFromFile(newYML) + if oerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s unreadable: %w", ErrRelocationConflict, oldYML, oerr) + } + if nerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: new %s unreadable: %w", ErrRelocationConflict, newYML, nerr) + } + if configsMaterialEqual(oldCfg, newCfg) { + return SharedRelocation{Kind: relocBothEqual, OldPath: oldDir, NewPath: newDir}, nil + } + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s and new %s have different settings; reconcile (or delete one) before running gro init", + ErrRelocationConflict, oldYML, newYML) +} + +// loadConfigFromFile parses a single config file (yaml or, by extension, the +// legacy json file). Defaults are NOT applied — we want the user-authored +// content for equality. +func loadConfigFromFile(path string) (Config, error) { + data, err := os.ReadFile(path) //nolint:gosec // path composed from validated config dir + if err != nil { + return Config{}, err + } + var cfg Config + switch filepath.Ext(path) { + case ".json": + if uerr := json.Unmarshal(data, &cfg); uerr != nil { + return Config{}, uerr + } + default: + if uerr := yaml.Unmarshal(data, &cfg); uerr != nil { + return Config{}, uerr + } + } + return cfg, nil +} + +// configsMaterialEqual compares the user-meaningful subset of two Configs. +// OAuthClientPath is intentionally excluded: it is location-dependent (each +// path defaults to /oauth_client.json), so two otherwise-identical +// configs from old and new dirs will differ only by this field, which is not +// a real divergence. CredentialRef, GrantedScopes, Keyring.Backend — these +// are the real user settings post-MON-5371. +func configsMaterialEqual(a, b Config) bool { + if a.CredentialRef != b.CredentialRef { + return false + } + if a.Keyring.Backend != b.Keyring.Backend { + return false + } + if !slicesEqualSorted(a.GrantedScopes, b.GrantedScopes) { + return false + } + return true +} + +func slicesEqualSorted(a, b []string) bool { + if len(a) != len(b) { + return false + } + aa := append([]string(nil), a...) + bb := append([]string(nil), b...) + sort.Strings(aa) + sort.Strings(bb) + return reflect.DeepEqual(aa, bb) +} + +// ApplyConfigRelocation copies every file in the old config dir to the new +// dir EXCEPT token.json (the only access secret; handled by keychain.migrate +// via §1.8 conflict semantics — see MON-5371 plan). Idempotent: if the new +// dir already has a same-named file, that one is left untouched. Files are +// written via temp+rename at 0600 under a 0700 dir. The old dir is not +// modified — leave-old gives the user a recovery point and matches the +// MON-5370 family pattern. +func ApplyConfigRelocation(r SharedRelocation) error { + if !r.CopyNeeded { + return nil + } + if r.OldPath == "" || r.NewPath == "" { + return fmt.Errorf("config: ApplyConfigRelocation called with empty path") + } + if err := os.MkdirAll(r.NewPath, DirPerm); err != nil { + return fmt.Errorf("creating new config dir: %w", err) + } + entries, err := os.ReadDir(r.OldPath) + if err != nil { + return fmt.Errorf("reading old config dir %s: %w", r.OldPath, err) + } + for _, e := range entries { + if e.IsDir() { + continue // pre-B2b cache subdir etc.; not part of config relocation + } + if e.Name() == TokenFile { + continue // secret — handled by keychain.migrate + } + dst := filepath.Join(r.NewPath, e.Name()) + if fileExists(dst) { + continue // idempotent on re-run + } + src := filepath.Join(r.OldPath, e.Name()) + if err := copyFileAtomic(src, dst); err != nil { + return fmt.Errorf("copying %s → %s: %w", src, dst, err) + } + } + return nil +} + +func copyFileAtomic(src, dst string) error { + in, err := os.Open(src) //nolint:gosec // path from old config dir + if err != nil { + return err + } + defer func() { _ = in.Close() }() + + dir := filepath.Dir(dst) + tmp, err := os.CreateTemp(dir, filepath.Base(dst)+"-*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + if _, err := io.Copy(tmp, in); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return err + } + if err := os.Chmod(tmpPath, TokenPerm); err != nil { + _ = os.Remove(tmpPath) + return err + } + if err := os.Rename(tmpPath, dst); err != nil { + _ = os.Remove(tmpPath) + return err + } + return nil +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// LoadConfigForRuntime is the soft-conflict variant of LoadConfig for non-init +// callers. On ErrRelocationConflict it prints a one-shot stderr warning, then +// returns the canonical (new-dir) config so the command can keep working. +// Init uses strict LoadConfig (fail-loud) at its relocation gate. +func LoadConfigForRuntime() (*Config, error) { + cfg, err := LoadConfig() + if err != nil && errors.Is(err, ErrRelocationConflict) { + warnReloConflictOnce(err) + return cfg, nil + } + return cfg, err +} + +var reloConflictOnce sync.Once + +func warnReloConflictOnce(err error) { + reloConflictOnce.Do(func() { + fmt.Fprintf(os.Stderr, "warning: %v; using the new config. Run `gro init` to reconcile.\n", err) + }) +} diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go new file mode 100644 index 0000000..82245e8 --- /dev/null +++ b/internal/config/relocate_test.go @@ -0,0 +1,320 @@ +package config + +import ( + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/open-cli-collective/cli-common/statedirtest" +) + +// reloctest fixture: gives two distinct directories (old and new), both +// inside the hermetic temp root. +func reloctest(t *testing.T) (oldDir, newDir string) { + t.Helper() + root := statedirtest.Hermetic(t) + oldDir = filepath.Join(root, "old", DirName) + newDir = filepath.Join(root, "new", DirName) + return oldDir, newDir +} + +// detectAt exercises the pure-function core that takes an injected newDir, +// so the four cases are testable on Linux even though Linux's real +// os.UserConfigDir collapses to old==new. +func detectAt(t *testing.T, oldDir, newDir string) (SharedRelocation, error) { + t.Helper() + // Point XDG_CONFIG_HOME at the parent of oldDir so oldHandRolledConfigDir + // resolves to oldDir on Linux. On macOS/Windows XDG_CONFIG_HOME is + // ignored by the new path resolver, but the old path resolver still + // reads it — so old/new diverge as intended. + t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) + return detectRelocation(newDir) +} + +func TestRelocate_OldOnly_CopiedAtInit(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("credential_ref: google-readonly/old\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, TokenFile), []byte(`{"access_token":"secret"}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, OAuthClientFile), []byte("not-secret"), 0o600); err != nil { + t.Fatal(err) + } + + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocOldOnly || !r.CopyNeeded { + t.Fatalf("kind=%v copyNeeded=%v, want relocOldOnly w/ copyNeeded", r.Kind, r.CopyNeeded) + } + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + // config.yml and oauth_client.json copied to new. + if _, err := os.Stat(filepath.Join(newDir, ConfigFileYAML)); err != nil { + t.Errorf("config.yml not copied to new: %v", err) + } + if _, err := os.Stat(filepath.Join(newDir, OAuthClientFile)); err != nil { + t.Errorf("oauth_client.json not copied to new: %v", err) + } + // token.json NEVER copied (handled by §1.8 migrator). + if _, err := os.Stat(filepath.Join(newDir, TokenFile)); !os.IsNotExist(err) { + t.Errorf("token.json must NOT be copied (it's a secret); stat err=%v", err) + } + // Old preserved (recovery point). + if _, err := os.Stat(filepath.Join(oldDir, ConfigFileYAML)); err != nil { + t.Errorf("old config.yml must remain (copy-leave-old): %v", err) + } + if _, err := os.Stat(filepath.Join(oldDir, TokenFile)); err != nil { + t.Errorf("old token.json must remain (migrator handles it): %v", err) + } +} + +func TestRelocate_NewOnly_LeftUntouched(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(newDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte("credential_ref: google-readonly/new\n"), 0o600); err != nil { + t.Fatal(err) + } + + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone || r.CopyNeeded { + t.Fatalf("kind=%v copyNeeded=%v, want relocNone", r.Kind, r.CopyNeeded) + } + // Apply is a no-op; new content survives. + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + data, _ := os.ReadFile(filepath.Join(newDir, ConfigFileYAML)) + if !strings.Contains(string(data), "google-readonly/new") { + t.Errorf("new config must be untouched, got %q", string(data)) + } +} + +func TestRelocate_Equal_NoOp(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(d, ConfigFileYAML), []byte("credential_ref: google-readonly/same\n"), 0o600); err != nil { + t.Fatal(err) + } + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocBothEqual { + t.Fatalf("kind=%v, want relocBothEqual", r.Kind) + } + if r.CopyNeeded { + t.Errorf("equal case must not request a copy") + } +} + +func TestRelocate_Divergent_FailLoudNamesBothPaths_MutatesNothing(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: google-readonly/old\n"}, + {newDir, "credential_ref: google-readonly/new\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, ConfigFileYAML), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + // Snapshot pre-detect state. + oldBefore, _ := os.ReadFile(filepath.Join(oldDir, ConfigFileYAML)) + newBefore, _ := os.ReadFile(filepath.Join(newDir, ConfigFileYAML)) + + r, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("want ErrRelocationConflict, got %v", err) + } + if r.Kind != relocBothDivergent { + t.Errorf("kind=%v, want relocBothDivergent", r.Kind) + } + // Error names both paths. + if !strings.Contains(err.Error(), oldDir) || !strings.Contains(err.Error(), newDir) { + t.Errorf("error must name both paths: %v", err) + } + // Nothing was mutated. + oldAfter, _ := os.ReadFile(filepath.Join(oldDir, ConfigFileYAML)) + newAfter, _ := os.ReadFile(filepath.Join(newDir, ConfigFileYAML)) + if string(oldBefore) != string(oldAfter) { + t.Errorf("old config mutated by detect") + } + if string(newBefore) != string(newAfter) { + t.Errorf("new config mutated by detect") + } +} + +func TestRelocate_MalformedOld_FailLoud(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("not-valid-yaml: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte("credential_ref: google-readonly/new\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed old must fail loud, got %v", err) + } + if !strings.Contains(err.Error(), oldDir) { + t.Errorf("error must name the malformed file: %v", err) + } +} + +func TestRelocate_MalformedNew_FailLoud(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("credential_ref: google-readonly/old\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte("not-valid-yaml: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed new must fail loud, got %v", err) + } + if !strings.Contains(err.Error(), newDir) { + t.Errorf("error must name the malformed file: %v", err) + } +} + +func TestRelocate_Neither_PathResolvedNotCreated(t *testing.T) { + oldDir, newDir := reloctest(t) + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone { + t.Errorf("kind=%v, want relocNone", r.Kind) + } + if _, err := os.Stat(oldDir); !os.IsNotExist(err) { + t.Errorf("detect must not create old dir; stat err=%v", err) + } + if _, err := os.Stat(newDir); !os.IsNotExist(err) { + t.Errorf("detect must not create new dir; stat err=%v", err) + } +} + +func TestRelocate_LinuxOldEqualsNew_ShortCircuits(t *testing.T) { + // When old path resolves identical to new path (the Linux steady state), + // detect must short-circuit to relocNone without inspecting contents. + // Simulate by pointing both at the same dir. + root := statedirtest.Hermetic(t) + same := filepath.Join(root, "same") + if err := os.MkdirAll(same, 0o700); err != nil { + t.Fatal(err) + } + // Point XDG_CONFIG_HOME so oldHandRolledConfigDir resolves to /. + t.Setenv("XDG_CONFIG_HOME", same) + r, err := detectRelocation(filepath.Join(same, DirName)) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone { + t.Errorf("kind=%v, want relocNone on path-identity short-circuit", r.Kind) + } +} + +func TestApplyConfigRelocation_IdempotentSkipsExistingNew(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(newDir, 0o700); err != nil { + t.Fatal(err) + } + // new already has config.yml with distinct content; copy must NOT overwrite. + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte("credential_ref: google-readonly/new\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("credential_ref: google-readonly/old\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, OAuthClientFile), []byte("not-secret"), 0o600); err != nil { + t.Fatal(err) + } + + r := SharedRelocation{Kind: relocOldOnly, OldPath: oldDir, NewPath: newDir, CopyNeeded: true} + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + got, _ := os.ReadFile(filepath.Join(newDir, ConfigFileYAML)) + if !strings.Contains(string(got), "google-readonly/new") { + t.Errorf("existing new config.yml must not be overwritten, got %q", string(got)) + } + // oauth_client.json (absent in new) gets copied. + if _, err := os.Stat(filepath.Join(newDir, OAuthClientFile)); err != nil { + t.Errorf("absent-in-new file must be copied: %v", err) + } +} + +func TestLoadConfigForRuntime_SoftConflict(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: google-readonly/old\n"}, + {newDir, "credential_ref: google-readonly/new\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, ConfigFileYAML), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + // Make the resolver pick newDir. On Linux XDG_CONFIG_HOME drives both old + // (hand-rolled) and new (statedir → os.UserConfigDir → XDG_CONFIG_HOME), + // which would collapse to old==new. To force divergence on Linux too, + // point them at distinct subtrees: XDG_CONFIG_HOME at oldDir's parent, + // HOME at newDir's parent (os.UserConfigDir on darwin/Windows uses HOME + // derivatives, on Linux uses XDG — covered above by the byte-snapshot + // divergent test, so here we focus on the soft-conflict semantic). + t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) + t.Setenv("HOME", filepath.Dir(newDir)) + + // Override LoadConfig's resolver to use our injected pair so the test is + // hermetic across OSes. Done by calling detectRelocation directly and + // asserting LoadConfig's documented contract via the public wrapper. + // Reset the once-warn so the test can verify it fires. + reloConflictOnce = onceReset() + + // On Linux this won't actually diverge unless paths differ — skip there. + if old, _ := oldHandRolledConfigDir(); old == newDir { + t.Skip("path-identity short-circuit; covered by other tests") + } +} + +// onceReset returns a fresh sync.Once for tests that need to re-arm +// reloConflictOnce. Tests must reset before relying on a fire. +func onceReset() (o sync.Once) { return } diff --git a/internal/credtest/credtest.go b/internal/credtest/credtest.go index 23ee536..6079401 100644 --- a/internal/credtest/credtest.go +++ b/internal/credtest/credtest.go @@ -1,37 +1,39 @@ // Package credtest provides a hermetic credential environment for tests -// (§1.12 test obligation). It forces credstore's encrypted-file backend -// inside a per-test temp HOME with a fixed passphrase, so no test ever -// touches the real OS keyring, shells out to `security`/`secret-tool`, or -// depends on machine state. +// (§1.12 test obligation). It delegates state-dir isolation to the shared +// cli-common/statedirtest helper (the full 7-var env set per §3.1 — closes +// the Windows real-dir leak the old HOME/XDG-only setup had), then layers +// the gro-specific keyring backend selection on top: credstore's +// encrypted-file backend with a known passphrase, plus the two legacy +// keychain/secret-tool scan disablers so no test ever shells out. // -// It is deliberately a tiny leaf (only testing + migrationsink) so that the -// white-box `package keychain` tests can import it without the -// keychain<->testutil import cycle that the fixture-heavy internal/testutil -// would introduce. +// It is deliberately a tiny leaf (only testing + statedirtest + config + +// migrationsink) so that the white-box `package keychain` tests can import it +// without the keychain<->testutil import cycle that the fixture-heavy +// internal/testutil would introduce. package credtest import ( - "path/filepath" "testing" + "github.com/open-cli-collective/cli-common/statedirtest" + + "github.com/open-cli-collective/google-readonly/internal/config" "github.com/open-cli-collective/google-readonly/internal/migrationsink" ) -// Setup isolates HOME/XDG to a temp dir and forces credstore's file backend -// with a known passphrase via the §1.4 named env vars. The darwin `security` -// and Linux `secret-tool` legacy probes are neutralized so the suite is -// hermetic regardless of the destination backend (§2.3). Returns the temp -// dir so a test can plant legacy artifacts (token.json, credentials.json, -// config.json, a keychain item) under it. +// Setup isolates the full §3.1 7-var env set under t.TempDir() (via +// statedirtest.Hermetic) and forces credstore's file backend with a known +// passphrase via the §1.4 named env vars. The darwin `security` and Linux +// `secret-tool` legacy probes are neutralized so the suite is hermetic +// regardless of the destination backend (§2.3). Returns the temp root so a +// test can plant legacy artifacts (token.json, credentials.json, config.json, +// a keychain item) — but tests should resolve their paths through +// config.GetConfigDir() / ConfigDir(t) below, not by hand-building subdirs, +// because os.UserConfigDir is platform-native (macOS ~/Library/Application +// Support, Windows %APPDATA%) and not derived from any single env var. func Setup(t *testing.T) string { t.Helper() - tmp := t.TempDir() - t.Setenv("HOME", tmp) - t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, "xdgconfig")) - // Isolate the OS cache dir too (B2b): XDG_CACHE_HOME covers Linux/CI, - // LOCALAPPDATA covers Windows os.UserCacheDir; HOME already covers macOS. - t.Setenv("XDG_CACHE_HOME", filepath.Join(tmp, "xdgcache")) - t.Setenv("LOCALAPPDATA", filepath.Join(tmp, "localappdata")) + tmp := statedirtest.Hermetic(t) t.Setenv("GOOGLE_READONLY_KEYRING_BACKEND", "file") t.Setenv("GOOGLE_READONLY_KEYRING_PASSPHRASE", "test-passphrase") t.Setenv("GRO_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN", "1") @@ -40,3 +42,16 @@ func Setup(t *testing.T) string { t.Cleanup(migrationsink.Reset) return tmp } + +// ConfigDir resolves the post-statedirtest hermetic config dir and creates +// it. Tests that plant legacy artifacts in the config dir should use this +// rather than hand-building subdirs of Setup's tmp root, which only worked on +// Linux pre-MON-5371. +func ConfigDir(t *testing.T) string { + t.Helper() + dir, err := config.GetConfigDir() + if err != nil { + t.Fatalf("credtest.ConfigDir: %v", err) + } + return dir +} diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 7823a7a..e42966e 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -68,7 +68,7 @@ func OpenForMigrationOverwrite() (*Store, error) { return open(true, true) } func OpenNoMigrate() (*Store, error) { return open(false, false) } func open(overwrite, runMigration bool) (*Store, error) { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return nil, err } @@ -83,7 +83,7 @@ func open(overwrite, runMigration bool) (*Store, error) { // discover the default ref's legacy data and could write it under the wrong // service/profile). func OpenRef(ref string) (*Store, error) { - cfg, err := config.LoadConfig() + cfg, err := config.LoadConfigForRuntime() if err != nil { return nil, err } diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 5e28dc0..b6b3f6e 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -138,8 +138,8 @@ func TestStoreRoundTripAndDelete(t *testing.T) { // ---- token migration matrix (file token.json) ---------------------------- func TestMigrateTokenFileAndIdempotent(t *testing.T) { - tmp := credtest.Setup(t) - tokenPath := filepath.Join(tmp, "xdgconfig", config.DirName, "token.json") + credtest.Setup(t) + tokenPath := filepath.Join(credtest.ConfigDir(t), "token.json") if err := os.MkdirAll(filepath.Dir(tokenPath), 0o700); err != nil { t.Fatal(err) } @@ -172,7 +172,7 @@ func TestMigrateTokenFileAndIdempotent(t *testing.T) { } func TestMigrateConflictFailsLoudWithoutLeaking(t *testing.T) { - tmp := credtest.Setup(t) + credtest.Setup(t) pre, err := openWith(testCfg(), false, false) if err != nil { t.Fatal(err) @@ -182,7 +182,7 @@ func TestMigrateConflictFailsLoudWithoutLeaking(t *testing.T) { } _ = pre.Close() - tokenPath := filepath.Join(tmp, "xdgconfig", config.DirName, "token.json") + tokenPath := filepath.Join(credtest.ConfigDir(t), "token.json") _ = os.MkdirAll(filepath.Dir(tokenPath), 0o700) if err := os.WriteFile(tokenPath, []byte(tokenB), 0o600); err != nil { // differs from keyring t.Fatal(err) @@ -213,8 +213,8 @@ func TestMigrateConflictFailsLoudWithoutLeaking(t *testing.T) { func TestMigrateOAuthClientJSON(t *testing.T) { t.Run("copy-only when target absent", func(t *testing.T) { - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) + credtest.Setup(t) + dir := credtest.ConfigDir(t) _ = os.MkdirAll(dir, 0o700) legacy := filepath.Join(dir, "credentials.json") if err := os.WriteFile(legacy, []byte(validClientJSONFixture), 0o600); err != nil { @@ -237,8 +237,8 @@ func TestMigrateOAuthClientJSON(t *testing.T) { }) t.Run("target valid present: legacy removed", func(t *testing.T) { - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) + credtest.Setup(t) + dir := credtest.ConfigDir(t) _ = os.MkdirAll(dir, 0o700) legacy := filepath.Join(dir, "credentials.json") target := filepath.Join(dir, "oauth_client.json") @@ -254,8 +254,8 @@ func TestMigrateOAuthClientJSON(t *testing.T) { }) t.Run("legacy valid, target invalid: target rewritten from legacy", func(t *testing.T) { - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) + credtest.Setup(t) + dir := credtest.ConfigDir(t) _ = os.MkdirAll(dir, 0o700) legacy := filepath.Join(dir, "credentials.json") target := filepath.Join(dir, "oauth_client.json") @@ -274,8 +274,8 @@ func TestMigrateOAuthClientJSON(t *testing.T) { }) t.Run("both invalid: nothing deleted, error names paths+fingerprints", func(t *testing.T) { - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) + credtest.Setup(t) + dir := credtest.ConfigDir(t) _ = os.MkdirAll(dir, 0o700) legacy := filepath.Join(dir, "credentials.json") target := filepath.Join(dir, "oauth_client.json") diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 28ddc7a..f7efcff 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -225,6 +225,7 @@ func discover() []candidate { } } + // New (statedir-resolved) token.json. This is the post-MON-5371 location. if path, err := config.GetTokenPath(); err == nil { if v, ok := readLegacyTokenFile(path); ok { out = append(out, candidate{ @@ -234,6 +235,22 @@ func discover() []candidate { }) } } + // Old hand-rolled token.json (pre-MON-5371): $XDG_CONFIG_HOME else + // ~/.config/google-readonly/token.json on every OS. On Linux this is the + // same path as the new resolver, so we dedupe to avoid emitting two + // candidates with identical bytes (which planMigration would correctly + // treat as equal/non-conflicting, but enumerating twice is just noise). + if oldPath, err := config.OldHandRolledTokenPath(); err == nil { + if newPath, nerr := config.GetTokenPath(); nerr != nil || newPath != oldPath { + if v, ok := readLegacyTokenFile(oldPath); ok { + out = append(out, candidate{ + location: fmt.Sprintf("file:%s", oldPath), + value: v, + deleter: func() error { return secureDelete(oldPath) }, + }) + } + } + } return out } diff --git a/internal/noleak/noleak_test.go b/internal/noleak/noleak_test.go index bffb17e..f08449f 100644 --- a/internal/noleak/noleak_test.go +++ b/internal/noleak/noleak_test.go @@ -53,11 +53,8 @@ const clientJSON = `{"installed":{"client_id":"123.apps.googleusercontent.com"," // values are the canaries, and returns the config dir. func seed(t *testing.T) string { t.Helper() - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) - if err := os.MkdirAll(dir, 0o700); err != nil { - t.Fatal(err) - } + credtest.Setup(t) + dir := credtest.ConfigDir(t) if err := os.WriteFile(filepath.Join(dir, config.OAuthClientFile), []byte(clientJSON), 0o644); err != nil { t.Fatal(err) } @@ -202,9 +199,8 @@ func TestMigrationSignal_TextFlushesToStderrOnce(t *testing.T) { // via config test, which runs keychain.Open() — migrating a planted legacy // token — then fails creating the Gmail client with no network). func TestMigrationSignal_SurvivesDownstreamError(t *testing.T) { - tmp := credtest.Setup(t) - dir := filepath.Join(tmp, "xdgconfig", config.DirName) - _ = os.MkdirAll(dir, 0o700) + credtest.Setup(t) + dir := credtest.ConfigDir(t) _ = os.WriteFile(filepath.Join(dir, config.OAuthClientFile), []byte(clientJSON), 0o644) if err := os.WriteFile(filepath.Join(dir, "token.json"), []byte(`{"access_token":"`+access+`","refresh_token":"`+refresh+`","token_type":"Bearer"}`), 0o600); err != nil { From fb5e789a08292557039fd846b288beae7d29b691 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 18:28:21 -0400 Subject: [PATCH 2/6] fix(state): address Codex PR review blockers + TDD gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 `/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] --- internal/cache/cache.go | 28 ++++- internal/config/config.go | 13 +++ internal/config/relocate.go | 80 +++++++++++--- internal/config/relocate_test.go | 163 +++++++++++++++++++++++++---- internal/keychain/keychain_test.go | 73 +++++++++++++ 5 files changed, 313 insertions(+), 44 deletions(-) diff --git a/internal/cache/cache.go b/internal/cache/cache.go index d3dc55f..41ab018 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -73,22 +73,40 @@ func New() (*Cache, error) { // because cli-common/cache's identity/version check + this package's // parse-error-mapped-to-miss demote it on the next read. func migrateLegacyCacheDir(newDir string) { - legacy, err := config.LegacyCacheDir() - if err != nil { - return + // Two possible legacy locations on macOS/Windows: (a) the pre-B2b + // "cache" subdir under the NEW (statedir-resolved) config dir — what a + // user reached if they already ran post-port code once — and (b) the + // same subdir under the OLD hand-rolled config dir, which is where a + // pure pre-MON-5371 install lived. Both are byte-carried best-effort; + // any failure abandons the migration without touching New's result. On + // Linux the two paths collapse, so the second probe is a stat-only no-op. + probes := []func() (string, error){ + config.LegacyCacheDir, + config.OldHandRolledLegacyCacheDir, + } + tried := map[string]bool{} + for _, p := range probes { + legacy, err := p() + if err != nil || tried[legacy] { + continue + } + tried[legacy] = true + tryCarryLegacyCache(legacy, newDir) } +} + +func tryCarryLegacyCache(legacy, newDir string) { if _, err := os.Stat(legacy); err != nil { return } legacyDrives := filepath.Join(legacy, DrivesFile) - // New cache lives at //drives.json now. newDrives := filepath.Join(newDir, instanceKey, DrivesFile) switch _, serr := os.Stat(newDrives); { case serr == nil: // New cache already present: nothing to carry, safe to drop legacy. case os.IsNotExist(serr): - data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // path from config dir + data, rerr := os.ReadFile(legacyDrives) //nolint:gosec // path from legacy config dir switch { case rerr == nil: if mkErr := os.MkdirAll(filepath.Dir(newDrives), 0o700); mkErr != nil { diff --git a/internal/config/config.go b/internal/config/config.go index c0b4f4e..2b03955 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -260,6 +260,8 @@ func LoadConfig() (*Config, error) { } } if !read && reloc.Kind == relocOldOnly && reloc.OldPath != "" { + // Try old/config.yml first; fall back to old/config.json (a + // pre-MON-5371 macOS/Windows user may be on either form). oldYML := filepath.Join(reloc.OldPath, ConfigFileYAML) if data, err := os.ReadFile(oldYML); err == nil { //nolint:gosec // path from hand-rolled legacy dir if uerr := yaml.Unmarshal(data, cfg); uerr != nil { @@ -269,6 +271,17 @@ func LoadConfig() (*Config, error) { } else if !os.IsNotExist(err) { return nil, fmt.Errorf("read config %s: %w", oldYML, err) } + if !read { + oldJSON := filepath.Join(reloc.OldPath, ConfigFile) + if data, err := os.ReadFile(oldJSON); err == nil { //nolint:gosec // path from hand-rolled legacy dir + if jerr := json.Unmarshal(data, cfg); jerr != nil { + return nil, fmt.Errorf("parse legacy config %s: %w", oldJSON, jerr) + } + read = true + } else if !os.IsNotExist(err) { + return nil, fmt.Errorf("read config %s: %w", oldJSON, err) + } + } } if !read { if jerr := loadLegacyJSON(cfg); jerr != nil { diff --git a/internal/config/relocate.go b/internal/config/relocate.go index dd39ace..2f1a2f9 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -73,6 +73,20 @@ func OldHandRolledTokenPath() (string, error) { return filepath.Join(dir, TokenFile), nil } +// OldHandRolledLegacyCacheDir is the pre-B2b cache subdir under the pre- +// MON-5371 hand-rolled config dir (a real artifact on macOS/Windows installs +// that pre-date both B2b and MON-5371). cli-common does NOT resolve here, +// so internal/cache's migrator probes both this path and the in-resolver +// LegacyCacheDir, byte-carrying whichever is present. On Linux this resolves +// identical to LegacyCacheDir and dedupes inside the migrator. +func OldHandRolledLegacyCacheDir() (string, error) { + dir, err := oldHandRolledConfigDir() + if err != nil { + return "", err + } + return filepath.Join(dir, legacyCacheSubdir), nil +} + // DetectConfigRelocation classifies the old/new pair without touching disk // beyond stats and reads. Never copies, never writes. On Linux (old==new) it // short-circuits to relocNone. On macOS/Windows it returns one of the four @@ -100,10 +114,12 @@ func detectRelocation(newDir string) (SharedRelocation, error) { return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil } - oldYML := filepath.Join(oldDir, ConfigFileYAML) - newYML := filepath.Join(newDir, ConfigFileYAML) - oldPresent := fileExists(oldYML) - newPresent := fileExists(newYML) + // Both .yml and the legacy .json count as a present config. A + // pre-MON-5371 macOS/Windows user could be on either form; the wholesale + // copy at init carries either over, and the per-file migrator + // (promoteLegacyConfigJSON) takes it from there. + oldPath, oldPresent := firstExistingConfig(oldDir) + newPath, newPresent := firstExistingConfig(newDir) switch { case !oldPresent && !newPresent: return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil @@ -114,22 +130,22 @@ func detectRelocation(newDir string) (SharedRelocation, error) { } // Both present — load and compare comparable subset. - oldCfg, oerr := loadConfigFromFile(oldYML) - newCfg, nerr := loadConfigFromFile(newYML) + oldCfg, oerr := loadConfigFromFile(oldPath) + newCfg, nerr := loadConfigFromFile(newPath) if oerr != nil { return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, - fmt.Errorf("%w: old %s unreadable: %w", ErrRelocationConflict, oldYML, oerr) + fmt.Errorf("%w: old %s unreadable: %w", ErrRelocationConflict, oldPath, oerr) } if nerr != nil { return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, - fmt.Errorf("%w: new %s unreadable: %w", ErrRelocationConflict, newYML, nerr) + fmt.Errorf("%w: new %s unreadable: %w", ErrRelocationConflict, newPath, nerr) } - if configsMaterialEqual(oldCfg, newCfg) { + if configsMaterialEqual(oldCfg, newCfg, oldDir, newDir) { return SharedRelocation{Kind: relocBothEqual, OldPath: oldDir, NewPath: newDir}, nil } return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, fmt.Errorf("%w: old %s and new %s have different settings; reconcile (or delete one) before running gro init", - ErrRelocationConflict, oldYML, newYML) + ErrRelocationConflict, oldPath, newPath) } // loadConfigFromFile parses a single config file (yaml or, by extension, the @@ -155,12 +171,14 @@ func loadConfigFromFile(path string) (Config, error) { } // configsMaterialEqual compares the user-meaningful subset of two Configs. -// OAuthClientPath is intentionally excluded: it is location-dependent (each -// path defaults to /oauth_client.json), so two otherwise-identical -// configs from old and new dirs will differ only by this field, which is not -// a real divergence. CredentialRef, GrantedScopes, Keyring.Backend — these -// are the real user settings post-MON-5371. -func configsMaterialEqual(a, b Config) bool { +// CredentialRef / GrantedScopes / Keyring.Backend are real user settings and +// any difference is a divergence. OAuthClientPath is canonicalized through +// oauthClientPathEquiv so that two configs whose paths happen to be each +// dir's default (`/oauth_client.json`) are treated as equal — +// that's a location artifact, not a user choice — while an explicit +// non-default path that differs between the two sides is a real divergence +// and must fail loud (an org override the user set deliberately). +func configsMaterialEqual(a, b Config, oldDir, newDir string) bool { if a.CredentialRef != b.CredentialRef { return false } @@ -170,9 +188,26 @@ func configsMaterialEqual(a, b Config) bool { if !slicesEqualSorted(a.GrantedScopes, b.GrantedScopes) { return false } + if !oauthClientPathEquiv(a.OAuthClientPath, b.OAuthClientPath, oldDir, newDir) { + return false + } return true } +// oauthClientPathEquiv treats "both empty", "both equal to their own dir's +// default oauth_client.json", and "both literally equal" as equivalent. Any +// other combination — including one side at its default and the other at a +// non-default explicit path — is a divergence the user set deliberately and +// must surface. +func oauthClientPathEquiv(aPath, bPath, aDir, bDir string) bool { + if aPath == bPath { + return true + } + aIsDefault := aPath == "" || aPath == filepath.Join(aDir, OAuthClientFile) + bIsDefault := bPath == "" || bPath == filepath.Join(bDir, OAuthClientFile) + return aIsDefault && bIsDefault +} + func slicesEqualSorted(a, b []string) bool { if len(a) != len(b) { return false @@ -262,6 +297,19 @@ func fileExists(path string) bool { return err == nil } +// firstExistingConfig returns (/config.yml, true) if the YAML is present, +// else (/config.json, true) if only the legacy JSON is present, else +// ("", false). Mirrors LoadConfig's read-priority (YAML wins). +func firstExistingConfig(dir string) (string, bool) { + if p := filepath.Join(dir, ConfigFileYAML); fileExists(p) { + return p, true + } + if p := filepath.Join(dir, ConfigFile); fileExists(p) { + return p, true + } + return "", false +} + // LoadConfigForRuntime is the soft-conflict variant of LoadConfig for non-init // callers. On ErrRelocationConflict it prints a one-shot stderr warning, then // returns the canonical (new-dir) config so the command can keep working. diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index 82245e8..3e2db7b 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -227,6 +227,83 @@ func TestRelocate_Neither_PathResolvedNotCreated(t *testing.T) { } } +func TestRelocate_OAuthClientPath_DefaultDefaultIsEqual(t *testing.T) { + // Both sides reference their respective dir's default oauth_client.json + // — that's a location artifact, not a user choice. Equal. + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + oldCfg := "credential_ref: google-readonly/same\noauth_client_path: " + filepath.Join(oldDir, OAuthClientFile) + "\n" + newCfg := "credential_ref: google-readonly/same\noauth_client_path: " + filepath.Join(newDir, OAuthClientFile) + "\n" + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte(oldCfg), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte(newCfg), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocBothEqual { + t.Errorf("kind=%v, want relocBothEqual (defaults differ only by dir prefix)", r.Kind) + } +} + +func TestRelocate_OAuthClientPath_ExplicitNonDefaultDivergence_FailsLoud(t *testing.T) { + // User explicitly set oauth_client_path to a non-default location on + // one side. That's a deliberate org override and must surface. + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + oldCfg := "credential_ref: google-readonly/same\noauth_client_path: /opt/myorg/oauth_client.json\n" + newCfg := "credential_ref: google-readonly/same\noauth_client_path: " + filepath.Join(newDir, OAuthClientFile) + "\n" + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte(oldCfg), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte(newCfg), 0o600); err != nil { + t.Fatal(err) + } + _, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("explicit oauth_client_path divergence must fail loud, got %v", err) + } +} + +func TestRelocate_OldOnlyConfigJSON_TriggersCopy(t *testing.T) { + // A pre-MON-5371 install on macOS may have only legacy config.json (not + // yet promoted to config.yml). Detection must still classify as + // relocOldOnly so init copies the JSON into the new dir, where the + // existing promoteLegacyConfigJSON migrator picks it up. + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFile), []byte(`{"credential_ref":"google-readonly/from-old-json"}`), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocOldOnly || !r.CopyNeeded { + t.Fatalf("kind=%v copy=%v, want relocOldOnly w/ copy (legacy config.json present in old)", + r.Kind, r.CopyNeeded) + } + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + if _, err := os.Stat(filepath.Join(newDir, ConfigFile)); err != nil { + t.Errorf("legacy config.json must be copied into new dir: %v", err) + } +} + func TestRelocate_LinuxOldEqualsNew_ShortCircuits(t *testing.T) { // When old path resolves identical to new path (the Linux steady state), // detect must short-circuit to relocNone without inspecting contents. @@ -280,7 +357,46 @@ func TestApplyConfigRelocation_IdempotentSkipsExistingNew(t *testing.T) { } } -func TestLoadConfigForRuntime_SoftConflict(t *testing.T) { +// loadConfigForRuntimeAt is the testable seam for the runtime soft-conflict +// wrapper. It wires both detectRelocation and the YAML load through an +// injected newDir + oldDir, which is the only way to exercise the divergent +// branch on a platform where os.UserConfigDir == $XDG_CONFIG_HOME (Linux). +// Production code paths use LoadConfigForRuntime via the real resolver. +func loadConfigForRuntimeAt(t *testing.T, oldDir, newDir string) (*Config, error) { + t.Helper() + t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) + // Reset the warn-once before the test so a previous test's flag doesn't + // suppress the warning we want to verify. + reloConflictOnce = sync.Once{} + r, err := detectRelocation(newDir) + if err != nil && !errors.Is(err, ErrRelocationConflict) { + return nil, err + } + cfg := &Config{} + if newPath, ok := firstExistingConfig(r.NewPath); ok { + c, lerr := loadConfigFromFile(newPath) + if lerr != nil { + return nil, lerr + } + cfg = &c + } else if r.Kind == relocOldOnly { + if oldPath, ok := firstExistingConfig(r.OldPath); ok { + c, lerr := loadConfigFromFile(oldPath) + if lerr != nil { + return nil, lerr + } + cfg = &c + } + } + cfg.applyDefaults() + if errors.Is(err, ErrRelocationConflict) { + warnReloConflictOnce(err) + return cfg, nil + } + return cfg, err +} + +func TestLoadConfigForRuntime_SoftConflict_ReturnsCanonical(t *testing.T) { oldDir, newDir := reloctest(t) for _, p := range []struct{ dir, content string }{ {oldDir, "credential_ref: google-readonly/old\n"}, @@ -293,28 +409,29 @@ func TestLoadConfigForRuntime_SoftConflict(t *testing.T) { t.Fatal(err) } } - // Make the resolver pick newDir. On Linux XDG_CONFIG_HOME drives both old - // (hand-rolled) and new (statedir → os.UserConfigDir → XDG_CONFIG_HOME), - // which would collapse to old==new. To force divergence on Linux too, - // point them at distinct subtrees: XDG_CONFIG_HOME at oldDir's parent, - // HOME at newDir's parent (os.UserConfigDir on darwin/Windows uses HOME - // derivatives, on Linux uses XDG — covered above by the byte-snapshot - // divergent test, so here we focus on the soft-conflict semantic). - t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) - t.Setenv("HOME", filepath.Dir(newDir)) - - // Override LoadConfig's resolver to use our injected pair so the test is - // hermetic across OSes. Done by calling detectRelocation directly and - // asserting LoadConfig's documented contract via the public wrapper. - // Reset the once-warn so the test can verify it fires. - reloConflictOnce = onceReset() - - // On Linux this won't actually diverge unless paths differ — skip there. - if old, _ := oldHandRolledConfigDir(); old == newDir { - t.Skip("path-identity short-circuit; covered by other tests") + cfg, err := loadConfigForRuntimeAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("soft-conflict must return nil error, got %v", err) + } + // Returns the canonical (new-dir) cfg. + if cfg.CredentialRef != "google-readonly/new" { + t.Errorf("soft-conflict must return new-dir cfg, got CredentialRef=%q", cfg.CredentialRef) } } -// onceReset returns a fresh sync.Once for tests that need to re-arm -// reloConflictOnce. Tests must reset before relying on a fire. -func onceReset() (o sync.Once) { return } +func TestLoadConfigForRuntime_NoConflict_PassesThrough(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(newDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, ConfigFileYAML), []byte("credential_ref: google-readonly/only-new\n"), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := loadConfigForRuntimeAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("err=%v", err) + } + if cfg.CredentialRef != "google-readonly/only-new" { + t.Errorf("got %q, want google-readonly/only-new", cfg.CredentialRef) + } +} diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index b6b3f6e..9ecd42e 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -209,6 +209,79 @@ func TestMigrateConflictFailsLoudWithoutLeaking(t *testing.T) { } } +// TestMigrateTokenFile_OldHandRolledPath covers the MON-5371 token-source +// enumeration extension: a pre-MON-5371 macOS/Windows install can have a +// token.json at the OLD hand-rolled config dir (not the new statedir- +// resolved dir). The migrator must find it and migrate it just like a +// new-dir token. +func TestMigrateTokenFile_OldHandRolledPath(t *testing.T) { + credtest.Setup(t) + oldTokenPath, err := config.OldHandRolledTokenPath() + if err != nil { + t.Fatalf("OldHandRolledTokenPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(oldTokenPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(oldTokenPath, []byte(tokenA), 0o600); err != nil { + t.Fatal(err) + } + + st, err := openWith(testCfg(), false, true) // runMigration + if err != nil { + t.Fatalf("migrate open: %v", err) + } + tok, err := st.Token() + if err != nil || tok.AccessToken != "AAA" { + t.Fatalf("token at OLD path not migrated into keyring: %+v err=%v", tok, err) + } + _ = st.Close() + if _, statErr := os.Stat(oldTokenPath); !os.IsNotExist(statErr) { + t.Fatal("old-path token.json must be removed after migration (no stale plaintext)") + } +} + +// TestMigrateTokenFile_OldAndNewDivergent covers the §1.8 invariant: if both +// old/token.json and new/token.json exist with different bytes, the migrator +// must fail loud and mutate nothing. +func TestMigrateTokenFile_OldAndNewDivergent(t *testing.T) { + credtest.Setup(t) + oldTokenPath, err := config.OldHandRolledTokenPath() + if err != nil { + t.Fatalf("OldHandRolledTokenPath: %v", err) + } + newTokenPath, err := config.GetTokenPath() + if err != nil { + t.Fatalf("GetTokenPath: %v", err) + } + if oldTokenPath == newTokenPath { + t.Skip("Linux: old and new paths identical — dedup makes divergence impossible") + } + if err := os.MkdirAll(filepath.Dir(oldTokenPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(newTokenPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(oldTokenPath, []byte(tokenA), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(newTokenPath, []byte(tokenB), 0o600); err != nil { + t.Fatal(err) + } + + _, err = openWith(testCfg(), false, true) + if err == nil { + t.Fatal("divergent old/new token.json must fail loud, got nil") + } + if _, e := os.Stat(oldTokenPath); e != nil { + t.Errorf("old token.json must remain on conflict: %v", e) + } + if _, e := os.Stat(newTokenPath); e != nil { + t.Errorf("new token.json must remain on conflict: %v", e) + } +} + // ---- deployment-material migration (credentials.json -> oauth_client.json) - func TestMigrateOAuthClientJSON(t *testing.T) { From 7ba31059f138c06ba69e368eaa20d96851e4822a Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 18:30:35 -0400 Subject: [PATCH 3/6] fix(state): malformed old-only config fails loud before any copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] --- internal/config/relocate.go | 8 +++++++ internal/config/relocate_test.go | 41 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 2f1a2f9..89b73fa 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -124,6 +124,14 @@ func detectRelocation(newDir string) (SharedRelocation, error) { case !oldPresent && !newPresent: return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil case oldPresent && !newPresent: + // Validate old-only parses cleanly BEFORE signaling CopyNeeded — + // otherwise init's ApplyConfigRelocation would propagate a malformed + // legacy file into the new dir, then LoadConfig would die parsing it + // post-relocation. §3.2 malformed-old: fail loud, mutate nothing. + if _, oerr := loadConfigFromFile(oldPath); oerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s is malformed: %w", ErrRelocationConflict, oldPath, oerr) + } return SharedRelocation{Kind: relocOldOnly, OldPath: oldDir, NewPath: newDir, CopyNeeded: true}, nil case !oldPresent && newPresent: return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index 3e2db7b..c067f39 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -304,6 +304,47 @@ func TestRelocate_OldOnlyConfigJSON_TriggersCopy(t *testing.T) { } } +func TestRelocate_OldOnlyMalformedYAML_FailLoud_MutatesNothing(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFileYAML), []byte("not-valid-yaml: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed old-only must fail loud, got %v", err) + } + if r.CopyNeeded { + t.Errorf("CopyNeeded must be false on malformed old-only (mutates nothing)") + } + // New dir must NOT have been created by detection. + if _, e := os.Stat(newDir); !os.IsNotExist(e) { + t.Errorf("new dir must not exist after malformed-old detect: stat err=%v", e) + } +} + +func TestRelocate_OldOnlyMalformedJSON_FailLoud_MutatesNothing(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, ConfigFile), []byte("{not valid json"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed old-only JSON must fail loud, got %v", err) + } + if r.CopyNeeded { + t.Errorf("CopyNeeded must be false on malformed old-only JSON") + } + if _, e := os.Stat(newDir); !os.IsNotExist(e) { + t.Errorf("new dir must not exist after malformed-old detect: stat err=%v", e) + } +} + func TestRelocate_LinuxOldEqualsNew_ShortCircuits(t *testing.T) { // When old path resolves identical to new path (the Linux steady state), // detect must short-circuit to relocNone without inspecting contents. From 36c13552644432eecec9e41e3102286fbbff534a Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 20:50:20 -0400 Subject: [PATCH 4/6] 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] --- internal/cache/cache_test.go | 23 +++++++++++++++++++++++ internal/cmd/initcmd/init.go | 26 +++++--------------------- internal/config/config.go | 11 ++++++++--- internal/config/relocate.go | 6 +++++- 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 8a73556..f24d4d4 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -236,4 +236,27 @@ func TestMigrateLegacyCacheDir(t *testing.T) { defer c.Clear() }) + t.Run("old-hand-rolled legacy cache subdir is carried (pre-MON-5371 install)", func(t *testing.T) { + hermetic(t) + oldLegacy, err := config.OldHandRolledLegacyCacheDir() + testutil.NoError(t, err) + newDir, err := config.CacheDirPath() + testutil.NoError(t, err) + // On Linux this is the same path as LegacyCacheDir — dedup handles it. + // On macOS/Windows this is the path a pure pre-MON-5371 install lived + // at and is what the dual-probe needs to find. + testutil.NoError(t, os.MkdirAll(oldLegacy, 0o700)) + testutil.NoError(t, os.WriteFile(filepath.Join(oldLegacy, DrivesFile), + []byte(`{"cached_at":"2026-01-01T00:00:00Z","ttl_hours":24,"drives":[{"id":"hand","name":"R"}]}`), 0o600)) + + c, err := New() + testutil.NoError(t, err) + defer c.Clear() + + got, err := os.ReadFile(filepath.Join(newDir, c.loc.InstanceKey, DrivesFile)) + testutil.NoError(t, err) + testutil.Contains(t, string(got), `"hand"`) + _, statErr := os.Stat(oldLegacy) + testutil.True(t, os.IsNotExist(statErr)) + }) } diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index 9824ea9..439b79e 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -320,21 +320,8 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { return err } - // Step 2: snapshot config.json existence BEFORE we save granted scopes. - // We use this snapshot to gate the cache-TTL prompt: if config existed - // already, we don't re-ask. Today's code samples this AFTER scope save, - // which made the prompt unreachable on first runs. - configPath, err := d.GetConfigPath() - if err != nil { - return fmt.Errorf("getting config path: %w", err) - } - configExistedBefore := false - if _, err := d.Stat(configPath); err == nil { - configExistedBefore = true - } - // Step 3: token resolution. - handled, err := tryExistingToken(ctx, d, opts, configExistedBefore) + handled, err := tryExistingToken(ctx, d, opts) if err != nil { return err } @@ -400,7 +387,6 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { if saveErr := d.SaveConfig(cfg); saveErr != nil { d.View.Error("Warning: saving granted scopes: %v", saveErr) } - _ = configExistedBefore // pre-MON-5371 snapshot-before-save gate for the AskCacheTTL prompt; the prompt is gone (TTL is hard-coded per resource, §4.4) so the snapshot has no remaining consumer. // Step 7: verify + render `gro me` one-liner. People failure is fatal — // init's success contract is that you can immediately run `gro me`. @@ -434,7 +420,7 @@ func runWith(ctx context.Context, d initDeps, opts *initOptions) error { // but People-insufficient token (typical of users who upgraded gro) must // trigger re-auth here, otherwise `gro me`'s "run gro init" message // produces an infinite remediation loop. -func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, configExistedBefore bool) (bool, error) { +func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions) (bool, error) { if !d.HasStoredToken() { return false, nil } @@ -457,7 +443,7 @@ func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, config // for everything past the recorded-scope check above. if opts.noVerify { d.View.Success("Existing token accepted (--no-verify; API not called)") - return true, finishExisting(d, configExistedBefore, nil /* no profile */) + return true, finishExisting(d, nil /* no profile */) } email, err := d.GmailVerify(ctx) @@ -488,7 +474,7 @@ func tryExistingToken(ctx context.Context, d initDeps, opts *initOptions, config return false, fmt.Errorf("verifying People API: %w", err) } - return true, finishExisting(d, configExistedBefore, profile) + return true, finishExisting(d, profile) } // promptAndDeleteForReauth asks the user whether to re-auth and clears the @@ -518,13 +504,11 @@ func promptAndDeleteForReauth(d initDeps) error { // granted scopes are an unknown to this run, and writing auth.AllScopes // would falsely claim the token is current. Only a fresh OAuth flow knows // what was just granted. -func finishExisting(d initDeps, configExistedBefore bool, profile *people.Profile) error { +func finishExisting(d initDeps, profile *people.Profile) error { if profile != nil { d.View.Println("") mecmd.RenderOneLiner(d.View.Out, profile) } - - _ = configExistedBefore // pre-MON-5371 TTL prompt gate (now gone). return nil } diff --git a/internal/config/config.go b/internal/config/config.go index 2b03955..b9caad7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -245,10 +245,15 @@ func LoadConfig() (*Config, error) { } // Priority: new/config.yml → old/config.yml (only if new is absent) → - // legacy config.json at the new dir. + // legacy config.json at the new dir. When relErr is already set (a + // detect-time ErrRelocationConflict — including a malformed-new wrapped + // inside it), we do not re-surface a bare parse error here: the wrapped + // conflict error already names both paths and the underlying cause, and + // returning the bare parse error instead would make LoadConfigForRuntime + // hard-fail instead of soft-degrading. Skip the re-parse on conflict. cfg := &Config{} read := false - if reloc.NewPath != "" { + if reloc.NewPath != "" && relErr == nil { newYML := filepath.Join(reloc.NewPath, ConfigFileYAML) if data, err := os.ReadFile(newYML); err == nil { //nolint:gosec // path from validated dir if uerr := yaml.Unmarshal(data, cfg); uerr != nil { @@ -283,7 +288,7 @@ func LoadConfig() (*Config, error) { } } } - if !read { + if !read && relErr == nil { if jerr := loadLegacyJSON(cfg); jerr != nil { return nil, jerr } diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 89b73fa..f69ea82 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -190,7 +190,11 @@ func configsMaterialEqual(a, b Config, oldDir, newDir string) bool { if a.CredentialRef != b.CredentialRef { return false } - if a.Keyring.Backend != b.Keyring.Backend { + // reflect.DeepEqual on the whole Keyring sub-struct: a new field added + // to KeyringConfig (e.g. a passphrase env-var name) must not silently + // classify as "same" just because the old code path only knew about + // Backend. + if !reflect.DeepEqual(a.Keyring, b.Keyring) { return false } if !slicesEqualSorted(a.GrantedScopes, b.GrantedScopes) { From 94f60bb32e00bfe5188bd577ae4c64d160dbc60c Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 20:56:04 -0400 Subject: [PATCH 5/6] =?UTF-8?q?fix(state):=20daemon=20round=202=20?= =?UTF-8?q?=E2=80=94=20real=20regression=20+=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 / instead of the tool-level Root so a future multi-instance Locator can't have one instance's Clear evict the others. [MON-5371] --- internal/cache/cache.go | 7 +++++-- internal/cache/cache_test.go | 3 ++- internal/config/config.go | 35 +++++++++++++++++++++++++---------- internal/config/relocate.go | 6 ++++++ 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 41ab018..4482f81 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -158,9 +158,12 @@ func (c *Cache) SetDrives(drives []*CachedDrive) error { return nil } -// Clear removes all cached data. +// Clear removes all cached data for this instance. Scoped to +// / rather than the tool-level Root so a future move to a +// multi-instance Locator can't have one instance's Clear() silently evict +// every other instance's cache. func (c *Cache) Clear() error { - return os.RemoveAll(c.loc.Root) + return os.RemoveAll(filepath.Join(c.loc.Root, c.loc.InstanceKey)) } // GetDir returns the cache directory path. diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index f24d4d4..d9faad4 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -138,7 +138,8 @@ func TestCache_Clear(t *testing.T) { testutil.NoError(t, c.SetDrives([]*CachedDrive{{ID: "test", Name: "Test"}})) testutil.NoError(t, c.Clear()) - _, err = os.Stat(c.loc.Root) + // Clear scoped to /, not the tool-level Root. + _, err = os.Stat(filepath.Join(c.loc.Root, c.loc.InstanceKey)) testutil.True(t, os.IsNotExist(err)) } diff --git a/internal/config/config.go b/internal/config/config.go index b9caad7..8499f8d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -245,23 +245,38 @@ func LoadConfig() (*Config, error) { } // Priority: new/config.yml → old/config.yml (only if new is absent) → - // legacy config.json at the new dir. When relErr is already set (a - // detect-time ErrRelocationConflict — including a malformed-new wrapped - // inside it), we do not re-surface a bare parse error here: the wrapped - // conflict error already names both paths and the underlying cause, and - // returning the bare parse error instead would make LoadConfigForRuntime - // hard-fail instead of soft-degrading. Skip the re-parse on conflict. + // legacy config.json at the new dir. We attempt the new-dir read even + // when relErr is set (a detect-time ErrRelocationConflict) so callers + // soft-degrading via LoadConfigForRuntime actually get the user's new- + // dir settings (non-default OAuthClientPath, recorded GrantedScopes, + // etc.) — returning all-defaults instead would silently mask issues + // like a re-auth requirement or break OAuth client resolution. On a + // parse error while relErr is set we DON'T re-surface the bare parse + // error: the wrapped conflict already names both paths and the cause, + // and a hard-fail here would defeat the soft-degrade contract. cfg := &Config{} read := false - if reloc.NewPath != "" && relErr == nil { + if reloc.NewPath != "" { newYML := filepath.Join(reloc.NewPath, ConfigFileYAML) if data, err := os.ReadFile(newYML); err == nil { //nolint:gosec // path from validated dir if uerr := yaml.Unmarshal(data, cfg); uerr != nil { - return nil, fmt.Errorf("parse config %s: %w", newYML, uerr) + if relErr != nil { + // Already returning a conflict; keep cfg empty for this side + // rather than propagate a parse error LoadConfigForRuntime + // can't soft-degrade. + cfg = &Config{} + } else { + return nil, fmt.Errorf("parse config %s: %w", newYML, uerr) + } + } else { + read = true } - read = true } else if !os.IsNotExist(err) { - return nil, fmt.Errorf("read config %s: %w", newYML, err) + if relErr != nil { + cfg = &Config{} + } else { + return nil, fmt.Errorf("read config %s: %w", newYML, err) + } } } if !read && reloc.Kind == relocOldOnly && reloc.OldPath != "" { diff --git a/internal/config/relocate.go b/internal/config/relocate.go index f69ea82..11f69aa 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -212,6 +212,12 @@ func configsMaterialEqual(a, b Config, oldDir, newDir string) bool { // non-default explicit path — is a divergence the user set deliberately and // must surface. func oauthClientPathEquiv(aPath, bPath, aDir, bDir string) bool { + // Normalize tilde and relative components on both sides before comparing + // (loadConfigFromFile deliberately skips applyDefaults / ExpandPath so + // that we compare user-authored bytes — but a `~/oauth_client.json` and + // the same path fully expanded should not look divergent). + aPath = ExpandPath(aPath) + bPath = ExpandPath(bPath) if aPath == bPath { return true } From 5436775913b146651b1749376d08898965d0815e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 19 May 2026 21:05:37 -0400 Subject: [PATCH 6/6] 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] --- internal/config/config.go | 11 ++++ internal/config/relocate.go | 10 +++- internal/config/relocate_test.go | 94 ++++++++++++++++++++++++++------ 3 files changed, 94 insertions(+), 21 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 8499f8d..e9e85db 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -307,6 +307,17 @@ func LoadConfig() (*Config, error) { if jerr := loadLegacyJSON(cfg); jerr != nil { return nil, jerr } + read = true // loadLegacyJSON either populated cfg or left it empty (fresh install) + } + + // If a relocation conflict happened AND we could not actually read the + // canonical config (e.g. malformed YAML in new, or malformed legacy + // JSON), soft-degrade is unsafe — the runtime command would silently + // fall back to default settings (e.g. swap credential_ref back to the + // default) and mask a real problem. Return nil cfg so + // LoadConfigForRuntime hard-fails instead of warning-and-defaulting. + if !read && relErr != nil { + return nil, relErr } cfg.applyDefaults() diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 11f69aa..c4f0006 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -330,11 +330,15 @@ func firstExistingConfig(dir string) (string, bool) { // LoadConfigForRuntime is the soft-conflict variant of LoadConfig for non-init // callers. On ErrRelocationConflict it prints a one-shot stderr warning, then -// returns the canonical (new-dir) config so the command can keep working. -// Init uses strict LoadConfig (fail-loud) at its relocation gate. +// returns the canonical (new-dir) config so the command can keep working — +// BUT only when a canonical config was actually read. If LoadConfig couldn't +// populate cfg (e.g. malformed YAML/JSON on the canonical side), the runtime +// must hard-fail instead of warning-and-defaulting, otherwise it would +// silently swap CredentialRef etc. back to defaults and mask the corrupt +// file. Init uses strict LoadConfig (fail-loud) at its relocation gate. func LoadConfigForRuntime() (*Config, error) { cfg, err := LoadConfig() - if err != nil && errors.Is(err, ErrRelocationConflict) { + if err != nil && errors.Is(err, ErrRelocationConflict) && cfg != nil { warnReloConflictOnce(err) return cfg, nil } diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index c067f39..5e37535 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -399,42 +399,71 @@ func TestApplyConfigRelocation_IdempotentSkipsExistingNew(t *testing.T) { } // loadConfigForRuntimeAt is the testable seam for the runtime soft-conflict -// wrapper. It wires both detectRelocation and the YAML load through an -// injected newDir + oldDir, which is the only way to exercise the divergent -// branch on a platform where os.UserConfigDir == $XDG_CONFIG_HOME (Linux). -// Production code paths use LoadConfigForRuntime via the real resolver. +// wrapper. It mirrors LoadConfig + LoadConfigForRuntime against an injected +// newDir + oldDir so the divergent and malformed-canonical branches can be +// exercised on a platform where os.UserConfigDir == $XDG_CONFIG_HOME +// (Linux). Production code paths use LoadConfigForRuntime via the real +// resolver. func loadConfigForRuntimeAt(t *testing.T, oldDir, newDir string) (*Config, error) { t.Helper() t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) // Reset the warn-once before the test so a previous test's flag doesn't // suppress the warning we want to verify. reloConflictOnce = sync.Once{} - r, err := detectRelocation(newDir) - if err != nil && !errors.Is(err, ErrRelocationConflict) { - return nil, err + + r, derr := detectRelocation(newDir) + relErr := error(nil) + if derr != nil && errors.Is(derr, ErrRelocationConflict) { + relErr = derr + } else if derr != nil { + return nil, derr } + cfg := &Config{} - if newPath, ok := firstExistingConfig(r.NewPath); ok { - c, lerr := loadConfigFromFile(newPath) - if lerr != nil { - return nil, lerr + read := false + // Attempt the new-dir read unconditionally so callers soft-degrading + // get the user's actual settings alongside relErr. + if r.NewPath != "" { + if newPath, ok := firstExistingConfig(r.NewPath); ok { + c, lerr := loadConfigFromFile(newPath) + if lerr != nil { + if relErr == nil { + return nil, lerr + } + // Suppress parse-error propagation under conflict so the + // caller can still soft-degrade — IF a config was readable. + } else { + cfg = &c + read = true + } } - cfg = &c - } else if r.Kind == relocOldOnly { + } + if !read && r.Kind == relocOldOnly && r.OldPath != "" { if oldPath, ok := firstExistingConfig(r.OldPath); ok { c, lerr := loadConfigFromFile(oldPath) if lerr != nil { - return nil, lerr + if relErr == nil { + return nil, lerr + } + } else { + cfg = &c + read = true } - cfg = &c } } + // Mirror LoadConfig's malformed-under-conflict hard-fail: if we couldn't + // read the canonical config, soft-degrade would silently swap defaults. + if !read && relErr != nil { + return nil, relErr + } cfg.applyDefaults() - if errors.Is(err, ErrRelocationConflict) { - warnReloConflictOnce(err) + + // LoadConfigForRuntime soft-degrade. + if relErr != nil { + warnReloConflictOnce(relErr) return cfg, nil } - return cfg, err + return cfg, nil } func TestLoadConfigForRuntime_SoftConflict_ReturnsCanonical(t *testing.T) { @@ -460,6 +489,35 @@ func TestLoadConfigForRuntime_SoftConflict_ReturnsCanonical(t *testing.T) { } } +func TestLoadConfigForRuntime_MalformedCanonicalUnderConflict_HardFails(t *testing.T) { + // Both old and new present; new is malformed. detectRelocation returns + // ErrRelocationConflict; LoadConfig cannot read the canonical config; + // LoadConfigForRuntime must hard-fail instead of returning all-defaults + // (which would silently swap CredentialRef back to the default). + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: google-readonly/old\n"}, + {newDir, "not-valid-yaml: : :\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, ConfigFileYAML), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + cfg, err := loadConfigForRuntimeAt(t, oldDir, newDir) + if err == nil { + t.Fatalf("malformed canonical config under conflict must hard-fail, got cfg=%+v err=nil", cfg) + } + if !errors.Is(err, ErrRelocationConflict) { + t.Errorf("error must wrap ErrRelocationConflict, got %v", err) + } + if cfg != nil { + t.Errorf("cfg must be nil on hard-fail, got %+v", cfg) + } +} + func TestLoadConfigForRuntime_NoConflict_PassesThrough(t *testing.T) { oldDir, newDir := reloctest(t) if err := os.MkdirAll(newDir, 0o700); err != nil {