From a0f2d80fa41ae8a9d4b259d644b66f3444e5c1b7 Mon Sep 17 00:00:00 2001 From: Nishant Joshi Date: Wed, 23 Sep 2026 14:27:00 -0700 Subject: [PATCH] fix(codex): preserve valid tool inputs and result pairing --- docs/formats/codex.md | 10 ++- src/harness/codex.rs | 40 +++------ tests/integration/codex.rs | 166 ++++++++++++++++++++++++++----------- 3 files changed, 141 insertions(+), 75 deletions(-) diff --git a/docs/formats/codex.md b/docs/formats/codex.md index 5516f18..c42fd90 100644 --- a/docs/formats/codex.md +++ b/docs/formats/codex.md @@ -83,7 +83,15 @@ the form `["bash"|"sh"|"zsh", "-lc"|"-c", cmd]` collapse to the inner command. representation keeps every payload as raw JSON, so native ↔ disk round-trips are lossless. - **`apply_patch` is best-effort.** Only a lone single-hunk update maps to `Edit` and a lone file add to `Write`; multi-file, multi-hunk, delete, and move patches stay as a raw `ApplyPatch` - with the touched paths listed. + with the touched paths listed. On export, `Edit` and `Write` become `apply_patch` + custom-tool calls, and raw `ApplyPatch` envelopes are unwrapped. Their results use + `custom_tool_call_output`, preserving the error flag. Patches are line-based: + `replace_all` becomes one hunk, Codex can normalize trailing blank lines, and the Common + reader does not retain the final line terminator. Exported `Write` calls use an add-file + patch, which does not encode whether the original call created or overwrote a file. +- **Shell export.** `Bash` becomes `exec_command` with `cmd` and optional `workdir`. + The canonical timeout, description, and background fields have no matching fields in + this mapping and are omitted. Other tools keep their canonical function-call form. - **Resume is picky.** `from_common` must emit `model_provider: "openai"` in `session_meta` — current Codex resolves a null provider to the empty name and fails resume with ``Model provider `` not found``. `base_instructions` may be null (defaults substitute). Foreign diff --git a/src/harness/codex.rs b/src/harness/codex.rs index 65e8ae6..8c1cb88 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -656,8 +656,8 @@ fn push_message_lines(lines: &mut Vec, msg: &Message, ts: &str, patch_ids: } /// Emit the native call line for one tool invocation: `exec_command` for -/// shell, `custom_tool_call` for edits, `web_search_call` for search — the -/// exact reverse of the inbound normalization, so Codex validates the replay. +/// shell and `custom_tool_call` for edits. Foreign tools keep their canonical +/// function-call form and their paired function-call results. fn push_tool_use_lines(lines: &mut Vec, ts: &str, id: &str, tool: &Tool) { match tool { Tool::Bash { @@ -683,18 +683,6 @@ fn push_tool_use_lines(lines: &mut Vec, ts: &str, id: &str, tool: &Tool) { }), )); } - Tool::Raw { tool_name, input } if tool_name == "WebSearch" => { - lines.push(meta_line_str( - ts, - "response_item", - json!({ - "type": "web_search_call", - "status": "completed", - "call_id": id, - "action": input, - }), - )); - } Tool::Edit { file_path, old_string, @@ -705,26 +693,24 @@ fn push_tool_use_lines(lines: &mut Vec, ts: &str, id: &str, tool: &Tool) { lines, ts, id, - &Value::String(apply_patch_update(file_path, old_string, new_string)), + &apply_patch_update(file_path, old_string, new_string), ); } Tool::Write { file_path, content } => { - push_custom_tool_call( - lines, - ts, - id, - &Value::String(apply_patch_add(file_path, content)), - ); + push_custom_tool_call(lines, ts, id, &apply_patch_add(file_path, content)); } Tool::Raw { tool_name, input } if tool_name == "ApplyPatch" => { // The fallback shape inbound keeps is // `{"patch": , "files": [...]}`; unwrap it // so live Codex sees the string input it wrote. - let input = match input { - Value::Object(obj) if obj.get("patch").is_some_and(Value::is_string) => { - obj["patch"].clone() - } - _ => input.clone(), + let input = match input.get("patch").and_then(Value::as_str) { + Some(patch) => patch.to_owned(), + None => match input { + Value::String(text) => text.clone(), + // Even a malformed historical call must satisfy Codex's + // string input type. The reader decodes JSON strings. + other => other.to_string(), + }, }; push_custom_tool_call(lines, ts, id, &input); } @@ -777,7 +763,7 @@ fn apply_patch_add(file_path: &str, content: &str) -> String { patch.join("\n") } -fn push_custom_tool_call(lines: &mut Vec, ts: &str, id: &str, input: &Value) { +fn push_custom_tool_call(lines: &mut Vec, ts: &str, id: &str, input: &str) { lines.push(meta_line_str( ts, "response_item", diff --git a/tests/integration/codex.rs b/tests/integration/codex.rs index 0f0387a..b661d4d 100644 --- a/tests/integration/codex.rs +++ b/tests/integration/codex.rs @@ -279,7 +279,14 @@ fn codec_fixpoint_through_common_loses_nothing() { #[test] fn from_common_denormalizes_bash_to_exec_command() { - let common = sample_common(); + let mut common = sample_common(); + if let common::Block::ToolUse { + tool: common::Tool::Bash { workdir, .. }, + .. + } = &mut common.body[2].content[0] + { + *workdir = Some("/repo with spaces".into()); + } let native = codex::Codex::from_common(&common).unwrap(); let mut found = false; for line in &native.body { @@ -306,6 +313,8 @@ fn from_common_denormalizes_bash_to_exec_command() { .and_then(|s| s.parse().ok()) .unwrap_or(serde_json::Value::Null); if args.get("cmd").and_then(serde_json::Value::as_str) == Some("ls") { + assert_eq!(args["workdir"], "/repo with spaces"); + assert_eq!(line.payload["call_id"], "call-x"); found = true; } } @@ -316,52 +325,6 @@ fn from_common_denormalizes_bash_to_exec_command() { ); } -#[test] -fn from_common_denormalizes_web_search_to_web_search_call() { - let mut common = sample_common(); - common.body.push(common::Message { - role: common::Role::Assistant, - content: vec![common::Block::ToolUse { - id: "ws-call".into(), - tool: common::Tool::Raw { - tool_name: "WebSearch".into(), - input: serde_json::json!({"query": "rust lang"}), - }, - }], - timestamp: ts("2026-01-02T03:04:11.000Z"), - model: None, - stop_reason: None, - usage: None, - }); - common.body.push(common::Message { - role: common::Role::User, - content: vec![common::Block::ToolResult { - tool_use_id: "ws-call".into(), - content: common::ToolOutput::Text("found it".into()), - is_error: false, - }], - timestamp: ts("2026-01-02T03:04:12.000Z"), - model: None, - stop_reason: None, - usage: None, - }); - let native = codex::Codex::from_common(&common).unwrap(); - let found = native.body.iter().any(|line| { - line.kind == "response_item" - && line.payload.get("type").and_then(serde_json::Value::as_str) - == Some("web_search_call") - && line - .payload - .get("call_id") - .and_then(serde_json::Value::as_str) - == Some("ws-call") - }); - assert!( - found, - "from_common must emit native web_search_call instead of generic function_call" - ); -} - #[test] fn from_common_denormalizes_edit_to_apply_patch_with_error_result() { let mut common = sample_common(); @@ -437,3 +400,112 @@ fn from_common_denormalizes_edit_to_apply_patch_with_error_result() { let back = codex::Codex::to_common(&native).unwrap(); assert_eq!(common, back); } + +#[test] +fn from_common_patch_envelopes_encode_multiline_and_empty_files() { + for (tool, expected_input) in [ + ( + common::Tool::Edit { + file_path: "src/main.rs".into(), + old_string: "old\n\n".into(), + new_string: "new\n\n".into(), + replace_all: false, + }, + "*** Begin Patch\n*** Update File: src/main.rs\n@@\n-old\n-\n+new\n+\n*** End Patch", + ), + ( + common::Tool::Write { + file_path: "notes.md".into(), + content: "first\nsecond\n\n".into(), + }, + "*** Begin Patch\n*** Add File: notes.md\n+first\n+second\n+\n*** End Patch", + ), + ( + common::Tool::Write { + file_path: "empty.txt".into(), + content: String::new(), + }, + "*** Begin Patch\n*** Add File: empty.txt\n*** End Patch", + ), + ] { + let mut common = sample_common(); + common.body[2].content = vec![common::Block::ToolUse { + id: "call-x".into(), + tool, + }]; + let native = codex::Codex::from_common(&common).unwrap(); + let call = native + .body + .iter() + .find(|line| line.kind == "response_item" && line.payload["type"] == "custom_tool_call") + .unwrap(); + assert_eq!(call.payload["input"], expected_input); + } +} + +#[test] +fn from_common_raw_patch_inputs_are_always_strings() { + let patch = + "*** Begin Patch\n*** Delete File: old.rs\n*** Add File: new.rs\n+hello\n*** End Patch"; + for input in [ + serde_json::json!({"patch": patch, "files": ["old.rs", "new.rs"]}), + // Failed native calls can carry malformed arguments. Their history + // still needs a string input, as required by Codex's CustomToolCall. + serde_json::json!({"unexpected": "argument"}), + serde_json::json!(["unexpected", "array"]), + serde_json::Value::Null, + ] { + let mut common = sample_common(); + common.body[2].content = vec![common::Block::ToolUse { + id: "call-x".into(), + tool: common::Tool::Raw { + tool_name: "ApplyPatch".into(), + input, + }, + }]; + let native = codex::Codex::from_common(&common).unwrap(); + let call = native + .body + .iter() + .find(|line| line.kind == "response_item" && line.payload["type"] == "custom_tool_call") + .unwrap(); + assert!(call.payload["input"].is_string(), "{call:?}"); + let result = native + .body + .iter() + .find(|line| { + line.kind == "response_item" && line.payload["type"] == "custom_tool_call_output" + }) + .unwrap(); + assert_eq!(call.payload["call_id"], result.payload["call_id"]); + let back = codex::Codex::to_common(&native).unwrap(); + assert_eq!(common, back); + } +} + +#[test] +fn from_common_keeps_foreign_web_search_paired_with_its_function_result() { + let mut common = sample_common(); + common.body[2].content = vec![common::Block::ToolUse { + id: "call-x".into(), + tool: common::Tool::Raw { + tool_name: "WebSearch".into(), + input: serde_json::json!({"query": "rust lang"}), + }, + }]; + let native = codex::Codex::from_common(&common).unwrap(); + // A foreign search input has no native WebSearchAction type tag, and + // its function result must keep a corresponding function call. + assert!( + !native.body.iter().any(|line| { + line.kind == "response_item" && line.payload["type"] == "web_search_call" + }) + ); + assert!(native.body.iter().any(|line| { + line.kind == "response_item" + && line.payload["type"] == "function_call" + && line.payload["name"] == "WebSearch" + && line.payload["call_id"] == "call-x" + })); + assert_eq!(common, codex::Codex::to_common(&native).unwrap()); +}