fix(v0.4.1): 5 user-reported pain points (#3, #4, #6, #7, #8)#174
Merged
fix(v0.4.1): 5 user-reported pain points (#3, #4, #6, #7, #8)#174
Conversation
Pain point: users reverse-engineer the feature-model YAML schema because `rivet variant --help` has no field reference and `selects:` vs `selected:` / group types / s-expression constraint syntax / bindings file shape are undocumented. Changes: - Add docs/feature-model-schema.md: top-level reference for feature model YAML (root, features, group types, constraint syntax) with a worked example. - Add docs/feature-model-bindings.md: dedicated binding file reference. - Link both from docs/getting-started.md. - Variant subcommand doc-comment now points at the schema reference so `rivet variant --help` surfaces it. - Add `rivet variant init <name>` scaffolder that writes a starter feature-model.yaml + bindings/<name>.yaml with comments documenting every field. Tests: 3 new integration tests in rivet-cli/tests/variant_init.rs covering scaffolded file contents, overwrite protection, and that the scaffolded template parses clean via `rivet variant list`. Implements: REQ-042, REQ-043, REQ-044 Refs: REQ-046 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…very) Pain point: `rivet init --hooks` emitted a pre-commit hook that ran `rivet validate` at the git root. If the rivet project is relocated inside the working tree (e.g. moved to subdir/), the hook either silently validates the wrong directory or fails to find rivet.yaml. Fix: the installed pre-commit hook now walks up from $PWD until it finds a directory containing rivet.yaml, then cd's there before invoking `rivet validate`. If no rivet.yaml exists in the ancestor chain, the hook exits 0 silently so it does not block commits in unrelated repositories. Tests: rivet-cli/tests/hooks_install.rs adds 2 integration tests — one verifies the hook body does not embed a hard-coded -p/--project flag and uses the walk-up pattern; one stages a fresh project, moves rivet.yaml into a subdirectory, and confirms the hook still discovers it when run from a nested path. Fixes: REQ-051 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mics) Pain point: variant-scoped validation required --model, --variant, and --binding to be passed together — there was no way to validate just model/binding consistency, and no single-invocation way to assert a whole batch of variants is valid. Changes: - `rivet validate --model X --binding Y` (no --variant) now parses the model, parses the binding, and checks that every feature referenced in the binding exists in the model. Reports a clear diagnostic on unknown feature names instead of the old "must all be provided together" error. The full --model + --variant + --binding mode is unchanged. - `rivet variant check-all --model M --binding B` iterates every variant declared under `variants:` in the binding file, prints a PASS/FAIL line per variant, and exits non-zero if any fail. - `FeatureBinding` in rivet-core grows an optional `variants:` field (default empty) so the same file can carry bindings and declared variants without schema churn. Tests: 5 new integration tests in rivet-cli/tests/variant_scoped_api.rs cover the no-variant validate mode, the unknown-feature diagnostic, check-all exit codes for mixed/all-pass fixtures, and JSON output shape. Existing feature_model unit tests still pass (binding YAML is backward-compatible — `variants:` defaults to empty). Implements: REQ-044, REQ-045, REQ-046 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pain point: `[FilterError { offset: 14, message: "unexpected atom at
top level" }]` exposed parser internals. Users writing `A and B`,
`and A B`, or `(bogus A B)` got a positional offset with no hint that
they were using the wrong syntax.
Fix: extend `FilterError` with an optional `note` field, populated by a
classifier that inspects the source before parsing. Three common shapes
get a semantic nudge:
- bare infix (`A and B`) → suggest `(and A B)`.
- missing outer parens (`and A B`) → suggest wrapping it.
- unknown head (`(bogus …)`) → reference the supported form list.
`FilterError::Display` renders the positional detail followed by the
note on a new line. Feature-model constraint errors now format via
Display instead of Debug, so the note bubbles out through
`rivet variant check` / `rivet validate --model` paths.
Tests: 4 new unit tests in sexpr_eval covering the three error shapes
plus a success case that must carry no note. All pre-existing tests
unchanged.
Fixes: REQ-043
Implements: REQ-042
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ry vs implied) Pain point: `rivet variant solve` output mixed user-picked features with ones the solver added via mandatory-group propagation or constraint implication. A flat list like `base, auth, oauth, token-cache, metrics` didn't tell the user which features were their intent and which were downstream effects. Minimum-impact change per scope-limits brief (risk of conflict with PR #156 cross-tree constraint work): - Extend `ResolvedVariant` with a per-feature `origins: BTreeMap<String, FeatureOrigin>` where FeatureOrigin is UserSelected / Mandatory / ImpliedBy(name) / AllowedButUnbound. - Populate origins alongside the existing selected-set fixpoint loop — no algorithmic changes. First-reason-wins on insertion so user selection beats later mandatory/implied discoveries. - Text output of `rivet variant solve` prints one feature per line, prefixed with `+`, labeled (mandatory) / (selected) / (implied by X). - JSON output is strictly additive: `effective_features` + `feature_count` preserved, new `origins` object keyed by feature name. Tests: - 4 new unit tests in rivet-core/src/feature_model.rs covering each origin variant. - 2 new integration tests in rivet-cli/tests/variant_solve_origins.rs asserting text prefixes/labels and JSON backwards compatibility. - All 15 pre-existing feature_model unit tests still pass; all 6 proptest_feature_model properties still hold. Implements: REQ-043, REQ-046 Refs: REQ-052 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
avrabe
force-pushed
the
fix/v041-pain-points-3-8
branch
from
c46a375 to
ce8a2d4
Compare
![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: ce8a2d4 | Previous: 61bfc41 | Ratio |
|---|---|---|---|
store_lookup/100 |
2017 ns/iter (± 6) |
1677 ns/iter (± 3) |
1.20 |
store_lookup/1000 |
25058 ns/iter (± 185) |
19262 ns/iter (± 54) |
1.30 |
diff/100 |
64180 ns/iter (± 128) |
53201 ns/iter (± 407) |
1.21 |
query/100 |
777 ns/iter (± 4) |
646 ns/iter (± 2) |
1.20 |
query/1000 |
6861 ns/iter (± 112) |
5450 ns/iter (± 12) |
1.26 |
This comment was automatically generated by workflow using github-action-benchmark.
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.
Fixes a batch of v0.4.0 rough edges reported against the variant / hooks / sexpr surfaces. Each commit is self-contained with its own tests.
Summary of commits
c4fcb00docs+feat(variant): feature-model schema reference + init scaffolderrivet variant initnow scaffolds starter files with commented fields.7a35861fix(hooks): pre-commit hook walks up to find rivet.yamlrivet.yamlno longer breaks the pre-commit hook — marker discovery from$PWD.01c244efeat(variant): check-all + optional --variant on validate--variantnow optional for model+binding consistency validation; newrivet variant check-alliterates declared variants.612bfe6fix(sexpr): semantic notes on filter parse errorsFilterErrormessages now carry a semantic note for bare infix / missing parens / unknown head shapes.c46a375feat(variant-solve): per-feature origin trackingrivet variant solvelabels each effective feature as (mandatory) / (selected) / (implied by X); JSON output adds anoriginsmap and stays backward-compatible.Tests
rivet-cli/tests/variant_init.rsrivet-cli/tests/hooks_install.rsrivet-cli/tests/variant_scoped_api.rsrivet-cli/tests/variant_solve_origins.rsrivet-core/src/sexpr_eval.rsrivet-core/src/feature_model.rsTotal: 20 new tests. All 589 pre-existing rivet-core lib tests still pass; all 6 proptest_feature_model properties still hold;
cargo clippy --workspace --all-targets -- -D warningsclean.Scope limits respected
Per the brief's scope note on PR #156 (cross-tree constraints): fix #8 extends
ResolvedVariantwith a neworiginsfield and populates it alongside the existing fixpoint loop — no refactor of the solve algorithm. Tests confirm PR #156's cross-tree semantics still hold.Test plan
cargo test -p rivet-cli --test variant_initcargo test -p rivet-cli --test hooks_installcargo test -p rivet-cli --test variant_scoped_apicargo test -p rivet-cli --test variant_solve_originscargo test -p rivet-core --lib feature_modelcargo test -p rivet-core --lib sexpr_evalcargo test --workspace --lib(589 tests)cargo clippy --workspace --all-targets -- -D warnings🤖 Generated with Claude Code