feat: support local plugin install#8448
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
--editable/-eflag name suggests an editable install (e.g., symlink) butinstall_local_plugindoes a full copy; consider renaming the flag or adjusting behavior to avoid surprising users. - Before using
plugin_nameas a directory name ininstall_local_plugin, consider validating/sanitizing it (e.g., disallowing path separators or reserved names) to avoid filesystem issues or unintended paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `--editable/-e` flag name suggests an editable install (e.g., symlink) but `install_local_plugin` does a full copy; consider renaming the flag or adjusting behavior to avoid surprising users.
- Before using `plugin_name` as a directory name in `install_local_plugin`, consider validating/sanitizing it (e.g., disallowing path separators or reserved names) to avoid filesystem issues or unintended paths.
## Individual Comments
### Comment 1
<location path="astrbot/cli/utils/plugin.py" line_range="208-209" />
<code_context>
+ f"Local plugin {source_path} must contain metadata.yaml with a valid name"
+ )
+
+ target_path = plugins_dir / plugin_name
+ if target_path.exists():
+ raise click.ClickException(f"Plugin {plugin_name} already exists")
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid deleting an existing plugin directory on concurrent installs or pre-existing files
The `if target_path.exists(): raise ...` pattern combined with `copytree` and unconditional `rmtree` on failure is racy: if another process creates the directory between the existence check and `copytree`, `copytree` will fail and your handler may delete that newly created directory. To avoid this, either:
- Check for the specific "already exists" error (e.g., with `dirs_exist_ok=False`) and skip cleanup in that case, or
- Track whether this invocation created the directory and only remove it when that is true.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to install plugins from a local directory, adding an --editable (-e) option to the install command. The feedback focuses on correcting the implementation of the editable installation: instead of copying the directory, an editable install should create a symlink to the source directory to allow live updates. Additionally, non-editable installations should ignore common VCS, virtual environment, and IDE directories during copying. The feedback also suggests updating the CLI command and adding test assertions to verify symlink creation for editable installs and standard directory copying for non-editable installs.
| assert result.exit_code == 0 | ||
| assert ( | ||
| root / "data" / "plugins" / "astrbot_plugin_local_demo" / "metadata.yaml" | ||
| ).exists() |
There was a problem hiding this comment.
Add an assertion to verify that the non-editable local installation copies the directory instead of creating a symlink.
| assert result.exit_code == 0 | |
| assert ( | |
| root / "data" / "plugins" / "astrbot_plugin_local_demo" / "metadata.yaml" | |
| ).exists() | |
| target = root / "data" / "plugins" / "astrbot_plugin_local_demo" | |
| assert result.exit_code == 0 | |
| assert not target.is_symlink() | |
| assert (target / "metadata.yaml").exists() |
There was a problem hiding this comment.
Pull request overview
Adds CLI support for installing plugins from a local directory and ignores Zed editor settings.
Changes:
- New
install_local_plugin()helper that validates a local plugin directory, readsmetadata.yamlfor the name, and copies it intodata/plugins/<name>. - Updates
astrbot plugin installto accept-e/--editable PATHand to dispatch to the local-install path when the positionalnamehappens to be an existing directory. - Adds CLI tests for the new flow and adds
.zed/to.gitignore.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| astrbot/cli/commands/cmd_plug.py | Makes name optional, adds -e/--editable option, and dispatches to local install when appropriate. |
| astrbot/cli/utils/plugin.py | Adds install_local_plugin() helper that copies a local plugin directory into the plugins dir. |
| astrbot/cli/utils/init.py | Re-exports install_local_plugin. |
| tests/test_cli_plugin.py | New CLI tests covering editable install, path-as-name install, conflict, and missing-argument errors. |
| .gitignore | Ignores .zed/ editor workspace settings. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@copilot 审查一遍 |
Summary
astrbot plugin install -e PATHfor installing plugins from local directoriesastrbot plugin install PATHwhen PATH is an existing local directoryTests
uv run pytest tests/test_cli_plugin.py -vuv run ruff check astrbot/cli/commands/cmd_plug.py astrbot/cli/utils/plugin.py astrbot/cli/utils/__init__.py tests/test_cli_plugin.pyNote:
uv run pytest tests/test_code_quality_typing.py -vwas not run because the file does not exist on this local master-based branch.Summary by Sourcery
Add support for installing plugins from local directories via the CLI and cover the new behavior with tests.
New Features:
Tests: