diff --git a/convex/githubImport.test.ts b/convex/githubImport.test.ts index 172f3817ba..35a6aa20af 100644 --- a/convex/githubImport.test.ts +++ b/convex/githubImport.test.ts @@ -5,6 +5,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: { @@ -14,9 +15,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, @@ -49,6 +154,7 @@ describe("githubImport", () => { }); afterEach(() => { + vi.mocked(publishVersionForUser).mockReset(); if (originalGitHubToken) { process.env.GITHUB_TOKEN = originalGitHubToken; } else { @@ -787,4 +893,86 @@ 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("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() + .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 d29817c720..7104a9d5b9 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,98 @@ 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>; + let filesPersisted = false; 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, + onFilesPersisted: () => { + filesPersisted = true; + }, + }, + ); + } catch (error) { + throw new ConvexError(buildPublishFailureMessage(error)); + } + + return { ok: true, slug: slugBase, version, ...result }; } catch (error) { - throw new ConvexError(buildPublishFailureMessage(error)); + if (!filesPersisted) { + await Promise.allSettled(storedFiles.map((file) => ctx.storage.delete(file.storageId))); + } + throw error; } - - return { ok: true, slug: slugBase, version, ...result }; } async function listOwnedPublicGitHubReposForUser( 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