fix(archive): fall back to copy+remove on EPERM/EXDEV (fixes #197) #605
Conversation
…AI#197) On Windows, fs.rename() often fails with EPERM when moving non-empty directories. Fall back to recursive copy then rm when rename throws EPERM or EXDEV so 'openspec archive' succeeds where Move-Item works.
📝 WalkthroughWalkthroughAdds two internal helper functions to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR fixes a Windows-specific issue where Key Changes:
Implementation Quality: Minor Consideration: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ArchiveCommand
participant moveDirectory
participant fs
participant copyDirRecursive
User->>ArchiveCommand: execute(changeName)
ArchiveCommand->>ArchiveCommand: Validate change exists
ArchiveCommand->>ArchiveCommand: Create archive path
ArchiveCommand->>moveDirectory: moveDirectory(changeDir, archivePath)
moveDirectory->>fs: rename(src, dest)
alt Rename succeeds
fs-->>moveDirectory: Success
moveDirectory-->>ArchiveCommand: Directory moved
else Rename fails with EPERM/EXDEV
fs-->>moveDirectory: Error (EPERM or EXDEV)
moveDirectory->>copyDirRecursive: copyDirRecursive(src, dest)
copyDirRecursive->>fs: mkdir(dest, recursive)
copyDirRecursive->>fs: readdir(src, withFileTypes)
loop For each entry
alt Entry is directory
copyDirRecursive->>copyDirRecursive: copyDirRecursive(srcPath, destPath)
else Entry is file
copyDirRecursive->>fs: copyFile(srcPath, destPath)
end
end
copyDirRecursive-->>moveDirectory: Copy complete
moveDirectory->>fs: rm(src, recursive, force)
fs-->>moveDirectory: Source removed
moveDirectory-->>ArchiveCommand: Directory moved via copy+remove
else Other error
fs-->>moveDirectory: Error (other)
moveDirectory-->>ArchiveCommand: Throw error
end
ArchiveCommand-->>User: Archive complete
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function copyDirRecursive(src: string, dest: string): Promise<void> { | ||
| await fs.mkdir(dest, { recursive: true }); | ||
| const entries = await fs.readdir(src, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const srcPath = path.join(src, entry.name); | ||
| const destPath = path.join(dest, entry.name); | ||
| if (entry.isDirectory()) { | ||
| await copyDirRecursive(srcPath, destPath); | ||
| } else { | ||
| await fs.copyFile(srcPath, destPath); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider handling symlinks - copyFile follows symlinks rather than copying them, which could lead to unexpected behavior if the change directory contains symlinks. Use fs.lstat instead of checking entry.isDirectory() and handle symlinks with fs.symlink to preserve them.
| async function copyDirRecursive(src: string, dest: string): Promise<void> { | |
| await fs.mkdir(dest, { recursive: true }); | |
| const entries = await fs.readdir(src, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const srcPath = path.join(src, entry.name); | |
| const destPath = path.join(dest, entry.name); | |
| if (entry.isDirectory()) { | |
| await copyDirRecursive(srcPath, destPath); | |
| } else { | |
| await fs.copyFile(srcPath, destPath); | |
| } | |
| } | |
| } | |
| async function copyDirRecursive(src: string, dest: string): Promise<void> { | |
| await fs.mkdir(dest, { recursive: true }); | |
| const entries = await fs.readdir(src, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const srcPath = path.join(src, entry.name); | |
| const destPath = path.join(dest, entry.name); | |
| if (entry.isSymbolicLink()) { | |
| const target = await fs.readlink(srcPath); | |
| await fs.symlink(target, destPath); | |
| } else if (entry.isDirectory()) { | |
| await copyDirRecursive(srcPath, destPath); | |
| } else { | |
| await fs.copyFile(srcPath, destPath); | |
| } | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/archive.ts
Line: 16:28
Comment:
Consider handling symlinks - `copyFile` follows symlinks rather than copying them, which could lead to unexpected behavior if the change directory contains symlinks. Use `fs.lstat` instead of checking `entry.isDirectory()` and handle symlinks with `fs.symlink` to preserve them.
```suggestion
async function copyDirRecursive(src: string, dest: string): Promise<void> {
await fs.mkdir(dest, { recursive: true });
const entries = await fs.readdir(src, { withFileTypes: true });
for (const entry of entries) {
const srcPath = path.join(src, entry.name);
const destPath = path.join(dest, entry.name);
if (entry.isSymbolicLink()) {
const target = await fs.readlink(srcPath);
await fs.symlink(target, destPath);
} else if (entry.isDirectory()) {
await copyDirRecursive(srcPath, destPath);
} else {
await fs.copyFile(srcPath, destPath);
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
I don't think handling for symlinks makes sense here mr greptile
There was a problem hiding this comment.
You're right. For the specific use case of archiving OpenSpec changes, symlink handling would be overkill. The changes directory typically contains straightforward files and folders - handling symlinks would add unnecessary complexity for a scenario that's unlikely to occur in practice.
The current implementation with copyFile and directory recursion is appropriate for the archive functionality.
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
2 participants
On Windows,
openspec archivecould fail with:even when there is no real permission issue and the same move works in PowerShell.
Cause
fs.rename()often throws EPERM for non-empty directories on Windows (and EXDEV across devices). The CLI now treats EPERM/EXDEV as non-fatal and falls back to copy-then-remove: recursively copy the change dir into the archive path, thenfs.rm(src, { recursive: true, force: true }).Changes
moveDirectory()insrc/core/archive.ts: tryfs.rename; on EPERM/EXDEV usecopyDirRecursive+fs.rm.Testing
test/core/archive.test.tsexercises the archive path; the new behavior is used whenever rename throws EPERM/EXDEV.openspec archive <change> --yeson Windows in a repo where it previously failed; it should complete and the change should appear underopenspec/changes/archive/.Fixes #197
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.