-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(skills): enhance skill installation to support multiple top-level folders and add duplicate handling, and Chinese skill name support #6952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1b8c8da
48f8a04
28c366b
c87c9b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||||||||||||
| import shutil | ||||||||||||||||||||||||||||||||||||||||||||||||
| import traceback | ||||||||||||||||||||||||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import Awaitable, Callable | ||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -44,6 +43,17 @@ def _to_bool(value: Any, default: bool = False) -> bool: | |||||||||||||||||||||||||||||||||||||||||||||||
| _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) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| class SkillsRoute(Route): | ||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, context: RouteContext, core_lifecycle) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| super().__init__(context) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -164,11 +174,24 @@ async def upload_skill(self): | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| temp_dir = get_astrbot_temp_path() | ||||||||||||||||||||||||||||||||||||||||||||||||
| os.makedirs(temp_dir, exist_ok=True) | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_path = os.path.join(temp_dir, filename) | ||||||||||||||||||||||||||||||||||||||||||||||||
| skill_mgr = SkillManager() | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_path = _next_available_temp_path(temp_dir, filename) | ||||||||||||||||||||||||||||||||||||||||||||||||
| await file.save(temp_path) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| 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). | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+181
to
+194
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: The outer try/except in The outer
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| await sync_skills_to_active_sandboxes() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -208,6 +231,7 @@ async def batch_upload_skills(self): | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| succeeded = [] | ||||||||||||||||||||||||||||||||||||||||||||||||
| failed = [] | ||||||||||||||||||||||||||||||||||||||||||||||||
| skipped = [] | ||||||||||||||||||||||||||||||||||||||||||||||||
| skill_mgr = SkillManager() | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_dir = get_astrbot_temp_path() | ||||||||||||||||||||||||||||||||||||||||||||||||
| os.makedirs(temp_dir, exist_ok=True) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -226,14 +250,42 @@ async def batch_upload_skills(self): | |||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| temp_path = os.path.join( | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_dir, f"batch_{uuid.uuid4().hex}_{filename}" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_path = _next_available_temp_path(temp_dir, filename) | ||||||||||||||||||||||||||||||||||||||||||||||||
| await file.save(temp_path) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| skill_name = skill_mgr.install_skill_from_zip( | ||||||||||||||||||||||||||||||||||||||||||||||||
| temp_path, overwrite=True | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||
| except FileExistsError: | ||||||||||||||||||||||||||||||||||||||||||||||||
| skipped.append( | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "filename": filename, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "name": Path(filename).stem, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "error": "Skill already exists.", | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| skill_name = None | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if skill_name is None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||
| succeeded.append({"filename": filename, "name": skill_name}) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -255,8 +307,10 @@ async def batch_upload_skills(self): | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| total = len(file_list) | ||||||||||||||||||||||||||||||||||||||||||||||||
| success_count = len(succeeded) | ||||||||||||||||||||||||||||||||||||||||||||||||
| skipped_count = len(skipped) | ||||||||||||||||||||||||||||||||||||||||||||||||
| failed_count = len(failed) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if success_count == total: | ||||||||||||||||||||||||||||||||||||||||||||||||
| if failed_count == 0 and success_count == total: | ||||||||||||||||||||||||||||||||||||||||||||||||
| message = f"All {total} skill(s) uploaded successfully." | ||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| Response() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -265,18 +319,35 @@ async def batch_upload_skills(self): | |||||||||||||||||||||||||||||||||||||||||||||||
| "total": total, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "succeeded": succeeded, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "failed": failed, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "skipped": skipped, | ||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||
| message, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| .__dict__ | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if failed_count == 0 and success_count == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||
| message = f"All {total} file(s) were skipped." | ||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| Response() | ||||||||||||||||||||||||||||||||||||||||||||||||
| .ok( | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "total": total, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "succeeded": succeeded, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "failed": failed, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "skipped": skipped, | ||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||
| message, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| .__dict__ | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if success_count == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||
| if success_count == 0 and skipped_count == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||
| message = f"Upload failed for all {total} file(s)." | ||||||||||||||||||||||||||||||||||||||||||||||||
| resp = Response().error(message) | ||||||||||||||||||||||||||||||||||||||||||||||||
| resp.data = { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "total": total, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "succeeded": succeeded, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "failed": failed, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "skipped": skipped, | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| return resp.__dict__ | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -288,6 +359,7 @@ async def batch_upload_skills(self): | |||||||||||||||||||||||||||||||||||||||||||||||
| "total": total, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "succeeded": succeeded, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "failed": failed, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "skipped": skipped, | ||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||
| message, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Consider using a more robust mechanism than
os.path.existsloops for picking a temp path.The
_next_available_temp_pathloop has a TOCTTOU race betweenos.path.existsandfile.saveunder concurrent uploads to the sametemp_dir/filename. Prefer an OS-backed primitive liketempfile.NamedTemporaryFile(delete=False, dir=temp_dir, suffix=suffix)ortempfile.mkstempto 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: