diff --git a/convex/httpApiV1.handlers.test.ts b/convex/httpApiV1.handlers.test.ts index 241d956004..4719fa4d22 100644 --- a/convex/httpApiV1.handlers.test.ts +++ b/convex/httpApiV1.handlers.test.ts @@ -8474,6 +8474,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", @@ -8515,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" }; @@ -8587,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" }; @@ -8625,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", @@ -8652,8 +8738,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" }, @@ -8663,6 +8751,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 () => { @@ -8709,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.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 c3816098df..4b6acafa0d 100644 --- a/convex/httpApiV1/skillsV1.ts +++ b/convex/httpApiV1/skillsV1.ts @@ -85,6 +85,7 @@ import { MAX_RAW_FILE_BYTES, type AmbiguousSkillSlugChoice, ambiguousSkillSlugResponse, + deleteStoredMultipartFiles, formatAuthzMessage, formatUserFacingErrorMessage, getPathSegments, @@ -2673,11 +2674,20 @@ 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"; @@ -2691,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) => @@ -2725,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