diff --git a/docs/reference/cli-validate.md b/docs/reference/cli-validate.md index 8572cf6..c3f46c4 100644 --- a/docs/reference/cli-validate.md +++ b/docs/reference/cli-validate.md @@ -14,7 +14,35 @@ loom validate [path] - Parameter names are non-empty and unique - Operation names are non-empty and unique - Each operation has exactly one action type -- Patch engines are valid (`smp` or `json6902`) +- Required fields per action type are present +- Enums are valid: patch engine (`smp` / `json6902`), PR provider, LLM provider and mode +- Durations parse: `shell.timeout`, `llm.retryDelay` +- Every templatable field parses as a Go template and only references declared params +- Exclude/include patterns are usable globs — see [File filtering](#file-filtering) below +- Destinations stay inside the target directory: `patch.target`, `newFiles.dest`, `llm.target` +- `newFiles.source` is an existing directory and `patch.path` an existing file +- No patch file is also rendered into the target as module output + +Checks that depend on a value only known at run time are skipped when the field +is templated. + +## File filtering + +Exclude and include patterns are matched against **base names** with +`filepath.Match`, and a pattern that fails to compile silently matches nothing. +Both are easy to get wrong in a way a run never reports, so `validate` rejects +them up front: + +```yaml +excludes: + - "__functions/patches/*.yaml" # error: matches base names only + - "__functions[" # error: invalid glob pattern + - "__functions" # correct +``` + +The same silent failure mode is why a patch file that survives a `newFiles` +walk is an error — it would be rendered into the target as output instead of +being applied as a patch. Fix it by excluding the directory the patch lives in. ## Example @@ -22,4 +50,6 @@ loom validate [path] loom validate ./onboard-service ``` -On success, exits with code 0 and prints nothing. On failure, prints validation errors and exits with a non-zero code. +All violations are reported together, one per line, so a config can be fixed in +a single pass. On success, prints a confirmation and exits 0; on failure, prints +the violations and exits non-zero. diff --git a/pkg/config/validate.go b/pkg/config/validate.go index 7ccfaac..7951795 100644 --- a/pkg/config/validate.go +++ b/pkg/config/validate.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "sort" "strings" "text/template" @@ -110,10 +111,14 @@ func validate(lf *LoomFile, moduleDir string) error { } for i, e := range lf.Spec.Excludes { - checkTmpl(fmt.Sprintf("spec.excludes[%d]", i), e) + field := fmt.Sprintf("spec.excludes[%d]", i) + checkTmpl(field, e) + checkGlob(fail, field, e) } for i, e := range lf.Spec.Includes { - checkTmpl(fmt.Sprintf("spec.includes[%d]", i), e) + field := fmt.Sprintf("spec.includes[%d]", i) + checkTmpl(field, e) + checkGlob(fail, field, e) } if lf.Spec.Target != nil { @@ -151,6 +156,11 @@ func validate(lf *LoomFile, moduleDir string) error { } } + // Non-templated newFiles sources and patch paths, paired with their + // operation names, for the cross-operation check after the loop. + type opPath struct{ op, path string } + var newFilesSources, patchPaths []opPath + opNames := make(map[string]bool) for _, op := range lf.Spec.Operations { if op.Name == "" { @@ -196,10 +206,13 @@ func validate(lf *LoomFile, moduleDir string) error { fail("operation %q: newFiles source %q not found in module directory", op.Name, op.NewFiles.Source) } else if !info.IsDir() { fail("operation %q: newFiles source %q is not a directory", op.Name, op.NewFiles.Source) + } else { + newFilesSources = append(newFilesSources, opPath{op.Name, op.NewFiles.Source}) } } checkTmpl(fmt.Sprintf("operation %q newFiles.source", op.Name), op.NewFiles.Source) checkTmpl(fmt.Sprintf("operation %q newFiles.dest", op.Name), op.NewFiles.Dest) + checkTargetPath(fail, op.Name, "newFiles dest", op.NewFiles.Dest) } if op.Patch != nil { @@ -211,6 +224,8 @@ func validate(lf *LoomFile, moduleDir string) error { fail("operation %q: patch file %q not found in module directory", op.Name, op.Patch.Path) } else if info.IsDir() { fail("operation %q: patch path %q is a directory, expected a file", op.Name, op.Patch.Path) + } else { + patchPaths = append(patchPaths, opPath{op.Name, op.Patch.Path}) } } if op.Patch.Target == "" { @@ -237,6 +252,7 @@ func validate(lf *LoomFile, moduleDir string) error { checkTmpl(fmt.Sprintf("operation %q patch.path", op.Name), op.Patch.Path) checkTmpl(fmt.Sprintf("operation %q patch.target", op.Name), op.Patch.Target) checkTmpl(fmt.Sprintf("operation %q patch.preserveComments", op.Name), op.Patch.PreserveComments) + checkTargetPath(fail, op.Name, "patch target", op.Patch.Target) } if op.Shell != nil { @@ -311,6 +327,11 @@ func validate(lf *LoomFile, moduleDir string) error { if op.LLM.Retries < 0 { fail("operation %q: llm retries must be >= 0", op.Name) } + // A negative limit is silently dropped at call time (only > 0 is + // forwarded to the provider), so the run would quietly ignore it. + if op.LLM.MaxTokens < 0 { + fail("operation %q: llm maxTokens must be >= 0", op.Name) + } if op.LLM.RetryDelay != "" && !isTemplated(op.LLM.RetryDelay) { if _, err := time.ParseDuration(op.LLM.RetryDelay); err != nil { fail("operation %q: invalid llm retryDelay %q: %v", op.Name, op.LLM.RetryDelay, err) @@ -326,6 +347,7 @@ func validate(lf *LoomFile, moduleDir string) error { checkTmpl(fmt.Sprintf("operation %q llm.prompt", op.Name), op.LLM.Prompt) checkTmpl(fmt.Sprintf("operation %q llm.systemPrompt", op.Name), op.LLM.SystemPrompt) checkTmpl(fmt.Sprintf("operation %q llm.target", op.Name), op.LLM.Target) + checkTargetPath(fail, op.Name, "llm target", op.LLM.Target) checkTmpl(fmt.Sprintf("operation %q llm.mode", op.Name), op.LLM.Mode) checkTmpl(fmt.Sprintf("operation %q llm.retryDelay", op.Name), op.LLM.RetryDelay) if pc := op.LLM.ProviderConfig; pc != nil { @@ -336,9 +358,81 @@ func validate(lf *LoomFile, moduleDir string) error { } } + // A patch file that also survives a newFiles walk is rendered into the + // target as module output — the classic symptom of forgetting to list the + // utility directory in spec.excludes. Skipped when any filter pattern is + // templated, since the run-time filter would then differ from this one. + if moduleDir != "" && len(newFilesSources) > 0 && len(patchPaths) > 0 && !anyTemplated(lf.Spec.Excludes, lf.Spec.Includes) { + filter := &util.FilterOptions{Excludes: lf.Spec.Excludes, Includes: lf.Spec.Includes} + for _, src := range newFilesSources { + srcDir := util.ExpandPath(moduleDir, src.path) + rendered, err := util.WalkTemplateFiles(srcDir, filter) + if err != nil { + continue + } + renderedSet := make(map[string]bool, len(rendered)) + for _, f := range rendered { + renderedSet[f] = true + } + for _, p := range patchPaths { + rel, err := filepath.Rel(srcDir, util.ExpandPath(moduleDir, p.path)) + if err != nil || !renderedSet[rel] { + continue + } + fail("operation %q: patch file %q is also rendered into the target by newFiles operation %q — exclude it via spec.excludes", p.op, p.path, src.op) + } + } + } + return errors.Join(errs...) } +// checkGlob records a violation for exclude/include patterns that can never +// match at run time. Filtering matches base names with filepath.Match and +// swallows its error, so a malformed pattern or one carrying a path separator +// silently matches nothing instead of failing the run. Templated patterns are +// resolved at run time and skipped. +func checkGlob(fail func(string, ...any), field, pattern string) { + if isTemplated(pattern) { + return + } + if pattern == "" { + fail("%s: pattern cannot be empty", field) + return + } + if strings.Contains(pattern, "/") { + fail("%s: pattern %q contains a path separator, but patterns match base names only", field, pattern) + return + } + if _, err := filepath.Match(pattern, "x"); err != nil { + fail("%s: invalid glob pattern %q: %v", field, pattern, err) + } +} + +// checkTargetPath records a violation for a destination that would resolve +// outside the target directory once joined to it at run time. Templated values +// are resolved at run time and skipped. +func checkTargetPath(fail func(string, ...any), opName, field, value string) { + if value == "" || isTemplated(value) { + return + } + if clean := filepath.Clean(value); clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + fail("operation %q: %s %q escapes the target directory", opName, field, value) + } +} + +// anyTemplated reports whether any string in the given lists is templated. +func anyTemplated(lists ...[]string) bool { + for _, l := range lists { + for _, s := range l { + if isTemplated(s) { + return true + } + } + } + return false +} + // isTemplated returns true if the string contains Go template expressions. func isTemplated(s string) bool { return strings.Contains(s, "{{") diff --git a/pkg/config/validate_test.go b/pkg/config/validate_test.go index 02e8afa..6d407cc 100644 --- a/pkg/config/validate_test.go +++ b/pkg/config/validate_test.go @@ -1228,3 +1228,227 @@ func TestValidate_LLMVertexWithProviderConfig(t *testing.T) { t.Fatalf("unexpected error: %v", err) } } + +// --- exclude/include glob patterns --- + +func TestValidate_ExcludeMalformedGlob(t *testing.T) { + lf := validLoomFile() + lf.Spec.Excludes = []string{"__functions["} + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for malformed exclude glob") + } + if !strings.Contains(err.Error(), `spec.excludes[0]: invalid glob pattern "__functions["`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_IncludeMalformedGlob(t *testing.T) { + lf := validLoomFile() + lf.Spec.Includes = []string{"[a-"} + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for malformed include glob") + } + if !strings.Contains(err.Error(), `spec.includes[0]: invalid glob pattern "[a-"`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_ExcludeWithPathSeparator(t *testing.T) { + lf := validLoomFile() + lf.Spec.Excludes = []string{"__functions/patches/*.yaml"} + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for exclude pattern with a path separator") + } + if !strings.Contains(err.Error(), "contains a path separator") { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_ExcludeEmptyPattern(t *testing.T) { + lf := validLoomFile() + lf.Spec.Excludes = []string{""} + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for empty exclude pattern") + } + if !strings.Contains(err.Error(), "spec.excludes[0]: pattern cannot be empty") { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_ExcludeValidGlobs(t *testing.T) { + lf := validLoomFile() + lf.Spec.Excludes = []string{"__functions", "*.md", "[abc]*.yaml"} + lf.Spec.Includes = []string{"README.md"} + + if err := Validate(lf); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestValidate_ExcludeTemplatedGlobSkipped(t *testing.T) { + lf := validLoomFile() + lf.Spec.Params = []ParamDef{{Name: "svc"}} + // Would look like a separator violation once rendered — resolved at run time. + lf.Spec.Excludes = []string{"{{ .svc }}"} + + if err := Validate(lf); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// --- destination paths escaping the target dir --- + +func TestValidate_PatchTargetEscapesTargetDir(t *testing.T) { + lf := validLoomFile() + lf.Spec.Operations = []Operation{ + {Name: "p", Patch: &Patch{Path: "patch.yaml", Target: "../../etc/passwd"}}, + } + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for patch target escaping the target dir") + } + if !strings.Contains(err.Error(), `patch target "../../etc/passwd" escapes the target directory`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_NewFilesDestEscapesTargetDir(t *testing.T) { + lf := validLoomFile() + lf.Spec.Operations = []Operation{ + {Name: "nf", NewFiles: &NewFiles{Source: "templates", Dest: "sub/../../out"}}, + } + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for newFiles dest escaping the target dir") + } + if !strings.Contains(err.Error(), `newFiles dest "sub/../../out" escapes the target directory`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_LLMTargetEscapesTargetDir(t *testing.T) { + lf := validLoomFile() + lf.Spec.Operations = []Operation{ + {Name: "gen", LLM: &LLM{ + Provider: "openai", + Model: "gpt-4o", + Prompt: "hi", + Target: "../out.yaml", + }}, + } + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for llm target escaping the target dir") + } + if !strings.Contains(err.Error(), `llm target "../out.yaml" escapes the target directory`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidate_TargetPathsInsideTargetDirOK(t *testing.T) { + lf := validLoomFile() + lf.Spec.Params = []ParamDef{{Name: "svc"}} + lf.Spec.Operations = []Operation{ + {Name: "p", Patch: &Patch{Path: "patch.yaml", Target: "app/../k8s/deploy.yaml"}}, + {Name: "nf", NewFiles: &NewFiles{Source: "templates", Dest: "charts"}}, + // Templated: resolved at run time, so not checked here. + {Name: "nf2", NewFiles: &NewFiles{Source: "templates", Dest: "{{ .svc }}/../out"}}, + } + + if err := Validate(lf); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// --- llm maxTokens --- + +func TestValidate_LLMNegativeMaxTokens(t *testing.T) { + lf := validLoomFile() + lf.Spec.Operations = []Operation{ + {Name: "gen", LLM: &LLM{ + Provider: "openai", + Model: "gpt-4o", + Prompt: "hi", + Target: "out.yaml", + MaxTokens: -1, + }}, + } + + err := Validate(lf) + if err == nil { + t.Fatal("expected error for negative llm maxTokens") + } + if !strings.Contains(err.Error(), "llm maxTokens must be >= 0") { + t.Errorf("unexpected error: %v", err) + } +} + +// --- patch file leaking into newFiles output (ValidateInDir) --- + +// patchLeakModule builds a module dir with a newFiles source directory that +// also holds the patch file, plus the matching config. +func patchLeakModule(t *testing.T, excludes []string) (string, *LoomFile) { + t.Helper() + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "__functions", "patches"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "__functions", "patches", "deploy.yaml"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + lf := validLoomFile() + lf.Spec.Excludes = excludes + lf.Spec.Operations = []Operation{ + {Name: "nf", NewFiles: &NewFiles{Source: "."}}, + {Name: "p", Patch: &Patch{Path: "__functions/patches/deploy.yaml", Target: "deploy.yaml"}}, + } + return dir, lf +} + +func TestValidateInDir_PatchFileRenderedByNewFiles(t *testing.T) { + dir, lf := patchLeakModule(t, nil) + + err := ValidateInDir(lf, dir) + if err == nil { + t.Fatal("expected error for patch file rendered into the target by newFiles") + } + if !strings.Contains(err.Error(), `patch file "__functions/patches/deploy.yaml" is also rendered into the target by newFiles operation "nf"`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestValidateInDir_PatchFileExcludedFromNewFiles(t *testing.T) { + dir, lf := patchLeakModule(t, []string{"__functions"}) + + if err := ValidateInDir(lf, dir); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestValidateInDir_PatchLeakSkippedWhenFilterTemplated(t *testing.T) { + dir, lf := patchLeakModule(t, []string{"{{ .svc }}"}) + lf.Spec.Params = []ParamDef{{Name: "svc"}} + + if err := ValidateInDir(lf, dir); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestValidate_PatchLeakNotCheckedWithoutModuleDir(t *testing.T) { + _, lf := patchLeakModule(t, nil) + + if err := Validate(lf); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/specs/module.md b/specs/module.md index cabf7ae..58087fb 100644 --- a/specs/module.md +++ b/specs/module.md @@ -961,6 +961,15 @@ When the module directory is known (`loom validate`, `loom run`, `loom bulk` file, resolved against the module directory exactly as at run time. Templated paths are skipped. +Two classes of mistake are rejected because the run itself cannot report them. +File filtering (F1/F2) matches base names with `filepath.Match` and discards its +error, so an exclude/include pattern that is malformed or carries a path +separator silently matches nothing; both are violations. For the same reason, a +patch file that survives a `newFiles` walk is a violation: it would be rendered +into the target as module output instead of being applied as a patch (the +`__functions` footgun in F3). That last check is skipped when any +exclude/include pattern is templated, since the run-time filter would differ. + | Rule | Error | |------|-------| | Config contains only known fields (strict decoding; applies to `loom.yaml` and evaluated `loom.jsonnet` output) | `parsing loom.yaml: ... field not found in type ...` | @@ -971,6 +980,9 @@ paths are skipped. | Param names unique across `params` and `dynamicParams` | `duplicate param name ""` | | Dynamic param `command` required | `dynamicParam "": command is required` | | `spec.target.url` required when `spec.target` present | `spec.target.url is required` | +| Exclude/include patterns non-empty (skipped when templated) | `spec.excludes[]: pattern cannot be empty` | +| Exclude/include patterns carry no path separator, since only base names are matched (skipped when templated) | `spec.excludes[]: pattern "

" contains a path separator, but patterns match base names only` | +| Exclude/include patterns compile as globs (skipped when templated) | `spec.includes[]: invalid glob pattern "

": ` | | Module names non-empty | `module name cannot be empty` | | Module names unique | `duplicate module name ""` | | Module `source` required | `module "": source is required` | @@ -983,6 +995,9 @@ paths are skipped. | `patch.path` exists and is a file (module dir known, non-templated) | `operation "": patch file "" not found in module directory` | | `patch.target` required | `operation "": patch target is required` | | Patch engine is `smp` or `json6902` (skipped when templated) | `unknown patch engine ""` | +| `patch.target`, `newFiles.dest`, `llm.target` stay inside the target directory (skipped when templated) | `operation "": patch target "" escapes the target directory` | +| A patch file is not also rendered into the target by a `newFiles` operation (module dir known, non-templated paths and filters) | `operation "": patch file "" is also rendered into the target by newFiles operation "" — exclude it via spec.excludes` | +| `llm.maxTokens` is >= 0 | `operation "": llm maxTokens must be >= 0` | | `shell.command` required | `operation "": shell command is required` | | `shell.timeout` is a valid duration (skipped when templated) | `operation "": invalid shell timeout ""` | | `commitPush.message` required | `operation "": commitPush message is required` |