Add error checking for rename() calls in DevPullCommand - #21
Merged
Merged
Conversation
Merged
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Co-authored-by: sak0a <24781653+sak0a@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
[WIP] WIP address feedback on 'Feature/dev pull command' PR
Add error checking for rename() calls in DevPullCommand
Feb 7, 2026
sak0a
marked this pull request as ready for review
February 7, 2026 17:19
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of notur:dev:pull by detecting filesystem move failures during preservation/restoration of heavy install artifacts (vendor/, node_modules/) and surfacing actionable errors instead of silently continuing.
Changes:
- Check
rename()return values when preservingvendor/andnode_modules/ - Check
rename()return values when restoring preserved directories - Throw
RuntimeExceptionwith source/destination paths and guidance when operations fail
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+275
to
+279
| $tmpDest = $tmpPreserve . '/' . $dir; | ||
| if (!rename($dirPath, $tmpDest)) { | ||
| throw new \RuntimeException( | ||
| "Failed to preserve directory: {$dirPath} to {$tmpDest}. " . | ||
| "Check permissions and ensure the directory is not locked." |
There was a problem hiding this comment.
Using sys_get_temp_dir() as the preserve destination can cause rename() to fail with cross-device link errors (EXDEV) when the system temp directory is on a different filesystem than the project/vendor directory. To make this robust, consider creating the preserve directory on the same filesystem as $noturRoot (e.g., a hidden directory adjacent to it) or falling back to a copy+delete strategy when rename() fails with EXDEV.
| @@ -272,7 +272,13 @@ private function replaceVendorFiles(string $noturRoot, string $sourcePath): void | |||
| if (!is_dir($tmpPreserve)) { | |||
| mkdir($tmpPreserve, 0755, true); | |||
There was a problem hiding this comment.
mkdir($tmpPreserve, 0755, true) is not checked for failure. If the temp directory cannot be created (permissions, full disk), the subsequent rename() error will be misleading. Consider checking the mkdir() return value (and/or re-checking is_dir after) and throwing a specific exception when creation fails.
Suggested change
| mkdir($tmpPreserve, 0755, true); | |
| if (!@mkdir($tmpPreserve, 0755, true) && !is_dir($tmpPreserve)) { | |
| throw new \RuntimeException( | |
| "Failed to create temporary preserve directory: {$tmpPreserve}. " . | |
| "Check permissions, available disk space, and directory ownership." | |
| ); | |
| } |
Comment on lines
+276
to
+281
| if (!rename($dirPath, $tmpDest)) { | ||
| throw new \RuntimeException( | ||
| "Failed to preserve directory: {$dirPath} to {$tmpDest}. " . | ||
| "Check permissions and ensure the directory is not locked." | ||
| ); | ||
| } |
There was a problem hiding this comment.
In Laravel/Illuminate commands, a failed rename() can emit an E_WARNING that the framework converts into an ErrorException, which can bypass this if (!rename(...)) branch and prevent your custom RuntimeException message from being used. Consider suppressing the warning (e.g., @rename) and then throwing based on the boolean return (optionally capturing the last error message for extra detail).
Comment on lines
+301
to
+306
| if (!rename($tmpSource, $destPath)) { | ||
| throw new \RuntimeException( | ||
| "Failed to restore directory: {$tmpSource} to {$destPath}. " . | ||
| "Check permissions and ensure the directory is not locked." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same as above: rename() failure may raise an E_WARNING that gets converted to an exception before returning false, so this branch might never run and the error output may be less actionable than intended. Suppressing the warning and throwing based on the return value will ensure this message is consistently used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
replaceVendorFiles()method usesrename()to preserve/restorevendor/andnode_modules/directories but doesn't check return values. Failures due to permissions or locks could silently corrupt install artifacts.Changes
rename()return values for both preserve and restore operationsRuntimeExceptionwith actionable error messages on failure✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.