From 58295cb13adb8863a59c7db97ab43f8e77ca6f0b Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sun, 17 May 2026 08:10:44 +0200 Subject: [PATCH] =?UTF-8?q?feat(variant):=20Phase=202=20=E2=80=94=20thread?= =?UTF-8?q?=20fields=5Fper=5Fvariant=20through=20validate=20(#287)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #287. v0.10.0 (PR #285) shipped `Artifact::fields_per_variant` and the `fields_for_variant(Option<&str>) -> Cow<'_, BTreeMap<...>>` resolver as a building block but nothing consumed variant overlays during validate. This PR wires the resolver through the validation engine. **rivet-core/src/schema.rs** — variant-aware helpers (additive): - `get_field_value_for_variant(artifact, field, variant)` resolves through `Artifact::fields_for_variant` when `variant` is `Some(_)`, delegates to the existing borrowed-Cow path when `None` (zero allocations on the no-variant path). - `Condition::matches_artifact_for_variant_with(...)`. - `Requirement::check_for_variant(...)` for `RequiredFields`. Per design §6 Phase 2 scope ("fields only"), `RequiredLinks` stays variant-flat — links aren't overlayed. **rivet-core/src/validate.rs** — additive wrappers + threading: - New public APIs: `validate_with_variant`, `validate_with_externals_and_variant`, `validate_structural_with_variant`, `validate_structural_with_externals_and_variant`. - Existing `validate*` functions thin-wrap the variant-aware version with `variant: None`. No breaking signature changes. - Required-fields and allowed-values reads now go through `artifact.fields_for_variant(variant)` (resolved once per artifact as `effective_fields`). - Conditional rules (phase 8) use `cond.matches_artifact_for_variant_with` + `rule.then.check_for_variant`. **rivet-cli/src/main.rs cmd_validate** — threads the active variant: - When `--variant ` is set, falls through to the direct path (salsa doesn't yet take variant as a tracked input) and calls `validate_with_externals_and_variant(..., active_variant)`. - Baseline-only / no-variant / `--direct` paths preserved. Tests added (rivet-core/src/validate.rs, mod tests): 1. `conditional_rule_respects_variant_field_overlay` — artifact has `fields.priority=must` and `fields-per-variant.automotive.priority= should`. Conditional rule fires on `priority==must`. Without variant: rule fires. With `Some("automotive")`: doesn't fire. With unknown variant: behaves like no variant (1 diag). 2. `required_field_satisfied_by_variant_overlay` — required field `asil` absent from `fields` but present in `fields-per-variant.automotive.asil=D`. Without variant: 1 required-field error. With `Some("automotive")`: 0 errors. NOT in this PR (deliberately): - `rivet list --variant ` filtering. - `rivet coverage --variant ` scoping. - Variant-aware s-expr validation rules (phase 9 in validate.rs). - Salsa-tracked variant input (direct-path fallback for now). - Cross-product multi-axis variants. - `when:` clause on external-anchor. Implements: REQ-004, REQ-007 Refs: FEAT-001, #287 Co-Authored-By: Claude Opus 4.7 --- rivet-cli/src/main.rs | 18 ++- rivet-core/src/schema.rs | 148 ++++++++++++++++++++++++ rivet-core/src/validate.rs | 224 +++++++++++++++++++++++++++++++++++-- 3 files changed, 381 insertions(+), 9 deletions(-) diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index a4ed02b..702e015 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -4595,8 +4595,22 @@ fn cmd_validate( // `reclassify_externals_diagnostics` as a post-pass to converge on the // same diagnostic set. let is_scoped = baseline_name.is_some() || variant_scope_name.is_some(); - let mut diagnostics = if direct { - validate::validate_with_externals(&store, &schema, &graph, &external_schemas) + // Per-variant field overlays (Phase 2 of #287) need to thread the + // active variant name through validate so required-fields, + // allowed-values, and conditional-rule checks consult + // `Artifact::fields_for_variant`. The salsa path doesn't yet take + // variant as a tracked input, so when a variant is active we fall + // through to the direct path's variant-aware entrypoint. + let active_variant: Option<&str> = variant_scope_name.as_ref().map(|(name, _)| name.as_str()); + let use_direct = direct || active_variant.is_some(); + let mut diagnostics = if use_direct { + validate::validate_with_externals_and_variant( + &store, + &schema, + &graph, + &external_schemas, + active_variant, + ) } else { let all_diags = run_salsa_validation(cli, &config)?; let scoped_diags = if is_scoped { diff --git a/rivet-core/src/schema.rs b/rivet-core/src/schema.rs index 929cb7d..0403716 100644 --- a/rivet-core/src/schema.rs +++ b/rivet-core/src/schema.rs @@ -594,6 +594,46 @@ impl Condition { _ => None, } } + + /// Variant-aware twin of [`matches_artifact_with`]. + /// + /// Resolves the field value through the artifact's per-variant + /// overlay when `variant` is `Some(_)`. When `variant` is `None`, + /// behaves identically to [`matches_artifact_with`]. + #[inline] + pub fn matches_artifact_for_variant_with( + &self, + artifact: &Artifact, + compiled: Option<&Regex>, + variant: Option<&str>, + ) -> bool { + // Fast path: no variant means no overlay; preserve borrowed-Cow path. + if variant.is_none() { + return self.matches_artifact_with(artifact, compiled); + } + match self { + Condition::Equals { field, value } => { + get_field_value_for_variant(artifact, field, variant).is_some_and(|v| v == *value) + } + Condition::Matches { field, pattern } => { + if let Some(re) = compiled { + get_field_value_for_variant(artifact, field, variant) + .is_some_and(|v| re.is_match(&v)) + } else { + // Inline-compile fallback to match the non-variant path + // (callers should normally pre-compile via `compile_regex`). + let Ok(re) = Regex::new(pattern) else { + return false; + }; + get_field_value_for_variant(artifact, field, variant) + .is_some_and(|v| re.is_match(&v)) + } + } + Condition::Exists { field } => { + get_field_value_for_variant(artifact, field, variant).is_some() + } + } + } } /// Get a string value for a field from an artifact, checking base fields first. @@ -667,6 +707,67 @@ fn resolve_dotted_path<'a>(value: &'a serde_yaml::Value, rest: &str) -> Option`) because the +/// merged map produced by `fields_for_variant` is short-lived when a +/// variant is active. When `variant` is `None`, we delegate to the +/// borrowed-Cow fast path. +/// +/// Base fields (`status`, `description`, `title`, `id`, `tags`) are NOT +/// overlay-able — variants only affect entries in the `fields` map, +/// matching the design in `docs/design/variant-aware-properties.md` §5.2. +#[inline] +pub(crate) fn get_field_value_for_variant( + artifact: &Artifact, + field: &str, + variant: Option<&str>, +) -> Option { + // When no variant is active, defer to the borrowed-Cow fast path + // to avoid the merge clone. + if variant.is_none() { + return get_field_value(artifact, field).map(|c| c.into_owned()); + } + + // Dotted paths (e.g. `provenance.created-by`) read from the fields + // map only — and we want them variant-aware too. + if let Some(dot_pos) = field.find('.') { + let root = &field[..dot_pos]; + let rest = &field[dot_pos + 1..]; + let merged = artifact.fields_for_variant(variant); + let root_val = merged.get(root)?; + return resolve_dotted_path(root_val, rest).map(Cow::into_owned); + } + + match field { + "status" => artifact.status.clone(), + "description" => artifact.description.clone(), + "title" => Some(artifact.title.clone()), + "id" => Some(artifact.id.clone()), + _ => { + if field == "tags" { + if artifact.tags.is_empty() { + None + } else { + Some(artifact.tags.join(",")) + } + } else { + let merged = artifact.fields_for_variant(variant); + merged + .get(field) + .and_then(yaml_value_to_cow) + .map(Cow::into_owned) + } + } + } +} + /// A requirement that must be met when a condition holds. /// /// YAML examples: @@ -758,6 +859,53 @@ impl Requirement { } diags } + + /// Variant-aware twin of [`check`]. `RequiredFields` reads through + /// the per-variant overlay so a field declared only on the variant + /// satisfies the requirement. + /// + /// `RequiredLinks` is not variant-aware (Phase 2 deliberately keeps + /// links variant-flat — see `docs/design/variant-aware-properties.md` + /// §6 Phase 2 scope: "fields only"). + pub fn check_for_variant( + &self, + artifact: &Artifact, + rule_name: &str, + severity: Severity, + variant: Option<&str>, + ) -> Vec { + if variant.is_none() { + return self.check(artifact, rule_name, severity); + } + match self { + Requirement::RequiredFields { fields } => { + let mut diags = Vec::new(); + for field_name in fields { + let has_field = + get_field_value_for_variant(artifact, field_name, variant).is_some(); + if !has_field { + diags.push(crate::validate::Diagnostic { + source_file: None, + line: None, + column: None, + severity, + artifact_id: Some(artifact.id.clone()), + rule: rule_name.to_string(), + message: format!( + "conditional rule '{}': field '{}' is required when condition is met", + rule_name, field_name + ), + }); + } + } + diags + } + Requirement::RequiredLinks { .. } => { + // Links remain variant-agnostic in Phase 2. + self.check(artifact, rule_name, severity) + } + } + } } // ── Conditional rule consistency checks ──────────────────────────────── diff --git a/rivet-core/src/validate.rs b/rivet-core/src/validate.rs index 6d7cf82..573b4ad 100644 --- a/rivet-core/src/validate.rs +++ b/rivet-core/src/validate.rs @@ -306,6 +306,23 @@ pub fn validate(store: &Store, schema: &Schema, graph: &LinkGraph) -> Vec, +) -> Vec { + validate_with_externals_and_variant(store, schema, graph, &BTreeMap::new(), variant) +} + /// Validate a store against a schema, link graph, and per-prefix external /// schemas. Externally-prefixed artifacts (id like `:`) are /// type-checked against `externals[prefix]` when present, so cross-repo @@ -317,29 +334,56 @@ pub fn validate_with_externals( graph: &LinkGraph, externals: &ExternalSchemas, ) -> Vec { - let mut diagnostics = validate_structural_with_externals(store, schema, graph, externals); + validate_with_externals_and_variant(store, schema, graph, externals, None) +} + +/// Variant-aware twin of [`validate_with_externals`]. When `variant` is +/// `Some(name)`, per-artifact field reads inside the structural and +/// conditional-rule checks resolve through `fields_for_variant`. +pub fn validate_with_externals_and_variant( + store: &Store, + schema: &Schema, + graph: &LinkGraph, + externals: &ExternalSchemas, + variant: Option<&str>, +) -> Vec { + let mut diagnostics = + validate_structural_with_externals_and_variant(store, schema, graph, externals, variant); // 0. Check conditional rule consistency (schema-level) diagnostics.extend(crate::schema::check_conditional_consistency( &schema.conditional_rules, )); - // 8. Check conditional rules (pre-compile regexes to avoid re-compilation per artifact) + // 8. Check conditional rules (pre-compile regexes to avoid re-compilation per artifact). + // + // When a variant is active, `matches_artifact_for_variant_with` and + // `check_for_variant` resolve field reads through the artifact's + // per-variant overlay so a rule whose `when` condition is satisfied + // by the default `priority: must` may NOT fire under a variant that + // overrides `priority: should` (and vice versa). The `variant: None` + // path delegates to the borrowed-Cow fast path. for rule in &schema.conditional_rules { let compiled_re = rule.when.compile_regex(); let condition_re = rule.condition.as_ref().and_then(|c| c.compile_regex()); for artifact in store.iter() { // If a precondition is set, it must also match if let Some(cond) = &rule.condition { - if !cond.matches_artifact_with(artifact, condition_re.as_ref()) { + if !cond.matches_artifact_for_variant_with(artifact, condition_re.as_ref(), variant) + { continue; } } if rule .when - .matches_artifact_with(artifact, compiled_re.as_ref()) + .matches_artifact_for_variant_with(artifact, compiled_re.as_ref(), variant) { - diagnostics.extend(rule.then.check(artifact, &rule.name, rule.severity)); + diagnostics.extend(rule.then.check_for_variant( + artifact, + &rule.name, + rule.severity, + variant, + )); } } } @@ -466,6 +510,21 @@ pub fn validate_structural(store: &Store, schema: &Schema, graph: &LinkGraph) -> validate_structural_with_externals(store, schema, graph, &BTreeMap::new()) } +/// Variant-aware twin of [`validate_structural`]. +/// +/// Required-field and allowed-values checks resolve through the +/// per-variant overlay so a field that only exists on the variant +/// satisfies a `required: true` field definition, and a variant-only +/// value is checked against `allowed-values`. +pub fn validate_structural_with_variant( + store: &Store, + schema: &Schema, + graph: &LinkGraph, + variant: Option<&str>, +) -> Vec { + validate_structural_with_externals_and_variant(store, schema, graph, &BTreeMap::new(), variant) +} + /// Structural validation aware of per-prefix external schemas. /// /// Behaves like [`validate_structural`] but consults `externals[prefix]` when @@ -480,6 +539,20 @@ pub fn validate_structural_with_externals( schema: &Schema, graph: &LinkGraph, externals: &ExternalSchemas, +) -> Vec { + validate_structural_with_externals_and_variant(store, schema, graph, externals, None) +} + +/// Variant-aware twin of [`validate_structural_with_externals`]. When +/// `variant` is `Some(name)`, required-field presence and allowed-value +/// checks read through [`crate::model::Artifact::fields_for_variant`] +/// so the per-variant overlay wins over the default `fields` map. +pub fn validate_structural_with_externals_and_variant( + store: &Store, + schema: &Schema, + graph: &LinkGraph, + externals: &ExternalSchemas, + variant: Option<&str>, ) -> Vec { let mut diagnostics = Vec::new(); @@ -518,9 +591,14 @@ pub fn validate_structural_with_externals( } }; + // Resolve the fields map once per artifact through the variant + // overlay. `Cow::Borrowed(&artifact.fields)` when no variant + // applies — zero allocations on the common path. + let effective_fields = artifact.fields_for_variant(variant); + // 2. Check required fields for field in &type_def.fields { - if field.required && !artifact.fields.contains_key(&field.name) { + if field.required && !effective_fields.contains_key(&field.name) { // Also check if the field name matches a base field (description, etc.) let has_base = match field.name.as_str() { "description" => artifact.description.is_some(), @@ -545,7 +623,7 @@ pub fn validate_structural_with_externals( // 3. Check allowed values if let Some(allowed) = &field.allowed_values { - if let Some(value) = artifact.fields.get(&field.name) { + if let Some(value) = effective_fields.get(&field.name) { if let Some(s) = value.as_str() { // Value is already a YAML string — straightforward check if !allowed.iter().any(|a| a == s) { @@ -3442,4 +3520,136 @@ then: phase-9 validation rules" ); } + + // ── Variant-aware field overlays (issue #287, Phase 2) ─────────────── + // + // Phase 2 wires `Artifact::fields_for_variant` into the validate + // engine: required-fields, allowed-values, and conditional rules + // resolve through the per-variant overlay so an active variant's + // value wins over the default `fields` map. The two tests below + // verify the visible-from-CLI behaviour change. + + /// Test A: conditional rule fires for the default flavour but is + /// suppressed under a variant whose overlay disables the trigger. + /// + /// Setup: + /// - Artifact `R-1` has `fields.priority = must` and + /// `fields-per-variant.automotive.priority = should`. + /// - Conditional rule: "when priority equals 'must', require + /// `rationale` field". + /// + /// Expected: + /// - `validate(store, schema, graph)` (no variant): rule fires + /// because the default flavour has `priority: must`. + /// - `validate_with_variant(.., Some("automotive"))`: rule does + /// NOT fire because the variant overlay sets `priority: should`. + // rivet: verifies REQ-004 + #[test] + fn conditional_rule_respects_variant_field_overlay() { + let rule = ConditionalRule { + name: "must-needs-rationale".to_string(), + description: None, + condition: None, + when: Condition::Equals { + field: "priority".to_string(), + value: "must".to_string(), + }, + then: Requirement::RequiredFields { + fields: vec!["rationale".to_string()], + }, + severity: Severity::Error, + }; + let schema = make_schema(vec![rule]); + + let mut art = make_artifact("R-1", "test", None, None, vec![], vec![]); + art.fields.insert( + "priority".to_string(), + serde_yaml::Value::String("must".to_string()), + ); + let mut overlay = BTreeMap::new(); + overlay.insert( + "priority".to_string(), + serde_yaml::Value::String("should".to_string()), + ); + art.fields_per_variant + .insert("automotive".to_string(), overlay); + + let mut store = Store::new(); + store.insert(art).unwrap(); + let graph = LinkGraph::build(&store, &schema); + + // No variant: condition matches default `priority: must` → rule fires. + let diags_default = validate(&store, &schema, &graph); + assert_eq!( + rule_count(&diags_default, "must-needs-rationale"), + 1, + "default flavour: priority=must matches, rationale missing → rule must fire", + ); + + // Variant active: overlay replaces `priority` with `should` → rule does NOT fire. + let diags_variant = validate_with_variant(&store, &schema, &graph, Some("automotive")); + assert_eq!( + rule_count(&diags_variant, "must-needs-rationale"), + 0, + "variant 'automotive' overlays priority=should so the rule's 'when' \ + precondition fails — the rule must not fire", + ); + + // Unknown variant falls back to defaults (overlay miss → borrowed Cow). + let diags_unknown = validate_with_variant(&store, &schema, &graph, Some("does-not-exist")); + assert_eq!( + rule_count(&diags_unknown, "must-needs-rationale"), + 1, + "unknown variant name must behave like the default flavour", + ); + } + + /// Test B: a required field is missing on the default flavour but + /// satisfied by a variant overlay. + /// + /// Setup: + /// - Schema declares `asil` as a required field on the "test" + /// type. + /// - Artifact has no `fields.asil` but has + /// `fields-per-variant.automotive.asil = D`. + /// + /// Expected: + /// - `validate_structural` (no variant): emits `required-field`. + /// - `validate_structural_with_variant(.., Some("automotive"))`: + /// passes (overlay supplies the field). + // rivet: verifies REQ-004 + #[test] + fn required_field_satisfied_by_variant_overlay() { + let schema = schema_with_fields(vec![required_field("asil")]); + + let mut art = make_artifact("R-2", "test", None, None, vec![], vec![]); + let mut overlay = BTreeMap::new(); + overlay.insert( + "asil".to_string(), + serde_yaml::Value::String("D".to_string()), + ); + art.fields_per_variant + .insert("automotive".to_string(), overlay); + + let mut store = Store::new(); + store.insert(art).unwrap(); + let graph = LinkGraph::build(&store, &schema); + + // Default flavour: no `asil` on the artifact → required-field fires. + let diags_default = validate_structural(&store, &schema, &graph); + assert_eq!( + rule_count(&diags_default, "required-field"), + 1, + "default flavour: asil missing → required-field must fire", + ); + + // Variant active: overlay supplies `asil` → check passes. + let diags_variant = + validate_structural_with_variant(&store, &schema, &graph, Some("automotive")); + assert_eq!( + rule_count(&diags_variant, "required-field"), + 0, + "variant 'automotive' overlay supplies asil=D → required-field must NOT fire", + ); + } }