From 094ebd4deb584f869c12f0e5a772f95a7014d0e0 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sat, 5 Sep 2026 05:16:17 -0700 Subject: [PATCH 1/4] fix(import): delete GitHub skill blobs when import fails importGitHubSkillForUser stored Convex blobs before validating slug/owner/semver, and publish failures left those ids unreferenced. Signed-off-by: Sebastien Tardif --- convex/githubImport.test.ts | 168 ++++++++++++++++++++++++++++++++++++ convex/githubImport.ts | 152 ++++++++++++++++---------------- 2 files changed, 247 insertions(+), 73 deletions(-) diff --git a/convex/githubImport.test.ts b/convex/githubImport.test.ts index 7fe977925f..62a947fd26 100644 --- a/convex/githubImport.test.ts +++ b/convex/githubImport.test.ts @@ -4,6 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { internal } from "./_generated/api"; import { __test } from "./githubImport"; import { buildGitHubZipForTests } from "./lib/githubImport"; +import { publishVersionForUser } from "./lib/skillPublish"; vi.mock("./_generated/api", () => ({ internal: { @@ -13,9 +14,113 @@ vi.mock("./_generated/api", () => ({ skills: { getSkillBySlugInternal: Symbol("getSkillBySlugInternal"), }, + publishers: { + resolvePublishTargetForUserInternal: Symbol("resolvePublishTargetForUserInternal"), + }, }, })); +vi.mock("./lib/skillPublish", () => ({ + publishVersionForUser: vi.fn(), +})); + +const IMPORT_COMMIT = "a".repeat(40); +const IMPORT_OWNER = "vyctorbrzezowski"; +const IMPORT_REPO = "public-skill"; + +function requestUrl(input: RequestInfo | URL): string { + if (typeof input === "string") return input; + if (input instanceof URL) return input.toString(); + return input.url; +} + +function buildOwnedImportZip( + entries: Record = { + [`${IMPORT_REPO}/SKILL.md`]: "# Demo skill\n", + [`${IMPORT_REPO}/notes.md`]: "notes\n", + }, +) { + return buildGitHubZipForTests(entries); +} + +function makeOwnedImportFetch(zip: Uint8Array) { + return vi.fn(async (input: RequestInfo | URL) => { + const url = requestUrl(input); + if (url === `https://api.github.com/user/123`) { + return { + ok: true, + json: async () => ({ + id: 123, + login: IMPORT_OWNER, + avatar_url: "https://avatars.githubusercontent.com/u/123?v=4", + }), + }; + } + if (url === `https://api.github.com/repos/${IMPORT_OWNER}/${IMPORT_REPO}`) { + return { + ok: true, + json: async () => ({ + name: IMPORT_REPO, + full_name: `${IMPORT_OWNER}/${IMPORT_REPO}`, + private: false, + visibility: "public", + owner: { id: 123, login: IMPORT_OWNER }, + archived: false, + disabled: false, + fork: false, + }), + }; + } + if (url === `https://api.github.com/repos/${IMPORT_OWNER}/${IMPORT_REPO}/commits/main`) { + return { + ok: true, + json: async () => ({ sha: IMPORT_COMMIT }), + }; + } + if (url === `https://codeload.github.com/${IMPORT_OWNER}/${IMPORT_REPO}/zip/${IMPORT_COMMIT}`) { + return { + ok: true, + headers: { get: () => null }, + arrayBuffer: async () => zip.buffer.slice(zip.byteOffset, zip.byteOffset + zip.byteLength), + }; + } + throw new Error(`unexpected fetch: ${url}`); + }); +} + +function makeImportCtx(overrides?: { + store?: ReturnType; + delete?: ReturnType; +}) { + let nextStorageId = 1; + const store = overrides?.store ?? vi.fn(async () => `storage:${nextStorageId++}` as const); + const del = overrides?.delete ?? vi.fn(async () => undefined); + return { + runQuery: vi.fn().mockResolvedValue("123"), + runMutation: vi.fn().mockResolvedValue({ publisherId: "publishers:1" }), + storage: { + store, + delete: del, + }, + }; +} + +function makeImportArgs(overrides: Record = {}) { + return { + url: `https://github.com/${IMPORT_OWNER}/${IMPORT_REPO}/tree/main`, + commit: IMPORT_COMMIT, + candidatePath: "", + selectedPaths: ["SKILL.md", "notes.md"], + slug: "public-skill", + ownerHandle: IMPORT_OWNER, + displayName: "Public Skill", + version: "1.0.0", + tags: ["latest"], + acceptLicenseTerms: true, + ...overrides, + }; +} + const originalGitHubToken = process.env.GITHUB_TOKEN; const originalGitHubAppEnv = { appId: process.env.GITHUB_APP_ID, @@ -32,6 +137,7 @@ describe("githubImport", () => { }); afterEach(() => { + vi.mocked(publishVersionForUser).mockReset(); if (originalGitHubToken) { process.env.GITHUB_TOKEN = originalGitHubToken; } else { @@ -728,4 +834,66 @@ describe("githubImport", () => { expect.objectContaining({ headers: expect.any(Object) }), ); }); + + it("validates import metadata before storing Convex blobs", async () => { + const ctx = makeImportCtx(); + const fetchMock = makeOwnedImportFetch(buildOwnedImportZip()); + + await expect( + __test.importGitHubSkillForUser( + ctx as never, + "users:1" as never, + makeImportArgs({ version: "not-semver" }), + fetchMock as never, + ), + ).rejects.toThrow(/Version must be valid semver/); + + expect(ctx.storage.store).not.toHaveBeenCalled(); + expect(ctx.storage.delete).not.toHaveBeenCalled(); + expect(publishVersionForUser).not.toHaveBeenCalled(); + }); + + it("deletes stored blobs when publishVersionForUser fails after a successful store", async () => { + const ctx = makeImportCtx(); + const fetchMock = makeOwnedImportFetch(buildOwnedImportZip()); + vi.mocked(publishVersionForUser).mockRejectedValueOnce(new Error("slug exists")); + + await expect( + __test.importGitHubSkillForUser( + ctx as never, + "users:1" as never, + makeImportArgs(), + fetchMock as never, + ), + ).rejects.toThrow(/Import failed during publish: slug exists/); + + expect(ctx.storage.store).toHaveBeenCalledTimes(2); + expect(ctx.storage.delete).toHaveBeenCalledTimes(2); + expect(ctx.storage.delete).toHaveBeenCalledWith("storage:1"); + expect(ctx.storage.delete).toHaveBeenCalledWith("storage:2"); + }); + + it("deletes already-stored blobs when a later store call fails", async () => { + const store = vi + .fn() + .mockResolvedValueOnce("storage:1") + .mockRejectedValueOnce(new Error("disk full")); + const del = vi.fn(async () => undefined); + const ctx = makeImportCtx({ store, delete: del }); + const fetchMock = makeOwnedImportFetch(buildOwnedImportZip()); + + await expect( + __test.importGitHubSkillForUser( + ctx as never, + "users:1" as never, + makeImportArgs(), + fetchMock as never, + ), + ).rejects.toThrow(/Failed to store file "notes.md" \(6 bytes\)\. disk full/); + + expect(store).toHaveBeenCalledTimes(2); + expect(del).toHaveBeenCalledTimes(1); + expect(del).toHaveBeenCalledWith("storage:1"); + expect(publishVersionForUser).not.toHaveBeenCalled(); + }); }); diff --git a/convex/githubImport.ts b/convex/githubImport.ts index dc534d1e67..fa9d27108d 100644 --- a/convex/githubImport.ts +++ b/convex/githubImport.ts @@ -305,50 +305,6 @@ async function importGitHubSkillForUser( throw new ConvexError("The skill file must be selected"); } - let totalBytes = 0; - const storedFiles: Array<{ - path: string; - size: number; - storageId: Id<"_storage">; - sha256: string; - contentType?: string; - }> = []; - - for (const path of selected.sort()) { - if (candidateRoot && !path.startsWith(candidateRoot)) { - throw new ConvexError("Selected file is outside the chosen skill folder"); - } - - const bytes = byPath.get(path); - if (!bytes) continue; - totalBytes += bytes.byteLength; - if (totalBytes > MAX_SELECTED_BYTES) throw new ConvexError("Selected files exceed 50MB limit"); - - const relPath = candidateRoot ? path.slice(candidateRoot.length) : path; - const sanitized = sanitizePath(relPath); - if (!sanitized) throw new ConvexError("Invalid file paths"); - - const sha256 = await sha256Hex(bytes); - const safeBytes = new Uint8Array(bytes); - let storageId: Id<"_storage">; - try { - storageId = await ctx.storage.store( - new Blob([safeBytes], { type: "application/octet-stream" }), - ); - } catch (error) { - throw new ConvexError(buildStoreFailureMessage(sanitized, bytes.byteLength, error)); - } - storedFiles.push({ - path: sanitized, - size: bytes.byteLength, - storageId, - sha256, - contentType: "application/octet-stream", - }); - } - - if (storedFiles.length === 0) throw new ConvexError("No files selected"); - const slugBase = (args.slug ?? "").trim().toLowerCase(); const displayName = (args.displayName ?? "").trim(); const tags = (args.tags ?? ["latest"]).map((tag) => tag.trim()).filter(Boolean); @@ -366,39 +322,89 @@ async function importGitHubSkillForUser( minimumRole: "publisher", })) as { publisherId: Id<"publishers"> }; - const sourceProvenance = { - kind: "github" as const, - url: resolved.originalUrl, - repo: `${resolved.owner}/${resolved.repo}`, - ref: resolved.ref, - commit: resolved.commit, - path: candidate.path, - importedAt: Date.now(), - }; + const storedFiles: Array<{ + path: string; + size: number; + storageId: Id<"_storage">; + sha256: string; + contentType?: string; + }> = []; - let result: Awaited>; try { - result = await publishVersionForUser( - ctx, - userId, - { - slug: slugBase, - displayName, - version, - changelog: "", - tags, - categories: args.categories, - topics: args.topics, - files: storedFiles, - source: sourceProvenance, - }, - { ownerPublisherId: target.publisherId, sourceProvenance }, - ); + let totalBytes = 0; + for (const path of selected.sort()) { + if (candidateRoot && !path.startsWith(candidateRoot)) { + throw new ConvexError("Selected file is outside the chosen skill folder"); + } + + const bytes = byPath.get(path); + if (!bytes) continue; + totalBytes += bytes.byteLength; + if (totalBytes > MAX_SELECTED_BYTES) + throw new ConvexError("Selected files exceed 50MB limit"); + + const relPath = candidateRoot ? path.slice(candidateRoot.length) : path; + const sanitized = sanitizePath(relPath); + if (!sanitized) throw new ConvexError("Invalid file paths"); + + const sha256 = await sha256Hex(bytes); + const safeBytes = new Uint8Array(bytes); + let storageId: Id<"_storage">; + try { + storageId = await ctx.storage.store( + new Blob([safeBytes], { type: "application/octet-stream" }), + ); + } catch (error) { + throw new ConvexError(buildStoreFailureMessage(sanitized, bytes.byteLength, error)); + } + storedFiles.push({ + path: sanitized, + size: bytes.byteLength, + storageId, + sha256, + contentType: "application/octet-stream", + }); + } + + if (storedFiles.length === 0) throw new ConvexError("No files selected"); + + const sourceProvenance = { + kind: "github" as const, + url: resolved.originalUrl, + repo: `${resolved.owner}/${resolved.repo}`, + ref: resolved.ref, + commit: resolved.commit, + path: candidate.path, + importedAt: Date.now(), + }; + + let result: Awaited>; + try { + result = await publishVersionForUser( + ctx, + userId, + { + slug: slugBase, + displayName, + version, + changelog: "", + tags, + categories: args.categories, + topics: args.topics, + files: storedFiles, + source: sourceProvenance, + }, + { ownerPublisherId: target.publisherId, sourceProvenance }, + ); + } catch (error) { + throw new ConvexError(buildPublishFailureMessage(error)); + } + + return { ok: true, slug: slugBase, version, ...result }; } catch (error) { - throw new ConvexError(buildPublishFailureMessage(error)); + await Promise.allSettled(storedFiles.map((file) => ctx.storage.delete(file.storageId))); + throw error; } - - return { ok: true, slug: slugBase, version, ...result }; } async function listOwnedPublicGitHubReposForUser( From d4bcf53fd9278badbe4010076452ffbc9ed01b37 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 7 Sep 2026 12:12:43 -0700 Subject: [PATCH 2/4] fix: delete unpublished skill blobs when publish fails Multipart POST /api/v1/skills stored each uploaded file, then parsed the payload. An invalid body, a later oversized file, or a 400 after parse (license reject, owner resolution) left Convex blobs unreferenced. Match parseMultipartSkillScan: reject oversized parts before store, delete stored ids if store or parse fails, and delete them when the handler returns 400 after a successful parse. Replayed onto upstream/main d3bde70e. Signed-off-by: Sebastien Tardif --- convex/httpApiV1.handlers.test.ts | 46 +++++++++++++++- convex/httpApiV1.shared.test.ts | 72 +++++++++++++++++++++++++ convex/httpApiV1/shared.ts | 90 ++++++++++++++++++------------- convex/httpApiV1/skillsV1.ts | 17 ++++-- 4 files changed, 184 insertions(+), 41 deletions(-) diff --git a/convex/httpApiV1.handlers.test.ts b/convex/httpApiV1.handlers.test.ts index 088304e621..e6be74e035 100644 --- a/convex/httpApiV1.handlers.test.ts +++ b/convex/httpApiV1.handlers.test.ts @@ -8277,6 +8277,46 @@ describe("httpApiV1 handlers", () => { expect(publishVersionForUser).not.toHaveBeenCalled(); }); + it("publish multipart deletes stored blobs when owner resolution fails", async () => { + vi.mocked(requireApiTokenUser).mockResolvedValueOnce({ + userId: "users:1", + user: { handle: "p" }, + } as never); + const runMutation = vi.fn(async (_mutation: unknown, args: Record) => { + if (isRateLimitArgs(args)) return okRate(); + throw new Error("Publisher not found"); + }); + const form = new FormData(); + form.set( + "payload", + JSON.stringify({ + slug: "demo", + displayName: "Demo", + ownerHandle: "@missing", + version: "1.0.0", + changelog: "", + acceptLicenseTerms: true, + tags: ["latest"], + }), + ); + form.append("files", new Blob(["hello"], { type: "text/plain" }), "SKILL.md"); + const store = vi.fn().mockResolvedValue("storage:1"); + const remove = vi.fn().mockResolvedValue(undefined); + const response = await __handlers.publishSkillV1Handler( + makeCtx({ runMutation, storage: { store, delete: remove } }), + new Request("https://example.com/api/v1/skills", { + method: "POST", + headers: { Authorization: "Bearer clh_test" }, + body: form, + }), + ); + expect(response.status).toBe(400); + expect(await response.text()).toMatch(/publisher not found/i); + expect(publishVersionForUser).not.toHaveBeenCalled(); + expect(store).toHaveBeenCalledTimes(1); + expect(remove).toHaveBeenCalledWith("storage:1"); + }); + it("publish json rejects omitted license terms", async () => { vi.mocked(requireApiTokenUser).mockResolvedValueOnce({ userId: "users:1", @@ -8455,8 +8495,10 @@ describe("httpApiV1 handlers", () => { }), ); form.append("files", new Blob(["hello"], { type: "text/plain" }), "SKILL.md"); + const store = vi.fn().mockResolvedValue("storage:1"); + const remove = vi.fn().mockResolvedValue(undefined); const response = await __handlers.publishSkillV1Handler( - makeCtx({ runMutation, storage: { store: vi.fn().mockResolvedValue("storage:1") } }), + makeCtx({ runMutation, storage: { store, delete: remove } }), new Request("https://example.com/api/v1/skills", { method: "POST", headers: { Authorization: "Bearer clh_test" }, @@ -8466,6 +8508,8 @@ describe("httpApiV1 handlers", () => { expect(response.status).toBe(400); expect(await response.text()).toMatch(/license terms must be accepted/i); expect(publishVersionForUser).not.toHaveBeenCalled(); + expect(store).toHaveBeenCalledTimes(1); + expect(remove).toHaveBeenCalledWith("storage:1"); }); it("publish rejects explicit license refusal", async () => { diff --git a/convex/httpApiV1.shared.test.ts b/convex/httpApiV1.shared.test.ts index bc015d418f..57623ff597 100644 --- a/convex/httpApiV1.shared.test.ts +++ b/convex/httpApiV1.shared.test.ts @@ -5,10 +5,12 @@ import type { Id } from "./_generated/dataModel"; import type { ActionCtx } from "./_generated/server"; import { formatUserFacingErrorMessage, + parseMultipartPublish, parseMultipartSkillScan, resolveTagsBatch, softDeleteErrorToResponse, } from "./httpApiV1/shared"; +import { MAX_PUBLISH_FILE_BYTES } from "./lib/publishLimits"; function makeCtx() { return { @@ -161,4 +163,74 @@ describe("http API v1 shared helpers", () => { ).rejects.toThrow("update is not valid for uploaded scans"); expect(store).not.toHaveBeenCalled(); }); + + it("deletes stored publish blobs when the payload fails after upload", async () => { + const form = new FormData(); + form.set( + "payload", + JSON.stringify({ + displayName: "Demo", + version: "1.0.0", + changelog: "", + tags: ["latest"], + }), + ); + form.append("files", new Blob(["# Demo"], { type: "text/markdown" }), "SKILL.md"); + const request = new Request("https://clawhub.ai/api/v1/skills", { + method: "POST", + body: form, + }); + const store = vi.fn().mockResolvedValue("storage:1"); + const remove = vi.fn().mockResolvedValue(undefined); + const ctx = { + storage: { + store, + delete: remove, + }, + } as unknown as ActionCtx; + + await expect(parseMultipartPublish(ctx, request)).rejects.toThrow(/slug/i); + expect(store).toHaveBeenCalledTimes(1); + expect(remove).toHaveBeenCalledWith("storage:1"); + }); + + it("does not store publish files when a later part exceeds the size limit", async () => { + const form = new FormData(); + form.set( + "payload", + JSON.stringify({ + slug: "demo", + displayName: "Demo", + version: "1.0.0", + changelog: "", + acceptLicenseTerms: true, + tags: ["latest"], + }), + ); + form.append("files", new Blob(["# Demo"], { type: "text/markdown" }), "SKILL.md"); + form.append( + "files", + new File([new Uint8Array(MAX_PUBLISH_FILE_BYTES + 1)], "big.bin", { + type: "application/octet-stream", + }), + ); + const request = new Request("https://clawhub.ai/api/v1/skills", { + method: "POST", + body: form, + }); + const store = vi.fn().mockResolvedValue("storage:1"); + const remove = vi.fn().mockResolvedValue(undefined); + const ctx = { + storage: { + store, + delete: remove, + }, + } as unknown as ActionCtx; + + await expect(parseMultipartPublish(ctx, request)).rejects.toThrow( + 'File "big.bin" exceeds 10MB limit', + ); + expect(store).not.toHaveBeenCalled(); + expect(remove).not.toHaveBeenCalled(); + }); }); diff --git a/convex/httpApiV1/shared.ts b/convex/httpApiV1/shared.ts index 75ca88db06..46d6942041 100644 --- a/convex/httpApiV1/shared.ts +++ b/convex/httpApiV1/shared.ts @@ -386,6 +386,13 @@ function toFileLike(entry: FormDataEntryValue): FileLikeEntry | null { return entry as FileLikeEntry; } +export async function deleteStoredMultipartFiles( + ctx: ActionCtx, + files: Array<{ storageId: Id<"_storage"> }>, +) { + await Promise.allSettled(files.map((file) => ctx.storage.delete(file.storageId))); +} + export async function parseMultipartPublish( ctx: ActionCtx, request: Request, @@ -402,6 +409,14 @@ export async function parseMultipartPublish( throw new Error("Invalid JSON payload"); } + const fileEntries = form + .getAll("files") + .map((entry) => toFileLike(entry)) + .filter((file): file is FileLikeEntry => Boolean(file)) + .filter((file) => !isMacJunkPath(file.name)); + const oversized = fileEntries.find((file) => file.size > MAX_PUBLISH_FILE_BYTES); + if (oversized) throw new Error(getPublishFileSizeError(oversized.name)); + const files: Array<{ path: string; size: number; @@ -410,44 +425,47 @@ export async function parseMultipartPublish( contentType?: string; }> = []; - for (const entry of form.getAll("files")) { - const file = toFileLike(entry); - if (!file) continue; - const path = file.name; - if (isMacJunkPath(path)) continue; - const size = file.size; - if (size > MAX_PUBLISH_FILE_BYTES) { - throw new Error(getPublishFileSizeError(path)); + try { + for (const file of fileEntries) { + const path = file.name; + const size = file.size; + const contentType = file.type || undefined; + const buffer = new Uint8Array(await file.arrayBuffer()); + const sha256 = await sha256Hex(buffer); + const storageId = await ctx.storage.store(file as Blob); + files.push({ path, size, storageId, sha256, contentType }); } - const contentType = file.type || undefined; - const buffer = new Uint8Array(await file.arrayBuffer()); - const sha256 = await sha256Hex(buffer); - const storageId = await ctx.storage.store(file as Blob); - files.push({ path, size, storageId, sha256, contentType }); - } - const forkOf = payload.forkOf && typeof payload.forkOf === "object" ? payload.forkOf : undefined; - const hasAcceptLicenseTerms = Object.prototype.hasOwnProperty.call(payload, "acceptLicenseTerms"); - const body = { - slug: payload.slug, - displayName: payload.displayName, - ...(typeof payload.ownerHandle === "string" ? { ownerHandle: payload.ownerHandle } : {}), - ...(typeof payload.sourceOwnerHandle === "string" - ? { sourceOwnerHandle: payload.sourceOwnerHandle } - : {}), - ...(typeof payload.migrateOwner === "boolean" ? { migrateOwner: payload.migrateOwner } : {}), - version: payload.version, - changelog: typeof payload.changelog === "string" ? payload.changelog : "", - ...(hasAcceptLicenseTerms ? { acceptLicenseTerms: payload.acceptLicenseTerms } : {}), - tags: Array.isArray(payload.tags) ? payload.tags : undefined, - ...(Array.isArray(payload.categories) ? { categories: payload.categories } : {}), - ...(Array.isArray(payload.topics) ? { topics: payload.topics } : {}), - ...(payload.source ? { source: payload.source } : {}), - files, - ...(forkOf ? { forkOf } : {}), - }; + const forkOf = + payload.forkOf && typeof payload.forkOf === "object" ? payload.forkOf : undefined; + const hasAcceptLicenseTerms = Object.prototype.hasOwnProperty.call( + payload, + "acceptLicenseTerms", + ); + const body = { + slug: payload.slug, + displayName: payload.displayName, + ...(typeof payload.ownerHandle === "string" ? { ownerHandle: payload.ownerHandle } : {}), + ...(typeof payload.sourceOwnerHandle === "string" + ? { sourceOwnerHandle: payload.sourceOwnerHandle } + : {}), + ...(typeof payload.migrateOwner === "boolean" ? { migrateOwner: payload.migrateOwner } : {}), + version: payload.version, + changelog: typeof payload.changelog === "string" ? payload.changelog : "", + ...(hasAcceptLicenseTerms ? { acceptLicenseTerms: payload.acceptLicenseTerms } : {}), + tags: Array.isArray(payload.tags) ? payload.tags : undefined, + ...(Array.isArray(payload.categories) ? { categories: payload.categories } : {}), + ...(Array.isArray(payload.topics) ? { topics: payload.topics } : {}), + ...(payload.source ? { source: payload.source } : {}), + files, + ...(forkOf ? { forkOf } : {}), + }; - return parsePublishBody(body); + return parsePublishBody(body); + } catch (error) { + await deleteStoredMultipartFiles(ctx, files); + throw error; + } } export async function parseMultipartSkillScan( @@ -508,7 +526,7 @@ export async function parseMultipartSkillScan( files.push({ path, size, storageId, sha256, contentType }); } } catch (error) { - await Promise.allSettled(files.map((file) => ctx.storage.delete(file.storageId))); + await deleteStoredMultipartFiles(ctx, files); throw error; } diff --git a/convex/httpApiV1/skillsV1.ts b/convex/httpApiV1/skillsV1.ts index 01ef15b7e5..c507ae36da 100644 --- a/convex/httpApiV1/skillsV1.ts +++ b/convex/httpApiV1/skillsV1.ts @@ -82,6 +82,7 @@ import { MAX_RAW_FILE_BYTES, type AmbiguousSkillSlugChoice, ambiguousSkillSlugResponse, + deleteStoredMultipartFiles, formatAuthzMessage, formatUserFacingErrorMessage, getPathSegments, @@ -2661,11 +2662,19 @@ export async function publishSkillV1Handler(ctx: ActionCtx, request: Request) { if (contentType.includes("multipart/form-data")) { const payload = await parseMultipartPublish(ctx, request); - if (!hasAcceptedLegacyLicenseTerms(payload.acceptLicenseTerms)) { - return text("MIT-0 license terms must be accepted to publish skills", 400, rate.headers); + let keepStoredFiles = false; + try { + if (!hasAcceptedLegacyLicenseTerms(payload.acceptLicenseTerms)) { + return text("MIT-0 license terms must be accepted to publish skills", 400, rate.headers); + } + const result = await publishSkillPayloadForApiUser(ctx, auth.userId, payload); + keepStoredFiles = true; + return json({ ok: true, ...result }, 200, rate.headers); + } finally { + if (!keepStoredFiles) { + await deleteStoredMultipartFiles(ctx, payload.files); + } } - const result = await publishSkillPayloadForApiUser(ctx, auth.userId, payload); - return json({ ok: true, ...result }, 200, rate.headers); } } catch (error) { const message = error instanceof Error ? error.message : "Publish failed"; From 9d2945bfeadf8a2c60568d3634fdf7a8100e7118 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Tue, 15 Sep 2026 22:14:37 -0700 Subject: [PATCH 3/4] fix: preserve multipart skill files after persistence --- convex/httpApiV1.handlers.test.ts | 79 ++++++++++++++++++++++++------- convex/httpApiV1/skillsV1.ts | 7 ++- convex/lib/skillPublish.test.ts | 72 ++++++++++++++++++++++++++++ convex/lib/skillPublish.ts | 5 ++ specs/spec.md | 9 ++++ 5 files changed, 154 insertions(+), 18 deletions(-) diff --git a/convex/httpApiV1.handlers.test.ts b/convex/httpApiV1.handlers.test.ts index ce78c80ff3..4719fa4d22 100644 --- a/convex/httpApiV1.handlers.test.ts +++ b/convex/httpApiV1.handlers.test.ts @@ -8555,11 +8555,12 @@ describe("httpApiV1 handlers", () => { userId: "users:1", user: { handle: "p" }, } as never); - vi.mocked(publishVersionForUser).mockResolvedValueOnce({ - skillId: "s", - versionId: "v", - embeddingId: "e", - } as never); + vi.mocked(publishVersionForUser).mockImplementationOnce( + async (_ctx, _userId, _args, options) => { + options?.onFilesPersisted?.(); + return { skillId: "s", versionId: "v", embeddingId: "e" } as never; + }, + ); const runMutation = vi.fn(async (_mutation: unknown, args: Record) => { if (isRateLimitArgs(args)) return okRate(); if (args.ownerHandle === "me") return { publisherId: "publishers:me" }; @@ -8627,11 +8628,12 @@ describe("httpApiV1 handlers", () => { userId: "users:1", user: { handle: "p" }, } as never); - vi.mocked(publishVersionForUser).mockResolvedValueOnce({ - skillId: "s", - versionId: "v", - embeddingId: "e", - } as never); + vi.mocked(publishVersionForUser).mockImplementationOnce( + async (_ctx, _userId, _args, options) => { + options?.onFilesPersisted?.(); + return { skillId: "s", versionId: "v", embeddingId: "e" } as never; + }, + ); const runMutation = vi.fn(async (_mutation: unknown, args: Record) => { if (isRateLimitArgs(args)) return okRate(); if (args.ownerHandle === "openclaw") return { publisherId: "publishers:openclaw" }; @@ -8665,10 +8667,54 @@ describe("httpApiV1 handlers", () => { expect.anything(), "users:1", expect.not.objectContaining({ ownerHandle: expect.anything() }), - { ownerPublisherId: "publishers:openclaw" }, + { ownerPublisherId: "publishers:openclaw", onFilesPersisted: expect.any(Function) }, ); }); + it.each([false, true])( + "multipart cleanup respects persisted files after failure (%s)", + async (persisted) => { + vi.mocked(requireApiTokenUser).mockResolvedValueOnce({ + userId: "users:1", + user: { handle: "p" }, + } as never); + vi.mocked(publishVersionForUser).mockImplementationOnce( + async (_ctx, _userId, _args, options) => { + if (persisted) options?.onFilesPersisted?.(); + throw new Error(persisted ? "followup failed" : "insert rejected"); + }, + ); + const remove = vi.fn(); + const form = new FormData(); + form.set( + "payload", + JSON.stringify({ + slug: "demo", + displayName: "Demo", + version: "1.0.0", + acceptLicenseTerms: true, + tags: ["latest"], + }), + ); + form.append("files", new Blob(["# Demo\n"]), "SKILL.md"); + const response = await __handlers.publishSkillV1Handler( + makeCtx({ + runMutation: vi.fn().mockResolvedValue(okRate()), + storage: { store: vi.fn().mockResolvedValue("storage:request"), delete: remove }, + }), + new Request("https://example.com/api/v1/skills", { + method: "POST", + headers: { Authorization: "Bearer clh_test" }, + body: form, + }), + ); + expect(response.status).toBe(400); + expect(await response.text()).toContain(persisted ? "followup failed" : "insert rejected"); + if (persisted) expect(remove).not.toHaveBeenCalled(); + else expect(remove).toHaveBeenCalledExactlyOnceWith("storage:request"); + }, + ); + it("publish multipart rejects omitted license terms", async () => { vi.mocked(requireApiTokenUser).mockResolvedValueOnce({ userId: "users:1", @@ -8753,11 +8799,12 @@ describe("httpApiV1 handlers", () => { userId: "users:1", user: { handle: "p" }, } as never); - vi.mocked(publishVersionForUser).mockResolvedValueOnce({ - skillId: "s", - versionId: "v", - embeddingId: "e", - } as never); + vi.mocked(publishVersionForUser).mockImplementationOnce( + async (_ctx, _userId, _args, options) => { + options?.onFilesPersisted?.(); + return { skillId: "s", versionId: "v", embeddingId: "e" } as never; + }, + ); const runMutation = vi.fn().mockResolvedValue(okRate()); const store = vi.fn().mockResolvedValue("storage:1"); const form = new FormData(); diff --git a/convex/httpApiV1/skillsV1.ts b/convex/httpApiV1/skillsV1.ts index 5ac68f4197..4b6acafa0d 100644 --- a/convex/httpApiV1/skillsV1.ts +++ b/convex/httpApiV1/skillsV1.ts @@ -2679,8 +2679,9 @@ export async function publishSkillV1Handler(ctx: ActionCtx, request: Request) { if (!hasAcceptedLegacyLicenseTerms(payload.acceptLicenseTerms)) { return text("MIT-0 license terms must be accepted to publish skills", 400, rate.headers); } - const result = await publishSkillPayloadForApiUser(ctx, auth.userId, payload); - keepStoredFiles = true; + const result = await publishSkillPayloadForApiUser(ctx, auth.userId, payload, () => { + keepStoredFiles = true; + }); return json({ ok: true, ...result }, 200, rate.headers); } finally { if (!keepStoredFiles) { @@ -2700,6 +2701,7 @@ async function publishSkillPayloadForApiUser( ctx: ActionCtx, userId: Id<"users">, payload: ReturnType, + onFilesPersisted?: () => void, ) { const { ownerHandle, sourceOwnerHandle, migrateOwner, ...publishPayload } = payload; const uploadTickets = publishPayload.files.flatMap((file) => @@ -2734,6 +2736,7 @@ async function publishSkillPayloadForApiUser( ...(source ? { sourceOwnerPublisherId: source.publisherId } : {}), ...(shouldMigrateOwner ? { migrateOwner: true } : {}), ...(uploadTickets.length > 0 ? { skillPublishUploadTickets: uploadTickets } : {}), + ...(onFilesPersisted ? { onFilesPersisted } : {}), }, ); } diff --git a/convex/lib/skillPublish.test.ts b/convex/lib/skillPublish.test.ts index db25bcca65..9d8d97e25b 100644 --- a/convex/lib/skillPublish.test.ts +++ b/convex/lib/skillPublish.test.ts @@ -1,4 +1,5 @@ import { createHash } from "node:crypto"; +import { getFunctionName } from "convex/server"; import { describe, expect, it, vi } from "vitest"; import { MAX_PUBLISH_FILE_BYTES } from "./publishLimits"; import { @@ -13,6 +14,77 @@ vi.mock("./embeddings", () => ({ })); describe("skillPublish", () => { + it.each([ + { staged: false, failure: "insert" }, + { staged: true, failure: "insert" }, + { staged: false, failure: "followup" }, + { staged: true, failure: "attempt" }, + { staged: true, failure: "none" }, + ])("transfers file ownership at persistence ($staged, $failure)", async ({ staged, failure }) => { + const events: string[] = []; + const markdown = "---\ndescription: Verify durable upload ownership.\n---\n# Ownership proof\n"; + const ctx = { + runQuery: vi.fn(async (ref: Parameters[0]) => + getFunctionName(ref) === "users:getByIdInternal" + ? { _id: "users:1", handle: "demo", createdAt: 1 } + : null, + ), + runMutation: vi.fn(async (ref: Parameters[0]) => { + const name = getFunctionName(ref); + if (name === "skills:insertVersion") { + events.push("insert"); + if (failure === "insert") throw new Error("insert rejected"); + return { skillId: "skills:demo", versionId: "skillVersions:demo" }; + } + if (name === "publishAttempts:createSkillPublishAttemptInternal") { + events.push("attempt"); + if (failure === "attempt") throw new Error("attempt failed"); + return { attemptId: "publishAttempts:demo", status: "pending_checks" }; + } + if (name === "skills:discardPendingPublicationInternal") events.push("discard"); + return null; + }), + scheduler: { + runAfter: vi.fn(async () => { + throw new Error("followup failed"); + }), + }, + storage: { get: vi.fn(async () => new Blob([markdown])) }, + }; + const result = publishVersionForUser( + ctx as never, + "users:1" as never, + { + slug: "ownership-proof", + displayName: "Ownership Proof", + version: "1.0.0", + changelog: "Initial release", + files: [file("_storage:skill", "SKILL.md", markdown.length, "text/markdown")], + }, + { + bypassGitHubAccountAge: true, + bypassQualityGate: true, + skipWebhook: true, + stagePrePublicationChecks: staged, + onFilesPersisted: () => { + events.push("persisted"); + }, + }, + ); + if (failure === "none") await expect(result).resolves.toMatchObject({ status: "pending" }); + else + await expect(result).rejects.toThrow( + failure === "insert" ? "insert rejected" : `${failure} failed`, + ); + expect(events).toEqual( + failure === "insert" + ? ["insert"] + : staged + ? ["insert", "persisted", "attempt", ...(failure === "attempt" ? ["discard"] : [])] + : ["insert", "persisted"], + ); + }); + it("normalizes agents/openai.yaml presentation metadata and hosts its icon", async () => { const skillMarkdown = "---\nname: Demo Skill\ndescription: SKILL.md summary.\n---\n# Demo Skill\n"; diff --git a/convex/lib/skillPublish.ts b/convex/lib/skillPublish.ts index 6ce413258e..dafe1d9ad6 100644 --- a/convex/lib/skillPublish.ts +++ b/convex/lib/skillPublish.ts @@ -155,6 +155,9 @@ export type PublishOptions = { migrateOwner?: boolean; stagePrePublicationChecks?: boolean; skillPublishUploadTickets?: Id<"skillPublishUploadTickets">[]; + // Called synchronously once a pending or published version owns the files. + // Later failures belong to publication compensation, not request upload cleanup. + onFilesPersisted?: () => void; }; type InternalPublishOptions = PublishOptions; @@ -582,6 +585,7 @@ async function publishVersionForUserInternal( internal.skills.insertVersion, skillInsertArgs, )) as PublishResult; + options.onFilesPersisted?.(); await scheduleSkillPublishFollowups(ctx, publishResult, followup); return { ...publishResult, @@ -600,6 +604,7 @@ async function publishVersionForUserInternal( internal.skills.insertVersion, pendingInsertArgs, )) as PublishResult; + options.onFilesPersisted?.(); const staged = (await ctx .runMutation(internal.publishAttempts.createSkillPublishAttemptInternal, { diff --git a/specs/spec.md b/specs/spec.md index 4e58c86139..7030e4e725 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -143,6 +143,15 @@ From SKILL.md frontmatter + AgentSkills + Clawdis extensions: - GitHub account age ≥ 14 days 5. Server stores files + metadata, sets `latest` tag, updates stats. +Multipart uploads remain request-owned until `skills.insertVersion` commits a +pending or published version. `publishVersionForUser` signals that ownership +transfer immediately after the mutation returns, before attempt creation or +scan scheduling. Request cleanup deletes only unadopted uploads, including +partial stores and rejected publication; a later failure must not delete files +referenced by a committed version. Pending-version compensation owns its own +file cleanup, even if compensation itself fails. Upload cleanup must not infer +ownership from the final HTTP status or publishing helper success. + Local fixture data lives in `convex/devSeed.ts` and `fixtures/public-corpus/`. ## Versioning + tags From 4cce941fcc78942135ad33479a8864b9582a0c23 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Tue, 15 Sep 2026 22:20:32 -0700 Subject: [PATCH 4/4] fix: preserve imported skill files after persistence --- convex/githubImport.test.ts | 20 ++++++++++++++++++++ convex/githubImport.ts | 13 +++++++++++-- specs/spec.md | 4 ++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/convex/githubImport.test.ts b/convex/githubImport.test.ts index fe7e1f0ae4..35a6aa20af 100644 --- a/convex/githubImport.test.ts +++ b/convex/githubImport.test.ts @@ -932,6 +932,26 @@ describe("githubImport", () => { expect(ctx.storage.delete).toHaveBeenCalledWith("storage:2"); }); + it("retains imported files when publication fails after persistence", async () => { + const ctx = makeImportCtx(); + vi.mocked(publishVersionForUser).mockImplementationOnce( + async (_ctx, _userId, _args, options) => { + options?.onFilesPersisted?.(); + throw new Error("post-commit followup failed"); + }, + ); + await expect( + __test.importGitHubSkillForUser( + ctx as never, + "users:1" as never, + makeImportArgs(), + makeOwnedImportFetch(buildOwnedImportZip()) as never, + ), + ).rejects.toThrow(/post-commit followup failed/); + expect(ctx.storage.store).toHaveBeenCalledTimes(2); + expect(ctx.storage.delete).not.toHaveBeenCalled(); + }); + it("deletes already-stored blobs when a later store call fails", async () => { const store = vi .fn() diff --git a/convex/githubImport.ts b/convex/githubImport.ts index 628b9f1d21..7104a9d5b9 100644 --- a/convex/githubImport.ts +++ b/convex/githubImport.ts @@ -330,6 +330,7 @@ async function importGitHubSkillForUser( contentType?: string; }> = []; + let filesPersisted = false; try { let totalBytes = 0; for (const path of selected.sort()) { @@ -394,7 +395,13 @@ async function importGitHubSkillForUser( files: storedFiles, source: sourceProvenance, }, - { ownerPublisherId: target.publisherId, sourceProvenance }, + { + ownerPublisherId: target.publisherId, + sourceProvenance, + onFilesPersisted: () => { + filesPersisted = true; + }, + }, ); } catch (error) { throw new ConvexError(buildPublishFailureMessage(error)); @@ -402,7 +409,9 @@ async function importGitHubSkillForUser( return { ok: true, slug: slugBase, version, ...result }; } catch (error) { - await Promise.allSettled(storedFiles.map((file) => ctx.storage.delete(file.storageId))); + if (!filesPersisted) { + await Promise.allSettled(storedFiles.map((file) => ctx.storage.delete(file.storageId))); + } throw error; } } diff --git a/specs/spec.md b/specs/spec.md index 7030e4e725..97c91e8bbd 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -152,6 +152,10 @@ referenced by a committed version. Pending-version compensation owns its own file cleanup, even if compensation itself fails. Upload cleanup must not infer ownership from the final HTTP status or publishing helper success. +GitHub imports validate publish metadata and resolve the owner before storing +selected files. They use the same persistence signal to end request cleanup; +failed stores or pre-persistence publication remove only that import's uploads. + Local fixture data lives in `convex/devSeed.ts` and `fixtures/public-corpus/`. ## Versioning + tags