Skip to content

Commit b3ba6b9

Browse files
cm-dyoshikawaclaude
andcommitted
refactor(sources): unify root-fallback decision and short-circuit github root fetch
Follow-ups from the PR #2118 review. Finding 1 (DRY): the "use the repo root as the requested skill" decision was implemented twice with asymmetric gates — an explicit SKILL.md check in the git path (`groupRemoteFilesBySkillRoot`) and a delegated check in the github path (`discoverGithubSkillDirs`). Extract a shared `shouldUseRootFallback({ skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir })` used by both so they cannot drift. Finding 2 (perf + behavior lock): the github fallback trigger was widened in #2118 to fire whenever the requested dir is absent, which performed a full root-file fetch even when the root had no SKILL.md to install. Detect the root SKILL.md from the directory listing already in hand and gate on it via the shared helper, so the full root fetch is short-circuited when nothing would be installed. Add tests locking the intended behavior: the root SKILL.md is installed under the requested name when it coexists with real skill dirs and the requested skill is absent, and no root files are fetched when there is no root SKILL.md. Finding 3 (.gitignore drive-by): `.tokensave/` is a third-party byproduct that no tool emits, so the hand-written .gitignore section is the correct owner (per the gitignore guidelines); left as-is intentionally. Closes #2119 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 43c3143 commit b3ba6b9

2 files changed

Lines changed: 108 additions & 5 deletions

File tree

src/lib/sources.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,6 +1247,72 @@ describe("resolveAndFetchSources", () => {
12471247
);
12481248
});
12491249

1250+
it("installs the root SKILL.md under the requested name when real skill dirs coexist and the requested skill is absent", async () => {
1251+
// Locks the (intended) widened-fallback behavior: when the repository has
1252+
// real skill dirs plus an incidental root SKILL.md and the caller requests a
1253+
// skill name that matches no dir, the root is installed under the requested
1254+
// name.
1255+
mockClientInstance.listDirectory.mockImplementation(
1256+
async (_owner: string, _repo: string, path: string) => {
1257+
if (path === ".") {
1258+
return [
1259+
{ name: "other-skill", path: "other-skill", type: "dir", size: 0 },
1260+
{ name: "SKILL.md", path: "SKILL.md", type: "file", size: 50 },
1261+
{ name: "README.md", path: "README.md", type: "file", size: 20 },
1262+
];
1263+
}
1264+
return [];
1265+
},
1266+
);
1267+
mockClientInstance.getFileContent.mockImplementation(
1268+
async (_o: string, _r: string, path: string) => {
1269+
if (path === "SKILL.md") return "# Root Skill";
1270+
if (path === "README.md") return "docs";
1271+
return "";
1272+
},
1273+
);
1274+
1275+
const result = await resolveAndFetchSources({
1276+
logger,
1277+
sources: [{ source: "org/repo:.", skills: ["nonexistent"] }],
1278+
projectRoot: testDir,
1279+
});
1280+
1281+
expect(result.fetchedSkillCount).toBe(1);
1282+
expect(writeFileContent).toHaveBeenCalledWith(
1283+
join(testDir, RULESYNC_CURATED_SKILLS_RELATIVE_DIR_PATH, "nonexistent", "SKILL.md"),
1284+
"# Root Skill",
1285+
);
1286+
});
1287+
1288+
it("does not fetch root files when the requested skill is absent and there is no root SKILL.md", async () => {
1289+
// The fallback (and its full root-file fetch) must be short-circuited when the
1290+
// directory listing shows no root SKILL.md — nothing would be installed, so no
1291+
// root content should be fetched.
1292+
mockClientInstance.listDirectory.mockImplementation(
1293+
async (_owner: string, _repo: string, path: string) => {
1294+
if (path === ".") {
1295+
return [
1296+
{ name: "other-skill", path: "other-skill", type: "dir", size: 0 },
1297+
{ name: "README.md", path: "README.md", type: "file", size: 20 },
1298+
];
1299+
}
1300+
return [];
1301+
},
1302+
);
1303+
1304+
const result = await resolveAndFetchSources({
1305+
logger,
1306+
sources: [{ source: "org/repo:.", skills: ["nonexistent"] }],
1307+
projectRoot: testDir,
1308+
});
1309+
1310+
expect(result.fetchedSkillCount).toBe(0);
1311+
expect(writeFileContent).not.toHaveBeenCalled();
1312+
// No root file contents are fetched because the fallback is short-circuited.
1313+
expect(mockClientInstance.getFileContent).not.toHaveBeenCalled();
1314+
});
1315+
12501316
it("should treat backslash-separated git paths as nested skill files", async () => {
12511317
const { resolveDefaultRef, fetchSkillFiles } = await import("./git-client.js");
12521318
vi.mocked(resolveDefaultRef).mockResolvedValue({ ref: "main", sha: "d".repeat(40) });

src/lib/sources.ts

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,33 @@ function getFirstPathSeparatorIndex(path: string): number {
434434
return Math.min(slashIndex, backslashIndex);
435435
}
436436

437+
/**
438+
* Decide whether a repository's root-level files should be installed as the
439+
* single requested skill (the "root fallback").
440+
*
441+
* A root fallback fires only when a single, non-wildcard skill was requested,
442+
* that skill's own directory is absent, and the repository root actually carries
443+
* a `SKILL.md`. Both the git transport (`groupRemoteFilesBySkillRoot`) and the
444+
* GitHub transport (`discoverGithubSkillDirs`) gate on these same conditions, so
445+
* the decision lives here to keep the two paths from drifting.
446+
*/
447+
function shouldUseRootFallback(params: {
448+
skillFilter: string[];
449+
isWildcard: boolean;
450+
hasRootSkillFile: boolean;
451+
hasRequestedSkillDir: boolean;
452+
}): boolean {
453+
const { skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir } = params;
454+
const [singleSkillName] = skillFilter;
455+
return (
456+
!isWildcard &&
457+
skillFilter.length === 1 &&
458+
singleSkillName !== undefined &&
459+
hasRootSkillFile &&
460+
!hasRequestedSkillDir
461+
);
462+
}
463+
437464
function groupRemoteFilesBySkillRoot(params: {
438465
remoteFiles: RemoteSkillFile[];
439466
skillFilter: string[];
@@ -464,11 +491,13 @@ function groupRemoteFilesBySkillRoot(params: {
464491
const [singleSkillName] = skillFilter;
465492
const hasRootSkillFile = rootLevelFiles.some((file) => file.relativePath === SKILL_FILE_NAME);
466493
if (
467-
!isWildcard &&
468-
skillFilter.length === 1 &&
469494
singleSkillName !== undefined &&
470-
hasRootSkillFile &&
471-
!grouped.has(singleSkillName)
495+
shouldUseRootFallback({
496+
skillFilter,
497+
isWildcard,
498+
hasRootSkillFile,
499+
hasRequestedSkillDir: grouped.has(singleSkillName),
500+
})
472501
) {
473502
grouped.set(singleSkillName, rootLevelFiles);
474503
}
@@ -729,7 +758,15 @@ async function discoverGithubSkillDirs(params: {
729758
const [singleSkillName] = skillFilter;
730759
const hasRequestedSkillDir =
731760
singleSkillName !== undefined && remoteSkillDirs.some((d) => d.name === singleSkillName);
732-
if (!isWildcard && skillFilter.length === 1 && !hasRequestedSkillDir) {
761+
// Detect a root-level SKILL.md from the directory listing we already have, so
762+
// the fallback (and its full root-file fetch) is skipped when there is no
763+
// root skill to install — not just when the requested dir is absent.
764+
const hasRootSkillFile = entries.some(
765+
(entry) => entry.type === "file" && entry.name === SKILL_FILE_NAME,
766+
);
767+
if (
768+
shouldUseRootFallback({ skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir })
769+
) {
733770
const fallback = await fetchRootLevelFallbackSkill({
734771
entries,
735772
parsed,

0 commit comments

Comments
 (0)