fix(core): refresh cached remote skills (#34059)

packages/core/src/skill/discovery.ts

@@ -52,6 +52,7 @@ function isSafeRelativePath(value: string) {
 
 class IndexSkill extends Schema.Class<IndexSkill>("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()))

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<string, string> = {}) {
-  const tmp = await tmpdir()
+async function pull(skills: unknown[], files: Record<string, string> = {}, cache?: Awaited<ReturnType<typeof tmpdir>>) {
+  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]()
+    }
+  })
 })

packages/opencode/src/skill/discovery.ts

@@ -13,6 +13,7 @@ const fileConcurrency = 8
 class IndexSkill extends Schema.Class<IndexSkill>("IndexSkill")({
   name: Schema.String,
   files: Schema.Array(Schema.String),
+  version: Schema.optional(Schema.String),
 }) {}
 
 class Index extends Schema.Class<Index>("Index")({
@@ -76,17 +77,53 @@ export const layer: Layer.Layer<Service, never, FSUtil.Service | Path.Path | Htt
         (skill) =>
           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 },
       )

packages/opencode/test/skill/discovery.test.ts

@@ -11,6 +11,10 @@ import { testEffect } from "../lib/effect"
 let CLOUDFLARE_SKILLS_URL: string
 let server: ReturnType<typeof Bun.serve>
 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)
+    }),
+  )
 })