fix: prevent data loss when copying directory to its own parent#1241
fix: prevent data loss when copying directory to its own parent#1241nfischer merged 6 commits intoshelljs:mainfrom
Conversation
…ljs#1092) When using `cp -R /path/to/dir /path/to/`, the destination resolves to the same directory as the source. Previously, this would silently truncate all files inside the directory because `copyFileSync` would open the destination file with 'w' mode (which truncates) before reading from the source — which is the same file. Now we detect same-path situations for recursive directory copies the same way we already do for individual files, emitting an error message and skipping the copy instead of destroying content. Also added a same-file guard inside `copyFileSync` as a safety net against data loss. Fixes shelljs#1092
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1241 +/- ##
=======================================
Coverage 97.09% 97.09%
=======================================
Files 36 36
Lines 1514 1517 +3
=======================================
+ Hits 1470 1473 +3
Misses 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove unused dest dir and fix misleading comment. Both src1 and src2 are being copied to their own parent, so both trigger same-file errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
Tentatively LGTM, but please look into the report about code coverage and see if we need a test case here.
Adds a test case for copying a single file to itself, verifying the same-file detection at the _cp level catches it and preserves data. The copyFileSync guard at line 29 is defense-in-depth for any future callers; the _cp function already catches this case before reaching it.
|
Good call, thanks for the review! I'll look into the codecov report and add a test case for the uncovered line. Will push a fix shortly. |
Move the same-file error reporting into copyFileSync and remove the duplicate check from _cp's file section. This ensures the copyFileSync guard is exercised by the existing "copy single file to itself" test, covering the previously uncovered line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added the test case to cover that missing line. codecov should be happy now! |
|
Hi @nfischer, all review comments have been addressed:
All 75 cp tests pass. Would you be able to take another look when you get a chance? Thanks! |
|
following up on this - the data loss scenario when copying a directory into its parent is pretty nasty. anything else you'd want to see? |
|
hey @nfischer, just wanted to check if you had a chance to look at the latest changes. all the review feedback has been addressed and tests are passing. let me know if there's anything else! |
|
Thank you for your patience. This looks great now! |
2 participants
Fixes #1092
Problem
shell.cp('-R', '/path/to/srcdir', '/path/to/')silently erases the contents of all files insidesrcdir. This happens because the resolved destination path is the same as the source, andcopyFileSyncopens the destination with'w'mode (truncating it to zero bytes) before reading from the source file — which is the same file.Real
cpin bash detects this and exits with"are the same file". ShellJS already had this check for individual file copies, but it was missing for recursive directory copies.Fix
_cp(), matching the existing behavior for individual files — emits"'X' and 'X' are the same file"and skips the copycopyFileSync()as a safety net so that even if called directly with identical src/dest paths, it bails out instead of truncatingTests
Added three new test cases:
All existing cp tests continue to pass. Verified the fix manually against the exact repro from the issue.