feat: 让 LLM 能自主创建和安装 Skill

[Blueteemo]

关联 #7657

改动

• 新增 skill_tools.py：CreateSkillZipTool 和 InstallSkillFromZipTool 两个 FunctionTool
• 更新 __init__.py：导出新工具
• 更新 build_skills_prompt：加了创建 Skill 的规则说明

背景

AstrBot 的 Neo Skills 工具链只在 shipyard_neo 沙箱模式可用，local 模式下 LLM 无法自主创建 Skill。现有 SkillManager.install_skill_from_zip() 方法存在但未暴露为 FunctionTool。

工作流

LLM 用 astrbot_file_write_tool 写 SKILL.md → create_skill_zip 打包 → install_skill_from_zip 安装。或者直接写文件到 data/skills/{name}/ 即可自动发现。

Summary by Sourcery

Enable LLMs to self-author and manage skills in local runtime by exposing skill packaging and installation tools and updating skill usage guidance.

New Features:

• Expose CreateSkillZipTool and InstallSkillFromZipTool as function tools for packaging and installing skills from ZIP archives in local or sandbox computer runtimes.

Enhancements:

• Extend the skills prompt instructions to describe how LLMs can create, package, and install new skills using SKILL.md files and the new tools.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In CreateSkillZipTool.call, arcname = skill_name / file_path.relative_to(skill_dir) will fail because skill_name is a string; consider using Path(skill_name) / ... or building the archive name as a string to avoid a type error.
• Both tools duplicate logic for resolving temp paths differently in local vs non-local runtimes; consider extracting a small helper for resolving temp file locations so future changes to temp directory handling stay consistent in one place.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `CreateSkillZipTool.call`, `arcname = skill_name / file_path.relative_to(skill_dir)` will fail because `skill_name` is a string; consider using `Path(skill_name) / ...` or building the archive name as a string to avoid a type error.
- Both tools duplicate logic for resolving temp paths differently in local vs non-local runtimes; consider extracting a small helper for resolving temp file locations so future changes to temp directory handling stay consistent in one place.

## Individual Comments

### Comment 1
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="123-124" />
<code_context>
+            with zipfile.ZipFile(str(zip_path), "w", zipfile.ZIP_DEFLATED) as zf:
+                for root, _dirs, files in os.walk(skill_dir):
+                    for file in files:
+                        file_path = Path(root) / file
+                        arcname = skill_name / file_path.relative_to(skill_dir)
+                        zf.write(str(file_path), str(arcname))
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix path joining when building `arcname` to avoid using `/` with a string.

`skill_name` is a `str`, so `skill_name / file_path.relative_to(skill_dir)` will raise a `TypeError` because `/` is only defined for `Path` objects. You can either convert `skill_name` to a `Path`:

```python
skill_root = Path(skill_name)
arcname = skill_root / file_path.relative_to(skill_dir)
```

or use `os.path.join` and keep everything as strings:

```python
arcname = os.path.join(skill_name, str(file_path.relative_to(skill_dir)))
```

and then pass `str(arcname)` to `zf.write` so the zip is created correctly.
</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.}

[gemini-code-assist]

Code Review

This pull request introduces functionality for the LLM to self-author skills in a local runtime environment. It updates the skill manager's prompt to guide the LLM through the creation process and adds two new tools: CreateSkillZipTool for packaging skill directories into ZIP archives and InstallSkillFromZipTool for registering skills from those archives. Review feedback identified a critical TypeError in the path joining logic within the packaging tool and suggested ensuring parent directories exist before file creation to prevent potential errors.

[Blueteemo]

@sourcery-ai review

[sourcery-ai]

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

• Both tools import and depend on the private helper _normalize_skill_markdown_path; consider exposing a public utility in skill_manager (or reusing an existing public API) to avoid coupling to a private implementation detail that may change.
• The call methods catch a broad Exception and return the stringified error to the caller; you may want to narrow the exception handling and/or log the underlying error separately while returning a more structured ToolExecResult error to keep the tool protocol stable and avoid leaking internal details.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Both tools import and depend on the private helper `_normalize_skill_markdown_path`; consider exposing a public utility in `skill_manager` (or reusing an existing public API) to avoid coupling to a private implementation detail that may change.
- The `call` methods catch a broad `Exception` and return the stringified error to the caller; you may want to narrow the exception handling and/or log the underlying error separately while returning a more structured `ToolExecResult` error to keep the tool protocol stable and avoid leaking internal details.

## Individual Comments

### Comment 1
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="40-46" />
<code_context>
+_SKILL_NAME_RE = re.compile(r"^[\w.\-]+$")
+
+
+def _resolve_temp_path(local_env: bool, filename: str) -> Path:
+    """Return temp directory path, consistent across local/sandbox runtimes."""
+    if local_env:
+        from astrbot.core.utils.astrbot_path import get_astrbot_temp_path
+
+        return Path(get_astrbot_temp_path()) / filename
+    return Path(f"/tmp/{filename}")
+
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Sanitize `filename` in `_resolve_temp_path` to avoid directory traversal outside the temp directory.

Because `filename` can come from user/LLM inputs (e.g., `zip_path` in `InstallSkillFromZipTool` and the skill name in `CreateSkillZipTool`), values like `"../foo"` or paths with separators can escape the temp root and access arbitrary filesystem locations. Please normalize the input, e.g. by using `Path(filename).name` when only a bare file name is expected, or by resolving the combined path and asserting it remains within the base temp directory (e.g., `resolved.is_relative_to(base)` or an equivalent manual check). This ensures all operations are confined to the temp directory, even in local mode.
</issue_to_address>

### Comment 2
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="134-143" />
<code_context>
+
+            return f"Successfully installed skill(s): {installed}"
+
+        except Exception as e:
+            return f"Error installing skill from zip: {e}"
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Avoid returning raw exception messages directly to the tool caller to limit information leakage.

Here you return `str(e)` directly. The underlying exception may expose internal paths or implementation details to the caller. Consider logging the full exception on the server and returning a generic error message (optionally with a coarse category like validation vs I/O and a short, pre-vetted reason).
</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.}

[Blueteemo]

@sourcery-ai review

[sourcery-ai]

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

• CreateSkillZipTool currently imports and relies on the private helper _normalize_skill_markdown_path; consider exposing a public helper in skill_manager (or in a shared utils module) instead of depending on a private implementation detail.
• InstallSkillFromZipTool accepts absolute zip_path values and only constrains relative paths via _resolve_temp_path; if you want to keep tool usage confined to the intended temp directory, consider rejecting absolute paths or routing all input through _resolve_temp_path for consistent behavior across runtimes.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- CreateSkillZipTool currently imports and relies on the private helper `_normalize_skill_markdown_path`; consider exposing a public helper in `skill_manager` (or in a shared utils module) instead of depending on a private implementation detail.
- InstallSkillFromZipTool accepts absolute `zip_path` values and only constrains relative paths via `_resolve_temp_path`; if you want to keep tool usage confined to the intended temp directory, consider rejecting absolute paths or routing all input through `_resolve_temp_path` for consistent behavior across runtimes.

## Individual Comments

### Comment 1
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="43-51" />
<code_context>
+_SKILL_NAME_RE = re.compile(r"^[\w.\-]+$")
+
+
+def _resolve_temp_path(local_env: bool, filename: str) -> Path:
+    """Return temp directory path, consistent across local/sandbox runtimes.
+
+    Raises ValueError if *filename* would escape the temp directory
+    (e.g. contains ``..`` components).
+    """
+    # Reject directory-traversal attempts
+    clean = Path(filename)
+    if clean.is_absolute() or ".." in clean.parts:
+        raise ValueError(f"Invalid filename: {filename!r}")
+
</code_context>
<issue_to_address>
**suggestion:** Consider surfacing a clearer error when `_resolve_temp_path` rejects a filename instead of falling through to the generic exception handler.

Because `_resolve_temp_path` raises `ValueError` for invalid user input (e.g., `..` segments or absolute paths), that exception currently gets turned into a generic "Error creating/installing skill" by the broad `except Exception` handlers. Consider catching `ValueError` explicitly at the call sites and returning a specific message (e.g., "Invalid relative path") for these validation failures, keeping the generic message only for unexpected exceptions.

Suggested implementation:

```python
    except ValueError as exc:
        # Validation error from _resolve_temp_path (e.g., absolute path or ".." segments)
        logger.warning("Invalid relative path when creating skill zip: %s", exc)
        return ToolResult(error="Invalid relative path; paths must be relative to the skill workspace")
    except Exception as exc:
        logger.exception("Error creating skill zip")
        return ToolResult(error="Error creating skill zip")

```

```python
    except ValueError as exc:
        # Validation error from _resolve_temp_path (e.g., absolute path or ".." segments)
        logger.warning("Invalid relative path when installing skill from zip: %s", exc)
        return ToolResult(error="Invalid relative path; paths must be relative to the skill workspace")
    except Exception as exc:
        logger.exception("Error installing skill from zip")
        return ToolResult(error="Error installing skill from zip")

```

I assumed there are two broad `except Exception` handlers whose messages are `"Error creating skill zip"` and `"Error installing skill from zip"`, and that both wrap calls to `_resolve_temp_path`. If the log/error strings differ, or there are additional call sites for `_resolve_temp_path`, mirror this pattern there as well: add a leading `except ValueError` that logs at `warning` level and returns a specific, user-facing message, followed by the existing `except Exception` block unchanged. Also, if your `ToolResult` (or equivalent return type) uses a different field name or structure for user-visible messages, adapt the `error=` usage accordingly.
</issue_to_address>

### Comment 2
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="63-72" />
<code_context>
+class CreateSkillZipTool(FunctionTool):
</code_context>
<issue_to_address>
**suggestion:** It may be safer to explicitly disallow non-directory targets (e.g., if `skill_dir` is accidentally a file) before walking and zipping.

Right now you only check `skill_dir.exists()`. If `data/skills/<skill_name>` exists as a regular file, `os.walk(skill_dir)` will still run and can behave unexpectedly. Consider also checking `skill_dir.is_dir()` and failing with a clear error when it isn’t a directory.

Suggested implementation:

```python
    The skill directory must already exist under ``data/skills/<skill_name>/``
    and contain at least a ``SKILL.md`` file.  The resulting ZIP is written
    to the temp directory and the path is returned so that
    ``install_skill_from_zip`` can consume it.
    """

    name: str = "astrbot_create_skill_zip"
    description: str = (

```

```python
        if not skill_dir.exists():
            raise ValueError(f"Skill directory does not exist: {skill_dir}")
        if not skill_dir.is_dir():
            raise ValueError(
                f"Expected a skill directory at {skill_dir}, but found a non-directory file"
            )

```

I assumed the existence check currently looks like `if not skill_dir.exists():` inside the `CreateSkillZipTool` implementation (likely in a `__call__` or `run` method). If the exact code differs (e.g., different variable name, different error type, or combined with other conditions), you should:
1. Locate the place where `skill_dir` is validated before `os.walk(skill_dir)` or any zipping logic.
2. After the existing `exists()` check, add a separate `if not skill_dir.is_dir():` guard that raises a clear error (e.g., `ValueError` or your project-specific exception) stating that a directory was expected but a non-directory was found.
3. Ensure that the `is_dir()` check happens before any call to `os.walk(skill_dir)` or similar filesystem traversal to prevent unexpected behavior when `skill_dir` is a regular file or symlink.
</issue_to_address>

### Comment 3
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="153-162" />
<code_context>
+class InstallSkillFromZipTool(FunctionTool):
</code_context>
<issue_to_address>
**🚨 question (security):** Allowing arbitrary absolute `zip_path` values bypasses your temp-path safety checks and might be worth tightening or documenting.

Relative `zip_path`s go through `_resolve_temp_path`, which constrains them to the temp dir and blocks `..` traversal. Absolute paths bypass this and can point anywhere the process can access. If the goal is to keep this tool scoped to temp-only files, consider rejecting absolute paths or gating them behind an explicit opt-in. If absolute paths are required, it would help to call this out in the docstring and/or enforce that the path resides under a configured skills/temp root to limit damage from misconfiguration.
</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.}

[Blueteemo]

@sourcery-ai review

[sourcery-ai]

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

• The absolute-path allowlist in InstallSkillFromZipTool looks inconsistent with _resolve_temp_path: _resolve_temp_path uses /tmp for non-local runtimes, but InstallSkillFromZipTool only adds /tmp to allowed_roots when local_env is true; consider aligning these conditions so the security checks and path resolution behave consistently across local vs sandbox environments.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The absolute-path allowlist in `InstallSkillFromZipTool` looks inconsistent with `_resolve_temp_path`: `_resolve_temp_path` uses `/tmp` for non-local runtimes, but `InstallSkillFromZipTool` only adds `/tmp` to `allowed_roots` when `local_env` is true; consider aligning these conditions so the security checks and path resolution behave consistently across local vs sandbox environments.

## Individual Comments

### Comment 1
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="54-63" />
<code_context>
+                    Path(get_astrbot_temp_path()),
+                    Path(get_astrbot_skills_path()),
+                ]
+                if local_env:
+                    allowed_roots.append(Path("/tmp"))
+                if not any(_is_within(resolved, root) for root in allowed_roots):
+                    return (
</code_context>
<issue_to_address>
**issue (bug_risk):** The /tmp allowlist logic seems inverted relative to local vs sandbox runtimes.

In `InstallSkillFromZipTool.call`, `/tmp` is added to `allowed_roots` only when `local_env` is `True`, but `_resolve_temp_path` uses `/tmp` as the temp dir in sandbox (non-local) mode. This means absolute `/tmp/...` paths are permitted locally but rejected in sandbox, while sandbox relative paths still resolve under `/tmp`. This inversion can lead to confusing, inconsistent behavior. Please adjust the condition (e.g., `if not local_env:`) or otherwise align the allowlist with the environments that should trust `/tmp` absolute paths.
</issue_to_address>

### Comment 2
<location path="astrbot/core/tools/computer_tools/skill_tools.py" line_range="90-99" />
<code_context>
+                        "the temp directory."
+                    ),
+                },
+                "skill_name": {
+                    "type": "string",
+                    "description": (
+                        "Optional name override for the installed skill. "
+                        "If omitted, the name is derived from the zip contents."
+                    ),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider validating the optional skill_name override similarly to skill creation.

`CreateSkillZipTool` validates `skill_name` with `_SKILL_NAME_RE`, but `InstallSkillFromZipTool` forwards the optional override directly to `SkillManager.install_skill_from_zip`. If `SkillManager` expects the same naming constraints, validate the override with `_SKILL_NAME_RE` here as well and surface a clear error for invalid values.
</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.}