From ce7fc27f89171f1d6c3e7af68035f13a4e653b2e Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Mon, 1 Dec 2025 13:31:39 -0800 Subject: [PATCH] [app-server] feat: add old_content and new_content to FileChange update patches --- .../app-server-protocol/src/protocol/v2.rs | 6 +- .../app-server/src/bespoke_event_handling.rs | 10 +- .../app-server/tests/suite/v2/turn_start.rs | 140 +++++++++++++++++- codex-rs/apply-patch/src/lib.rs | 12 ++ codex-rs/core/src/apply_patch.rs | 5 +- codex-rs/core/src/turn_diff_tracker.rs | 14 ++ .../src/event_processor_with_human_output.rs | 1 + .../tests/event_processor_with_json_output.rs | 4 + codex-rs/mcp-server/tests/suite/codex_tool.rs | 2 + codex-rs/protocol/src/protocol.rs | 17 +++ codex-rs/tui/src/chatwidget.rs | 2 + codex-rs/tui/src/diff_render.rs | 14 ++ 12 files changed, 221 insertions(+), 6 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index bd38539bb2..a742e8507d 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1146,7 +1146,11 @@ pub struct FileUpdateChange { pub enum PatchChangeKind { Add, Delete, - Update { move_path: Option }, + Update { + move_path: Option, + old_content: String, + new_content: String, + }, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index efdce3162d..330ba76c3b 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -968,8 +968,15 @@ fn map_patch_change_kind(change: &CoreFileChange) -> V2PatchChangeKind { match change { CoreFileChange::Add { .. } => V2PatchChangeKind::Add, CoreFileChange::Delete { .. } => V2PatchChangeKind::Delete, - CoreFileChange::Update { move_path, .. } => V2PatchChangeKind::Update { + CoreFileChange::Update { + move_path, + old_content, + new_content, + .. + } => V2PatchChangeKind::Update { move_path: move_path.clone(), + old_content: old_content.clone(), + new_content: new_content.clone(), }, } } @@ -981,6 +988,7 @@ fn format_file_change_diff(change: &CoreFileChange) -> String { CoreFileChange::Update { unified_diff, move_path, + .. } => { if let Some(path) = move_path { format!("{unified_diff}\n\nMoved to: {}", path.display()) diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 03ee279e51..9cd3fd821b 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -629,10 +629,19 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { *** Add File: README.md +new line *** End Patch +"#; + let update_patch = r#"*** Begin Patch +*** Update File: README.md +@@ +-new line ++updated line +*** End Patch "#; let responses = vec![ create_apply_patch_sse_response(patch, "patch-call")?, create_final_assistant_message_sse_response("patch applied")?, + create_apply_patch_sse_response(update_patch, "patch-call-2")?, + create_final_assistant_message_sse_response("patch updated")?, ]; let server = create_mock_chat_completions_server(responses).await; create_config_toml(&codex_home, &server.uri(), "untrusted")?; @@ -707,8 +716,8 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { assert_eq!(params.item_id, "patch-call"); assert_eq!(params.thread_id, thread.id); assert_eq!(params.turn_id, turn.id); - let expected_readme_path = workspace.join("README.md"); - let expected_readme_path = expected_readme_path.to_string_lossy().into_owned(); + let readme_path = workspace.join("README.md"); + let expected_readme_path = readme_path.to_string_lossy().into_owned(); pretty_assertions::assert_eq!( started_changes, vec![codex_app_server_protocol::FileUpdateChange { @@ -775,9 +784,134 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { ) .await??; - let readme_contents = std::fs::read_to_string(expected_readme_path)?; + let readme_contents = std::fs::read_to_string(&readme_path)?; assert_eq!(readme_contents, "new line\n"); + let second_turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "update patch".into(), + }], + approval_policy: Some(codex_app_server_protocol::AskForApproval::Never), + sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::DangerFullAccess), + model: Some("mock-model".to_string()), + effort: Some(ReasoningEffort::Medium), + summary: Some(ReasoningSummary::Auto), + cwd: Some(workspace.clone()), + }) + .await?; + let second_turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_turn_req)), + ) + .await??; + let TurnStartResponse { turn: second_turn } = + to_response::(second_turn_resp)?; + + let started_update_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { + ref id, + status, + ref changes, + } = started_update_file_change + else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-2"); + assert_eq!(status, PatchApplyStatus::InProgress); + let update_changes = changes.clone(); + let update_change = update_changes + .first() + .expect("expected a single update change"); + assert_eq!(update_change.path, expected_readme_path); + match &update_change.kind { + PatchChangeKind::Update { + move_path, + old_content, + new_content, + } => { + assert!(move_path.is_none()); + assert_eq!(old_content, "new line\n"); + assert_eq!(new_content, "updated line\n"); + } + other => panic!("expected PatchChangeKind::Update, got: {other:?}"), + } + assert!( + update_change.diff.contains("-new line"), + "expected diff to include removal of original line, got: {}", + update_change.diff + ); + assert!( + update_change.diff.contains("+updated line"), + "expected diff to include addition of updated line, got: {}", + update_change.diff + ); + + let output_delta_notif = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/fileChange/outputDelta"), + ) + .await??; + let output_delta: FileChangeOutputDeltaNotification = serde_json::from_value( + output_delta_notif + .params + .clone() + .expect("item/fileChange/outputDelta params"), + )?; + assert_eq!(output_delta.thread_id, thread.id); + assert_eq!(output_delta.turn_id, second_turn.id); + assert_eq!(output_delta.item_id, "patch-call-2"); + assert!( + !output_delta.delta.is_empty(), + "expected delta to be non-empty, got: {}", + output_delta.delta + ); + + let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let completed_notif = mcp + .read_stream_until_notification_message("item/completed") + .await?; + let completed: ItemCompletedNotification = serde_json::from_value( + completed_notif + .params + .clone() + .expect("item/completed params"), + )?; + if let ThreadItem::FileChange { .. } = completed.item { + return Ok::(completed.item); + } + } + }) + .await??; + let ThreadItem::FileChange { ref id, status, .. } = completed_file_change else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-2"); + assert_eq!(status, PatchApplyStatus::Completed); + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let updated_readme_contents = std::fs::read_to_string(&readme_path)?; + assert_eq!(updated_readme_contents, "updated line\n"); + Ok(()) } diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 867d19a2e8..ed131097ac 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -194,6 +194,8 @@ pub enum ApplyPatchFileChange { move_path: Option, /// new_content that will result after the unified_diff is applied. new_content: String, + /// original content before the diff was applied. + old_content: String, }, } @@ -329,6 +331,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp let ApplyPatchFileUpdate { unified_diff, content: contents, + old_content, } = match unified_diff_from_chunks(&path, &chunks) { Ok(diff) => diff, Err(e) => { @@ -341,6 +344,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp unified_diff, move_path: move_path.map(|p| effective_cwd.join(p)), new_content: contents, + old_content, }, ); } @@ -846,6 +850,7 @@ fn apply_replacements( pub struct ApplyPatchFileUpdate { unified_diff: String, content: String, + old_content: String, } pub fn unified_diff_from_chunks( @@ -869,6 +874,7 @@ pub fn unified_diff_from_chunks_with_context( Ok(ApplyPatchFileUpdate { unified_diff, content: new_contents, + old_content: original_contents, }) } @@ -1467,6 +1473,7 @@ PATCH"#, let expected = ApplyPatchFileUpdate { unified_diff: expected_diff.to_string(), content: "foo\nBAR\nbaz\nQUX\n".to_string(), + old_content: "foo\nbar\nbaz\nqux\n".to_string(), }; assert_eq!(expected, diff); } @@ -1503,6 +1510,7 @@ PATCH"#, let expected = ApplyPatchFileUpdate { unified_diff: expected_diff.to_string(), content: "FOO\nbar\nbaz\n".to_string(), + old_content: "foo\nbar\nbaz\n".to_string(), }; assert_eq!(expected, diff); } @@ -1540,6 +1548,7 @@ PATCH"#, let expected = ApplyPatchFileUpdate { unified_diff: expected_diff.to_string(), content: "foo\nbar\nBAZ\n".to_string(), + old_content: "foo\nbar\nbaz\n".to_string(), }; assert_eq!(expected, diff); } @@ -1574,6 +1583,7 @@ PATCH"#, let expected = ApplyPatchFileUpdate { unified_diff: expected_diff.to_string(), content: "foo\nbar\nbaz\nquux\n".to_string(), + old_content: "foo\nbar\nbaz\n".to_string(), }; assert_eq!(expected, diff); } @@ -1629,6 +1639,7 @@ PATCH"#, let expected = ApplyPatchFileUpdate { unified_diff: expected_diff.to_string(), content: "a\nB\nc\nd\nE\nf\ng\n".to_string(), + old_content: "a\nb\nc\nd\ne\nf\n".to_string(), }; assert_eq!(expected, diff); @@ -1688,6 +1699,7 @@ g .to_string(), move_path: None, new_content: "updated session directory content\n".to_string(), + old_content: "session directory content\n".to_string(), }, )]), patch: argv[1].clone(), diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index dffe94be61..da7fea0820 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -105,10 +105,13 @@ pub(crate) fn convert_apply_patch_to_protocol( ApplyPatchFileChange::Update { unified_diff, move_path, - new_content: _new_content, + new_content, + old_content, } => FileChange::Update { unified_diff: unified_diff.clone(), move_path: move_path.clone(), + old_content: old_content.clone(), + new_content: new_content.clone(), }, }; result.insert(path.clone(), protocol_change); diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs index 06c40deb90..096ca18d85 100644 --- a/codex-rs/core/src/turn_diff_tracker.rs +++ b/codex-rs/core/src/turn_diff_tracker.rs @@ -545,6 +545,8 @@ index {ZERO_OID}..{right_oid} FileChange::Update { unified_diff: "".to_owned(), move_path: None, + old_content: "foo\n".to_owned(), + new_content: "foo\nbar\n".to_owned(), }, )]); acc.on_patch_begin(&update_changes); @@ -620,6 +622,8 @@ index {left_oid}..{ZERO_OID} FileChange::Update { unified_diff: "".to_owned(), move_path: Some(dest.clone()), + old_content: "line\n".to_owned(), + new_content: "line2\n".to_owned(), }, )]); acc.on_patch_begin(&mv_changes); @@ -660,6 +664,8 @@ index {left_oid}..{right_oid} FileChange::Update { unified_diff: "".to_owned(), move_path: Some(dest.clone()), + old_content: "same\n".to_owned(), + new_content: "same\n".to_owned(), }, )]); acc.on_patch_begin(&mv_changes); @@ -682,6 +688,8 @@ index {left_oid}..{right_oid} FileChange::Update { unified_diff: "".into(), move_path: Some(dest.clone()), + old_content: String::new(), + new_content: "hello\n".to_owned(), }, )]); acc.on_patch_begin(&mv); @@ -722,6 +730,8 @@ index {ZERO_OID}..{right_oid} FileChange::Update { unified_diff: "".to_owned(), move_path: None, + old_content: "foo\n".to_owned(), + new_content: "foo\nbar\n".to_owned(), }, )]); acc.on_patch_begin(&update_a); @@ -802,6 +812,8 @@ index {left_oid_b}..{ZERO_OID} FileChange::Update { unified_diff: "".to_owned(), move_path: None, + old_content: String::new(), + new_content: String::new(), }, )]); acc.on_patch_begin(&update_changes); @@ -868,6 +880,8 @@ index {ZERO_OID}..{right_oid} FileChange::Update { unified_diff: "".to_owned(), move_path: None, + old_content: "foo\n".to_owned(), + new_content: "foo\nbar\n".to_owned(), }, )]); acc.on_patch_begin(&update_changes); diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 64a5358f35..7d29800abe 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -406,6 +406,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { FileChange::Update { unified_diff, move_path, + .. } => { let header = if let Some(dest) = move_path { format!( diff --git a/codex-rs/exec/tests/event_processor_with_json_output.rs b/codex-rs/exec/tests/event_processor_with_json_output.rs index 7b2d3c18ba..0827f83100 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -823,6 +823,8 @@ fn patch_apply_success_produces_item_completed_patchapply() { FileChange::Update { unified_diff: "--- c/modified.txt\n+++ c/modified.txt\n@@\n-old\n+new\n".to_string(), move_path: Some(PathBuf::from("c/renamed.txt")), + old_content: "-old\n".to_string(), + new_content: "+new\n".to_string(), }, ); @@ -895,6 +897,8 @@ fn patch_apply_failure_produces_item_completed_patchapply_failed() { FileChange::Update { unified_diff: "--- file.txt\n+++ file.txt\n@@\n-old\n+new\n".to_string(), move_path: None, + old_content: "-old\n".to_string(), + new_content: "+new\n".to_string(), }, ); diff --git a/codex-rs/mcp-server/tests/suite/codex_tool.rs b/codex-rs/mcp-server/tests/suite/codex_tool.rs index f65495c473..7b89d9e28a 100644 --- a/codex-rs/mcp-server/tests/suite/codex_tool.rs +++ b/codex-rs/mcp-server/tests/suite/codex_tool.rs @@ -269,6 +269,8 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { FileChange::Update { unified_diff: "@@ -1 +1 @@\n-original content\n+modified content\n".to_string(), move_path: None, + old_content: "original content\n".to_string(), + new_content: "modified content\n".to_string(), }, ); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 0f5541c2d5..30e2674655 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1653,6 +1653,8 @@ pub enum FileChange { Update { unified_diff: String, move_path: Option, + old_content: String, + new_content: String, }, } @@ -1717,6 +1719,21 @@ mod tests { assert!(event.as_legacy_events(false).is_empty()); } + #[test] + fn file_change_update_serializes_old_and_new_content() { + let change = FileChange::Update { + unified_diff: "--- a\n+++ b\n@@ -1 +1 @@\n-old\n+new\n".to_string(), + move_path: None, + old_content: "old\n".to_string(), + new_content: "new\n".to_string(), + }; + + let serialized = serde_json::to_value(change).expect("should serialize"); + + assert_eq!(serialized["old_content"], "old\n"); + assert_eq!(serialized["new_content"], "new\n"); + } + /// Serialize Event to verify that its JSON representation has the expected /// amount of nesting. #[test] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index d11d983cf1..ee074901d8 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1560,6 +1560,8 @@ impl ChatWidget { FileChange::Update { unified_diff: "+test\n-test2".to_string(), move_path: None, + old_content: "test2".to_string(), + new_content: "test".to_string(), }, ), ]), diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 24c5be597b..fdd1a9100a 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -484,6 +484,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: None, + old_content: original.to_string(), + new_content: modified.to_string(), }, ); @@ -504,6 +506,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: Some(PathBuf::from("new_name.rs")), + old_content: original.to_string(), + new_content: modified.to_string(), }, ); @@ -524,6 +528,8 @@ mod tests { FileChange::Update { unified_diff: patch_a, move_path: None, + old_content: "one\n".to_string(), + new_content: "one changed\n".to_string(), }, ); @@ -590,6 +596,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: None, + old_content: original.to_string(), + new_content: modified.to_string(), }, ); @@ -613,6 +621,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: None, + old_content: original.to_string(), + new_content: modified.to_string(), }, ); @@ -640,6 +650,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: None, + old_content: original.clone(), + new_content: modified.clone(), }, ); @@ -663,6 +675,8 @@ mod tests { FileChange::Update { unified_diff: patch, move_path: Some(abs_new), + old_content: original.to_string(), + new_content: modified.to_string(), }, );