fix(schema): merge same-name types by union, not replace (#154)#280
Merged
Conversation
📐 Rivet artifact deltaNo artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph. |
Bridge and overlay schemas that declared an artifact-type or link-type
with the same name as a base schema's silently lost the base's fields.
The merge was a plain HashMap::insert — later wins, parent fields gone.
This is the silent-correctness bug behind every bridge schema that
re-declares a type to add a single safety attribute. A real reproducer
in this repo: schemas/safety-case.yaml declares `safety-goal` with
fields [claim, goal-type, asil, undeveloped] and link-field
sub-goal-of; schemas/iso-26262.yaml re-declares `safety-goal` with
fields [asil]. Loading [common, safety-case, iso-26262] previously
gave a `safety-goal` with only [asil] — the GSN claim, goal-type, and
sub-goal-of link-field were silently dropped. Every safety-case
artifact in such a project then tripped "field not defined" diagnostics
and zero declared link-fields broke cardinality enforcement.
The fix:
- Add `ArtifactTypeDef::merge_in_place(&mut self, other: ArtifactTypeDef)`
with per-field union semantics:
* `description`: later wins when non-empty
* `fields`, `link_fields`: union by name; later wins on same-name
conflicts (so an overlay can change a field's `required` or
`field_type` deliberately, but cannot accidentally drop unrelated
fields)
* `shorthand_links`, `common_mistakes`: append/extend
* `yaml_sections`: union with dedup
* `aspice_process`, `example`, `yaml_section`, `yaml_section_suffix`:
later-Some wins
- Add `LinkTypeDef::merge_in_place` with the same shape: scalar fields
take later-non-empty; `source_types` and `target_types` union with
dedup.
- Free helper `merge_named_vec` does the union-by-name on Vec<T>
(used for both `fields` and `link_fields`).
- `Schema::merge` switches from `insert` to `entry().{merge_in_place
| insert}` for both artifact_types and link_types.
Six regression tests in `schema::tests`:
merge_same_name_artifact_type_unions_fields
merge_same_name_artifact_type_unions_link_fields
merge_preserves_shorthand_links_from_parent
merge_idempotent_with_same_file_twice
merge_order_independent_for_disjoint_additions
merge_same_name_link_type_unions_target_types
The shorthand-link preservation test guards the secondary symptom from
parent's `controller: …` shorthand silently stopped expanding into a
`links:` entry — because `Schema::merge` only populates
`shorthand_links` from the type that survived the insert. With this
fix, the parent's link-fields stay in place and the shorthand
expansion in `yaml_hir::extract_section_item` keeps working.
Backward-compat: projects that relied on the replace behaviour were
already re-declaring every parent field (per the G.2 warning in
rivet-cli/src/quickstart.md), so for them the union is a no-op —
same-name fields just stay identical. The new failure mode is
"deliberately removing a parent field", which has no syntax today
(no `remove-fields:` or `override: true` marker), so removing that
capability isn't a regression.
Fixes: #154
Implements: REQ-010
Refs: REQ-004
CI Format job flagged whitespace drift in the new #154 regression tests (long argument lists wrapping differently than rustfmt expects). Pure formatting; no semantic change.
996a57b to
f85e2ea
Compare
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: f85e2ea | Previous: fd4cf19 | Ratio |
|---|---|---|---|
link_graph_build/10000 |
41232527 ns/iter (± 2308992) |
30575891 ns/iter (± 2813963) |
1.35 |
query/10000 |
142236 ns/iter (± 1033) |
110844 ns/iter (± 721) |
1.28 |
This comment was automatically generated by workflow using github-action-benchmark.
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
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.
Summary
Closes #154. Bridge and overlay schemas that re-declared an artifact-type or link-type with the same name as a base schema silently lost the base's fields. The merge was a plain
HashMap::insert— later wins, parent fields gone.This is the silent-correctness bug behind every bridge schema that re-declares a type to add a single safety attribute. A real reproducer in this repo:
Every safety-case artifact in such a project then tripped "field not defined" diagnostics and the missing link-fields broke cardinality enforcement.
The fix
Merge semantics per field
Test plan
Six new regression tests in `schema::tests`:
Secondary symptom from #154 (stpa-yaml shorthand)
The issue body also reports that `stpa-yaml` shorthand isn't expanded into the link graph when a child schema redeclares a type. This fix collapses that symptom automatically: `Schema::merge` builds `shorthand_links` from the type's `link_fields`, and the previous insert-based merge dropped the parent's `link_fields` — so the shorthand expansion in `yaml_hir::extract_section_item` silently stopped working. With union-merge, the parent's `link_fields` stay; the shorthand expansion keeps working. No separate code change needed; the regression test `merge_preserves_shorthand_links_from_parent` guards this path.
Backward-compat
Projects that relied on replace behaviour were already re-declaring every parent field (per the G.2 warning in `rivet-cli/src/quickstart.md`). For them, the union is a no-op — same-name fields just stay identical. The new failure mode is "deliberate parent-field removal", which has no syntax today (no `remove-fields:` or `override: true` marker), so removing that capability isn't a regression. If a future RFC adds explicit override semantics, that's an additive change.
The G.2 advice in `quickstart.md:569-577` becomes obsolete after this lands — worth a follow-up doc edit.
Trailer
`Fixes: #154` · `Implements: REQ-010` (schema) · `Refs: REQ-004` (validation correctness)
🤖 Generated with Claude Code