fix: return HTTP errors for failed skill downloads#9213
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
dosubot
Bot
added
size:XS
![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 improves error handling in the skill download endpoint by raising specific HTTPExceptions (404, 400, or 500) instead of returning generic error responses, and adds a corresponding test case. The feedback suggests making the exception message matching more robust by avoiding exact string comparisons, and expanding the test suite to cover the newly introduced 400 and 500 error status codes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| message = str(exc) | ||
| status_code = 404 if message == "Local skill not found" else 400 |
There was a problem hiding this comment.
Exact string matching on exception messages (message == "Local skill not found") is fragile. If the exception message in the service layer is ever modified (e.g., to include the skill name or for internationalization), this check will silently fail and fallback to a 400 status code instead of 404. Checking for the presence of "not found" in a case-insensitive manner is more robust.
| message = str(exc) | |
| status_code = 404 if message == "Local skill not found" else 400 | |
| message = str(exc) | |
| status_code = 404 if "not found" in message.lower() else 400 |
There was a problem hiding this comment.
Addressed in b11eb0d. _download_skill() no longer checks the exact "Local skill not found" string; it now maps not-found service errors with case-insensitive "not found" matching so minor wording changes do not silently downgrade the response to 400.
| 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(_name: str): | ||
| raise SkillsServiceError("Local skill not found") | ||
|
|
||
| monkeypatch.setattr( | ||
| skill_service, | ||
| "prepare_skill_archive", | ||
| fake_prepare_skill_archive, | ||
| ) | ||
|
|
||
| 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" |
There was a problem hiding this comment.
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"]There was a problem hiding this comment.
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.
![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 left some high level feedback:
- Mapping
SkillsServiceErrorto HTTP 404 by comparing the exception message string ("Local skill not found") is brittle and will break if messages are localized or changed; consider adding a specific error type/attribute or error code onSkillsServiceErrorand branching on that instead. - The generic
except Exceptionpath in_download_skillraises a 500 with the raw exception message as the response detail, which may leak internal information; consider returning a generic message to clients and logging the full exception separately.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Mapping `SkillsServiceError` to HTTP 404 by comparing the exception message string (`"Local skill not found"`) is brittle and will break if messages are localized or changed; consider adding a specific error type/attribute or error code on `SkillsServiceError` and branching on that instead.
- The generic `except Exception` path in `_download_skill` raises a 500 with the raw exception message as the response detail, which may leak internal information; consider returning a generic message to clients and logging the full exception separately.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
dosubot
Bot
added
size:S
|
Reviewer feedback has been addressed in b11eb0d:
Local validation after the reviewer changes: |
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Follow-up reviewer change pushed in 3314d1e:
Latest local validation: |
2 participants
Fixes #9212
Skill archive download failures now return HTTP error statuses instead of HTTP 200 JSON bodies, so blob/ZIP callers do not save backend error envelopes as fake
.zipfiles.What Problem This Solves
The skill archive download endpoints succeed with
FileResponse(..., media_type="application/zip"), but_download_skill()previously caughtSkillsServiceErrorand returnederror(str(exc))directly.Because
error()is a normal JSON envelope, FastAPI serialized failed downloads as HTTP 200 JSON. The dashboard download flow callsskillApi.download()withresponseType: 'blob', then wraps the response asapplication/zipbefore saving it as<skill>.zip. That means failed archive preparation could be persisted by the browser as a ZIP-looking file containing an error payload.Modifications
Updated
astrbot/dashboard/api/skills.pyso failed skill archive downloads raiseHTTPExceptioninstead of returning a 200 JSON envelope.Added an optional
SkillsServiceError.status_codeattribute so the service layer can mark missing local skill archives as 404 without parsing exception text; otherSkillsServiceErrorcases default to 400.Preserved full unexpected-exception logging while returning a generic HTTP 500 message to clients for unexpected archive preparation failures.
Added focused FastAPI v1 regression coverage for 404, 400, and 500 archive failure paths; 404 is covered through both query-name and path-name archive endpoints.
Left successful ZIP downloads, skill upload/list/edit/delete behavior, frontend API code, OpenAPI generation, and normal JSON skill APIs unchanged.
This is NOT a breaking change. This is a focused bug fix.
Evidence
Before this change, a failed archive preparation path looked like this:
After this change, the same failure path returns a non-2xx JSON error response, so blob/ZIP callers do not treat the failed download as a successful archive. The regression test also covers the 400 branch for default service errors and the 500 branch for unexpected exceptions, including the guarantee that the raw internal exception message is not returned to the client.
Possible call chain / impact
This only changes failed skill archive download responses. Successful archive downloads still return
application/zip, and ordinary skill JSON APIs keep the existing dashboard response envelope behavior.Screenshots or Test Results
Checklist
Summary by Sourcery
Return appropriate HTTP error statuses for failed skill archive downloads instead of successful ZIP responses.
Bug Fixes:
Tests: