ryo_executor/executor/

blueprint_executor.rs

//! BlueprintExecutor: Execute ParallelBlueprint against AnalysisContext
//!
//! Uses MutationRegistry to delegate all MutationSpec → Mutation conversions
//! to specialized Converter implementations.
//!
//! ```text
//! ParallelBlueprint
//!    │
//!    ▼ execute()
//! BlueprintExecutor
//!    │
//!    ├── Strategy Selection (Sequential / Wavefront)
//!    ├── Topological sort by dependencies
//!    ├── Delegate to MutationRegistry.convert_and_apply()
//!    │      ├── RenameConverter
//!    │      ├── FieldConverter
//!    │      ├── AddItemConverter
//!    │      ├── IdiomConverter (13 idioms)
//!    │      ├── TraitConverter
//!    │      ├── MoveConverter
//!    │      ├── PluginConverter (WASM skeleton)
//!    │      └── ... (31 converters total)
//!    │
//!    ▼
//! BlueprintResult
//! ```
//!
//! ## Execution Strategies
//!
//! - **Sequential**: Execute mutations one by one (safe, debuggable)
//! - **Wavefront**: Execute independent mutations in parallel within each wave
//!
//! ```text
//! Wave 0: [Spec_A, Spec_B, Spec_C] → parallel compute → barrier → apply
//! Wave 1: [Spec_D]                 → parallel compute → barrier → apply
//! Wave 2: [Spec_E, Spec_F]         → parallel compute → barrier → apply
//! ```

use super::blueprint::ParallelBlueprint;
use super::registry::MutationRegistry;
use super::spec::{MutationSpec, MutationTargetSymbol};
use crate::engine::{collect_affected_ids, MutationEvent};
use ryo_analysis::{AnalysisContext, RegistryUpdateBatch, SymbolPath};
use ryo_source::pure::ToSynError;
use ryo_symbol::{MetadataError, WorkspaceFilePath};
use std::collections::HashSet;
use std::sync::Arc;
use tracing::{debug, info, instrument, warn};

/// Error during file sync after blueprint execution.
#[derive(Debug, thiserror::Error)]
pub enum SyncError {
    #[error("cargo metadata unavailable: {0}")]
    Metadata(#[from] MetadataError),
    #[error("source generation failed: {0}")]
    SourceGeneration(#[from] ToSynError),
}

/// Execution strategy for BlueprintExecutor
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum ExecutionStrategy {
    /// Execute mutations one by one (safe, debuggable)
    #[default]
    Sequential,

    /// Execute independent mutations in parallel within each wave
    /// Uses Collect-then-Apply pattern for thread safety
    Wavefront,
}

/// Suggest the best execution strategy based on blueprint characteristics
pub fn suggest_strategy(blueprint: &ParallelBlueprint) -> ExecutionStrategy {
    let parallelism = blueprint.parallelism();
    let mutation_count = blueprint.mutations.len();

    // Heuristics for strategy selection:
    // - Low parallelism (≤1.2) → Sequential (no benefit from parallel)
    // - Very few mutations (≤2) → Sequential (overhead not worth it)
    // - Otherwise → Wavefront
    if parallelism <= 1.2 || mutation_count <= 2 {
        ExecutionStrategy::Sequential
    } else {
        ExecutionStrategy::Wavefront
    }
}

/// Result of executing a blueprint
#[derive(Debug, Clone)]
pub struct BlueprintResult {
    /// Results for each mutation spec
    pub results: Vec<SpecResult>,

    /// Total changes across all mutations
    pub total_changes: usize,

    /// Files that were modified
    pub modified_files: Vec<WorkspaceFilePath>,

    /// Whether execution completed successfully
    pub success: bool,

    /// Error message if failed
    pub error: Option<String>,

    /// Registry updates to apply (for Context-centric design)
    ///
    /// These updates should be applied to the SymbolRegistry after
    /// execution to keep it in sync with the AST changes.
    pub registry_updates: RegistryUpdateBatch,
}

/// Result of executing a single MutationSpec
#[derive(Debug, Clone)]
pub struct SpecResult {
    /// Index in the blueprint
    pub index: usize,

    /// The spec that was executed
    pub spec_type: String,

    /// Number of changes made
    pub changes: usize,

    /// Files affected
    pub affected_files: Vec<WorkspaceFilePath>,

    /// Symbols affected (for history tracking)
    pub affected_symbols: Vec<SymbolPath>,

    /// Whether this spec succeeded
    pub success: bool,

    /// Error message if failed
    pub error: Option<String>,

    /// Registry updates from this spec
    pub registry_updates: RegistryUpdateBatch,

    /// Mutation events emitted during execution (for incremental updates)
    pub events: Vec<MutationEvent>,
}

impl BlueprintResult {
    pub fn success(results: Vec<SpecResult>, modified_files: Vec<WorkspaceFilePath>) -> Self {
        let total_changes = results.iter().map(|r| r.changes).sum();
        // Collect all registry updates from spec results
        let mut all_updates = RegistryUpdateBatch::new();
        for result in &results {
            for update in &result.registry_updates {
                all_updates.push(update.clone());
            }
        }
        Self {
            results,
            total_changes,
            modified_files,
            success: true,
            error: None,
            registry_updates: all_updates,
        }
    }

    pub fn failure(error: impl Into<String>) -> Self {
        Self {
            results: vec![],
            total_changes: 0,
            modified_files: vec![],
            success: false,
            error: Some(error.into()),
            registry_updates: RegistryUpdateBatch::new(),
        }
    }
}

/// Executor for ParallelBlueprint
#[derive(Debug)]
pub struct BlueprintExecutor {
    /// Registry for MutationSpec → Mutation conversion
    registry: MutationRegistry,

    /// Execution strategy
    pub strategy: ExecutionStrategy,

    /// Whether to verify compile after each mutation
    pub verify_after_each: bool,

    /// Whether to stop on first error
    pub stop_on_error: bool,

    /// Whether to ignore conflicts and process sequentially
    /// Default: true (conflicts are ignored, specs processed in order)
    pub ignore_conflicts: bool,
}

impl Default for BlueprintExecutor {
    fn default() -> Self {
        Self {
            registry: MutationRegistry::default(),
            strategy: ExecutionStrategy::default(),
            verify_after_each: false,
            stop_on_error: true,
            ignore_conflicts: true,
        }
    }
}

/// Convert a workspace-relative `WorkspaceFilePath` into the crate-relative key
/// expected by `RegistryGenerator::with_existing_files` (e.g. `"src/storage.rs"`).
///
/// Returns `None` when the metadata provider cannot resolve the crate's layout
/// or when the workspace-relative path does not live under the layout prefix.
///
/// Extracted from the inline closure inside [`BlueprintExecutor::sync_files_and_rebuild`]
/// so the hyphen-named-crate strip_prefix behaviour (Cargo name `ryo-app` ↔ module
/// name `ryo_app`) is independently testable.
pub(crate) fn ws_file_to_crate_relative_key<P: ryo_symbol::WorkspaceMetadataProvider>(
    wfp: &WorkspaceFilePath,
    metadata: &P,
) -> Option<String> {
    use ryo_symbol::CrateName;
    let crate_name = CrateName::new(wfp.crate_name().as_str()).ok()?;
    let layout = metadata.crate_layout(&crate_name)?;
    let ws_rel = wfp.as_relative();
    let prefix = layout.to_workspace_relative("");
    let prefix_path = prefix.as_path();
    let stripped = if prefix_path.as_os_str().is_empty() {
        ws_rel.to_path_buf()
    } else {
        ws_rel.strip_prefix(prefix_path).ok()?.to_path_buf()
    };
    stripped.to_str().map(str::to_string)
}

/// Convert a workspace-relative `WorkspaceFilePath` into a `(CrateName,
/// crate_relative_key)` pair used for per-crate `existing_file_keys`
/// construction inside [`BlueprintExecutor::sync_files_and_rebuild`].
///
/// Returns `None` under the same conditions as
/// [`ws_file_to_crate_relative_key`]. The returned `CrateName` is the same
/// `CrateName` used by `RegistryGenerator::with_existing_files_per_crate`
/// keying, so the per-crate isolation contract is preserved end-to-end.
pub(crate) fn ws_file_to_crate_pair<P: ryo_symbol::WorkspaceMetadataProvider>(
    wfp: &WorkspaceFilePath,
    metadata: &P,
) -> Option<(ryo_symbol::CrateName, String)> {
    use ryo_symbol::CrateName;
    let crate_name = CrateName::new(wfp.crate_name().as_str()).ok()?;
    let key = ws_file_to_crate_relative_key(wfp, metadata)?;
    Some((crate_name, key))
}

impl BlueprintExecutor {
    pub fn new() -> Self {
        Self::default()
    }

    // ========================================================================
    // V2 API: ASTRegistry-centric execution (new path)
    // ========================================================================

    /// Execute blueprint using ASTRegistry-centric path.
    ///
    /// This is the new execution path that:
    /// 1. Executes mutations directly on ASTRegistry (no file I/O)
    /// 2. Returns MutationEvents for incremental updates
    ///
    /// **Important**: This method only updates ASTRegistry. Callers must:
    /// - Call `ctx.sync_files_and_rebuild()` for full sync (files + graphs)
    /// - Or skip sync for lightweight precheck scenarios
    ///
    /// Unimplemented MutationSpecs will panic - this is intentional
    /// as we're building towards complete migration.
    #[instrument(skip(self, blueprint, ctx), fields(mutations = blueprint.mutations.len()))]
    pub fn execute_v2(
        &self,
        blueprint: &ParallelBlueprint,
        ctx: &mut AnalysisContext,
    ) -> BlueprintResult {
        debug!(
            "Starting blueprint execution with {} mutations",
            blueprint.mutations.len()
        );

        // Check for conflicts (skip if ignore_conflicts is true)
        if !self.ignore_conflicts && blueprint.needs_escalation() {
            warn!(
                "Blueprint has {} conflicts requiring escalation",
                blueprint.conflicts.len()
            );
            return BlueprintResult::failure(format!(
                "Blueprint has {} conflicts requiring escalation",
                blueprint.conflicts.len()
            ));
        }

        let mut results: Vec<SpecResult> = Vec::new();
        let mut completed: HashSet<usize> = HashSet::new();

        // Execute in topological order
        while completed.len() < blueprint.mutations.len() {
            let ready = blueprint
                .deps
                .ready_set(blueprint.mutations.len(), &completed);

            if ready.is_empty() {
                if completed.len() < blueprint.mutations.len() {
                    warn!("Dependency cycle detected");
                    return BlueprintResult::failure("Dependency cycle detected");
                }
                break;
            }

            debug!("Ready to execute {} specs", ready.len());

            for idx in ready {
                let spec = &blueprint.mutations[idx];
                debug!("Executing spec {}: {:?}", idx, spec);
                let result = self.execute_spec_v2(idx, spec, ctx);

                if let Some(ref error) = result.error {
                    warn!(
                        "Spec {} completed with error: success={}, changes={}, error={}",
                        idx, result.success, result.changes, error
                    );
                } else {
                    debug!(
                        "Spec {} completed: success={}, changes={}, events={}",
                        idx,
                        result.success,
                        result.changes,
                        result.events.len()
                    );
                }

                let success = result.success;
                results.push(result);
                completed.insert(idx);

                if !success && self.stop_on_error {
                    let total_changes = results.iter().map(|r| r.changes).sum();

                    // Get the error from the failed spec result
                    let spec_error = results
                        .last()
                        .and_then(|r| r.error.as_ref())
                        .map(|e| format!("Spec {} failed: {}", idx, e))
                        .unwrap_or_else(|| format!("Stopped at spec {} (no error message)", idx));

                    warn!("Stopping on error at spec {}: {}", idx, spec_error);

                    return BlueprintResult {
                        results,
                        total_changes,
                        modified_files: Vec::new(),
                        success: false,
                        error: Some(spec_error),
                        registry_updates: RegistryUpdateBatch::new(),
                    };
                }
            }
        }

        let total_changes: usize = results.iter().map(|r| r.changes).sum();
        info!(
            "Blueprint execution completed: {} results, {} total changes",
            results.len(),
            total_changes
        );

        BlueprintResult::success(results, Vec::new())
    }

    /// Synchronize files and rebuild analysis graphs after execute_v2.
    ///
    /// This method should be called after `execute_v2()` when you need:
    /// - Updated source files (for disk write or cargo check)
    /// - Updated analysis graphs (code_graph, typeflow, dataflow, detail_store)
    ///
    /// For lightweight precheck scenarios, skip this call entirely.
    ///
    /// # File Path Resolution
    ///
    /// This method handles the conversion from `RegistryGenerator`'s output
    /// (crate-relative paths like `"src/lib.rs"`) to workspace-relative paths
    /// (like `"crates/core/src/lib.rs"`).
    ///
    /// The conversion works as follows:
    ///
    /// 1. **Extract crate roots from existing files**: For each crate, find its
    ///    root directory by looking at existing `WorkspaceFilePath`s in `ctx.files`.
    ///    For example, `"crates/core/src/lib.rs"` → crate_root = `"crates/core"`.
    ///
    /// 2. **Combine crate_root + crate-relative path**: The generator outputs
    ///    `"src/lib.rs"`, and we prepend the crate_root to get the full
    ///    workspace-relative path: `"crates/core" + "src/lib.rs"` → `"crates/core/src/lib.rs"`.
    ///
    /// This approach keeps `RegistryGenerator` focused on pure SymbolPath-based
    /// generation, while the caller (this function) handles workspace layout concerns.
    ///
    /// # Arguments
    /// - `result`: The BlueprintResult from execute_v2
    /// - `ctx`: The AnalysisContext that was mutated
    ///
    /// # Returns
    /// List of modified file paths (as `WorkspaceFilePath`)
    #[instrument(skip(result, ctx), fields(total_changes = result.total_changes))]
    pub fn sync_files_and_rebuild(
        result: &BlueprintResult,
        ctx: &mut AnalysisContext,
    ) -> Result<Vec<WorkspaceFilePath>, SyncError> {
        use crate::engine::{collect_modified_symbols, RegistryGenerator};
        use ryo_symbol::CargoMetadataProvider;

        debug!(
            "Starting sync_files_and_rebuild with {} results",
            result.results.len()
        );

        // Step 1: Collect all events from execution results
        let all_events: Vec<MutationEvent> = result
            .results
            .iter()
            .flat_map(|r| r.events.clone())
            .collect();

        debug!(
            "Collected {} mutation events from results",
            all_events.len()
        );
        // INVARIANT: No mutation events → no files changed → nothing to sync.
        // This prevents the expensive generate path from running on 0-mutation results.
        if all_events.is_empty() {
            debug!("No mutation events — skipping file sync entirely");
            return Ok(Vec::new());
        }

        // Step 2: Determine modified files from events
        let modified_symbol_ids = collect_modified_symbols(&all_events, ctx.registry());
        debug!("Found {} modified symbols", modified_symbol_ids.len());
        // Step 3: Generator determines affected files and generates ONLY those files.
        //
        // DESIGN RULE: Full-workspace generation (generate/dump_all) is PROHIBITED here.
        // Only files containing modified symbols may be regenerated.
        // This is critical because:
        //   1. to_source() (prettyplease) is expensive per file
        //   2. Regenerating unmodified files causes format drift (vec![] → vec!(), shorthand expansion)
        //   3. Writing unmodified files wastes I/O and triggers unnecessary re-indexing
        let metadata = CargoMetadataProvider::from_directory(&ctx.workspace_root)?;

        // Collect existing crate-relative file paths **per crate** so the
        // generator preserves on-disk layout (`<mod>/mod.rs` vs flat `<mod>.rs`)
        // **without cross-crate contamination**.
        //
        // Pre-fix: a single workspace-wide HashSet caused
        // `ryo-executor/src/executor/mod.rs` to bleed into `ryo-agent`'s lookup
        // and emit a phantom `executor/mod.rs` next to that crate's existing
        // flat `executor.rs` — the RL061 mod-ambiguity bug captured by
        // `crates/ryo-app/tests/sync_phantom_mod_rs_probe.rs`. Per-crate
        // isolation keys the key set by `CrateName` so each crate sees only
        // its own layout.
        let mut existing_per_crate: std::collections::HashMap<
            ryo_symbol::CrateName,
            std::collections::HashSet<String>,
        > = std::collections::HashMap::new();
        for wfp in ctx.files().keys() {
            if let Some((crate_name, key)) = ws_file_to_crate_pair(wfp, &metadata) {
                existing_per_crate
                    .entry(crate_name)
                    .or_default()
                    .insert(key);
            }
        }

        // R2 fix: derive the set of crates that own a `src/lib.rs` on
        // disk straight from the per-crate existing-file map. The
        // generator uses this to decline emitting a lib-style root for
        // binary-only crates (= those NOT in this set when their
        // symbols carry no `main::` prefix). The previous sync-side
        // defensive guard from `f42eb504` is replaced by this pre-flight
        // filter so the phantom `src/lib.rs` is never generated.
        let lib_crates: std::collections::HashSet<ryo_symbol::CrateName> = existing_per_crate
            .iter()
            .filter_map(|(crate_name, keys)| {
                if keys.contains("src/lib.rs") {
                    Some(crate_name.clone())
                } else {
                    None
                }
            })
            .collect();

        let generator = RegistryGenerator::multi_file()
            .with_existing_files_per_crate(existing_per_crate)
            .with_lib_crates(lib_crates);
        let workspace = generator.generate_affected(
            &ctx.ast_registry,
            ctx.registry(),
            &modified_symbol_ids,
            &metadata,
        )?;

        debug!(
            "Generator produced {} crates with {} total files",
            workspace.crates.len(),
            workspace.total_files()
        );

        // Step 4: Convert generated files to WorkspaceFilePath and update context
        // Generator returns crate-relative paths (e.g., "src/lib.rs")
        // We need to resolve these to workspace-relative paths using CrateLayout
        let mut modified_files = Vec::new();

        for generated_crate in workspace.crates.values() {
            debug!(
                "Processing crate {} with {} files",
                generated_crate.crate_name,
                generated_crate.files.len()
            );

            // Get CrateLayout for this crate to convert paths correctly
            use ryo_symbol::{CrateName, WorkspacePathResolver};
            let crate_name = CrateName::new(&generated_crate.crate_name).expect(
                "generator-emitted crate names must already satisfy CrateName validation; \
                 reaching this expect means the generator is producing invalid names",
            );
            let layout = metadata.crate_layout(&crate_name);

            for (crate_relative_path, generated_file) in &generated_crate.files {
                // (R2: the defensive `src/lib.rs` skip guard that used to
                // live here was replaced by the `RegistryGenerator::with_lib_crates`
                // pre-flight filter above — the generator simply never
                // emits a lib-style root for binary-only crates, so the
                // post-hoc guard became dead code.)

                // Convert crate-relative path to workspace-relative path using CrateLayout
                let workspace_relative = match &layout {
                    Some(layout) => layout.to_workspace_relative(crate_relative_path),
                    None => {
                        // Fallback: use crate-relative path as-is (single-crate workspace)
                        std::path::PathBuf::from(crate_relative_path.as_str())
                    }
                };

                // Find corresponding WorkspaceFilePath from context
                // Match by crate_name and workspace-relative path
                let workspace_file = ctx
                    .files()
                    .keys()
                    .find(|wfp| {
                        wfp.crate_name().as_str() == generated_crate.crate_name
                            && wfp.as_relative() == workspace_relative.as_path()
                    })
                    .cloned();

                let wfp = if let Some(existing) = workspace_file {
                    debug!(
                        "Updating existing file: {} ({})",
                        crate_relative_path,
                        existing.as_relative().display()
                    );
                    existing
                } else {
                    // New file: construct WorkspaceFilePath with correct workspace-relative path
                    debug!(
                        "Creating new file: {} -> {} for crate {}",
                        crate_relative_path,
                        workspace_relative.display(),
                        generated_crate.crate_name
                    );
                    let resolver =
                        WorkspacePathResolver::new(ctx.workspace_root.as_ref().to_path_buf());
                    resolver.resolve_relative_with_crate(&workspace_relative, crate_name.clone())
                };

                let parsed = ryo_source::pure::PureFile::from_source(&generated_file.source);
                let pure_file = parsed.unwrap_or_else(|_e| ryo_source::pure::PureFile::new());
                ctx.files_mut().insert(wfp.clone(), Arc::new(pure_file));
                modified_files.push(wfp);
            }
        }

        debug!(
            "Updated {} files in context from generator output",
            modified_files.len()
        );

        // Step 5: Rebuild analysis graphs using symbol-based incremental update
        if !all_events.is_empty() {
            let affected_ids = collect_affected_ids(&all_events, ctx.registry());
            debug!("Rebuilding with {} affected symbol IDs", affected_ids.len());
            ctx.rebuild_after_mutation_by_symbols(&affected_ids);
        } else if !modified_files.is_empty() {
            // Fallback to file-based update if no events were collected
            debug!(
                "Rebuilding with {} modified files (fallback)",
                modified_files.len()
            );
            ctx.rebuild_after_mutation(&modified_files);
        }

        info!(
            "sync_files_and_rebuild completed: {} files modified",
            modified_files.len()
        );
        Ok(modified_files)
    }

    /// Execute a single MutationSpec via ASTRegistry path.
    ///
    /// Uses convert_v2() when available, falls back to direct construction
    /// for specs that haven't been migrated yet.
    fn execute_spec_v2(
        &self,
        index: usize,
        spec: &MutationSpec,
        ctx: &mut AnalysisContext,
    ) -> SpecResult {
        use crate::engine::ASTMutationEngine;

        let spec_type = spec_type_name(spec);
        // Compute affected_symbols from spec targets
        let affected_symbols: Vec<SymbolPath> = spec
            .get_targets()
            .iter()
            .filter_map(|target| target.to_path(ctx.registry()))
            .collect();

        // Try V2 path first (convert_v2 → execute_ast_reg_batch_dyn)
        match self.registry.convert_v2(spec, ctx) {
            Ok(mutations) => {
                let exec_result = ASTMutationEngine::execute_ast_reg_batch_dyn(mutations, ctx);

                // For AddItem: register the newly added symbol for deferred resolution
                // This allows later intents (AddDerive, AddVariant) to reference it by name
                if let MutationSpec::AddItem {
                    target, content, ..
                } = spec
                {
                    // Only register if target is already resolved to a SymbolPath
                    if let MutationTargetSymbol::ByPath(path) = target {
                        register_item_from_content(ctx, path, content);
                    } else if let MutationTargetSymbol::ById(id) = target {
                        // Resolve SymbolId to SymbolPath and clone it
                        if let Some(path) = ctx.registry().resolve(*id).cloned() {
                            register_item_from_content(ctx, &path, content);
                        }
                    }
                }

                SpecResult {
                    index,
                    spec_type,
                    success: true,
                    changes: exec_result.result.changes,
                    affected_files: vec![], // TODO: Extract from events
                    affected_symbols,
                    error: None,
                    registry_updates: RegistryUpdateBatch::new(),
                    events: exec_result.events,
                }
            }
            Err(e) => SpecResult {
                index,
                spec_type,
                success: false,
                changes: 0,
                affected_files: vec![],
                affected_symbols,
                error: Some(format!("convert_v2 failed: {}", e)),
                registry_updates: RegistryUpdateBatch::new(),
                events: vec![],
            },
        }
    }

    /// Set execution strategy
    pub fn with_strategy(mut self, strategy: ExecutionStrategy) -> Self {
        self.strategy = strategy;
        self
    }

    /// Enable compile verification after each mutation
    pub fn with_verify(mut self, verify: bool) -> Self {
        self.verify_after_each = verify;
        self
    }

    /// Stop execution on first error
    pub fn with_stop_on_error(mut self, stop: bool) -> Self {
        self.stop_on_error = stop;
        self
    }
}

/// Register a symbol from AddItem content for deferred resolution.
///
/// This allows later intents in a batch (e.g., AddDerive, AddVariant) to reference
/// symbols created by earlier AddItem intents.
fn register_item_from_content(
    ctx: &mut AnalysisContext,
    target: &ryo_symbol::SymbolPath,
    content: &str,
) {
    use ryo_symbol::SymbolKind;

    let trimmed = content.trim();

    // Skip attributes and find the item declaration
    let mut lines = trimmed.lines();
    let mut decl_line = "";
    for line in lines.by_ref() {
        let line = line.trim();
        if !line.starts_with('#') && !line.starts_with("//") && !line.is_empty() {
            decl_line = line;
            break;
        }
    }

    // Parse: pub? (struct|enum|fn|type|const|static|trait|mod) NAME
    let tokens: Vec<&str> = decl_line.split_whitespace().collect();
    if tokens.is_empty() {
        return;
    }

    let mut idx = 0;

    // Skip visibility
    if tokens.get(idx) == Some(&"pub") {
        idx += 1;
        // Skip pub(crate), pub(super), etc.
        if let Some(t) = tokens.get(idx) {
            if t.starts_with('(') {
                idx += 1;
            }
        }
    }

    // Get keyword
    let Some(keyword) = tokens.get(idx) else {
        return;
    };
    idx += 1;

    let kind = match *keyword {
        "struct" => SymbolKind::Struct,
        "enum" => SymbolKind::Enum,
        "fn" => SymbolKind::Function,
        "type" => SymbolKind::TypeAlias,
        "const" => SymbolKind::Const,
        "static" => SymbolKind::Static,
        "trait" => SymbolKind::Trait,
        "mod" => SymbolKind::Mod,
        "impl" => return, // Skip impl blocks - they don't create new symbols
        _ => return,
    };

    // Get name (may include generics like "Foo<T>")
    let Some(name_token) = tokens.get(idx) else {
        return;
    };
    let name = name_token
        .split(['<', '{', '(', ':'])
        .next()
        .unwrap_or(name_token);

    // Build full symbol path
    let target_str = target.to_string();
    let is_crate_root = target_str == "crate";
    let full_path = if is_crate_root {
        ryo_symbol::SymbolPath::parse(&format!("crate::{}", name))
    } else {
        ryo_symbol::SymbolPath::parse(&format!("{}::{}", target_str, name))
    };

    let Ok(path) = full_path else {
        return;
    };

    // Register in registry (ignore errors - symbol may already exist)
    let _ = ctx.registry_mut().register(path, kind);
}

/// Get a short type name for a MutationSpec
fn spec_type_name(spec: &MutationSpec) -> String {
    match spec {
        MutationSpec::Rename { .. } => "Rename".to_string(),
        MutationSpec::AddField { .. } => "AddField".to_string(),
        MutationSpec::RemoveField { .. } => "RemoveField".to_string(),
        MutationSpec::ChangeVisibility { .. } => "ChangeVisibility".to_string(),
        MutationSpec::AddDerive { .. } => "AddDerive".to_string(),
        MutationSpec::RemoveDerive { .. } => "RemoveDerive".to_string(),
        MutationSpec::AddVariant { .. } => "AddVariant".to_string(),
        MutationSpec::RemoveVariant { .. } => "RemoveVariant".to_string(),
        MutationSpec::AddMatchArm { .. } => "AddMatchArm".to_string(),
        MutationSpec::RemoveMatchArm { .. } => "RemoveMatchArm".to_string(),
        MutationSpec::ReplaceMatchArm { .. } => "ReplaceMatchArm".to_string(),
        MutationSpec::AddStructLiteralField { .. } => "AddStructLiteralField".to_string(),
        MutationSpec::RemoveStructLiteralField { .. } => "RemoveStructLiteralField".to_string(),
        MutationSpec::AddItem { .. } => "AddItem".to_string(),
        MutationSpec::RemoveItem { .. } => "RemoveItem".to_string(),
        MutationSpec::ReplaceCode { .. } => "ReplaceCode".to_string(),
        MutationSpec::AddMethod { .. } => "AddMethod".to_string(),
        MutationSpec::RemoveMethod { .. } => "RemoveMethod".to_string(),
        MutationSpec::RemoveMod { .. } => "RemoveMod".to_string(),
        MutationSpec::CreateMod { .. } => "CreateMod".to_string(),
        MutationSpec::OrganizeImports { .. } => "OrganizeImports".to_string(),
        MutationSpec::LoopToIterator { .. } => "LoopToIterator".to_string(),
        MutationSpec::UnwrapToQuestion { .. } => "UnwrapToQuestion".to_string(),
        MutationSpec::AddSpec { .. } => "AddSpec".to_string(),
        MutationSpec::RemoveSpec { .. } => "RemoveSpec".to_string(),
        MutationSpec::ValidateSpec { .. } => "ValidateSpec".to_string(),
        MutationSpec::ExtractTrait { .. } => "ExtractTrait".to_string(),
        MutationSpec::InlineTrait { .. } => "InlineTrait".to_string(),
        MutationSpec::ReplaceType { .. } => "ReplaceType".to_string(),
        MutationSpec::EnumToTrait { .. } => "EnumToTrait".to_string(),
        MutationSpec::MoveItem { .. } => "MoveItem".to_string(),
        MutationSpec::AssignOp { .. } => "AssignOp".to_string(),
        MutationSpec::BoolSimplify { .. } => "BoolSimplify".to_string(),
        MutationSpec::CloneOnCopy { .. } => "CloneOnCopy".to_string(),
        MutationSpec::CollapsibleIf { .. } => "CollapsibleIf".to_string(),
        MutationSpec::NoOpArmToTodo { .. } => "NoOpArmToTodo".to_string(),
        MutationSpec::ComparisonToMethod { .. } => "ComparisonToMethod".to_string(),
        MutationSpec::RedundantClosure { .. } => "RedundantClosure".to_string(),
        MutationSpec::IntroduceVariable { .. } => "IntroduceVariable".to_string(),
        MutationSpec::ManualMap { .. } => "ManualMap".to_string(),
        MutationSpec::MatchToIfLet { .. } => "MatchToIfLet".to_string(),
        MutationSpec::FilterNext { .. } => "FilterNext".to_string(),
        MutationSpec::MapUnwrapOr { .. } => "MapUnwrapOr".to_string(),
        MutationSpec::ReplaceExpr { .. } => "ReplaceExpr".to_string(),
        MutationSpec::RemoveStatement { .. } => "RemoveStatement".to_string(),
        MutationSpec::InsertStatement { .. } => "InsertStatement".to_string(),
        MutationSpec::ReplaceStatement { .. } => "ReplaceStatement".to_string(),
        MutationSpec::PluginTransform { .. } => "PluginTransform".to_string(),
        MutationSpec::DuplicateFunction { .. } => "DuplicateFunction".to_string(),
        MutationSpec::DuplicateStruct { .. } => "DuplicateStruct".to_string(),
        MutationSpec::DuplicateEnum { .. } => "DuplicateEnum".to_string(),
        MutationSpec::DuplicateModTree { .. } => "DuplicateModTree".to_string(),
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use crate::executor::spec::{Scope, SelfParam, SymbolPath, Visibility};
    use ryo_analysis::testing::{ContextBuilder, ContextTestExt};
    use ryo_symbol::SymbolId;

    /// Create a dummy SymbolId for testing
    fn dummy_id(index: u32) -> SymbolId {
        SymbolId::parse(&format!("{}v1", index)).expect("valid dummy id")
    }

    /// Helper function that executes a blueprint and syncs files.
    /// This is the standard pattern for tests that need to read from ctx.files() after execution.
    fn execute_and_sync(
        executor: &BlueprintExecutor,
        blueprint: &ParallelBlueprint,
        ctx: &mut AnalysisContext,
    ) -> BlueprintResult {
        let result = executor.execute_v2(blueprint, ctx);
        if result.success {
            BlueprintExecutor::sync_files_and_rebuild(&result, ctx).unwrap();
        }
        result
    }

    fn create_test_context() -> AnalysisContext {
        let code = r#"
struct Config {
    name: String,
}

impl Config {
    fn new() -> Self {
        Self { name: String::new() }
    }
}
"#;
        ContextBuilder::new()
            .with_file("src/config.rs", code)
            .build()
    }

    #[test]
    fn test_blueprint_executor_rename() {
        let mut ctx = create_test_context();

        // Look up the symbol_id for Config from the context's registry
        let symbol_id = ctx
            .registry()
            .lookup_by_name("Config")
            .expect("Config should exist in registry");

        let specs = vec![MutationSpec::Rename {
            target: MutationTargetSymbol::ById(symbol_id),
            to: "AppConfig".to_string(),
            scope: Scope::Project,
        }];
        let blueprint = ParallelBlueprint::from_mutations(specs);

        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(exec_result.success);
        assert!(exec_result.total_changes > 0);

        // Verify the rename happened
        let file = ctx.test_file("src/config.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("AppConfig"));
        assert!(!source.contains("struct Config"));
    }

    #[test]
    fn test_blueprint_executor_add_derive() {
        let mut ctx = create_test_context();

        // Lookup the actual SymbolId (file is src/config.rs, so path is crate::config::Config)
        let path = SymbolPath::parse("test_crate::config::Config").unwrap();
        let symbol_id = ctx.registry().lookup(&path).expect("Config should exist");

        let specs = vec![MutationSpec::AddDerive {
            target: MutationTargetSymbol::ById(symbol_id),
            derives: vec!["Debug".to_string(), "Clone".to_string()],
        }];
        let blueprint = ParallelBlueprint::from_mutations(specs);

        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(exec_result.success);

        let file = ctx.test_file("src/config.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("derive"), "Expected derive in: {}", source);
        assert!(source.contains("Debug"));
    }

    #[test]
    fn test_blueprint_executor_organize_imports() {
        let code = r#"
use std::collections::HashMap;
use std::io::Write;
use std::collections::HashSet;
use std::io::Read;

fn main() {}
"#;
        let mut ctx = ContextBuilder::new().with_file("src/main.rs", code).build();

        let specs = vec![MutationSpec::OrganizeImports {
            module_id: None,
            deduplicate: true,
            merge_groups: true,
        }];
        let blueprint = ParallelBlueprint::from_mutations(specs);

        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(exec_result.success);
    }

    #[test]
    fn test_blueprint_with_conflicts_fails_when_not_ignored() {
        use ryo_symbol::{SymbolKind, SymbolPath, SymbolRegistry};

        let mut ctx = create_test_context();

        // Create dummy symbol IDs for testing actual conflicts
        let mut symbol_registry = SymbolRegistry::new();
        let path_a = SymbolPath::parse("test_crate::A").unwrap();
        let symbol_a = symbol_registry
            .register(path_a, SymbolKind::Struct)
            .unwrap();

        // Create blueprint with actual conflicts (same target renamed twice)
        let specs = vec![
            MutationSpec::Rename {
                target: MutationTargetSymbol::ById(symbol_a),
                to: "B".to_string(),
                scope: Scope::Project,
            },
            MutationSpec::Rename {
                target: MutationTargetSymbol::ById(symbol_a),
                to: "C".to_string(),
                scope: Scope::Project,
            },
        ];

        let blueprint = ParallelBlueprint::from_mutations(specs);

        // Verify blueprint has conflicts
        assert!(
            !blueprint.conflicts.is_empty(),
            "Blueprint should have conflicts"
        );

        // With ignore_conflicts = false, conflicts should fail
        let mut executor = BlueprintExecutor::new();
        executor.ignore_conflicts = false;
        let result = execute_and_sync(&executor, &blueprint, &mut ctx);

        // Should fail because of conflicts
        assert!(
            !result.success,
            "Execution should fail when conflicts are not ignored"
        );
        assert!(result.error.is_some(), "Should have error message");
        assert!(
            result.error.unwrap().contains("conflict"),
            "Error should mention conflicts"
        );
    }

    // Note: Intent-based tests have been removed as they use deprecated ryo_core::Intent.
    // Converter-level tests in registry/converters/ provide comprehensive coverage.
    // TODO: Add MutationSpec-based integration tests if needed.

    // === AddItem Tests ===

    #[test]
    fn test_blueprint_executor_add_item_struct() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// empty file\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "pub struct Config {}".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem struct failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("pub struct Config"),
            "Struct not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_item_fn() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// empty file\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "fn helper() {}".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem fn failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("fn helper"),
            "Function not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_item_use() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "pub struct Dummy;\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "use HashMap;".to_string(),
            position: super::super::spec::InsertPosition::Top,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem use failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("use HashMap"), "Use not added: {}", source);
    }

    #[test]
    fn test_blueprint_executor_add_item_impl() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "struct Foo {}\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "impl Foo {}".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem impl failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("impl Foo"), "Impl not added: {}", source);
    }

    #[test]
    fn test_blueprint_executor_add_item_enum() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// empty file\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "pub enum Status { Pending, Active, Completed }".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem enum failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("pub enum Status"),
            "Enum not added: {}",
            source
        );
    }

    // === AddMethod Tests ===

    #[test]
    fn test_blueprint_executor_add_method_basic() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "pub struct Config {}\n\nimpl Config {}\n")
            .build();

        let spec = MutationSpec::AddMethod {
            target: MutationTargetSymbol::ByKindAndName(
                crate::executor::ItemKind::Impl,
                "Config".to_string(),
            ),
            method_name: "new".to_string(),
            params: vec![],
            return_type: Some("Self".to_string()),
            body: "Self {}".to_string(),
            is_pub: true,
            self_param: None,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddMethod basic failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("pub fn new"),
            "Method not added: {}",
            source
        );
        assert!(
            source.contains("-> Self"),
            "Return type not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_method_with_self() {
        let mut ctx = ContextBuilder::new()
            .with_file(
                "src/lib.rs",
                "pub struct Counter { value: u32 }\n\nimpl Counter {}\n",
            )
            .build();

        let spec = MutationSpec::AddMethod {
            target: MutationTargetSymbol::ByKindAndName(
                crate::executor::ItemKind::Impl,
                "Counter".to_string(),
            ),
            method_name: "get".to_string(),
            params: vec![],
            return_type: Some("u32".to_string()),
            body: "self.value".to_string(),
            is_pub: true,
            self_param: Some(SelfParam::Ref),
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddMethod with_self failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("&self"), "Self param not added: {}", source);
        assert!(source.contains("fn get"), "Method not added: {}", source);
    }

    #[test]
    fn test_blueprint_executor_add_method_with_mut_self() {
        let mut ctx = ContextBuilder::new()
            .with_file(
                "src/lib.rs",
                "pub struct Counter { value: u32 }\n\nimpl Counter {}\n",
            )
            .build();

        let spec = MutationSpec::AddMethod {
            target: MutationTargetSymbol::ByKindAndName(
                crate::executor::ItemKind::Impl,
                "Counter".to_string(),
            ),
            method_name: "increment".to_string(),
            params: vec![],
            return_type: None,
            body: "self.value += 1".to_string(),
            is_pub: true,
            self_param: Some(SelfParam::Mut),
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddMethod with_mut_self failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("&mut self"),
            "Mut self param not added: {}",
            source
        );
        assert!(
            source.contains("fn increment"),
            "Method not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_method_with_params() {
        let mut ctx = ContextBuilder::new()
            .with_file(
                "src/lib.rs",
                "pub struct Calculator {}\n\nimpl Calculator {}\n",
            )
            .build();

        let spec = MutationSpec::AddMethod {
            target: MutationTargetSymbol::ByKindAndName(
                crate::executor::ItemKind::Impl,
                "Calculator".to_string(),
            ),
            method_name: "add".to_string(),
            params: vec![
                ("a".to_string(), "i32".to_string()),
                ("b".to_string(), "i32".to_string()),
            ],
            return_type: Some("i32".to_string()),
            body: "a + b".to_string(),
            is_pub: true,
            self_param: None,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddMethod with_params failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("fn add"), "Method not added: {}", source);
        assert!(source.contains("a: i32"), "Param a not added: {}", source);
        assert!(source.contains("b: i32"), "Param b not added: {}", source);
    }

    // === Multi-file Tests ===

    #[test]
    fn test_blueprint_executor_multi_file_rename() {
        let mut ctx = ContextBuilder::new()
            .with_file(
                "src/lib.rs",
                r#"mod models;

fn process(task: Task) -> Task {
    task
}
"#,
            )
            .with_file(
                "src/models.rs",
                r#"pub struct Task {
    pub id: u32,
    pub name: String,
}
"#,
            )
            .build();

        // Look up the symbol_id for Task from the context's registry
        let symbol_id = ctx
            .registry()
            .lookup_by_name("Task")
            .expect("Task should exist in registry");

        // Rename Task to TodoItem across all files
        let spec = MutationSpec::Rename {
            target: MutationTargetSymbol::ById(symbol_id),
            to: "TodoItem".to_string(),
            scope: Scope::Project,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Multi-file rename failed: {:?}",
            exec_result.error
        );

        // Check lib.rs type annotations were renamed
        let lib = ctx.test_file("src/lib.rs").unwrap();
        let lib_source = lib.to_source().unwrap();
        assert!(
            lib_source.contains("task: TodoItem"),
            "lib.rs type not renamed: {}",
            lib_source
        );
        assert!(
            lib_source.contains("-> TodoItem"),
            "lib.rs return type not renamed: {}",
            lib_source
        );

        // Check models.rs struct was renamed
        let models = ctx.test_file("src/models.rs").unwrap();
        let models_source = models.to_source().unwrap();
        assert!(
            models_source.contains("struct TodoItem"),
            "models.rs struct not renamed: {}",
            models_source
        );
        assert!(
            !models_source.contains("struct Task"),
            "models.rs still has struct Task: {}",
            models_source
        );
    }

    #[test]
    fn test_blueprint_executor_multi_file_add_items() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "pub mod models;\n")
            .with_file("src/models.rs", "pub struct Placeholder;\n")
            .build();

        // Add struct to models.rs
        let spec1 = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate::models").unwrap(),
            )),
            content: "pub struct User { id: u32 }".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        // Add use to lib.rs
        let spec2 = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "use models::User;".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec1, spec2]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Multi-file add items failed: {:?}",
            exec_result.error
        );

        // Check models.rs has User struct
        let models = ctx.test_file("src/models.rs").unwrap();
        let models_source = models.to_source().unwrap();
        assert!(
            models_source.contains("pub struct User"),
            "User not added to models.rs: {}",
            models_source
        );

        // Check lib.rs has use statement
        let lib = ctx.test_file("src/lib.rs").unwrap();
        let lib_source = lib.to_source().unwrap();
        assert!(
            lib_source.contains("use models::User"),
            "Use not added to lib.rs: {}",
            lib_source
        );
    }

    // === Generic and Async Tests ===

    #[test]
    fn test_blueprint_executor_add_item_generic_struct() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// empty file\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "pub struct Container<T> { value: T }".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem generic struct failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("struct Container"),
            "Generic struct not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_item_async_fn() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// empty file\n")
            .build();

        let spec = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "pub async fn fetch_data() -> String { String::new() }".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddItem async fn failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("fn fetch_data"),
            "Async fn not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_field_to_generic_struct() {
        use ryo_analysis::SymbolKind;

        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "pub struct Wrapper<T> { inner: T }\n")
            .build();

        // Get symbol_id for Wrapper struct
        let symbol_id = ctx
            .registry
            .iter()
            .find(|(id, path)| {
                path.name() == "Wrapper" && ctx.registry.kind(*id) == Some(SymbolKind::Struct)
            })
            .map(|(id, _)| id)
            .expect("Wrapper struct not found in registry");

        let spec = MutationSpec::AddField {
            target: MutationTargetSymbol::ById(symbol_id),
            field_name: "count".to_string(),
            field_type: "usize".to_string(),
            visibility: Visibility::Pub,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddField to generic struct failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("pub count: usize"),
            "Field not added to generic struct: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_add_derive_to_generic_struct() {
        let mut ctx = ContextBuilder::new()
            .with_file(
                "src/lib.rs",
                "pub struct Pair<T, U> { first: T, second: U }\n",
            )
            .build();

        // Lookup the actual SymbolId
        let path = SymbolPath::parse("test_crate::Pair").unwrap();
        let symbol_id = ctx.registry().lookup(&path).expect("Pair should exist");

        let spec = MutationSpec::AddDerive {
            target: MutationTargetSymbol::ById(symbol_id),
            derives: vec!["Debug".to_string(), "Clone".to_string()],
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "AddDerive to generic struct failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("Debug"),
            "Debug derive not added: {}",
            source
        );
        assert!(
            source.contains("Clone"),
            "Clone derive not added: {}",
            source
        );
    }

    // === Module Tests ===
    // Note: AddMod was consolidated into CreateMod. These tests use CreateMod with empty content
    // to test the mod declaration functionality.

    #[test]
    fn test_blueprint_executor_create_mod_declaration() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "use std::io;\n\nfn main() {}\n")
            .build();

        let spec = MutationSpec::CreateMod {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            mod_name: "models".to_string(),
            content: String::new(),
            is_pub: false,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "CreateMod failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("mod models;"), "Mod not added: {}", source);
    }

    #[test]
    fn test_blueprint_executor_create_pub_mod_declaration() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "fn main() {}\n")
            .build();

        let spec = MutationSpec::CreateMod {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            mod_name: "api".to_string(),
            content: String::new(),
            is_pub: true,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "CreateMod pub failed: {:?}",
            exec_result.error
        );

        let file = ctx.test_file("src/lib.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("pub mod api;"),
            "Pub mod not added: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_create_file() {
        // NOTE: Don't pre-declare "mod models;" - CreateMod will add both the declaration and content
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "// lib.rs\n")
            .build();

        let spec = MutationSpec::CreateMod {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            mod_name: "models".to_string(),
            content: "pub struct Model { id: u32 }".to_string(),
            is_pub: true,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "CreateFile failed: {:?}",
            exec_result.error
        );

        // Check file was created (file style: src/models.rs - Ryo uses file-style modules by default)
        assert!(ctx.test_file("src/models.rs").is_some(), "File not created");

        let file = ctx.test_file("src/models.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(
            source.contains("struct Model"),
            "Content not correct: {}",
            source
        );
    }

    #[test]
    fn test_blueprint_executor_create_module_workflow() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "fn main() {}\n")
            .build();

        // CreateMod handles both mod declaration and file creation
        let spec = MutationSpec::CreateMod {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            mod_name: "utils".to_string(),
            content: "pub fn helper() {}".to_string(),
            is_pub: true,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new();
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Module workflow failed: {:?}",
            exec_result.error
        );

        // Check lib.rs has mod declaration
        let lib = ctx.test_file("src/lib.rs").unwrap();
        let lib_source = lib.to_source().unwrap();
        assert!(
            lib_source.contains("pub mod utils;"),
            "Mod not added to lib: {}",
            lib_source
        );

        // Check utils file exists and has function (file style: src/utils.rs)
        let utils = ctx.test_file("src/utils.rs").unwrap();
        let utils_source = utils.to_source().unwrap();
        assert!(
            utils_source.contains("fn helper"),
            "Function not in utils: {}",
            utils_source
        );
    }

    // === Wavefront Execution Tests ===

    #[test]
    fn test_wavefront_execution_basic() {
        let mut ctx = create_test_context();

        // Lookup the actual SymbolId (file is src/config.rs, so path is crate::config::Config)
        let path = SymbolPath::parse("test_crate::config::Config").unwrap();
        let symbol_id = ctx.registry().lookup(&path).expect("Config should exist");

        let specs = vec![MutationSpec::AddDerive {
            target: MutationTargetSymbol::ById(symbol_id),
            derives: vec!["Debug".to_string()],
        }];
        let blueprint = ParallelBlueprint::from_mutations(specs);

        let executor = BlueprintExecutor::new().with_strategy(ExecutionStrategy::Wavefront);
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Wavefront basic failed: {:?}",
            exec_result.error
        );
        assert!(exec_result.total_changes > 0);

        let file = ctx.test_file("src/config.rs").unwrap();
        let source = file.to_source().unwrap();
        assert!(source.contains("Debug"), "Derive not added: {}", source);
    }

    #[test]
    fn test_wavefront_execution_multi_file() {
        // Production-realistic fixture: the parent file (`src/lib.rs`)
        // must declare its child module (`pub mod models;`) so the
        // analysis context picks `models` up with its real
        // visibility via `collect_from_item`. Omitting the
        // declaration here previously worked only because
        // `collect_from_file` registered the child's mod-symbol
        // itself — a redundant push that races against the parent's
        // record and drove the lib.rs `pub mod X;` → `mod X;` diff
        // surfaced by `rl061_tempfile_diff_observe`.
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "pub mod models;\n")
            .with_file("src/models.rs", "// models\n")
            .build();

        // Add items to different files (can run in parallel)
        let spec1 = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate::models").unwrap(),
            )),
            content: "pub struct User { id: u32 }".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let spec2 = MutationSpec::AddItem {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            content: "fn main() {}".to_string(),
            position: super::super::spec::InsertPosition::Bottom,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec1, spec2]);
        let executor = BlueprintExecutor::new().with_strategy(ExecutionStrategy::Wavefront);
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Wavefront multi-file failed: {:?}",
            exec_result.error
        );

        let models = ctx.test_file("src/models.rs").unwrap();
        assert!(
            models.to_source().unwrap().contains("pub struct User"),
            "User not added to models.rs"
        );

        let lib = ctx.test_file("src/lib.rs").unwrap();
        assert!(
            lib.to_source().unwrap().contains("fn main"),
            "main not added to lib.rs"
        );
    }

    #[test]
    fn test_wavefront_execution_with_dependencies() {
        let mut ctx = ContextBuilder::new()
            .with_file("src/lib.rs", "fn main() {}\n")
            .build();

        // CreateMod handles both mod declaration and file creation
        let spec = MutationSpec::CreateMod {
            target: MutationTargetSymbol::ByPath(Box::new(
                SymbolPath::parse("test_crate").unwrap(),
            )),
            mod_name: "api".to_string(),
            content: "pub fn endpoint() {}".to_string(),
            is_pub: true,
        };

        let blueprint = ParallelBlueprint::from_mutations(vec![spec]);
        let executor = BlueprintExecutor::new().with_strategy(ExecutionStrategy::Wavefront);
        let exec_result = execute_and_sync(&executor, &blueprint, &mut ctx);

        assert!(
            exec_result.success,
            "Wavefront with deps failed: {:?}",
            exec_result.error
        );

        // Verify dependency order was respected
        let lib = ctx.test_file("src/lib.rs").unwrap();
        assert!(
            lib.to_source().unwrap().contains("pub mod api;"),
            "Mod not added to lib"
        );

        // Check api file (file style: src/api.rs)
        let api = ctx.test_file("src/api.rs").unwrap();
        assert!(
            api.to_source().unwrap().contains("fn endpoint"),
            "Function not in api"
        );
    }

    #[test]
    fn test_suggest_strategy() {
        // Single mutation → Sequential
        let specs = vec![MutationSpec::AddDerive {
            target: MutationTargetSymbol::ById(dummy_id(1)),
            derives: vec!["Debug".to_string()],
        }];
        let blueprint = ParallelBlueprint::from_mutations(specs);
        assert_eq!(suggest_strategy(&blueprint), ExecutionStrategy::Sequential);

        // Two mutations → Sequential (too few)
        let specs = vec![
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Debug".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Clone".to_string()],
            },
        ];
        let blueprint = ParallelBlueprint::from_mutations(specs);
        assert_eq!(suggest_strategy(&blueprint), ExecutionStrategy::Sequential);

        // Multiple independent mutations → Wavefront
        let specs = vec![
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Debug".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Clone".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Default".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Hash".to_string()],
            },
        ];
        let blueprint = ParallelBlueprint::from_mutations(specs);
        assert_eq!(suggest_strategy(&blueprint), ExecutionStrategy::Wavefront);
    }

    #[test]
    #[ignore = "V1 path disabled - needs V2 migration"]
    fn test_sequential_and_wavefront_produce_same_result() {
        let code = r#"
struct A {}
struct B {}
struct C {}
"#;

        let specs = vec![
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Debug".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Clone".to_string()],
            },
            MutationSpec::AddDerive {
                target: MutationTargetSymbol::ById(dummy_id(1)),
                derives: vec!["Default".to_string()],
            },
        ];
        let blueprint = ParallelBlueprint::from_mutations(specs.clone());

        // Execute with Sequential
        let mut ctx_seq = ContextBuilder::new().with_file("src/lib.rs", code).build();
        let executor_seq = BlueprintExecutor::new().with_strategy(ExecutionStrategy::Sequential);
        let result_seq = execute_and_sync(&executor_seq, &blueprint, &mut ctx_seq);

        // Execute with Wavefront
        let mut ctx_wave = ContextBuilder::new().with_file("src/lib.rs", code).build();
        let executor_wave = BlueprintExecutor::new().with_strategy(ExecutionStrategy::Wavefront);
        let result_wave = execute_and_sync(&executor_wave, &blueprint, &mut ctx_wave);

        // Both should succeed
        assert!(result_seq.success, "Sequential failed");
        assert!(result_wave.success, "Wavefront failed");

        // Both should produce the same output
        let source_seq = ctx_seq
            .test_file("src/lib.rs")
            .unwrap()
            .to_source()
            .unwrap();
        let source_wave = ctx_wave
            .test_file("src/lib.rs")
            .unwrap()
            .to_source()
            .unwrap();

        assert!(source_seq.contains("Debug"), "Sequential missing Debug");
        assert!(source_seq.contains("Clone"), "Sequential missing Clone");
        assert!(source_seq.contains("Default"), "Sequential missing Default");
        assert!(source_wave.contains("Debug"), "Wavefront missing Debug");
        assert!(source_wave.contains("Clone"), "Wavefront missing Clone");
        assert!(source_wave.contains("Default"), "Wavefront missing Default");
    }

    #[test]
    #[ignore = "flaky: µs-level timing comparison depends on CPU load"]
    fn test_execute_v2_without_sync_is_faster() {
        use ryo_analysis::SymbolKind;
        use std::time::Instant;

        // Create a larger context for more realistic benchmark
        let code = r#"
pub struct Config { name: String, value: i32 }
pub struct User { id: u64, name: String, email: String }
pub struct Order { id: u64, user_id: u64, total: f64 }
pub enum Status { Pending, Active, Completed, Failed }
pub trait Processor { fn process(&self); }
impl Processor for Config { fn process(&self) {} }
impl Processor for User { fn process(&self) {} }
"#;

        // Helper to create specs with resolved symbol_ids
        fn create_specs(ctx: &ryo_analysis::AnalysisContext) -> Vec<MutationSpec> {
            let config_id = ctx
                .registry
                .iter()
                .find(|(id, path)| {
                    path.name() == "Config" && ctx.registry.kind(*id) == Some(SymbolKind::Struct)
                })
                .map(|(id, _)| id)
                .expect("Config not found");

            let user_id = ctx
                .registry
                .iter()
                .find(|(id, path)| {
                    path.name() == "User" && ctx.registry.kind(*id) == Some(SymbolKind::Struct)
                })
                .map(|(id, _)| id)
                .expect("User not found");

            vec![
                MutationSpec::AddField {
                    target: MutationTargetSymbol::ById(config_id),
                    field_name: "enabled".to_string(),
                    field_type: "bool".to_string(),
                    visibility: Visibility::Pub,
                },
                MutationSpec::AddDerive {
                    target: MutationTargetSymbol::ById(user_id),
                    derives: vec!["Debug".to_string(), "Clone".to_string()],
                },
            ]
        }

        // Benchmark execute_v2 only (no sync)
        let iterations = 10;
        let mut execute_only_times = Vec::with_capacity(iterations);

        for _ in 0..iterations {
            let mut ctx = ContextBuilder::new().with_file("src/lib.rs", code).build();
            let specs = create_specs(&ctx);
            let blueprint = ParallelBlueprint::from_mutations(specs);
            let executor = BlueprintExecutor::new();

            let t0 = Instant::now();
            let result = executor.execute_v2(&blueprint, &mut ctx);
            execute_only_times.push(t0.elapsed());

            assert!(result.success);
        }

        // Benchmark execute_v2 + sync
        let mut execute_with_sync_times = Vec::with_capacity(iterations);

        for _ in 0..iterations {
            let mut ctx = ContextBuilder::new().with_file("src/lib.rs", code).build();
            let specs = create_specs(&ctx);
            let blueprint = ParallelBlueprint::from_mutations(specs);
            let executor = BlueprintExecutor::new();

            let t0 = Instant::now();
            let result = executor.execute_v2(&blueprint, &mut ctx);
            BlueprintExecutor::sync_files_and_rebuild(&result, &mut ctx).unwrap();
            execute_with_sync_times.push(t0.elapsed());

            assert!(result.success);
        }

        let avg_execute_only: u128 = execute_only_times
            .iter()
            .map(|d| d.as_micros())
            .sum::<u128>()
            / iterations as u128;
        let avg_with_sync: u128 = execute_with_sync_times
            .iter()
            .map(|d| d.as_micros())
            .sum::<u128>()
            / iterations as u128;

        eprintln!(
            "execute_v2 only: {}µs avg, execute_v2 + sync: {}µs avg, sync overhead: {:.1}x",
            avg_execute_only,
            avg_with_sync,
            avg_with_sync as f64 / avg_execute_only as f64
        );

        // execute_v2 without sync should be faster
        assert!(
            avg_execute_only < avg_with_sync,
            "execute_v2 only ({}µs) should be faster than with sync ({}µs)",
            avg_execute_only,
            avg_with_sync
        );
    }

    // ========================================================================
    // ws_file_to_crate_relative_key regression tests (mod ambiguity candidate #1)
    //
    // These pin the strip_prefix / CrateName layer that drives
    // `existing_file_keys` construction inside `sync_files_and_rebuild`.
    // The hypothesis under test: hyphen-named workspace crates (Cargo name
    // `ryo-app`, module name `ryo_app`) round-trip correctly so that
    // `crates/ryo-app/src/storage.rs` collapses to `"src/storage.rs"`.
    //
    // If any of these FAIL, candidate #1 from journal Decided is confirmed:
    // the inline closure misses files for the affected layout, leaving
    // `existing_file_keys` empty and letting the generator emit a duplicate
    // `<mod>/mod.rs` next to the existing flat `<mod>.rs`.
    // ========================================================================

    use ryo_symbol::{CrateLayout, CrateName, WorkspaceFilePath, WorkspaceMetadataProvider};
    use std::path::{Path, PathBuf};

    /// In-test provider that returns a single fixed `CrateLayout` for any
    /// `CrateName`. Only the methods used by `ws_file_to_crate_relative_key`
    /// are meaningfully implemented; the rest panic so accidental reliance is
    /// caught in the test.
    struct FixedLayoutProvider {
        layout: CrateLayout,
    }

    impl WorkspaceMetadataProvider for FixedLayoutProvider {
        fn crate_for_file(&self, _path: &WorkspaceFilePath) -> Option<CrateName> {
            unreachable!("ws_file_to_crate_relative_key does not call crate_for_file");
        }
        fn all_crates(&self) -> Vec<CrateName> {
            vec![]
        }
        fn crate_root(&self, _crate_name: &CrateName) -> Option<PathBuf> {
            None
        }
        fn workspace_root(&self) -> &Path {
            Path::new("/workspace")
        }
        fn crate_layout(&self, _crate_name: &CrateName) -> Option<CrateLayout> {
            Some(self.layout.clone())
        }
    }

    #[test]
    fn ws_key_root_layout_flat_file() {
        let wfp = WorkspaceFilePath::new_for_test("src/storage.rs", "/workspace", "test_crate");
        let provider = FixedLayoutProvider {
            layout: CrateLayout::Root,
        };
        assert_eq!(
            ws_file_to_crate_relative_key(&wfp, &provider).as_deref(),
            Some("src/storage.rs")
        );
    }

    #[test]
    fn ws_key_in_crates_hyphen_name_flat_file() {
        // The exact scenario from journal candidate #1:
        // - Cargo name "ryo-app" (hyphen)
        // - Module name "ryo_app" (the form `wfp.crate_name()` carries)
        // - On-disk path crates/ryo-app/src/storage.rs
        // - Expected crate-relative key "src/storage.rs"
        let wfp = WorkspaceFilePath::new_for_test(
            "crates/ryo-app/src/storage.rs",
            "/workspace",
            "ryo_app",
        );
        let provider = FixedLayoutProvider {
            layout: CrateLayout::InCrates {
                crate_dir_name: "ryo-app".to_string(),
            },
        };
        assert_eq!(
            ws_file_to_crate_relative_key(&wfp, &provider).as_deref(),
            Some("src/storage.rs"),
            "Hyphen-named InCrates layout must strip 'crates/ryo-app/' prefix \
             so the generator's with_existing_files sees the flat src/storage.rs"
        );
    }

    #[test]
    fn ws_key_in_crates_hyphen_name_nested_module() {
        let wfp = WorkspaceFilePath::new_for_test(
            "crates/ryo-app/src/discover/service.rs",
            "/workspace",
            "ryo_app",
        );
        let provider = FixedLayoutProvider {
            layout: CrateLayout::InCrates {
                crate_dir_name: "ryo-app".to_string(),
            },
        };
        assert_eq!(
            ws_file_to_crate_relative_key(&wfp, &provider).as_deref(),
            Some("src/discover/service.rs")
        );
    }

    #[test]
    fn ws_key_custom_prefix_layout() {
        let wfp = WorkspaceFilePath::new_for_test("packages/core/src/lib.rs", "/workspace", "core");
        let provider = FixedLayoutProvider {
            layout: CrateLayout::Custom {
                prefix: PathBuf::from("packages/core"),
            },
        };
        assert_eq!(
            ws_file_to_crate_relative_key(&wfp, &provider).as_deref(),
            Some("src/lib.rs")
        );
    }

    #[test]
    fn ws_key_layout_lookup_failure_returns_none() {
        // Provider that always fails to resolve the layout → caller drops the entry.
        struct NoLayoutProvider;
        impl WorkspaceMetadataProvider for NoLayoutProvider {
            fn crate_for_file(&self, _p: &WorkspaceFilePath) -> Option<CrateName> {
                None
            }
            fn all_crates(&self) -> Vec<CrateName> {
                vec![]
            }
            fn crate_root(&self, _c: &CrateName) -> Option<PathBuf> {
                None
            }
            fn workspace_root(&self) -> &Path {
                Path::new("/workspace")
            }
            fn crate_layout(&self, _c: &CrateName) -> Option<CrateLayout> {
                None
            }
        }
        let wfp = WorkspaceFilePath::new_for_test(
            "crates/ryo-app/src/storage.rs",
            "/workspace",
            "ryo_app",
        );
        assert_eq!(ws_file_to_crate_relative_key(&wfp, &NoLayoutProvider), None);
    }
}