fix: allow multiple skills in a single zip archive

[jmt059]

Motivation / 动机

The Web UI explicitly states that users can import zip archives containing multiple skills ("支持压缩包内含多个 skills 文件夹"). However, the current backend logic in install_skill_from_zip within skill_manager.py enforces a strict check: if len(top_dirs) != 1:. This raises a Zip archive must contain a single top-level folder. error if users upload multiple skills in parallel. Furthermore, wrapping them in a root folder bypasses this check but fails with SKILL.md not found, as the code does not iterate through subdirectories. This PR fixes the discrepancy between the frontend prompt and the backend logic.

Modifications / 改动点

• Removed the strict single-folder validation (len(top_dirs) != 1) in astrbot/core/skills/skill_manager.py.

• Refactored install_skill_from_zip to iterate through all top-level directories in the archive.

• It now correctly extracts, identifies, and registers each valid skill folder containing a SKILL.md (or skill.md), aligning backend behavior with frontend documentation while strictly preserving existing Zip Slip security checks.

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

Screenshots or Test Results / 运行截图或测试结果

Locally tested with a zip file containing 12 individual skill folders.
Result: All 12 skills were successfully parsed, moved to the data/skills/ directory, and registered as active in the system without triggering the previous ValueError exceptions.

---

Checklist / 检查清单

• 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  / 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。(N/A - This is a bug fix)

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

Allow installing multiple skills from a single zip archive while preserving existing validation and security checks.

Bug Fixes:

• Fix skill installation failures when uploading zip archives containing multiple top-level skill folders.
• Ensure root-mode zip installations correctly validate the presence of SKILL.md at the archive root and use a validated skill name.

Enhancements:

• Support installing and activating all valid skill folders within a zip archive in one operation, skipping invalid or malformed entries and collecting their names in the return value.
• Improve error reporting by raising a clear error when no valid SKILL.md is found in any folder and by including the skill name in file-exists errors.

[sourcery-ai]

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

• The return value of install_skill_from_zip has changed from a single skill name to a comma-separated string of names, which is a behavioral/API change; consider returning the previous type (e.g., the first skill or a list) or adding a new helper to keep the original contract intact.
• In the multi-folder branch, archive_skill_name is only honored when there is exactly one top-level directory; if this is intentional, it might be clearer to either reject archive_skill_name when multiple skills are present or explicitly document/log that it will be ignored in that case.
• The new logic silently skips invalid or SKILL.md-less top-level folders and only fails if none are valid; if some invalid folders should still be surfaced to the user, consider accumulating and reporting which entries were skipped and why instead of fully swallowing them.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The return value of `install_skill_from_zip` has changed from a single skill name to a comma-separated string of names, which is a behavioral/API change; consider returning the previous type (e.g., the first skill or a list) or adding a new helper to keep the original contract intact.
- In the multi-folder branch, `archive_skill_name` is only honored when there is exactly one top-level directory; if this is intentional, it might be clearer to either reject `archive_skill_name` when multiple skills are present or explicitly document/log that it will be ignored in that case.
- The new logic silently skips invalid or SKILL.md-less top-level folders and only fails if none are valid; if some invalid folders should still be surfaced to the user, consider accumulating and reporting which entries were skipped and why instead of fully swallowing them.

## Individual Comments

### Comment 1
<location path="astrbot/core/skills/skill_manager.py" line_range="551-555" />
<code_context>
         if not zipfile.is_zipfile(zip_path):
             raise ValueError("Uploaded file is not a valid zip archive.")

+        installed_skills = []
+
         with zipfile.ZipFile(zip_path) as zf:
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning a comma-separated string of skill names changes the function’s API and may break callers expecting a single skill name.

This also changes the return type from a single `skill_name` string to a comma-joined aggregation, which can silently break any code that treats it as a unique identifier or key. Comma-delimiting is fragile (e.g., skill names containing commas become ambiguous). If multiple installs are needed, prefer returning a structured type such as `List[str]`, and consider a backward‑compatible path (e.g., keeping the old single-skill API or adding a separate multi-install function).
</issue_to_address>

### Comment 2
<location path="astrbot/core/skills/skill_manager.py" line_range="635" />
<code_context>
+                        ):
+                            continue
+                            
+                        skill_name = archive_skill_name if archive_skill_name and len(top_dirs) == 1 else archive_root_name_normalized
+                        
+                        src_dir = Path(tmp_dir) / archive_root_name
</code_context>
<issue_to_address>
**issue (bug_risk):** The provided archive_skill_name is no longer validated for non-root installs and may silently be ignored or accept invalid names.

Previously, `archive_skill_name` for non-root installs was regex-validated (`_SKILL_NAME_RE.fullmatch(...)`) and rejected when invalid. Now:

- It’s used as `skill_name` when `len(top_dirs) == 1` with no validation.
- It’s ignored when `len(top_dirs) > 1`, changing earlier behavior where invalid values would raise.

Please restore validation for `archive_skill_name` in non-root mode (mirroring the root-mode branch) and clearly define when it takes precedence over the derived folder name to avoid silent ignores or invalid skill directories.
</issue_to_address>

### Comment 3
<location path="astrbot/core/skills/skill_manager.py" line_range="643-647" />
<code_context>
-                if not src_dir.exists():
-                    raise ValueError("Skill folder not found after extraction.")
-                dest_dir = Path(self.skills_root) / skill_name
-                if dest_dir.exists():
-                    if not overwrite:
-                        raise FileExistsError("Skill already exists.")
-                    shutil.rmtree(dest_dir)
-                shutil.move(str(src_dir), str(dest_dir))
-
-        self.set_skill_active(skill_name, True)
</code_context>
<issue_to_address>
**suggestion:** Partial-install behavior on FileExistsError can leave the system in a mixed state when multiple skills are in one archive.

In the multi-skill, non-root branch, skills are installed in a loop over `top_dirs`. With `overwrite=False`, if a later `dest_dir` already exists, `FileExistsError` is raised after earlier skills may have been installed and activated, without any record of which ones succeeded.

If multi-skill archives are supported, consider either:
- Pre-validating all `dest_dir` paths and failing before any changes, or
- Adding a rollback mechanism, or
- Explicitly documenting that partial installation is possible and acceptable for this API.

Suggested implementation:

```python
                # Pre-validate destination directories for multi-skill archives to avoid
                # partial installation when overwrite=False. We do this before any
                # filesystem mutations so that either all skills are installed or none are.
                if not root_mode and not overwrite:
                    conflict_dirs: list[str] = []

                    for src_dir in top_dirs:
                        # Derive the skill name from the top-level directory name, using
                        # the same normalization and validation rules as for root_mode.
                        candidate_name = _normalize_skill_name(src_dir.name)
                        if not candidate_name or not _SKILL_NAME_RE.fullmatch(candidate_name):
                            raise ValueError(f"Invalid skill name: {src_dir.name!r}.")
                        dest_dir = Path(self.skills_root) / candidate_name
                        if dest_dir.exists():
                            conflict_dirs.append(str(dest_dir))

                    if conflict_dirs:
                        # Fail before performing any install operations so we never end up
                        # with a partially-installed multi-skill archive.
                        raise FileExistsError(
                            "One or more skills from the archive already exist and "
                            "overwrite=False. No skills were installed. Conflicting "
                            f"paths: {', '.join(conflict_dirs)}"
                        )

                if root_mode:
                    archive_hint = _normalize_skill_name(
                        archive_skill_name or zip_path_obj.stem
                    )
                    if not archive_hint or not _SKILL_NAME_RE.fullmatch(archive_hint):
                        raise ValueError("Invalid skill name.")
                    skill_name = archive_hint

```

This edit assumes:
1. The current function has access to:
   - `root_mode` (bool), `overwrite` (bool), and `top_dirs` (an iterable of `Path` objects for each top-level directory in the extracted archive).
   - `self.skills_root`, `_normalize_skill_name`, and `_SKILL_NAME_RE` as shown in the snippet.
2. The multi-skill branch (the `else` of `if root_mode:`) uses `src_dir.name` to derive `skill_name` (or equivalent). If it uses a different mechanism, the same normalization/validation logic used here should be mirrored there to keep behavior consistent.

If `top_dirs` is named differently or constructed in a different scope, adjust the `for src_dir in top_dirs:` loop to iterate over the correct collection of top-level extracted skill directories.
</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 refactors the skill installation process to support batch installation of multiple skills from a single zip archive. It introduces logic to iterate through top-level directories, validating each for a required 'SKILL.md' file before extraction and activation. A review comment suggested improving code readability by replacing a complex ternary expression for skill name assignment with a clearer if-else structure.

[Soulter]

please use ruff format to format your code first.

btw, there only shows one skill's name when uploaded a zip file which has multiple skills:

[jmt059]

please use ruff format to format your code first.

btw, there only shows one skill's name when uploaded a zip file which has multiple skills:

Hi, regarding your question about "only showing one skill's name when uploading a zip with multiple skills":
I suspect you might have tested the previous version of this PR. I was temporarily misguided by the Sourcery AI bot earlier, which suggested using return installed_skills[0]. That might have caused the backend to only return the very first parsed name (like tavily) to the frontend.
I have now reverted this to return ", ".join(installed_skills) in my latest commit. The backend now correctly returns all extracted skill names.
Thanks again for the review and approval.