From 971518c6d93d191a713df05d79f6aa0afdd3093b Mon Sep 17 00:00:00 2001 From: Shoubhit Dash Date: Fri, 26 Jun 2026 17:42:50 +0530 Subject: [PATCH] fix(core): refresh cached remote skills (#34059) --- packages/core/src/skill/discovery.ts | 72 +++++++++++++++---- packages/core/test/skill-discovery.test.ts | 64 ++++++++++++++++- packages/opencode/src/skill/discovery.ts | 57 ++++++++++++--- .../opencode/test/skill/discovery.test.ts | 46 ++++++++++++ 4 files changed, 215 insertions(+), 24 deletions(-) diff --git a/packages/core/src/skill/discovery.ts b/packages/core/src/skill/discovery.ts index 6402dd3b71..8c885eef1f 100644 --- a/packages/core/src/skill/discovery.ts +++ b/packages/core/src/skill/discovery.ts @@ -52,6 +52,7 @@ function isSafeRelativePath(value: string) { class IndexSkill extends Schema.Class("SkillDiscovery.IndexSkill")({ name: Schema.String, + version: Schema.optional(Schema.String), files: Schema.Array(Schema.String), }) {} @@ -80,12 +81,15 @@ export const layer = Layer.effect( ) const download = Effect.fn("SkillDiscovery.download")(function* (url: string, destination: string) { - if (yield* fs.exists(destination).pipe(Effect.orDie)) return - yield* HttpClientRequest.get(url).pipe( + if (yield* fs.exists(destination).pipe(Effect.orDie)) return true + return yield* HttpClientRequest.get(url).pipe( http.execute, Effect.flatMap((response) => response.arrayBuffer), Effect.flatMap((body) => fs.writeWithDirs(destination, new Uint8Array(body))), - Effect.catch((error) => Effect.logError("failed to download skill file", { url, error })), + Effect.as(true), + Effect.catch((error) => + Effect.logError("failed to download skill file", { url, error }).pipe(Effect.as(false)), + ), ) }) @@ -120,6 +124,7 @@ export const layer = Layer.effect( } const skillUrl = new URL(`${encodeURIComponent(skill.name)}/`, source) + const versionFile = path.join(root, ".opencode-version") const files = skill.files.map((file) => { if (!isSafeRelativePath(file)) return undefined let resource: URL @@ -135,23 +140,66 @@ export const layer = Layer.effect( return { url: resource.href, destination, + file, } }) if (files.some((file) => file === undefined)) { return [] } - return [{ skill, root, files: files as { url: string; destination: string }[] }] + return [{ skill, root, versionFile, files: files as { url: string; destination: string; file: string }[] }] }), - ({ skill, root, files }) => + ({ skill, root, versionFile, files }) => Effect.gen(function* () { - yield* Effect.forEach(files, (file) => download(file.url, file.destination), { - concurrency: fileConcurrency, - discard: true, - }) - return (yield* fs.exists(path.join(root, "SKILL.md")).pipe(Effect.orDie)) || + const version = skill.version + const current = + version === undefined + ? undefined + : yield* fs.readFileStringSafe(versionFile).pipe(Effect.catch(() => Effect.succeed(undefined))) + if (version === undefined || current === version) { + yield* Effect.forEach(files, (file) => download(file.url, file.destination), { + concurrency: fileConcurrency, + discard: true, + }) + } else { + const token = crypto.randomUUID() + const staging = `${root}.tmp-${token}` + const backup = `${root}.old-${token}` + yield* Effect.gen(function* () { + const downloaded = yield* Effect.forEach( + files, + (file) => download(file.url, path.resolve(staging, file.file)), + { concurrency: fileConcurrency }, + ) + if (!downloaded.every(Boolean)) return + const exists = + (yield* fs.exists(path.join(staging, "SKILL.md")).pipe(Effect.orDie)) || + (yield* fs.exists(path.join(staging, `${skill.name}.md`)).pipe(Effect.orDie)) + if (!exists) return + yield* fs.writeFileString(path.join(staging, ".opencode-version"), version) + yield* Effect.uninterruptible( + Effect.gen(function* () { + const cached = yield* fs.exists(root).pipe(Effect.orDie) + if (cached) yield* fs.rename(root, backup) + yield* fs.rename(staging, root).pipe( + Effect.catch((error) => + Effect.gen(function* () { + if (cached) yield* fs.rename(backup, root).pipe(Effect.ignore) + return yield* Effect.fail(error) + }), + ), + ) + if (cached) yield* fs.remove(backup, { recursive: true, force: true }).pipe(Effect.ignore) + }), + ) + }).pipe( + Effect.catch((error) => Effect.logError("failed to refresh skill", { skill: skill.name, error })), + Effect.ensuring(fs.remove(staging, { recursive: true, force: true }).pipe(Effect.ignore)), + ) + } + const exists = + (yield* fs.exists(path.join(root, "SKILL.md")).pipe(Effect.orDie)) || (yield* fs.exists(path.join(root, `${skill.name}.md`)).pipe(Effect.orDie)) - ? [AbsolutePath.make(root)] - : [] + return exists ? [AbsolutePath.make(root)] : [] }), { concurrency: skillConcurrency }, ).pipe(Effect.map((directories) => directories.flat())) diff --git a/packages/core/test/skill-discovery.test.ts b/packages/core/test/skill-discovery.test.ts index 5fcecae4c7..7049374f31 100644 --- a/packages/core/test/skill-discovery.test.ts +++ b/packages/core/test/skill-discovery.test.ts @@ -10,8 +10,8 @@ import { tmpdir } from "./fixture/tmpdir" const base = "https://skills.example.test/catalog/" -async function pull(skills: unknown[], files: Record = {}) { - const tmp = await tmpdir() +async function pull(skills: unknown[], files: Record = {}, cache?: Awaited>) { + const tmp = cache ?? (await tmpdir()) const requests: string[] = [] const http = Layer.succeed( HttpClient.HttpClient, @@ -101,4 +101,64 @@ describe("SkillDiscovery.pull", () => { await result.tmp[Symbol.asyncDispose]() } }) + + test("refreshes cached files when the version changes", async () => { + const tmp = await tmpdir() + try { + const first = await pull( + [{ name: "deploy", version: "1", files: ["SKILL.md"] }], + { + [`${base}deploy/SKILL.md`]: "# Old", + }, + tmp, + ) + const second = await pull( + [{ name: "deploy", version: "2", files: ["SKILL.md"] }], + { + [`${base}deploy/SKILL.md`]: "# New", + }, + tmp, + ) + + expect(await fs.readFile(path.join(first.directories[0], "SKILL.md"), "utf8")).toBe("# New") + expect(second.requests).toContain(`${base}deploy/SKILL.md`) + const third = await pull( + [{ name: "deploy", version: "2", files: ["SKILL.md"] }], + { [`${base}deploy/SKILL.md`]: "# Ignored" }, + tmp, + ) + expect(third.requests).toEqual([`${base}index.json`]) + } finally { + await tmp[Symbol.asyncDispose]() + } + }) + + test("publishes complete updates and removes stale files", async () => { + const tmp = await tmpdir() + try { + const first = await pull( + [{ name: "deploy", version: "1", files: ["SKILL.md", "old.md"] }], + { + [`${base}deploy/SKILL.md`]: "# Old", + [`${base}deploy/old.md`]: "old reference", + }, + tmp, + ) + const root = first.directories[0] + + await pull( + [{ name: "deploy", version: "2", files: ["SKILL.md", "missing.md"] }], + { [`${base}deploy/SKILL.md`]: "# Partial" }, + tmp, + ) + expect(await fs.readFile(path.join(root, "SKILL.md"), "utf8")).toBe("# Old") + expect(await fs.readFile(path.join(root, "old.md"), "utf8")).toBe("old reference") + + await pull([{ name: "deploy", version: "3", files: ["SKILL.md"] }], { [`${base}deploy/SKILL.md`]: "# New" }, tmp) + expect(await fs.readFile(path.join(root, "SKILL.md"), "utf8")).toBe("# New") + expect(await Bun.file(path.join(root, "old.md")).exists()).toBe(false) + } finally { + await tmp[Symbol.asyncDispose]() + } + }) }) diff --git a/packages/opencode/src/skill/discovery.ts b/packages/opencode/src/skill/discovery.ts index cbb997a020..8440d682a2 100644 --- a/packages/opencode/src/skill/discovery.ts +++ b/packages/opencode/src/skill/discovery.ts @@ -13,6 +13,7 @@ const fileConcurrency = 8 class IndexSkill extends Schema.Class("IndexSkill")({ name: Schema.String, files: Schema.Array(Schema.String), + version: Schema.optional(Schema.String), }) {} class Index extends Schema.Class("Index")({ @@ -76,17 +77,53 @@ export const layer: Layer.Layer Effect.gen(function* () { const root = path.join(cache, skill.name) + const versionFile = path.join(root, ".opencode-version") + const version = skill.version + const current = + version === undefined + ? undefined + : yield* fs.readFileStringSafe(versionFile).pipe(Effect.catch(() => Effect.succeed(undefined))) - yield* Effect.forEach( - skill.files, - (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(root, file)), - { - concurrency: fileConcurrency, - }, - ) - - const md = path.join(root, "SKILL.md") - return (yield* fs.exists(md).pipe(Effect.orDie)) ? root : null + if (version === undefined || current === version) { + yield* Effect.forEach( + skill.files, + (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(root, file)), + { concurrency: fileConcurrency, discard: true }, + ) + } else { + const token = crypto.randomUUID() + const staging = `${root}.tmp-${token}` + const backup = `${root}.old-${token}` + yield* Effect.gen(function* () { + const downloaded = yield* Effect.forEach( + skill.files, + (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(staging, file)), + { concurrency: fileConcurrency }, + ) + if (!downloaded.every(Boolean)) return + if (!(yield* fs.exists(path.join(staging, "SKILL.md")).pipe(Effect.orDie))) return + yield* fs.writeFileString(path.join(staging, ".opencode-version"), version) + yield* Effect.uninterruptible( + Effect.gen(function* () { + const cached = yield* fs.exists(root).pipe(Effect.orDie) + if (cached) yield* fs.rename(root, backup) + yield* fs.rename(staging, root).pipe( + Effect.catch((error) => + Effect.gen(function* () { + if (cached) yield* fs.rename(backup, root).pipe(Effect.ignore) + return yield* Effect.fail(error) + }), + ), + ) + if (cached) yield* fs.remove(backup, { recursive: true, force: true }).pipe(Effect.ignore) + }), + ) + }).pipe( + Effect.catch((error) => Effect.logError("failed to refresh skill", { skill: skill.name, error })), + Effect.ensuring(fs.remove(staging, { recursive: true, force: true }).pipe(Effect.ignore)), + ) + } + return (yield* fs.exists(path.join(root, "SKILL.md")).pipe(Effect.orDie)) ? root : null }), { concurrency: skillConcurrency }, ) diff --git a/packages/opencode/test/skill/discovery.test.ts b/packages/opencode/test/skill/discovery.test.ts index 5dc5d5195b..a5833e7baf 100644 --- a/packages/opencode/test/skill/discovery.test.ts +++ b/packages/opencode/test/skill/discovery.test.ts @@ -11,6 +11,10 @@ import { testEffect } from "../lib/effect" let CLOUDFLARE_SKILLS_URL: string let server: ReturnType let downloadCount = 0 +let mutableVersion = "1" +let mutableContent = "# Old" +let mutableDownloadCount = 0 +let mutableFiles = ["SKILL.md"] const fixturePath = path.join(import.meta.dir, "../fixture/skills") const cacheDir = path.join(Global.Path.cache, "skills") @@ -24,6 +28,15 @@ beforeAll(async () => { async fetch(req) { const url = new URL(req.url) + if (url.pathname === "/mutable/index.json") { + return Response.json({ skills: [{ name: "mutable", version: mutableVersion, files: mutableFiles }] }) + } + if (url.pathname === "/mutable/mutable/SKILL.md") { + mutableDownloadCount++ + return new Response(mutableContent) + } + if (url.pathname === "/mutable/mutable/old.md") return new Response("old reference") + // route /.well-known/skills/* to the fixture directory if (url.pathname.startsWith("/.well-known/skills/")) { const filePath = url.pathname.replace("/.well-known/skills/", "") @@ -136,4 +149,37 @@ describe("Discovery.pull", () => { expect(downloadCount).toBe(firstCount) }), ) + + it.live("refreshes a remote skill when its version changes", () => + Effect.gen(function* () { + yield* Effect.promise(() => rm(cacheDir, { recursive: true, force: true })) + mutableVersion = "1" + mutableContent = "# Old" + mutableDownloadCount = 0 + mutableFiles = ["SKILL.md", "old.md"] + const discovery = yield* Discovery.Service + const url = `http://localhost:${server.port}/mutable/` + + const first = yield* discovery.pull(url) + expect(yield* Effect.promise(() => Bun.file(path.join(first[0], "SKILL.md")).text())).toBe("# Old") + + mutableVersion = "2" + mutableContent = "# Partial" + mutableFiles = ["SKILL.md", "missing.md"] + const second = yield* discovery.pull(url) + expect(yield* Effect.promise(() => Bun.file(path.join(second[0], "SKILL.md")).text())).toBe("# Old") + expect(yield* Effect.promise(() => Bun.file(path.join(second[0], "old.md")).text())).toBe("old reference") + + mutableVersion = "3" + mutableContent = "# New" + mutableFiles = ["SKILL.md"] + yield* discovery.pull(url) + expect(yield* Effect.promise(() => Bun.file(path.join(second[0], "SKILL.md")).text())).toBe("# New") + expect(yield* Effect.promise(() => Bun.file(path.join(second[0], "old.md")).exists())).toBe(false) + expect(mutableDownloadCount).toBe(3) + + yield* discovery.pull(url) + expect(mutableDownloadCount).toBe(3) + }), + ) })