diff --git a/packages/dotagents-lib/src/skills/resolver.integration.test.ts b/packages/dotagents-lib/src/skills/resolver.integration.test.ts index 5b215c1..a6dd63b 100644 --- a/packages/dotagents-lib/src/skills/resolver.integration.test.ts +++ b/packages/dotagents-lib/src/skills/resolver.integration.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; @@ -67,6 +67,7 @@ description: Code review skill }); afterEach(async () => { + vi.unstubAllEnvs(); await rm(tmpDir, { recursive: true }); }); @@ -134,6 +135,61 @@ description: A local skill } }); + it("prefers an explicit ref over an inline ref for single and wildcard resolution", async () => { + await exec("git", ["branch", "inline-ref"], { cwd: repoDir }); + await exec("git", ["checkout", "-b", "explicit-ref"], { cwd: repoDir }); + await mkdir(join(repoDir, "explicit-only"), { recursive: true }); + await writeFile( + join(repoDir, "explicit-only", "SKILL.md"), + `--- +name: explicit-only +description: Only available from the explicit ref +--- +`, + ); + await exec("git", ["add", "."], { cwd: repoDir }); + await exec("git", ["commit", "-m", "add explicit-only skill"], { + cwd: repoDir, + }); + + vi.stubEnv("GIT_CONFIG_COUNT", "1"); + vi.stubEnv( + "GIT_CONFIG_KEY_0", + `url.file://${repoDir}.insteadOf`, + ); + vi.stubEnv( + "GIT_CONFIG_VALUE_0", + "https://github.com/example/skills", + ); + + const source = "https://github.com/example/skills@inline-ref"; + const [single, wildcard] = await Promise.all([ + resolveSkill( + "explicit-only", + { source, ref: "explicit-ref" }, + { stateDir: join(stateDir, "single") }, + ), + resolveWildcardSkills( + { source, ref: "explicit-ref" }, + { stateDir: join(stateDir, "wildcard") }, + ), + ]); + + expect(single).toMatchObject({ + type: "git", + resolvedRef: "explicit-ref", + resolvedPath: "explicit-only", + }); + expect(wildcard).toContainEqual({ + name: "explicit-only", + resolved: expect.objectContaining({ + type: "git", + resolvedRef: "explicit-ref", + resolvedPath: "explicit-only", + }), + }); + }); + it("throws ResolveError when skill not found in repo", async () => { await expect( resolveSkill( diff --git a/packages/dotagents-lib/src/skills/resolver.ts b/packages/dotagents-lib/src/skills/resolver.ts index 54711cc..3068cba 100644 --- a/packages/dotagents-lib/src/skills/resolver.ts +++ b/packages/dotagents-lib/src/skills/resolver.ts @@ -325,44 +325,93 @@ export interface ResolveOpts { trust?: TrustPolicy; } -export async function resolveSkill( - skillName: string, - dep: { source: string; ref?: string; path?: string }, +type AcquiredSkillSource = + | { type: "local"; skillDir: string } + | { type: "well-known"; cacheDir: string | null; resolvedUrl: string } + | { + type: "git"; + repoDir: string; + resolvedUrl: string; + resolvedRef?: string; + commit: string; + }; + +async function acquireSkillSource( + dep: { source: string; ref?: string }, opts: ResolveOpts, -): Promise { +): Promise { const sourceForResolve = applyDefaultRepositorySource( dep.source, opts.defaultRepositorySource, ); - // Validate the EXPANDED source so that owner/repo shorthand under a non-default - // host (e.g. defaultRepositorySource: "gitlab") is checked against the actual - // clone URL, not the shorthand's implicit github.com mapping. + // Trust the expanded source before any local or network acquisition. if (opts.trust) {validateTrustedSource(sourceForResolve, opts.trust);} const parsed = parseSource(sourceForResolve); - if (parsed.type === "local") { const projectRoot = opts.projectRoot || process.cwd(); const skillDir = await resolveLocalSource(projectRoot, parsed.path!); - return { type: "local", source: dep.source, skillDir }; + return { type: "local", skillDir }; } if (parsed.type === "well-known") { - const baseUrl = parsed.url!; + const resolvedUrl = parsed.url!; const cached = await ensureWellKnownCached({ stateDir: opts.stateDir, - url: baseUrl, - cacheKey: wellKnownCacheKey(baseUrl), + url: resolvedUrl, + cacheKey: wellKnownCacheKey(resolvedUrl), ttlMs: opts.ttlMs, }); + return { + type: "well-known", + cacheDir: cached?.cacheDir ?? null, + resolvedUrl, + }; + } + + const url = parsed.url!; + const resolvedUrl = parsed.cloneUrl ?? url; + const resolvedRef = dep.ref ?? parsed.ref; + const cacheKey = parsed.type === "github" + ? `${parsed.owner}/${parsed.repo}` + : sanitizeCacheKey(url); + const excluded = isSourceExcluded(dep.source, opts.minimumReleaseAgeExclude); + const cached = await ensureCached({ + stateDir: opts.stateDir, + url: resolvedUrl, + cacheKey, + ref: resolvedRef, + minimumReleaseAge: excluded ? undefined : opts.minimumReleaseAge, + }); + + return { + type: "git", + repoDir: cached.repoDir, + resolvedUrl, + resolvedRef, + commit: cached.commit, + }; +} + +export async function resolveSkill( + skillName: string, + dep: { source: string; ref?: string; path?: string }, + opts: ResolveOpts, +): Promise { + const acquired = await acquireSkillSource(dep, opts); - if (!cached) { + if (acquired.type === "local") { + return { type: "local", source: dep.source, skillDir: acquired.skillDir }; + } + + if (acquired.type === "well-known") { + if (!acquired.cacheDir) { throw new ResolveError( - `No skills found at ${baseUrl}. If this is a git repository, use the git: prefix: git:${baseUrl}`, + `No skills found at ${acquired.resolvedUrl}. If this is a git repository, use the git: prefix: git:${acquired.resolvedUrl}`, ); } - const discovered = await discoverSkill(cached.cacheDir, skillName, { scanDirs: opts.scanDirs }); + const discovered = await discoverSkill(acquired.cacheDir, skillName, { scanDirs: opts.scanDirs }); if (!discovered) { throw new ResolveError( `Skill "${skillName}" not found in ${dep.source}. ` + @@ -373,37 +422,17 @@ export async function resolveSkill( return { type: "well-known", source: dep.source, - resolvedUrl: baseUrl, - skillDir: join(cached.cacheDir, discovered.path), + resolvedUrl: acquired.resolvedUrl, + skillDir: join(acquired.cacheDir, discovered.path), }; } - // Git source (GitHub or generic git) - const url = parsed.url!; - const cloneUrl = parsed.cloneUrl ?? url; - const ref = dep.ref ?? parsed.ref; - const cacheKey = - parsed.type === "github" - ? `${parsed.owner}/${parsed.repo}` - : sanitizeCacheKey(url); - - const excluded = isSourceExcluded(dep.source, opts.minimumReleaseAgeExclude); - const cached = await ensureCached({ - stateDir: opts.stateDir, - url: cloneUrl, - cacheKey, - ref, - minimumReleaseAge: excluded ? undefined : opts.minimumReleaseAge, - }); - - // Discover the skill within the repo let discovered: DiscoveredSkill | null; if (dep.path) { - // Explicit path override — load directly - const meta = await loadSkillMd(join(cached.repoDir, dep.path, "SKILL.md")); + const meta = await loadSkillMd(join(acquired.repoDir, dep.path, "SKILL.md")); discovered = { path: dep.path, meta }; } else { - discovered = await discoverSkill(cached.repoDir, skillName, { scanDirs: opts.scanDirs }); + discovered = await discoverSkill(acquired.repoDir, skillName, { scanDirs: opts.scanDirs }); } if (!discovered) { @@ -416,11 +445,11 @@ export async function resolveSkill( return { type: "git", source: dep.source, - resolvedUrl: cloneUrl, + resolvedUrl: acquired.resolvedUrl, resolvedPath: discovered.path, - resolvedRef: ref, - commit: cached.commit, - skillDir: join(cached.repoDir, discovered.path), + resolvedRef: acquired.resolvedRef, + commit: acquired.commit, + skillDir: join(acquired.repoDir, discovered.path), }; } @@ -440,21 +469,11 @@ export async function resolveWildcardSkills( dep: WildcardDependencyInput, opts: ResolveOpts, ): Promise { - const sourceForResolve = applyDefaultRepositorySource( - dep.source, - opts.defaultRepositorySource, - ); - // See note on resolveSkill: validate the expanded source so shorthand under a - // non-default host can't bypass the policy. - if (opts.trust) {validateTrustedSource(sourceForResolve, opts.trust);} - - const parsed = parseSource(sourceForResolve); + const acquired = await acquireSkillSource(dep, opts); const excludeSet = new Set(dep.exclude); - if (parsed.type === "local") { - const projectRoot = opts.projectRoot || process.cwd(); - const skillDir = await resolveLocalSource(projectRoot, parsed.path!); - const discovered = await discoverAllSkills(skillDir, { scanDirs: opts.scanDirs }); + if (acquired.type === "local") { + const discovered = await discoverAllSkills(acquired.skillDir, { scanDirs: opts.scanDirs }); return discovered .filter( (d) => @@ -465,25 +484,20 @@ export async function resolveWildcardSkills( resolved: { type: "local" as const, source: dep.source, - skillDir: join(skillDir, d.path), + skillDir: join(acquired.skillDir, d.path), }, })); } - if (parsed.type === "well-known") { - const baseUrl = parsed.url!; - const cached = await ensureWellKnownCached({ - stateDir: opts.stateDir, - url: baseUrl, - cacheKey: wellKnownCacheKey(baseUrl), - ttlMs: opts.ttlMs, - }); - - if (!cached) { + if (acquired.type === "well-known") { + if (!acquired.cacheDir) { return []; } - const discovered = await discoverAllSkills(cached.cacheDir, { scanDirs: opts.scanDirs }); + const cacheDir = acquired.cacheDir; + const discovered = await discoverAllSkills(cacheDir, { + scanDirs: opts.scanDirs, + }); return discovered .filter( (d) => !excludeSet.has(d.meta.name) && VALID_SKILL_NAME.test(d.meta.name), @@ -493,31 +507,13 @@ export async function resolveWildcardSkills( resolved: { type: "well-known" as const, source: dep.source, - resolvedUrl: baseUrl, - skillDir: join(cached.cacheDir, d.path), + resolvedUrl: acquired.resolvedUrl, + skillDir: join(cacheDir, d.path), }, })); } - // Git source - const url = parsed.url!; - const cloneUrl = parsed.cloneUrl ?? url; - const ref = dep.ref ?? parsed.ref; - const cacheKey = - parsed.type === "github" - ? `${parsed.owner}/${parsed.repo}` - : sanitizeCacheKey(url); - - const excluded = isSourceExcluded(dep.source, opts.minimumReleaseAgeExclude); - const cached = await ensureCached({ - stateDir: opts.stateDir, - url: cloneUrl, - cacheKey, - ref, - minimumReleaseAge: excluded ? undefined : opts.minimumReleaseAge, - }); - - const discovered = await discoverAllSkills(cached.repoDir, { scanDirs: opts.scanDirs }); + const discovered = await discoverAllSkills(acquired.repoDir, { scanDirs: opts.scanDirs }); return discovered .filter( @@ -528,11 +524,11 @@ export async function resolveWildcardSkills( resolved: { type: "git" as const, source: dep.source, - resolvedUrl: cloneUrl, + resolvedUrl: acquired.resolvedUrl, resolvedPath: d.path, - resolvedRef: ref, - commit: cached.commit, - skillDir: join(cached.repoDir, d.path), + resolvedRef: acquired.resolvedRef, + commit: acquired.commit, + skillDir: join(acquired.repoDir, d.path), }, })); } diff --git a/packages/dotagents-lib/src/skills/resolver.wellknown.test.ts b/packages/dotagents-lib/src/skills/resolver.wellknown.test.ts new file mode 100644 index 0000000..64059c0 --- /dev/null +++ b/packages/dotagents-lib/src/skills/resolver.wellknown.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it, vi } from "vitest"; +import { ResolveError, resolveSkill, resolveWildcardSkills } from "./resolver.js"; + +vi.mock("../sources/wellknown.js", () => ({ + ensureWellKnownCached: vi.fn().mockResolvedValue(null), +})); + +const STATE_DIR = "/tmp/dotagents-well-known-test-cache"; +const SOURCE = "https://skills.example.com"; + +describe("missing well-known index", () => { + it("rejects single-skill resolution", async () => { + await expect( + resolveSkill("review", { source: SOURCE }, { stateDir: STATE_DIR }), + ).rejects.toThrow(ResolveError); + }); + + it("returns no wildcard skills", async () => { + await expect( + resolveWildcardSkills( + { source: SOURCE, exclude: [] }, + { stateDir: STATE_DIR }, + ), + ).resolves.toEqual([]); + }); +});