Conversation
…l folders and add duplicate handling closes: #6949
dosubot
bot
added
the
size:L
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 significantly improves the user experience for installing skills by making the process more adaptable to various archive structures and providing better feedback for batch operations. It allows for greater flexibility in how skill archives are packaged and handles duplicate installations more robustly, enhancing the overall reliability and usability of the skill management system. 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
feature:plugin
![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="630-634" />
<code_context>
continue
zf.extract(member, tmp_dir)
- src_dir = Path(tmp_dir) / skill_name
+ src_dir = Path(tmp_dir) if root_mode else Path(tmp_dir) / archive_root_name
+ normalized_path = _normalize_skill_markdown_path(src_dir)
+ if normalized_path is None:
+ raise ValueError("SKILL.md not found in the skill folder.")
_normalize_skill_markdown_path(src_dir)
if not src_dir.exists():
raise ValueError("Skill folder not found after extraction.")
</code_context>
<issue_to_address>
**suggestion:** Avoid calling `_normalize_skill_markdown_path` twice or make use of the `normalized_path` result.
`_normalize_skill_markdown_path(src_dir)` is currently called once to assign `normalized_path` (only for a `None` check) and then called again with the result discarded. This is either redundant or risks unintended double side effects. Call it once, check for `None`, and then reuse that same `normalized_path` value (or drop the variable entirely if you truly just need the side effect).
</issue_to_address>
### Comment 2
<location path="astrbot/dashboard/routes/skills.py" line_range="181-194" />
<code_context>
- skill_mgr = SkillManager()
- skill_name = skill_mgr.install_skill_from_zip(temp_path, overwrite=True)
+ try:
+ try:
+ skill_name = skill_mgr.install_skill_from_zip(
+ temp_path, overwrite=False, skill_name_hint=Path(filename).stem
+ )
+ except TypeError:
+ # Backward compatibility for callers that do not accept skill_name_hint
+ skill_name = skill_mgr.install_skill_from_zip(
+ temp_path, overwrite=False
+ )
+ except Exception:
+ # Keep behavior consistent with previous implementation
+ # and bubble up install errors (including duplicates).
</code_context>
<issue_to_address>
**suggestion:** The outer try/except in `upload_skill` that just re-raises looks unnecessary.
The outer `try/except Exception: raise` adds no behavior beyond normal exception propagation, since it neither adds context nor transforms the error. Unless you plan to add logging/metrics here soon, you can drop this wrapper to simplify the control flow without changing behavior.
```suggestion
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False, skill_name_hint=Path(filename).stem
)
except TypeError:
# Backward compatibility for callers that do not accept skill_name_hint
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)
```
</issue_to_address>
### Comment 3
<location path="astrbot/dashboard/routes/skills.py" line_range="46-54" />
<code_context>
_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")
+def _next_available_temp_path(temp_dir: str, filename: str) -> str:
+ stem = Path(filename).stem
+ suffix = Path(filename).suffix
+ candidate = filename
+ index = 1
+ while os.path.exists(os.path.join(temp_dir, candidate)):
+ candidate = f"{stem}_{index}{suffix}"
+ index += 1
+ return os.path.join(temp_dir, candidate)
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using a more robust mechanism than `os.path.exists` loops for picking a temp path.
The `_next_available_temp_path` loop has a TOCTTOU race between `os.path.exists` and `file.save` under concurrent uploads to the same `temp_dir`/`filename`. Prefer an OS-backed primitive like `tempfile.NamedTemporaryFile(delete=False, dir=temp_dir, suffix=suffix)` or `tempfile.mkstemp` to avoid collisions. If you need a readable base name, you can include the original stem in the generated prefix while still relying on the OS for uniqueness.
Suggested implementation:
```python
import os
import re
import shutil
import tempfile
import traceback
from collections.abc import Awaitable, Callable
from pathlib import Path
from typing import Any
_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")
```
```python
def _next_available_temp_path(temp_dir: str, filename: str) -> str:
stem = Path(filename).stem
suffix = Path(filename).suffix
# Use an OS-backed primitive to avoid TOCTTOU issues under concurrent uploads.
# We include the original stem in the prefix for readability, while relying
# on the OS for uniqueness.
fd, path = tempfile.mkstemp(
dir=temp_dir,
prefix=f"{stem}_",
suffix=suffix,
)
os.close(fd)
return path
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
astrbot/core/skills/skill_manager.py
Outdated
| src_dir = Path(tmp_dir) if root_mode else Path(tmp_dir) / archive_root_name | ||
| normalized_path = _normalize_skill_markdown_path(src_dir) | ||
| if normalized_path is None: | ||
| raise ValueError("SKILL.md not found in the skill folder.") | ||
| _normalize_skill_markdown_path(src_dir) |
There was a problem hiding this comment.
suggestion: Avoid calling _normalize_skill_markdown_path twice or make use of the normalized_path result.
_normalize_skill_markdown_path(src_dir) is currently called once to assign normalized_path (only for a None check) and then called again with the result discarded. This is either redundant or risks unintended double side effects. Call it once, check for None, and then reuse that same normalized_path value (or drop the variable entirely if you truly just need the side effect).
| try: | ||
| try: | ||
| skill_name = skill_mgr.install_skill_from_zip( | ||
| temp_path, overwrite=False, skill_name_hint=Path(filename).stem | ||
| ) | ||
| except TypeError: | ||
| # Backward compatibility for callers that do not accept skill_name_hint | ||
| skill_name = skill_mgr.install_skill_from_zip( | ||
| temp_path, overwrite=False | ||
| ) | ||
| except Exception: | ||
| # Keep behavior consistent with previous implementation | ||
| # and bubble up install errors (including duplicates). | ||
| raise |
There was a problem hiding this comment.
suggestion: The outer try/except in upload_skill that just re-raises looks unnecessary.
The outer try/except Exception: raise adds no behavior beyond normal exception propagation, since it neither adds context nor transforms the error. Unless you plan to add logging/metrics here soon, you can drop this wrapper to simplify the control flow without changing behavior.
| try: | |
| try: | |
| skill_name = skill_mgr.install_skill_from_zip( | |
| temp_path, overwrite=False, skill_name_hint=Path(filename).stem | |
| ) | |
| except TypeError: | |
| # Backward compatibility for callers that do not accept skill_name_hint | |
| skill_name = skill_mgr.install_skill_from_zip( | |
| temp_path, overwrite=False | |
| ) | |
| except Exception: | |
| # Keep behavior consistent with previous implementation | |
| # and bubble up install errors (including duplicates). | |
| raise | |
| try: | |
| skill_name = skill_mgr.install_skill_from_zip( | |
| temp_path, overwrite=False, skill_name_hint=Path(filename).stem | |
| ) | |
| except TypeError: | |
| # Backward compatibility for callers that do not accept skill_name_hint | |
| skill_name = skill_mgr.install_skill_from_zip( | |
| temp_path, overwrite=False | |
| ) |
| def _next_available_temp_path(temp_dir: str, filename: str) -> str: | ||
| stem = Path(filename).stem | ||
| suffix = Path(filename).suffix | ||
| candidate = filename | ||
| index = 1 | ||
| while os.path.exists(os.path.join(temp_dir, candidate)): | ||
| candidate = f"{stem}_{index}{suffix}" | ||
| index += 1 | ||
| return os.path.join(temp_dir, candidate) |
There was a problem hiding this comment.
suggestion (bug_risk): Consider using a more robust mechanism than os.path.exists loops for picking a temp path.
The _next_available_temp_path loop has a TOCTTOU race between os.path.exists and file.save under concurrent uploads to the same temp_dir/filename. Prefer an OS-backed primitive like tempfile.NamedTemporaryFile(delete=False, dir=temp_dir, suffix=suffix) or tempfile.mkstemp to avoid collisions. If you need a readable base name, you can include the original stem in the generated prefix while still relying on the OS for uniqueness.
Suggested implementation:
import os
import re
import shutil
import tempfile
import traceback
from collections.abc import Awaitable, Callable
from pathlib import Path
from typing import Any
_SKILL_NAME_RE = re.compile(r"^[A-Za-z0-9._-]+$")def _next_available_temp_path(temp_dir: str, filename: str) -> str:
stem = Path(filename).stem
suffix = Path(filename).suffix
# Use an OS-backed primitive to avoid TOCTTOU issues under concurrent uploads.
# We include the original stem in the prefix for readability, while relying
# on the OS for uniqueness.
fd, path = tempfile.mkstemp(
dir=temp_dir,
prefix=f"{stem}_",
suffix=suffix,
)
os.close(fd)
return path![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 refactors the skill installation process to support more flexible archive structures, allowing SKILL.md to be either at the root or within a single top-level folder. It also introduces a skill_name_hint for installation and enhances the batch skill upload functionality to gracefully handle existing skills by marking them as skipped. Review feedback indicates that the UI text describing the archive structure is misleading in multiple languages and requires correction. Additionally, there are several opportunities to improve code clarity and reduce redundancy in skill_manager.py, such as removing redundant validation checks and duplicate function calls. The batch_upload_skills function in skills.py also contains duplicated error handling logic for FileExistsError that should be refactored for better maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
dashboard/src/i18n/locales/en-US/features/extension.json (245)
The updated structureRequirement text is misleading. The new implementation supports a zip archive with either a single top-level folder containing the skill, or a SKILL.md file at the root of the archive. It does not support "multiple skills folders" within a single zip archive. Please update the text to accurately describe the two supported formats.
A more accurate description would be: "Each zip must contain either a single top-level folder with the skill files, or have the SKILL.md file at the root of the archive."
"structureRequirement": "Each zip must contain either a single top-level folder with the skill files, or have the `SKILL.md` file at the root of the archive.",
dashboard/src/i18n/locales/ru-RU/features/extension.json (244)
The updated structureRequirement text is misleading. The new implementation supports a zip archive with either a single top-level folder containing the skill, or a SKILL.md file at the root of the archive. It does not support "multiple skills folders" within a single zip archive. Please update the text to accurately describe the two supported formats.
A more accurate description in Russian would be: "Архив должен содержать либо одну папку верхнего уровня с файлами навыка, либо файл SKILL.md в корне архива."
"structureRequirement": "Архив должен содержать либо одну папку верхнего уровня с файлами навыка, либо файл `SKILL.md` в корне архива.",
dashboard/src/i18n/locales/zh-CN/features/extension.json (248)
The updated structureRequirement text is misleading. The new implementation supports a zip archive with either a single top-level folder containing the skill, or a SKILL.md file at the root of the archive. It does not support "multiple skills folders" within a single zip archive. Please update the text to accurately describe the two supported formats.
A more accurate description in Chinese would be: "每个 zip 压缩包必须只包含一个顶层目录（其中包含技能文件），或者在压缩包的根目录中包含 SKILL.md 文件。"
"structureRequirement": "每个 zip 压缩包必须只包含一个顶层目录（其中包含技能文件），或者在压缩包的根目录中包含 `SKILL.md` 文件。",
astrbot/core/skills/skill_manager.py (38-39)
The validation check if not _SKILL_NAME_RE.match(normalized): appears to be redundant. The preceding normalization steps (lines 34-37) should already guarantee that normalized conforms to the _SKILL_NAME_RE pattern. If this check were to fail, it would indicate a logic error in the normalization code itself.
Consider removing this check for clarity. If you wish to keep it as a safeguard, changing it to an assert would better reflect its purpose as an internal consistency check.
assert _SKILL_NAME_RE.match(normalized), "Internal error: normalized skill name is invalid"astrbot/core/skills/skill_manager.py (614-622)
The check for SKILL.md within the if root_mode: block is redundant. The root_mode flag is already set to True only if a SKILL.md or skill.md file exists at the root of the archive (lines 566-574). This re-check is unnecessary and can be removed.
if not root_mode:
if (
f"{archive_root_name}/SKILL.md" not in file_names
and f"{archive_root_name}/skill.md" not in file_names
):
raise ValueError("SKILL.md not found in the skill folder.")astrbot/core/skills/skill_manager.py (634)
The function _normalize_skill_markdown_path is called twice consecutively (lines 631 and 634). The first call's result is used, but the second call is redundant as its return value is ignored. This function has side effects (renaming files), so calling it twice is unnecessary and could be confusing. Please remove the second call.
astrbot/dashboard/routes/skills.py (256-285)
There is duplicated code for handling FileExistsError. The block that appends to the skipped list and sets skill_name = None appears in two separate except clauses. This can be refactored to improve maintainability.
You could extract the duplicate handling into a local function or restructure the try...except blocks.
try:
try:
skill_name = skill_mgr.install_skill_from_zip(
temp_path,
overwrite=False,
skill_name_hint=Path(filename).stem,
)
except TypeError:
# Backward compatibility for monkeypatched implementations in tests
skill_name = skill_mgr.install_skill_from_zip(
temp_path, overwrite=False
)
except FileExistsError:
skipped.append(
{
"filename": filename,
"name": Path(filename).stem,
"error": "Skill already exists.",
}
)
skill_name = None
Soulter
changed the title
1 participant
closes: #6949
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
Enhance skill installation to support root-level skill archives and improve duplicate handling and batch upload feedback.
New Features:
Enhancements: