fix(values): schema validation and ref internal support

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

Author: brandtkeller

src/pkg/packager/layout/assemble.go

@@ -1042,9 +1042,9 @@ func mergeAndWriteValuesSchema(ctx context.Context, parentSchema string, importe
 		return nil
 	}
 
-	// loadSchema reads a schema file, rejects any "$ref" pointers before handing the
-	// document to gojsonschema, then validates the schema structure. CheckNoRefs runs
-	// first so that gojsonschema never attempts to resolve external URIs.
+	// loadSchema reads a schema file, rejects any external "$ref" pointers before handing
+	// the document to gojsonschema, then validates the schema structure. CheckNoExternalRefs
+	// runs first so that gojsonschema never attempts to resolve external URIs.
 	loadSchema := func(relPath, label string) (map[string]any, error) {
 		src := relPath
 		if !filepath.IsAbs(src) {
@@ -1058,7 +1058,7 @@ func mergeAndWriteValuesSchema(ctx context.Context, parentSchema string, importe
 		if err := json.Unmarshal(b, &s); err != nil {
 			return nil, fmt.Errorf("parsing %s schema: %w", label, err)
 		}
-		if err := value.CheckNoRefs(s); err != nil {
+		if err := value.CheckNoExternalRefs(s); err != nil {
 			return nil, fmt.Errorf("%s schema %s: %w", label, relPath, err)
 		}
 		if err := value.ValidateSchemaDocument(s); err != nil {
@@ -1093,10 +1093,13 @@ func mergeAndWriteValuesSchema(ctx context.Context, parentSchema string, importe
 		if err != nil {
 			return err
 		}
+		if schemaVersion(child) == "" {
+			return fmt.Errorf("imported schema %s: missing \"$schema\" version declaration; all schemas being merged must specify a version", schemaRelPath)
+		}
 		if merged == nil {
 			merged = child
 		} else {
-			if err := checkCompatibleDialect(merged, child, schemaRelPath); err != nil {
+			if err := checkCompatibleVersion(merged, child, schemaRelPath); err != nil {
 				return err
 			}
 			merged = value.MergeSchemas(merged, child)
@@ -1109,12 +1112,19 @@ func mergeAndWriteValuesSchema(ctx context.Context, parentSchema string, importe
 		if err != nil {
 			return err
 		}
-		if err := checkCompatibleDialect(parent, merged, "imported schemas"); err != nil {
+		if schemaVersion(parent) == "" {
+			return fmt.Errorf("parent schema %s: missing \"$schema\" version declaration; all schemas being merged must specify a version", parentSchema)
+		}
+		if err := checkCompatibleVersion(parent, merged, "imported schemas"); err != nil {
 			return err
 		}
 		merged = value.MergeSchemas(parent, merged)
 	}
 
+	if err := value.ValidateSchemaDocument(merged); err != nil {
+		return fmt.Errorf("merged values schema is invalid: %w", err)
+	}
+
 	dst := filepath.Join(buildPath, ValuesSchema)
 	l.Debug("writing merged values schema", "dst", dst)
 	b, err := json.MarshalIndent(merged, "", "  ")
@@ -1127,20 +1137,21 @@ func mergeAndWriteValuesSchema(ctx context.Context, parentSchema string, importe
 	return nil
 }
 
-// schemaDialect extracts the "$schema" dialect URI from a schema map, returning "" if absent or not a string
-func schemaDialect(s map[string]any) string {
+// schemaVersion extracts the "$schema" version URI from a schema map, returning "" if absent or not a string.
+func schemaVersion(s map[string]any) string {
 	if v, ok := s["$schema"].(string); ok {
 		return v
 	}
 	return ""
 }
 
-// checkCompatibleDialect errors when both schemas explicitly declare a "$schema" dialect that differ
-func checkCompatibleDialect(accumulated, incoming map[string]any, incomingLabel string) error {
-	a := schemaDialect(accumulated)
-	b := schemaDialect(incoming)
-	if a != "" && b != "" && a != b {
-		return fmt.Errorf("cannot merge schemas with different dialects: accumulated schema uses %q but %s declares %q", a, incomingLabel, b)
+// checkCompatibleVersion errors when the accumulated merged schema and an incoming schema declare
+// different "$schema" versions, preventing silent cross-version merge bugs.
+func checkCompatibleVersion(accumulated, incoming map[string]any, incomingLabel string) error {
+	a := schemaVersion(accumulated)
+	b := schemaVersion(incoming)
+	if a != b {
+		return fmt.Errorf("cannot merge schemas with different versions: accumulated schema uses %q but %s declares %q", a, incomingLabel, b)
 	}
 	return nil
 }

src/pkg/packager/layout/assemble_test.go

@@ -429,43 +429,61 @@ func TestMergeAndWriteValuesSchema(t *testing.T) {
 		require.Equal(t, string(original), string(written), "verbatim copy should match source file exactly")
 	})
 
-	t.Run("rejects parent schema containing $ref even with no children", func(t *testing.T) {
+	t.Run("rejects parent schema containing external $ref", func(t *testing.T) {
 		t.Parallel()
 		buildPath := t.TempDir()
-		err := mergeAndWriteValuesSchema(ctx, "child-with-ref.schema.json", nil, testdataDir, buildPath)
+		err := mergeAndWriteValuesSchema(ctx, "child-with-external-ref.schema.json", nil, testdataDir, buildPath)
 		require.ErrorContains(t, err, "$ref")
 	})
 
-	t.Run("rejects child schema containing $ref", func(t *testing.T) {
+	t.Run("rejects child schema containing external $ref", func(t *testing.T) {
 		t.Parallel()
 		buildPath := t.TempDir()
-		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-with-ref.schema.json"}, testdataDir, buildPath)
+		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-with-external-ref.schema.json"}, testdataDir, buildPath)
 		require.ErrorContains(t, err, "$ref")
 	})
 
-	t.Run("rejects merge when parent and child declare different dialects", func(t *testing.T) {
+	t.Run("allows internal fragment refs in schemas being merged", func(t *testing.T) {
+		t.Parallel()
+		buildPath := t.TempDir()
+		// child-with-ref.schema.json uses "$ref": "#/definitions/name" — internal, safe to merge
+		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-with-ref.schema.json"}, testdataDir, buildPath)
+		require.NoError(t, err)
+	})
+
+	t.Run("rejects merge when parent and child declare different versions", func(t *testing.T) {
 		t.Parallel()
 		buildPath := t.TempDir()
 		// parent-with-required declares draft-07; child-wrong-version declares 2019-09
 		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-wrong-version.schema.json"}, testdataDir, buildPath)
-		require.ErrorContains(t, err, "different dialects")
+		require.ErrorContains(t, err, "different versions")
 		require.ErrorContains(t, err, "draft-07")
 		require.ErrorContains(t, err, "2019-09")
 	})
 
-	t.Run("allows merge when child omits dialect", func(t *testing.T) {
+	t.Run("preserves child definitions when parent overrides with empty map so internal refs remain valid", func(t *testing.T) {
 		t.Parallel()
 		buildPath := t.TempDir()
-		// child-no-dialect has no $schema; absence is treated as compatible with any version
-		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-no-dialect.schema.json"}, testdataDir, buildPath)
+		// child-with-ref uses $ref: "#/definitions/name" with a matching definition.
+		// parent-overrides-definitions sets definitions: {} (empty), which previously
+		// deleted the child's definition and left the $ref unresolvable.
+		// With definitions merged like properties, the child-only "name" entry survives.
+		err := mergeAndWriteValuesSchema(ctx, "parent-overrides-definitions.schema.json", []string{"child-with-ref.schema.json"}, testdataDir, buildPath)
 		require.NoError(t, err)
 	})
 
-	t.Run("allows merge when both schemas omit dialect", func(t *testing.T) {
+	t.Run("rejects merge when child omits version", func(t *testing.T) {
 		t.Parallel()
 		buildPath := t.TempDir()
-		err := mergeAndWriteValuesSchema(ctx, "", []string{"child-no-dialect.schema.json", "child-no-dialect.schema.json"}, testdataDir, buildPath)
-		require.NoError(t, err)
+		err := mergeAndWriteValuesSchema(ctx, "parent-with-required.schema.json", []string{"child-no-dialect.schema.json"}, testdataDir, buildPath)
+		require.ErrorContains(t, err, "missing \"$schema\" version declaration")
+	})
+
+	t.Run("rejects merge when parent omits version", func(t *testing.T) {
+		t.Parallel()
+		buildPath := t.TempDir()
+		err := mergeAndWriteValuesSchema(ctx, "child-no-dialect.schema.json", []string{"child.schema.json"}, testdataDir, buildPath)
+		require.ErrorContains(t, err, "missing \"$schema\" version declaration")
 	})
 
 	mergeTests := []struct {

src/pkg/packager/layout/testdata/schema-merge/child-with-external-ref.schema.json

@@ -0,0 +1,7 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "type": "object",
+  "properties": {
+    "appName": { "$ref": "./defs/name.json" }
+  }
+}

src/pkg/packager/layout/testdata/schema-merge/parent-overrides-definitions.schema.json

@@ -0,0 +1,5 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "type": "object",
+  "definitions": {}
+}

src/pkg/packager/load/testdata/import/values/schema-deep/expected-merged-schema.json

@@ -7,41 +7,46 @@ import (
 	"fmt"
 	"maps"
 	"slices"
+	"strings"
 )
 
-// CheckNoRefs returns an error if the schema object contains any "$ref" pointers.
-// "$ref" is not supported because referenced files are not bundled into the assembled
-// package and cannot be resolved after assembly. Flatten the schema into a single
-// self-contained file before use.
-func CheckNoRefs(schema map[string]any) error {
-	return checkNoRefsInObject(schema)
+// CheckNoExternalRefs returns an error if the schema contains any external reference
+// pointers ($ref, $dynamicRef, $recursiveRef). Internal fragment references that start
+// with "#" — such as "#/definitions/Foo" or "#/$defs/Foo" — are allowed because the
+// referenced definition is part of the same document and travels with the schema during
+// merge and assembly. External references (relative file paths, HTTP URIs) are rejected
+// because the referenced files are not bundled into the assembled package.
+func CheckNoExternalRefs(schema map[string]any) error {
+	return checkNoExternalRefsInObject(schema)
 }
 
-var blockedRefKeywords = []string{"$ref", "$dynamicRef", "$recursiveRef"}
+var externalRefKeywords = []string{"$ref", "$dynamicRef", "$recursiveRef"}
 
-func checkNoRefsInObject(node map[string]any) error {
-	for _, kw := range blockedRefKeywords {
-		if _, has := node[kw]; has {
-			return fmt.Errorf("schema contains a %q pointer; flatten the schema into a single self-contained file", kw)
+func checkNoExternalRefsInObject(node map[string]any) error {
+	for _, kw := range externalRefKeywords {
+		if val, has := node[kw]; has {
+			if ref, ok := val.(string); ok && !strings.HasPrefix(ref, "#") {
+				return fmt.Errorf("schema contains an external %q pointer %q; only internal references (\"#/...\") are supported — external files are not bundled into the assembled package", kw, ref)
+			}
 		}
 	}
 	for _, key := range slices.Sorted(maps.Keys(node)) {
-		if err := checkNoRefsInValue(key, node[key]); err != nil {
+		if err := checkNoExternalRefsInValue(key, node[key]); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func checkNoRefsInValue(key string, val any) error {
+func checkNoExternalRefsInValue(key string, val any) error {
 	switch v := val.(type) {
 	case map[string]any:
-		if err := checkNoRefsInObject(v); err != nil {
+		if err := checkNoExternalRefsInObject(v); err != nil {
 			return fmt.Errorf("%s: %w", key, err)
 		}
 	case []any:
 		for i, item := range v {
-			if err := checkNoRefsInValue(fmt.Sprintf("%s[%d]", key, i), item); err != nil {
+			if err := checkNoExternalRefsInValue(fmt.Sprintf("%s[%d]", key, i), item); err != nil {
 				return err
 			}
 		}
@@ -77,15 +82,17 @@ func copyMap(m map[string]any) map[string]any {
 
 // MergeSchemas merges child into parent with parent-wins semantics and returns a new map.
 // Neither parent nor child is modified. Rules:
-//   - "properties": recursively merged; parent wins on same key
+//   - "properties", "definitions", "$defs", "patternProperties", "dependentSchemas":
+//     all are maps of string→schema and are recursively merged; parent wins on same key,
+//     child-only entries are preserved so internal $ref pointers remain valid
 //   - "required": union of both arrays, deduplicated
 //   - all other keys: parent wins (child value used only when key absent from parent)
 func MergeSchemas(parent, child map[string]any) map[string]any {
 	result := copyMap(parent)
 	for key, childVal := range child {
 		switch key {
-		case "properties":
-			result["properties"] = mergeProperties(result["properties"], childVal)
+		case "properties", "definitions", "$defs", "patternProperties", "dependentSchemas":
+			result[key] = mergeProperties(result[key], childVal)
 		case "required":
 			if req := mergeRequired(result["required"], childVal); len(req) > 0 {
 				result["required"] = req