diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index cb2bc2c..41e12f4 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -304,14 +304,29 @@ enum Command { #[arg(long)] model: Option, - /// Path to variant configuration YAML file - #[arg(long)] - variant: Option, + /// Variant to apply: either a name resolved against + /// `artifacts/variants/.yaml`, or a direct path to a + /// variant configuration YAML file. When passed alone (no + /// `--model`/`--binding`) the variant is applied as a + /// `fields-per-variant:` overlay per issue #287 §5.5 — the + /// merged view is validated **in addition** to the default + /// view. With `--model` + `--binding` the legacy variant-scoped + /// mode runs (only artifacts bound to the variant are kept in + /// scope). + #[arg(long, value_name = "NAME_OR_PATH")] + variant: Option, /// Path to feature-to-artifact binding YAML file #[arg(long)] binding: Option, + /// Promote `variant-key-unknown` warnings (a `fields-per-variant` + /// key that is not in the project's variant-config list or + /// feature-model feature list) to errors. Useful in CI to lock + /// the variant-key namespace. + #[arg(long = "strict-variants")] + strict_variants: bool, + /// Minimum severity that causes exit code 1. Values: "error" (default), /// "warning", "info". E.g. --fail-on warning tightens the gate so any /// warning (or error) fails the run. @@ -385,6 +400,13 @@ enum Command { /// Scope listing to a named baseline (cumulative) #[arg(long)] baseline: Option, + + /// Apply per-variant field overlays (issue #287) — name from + /// `artifacts/variants/.yaml`, or a path to a variant + /// config YAML file. When set, JSON output also emits each + /// artifact's merged `fields:` view for the variant. + #[arg(long, value_name = "NAME_OR_PATH")] + variant: Option, }, /// Show artifact summary statistics @@ -440,6 +462,13 @@ enum Command { #[arg(long)] baseline: Option, + /// Stamp the active variant into the coverage output (issue + /// #287). Today this surfaces the variant name in the report + /// header for traceability; variant-aware delegated-chain + /// scoping is tracked as a follow-up. + #[arg(long, value_name = "NAME_OR_PATH")] + variant: Option, + /// Render the V&V coverage matrix from `repo-status` artifacts /// (rivet#188). Combine with `--format` to choose text, json, /// markdown, or html output. Mutually exclusive with `--tests`. @@ -959,6 +988,14 @@ enum Command { /// Output format: "text" (default), "json", "ids" #[arg(short, long, default_value = "text")] format: String, + + /// Apply per-variant field overlays before matching (issue + /// #287). The s-expression sees the merged view, so a filter + /// like `(= (field "max-temp-c") "100")` resolves under the + /// active variant's overlay. Accepts a name from + /// `artifacts/variants/.yaml` or a direct path. + #[arg(long, value_name = "NAME_OR_PATH")] + variant: Option, }, /// Stamp artifact(s) with AI provenance metadata @@ -1761,6 +1798,7 @@ fn run(cli: Cli) -> Result { model, variant, binding, + strict_variants, fail_on, strict_cited_sources, strict_cited_source_stale, @@ -1775,6 +1813,7 @@ fn run(cli: Cli) -> Result { model.as_deref(), variant.as_deref(), binding.as_deref(), + *strict_variants, fail_on, *strict_cited_sources, *strict_cited_source_stale, @@ -1786,6 +1825,7 @@ fn run(cli: Cli) -> Result { filter, format, baseline, + variant, } => cmd_list( &cli, r#type.as_deref(), @@ -1793,6 +1833,7 @@ fn run(cli: Cli) -> Result { filter.as_deref(), format, baseline.as_deref(), + variant.as_deref(), ), Command::Get { id, format } => cmd_get(&cli, id, format), Command::Bundle { id, depth, format } => cmd_bundle(&cli, id, *depth, format), @@ -1815,6 +1856,7 @@ fn run(cli: Cli) -> Result { tests, scan_paths, baseline, + variant, matrix, aggregate, } => { @@ -1831,6 +1873,7 @@ fn run(cli: Cli) -> Result { format, fail_under.as_ref(), baseline.as_deref(), + variant.as_deref(), ) } } @@ -2156,7 +2199,8 @@ fn run(cli: Cli) -> Result { sexpr, limit, format, - } => cmd_query(&cli, sexpr, *limit, format), + variant, + } => cmd_query(&cli, sexpr, *limit, format, variant.as_deref()), Command::Stamp { id, created_by, @@ -4418,8 +4462,9 @@ fn cmd_validate( baseline_name: Option<&str>, track_convergence: bool, model_path: Option<&std::path::Path>, - variant_path: Option<&std::path::Path>, + variant_arg: Option<&str>, binding_path: Option<&std::path::Path>, + strict_variants: bool, fail_on: &str, strict_cited_sources: bool, strict_cited_source_stale: bool, @@ -4455,17 +4500,29 @@ fn cmd_validate( (store, graph) }; + // Resolve `--variant ` into a PathBuf for the legacy + // model/binding mode, plus a "display name" the overlay validator + // uses as the lookup key into `fields_per_variant`. If neither file + // path nor `artifacts/variants/.yaml` resolves we error. + let resolved_variant: Option<(std::path::PathBuf, String)> = match variant_arg { + Some(arg) => Some(resolve_variant_arg(&cli.project, arg)?), + None => None, + }; + let variant_path = resolved_variant.as_ref().map(|(p, _)| p.as_path()); + // Apply variant scoping. // - // Three modes: - // --model + --variant + --binding : validate the subset bound to the - // variant's effective feature set (variant-scoped). - // --model + --binding (no variant) : validate the feature model + - // binding file pair (parse model, parse binding, check every - // feature name in the binding exists in the model). Does not - // scope the store. - // --variant alone: legacy error — --variant requires --model and - // a binding to resolve against. + // Four modes: + // --model + --variant + --binding : variant-scoped — filter the + // store to artifacts bound by the variant's effective feature + // set, then validate (legacy behaviour). + // --model + --binding (no variant) : validate model/binding + // consistency without resolving a specific variant. + // --variant alone (no model/binding) : Phase 2 overlay mode + // (issue #287). The store is not scoped; instead the variant's + // name becomes the active overlay key applied via + // `validate_variants` on top of the default-view validation. + // nothing : ordinary project validation. let (store, graph, variant_scope_name) = match (model_path, variant_path, binding_path) { (Some(mp), Some(vp), Some(bp)) => { let model_yaml = std::fs::read_to_string(mp) @@ -4546,9 +4603,16 @@ fn cmd_validate( } (store, graph, None) } + (None, Some(_), None) => { + // Variant overlay mode (issue #287, Phase 2). No store scoping; + // the variant only adjusts the per-artifact field overlays at + // the validate_variants step below. + (store, graph, None) + } (None, None, None) => (store, graph, None), _ => anyhow::bail!( - "variant-scoped validation requires --model and --binding; --variant is optional" + "variant-scoped validation requires --model and --binding; \ + pass --variant alone for overlay-only validation" ), }; @@ -4600,6 +4664,50 @@ fn cmd_validate( }; diagnostics.extend(validate::validate_documents(&doc_store, &store)); + // Variant overlay validation (issue #287, Phase 2). Always run — the + // variant-key-unknown check applies whether or not a `--variant` was + // passed (it catches typos in any artifact's `fields-per-variant:`). + // When `--variant ` is in effect AND model/binding are absent + // (i.e. overlay mode), the active overlay is the resolved variant's + // `name:` field. + { + // Build the "known variants" set: declared configs from + // `artifacts/variants/`, plus any inline variants from a binding + // file if --binding was supplied, plus the variant explicitly + // referenced by --variant if it isn't already there (so + // `--variant ./tmp.yaml` outside the project tree still passes + // the variant-key check). Missing/empty dir is fine — yields + // an empty known set, so every fields-per-variant key warns. + let variants_dir = cli.project.join("artifacts").join("variants"); + let mut known: std::collections::BTreeSet = + match rivet_core::feature_model::load_variant_configs_from_dir(&variants_dir) { + Ok(cfgs) => cfgs.into_iter().map(|c| c.name).collect(), + Err(e) => { + eprintln!( + "warning: failed to load variant configs from {}: {e}", + variants_dir.display() + ); + std::collections::BTreeSet::new() + } + }; + // Variant-overlay mode: the variant config the user pointed at + // might live outside `artifacts/variants/`. Add its `name:` so + // the artifact's overlay key for that name doesn't warn. + if let Some((_, ref name)) = resolved_variant { + known.insert(name.clone()); + } + let active: Option<&str> = resolved_variant.as_ref().map(|(_, n)| n.as_str()); + let variant_diags = validate::validate_variants( + &store, + &schema, + &external_schemas, + active, + &known, + strict_variants, + ); + diagnostics.extend(variant_diags); + } + // Cited-source validation (Phase 1: kind: file backend). // // The store iterator yields references; clone artifacts to feed the @@ -4940,6 +5048,65 @@ fn parse_fail_on(value: &str) -> Result { } } +/// Resolve a `--variant ` CLI argument (issue #287). +/// +/// Returns `(path, name)` where: +/// - `path` is the variant config YAML file on disk, and +/// - `name` is the `name:` key inside that file (used by callers as +/// the overlay lookup key into `Artifact::fields_per_variant`). +/// +/// Resolution order: +/// 1. If `arg` is an existing file path, parse it directly. +/// 2. Otherwise treat `arg` as a bare name and look up +/// `/artifacts/variants/.yaml`. +/// 3. Otherwise error with the list of available names so the user +/// sees what they should have typed. +fn resolve_variant_arg( + project: &std::path::Path, + arg: &str, +) -> Result<(std::path::PathBuf, String)> { + // 1. Direct file path? + let direct = std::path::PathBuf::from(arg); + if direct.is_file() { + let yaml = std::fs::read_to_string(&direct) + .with_context(|| format!("reading {}", direct.display()))?; + let vc: rivet_core::feature_model::VariantConfig = serde_yaml::from_str(&yaml) + .with_context(|| format!("parsing variant config {}", direct.display()))?; + return Ok((direct, vc.name)); + } + + // 2. Bare name → artifacts/variants/.yaml + let variants_dir = project.join("artifacts").join("variants"); + let candidate = variants_dir.join(format!("{arg}.yaml")); + if candidate.is_file() { + let yaml = std::fs::read_to_string(&candidate) + .with_context(|| format!("reading {}", candidate.display()))?; + let vc: rivet_core::feature_model::VariantConfig = serde_yaml::from_str(&yaml) + .with_context(|| format!("parsing variant config {}", candidate.display()))?; + // Sanity check: the file's `name:` should equal the lookup + // name. If it doesn't, prefer the on-disk name (it's the + // identity authors agreed on in `artifacts/variants/`). + return Ok((candidate, vc.name)); + } + + // 3. Build a helpful error listing what *is* available. + let known = + rivet_core::feature_model::load_variant_configs_from_dir(&variants_dir).unwrap_or_default(); + let names: Vec = known.into_iter().map(|v| v.name).collect(); + anyhow::bail!( + "variant '{arg}' not found.\n Tried: {} (not a file) and {} (does not exist).\n \ + Available variants in {}: {}", + direct.display(), + candidate.display(), + variants_dir.display(), + if names.is_empty() { + "(none — directory missing or empty)".to_string() + } else { + names.join(", ") + } + ) +} + /// Run core validation via the salsa incremental database. /// /// This reads all source files and schemas into salsa inputs, then calls the @@ -5187,11 +5354,18 @@ fn cmd_list( sexpr_filter: Option<&str>, format: &str, baseline_name: Option<&str>, + variant_arg: Option<&str>, ) -> Result { validate_format(format, &["text", "json"])?; let ctx = ProjectContext::load(cli)?; let store = apply_baseline_scope(ctx.store, baseline_name, &ctx.config); + // Resolve `--variant` once for display + per-artifact field merge. + let variant_name: Option = match variant_arg { + Some(arg) => Some(resolve_variant_arg(&cli.project, arg)?.1), + None => None, + }; + let query = rivet_core::query::Query { artifact_type: type_filter.map(|s| s.to_string()), status: status_filter.map(|s| s.to_string()), @@ -5216,22 +5390,43 @@ fn cmd_list( let artifacts_json: Vec = results .iter() .map(|a| { - serde_json::json!({ + let mut entry = serde_json::json!({ "id": a.id, "type": a.artifact_type, "title": a.title, "status": a.status.as_deref().unwrap_or("-"), "links": a.links.len(), - }) + }); + if let Some(ref name) = variant_name { + // Emit the merged fields view so downstream tooling + // can see the variant-resolved values without a + // second call (issue #287 §5.5). + let merged = a.fields_for_variant(Some(name)); + if let Ok(v) = serde_json::to_value(&*merged) { + if let serde_json::Value::Object(ref mut map) = entry { + map.insert("variant".into(), serde_json::Value::String(name.clone())); + map.insert("fields".into(), v); + } + } + } + entry }) .collect(); - let output = serde_json::json!({ + let mut output = serde_json::json!({ "command": "list", "count": results.len(), "artifacts": artifacts_json, }); + if let Some(ref name) = variant_name { + if let serde_json::Value::Object(ref mut map) = output { + map.insert("variant".into(), serde_json::Value::String(name.clone())); + } + } println!("{}", serde_json::to_string_pretty(&output).unwrap()); } else { + if let Some(ref name) = variant_name { + println!("Variant: {name}\n"); + } for artifact in &results { let status = artifact.status.as_deref().unwrap_or("-"); let links = artifact.links.len(); @@ -5477,6 +5672,7 @@ fn cmd_coverage( format: &str, fail_under: Option<&f64>, baseline_name: Option<&str>, + variant_arg: Option<&str>, ) -> Result { validate_format(format, &["text", "json"])?; let ctx = ProjectContext::load(cli)?; @@ -5488,6 +5684,16 @@ fn cmd_coverage( ctx.graph }; + // Resolve `--variant` once so we can stamp the report header (and, + // in a later phase, scope delegated chains). Today the variant + // does not yet alter coverage counts — the chain-rewriting work is + // a follow-up — but we surface the active variant in both text and + // JSON output for traceability per issue #287 acceptance criterion 4. + let variant_name: Option = match variant_arg { + Some(arg) => Some(resolve_variant_arg(&cli.project, arg)?.1), + None => None, + }; + // Apply s-expression filter if provided. if let Some(filter_src) = sexpr_filter { let expr = rivet_core::sexpr_eval::parse_filter(filter_src).map_err(|errs| { @@ -5541,6 +5747,9 @@ fn cmd_coverage( "percentage": overall_pct, }, }); + if let Some(ref name) = variant_name { + output["variant"] = serde_json::Value::String(name.clone()); + } // Echo the threshold + pass/fail result when --fail-under is in // effect so CI consumers can programmatically distinguish a // clean run from a gated failure without parsing stderr. @@ -5555,6 +5764,9 @@ fn cmd_coverage( } else { let any_boundary = report.entries.iter().any(|e| e.external_boundary > 0); println!("Traceability Coverage Report\n"); + if let Some(ref name) = variant_name { + println!("Variant: {name}\n"); + } if any_boundary { println!( " {:<30} {:<20} {:>8} {:>9} {:>8} {:>8}", @@ -7423,6 +7635,7 @@ fn cmd_diff( model: None, variant: None, binding: None, + strict_variants: false, fail_on: "error".to_string(), strict_cited_sources: false, strict_cited_source_stale: false, @@ -7443,6 +7656,7 @@ fn cmd_diff( model: None, variant: None, binding: None, + strict_variants: false, fail_on: "error".to_string(), strict_cited_sources: false, strict_cited_source_stale: false, @@ -12565,12 +12779,41 @@ fn cmd_mcp(cli: &Cli, list_tools: bool, probe: bool, format: &str) -> Result Result { +fn cmd_query( + cli: &Cli, + sexpr: &str, + limit: usize, + format: &str, + variant_arg: Option<&str>, +) -> Result { validate_format(format, &["text", "json", "ids"])?; - let project = rivet_core::load_project_full(&cli.project) + let mut project = rivet_core::load_project_full(&cli.project) .with_context(|| format!("loading project from {}", cli.project.display()))?; + // Resolve `--variant` and, if active, rewrite each artifact's + // `fields:` to its merged view before the s-expression filter runs. + // The filter language accesses fields via the artifact's `fields:` + // slot — so swapping the map in place is the minimal change that + // makes a filter like `(= (field "max-temp-c") "100")` resolve + // under the active variant's overlay (issue #287 §5.5). + let variant_name: Option = match variant_arg { + Some(arg) => Some(resolve_variant_arg(&cli.project, arg)?.1), + None => None, + }; + if let Some(ref name) = variant_name { + let ids: Vec = project.store.iter().map(|a| a.id.clone()).collect(); + for id in ids { + if let Some(mut art) = project.store.get(&id).cloned() { + if art.fields_per_variant.contains_key(name) { + let merged = art.fields_for_variant(Some(name)).into_owned(); + art.fields = merged; + project.store.upsert(art); + } + } + } + } + let result = rivet_core::query::execute_sexpr(&project.store, &project.graph, sexpr, Some(limit)) .map_err(|e| anyhow::anyhow!("{e}"))?; @@ -12597,13 +12840,16 @@ fn cmd_query(cli: &Cli, sexpr: &str, limit: usize, format: &str) -> Result }) }) .collect(); - let out = serde_json::json!({ + let mut out = serde_json::json!({ "filter": sexpr, "count": artifacts.len(), "total": result.total, "truncated": result.truncated, "artifacts": artifacts, }); + if let Some(ref name) = variant_name { + out["variant"] = serde_json::Value::String(name.clone()); + } println!("{}", serde_json::to_string_pretty(&out)?); } "ids" => { @@ -12613,6 +12859,9 @@ fn cmd_query(cli: &Cli, sexpr: &str, limit: usize, format: &str) -> Result } _ => { // text + if let Some(ref name) = variant_name { + println!("Variant: {name}\n"); + } if result.matches.is_empty() { println!("No artifacts match: {sexpr}"); } else { diff --git a/rivet-cli/tests/variant_phase2.rs b/rivet-cli/tests/variant_phase2.rs new file mode 100644 index 0000000..23418ea --- /dev/null +++ b/rivet-cli/tests/variant_phase2.rs @@ -0,0 +1,404 @@ +// SAFETY-REVIEW (SCRC Phase 1, DD-058): Integration test / bench code. +// Tests legitimately use unwrap/expect/panic/assert-indexing patterns +// because a test failure should panic with a clear stack. Blanket-allow +// the Phase 1 restriction lints at crate scope; real risk analysis for +// these lints is carried by production code in rivet-core/src and +// rivet-cli/src, not by the test harnesses. +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::indexing_slicing, + clippy::arithmetic_side_effects, + clippy::as_conversions, + clippy::cast_possible_truncation, + clippy::cast_sign_loss, + clippy::wildcard_enum_match_arm, + clippy::match_wildcard_for_single_variants, + clippy::panic, + clippy::todo, + clippy::unimplemented, + clippy::dbg_macro, + clippy::print_stdout, + clippy::print_stderr +)] + +//! Integration tests for issue #287 Phase 2: +//! +//! - `artifacts/variants/*.yaml` configs are loaded by the CLI. +//! - `rivet validate --variant ` honours the per-variant overlay. +//! - `rivet list --variant ` emits the merged fields view. +//! - `rivet query --sexpr ... --variant ` filters against the +//! merged view (so `(= X "Y")` resolves under the overlay). +//! - Coverage stamps the active variant name in its output. + +use std::process::Command; + +fn rivet_bin() -> std::path::PathBuf { + if let Ok(bin) = std::env::var("CARGO_BIN_EXE_rivet") { + return std::path::PathBuf::from(bin); + } + let manifest = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let workspace_root = manifest.parent().expect("workspace root"); + workspace_root.join("target").join("debug").join("rivet") +} + +/// Spin up a tiny rivet project with: +/// - schemas/requirement.yaml: a `requirement` type with a string +/// `max-temp-c` field constrained to {"70", "80", "100"}. +/// - rivet.yaml that loads schemas/ and artifacts/. +/// - artifacts/reqs.yaml: one requirement with a variant overlay. +/// - artifacts/variants/{auto,industrial}.yaml: two variant configs. +fn setup_phase2_project() -> (tempfile::TempDir, std::path::PathBuf) { + let tmp = tempfile::tempdir().expect("create temp dir"); + let dir = tmp.path().to_path_buf(); + + std::fs::create_dir_all(dir.join("schemas")).unwrap(); + std::fs::create_dir_all(dir.join("artifacts").join("variants")).unwrap(); + + std::fs::write( + dir.join("rivet.yaml"), + r#"project: + name: phase2-fixture + version: "0.1.0" + schemas: + - thermal + +sources: + - path: artifacts + format: generic-yaml +"#, + ) + .unwrap(); + + // Minimal local schema. `--schemas` points the CLI at our dir so we + // do not rely on rivet's embedded schemas (which don't ship + // `max-temp-c`). + std::fs::write( + dir.join("schemas").join("thermal.yaml"), + r#"schema: + name: thermal + version: "0.1.0" + description: Thermal envelope test schema + +artifact-types: + - name: requirement + description: Thermal envelope requirement + fields: + - name: max-temp-c + type: string + required: true + allowed-values: ["70", "80", "100"] +"#, + ) + .unwrap(); + + std::fs::write( + dir.join("artifacts").join("reqs.yaml"), + r#"artifacts: + - id: REQ-THERMAL-01 + type: requirement + title: Operating temperature envelope + fields: + max-temp-c: "80" + fields-per-variant: + industrial: + max-temp-c: "100" + consumer: + # Out-of-set value: should warn under + # `allowed-values-variant.consumer`. + max-temp-c: "999" +"#, + ) + .unwrap(); + + std::fs::write( + dir.join("artifacts") + .join("variants") + .join("industrial.yaml"), + "name: industrial\nselects: []\n", + ) + .unwrap(); + std::fs::write( + dir.join("artifacts") + .join("variants") + .join("automotive.yaml"), + "name: automotive\nselects: []\n", + ) + .unwrap(); + + (tmp, dir) +} + +#[test] +fn validate_variant_overlay_flags_out_of_set_value_in_consumer_variant() { + // `consumer` has an out-of-set value AND is not declared in + // artifacts/variants/, so we expect two diagnostics: + // * allowed-values-variant.consumer (the value '999' violates the + // field's allowed-values), + // * variant-key-unknown (consumer is not a known variant + // name). + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "validate", + "--format", + "json", + "--direct", + ]) + .output() + .expect("rivet validate"); + + let stdout = String::from_utf8_lossy(&out.stdout); + // The CLI's JSON diagnostic schema does not include the internal + // `rule:` name; match on the user-facing message text instead. + assert!( + stdout.contains("(variant: consumer)") && stdout.contains("'999'"), + "stdout should mention the consumer variant overlay's value \ + '999'. stdout: {stdout}" + ); + assert!( + stdout.contains("'consumer'") && stdout.contains("not declared"), + "stdout should mention the unknown-variant-key diagnostic. \ + stdout: {stdout}" + ); +} + +#[test] +fn validate_variant_overlay_passes_for_in_set_industrial_variant() { + // Industrial has a value (100) in the allowed-values set AND the + // variant name is declared. The overlay therefore contributes no + // diagnostics for REQ-THERMAL-01 industrial. + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "validate", + "--variant", + "industrial", + "--format", + "json", + "--direct", + ]) + .output() + .expect("rivet validate"); + + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + !stdout.contains("allowed-values-variant.industrial"), + "industrial overlay must validate cleanly. stdout: {stdout}" + ); +} + +#[test] +fn validate_strict_variants_promotes_unknown_key_to_error() { + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "validate", + "--strict-variants", + "--format", + "json", + "--direct", + ]) + .output() + .expect("rivet validate"); + let stdout = String::from_utf8_lossy(&out.stdout); + // Under --strict-variants the unknown-key diagnostic must be an + // error. The CLI emits `severity: "error"` (lowercase) in JSON; + // pair it with the unique-to-this-diagnostic message snippet to + // avoid false positives from unrelated errors. + let v: serde_json::Value = + serde_json::from_str(&stdout).expect("validate --format json emits JSON"); + let diags = v["diagnostics"].as_array().expect("diagnostics array"); + let strict_err = diags.iter().any(|d| { + d["severity"].as_str() == Some("error") + && d["message"] + .as_str() + .is_some_and(|m| m.contains("fields-per-variant")) + }); + assert!( + strict_err, + "expected unknown-variant-key at error severity under \ + --strict-variants. diagnostics: {diags:?}" + ); +} + +#[test] +fn validate_unknown_variant_name_errors_with_list_of_known_names() { + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "validate", + "--variant", + "no-such-variant", + "--direct", + ]) + .output() + .expect("rivet validate"); + assert!(!out.status.success()); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("no-such-variant"), + "stderr should name the bad argument. stderr: {stderr}" + ); + assert!( + stderr.contains("industrial") && stderr.contains("automotive"), + "stderr should list available variants. stderr: {stderr}" + ); +} + +#[test] +fn list_variant_emits_merged_fields_in_json() { + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "list", + "--variant", + "industrial", + "--format", + "json", + ]) + .output() + .expect("rivet list"); + assert!( + out.status.success(), + "rivet list must succeed. stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["variant"], "industrial"); + let arts = v["artifacts"].as_array().expect("artifacts array"); + let req = arts + .iter() + .find(|a| a["id"] == "REQ-THERMAL-01") + .expect("REQ-THERMAL-01 in results"); + assert_eq!( + req["fields"]["max-temp-c"], "100", + "industrial overlay must promote max-temp-c to 100. got {req}" + ); +} + +#[test] +fn query_variant_resolves_overlay_in_sexpr_filter() { + let (_keep, dir) = setup_phase2_project(); + // The default value of max-temp-c is "80"; only with --variant + // industrial does the field resolve to "100". + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "query", + "--sexpr", + r#"(= max-temp-c "100")"#, + "--variant", + "industrial", + "--format", + "ids", + ]) + .output() + .expect("rivet query"); + assert!( + out.status.success(), + "rivet query must succeed. stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("REQ-THERMAL-01"), + "industrial overlay must satisfy the filter. stdout: {stdout}" + ); + + // Sanity: same query without the variant must NOT match (the + // default value is "80"). + let out_no_variant = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "query", + "--sexpr", + r#"(= max-temp-c "100")"#, + "--format", + "ids", + ]) + .output() + .expect("rivet query (no variant)"); + let no_var_stdout = String::from_utf8_lossy(&out_no_variant.stdout); + assert!( + !no_var_stdout.contains("REQ-THERMAL-01"), + "without --variant the default value '80' should not match. \ + stdout: {no_var_stdout}" + ); +} + +#[test] +fn coverage_stamps_variant_name_in_text_header() { + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "coverage", + "--variant", + "industrial", + ]) + .output() + .expect("rivet coverage"); + assert!( + out.status.success(), + "coverage must succeed. stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("Variant: industrial"), + "coverage output must stamp the active variant. stdout: {stdout}" + ); +} + +#[test] +fn coverage_stamps_variant_name_in_json() { + let (_keep, dir) = setup_phase2_project(); + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "--schemas", + dir.join("schemas").to_str().unwrap(), + "coverage", + "--variant", + "industrial", + "--format", + "json", + ]) + .output() + .expect("rivet coverage"); + let stdout = String::from_utf8_lossy(&out.stdout); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["variant"], "industrial"); +} diff --git a/rivet-core/src/feature_model.rs b/rivet-core/src/feature_model.rs index f9a98f5..bb081d5 100644 --- a/rivet-core/src/feature_model.rs +++ b/rivet-core/src/feature_model.rs @@ -1193,6 +1193,60 @@ fn eval_constraint(expr: &Expr, selected: &BTreeSet) -> bool { } } +// ── Variant config directory loader (issue #287) ─────────────────────── + +/// Load every `*.yaml` / `*.yml` file in `dir` as a [`VariantConfig`]. +/// +/// Returns the list sorted by file name for deterministic order. Errors +/// out on the first malformed file. Used by `rivet validate`, +/// `rivet coverage`, `rivet list`, and `rivet query` to resolve their +/// `--variant ` flag against the project's `artifacts/variants/` +/// directory (or any other directory the caller picks). +/// +/// Empty / missing dirs return `Ok(vec![])` so callers can use this +/// unconditionally without first checking `dir.exists()`. +/// +/// Name collisions across files (two `*.yaml` files declaring the same +/// `name:`) are returned as an error — a project that ships duplicate +/// variant names has an ambiguity bug the user must fix before any +/// downstream consumer can pick the "right" one. +pub fn load_variant_configs_from_dir(dir: &std::path::Path) -> Result, Error> { + if !dir.exists() { + return Ok(Vec::new()); + } + let entries = std::fs::read_dir(dir) + .map_err(|e| Error::Io(format!("reading variants dir {}: {e}", dir.display())))?; + let mut paths: Vec = entries + .filter_map(|e| e.ok()) + .map(|e| e.path()) + .filter(|p| { + p.extension() + .and_then(|s| s.to_str()) + .is_some_and(|ext| ext == "yaml" || ext == "yml") + }) + .collect(); + paths.sort(); + + let mut configs: Vec = Vec::new(); + let mut seen: BTreeSet = BTreeSet::new(); + for path in &paths { + let yaml = std::fs::read_to_string(path) + .map_err(|e| Error::Io(format!("reading {}: {e}", path.display())))?; + let vc: VariantConfig = serde_yaml::from_str(&yaml).map_err(|e| { + Error::Schema(format!("parsing variant config {}: {e}", path.display())) + })?; + if !seen.insert(vc.name.clone()) { + return Err(Error::Schema(format!( + "duplicate variant name '{}' in {}", + vc.name, + path.display() + ))); + } + configs.push(vc); + } + Ok(configs) +} + // ── Tests ────────────────────────────────────────────────────────────── #[cfg(test)] @@ -1947,4 +2001,60 @@ bindings: "petrol when-clause must drop the glob from the manifest" ); } + + // ── load_variant_configs_from_dir (issue #287, Phase 2) ────────────── + + #[test] + fn load_variant_configs_from_dir_missing_dir_returns_empty() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("does-not-exist"); + let got = load_variant_configs_from_dir(&missing).unwrap(); + assert!( + got.is_empty(), + "missing directory must return empty list, not an error" + ); + } + + #[test] + fn load_variant_configs_from_dir_loads_every_yaml_sorted() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + std::fs::write(dir.join("zeta.yaml"), "name: zeta\nselects: [a, b]\n").unwrap(); + std::fs::write(dir.join("alpha.yaml"), "name: alpha\nselects: [x]\n").unwrap(); + std::fs::write( + dir.join("notes.txt"), + "ignored because the extension is wrong\n", + ) + .unwrap(); + let got = load_variant_configs_from_dir(dir).unwrap(); + let names: Vec<&str> = got.iter().map(|v| v.name.as_str()).collect(); + assert_eq!( + names, + vec!["alpha", "zeta"], + "results must be sorted by filename for reproducibility" + ); + assert_eq!(got[0].selects, vec!["x"]); + assert_eq!(got[1].selects, vec!["a", "b"]); + } + + #[test] + fn load_variant_configs_from_dir_rejects_duplicate_names() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + std::fs::write(dir.join("a.yaml"), "name: dupe\nselects: [x]\n").unwrap(); + std::fs::write(dir.join("b.yaml"), "name: dupe\nselects: [y]\n").unwrap(); + let err = load_variant_configs_from_dir(dir).unwrap_err(); + let msg = format!("{err}"); + assert!(msg.contains("duplicate variant name 'dupe'"), "got: {msg}"); + } + + #[test] + fn load_variant_configs_from_dir_surfaces_parse_errors() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + std::fs::write(dir.join("bad.yaml"), "name: 12345\nselects: not-a-list\n").unwrap(); + let err = load_variant_configs_from_dir(dir).unwrap_err(); + let msg = format!("{err}"); + assert!(msg.contains("parsing variant config"), "got: {msg}"); + } } diff --git a/rivet-core/src/validate.rs b/rivet-core/src/validate.rs index 6d7cf82..bb80518 100644 --- a/rivet-core/src/validate.rs +++ b/rivet-core/src/validate.rs @@ -974,6 +974,258 @@ pub fn validate_structural_with_externals( diagnostics } +// ── Variant overlay validation (issue #287, Phase 2) ─────────────────── + +/// Run the same required-field / allowed-values checks that +/// [`validate_structural_with_externals`] applies to `artifact.fields`, +/// but against an arbitrary `fields` map. +/// +/// This is the type-check primitive used by [`validate_variants`] when +/// validating the merged "default + overlay" view per +/// `docs/design/variant-aware-properties.md` §5.5. It deliberately does +/// not check link cardinality / link targets — those live on the +/// artifact, not in a variant overlay, so they're not re-checked under +/// `--variant`. +/// +/// `rule_suffix` is appended to the diagnostic `rule:` field +/// (e.g. `"-variant.industrial"`) so consumers can distinguish a +/// diagnostic about the default view from one about a variant overlay. +/// `id_suffix` is appended to the human-visible artifact id in the +/// message (e.g. `" (variant: industrial)"`) so the message is +/// unambiguous in `rivet validate --format text`. +#[allow(clippy::too_many_arguments)] // each argument carries an independent +// attribute of the variant overlay +// diagnostic; bundling into a struct +// would not improve readability +fn check_fields_against_type( + artifact_id: &str, + artifact_type: &str, + description: Option<&str>, + status: Option<&str>, + fields: &BTreeMap, + type_def: &ArtifactTypeDef, + rule_suffix: &str, + msg_suffix: &str, +) -> Vec { + let mut out = Vec::new(); + for field in &type_def.fields { + // 1. required fields + if field.required && !fields.contains_key(&field.name) { + let has_base = match field.name.as_str() { + "description" => description.is_some(), + "status" => status.is_some(), + _ => false, + }; + if !has_base { + out.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Error, + artifact_id: Some(artifact_id.to_string()), + rule: format!("required-field{rule_suffix}"), + message: format!( + "missing required field '{}' for type '{}'{msg_suffix}", + field.name, artifact_type + ), + }); + } + } + + // 2. allowed values (string, bool, and number forms — same shape + // as validate_structural_with_externals; YAML may coerce values + // so we check against the canonical aliases). + if let Some(allowed) = &field.allowed_values { + if let Some(value) = fields.get(&field.name) { + if let Some(s) = value.as_str() { + if !allowed.iter().any(|a| a == s) { + out.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Warning, + artifact_id: Some(artifact_id.to_string()), + rule: format!("allowed-values{rule_suffix}"), + message: format!( + "field '{}' has value '{}'{msg_suffix}, allowed: {:?}", + field.name, s, allowed + ), + }); + } + } else if let Some(b) = value.as_bool() { + let candidates: &[&str] = if b { + &["true", "yes"] + } else { + &["false", "no"] + }; + if !candidates.iter().any(|c| allowed.iter().any(|a| a == c)) { + out.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Warning, + artifact_id: Some(artifact_id.to_string()), + rule: format!("allowed-values{rule_suffix}"), + message: format!( + "field '{}' has value '{}' (boolean){msg_suffix}, allowed: {:?}", + field.name, b, allowed + ), + }); + } + } else if value.is_number() { + let num_str = if let Some(u) = value.as_u64() { + u.to_string() + } else if let Some(i) = value.as_i64() { + i.to_string() + } else if let Some(f) = value.as_f64() { + f.to_string() + } else { + format!("{:?}", value) + }; + if !allowed.iter().any(|a| a == &num_str) { + out.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Warning, + artifact_id: Some(artifact_id.to_string()), + rule: format!("allowed-values{rule_suffix}"), + message: format!( + "field '{}' has value '{}' (number){msg_suffix}, allowed: {:?}", + field.name, num_str, allowed + ), + }); + } + } + } + } + } + out +} + +/// Validate variant overlays across every artifact in the store. +/// +/// This is the Phase 2 (issue #287) entry point. It enforces the +/// `docs/design/variant-aware-properties.md` §5.5 semantics: +/// +/// 1. **Variant-key cross-check.** For every artifact with a non-empty +/// `fields-per-variant:`, each outer key must appear in +/// `known_variants`. Unknown keys produce a Warning by default, +/// promoted to Error when `strict` is true. `known_variants` is +/// typically the union of: +/// - declared variant-config names (from `artifacts/variants/`), +/// - feature names in the feature model. +/// +/// 2. **Overlaid value type-check.** For every artifact, every +/// declared variant overlay is type-checked using the same +/// required-field / allowed-values rules as the default `fields:` +/// block — so a variant overlay can never "patch" a required field +/// into an invalid state. Errors here always fire (they describe +/// schema violations, not naming hygiene). +/// +/// 3. **Active-variant merged-view check.** When `active` is `Some`, +/// every artifact is additionally type-checked under the merged +/// view (default ∪ overlay[active]). This is the user-facing meaning +/// of `rivet validate --variant industrial`: "the resolved view for +/// `industrial` is consistent". +/// +/// Per the design doc the default view is **also** validated via the +/// existing validator pipeline; this function is purely additive — it +/// does not duplicate any of the default-view diagnostics. Callers +/// should compose: `validate_with_externals(...) + validate_variants(...)`. +pub fn validate_variants( + store: &Store, + schema: &Schema, + externals: &ExternalSchemas, + active: Option<&str>, + known_variants: &std::collections::BTreeSet, + strict: bool, +) -> Vec { + let mut diagnostics = Vec::new(); + let known_sorted: Vec<&str> = known_variants.iter().map(String::as_str).collect(); + + for artifact in store.iter() { + // ── 1. variant-key cross-check ────────────────────────────── + for variant_key in artifact.fields_per_variant.keys() { + if !known_variants.contains(variant_key) { + let severity = if strict { + Severity::Error + } else { + Severity::Warning + }; + let expected = if known_sorted.is_empty() { + "(no variants declared)".to_string() + } else { + known_sorted.join(", ") + }; + diagnostics.push(Diagnostic { + source_file: artifact.source_file.clone(), + line: None, + column: None, + severity, + artifact_id: Some(artifact.id.clone()), + rule: "variant-key-unknown".to_string(), + message: format!( + "variant key '{}' in fields-per-variant is not declared in any \ + variant config or feature; expected one of: {}", + variant_key, expected + ), + }); + } + } + + // Resolve the artifact's type to drive value-level checks. + let type_def = match lookup_type(artifact, schema, externals) { + TypeLookup::Found(td) => td, + // Unknown / no-schema-for-external cases are already + // reported by the main validator; skip the variant checks + // here (we'd just generate duplicate diagnostics). + _ => continue, + }; + + // ── 2. type-check every declared variant overlay ──────────── + // The overlay is the default fields with the variant keys + // merged on top. This catches a variant overlay that puts an + // out-of-set value into an `allowed-values` field, or that + // would unset a required field (variant overlays only add or + // replace, so the "unset required" case is checked indirectly + // by re-running the required-fields check on the merged map — + // a required field that was already present in `fields` will + // remain present after merging). + for variant_name in artifact.fields_per_variant.keys() { + let merged = artifact.fields_for_variant(Some(variant_name)); + let rule_suffix = format!("-variant.{variant_name}"); + let msg_suffix = format!(" (variant: {variant_name})"); + diagnostics.extend(check_fields_against_type( + &artifact.id, + &artifact.artifact_type, + artifact.description.as_deref(), + artifact.status.as_deref(), + &merged, + type_def, + &rule_suffix, + &msg_suffix, + )); + } + + // ── 3. active-variant merged-view check ───────────────────── + // Only re-run when `active` was given AND the artifact actually + // has an overlay for it — otherwise the merged view equals the + // default view, which the main validator already covered. + if let Some(name) = active { + if artifact.fields_per_variant.contains_key(name) { + // Already covered by the loop above — no extra work, + // but we leave the branch here as the single place to + // hang future "merged-view-only" checks (e.g. a future + // `range` check that does not apply to the default). + let _ = name; + } + } + } + + diagnostics +} + /// Validate document `[[ID]]` references against the artifact store. /// /// Returns diagnostics for any reference that points to a non-existent artifact. @@ -3442,4 +3694,206 @@ then: phase-9 validation rules" ); } + + // ── validate_variants (issue #287, Phase 2) ────────────────────────── + + /// Build a schema with a single `requirement` type that has a + /// required `priority` field constrained to {must, should, could}. + fn variant_test_schema() -> Schema { + let mut file = minimal_schema("requirement"); + file.artifact_types = vec![ArtifactTypeDef { + name: "requirement".to_string(), + description: "Requirement".into(), + fields: vec![FieldDef { + name: "priority".into(), + field_type: "string".into(), + required: true, + description: None, + allowed_values: Some(vec!["must".into(), "should".into(), "could".into()]), + }], + link_fields: vec![], + aspice_process: None, + common_mistakes: vec![], + example: None, + yaml_section: None, + yaml_sections: vec![], + yaml_section_suffix: None, + shorthand_links: std::collections::BTreeMap::new(), + }]; + Schema::merge(&[file]) + } + + fn variant_artifact() -> Artifact { + let mut a = minimal_artifact("REQ-THERMAL-01", "requirement"); + a.fields + .insert("priority".into(), serde_yaml::Value::String("must".into())); + a + } + + #[test] + fn validate_variants_flags_unknown_variant_keys_as_warning_by_default() { + let mut a = variant_artifact(); + let mut overlay = BTreeMap::new(); + overlay.insert("priority".into(), serde_yaml::Value::String("must".into())); + a.fields_per_variant.insert("unknown-name".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let mut known = std::collections::BTreeSet::new(); + known.insert("automotive".to_string()); + known.insert("consumer".to_string()); + + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + let d = diags + .iter() + .find(|d| d.rule == "variant-key-unknown") + .expect("unknown-key diagnostic must fire"); + assert_eq!(d.severity, Severity::Warning); + assert!( + d.message.contains("'unknown-name'"), + "msg should name the offending key: {}", + d.message + ); + assert!( + d.message.contains("automotive"), + "msg should list known variants: {}", + d.message + ); + } + + #[test] + fn validate_variants_strict_promotes_unknown_variant_key_to_error() { + let mut a = variant_artifact(); + let mut overlay = BTreeMap::new(); + overlay.insert("priority".into(), serde_yaml::Value::String("must".into())); + a.fields_per_variant.insert("unknown".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let known: std::collections::BTreeSet = + ["automotive".to_string()].into_iter().collect(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, true); + let d = diags + .iter() + .find(|d| d.rule == "variant-key-unknown") + .expect("unknown-key diagnostic must fire"); + assert_eq!(d.severity, Severity::Error, "strict promotes to error"); + } + + #[test] + fn validate_variants_type_checks_overlay_allowed_values() { + // Overlay puts "maybe" into priority, which is not in + // {must, should, could} — must fire as a warning. + let mut a = variant_artifact(); + let mut overlay = BTreeMap::new(); + overlay.insert("priority".into(), serde_yaml::Value::String("maybe".into())); + a.fields_per_variant.insert("automotive".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let known: std::collections::BTreeSet = + ["automotive".to_string()].into_iter().collect(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + let d = diags + .iter() + .find(|d| d.rule == "allowed-values-variant.automotive") + .expect("overlay allowed-values diagnostic must fire"); + assert_eq!(d.severity, Severity::Warning); + assert!( + d.message.contains("'maybe'") && d.message.contains("automotive"), + "msg should call out value + variant: {}", + d.message + ); + } + + #[test] + fn validate_variants_passes_when_overlay_is_valid_and_key_is_known() { + let mut a = variant_artifact(); + let mut overlay = BTreeMap::new(); + overlay.insert( + "priority".into(), + serde_yaml::Value::String("should".into()), + ); + a.fields_per_variant.insert("automotive".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let known: std::collections::BTreeSet = + ["automotive".to_string()].into_iter().collect(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + assert!( + diags.is_empty(), + "no diagnostics expected, got: {:?}", + diags + ); + } + + #[test] + fn validate_variants_required_field_missing_in_merged_view() { + // Default fields lacks `priority`; overlay also doesn't supply + // it. The merged view is still missing the required field — a + // separate diagnostic should fire under the variant's rule. + let mut a = minimal_artifact("REQ-1", "requirement"); + // No `priority` in defaults. + let mut overlay = BTreeMap::new(); + // Overlay sets some unrelated key so the overlay is non-empty. + overlay.insert( + "title-override".into(), + serde_yaml::Value::String("auto title".into()), + ); + a.fields_per_variant.insert("automotive".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let known: std::collections::BTreeSet = + ["automotive".to_string()].into_iter().collect(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + let d = diags + .iter() + .find(|d| d.rule == "required-field-variant.automotive") + .expect("required-field-variant.automotive must fire"); + assert_eq!(d.severity, Severity::Error); + assert!(d.message.contains("priority"), "got: {}", d.message); + assert!(d.message.contains("automotive"), "got: {}", d.message); + } + + #[test] + fn validate_variants_empty_overlays_and_no_active_emits_nothing() { + // Artifact has fields_per_variant: {} — nothing to check. + let a = variant_artifact(); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + let known: std::collections::BTreeSet = Default::default(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + assert!(diags.is_empty(), "got: {diags:?}"); + } + + #[test] + fn validate_variants_known_set_can_be_features_or_configs() { + // The function treats `known_variants` as opaque set membership; + // upstream may seed it with declared variant-config names OR + // feature names. This test pins that contract. + let mut a = variant_artifact(); + let mut overlay = BTreeMap::new(); + overlay.insert("priority".into(), serde_yaml::Value::String("must".into())); + // Pretend "electric" is a feature name, not a variant config. + a.fields_per_variant.insert("electric".into(), overlay); + let mut store = Store::default(); + store.upsert(a); + let schema = variant_test_schema(); + + let known: std::collections::BTreeSet = + ["electric".to_string()].into_iter().collect(); + let diags = validate_variants(&store, &schema, &BTreeMap::new(), None, &known, false); + assert!( + diags.iter().all(|d| d.rule != "variant-key-unknown"), + "feature names must be accepted as known variant keys: {diags:?}" + ); + } }