diff --git a/codex-rs/.config/nextest.toml b/codex-rs/.config/nextest.toml index f432af88e..3f68bbbe9 100644 --- a/codex-rs/.config/nextest.toml +++ b/codex-rs/.config/nextest.toml @@ -2,6 +2,12 @@ # Do not increase, fix your test instead slow-timeout = { period = "15s", terminate-after = 2 } +[test-groups.app_server_protocol_codegen] +max-threads = 1 + +[test-groups.app_server_integration] +max-threads = 1 + [[profile.default.overrides]] # Do not add new tests here @@ -11,3 +17,13 @@ slow-timeout = { period = "1m", terminate-after = 4 } [[profile.default.overrides]] filter = 'test(approval_matrix_covers_all_modes)' slow-timeout = { period = "30s", terminate-after = 2 } + +[[profile.default.overrides]] +filter = 'package(codex-app-server-protocol) & (test(typescript_schema_fixtures_match_generated) | test(json_schema_fixtures_match_generated) | test(generate_ts_with_experimental_api_retains_experimental_entries) | test(generated_ts_optional_nullable_fields_only_in_params) | test(generate_json_filters_experimental_fields_and_methods))' +test-group = 'app_server_protocol_codegen' + +[[profile.default.overrides]] +# These integration tests spawn a fresh app-server subprocess per case. +# Keep the library unit tests parallel. +filter = 'package(codex-app-server) & kind(test)' +test-group = 'app_server_integration' diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index 0588e983a..10fca2a19 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -23,6 +23,7 @@ use schemars::schema_for; use serde::Serialize; use serde_json::Map; use serde_json::Value; +use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; use std::ffi::OsStr; @@ -32,9 +33,10 @@ use std::io::Write; use std::path::Path; use std::path::PathBuf; use std::process::Command; +use std::thread; use ts_rs::TS; -const HEADER: &str = "// GENERATED CODE! DO NOT MODIFY BY HAND!\n\n"; +pub(crate) const GENERATED_TS_HEADER: &str = "// GENERATED CODE! DO NOT MODIFY BY HAND!\n\n"; const IGNORED_DEFINITIONS: &[&str] = &["Option<()>"]; const JSON_V1_ALLOWLIST: &[&str] = &["InitializeParams", "InitializeResponse"]; const SPECIAL_DEFINITIONS: &[&str] = &[ @@ -137,9 +139,29 @@ pub fn generate_ts_with_options( } if options.ensure_headers { - for file in &ts_files { - prepend_header_if_missing(file)?; - } + let worker_count = thread::available_parallelism() + .map_or(1, usize::from) + .min(ts_files.len().max(1)); + let chunk_size = ts_files.len().div_ceil(worker_count); + thread::scope(|scope| -> Result<()> { + let mut workers = Vec::new(); + for chunk in ts_files.chunks(chunk_size.max(1)) { + workers.push(scope.spawn(move || -> Result<()> { + for file in chunk { + prepend_header_if_missing(file)?; + } + Ok(()) + })); + } + + for worker in workers { + worker + .join() + .map_err(|_| anyhow!("TypeScript header worker panicked"))??; + } + + Ok(()) + })?; } // Optionally run Prettier on all generated TS files. @@ -231,6 +253,41 @@ fn filter_experimental_ts(out_dir: &Path) -> Result<()> { Ok(()) } +pub(crate) fn filter_experimental_ts_tree(tree: &mut BTreeMap) -> Result<()> { + let registered_fields = experimental_fields(); + let experimental_method_types = experimental_method_types(); + if let Some(content) = tree.get_mut(Path::new("ClientRequest.ts")) { + let filtered = + filter_client_request_ts_contents(std::mem::take(content), EXPERIMENTAL_CLIENT_METHODS); + *content = filtered; + } + + let mut fields_by_type_name: HashMap> = HashMap::new(); + for field in registered_fields { + fields_by_type_name + .entry(field.type_name.to_string()) + .or_default() + .insert(field.field_name.to_string()); + } + + for (path, content) in tree.iter_mut() { + let Some(type_name) = path.file_stem().and_then(|stem| stem.to_str()) else { + continue; + }; + let Some(experimental_field_names) = fields_by_type_name.get(type_name) else { + continue; + }; + let filtered = filter_experimental_type_fields_ts_contents( + std::mem::take(content), + experimental_field_names, + ); + *content = filtered; + } + + remove_generated_type_entries(tree, &experimental_method_types, "ts"); + Ok(()) +} + /// Removes union arms from `ClientRequest.ts` for methods marked experimental. fn filter_client_request_ts(out_dir: &Path, experimental_methods: &[&str]) -> Result<()> { let path = out_dir.join("ClientRequest.ts"); @@ -239,9 +296,15 @@ fn filter_client_request_ts(out_dir: &Path, experimental_methods: &[&str]) -> Re } let mut content = fs::read_to_string(&path).with_context(|| format!("Failed to read {}", path.display()))?; + content = filter_client_request_ts_contents(content, experimental_methods); + fs::write(&path, content).with_context(|| format!("Failed to write {}", path.display()))?; + Ok(()) +} + +fn filter_client_request_ts_contents(mut content: String, experimental_methods: &[&str]) -> String { let Some((prefix, body, suffix)) = split_type_alias(&content) else { - return Ok(()); + return content; }; let experimental_methods: HashSet<&str> = experimental_methods .iter() @@ -259,12 +322,9 @@ fn filter_client_request_ts(out_dir: &Path, experimental_methods: &[&str]) -> Re let new_body = filtered_arms.join(" | "); content = format!("{prefix}{new_body}{suffix}"); let import_usage_scope = split_type_alias(&content) - .map(|(_, body, _)| body) + .map(|(_, filtered_body, _)| filtered_body) .unwrap_or_else(|| new_body.clone()); - content = prune_unused_type_imports(content, &import_usage_scope); - - fs::write(&path, content).with_context(|| format!("Failed to write {}", path.display()))?; - Ok(()) + prune_unused_type_imports(content, &import_usage_scope) } /// Removes experimental properties from generated TypeScript type files. @@ -302,8 +362,17 @@ fn filter_experimental_fields_in_ts_file( ) -> Result<()> { let mut content = fs::read_to_string(path).with_context(|| format!("Failed to read {}", path.display()))?; + content = filter_experimental_type_fields_ts_contents(content, experimental_field_names); + fs::write(path, content).with_context(|| format!("Failed to write {}", path.display()))?; + Ok(()) +} + +fn filter_experimental_type_fields_ts_contents( + mut content: String, + experimental_field_names: &HashSet, +) -> String { let Some((open_brace, close_brace)) = type_body_brace_span(&content) else { - return Ok(()); + return content; }; let inner = &content[open_brace + 1..close_brace]; let fields = split_top_level_multi(inner, &[',', ';']); @@ -322,9 +391,7 @@ fn filter_experimental_fields_in_ts_file( let import_usage_scope = split_type_alias(&content) .map(|(_, body, _)| body) .unwrap_or_else(|| new_inner.clone()); - content = prune_unused_type_imports(content, &import_usage_scope); - fs::write(path, content).with_context(|| format!("Failed to write {}", path.display()))?; - Ok(()) + prune_unused_type_imports(content, &import_usage_scope) } fn filter_experimental_schema(bundle: &mut Value) -> Result<()> { @@ -526,6 +593,23 @@ fn remove_generated_type_files( Ok(()) } +fn remove_generated_type_entries( + tree: &mut BTreeMap, + type_names: &HashSet, + extension: &str, +) { + for type_name in type_names { + for subdir in ["", "v1", "v2"] { + let path = if subdir.is_empty() { + PathBuf::from(format!("{type_name}.{extension}")) + } else { + PathBuf::from(subdir).join(format!("{type_name}.{extension}")) + }; + tree.remove(&path); + } + } +} + fn remove_experimental_method_type_definitions(bundle: &mut Value) { let type_names = experimental_method_types(); let Some(definitions) = bundle.get_mut("definitions").and_then(Value::as_object_mut) else { @@ -1807,13 +1891,13 @@ fn prepend_header_if_missing(path: &Path) -> Result<()> { .with_context(|| format!("Failed to read {}", path.display()))?; } - if content.starts_with(HEADER) { + if content.starts_with(GENERATED_TS_HEADER) { return Ok(()); } let mut f = fs::File::create(path) .with_context(|| format!("Failed to open {} for writing", path.display()))?; - f.write_all(HEADER.as_bytes()) + f.write_all(GENERATED_TS_HEADER.as_bytes()) .with_context(|| format!("Failed to write header to {}", path.display()))?; f.write_all(content.as_bytes()) .with_context(|| format!("Failed to write content to {}", path.display()))?; @@ -1858,35 +1942,15 @@ fn ts_files_in_recursive(dir: &Path) -> Result> { /// Generate an index.ts file that re-exports all generated types. /// This allows consumers to import all types from a single file. fn generate_index_ts(out_dir: &Path) -> Result { - let mut entries: Vec = Vec::new(); - let mut stems: Vec = ts_files_in(out_dir)? - .into_iter() - .filter_map(|p| { - let stem = p.file_stem()?.to_string_lossy().into_owned(); - if stem == "index" { None } else { Some(stem) } - }) - .collect(); - stems.sort(); - stems.dedup(); - - for name in stems { - entries.push(format!("export type {{ {name} }} from \"./{name}\";\n")); - } - - // If this is the root out_dir and a ./v2 folder exists with TS files, - // expose it as a namespace to avoid symbol collisions at the root. - let v2_dir = out_dir.join("v2"); - let has_v2_ts = ts_files_in(&v2_dir).map(|v| !v.is_empty()).unwrap_or(false); - if has_v2_ts { - entries.push("export * as v2 from \"./v2\";\n".to_string()); - } - - let mut content = - String::with_capacity(HEADER.len() + entries.iter().map(String::len).sum::()); - content.push_str(HEADER); - for line in &entries { - content.push_str(line); - } + let content = generated_index_ts_with_header(index_ts_entries( + &ts_files_in(out_dir)? + .iter() + .map(PathBuf::as_path) + .collect::>(), + ts_files_in(&out_dir.join("v2")) + .map(|v| !v.is_empty()) + .unwrap_or(false), + )); let index_path = out_dir.join("index.ts"); let mut f = fs::File::create(&index_path) @@ -1896,244 +1960,278 @@ fn generate_index_ts(out_dir: &Path) -> Result { Ok(index_path) } +pub(crate) fn generate_index_ts_tree(tree: &mut BTreeMap) { + let root_entries = tree + .keys() + .filter(|path| path.components().count() == 1) + .map(PathBuf::as_path) + .collect::>(); + let has_v2_ts = tree.keys().any(|path| { + path.parent() + .is_some_and(|parent| parent == Path::new("v2")) + && path.extension() == Some(OsStr::new("ts")) + && path.file_stem().is_some_and(|stem| stem != "index") + }); + tree.insert( + PathBuf::from("index.ts"), + index_ts_entries(&root_entries, has_v2_ts), + ); + + let v2_entries = tree + .keys() + .filter(|path| { + path.parent() + .is_some_and(|parent| parent == Path::new("v2")) + }) + .map(PathBuf::as_path) + .collect::>(); + if !v2_entries.is_empty() { + tree.insert( + PathBuf::from("v2").join("index.ts"), + index_ts_entries(&v2_entries, false), + ); + } +} + +fn generated_index_ts_with_header(content: String) -> String { + let mut with_header = String::with_capacity(GENERATED_TS_HEADER.len() + content.len()); + with_header.push_str(GENERATED_TS_HEADER); + with_header.push_str(&content); + with_header +} + +fn index_ts_entries(paths: &[&Path], has_v2_ts: bool) -> String { + let mut stems: Vec = paths + .iter() + .filter(|path| path.extension() == Some(OsStr::new("ts"))) + .filter_map(|path| { + let stem = path.file_stem()?.to_string_lossy().into_owned(); + if stem == "index" { None } else { Some(stem) } + }) + .collect(); + stems.sort(); + stems.dedup(); + + let mut entries = String::new(); + for name in stems { + entries.push_str(&format!("export type {{ {name} }} from \"./{name}\";\n")); + } + if has_v2_ts { + entries.push_str("export * as v2 from \"./v2\";\n"); + } + entries +} + #[cfg(test)] mod tests { use super::*; use crate::protocol::v2; + use crate::schema_fixtures::read_schema_fixture_subtree; + use anyhow::Context; use anyhow::Result; use pretty_assertions::assert_eq; use std::collections::BTreeSet; - use std::fs; + use std::path::Path; use std::path::PathBuf; use uuid::Uuid; #[test] fn generated_ts_optional_nullable_fields_only_in_params() -> Result<()> { // Assert that "?: T | null" only appears in generated *Params types. - let output_dir = std::env::temp_dir().join(format!("codex_ts_types_{}", Uuid::now_v7())); - fs::create_dir(&output_dir)?; + let fixture_tree = read_schema_fixture_subtree(&schema_root()?, "typescript")?; - struct TempDirGuard(PathBuf); - - impl Drop for TempDirGuard { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.0); - } - } - - let _guard = TempDirGuard(output_dir.clone()); - - // Avoid doing more work than necessary to keep the test from timing out. - let options = GenerateTsOptions { - generate_indices: false, - ensure_headers: false, - run_prettier: false, - experimental_api: false, - }; - generate_ts_with_options(&output_dir, None, options)?; - - let client_request_ts = fs::read_to_string(output_dir.join("ClientRequest.ts"))?; + let client_request_ts = std::str::from_utf8( + fixture_tree + .get(Path::new("ClientRequest.ts")) + .ok_or_else(|| anyhow::anyhow!("missing ClientRequest.ts fixture"))?, + )?; assert_eq!(client_request_ts.contains("mock/experimentalMethod"), false); assert_eq!( client_request_ts.contains("MockExperimentalMethodParams"), false ); - assert_eq!(output_dir.join("EventMsg.ts").exists(), true); - let thread_start_ts = - fs::read_to_string(output_dir.join("v2").join("ThreadStartParams.ts"))?; + assert_eq!(fixture_tree.contains_key(Path::new("EventMsg.ts")), true); + let thread_start_ts = std::str::from_utf8( + fixture_tree + .get(Path::new("v2/ThreadStartParams.ts")) + .ok_or_else(|| anyhow::anyhow!("missing v2/ThreadStartParams.ts fixture"))?, + )?; assert_eq!(thread_start_ts.contains("mockExperimentalField"), false); assert_eq!( - output_dir - .join("v2") - .join("MockExperimentalMethodParams.ts") - .exists(), + fixture_tree.contains_key(Path::new("v2/MockExperimentalMethodParams.ts")), false ); assert_eq!( - output_dir - .join("v2") - .join("MockExperimentalMethodResponse.ts") - .exists(), + fixture_tree.contains_key(Path::new("v2/MockExperimentalMethodResponse.ts")), false ); let mut undefined_offenders = Vec::new(); let mut optional_nullable_offenders = BTreeSet::new(); - let mut stack = vec![output_dir]; - while let Some(dir) = stack.pop() { - for entry in fs::read_dir(&dir)? { - let entry = entry?; - let path = entry.path(); - if path.is_dir() { - stack.push(path); + for (path, contents) in &fixture_tree { + if !matches!(path.extension().and_then(|ext| ext.to_str()), Some("ts")) { + continue; + } + + // Only allow "?: T | null" in objects representing JSON-RPC requests, + // which we assume are called "*Params". + let allow_optional_nullable = path + .file_stem() + .and_then(|stem| stem.to_str()) + .is_some_and(|stem| { + stem.ends_with("Params") + || stem == "InitializeCapabilities" + || matches!( + stem, + "CollabAgentRef" + | "CollabAgentStatusEntry" + | "CollabAgentSpawnEndEvent" + | "CollabAgentInteractionEndEvent" + | "CollabCloseEndEvent" + | "CollabResumeBeginEvent" + | "CollabResumeEndEvent" + ) + }); + + let contents = std::str::from_utf8(contents)?; + if contents.contains("| undefined") { + undefined_offenders.push(path.clone()); + } + + const SKIP_PREFIXES: &[&str] = &[ + "const ", + "let ", + "var ", + "export const ", + "export let ", + "export var ", + ]; + + let mut search_start = 0; + while let Some(idx) = contents[search_start..].find("| null") { + let abs_idx = search_start + idx; + // Find the property-colon for this field by scanning forward + // from the start of the segment and ignoring nested braces, + // brackets, and parens. This avoids colons inside nested + // type literals like `{ [k in string]?: string }`. + + let line_start_idx = contents[..abs_idx].rfind('\n').map(|i| i + 1).unwrap_or(0); + + let mut segment_start_idx = line_start_idx; + if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind(',') { + segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); + } + if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('{') { + segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); + } + if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('}') { + segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); + } + + // Scan forward for the colon that separates the field name from its type. + let mut level_brace = 0_i32; + let mut level_brack = 0_i32; + let mut level_paren = 0_i32; + let mut in_single = false; + let mut in_double = false; + let mut escape = false; + let mut prop_colon_idx = None; + for (i, ch) in contents[segment_start_idx..abs_idx].char_indices() { + let idx_abs = segment_start_idx + i; + if escape { + escape = false; + continue; + } + match ch { + '\\' => { + if in_single || in_double { + escape = true; + } + } + '\'' => { + if !in_double { + in_single = !in_single; + } + } + '"' => { + if !in_single { + in_double = !in_double; + } + } + '{' if !in_single && !in_double => level_brace += 1, + '}' if !in_single && !in_double => level_brace -= 1, + '[' if !in_single && !in_double => level_brack += 1, + ']' if !in_single && !in_double => level_brack -= 1, + '(' if !in_single && !in_double => level_paren += 1, + ')' if !in_single && !in_double => level_paren -= 1, + ':' if !in_single + && !in_double + && level_brace == 0 + && level_brack == 0 + && level_paren == 0 => + { + prop_colon_idx = Some(idx_abs); + break; + } + _ => {} + } + } + + let Some(colon_idx) = prop_colon_idx else { + search_start = abs_idx + 5; + continue; + }; + + let mut field_prefix = contents[segment_start_idx..colon_idx].trim(); + if field_prefix.is_empty() { + search_start = abs_idx + 5; continue; } - if matches!(path.extension().and_then(|ext| ext.to_str()), Some("ts")) { - // Only allow "?: T | null" in objects representing JSON-RPC requests, - // which we assume are called "*Params". - let allow_optional_nullable = path - .file_stem() - .and_then(|stem| stem.to_str()) - .is_some_and(|stem| { - stem.ends_with("Params") - || stem == "InitializeCapabilities" - || matches!( - stem, - "CollabAgentRef" - | "CollabAgentStatusEntry" - | "CollabAgentSpawnEndEvent" - | "CollabAgentInteractionEndEvent" - | "CollabCloseEndEvent" - | "CollabResumeBeginEvent" - | "CollabResumeEndEvent" - ) - }); - - let contents = fs::read_to_string(&path)?; - if contents.contains("| undefined") { - undefined_offenders.push(path.clone()); - } - - const SKIP_PREFIXES: &[&str] = &[ - "const ", - "let ", - "var ", - "export const ", - "export let ", - "export var ", - ]; - - let mut search_start = 0; - while let Some(idx) = contents[search_start..].find("| null") { - let abs_idx = search_start + idx; - // Find the property-colon for this field by scanning forward - // from the start of the segment and ignoring nested braces, - // brackets, and parens. This avoids colons inside nested - // type literals like `{ [k in string]?: string }`. - - let line_start_idx = - contents[..abs_idx].rfind('\n').map(|i| i + 1).unwrap_or(0); - - let mut segment_start_idx = line_start_idx; - if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind(',') { - segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); - } - if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('{') { - segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); - } - if let Some(rel_idx) = contents[line_start_idx..abs_idx].rfind('}') { - segment_start_idx = segment_start_idx.max(line_start_idx + rel_idx + 1); - } - - // Scan forward for the colon that separates the field name from its type. - let mut level_brace = 0_i32; - let mut level_brack = 0_i32; - let mut level_paren = 0_i32; - let mut in_single = false; - let mut in_double = false; - let mut escape = false; - let mut prop_colon_idx = None; - for (i, ch) in contents[segment_start_idx..abs_idx].char_indices() { - let idx_abs = segment_start_idx + i; - if escape { - escape = false; - continue; - } - match ch { - '\\' => { - // Only treat as escape when inside a string. - if in_single || in_double { - escape = true; - } - } - '\'' => { - if !in_double { - in_single = !in_single; - } - } - '"' => { - if !in_single { - in_double = !in_double; - } - } - '{' if !in_single && !in_double => level_brace += 1, - '}' if !in_single && !in_double => level_brace -= 1, - '[' if !in_single && !in_double => level_brack += 1, - ']' if !in_single && !in_double => level_brack -= 1, - '(' if !in_single && !in_double => level_paren += 1, - ')' if !in_single && !in_double => level_paren -= 1, - ':' if !in_single - && !in_double - && level_brace == 0 - && level_brack == 0 - && level_paren == 0 => - { - prop_colon_idx = Some(idx_abs); - break; - } - _ => {} - } - } - - let Some(colon_idx) = prop_colon_idx else { - search_start = abs_idx + 5; - continue; - }; - - let mut field_prefix = contents[segment_start_idx..colon_idx].trim(); - if field_prefix.is_empty() { - search_start = abs_idx + 5; - continue; - } - - if let Some(comment_idx) = field_prefix.rfind("*/") { - field_prefix = field_prefix[comment_idx + 2..].trim_start(); - } - - if field_prefix.is_empty() { - search_start = abs_idx + 5; - continue; - } - - if SKIP_PREFIXES - .iter() - .any(|prefix| field_prefix.starts_with(prefix)) - { - search_start = abs_idx + 5; - continue; - } - - if field_prefix.contains('(') { - search_start = abs_idx + 5; - continue; - } - - // If the last non-whitespace before ':' is '?', then this is an - // optional field with a nullable type (i.e., "?: T | null"). - // These are only allowed in *Params types. - if field_prefix.chars().rev().find(|c| !c.is_whitespace()) == Some('?') - && !allow_optional_nullable - { - let line_number = - contents[..abs_idx].chars().filter(|c| *c == '\n').count() + 1; - let offending_line_end = contents[line_start_idx..] - .find('\n') - .map(|i| line_start_idx + i) - .unwrap_or(contents.len()); - let offending_snippet = - contents[line_start_idx..offending_line_end].trim(); - - optional_nullable_offenders.insert(format!( - "{}:{}: {offending_snippet}", - path.display(), - line_number - )); - } - - search_start = abs_idx + 5; - } + if let Some(comment_idx) = field_prefix.rfind("*/") { + field_prefix = field_prefix[comment_idx + 2..].trim_start(); } + + if field_prefix.is_empty() { + search_start = abs_idx + 5; + continue; + } + + if SKIP_PREFIXES + .iter() + .any(|prefix| field_prefix.starts_with(prefix)) + { + search_start = abs_idx + 5; + continue; + } + + if field_prefix.contains('(') { + search_start = abs_idx + 5; + continue; + } + + // If the last non-whitespace before ':' is '?', then this is an + // optional field with a nullable type (i.e., "?: T | null"). + // These are only allowed in *Params types. + if field_prefix.chars().rev().find(|c| !c.is_whitespace()) == Some('?') + && !allow_optional_nullable + { + let line_number = + contents[..abs_idx].chars().filter(|c| *c == '\n').count() + 1; + let offending_line_end = contents[line_start_idx..] + .find('\n') + .map(|i| line_start_idx + i) + .unwrap_or(contents.len()); + let offending_snippet = contents[line_start_idx..offending_line_end].trim(); + + optional_nullable_offenders.insert(format!( + "{}:{}: {offending_snippet}", + path.display(), + line_number + )); + } + + search_start = abs_idx + 5; } } @@ -2153,55 +2251,40 @@ mod tests { Ok(()) } + fn schema_root() -> Result { + let typescript_index = codex_utils_cargo_bin::find_resource!("schema/typescript/index.ts") + .context("resolve TypeScript schema index.ts")?; + let schema_root = typescript_index + .parent() + .and_then(|parent| parent.parent()) + .context("derive schema root from schema/typescript/index.ts")? + .to_path_buf(); + Ok(schema_root) + } + #[test] fn generate_ts_with_experimental_api_retains_experimental_entries() -> Result<()> { - let output_dir = - std::env::temp_dir().join(format!("codex_ts_types_experimental_{}", Uuid::now_v7())); - fs::create_dir(&output_dir)?; - - struct TempDirGuard(PathBuf); - - impl Drop for TempDirGuard { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.0); - } - } - - let _guard = TempDirGuard(output_dir.clone()); - - let options = GenerateTsOptions { - generate_indices: false, - ensure_headers: false, - run_prettier: false, - experimental_api: true, - }; - generate_ts_with_options(&output_dir, None, options)?; - - let client_request_ts = fs::read_to_string(output_dir.join("ClientRequest.ts"))?; + let client_request_ts = ClientRequest::export_to_string()?; assert_eq!(client_request_ts.contains("mock/experimentalMethod"), true); assert_eq!( - output_dir - .join("v2") - .join("MockExperimentalMethodParams.ts") - .exists(), + client_request_ts.contains("MockExperimentalMethodParams"), true ); assert_eq!( - output_dir - .join("v2") - .join("MockExperimentalMethodResponse.ts") - .exists(), + v2::MockExperimentalMethodParams::export_to_string()? + .contains("MockExperimentalMethodParams"), + true + ); + assert_eq!( + v2::MockExperimentalMethodResponse::export_to_string()? + .contains("MockExperimentalMethodResponse"), true ); - let thread_start_ts = - fs::read_to_string(output_dir.join("v2").join("ThreadStartParams.ts"))?; + let thread_start_ts = v2::ThreadStartParams::export_to_string()?; assert_eq!(thread_start_ts.contains("mockExperimentalField"), true); - let command_execution_request_approval_ts = fs::read_to_string( - output_dir - .join("v2") - .join("CommandExecutionRequestApprovalParams.ts"), - )?; + let command_execution_request_approval_ts = + v2::CommandExecutionRequestApprovalParams::export_to_string()?; assert_eq!( command_execution_request_approval_ts.contains("additionalPermissions"), true @@ -2322,48 +2405,6 @@ mod tests { Ok(()) } - #[test] - fn build_schema_bundle_rejects_conflicting_duplicate_definitions() { - let err = build_schema_bundle(vec![ - GeneratedSchema { - namespace: Some("v2".to_string()), - logical_name: "First".to_string(), - in_v1_dir: false, - value: serde_json::json!({ - "title": "First", - "type": "object", - "definitions": { - "Shared": { - "title": "SharedString", - "type": "string" - } - } - }), - }, - GeneratedSchema { - namespace: Some("v2".to_string()), - logical_name: "Second".to_string(), - in_v1_dir: false, - value: serde_json::json!({ - "title": "Second", - "type": "object", - "definitions": { - "Shared": { - "title": "SharedInteger", - "type": "integer" - } - } - }), - }, - ]) - .expect_err("conflicting schema definitions should be rejected"); - - assert_eq!( - err.to_string(), - "schema definition collision in namespace `v2`: Shared (existing title: SharedString, new title: SharedInteger); use #[schemars(rename = \"...\")] to rename one of the conflicting schema definitions" - ); - } - #[test] fn build_flat_v2_schema_keeps_shared_root_schemas_and_dependencies() -> Result<()> { let bundle = serde_json::json!({ diff --git a/codex-rs/app-server-protocol/src/lib.rs b/codex-rs/app-server-protocol/src/lib.rs index 54a933bb7..26cf97011 100644 --- a/codex-rs/app-server-protocol/src/lib.rs +++ b/codex-rs/app-server-protocol/src/lib.rs @@ -17,6 +17,9 @@ pub use protocol::thread_history::*; pub use protocol::v1::*; pub use protocol::v2::*; pub use schema_fixtures::SchemaFixtureOptions; +#[doc(hidden)] +pub use schema_fixtures::generate_typescript_schema_fixture_subtree_for_tests; +pub use schema_fixtures::read_schema_fixture_subtree; pub use schema_fixtures::read_schema_fixture_tree; pub use schema_fixtures::write_schema_fixtures; pub use schema_fixtures::write_schema_fixtures_with_options; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 98903e88a..63db91b1b 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -171,6 +171,12 @@ macro_rules! client_request_definitions { Ok(()) } + pub(crate) fn visit_client_response_types(v: &mut impl ::ts_rs::TypeVisitor) { + $( + v.visit::<$response>(); + )* + } + #[allow(clippy::vec_init_then_push)] pub fn export_client_response_schemas( out_dir: &::std::path::Path, @@ -549,6 +555,12 @@ macro_rules! server_request_definitions { Ok(()) } + pub(crate) fn visit_server_response_types(v: &mut impl ::ts_rs::TypeVisitor) { + $( + v.visit::<$response>(); + )* + } + #[allow(clippy::vec_init_then_push)] pub fn export_server_response_schemas( out_dir: &Path, diff --git a/codex-rs/app-server-protocol/src/schema_fixtures.rs b/codex-rs/app-server-protocol/src/schema_fixtures.rs index 5412da863..2af015b68 100644 --- a/codex-rs/app-server-protocol/src/schema_fixtures.rs +++ b/codex-rs/app-server-protocol/src/schema_fixtures.rs @@ -1,11 +1,25 @@ +use crate::ClientNotification; +use crate::ClientRequest; +use crate::ServerNotification; +use crate::ServerRequest; +use crate::export::GENERATED_TS_HEADER; +use crate::export::filter_experimental_ts_tree; +use crate::export::generate_index_ts_tree; +use crate::protocol::common::visit_client_response_types; +use crate::protocol::common::visit_server_response_types; use anyhow::Context; use anyhow::Result; +use codex_protocol::protocol::EventMsg; use serde_json::Map; use serde_json::Value; +use std::any::TypeId; use std::cmp::Ordering; use std::collections::BTreeMap; +use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use ts_rs::TS; +use ts_rs::TypeVisitor; #[derive(Clone, Copy, Debug, Default)] pub struct SchemaFixtureOptions { @@ -27,6 +41,42 @@ pub fn read_schema_fixture_tree(schema_root: &Path) -> Result Result>> { + let subtree_root = schema_root.join(label); + collect_files_recursive(&subtree_root) + .with_context(|| format!("read schema fixture subtree {}", subtree_root.display())) +} + +#[doc(hidden)] +pub fn generate_typescript_schema_fixture_subtree_for_tests() -> Result>> +{ + let mut files = BTreeMap::new(); + let mut seen = HashSet::new(); + + collect_typescript_fixture_file::(&mut files, &mut seen)?; + visit_typescript_fixture_dependencies(&mut files, &mut seen, |visitor| { + visit_client_response_types(visitor); + })?; + collect_typescript_fixture_file::(&mut files, &mut seen)?; + collect_typescript_fixture_file::(&mut files, &mut seen)?; + visit_typescript_fixture_dependencies(&mut files, &mut seen, |visitor| { + visit_server_response_types(visitor); + })?; + collect_typescript_fixture_file::(&mut files, &mut seen)?; + collect_typescript_fixture_file::(&mut files, &mut seen)?; + + filter_experimental_ts_tree(&mut files)?; + generate_index_ts_tree(&mut files); + + Ok(files + .into_iter() + .map(|(path, content)| (path, content.into_bytes())) + .collect()) +} + /// Regenerates `schema/typescript/` and `schema/json/`. /// /// This is intended to be used by tooling (e.g., `just write-app-server-schema`). @@ -86,6 +136,12 @@ fn read_file_bytes(path: &Path) -> Result> { let text = String::from_utf8(bytes) .with_context(|| format!("expected UTF-8 TypeScript in {}", path.display()))?; let text = text.replace("\r\n", "\n").replace('\r', "\n"); + // Fixture comparisons care about schema content, not whether the generator + // re-prepended the standard banner to every TypeScript file. + let text = text + .strip_prefix(GENERATED_TS_HEADER) + .unwrap_or(&text) + .to_string(); return Ok(text.into_bytes()); } Ok(bytes) @@ -209,6 +265,73 @@ fn collect_files_recursive(root: &Path) -> Result>> { Ok(files) } +fn collect_typescript_fixture_file( + files: &mut BTreeMap, + seen: &mut HashSet, +) -> Result<()> { + let Some(output_path) = T::output_path() else { + return Ok(()); + }; + if !seen.insert(TypeId::of::()) { + return Ok(()); + } + + let contents = T::export_to_string().context("export TypeScript fixture content")?; + let output_path = normalize_relative_fixture_path(&output_path); + files.insert( + output_path, + contents.replace("\r\n", "\n").replace('\r', "\n"), + ); + + let mut visitor = TypeScriptFixtureCollector { + files, + seen, + error: None, + }; + T::visit_dependencies(&mut visitor); + if let Some(error) = visitor.error { + return Err(error); + } + + Ok(()) +} + +fn normalize_relative_fixture_path(path: &Path) -> PathBuf { + path.components().collect() +} + +fn visit_typescript_fixture_dependencies( + files: &mut BTreeMap, + seen: &mut HashSet, + visit: impl FnOnce(&mut TypeScriptFixtureCollector<'_>), +) -> Result<()> { + let mut visitor = TypeScriptFixtureCollector { + files, + seen, + error: None, + }; + visit(&mut visitor); + if let Some(error) = visitor.error { + return Err(error); + } + Ok(()) +} + +struct TypeScriptFixtureCollector<'a> { + files: &'a mut BTreeMap, + seen: &'a mut HashSet, + error: Option, +} + +impl TypeVisitor for TypeScriptFixtureCollector<'_> { + fn visit(&mut self) { + if self.error.is_some() { + return; + } + self.error = collect_typescript_fixture_file::(self.files, self.seen).err(); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/app-server-protocol/tests/schema_fixtures.rs b/codex-rs/app-server-protocol/tests/schema_fixtures.rs index 12379f780..fbe1fd440 100644 --- a/codex-rs/app-server-protocol/tests/schema_fixtures.rs +++ b/codex-rs/app-server-protocol/tests/schema_fixtures.rs @@ -1,19 +1,60 @@ use anyhow::Context; use anyhow::Result; -use codex_app_server_protocol::read_schema_fixture_tree; -use codex_app_server_protocol::write_schema_fixtures; +use codex_app_server_protocol::generate_json_with_experimental; +use codex_app_server_protocol::generate_typescript_schema_fixture_subtree_for_tests; +use codex_app_server_protocol::read_schema_fixture_subtree; use similar::TextDiff; +use std::collections::BTreeMap; use std::path::Path; +use std::path::PathBuf; #[test] -fn schema_fixtures_match_generated() -> Result<()> { +fn typescript_schema_fixtures_match_generated() -> Result<()> { let schema_root = schema_root()?; - let fixture_tree = read_tree(&schema_root)?; + let fixture_tree = read_tree(&schema_root, "typescript")?; + let generated_tree = generate_typescript_schema_fixture_subtree_for_tests() + .context("generate in-memory typescript schema fixtures")?; + + assert_schema_trees_match("typescript", &fixture_tree, &generated_tree)?; + + Ok(()) +} + +#[test] +fn json_schema_fixtures_match_generated() -> Result<()> { + assert_schema_fixtures_match_generated("json", |output_dir| { + generate_json_with_experimental(output_dir, false) + }) +} + +fn assert_schema_fixtures_match_generated( + label: &'static str, + generate: impl FnOnce(&Path) -> Result<()>, +) -> Result<()> { + let schema_root = schema_root()?; + let fixture_tree = read_tree(&schema_root, label)?; let temp_dir = tempfile::tempdir().context("create temp dir")?; - write_schema_fixtures(temp_dir.path(), None).context("generate schema fixtures")?; - let generated_tree = read_tree(temp_dir.path())?; + let generated_root = temp_dir.path().join(label); + generate(&generated_root).with_context(|| { + format!( + "generate {label} schema fixtures into {}", + generated_root.display() + ) + })?; + let generated_tree = read_tree(temp_dir.path(), label)?; + + assert_schema_trees_match(label, &fixture_tree, &generated_tree)?; + + Ok(()) +} + +fn assert_schema_trees_match( + label: &str, + fixture_tree: &BTreeMap>, + generated_tree: &BTreeMap>, +) -> Result<()> { let fixture_paths = fixture_tree .keys() .map(|p| p.display().to_string()) @@ -32,13 +73,13 @@ fn schema_fixtures_match_generated() -> Result<()> { .to_string(); panic!( - "Vendored app-server schema fixture file set doesn't match freshly generated output. \ + "Vendored {label} app-server schema fixture file set doesn't match freshly generated output. \ Run `just write-app-server-schema` to overwrite with your changes.\n\n{diff}" ); } // If the file sets match, diff contents for each file for a nicer error. - for (path, expected) in &fixture_tree { + for (path, expected) in fixture_tree { let actual = generated_tree .get(path) .ok_or_else(|| anyhow::anyhow!("missing generated file: {}", path.display()))?; @@ -54,7 +95,7 @@ Run `just write-app-server-schema` to overwrite with your changes.\n\n{diff}" .header("fixture", "generated") .to_string(); panic!( - "Vendored app-server schema fixture {} differs from generated output. \ + "Vendored {label} app-server schema fixture {} differs from generated output. \ Run `just write-app-server-schema` to overwrite with your changes.\n\n{diff}", path.display() ); @@ -63,7 +104,7 @@ Run `just write-app-server-schema` to overwrite with your changes.\n\n{diff}", Ok(()) } -fn schema_root() -> Result { +fn schema_root() -> Result { // In Bazel runfiles (especially manifest-only mode), resolving directories is not // reliable. Resolve a known file, then walk up to the schema root. let typescript_index = codex_utils_cargo_bin::find_resource!("schema/typescript/index.ts") @@ -92,6 +133,11 @@ fn schema_root() -> Result { Ok(schema_root) } -fn read_tree(root: &Path) -> Result>> { - read_schema_fixture_tree(root).context("read schema fixture tree") +fn read_tree(root: &Path, label: &str) -> Result>> { + read_schema_fixture_subtree(root, label).with_context(|| { + format!( + "read {label} schema fixture subtree from {}", + root.display() + ) + }) }