fix(validator): fail closed on unmatched declared checks (#2129)

Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>

Author: njhensley

docs/contributor/validator.md

@@ -44,6 +44,25 @@ spec:
           value: ">= 450"            # GB/s
 ```
 
+**Declared checks are resolved fail-closed.** Before the cluster is
+prepared or any Job runs, `Validator.preflightDeclaredChecks`
+(`pkg/validator/validator.go`) rejects the run with `ErrCodeInvalidRequest`
+if any declared check name does not resolve to exactly one catalog validator
+in its declared phase — a name matching nothing (typo or a check missing from
+the loaded, possibly `--data`, catalog), a name that exists only under a
+different phase (`nccl-all-reduce-bw` declared under `deployment`), or a name
+declared more than once in one phase's `checks` list. All offenders across
+every requested phase are aggregated into a single error. This closes the
+fail-open path where an all-unmatched phase silently filtered to zero tests →
+`skipped` → nonblocking, letting `aicr validate --fail-on-error` exit `0` on a
+recipe whose required gate the catalog cannot supply
+([#2121](https://github.com/NVIDIA/aicr/issues/2121)). The gate runs in
+`--no-cluster` mode too, so typos are caught in offline recipe validation.
+There is no opt-out: a declared name that resolves nowhere is always an
+authoring error. A check that is *legitimately* not applicable at runtime
+reports its own `skip` sentinel from inside the container — it is still
+declared and still resolves to a catalog entry.
+
 Top-level `constraints` — and any declared under
 `validation.readiness.constraints` — are evaluated as a **pre-flight
 gate** before phase checks run; other phases' `constraints` are

docs/user/cli-reference.md

@@ -1003,6 +1003,8 @@ Validation can be run in different phases to validate different aspects of the d
 
 > **Note:** Readiness constraints (K8s version, OS, kernel) are always evaluated implicitly before any phase runs. If readiness fails, validation stops before deploying any Jobs and exits 2 (`INVALID_REQUEST`). This gate always fails closed — `--fail-on-error=false` scopes to phase check results and does not downgrade a readiness failure.
 >
+> **Declared-check pre-flight:** Every check named under a phase's `checks` list must resolve to exactly one catalog validator in that phase. Before any Job is deployed, `validate` fails closed with exit 2 (`INVALID_REQUEST`) if a declared check matches no validator (a typo, or a check missing from the loaded `--data` catalog), exists only under a different phase, or is declared more than once — reporting every offender at once. This runs in `--no-cluster` mode too, and like readiness it is independent of `--fail-on-error`. It replaces the previous warn-and-continue behavior, which let a phase with only unresolved checks report `skipped` and exit `0`.
+>
 > **Version skew:** Snapshots and recipes record the `aicr` version that produced them. When the recipe, the snapshot, and the running binary report different release versions, `validate` logs a single advisory warning (`version skew detected across validate inputs`) naming all three. This is a debugging breadcrumb — mixing artifacts from different versions can surface as confusing failures — and does **not** fail the command. Dev (`dev`) and pre-release (`-next`) builds are ignored to avoid noise.
 >
 > **apiVersion gate:** Snapshots and catalog artifacts use `aicr.run/v1alpha2`;
@@ -1301,7 +1303,7 @@ Results are output in CTRF (Common Test Report Format) — an industry-standard
 | Code | Description |
 |------|-------------|
 | `0` | All phases passed or were skipped (also returned under `--fail-on-error=false` even when phases report `failed`/`other`) |
-| `2` | Invalid input (bad flags, missing recipe), or a readiness pre-flight constraint not met — the readiness gate always fails closed here regardless of `--fail-on-error` |
+| `2` | Invalid input (bad flags, missing recipe), a readiness pre-flight constraint not met, or a declared check that does not resolve to exactly one catalog validator in its phase (unmatched, cross-phase, or duplicate) — these pre-flight gates always fail closed here regardless of `--fail-on-error` |
 | `5` | Timeout (validator section or context deadline exceeded) |
 | `8` | One or more phase checks reported `failed` or `other` (crash/OOM/deadline) — when `--fail-on-error` is set |
 

pkg/validator/v1/catalog.go

@@ -262,3 +262,34 @@ func (c *ValidatorCatalog) UnmatchedChecks(phase Phase, validationInput *Validat
 
 	return unmatched
 }
+
+// DuplicateChecks returns each check name declared more than once in phase's
+// checks list, in order of first appearance and reported once regardless of
+// how many times it repeats. UnmatchedChecks dedups declared names before
+// comparing against the catalog, so it cannot see duplicates; the preflight
+// needs this complement to reject a checks list that names the same gate twice
+// (a copy-paste error that inflates the apparent test count without adding
+// coverage). Pure function — no catalog is consulted. Returns nil when every
+// declared name for the phase is unique.
+func DuplicateChecks(phase Phase, validationInput *ValidationInput) []string {
+	phaseChecks := checksForPhase(phase, validationInput)
+	if len(phaseChecks) == 0 {
+		return nil
+	}
+
+	counts := make(map[string]int, len(phaseChecks))
+	for _, name := range phaseChecks {
+		counts[name]++
+	}
+
+	var dupes []string
+	reported := make(map[string]bool)
+	for _, name := range phaseChecks {
+		if counts[name] > 1 && !reported[name] {
+			reported[name] = true
+			dupes = append(dupes, name)
+		}
+	}
+
+	return dupes
+}

pkg/validator/v1/catalog_test.go

@@ -293,6 +293,76 @@ func TestUnmatchedChecks(t *testing.T) {
 	}
 }
 
+func TestDuplicateChecks(t *testing.T) {
+	tests := []struct {
+		name   string
+		phase  Phase
+		checks []string
+		want   []string
+	}{
+		{
+			name:   "no duplicates",
+			phase:  PhaseDeployment,
+			checks: []string{"operator-health", "expected-resources"},
+			want:   nil,
+		},
+		{
+			name:   "single duplicate reported once",
+			phase:  PhaseDeployment,
+			checks: []string{"operator-health", "operator-health"},
+			want:   []string{"operator-health"},
+		},
+		{
+			name:   "triple occurrence reported once",
+			phase:  PhasePerformance,
+			checks: []string{"nccl", "nccl", "nccl"},
+			want:   []string{"nccl"},
+		},
+		{
+			name:   "multiple distinct duplicates in declaration order",
+			phase:  PhaseConformance,
+			checks: []string{"a", "b", "a", "c", "b"},
+			want:   []string{"a", "b"},
+		},
+		{
+			name:   "empty checks",
+			phase:  PhaseDeployment,
+			checks: nil,
+			want:   nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			vi := &ValidationInput{}
+			switch tt.phase {
+			case PhaseDeployment:
+				vi.Config.Deployment = &ValidationPhase{Checks: tt.checks}
+			case PhasePerformance:
+				vi.Config.Performance = &ValidationPhase{Checks: tt.checks}
+			case PhaseConformance:
+				vi.Config.Conformance = &ValidationPhase{Checks: tt.checks}
+			}
+
+			got := DuplicateChecks(tt.phase, vi)
+			if len(got) != len(tt.want) {
+				t.Fatalf("DuplicateChecks() = %v, want %v", got, tt.want)
+			}
+			for i := range got {
+				if got[i] != tt.want[i] {
+					t.Errorf("DuplicateChecks()[%d] = %q, want %q", i, got[i], tt.want[i])
+				}
+			}
+		})
+	}
+}
+
+func TestDuplicateChecks_NilValidationInput(t *testing.T) {
+	if got := DuplicateChecks(PhaseDeployment, nil); got != nil {
+		t.Errorf("DuplicateChecks(nil) = %v, want nil", got)
+	}
+}
+
 func TestUnmatchedChecks_NilReceiverAndNoChecks(t *testing.T) {
 	var nilCat *ValidatorCatalog
 	if got := nilCat.UnmatchedChecks(PhaseDeployment, &ValidationInput{}); got != nil {

pkg/validator/validator.go

@@ -242,6 +242,14 @@ func (v *Validator) ValidatePhases(
 		return nil, errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to load validator catalog")
 	}
 
+	// Fail closed on unmatched, cross-phase, or duplicate declared checks
+	// before preparing the cluster or running any Job. Runs for the normalized
+	// phase set and in --no-cluster mode too, so an unresolved required gate
+	// cannot masquerade as a skipped (spuriously passing) phase (issue #2121).
+	if err = v.preflightDeclaredChecks(cat, phases, validationInput); err != nil {
+		return nil, err
+	}
+
 	// --no-cluster: report all as skipped, no K8s calls
 	if v.NoCluster {
 		return v.phasesSkipped(cat, phases, "skipped - no-cluster mode"), nil
@@ -336,10 +344,15 @@ func (v *Validator) ValidatePhase(
 		return nil, errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to load validator catalog")
 	}
 
+	// Fail closed on unmatched, cross-phase, or duplicate declared checks
+	// before the no-cluster short-circuit and before any cluster preparation,
+	// matching ValidatePhases: a per-phase caller must not be able to skip an
+	// unresolved required gate into a spuriously passing run (issue #2121).
+	if err = v.preflightDeclaredChecks(cat, []Phase{phase}, validationInput); err != nil {
+		return nil, err
+	}
+
 	if v.NoCluster {
-		// Warn on unmatched check names even in no-cluster mode so typos are
-		// caught during offline recipe validation, not just live runs.
-		warnUnmatchedChecks(cat, phase, validationInput)
 		return v.phaseSkipped(cat, phase, "skipped - no-cluster mode"), nil
 	}
 
@@ -353,21 +366,56 @@ func (v *Validator) ValidatePhase(
 	return v.runPhase(ctx, cs.clientset, cs.factory, cat, phase, validationInput)
 }
 
-// warnUnmatchedChecks emits a structured warning for every declared check name
-// that matched no catalog entry in its phase. A name that exists under a
-// different phase is called out as a likely misplacement; anything else is a
-// probable typo or a check missing from the loaded (possibly --data) catalog.
-// Advisory only — it never fails the run.
-func warnUnmatchedChecks(cat *catalog.ValidatorCatalog, phase Phase, validationInput *v1.ValidationInput) {
-	for _, u := range cat.UnmatchedChecks(phase, validationInput) {
-		if u.OtherPhase != "" {
-			slog.Warn("declared check matches no validator in this phase; it exists under a different phase",
-				"check", u.Name, "phase", u.Phase, "foundInPhase", u.OtherPhase)
-			continue
+// preflightDeclaredChecks fails closed when any declared check name does not
+// resolve to exactly one catalog entry in its declared phase. It aggregates
+// three defects across every requested phase into a single error so mixed
+// valid/invalid lists surface every problem in one pass:
+//
+//   - unmatched: a name matching no validator in the catalog at all (typo, a
+//     check missing from an incomplete external --data catalog, or a missing
+//     embedded validator);
+//   - cross-phase: a name that exists but under a different phase (a
+//     misplacement, e.g. a performance check declared under deployment);
+//   - duplicate: a name declared more than once in one phase's checks list.
+//
+// This runs BEFORE the cluster is prepared or any Job is deployed, in both
+// live and --no-cluster modes. Without it an all-unmatched phase silently
+// filters down to zero tests → StatusSkipped → nonblocking, so
+// `aicr validate --fail-on-error` exits 0 on a recipe that names a required
+// gate the catalog cannot supply (issue #2121). Returns nil when every
+// declared check for every requested phase resolves exactly once.
+func (v *Validator) preflightDeclaredChecks(
+	cat *catalog.ValidatorCatalog,
+	phases []Phase,
+	validationInput *v1.ValidationInput,
+) error {
+
+	var problems []string
+	for _, phase := range phases {
+		for _, u := range cat.UnmatchedChecks(phase, validationInput) {
+			if u.OtherPhase != "" {
+				problems = append(problems, fmt.Sprintf(
+					"declared check %q in phase %s matches no validator in that phase (found under phase: %s)",
+					u.Name, u.Phase, u.OtherPhase))
+				continue
+			}
+			problems = append(problems, fmt.Sprintf(
+				"declared check %q in phase %s matches no validator in the catalog",
+				u.Name, u.Phase))
+		}
+		for _, name := range v1.DuplicateChecks(phase, validationInput) {
+			problems = append(problems, fmt.Sprintf(
+				"declared check %q is declared more than once in phase %s", name, phase))
 		}
-		slog.Warn("declared check matches no validator in the catalog; it will not run",
-			"check", u.Name, "phase", u.Phase)
 	}
+
+	if len(problems) == 0 {
+		return nil
+	}
+
+	return errors.New(errors.ErrCodeInvalidRequest,
+		"validation declares checks that do not match the validator catalog:\n  - "+
+			strings.Join(problems, "\n  - "))
 }
 
 // runPhase executes all validators for a single phase sequentially.
@@ -391,11 +439,10 @@ func (v *Validator) runPhase(
 	slog.Info("running validation phase", "phase", phase,
 		"catalog", len(allEntries), "selected", len(entries))
 
-	// Surface declared check names that matched no catalog entry for this
-	// phase. Silently dropping them lets a typo'd or misplaced check name
-	// (e.g. a performance check declared under deployment) produce an empty,
-	// spuriously-passing phase — the fail-open direction for a gate.
-	warnUnmatchedChecks(cat, phase, validationInput)
+	// Note: unmatched, cross-phase, and duplicate declared checks are rejected
+	// up front by preflightDeclaredChecks (in ValidatePhase/ValidatePhases)
+	// before this phase ever runs, so by here every declared check for the
+	// phase resolves to exactly one catalog entry.
 
 	builder := ctrf.NewBuilder("aicr", v.Version, string(phase))