diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index e361c75..5c66ffa 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -87,7 +87,13 @@ directory resolves from the pipeline root. Preserve digest relocation invariance ## 3. Enforce unique base definitions even when a profile removes them -Status: Open — P2 +Status: Resolved — P2 + +Resolution: Named-profile loading now validates the independently resolved base +through the same ConfigKit definition and public metadata history before accepting +the selected result. Duplicate ordinary or family definitions therefore fail even +when an overlay nulls their container or all of Scriptorium. A single base owner +with a legitimate profile override remains supported. **Evidence:** `rejectDuplicateBaseDefinitions` at `internal/config/pipeline_configkit.go:354` enumerates only artifact/family keys diff --git a/internal/config/pipeline_configkit.go b/internal/config/pipeline_configkit.go index 505eba3..023e817 100644 --- a/internal/config/pipeline_configkit.go +++ b/internal/config/pipeline_configkit.go @@ -42,18 +42,20 @@ type pipelineFieldOwnership struct { func loadPipelineWithConfigKit(path string, opts PipelineLoadOptions) (*PipelineConfig, error) { profile := selectedPipelineProfileName(opts) - result, err := configkit.Load(configkit.File(path), configkit.Options[PipelineConfig]{ - Imports: true, - Profile: profile, - Definition: configkit.Definition[PipelineConfig]{ - Defaults: pipelineConfigDefaults, - Resolve: resolvePipelineConfig, - Validate: validatePipelineConfigKit, - }, - }) + result, err := loadPipelineConfigKitResult(path, profile) if err != nil { return nil, describePipelineLoadError(err) } + if profile != "" { + base, err := loadPipelineConfigKitResult(path, "") + if err != nil { + return nil, describePipelineLoadError(err) + } + baseConfig := base.Config + if err := rejectDuplicateBaseDefinitions(&baseConfig, base.Metadata(), base.Sources()); err != nil { + return nil, fmt.Errorf("load pipeline config: %w", err) + } + } cfg := result.Config metadata, err := pipelineMetadataFromConfigKit(&cfg, result, opts) if err != nil { @@ -63,6 +65,18 @@ func loadPipelineWithConfigKit(path string, opts PipelineLoadOptions) (*Pipeline return &cfg, nil } +func loadPipelineConfigKitResult(path, profile string) (*configkit.Result[PipelineConfig], error) { + return configkit.Load(configkit.File(path), configkit.Options[PipelineConfig]{ + Imports: true, + Profile: profile, + Definition: configkit.Definition[PipelineConfig]{ + Defaults: pipelineConfigDefaults, + Resolve: resolvePipelineConfig, + Validate: validatePipelineConfigKit, + }, + }) +} + func describePipelineLoadError(err error) error { return describeConfigKitLoadError("pipeline", err) } diff --git a/internal/config/pipeline_configkit_test.go b/internal/config/pipeline_configkit_test.go index fb61169..3a6f7f2 100644 --- a/internal/config/pipeline_configkit_test.go +++ b/internal/config/pipeline_configkit_test.go @@ -52,10 +52,10 @@ whisperx: func TestPipelineConfigKitRejectsDuplicateBaseDeclarations(t *testing.T) { for _, test := range []struct { - name, first, second string + name, first, second, clearedContainer string }{ - {name: "artifact", first: "scriptorium:\n artifacts:\n recap:\n prompt_id: recap\n", second: "scriptorium:\n artifacts:\n recap:\n output_path: artifacts/recap.md\n"}, - {name: "artifact family", first: "scriptorium:\n artifact_families:\n recap:\n for_each: party.characters\n", second: "scriptorium:\n artifact_families:\n recap:\n output_path_pattern: artifacts/{character_id}.md\n"}, + {name: "artifact", first: "scriptorium:\n artifacts:\n recap:\n prompt_id: recap\n", second: "scriptorium:\n artifacts:\n recap:\n output_path: artifacts/recap.md\n", clearedContainer: "scriptorium:\n artifacts: null\n"}, + {name: "artifact family", first: "scriptorium:\n artifact_families:\n recap:\n for_each: party.characters\n", second: "scriptorium:\n artifact_families:\n recap:\n output_path_pattern: artifacts/{character_id}.md\n", clearedContainer: "scriptorium:\n artifact_families: null\n"}, } { t.Run(test.name, func(t *testing.T) { dir := t.TempDir() @@ -68,16 +68,46 @@ whisperx: `) writePipelineFixture(t, dir, "first.yml", test.first) writePipelineFixture(t, dir, "second.yml", test.second) + writePipelineFixture(t, dir, "profiles/clear-container.yml", test.clearedContainer) + writePipelineFixture(t, dir, "profiles/clear-scriptorium.yml", "scriptorium: null\n") - _, err := LoadPipeline(filepath.Join(dir, "pipeline.yml")) - if err == nil || !strings.Contains(err.Error(), "declared by multiple base files") || - !strings.Contains(err.Error(), "first.yml") || !strings.Contains(err.Error(), "second.yml") { - t.Fatalf("duplicate declaration error = %v", err) + for _, profile := range []string{"", "clear-container", "clear-scriptorium"} { + t.Run(profile, func(t *testing.T) { + options := PipelineLoadOptions{} + if profile != "" { + options.Profile = &profile + } + _, err := LoadPipelineWithOptions(filepath.Join(dir, "pipeline.yml"), options) + if err == nil || !strings.Contains(err.Error(), "declared by multiple base files") || + !strings.Contains(err.Error(), "first.yml") || !strings.Contains(err.Error(), "second.yml") { + t.Fatalf("duplicate declaration error = %v", err) + } + }) } }) } } +func TestPipelineConfigKitAllowsOneBaseDeclarationWithProfileOverride(t *testing.T) { + dir := t.TempDir() + writePipelineFixture(t, dir, "pipeline.yml", `configkit: + imports: + - path: artifact.yml +whisperx: + transcribe_url: https://transcription.example.test/transcribe +`) + writePipelineFixture(t, dir, "artifact.yml", "scriptorium:\n artifacts:\n recap:\n prompt_id: base\n output_path: artifacts/recap.md\n") + writePipelineFixture(t, dir, "profiles/testing.yml", "scriptorium:\n artifacts:\n recap:\n prompt_id: testing\n") + profile := "testing" + cfg, err := LoadPipelineWithOptions(filepath.Join(dir, "pipeline.yml"), PipelineLoadOptions{Profile: &profile}) + if err != nil { + t.Fatal(err) + } + if got := cfg.Scriptorium.Artifacts["recap"].PromptID; got != "testing" { + t.Fatalf("profile prompt = %q, want testing", got) + } +} + func TestPipelineConfigKitRejectsUnknownDirectiveAlias(t *testing.T) { dir := t.TempDir() path := writePipelineFixture(t, dir, "pipeline.yml", "composition:\n imports: []\n")