Fix/plugin permission errors #180
Conversation
- Display controller now checks plugin_instance.modes first before falling back to manifest - This allows plugins to dynamically provide modes based on enabled leagues - Fixes issue where disabled leagues (WNBA, NCAAW) appeared in available modes - Plugins can now control their available modes at runtime based on config
- Added _safe_remove_directory() method to handle permission errors gracefully - Fixes permissions on __pycache__ directories before removal - Updates uninstall_plugin() and install methods to use safe removal - Resolves [Errno 13] Permission denied errors during plugin install/uninstall
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 8. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
📝 WalkthroughWalkthroughDisplay mode resolution now prefers a plugin instance's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/plugin_system/store_manager.py (1)
1390-1455: Solid error handling approach; minor refinements suggested.The multi-layered approach (normal removal → fix permissions → ignore_errors) is robust for handling
__pycache__permission issues. A few refinements would address static analysis hints and improve debugging:
- Unused loop variable: Rename
dirsto_dirsto indicate it's intentionally unused- Better error logging: Use
logging.exceptionto preserve stack traces for debugging- The 0o777 permissions (S103): Acceptable here since permissions are temporary before deletion
♻️ Suggested refinements
- for root, dirs, files in os.walk(path): + for root, _dirs, files in os.walk(path): root_path = Path(root)- except Exception as e2: - self.logger.error(f"Failed to remove {path} even after fixing permissions: {e2}") + except Exception: + self.logger.exception(f"Failed to remove {path} even after fixing permissions")- self.logger.error(f"Could not remove {path} even with ignore_errors") + self.logger.warning(f"Could not remove {path} even with ignore_errors - directory may require manual cleanup")- except Exception as e3: - self.logger.error(f"Final removal attempt failed for {path}: {e3}") + except Exception: + self.logger.exception(f"Final removal attempt failed for {path}")- except Exception as e: - self.logger.error(f"Unexpected error removing {path}: {e}") + except Exception: + self.logger.exception(f"Unexpected error removing {path}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/display_controller.pysrc/plugin_system/store_manager.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
**/*.py: Prefer clear, readable code over clever optimizations (Simplicity First principle)
Make intentions clear through naming and structure (Explicit over Implicit principle)
Validate inputs and handle errors early (Fail Fast principle)
Use docstrings for classes and complex functions
Use PascalCase for class names (e.g.,NHLRecentManager)
Use snake_case for function and variable names (e.g.,fetch_game_data)
Use UPPER_SNAKE_CASE for constants (e.g.,ESPN_NHL_SCOREBOARD_URL)
Use leading underscore for private methods (e.g.,_fetch_data)
Use structured logging with context (e.g.,[NHL Recent]) for logging messages
Catch specific exceptions, not bareexcept:statements
Provide user-friendly error messages that explain what went wrong and potential solutions
Implement graceful degradation to continue operation when non-critical features fail
Use type hints for function parameters and return values
Validate required configuration fields on initialization
Provide sensible default values in code rather than in configuration files
Handle different deployment contexts with environment awareness in code
**/*.py: Code must run on Raspberry Pi, not Windows development machine
Optimize code for Raspberry Pi's limited RAM and CPU capabilities
Clean up resources regularly to manage memory effectively
Implement comprehensive logging for remote debugging on Raspberry Pi
Provide clear error messages for troubleshooting
Files:
src/plugin_system/store_manager.pysrc/display_controller.py
**/*manager*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
All sports managers should implement the BaseManager pattern with
__init__(self, config, display_manager, cache_manager),update()for fetching/processing data, anddisplay(self, force_clear=False)for rendering
Files:
src/plugin_system/store_manager.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
All sports, weather, and stock managers should be organized in src/ directory
src/**/*.py: Use rpi-rgb-led-matrix library for hardware control on Raspberry Pi
Use cache_manager.py for data persistence and caching
Implement non-blocking data fetching using background services
Minimize unnecessary redraws to optimize LED matrix display performance
Implement health checks and status reporting
Provide web interface for remote configuration and monitoring on Raspberry Pi
Files:
src/plugin_system/store_manager.pysrc/display_controller.py
src/*.py
📄 CodeRabbit inference engine (.cursor/rules/configuration-management.mdc)
src/*.py: Check required fields and data types on startup
Provide sensible defaults in code, not just config
Handle development vs production differences in configuration
Check that configuration intervals are reasonable (range validation)
Verify required services are available during configuration dependency checking
Provide fallback/default values when configuration is missing or invalid
Handle configuration changes between versions with migration logic
src/*.py: Use structured logging prefixes like[NHL Recent],[NFL Live]for consistent identification of log sources
Include relevant context information in logs such as team names, game status, and dates
Useinfolevel for normal operations and status updates,debuglevel for detailed troubleshooting information,warninglevel for non-critical issues, anderrorlevel for problems requiring attention
Catch specific exception types likerequests.exceptions.RequestExceptionrather than generic exceptions, and log errors with descriptive context
Provide user-friendly error messages that explain the situation (e.g., 'No games available during off-season'), provide context, suggest solutions, and distinguish between different issue types (API problems vs no data vs filtering results)
Implement graceful degradation by using fallback content, cached data, and allowing continued operation when non-critical features fail
Implement comprehensive logging throughout the application including API responses, filtering results, and display updates to support debugging
Log state transitions and track current state to aid in debugging and understanding application flow
Monitor and log performance metrics including timing and resource usage
Include stack traces in error logs to provide complete error context for debugging
Use seasonal messaging that provides different context and information depending on the time of year (e.g., different messages for off-season vs active season)
Include helpful context in off-seas...
Files:
src/display_controller.py
src/display_controller.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Display management logic should be implemented in src/display_controller.py
Files:
src/display_controller.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Support different display sizes by checking `display_manager.width` and `display_manager.height`
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manifest.json : `manifest.json` must include required fields: id, name, version, entry_point, class_name, and display_modes
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Plugin class must implement `display()` method for rendering
Applied to files:
src/display_controller.py
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Support different display sizes by checking `display_manager.width` and `display_manager.height`
Applied to files:
src/display_controller.py
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Plugins with live content must implement `get_live_modes()` to specify which modes are live
Applied to files:
src/display_controller.py
📚 Learning: 2025-12-27T19:17:31.048Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-12-27T19:17:31.048Z
Learning: Applies to src/display_controller.py : Display management logic should be implemented in [src/display_controller.py](mdc:src/display_controller.py)
Applied to files:
src/display_controller.py
📚 Learning: 2025-12-27T19:16:24.801Z
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manifest.json : `manifest.json` must include required fields: id, name, version, entry_point, class_name, and display_modes
Applied to files:
src/display_controller.py
🪛 Ruff (0.14.10)
src/plugin_system/store_manager.py
1410-1410: Consider moving this statement to an else block
(TRY300)
1418-1418: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
1422-1422: os.chmod setting a permissive mask 0o777 on file or directory
(S103)
1430-1430: os.chmod setting a permissive mask 0o777 on file or directory
(S103)
1437-1437: Consider moving this statement to an else block
(TRY300)
1438-1438: Do not catch blind exception: Exception
(BLE001)
1439-1439: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1448-1448: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1450-1450: Do not catch blind exception: Exception
(BLE001)
1451-1451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1453-1453: Do not catch blind exception: Exception
(BLE001)
1454-1454: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
src/display_controller.py (1)
253-262: LGTM! Clean priority resolution for display modes.The logic correctly prioritizes plugin-provided
modes(enabling dynamic mode lists based on enabled leagues/features) with appropriate fallback to manifest configuration. Thelist()call ensures the plugin's internal state isn't mutated, and the debug logging will aid troubleshooting.Based on learnings, this aligns well with the manifest requirement for
display_modeswhile allowing plugins to dynamically adjust their available modes at runtime.src/plugin_system/store_manager.py (3)
775-778: LGTM! Proper failure propagation.Correctly replaces the previous silent failure path with explicit error handling and appropriate return value.
1010-1014: LGTM! Structured error response.Properly returns a failure dict with actionable error message, enabling API callers to understand and report the failure.
1508-1513: LGTM! Clean integration.The uninstall operation now properly handles permission errors that can occur with
__pycache__directories, improving reliability on Raspberry Pi deployments where such issues are more common.
- Rename unused 'dirs' variable to '_dirs' to indicate intentional non-use - Use logger.exception() instead of logger.error() to preserve stack traces - Add comment explaining 0o777 permissions are acceptable (temporary before deletion)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin_system/store_manager.py (1)
834-834: Consider using_safe_remove_directoryhere for consistency.This
shutil.rmtree(correct_path)call doesn't useignore_errorsor the new_safe_remove_directoryhelper. It could fail with the same permission errors this PR is addressing, causing manifest ID rename operations to fail unexpectedly.🐛 Suggested fix
if correct_path.exists(): self.logger.warning(f"Target directory {manifest_plugin_id} already exists, removing it") - shutil.rmtree(correct_path) + if not self._safe_remove_directory(correct_path): + self.logger.error(f"Failed to remove target directory: {correct_path}") + return False
🧹 Nitpick comments (1)
src/plugin_system/store_manager.py (1)
1390-1455: Good approach for handling permission errors during directory removal.The multi-layered retry strategy with permission fixes is appropriate for handling
__pycache__permission issues. The permissive 0o777 mode is acceptable here since it's temporary before deletion.Minor logging improvement:
logging.exception()automatically includes the exception traceback, so including the exception object in the message is redundant (per TRY401).🔧 Optional: Simplify exception logging
except Exception as e2: - self.logger.exception(f"Failed to remove {path} even after fixing permissions: {e2}") + self.logger.exception(f"Failed to remove {path} even after fixing permissions") # Last resort: try with ignore_errors try: shutil.rmtree(path, ignore_errors=True) # Check if it actually got removed if not path.exists(): self.logger.warning(f"Removed {path} with ignore_errors=True (some files may remain)") return True else: self.logger.error(f"Could not remove {path} even with ignore_errors") return False except Exception as e3: - self.logger.exception(f"Final removal attempt failed for {path}: {e3}") + self.logger.exception(f"Final removal attempt failed for {path}") return False except Exception as e: - self.logger.exception(f"Unexpected error removing {path}: {e}") + self.logger.exception(f"Unexpected error removing {path}") return False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plugin_system/store_manager.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
**/*.py: Prefer clear, readable code over clever optimizations (Simplicity First principle)
Make intentions clear through naming and structure (Explicit over Implicit principle)
Validate inputs and handle errors early (Fail Fast principle)
Use docstrings for classes and complex functions
Use PascalCase for class names (e.g.,NHLRecentManager)
Use snake_case for function and variable names (e.g.,fetch_game_data)
Use UPPER_SNAKE_CASE for constants (e.g.,ESPN_NHL_SCOREBOARD_URL)
Use leading underscore for private methods (e.g.,_fetch_data)
Use structured logging with context (e.g.,[NHL Recent]) for logging messages
Catch specific exceptions, not bareexcept:statements
Provide user-friendly error messages that explain what went wrong and potential solutions
Implement graceful degradation to continue operation when non-critical features fail
Use type hints for function parameters and return values
Validate required configuration fields on initialization
Provide sensible default values in code rather than in configuration files
Handle different deployment contexts with environment awareness in code
**/*.py: Code must run on Raspberry Pi, not Windows development machine
Optimize code for Raspberry Pi's limited RAM and CPU capabilities
Clean up resources regularly to manage memory effectively
Implement comprehensive logging for remote debugging on Raspberry Pi
Provide clear error messages for troubleshooting
Files:
src/plugin_system/store_manager.py
**/*manager*.py
📄 CodeRabbit inference engine (.cursor/rules/coding-standards.mdc)
All sports managers should implement the BaseManager pattern with
__init__(self, config, display_manager, cache_manager),update()for fetching/processing data, anddisplay(self, force_clear=False)for rendering
Files:
src/plugin_system/store_manager.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
All sports, weather, and stock managers should be organized in src/ directory
src/**/*.py: Use rpi-rgb-led-matrix library for hardware control on Raspberry Pi
Use cache_manager.py for data persistence and caching
Implement non-blocking data fetching using background services
Minimize unnecessary redraws to optimize LED matrix display performance
Implement health checks and status reporting
Provide web interface for remote configuration and monitoring on Raspberry Pi
Files:
src/plugin_system/store_manager.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Support different display sizes by checking `display_manager.width` and `display_manager.height`
Learnt from: CR
Repo: ChuckBuilds/LEDMatrix PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-27T19:16:24.801Z
Learning: Applies to plugins/*/manager.py : Plugins with live content must implement `get_live_modes()` to specify which modes are live
🪛 Ruff (0.14.10)
src/plugin_system/store_manager.py
1410-1410: Consider moving this statement to an else block
(TRY300)
1422-1422: os.chmod setting a permissive mask 0o777 on file or directory
(S103)
1430-1430: os.chmod setting a permissive mask 0o777 on file or directory
(S103)
1437-1437: Consider moving this statement to an else block
(TRY300)
1439-1439: Redundant exception object included in logging.exception call
(TRY401)
1448-1448: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1451-1451: Redundant exception object included in logging.exception call
(TRY401)
1454-1454: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (3)
src/plugin_system/store_manager.py (3)
775-777: LGTM!Good integration of the new safe removal helper. The early return on failure prevents cascading issues during reinstallation.
1010-1014: LGTM!The structured error response is consistent with the method's return pattern and provides clear feedback when directory removal fails.
1508-1513: LGTM!Clean integration with appropriate success and failure logging.
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.