Skip to content

Commit b16bb9c

Browse files
avrabeclaude
andauthored
fix(variants): skip feature-model binding files in load_variant_configs_from_dir (#532) (#539)
`rivet validate` globs every `*.yaml` in `artifacts/variants/` and parses each as a flat single-variant config (which requires a top-level `name:`). When a feature-model binding file (the natural co-location: `bindings:` map + optional `variants:` list, the `FeatureBinding` shape) lives in that directory beside the single-variant configs it binds, the parse fails and the CLI prints warning: failed to load variant configs from ./artifacts/variants: Schema error: parsing variant config ./artifacts/variants/bindings.yaml: missing field `name` at line 18 column 1 — benign (validate still PASSes) but noise any product-line consumer that co-locates its binding file hits. `load_variant_configs_from_dir` now peeks each file as `serde_yaml::Value` before the typed parse and silently skips files whose top-level shape is a binding file (a `bindings:` key with no `name:` or `variant:` sibling — the discriminator excludes the flat single-variant form and the `variant:`-wrapped form `rivet variant init` writes per #514, so the wrapped form's existing parse path still fires). Unparseable YAML still surfaces an error through the existing parse path. Confirmed: built the binary, reproduced the issue's exact directory layout (sem-m3-gcc.yaml + sem-smp-x86.yaml + bindings.yaml with variants/bindings top-level keys), and `rivet validate` now reports `Result: PASS (0 warnings)` with no mention of bindings.yaml. Two new tests: one for the binding-file skip on the issue's repro shape, one regression guard that the `variant:`-wrapped form is NOT absorbed by the skip. All 57 feature_model unit tests + 8 variant_phase2 integration tests pass; `cargo clippy --all-targets -- -D warnings` and `cargo fmt --all -- --check` clean. Closes #532. Fixes: REQ-004 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 01f2976 commit b16bb9c

1 file changed

Lines changed: 105 additions & 0 deletions

File tree

rivet-core/src/feature_model.rs

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,6 +1640,16 @@ pub fn load_variant_configs_from_dir(dir: &std::path::Path) -> Result<Vec<Varian
16401640
for path in &paths {
16411641
let yaml = std::fs::read_to_string(path)
16421642
.map_err(|e| Error::Io(format!("reading {}: {e}", path.display())))?;
1643+
// A feature-model binding file (the `bindings:` map + optional
1644+
// `variants:` list shape of `FeatureBinding`) can legitimately live
1645+
// beside the single-variant configs it binds. Detect and skip it so
1646+
// the loader doesn't fail trying to parse it as `VariantConfig`
1647+
// (#532). The discriminator is a top-level `bindings:` key with
1648+
// neither `name:` (flat variant config) nor `variant:` (wrapped
1649+
// variant config from `rivet variant init`, see #514) present.
1650+
if is_feature_binding_file(&yaml) {
1651+
continue;
1652+
}
16431653
let vc: VariantConfig = serde_yaml::from_str(&yaml).map_err(|e| {
16441654
Error::Schema(format!("parsing variant config {}: {e}", path.display()))
16451655
})?;
@@ -1655,6 +1665,25 @@ pub fn load_variant_configs_from_dir(dir: &std::path::Path) -> Result<Vec<Varian
16551665
Ok(configs)
16561666
}
16571667

1668+
/// True when `yaml` is structurally a feature-model binding file (a
1669+
/// top-level `bindings:` map with no `name:`/`variant:` siblings), as
1670+
/// opposed to a single-variant config. Used by `load_variant_configs_from_dir`
1671+
/// to skip binding files that share the `artifacts/variants/` directory
1672+
/// with the variant configs they bind (#532). Unparseable YAML returns
1673+
/// `false` so the caller's existing error path still fires.
1674+
fn is_feature_binding_file(yaml: &str) -> bool {
1675+
let Ok(value) = serde_yaml::from_str::<serde_yaml::Value>(yaml) else {
1676+
return false;
1677+
};
1678+
let serde_yaml::Value::Mapping(map) = value else {
1679+
return false;
1680+
};
1681+
let key = |k: &str| serde_yaml::Value::String(k.to_string());
1682+
map.contains_key(key("bindings"))
1683+
&& !map.contains_key(key("name"))
1684+
&& !map.contains_key(key("variant"))
1685+
}
1686+
16581687
// ── Tests ──────────────────────────────────────────────────────────────
16591688

16601689
#[cfg(test)]
@@ -2487,6 +2516,82 @@ bindings:
24872516
assert!(msg.contains("parsing variant config"), "got: {msg}");
24882517
}
24892518

2519+
// #532: a feature-model binding file (`variants:` list + `bindings:` map)
2520+
// can legitimately live in artifacts/variants/ beside the single-variant
2521+
// configs it binds. The loader must skip it rather than fail with
2522+
// "missing field `name`".
2523+
#[test]
2524+
fn load_variant_configs_from_dir_skips_binding_files() {
2525+
let tmp = tempfile::tempdir().unwrap();
2526+
let dir = tmp.path();
2527+
// The repro from #532: real single-variant configs beside a binding
2528+
// file. Only the named configs should load; the binding file is
2529+
// silently skipped.
2530+
std::fs::write(
2531+
dir.join("sem-m3-gcc.yaml"),
2532+
"name: sem-m3-gcc\nselects: [m3, gcc]\n",
2533+
)
2534+
.unwrap();
2535+
std::fs::write(
2536+
dir.join("sem-smp-x86.yaml"),
2537+
"name: sem-smp-x86\nselects: [smp, x86]\n",
2538+
)
2539+
.unwrap();
2540+
std::fs::write(
2541+
dir.join("bindings.yaml"),
2542+
r#"
2543+
variants:
2544+
- name: sem-m3-gcc
2545+
selects: [m3, gcc]
2546+
bindings:
2547+
m3:
2548+
artifacts: []
2549+
gcc:
2550+
artifacts: []
2551+
"#,
2552+
)
2553+
.unwrap();
2554+
let got = load_variant_configs_from_dir(dir).expect("binding file must not fail the load");
2555+
let names: Vec<&str> = got.iter().map(|v| v.name.as_str()).collect();
2556+
assert_eq!(
2557+
names,
2558+
vec!["sem-m3-gcc", "sem-smp-x86"],
2559+
"binding file must be skipped; only single-variant configs load"
2560+
);
2561+
}
2562+
2563+
// The wrapped variant config form `rivet variant init` writes (#514) has
2564+
// a top-level `variant:` key alongside a `bindings:` block. The skip
2565+
// discriminator must NOT confuse it with a pure binding file — and the
2566+
// loader must still surface its parse failure (it's not in flat form),
2567+
// not silently drop it. This is the regression guard for the discriminator.
2568+
#[test]
2569+
fn load_variant_configs_from_dir_does_not_skip_wrapped_variant_form() {
2570+
let tmp = tempfile::tempdir().unwrap();
2571+
let dir = tmp.path();
2572+
std::fs::write(
2573+
dir.join("init.yaml"),
2574+
r#"
2575+
variant:
2576+
name: kiln
2577+
selects: [telemetry]
2578+
bindings:
2579+
telemetry:
2580+
artifacts: []
2581+
"#,
2582+
)
2583+
.unwrap();
2584+
// The loader currently uses flat-form parsing, so this surfaces as a
2585+
// parse error rather than silently disappearing — that's the
2586+
// contract we need to preserve. (If/when the loader adopts
2587+
// `VariantConfig::from_yaml_str`, this test should flip to assert
2588+
// the wrapped form loads, which is fine — but the binding-file
2589+
// skip must not have absorbed it.)
2590+
let err = load_variant_configs_from_dir(dir).unwrap_err();
2591+
let msg = format!("{err}");
2592+
assert!(msg.contains("parsing variant config"), "got: {msg}");
2593+
}
2594+
24902595
// ── Feature-model composition (REQ-083) ────────────────────────────
24912596

24922597
const SUB_POWERTRAIN: &str = r#"

0 commit comments

Comments
 (0)