Replace configuration and secret loading globals

2026-09-05 17:48:22 +00:00
parent 1484f51f48
commit c3307ab5fb
8 changed files with 154 additions and 69 deletions
--- a/docs/roadmap/implementation.md
+++ b/docs/roadmap/implementation.md
@@ -100,7 +100,7 @@ internal document in the same stage only when its owned behavior changes.

 ## Stage 1 — Replace Configuration And Secret-Loading Globals

-**Status:** Pending
+**Status:** Completed

 ### Goal

--- a/internal/app/command_dependencies.go
+++ b/internal/app/command_dependencies.go
@@ -0,0 +1,44 @@
+package app
+
+import (
+	"context"
+	"os"
+
+	"gitea.maximumdirect.net/eric/narratio/internal/adapters/storage"
+	"gitea.maximumdirect.net/eric/narratio/internal/config"
+)
+
+type commandDependencies struct {
+	loadPipelineConfig         func(string, config.PipelineLoadOptions) (*config.PipelineConfig, error)
+	downloadObjectToTemp       func(context.Context, storage.ObjectStore, string, string) (string, error)
+	readSecretsDirectory       func(string) ([]os.DirEntry, error)
+	newObjectStore             func(context.Context, *config.Config) (storage.ObjectStore, error)
+	discoverRemoteCurrentState func(context.Context, *config.Config, storage.ObjectStore) (*RemoteCurrentState, error)
+	buildRestorePlan           func(context.Context, *config.Config, *RemoteCurrentState, storage.ObjectStore, RestorePlanOptions) (*RestorePlan, error)
+	executeRestorePlan         func(context.Context, *config.Config, *RemoteCurrentState, *RestorePlan, *RestoreReport, storage.ObjectStore) (*RestoreExecutionResult, error)
+}
+
+func normalizeCommandDependencies(deps commandDependencies) commandDependencies {
+	if deps.loadPipelineConfig == nil {
+		deps.loadPipelineConfig = config.LoadPipelineWithOptions
+	}
+	if deps.downloadObjectToTemp == nil {
+		deps.downloadObjectToTemp = storage.DownloadObjectToTemp
+	}
+	if deps.readSecretsDirectory == nil {
+		deps.readSecretsDirectory = os.ReadDir
+	}
+	if deps.newObjectStore == nil {
+		deps.newObjectStore = storage.NewObjectStoreFromConfig
+	}
+	if deps.discoverRemoteCurrentState == nil {
+		deps.discoverRemoteCurrentState = discoverRemoteCurrentState
+	}
+	if deps.buildRestorePlan == nil {
+		deps.buildRestorePlan = buildRestorePlan
+	}
+	if deps.executeRestorePlan == nil {
+		deps.executeRestorePlan = executeRestorePlan
+	}
+	return deps
+}
--- a/internal/app/config_loader.go
+++ b/internal/app/config_loader.go
@@ -16,9 +16,6 @@ import (

 type pipelineCampaignConfig = config.LoadedPipelineCampaign

-var downloadObjectToTempFn = storage.DownloadObjectToTemp
-var loadPipelineConfigFn = config.LoadPipelineWithOptions
-
 type commandConfig struct {
 	Config  *config.Config
 	cleanup func() error
@@ -38,6 +35,13 @@ func retainedCommandConfig(cfg *config.Config) *commandConfig {
 }

 func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaignFileFlag, sessionFlag string, sessionOpts config.SessionLoadOptions) (loaded *commandConfig, err error) {
+	return loadCommandConfigWithDependencies(ctx, pipelineFlag, campaignFlag, campaignFileFlag, sessionFlag, sessionOpts, commandDependencies{
+		newObjectStore: newObjectStoreFromConfigFn,
+	})
+}
+
+func loadCommandConfigWithDependencies(ctx context.Context, pipelineFlag, campaignFlag, campaignFileFlag, sessionFlag string, sessionOpts config.SessionLoadOptions, deps commandDependencies) (loaded *commandConfig, err error) {
+	deps = normalizeCommandDependencies(deps)
 	var cleanup func() error
 	defer func() {
 		if err == nil || cleanup == nil {
@@ -48,7 +52,7 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaign
 		}
 	}()

-	base, err := loadPipelineCampaignConfig(pipelineFlag, campaignFlag, campaignFileFlag, config.PipelineLoadOptions{Profile: sessionOpts.Profile})
+	base, err := loadPipelineCampaignConfigWithDependencies(pipelineFlag, campaignFlag, campaignFileFlag, config.PipelineLoadOptions{Profile: sessionOpts.Profile}, deps)
 	if err != nil {
 		return nil, err
 	}
@@ -88,7 +92,7 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaign
 	if err != nil {
 		return nil, err
 	}
-	store, err := newCommandObjectStore(ctx, partialCfg, nil)
+	store, err := newCommandObjectStoreWithDependencies(ctx, partialCfg, nil, deps)
 	if err != nil {
 		return nil, missingSessionConfigError(discoveredSession.Searched, fmt.Sprintf("remote session %q unavailable: %v", remoteKey, err))
 	}
@@ -97,7 +101,7 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaign
 	if err != nil {
 		return nil, missingSessionConfigError(discoveredSession.Searched, err.Error())
 	}
-	sessionTempPath, err := downloadObjectToTempFn(ctx, store, remoteKey, "narratio-session-*.yml")
+	sessionTempPath, err := deps.downloadObjectToTemp(ctx, store, remoteKey, "narratio-session-*.yml")
 	if err != nil {
 		return nil, missingSessionConfigError(discoveredSession.Searched, fmt.Sprintf("remote session %q download failed: %v", remoteKey, err))
 	}
@@ -138,7 +142,12 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaign
 }

 func loadPipelineCampaignConfig(pipelineFlag, campaignFlag, campaignFileFlag string, pipelineOpts config.PipelineLoadOptions) (*pipelineCampaignConfig, error) {
-	loadedPipelinePath, pipelineCfg, err := loadPipelineConfig(pipelineFlag, pipelineOpts)
+	return loadPipelineCampaignConfigWithDependencies(pipelineFlag, campaignFlag, campaignFileFlag, pipelineOpts, commandDependencies{})
+}
+
+func loadPipelineCampaignConfigWithDependencies(pipelineFlag, campaignFlag, campaignFileFlag string, pipelineOpts config.PipelineLoadOptions, deps commandDependencies) (*pipelineCampaignConfig, error) {
+	deps = normalizeCommandDependencies(deps)
+	loadedPipelinePath, pipelineCfg, err := loadPipelineConfigWithDependencies(pipelineFlag, pipelineOpts, deps)
 	if err != nil {
 		return nil, err
 	}
@@ -163,11 +172,16 @@ func loadPipelineCampaignConfig(pipelineFlag, campaignFlag, campaignFileFlag str
 }

 func loadPipelineConfig(pipelineFlag string, opts config.PipelineLoadOptions) (string, *config.PipelineConfig, error) {
+	return loadPipelineConfigWithDependencies(pipelineFlag, opts, commandDependencies{})
+}
+
+func loadPipelineConfigWithDependencies(pipelineFlag string, opts config.PipelineLoadOptions, deps commandDependencies) (string, *config.PipelineConfig, error) {
+	deps = normalizeCommandDependencies(deps)
 	resolvedPipelinePath, err := resolvePipelineConfigPath(pipelineFlag)
 	if err != nil {
 		return "", nil, err
 	}
-	pipelineCfg, err := loadPipelineConfigFn(resolvedPipelinePath, opts)
+	pipelineCfg, err := deps.loadPipelineConfig(resolvedPipelinePath, opts)
 	if err != nil {
 		return "", nil, err
 	}
--- a/internal/app/config_loader_test.go
+++ b/internal/app/config_loader_test.go
@@ -13,29 +13,29 @@ func TestLoadCommandConfigRetainsInitiallyLoadedPipeline(t *testing.T) {
 	workspaceRoot := t.TempDir()
 	pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot)

-	originalLoader := loadPipelineConfigFn
 	loadCalls := 0
-	loadPipelineConfigFn = func(path string, opts config.PipelineLoadOptions) (*config.PipelineConfig, error) {
-		loaded, err := originalLoader(path, opts)
-		if err != nil {
-			return nil, err
-		}
-		loadCalls++
+	deps := commandDependencies{
+		loadPipelineConfig: func(path string, opts config.PipelineLoadOptions) (*config.PipelineConfig, error) {
+			loaded, err := config.LoadPipelineWithOptions(path, opts)
+			if err != nil {
+				return nil, err
+			}
+			loadCalls++

-		data, err := os.ReadFile(path)
-		if err != nil {
-			return nil, err
-		}
-		changedRoot := workspaceRoot + "-changed"
-		updated := strings.ReplaceAll(string(data), workspaceRoot, changedRoot)
-		if err := os.WriteFile(path, []byte(updated), 0o644); err != nil {
-			return nil, err
-		}
-		return loaded, nil
+			data, err := os.ReadFile(path)
+			if err != nil {
+				return nil, err
+			}
+			changedRoot := workspaceRoot + "-changed"
+			updated := strings.ReplaceAll(string(data), workspaceRoot, changedRoot)
+			if err := os.WriteFile(path, []byte(updated), 0o644); err != nil {
+				return nil, err
+			}
+			return loaded, nil
+		},
 	}
-	t.Cleanup(func() { loadPipelineConfigFn = originalLoader })

-	loaded, err := loadCommandConfig(context.Background(), pipelinePath, "", campaignPath, sessionPath, config.SessionLoadOptions{SessionID: "2026-05-03"})
+	loaded, err := loadCommandConfigWithDependencies(context.Background(), pipelinePath, "", campaignPath, sessionPath, config.SessionLoadOptions{SessionID: "2026-05-03"}, deps)
 	if err != nil {
 		t.Fatalf("loadCommandConfig() error = %v", err)
 	}
--- a/internal/app/object_store.go
+++ b/internal/app/object_store.go
@@ -10,10 +10,15 @@ import (
 )

 func newCommandObjectStore(ctx context.Context, cfg *config.Config, logger *slog.Logger) (storage.ObjectStore, error) {
-	if _, err := loadSecretsFromConfig(cfg, logger); err != nil {
+	return newCommandObjectStoreWithDependencies(ctx, cfg, logger, commandDependencies{newObjectStore: newObjectStoreFromConfigFn})
+}
+
+func newCommandObjectStoreWithDependencies(ctx context.Context, cfg *config.Config, logger *slog.Logger, deps commandDependencies) (storage.ObjectStore, error) {
+	deps = normalizeCommandDependencies(deps)
+	if _, err := loadSecretsFromConfigWithDependencies(cfg, logger, deps); err != nil {
 		return nil, fmt.Errorf("load secrets from files: %w", err)
 	}
-	return initializeObjectStoreBackend(ctx, cfg, newObjectStoreFromConfigFn)
+	return initializeObjectStoreBackend(ctx, cfg, deps.newObjectStore)
 }

 func initializeObjectStoreBackend(
--- a/internal/app/remote_session_test.go
+++ b/internal/app/remote_session_test.go
@@ -74,10 +74,10 @@ inputs:
  audio_s3:
    prefix: audio/
 `)
-	var storeInitCalls int
-	restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{filepath.Join(t.TempDir(), "session.yml")})
+	setSessionConfigSearchPaths(t, []string{filepath.Join(t.TempDir(), "session.yml")})
+	deps, storeInitCalls := remoteSessionLoaderDependencies(fake)

-	loaded, err := loadCommandConfig(context.Background(), pipelinePath, "", campaignPath, "", config.SessionLoadOptions{SessionID: "2026-05-03"})
+	loaded, err := loadCommandConfigWithDependencies(context.Background(), pipelinePath, "", campaignPath, "", config.SessionLoadOptions{SessionID: "2026-05-03"}, deps)
 	if err != nil {
 		t.Fatalf("loadCommandConfig() error = %v", err)
 	}
@@ -85,6 +85,9 @@ inputs:
 	if loaded.Config.Party.Canonical == nil {
 		t.Fatalf("remote config party = %#v", loaded.Config.Party)
 	}
+	if *storeInitCalls != 1 {
+		t.Fatalf("object store init calls = %d, want 1", *storeInitCalls)
+	}
 }

 func TestRemoteSessionConfigIsRemovedAfterEveryCommandExit(t *testing.T) {
@@ -153,8 +156,8 @@ inputs:
 			var storeInitCalls int
 			restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{filepath.Join(t.TempDir(), "session.yml")})

-			var downloadedPath string
-			captureRemoteSessionTempPath(t, &downloadedPath)
+			tempRoot := t.TempDir()
+			t.Setenv("TMPDIR", tempRoot)
 			executor := tt.executor
 			if executor == nil {
 				executor = executePlan
@@ -170,11 +173,12 @@ inputs:
 			if !tt.wantSuccessful && code == 0 {
 				t.Fatal("exit code = 0, want non-zero")
 			}
-			if downloadedPath == "" {
-				t.Fatal("remote session download path was not captured")
+			entries, err := os.ReadDir(tempRoot)
+			if err != nil {
+				t.Fatalf("read temp root: %v", err)
 			}
-			if _, err := os.Stat(downloadedPath); !errors.Is(err, os.ErrNotExist) {
-				t.Fatalf("downloaded remote session path still exists or could not be inspected: %q, err=%v", downloadedPath, err)
+			if len(entries) != 0 {
+				t.Fatalf("temporary remote session files remain after command exit: %v", entries)
 			}
 		})
 	}
@@ -189,12 +193,12 @@ inputs:
  audio_s3:
    prefix: audio/
 `)
-	var storeInitCalls int
-	restoreAppConfigTestGlobals(t, fake, &storeInitCalls, []string{filepath.Join(t.TempDir(), "session.yml")})
+	setSessionConfigSearchPaths(t, []string{filepath.Join(t.TempDir(), "session.yml")})
+	deps, _ := remoteSessionLoaderDependencies(fake)

 	var downloadedPath string
-	captureRemoteSessionTempPath(t, &downloadedPath)
-	loaded, err := loadCommandConfig(context.Background(), pipelinePath, "", campaignPath, "", config.SessionLoadOptions{SessionID: "2026-05-03"})
+	deps.downloadObjectToTemp = captureRemoteSessionTempPath(&downloadedPath)
+	loaded, err := loadCommandConfigWithDependencies(context.Background(), pipelinePath, "", campaignPath, "", config.SessionLoadOptions{SessionID: "2026-05-03"}, deps)
 	if err != nil {
 		t.Fatalf("loadCommandConfig() error = %v", err)
 	}
@@ -212,17 +216,14 @@ inputs:
 	}
 }

-func captureRemoteSessionTempPath(t *testing.T, destination *string) {
-	t.Helper()
-	original := downloadObjectToTempFn
-	downloadObjectToTempFn = func(ctx context.Context, store storage.ObjectStore, key, pattern string) (string, error) {
-		path, err := original(ctx, store, key, pattern)
+func captureRemoteSessionTempPath(destination *string) func(context.Context, storage.ObjectStore, string, string) (string, error) {
+	return func(ctx context.Context, store storage.ObjectStore, key, pattern string) (string, error) {
+		path, err := storage.DownloadObjectToTemp(ctx, store, key, pattern)
 		if err == nil {
 			*destination = path
 		}
 		return path, err
 	}
-	t.Cleanup(func() { downloadObjectToTempFn = original })
 }

 func TestExecuteRemoteSessionFallbackLoadsSecretsBeforeObjectStoreInit(t *testing.T) {
@@ -445,6 +446,23 @@ func restoreAppConfigTestGlobals(t *testing.T, fake *storage.FakeBackend, storeI
 	})
 }

+func remoteSessionLoaderDependencies(fake *storage.FakeBackend) (commandDependencies, *int) {
+	storeInitCalls := new(int)
+	return commandDependencies{
+		newObjectStore: func(context.Context, *config.Config) (storage.ObjectStore, error) {
+			*storeInitCalls++
+			return fake, nil
+		},
+	}, storeInitCalls
+}
+
+func setSessionConfigSearchPaths(t *testing.T, paths []string) {
+	t.Helper()
+	original := append([]string(nil), config.DefaultSessionConfigSearchPaths...)
+	config.DefaultSessionConfigSearchPaths = append([]string(nil), paths...)
+	t.Cleanup(func() { config.DefaultSessionConfigSearchPaths = original })
+}
+
 func seedRemoteSessionConfig(t *testing.T, fake *storage.FakeBackend, sessionID, content string) string {
 	t.Helper()
 	sessionPrefix := artifacts.S3SessionPrefix("dnd", "sample-campaign", sessionID)
--- a/internal/app/secrets_env.go
+++ b/internal/app/secrets_env.go
@@ -13,7 +13,6 @@ import (
 )

 var envVarNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
-var readSecretsDirectoryEntries = os.ReadDir

 // APIKeyFileByteLimit bounds one credential value read from the configured
 // secrets directory. API keys are expected to be short single-line values.
@@ -30,6 +29,11 @@ type secretsLoadStats struct {
 }

 func loadSecretsFromConfig(cfg *config.Config, logger *slog.Logger) (*secretsLoadStats, error) {
+	return loadSecretsFromConfigWithDependencies(cfg, logger, commandDependencies{})
+}
+
+func loadSecretsFromConfigWithDependencies(cfg *config.Config, logger *slog.Logger, deps commandDependencies) (*secretsLoadStats, error) {
+	deps = normalizeCommandDependencies(deps)
 	if cfg == nil || cfg.Pipeline == nil || cfg.Pipeline.Secrets == nil {
 		return nil, nil
 	}
@@ -60,7 +64,7 @@ func loadSecretsFromConfig(cfg *config.Config, logger *slog.Logger) (*secretsLoa
 		return nil, fmt.Errorf("API-key loader: validate secrets env_dir: %w", err)
 	}

-	entries, err := readSecretsDirectoryEntries(resolvedDir)
+	entries, err := deps.readSecretsDirectory(resolvedDir)
 	if err != nil {
 		return nil, fmt.Errorf("API-key loader: read secrets env_dir %q: %w", resolvedDir, err)
 	}
--- a/internal/app/secrets_env_test.go
+++ b/internal/app/secrets_env_test.go
@@ -237,25 +237,25 @@ func TestLoadSecretsFromConfigRejectsSymlinkAndAncestorReplacement(t *testing.T)
 		t.Fatalf("Remove(leaf symlink) error = %v", err)
 	}
 	mustWriteSecretFile(t, filepath.Join(dir, "OPENROUTER_API_KEY"), "inside-secret")
-	original := readSecretsDirectoryEntries
-	readSecretsDirectoryEntries = func(path string) ([]os.DirEntry, error) {
-		entries, err := original(path)
-		if err != nil {
-			return nil, err
-		}
-		if err := os.Remove(filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
-			return nil, err
-		}
-		if err := os.Remove(dir); err != nil {
-			return nil, err
-		}
-		if err := os.Symlink(outside, dir); err != nil {
-			return nil, err
-		}
-		return entries, nil
+	deps := commandDependencies{
+		readSecretsDirectory: func(path string) ([]os.DirEntry, error) {
+			entries, err := os.ReadDir(path)
+			if err != nil {
+				return nil, err
+			}
+			if err := os.Remove(filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
+				return nil, err
+			}
+			if err := os.Remove(dir); err != nil {
+				return nil, err
+			}
+			if err := os.Symlink(outside, dir); err != nil {
+				return nil, err
+			}
+			return entries, nil
+		},
 	}
-	t.Cleanup(func() { readSecretsDirectoryEntries = original })
-	_, err = loadSecretsFromConfig(secretConfig(dir), nil)
+	_, err = loadSecretsFromConfigWithDependencies(secretConfig(dir), nil, deps)
 	if err == nil || !strings.Contains(err.Error(), "API-key loader") {
 		t.Fatalf("loadSecretsFromConfig(ancestor replacement) error = %v, want failure", err)
 	}