kumiho-construct 2026.5.11

use anyhow::{Context, Result};
use directories::UserDirs;
use reqwest::Url;
use serde::{Deserialize, Serialize};
use std::collections::{HashMap, HashSet};
use std::io::Cursor;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::time::{Duration, SystemTime};

use zip::ZipArchive;

mod audit;
#[cfg(feature = "skill-creation")]
pub mod auto_improve;
#[cfg(feature = "skill-creation")]
pub mod auto_rollback;
#[cfg(feature = "skill-creation")]
pub mod creator;
pub mod effectiveness;
pub mod effectiveness_cache;
#[cfg(feature = "skill-creation")]
pub mod improver;
pub mod registration;
pub mod testing;

// Re-export the trait + score type so consumers can use them as
// `crate::skills::SkillEffectivenessProvider`.  The concrete providers
// (NoOpProvider / InMemoryProvider) are reachable via
// `crate::skills::effectiveness::…` when callers need them.
pub use effectiveness::{EffectivenessScore, SkillEffectivenessProvider};
pub use effectiveness_cache::EffectivenessCache;

const OPEN_SKILLS_REPO_URL: &str = "https://github.com/besoeasy/open-skills";
const OPEN_SKILLS_SYNC_MARKER: &str = ".construct-open-skills-sync";
const OPEN_SKILLS_SYNC_INTERVAL_SECS: u64 = 60 * 60 * 24 * 7;

// ─── ClawhHub / OpenClaw registry installers ───────────────────────────────
const CLAWHUB_DOMAIN: &str = "clawhub.ai";
const CLAWHUB_WWW_DOMAIN: &str = "www.clawhub.ai";
const CLAWHUB_DOWNLOAD_API: &str = "https://clawhub.ai/api/v1/download";
const MAX_CLAWHUB_ZIP_BYTES: u64 = 50 * 1024 * 1024; // 50 MiB

/// A skill is a user-defined or community-built capability.
/// Skills live in `~/.construct/workspace/skills/<name>/SKILL.md`
/// and can include tool definitions, prompts, and automation scripts.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Skill {
    pub name: String,
    pub description: String,
    pub version: String,
    #[serde(default)]
    pub author: Option<String>,
    #[serde(default)]
    pub tags: Vec<String>,
    #[serde(default)]
    pub tools: Vec<SkillTool>,
    #[serde(default)]
    pub prompts: Vec<String>,
    #[serde(skip)]
    pub location: Option<PathBuf>,
}

/// A tool defined by a skill (shell command, HTTP call, etc.)
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct SkillTool {
    pub name: String,
    pub description: String,
    /// "shell", "http", "script"
    pub kind: String,
    /// The command/URL/script to execute
    pub command: String,
    #[serde(default)]
    pub args: HashMap<String, String>,
}

/// Skill manifest parsed from SKILL.toml
#[derive(Debug, Clone, Serialize, Deserialize)]
pub(crate) struct SkillManifest {
    pub(crate) skill: SkillMeta,
    #[serde(default, skip_serializing_if = "Vec::is_empty")]
    pub(crate) tools: Vec<SkillTool>,
    /// Legacy embedded prompt content.  Kept on the manifest for
    /// backwards compatibility — newer manifests use
    /// `[skill].content_file` instead and leave this empty.
    #[serde(default, skip_serializing_if = "Vec::is_empty")]
    pub(crate) prompts: Vec<String>,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub(crate) struct SkillMeta {
    pub(crate) name: String,
    pub(crate) description: String,
    #[serde(default = "default_version")]
    pub(crate) version: String,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub(crate) author: Option<String>,
    #[serde(default, skip_serializing_if = "Vec::is_empty")]
    pub(crate) tags: Vec<String>,
    /// Optional pointer to an external content file (markdown), relative
    /// to the SKILL.toml directory.  When present, the file's body is used
    /// as the skill's prompt content and the manifest's embedded
    /// `prompts = [...]` array is ignored.
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub(crate) content_file: Option<String>,
    /// Optional Kumiho kref pointing at this skill's published revision,
    /// e.g. `kref://CognitiveMemory/Skills/my-skill.skill?t=published`.
    /// Manifests written before the kind rename use a `.skilldef`
    /// suffix; both forms remain readable (see
    /// [`crate::skills::registration::LEGACY_SKILL_ITEM_KIND`]).
    /// Set by [`crate::skills::registration::register_skill_with_kumiho`]
    /// the first time a skill is installed; thereafter every install path
    /// (ClawHub, GitHub link, dashboard, creator.rs, daemon-startup scan)
    /// is idempotent because the manifest already carries the pointer.
    ///
    /// The `?t=published` query is what makes the manifest write-once
    /// across revisions: when SkillImprover creates a new revision and
    /// retags `published`, this pointer resolves to the new revision
    /// automatically without rewriting SKILL.toml.
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub(crate) kref: Option<String>,
}

#[derive(Debug, Clone, Default)]
struct SkillMarkdownMeta {
    name: Option<String>,
    description: Option<String>,
    version: Option<String>,
    author: Option<String>,
    tags: Vec<String>,
}

fn default_version() -> String {
    "0.1.0".to_string()
}

/// Emit a user-visible warning when a skill directory is skipped due to audit
/// findings. When the findings mention blocked scripts and `allow_scripts` is
/// `false`, the message includes actionable remediation guidance so users know
/// how to enable their skill.
fn warn_skipped_skill(path: &Path, summary: &str, allow_scripts: bool) {
    let scripts_blocked = summary.contains("script-like files are blocked");
    if scripts_blocked && !allow_scripts {
        tracing::warn!(
            "skipping skill directory {}: {summary}. \
             To allow script files in skills, set `skills.allow_scripts = true` in your config.",
            path.display(),
        );
        eprintln!(
            "warning: skill '{}' was skipped because it contains script files. \
             Set `skills.allow_scripts = true` in your construct config to enable it.",
            path.file_name()
                .map(|n| n.to_string_lossy().into_owned())
                .unwrap_or_else(|| path.display().to_string()),
        );
    } else {
        tracing::warn!(
            "skipping insecure skill directory {}: {summary}",
            path.display(),
        );
    }
}

/// Load all skills from the workspace skills directory
pub fn load_skills(workspace_dir: &Path) -> Vec<Skill> {
    load_skills_with_open_skills_config(workspace_dir, None, None, None)
}

/// Load skills using runtime config values (preferred at runtime).
pub fn load_skills_with_config(workspace_dir: &Path, config: &crate::config::Config) -> Vec<Skill> {
    load_skills_with_open_skills_config(
        workspace_dir,
        Some(config.skills.open_skills_enabled),
        config.skills.open_skills_dir.as_deref(),
        Some(config.skills.allow_scripts),
    )
}

/// Load skills using explicit open-skills settings.
pub fn load_skills_with_open_skills_settings(
    workspace_dir: &Path,
    open_skills_enabled: bool,
    open_skills_dir: Option<&str>,
) -> Vec<Skill> {
    load_skills_with_open_skills_config(
        workspace_dir,
        Some(open_skills_enabled),
        open_skills_dir,
        None,
    )
}

fn load_skills_with_open_skills_config(
    workspace_dir: &Path,
    config_open_skills_enabled: Option<bool>,
    config_open_skills_dir: Option<&str>,
    config_allow_scripts: Option<bool>,
) -> Vec<Skill> {
    let mut skills = Vec::new();
    let allow_scripts = config_allow_scripts.unwrap_or(false);

    if let Some(open_skills_dir) =
        ensure_open_skills_repo(config_open_skills_enabled, config_open_skills_dir)
    {
        skills.extend(load_open_skills(&open_skills_dir, allow_scripts));
    }

    skills.extend(load_workspace_skills(workspace_dir, allow_scripts));
    skills
}

fn load_workspace_skills(workspace_dir: &Path, allow_scripts: bool) -> Vec<Skill> {
    let skills_dir = workspace_dir.join("skills");
    load_skills_from_directory(&skills_dir, allow_scripts)
}

pub fn load_skills_from_directory(skills_dir: &Path, allow_scripts: bool) -> Vec<Skill> {
    if !skills_dir.exists() {
        return Vec::new();
    }

    let mut skills = Vec::new();

    let Ok(entries) = std::fs::read_dir(skills_dir) else {
        return skills;
    };

    for entry in entries.flatten() {
        let path = entry.path();
        if !path.is_dir() {
            continue;
        }

        match audit::audit_skill_directory_with_options(
            &path,
            audit::SkillAuditOptions { allow_scripts },
        ) {
            Ok(report) if report.is_clean() => {}
            Ok(report) => {
                let summary = report.summary();
                warn_skipped_skill(&path, &summary, allow_scripts);
                continue;
            }
            Err(err) => {
                tracing::warn!(
                    "skipping unauditable skill directory {}: {err}",
                    path.display()
                );
                continue;
            }
        }

        // Try SKILL.toml first, then SKILL.md
        let manifest_path = path.join("SKILL.toml");
        let md_path = path.join("SKILL.md");

        if manifest_path.exists() {
            if let Ok(skill) = load_skill_toml(&manifest_path) {
                skills.push(skill);
            }
        } else if md_path.exists() {
            if let Ok(skill) = load_skill_md(&md_path, &path) {
                skills.push(skill);
            }
        }
    }

    skills
}

fn finalize_open_skill(mut skill: Skill) -> Skill {
    if !skill.tags.iter().any(|tag| tag == "open-skills") {
        skill.tags.push("open-skills".to_string());
    }
    if skill.author.is_none() {
        skill.author = Some("besoeasy/open-skills".to_string());
    }
    skill
}

fn load_open_skills_from_directory(skills_dir: &Path, allow_scripts: bool) -> Vec<Skill> {
    if !skills_dir.exists() {
        return Vec::new();
    }

    let mut skills = Vec::new();

    let Ok(entries) = std::fs::read_dir(skills_dir) else {
        return skills;
    };

    for entry in entries.flatten() {
        let path = entry.path();
        if !path.is_dir() {
            continue;
        }

        match audit::audit_skill_directory_with_options(
            &path,
            audit::SkillAuditOptions { allow_scripts },
        ) {
            Ok(report) if report.is_clean() => {}
            Ok(report) => {
                let summary = report.summary();
                warn_skipped_skill(&path, &summary, allow_scripts);
                continue;
            }
            Err(err) => {
                tracing::warn!(
                    "skipping unauditable open-skill directory {}: {err}",
                    path.display()
                );
                continue;
            }
        }

        let manifest_path = path.join("SKILL.toml");
        let md_path = path.join("SKILL.md");

        if manifest_path.exists() {
            if let Ok(skill) = load_skill_toml(&manifest_path) {
                skills.push(finalize_open_skill(skill));
            }
        } else if md_path.exists() {
            if let Ok(skill) = load_open_skill_md(&md_path) {
                skills.push(skill);
            }
        }
    }

    skills
}

fn load_open_skills(repo_dir: &Path, allow_scripts: bool) -> Vec<Skill> {
    // Modern open-skills layout stores skill packages in `skills/<name>/SKILL.md`.
    // Prefer that structure to avoid treating repository docs (e.g. CONTRIBUTING.md)
    // as executable skills.
    let nested_skills_dir = repo_dir.join("skills");
    if nested_skills_dir.is_dir() {
        return load_open_skills_from_directory(&nested_skills_dir, allow_scripts);
    }

    let mut skills = Vec::new();

    let Ok(entries) = std::fs::read_dir(repo_dir) else {
        return skills;
    };

    for entry in entries.flatten() {
        let path = entry.path();
        if !path.is_file() {
            continue;
        }

        let is_markdown = path
            .extension()
            .and_then(|ext| ext.to_str())
            .is_some_and(|ext| ext.eq_ignore_ascii_case("md"));
        if !is_markdown {
            continue;
        }

        let is_readme = path
            .file_name()
            .and_then(|name| name.to_str())
            .is_some_and(|name| name.eq_ignore_ascii_case("README.md"));
        if is_readme {
            continue;
        }

        match audit::audit_open_skill_markdown(&path, repo_dir) {
            Ok(report) if report.is_clean() => {}
            Ok(report) => {
                tracing::warn!(
                    "skipping insecure open-skill file {}: {}",
                    path.display(),
                    report.summary()
                );
                continue;
            }
            Err(err) => {
                tracing::warn!(
                    "skipping unauditable open-skill file {}: {err}",
                    path.display()
                );
                continue;
            }
        }

        if let Ok(skill) = load_open_skill_md(&path) {
            skills.push(skill);
        }
    }

    skills
}

fn parse_open_skills_enabled(raw: &str) -> Option<bool> {
    match raw.trim().to_ascii_lowercase().as_str() {
        "1" | "true" | "yes" | "on" => Some(true),
        "0" | "false" | "no" | "off" => Some(false),
        _ => None,
    }
}

fn open_skills_enabled_from_sources(
    config_open_skills_enabled: Option<bool>,
    env_override: Option<&str>,
) -> bool {
    if let Some(raw) = env_override {
        if let Some(enabled) = parse_open_skills_enabled(raw) {
            return enabled;
        }
        if !raw.trim().is_empty() {
            tracing::warn!(
                "Ignoring invalid CONSTRUCT_OPEN_SKILLS_ENABLED (valid: 1|0|true|false|yes|no|on|off)"
            );
        }
    }

    config_open_skills_enabled.unwrap_or(false)
}

fn open_skills_enabled(config_open_skills_enabled: Option<bool>) -> bool {
    let env_override = std::env::var("CONSTRUCT_OPEN_SKILLS_ENABLED").ok();
    open_skills_enabled_from_sources(config_open_skills_enabled, env_override.as_deref())
}

fn resolve_open_skills_dir_from_sources(
    env_dir: Option<&str>,
    config_dir: Option<&str>,
    home_dir: Option<&Path>,
) -> Option<PathBuf> {
    let parse_dir = |raw: &str| {
        let trimmed = raw.trim();
        if trimmed.is_empty() {
            None
        } else {
            Some(PathBuf::from(trimmed))
        }
    };

    if let Some(env_dir) = env_dir.and_then(parse_dir) {
        return Some(env_dir);
    }
    if let Some(config_dir) = config_dir.and_then(parse_dir) {
        return Some(config_dir);
    }
    home_dir.map(|home| home.join("open-skills"))
}

fn resolve_open_skills_dir(config_open_skills_dir: Option<&str>) -> Option<PathBuf> {
    let env_dir = std::env::var("CONSTRUCT_OPEN_SKILLS_DIR").ok();
    let home_dir = UserDirs::new().map(|dirs| dirs.home_dir().to_path_buf());
    resolve_open_skills_dir_from_sources(
        env_dir.as_deref(),
        config_open_skills_dir,
        home_dir.as_deref(),
    )
}

fn ensure_open_skills_repo(
    config_open_skills_enabled: Option<bool>,
    config_open_skills_dir: Option<&str>,
) -> Option<PathBuf> {
    if !open_skills_enabled(config_open_skills_enabled) {
        return None;
    }

    let repo_dir = resolve_open_skills_dir(config_open_skills_dir)?;

    if !repo_dir.exists() {
        if !clone_open_skills_repo(&repo_dir) {
            return None;
        }
        let _ = mark_open_skills_synced(&repo_dir);
        return Some(repo_dir);
    }

    if should_sync_open_skills(&repo_dir) {
        if pull_open_skills_repo(&repo_dir) {
            let _ = mark_open_skills_synced(&repo_dir);
        } else {
            tracing::warn!(
                "open-skills update failed; using local copy from {}",
                repo_dir.display()
            );
        }
    }

    Some(repo_dir)
}

fn clone_open_skills_repo(repo_dir: &Path) -> bool {
    if let Some(parent) = repo_dir.parent() {
        if let Err(err) = std::fs::create_dir_all(parent) {
            tracing::warn!(
                "failed to create open-skills parent directory {}: {err}",
                parent.display()
            );
            return false;
        }
    }

    let output = Command::new("git")
        .args(["clone", "--depth", "1", OPEN_SKILLS_REPO_URL])
        .arg(repo_dir)
        .output();

    match output {
        Ok(result) if result.status.success() => {
            tracing::info!("initialized open-skills at {}", repo_dir.display());
            true
        }
        Ok(result) => {
            let stderr = String::from_utf8_lossy(&result.stderr);
            tracing::warn!("failed to clone open-skills: {stderr}");
            false
        }
        Err(err) => {
            tracing::warn!("failed to run git clone for open-skills: {err}");
            false
        }
    }
}

fn pull_open_skills_repo(repo_dir: &Path) -> bool {
    // If user points to a non-git directory via env var, keep using it without pulling.
    if !repo_dir.join(".git").exists() {
        return true;
    }

    let output = Command::new("git")
        .arg("-C")
        .arg(repo_dir)
        .args(["pull", "--ff-only"])
        .output();

    match output {
        Ok(result) if result.status.success() => true,
        Ok(result) => {
            let stderr = String::from_utf8_lossy(&result.stderr);
            tracing::warn!("failed to pull open-skills updates: {stderr}");
            false
        }
        Err(err) => {
            tracing::warn!("failed to run git pull for open-skills: {err}");
            false
        }
    }
}

fn should_sync_open_skills(repo_dir: &Path) -> bool {
    let marker = repo_dir.join(OPEN_SKILLS_SYNC_MARKER);
    let Ok(metadata) = std::fs::metadata(marker) else {
        return true;
    };
    let Ok(modified_at) = metadata.modified() else {
        return true;
    };
    let Ok(age) = SystemTime::now().duration_since(modified_at) else {
        return true;
    };

    age >= Duration::from_secs(OPEN_SKILLS_SYNC_INTERVAL_SECS)
}

fn mark_open_skills_synced(repo_dir: &Path) -> Result<()> {
    std::fs::write(repo_dir.join(OPEN_SKILLS_SYNC_MARKER), b"synced")?;
    Ok(())
}

/// Load a skill from a SKILL.toml manifest.
///
/// Resolution order for the prompt content:
/// 1. **`[skill].content_file`** — external markdown file relative to
///    the SKILL.toml directory.  When set, its body is used as the
///    single prompt entry (the manifest's `prompts = [...]` array is
///    ignored).  If the file is missing or unreadable we log a warning
///    and fall back to the embedded prompts so the skill is still usable.
/// 2. **embedded `prompts = [...]`** — legacy path used by skills that
///    haven't been migrated yet.
fn load_skill_toml(path: &Path) -> Result<Skill> {
    let content = std::fs::read_to_string(path)?;
    let manifest: SkillManifest = toml::from_str(&content)?;

    let prompts = resolve_skill_prompts(path, &manifest);

    Ok(Skill {
        name: manifest.skill.name,
        description: manifest.skill.description,
        version: manifest.skill.version,
        author: manifest.skill.author,
        tags: manifest.skill.tags,
        tools: manifest.tools,
        prompts,
        location: Some(path.to_path_buf()),
    })
}

/// Resolve the prompts for a skill manifest.  Kept separate from
/// [`load_skill_toml`] so callers (and unit tests) can exercise the
/// precedence rules without rewriting the loader.
fn resolve_skill_prompts(manifest_path: &Path, manifest: &SkillManifest) -> Vec<String> {
    let Some(rel) = manifest.skill.content_file.as_deref() else {
        return manifest.prompts.clone();
    };

    let manifest_dir = manifest_path
        .parent()
        .map(Path::to_path_buf)
        .unwrap_or_else(|| PathBuf::from("."));
    let content_path = manifest_dir.join(rel);

    match std::fs::read_to_string(&content_path) {
        Ok(body) => {
            if !manifest.prompts.is_empty() {
                tracing::warn!(
                    skill_manifest = %manifest_path.display(),
                    content_file = %content_path.display(),
                    "SKILL.toml has both `content_file` and `prompts = [...]`; \
                     using content_file and ignoring embedded prompts",
                );
            }
            vec![body]
        }
        Err(err) => {
            tracing::warn!(
                skill_manifest = %manifest_path.display(),
                content_file = %content_path.display(),
                error = %err,
                "SKILL.toml content_file unreadable; falling back to embedded prompts",
            );
            manifest.prompts.clone()
        }
    }
}

/// Outcome of [`migrate_skill_toml_to_content_file`].
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum SkillContentMigration {
    /// SKILL.toml already has a `content_file` pointer — no work to do.
    AlreadyMigrated,
    /// Manifest had no embedded prompts to migrate.
    NothingToMigrate,
    /// Successfully extracted prompts to a new content file and updated
    /// the manifest in place.
    Migrated {
        manifest: PathBuf,
        content_file: PathBuf,
    },
}

/// Migrate a legacy SKILL.toml (embedded `prompts = [...]`) into the
/// new content-file shape:
///
/// * Joins the prompts array into a single markdown body (entries
///   separated by a blank line).
/// * Writes the body to `<skill_dir>/contents/r1-<YYYY-MM-DD>.md`.
/// * Rewrites the manifest in place with `[skill].content_file =
///   "contents/r1-<YYYY-MM-DD>.md"` and the `prompts` table removed.
///
/// Idempotent: when the manifest already has `content_file` set the
/// function returns [`SkillContentMigration::AlreadyMigrated`] without
/// touching disk.  When the manifest has no prompts to migrate it
/// returns [`SkillContentMigration::NothingToMigrate`].
///
/// Used by an admin / CLI tool — not called from the hot path.  Step 6b
/// will add Kumiho revisioning on top of the migrated layout; doing the
/// disk-side split as its own step keeps the rollout cheap to verify.
pub fn migrate_skill_toml_to_content_file(manifest_path: &Path) -> Result<SkillContentMigration> {
    let raw = std::fs::read_to_string(manifest_path)
        .with_context(|| format!("reading SKILL.toml manifest: {}", manifest_path.display()))?;
    let manifest: SkillManifest = toml::from_str(&raw)
        .with_context(|| format!("parsing SKILL.toml manifest: {}", manifest_path.display()))?;

    if manifest.skill.content_file.is_some() {
        return Ok(SkillContentMigration::AlreadyMigrated);
    }
    if manifest.prompts.is_empty() {
        return Ok(SkillContentMigration::NothingToMigrate);
    }

    let manifest_dir = manifest_path
        .parent()
        .map(Path::to_path_buf)
        .unwrap_or_else(|| PathBuf::from("."));
    let contents_dir = manifest_dir.join("contents");
    std::fs::create_dir_all(&contents_dir)
        .with_context(|| format!("creating contents/ directory: {}", contents_dir.display()))?;

    let date_stamp = chrono::Utc::now().format("%Y-%m-%d");
    let rel_filename = format!("contents/r1-{date_stamp}.md");
    let content_path = manifest_dir.join(&rel_filename);

    // Body: one prompt per paragraph, blank line between, trailing newline.
    let mut body = String::new();
    for (i, p) in manifest.prompts.iter().enumerate() {
        if i > 0 {
            body.push_str("\n\n");
        }
        body.push_str(p.trim_end());
    }
    if !body.ends_with('\n') {
        body.push('\n');
    }

    std::fs::write(&content_path, body.as_bytes())
        .with_context(|| format!("writing content file: {}", content_path.display()))?;

    // Rebuild the manifest with content_file set + prompts removed.
    let mut new_manifest = manifest.clone();
    new_manifest.skill.content_file = Some(rel_filename.clone());
    new_manifest.prompts = Vec::new();

    let serialized = toml::to_string_pretty(&new_manifest)
        .context("serializing migrated SKILL.toml manifest")?;
    std::fs::write(manifest_path, serialized.as_bytes()).with_context(|| {
        format!(
            "writing migrated SKILL.toml manifest: {}",
            manifest_path.display()
        )
    })?;

    Ok(SkillContentMigration::Migrated {
        manifest: manifest_path.to_path_buf(),
        content_file: content_path,
    })
}

/// Load a skill from a SKILL.md file (simpler format)
fn load_skill_md(path: &Path, dir: &Path) -> Result<Skill> {
    let content = std::fs::read_to_string(path)?;
    let parsed = parse_skill_markdown(&content);
    let name = dir
        .file_name()
        .and_then(|n| n.to_str())
        .unwrap_or("unknown")
        .to_string();

    Ok(Skill {
        name: parsed.meta.name.unwrap_or(name),
        description: parsed
            .meta
            .description
            .filter(|value| !value.trim().is_empty())
            .unwrap_or_else(|| extract_description(&parsed.body)),
        version: parsed.meta.version.unwrap_or_else(default_version),
        author: parsed.meta.author,
        tags: parsed.meta.tags,
        tools: Vec::new(),
        prompts: vec![parsed.body],
        location: Some(path.to_path_buf()),
    })
}

fn load_open_skill_md(path: &Path) -> Result<Skill> {
    let content = std::fs::read_to_string(path)?;
    let parsed = parse_skill_markdown(&content);
    let file_stem = path
        .file_stem()
        .and_then(|n| n.to_str())
        .unwrap_or("open-skill")
        .to_string();
    let name = if file_stem.eq_ignore_ascii_case("skill") {
        path.parent()
            .and_then(|dir| dir.file_name())
            .and_then(|name| name.to_str())
            .unwrap_or(&file_stem)
            .to_string()
    } else {
        file_stem
    };
    Ok(finalize_open_skill(Skill {
        name: parsed.meta.name.unwrap_or(name),
        description: parsed
            .meta
            .description
            .filter(|value| !value.trim().is_empty())
            .unwrap_or_else(|| extract_description(&parsed.body)),
        version: parsed
            .meta
            .version
            .unwrap_or_else(|| "open-skills".to_string()),
        author: parsed
            .meta
            .author
            .or_else(|| Some("besoeasy/open-skills".to_string())),
        tags: parsed.meta.tags,
        tools: Vec::new(),
        prompts: vec![parsed.body],
        location: Some(path.to_path_buf()),
    }))
}

struct ParsedSkillMarkdown {
    meta: SkillMarkdownMeta,
    body: String,
}

fn parse_skill_markdown(content: &str) -> ParsedSkillMarkdown {
    if let Some((frontmatter, body)) = split_skill_frontmatter(content) {
        let meta = parse_simple_frontmatter(&frontmatter);
        return ParsedSkillMarkdown { meta, body };
    }

    ParsedSkillMarkdown {
        meta: SkillMarkdownMeta::default(),
        body: content.to_string(),
    }
}

/// Lightweight YAML-like frontmatter parser for simple `key: value` pairs.
/// Replaces `serde_yaml` to avoid pulling in the full YAML parser (~30KB)
/// for a struct with only 5 optional string fields.
fn parse_simple_frontmatter(s: &str) -> SkillMarkdownMeta {
    let mut meta = SkillMarkdownMeta::default();
    let mut collecting_tags = false;
    for line in s.lines() {
        // Handle YAML list items under `tags:` (e.g. "  - parser")
        if collecting_tags {
            let trimmed = line.trim();
            if let Some(item) = trimmed.strip_prefix("- ") {
                let tag = item.trim().trim_matches('"').trim_matches('\'');
                if !tag.is_empty() {
                    meta.tags.push(tag.to_string());
                }
                continue;
            }
            // Non-list-item line → stop collecting tags
            collecting_tags = false;
        }
        let Some((key, val)) = line.split_once(':') else {
            continue;
        };
        let key = key.trim();
        let val = val.trim().trim_matches('"').trim_matches('\'');
        match key {
            "name" => meta.name = Some(val.to_string()),
            "description" => meta.description = Some(val.to_string()),
            "version" => meta.version = Some(val.to_string()),
            "author" => meta.author = Some(val.to_string()),
            "tags" => {
                if val.is_empty() {
                    // YAML block list follows on subsequent lines
                    collecting_tags = true;
                } else {
                    // Inline: [a, b, c] or comma-separated
                    let val = val.trim_start_matches('[').trim_end_matches(']');
                    meta.tags = val
                        .split(',')
                        .map(|t| t.trim().trim_matches('"').trim_matches('\'').to_string())
                        .filter(|t| !t.is_empty())
                        .collect();
                }
            }
            _ => {}
        }
    }
    meta
}

fn split_skill_frontmatter(content: &str) -> Option<(String, String)> {
    let normalized = content.replace("\r\n", "\n");
    let rest = normalized.strip_prefix("---\n")?;
    if let Some(idx) = rest.find("\n---\n") {
        let frontmatter = rest[..idx].to_string();
        let body = rest[idx + 5..].to_string();
        return Some((frontmatter, body));
    }
    if let Some(frontmatter) = rest.strip_suffix("\n---") {
        return Some((frontmatter.to_string(), String::new()));
    }
    None
}

fn extract_description(content: &str) -> String {
    content
        .lines()
        .find(|line| !line.starts_with('#') && !line.trim().is_empty())
        .unwrap_or("No description")
        .trim()
        .to_string()
}

fn append_xml_escaped(out: &mut String, text: &str) {
    for ch in text.chars() {
        match ch {
            '&' => out.push_str("&amp;"),
            '<' => out.push_str("&lt;"),
            '>' => out.push_str("&gt;"),
            '"' => out.push_str("&quot;"),
            '\'' => out.push_str("&apos;"),
            _ => out.push(ch),
        }
    }
}

fn write_xml_text_element(out: &mut String, indent: usize, tag: &str, value: &str) {
    for _ in 0..indent {
        out.push(' ');
    }
    out.push('<');
    out.push_str(tag);
    out.push('>');
    append_xml_escaped(out, value);
    out.push_str("</");
    out.push_str(tag);
    out.push_str(">\n");
}

fn resolve_skill_location(skill: &Skill, workspace_dir: &Path) -> PathBuf {
    skill.location.clone().unwrap_or_else(|| {
        workspace_dir
            .join("skills")
            .join(&skill.name)
            .join("SKILL.md")
    })
}

fn render_skill_location(skill: &Skill, workspace_dir: &Path, prefer_relative: bool) -> String {
    let location = resolve_skill_location(skill, workspace_dir);
    if prefer_relative {
        if let Ok(relative) = location.strip_prefix(workspace_dir) {
            return relative.display().to_string();
        }
    }
    location.display().to_string()
}

/// Build the "Available Skills" system prompt section with full skill instructions.
pub fn skills_to_prompt(skills: &[Skill], workspace_dir: &Path) -> String {
    skills_to_prompt_with_mode(
        skills,
        workspace_dir,
        crate::config::SkillsPromptInjectionMode::Full,
    )
}

/// Build the "Available Skills" prompt section, optionally reranking skills by
/// effectiveness score before injection.
///
/// `effectiveness` defaults to a no-op provider (preserves existing static
/// load order).  Pass a real provider — typically a daemon-level cache fed by
/// `get_skill_effectiveness` from the operator MCP — to bubble high-success
/// skills to the top of the list, giving the LLM a clearer signal about what
/// has worked recently.  Sort is stable for skills tied on score.
pub fn skills_to_prompt_with_mode_and_effectiveness(
    skills: &[Skill],
    workspace_dir: &Path,
    mode: crate::config::SkillsPromptInjectionMode,
    effectiveness: &dyn SkillEffectivenessProvider,
) -> String {
    if skills.is_empty() {
        return String::new();
    }

    // Build a sorted Vec<&Skill> rather than cloning the underlying skills.
    let mut ordered: Vec<&Skill> = skills.iter().collect();
    // Stable sort by descending effectiveness sort_key.  Skills with no score
    // fall back to the neutral 0.5 default — see EffectivenessScore::NEUTRAL.
    ordered.sort_by(|a, b| {
        let a_key = effectiveness
            .score(&a.name)
            .map(|s| s.sort_key())
            .unwrap_or((EffectivenessScore::NEUTRAL, 0));
        let b_key = effectiveness
            .score(&b.name)
            .map(|s| s.sort_key())
            .unwrap_or((EffectivenessScore::NEUTRAL, 0));
        // Reverse so higher key sorts first.  Total-order via partial_cmp on
        // (f64, u32); the f64 part is always finite (rate ∈ [0,1] or NEUTRAL).
        b_key
            .partial_cmp(&a_key)
            .unwrap_or(std::cmp::Ordering::Equal)
    });

    render_skills_prompt(&ordered, workspace_dir, mode)
}

/// Build the "Available Skills" system prompt section with configurable verbosity.
///
/// Backwards-compatible entry point that preserves the static load order of
/// skills.  For effectiveness-weighted reranking call
/// [`skills_to_prompt_with_mode_and_effectiveness`].
pub fn skills_to_prompt_with_mode(
    skills: &[Skill],
    workspace_dir: &Path,
    mode: crate::config::SkillsPromptInjectionMode,
) -> String {
    if skills.is_empty() {
        return String::new();
    }
    let ordered: Vec<&Skill> = skills.iter().collect();
    render_skills_prompt(&ordered, workspace_dir, mode)
}

/// Render the `<available_skills>` block for an already-ordered list of
/// skill references.  Both public entry points (with and without
/// effectiveness reranking) share this body so the XML output stays
/// identical when no scores are available.
fn render_skills_prompt(
    skills: &[&Skill],
    workspace_dir: &Path,
    mode: crate::config::SkillsPromptInjectionMode,
) -> String {
    use std::fmt::Write;

    if skills.is_empty() {
        return String::new();
    }

    let mut prompt = match mode {
        crate::config::SkillsPromptInjectionMode::Full => String::from(
            "## Available Skills\n\n\
             Skill instructions and tool metadata are preloaded below.\n\
             Follow these instructions directly; do not read skill files at runtime unless the user asks.\n\n\
             <available_skills>\n",
        ),
        crate::config::SkillsPromptInjectionMode::Compact => String::from(
            "## Available Skills\n\n\
             Skill summaries are preloaded below to keep context compact.\n\
             Skill instructions are loaded on demand: call `read_skill(name)` with the skill's `<name>` when you need the full skill file.\n\
             The `location` field is included for reference.\n\n\
             <available_skills>\n",
        ),
    };

    for skill in skills {
        let _ = writeln!(prompt, "  <skill>");
        write_xml_text_element(&mut prompt, 4, "name", &skill.name);
        write_xml_text_element(&mut prompt, 4, "description", &skill.description);
        let location = render_skill_location(
            skill,
            workspace_dir,
            matches!(mode, crate::config::SkillsPromptInjectionMode::Compact),
        );
        write_xml_text_element(&mut prompt, 4, "location", &location);

        // In Full mode, inline both instructions and tools.
        // In Compact mode, skip instructions (loaded on demand) but keep tools
        // so the LLM knows which skill tools are available.
        if matches!(mode, crate::config::SkillsPromptInjectionMode::Full)
            && !skill.prompts.is_empty()
        {
            let _ = writeln!(prompt, "    <instructions>");
            for instruction in &skill.prompts {
                write_xml_text_element(&mut prompt, 6, "instruction", instruction);
            }
            let _ = writeln!(prompt, "    </instructions>");
        }

        if !skill.tools.is_empty() {
            // Tools with known kinds (shell, script, http) are registered as
            // callable tool specs and can be invoked directly via function calling.
            // We note them here for context but mark them as callable.
            let registered: Vec<_> = skill
                .tools
                .iter()
                .filter(|t| matches!(t.kind.as_str(), "shell" | "script" | "http"))
                .collect();
            let unregistered: Vec<_> = skill
                .tools
                .iter()
                .filter(|t| !matches!(t.kind.as_str(), "shell" | "script" | "http"))
                .collect();

            if !registered.is_empty() {
                let _ = writeln!(
                    prompt,
                    "    <callable_tools hint=\"These are registered as callable tool specs. Invoke them directly by name ({{}}.{{}}) instead of using shell.\">"
                );
                for tool in &registered {
                    let _ = writeln!(prompt, "      <tool>");
                    write_xml_text_element(
                        &mut prompt,
                        8,
                        "name",
                        &format!("{}.{}", skill.name, tool.name),
                    );
                    write_xml_text_element(&mut prompt, 8, "description", &tool.description);
                    let _ = writeln!(prompt, "      </tool>");
                }
                let _ = writeln!(prompt, "    </callable_tools>");
            }

            if !unregistered.is_empty() {
                let _ = writeln!(prompt, "    <tools>");
                for tool in &unregistered {
                    let _ = writeln!(prompt, "      <tool>");
                    write_xml_text_element(&mut prompt, 8, "name", &tool.name);
                    write_xml_text_element(&mut prompt, 8, "description", &tool.description);
                    write_xml_text_element(&mut prompt, 8, "kind", &tool.kind);
                    let _ = writeln!(prompt, "      </tool>");
                }
                let _ = writeln!(prompt, "    </tools>");
            }
        }

        let _ = writeln!(prompt, "  </skill>");
    }

    prompt.push_str("</available_skills>");
    prompt
}

/// Convert skill tools into callable `Tool` trait objects.
///
/// Each skill's `[[tools]]` entries are converted to either `SkillShellTool`
/// (for `shell`/`script` kinds) or `SkillHttpTool` (for `http` kind),
/// enabling them to appear as first-class callable tool specs rather than
/// only as XML in the system prompt.
pub fn skills_to_tools(
    skills: &[Skill],
    security: std::sync::Arc<crate::security::SecurityPolicy>,
) -> Vec<Box<dyn crate::tools::traits::Tool>> {
    let mut tools: Vec<Box<dyn crate::tools::traits::Tool>> = Vec::new();
    for skill in skills {
        for tool in &skill.tools {
            match tool.kind.as_str() {
                "shell" | "script" => {
                    tools.push(Box::new(crate::tools::skill_tool::SkillShellTool::new(
                        &skill.name,
                        tool,
                        security.clone(),
                    )));
                }
                "http" => {
                    tools.push(Box::new(crate::tools::skill_http::SkillHttpTool::new(
                        &skill.name,
                        tool,
                    )));
                }
                other => {
                    tracing::warn!(
                        "Unknown skill tool kind '{}' for {}.{}, skipping",
                        other,
                        skill.name,
                        tool.name
                    );
                }
            }
        }
    }
    tools
}

/// Get the skills directory path
pub fn skills_dir(workspace_dir: &Path) -> PathBuf {
    workspace_dir.join("skills")
}

/// Initialize the skills directory with a README
pub fn init_skills_dir(workspace_dir: &Path) -> Result<()> {
    let dir = skills_dir(workspace_dir);
    std::fs::create_dir_all(&dir)?;

    let readme = dir.join("README.md");
    if !readme.exists() {
        std::fs::write(
            &readme,
            "# Construct Skills\n\n\
             Each subdirectory is a skill. Create a `SKILL.toml` or `SKILL.md` file inside.\n\n\
             ## SKILL.toml format\n\n\
             ```toml\n\
             [skill]\n\
             name = \"my-skill\"\n\
             description = \"What this skill does\"\n\
             version = \"0.1.0\"\n\
             author = \"your-name\"\n\
             tags = [\"productivity\", \"automation\"]\n\n\
             [[tools]]\n\
             name = \"my_tool\"\n\
             description = \"What this tool does\"\n\
             kind = \"shell\"\n\
             command = \"echo hello\"\n\
             ```\n\n\
             ## SKILL.md format (simpler)\n\n\
             Just write a markdown file with instructions for the agent.\n\
             Optional YAML frontmatter is supported for `name`, `description`, `version`, `author`, and `tags`.\n\
             The agent will read it and follow the instructions.\n\n\
             ## Installing community skills\n\n\
             ```bash\n\
             construct skills install <source>\n\
             construct skills list\n\
             ```\n",
        )?;
    }

    Ok(())
}

fn is_clawhub_host(host: &str) -> bool {
    host.eq_ignore_ascii_case(CLAWHUB_DOMAIN) || host.eq_ignore_ascii_case(CLAWHUB_WWW_DOMAIN)
}

fn parse_clawhub_url(source: &str) -> Option<Url> {
    let parsed = Url::parse(source).ok()?;
    match parsed.scheme() {
        "https" | "http" => {}
        _ => return None,
    }

    if !parsed.host_str().is_some_and(is_clawhub_host) {
        return None;
    }

    Some(parsed)
}

fn is_clawhub_source(source: &str) -> bool {
    if source.starts_with("clawhub:") {
        return true;
    }
    parse_clawhub_url(source).is_some()
}

fn clawhub_download_url(source: &str) -> Result<String> {
    // Short prefix: clawhub:<slug>
    if let Some(slug) = source.strip_prefix("clawhub:") {
        let slug = slug.trim().trim_end_matches('/');
        if slug.is_empty() || slug.contains('/') {
            anyhow::bail!(
                "invalid clawhub source '{}': expected 'clawhub:<slug>' (no slashes in slug)",
                source
            );
        }
        return Ok(format!("{CLAWHUB_DOWNLOAD_API}?slug={slug}"));
    }

    // Profile URL: https://clawhub.ai/<owner>/<slug> or https://www.clawhub.ai/<slug>
    if let Some(parsed) = parse_clawhub_url(source) {
        let path = parsed
            .path_segments()
            .into_iter()
            .flatten()
            .collect::<Vec<_>>()
            .join("/");

        if path.is_empty() {
            anyhow::bail!("could not extract slug from ClawhHub URL: {source}");
        }

        return Ok(format!("{CLAWHUB_DOWNLOAD_API}?slug={path}"));
    }

    anyhow::bail!("unrecognised ClawhHub source format: {source}")
}

fn normalize_skill_name(s: &str) -> String {
    s.to_lowercase()
        .chars()
        .map(|c| if c == '-' { '_' } else { c })
        .filter(|c| c.is_ascii_alphanumeric() || *c == '_')
        .collect()
}

fn clawhub_skill_dir_name(source: &str) -> Result<String> {
    if let Some(slug) = source.strip_prefix("clawhub:") {
        let slug = slug.trim().trim_end_matches('/');
        let base = slug.rsplit('/').next().unwrap_or(slug);
        let name = normalize_skill_name(base);
        return Ok(if name.is_empty() {
            "skill".to_string()
        } else {
            name
        });
    }

    let parsed = parse_clawhub_url(source)
        .ok_or_else(|| anyhow::anyhow!("invalid clawhub URL: {source}"))?;

    let path = parsed
        .path_segments()
        .into_iter()
        .flatten()
        .collect::<Vec<_>>();

    let base = path.last().copied().unwrap_or("skill");
    let name = normalize_skill_name(base);
    Ok(if name.is_empty() {
        "skill".to_string()
    } else {
        name
    })
}

fn is_git_source(source: &str) -> bool {
    // ClawHub URLs look like https:// but are not git repos
    if is_clawhub_source(source) {
        return false;
    }
    is_git_scheme_source(source, "https://")
        || is_git_scheme_source(source, "http://")
        || is_git_scheme_source(source, "ssh://")
        || is_git_scheme_source(source, "git://")
        || is_git_scp_source(source)
}

fn is_git_scheme_source(source: &str, scheme: &str) -> bool {
    let Some(rest) = source.strip_prefix(scheme) else {
        return false;
    };
    if rest.is_empty() || rest.starts_with('/') {
        return false;
    }

    let host = rest.split(['/', '?', '#']).next().unwrap_or_default();
    !host.is_empty()
}

fn is_git_scp_source(source: &str) -> bool {
    // SCP-like syntax accepted by git, e.g. git@host:owner/repo.git
    // Keep this strict enough to avoid treating local paths as git remotes.
    let Some((user_host, remote_path)) = source.split_once(':') else {
        return false;
    };
    if remote_path.is_empty() {
        return false;
    }
    if source.contains("://") {
        return false;
    }

    let Some((user, host)) = user_host.split_once('@') else {
        return false;
    };
    !user.is_empty()
        && !host.is_empty()
        && !user.contains('/')
        && !user.contains('\\')
        && !host.contains('/')
        && !host.contains('\\')
}

fn snapshot_skill_children(skills_path: &Path) -> Result<HashSet<PathBuf>> {
    let mut paths = HashSet::new();
    for entry in std::fs::read_dir(skills_path)? {
        let entry = entry?;
        paths.insert(entry.path());
    }
    Ok(paths)
}

fn detect_newly_installed_directory(
    skills_path: &Path,
    before: &HashSet<PathBuf>,
) -> Result<PathBuf> {
    let mut created = Vec::new();
    for entry in std::fs::read_dir(skills_path)? {
        let entry = entry?;
        let path = entry.path();
        if !before.contains(&path) && path.is_dir() {
            created.push(path);
        }
    }

    match created.len() {
        1 => Ok(created.remove(0)),
        0 => anyhow::bail!(
            "Unable to determine installed skill directory after clone (no new directory found)"
        ),
        _ => anyhow::bail!(
            "Unable to determine installed skill directory after clone (multiple new directories found)"
        ),
    }
}

fn enforce_skill_security_audit(
    skill_path: &Path,
    allow_scripts: bool,
) -> Result<audit::SkillAuditReport> {
    let report = audit::audit_skill_directory_with_options(
        skill_path,
        audit::SkillAuditOptions { allow_scripts },
    )?;
    if report.is_clean() {
        return Ok(report);
    }

    anyhow::bail!("Skill security audit failed: {}", report.summary());
}

fn remove_git_metadata(skill_path: &Path) -> Result<()> {
    let git_dir = skill_path.join(".git");
    if git_dir.exists() {
        std::fs::remove_dir_all(&git_dir)
            .with_context(|| format!("failed to remove {}", git_dir.display()))?;
    }
    Ok(())
}

fn copy_dir_recursive_secure(src: &Path, dest: &Path) -> Result<()> {
    let src_meta = std::fs::symlink_metadata(src)
        .with_context(|| format!("failed to read metadata for {}", src.display()))?;
    if src_meta.file_type().is_symlink() {
        anyhow::bail!(
            "Refusing to copy symlinked skill source path: {}",
            src.display()
        );
    }
    if !src_meta.is_dir() {
        anyhow::bail!("Skill source must be a directory: {}", src.display());
    }

    std::fs::create_dir_all(dest)
        .with_context(|| format!("failed to create destination {}", dest.display()))?;
    for entry in std::fs::read_dir(src)? {
        let entry = entry?;
        let src_path = entry.path();
        let dest_path = dest.join(entry.file_name());
        let metadata = std::fs::symlink_metadata(&src_path)
            .with_context(|| format!("failed to read metadata for {}", src_path.display()))?;

        if metadata.file_type().is_symlink() {
            anyhow::bail!(
                "Refusing to copy symlink within skill source: {}",
                src_path.display()
            );
        }

        if metadata.is_dir() {
            copy_dir_recursive_secure(&src_path, &dest_path)?;
        } else if metadata.is_file() {
            std::fs::copy(&src_path, &dest_path).with_context(|| {
                format!(
                    "failed to copy skill file from {} to {}",
                    src_path.display(),
                    dest_path.display()
                )
            })?;
        }
    }

    Ok(())
}

fn install_local_skill_source(
    source: &str,
    skills_path: &Path,
    allow_scripts: bool,
) -> Result<(PathBuf, usize)> {
    let source_path = PathBuf::from(source);
    if !source_path.exists() {
        anyhow::bail!("Source path does not exist: {source}");
    }

    let source_path = source_path
        .canonicalize()
        .with_context(|| format!("failed to canonicalize source path {source}"))?;
    let _ = enforce_skill_security_audit(&source_path, allow_scripts)?;

    let name = source_path
        .file_name()
        .context("Source path must include a directory name")?;
    let dest = skills_path.join(name);
    if dest.exists() {
        anyhow::bail!("Destination skill already exists: {}", dest.display());
    }

    if let Err(err) = copy_dir_recursive_secure(&source_path, &dest) {
        let _ = std::fs::remove_dir_all(&dest);
        return Err(err);
    }

    match enforce_skill_security_audit(&dest, allow_scripts) {
        Ok(report) => Ok((dest, report.files_scanned)),
        Err(err) => {
            let _ = std::fs::remove_dir_all(&dest);
            Err(err)
        }
    }
}

fn install_git_skill_source(
    source: &str,
    skills_path: &Path,
    allow_scripts: bool,
) -> Result<(PathBuf, usize)> {
    let before = snapshot_skill_children(skills_path)?;
    let output = std::process::Command::new("git")
        .args(["clone", "--depth", "1", source])
        .current_dir(skills_path)
        .output()?;
    if !output.status.success() {
        let stderr = String::from_utf8_lossy(&output.stderr);
        anyhow::bail!("Git clone failed: {stderr}");
    }

    let installed_dir = detect_newly_installed_directory(skills_path, &before)?;
    remove_git_metadata(&installed_dir)?;
    match enforce_skill_security_audit(&installed_dir, allow_scripts) {
        Ok(report) => Ok((installed_dir, report.files_scanned)),
        Err(err) => {
            let _ = std::fs::remove_dir_all(&installed_dir);
            Err(err)
        }
    }
}

fn install_clawhub_skill_source(
    source: &str,
    skills_path: &Path,
    allow_scripts: bool,
) -> Result<(PathBuf, usize)> {
    let download_url = clawhub_download_url(source)
        .with_context(|| format!("invalid ClawhHub source: {source}"))?;
    let skill_dir_name = clawhub_skill_dir_name(source)?;
    let installed_dir = skills_path.join(&skill_dir_name);
    if installed_dir.exists() {
        anyhow::bail!(
            "Destination skill already exists: {}",
            installed_dir.display()
        );
    }

    let client = reqwest::blocking::Client::builder()
        .timeout(Duration::from_secs(30))
        .build()?;

    let resp = client
        .get(&download_url)
        .send()
        .with_context(|| format!("failed to fetch zip from {download_url}"))?;

    if resp.status() == reqwest::StatusCode::TOO_MANY_REQUESTS {
        anyhow::bail!("ClawhHub rate limit reached (HTTP 429). Wait a moment and retry.");
    }
    if !resp.status().is_success() {
        anyhow::bail!("ClawhHub download failed (HTTP {})", resp.status());
    }

    let bytes = resp.bytes()?.to_vec();
    if bytes.len() as u64 > MAX_CLAWHUB_ZIP_BYTES {
        anyhow::bail!(
            "ClawhHub zip rejected: too large ({} bytes > {})",
            bytes.len(),
            MAX_CLAWHUB_ZIP_BYTES
        );
    }

    std::fs::create_dir_all(&installed_dir)?;

    let cursor = Cursor::new(bytes);
    let mut archive = ZipArchive::new(cursor).context("downloaded content is not a valid zip")?;

    for i in 0..archive.len() {
        let mut entry = archive.by_index(i)?;
        let raw_name = entry.name().to_string();

        if raw_name.is_empty()
            || raw_name.contains("..")
            || raw_name.starts_with('/')
            || raw_name.contains('\\')
            || raw_name.contains(':')
        {
            let _ = std::fs::remove_dir_all(&installed_dir);
            anyhow::bail!("zip entry contains unsafe path: {raw_name}");
        }

        let out_path = installed_dir.join(&raw_name);
        if entry.is_dir() {
            std::fs::create_dir_all(&out_path)?;
            continue;
        }

        if let Some(parent) = out_path.parent() {
            std::fs::create_dir_all(parent)?;
        }

        let mut out_file = std::fs::File::create(&out_path)
            .with_context(|| format!("failed to create extracted file: {}", out_path.display()))?;
        std::io::copy(&mut entry, &mut out_file)?;
    }

    let has_manifest =
        installed_dir.join("SKILL.md").exists() || installed_dir.join("SKILL.toml").exists();
    if !has_manifest {
        std::fs::write(
            installed_dir.join("SKILL.toml"),
            format!(
                "[skill]\nname = \"{}\"\ndescription = \"ClawhHub installed skill\"\nversion = \"0.1.0\"\n",
                skill_dir_name
            ),
        )?;
    }

    match enforce_skill_security_audit(&installed_dir, allow_scripts) {
        Ok(report) => Ok((installed_dir, report.files_scanned)),
        Err(err) => {
            let _ = std::fs::remove_dir_all(&installed_dir);
            Err(err)
        }
    }
}

/// Handle the `skills` CLI command
#[allow(clippy::too_many_lines)]
/// Best-effort Kumiho registration after a CLI skill install.
///
/// `handle_command` is synchronous (CLI entry point — no surrounding
/// tokio runtime), so we spin up a single-thread runtime just for this
/// async call.  Any failure here is logged + printed but does NOT
/// propagate: the install itself already succeeded on disk, and the
/// daemon-startup scan in `src/gateway/mod.rs::run_gateway` will retry
/// registration on next daemon start.
fn register_after_install_best_effort(installed_dir: &Path, config: &crate::config::Config) {
    let runtime = match tokio::runtime::Builder::new_current_thread()
        .enable_all()
        .build()
    {
        Ok(rt) => rt,
        Err(e) => {
            tracing::warn!(
                error = %e,
                skill_dir = %installed_dir.display(),
                "skill install: could not start tokio runtime for Kumiho registration; \
                 daemon-startup scan will retry",
            );
            return;
        }
    };

    let memory_project = config.kumiho.memory_project.clone();
    let kumiho_client = crate::gateway::kumiho_client::build_client_from_config(config);

    let result = runtime.block_on(registration::register_skill_with_kumiho(
        installed_dir,
        &kumiho_client,
        &memory_project,
    ));

    match result {
        Ok(registration::SkillRegistration::AlreadyRegistered { kref }) => {
            println!(
                "  {} Already registered with Kumiho: {}",
                console::style("·").dim(),
                kref
            );
        }
        Ok(registration::SkillRegistration::Registered { kref, .. }) => {
            println!(
                "  {} Registered with Kumiho: {}",
                console::style("✓").green().bold(),
                kref
            );
        }
        Err(e) => {
            println!(
                "  {} Could not register with Kumiho yet: {e}",
                console::style("⚠").yellow().bold(),
            );
            println!(
                "    The skill is installed and will be picked up by the daemon's \
                 startup scan on next start."
            );
            tracing::warn!(
                error = ?e,
                skill_dir = %installed_dir.display(),
                "skill install: Kumiho registration deferred",
            );
        }
    }
}

pub fn handle_command(command: crate::SkillCommands, config: &crate::config::Config) -> Result<()> {
    let workspace_dir = &config.workspace_dir;
    match command {
        crate::SkillCommands::List => {
            let skills = load_skills_with_config(workspace_dir, config);
            if skills.is_empty() {
                println!("No skills installed.");
                println!();
                println!("  Create one: mkdir -p ~/.construct/workspace/skills/my-skill");
                println!(
                    "              echo '# My Skill' > ~/.construct/workspace/skills/my-skill/SKILL.md"
                );
                println!();
                println!("  Or install: construct skills install <source>");
            } else {
                println!("Installed skills ({}):", skills.len());
                println!();
                for skill in &skills {
                    println!(
                        "  {} {} — {}",
                        console::style(&skill.name).white().bold(),
                        console::style(format!("v{}", skill.version)).dim(),
                        skill.description
                    );
                    if !skill.tools.is_empty() {
                        println!(
                            "    Tools: {}",
                            skill
                                .tools
                                .iter()
                                .map(|t| t.name.as_str())
                                .collect::<Vec<_>>()
                                .join(", ")
                        );
                    }
                    if !skill.tags.is_empty() {
                        println!("    Tags:  {}", skill.tags.join(", "));
                    }
                }
            }
            println!();
            Ok(())
        }
        crate::SkillCommands::Audit { source } => {
            let source_path = PathBuf::from(&source);
            let target = if source_path.exists() {
                source_path
            } else {
                skills_dir(workspace_dir).join(&source)
            };

            if !target.exists() {
                anyhow::bail!("Skill source or installed skill not found: {source}");
            }

            let report = audit::audit_skill_directory_with_options(
                &target,
                audit::SkillAuditOptions {
                    allow_scripts: config.skills.allow_scripts,
                },
            )?;
            if report.is_clean() {
                println!(
                    "  {} Skill audit passed for {} ({} files scanned).",
                    console::style("✓").green().bold(),
                    target.display(),
                    report.files_scanned
                );
                return Ok(());
            }

            println!(
                "  {} Skill audit failed for {}",
                console::style("✗").red().bold(),
                target.display()
            );
            for finding in report.findings {
                println!("    - {finding}");
            }
            anyhow::bail!("Skill audit failed.");
        }
        crate::SkillCommands::Install { source } => {
            println!("Installing skill from: {source}");

            let skills_path = skills_dir(workspace_dir);
            std::fs::create_dir_all(&skills_path)?;

            let (installed_dir, files_scanned) = if is_clawhub_source(&source) {
                install_clawhub_skill_source(&source, &skills_path, config.skills.allow_scripts)
                    .with_context(|| format!("failed to install skill from ClawHub: {source}"))?
            } else if is_git_source(&source) {
                install_git_skill_source(&source, &skills_path, config.skills.allow_scripts)
                    .with_context(|| format!("failed to install git skill source: {source}"))?
            } else {
                install_local_skill_source(&source, &skills_path, config.skills.allow_scripts)
                    .with_context(|| format!("failed to install local skill source: {source}"))?
            };
            println!(
                "  {} Skill installed and audited: {} ({} files scanned)",
                console::style("✓").green().bold(),
                installed_dir.display(),
                files_scanned
            );
            println!("  Security audit completed successfully.");

            // Register the skill with Kumiho so it gets a versioned identity
            // (item + revision + artifact + published tag) and SKILL.toml is
            // rewritten with `[skill].kref = "kref://<memory_project>/Skills/
            // <slug>.skill?t=published"`.  Best-effort — when Kumiho is
            // unreachable we log and continue; the daemon-startup scan in
            // src/gateway/mod.rs picks up unregistered skills on next start.
            register_after_install_best_effort(&installed_dir, config);

            Ok(())
        }
        crate::SkillCommands::Remove { name } => {
            // Reject path traversal attempts
            if name.contains("..") || name.contains('/') || name.contains('\\') {
                anyhow::bail!("Invalid skill name: {name}");
            }

            let skill_path = skills_dir(workspace_dir).join(&name);

            // Verify the resolved path is actually inside the skills directory
            let canonical_skills = skills_dir(workspace_dir)
                .canonicalize()
                .unwrap_or_else(|_| skills_dir(workspace_dir));
            if let Ok(canonical_skill) = skill_path.canonicalize() {
                if !canonical_skill.starts_with(&canonical_skills) {
                    anyhow::bail!("Skill path escapes skills directory: {name}");
                }
            }

            if !skill_path.exists() {
                anyhow::bail!("Skill not found: {name}");
            }

            std::fs::remove_dir_all(&skill_path)?;
            println!(
                "  {} Skill '{}' removed.",
                console::style("✓").green().bold(),
                name
            );
            Ok(())
        }
        crate::SkillCommands::Test { name, verbose } => {
            let results = if let Some(ref skill_name) = name {
                // Test a single skill
                let source_path = PathBuf::from(skill_name);
                let target = if source_path.exists() {
                    source_path
                } else {
                    skills_dir(workspace_dir).join(skill_name)
                };

                if !target.exists() {
                    anyhow::bail!("Skill not found: {}", skill_name);
                }

                let r = testing::test_skill(&target, skill_name, verbose)?;
                if r.tests_run == 0 {
                    println!(
                        "  {} No TEST.sh found for skill '{}'.",
                        console::style("-").dim(),
                        skill_name,
                    );
                    return Ok(());
                }
                vec![r]
            } else {
                // Test all skills
                let dirs = vec![skills_dir(workspace_dir)];
                testing::test_all_skills(&dirs, verbose)?
            };

            testing::print_results(&results);

            let any_failed = results.iter().any(|r| !r.failures.is_empty());
            if any_failed {
                anyhow::bail!("Some skill tests failed.");
            }
            Ok(())
        }
    }
}

#[cfg(test)]
#[allow(clippy::similar_names)]
mod tests {
    use super::*;
    use std::fs;
    use std::sync::{Mutex, OnceLock};

    fn open_skills_env_lock() -> &'static Mutex<()> {
        static ENV_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
        ENV_LOCK.get_or_init(|| Mutex::new(()))
    }

    struct EnvVarGuard {
        key: &'static str,
        original: Option<String>,
    }

    impl EnvVarGuard {
        fn unset(key: &'static str) -> Self {
            let original = std::env::var(key).ok();
            // SAFETY: test-only, single-threaded test runner.
            unsafe { std::env::remove_var(key) };
            Self { key, original }
        }
    }

    impl Drop for EnvVarGuard {
        fn drop(&mut self) {
            if let Some(value) = &self.original {
                // SAFETY: test-only, single-threaded test runner.
                unsafe { std::env::set_var(self.key, value) };
            } else {
                // SAFETY: test-only, single-threaded test runner.
                unsafe { std::env::remove_var(self.key) };
            }
        }
    }

    #[test]
    fn load_empty_skills_dir() {
        let dir = tempfile::tempdir().unwrap();
        let skills = load_skills(dir.path());
        assert!(skills.is_empty());
    }

    #[test]
    fn load_skill_from_toml() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("test-skill");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.toml"),
            r#"
[skill]
name = "test-skill"
description = "A test skill"
version = "1.0.0"
tags = ["test"]

[[tools]]
name = "hello"
description = "Says hello"
kind = "shell"
command = "echo hello"
"#,
        )
        .unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "test-skill");
        assert_eq!(skills[0].tools.len(), 1);
        assert_eq!(skills[0].tools[0].name, "hello");
    }

    #[test]
    fn load_skill_from_md() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("md-skill");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.md"),
            "# My Skill\nThis skill does cool things.\n",
        )
        .unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "md-skill");
        assert!(skills[0].description.contains("cool things"));
    }

    #[test]
    fn load_skill_from_md_frontmatter_uses_metadata_and_body() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("md-skill");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.md"),
            "---\nname: pdf\ndescription: Use this skill for PDFs\nversion: 1.2.3\nauthor: maintainer\ntags:\n  - docs\n  - pdf\n---\n# PDF Processing Guide\nExtract text carefully.\n",
        )
        .unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "pdf");
        assert_eq!(skills[0].description, "Use this skill for PDFs");
        assert_eq!(skills[0].version, "1.2.3");
        assert_eq!(skills[0].author.as_deref(), Some("maintainer"));
        assert_eq!(skills[0].tags, vec!["docs", "pdf"]);
        assert!(skills[0].prompts[0].contains("# PDF Processing Guide"));
        assert!(!skills[0].prompts[0].contains("name: pdf"));
    }

    #[test]
    fn skills_to_prompt_empty() {
        let prompt = skills_to_prompt(&[], Path::new("/tmp"));
        assert!(prompt.is_empty());
    }

    // ── content_file precedence + migration (step 6a) ────────────────

    fn write_skill_toml(dir: &Path, body: &str) -> PathBuf {
        let path = dir.join("SKILL.toml");
        fs::write(&path, body).unwrap();
        path
    }

    #[test]
    fn load_skill_toml_uses_content_file_when_set() {
        let dir = tempfile::tempdir().unwrap();
        fs::create_dir_all(dir.path().join("contents")).unwrap();
        fs::write(
            dir.path().join("contents/r1-2026-04-27.md"),
            "## external content\n\nthe external markdown body",
        )
        .unwrap();

        let manifest = write_skill_toml(
            dir.path(),
            r#"[skill]
name = "ext-skill"
description = "external content"
version = "0.1.0"
content_file = "contents/r1-2026-04-27.md"
"#,
        );

        let skill = load_skill_toml(&manifest).expect("loads");
        assert_eq!(skill.name, "ext-skill");
        assert_eq!(skill.prompts.len(), 1);
        assert!(skill.prompts[0].contains("the external markdown body"));
    }

    #[test]
    fn load_skill_toml_falls_back_to_embedded_when_content_file_missing() {
        let dir = tempfile::tempdir().unwrap();
        // `prompts` is a top-level field on SkillManifest — it must come
        // BEFORE the [skill] table or TOML treats it as a child of
        // [skill] (see the migrate test below for the same gotcha with
        // [[tools]]).
        let manifest = write_skill_toml(
            dir.path(),
            r#"prompts = ["embedded fallback content"]

[skill]
name = "fallback-skill"
description = "ext missing"
version = "0.1.0"
content_file = "contents/does-not-exist.md"
"#,
        );

        let skill = load_skill_toml(&manifest).expect("loads");
        assert_eq!(skill.prompts, vec!["embedded fallback content".to_string()]);
    }

    #[test]
    fn load_skill_toml_uses_embedded_when_no_content_file() {
        let dir = tempfile::tempdir().unwrap();
        let manifest = write_skill_toml(
            dir.path(),
            r#"prompts = ["legacy step one", "legacy step two"]

[skill]
name = "legacy-skill"
description = "no content_file"
version = "0.1.0"
"#,
        );

        let skill = load_skill_toml(&manifest).expect("loads");
        assert_eq!(skill.prompts.len(), 2);
        assert_eq!(skill.prompts[0], "legacy step one");
        assert_eq!(skill.prompts[1], "legacy step two");
    }

    #[test]
    fn load_skill_toml_content_file_takes_precedence_over_prompts() {
        let dir = tempfile::tempdir().unwrap();
        fs::create_dir_all(dir.path().join("contents")).unwrap();
        fs::write(
            dir.path().join("contents/r1.md"),
            "WIN: external content used",
        )
        .unwrap();

        let manifest = write_skill_toml(
            dir.path(),
            r#"prompts = ["LOSE: should be ignored"]

[skill]
name = "both-skill"
description = "both fields set"
version = "0.1.0"
content_file = "contents/r1.md"
"#,
        );

        let skill = load_skill_toml(&manifest).expect("loads");
        assert_eq!(skill.prompts.len(), 1);
        assert!(skill.prompts[0].contains("WIN"));
        assert!(!skill.prompts[0].contains("LOSE"));
    }

    #[test]
    fn migrate_skill_toml_writes_content_file_and_rewrites_manifest() {
        let dir = tempfile::tempdir().unwrap();
        // `prompts` MUST come before [skill] / [[tools]] tables so it
        // parses as a top-level field (the very gotcha this PR avoids
        // by externalising prompt content).
        let manifest = write_skill_toml(
            dir.path(),
            r#"prompts = ["Step alpha", "Step beta"]

[skill]
name = "to-migrate"
description = "legacy skill"
version = "0.2.0"

[[tools]]
name = "ping"
description = "echo"
kind = "shell"
command = "echo hi"
"#,
        );

        let result = migrate_skill_toml_to_content_file(&manifest).expect("migrates");
        match &result {
            SkillContentMigration::Migrated {
                manifest: m,
                content_file,
            } => {
                assert_eq!(m, &manifest);
                assert!(content_file.exists());
                assert!(content_file.starts_with(dir.path().join("contents")));
                let body = fs::read_to_string(content_file).unwrap();
                assert!(body.contains("Step alpha"));
                assert!(body.contains("Step beta"));
                // Paragraph break between entries.
                assert!(body.contains("Step alpha\n\nStep beta"));
            }
            other => panic!("expected Migrated, got {other:?}"),
        }

        // Reload via the public loader — should now resolve via content_file.
        let skill = load_skill_toml(&manifest).expect("loads after migration");
        assert_eq!(skill.prompts.len(), 1);
        assert!(skill.prompts[0].contains("Step alpha"));
        assert!(skill.prompts[0].contains("Step beta"));
        // Tools survived.
        assert_eq!(skill.tools.len(), 1);
        assert_eq!(skill.tools[0].name, "ping");

        // Manifest no longer carries embedded prompts.
        let raw = fs::read_to_string(&manifest).unwrap();
        assert!(raw.contains("content_file"));
        assert!(!raw.contains("prompts"));
    }

    #[test]
    fn migrate_skill_toml_idempotent_on_already_migrated() {
        let dir = tempfile::tempdir().unwrap();
        fs::create_dir_all(dir.path().join("contents")).unwrap();
        fs::write(dir.path().join("contents/r1.md"), "already there").unwrap();

        let manifest = write_skill_toml(
            dir.path(),
            r#"[skill]
name = "already"
description = "migrated"
version = "0.1.0"
content_file = "contents/r1.md"
"#,
        );

        let result = migrate_skill_toml_to_content_file(&manifest).expect("idempotent");
        assert_eq!(result, SkillContentMigration::AlreadyMigrated);
        // Manifest unchanged.
        let raw = fs::read_to_string(&manifest).unwrap();
        assert!(raw.contains("content_file = \"contents/r1.md\""));
    }

    #[test]
    fn migrate_skill_toml_no_op_when_nothing_to_migrate() {
        let dir = tempfile::tempdir().unwrap();
        let manifest = write_skill_toml(
            dir.path(),
            r#"[skill]
name = "empty"
description = "no prompts"
version = "0.1.0"
"#,
        );

        let result = migrate_skill_toml_to_content_file(&manifest).expect("noop ok");
        assert_eq!(result, SkillContentMigration::NothingToMigrate);
        // No contents directory created.
        assert!(!dir.path().join("contents").exists());
    }

    #[test]
    fn skills_to_prompt_with_skills() {
        let skills = vec![Skill {
            name: "test".to_string(),
            description: "A test".to_string(),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![],
            prompts: vec!["Do the thing.".to_string()],
            location: None,
        }];
        let prompt = skills_to_prompt(&skills, Path::new("/tmp"));
        assert!(prompt.contains("<available_skills>"));
        assert!(prompt.contains("<name>test</name>"));
        assert!(prompt.contains("<instruction>Do the thing.</instruction>"));
    }

    #[test]
    fn skills_to_prompt_with_effectiveness_reranks_high_success_first() {
        use crate::skills::effectiveness::InMemoryProvider;

        let mk = |name: &str| Skill {
            name: name.to_string(),
            description: format!("description for {name}"),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![],
            prompts: vec![],
            location: None,
        };
        // Static load order: a, b, c.  Effectiveness order should put `c`
        // first (rate=0.95), then `a` (rate=0.5 implicit / no data), then
        // `b` (rate=0.1).
        let skills = vec![mk("alpha"), mk("beta"), mk("gamma")];

        let mut provider = InMemoryProvider::default();
        provider.insert(
            "gamma",
            EffectivenessScore {
                rate: Some(0.95),
                total: 20,
            },
        );
        provider.insert(
            "beta",
            EffectivenessScore {
                rate: Some(0.10),
                total: 30,
            },
        );

        let prompt = skills_to_prompt_with_mode_and_effectiveness(
            &skills,
            Path::new("/tmp"),
            crate::config::SkillsPromptInjectionMode::Full,
            &provider,
        );

        // gamma should appear before alpha, alpha before beta in the rendered
        // prompt — the only signal is order of <name>…</name> occurrences.
        let pos_gamma = prompt.find("<name>gamma</name>").expect("gamma rendered");
        let pos_alpha = prompt.find("<name>alpha</name>").expect("alpha rendered");
        let pos_beta = prompt.find("<name>beta</name>").expect("beta rendered");
        assert!(
            pos_gamma < pos_alpha,
            "gamma (0.95) should outrank alpha (none/0.5)"
        );
        assert!(
            pos_alpha < pos_beta,
            "alpha (none/0.5) should outrank beta (0.10)"
        );
    }

    #[test]
    fn skills_to_prompt_with_no_op_provider_preserves_load_order() {
        let mk = |name: &str| Skill {
            name: name.to_string(),
            description: String::new(),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![],
            prompts: vec![],
            location: None,
        };
        let skills = vec![mk("alpha"), mk("beta"), mk("gamma")];

        let prompt = skills_to_prompt_with_mode_and_effectiveness(
            &skills,
            Path::new("/tmp"),
            crate::config::SkillsPromptInjectionMode::Full,
            crate::skills::effectiveness::no_op_provider(),
        );

        let pos_alpha = prompt.find("<name>alpha</name>").unwrap();
        let pos_beta = prompt.find("<name>beta</name>").unwrap();
        let pos_gamma = prompt.find("<name>gamma</name>").unwrap();
        assert!(pos_alpha < pos_beta && pos_beta < pos_gamma);
    }

    #[test]
    fn skills_to_prompt_compact_mode_omits_instructions_but_keeps_tools() {
        let skills = vec![Skill {
            name: "test".to_string(),
            description: "A test".to_string(),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![SkillTool {
                name: "run".to_string(),
                description: "Run task".to_string(),
                kind: "shell".to_string(),
                command: "echo hi".to_string(),
                args: HashMap::new(),
            }],
            prompts: vec!["Do the thing.".to_string()],
            location: Some(PathBuf::from("/tmp/workspace/skills/test/SKILL.md")),
        }];
        let prompt = skills_to_prompt_with_mode(
            &skills,
            Path::new("/tmp/workspace"),
            crate::config::SkillsPromptInjectionMode::Compact,
        );

        assert!(prompt.contains("<available_skills>"));
        assert!(prompt.contains("<name>test</name>"));
        assert!(prompt.contains("<location>skills/test/SKILL.md</location>"));
        assert!(prompt.contains("loaded on demand"));
        assert!(prompt.contains("read_skill(name)"));
        assert!(!prompt.contains("<instructions>"));
        assert!(!prompt.contains("<instruction>Do the thing.</instruction>"));
        // Compact mode should still include tools so the LLM knows about them.
        // Registered tools (shell/script/http) appear under <callable_tools>.
        assert!(prompt.contains("<callable_tools"));
        assert!(prompt.contains("<name>test.run</name>"));
    }

    #[test]
    fn init_skills_creates_readme() {
        let dir = tempfile::tempdir().unwrap();
        init_skills_dir(dir.path()).unwrap();
        assert!(dir.path().join("skills").join("README.md").exists());
    }

    #[test]
    fn init_skills_idempotent() {
        let dir = tempfile::tempdir().unwrap();
        init_skills_dir(dir.path()).unwrap();
        init_skills_dir(dir.path()).unwrap(); // second call should not fail
        assert!(dir.path().join("skills").join("README.md").exists());
    }

    #[test]
    fn load_nonexistent_dir() {
        let dir = tempfile::tempdir().unwrap();
        let fake = dir.path().join("nonexistent");
        let skills = load_skills(&fake);
        assert!(skills.is_empty());
    }

    #[test]
    fn load_ignores_files_in_skills_dir() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        fs::create_dir_all(&skills_dir).unwrap();
        // A file, not a directory — should be ignored
        fs::write(skills_dir.join("not-a-skill.txt"), "hello").unwrap();
        let skills = load_skills(dir.path());
        assert!(skills.is_empty());
    }

    #[test]
    fn load_ignores_dir_without_manifest() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let empty_skill = skills_dir.join("empty-skill");
        fs::create_dir_all(&empty_skill).unwrap();
        // Directory exists but no SKILL.toml or SKILL.md
        let skills = load_skills(dir.path());
        assert!(skills.is_empty());
    }

    #[test]
    fn load_multiple_skills() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");

        for name in ["alpha", "beta", "gamma"] {
            let skill_dir = skills_dir.join(name);
            fs::create_dir_all(&skill_dir).unwrap();
            fs::write(
                skill_dir.join("SKILL.md"),
                format!("# {name}\nSkill {name} description.\n"),
            )
            .unwrap();
        }

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 3);
    }

    #[test]
    fn toml_skill_with_multiple_tools() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("multi-tool");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.toml"),
            r#"
[skill]
name = "multi-tool"
description = "Has many tools"
version = "2.0.0"
author = "tester"
tags = ["automation", "devops"]

[[tools]]
name = "build"
description = "Build the project"
kind = "shell"
command = "cargo build"

[[tools]]
name = "test"
description = "Run tests"
kind = "shell"
command = "cargo test"

[[tools]]
name = "deploy"
description = "Deploy via HTTP"
kind = "http"
command = "https://api.example.com/deploy"
"#,
        )
        .unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        let s = &skills[0];
        assert_eq!(s.name, "multi-tool");
        assert_eq!(s.version, "2.0.0");
        assert_eq!(s.author.as_deref(), Some("tester"));
        assert_eq!(s.tags, vec!["automation", "devops"]);
        assert_eq!(s.tools.len(), 3);
        assert_eq!(s.tools[0].name, "build");
        assert_eq!(s.tools[1].kind, "shell");
        assert_eq!(s.tools[2].kind, "http");
    }

    #[test]
    fn toml_skill_minimal() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("minimal");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.toml"),
            r#"
[skill]
name = "minimal"
description = "Bare minimum"
"#,
        )
        .unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].version, "0.1.0"); // default version
        assert!(skills[0].author.is_none());
        assert!(skills[0].tags.is_empty());
        assert!(skills[0].tools.is_empty());
    }

    #[test]
    fn toml_skill_invalid_syntax_skipped() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("broken");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(skill_dir.join("SKILL.toml"), "this is not valid toml {{{{").unwrap();

        let skills = load_skills(dir.path());
        assert!(skills.is_empty()); // broken skill is skipped
    }

    #[test]
    fn md_skill_heading_only() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("heading-only");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(skill_dir.join("SKILL.md"), "# Just a Heading\n").unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].description, "No description");
    }

    #[test]
    fn skills_to_prompt_includes_tools() {
        let skills = vec![Skill {
            name: "weather".to_string(),
            description: "Get weather".to_string(),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![SkillTool {
                name: "get_weather".to_string(),
                description: "Fetch forecast".to_string(),
                kind: "shell".to_string(),
                command: "curl wttr.in".to_string(),
                args: HashMap::new(),
            }],
            prompts: vec![],
            location: None,
        }];
        let prompt = skills_to_prompt(&skills, Path::new("/tmp"));
        assert!(prompt.contains("weather"));
        // Registered tools (shell kind) now appear under <callable_tools> with
        // prefixed names (skill_name.tool_name).
        assert!(prompt.contains("<callable_tools"));
        assert!(prompt.contains("<name>weather.get_weather</name>"));
        assert!(prompt.contains("<description>Fetch forecast</description>"));
    }

    #[test]
    fn skills_to_prompt_escapes_xml_content() {
        let skills = vec![Skill {
            name: "xml<skill>".to_string(),
            description: "A & B".to_string(),
            version: "1.0.0".to_string(),
            author: None,
            tags: vec![],
            tools: vec![],
            prompts: vec!["Use <tool> & check \"quotes\".".to_string()],
            location: None,
        }];

        let prompt = skills_to_prompt(&skills, Path::new("/tmp"));
        assert!(prompt.contains("<name>xml&lt;skill&gt;</name>"));
        assert!(prompt.contains("<description>A &amp; B</description>"));
        assert!(prompt.contains(
            "<instruction>Use &lt;tool&gt; &amp; check &quot;quotes&quot;.</instruction>"
        ));
    }

    #[test]
    fn git_source_detection_accepts_remote_protocols_and_scp_style() {
        let sources = [
            "https://github.com/some-org/some-skill.git",
            "http://github.com/some-org/some-skill.git",
            "ssh://git@github.com/some-org/some-skill.git",
            "git://github.com/some-org/some-skill.git",
            "git@github.com:some-org/some-skill.git",
            "git@localhost:skills/some-skill.git",
        ];

        for source in sources {
            assert!(
                is_git_source(source),
                "expected git source detection for '{source}'"
            );
        }
    }

    #[test]
    fn git_source_detection_rejects_local_paths_and_invalid_inputs() {
        let sources = [
            "./skills/local-skill",
            "/tmp/skills/local-skill",
            "C:\\skills\\local-skill",
            "git@github.com",
            "ssh://",
            "not-a-url",
            "dir/git@github.com:org/repo.git",
        ];

        for source in sources {
            assert!(
                !is_git_source(source),
                "expected local/invalid source detection for '{source}'"
            );
        }
    }

    #[test]
    fn clawhub_source_is_not_git_source() {
        assert!(!is_git_source("https://clawhub.ai/steipete/summarize"));
        assert!(!is_git_source("https://www.clawhub.ai/steipete/summarize"));
        assert!(is_clawhub_source("https://clawhub.ai/steipete/summarize"));
        assert!(is_clawhub_source("clawhub:summarize"));
    }

    #[test]
    fn clawhub_download_url_building() {
        assert_eq!(
            clawhub_download_url("https://clawhub.ai/steipete/gog").unwrap(),
            "https://clawhub.ai/api/v1/download?slug=steipete/gog"
        );
        assert_eq!(
            clawhub_download_url("https://www.clawhub.ai/steipete/gog").unwrap(),
            "https://clawhub.ai/api/v1/download?slug=steipete/gog"
        );
        assert_eq!(
            clawhub_download_url("https://clawhub.ai/gog").unwrap(),
            "https://clawhub.ai/api/v1/download?slug=gog"
        );
        assert_eq!(
            clawhub_download_url("clawhub:gog").unwrap(),
            "https://clawhub.ai/api/v1/download?slug=gog"
        );
    }

    #[test]
    fn non_clawhub_https_urls_still_detected_as_git() {
        let git_urls = [
            "https://github.com/some-org/some-skill.git",
            "https://gitlab.com/owner/repo",
        ];
        for url in git_urls {
            assert!(!is_clawhub_source(url));
            assert!(is_git_source(url));
        }
    }

    #[test]
    fn skills_dir_path() {
        let base = std::path::Path::new("/home/user/.construct");
        let dir = skills_dir(base);
        assert_eq!(dir, PathBuf::from("/home/user/.construct/skills"));
    }

    #[test]
    fn toml_prefers_over_md() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("dual");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.toml"),
            "[skill]\nname = \"from-toml\"\ndescription = \"TOML wins\"\n",
        )
        .unwrap();
        fs::write(skill_dir.join("SKILL.md"), "# From MD\nMD description\n").unwrap();

        let skills = load_skills(dir.path());
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "from-toml"); // TOML takes priority
    }

    #[test]
    fn open_skills_enabled_resolution_prefers_env_then_config_then_default_false() {
        assert!(!open_skills_enabled_from_sources(None, None));
        assert!(open_skills_enabled_from_sources(Some(true), None));
        assert!(!open_skills_enabled_from_sources(Some(true), Some("0")));
        assert!(open_skills_enabled_from_sources(Some(false), Some("yes")));
        // Invalid env values should fall back to config.
        assert!(open_skills_enabled_from_sources(
            Some(true),
            Some("invalid")
        ));
        assert!(!open_skills_enabled_from_sources(
            Some(false),
            Some("invalid")
        ));
    }

    #[test]
    fn resolve_open_skills_dir_resolution_prefers_env_then_config_then_home() {
        let home = Path::new("/tmp/home-dir");
        assert_eq!(
            resolve_open_skills_dir_from_sources(
                Some("/tmp/env-skills"),
                Some("/tmp/config"),
                Some(home)
            ),
            Some(PathBuf::from("/tmp/env-skills"))
        );
        assert_eq!(
            resolve_open_skills_dir_from_sources(
                Some("   "),
                Some("/tmp/config-skills"),
                Some(home)
            ),
            Some(PathBuf::from("/tmp/config-skills"))
        );
        assert_eq!(
            resolve_open_skills_dir_from_sources(None, None, Some(home)),
            Some(PathBuf::from("/tmp/home-dir/open-skills"))
        );
        assert_eq!(resolve_open_skills_dir_from_sources(None, None, None), None);
    }

    #[test]
    fn load_skills_with_config_reads_open_skills_dir_without_network() {
        let _env_guard = open_skills_env_lock().lock().unwrap();
        let _enabled_guard = EnvVarGuard::unset("CONSTRUCT_OPEN_SKILLS_ENABLED");
        let _dir_guard = EnvVarGuard::unset("CONSTRUCT_OPEN_SKILLS_DIR");

        let dir = tempfile::tempdir().unwrap();
        let workspace_dir = dir.path().join("workspace");
        fs::create_dir_all(workspace_dir.join("skills")).unwrap();

        let open_skills_dir = dir.path().join("open-skills-local");
        fs::create_dir_all(open_skills_dir.join("skills/http_request")).unwrap();
        fs::write(open_skills_dir.join("README.md"), "# open skills\n").unwrap();
        fs::write(
            open_skills_dir.join("CONTRIBUTING.md"),
            "# contribution guide\n",
        )
        .unwrap();
        fs::write(
            open_skills_dir.join("skills/http_request/SKILL.md"),
            "# HTTP request\nFetch API responses.\n",
        )
        .unwrap();

        let mut config = crate::config::Config::default();
        config.workspace_dir = workspace_dir.clone();
        config.skills.open_skills_enabled = true;
        config.skills.open_skills_dir = Some(open_skills_dir.to_string_lossy().to_string());

        let skills = load_skills_with_config(&workspace_dir, &config);
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "http_request");
        assert_ne!(skills[0].name, "CONTRIBUTING");
    }

    #[test]
    fn load_open_skill_md_frontmatter_uses_metadata_and_strips_block() {
        let _env_guard = open_skills_env_lock().lock().unwrap();
        let _enabled_guard = EnvVarGuard::unset("CONSTRUCT_OPEN_SKILLS_ENABLED");
        let _dir_guard = EnvVarGuard::unset("CONSTRUCT_OPEN_SKILLS_DIR");

        let dir = tempfile::tempdir().unwrap();
        let workspace_dir = dir.path().join("workspace");
        fs::create_dir_all(workspace_dir.join("skills")).unwrap();

        let open_skills_dir = dir.path().join("open-skills-local");
        fs::create_dir_all(open_skills_dir.join("skills/pdf")).unwrap();
        fs::write(
            open_skills_dir.join("skills/pdf/SKILL.md"),
            "---\nname: pdf\ndescription: Use this skill whenever the user needs PDF help.\nauthor: community\ntags:\n  - parser\n---\n# PDF Guide\nInspect files safely.\n",
        )
        .unwrap();

        let mut config = crate::config::Config::default();
        config.workspace_dir = workspace_dir.clone();
        config.skills.open_skills_enabled = true;
        config.skills.open_skills_dir = Some(open_skills_dir.to_string_lossy().to_string());

        let skills = load_skills_with_config(&workspace_dir, &config);
        assert_eq!(skills.len(), 1);
        assert_eq!(skills[0].name, "pdf");
        assert_eq!(
            skills[0].description,
            "Use this skill whenever the user needs PDF help."
        );
        assert_eq!(skills[0].author.as_deref(), Some("community"));
        assert!(skills[0].tags.iter().any(|tag| tag == "parser"));
        assert!(skills[0].tags.iter().any(|tag| tag == "open-skills"));
        assert!(skills[0].prompts[0].contains("# PDF Guide"));
        assert!(!skills[0].prompts[0].contains("description: Use this skill"));
    }

    #[test]
    fn skill_with_scripts_skipped_when_allow_scripts_false() {
        let dir = tempfile::tempdir().unwrap();
        let skills_dir = dir.path().join("skills");
        let skill_dir = skills_dir.join("obsidian");
        fs::create_dir_all(&skill_dir).unwrap();

        fs::write(
            skill_dir.join("SKILL.toml"),
            r#"
[skill]
name = "obsidian"
description = "Obsidian vault tool"

[[tools]]
name = "search"
description = "Search vault"
kind = "shell"
command = "obsidian search {{query}}"
"#,
        )
        .unwrap();
        fs::write(skill_dir.join("setup.sh"), "#!/bin/bash\necho setup\n").unwrap();

        // With allow_scripts=false (default), skill should be skipped
        let skills = load_skills_from_directory(&skills_dir, false);
        assert!(
            skills.is_empty(),
            "skill with script files should be skipped when allow_scripts=false"
        );

        // With allow_scripts=true, skill should load
        let skills = load_skills_from_directory(&skills_dir, true);
        assert_eq!(skills.len(), 1, "skill should load when allow_scripts=true");
        assert_eq!(skills[0].name, "obsidian");
    }
}

#[cfg(test)]
mod symlink_tests;