diff --git a/server/src/session/claude/translate.rs b/server/src/session/claude/translate.rs index c7afdb5..4957405 100644 --- a/server/src/session/claude/translate.rs +++ b/server/src/session/claude/translate.rs @@ -15,7 +15,9 @@ use std::sync::{Arc, Mutex}; use serde_json::{Value, json}; -use super::super::driver::{Event, QuestionOption, SessionStatus, context_tokens, patch_start}; +use super::super::driver::{ + Event, QuestionOption, SessionStatus, context_tokens, patch_start, prefixed_lines, +}; use super::super::subagent::Subagents; /// Whether this line is the CLI opening a fresh model call. @@ -1169,18 +1171,6 @@ fn replacement_diff(input: &Value) -> String { ) } -fn prefixed_lines(prefix: char, text: &str) -> String { - text.split_inclusive('\n') - .map(|line| { - if line.ends_with('\n') { - format!("{prefix}{line}") - } else { - format!("{prefix}{line}\n") - } - }) - .collect() -} - /// Whether this line is a subagent's *own* turn ending -- the only thing /// that does, per `SUBAGENTS.md`: not the parent's `tool_result`, which for /// a background Task arrives at launch rather than at completion. diff --git a/server/src/session/codex/translate.rs b/server/src/session/codex/translate.rs index a7b4b53..e3df738 100644 --- a/server/src/session/codex/translate.rs +++ b/server/src/session/codex/translate.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use serde_json::{Value, json}; -use super::super::driver::{Event, SessionStatus, patch_start}; +use super::super::driver::{Event, SessionStatus, patch_start, prefixed_lines}; use super::super::subagent::Subagents; #[derive(Default)] @@ -741,12 +741,17 @@ fn file_change_diff(item: &Value) -> String { Some(Value::Object(changes)) => changes .iter() .map(|(path, change)| { + let kind = change.get("type").and_then(Value::as_str); render_file_diff( path, - change.get("type").and_then(Value::as_str), + kind, change.get("move_path").and_then(Value::as_str), change - .get("unified_diff") + .get(if matches!(kind, Some("add" | "delete")) { + "content" + } else { + "unified_diff" + }) .and_then(Value::as_str) .unwrap_or_default(), ) @@ -759,7 +764,9 @@ fn file_change_diff(item: &Value) -> String { fn render_file_diff(path: &str, kind: Option<&str>, moved: Option<&str>, body: &str) -> String { // Some protocol revisions carry complete unified diffs and some carry only their hunks. - if body.starts_with("--- ") || body.starts_with("diff --git ") { + if !matches!(kind, Some("add" | "delete")) + && (body.starts_with("--- ") || body.starts_with("diff --git ")) + { return body.to_string(); } let from = if kind == Some("add") { @@ -772,6 +779,14 @@ fn render_file_diff(path: &str, kind: Option<&str>, moved: Option<&str>, body: & } else { moved.unwrap_or(path) }; + let body = match kind { + // Add and delete carry file content, not a unified diff. Prefixing it here makes the + // protocol's change kind authoritative: a '-' that belongs to a Markdown bullet can never + // be mistaken for diff syntax. + Some("add") => prefixed_lines('+', body), + Some("delete") => prefixed_lines('-', body), + _ => body.to_string(), + }; format!("--- {from}\n+++ {to}\n{body}") } @@ -1069,6 +1084,28 @@ mod tests { ); } + #[test] + fn whole_file_changes_get_markers_from_their_kind_not_their_content() { + let item = line( + r#"{"changes":[{"path":"added.md","kind":{"type":"add"},"diff":"--- a rule\n- a bullet\nplain\n"},{"path":"deleted.md","kind":{"type":"delete"},"diff":"- another bullet\nplain\n"}]}"#, + ); + assert_eq!( + file_change_diff(&item), + "--- /dev/null\n+++ added.md\n+--- a rule\n+- a bullet\n+plain\n\n--- deleted.md\n+++ /dev/null\n-- another bullet\n-plain\n" + ); + } + + #[test] + fn legacy_whole_file_changes_show_the_content_they_changed() { + let item = line( + r#"{"changes":{"added.md":{"type":"add","content":"new\n"},"deleted.md":{"type":"delete","content":"old\n"}}}"#, + ); + assert_eq!( + file_change_diff(&item), + "--- /dev/null\n+++ added.md\n+new\n\n--- deleted.md\n+++ /dev/null\n-old\n" + ); + } + #[test] fn native_app_server_completion_corrects_provisional_streaming() { let mut translator = Translator::default(); diff --git a/server/src/session/driver.rs b/server/src/session/driver.rs index bd3dd7a..6009d1c 100644 --- a/server/src/session/driver.rs +++ b/server/src/session/driver.rs @@ -470,6 +470,18 @@ pub(super) fn patch_start(id: String, diff: String) -> Event { } } +pub(super) fn prefixed_lines(prefix: char, text: &str) -> String { + text.split_inclusive('\n') + .map(|line| { + if line.ends_with('\n') { + format!("{prefix}{line}") + } else { + format!("{prefix}{line}\n") + } + }) + .collect() +} + /// How much the model was holding, from the three figures a turn reports: /// the input side only, prompt plus both cache figures. A cached token is /// cheaper but it is still one the model was given; output is what the turn diff --git a/server/src/session/echo.rs b/server/src/session/echo.rs index c168043..6c35421 100644 --- a/server/src/session/echo.rs +++ b/server/src/session/echo.rs @@ -820,7 +820,7 @@ impl EchoDriver { let id = format!("p-{}", super::random_hex()); send(patch_start( id.clone(), - "--- src/example.rs\n+++ src/example.rs\n@@ -1,3 +1,3 @@\n fn answer() -> u8 {\n- 41\n+ 42\n }\n" + "--- src/example.rs\n+++ src/example.rs\n@@ -1,3 +1,3 @@\n fn answer() -> u8 {\n- 41\n+ 42\n }\n\n--- /dev/null\n+++ notes.md\n+- added bullet\n+added prose\n\n--- old-notes.md\n+++ /dev/null\n-- removed bullet\n-removed prose\n" .to_string(), )); tokio::time::sleep(DELTA_DELAY).await;