Open
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
dashboard/routes/skills.pythe route uses the private methodSkillManager._get_plugin_skill_dir; consider exposing this as a public helper onSkillManager(or reusing the existingis_plugin_skill/ a newget_plugin_skill_dir) instead of reaching into a private implementation detail from the route layer. - Several handlers in
dashboard/routes/skills.pyrepeatedly constructSkillManager()and callis_plugin_skill(name)within the same request path (e.g.,list_skill_files,get_skill_file,update_skill_file); to avoid repeated filesystem scans, consider creating a singleSkillManagerinstance per request and reusing its plugin-skill checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `dashboard/routes/skills.py` the route uses the private method `SkillManager._get_plugin_skill_dir`; consider exposing this as a public helper on `SkillManager` (or reusing the existing `is_plugin_skill` / a new `get_plugin_skill_dir`) instead of reaching into a private implementation detail from the route layer.
- Several handlers in `dashboard/routes/skills.py` repeatedly construct `SkillManager()` and call `is_plugin_skill(name)` within the same request path (e.g., `list_skill_files`, `get_skill_file`, `update_skill_file`); to avoid repeated filesystem scans, consider creating a single `SkillManager` instance per request and reusing its plugin-skill checks.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/routes/skills.py" line_range="115-116" />
<code_context>
"Sandbox preset skill cannot be opened from local skill files."
)
+ plugin_skill_dir = skill_mgr._get_plugin_skill_dir(skill_name)
+ if plugin_skill_dir is not None:
+ return plugin_skill_dir.resolve(strict=True)
+
</code_context>
<issue_to_address>
**suggestion:** Avoid depending on SkillManager's private `_get_plugin_skill_dir` from the HTTP route.
The route now calls `skill_mgr._get_plugin_skill_dir(...)`, which is a private helper on `SkillManager`, so the route becomes fragile to internal refactors. Since you already added `is_plugin_skill`, consider either:
- Adding a public `get_plugin_skill_dir` on `SkillManager`, or
- Extending `SkillInfo`/`list_skills` to include the resolved plugin path and deriving the directory from there.
This keeps the boundary between the manager and the dashboard code stable.
Suggested implementation:
```python
# Prefer the public SkillManager API to avoid coupling to private helpers
plugin_skill_dir = skill_mgr.get_plugin_skill_dir(skill_name)
if plugin_skill_dir is not None:
return plugin_skill_dir.resolve(strict=True)
```
To fully implement the suggestion and keep the route decoupled from SkillManager internals, you also need to:
1. In the `SkillManager` class (where `_get_plugin_skill_dir` is defined):
- Add a public wrapper method:
```python
def get_plugin_skill_dir(self, skill_name: str) -> Path | None:
"""Public API to resolve the directory for a plugin skill, if any."""
return self._get_plugin_skill_dir(skill_name)
```
- Ensure it’s imported/used consistently wherever plugin skill directories are needed instead of calling `_get_plugin_skill_dir` directly.
2. Optionally (if you choose the `SkillInfo` approach instead):
- Extend `SkillInfo` and `list_skills` to include the resolved plugin path (e.g. `plugin_path: Path | None`).
- Then, in `astrbot/dashboard/routes/skills.py`, derive `plugin_skill_dir` from the `SkillInfo` returned by `list_skills` instead of calling into `SkillManager` at all.
Either approach satisfies the review comment; the minimal change is (1) with the public wrapper.
</issue_to_address>
### Comment 2
<location path="tests/test_skill_metadata_enrichment.py" line_range="444-453" />
<code_context>
+def test_list_skills_includes_plugin_provided_skills(monkeypatch, tmp_path: Path):
</code_context>
<issue_to_address>
**suggestion (testing):** Add more coverage for different plugin skill layouts and name conflicts
This already covers the nested plugin layout and the key metadata fields. Please also add: (1) a case where a plugin exposes a skill directly as `plugins/<plugin>/skills/SKILL.md` (skill name == plugin name); and (2) a conflict case where a local skill and a plugin skill share the same name, asserting the plugin skill is ignored. This will cover all branches of `_iter_plugin_skill_dirs` and the conflict resolution in `list_skills`.
Suggested implementation:
```python
def test_list_skills_includes_plugin_provided_skills(monkeypatch, tmp_path: Path):
"""
Ensure `list_skills`:
* Includes plugin-provided skills from nested layouts
* Includes plugin-provided skills when exposed directly as plugins/<plugin>/skills/SKILL.md
* Prefers local skills when there is a name conflict with a plugin skill
"""
from astrbot.core.skills.skill_manager import list_skills
# Base paths
data_dir = tmp_path / "data"
skills_root = tmp_path / "skills"
plugins_root = tmp_path / "plugins"
data_dir.mkdir(parents=True, exist_ok=True)
skills_root.mkdir(parents=True, exist_ok=True)
plugins_root.mkdir(parents=True, exist_ok=True)
# Point the skill manager at our temporary data directory
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_data_path",
lambda: str(data_dir),
)
# In case the implementation also uses a separate skills root helper,
# prefer our explicitly created `skills_root` for local skills.
try:
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_skills_path",
lambda: str(skills_root),
)
except AttributeError:
# Older versions may not expose this; `list_skills` might take a root arg instead.
pass
# ------------------------------------------------------------------
# Local skills (for baseline and for conflict with plugin skills)
# ------------------------------------------------------------------
local_echo = skills_root / "echo"
local_echo.mkdir(parents=True, exist_ok=True)
(local_echo / "SKILL.md").write_text("# Echo\n\nLocal echo skill.", encoding="utf-8")
# Local skill that will conflict with a plugin skill of the same name
local_conflict = skills_root / "conflict"
local_conflict.mkdir(parents=True, exist_ok=True)
(local_conflict / "SKILL.md").write_text(
"# Conflict\n\nLocal conflict skill.", encoding="utf-8"
)
# ------------------------------------------------------------------
# Plugin skill: nested layout
# data/plugins/nested-plugin/skills/nested-skill/SKILL.md
# ------------------------------------------------------------------
nested_plugin_root = plugins_root / "nested-plugin" / "skills" / "nested-skill"
nested_plugin_root.mkdir(parents=True, exist_ok=True)
(nested_plugin_root / "SKILL.md").write_text(
"# Nested Skill\n\nPlugin nested skill.", encoding="utf-8"
)
# ------------------------------------------------------------------
# Plugin skill: direct layout with SKILL.md directly under skills/
# data/plugins/direct-plugin/skills/SKILL.md
# (skill name is the plugin name)
# ------------------------------------------------------------------
direct_plugin_skills_dir = plugins_root / "direct-plugin" / "skills"
direct_plugin_skills_dir.mkdir(parents=True, exist_ok=True)
(direct_plugin_skills_dir / "SKILL.md").write_text(
"# Direct Plugin\n\nSkill exposed as SKILL.md directly under skills.",
encoding="utf-8",
)
# ------------------------------------------------------------------
# Plugin skill that conflicts with a local skill:
# local: skills/conflict/SKILL.md
# plugin: plugins/conflict-plugin/skills/conflict/SKILL.md
# list_skills should prefer the local skill and ignore the plugin skill.
# ------------------------------------------------------------------
conflict_plugin_root = plugins_root / "conflict-plugin" / "skills" / "conflict"
conflict_plugin_root.mkdir(parents=True, exist_ok=True)
(conflict_plugin_root / "SKILL.md").write_text(
"# Conflict (Plugin)\n\nShould be ignored in favor of local.",
encoding="utf-8",
)
# ------------------------------------------------------------------
# Execute
# ------------------------------------------------------------------
try:
# Newer signature that accepts an explicit skills root
skills = list_skills(str(skills_root))
except TypeError:
# Older signature that discovers the skills root via get_astrbot_data_path
skills = list_skills()
# Normalize to a name -> skill mapping to make assertions easier.
# We assume each skill object has a `.name` attribute; if the implementation
# uses a different attribute, adjust this mapping accordingly.
skills_by_name = {getattr(s, "name", None): s for s in skills}
# ------------------------------------------------------------------
# Assertions
# ------------------------------------------------------------------
# 1) Existing coverage: nested plugin layout is discovered
assert "nested-skill" in skills_by_name, "nested plugin skill should be listed"
# 2) Direct `plugins/<plugin>/skills/SKILL.md` layout is discovered.
# Most implementations will derive the skill name from the plugin name.
assert "direct-plugin" in skills_by_name, (
"plugin exposing SKILL.md directly under skills/ must be listed "
"with the plugin name as the skill name"
)
# 3) Conflict resolution: local skill must win over plugin skill
# We only assert that the name exists and comes from the local source.
# If the SkillMetadata exposes an origin field, assert on that as well.
assert "conflict" in skills_by_name, "conflicting skill name must be present"
conflict_skill = skills_by_name["conflict"]
# If the implementation tracks origin or plugin metadata, prefer the local one.
# Adjust these attribute checks to the actual metadata fields available.
if hasattr(conflict_skill, "plugin"):
# Local skills should not be associated with a plugin
assert conflict_skill.plugin in (None, "", False)
if hasattr(conflict_skill, "is_plugin"):
assert not conflict_skill.is_plugin
if hasattr(conflict_skill, "origin"):
assert conflict_skill.origin == "local"
```
The exact assertions may need small adjustments to match your existing `SkillMetadata`/`list_skills` API:
1. If the skill objects do not have a `.name` attribute, replace `getattr(s, "name", None)` with the correct attribute used for the skill name.
2. If `list_skills` does not accept a root path argument (or uses a different parameter), keep only the correct call signature and remove the `try/except TypeError` block.
3. If your implementation exposes origin information differently (e.g. `s.source`, `s.from_plugin`, or `s.plugin_name`), update the conflict assertions to check that the returned `conflict` skill is the local one (e.g. origin is `"local"` or no plugin is associated).
4. If your plugin discovery uses a different base path (e.g. `data_dir / "plugins"` vs a config), adjust `plugins_root` construction so the directories we create match `_iter_plugin_skill_dirs`’ expectations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+115
to
+116
| plugin_skill_dir = skill_mgr._get_plugin_skill_dir(skill_name) | ||
| if plugin_skill_dir is not None: |
Contributor
There was a problem hiding this comment.
suggestion: Avoid depending on SkillManager's private _get_plugin_skill_dir from the HTTP route.
The route now calls skill_mgr._get_plugin_skill_dir(...), which is a private helper on SkillManager, so the route becomes fragile to internal refactors. Since you already added is_plugin_skill, consider either:
- Adding a public
get_plugin_skill_dironSkillManager, or - Extending
SkillInfo/list_skillsto include the resolved plugin path and deriving the directory from there.
This keeps the boundary between the manager and the dashboard code stable.
Suggested implementation:
# Prefer the public SkillManager API to avoid coupling to private helpers
plugin_skill_dir = skill_mgr.get_plugin_skill_dir(skill_name)
if plugin_skill_dir is not None:
return plugin_skill_dir.resolve(strict=True)To fully implement the suggestion and keep the route decoupled from SkillManager internals, you also need to:
-
In the
SkillManagerclass (where_get_plugin_skill_diris defined):- Add a public wrapper method:
def get_plugin_skill_dir(self, skill_name: str) -> Path | None: """Public API to resolve the directory for a plugin skill, if any.""" return self._get_plugin_skill_dir(skill_name)
- Ensure it’s imported/used consistently wherever plugin skill directories are needed instead of calling
_get_plugin_skill_dirdirectly.
- Add a public wrapper method:
-
Optionally (if you choose the
SkillInfoapproach instead):- Extend
SkillInfoandlist_skillsto include the resolved plugin path (e.g.plugin_path: Path | None). - Then, in
astrbot/dashboard/routes/skills.py, deriveplugin_skill_dirfrom theSkillInforeturned bylist_skillsinstead of calling intoSkillManagerat all.
- Extend
Either approach satisfies the review comment; the minimal change is (1) with the public wrapper.
Comment on lines
+444
to
+453
| def test_list_skills_includes_plugin_provided_skills(monkeypatch, tmp_path: Path): | ||
| data_dir = tmp_path / "data" | ||
| skills_root = tmp_path / "skills" | ||
| plugins_root = tmp_path / "plugins" | ||
| data_dir.mkdir(parents=True, exist_ok=True) | ||
| skills_root.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_data_path", | ||
| lambda: str(data_dir), |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add more coverage for different plugin skill layouts and name conflicts
This already covers the nested plugin layout and the key metadata fields. Please also add: (1) a case where a plugin exposes a skill directly as plugins/<plugin>/skills/SKILL.md (skill name == plugin name); and (2) a conflict case where a local skill and a plugin skill share the same name, asserting the plugin skill is ignored. This will cover all branches of _iter_plugin_skill_dirs and the conflict resolution in list_skills.
Suggested implementation:
def test_list_skills_includes_plugin_provided_skills(monkeypatch, tmp_path: Path):
"""
Ensure `list_skills`:
* Includes plugin-provided skills from nested layouts
* Includes plugin-provided skills when exposed directly as plugins/<plugin>/skills/SKILL.md
* Prefers local skills when there is a name conflict with a plugin skill
"""
from astrbot.core.skills.skill_manager import list_skills
# Base paths
data_dir = tmp_path / "data"
skills_root = tmp_path / "skills"
plugins_root = tmp_path / "plugins"
data_dir.mkdir(parents=True, exist_ok=True)
skills_root.mkdir(parents=True, exist_ok=True)
plugins_root.mkdir(parents=True, exist_ok=True)
# Point the skill manager at our temporary data directory
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_data_path",
lambda: str(data_dir),
)
# In case the implementation also uses a separate skills root helper,
# prefer our explicitly created `skills_root` for local skills.
try:
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_skills_path",
lambda: str(skills_root),
)
except AttributeError:
# Older versions may not expose this; `list_skills` might take a root arg instead.
pass
# ------------------------------------------------------------------
# Local skills (for baseline and for conflict with plugin skills)
# ------------------------------------------------------------------
local_echo = skills_root / "echo"
local_echo.mkdir(parents=True, exist_ok=True)
(local_echo / "SKILL.md").write_text("# Echo\n\nLocal echo skill.", encoding="utf-8")
# Local skill that will conflict with a plugin skill of the same name
local_conflict = skills_root / "conflict"
local_conflict.mkdir(parents=True, exist_ok=True)
(local_conflict / "SKILL.md").write_text(
"# Conflict\n\nLocal conflict skill.", encoding="utf-8"
)
# ------------------------------------------------------------------
# Plugin skill: nested layout
# data/plugins/nested-plugin/skills/nested-skill/SKILL.md
# ------------------------------------------------------------------
nested_plugin_root = plugins_root / "nested-plugin" / "skills" / "nested-skill"
nested_plugin_root.mkdir(parents=True, exist_ok=True)
(nested_plugin_root / "SKILL.md").write_text(
"# Nested Skill\n\nPlugin nested skill.", encoding="utf-8"
)
# ------------------------------------------------------------------
# Plugin skill: direct layout with SKILL.md directly under skills/
# data/plugins/direct-plugin/skills/SKILL.md
# (skill name is the plugin name)
# ------------------------------------------------------------------
direct_plugin_skills_dir = plugins_root / "direct-plugin" / "skills"
direct_plugin_skills_dir.mkdir(parents=True, exist_ok=True)
(direct_plugin_skills_dir / "SKILL.md").write_text(
"# Direct Plugin\n\nSkill exposed as SKILL.md directly under skills.",
encoding="utf-8",
)
# ------------------------------------------------------------------
# Plugin skill that conflicts with a local skill:
# local: skills/conflict/SKILL.md
# plugin: plugins/conflict-plugin/skills/conflict/SKILL.md
# list_skills should prefer the local skill and ignore the plugin skill.
# ------------------------------------------------------------------
conflict_plugin_root = plugins_root / "conflict-plugin" / "skills" / "conflict"
conflict_plugin_root.mkdir(parents=True, exist_ok=True)
(conflict_plugin_root / "SKILL.md").write_text(
"# Conflict (Plugin)\n\nShould be ignored in favor of local.",
encoding="utf-8",
)
# ------------------------------------------------------------------
# Execute
# ------------------------------------------------------------------
try:
# Newer signature that accepts an explicit skills root
skills = list_skills(str(skills_root))
except TypeError:
# Older signature that discovers the skills root via get_astrbot_data_path
skills = list_skills()
# Normalize to a name -> skill mapping to make assertions easier.
# We assume each skill object has a `.name` attribute; if the implementation
# uses a different attribute, adjust this mapping accordingly.
skills_by_name = {getattr(s, "name", None): s for s in skills}
# ------------------------------------------------------------------
# Assertions
# ------------------------------------------------------------------
# 1) Existing coverage: nested plugin layout is discovered
assert "nested-skill" in skills_by_name, "nested plugin skill should be listed"
# 2) Direct `plugins/<plugin>/skills/SKILL.md` layout is discovered.
# Most implementations will derive the skill name from the plugin name.
assert "direct-plugin" in skills_by_name, (
"plugin exposing SKILL.md directly under skills/ must be listed "
"with the plugin name as the skill name"
)
# 3) Conflict resolution: local skill must win over plugin skill
# We only assert that the name exists and comes from the local source.
# If the SkillMetadata exposes an origin field, assert on that as well.
assert "conflict" in skills_by_name, "conflicting skill name must be present"
conflict_skill = skills_by_name["conflict"]
# If the implementation tracks origin or plugin metadata, prefer the local one.
# Adjust these attribute checks to the actual metadata fields available.
if hasattr(conflict_skill, "plugin"):
# Local skills should not be associated with a plugin
assert conflict_skill.plugin in (None, "", False)
if hasattr(conflict_skill, "is_plugin"):
assert not conflict_skill.is_plugin
if hasattr(conflict_skill, "origin"):
assert conflict_skill.origin == "local"The exact assertions may need small adjustments to match your existing SkillMetadata/list_skills API:
- If the skill objects do not have a
.nameattribute, replacegetattr(s, "name", None)with the correct attribute used for the skill name. - If
list_skillsdoes not accept a root path argument (or uses a different parameter), keep only the correct call signature and remove thetry/except TypeErrorblock. - If your implementation exposes origin information differently (e.g.
s.source,s.from_plugin, ors.plugin_name), update the conflict assertions to check that the returnedconflictskill is the local one (e.g. origin is"local"or no plugin is associated). - If your plugin discovery uses a different base path (e.g.
data_dir / "plugins"vs a config), adjustplugins_rootconstruction so the directories we create match_iter_plugin_skill_dirs’ expectations.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enables plugins to bundle skills, which are automatically discovered and integrated into the SkillManager as read-only resources. The changes include logic for synchronizing these skills to active sandboxes, dashboard UI updates to display plugin sources, and safeguards to prevent the deletion or modification of plugin-provided skills. Feedback highlights a performance improvement opportunity by using the zipfile module for bundling instead of shutil.copytree to avoid redundant disk I/O and potential event loop blocking. Additionally, a regex inconsistency between the core and dashboard was identified that could cause issues with non-ASCII skill names.
| shutil.rmtree(bundle_root) | ||
| bundle_root.mkdir(parents=True) | ||
| for skill_name, skill_dir in sync_skill_dirs: | ||
| shutil.copytree(skill_dir, bundle_root / skill_name) |
Contributor
There was a problem hiding this comment.
Using shutil.copytree to create an intermediate directory structure before zipping is inefficient as it performs redundant disk I/O and increases temporary storage requirements. For a large number of skills or large assets, this can be slow and block the event loop. Consider using the zipfile module to add files directly to the archive from their original locations. Additionally, ensure this new functionality is accompanied by corresponding unit tests.
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
| skills_dir, | ||
| rename_legacy=False, | ||
| ) | ||
| if direct_skill_md is not None and _SKILL_NAME_RE.match(plugin_name): |
Contributor
There was a problem hiding this comment.
The _SKILL_NAME_RE used here (^[\w.-]+$) allows Unicode characters by default in Python 3. However, the corresponding regex in astrbot/dashboard/routes/skills.py (^[A-Za-z0-9._-]+$) is restricted to ASCII. This inconsistency will cause plugin skills with non-ASCII names to be correctly loaded by the core but rejected by the dashboard API. To ensure consistent behavior and avoid code duplication, refactor the logic into a shared helper function or constant.
References
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
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
1 participant
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.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add support for skills bundled inside plugins and surface them as read-only, plugin-sourced skills throughout the skill management and sandbox sync flows.
New Features:
Enhancements:
Documentation:
Tests: