check refuses placeholders that could never expand

4 files changed, 61 insertions, 0 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10aa9b1..1cdf8b5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,10 @@
   "keep" rule like `(rule "keep" (when (content "confidential")) (stop))`
   protects an unreadable file from the rules below it. `explain` shows the
   rule as "undecided" and the later rules as stopped by it.
+- `krino check`, and every run, refuses a placeholder that could never
+  expand (`{foo}`, `{mtime}` without a format, `%B`, `{0}`, `{10}`, an
+  unclosed `{`) at the action's file:line:col, instead of skipping the step
+  at plan time.
 - `make ci` passes on OpenBSD 7.9 (OpenBSD make) and FreeBSD 15.0 (bmake)
   with Go 1.26.8; the design notes that OpenBSD's `go` package needs
   `GOTOOLCHAIN=auto` to use it.
diff --git a/internal/engine/engine.go b/internal/engine/engine.go
index 00a249d..958e4f9 100644
--- a/internal/engine/engine.go
+++ b/internal/engine/engine.go
@@ -177,6 +177,11 @@ func Load(mainFile string, names ...string) (*Engine, []*config.Diag) {
 func checkCaptures(file string, r *config.Rule, c *cond.Cond) *config.Diag {
 	groups := c.NameGroups()
 	for _, a := range r.Actions {
+		if err := plan.CheckTemplate(a.Arg); err != nil {
+			// A placeholder that could never expand is a config error, not a
+			// step skipped at plan time (plan 12).
+			return &config.Diag{File: file, Pos: a.Pos, Msg: fmt.Sprintf("rule %q: %v", r.Name, err)}
+		}
 		n, err := plan.MaxIndex(a.Arg)
 		if err != nil || n == 0 {
 			continue
diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go
index 9de3ea4..4af8f79 100644
--- a/internal/engine/engine_test.go
+++ b/internal/engine/engine_test.go
@@ -256,3 +256,41 @@ func TestLoadAcceptsDuplicateWithMove(t *testing.T) {
 		}
 	}
 }
+
+// TestLoadRefusesBadPlaceholders: a copy or move destination or a rename
+// name whose placeholders could never expand is a config error at load, so
+// krino check reports it at the action's position instead of a run skipping
+// the step (plan 12).
+func TestLoadRefusesBadPlaceholders(t *testing.T) {
+	for _, c := range []struct {
+		action, want string
+	}{
+		{`(move "Out/{foo}")`, "unknown placeholder {foo}"},
+		{`(move "Out/{mtime}")`, "unknown placeholder {mtime}"},
+		{`(move "Out/{now}")`, "unknown placeholder {now}"},
+		{`(move "Out/{mtime:%B}")`, "%B"},
+		{`(rename "{0}-x")`, "numbered from 1"},
+		{`(copy "Out/{10}")`, "unknown placeholder {10}"},
+		{`(move "Out/{name")`, "unclosed placeholder"},
+	} {
+		h := sandbox(t)
+		os.MkdirAll(filepath.Join(h, "dl"), 0o755)
+		main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (when (name \"^(a)\")) " + c.action + ")\n"})
+		_, errs := Load(main)
+		found := false
+		for _, e := range errs {
+			if strings.Contains(e.Error(), c.want) && strings.Contains(e.Error(), "dl.conf:2:") {
+				found = true
+			}
+		}
+		if !found {
+			t.Errorf("%s: errors %v; want %q at line 2", c.action, errs, c.want)
+		}
+	}
+	h := sandbox(t)
+	os.MkdirAll(filepath.Join(h, "dl"), 0o755)
+	main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (when (name \"^(a)\")) (rename \"{1}-{stem}{ext}\") (move \"Out/{mtime:%Y/%m}/{{x}}/{now:%j}\"))\n"})
+	if _, errs := Load(main); len(errs) > 0 {
+		t.Errorf("valid placeholders refused: %v", errs)
+	}
+}
diff --git a/internal/plan/index.go b/internal/plan/index.go
index abe073f..0409295 100644
--- a/internal/plan/index.go
+++ b/internal/plan/index.go
@@ -6,8 +6,22 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"time"
 )
 
+// CheckTemplate reports the first reason s's placeholders could never
+// expand - an unknown placeholder, {mtime} or {now} without a format, an
+// unknown format code, {0}, {10} and up, an unclosed "{" - or nil. Capture
+// counts are not checked here; that needs the rule's name tests.
+func CheckTemplate(s string) error {
+	if _, err := MaxIndex(s); err != nil {
+		return err
+	}
+	facts := Facts{Name: "a.b", Captures: make([]string, 10), ModTime: time.Unix(0, 0), Now: time.Unix(0, 0)}
+	_, err := Expand(s, facts)
+	return err
+}
+
 // MaxIndex returns the highest {N} used in s, 0 when none. It reports the
 // same errors Expand does for a malformed placeholder: an unclosed
 // placeholder, or {0} (capture groups are numbered from 1). It shares