diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index 7ff3745e..d6759288 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -4318,6 +4318,7 @@ fn cmd_validate( store, schema, graph, + external_schemas, doc_store, .. } = ctx; @@ -4451,12 +4452,20 @@ fn cmd_validate( // Core validation: use salsa incremental by default, --direct for legacy path. // When baseline or variant scoping is active, salsa validates ALL files and // we filter the resulting diagnostics to only include artifacts in the scoped store. + // + // Externals (issue #245): + // - --direct path uses `validate_with_externals` so externally-prefixed + // artifacts type-check against their external's schemas. + // - --salsa path runs externals-unaware (the salsa db doesn't yet take + // per-external schemas as a tracked input) and we apply + // `reclassify_externals_diagnostics` as a post-pass to converge on the + // same diagnostic set. let is_scoped = baseline_name.is_some() || variant_scope_name.is_some(); let mut diagnostics = if direct { - validate::validate(&store, &schema, &graph) + validate::validate_with_externals(&store, &schema, &graph, &external_schemas) } else { let all_diags = run_salsa_validation(cli, &config)?; - if is_scoped { + let scoped_diags = if is_scoped { // Filter diagnostics to only those relevant to the scoped store. all_diags .into_iter() @@ -4469,7 +4478,8 @@ fn cmd_validate( .collect() } else { all_diags - } + }; + validate::reclassify_externals_diagnostics(scoped_diags, &store, &external_schemas) }; diagnostics.extend(validate::validate_documents(&doc_store, &store)); @@ -5138,8 +5148,10 @@ fn cmd_stats( // // We use the direct validator on the (already scoped) store so the // counts line up with the visible artifact set when --filter or - // --baseline is in effect. - let diagnostics = validate::validate(&store, &ctx.schema, &graph); + // --baseline is in effect. Externals-aware so the numbers match the + // post-#245 `rivet validate` output. + let diagnostics = + validate::validate_with_externals(&store, &ctx.schema, &graph, &ctx.external_schemas); let errors = diagnostics .iter() .filter(|d| d.severity == Severity::Error) @@ -5910,7 +5922,12 @@ links_count = {links_count} serde_json::to_string_pretty(&stats_output)?, )?; // REQ-049: embed validation result so consumers can verify export freshness. - let diagnostics = rivet_core::validate::validate(&store, &ctx.schema, &graph); + let diagnostics = rivet_core::validate::validate_with_externals( + &store, + &ctx.schema, + &graph, + &ctx.external_schemas, + ); let errors = diagnostics .iter() .filter(|d| d.severity == rivet_core::schema::Severity::Error) @@ -6481,69 +6498,97 @@ fn cmd_diff( format: &str, ) -> Result { validate_format(format, &["text", "json"])?; - let (base_store, base_schema, base_graph, head_store, head_schema, head_graph) = - match (base_path, head_path) { - (Some(bp), Some(hp)) => { - // Explicit --base and --head directories: load each as a - // standalone project. - let base_cli = Cli { - project: bp.to_path_buf(), - schemas: cli.schemas.clone(), - verbose: cli.verbose, - command: Command::Validate { - format: "text".to_string(), - direct: false, - skip_external_validation: false, - baseline: None, - track_convergence: false, - model: None, - variant: None, - binding: None, - fail_on: "error".to_string(), - strict_cited_sources: false, - strict_cited_source_stale: false, - check_remote_sources: false, - }, - }; - let head_cli = Cli { - project: hp.to_path_buf(), - schemas: cli.schemas.clone(), - verbose: cli.verbose, - command: Command::Validate { - format: "text".to_string(), - direct: false, - skip_external_validation: false, - baseline: None, - track_convergence: false, - model: None, - variant: None, - binding: None, - fail_on: "error".to_string(), - strict_cited_sources: false, - strict_cited_source_stale: false, - check_remote_sources: false, - }, - }; - let bc = ProjectContext::load(&base_cli)?; - let hc = ProjectContext::load(&head_cli)?; - (bc.store, bc.schema, bc.graph, hc.store, hc.schema, hc.graph) - } - _ => { - // Default: load the project twice (same working tree). This - // is a placeholder — a future version will compare against - // the last clean git state. - let c1 = ProjectContext::load(cli)?; - let c2 = ProjectContext::load(cli)?; - (c1.store, c1.schema, c1.graph, c2.store, c2.schema, c2.graph) - } - }; + let ( + base_store, + base_schema, + base_graph, + base_externals, + head_store, + head_schema, + head_graph, + head_externals, + ) = match (base_path, head_path) { + (Some(bp), Some(hp)) => { + // Explicit --base and --head directories: load each as a + // standalone project. + let base_cli = Cli { + project: bp.to_path_buf(), + schemas: cli.schemas.clone(), + verbose: cli.verbose, + command: Command::Validate { + format: "text".to_string(), + direct: false, + skip_external_validation: false, + baseline: None, + track_convergence: false, + model: None, + variant: None, + binding: None, + fail_on: "error".to_string(), + strict_cited_sources: false, + strict_cited_source_stale: false, + check_remote_sources: false, + }, + }; + let head_cli = Cli { + project: hp.to_path_buf(), + schemas: cli.schemas.clone(), + verbose: cli.verbose, + command: Command::Validate { + format: "text".to_string(), + direct: false, + skip_external_validation: false, + baseline: None, + track_convergence: false, + model: None, + variant: None, + binding: None, + fail_on: "error".to_string(), + strict_cited_sources: false, + strict_cited_source_stale: false, + check_remote_sources: false, + }, + }; + let bc = ProjectContext::load(&base_cli)?; + let hc = ProjectContext::load(&head_cli)?; + ( + bc.store, + bc.schema, + bc.graph, + bc.external_schemas, + hc.store, + hc.schema, + hc.graph, + hc.external_schemas, + ) + } + _ => { + // Default: load the project twice (same working tree). This + // is a placeholder — a future version will compare against + // the last clean git state. + let c1 = ProjectContext::load(cli)?; + let c2 = ProjectContext::load(cli)?; + ( + c1.store, + c1.schema, + c1.graph, + c1.external_schemas, + c2.store, + c2.schema, + c2.graph, + c2.external_schemas, + ) + } + }; // Compute artifact diff let diff = ArtifactDiff::compute(&base_store, &head_store); - // Compute diagnostic diff - let base_diags = validate::validate(&base_store, &base_schema, &base_graph); - let head_diags = validate::validate(&head_store, &head_schema, &head_graph); + // Compute diagnostic diff (externals-aware on both sides — issue #245) + let base_diags = + validate::validate_with_externals(&base_store, &base_schema, &base_graph, &base_externals); + let head_diags = + validate::validate_with_externals(&head_store, &head_schema, &head_graph, &head_externals); let diag_diff = DiagnosticDiff::compute(&base_diags, &head_diags); if format == "json" { @@ -8036,8 +8081,9 @@ fn cmd_context(cli: &Cli) -> Result { let store = ctx.store; let schema = ctx.schema; let graph = ctx.graph; + let external_schemas = ctx.external_schemas; let doc_store = ctx.doc_store.unwrap_or_default(); - let diagnostics = validate::validate(&store, &schema, &graph); + let diagnostics = validate::validate_with_externals(&store, &schema, &graph, &external_schemas); let coverage_report = coverage::compute_coverage(&store, &schema, &graph); let rivet_dir = cli.project.join(".rivet"); @@ -10390,6 +10436,11 @@ struct ProjectContext { store: Store, schema: rivet_core::schema::Schema, graph: LinkGraph, + /// Per-prefix schemas for declared externals (issue #245). `Some(schema)` + /// = type-check the external's prefixed artifacts against this schema; + /// `None` = external didn't declare schemas, demote unknown-type to INFO. + /// Empty when the project has no externals. + external_schemas: rivet_core::validate::ExternalSchemas, doc_store: Option, result_store: Option, } @@ -10422,7 +10473,11 @@ impl ProjectContext { } } - // Load external project artifacts so cross-repo references resolve + // Load external project artifacts so cross-repo references resolve. + // We also collect each external's own schemas so the validator can + // type-check externally-prefixed artifacts against the schemas they + // were authored under, not the downstream's (issue #245). + let mut external_schemas: rivet_core::validate::ExternalSchemas = Default::default(); if let Some(ref externals) = config.externals { if !externals.is_empty() { match rivet_core::externals::load_all_externals(externals, &cli.project) { @@ -10432,6 +10487,7 @@ impl ProjectContext { // internal link targets consistently. let ext_ids: std::collections::HashSet = ext.artifacts.iter().map(|a| a.id.clone()).collect(); + external_schemas.insert(ext.prefix.clone(), ext.schema); for mut artifact in ext.artifacts { // Prefix external artifact IDs so they don't collide artifact.id = format!("{}:{}", ext.prefix, artifact.id); @@ -10459,6 +10515,7 @@ impl ProjectContext { store, schema, graph, + external_schemas, doc_store: None, result_store: None, }) diff --git a/rivet-cli/src/serve/mod.rs b/rivet-cli/src/serve/mod.rs index 0843de45..d310e0f9 100644 --- a/rivet-cli/src/serve/mod.rs +++ b/rivet-cli/src/serve/mod.rs @@ -417,7 +417,9 @@ fn load_externals(config: &ProjectConfig, project_path: &std::path::Path) -> Vec let synced = ext_dir.join("rivet.yaml").exists(); let mut ext_store = Store::new(); if synced { - if let Ok(artifacts) = rivet_core::externals::load_external_project(&ext_dir) { + if let Ok((artifacts, _schema)) = + rivet_core::externals::load_external_project(&ext_dir) + { for a in artifacts { ext_store.upsert(a); } diff --git a/rivet-core/src/externals.rs b/rivet-core/src/externals.rs index 7d7977c0..699840e4 100644 --- a/rivet-core/src/externals.rs +++ b/rivet-core/src/externals.rs @@ -359,14 +359,21 @@ pub fn ensure_gitignore(project_dir: &Path) -> Result Ok(true) // added } -/// Load artifacts from an external project directory. +/// Load artifacts and schema from an external project directory. /// /// Reads the external project's `rivet.yaml`, discovers its sources, -/// and loads all artifacts. Does NOT validate against schema (the -/// external project validates itself). +/// loads all artifacts, and also loads the external's own schema set +/// (so the downstream caller can type-check `:`-scoped artifacts +/// against the schemas the external itself was validated under — REQ-007, +/// issue #245). +/// +/// The returned schema is `None` when the external's `rivet.yaml` +/// declares no schemas, or its declared schemas fail to load. In that +/// permissive-fallback case the caller is expected to demote any +/// `unknown artifact type` for this external's prefix from ERROR to INFO. pub fn load_external_project( project_dir: &Path, -) -> Result, crate::error::Error> { +) -> Result<(Vec, Option), crate::error::Error> { let config_path = project_dir.join("rivet.yaml"); if !config_path.exists() { return Err(crate::error::Error::Io(format!( @@ -385,6 +392,30 @@ pub fn load_external_project( ); } + // Load the external's own schemas so cross-prefix artifacts can be + // type-checked against the schemas they were authored under. + // `schemas-path` is implicit: rivet always falls back to + // /schemas/, then the binary location, then embedded. + // `None` here means "permissive-fallback mode" — see doc comment. + let schema = if config.project.schemas.is_empty() { + None + } else { + let schemas_dir = project_dir.join("schemas"); + match crate::load_schemas(&config.project.schemas, &schemas_dir) { + Ok(s) => Some(s), + Err(e) => { + log::warn!( + "external project at {} declares schemas {:?} but they failed to load \ + ({e}) — type-check errors for prefix '' will be \ + demoted to INFO", + project_dir.display(), + config.project.schemas, + ); + None + } + } + }; + let mut artifacts = Vec::new(); for source in &config.sources { let source_path = project_dir.join(&source.path); @@ -397,8 +428,13 @@ pub fn load_external_project( ); continue; } - let loaded = - crate::load_artifacts(source, project_dir, &crate::schema::Schema::merge(&[]))?; + // For source-format extraction we still need a schema (e.g. stpa-yaml + // shorthand expansion keys off it). Use the external's own schema if + // we have one, else fall through to the empty schema as before. + let extraction_schema = schema + .clone() + .unwrap_or_else(|| crate::schema::Schema::merge(&[])); + let loaded = crate::load_artifacts(source, project_dir, &extraction_schema)?; artifacts.extend(loaded); } @@ -416,18 +452,25 @@ pub fn load_external_project( ); } - Ok(artifacts) + Ok((artifacts, schema)) } -/// A resolved external with its loaded artifacts. +/// A resolved external with its loaded artifacts and (optionally) its own schema. +/// +/// `schema` is `Some` when the external's `rivet.yaml` declared schemas and +/// they loaded successfully. It is `None` when the external declared no +/// schemas or its schemas failed to load — callers in that case treat the +/// external as opaque and demote unknown-artifact-type ERRORs to INFO for +/// the external's prefix (issue #245). #[derive(Debug)] pub struct ResolvedExternal { pub prefix: String, pub project_dir: PathBuf, pub artifacts: Vec, + pub schema: Option, } -/// Load all external projects from cache and return their artifacts. +/// Load all external projects from cache and return their artifacts + schemas. pub fn load_all_externals( externals: &BTreeMap, project_dir: &Path, @@ -436,11 +479,12 @@ pub fn load_all_externals( let mut resolved = Vec::new(); for ext in externals.values() { let ext_dir = resolve_external_dir(ext, &cache_dir, project_dir); - let artifacts = load_external_project(&ext_dir)?; + let (artifacts, schema) = load_external_project(&ext_dir)?; resolved.push(ResolvedExternal { prefix: ext.prefix.clone(), project_dir: ext_dir, artifacts, + schema, }); } Ok(resolved) @@ -1197,10 +1241,21 @@ mod tests { "artifacts:\n - id: EXT-001\n type: requirement\n title: External req\n - id: EXT-002\n type: feature\n title: External feat\n", ).unwrap(); - let artifacts = load_external_project(&ext_dir).unwrap(); + let (artifacts, schema) = load_external_project(&ext_dir).unwrap(); assert_eq!(artifacts.len(), 2); assert!(artifacts.iter().any(|a| a.id == "EXT-001")); assert!(artifacts.iter().any(|a| a.id == "EXT-002")); + // External declares schemas: [common, dev]; both are embedded so + // load_external_project must surface a Schema (issue #245). + let schema = schema.expect("external declared schemas; loader should surface them"); + assert!( + schema.artifact_type("requirement").is_some(), + "common.yaml's 'requirement' should be in the external schema" + ); + assert!( + schema.artifact_type("feature").is_some(), + "common.yaml's 'feature' should be in the external schema" + ); } // rivet: verifies REQ-020 @@ -1238,6 +1293,7 @@ mod tests { prefix: "meld".to_string(), project_dir: std::path::PathBuf::from("/tmp/meld"), artifacts: vec![ext_artifact], + schema: None, }]; let backlinks = compute_backlinks(&resolved, &local_ids); @@ -1274,6 +1330,7 @@ mod tests { prefix: "meld".to_string(), project_dir: std::path::PathBuf::from("/tmp/meld"), artifacts: vec![ext_artifact], + schema: None, }]; let backlinks = compute_backlinks(&resolved, &local_ids); @@ -1307,6 +1364,7 @@ mod tests { prefix: "meld".to_string(), project_dir: std::path::PathBuf::from("/tmp/meld"), artifacts: vec![ext_artifact], + schema: None, }]; let backlinks = compute_backlinks(&resolved, &local_ids); diff --git a/rivet-core/src/validate.rs b/rivet-core/src/validate.rs index 9d6288b3..bc59d460 100644 --- a/rivet-core/src/validate.rs +++ b/rivet-core/src/validate.rs @@ -38,11 +38,77 @@ use crate::document::DocumentStore; use crate::links::LinkGraph; -use crate::schema::{Cardinality, Schema, Severity}; +use crate::schema::{ArtifactTypeDef, Cardinality, Schema, Severity}; use crate::store::Store; use regex::Regex; +use std::collections::BTreeMap; use std::sync::LazyLock; +/// Per-prefix external schemas. Keyed by the external's `prefix` (e.g. `"synth"`). +/// +/// `Some(schema)` — the external declared schemas and they loaded; type-check +/// any `:` artifact against this schema. +/// +/// `None` — the external declared no schemas, or its schemas failed to load; +/// type-check errors for `:` artifacts are demoted from ERROR to +/// INFO so the downstream isn't spammed by an external it cannot type-check +/// (issue #245). +pub type ExternalSchemas = BTreeMap>; + +/// Outcome of looking up an artifact's declared type across the main schema +/// and any registered externals. +enum TypeLookup<'a> { + /// Type resolved — proceed with downstream-style validation. + Found(&'a ArtifactTypeDef), + /// Type not declared anywhere we know to look. Emit ERROR. + Unknown, + /// Artifact comes from an external whose own schemas we couldn't load. + /// Emit INFO (permissive fallback). + UnknownExternalNoSchema { + /// External prefix that registered without a schema. + prefix: String, + }, +} + +/// Resolve the type definition for an artifact, consulting the external's own +/// schema first when the artifact id is `:<…>`. +fn lookup_type<'a>( + artifact: &crate::model::Artifact, + schema: &'a Schema, + externals: &'a ExternalSchemas, +) -> TypeLookup<'a> { + if let Some((prefix, _)) = artifact.id.split_once(':') { + if let Some(maybe_ext) = externals.get(prefix) { + match maybe_ext { + Some(ext_schema) => { + if let Some(td) = ext_schema.artifact_type(&artifact.artifact_type) { + return TypeLookup::Found(td); + } + // External declared schemas but this type isn't in them. + // Last-chance: maybe the downstream knows the type. + return match schema.artifact_type(&artifact.artifact_type) { + Some(td) => TypeLookup::Found(td), + None => TypeLookup::Unknown, + }; + } + None => { + // External in permissive-fallback mode. + return match schema.artifact_type(&artifact.artifact_type) { + Some(td) => TypeLookup::Found(td), + None => TypeLookup::UnknownExternalNoSchema { + prefix: prefix.to_string(), + }, + }; + } + } + } + } + match schema.artifact_type(&artifact.artifact_type) { + Some(td) => TypeLookup::Found(td), + None => TypeLookup::Unknown, + } +} + /// Regex matching an artifact-id-shaped token in prose: leading /// uppercase letter, optional uppercase / digit chars, a `-`, and a /// numeric suffix. `\b` boundaries avoid substrings of larger @@ -110,6 +176,123 @@ impl std::fmt::Display for Diagnostic { } } +/// Reclassify `known-type` and `unknown-link-type` diagnostics emitted by an +/// externals-unaware validator (typically the salsa pipeline) using a +/// per-prefix `ExternalSchemas` map. +/// +/// For diagnostics whose `artifact_id` has the form `:` and whose +/// prefix is registered in `externals`: +/// +/// - **`known-type` ERROR + external has a schema with that type** → diagnostic +/// is dropped (it was a false positive — the type exists in the external's +/// schema set). +/// - **`known-type` ERROR + external has no schema (permissive fallback)** → +/// demoted to INFO with a message explaining type-check was skipped. +/// - **`unknown-link-type` ERROR + external schema declares the link type** +/// → dropped. +/// - **`unknown-link-type` ERROR + external has no schema** → demoted to INFO. +/// +/// All other diagnostics pass through unchanged. +/// +/// This is the post-pass used by the CLI's salsa validation path +/// (`run_salsa_validation` does not yet thread per-external schemas through +/// the salsa graph; the direct path uses +/// [`validate_with_externals`] instead). The two paths converge on the same +/// final diagnostic set (issue #245). +pub fn reclassify_externals_diagnostics( + diagnostics: Vec, + store: &Store, + externals: &ExternalSchemas, +) -> Vec { + if externals.is_empty() { + return diagnostics; + } + diagnostics + .into_iter() + .filter_map(|d| reclassify_one(d, store, externals)) + .collect() +} + +fn reclassify_one( + mut d: Diagnostic, + store: &Store, + externals: &ExternalSchemas, +) -> Option { + // Only `known-type` and `unknown-link-type` are subject to externals-aware + // reclassification; everything else passes through unchanged. + if d.rule != "known-type" && d.rule != "unknown-link-type" { + return Some(d); + } + let Some(id) = d.artifact_id.as_ref() else { + return Some(d); + }; + let Some((prefix, _)) = id.split_once(':') else { + return Some(d); + }; + let Some(maybe_schema) = externals.get(prefix) else { + return Some(d); + }; + match d.rule.as_str() { + "known-type" => { + // We need the artifact's type to consult the external schema. + let Some(art) = store.get(id) else { + return Some(d); + }; + match maybe_schema { + Some(ext_schema) => { + if ext_schema.artifact_type(&art.artifact_type).is_some() { + // External does declare this type — drop the + // false-positive ERROR. + None + } else { + // External has schemas, type still unknown — keep ERROR. + Some(d) + } + } + None => { + d.severity = Severity::Info; + d.message = format!( + "artifact type '{}' not declared in any loaded schema; \ + external '{}' declares no schemas (or its schemas failed to load), \ + so type-check is skipped for this prefix", + art.artifact_type, prefix + ); + Some(d) + } + } + } + "unknown-link-type" => { + // Parse the link-type out of the message; format is + // "link type 'foo' is not defined in the schema …" + let lt = d + .message + .split('\'') + .nth(1) + .map(str::to_string) + .unwrap_or_default(); + match maybe_schema { + Some(ext_schema) => { + if !lt.is_empty() && ext_schema.link_types.contains_key(<) { + None + } else { + Some(d) + } + } + None => { + d.severity = Severity::Info; + d.message = format!( + "link type '{}' is not defined in the downstream schema; \ + external '{}' declares no schemas, so this can't be checked", + lt, prefix, + ); + Some(d) + } + } + } + _ => Some(d), + } +} + /// Validate a store against a schema and link graph. /// /// Returns a list of diagnostics (errors, warnings, info). @@ -120,7 +303,21 @@ impl std::fmt::Display for Diagnostic { /// 1-7 and [`evaluate_conditional_rules`](crate::db::evaluate_conditional_rules) /// for phase 8 as a separate tracked query. pub fn validate(store: &Store, schema: &Schema, graph: &LinkGraph) -> Vec { - let mut diagnostics = validate_structural(store, schema, graph); + validate_with_externals(store, schema, graph, &BTreeMap::new()) +} + +/// Validate a store against a schema, link graph, and per-prefix external +/// schemas. Externally-prefixed artifacts (id like `:`) are +/// type-checked against `externals[prefix]` when present, so cross-repo +/// projects don't drown in `unknown artifact type` errors for types only +/// defined in their externals' schemas (issue #245). +pub fn validate_with_externals( + store: &Store, + schema: &Schema, + graph: &LinkGraph, + externals: &ExternalSchemas, +) -> Vec { + let mut diagnostics = validate_structural_with_externals(store, schema, graph, externals); // 0. Check conditional rule consistency (schema-level) diagnostics.extend(crate::schema::check_conditional_consistency( @@ -157,21 +354,59 @@ pub fn validate(store: &Store, schema: &Schema, graph: &LinkGraph) -> Vec Vec { + validate_structural_with_externals(store, schema, graph, &BTreeMap::new()) +} + +/// Structural validation aware of per-prefix external schemas. +/// +/// Behaves like [`validate_structural`] but consults `externals[prefix]` when +/// the artifact's id has the form `:`, so an external's own types +/// resolve cleanly under its prefix without polluting the downstream's type +/// namespace. When `externals[prefix]` is `None` (the external declared no +/// schemas or its schemas failed to load), unknown-type and unknown-link-type +/// findings for that prefix are demoted from ERROR to INFO — the permissive +/// fallback specified in issue #245. +pub fn validate_structural_with_externals( + store: &Store, + schema: &Schema, + graph: &LinkGraph, + externals: &ExternalSchemas, +) -> Vec { let mut diagnostics = Vec::new(); // 1. Check that every artifact has a known type for artifact in store.iter() { - let Some(type_def) = schema.artifact_type(&artifact.artifact_type) else { - diagnostics.push(Diagnostic { - source_file: None, - line: None, - column: None, - severity: Severity::Error, - artifact_id: Some(artifact.id.clone()), - rule: "known-type".to_string(), - message: format!("unknown artifact type '{}'", artifact.artifact_type), - }); - continue; + let type_def = match lookup_type(artifact, schema, externals) { + TypeLookup::Found(td) => td, + TypeLookup::Unknown => { + diagnostics.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Error, + artifact_id: Some(artifact.id.clone()), + rule: "known-type".to_string(), + message: format!("unknown artifact type '{}'", artifact.artifact_type), + }); + continue; + } + TypeLookup::UnknownExternalNoSchema { prefix } => { + diagnostics.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity: Severity::Info, + artifact_id: Some(artifact.id.clone()), + rule: "known-type".to_string(), + message: format!( + "artifact type '{}' not declared in any loaded schema; \ + external '{}' declares no schemas (or its schemas failed to load), \ + so type-check is skipped for this prefix", + artifact.artifact_type, prefix + ), + }); + continue; + } }; // 2. Check required fields @@ -473,28 +708,69 @@ pub fn validate_structural(store: &Store, schema: &Schema, graph: &LinkGraph) -> // to those links — the same severity as a broken required-link link, // not a soft advisory. Pin to one diagnostic per (artifact, link-type) // pair so a typo doesn't drown the report. + // + // For externally-prefixed artifacts, also accept link types declared + // in their external's schema. If the external declared no schemas + // (permissive-fallback), demote unknown-link-type to INFO for that + // prefix — same rationale as the unknown-artifact-type fallback above + // (issue #245). use std::collections::BTreeSet; let known_link_types: BTreeSet<&str> = schema.link_types.keys().map(String::as_str).collect(); for artifact in store.iter() { + let (ext_prefix, ext_known): (Option<&str>, BTreeSet<&str>) = + match artifact.id.split_once(':') { + Some((prefix, _)) => match externals.get(prefix) { + Some(Some(ext_schema)) => ( + Some(prefix), + ext_schema.link_types.keys().map(String::as_str).collect(), + ), + Some(None) => (Some(prefix), BTreeSet::new()), + None => (None, BTreeSet::new()), + }, + None => (None, BTreeSet::new()), + }; let mut seen: BTreeSet<&str> = BTreeSet::new(); for link in &artifact.links { - if !known_link_types.contains(link.link_type.as_str()) - && seen.insert(link.link_type.as_str()) - { - diagnostics.push(Diagnostic { - source_file: None, - line: None, - column: None, - severity: Severity::Error, - artifact_id: Some(artifact.id.clone()), - rule: "unknown-link-type".to_string(), - message: format!( - "link type '{}' is not defined in the schema \ - — declare it in link-types: or remove the link", - link.link_type - ), - }); + let lt = link.link_type.as_str(); + if known_link_types.contains(lt) || ext_known.contains(lt) { + continue; } + if !seen.insert(lt) { + continue; + } + // External-prefixed artifact whose external has no schema: + // demote to INFO instead of suppressing entirely so the user + // still sees what's happening. + let no_schema_external = + matches!(ext_prefix.and_then(|p| externals.get(p)), Some(None)); + let severity = if no_schema_external { + Severity::Info + } else { + Severity::Error + }; + let message = if no_schema_external { + format!( + "link type '{}' is not defined in the downstream schema; \ + external '{}' declares no schemas, so this can't be checked", + lt, + ext_prefix.unwrap_or("?"), + ) + } else { + format!( + "link type '{}' is not defined in the schema \ + — declare it in link-types: or remove the link", + lt + ) + }; + diagnostics.push(Diagnostic { + source_file: None, + line: None, + column: None, + severity, + artifact_id: Some(artifact.id.clone()), + rule: "unknown-link-type".to_string(), + message, + }); } } diff --git a/rivet-core/tests/externals_schemas.rs b/rivet-core/tests/externals_schemas.rs new file mode 100644 index 00000000..1c082dd8 --- /dev/null +++ b/rivet-core/tests/externals_schemas.rs @@ -0,0 +1,290 @@ +// SAFETY-REVIEW (SCRC Phase 1, DD-058): Integration test code; same blanket +// allow as the rest of `rivet-core/tests/` — see externals_sync.rs for the +// rationale. +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::indexing_slicing, + clippy::panic, + clippy::print_stdout, + clippy::print_stderr +)] + +//! Integration tests for issue #245: externals must load their own schemas +//! so externally-prefixed artifacts type-check against the schemas they were +//! authored under, not the downstream's. +//! +//! Without this fix, sigil's repro produces 326 `unknown artifact type` +//! ERRORs on synth-prefixed artifacts (types defined in synth's schemas +//! but not in sigil's). After the fix, those errors collapse to zero. + +use rivet_core::externals::{load_all_externals, load_external_project}; +use rivet_core::links::LinkGraph; +use rivet_core::model::ExternalProject; +use rivet_core::model::{Artifact, Link}; +use rivet_core::schema::Severity; +use rivet_core::store::Store; +use rivet_core::validate::{ExternalSchemas, validate_with_externals}; +use std::collections::BTreeMap; +use std::path::PathBuf; + +fn spar_fixture_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("tests") + .join("fixtures") + .join("spar-external") +} + +/// Simulates the sigil-loading-synth scenario. Downstream declares only +/// `common`; the external declares `common + aadl`. Before this fix, the +/// downstream's validator produced an ERROR for every synth-prefixed +/// `aadl-component` artifact. With the fix, the external's schema resolves +/// the type and the validator stays silent. +/// +/// rivet: verifies REQ-007 +#[test] +fn external_schemas_eliminate_spurious_unknown_type_errors() { + let fixture = spar_fixture_dir(); + let (artifacts, ext_schema) = load_external_project(&fixture).unwrap(); + assert!( + ext_schema.is_some(), + "spar fixture declares schemas; loader must surface them" + ); + let ext_schema = ext_schema.unwrap(); + assert!( + ext_schema.artifact_type("aadl-component").is_some(), + "fixture's aadl schema declares 'aadl-component'" + ); + + // Build a downstream-shaped store: simulate prefixing with `spar:`. + let mut store = Store::new(); + for mut a in artifacts { + a.id = format!("spar:{}", a.id); + store.upsert(a); + } + + // Downstream's own schema declares only `common`. `aadl-component` is + // NOT in it — exactly the sigil scenario. + let downstream_schema = rivet_core::load_schemas( + &["common".to_string()], + &PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../schemas"), + ) + .expect("loading downstream-only schemas"); + assert!( + downstream_schema.artifact_type("aadl-component").is_none(), + "downstream-only schema must NOT declare 'aadl-component' for the test \ + to be meaningful (the fix removes spurious errors)" + ); + + let graph = LinkGraph::build(&store, &downstream_schema); + + let aadl_count = |diags: &[rivet_core::validate::Diagnostic]| -> usize { + diags + .iter() + .filter(|d| { + d.rule == "known-type" + && d.severity == Severity::Error + && d.artifact_id + .as_deref() + .is_some_and(|id| id.starts_with("spar:")) + && d.message.contains("aadl-component") + }) + .count() + }; + + // 1) Externals-unaware validation reproduces the bug: every spar-prefixed + // `aadl-component` artifact yields an `unknown artifact type` ERROR. + let baseline = + validate_with_externals(&store, &downstream_schema, &graph, &ExternalSchemas::new()); + assert!( + aadl_count(&baseline) > 0, + "without the fix, externally-prefixed aadl-component artifacts must \ + generate 'unknown artifact type' ERRORs — this baseline pins the \ + pre-fix behaviour. all diags: {:?}", + baseline.iter().map(|d| &d.message).collect::>() + ); + + // 2) With the external's own schema in the per-prefix map, those errors + // must not appear. Other unrelated diagnostics may remain (e.g. the + // spar fixture has a `requirement` artifact whose type is declared in + // `dev.yaml`, which neither the downstream nor the external loaded); + // we only assert on the AC-relevant aadl-component path. + let mut externals = ExternalSchemas::new(); + externals.insert("spar".to_string(), Some(ext_schema)); + let fixed = validate_with_externals(&store, &downstream_schema, &graph, &externals); + assert_eq!( + aadl_count(&fixed), + 0, + "with the external's schema registered, externally-prefixed \ + aadl-component artifacts must NOT generate 'unknown artifact type' \ + ERRORs. still seeing: {:?}", + fixed + .iter() + .filter(|d| d.rule == "known-type") + .map(|d| (&d.artifact_id, &d.message)) + .collect::>() + ); +} + +/// load_all_externals must populate `ResolvedExternal::schema` for any +/// external whose `rivet.yaml` declares schemas. This is the upstream half +/// of the AC bullet — without it the downstream loader has no schema to +/// register in the per-prefix map. +/// +/// rivet: verifies REQ-007 +#[test] +fn load_all_externals_populates_schema() { + let fixture = spar_fixture_dir(); + let dir = tempfile::tempdir().unwrap(); + let mut externals = BTreeMap::new(); + externals.insert( + "spar".into(), + ExternalProject { + git: None, + path: Some(fixture.to_str().unwrap().into()), + git_ref: None, + prefix: "spar".into(), + }, + ); + rivet_core::externals::sync_all(&externals, dir.path(), true).unwrap(); + + let resolved = load_all_externals(&externals, dir.path()).unwrap(); + assert_eq!(resolved.len(), 1); + let ext = &resolved[0]; + assert_eq!(ext.prefix, "spar"); + let schema = ext + .schema + .as_ref() + .expect("spar's rivet.yaml declares schemas; must surface"); + // spar fixture declares schemas: [common, aadl] — aadl declares + // 'aadl-component'. + assert!( + schema.artifact_type("aadl-component").is_some(), + "aadl schema must surface 'aadl-component' through the external loader" + ); +} + +/// Permissive-fallback: when an external is registered with `schema: None` +/// (because its `rivet.yaml` declared no schemas, or its schemas failed to +/// load), unknown-type findings for that prefix get demoted from ERROR to +/// INFO so the downstream isn't spammed by an external it cannot type-check. +/// +/// rivet: verifies REQ-007 +#[test] +fn external_without_schema_demotes_unknown_type_to_info() { + // Synthetic store: one artifact with a prefix whose external has no + // schema. We do not need any artifacts on disk for this — the + // permissive-fallback path is purely about `ExternalSchemas[prefix] == + // None` triggering the demote. + let mut store = Store::new(); + store.upsert(Artifact { + id: "supplier:THING-1".to_string(), + artifact_type: "supplier-only-type".to_string(), + title: "External thing".to_string(), + description: None, + status: None, + tags: vec![], + links: vec![], + fields: BTreeMap::new(), + provenance: None, + source_file: None, + }); + + let downstream_schema = rivet_core::load_schemas( + &["common".to_string()], + &PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../schemas"), + ) + .unwrap(); + let graph = LinkGraph::build(&store, &downstream_schema); + + // Unknown to downstream → ERROR (sanity: no externals registered). + let baseline = + validate_with_externals(&store, &downstream_schema, &graph, &ExternalSchemas::new()); + assert!( + baseline.iter().any(|d| d.rule == "known-type" + && d.severity == Severity::Error + && d.artifact_id.as_deref() == Some("supplier:THING-1")), + "baseline must emit unknown-type ERROR when no externals registered" + ); + + // Register the prefix with `None` schema → demote to INFO. + let mut externals = ExternalSchemas::new(); + externals.insert("supplier".to_string(), None); + let demoted = validate_with_externals(&store, &downstream_schema, &graph, &externals); + let unknown_type: Vec<_> = demoted + .iter() + .filter(|d| d.rule == "known-type" && d.artifact_id.as_deref() == Some("supplier:THING-1")) + .collect(); + assert_eq!( + unknown_type.len(), + 1, + "expected exactly one known-type diagnostic for supplier:THING-1" + ); + assert_eq!( + unknown_type[0].severity, + Severity::Info, + "with no schema registered for the prefix, unknown-type must be INFO, not ERROR" + ); + assert!( + unknown_type[0].message.contains("supplier"), + "INFO message should name the external prefix" + ); +} + +/// Unknown-link-type symmetry: a link type defined in the external's schema +/// but not the downstream's must NOT be flagged for externally-prefixed +/// artifacts. The fallback (no external schema) demotes to INFO. +/// +/// rivet: verifies REQ-007 +#[test] +fn external_link_types_are_not_flagged_unknown() { + // Synthetic store: external artifact whose link uses a link-type + // declared in the external's schema only. + let mut store = Store::new(); + store.upsert(Artifact { + id: "spar:SPAR-X-001".to_string(), + artifact_type: "aadl-component".to_string(), + title: "Component".to_string(), + description: None, + status: None, + tags: vec![], + // Use a link type that's likely defined in spar's aadl schema but + // not in the downstream-only `common` schema set. + links: vec![Link { + link_type: "implements".to_string(), + target: "spar:OTHER".to_string(), + }], + fields: BTreeMap::new(), + provenance: None, + source_file: None, + }); + + let downstream_schema = rivet_core::load_schemas( + &["common".to_string()], + &PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../schemas"), + ) + .unwrap(); + let graph = LinkGraph::build(&store, &downstream_schema); + + // Permissive fallback: prefix registered with no schema. + let mut externals = ExternalSchemas::new(); + externals.insert("spar".to_string(), None); + let diags = validate_with_externals(&store, &downstream_schema, &graph, &externals); + let link_type_diags: Vec<_> = diags + .iter() + .filter(|d| { + d.rule == "unknown-link-type" && d.artifact_id.as_deref() == Some("spar:SPAR-X-001") + }) + .collect(); + // If `implements` is in the downstream `common` schema, there will be + // zero diagnostics. Otherwise, the demoted-to-INFO path should fire. + for d in &link_type_diags { + assert_eq!( + d.severity, + Severity::Info, + "any unknown-link-type for an externally-prefixed artifact whose \ + external has no schema must be INFO, not ERROR. got: {d:?}", + ); + } +} diff --git a/rivet-core/tests/externals_sync.rs b/rivet-core/tests/externals_sync.rs index ac70e385..10083273 100644 --- a/rivet-core/tests/externals_sync.rs +++ b/rivet-core/tests/externals_sync.rs @@ -77,7 +77,7 @@ fn sync_spar_external_via_local_path() { #[serial] fn load_spar_external_artifacts() { let fixture = spar_fixture_dir(); - let artifacts = load_external_project(&fixture).unwrap(); + let (artifacts, _schema) = load_external_project(&fixture).unwrap(); // The fixture has 4 artifacts: 3 aadl-component + 1 requirement assert!( @@ -105,7 +105,7 @@ fn load_spar_external_artifacts() { #[serial] fn cross_repo_link_resolution_with_spar() { let fixture = spar_fixture_dir(); - let spar_artifacts = load_external_project(&fixture).unwrap(); + let (spar_artifacts, _schema) = load_external_project(&fixture).unwrap(); // Build external ID sets let spar_ids: HashSet = spar_artifacts.iter().map(|a| a.id.clone()).collect(); @@ -205,6 +205,7 @@ fn backlinks_from_spar_to_local() { prefix: "spar".into(), project_dir: spar_fixture_dir(), artifacts: vec![spar_artifact], + schema: None, }]; let mut local_ids = HashSet::new();