clawft-core 0.8.3

//! Autonomous skill creation (C4a).
//!
//! Detects repeated task patterns and auto-generates skill definitions.
//! Generated skills are installed in `~/.clawft/skills/` in a "pending"
//! state and require user approval before activation.
//!
//! **Review timing (WEFT-74 / ADR-080):** the canonical approval path is
//! CLI (`weft skills pending|approve|reject`). Interactive agent start may
//! print a non-blocking notice via [`pending_review_start_notice`]; a
//! blocking TTY prompt at loop start is **not** used.
//!
//! **Disabled by default** -- must be opted into via configuration.
//!
//! # Security
//!
//! Auto-generated skills have minimal permissions:
//! - No shell access
//! - No network access
//! - Filesystem limited to workspace directory (explicit allowlist in
//!   SKILL.md frontmatter; see [`SkillFilesystemAllowlist`])
//!
//! At load time the skill's filesystem allowlist is **intersected** with
//! the host agent's sandbox paths via
//! [`effective_filesystem_on_load`]. The effective set never exceeds the
//! host (WEFT-75).
//!
//! # Pattern Detection
//!
//! The detector tracks sequences of tool calls. When the same sequence
//! appears at least `threshold` times (default: 3), a skill candidate
//! is generated.

use std::collections::HashMap;
use std::path::{Path, PathBuf};
use std::sync::Arc;

use tracing::{debug, info, warn};

use clawft_types::skill::SkillDefinition;

use crate::pipeline::learner::TrajectoryLearner;
use crate::pipeline::mutation::{TrajectoryHint, auto_select_strategy, mutate_prompt};
use crate::security::intersect_path_allowlists;

// ---------------------------------------------------------------------------
// Filesystem allowlist (WEFT-75)
// ---------------------------------------------------------------------------

/// Workspace-relative path used as the minimal autogen FS root.
///
/// Matches the design target "filesystem limited to workspace directory".
pub const AUTOGEN_WORKSPACE_PATH: &str = ".";

/// Frontmatter key under which the filesystem allowlist is emitted/parsed.
pub const FILESYSTEM_FRONTMATTER_KEY: &str = "filesystem";

/// Explicit filesystem allowlist for a skill (SKILL.md `filesystem:` block).
///
/// Autogenerated skills always emit a **minimal** instance of this struct
/// via [`SkillFilesystemAllowlist::minimal_autogen`]. At load time the
/// skill's allowlist is intersected with the host agent so the effective
/// permissions never exceed either side.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SkillFilesystemAllowlist {
    /// Paths the skill may read (relative or absolute).
    pub readable_paths: Vec<String>,
    /// Paths the skill may write.
    pub writable_paths: Vec<String>,
    /// Whether new files may be created under writable paths.
    pub allow_create: bool,
    /// Whether files may be deleted under writable paths.
    pub allow_delete: bool,
}

impl Default for SkillFilesystemAllowlist {
    fn default() -> Self {
        Self::minimal_autogen()
    }
}

impl SkillFilesystemAllowlist {
    /// Minimal allowlist for autogenerated skills.
    ///
    /// - Read/write limited to the workspace root (`.`)
    /// - No create / delete
    pub fn minimal_autogen() -> Self {
        Self {
            readable_paths: vec![AUTOGEN_WORKSPACE_PATH.into()],
            writable_paths: vec![AUTOGEN_WORKSPACE_PATH.into()],
            allow_create: false,
            allow_delete: false,
        }
    }

    /// Render as a YAML frontmatter fragment (no leading/trailing `---`).
    ///
    /// Example:
    /// ```text
    /// filesystem:
    ///   readable-paths:
    ///     - .
    ///   writable-paths:
    ///     - .
    ///   allow-create: false
    ///   allow-delete: false
    /// ```
    pub fn to_yaml_frontmatter(&self) -> String {
        let mut out = String::from("filesystem:\n");
        out.push_str("  readable-paths:\n");
        if self.readable_paths.is_empty() {
            out.push_str("    []\n");
        } else {
            for p in &self.readable_paths {
                out.push_str(&format!("    - {p}\n"));
            }
        }
        out.push_str("  writable-paths:\n");
        if self.writable_paths.is_empty() {
            out.push_str("    []\n");
        } else {
            for p in &self.writable_paths {
                out.push_str(&format!("    - {p}\n"));
            }
        }
        out.push_str(&format!("  allow-create: {}\n", self.allow_create));
        out.push_str(&format!("  allow-delete: {}\n", self.allow_delete));
        out
    }

    /// Parse from a JSON value (as stored in [`SkillDefinition::metadata`]).
    ///
    /// Accepts both kebab-case (`readable-paths`) and snake_case keys.
    pub fn from_json(value: &serde_json::Value) -> Option<Self> {
        let obj = value.as_object()?;
        let readable = string_list_from_map(obj, "readable-paths")
            .or_else(|| string_list_from_map(obj, "readable_paths"))
            .unwrap_or_default();
        let writable = string_list_from_map(obj, "writable-paths")
            .or_else(|| string_list_from_map(obj, "writable_paths"))
            .unwrap_or_default();
        let allow_create = bool_from_map(obj, "allow-create")
            .or_else(|| bool_from_map(obj, "allow_create"))
            .unwrap_or(false);
        let allow_delete = bool_from_map(obj, "allow-delete")
            .or_else(|| bool_from_map(obj, "allow_delete"))
            .unwrap_or(false);
        Some(Self {
            readable_paths: readable,
            writable_paths: writable,
            allow_create,
            allow_delete,
        })
    }

    /// Extract the filesystem allowlist from a loaded skill.
    ///
    /// Looks up `metadata["filesystem"]`. For autogenerated skills that
    /// lack an explicit block (legacy), falls back to
    /// [`Self::minimal_autogen`]. Returns `None` for non-autogen skills
    /// without a `filesystem` block.
    pub fn from_skill(skill: &SkillDefinition) -> Option<Self> {
        if let Some(value) = skill.metadata.get(FILESYSTEM_FRONTMATTER_KEY) {
            return Self::from_json(value);
        }
        if is_autogenerated_skill(skill) {
            return Some(Self::minimal_autogen());
        }
        None
    }

    /// Whether this allowlist does not exceed `other`.
    ///
    /// - Empty path lists on `other` mean unrestricted (same as
    ///   [`intersect_path_allowlists`]): any self paths are allowed.
    /// - Non-empty `other` lists require every self path to be a member.
    /// - Boolean flags on self may only be true if `other` also allows them.
    ///
    /// Used to assert the effective set never exceeds the host.
    pub fn is_subset_of(&self, other: &Self) -> bool {
        let read_ok = other.readable_paths.is_empty()
            || self
                .readable_paths
                .iter()
                .all(|p| other.readable_paths.iter().any(|h| paths_equal(p, h)));
        let write_ok = other.writable_paths.is_empty()
            || self
                .writable_paths
                .iter()
                .all(|p| other.writable_paths.iter().any(|h| paths_equal(p, h)));
        let create_ok = !self.allow_create || other.allow_create;
        let delete_ok = !self.allow_delete || other.allow_delete;
        read_ok && write_ok && create_ok && delete_ok
    }
}

fn string_list_from_map(
    obj: &serde_json::Map<String, serde_json::Value>,
    key: &str,
) -> Option<Vec<String>> {
    let arr = obj.get(key)?.as_array()?;
    Some(
        arr.iter()
            .filter_map(|v| v.as_str().map(String::from))
            .collect(),
    )
}

fn bool_from_map(obj: &serde_json::Map<String, serde_json::Value>, key: &str) -> Option<bool> {
    obj.get(key)?.as_bool()
}

fn paths_equal(a: &str, b: &str) -> bool {
    let na = if a == "/" {
        a.to_string()
    } else {
        a.trim_end_matches('/').to_string()
    };
    let nb = if b == "/" {
        b.to_string()
    } else {
        b.trim_end_matches('/').to_string()
    };
    na == nb
}

/// Whether a skill was produced by the autogen pipeline.
///
/// True when frontmatter set `autogenerated: true` (stored in metadata).
pub fn is_autogenerated_skill(skill: &SkillDefinition) -> bool {
    skill
        .metadata
        .get("autogenerated")
        .and_then(|v| v.as_bool())
        .unwrap_or(false)
}

/// Intersect a skill's filesystem allowlist with the host agent's.
///
/// - Path lists use [`intersect_path_allowlists`] (skill ∩ host when both
///   non-empty; empty side defers to the other).
/// - Boolean flags are AND'd so the skill cannot escalate create/delete.
///
/// The result is always a subset of the host when the host paths are
/// non-empty (path membership) and never grants a flag the host denies.
pub fn intersect_filesystem_allowlist(
    skill: &SkillFilesystemAllowlist,
    host: &SkillFilesystemAllowlist,
) -> SkillFilesystemAllowlist {
    SkillFilesystemAllowlist {
        readable_paths: intersect_path_allowlists(&skill.readable_paths, &host.readable_paths),
        writable_paths: intersect_path_allowlists(&skill.writable_paths, &host.writable_paths),
        allow_create: skill.allow_create && host.allow_create,
        allow_delete: skill.allow_delete && host.allow_delete,
    }
}

/// Resolve the effective filesystem allowlist when loading a skill under
/// a host agent sandbox (WEFT-75 load-time intersection).
///
/// 1. Extract the skill's declared allowlist from metadata (or minimal
///    defaults for autogenerated skills).
/// 2. Intersect with `host`.
/// 3. Return the effective set (never exceeds host when host is non-empty).
///
/// For non-autogen skills without a `filesystem` block, returns a clone
/// of `host` (no additional restriction beyond the agent sandbox).
pub fn effective_filesystem_on_load(
    skill: &SkillDefinition,
    host: &SkillFilesystemAllowlist,
) -> SkillFilesystemAllowlist {
    match SkillFilesystemAllowlist::from_skill(skill) {
        Some(skill_fs) => intersect_filesystem_allowlist(&skill_fs, host),
        None => host.clone(),
    }
}

// ---------------------------------------------------------------------------
// Configuration
// ---------------------------------------------------------------------------

/// Configuration for autonomous skill creation.
///
/// When [`Self::learner`] is set (the active pipeline
/// [`TrajectoryLearner`]), skill generation uses
/// [`generate_skill_md_with_learning`] so successful trajectory patterns
/// mutate the candidate `SKILL.md` (WEFT-66).
#[derive(Clone)]
pub struct AutogenConfig {
    /// Whether autonomous skill creation is enabled.
    /// Default: `false` (disabled).
    pub enabled: bool,
    /// Minimum number of pattern repetitions before suggesting a skill.
    /// Default: 3.
    pub threshold: usize,
    /// Maximum number of pending skills allowed at once.
    /// Prevents unbounded growth. Default: 10.
    pub max_pending: usize,
    /// Directory where generated skills are installed.
    /// Default: `~/.clawft/skills/`
    pub install_dir: Option<PathBuf>,
    /// Active GEPA trajectory learner shared with pipeline stage 6.
    ///
    /// When `Some` and autogen is enabled, candidate skills are improved
    /// via [`improve_skill_instructions`] / [`generate_skill_md_with_learning`].
    /// Default: `None` (pattern-only generation, no learning mutation).
    pub learner: Option<Arc<TrajectoryLearner>>,
}

impl std::fmt::Debug for AutogenConfig {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("AutogenConfig")
            .field("enabled", &self.enabled)
            .field("threshold", &self.threshold)
            .field("max_pending", &self.max_pending)
            .field("install_dir", &self.install_dir)
            .field(
                "learner",
                &self
                    .learner
                    .as_ref()
                    .map(|l| format!("TrajectoryLearner(n={})", l.trajectory_count())),
            )
            .finish()
    }
}

impl Default for AutogenConfig {
    fn default() -> Self {
        Self {
            enabled: false,
            threshold: 3,
            max_pending: 10,
            install_dir: None,
            learner: None,
        }
    }
}

impl AutogenConfig {
    /// Get the effective install directory, defaulting to `~/.clawft/skills/`.
    pub fn install_dir(&self) -> PathBuf {
        self.install_dir.clone().unwrap_or_else(|| {
            #[cfg(feature = "native")]
            {
                dirs::home_dir()
                    .unwrap_or_else(|| PathBuf::from("."))
                    .join(".clawft")
                    .join("skills")
            }
            #[cfg(not(feature = "native"))]
            {
                PathBuf::from(".clawft").join("skills")
            }
        })
    }

    /// Attach the active pipeline [`TrajectoryLearner`] for learning-driven
    /// skill generation (WEFT-66).
    pub fn with_learner(mut self, learner: Arc<TrajectoryLearner>) -> Self {
        self.learner = Some(learner);
        self
    }
}

// ---------------------------------------------------------------------------
// Pattern Detection
// ---------------------------------------------------------------------------

/// A sequence of tool calls forming a pattern.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct ToolCallPattern {
    /// Ordered list of tool names in the pattern.
    pub tool_names: Vec<String>,
}

impl ToolCallPattern {
    /// Create a new pattern from tool names.
    pub fn new(tool_names: Vec<String>) -> Self {
        Self { tool_names }
    }

    /// Generate a suggested skill name from the pattern.
    pub fn suggested_name(&self) -> String {
        if self.tool_names.is_empty() {
            return "unnamed-skill".to_string();
        }
        let parts: Vec<&str> = self.tool_names.iter().map(|s| s.as_str()).collect();
        let name = parts.join("-then-");
        // Truncate to reasonable length
        if name.len() > 60 {
            format!("{}-auto", &name[..56])
        } else {
            format!("{name}-auto")
        }
    }

    /// Number of steps in the pattern.
    pub fn len(&self) -> usize {
        self.tool_names.len()
    }

    /// Whether the pattern is empty.
    pub fn is_empty(&self) -> bool {
        self.tool_names.is_empty()
    }
}

/// Detects repeated tool call patterns in the agent's execution history.
pub struct PatternDetector {
    /// Configuration for pattern detection.
    config: AutogenConfig,
    /// Counts of each observed pattern.
    pattern_counts: HashMap<ToolCallPattern, usize>,
    /// Recent tool calls (sliding window).
    recent_calls: Vec<String>,
    /// Maximum window size for pattern detection.
    max_window: usize,
    /// Patterns that have already been reported (to avoid duplicates).
    reported: HashMap<ToolCallPattern, bool>,
}

impl PatternDetector {
    /// Create a new pattern detector.
    pub fn new(config: AutogenConfig) -> Self {
        Self {
            config,
            pattern_counts: HashMap::new(),
            recent_calls: Vec::new(),
            max_window: 10,
            reported: HashMap::new(),
        }
    }

    /// Record a tool call.
    pub fn record_tool_call(&mut self, tool_name: &str) {
        if !self.config.enabled {
            return;
        }

        self.recent_calls.push(tool_name.to_string());

        // Keep window bounded
        if self.recent_calls.len() > self.max_window * 3 {
            self.recent_calls.drain(..self.max_window);
        }

        // Extract patterns of length 2..=max_window from recent calls
        let n = self.recent_calls.len();
        for len in 2..=self.max_window.min(n) {
            let pattern = ToolCallPattern::new(self.recent_calls[n - len..n].to_vec());
            *self.pattern_counts.entry(pattern).or_insert(0) += 1;
        }
    }

    /// Check for patterns that have reached the threshold.
    ///
    /// Returns newly detected patterns (patterns that just crossed the
    /// threshold and have not been reported before).
    pub fn detect_candidates(&mut self) -> Vec<ToolCallPattern> {
        if !self.config.enabled {
            return vec![];
        }

        let threshold = self.config.threshold;
        let mut candidates = Vec::new();

        for (pattern, count) in &self.pattern_counts {
            if *count >= threshold && !self.reported.contains_key(pattern) {
                candidates.push(pattern.clone());
            }
        }

        // Mark as reported
        for candidate in &candidates {
            self.reported.insert(candidate.clone(), true);
        }

        candidates
    }

    /// Get the count for a specific pattern.
    pub fn pattern_count(&self, pattern: &ToolCallPattern) -> usize {
        self.pattern_counts.get(pattern).copied().unwrap_or(0)
    }

    /// Reset all pattern tracking state.
    pub fn reset(&mut self) {
        self.pattern_counts.clear();
        self.recent_calls.clear();
        self.reported.clear();
    }

    /// Whether the detector is enabled.
    pub fn is_enabled(&self) -> bool {
        self.config.enabled
    }

    /// Borrow the detector's configuration (includes install dir + learner).
    pub fn config(&self) -> &AutogenConfig {
        &self.config
    }

    /// Effective skill install directory from the embedded config.
    pub fn install_dir(&self) -> PathBuf {
        self.config.install_dir()
    }

    /// Active trajectory learner, if wired into [`AutogenConfig`].
    pub fn learner(&self) -> Option<&TrajectoryLearner> {
        self.config.learner.as_deref()
    }

    /// Clone the [`Arc`] to the trajectory learner, if wired.
    pub fn learner_arc(&self) -> Option<Arc<TrajectoryLearner>> {
        self.config.learner.clone()
    }

    /// Attach (or replace) the trajectory learner used for skill mutation.
    pub fn set_learner(&mut self, learner: Option<Arc<TrajectoryLearner>>) {
        self.config.learner = learner;
    }
}

// ---------------------------------------------------------------------------
// Skill Generation
// ---------------------------------------------------------------------------

/// A generated skill candidate awaiting user approval.
#[derive(Debug, Clone)]
pub struct SkillCandidate {
    /// Suggested skill name.
    pub name: String,
    /// Description of what the skill does.
    pub description: String,
    /// The tool call pattern that triggered generation.
    pub pattern: ToolCallPattern,
    /// Generated SKILL.md content.
    pub skill_md: String,
    /// Approval state.
    pub state: CandidateState,
}

/// State of a skill candidate in the approval pipeline.
#[non_exhaustive]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum CandidateState {
    /// Awaiting user review and approval.
    Pending,
    /// Approved by user, ready for installation.
    Approved,
    /// Rejected by user.
    Rejected,
}

/// Generates SKILL.md content from a detected pattern.
///
/// Emits an explicit **minimal** filesystem allowlist under the
/// `filesystem:` frontmatter key (WEFT-75). At load time the host agent
/// must intersect this with its own sandbox via
/// [`effective_filesystem_on_load`].
pub fn generate_skill_md(pattern: &ToolCallPattern) -> SkillCandidate {
    let name = pattern.suggested_name();
    let tools_list = pattern
        .tool_names
        .iter()
        .map(|t| format!("  - {t}"))
        .collect::<Vec<_>>()
        .join("\n");

    let description = format!(
        "Auto-generated skill: executes {} in sequence",
        pattern
            .tool_names
            .iter()
            .map(|t| format!("`{t}`"))
            .collect::<Vec<_>>()
            .join(" -> ")
    );

    let instructions = format!(
        "Execute the following tools in order:\n{}",
        pattern
            .tool_names
            .iter()
            .enumerate()
            .map(|(i, t)| format!("{}. Use the `{t}` tool", i + 1))
            .collect::<Vec<_>>()
            .join("\n")
    );

    let filesystem_yaml = SkillFilesystemAllowlist::minimal_autogen().to_yaml_frontmatter();

    let skill_md = format!(
        "---\n\
         name: {name}\n\
         description: \"{description}\"\n\
         version: 0.1.0\n\
         allowed-tools:\n\
         {tools_list}\n\
         user-invocable: false\n\
         autogenerated: true\n\
         {filesystem_yaml}\
         ---\n\n\
         {instructions}\n"
    );

    SkillCandidate {
        name,
        description,
        pattern: pattern.clone(),
        skill_md,
        state: CandidateState::Pending,
    }
}

/// Install a skill candidate to the managed skills directory.
///
/// Creates a directory `{install_dir}/{name}/` with a `SKILL.md` file.
/// The skill is installed in "pending" state -- it will not be loaded
/// by the skill watcher until approved.
pub fn install_pending_skill(
    candidate: &SkillCandidate,
    install_dir: &Path,
) -> Result<PathBuf, String> {
    let skill_dir = install_dir.join(&candidate.name);

    std::fs::create_dir_all(&skill_dir).map_err(|e| format!("create skill dir: {e}"))?;

    let skill_path = skill_dir.join("SKILL.md");
    std::fs::write(&skill_path, &candidate.skill_md).map_err(|e| format!("write SKILL.md: {e}"))?;

    // Write a .pending marker file -- the skill watcher should
    // skip loading skills with this marker until approved.
    // Review path: CLI (`weft skills pending|approve|reject`) + optional
    // non-blocking start notice (WEFT-74 / ADR-080) — never a blocking prompt.
    let marker_path = skill_dir.join(PENDING_MARKER);
    std::fs::write(&marker_path, "awaiting user approval")
        .map_err(|e| format!("write .pending marker: {e}"))?;

    info!(
        skill = %candidate.name,
        path = %skill_dir.display(),
        "installed pending skill"
    );

    Ok(skill_dir)
}

/// Approve a pending skill by removing the `.pending` marker.
pub fn approve_skill(skill_dir: &Path) -> Result<(), String> {
    let marker = skill_dir.join(PENDING_MARKER);
    if marker.exists() {
        std::fs::remove_file(&marker).map_err(|e| format!("remove .pending: {e}"))?;
        info!(skill_dir = %skill_dir.display(), "approved pending skill");
        Ok(())
    } else {
        Err("skill is not in pending state".into())
    }
}

/// Reject a pending skill by removing its directory.
pub fn reject_skill(skill_dir: &Path) -> Result<(), String> {
    if skill_dir.exists() {
        std::fs::remove_dir_all(skill_dir).map_err(|e| format!("remove skill dir: {e}"))?;
        info!(skill_dir = %skill_dir.display(), "rejected and removed pending skill");
        Ok(())
    } else {
        Err("skill directory does not exist".into())
    }
}

/// Marker filename signaling a skill awaits human review (autogen / staged).
pub const PENDING_MARKER: &str = ".pending";

/// Check if a skill directory is in pending state.
pub fn is_pending(skill_dir: &Path) -> bool {
    skill_dir.join(PENDING_MARKER).exists()
}

// ---------------------------------------------------------------------------
// Pending-skill review timing (WEFT-74 / ADR-080)
// ---------------------------------------------------------------------------

/// How operators are notified of `.pending` autogen skills.
///
/// Product default is [`PendingReviewTiming::CliWithStartNotice`]: the
/// canonical review path is the CLI (`weft skills pending|approve|reject`),
/// with an optional **non-blocking** one-line notice at interactive agent
/// start so pending candidates do not accumulate unseen. A **blocking**
/// interactive prompt at agent-loop start is intentionally **not** offered
/// (breaks headless/CI, daemon, non-TTY, and mid-session focus).
///
/// See `docs/adr/adr-080-pending-skill-review-timing.md`.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum PendingReviewTiming {
    /// Review only via CLI (`weft skills pending|approve|reject`).
    /// No automatic surface at agent start.
    CliOnly,
    /// CLI review plus a non-blocking notice at interactive agent start.
    ///
    /// **Default.** Does not block the agent loop or require stdin.
    #[default]
    CliWithStartNotice,
}

impl PendingReviewTiming {
    /// Canonical product policy (ADR-080).
    pub const DEFAULT_POLICY: Self = Self::CliWithStartNotice;

    /// Whether this policy emits a start-of-session notice when pending
    /// skills exist.
    pub fn emits_start_notice(self) -> bool {
        matches!(self, Self::CliWithStartNotice)
    }
}

/// Summary of one skill directory marked `.pending`.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PendingSkillInfo {
    /// Directory basename (skill name).
    pub name: String,
    /// Absolute or install-dir-relative path to the skill directory.
    pub path: PathBuf,
}

/// List skill directories under `install_dir` that carry a `.pending` marker.
///
/// Returns an empty vec when the directory does not exist. Entries are
/// sorted by name. Does not read SKILL.md contents (see CLI
/// `weft skills pending` for previews).
pub fn list_pending_skills(install_dir: &Path) -> Result<Vec<PendingSkillInfo>, String> {
    if !install_dir.exists() {
        return Ok(Vec::new());
    }

    let mut out = Vec::new();
    let entries =
        std::fs::read_dir(install_dir).map_err(|e| format!("read skills dir: {e}"))?;
    for entry in entries {
        let entry = entry.map_err(|e| format!("read dir entry: {e}"))?;
        let path = entry.path();
        if !path.is_dir() {
            continue;
        }
        if !is_pending(&path) {
            continue;
        }
        let name = path
            .file_name()
            .and_then(|n| n.to_str())
            .unwrap_or("unknown")
            .to_string();
        out.push(PendingSkillInfo { name, path });
    }
    out.sort_by(|a, b| a.name.cmp(&b.name));
    Ok(out)
}

/// Build a non-blocking start notice for pending skills.
///
/// Returns [`None`] when there is nothing to review, or when `timing`
/// does not emit start notices. **Never** prompts for input — callers
/// print the string and continue.
pub fn format_pending_review_notice(
    pending: &[PendingSkillInfo],
    timing: PendingReviewTiming,
) -> Option<String> {
    if !timing.emits_start_notice() || pending.is_empty() {
        return None;
    }

    let count = pending.len();
    let names: Vec<&str> = pending.iter().map(|p| p.name.as_str()).collect();
    let preview = match names.len() {
        0 => return None,
        1 => names[0].to_string(),
        2 => format!("{}, {}", names[0], names[1]),
        3 => format!("{}, {}, {}", names[0], names[1], names[2]),
        n => format!("{}, {} (+{} more)", names[0], names[1], n - 2),
    };

    Some(format!(
        "Pending skills ({count}): {preview}. \
         Review: weft skills pending | approve <name> | reject <name>"
    ))
}

/// Resolve the default user skills install dir (`~/.clawft/skills/`).
///
/// Shared by CLI start-notice and autogen install so paths stay consistent.
pub fn default_user_skills_dir() -> Option<PathBuf> {
    #[cfg(feature = "native")]
    {
        dirs::home_dir().map(|h| h.join(".clawft").join("skills"))
    }
    #[cfg(not(feature = "native"))]
    {
        Some(PathBuf::from(".clawft").join("skills"))
    }
}

/// Scan the default (or provided) skills dir and format a start notice.
///
/// Convenience for interactive REPL boot. Failures to read the directory
/// yield `None` (notice is best-effort; never blocks session start).
pub fn pending_review_start_notice(
    install_dir: Option<&Path>,
    timing: PendingReviewTiming,
) -> Option<String> {
    if !timing.emits_start_notice() {
        return None;
    }
    let owned;
    let dir = match install_dir {
        Some(p) => p,
        None => {
            owned = default_user_skills_dir()?;
            &owned
        }
    };
    let pending = list_pending_skills(dir).ok()?;
    format_pending_review_notice(&pending, timing)
}

// ---------------------------------------------------------------------------
// Trajectory-based prompt improvement
// ---------------------------------------------------------------------------

/// Improve a generated skill's instructions using trajectory data.
///
/// If the [`TrajectoryLearner`] has successful patterns (trajectories
/// scoring >= 0.8), this converts them into [`TrajectoryHint`]s, selects
/// the best mutation strategy via [`auto_select_strategy`], and applies
/// [`mutate_prompt`] to the skill instructions.
///
/// Returns the (possibly improved) instructions. If there are no
/// successful patterns, the original instructions are returned unchanged.
pub fn improve_skill_instructions(instructions: &str, learner: &TrajectoryLearner) -> String {
    // `get_best_trajectories` returns the top-N by score among *all*
    // trajectories (including poor ones). Filter to high-quality only
    // before chaining with `get_poor_trajectories` so poor items are not
    // double-counted — that previously biased `auto_select_strategy`
    // toward RemoveIneffective, which is a no-op on fresh SKILL.md bodies
    // that share no phrases with trajectory feedback (WEFT-66).
    let best: Vec<_> = learner
        .get_best_trajectories(10)
        .into_iter()
        .filter(|t| t.trajectory.quality.overall >= 0.8)
        .collect();
    let poor = learner.get_poor_trajectories(10);

    if best.is_empty() && poor.is_empty() {
        return instructions.to_string();
    }

    let hints: Vec<TrajectoryHint> = best
        .iter()
        .chain(poor.iter())
        .map(|st| TrajectoryHint {
            request_content: st
                .trajectory
                .request
                .messages
                .first()
                .map(|m| m.content.clone())
                .unwrap_or_default(),
            quality_score: st.trajectory.quality.overall,
            feedback: st.feedback.clone(),
        })
        .collect();

    let strategy = auto_select_strategy(&hints);
    debug!(
        ?strategy,
        hints_count = hints.len(),
        "mutating skill instructions"
    );
    mutate_prompt(instructions, &hints, strategy)
}

/// Generate a skill candidate and optionally improve it using trajectory data.
///
/// Combines [`generate_skill_md`] with [`improve_skill_instructions`] when
/// a [`TrajectoryLearner`] is available. Mutation is applied to the
/// instruction body (after the YAML frontmatter) so the frontmatter
/// allowlist / name / tools stay stable (WEFT-66).
pub fn generate_skill_md_with_learning(
    pattern: &ToolCallPattern,
    learner: Option<&TrajectoryLearner>,
) -> SkillCandidate {
    let mut candidate = generate_skill_md(pattern);

    if let Some(learner) = learner {
        let patterns = learner.extract_successful_patterns();
        if !patterns.is_empty() {
            debug!(
                pattern_count = patterns.len(),
                "improving skill via trajectory learning"
            );
            // Split on the closing frontmatter fence so YAML is preserved.
            if let Some(idx) = candidate.skill_md.find("\n---\n") {
                let (front, rest) = candidate.skill_md.split_at(idx + "\n---\n".len());
                let body = rest.trim_start_matches('\n');
                let improved = improve_skill_instructions(body, learner);
                candidate.skill_md = format!("{front}\n{improved}");
                if !candidate.skill_md.ends_with('\n') {
                    candidate.skill_md.push('\n');
                }
            } else {
                candidate.skill_md = improve_skill_instructions(&candidate.skill_md, learner);
            }
        }
    }

    candidate
}

// ---------------------------------------------------------------------------
// Phase 4: auto-promotion (.claude/skills → .clawft/skills) — WEFT-348
// ---------------------------------------------------------------------------

/// Default successful-invocation count before promoting a skill.
///
/// Matches the chat-agent-v1 Phase 4 sketch ("after enough successful uses")
/// and the AC example of ≥10 successful invocations.
pub const DEFAULT_PROMOTION_THRESHOLD: usize = 10;

/// Path component sequence that identifies Claude Code skill roots.
pub const CLAUDE_SKILLS_MARKER: &str = ".claude/skills";

/// Filename for the append-only promotion audit log (JSONL) under the
/// target skills directory.
pub const PROMOTION_AUDIT_FILENAME: &str = ".promotion-audit.jsonl";

/// Runtime configuration for skill auto-promotion (WEFT-348).
///
/// Mirrors [`clawft_types::config::SkillPromotionConfig`] plus path knobs
/// used by the promoter implementation.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SkillPromotionConfig {
    /// Master switch. When `false`, [`SkillPromoter::record_success`] is a
    /// no-op that returns [`PromotionOutcome::Disabled`].
    pub enabled: bool,
    /// Successful invocations required before promoting. Default: 10.
    pub threshold: usize,
    /// Destination root for promoted skills (workspace or user
    /// `.clawft/skills/`). When `None`, uses
    /// [`default_user_skills_dir`] / relative `.clawft/skills`.
    pub target_dir: Option<PathBuf>,
    /// Optional explicit audit log path. When `None`, writes under
    /// `{target_dir}/.promotion-audit.jsonl`.
    pub audit_log_path: Option<PathBuf>,
}

impl Default for SkillPromotionConfig {
    fn default() -> Self {
        Self {
            enabled: true,
            threshold: DEFAULT_PROMOTION_THRESHOLD,
            target_dir: None,
            audit_log_path: None,
        }
    }
}

impl SkillPromotionConfig {
    /// Build from the serializable config section (threshold + enabled).
    pub fn from_types(cfg: &clawft_types::config::SkillPromotionConfig) -> Self {
        Self {
            enabled: cfg.enabled,
            threshold: cfg.threshold.max(1),
            target_dir: None,
            audit_log_path: None,
        }
    }

    /// Resolve the effective target skills directory.
    pub fn target_dir(&self) -> PathBuf {
        self.target_dir
            .clone()
            .or_else(default_user_skills_dir)
            .unwrap_or_else(|| PathBuf::from(".clawft").join("skills"))
    }

    /// Resolve the audit log path.
    pub fn audit_log_path(&self) -> PathBuf {
        self.audit_log_path
            .clone()
            .unwrap_or_else(|| self.target_dir().join(PROMOTION_AUDIT_FILENAME))
    }

    /// Builder: override threshold (clamped to ≥1).
    pub fn with_threshold(mut self, threshold: usize) -> Self {
        self.threshold = threshold.max(1);
        self
    }

    /// Builder: set target directory for promoted skills.
    pub fn with_target_dir(mut self, dir: impl Into<PathBuf>) -> Self {
        self.target_dir = Some(dir.into());
        self
    }

    /// Builder: set explicit audit log path.
    pub fn with_audit_log_path(mut self, path: impl Into<PathBuf>) -> Self {
        self.audit_log_path = Some(path.into());
        self
    }
}

/// One promotion event written to the audit log.
#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
pub struct SkillPromotionAuditEntry {
    /// Skill directory basename.
    pub skill_name: String,
    /// Source path under `.claude/skills/…`.
    pub source_path: PathBuf,
    /// Destination path under `.clawft/skills/…`.
    pub target_path: PathBuf,
    /// Successful invocation count that triggered promotion.
    pub successful_invocations: usize,
    /// Configured threshold at promotion time.
    pub threshold: usize,
    /// Unix epoch milliseconds when the promotion occurred.
    pub timestamp_unix_ms: u64,
    /// Event type constant for log consumers.
    pub event: String,
}

impl SkillPromotionAuditEntry {
    /// Event type string written into every promotion audit record.
    pub const EVENT_TYPE: &'static str = "skill.promoted";

    fn new(
        skill_name: impl Into<String>,
        source_path: PathBuf,
        target_path: PathBuf,
        successful_invocations: usize,
        threshold: usize,
    ) -> Self {
        Self {
            skill_name: skill_name.into(),
            source_path,
            target_path,
            successful_invocations,
            threshold,
            timestamp_unix_ms: unix_ms_now(),
            event: Self::EVENT_TYPE.into(),
        }
    }
}

/// Result of recording a successful skill invocation.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum PromotionOutcome {
    /// Promotion is disabled in config.
    Disabled,
    /// Skill source is not under `.claude/skills/` (not a candidate).
    NotEligible,
    /// Count updated; threshold not yet reached.
    Counted {
        /// Running success count for this skill.
        success_count: usize,
        /// Configured threshold.
        threshold: usize,
    },
    /// Skill already lives under the target dir (or was promoted earlier).
    AlreadyPromoted {
        /// Existing target path.
        target: PathBuf,
    },
    /// Threshold breached and the skill was copied to `.clawft/skills/`.
    Promoted {
        /// Audit entry written for this promotion.
        entry: SkillPromotionAuditEntry,
    },
    /// Filesystem or I/O failure during promotion.
    Error(String),
}

/// Tracks successful skill invocations and auto-promotes `.claude/skills/*`
/// into `.clawft/skills/` when the configurable threshold is reached.
///
/// # Design (WEFT-348 / chat-agent-v1 Phase 4)
///
/// | Concern | Behavior |
/// |---------|----------|
/// | Eligibility | Only paths containing `.claude/skills` as components |
/// | Counter | Per skill name, successful invocations only |
/// | Threshold | Configurable (default 10); breach → promote once |
/// | Promote | Recursive copy of skill directory → target root |
/// | Audit | In-memory ring + append-only JSONL on disk |
/// | Idempotency | Second promotion of same name is `AlreadyPromoted` |
///
/// Callers (agent loop, REPL, gateway) invoke
/// [`SkillPromoter::record_success`] after a turn that successfully used
/// a skill whose `source_path` is under `.claude/skills/`.
pub struct SkillPromoter {
    config: SkillPromotionConfig,
    /// Per-skill successful invocation counts (eligible skills only).
    success_counts: HashMap<String, usize>,
    /// Skills already promoted this process (name → target path).
    promoted: HashMap<String, PathBuf>,
    /// In-memory audit log (capped).
    audit_log: Vec<SkillPromotionAuditEntry>,
    /// Cap on in-memory audit entries.
    max_audit_entries: usize,
}

impl SkillPromoter {
    /// Create a promoter with the given config.
    pub fn new(config: SkillPromotionConfig) -> Self {
        Self {
            config,
            success_counts: HashMap::new(),
            promoted: HashMap::new(),
            audit_log: Vec::new(),
            max_audit_entries: 10_000,
        }
    }

    /// Default-config promoter (enabled, threshold 10).
    pub fn with_defaults() -> Self {
        Self::new(SkillPromotionConfig::default())
    }

    /// Borrow the runtime config.
    pub fn config(&self) -> &SkillPromotionConfig {
        &self.config
    }

    /// Whether promotion is currently enabled.
    pub fn is_enabled(&self) -> bool {
        self.config.enabled
    }

    /// Running success count for `skill_name` (0 if never recorded).
    pub fn success_count(&self, skill_name: &str) -> usize {
        self.success_counts.get(skill_name).copied().unwrap_or(0)
    }

    /// Whether `skill_name` has already been promoted.
    pub fn is_promoted(&self, skill_name: &str) -> bool {
        self.promoted.contains_key(skill_name)
    }

    /// In-memory audit entries (oldest first).
    pub fn audit_log(&self) -> &[SkillPromotionAuditEntry] {
        &self.audit_log
    }

    /// Reset counters and in-memory audit (does not delete on-disk audit).
    pub fn reset(&mut self) {
        self.success_counts.clear();
        self.promoted.clear();
        self.audit_log.clear();
    }

    /// Record a **successful** skill invocation and auto-promote on threshold.
    ///
    /// `source_path` is the on-disk skill directory (or a file inside it).
    /// Only paths under `.claude/skills/` are counted; others return
    /// [`PromotionOutcome::NotEligible`] without mutating state.
    pub fn record_success(
        &mut self,
        skill_name: &str,
        source_path: &Path,
    ) -> PromotionOutcome {
        if !self.config.enabled {
            return PromotionOutcome::Disabled;
        }

        if skill_name.is_empty() {
            return PromotionOutcome::Error("skill name is empty".into());
        }

        // Resolve skill directory (source_path may be SKILL.md).
        let skill_dir = resolve_skill_dir(source_path);

        if !is_claude_skills_path(&skill_dir) {
            debug!(
                skill = %skill_name,
                path = %skill_dir.display(),
                "skill promotion: not a .claude/skills source, skipping"
            );
            return PromotionOutcome::NotEligible;
        }

        if let Some(target) = self.promoted.get(skill_name) {
            return PromotionOutcome::AlreadyPromoted {
                target: target.clone(),
            };
        }

        let target_root = self.config.target_dir();
        let expected_target = target_root.join(skill_name);
        if expected_target.exists() && expected_target.join("SKILL.md").exists() {
            self.promoted
                .insert(skill_name.to_string(), expected_target.clone());
            return PromotionOutcome::AlreadyPromoted {
                target: expected_target,
            };
        }

        let count = {
            let entry = self
                .success_counts
                .entry(skill_name.to_string())
                .or_insert(0);
            *entry = entry.saturating_add(1);
            *entry
        };

        let threshold = self.config.threshold.max(1);
        if count < threshold {
            debug!(
                skill = %skill_name,
                count,
                threshold,
                "skill promotion: counted successful invocation"
            );
            return PromotionOutcome::Counted {
                success_count: count,
                threshold,
            };
        }

        // Threshold breached — promote.
        match promote_skill_directory(&skill_dir, &target_root, skill_name) {
            Ok(target_path) => {
                let entry = SkillPromotionAuditEntry::new(
                    skill_name,
                    skill_dir,
                    target_path.clone(),
                    count,
                    threshold,
                );
                self.push_audit(entry.clone());
                if let Err(e) = append_audit_log(&self.config.audit_log_path(), &entry) {
                    warn!(
                        error = %e,
                        skill = %skill_name,
                        "skill promotion: failed to write audit log file"
                    );
                }
                self.promoted
                    .insert(skill_name.to_string(), target_path);
                info!(
                    skill = %skill_name,
                    count,
                    threshold,
                    "skill promotion: promoted .claude/skills → .clawft/skills"
                );
                PromotionOutcome::Promoted { entry }
            }
            Err(e) => {
                warn!(error = %e, skill = %skill_name, "skill promotion: copy failed");
                PromotionOutcome::Error(e)
            }
        }
    }

    fn push_audit(&mut self, entry: SkillPromotionAuditEntry) {
        self.audit_log.push(entry);
        if self.audit_log.len() > self.max_audit_entries {
            let overflow = self.audit_log.len() - self.max_audit_entries;
            self.audit_log.drain(..overflow);
        }
    }
}

/// Whether `path` is under a `.claude/skills` directory tree.
///
/// Matches both absolute paths (`/home/u/proj/.claude/skills/foo`) and
/// relative ones (`.claude/skills/foo`). Component-wise so Windows
/// separators also work when present as individual path parts.
pub fn is_claude_skills_path(path: &Path) -> bool {
    let components: Vec<String> = path
        .components()
        .filter_map(|c| match c {
            std::path::Component::Normal(s) => Some(s.to_string_lossy().into_owned()),
            _ => None,
        })
        .collect();

    components
        .windows(2)
        .any(|w| w[0] == ".claude" && w[1] == "skills")
}

/// Resolve the skill directory from a path that may point at `SKILL.md`
/// or another file inside the skill folder.
fn resolve_skill_dir(path: &Path) -> PathBuf {
    if path.is_file()
        || path
            .file_name()
            .and_then(|n| n.to_str())
            .is_some_and(|n| n.eq_ignore_ascii_case("SKILL.md") || n == "skill.json")
    {
        path.parent().unwrap_or(path).to_path_buf()
    } else {
        path.to_path_buf()
    }
}

/// Copy a skill directory into `target_root/{name}/`.
///
/// Creates parent directories as needed. Fails if the destination already
/// exists (callers should check first for idempotency). Does **not** copy
/// `.pending` markers — promoted skills are immediately active.
pub fn promote_skill_directory(
    source_dir: &Path,
    target_root: &Path,
    name: &str,
) -> Result<PathBuf, String> {
    if !source_dir.exists() {
        return Err(format!(
            "source skill directory does not exist: {}",
            source_dir.display()
        ));
    }
    if !source_dir.is_dir() {
        return Err(format!(
            "source is not a directory: {}",
            source_dir.display()
        ));
    }

    // Basic name hygiene (reject path separators / traversal).
    if name.is_empty()
        || name.contains('/')
        || name.contains('\\')
        || name == "."
        || name == ".."
        || name.contains("..")
    {
        return Err(format!("invalid skill name for promotion: {name:?}"));
    }

    let dest = target_root.join(name);
    if dest.exists() {
        return Err(format!(
            "destination already exists: {}",
            dest.display()
        ));
    }

    std::fs::create_dir_all(target_root)
        .map_err(|e| format!("create target root {}: {e}", target_root.display()))?;

    copy_dir_recursive(source_dir, &dest)?;

    // Strip any pending marker so promoted skills load immediately.
    let pending = dest.join(PENDING_MARKER);
    if pending.exists() {
        let _ = std::fs::remove_file(&pending);
    }

    // Stamp a small sidecar so operators can see the skill was promoted.
    let stamp = dest.join(".promoted-from-claude");
    let stamp_body = format!(
        "source={}\npromoted_at_ms={}\n",
        source_dir.display(),
        unix_ms_now()
    );
    let _ = std::fs::write(&stamp, stamp_body);

    Ok(dest)
}

/// Recursively copy `src` directory into `dst` (which must not exist).
fn copy_dir_recursive(src: &Path, dst: &Path) -> Result<(), String> {
    std::fs::create_dir_all(dst).map_err(|e| format!("create {}: {e}", dst.display()))?;
    let entries = std::fs::read_dir(src).map_err(|e| format!("read {}: {e}", src.display()))?;
    for entry in entries {
        let entry = entry.map_err(|e| format!("dir entry: {e}"))?;
        let file_type = entry
            .file_type()
            .map_err(|e| format!("file type: {e}"))?;
        let from = entry.path();
        let to = dst.join(entry.file_name());
        if file_type.is_dir() {
            copy_dir_recursive(&from, &to)?;
        } else if file_type.is_file() {
            std::fs::copy(&from, &to)
                .map_err(|e| format!("copy {} → {}: {e}", from.display(), to.display()))?;
        }
        // Skip symlinks for safety.
    }
    Ok(())
}

/// Append one audit entry as a JSON line.
fn append_audit_log(path: &Path, entry: &SkillPromotionAuditEntry) -> Result<(), String> {
    use std::io::Write;
    if let Some(parent) = path.parent() {
        std::fs::create_dir_all(parent)
            .map_err(|e| format!("create audit parent {}: {e}", parent.display()))?;
    }
    let line = serde_json::to_string(entry).map_err(|e| format!("serialize audit: {e}"))?;
    let mut file = std::fs::OpenOptions::new()
        .create(true)
        .append(true)
        .open(path)
        .map_err(|e| format!("open audit log {}: {e}", path.display()))?;
    writeln!(file, "{line}").map_err(|e| format!("write audit log: {e}"))?;
    Ok(())
}

fn unix_ms_now() -> u64 {
    use std::time::{SystemTime, UNIX_EPOCH};
    SystemTime::now()
        .duration_since(UNIX_EPOCH)
        .map(|d| d.as_millis() as u64)
        .unwrap_or(0)
}

// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------

#[cfg(test)]
mod tests {
    use super::*;

    fn enabled_config() -> AutogenConfig {
        AutogenConfig {
            enabled: true,
            threshold: 3,
            max_pending: 10,
            install_dir: None,
            learner: None,
        }
    }

    fn disabled_config() -> AutogenConfig {
        AutogenConfig::default()
    }

    // -- AutogenConfig tests --

    #[test]
    fn config_default_is_disabled() {
        let config = AutogenConfig::default();
        assert!(!config.enabled);
        assert_eq!(config.threshold, 3);
        assert_eq!(config.max_pending, 10);
        assert!(config.learner.is_none());
    }

    #[test]
    fn config_with_learner_attaches_arc() {
        let learner = Arc::new(TrajectoryLearner::new(
            crate::pipeline::learner::TrajectoryLearnerConfig::default(),
        ));
        let config = AutogenConfig::default().with_learner(Arc::clone(&learner));
        assert!(config.learner.is_some());
        assert_eq!(
            Arc::as_ptr(config.learner.as_ref().unwrap()),
            Arc::as_ptr(&learner)
        );
    }

    #[test]
    fn config_install_dir_default() {
        let config = AutogenConfig::default();
        let dir = config.install_dir();
        assert!(dir.to_string_lossy().contains(".clawft"));
        assert!(dir.to_string_lossy().contains("skills"));
    }

    // -- ToolCallPattern tests --

    #[test]
    fn pattern_suggested_name() {
        let pattern = ToolCallPattern::new(vec!["read_file".into(), "edit_file".into()]);
        let name = pattern.suggested_name();
        assert!(name.contains("read_file"));
        assert!(name.contains("edit_file"));
        assert!(name.ends_with("-auto"));
    }

    #[test]
    fn pattern_suggested_name_long_truncated() {
        let tools: Vec<String> = (0..20).map(|i| format!("tool_{i}")).collect();
        let pattern = ToolCallPattern::new(tools);
        let name = pattern.suggested_name();
        assert!(name.len() <= 65, "name too long: {}", name.len());
    }

    #[test]
    fn pattern_empty() {
        let pattern = ToolCallPattern::new(vec![]);
        assert!(pattern.is_empty());
        assert_eq!(pattern.len(), 0);
        assert_eq!(pattern.suggested_name(), "unnamed-skill");
    }

    // -- PatternDetector tests --

    #[test]
    fn detector_disabled_records_nothing() {
        let mut detector = PatternDetector::new(disabled_config());
        detector.record_tool_call("read_file");
        detector.record_tool_call("edit_file");
        let candidates = detector.detect_candidates();
        assert!(candidates.is_empty());
    }

    #[test]
    fn detector_does_not_fire_below_threshold() {
        let mut detector = PatternDetector::new(enabled_config());
        // Record pattern twice (below threshold of 3)
        for _ in 0..2 {
            detector.record_tool_call("read_file");
            detector.record_tool_call("edit_file");
        }
        let candidates = detector.detect_candidates();
        assert!(candidates.is_empty());
    }

    #[test]
    fn detector_fires_at_threshold() {
        let mut detector = PatternDetector::new(enabled_config());
        // Record pattern 3 times (at threshold)
        for _ in 0..3 {
            detector.record_tool_call("read_file");
            detector.record_tool_call("edit_file");
        }
        let candidates = detector.detect_candidates();
        assert!(!candidates.is_empty(), "should detect at least one pattern");
    }

    #[test]
    fn detector_does_not_re_report() {
        let mut detector = PatternDetector::new(enabled_config());
        for _ in 0..5 {
            detector.record_tool_call("read_file");
            detector.record_tool_call("edit_file");
        }
        let first = detector.detect_candidates();
        assert!(!first.is_empty());

        // Second call should not re-report the same pattern
        let second = detector.detect_candidates();
        assert!(second.is_empty(), "should not re-report same pattern");
    }

    #[test]
    fn detector_reset_clears_state() {
        let mut detector = PatternDetector::new(enabled_config());
        for _ in 0..3 {
            detector.record_tool_call("a");
            detector.record_tool_call("b");
        }
        let _ = detector.detect_candidates();
        detector.reset();

        // After reset, same pattern should be detected again
        for _ in 0..3 {
            detector.record_tool_call("a");
            detector.record_tool_call("b");
        }
        let candidates = detector.detect_candidates();
        assert!(!candidates.is_empty());
    }

    #[test]
    fn detector_configurable_threshold() {
        let config = AutogenConfig {
            enabled: true,
            threshold: 5,
            ..Default::default()
        };
        let mut detector = PatternDetector::new(config);

        for _ in 0..4 {
            detector.record_tool_call("x");
            detector.record_tool_call("y");
        }
        assert!(detector.detect_candidates().is_empty(), "below threshold");

        detector.record_tool_call("x");
        detector.record_tool_call("y");
        assert!(!detector.detect_candidates().is_empty(), "at threshold");
    }

    // -- Skill generation tests --

    #[test]
    fn generate_skill_md_valid() {
        let pattern = ToolCallPattern::new(vec![
            "read_file".into(),
            "edit_file".into(),
            "write_file".into(),
        ]);
        let candidate = generate_skill_md(&pattern);

        assert!(candidate.name.contains("auto"));
        assert_eq!(candidate.state, CandidateState::Pending);
        assert!(candidate.skill_md.contains("---"));
        assert!(candidate.skill_md.contains("read_file"));
        assert!(candidate.skill_md.contains("edit_file"));
        assert!(candidate.skill_md.contains("write_file"));
        assert!(candidate.skill_md.contains("autogenerated: true"));
        assert!(candidate.skill_md.contains("user-invocable: false"));
    }

    #[test]
    fn generate_skill_md_description_contains_tools() {
        let pattern = ToolCallPattern::new(vec!["a".into(), "b".into()]);
        let candidate = generate_skill_md(&pattern);
        assert!(candidate.description.contains("`a`"));
        assert!(candidate.description.contains("`b`"));
    }

    // -- Install/approve/reject tests --

    #[test]
    fn install_pending_skill_creates_files() {
        let dir = std::env::temp_dir().join("clawft_autogen_install_test");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        let pattern = ToolCallPattern::new(vec!["read_file".into(), "edit_file".into()]);
        let candidate = generate_skill_md(&pattern);

        let result = install_pending_skill(&candidate, &dir);
        assert!(result.is_ok());

        let skill_dir = result.unwrap();
        assert!(skill_dir.join("SKILL.md").exists());
        assert!(skill_dir.join(".pending").exists());
        assert!(is_pending(&skill_dir));

        let _ = std::fs::remove_dir_all(&dir);
    }

    #[test]
    fn approve_skill_removes_marker() {
        let dir = std::env::temp_dir().join("clawft_autogen_approve_test");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        let pattern = ToolCallPattern::new(vec!["a".into(), "b".into()]);
        let candidate = generate_skill_md(&pattern);
        let skill_dir = install_pending_skill(&candidate, &dir).unwrap();

        assert!(is_pending(&skill_dir));
        assert!(approve_skill(&skill_dir).is_ok());
        assert!(!is_pending(&skill_dir));
        assert!(skill_dir.join("SKILL.md").exists());

        let _ = std::fs::remove_dir_all(&dir);
    }

    #[test]
    fn reject_skill_removes_directory() {
        let dir = std::env::temp_dir().join("clawft_autogen_reject_test");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        let pattern = ToolCallPattern::new(vec!["x".into(), "y".into()]);
        let candidate = generate_skill_md(&pattern);
        let skill_dir = install_pending_skill(&candidate, &dir).unwrap();

        assert!(skill_dir.exists());
        assert!(reject_skill(&skill_dir).is_ok());
        assert!(!skill_dir.exists());

        let _ = std::fs::remove_dir_all(&dir);
    }

    #[test]
    fn approve_non_pending_fails() {
        let dir = std::env::temp_dir().join("clawft_autogen_approve_nopend");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        // No .pending marker
        let result = approve_skill(&dir);
        assert!(result.is_err());
        assert!(result.unwrap_err().contains("not in pending state"));

        let _ = std::fs::remove_dir_all(&dir);
    }

    // -- Pending review timing (WEFT-74 / ADR-080) --

    #[test]
    fn pending_review_timing_default_is_cli_with_start_notice() {
        assert_eq!(
            PendingReviewTiming::default(),
            PendingReviewTiming::CliWithStartNotice
        );
        assert_eq!(
            PendingReviewTiming::DEFAULT_POLICY,
            PendingReviewTiming::CliWithStartNotice
        );
        assert!(PendingReviewTiming::CliWithStartNotice.emits_start_notice());
        assert!(!PendingReviewTiming::CliOnly.emits_start_notice());
    }

    #[test]
    fn list_pending_skills_empty_when_dir_missing() {
        let dir = std::env::temp_dir().join("clawft_autogen_list_missing_weft74");
        let _ = std::fs::remove_dir_all(&dir);
        let pending = list_pending_skills(&dir).unwrap();
        assert!(pending.is_empty());
    }

    #[test]
    fn list_pending_skills_empty_one_many() {
        let dir = std::env::temp_dir().join("clawft_autogen_list_pending_weft74");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        // Empty install dir.
        assert!(list_pending_skills(&dir).unwrap().is_empty());

        // Approved skill (no marker) is ignored.
        let approved = dir.join("approved-skill");
        std::fs::create_dir_all(&approved).unwrap();
        std::fs::write(approved.join("SKILL.md"), "---\nname: approved\n---\n").unwrap();
        assert!(list_pending_skills(&dir).unwrap().is_empty());

        // One pending.
        let p1 = ToolCallPattern::new(vec!["read_file".into(), "edit_file".into()]);
        let c1 = generate_skill_md(&p1);
        install_pending_skill(&c1, &dir).unwrap();
        let one = list_pending_skills(&dir).unwrap();
        assert_eq!(one.len(), 1);
        assert_eq!(one[0].name, c1.name);

        // Many pending (sorted by name).
        let p2 = ToolCallPattern::new(vec!["a".into(), "b".into()]);
        let mut c2 = generate_skill_md(&p2);
        c2.name = "aaa-pending".into();
        install_pending_skill(&c2, &dir).unwrap();
        let many = list_pending_skills(&dir).unwrap();
        assert_eq!(many.len(), 2);
        assert_eq!(many[0].name, "aaa-pending");
        assert!(many.iter().any(|p| p.name == c1.name));

        let _ = std::fs::remove_dir_all(&dir);
    }

    #[test]
    fn format_pending_review_notice_respects_timing_and_count() {
        let empty: Vec<PendingSkillInfo> = Vec::new();
        assert!(format_pending_review_notice(
            &empty,
            PendingReviewTiming::CliWithStartNotice
        )
        .is_none());

        let pending = vec![
            PendingSkillInfo {
                name: "alpha-auto".into(),
                path: PathBuf::from("/tmp/alpha-auto"),
            },
            PendingSkillInfo {
                name: "beta-auto".into(),
                path: PathBuf::from("/tmp/beta-auto"),
            },
            PendingSkillInfo {
                name: "gamma-auto".into(),
                path: PathBuf::from("/tmp/gamma-auto"),
            },
            PendingSkillInfo {
                name: "delta-auto".into(),
                path: PathBuf::from("/tmp/delta-auto"),
            },
        ];

        // CliOnly never notices.
        assert!(
            format_pending_review_notice(&pending, PendingReviewTiming::CliOnly).is_none()
        );

        let notice =
            format_pending_review_notice(&pending, PendingReviewTiming::CliWithStartNotice)
                .expect("notice when pending + CliWithStartNotice");
        assert!(notice.contains("Pending skills (4)"));
        assert!(notice.contains("alpha-auto"));
        assert!(notice.contains("+2 more"));
        assert!(notice.contains("weft skills pending"));
        assert!(notice.contains("approve"));
        assert!(notice.contains("reject"));

        // Single name, no "+N more".
        let one = format_pending_review_notice(
            &pending[..1],
            PendingReviewTiming::CliWithStartNotice,
        )
        .unwrap();
        assert!(one.contains("Pending skills (1): alpha-auto"));
        assert!(!one.contains("more"));
    }

    #[test]
    fn pending_review_start_notice_best_effort() {
        let dir = std::env::temp_dir().join("clawft_autogen_start_notice_weft74");
        let _ = std::fs::remove_dir_all(&dir);
        let _ = std::fs::create_dir_all(&dir);

        assert!(
            pending_review_start_notice(Some(&dir), PendingReviewTiming::CliWithStartNotice)
                .is_none()
        );

        let pattern = ToolCallPattern::new(vec!["x".into(), "y".into()]);
        let candidate = generate_skill_md(&pattern);
        install_pending_skill(&candidate, &dir).unwrap();

        let notice =
            pending_review_start_notice(Some(&dir), PendingReviewTiming::CliWithStartNotice)
                .expect("pending skill should produce start notice");
        assert!(notice.contains(&candidate.name));
        assert!(notice.contains("weft skills pending"));

        // CliOnly suppresses notice even when pending exists.
        assert!(
            pending_review_start_notice(Some(&dir), PendingReviewTiming::CliOnly).is_none()
        );

        let _ = std::fs::remove_dir_all(&dir);
    }

    #[test]
    fn generated_skill_has_minimal_permissions() {
        let pattern = ToolCallPattern::new(vec!["read_file".into()]);
        let candidate = generate_skill_md(&pattern);

        // Generated skill should NOT have shell, network, or broad FS access
        assert!(!candidate.skill_md.contains("shell: true"));
        assert!(!candidate.skill_md.contains("network:"));
        // user-invocable should be false for safety
        assert!(candidate.skill_md.contains("user-invocable: false"));
        // WEFT-75: explicit minimal filesystem allowlist
        assert!(candidate.skill_md.contains("filesystem:"));
        assert!(candidate.skill_md.contains("readable-paths:"));
        assert!(candidate.skill_md.contains("writable-paths:"));
        assert!(candidate.skill_md.contains("allow-create: false"));
        assert!(candidate.skill_md.contains("allow-delete: false"));
        assert!(candidate.skill_md.contains(&format!("- {AUTOGEN_WORKSPACE_PATH}")));
    }

    // -- Filesystem allowlist (WEFT-75) --

    #[test]
    fn generate_skill_md_emits_explicit_minimal_allowlist() {
        let pattern = ToolCallPattern::new(vec!["read_file".into(), "edit_file".into()]);
        let candidate = generate_skill_md(&pattern);
        let minimal = SkillFilesystemAllowlist::minimal_autogen();

        // Round-trip via skills_v2 parser so metadata.filesystem is populated.
        let skill = crate::agent::skills_v2::parse_skill_md(&candidate.skill_md, None)
            .expect("generated SKILL.md must parse");
        assert!(is_autogenerated_skill(&skill));
        let fs = SkillFilesystemAllowlist::from_skill(&skill)
            .expect("autogen skill must carry filesystem allowlist");
        assert_eq!(fs, minimal);
        assert_eq!(fs.readable_paths, vec![AUTOGEN_WORKSPACE_PATH]);
        assert_eq!(fs.writable_paths, vec![AUTOGEN_WORKSPACE_PATH]);
        assert!(!fs.allow_create);
        assert!(!fs.allow_delete);
    }

    #[test]
    fn broader_host_effective_allowlist_is_minimum() {
        // Synthetic broader-perm host loads autogen skill; effective is skill minimum.
        let pattern = ToolCallPattern::new(vec!["read_file".into()]);
        let candidate = generate_skill_md(&pattern);
        let skill = crate::agent::skills_v2::parse_skill_md(&candidate.skill_md, None).unwrap();

        let host = SkillFilesystemAllowlist {
            readable_paths: vec![
                ".".into(),
                "/home".into(),
                "/tmp".into(),
                "/var/log".into(),
            ],
            writable_paths: vec![".".into(), "/tmp".into(), "/home/out".into()],
            allow_create: true,
            allow_delete: true,
        };

        let effective = effective_filesystem_on_load(&skill, &host);
        let minimal = SkillFilesystemAllowlist::minimal_autogen();

        assert_eq!(
            effective, minimal,
            "broader host must not widen autogen skill FS beyond minimal"
        );
        assert!(
            effective.is_subset_of(&host),
            "effective must never exceed host"
        );
        assert!(
            effective.is_subset_of(&minimal),
            "effective must not exceed skill-declared minimum"
        );
        assert!(!effective.allow_create);
        assert!(!effective.allow_delete);
    }

    #[test]
    fn narrower_host_further_restricts_autogen_skill() {
        let pattern = ToolCallPattern::new(vec!["read_file".into()]);
        let candidate = generate_skill_md(&pattern);
        let skill = crate::agent::skills_v2::parse_skill_md(&candidate.skill_md, None).unwrap();

        // Host does not allow workspace "."; only /tmp.
        let host = SkillFilesystemAllowlist {
            readable_paths: vec!["/tmp".into()],
            writable_paths: vec!["/tmp".into()],
            allow_create: false,
            allow_delete: false,
        };

        let effective = effective_filesystem_on_load(&skill, &host);
        // Intersection of {.} ∩ {/tmp} is empty — skill cannot exceed host.
        assert!(effective.readable_paths.is_empty());
        assert!(effective.writable_paths.is_empty());
        assert!(effective.is_subset_of(&host));
    }

    #[test]
    fn intersect_filesystem_allowlist_ands_boolean_flags() {
        let skill = SkillFilesystemAllowlist {
            readable_paths: vec![".".into()],
            writable_paths: vec![".".into()],
            allow_create: true,
            allow_delete: false,
        };
        let host = SkillFilesystemAllowlist {
            readable_paths: vec![".".into()],
            writable_paths: vec![".".into()],
            allow_create: false,
            allow_delete: true,
        };
        let effective = intersect_filesystem_allowlist(&skill, &host);
        assert!(!effective.allow_create);
        assert!(!effective.allow_delete);
    }

    #[test]
    fn non_autogen_skill_without_fs_defers_to_host() {
        let skill = SkillDefinition::new("manual", "hand-written skill");
        let host = SkillFilesystemAllowlist {
            readable_paths: vec!["/workspace".into()],
            writable_paths: vec!["/workspace".into()],
            allow_create: true,
            allow_delete: false,
        };
        let effective = effective_filesystem_on_load(&skill, &host);
        assert_eq!(effective, host);
    }

    #[test]
    fn legacy_autogen_without_fs_block_uses_minimal_defaults() {
        // Older generated skills only had autogenerated: true.
        let mut skill = SkillDefinition::new("legacy-auto", "old autogen skill");
        skill
            .metadata
            .insert("autogenerated".into(), serde_json::json!(true));
        let host = SkillFilesystemAllowlist {
            readable_paths: vec![".".into(), "/extra".into()],
            writable_paths: vec![".".into(), "/extra".into()],
            allow_create: true,
            allow_delete: true,
        };
        let effective = effective_filesystem_on_load(&skill, &host);
        assert_eq!(effective, SkillFilesystemAllowlist::minimal_autogen());
    }

    #[test]
    fn is_enabled_reflects_config() {
        let enabled = PatternDetector::new(enabled_config());
        assert!(enabled.is_enabled());

        let disabled = PatternDetector::new(disabled_config());
        assert!(!disabled.is_enabled());
    }

    // -- Trajectory-based improvement tests --

    fn make_test_learner() -> TrajectoryLearner {
        use crate::pipeline::learner::TrajectoryLearnerConfig;
        use crate::pipeline::traits::{
            ChatRequest, LlmMessage, QualityScore, RoutingDecision, Trajectory,
        };
        use clawft_types::provider::{ContentBlock, LlmResponse, StopReason, Usage};

        let learner = TrajectoryLearner::new(TrajectoryLearnerConfig::default());

        let make_traj = |overall: f32, content: &str| Trajectory {
            request: ChatRequest {
                messages: vec![LlmMessage {
                    role: "user".into(),
                    content: content.into(),
                    tool_call_id: None,
                    tool_calls: None,
                }],
                tools: vec![],
                model: None,
                max_tokens: None,
                temperature: None,
                auth_context: None,
                complexity_boost: 0.0,
                tool_choice: None,
            },
            routing: RoutingDecision::default(),
            response: LlmResponse {
                id: "r".into(),
                content: vec![ContentBlock::Text { text: "ok".into() }],
                stop_reason: StopReason::EndTurn,
                usage: Usage {
                    input_tokens: 5,
                    output_tokens: 2,
                    total_tokens: 0,
                },
                metadata: std::collections::HashMap::new(),
            },
            quality: QualityScore {
                overall,
                relevance: overall,
                coherence: overall,
            },
        };

        // Record some good and poor trajectories
        use crate::pipeline::traits::LearningBackend;
        learner.record(&make_traj(0.95, "Write a function to sort a list"));
        learner.record(&make_traj(0.9, "Explain recursion with examples"));
        learner.record(&make_traj(0.3, "Fix the bug"));
        learner.record(&make_traj(0.4, "Summarize article"));

        learner
    }

    #[test]
    fn improve_skill_instructions_with_patterns() {
        let learner = make_test_learner();
        let original = "Use the read_file tool\nCheck the output";
        let improved = improve_skill_instructions(original, &learner);

        // Should be different since we have trajectory data
        assert!(!improved.is_empty());
    }

    #[test]
    fn improve_skill_instructions_empty_learner_returns_original() {
        use crate::pipeline::learner::TrajectoryLearnerConfig;
        let learner = TrajectoryLearner::new(TrajectoryLearnerConfig::default());
        let original = "Use the read_file tool";
        let result = improve_skill_instructions(original, &learner);
        assert_eq!(result, original);
    }

    #[test]
    fn generate_skill_md_with_learning_no_learner() {
        let pattern = ToolCallPattern::new(vec!["a".into(), "b".into()]);
        let candidate = generate_skill_md_with_learning(&pattern, None);
        assert!(candidate.skill_md.contains("autogenerated: true"));
        let baseline = generate_skill_md(&pattern);
        assert_eq!(candidate.skill_md, baseline.skill_md);
    }

    #[test]
    fn generate_skill_md_with_learning_uses_learner() {
        let learner = make_test_learner();
        let pattern = ToolCallPattern::new(vec!["read_file".into(), "edit_file".into()]);
        let baseline = generate_skill_md(&pattern);
        let candidate = generate_skill_md_with_learning(&pattern, Some(&learner));

        // Frontmatter + tools remain valid.
        assert!(candidate.skill_md.contains("read_file"));
        assert!(candidate.skill_md.contains("autogenerated: true"));
        assert!(candidate.skill_md.contains("filesystem:"));
        assert!(!candidate.skill_md.is_empty());

        // Learning mutates the body using successful trajectories
        // (make_test_learner seeds quality ≥ 0.8 → AddExamples strategy).
        assert_ne!(
            candidate.skill_md, baseline.skill_md,
            "trajectory learning must change SKILL.md body"
        );
        assert!(
            candidate.skill_md.contains("Successful Examples")
                || candidate.skill_md.contains("Write a function")
                || candidate.skill_md.contains("Explain recursion")
                || candidate.skill_md.contains("Step "),
            "expected example injection or rephrase markers in improved SKILL.md: {}",
            candidate.skill_md
        );
    }

    /// End-to-end WEFT-66: AutogenConfig with an active TrajectoryLearner
    /// drives learning-based generation the same way the agent loop does.
    #[test]
    fn e2e_autogen_config_learner_improves_skill_md() {
        let learner = Arc::new(make_test_learner());
        let config = AutogenConfig {
            enabled: true,
            threshold: 2,
            max_pending: 10,
            install_dir: None,
            learner: Some(Arc::clone(&learner)),
        };
        assert!(config.learner.is_some());

        let mut detector = PatternDetector::new(config);
        // Threshold=2: record the same 2-tool sequence twice.
        for _ in 0..2 {
            detector.record_tool_call("read_file");
            detector.record_tool_call("edit_file");
        }
        let candidates = detector.detect_candidates();
        assert!(
            !candidates.is_empty(),
            "pattern detector should emit at least one candidate"
        );

        let pattern = &candidates[0];
        let baseline = generate_skill_md(pattern);
        let improved = generate_skill_md_with_learning(pattern, detector.learner());

        assert_ne!(improved.skill_md, baseline.skill_md);
        assert!(improved.skill_md.contains("read_file"));
        assert!(improved.skill_md.contains("autogenerated: true"));
        assert!(
            improved.skill_md.contains("Successful Examples")
                || improved.skill_md.contains("Write a function")
                || improved.skill_md.contains("Explain recursion"),
            "improved body should reflect synthetic high-quality trajectories: {}",
            improved.skill_md
        );
    }

    // -- WEFT-348: skill auto-promotion (.claude/skills → .clawft/skills) --

    fn write_claude_skill(root: &Path, name: &str, body: &str) -> PathBuf {
        let skill_dir = root
            .join(".claude")
            .join("skills")
            .join(name);
        std::fs::create_dir_all(&skill_dir).unwrap();
        let md = format!(
            "---\nname: {name}\ndescription: \"test skill {name}\"\nversion: 0.1.0\n---\n\n{body}\n"
        );
        std::fs::write(skill_dir.join("SKILL.md"), md).unwrap();
        skill_dir
    }

    fn promotion_fixture() -> (tempfile::TempDir, PathBuf, PathBuf) {
        let tmp = tempfile::tempdir().expect("tempdir");
        let claude_skill = write_claude_skill(tmp.path(), "plane-workflow", "Do plane things.");
        let target = tmp.path().join(".clawft").join("skills");
        std::fs::create_dir_all(&target).unwrap();
        (tmp, claude_skill, target)
    }

    #[test]
    fn is_claude_skills_path_detects_marker() {
        assert!(is_claude_skills_path(Path::new(
            "/home/u/proj/.claude/skills/foo"
        )));
        assert!(is_claude_skills_path(Path::new(
            ".claude/skills/foo/SKILL.md"
        )));
        assert!(!is_claude_skills_path(Path::new(
            "/home/u/proj/.clawft/skills/foo"
        )));
        assert!(!is_claude_skills_path(Path::new("skills/foo")));
        assert!(!is_claude_skills_path(Path::new(
            "/home/u/.claude/not-skills/foo"
        )));
    }

    #[test]
    fn promotion_config_defaults() {
        let cfg = SkillPromotionConfig::default();
        assert!(cfg.enabled);
        assert_eq!(cfg.threshold, DEFAULT_PROMOTION_THRESHOLD);
        assert_eq!(DEFAULT_PROMOTION_THRESHOLD, 10);
    }

    #[test]
    fn promotion_config_from_types() {
        let types_cfg = clawft_types::config::SkillPromotionConfig {
            enabled: false,
            threshold: 25,
        };
        let cfg = SkillPromotionConfig::from_types(&types_cfg);
        assert!(!cfg.enabled);
        assert_eq!(cfg.threshold, 25);
    }

    #[test]
    fn promotion_disabled_is_noop() {
        let (tmp, source, target) = promotion_fixture();
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(2)
                .with_target_dir(&target),
        );
        // Force disabled
        let mut cfg = promoter.config().clone();
        cfg.enabled = false;
        promoter = SkillPromoter::new(cfg.with_target_dir(&target).with_threshold(2));

        let outcome = promoter.record_success("plane-workflow", &source);
        assert_eq!(outcome, PromotionOutcome::Disabled);
        assert_eq!(promoter.success_count("plane-workflow"), 0);
        assert!(!target.join("plane-workflow").exists());
        drop(tmp);
    }

    #[test]
    fn promotion_not_eligible_for_clawft_source() {
        let tmp = tempfile::tempdir().unwrap();
        let clawft_skill = tmp.path().join(".clawft").join("skills").join("local");
        std::fs::create_dir_all(&clawft_skill).unwrap();
        std::fs::write(clawft_skill.join("SKILL.md"), "---\nname: local\n---\n").unwrap();
        let target = tmp.path().join("out");
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(1)
                .with_target_dir(&target),
        );
        let outcome = promoter.record_success("local", &clawft_skill);
        assert_eq!(outcome, PromotionOutcome::NotEligible);
        assert_eq!(promoter.success_count("local"), 0);
    }

    #[test]
    fn promotion_counts_below_threshold() {
        let (tmp, source, target) = promotion_fixture();
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(5)
                .with_target_dir(&target),
        );

        for i in 1..=4 {
            match promoter.record_success("plane-workflow", &source) {
                PromotionOutcome::Counted {
                    success_count,
                    threshold,
                } => {
                    assert_eq!(success_count, i);
                    assert_eq!(threshold, 5);
                }
                other => panic!("expected Counted, got {other:?}"),
            }
        }
        assert!(!target.join("plane-workflow").exists());
        assert!(promoter.audit_log().is_empty());
        drop(tmp);
    }

    #[test]
    fn promotion_auto_promotes_at_threshold_and_audits() {
        let (tmp, source, target) = promotion_fixture();
        let audit = tmp.path().join("audit.jsonl");
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(3)
                .with_target_dir(&target)
                .with_audit_log_path(&audit),
        );

        for _ in 0..2 {
            assert!(matches!(
                promoter.record_success("plane-workflow", &source),
                PromotionOutcome::Counted { .. }
            ));
        }

        let outcome = promoter.record_success("plane-workflow", &source);
        match outcome {
            PromotionOutcome::Promoted { entry } => {
                assert_eq!(entry.skill_name, "plane-workflow");
                assert_eq!(entry.successful_invocations, 3);
                assert_eq!(entry.threshold, 3);
                assert_eq!(entry.event, SkillPromotionAuditEntry::EVENT_TYPE);
                assert!(entry.target_path.ends_with("plane-workflow"));
                assert!(entry.target_path.join("SKILL.md").exists());
                assert!(entry.target_path.join(".promoted-from-claude").exists());
            }
            other => panic!("expected Promoted, got {other:?}"),
        }

        // Audit log in memory + on disk.
        assert_eq!(promoter.audit_log().len(), 1);
        let disk = std::fs::read_to_string(&audit).expect("audit file");
        assert!(disk.contains("skill.promoted"));
        assert!(disk.contains("plane-workflow"));

        // Content was copied.
        let promoted_md = std::fs::read_to_string(target.join("plane-workflow").join("SKILL.md"))
            .expect("promoted SKILL.md");
        assert!(promoted_md.contains("plane-workflow"));
        assert!(promoted_md.contains("Do plane things."));
        drop(tmp);
    }

    #[test]
    fn promotion_is_idempotent_after_first() {
        let (tmp, source, target) = promotion_fixture();
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(2)
                .with_target_dir(&target),
        );

        assert!(matches!(
            promoter.record_success("plane-workflow", &source),
            PromotionOutcome::Counted { .. }
        ));
        assert!(matches!(
            promoter.record_success("plane-workflow", &source),
            PromotionOutcome::Promoted { .. }
        ));
        // Further successes do not re-promote / re-audit.
        match promoter.record_success("plane-workflow", &source) {
            PromotionOutcome::AlreadyPromoted { target: t } => {
                assert!(t.ends_with("plane-workflow"));
            }
            other => panic!("expected AlreadyPromoted, got {other:?}"),
        }
        assert_eq!(promoter.audit_log().len(), 1);
        drop(tmp);
    }

    #[test]
    fn promotion_detects_preexisting_target_as_already_promoted() {
        let (tmp, source, target) = promotion_fixture();
        // Pre-create destination as if a previous process already promoted.
        let dest = target.join("plane-workflow");
        std::fs::create_dir_all(&dest).unwrap();
        std::fs::write(dest.join("SKILL.md"), "---\nname: plane-workflow\n---\n").unwrap();

        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(1)
                .with_target_dir(&target),
        );
        match promoter.record_success("plane-workflow", &source) {
            PromotionOutcome::AlreadyPromoted { target: t } => {
                assert_eq!(t, dest);
            }
            other => panic!("expected AlreadyPromoted, got {other:?}"),
        }
        assert_eq!(promoter.success_count("plane-workflow"), 0);
        drop(tmp);
    }

    #[test]
    fn promotion_accepts_skill_md_path_as_source() {
        let (tmp, source, target) = promotion_fixture();
        let skill_md = source.join("SKILL.md");
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(1)
                .with_target_dir(&target),
        );
        match promoter.record_success("plane-workflow", &skill_md) {
            PromotionOutcome::Promoted { entry } => {
                assert!(entry.target_path.join("SKILL.md").exists());
            }
            other => panic!("expected Promoted, got {other:?}"),
        }
        drop(tmp);
    }

    #[test]
    fn promote_skill_directory_rejects_traversal_names() {
        let tmp = tempfile::tempdir().unwrap();
        let src = tmp.path().join("src");
        std::fs::create_dir_all(&src).unwrap();
        let dest_root = tmp.path().join("dest");
        let err = promote_skill_directory(&src, &dest_root, "../escape").unwrap_err();
        assert!(err.contains("invalid skill name"), "{err}");
    }

    #[test]
    fn configurable_threshold_example_ten() {
        // AC: configurable promotion threshold (e.g. ≥10 successful invocations).
        let (tmp, source, target) = promotion_fixture();
        let mut promoter = SkillPromoter::new(
            SkillPromotionConfig::default()
                .with_threshold(10)
                .with_target_dir(&target),
        );
        for _ in 0..9 {
            assert!(matches!(
                promoter.record_success("plane-workflow", &source),
                PromotionOutcome::Counted { .. }
            ));
        }
        assert!(!target.join("plane-workflow").exists());
        assert!(matches!(
            promoter.record_success("plane-workflow", &source),
            PromotionOutcome::Promoted { .. }
        ));
        assert!(target.join("plane-workflow").join("SKILL.md").exists());
        drop(tmp);
    }
}