fix(processor): improve safety checks for non-existent and invalid paths

Refactors the path safety checking logic in `src/processor/safety.rs` to more robustly handle non-existent target paths and intermediate path components that are files instead of directories.

- Replaced the previous `check_nonexistent_target_safety` with a recursive `check_nonexistent_path_safety` function.
- Explicitly checks for `io::ErrorKind::NotADirectory` during canonicalization and metadata lookups for both the target path and its ancestors.
- Maps `NotADirectory` errors encountered during safety checks to the more specific `ProcessError::ParentIsNotDirectory` for clearer error reporting.

This commit also ensures the processing summary is always printed in `src/main.rs`, even if no actions are parsed from the input markdown file. Previously, the summary was skipped in this case.

Supporting changes include:
- Added unit tests (`src/processor/safety_tests.rs`) for the refactored safety logic.
- Added CLI integration tests (`tests/cli/empty_input.rs`) to verify the summary output behavior for empty/non-actionable inputs.
- Moved safety integration tests to unit tests.
- Added a processor integration test for handling empty path strings.

Author: Romelium

src/main.rs

@@ -25,34 +25,37 @@ fn run() -> Result<Summary, AppError> {
     println!("\nParsing markdown for file actions...");
     let parsed_actions = parse_markdown(&markdown_content)?; // Use lib function
 
+    // Check if actions were found and print appropriate message
     if parsed_actions.is_empty() {
         // Basic check if content might have had actionable items
         if markdown_content.contains("```")
             || markdown_content.contains("//")
             || markdown_content.contains("**")
             || markdown_content.contains("##")
         {
-            eprintln!("\nWarning: No valid actions extracted. Check formatting.");
+            // CHANGE: Use println! instead of eprintln! for this warning
+            println!("\nWarning: No valid actions extracted. Check formatting.");
         } else {
             println!("\nInfo: No actionable content found.");
         }
         println!("\nNo actions to process.");
-        return Ok(Summary::default()); // Return empty summary
+        // REMOVE the early return: continue processing to print the zero summary
+        // return Ok(Summary::default()); // Return empty summary
+    } else {
+        println!(
+            "\nFound {} actions to process (sorted by document order).",
+            parsed_actions.len()
+        );
     }
 
-    println!(
-        "\nFound {} actions to process (sorted by document order).",
-        parsed_actions.len()
-    );
-
-    // Process actions using the library function
+    // Process actions using the library function (will do nothing if actions is empty)
     let summary = process_actions(&cli.output_dir, parsed_actions, cli.force)?;
 
     // Print summary needs the *resolved* base path for display
     // Resolve again for printing; process_actions resolves internally for safety.
     // Use original path if canonicalize fails (e.g., dir deleted during processing).
     let resolved_output_dir_display = cli.output_dir.canonicalize().unwrap_or(cli.output_dir);
-    // Call the imported print_summary function
+    // Call the imported print_summary function - THIS WILL NOW ALWAYS RUN
     print_summary(&summary, &resolved_output_dir_display);
 
     Ok(summary)

src/processor/mod.rs

@@ -13,6 +13,12 @@ mod delete;
 mod safety;
 mod summary_updater;
 
+// Declare the unit test module for safety
+#[cfg(test)]
+mod safety_tests;
+#[cfg(test)] // Also declare the existing summary_updater_tests module here
+mod summary_updater_tests;
+
 /// Processes a list of actions against the filesystem relative to a base directory.
 pub fn process_actions(
     base_dir: &Path,

src/processor/safety.rs

@@ -55,7 +55,8 @@ pub(crate) fn ensure_path_safe(base_dir: &Path, target_path: &Path) -> Result<()
         }
         Err(ref e) if e.kind() == ErrorKind::NotFound => {
             // Target doesn't exist: Check safety based on its intended parent.
-            check_nonexistent_target_safety(target_path, &canonical_base)
+            // Pass the original target_path for error reporting context if needed.
+            check_nonexistent_path_safety(target_path, &canonical_base)
         }
         // *** CATCH NotADirectory during metadata check (parent is file) ***
         Err(ref e) if e.kind() == ErrorKind::NotADirectory => {
@@ -71,68 +72,101 @@ pub(crate) fn ensure_path_safe(base_dir: &Path, target_path: &Path) -> Result<()
     }
 }
 
-/// Checks safety for a target path that does not yet exist by examining its parent.
-fn check_nonexistent_target_safety(
-    target_path: &Path,
+/// Recursively checks safety for a path that does not necessarily exist
+/// by examining its ancestors relative to the canonical base.
+fn check_nonexistent_path_safety(
+    path_to_check: &Path,
     canonical_base: &Path,
 ) -> Result<(), ProcessError> {
-    if let Some(parent) = target_path.parent() {
-        // Canonicalize the parent directory.
+    // Base case: If the path_to_check *is* the base directory, it's safe by definition.
+    // We need to compare canonical paths if possible.
+    match path_to_check.canonicalize() {
+        Ok(canonical_check) if canonical_check == *canonical_base => return Ok(()),
+        Ok(_) => {} // Path exists and is not the base, continue to parent check
+        Err(ref e) if e.kind() == ErrorKind::NotFound => {} // Path doesn't exist, continue
+        Err(ref e) if e.kind() == ErrorKind::NotADirectory => {
+            // An intermediate component is a file.
+            return Err(ProcessError::ParentIsNotDirectory {
+                path: path_to_check.to_path_buf(), // Report the path we were checking
+                parent_path: path_to_check
+                    .parent()
+                    .unwrap_or(path_to_check)
+                    .to_path_buf(),
+            });
+        }
+        Err(e) => {
+            // Other canonicalization error (permissions?)
+            return Err(ProcessError::PathResolution {
+                path: path_to_check.to_path_buf(),
+                details: format!("Failed to canonicalize path during safety check: {}", e),
+            });
+        }
+    }
+
+    // Get the parent of the path we are currently checking.
+    if let Some(parent) = path_to_check.parent() {
+        // If the parent *is* the base directory, the path is safe (it's directly inside).
+        // Compare canonical paths if possible for robustness.
         match parent.canonicalize() {
             Ok(canonical_parent) => {
-                // Check if the existing parent is within the base directory.
-                if canonical_parent.starts_with(canonical_base) {
-                    // Parent is safe, so creating the target inside it is considered safe.
-                    Ok(())
-                } else {
-                    // Parent exists but is outside the base directory. Unsafe.
-                    Err(ProcessError::PathNotSafe {
-                        resolved_path: canonical_parent, // Report the parent path
+                if canonical_parent == *canonical_base {
+                    return Ok(());
+                }
+                // Parent exists and is not the base. Check if it's *within* the base.
+                if !canonical_parent.starts_with(canonical_base) {
+                    return Err(ProcessError::PathNotSafe {
+                        resolved_path: canonical_parent, // Report the unsafe parent
                         base_path: canonical_base.to_path_buf(),
-                    })
+                    });
                 }
+                // Parent exists and is within the base. Now, ensure it's actually a directory.
+                match fs::metadata(&canonical_parent) {
+                    Ok(meta) if meta.is_dir() => {
+                        // Parent exists, is safe, and is a directory. Path is safe.
+                        Ok(())
+                    }
+                    Ok(_) => {
+                        // Parent exists, is safe, but is NOT a directory.
+                        Err(ProcessError::ParentIsNotDirectory {
+                            path: path_to_check.to_path_buf(), // The path we were trying to create/check
+                            parent_path: parent.to_path_buf(), // The parent that is not a directory
+                        })
+                    }
+                    Err(e) => {
+                        // Error getting metadata for the existing canonical parent (permissions?)
+                        Err(ProcessError::Io { source: e })
+                    }
+                }
+            }
+            Err(ref e) if e.kind() == ErrorKind::NotFound => {
+                // Parent does not exist. Recursively check the parent's safety.
+                check_nonexistent_path_safety(parent, canonical_base)
             }
-            // *** CATCH NotADirectory during PARENT canonicalization ***
-            Err(ref parent_err) if parent_err.kind() == ErrorKind::NotADirectory => {
+            Err(ref e) if e.kind() == ErrorKind::NotADirectory => {
+                // An intermediate component in the *parent's* path is a file.
                 Err(ProcessError::ParentIsNotDirectory {
-                    path: target_path.to_path_buf(), // The target we were trying to create
-                    parent_path: parent.to_path_buf(), // The parent that is not a directory
+                    path: path_to_check.to_path_buf(), // The original path being checked
+                    parent_path: parent.to_path_buf(), // The parent path that failed
                 })
             }
-            Err(ref parent_err) if parent_err.kind() == ErrorKind::NotFound => {
-                // Parent directory itself doesn't exist. This implies it needs to be
-                // created. Assume `create_dir_all` will handle safety within the base.
-                // We could recursively check the parent's parent, but relying on
-                // the initial base check and `create_dir_all` is often sufficient.
-                // If the non-existent parent's path *string* looks unsafe (e.g., "../.."),
-                // it might be caught earlier, but canonicalization handles this better.
-                // For simplicity here, assume okay if parent *would* be created inside base.
-                // A stricter check could trace the path components upwards.
-                // Let's check if the parent *path itself* starts with base logically.
-                if parent.starts_with(canonical_base) {
-                    Ok(())
-                } else {
-                    // If even the logical parent path isn't inside base, it's unsafe.
-                    // This check is less robust than canonicalization but handles simple cases.
-                    Err(ProcessError::PathNotSafe {
-                        resolved_path: parent.to_path_buf(), // Report logical parent
-                        base_path: canonical_base.to_path_buf(),
-                    })
-                }
-            }
-            Err(parent_err) => {
-                // Other error canonicalizing the parent (e.g., permissions).
+            Err(e) => {
+                // Other error canonicalizing the parent (permissions?)
                 Err(ProcessError::PathResolution {
                     path: parent.to_path_buf(),
-                    details: format!("Failed to canonicalize parent directory: {}", parent_err),
+                    details: format!("Failed to canonicalize parent directory: {}", e),
                 })
             }
         }
     } else {
-        // Cannot get parent (e.g., root path "/" or similar). Unlikely for relative paths.
+        // Cannot get parent (e.g., root path "/" or similar).
+        // If we reached here, it means the path wasn't the base itself.
+        // This implies an attempt to access something outside the base, potentially the root.
         Err(ProcessError::PathResolution {
-            path: target_path.to_path_buf(),
-            details: "Cannot determine parent directory for safety check.".to_string(),
+            path: path_to_check.to_path_buf(),
+            details: "Cannot determine parent directory for safety check (potentially root path)."
+                .to_string(),
         })
     }
 }
+
+// --- REMOVED old check_nonexistent_target_safety function ---

src/processor/safety_tests.rs

@@ -0,0 +1,145 @@
+//! Unit tests specifically for the path safety logic in src/processor/safety.rs
+//! These tests live alongside the code they test.
+
+// Use assert_fs for temporary directory and file manipulation in tests
+use assert_fs::prelude::*;
+use assert_fs::TempDir;
+// Use crate::errors for ProcessError within the same crate
+use crate::errors::ProcessError;
+
+// Helper function to call the safety check directly using super::
+// It now takes TempDir directly for convenience in setting up paths.
+fn check_safety(temp_dir: &TempDir, target_rel_path: &str) -> Result<(), ProcessError> {
+    let base_path = temp_dir.path();
+    let target_path = base_path.join(target_rel_path);
+    // Need to canonicalize the base path first for the check function
+    let canonical_base = base_path
+        .canonicalize()
+        .expect("Failed to canonicalize base path for test setup");
+    // Call the function in the parent module's safety submodule
+    super::safety::ensure_path_safe(&canonical_base, &target_path)
+}
+
+// Helper to create a temp dir - replaces setup_temp_dir from test_common
+fn setup_temp_dir() -> TempDir {
+    TempDir::new().expect("Failed to create temporary directory for test")
+}
+
+#[test]
+fn test_safety_target_exists_safe() {
+    let temp_dir = setup_temp_dir();
+    temp_dir.child("safe_file.txt").write_str("safe").unwrap();
+    assert!(check_safety(&temp_dir, "safe_file.txt").is_ok());
+}
+
+#[test]
+fn test_safety_target_exists_unsafe() {
+    let temp_dir = setup_temp_dir();
+    // We can't easily create a file outside the temp dir in a portable way.
+    // Instead, we test the logic using relative paths that *would* escape.
+    // The function canonicalizes, so `base/../sibling` becomes `/path/to/sibling`.
+    let result = check_safety(&temp_dir, "../unsafe_file.txt");
+    assert!(matches!(result, Err(ProcessError::PathNotSafe { .. })));
+}
+
+#[test]
+fn test_safety_target_not_exist_parent_safe() {
+    let temp_dir = setup_temp_dir();
+    temp_dir.child("safe_dir").create_dir_all().unwrap();
+    // Target doesn't exist, but parent does and is inside base.
+    assert!(check_safety(&temp_dir, "safe_dir/new_file.txt").is_ok());
+}
+
+#[test]
+fn test_safety_target_not_exist_parent_unsafe() {
+    let temp_dir = setup_temp_dir();
+    // Parent resolves outside the base directory.
+    let result = check_safety(&temp_dir, "../unsafe_dir/new_file.txt");
+    assert!(matches!(result, Err(ProcessError::PathNotSafe { .. })));
+}
+
+#[test]
+fn test_safety_target_not_exist_parent_not_exist_safe() {
+    let temp_dir = setup_temp_dir();
+    // Neither target nor parent exists, but the logical path is within base.
+    assert!(check_safety(&temp_dir, "new_dir/new_file.txt").is_ok());
+}
+
+#[test]
+fn test_safety_target_not_exist_parent_not_exist_unsafe() {
+    let temp_dir = setup_temp_dir();
+    // Neither target nor parent exists, and the logical path points outside.
+    let result = check_safety(&temp_dir, "../new_unsafe_dir/new_file.txt");
+    // This case relies on the non-canonicalized check in check_nonexistent_target_safety
+    // or potentially fails during parent canonicalization if the unsafe parent *did* exist.
+    // Given the current implementation, it should detect the parent is outside base.
+    assert!(matches!(result, Err(ProcessError::PathNotSafe { .. })));
+}
+
+#[test]
+fn test_safety_intermediate_component_is_file() {
+    let temp_dir = setup_temp_dir();
+    temp_dir
+        .child("file_as_dir")
+        .write_str("I am a file")
+        .unwrap();
+    // Try to create something inside the path where 'file_as_dir' is treated as a directory.
+    let result = check_safety(&temp_dir, "file_as_dir/nested_file.txt");
+
+    // This should fail because the parent ('file_as_dir') is not a directory.
+    // The error can come from metadata check or canonicalization depending on exact flow.
+    assert!(
+        matches!(result, Err(ProcessError::ParentIsNotDirectory { .. }))
+            || matches!(result, Err(ProcessError::Io { .. })) // Canonicalize might return generic IO error sometimes
+            || matches!(result, Err(ProcessError::PathResolution { .. })), // Or path resolution error
+        "Expected ParentIsNotDirectory or related error, got {:?}",
+        result
+    );
+}
+
+#[test]
+#[cfg(unix)] // Test behavior with symlinks (more relevant on Unix)
+fn test_safety_symlink_inside_base() {
+    let temp_dir = setup_temp_dir();
+    temp_dir.child("target_dir").create_dir_all().unwrap();
+    temp_dir
+        .child("target_dir/actual_file.txt")
+        .write_str("actual")
+        .unwrap();
+    let link_path = temp_dir.path().join("safe_link");
+    std::os::unix::fs::symlink("target_dir/actual_file.txt", &link_path)
+        .expect("Failed to create symlink");
+
+    // Checking the link itself should be safe as it resides within base
+    assert!(check_safety(&temp_dir, "safe_link").is_ok());
+    // Checking a path *through* the link should also be safe if target is safe
+    assert!(check_safety(&temp_dir, "safe_link").is_ok()); // ensure_path_safe canonicalizes
+}
+
+#[test]
+#[cfg(unix)]
+fn test_safety_symlink_points_outside_base() {
+    let temp_dir = setup_temp_dir();
+    let sibling_dir = temp_dir
+        .path()
+        .parent()
+        .unwrap()
+        .join("strux_test_sibling_target");
+    // Use std::fs or fs_err for consistency within unit tests if needed elsewhere
+    std::fs::create_dir_all(&sibling_dir).expect("Failed to create sibling dir");
+    std::fs::write(sibling_dir.join("outside_file.txt"), "outside")
+        .expect("Failed to write outside file");
+
+    let link_path = temp_dir.path().join("unsafe_link");
+    // Create a link pointing outside the temp dir
+    std::os::unix::fs::symlink("../strux_test_sibling_target/outside_file.txt", &link_path)
+        .expect("Failed to create symlink");
+
+    // Accessing the link itself is okay (it lives inside base)
+    // But ensure_path_safe canonicalizes, revealing the unsafe target.
+    let result = check_safety(&temp_dir, "unsafe_link");
+    assert!(matches!(result, Err(ProcessError::PathNotSafe { .. })));
+
+    // Clean up the sibling directory
+    std::fs::remove_dir_all(&sibling_dir).expect("Failed to remove sibling dir");
+}

tests/cli.rs

@@ -11,6 +11,8 @@ mod basic;
 mod create;
 #[path = "cli/delete.rs"]
 mod delete;
+#[path = "cli/empty_input.rs"]
+mod empty_input;
 #[path = "cli/errors.rs"]
 mod errors;
 #[path = "cli/overwrite_skip.rs"]