fix: prevent data loss when copying directory to its own parent by veeceey · Pull Request #1241 · shelljs/shelljs

veeceey · 2026-02-13T08:53:45Z

Problem

shell.cp('-R', '/path/to/srcdir', '/path/to/') silently erases the contents of all files inside srcdir. This happens because the resolved destination path is the same as the source, and copyFileSync opens the destination with 'w' mode (truncating it to zero bytes) before reading from the source file — which is the same file.

Real cp in 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

Added a same-path check in the recursive directory copy branch of _cp(), matching the existing behavior for individual files — emits "'X' and 'X' are the same file" and skips the copy
Added a same-file guard in copyFileSync() as a safety net so that even if called directly with identical src/dest paths, it bails out instead of truncating

Tests

Added three new test cases:

Copy a directory to the parent it already lives in
Same scenario with trailing slash on destination
Multiple sources where one resolves to the same location (should error for that one and continue with the rest)

All existing cp tests continue to pass. Verified the fix manually against the exact repro from the issue.

…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

nfischer · 2026-02-14T09:13:19Z

test/cp.js

+  // Also create a separate dest dir
+  shell.mkdir(`${t.context.tmp}/dest`);
+
+  // Copy src1 to dest (should succeed) and src1 to its own parent (should fail)


This comment and the following line of code don't seem to match up together. You say that copying src1 to dest should succeed, but I don't see a line of code in this unit test which does that.

You're right, the comment was misleading. I've updated it to accurately describe what the test does: both directories are being copied to their own parent, so both should fail with the same-file error.

Now the name of the test case doesn't match up with what the test is doing.

I think you want to go in the other direction: create a subfolder structure that looks like:

test's temp directory | +— src1 | +— destination | +— src2

Then your code looks like:

const result = shell.cp('-R', `${t.context.tmp}/src1`, `${t.context.tmp}/destination/src2`, `${t.context.tmp}/destination`); // src1 copy should succeed, src2 copy should fail (but the original src2 is still intact)

Thanks for the suggestion on the test structure! The previous commit already restructured it with src1 in the tmp root and src2 inside destination, so src1 succeeds and src2 fails with the same-file error. I believe the test now matches what you described.

nfischer · 2026-02-14T09:13:43Z

test/cp.js

+  shell.ShellString('content2').to(`${t.context.tmp}/src2/file`);
+
+  // Also create a separate dest dir
+  shell.mkdir(`${t.context.tmp}/dest`);


As far as I can tell, this folder is unused in this test.

Good catch! Removed the unused dest dir and updated the comment. Both src1 and src2 are being copied to their own parent directory, so both trigger the same-file error. Fixed in the latest push.

codecov · 2026-02-14T09:15:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.09%. Comparing base (1dd3fe7) to head (6e4b04a).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Tentatively LGTM, but please look into the report about code coverage and see if we need a test case here.

nfischer · 2026-02-16T21:24:13Z

src/cp.js


+  // Bail if src and dest are the same file to avoid data loss
+  if (path.relative(srcFile, destFile) === '') {
+    return;


Codecov says this line is not covered by the tests. Can you look into why and see if we need to add a test case?

Good catch! I've added a test for copying a single file to itself (e.g. cp myfile.txt myfile.txt), which covers the same-file detection path in _cp. The guard in copyFileSync at line 29 is defense-in-depth — the _cp function catches the same-file case before reaching copyFileSync, so that specific return won't show as covered in normal usage. But the new test does improve overall coverage by exercising the file-level same-file check. Let me know if you'd like me to approach this differently.

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.

veeceey · 2026-02-20T05:50:18Z

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>

veeceey · 2026-02-20T06:00:49Z

Added the test case to cover that missing line. codecov should be happy now!

veeceey · 2026-02-24T07:33:37Z

Hi @nfischer, all review comments have been addressed:

Test mismatch / unused folder — Restructured the multi-dir copy test so src1 lives in the tmp root and src2 lives inside destination/. The cp command copies both into destination, so src1 succeeds while src2 triggers the same-file error. Unused folder removed.
Codecov coverage gap on copyFileSync line 29 — Moved the same-file error reporting into copyFileSync itself (and removed the duplicate check from _cp's file branch). The "copy single file to itself" test now exercises that path directly. Codecov confirms all modified lines are covered.

All 75 cp tests pass. Would you be able to take another look when you get a chance? Thanks!

veeceey · 2026-03-15T03:04:48Z

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?

veeceey · 2026-04-09T07:43:19Z

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!

nfischer reviewed Feb 14, 2026

View reviewed changes

veeceey and others added 2 commits February 15, 2026 17:33

fix: address review feedback on multi-dir copy test

f3ce200

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>

fix: restructure multi-dir copy test with proper src1 success case

ad171c0

nfischer reviewed Feb 16, 2026

View reviewed changes

veeceey added 2 commits February 18, 2026 00:40

Merge branch 'main' into fix/issue-1092-cp-data-loss

b4de336

Conversation

veeceey commented Feb 13, 2026

Problem

Fix

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veeceey commented Feb 20, 2026

Uh oh!

veeceey commented Feb 20, 2026

Uh oh!

veeceey commented Feb 24, 2026

Uh oh!

veeceey commented Mar 15, 2026

Uh oh!

veeceey commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Feb 14, 2026 •

edited

Loading