fix(safeoutputs): surface metadata errors and allow symlink-removal patches

Two review fixes for the symlink-exfiltration PR:

1. push_file_change_skipping_symlinks: split the catch-all arm so IO errors from symlink_metadata (e.g. permissions denied) are surfaced via warn! rather than silently swallowed. Non-file, non-symlink entries (directories, fifos, sockets) are still silently skipped.

2. validate_patch_paths: only reject patch lines that INTRODUCE a symlink (new file mode 120000, new mode 120000). Patches that convert a symlink into a regular file (old mode 120000 + new mode 100644) or delete an existing symlink (deleted file mode 120000) are now allowed — they are legitimate cleanup operations and produce a symlink-free worktree, so they pose no exfiltration risk.

Adds two positive tests covering symlink→file conversion and symlink deletion; existing negative tests still pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jamesadevine

src/safeoutputs/create_pull_request.rs

@@ -1814,7 +1814,15 @@ async fn push_file_change_skipping_symlinks(
                 file_path
             );
         }
-        _ => {}
+        Ok(_) => {
+            // Not a regular file (e.g. directory, fifo, socket) — silently skip.
+        }
+        Err(e) => {
+            warn!(
+                "Failed to read metadata for {}: {} — skipping",
+                file_path, e
+            );
+        }
     }
     Ok(())
 }
@@ -1905,14 +1913,23 @@ fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
             let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"');
             validate_single_path(path)?;
         } else if line.starts_with("new file mode 120000")
-            || line.starts_with("old mode 120000")
             || line.starts_with("new mode 120000")
         {
-            // Reject symlink entries (git mode 120000 = symbolic link).
-            // Symlinks in a patch could be used to make Stage 3 follow them to
-            // arbitrary filesystem paths (e.g. /proc/self/environ) when collecting
-            // file changes to upload to ADO.
-            anyhow::bail!("Patch contains a symlink entry (mode 120000), which is not allowed");
+            // Reject patch lines that INTRODUCE a symlink (git mode 120000).
+            // Either of these lines means the resulting tree contains a symlink:
+            //   - "new file mode 120000" — a freshly added symlink
+            //   - "new mode 120000"      — an existing file converted to a symlink
+            // A symlink in the worktree could make Stage 3 follow it to arbitrary
+            // filesystem paths (e.g. /proc/self/environ) when collecting file
+            // changes to upload to ADO.
+            //
+            // We deliberately do NOT reject "old mode 120000" on its own: a patch
+            // with "old mode 120000" + "new mode 100644" converts a symlink into a
+            // regular file, which is a legitimate cleanup operation and produces a
+            // safe worktree.
+            anyhow::bail!(
+                "Patch introduces a symlink (mode 120000), which is not allowed"
+            );
         }
     }
     Ok(())
@@ -2297,6 +2314,40 @@ index 0000000..abcdefg
         );
     }
 
+    #[test]
+    fn test_validate_patch_paths_symlink_to_file_allowed() {
+        // Converting a symlink into a regular file is a legitimate cleanup
+        // operation: the resulting worktree contains no symlinks, so there is
+        // no exfiltration risk. The "old mode 120000" line on its own must not
+        // cause rejection.
+        let patch = "diff --git a/file.txt b/file.txt\n\
+                     old mode 120000\n\
+                     new mode 100644\n\
+                     --- a/file.txt\n\
+                     +++ b/file.txt\n\
+                     @@ -1 +1 @@\n\
+                     -/etc/passwd\n\
+                     +hello world\n";
+        assert!(
+            validate_patch_paths(patch).is_ok(),
+            "patch converting symlink → regular file should be allowed"
+        );
+    }
+
+    #[test]
+    fn test_validate_patch_paths_symlink_deletion_allowed() {
+        // Deleting an existing symlink is also legitimate cleanup — no symlink
+        // remains in the worktree afterwards.
+        let patch = "diff --git a/link b/link\n\
+                     deleted file mode 120000\n\
+                     --- a/link\n\
+                     +++ /dev/null\n";
+        assert!(
+            validate_patch_paths(patch).is_ok(),
+            "patch deleting an existing symlink should be allowed"
+        );
+    }
+
     #[test]
     fn test_validate_single_path_valid() {
         assert!(validate_single_path("src/main.rs").is_ok());