feat: supports plugin to add skills

[Soulter]

Modifications / 改动点

• This is NOT a breaking change. / 这不是一个破坏性变更。

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.txt and pyproject.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:

• Discover and register skills provided under a plugin's skills directory, including support for a single-skill layout and attribution to the originating plugin.
• Expose plugin-provided skills in the skill manager and WebUI, showing their plugin source and treating them as read-only while still allowing enable/disable toggling.
• Include plugin-provided skills when syncing skills to sandboxes so that bundled skills are available in sandboxed environments.

Enhancements:

• Extend skill metadata to track plugin origin and readonly status for better source-aware behavior.
• Update dashboard skill file APIs and UI controls to prevent editing, downloading, or deleting skills that come from plugins or sandbox presets, with appropriate messaging.
• Trigger sandbox skill resync after plugin install, update, reload, or uninstall so bundled skills stay in sync with active sandboxes.

Documentation:

• Document how to bundle skills with a plugin in both English and Chinese developer guides, including directory structure and read-only behavior of plugin-provided skills.

Tests:

• Add tests to verify plugin-provided skills are discovered by the skill manager and included in sandbox sync payloads.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• 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.

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>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

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:

        # 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:

     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.

[sourcery-ai]

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:

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.

[gemini-code-assist]

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.

[gemini-code-assist]

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

1. New functionality, such as handling attachments, should be accompanied by corresponding unit tests.

[gemini-code-assist]

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

1. 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.