fix(skills-hub): path traversal in remove() + add /skill remove Discord command

[Copilot]

📋 Summary

Two gaps in PR mofa-org#75's Skills Hub implementation: a path traversal vulnerability in SkillHubClient::remove(), and the missing /skill remove Discord slash command required by issue mofa-org#56's "Remove installed skills" user story.

🔗 Related Issues

Related to mofa-org#56

---

🧠 Context

remove() joined managed_root with an unvalidated name before calling fs::remove_dir_all(). A name like ../important_dir would silently delete directories outside ~/.mofaclaw/skills/hub/. Both install() and get_installed() already called validate_skill_name() — remove() was the odd one out.

Separately, the hub client's remove() method was never surfaced as a Discord slash command, leaving the "Manage Installed Skills → Remove" user story from issue mofa-org#56 unfulfilled.

---

🛠️ Changes

• Security: SkillHubClient::remove() — add validate_skill_name(name)?; guard before path join, consistent with install() and get_installed()
• Feature: /skill remove <name> Discord subcommand — member-role gated, mirrors /skill install permission model; registered in the skill command group
• Tests: remove_rejects_path_traversal_name, remove_returns_false_for_nonexistent_skill (unit); test_hub_client_remove_uninstalls_skill (integration)

---

🧪 How you Tested

1. cargo test -p mofaclaw-core skills_hub — 11/11 pass (was 10)
2. cargo test -p mofaclaw-core — 114/114 unit tests pass
3. cargo build -p mofaclaw-core — clean build

---

📸 Screenshots / Logs (if applicable)

running 11 tests
test skills_hub_tests::test_config_builder ... ok
test skills_hub_tests::test_config_builder_reads_root_skills_settings ... ok
test skills_hub_tests::test_default_hub_url ... ok
test skills_hub_tests::test_default_mofaclaw_paths ... ok
test skills_hub_tests::test_hub_client_remove_uninstalls_skill ... ok
test skills_hub_tests::test_load_config_rejects_removed_auto_install_key ... ok
test skills_hub_tests::test_parse_skill_version ... ok
test skills_hub_tests::test_skill_catalog_entry_matching ... ok
test skills_hub_tests::test_skill_config_with_custom_cache_root ... ok
test skills_hub_tests::test_skill_config_with_custom_managed_root ... ok
test skills_hub_tests::test_context_builder_discovers_skill_installed_via_hub_client_in_runtime_prompt ... ok

test result: ok. 11 passed; 0 failed

---

⚠️ Breaking Changes

• No breaking changes

---

🧹 Checklist

Code Quality

• Code follows Rust idioms and project conventions
• cargo fmt run
• cargo clippy --workspace --all-features passes locally

Testing

• Tests added/updated
• cargo test --workspace --all-features passes locally
• cargo build --examples (if examples are present)

Documentation

• Public APIs documented
• README / docs updated (if needed)

PR Hygiene

• PR is small and focused (one logical change)
• Branch is up to date with main
• No unrelated commits
• Commit messages explain why, not only what

---

🚀 Deployment Notes (if applicable)

No migrations or config changes required.

---

🧩 Additional Notes for Reviewers

The validate_skill_name fix is a one-liner but high-impact — without it, any caller (Discord command, API, test) passing a crafted name could blow away arbitrary directories under the user's home. The pattern is already established by install() and get_installed(); remove() was simply missed.

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.