Persist config backward-compat migrations to load path (#5416)

Loading a config from an explicit path (PathProvider, LoadOrCreate-
ConfigWithPath) that needed a backward-compatibility migration wrote
the migrated result to the default xdg path instead of the path it
was loaded from. applyBackwardCompatibility called the path-agnostic
save(), which always resolves to getConfigPath().

For tests this clobbered the developer's real config file whenever a
PathProvider loaded a temp config with a secrets provider set but
setup_completed false, defeating the isolation added for #894. It was
also a latent correctness bug for any path-based load in production.

Thread the load path through applyBackwardCompatibility and persist
via saveToPath(configPath). Remove the now-unused save() helper.

Closes #894

Co-authored-by: Dan Barr <6922515+danbarr@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: danbarr

pkg/config/config.go

@@ -190,8 +190,11 @@ func createNewConfigWithDefaults() Config {
 	}
 }
 
-// applyBackwardCompatibility applies backward compatibility fixes to existing configs
-func applyBackwardCompatibility(config *Config) error {
+// applyBackwardCompatibility applies backward compatibility fixes to existing configs.
+// Any migration that needs to be persisted is written back to configPath, the same
+// path the config was loaded from, so that path-based loads (e.g. PathProvider) stay
+// isolated. An empty configPath falls back to the default path via saveToPath.
+func applyBackwardCompatibility(config *Config, configPath string) error {
 	// Hack - if the secrets provider type is set to the old `basic` type,
 	// just change it to `encrypted`.
 	if config.Secrets.ProviderType == "basic" {
@@ -202,7 +205,7 @@ func applyBackwardCompatibility(config *Config) error {
 			_ = os.Remove(oldPath)
 		}
 		config.Secrets.ProviderType = string(secrets.EncryptedType)
-		err = config.save()
+		err = config.saveToPath(configPath)
 		if err != nil {
 			return fmt.Errorf("error updating config: %w", err)
 		}
@@ -212,7 +215,7 @@ func applyBackwardCompatibility(config *Config) error {
 	// consider it as setup completed (for existing users)
 	if config.Secrets.ProviderType != "" && !config.Secrets.SetupCompleted {
 		config.Secrets.SetupCompleted = true
-		err := config.save()
+		err := config.saveToPath(configPath)
 		if err != nil {
 			return fmt.Errorf("error updating config for backward compatibility: %w", err)
 		}
@@ -291,8 +294,9 @@ func LoadOrCreateConfigFromPath(configPath string) (*Config, error) {
 			return nil, fmt.Errorf("failed to parse config file yaml: %w", err)
 		}
 
-		// Apply backward compatibility fixes
-		err = applyBackwardCompatibility(&config)
+		// Apply backward compatibility fixes, persisting any migration back to the
+		// same path the config was loaded from.
+		err = applyBackwardCompatibility(&config, configPath)
 		if err != nil {
 			return nil, fmt.Errorf("failed to apply backward compatibility fixes: %w", err)
 		}
@@ -301,11 +305,6 @@ func LoadOrCreateConfigFromPath(configPath string) (*Config, error) {
 	return &config, nil
 }
 
-// Save serializes the config struct and writes it to disk.
-func (c *Config) save() error {
-	return c.saveToPath("")
-}
-
 // saveToPath serializes the config struct and writes it to a specific path.
 // If configPath is empty, it uses the default path.
 func (c *Config) saveToPath(configPath string) error {

pkg/config/config_test.go

@@ -282,6 +282,46 @@ func TestUpdateConfigAtPath_CallbackError(t *testing.T) {
 		"config should not be written to disk when the callback returns an error")
 }
 
+// TestLoadFromPath_BackwardCompatMigrationStaysOnPath guards the test-isolation
+// regression from issue #894: a path-based load that triggers a backward-compat
+// migration must persist the migration back to the same path, never to the
+// default (real) config path. See applyBackwardCompatibility.
+//
+//nolint:paralleltest // swaps the package-level getConfigPath; must not run in parallel
+func TestLoadFromPath_BackwardCompatMigrationStaysOnPath(t *testing.T) {
+	// Not parallel: this test swaps the package-level getConfigPath sentinel.
+
+	// Point the default path generator at a sentinel that must never be written.
+	sentinelPath := filepath.Join(t.TempDir(), "should-never-exist", "config.yaml")
+	originalGetConfigPath := getConfigPath
+	getConfigPath = func() (string, error) { return sentinelPath, nil }
+	t.Cleanup(func() { getConfigPath = originalGetConfigPath })
+
+	// A config that triggers the "provider set but setup_completed false" migration.
+	_, configPath := SetupTestConfig(t, &Config{
+		Secrets: Secrets{
+			ProviderType:   string(secrets.EncryptedType),
+			SetupCompleted: false,
+		},
+	})
+
+	config, err := LoadOrCreateConfigWithPath(configPath)
+	require.NoError(t, err)
+	assert.True(t, config.Secrets.SetupCompleted,
+		"backward-compat migration should mark setup as completed")
+
+	// The migration must be persisted to the path we loaded from.
+	reloaded, err := LoadOrCreateConfigWithPath(configPath)
+	require.NoError(t, err)
+	assert.True(t, reloaded.Secrets.SetupCompleted,
+		"migration should be persisted back to the loaded path")
+
+	// The default/real config path must never have been touched.
+	_, statErr := os.Stat(sentinelPath)
+	assert.ErrorIs(t, statErr, os.ErrNotExist,
+		"backward-compat migration must not write to the default config path")
+}
+
 func TestSecrets_GetProviderType_EnvironmentVariable(t *testing.T) {
 	t.Parallel()