-
Notifications
You must be signed in to change notification settings - Fork 186
Fix Windows RecursionError in temporary directory cleanup #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add WindowsFileSystemCleaner class with multiple fallback strategies - Implement safe_rmtree with recursion protection (max depth 100) - Add retry logic for transient Windows filesystem errors - Support for locked files, permissions, and long path issues - Include safe temporary directory creation helpers Fixes RecursionError in Windows temporary directory cleanup operations.
- Add safe_temp_directory context manager with platform-aware cleanup - Implement SafeTemporaryDirectory as drop-in replacement for tempfile.TemporaryDirectory - Support custom prefixes, suffixes, and error handling options - Compatible interface with existing tempfile.TemporaryDirectory usage - Automatic cleanup using platform-specific safe removal methods Provides Windows-safe temporary directory management without code changes.
- Implement WindowsPatches class for automatic monkey patching - Replace tempfile.TemporaryDirectory with SafeTemporaryDirectory on Windows - Add context manager for temporary patch application - Auto-apply patches on module import for maximum compatibility - Provide manual patch control with apply/remove methods Enables transparent Windows filesystem fixes without modifying existing code.
- Add safe_rmtree method to PlatformAdapter class - Integrate Windows cleanup utilities for Windows platforms - Add create_safe_tempdir method with platform-specific handling - Maintain fallback to standard operations on non-Windows platforms - Extend platform adapter pattern to cover filesystem operations Completes cross-platform filesystem compatibility layer.
- Auto-apply Windows compatibility patches when claudecode module is imported - Graceful fallback if Windows patches are not available - Transparent activation without requiring code changes - Ensures maximum compatibility across all usage patterns Completes non-invasive Windows RecursionError fix implementation.
- Add 18 pytest tests covering all Windows cleanup functionality - Test platform detection, safe temp operations, context managers - Validate SafeTemporaryDirectory class behavior and error handling - Include Windows-specific functionality and cross-platform compatibility - Add performance and reliability tests with recursion protection - Provide integration test for complete workflow validation Ensures robust testing of Windows RecursionError fixes (191 total tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a critical Windows filesystem issue where RecursionError
was occurring in 5 workflow integration tests during temporary directory cleanup, caused by infinite recursion in shutil.rmtree()
.
The solution implements a comprehensive cross-platform adapter pattern with multiple fallback cleanup strategies:
- Windows-specific cleanup utilities with recursion protection
- Drop-in replacement for
tempfile.TemporaryDirectory
- Automatic monkey patching on Windows platforms
- Additional test coverage for the fix
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
claudecode/windows_patches.py |
Monkey-patching framework for Windows compatibility with auto-application on import |
claudecode/windows_cleanup.py |
Windows filesystem cleanup utilities with recursion protection and multiple fallback strategies |
claudecode/test_windows_cleanup.py |
Comprehensive test suite for Windows cleanup functionality |
claudecode/test_utils.py |
Enhanced test utilities with Windows-specific testing capabilities |
claudecode/safe_temp.py |
Platform-aware temporary directory management as drop-in replacement |
claudecode/platform_utils.py |
Cross-platform adapter for handling OS-specific command execution and filesystem operations |
claudecode/github_action_audit.py |
Updated to use platform adapter for Claude CLI execution |
claudecode/__init__.py |
Auto-applies Windows patches on module import |
README.md |
Added Windows setup and troubleshooting documentation |
""" | ||
Manager for Windows-specific monkey patches. | ||
Handles patching to fix Windows filesystem issues |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space in comment. Should be 'Handles patching' instead of 'Handles patching'.
Handles patching to fix Windows filesystem issues | |
Handles patching to fix Windows filesystem issues |
Copilot uses AI. Check for mistakes.
Args: | ||
max_retries: Retry attempts for locked files | ||
retry_delay: Delay between retries (seconds) | ||
max_depth: Max directory depth (prevents infinite recursion) |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The magic number 100 for max_depth should be documented or made configurable. Consider adding a constant or explaining why 100 is the appropriate recursion limit.
max_depth: Max directory depth (prevents infinite recursion) | |
def __init__(self, max_retries: int = 3, retry_delay: float = 0.1, max_depth: int = DEFAULT_MAX_DEPTH): | |
""" | |
Args: | |
max_retries: Retry attempts for locked files | |
retry_delay: Delay between retries (seconds) | |
max_depth: Max directory depth (prevents infinite recursion, default: DEFAULT_MAX_DEPTH) |
Copilot uses AI. Check for mistakes.
|
||
1. Install Node.js (v18 or later) and npm: | ||
- Download from https://nodejs.org or use nvm-windows. | ||
2. Install the Claude CLI:npm install -g @anthropic-ai/claude-code |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon. Should be '2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code'.
2. Install the Claude CLI:npm install -g @anthropic-ai/claude-code | |
2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code |
Copilot uses AI. Check for mistakes.
[Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
||
4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | ||
5. Verify installation:claude --version |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon. Should be '3. Add npm global directory to PATH: $npmPath = npm config get prefix'.
5. Verify installation:claude --version | |
2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code | |
3. Add npm global directory to PATH: $npmPath = npm config get prefix | |
[Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | |
4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | |
5. Verify installation: claude --version |
Copilot uses AI. Check for mistakes.
3. Add npm global directory to PATH:$npmPath = npm config get prefix | ||
[Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
||
4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon. Should be '4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned'.
4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | |
4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned |
Copilot uses AI. Check for mistakes.
[Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
||
4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | ||
5. Verify installation:claude --version |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon. Should be '5. Verify installation: claude --version'.
5. Verify installation:claude --version | |
5. Verify installation: claude --version |
Copilot uses AI. Check for mistakes.
|
||
Expected output: 1.0.71 (Claude Code) or similar. | ||
|
||
Troubleshooting |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'Troubleshooting' section should be formatted as a proper header with markdown syntax (## Troubleshooting).
Troubleshooting | |
## Troubleshooting |
Copilot uses AI. Check for mistakes.
Problem
Windows filesystem operations were causing
RecursionError
in 5 workflow integration tests due to infinite recursion inshutil.rmtree()
during temporary directory cleanup.Solution
tempfile.TemporaryDirectory