Move all Kitfile validation into one place and fix path handling

amisevsk · amisevsk · commit 0920c0e56a14 · 2026-03-10T12:49:59.000-04:00
* Merge split Kitfile validation (previously in
  pkg/lib/kitfile/validate.go and pkg/artifact/kitfile.go) into the
  artifact package
* Fix handling of duplicate paths by checking against cleaned filepaths
  instead of raw strings
* Add test cases for validation covering added validation processes

Signed-off-by: Angel Misevski &lt;amisevsk@gmail.com&gt;
diff --git a/pkg/artifact/kitfile.go b/pkg/artifact/kitfile.go
@@ -22,14 +22,22 @@ import (
 	"fmt"
 	"io"
 	"net/url"
+	"path"
+	"path/filepath"
+	"regexp"
 	"slices"
+	"strings"
 	"time"
 
 	modelspecv1 "github.com/modelpack/model-spec/specs-go/v1"
 	"github.com/opencontainers/go-digest"
 	"go.yaml.in/yaml/v3"
 )
 
+const modelPartTypeMaxLen = 64
+
+var modelPartTypeRegexp = regexp.MustCompile(`^[\w][\w.-]*$`)
+
 // PathType represents different types of paths in Kitfile fields, e.g. local on-disk paths or remote S3 URLs.
 type PathType int
 
@@ -164,79 +172,122 @@ func (kf *KitFile) MarshalToYAML() ([]byte, error) {
 }
 
 func (kf *KitFile) Validate() error {
+	var errs []string
+	addErr := func(format string, a ...any) {
+		s := fmt.Sprintf(format, a...)
+		errs = append(errs, fmt.Sprintf("  * %s", s))
+	}
 	if kf.ManifestVersion != "1.0.0" {
-		return fmt.Errorf("invalid manifestVersion: expect 1.0.0 but got %s", kf.ManifestVersion)
+		addErr("invalid manifestVersion: expect 1.0.0 but got %s", kf.ManifestVersion)
+	}
+
+	// Map of paths to the component that uses them; used to detect duplicate paths
+	paths := map[string][]string{}
+	addPath := func(path, source string) {
+		if path == "" {
+			path = "."
+		}
+		path = filepath.Clean(path)
+		paths[path] = append(paths[path], source)
 	}
 
 	if kf.Model != nil {
+		addPath(kf.Model.Path, fmt.Sprintf("model %s", kf.Model.Name))
 		modelPathType, err := GetPathType(kf.Model.Path)
 		if err != nil {
-			return fmt.Errorf("invalid path for model: %w", err)
+			addErr("invalid path for model: %s", err)
 		}
 		if modelPathType != LocalPathType && modelPathType != ModelReferencePathType {
-			return fmt.Errorf("invalid path for model: only local paths and ModelKit references are permitted")
+			addErr("invalid path for model: only local paths and ModelKit references are permitted")
 		}
 		for _, part := range kf.Model.Parts {
+			addPath(part.Path, fmt.Sprintf("modelpart %s", part.Name))
 			partPathType, err := GetPathType(part.Path)
 			if err != nil {
-				return fmt.Errorf("invalid path for model part (%s): %w", part.Path, err)
+				addErr("invalid path for model part (%s): %s", part.Path, err)
 			}
 			if partPathType != LocalPathType {
-				return fmt.Errorf("invalid path for model part (%s): only local paths are permitted", part.Path)
+				addErr("invalid path for model part (%s): only local paths are permitted", part.Path)
+			}
+			if part.Type != "" {
+				if !modelPartTypeRegexp.MatchString(part.Type) {
+					addErr("modelpart %s has invalid type (must be alphanumeric with dots, dashes, and underscores)", part.Name)
+				}
+				if len(part.Type) > modelPartTypeMaxLen {
+					addErr("modelpart %s type is too long (must be fewer than %d characters)", part.Name, modelPartTypeMaxLen)
+				}
 			}
 		}
 	}
-	for _, code := range kf.Code {
+	for idx, code := range kf.Code {
+		addPath(code.Path, fmt.Sprintf("code layer %d", idx))
 		pathType, err := GetPathType(code.Path)
 		if err != nil {
-			return fmt.Errorf("invalid path for code (%s): %w", code.Path, err)
+			addErr("invalid path for code (%s): %s", code.Path, err)
 		}
 		if pathType != LocalPathType {
-			return fmt.Errorf("invalid path for code (%s): only local paths are permitted", code.Path)
+			addErr("invalid path for code (%s): only local paths are permitted", code.Path)
 		}
 	}
-	for _, dataset := range kf.DataSets {
+	for idx, dataset := range kf.DataSets {
+		addPath(dataset.Path, fmt.Sprintf("dataset layer %d", idx))
 		pathType, err := GetPathType(dataset.Path)
 		if err != nil {
-			return fmt.Errorf("invalid path for dataset (%s): %w", dataset.Path, err)
+			addErr("invalid path for dataset (%s): %s", dataset.Path, err)
 		}
 		if pathType != LocalPathType {
-			return fmt.Errorf("invalid path for dataset (%s): only local paths are permitted", dataset.Path)
+			addErr("invalid path for dataset (%s): only local paths are permitted", dataset.Path)
 		}
 		if dataset.RemotePath != "" {
 			remotePathType, err := GetPathType(dataset.RemotePath)
 			if err != nil {
-				return fmt.Errorf("invalid remote path for dataset (%s): %w", dataset.RemotePath, err)
+				addErr("invalid remote path for dataset (%s): %s", dataset.RemotePath, err)
 			}
 			if remotePathType != S3PathType {
-				return fmt.Errorf("only S3 URLs are supported for remote dataset paths (%s)", dataset.RemotePath)
+				addErr("only S3 URLs are supported for remote dataset paths (%s)", dataset.RemotePath)
 			}
 			if dataset.RemoteHash == "" {
-				return fmt.Errorf("remoteHash is required when remote dataset paths are used (%s)", dataset.RemotePath)
+				addErr("remoteHash is required when remote dataset paths are used (%s)", dataset.RemotePath)
 			}
 		} else {
 			if dataset.RemoteHash != "" {
-				return fmt.Errorf("remote hash is only applicable when remotePath is set")
+				addErr("remote hash is only applicable when remotePath is set")
 			}
 		}
 	}
-	for _, doc := range kf.Docs {
+	for idx, doc := range kf.Docs {
+		addPath(doc.Path, fmt.Sprintf("docs layer %d", idx))
 		pathType, err := GetPathType(doc.Path)
 		if err != nil {
-			return fmt.Errorf("invalid path for doc (%s): %w", doc.Path, err)
+			addErr("invalid path for doc (%s): %s", doc.Path, err)
 		}
 		if pathType != LocalPathType {
-			return fmt.Errorf("invalid path for doc (%s): only local paths are permitted", doc.Path)
+			addErr("invalid path for doc (%s): only local paths are permitted", doc.Path)
 		}
 	}
 	for _, prompt := range kf.Prompts {
 		pathType, err := GetPathType(prompt.Path)
 		if err != nil {
-			return fmt.Errorf("invalid path for prompt (%s): %w", prompt.Path, err)
+			addErr("invalid path for prompt (%s): %s", prompt.Path, err)
 		}
 		if pathType != LocalPathType {
-			return fmt.Errorf("invalid path for prompt (%s): only local paths are permitted", prompt.Path)
+			addErr("invalid path for prompt (%s): only local paths are permitted", prompt.Path)
+		}
+	}
+
+	for layerPath, layerIds := range paths {
+		if len := len(layerIds); len > 1 {
+			addErr("%s and %s use the same path %s", strings.Join(layerIds[:len-1], ", "), layerIds[len-1], layerPath)
 		}
+		if path.IsAbs(layerPath) || filepath.IsAbs(layerPath) {
+			addErr("absolute paths are not supported in a Kitfile (path %s in %s)", layerPath, layerIds[0])
+		}
+	}
+
+	if len(errs) > 0 {
+		// Iterating through the paths map is random; sort to get a consistent message
+		slices.Sort(errs)
+		return fmt.Errorf("errors while validating Kitfile: \n%s", strings.Join(errs, "\n"))
 	}
 
 	return nil
diff --git a/pkg/artifact/kitfile_test.go b/pkg/artifact/kitfile_test.go
@@ -79,7 +79,7 @@ func (tc verifyTestCase) withName(name string) verifyTestCase {
 	return tc
 }
 
-func TestVerifyKitfile(t *testing.T) {
+func TestValidateKitfile(t *testing.T) {
 	tests := loadAllTestCasesOrPanic[verifyTestCase](t, filepath.Join("testdata", "validation"))
 	for _, tt := range tests {
 		t.Run(tt.Name, func(t *testing.T) {
@@ -170,9 +170,9 @@ model:
   path: model-files
   license: "license-h"
   parts:
-  - path: part-files
+  - path: part-files-1
     license: "license-f"
-  - path: part-files
+  - path: part-files-2
     license: "license-e"
 datasets:
 - path: dataset
@@ -198,9 +198,9 @@ model:
   path: model-files
   license: MIT
   parts:
-  - path: part-files
+  - path: part-files-1
     license: Apache-2.0
-  - path: part-files
+  - path: part-files-2
     license: MIT
 datasets:
 - path: dataset
diff --git a/pkg/artifact/testdata/parameters/test-basic-parameters.yaml b/pkg/artifact/testdata/parameters/test-basic-parameters.yaml
@@ -5,9 +5,9 @@ kitfileYaml: |
     name: test-kitfile
   model:
     name: test-model
-    path: /tmp/path/to/model
+    path: tmp/path/to/model
     parameters:
       test-a: a
       test-b: b
       test-c: c
-kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"/tmp/path/to/model","parameters":{"test-a":"a","test-b":"b","test-c":"c"}}}'
+kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"tmp/path/to/model","parameters":{"test-a":"a","test-b":"b","test-c":"c"}}}'
diff --git a/pkg/artifact/testdata/parameters/test-data-types.yaml b/pkg/artifact/testdata/parameters/test-data-types.yaml
@@ -5,12 +5,12 @@ kitfileYaml: |
     name: test-kitfile
   model:
     name: test-model
-    path: /tmp/path/to/model
+    path: tmp/path/to/model
     parameters:
       test-bool: true
       test-bool-str: "true"
       test-float: 12.3
       test-negative: -123
       test-number: 123
       test-string: string
-kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"/tmp/path/to/model","parameters":{"test-bool":true,"test-bool-str":"true","test-float":12.3,"test-negative":-123,"test-number":123,"test-string":"string"}}}'
+kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"tmp/path/to/model","parameters":{"test-bool":true,"test-bool-str":"true","test-float":12.3,"test-negative":-123,"test-number":123,"test-string":"string"}}}'
diff --git a/pkg/artifact/testdata/parameters/test-list-parameters.yaml b/pkg/artifact/testdata/parameters/test-list-parameters.yaml
@@ -5,9 +5,9 @@ kitfileYaml: |
     name: test-kitfile
   model:
     name: test-model
-    path: /tmp/path/to/model
+    path: tmp/path/to/model
     parameters:
       - item-a
       - item-b
       - item-c
-kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"/tmp/path/to/model","parameters":["item-a","item-b","item-c"]}}'
+kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"tmp/path/to/model","parameters":["item-a","item-b","item-c"]}}'
diff --git a/pkg/artifact/testdata/parameters/test-nested-parameters.yaml b/pkg/artifact/testdata/parameters/test-nested-parameters.yaml
@@ -5,7 +5,7 @@ kitfileYaml: |
     name: test-kitfile
   model:
     name: test-model
-    path: /tmp/path/to/model
+    path: tmp/path/to/model
     parameters:
       test-list-of-lists:
         - - a-1
@@ -28,4 +28,4 @@ kitfileYaml: |
         test-a: a
         test-b: b
         test-c: c
-kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"/tmp/path/to/model","parameters":{"test-list-of-lists":[["a-1","a-2"],["b-1","b-2"]],"test-maps":{"a":{"b":{"c":{"d":{"e":"f"}}}}},"test-nested-list":["item-a","item-b",{"item-object":[{"subfield-1":1},{"subfield-2":2}]}],"test-nested-map":{"test-a":"a","test-b":"b","test-c":"c"}}}}'
+kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"tmp/path/to/model","parameters":{"test-list-of-lists":[["a-1","a-2"],["b-1","b-2"]],"test-maps":{"a":{"b":{"c":{"d":{"e":"f"}}}}},"test-nested-list":["item-a","item-b",{"item-object":[{"subfield-1":1},{"subfield-2":2}]}],"test-nested-map":{"test-a":"a","test-b":"b","test-c":"c"}}}}'
diff --git a/pkg/artifact/testdata/parameters/test-single-value.yaml b/pkg/artifact/testdata/parameters/test-single-value.yaml
@@ -5,7 +5,7 @@ kitfileYaml: |
     name: test-kitfile
   model:
     name: test-model
-    path: /tmp/path/to/model
+    path: tmp/path/to/model
     parameters: |
       test-string
-kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"/tmp/path/to/model","parameters":"test-string\n"}}'
+kitfileJson: '{"manifestVersion":"1.0.0","package":{"name":"test-kitfile"},"model":{"name":"test-model","path":"tmp/path/to/model","parameters":"test-string\n"}}'
diff --git a/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_1.yaml b/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_1.yaml
@@ -0,0 +1,7 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  datasets:
+    - path: test-path
+  code:
+    - path: test-path
+errRegexp: "use the same path test-path"
diff --git a/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_2.yaml b/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_2.yaml
@@ -0,0 +1,7 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path
+  code:
+    - path: test-path
+errRegexp: "use the same path test-path"
diff --git a/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_3.yaml b/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_3.yaml
@@ -0,0 +1,7 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path/
+  datasets:
+    - path: test-path
+errRegexp: "use the same path test-path"
diff --git a/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_4.yaml b/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_4.yaml
@@ -0,0 +1,7 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path/
+    parts:
+      - path: ./test-path
+errRegexp: "use the same path test-path"
diff --git a/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_5.yaml b/pkg/artifact/testdata/validation/duplicate-path-in-kitfile_5.yaml
@@ -0,0 +1,7 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path/
+    parts:
+      - path: ./some/meandering/../../test-path
+errRegexp: "use the same path test-path"
diff --git a/pkg/artifact/testdata/validation/invalid-model-part-type-length.yaml b/pkg/artifact/testdata/validation/invalid-model-part-type-length.yaml
@@ -0,0 +1,8 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path
+    parts:
+      - path: part-path
+        type: valid-otherwise-but-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long
+errRegexp: "type is too long "
diff --git a/pkg/artifact/testdata/validation/invalid-model-part-type.yaml b/pkg/artifact/testdata/validation/invalid-model-part-type.yaml
@@ -0,0 +1,8 @@
+kitfile: |
+  manifestVersion: 1.0.0
+  model:
+    path: test-path
+    parts:
+      - path: part-path
+        type: contains a space
+errRegexp: "has invalid type"
diff --git a/pkg/cmd/kitimport/gitimport.go b/pkg/cmd/kitimport/gitimport.go
@@ -29,7 +29,6 @@ import (
 	"github.com/kitops-ml/kitops/pkg/lib/external/git"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem/cache"
-	kfutils "github.com/kitops-ml/kitops/pkg/lib/kitfile"
 	kfgen "github.com/kitops-ml/kitops/pkg/lib/kitfile/generate"
 	"github.com/kitops-ml/kitops/pkg/lib/util"
 	"github.com/kitops-ml/kitops/pkg/output"
@@ -57,9 +56,6 @@ func importUsingGit(ctx context.Context, opts *importOptions) error {
 		if err := kitfile.LoadModel(os.Stdin); err != nil {
 			return fmt.Errorf("failed to read Kitfile from input: %w", err)
 		}
-		if err := kfutils.ValidateKitfile(kitfile); err != nil {
-			return err
-		}
 	} else if opts.kitfilePath != "" {
 		kf, err := readExistingKitfile(opts.kitfilePath)
 		if err != nil {
diff --git a/pkg/cmd/kitimport/hfimport.go b/pkg/cmd/kitimport/hfimport.go
@@ -28,7 +28,6 @@ import (
 	"github.com/kitops-ml/kitops/pkg/lib/external/hf"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem/cache"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem/ignore"
-	kfutils "github.com/kitops-ml/kitops/pkg/lib/kitfile"
 	kfgen "github.com/kitops-ml/kitops/pkg/lib/kitfile/generate"
 	repoutil "github.com/kitops-ml/kitops/pkg/lib/repo/util"
 	"github.com/kitops-ml/kitops/pkg/lib/util"
@@ -64,9 +63,6 @@ func importUsingHF(ctx context.Context, opts *importOptions) error {
 		if err := kitfile.LoadModel(os.Stdin); err != nil {
 			return fmt.Errorf("failed to read Kitfile from input: %w", err)
 		}
-		if err := kfutils.ValidateKitfile(kitfile); err != nil {
-			return err
-		}
 	} else if opts.kitfilePath != "" {
 		kf, err := readExistingKitfile(opts.kitfilePath)
 		if err != nil {
diff --git a/pkg/cmd/kitimport/util.go b/pkg/cmd/kitimport/util.go
@@ -32,7 +32,6 @@ import (
 	"github.com/kitops-ml/kitops/pkg/lib/constants/mediatype"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem"
 	"github.com/kitops-ml/kitops/pkg/lib/filesystem/ignore"
-	kfutils "github.com/kitops-ml/kitops/pkg/lib/kitfile"
 	kfgen "github.com/kitops-ml/kitops/pkg/lib/kitfile/generate"
 	"github.com/kitops-ml/kitops/pkg/lib/repo/local"
 	"github.com/kitops-ml/kitops/pkg/lib/util"
@@ -79,9 +78,6 @@ func readExistingKitfile(kfPath string) (*artifact.KitFile, error) {
 	if err := kitfile.LoadModel(kfFile); err != nil {
 		return nil, fmt.Errorf("failed to load Kitfile from %s: %w", kfPath, err)
 	}
-	if err := kfutils.ValidateKitfile(kitfile); err != nil {
-		return nil, fmt.Errorf("kitfile (%s) is invalid: %w", kfPath, err)
-	}
 	return kitfile, nil
 }
 
diff --git a/pkg/cmd/pack/pack.go b/pkg/cmd/pack/pack.go
@@ -123,9 +123,6 @@ func readKitfile(modelFile string) (*artifact.KitFile, error) {
 	if err := kitfile.LoadModel(kitfileContentReader); err != nil {
 		return nil, err
 	}
-	if err := kfutils.ValidateKitfile(kitfile); err != nil {
-		return nil, err
-	}
 	return kitfile, nil
 }
 
diff --git a/pkg/lib/kitfile/resolve.go b/pkg/lib/kitfile/resolve.go
@@ -79,9 +79,6 @@ func ResolveKitfile(ctx context.Context, configHome, kitfileRef, baseRef string)
 		}
 		resolved = mergeKitfiles(resolved, kitfile)
 		if resolved.Model == nil || !artifact.IsModelKitReference(resolved.Model.Path) {
-			if err := ValidateKitfile(resolved); err != nil {
-				return nil, err
-			}
 			return resolved, nil
 		}
 		if idx := getIndex(refChain, resolved.Model.Path); idx != -1 {
diff --git a/pkg/lib/kitfile/validate.go b/pkg/lib/kitfile/validate.go
