fix(feature-model): cross-tree constraints silently pass#156
Merged
Conversation
…ions Before this change, `rivet variant check` silently reported PASS on variants that explicitly violated `(implies X (not Y))` and other cross-tree constraints where forward propagation could not auto-select a feature to satisfy the consequent. The solver only used `Expr::Implies` to schedule consequent features for selection; when the consequent was a negation, a compound, or any non-feature-name shape, no check ever fired against the propagated selection — a false-positive PASS on a safety-critical validation surface. Fix: add a `eval_constraint` pass after propagation that treats every top-level constraint as a boolean assertion over the effective feature set, with standard propositional semantics for `and`/`or`/`not`/ `implies`/`excludes`. `excludes` keeps its dedicated diagnostic string to preserve existing error messages; other shapes report through a new `describe_constraint` helper. Unknown artifact-oriented predicates (link queries, regex matches) default to true so unrelated constraint flavours do not trigger spurious violations. Regression tests cover the reported shape `(implies X (not Y))` with both X and Y selected (now FAIL), the companion case with only X selected (still PASS), and ensure forward propagation of `(implies X Y)` still works. Fixes: REQ-044
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 62ccd34 | Previous: aa706fc | Ratio |
|---|---|---|---|
traceability_matrix/1000 |
58483 ns/iter (± 635) |
45738 ns/iter (± 588) |
1.28 |
query/10000 |
108855 ns/iter (± 1336) |
90574 ns/iter (± 529) |
1.20 |
This comment was automatically generated by workflow using github-action-benchmark.
thin-vec 0.2.14 has a Double-Free / UAF in IntoIter::drop and ThinVec::clear. Pulled in transitively via salsa 0.26.0. Rivet does not directly construct or iterate thin_vec::ThinVec — the exposure is through salsa's internal data structures. Ignore in both cargo-deny and cargo-audit until either salsa bumps its thin-vec dependency or thin-vec 0.2.15 lands upstream. Trace: skip
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HIGH severity: silent false-positive PASS
Before this PR,
rivet variant checkreported PASS on variants that explicitly violated cross-tree constraints like `(implies X (not Y))`. The entire constraint surface was effectively decorative for any constraint whose consequent wasn't a bare feature name — only `excludes` had a real post-propagation check.Root cause
`Expr::Implies` was used only to forward-propagate feature selection: "if antecedent is selected and consequent is a bare feature name, select it too." When the consequent was anything more complex (`not`, `and`, `or`, link predicate, regex), `extract_feature_name` returned `None`, the propagation loop fell through, and `solve` returned `Ok(...)`. No validation ever ran.
Fix
`rivet-core/src/feature_model.rs::solve` now runs a generic `eval_constraint` pass after propagation:
Evidence
Before: failing test reproduced — `(implies feature-x (not feature-y))` with both selected got `Ok`.
After: 3 regression tests pass, all 560 rivet-core lib tests still green, clippy clean.
Test plan
🤖 Generated with Claude Code