fix(skills): avoid false invalid zip uploads (#893)

internal/http/skills_upload.go

@@ -54,17 +54,27 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) {
 		writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to create temp file")})
 		return
 	}
-	defer os.Remove(tmp.Name())
-	defer tmp.Close()
+	tmpName := tmp.Name()
+	defer os.Remove(tmpName)
 
 	size, err := io.Copy(tmp, file)
 	if err != nil {
+		tmp.Close()
 		writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to save upload")})
 		return
 	}
+	if err := tmp.Sync(); err != nil {
+		tmp.Close()
+		writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to finalize upload")})
+		return
+	}
+	if err := tmp.Close(); err != nil {
+		writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to finalize upload")})
+		return
+	}
 
 	// Open as zip
-	zr, err := zip.OpenReader(tmp.Name())
+	zr, err := zip.OpenReader(tmpName)
 	if err != nil {
 		writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidRequest, "invalid ZIP file")})
 		return

ui/web/src/pages/skills/lib/resolve-upload-skills.test.ts

@@ -0,0 +1,39 @@
+import { describe, expect, it, vi } from "vitest";
+
+import { resolveUploadSkills } from "./resolve-upload-skills";
+import type { MultiSkillZipValidation } from "./validate-skill-zip";
+
+function makeZipFile(name = "skill.zip"): File {
+  return new File([new Uint8Array([0x50, 0x4b, 0x03, 0x04])], name, {
+    type: "application/zip",
+  });
+}
+
+describe("resolveUploadSkills", () => {
+  it("falls back to direct upload when browser ZIP parsing reports invalidZip", async () => {
+    const validateArchive = vi.fn<(_: File) => Promise<MultiSkillZipValidation>>()
+      .mockResolvedValue({ skills: [], error: "upload.invalidZip" });
+
+    const result = await resolveUploadSkills(makeZipFile(), validateArchive);
+
+    expect(result).toEqual([{ dir: "", status: "valid" }]);
+  });
+
+  it("falls back to direct upload when browser ZIP parsing throws", async () => {
+    const validateArchive = vi.fn<(_: File) => Promise<MultiSkillZipValidation>>()
+      .mockRejectedValue(new Error("unsupported zip variant"));
+
+    const result = await resolveUploadSkills(makeZipFile(), validateArchive);
+
+    expect(result).toEqual([{ dir: "", status: "valid" }]);
+  });
+
+  it("keeps real validation errors blocking", async () => {
+    const validateArchive = vi.fn<(_: File) => Promise<MultiSkillZipValidation>>()
+      .mockResolvedValue({ skills: [], error: "upload.onlyZip" });
+
+    const result = await resolveUploadSkills(makeZipFile("skill.txt"), validateArchive);
+
+    expect(result).toEqual([{ dir: "", status: "invalid", error: "upload.onlyZip" }]);
+  });
+});

ui/web/src/pages/skills/lib/resolve-upload-skills.ts

@@ -0,0 +1,45 @@
+import { validateMultiSkillZip, type MultiSkillZipValidation } from "./validate-skill-zip";
+import type { SkillEntry, SkillStatus } from "./skill-upload-types";
+
+type SkillEntrySeed = Omit<SkillEntry, "id">;
+
+type ValidateSkillArchive = (file: File) => Promise<MultiSkillZipValidation>;
+
+// Browser-side ZIP parsing is best-effort only. Some valid ZIP variants are
+// accepted by the backend but rejected by JSZip, so fall back to a direct
+// single-file upload instead of blocking the user locally.
+export async function resolveUploadSkills(
+  file: File,
+  validateArchive: ValidateSkillArchive = validateMultiSkillZip,
+): Promise<SkillEntrySeed[]> {
+  try {
+    const validation = await validateArchive(file);
+    if (validation.error === "upload.invalidZip") {
+      return [fallbackUploadSkill()];
+    }
+    if (validation.error) {
+      return [invalidSkill(validation.error)];
+    }
+    if (validation.skills.length === 0) {
+      return [invalidSkill("upload.noSkillMd")];
+    }
+    return validation.skills.map((skill) => ({
+      dir: skill.dir,
+      status: skill.valid ? ("valid" as SkillStatus) : ("invalid" as SkillStatus),
+      name: skill.name,
+      slug: skill.slug,
+      contentHash: skill.contentHash,
+      error: skill.error,
+    }));
+  } catch {
+    return [fallbackUploadSkill()];
+  }
+}
+
+function fallbackUploadSkill(): SkillEntrySeed {
+  return { dir: "", status: "valid" };
+}
+
+function invalidSkill(error: string): SkillEntrySeed {
+  return { dir: "", status: "invalid", error };
+}

ui/web/src/pages/skills/skill-upload-dialog.tsx

@@ -10,8 +10,8 @@ import {
   DialogTitle,
 } from "@/components/ui/dialog";
 import { Button } from "@/components/ui/button";
-import { validateMultiSkillZip } from "./lib/validate-skill-zip";
 import { createSkillSubZip } from "./lib/create-skill-sub-zip";
+import { resolveUploadSkills } from "./lib/resolve-upload-skills";
 import { uniqueId } from "@/lib/utils";
 import type { SkillUploadResponse } from "./hooks/use-skills";
 import type { FileEntry, SkillStatus } from "./lib/skill-upload-types";
@@ -54,39 +54,14 @@ export function SkillUploadDialog({ open, onOpenChange, onUpload }: SkillUploadD
     // Validate all files concurrently
     const results = await Promise.all(
       pending.map(async (entry) => {
-        try {
-          const validation = await validateMultiSkillZip(entry.file);
-          const placeholderId = entry.skills[0]?.id ?? uniqueId();
-          if (validation.error) {
-            return {
-              id: entry.id,
-              skills: [{ id: placeholderId, dir: "", status: "invalid" as SkillStatus, error: validation.error }],
-            };
-          }
-          if (validation.skills.length === 0) {
-            return {
-              id: entry.id,
-              skills: [{ id: placeholderId, dir: "", status: "invalid" as SkillStatus, error: "upload.noSkillMd" }],
-            };
-          }
-          return {
-            id: entry.id,
-            skills: validation.skills.map((s) => ({
-              id: uniqueId(),
-              dir: s.dir,
-              status: s.valid ? ("valid" as SkillStatus) : ("invalid" as SkillStatus),
-              name: s.name,
-              slug: s.slug,
-              contentHash: s.contentHash,
-              error: s.error,
-            })),
-          };
-        } catch {
-          return {
-            id: entry.id,
-            skills: [{ id: entry.skills[0]?.id ?? uniqueId(), dir: "", status: "invalid" as SkillStatus, error: "upload.invalidZip" }],
-          };
-        }
+        const resolved = await resolveUploadSkills(entry.file);
+        return {
+          id: entry.id,
+          skills: resolved.map((skill) => ({
+            id: uniqueId(),
+            ...skill,
+          })),
+        };
       }),
     );