diff --git a/crates/goose-mcp/src/developer/tests/test_diff.rs b/crates/goose-mcp/src/developer/tests/test_diff.rs index 1e0f93e2..457bc4b3 100644 --- a/crates/goose-mcp/src/developer/tests/test_diff.rs +++ b/crates/goose-mcp/src/developer/tests/test_diff.rs @@ -403,4 +403,101 @@ diff --git a/file2.txt b/file2.txt let content = std::fs::read_to_string(&file_path).unwrap(); assert!(content.contains("goodbye")); } + + #[tokio::test] + async fn test_text_editor_write_adds_trailing_newline() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + + let result = text_editor_write(&file_path, "Hello, World!").await; + + assert!(result.is_ok()); + let content = std::fs::read_to_string(&file_path).unwrap(); + assert!(content.ends_with('\n'), "File should end with newline"); + assert_eq!(content, "Hello, World!\n"); + } + + #[tokio::test] + async fn test_text_editor_write_preserves_existing_newline() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + + let result = text_editor_write(&file_path, "Hello, World!\n").await; + + assert!(result.is_ok()); + let content = std::fs::read_to_string(&file_path).unwrap(); + assert!(content.ends_with('\n'), "File should end with newline"); + assert_eq!(content, "Hello, World!\n"); + } + + #[tokio::test] + async fn test_text_editor_write_multiline_adds_trailing_newline() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + + let content_without_newline = "line1\nline2\nline3"; + let result = text_editor_write(&file_path, content_without_newline).await; + + assert!(result.is_ok()); + let content = std::fs::read_to_string(&file_path).unwrap(); + assert!(content.ends_with('\n'), "File should end with newline"); + assert_eq!(content, "line1\nline2\nline3\n"); + } + + #[tokio::test] + async fn test_apply_diff_adds_trailing_newline() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + + std::fs::write(&file_path, "line1\nline2\nline3").unwrap(); + + let diff = r#"--- a/test.txt ++++ b/test.txt +@@ -1,3 +1,3 @@ + line1 +-line2 ++line2_modified + line3"#; + + let history = Arc::new(Mutex::new(HashMap::new())); + let result = apply_diff(&file_path, diff, &history).await; + + assert!(result.is_ok()); + let content = std::fs::read_to_string(&file_path).unwrap(); + assert!( + content.ends_with('\n'), + "File should end with newline after apply_diff" + ); + assert!(content.contains("line2_modified")); + } + + #[tokio::test] + async fn test_apply_diff_maintains_trailing_newline() { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.txt"); + + std::fs::write(&file_path, "line1\nline2\nline3\n").unwrap(); + + let diff = r#"--- a/test.txt ++++ b/test.txt +@@ -1,3 +1,3 @@ + line1 +-line2 ++line2_modified + line3"#; + + let history = Arc::new(Mutex::new(HashMap::new())); + let result = apply_diff(&file_path, diff, &history).await; + + assert!(result.is_ok()); + let content = std::fs::read_to_string(&file_path).unwrap(); + assert!( + content.ends_with('\n'), + "File should maintain trailing newline" + ); + assert_eq!( + content, "line1\nline2_modified\nline3\n", + "Content should be modified and end with newline" + ); + } } diff --git a/crates/goose-mcp/src/developer/text_editor.rs b/crates/goose-mcp/src/developer/text_editor.rs index 06a3d125..d7f0bdb4 100644 --- a/crates/goose-mcp/src/developer/text_editor.rs +++ b/crates/goose-mcp/src/developer/text_editor.rs @@ -286,23 +286,15 @@ fn apply_single_patch( Ok(()) } -/// Applies any diff (single or multi-file) using mpatch for fuzzy matching -pub async fn apply_diff( - base_path: &Path, - diff_content: &str, - file_history: &std::sync::Arc>>>, -) -> Result, ErrorData> { - // Validate size - validate_diff_size(diff_content)?; - - // Parse patches using mpatch - wrap in markdown block if not already wrapped +/// Parses diff content into patches with proper error handling +fn parse_diff_content(diff_content: &str) -> Result, ErrorData> { let wrapped_diff = if diff_content.contains("```diff") || diff_content.contains("```patch") { diff_content.to_string() } else { format!("```diff\n{}\n```", diff_content) }; - let patches = parse_diffs(&wrapped_diff).map_err(|e| match e { + parse_diffs(&wrapped_diff).map_err(|e| match e { PatchError::MissingFileHeader => ErrorData::new( ErrorCode::INVALID_PARAMS, "Invalid diff format: Missing file header (e.g., '--- a/path/to/file')".to_string(), @@ -326,43 +318,41 @@ pub async fn apply_diff( format!("Target file not found: {}", path.display()), None, ), - })?; + }) +} - // Validate file count - if patches.len() > MAX_FILES_IN_DIFF { - return Err(ErrorData::new( - ErrorCode::INVALID_PARAMS, - format!( - "Too many files in diff ({}). Maximum is {} files.", - patches.len(), - MAX_FILES_IN_DIFF - ), - None, - )); +/// Ensures all patched files end with a newline +fn ensure_trailing_newlines(patches: &[mpatch::Patch], base_dir: &Path) -> Result<(), ErrorData> { + for patch in patches { + let adjusted_base_dir = adjust_base_dir_for_overlap(base_dir, &patch.file_path); + let file_path = adjusted_base_dir.join(&patch.file_path); + + if file_path.exists() { + let content = std::fs::read_to_string(&file_path).map_err(|e| { + ErrorData::new( + ErrorCode::INTERNAL_ERROR, + format!("Failed to read file for post-processing: {}", e), + None, + ) + })?; + + if !content.ends_with('\n') { + let content_with_newline = format!("{}\n", content); + std::fs::write(&file_path, content_with_newline).map_err(|e| { + ErrorData::new( + ErrorCode::INTERNAL_ERROR, + format!("Failed to add trailing newline: {}", e), + None, + ) + })?; + } + } } + Ok(()) +} - // Determine base directory - let base_dir = if base_path.is_file() { - base_path.parent().unwrap_or(Path::new(".")).to_path_buf() - } else { - base_path.to_path_buf() - }; - - // Apply all patches with fuzzy matching - let mut results = DiffResults::default(); - let mut failed_hunks = Vec::new(); - - for patch in &patches { - apply_single_patch( - patch, - &base_dir, - file_history, - &mut results, - &mut failed_hunks, - )?; - } - - // Report any partial failures +/// Reports partial failures from patch application +fn report_partial_failures(failed_hunks: &[String]) { if !failed_hunks.is_empty() { let error_msg = format!( "Some patches were only partially applied (fuzzy matching at 70% similarity):\n\n{}\n\n\ @@ -377,13 +367,55 @@ pub async fn apply_diff( tracing::warn!("{}", error_msg); } +} + +/// Applies any diff (single or multi-file) using mpatch for fuzzy matching +pub async fn apply_diff( + base_path: &Path, + diff_content: &str, + file_history: &std::sync::Arc>>>, +) -> Result, ErrorData> { + validate_diff_size(diff_content)?; + let patches = parse_diff_content(diff_content)?; + + if patches.len() > MAX_FILES_IN_DIFF { + return Err(ErrorData::new( + ErrorCode::INVALID_PARAMS, + format!( + "Too many files in diff ({}). Maximum is {} files.", + patches.len(), + MAX_FILES_IN_DIFF + ), + None, + )); + } + + let base_dir = if base_path.is_file() { + base_path.parent().unwrap_or(Path::new(".")).to_path_buf() + } else { + base_path.to_path_buf() + }; + + let mut results = DiffResults::default(); + let mut failed_hunks = Vec::new(); + + for patch in &patches { + apply_single_patch( + patch, + &base_dir, + file_history, + &mut results, + &mut failed_hunks, + )?; + } + + ensure_trailing_newlines(&patches, &base_dir)?; + report_partial_failures(&failed_hunks); - // Count line changes let (lines_added, lines_removed) = count_line_changes(diff_content); results.lines_added = lines_added; results.lines_removed = lines_removed; - // Generate summary let is_single_file = patches.len() == 1; Ok(generate_summary(&results, is_single_file, base_path)) } @@ -765,7 +797,12 @@ pub async fn text_editor_replace( match editor.edit_code(&content, old_str, new_str).await { Ok(updated_content) => { // Write the updated content directly - let normalized_content = normalize_line_endings(&updated_content); + let mut normalized_content = normalize_line_endings(&updated_content); + + if !normalized_content.ends_with('\n') { + normalized_content.push('\n'); + } + std::fs::write(path, &normalized_content).map_err(|e| { ErrorData::new( ErrorCode::INTERNAL_ERROR, @@ -811,7 +848,12 @@ pub async fn text_editor_replace( save_file_history(path, file_history)?; let new_content = content.replace(old_str, new_str); - let normalized_content = normalize_line_endings(&new_content); + let mut normalized_content = normalize_line_endings(&new_content); + + if !normalized_content.ends_with('\n') { + normalized_content.push('\n'); + } + std::fs::write(path, &normalized_content).map_err(|e| { ErrorData::new( ErrorCode::INTERNAL_ERROR,