diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 17bab1e24b..adead3ac98 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3846,6 +3846,7 @@ dependencies = [ "pretty_assertions", "serde", "serde_json", + "toml 0.9.11+spec-1.1.0", ] [[package]] diff --git a/codex-rs/schema-evolution/Cargo.toml b/codex-rs/schema-evolution/Cargo.toml index 3dc8e0247d..dd6011ca0e 100644 --- a/codex-rs/schema-evolution/Cargo.toml +++ b/codex-rs/schema-evolution/Cargo.toml @@ -16,6 +16,7 @@ workspace = true anyhow = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +toml = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/schema-evolution/src/known_breakage.rs b/codex-rs/schema-evolution/src/known_breakage.rs new file mode 100644 index 0000000000..d8d35cf36b --- /dev/null +++ b/codex-rs/schema-evolution/src/known_breakage.rs @@ -0,0 +1,276 @@ +use crate::ApiSchema; +use crate::SchemaBreakage; +use crate::ViolationKind; +use crate::find_request_narrowing; +use anyhow::Context; +use anyhow::Result; +use anyhow::bail; +use serde::Deserialize; +use serde::Serialize; +use serde_json::Value; + +const KNOWN_BREAKAGE_LOG_VERSION: u32 = 1; + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(deny_unknown_fields)] +pub struct KnownBreakage { + pub id: u64, + pub kind: ViolationKind, + pub method: String, + pub path: String, + pub before_json: String, + pub after_json: String, + pub justification: String, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(deny_unknown_fields)] +pub struct KnownBreakageLog { + version: u32, + #[serde(default)] + breakages: Vec, +} + +#[derive(Serialize)] +struct KnownBreakageTemplate<'a> { + breakages: &'a [KnownBreakage], +} + +impl KnownBreakage { + fn from_breakage(id: u64, breakage: &SchemaBreakage) -> Self { + Self { + id, + kind: breakage.kind.clone(), + method: breakage.method.clone(), + path: breakage.path.clone(), + before_json: canonical_json(&breakage.before), + after_json: canonical_json(&breakage.after), + justification: String::new(), + } + } + + fn matches(&self, breakage: &SchemaBreakage) -> bool { + self.kind == breakage.kind + && self.method == breakage.method + && self.path == breakage.path + && self.before_json == canonical_json(&breakage.before) + && self.after_json == canonical_json(&breakage.after) + } +} + +impl KnownBreakageLog { + /// Parses one versioned, append-only known-breakage log. + pub fn parse(contents: &str, label: &str) -> Result { + toml::from_str(contents).with_context(|| format!("parse known-breakage log {label}")) + } +} + +/// Checks request-schema narrowing and verifies that each breakage was newly recorded. +/// +/// The earlier log must be an exact prefix of the current log. This makes the log an +/// append-only history and prevents an older entry from acknowledging a new change. +pub fn check_request_narrowing( + before: &ApiSchema, + after: &ApiSchema, + before_log: &KnownBreakageLog, + after_log: &KnownBreakageLog, +) -> Result> { + let breakages = find_request_narrowing(before, after)?; + let problems = known_breakage_problems(before_log, after_log, &breakages); + if !problems.is_empty() { + bail!(failure_report( + &problems, &breakages, before_log, after_log + )?); + } + Ok(breakages) +} + +fn known_breakage_problems( + before_log: &KnownBreakageLog, + after_log: &KnownBreakageLog, + breakages: &[SchemaBreakage], +) -> Vec { + let mut problems = validate_log("before", before_log); + problems.extend(validate_log("after", after_log)); + + for (index, previous) in before_log.breakages.iter().enumerate() { + match after_log.breakages.get(index) { + Some(current) if current == previous => {} + Some(_) => problems.push(format!( + "known breakage {} was edited or reordered; existing entries are append-only", + previous.id + )), + None => problems.push(format!( + "known breakage {} was deleted; existing entries are append-only", + previous.id + )), + } + } + + let appended = after_log + .breakages + .get(before_log.breakages.len()..) + .unwrap_or_default(); + for breakage in breakages { + match appended + .iter() + .filter(|entry| entry.matches(breakage)) + .count() + { + 0 => problems.push(format!( + "missing a new known-breakage entry for {} {:?} at {}", + breakage.method, breakage.kind, breakage.path + )), + 1 => {} + _ => problems.push(format!( + "multiple new known-breakage entries match {} at {}", + breakage.method, breakage.path + )), + } + } + for entry in appended { + if !breakages.iter().any(|breakage| entry.matches(breakage)) { + problems.push(format!( + "new known breakage {} does not match a detected request-schema breakage", + entry.id + )); + } + } + problems +} + +fn validate_log(label: &str, log: &KnownBreakageLog) -> Vec { + let mut problems = Vec::new(); + if log.version != KNOWN_BREAKAGE_LOG_VERSION { + problems.push(format!( + "{label} known-breakage log must use version {KNOWN_BREAKAGE_LOG_VERSION}" + )); + } + for (index, entry) in log.breakages.iter().enumerate() { + let expected_id = index as u64 + 1; + if entry.id != expected_id { + problems.push(format!( + "{label} known-breakage entry {} must have id {expected_id}", + entry.id + )); + } + if entry.justification.trim().is_empty() { + problems.push(format!( + "{label} known breakage {} needs a justification", + entry.id + )); + } + if entry.method.trim().is_empty() { + problems.push(format!( + "{label} known breakage {} needs a method", + entry.id + )); + } + if entry.path.trim().is_empty() { + problems.push(format!("{label} known breakage {} needs a path", entry.id)); + } + validate_snapshot( + label, + entry, + "before_json", + &entry.before_json, + &mut problems, + ); + validate_snapshot(label, entry, "after_json", &entry.after_json, &mut problems); + } + problems +} + +fn validate_snapshot( + label: &str, + entry: &KnownBreakage, + field: &str, + snapshot: &str, + problems: &mut Vec, +) { + if serde_json::from_str::(snapshot).is_err() { + problems.push(format!( + "{label} known breakage {} has invalid JSON in {field}", + entry.id + )); + } +} + +fn failure_report( + problems: &[String], + breakages: &[SchemaBreakage], + before_log: &KnownBreakageLog, + after_log: &KnownBreakageLog, +) -> Result { + let mut report = String::from("request schema compatibility lint failed:\n"); + for problem in problems { + report.push_str(&format!("- {problem}\n")); + } + for breakage in breakages { + report.push_str(&format!( + "- {} {:?} at {}: {} -> {}\n", + breakage.method, breakage.kind, breakage.path, breakage.before, breakage.after + )); + } + + let missing = if log_can_be_extended(before_log, after_log, breakages) { + let appended = &after_log.breakages[before_log.breakages.len()..]; + breakages + .iter() + .filter(|breakage| !appended.iter().any(|entry| entry.matches(breakage))) + .enumerate() + .map(|(index, breakage)| { + KnownBreakage::from_breakage( + after_log.breakages.len() as u64 + index as u64 + 1, + breakage, + ) + }) + .collect::>() + } else { + Vec::new() + }; + if !missing.is_empty() { + report.push_str(&format!( + "\nappend one entry per breakage to the known-breakage log:\n{}\n", + toml::to_string_pretty(&KnownBreakageTemplate { + breakages: &missing + })? + )); + } + Ok(report) +} + +fn log_can_be_extended( + before_log: &KnownBreakageLog, + after_log: &KnownBreakageLog, + breakages: &[SchemaBreakage], +) -> bool { + if !validate_log("before", before_log).is_empty() + || !validate_log("after", after_log).is_empty() + || !after_log.breakages.starts_with(&before_log.breakages) + { + return false; + } + let appended = &after_log.breakages[before_log.breakages.len()..]; + appended.iter().all(|entry| { + breakages + .iter() + .filter(|breakage| entry.matches(breakage)) + .count() + == 1 + }) && breakages.iter().all(|breakage| { + appended + .iter() + .filter(|entry| entry.matches(breakage)) + .count() + <= 1 + }) +} + +fn canonical_json(value: &Value) -> String { + serde_json::to_string(value).unwrap_or_default() +} + +#[cfg(test)] +#[path = "known_breakage_tests.rs"] +mod tests; diff --git a/codex-rs/schema-evolution/src/known_breakage_tests.rs b/codex-rs/schema-evolution/src/known_breakage_tests.rs new file mode 100644 index 0000000000..587806a49e --- /dev/null +++ b/codex-rs/schema-evolution/src/known_breakage_tests.rs @@ -0,0 +1,184 @@ +use super::*; +use crate::test_support::request_schema; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn only_new_exact_entries_acknowledge_breakages() -> Result<()> { + let (before, after, breakage) = type_narrowing()?; + let before_log = log(Vec::new()); + let after_log = log(vec![known(/*id*/ 1, &breakage)]); + + assert_eq!( + check_request_narrowing(&before, &after, &before_log, &after_log)?, + vec![breakage] + ); + Ok(()) +} + +#[test] +fn existing_entries_form_a_complete_sequential_prefix() { + let first = old_breakage(/*id*/ 1, "test/old"); + let before = log(vec![first.clone()]); + let after = log(Vec::new()); + + assert_eq!( + known_breakage_problems(&before, &after, &[]), + vec!["known breakage 1 was deleted; existing entries are append-only".to_string()] + ); + + let out_of_sequence = log(vec![KnownBreakage { id: 2, ..first }]); + assert_eq!( + validate_log("after", &out_of_sequence), + vec!["after known-breakage entry 2 must have id 1".to_string()] + ); +} + +#[test] +fn historical_entries_cannot_acknowledge_a_new_diff_and_cannot_be_edited() -> Result<()> { + let (_, _, breakage) = type_narrowing()?; + let historical = known(/*id*/ 1, &breakage); + let before = log(vec![historical.clone()]); + + assert_eq!( + known_breakage_problems( + &before, + &log(vec![historical.clone()]), + std::slice::from_ref(&breakage), + ), + vec![ + "missing a new known-breakage entry for test/method TypeNarrowed at params".to_string() + ] + ); + + let edited = KnownBreakage { + justification: "a different justification".to_string(), + ..historical + }; + assert_eq!( + known_breakage_problems(&before, &log(vec![edited]), &[]), + vec![ + "known breakage 1 was edited or reordered; existing entries are append-only" + .to_string() + ] + ); + Ok(()) +} + +#[test] +fn stale_mismatched_and_duplicate_new_entries_are_rejected() -> Result<()> { + let (_, _, breakage) = type_narrowing()?; + let stale = KnownBreakage { + method: "test/other".to_string(), + ..known(/*id*/ 1, &breakage) + }; + assert_eq!( + known_breakage_problems( + &log(Vec::new()), + &log(vec![stale]), + std::slice::from_ref(&breakage), + ), + vec![ + "missing a new known-breakage entry for test/method TypeNarrowed at params".to_string(), + "new known breakage 1 does not match a detected request-schema breakage".to_string(), + ] + ); + + assert_eq!( + known_breakage_problems( + &log(Vec::new()), + &log(vec![known(/*id*/ 1, &breakage), known(/*id*/ 2, &breakage),]), + &[breakage], + ), + vec!["multiple new known-breakage entries match test/method at params".to_string()] + ); + Ok(()) +} + +#[test] +fn log_metadata_and_json_snapshots_are_validated() { + let invalid = KnownBreakageLog { + version: 2, + breakages: vec![KnownBreakage { + id: 2, + kind: ViolationKind::MethodRemoved, + method: String::new(), + path: String::new(), + before_json: "not json".to_string(), + after_json: "also not json".to_string(), + justification: String::new(), + }], + }; + + assert_eq!( + validate_log("after", &invalid), + vec![ + "after known-breakage log must use version 1".to_string(), + "after known-breakage entry 2 must have id 1".to_string(), + "after known breakage 2 needs a justification".to_string(), + "after known breakage 2 needs a method".to_string(), + "after known breakage 2 needs a path".to_string(), + "after known breakage 2 has invalid JSON in before_json".to_string(), + "after known breakage 2 has invalid JSON in after_json".to_string(), + ] + ); +} + +#[test] +fn templates_are_numbered_after_a_valid_prefix_only() -> Result<()> { + let (_, _, breakage) = type_narrowing()?; + let before = log(vec![old_breakage(/*id*/ 1, "test/old")]); + let valid_after = before.clone(); + let valid_report = failure_report( + &["missing".to_string()], + std::slice::from_ref(&breakage), + &before, + &valid_after, + )?; + assert!(valid_report.contains("[[breakages]]\nid = 2")); + + let invalid_after = log(vec![ + old_breakage(/*id*/ 1, "test/old"), + old_breakage(/*id*/ 2, "test/stale"), + ]); + let invalid_report = + failure_report(&["stale".to_string()], &[breakage], &before, &invalid_after)?; + assert!(!invalid_report.contains("[[breakages]]")); + Ok(()) +} + +fn type_narrowing() -> Result<(ApiSchema, ApiSchema, SchemaBreakage)> { + let before = ApiSchema::parse(&request_schema(json!({ "type": ["null", "string"] })))?; + let after = ApiSchema::parse(&request_schema(json!({ "type": "string" })))?; + let breakage = find_request_narrowing(&before, &after)? + .into_iter() + .next() + .context("expected a type narrowing")?; + Ok((before, after, breakage)) +} + +fn known(id: u64, breakage: &SchemaBreakage) -> KnownBreakage { + KnownBreakage { + justification: "documents the accepted wire-format break".to_string(), + ..KnownBreakage::from_breakage(id, breakage) + } +} + +fn old_breakage(id: u64, method: &str) -> KnownBreakage { + KnownBreakage { + id, + kind: ViolationKind::MethodRemoved, + method: method.to_string(), + path: "request".to_string(), + before_json: "true".to_string(), + after_json: "false".to_string(), + justification: "documents the accepted wire-format break".to_string(), + } +} + +fn log(breakages: Vec) -> KnownBreakageLog { + KnownBreakageLog { + version: KNOWN_BREAKAGE_LOG_VERSION, + breakages, + } +} diff --git a/codex-rs/schema-evolution/src/lib.rs b/codex-rs/schema-evolution/src/lib.rs index a8ef15f421..323f3e3857 100644 --- a/codex-rs/schema-evolution/src/lib.rs +++ b/codex-rs/schema-evolution/src/lib.rs @@ -1,4 +1,5 @@ mod compare; +mod known_breakage; mod model; mod parse; mod violation; @@ -7,6 +8,9 @@ mod violation; mod test_support; use compare::compare_api_schemas; +pub use known_breakage::KnownBreakage; +pub use known_breakage::KnownBreakageLog; +pub use known_breakage::check_request_narrowing; pub(crate) use model::AdditionalItems; pub(crate) use model::AdditionalProperties; pub use model::ApiSchema;