From c3307ab5fb5a697685f9b07796d9ef5c66c016e6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 5 Sep 2026 17:48:22 +0000 Subject: [PATCH] Replace configuration and secret loading globals --- docs/roadmap/implementation.md | 2 +- internal/app/command_dependencies.go | 44 ++++++++++++++++++++++ internal/app/config_loader.go | 30 +++++++++++---- internal/app/config_loader_test.go | 38 +++++++++---------- internal/app/object_store.go | 9 ++++- internal/app/remote_session_test.go | 56 ++++++++++++++++++---------- internal/app/secrets_env.go | 8 +++- internal/app/secrets_env_test.go | 36 +++++++++--------- 8 files changed, 154 insertions(+), 69 deletions(-) create mode 100644 internal/app/command_dependencies.go diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 60fb490..e1030b2 100644 --- 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 diff --git a/internal/app/command_dependencies.go b/internal/app/command_dependencies.go new file mode 100644 index 0000000..22443f7 --- /dev/null +++ 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 +} diff --git a/internal/app/config_loader.go b/internal/app/config_loader.go index 3a554ca..affeb6e 100644 --- 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 } diff --git a/internal/app/config_loader_test.go b/internal/app/config_loader_test.go index eb6a4ab..00f0e54 100644 --- 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) } diff --git a/internal/app/object_store.go b/internal/app/object_store.go index f44e723..21867f7 100644 --- 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( diff --git a/internal/app/remote_session_test.go b/internal/app/remote_session_test.go index 19148d3..f7c5340 100644 --- 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) diff --git a/internal/app/secrets_env.go b/internal/app/secrets_env.go index 84fd240..1c21825 100644 --- 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) } diff --git a/internal/app/secrets_env_test.go b/internal/app/secrets_env_test.go index 70aa280..f526d12 100644 --- 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) }