Fix test isolation to prevent config file modifications (#1667)

Problem:
Tests were modifying the real configuration file at ~/.config/toolhive/config.yaml
because they were using the global singleton config pattern, which automatically
loads and saves to the default XDG config path.

Root Cause:
1. The singleton pattern in pkg/config was being triggered by tests
2. LoadOrCreateConfigWithPath had a critical bug where it called config.save()
   instead of config.saveToPath(configPath), causing even tests with temp
   directories to save to the real config location
3. Tests were directly calling config.GetConfig() which triggers singleton
   initialization with the real config file

Solution:
1. Introduced dependency injection with config.Provider interface
   - DefaultProvider: Uses the singleton (for production)
   - PathProvider: Uses a specific path (for tests)
2. Fixed the critical bug in LoadOrCreateConfigWithPath (line 179)
   - Changed from config.save() to config.saveToPath(configPath)
3. Updated all components to accept config providers:
   - API routes (registry, secrets)
   - Client manager
   - Workloads manager
   - Registry factory
   - Runner components
   - Migration utilities
4. Modified all tests to use CreateTestConfigProvider() helper
   - Creates temporary directories for test isolation
   - Uses PathProvider to avoid singleton initialization
5. Fixed race conditions in parallel tests by ensuring each
   subtest has its own mock controller

Impact:
- Tests no longer modify the real config file
- Each test runs in complete isolation with its own config
- Parallel test execution is now safe
- Production code continues to use the singleton pattern
- Backward compatibility maintained for existing code

Verified:
All 56 test functions in pkg/api/v1 and pkg/registry now pass
without modifying ~/.config/toolhive/config.yaml

Signed-off-by: Juan Antonio Osorio <[email protected]>
Co-authored-by: Don Browne <[email protected]>

Author: JAORMX

cmd/thv/app/config.go

@@ -130,7 +130,8 @@ func setCACertCmdFunc(_ *cobra.Command, args []string) error {
 }
 
 func getCACertCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.CACertificatePath == "" {
 		fmt.Println("No CA certificate is currently configured.")
@@ -148,7 +149,8 @@ func getCACertCmdFunc(_ *cobra.Command, _ []string) error {
 }
 
 func unsetCACertCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.CACertificatePath == "" {
 		fmt.Println("No CA certificate is currently configured.")
@@ -171,9 +173,11 @@ func setRegistryCmdFunc(_ *cobra.Command, args []string) error {
 	input := args[0]
 	registryType, cleanPath := config.DetectRegistryType(input)
 
+	provider := config.NewDefaultProvider()
+
 	switch registryType {
 	case config.RegistryTypeURL:
-		err := config.SetRegistryURL(cleanPath, allowPrivateRegistryIp)
+		err := provider.SetRegistryURL(cleanPath, allowPrivateRegistryIp)
 		if err != nil {
 			return err
 		}
@@ -188,14 +192,15 @@ func setRegistryCmdFunc(_ *cobra.Command, args []string) error {
 		}
 		return nil
 	case config.RegistryTypeFile:
-		return config.SetRegistryFile(cleanPath)
+		return provider.SetRegistryFile(cleanPath)
 	default:
 		return fmt.Errorf("unsupported registry type")
 	}
 }
 
 func getRegistryCmdFunc(_ *cobra.Command, _ []string) error {
-	url, localPath, _, registryType := config.GetRegistryConfig()
+	provider := config.NewDefaultProvider()
+	url, localPath, _, registryType := provider.GetRegistryConfig()
 
 	switch registryType {
 	case config.RegistryTypeURL:
@@ -213,14 +218,15 @@ func getRegistryCmdFunc(_ *cobra.Command, _ []string) error {
 }
 
 func unsetRegistryCmdFunc(_ *cobra.Command, _ []string) error {
-	url, localPath, _, registryType := config.GetRegistryConfig()
+	provider := config.NewDefaultProvider()
+	url, localPath, _, registryType := provider.GetRegistryConfig()
 
 	if registryType == "default" {
 		fmt.Println("No custom registry is currently configured.")
 		return nil
 	}
 
-	err := config.UnsetRegistry()
+	err := provider.UnsetRegistry()
 	if err != nil {
 		return fmt.Errorf("failed to update configuration: %w", err)
 	}

cmd/thv/app/otel.go

@@ -136,7 +136,8 @@ func setOtelEndpointCmdFunc(_ *cobra.Command, args []string) error {
 }
 
 func getOtelEndpointCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.OTEL.Endpoint == "" {
 		fmt.Println("No OpenTelemetry endpoint is currently configured.")
@@ -148,7 +149,8 @@ func getOtelEndpointCmdFunc(_ *cobra.Command, _ []string) error {
 }
 
 func unsetOtelEndpointCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.OTEL.Endpoint == "" {
 		fmt.Println("No OpenTelemetry endpoint is currently configured.")
@@ -191,7 +193,8 @@ func setOtelSamplingRateCmdFunc(_ *cobra.Command, args []string) error {
 }
 
 func getOtelSamplingRateCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.OTEL.SamplingRate == 0.0 {
 		fmt.Println("No OpenTelemetry sampling rate is currently configured.")
@@ -203,7 +206,8 @@ func getOtelSamplingRateCmdFunc(_ *cobra.Command, _ []string) error {
 }
 
 func unsetOtelSamplingRateCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if cfg.OTEL.SamplingRate == 0.0 {
 		fmt.Println("No OpenTelemetry sampling rate is currently configured.")
@@ -243,7 +247,8 @@ func setOtelEnvVarsCmdFunc(_ *cobra.Command, args []string) error {
 }
 
 func getOtelEnvVarsCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if len(cfg.OTEL.EnvVars) == 0 {
 		fmt.Println("No OpenTelemetry environment variables are currently configured.")
@@ -255,7 +260,8 @@ func getOtelEnvVarsCmdFunc(_ *cobra.Command, _ []string) error {
 }
 
 func unsetOtelEnvVarsCmdFunc(_ *cobra.Command, _ []string) error {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	if len(cfg.OTEL.EnvVars) == 0 {
 		fmt.Println("No OpenTelemetry environment variables are currently configured.")

cmd/thv/app/run_flags.go

@@ -286,7 +286,8 @@ func setupOIDCConfiguration(cmd *cobra.Command, runFlags *RunFlags) (*auth.Token
 
 // setupTelemetryConfiguration sets up telemetry configuration with config fallbacks
 func setupTelemetryConfiguration(cmd *cobra.Command, runFlags *RunFlags) *telemetry.Config {
-	config := cfg.GetConfig()
+	configProvider := cfg.NewDefaultProvider()
+	config := configProvider.GetConfig()
 	finalOtelEndpoint, finalOtelSamplingRate, finalOtelEnvironmentVariables := getTelemetryFromFlags(cmd, config,
 		runFlags.OtelEndpoint, runFlags.OtelSamplingRate, runFlags.OtelEnvironmentVariables)
 
@@ -306,7 +307,8 @@ func setupRuntimeAndValidation(ctx context.Context) (runtime.Deployer, runner.En
 	if process.IsDetached() || runtime.IsKubernetesRuntime() {
 		envVarValidator = &runner.DetachedEnvVarValidator{}
 	} else {
-		envVarValidator = &runner.CLIEnvVarValidator{}
+		cfgProvider := cfg.NewDefaultProvider()
+		envVarValidator = runner.NewCLIEnvVarValidator(cfgProvider)
 	}
 
 	return rt, envVarValidator, nil

cmd/thv/app/secret.go

@@ -167,7 +167,9 @@ Note that some providers (like 1Password) are read-only and do not support setti
 
 			// Check if the provider supports writing secrets
 			if !manager.Capabilities().CanWrite {
-				providerType, _ := config.GetConfig().Secrets.GetProviderType()
+				configProvider := config.NewDefaultProvider()
+				cfg := configProvider.GetConfig()
+				providerType, _ := cfg.Secrets.GetProviderType()
 				fmt.Fprintf(os.Stderr, "Error: The %s secrets provider does not support setting secrets (read-only)\n", providerType)
 				return
 			}
@@ -250,7 +252,9 @@ If your provider is read-only or doesn't support deletion, this command returns
 
 			// Check if the provider supports deleting secrets
 			if !manager.Capabilities().CanDelete {
-				providerType, _ := config.GetConfig().Secrets.GetProviderType()
+				configProvider := config.NewDefaultProvider()
+				cfg := configProvider.GetConfig()
+				providerType, _ := cfg.Secrets.GetProviderType()
 				fmt.Fprintf(os.Stderr, "Error: The %s secrets provider does not support deleting secrets\n", providerType)
 				return
 			}
@@ -284,7 +288,9 @@ If descriptions exist for the secrets, the command displays them alongside the n
 
 			// Check if the provider supports listing secrets
 			if !manager.Capabilities().CanList {
-				providerType, _ := config.GetConfig().Secrets.GetProviderType()
+				configProvider := config.NewDefaultProvider()
+				cfg := configProvider.GetConfig()
+				providerType, _ := cfg.Secrets.GetProviderType()
 				fmt.Fprintf(os.Stderr, "Error: The %s secrets provider does not support listing secrets\n", providerType)
 				return
 			}
@@ -344,7 +350,8 @@ This command only works with the 'encrypted' secrets provider.`,
 }
 
 func getSecretsManager() (secrets.Provider, error) {
-	cfg := config.GetConfig()
+	configProvider := config.NewDefaultProvider()
+	cfg := configProvider.GetConfig()
 
 	// Check if secrets setup has been completed
 	if !cfg.Secrets.SetupCompleted {

pkg/api/v1/groups_test.go

@@ -277,17 +277,21 @@ func TestGroupsRouter_Integration(t *testing.T) {
 	logger.Initialize()
 
 	// Test with real managers (integration test)
+	// Use a test config provider to avoid modifying the real config file
+	configProvider, cleanup := CreateTestConfigProvider(t, nil)
+	t.Cleanup(cleanup)
+
 	groupManager, err := groups.NewManager()
 	if err != nil {
 		t.Skip("Skipping integration test: failed to create group manager")
 	}
 
-	workloadManager, err := workloads.NewManager(context.Background())
+	workloadManager, err := workloads.NewManagerWithProvider(context.Background(), configProvider)
 	if err != nil {
 		t.Skip("Skipping integration test: failed to create workload manager")
 	}
 
-	clientManager, err := client.NewManager(context.Background())
+	clientManager, err := client.NewManagerWithProvider(context.Background(), configProvider)
 	if err != nil {
 		t.Skip("Skipping integration test: failed to create client manager")
 	}

pkg/api/v1/healtcheck_test.go

@@ -15,20 +15,19 @@ import (
 func TestGetHealthcheck(t *testing.T) {
 	t.Parallel()
 
-	// Create a new gomock controller
-	ctrl := gomock.NewController(t)
-	t.Cleanup(func() {
-		ctrl.Finish()
-	})
-
-	// Create a mock runtime
-	mockRuntime := mocks.NewMockRuntime(ctrl)
-
-	// Create healthcheck routes with the mock runtime
-	routes := &healthcheckRoutes{containerRuntime: mockRuntime}
-
 	t.Run("returns 204 when runtime is running", func(t *testing.T) {
 		t.Parallel()
+		// Create a new gomock controller for this subtest
+		ctrl := gomock.NewController(t)
+		t.Cleanup(func() {
+			ctrl.Finish()
+		})
+
+		// Create a mock runtime
+		mockRuntime := mocks.NewMockRuntime(ctrl)
+
+		// Create healthcheck routes with the mock runtime
+		routes := &healthcheckRoutes{containerRuntime: mockRuntime}
 
 		// Setup mock to return nil (no error) when IsRunning is called
 		mockRuntime.EXPECT().
@@ -49,6 +48,17 @@ func TestGetHealthcheck(t *testing.T) {
 
 	t.Run("returns 503 when runtime is not running", func(t *testing.T) {
 		t.Parallel()
+		// Create a new gomock controller for this subtest
+		ctrl := gomock.NewController(t)
+		t.Cleanup(func() {
+			ctrl.Finish()
+		})
+
+		// Create a mock runtime
+		mockRuntime := mocks.NewMockRuntime(ctrl)
+
+		// Create healthcheck routes with the mock runtime
+		routes := &healthcheckRoutes{containerRuntime: mockRuntime}
 
 		// Create an error to return
 		expectedError := errors.New("container runtime is not available")