From df213bc2ba8e6ec2cd6bd3721b3296506e165381 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 16 Sep 2026 04:26:24 +0000 Subject: [PATCH] Isolate pipeline configuration discovery --- docs/roadmap/implementation.md | 14 ++++- .../app/analyze_artifacts_commands_test.go | 18 +++++-- internal/app/command_dependencies.go | 4 ++ internal/app/commands_test.go | 22 ++++++-- internal/app/config_loader.go | 2 +- internal/app/operator_helpers_test.go | 14 ++--- internal/app/pipeline_config_path_test.go | 54 ++++++++++++++++++- internal/app/test_environment_test.go | 28 ++++++++++ 8 files changed, 137 insertions(+), 19 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 431dbaa..ca332c8 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -36,7 +36,19 @@ migration plan; that history remains in Git. ## Stage 1 — Isolate command configuration discovery -Status: Not started +Status: Complete — 2026-09-16 + +Completion: Added an invocation-local pipeline discovery dependency with the +production resolver as its default, then moved the affected missing-config tests +onto real resolver calls over temporary candidate paths. The command-boundary +coverage verifies successful default selection reaches the configured loader and +that discovery failures stop before bootstrap, typed loading, or execution. + +Checks: + +- `go test ./internal/app -run 'TestResolvePipelineConfigPath|TestExecute.*Missing|TestExecuteSessionInitMissingDefaultConfig'` — pass. +- `GOWORK=off go test -race ./internal/app` — pass. +- `GOWORK=off go test ./...` — pass with ConfigKit v0.1.2. Purpose: Make missing-config command tests deterministic before changing the dependency, while exercising the real discovery logic. diff --git a/internal/app/analyze_artifacts_commands_test.go b/internal/app/analyze_artifacts_commands_test.go index b326438..06f504e 100644 --- a/internal/app/analyze_artifacts_commands_test.go +++ b/internal/app/analyze_artifacts_commands_test.go @@ -272,13 +272,18 @@ func TestExecuteAnalyzeRejectsPositionalArgsAndForceFlag(t *testing.T) { } func TestExecuteAnalyzeMissingConfigUsesRunStageLoadingPath(t *testing.T) { + deps, missingPath, executor := guardedMissingPipelineDiscovery(t) var stdout bytes.Buffer var stderr bytes.Buffer - code := Execute([]string{"analyze", "2026-05-03"}, &stdout, &stderr) + code := executeWithDependencies([]string{"analyze", "2026-05-03"}, &stdout, &stderr, deps, executor) if code == 0 { t.Fatal("exit code = 0, want non-zero") } - if !strings.Contains(stderr.String(), "analyze: no pipeline config path provided and no default pipeline config found; searched:") { + if stdout.Len() != 0 { + t.Fatalf("stdout = %q, want empty", stdout.String()) + } + if got := stderr.String(); !strings.Contains(got, "analyze: no pipeline config path provided") || + !strings.Contains(got, "no default pipeline config found") || !strings.Contains(got, missingPath) { t.Fatalf("stderr = %q, want pipeline discovery error", stderr.String()) } } @@ -371,13 +376,18 @@ func TestExecutePublishUnknownArtifactFailsValidation(t *testing.T) { } func TestExecutePublishMissingConfigUsesRunStageLoadingPath(t *testing.T) { + deps, missingPath, executor := guardedMissingPipelineDiscovery(t) var stdout bytes.Buffer var stderr bytes.Buffer - code := Execute([]string{"publish", "2026-05-03"}, &stdout, &stderr) + code := executeWithDependencies([]string{"publish", "2026-05-03"}, &stdout, &stderr, deps, executor) if code == 0 { t.Fatal("exit code = 0, want non-zero") } - if !strings.Contains(stderr.String(), "publish: no pipeline config path provided and no default pipeline config found; searched:") { + if stdout.Len() != 0 { + t.Fatalf("stdout = %q, want empty", stdout.String()) + } + if got := stderr.String(); !strings.Contains(got, "publish: no pipeline config path provided") || + !strings.Contains(got, "no default pipeline config found") || !strings.Contains(got, missingPath) { t.Fatalf("stderr = %q, want pipeline discovery error", stderr.String()) } } diff --git a/internal/app/command_dependencies.go b/internal/app/command_dependencies.go index 48b0ee6..130f0c4 100644 --- a/internal/app/command_dependencies.go +++ b/internal/app/command_dependencies.go @@ -11,6 +11,7 @@ import ( type commandDependencies struct { diagnostics io.Writer + resolvePipelineConfigPath func(string) (string, error) loadPipelineConfig func(string, config.PipelineLoadOptions) (*config.PipelineConfig, error) downloadObjectToTemp func(context.Context, storage.ObjectStore, string, string) (string, error) loadSecretsDir func(string) (*configkit.SecretsReport, error) @@ -25,6 +26,9 @@ func normalizeCommandDependencies(deps commandDependencies) commandDependencies if deps.diagnostics == nil { deps.diagnostics = io.Discard } + if deps.resolvePipelineConfigPath == nil { + deps.resolvePipelineConfigPath = resolvePipelineConfigPath + } if deps.loadPipelineConfig == nil { deps.loadPipelineConfig = config.LoadPipelineWithOptions } diff --git a/internal/app/commands_test.go b/internal/app/commands_test.go index c49ff2f..f723f89 100644 --- a/internal/app/commands_test.go +++ b/internal/app/commands_test.go @@ -59,9 +59,10 @@ func TestExecuteValidCommands(t *testing.T) { func TestExecuteMissingRequiredFlags(t *testing.T) { cases := []struct { - name string - args []string - want string + name string + args []string + want string + controlledDiscovery bool }{ {name: "run missing session", args: []string{"run"}, want: "run: session_id is required"}, {name: "plan old top-level removed", args: []string{"plan"}, want: `unknown command: "plan"`}, @@ -69,7 +70,7 @@ func TestExecuteMissingRequiredFlags(t *testing.T) { {name: "resume removed", args: []string{"resume"}, want: `unknown command: "resume"`}, {name: "run-stage missing name", args: []string{"run-stage", "--config", "a", "--session", "b"}, want: "run-stage: expected stage name and session_id"}, {name: "run-stage missing session", args: []string{"run-stage", "polish"}, want: "run-stage: expected stage name and session_id"}, - {name: "run missing config uses defaults", args: []string{"run", "2026-05-03", "--session", "session.yml"}, want: "run: no pipeline config path provided and no default pipeline config found; searched:"}, + {name: "run missing config uses defaults", args: []string{"run", "2026-05-03", "--session", "session.yml"}, want: "run: no pipeline config path provided", controlledDiscovery: true}, } for _, tc := range cases { @@ -77,7 +78,13 @@ func TestExecuteMissingRequiredFlags(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer - code := Execute(tc.args, &stdout, &stderr) + deps := commandDependencies{} + executor := pipelineExecutor(executePlan) + missingPath := "" + if tc.controlledDiscovery { + deps, missingPath, executor = guardedMissingPipelineDiscovery(t) + } + code := executeWithDependencies(tc.args, &stdout, &stderr, deps, executor) if code == 0 { t.Fatalf("exit code = 0, want non-zero") } @@ -87,6 +94,11 @@ func TestExecuteMissingRequiredFlags(t *testing.T) { if !strings.Contains(stderr.String(), tc.want) { t.Fatalf("stderr = %q, want to contain %q", stderr.String(), tc.want) } + if tc.controlledDiscovery { + if got := stderr.String(); !strings.Contains(got, "no default pipeline config found") || !strings.Contains(got, missingPath) { + t.Fatalf("stderr = %q, want controlled searched path %q", got, missingPath) + } + } }) } } diff --git a/internal/app/config_loader.go b/internal/app/config_loader.go index ded80bc..ba4a9ef 100644 --- a/internal/app/config_loader.go +++ b/internal/app/config_loader.go @@ -163,7 +163,7 @@ func loadPipelineCampaignConfigWithDependencies(pipelineFlag, campaignFlag, camp func loadPipelineConfigWithDependencies(pipelineFlag string, opts config.PipelineLoadOptions, secretsDir *string, deps commandDependencies) (string, *config.PipelineConfig, error) { deps = normalizeCommandDependencies(deps) - resolvedPipelinePath, err := resolvePipelineConfigPath(pipelineFlag) + resolvedPipelinePath, err := deps.resolvePipelineConfigPath(pipelineFlag) if err != nil { return "", nil, err } diff --git a/internal/app/operator_helpers_test.go b/internal/app/operator_helpers_test.go index ac3fc9d..b9f7f9f 100644 --- a/internal/app/operator_helpers_test.go +++ b/internal/app/operator_helpers_test.go @@ -164,19 +164,19 @@ func TestExecuteSessionInitRequiresSessionID(t *testing.T) { } func TestExecuteSessionInitMissingDefaultConfigReportsSearchedPaths(t *testing.T) { - origPipelineDefaults := append([]string(nil), config.DefaultPipelineConfigSearchPaths...) - config.DefaultPipelineConfigSearchPaths = []string{filepath.Join(t.TempDir(), "missing-pipeline.yml")} - t.Cleanup(func() { - config.DefaultPipelineConfigSearchPaths = origPipelineDefaults - }) + deps, missingPath, executor := guardedMissingPipelineDiscovery(t) var stdout bytes.Buffer var stderr bytes.Buffer - code := Execute([]string{"session", "init", "2026-06-07", "--remote"}, &stdout, &stderr) + code := executeWithDependencies([]string{"session", "init", "2026-06-07", "--remote"}, &stdout, &stderr, deps, executor) if code == 0 { t.Fatal("exit code = 0, want non-zero") } - if !strings.Contains(stderr.String(), "session init: no pipeline config path provided and no default pipeline config found; searched:") { + if stdout.Len() != 0 { + t.Fatalf("stdout = %q, want empty", stdout.String()) + } + if got := stderr.String(); !strings.Contains(got, "session init: no pipeline config path provided") || + !strings.Contains(got, "no default pipeline config found") || !strings.Contains(got, missingPath) { t.Fatalf("stderr = %q, want default pipeline searched-path error", stderr.String()) } } diff --git a/internal/app/pipeline_config_path_test.go b/internal/app/pipeline_config_path_test.go index b8c4918..c8661d5 100644 --- a/internal/app/pipeline_config_path_test.go +++ b/internal/app/pipeline_config_path_test.go @@ -1,10 +1,16 @@ package app import ( + "bytes" + "context" + "errors" "os" "path/filepath" "strings" "testing" + + configkit "gitea.maximumdirect.net/eric/ConfigKit" + "gitea.maximumdirect.net/eric/narratio/internal/config" ) func TestResolvePipelineConfigPathWithCandidatesExplicitWins(t *testing.T) { @@ -55,11 +61,57 @@ func TestResolvePipelineConfigPathWithCandidatesPrecedence(t *testing.T) { } func TestResolvePipelineConfigPathWithCandidatesMissing(t *testing.T) { - _, err := resolvePipelineConfigPathWithCandidates("", []string{"/does/not/exist/one.yml", "/does/not/exist/two.yml"}) + directory := t.TempDir() + first := filepath.Join(directory, "one.yml") + second := filepath.Join(directory, "two.yml") + _, err := resolvePipelineConfigPathWithCandidates("", []string{first, second}) if err == nil { t.Fatal("expected error, got nil") } if !strings.Contains(err.Error(), "no default pipeline config found") { t.Fatalf("error = %q, want missing-defaults context", err.Error()) } + if !strings.Contains(err.Error(), first) || !strings.Contains(err.Error(), second) { + t.Fatalf("error = %q, want controlled searched paths", err.Error()) + } +} + +func TestExecuteMissingExplicitConfigDiscoversExistingDefaultBeforeLoading(t *testing.T) { + directory := t.TempDir() + missing := filepath.Join(directory, "missing.yml") + existing := filepath.Join(directory, "pipeline.yml") + if err := os.WriteFile(existing, []byte("workspace:\n root: ./tmp\n"), 0o644); err != nil { + t.Fatalf("write default pipeline: %v", err) + } + + controlledErr := errors.New("controlled pipeline loader boundary") + loadedPath := "" + deps := commandDependenciesForPipelineCandidates(missing, existing) + deps.loadPipelineConfig = func(path string, _ config.PipelineLoadOptions) (*config.PipelineConfig, error) { + loadedPath = path + return nil, controlledErr + } + deps.loadSecretsDir = func(string) (*configkit.SecretsReport, error) { + t.Fatal("absent conventional secrets directory unexpectedly invoked bootstrap loader") + return nil, nil + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := executeWithDependencies([]string{"config", "validate"}, &stdout, &stderr, deps, func(context.Context, *config.Config, BoundedPlan, RunOptions) (*RunSummary, error) { + t.Fatal("config validation unexpectedly invoked pipeline executor") + return nil, nil + }) + if code == 0 { + t.Fatal("exit code = 0, want controlled loader failure") + } + if loadedPath != existing { + t.Fatalf("loader path = %q, want discovered default %q", loadedPath, existing) + } + if stdout.Len() != 0 { + t.Fatalf("stdout = %q, want empty", stdout.String()) + } + if !strings.Contains(stderr.String(), controlledErr.Error()) { + t.Fatalf("stderr = %q, want controlled loader error", stderr.String()) + } } diff --git a/internal/app/test_environment_test.go b/internal/app/test_environment_test.go index 01c6ee3..fe2c9fe 100644 --- a/internal/app/test_environment_test.go +++ b/internal/app/test_environment_test.go @@ -1,11 +1,13 @@ package app import ( + "context" "os" "path/filepath" "testing" configkit "gitea.maximumdirect.net/eric/ConfigKit" + "gitea.maximumdirect.net/eric/narratio/internal/config" ) // restoreEnvAfterTest clears names for a test and restores both their prior @@ -47,3 +49,29 @@ func mustWriteSecretFile(t *testing.T, path, value string) { t.Fatal(err) } } + +func commandDependenciesForPipelineCandidates(candidates ...string) commandDependencies { + ownedCandidates := append([]string(nil), candidates...) + return commandDependencies{resolvePipelineConfigPath: func(flagValue string) (string, error) { + return resolvePipelineConfigPathWithCandidates(flagValue, ownedCandidates) + }} +} + +func guardedMissingPipelineDiscovery(t *testing.T) (commandDependencies, string, pipelineExecutor) { + t.Helper() + candidate := filepath.Join(t.TempDir(), "missing-pipeline.yml") + deps := commandDependenciesForPipelineCandidates(candidate) + deps.loadSecretsDir = func(string) (*configkit.SecretsReport, error) { + t.Fatal("secret bootstrap continued after pipeline discovery failed") + return nil, nil + } + deps.loadPipelineConfig = func(string, config.PipelineLoadOptions) (*config.PipelineConfig, error) { + t.Fatal("typed pipeline loading continued after pipeline discovery failed") + return nil, nil + } + executor := func(context.Context, *config.Config, BoundedPlan, RunOptions) (*RunSummary, error) { + t.Fatal("pipeline execution continued after pipeline discovery failed") + return nil, nil + } + return deps, candidate, executor +}