diff --git a/.changeset/safe-skill-removal.md b/.changeset/safe-skill-removal.md new file mode 100644 index 00000000000..20010d1b6d0 --- /dev/null +++ b/.changeset/safe-skill-removal.md @@ -0,0 +1,6 @@ +--- +"@kilocode/cli": patch +"kilo-code": patch +--- + +Prevent skill removal from recursively deleting working directories. diff --git a/packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx b/packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx index dd61fa49405..63d45639c48 100644 --- a/packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx +++ b/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 === "" + // View states for the agents subtab type AgentView = "list" | "create" | "edit" @@ -848,10 +850,10 @@ const AgentBehaviourTab: Component = () => { }} >
{skill.description}
- {skill.location !== "builtin" &&
{skill.location}
} + {!builtin(skill) &&
{skill.location}
} - {skill.location !== "builtin" && ( + {!builtin(skill) && ( confirmRemoveSkill(skill)} /> )} diff --git a/packages/opencode/src/kilocode/server/httpapi/groups/kilocode.ts b/packages/opencode/src/kilocode/server/httpapi/groups/kilocode.ts index 20c13ae84b9..5af99a9c83d 100644 --- a/packages/opencode/src/kilocode/server/httpapi/groups/kilocode.ts +++ b/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, { diff --git a/packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts b/packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts index 75faa4c6fb2..88e364e51a7 100644 --- a/packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts +++ b/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 }) diff --git a/packages/opencode/src/kilocode/skill-remove.ts b/packages/opencode/src/kilocode/skill-remove.ts new file mode 100644 index 00000000000..d541f33e922 --- /dev/null +++ b/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 = "" + +type Info = Pick + +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) +} diff --git a/packages/opencode/src/skill/index.ts b/packages/opencode/src/skill/index.ts index cb70f263823..bbaded35973 100644 --- a/packages/opencode/src/skill/index.ts +++ b/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 "." diff --git a/packages/opencode/test/kilocode/builtin-skills.test.ts b/packages/opencode/test/kilocode/builtin-skills.test.ts index fd3672ad70d..1bf05dcd427 100644 --- a/packages/opencode/test/kilocode/builtin-skills.test.ts +++ b/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", () => diff --git a/packages/opencode/test/kilocode/server/httpapi-exercise-scenarios.ts b/packages/opencode/test/kilocode/server/httpapi-exercise-scenarios.ts index 02cde56c670..6236f5d8a53 100644 --- a/packages/opencode/test/kilocode/server/httpapi-exercise-scenarios.ts +++ b/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 diff --git a/packages/opencode/test/kilocode/skill-remove.test.ts b/packages/opencode/test/kilocode/skill-remove.test.ts new file mode 100644 index 00000000000..1a259d769a8 --- /dev/null +++ b/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("", [info("")])).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) + }) +})