From abd405015b6fd826c605b94088d53e5e7f2d3435 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 5 Sep 2026 00:29:05 +0000 Subject: [PATCH] Make Scriptorium validation deterministic --- docs/roadmap/implementation.md | 2 +- internal/config/scriptorium_test.go | 22 ++++ internal/config/validate_scriptorium.go | 139 ++++++++++++++++++------ 3 files changed, 130 insertions(+), 33 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 937b6ae..a4d0499 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -310,7 +310,7 @@ configuration only after every check succeeds. ## Stage 4 — Decompose Deterministic Scriptorium Validation -**Status:** Pending +**Status:** Completed ### Goal diff --git a/internal/config/scriptorium_test.go b/internal/config/scriptorium_test.go index 6cd4e3a..60e1a92 100644 --- a/internal/config/scriptorium_test.go +++ b/internal/config/scriptorium_test.go @@ -574,6 +574,28 @@ func TestScriptoriumLoadAndValidate(t *testing.T) { } } +func TestScriptoriumDiagnosticsAreIndependentOfMapInsertionOrder(t *testing.T) { + invalidArtifact := ScriptoriumArtifactConfig{Enabled: true} + firstArtifacts := make(map[string]ScriptoriumArtifactConfig) + firstArtifacts["beta"] = invalidArtifact + firstArtifacts["alpha"] = invalidArtifact + secondArtifacts := make(map[string]ScriptoriumArtifactConfig) + secondArtifacts["alpha"] = invalidArtifact + secondArtifacts["beta"] = invalidArtifact + + firstErr := validateScriptorium(&ScriptoriumConfig{Binary: "scriptorium", Artifacts: firstArtifacts}, nil) + secondErr := validateScriptorium(&ScriptoriumConfig{Binary: "scriptorium", Artifacts: secondArtifacts}, nil) + if firstErr == nil || secondErr == nil { + t.Fatalf("validateScriptorium() errors = %v / %v, want validation failures", firstErr, secondErr) + } + if firstErr.Error() != secondErr.Error() { + t.Fatalf("equivalent maps produced different diagnostics:\nfirst: %v\nsecond: %v", firstErr, secondErr) + } + if !strings.Contains(firstErr.Error(), "pipeline.scriptorium.artifacts.") || !strings.Contains(firstErr.Error(), "prompt_id") { + t.Fatalf("validation error = %v, want artifact field and semantic category", firstErr) + } +} + func TestScriptoriumLegacyTranscriptSourcesRejected(t *testing.T) { legacyTranscriptSources := []string{ "narratio.transcript." + "merged", diff --git a/internal/config/validate_scriptorium.go b/internal/config/validate_scriptorium.go index 9d3b5c3..6d4513f 100644 --- a/internal/config/validate_scriptorium.go +++ b/internal/config/validate_scriptorium.go @@ -23,17 +23,56 @@ func validateScriptorium(cfg *ScriptoriumConfig, notarius *NotariusConfig) error return err } - configuredArtifacts := make(map[string]struct{}, len(cfg.Artifacts)) - referencedArtifacts := make(map[string]struct{}) - outputOwners := make(map[string]string, len(cfg.Artifacts)) - for artifactName := range cfg.Artifacts { - if !artifactpolicy.IsConfiguredKey(artifactName) { - return fmt.Errorf("pipeline.scriptorium.artifacts keys must match ^[a-z][a-z0-9_]*$") - } - configuredArtifacts[artifactName] = struct{}{} + artifactNames, configuredArtifacts, err := validateScriptoriumArtifactKeys(cfg.Artifacts) + if err != nil { + return err + } + if err := validateScriptoriumDeclarations(cfg.Artifacts, artifactNames); err != nil { + return err + } + referencedArtifacts, err := validateScriptoriumDependenciesAndInputs( + cfg.Artifacts, + artifactNames, + configuredArtifacts, + notariusOutputKeySet(notarius), + ) + if err != nil { + return err + } + if err := validateScriptoriumVariables(cfg.Artifacts, artifactNames); err != nil { + return err + } + if err := validateScriptoriumReferencedOutputs(cfg.Artifacts, referencedArtifacts); err != nil { + return err + } + if err := ValidateScriptoriumArtifactDependencies(cfg.Artifacts); err != nil { + return err } - for artifactName, artifactCfg := range cfg.Artifacts { + return nil +} + +func validateScriptoriumArtifactKeys(artifacts map[string]ScriptoriumArtifactConfig) ([]string, map[string]struct{}, error) { + names := make([]string, 0, len(artifacts)) + for name := range artifacts { + names = append(names, name) + } + sort.Strings(names) + + configured := make(map[string]struct{}, len(names)) + for _, name := range names { + if !artifactpolicy.IsConfiguredKey(name) { + return nil, nil, fmt.Errorf("pipeline.scriptorium.artifacts keys must match ^[a-z][a-z0-9_]*$") + } + configured[name] = struct{}{} + } + return names, configured, nil +} + +func validateScriptoriumDeclarations(artifacts map[string]ScriptoriumArtifactConfig, artifactNames []string) error { + outputOwners := make(map[string]string, len(artifactNames)) + for _, artifactName := range artifactNames { + artifactCfg := artifacts[artifactName] if artifactCfg.Enabled && strings.TrimSpace(artifactCfg.PromptID) == "" { return fmt.Errorf("pipeline.scriptorium.artifacts.%s.prompt_id is required when enabled", artifactName) } @@ -49,48 +88,66 @@ func validateScriptorium(cfg *ScriptoriumConfig, notarius *NotariusConfig) error return err } if previous, exists := outputOwners[artifactCfg.OutputPath]; exists { - return fmt.Errorf("%s.output_path %q duplicates artifact %q", pathField, artifactCfg.OutputPath, previous) + return fmt.Errorf("%s %q duplicates artifact %q", pathField, artifactCfg.OutputPath, previous) } outputOwners[artifactCfg.OutputPath] = artifactName } if err := validateDuration("pipeline.scriptorium.artifacts."+artifactName+".timeout", artifactCfg.Timeout); err != nil { return err } + } + return nil +} +func validateScriptoriumDependenciesAndInputs( + artifacts map[string]ScriptoriumArtifactConfig, + artifactNames []string, + configuredArtifacts map[string]struct{}, + extractionOutputs map[string]struct{}, +) (map[string]struct{}, error) { + referencedArtifacts := make(map[string]struct{}) + for _, artifactName := range artifactNames { + artifactCfg := artifacts[artifactName] depSet := make(map[string]struct{}, len(artifactCfg.DependsOn)) for i, depName := range artifactCfg.DependsOn { trimmedDep := strings.TrimSpace(depName) field := fmt.Sprintf("pipeline.scriptorium.artifacts.%s.depends_on[%d]", artifactName, i) if trimmedDep == "" { - return fmt.Errorf("%s must be non-empty", field) + return nil, fmt.Errorf("%s must be non-empty", field) } if _, ok := configuredArtifacts[trimmedDep]; !ok { - return fmt.Errorf("%s %q is not a configured artifact key", field, depName) + return nil, fmt.Errorf("%s %q is not a configured artifact key", field, depName) } if trimmedDep == artifactName { - return fmt.Errorf("pipeline.scriptorium.artifacts.%s.depends_on must not include itself", artifactName) + return nil, fmt.Errorf("pipeline.scriptorium.artifacts.%s.depends_on must not include itself", artifactName) } depSet[trimmedDep] = struct{}{} referencedArtifacts[trimmedDep] = struct{}{} } - for inputName, inputCfg := range artifactCfg.Inputs { + inputNames := make([]string, 0, len(artifactCfg.Inputs)) + for inputName := range artifactCfg.Inputs { + inputNames = append(inputNames, inputName) + } + sort.Strings(inputNames) + for _, inputName := range inputNames { + inputCfg := artifactCfg.Inputs[inputName] trimmedInputName := strings.TrimSpace(inputName) if trimmedInputName == "" { - return fmt.Errorf("pipeline.scriptorium.artifacts.%s.inputs keys must be non-empty", artifactName) + return nil, fmt.Errorf("pipeline.scriptorium.artifacts.%s.inputs keys must be non-empty", artifactName) } source := strings.TrimSpace(inputCfg.Source) if source == "" { - return fmt.Errorf("pipeline.scriptorium.artifacts.%s.inputs.%s.source is required", artifactName, inputName) + return nil, fmt.Errorf("pipeline.scriptorium.artifacts.%s.inputs.%s.source is required", artifactName, inputName) } - referencedArtifact, err := validateScriptoriumInputSource(artifactName, inputName, source, configuredArtifacts, notariusOutputKeySet(notarius)) + referencedArtifact, err := validateScriptoriumInputSource(artifactName, inputName, source, configuredArtifacts, extractionOutputs) if err != nil { - return err + return nil, err } if referencedArtifact != "" { if _, ok := depSet[referencedArtifact]; !ok { - return fmt.Errorf( + return nil, fmt.Errorf( "pipeline.scriptorium.artifacts.%s.inputs.%s.source %q requires depends_on entry %q", artifactName, inputName, @@ -101,7 +158,20 @@ func validateScriptorium(cfg *ScriptoriumConfig, notarius *NotariusConfig) error referencedArtifacts[referencedArtifact] = struct{}{} } } - for varName, varValue := range artifactCfg.Vars { + } + return referencedArtifacts, nil +} + +func validateScriptoriumVariables(artifacts map[string]ScriptoriumArtifactConfig, artifactNames []string) error { + for _, artifactName := range artifactNames { + artifactCfg := artifacts[artifactName] + variableNames := make([]string, 0, len(artifactCfg.Vars)) + for varName := range artifactCfg.Vars { + variableNames = append(variableNames, varName) + } + sort.Strings(variableNames) + for _, varName := range variableNames { + varValue := artifactCfg.Vars[varName] if strings.TrimSpace(varName) == "" { return fmt.Errorf("pipeline.scriptorium.artifacts.%s.vars keys must be non-empty", artifactName) } @@ -112,18 +182,21 @@ func validateScriptorium(cfg *ScriptoriumConfig, notarius *NotariusConfig) error } } } + return nil +} +func validateScriptoriumReferencedOutputs(artifacts map[string]ScriptoriumArtifactConfig, referencedArtifacts map[string]struct{}) error { + artifactNames := make([]string, 0, len(referencedArtifacts)) for artifactName := range referencedArtifacts { - artifactCfg := cfg.Artifacts[artifactName] + artifactNames = append(artifactNames, artifactName) + } + sort.Strings(artifactNames) + for _, artifactName := range artifactNames { + artifactCfg := artifacts[artifactName] if strings.TrimSpace(artifactCfg.OutputPath) == "" { return fmt.Errorf("pipeline.scriptorium.artifacts.%s.output_path is required when artifact is referenced", artifactName) } } - - if err := ValidateScriptoriumArtifactDependencies(cfg.Artifacts); err != nil { - return err - } - return nil } @@ -192,8 +265,15 @@ func ValidateScriptoriumArtifactDependencies(artifacts map[string]ScriptoriumArt return nil } + nodes := make([]string, 0, len(artifacts)) + for name := range artifacts { + nodes = append(nodes, name) + } + sort.Strings(nodes) + graph := make(map[string][]string, len(artifacts)) - for name, cfg := range artifacts { + for _, name := range nodes { + cfg := artifacts[name] if !artifactpolicy.IsConfiguredKey(name) { return fmt.Errorf("pipeline.scriptorium.artifacts keys must match ^[a-z][a-z0-9_]*$") } @@ -240,11 +320,6 @@ func ValidateScriptoriumArtifactDependencies(artifacts map[string]ScriptoriumArt return nil } - nodes := make([]string, 0, len(artifacts)) - for node := range artifacts { - nodes = append(nodes, node) - } - sort.Strings(nodes) for _, node := range nodes { if err := visit(node); err != nil { return err