fix: ensure trailing newline in files created by text_editor tool (#5336)

Signed-off-by: The-Best-Codes <bestcodes.official@gmail.com>

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"
+        );
+    }
 }

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
+/// Parses diff content into patches with proper error handling
-pub async fn apply_diff(
+fn parse_diff_content(diff_content: &str) -> Result<Vec<mpatch::Patch>, ErrorData> {
-    base_path: &Path,
-    diff_content: &str,
-    file_history: &std::sync::Arc<std::sync::Mutex<HashMap<PathBuf, Vec<String>>>>,
-) -> Result<Vec<Content>, ErrorData> {
-    // Validate size
-    validate_diff_size(diff_content)?;
-
-    // Parse patches using mpatch - wrap in markdown block if not already wrapped
     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
+/// Ensures all patched files end with a newline
-    if patches.len() > MAX_FILES_IN_DIFF {
+fn ensure_trailing_newlines(patches: &[mpatch::Patch], base_dir: &Path) -> Result<(), ErrorData> {
-        return Err(ErrorData::new(
+    for patch in patches {
-            ErrorCode::INVALID_PARAMS,
+        let adjusted_base_dir = adjust_base_dir_for_overlap(base_dir, &patch.file_path);
-            format!(
+        let file_path = adjusted_base_dir.join(&patch.file_path);
-                "Too many files in diff ({}). Maximum is {} files.",
+
-                patches.len(),
+        if file_path.exists() {
-                MAX_FILES_IN_DIFF
+            let content = std::fs::read_to_string(&file_path).map_err(|e| {
-            ),
+                ErrorData::new(
-            None,
+                    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
+/// Reports partial failures from patch application
-    let base_dir = if base_path.is_file() {
+fn report_partial_failures(failed_hunks: &[String]) {
-        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
     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<std::sync::Mutex<HashMap<PathBuf, Vec<String>>>>,
+) -> Result<Vec<Content>, 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,