fix(configure): preserve existing config instead of overwriting it

CHANGELOG.md

@@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file.
 
 ### Fixed
 
+- `grant configure` no longer discards your saved favorites, `default_provider` and `cache_ttl` when re-run
 - `grant favorites add` now fails immediately without a terminal instead of authenticating first
 - `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags
 - Interactive selectors now elevate the row you picked, not another target or Entra ID group that happens to render the same way

CLAUDE.md

@@ -176,7 +176,8 @@ Custom `SCAAccessService` follows SDK conventions:
 ## Config
 - App config: `~/.grant/config.yaml`
 - SDK profile: `~/.idsec/profiles/grant` (default; override via `IDSEC_PROFILES_FOLDER`)
-- **`runConfigure` rebuilds the config from scratch** (`cmd/configure.go`): it never reads the existing file, it constructs a fresh `&config.Config{}` with a hardcoded `default_provider` and an empty `Favorites` map and `Save`s that, so every favorite and the user's `default_provider` are silently destroyed. That is also what keeps `grant configure` reachable when the on-disk config is unloadable (the no-lockout property, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`), so it is not purely a bug — but it means **`grant configure` must never be advertised as the remedy for a bad config value**. The user-facing remedy is to edit the file, whose path every load error names. Known sharp edge, follow-up, deliberately not fixed here
+- **`runConfigure` merges onto the existing config** (`cmd/configure.go`): it calls `config.Load` and overwrites only what it owns (`profile`), so favorites, `default_provider` and `cache_ttl` survive a re-run (`TestConfigure_PreservesExistingConfigOnRerun`). `Load` treats an absent file as success returning `DefaultConfig()`, so a first run merges onto defaults (`TestConfigure_FirstRunMergesOntoDefaults`)
+- **The rebuild-from-defaults fallback is the no-lockout property** and only fires when `Load` *fails*: configure must stay usable when the on-disk config is unloadable, so it warns on stderr and writes a fresh `DefaultConfig()` over the top (`TestConfigure_UnloadableConfigIsRebuiltFromDefaults`). Do not make configure fail on an unloadable config — that locks the user out of the one command that can repair it. On that path the old file cannot be read, so its favorites are genuinely lost; therefore **`grant configure` must still never be advertised as the remedy for a bad config value**. The user-facing remedy is to edit the file, whose path every load error names
 - Always resolve the profile directory with `profiles.GetProfilesFolder()` (SDK) — never hand-roll it. The SDK reads `os.Getenv("HOME")`, not `os.UserHomeDir()`; on Windows `HOME` is frequently unset, so it resolves to a **relative** `.idsec/profiles` under the process CWD. Any code that prints or computes the profile path must agree with the loader, so reproduce the SDK's behavior rather than "correcting" it
 
 ## Keyring

cmd/configure.go

@@ -114,19 +114,34 @@ func runConfigure(cmd *cobra.Command, saver profileSaver, tenantURL, username st
 	profileDir := profiles.GetProfilesFolder()
 	profilePath := filepath.Join(profileDir, "grant")
 
-	// Create app config
-	cfg := &config.Config{
-		Profile:         "grant",
-		DefaultProvider: "azure",
-		Favorites:       make(map[string]config.Favorite),
-	}
-
-	// Save app config
-	log.Info("Saving config...")
+	// Merge onto the existing app config so a re-run keeps everything the user
+	// was not just prompted for (favorites, default_provider, cache_ttl).
 	cfgPath, err := config.ConfigPath()
 	if err != nil {
 		return err
 	}
+	cfg, err := config.Load(cfgPath)
+	if err != nil {
+		// The file exists but cannot be used. Rebuilding from defaults is what
+		// keeps configure reachable with a broken config (the no-lockout
+		// property) — but it drops the rest of the file, so say so rather than
+		// doing it silently. configure is still not the advertised remedy for a
+		// bad value; editing the file named in the load error is.
+		fmt.Fprintf(cmd.ErrOrStderr(),
+			"Warning: existing config %s could not be read (%v); rebuilding it from defaults, any favorites in it are lost\n",
+			cfgPath, err)
+		cfg = config.DefaultConfig()
+	}
+	cfg.Profile = "grant"
+	if cfg.Favorites == nil {
+		cfg.Favorites = make(map[string]config.Favorite)
+	}
+	if cfg.DefaultProvider == "" {
+		cfg.DefaultProvider = config.DefaultConfig().DefaultProvider
+	}
+
+	// Save app config
+	log.Info("Saving config...")
 	if err := config.Save(cfg, cfgPath); err != nil {
 		return fmt.Errorf("failed to save config: %w", err)
 	}

cmd/configure_test.go

@@ -515,21 +515,112 @@ func TestConfigureLongHelpHasNoLegacyPath(t *testing.T) {
 	}
 }
 
-// TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites pins what
-// `grant configure` actually does to a config it cannot load: it never reads the
-// old file, it builds a fresh Config from scratch and writes that over the top.
+// TestConfigure_PreservesExistingConfigOnRerun pins that a re-run of
+// `grant configure` over a LOADABLE config keeps everything the user did not
+// answer a prompt for. configure prompts for the username and Identity URL
+// only; favorites, default_provider and cache_ttl live in the same file and
+// must survive untouched.
 //
-// That keeps configure reachable with a broken config — the no-lockout property
-// — but it is NOT an endorsed recovery path for a bad cache_ttl, and no error
-// text, doc or help string should point a user at it. runConfigure discards
-// every favorite and resets default_provider, so using it to fix a
-// one-character typo silently destroys unrelated config. The documented remedy
-// is to edit the file named in the load error. The favorites assertion below
-// pins that loss so it stays visible; it is recorded behavior, not desired
-// behavior, and is flagged as a follow-up in CLAUDE.md.
+// Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process.
+func TestConfigure_PreservesExistingConfigOnRerun(t *testing.T) {
+	dir := t.TempDir()
+	cfgPath := filepath.Join(dir, "config.yaml")
+	existing := "profile: grant\ndefault_provider: aws\ncache_ttl: 30m\n" +
+		"favorites:\n  prod:\n    provider: azure\n    target: Prod-EastUS\n    role: Contributor\n"
+	if err := os.WriteFile(cfgPath, []byte(existing), 0o600); err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+	t.Setenv("GRANT_CONFIG", cfgPath)
+	t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(dir, "profiles"))
+
+	cmd := NewConfigureCommandWithDeps(&mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com")
+	if _, err := executeCommand(cmd); err != nil {
+		t.Fatalf("configure() error = %v, want nil", err)
+	}
+
+	cfg, err := config.Load(cfgPath)
+	if err != nil {
+		t.Fatalf("Load() after configure = %v", err)
+	}
+	if cfg.DefaultProvider != "aws" {
+		t.Errorf("default_provider = %q after configure, want %q preserved", cfg.DefaultProvider, "aws")
+	}
+	if cfg.CacheTTL != "30m" {
+		t.Errorf("cache_ttl = %q after configure, want %q preserved", cfg.CacheTTL, "30m")
+	}
+	fav, ok := cfg.Favorites["prod"]
+	if !ok {
+		t.Fatalf("favorites = %v after configure, want the %q favorite preserved", cfg.Favorites, "prod")
+	}
+	want := config.Favorite{Provider: "azure", Target: "Prod-EastUS", Role: "Contributor"}
+	if fav != want {
+		t.Errorf("favorites[%q] = %+v, want %+v", "prod", fav, want)
+	}
+	if cfg.Profile != "grant" {
+		t.Errorf("profile = %q after configure, want %q", cfg.Profile, "grant")
+	}
+}
+
+// TestConfigure_FirstRunMergesOntoDefaults pins the absent-config case:
+// config.Load reports os.ErrNotExist as a SUCCESS returning DefaultConfig(), so
+// a first run merges onto defaults rather than taking the unloadable fallback.
 //
 // Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process.
-func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) {
+func TestConfigure_FirstRunMergesOntoDefaults(t *testing.T) {
+	dir := t.TempDir()
+	cfgPath := filepath.Join(dir, "config.yaml")
+	t.Setenv("GRANT_CONFIG", cfgPath)
+	t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(dir, "profiles"))
+
+	cmd := NewConfigureCommandWithDeps(&mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com")
+	out, err := executeCommand(cmd)
+	if err != nil {
+		t.Fatalf("configure() error = %v, want nil", err)
+	}
+	// The rebuild warning belongs to the unloadable path only; an absent file
+	// is a normal first run, not a recovery.
+	if strings.Contains(out, "could not be read") {
+		t.Errorf("first run warned about an unreadable config\ngot:\n%s", out)
+	}
+
+	cfg, err := config.Load(cfgPath)
+	if err != nil {
+		t.Fatalf("Load() after configure = %v", err)
+	}
+	if cfg.Profile != "grant" {
+		t.Errorf("profile = %q, want %q", cfg.Profile, "grant")
+	}
+	if cfg.DefaultProvider != "azure" {
+		t.Errorf("default_provider = %q, want the default %q", cfg.DefaultProvider, "azure")
+	}
+	if cfg.Favorites == nil {
+		t.Error("favorites = nil, want an initialized map")
+	}
+	if len(cfg.Favorites) != 0 {
+		t.Errorf("favorites = %v, want empty on a first run", cfg.Favorites)
+	}
+	if cfg.CacheTTL != "" {
+		t.Errorf("cache_ttl = %q, want empty on a first run", cfg.CacheTTL)
+	}
+}
+
+// TestConfigure_UnloadableConfigIsRebuiltFromDefaults pins what `grant
+// configure` does to a config it cannot load: it falls back to a fresh Config
+// and writes that over the top.
+//
+// That is what keeps configure reachable with a broken config — the no-lockout
+// property — but it is NOT an endorsed recovery path for a bad cache_ttl, and
+// no error text, doc or help string should point a user at it. On this path the
+// old file cannot be read at all, so favorites and default_provider are lost;
+// the assertions below pin that loss so it stays visible, and configure warns
+// on stderr rather than doing it silently. The documented remedy for a bad
+// value is still to edit the file named in the load error.
+//
+// (A LOADABLE config is preserved instead — see
+// TestConfigure_PreservesExistingConfigOnRerun.)
+//
+// Not parallel: sets GRANT_CONFIG and IDSEC_PROFILES_FOLDER for the process.
+func TestConfigure_UnloadableConfigIsRebuiltFromDefaults(t *testing.T) {
 	dir := t.TempDir()
 	cfgPath := filepath.Join(dir, "config.yaml")
 	broken := "profile: grant\ndefault_provider: aws\ncache_ttl: garbage\n" +
@@ -542,9 +633,20 @@ func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) {
 
 	cmd := NewConfigureCommand()
 	cmd.SetOut(&strings.Builder{})
+	stderr := &strings.Builder{}
+	cmd.SetErr(stderr)
 	err := runConfigure(cmd, &mockProfileSaver{}, "https://example.cyberark.cloud", "test.user@example.com")
 	if err != nil {
-		t.Fatalf("runConfigure() error = %v, want nil; configure must not read the broken config", err)
+		t.Fatalf("runConfigure() error = %v, want nil; a broken config must not lock the user out of configure", err)
+	}
+
+	// The rebuild must be announced, not silent: this is the one path that
+	// still drops the user's other settings.
+	if !strings.Contains(stderr.String(), "could not be read") {
+		t.Errorf("stderr missing the rebuild warning\ngot:\n%s", stderr.String())
+	}
+	if !strings.Contains(stderr.String(), cfgPath) {
+		t.Errorf("stderr warning does not name the config path %q\ngot:\n%s", cfgPath, stderr.String())
 	}
 
 	// The broken value must be gone and the rewritten file must now load.
@@ -556,11 +658,11 @@ func TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites(t *testing.T) {
 		t.Errorf("cache_ttl = %q after configure, want it rewritten away", cfg.CacheTTL)
 	}
 
-	// The sharp edge: everything else in the file went with it. Pinned so a
-	// future change to runConfigure has to acknowledge this, and so nobody
+	// The remaining sharp edge: on this path the old file cannot be read, so
+	// everything else in it goes with the broken value. Pinned so nobody
 	// mistakes configure for a safe repair tool.
 	if len(cfg.Favorites) != 0 {
-		t.Errorf("favorites = %v after configure, want them clobbered (pinned pre-existing behavior)", cfg.Favorites)
+		t.Errorf("favorites = %v after configure, want them gone (unreadable config cannot be merged)", cfg.Favorites)
 	}
 	if cfg.DefaultProvider != "azure" {
 		t.Errorf("default_provider = %q after configure, want the hardcoded %q (the user's \"aws\" is lost)", cfg.DefaultProvider, "azure")

docs/mutation-ledger.md

@@ -220,7 +220,7 @@ premise does not hold).
 | CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | done |
 | CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`, `TestSessionTimestampRetention_IsTwentyFourHours`, `TestCleanupSessions_IgnoresRetention`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | done |
 | CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | `TestLoad_UnreadableIsNotTreatedAsMissing`: `Load(<dir>)` errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | done |
-| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately, so it stays reachable with a broken file — but it is **not** the advertised remedy: it rebuilds the config from scratch and drops favorites and `default_provider`, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`. The user-facing remedy is to edit the file named in the load error) | PR6 | done |
+| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-1h`, `garbage` plus a `30m` positive control; `TestLoad_InvalidCacheTTLErrors`; `TestBuildCachedLister_TTL` in `cmd`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go`. CHANGELOG `### Changed`. **Ripple coverage, honestly:** the two `request_submit.go` sites are covered — they discarded the `Load` error and substituted `DefaultConfig()`, which made `request submit` the one command where an invalid `cache_ttl` was neither honored nor reported; they now propagate it (`TestResolveSubmit_ConfigLoadErrorPropagates`), and the cache construction is extracted into `buildCachedRolesLister` so its bad-TTL arm is reachable (`TestBuildCachedRolesLister_TTL`). The **seven `buildCachedLister` call sites' error propagation is NOT pinned**: mutating each to `cachedLister, _ := ...` all SURVIVED. They sit in production `RunE` closures that unit tests cannot reach because `bootstrapImpl` is stubbed to fail first. Static reading confirms all eight sites are correct — no `if err != nil { ttl = Default }` anywhere, and no remaining discard. Deliberately **not** fixed by restructuring production code to make them testable: the payoff is a duplicated propagation check on a path `config.Load` validation already covers. Note also that `config.Load` is not reached "at startup" — commands that authenticate first (`status`, `list`, root, `env`, `request submit`) surface an auth failure before the config error when unauthenticated, and `configure` never calls `Load` at all (deliberately, so it stays reachable with a broken file — but it is **not** the advertised remedy: it rebuilds the config from scratch and drops favorites and `default_provider`, pinned by `TestConfigure_OverwritesInvalidCacheTTLAndClobbersFavorites`. The user-facing remedy is to edit the file named in the load error). *Superseded:* `configure` now calls `Load` and merges onto a successful result, falling back to defaults (with a stderr warning) only when the load fails; the no-lockout property and "not the advertised remedy" guidance are unchanged, and the pinning test is now `TestConfigure_UnloadableConfigIsRebuiltFromDefaults` | PR6 | done |
 | CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | done |
 | CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | done |
 | CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | done |