fix: rename same-file detection via path canonicalization (#557)

SebTardif · web-flow · commit a1b5573a5737 · 2026-06-12T18:27:23.000Z
The rename command and tx engine file.rename used raw PathBuf
comparison to detect same-file renames. Paths like file.md and
./file.md produce different PathBuf values but resolve to the same
file. Without canonicalization, a rename with write policy transforms
(e.g. --ensure-final-newline) would write the transformed content to
the destination, then delete the source, destroying the file.

Now compares canonicalized paths in both validate_rename_paths() and
the tx engine FileRename handler.

Signed-off-by: Sebastien Tardif &lt;sebtardif@ncf.ca&gt;
diff --git a/README.md b/README.md
@@ -10,7 +10,7 @@
 [![Release](https://img.shields.io/github/v/release/patchloom/patchloom?logo=github&sort=semver)](https://github.com/patchloom/patchloom/releases/latest)
 [![License](https://img.shields.io/badge/license-MIT%2FApache--2.0-blue)](./LICENSE)
 
-[![Tests](https://img.shields.io/badge/tests-1354%20passing-brightgreen)](#)
+[![Tests](https://img.shields.io/badge/tests-1357%20passing-brightgreen)](#)
 [![Coverage](https://img.shields.io/endpoint?url=https://gist.githubusercontent.com/SebTardif/6a26adf6bfae45f530465f626c9154f4/raw/coverage.json)](https://github.com/patchloom/patchloom/actions/workflows/ci.yml)
 [![OpenSSF Best Practices](https://www.bestpractices.dev/projects/13097/badge)](https://www.bestpractices.dev/projects/13097)
 [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/patchloom/patchloom/badge)](https://securityscorecards.dev/viewer/?uri=github.com/patchloom/patchloom)
@@ -414,7 +414,7 @@ flowchart LR
 
 ## Status
 
-1354 passing tests across 20 commands. Tested with Grok 4.3, GPT-5.4, and Claude Opus 4.6.
+1357 passing tests across 20 commands. Tested with Grok 4.3, GPT-5.4, and Claude Opus 4.6.
 
 ## Full command reference
 
diff --git a/src/cmd/rename.rs b/src/cmd/rename.rs
@@ -57,7 +57,12 @@ fn validate_rename_paths(
     if dst.exists() && !dst.is_file() {
         anyhow::bail!("destination is not a file: {dst_label}");
     }
-    if src == dst {
+    if src == dst
+        || matches!(
+            (src.canonicalize(), dst.canonicalize()),
+            (Ok(ref s), Ok(ref d)) if s == d
+        )
+    {
         return Ok(RenameValidation::NoOp);
     }
     if !force && dst.exists() {
@@ -500,6 +505,29 @@ mod tests {
         assert_eq!(fs::read_to_string(&file).unwrap(), "hello\n");
     }
 
+    #[test]
+    fn rename_same_file_via_different_path_is_noop() {
+        let dir = TempDir::new().unwrap();
+        let file = dir.path().join("same.txt");
+        fs::write(&file, "hello\n").unwrap();
+
+        let mut global = global_for(&dir);
+        global.apply = true;
+
+        // Use "./same.txt" as destination - different string, same file.
+        let args = RenameArgs {
+            from: file.to_string_lossy().into_owned(),
+            to: dir.path().join("./same.txt").to_string_lossy().into_owned(),
+            force: true,
+            write: Default::default(),
+        };
+
+        let code = run(args, &global).unwrap();
+        assert_eq!(code, exit::SUCCESS);
+        assert!(file.exists(), "file must not be deleted");
+        assert_eq!(fs::read_to_string(&file).unwrap(), "hello\n");
+    }
+
     #[cfg(feature = "mcp")]
     #[test]
     fn apply_rename_same_path_is_noop_without_force() {
diff --git a/src/cmd/tx.rs b/src/cmd/tx.rs
@@ -1215,8 +1215,13 @@ fn execute_operation(op: &Operation, tx: &mut TxState<'_>) -> anyhow::Result<usi
                 anyhow::bail!("destination is not a file: {to}");
             }
 
-            // If source and destination are the same path, no-op.
-            if src_path == dst_path {
+            // If source and destination resolve to the same file, no-op.
+            if src_path == dst_path
+                || matches!(
+                    (src_path.canonicalize(), dst_path.canonicalize()),
+                    (Ok(ref s), Ok(ref d)) if s == d
+                )
+            {
                 return Ok(0);
             }
 
diff --git a/tests/integration.rs b/tests/integration.rs
@@ -4691,6 +4691,29 @@ fn test_rename_same_path_without_force_is_noop() {
     assert_eq!(fs::read_to_string(&path).unwrap(), "content\n");
 }
 
+#[test]
+fn test_rename_same_file_via_different_path_is_noop() {
+    let dir = TempDir::new().unwrap();
+    let file = dir.path().join("same.txt");
+    fs::write(&file, "content\n").unwrap();
+
+    // Use ./same.txt as destination - different string, same file.
+    let alt_path = dir.path().join("./same.txt");
+
+    Command::cargo_bin("patchloom")
+        .unwrap()
+        .arg("rename")
+        .arg(&file)
+        .arg(&alt_path)
+        .arg("--apply")
+        .arg("--force")
+        .assert()
+        .success();
+
+    assert!(file.exists(), "file must not be deleted");
+    assert_eq!(fs::read_to_string(&file).unwrap(), "content\n");
+}
+
 #[test]
 fn test_rename_force_overwrites() {
     let dir = TempDir::new().unwrap();
@@ -7496,6 +7519,40 @@ fn test_tx_file_rename_same_path_is_noop() {
     );
 }
 
+#[test]
+fn test_tx_file_rename_same_file_via_different_path_is_noop() {
+    let dir = TempDir::new().unwrap();
+    fs::write(dir.path().join("same.txt"), "keep me\n").unwrap();
+
+    let plan = serde_json::json!({
+        "version": "1",
+        "operations": [
+            {"op": "file.rename", "from": "same.txt", "to": "./same.txt", "force": true}
+        ]
+    });
+    let plan_file = dir.path().join("plan.json");
+    fs::write(&plan_file, serde_json::to_string(&plan).unwrap()).unwrap();
+
+    Command::cargo_bin("patchloom")
+        .unwrap()
+        .arg("--cwd")
+        .arg(dir.path())
+        .arg("tx")
+        .arg(&plan_file)
+        .arg("--apply")
+        .assert()
+        .success();
+
+    assert!(
+        dir.path().join("same.txt").exists(),
+        "file must not be deleted"
+    );
+    assert_eq!(
+        fs::read_to_string(dir.path().join("same.txt")).unwrap(),
+        "keep me\n"
+    );
+}
+
 #[test]
 fn test_batch_file_rename() {
     let dir = TempDir::new().unwrap();

Original file line number	Diff line number	Diff line change
`@@ -1215,8 +1215,13 @@ fn execute_operation(op: &Operation, tx: &mut TxState<'_>) -> anyhow::Result<usi`
`1215`	`1215`	`anyhow::bail!("destination is not a file: {to}");`
`1216`	`1216`	`}`
`1217`	`1217`
`1218`		`- // If source and destination are the same path, no-op.`
`1219`		`- if src_path == dst_path {`
	`1218`	`+ // If source and destination resolve to the same file, no-op.`
	`1219`	`+ if src_path == dst_path`
	`1220`	`+ \|\| matches!(`
	`1221`	`+ (src_path.canonicalize(), dst_path.canonicalize()),`
	`1222`	`+ (Ok(ref s), Ok(ref d)) if s == d`
	`1223`	`+ )`
	`1224`	`+ {`
`1220`	`1225`	`return Ok(0);`
`1221`	`1226`	`}`
`1222`	`1227`