Skip to content

fix(validate): catch config mistakes a run reports silently#40

Open
rickliujh wants to merge 1 commit into
mainfrom
fix/validate-silent-config-gaps
Open

fix(validate): catch config mistakes a run reports silently#40
rickliujh wants to merge 1 commit into
mainfrom
fix/validate-silent-config-gaps

Conversation

@rickliujh

Copy link
Copy Markdown
Owner

loom validate accepted several configs that a real run then got wrong without saying so. Each gap below is a case where nothing — not validate, not the run — tells you the module misbehaved.

Same fixture module, before vs. after:

main:  ✔ module config is valid
       (…and the run wrote __functions/patches/deploy.yaml into the target as output)

this:  ✖ spec.excludes[0]: pattern "__functions/patches/*.yaml" contains a path separator, but patterns match base names only
       operation "patch-deploy": patch target "../../../etc/deploy.yaml" escapes the target directory
       operation "patch-deploy": patch file "__functions/patches/deploy.yaml" is also rendered into the target by newFiles operation "new-files" — exclude it via spec.excludes

What was missing

  • Dead glob patterns. File filtering matches base names with filepath.Match and discards its error (internal/util/fs.go:27). So "__functions[" (malformed) and "__functions/patches/*.yaml" (separator) each matched nothing, silently, and the files they were meant to filter were rendered into the target. Empty patterns too.
  • Patch file leaking into newFiles output — the F3 __functions footgun. ValidateInDir now replays the real walk filter over each non-templated newFiles.source and errors if a patch file survives it. Skipped when any exclude/include is templated, since the run-time filter would differ.
  • Destinations escaping the target dir. patch.target, newFiles.dest and llm.target were filepath.Join'd to the target dir unchecked, so a ../ prefix wrote outside the clone.
  • llm.maxTokens < 0 was dropped at call time (pkg/llm/llm.go:52 only forwards > 0), so a negative limit was silently ignored. retries already had this check.

All four are reported with the rest of the violations in one pass, and all are skipped when the field is templated.

Also

  • specs/module.md: new rows in the Validation Rules table plus a note on why these two classes are rejected (the run cannot report them).
  • docs/reference/cli-validate.md: the "What It Checks" list was badly stale — it omitted template checks, filesystem checks, required fields and most enums, and claimed success prints nothing.

Test plan

  • 18 new tests in pkg/config/validate_test.go covering each rule, its templated-skip path, and the valid cases.
  • go build ./... and go test ./... green.
  • Driven end-to-end with a real fixture: confirmed the pre-change binary reports "valid" and then leaks the patch file into the target on loom run --local-run, and that the new binary rejects the config.

Not included

A param name that isn't a valid Go template identifier (my-param) is unusable via {{ .my-param }} but legal via {{ index . "my-param" }}, so flagging it would risk false positives. Left alone.

🤖 Generated with Claude Code

loom validate accepted several configs that a real run then got wrong
without saying so:

- Exclude/include patterns are matched against base names with
  filepath.Match, whose error is discarded — a malformed pattern or one
  carrying a path separator silently matched nothing, so files meant to
  be filtered were rendered into the target.
- A patch file that survives a newFiles walk is delivered as module
  output instead of being applied as a patch (the __functions footgun).
- patch.target / newFiles.dest / llm.target were joined to the target
  dir unchecked, so a "../" prefix wrote outside it.
- llm.maxTokens < 0 was dropped at call time (only > 0 is forwarded).

All four are now violations, reported with the rest in one pass and
skipped when the value is templated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant