fix: prevent recursive skill removal

.changeset/safe-skill-removal.md

@@ -0,0 +1,6 @@
+---
+"@kilocode/cli": patch
+"kilo-code": patch
+---
+
+Prevent skill removal from recursively deleting working directories.

packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx

@@ -43,6 +43,8 @@ interface SelectOption {
 
 import SettingsRow from "./SettingsRow"
 
+const builtin = (skill: SkillInfo) => skill.location === "builtin" || skill.location === "<built-in>"
+
 // View states for the agents subtab
 type AgentView = "list" | "create" | "edit"
 
@@ -848,10 +850,10 @@ const AgentBehaviourTab: Component = () => {
                     }}
                   >
                     <div>{skill.description}</div>
-                    {skill.location !== "builtin" && <div>{skill.location}</div>}
+                    {!builtin(skill) && <div>{skill.location}</div>}
                   </div>
                 </div>
-                {skill.location !== "builtin" && (
+                {!builtin(skill) && (
                   <IconButton size="small" variant="ghost" icon="close" onClick={() => confirmRemoveSkill(skill)} />
                 )}
               </div>

packages/opencode/src/kilocode/server/httpapi/groups/kilocode.ts

@@ -48,7 +48,7 @@ export const KilocodeApi = HttpApi.make("kilocode")
           OpenApi.annotations({
             identifier: "kilocode.removeSkill",
             summary: "Remove a skill",
-            description: "Remove a skill by deleting its directory from disk and clearing it from cache.",
+            description: "Remove a skill by deleting its manifest from disk and clearing it from cache.",
           }),
         ),
         HttpApiEndpoint.post("removeAgent", KilocodePaths.removeAgent, {

packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts

@@ -1,6 +1,7 @@
 import { Effect } from "effect"
-import { HttpApiBuilder } from "effect/unstable/httpapi"
+import { HttpApiBuilder, HttpApiError } from "effect/unstable/httpapi"
 import * as KiloAgent from "@/kilocode/agent"
+import * as KiloSkill from "@/kilocode/skill-remove"
 import { Agent } from "@/agent/agent"
 import { Config } from "@/config/config"
 import { EffectBridge } from "@/effect/bridge"
@@ -14,6 +15,7 @@ import { RemoveAgentPayload, RemoveSkillPayload } from "../groups/kilocode"
 export const kilocodeHandlers = HttpApiBuilder.group(InstanceHttpApi, "kilocode", (handlers) =>
   Effect.gen(function* () {
     const agents = yield* Agent.Service
+    const skills = yield* Skill.Service
     const config = yield* Config.Service
     const store = yield* InstanceStore.Service
 
@@ -24,7 +26,13 @@ export const kilocodeHandlers = HttpApiBuilder.group(InstanceHttpApi, "kilocode"
     const removeSkill = Effect.fn("KilocodeHttpApi.removeSkill")(function* (ctx: {
       payload: typeof RemoveSkillPayload.Type
     }) {
-      yield* Effect.promise(() => Skill.remove(ctx.payload.location))
+      const instance = yield* InstanceState.context
+      const entries = yield* skills.all()
+      yield* Effect.tryPromise({
+        try: () => KiloSkill.remove(ctx.payload.location, entries),
+        catch: () => new HttpApiError.BadRequest({}),
+      })
+      yield* store.dispose(instance)
       return true
     })
 

packages/opencode/src/kilocode/skill-remove.ts

@@ -0,0 +1,37 @@
+import { unlink } from "node:fs/promises"
+import path from "node:path"
+import { Global } from "@opencode-ai/core/global"
+import { Skill } from "@/skill"
+
+const LEGACY_BUILTIN_LOCATION = "<built-in>"
+
+type Info = Pick<Skill.Info, "location">
+
+export function builtin(location: string) {
+  return location === Skill.BUILTIN_LOCATION || location === LEGACY_BUILTIN_LOCATION
+}
+
+export function target(location: string, skills: readonly Info[]) {
+  if (builtin(location)) throw new Error("cannot remove built-in skill")
+
+  const skill = skills.find((item) => item.location === location)
+  if (!skill) throw new Error("skill not found in registry")
+  if (builtin(skill.location)) throw new Error("cannot remove built-in skill")
+  if (!path.isAbsolute(skill.location)) throw new Error("skill location must be absolute")
+
+  const file = path.resolve(skill.location)
+  if (path.basename(file) !== "SKILL.md") throw new Error("skill location must reference SKILL.md")
+
+  const cache = path.join(Global.Path.cache, "skills")
+  const relative = path.relative(cache, file)
+  if (relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative))) {
+    throw new Error("remove URL-backed skills from configuration")
+  }
+  return file
+}
+
+export async function remove(location: string, skills: readonly Info[]) {
+  const file = target(location, skills)
+  // Removing only the manifest disables discovery without recursively deleting user files.
+  await unlink(file)
+}

packages/opencode/src/skill/index.ts

@@ -14,7 +14,6 @@ import { RuntimeFlags } from "@/effect/runtime-flags"
 import { Glob } from "@opencode-ai/core/util/glob"
 import * as Log from "@opencode-ai/core/util/log"
 import { Discovery } from "./discovery"
-import { rm } from "fs/promises" // kilocode_change
 import { BUILTIN_SKILLS } from "../kilocode/skills/builtin" // kilocode_change
 import CUSTOMIZE_OPENCODE_SKILL_BODY from "./prompt/customize-opencode.md" with { type: "text" }
 import { isRecord } from "@/util/record"
@@ -349,15 +348,4 @@ export function fmt(list: Info[], opts: { verbose: boolean }) {
   ].join("\n")
 }
 
-// kilocode_change start - skill removal
-export async function remove(location: string) {
-  if (location === BUILTIN_LOCATION) {
-    throw new Error("cannot remove built-in skill")
-  }
-  const resolved = path.resolve(location)
-  const dir = path.dirname(resolved)
-  await rm(dir, { recursive: true, force: true })
-}
-// kilocode_change end
-
 export * as Skill from "."

packages/opencode/test/kilocode/builtin-skills.test.ts

@@ -3,6 +3,7 @@ import { Effect, Layer } from "effect"
 import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner"
 import path from "path"
 import { Skill } from "../../src/skill"
+import * as KiloSkill from "../../src/kilocode/skill-remove"
 import { BUILTIN_SKILLS } from "../../src/kilocode/skills/builtin"
 import { TestInstance } from "../fixture/fixture"
 import { testEffect } from "../lib/effect"
@@ -40,6 +41,18 @@ it.instance(
   { git: true },
 )
 
+it.instance(
+  "customize-opencode is protected from removal",
+  () =>
+    Effect.gen(function* () {
+      const skill = yield* Skill.Service
+      const item = yield* skill.get("customize-opencode")
+      expect(item).toBeDefined()
+      expect(KiloSkill.builtin(item!.location)).toBe(true)
+    }),
+  { git: true },
+)
+
 it.instance(
   "user skill overrides built-in with same name",
   () =>

packages/opencode/test/kilocode/server/httpapi-exercise-scenarios.ts

@@ -236,12 +236,33 @@ export const kiloScenarios: Scenario[] = [
     .post("/kilocode/skill/remove", "kilocode.removeSkill")
     .mutating()
     .preserveDatabase()
-    .seeded((ctx) => file(ctx, ".opencode/skill/httpapi-remove/SKILL.md", "# HTTP API remove\n"))
-    .at((ctx) => ({ path: "/kilocode/skill/remove", headers: ctx.headers(), body: { location: ctx.state } }))
+    .seeded((ctx) =>
+      Effect.gen(function* () {
+        const location = yield* file(
+          ctx,
+          ".opencode/skill/httpapi-remove/SKILL.md",
+          "---\nname: httpapi-remove\ndescription: HTTP API removal fixture.\n---\n# HTTP API remove\n",
+        )
+        const sentinel = yield* file(ctx, ".opencode/skill/httpapi-remove/KEEP.txt", "synthetic sentinel\n")
+        return { location, sentinel }
+      }),
+    )
+    .at((ctx) => ({
+      path: "/kilocode/skill/remove",
+      headers: ctx.headers(),
+      body: { location: ctx.state.location },
+    }))
     .jsonEffect(200, (body, ctx) =>
       Effect.gen(function* () {
         check(body === true, "skill removal should return true")
-        check(!(yield* Effect.promise(() => Bun.file(ctx.state).exists())), "removed skill should not remain on disk")
+        check(
+          !(yield* Effect.promise(() => Bun.file(ctx.state.location).exists())),
+          "removed skill should not remain on disk",
+        )
+        check(
+          yield* Effect.promise(() => Bun.file(ctx.state.sentinel).exists()),
+          "skill removal should preserve sibling files",
+        )
       }),
     ),
   http.protected

packages/opencode/test/kilocode/skill-remove.test.ts

@@ -0,0 +1,46 @@
+import { describe, expect, test } from "bun:test"
+import path from "node:path"
+import { Global } from "@opencode-ai/core/global"
+import { target } from "../../src/kilocode/skill-remove"
+
+const info = (location: string) => ({
+  name: "synthetic",
+  description: "Synthetic skill used for path validation.",
+  location,
+  content: "synthetic",
+})
+
+describe("skill removal target", () => {
+  test("rejects the canonical built-in location", () => {
+    expect(() => target("builtin", [info("builtin")])).toThrow("cannot remove built-in skill")
+  })
+
+  test("rejects the legacy customize-opencode built-in location", () => {
+    expect(() => target("<built-in>", [info("<built-in>")])).toThrow("cannot remove built-in skill")
+  })
+
+  test("rejects locations that are not in the active skill registry", () => {
+    const location = path.join(path.parse(process.cwd()).root, "__kilo_synthetic__", "SKILL.md")
+    expect(() => target(location, [])).toThrow("skill not found in registry")
+  })
+
+  test("rejects relative registered locations", () => {
+    const location = path.join("synthetic", "SKILL.md")
+    expect(() => target(location, [info(location)])).toThrow("skill location must be absolute")
+  })
+
+  test("rejects registered locations that are not manifests", () => {
+    const location = path.join(path.parse(process.cwd()).root, "__kilo_synthetic__", "skill")
+    expect(() => target(location, [info(location)])).toThrow("skill location must reference SKILL.md")
+  })
+
+  test("rejects URL-backed cache entries", () => {
+    const location = path.join(Global.Path.cache, "skills", "synthetic", "SKILL.md")
+    expect(() => target(location, [info(location)])).toThrow("remove URL-backed skills from configuration")
+  })
+
+  test("returns only the registered skill manifest", () => {
+    const location = path.join(path.parse(process.cwd()).root, "__kilo_synthetic__", "skill", "SKILL.md")
+    expect(target(location, [info(location)])).toBe(location)
+  })
+})