From e39fd70b98b70931f58ca8ac567b5d7df89618de Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Mon, 6 Jul 2026 10:05:54 -0700 Subject: [PATCH] fix: sync catalog copies for GitHub sources (#2974) --- convex/githubSkillSync.test.ts | 130 +++++++++++++++++++++++++++++++++ convex/lib/githubSkillSync.ts | 15 ++-- specs/github-backed-skills.md | 6 ++ 3 files changed, 143 insertions(+), 8 deletions(-) diff --git a/convex/githubSkillSync.test.ts b/convex/githubSkillSync.test.ts index ff72c334..80cf97cf 100644 --- a/convex/githubSkillSync.test.ts +++ b/convex/githubSkillSync.test.ts @@ -1,4 +1,5 @@ import { getFunctionName } from "convex/server"; +import { ConvexError } from "convex/values"; import { zipSync } from "fflate"; import { describe, expect, it, vi } from "vitest"; import { @@ -380,6 +381,135 @@ describe("configurePublicGitHubSkillSourceHandler", () => { ); }); + it("prefers nested catalog skill paths over duplicate plugin package copies", async () => { + const zip = zipSync({ + "repo-main/plugins/aws-core/skills/amazon-bedrock/SKILL.md": new TextEncoder().encode( + "# Amazon Bedrock Plugin Copy\n", + ), + "repo-main/skills/core-skills/amazon-bedrock/SKILL.md": new TextEncoder().encode( + "# Amazon Bedrock\n", + ), + }); + const runQuery = vi.fn(async () => ({ + ownerUserId: "users:publisher-owner", + existingSource: null, + official: true, + })); + const runMutation = vi.fn(async () => ({ ok: true, stats: { discovered: 1 } })); + const fetchMock = vi + .fn() + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + full_name: "aws/agent-toolkit-for-aws", + private: false, + visibility: "public", + default_branch: "main", + disabled: false, + }), + }) + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ sha: "1".repeat(40) }), + }) + .mockResolvedValueOnce({ + ok: true, + headers: new Headers({ "content-length": String(zip.byteLength) }), + body: null, + arrayBuffer: async () => zip.buffer.slice(zip.byteOffset, zip.byteOffset + zip.byteLength), + }); + + await expect( + configurePublicGitHubSkillSourceHandler( + { runQuery, runMutation, auth: { getUserIdentity: vi.fn() } } as never, + { + ownerPublisherId: "publishers:local" as never, + repo: "aws/agent-toolkit-for-aws", + }, + fetchMock as never, + { + userId: "users:actor" as never, + }, + ), + ).resolves.toEqual({ ok: true, stats: { discovered: 1 } }); + + expect(runMutation).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + snapshot: expect.objectContaining({ + skills: [ + expect.objectContaining({ + slug: "amazon-bedrock", + path: "skills/core-skills/amazon-bedrock", + }), + ], + }), + }), + ); + }); + + it("rejects ambiguous catalog duplicate slugs with a client-visible error", async () => { + const zip = zipSync({ + "repo-main/skills/core-skills/amazon-bedrock/SKILL.md": new TextEncoder().encode( + "# Amazon Bedrock\n", + ), + "repo-main/skills/other-skills/amazon-bedrock/SKILL.md": new TextEncoder().encode( + "# Amazon Bedrock Duplicate\n", + ), + }); + const runQuery = vi.fn(async () => ({ + ownerUserId: "users:publisher-owner", + existingSource: null, + official: true, + })); + const runMutation = vi.fn(); + const fetchMock = vi + .fn() + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + full_name: "aws/agent-toolkit-for-aws", + private: false, + visibility: "public", + default_branch: "main", + disabled: false, + }), + }) + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ sha: "1".repeat(40) }), + }) + .mockResolvedValueOnce({ + ok: true, + headers: new Headers({ "content-length": String(zip.byteLength) }), + body: null, + arrayBuffer: async () => zip.buffer.slice(zip.byteOffset, zip.byteOffset + zip.byteLength), + }); + + let caught: unknown; + try { + await configurePublicGitHubSkillSourceHandler( + { runQuery, runMutation, auth: { getUserIdentity: vi.fn() } } as never, + { + ownerPublisherId: "publishers:local" as never, + repo: "aws/agent-toolkit-for-aws", + }, + fetchMock as never, + { + userId: "users:actor" as never, + }, + ); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(ConvexError); + expect((caught as { data?: unknown }).data).toMatch( + /duplicate normalized slug "amazon-bedrock"/i, + ); + expect(runMutation).not.toHaveBeenCalled(); + }); + it("rejects non-official publishers before fetching skill contents", async () => { const runQuery = vi.fn(async () => ({ ownerUserId: "users:publisher-owner", diff --git a/convex/lib/githubSkillSync.ts b/convex/lib/githubSkillSync.ts index ae4cf796..79c11595 100644 --- a/convex/lib/githubSkillSync.ts +++ b/convex/lib/githubSkillSync.ts @@ -1,3 +1,4 @@ +import { ConvexError } from "convex/values"; import { shouldPreserveSecurityScanStateForUnchangedContent, type SourceBackedSkillScanStatus, @@ -512,13 +513,13 @@ function sameLatestVersionSummary( function assertStoredMarkdownSize(path: string, bytes: Uint8Array) { if (bytes.byteLength > MAX_STORED_MARKDOWN_BYTES) { - throw new Error(`GitHub skill markdown file is too large to cache: ${path}`); + throw new ConvexError(`GitHub skill markdown file is too large to cache: ${path}`); } } function assertStoredSkillContentSize(totalBytes: number) { if (totalBytes > MAX_STORED_SKILL_CONTENT_BYTES) { - throw new Error("GitHub skill cached markdown is too large"); + throw new ConvexError("GitHub skill cached markdown is too large"); } } @@ -566,11 +567,9 @@ function discoverSkillPaths(entries: Record) { continue; } - const canonicalPath = `skills/${slug}/${SKILL_MARKDOWN_BASENAME}`; - const exactTopLevelMatches = paths.filter((path) => path.toLowerCase() === canonicalPath); - const topLevelSkillMatches = paths.filter((path) => path.toLowerCase().startsWith("skills/")); - if (exactTopLevelMatches.length === 1 && topLevelSkillMatches.length === 1) { - selected.push(exactTopLevelMatches[0] as string); + const catalogSkillMatches = paths.filter((path) => path.toLowerCase().startsWith("skills/")); + if (catalogSkillMatches.length === 1) { + selected.push(catalogSkillMatches[0] as string); continue; } @@ -585,7 +584,7 @@ function discoverSkillPaths(entries: Record) { } function duplicateSkillSlugError(slug: string, firstPath: string, secondPath: string) { - return new Error( + return new ConvexError( `GitHub skill source has duplicate normalized slug "${slug}" at ${firstPath} and ${secondPath}`, ); } diff --git a/specs/github-backed-skills.md b/specs/github-backed-skills.md index e0a13ce3..b9667d3c 100644 --- a/specs/github-backed-skills.md +++ b/specs/github-backed-skills.md @@ -102,6 +102,12 @@ mutation. The intended split is: 2. Fetch small target rows for changed/current skills. 3. Persist `SKILL.md` / `skill-card.md` content per skill. +Source discovery treats the repo's `skills/` tree as the canonical catalog area. +If package directories also contain copied skill folders that normalize to the +same slug, and exactly one matching `SKILL.md` path lives under `skills/`, sync +uses that catalog path. If two or more catalog paths normalize to the same slug, +sync rejects the repo with a client-visible validation error. + ## Manifest Rendering The only supported display manifest today is `skills.sh.json`.