fix: prevent recursive skill removal by marius-kilocode · Pull Request #11240 · Kilo-Org/kilocode

marius-kilocode · 2026-06-15T09:32:15Z

Issue #11227 reports complete loss of a user's working directory after removing customize-opencode from Settings > Agent Behaviour > Skills. The removal used recursive rm rather than Trash, so the workspace contents, .git data, uncommitted work, and untracked files could be permanently lost.

Root cause

The skill model used one unrestricted location: string for two different concepts:

a real SKILL.md filesystem path for discovered skills
a pseudo-location identifying a built-in skill

Kilo's built-in skills use the sentinel "builtin", while upstream customize-opencode uses "<built-in>". The settings UI and backend guard only recognized the Kilo sentinel. As a result, customize-opencode displayed a remove button and its pseudo-location passed through the webview, extension, SDK, and HTTP endpoint unchanged.

The backend then performed the equivalent of:

const resolved = path.resolve(location)
const dir = path.dirname(resolved)
await rm(dir, { recursive: true, force: true })

For location === "<built-in>", path.resolve() produced <server cwd>/<built-in>, and path.dirname() selected the server cwd. The VS Code extension starts the shared CLI backend with the first workspace folder as its cwd, so the recursive removal targeted the entire workspace.

The endpoint also accepted arbitrary client-supplied strings. Without registry validation, relative paths, traversal, absolute paths, and other malformed locations could select parent directories outside the intended skill directory.

How the vulnerable path developed

#7070, merged March 16, introduced discovered-skill removal in the settings UI and CLI backend. The implementation deleted path.dirname(location) recursively. Automated review flagged this as critical because the location came from the client. Commit bce8c6e6 added active-registry validation before the PR merged, but the destructive primitive remained.
#7603, merged March 27, introduced Kilo's kilo-config built-in skill with location "builtin". It added backend guards for that exact sentinel in commits 61170cea and d904bc7d.
#8772, the OpenCode v1.3.0 merge on April 11, removed the registry-validation behavior added in feat: add ability to remove discovered skills #7070 during the skill-service refactor. The remaining protection rejected only the exact Kilo "builtin" sentinel before recursively deleting the resolved parent directory.
#8919, merged April 15, restored Kilo built-in skill registration after another upstream refactor. Commit a5c422e8 also hid the remove button for skills whose location was exactly "builtin". Both UI and backend protection therefore depended on Kilo's sentinel value rather than built-in provenance.
Upstream OpenCode #26617, merged May 10, introduced customize-opencode with the distinct pseudo-location "<built-in>", initially behind a rollout flag. Upstream #26899, merged May 11, enabled it by default.
Kilo's OpenCode v1.14.46 merge #10822, merged June 3, brought customize-opencode and its "<built-in>" pseudo-location into Kilo. The compatibility layer initially retained rollout gating.
Kilo's OpenCode v1.14.51 merge #11031, merged June 10, adopted the upstream default-on behavior. This made the mismatched built-in visible to all users while Kilo's UI and backend still recognized only "builtin", completing the trigger shipped in v7.3.45.

No individual upstream change introduced recursive deletion. The data-loss path resulted from the interaction between Kilo's existing removal implementation, lost registry validation during an earlier merge, sentinel-specific guards, and upstream's later built-in skill representation.

Durable behavior introduced here

Removal now lives in a Kilo-owned fail-closed boundary rather than shared upstream skill code:

resolve the supplied location against the active backend registry before any filesystem operation
reject Kilo and legacy/upstream built-in pseudo-locations
reject unregistered, relative, malformed, and URL-cache locations
accept only an absolute registered SKILL.md manifest
unlink only that manifest, never its directory or parent
invalidate instance state after a successful removal
hide the action for both currently known built-in representations in the settings UI

The important safety property is structural: this removal path no longer has recursive directory-deletion capability. Future upstream changes to built-in sentinels may cause removal to be rejected, but cannot turn a pseudo-location into recursive workspace deletion.

Fixes #11227

kilo-code-bot · 2026-06-15T09:36:24Z

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
`packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts`	33	`catch` discards original error — all failures become an empty `BadRequest` with no message

Other Observations (not in diff)

No additional observations.

Files Reviewed (7 files)

.changeset/safe-skill-removal.md — no issues
packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx — no issues; builtin() helper correctly guards both "builtin" and "<built-in>" representations; the duplication from skill-remove.ts is expected since the webview cannot import CLI packages
packages/opencode/src/kilocode/server/httpapi/groups/kilocode.ts — no issues
packages/opencode/src/kilocode/server/httpapi/handlers/kilocode.ts — 1 warning (error swallowed in catch)
packages/opencode/src/kilocode/skill-remove.ts — no issues; URL-cache path guard logic (path.relative check) is correct
packages/opencode/src/skill/index.ts — old recursive rm correctly removed
packages/opencode/test/kilocode/skill-remove.test.ts — good unit coverage of all rejection paths
packages/opencode/test/kilocode/builtin-skills.test.ts — sentinel sibling test correctly exercises the non-recursive deletion behavior
packages/opencode/test/kilocode/server/httpapi-exercise-scenarios.ts — scenario updated properly

Fix these issues in Kilo Cloud

_{Reviewed by claude-4.6-sonnet-20260217 · 1,309,014 tokens}

_{Review guidance: REVIEW.md from base branch main}

Port from Kilo-Org/kilocode#11240. Their issue #11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O.

…true) runs async (#46968) * fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue #11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * fix(delegation): forward background flag in delegate_task dispatch delegate_task is an _AGENT_LOOP_TOOLS member, so every surface (CLI, gateway, desktop/TUI) routes it through AIAgent._dispatch_delegate_task. That forwarder passed every schema field except background, so delegate_task(background=true) was silently downgraded to a synchronous run and returned the sync results payload instead of a delegation_id. The model sees background in the schema (the call validates), but the value never reached the function. Add the one missing kwarg so async background delegation actually engages.

…true) runs async (NousResearch#46968) * fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue NousResearch#11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * fix(delegation): forward background flag in delegate_task dispatch delegate_task is an _AGENT_LOOP_TOOLS member, so every surface (CLI, gateway, desktop/TUI) routes it through AIAgent._dispatch_delegate_task. That forwarder passed every schema field except background, so delegate_task(background=true) was silently downgraded to a synchronous run and returned the sync results payload instead of a delegation_id. The model sees background in the schema (the call validates), but the value never reached the function. Add the one missing kwarg so async background delegation actually engages.

* fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue #11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * feat(desktop): allow /browser connect on a local gateway /browser was hardcoded as terminal-only in the desktop slash palette, so the chat GUI rejected it with "only available in the terminal interface." The TUI already drives the live CDP connection via the browser.manage RPC. Wire the same RPC into the desktop dispatcher as a /browser action handler, gated to local-gateway connections ($connection.mode !== 'remote'). connect mutates BROWSER_CDP_URL (and may launch Chrome) in the gateway process, so it's only meaningful when that process runs on this machine; a remote gateway gets a clear "local gateway only" message instead.

…search#46929) Port from Kilo-Org/kilocode#11240. Their issue NousResearch#11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O.

…true) runs async (NousResearch#46968) * fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue NousResearch#11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * fix(delegation): forward background flag in delegate_task dispatch delegate_task is an _AGENT_LOOP_TOOLS member, so every surface (CLI, gateway, desktop/TUI) routes it through AIAgent._dispatch_delegate_task. That forwarder passed every schema field except background, so delegate_task(background=true) was silently downgraded to a synchronous run and returned the sync results payload instead of a delegation_id. The model sees background in the schema (the call validates), but the value never reached the function. Add the one missing kwarg so async background delegation actually engages.

…h#47245) * fix(skills): guard recursive skill delete against tree-escape Port from Kilo-Org/kilocode#11240. Their issue NousResearch#11227 lost a user's entire working directory: a built-in-skill sentinel location resolved to the server cwd and the skill-removal endpoint ran a recursive delete on it. Hermes' /skills uninstall path (skills_hub.py) is already hardened, but the agent-facing skill_manage(action='delete') path did a bare shutil.rmtree(skill_dir) with no last-line validation. Add _validate_delete_target(): refuse to rmtree a path that (1) isn't strictly inside a known skills root, (2) is a skills root itself, or (3) is reached via a symlink/junction. Tests: 4 cases (normal delete works; symlinked dir, skills-root, out-of-tree all refused). E2E verified with real symlink + file I/O. * feat(desktop): allow /browser connect on a local gateway /browser was hardcoded as terminal-only in the desktop slash palette, so the chat GUI rejected it with "only available in the terminal interface." The TUI already drives the live CDP connection via the browser.manage RPC. Wire the same RPC into the desktop dispatcher as a /browser action handler, gated to local-gateway connections ($connection.mode !== 'remote'). connect mutates BROWSER_CDP_URL (and may launch Chrome) in the gateway process, so it's only meaningful when that process runs on this machine; a remote gateway gets a clear "local gateway only" message instead.