use std::collections::HashSet;
use std::io::Write;
use std::process::{Command, Stdio};
use std::sync::LazyLock;
use std::time::Duration;

use crate::doctor;
use crate::markdown;
use crate::model::{AppContext, CodeBlock, CommandDef, PLACEHOLDER_RE};
use crate::registry_config::{self, HttpMethod, RegistryEndpoint};
use crate::runner::CREFT_ARG_ENV_PREFIX;
use crate::store;

/// Outcome of validating a parsed skill.
#[derive(Debug)]
pub struct ValidationResult {
    /// Hard errors that block the save.
    pub errors: Vec<ValidationDiagnostic>,
    /// Warnings printed to stderr but do not block the save.
    pub warnings: Vec<ValidationDiagnostic>,
}

impl ValidationResult {
    /// Returns `true` if the result contains any hard errors (blocking the save).
    pub fn has_errors(&self) -> bool {
        !self.errors.is_empty()
    }

    #[cfg(test)]
    pub fn is_clean(&self) -> bool {
        self.errors.is_empty() && self.warnings.is_empty()
    }
}

/// A single validation finding, either an error or a warning.
#[derive(Debug)]
pub struct ValidationDiagnostic {
    /// Which code block (0-indexed) this finding applies to.
    /// None for skill-level findings (e.g., no code blocks).
    pub block_index: Option<usize>,
    /// Language of the block, for display context.
    pub lang: Option<String>,
    /// Human-readable description of the problem.
    pub message: String,
    /// Optional line number within the code block (1-indexed).
    pub line: Option<usize>,
}

impl std::fmt::Display for ValidationDiagnostic {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match (self.block_index, &self.lang, self.line) {
            (Some(idx), Some(lang), Some(line)) => {
                write!(
                    f,
                    "block {} ({}), line {}: {}",
                    idx + 1,
                    lang,
                    line,
                    self.message
                )
            }
            (Some(idx), Some(lang), None) => {
                write!(f, "block {} ({}): {}", idx + 1, lang, self.message)
            }
            (Some(idx), None, _) => {
                write!(f, "block {}: {}", idx + 1, self.message)
            }
            (None, _, _) => {
                write!(f, "{}", self.message)
            }
        }
    }
}

/// Maximum description length before a warning is emitted.
/// Descriptions longer than this degrade `creft list` output.
pub(crate) const DESCRIPTION_WARN_LEN: usize = 80;

/// Flag long names that creft intercepts before the skill's argument parser.
///
/// Keep in sync with the filter closure in `src/cmd/run.rs`.
const RESERVED_FLAG_NAMES: &[&str] = &["help", "docs", "dry-run", "verbose", "version"];

/// Flag short forms that creft intercepts before the skill's argument parser.
///
/// Keep in sync with the filter closure in `src/cmd/run.rs`.
const RESERVED_SHORT_FLAGS: &[char] = &['h', 'v', 'V'];

/// Validate flag names, arg names, and short forms for conflicts and reserved names.
///
/// All findings are hard errors because they represent conditions that guarantee
/// the declared flag or arg is unreachable at runtime.
fn check_frontmatter_names(def: &CommandDef, errors: &mut Vec<ValidationDiagnostic>) {
    // Stage 2: character and format validation runs before conflict detection so
    // that a malformed name does not generate a spurious duplicate error as well.
    for flag in &def.flags {
        validate_flag_name_format(&flag.name, errors);
        if let Some(short) = &flag.short {
            validate_short_format(short, errors);
        }
    }
    for arg in &def.args {
        validate_arg_name_format(&arg.name, errors);
    }

    // Stage 1: reserved name checks.
    for flag in &def.flags {
        if RESERVED_FLAG_NAMES.contains(&flag.name.as_str()) {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!(
                    "flag '{}' is reserved by creft and cannot be declared",
                    flag.name
                ),
                line: None,
            });
        }
        // Only check valid single-char shorts; malformed ones were already
        // flagged in the format pass above.
        if let Some(short) = &flag.short
            && let Some(ch) = short.chars().next()
            && short.len() == 1
            && RESERVED_SHORT_FLAGS.contains(&ch)
        {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!(
                    "flag short form '{}' is reserved by creft and cannot be declared",
                    short
                ),
                line: None,
            });
        }
    }

    // Stage 1: duplicate detection.
    let mut seen_flag_names: HashSet<&str> = HashSet::new();
    for flag in &def.flags {
        if !seen_flag_names.insert(flag.name.as_str()) {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!("duplicate flag name '{}'", flag.name),
                line: None,
            });
        }
    }

    let mut seen_shorts: HashSet<char> = HashSet::new();
    for flag in &def.flags {
        if let Some(short) = &flag.short
            && let Some(ch) = short.chars().next()
            && short.chars().count() == 1
            && !seen_shorts.insert(ch)
        {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!("duplicate flag short form '{}'", short),
                line: None,
            });
        }
    }

    let mut seen_arg_names: HashSet<&str> = HashSet::new();
    for arg in &def.args {
        if !seen_arg_names.insert(arg.name.as_str()) {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!("duplicate arg name '{}'", arg.name),
                line: None,
            });
        }
    }

    // Stage 1: flag-arg name conflict.
    let flag_names: HashSet<&str> = def.flags.iter().map(|f| f.name.as_str()).collect();
    for arg in &def.args {
        if flag_names.contains(arg.name.as_str()) {
            errors.push(ValidationDiagnostic {
                block_index: None,
                lang: None,
                message: format!("flag '{}' conflicts with arg '{}'", arg.name, arg.name),
                line: None,
            });
        }
    }
}

/// Validate that a flag name uses only allowed characters and starts correctly.
///
/// Valid: `[a-zA-Z_][a-zA-Z0-9_-]*`. Empty names and names starting with a digit
/// are rejected because lexopt cannot parse them as `--flag` tokens.
fn validate_flag_name_format(name: &str, errors: &mut Vec<ValidationDiagnostic>) {
    if name.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: "flag name cannot be empty".into(),
            line: None,
        });
        return;
    }
    let first = name.chars().next().unwrap();
    if !first.is_ascii_alphabetic() && first != '_' {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "flag name '{}' must start with a letter or underscore (expected [a-zA-Z_][a-zA-Z0-9_-]*)",
                name
            ),
            line: None,
        });
        return;
    }
    if !name
        .chars()
        .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
    {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "flag name '{}' contains invalid characters (expected [a-zA-Z_][a-zA-Z0-9_-]*)",
                name
            ),
            line: None,
        });
    }
}

/// Validate that an arg name uses only allowed characters and starts correctly.
///
/// Same character rules as flag names — args appear as template placeholders
/// and must survive hyphen-to-underscore conversion without ambiguity.
fn validate_arg_name_format(name: &str, errors: &mut Vec<ValidationDiagnostic>) {
    if name.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: "arg name cannot be empty".into(),
            line: None,
        });
        return;
    }
    let first = name.chars().next().unwrap();
    if !first.is_ascii_alphabetic() && first != '_' {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "arg name '{}' must start with a letter or underscore (expected [a-zA-Z_][a-zA-Z0-9_-]*)",
                name
            ),
            line: None,
        });
        return;
    }
    if !name
        .chars()
        .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
    {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "arg name '{}' contains invalid characters (expected [a-zA-Z_][a-zA-Z0-9_-]*)",
                name
            ),
            line: None,
        });
    }
}

/// Validate that a flag short form is a single ASCII letter.
///
/// Digits are excluded because lexopt interprets `-1` as a negative number
/// argument rather than a flag. Multi-char values are not valid short forms.
fn validate_short_format(short: &str, errors: &mut Vec<ValidationDiagnostic>) {
    if short.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: "flag short form cannot be empty".into(),
            line: None,
        });
        return;
    }
    let mut chars = short.chars();
    let ch = chars.next().unwrap();
    if chars.next().is_some() {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "flag short form must be a single ASCII letter, got '{}'",
                short
            ),
            line: None,
        });
        return;
    }
    if !ch.is_ascii_alphabetic() {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "flag short form must be a single ASCII letter, got '{}'",
                short
            ),
            line: None,
        });
    }
}

/// Validate a parsed skill's code blocks.
///
/// Returns a `ValidationResult` with errors (blocking) and warnings (advisory).
/// Does not modify the skill or execute any code.
///
/// Pass `None` for `ctx` to skip command and sub-skill existence checks
/// (useful in tests where no registry is available).
pub fn validate_skill(
    def: &CommandDef,
    blocks: &[CodeBlock],
    body: &str,
    ctx: Option<&AppContext>,
) -> ValidationResult {
    let mut errors = Vec::new();
    let mut warnings = Vec::new();

    // Fence nesting check runs before other checks — a skill with improperly
    // nested fences will be misparsed by the runner, potentially invoking LLM
    // providers unintentionally. This is a correctness issue, not a style issue.
    for w in markdown::check_fence_nesting(body) {
        errors.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: w.message,
            line: None,
        });
    }

    // Frontmatter name checks run first among structural checks — they are more
    // fundamental than block-level problems and give the author the most
    // actionable feedback.
    check_frontmatter_names(def, &mut errors);

    // Skill-level check: description length.
    if def.description.len() > DESCRIPTION_WARN_LEN {
        warnings.push(ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: format!(
                "description is long ({} chars). Keep descriptions under {} characters for clean list output",
                def.description.len(),
                DESCRIPTION_WARN_LEN,
            ),
            line: None,
        });
    }

    for (i, block) in blocks.iter().enumerate() {
        // LLM blocks have their own validation path.
        if block.lang == "llm" {
            check_llm_block(def, block, i, &mut errors, &mut warnings);
            continue;
        }

        check_placeholders(def, block, i, &mut warnings);

        // Record the error count before syntax check so we can gate shellcheck
        // on whether syntax passed.
        let errors_before = errors.len();
        check_syntax(block, i, &mut errors);
        let syntax_ok = errors.len() == errors_before;

        if syntax_ok && doctor::is_shell_lang(&block.lang) {
            let sanitized = sanitize_placeholders(&block.code);
            check_shellcheck(&sanitized, i, &block.lang, &mut warnings);
        }

        // Check creft sub-skill invocations reference real skills.
        if doctor::is_shell_lang(&block.lang) {
            check_sub_skill_existence(block, i, ctx, &mut warnings);
        }

        check_dependency_resolution(block, i, ctx, &mut warnings);
        check_unknown_lang_binary(block, i, &mut warnings);
        check_extension_value(block, i, &mut errors);
    }

    ValidationResult { errors, warnings }
}

fn check_placeholders(
    def: &CommandDef,
    block: &CodeBlock,
    block_index: usize,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    let mut declared: HashSet<&str> = HashSet::new();
    declared.insert("prev");
    for arg in &def.args {
        declared.insert(arg.name.as_str());
    }
    for flag in &def.flags {
        declared.insert(flag.name.as_str());
    }

    let re = &*PLACEHOLDER_RE;
    for caps in re.captures_iter(&block.code) {
        let name = caps.get(1).map(|m| m.as_str()).unwrap_or("");
        let has_default = caps.get(2).is_some();

        if name == "prev" && block_index == 0 {
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(block.lang.clone()),
                message: "{{prev}} in first block has no preceding block output".into(),
                line: None,
            });
        } else if !declared.contains(name) {
            if has_default {
                warnings.push(ValidationDiagnostic {
                    block_index: Some(block_index),
                    lang: Some(block.lang.clone()),
                    message: format!(
                        "placeholder {{{{{}}}}} is not declared in args or flags (has default)",
                        name
                    ),
                    line: None,
                });
            } else {
                warnings.push(ValidationDiagnostic {
                    block_index: Some(block_index),
                    lang: Some(block.lang.clone()),
                    message: format!(
                        "placeholder {{{{{}}}}} is not declared in args or flags",
                        name
                    ),
                    line: None,
                });
            }
        }
    }

    // In shell blocks, every declared arg and flag is injected as a
    // CREFT_ARG_<NAME> environment variable. Inform the author of the mapping
    // so they can read the value at runtime without guessing the transformation.
    if doctor::is_shell_lang(&block.lang) {
        let all_names = def
            .args
            .iter()
            .map(|a| a.name.as_str())
            .chain(def.flags.iter().map(|f| f.name.as_str()));
        for name in all_names {
            let env_name = format!(
                "{}{}",
                CREFT_ARG_ENV_PREFIX,
                name.replace('-', "_").to_uppercase()
            );
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(block.lang.clone()),
                message: format!(
                    "shell block can read '{}' as environment variable {}",
                    name, env_name
                ),
                line: None,
            });
        }
    }
}

/// Validate an `llm` code block.
///
/// Checks:
/// 1. YAML header parse error (hard error)
/// 2. Empty prompt after header extraction (hard error)
/// 3. Placeholder references in the prompt body (same as other blocks)
/// 4. Provider CLI not on PATH (warning)
fn check_llm_block(
    def: &CommandDef,
    block: &CodeBlock,
    block_index: usize,
    errors: &mut Vec<ValidationDiagnostic>,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    // YAML header parse failure takes priority — emit error and stop.
    if let Some(parse_err) = &block.llm_parse_error {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some("llm".to_string()),
            message: format!("invalid YAML header in llm block: {}", parse_err),
            line: None,
        });
        return;
    }

    if block.code.trim().is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some("llm".to_string()),
            message: "llm block has no prompt text".to_string(),
            line: None,
        });
        return;
    }

    check_placeholders(def, block, block_index, warnings);

    // Provider CLI PATH check (warning only — provider may be available in the
    // execution environment but not the authoring environment).
    if let Some(config) = &block.llm_config {
        let provider = if config.provider.is_empty() {
            "claude"
        } else {
            config.provider.as_str()
        };
        if doctor::which_path(provider).is_none() {
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some("llm".to_string()),
                message: format!(
                    "llm provider '{}' not found on PATH (install the provider CLI or ignore if \
                     running in a different environment)",
                    provider
                ),
                line: None,
            });
        }
    }
}

/// Replace all `{{placeholder}}` and `{{placeholder|default}}` patterns with
/// `__CREFT_PH__` so that syntax checkers see a valid identifier rather than
/// a template token that may look like illegal syntax.
///
/// Using a bare identifier works in all supported languages:
/// - Shell: `__CREFT_PH__` is a valid word / variable name
/// - Python: `__CREFT_PH__` is a valid identifier
/// - Node/JS: `__CREFT_PH__` is a valid identifier
/// - Ruby: `__CREFT_PH__` is a valid identifier (dunders style, syntactically fine)
fn sanitize_placeholders(code: &str) -> String {
    let re = &*PLACEHOLDER_RE;
    re.replace_all(code, "__CREFT_PH__").into_owned()
}

/// Dispatch syntax checking to the language-appropriate function.
fn check_syntax(block: &CodeBlock, block_index: usize, errors: &mut Vec<ValidationDiagnostic>) {
    match block.lang.as_str() {
        "bash" | "sh" | "zsh" => check_shell_syntax(block, block_index, errors),
        "python" | "python3" => check_python_syntax(block, block_index, errors),
        "node" | "javascript" | "js" => check_node_syntax(block, block_index, errors),
        _ => {} // Unknown language — skip silently
    }
}

/// Run `bash -n` (or `sh -n` / `zsh -n`) on sanitized code.
///
/// If the tool is not on PATH the check is silently skipped.
/// After a clean `bash -n` pass, also runs shellcheck (warnings only) if available.
fn check_shell_syntax(
    block: &CodeBlock,
    block_index: usize,
    errors: &mut Vec<ValidationDiagnostic>,
) {
    let sanitized = sanitize_placeholders(&block.code);

    let shell = match block.lang.as_str() {
        "sh" => "sh",
        "zsh" => "zsh",
        _ => "bash",
    };

    if doctor::which_path(shell).is_none() {
        return;
    }

    let mut tmp = match tempfile::Builder::new().suffix(".sh").tempfile() {
        Ok(f) => f,
        Err(_) => return,
    };
    if tmp.write_all(sanitized.as_bytes()).is_err() {
        return;
    }
    let _ = tmp.flush(); // flush before handing the path to a subprocess

    let output = match Command::new(shell)
        .arg("-n")
        .arg(tmp.path())
        .stdout(Stdio::null())
        .stderr(Stdio::piped())
        .output()
    {
        Ok(o) => o,
        Err(_) => return,
    };

    if !output.status.success() {
        let stderr = String::from_utf8_lossy(&output.stderr);
        parse_shell_errors(&stderr, block_index, &block.lang, errors);
    }
}

/// Parse `bash -n` / `sh -n` stderr into diagnostics.
///
/// Expected format: `<file>: line N: <message>`
fn parse_shell_errors(
    stderr: &str,
    block_index: usize,
    lang: &str,
    errors: &mut Vec<ValidationDiagnostic>,
) {
    static SHELL_ERR_RE: LazyLock<regex::Regex> =
        LazyLock::new(|| regex::Regex::new(r": line (\d+): (.+)$").unwrap());

    let mut found_any = false;
    for line in stderr.lines() {
        if let Some(caps) = SHELL_ERR_RE.captures(line) {
            let lineno: usize = caps[1].parse().unwrap_or(0);
            let msg = caps[2].trim().to_string();
            errors.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(lang.to_string()),
                message: msg,
                line: if lineno > 0 { Some(lineno) } else { None },
            });
            found_any = true;
        }
    }

    if !found_any && !stderr.trim().is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some(lang.to_string()),
            message: stderr.trim().to_string(),
            line: None,
        });
    }
}

/// Run `python3 -B -c "import ast,sys; ast.parse(open(sys.argv[1]).read())"`.
///
/// If `python3` is not on PATH the check is silently skipped.
fn check_python_syntax(
    block: &CodeBlock,
    block_index: usize,
    errors: &mut Vec<ValidationDiagnostic>,
) {
    if doctor::which_path("python3").is_none() {
        return;
    }

    let sanitized = sanitize_placeholders(&block.code);

    let mut tmp = match tempfile::Builder::new().suffix(".py").tempfile() {
        Ok(f) => f,
        Err(_) => return,
    };
    if tmp.write_all(sanitized.as_bytes()).is_err() {
        return;
    }
    let _ = tmp.flush();

    let path_str = tmp.path().to_string_lossy().into_owned();

    let output = match Command::new("python3")
        .args([
            "-B",
            "-c",
            "import ast,sys; ast.parse(open(sys.argv[1]).read())",
            &path_str,
        ])
        .stdout(Stdio::null())
        .stderr(Stdio::piped())
        .output()
    {
        Ok(o) => o,
        Err(_) => return,
    };

    if !output.status.success() {
        let stderr = String::from_utf8_lossy(&output.stderr);
        parse_python_errors(&stderr, block_index, errors);
    }
}

/// Parse Python syntax error output into diagnostics.
///
/// Python stderr format (abbreviated):
/// ```
///   File "<file>", line N
///     <code>
///          ^
/// SyntaxError: <message>
/// ```
fn parse_python_errors(stderr: &str, block_index: usize, errors: &mut Vec<ValidationDiagnostic>) {
    static PY_LINE_RE: LazyLock<regex::Regex> =
        LazyLock::new(|| regex::Regex::new(r"line (\d+)").unwrap());
    static PY_ERR_RE: LazyLock<regex::Regex> = LazyLock::new(|| {
        regex::Regex::new(r"(?:SyntaxError|IndentationError|TabError): (.+)").unwrap()
    });

    let line_num = PY_LINE_RE
        .captures(stderr)
        .and_then(|c| c[1].parse::<usize>().ok());

    let message = if let Some(caps) = PY_ERR_RE.captures(stderr) {
        caps[1].trim().to_string()
    } else {
        stderr.trim().to_string()
    };

    if !message.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some("python".to_string()),
            message,
            line: line_num,
        });
    }
}

/// Run `node --check <tempfile>`.
///
/// If `node` is not on PATH the check is silently skipped.
fn check_node_syntax(
    block: &CodeBlock,
    block_index: usize,
    errors: &mut Vec<ValidationDiagnostic>,
) {
    if doctor::which_path("node").is_none() {
        return;
    }

    let sanitized = sanitize_placeholders(&block.code);

    let mut tmp = match tempfile::Builder::new().suffix(".js").tempfile() {
        Ok(f) => f,
        Err(_) => return,
    };
    if tmp.write_all(sanitized.as_bytes()).is_err() {
        return;
    }
    let _ = tmp.flush();

    let output = match Command::new("node")
        .arg("--check")
        .arg(tmp.path())
        .stdout(Stdio::null())
        .stderr(Stdio::piped())
        .output()
    {
        Ok(o) => o,
        Err(_) => return,
    };

    if !output.status.success() {
        let stderr = String::from_utf8_lossy(&output.stderr);
        parse_node_errors(&stderr, block_index, errors);
    }
}

/// Parse Node.js syntax error output into diagnostics.
///
/// Node --check stderr format:
/// ```
/// <file>:N
/// <code>
///      ^
/// SyntaxError: <message>
/// ```
fn parse_node_errors(stderr: &str, block_index: usize, errors: &mut Vec<ValidationDiagnostic>) {
    static NODE_LINE_RE: LazyLock<regex::Regex> =
        LazyLock::new(|| regex::Regex::new(r":(\d+)\b").unwrap());
    static NODE_ERR_RE: LazyLock<regex::Regex> =
        LazyLock::new(|| regex::Regex::new(r"SyntaxError: (.+)").unwrap());

    let first_line = stderr.lines().next().unwrap_or("");
    let line_num = NODE_LINE_RE
        .captures(first_line)
        .and_then(|c| c[1].parse::<usize>().ok());

    let message = if let Some(caps) = NODE_ERR_RE.captures(stderr) {
        caps[1].trim().to_string()
    } else {
        stderr.trim().to_string()
    };

    if !message.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some("node".to_string()),
            message,
            line: line_num,
        });
    }
}

/// Run shellcheck on sanitized shell code (stdin mode) and collect findings as warnings.
///
/// Shellcheck is entirely optional — if not on PATH, this is a no-op.
/// All shellcheck findings become warnings regardless of shellcheck's own severity.
pub(crate) fn check_shellcheck(
    sanitized_code: &str,
    block_index: usize,
    lang: &str,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    if doctor::which_path("shellcheck").is_none() {
        return;
    }

    let shell_dialect = match lang {
        "sh" => "sh",
        "zsh" => "bash", // shellcheck doesn't support zsh natively; bash is closest
        _ => "bash",
    };

    let mut child = match Command::new("shellcheck")
        .args([
            "-s",
            shell_dialect,
            "-f",
            "gcc",
            "-e",
            "SC2034", // variable appears unused — common in skill block snippets
            "-e",
            "SC2086", // double quote to prevent globbing — fires on template placeholders
            "-e",
            "SC2154", // variable referenced but not assigned — common for cross-block vars
            "-",
        ])
        .stdin(Stdio::piped())
        .stdout(Stdio::piped())
        .stderr(Stdio::null())
        .spawn()
    {
        Ok(c) => c,
        Err(_) => return,
    };

    if let Some(mut stdin) = child.stdin.take() {
        let _ = stdin.write_all(sanitized_code.as_bytes());
        // drop signals EOF to shellcheck
    }

    let output = match child.wait_with_output() {
        Ok(o) => o,
        Err(_) => return,
    };

    // gcc format: <file>:<line>:<col>: <level>: <message>
    static SC_RE: LazyLock<regex::Regex> =
        LazyLock::new(|| regex::Regex::new(r"^[^:]+:(\d+):\d+: \w+: (.+)$").unwrap());

    let stdout = String::from_utf8_lossy(&output.stdout);
    for line in stdout.lines() {
        if let Some(caps) = SC_RE.captures(line) {
            let lineno: usize = caps[1].parse().unwrap_or(0);
            let msg = caps[2].trim().to_string();
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(lang.to_string()),
                message: msg,
                line: if lineno > 0 { Some(lineno) } else { None },
            });
        }
    }
}

/// Check that commands referenced in a shell block exist on PATH.
///
/// Uses the same extraction logic as `creft doctor` to parse command names
/// from shell code, then probes PATH for each. Missing commands produce
/// warnings (never errors).
///
/// Placeholders are sanitized before extraction to avoid false positives
/// from template tokens like `{{name|default}}` where `|` would look like
/// a pipe and `default` would be extracted as a command name.
/// Check that creft sub-skill invocations reference skills that exist.
///
/// Parses `creft <name>` patterns from shell code and attempts to resolve
/// each via the store. Unresolved skills produce warnings.
/// Skipped when `ctx` is None (unit test context).
fn check_sub_skill_existence(
    block: &CodeBlock,
    block_index: usize,
    ctx: Option<&AppContext>,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    let ctx = match ctx {
        Some(c) => c,
        None => return, // No AppContext -- skip sub-skill checking
    };

    // Sanitize placeholders before extraction to avoid matching `creft` inside
    // template patterns like `echo "creft {{skill|default}}"`.
    let sanitized = sanitize_placeholders(&block.code);
    let calls = doctor::extract_creft_calls(&sanitized);
    for skill_name in &calls {
        let args: Vec<String> = skill_name.split_whitespace().map(String::from).collect();
        if args.is_empty() {
            continue;
        }
        if store::resolve_command(ctx, &args).is_err() {
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(block.lang.clone()),
                message: format!(
                    "skill '{}' not found (referenced as creft sub-skill)",
                    skill_name
                ),
                line: None,
            });
        }
    }
}

/// Check that declared dependencies can be resolved in their package registry.
///
/// For Python/Node: fires HTTP requests at the configured registry endpoints.
/// For Shell: checks PATH via `which_path` (reuses existing logic).
/// All deps for a single block are checked concurrently via `std::thread::scope`.
/// Skipped entirely when `block.deps` is empty.
fn check_dependency_resolution(
    block: &CodeBlock,
    block_index: usize,
    ctx: Option<&AppContext>,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    if block.deps.is_empty() {
        return;
    }

    match block.lang.as_str() {
        "python" | "python3" => {
            let endpoints = registry_config::resolve_pypi(ctx);
            let labels = endpoints
                .iter()
                .map(|e| e.label.as_str())
                .collect::<Vec<_>>()
                .join(", ");
            let dep_endpoints: Vec<(&String, &[RegistryEndpoint])> = block
                .deps
                .iter()
                .map(|dep| (dep, endpoints.as_slice()))
                .collect();
            check_deps_preresolved(block, block_index, &dep_endpoints, &labels, warnings);
        }
        "node" | "js" | "javascript" => {
            let npm_config = registry_config::resolve_npm(ctx);
            let labels = npm_config
                .defaults
                .iter()
                .map(|e| e.label.as_str())
                .collect::<Vec<_>>()
                .join(", ");
            // Pre-resolve per-dep endpoints before entering thread::scope to
            // avoid lifetime issues with the closure borrowing npm_config.
            let dep_endpoints: Vec<(&String, &[RegistryEndpoint])> = block
                .deps
                .iter()
                .map(|dep| (dep, npm_config.endpoints_for(dep)))
                .collect();
            check_deps_preresolved(block, block_index, &dep_endpoints, &labels, warnings);
        }
        "bash" | "sh" | "zsh" => check_shell_deps(block, block_index, warnings),
        _ => {} // Unknown language -- skip dep checking silently
    }
}

/// Check deps against pre-resolved registry endpoints using HTTP requests.
///
/// For each dep, tries endpoints in order. A dep is "found" if ANY endpoint
/// returns non-404. Only warns if ALL endpoints return 404.
///
/// Uses pre-resolved endpoints to avoid closure lifetime issues with
/// `std::thread::scope`. For Python, all deps share the same endpoints.
/// For npm, scoped packages may use different endpoints via `NpmRegistryConfig::endpoints_for`.
///
/// Non-404 errors (network errors, auth failures) are treated as "can't determine"
/// and never produce warnings. Only a definitive 404 advances to the next endpoint.
fn check_deps_preresolved<'a>(
    block: &'a CodeBlock,
    block_index: usize,
    dep_endpoints: &[(&'a String, &'a [RegistryEndpoint])],
    all_labels: &str,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    let agent = ureq::Agent::new_with_config(
        ureq::config::Config::builder()
            .timeout_global(Some(Duration::from_secs(3)))
            .build(),
    );

    let not_found: Vec<&String> = std::thread::scope(|s| {
        let handles: Vec<_> = dep_endpoints
            .iter()
            .map(|(dep, endpoints)| {
                let agent = agent.clone();
                let dep = *dep;
                let endpoints = *endpoints;
                s.spawn(move || {
                    for endpoint in endpoints {
                        let url = endpoint.url_for(dep);
                        let mut request = match endpoint.method {
                            HttpMethod::Head => agent.head(&url),
                            HttpMethod::Get => agent.get(&url),
                        };
                        if let Some(auth) = &endpoint.auth {
                            request = request.header("Authorization", &auth.header_value());
                        }
                        match request.call() {
                            Ok(_) => return false,
                            Err(ureq::Error::StatusCode(404)) => continue,
                            Err(_) => return false,
                        }
                    }
                    true
                })
            })
            .collect();

        dep_endpoints
            .iter()
            .zip(handles)
            .filter_map(|((dep, _), handle)| {
                if handle.join().unwrap_or(false) {
                    Some(*dep)
                } else {
                    None
                }
            })
            .collect()
    });

    emit_not_found_warnings(not_found, block, block_index, all_labels, warnings);
}

/// Emit warnings for deps not found on any registry.
fn emit_not_found_warnings(
    not_found: Vec<&String>,
    block: &CodeBlock,
    block_index: usize,
    all_labels: &str,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    for dep in not_found {
        warnings.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some(block.lang.clone()),
            message: if all_labels.contains(", ") {
                format!(
                    "dependency '{}' not found on any configured registry ({})",
                    dep, all_labels
                )
            } else {
                format!("dependency '{}' not found on {}", dep, all_labels)
            },
            line: None,
        });
    }
}

/// Check shell block dependencies via PATH lookup (no network).
///
/// Each dep name in `block.deps` is looked up in PATH via `which_path`.
/// Missing deps produce warnings.
fn check_shell_deps(
    block: &CodeBlock,
    block_index: usize,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    for dep in &block.deps {
        if doctor::which_path(dep).is_none() {
            warnings.push(ValidationDiagnostic {
                block_index: Some(block_index),
                lang: Some(block.lang.clone()),
                message: format!("dependency '{}' not found on PATH", dep),
                line: None,
            });
        }
    }
}

/// Warn when a block's lang tag is non-family and the tag-as-binary is not on PATH.
///
/// Skills travel between machines; this is a warning, not an error. The author
/// can install the binary later (or use the skill on a machine that has it). The
/// message states what creft will do at execution time so the author knows the
/// failure mode before saving.
fn check_unknown_lang_binary(
    block: &CodeBlock,
    block_index: usize,
    warnings: &mut Vec<ValidationDiagnostic>,
) {
    if crate::model::is_known_family(&block.lang) {
        return;
    }
    if doctor::which_path(&block.lang).is_some() {
        return;
    }
    warnings.push(ValidationDiagnostic {
        block_index: Some(block_index),
        lang: Some(block.lang.clone()),
        message: format!(
            "interpreter '{}' not found on PATH (block will run via stdin if available at execution time)",
            block.lang
        ),
        line: None,
    });
}

/// Reject `# extension:` values that are not safe identifiers.
///
/// Allowed characters: ASCII letters, digits, `_`, `-`, and inner dots. Inner
/// dots support compound extensions like `tar.gz`.
///
/// `parse_block_directives` strips exactly one leading dot before writing
/// `block.extension`, so `# extension: .mjs` arrives here as `"mjs"` and is
/// accepted. A double leading dot (`# extension: ..mjs`) strips to `".mjs"` —
/// still starts with a dot — and is rejected by the `starts_with('.')` check.
///
/// Trailing dots are rejected because `prepare_block_script` composes the
/// suffix as `format!(".{ext}")`: a trailing dot produces `bar.`, which several
/// tools mishandle. Path separators and shell metacharacters are rejected to
/// prevent temp-file injection.
///
/// An empty `# extension:` value is rejected separately with a clearer message.
fn check_extension_value(
    block: &CodeBlock,
    block_index: usize,
    errors: &mut Vec<ValidationDiagnostic>,
) {
    let ext = match &block.extension {
        Some(s) => s.as_str(),
        None => return,
    };
    if ext.is_empty() {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some(block.lang.clone()),
            message: "'# extension:' value is empty (set a non-empty extension)".to_string(),
            line: None,
        });
        return;
    }
    let safe = ext
        .chars()
        .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.');
    if !safe || ext.starts_with('.') || ext.ends_with('.') {
        errors.push(ValidationDiagnostic {
            block_index: Some(block_index),
            lang: Some(block.lang.clone()),
            message: format!(
                "'# extension:' value '{}' is not a simple identifier (use letters, digits, '_', '-', inner '.')",
                ext
            ),
            line: None,
        });
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use crate::model::{Arg, Flag};
    #[allow(unused_imports)]
    use pretty_assertions::{assert_eq, assert_ne};
    use rstest::rstest;

    fn make_def(args: Vec<&str>, flags: Vec<&str>) -> CommandDef {
        CommandDef {
            name: "test".into(),
            description: "test command".into(),
            args: args
                .into_iter()
                .map(|n| Arg {
                    name: n.into(),
                    description: String::new(),
                    default: None,
                    required: false,
                    validation: None,
                })
                .collect(),
            flags: flags
                .into_iter()
                .map(|n| Flag {
                    name: n.into(),
                    short: None,
                    description: String::new(),
                    r#type: "string".into(),
                    default: None,
                    validation: None,
                })
                .collect(),
            env: vec![],
            tags: vec![],
            supports: vec![],
        }
    }

    fn make_block(lang: &str, code: &str) -> CodeBlock {
        CodeBlock {
            lang: lang.into(),
            code: code.into(),
            deps: vec![],
            extension: None,
            flags: None,
            llm_config: None,
            llm_parse_error: None,
        }
    }

    #[test]
    fn test_placeholder_all_declared() {
        let def = make_def(vec!["repo", "number"], vec![]);
        let block = make_block("bash", "gh api repos/{{repo}}/issues/{{number}}");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty(), "expected no errors");
        // Two env-var hints are emitted (one per declared arg in the shell block);
        // no undeclared-placeholder warnings.
        assert_eq!(result.warnings.len(), 2);
        assert!(
            result
                .warnings
                .iter()
                .all(|w| w.message.contains("environment variable")),
            "all warnings should be env-var hints, got: {:?}",
            result.warnings
        );
    }

    #[test]
    fn test_placeholder_undeclared_no_default() {
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo {{foo}}");
        let result = validate_skill(&def, &[block], "", None);
        assert_eq!(result.warnings.len(), 1);
        assert!(
            result.warnings[0]
                .message
                .contains("not declared in args or flags")
        );
        assert!(!result.warnings[0].message.contains("has default"));
        assert!(result.errors.is_empty());
    }

    #[test]
    fn test_placeholder_undeclared_with_default() {
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo {{foo|bar}}");
        let result = validate_skill(&def, &[block], "", None);
        assert_eq!(result.warnings.len(), 1);
        assert!(result.warnings[0].message.contains("has default"));
        assert!(result.errors.is_empty());
    }

    #[test]
    fn test_placeholder_prev_in_first_block() {
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo {{prev}}");
        let result = validate_skill(&def, &[block], "", None);
        assert_eq!(result.warnings.len(), 1);
        assert!(
            result.warnings[0]
                .message
                .contains("first block has no preceding block output")
        );
    }

    #[test]
    fn test_placeholder_prev_in_later_block() {
        let def = make_def(vec![], vec![]);
        let block0 = make_block("bash", "echo hello");
        let block1 = make_block("bash", "echo {{prev}}");
        let result = validate_skill(&def, &[block0, block1], "", None);
        assert!(
            result.is_clean(),
            "expected no warnings for {{prev}} in block 1"
        );
    }

    #[test]
    fn test_placeholder_flags_recognized() {
        let def = make_def(vec![], vec!["format", "style"]);
        let block = make_block("bash", "echo {{format}} {{style}}");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty(), "flags should be in declared set");
        // Two env-var hints are emitted (one per declared flag in the shell block);
        // no undeclared-placeholder warnings.
        assert_eq!(result.warnings.len(), 2);
        assert!(
            result
                .warnings
                .iter()
                .all(|w| w.message.contains("environment variable")),
            "all warnings should be env-var hints"
        );
    }

    #[test]
    fn test_no_placeholders() {
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo hello world");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.is_clean());
    }

    #[test]
    fn test_empty_blocks() {
        let def = make_def(vec![], vec![]);
        let result = validate_skill(&def, &[], "", None);
        assert!(result.is_clean());
    }

    #[test]
    fn test_prev_declared_as_arg_still_warns_in_block0() {
        // Even if someone names an arg "prev", {{prev}} in block 0 still warns
        // because the semantics of {{prev}} as "previous block output" don't
        // apply when there's no previous block.
        let def = make_def(vec!["prev"], vec![]);
        let block = make_block("bash", "echo {{prev}}");
        let result = validate_skill(&def, &[block], "", None);
        // Two warnings: {{prev}}-in-block-0 warning and the CREFT_ARG_PREV env-var hint.
        assert_eq!(result.warnings.len(), 2);
        assert!(
            result.warnings.iter().any(|w| w
                .message
                .contains("first block has no preceding block output")),
            "expected the {{prev}}-in-block-0 warning"
        );
        assert!(
            result
                .warnings
                .iter()
                .any(|w| w.message.contains("CREFT_ARG_PREV")),
            "expected the CREFT_ARG_PREV env-var hint"
        );
    }

    #[test]
    fn test_display_format_with_block_lang_line() {
        let d = ValidationDiagnostic {
            block_index: Some(0),
            lang: Some("bash".into()),
            message: "some error".into(),
            line: Some(3),
        };
        assert_eq!(format!("{}", d), "block 1 (bash), line 3: some error");
    }

    #[test]
    fn test_display_format_with_block_lang_no_line() {
        let d = ValidationDiagnostic {
            block_index: Some(1),
            lang: Some("python".into()),
            message: "undeclared placeholder".into(),
            line: None,
        };
        assert_eq!(format!("{}", d), "block 2 (python): undeclared placeholder");
    }

    #[test]
    fn test_display_format_no_block() {
        let d = ValidationDiagnostic {
            block_index: None,
            lang: None,
            message: "skill-level issue".into(),
            line: None,
        };
        assert_eq!(format!("{}", d), "skill-level issue");
    }

    #[test]
    fn test_multiple_undeclared_placeholders() {
        let def = make_def(vec!["declared"], vec![]);
        let block = make_block("bash", "echo {{declared}} {{missing1}} {{missing2}}");
        let result = validate_skill(&def, &[block], "", None);
        // Three warnings: undeclared for missing1, undeclared for missing2, and
        // the CREFT_ARG_DECLARED env-var hint for the declared arg.
        assert_eq!(result.warnings.len(), 3);
        let undeclared_count = result
            .warnings
            .iter()
            .filter(|w| w.message.contains("not declared in args or flags"))
            .count();
        assert_eq!(
            undeclared_count, 2,
            "missing1 and missing2 each produce a warning"
        );
    }

    // ── sanitize_placeholders unit tests ─────────────────────────────────────

    #[test]
    fn test_sanitize_placeholders_shell() {
        let result = sanitize_placeholders("echo {{name}}");
        assert_eq!(result, "echo __CREFT_PH__");
    }

    #[test]
    fn test_sanitize_placeholders_with_default() {
        let result = sanitize_placeholders("echo {{name|foo}}");
        assert_eq!(result, "echo __CREFT_PH__");
    }

    #[test]
    fn test_sanitize_placeholders_multiple() {
        let result = sanitize_placeholders("echo {{a}} {{b|default}} {{c}}");
        assert_eq!(result, "echo __CREFT_PH__ __CREFT_PH__ __CREFT_PH__");
    }

    #[test]
    fn test_sanitize_preserves_non_placeholder_braces() {
        // ${VAR} is shell variable syntax, not a creft placeholder — must be untouched.
        let result = sanitize_placeholders("echo ${HOME} {{name}}");
        assert_eq!(result, "echo ${HOME} __CREFT_PH__");
    }

    #[test]
    fn test_sanitize_placeholder_inside_string() {
        // Placeholder embedded inside a double-quoted string.
        let result = sanitize_placeholders(r#"echo "hello {{name}}""#);
        assert_eq!(result, r#"echo "hello __CREFT_PH__""#);
    }

    // ── syntax validation unit tests ─────────────────────────────────────────

    #[test]
    fn test_valid_bash_no_errors() {
        // Only runs if bash is available; otherwise the check is skipped silently.
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo hello\nif true; then\n  echo ok\nfi\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "valid bash should produce no errors"
        );
    }

    #[test]
    fn test_invalid_bash_produces_error() {
        // Only meaningful if bash is on PATH.
        if crate::doctor::which_path("bash").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        // Missing `fi` — bash -n will catch this.
        let block = make_block("bash", "if true; then\n  echo broken\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.errors.is_empty(),
            "invalid bash should produce at least one error"
        );
    }

    #[test]
    fn test_invalid_python_produces_error() {
        if crate::doctor::which_path("python3").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        // Invalid indentation — ast.parse will reject this.
        let block = make_block("python", "def foo():\nprint('bad indent')\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.errors.is_empty(),
            "invalid python should produce at least one error"
        );
    }

    #[test]
    fn test_valid_python_no_errors() {
        if crate::doctor::which_path("python3").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("python", "def foo():\n    print('hello')\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "valid python should produce no errors"
        );
    }

    #[test]
    fn test_bash_placeholder_no_false_positive() {
        // A bash block with a placeholder should not cause a syntax error after
        // sentinel substitution — {{name}} becomes __CREFT_PH__ which is valid.
        if crate::doctor::which_path("bash").is_none() {
            return;
        }
        let def = make_def(vec!["name"], vec![]);
        let block = make_block("bash", "echo {{name}}\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "placeholder in bash should not cause syntax error after sanitization; errors: {:?}",
            result.errors
        );
    }

    #[test]
    fn test_unknown_language_no_errors() {
        // Unknown languages are silently skipped — no errors, no warnings from syntax check.
        let def = make_def(vec![], vec![]);
        let block = make_block("cobol", "MOVE 1 TO X");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty());
    }

    #[test]
    fn test_typescript_not_validated() {
        // TypeScript is excluded from syntax validation.
        let def = make_def(vec![], vec![]);
        // Deliberately broken TS syntax — should produce no error since ts is not checked.
        let block = make_block("typescript", "const x: = 1;");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "typescript should not be validated (too heavy)"
        );
    }

    // ── Arg and flag env var diagnostics ────────────────────────────────────

    /// Returns warnings whose message contains "environment variable" — the
    /// marker for the CREFT_ARG_ hint emitted by check_placeholders.
    fn env_var_hints(result: &ValidationResult) -> Vec<&ValidationDiagnostic> {
        result
            .warnings
            .iter()
            .filter(|w| w.message.contains("environment variable"))
            .collect()
    }

    #[test]
    fn hyphenated_flag_in_bash_block_emits_env_var_warning() {
        let def = make_def(vec![], vec!["always-confirm"]);
        let block = make_block("bash", "echo hello\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "hyphenated flag in bash block should not produce errors"
        );
        let hints = env_var_hints(&result);
        assert_eq!(hints.len(), 1, "expected exactly one env var hint warning");
        assert!(
            hints[0].message.contains("always-confirm"),
            "warning should name the flag"
        );
        assert!(
            hints[0].message.contains("CREFT_ARG_ALWAYS_CONFIRM"),
            "warning should show the prefixed env var name"
        );
    }

    #[test]
    fn hyphenated_flag_in_non_shell_block_emits_no_diagnostic() {
        // Python authors use os.environ directly; the env-var hint is only
        // useful for shell blocks.
        let def = make_def(vec![], vec!["always-confirm"]);
        let block = make_block("python", "print('hello')\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "hyphenated flag in python block should not produce errors"
        );
        assert!(
            env_var_hints(&result).is_empty(),
            "hyphenated flag in python block should not produce env var hints"
        );
    }

    #[test]
    fn non_hyphenated_flag_in_bash_block_emits_env_var_hint() {
        let def = make_def(vec![], vec!["format"]);
        let block = make_block("bash", "echo hello\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty());
        let hints = env_var_hints(&result);
        assert_eq!(
            hints.len(),
            1,
            "every flag in a bash block should emit an env var hint"
        );
        assert!(
            hints[0].message.contains("format"),
            "hint should name the flag"
        );
        assert!(
            hints[0].message.contains("CREFT_ARG_FORMAT"),
            "hint should show the prefixed env var name"
        );
    }

    #[test]
    fn multiple_hyphenated_flags_each_emit_a_warning() {
        // Uses non-reserved hyphenated flag names to test env var hint emission.
        let def = make_def(vec![], vec!["output-format", "output-path"]);
        let block = make_block("bash", "echo hello\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty());
        let hints = env_var_hints(&result);
        assert_eq!(hints.len(), 2, "each flag should produce one env var hint");
        let messages: Vec<&str> = hints.iter().map(|w| w.message.as_str()).collect();
        assert!(
            messages
                .iter()
                .any(|m| m.contains("output-format") && m.contains("CREFT_ARG_OUTPUT_FORMAT"))
        );
        assert!(
            messages
                .iter()
                .any(|m| m.contains("output-path") && m.contains("CREFT_ARG_OUTPUT_PATH"))
        );
    }

    #[test]
    fn hyphenated_flag_with_template_usage_passes_bash_syntax() {
        // {{always-confirm}} is valid after sanitize_placeholders replaces it with
        // __CREFT_PH__. The warning about the env var name should still be emitted.
        if crate::doctor::which_path("bash").is_none() {
            return;
        }
        let def = make_def(vec![], vec!["always-confirm"]);
        let block = make_block("bash", "echo {{always-confirm}}\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "{{always-confirm}} in bash should not produce a syntax error; errors: {:?}",
            result.errors
        );
        assert!(
            env_var_hints(&result)
                .iter()
                .any(|w| w.message.contains("CREFT_ARG_ALWAYS_CONFIRM")),
            "expected env var hint warning"
        );
    }

    // ── Display: block with no lang ──────────────────────────────────────────

    #[test]
    fn test_display_format_block_no_lang() {
        // (Some(idx), None, _) arm
        let d = ValidationDiagnostic {
            block_index: Some(2),
            lang: None,
            message: "some message".into(),
            line: None,
        };
        assert_eq!(format!("{}", d), "block 3: some message");
    }

    #[test]
    fn test_display_format_block_no_lang_with_line() {
        // (Some(idx), None, Some(line)) — line is ignored per Display impl
        let d = ValidationDiagnostic {
            block_index: Some(0),
            lang: None,
            message: "msg".into(),
            line: Some(5),
        };
        // The `(Some(idx), None, _)` arm doesn't include the line
        assert_eq!(format!("{}", d), "block 1: msg");
    }

    // ── parse_shell_errors: unparsed raw stderr ──────────────────────────────

    #[test]
    fn test_parse_shell_errors_raw_stderr_fallback() {
        // When stderr doesn't match the ": line N: msg" pattern, raw text is emitted.
        let mut errors = Vec::new();
        parse_shell_errors("some unparsed error text", 0, "bash", &mut errors);
        assert_eq!(errors.len(), 1);
        assert!(errors[0].message.contains("some unparsed error text"));
        assert!(errors[0].line.is_none());
    }

    #[test]
    fn test_parse_shell_errors_empty_stderr_no_errors() {
        let mut errors = Vec::new();
        parse_shell_errors("", 0, "bash", &mut errors);
        assert!(errors.is_empty(), "empty stderr should produce no errors");
    }

    #[test]
    fn test_parse_shell_errors_whitespace_only_no_errors() {
        let mut errors = Vec::new();
        parse_shell_errors("   \n   ", 0, "bash", &mut errors);
        assert!(
            errors.is_empty(),
            "whitespace-only stderr should produce no errors"
        );
    }

    // ── sh syntax checking ────────────────────────────────────────────────────

    #[test]
    fn test_valid_sh_no_errors() {
        if crate::doctor::which_path("sh").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("sh", "echo hello\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "valid sh should produce no errors"
        );
    }

    #[test]
    fn test_invalid_sh_produces_error() {
        if crate::doctor::which_path("sh").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("sh", "if true; then\n  echo broken\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.errors.is_empty(),
            "invalid sh should produce at least one error"
        );
    }

    // ── zsh syntax checking ───────────────────────────────────────────────────

    #[test]
    fn test_valid_zsh_no_errors() {
        if crate::doctor::which_path("zsh").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("zsh", "echo hello\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "valid zsh should produce no errors"
        );
    }

    // ── parse_python_errors: raw stderr fallback ──────────────────────────────

    #[test]
    fn test_parse_python_errors_raw_fallback() {
        let mut errors = Vec::new();
        // Text without a SyntaxError/IndentationError/TabError prefix
        parse_python_errors("some random error text\nmore text", 0, &mut errors);
        assert_eq!(errors.len(), 1);
        // Should use raw stripped text
        assert!(errors[0].message.contains("some random error text"));
    }

    #[test]
    fn test_parse_python_errors_with_syntax_error() {
        let mut errors = Vec::new();
        let stderr = "  File \"<file>\", line 3\nSyntaxError: invalid syntax";
        parse_python_errors(stderr, 0, &mut errors);
        assert_eq!(errors.len(), 1);
        assert_eq!(errors[0].line, Some(3));
        assert!(errors[0].message.contains("invalid syntax"));
    }

    #[test]
    fn test_parse_python_errors_empty_message_ignored() {
        let mut errors = Vec::new();
        parse_python_errors("", 0, &mut errors);
        assert!(errors.is_empty());
    }

    // ── parse_node_errors ─────────────────────────────────────────────────────

    #[test]
    fn test_parse_node_errors_with_syntax_error() {
        let mut errors = Vec::new();
        let stderr = "/tmp/foo.js:5\n  const x = ;\n         ^\nSyntaxError: Unexpected token ';'";
        parse_node_errors(stderr, 0, &mut errors);
        assert_eq!(errors.len(), 1);
        assert_eq!(errors[0].line, Some(5));
        assert!(errors[0].message.contains("Unexpected token"));
    }

    #[test]
    fn test_parse_node_errors_raw_fallback() {
        let mut errors = Vec::new();
        let stderr = "some node error without SyntaxError prefix";
        parse_node_errors(stderr, 0, &mut errors);
        assert_eq!(errors.len(), 1);
        assert!(errors[0].message.contains("some node error"));
    }

    #[test]
    fn test_parse_node_errors_empty_ignored() {
        let mut errors = Vec::new();
        parse_node_errors("", 0, &mut errors);
        assert!(errors.is_empty());
    }

    // ── node syntax checking ──────────────────────────────────────────────────

    #[test]
    fn test_valid_node_no_errors() {
        if crate::doctor::which_path("node").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("node", "console.log('hello');\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "valid node should produce no errors"
        );
    }

    #[test]
    fn test_invalid_node_produces_error() {
        if crate::doctor::which_path("node").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("node", "const x = ;\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.errors.is_empty(),
            "invalid node should produce at least one error"
        );
    }

    // ── javascript/js language aliases ────────────────────────────────────────

    #[test]
    fn test_valid_javascript_alias_no_errors() {
        if crate::doctor::which_path("node").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("javascript", "var x = 1;\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(result.errors.is_empty());
    }

    // ── description length warning ────────────────────────────────────────────

    fn make_def_with_desc(description: &str) -> CommandDef {
        CommandDef {
            name: "test".into(),
            description: description.into(),
            args: vec![],
            flags: vec![],
            env: vec![],
            tags: vec![],
            supports: vec![],
        }
    }

    #[test]
    fn test_description_length_warn_over_80() {
        // 81-character description should produce exactly one warning about length.
        let desc = "a".repeat(DESCRIPTION_WARN_LEN + 1);
        let def = make_def_with_desc(&desc);
        let result = validate_skill(&def, &[], "", None);
        assert_eq!(
            result.warnings.len(),
            1,
            "expected one warning for description over {DESCRIPTION_WARN_LEN} chars"
        );
        assert!(
            result.warnings[0].message.contains("description is long"),
            "warning message should mention description is long; got: {:?}",
            result.warnings[0].message
        );
        assert!(
            result.warnings[0]
                .message
                .contains(&(DESCRIPTION_WARN_LEN + 1).to_string()),
            "warning should include the actual char count; got: {:?}",
            result.warnings[0].message
        );
        assert!(
            result.errors.is_empty(),
            "description length is a warning, not an error"
        );
    }

    #[test]
    fn test_description_length_no_warn_at_limit() {
        // Exactly 80 characters: no warning.
        let desc = "a".repeat(DESCRIPTION_WARN_LEN);
        let def = make_def_with_desc(&desc);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.warnings.is_empty(),
            "no warning expected at exactly {DESCRIPTION_WARN_LEN} chars"
        );
    }

    #[test]
    fn test_description_length_no_warn_short() {
        let def = make_def_with_desc("Short description");
        let result = validate_skill(&def, &[], "", None);
        // May have no warnings (no blocks, no placeholders).
        let has_desc_warn = result
            .warnings
            .iter()
            .any(|w| w.message.contains("description is long"));
        assert!(
            !has_desc_warn,
            "short description should not warn about length"
        );
    }

    #[test]
    fn test_description_length_diagnostic_has_no_block_index() {
        // The diagnostic for description length must be skill-level (no block_index).
        let desc = "a".repeat(DESCRIPTION_WARN_LEN + 1);
        let def = make_def_with_desc(&desc);
        let result = validate_skill(&def, &[], "", None);
        let desc_warn = result
            .warnings
            .iter()
            .find(|w| w.message.contains("description is long"))
            .expect("expected a description-length warning");
        assert!(
            desc_warn.block_index.is_none(),
            "description length warning must not have a block_index"
        );
        assert!(
            desc_warn.lang.is_none(),
            "description length warning must not have a lang"
        );
    }

    #[test]
    fn test_parse_shell_errors_lineno_zero_becomes_none() {
        // Line numbers of 0 are meaningless; they must be normalized to None.
        let mut errors = Vec::new();
        parse_shell_errors(
            "/tmp/foo.sh: line 0: syntax error near unexpected token",
            0,
            "bash",
            &mut errors,
        );
        assert_eq!(errors.len(), 1);
        assert_eq!(
            errors[0].line, None,
            "line 0 should become None, not Some(0)"
        );
    }

    #[test]
    fn test_parse_shell_errors_lineno_one_becomes_some() {
        let mut errors = Vec::new();
        parse_shell_errors(
            "/tmp/foo.sh: line 1: syntax error near unexpected token",
            0,
            "bash",
            &mut errors,
        );
        assert_eq!(errors.len(), 1);
        assert_eq!(errors[0].line, Some(1), "line 1 should become Some(1)");
    }

    #[test]
    fn test_shellcheck_skipped_when_syntax_fails() {
        // shellcheck must not run when syntax fails: the gate is
        // `syntax_ok && is_shell_lang`, so a failed syntax check keeps
        // shellcheck out even for shell blocks.
        if crate::doctor::which_path("bash").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        // Missing `fi` — bash -n will add errors, so syntax_ok will be false.
        let block = make_block("bash", "if true; then\n  echo broken\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.errors.is_empty(),
            "syntax failure should produce errors"
        );
        let _ = result.warnings;
    }

    #[test]
    fn test_shellcheck_skipped_for_non_shell_lang() {
        // shellcheck only runs for shell blocks; python blocks must never trigger it.
        if crate::doctor::which_path("python3").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("python", "x = 1\n");
        let result = validate_skill(&def, &[block], "", None);
        // No shellcheck warnings should be present for a python block.
        let shellcheck_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.lang.as_deref() == Some("python"))
            .collect();
        assert!(
            shellcheck_warnings.is_empty(),
            "python block must not produce shellcheck warnings: {:?}",
            shellcheck_warnings
        );
    }

    #[test]
    fn test_parse_shell_errors_structured_plus_fallback_exclusive() {
        // When structured parsing matches at least one line, the raw-stderr
        // fallback must not fire — only one diagnostic for a single error.
        let mut errors = Vec::new();
        let stderr =
            "/tmp/foo.sh: line 5: syntax error near unexpected token\nsome other junk line";
        parse_shell_errors(stderr, 0, "bash", &mut errors);
        assert_eq!(
            errors.len(),
            1,
            "structured match must suppress fallback; got {:?}",
            errors
        );
        assert_eq!(errors[0].line, Some(5));
    }

    #[test]
    fn test_is_clean_false_with_warnings_only() {
        // is_clean requires both errors AND warnings to be empty.
        let result = ValidationResult {
            errors: vec![],
            warnings: vec![ValidationDiagnostic {
                block_index: Some(0),
                lang: Some("bash".into()),
                message: "a warning".into(),
                line: None,
            }],
        };
        assert!(!result.is_clean(), "warnings-only result must not be clean");
    }

    #[test]
    fn test_is_clean_false_with_errors_only() {
        let result = ValidationResult {
            errors: vec![ValidationDiagnostic {
                block_index: Some(0),
                lang: Some("bash".into()),
                message: "an error".into(),
                line: None,
            }],
            warnings: vec![],
        };
        assert!(!result.is_clean(), "errors-only result must not be clean");
    }

    #[test]
    fn test_is_clean_true_when_both_empty() {
        let result = ValidationResult {
            errors: vec![],
            warnings: vec![],
        };
        assert!(result.is_clean(), "empty result must be clean");
    }

    #[test]
    fn test_which_path_returns_none_for_nonexistent_tool() {
        assert!(
            crate::doctor::which_path("__creft_nonexistent_tool_xyzzy__").is_none(),
            "nonexistent tool must not appear to be on PATH"
        );
    }

    #[test]
    fn test_shellcheck_produces_warnings_for_known_issue() {
        // Verify shellcheck integration end-to-end: a pattern that is NOT in our
        // exclusion list (SC2034/SC2086/SC2154) must still produce a warning.
        // SC2162: read without -r will mangle backslashes. Not excluded.
        if crate::doctor::which_path("shellcheck").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "read answer\necho \"$answer\"\n");
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            !result.warnings.is_empty(),
            "shellcheck must produce at least one warning for 'read' without -r (SC2162); got none"
        );
    }

    // ── known behavioral equivalences ────────────────────────────────────────────
    //
    // 1. Shell dialect arms: replacing "sh" => "sh" or "zsh" => "zsh" with a
    //    fallthrough to "bash" produces identical syntax-check results on all
    //    platforms, so these branches aren't distinguished by tests.
    //
    // 2. Shellcheck lineno guard: shellcheck never emits line 0, so `> 0`
    //    and `>= 0` are behaviorally identical for real shellcheck output.

    // ── check_sub_skill_existence tests ──────────────────────────────────────────

    #[test]
    fn test_sub_skill_skipped_when_no_ctx() {
        // With ctx=None, sub-skill checking is skipped entirely.
        let def = make_def(vec![], vec![]);
        // creft nonexistent-xyzzy-skill would fail resolution if ctx were present.
        let block = make_block("bash", "creft nonexistent-xyzzy-skill");
        let result = validate_skill(&def, &[block], "", None);
        let sub_skill_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| {
                w.message
                    .contains("not found (referenced as creft sub-skill)")
            })
            .collect();
        assert!(
            sub_skill_warnings.is_empty(),
            "sub-skill checking must be skipped when ctx is None; got: {:?}",
            sub_skill_warnings
        );
    }

    #[test]
    fn test_sub_skill_skipped_for_python_block() {
        // Sub-skill checking only applies to shell blocks.
        let def = make_def(vec![], vec![]);
        // Even if ctx were provided, python blocks are skipped.
        let block = make_block("python", "# creft nonexistent-xyzzy-skill");
        let result = validate_skill(&def, &[block], "", None);
        let sub_skill_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| {
                w.message
                    .contains("not found (referenced as creft sub-skill)")
            })
            .collect();
        assert!(
            sub_skill_warnings.is_empty(),
            "sub-skill checking must be skipped for python blocks; got: {:?}",
            sub_skill_warnings
        );
    }

    // ── dependency resolution unit tests ─────────────────────────────────────

    fn make_block_with_deps(lang: &str, code: &str, deps: Vec<&str>) -> CodeBlock {
        CodeBlock {
            lang: lang.into(),
            code: code.into(),
            deps: deps.into_iter().map(String::from).collect(),
            extension: None,
            flags: None,
            llm_config: None,
            llm_parse_error: None,
        }
    }

    #[test]
    fn test_dep_resolution_shell_missing() {
        // A shell block with a declared dep that doesn't exist on PATH produces a
        // warning containing "not found on PATH".
        let def = make_def(vec![], vec![]);
        let block = make_block_with_deps("bash", "echo hello", vec!["__nonexistent_dep_xyzzy__"]);
        let result = validate_skill(&def, &[block], "", None);
        let dep_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.contains("not found on PATH"))
            .collect();
        assert_eq!(
            dep_warnings.len(),
            1,
            "expected one dep warning; got: {:?}",
            result.warnings
        );
        assert!(
            dep_warnings[0]
                .message
                .contains("__nonexistent_dep_xyzzy__")
        );
    }

    #[test]
    fn test_dep_resolution_shell_present() {
        // A shell block with `ls` as a dep produces no dep warning — ls is
        // universally available as a binary (not just a shell builtin).
        if crate::doctor::which_path("ls").is_none() {
            return;
        }
        let def = make_def(vec![], vec![]);
        let block = make_block_with_deps("bash", "ls -la", vec!["ls"]);
        let result = validate_skill(&def, &[block], "", None);
        let dep_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.starts_with("dependency"))
            .collect();
        assert!(
            dep_warnings.is_empty(),
            "ls should be found on PATH; got: {:?}",
            dep_warnings
        );
    }

    #[test]
    fn test_dep_resolution_empty_deps_no_check() {
        // A block with no deps declared produces no dep resolution warnings.
        let def = make_def(vec![], vec![]);
        let block = make_block("python", "import sys");
        let result = validate_skill(&def, &[block], "", None);
        let dep_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.starts_with("dependency"))
            .collect();
        assert!(
            dep_warnings.is_empty(),
            "empty deps should produce no dep warnings; got: {:?}",
            dep_warnings
        );
    }

    #[test]
    fn test_dep_resolution_skipped_for_unknown_lang() {
        // A block with an unknown language and declared deps produces no warnings.
        let def = make_def(vec![], vec![]);
        let block = make_block_with_deps("cobol", "MOVE 1 TO X", vec!["some-cobol-lib"]);
        let result = validate_skill(&def, &[block], "", None);
        let dep_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.starts_with("dependency"))
            .collect();
        assert!(
            dep_warnings.is_empty(),
            "unknown lang should skip dep checking; got: {:?}",
            dep_warnings
        );
    }

    #[test]
    fn test_endpoint_url_for_pypi() {
        use crate::registry_config::{HttpMethod, RegistryEndpoint};
        let ep = RegistryEndpoint {
            url_template: "https://pypi.org/pypi/{}/json".to_string(),
            auth: None,
            label: "PyPI".to_string(),
            method: HttpMethod::Head,
        };
        assert_eq!(
            ep.url_for("requests"),
            "https://pypi.org/pypi/requests/json"
        );
    }

    #[test]
    fn test_endpoint_url_for_npm() {
        use crate::registry_config::{HttpMethod, RegistryEndpoint};
        let ep = RegistryEndpoint {
            url_template: "https://registry.npmjs.org/{}".to_string(),
            auth: None,
            label: "npm".to_string(),
            method: HttpMethod::Head,
        };
        assert_eq!(ep.url_for("lodash"), "https://registry.npmjs.org/lodash");
    }

    // ── llm block validation tests ───────────────────────────────────────────────

    fn make_llm_block(code: &str, config: Option<crate::model::LlmConfig>) -> CodeBlock {
        CodeBlock {
            lang: "llm".into(),
            code: code.into(),
            deps: vec![],
            extension: None,
            flags: None,
            llm_config: config,
            llm_parse_error: None,
        }
    }

    fn default_llm_config() -> crate::model::LlmConfig {
        crate::model::LlmConfig::default()
    }

    #[test]
    fn test_llm_block_empty_prompt_error() {
        let def = make_def(vec![], vec![]);
        let block = make_llm_block("   ", Some(default_llm_config()));
        let result = validate_skill(&def, &[block], "", None);
        assert_eq!(result.errors.len(), 1);
        assert!(
            result.errors[0].message.contains("no prompt text"),
            "expected 'no prompt text' error, got: {}",
            result.errors[0].message
        );
    }

    #[test]
    fn test_llm_block_yaml_parse_error_reported() {
        let def = make_def(vec![], vec![]);
        let mut block = make_llm_block("some prompt", None);
        block.llm_parse_error = Some("mapping values are not allowed here".to_string());
        let result = validate_skill(&def, &[block], "", None);
        assert_eq!(result.errors.len(), 1);
        assert!(
            result.errors[0].message.contains("invalid YAML header"),
            "expected YAML parse error message, got: {}",
            result.errors[0].message
        );
        assert!(
            result.errors[0]
                .message
                .contains("mapping values are not allowed here"),
            "expected original parse error in message"
        );
    }

    #[test]
    fn test_llm_block_valid_prompt_clean() {
        // cat is universally available — this block should have no errors.
        let def = make_def(vec![], vec![]);
        let mut config = default_llm_config();
        config.provider = "cat".to_string();
        let block = make_llm_block("hello from llm block", Some(config));
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "expected no errors, got: {:?}",
            result.errors
        );
    }

    #[test]
    fn test_llm_block_provider_warning_when_not_on_path() {
        let def = make_def(vec![], vec![]);
        let mut config = default_llm_config();
        config.provider = "nonexistent-llm-provider-xyz".to_string();
        let block = make_llm_block("my prompt", Some(config));
        let result = validate_skill(&def, &[block], "", None);
        assert!(
            result.errors.is_empty(),
            "missing provider should warn, not error"
        );
        assert!(
            result
                .warnings
                .iter()
                .any(|w| w.message.contains("nonexistent-llm-provider-xyz")),
            "expected provider not found warning, got: {:?}",
            result.warnings
        );
    }

    #[test]
    fn test_llm_block_skips_syntax_check() {
        // An llm block with content that looks like bad shell — should not trigger syntax errors.
        let def = make_def(vec![], vec![]);
        let mut config = default_llm_config();
        config.provider = "cat".to_string();
        let block = make_llm_block("if then else fi {{ broken shell syntax }", Some(config));
        let result = validate_skill(&def, &[block], "", None);
        // No syntax errors from the llm block content.
        assert!(
            result.errors.is_empty(),
            "llm blocks should not be syntax-checked, got errors: {:?}",
            result.errors
        );
    }

    #[test]
    fn test_llm_block_placeholder_check_works() {
        // An undeclared placeholder in the prompt body should warn.
        let def = make_def(vec![], vec![]);
        let mut config = default_llm_config();
        config.provider = "cat".to_string();
        let block = make_llm_block("Summarize {{some_arg}}", Some(config));
        let result = validate_skill(&def, &[block], "", None);
        let has_placeholder_warning = result
            .warnings
            .iter()
            .any(|w| w.message.contains("some_arg"));
        assert!(
            has_placeholder_warning,
            "expected placeholder warning for undeclared {{{{some_arg}}}}, got: {:?}",
            result.warnings
        );
    }

    #[test]
    fn test_llm_block_prev_placeholder_in_second_block_no_warning() {
        // {{prev}} in the second block (not block 0) should not produce a warning.
        let def = make_def(vec![], vec![]);
        let bash_block = make_block("bash", "echo hello");
        let mut config = default_llm_config();
        config.provider = "cat".to_string();
        let llm_block = make_llm_block("Review this: {{prev}}", Some(config));
        let result = validate_skill(&def, &[bash_block, llm_block], "", None);
        let has_prev_warning = result.warnings.iter().any(|w| w.message.contains("prev"));
        assert!(
            !has_prev_warning,
            "{{{{prev}}}} in block 2 should not produce a warning, got: {:?}",
            result.warnings
        );
    }

    // ── frontmatter name validation: Stage 1 reserved names ──────────────────

    fn make_flag(name: &str, short: Option<&str>) -> Flag {
        Flag {
            name: name.into(),
            short: short.map(Into::into),
            description: String::new(),
            r#type: "string".into(),
            default: None,
            validation: None,
        }
    }

    fn make_def_with_flags(flags: Vec<Flag>) -> CommandDef {
        CommandDef {
            name: "test".into(),
            description: "test command".into(),
            args: vec![],
            flags,
            env: vec![],
            tags: vec![],
            supports: vec![],
        }
    }

    fn make_def_with_flags_and_args(flags: Vec<Flag>, args: Vec<&str>) -> CommandDef {
        CommandDef {
            name: "test".into(),
            description: "test command".into(),
            args: args
                .into_iter()
                .map(|n| Arg {
                    name: n.into(),
                    description: String::new(),
                    default: None,
                    required: false,
                    validation: None,
                })
                .collect(),
            flags,
            env: vec![],
            tags: vec![],
            supports: vec![],
        }
    }

    #[rstest]
    #[case::help("help")]
    #[case::dry_run("dry-run")]
    #[case::verbose("verbose")]
    #[case::docs("docs")]
    #[case::version("version")]
    fn reserved_flag_name_produces_error(#[case] name: &str) {
        let def = make_def_with_flags(vec![make_flag(name, None)]);
        let result = validate_skill(&def, &[], "", None);
        let msg = result
            .errors
            .iter()
            .find(|e| e.message.contains("reserved"))
            .map(|e| e.message.as_str())
            .unwrap_or("(no reserved error found)");
        assert!(
            result.errors.iter().any(|e| e.message.contains("reserved")),
            "expected 'reserved' error for flag '{name}', got: {msg}"
        );
    }

    #[rstest]
    #[case::h("h")]
    #[case::v("v")]
    #[case::cap_v("V")]
    fn reserved_short_flag_produces_error(#[case] short: &str) {
        let def = make_def_with_flags(vec![make_flag("output", Some(short))]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("reserved")),
            "expected 'reserved' error for short flag '{short}', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn duplicate_flag_long_names_produce_error() {
        let def = make_def_with_flags(vec![make_flag("output", None), make_flag("output", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("duplicate")),
            "expected 'duplicate' error for repeated flag name, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn duplicate_flag_short_forms_produce_error() {
        let def = make_def_with_flags(vec![
            make_flag("output", Some("o")),
            make_flag("other", Some("o")),
        ]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("duplicate")),
            "expected 'duplicate' error for repeated short form, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn duplicate_arg_names_produce_error() {
        let def = make_def(vec!["repo", "repo"], vec![]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("duplicate")),
            "expected 'duplicate' error for repeated arg name, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn flag_and_arg_sharing_name_produce_conflict_error() {
        let def = make_def_with_flags_and_args(vec![make_flag("output", None)], vec!["output"]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("conflict")),
            "expected 'conflict' error for flag/arg name collision, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn valid_flag_and_short_passes_cleanly() {
        let def = make_def_with_flags(vec![make_flag("output", Some("o"))]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.is_empty(),
            "expected no errors for valid flag, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn no_flags_no_args_passes_cleanly() {
        let def = make_def(vec![], vec![]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.is_empty(),
            "expected no errors for empty flags/args, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    // ── frontmatter name validation: Stage 2 format checks ───────────────────

    #[test]
    fn valid_flag_name_output_passes() {
        let def = make_def_with_flags(vec![make_flag("output", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.is_empty(),
            "expected no errors for flag 'output', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn empty_flag_name_produces_error() {
        let def = make_def_with_flags(vec![make_flag("", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("empty")),
            "expected 'empty' error for blank flag name, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn flag_name_with_space_produces_invalid_chars_error() {
        let def = make_def_with_flags(vec![make_flag("foo bar", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("invalid characters")),
            "expected 'invalid characters' error for 'foo bar', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn flag_name_with_slash_produces_invalid_chars_error() {
        let def = make_def_with_flags(vec![make_flag("foo/bar", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("invalid characters")),
            "expected 'invalid characters' error for 'foo/bar', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn flag_name_starting_with_digit_produces_error() {
        let def = make_def_with_flags(vec![make_flag("3things", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("must start with")),
            "expected start-with error for '3things', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn empty_arg_name_produces_error() {
        let def = make_def(vec![""], vec![]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("empty")),
            "expected 'empty' error for blank arg name, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn arg_name_with_space_produces_invalid_chars_error() {
        let def = make_def(vec!["my arg"], vec![]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("invalid characters")),
            "expected 'invalid characters' error for 'my arg', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn multichar_short_form_produces_error() {
        let def = make_def_with_flags(vec![make_flag("output", Some("ab"))]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("single ASCII letter")),
            "expected 'single ASCII letter' error for short 'ab', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn digit_short_form_produces_error() {
        let def = make_def_with_flags(vec![make_flag("output", Some("3"))]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("single ASCII letter")),
            "expected 'single ASCII letter' error for short '3', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn empty_short_form_produces_error() {
        let def = make_def_with_flags(vec![make_flag("output", Some(""))]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("empty")),
            "expected 'empty' error for blank short form, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn dry_run_is_caught_by_reserved_not_char_check() {
        // "dry-run" contains a hyphen which is valid — the reserved check fires,
        // not the character check.
        let def = make_def_with_flags(vec![make_flag("dry-run", None)]);
        let result = validate_skill(&def, &[], "", None);
        assert!(
            result.errors.iter().any(|e| e.message.contains("reserved")),
            "expected 'reserved' error for 'dry-run', got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
        // Must not also produce an invalid-characters error for 'dry-run'.
        assert!(
            !result
                .errors
                .iter()
                .any(|e| e.message.contains("invalid characters") && e.message.contains("dry-run")),
            "should not produce 'invalid characters' error for 'dry-run' (valid chars)"
        );
    }

    // ── fence nesting integration tests ───────────────────────────────────────

    #[test]
    fn validate_skill_rejects_body_with_improperly_nested_fences() {
        let def = make_def(vec![], vec![]);
        // 3-backtick bash block containing an inner ```python fence — the inner
        // fence would close the outer block, truncating the bash block.
        let body = "```bash\necho hello\n```python\nprint('hi')\n```\n```\n";
        let result = validate_skill(&def, &[], body, None);
        assert!(
            result.has_errors(),
            "expected fence nesting error, got no errors"
        );
        assert!(
            result
                .errors
                .iter()
                .any(|e| e.message.contains("backticks")),
            "expected error message to mention backticks, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    #[test]
    fn validate_skill_accepts_body_with_four_backtick_outer_fence() {
        let def = make_def(vec![], vec![]);
        // 4-backtick outer fence containing inner 3-backtick examples — valid.
        let body = "````bash\necho hello\n```python\nprint('hi')\n```\n````\n";
        let result = validate_skill(&def, &[], body, None);
        assert!(
            !result.has_errors(),
            "expected no errors for properly nested fences, got: {:?}",
            result.errors.iter().map(|e| &e.message).collect::<Vec<_>>()
        );
    }

    fn make_block_with_extension(lang: &str, code: &str, extension: Option<&str>) -> CodeBlock {
        CodeBlock {
            lang: lang.into(),
            code: code.into(),
            deps: vec![],
            extension: extension.map(str::to_string),
            flags: None,
            llm_config: None,
            llm_parse_error: None,
        }
    }

    /// A non-family lang tag whose binary is absent from PATH produces a warning
    /// with the interpreter name and the "via stdin" message.
    #[test]
    fn validate_skill_warns_when_unknown_tag_binary_missing() {
        let def = make_def(vec![], vec![]);
        // Use a tag that cannot exist on any machine.
        let block = make_block("an-interpreter-that-cannot-exist-9a8b7c6d", "echo hello");
        let result = validate_skill(&def, &[block], "", None);
        let path_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.contains("not found on PATH"))
            .collect();
        assert_eq!(
            path_warnings.len(),
            1,
            "expected exactly one PATH warning; got: {:?}",
            result.warnings
        );
        assert!(
            path_warnings[0]
                .message
                .contains("an-interpreter-that-cannot-exist-9a8b7c6d"),
            "warning must name the missing interpreter"
        );
        assert!(
            path_warnings[0].message.contains("via stdin"),
            "warning must mention stdin execution"
        );
        assert_eq!(
            path_warnings[0].block_index,
            Some(0),
            "warning must identify the block"
        );
    }

    /// Known-family tags (bash, python, etc.) never trigger the PATH warning —
    /// `creft doctor` handles family interpreter availability separately.
    #[rstest]
    #[case::bash("bash")]
    #[case::sh("sh")]
    #[case::zsh("zsh")]
    #[case::python("python")]
    #[case::python3("python3")]
    #[case::node("node")]
    #[case::javascript("javascript")]
    #[case::js("js")]
    #[case::typescript("typescript")]
    #[case::ts("ts")]
    fn validate_skill_no_path_warning_for_known_families(#[case] lang: &str) {
        let def = make_def(vec![], vec![]);
        let block = make_block(lang, "echo hello");
        let result = validate_skill(&def, &[block], "", None);
        let path_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.contains("not found on PATH") && w.message.contains("via stdin"))
            .collect();
        assert!(
            path_warnings.is_empty(),
            "known family '{lang}' must not produce an interpreter PATH warning; got: {:?}",
            path_warnings
        );
    }

    /// The warning carries the correct block_index for the second block (index 1)
    /// in a multi-block skill.
    #[test]
    fn validate_skill_path_warning_carries_correct_block_index() {
        let def = make_def(vec![], vec![]);
        let bash_block = make_block("bash", "echo first");
        let unknown_block = make_block("an-interpreter-that-cannot-exist-9a8b7c6d", "echo second");
        let result = validate_skill(&def, &[bash_block, unknown_block], "", None);
        let path_warnings: Vec<_> = result
            .warnings
            .iter()
            .filter(|w| w.message.contains("via stdin"))
            .collect();
        assert_eq!(path_warnings.len(), 1, "expected exactly one PATH warning");
        assert_eq!(
            path_warnings[0].block_index,
            Some(1),
            "PATH warning must reference block index 1 (the second block)"
        );
    }

    /// Valid extensions that should produce no errors.
    #[rstest]
    #[case::simple("rb")]
    #[case::dotted("tar.gz")]
    #[case::with_hyphen("min.js")]
    #[case::underscore("my_ext")]
    #[case::pyi("pyi")]
    // The parser strips one leading dot before writing block.extension, so the
    // validator only sees the residue without it. Direct fixture must reflect
    // the post-parse value ("mjs", not ".mjs").
    #[case::post_parse_mjs("mjs")]
    fn check_extension_value_accepts_valid_extensions(#[case] ext: &str) {
        let def = make_def(vec![], vec![]);
        let block = make_block_with_extension("ruby", "puts 'hi'", Some(ext));
        let result = validate_skill(&def, &[block], "", None);
        let ext_errors: Vec<_> = result
            .errors
            .iter()
            .filter(|e| e.message.contains("extension"))
            .collect();
        assert!(
            ext_errors.is_empty(),
            "extension '{ext}' should be valid; got errors: {:?}",
            ext_errors
        );
    }

    /// Invalid extensions that must produce an error. Each case identifies the
    /// rule it exercises.
    ///
    /// Note on `leading_dot`: the parser strips one leading dot before writing
    /// `block.extension`, so `# extension: .foo` in markdown produces
    /// `block.extension = Some("foo")` — a valid identifier that the validator
    /// accepts. The case below (`".foo"`) tests a direct fixture that bypasses
    /// the parser, demonstrating that the validator still enforces the invariant
    /// (block.extension must not carry a leading dot). The end-to-end behaviour
    /// is covered by `extension_leading_dot_round_trip_accepted`.
    #[rstest]
    #[case::leading_dot(
        ".foo",
        "leading dot — direct fixture bypasses parser; invariant check"
    )]
    #[case::trailing_dot("bar.", "trailing dot")]
    #[case::dot_only(".", "dot only")]
    #[case::path_separator("../etc/passwd", "path separator")]
    #[case::slash("foo/bar", "slash")]
    #[case::space("foo bar", "space")]
    fn check_extension_value_rejects_invalid_extensions(#[case] ext: &str, #[case] _reason: &str) {
        let def = make_def(vec![], vec![]);
        let block = make_block_with_extension("ruby", "puts 'hi'", Some(ext));
        let result = validate_skill(&def, &[block], "", None);
        let ext_errors: Vec<_> = result
            .errors
            .iter()
            .filter(|e| e.message.contains("extension"))
            .collect();
        assert_eq!(
            ext_errors.len(),
            1,
            "extension '{ext}' ({_reason}) should produce exactly one error; got: {:?}",
            ext_errors
        );
        assert!(
            ext_errors[0].message.contains(ext)
                || ext_errors[0].message.contains("simple identifier"),
            "error message should describe the problem; got: {}",
            ext_errors[0].message
        );
    }

    /// An empty `# extension:` value produces a dedicated error message (not the
    /// generic identifier message).
    #[test]
    fn check_extension_value_rejects_empty_value() {
        let def = make_def(vec![], vec![]);
        let block = make_block_with_extension("ruby", "puts 'hi'", Some(""));
        let result = validate_skill(&def, &[block], "", None);
        let ext_errors: Vec<_> = result
            .errors
            .iter()
            .filter(|e| e.message.contains("extension"))
            .collect();
        assert_eq!(
            ext_errors.len(),
            1,
            "empty extension should produce exactly one error; got: {:?}",
            ext_errors
        );
        assert!(
            ext_errors[0].message.contains("empty"),
            "error must mention 'empty'; got: {}",
            ext_errors[0].message
        );
    }

    /// Prove that `# extension: .mjs` in markdown round-trips through the parser
    /// and produces no extension-related validation error.
    ///
    /// This is the load-bearing test: it exercises the full user journey from
    /// authored markdown to `validate_skill`, confirming that the parser's
    /// leading-dot strip relaxes the validator's gate at the user-visible level.
    #[test]
    fn extension_leading_dot_round_trip_accepted() {
        let body = "```bash\n# extension: .mjs\necho hi\n```\n";
        let (_, blocks) = crate::markdown::extract_blocks(body);
        assert_eq!(blocks.len(), 1, "expected one block");
        // Parser must have stored "mjs", not ".mjs".
        assert_eq!(
            blocks[0].extension.as_deref(),
            Some("mjs"),
            "parser must strip the leading dot; got: {:?}",
            blocks[0].extension
        );
        let def = make_def(vec![], vec![]);
        let result = validate_skill(&def, &blocks, "", None);
        let ext_errors: Vec<_> = result
            .errors
            .iter()
            .filter(|e| e.message.contains("extension"))
            .collect();
        assert!(
            ext_errors.is_empty(),
            "# extension: .mjs must be accepted after parser normalization; got errors: {:?}",
            ext_errors
        );
    }

    /// When `block.extension` is `None`, `check_extension_value` produces no errors.
    #[test]
    fn check_extension_value_skipped_when_none() {
        let def = make_def(vec![], vec![]);
        let block = make_block("bash", "echo hello");
        let result = validate_skill(&def, &[block], "", None);
        let ext_errors: Vec<_> = result
            .errors
            .iter()
            .filter(|e| e.message.contains("extension"))
            .collect();
        assert!(
            ext_errors.is_empty(),
            "no extension directive → no extension errors; got: {:?}",
            ext_errors
        );
    }
}
