mcp-execution-skill 0.9.0

//! Server metadata sidecar reader.
//!
//! Reads the structured `_meta.json` sidecar emitted by
//! `mcp-execution-codegen` alongside a server's generated TypeScript tool
//! files, and maps it into this crate's [`ParsedToolFile`] / [`ParsedParameter`]
//! types for skill generation.
//!
//! Prior to this module, tool metadata was recovered by re-parsing the
//! generated `.ts` files with regexes — a lossy, fragile round-trip that,
//! among other issues, could never recover parameter descriptions. The
//! sidecar is a structured, serde-derived contract shared with codegen via
//! `mcp_execution_core::metadata`, so no re-parsing of generated source is
//! needed at all.

use mcp_execution_core::metadata::{METADATA_FILE_NAME, METADATA_SCHEMA_VERSION, ServerMetadata};
use regex::Regex;
use serde::Deserialize;
use std::path::Path;
use std::sync::LazyLock;
use thiserror::Error;

/// Maximum number of tools accepted from a single sidecar (denial-of-service protection).
pub const MAX_TOOL_FILES: usize = 500;

/// Maximum sidecar file size to read in bytes (1MB).
pub const MAX_FILE_SIZE: u64 = 1024 * 1024;

/// Maximum size of a `SKILL.md`'s extracted YAML frontmatter block, in bytes.
///
/// `serde_norway` (like other libyaml-based parsers) is not linear-time on
/// pathologically nested input (e.g. deeply nested flow sequences), so
/// bounding only the overall `SKILL.md` content size does not bound parse
/// latency. A real `name`/`description` frontmatter is a few hundred bytes at
/// most, so 8KB is already generous while keeping [`extract_skill_metadata`]
/// cheap enough to run synchronously on `save_skill`'s request-handling task.
///
/// This cap is the project-wide contract for any YAML entry point, not a local
/// detail of this one — see the project constitution's security section.
pub const MAX_FRONTMATTER_SIZE: usize = 8 * 1024;

// Locates the raw YAML block between a SKILL.md's `---` delimiters. The
// block's contents are handed to `serde_norway` for actual parsing, so this
// regex never inspects individual field values (see `extract_skill_metadata`).
static FRONTMATTER_REGEX: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"^---\s*\n([\s\S]*?)\n---").expect("valid regex"));

// `sanitize_path_for_error` lives in `mcp-execution-core`, the workspace's
// security-validation foundation crate.
use mcp_execution_core::sanitize_path_for_error;

/// Errors that can occur while scanning a server directory for its `_meta.json` sidecar.
#[derive(Debug, Error)]
pub enum ScanError {
    /// I/O error reading the directory or sidecar file.
    #[error("I/O error: {0}")]
    Io(#[from] std::io::Error),

    /// Directory does not exist.
    #[error("directory does not exist: {path}")]
    DirectoryNotFound {
        /// Sanitized path of the missing directory.
        path: String,
    },

    /// The `_meta.json` sidecar is missing from the server directory.
    #[error("metadata sidecar not found: {path} (was the server directory regenerated?)")]
    MissingMetadata {
        /// Sanitized path of the expected sidecar file.
        path: String,
    },

    /// The `_meta.json` sidecar could not be parsed as valid `ServerMetadata` JSON.
    #[error("failed to parse metadata sidecar {path}: {source}")]
    MetadataParse {
        /// Sanitized path of the sidecar file that failed to parse.
        path: String,
        /// Underlying JSON deserialization error.
        #[source]
        source: serde_json::Error,
    },

    /// The sidecar's `schema_version` does not match the version this crate understands.
    #[error("unsupported metadata schema version: found {found}, expected {expected}")]
    UnsupportedSchema {
        /// Schema version read from the sidecar.
        found: u32,
        /// Schema version this crate supports (`METADATA_SCHEMA_VERSION`).
        expected: u32,
    },

    /// Too many tools in the sidecar (denial-of-service protection).
    #[error("too many tools: {count} exceeds limit of {limit}")]
    TooManyFiles {
        /// Number of tools listed in the sidecar.
        count: usize,
        /// Maximum allowed number of tools (`MAX_TOOL_FILES`).
        limit: usize,
    },

    /// Sidecar file too large to process.
    #[error("file too large: {path} ({size} bytes exceeds {limit} limit)")]
    FileTooLarge {
        /// Sanitized path of the oversized sidecar file.
        path: String,
        /// Actual size of the file, in bytes.
        size: u64,
        /// Maximum allowed size, in bytes (`MAX_FILE_SIZE`).
        limit: u64,
    },

    /// A tool listed in the `_meta.json` sidecar has no corresponding `.ts`
    /// file on disk.
    ///
    /// This indicates the sidecar and the generated TypeScript files have
    /// drifted apart — e.g. the file was deleted manually, or a `generate`
    /// run was interrupted before writing it.
    #[error(
        "stale metadata: tool '{tool}' is listed in {sidecar_path} but its file '{expected_file}' \
         is missing (re-run 'generate' to regenerate this server)"
    )]
    StaleMetadata {
        /// MCP tool name listed in the sidecar.
        tool: String,
        /// `.ts` file name expected on disk for `tool`.
        expected_file: String,
        /// Sanitized path of the sidecar that references `tool`.
        sidecar_path: String,
    },
}

/// Result of [`scan_tools_directory`]: the parsed tools plus any non-fatal
/// drift warnings.
///
/// A warning does not fail the scan — it flags a `.ts` file on disk that was
/// excluded from `tools` because the sidecar has no matching entry for it.
/// Callers that only inspect `tools` would otherwise have no way to detect
/// this drift short of tailing server-side `tracing` output.
#[derive(Debug, Clone, Default)]
pub struct ScanResult {
    /// Parsed tools, sorted by name.
    pub tools: Vec<ParsedToolFile>,

    /// Non-fatal warnings, e.g. `.ts` files excluded for lacking a sidecar entry.
    pub warnings: Vec<String>,
}

/// Parsed metadata from a server's generated tool set.
#[derive(Debug, Clone)]
pub struct ParsedToolFile {
    /// Original MCP tool name.
    pub name: String,

    /// TypeScript function name (`PascalCase` filename).
    pub typescript_name: String,

    /// Server identifier.
    pub server_id: String,

    /// Category for grouping.
    pub category: Option<String>,

    /// Keywords for discovery.
    pub keywords: Vec<String>,

    /// Tool description.
    pub description: Option<String>,

    /// Parsed parameters for the tool.
    pub parameters: Vec<ParsedParameter>,
}

/// A parsed parameter from a tool's metadata.
#[derive(Debug, Clone)]
pub struct ParsedParameter {
    /// Parameter name.
    pub name: String,

    /// TypeScript type (e.g., "string", "number", "boolean").
    pub typescript_type: String,

    /// Whether the parameter is required.
    pub required: bool,

    /// Parameter description.
    pub description: Option<String>,
}

impl From<mcp_execution_core::metadata::ParameterMetadata> for ParsedParameter {
    fn from(meta: mcp_execution_core::metadata::ParameterMetadata) -> Self {
        Self {
            name: meta.name,
            typescript_type: meta.typescript_type,
            required: meta.required,
            description: meta.description,
        }
    }
}

/// Builds a [`ParsedToolFile`] from a sidecar tool entry and the server ID it belongs to.
///
/// A plain function rather than a `From<ToolMetadata>` impl: `ToolMetadata` carries no
/// `server_id` of its own (it lives once on the enclosing [`ServerMetadata`]), so a `From` impl
/// could only ever produce a `ParsedToolFile` with a placeholder `server_id` that every caller
/// then had to patch in after construction — a representable-but-wrong intermediate state.
/// Taking `server_id` as a parameter removes that sentinel from this construction path;
/// `ParsedToolFile`'s fields remain public, so callers that build one directly (e.g. test
/// fixtures) are unaffected and can still set an arbitrary `server_id` themselves.
fn parsed_tool_file_from_metadata(
    meta: mcp_execution_core::metadata::ToolMetadata,
    server_id: &str,
) -> ParsedToolFile {
    ParsedToolFile {
        name: meta.name.into_inner(),
        typescript_name: meta.typescript_name,
        server_id: server_id.to_string(),
        category: meta.category,
        keywords: meta.keywords,
        description: meta.description,
        parameters: meta.parameters.into_iter().map(Into::into).collect(),
    }
}

/// Scan a server directory and read its `_meta.json` sidecar.
///
/// Reads the structured metadata sidecar written by `mcp-execution-codegen`
/// and maps each tool entry into a [`ParsedToolFile`]. Unlike the former
/// regex-based `.ts` scanner, tool metadata (name, category, keywords,
/// parameters) is never re-parsed from generated TypeScript source — the
/// sidecar remains the single source of truth for that. However, each
/// sidecar entry's `.ts` file is cross-checked for existence on disk to
/// detect drift between the sidecar and the generated files (see issues
/// #154, #155): a missing file is a hard error, while an unreferenced `.ts`
/// file on disk is logged via `tracing::warn!`, omitted from the result, and
/// named in the returned [`ScanResult::warnings`] (see issue #161).
///
/// # Arguments
///
/// * `dir` - Path to server directory (e.g., `~/.claude/servers/github`)
///
/// # Returns
///
/// [`ScanResult`] with one `ParsedToolFile` per tool in the sidecar (sorted
/// by name) plus any non-fatal drift warnings.
///
/// # Errors
///
/// Returns `ScanError` if the directory doesn't exist, the sidecar is
/// missing or malformed, the sidecar's tool count exceeds
/// [`MAX_TOOL_FILES`], or a sidecar entry's `.ts` file is missing from disk
/// ([`ScanError::StaleMetadata`]).
///
/// # Examples
///
/// ```no_run
/// use mcp_execution_skill::scan_tools_directory;
/// use std::path::Path;
///
/// # async fn example() -> Result<(), mcp_execution_skill::ScanError> {
/// let result = scan_tools_directory(Path::new("/home/user/.claude/servers/github")).await?;
/// println!("Found {} tools", result.tools.len());
/// # Ok(())
/// # }
/// ```
pub async fn scan_tools_directory(dir: &Path) -> Result<ScanResult, ScanError> {
    // Canonicalize the base directory to resolve symlinks and get absolute path
    let canonical_base = tokio::fs::canonicalize(dir).await.map_err(|err| {
        if err.kind() == std::io::ErrorKind::NotFound {
            ScanError::DirectoryNotFound {
                path: sanitize_path_for_error(dir),
            }
        } else {
            ScanError::Io(err)
        }
    })?;

    let meta_path = canonical_base.join(METADATA_FILE_NAME);

    // SECURITY: Canonicalize the sidecar path and validate it stays within the base
    // directory, preventing path traversal via a symlinked `_meta.json`.
    let canonical_meta = match tokio::fs::canonicalize(&meta_path).await {
        Ok(path) if path.starts_with(&canonical_base) => path,
        Ok(_) => {
            return Err(ScanError::MissingMetadata {
                path: sanitize_path_for_error(&meta_path),
            });
        }
        Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
            return Err(ScanError::MissingMetadata {
                path: sanitize_path_for_error(&meta_path),
            });
        }
        Err(err) => return Err(ScanError::Io(err)),
    };

    let file_metadata = tokio::fs::metadata(&canonical_meta).await?;
    if file_metadata.len() > MAX_FILE_SIZE {
        return Err(ScanError::FileTooLarge {
            path: sanitize_path_for_error(&meta_path),
            size: file_metadata.len(),
            limit: MAX_FILE_SIZE,
        });
    }

    let content = tokio::fs::read_to_string(&canonical_meta).await?;

    let meta: ServerMetadata =
        serde_json::from_str(&content).map_err(|source| ScanError::MetadataParse {
            path: sanitize_path_for_error(&meta_path),
            source,
        })?;

    if meta.schema_version != METADATA_SCHEMA_VERSION {
        return Err(ScanError::UnsupportedSchema {
            found: meta.schema_version,
            expected: METADATA_SCHEMA_VERSION,
        });
    }

    if meta.tools.len() > MAX_TOOL_FILES {
        return Err(ScanError::TooManyFiles {
            count: meta.tools.len(),
            limit: MAX_TOOL_FILES,
        });
    }

    let warnings = verify_tool_files_on_disk(&canonical_base, &meta.tools, &meta_path).await?;

    let server_id = meta.server_id.into_inner();
    let mut tools: Vec<ParsedToolFile> = meta
        .tools
        .into_iter()
        .map(|tool| parsed_tool_file_from_metadata(tool, &server_id))
        .collect();

    // Sort by name for consistent ordering
    tools.sort_by(|a, b| a.name.cmp(&b.name));

    Ok(ScanResult { tools, warnings })
}

/// Cross-checks sidecar tool entries against the `.ts` files actually
/// present in `dir`, guarding against drift between `_meta.json` and the
/// generated TypeScript output (see issues #154, #155).
///
/// Every sidecar entry must have a matching `{typescript_name}.ts` file, or
/// this returns [`ScanError::StaleMetadata`]. `.ts` files present on disk
/// but not referenced by the sidecar are not fatal — regenerating tool
/// files is a normal part of `generate` — but are logged via
/// `tracing::warn!` and returned as human-readable warning strings so the
/// drift isn't silently dropped from `SKILL.md` or invisible to structured
/// callers (issue #161).
///
/// # Errors
///
/// Returns `ScanError::Io` if the directory cannot be read, or
/// `ScanError::StaleMetadata` if a sidecar entry's `.ts` file is missing.
async fn verify_tool_files_on_disk(
    dir: &Path,
    tools: &[mcp_execution_core::metadata::ToolMetadata],
    meta_path: &Path,
) -> Result<Vec<String>, ScanError> {
    // Generated aggregator file, not a per-tool file — never expected in the sidecar.
    const INDEX_FILE_NAME: &str = "index.ts";

    let mut expected_files: std::collections::HashSet<String> =
        std::collections::HashSet::with_capacity(tools.len());

    for tool in tools {
        let file_name = format!("{}.ts", tool.typescript_name);
        if !dir.join(&file_name).is_file() {
            return Err(ScanError::StaleMetadata {
                tool: tool.name.to_string(),
                expected_file: file_name,
                sidecar_path: sanitize_path_for_error(meta_path),
            });
        }
        expected_files.insert(file_name);
    }

    let mut warnings = Vec::new();
    let mut entries = tokio::fs::read_dir(dir).await?;
    while let Some(entry) = entries.next_entry().await? {
        let path = entry.path();
        if path.extension().and_then(std::ffi::OsStr::to_str) != Some("ts") {
            continue;
        }
        let Some(file_name) = path.file_name().and_then(std::ffi::OsStr::to_str) else {
            continue;
        };
        if file_name == INDEX_FILE_NAME || expected_files.contains(file_name) {
            continue;
        }
        tracing::warn!(
            file = %file_name,
            "found .ts tool file not referenced by _meta.json; it will be omitted from SKILL.md \
             (re-run 'generate' to refresh the sidecar)"
        );
        warnings.push(format!(
            "'{file_name}' is not referenced by _meta.json and was excluded from SKILL.md \
             (re-run 'generate' to refresh the sidecar)"
        ));
    }

    Ok(warnings)
}

/// Errors that can occur while extracting [`SkillMetadata`](crate::types::SkillMetadata)
/// from a `SKILL.md`'s YAML frontmatter.
#[derive(Debug, Error)]
pub enum SkillMetadataError {
    /// The content did not start with a `---`-delimited YAML frontmatter block.
    #[error("YAML frontmatter not found")]
    MissingFrontmatter,

    /// The extracted frontmatter block exceeded `MAX_FRONTMATTER_SIZE`.
    #[error("YAML frontmatter too large: {size} bytes exceeds {limit} limit")]
    FrontmatterTooLarge {
        /// Actual size of the rejected frontmatter block, in bytes.
        size: usize,
        /// Maximum allowed size (`MAX_FRONTMATTER_SIZE`).
        limit: usize,
    },

    /// The frontmatter block was not valid YAML.
    ///
    /// The message is a rendering of the underlying `serde_norway` error
    /// (captured eagerly rather than storing the error type itself, so this
    /// crate's public API is not pinned to a specific `serde_norway`
    /// version) with its line number corrected to be relative to the whole
    /// `SKILL.md` file rather than the extracted frontmatter block — the
    /// block starts one line after the file's opening `---` delimiter.
    #[error("failed to parse YAML frontmatter: {0}")]
    InvalidYaml(String),

    /// A required field was absent, or present but empty, in an otherwise
    /// valid frontmatter block.
    #[error("'{field}' field is missing or empty in frontmatter")]
    MissingField {
        /// Name of the missing/empty field (e.g. `"name"`, `"description"`).
        field: &'static str,
    },
}

/// Renders a `serde_norway` deserialization error, correcting its line number
/// to be relative to the whole `SKILL.md` file rather than the frontmatter
/// block passed to `serde_norway::from_str` (see
/// [`SkillMetadataError::InvalidYaml`]).
fn describe_yaml_error(err: &serde_norway::Error) -> String {
    let rendered = err.to_string();
    let Some(location) = err.location() else {
        return rendered;
    };
    // The block starts one line after the file's opening `---`, so the
    // block-relative line number under-counts the file line by exactly one.
    let block_relative = format!("line {} column {}", location.line(), location.column());
    let file_relative = format!("line {} column {}", location.line() + 1, location.column());
    rendered.replacen(&block_relative, &file_relative, 1)
}

/// Raw shape of a `SKILL.md`'s YAML frontmatter block.
///
/// Both fields are optional at the YAML level so that a missing `name` and a
/// missing `description` are reported as distinct
/// [`SkillMetadataError::MissingField`] variants rather than being folded
/// into one generic deserialization failure.
///
/// # Regression coverage (issue #359)
///
/// Both fields being plain `Option<String>` (no `#[serde(flatten)]`, no buffering
/// `deserialize_with`, no untagged/`Value` retype) is why an alias bomb placed under an
/// undeclared key or under `description` itself is discarded today without expanding nested
/// aliases (`tests::test_extract_skill_metadata_alias_bomb_under_unknown_key_stays_ok`,
/// `tests::test_extract_skill_metadata_alias_bomb_under_declared_field_short_circuits`). If
/// `description` (or `name`) is ever changed to a buffering type — `serde_norway::Value`, an
/// untagged enum, or a `#[serde(deserialize_with)]` that internally buffers through one of
/// those while keeping the field declared as `Option<String>` — that change reopens the
/// amplification path for a bomb placed directly under the affected key, and the
/// declared-field test above flips loudly (its error stops being a plain type mismatch and
/// becomes `serde_norway`'s "repetition limit exceeded"). A `Value`/untagged retype is also
/// compile-blocked at `require_field`'s `Option<String>` parameter in the call below; a
/// buffering `deserialize_with` is not, since the declared type is unchanged. Both underlying
/// assumptions are additionally pinned in isolation, against local test-only structs shaped
/// like each hypothetical retype, by
/// `tests::test_serde_norway_buffers_alias_bomb_when_declared_field_is_value_typed` and
/// `tests::test_serde_norway_buffers_alias_bomb_when_declared_field_uses_deserialize_with`. If
/// this struct's field shape is ever changed in either direction, all three tests above need
/// re-checking against the new shape.
#[derive(Debug, Deserialize)]
struct RawFrontmatter {
    name: Option<String>,
    description: Option<String>,
}

/// Returns `value` if present and non-blank, otherwise a
/// [`SkillMetadataError::MissingField`] naming `field`.
///
/// Treats an absent key, a null/empty scalar (`name:`, `name: null`,
/// `name: ~`), and a blank string (`name: ""`, `name: "   "`) as equally
/// invalid — a skill with an empty name or description is not usable.
fn require_field(value: Option<String>, field: &'static str) -> Result<String, SkillMetadataError> {
    match value {
        Some(v) if !v.trim().is_empty() => Ok(v),
        _ => Err(SkillMetadataError::MissingField { field }),
    }
}

/// Extract skill metadata from SKILL.md content.
///
/// Parses YAML frontmatter to extract name and description, and counts
/// sections (H2 headers) and words. Frontmatter is parsed with a real YAML
/// parser (`serde_norway`), so block scalars (`description: |` / `>`) and
/// quoted scalars (`name: "my-name"`) are handled per the YAML spec rather
/// than by a single-line regex capture.
///
/// # Arguments
///
/// * `content` - SKILL.md content with YAML frontmatter
///
/// # Returns
///
/// `SkillMetadata` with extracted information.
///
/// # Errors
///
/// Returns [`SkillMetadataError`] if the YAML frontmatter is missing, too
/// large (`MAX_FRONTMATTER_SIZE`), malformed, or a required field (`name`,
/// `description`) is absent or empty.
///
/// # Examples
///
/// ```
/// use mcp_execution_skill::extract_skill_metadata;
///
/// let content = r"---
/// name: github-progressive
/// description: GitHub MCP server operations
/// ---
///
/// # GitHub Progressive
///
/// ## Quick Start
///
/// Content here.
/// ";
///
/// let metadata = extract_skill_metadata(content).unwrap();
/// assert_eq!(metadata.name, "github-progressive");
/// assert_eq!(metadata.description, "GitHub MCP server operations");
/// ```
pub fn extract_skill_metadata(
    content: &str,
) -> Result<crate::types::SkillMetadata, SkillMetadataError> {
    use crate::types::SkillMetadata;

    // Locate the raw YAML block between the `---` delimiters (using pre-compiled regex).
    let frontmatter_block = FRONTMATTER_REGEX
        .captures(content)
        .and_then(|c| c.get(1))
        .map(|m| m.as_str())
        .ok_or(SkillMetadataError::MissingFrontmatter)?;

    // Bound parse cost before handing the block to `serde_norway`: YAML parsing is not
    // linear-time on pathologically nested input, so the overall SKILL.md size limit
    // (`MAX_SKILL_CONTENT_SIZE` in mcp-execution-server) does not bound parse latency.
    if frontmatter_block.len() > MAX_FRONTMATTER_SIZE {
        return Err(SkillMetadataError::FrontmatterTooLarge {
            size: frontmatter_block.len(),
            limit: MAX_FRONTMATTER_SIZE,
        });
    }

    let frontmatter: RawFrontmatter = serde_norway::from_str(frontmatter_block)
        .map_err(|e| SkillMetadataError::InvalidYaml(describe_yaml_error(&e)))?;

    let name = require_field(frontmatter.name, "name")?;
    let description = require_field(frontmatter.description, "description")?;

    // Count sections (H2 headers)
    let section_count = content.lines().filter(|l| l.starts_with("## ")).count();

    // Count words (approximate)
    let word_count = content.split_whitespace().count();

    Ok(SkillMetadata {
        name,
        description,
        section_count,
        word_count,
    })
}

#[cfg(test)]
mod tests {
    use super::*;
    use mcp_execution_core::metadata::{ParameterMetadata, ToolMetadata};
    use mcp_execution_core::{ServerId, ToolName};
    use tempfile::TempDir;

    fn sample_metadata(tool_count: usize) -> ServerMetadata {
        ServerMetadata {
            schema_version: METADATA_SCHEMA_VERSION,
            server_id: ServerId::new("github").unwrap(),
            server_name: "GitHub".to_string(),
            server_version: "1.0.0".to_string(),
            tools: (0..tool_count)
                .map(|i| ToolMetadata {
                    name: ToolName::new(format!("tool_{i}")).unwrap(),
                    typescript_name: format!("tool{i}"),
                    category: Some("test".to_string()),
                    keywords: vec!["test".to_string()],
                    description: Some(format!("Tool {i}")),
                    parameters: vec![ParameterMetadata {
                        name: "param".to_string(),
                        typescript_type: "string".to_string(),
                        required: true,
                        description: Some("A parameter".to_string()),
                    }],
                })
                .collect(),
        }
    }

    /// Writes `_meta.json` plus a matching stub `.ts` file for each tool, since
    /// `scan_tools_directory` cross-checks the sidecar against files on disk.
    async fn write_metadata(dir: &Path, meta: &ServerMetadata) {
        let content = serde_json::to_string_pretty(meta).unwrap();
        tokio::fs::write(dir.join(METADATA_FILE_NAME), content)
            .await
            .unwrap();

        for tool in &meta.tools {
            tokio::fs::write(
                dir.join(format!("{}.ts", tool.typescript_name)),
                "export {}",
            )
            .await
            .unwrap();
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_round_trip_preserves_parameter_descriptions() {
        // Issue #141 regression: the old regex-based parser hard-coded parameter
        // descriptions to `None`. The sidecar-backed scanner must preserve them.
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(2);
        write_metadata(temp_dir.path(), &meta).await;

        let result = scan_tools_directory(temp_dir.path()).await.unwrap();
        let tools = result.tools;

        assert_eq!(tools.len(), 2);
        assert_eq!(tools[0].name, "tool_0");
        assert_eq!(tools[0].server_id, "github");
        assert_eq!(tools[0].parameters.len(), 1);
        assert_eq!(
            tools[0].parameters[0].description,
            Some("A parameter".to_string()),
            "parameter descriptions must survive the sidecar round-trip"
        );
        assert!(result.warnings.is_empty());
    }

    #[tokio::test]
    async fn test_scan_tools_directory_sorts_by_name() {
        let temp_dir = TempDir::new().unwrap();
        let mut meta = sample_metadata(0);
        meta.tools = vec![
            ToolMetadata {
                name: ToolName::new("zebra").unwrap(),
                typescript_name: "zebra".to_string(),
                category: None,
                keywords: vec![],
                description: None,
                parameters: vec![],
            },
            ToolMetadata {
                name: ToolName::new("alpha").unwrap(),
                typescript_name: "alpha".to_string(),
                category: None,
                keywords: vec![],
                description: None,
                parameters: vec![],
            },
        ];
        write_metadata(temp_dir.path(), &meta).await;

        let tools = scan_tools_directory(temp_dir.path()).await.unwrap().tools;

        assert_eq!(tools[0].name, "alpha");
        assert_eq!(tools[1].name, "zebra");
    }

    #[tokio::test]
    async fn test_scan_tools_directory_stale_metadata_missing_ts_file() {
        // Issue #154/#155 regression: a sidecar entry whose `.ts` file was
        // deleted (or never written, e.g. an interrupted `generate`) must be
        // reported instead of silently vanishing from `SKILL.md`.
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(1);
        // Write only the sidecar, not the tool's `.ts` file.
        let content = serde_json::to_string_pretty(&meta).unwrap();
        tokio::fs::write(temp_dir.path().join(METADATA_FILE_NAME), content)
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        match result {
            Err(ScanError::StaleMetadata {
                tool,
                expected_file,
                ..
            }) => {
                assert_eq!(tool, "tool_0");
                assert_eq!(expected_file, "tool0.ts");
            }
            other => panic!("expected StaleMetadata, got: {other:?}"),
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_stale_metadata_reports_first_missing_in_sidecar_order() {
        // With multiple tools in the sidecar, only some of which have a missing
        // `.ts` file, the check short-circuits on the first missing entry in
        // sidecar order rather than scanning every tool up front.
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(3);
        let content = serde_json::to_string_pretty(&meta).unwrap();
        tokio::fs::write(temp_dir.path().join(METADATA_FILE_NAME), content)
            .await
            .unwrap();

        // Only write the `.ts` file for the middle tool; `tool_0` and `tool_2`
        // are both missing, but `tool_0` is first in sidecar order.
        tokio::fs::write(temp_dir.path().join("tool1.ts"), "export {}")
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        match result {
            Err(ScanError::StaleMetadata {
                tool,
                expected_file,
                ..
            }) => {
                assert_eq!(tool, "tool_0");
                assert_eq!(expected_file, "tool0.ts");
            }
            other => panic!("expected StaleMetadata for tool_0, got: {other:?}"),
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_extra_ts_file_excluded_from_result() {
        // Issue #154/#155 regression: a `.ts` file on disk that the sidecar
        // does not reference (e.g. left over from a renamed/removed tool) must
        // not be fatal and must not leak into the scan result — it is logged
        // via `tracing::warn!` instead.
        //
        // Issue #161: the drift must also be visible in the returned
        // `ScanResult::warnings`, not just in the tracing log line.
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(1);
        write_metadata(temp_dir.path(), &meta).await;

        tokio::fs::write(temp_dir.path().join("orphan.ts"), "export {}")
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await.unwrap();

        assert_eq!(
            result.tools.len(),
            1,
            "the orphaned .ts file must not be reported as a tool"
        );
        assert_eq!(result.tools[0].name, "tool_0");
        assert_eq!(
            result.warnings.len(),
            1,
            "the orphaned .ts file must be surfaced as a warning"
        );
        assert!(
            result.warnings[0].contains("orphan.ts"),
            "warning must name the excluded file: {:?}",
            result.warnings[0]
        );
    }

    #[tokio::test]
    async fn test_scan_tools_directory_index_ts_not_treated_as_extra() {
        // `index.ts` is the generated aggregator file and is never listed in
        // the sidecar; its presence alone must not affect the scan result.
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(1);
        write_metadata(temp_dir.path(), &meta).await;

        tokio::fs::write(temp_dir.path().join("index.ts"), "export * from './tool0';")
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await.unwrap();

        assert_eq!(result.tools.len(), 1);
        assert_eq!(result.tools[0].name, "tool_0");
        assert!(
            result.warnings.is_empty(),
            "index.ts must not be reported as a warning"
        );
    }

    #[test]
    fn test_stale_metadata_error_message_tells_user_to_regenerate() {
        let err = ScanError::StaleMetadata {
            tool: "create_issue".to_string(),
            expected_file: "createIssue.ts".to_string(),
            sidecar_path: "~/.claude/servers/github/_meta.json".to_string(),
        };

        let message = err.to_string();
        assert!(
            message.contains("create_issue"),
            "message must name the affected tool"
        );
        assert!(
            message.contains("createIssue.ts"),
            "message must name the missing file"
        );
        assert!(
            message.contains("re-run 'generate'"),
            "message must tell the user how to fix it: {message}"
        );
    }

    #[tokio::test]
    async fn test_scan_tools_directory_missing_metadata() {
        let temp_dir = TempDir::new().unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        assert!(matches!(result, Err(ScanError::MissingMetadata { .. })));
    }

    #[tokio::test]
    async fn test_scan_tools_directory_corrupt_json() {
        let temp_dir = TempDir::new().unwrap();
        tokio::fs::write(temp_dir.path().join(METADATA_FILE_NAME), "{not valid json")
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        assert!(matches!(result, Err(ScanError::MetadataParse { .. })));
    }

    /// Regression test for issue #317: `ServerMetadata.server_id`/`ToolMetadata.name` now go
    /// through `ServerId`/`ToolName`'s `#[serde(try_from = "String")]`, so a sidecar that is
    /// syntactically valid JSON but carries a semantically-invalid `server_id` (here, one
    /// containing a path separator) now fails to deserialize at all — surfacing as
    /// `ScanError::MetadataParse` — where before this change it would have deserialized as a
    /// plain, unvalidated `String`.
    #[tokio::test]
    async fn test_scan_tools_directory_rejects_invalid_server_id_in_valid_json() {
        let temp_dir = TempDir::new().unwrap();
        let json = r#"{
            "schema_version": 1,
            "server_id": "not/a/valid/id",
            "server_name": "GitHub",
            "server_version": "1.0.0",
            "tools": []
        }"#;
        tokio::fs::write(temp_dir.path().join(METADATA_FILE_NAME), json)
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        assert!(matches!(result, Err(ScanError::MetadataParse { .. })));
    }

    /// Same regression as above, for `ToolMetadata.name`.
    #[tokio::test]
    async fn test_scan_tools_directory_rejects_invalid_tool_name_in_valid_json() {
        let temp_dir = TempDir::new().unwrap();
        let json = r#"{
            "schema_version": 1,
            "server_id": "github",
            "server_name": "GitHub",
            "server_version": "1.0.0",
            "tools": [{
                "name": "../escape",
                "typescript_name": "escape",
                "category": null,
                "keywords": [],
                "description": null,
                "parameters": []
            }]
        }"#;
        tokio::fs::write(temp_dir.path().join(METADATA_FILE_NAME), json)
            .await
            .unwrap();

        let result = scan_tools_directory(temp_dir.path()).await;

        assert!(matches!(result, Err(ScanError::MetadataParse { .. })));
    }

    #[tokio::test]
    async fn test_scan_tools_directory_unsupported_schema() {
        let temp_dir = TempDir::new().unwrap();
        let mut meta = sample_metadata(1);
        meta.schema_version = METADATA_SCHEMA_VERSION + 1;
        write_metadata(temp_dir.path(), &meta).await;

        let result = scan_tools_directory(temp_dir.path()).await;

        match result {
            Err(ScanError::UnsupportedSchema { found, expected }) => {
                assert_eq!(found, METADATA_SCHEMA_VERSION + 1);
                assert_eq!(expected, METADATA_SCHEMA_VERSION);
            }
            other => panic!("expected UnsupportedSchema, got: {other:?}"),
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_too_many_tools() {
        let temp_dir = TempDir::new().unwrap();
        let meta = sample_metadata(MAX_TOOL_FILES + 1);
        write_metadata(temp_dir.path(), &meta).await;

        let result = scan_tools_directory(temp_dir.path()).await;

        match result {
            Err(ScanError::TooManyFiles { count, limit }) => {
                assert_eq!(count, MAX_TOOL_FILES + 1);
                assert_eq!(limit, MAX_TOOL_FILES);
            }
            other => panic!("expected TooManyFiles, got: {other:?}"),
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_file_too_large() {
        let temp_dir = TempDir::new().unwrap();
        let mut meta = sample_metadata(1);
        // MAX_FILE_SIZE (1MB) always fits in usize; the cast cannot truncate.
        #[allow(clippy::cast_possible_truncation)]
        let padding = "a".repeat((MAX_FILE_SIZE as usize) + 1);
        meta.tools[0].description = Some(padding);
        write_metadata(temp_dir.path(), &meta).await;

        let result = scan_tools_directory(temp_dir.path()).await;

        match result {
            Err(ScanError::FileTooLarge { size, limit, .. }) => {
                assert!(size > MAX_FILE_SIZE);
                assert_eq!(limit, MAX_FILE_SIZE);
            }
            other => panic!("expected FileTooLarge, got: {other:?}"),
        }
    }

    #[tokio::test]
    async fn test_scan_tools_directory_nonexistent() {
        let result = scan_tools_directory(Path::new("/nonexistent/path/for/testing")).await;

        assert!(matches!(result, Err(ScanError::DirectoryNotFound { .. })));
    }

    #[tokio::test]
    #[cfg(unix)]
    async fn test_scan_tools_directory_canonicalize_non_not_found_error_propagates_as_io() {
        // Issue #302 regression: a symlink loop makes `canonicalize` fail with
        // `ErrorKind::FilesystemLoop`/`Other` (never `NotFound`). Before the fix,
        // every canonicalize failure — regardless of kind — collapsed into
        // `DirectoryNotFound`, silently discarding the real error.
        let temp_dir = TempDir::new().unwrap();
        let loop_path = temp_dir.path().join("loop");
        std::os::unix::fs::symlink(&loop_path, &loop_path).unwrap();

        let result = scan_tools_directory(&loop_path).await;

        match result {
            Err(ScanError::Io(err)) => {
                assert_ne!(err.kind(), std::io::ErrorKind::NotFound);
            }
            other => panic!("expected ScanError::Io, got: {other:?}"),
        }
    }

    // ========================================================================
    // extract_skill_metadata Tests
    // ========================================================================

    #[test]
    fn test_extract_skill_metadata_valid() {
        let content = r"---
name: github-progressive
description: GitHub MCP server operations
---

# GitHub Progressive

## Quick Start

Content here.

## Common Tasks

More content.
";

        let result = extract_skill_metadata(content);
        assert!(result.is_ok());

        let metadata = result.unwrap();
        assert_eq!(metadata.name, "github-progressive");
        assert_eq!(metadata.description, "GitHub MCP server operations");
        assert_eq!(metadata.section_count, 2);
        assert!(metadata.word_count > 0);
    }

    #[test]
    fn test_extract_skill_metadata_no_frontmatter() {
        let content = "# Test\n\nNo frontmatter";

        let result = extract_skill_metadata(content);
        assert!(matches!(
            result,
            Err(SkillMetadataError::MissingFrontmatter)
        ));
    }

    #[test]
    fn test_extract_skill_metadata_missing_name() {
        let content = "---\ndescription: test\n---\n# Test";

        let result = extract_skill_metadata(content);
        assert!(matches!(
            result,
            Err(SkillMetadataError::MissingField { field: "name" })
        ));
    }

    #[test]
    fn test_extract_skill_metadata_missing_description() {
        let content = "---\nname: test\n---\n# Test";

        let result = extract_skill_metadata(content);
        assert!(matches!(
            result,
            Err(SkillMetadataError::MissingField {
                field: "description"
            })
        ));
    }

    #[test]
    fn test_extract_skill_metadata_invalid_yaml() {
        // Syntactically invalid YAML (an unterminated flow sequence) must surface
        // as `SkillMetadataError::InvalidYaml`, wrapping the underlying serde_norway error.
        let content = "---\nname: [unterminated\ndescription: test\n---\n# Test";

        let result = extract_skill_metadata(content);
        let Err(SkillMetadataError::InvalidYaml(message)) = &result else {
            panic!("expected InvalidYaml, got: {result:?}");
        };
        // Issue #203 follow-up (M3): the error's line number must be file-relative, not
        // relative to the extracted frontmatter block. `name: [unterminated` is file line 2
        // (after the opening `---` on line 1); the block-relative location.line() the
        // underlying serde_norway error reports for this input is 1.
        assert!(
            message.contains("line 2"),
            "expected file-relative 'line 2', got: {message:?}"
        );
    }

    #[test]
    fn test_extract_skill_metadata_frontmatter_too_large() {
        // Issue #203 follow-up (S2): a pathologically large frontmatter block must be
        // rejected before it reaches `serde_norway::from_str`, since YAML parsing is not
        // linear-time on deeply nested input.
        let padding = "a".repeat(MAX_FRONTMATTER_SIZE + 1);
        let content = format!("---\nname: test\ndescription: {padding}\n---\n# Test");

        let result = extract_skill_metadata(&content);

        match result {
            Err(SkillMetadataError::FrontmatterTooLarge { size, limit }) => {
                assert!(size > MAX_FRONTMATTER_SIZE);
                assert_eq!(limit, MAX_FRONTMATTER_SIZE);
            }
            other => panic!("expected FrontmatterTooLarge, got: {other:?}"),
        }
    }

    #[test]
    fn test_extract_skill_metadata_null_name_rejected() {
        // Issue #203 follow-up (M4): `name:`/`name: null`/`name: ~` all deserialize to
        // `None`, same as an absent key, and must be rejected the same way.
        let content = "---\nname: ~\ndescription: test\n---\n# Test";

        let result = extract_skill_metadata(content);

        assert!(matches!(
            result,
            Err(SkillMetadataError::MissingField { field: "name" })
        ));
    }

    #[test]
    fn test_extract_skill_metadata_empty_string_name_rejected() {
        // Issue #203 follow-up (M4): an empty-string name/description is present but
        // useless, and must not silently reach `SkillMetadata`.
        let content = "---\nname: \"\"\ndescription: test\n---\n# Test";

        let result = extract_skill_metadata(content);

        assert!(matches!(
            result,
            Err(SkillMetadataError::MissingField { field: "name" })
        ));
    }

    #[test]
    fn test_extract_skill_metadata_with_extra_fields() {
        let content = r"---
name: test-skill
description: Test description
version: 1.0.0
author: Test Author
---

# Test
";

        let result = extract_skill_metadata(content);
        assert!(result.is_ok());

        let metadata = result.unwrap();
        assert_eq!(metadata.name, "test-skill");
        assert_eq!(metadata.description, "Test description");
    }

    #[test]
    fn test_extract_skill_metadata_block_literal_scalar() {
        // Issue #203 regression: a `description: |` block literal scalar must have its
        // real multi-line body captured, not just the `|` marker character.
        let content = r"---
name: test-skill
description: |
  This is a block literal description
  spanning multiple lines.
---

# Test
";

        let metadata = extract_skill_metadata(content).unwrap();
        assert_ne!(metadata.description, "|");
        assert!(metadata.description.contains("block literal description"));
        assert!(metadata.description.contains("spanning multiple lines."));
    }

    #[test]
    fn test_extract_skill_metadata_folded_block_scalar() {
        // Issue #203 regression: a `description: >` folded block scalar must have its
        // real multi-line body captured, not just the `>` marker character.
        let content = r"---
name: test-skill
description: >
  This is a folded description
  spanning multiple lines.
---

# Test
";

        let metadata = extract_skill_metadata(content).unwrap();
        assert_ne!(metadata.description, ">");
        assert!(metadata.description.contains("folded description"));
        assert!(metadata.description.contains("spanning multiple lines."));
    }

    #[test]
    fn test_extract_skill_metadata_quoted_scalars() {
        // Issue #203 regression: quote characters must be stripped by the YAML
        // parser, not captured verbatim into the field value.
        let content = r#"---
name: "quoted-name"
description: 'quoted text'
---

# Test
"#;

        let metadata = extract_skill_metadata(content).unwrap();
        assert_eq!(metadata.name, "quoted-name");
        assert_eq!(metadata.description, "quoted text");
    }

    /// Builds the flow-sequence body of an 8-anchor alias bomb shared by every alias-bomb test
    /// below: 8 anchors (`a0..a7`), each referencing the previous 8 times — 8^8 ~= 16.7M leaves
    /// if ever fully expanded. `preamble` is prepended verbatim; it decides which YAML key
    /// (declared or not) the bomb ends up under. At a few hundred bytes the result sits well
    /// under `MAX_FRONTMATTER_SIZE` (8 KiB); shrinking the branching factor, the anchor count,
    /// or the frontmatter cap changes that margin and must be re-checked against callers' size
    /// assertions.
    fn alias_bomb_fixture(preamble: &str) -> String {
        use std::fmt::Write as _;

        let mut frontmatter = String::from(preamble);
        writeln!(frontmatter, "  - &a0 [x, x, x, x, x, x, x, x]").unwrap();
        for level in 1..=7 {
            let prev = level - 1;
            let refs = (0..8)
                .map(|_| format!("*a{prev}"))
                .collect::<Vec<_>>()
                .join(", ");
            writeln!(frontmatter, "  - &a{level} [{refs}]").unwrap();
        }
        writeln!(frontmatter, "  - *a7").unwrap();
        frontmatter
    }

    #[test]
    fn test_extract_skill_metadata_alias_bomb_under_unknown_key_stays_ok() {
        // ADR-341 §3.3/§5, corrected per review: `RawFrontmatter` has no
        // `#[serde(deny_unknown_fields)]`, so a key it does not declare is discarded via a
        // lazy visitor that never expands nested aliases — the bomb below sits under such a
        // key (`unknown_key`), with `name`/`description` left valid, so parsing succeeds
        // fast today. A `#[serde(flatten)]`-style buffering field would force every unknown
        // key to be materialized into a `Content`-like structure before per-field routing,
        // which *does* expand aliases and trips serde_norway's own repetition-limit guard —
        // flipping this exact fixture's outcome from `Ok` to `Err`. That deterministic
        // outcome flip, not wall-clock timing, is the primary thing this test pins: a single
        // cold-process call's timing noise (measured up to ~1.6ms) is not a safe
        // discriminator against the ADR's ~4-7ms regression floor. An earlier version of
        // this test put the bomb on the *declared* `description` field instead, where
        // serde's derive reads it directly regardless of `flatten` — that fixture could
        // never detect the regression it claimed to guard, at any budget.
        //
        // Fixture shape: see `alias_bomb_fixture`'s doc comment.
        //
        // Deliberately out of scope here: retyping `description` itself to a buffering type
        // (`serde_norway::Value`, an untagged enum, a buffering `deserialize_with`) is a
        // distinct regression vector this fixture does not cover — see
        // `test_serde_norway_buffers_alias_bomb_when_declared_field_is_value_typed` and
        // `test_serde_norway_buffers_alias_bomb_when_declared_field_uses_deserialize_with`
        // below (issue #359).
        use std::time::{Duration, Instant};

        let frontmatter =
            alias_bomb_fixture("name: test-skill\ndescription: valid description\nunknown_key:\n");

        let content = format!("---\n{frontmatter}---\n# Test\n");
        assert!(
            content.len() <= MAX_FRONTMATTER_SIZE,
            "fixture must stay under the frontmatter cap to exercise the parser, not the size guard"
        );

        let start = Instant::now();
        let result = extract_skill_metadata(&content);
        let elapsed = start.elapsed();

        assert!(
            result.is_ok(),
            "expected Ok: an alias bomb under a key RawFrontmatter does not declare must be \
             ignored today without expansion; if this now errors, RawFrontmatter's field shape \
             likely changed (e.g. a #[serde(flatten)] field) and reopened the amplification path \
             serde_norway's own repetition-limit guard then catches at ms-scale cost instead of \
             being ignored at us-scale cost. got: {result:?}"
        );
        // Sanity bound only, not the detection mechanism (see comment above): guards against an
        // outright hang rather than the ms-scale regression, which the Ok/Err flip already
        // catches deterministically regardless of timing noise.
        assert!(
            elapsed < Duration::from_secs(1),
            "parse took {elapsed:?}, unexpectedly long even accounting for cold-process noise"
        );
    }

    #[test]
    fn test_serde_norway_buffers_alias_bomb_when_declared_field_is_value_typed() {
        // Issue #359 ("vector 2", sub-case A). The vector-1 test above pins the *unknown-key*
        // amplification path, which only opens up if `RawFrontmatter` grows a
        // `#[serde(flatten)]`-style field; it cannot detect a second, distinct regression: a
        // *declared* field (e.g. `description`) retyped from `Option<String>` to a buffering
        // type such as `serde_norway::Value` or an untagged enum. Buffering a declared field
        // forces serde to materialize its value before matching it against the target type,
        // which expands nested aliases and trips serde_norway's own repetition-limit guard.
        // Unlike vector 1, there is no `Ok` baseline here: deserializing a YAML sequence into
        // `Option<String>` always errs, buffering or not. The regression this pins is not
        // presence-vs-absence of an error but *which* error: a cheap, immediate type-mismatch
        // without buffering, versus this expensive repetition-limit trip once buffered.
        //
        // This is a canary on `serde_norway`'s own buffering behavior, not a regression test
        // against production `RawFrontmatter` (see its doc comment): that struct's
        // `description` is `Option<String>` today, so this exact retype is compile-blocked at
        // `require_field`'s `Option<String>` parameter, its `extract_skill_metadata` call site
        // — if attempted, the build fails before this test could ever run. It is kept to pin
        // the assumption the sibling `deserialize_with` test below relies on, since that one is
        // NOT compile-blocked.
        //
        // As in vector 1, the primary assertion is the deterministic outcome flip, not
        // wall-clock timing. The ~5.3ms release / ~61.7ms debug degradation figures quoted when
        // this vector was raised are from issue #359's own reproduction, not independently
        // re-verified against vendored sources the way ADR-341 §3.1-§3.6 were — illustrative
        // only, not a re-confirmed Evidence Ledger figure.
        use std::time::{Duration, Instant};

        #[derive(Debug, Deserialize)]
        #[allow(
            dead_code,
            reason = "fields exist only to mirror RawFrontmatter's shape"
        )]
        struct RawFrontmatterBufferedDescription {
            name: Option<String>,
            description: serde_norway::Value,
        }

        let frontmatter = alias_bomb_fixture("name: test-skill\ndescription:\n");
        // Unlike vector 1, this path never reaches `extract_skill_metadata`, so
        // `MAX_FRONTMATTER_SIZE` is not enforced here; this only records that the fixture has
        // headroom under that constant, for comparability with vector 1's fixture.
        assert!(
            frontmatter.len() <= MAX_FRONTMATTER_SIZE,
            "fixture unexpectedly exceeds MAX_FRONTMATTER_SIZE; re-check alias_bomb_fixture's margin"
        );

        let start = Instant::now();
        let result = serde_norway::from_str::<RawFrontmatterBufferedDescription>(&frontmatter);
        let elapsed = start.elapsed();

        let err = result.expect_err(
            "expected Err: buffering a declared field into serde_norway::Value forces alias \
             expansion before per-field routing, which should trip serde_norway's own \
             repetition-limit guard",
        );
        assert!(
            err.to_string().contains("repetition limit exceeded"),
            "expected serde_norway's own repetition-limit guard specifically, not some \
             unrelated error a future serde_norway version might introduce instead; got: {err}"
        );
        // Sanity bound only: `from_str` has already returned by the time `elapsed` is measured,
        // so this cannot catch a genuine hang (this workspace's nextest config sets no
        // slow-timeout/terminate-after). It only flags a completed-but-pathologically-slow
        // parse that the deterministic `Err` assertion above would not otherwise surface.
        assert!(
            elapsed < Duration::from_secs(1),
            "parse took {elapsed:?}, unexpectedly long even accounting for cold-process noise"
        );
    }

    #[test]
    fn test_serde_norway_buffers_alias_bomb_when_declared_field_uses_deserialize_with() {
        // Issue #359 ("vector 2", sub-case B — the genuinely silent one, per review). The
        // sibling test above pins the same amplification path for a field whose *type* changes
        // to `serde_norway::Value`, which is compile-blocked at `extract_skill_metadata`'s
        // `require_field` call site. This test pins the sub-case that is NOT compile-blocked: a
        // `#[serde(deserialize_with = ...)]` that buffers through `serde_norway::Value`
        // internally while keeping the field's declared Rust type as `Option<String>` — exactly
        // the shape `require_field` expects, so this variant would compile and could land
        // without any type-level signal.
        //
        // See `RawFrontmatter`'s doc comment for which future edits this canary (and its
        // sibling above) are meant to catch, and why both must be extended or replaced if
        // `RawFrontmatter` itself is ever changed in either direction.
        use std::time::{Duration, Instant};

        fn buffer_via_value<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>
        where
            D: serde::Deserializer<'de>,
        {
            let value = serde_norway::Value::deserialize(deserializer)?;
            Ok(value.as_str().map(str::to_string))
        }

        #[derive(Debug, Deserialize)]
        #[allow(
            dead_code,
            reason = "fields exist only to mirror RawFrontmatter's shape"
        )]
        struct RawFrontmatterDeserializeWithDescription {
            name: Option<String>,
            #[serde(deserialize_with = "buffer_via_value")]
            description: Option<String>,
        }

        let frontmatter = alias_bomb_fixture("name: test-skill\ndescription:\n");
        // See the sibling test above: this path also bypasses `extract_skill_metadata`, so
        // this only records headroom under `MAX_FRONTMATTER_SIZE`, not a production size guard.
        assert!(
            frontmatter.len() <= MAX_FRONTMATTER_SIZE,
            "fixture unexpectedly exceeds MAX_FRONTMATTER_SIZE; re-check alias_bomb_fixture's margin"
        );

        let start = Instant::now();
        let result =
            serde_norway::from_str::<RawFrontmatterDeserializeWithDescription>(&frontmatter);
        let elapsed = start.elapsed();

        let err = result.expect_err(
            "expected Err: a buffering deserialize_with should force alias expansion before \
             per-field routing just like an outright Value-typed field, even though the \
             declared Rust type here stays Option<String>",
        );
        assert!(
            err.to_string().contains("repetition limit exceeded"),
            "expected serde_norway's own repetition-limit guard specifically, not some \
             unrelated error a future serde_norway version might introduce instead; got: {err}"
        );
        // Sanity bound only (see the sibling test above for why this cannot catch a hang).
        assert!(
            elapsed < Duration::from_secs(1),
            "parse took {elapsed:?}, unexpectedly long even accounting for cold-process noise"
        );
    }

    #[test]
    fn test_extract_skill_metadata_alias_bomb_under_declared_field_short_circuits() {
        // Issue #359 ("vector 2", production-coupled canary). The two tests above pin the
        // buffering assumption against local, decoupled types; this one closes that gap for
        // real against production `RawFrontmatter` + `extract_skill_metadata`, the same way
        // vector 1 does for the unknown-key path. `describe_yaml_error` (used by
        // `extract_skill_metadata`) passes the underlying `serde_norway` error's `Display`
        // through into `SkillMetadataError::InvalidYaml(String)`, only rewriting its embedded
        // line/column offset — see `test_extract_skill_metadata_invalid_yaml` for existing
        // precedent asserting on that message — so the error text (including the
        // "repetition limit exceeded" substring this test checks for, untouched by that
        // rewrite) is directly inspectable here without a local test-only struct.
        //
        // Today `RawFrontmatter::description` is `Option<String>`, so deserializing the bomb
        // sequence into it short-circuits on an immediate type mismatch before any buffering
        // can happen — the error is NOT the repetition-limit guard. If `description` is ever
        // retyped to a buffering shape (see `RawFrontmatter`'s doc comment), this assertion
        // flips loudly: the error becomes "repetition limit exceeded", the same string the two
        // sibling tests above pin directly.
        let content = format!(
            "---\n{}---\n# Test\n",
            alias_bomb_fixture("name: test-skill\ndescription:\n")
        );
        assert!(
            content.len() <= MAX_FRONTMATTER_SIZE,
            "fixture must stay under the frontmatter cap to exercise the parser, not the size guard"
        );

        let result = extract_skill_metadata(&content);
        let Err(SkillMetadataError::InvalidYaml(message)) = &result else {
            panic!("expected InvalidYaml, got: {result:?}");
        };
        assert!(
            !message.contains("repetition limit exceeded"),
            "RawFrontmatter::description now trips serde_norway's repetition-limit guard on \
             this fixture, meaning it was retyped to a buffering shape (serde_norway::Value, an \
             untagged enum, or a buffering deserialize_with) without extending this test — see \
             RawFrontmatter's doc comment. got: {message:?}"
        );
    }
}