fix(skills): support rootless skill zip uploads#6982
fix(skills): support rootless skill zip uploads#6982ANGJustinl wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a common issue where valid skill packages were rejected during WebUI upload due to strict zip archive structure requirements. The changes introduce a more tolerant skill installation process by intelligently detecting the skill's root directory within a zip file, whether it's at the root level or nested. This significantly improves the user experience by allowing a wider variety of skill package formats while maintaining validation against ambiguous multi-skill archives. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the skill installation process from zip archives. The install_skill_from_zip function has been updated to be more flexible, allowing SKILL.md to be located either at the root of the zip archive or within a nested directory. New helper functions were introduced to normalize zip entry paths, discover the skill root within an archive, parse skill frontmatter, and infer the skill name from various sources. This ensures that only one skill can be installed per zip file. Corresponding UI text in the dashboard has been updated to reflect these new capabilities, and comprehensive unit tests have been added to cover rootless, nested, and multi-skill archive scenarios.
dosubot
bot
added
the
area:webui
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/skills/skill_manager.py" line_range="673-682" />
<code_context>
+ validated_members: list[tuple[zipfile.ZipInfo, PurePosixPath]] = []
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Extraction still uses the raw zip filenames rather than the normalized paths, which may diverge from the validated structure.
You compute a normalized path for each `ZipInfo` but then extract using `member.filename`, so the actual directory structure comes from the raw names, not the validated paths. To avoid discrepancies (especially with odd path forms), either extract using the normalized paths (e.g., manually writing files) or add a check that `Path(member.filename)` under `tmp_dir` resolves to the same location as `normalized_path`.
Suggested implementation:
```python
validated_members: list[tuple[zipfile.ZipInfo, PurePosixPath]] = []
file_paths: list[PurePosixPath] = []
normalized_paths: list[PurePosixPath] = []
for member in zf.infolist():
```
To fully implement the suggestion, you should adjust the code inside and after the `for member in zf.infolist():` loop as follows (exact code will depend on the current implementation):
1. **Inside the loop (where you already compute the normalized path)**:
- Assuming you already compute something like:
```python
normalized_path = PurePosixPath(<some_normalization_logic_using_member.filename>)
```
- Append it to `normalized_paths`:
```python
normalized_paths.append(normalized_path)
```
- Before appending to `validated_members`, compute the resolved targets under `tmp_dir` (or whatever temporary root you use):
```python
raw_target = (tmp_dir / member.filename).resolve()
normalized_target = (tmp_dir / normalized_path).resolve()
if raw_target != normalized_target:
raise ValueError(
f"Zip member {member.filename!r} resolves to {raw_target} "
f"which does not match validated normalized path {normalized_target}"
)
```
- Only after this check, append to `validated_members`:
```python
validated_members.append((member, normalized_path))
```
2. **Where you perform extraction**:
- If you currently do:
```python
for member in zf.infolist():
zf.extract(member, path=tmp_dir)
```
change it to iterate over `validated_members` and use the normalized path:
```python
for member, normalized_path in validated_members:
target_path = (tmp_dir / normalized_path).resolve()
target_path.parent.mkdir(parents=True, exist_ok=True)
with zf.open(member, "r") as src, target_path.open("wb") as dst:
shutil.copyfileobj(src, dst)
```
- This ensures the actual filesystem layout derives from the validated normalized paths, not the raw zip filenames.
3. **Imports**:
- Ensure the file has:
```python
from pathlib import Path, PurePosixPath
import shutil
```
(or re-use existing imports if they are already present).
These changes will guarantee that either:
- Extraction strictly follows the normalized/validated structure, or
- A mismatch between the raw filename and normalized path is detected and rejected before extraction.
</issue_to_address>
### Comment 2
<location path="tests/test_skill_zip_install.py" line_range="72-81" />
<code_context>
+def test_install_skill_from_nested_zip_root(monkeypatch, tmp_path: Path):
</code_context>
<issue_to_address>
**suggestion (testing):** Exercise additional `SkillManager.install_skill_from_zip` name sources including `skill_name_hint` and invalid frontmatter.
The nested-root test currently covers only the auto-discovery + valid frontmatter path. Please also add tests for the remaining `_infer_skill_name` branches: (1) nested archive where `skill_name_hint` is provided and takes precedence over folder/frontmatter; (2) invalid folder and frontmatter names but valid sanitized zip stem; and (3) all name sources invalid, asserting that `ValueError("Unable to determine a valid skill name"...)` is raised, to lock in precedence and error handling behavior.
</issue_to_address>
### Comment 3
<location path="tests/test_skill_zip_install.py" line_range="100-109" />
<code_context>
+ ) == "echo ok"
+
+
+def test_install_skill_from_zip_rejects_multiple_skill_roots(
+ monkeypatch,
+ tmp_path: Path,
+):
+ manager, _skills_root = _make_manager(monkeypatch, tmp_path)
+ archive_path = tmp_path / "ambiguous.zip"
+ _write_zip(
+ archive_path,
+ {
+ "skill_a/SKILL.md": "---\nname: skill-a\n---\n",
+ "skill_b/SKILL.md": "---\nname: skill-b\n---\n",
+ },
+ )
+
+ with pytest.raises(ValueError, match="Multiple skill roots found"):
+ manager.install_skill_from_zip(str(archive_path))
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for zip path normalization and security checks (absolute paths and `..` entries).
The new `_normalize_zip_entry_path` and `_discover_skill_root_from_archive` helpers add stricter checks for absolute paths and `..` segments but their failure modes aren’t directly tested. Please add cases that build zips with entries like `/evil/SKILL.md`, `C:\evil\SKILL.md` (Windows-style), and `../escape/SKILL.md`, and assert that `install_skill_from_zip` raises `ValueError` with the expected messages so regressions in the normalization logic are caught early.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| validated_members: list[tuple[zipfile.ZipInfo, PurePosixPath]] = [] | ||
| file_paths: list[PurePosixPath] = [] | ||
|
|
||
| archive_skill_name = None | ||
| if skill_name_hint is not None: | ||
| archive_skill_name = _normalize_skill_name(skill_name_hint) | ||
| if archive_skill_name and not _SKILL_NAME_RE.fullmatch( | ||
| archive_skill_name | ||
| ): | ||
| raise ValueError("Invalid skill name.") | ||
| for member in zf.infolist(): | ||
| normalized_path = _normalize_zip_entry_path(member.filename) | ||
| if normalized_path is None: | ||
| continue | ||
| validated_members.append((member, normalized_path)) | ||
| if not member.is_dir(): | ||
| file_paths.append(normalized_path) |
There was a problem hiding this comment.
suggestion (bug_risk): Extraction still uses the raw zip filenames rather than the normalized paths, which may diverge from the validated structure.
You compute a normalized path for each ZipInfo but then extract using member.filename, so the actual directory structure comes from the raw names, not the validated paths. To avoid discrepancies (especially with odd path forms), either extract using the normalized paths (e.g., manually writing files) or add a check that Path(member.filename) under tmp_dir resolves to the same location as normalized_path.
Suggested implementation:
validated_members: list[tuple[zipfile.ZipInfo, PurePosixPath]] = []
file_paths: list[PurePosixPath] = []
normalized_paths: list[PurePosixPath] = []
for member in zf.infolist():To fully implement the suggestion, you should adjust the code inside and after the for member in zf.infolist(): loop as follows (exact code will depend on the current implementation):
-
Inside the loop (where you already compute the normalized path):
- Assuming you already compute something like:
normalized_path = PurePosixPath(<some_normalization_logic_using_member.filename>)
- Append it to
normalized_paths:normalized_paths.append(normalized_path)
- Before appending to
validated_members, compute the resolved targets undertmp_dir(or whatever temporary root you use):raw_target = (tmp_dir / member.filename).resolve() normalized_target = (tmp_dir / normalized_path).resolve() if raw_target != normalized_target: raise ValueError( f"Zip member {member.filename!r} resolves to {raw_target} " f"which does not match validated normalized path {normalized_target}" )
- Only after this check, append to
validated_members:validated_members.append((member, normalized_path))
- Assuming you already compute something like:
-
Where you perform extraction:
- If you currently do:
change it to iterate over
for member in zf.infolist(): zf.extract(member, path=tmp_dir)
validated_membersand use the normalized path:for member, normalized_path in validated_members: target_path = (tmp_dir / normalized_path).resolve() target_path.parent.mkdir(parents=True, exist_ok=True) with zf.open(member, "r") as src, target_path.open("wb") as dst: shutil.copyfileobj(src, dst)
- This ensures the actual filesystem layout derives from the validated normalized paths, not the raw zip filenames.
- If you currently do:
-
Imports:
- Ensure the file has:
(or re-use existing imports if they are already present).
from pathlib import Path, PurePosixPath import shutil
- Ensure the file has:
These changes will guarantee that either:
- Extraction strictly follows the normalized/validated structure, or
- A mismatch between the raw filename and normalized path is detected and rejected before extraction.
| def test_install_skill_from_nested_zip_root(monkeypatch, tmp_path: Path): | ||
| manager, skills_root = _make_manager(monkeypatch, tmp_path) | ||
| archive_path = tmp_path / "nested.zip" | ||
| _write_zip( | ||
| archive_path, | ||
| { | ||
| "downloaded-package/README.md": "outer readme", | ||
| "downloaded-package/skills/demo_skill/SKILL.md": ( | ||
| "---\n" | ||
| "name: demo-skill\n" |
There was a problem hiding this comment.
suggestion (testing): Exercise additional SkillManager.install_skill_from_zip name sources including skill_name_hint and invalid frontmatter.
The nested-root test currently covers only the auto-discovery + valid frontmatter path. Please also add tests for the remaining _infer_skill_name branches: (1) nested archive where skill_name_hint is provided and takes precedence over folder/frontmatter; (2) invalid folder and frontmatter names but valid sanitized zip stem; and (3) all name sources invalid, asserting that ValueError("Unable to determine a valid skill name"...) is raised, to lock in precedence and error handling behavior.
| def test_install_skill_from_zip_rejects_multiple_skill_roots( | ||
| monkeypatch, | ||
| tmp_path: Path, | ||
| ): | ||
| manager, _skills_root = _make_manager(monkeypatch, tmp_path) | ||
| archive_path = tmp_path / "ambiguous.zip" | ||
| _write_zip( | ||
| archive_path, | ||
| { | ||
| "skill_a/SKILL.md": "---\nname: skill-a\n---\n", |
There was a problem hiding this comment.
suggestion (testing): Add tests for zip path normalization and security checks (absolute paths and .. entries).
The new _normalize_zip_entry_path and _discover_skill_root_from_archive helpers add stricter checks for absolute paths and .. segments but their failure modes aren’t directly tested. Please add cases that build zips with entries like /evil/SKILL.md, C:\evil\SKILL.md (Windows-style), and ../escape/SKILL.md, and assert that install_skill_from_zip raises ValueError with the expected messages so regressions in the normalization logic are caught early.
|
已进行部署测试 |
There was a problem hiding this comment.
Pull request overview
This PR improves AstrBot skill zip installation so the backend can install skills from archives that don’t have a single top-level folder (e.g., skill files at the zip root or nested under an extra wrapper directory), and updates the WebUI copy/tests accordingly.
Changes:
- Update skill zip installation to detect the skill root by locating
SKILL.md/skill.md, infer the installed skill name, and keep rejecting ambiguous multi-root archives. - Add unit tests for root-level, nested-root, and ambiguous multi-root skill archives; extend dashboard batch-upload tests for rootless archives.
- Update WebUI i18n strings (EN/ZH/RU) to clarify automatic skill root detection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
astrbot/core/skills/skill_manager.py |
Implements root discovery via SKILL.md, path normalization, and name inference for more tolerant zip installs. |
tests/test_skill_zip_install.py |
Adds focused unit tests for rootless/nested skill zips and ambiguous multi-root rejection. |
tests/test_dashboard.py |
Extends batch-upload API test coverage to include rootless skill archives. |
dashboard/src/i18n/locales/en-US/features/extension.json |
Updates upload hint text to reflect auto-detected skill roots. |
dashboard/src/i18n/locales/zh-CN/features/extension.json |
Updates upload hint text to reflect auto-detected skill roots. |
dashboard/src/i18n/locales/ru-RU/features/extension.json |
Updates upload hint text to reflect auto-detected skill roots. |
| if not member_name or _is_ignored_zip_entry(member_name): | ||
| continue | ||
| extraction_root = Path(tmp_dir) | ||
| for member, _normalized_path in validated_members: |
There was a problem hiding this comment.
install_skill_from_zip() computes skill_root_rel before extraction, but still extracts all validated zip members into the temp directory, including files outside the detected skill root. This increases disk usage and exposure to zip-bomb style uploads, and it’s unnecessary since you already track each member’s normalized path. Consider filtering validated_members to only those under skill_root_rel (plus any required parent dirs) or rejecting archives that contain entries outside the chosen root to keep validation strict and avoid extracting unrelated content.
| for member, _normalized_path in validated_members: | |
| for member, normalized_path in validated_members: | |
| # Only extract entries that belong to the detected skill root. | |
| # If skill_root_rel has no parts, the whole archive is treated as the root. | |
| if skill_root_rel.parts and not normalized_path.is_relative_to( | |
| skill_root_rel | |
| ): | |
| continue |
| def _discover_skill_root_from_archive( | ||
| file_paths: list[PurePosixPath], | ||
| ) -> PurePosixPath: | ||
| candidate_dirs = { | ||
| path.parent for path in file_paths if path.name in _SKILL_MARKDOWN_FILENAMES | ||
| } | ||
| if not candidate_dirs: | ||
| raise ValueError("SKILL.md not found in the zip archive.") | ||
|
|
||
| ordered_candidates = sorted( | ||
| candidate_dirs, key=lambda path: (len(path.parts), str(path)) | ||
| ) | ||
| shallowest_depth = len(ordered_candidates[0].parts) | ||
| shallowest_candidates = [ | ||
| path for path in ordered_candidates if len(path.parts) == shallowest_depth | ||
| ] | ||
| if len(shallowest_candidates) != 1: | ||
| raise ValueError( | ||
| "Multiple skill roots found in the zip archive. Please upload one skill per zip." | ||
| ) | ||
|
|
||
| skill_root = shallowest_candidates[0] | ||
| for candidate in ordered_candidates[1:]: | ||
| if not _path_is_ancestor_or_same(skill_root, candidate): | ||
| raise ValueError( | ||
| "Multiple skill roots found in the zip archive. Please upload one skill per zip." | ||
| ) | ||
| return skill_root |
There was a problem hiding this comment.
_discover_skill_root_from_archive() returns path.parent for SKILL.md entries, which for a root-level SKILL.md is typically PurePosixPath('.'). The subsequent logic treats .parts as indicating “has a directory”, which can make root-level archives behave like nested ones (e.g., the if not skill_root_rel.parts / if skill_root_rel.parts branches become misleading or unreachable), and it can also make the ancestor check treat the root as not an ancestor of nested candidates. Consider normalizing a root-level skill root to an explicit sentinel (e.g., empty tuple / None) or special-casing PurePosixPath('.') so root-depth is 0 and ancestor checks work as intended.
|
what are the differences with #6952? |
This PR fixes skill zip uploads in the WebUI
Currently, skill uploads may fail when the zip does not contain a single top-level folder. Many skill packages downloaded from external sites place the skill files directly at the zip root, or place
SKILL.mdinside a nested directory instead ofskillname/. This causes valid skill packages to be rejected during upload.This change makes skill zip installation more tolerant while still keeping archive validation strict enough to reject ambiguous multi-skill packages.
Modifications / 改动点
Updated
astrbot/core/skills/skill_manager.pySKILL.mdorskill.mdin the archive instead of requiring exactly one top-level folder.SKILL.mdfrontmatter, or the zip filename when needed.Added tests in
tests/test_skill_zip_install.pyExtended
tests/test_dashboard.pyUpdated WebUI upload copy in:
dashboard/src/i18n/locales/en-US/features/extension.jsondashboard/src/i18n/locales/zh-CN/features/extension.jsondashboard/src/i18n/locales/ru-RU/features/extension.jsonThis is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Verification steps executed locally:
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
Relax skill zip installation to detect the skill root from SKILL.md and infer the skill name, allowing rootless and nested archives while still rejecting ambiguous multi-skill packages.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: