From 53bcfaf42d996d4e93e66c86f41a55b7ff3b56fc Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 20 Feb 2026 17:51:53 -0800 Subject: [PATCH] fix: explicitly list name collisions in JSON schema generation (#12406) ## Why JSON schema codegen was silently resolving naming collisions by appending numeric suffixes (for example `...2`, `...3`). That makes the generated schema names unstable: removing an earlier colliding type can cause a later type to be renumbered, which is a breaking change for consumers that referenced the old generated name. This PR makes those collisions explicit and reviewable. Though note that once we remove `v1` from the codegen, we will no longer support naming collisions. Or rather, naming collisions will have to be handled explicitly rather than the numeric suffix approach. ## What Changed - In `codex-rs/app-server-protocol/src/export.rs`, replaced implicit numeric suffix collision handling for generated variant titles with explicit special-case maps. - Added a panic when a collision occurs without an entry in the map, so new collisions fail loudly instead of silently renaming generated schema types. - Added the currently required special cases so existing generated names remain stable. - Extended the same approach to numbered `definitions` / `$defs` collisions (for example `MessagePhase2`-style names) so those are also explicitly tracked. ## Verification - Ran targeted generator-path test: - `cargo test -p codex-app-server-protocol generate_json_filters_experimental_fields_and_methods -- --nocapture` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12406). * #12408 * __->__ #12406 --- codex-rs/app-server-protocol/src/export.rs | 239 ++++++++++++++++++++- 1 file changed, 235 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index ec51dd158..1a4edfdf5 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -32,10 +32,93 @@ use std::io::Write; use std::path::Path; use std::path::PathBuf; use std::process::Command; +use std::sync::LazyLock; use ts_rs::TS; const HEADER: &str = "// GENERATED CODE! DO NOT MODIFY BY HAND!\n\n"; const IGNORED_DEFINITIONS: &[&str] = &["Option<()>"]; +static VARIANT_TITLE_COLLISION_OVERRIDES: LazyLock> = + LazyLock::new(|| { + HashMap::from([ + ( + "base=McpStartupStatus|generated=StateMcpStartupStatus|literal:state=ready|only_property=state|required_only=state", + "StateMcpStartupStatus2", + ), + ( + "base=McpStartupStatus|generated=StateMcpStartupStatus|literal:state=cancelled|only_property=state|required_only=state", + "StateMcpStartupStatus3", + ), + ]) + }); +static NUMBERED_DEFINITION_COLLISION_OVERRIDES: LazyLock> = + LazyLock::new(|| { + HashMap::from([ + ( + "schema=ClientRequest|container=definitions|generated=AskForApproval2|base=AskForApproval", + "AskForApproval2", + ), + ( + "schema=ClientRequest|container=definitions|generated=NetworkAccess2|base=NetworkAccess", + "NetworkAccess2", + ), + ( + "schema=ClientRequest|container=definitions|generated=ReadOnlyAccess2|base=ReadOnlyAccess", + "ReadOnlyAccess2", + ), + ( + "schema=ClientRequest|container=definitions|generated=SandboxMode2|base=SandboxMode", + "SandboxMode2", + ), + ( + "schema=ClientRequest|container=definitions|generated=SandboxPolicy2|base=SandboxPolicy", + "SandboxPolicy2", + ), + ( + "schema=ServerNotification|container=definitions|generated=ByteRange2|base=ByteRange", + "ByteRange2", + ), + ( + "schema=ServerNotification|container=definitions|generated=CodexErrorInfo2|base=CodexErrorInfo", + "CodexErrorInfo2", + ), + ( + "schema=ServerNotification|container=definitions|generated=CreditsSnapshot2|base=CreditsSnapshot", + "CreditsSnapshot2", + ), + ( + "schema=ServerNotification|container=definitions|generated=MessagePhase2|base=MessagePhase", + "MessagePhase2", + ), + ( + "schema=ServerNotification|container=definitions|generated=ModelRerouteReason2|base=ModelRerouteReason", + "ModelRerouteReason2", + ), + ( + "schema=ServerNotification|container=definitions|generated=PatchApplyStatus2|base=PatchApplyStatus", + "PatchApplyStatus2", + ), + ( + "schema=ServerNotification|container=definitions|generated=RateLimitSnapshot2|base=RateLimitSnapshot", + "RateLimitSnapshot2", + ), + ( + "schema=ServerNotification|container=definitions|generated=RateLimitWindow2|base=RateLimitWindow", + "RateLimitWindow2", + ), + ( + "schema=ServerNotification|container=definitions|generated=TextElement2|base=TextElement", + "TextElement2", + ), + ( + "schema=ServerNotification|container=definitions|generated=UserInput2|base=UserInput", + "UserInput2", + ), + ( + "schema=ServerNotification|container=definitions|generated=WebSearchAction2|base=WebSearchAction", + "WebSearchAction2", + ), + ]) + }); #[derive(Clone)] pub struct GeneratedSchema { @@ -950,6 +1033,7 @@ where let file_stem = name.trim(); let schema = schema_for!(T); let mut schema_value = serde_json::to_value(schema)?; + enforce_numbered_definition_collision_overrides(file_stem, &mut schema_value); annotate_schema(&mut schema_value, Some(file_stem)); // If the name looks like a namespaced path (e.g., "v2::Type"), mirror // the TypeScript layout and write to out_dir/v2/Type.json. Otherwise @@ -980,6 +1064,101 @@ where }) } +fn enforce_numbered_definition_collision_overrides(schema_name: &str, schema: &mut Value) { + for defs_key in ["definitions", "$defs"] { + let renames = { + let Some(defs) = schema.get(defs_key).and_then(Value::as_object) else { + continue; + }; + collect_numbered_definition_collision_renames(schema_name, defs_key, defs) + }; + + if renames.is_empty() { + continue; + } + + #[expect(clippy::expect_used)] + let defs = schema + .get_mut(defs_key) + .and_then(Value::as_object_mut) + .expect("definition map should still be present"); + for (old_name, new_name, collision_key) in &renames { + if old_name == new_name { + continue; + } + assert!( + !defs.contains_key(new_name), + "Numbered definition collision override produced duplicate definition {new_name} for {collision_key}", + ); + let schema_value = defs.remove(old_name).unwrap_or_else(|| { + panic!("Missing generated numbered definition {old_name} for {collision_key}") + }); + defs.insert(new_name.clone(), schema_value); + } + + for (old_name, new_name, _) in renames { + if old_name != new_name { + rewrite_named_local_ref(schema, defs_key, &old_name, &new_name); + } + } + } +} + +fn collect_numbered_definition_collision_renames( + schema_name: &str, + defs_key: &str, + defs: &Map, +) -> Vec<(String, String, String)> { + let mut renames = Vec::new(); + + for generated_name in defs.keys() { + let base_name = generated_name.trim_end_matches(|c: char| c.is_ascii_digit()); + if base_name == generated_name || !defs.contains_key(base_name) { + continue; + } + + let collision_key = format!( + "schema={schema_name}|container={defs_key}|generated={generated_name}|base={base_name}" + ); + let replacement = NUMBERED_DEFINITION_COLLISION_OVERRIDES + .get(collision_key.as_str()) + .map(|value| (*value).to_string()) + .unwrap_or_else(|| { + panic!("Missing numbered definition collision override for {collision_key}") + }); + renames.push((generated_name.clone(), replacement, collision_key)); + } + + renames +} + +fn rewrite_named_local_ref(value: &mut Value, defs_key: &str, old_name: &str, new_name: &str) { + let direct = format!("#/{defs_key}/{old_name}"); + let prefixed = format!("{direct}/"); + let replacement = format!("#/{defs_key}/{new_name}"); + let replacement_prefixed = format!("{replacement}/"); + match value { + Value::Object(obj) => { + if let Some(Value::String(reference)) = obj.get_mut("$ref") { + if reference == &direct { + *reference = replacement; + } else if let Some(rest) = reference.strip_prefix(&prefixed) { + *reference = format!("{replacement_prefixed}{rest}"); + } + } + for child in obj.values_mut() { + rewrite_named_local_ref(child, defs_key, old_name, new_name); + } + } + Value::Array(items) => { + for child in items { + rewrite_named_local_ref(child, defs_key, old_name, new_name); + } + } + _ => {} + } +} + pub(crate) fn write_json_schema(out_dir: &Path, name: &str) -> Result where T: JsonSchema, @@ -1198,10 +1377,20 @@ fn annotate_variant_list(variants: &mut [Value], base: Option<&str>) { && let Some(name) = variant_definition_name(base_name, variant) { let mut candidate = name.clone(); - let mut index = 2; - while seen.contains(&candidate) { - candidate = format!("{name}{index}"); - index += 1; + if seen.contains(&candidate) { + let collision_key = variant_title_collision_key(base_name, &name, variant); + candidate = VARIANT_TITLE_COLLISION_OVERRIDES + .get(collision_key.as_str()) + .map(|value| (*value).to_string()) + .unwrap_or_else(|| { + panic!( + "Missing variant title collision override for {collision_key} (generated name: {name})" + ) + }); + assert!( + !seen.contains(&candidate), + "Variant title collision override produced duplicate title {candidate} for {collision_key}", + ); } if let Some(obj) = variant.as_object_mut() { obj.insert("title".into(), Value::String(candidate.clone())); @@ -1221,6 +1410,48 @@ fn annotate_variant_list(variants: &mut [Value], base: Option<&str>) { } } +fn variant_title_collision_key(base: &str, generated_name: &str, variant: &Value) -> String { + let mut parts = vec![ + format!("base={base}"), + format!("generated={generated_name}"), + ]; + + if let Some(props) = variant.get("properties").and_then(Value::as_object) { + for key in DISCRIMINATOR_KEYS { + if let Some(value) = literal_from_property(props, key) { + parts.push(format!("{key}={value}")); + } + } + for (key, value) in props { + if DISCRIMINATOR_KEYS.contains(&key.as_str()) { + continue; + } + if let Some(literal) = string_literal(value) { + parts.push(format!("literal:{key}={literal}")); + } + } + + if props.len() == 1 + && let Some(key) = props.keys().next() + { + parts.push(format!("only_property={key}")); + } + } + + if let Some(required) = variant.get("required").and_then(Value::as_array) + && required.len() == 1 + && let Some(key) = required[0].as_str() + { + parts.push(format!("required_only={key}")); + } + + if parts.len() == 2 { + parts.push(format!("variant={variant}")); + } + + parts.join("|") +} + const DISCRIMINATOR_KEYS: &[&str] = &["type", "method", "mode", "status", "role", "reason"]; fn set_discriminator_titles(props: &mut Map, owner: &str) {