algocline_app/service/pkg/

install.rs

//! `pkg_install` — install a package from a Git URL or local path.

use std::path::{Path, PathBuf};

use super::super::alc_toml::{
    add_package_entry, load_alc_toml_document, save_alc_toml, PackageDep,
};
use super::super::hub;
use super::super::lockfile::{load_lockfile, save_lockfile, LockFile, LockPackage};
use super::super::manifest;
use super::super::path::{copy_dir, ContainedPath};
use super::super::resolve::{
    install_scenarios_from_dir, packages_dir, scenarios_dir, DirEntryFailures, AUTO_INSTALL_SOURCES,
};
use super::super::source::PackageSource;
use super::super::{AppService, ProjectFilesError};

/// Explicit install dispatch. Carries exactly the information `pkg_install`
/// needs after classification so that downstream code does not re-classify
/// a string (which is racy: the local directory may disappear between the
/// caller's check and the installer's check).
#[derive(Debug, Clone)]
pub(crate) enum InstallSource {
    /// Copy from a local directory (absolute path).
    LocalPath(PathBuf),
    /// Clone from a Git URL (already normalized with scheme or `git@`).
    GitUrl(String),
}

/// Classify a caller-provided `url` string into an [`InstallSource`].
///
/// Must stay consistent with [`super::super::source::infer_from_legacy_source_string`]:
/// an absolute-path-*shaped* string maps to [`InstallSource::LocalPath`]
/// (matching `PackageSource::Installed`), everything else maps to a normalized
/// Git URL. Classification is deliberately syntactic — no filesystem probes.
/// Rationale: a dir that is_absolute but currently missing used to fall through
/// to the Git arm and produce `https:///abs/path`, which git rejects with
/// `unable to find remote helper for 'https'`. Keeping the classification
/// syntactic gives `install_from_local_path` a chance to surface a diagnostic
/// "Failed to read source dir" error instead.
fn classify_install_url(url: &str) -> InstallSource {
    let local_path = Path::new(url);
    if local_path.is_absolute() {
        return InstallSource::LocalPath(local_path.to_path_buf());
    }

    InstallSource::GitUrl(prefix_git_scheme_if_missing(url))
}

/// Prepend `https://` to a Git remote-style string that lacks a scheme.
///
/// Accepts `http://`, `https://`, `file://`, and `git@` prefixes as-is; any
/// other input (e.g. bare `github.com/a/b`) is prefixed with `https://`.
/// Shared between `classify_install_url` (install path) and
/// `pkg::repair::normalize_git_url` (repair path) — both need the same
/// normalization when routing an already-decided Git URL through `git clone`.
pub(super) fn prefix_git_scheme_if_missing(url: &str) -> String {
    if url.starts_with("http://")
        || url.starts_with("https://")
        || url.starts_with("file://")
        || url.starts_with("git@")
    {
        url.to_string()
    } else {
        format!("https://{url}")
    }
}

impl AppService {
    /// Install a package from a Git URL or local path (string-typed, public MCP API).
    ///
    /// Classifies `url` via [`classify_install_url`] then delegates to
    /// [`AppService::pkg_install_typed`]. Callers that already hold a
    /// classified [`InstallSource`] (e.g. `pkg_repair`) should call the
    /// typed API directly to avoid re-classifying a stale string.
    pub async fn pkg_install(
        &self,
        url: String,
        name: Option<String>,
        force: Option<bool>,
    ) -> Result<String, String> {
        let source = classify_install_url(&url);
        self.pkg_install_typed(source, name, force).await
    }

    /// Typed install dispatch. Does no string re-classification; branches
    /// explicitly on the already-classified [`InstallSource`].
    pub(crate) async fn pkg_install_typed(
        &self,
        source: InstallSource,
        name: Option<String>,
        force: Option<bool>,
    ) -> Result<String, String> {
        let app_dir = self.log_config.app_dir();
        let pkg_dir = packages_dir(&app_dir);
        std::fs::create_dir_all(&pkg_dir)
            .map_err(|e| ProjectFilesError::PackagesDir {
                path: pkg_dir.display().to_string(),
                source: e,
            })
            .map_err(|e| e.to_string())?;

        let git_url = match source {
            InstallSource::LocalPath(path) => {
                return self.install_from_local_path(&path, &pkg_dir, name).await;
            }
            InstallSource::GitUrl(u) => u,
        };
        // `url` is the recorded form used for manifest/hub. Normalization
        // happens in `classify_install_url`, so this is already the
        // scheme-prefixed form (e.g. `https://github.com/x`).
        let url = git_url.clone();

        let staging = tempfile::tempdir().map_err(|e| format!("Failed to create temp dir: {e}"))?;

        // Bound `git clone` wall time. Without this a misconfigured remote
        // (auth prompt, unreachable host, slow network) can block the MCP
        // tool call indefinitely. 60s covers normal shallow clones of our
        // bundled-packages-sized repos with margin.
        let clone_future = tokio::process::Command::new("git")
            .args([
                "clone",
                "--depth",
                "1",
                &git_url,
                &staging.path().to_string_lossy(),
            ])
            .output();
        let output = tokio::time::timeout(std::time::Duration::from_secs(60), clone_future)
            .await
            .map_err(|_| format!("git clone timed out after 60s: {git_url}"))?
            .map_err(|e| format!("Failed to run git: {e}"))?;

        if !output.status.success() {
            let stderr = String::from_utf8_lossy(&output.stderr);
            return Err(format!("git clone failed: {stderr}"));
        }

        // Remove .git dir from staging (best-effort; absent .git would be
        // surprising but not fatal).
        if let Err(e) = std::fs::remove_dir_all(staging.path().join(".git")) {
            if e.kind() != std::io::ErrorKind::NotFound {
                tracing::warn!(
                    "pkg_install: failed to strip .git from staging {}: {e}",
                    staging.path().display()
                );
            }
        }

        // Collection mode: scan for subdirs containing init.lua
        {
            if name.is_some() {
                return Err("The 'name' parameter is no longer supported. \
                     Single-package install mode was removed in v0.36.0; \
                     package names are derived from subdirectory names in collection layout \
                     (<repo>/<name>/init.lua)."
                    .to_string());
            }

            let force = force.unwrap_or(false);
            let mut installed = Vec::new();
            let mut skipped = Vec::new();
            // Dev symlinks (pkg_link scope=global) previously blocked collection
            // install with a hard `ContainedPath::child` error because their
            // `canonicalize` target lives outside the packages base. Collect
            // them as a distinct "symlink-skipped" bucket, skip install for the
            // affected pkg, and continue with the rest — the user runs
            // `pkg_unlink <name>` if they want the git-clone copy to win.
            let mut skipped_symlinks: Vec<String> = Vec::new();

            let entries = std::fs::read_dir(staging.path())
                .map_err(|e| format!("Failed to read staging dir: {e}"))?;

            for entry in entries {
                let entry = entry.map_err(|e| format!("Failed to read entry: {e}"))?;
                let path = entry.path();
                if !path.is_dir() {
                    continue;
                }
                if !path.join("init.lua").exists() {
                    continue;
                }
                let pkg_name = entry.file_name().to_string_lossy().to_string();

                // Pre-check: a legitimate `pkg_link` symlink at the destination
                // points outside the packages base, which would fail
                // `ContainedPath::child`'s canonicalize-escape check and abort
                // the whole batch. Detect the symlink first and route to
                // `skipped_symlinks` so the install proceeds for other pkgs.
                let candidate = pkg_dir.join(&pkg_name);
                if candidate
                    .symlink_metadata()
                    .map(|m| m.file_type().is_symlink())
                    .unwrap_or(false)
                {
                    tracing::warn!(
                        "pkg_install: skipping '{pkg_name}' — destination is an existing symlink \
                         (likely a `pkg_link` dev link); run `pkg_unlink {pkg_name}` to replace it"
                    );
                    skipped_symlinks.push(pkg_name);
                    continue;
                }

                // Go through ContainedPath::child to block path traversal from
                // a malicious subdir name (`..`, `foo/../bar`) — the staging
                // dir is untrusted input in the general case.
                let dest = ContainedPath::child(&pkg_dir, &pkg_name)?;
                if dest.as_ref().exists() {
                    if !force {
                        skipped.push(pkg_name);
                        continue;
                    }
                    // force=true: remove existing tree before overwriting
                    std::fs::remove_dir_all(dest.as_ref()).map_err(|e| {
                        format!("Failed to remove existing package '{pkg_name}': {e}")
                    })?;
                }
                copy_dir(&path, dest.as_ref())
                    .map_err(|e| format!("Failed to copy package '{pkg_name}': {e}"))?;
                installed.push(pkg_name);
            }

            // Import bundled cards from each package's cards/ subdirectory.
            let mut cards_installed: Vec<String> = Vec::new();
            for pkg_name in installed.iter().chain(skipped.iter()) {
                let cards_subdir = staging.path().join(pkg_name).join("cards");
                if cards_subdir.is_dir() {
                    let imported = self.import_pkg_bundled_cards(pkg_name, &cards_subdir);
                    cards_installed.extend(imported);
                }
            }

            // Install bundled scenarios only when an explicit `scenarios/` subdir exists.
            let scenarios_subdir = staging.path().join("scenarios");
            let mut scenarios_installed: Vec<String> = Vec::new();
            let mut scenarios_failures: DirEntryFailures = Vec::new();
            if scenarios_subdir.is_dir() {
                let sc_dir = scenarios_dir(&app_dir);
                std::fs::create_dir_all(&sc_dir)
                    .map_err(|e| format!("Failed to create scenarios dir: {e}"))?;
                {
                    if let Ok(result) = install_scenarios_from_dir(&scenarios_subdir, &sc_dir) {
                        if let Ok(parsed) = serde_json::from_str::<serde_json::Value>(&result) {
                            if let Some(arr) = parsed.get("installed").and_then(|v| v.as_array()) {
                                scenarios_installed = arr
                                    .iter()
                                    .filter_map(|v| v.as_str().map(String::from))
                                    .collect();
                            }
                            if let Some(arr) = parsed.get("failures").and_then(|v| v.as_array()) {
                                scenarios_failures = arr
                                    .iter()
                                    .filter_map(|v| v.as_str().map(String::from))
                                    .collect();
                            }
                        }
                    }
                }
            }

            if installed.is_empty() && skipped.is_empty() && skipped_symlinks.is_empty() {
                return Err(
                    "Expected */init.lua (collection layout). Single-package mode (init.lua at root) was removed in v0.36.0."
                        .to_string(),
                );
            }

            // Record in manifest + hub registry.
            let mut storage_warnings: Vec<String> = Vec::new();
            if let Err(e) = manifest::record_install_batch(
                &app_dir,
                &installed,
                super::super::source::PackageSource::Git {
                    url: url.clone(),
                    rev: None,
                },
            ) {
                storage_warnings.push(format!("manifest record_install_batch: {e}"));
            }
            if let Err(e) = hub::register_source(&app_dir, &url, "pkg_install") {
                storage_warnings.push(format!("hub register_source: {e}"));
            }

            // Update alc.toml + alc.lock if project root is found.
            // Fatal errors from the update (e.g. alc.toml load failure) are
            // degraded to warnings — the pkg copy already succeeded.
            let project_files_warnings =
                match self.update_project_files_for_install(&installed).await {
                    Ok(ws) => ws,
                    Err(e) => vec![e.to_string()],
                };

            let mut response = serde_json::json!({
                "installed": installed,
                "skipped": skipped,
                "skipped_symlinks": skipped_symlinks,
                "cards_installed": cards_installed,
                "scenarios_installed": scenarios_installed,
                "scenarios_failures": scenarios_failures,
                "mode": "collection",
            });
            if let Some(tp) = super::super::resolve::types_stub_path(&app_dir) {
                response["types_path"] = serde_json::Value::String(tp);
            }
            if let Some(tp) = super::super::resolve::alc_shapes_types_stub_path(&app_dir) {
                response["alc_shapes_types_path"] = serde_json::Value::String(tp);
            }
            if !storage_warnings.is_empty() {
                response["storage_warnings"] = serde_json::json!(storage_warnings);
            }
            if !project_files_warnings.is_empty() {
                response["project_files_warnings"] = serde_json::json!(project_files_warnings);
            }
            Ok(response.to_string())
        }
    }

    /// Install from a local directory path (supports dirty/uncommitted files).
    async fn install_from_local_path(
        &self,
        source: &Path,
        pkg_dir: &Path,
        name: Option<String>,
    ) -> Result<String, String> {
        let app_dir = self.log_config.app_dir();
        // Reject a missing source dir up front. Without this check, a missing
        // path surfaces as a misleading scan error rather than a clear
        // diagnostic.
        if !source.exists() {
            return Err(format!(
                "Source directory does not exist: {}",
                source.display()
            ));
        }

        // Collection mode: scan for subdirs containing init.lua
        {
            if name.is_some() {
                return Err("The 'name' parameter is no longer supported. \
                     Single-package install mode was removed in v0.36.0; \
                     package names are derived from subdirectory names in collection layout \
                     (<repo>/<name>/init.lua)."
                    .to_string());
            }

            let mut installed = Vec::new();
            let mut updated = Vec::new();

            let entries =
                std::fs::read_dir(source).map_err(|e| format!("Failed to read source dir: {e}"))?;

            for entry in entries {
                let entry = entry.map_err(|e| format!("Failed to read entry: {e}"))?;
                let path = entry.path();
                if !path.is_dir() || !path.join("init.lua").exists() {
                    continue;
                }
                let pkg_name = entry.file_name().to_string_lossy().to_string();
                // Guard against traversal-shaped subdir names from an
                // untrusted source tree, matching the git-clone branch.
                let dest = ContainedPath::child(pkg_dir, &pkg_name)?;
                let existed = dest.as_ref().exists();
                if existed {
                    if let Err(e) = std::fs::remove_dir_all(dest.as_ref()) {
                        tracing::warn!(
                            "pkg_install: failed to remove existing dest {} before overwrite: {e}",
                            dest.as_ref().display()
                        );
                    }
                }
                copy_dir(&path, dest.as_ref())
                    .map_err(|e| format!("Failed to copy package '{pkg_name}': {e}"))?;
                if let Err(e) = std::fs::remove_dir_all(dest.as_ref().join(".git")) {
                    if e.kind() != std::io::ErrorKind::NotFound {
                        tracing::warn!(
                            "pkg_install: failed to strip .git from {}: {e}",
                            dest.as_ref().display()
                        );
                    }
                }
                if existed {
                    updated.push(pkg_name);
                } else {
                    installed.push(pkg_name);
                }
            }

            if installed.is_empty() && updated.is_empty() {
                return Err(
                    "Expected */init.lua (collection layout). Single-package mode (init.lua at root) was removed in v0.36.0."
                        .to_string(),
                );
            }

            // Import bundled cards from each package's cards/ subdirectory.
            let mut cards_installed: Vec<String> = Vec::new();
            for pkg_name in installed.iter().chain(updated.iter()) {
                let cards_subdir = source.join(pkg_name).join("cards");
                if cards_subdir.is_dir() {
                    let imported = self.import_pkg_bundled_cards(pkg_name, &cards_subdir);
                    cards_installed.extend(imported);
                }
            }

            // Record in manifest. Batch local-path installs use
            // `Path { path }` to preserve the source location in the typed
            // form so `pkg_repair` can re-copy from the same source and
            // `pkg_list` can show where the bytes came from. Storage
            // failures surface via `storage_warnings`.
            let source_str = source.display().to_string();
            let all_names: Vec<String> = installed.iter().chain(updated.iter()).cloned().collect();
            let mut storage_warnings: Vec<String> = Vec::new();
            if let Err(e) = manifest::record_install_batch(
                &app_dir,
                &all_names,
                super::super::source::PackageSource::Path {
                    path: source_str.clone(),
                },
            ) {
                storage_warnings.push(format!("manifest record_install_batch: {e}"));
            }
            if let Err(e) = hub::register_source(&app_dir, &source_str, "pkg_install") {
                storage_warnings.push(format!("hub register_source: {e}"));
            }

            // Update alc.toml + alc.lock for newly installed packages.
            // Fatal errors from the update (e.g. alc.toml load failure) are
            // degraded to warnings — the pkg copy already succeeded.
            let project_files_warnings =
                match self.update_project_files_for_install(&installed).await {
                    Ok(ws) => ws,
                    Err(e) => vec![e.to_string()],
                };

            let mut response = serde_json::json!({
                "installed": installed,
                "updated": updated,
                "cards_installed": cards_installed,
                "mode": "local_collection",
            });
            if let Some(tp) = super::super::resolve::types_stub_path(&app_dir) {
                response["types_path"] = serde_json::Value::String(tp);
            }
            if let Some(tp) = super::super::resolve::alc_shapes_types_stub_path(&app_dir) {
                response["alc_shapes_types_path"] = serde_json::Value::String(tp);
            }
            if !storage_warnings.is_empty() {
                response["storage_warnings"] = serde_json::json!(storage_warnings);
            }
            if !project_files_warnings.is_empty() {
                response["project_files_warnings"] = serde_json::json!(project_files_warnings);
            }
            Ok(response.to_string())
        }
    }

    /// After a successful cache install, update `alc.toml` and `alc.lock` if a project
    /// root (containing `alc.toml`) is found.  Load failures are surfaced as `Err`;
    /// save failures are collected into the returned warnings vec.  Lock acquisition
    /// failures are degraded to a single warning so the install result stays `Ok`.
    async fn update_project_files_for_install(
        &self,
        names: &[String],
    ) -> Result<Vec<String>, ProjectFilesError> {
        let root = match self.resolve_root(None) {
            Some(r) => r,
            None => return Ok(Vec::new()), // No project root → skip (current-compat)
        };

        // Resolve per-package versions *before* taking the lock, so the
        // lock-held critical section contains only synchronous I/O
        // (load → mutate → save). `fetch_pkg_version` dispatches into the
        // shared Lua executor and may await arbitrarily long.
        let mut resolved: Vec<(String, Option<String>)> = Vec::with_capacity(names.len());
        for name in names {
            let version = self.fetch_pkg_version(name).await;
            resolved.push((name.clone(), version));
        }

        // Guard the alc.toml / alc.lock load→modify→save against overlapping
        // `pkg_install` calls that target the same project root. Without this
        // advisory lock two concurrent installs can each load the old state,
        // apply their own mutation, and race to save — the later writer
        // silently overwrites the earlier's entry.
        //
        // `From<LockError> for ProjectFilesError` is implemented via `#[from]` on
        // the `Lock` variant in `service/error.rs`, so lock acquisition errors
        // are injected as typed `ProjectFilesError::Lock` values — no `.to_string()`
        // flattening at this call site.
        let lock_path = project_files_lock_path(&root);
        super::super::lock::with_exclusive_lock(&lock_path, move || {
            let mut warnings: Vec<String> = Vec::new();

            // Load alc.toml document (preserving comments/formatting).
            // file absent (Ok(None)) is a normal skip; corruption (Err) is fatal.
            let mut doc = match load_alc_toml_document(&root) {
                Ok(Some(d)) => d,
                Ok(None) => return Ok(Vec::new()), // alc.toml not found → skip
                Err(e) => return Err(ProjectFilesError::AlcTomlLoad(e)),
            };

            // Load or create alc.lock.
            // file absent (Ok(None)) → start with empty lockfile (normal init path).
            // corruption (Err) is fatal — same policy as alc.toml.
            let mut lock = match load_lockfile(&root) {
                Ok(Some(l)) => l,
                Ok(None) => LockFile {
                    version: 1,
                    packages: Vec::new(),
                },
                Err(e) => return Err(ProjectFilesError::AlcLockLoad(e)),
            };

            for (name, version) in &resolved {
                // Add to alc.toml (no-op if already present).
                add_package_entry(&mut doc, name, &PackageDep::Version("*".to_string()));
                // Upsert into alc.lock with the pre-resolved version.
                upsert_lock_entry(
                    &mut lock,
                    name.clone(),
                    version.clone(),
                    PackageSource::Installed,
                );
            }

            // Save failures are non-fatal: collect as warnings so the caller
            // can surface them in the response JSON. Both saves are attempted
            // independently (one failure does not skip the other).
            if let Err(e) = save_alc_toml(&root, &doc) {
                warnings.push(ProjectFilesError::AlcTomlSave(e).to_string());
            }
            if let Err(e) = save_lockfile(&root, &lock) {
                warnings.push(ProjectFilesError::AlcLockSave(e).to_string());
            }
            Ok(warnings)
        })
    }

    /// Fetch package version via `eval_simple` (best-effort; returns `None` on failure).
    async fn fetch_pkg_version(&self, name: &str) -> Option<String> {
        if !is_safe_pkg_name(name) {
            return None;
        }
        let code = format!(
            r#"package.loaded["{name}"] = nil
local pkg = require("{name}")
return (pkg.meta or {{}}).version"#
        );
        match self.executor.eval_simple(code).await {
            Ok(serde_json::Value::String(v)) if !v.is_empty() => Some(v),
            _ => None,
        }
    }

    /// Install all bundled sources (collection layout).
    pub(in crate::service) async fn auto_install_bundled_packages(&self) -> Result<(), String> {
        let mut errors: Vec<String> = Vec::new();
        for url in AUTO_INSTALL_SOURCES {
            tracing::info!("auto-installing from {url}");
            if let Err(e) = self.pkg_install(url.to_string(), None, None).await {
                tracing::warn!("failed to auto-install from {url}: {e}");
                errors.push(format!("{url}: {e}"));
            }
        }
        // Fail only if ALL sources failed
        if errors.len() == AUTO_INSTALL_SOURCES.len() {
            return Err(format!(
                "Failed to auto-install bundled packages: {}",
                errors.join("; ")
            ));
        }
        Ok(())
    }
}

// ─── Helpers ────────────────────────────────────────────────────────────────

/// Path to the advisory lock file guarding `alc.toml` + `alc.lock` updates
/// within a project root. The lock file sits alongside the project files so
/// two processes working in the same checkout serialize on the same path.
///
/// The filename is deliberately distinct from `alc.lock` itself — the latter
/// is the dependency lockfile users read, while `.alc-install.lock` is an
/// internal flock companion. Consumers who share a project tree via `.gitignore`
/// should ignore it alongside other temp files; algocline does not add it
/// automatically today.
fn project_files_lock_path(root: &std::path::Path) -> std::path::PathBuf {
    root.join(".alc-install.lock")
}

/// Returns `true` iff `name` is safe to interpolate into a Lua source string.
fn is_safe_pkg_name(name: &str) -> bool {
    !name.is_empty()
        && name
            .bytes()
            .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-')
}

/// Insert or update a `LockPackage` entry in the lockfile.
fn upsert_lock_entry(
    lock: &mut LockFile,
    name: String,
    version: Option<String>,
    source: PackageSource,
) {
    if let Some(existing) = lock.packages.iter_mut().find(|p| p.name == name) {
        existing.version = version;
        existing.source = source;
    } else {
        lock.packages.push(LockPackage {
            name,
            version,
            source,
        });
    }
}

#[cfg(test)]
mod tests {
    use super::super::super::alc_toml::save_alc_toml;
    use super::super::super::lock::with_exclusive_lock;
    use super::super::super::lockfile::save_lockfile;
    use super::*;

    // ── (b) closure load failure → fatal Err propagated through Result ────────

    /// When `alc.toml` exists but is corrupt (unparseable TOML), the closure
    /// must return `Err(...)` rather than silently skipping.
    #[test]
    fn load_alc_toml_corrupt_yields_fatal_err() {
        let tmp = tempfile::tempdir().unwrap();
        let root = tmp.path();
        // Write corrupt TOML
        std::fs::write(root.join("alc.toml"), b"[[not valid toml = {").unwrap();

        let lock_path = root.join(".alc-install.lock");
        let result: Result<Vec<String>, String> =
            with_exclusive_lock(&lock_path, move || match load_alc_toml_document(root) {
                Ok(Some(_d)) => Ok(Vec::new()),
                Ok(None) => Ok(Vec::new()),
                Err(e) => Err(format!("alc.toml load: {e}")),
            });

        assert!(
            result.is_err(),
            "Expected Err on corrupt alc.toml, got: {result:?}"
        );
        let msg = result.unwrap_err();
        assert!(
            msg.contains("alc.toml load:"),
            "Error should contain 'alc.toml load:', got: {msg}"
        );
    }

    /// When `alc.lock` exists but is corrupt, the closure must return `Err`.
    #[test]
    fn load_alc_lock_corrupt_yields_fatal_err() {
        let tmp = tempfile::tempdir().unwrap();
        let root = tmp.path();
        // Write a valid alc.toml so it passes the first check
        std::fs::write(root.join("alc.toml"), b"[packages]\n").unwrap();
        // Write corrupt alc.lock
        std::fs::write(root.join("alc.lock"), b"version = 999\n[[package]]\n").unwrap();

        let lock_path = root.join(".alc-install.lock");
        let result: Result<Vec<String>, String> = with_exclusive_lock(&lock_path, move || {
            let _doc = match load_alc_toml_document(root) {
                Ok(Some(d)) => d,
                Ok(None) => return Ok(Vec::new()),
                Err(e) => return Err(format!("alc.toml load: {e}")),
            };
            match load_lockfile(root) {
                Ok(Some(_l)) => Ok(Vec::new()),
                Ok(None) => Ok(Vec::new()),
                Err(e) => Err(format!("alc.lock load: {e}")),
            }
        });

        assert!(
            result.is_err(),
            "Expected Err on corrupt alc.lock, got: {result:?}"
        );
        let msg = result.unwrap_err();
        assert!(
            msg.contains("alc.lock load:"),
            "Error should contain 'alc.lock load:', got: {msg}"
        );
    }

    // ── (a) closure save failure → warnings collected (not fatal) ─────────────

    /// When alc.toml exists and is valid but the save path is non-writable,
    /// the failure should appear in the returned warnings vec (not as Err).
    #[test]
    fn save_failure_produces_warning_not_fatal_err() {
        let tmp = tempfile::tempdir().unwrap();
        let root = tmp.path();
        // Write a minimal valid alc.toml
        std::fs::write(root.join("alc.toml"), b"[packages]\n").unwrap();

        // Place a regular FILE at bad_root so create_dir_all(bad_root) fails
        // (cannot create a directory at a path occupied by a non-directory).
        // Both save_alc_toml and save_lockfile call create_dir_all(parent)
        // where parent = bad_root, so the file blocker triggers the failure.
        let bad_root = root.join("blocked_subdir");
        std::fs::write(&bad_root, b"this is a file, not a dir").unwrap();

        let lock_path = root.join(".alc-install.lock");
        let root_owned = root.to_path_buf();
        let bad_root_owned = bad_root.clone();
        let result: Result<Vec<String>, String> = with_exclusive_lock(&lock_path, move || {
            let mut warnings: Vec<String> = Vec::new();
            let doc = match load_alc_toml_document(&root_owned) {
                Ok(Some(d)) => d,
                Ok(None) => return Ok(Vec::new()),
                Err(e) => return Err(format!("alc.toml load: {e}")),
            };
            if let Err(e) = save_alc_toml(&bad_root_owned, &doc) {
                warnings.push(format!("alc.toml save: {e}"));
            }
            let lock = LockFile {
                version: 1,
                packages: Vec::new(),
            };
            if let Err(e) = save_lockfile(&bad_root_owned, &lock) {
                warnings.push(format!("alc.lock save: {e}"));
            }
            Ok(warnings)
        });

        assert!(
            result.is_ok(),
            "Expected Ok even with save failures, got: {result:?}"
        );
        let warnings = result.unwrap();
        assert!(
            !warnings.is_empty(),
            "Expected at least one save warning, got empty warnings"
        );
        assert!(
            warnings.iter().any(|w| w.contains("alc.toml save:")),
            "Expected 'alc.toml save:' warning, got: {warnings:?}"
        );
    }

    // ── (c) caller transforms fatal Err into project_files_warnings ───────────

    /// The caller pattern `match result { Ok(ws) => ws, Err(e) => vec![e] }`
    /// must convert a fatal Err into a single-element warnings vec so the
    /// install response remains Ok.
    #[test]
    fn caller_degrades_fatal_err_to_project_files_warnings() {
        // Simulate the update returning a fatal Err (e.g. alc.toml load failure)
        let update_result: Result<Vec<String>, String> =
            Err("alc.toml load: TOML parse error at line 1".to_string());

        // This is the exact pattern used in each of the 4 callers.
        let project_files_warnings = match update_result {
            Ok(ws) => ws,
            Err(e) => vec![e],
        };

        assert_eq!(project_files_warnings.len(), 1);
        assert!(
            project_files_warnings[0].contains("alc.toml load:"),
            "Warning should contain the original error message"
        );
    }

    /// When update returns Ok with warnings, they pass through unchanged.
    #[test]
    fn caller_passes_through_ok_warnings() {
        let update_result: Result<Vec<String>, String> = Ok(vec![
            "alc.toml save: permission denied".to_string(),
            "alc.lock save: no space left".to_string(),
        ]);

        let project_files_warnings = match update_result {
            Ok(ws) => ws,
            Err(e) => vec![e],
        };

        assert_eq!(project_files_warnings.len(), 2);
    }

    /// When update returns Ok with no warnings, the empty vec is gated out
    /// of the response JSON (mirrors storage_warnings convention).
    #[test]
    fn empty_warnings_are_not_added_to_response() {
        let update_result: Result<Vec<String>, String> = Ok(Vec::new());

        let project_files_warnings = match update_result {
            Ok(ws) => ws,
            Err(e) => vec![e],
        };

        // Gate mirrors the `if !project_files_warnings.is_empty()` check in callers
        let mut response = serde_json::json!({ "installed": ["mypkg"], "mode": "collection" });
        if !project_files_warnings.is_empty() {
            response["project_files_warnings"] = serde_json::json!(project_files_warnings);
        }

        assert!(
            response.get("project_files_warnings").is_none(),
            "project_files_warnings should not appear when warnings are empty"
        );
    }

    // ── Helpers ──────────────────────────────────────────────────────────────

    #[test]
    fn upsert_lock_entry_inserts_new_package() {
        let mut lock = LockFile {
            version: 1,
            packages: Vec::new(),
        };
        upsert_lock_entry(
            &mut lock,
            "mypkg".to_string(),
            Some("1.0.0".to_string()),
            PackageSource::Installed,
        );
        assert_eq!(lock.packages.len(), 1);
        assert_eq!(lock.packages[0].name, "mypkg");
        assert_eq!(lock.packages[0].version, Some("1.0.0".to_string()));
    }

    #[test]
    fn upsert_lock_entry_updates_existing_package() {
        let mut lock = LockFile {
            version: 1,
            packages: Vec::new(),
        };
        upsert_lock_entry(
            &mut lock,
            "mypkg".to_string(),
            Some("1.0.0".to_string()),
            PackageSource::Installed,
        );
        upsert_lock_entry(
            &mut lock,
            "mypkg".to_string(),
            Some("2.0.0".to_string()),
            PackageSource::Installed,
        );
        assert_eq!(lock.packages.len(), 1);
        assert_eq!(lock.packages[0].version, Some("2.0.0".to_string()));
    }

    #[test]
    fn is_safe_pkg_name_accepts_valid_names() {
        assert!(is_safe_pkg_name("my_pkg"));
        assert!(is_safe_pkg_name("my-pkg"));
        assert!(is_safe_pkg_name("mypkg123"));
    }

    #[test]
    fn is_safe_pkg_name_rejects_invalid_names() {
        assert!(!is_safe_pkg_name(""));
        assert!(!is_safe_pkg_name("my pkg"));
        assert!(!is_safe_pkg_name("../escape"));
        assert!(!is_safe_pkg_name("pkg;rm -rf /"));
    }
}