Merge pull request #2198 from dyoshikawa/resolve-issue-2119-root-fallback-dry

refactor(sources): unify root-fallback decision and short-circuit github root fetch

Author: dyoshikawa

src/lib/sources.test.ts

@@ -1247,6 +1247,72 @@ describe("resolveAndFetchSources", () => {
     );
   });
 
+  it("installs the root SKILL.md under the requested name when real skill dirs coexist and the requested skill is absent", async () => {
+    // Locks the (intended) widened-fallback behavior: when the repository has
+    // real skill dirs plus an incidental root SKILL.md and the caller requests a
+    // skill name that matches no dir, the root is installed under the requested
+    // name.
+    mockClientInstance.listDirectory.mockImplementation(
+      async (_owner: string, _repo: string, path: string) => {
+        if (path === ".") {
+          return [
+            { name: "other-skill", path: "other-skill", type: "dir", size: 0 },
+            { name: "SKILL.md", path: "SKILL.md", type: "file", size: 50 },
+            { name: "README.md", path: "README.md", type: "file", size: 20 },
+          ];
+        }
+        return [];
+      },
+    );
+    mockClientInstance.getFileContent.mockImplementation(
+      async (_o: string, _r: string, path: string) => {
+        if (path === "SKILL.md") return "# Root Skill";
+        if (path === "README.md") return "docs";
+        return "";
+      },
+    );
+
+    const result = await resolveAndFetchSources({
+      logger,
+      sources: [{ source: "org/repo:.", skills: ["nonexistent"] }],
+      projectRoot: testDir,
+    });
+
+    expect(result.fetchedSkillCount).toBe(1);
+    expect(writeFileContent).toHaveBeenCalledWith(
+      join(testDir, RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATH, "nonexistent", "SKILL.md"),
+      "# Root Skill",
+    );
+  });
+
+  it("does not fetch root files when the requested skill is absent and there is no root SKILL.md", async () => {
+    // The fallback (and its full root-file fetch) must be short-circuited when the
+    // directory listing shows no root SKILL.md — nothing would be installed, so no
+    // root content should be fetched.
+    mockClientInstance.listDirectory.mockImplementation(
+      async (_owner: string, _repo: string, path: string) => {
+        if (path === ".") {
+          return [
+            { name: "other-skill", path: "other-skill", type: "dir", size: 0 },
+            { name: "README.md", path: "README.md", type: "file", size: 20 },
+          ];
+        }
+        return [];
+      },
+    );
+
+    const result = await resolveAndFetchSources({
+      logger,
+      sources: [{ source: "org/repo:.", skills: ["nonexistent"] }],
+      projectRoot: testDir,
+    });
+
+    expect(result.fetchedSkillCount).toBe(0);
+    expect(writeFileContent).not.toHaveBeenCalled();
+    // No root file contents are fetched because the fallback is short-circuited.
+    expect(mockClientInstance.getFileContent).not.toHaveBeenCalled();
+  });
+
   it("should treat backslash-separated git paths as nested skill files", async () => {
     const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js");
     vi.mocked(resolveDefaultRef).mockResolvedValue({ ref: "main", sha: "d".repeat(40) });

src/lib/sources.ts

@@ -434,6 +434,33 @@ function getFirstPathSeparatorIndex(path: string): number {
   return Math.min(slashIndex, backslashIndex);
 }
 
+/**
+ * Decide whether a repository's root-level files should be installed as the
+ * single requested skill (the "root fallback").
+ *
+ * A root fallback fires only when a single, non-wildcard skill was requested,
+ * that skill's own directory is absent, and the repository root actually carries
+ * a `SKILL.md`. Both the git transport (`groupRemoteFilesBySkillRoot`) and the
+ * GitHub transport (`discoverGithubSkillDirs`) gate on these same conditions, so
+ * the decision lives here to keep the two paths from drifting.
+ */
+function shouldUseRootFallback(params: {
+  skillFilter: string[];
+  isWildcard: boolean;
+  hasRootSkillFile: boolean;
+  hasRequestedSkillDir: boolean;
+}): boolean {
+  const { skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir } = params;
+  const [singleSkillName] = skillFilter;
+  return (
+    !isWildcard &&
+    skillFilter.length === 1 &&
+    singleSkillName !== undefined &&
+    hasRootSkillFile &&
+    !hasRequestedSkillDir
+  );
+}
+
 function groupRemoteFilesBySkillRoot(params: {
   remoteFiles: RemoteSkillFile[];
   skillFilter: string[];
@@ -464,11 +491,13 @@ function groupRemoteFilesBySkillRoot(params: {
   const [singleSkillName] = skillFilter;
   const hasRootSkillFile = rootLevelFiles.some((file) => file.relativePath === SKILL_FILE_NAME);
   if (
-    !isWildcard &&
-    skillFilter.length === 1 &&
     singleSkillName !== undefined &&
-    hasRootSkillFile &&
-    !grouped.has(singleSkillName)
+    shouldUseRootFallback({
+      skillFilter,
+      isWildcard,
+      hasRootSkillFile,
+      hasRequestedSkillDir: grouped.has(singleSkillName),
+    })
   ) {
     grouped.set(singleSkillName, rootLevelFiles);
   }
@@ -729,7 +758,15 @@ async function discoverGithubSkillDirs(params: {
     const [singleSkillName] = skillFilter;
     const hasRequestedSkillDir =
       singleSkillName !== undefined && remoteSkillDirs.some((d) => d.name === singleSkillName);
-    if (!isWildcard && skillFilter.length === 1 && !hasRequestedSkillDir) {
+    // Detect a root-level SKILL.md from the directory listing we already have, so
+    // the fallback (and its full root-file fetch) is skipped when there is no
+    // root skill to install — not just when the requested dir is absent.
+    const hasRootSkillFile = entries.some(
+      (entry) => entry.type === "file" && entry.name === SKILL_FILE_NAME,
+    );
+    if (
+      shouldUseRootFallback({ skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir })
+    ) {
       const fallback = await fetchRootLevelFallbackSkill({
         entries,
         parsed,