mirror of
https://github.com/openai/codex.git
synced 2026-04-28 00:25:56 +00:00
ignore v1 in JSON schema codegen (#12408)
## Why The generated unnamespaced JSON envelope schemas (`ClientRequest` and `ServerNotification`) still contained both v1 and v2 variants, which pulled legacy v1/core types and v2 types into the same `definitions` graph. That caused `schemars` to produce numeric suffix names (for example `AskForApproval2`, `ByteRange2`, `MessagePhase2`). This PR moves JSON codegen toward v2-only output while preserving the unnamespaced envelope artifacts, and avoids reintroducing numeric-suffix tolerance by removing the v1/internal-only variants that caused the collisions in those envelope schemas. ## What Changed - In `codex-rs/app-server-protocol/src/export.rs`, JSON generation now excludes v1 schema artifacts (`v1/*`) while continuing to emit unnamespaced/root JSON schemas and the JSON bundle. - Added a narrow JSON v1 allowlist (`JSON_V1_ALLOWLIST`) so `InitializeParams` and `InitializeResponse` are still emitted. - Added JSON-only post-processing for the mixed envelope schemas before collision checks run: - `ClientRequest`: strips v1 request variants from the generated `oneOf` using the temporary `V1_CLIENT_REQUEST_METHODS` list - `ServerNotification`: strips v1 notifications plus the internal-only `rawResponseItem/completed` notification using the temporary `EXCLUDED_SERVER_NOTIFICATION_METHODS_FOR_JSON` list - Added a temporary local-definition pruning pass for those envelope schemas so now-unreferenced v1/core definitions are removed from `definitions` after method filtering. - Updated the variant-title naming heuristic for single-property literal object variants to use the literal value (when available), avoiding collisions like multiple `state`-only variants all deriving the same title. - Collision handling remains fail-fast (no numeric suffix fallback map in this PR path). ## Verification - `just write-app-server-schema` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12408). * __->__ #12408 * #12406
This commit is contained in:
Michael Bolin
@@ -32,93 +32,41 @@ 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<HashMap<&'static str, &'static str>> =
|
||||
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<HashMap<&'static str, &'static str>> =
|
||||
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",
|
||||
),
|
||||
])
|
||||
});
|
||||
const JSON_V1_ALLOWLIST: &[&str] = &["InitializeParams", "InitializeResponse"];
|
||||
const V1_CLIENT_REQUEST_METHODS: &[&str] = &[
|
||||
"newConversation",
|
||||
"getConversationSummary",
|
||||
"listConversations",
|
||||
"resumeConversation",
|
||||
"forkConversation",
|
||||
"archiveConversation",
|
||||
"sendUserMessage",
|
||||
"sendUserTurn",
|
||||
"interruptConversation",
|
||||
"addConversationListener",
|
||||
"removeConversationListener",
|
||||
"gitDiffToRemote",
|
||||
"loginApiKey",
|
||||
"loginChatGpt",
|
||||
"cancelLoginChatGpt",
|
||||
"logoutChatGpt",
|
||||
"getAuthStatus",
|
||||
"getUserSavedConfig",
|
||||
"setDefaultModel",
|
||||
"getUserAgent",
|
||||
"userInfo",
|
||||
"execOneOffCommand",
|
||||
];
|
||||
const EXCLUDED_SERVER_NOTIFICATION_METHODS_FOR_JSON: &[&str] = &[
|
||||
"authStatusChange",
|
||||
"loginChatGptComplete",
|
||||
"sessionConfigured",
|
||||
"rawResponseItem/completed",
|
||||
];
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct GeneratedSchema {
|
||||
@@ -264,6 +212,8 @@ pub fn generate_json_with_experimental(out_dir: &Path, experimental_api: bool) -
|
||||
schemas.extend(export_server_response_schemas(out_dir)?);
|
||||
schemas.extend(export_client_notification_schemas(out_dir)?);
|
||||
schemas.extend(export_server_notification_schemas(out_dir)?);
|
||||
schemas
|
||||
.retain(|schema| !schema.in_v1_dir || JSON_V1_ALLOWLIST.contains(&schema.logical_name()));
|
||||
|
||||
let mut bundle = build_schema_bundle(schemas)?;
|
||||
if !experimental_api {
|
||||
@@ -1031,14 +981,23 @@ where
|
||||
T: JsonSchema,
|
||||
{
|
||||
let file_stem = name.trim();
|
||||
let (raw_namespace, logical_name) = split_namespace(file_stem);
|
||||
let include_in_json_codegen =
|
||||
raw_namespace != Some("v1") || JSON_V1_ALLOWLIST.contains(&logical_name);
|
||||
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 include_in_json_codegen {
|
||||
if file_stem == "ClientRequest" {
|
||||
strip_v1_client_request_variants_from_json_schema(&mut schema_value);
|
||||
} else if file_stem == "ServerNotification" {
|
||||
strip_v1_server_notification_variants_from_json_schema(&mut schema_value);
|
||||
}
|
||||
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
|
||||
// write alongside the legacy files.
|
||||
let (raw_namespace, logical_name) = split_namespace(file_stem);
|
||||
let out_path = if let Some(ns) = raw_namespace {
|
||||
let dir = out_dir.join(ns);
|
||||
ensure_dir(&dir)?;
|
||||
@@ -1047,7 +1006,7 @@ where
|
||||
out_dir.join(format!("{file_stem}.json"))
|
||||
};
|
||||
|
||||
if !IGNORED_DEFINITIONS.contains(&logical_name) {
|
||||
if include_in_json_codegen && !IGNORED_DEFINITIONS.contains(&logical_name) {
|
||||
write_pretty_json(out_path, &schema_value)
|
||||
.with_context(|| format!("Failed to write JSON schema for {file_stem}"))?;
|
||||
}
|
||||
@@ -1066,96 +1025,162 @@ 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() {
|
||||
let Some(defs) = schema.get(defs_key).and_then(Value::as_object) else {
|
||||
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);
|
||||
}
|
||||
}
|
||||
};
|
||||
detect_numbered_definition_collisions(schema_name, defs_key, defs);
|
||||
}
|
||||
}
|
||||
|
||||
fn collect_numbered_definition_collision_renames(
|
||||
fn strip_v1_client_request_variants_from_json_schema(schema: &mut Value) {
|
||||
let v1_methods: HashSet<&str> = V1_CLIENT_REQUEST_METHODS.iter().copied().collect();
|
||||
strip_method_variants_from_json_schema(schema, &v1_methods);
|
||||
}
|
||||
|
||||
fn strip_v1_server_notification_variants_from_json_schema(schema: &mut Value) {
|
||||
let methods: HashSet<&str> = EXCLUDED_SERVER_NOTIFICATION_METHODS_FOR_JSON
|
||||
.iter()
|
||||
.copied()
|
||||
.collect();
|
||||
strip_method_variants_from_json_schema(schema, &methods);
|
||||
}
|
||||
|
||||
fn strip_method_variants_from_json_schema(schema: &mut Value, methods_to_remove: &HashSet<&str>) {
|
||||
{
|
||||
let Some(root) = schema.as_object_mut() else {
|
||||
return;
|
||||
};
|
||||
let Some(Value::Array(variants)) = root.get_mut("oneOf") else {
|
||||
return;
|
||||
};
|
||||
variants.retain(|variant| !is_method_variant_in_set(variant, methods_to_remove));
|
||||
}
|
||||
|
||||
let reachable = reachable_local_definitions(schema, "definitions");
|
||||
let Some(root) = schema.as_object_mut() else {
|
||||
return;
|
||||
};
|
||||
if let Some(definitions) = root.get_mut("definitions").and_then(Value::as_object_mut) {
|
||||
definitions.retain(|name, _| reachable.contains(name));
|
||||
}
|
||||
}
|
||||
|
||||
fn is_method_variant_in_set(value: &Value, methods: &HashSet<&str>) -> bool {
|
||||
let Value::Object(map) = value else {
|
||||
return false;
|
||||
};
|
||||
let Some(properties) = map.get("properties").and_then(Value::as_object) else {
|
||||
return false;
|
||||
};
|
||||
let Some(method_schema) = properties.get("method") else {
|
||||
return false;
|
||||
};
|
||||
let Some(method) = string_literal(method_schema) else {
|
||||
return false;
|
||||
};
|
||||
methods.contains(method)
|
||||
}
|
||||
|
||||
fn reachable_local_definitions(schema: &Value, defs_key: &str) -> HashSet<String> {
|
||||
let Some(definitions) = schema.get(defs_key).and_then(Value::as_object) else {
|
||||
return HashSet::new();
|
||||
};
|
||||
let mut queue: Vec<String> = Vec::new();
|
||||
let mut reachable: HashSet<String> = HashSet::new();
|
||||
|
||||
collect_local_definition_refs_excluding_maps(schema, defs_key, &mut queue, &mut reachable);
|
||||
|
||||
while let Some(name) = queue.pop() {
|
||||
if let Some(def_schema) = definitions.get(&name) {
|
||||
collect_local_definition_refs(def_schema, defs_key, &mut queue, &mut reachable);
|
||||
}
|
||||
}
|
||||
reachable
|
||||
}
|
||||
|
||||
fn collect_local_definition_refs_excluding_maps(
|
||||
value: &Value,
|
||||
defs_key: &str,
|
||||
queue: &mut Vec<String>,
|
||||
reachable: &mut HashSet<String>,
|
||||
) {
|
||||
match value {
|
||||
Value::Object(map) => {
|
||||
for (key, child) in map {
|
||||
if key == defs_key || key == "$defs" || key == "definitions" {
|
||||
continue;
|
||||
}
|
||||
collect_local_definition_refs_excluding_maps(child, defs_key, queue, reachable);
|
||||
}
|
||||
}
|
||||
Value::Array(items) => {
|
||||
for child in items {
|
||||
collect_local_definition_refs_excluding_maps(child, defs_key, queue, reachable);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
collect_local_definition_ref_here(value, defs_key, queue, reachable);
|
||||
}
|
||||
|
||||
fn collect_local_definition_refs(
|
||||
value: &Value,
|
||||
defs_key: &str,
|
||||
queue: &mut Vec<String>,
|
||||
reachable: &mut HashSet<String>,
|
||||
) {
|
||||
collect_local_definition_ref_here(value, defs_key, queue, reachable);
|
||||
match value {
|
||||
Value::Object(map) => {
|
||||
for child in map.values() {
|
||||
collect_local_definition_refs(child, defs_key, queue, reachable);
|
||||
}
|
||||
}
|
||||
Value::Array(items) => {
|
||||
for child in items {
|
||||
collect_local_definition_refs(child, defs_key, queue, reachable);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
fn collect_local_definition_ref_here(
|
||||
value: &Value,
|
||||
defs_key: &str,
|
||||
queue: &mut Vec<String>,
|
||||
reachable: &mut HashSet<String>,
|
||||
) {
|
||||
let Some(reference) = value
|
||||
.as_object()
|
||||
.and_then(|obj| obj.get("$ref"))
|
||||
.and_then(Value::as_str)
|
||||
else {
|
||||
return;
|
||||
};
|
||||
let Some(name) = reference.strip_prefix(&format!("#/{defs_key}/")) else {
|
||||
return;
|
||||
};
|
||||
let name = name.split('/').next().unwrap_or(name);
|
||||
if reachable.insert(name.to_string()) {
|
||||
queue.push(name.to_string());
|
||||
}
|
||||
}
|
||||
|
||||
fn detect_numbered_definition_collisions(
|
||||
schema_name: &str,
|
||||
defs_key: &str,
|
||||
defs: &Map<String, Value>,
|
||||
) -> 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}"
|
||||
panic!(
|
||||
"Numbered definition naming collision detected: 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);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1263,7 +1288,11 @@ fn variant_definition_name(base: &str, variant: &Value) -> Option<String> {
|
||||
if props.len() == 1
|
||||
&& let Some(key) = props.keys().next()
|
||||
{
|
||||
let pascal = to_pascal_case(key);
|
||||
let pascal = props
|
||||
.get(key)
|
||||
.and_then(string_literal)
|
||||
.map(to_pascal_case)
|
||||
.unwrap_or_else(|| to_pascal_case(key));
|
||||
return Some(format!("{pascal}{base}"));
|
||||
}
|
||||
}
|
||||
@@ -1376,20 +1405,11 @@ fn annotate_variant_list(variants: &mut [Value], base: Option<&str>) {
|
||||
&& let Some(base_name) = base
|
||||
&& let Some(name) = variant_definition_name(base_name, variant)
|
||||
{
|
||||
let mut candidate = name.clone();
|
||||
let candidate = name.clone();
|
||||
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}",
|
||||
panic!(
|
||||
"Variant title naming collision detected: {collision_key} (generated name: {name})"
|
||||
);
|
||||
}
|
||||
if let Some(obj) = variant.as_object_mut() {
|
||||
|
||||
Reference in New Issue
Block a user