-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 2 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) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+46
to
+54
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 (bug_risk): Consider using a more robust mechanism than The 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 |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| 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: Avoid calling
_normalize_skill_markdown_pathtwice or make use of thenormalized_pathresult._normalize_skill_markdown_path(src_dir)is currently called once to assignnormalized_path(only for aNonecheck) and then called again with the result discarded. This is either redundant or risks unintended double side effects. Call it once, check forNone, and then reuse that samenormalized_pathvalue (or drop the variable entirely if you truly just need the side effect).