Separate version interpretation from reporting

internal/engine/load_test.go

@@ -463,6 +463,41 @@ func TestLoadVersionReportSuppressesReturnedErrorTextAndDefaults(t *testing.T) {
 	}
 }
 
+func TestVersionWarningDoesNotSuppressReturnedError(t *testing.T) {
+	t.Parallel()
+
+	type config struct{}
+	versionPath := Keys("schema_version")
+	result, err := testLoad(Bytes("version.yml", []byte("{}\n")), Options[config]{
+		Definition: Definition[config]{Version: &VersionRule{
+			Path: versionPath,
+			Check: func(_ Version, validation *Validation) error {
+				validation.Warning(versionPath, "review schema compatibility")
+				return errors.New("CANARY_VERSION_ERROR")
+			},
+		}},
+	})
+	if result != nil || err == nil {
+		t.Fatalf("result = %#v, err = %v", result, err)
+	}
+	var loadError *LoadError
+	if !errors.As(err, &loadError) {
+		t.Fatalf("error type = %T", err)
+	}
+	diagnostics := loadError.Diagnostics()
+	var warning, hookFailure bool
+	for _, diagnostic := range diagnostics {
+		if strings.Contains(diagnostic.Message, "CANARY") {
+			t.Fatalf("returned checker error text was exposed: %#v", diagnostic)
+		}
+		warning = warning || diagnostic.Code == CodeVersionWarning && diagnostic.Severity == SeverityWarning
+		hookFailure = hookFailure || diagnostic.Code == CodeVersionHookFailed && diagnostic.Severity == SeverityError
+	}
+	if !warning || !hookFailure {
+		t.Fatalf("diagnostics = %#v", diagnostics)
+	}
+}
+
 func TestLoadMetadataReflectsDefaultsInputAndDetachedRecords(t *testing.T) {
 	t.Parallel()
 

internal/engine/validation.go

@@ -187,34 +187,59 @@ func validationValueMutations(before, after *document.Node) []Diagnostic {
 	return diagnostics
 }
 
+type versionCheckResult struct {
+	version     Version
+	diagnostics []Diagnostic
+}
+
 func checkVersion(rule *VersionRule, root *document.Node) []Diagnostic {
+	return checkVersionResult(rule, root).diagnostics
+}
+
+func checkVersionResult(rule *VersionRule, root *document.Node) versionCheckResult {
 	if rule == nil {
-		return nil
+		return versionCheckResult{}
 	}
-	version := Version{}
+	result := interpretVersion(rule, root)
+	if hasErrors(result.diagnostics) {
+		result.diagnostics = canonicalDiagnostics(result.diagnostics)
+		return result
+	}
+	result.diagnostics = append(result.diagnostics, reportVersion(rule, result.version, root)...)
+	result.diagnostics = canonicalDiagnostics(result.diagnostics)
+	return result
+}
+
+func interpretVersion(rule *VersionRule, root *document.Node) versionCheckResult {
+	result := versionCheckResult{}
 	node, present := documentAtPath(root, rule.Path)
 	if present {
-		version.Present = true
+		result.version.Present = true
 		switch node.Kind {
 		case document.Null:
-			version.Null = true
+			result.version.Null = true
 		case document.String:
-			version.Text = node.Text
+			result.version.Text = node.Text
 		case document.Number:
 			if isIntegerVersion(node) {
-				version.Text = node.Text
-				break
+				result.version.Text = node.Text
+				return result
 			}
 			fallthrough
 		default:
 			location, _ := nearestDocumentLocation(root, rule.Path)
 			primary := documentLocationPointer(location)
-			return []Diagnostic{{
+			result.diagnostics = []Diagnostic{{
 				Code: CodeVersionType, Severity: SeverityError, Path: rule.Path.clone(),
 				Message: "schema version must be a string, integer, or null", Primary: primary,
 			}}
+			return result
 		}
 	}
+	return result
+}
+
+func reportVersion(rule *VersionRule, version Version, root *document.Node) []Diagnostic {
 	state := &validationState{
 		active: true, errorCode: CodeVersionError, warningCode: CodeVersionWarning,
 		invalidPath: CodeVersionError, document: root,
@@ -229,7 +254,7 @@ func checkVersion(rule *VersionRule, root *document.Node) []Diagnostic {
 			Message: "schema-version check failed", Primary: documentLocationPointer(location),
 		})
 	}
-	return canonicalDiagnostics(state.diagnostics)
+	return state.diagnostics
 }
 
 func isIntegerVersion(node *document.Node) bool {