fix(externals): load each external's schemas; type-check prefixed artifacts against them (#245)#267
Merged
Merged
Conversation
…ifacts against them (#245) When a downstream rivet.yaml declares an external (e.g. sigil declares synth), the loader pulled in the external's artifacts but consulted only the downstream's schemas during type-checking. Any artifact whose `type` was defined in the external's schemas but not the downstream's emitted ERROR: unknown artifact type — 326 errors in the sigil-with-synth repro, drowning out 54 real lifecycle gaps. This change: - `rivet-core/src/externals.rs`. `load_external_project` now also reads the external's `project.schemas` list and loads them into a `Schema`, returned alongside the artifacts. `ResolvedExternal` carries the schema via a new `schema: Option<Schema>` field. `None` is the permissive-fallback (external declared no schemas, or they failed to load). - `rivet-core/src/validate.rs`. New `ExternalSchemas` map (prefix -> Option<Schema>) and `validate_with_externals` / `validate_structural_with_externals` entry points that consult `externals[prefix]` first when an artifact id is `<prefix>:<id>`, falling through to the downstream schema. Unknown-link-type check is symmetrically extended. The original `validate` / `validate_structural` are now thin wrappers that delegate with an empty externals map (backwards-compatible). - `reclassify_externals_diagnostics` — post-pass for the salsa validation path, which doesn't yet take per-external schemas as a tracked input. Reclassifies known-type and unknown-link-type diagnostics on prefixed artifacts so the salsa output converges with validate_with_externals. - `rivet-cli/src/main.rs`. `ProjectContext` gains an `external_schemas` field populated during `load`. `cmd_validate` (--direct uses validate_with_externals; default uses salsa + reclassify), `cmd_stats`, `cmd_diff`, `cmd_context`, and the dashboard export all switch to the externals-aware variants so their counts agree. - Permissive fallback: when the external declared no schemas, the prefix is registered with `Some(prefix) -> None` and unknown-type / unknown-link-type findings for that prefix are demoted from ERROR to INFO with a message naming the external. Tests: - `rivet-core/tests/externals_schemas.rs` (new, 4 tests). Verifies the spar fixture (declares schemas: [common, aadl]) eliminates spurious aadl-component ERRORs once registered; verifies load_all_externals populates ResolvedExternal.schema; verifies the no-schema fallback demotes unknown-type to INFO; verifies unknown-link-type symmetry. - `rivet-core/src/externals.rs` unit test asserts the schema field is populated and contains expected types. - `rivet-core/tests/externals_sync.rs` updated for the new tuple return. Verification: - `cargo test -p rivet-core -p rivet-cli` — green (920 + new 4 in rivet-core lib; full integration suite green in rivet-cli including the previously-broken `stats_json_counts_match_validate`). - `cargo clippy --workspace --all-targets -- -D warnings` — clean (only the pre-existing MSRV-mismatch warning). - `cargo fmt --all -- --check` — clean. - `cargo run --release -p rivet-cli -- validate` against the rivet repo: `Result: FAIL (6 errors, 140 warnings, 0 broken cross-refs)` — byte-identical to the pristine main baseline. Closes #245 Implements: REQ-007 Verifies: REQ-007 https://claude.ai/code/session_01TvVQDk6UyLrSj6jc28LQqM
📐 Rivet artifact deltaNo artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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: 1c1bf3f | Previous: 08b87f8 | Ratio |
|---|---|---|---|
query/10000 |
158604 ns/iter (± 433) |
92475 ns/iter (± 909) |
1.72 |
This comment was automatically generated by workflow using github-action-benchmark.
5 tasks
avrabe
added a commit
that referenced
this pull request
May 11, 2026
Three queued feature requests now land: rivet bundle (#266), rivet coverage --matrix (#243), s-expr linked-via operator (#265). Plus externals load their own schemas (#267) and STPA TCL numbering is corrected to ISO 26262-8 (#257). Infrastructure: CI concurrency control across all workflows (#258), migration to self-hosted smithy runners (#262), release-npm trigger fix that retroactively unblocked v0.7.0/v0.8.0 npm publication (#261), weekly dependabot (#216), and the wasmtime 42→43 upgrade that retires the RUSTSEC-2026-0114 suppression introduced in v0.8.0 (#260). #125 (provenance-lifecycle) intentionally deferred — 5-week-old branch with conflicts in heavily-churned files (CLAUDE.md, ci.yml, settings). Needs its own attention session, not safe to autonomously rebase. Refs: FEAT-001 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 #245 — when a downstream
rivet.yamldeclares an external (e.g. sigil declares synth), the loader pulled in the external's artifacts but consulted only the downstream's schemas during type-checking. Any artifact whosetypelived in the external's schemas but not the downstream's emittedERROR: unknown artifact type— 326 errors in the sigil-with-synth repro from the issue body, drowning out 54 real lifecycle gaps.This PR closes the gap by loading each external's own schemas alongside its artifacts and consulting them under the external's
prefix:namespace, so external-prefixed artifacts type-check against the schemas they were authored under.Picked up per the 2026-05-09 triage comment ("acceptance criteria are clear … queued for next run … criteria are concrete enough for direct pickup").
What changed
rivet-core/src/externals.rs—load_external_projectnow also reads the external'sproject.schemaslist and loads it into aSchema, returned as(Vec<Artifact>, Option<Schema>).ResolvedExternalcarries the schema via a newschema: Option<Schema>field.Noneis the permissive-fallback (external declared no schemas, or they failed to load).rivet-core/src/validate.rs— newExternalSchemasmap (prefix -> Option<Schema>) andvalidate_with_externals/validate_structural_with_externalsentry points that consultexternals[prefix]first when an artifact id is<prefix>:<id>, falling through to the downstream schema. The unknown-link-type check is symmetrically extended. The originalvalidate/validate_structuralare now thin wrappers that delegate with an empty externals map (backwards-compatible for snapshot/export/proofs/db callers).reclassify_externals_diagnostics— post-pass for the salsa validation path, which doesn't yet take per-external schemas as a tracked input. Reclassifiesknown-typeandunknown-link-typediagnostics on prefixed artifacts so the salsa output converges withvalidate_with_externals. The--directand salsa paths now produce identical diagnostic sets.rivet-cli/src/main.rs—ProjectContextgains anexternal_schemasfield populated duringload.cmd_validate(--directusesvalidate_with_externals; default uses salsa + reclassify),cmd_stats,cmd_diff,cmd_context, and the dashboard export all switch to the externals-aware variants so their counts agree.Some(prefix) -> Noneand unknown-type / unknown-link-type findings for that prefix are demoted from ERROR to INFO with a message naming the external.Acceptance — per the 2026-05-09 triage comment on #245
rivet.yamlschemas-path/schemaslist into the type-check namespace —load_external_projectreadsconfig.project.schemasand callsload_schemasagainst<external>/schemas/(with the same on-disk → embedded fallback the downstream uses). Verified byload_all_externals_populates_schemaintests/externals_schemas.rs.prefix(no namespace collision with downstream-owned types of the same name) —lookup_typekeys on the<prefix>:<id>shape and consultsexternals[prefix]first. Downstream-owned types of the same name remain authoritative for non-prefixed artifacts.validate_structural_with_externalsresolves the type-def from the external's schema when a prefix matches; the rest of structural validation (required-fields, allowed-values, link cardinality, link target types) uses that resolved type-def. Verified end-to-end byexternal_schemas_eliminate_spurious_unknown_type_errors.unknown artifact typefrom ERROR to INFO for that external's prefix only — covered by both the inline path (TypeLookup::UnknownExternalNoSchema) and the salsa post-pass; verified byexternal_without_schema_demotes_unknown_type_to_info.external_schemas_eliminate_spurious_unknown_type_errorspins the pre-fix behaviour by running the externals-unaware path on the same store and asserting the spurious ERRORs appear, then runs the externals-aware path and asserts they're gone. The spar fixture'sschemas: [common, aadl]is exactly the shape the AC describes.pulseengine/rivet-only scope. The in-repo reproducer (spar fixture, typeaadl-componentnot in downstream[common]) collapses the spurious-error class to zero, which is the same code path. Sigil maintainer can confirm by re-runningrivet validateagainst this PR.Implements: REQ-007(CLI / externals) — present, plusVerifies: REQ-007for the new tests.Test plan
cargo test -p rivet-core— 920 lib + 4 new inexternals_schemas+ every other integration suite green; previously-failingcli_commands::stats_json_counts_match_validatenow passes (stats and validate counts converge once stats also uses externals-aware validation).cargo test -p rivet-cli— full suite green.cargo clippy --workspace --all-targets -- -D warnings— clean (only the pre-existing MSRV-mismatch warning at workspace root).cargo fmt --all -- --check— clean.cargo run --release -p rivet-cli -- --project /home/user/rivet validate—Result: FAIL (6 errors, 140 warnings, 0 broken cross-refs)— byte-identical to the pristinemainbaseline. The 6 ERRORs are the spar fixture's missing-required-field findings onaadl-componentartifacts, which are now reachable because the type successfully resolves through the external schema (previously suppressed by the unknown-type early-return).validate,validate --direct,stats) agree onerrors: 6 / warnings: 140 / infos: 124against the rivet repo, eliminating the prior salsa-vs-direct divergence.Out of scope
reclassify_externals_diagnosticspost-pass. The post-pass is byte-identical in output, but architecturally cleaner would be a newExternalSchemaSetsalsa input. Deferred — the post-pass is small, contained, and tested.external-anchorsemantics but live behind the broader externals-of-externals story; out of Externals: load each external repo's schemas alongside its artifacts #245.🤖 Generated with Claude Code — issue-triage agent run 2026-05-10.
Generated by Claude Code