diff --git a/CHANGELOG.md b/CHANGELOG.md index 09d33121..c8b7fb7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,16 @@ details, release history over commit history. scalar; a type name may not overwrite an existing `stats --json` key; and ill-typed `descriptions`, `starter_bodies`, and `guidance` values are skipped with `artifact-spec-skipped` instead of being admitted as empty. +- ADR-150 composition correctness: a type override's rationale resolves + across the declaring corpus instead of only the directory or `--top-level` + scope a command names, and ignores case as ADR-137 rationales do; two + declarations that differ only in JSON key order are one type instead of a + `corpus-federation-artifact-type-conflict`; the composed-registry memo is + keyed on the federation topology as well as the configs, so a long-running + process sees a changed parent graph; `decided new` and `decided migrate` in a + version-2 repository again count identifiers in sibling top-level + directories as issued; and `artifact_spec_bundles` reports `source: null`, + not the `prefer: local` keyword, for a local bundle without `corpus.source`. - `decided new` and `decided migrate` in a repository with a version-2 federation manifest, which failed with `federated-corpus-snapshot-failed` because the identifier-collision scan composed the repository root, a path diff --git a/decisions/designs/third-party-artifact-extensibility.md b/decisions/designs/third-party-artifact-extensibility.md index 51f9e6d1..d49336f7 100644 --- a/decisions/designs/third-party-artifact-extensibility.md +++ b/decisions/designs/third-party-artifact-extensibility.md @@ -322,8 +322,9 @@ memo: source). 3. Group candidates by name in order of first appearance. One distinct content per name is admitted as is; elements that compare equal as - admitted elements are silent duplicates, so key order and whitespace in a - bundle file cannot manufacture a conflict. Two or more distinct contents + admitted data are silent duplicates (map-like fields compare as maps, lists + in order, because list order is rendered order), so key order and + whitespace in a bundle file cannot manufacture a conflict. Two or more distinct contents are a collision: with no override at this source the composition stops with `corpus-federation-artifact-type-conflict`, naming the type and every declaring source; with an override, the candidate declared by `prefer` @@ -335,14 +336,23 @@ memo: family already uses for a malformed ADR-137 declaration. 5. The effective registry is the built-ins in registry order followed by the surviving elements in first-appearance order. The winner of a collision is - what descendants inherit; a descendant that declares yet another content - for the name collides afresh and needs its own override, mirroring the - override chains of ADR-147. + what descendants inherit through this source; a descendant that declares + yet another content for the name, or that reaches a losing declaration + again through another parent (a diamond), collides afresh and needs its + own override, mirroring the override chains and explicit diamond + convergence of ADR-147. A middle node's resolution governs its own view + only; it is not carried past a sibling path. **Rationale check.** `rationale` must resolve to exactly one live local Decision of the declaring source: one item whose origin is that source and -whose canonical identifier matches, classified `decision`, and live by the -same predicate the artifact overrides use. Items only exist after the +whose canonical identifier matches ignoring case (as ADR-137 rationales do), +classified `decision`, and live by the same predicate the artifact overrides +use. An inherited source's corpus is captured whole; the root's captured +files may be only the command's directory or `--top-level` scope, so a +root-owned rationale not found there is resolved over a walk of the root's +working tree with every materialised parent excluded. An override is +config-level policy, so its validity cannot depend on the directory a +command names. Items only exist after the closure's files are parsed, so this check runs immediately after parsing in both composition functions; a failure is `corpus-federation-invalid-override` and fails the composition like any other override defect. The registry is @@ -350,10 +360,13 @@ installed before parsing so inherited types classify; a failed rationale check stops the run before anything is served, so the ordering is not observable. -**Process slot.** Built registries are memoised by a key over every source's -`(source, config bytes)` in composition order, which fixes the pins and the -overrides; the local-only registry of section 1 uses the same key shape with -one frame. Bundle bytes are re-read and re-hashed against their pin on every +**Process slot.** Composed registries are memoised by a key over the root +source and, per source in source order, its identity, layer, config bytes, +and canonical parent list: the configs fix the pins and the overrides, and +the parent lists fix the topology, which the manifests carry rather than the +configs, so a changed graph under byte-identical configs recomposes. The +local-only registry of section 1 keys on its one `(source, config bytes)` +frame. Bundle bytes are re-read and re-hashed against their pin on every sync before the memo is consulted, so a bundle edited without a re-pin fails closed on the next command or request rather than serving the previous registry. `sync_registry` keeps an installed federated registry while the diff --git a/docs/cli.md b/docs/cli.md index 2524355b..eb2b2d6f 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -398,7 +398,9 @@ lists its declared types after the built-ins; `schema --template` and federated repository the types inherited from parents (ADR-150) are part of the effective registry: `decided new` composes the closure first and its identifier-collision scan covers the composed closure of the top-level corpus -directory holding the target; `schema` and `templates` list them when given +directory holding the target plus every other file in the repository, so an +identifier issued in a sibling directory or by a replaced parent artifact is +not reused; `schema` and `templates` list them when given `--corpus `, which composes that directory's closure, and list the local registry only when run without it. `--corpus` names a corpus directory; the root of a version-1 child is its corpus, while the root of a version-2 diff --git a/docs/validation.md b/docs/validation.md index 2b4e0b28..df7df064 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -146,7 +146,9 @@ carry the bundle's own digest, so no new pin is needed and a parent cannot change its types without changing the digest the child verifies. - **Identical declarations are silent; different ones are an error.** Two - sources declaring the same element content are one type. Two sources + sources declaring the same element content are one type; content is + compared as data, so JSON key order does not matter while list order (the + rendered order of sections and values) does. Two sources declaring the same name with different content stop every command and MCP tool with `corpus-federation-artifact-type-conflict`, naming the type and the sources, in the same failure class as a duplicate parent or a cycle. @@ -166,20 +168,26 @@ change its types without changing the digest the child verifies. rationale: APP-KWJ9D3C1S10N # a live local Decision ``` - `rationale` must resolve to exactly one Accepted, unretired Decision of the - declaring corpus; a name may be overridden at most once; an override for a + `rationale` must resolve, ignoring case, to exactly one Accepted, unretired + Decision of the declaring corpus. An override is config-level policy, so the + Decision is looked up across that whole corpus (a materialised parent's tree + excluded) whatever directory or `--top-level` scope a command names. A name + may be overridden at most once; an override for a name that does not collide, a `prefer` outside the corpus's transitive parents, or a preferred source that declares no candidate is `corpus-federation-invalid-override`, the finding artifact overrides already - use. The winner is what the corpus's descendants inherit; a descendant that - declares yet another content collides afresh and needs its own override. + use. The winner is what the corpus's descendants inherit through it; a + descendant that declares yet another content, or that reaches the losing + declaration again through another parent (a diamond), collides afresh and + needs its own override, which may prefer any source in its inherited view. - **A parent's bundle is verified on every command.** Its bytes are checked against the digest in the parent's captured config before any element is admitted; a mismatch is the parent-side `artifact-spec-bundle-digest-mismatch`, reported with the parent's source, and fails composition. - **Provenance is per source.** `decided validate --json` gains `artifact_spec_bundles`: one entry per source that pinned a bundle, in - composition order, with `source`, `layer`, `path`, `digest`, `admitted`, and + composition order, with `source` (`null` for a local bundle whose config + declares no `corpus.source`), `layer`, `path`, `digest`, `admitted`, and `warnings`. The single `artifact_spec_bundle` object stays for the local bundle. The human output adds one `WARN` block per inherited bundle with skipped elements, and `decided doctor` names the source in each inherited diff --git a/rust/decided/tests/spec_bundle.rs b/rust/decided/tests/spec_bundle.rs index 56b26498..f57713d9 100644 --- a/rust/decided/tests/spec_bundle.rs +++ b/rust/decided/tests/spec_bundle.rs @@ -746,3 +746,27 @@ fn okf_types_are_emitted_as_safe_scalars() { .unwrap(); assert!(policy.contains("type: Policy\n"), "{policy}"); } + +#[test] +fn a_local_bundle_without_a_declared_source_reports_a_null_source() { + let root = fixture_copy("null-source"); + let named = json(&run_in(&root, &["validate", "decisions", "--json"])); + assert_eq!( + named["artifact_spec_bundles"][0]["source"], + "asdecided/fixtures-spec-bundle" + ); + // Without `corpus.source` the label is null, never the `prefer: local` + // keyword a source name could be mistaken for. + let config = root.join(".decided/config.yaml"); + let text = fs::read_to_string(&config) + .unwrap() + .replace("corpus:\n source: asdecided/fixtures-spec-bundle\n", ""); + assert!(!text.contains("corpus:")); + fs::write(&config, text).unwrap(); + let output = run_in(&root, &["validate", "decisions", "--json"]); + let payload = json(&output); + let entry = &payload["artifact_spec_bundles"][0]; + assert!(entry["source"].is_null(), "{entry}"); + assert_eq!(entry["layer"], "local"); + assert_eq!(entry["path"], ".decided/artifact-specs.json"); +} diff --git a/rust/decided/tests/spec_federation.rs b/rust/decided/tests/spec_federation.rs index 828776ac..ff2a0631 100644 --- a/rust/decided/tests/spec_federation.rs +++ b/rust/decided/tests/spec_federation.rs @@ -408,6 +408,68 @@ fn a_decision_backed_override_selects_the_preferred_declaration() { assert_eq!(json(&output)["valid"], true); } +#[test] +fn a_type_override_rationale_resolves_across_the_whole_local_corpus() { + let root = fixture_copy("override-scope"); + let policy: serde_json::Value = + serde_json::from_slice(&fs::read(root.join(".decided/artifact-specs.json")).unwrap()) + .unwrap(); + let policy = policy["artifact_specs"][0].to_string(); + write_and_repin( + &root, + ".decided/artifact-specs.json", + ".decided/config.yaml", + &[&policy, RUNBOOK_ALT], + ); + // Rationale identifiers casefold, as ADR-137 artifact-override + // rationales do. + let lowered = LIVE_RATIONALE.to_lowercase(); + append_overrides(&root, &[("runbook", "local", &lowered)]); + fs::create_dir_all(root.join("decisions/runbooks")).unwrap(); + fs::write( + root.join("decisions/runbooks/rotate-keys.md"), + "---\nschema_version: 1\nid: SPC-000000000009\ntype: runbook\n---\n# Rotate Keys\n\n## Status\n\nActive\n\n## Purpose\n\nRotate credentials.\n", + ) + .unwrap(); + + // The Decision lives in decisions/decisions/, outside every scope below + // but the first; a type override is config-level policy and resolves in + // the declaring corpus whatever directory a command names. + for args in [ + &["validate", "decisions", "--json"][..], + &["validate", "decisions/runbooks", "--json"], + &["validate", "decisions", "--top-level", "--json"], + &["stats", "decisions/runbooks", "--json"], + &["doctor", "decisions/runbooks", "--json"], + ] { + let output = run_in(&root, args); + assert_eq!( + output.status.code(), + Some(0), + "{args:?}: {}{}", + stdout(&output), + stderr(&output) + ); + } + fs::create_dir_all(root.join("other")).unwrap(); + let created = run_in(&root, &["new", "decision", "other/choice.md"]); + assert_eq!(created.status.code(), Some(0), "{}", stderr(&created)); + + // A parent's Decision is not a local rationale, in any scope. + let config = root.join(".decided/config.yaml"); + let text = fs::read_to_string(&config) + .unwrap() + .replace(&lowered, "SPS-000000000002"); + fs::write(&config, text).unwrap(); + let output = run_in(&root, &["validate", "decisions/runbooks", "--json"]); + assert_eq!(output.status.code(), Some(1), "{}", stdout(&output)); + assert!( + stdout(&output).contains(INVALID_OVERRIDE) && stdout(&output).contains("does not resolve"), + "{}", + stdout(&output) + ); +} + #[test] fn override_defects_fail_with_one_stable_code_each() { let policy_of = |root: &Path| -> String { @@ -937,3 +999,58 @@ fn historical_exports_classify_under_the_bundles_pinned_at_that_revision() { )); assert_eq!(then, ["decision", "policy", "runbook"]); } + +#[test] +fn a_version_one_type_override_rationale_resolves_outside_the_command_scope() { + let repo = FederationRepo::new("spec-override-v-one"); + let runbook = standards_runbook(&fixture()); + let bundle = format!("{{\"artifact_specs\":[{runbook}]}}\n"); + repo.write("vendor/standards/.decided/artifact-specs.json", &bundle); + repo.append( + "vendor/standards/.decided/config.yaml", + &format!( + "artifact_types:\n version: 1\n bundle:\n path: .decided/artifact-specs.json\n digest: sha256:{}\n", + sha256(bundle.as_bytes()) + ), + ); + let local = format!("{{\"artifact_specs\":[{RUNBOOK_ALT}]}}\n"); + repo.write(".decided/artifact-specs.json", &local); + repo.append( + ".decided/config.yaml", + &format!( + "artifact_types:\n version: 1\n bundle:\n path: .decided/artifact-specs.json\n digest: sha256:{}\n overrides:\n - name: runbook\n prefer: local\n rationale: {}\n", + sha256(local.as_bytes()), + federation_support::CHILD_DECISION_ID.to_lowercase() + ), + ); + repo.write( + "decisions/runbooks/rotate-keys.md", + "---\nschema_version: 1\nid: APP-000000000009\ntype: runbook\n---\n# Rotate Keys\n\n## Status\n\nActive\n\n## Purpose\n\nRotate credentials.\n", + ); + repo.activate(); + + // The rationale Decision sits in decisions/decisions/, which neither the + // subdirectory nor the top-level scope reaches. + for args in [ + &["validate", "decisions", "--json"][..], + &["validate", "decisions/runbooks", "--json"], + &["validate", "decisions", "--top-level", "--json"], + ] { + let output = repo.run(args); + assert_eq!( + output.status.code(), + Some(0), + "{args:?}: {}{}", + stdout(&output), + stderr(&output) + ); + } + let payload = json(&repo.run(&["validate", "decisions/runbooks", "--json"])); + let row = payload["files"] + .as_array() + .unwrap() + .iter() + .find(|f| f["artifact_type"] == "runbook") + .expect("the local runbook classifies under the preferred declaration"); + assert_eq!(row["status"], "valid"); +} diff --git a/rust/rac-engine/src/doctor.rs b/rust/rac-engine/src/doctor.rs index a67bd369..0ea302c9 100644 --- a/rust/rac-engine/src/doctor.rs +++ b/rust/rac-engine/src/doctor.rs @@ -184,7 +184,8 @@ fn spec_bundle_findings() -> Vec { problem: if inherited { format!( "inherited source '{}': {element} skipped: {}", - source.source, warning.message + source.source.as_deref().unwrap_or_default(), + warning.message ) } else { format!("{element} skipped: {}", warning.message) @@ -194,7 +195,7 @@ fn spec_bundle_findings() -> Vec { "Fix the element in the spec bundle of '{}' and re-pin its digest in \ that corpus's .decided/config.yaml; the child inherits the parent's \ admitted types as pinned (ADR-150).", - source.source + source.source.as_deref().unwrap_or_default() ) } else { "Fix the element in the spec bundle and re-pin its digest in \ diff --git a/rust/rac-engine/src/federated_corpus.rs b/rust/rac-engine/src/federated_corpus.rs index e74e3dcc..6261f584 100644 --- a/rust/rac-engine/src/federated_corpus.rs +++ b/rust/rac-engine/src/federated_corpus.rs @@ -734,7 +734,14 @@ pub fn compose_verified_generation_from_snapshot( if let Some(registry) = registry { crate::spec_composition::verify_override_rationales( registry, + &verified.child_source, local.iter().chain(inherited.iter()), + || { + crate::spec_composition::root_tree_items( + &verified.child_repository_root, + std::slice::from_ref(&verified.materialisation_root), + ) + }, ) .map_err(|error| spec_error(verified, error))?; } diff --git a/rust/rac-engine/src/graph_federated_corpus.rs b/rust/rac-engine/src/graph_federated_corpus.rs index bcaf04e7..9ce07791 100644 --- a/rust/rac-engine/src/graph_federated_corpus.rs +++ b/rust/rac-engine/src/graph_federated_corpus.rs @@ -243,8 +243,18 @@ pub fn compose_verified_federation( let registry = install_closure_registry(&federation)?; let parsed = parse_and_validate_snapshots(&federation)?; if let Some(registry) = registry { - crate::spec_composition::verify_override_rationales(registry, parsed.items.iter()) - .map_err(|error| spec_error(&federation, error))?; + crate::spec_composition::verify_override_rationales( + registry, + &federation.root_source, + parsed.items.iter(), + || { + crate::spec_composition::root_tree_items( + &federation.repository_root, + &federation.materialisation_roots, + ) + }, + ) + .map_err(|error| spec_error(&federation, error))?; } validate_nested_v1( diff --git a/rust/rac-engine/src/output.rs b/rust/rac-engine/src/output.rs index 8fa1124f..0ac9813a 100644 --- a/rust/rac-engine/src/output.rs +++ b/rust/rac-engine/src/output.rs @@ -383,7 +383,8 @@ pub fn render_validate_dir_human(result: &DirectoryValidation) -> String { { lines.push(format!( "WARN {}:{} (artifact spec bundle, inherited)", - source.source, source.bundle.pin.path + source.source.as_deref().unwrap_or_default(), + source.bundle.pin.path )); for warning in &source.bundle.warnings { let label = match &warning.name { diff --git a/rust/rac-engine/src/scaffold.rs b/rust/rac-engine/src/scaffold.rs index 642f050e..ac0962fe 100644 --- a/rust/rac-engine/src/scaffold.rs +++ b/rust/rac-engine/src/scaffold.rs @@ -672,12 +672,19 @@ fn issued_ids(repository_root: &str, target_dir: &str) -> Result let items = match graph_collision_scope(repository_root, target_dir)? { // A version-2 graph rejects the repository root as a corpus path, so // the collision set is the composed closure of the top-level corpus - // directory holding the target, inherited layer included: an - // identifier a parent already issued is not free either. - Some(directory) => crate::federated_corpus::load_composed_corpus(&directory, true) - .map_err(|error| ScaffoldError::MalformedRepositoryConfig(error.to_string()))? - .map(|corpus| corpus.effective().cloned().collect::>()) - .unwrap_or_default(), + // directory holding the target, inherited layer included (an + // identifier a parent already issued is not free either), plus a + // plain walk of the whole repository, so identifiers in sibling + // top-level directories and in replaced parent artifacts stay issued + // as they were before the graph existed. + Some(directory) => { + let mut items = crate::federated_corpus::load_composed_corpus(&directory, true) + .map_err(|error| ScaffoldError::MalformedRepositoryConfig(error.to_string()))? + .map(|corpus| corpus.effective().cloned().collect::>()) + .unwrap_or_default(); + items.extend(crate::relationships::corpus_items(repository_root, true)); + items + } None => local_items(repository_root, true)?, }; Ok(items diff --git a/rust/rac-engine/src/spec.rs b/rust/rac-engine/src/spec.rs index 023c1ade..324903e1 100644 --- a/rust/rac-engine/src/spec.rs +++ b/rust/rac-engine/src/spec.rs @@ -361,7 +361,10 @@ pub struct SpecStanza { /// One source's contribution to the effective registry (ADR-150 decision 6). #[derive(Debug, Clone, PartialEq, Eq)] pub struct SourceSpecBundle { - pub source: String, + /// The contributing corpus's `corpus.source`; `None` only for a local + /// bundle in a config that declares no source (serialised as `null`, never + /// as the `prefer: local` keyword). + pub source: Option, pub layer: crate::corpus::Layer, pub bundle: SpecBundle, } @@ -1494,7 +1497,10 @@ pub fn sync_registry(start_dir: &str) -> Result<(), SpecBundleError> { install(None); return Ok(()); }; - let source = local_source(&config_path, text).unwrap_or_else(|| OVERRIDE_PREFER_LOCAL.into()); + let declared_source = local_source(&config_path, text); + let source = declared_source + .clone() + .unwrap_or_else(|| OVERRIDE_PREFER_LOCAL.into()); if !manifest_present { if let Some(first) = stanza.overrides.first() { // Nothing can collide in a corpus that declares no parents. @@ -1517,7 +1523,7 @@ pub fn sync_registry(start_dir: &str) -> Result<(), SpecBundleError> { let registry = registry_for_key(&key, || { let mut registry = admit_bundle(&path, &bundle_bytes, &pin)?; registry.sources = vec![SourceSpecBundle { - source: source.clone(), + source: declared_source.clone(), layer: crate::corpus::Layer::Local, bundle: registry .bundle diff --git a/rust/rac-engine/src/spec_composition.rs b/rust/rac-engine/src/spec_composition.rs index 7eb930ab..82a11ce5 100644 --- a/rust/rac-engine/src/spec_composition.rs +++ b/rust/rac-engine/src/spec_composition.rs @@ -15,9 +15,10 @@ //! its result in the process slot `spec` owns before any file is classified. use std::collections::{BTreeMap, BTreeSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use crate::corpus::Layer; +use crate::pycompat::py_casefold; use crate::relationships::CorpusItem; use crate::spec::{ self, AppliedOverride, ArtifactSpec, Registry, SourceSpecBundle, SpecBundleError, SpecStanza, @@ -86,11 +87,7 @@ pub fn install_effective_registry( spec::install(None); return Ok(None); } - let frames: Vec<(&str, &[u8])> = nodes - .iter() - .map(|node| (node.source.as_str(), node.config_bytes.as_slice())) - .collect(); - let key = spec::registry_key(&frames); + let key = composition_key(root_source, nodes); let registry = spec::registry_for_key(&key, || compose(root_source, nodes, &parsed))?; spec::install(Some(registry)); Ok(Some(registry)) @@ -112,21 +109,81 @@ pub fn verify_pinned_bundles(nodes: &[SourceSpecInput]) -> Result String { + let mut hasher = crate::sha256::Sha256::new(); + hasher.update(COMPOSITION_KEY_DOMAIN); + let mut field = |bytes: &[u8]| { + hasher.update(&(bytes.len() as u64).to_be_bytes()); + hasher.update(bytes); + }; + field(root_source.as_bytes()); + let mut ordered: Vec<&SourceSpecInput> = nodes.iter().collect(); + ordered.sort_by(|left, right| left.source.cmp(&right.source)); + for node in ordered { + field(node.source.as_bytes()); + field(node.layer.as_str().as_bytes()); + field(&node.config_bytes); + field(&(node.parents.len() as u64).to_be_bytes()); + for parent in &node.parents { + field(parent.as_bytes()); + } + } + hasher.hexdigest() +} + +/// Every artifact of the root source's working tree outside the materialised +/// parents: the corpus a root-owned type override's rationale resolves in, +/// whatever directory or recursion the command itself was scoped to. A type +/// override is config-level policy, so its Decision need not sit under the +/// directory a command names. +pub fn root_tree_items(repository_root: &Path, materialised: &[PathBuf]) -> Vec { + let canonical = + |path: &Path| std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + let materialised: Vec = materialised.iter().map(|root| canonical(root)).collect(); + crate::relationships::corpus_items(&repository_root.display().to_string(), true) + .into_iter() + .filter(|item| { + let path = canonical(Path::new(&item.path)); + !materialised.iter().any(|root| path.starts_with(root)) + }) + .collect() +} + +/// Check every applied override's rationale: it must resolve, ignoring case +/// as ADR-137 rationales do, to exactly one live local Decision of the +/// declaring source. An inherited source's corpus is captured whole in +/// `items`; the root's may be only the command's scope, so a root-owned +/// rationale that is not in scope is resolved over `root_tree` (called at +/// most once, and only then). pub fn verify_override_rationales<'a>( registry: &Registry, + root_source: &str, items: impl IntoIterator, + root_tree: impl FnOnce() -> Vec, ) -> Result<(), SpecBundleError> { let items: Vec<&CorpusItem> = items.into_iter().collect(); + let mut root_tree = Some(root_tree); + let mut whole_root: Option> = None; for applied in registry.overrides() { - let matches: Vec<&CorpusItem> = items + let rationale = py_casefold(&applied.rationale); + let carries = |item: &CorpusItem| py_casefold(&item.key.canonical_id) == rationale; + let mut matches: Vec<&CorpusItem> = items .iter() .copied() - .filter(|item| { - item.origin.source == applied.owner && item.key.canonical_id == applied.rationale - }) + .filter(|item| item.origin.source == applied.owner && carries(item)) .collect(); + if matches.is_empty() && applied.owner == root_source { + let tree = whole_root + .get_or_insert_with(|| root_tree.take().map(|walk| walk()).unwrap_or_default()); + matches = tree.iter().filter(|item| carries(item)).collect(); + } let reason = match matches.as_slice() { [] => Some(format!( "rationale '{}' does not resolve to a local artifact of '{}'", @@ -165,6 +222,47 @@ pub fn verify_override_rationales<'a>( Ok(()) } +/// Content identity of two admitted elements (ADR-150): map-like fields +/// compare as maps, so JSON key order cannot manufacture a conflict. List +/// order stays significant because it is rendered order. The destructuring is +/// exhaustive so a new field cannot be silently left out of the comparison. +fn same_content(left: &ArtifactSpec, right: &ArtifactSpec) -> bool { + fn map(pairs: &[(String, V)]) -> BTreeMap<&str, &V> { + pairs + .iter() + .map(|(key, value)| (key.as_str(), value)) + .collect() + } + let ArtifactSpec { + name, + display, + required, + recommended, + optional, + metadata, + retired_status, + descriptions, + guidance, + synonyms, + id_field, + starter_bodies, + okf_type, + } = left; + *name == right.name + && *display == right.display + && *required == right.required + && *recommended == right.recommended + && *optional == right.optional + && map(metadata) == map(&right.metadata) + && *retired_status == right.retired_status + && map(descriptions) == map(&right.descriptions) + && map(guidance) == map(&right.guidance) + && map(synonyms) == map(&right.synonyms) + && *id_field == right.id_field + && map(starter_bodies) == map(&right.starter_bodies) + && *okf_type == right.okf_type +} + /// Attribute a bundle or config failure to an inherited source. fn in_source( node: &SourceSpecInput, @@ -215,7 +313,9 @@ fn compose( let local_bundle = composer .sources .iter() - .find(|source| source.source == root_source && source.layer == Layer::Local) + .find(|source| { + source.source.as_deref() == Some(root_source) && source.layer == Layer::Local + }) .map(|source| source.bundle.clone()); let mut specs = spec::builtin_specs().to_vec(); specs.extend(effective.into_iter().map(|element| element.spec)); @@ -276,7 +376,7 @@ impl<'a> Composer<'a> { .cloned() .expect("an admitted bundle carries its report"); self.sources.push(SourceSpecBundle { - source: source.to_string(), + source: Some(source.to_string()), layer: node.layer, bundle, }); @@ -308,7 +408,7 @@ impl<'a> Composer<'a> { }); match group .iter_mut() - .find(|element| element.spec == candidate.spec) + .find(|element| same_content(&element.spec, &candidate.spec)) { Some(same) => { for origin in candidate.origins { @@ -538,7 +638,7 @@ mod tests { let sources: Vec<(&str, Layer)> = registry .sources() .iter() - .map(|s| (s.source.as_str(), s.layer)) + .map(|s| (s.source.as_deref().unwrap(), s.layer)) .collect(); assert_eq!( sources, @@ -772,6 +872,162 @@ mod tests { .starts_with("inherited source 'acme/parent': ")); } + /// `RUNBOOK` with two descriptions, in the given key order. + fn runbook_described(reversed: bool) -> String { + let descriptions = if reversed { + r#"{"steps":"How","purpose":"Why"}"# + } else { + r#"{"purpose":"Why","steps":"How"}"# + }; + RUNBOOK.replace( + r#""descriptions":{}"#, + &format!(r#""descriptions":{descriptions}"#), + ) + } + + #[test] + fn key_order_alone_is_not_a_conflict_but_list_order_is() { + let child = scratch("order-child"); + let parent = scratch("order-parent"); + let forward = runbook_described(false); + let child_pin = pin(&child, &[&runbook_described(true)]); + let parent_pin = pin(&parent, &[&forward]); + let nodes = |child_pin: &BundlePin, parent_pin: &BundlePin| { + [ + node( + "acme/child", + Layer::Local, + &child, + Some(child_pin), + &[], + &["acme/parent"], + ), + node( + "acme/parent", + Layer::Inherited, + &parent, + Some(parent_pin), + &[], + &[], + ), + ] + }; + let registry = compose_only("acme/child", &nodes(&child_pin, &parent_pin)).unwrap(); + assert_eq!(names(®istry), ["runbook"]); + + // Reordering a list changes rendered order, so it is different content. + let reordered = forward.replace(r#"["purpose","steps"]"#, r#"["steps","purpose"]"#); + assert_ne!(reordered, forward); + let child_pin = pin(&child, &[&reordered]); + let error = compose_only("acme/child", &nodes(&child_pin, &parent_pin)).unwrap_err(); + assert_eq!( + error.stable_code(), + "corpus-federation-artifact-type-conflict" + ); + } + + #[test] + fn a_topology_change_under_identical_configs_recomposes() { + let root = scratch("topo-root"); + let a = scratch("topo-a"); + let b = scratch("topo-b"); + let a_pin = pin(&a, &[RUNBOOK_ALT]); + let b_pin = pin(&b, &[RUNBOOK]); + let prefer_local = [overr("runbook", "local")]; + // T1: root -> a -> b, and a resolves its collision with b. + let chain = [ + node("acme/root", Layer::Local, &root, None, &[], &["acme/a"]), + node( + "acme/a", + Layer::Inherited, + &a, + Some(&a_pin), + &prefer_local, + &["acme/b"], + ), + node("acme/b", Layer::Inherited, &b, Some(&b_pin), &[], &[]), + ]; + let registry = install_effective_registry("acme/root", &chain) + .unwrap() + .unwrap(); + assert_eq!(registry.spec_for("runbook").unwrap().display, "Run Book"); + // T2: the same config bytes, but root -> a and root -> b directly. The + // override in a no longer names a collision, exactly as a fresh + // process reports it; the memo must not serve T1's registry. + let fan = [ + node( + "acme/root", + Layer::Local, + &root, + None, + &[], + &["acme/a", "acme/b"], + ), + node( + "acme/a", + Layer::Inherited, + &a, + Some(&a_pin), + &prefer_local, + &[], + ), + node("acme/b", Layer::Inherited, &b, Some(&b_pin), &[], &[]), + ]; + assert_eq!( + chain.iter().map(|n| &n.config_bytes).collect::>(), + fan.iter().map(|n| &n.config_bytes).collect::>() + ); + let error = install_effective_registry("acme/root", &fan).unwrap_err(); + assert_eq!(error.stable_code(), "corpus-federation-invalid-override"); + assert!(error.detail().contains("does not collide"), "{error}"); + spec::install(None); + } + + #[test] + fn a_diamond_reopens_a_collision_a_middle_node_resolved() { + let root = scratch("dia-root"); + let a = scratch("dia-a"); + let b = scratch("dia-b"); + let g = scratch("dia-g"); + let a_pin = pin(&a, &[RUNBOOK_ALT]); + let g_pin = pin(&g, &[RUNBOOK]); + let with = |root_overrides: &[TypeOverride]| { + [ + node( + "acme/root", + Layer::Local, + &root, + None, + root_overrides, + &["acme/a", "acme/b"], + ), + node( + "acme/a", + Layer::Inherited, + &a, + Some(&a_pin), + &[overr("runbook", "local")], + &["acme/g"], + ), + node("acme/b", Layer::Inherited, &b, None, &[], &["acme/g"]), + node("acme/g", Layer::Inherited, &g, Some(&g_pin), &[], &[]), + ] + }; + // a's resolution governs a's view only: the root reaches g's runbook + // again through b and must decide for itself. + let error = compose_only("acme/root", &with(&[])).unwrap_err(); + assert_eq!( + error.stable_code(), + "corpus-federation-artifact-type-conflict" + ); + assert_eq!(error.source(), Some("acme/root")); + assert!(error.detail().contains("'acme/a' and 'acme/g'"), "{error}"); + let registry = compose_only("acme/root", &with(&[overr("runbook", "acme/a")])).unwrap(); + assert_eq!(registry.spec_for("runbook").unwrap().display, "Run Book"); + let registry = compose_only("acme/root", &with(&[overr("runbook", "acme/g")])).unwrap(); + assert_eq!(registry.spec_for("runbook").unwrap().display, "Runbook"); + } + #[test] fn a_closure_without_any_stanza_is_inert() { let child = scratch("inert-child");