diff --git a/README.md b/README.md index 1a8b796ef7..b70c4d133f 100644 --- a/README.md +++ b/README.md @@ -1662,6 +1662,9 @@ view. Built-in and user-created/imported skills are listed separately. The Community tab supports importing `.skill` archives into My skills. General preferences remain in Settings. +Failed skill requests retain their HTTP error status and a readable fallback +when the server returns an unusable error body. + The **ppt-generation** skill applies photo EXIF orientation before fitting and embedding slide images, so rotated or mirrored photos retain their displayed orientation. diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 51b80b0805..e81a9d5776 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -280,6 +280,10 @@ or user/skill changes. `core/skills/export.ts` owns the revision-bound Blob down HTTP 409 requires explicit preview refresh. Keep file lists paginated and diagnostics localized. Browser handoff does not prove the file was saved to disk. +`core/skills/error-detail.ts` reads error JSON as unknown for skill management +and export. Narrow records and string fields before use; malformed bodies must +preserve typed HTTP failures and the upload API's soft-failure contract. + Sidebar rows request deletion through `ThreadDeleteDialogProvider`, hosted in `WorkspaceSidebar` outside the virtualized flat/project lists. Keep the selected thread snapshot and retry UI alive when a partial deletion removes its row. diff --git a/frontend/src/core/skills/api.ts b/frontend/src/core/skills/api.ts index 6f9626f006..93e8f204a7 100644 --- a/frontend/src/core/skills/api.ts +++ b/frontend/src/core/skills/api.ts @@ -1,6 +1,7 @@ import { fetch } from "@/core/api/fetcher"; import { getBackendBaseURL } from "@/core/config"; +import { isErrorDetailRecord, readSkillErrorDetail } from "./error-detail"; import type { Skill, SkillLoadDiagnostic } from "./type"; // Keep this in lockstep with `_MAX_SKILL_ARCHIVE_UPLOAD_BYTES` in @@ -93,26 +94,16 @@ export function formatSkillSecurityFindings( } async function readErrorDetail(response: Response): Promise { - const data = (await response.json().catch(() => ({}))) as { - detail?: - | string - | { - message?: unknown; - skill_name?: unknown; - findings?: unknown; - }; - }; - if (typeof data.detail === "string") { - return { message: data.detail, findings: [] }; + const detail = await readSkillErrorDetail(response); + if (typeof detail === "string") { + return { message: detail, findings: [] }; } - if (typeof data.detail?.message === "string") { + if (isErrorDetailRecord(detail) && typeof detail.message === "string") { return { - message: data.detail.message, + message: detail.message, skillName: - typeof data.detail.skill_name === "string" - ? data.detail.skill_name - : undefined, - findings: parseSecurityFindings(data.detail.findings), + typeof detail.skill_name === "string" ? detail.skill_name : undefined, + findings: parseSecurityFindings(detail.findings), }; } return { diff --git a/frontend/src/core/skills/error-detail.ts b/frontend/src/core/skills/error-detail.ts new file mode 100644 index 0000000000..b6e9df4eb3 --- /dev/null +++ b/frontend/src/core/skills/error-detail.ts @@ -0,0 +1,12 @@ +export function isErrorDetailRecord( + value: unknown, +): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +export async function readSkillErrorDetail( + response: Response, +): Promise { + const data: unknown = await response.json().catch(() => null); + return isErrorDetailRecord(data) ? data.detail : undefined; +} diff --git a/frontend/src/core/skills/export.ts b/frontend/src/core/skills/export.ts index 4940a0436d..fca624d30e 100644 --- a/frontend/src/core/skills/export.ts +++ b/frontend/src/core/skills/export.ts @@ -1,6 +1,8 @@ import { fetch } from "@/core/api/fetcher"; import { getBackendBaseURL } from "@/core/config"; +import { isErrorDetailRecord, readSkillErrorDetail } from "./error-detail"; + export interface SkillExportNotice { code: string; message: string; @@ -46,18 +48,16 @@ async function exportRequest( { signal, cache: "no-store" }, ); if (!response.ok) { - const data = (await response.json().catch(() => ({}))) as { - detail?: { code?: string; message?: string } | string; - }; - const detail = data.detail; + const detail = await readSkillErrorDetail(response); + const record = isErrorDetailRecord(detail) ? detail : undefined; throw new SkillExportRequestError( response.status, - typeof detail === "object" - ? (detail.code ?? "skill_export_failed") - : "skill_export_failed", + typeof record?.code === "string" ? record.code : "skill_export_failed", typeof detail === "string" ? detail - : (detail?.message ?? "Could not export this skill."), + : typeof record?.message === "string" + ? record.message + : "Could not export this skill.", ); } return response; diff --git a/frontend/tests/unit/core/skills/api.test.ts b/frontend/tests/unit/core/skills/api.test.ts index 2e53954c60..fc3b288b35 100644 --- a/frontend/tests/unit/core/skills/api.test.ts +++ b/frontend/tests/unit/core/skills/api.test.ts @@ -36,6 +36,25 @@ beforeEach(() => { }); describe("skills api", () => { + test("retains a typed HTTP failure when the response body is JSON null", async () => { + mockedFetch.mockResolvedValueOnce(jsonResponse(403, null)); + await expect(enableSkill("demo", true)).rejects.toMatchObject({ + name: "SkillRequestError", + status: 403, + message: "HTTP 403: Error", + }); + }); + + test("keeps upload soft failures when the response body is JSON null", async () => { + mockedFetch.mockResolvedValueOnce(jsonResponse(500, null)); + await expect( + uploadSkillArchive(new File(["archive"], "demo.skill")), + ).resolves.toEqual({ + success: false, + skill_name: "", + message: "HTTP 500: Error", + }); + }); test("uploads a local .skill archive as multipart form data", async () => { mockedFetch.mockResolvedValueOnce( jsonResponse(200, { diff --git a/frontend/tests/unit/core/skills/export.test.ts b/frontend/tests/unit/core/skills/export.test.ts index a509f844b4..080b65d13e 100644 --- a/frontend/tests/unit/core/skills/export.test.ts +++ b/frontend/tests/unit/core/skills/export.test.ts @@ -12,6 +12,31 @@ import { describe("custom skill export requests", () => { beforeEach(() => request.mockReset()); + for (const body of [ + null, + { detail: null }, + { detail: { code: [], message: 123 } }, + ]) { + for (const operation of ["preview", "download"]) { + it(`keeps a typed ${operation} failure for ${JSON.stringify(body)}`, async () => { + request.mockResolvedValue( + new Response(JSON.stringify(body), { status: 503 }), + ); + const signal = new AbortController().signal; + const result = + operation === "preview" + ? loadSkillExportManifest("demo", signal) + : downloadSkillExport("demo", "revision", signal); + await expect(result).rejects.toMatchObject({ + name: "SkillExportRequestError", + status: 503, + code: "skill_export_failed", + message: "Could not export this skill.", + }); + expect(request).toHaveBeenCalledTimes(1); + }); + } + } it("binds download to the exact preview and forwards cancellation", async () => { request.mockResolvedValue( new Response("zip", { headers: { "content-type": "application/zip" } }),