Make Scriptorium validation deterministic

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
 

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",

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))
+	artifactNames, configuredArtifacts, err := validateScriptoriumArtifactKeys(cfg.Artifacts)
-	referencedArtifacts := make(map[string]struct{})
+	if err != nil {
-	outputOwners := make(map[string]string, len(cfg.Artifacts))
+		return err
-	for artifactName := range cfg.Artifacts {
+	}
-		if !artifactpolicy.IsConfiguredKey(artifactName) {
+	if err := validateScriptoriumDeclarations(cfg.Artifacts, artifactNames); err != nil {
-			return fmt.Errorf("pipeline.scriptorium.artifacts keys must match ^[a-z][a-z0-9_]*$")
+		return err
-		}
+	}
-		configuredArtifacts[artifactName] = struct{}{}
+	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