Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions astrbot/dashboard/api/skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from typing import Any

from fastapi import APIRouter, Depends, Request
from fastapi import APIRouter, Depends, HTTPException, Request
from fastapi.responses import FileResponse

from astrbot.core import logger
Expand Down Expand Up @@ -94,10 +94,14 @@ async def _download_skill(service: SkillsService, name: str):
try:
return _archive_response(service.prepare_skill_archive(name))
except SkillsServiceError as exc:
return error(str(exc))
message = str(exc)
raise HTTPException(status_code=exc.status_code, detail=message) from exc
except Exception as exc:
logger.error(str(exc), exc_info=True)
return error(str(exc))
raise HTTPException(
status_code=500,
detail="Failed to prepare skill archive",
) from exc


@router.get("/skills")
Expand Down
6 changes: 4 additions & 2 deletions astrbot/dashboard/services/skills_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@


class SkillsServiceError(Exception):
pass
def __init__(self, message: str, *, status_code: int = 400) -> None:
super().__init__(message)
self.status_code = status_code


@dataclass
Expand Down Expand Up @@ -436,7 +438,7 @@ def prepare_skill_archive(self, name: str) -> SkillArchive:
skill_dir = Path(skill_mgr.skills_root) / skill_name
skill_md = skill_dir / "SKILL.md"
if not skill_dir.is_dir() or not skill_md.exists():
raise SkillsServiceError("Local skill not found")
raise SkillsServiceError("Local skill not found", status_code=404)

export_dir = Path(get_astrbot_temp_path()) / "skill_exports"
export_dir.mkdir(parents=True, exist_ok=True)
Expand Down
82 changes: 79 additions & 3 deletions tests/test_fastapi_v1_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
PLUGIN_UPDATE_SOURCE_REQUIRED_MESSAGE,
PluginServiceError,
)
from astrbot.dashboard.services.skills_service import SkillArchive
from astrbot.dashboard.services.skills_service import SkillArchive, SkillsServiceError

JWT_SECRET = "fastapi-v1-test-secret-with-32-bytes"

Expand Down Expand Up @@ -1215,8 +1215,7 @@ async def test_v1_knowledge_base_create_validation_uses_api_error_shape(
assert missing_provider_response.status_code == 200
assert missing_provider_response.json()["status"] == "error"
assert (
missing_provider_response.json()["message"]
== "缺少参数 embedding_provider_id"
missing_provider_response.json()["message"] == "缺少参数 embedding_provider_id"
)


Expand Down Expand Up @@ -3227,6 +3226,83 @@ async def fake_update_skill_file(data):
assert delete_response.json()["data"]["payload"] == {"name": skill_name}


@pytest.mark.asyncio
async def test_v1_skill_archive_errors_return_http_status(
asgi_app: FastAPI,
asgi_client: httpx.AsyncClient,
monkeypatch: pytest.MonkeyPatch,
):
skill_service = asgi_app.state.services.skills

def fake_prepare_skill_archive_404(_name: str):
raise SkillsServiceError("Local skill not found", status_code=404)

monkeypatch.setattr(
skill_service,
"prepare_skill_archive",
fake_prepare_skill_archive_404,
)

by_name_response = await asgi_client.get(
"/api/v1/skills/archive",
params={"skill_name": "missing_skill"},
headers=_jwt_headers(),
)
path_response = await asgi_client.get(
"/api/v1/skills/missing_skill/archive",
headers=_jwt_headers(),
)

assert by_name_response.status_code == 404
assert by_name_response.headers["content-type"].startswith("application/json")
assert by_name_response.json()["status"] == "error"
assert by_name_response.json()["message"] == "Local skill not found"
assert path_response.status_code == 404
assert path_response.headers["content-type"].startswith("application/json")
assert path_response.json()["message"] == "Local skill not found"
Comment on lines +3230 to +3262

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test currently only covers the 404 error path. Since this PR introduces new error handling that maps different exceptions to 400 and 500 status codes, it is highly recommended to expand the test suite to cover these cases as well to ensure full coverage and prevent future regressions.

async def test_v1_skill_archive_errors_return_http_status(
    asgi_app: FastAPI,
    asgi_client: httpx.AsyncClient,
    monkeypatch: pytest.MonkeyPatch,
):
    skill_service = asgi_app.state.services.skills

    # Test 404 Not Found
    def fake_prepare_skill_archive_404(_name: str):
        raise SkillsServiceError("Local skill not found")

    monkeypatch.setattr(
        skill_service,
        "prepare_skill_archive",
        fake_prepare_skill_archive_404,
    )

    by_name_response = await asgi_client.get(
        "/api/v1/skills/archive",
        params={"skill_name": "missing_skill"},
        headers=_jwt_headers(),
    )
    path_response = await asgi_client.get(
        "/api/v1/skills/missing_skill/archive",
        headers=_jwt_headers(),
    )

    assert by_name_response.status_code == 404
    assert by_name_response.headers["content-type"].startswith("application/json")
    assert by_name_response.json()["status"] == "error"
    assert by_name_response.json()["message"] == "Local skill not found"
    assert path_response.status_code == 404
    assert path_response.headers["content-type"].startswith("application/json")
    assert path_response.json()["message"] == "Local skill not found"

    # Test 400 Bad Request
    def fake_prepare_skill_archive_400(_name: str):
        raise SkillsServiceError("Invalid skill name")

    monkeypatch.setattr(
        skill_service,
        "prepare_skill_archive",
        fake_prepare_skill_archive_400,
    )

    bad_response = await asgi_client.get(
        "/api/v1/skills/archive",
        params={"skill_name": "invalid_skill"},
        headers=_jwt_headers(),
    )
    assert bad_response.status_code == 400
    assert bad_response.json()["status"] == "error"
    assert bad_response.json()["message"] == "Invalid skill name"

    # Test 500 Internal Server Error
    def fake_prepare_skill_archive_500(_name: str):
        raise RuntimeError("Unexpected database error")

    monkeypatch.setattr(
        skill_service,
        "prepare_skill_archive",
        fake_prepare_skill_archive_500,
    )

    server_error_response = await asgi_client.get(
        "/api/v1/skills/archive",
        params={"skill_name": "error_skill"},
        headers=_jwt_headers(),
    )
    assert server_error_response.status_code == 500
    assert server_error_response.json()["status"] == "error"
    assert "Unexpected database error" in server_error_response.json()["message"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in b11eb0d. The regression test now covers the 404, 400, and 500 archive failure paths. The 500 case also asserts that the raw internal exception text is not returned to the client; clients receive the generic Failed to prepare skill archive message while the full exception remains logged server-side.


def fake_prepare_skill_archive_400(_name: str):
raise SkillsServiceError("Invalid skill name")

monkeypatch.setattr(
skill_service,
"prepare_skill_archive",
fake_prepare_skill_archive_400,
)

bad_request_response = await asgi_client.get(
"/api/v1/skills/archive",
params={"skill_name": "invalid_skill"},
headers=_jwt_headers(),
)

assert bad_request_response.status_code == 400
assert bad_request_response.headers["content-type"].startswith("application/json")
assert bad_request_response.json()["status"] == "error"
assert bad_request_response.json()["message"] == "Invalid skill name"

def fake_prepare_skill_archive_500(_name: str):
raise RuntimeError("Unexpected database error")

monkeypatch.setattr(
skill_service,
"prepare_skill_archive",
fake_prepare_skill_archive_500,
)

server_error_response = await asgi_client.get(
"/api/v1/skills/archive",
params={"skill_name": "error_skill"},
headers=_jwt_headers(),
)

assert server_error_response.status_code == 500
assert server_error_response.headers["content-type"].startswith("application/json")
assert server_error_response.json()["status"] == "error"
assert server_error_response.json()["message"] == "Failed to prepare skill archive"
assert "Unexpected database error" not in server_error_response.text


@pytest.mark.asyncio
async def test_v1_safe_persona_routes_accept_slash_ids(
asgi_client: httpx.AsyncClient,
Expand Down
Loading