Validate hidden base artifact ownership conflicts
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user