feat(config): validate jobs, not just the global section (#778)

Closes #773. Depends on nothing; complements #777, which fixed the
daemon half.

## The validator never saw a job

It walks structs and skipped maps — and every job lives in one. So an
unparsable schedule passed `validate` with the strict flag on or off,
and the only later sign was a warning while the daemon started and the
job never fired.

Two things were needed to reach a job's fields:

- **Job sections are maps**, so the walk descends into them.
- **Every job type embeds `core.<Kind>Job`, which embeds `BareJob`** —
where `schedule` and `command` live. Squashed structs were skipped too,
so even after traversing the maps the schedule stayed invisible. They
are now descended into **only inside a job**, so the global section
keeps the behaviour it had.

## Strict, but not the wrong kind of strict

Applying the existing rule to jobs would have rejected configurations
that work today. *"A field with no `default` tag is required"* is a
heuristic tuned to the global section; on a job it demands nearly every
key a job can carry.

So that rule is suppressed inside jobs, and what a job needs is stated
explicitly — taken from what the runtime already demands, not invented
here:

| section | required | source |
|---|---|---|
| `job-exec` | `container`, `command` | `RunExec(ctx, j.Container, …)` |
| `job-run` | `image` **or** `container` | `core.RunJob.Validate` →
`ErrImageOrContainer` |
| `job-service-run` | `image` | `core.RunServiceJob.Validate` →
`ErrImageRequired` |
| `job-local` | `command` | `args.GetArgs(j.Command)` |
| `job-compose` | `file`, `service` | the `docker compose -f … run` it
builds |
| all | `schedule` | nothing to register the job under otherwise |

Errors name the section and the job the user wrote:

```
config validation error for field 'job-exec "foo": container':
  is required (the container to exec in)
```

## Impact measured, not assumed

Every config in the repo still validates clean: `example/ofelia.ini`,
`test/test-config.ini`, `test/run-job/ofelia.ini`. There is an e2e test
pinning the shipped example specifically, so adding job checks cannot
start rejecting the file new users are handed.

**One existing fixture had to change rather than the rule.**
`TestValidateExecuteValidFile` declared a `job-exec` with no `container`
and asserted the config was valid. It is not — verified against the
running daemon, that job fails on *every tick*:

```
error: job run: exec run: run_exec container "": invalid container name or ID: value is empty
```

The test was pinning a config that cannot work. This is precisely the
class of failure the change exists to catch, so the fixture gained a
container.

## Note on the constants

`command`, `image`, `container` and `schedule` are now constants. They
were literals in the format switch, which carried a comment arguing that
extracting them only relocates duplication — true while they appeared
once. The requirements table makes them appear in two independent lists
with nothing tying them together, which is what goconst was pointing at.

## Test plan

- [x] `go test ./...` — green
- [x] `go test -race -tags=e2e ./e2e/...` — green
- [x] `golangci-lint run` incl. `--build-tags="e2e unix"` — 0 issues
- [x] `lefthook run pre-push` — exit 0
- [x] All three repo configs validate clean, before and after
- [x] Each requirement verified to fire, and each valid form verified to
pass (including `job-run` with only a container)

Author: CybotTM

cli/validate_test.go

@@ -20,9 +20,14 @@ func TestValidateExecuteValidFile(t *testing.T) {
 	// Not parallel: modifies global os.Stdout which races with other tests.
 
 	configFile := filepath.Join(t.TempDir(), "config.ini")
+	// container is part of a runnable job-exec: without it every run fails
+	// with `run_exec container "": invalid container name or ID`. The fixture
+	// omitted it and this test asserted the config was valid, which is what
+	// job validation now catches.
 	content := `
 [job-exec "foo"]
 schedule = @every 10s
+container = some-container
 command = echo "foo"
 `
 	err := os.WriteFile(configFile, []byte(content), 0o644)

config/validator.go

@@ -208,6 +208,13 @@ func (cv *Validator2) Validate() error {
 
 // validateStruct recursively validates struct fields based on tags
 func (cv *Validator2) validateStruct(v *Validator, obj any, path string) {
+	cv.validateStructIn(v, obj, path, false)
+}
+
+// validateStructIn is validateStruct with the job flag, which travels down
+// through nested and squashed structs so an embedded job field is still
+// recognized as being inside a job.
+func (cv *Validator2) validateStructIn(v *Validator, obj any, path string, inJob bool) {
 	val, ok := derefToStruct(obj)
 	if !ok {
 		return
@@ -226,14 +233,35 @@ func (cv *Validator2) validateStruct(v *Validator, obj any, path string) {
 		fieldPath := resolveFieldPath(path, fieldType.Name, fieldType.Tag.Get("gcfg"), mapstructureTag)
 
 		// Handle nested structs
-		if field.Kind() == reflect.Struct && mapstructureTag != ",squash" {
-			cv.validateStruct(v, field.Interface(), fieldPath)
+		if field.Kind() == reflect.Struct {
+			squashed := mapstructureTag == ",squash"
+			if !squashed {
+				cv.validateStructIn(v, field.Interface(), fieldPath, inJob)
+				continue
+			}
+			// A squashed struct contributes its fields at the parent's level,
+			// so it is walked with the parent's path. Only inside a job:
+			// every job type embeds core.<Kind>Job and, through it, BareJob,
+			// which is where schedule and command live — skipping squashed
+			// structs is why a job's schedule was never checked. Outside a job
+			// the old behavior stands, so the global section is untouched by
+			// this change.
+			if inJob {
+				cv.validateStructIn(v, field.Interface(), path, inJob)
+			}
 			continue
 		}
 
-		// Validate based on field type and value. The enclosing struct travels
-		// with it so a field can be required conditionally on a sibling.
-		cv.validateField(v, val, field, fieldPath, defaultTag)
+		// Job sections are maps keyed by the name the user wrote; walk them so
+		// a job's fields are validated at all. Skipping maps meant no job was
+		// ever reachable by the validator.
+		if field.Kind() == reflect.Map {
+			cv.validateJobMap(v, field, fieldPath)
+			continue
+		}
+
+		// Validate based on field type and value
+		cv.validateField(v, field, fieldCtx{parent: val, path: fieldPath, defaultTag: defaultTag, inJob: inJob})
 	}
 }
 
@@ -274,17 +302,29 @@ func resolveFieldPath(parentPath, fieldName, gcfgTag, mapstructureTag string) st
 	return fieldPath
 }
 
+// fieldCtx carries what a field needs beyond its own value: the struct it
+// belongs to (so a requirement can be conditional on a sibling), its config
+// key, its default tag, and whether it sits inside a job section.
+type fieldCtx struct {
+	parent     reflect.Value
+	path       string
+	defaultTag string
+	// inJob suppresses the "a field with no default is required" rule. That
+	// rule is a heuristic tuned to the global section; applied to a job it
+	// would demand nearly every key a job can carry. What a job genuinely
+	// needs is stated in jobRequirements and checked separately.
+	inJob bool
+}
+
 // validateField validates individual fields based on their type and tags
-func (cv *Validator2) validateField(
-	v *Validator, parent, field reflect.Value, path string, defaultTag string,
-) {
+func (cv *Validator2) validateField(v *Validator, field reflect.Value, ctx fieldCtx) {
 	switch field.Kind() {
 	case reflect.String:
-		cv.validateStringField(v, parent, field, path, defaultTag)
+		cv.validateStringField(v, field, ctx)
 	case reflect.Int, reflect.Int64:
-		cv.validateIntField(v, field, path)
+		cv.validateIntField(v, field, ctx.path)
 	case reflect.Slice:
-		cv.validateSliceField(v, field, path)
+		cv.validateSliceField(v, field, ctx.path)
 	case reflect.Invalid, reflect.Bool, reflect.Int8, reflect.Int16, reflect.Int32,
 		reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr,
 		reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128,
@@ -299,50 +339,63 @@ func (cv *Validator2) validateField(
 }
 
 // validateStringField validates string type fields
-func (cv *Validator2) validateStringField(
-	v *Validator, parent, field reflect.Value, path string, defaultTag string,
-) {
+func (cv *Validator2) validateStringField(v *Validator, field reflect.Value, ctx fieldCtx) {
 	str := field.String()
 
 	// Skip validation for fields with defaults when they're empty
-	if defaultTag != "" && str == "" {
+	if ctx.defaultTag != "" && str == "" {
 		return
 	}
 
 	// Check for required fields
-	if defaultTag == "" && str == "" && !cv.isOptionalField(path) && cv.gateIsOpen(parent, path) {
-		v.ValidateRequired(path, str)
+	if ctx.defaultTag == "" && str == "" && !ctx.inJob &&
+		!cv.isOptionalField(ctx.path) && cv.gateIsOpen(ctx.parent, ctx.path) {
+		v.ValidateRequired(ctx.path, str)
 	}
 
 	// Validate specific string fields
 	if str != "" {
-		cv.validateSpecificStringField(v, path, str)
+		cv.validateSpecificStringField(v, ctx.path, str)
 	}
 }
 
+// fieldKey returns the config key a path ends in, dropping any section
+// qualifier the path carries for reporting.
+func fieldKey(path string) string {
+	if i := strings.LastIndex(path, "."); i >= 0 {
+		return strings.ToLower(path[i+1:])
+	}
+	return strings.ToLower(path)
+}
+
 // validateSpecificStringField validates specific string field formats
 func (cv *Validator2) validateSpecificStringField(v *Validator, path string, str string) {
 	// First perform general security validation
 	if !cv.performSecurityValidation(v, path, str) {
 		return // Stop validation if security check fails
 	}
 
-	// Validate based on field type. The case strings are user-facing INI key
-	// names; Go struct tags (gcfg:"…" / mapstructure:"…") on Config require
-	// them as literals there too, so extracting constants only relocates the
-	// duplication. Suppressing keeps the switch readable.
-	switch path {
-	case "schedule", "cron": //nolint:goconst // see comment above switch
+	// Match on the last segment of the path. Inside a job the path is
+	// qualified with its section ("job-local \"backup\".schedule") so the error
+	// says which job is at fault, while the key that selects the check is the
+	// bare field name. Without this the schedule of a job was never checked:
+	// the qualified path matched no case and fell through silently.
+	//
+	// The case strings are user-facing INI key names; Go struct tags
+	// (gcfg:"…" / mapstructure:"…") on Config require them as literals there
+	// too, so extracting constants only relocates the duplication.
+	switch fieldKey(path) {
+	case keySchedule, "cron":
 		cv.validateCronField(v, path, str)
 	case "email-to", "email-from": //nolint:goconst // see comment above switch
 		cv.validateEmailField(v, path, str)
 	case "web-address", "pprof-address":
 		cv.validateAddressField(v, path, str)
 	case "log-level": //nolint:goconst // see comment above switch
 		cv.validateLogLevelField(v, path, str)
-	case "command", "cmd":
+	case keyCommand, "cmd":
 		cv.validateCommandField(v, path, str)
-	case "image":
+	case keyImage:
 		cv.validateImageField(v, path, str)
 	case "save-folder", "working_dir":
 		cv.validatePathField(v, path, str)
@@ -572,3 +625,141 @@ func (cv *Validator2) isValidLogLevel(level string) bool {
 	level = strings.ToLower(level)
 	return slices.Contains(validLevels, level)
 }
+
+// INI keys named in more than one place here: the switch that picks a format
+// check, and the table of what each job kind needs. They were literals in both
+// until the table arrived, at which point the same key existed twice with
+// nothing tying the two together.
+const (
+	keySchedule  = "schedule"
+	keyCommand   = "command"
+	keyImage     = "image"
+	keyContainer = "container"
+)
+
+// jobRequirement is one thing a job of a given kind must carry. Several field
+// names mean "at least one of these", which is how job-run accepts either an
+// image to start or an existing container to reuse.
+type jobRequirement struct {
+	fields []string
+	// why is appended to the error so the message says what the field is for
+	// rather than only that it is missing.
+	why string
+}
+
+// jobRequirements states what each job section needs in order to run.
+//
+// The entries are taken from what the runtime already demands, not invented
+// here: core.RunJob.Validate returns ErrImageOrContainer, core.RunServiceJob
+// .Validate returns ErrImageRequired, ExecJob execs by container id and
+// command, LocalJob runs a command, and ComposeJob shells out with a file and
+// a service. Checking them here moves those failures from "the job silently
+// never runs" to "validate says so".
+var jobRequirements = map[string][]jobRequirement{
+	"job-exec": {
+		{fields: []string{keyContainer}, why: "the container to exec in"},
+		{fields: []string{keyCommand}, why: "the command to run"},
+	},
+	"job-run": {
+		{fields: []string{keyImage, keyContainer}, why: "an image to start or an existing container to reuse"},
+	},
+	"job-local": {
+		{fields: []string{keyCommand}, why: "the command to run"},
+	},
+	"job-service-run": {
+		{fields: []string{keyImage}, why: "the image to create the swarm service from"},
+	},
+	"job-compose": {
+		{fields: []string{"file"}, why: "the compose file"},
+		{fields: []string{"service"}, why: "the service to run"},
+	},
+}
+
+// scheduleRequirement applies to every job kind: without a schedule there is
+// nothing to register the job under.
+var scheduleRequirement = jobRequirement{fields: []string{keySchedule}, why: "when to run"}
+
+// validateJobMap walks the jobs of one section. Each entry is validated like
+// any other struct — so formats are checked — and then against the
+// requirements for its kind.
+func (cv *Validator2) validateJobMap(v *Validator, m reflect.Value, section string) {
+	if m.Kind() != reflect.Map || m.IsNil() {
+		return
+	}
+
+	reqs, known := jobRequirements[section]
+	for _, key := range m.MapKeys() {
+		entry := m.MapIndex(key)
+		val, ok := derefToStruct(entry.Interface())
+		if !ok {
+			continue
+		}
+
+		// Field-level checks (formats, ranges) for everything the job carries.
+		// The path carries the section and the job name so an error names the
+		// job the user wrote rather than a bare key.
+		cv.validateStructIn(v, entry.Interface(), fmt.Sprintf("%s %q", section, key.String()), true)
+
+		if !known {
+			continue
+		}
+		jobName := key.String()
+		for _, req := range append([]jobRequirement{scheduleRequirement}, reqs...) {
+			cv.checkJobRequirement(v, val, section, jobName, req)
+		}
+	}
+}
+
+// checkJobRequirement reports a requirement that no field satisfies.
+func (cv *Validator2) checkJobRequirement(
+	v *Validator, job reflect.Value, section, jobName string, req jobRequirement,
+) {
+	for _, name := range req.fields {
+		if strings.TrimSpace(fieldValueByKey(job, name)) != "" {
+			return
+		}
+	}
+
+	v.AddError(
+		fmt.Sprintf("%s %q: %s", section, jobName, strings.Join(req.fields, " or ")),
+		"",
+		fmt.Sprintf("is required (%s)", req.why),
+	)
+}
+
+// fieldValueByKey returns the string value of the field carrying the given
+// config key, searching embedded structs because a job's schedule and command
+// live on the BareJob it embeds rather than on the job type itself.
+func fieldValueByKey(val reflect.Value, key string) string {
+	if val.Kind() != reflect.Struct {
+		return ""
+	}
+
+	typ := val.Type()
+	for fieldType := range typ.Fields() {
+		field := val.FieldByIndex(fieldType.Index)
+
+		// Embedded structs are descended into even when the embedded type is
+		// unexported: the job types embed exported ones today, but the values
+		// are only read here, never handed out, so there is no reason for the
+		// lookup to depend on that staying true.
+		if fieldType.Anonymous && field.Kind() == reflect.Struct {
+			if found := fieldValueByKey(field, key); found != "" {
+				return found
+			}
+			continue
+		}
+
+		if !fieldType.IsExported() {
+			continue
+		}
+		if resolveFieldPath("", fieldType.Name, fieldType.Tag.Get("gcfg"), fieldType.Tag.Get("mapstructure")) != key &&
+			!strings.EqualFold(fieldType.Name, key) {
+			continue
+		}
+		if field.Kind() == reflect.String {
+			return field.String()
+		}
+	}
+	return ""
+}

config/validator_boundary_test.go

@@ -432,7 +432,7 @@ func TestValidator2ValidateStringFieldDefaults(t *testing.T) {
 			v := NewValidator()
 
 			field := reflect.ValueOf(tt.value)
-			cv.validateStringField(v, reflect.Value{}, field, tt.path, tt.defaultTag)
+			cv.validateStringField(v, field, fieldCtx{path: tt.path, defaultTag: tt.defaultTag})
 
 			if v.HasErrors() != tt.wantError {
 				t.Errorf("validateStringField(%q, %q, %q) hasError = %v, want %v",