From 901dbdbfdabb846a9f16d8ac5abded07ca7bfb43 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 25 Jun 2026 14:51:36 +0000 Subject: [PATCH] detect union schema narrowing --- codex-rs/schema-evolution/src/compare.rs | 49 +++- .../schema-evolution/src/compare/union.rs | 243 ++++++++++++++++++ .../src/compare/union_tests.rs | 88 +++++++ .../schema-evolution/src/compare_tests.rs | 20 ++ codex-rs/schema-evolution/src/lib.rs | 1 + codex-rs/schema-evolution/src/violation.rs | 15 ++ 6 files changed, 415 insertions(+), 1 deletion(-) create mode 100644 codex-rs/schema-evolution/src/compare/union.rs create mode 100644 codex-rs/schema-evolution/src/compare/union_tests.rs diff --git a/codex-rs/schema-evolution/src/compare.rs b/codex-rs/schema-evolution/src/compare.rs index ce875bb186..c4e79dbf2e 100644 --- a/codex-rs/schema-evolution/src/compare.rs +++ b/codex-rs/schema-evolution/src/compare.rs @@ -1,5 +1,6 @@ mod array; mod object; +mod union; mod value; use crate::ApiSchema; @@ -81,6 +82,34 @@ impl<'a> CompareCx<'a> { after: SchemaSnapshot(after), }); } + + pub(super) fn probe( + &self, + base: SchemaId, + current: SchemaId, + path: &SchemaPath, + ) -> Result> { + let mut probe = Self::new(self.base, self.current, self.method); + probe.active.clone_from(&self.active); + base.compare(¤t, &mut probe, path)?; + Ok(probe.violations) + } + + pub(super) fn reverse_probe( + &self, + current: SchemaId, + base: SchemaId, + path: &SchemaPath, + ) -> Result> { + let mut probe = Self::new(self.current, self.base, self.method); + probe.active = self + .active + .iter() + .map(|(base, current)| (*current, *base)) + .collect(); + current.compare(&base, &mut probe, path)?; + Ok(probe.violations) + } } impl CompareNarrowing for Method { @@ -135,7 +164,7 @@ impl CompareNarrowing for SchemaId { Ok(()) } (SchemaNode::Rules(base), SchemaNode::Rules(current)) => { - compare_rules(base, current, cx, path) + compare_rules(base_id, base, current_id, current, cx, path) } (SchemaNode::Reference(_), _) | (_, SchemaNode::Reference(_)) => { unreachable!("resolve returns concrete schema nodes") @@ -147,13 +176,31 @@ impl CompareNarrowing for SchemaId { } fn compare_rules( + base_id: SchemaId, base: &SchemaRules, + current_id: SchemaId, current: &SchemaRules, cx: &mut CompareCx<'_>, path: &SchemaPath, ) -> Result<()> { value::compare_values(base.values.as_ref(), current.values.as_ref(), cx, path); value::compare_types(base.types.as_ref(), current.types.as_ref(), cx, path); + union::compare_optional( + base.any_of.as_ref(), + current.any_of.as_ref(), + base_id, + current_id, + cx, + path, + )?; + union::compare_optional( + base.one_of.as_ref(), + current.one_of.as_ref(), + base_id, + current_id, + cx, + path, + )?; object::compare_optional(base.object.as_ref(), current.object.as_ref(), cx, path)?; array::compare_optional(base.array.as_ref(), current.array.as_ref(), cx, path)?; value::compare_constraints(&base.constraints, ¤t.constraints, cx, path); diff --git a/codex-rs/schema-evolution/src/compare/union.rs b/codex-rs/schema-evolution/src/compare/union.rs new file mode 100644 index 0000000000..1a5b0e7644 --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/union.rs @@ -0,0 +1,243 @@ +use super::CompareCx; +use crate::ApiSchema; +use crate::SchemaId; +use crate::SchemaNode; +use crate::SchemaPath; +use crate::TypeSet; +use crate::UnionKind; +use crate::UnionSchema; +use crate::ValueSet; +use crate::VariantLabel; +use crate::Violation; +use anyhow::Result; +use serde_json::Value; + +pub(super) fn compare_optional( + base: Option<&UnionSchema>, + current: Option<&UnionSchema>, + base_schema: SchemaId, + _current_schema: SchemaId, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + match (base, current) { + (None, None) | (Some(_), None) => Ok(()), + (None, Some(current)) => { + let mut covered = false; + for branch in ¤t.variants { + if cx.probe(base_schema, *branch, path)?.is_empty() { + covered = true; + break; + } + } + if !covered + || (current.kind == UnionKind::OneOf + && !pairwise_disjoint(cx.current, ¤t.variants)?) + { + cx.constraint_changed(path, Value::Null, labels(cx.current, current)?); + } + Ok(()) + } + (Some(base), Some(current)) => compare(base, current, cx, path), + } +} + +fn compare( + base: &UnionSchema, + current: &UnionSchema, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + let mut all_covered = true; + for base_branch in &base.variants { + let key = branch_key(cx.base, *base_branch)?; + let mut matching_changes = None; + let mut covered = false; + for current_branch in ¤t.variants { + let changes = cx.probe(*base_branch, *current_branch, path)?; + if changes.is_empty() { + covered = true; + } + if matching_changes.is_none() && branch_key(cx.current, *current_branch)? == key { + matching_changes = Some(changes); + } + } + if !covered && let Some(changes) = matching_changes { + all_covered = false; + cx.violations.extend(changes); + } else if !covered { + all_covered = false; + cx.violations.push(Violation::UnionVariantRemoved { + at: cx.location(path), + variant: VariantLabel(key), + }); + } + } + let all_equivalent = base.kind == UnionKind::OneOf + && all_covered + && base.variants.len() == current.variants.len() + && equivalent_bijection(base, current, cx, path)?; + if base.kind == UnionKind::OneOf + && all_covered + && !all_equivalent + && !pairwise_disjoint(cx.current, ¤t.variants)? + { + cx.constraint_changed(path, labels(cx.base, base)?, labels(cx.current, current)?); + } + Ok(()) +} + +fn equivalent_bijection( + base: &UnionSchema, + current: &UnionSchema, + cx: &CompareCx<'_>, + path: &SchemaPath, +) -> Result { + let mut equivalent = vec![vec![false; current.variants.len()]; base.variants.len()]; + for (base_index, base_branch) in base.variants.iter().enumerate() { + for (current_index, current_branch) in current.variants.iter().enumerate() { + equivalent[base_index][current_index] = + cx.probe(*base_branch, *current_branch, path)?.is_empty() + && cx + .reverse_probe(*current_branch, *base_branch, path)? + .is_empty(); + } + } + let mut assigned = vec![None; current.variants.len()]; + for base_index in 0..base.variants.len() { + let mut seen = vec![false; current.variants.len()]; + if !assign_equivalent(base_index, &equivalent, &mut assigned, &mut seen) { + return Ok(false); + } + } + Ok(true) +} + +fn assign_equivalent( + base_index: usize, + equivalent: &[Vec], + assigned: &mut [Option], + seen: &mut [bool], +) -> bool { + for current_index in 0..assigned.len() { + if !equivalent[base_index][current_index] || seen[current_index] { + continue; + } + seen[current_index] = true; + if assigned[current_index] + .is_none_or(|previous| assign_equivalent(previous, equivalent, assigned, seen)) + { + assigned[current_index] = Some(base_index); + return true; + } + } + false +} + +fn labels(schema: &ApiSchema, union: &UnionSchema) -> Result { + let mut labels = union + .variants + .iter() + .map(|variant| branch_key(schema, *variant).map(Value::String)) + .collect::>>()?; + labels.sort_by_key(|label| serde_json::to_string(label).unwrap_or_default()); + Ok(Value::Array(labels)) +} + +fn branch_key(schema: &ApiSchema, id: SchemaId) -> Result { + let (_, node) = schema.resolve(id)?; + let SchemaNode::Rules(rules) = node else { + return Ok(format!( + "schema={}", + serde_json::to_string(&schema.snapshot(id)?)? + )); + }; + if let Some(values) = &rules.values { + return Ok(format!("enum={}", serde_json::to_string(&values.values)?)); + } + if let Some((name, values)) = discriminator(schema, id)? { + return Ok(format!("{name}={}", serde_json::to_string(&values.values)?)); + } + if let Some(types) = &rules.types { + return Ok(format!("type={}", serde_json::to_string(&types.to_json())?)); + } + Ok(format!( + "schema={}", + serde_json::to_string(&schema.snapshot(id)?)? + )) +} + +fn pairwise_disjoint(schema: &ApiSchema, variants: &[SchemaId]) -> Result { + for (index, left) in variants.iter().enumerate() { + for right in &variants[index + 1..] { + if disjoint(schema, *left, *right)? { + continue; + } + return Ok(false); + } + } + Ok(true) +} + +fn disjoint(schema: &ApiSchema, left: SchemaId, right: SchemaId) -> Result { + let left_types = types(schema, left)?; + let right_types = types(schema, right)?; + if matches!((left_types, right_types), (Some(left), Some(right)) if left.accepted_types().is_disjoint(&right.accepted_types())) + { + return Ok(true); + } + let left_values = values(schema, left)?; + let right_values = values(schema, right)?; + if matches!((left_values, right_values), (Some(left), Some(right)) if left.values.iter().all(|value| !right.values.contains(value))) + { + return Ok(true); + } + let left_discriminator = discriminator(schema, left)?; + let right_discriminator = discriminator(schema, right)?; + Ok(matches!( + (left_discriminator, right_discriminator), + (Some((left_name, left)), Some((right_name, right))) + if left_name == right_name + && left.values.iter().all(|value| !right.values.contains(value)) + )) +} + +fn types(schema: &ApiSchema, id: SchemaId) -> Result> { + let (_, node) = schema.resolve(id)?; + Ok(match node { + SchemaNode::Rules(rules) => rules.types.as_ref(), + SchemaNode::Any | SchemaNode::Never => None, + SchemaNode::Reference(_) => unreachable!("resolve returns concrete schema nodes"), + }) +} + +fn values(schema: &ApiSchema, id: SchemaId) -> Result> { + let (_, node) = schema.resolve(id)?; + Ok(match node { + SchemaNode::Rules(rules) => rules.values.as_ref(), + SchemaNode::Any | SchemaNode::Never => None, + SchemaNode::Reference(_) => unreachable!("resolve returns concrete schema nodes"), + }) +} + +fn discriminator(schema: &ApiSchema, id: SchemaId) -> Result> { + let (_, node) = schema.resolve(id)?; + let SchemaNode::Rules(rules) = node else { + return Ok(None); + }; + let Some(object) = &rules.object else { + return Ok(None); + }; + for (name, property) in &object.properties { + if object.required.contains(name) + && let Some(values) = values(schema, property.schema)? + { + return Ok(Some((name.clone(), values.clone()))); + } + } + Ok(None) +} + +#[cfg(test)] +#[path = "union_tests.rs"] +mod tests; diff --git a/codex-rs/schema-evolution/src/compare/union_tests.rs b/codex-rs/schema-evolution/src/compare/union_tests.rs new file mode 100644 index 0000000000..17c745692a --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/union_tests.rs @@ -0,0 +1,88 @@ +use crate::ViolationKind; +use crate::test_support::breakage; +use crate::test_support::compare; +use crate::test_support::request_schema; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn detects_removed_union_variants_but_allows_widening() -> anyhow::Result<()> { + let base = request_schema(json!({ + "anyOf": [{ "enum": ["a"] }, { "type": "null" }] + })); + let current = request_schema(json!({ "anyOf": [{ "enum": ["a", "b"] }] })); + assert_eq!( + compare(&base, ¤t)?, + vec![breakage( + ViolationKind::UnionVariantRemoved, + "params", + json!("type=\"null\""), + json!(null), + )] + ); + + let base = request_schema(json!({ "type": "string" })); + let current = request_schema(json!({ + "anyOf": [{ "type": "string" }, { "type": "null" }] + })); + assert_eq!(compare(&base, ¤t)?, vec![]); + Ok(()) +} + +#[test] +fn detects_new_overlap_in_one_of() -> anyhow::Result<()> { + let base = request_schema(json!({ + "oneOf": [{ "enum": ["a"] }, { "enum": ["b"] }] + })); + let current = request_schema(json!({ + "oneOf": [{ "enum": ["a", "b"] }, { "enum": ["b", "c"] }] + })); + let violations = compare(&base, ¤t)?; + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].kind, ViolationKind::ConstraintChanged); + assert_eq!(violations[0].path, "params"); + Ok(()) +} + +#[test] +fn compares_recursive_union_refs_without_reentering_forever() -> anyhow::Result<()> { + let mut base = request_schema(json!({ "$ref": "#/definitions/Params" })); + base["definitions"] = json!({ + "Params": { + "properties": { + "child": { "anyOf": [{ "$ref": "#/definitions/Params" }, { "type": "null" }] }, + "value": { "type": "number" } + }, + "type": "object" + } + }); + let mut current = base.clone(); + current["definitions"]["Params"]["properties"]["value"]["type"] = json!("integer"); + + assert_eq!( + compare(&base, ¤t)?, + vec![breakage( + ViolationKind::TypeNarrowed, + "params.value", + json!("number"), + json!("integer"), + )] + ); + Ok(()) +} + +#[test] +fn one_of_equivalence_preserves_duplicate_branch_multiplicity() -> anyhow::Result<()> { + let base = request_schema(json!({ + "oneOf": [{ "enum": ["a"] }, { "enum": ["a"] }, { "enum": ["b"] }] + })); + let current = request_schema(json!({ + "oneOf": [{ "enum": ["a"] }, { "enum": ["b"] }, { "enum": ["b"] }] + })); + + let violations = compare(&base, ¤t)?; + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].kind, ViolationKind::ConstraintChanged); + assert_eq!(violations[0].path, "params"); + Ok(()) +} diff --git a/codex-rs/schema-evolution/src/compare_tests.rs b/codex-rs/schema-evolution/src/compare_tests.rs index dce17fe06d..6c12355753 100644 --- a/codex-rs/schema-evolution/src/compare_tests.rs +++ b/codex-rs/schema-evolution/src/compare_tests.rs @@ -53,6 +53,26 @@ fn compares_typed_arguments_with_the_full_request_schema() -> Result<()> { Ok(()) } +#[test] +fn detects_request_level_union_narrowing_that_constrains_params() -> Result<()> { + let base = request_schema(json!({ "type": "number" })); + let mut current = base.clone(); + current["oneOf"][0]["anyOf"] = json!([{ + "properties": { + "id": { "type": ["string", "integer"] }, + "method": { "enum": ["test/method"], "type": "string" }, + "params": { "type": "integer" } + }, + "type": "object" + }]); + + let violations = compare(&base, ¤t)?; + assert!(violations.iter().any(|violation| { + violation.kind == ViolationKind::ConstraintChanged && violation.path == "request" + })); + Ok(()) +} + #[test] fn retains_method_and_request_level_constraints_in_the_typed_envelope() -> Result<()> { let mut base = request_schema(json!({ "type": "null" })); diff --git a/codex-rs/schema-evolution/src/lib.rs b/codex-rs/schema-evolution/src/lib.rs index 9b20caa383..a8ef15f421 100644 --- a/codex-rs/schema-evolution/src/lib.rs +++ b/codex-rs/schema-evolution/src/lib.rs @@ -29,6 +29,7 @@ pub(crate) use violation::Location; pub use violation::SchemaBreakage; pub(crate) use violation::SchemaPath; pub(crate) use violation::SchemaSnapshot; +pub(crate) use violation::VariantLabel; pub(crate) use violation::Violation; pub use violation::ViolationKind; diff --git a/codex-rs/schema-evolution/src/violation.rs b/codex-rs/schema-evolution/src/violation.rs index 98eb78937d..33a91a0085 100644 --- a/codex-rs/schema-evolution/src/violation.rs +++ b/codex-rs/schema-evolution/src/violation.rs @@ -39,6 +39,10 @@ pub enum Violation { before: Option, after: ValueSet, }, + UnionVariantRemoved { + at: Location, + variant: VariantLabel, + }, AdditionalPropertiesNarrowed { at: Location, before: AdditionalPropertiesValue, @@ -74,6 +78,9 @@ enum PathSegment { AdditionalProperties, } +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct VariantLabel(pub String); + #[derive(Clone, Debug, Eq, PartialEq)] pub enum AdditionalPropertiesValue { Any, @@ -193,6 +200,12 @@ impl Violation { before.as_ref().map_or(Value::Null, ValueSet::to_json), after.to_json(), ), + Self::UnionVariantRemoved { at, variant } => at_location( + ViolationKind::UnionVariantRemoved, + at, + Value::String(variant.0.clone()), + Value::Null, + ), Self::AdditionalPropertiesNarrowed { at, before, after } => at_location( ViolationKind::AdditionalPropertiesNarrowed, at, @@ -215,6 +228,7 @@ impl Violation { | Self::RequiredPropertyAdded { at } | Self::TypeNarrowed { at, .. } | Self::EnumNarrowed { at, .. } + | Self::UnionVariantRemoved { at, .. } | Self::AdditionalPropertiesNarrowed { at, .. } | Self::ConstraintChanged { at, .. } => Some(at), } @@ -233,6 +247,7 @@ impl Violation { | Self::RequiredPropertyAdded { at } | Self::TypeNarrowed { at, .. } | Self::EnumNarrowed { at, .. } + | Self::UnionVariantRemoved { at, .. } | Self::AdditionalPropertiesNarrowed { at, .. } | Self::ConstraintChanged { at, .. } => Some(at), }