fix(skills): guard recursive skill delete against tree-escape (port Kilo #11240)

[teknium1]

Summary

The agent-facing skill_manage(action='delete') path now refuses to recursively delete a skill directory that could escape the skills tree. Previously it called shutil.rmtree(skill_dir) with no last-line validation.

Ported from Kilo-Org/kilocode#11240. Their issue #11227 caused complete loss of a user's working directory: a built-in-skill sentinel location resolved to the server cwd, and the skill-removal endpoint ran a recursive delete on path.dirname() of it.

What was already safe (and what wasn't)

• /skills uninstall (tools/skills_hub.py) is already hardened — it has _normalize_lock_install_path, symlink/junction redirect rejection, and resolve-then-reject-root checks. No change there.
• The agent-facing skill_manage(action='delete') path in tools/skill_manager_tool.py had none of that. skill_dir comes from _find_skill (a real SKILL.md parent discovered by walking the roots), so arbitrary-path injection isn't directly reachable today — but the bare rmtree had no defense-in-depth against a poisoned/symlinked skills tree or a discovery edge case handing back a root.

Changes

• tools/skill_manager_tool.py: add _validate_delete_target() (+ _is_path_redirect helper) called before shutil.rmtree in _delete_skill. Refuses when the skill dir (1) doesn't resolve strictly inside a known skills root, (2) is a skills root (would wipe every installed skill), or (3) is reached via a symlink/junction (rmtree would follow it out of tree).
• tests/tools/test_skill_manager_tool.py: new TestDeleteSkillRmtreeGuard (4 cases).

How it was adapted

Kilo's fix is TypeScript/Effect over an HTTP endpoint that accepted a client-supplied location string; the core risk was arbitrary path → resolve → dirname → recursive rm. Hermes doesn't take a client path here (discovery-only), so this is the matching defense-in-depth for the recursive-delete primitive rather than a 1:1 port. The symlink/junction check matches the existing _is_path_redirect pattern already used in skills_hub.py.

Validation

	Before	After
Normal delete	works	works
Symlinked skill dir	rmtree follows link, deletes target contents	refused, target intact
Skills root handed to delete	wipes all skills	refused
Out-of-tree absolute path	deletes it	refused

tests/tools/test_skill_manager_tool.py: 94 passed (90 prior + 4 new). E2E verified with real symlink creation + file I/O against a temp HERMES_HOME.

Infographic

[github-actions]

🔎 Lint report: kilocode-port/skill-delete-rmtree-guard vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 10962 on HEAD, 10960 on base (🆕 +2)

🆕 New issues (2):

Rule	Count
unresolved-attribute	2

First entries

run_agent.py:2920: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`

✅ Fixed issues (1):

Rule	Count
invalid-assignment	1

First entries

tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`

Unchanged: 5771 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.