feat: cleaned up skill implementation

src/config/agent.rs

@@ -337,12 +337,10 @@ impl Agent {
         &self.config.mcp_servers
     }
 
-    #[allow(dead_code)]
     pub fn skills_enabled(&self) -> Option<bool> {
         self.config.skills_enabled
     }
 
-    #[allow(dead_code)]
     pub fn enabled_skills(&self) -> Option<&[String]> {
         self.config.enabled_skills.as_deref()
     }

src/config/mod.rs

@@ -268,6 +268,7 @@ pub fn install_builtins() -> Result<()> {
     Functions::install_builtin_global_tools(false)?;
     Agent::install_builtin_agents(false)?;
     Macro::install_macros(false)?;
+    Skill::install_builtin_skills(false)?;
     Ok(())
 }
 
@@ -276,18 +277,20 @@ pub enum AssetCategory {
     Agents,
     Macros,
     Functions,
+    Skills,
     #[value(name = "mcp_config")]
     McpConfig,
 }
 
 impl AssetCategory {
-    pub const NAMES: [&'static str; 4] = ["agents", "macros", "functions", "mcp_config"];
+    pub const NAMES: [&'static str; 5] = ["agents", "macros", "functions", "skills", "mcp_config"];
 
     pub fn parse(name: &str) -> Option<Self> {
         match name {
             "agents" => Some(Self::Agents),
             "macros" => Some(Self::Macros),
             "functions" => Some(Self::Functions),
+            "skills" => Some(Self::Skills),
             "mcp_config" => Some(Self::McpConfig),
             _ => None,
         }
@@ -333,6 +336,7 @@ pub fn install_assets(category: AssetCategory) -> Result<()> {
         AssetCategory::Agents => ("agents", paths::agents_data_dir()),
         AssetCategory::Macros => ("macros", paths::macros_dir()),
         AssetCategory::Functions => ("functions", paths::functions_dir()),
+        AssetCategory::Skills => ("skills", paths::skills_dir()),
         AssetCategory::McpConfig => ("MCP config", paths::mcp_config_file()),
     };
 
@@ -345,6 +349,7 @@ pub fn install_assets(category: AssetCategory) -> Result<()> {
         AssetCategory::Agents => Agent::install_builtin_agents(true)?,
         AssetCategory::Macros => Macro::install_macros(true)?,
         AssetCategory::Functions => Functions::install_builtin_global_tools(true)?,
+        AssetCategory::Skills => Skill::install_builtin_skills(true)?,
         AssetCategory::McpConfig => Functions::install_mcp_config()?,
     }
 

src/config/paths.rs

@@ -72,12 +72,10 @@ pub fn skills_dir() -> PathBuf {
     }
 }
 
-#[allow(dead_code)]
 pub fn skill_dir(name: &str) -> PathBuf {
     skills_dir().join(name)
 }
 
-#[allow(dead_code)]
 pub fn skill_file(name: &str) -> PathBuf {
     skill_dir(name).join("SKILL.md")
 }
@@ -251,7 +249,6 @@ pub fn has_macro(name: &str) -> bool {
     names.contains(&name.to_string())
 }
 
-#[allow(dead_code)]
 pub fn list_skills() -> Vec<String> {
     let mut names = Vec::new();
     if let Ok(rd) = read_dir(skills_dir()) {
@@ -270,7 +267,6 @@ pub fn list_skills() -> Vec<String> {
     names
 }
 
-#[allow(dead_code)]
 pub fn has_skill(name: &str) -> bool {
     skill_file(name).is_file()
 }

src/config/request_context.rs

@@ -4019,6 +4019,43 @@ mod tests {
         );
     }
 
+    #[test]
+    #[serial]
+    fn install_builtin_skills_force_overwrites_only_with_force() {
+        let _guard = TestConfigDirGuard::new();
+
+        Skill::install_builtin_skills(false).unwrap();
+        let file = paths::skill_file("git-master");
+        assert!(file.exists(), "git-master skill should be installed");
+
+        write(&file, "SENTINEL").unwrap();
+        Skill::install_builtin_skills(false).unwrap();
+        assert_eq!(
+            read_to_string(&file).unwrap(),
+            "SENTINEL",
+            "non-force install must not overwrite an existing skill"
+        );
+
+        Skill::install_builtin_skills(true).unwrap();
+        assert_ne!(
+            read_to_string(&file).unwrap(),
+            "SENTINEL",
+            "force install must overwrite the existing skill"
+        );
+    }
+
+    #[test]
+    #[serial]
+    fn install_builtin_skills_installs_all_bundled() {
+        let _guard = TestConfigDirGuard::new();
+
+        Skill::install_builtin_skills(false).unwrap();
+        assert!(paths::skill_file("git-master").exists());
+        assert!(paths::skill_file("ai-slop-remover").exists());
+        assert!(paths::skill_file("code-review").exists());
+        assert!(paths::skill_file("frontend-ui-ux").exists());
+    }
+
     #[test]
     #[serial]
     fn install_functions_force_preserves_user_mcp_json() {

src/config/role.rs

@@ -285,12 +285,10 @@ impl Role {
         self.continuation_prompt.as_deref()
     }
 
-    #[allow(dead_code)]
     pub fn skills_enabled(&self) -> Option<bool> {
         self.skills_enabled
     }
 
-    #[allow(dead_code)]
     pub fn enabled_skills(&self) -> Option<&str> {
         self.enabled_skills.as_deref()
     }

src/config/session.rs

@@ -79,12 +79,10 @@ pub struct Session {
 }
 
 impl Session {
-    #[allow(dead_code)]
     pub fn skills_enabled(&self) -> Option<bool> {
         self.skills_enabled
     }
 
-    #[allow(dead_code)]
     pub fn enabled_skills(&self) -> Option<&str> {
         self.enabled_skills.as_deref()
     }

src/config/skill.rs

@@ -6,6 +6,7 @@ use rust_embed::Embed;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use std::sync::LazyLock;
+use log::{debug, info};
 
 #[derive(Embed)]
 #[folder = "assets/skills/"]
@@ -39,7 +40,6 @@ pub struct Skill {
     auto_unload: Option<bool>,
 }
 
-#[allow(dead_code)]
 impl Skill {
     pub fn new(name: &str, content: &str) -> Self {
         let mut metadata = "";
@@ -84,11 +84,36 @@ impl Skill {
         skill
     }
 
-    pub fn builtin(name: &str) -> Result<Self> {
-        let content = SkillsAsset::get(&format!("{name}/SKILL.md"))
-            .ok_or_else(|| anyhow!("Unknown skill `{name}`"))?;
-        let content = unsafe { std::str::from_utf8_unchecked(&content.data) };
-        Ok(Skill::new(name, content))
+    pub fn install_builtin_skills(force: bool) -> Result<()> {
+        info!(
+            "Installing built-in skills in {}",
+            paths::skills_dir().display()
+        );
+
+        for file in SkillsAsset::iter() {
+            debug!("Processing skill file: {}", file.as_ref());
+
+            let embedded_file = SkillsAsset::get(&file).ok_or_else(|| {
+                anyhow!("Failed to load embedded skill file: {}", file.as_ref())
+            })?;
+            let content = unsafe { std::str::from_utf8_unchecked(&embedded_file.data) };
+            let file_path = paths::skills_dir().join(file.as_ref());
+
+            if file_path.exists() && !force {
+                debug!(
+                    "Skill file already exists, skipping: {}",
+                    file_path.display()
+                );
+                continue;
+            }
+
+            ensure_parent_exists(&file_path)?;
+            info!("Creating skill file: {}", file_path.display());
+            let mut skill_file = File::create(&file_path)?;
+            Write::write_all(&mut skill_file, content.as_bytes())?;
+        }
+
+        Ok(())
     }
 
     pub fn load(name: &str) -> Result<Self> {
@@ -99,12 +124,6 @@ impl Skill {
         Ok(Skill::new(name, &content))
     }
 
-    pub fn list_builtin_skill_names() -> Vec<String> {
-        SkillsAsset::iter()
-            .filter_map(|v| v.strip_suffix("/SKILL.md").map(|v| v.to_string()))
-            .collect()
-    }
-
     pub fn name(&self) -> &str {
         &self.name
     }
@@ -307,11 +326,4 @@ mod tests {
 
         assert!(skill.is_compatible(false, false));
     }
-
-    #[test]
-    fn builtin_returns_err_for_unknown_skill() {
-        let result = Skill::builtin("nonexistent_skill_xyz");
-
-        assert!(result.is_err());
-    }
 }

src/config/skill_policy.rs

@@ -7,15 +7,12 @@ use super::session::Session;
 use anyhow::{Result, bail};
 use std::collections::HashSet;
 
-#[allow(dead_code)]
 #[derive(Debug)]
 pub struct SkillPolicy {
     pub skills_enabled: bool,
-    pub visible: Option<HashSet<String>>,
     pub enabled: HashSet<String>,
 }
 
-#[allow(dead_code)]
 impl SkillPolicy {
     pub fn effective(
         global: &AppConfig,
@@ -101,7 +98,6 @@ impl SkillPolicy {
 
         Ok(Self {
             skills_enabled,
-            visible,
             enabled,
         })
     }
@@ -160,7 +156,6 @@ mod tests {
         .unwrap();
 
         assert!(policy.skills_enabled);
-        assert!(policy.visible.is_none());
         assert!(policy.enabled.is_empty());
     }
 

src/config/skill_registry.rs

@@ -5,7 +5,6 @@ use anyhow::{Result, bail};
 use indexmap::IndexMap;
 use std::collections::BTreeSet;
 
-#[allow(dead_code)]
 #[derive(Clone, Default)]
 pub struct SkillRegistry {
     loaded: IndexMap<String, Skill>,

src/function/mod.rs

@@ -355,7 +355,6 @@ impl Functions {
         self.declarations.extend(todo::todo_function_declarations());
     }
 
-    #[allow(dead_code)]
     pub fn append_skill_functions(&mut self) {
         self.declarations
             .extend(skill::skill_function_declarations());

src/function/skill.rs

@@ -9,7 +9,6 @@ use serde_json::{Value, json};
 
 pub const SKILL_FUNCTION_PREFIX: &str = "skill__";
 
-#[allow(dead_code)]
 pub fn skill_function_declarations() -> Vec<FunctionDeclaration> {
     vec![
         FunctionDeclaration {