fix: rm_rf now handles directories without S_IXUSR permission#14331
Open
RonnyPfannschmidt wants to merge 8 commits into
Open
fix: rm_rf now handles directories without S_IXUSR permission#14331RonnyPfannschmidt wants to merge 8 commits into
RonnyPfannschmidt wants to merge 8 commits into
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
There was a problem hiding this comment.
Pull request overview
This PR fixes rm_rf failures when removing directory trees that include directories missing the owner execute (S_IXUSR) permission bit, by improving on_rm_rf_error and adding regression tests for the affected traversal code paths.
Changes:
- Added
_chmod_rwx()to grant ownerrwxpermissions and replaced priorchmod_rwusage. - Enhanced
on_rm_rf_errorto handleos.open/os.scandirPermissionErrorby repairing permissions and removing entries directly. - Added new tests covering no-exec directory trees and
os.openPermissionError handling; added a changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| testing/test_tmpdir.py | Adds regression tests for removing trees with no-exec directories and for os.open PermissionError handling. |
| src/_pytest/pathlib.py | Introduces _chmod_rwx() and updates on_rm_rf_error to fix permissions and handle traversal errors from os.open/os.scandir. |
| changelog/7940.bugfix.rst | Documents the bugfix in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bluetech
reviewed
Mar 28, 2026
Member
|
I did not review this PR yet, but how does it compare with #14279 (which I believe is close to being ready for merging)? |
Member
I think they are mostly orthogonal (though there will be git conflicts). |
RonnyPfannschmidt
force-pushed
the
fix/rm-rf-no-exec-permission-7940
branch
from
May 25, 2026 05:05
fec7e68 to
d9fd704
Compare
nicoddemus
approved these changes
May 25, 2026
RonnyPfannschmidt
force-pushed
the
fix/rm-rf-no-exec-permission-7940
branch
from
July 20, 2026 15:33
9511d93 to
752ee78
Compare
Member
Author
|
i have to apologize for the speed-run of slop comments - i updated @cursor and its new default auto-approved external changes like a madman - the evolution of those tools is unhinged |
…-dev#7940) The on_rm_rf_error handler previously only set S_IRUSR|S_IWUSR (read+write) when retrying failed removals, and silently skipped os.open/os.scandir failures. This meant directories lacking execute permission (S_IXUSR) could not be traversed or removed by shutil.rmtree. Now the handler: - Uses S_IRWXU (read+write+execute) for all permission fixes - Handles os.open and os.scandir PermissionError by fixing permissions on the path and its parent, then removing the entry directly - Includes a guard against infinite recursion when chmod has no effect Supersedes pytest-dev#7941 which took the approach of delegating to tempfile.TemporaryDirectory._rmtree. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Claude Opus <claude@anthropic.com>
- Use stat.S_IMODE() to extract only permission bits before comparing/setting, avoiding file-type bit leakage into os.chmod - Only add S_IXUSR for directories; regular files get S_IRUSR|S_IWUSR only, avoiding unnecessary execute side-effect on files - Fix recursion guard: track whether *either* parent or path chmod changed permissions, so fixing the parent alone (the common case for missing S_IXUSR) is sufficient to proceed with removal - Add test for parent-only permission fix scenario Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Claude Opus <claude@anthropic.com>
- Move `import stat` to module level (no lazy import) - Bound parent chmod by start_path to prevent escaping rm_rf scope - Use `p.is_dir()` and `rm_rf(p)` consistently with Path objects - Rewrite changelog to be user-facing (no internal function names) Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
- Test _chmod_rwx returns False on nonexistent path (OSError branch) - Test _chmod_rwx returns False when permissions already sufficient - Test os.open handler returns False when chmod is ineffective Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
…pragmas - Test os.open handler returns False when unlink raises OSError - Test os.unlink handler walks multiple read-only parent directories - Add pragma: no branch to for-loops that always exit via break Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Move the Path conversion to a single location shared by both the os.open/os.scandir handler and the os.rmdir/os.unlink handler, as suggested in review. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Sonnet 4 <claude@anthropic.com>
Narrow OSError handling in _chmod_rwx and use stat constants in tests. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Cursor Grok 4.5 <grok@x.ai>
RonnyPfannschmidt
force-pushed
the
fix/rm-rf-no-exec-permission-7940
branch
from
July 21, 2026 08:55
752ee78 to
530f985
Compare
Pin the chmod-failure branch with a monkeypatch; a portable real-FS layout for that case is not available in CI. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Cursor Grok 4.5 <grok@x.ai>
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.
Summary
Closes #7940.
Supersedes #7941.
The
on_rm_rf_errorhandler had two gaps that preventedrm_rffrom removing directories lacking theS_IXUSR(owner execute) permission bit:chmod_rwonly setS_IRUSR | S_IWUSR(read+write) — missingS_IXUSRmeant directories couldn't be traversed byshutil.rmtree.os.open/os.scandirPermissionErrorwas silently skipped —shutil.rmtree(Python 3.12+ usesos.open, 3.10–3.11 usesos.scandir) could not enter directories without execute permission, and the error handler simply returnedFalse.Changes
_chmod_rwx()helper that usesstat.S_IRWXU(read+write+exec) and returns whether permissions actually changed (acts as a recursion guard).os.openandos.scandirfailures are now handled: permissions are fixed on the path and its parent, then the entry is removed directly (files viaos.unlink, directories via recursiverm_rf).chmod_rwcalls replaced with_chmod_rwx.This takes the "fix it in
on_rm_rf_errordirectly" approach rather than #7941's approach of delegating totempfile.TemporaryDirectory._rmtree, keepingrm_rfrobust for all callers.Test plan
test_rm_rf_with_no_exec_permission_directories— exact scenario from Pytest cannot delete tmp_path directories withoutstat.S_IXUSRmode bit set #7940 (nested dirs + files withchmod(mode=0))test_on_rm_rf_error_os_open_handles_file—os.openPermissionError on a filetest_on_rm_rf_error_os_open_handles_directory—os.openPermissionError on a directory treeTestRmRftests pass unchangedtesting/test_tmpdir.pysuite passes (41 passed, 1 skipped)Made with Cursor