fix: sync catalog copies for GitHub sources (#2974)

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",

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<string, Uint8Array>) {
       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<string, Uint8Array>) {
 }
 
 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}`,
   );
 }

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`.